fix(claude-code-agent): end the turn on the terminal ResultMessage, not stream EOF

[tmatup]

What

When the SDK stream delivers the terminal ResultMessage of a one-shot query(), communicate() now ends the turn immediately — instead of waiting for stream EOF (i.e. CLI process exit). The subprocess is then reaped with a bounded grace (10 s, so it can flush its session file for resume) followed by SIGKILL.

Why

A successful agent run could be scored 0. If the Claude CLI process lingers after emitting its result — a stray child process holding stdio, a slow shutdown flush — the stream never hits EOF, so the turn watchdog eventually hard-kills it and raises TurnTimeoutError. The task is then recorded final_status=ERROR, score=0 even though the agent finished cleanly.

Observed 3× on live eval sweeps (twice yesterday, once today, all at exactly turn_timeout+4s wall):

05:38:42  final assistant message completes; ResultMessage captured
          (subtype=success, stop_reason=end_turn)
05:47:30  watchdog: Turn timeout (1200s) fired — hard-killing subprocess (pid=77)
          → TurnTimeoutError → task scored 0

~530 s of dead waiting on a process whose result was already in hand.

How

• communicate() message loop: after state.dispatch(message), break when the terminal ResultMessage has been captured (state.sdk_result_summary set). The ResultMessage is by protocol the last message of a one-shot query — nothing follows it but process exit.
• New _reap_transport_after_result(): bounded grace (default 10 s) for the CLI to exit on its own — killing instantly could lose the session-file tail the SDK's own close() protects (see its #625 comment) — then SIGKILL. No-op in the common already-exited case. Breaking alone is not enough: async-generator finalization is GC-scheduled, and the SDK's anyio scopes swallow cooperative cancellation (same reason the watchdog exists).
• test_multiple_assistant_turns reshaped to the real one-shot protocol (single terminal ResultMessage; previously it interleaved two mid-stream ResultMessages, a shape the CLI never produces in one-shot mode).

Tests

• New: test_communicate_ends_turn_on_result_message_when_cli_lingers (the regression: stream yields result then hangs — must return a clean TurnRecord, not TurnTimeoutError), test_reap_after_result_kills_lingering_process (grace-then-kill + no-op paths).
• Full suite: 3313 passed, 10 skipped.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LrwDtggu1dG9WKAsgvxfnq

[github-actions]

Claude finished @tmatup's task in 1m 22s —— View job

---

Code Review in Progress

Todo List:

• Read .github/code_review.md for review standards
• Read CLAUDE.md for project conventions
• Review git diff origin/main...HEAD
• Read full context of changed files
• Analyze for correctness and edge cases
• Check cross-file consistency
• Perform "what's missing" analysis
• Post structured review feedback

[tmatup]

Deployment note for the eval images: the images baked at /opt/coder_eval carry 0.7.1 (5c285d6), not current main — overlaying this branch's file into them fails at import (coder_eval.pricing exists only on newer main). The fix is cherry-picked onto 5c285d6 as branch fix/turn-end-on-result-message-071 (no PR — deployment vehicle only), which is what the flow-v2 measurement images now overlay. Both commits are content-identical to this PR.

[tmatup]

Second commit (0fbed2c): the break exposed a pre-existing misclassification — fixed at its root.

First live run with the turn-end fix truncated every turn that spawned a sub-agent: ~20 s in, zero authored output, empty sandbox. Cause: _is_sdk_result_message duck-types the result as session_id + usage — and the sub-agent lifecycle message TaskProgressMessage carries both. The misread pre-existed (silently corrupting session-id advance and token backfill whenever a sub-agent ran); ending the turn on the "result" merely made it fatal.

The classifier now matches isinstance(message, ResultMessage) positively and rejects the entire SystemMessage family (all task-lifecycle messages subclass it); the duck-typed fallback survives only for non-SDK test mocks. New regression test drives a real TaskProgressMessage through communicate() mid-stream and asserts the turn ends on the true terminal result with the transcript intact. Suite: 3314 passed.

Backport branch fix/turn-end-on-result-message-071 updated with the same commit (050e4ef).

[uipreliga]

Review: coder_eval — pr:15

Scope: pr:15 · branch fix/turn-end-on-result-message · 0fbed2c · 2026-07-11T10:13Z · workflow variant

Change class: complex — reworks Claude-agent turn termination (ends the turn on the terminal ResultMessage instead of stream EOF), which requires correctness reasoning about token accounting and partial-turn/exit-path handling

coder_eval is in excellent shape — near-perfect across type safety, tests, security, architecture, API surface, and harness quality — with the one real risk concentrated in the new Claude-CLI reap path: a successful turn whose terminal ResultMessage lands within grace_seconds of the turn deadline can be misclassified as a crashed TurnTimeoutError and retried/0-scored, so the bottom line is that this single score-changing resilience bug should block merge while everything else is polish.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Code Quality & Style	9.5 / 10	0	0	1	0	_reap_transport_after_result duplicates _kill_transport's SIGKILL/_process kill block instead of delegating (drift + SDK-private coupling)
2. Type Safety	10 / 10	0	0	0	0	—
3. Test Health	10 / 10	0	0	0	0	—
4. Security	10 / 10	0	0	0	0	—
5. Architecture & Design	10 / 10	0	0	0	0	—
6. Error Handling & Resilience	8.9 / 10	0	1	0	1	Bounded reap runs INSIDE the watchdog scope, so a successful turn whose ResultMessage arrives within grace_seconds of the deadline is reclassified as a crashed TurnTimeoutError — reintroducing the exact 0-score bug the PR fixes, just in a narrower window
7. API Surface & Maintainability	10 / 10	0	0	0	0	—
8. Evaluation Harness Quality	10 / 10	0	0	0	0	—

Overall Score: 9.8 / 10 · Weakest Axis: Error Handling & Resilience at 8.9 / 10
Totals: 🔴 0 · 🟠 1 · 🟡 1 · 🔵 1 across 8 axes.

Blockers

1. [Axis 6] Bounded reap runs INSIDE the watchdog scope, so a successful turn whose ResultMessage arrives within grace_seconds of the deadline is reclassified as a crashed TurnTimeoutError — reintroducing the exact 0-score bug the PR fixes, just in a narrower window (src/coder_eval/agents/claude_code_agent.py:1138) — _pump_stream_message calls await self._reap_transport_after_result(transport) (line 1138) BEFORE returning True, and this runs inside the with ThreadedWatchdog(...) block (lines 908-916) that wraps async for message in query(...). _reap_transport_after_result polls for up to grace_seconds=10.0 (while proc.returncode is None and time.monotonic() < reap_deadline: await asyncio.sleep(0.1), lines 1161-1163) whenever the CLI process lingers — which is the very case the reap exists for. If the turn deadline falls within that window, the watchdog fires: _fire() in watchdog.py calls _on_turn_timeout (sets state.timeout_hit=True, kills the transport) AND loop.call_soon_threadsafe(task.cancel). The cancel raises CancelledError inside the reap's asyncio.sleep, which is caught at line 920; self._timed_out(state.timeout_hit, deadline) (line 926) returns True and _finalize_and_raise_timeout raises TurnTimeoutError. The timeout classification at lines 926/934/947 never checks state.sdk_result_summary, so a turn that already captured a non-error terminal ResultMessage (logically successful, subtype=success) is discarded as a crashed=True timeout and retried — precisely the 'successful run into a 0-score timeout ERROR' failure the PR's own test test_communicate_ends_turn_on_result_message_when_cli_lingers claims to eliminate (that test passes only because its fake process has returncode=0, so the reap returns instantly and never overlaps the deadline). Fix: reap AFTER leaving the watchdog scope (set a break flag inside the loop, then reap once the with ThreadedWatchdog block has exited and cancelled its timer), OR guard the timeout branches so a captured non-error ResultMessage suppresses timeout reclassification (e.g. if self._timed_out(...) and state.sdk_result_summary is None).

Non-blocking, but please consider before merge

1. [Axis 1] _reap_transport_after_result duplicates _kill_transport's SIGKILL/_process kill block instead of delegating (drift + SDK-private coupling) (src/coder_eval/agents/claude_code_agent.py:1164) — The new _reap_transport_after_result re-implements the hard-kill logic that _kill_transport already centralizes. Lines 1164-1171:

if proc.returncode is None:
    logger.warning(
        "Claude CLI subprocess (pid=%s) still alive %.0fs after its terminal ResultMessage; hard-killing",
        getattr(proc, "pid", "?"),
        grace_seconds,
    )
    with suppress(OSError):
        proc.kill()

are a near-copy of _kill_transport (lines 1186-1193), which does the same getattr(transport, "_process", None) + returncode guard + logger.warning("Hard-killing ...", getattr(proc, "pid", "?")) + with suppress(OSError): proc.kill(). Both are @staticmethod, so after the grace-poll loop the reap should delegate: if proc.returncode is None: ClaudeCodeAgent._kill_transport(transport). This is now a third _process-touching site (alongside _kill_transport and kill_sync which already delegate) that bypasses the canonical kill helper; a future change to kill semantics (e.g. terminate-before-kill, different log wording, ESRCH handling) updated in _kill_transport would silently miss this inline copy. Consolidating removes the redundant getattr/warn/suppress(OSError) block and keeps a single kill path.

Nits

1. [Axis 6] Hard-kill warning in the new reap path logs via the module logger instead of the task-prefixed adapter, dropping task attribution in concurrent batch runs (src/coder_eval/agents/claude_code_agent.py:1165) — _reap_transport_after_result is a @staticmethod and emits its lingering-process warning via the bare module logger (line 1165: logger.warning("Claude CLI subprocess (pid=%s) still alive %.0fs after its terminal ResultMessage; hard-killing", ...)), whereas every other diagnostic in the class uses the task-prefixed self._log (PrefixedAdapter). In a parallel run_batch, this hard-kill warning arrives with no task/run prefix, so an operator cannot attribute which task's CLI lingered. Consider making the reaper an instance method (or passing self._log) so the warning carries the same task context as the surrounding lifecycle logs.

What's Missing

Daily/nightly:

• 🟠 The Claude Code agent is THE primary nightly/production agent, and this change alters turn-timeout classification on the live run path. It fixes the sweep-kill class (successful sub-agent-spawning tasks scoring 0-timeout) but — per the Axis 6 high finding — reintroduces the same 0-score TurnTimeoutError for any turn whose terminal ResultMessage lands within the 10s reap grace of the turn_timeout deadline, because the reap polls INSIDE the watchdog scope. The PR never states this scoring-correctness blast radius on the nightly (near-timeout turns with a lingering CLI get mis-scored), nor confirms it needs no schema/container-image rebuild (it doesn't — no wire-format change). An unstated nightly blast radius on a scoring-correctness change is itself the gap. (trigger: src/coder_eval/agents/claude_code_agent.py) (restates: Axis 6: bounded reap runs inside the watchdog scope, reclassifying a successful near-deadline turn as a crashed TurnTimeoutError)

Tests:

• 🟠 No test exercises the reap-grace ↔ watchdog-deadline overlap — the exact window where the fix reintroduces the timeout misclassification. test_communicate_ends_turn_on_result_message_when_cli_lingers sets fake_transport._process.returncode = 0, so _reap_transport_after_result returns instantly and never overlaps the deadline; the failing path (returncode None while the terminal result is already captured and the watchdog fires mid-reap) is untested. A regression test that pins a lingering process (returncode=None) with a turn_timeout shorter than grace_seconds and asserts a successful TurnRecord (not TurnTimeoutError) would have caught the Axis 6 high bug. (trigger: tests/test_agent_timeout.py) (restates: Axis 6: bounded reap runs inside the watchdog scope, reclassifying a successful near-deadline turn as a crashed TurnTimeoutError)
• 🟡 _reap_transport_after_result's designed happy path — the process transitioning returncode None -> 0 DURING the grace poll (the flush-then-exit case the grace exists for) and therefore NOT being killed — is untested. test_reap_after_result_kills_lingering_process only covers permanently-None (kill) and already-exited (returncode=0 up front, no-op) plus None-transport no-ops; the poll-loop's actual grace behavior (wait, observe clean exit, skip SIGKILL) is never exercised. (trigger: tests/test_agent_timeout.py)
• 🔵 The new positive branch if isinstance(message, ResultMessage): return True in _is_sdk_result_message is not exercised with a real ResultMessage instance — every test (_fake_result_message, mock streams) uses a duck-typed SimpleNamespace that falls through to the hasattr(session_id)+hasattr(usage) fallback. The SystemMessage-exclusion branch is covered (TaskProgressMessage is a SystemMessage subclass), but the isinstance(ResultMessage) short-circuit that the docstring makes load-bearing has no direct assertion. (trigger: src/coder_eval/agents/claude_code_agent.py)

Parallel paths:

• 🔵 codex_agent.py and antigravity_agent.py wrap their stream consumption in the identical ThreadedWatchdog pattern but were not given an end-on-terminal-result / bounded-reap. The fix's stated goal — a finished, successful turn must not be held open by a lingering subprocess until the watchdog converts it into a TurnTimeoutError — is a shared concern, yet the PR doesn't state whether the Codex/Antigravity SDK iterators (thread terminal-Turn reconstruction / conversation.receive_steps) can also linger after their terminal result. They appear to terminate their iterators on completion (so likely immune), but this should be confirmed and noted rather than left as an unaddressed open question, since the Claude-specific fix leaves the three agents diverged on this behavior. (trigger: src/coder_eval/agents/claude_code_agent.py)

Harness & Lint Improvements

Static checks (lint / type):

• [ce-lint] Add CE025 (tests/lint/rules/ce025_transport_private_access.py, wired into ALL_RULES in tests/lint/runner.py): forbid touching the SDK-private transport handle — any getattr(x, "_process", ...) / x._process access, and any .kill()/.terminate() call on that process — anywhere in agents/claude_code_agent.py EXCEPT inside the single canonical _kill_transport helper (allowlist by enclosing function name). The rule reduces the transport-teardown surface to one function and makes a bare inline proc.kill() a lint failure, forcing delegation. Prevents: Finding A (medium/low, Axis 1+7): _reap_transport_after_result (claude_code_agent.py:1164-1171) re-implements kill_transport's getattr(,"_process")+returncode-guard+suppress(OSError) proc.kill() block, making it a third _process-touching site that will silently drift when kill semantics change in the canonical helper. CE025 flags that inline block and any future copy at lint time in make verify.
• [ce-lint] Add CE026 (tests/lint/rules/ce026_agent_uses_prefixed_log.py, wired into ALL_RULES): in agents/claude_code_agent.py, forbid bare module-logger.<level>(...) calls inside class methods, requiring the established self._log PrefixedAdapter convention instead. Allowlist the adapter construction site (self._log = PrefixedAdapter(logger, ...)) and genuine module-scope calls. Because a @staticmethod has no self, the rule mechanically pushes such a method to become an instance method — which is exactly the finding's recommended fix. Prevents: Finding C (low, Axis 6): the @staticmethod _reap_transport_after_result emits its lingering-process hard-kill warning via bare logger.warning (claude_code_agent.py:1165) instead of self._log, so in a parallel run_batch the warning arrives with no task/run prefix and can't be attributed. CE026 catches this and every future bare-logger diagnostic added to the agent class.

Harness improvements (not statically reachable):

• Add a regression test that exercises the reap/watchdog overlap window: a fake Claude CLI transport whose _process.returncode stays None through the whole grace-poll (so _reap_transport_after_result spins for its full grace_seconds) while a non-error terminal ResultMessage has already been dispatched (state.sdk_result_summary populated, subtype=success) and the turn deadline elapses mid-reap so ThreadedWatchdog fires (sets timeout_hit=True, cancels the task). Assert the turn ends via _end_turn_ok() and does NOT raise TurnTimeoutError or produce a crashed=True TurnRecord. The existing test_communicate_ends_turn_on_result_message_when_cli_lingers uses returncode=0, so its reap returns instantly and never overlaps the deadline — this new test (or a returncode=None parametrization of it) closes that blind spot. Pair it with the structural fix (reap AFTER the with ThreadedWatchdog scope exits, or guard the timeout branches with and state.sdk_result_summary is None). Why not static: The defect is async scope-nesting plus a missing runtime-state consult (a watchdog thread firing mid-await inside the query loop, and the timeout classifier at lines 926/934/947/finally never reading state.sdk_result_summary). Reproducing it needs a live event/turn-classification pipeline and a real timer race — there is no grep/AST shape a lint or pyright rule can see. Prevents: Finding B (high, Axis 6): a logically successful turn whose ResultMessage arrives within grace_seconds of the deadline is reclassified as a crashed TurnTimeoutError and retried, reintroducing the exact 0-score timeout bug the PR claims to fix, just in a narrower window.

Top 5 Priority Actions

1. SCORE-CHANGING: move _reap_transport_after_result out of the with ThreadedWatchdog scope (break inside the loop, reap after the timer is cancelled) — or gate every timeout branch on state.sdk_result_summary is None — at claude_code_agent.py:1138, so a lingering CLI whose non-error ResultMessage arrives within grace_seconds of the deadline is not reclassified as a crashed TurnTimeoutError and retried/0-scored for identical agent output.
2. Add a regression test exercising a lingering process (returncode is None) whose terminal ResultMessage lands within grace_seconds of the turn deadline, since the existing test_communicate_ends_turn_on_result_message_when_cli_lingers sets returncode=0 (test line 302) and so never overlaps the window that triggers the bug.
3. Route the reap path's hard-kill warning through the task-prefixed self._log (make the reaper an instance method rather than a @staticmethod on the bare module logger) at claude_code_agent.py:1165, so operators can attribute which task's CLI lingered in concurrent run_batch runs.
4. Consolidate the duplicated SIGKILL block in _reap_transport_after_result at claude_code_agent.py:1164 to delegate the with suppress(OSError): proc.kill() idiom to the canonical _kill_transport (keeping the reap's grace-context log line), eliminating the third _process-touching site and its drift risk.
5. Add a custom lint rule (next CE0xx) forbidding direct transport._process access anywhere except _kill_transport, turning this one-time consolidation into permanent enforcement against future kill-path duplication and SDK-private coupling.

---

Stats: 0 🔴 · 1 🟠 · 1 🟡 · 1 🔵 across 8 axes reviewed.