fix(goal): stop cancels and errors from re-running the goal, drop a stray edit - #780
Merged
Merged
Conversation
…tray edit 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>
Test Results 1 files 1 suites 8m 3s ⏱️ Results for commit 30f18b8. |
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.
mainis red. CI on #779:1 failed, 9474 passed—tests/server/test_goal_control.py::TestMaybeContinueGoal::test_cancelled_and_error_turns_skip_judging. Two defects, both introduced by #779.1. A user cancel or provider error restarted the goal loop
#779 made
/goalauto-continue past a turn the agent loop cut short, keyed onsubtype not in ("", "success"). But_run_turnalso returnscancelledonAbortErroranderroron an exception — both got swept in.So pressing ESC during a
/goalloop re-enqueued the work you had just killed (the_inbox.empty()preflight doesn't help — an interrupt leaves it empty), and a provider 5xx/auth failure retried up to the goal's whole turn budget.It 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 its precedent. A test already pinned this; #779 broke it.Narrowed to
EARLY_STOP_SUBTYPES.values()— the stops the agent loop produced. Those mean "the model did not finish", so retrying is right and matches pre-#779 behaviour (the turn arrived as "success", was judged not-done, and the loop retried).cancelled/errorare 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.pycarried, verbatim:Mutation-testing scaffolding committed by accident in #779 — the tree was mutated in place while the commit was taken, and the staged diff wasn't read.
Measured impact, because the comment overstates it:
apply_verdictstill runs before this block, soturns_usedstill ticks and the goal still deactivates at its cap. Atmax_turns=3it 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, which is exactly why it slipped through. So the new test asserts authority instead: patch
apply_verdictto say stop, assert nothing is enqueued. It fails against the exact code that shipped.Also adds 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 never saw it.Both new tests carry positive controls: this harness swallows exceptions, so a bare "nothing was enqueued" assertion would otherwise pass for the wrong reason if a stub were missing.
Verification
test_goal_control.pyno longer among the failures.tests/server: 469 passed.git show HEADconfirmed free of scaffolding.🤖 Generated with Claude Code