Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix: code review fixes (round 3)
Round-3 review found two High issues, one of them a regression introduced by
round 2's own fix, plus ripple that no earlier round had looked for.

- Antigravity: remove the poll-loop cycle cap. Round 2 made it apply
  alongside the deadline instead of only when there was no deadline, so a
  task configuring turn_timeout: 1200 (960s of polling) was silently cut at
  120 * 5s = 600s. On the timeout=None path the two bounds expired at the same
  instant anyway, so the cap bounded nothing the wall-clock deadline did not.
  One clock now covers both cost modes.

- Orchestrator: shield and track the forced-kill grading pass.
  check_all_async offloads each criterion to asyncio.to_thread, which is not
  cancellable, so the 60s budget left a run_command criterion's subprocess
  running inside a sandbox that run()'s finally was about to move or rmtree.
  The budget still bounds how long we WAIT for a verdict; _await_pending_grade
  bounds when teardown may start. Mirrors SubAgentRunner, which documents this
  same hazard.

- EvaluationResult.forced_kill records the kill durably, alongside
  final_status like max_turns_exhausted. Once grading can turn a TIMEOUT into
  SUCCESS the status stops being a usable proxy for "blew its budget":
  reports_experiment._cost_complete returned True for rows that lost in-flight
  spend, the error_log_tail allowlist dropped the only evidence of the kill,
  and telemetry could not count breaches. All three now key off the flag, and
  run.json carries it.

- Bound the two new unbounded awaits (the pre-grading agent quiesce and the
  poll-budget cancel); both run on a connection already declared unresponsive,
  outside any watchdog. The quiesce also catches BaseException so a queued
  task.cancel landing there cannot skip the grading pass it protects.

- DRY: _evaluation_loop now calls _gate_passed instead of keeping a second
  hand-maintained copy of the FIRED-ONLY rule, and the two timeout handlers
  collapse into _handle_forced_kill. That brings run() back under ruff's
  ceiling, so its # noqa: PLR0915 and its CE022 _TARGETS entry are both gone.

- Docs: REPORT_SCHEMA.md gains the TIMEOUT-is-a-fallback gotcha and the
  ERROR -> TIMEOUT migration for turn timeouts; CLAUDE.md records forced_kill;
  smoke_task_timeout.yaml's comments no longer claim criteria are never
  evaluated on a timeout, which this change made false.

Co-Authored-By: Claude Opus 5 <[email protected]>
  • Loading branch information
joeysbase and claude committed Aug 14, 2026
commit fc5c0e9e42ada53b9c90b33f0bb6db46cb3242de
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ action.yml # Published composite GitHub Action (coder-ev
- **Reconciliation message (stream self-reconciles to the turn total)**: The per-message stream consistently under-reports the authoritative turn total — a fixed prompt slice (~512 input tokens on Claude) is billed on no SDK-emitted message, and sub-agent input/cache only partially bubbles up. So `EventCollector.build_turn_record` appends one synthetic `ReconciliationMessage` (`role="reconciliation"`, in the `TranscriptMessage` union) per turn, carrying the per-bucket residual = `token_usage` − Σ(assistant message buckets). The invariant: **summing the four token buckets across `TurnRecord.messages` (assistant + reconciliation) equals `token_usage` exactly**, for both Claude and Codex (Codex's stream is already complete after `_recover_subagent_tool_calls`, so its residual is usually 0 and no entry is emitted). This is what lets the evalboard SUM the message stream as the source of truth instead of reading a separate aggregate ("agent tokens"): `selectTokenTotals` returns the stream sum whenever a reconciliation entry is present, and the timeline renders it as its own row. It is agent-agnostic (booked at the single `EventCollector` seam), carries no cost (cost stays on `token_usage`), and is excluded from generation/turn counts and the cost simulator. The LiteLLM open-weight actual-cost join (`litellm_cost.apply_actual_cost`) deliberately writes cost at the TURN level only (`token_usage.total_cost_usd` = the real OpenRouter bill) plus the per-call `TurnRecord.provider_call_costs` audit record; it does NOT touch the message token buckets, so `EventCollector` stays the single writer and this invariant holds on every backend. The Python `token_usage`/`total_token_usage` aggregate is unchanged and still authoritative for budget/judges/reports.
- **Harness run-limit parity**: a shared `BaseAgentConfig` field must mean the same thing on every backend, so a divergence is either fixed or documented — never silent. **`run_limits.max_turns` on Codex/Antigravity counts VISIBLE turns** (resolved tool calls, read live off the shared `EventCollector.visible_turn_count`, the same list `TurnRecord.commands` holds) because one `communicate()` is a single SDK turn on both, so a native counter would clamp at 1; claude-code keeps its native SDK cap, whose unit (an agent-loop turn) absorbs arbitrarily many parallel calls — the same number is NOT the same budget across harnesses. The cap is enforced on the same loop boundary as the cooperative early stop and finalizes cleanly as `max_turns_exhausted` (no crash, no retry); on Antigravity that boundary lives in `_drain()`, so the background-work poll loop honors it too. Known unfixed divergences: `permission_mode` on Codex and Antigravity (both run unconfined — the sandbox driver is the isolation boundary), `disallowed_tools` on Codex (forwarded, not SDK-enforced), `allowed_tools`/`disallowed_tools` on Antigravity (not read at all), and `turn_timeout` on Antigravity (the background-work poll loop is bounded by an earlier internal deadline at 80% of it, or a flat 600s when the task sets no timeout — at that bound a still-ACTIVE tool call is force-closed and graded normally, while a connection that never produced a clean turn end raises a real `TurnTimeoutError`). Full table + rationale: docs/agents/HARNESS_PARITY.md.
- **sandbox isolation**: Tasks that don't need MCP servers should set `setting_sources: []` in their `agent:` block to isolate the sandbox from the host project's CLAUDE.md and settings. Without this, the host project's CLAUDE.md (often 20 KB+) is injected into every API call, inflating cache-creation tokens and cost significantly.
- **Run-time caps (non-criterion enforcement)**: `TaskDefinition.run_limits` (`RunLimits` model) is the single namespace for all *task-level* run-time caps — `max_turns` / `task_timeout` / `turn_timeout` (structural) and `max_input_tokens` / `max_output_tokens` / `max_total_tokens` / `max_usd` (cumulative budget). Token/USD breaches abort with `FinalStatus.TOKEN_BUDGET_EXCEEDED` or `COST_BUDGET_EXCEEDED` (both `category == "failed"`). A **structural timeout is graded, not discarded**: `TaskTimeoutError` and `TurnTimeoutError` both run `Orchestrator._grade_after_forced_kill` against whatever the agent produced before the kill, so a task that timed out but already satisfied its criteria finalizes `SUCCESS` (plain, error_message cleared) instead of `TIMEOUT` — downstream consumers must not read `TIMEOUT` as "every timed-out run". Grading is bounded (60s), never raises, honors the same FIRED-ONLY early-stop gate as a normal run, and falls back to `TIMEOUT` on any failure. Structural caps are set from the CLI via `-D run_limits.max_turns=…` / `-D run_limits.task_timeout=…` / `-D run_limits.turn_timeout=…` (field-merged into `run_limits`); budget caps via `-D run_limits.max_usd=…` etc. or YAML. Layered config uses field-merge — a variant block overrides individual keys without replacing the task's block. The one *per-criterion* cap, `stop_early.decide_within`, deliberately lives on `LiveSuccessCriterion` instead (see below) — the watcher must attribute a decision-step timeout to a specific criterion, which `RunLimits` (task-scoped, criterion-agnostic) cannot express.
- **Run-time caps (non-criterion enforcement)**: `TaskDefinition.run_limits` (`RunLimits` model) is the single namespace for all *task-level* run-time caps — `max_turns` / `task_timeout` / `turn_timeout` (structural) and `max_input_tokens` / `max_output_tokens` / `max_total_tokens` / `max_usd` (cumulative budget). Token/USD breaches abort with `FinalStatus.TOKEN_BUDGET_EXCEEDED` or `COST_BUDGET_EXCEEDED` (both `category == "failed"`). A **structural timeout is graded, not discarded**: `TaskTimeoutError` and `TurnTimeoutError` both run `Orchestrator._grade_after_forced_kill` against whatever the agent produced before the kill, so a task that timed out but already satisfied its criteria finalizes `SUCCESS` (plain, error_message cleared) instead of `TIMEOUT` — downstream consumers must not read `TIMEOUT` as "every timed-out run". Grading is bounded (60s), never raises, honors the same FIRED-ONLY early-stop gate as a normal run, and falls back to `TIMEOUT` on any failure. `EvaluationResult.forced_kill` is the durable marker of the kill (like `max_turns_exhausted`) and is what consumers must read to ask "did this blow its budget?" — `reports_experiment._cost_complete`, the `error_log_tail` allowlist and the `ForcedKill` telemetry dim all key off it, not off the status. A turn-level timeout also moved from `FinalStatus.ERROR` (category `error`) to `TIMEOUT` (category `failed`) now that it has a dedicated handler, shifting rows between `tasks_error`/`tasks_failed` and between `<error>`/`<failure>` in JUnit. Structural caps are set from the CLI via `-D run_limits.max_turns=…` / `-D run_limits.task_timeout=…` / `-D run_limits.turn_timeout=…` (field-merged into `run_limits`); budget caps via `-D run_limits.max_usd=…` etc. or YAML. Layered config uses field-merge — a variant block overrides individual keys without replacing the task's block. The one *per-criterion* cap, `stop_early.decide_within`, deliberately lives on `LiveSuccessCriterion` instead (see below) — the watcher must attribute a decision-step timeout to a specific criterion, which `RunLimits` (task-scoped, criterion-agnostic) cannot express.
- **Early stop on criterion (opt-in, per-criterion arming)**: a `stop_early:` block (`StopEarlyPolicy`) on a criterion ends a single-shot run early once the run's **armed** criteria decide the outcome, so a raised `max_turns` isn't wasted on the smoke flavor. The block's PRESENCE is the arming and alone activates the watcher — there is **no run-level master switch**: `run_limits.stop_early: false` is the run-level KILL SWITCH that force-disarms every block (the one-line experiment-variant/`-D` override for an authoritative full run), and `run_limits.stop_early: true` (the removed master arm) is a hard `EarlyStopConfigError` at resolution. The block exists on `LiveSuccessCriterion` only (currently `skill_triggered`, `command_executed` — so arming an unobservable criterion is unrepresentable, a pydantic extra-forbid error). Arming carries one implicit trigger (a native live-fail may fail-stop the run); its keys refine it: `on_pass: stop` (pass-stop the moment the criterion live-passes; default `continue` just latches) and `decide_within: N` (still undecided after N tool-call steps latches an **effective fail**, fed through the same fail-stop rule, reported as `decision_budget_exceeded` — an ordinary weighted fail, NOT a gate-bypassing force-fail; cumulative across retry attempts of the same turn). A trigger whose polarity the instance can't decide (per the abstract, checker-independent `live_decidable_polarities()`, a pure function of the criterion's own fields, paired with the checker's `live_verdict` override by lint rule CE025, a registry-based whole-tree check) is **inert by design** — one dataset-fanned YAML line serves both positive rows (pass/timeout live) and distractor rows (fail live). Verdicts **latch**: once a criterion decides, its `live_verdict` is never polled again. Stop rule is weighted, not strict-boolean: `run_limits.stop_early_gate_threshold` (default `1.0`, reproducing strict-AND behavior exactly) is the minimum weighted score (`Σ weight·score / Σ weight` over the armed subset) required to pass; a fail-stop fires once the armed set's **ceiling** (best case for everything still undecided) can no longer reach the threshold — so a low-weight fail or timeout that can't doom the gate is absorbed and the run continues — and is **deferred while any pass-capable armed criterion is undecided** (a distractor misfire never truncates a positive row's recall signal); a pass-stop fires once the `on_pass: stop` subset's **floor** (worst case) already meets the threshold, and is symmetrically **deferred while any pass-capable armed criterion outside the `on_pass: stop` subset is undecided** (so an early pass never freezes a sibling `on_pass: continue` criterion's signal out of the trajectory). A fail-stop is therefore verdict-preserving; a pass-stop can miss a *later* distractor misfire, so authoritative P/R/F1 comes from a kill-switched (`stop_early: false`) run. Driven by `orchestration/early_stop.py::EarlyStopWatcher` (built when `early_stop_active(task)`: ≥1 armed criterion, kill switch not thrown) through the agent's cooperative `should_stop` seam (tool-call granularity, no SIGKILL); live verdicts only *trigger* the stop — the standard `check_all_async` on the frozen trajectory is authoritative. Gating is **FIRED-ONLY**: a run the watcher actually cut gates on the **armed subset** via the weighted `EvaluationResult.armed_criteria_passed`; a run that completes naturally — armed or not — gates strict-AND via `all_criteria_passed`, so adding a block never changes the verdict of a run it didn't cut. Note the gate keys on the watcher having FIRED (`result.early_stop is not None`), not on confirmed truncation — an agent that ignores `should_stop`, or a stop firing on the final message, still gates armed-only. Every resolution-time guardrail violation is a hard error at resolution (plan *and* run); the one load-time case — a `stop_early:` block on a non-live criterion — is a pydantic schema error at task load, which the run surface reports as a skipped task like any other malformed task. A runtime verdict bug **fails open** to a full run. Surfaces: `EarlyStopInfo` (incl. `gate_threshold` at stop time), report notes/badges, `stopped_early` run.json rows, `EarlyStopped`/`EarlyStopReason` telemetry dims. Worked rationale: docs/TASK_DEFINITION_GUIDE.md § `stop_early`. No blocks anywhere ⇒ behavior byte-for-byte unchanged.

## Success Criteria (15 types)
Expand Down
12 changes: 11 additions & 1 deletion docs/REPORT_SCHEMA.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ crashed, crash_reason}`) — the full transcript is in `task.json`.
### Missing cost is never fatal

Pricing degrades; the evaluation does not. A model absent from the rate card, a turn
the backend never priced, a hard-killed task that lost its in-flight spend: each one
the backend never priced, a hard-killed task that lost its in-flight spend (keyed on
`forced_kill`, since such a task may finalize `SUCCESS`): each one
lowers a total and sets `cost_complete: false`. None of them raises, none of them
books a zero, and none of them changes a run's exit code.

Expand Down Expand Up @@ -290,6 +291,15 @@ String enum values and their reporting category:
| `MAX_TURNS_EXHAUSTED` | failed | `M` |
| `TOKEN_BUDGET_EXCEEDED` | failed | `#` |
| `COST_BUDGET_EXCEEDED` | failed | `$` |

`TIMEOUT` is the **fallback** status for a structural timeout, not a synonym for
"this run timed out". A hard-killed run is graded against whatever the agent
produced, so one that already satisfied its criteria serializes as `SUCCESS`
with `error_message` cleared. To ask "did this task blow its budget?", read the
durable `forced_kill` flag on the row, not the status. A turn-level timeout also
now lands as `TIMEOUT` (category `failed`) rather than the `ERROR` (category
`error`) it produced before it had a dedicated handler — it moves between
`tasks_error` and `tasks_failed`, and between `<error>` and `<failure>` in JUnit.
| `ERROR` | error | `!` |
| `BUILD_FAILED` | error | `B` |

Expand Down
Loading
Loading