Skip to content

test(agent): refresh two assertions that predate current retry/fallback behavior - #295

Merged
OmarB97 merged 1 commit into
mainfrom
fix/run-agent-stale-assertions
Aug 2, 2026
Merged

test(agent): refresh two assertions that predate current retry/fallback behavior#295
OmarB97 merged 1 commit into
mainfrom
fix/run-agent-stale-assertions

Conversation

@OmarB97

@OmarB97 OmarB97 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes the two failing tests in tests/run_agent/test_run_agent.py. Both encode
call shapes that recent changes moved past; in both cases I checked the current
behavior is the intended one rather than assuming.

test_codex_content_filter_incomplete_routes_to_policy_fallback

Expected: _try_activate_fallback()
  Actual: _try_activate_fallback(<FailoverReason.content_policy_blocked: 'content_policy_blocked'>)

#269 threads the classified FailoverReason into the fallback switch. The test
predates that and asserted the bare call. Asserting that the reason is
propagated is strictly stronger, and it is what the test's own name
(routes_to_policy_fallback) claims to cover — the test already asserts the
same enum on the hook event two lines below.

test_output_cap_retry_request_pressure_lower_bound

assert 1 == 903

The output-cap retry no longer stops at
min(provider_available, context_length - estimate) - 64. It is then anchored
to output_tokens_that_fit():

_local_fit = output_tokens_that_fit(old_ctx, api_messages)
if _local_fit is not None:
    safe_out = max(1, min(safe_out, _local_fit))

That helper is documented as "the single source of truth for 'how many output
tokens fit'"
, and it reserves the input conservatively (an over-reservation
factor plus pad and window margin) so the value it returns is one the provider
will actually accept. It exists because some servers — notably vLLM — report the
offending input as a max_tokens-dependent lower bound, which makes a naive
shrink-and-retry loop never converge.

This test's scenario is a ~199k-token system prompt in a 200k window, i.e. a
near-full window, so the conservative reservation legitimately binds below the
plain arithmetic and the cap floors at 1. The test re-derived the old formula
and so went stale. The expectation now applies the same clamp the code does.
The floor-at-1 behavior itself already has dedicated coverage in the sibling
test_output_cap_retry_safety_floor_at_one.

Related Issue

No filed issue — surfaced once #286 unblocked uv sync --locked and the Python
test slices ran for the first time since 2026-07-10.

Type of Change

  • ✅ Tests (adding or improving test coverage)

Changes Made

  • tests/run_agent/test_run_agent.py — assert _try_activate_fallback receives FailoverReason.content_policy_blocked; include the output_tokens_that_fit() clamp in the expected retry cap.

How to Test

pytest tests/run_agent/test_run_agent.py -p no:randomly -q

Before: 2 failed, 434 passed. After: 436 passed.

⚠️ Run with an isolated HERMES_HOME. The suite reads the developer's real
~/.hermes/config.yaml, which can diverge from CI.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the full affected file: 436 passed
  • I've added tests for my changes — N/A, this PR repairs existing tests
  • I've tested on my platform: macOS 15 (Darwin 25.6.0), Python 3.11.15

Documentation & Housekeeping

  • I've updated relevant documentation — N/A
  • I've updated cli-config.yaml.example — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md — N/A
  • I've considered cross-platform impact — N/A, test-only
  • I've updated tool descriptions/schemas — N/A

…ck behavior

Both tests encode call shapes that recent changes moved past. Neither is a
production defect — the new behavior is the intended one in both cases.

test_codex_content_filter_incomplete_routes_to_policy_fallback
  #269 threads the classified FailoverReason into the fallback switch, so
  _try_activate_fallback is now called with the reason rather than bare.
  Asserting the reason is propagated is strictly stronger and matches what
  the test name claims to cover.

test_output_cap_retry_request_pressure_lower_bound
  The output-cap retry is now anchored to output_tokens_that_fit(), the
  documented single source of truth for "how many output tokens fit". It
  reserves the input conservatively so the returned cap is one the provider
  will accept, which against this test's near-full window (a ~199k-token
  system prompt in a 200k model window) binds below the plain
  min(provider_available, context - estimate) - 64 arithmetic the test
  re-derived. The expectation now applies the same clamp the code does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@OmarB97
OmarB97 merged commit 2e08411 into main Aug 2, 2026
29 of 30 checks passed
OmarB97 added a commit that referenced this pull request Aug 2, 2026
…text window (#300)

output_tokens_that_fit() returned its ``min_output`` floor (1) when the
reserved input filled the window, and both callers consumed that fabricated
1 as a real budget.

The reservation is multiplicative — int(est * 1.2) + 1024 + 512 — so it
exhausts the window once the rough estimate passes ~(ctx - 1536) / 1.2, about
82.7% fill. Past that point:

  * the pre-flight clamp in build_api_kwargs set max_tokens=1 on EVERY request,
    with no provider error involved at all; and
  * the reactive output-cap retry clamped a healthy provider-authoritative cap
    down to 1 — directly contradicting the branch immediately above it, which
    deliberately falls back to the provider's number precisely because "the
    rough local estimate can overshoot the real request size".

Measured against a 200,000-token window: a ~170k-token prompt (85% fill) with
the provider reporting available_tokens=25,000 produced a retry cap of 1
instead of 24,936. At 80% fill the cap is 6,455; at 83% it is 1 — a cliff, not
a degradation.

max_tokens=1 is the worst kind of failure here: the provider accepts it and
returns a single truncated token, so the turn reports success while handing
back output the user cannot use. Nothing fails loudly.

Fix: report None ("no usable cap — leave max_tokens to your own budget logic")
instead of fabricating a floor. Both call sites already guard None correctly,
so neither needed a code change; the reactive one gains a comment so the floor
is not reinstated. Above 82.7% fill the pre-flight clamp now no-ops and the
provider reports its own authoritative budget on the reactive path.

Introduced in #271; the vLLM/deepseek-v4-flash-w2 convergence property that
motivated #271 is unaffected — that regime sits at ~44% fill, where the fit
stays positive and the retry still converges in one step.

Also reverts the test half of #295 for test_output_cap_retry_request_pressure_
lower_bound. That assertion was loosened to match the defect; the original
arithmetic was right and now passes for the right reason. #295's other change
(threading FailoverReason into the fallback switch) is correct and stands.

Tests: 5 cases in tests/test_output_fit_preflight_clamp.py covering the
82.7%-100% fill band at both call sites. The sweep case holds every reported
fit to the documented "server tokenizes ~15% denser" safety property, which a
fabricated floor fails by construction. Verified they fail without the
production change and pass with it.

Co-authored-by: Omar Baradei <omar@kostudios.io>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant