fix(harness): stop the loop guard and plan mode from killing headless runs - #777
Merged
Merged
Conversation
… runs
Two model-agnostic harness defects found by comparing terminal-bench 2.1
trajectories. Both punished a model for HOW it called tools rather than for
whether it was making progress.
1. TOOL-FAILURE LOOP GUARD counted per failing BLOCK, not per batch.
`update_tool_failure_loop_guard` built its `failures` list from every result
in one batch, then incremented the counters once per failure. An assistant
turn issuing `threshold` (3) parallel calls that all failed the same way
therefore tripped the guard inside that single turn — on the model's FIRST
mistake, with no turn in which to correct it.
Live: two runs ended after 3 and 4 turns (31s / 38s) on a plain
`python3: command not found`. A serially-calling model hit the same missing
interpreter on the same task images at a similar rate (20 occurrences / 13
trials vs 17 / 8) and was never stopped once in 88 trials. The differentiator
was purely batching: 13.8% of turns issued >=3 parallel calls versus 0.7%.
Counters now increment at most once per batch per distinct key, which is what
this module's own docstring always claimed ("consecutive tool batches").
Strictly permissive: every counter is pointwise <= its old value and every
trip predicate is monotone, so it can only delay a trip, never cause one.
Verified by differential fuzz against the old behavior — 4000 randomized
trials, zero cases tripping earlier. A real loop still trips.
This is a deliberate divergence from TS (toolFailureLoopGuard.ts:137,173,201
increments per block) and is documented as such in the module docstring,
along with the measured delay bounds.
2. PLAN MODE was a one-way door headlessly.
ExitPlanMode is `requires_user_interaction`, hence bypass-immune, so
headlessly its ask always resolves to a deny — under bypass carrying the raw
dialog question, "Exit plan mode?", handed to the model as a tool error it
cannot act on. EnterPlanMode auto-allows. The model could enter plan mode and
never leave, after which every write was refused.
Live: 41 ExitPlanMode calls, a 100% error rate, across 16 of 32 trials —
5.7% of the entire tool budget spent on a round trip that could not complete.
Headless now unregisters both, beside the existing AskUserQuestion removal.
This is PARITY-FAITHFUL, not a divergence: ExitPlanModeV2Tool.ts:167-178 and
EnterPlanModeTool.ts:52-63 both disable when the user is not watching the TUI,
explicitly "so plan mode isn't a trap the model can enter but never leave."
3. REQUIRED FIX — removal did not mean removal.
`remove_tool()` only unadvertises. `run_tool_use` falls back to the full
base-tool list, and the port had dropped TS's guard accepting that fallback
only when the called name is a deprecated ALIAS (toolExecution.ts:415-426,
"Only fall back for tools where the name matches an alias, not the primary
name"). So any removed tool resolved by primary name and executed — which
would have made fix 2 cosmetic, since the plan-mode reminder NAMES
ExitPlanMode.
Restoring the guard also closes two holes that predate this change:
`--allowed-tools` / `--disallowed-tools` dropped tools from the advertised
schema while leaving them callable, and subagents could call the
ALL_AGENT_DISALLOWED_TOOLS set (Agent, Workflow, TaskStop, ...) by primary
name — defeating the explicit "prevent recursive workflow execution inside
subagents" containment.
Plus `NON_INTERACTIVE_PLAN_ADDENDUM`: the ported plan-mode text names
AskUserQuestion / ExitPlanMode as the only two ways to end a turn, so
`--permission-mode plan` headlessly needed a correction telling the model to
write the plan and end the turn. Appended rather than edited into the ported
text, so the port stays byte-comparable against typescript/.
MODEL-AGNOSTIC BY CONSTRUCTION: serial callers are bit-identical under the
guard change, no model is named anywhere, and the plan-mode removal drops
tools that cannot function on that surface for anyone. Availability is read
from `tool_registry` rather than `tool_context.options.tools` — the latter is
empty until query() populates it, which would have mislabelled the FIRST
query of every interactive session as non-interactive.
Both fixes and every new guard are mutation-tested. Full suite green (the 27
MCP-auth failures in a fresh worktree venv reproduce identically on pristine
main with these changes stashed — an mcp version resolution issue, not code).
NOT included, deliberately: a guard trip still returns `subtype: "success",
is_error: false`, so batch runners record a clean run with reward 0. That is
why this took a benchmark analysis to find. It changes headless exit semantics
for every model and every eval adapter, so it lands separately.
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.
Two model-agnostic harness defects found by comparing terminal-bench 2.1 trajectories between a parallel-calling model and a serial one. Both punished a model for how it called tools rather than for whether it was making progress.
1. The loop guard counted per failing block, not per batch
update_tool_failure_loop_guardbuilt itsfailureslist from every result in one batch, then incremented counters once per failure. An assistant turn issuingthreshold(3) parallel calls that all failed the same way tripped the guard inside that single turn — on the model's first mistake, with no turn in which to correct it.Live: two runs ended after 3 and 4 turns (31s / 38s) on a plain
python3: command not found. A serially-calling model hit the same missing interpreter on the same task images at a similar rate (20 occurrences/13 trials vs 17/8) and was never stopped once in 88 trials. The differentiator was purely batching — 13.8% of turns issued ≥3 parallel calls versus 0.7%.Counters now increment at most once per batch per distinct key, which is what the module's own docstring always claimed ("consecutive tool batches").
Strictly permissive: every counter is pointwise ≤ its old value and every trip predicate is monotone, so it can only delay a trip, never cause one. Verified by differential fuzz against the old behavior — 4000 randomized trials, zero cases tripping earlier. A real loop still trips.
Deliberate divergence from TS (
toolFailureLoopGuard.ts:137,173,201increments per block), documented in the module docstring with measured delay bounds.2. Plan mode was a one-way door headlessly
ExitPlanModeisrequires_user_interaction, hence bypass-immune, so headlessly its ask always resolves to a deny — under bypass carrying the raw dialog question,"Exit plan mode?", handed to the model as a tool error it cannot act on.EnterPlanModeauto-allows. The model could enter plan mode and never leave, after which every write was refused.Live: 41 ExitPlanMode calls, 100% error rate, across 16 of 32 trials — 5.7% of the entire tool budget on a round trip that could not complete.
Headless now unregisters both, beside the existing
AskUserQuestionremoval. This is parity-faithful, not a divergence:ExitPlanModeV2Tool.ts:167-178andEnterPlanModeTool.ts:52-63both disable when the user isn't watching the TUI, explicitly "so plan mode isn't a trap the model can enter but never leave."3. Removal did not mean removal (required for #2 to work)
remove_tool()only unadvertises.run_tool_usefalls back to the full base-tool list, and the port had dropped TS's guard accepting that fallback only when the called name is a deprecated alias (toolExecution.ts:415-426). So any removed tool resolved by primary name and executed — which would have made fix 2 cosmetic, since the plan-mode reminder names ExitPlanMode.Restoring the guard also closes two holes that predate this change:
--allowed-tools/--disallowed-toolsdropped tools from the advertised schema while leaving them callable.ALL_AGENT_DISALLOWED_TOOLSset (Agent,Workflow,TaskStop, …) by primary name, defeating the explicit "prevent recursive workflow execution inside subagents" containment.Plus
NON_INTERACTIVE_PLAN_ADDENDUM, since the ported plan text names AskUserQuestion/ExitPlanMode as the only ways to end a turn — appended rather than edited in, so the port stays byte-comparable againsttypescript/.Model-agnostic by construction
Serial callers are bit-identical under the guard change. No model is named anywhere. The plan-mode removal drops tools that cannot function on that surface for anyone.
Availability is read from
tool_registry, nottool_context.options.tools— the latter is empty untilquery()populates it, which would have mislabelled the first query of every interactive session as non-interactive, on exactly the turn carrying the full plan text.Verification
Error: No such tool available, an actionable message; a genuine deprecated alias still resolves and executes.mainwith these changes stashed — anmcpversion-resolution issue in that env, not code.Reviewed by the
criticsubagent across three rounds. It caught that my first cut of fix 2 was non-enforcing (issue 3 above), and that my availability check misfired on the TUI's first query.Not included, deliberately
A guard trip still returns
subtype: "success", is_error: false, so batch runners record a clean run with reward 0 — which is why this took a benchmark analysis to find. That changes headless exit semantics for every model and every eval adapter, so it lands separately.🤖 Generated with Claude Code