fix: address critic findings on #789 (always-zero num_turns, false docstring claims, coverage holes) - #790
Merged
Merged
Conversation
A critic pass on the merged commit found four defects and three coverage holes. Two of the four are mine asserting things that were not true. **UsageEvent.num_turns was structurally always 0** — the headless turn counter only advances after the whole agent loop returns, so no event emitted DURING a run can see a non-zero value. The harbor adapter then promoted that zero into killed trials' metrics, so the trial that did the most work reported num_turns: 0. That is the same floor-biased-low-on-the- longest-trials defect #789 was written to remove, relocated from tokens to turns, and worse than the old behaviour, which omitted the field rather than asserting a false zero. Replaced with `assistant_messages`, which counts round trips and actually increments mid-run. Deliberately NOT mapped onto num_turns anywhere: an agent-loop turn can span several round trips, so conflating them would re-introduce the same lie with a different number. The adapter test that should have caught this used num_turns: 4 — a value the producer cannot emit. **The "cached permanently / for the rest of the session" rationale was false.** _FAILURE_TTL_SECONDS = 90.0 and _cache_get expires entries; the docstring I added contradicted the one 350 lines above that exists to explain the TTL. The retry is still right — an empty 200 degrades that image for up to 90s, across however many turns fall in that window, which was enough to lose gcode-to-text. Rationale corrected to the real claim. **ContentProgressDeadline's docstring documented the design that was rejected** — `except Empty: deadline.check()` — and asserted a safety property ("can only fire while the queue is empty") that is false. Both call sites deliberately do the opposite. A maintainer following the docstring would have reintroduced the hang; mutating to it hangs. **No escape hatch, and one env var governed two wires.** The deadline read CLAUDE_STREAM_FIRST_EVENT_TIMEOUT_MS, so raising it to work around a false positive here also loosened the Anthropic watchdog, and no value could disable it. Now CLAWCODEX_CONTENT_PROGRESS_TIMEOUT_MS, where 0 disables. This bound is new and applies to ~15 providers at once; an operator who hits a false positive needs an off switch that is not a patched build. Three surviving mutants closed, all verified to fail now: * deleting the UsageEvent emission entirely * swapping live_usage for usage_total (the double-count both comments warn about) — no test called run_headless at all before * deleting the tool-call progress signal, the highest-consequence uncovered line: a model streaming a large Write emits tool-call argument deltas and nothing else, so the biggest tool calls would be the ones killed and replayed. The _chunk fake hardcoded tool_calls=None; it now models the real wire, where the name appears on the FIRST delta only. The two most important stream tests regressed by HANGING rather than failing, and there is no pytest-timeout plugin — in CI that burns the whole job budget and reports the wrong thing. They now run bounded on a thread: the Empty-gated mutant fails in 16s instead of hanging past 120s. Also fixed the skip docstring, which told the reader to run `uv run --python 3.13` from the repo root. That re-syncs the project .venv to 3.13 — it broke this repo's venv twice. Now --isolated --with pytest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The re-review came back APPROVE with two coverage gaps and a condition on the one deferral. All three are cheap and all three protect something load-bearing, so they land before the PR rather than after. **The escape hatch had no test of its own.** `test_deadline_can_be_disabled` pins the WIRING (that `enabled` short-circuits `expired()`) but not the parsing — its stream gap is ~1.2 s, which passes under any timeout >= 1.2 s. Two mutants survived: `0` falling back to 300 s instead of disabling, and a malformed value silently REMOVING the bound on every OpenAI-compatible provider at once. This knob is the entire mitigation for the gateway shape the deadline cannot distinguish from a stall, so it is the wrong function to leave unguarded. Both mutants now fail. **The guard on the num_turns defect was skipped in CI.** It lives in tests/test_headless_usage_events.py, which the 3.11 job cannot import (the adapter needs `typing.override` and harbor). So the regression that prompted the whole follow-up could come back with CI green — a guard nobody executes is a comment. Added a `Harbor adapter (3.13)` job running exactly the command the docstring documents. Verified it catches the mutant that maps `assistant_messages` back onto `num_turns`. **The off switch was undiscoverable**, which undercut the justification for deferring the OpenRouter-keepalive case. It appeared in no docs and the error an operator actually sees did not name it. The message now does, and the README gains a stream-stall tuning table showing both wires' knobs side by side with the reason they are separate. Also folds the measurement behind the 300 s default into the constant's docstring rather than leaving it asserted: across 727 completed trials in 44 harbor jobs, mean seconds per agent turn ran median 14.2 / p90 38.3 / p99 100.3 / max 183.8, with ZERO over 300 and one over 150 — on a metric that overstates model latency, since it includes tool execution. What the corpus cannot rule out is stated too: it contains only trials that COMPLETED, so a shape that reliably dies young is absent by construction. Fixes the mis-citation the critic caught: the 8-images arithmetic lives in `_substitute`'s failure branch, not on `_FAILURE_TTL_SECONDS`. Suite: 9641 passed, 9 skipped, 0 failures on 3.11. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Test Results 1 files 1 suites 7m 51s ⏱️ Results for commit 6fdd728. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A critic pass on the merged #789 returned REVISE with four defects and
three coverage holes; a re-review of the fixes returned APPROVE with two
residual gaps, which this PR also closes. Every finding was verified by
mutation or direct probe rather than by reading, in both directions.
The two defects where #789 asserted something untrue
UsageEvent.num_turnswas structurally always 0. The headless turncounter only advances after the agent loop returns, so no event emitted
during a run can see a non-zero value — and the harbor adapter then promoted
that zero into killed trials' metrics. The trial that did the most work
reported
num_turns: 0. That is the same floor-biased-low-on-the-longest-trials defect #789 was written to remove, relocated from tokens to turns, and
worse than the old behaviour, which omitted the field rather than asserting a
false zero. The adapter test that should have caught it used
num_turns: 4—a value the producer cannot emit.
Replaced with
assistant_messages, which counts round trips and actuallyincrements mid-run, and deliberately not mapped back onto
num_turnsanywhere: an agent-loop turn can span several round trips, so conflating them
would reintroduce the same lie with a plausible-looking number.
The "cached permanently / for the rest of the session" rationale was
false.
_FAILURE_TTL_SECONDS = 90.0and_cache_getexpires entries; thedocstring #789 added contradicted the one 350 lines above that exists to
explain the TTL. The retry is still right — an empty 200 degrades that image
for up to 90 s, across however many turns fall in that window — but the
reason given for it was wrong.
Docs that would have caused the next bug
ContentProgressDeadline's docstring documented the design that wasrejected (
except Empty: deadline.check()) and asserted a safety propertythat is false. Both call sites deliberately do the opposite. A maintainer
following the docstring would have reintroduced the hang — mutating to it
hangs. Rewritten with an explicit "this is the one way to get this wrong, and
it is the natural way to write it".
Escape hatch (the "don't hurt other providers" constraint)
#789 shipped a new hard bound on ~15 providers with no off switch, reading
CLAUDE_STREAM_FIRST_EVENT_TIMEOUT_MS— so raising it to work around a falsepositive also loosened the Anthropic watchdog. Now
CLAWCODEX_CONTENT_PROGRESS_TIMEOUT_MS, where0disables and a malformedvalue fails closed. Named in the error message and documented in the
README, because an escape hatch nobody can find is not a mitigation.
The 300 s default is now measured rather than asserted: across 727 completed
trials in 44 harbor jobs, mean seconds per agent turn ran median 14.2 / p90
38.3 / p99 100.3 / max 183.8 — zero above 300, on a metric that overstates
model latency since it includes tool execution. What the corpus cannot rule
out is stated too.
Coverage
Five mutants that previously survived now fail:
UsageEventemissionlive_usage→usage_total(the double-count)0no longer disables the deadlineNothing called
run_headlessat all before. The tool-call gap was thehighest-consequence uncovered line: a model streaming a large
Writeemitstool-call argument deltas and nothing else, so the biggest tool calls would
be the ones killed and replayed. Writing that test I made the same
unfaithful-fake mistake the critic had just flagged — set the tool name on
every delta and got
"WriteWriteWrite"— so_chunknow models the realwire, where the name appears on the first delta only.
Tests that hung now fail. The two most important stream tests regressed
by hanging, not failing, and there is no
pytest-timeout— in CI that burnsthe whole job budget and reports the wrong cause. Bounded on a thread: the
Empty-gated mutant fails in 16 s instead of hanging past 120 s.
New CI job. The guard on the
num_turnsdefect lived in a file the 3.11job cannot import, so the regression could return with CI green. A
Harbor adapter (3.13)job runs it on the interpreter the adapter actuallyuses. Verified it catches the mutant.
Also fixes the skip docstring, which told the reader to run
uv run --python 3.13from the repo root — that re-syncs the project venv, and it destroyedthis repo's twice during the work.
Verification
Suite: 9641 passed, 9 skipped, 0 failures on 3.11. Adapter tests: 7
passed under
uv run --isolated --python 3.13 --with harbor --with pytest.Deferred with the critic's explicit agreement: the OpenRouter
keepalive-comment shape (no measurement justifies a different threshold; the
off switch is the stopgap), and the in-session "Full Access" divergence from
the flag, which wants its own issue.
🤖 Generated with Claude Code