feat(agents)!: honor run_limits.max_turns on codex and antigravity - #110
feat(agents)!: honor run_limits.max_turns on codex and antigravity#110bai-uipath wants to merge 13 commits into
Conversation
…harness Closes #68. Closes #108. `system_prompt` is now defined as text APPENDED to each harness's own default agent prompt, and every backend maps it to its additive knob. This is a breaking change on claude-code, which previously mapped it to `--system-prompt` (replacement) and — because the SDK emits `--system-prompt ""` for None — ran with no system prompt at all when the field was unset. `run_limits.max_turns` was accepted and ignored on codex and antigravity. Both now cap on visible turns (resolved tool calls) read off a shared `EventCollector.visible_turn_count`, enforced on the same loop boundary as the cooperative early stop. claude-code keeps its native SDK cap. Antigravity now honors `allowed_tools` / `disallowed_tools` via `CapabilitiesConfig`, so the same task file exposes the same tool surface on all three backends. Structural tools are never stripped, and an allowlist that maps to nothing usable falls back to the harness default with a warning. Where a backend genuinely cannot implement a field it now declares that on its agent class (`Agent.config_support`) rather than dropping it silently: `validate_config_support` hard-errors at resolution on an unhonored field set to a non-default value, mirroring `validate_early_stop`. Today's declarations are all APPROXIMATED, so no existing run changes. Also drops the report's "N turn(s) avoided" claim (derived from `max_turns - sdk_turn_index`, which overstated by the whole budget on the single-SDK-turn backends), and pins the user simulator's model instead of letting BEDROCK_MODEL swap the simulated user underneath an A/B. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The claude-code append fix, the Codex developer_instructions mapping, and the matching docs are all covered by #92, which is further along in review and also carries the system_prompt_mode escape hatch that judge sub-agents need (their prompt is the entire scoring instrument and must not be prefixed by the claude_code coding-agent preset — SubAgentRunner builds a ClaudeCodeAgent, so an unconditional append would have shifted every llm_judge verdict). Leaves this PR to the rest of #68 plus #108: the visible-turn max_turns cap, Antigravity tool allowlists, the config_support declarations and resolution guard, the SDK env seam, the pinned simulator model, and the turns-avoided note. The system_prompt parity tests are gone; the shared visible-turn-count tests they were sharing a file with move to tests/test_visible_turn_cap.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both turned out to be their own design problems rather than a side note to the turn cap. config_support (the ConfigSupport/ConfigFieldSupport declaration plus the resolution guard at three seats) had no real user: every declaration was APPROXIMATED, so the guard only ever fired for a synthetic test agent, and the one field that genuinely warranted UNHONORED could not be declared without hard-erroring live nightlies. The Antigravity allowed_tools/disallowed_tools mapping works, but the questions it raises are not small: whether the config field should be spelled in Claude's tool vocabulary at all, and what an allowlist that maps to nothing should do (falling back to every tool hands the model MORE than it asked for, which is the wrong direction for an eval harness). Leaves the PR to the visible-turn max_turns cap, the SDK env seam, the pinned simulator model, and the turns-avoided note. HARNESS_PARITY.md narrows to the run-limit contract it can actually back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One task file per limit, run with --type claude-code / codex / antigravity, so the three harnesses can be compared on the same prompt. The max_turns fixture asks for 12 sequential tool calls under a cap of 4; the turn_timeout fixture blocks for 240s under a 45s watchdog. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ran both fixtures on claude-code (Bedrock), codex (gpt-5.4) and antigravity (gemini-3.5-flash) on the eval VM. Three findings worth writing down: The cap works and is now identical on the two backends that ignored it: 12 writes requested under max_turns 4, all three stop at exactly 4 resolved tool calls with max_turns_exhausted and a clean SUCCESS. The claude-code unit difference is bigger than "slightly different". Under a batching prompt with max_turns 2, claude-code permitted all 12 writes across 14 assistant messages, because one SDK agent-loop turn absorbs however many parallel calls the model emits. Codex and Antigravity stop at 2. Same number, very different budget. Antigravity's localharness backgrounds any shell command over ~10s, so turn_timeout never fires for a slow command: sleep 240 under a 45s watchdog ended the turn at 19.3s with the tool force-closed as unresolved, while claude-code and codex both timed out at ~45s with the partial turn captured. That also means any task whose real work is a long install or build is not the same task on Antigravity. Documented in both pages; no workaround attempted here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI requires every task YAML to carry at least one tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… loop #111 extracted the antigravity step loop into `_drain()` and added a poll loop for backgrounded work, both of which this branch's `max_turns` cap sits on top of. The cap check moves into `_drain()` so the initial drain and every poll re-drain honor it identically, and the poll loop now exits on a reached cap instead of continuing to wait out a background job on a run that is over. Drops the `_harness_spawn_guard` PATH mutation main still carries; this branch replaces it with the SDK's per-agent `env` seam. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Examples pinned Sonnet 4-6 / Opus 4-7 / the dated 20250514 and 20250929 ids, so a reader copying one starts on a model two generations back. Refreshed to Sonnet 5 / Opus 5, including the Bedrock inference-profile forms. Untouched on purpose: the `anthropic.claude-sonnet-4-6` values in the llm_judge and simulation sections are the documented code defaults (DEFAULT_JUDGE_MODEL), and experiments/default.yaml's pin is what runs, not an example — bumping either changes results, not docs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oll loop The measured turn_timeout row was taken before #111 landed and no longer holds: a backgrounded command is now polled for instead of ending the turn on an idle step stream. Re-ran the same fixture on the same box and model, plus an A/B on a job that finishes inside the budget (FAILURE at 19.5s before, SUCCESS at 75.0s after). Restates the residual divergence as what it now is: the terminal signal on a job that outlives the budget, not whether slow work completes at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bullet still described `Agent.config_support`, `ConfigFieldSupport`, and `orchestration/config_support.py::validate_config_support` as live machinery after this branch removed all three, pointing an agent reading CLAUDE.md at a module that no longer exists. Rewrites it around what survives: the visible-turn `max_turns` semantic, the claude-code unit divergence, and the current list of known-unfixed field divergences. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the PR The page mixed a durable per-harness contract with a dated experiment report, and the experiment half went stale inside a day when #111 changed the Antigravity timeout behavior. Durations measured on one box with one model also give an outside reader on coder-eval.com nothing to act on. Keeps every claim the numbers supported, drops the tables, and points at the fixtures under tasks/run_limits/ for anyone who wants to re-measure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three alerts, all in the merged test helper: - Two `lambda **kw: SimpleNamespace(**kw)` wrappers that are just `SimpleNamespace`. One is dropped outright — `CapabilitiesConfig` was scaffolding for the tool-mapping work this PR no longer carries. - A bare `await task_a` flagged as having no effect. Now bounded by `asyncio.wait_for`, so a regression that re-serializes concurrent starts fails the test instead of hanging the suite. Also refreshes two comments left describing the dropped scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @bai-uipath's task in 1m 49s —— View job 🔍 PR Review in Progress
|
Every other env test stubs LocalAgentConfig, so together they prove only that
coder_eval builds the right kwarg. A google-antigravity bump that dropped or
renamed `env` would leave all of them green while mock CLIs silently stopped
shadowing and the agent called the real tool: the exact silent-wrong-mode the
seam exists to prevent.
Asserts against the real class instead — the field exists, round-trips, and
defaults to None rather than {} (the connection reads `is not None` to decide
whether to build a merged env at all).
Verified end-to-end on the eval VM alongside this: a record_cli shim for `uip`
reached the agent's PATH and returned its sentinel, with the invocation logged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
akshaylive
left a comment
There was a problem hiding this comment.
Review: feat(agents)!: honor run_limits.max_turns on codex and antigravity
PR #110 by @bai-uipath · bai/harness-config-parity → main · OPEN · reviewed against 7142fdf
This is a well-executed piece of harness-parity work: it closes a real gap (Codex and Antigravity previously ignored run_limits.max_turns entirely), the two agents' new enforcement logic is structurally identical to each other and correctly diverges from Claude Code's native counter only where the SDKs actually differ, and the breaking change is documented unusually well for a repo this size — a new dedicated page (HARNESS_PARITY.md), cross-links from both agent docs, and worked task fixtures. Full test suite passes (4304 passed, 13 skipped), and security/type/error-handling checks came back clean. The main things worth addressing before merge are a couple of test gaps around the new "clean stop" exception path and a documentation gap around max_turns's per-agent unit semantics. Overall score 9.5/10, weakest axes are Code Quality and Harness Quality (tied at 8.9/10) — driven entirely by Medium/Low findings, no Critical or High issues.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 8.9/10 | 0 | 0 | 2 | 1 | max_turns_reached()/ended_cleanly duplicated verbatim across codex/antigravity turn-state classes |
| 2. Type Safety | 10.0/10 | 0 | 0 | 0 | 0 | No findings |
| 3. Test Health | 9.0/10 | 0 | 0 | 2 | 0 | No test drives an exception during teardown after ended_cleanly (max_turns/stopped_early) |
| 4. Security | 10.0/10 | 0 | 0 | 0 | 0 | No findings |
| 5. Architecture & Design | 9.7/10 | 0 | 0 | 0 | 3 | max_turns carries two incompatible units across backends by design (documented, not a defect) |
| 6. Error Handling & Resilience | 10.0/10 | 0 | 0 | 0 | 0 | No findings |
| 7. API Surface & Maintainability | 9.4/10 | 0 | 0 | 1 | 1 | TASK_DEFINITION_GUIDE.md still documents the removed "N turn(s) avoided" report note |
| 8. Evaluation Harness Quality | 8.9/10 | 0 | 0 | 2 | 1 | reports_stats.visible_turn_count can read cap + 1 under Antigravity's background-poll edge case |
Overall Score: 9.5 / 10 · Weakest Axis: Code Quality (1) and Harness Quality (8), tied at 8.9 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 7 · 🔵 6 across 8 axes reviewed.
Blockers
No blockers found.
Non-blocking, but please consider before merge
Scoring/reporting correctness
run_limits.max_turnsnow means two structurally different things per agent (a native SDK inner-loop turn on Claude Code, which can bundle parallel tool calls, vs. one resolved tool call on Codex/Antigravity), and onlydocs/agents/HARNESS_PARITY.mdsays so. The field's own Pydanticdescription(src/coder_eval/models/limits.py:32-36) gives no hint of the divergence, so a user setting onemax_turnsfor a cross-agent A/B can reasonably (and wrongly) assume it's a portable unit. Add a one-line pointer from the description toHARNESS_PARITY.md.reports_stats.visible_turn_count(src/coder_eval/reports_stats.py:422-432) counts allTurnRecord.commandsentries regardless ofresult_status, while the live cap check (EventCollector.visible_turn_count) counts only resolved calls. Under Antigravity's background-poll edge case (a >10s shell command still in flight when the cap fires), the orphaned call gets force-closed asresult_status: unknownand lands incommandsafter the cap decision — so the persisted metric can readcap + 1, contradictingHARNESS_PARITY.md's own claim that "the resolved count still matches" the cap. Doesn't affectfinal_status/score, but does affect theexpected_turns_overagethreshold metric and report badges.
Test coverage
- The PR widens the "ignore exception on a clean break" guard from
stopped_early_hitalone toended_cleanly(stopped_early_hit or max_turns_hit) in both agents specifically to stop a cap-triggered break from escalating into a crash/retry — but no test in eithertest_codex_agent.pyortest_antigravity_agent.pyactually raises during teardown aftermax_turns_hit=Trueto prove the turn still finalizes cleanly instead of raisingAgentCrashError. - The bundled
SimulationConfig.model/SimulationTelemetry.simulator_modeladdition reprioritizessimulator_cost_usd()'s pricing precedence and changes howorchestrator.pyresolvessim_model_id— but no test in the diff exercises either the new precedence or the new resolution path (tests/test_run_limits_orchestrator.py's only change is a mock-satisfaction tweak, not a behavioral assertion).
Docs
docs/TASK_DEFINITION_GUIDE.md:486still shows the removed "N turn(s) avoided" report-note phrase, butreports.py's own diff deliberately deletes that claim (its comment explains why: it overstated how much work an early-stopped row actually skipped). Update the doc's example to match.SimulationConfig.model(a field tracked by CE030) got a docs update only in the simulation section ofTASK_DEFINITION_GUIDE.md, not in any cross-referencing table — likely fine, flagged for completeness.
Minor code quality
_CodexTurnState/_AntigravityTurnState'smax_turns_reached()/ended_cleanlyare near-identical copy-pasted implementations with no shared base — both just delegate toEventCollector.visible_turn_count. A small shared helper would remove the two-copies-to-keep-in-sync risk.- The end-of-turn status precedence (
timeout > stopped_early > max_turns_exhausted > completed) is now hand-implemented three times (Codex, Antigravity, and the pre-existing Claude Code path) with three different shapes — worth a sharedresolve_end_status(...)helper if this precedence rule ever needs to change again.
Nits
docs/agents/HARNESS_PARITY.mdis a substantial new page — worth confirming it's linked from the docs nav (make docs-indexes/CE028) rather than orphaned.UserSimulator._resolve_model(src/coder_eval/simulation/user_simulator.py:233-234) importsto_anthropic_alias/to_bedrock_model/BedrockRoutelocally inside the method with no circular-import hazard and no comment explaining why — hoist to module scope or explain the choice.SimulationTelemetry.simulator_model(new field) currently has only one reader (simulator_cost_usd) — legitimate, but consider surfacing it in the human-readable simulation report block in a follow-up.
What's Missing
- Tests: covered above (items 3, 4) — no additional gaps found beyond those.
- Downstream consumers: covered above (item 2) — no other counting/classification consumer missed the new max_turns-cap status.
- Display & mapping dicts:
AgentEndStatus.MAX_TURNS_EXHAUSTED/FinalStatus.MAX_TURNS_EXHAUSTEDare pre-existing (from the Claude Code path), so no new enum value was introduced — all report renderers already handle it. Nothing identified. - Parallel paths: Codex and Antigravity's implementations were checked structurally identical to each other; the only divergence risk is the code-duplication note under item 7 above.
Harness & Lint Improvements
Static checks (lint / type):
- A lint rule that greps a diff for removed user-facing string literals in
reports*.pyand flags any doc underdocs/still quoting the removed phrase would have caught item 5 mechanically (Technique 1 applied to doc/code string parity — neither CE028 nor CE030 currently check this).
Harness improvements:
- A golden/parity test comparing
EventCollector.visible_turn_countagainst the persistedreports_stats.visible_turn_countunder a synthetic Antigravity background-poll-then-cap scenario would catch item 2's drift directly — this needs a live event stream to diff, so it can't be a pure static check. - Runtime fault-injection tests for "raise during post-cap/post-stop teardown" (item 3) are inherently a test-authoring gap, not something static analysis can surface.
Top 5 Priority Actions
- Add a one-line pointer from
RunLimits.max_turns's field description (and its row inTASK_DEFINITION_GUIDE.md) toHARNESS_PARITY.md, since the field now means a structurally different unit per agent and the schema description gives no hint of this. - Add tests on both agents that raise an exception during teardown after
ended_cleanly(stopped_early or max_turns) and assert the turn still finalizes cleanly withmax_turns_exhausted=Trueinstead of escalating toAgentCrashError. - Reconcile
reports_stats.visible_turn_count's counting of force-closedunknown-status commands against the cap-time decision, so a capped Antigravity run's persisted turn-count metric can't silently readcap + 1. - Add a direct test asserting
simulator_cost_usd()prices from the newSimulationConfig.model/SimulationTelemetry.simulator_modelwhen set, and an orchestrator-level test confirmingsim_model_idresolves from the simulator's actual model. - Extract the duplicated
max_turns_reached()/ended_cleanlyimplementations (and consider a sharedresolve_end_status(...)helper for the three hand-synced status-precedence cascades) across the agent implementations.
Change class: complex — introduces new max_turns enforcement/control-flow mirrored across two Agent implementations, changes turn-end status precedence logic, and is a documented breaking change
Stats: 0 🔴 · 0 🟠 · 7 🟡 · 6 🔵 across 8 axes reviewed.
Full per-axis breakdown: tmp/code-review-260813-1942/01-code-quality.md … tmp/code-review-260813-1942/08-harness-quality.md.

Closes #108. Advances #68 (
max_turnsonly — see Scope below).run_limits.max_turnswas accepted and silently ignored on Codex and Antigravity. A task file sayingmax_turns: 6ran capped on Claude Code and unbounded on the other two, so the same file was not the same task.max_turnson codex and antigravityBoth now cap on visible turns: resolved tool calls, the unit
visible_turn_countalready reports and the listTurnRecord.commandsalready holds. Both read it live off a sharedEventCollector.visible_turn_countso one value means one thing on both, rather than two counters that happen to agree.They need their own counter because each delivers a single SDK turn per
communicate(), so a native cap would clamp at 1 regardless of what the task asked for.The cap fires on the same loop boundary as the cooperative early stop: the step that reaches it is processed whole, the next is never pulled, the in-flight turn is cancelled server-side, and the run finalizes as
max_turns_exhausted— a clean stop, not a crash, not retried. Criteria are still checked against whatever the agent produced.On Antigravity that boundary lives inside
_drain(), which #111 introduced along with a poll loop for backgrounded work. Putting the check there means the initial drain and every poll re-drain honor the cap identically, and the poll loop stops polling once the cap is reached rather than waiting out a background job on a run that is already over. A regression test covers that seam: without the guard, a capped run keeps polling for its full 120-cycle budget.Verified on the eval VM
Two fixtures added under
tasks/run_limits/, one prompt per limit, run on all three harnesses (claude-code on Bedrock, codexgpt-5.4, antigravitygemini-3.5-flash).max_turns_cap.yaml— prompt asks for 12 sequential file writes,max_turns: 4:max_turns_exhaustedfinal_statusAll three stop at exactly 4 and finish cleanly, with the first file on disk so the criteria grade real work. On
main, codex and antigravity run the prompt to completion and write all 12.Batching prompt (parallel tool calls encouraged),
max_turns: 2:This quantifies the residual divergence rather than hiding it. claude-code keeps its native SDK cap, whose unit is the SDK's own agent-loop turn, and that turn absorbs however many parallel calls the model emits — so a cap of 2 permitted all 12 writes. Holding
max_turnsconstant across harnesses does not hold the budget constant. If you are A/B-ing across backends and the cap is close to binding, that is the number to distrust. Reimplementing claude-code's cap in the other unit would mean throwing away a real, working SDK cap, so it is documented instead.The codex
+1is the tool already in flight when the cap fired: the run stopped after 2 completed calls, and the third is force-closed withresult_status: unknownrather than dropped, so the trajectory shows what was interrupted.turn_timeout.yaml—sleep 240under a 45s watchdog:crashed: truecrashed: truecrashed: falseClaude Code and Codex behave identically here. Antigravity stops earlier and more gently, for the reason below.
Antigravity: what the 10-second boundary still costs you
The Antigravity localharness has a 10-second maximum synchronous wait for shell commands. Past it the command becomes a background task and the model gets a task id instead of a result. That is harness behavior, not something coder_eval configures;
sleep 5resolving normally in 10.4s pins the boundary.#111 made that survivable: the turn polls for the backgrounded result instead of finalizing on an idle step stream. Measured here on a command that finishes inside the budget (
sleep 60writing a file,turn_timeout: 300), same task and model on either side of that change:What remains is a bounded wait rather than an unlimited one: 80% of
turn_timeout(or 120 five-second cycles when the task sets no timeout), notturn_timeoutitself. A job that outlives that bound is force-closed as unresolved and graded on everything else, where Claude Code and Codex raise a turn timeout and mark the turn crashed.So the residual divergence is the terminal signal, not whether slow work completes: a long
npm installor build now runs to completion here the way it does on the other two, but a command that never finishes reads as an ordinary low score rather than a timeout. Documented inANTIGRAVITY.mdandHARNESS_PARITY.md.Smaller items
Drops the report's
N turn(s) avoidedclaim. It derived frommax_turns - sdk_turn_index, which on the single-SDK-turn backends advertised the whole budget as saved when only a tool-call tail was cut. The bound is still persisted onEarlyStopInfo, labelled there for what it is.Pins the user simulator's model. It passed
model=None, soBEDROCK_MODELdecided who the simulated user was — an A/B varying the subject model silently varied its interlocutor too. Now a config field with its own default, translated per backend the way the LLM judge's is, and recorded on the result so simulator cost prices from a fact.Antigravity's mock-CLI PATH shadowing moves to the SDK's
envseam (shipped in the 0.1.7 we already pin), deleting the process-wideos.environmutation and the lock that serialized every harness start. Concurrent tasks now get genuinely separate environments instead of a time-sliced global one.Verified against the real SDK rather than only the fake:
LocalAgentConfig.env→env=self.env→merged_env = {**os.environ, **env_map}→subprocess.Popen(env=merged_env), so our PATH wins over the inherited one and omittingenvstill spawns with plain inheritance. Confirmed live on the VM too: arecord_clishim foruipreached the agent's PATH, the agent gotMOCK-UIP-SENTINEL 9.9.9back, and the invocation landed incli_mocks/calls.jsonl. A test now pins the SDK-side field, since the unit tests stubLocalAgentConfigand would all stay green if a version bump dropped it while mock CLIs silently stopped shadowing.Refreshes the model ids in docs examples (Sonnet 4-6 / Opus 4-7 / the dated 2025 ids → Sonnet 5 / Opus 5). The
anthropic.claude-sonnet-4-6values underllm_judgeandsimulationare left alone: those are the documented code default, and moving them changes verdicts rather than docs.Scope
Three things were built and then deliberately cut, each because it turned out to be its own design problem rather than a side note to the turn cap:
system_promptparity is fix(agent): append system_prompt to the Claude Code preset instead of replacing #92's, which is further along in review and additionally carries thesystem_prompt_modeescape hatch that judge sub-agents need. Harness config parity: a base-config field must mean the same thing on every harness #68 stays open until it lands.config_support(declare per-agent divergences, reject unhonored config at resolution) had no real user: every declaration came outapproximated, so the guard only ever fired for a synthetic test agent, and the one field that genuinely warrantedunhonoredcould not be declared without hard-erroring live nightlies.allowed_toolsmapping works, but the questions around it are not small: whether the config field should be spelled in Claude's tool vocabulary at all, and what an allowlist that maps to nothing should do (falling back to every tool hands the model more than it asked for, which is the wrong direction for an eval harness).Reference
docs/agents/HARNESS_PARITY.mdcarries the run-limit contract: what each field means per harness, what a capped run looks like, and the Antigravity backgrounding behavior. The measurements stay here rather than on the page, since they are a snapshot of one box on one day and the page is published to coder-eval.com.tasks/run_limits/holds the fixtures to re-measure with.🤖 Generated with Claude Code