fix(antigravity): poll for backgrounded work instead of grading it incomplete - #111
Conversation
…complete AntigravityAgent.communicate() used to drain the SDK's step stream once and finalize the instant it went idle, so a task Gemini backgrounds and pauses on (intending to check back later) got graded before the work finished. Now the turn polls (sleep + re-drain) while an orphaned tool call is still ACTIVE, bounded by _MAX_BACKGROUND_POLLS and the existing turn watchdog. Hardened across three review rounds: the orphan signal allowlists ACTIVE (not "not yet closed") so a tool stuck on WAITING_FOR_USER/CANCELED/UNKNOWN is never polled forever; the fallback synthetic tool-call id (when the SDK's call.id is falsy) is stable across a step's own ACTIVE->DONE re-emissions and unique across trajectories; the poll loop exits promptly once the watchdog has decided to fire; and _drain() retries past the SDK's real two-layer generator re-entrancy window after a cooperative stop instead of crashing the next turn. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
🔍 Reviewing PR #111Todo List:
|
|
I'll analyze this and get back to you. |
The module already imports asyncio at the top level; the local re-import inside test_communicate_poll_budget_exhausted_finalizes_via_existing_timeout_path was dead weight. The other CodeQL finding on this PR (an unreachable trailing yield in a fake receive_steps() that raises CancelledError first) is a deliberate, necessary idiom -- Python only classifies a function as an async generator if its body contains a yield anywhere, reachable or not, and the same pattern already exists twice, unflagged, on main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @joeysbase's task in 2m 2s —— View job Code Review in Progress
|
bai-uipath
left a comment
There was a problem hiding this comment.
Right fix at the right layer. One ask before merge: derive the hardcoded poll cap from the turn deadline. Everything below that is fix-if-you-agree.
- The cap is meant to be the safety valve, and it can never open. It exists so a never-resolving orphan still ends in a graded task: give up polling, force-close as UNRESOLVED, finalize COMPLETED. But it is 600s and
experiments/default.yamlsetsturn_timeout: 300, so the watchdog always wins the race and that path never executes. The real exit isTurnTimeoutError, which propagates out beforecheck_all_asyncand lands the task as ERROR with zero criteria evaluated. - For one input class that is a strict regression. A tool call left ACTIVE with no real background job behind it used to finalize immediately and get graded on whatever the agent wrote. Now it burns 300s and gets graded on nothing, scoring 0 on work that may well have been finished.
- That input class is already observed. The validation run saw two tool calls from an abandoned attempt that never got a terminal status, and the orphan signal cannot tell those apart from a live background job by design.
- Root cause is the unit. Every other deadline in the agents layer comes from
run_limits; this one is a second clock counted in cycles, meaning 600s only by accident of the 5s interval, and it cannot see the timeout it is competing with. - Fix: bound the loop at ~0.8 of
timeout, keep the cycle cap only fortimeout is None. Net deletion of both constants and the margin arithmetic defending them. The "an in-loop deadline races the watchdog" objection holds for a check against the same value, not a fraction of it, where the loop is meant to win.
Rest is optional:
- Drop the three
harness-candidates.mdentries. All three conclude "Not promoted", which is a decline filed in a promote-when-picked-up queue, and the one durable fact is already in a code comment and two regression tests. - Minor: the drain's
except RuntimeErroralso wraps step processing, so unrelated errors get five silent retries; budget exhaustion emits only a log line, so nobody can measure how often this fires; the poll tests patch the realasyncio.sleepinstead of a module seam.
Allowlisting ACTIVE is the right call, and the zero-cost fast path is well covered. Fix what you agree with, otherwise lgtm.
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:111 (3 files) axis:1,2,3,4,5,6,7,8
Scope: pr:111 (3 files) axis:1,2,3,4,5,6,7,8 · branch fix/antigravity-wait-for-wakeup (PR #111 head) · 3abe902 · 2026-08-13T02:19Z · workflow variant
Change class: complex — introduces a bounded poll/re-drain control-flow loop around the SDK step stream, new orphaned-tool-call state tracking, synthetic tool-call id derivation, and generator re-entrancy retry handling; correctness requires reasoning about concurrency, watchdog interaction, and turn finalization.
Architecture, security, and error handling are clean (10/10 each) and the PR's new poll loop, orphan predicate, and cid fallback are fully covered — but one real harness risk remains: the post-idle poll budget (600 s) is not derived from the effective turn_timeout (300 s baseline), so a never-closing ACTIVE tool call now raises TurnTimeoutError → FinalStatus.ERROR with criteria never graded where main produced a scored result, and it sits alongside soft debt (stale communicate() contract docs, an untested _drain retry-replay path, zero lifecycle-teardown coverage, and a communicate() that grew D27 → E33); bottom line: ship after the poll-budget fix, schedule the rest.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 9.2 / 10 | 0 | 0 | 1 | 3 | Poll loop inlined into communicate() pushes it from CC 27 (D) to 33 (E) (and _handle_tool_call past the CC-20 bar) despite the PR's own _drain() extraction |
| 2. Type Safety | 9.9 / 10 | 0 | 0 | 0 | 1 | Test helper _tc annotates tid: str but the four new call sites pass None, contradicting the SDK's `ToolCall.id: str |
| 3. Test Health | 8.8 / 10 | 0 | 0 | 2 | 2 | _drain's new bounded-retry loop has no test for a non-re-entrancy or mid-stream RuntimeError; the exhaustion branch is covered only incidentally |
| 4. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 5. Architecture & Design | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 6. Error Handling & Resilience | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 7. API Surface & Maintainability | 9.3 / 10 | 0 | 0 | 1 | 2 | communicate() docstring (antigravity_agent.py:520-522) still states the pre-fix "iterate until the turn goes idle" contract, and no user-facing doc covers the new post-idle poll |
| 8. Evaluation Harness Quality | 9 / 10 | 0 | 1 | 0 | 0 | Post-idle poll budget (120 x 5 s = 600 s) is not derived from turn_timeout: a never-closing ACTIVE tool call burns the whole 300 s turn timeout and lands FinalStatus.ERROR with criteria never graded (poll-cap branch unreachable) |
Overall Score: 9.5 / 10 · Weakest Axis: Test Health at 8.8 / 10
Totals: 🔴 0 · 🟠 1 · 🟡 4 · 🔵 8 across 8 axes.
Blockers
- [Axis 8] Post-idle poll budget (120 x 5 s = 600 s) is not derived from turn_timeout: a never-closing ACTIVE tool call burns the whole 300 s turn timeout and lands FinalStatus.ERROR with criteria never graded (poll-cap branch unreachable) (
src/coder_eval/agents/antigravity_agent.py:604) — The graceful exit at line 620 (if poll_count >= _MAX_BACKGROUND_POLLS and state.has_orphaned_tool_call():→ warn and finalize, turn is still graded) is unreachable under the shipped limits:_MAX_BACKGROUND_POLLS = 120(line 140) x_BACKGROUND_POLL_INTERVAL_SECONDS = 5.0(line 112) = 600s of polling, whileexperiments/default.yaml:31setsturn_timeout: 300(layer-1 baseline for every run) andtasks/agents/antigravity_hello_world.yamlsets its ownturn_timeout: 300/task_timeout: 360. TheThreadedWatchdogtherefore always wins first:state.timeout_hitflips at 300s,_finalize_and_raise_timeoutraisesTurnTimeoutError, which has NO dedicated handler inrun()(orchestrator.py:528only catchesTaskTimeoutError) and lands inorchestrator.py:566 except Exception:→orchestrator.py:568 self.result.final_status = FinalStatus.ERROR.check_all_async(orchestrator.py:1559) is never reached, sosuccess_criteria_resultsis empty and score is 0 — andmodels/enums.py:37 FinalStatus.ERROR: "error"puts the row in the harness-error bucket, not the"failed"bucket thatFinalStatus.TIMEOUT(models/enums.py:42) uses. On main the same trajectory finalized immediately and produced a scored FAILURE (or SUCCESS). The PR's own validation confirms the trigger is common, not pathological: "Two tool calls from an earlier, abandoned attempt never received a terminal status from the SDK at all ... the poll loop ran them out to the_MAX_BACKGROUND_POLLScap" — 600s of polling, which only producedSUCCESS score=1.000because that task's turn budget exceeded 600s. Blast radius: this is the production run path for antigravity tasks driven by the external coder-eval-uipath / eval-runner repo (the validation taskenergy-unit-commitmentis not in this repo'stasks/), and the PR body states no nightly impact;task.json's shape is unchanged, but thefinal_statusvalue distribution shifts fromFAILUREtoERROR, which that repo's dashboards bucket as harness breakage. Fix: derive the poll deadline from the effective turn timeout (stop polling attimeout - marginso the loop always exits through line 620's graceful finalize), or set_MAX_BACKGROUND_POLLS * _BACKGROUND_POLL_INTERVAL_SECONDSbelow the defaultturn_timeoutof 300s. Add a test that a never-resolving orphan withtimeout=300.0finalizes and is graded rather than raisingTurnTimeoutError.
Non-blocking, but please consider before merge
- [Axis 1] Poll loop inlined into communicate() pushes it from CC 27 (D) to 33 (E) (and _handle_tool_call past the CC-20 bar) despite the PR's own _drain() extraction (
src/coder_eval/agents/antigravity_agent.py:504) — Verified with radon against both revisions (git show origin/main:...>/tmp/base_ag.py,git show pr-111:...>/tmp/pr111_ag.py;uv run radon cc -s):
| block | main | PR HEAD | delta |
|---|---|---|---|
AntigravityAgent.communicate |
D (27) @ base:409 | E (33) @ pr:504 | +6 |
_AntigravityTurnState._handle_tool_call |
C (19) @ base:674 | D (21) @ pr:819 | +2 |
| module average | — | B / 6.44 | — |
Explicit answer to the routed question: this PR did NOT create communicate's >20 crossing — it was already D(27) on main; the PR deepened it by +6, making it the single most complex block in the module and now more complex than either sibling agent's equivalent (ClaudeCodeAgent.communicate D(30), _CodexTurnState._flush_message E(34) — repo-wide radon cc -n C src/coder_eval/). This PR DID create _handle_tool_call's crossing (19 → 21) by adding the call_index parameter plus the branch step_key = f"{trajectory_id}:{step.step_index}" if trajectory_id else str(step.step_index) (pr:836) and self._tool_last_status[cid] = sstatus (pr:838).
Filed 🟡 Medium, not 🟠: the Axis-1 🟠 anchor scopes CC>20 to the named hot modules (orchestrator, checker, sandbox), and agents/antigravity_agent.py is not one; per "when torn between two levels, pick the lower one".
The fix is mechanical and matches what the PR already did once. The author correctly extracted _drain() (pr:451) so the drain shape lives in one place — but then inlined the whole new poll phase (pr:577-623, of which 600-618 is the loop) directly into communicate, whose try block now carries send → initial drain → poll loop → budget warning → cooperative-cancel, under a ThreadedWatchdog context, wrapped by two nested exception ladders. Extract the block at pr:600-623 into a sibling of _drain, e.g.:
async def _poll_for_backgrounded_work(self, conversation, state, should_stop) -> None:
poll_count = 0
while (not state.stopped_early_hit and not state.timeout_hit
and state.has_orphaned_tool_call() and poll_count < _MAX_BACKGROUND_POLLS):
...and let communicate read await self._poll_for_backgrounded_work(conversation, state, should_stop). That also gives the 23-line rationale comment at pr:577-599 a docstring to live in (see the separate prose finding) and drops poll_count (pr:568) out of communicate's local state.
2. [Axis 3] _drain's new bounded-retry loop has no test for a non-re-entrancy or mid-stream RuntimeError; the exhaustion branch is covered only incidentally (tests/test_antigravity_agent.py:1041) — antigravity_agent.py:495 except RuntimeError: catches EVERY RuntimeError out of the try body — including one raised after steps were already fed to state.process_step(step) (:489) — and then re-calls conversation.receive_steps() from scratch up to _RECEIVE_STEPS_REENTRY_RETRIES times. The only test of the retry, test_communicate_recovers_from_transient_reentrancy_after_cooperative_stop:1041, raises before any step is yielded and asserts a single line (:1076-1077 tr = await agent.communicate("do it again"); assert tr.agent_output == "second turn"). I instrumented it: receive_steps_call_count goes 1 -> 4, i.e. 2 of the 5 retries are consumed — nothing pins that headroom. The exhaustion raise at :497 is reached only incidentally by the PRE-EXISTING test_communicate_crash_sets_pending_partial_turn:469-480 (raise RuntimeError("kaboom")), which was written before the retry existed and asserts nothing about it.
Fix: add (a) a test where the fake yields two usage-bearing steps and THEN raises RuntimeError, asserting the retried drain does not duplicate tr.commands or double tr.token_usage; and (b) a test that a permanently-raising receive_steps() surfaces AgentCrashError after exactly _RECEIVE_STEPS_REENTRY_RETRIES calls (monkeypatch the constant low, as the cap test already does at :852), plus an assertion in the reentrancy test that receive_steps_call_count > 2 so it proves a retry actually happened.
3. [Axis 3] AntigravityAgent lifecycle teardown (stop/kill/kill_sync/_teardown/_conversation_or_none/get_environment_info) has zero test coverage, and _teardown() suppresses harness teardown failures with no log (src/coder_eval/agents/antigravity_agent.py:677) — Coverage of this module is 84.66% (370 stmts, 48 miss). Mapping the missing lines: 282/285/302->300/312 (_resolve_skills_paths error branches), 367-368/404-406/418-420 (start()'s SDK-import and thinking-level paths), 634/637/644/656-666 (outer exception handlers), 911/922/949/977/996 — all PRE-EXISTING. The single NEW-in-this-PR gap is 485->exit, an unreachable for-loop-exit branch in _drain (every iteration returns at :494 or raises at :497), i.e. a coverage artifact, not a real gap — the PR's new poll loop (:575-622), has_orphaned_tool_call (:951-968), and the cid fallback (:835-836) are all fully covered, which is genuine credit.
The largest uncovered contiguous block is pre-existing and is the Agent-ABC teardown surface: 679-680 (stop() -> await self._teardown() / self._mark_stopped()), 684-688 (kill()), 698 (kill_sync()), 708-713 (_conversation_or_none), 717-722 (_teardown). The sibling agent has this covered (tests/test_codex_agent.py:1517 test_kill_sync_interrupts_turn_and_closes, :1826 test_kill_sync_cleans_login_home); tests/test_timeout_orchestrator.py:216 only asserts on a MagicMock, so nothing verifies AntigravityAgent's kill_sync (:690) stays synchronous and non-raising for the watchdog's non-asyncio thread.
Fix: add the Codex-equivalent trio to tests/test_antigravity_agent.py — stop() closes the exit stack and leaves get_state() == AgentState.STOPPED; kill() calls conversation.cancel() then tears down; kill_sync() is a plain def, returns None, and does not raise when _sdk_agent is None.
4. [Axis 7] communicate() docstring (antigravity_agent.py:520-522) still states the pre-fix "iterate until the turn goes idle" contract, and no user-facing doc covers the new post-idle poll (src/coder_eval/agents/antigravity_agent.py:520) — The docstring at lines 520-522 still reads: "Drives one logical turn: conversation.send(prompt) then iterate receive_steps() until the turn goes idle, mapping the Gemini step stream onto the standardized event protocol." That is now false — the turn continues past idle via the poll loop at lines 600-618 whenever state.has_orphaned_tool_call() is true. The PR also changes no docs: docs/agents/ANTIGRAVITY.md (the user-facing agent guide, whose "Known limitations" section at lines 168-186 lists exactly four items of this shape) says nothing about backgrounded work, the poll, or its interaction with run_limits. Two user-visible consequences go unexplained anywhere: (a) a turn can now sit for minutes emitting no events (the poll only logs at DEBUG, line 607), so a run looks hung; (b) both shipped example tasks pin turn_timeout: 300 (tasks/agents/antigravity_hello_world.yaml and ..._docker.yaml), which is below the 600 s poll budget — so on an orphaned-tool turn the watchdog now fires where the turn previously finalized immediately, changing the terminal outcome for identical agent output. Fix: update the docstring to state that the turn polls past stream-idle while a tool call is ACTIVE, and add a "Known limitations" entry to docs/agents/ANTIGRAVITY.md naming the bound and telling users to size run_limits.turn_timeout/task_timeout for backgrounded jobs.
Nits
- [Axis 1] Rationale prose dominates the diff (~66% of added lines are comments/docstrings), including a 23-line inline block in communicate() restating what the named constants already carry (
src/coder_eval/agents/antigravity_agent.py:101) — Counted, not estimated:git diff origin/main...pr-111 -- src/coder_eval/agents/antigravity_agent.py | grep -c '^+'= 190 added lines;| grep -cE '^\+\s*#'= 82#lines; plus ~43 docstring lines (pr:457-483 and pr:952-967). Concrete ratios at PR HEAD:
- pr:101-140 — 35 comment lines for 3 constant assignments (
_BACKGROUND_POLL_INTERVAL_SECONDS = 5.0at 112,_RECEIVE_STEPS_REENTRY_RETRIES = 5at 120,_MAX_BACKGROUND_POLLS = 120at 140). - pr:577-599 — 23 consecutive comment lines immediately above a 24-line loop (600-623).
- pr:451-501 —
_drain: 27-line docstring (457-483) over a 17-line body (485-501). - pr:951-968 —
has_orphaned_tool_call: 16-line docstring (952-967) over a one-line body:return any(cid not in self._closed_tools and s == _STATUS_ACTIVE for cid, s in self._tool_last_status.items()).
The SDK facts here are genuinely non-obvious and worth recording (wait_for_wakeup() is a stub; receive_steps() is two nested generators). The problem is the argumentative register — the comments litigate against a hypothetical reviewer rather than explain the code: pr:110-111 "Not user-configurable: a tuning constant, not a feature."; pr:118-119 "this constant carries a 2.5x margin, not a separately-tuned budget."; pr:129-139 "Deliberately NOT 'break after N consecutive empty polls' instead: ..." (an 11-line rebuttal of a design not taken); pr:964-967 "The not in _closed_tools guard is layered on top (not a substitute) purely as a monotonicity backstop...".
Same pattern in the in-scope test file, where shipped docstrings cite a review thread a future reader cannot see: tests/test_antigravity_agent.py:553 "(Phase-2-review finding)", :555 and :714 "(final-review finding)", :758 and :1046 "(round-3 review finding)", :1087 "per round-3 review".
Recommendation: keep the ~8 lines that state observed SDK behavior next to the code that depends on it; move the design-alternatives-rejected and review-history material to the PR description and .claude/harness-candidates.md (which this PR already updates for exactly that purpose), and strip (round-3 review finding) / (final-review finding) / (Phase-2-review finding) from the seven test docstrings — the assertion itself is the durable record.
2. [Axis 1] New _tool_last_status is a fifth parallel cid-keyed collection typed dict[str, Any], leaving the ACTIVE comparison unchecked where a set of active ids would express the predicate (src/coder_eval/agents/antigravity_agent.py:785) — _AntigravityTurnState.__init__ now carries five collections keyed on the same cid: _seen_tools (pr:775), _closed_tools (pr:776), _open_tools (pr:777), _tool_input_keys (pr:781), and the new self._tool_last_status: dict[str, Any] = {} (pr:785). It is written on every call at pr:838 (self._tool_last_status[cid] = sstatus), never pruned (closed tools' entries live for the whole turn), stores the raw status as Any, and has exactly one consumer, which then needs a two-condition scan plus a documented "backstop" clause (pr:968):
return any(cid not in self._closed_tools and s == _STATUS_ACTIVE for cid, s in self._tool_last_status.items())The state the poll loop actually needs is a single membership question. Track it directly instead:
self._active_tools: set[str] = set() # in __init__
...
(self._active_tools.add if sstatus == _STATUS_ACTIVE else self._active_tools.discard)(cid) # in _handle_tool_call
...
def has_orphaned_tool_call(self) -> bool:
return bool(self._active_tools)This drops the Any, removes the not in _closed_tools backstop (a DONE/ERROR re-emission discards the id by construction, so closure is automatically authoritative), removes the per-check O(n) dict scan, and makes the allowlist-on-ACTIVE semantics the shape of the data rather than a 16-line docstring. It also keeps the container count from growing: _seen_tools is already fully redundant with _open_tools (both are populated only at pr:823/832 and never removed from) — pre-existing, not this PR's doing, but the direction of travel is worth reversing rather than extending.
3. [Axis 1] .claude/harness-candidates.md entries describe fixes that differ from the code this same PR ships (.claude/harness-candidates.md:327) — Both new notes under ## From the coder-eval-code-review of fix/antigravity-wait-for-wakeup (2026-08-12) (line 318) under-describe the shipped fix, so the deferred-harness log is already stale on arrival:
-
Line 327 says the fallback id was
"Fixed by deriving the fallback from(step.step_index, call_index)instead". The shipped code at antigravity_agent.py:836-837 also keys ontrajectory_id:trajectory_id = getattr(step, "trajectory_id", "") or "" step_key = f"{trajectory_id}:{step.step_index}" if trajectory_id else str(step.step_index) cid = call.id or f"{raw_name}_{step_key}_{call_index}"
That component is not incidental — this PR adds a dedicated test for it (tests/test_antigravity_agent.py:751
test_id_less_tool_calls_in_different_trajectories_do_not_collide, asserting{"run_command_1_0", "run_command_subagent-42:1_0"}), becausestep_indexalone collides across a sub-agent trajectory. As written, the note documents the intermediate fix that the same PR then superseded. -
Line 343 says
"Fixed by addingand not state.timeout_hitto the condition."— that is half the shipped fix. The loop also gained a mid-body early exit at antigravity_agent.py:609-614 (if state.timeout_hit: ... break) whose whole purpose is to skip the re-drain after the sleep, and which has its own test (tests/test_antigravity_agent.py:900test_communicate_poll_loop_exits_promptly_once_watchdog_flag_lands, assertingreceive_steps_call_count == 2). Omitting it makes the note's "not promoted to a CExxx rule" rationale rest on an incomplete description of the shape.
Update both sentences to match pr HEAD (mention trajectory_id in #1; mention the post-sleep break in #2).
4. [Axis 2] Test helper _tc annotates tid: str but the four new call sites pass None, contradicting the SDK's ToolCall.id: str | None (tests/test_antigravity_agent.py:233) — Line 233 is def _tc(name: str, tid: str, args: dict) -> SimpleNamespace:, but this PR adds four call sites that pass None for tid — lines 724, 736, 765 and 773 (e.g. line 724: tool_calls=[_tc("run_command", None, {"command_line": "sleep 12 && echo done"})]). Passing None is the correct test behavior: the wheel confirms ToolCall.id: str | None = None (google/antigravity/types.py:454), which is exactly the optionality the new synthetic-cid fallback at antigravity_agent.py:834 (cid = call.id or f"{raw_name}_{step_key}_{call_index}") exists to handle. The annotation is what is wrong, and because pyright excludes tests/ nothing flags it, so the helper's signature now actively misdocuments the SDK contract these tests were written to pin. Fix: def _tc(name: str, tid: str | None, args: dict[str, Any]) -> SimpleNamespace:.
5. [Axis 3] _WatchdogFiresLater.captured_on_timeout is class-level mutable state shared by two tests and never reset (tests/test_antigravity_agent.py:886) — 886: captured_on_timeout: Callable[[], None] | None = None is written from __init__ (889: _WatchdogFiresLater.captured_on_timeout = on_timeout) by both test_communicate_poll_loop_exits_promptly_once_watchdog_flag_lands:898 and test_communicate_poll_budget_exhausted_finalizes_via_existing_timeout_path:1080, and is never cleared. It therefore outlives each test holding a live closure over a finished turn's _AntigravityTurnState. Today both readers instantiate the class first so the value is fresh, but a future test that reads captured_on_timeout without a preceding communicate() would silently flip timeout_hit on a dead turn state and pass/fail for the wrong reason. Fix: reset it in an autouse fixture (or return the callback via a per-test list/monkeypatch.setattr on an instance) rather than parking it on the class.
6. [Axis 3] Two fast-path tests replace the global asyncio.sleep with a raising stub, so any incidental sleep fails them with a misleading message (tests/test_antigravity_agent.py:595) — antigravity_agent.py does a plain import asyncio (:21), so antigravity_agent.asyncio is sys.modules["asyncio"] — 595: monkeypatch.setattr(antigravity_agent.asyncio, "sleep", _sleep_should_not_be_called) (and the identical :626) swaps the stdlib asyncio.sleep process-wide for the test, with a body that raises AssertionError("asyncio.sleep must not be called on the no-orphan fast path"). Patching global asyncio.sleep is an established convention here (tests/test_orchestrator.py:1785, tests/test_retry_logic_comprehensive.py:63), but those install a no-op; a RAISING stub means _drain's own re-entrancy yield (antigravity_agent.py:502 await asyncio.sleep(0)) — or any sleep in an awaited library frame — fails the test with a message that misdescribes the cause, and the stub stops proving "the poll loop was not entered". The real contract is already asserted one line later (:597 assert conv.receive_steps_call_count == 1). Fix: make the stub a no-op recorder and assert sleep_calls == [], or scope the patch to the poll interval by asserting no call carries _BACKGROUND_POLL_INTERVAL_SECONDS.
7. [Axis 7] "Orphaned tool call" now means two different things inside _AntigravityTurnState (src/coder_eval/agents/antigravity_agent.py:951) — has_orphaned_tool_call() (line 951) returns any(cid not in self._closed_tools and s == _STATUS_ACTIVE for cid, s in self._tool_last_status.items()) (line 968) — an ACTIVE-only allowlist — while finalize() 20 lines later documents itself as "Close orphaned tools, flush leftover blocks, emit TurnEnd + AgentEnd" (line 971) and implements the broader sense: for cid, tel in self._open_tools.items(): if cid in self._closed_tools: continue (lines 981-983), i.e. any not-yet-closed tool regardless of status. So a WAITING_FOR_USER tool is "orphaned" to finalize() but deliberately not to has_orphaned_tool_call(). The (excellent) docstring at lines 956-966 explains the allowlist rationale, but the name is what the poll-loop call site reads (line 603). Rename the predicate to what it actually tests — e.g. has_active_background_tool_call() — or add a one-line cross-reference in finalize()'s docstring noting the two senses are intentionally different.
8. [Axis 7] .claude/harness-candidates.md: three "Not promoted" decisions are filed as open - [ ] TODOs, against the file's own resolved-item convention and the rubric's one-line deferral format (.claude/harness-candidates.md:318) — All three new entries under "## From the coder-eval-code-review of fix/antigravity-wait-for-wakeup (2026-08-12)" (lines 318-362) are explicit decisions not to add a guard — "Not promoted to a CExxx rule: this is the only id-fallback-driving-control-flow site in the codebase today" (line 328), "Not promoted: ThreadedWatchdog + a bespoke poll loop … no second instance exists to generalize a rule from" (line 344), "Not promoted: detecting "a test double is missing a delegation layer the source has" is a semantic match against third-party source" (line 359) — yet each is written as an unchecked - [ ] item in a file whose header says "Deferred lint/test guardrails … Promote to a CExxx rule (or a test) when picked up" (lines 3-4). The file already has a convention for items that are no longer pending: - [x] ~~**taskCarriesRepoTag …**~~ **RESOLVED in PR #94 review round 2.** (line 156). Every future reader of this queue will re-read ~45 lines to re-derive "nothing to do here". Note also that .claude/shared/review-rubric.md § Harness loop prescribes one line per deferral (- [ ] <rule> — <why nothing guards it today> — caught in <context>) and says a non-gap should be skipped rather than recorded. Either mark these - [x] under a "Considered and declined" heading, or compress each to one line.
What's Missing
Tests:
- 🟡 🟡 Token/reconciliation invariant is never asserted on a multi-drain turn. The CLAUDE.md invariant ("summing the four token buckets across
TurnRecord.messagesequalstoken_usageexactly") is pinned only in the pre-existing single-drain test (tests/test_antigravity_agent.py:400-413); every new polled test (:646poll-and-resume,:787two sequential background jobs) asserts onlyresult_status/agent_output/receive_steps_call_count. The poll path now foldsusage_metadatafrom N separatereceive_steps()cycles throughprocess_step→_flush_generation, so a re-drain that re-emits a usage-bearing DONE step would double-count tokens and cost intask.jsonwith the whole suite green. Addassert tr.token_usage.output_tokens == …plus the per-bucket message-sum assertions totest_communicate_polls_and_resumes_after_orphaned_tool_closes. (trigger: tests/test_antigravity_agent.py) - 🟠 🟠 No orchestrator-level test that a polled turn under a realistic
turn_timeoutis still graded. Every new poll test drivescommunicate()directly withtimeout=Noneor a monkeypatched watchdog; none exercises the shipped configuration (experiments/default.yaml:31 turn_timeout: 300vs. the 600 s poll budget), where a never-closing ACTIVE tool now raisesTurnTimeoutError→FinalStatus.ERRORwithcheck_all_asyncnever reached. A test at theOrchestratorlevel (or anEvaluationResult-shaped assertion) is what would have caught the un-derived budget before merge. (trigger: tests/test_antigravity_agent.py) (restates: Axis 8: Post-idle poll budget (120 x 5 s = 600 s) is not derived from turn_timeout) - 🟡 🟡
_drain's new bounded-retry loop ships with one happy-path test and no mid-stream / exhaustion coverage.antigravity_agent.py:495catches everyRuntimeErrorout of the drain — including one raised after steps were fed tostate.process_step— and restartsreceive_steps()from scratch, yet no test raises mid-stream (would prove replay does not duplicateagent_output/ token totals) and none pins the_RECEIVE_STEPS_REENTRY_RETRIESbound. (trigger: src/coder_eval/agents/antigravity_agent.py) (restates: Axis 3:_drain's new bounded-retry loop has no test for a non-re-entrancy or mid-stream RuntimeError)
Downstream consumers:
- 🟡 🟡
EarlyStopWatcher's UNRESOLVED-orphan carve-out was written for the pre-fix world and was not revisited.orchestration/early_stop.py:456-461deliberately records but never counts or evaluates on an UNRESOLVEDToolEndEvent, because a backgrounded antigravity tool ALWAYS force-closed that way atfinalize(). Polling now resolves those same calls into realToolEndEvents, so on an armed antigravity task a backgroundedrun_commandnow increments_tool_call_index(thedecide_withinstep budget) and runs_evaluate_impl— it can fire a live pass-stop or fail-stop where it structurally could not before. Neitherearly_stop.pynortests/test_early_stop.pywas touched, and no test covers an armed criterion deciding on a polled tool result. (trigger: src/coder_eval/agents/antigravity_agent.py) - 🔵 🔵 Turn duration now silently absorbs idle poll time, and no consumer was given a way to separate it.
finalize()reportsduration_seconds = time.monotonic() - self.turn_start_time(antigravity_agent.py:1024), which now includes up to 600 s ofasyncio.sleepwaiting on a background job. That value feedsreports.py:302's avg-turn-duration column and the evalboard duration aggregations (evalboard/lib/trends.ts:167,overview.ts:883), so antigravity turn-latency series get a step change with background wait indistinguishable from agent compute. Nopoll_seconds/ background-wait field was added toTurnRecordto let the reports split them. (trigger: src/coder_eval/agents/antigravity_agent.py)
Display & mapping dicts:
- 🟡 🟡 The hand-mirrored
StepStatusconstants got a new load-bearing consumer but no parity guard._STATUS_ACTIVE/_DONE/_ERROR(antigravity_agent.py:187-189) mirrorgoogle.antigravity.types.StepStatusas plain strings, and the poll loop's ACTIVE-only allowlist now depends on both the spelling AND the closed-world assumption that no other member denotes in-flight background work — WAITING_FOR_USER / CANCELED / UNKNOWN are enumerated only in prose (:956-966) and re-hardcoded as string literals in the test (tests/test_antigravity_agent.py:569). Nothing ties the mirror to the pinned SDK enum (google-antigravity==0.1.7,pyproject.toml:107) the wayevalboard/lib/__tests__/pricing-parity.test.tsdoes for the pricing table, so a dependabot bump that renamesACTIVEor adds aPENDING/BACKGROUNDmember silently reverts this entire fix to the fast path with the suite green. Add animportorskip-guarded parity test asserting the three constants are members ofStepStatusand that the full member set is exactly the six known values. (trigger: src/coder_eval/agents/antigravity_agent.py)
Daily/nightly:
- 🟡 🟡 Unstated baseline shift for the external eval-runner: backgrounded tool calls flip from
"unknown"to"success". Before this PR a backgroundedrun_commandwas force-closed byfinalize()asresult_status="unknown"(antigravity_agent.py:981-983, asserted attests/test_antigravity_agent.py:641); with polling it now resolves to"success".command_executedgates exactly on that value (criteria/command_executed.py:173 if criterion.require_success and cmd.result_status != "success"), andcommands_efficiency/skill_triggeredread the same trajectory — so task pass/fail, suite P/R/F1, and the evalboard regression tags shift on precisely the antigravity tasks this fix targets. That is the intended effect, but the PR never states the expected baseline movement for the nightly consumed by the externalcoder-eval-uipath/ eval-runner repo. (trigger: src/coder_eval/agents/antigravity_agent.py) - 🔵 🔵 Nightly wall-clock / throughput impact of the poll is unstated. A turn that previously finalized the instant the stream went idle can now hold a
run_batchworker for the fullrun_limits.turn_timeout(or 600 s where none is configured) on every orphaned-tool turn — the PR's own validation notes a task that "ran them out to the_MAX_BACKGROUND_POLLScap". Neither the PR body nordocs/agents/ANTIGRAVITY.mdstates the resulting per-cycle duration/cost change or advises sizingtask_timeoutfor parallel nightly batches. (trigger: src/coder_eval/agents/antigravity_agent.py)
Parallel paths:
- 🟡 🟡 The user-facing agent guide is the parallel surface of the changed docstring and was not updated.
docs/agents/ANTIGRAVITY.mdis untouched: its "Known limitations" list (lines 169-183, four items of exactly this shape) says nothing about the post-idle poll, its 600 s bound, the minutes-long event-silent window it creates (the loop logs only at DEBUG,antigravity_agent.py:607), or the need to sizerun_limits.turn_timeout/task_timeoutabove the background job — while both shipped example tasks pinturn_timeout: 300, below the poll budget. (trigger: src/coder_eval/agents/antigravity_agent.py) (restates: Axis 7:communicate()docstring still states the pre-fix "iterate until the turn goes idle" contract) - 🔵 🔵 The cross-agent cooperative-stop seam file was not extended for the PR's second
should_stopsite.tests/test_early_stop.pyis the SSOT for the "stop cuts the trajectory at the deciding step" contract across Claude / Codex / Antigravity (TestAntigravityCooperativeStopSeam:3127), but the new stop check inside the poll loop (antigravity_agent.py:615) is covered only agent-locally (tests/test_antigravity_agent.py:945), and that file's fake steps (_ag_step,_CountingConversation) carry no tool calls, so the seam suite never reaches the poll path at all. (trigger: src/coder_eval/agents/antigravity_agent.py)
Harness & Lint Improvements
Static checks (lint / type):
- [ce-lint] CE035 — an agent's internal sleep-loop budget must be derived from the effective turn timeout (or provably fit inside the shipped baseline). New whole-tree
@pytest.mark.lintclass intests/test_custom_lint.py(same wiring as CE026/CE034 — it reads YAML as well as Python AST, so it is not aBaseRuleintests/lint/runner.py). For everyWhile/Fornode insrc/coder_eval/agents/**whose body containsawait asyncio.sleep(<name>): either (a) the loop test references a deadline/timeout-derived name (a parameter, not a module constant), or (b)interval_constant * bound_constant(both resolved from module-levelast.Assignliterals) is strictly less thanturn_timeoutinexperiments/default.yaml— otherwise the loop's own cap branch is unreachable under the layer-1 baseline and the watchdog always wins first. Measured baseline cost onmain: zero violations — the only existing agent sleep-loop iscodex_agent.py:1787(attempts: int = 20xasyncio.sleep(0.1)= 2 s << 300 s), which passes. On PR HEAD it fires onantigravity_agent.py:600(_MAX_BACKGROUND_POLLS = 120x_BACKGROUND_POLL_INTERVAL_SECONDS = 5.0= 600 s vs.experiments/default.yaml:31 turn_timeout: 300). Prevents: The single 🟠 High finding (antigravity_agent.py:604 — post-idle poll budget not derived from turn_timeout; the graceful cap branch at :620 is dead code under the shipped 300 s baseline, so a never-closing ACTIVE tool call lands FinalStatus.ERROR with criteria never graded instead of a scored FAILURE). Also pre-empts the same shape in any future agent. - [ruff] Enable
C901with[tool.ruff.lint.mccabe] max-complexity = 18(add"C90"toselectin pyproject.toml). This is the exact precedent already established forPLR0915/PLR0912(max-statements = 80,max-branches = 25, with the comment "gates NEW growth past these bounds; a god-function must carry a visible# noqadebt marker") — complexity is the one dimension of that trio not currently gated. Measured: PR HEAD'sAntigravityAgent.communicateis mccabe 19 > 18 (fires);origin/main's same function is 17 (passes), so the threshold gates exactly this PR's growth. Baseline cost onmain= 5 functions needing a# noqa: C901debt marker:claude_code_agent.py:881 communicate(21),codex_agent.py:957 _setup_skills(19),isolation/docker_runner.py:1070 _build_argv(29),orchestrator.py:1798 _simulation_dialog_loop(22),reports_experiment.py:589 generate_variant_report(24). The finding's own prototype extraction (_poll_for_backgrounded_work) drops the function back under the bar (radon E33 -> D24). Prevents: 🟡 Medium:communicate()pushed from CC 27 (D) to 33 (E) and_handle_tool_callfrom 19 to 21, despite the PR's own_drain()extraction. Would have forced the extraction at authoring time rather than at review time. - [pyright] Type-check
tests/under a second, narrow pyright project. Addpyrightconfig.tests.jsonwithinclude: ["tests"],typeCheckingMode: "off", and onlyreportArgumentType/reportCallIssueset toerror; wire it asmake typecheck-testsand add it tomake verify+ the CI typecheck job. Verified against PR HEAD: it reportsArgument of type "None" cannot be assigned to parameter "tid" of type "str" in function "_tc"at tests/test_antigravity_agent.py 724, 736, 765, 773 — precisely the finding. Adoption must be incremental, not big-bang: measured today the suite has 675 errors across 82 of 240 test files (652reportArgumentType, 23reportCallIssue), so ship it with those 82 files listed inexcludeas an explicit shrinking baseline (a file drops off the list when it is cleaned), so every new or rewritten test file lands clean.tests/is currently in pyright'sexcludelist, which is exactly why nothing flagged this. Prevents: 🔵 Low (Axis 2): test helper_tc(tid: str)annotated non-Optional while four new call sites passNone, misdocumenting the SDK's realToolCall.id: str | Nonecontract that the new synthetic-cid fallback exists to handle. Would also catch the whole class of test doubles whose signatures drift from the source types they stand in for. - [ce-lint] CE036 —
kill_syncmust be synchronous and await-free on everyAgentsubclass. NewBaseRuleintests/lint/rules/ce036_kill_sync_is_sync.py, wired intoALL_RULESintests/lint/runner.py. For any class whose bases includeAgent(or a known agent base) that defineskill_sync: the node must beast.FunctionDef(notAsyncFunctionDef), must contain noAwait/AsyncWith/AsyncFor, and must not callasyncio.run/get_event_loop().run_until_complete. Zero violations onmain(all four agents already comply) — it pins the contract, which is currently pinned nowhere. This is the statically-reachable half of the teardown-coverage finding; the behavioural half (it must also not raise) goes to the harness bucket. Prevents: 🟡 Medium (Axis 3/6): AntigravityAgent's entire lifecycle surface (stop/kill/kill_sync/_teardown/_conversation_or_none/get_environment_info, lines 677-722) has zero test coverage, andkill_syncis invoked from the watchdog's non-asyncio timer thread — nothing today fails if it is changed to await. - [ce-lint] CE037 — agent tuning constants must appear in that agent's user-facing guide. New doc-parity
@pytest.mark.lintclass, shaped exactly like CE027 (doc/env-var parity) and CE030 (doc/schema parity). For eachsrc/coder_eval/agents/<name>_agent.py, every module-level constant matching_*(SECONDS|INTERVAL|TIMEOUT|POLLS|RETRIES|MAX_*)must be named verbatim indocs/agents/<NAME>.md, or carry an# EXEMPT: <reason>comment. Measured baseline cost onmain: zero — no agent module currently defines such a constant. On PR HEAD it fires on all three new constants (_BACKGROUND_POLL_INTERVAL_SECONDS,_RECEIVE_STEPS_REENTRY_RETRIES,_MAX_BACKGROUND_POLLS), forcing the 'Known limitations' entry the finding asks for. The complementary docstring-staleness half is not statically reachable (whether prose is now false is semantic) — see the harness bucket. Prevents: 🟡 Medium (Axis 7): the new 600 s post-idle poll is documented nowhere user-facing —docs/agents/ANTIGRAVITY.mdis untouched, so users get no guidance that a turn can now sit for minutes emitting only DEBUG logs, nor that both shipped example tasks pinturn_timeout: 300below the poll budget. - [ce-lint] CE038 — cap consecutive full-line comment runs at 20 in
src/coder_eval/(line-based, no AST needed;# noqa: CE038as the debt marker, matching the PLR0915 convention). Measured: longest run onmainis 22 (orchestration/early_stop.py:643) and the next is 20 (codex_agent.py:1282), so a> 20bar means exactly one pre-existing noqa. PR HEAD has a 23-line run atantigravity_agent.py:577(immediately above a 24-line loop) plus an 18-line run at :122 — against a pre-PR maximum of 9 in that same file. The rule does not judge whether a comment is useful; it forces prose past a threshold into a docstring or the PR description, which is exactly the finding's recommendation. Prevents: 🔵 Low (Axis 1/7): ~66% of the diff's added lines are comments/docstrings, including the 23-line inline block at :577-599 restating what the named constants already carry, an 11-line rebuttal of a design not taken, and a 27-line docstring over a 17-line body. - [ce-lint] CE039 — no review-round provenance in shipped source or test prose. Regex rule over
src/andtests/docstrings/comments forbiddinground-\d+ review,final-review finding,Phase-\d+-review,\(.*review finding\). Cheap (line-based), zero baseline cost to verify before landing. Would flag tests/test_antigravity_agent.py 553, 555, 714, 758, 1046, 1087. The rationale is durable: the assertion is the record; a reader six months from now cannot see 'round 3' of a closed PR thread. Prevents: 🔵 Low (Axis 1/7): seven test docstrings shipped citing an invisible review thread ('(Phase-2-review finding)', '(final-review finding)', '(round-3 review finding)', 'per round-3 review'). - [ce-lint] CE040 —
.claude/harness-candidates.mdcheckbox convention. Doc-surface@pytest.mark.lintrule: any entry whose body containsNot promotedorRESOLVEDmust not be an open- [ ]item (the file already establishes the closed form at line 156:- [x] ~~**...**~~ **RESOLVED in PR #94...**), and every open- [ ]entry must be a single line matching the deferral shape prescribed in.claude/shared/review-rubric.md§ Harness loop (- [ ] <rule> — <why nothing guards it today> — caught in <context>). Measured: 21 open- [ ]entries onmainto audit once; PR HEAD adds three multi-paragraph 'Not promoted' decisions filed as open TODOs (lines 318-362). Prevents: 🔵 Low (Axis 7): three explicit decisions not to add a guard are filed as open work items, so every future reader of the queue re-reads ~45 lines to re-derive 'nothing to do here'. - [ce-lint] CE041 — do not monkeypatch stdlib
asyncio.sleepthrough a module alias in tests. AST rule: forbidmonkeypatch.setattr(<anything>.asyncio, "sleep", ...)(andsetattr(asyncio, "sleep", ...)); direct callers to a sharedrecording_sleepconftest fixture that records durations and returns immediately. Becauseantigravity_agent.py:21does a plainimport asyncio,antigravity_agent.asyncio is sys.modules['asyncio']— the patch is process-wide for the test, and a raising stub then turns any incidental sleep (e.g._drain's ownawait asyncio.sleep(0)re-entrancy yield at :502) into a failure whose message misdescribes the cause. Baseline: 2 sites in the PR (tests/test_antigravity_agent.py:595, :626) plus the existing no-op patches at tests/test_orchestrator.py:1785 and tests/test_retry_logic_comprehensive.py:63, which the fixture subsumes. Prevents: 🔵 Low (Axis 3): two fast-path tests replace globalasyncio.sleepwith anAssertionError-raising stub, so any incidental sleep fails them with a misleading message — and the stub stops proving the thing it was written to prove (the real contract is already asserted one line later byreceive_steps_call_count == 1). - [ce-lint] CE042 — a test double must not assign to its own class attribute from an instance method. AST rule over
tests/: inside a method of classC, anAssignwhose target isAttribute(value=Name('C'))is a violation (use an instance attribute, a per-test list, ormonkeypatch.setattron the instance). Catches the general shape of leaked cross-test state, which is otherwise invisible until an unrelated test starts failing. Prevents: 🔵 Low (Axis 3):_WatchdogFiresLater.captured_on_timeout(tests/test_antigravity_agent.py:886) is written from__init__(:889) by two tests and never reset, so it outlives each test holding a live closure over a finished turn's_AntigravityTurnState. - [ce-lint] CE043 — no
Anyin annotated instance-attribute declarations undersrc/coder_eval/agents/**. AST rule: anAnnAssignwhose target isself.<x>and whose annotation subtree containsAnyis a violation unless it carries# noqa: CE043. Rationale: agent modules are the boundary where third-party SDK objects enter, so anAnystate field is exactly where a wrong-typed value survives to a consumer. Note the honest cost/benefit: 12 pre-existing sites would need noqa markers (7 incodex_agent.py, 4 inclaude_code_agent.py, 1 new inantigravity_agent.py), and the rule only reaches the typing half of the finding — the design half (five parallelcid-keyed collections where aset[str]of active ids expresses the predicate) is not statically detectable and stays a review judgment. Note also that pyright cannot do this natively: standard pyright has noreportExplicitAny(that is basedpyright), and ruff'sANN401covers parameters/returns only. Prevents: 🔵 Low (Axis 1/2/5): the newself._tool_last_status: dict[str, Any]is a fifth parallelcid-keyed collection, stores the raw SDK status unchecked, is never pruned, and needs a 16-line docstring plus a two-condition scan for what is a single membership question.
Harness improvements (not statically reachable):
- Shared agent-lifecycle conformance suite, parameterized over
AgentRegistry. Addtests/test_agent_contract.pythat enumerates registered agent types and runs one common body against each:stop()closes the exit stack and leavesget_state() == AgentState.STOPPED;kill()cancels the conversation then tears down;kill_sync()returnsNoneand does not raise when the SDK handle isNone; a mid-turn crash leaves acrashed=Truepending_turnthatdiscard_pending_turn()clears. Today this exists only for Codex (tests/test_codex_agent.py:1517, :1826);tests/test_timeout_orchestrator.py:216assertskill_synconly against aMagicMock. A registry-driven suite means the next agent (in-tree or plugin) inherits the coverage instead of re-litigating it. Why not static: CE036 can provekill_syncis a plaindef, but 'does not raise when there is no live conversation' and 'the exit stack is actually closed' are runtime states — they need an instantiated agent with a fake SDK conversation and an assertion on post-call state. Prevents: The 🟡 Medium teardown-coverage gap: AntigravityAgent lines 677-722 (measured 86.13% module coverage, 43 missed statements against the whole suite; the missed set679-680, 684-688, 698, 702, 708-713, 717-722is byte-identical file-scoped and suite-scoped). - Add a behavioural budget case to that same conformance suite: a never-resolving backgrounded tool call must produce a graded turn, not a harness error. With the shipped constant ratio preserved and the wall clock scaled (poll interval monkeypatched low, real
ThreadedWatchdog), assert thatcommunicate(timeout=T)returns aTurnRecordand the run reachescheck_all_async— i.e. the terminalfinal_statuslands in the"failed"bucket, neverFinalStatus.ERROR. Pair it with an orchestrator-level guard thatTurnTimeoutErrormaps toFinalStatus.TIMEOUT(category"failed"), sinceorchestrator.py:528catches onlyTaskTimeoutErrorand everything else falls to:566 except Exception->:568 FinalStatus.ERROR, which downstream dashboards bucket as harness breakage. Why not static: CE035 pins the constants' arithmetic relationship; it cannot observe the terminalfinal_statusa real turn produces. Reproducing this needs the watchdog's timer thread, a fake step stream, and the full orchestrator exception ladder — the reviewer had to execute a scaled repro to confirm it. Prevents: The 🟠 High poll-budget finding, and the broader Axis-8 blocker class it belongs to (a change that altersfinal_statusfor identical agent output: FAILURE onmain-> ERROR on the PR, with criteria never evaluated). - Turn on branch coverage and add a per-package floor for
src/coder_eval/agents/**. Setbranch = trueunder[tool.coverage.run]and add a second, higher--cov-fail-under(or acoverage report --include='src/coder_eval/agents/*' --fail-under=90step) inmake verifyand.github/workflows/pr-checks.yml. The global 80% floor (Makefile:60, pr-checks.yml:153) is why a module sitting at 86% with its entire lifecycle surface uncovered passes CI unnoticed. Branch mode additionally surfaces partial-branch artifacts like485->exitas first-class, which is how the reviewer had to distinguish 'unreachable by construction' from 'untested'. Why not static: Coverage is a runtime measurement by definition; no AST rule can know which lines a test suite executes. Prevents: Both 🟡 Medium Test Health findings — the untested_drainretry loop and the zero-coverage teardown block — neither of which dents the global 80% number. - Pin bounded-retry caps with an explicit call-count assertion, and give the suite a helper for it. Add a small
CallCounter/assert_attempts(n)test util plus the convention (documented next to the CE035/CE036 entries) that any_*_RETRIES/_MAX_*constant ships with a test that monkeypatches it low and asserts the exact number of underlying calls. Concretely for this PR: (a) a mid-streamRuntimeErrorcase — the fake yields two usage-bearing steps then raises — asserting the retried drain does not doubletr.token_usage, duplicatetr.agent_outputparts, or inflate the assistant-message count (note:tr.commandsis not the right target;_handle_tool_callalready dedupes on_seen_tools/_closed_tools, whereas_output_partsandtotal_usageaccumulate unconditionally on everyprocess_step); and (b) an assertion that a permanently-raisingreceive_steps()surfacesAgentCrashErrorafter exactly_RECEIVE_STEPS_REENTRY_RETRIEScalls. The existing reentrancy test consumes 2 of 5 retries and still passes with the constant at 3. Why not static: A lint rule can see that a retry constant exists and even that its name appears somewhere intests/, but not whether any test actually drives the loop to exhaustion or replays a partially-consumed stream — that requires executing the loop and counting. Prevents: The 🟡 Medium_drainretry-coverage finding (no test for a mid-streamRuntimeError; the exhaustion branch is reached only incidentally by a pre-existing test that asserts nothing about it). - Run one CI job with randomized test order (
pytest-randomly, or a shuffled-seed job), reporting the seed on failure so a discovered order-dependence is reproducible. Why not static: CE042 catches the directClassName.attr = ...shape; order dependence introduced indirectly (module-level singletons, a fixture that mutates a shared registry, a cached SDK handle) is only observable by executing the suite in a different order. Prevents: The 🔵 Low class-level-mutable-test-state finding, and the general class of tests that pass only because a sibling test ran first. - Generate
.claude/harness-candidates.mdentries mechanically from the settled review-finding JSON (title,file:lineanchor, verdict, one-line 'why nothing guards it today'), instead of hand-summarizing them in prose. The review workflow already has the structured findings in hand at the point the file is written; hand-transcription is the only reason the note can diverge from the code. Why not static: No checker can compare an English summary of a fix against the fix itself — that is exactly the semantic-judgment boundary. The fix is to delete the transcription step rather than to police its output. Prevents: The 🔵 Low finding that both new harness-candidate entries describe intermediate versions of the fix this same PR ships (the note omits thetrajectory_idcomponent of the cid fallback, which has its own dedicated test, and omits the post-sleepbreakat :609-614, which also has its own test) — i.e. the deferred-guardrail log is already stale on arrival. - Add an advisory 'changed body, unchanged docstring' report to the PR review workflow. For each function in the diff whose body changed by more than N lines while its docstring is byte-identical, list it in the review output as a docstring-staleness candidate (advisory, not blocking). Why not static: Whether a docstring has become false is semantic — a heuristic can flag the candidate but cannot decide it, so this belongs in the review workflow's output rather than in
make verify. Prevents: The 🟡 Medium Axis-7 finding:communicate()'s docstring still states the pre-fix 'iteratereceive_steps()until the turn goes idle' contract, which the new poll loop directly contradicts. (The user-facing-doc half of that finding is covered statically by CE037.)
Top 5 Priority Actions
- Derive the post-idle poll deadline from the effective turn timeout (stop at
timeout - margin) or drop_MAX_BACKGROUND_POLLS * _BACKGROUND_POLL_INTERVAL_SECONDSbelow 300 s — src/coder_eval/agents/antigravity_agent.py:604 (constants :112, :140) — so an orphaned ACTIVE tool call exits through the graceful cap branch and is graded instead of raisingTurnTimeoutErrorintoorchestrator.py:566→FinalStatus.ERRORwith emptysuccess_criteria_results, and add a test that a never-resolving orphan attimeout=300.0finalizes and is scored. - Pin
_drain's bounded-retry semantics at src/coder_eval/agents/antigravity_agent.py:485-497 with a test that raisesRuntimeErrorafter steps were already fed tostate.process_step(:489) and assertstr.token_usage/tr.agent_output/ assistant-message count are not doubled by the replay — silent double-counting can triprun_limits.max_total_tokens/max_usdand flip a run toTOKEN_BUDGET_EXCEEDEDfor identical agent output — plus a test that a permanently-raisingreceive_steps()surfacesAgentCrashErrorafter exactly_RECEIVE_STEPS_REENTRY_RETRIEScalls (tests/test_antigravity_agent.py:1041 currently proves neither). - Correct the now-false turn contract: rewrite the
communicate()docstring at src/coder_eval/agents/antigravity_agent.py:520-522 (it still says "iterate until the turn goes idle") and add a "Known limitations" entry to docs/agents/ANTIGRAVITY.md:169-183 naming the poll bound, the minutes-long event-silent window (DEBUG-only logging at :607), and the need to sizerun_limits.turn_timeout/task_timeoutabove it — the two shipped example tasks pinturn_timeout: 300, below the 600 s budget. - Close the AntigravityAgent lifecycle-teardown coverage hole at src/coder_eval/agents/antigravity_agent.py:677-722 (
stop/kill/kill_sync/get_environment_info/_conversation_or_none/_teardown, uncovered at 86.13% suite-scoped) by mirroring tests/test_codex_agent.py:1517 and :1826, sincekill_syncis invoked from the watchdog's non-asyncio thread and nothing pins it as a plain non-raisingdef, and log rather than silently swallow the failure in_teardown. - Take the mechanical complexity and state cleanups: extract the poll block at src/coder_eval/agents/antigravity_agent.py:600-623 into a
_poll_for_backgrounded_worksibling of_drain(prototyped:communicateE33 → D24, below its pre-PR D27), replace the fifth parallel cid-keyed_tool_last_status: dict[str, Any](:785) with an_active_tools: set[str]sohas_orphaned_tool_call(:951) becomesbool(self._active_tools), and fix the two.claude/harness-candidates.mdnotes at :327 and :343 that describe intermediate fixes this same PR superseded.
Stats: 0 🔴 · 1 🟠 · 4 🟡 · 8 🔵 across 8 axes reviewed.
The poll loop's graceful-exit path (force-close a never-resolving orphan as unresolved, finalize and grade normally) was bounded by a fixed cycle count (120 x 5s = 600s) that was double experiments/default.yaml's own default turn_timeout (300s). Since the pre-existing ThreadedWatchdog enforces timeout by cancelling the whole turn, it always won that race under default settings, making the graceful path dead code: a tool call spuriously left ACTIVE with no real background job behind it (a real, observed case from the PR's own validation run) went from "finalizes immediately, graded on whatever the agent wrote" pre-fix to "burns the full 300s, then crashes as TurnTimeoutError with zero criteria graded" post-fix -- a strict regression for that input class. Fixes by deriving a poll_deadline from 0.8x the actual timeout passed to communicate(), falling back to the cycle-based cap only when timeout is None. 0.8 is a fraction of the watchdog's own deadline, not an identical value, so it doesn't reintroduce the race an earlier review round removed -- it's a deliberately earlier internal deadline engineered to reliably win. Caught independently by two PR reviewers (bai-uipath, uipreliga) on the same line of arithmetic. Added a regression test proving a never-resolving orphan under a realistic 300s timeout now finalizes gracefully instead of crashing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Fixed in 59552e8 — the poll loop's exit bound now derives from Added Also fixed both Thank you both — this was a real, well-diagnosed blocker and I'd rather have caught it here than in production. |
… 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>
…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 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>
) * feat(agents)!: make a base-config field mean the same thing on every 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> * chore(agents): drop the system_prompt changes from this PR 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> * chore(agents): drop config_support and the Antigravity tool mapping 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> * test(run-limits): add cross-harness max_turns / turn_timeout fixtures 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> * docs(run-limits): record the measured cross-harness parity results 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> * test(run-limits): tag the parity fixtures CI requires every task YAML to carry at least one tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: use current-generation models in examples 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> * docs(run-limits): re-measure the antigravity timeout case after the poll 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> * docs(claude): drop the config_support contract from the repo guide 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> * docs(run-limits): keep the contract on the page, the measurements in 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> * test(antigravity): clear the CodeQL findings on the fake SDK helper 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> * test(antigravity): pin the SDK half of the env seam contract 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> * fix(codex): fold sub-agent tokens on a turn-cap stop `_recover_subagent_tool_calls` was skipped whenever the notification pump broke on purpose, which the turn cap had just joined. Recovery is the only writer of the `parent_tool_use_id`-tagged messages `_fold_subagent_tokens` sums, and Codex bills sub-agents on separate threads the parent total never sees, so skipping it silently dropped the child threads' tokens and cost from every capped run that spawned one. Early stop is opt-in and rare; `max_turns` is a standard run limit, so this would have under-reported spend routinely. The cap now recovers as usual. Its recovered child calls land beyond the cap's count, the same way the force-closed orphan already does: the cap bounds what the model was allowed to do, not what the record may explain. A cooperative stop keeps its pre-existing skip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(parity): state the real final_status of a capped run "`final_status` is a completed status" was wrong: a capped run that satisfies its criteria finishes as SUCCESS, and one that does not finishes as MAX_TURNS_EXHAUSTED, whose reporting category is `failed` (orchestrator.py's status assignment, `_STATUS_CATEGORIES`). On the page that exists to be the per-harness contract, that is the distinction anyone diffing a nightly needs. Also names the second way a capped Codex run records more commands than the cap: recovered sub-agent calls, alongside the force-closed orphan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(run-limits): make the max_turns fixture assert the cap bound The fixture's only criterion was `file_exists: step-01.txt`, which passes whether or not the cap fired — including on a harness that ignores max_turns entirely, the exact bug this fixture exists to catch. Adds the other half: step-12.txt must NOT exist. The prompt now chains each file's contents onto the previous one, so a step cannot be written without reading its predecessor and no amount of batching inside a single agent-loop turn reaches step 12 under a cap of 4. A run that produced the last file therefore ran uncapped, on any harness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Summary
AntigravityAgent.communicate()drained the SDK's step stream once and finalized the turn the instant it went idle. When Gemini backgrounds a long-running tool call (e.g. a slow solve) and pauses intending to check back later, the SDK'sreceive_steps()exhausts anyway — it never blocks waiting for future work — so coder-eval was grading the task with the background job still in flight.The turn now polls (sleep + re-drain) while an orphaned tool call is still
ACTIVE, bounded by_MAX_BACKGROUND_POLLS(120 cycles) and the pre-existing turn watchdog. A normal turn (which always closes its tool calls before the stream exhausts) takes this branch zero times — no added latency.Hardened across three rounds of independent review before being validated end-to-end against the real Gemini API:
ACTIVEspecifically (not "not yet closed"), so a tool stuck onWAITING_FOR_USER/CANCELED/UNKNOWNis never polled forever.call.idis falsy) is stable across a step's ownACTIVE→DONEre-emissions and unique across trajectories (a sub-agent trajectory can reuse the same step indices as the main one)._drain()retries past the SDK's real two-layer generator re-entrancy window after a cooperative stop (confirmed live thatcontextlib.aclosing()on the outerConversation.receive_steps()generator does not synchronously close the innerLocalConnectiongenerator that owns the re-entrancy guard) instead of crashing the next turn withRuntimeError.Validation
Ran
energy-unit-commitmentend-to-end against the real Gemini API (gemini-3.1-pro-preview) with the fix baked into freshly rebuilt images. Result:status=SUCCESS score=1.000, duration 890.9s.The run organically reproduced the original bug scenario and two extra edge cases:
_MAX_BACKGROUND_POLLScap and finalized cleanly instead of hanging or crashing.No
Concurrent receive_steps()errors, noAgentCrashError, noTurnTimeoutError— all failure modes the review rounds specifically hardened against.Test plan
make verify— all pass except one confirmed pre-existing, unrelated failure (test_effective_model_prefers_config_then_default, caused by a local.envoverride, reproduces identically on unmodifiedmain)make lint— 332/332 custom architectural rules passtests/test_antigravity_agent.py— 52/52 new + existing tests pass, including regression tests that were verified to fail without each corresponding fixCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com