fix(early-stop): close the two surfaces #778 left reporting success - #779
Merged
ericleepi314 merged 1 commit intoAug 1, 2026
Merged
Conversation
#778 made HEADLESS report a non-success result subtype when the agent loop cut a run short. It named two gaps. This closes both. A. THE AGENT-SERVER TURN OUTCOME WAS HARDCODED TO SUCCESS. `_AgentSession._run_turn` emitted `subtype="success"` regardless of why the loop stopped, so on the TUI / VS Code path a guard-killed turn was indistinguishable from a completed one. Three consumers gate on that field: `_maybe_continue_goal` fed such a turn to the /goal judge as evidence of progress, `_maybe_review_memories` learned from it, and the cron loop rearmed on it. It now derives the subtype from `result.terminal.reason` through the SAME `EARLY_STOP_SUBTYPES` map headless uses, so the two surfaces cannot drift. The two remaining hardcoded successes in that function are UserPromptSubmit hook blocks, which never run the loop — left deliberately, consistent with excluding `hook_stopped` / `stop_hook_prevented` from the map (operator policy working as configured, not the harness cutting a run short). /goal AUTO-CONTINUES past an early stop rather than dying. Before the subtype was derived, such a turn arrived as "success" with "[Max tool turns reached]" as its text, was judged not-done, and the loop simply retried; bailing out would have silently killed loops that used to recover. So the judge is skipped — there is no output to judge, and asking a model whether "[Stopped: …]" satisfies the goal only invites a wrong answer — and a synthetic `continue` is applied instead. That is also `judge_goal`'s own fail-open verdict, so it is a path the loop already handles. Deliberately still routed through `apply_verdict` rather than enqueuing a continuation directly: that call is what ticks `turns_used` and lets the goal's cap decide. Short-circuiting it would let a turn that keeps stopping early retry forever. B. A DEGENERATE TURN FELL THROUGH TO `completed`. `query.py` nudges an assistant turn with no tool calls, no text and nothing in the outbox. Once the nudge budget was spent the WHOLE block was skipped — including the DETECTION — so a still-degenerate turn reached `Terminal(reason="completed")` and was reported as a clean success with an empty result. The last-text / outbox computation is hoisted into an `is_degenerate` flag and a new `Terminal(reason="empty_response")` fires when the budget is spent and it is still degenerate. `agent_loop_compat` surfaces an explanation as response_text, worded to stay true on BOTH routes in (budget spent, and max_turns blocking the retries) rather than claiming prompting that may not have happened. Two correctness fixes fell out of building it: * `spoke_via_outbox` read the SESSION-LIFETIME outbox, which is created once per session and never cleared — so one AskUserQuestion / Brief / SendUserMessage anywhere in a session permanently disabled the check for every prompt after it, and the silent empty success came straight back. Now scoped to the current query (a watermark taken before the loop) AND to SendUserMessage, the only tool whose entry `agent_loop_compat` promotes to response_text — an AskUserQuestion entry used to suppress the check while contributing nothing to the answer. * The empty-turn and continuation-signal arms shared one budget, so a few continuation nudges could spend it before the first empty turn arrived. The empty turn then got no retry at all and the run hard-failed without the one round trip that recovers it (measured at 3/89 terminal-bench trials). They now have separate counters and the empty-turn arm runs ahead of the shared gate. THE TAXONOMY. `TerminalReason` gains `empty_response`, which TS has no counterpart for — the arm that detects it is itself port-only (TS's continuation nudge gates on non-empty text, so it never sees the case). The parity test moves from set-EQUALITY against the TS snapshot to "TS is a SUBSET and every extra is declared in `PYTHON_ONLY_TERMINAL_REASONS`". That keeps both teeth that matter — a dropped TS reason still fails, an undeclared extra still fails — and a companion test requires every declared extra to be in `EARLY_STOP_SUBTYPES`, so the escape hatch cannot reintroduce a silent success. DOWNSTREAM. The VS Code chat pane matched `subtype === 'error'` exactly, so the new subtypes fell through to "Completed (N turns)"; it now checks `is_error` and falls back to `msg.result` when nothing streamed. The TUI already keyed on `is_error` but printed the explanation twice — once as the turn text, once as a red error line — because an early stop carries its explanation in `result` and sets no separate `error` field; it now emits a short "run stopped early (<subtype>)" instead of echoing. Verified end to end against a local SSE server returning a genuinely empty assistant turn: `error_during_execution`, `is_error: true`, num_turns 4 (1 + 3 retries), explanation in `result`. Every fix mutation-tested, including against the exact mutation that proved an earlier version of the agent-server test was a placebo (compute the subtype, then discard it). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ericleepi314
added a commit
that referenced
this pull request
Aug 1, 2026
…tray edit (#780) Main is red on `tests/server/test_goal_control.py:: test_cancelled_and_error_turns_skip_judging` (CI on #779: 1 failed, 9474 passed). Two defects, both from #779. 1. A USER CANCEL OR PROVIDER ERROR RESTARTED THE GOAL LOOP. #779 made /goal auto-continue past a turn the agent loop cut short, keyed on `subtype not in ("", "success")`. But `_run_turn` also returns `cancelled` on AbortError and `error` on an exception, so both were swept in. Pressing ESC during a /goal loop RE-ENQUEUED the work the user had just killed (the `_inbox.empty()` preflight does not help — an interrupt leaves it empty), and a provider 5xx/auth failure retried up to the goal's whole turn budget. That contradicted the function's own docstring ("Skips when: … the turn was cancelled/errored") and `_after_wakeup_turn`'s rationale, which cites the goal loop's no-continuation-on-error guard as precedent. A test already pinned it; #779 broke it. Narrowed to `EARLY_STOP_SUBTYPES.values()` — the stops the AGENT LOOP itself produced. Those mean "the model did not finish", so retrying is right and matches the pre-#779 behaviour (the turn arrived as "success", was judged not-done, and the loop retried). `cancelled` / `error` are the same category as the hook stops deliberately excluded from that map: the USER or the PROVIDER ended the turn, not the harness cutting it short. 2. A STRAY EDIT REACHED main. `agent_server.py` carried, verbatim: if early_stop: # MUTANT: short-circuit the budget tick should_continue = True continuation = continuation or "[Continuing toward your standing goal]" That is mutation-testing scaffolding committed by accident in #779 — the tree was mutated in place while the commit was taken, and the staged diff was not read. Measured impact, since the comment overstates it: `apply_verdict` still runs BEFORE this block, so `turns_used` still ticks and the goal still deactivates at its cap. At max_turns=3 it yields 3 continuations instead of 2 — one extra model turn past budget, bounded, not a runaway. Still wrong: it overrides the goal's own decision to stop. TESTS. The cap test added in #779 cannot catch (2) — the cap genuinely holds under it — so the new test asserts AUTHORITY instead: patch `apply_verdict` to say stop, assert nothing is enqueued, with a positive control so it cannot pass vacuously. It fails against the exact code that shipped. Also added a cancel/error guard test in this file (the pre-existing one lives in test_goal_control.py, which is why the targeted runs during #779 missed it), likewise paired with a positive control — this harness swallows exceptions, so a bare "nothing was enqueued" assertion would otherwise pass for the wrong reason. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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.
#778 made headless report a non-success result subtype when the agent loop cut a run short. It named two gaps. This closes both.
A — the agent-server turn outcome was hardcoded to success
_AgentSession._run_turnemittedsubtype="success"regardless of why the loop stopped, so on the TUI / VS Code path a guard-killed turn was indistinguishable from a completed one. Three consumers gate on that field:_maybe_continue_goalfed such a turn to the /goal judge as evidence of progress,_maybe_review_memorieslearned from it, and the cron loop rearmed on it.It now derives the subtype from
result.terminal.reasonthrough the sameEARLY_STOP_SUBTYPESmap headless uses, so the two surfaces cannot drift.The two remaining hardcoded successes in that function are UserPromptSubmit hook blocks, which never run the loop — left deliberately, consistent with excluding
hook_stopped/stop_hook_preventedfrom the map (operator policy working as configured, not the harness cutting a run short)./goal auto-continues rather than dying
Before the subtype was derived, an early-stopped turn arrived as "success" with
[Max tool turns reached]as its text, was judged not-done, and the loop retried. Bailing out would have silently killed loops that used to recover on their own.So the judge is skipped — there's no output to judge, and asking a model whether
[Stopped: …]satisfies the goal only invites a wrong answer — and a syntheticcontinueis applied. That's alsojudge_goal's own fail-open verdict, so it's a path the loop already handles.Deliberately still routed through
apply_verdictrather than enqueuing directly: that call ticksturns_usedand lets the goal's cap decide. Short-circuiting it would let a turn that keeps stopping early retry forever.B — a degenerate turn fell through to
completedquery.pynudges an assistant turn with no tool calls, no text, nothing in the outbox. Once the nudge budget was spent the whole block was skipped — including the detection — so a still-degenerate turn reachedTerminal(reason="completed")and was reported as a clean success with an empty result.The detection is hoisted into an
is_degenerateflag and a newTerminal(reason="empty_response")fires when the budget is spent and it's still degenerate, with an explanation surfaced as response_text.Two correctness fixes fell out of building it:
spoke_via_outboxread the session-lifetime outbox, created once per session and never cleared — so oneAskUserQuestion/Brief/SendUserMessageanywhere in a session permanently disabled the check for every later prompt, and the silent empty success came straight back. Now scoped to the current query and toSendUserMessage, the only tool whose entry becomes response_text (anAskUserQuestionentry suppressed the check while contributing nothing).The taxonomy change
TerminalReasongainsempty_response, which TS has no counterpart for — the arm that detects it is itself port-only (TS's continuation nudge gates on non-empty text, so it never sees the case).The parity test moves from set-equality against the TS snapshot to "TS is a subset, and every extra is declared in
PYTHON_ONLY_TERMINAL_REASONS". Both teeth that matter are kept: a dropped TS reason still fails, an undeclared extra still fails. A companion test requires every declared extra to be inEARLY_STOP_SUBTYPES, so the escape hatch cannot reintroduce a silent success.Downstream
subtype === 'error'exactly, so the new subtypes fell through to "Completed (N turns)". Now checksis_errorand falls back tomsg.resultwhen nothing streamed.is_errorbut printed the explanation twice — once as turn text, once as a red error line — because an early stop carries its explanation inresultand sets no separateerrorfield. It now emits a shortrun stopped early (<subtype>).Verification
End to end against a local SSE server returning a genuinely empty assistant turn:
error_during_execution,is_error: true, num_turns 4 (1 + 3 retries), explanation inresult.Every fix mutation-tested — including against the exact mutation that proved an earlier version of the agent-server test was a placebo (compute the subtype, then discard it; the old test passed, the new behavioural one fails).
Reviewed by the
criticsubagent across two rounds on this PR. It caught the outbox scoping with a runtime repro, the shared nudge budget, the placebo test, and the VS Code gap.🤖 Generated with Claude Code