feat(early-stop)!: per-criterion arming via stop_early blocks on live criteria - #78
Conversation
067dba1 to
73b8240
Compare
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:78 (25 files) all 8 axes
Scope: pr:78 (25 files) all 8 axes · branch akshaya/earlystop-per-criterion-arming · 067dba1 · 2026-08-04T17:26Z · workflow variant
Change class: complex — replaces the run-level early-stop master arm with per-criterion stop_early: blocks, rewriting the arming/gating control flow (weighted ceiling/floor stop rules, deferral, fired-only gating) plus breaking schema renames; correctness requires reasoning about partial-trajectory verdict semantics
Architecture, security, API surface and error handling are excellent (three axes at 10/10, zero criticals), but the real risk is concentrated in one confirmed harness-correctness defect where a decide_within fail-stop can gate a task SUCCESS that pre-PR reported FAILURE for byte-identical agent output, compounded by YAML-order-dependent early-stop reason attribution and a structurally broken CLAUDE.md that silently lost four unrelated sections — so the bottom line is that this is a strong 9.4/10 change that should not ship until the gating invariant and the doc regression are fixed, with the remaining style/test-hygiene items safe as follow-ups.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 7.4 / 10 | 0 | 1 | 3 | 1 | PR collaterally deletes four unrelated CLAUDE.md sections (~67 lines: Success Criteria table, Evaluation Flow, Development Commands/lint-loop, ## Configuration heading) |
| 2. Type Safety | 9.3 / 10 | 0 | 0 | 1 | 2 | # type: ignore[call-arg] masks a nonexistent kwarg name, making test_guardrail3_unobservable_criterion_unrepresentable (tests/test_early_stop.py:814-824) vacuous |
| 3. Test Health | 9.5 / 10 | 0 | 0 | 1 | 0 | validate_early_stop's absent/unregistered-agent guard is uncovered, and its only reachable input (no agent block) yields a misleading diagnosis |
| 4. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 5. Architecture & Design | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 6. Error Handling & Resilience | 9.9 / 10 | 0 | 0 | 0 | 1 | Cooperative-stop capability probed via stringly-typed getattr instead of the declared ClassVar on the Agent ABC |
| 7. API Surface & Maintainability | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 8. Evaluation Harness Quality | 8.9 / 10 | 0 | 1 | 0 | 1 | A decide_within-driven fail-stop is not verdict-preserving: it can truncate a run whose armed criterion would score 1.0 (gating SUCCESS where main FAILED), and the docs still claim otherwise in 3 places |
Overall Score: 9.4 / 10 · Weakest Axis: Code Quality & Style at 7.4 / 10
Totals: 🔴 0 · 🟠 2 · 🟡 5 · 🔵 5 across 8 axes.
Blockers
- [Axis 1] PR collaterally deletes four unrelated CLAUDE.md sections (~67 lines: Success Criteria table, Evaluation Flow, Development Commands/lint-loop,
## Configurationheading) (CLAUDE.md:146) —git diff origin/main...pr-78 -- CLAUDE.mdshows the two rewritten early-stop bullets replacing a 75-line block that also contained four sections unrelated to early stop:## Success Criteria (14 types)(the 14-row criteria table + theweight/pass_threshold/suite_thresholdsparagraph),## Evaluation Flow,## Development Commands(the "MANDATORY: Run after every implementation phase" make block, the "could a custom lint rule have prevented this?" standing rule, the CE030 doc-parity note, the Docs-index-SSOT note, and the anchor-slugger convention), and the## Configurationheading. Only the early-stop and run-time-caps bullets needed to change. The result is structurally broken: PRCLAUDE.md:146is now- **ruff**: line-length=120, target py313, select E/F/I/N/W/UP/B/SIM/RUFsitting directly after the early-stop bullet inside## Key Architectural Patterns(heading at line 128), followed by- **pyright**…/- **pytest**…/- **Coverage threshold**: 80%(147-149) and then## Extension Points(151) — the ruff/pyright/pytest/coverage settings now read as architectural patterns and the file no longer documentsmake verifyat all. Comparegit show origin/main:CLAUDE.md | grep -n '^#\+ '(17 headings) with the PR's (12).uv run pytest tests/test_custom_lint.py -m lintis 158 passed, so no lint rule guards this. Fix: restore the four sections verbatim and keep only the two intended bullet rewrites. - [Axis 8] A
decide_within-driven fail-stop is not verdict-preserving: it can truncate a run whose armed criterion would score 1.0 (gating SUCCESS where main FAILED), and the docs still claim otherwise in 3 places (src/coder_eval/orchestrator.py:1586) — The new single-gate branch justifies itself with a claim the watcher cannot honor:# decision-budget stop is just a fail-stop whose deciding criterion / # timed out (the watcher only fires once the weighted ceiling / # proves the armed gate cannot pass), and the frozen-trajectory / # scores agree by construction(orchestrator.py:1582-1587). For a native live-fail that holds (monotone checkers). For adecide_withintimeout it does not:early_stop.py:536-538manufactures the fail out of undecided —budget_expired = verdict == "undecided" and budget is not None and tool_call_index >= budget/if budget_expired: verdict = "fail"— and_ceilingthen pins that criterion at 0, so the ceiling is NOT an upper bound on the authoritative score. The two trajectories genuinely differ: the watcher drops unresolved tool ends (early_stop.py:437-438:if event.status == ToolEndStatus.UNRESOLVED: return, docstring 423-424 "keeps a crashed attempt's orphan tools out of the retry-persistent partial trajectory") and deliberately does not latch an in-flight round's verdict (early_stop.py:539:if in_flight is None and verdict != "undecided":), whilecheck_all_async(turn_records=self.result.iterations)scores over the drained crashed-attempt partials, which DO contain those force-closed commands (EventCollector.on_event:elif isinstance(event, ToolEndEvent): self._commands[event.tool.tool_id] = event.tool, no status filter). Failure scenario on the shipped fixturetasks/early_stop_decision_budget_exceeded.yaml: attempt 1 dispatchespython app.py, then the turn crashes/times out before the tool result arrives; the partial turn is drained intoresult.iterations, but the watcher's collector never recorded it and itstool_call_indexkeeps counting (documented as cumulative across retries). On attempt 2 the criterion readsundecided,tool_call_index >= 3latches the synthetic fail,_ceiling == 0.0 < 1.0fires reasondecision_budget_exceeded— yetarmed_criteria_passedscores the same criterion 1.0 from the drained command, the armed gate passes, and the advisoryfile_exists: app.pycriterion is excluded from the gate, so the task reports SUCCESS even with no app.py. Pre-PR the identical output was forced to FAILURE. Fix: either treat a_budget_expiredstop as gate-relevant only after re-checking the authoritative score (i.e. do not claim guaranteed failure), or make the watcher's trajectory the same one the checker will score (record unresolved tool ends in the watcher's collector, as the agent's collector already does). At minimum delete the false "agree by construction" claim and add a test where the timed-out criterion's authoritative score is 1.0 and an unarmed criterion is 0.0 —tests/test_early_stop.py:2330 test_decision_budget_exceeded_gates_through_armed_gatescores every criterion 1.0, so it never exercises the advisory-demotion consequence.
Non-blocking, but please consider before merge
- [Axis 1] EarlyStopWatcher.init grows to 7-8 index-aligned parallel lists (radon A(3) -> C(14)), with derivable/projected members (
src/coder_eval/orchestration/early_stop.py:296) — Seven lists are keyed positionally toself._armed:_decidable(296),_pass_trigger(308),_fail_trigger(314),_budget(318),_latched(336),_budget_expired(337),_prev_verdicts(342).git show origin/main:src/coder_eval/orchestration/early_stop.pyhas only two (_armed_polarities,_prev_verdicts), anduv run radon cc -smeasuresEarlyStopWatcher.__init__at C (14) on the PR vs A (3) on main — the coupling is what this PR adds. They are re-zippedstrict=Trueat lines 309, 319, 321, 473, 583, 642 and indexed byiat 519, 536, 539-541, 556-564, 604, 651. Collapse them into one small_ArmedStatedataclass per armed criterion (criterion,checker,decidable,pass_trigger,budget,latched,budget_expired,prev_verdict) and iteratefor st in self._armed:— the positional-list-vs-list coupling this file now has is the exact shape the repo's own review rubric calls out as a candidate CEnnn lint rule. Also drop_fail_triggeroutright: line 314 isself._fail_trigger: list[bool] = ["fail" in pol for pol in self._decidable]— a derived list carrying zero information beyond_decidable, read at exactly one site (line 604self._fail_trigger[i]), which can read"fail" in self._decidable[i]directly. - [Axis 1]
_collect_verdictsdiscards the exactbudget_expiredfact it just computed, forcing_budget_drove(early_stop.py:545-565) to re-derive it heuristically with an always-True final conjunct (src/coder_eval/orchestration/early_stop.py:545) — This PR reclassifiedDECISION_BUDGET_EXCEEDEDinto an ordinary weighted fail —results.py:418-428now says "every reason gates identically througharmed_criteria_passed's weighted gate.DECISION_BUDGET_EXCEEDEDis a reporting label only" — yet the machinery to derive it grew: a persistent parallel latch (_budget_expired, line 337, written at 541) plus a 21-line second derivation_budget_drove(545-565) that re-computes the same fact for the not-yet-latched in-flight round, consulted twice per fail-stop (604, 611). Its final condition,and "fail" not in self._decidable[index](line 564), is unreachable-as-a-discriminator today:_budget[i]is non-None only when"pass" in pol(line 318-320), and both in-tree implementations oflive_decidable_polarities()return a set holding at most one polarity (criteria.py:800-806returnsfrozenset({"pass"})orfrozenset({"fail"});criteria.py:697-703adds"pass"only whenmax_count is Noneand"fail"only when it is not) — so"pass" in polalready implies"fail" not in poland the term is always True. It is speculative generality, and in the hypothetical it anticipates (a criterion decidable on both polarities) it is also wrong: on an in-flight round_budget_expired[i]is still False, so a budget-driven fail would be mislabeledCRITERION_FAILED. Since the label is the only remaining consumer, record it once where the conversion happens (line 537-538) — e.g. latch"fail_budget"as the verdict state, or have_collect_verdictsreturn(verdict, budget_driven)pairs — and delete_budget_drove. - [Axis 1] Early-stop tests still driven through removed API vocabulary (
max_steps_to_decide,stop_on_fail), withstop_on_fail=Truea provable no-op producing duplicate tests (tests/test_early_stop.py:170) —_block(*, stop_on_pass=False, stop_on_fail=False, max_steps_to_decide=None)(lines 170-182) translates the removed pre-PR vocabulary into the new schema, and_skill_crit/_cmd_crit(185-229) forward it, so ~200 call sites in the suite that documentsstop_early:never show aStopEarlyPolicyliteral. Two concrete costs. (a)stop_on_failcannot affect the built object — the body only mapson_pass="stop" if stop_on_pass else "continue"anddecide_within=max_steps_to_decide; it participates solely in theif not stop_on_pass and not stop_on_fail and max_steps_to_decide is None: return Nonearmed/unarmed guard.grep -c "stop_on_fail=True"= 48, of whichgrep -n "stop_on_fail=True" | grep -E "stop_on_pass=True|max_steps_to_decide="= 32 also set another flag (so the kwarg is inert), and 29 are the literal pairstop_on_pass=True, stop_on_fail=True, byte-identical tostop_on_pass=Truealone — which reads as if a separate fail arm still existed, the exact belief this PR removed. (b)max_steps_to_decideis the deleted field name, still in 40+ places including test names liketest_max_steps_to_decide_inert_on_fail_only_criterion(line 706) that assert ondecide_within. Rename the kwargs to the shipped keys (on_pass,decide_within) or passStopEarlyPolicy(...)/stop_early={}directly, and dropstop_on_failin favour of an explicitarmed=True. - [Axis 2]
# type: ignore[call-arg]masks a nonexistent kwarg name, makingtest_guardrail3_unobservable_criterion_unrepresentable(tests/test_early_stop.py:814-824) vacuous (tests/test_early_stop.py:823) — The test's stated contract (lines 814-817: 'an armed unobservable criterion cannot even be constructed (extra=forbid)') is not what it asserts. It passesstop_on_pass=True, # type: ignore[call-arg](line 823) — butstop_on_passis only a test-helper kwarg name (_block(*, stop_on_pass=...), line 171);grep -rn "stop_on_pass\|stop_on_fail" srcreturns zero hits, so it has never been a model field. Verified at PR HEAD that the assertion cannot distinguish the intended case from any typo:FileExistsCriterion(..., stop_on_pass=True)->stop_on_pass Extra inputs are not permitted [extra_forbidden]andFileExistsCriterion(..., totally_bogus_kwarg=True)->totally_bogus_kwarg Extra inputs are not permitted [extra_forbidden]— identical. The# type: ignoreis what suppresses the one signal that would have surfaced the wrong kwarg, and because pyright'sexcludeliststests(pyproject.toml:216) nothing else does. Change line 823 tostop_early=StopEarlyPolicy(),(the formtest_block_unrepresentable_on_unobservable_criterionat line 390 already uses correctly) and assert the error message mentionsstop_early. Note also that all six# type: ignore[...]comments added by this diff use mypy-style error codes while the project checks with pyright and excludestests/, so they are decorative. - [Axis 3] validate_early_stop's absent/unregistered-agent guard is uncovered, and its only reachable input (no agent block) yields a misleading diagnosis (
src/coder_eval/orchestration/early_stop.py:231) — Line 231 is the only statement in the 673-line rewrite that no test reaches (confirmed:uv run pytest tests/test_early_stop.py --cov=coder_eval.orchestration.early_stop --cov-branch→167 2 64 5 96.97% 231, 306, 436->443, 660->exit, 665->667). The uncovered guard is:
228: if registration is None:
229: # Not the same failure as an agent that opted out of cooperative stop:
230: # an unregistered type usually means a plugin is not installed/loaded.
231: raise EarlyStopConfigError(
232: f"criterion-level stop_early arming requires a registered agent type; {agent_type!r} is "
TestValidateEarlyStop (tests/test_early_stop.py:649) pins the other three guards by name and message — test_master_arm_true_rejected (:655, "has been removed"), test_guardrail5_simulation_rejected (:785, "simulation"), test_guardrail1_non_supporting_agent_rejected (:791, "cooperative stopping"), test_gate_threshold_zero_rejected (:688, "must be > 0.0") — and even pins the raise ORDER (test_raise_order_simulation_before_agent, :826). Guard (3)'s first half is the gap. Add two cases: (a) an armed task with agent_type="no-such-agent" asserting EarlyStopConfigError matching "not registered"; (b) task.agent = None — agent: ResolvedAgentConfig | None (src/coder_eval/models/tasks.py:343) makes this reachable, and it currently produces "...requires a registered agent type; None is not registered (is the providing plugin installed and loaded?)", which a test would either pin or expose as misleading for an agentless task.
Nits
- [Axis 1] Gratuitous function-local import of
early_stop_gate_notein reports_html._render_header with no import cycle to avoid (src/coder_eval/reports_html.py:348) — Line 348 isfrom .reports import early_stop_gate_noteinside_render_header, with no comment naming a cycle. There is none:grep -n '^from \.' src/coder_eval/reports.pyshowsreports.pyimports only.models(11) and.path_utils(22), andgrep -rn reports_html src/coder_eval/shows nothing inreports.py;python -c "import coder_eval.reports_html, coder_eval.reports"succeeds. Move it to the module-level import block besidefrom coder_eval.models import FinalStatus, …(line 20) — the repo's convention is that a function-local import means a real cycle (comparereports_stats.py:3-4, which documents its cycle-avoidance reason), so an undocumented one misleads the next reader into thinkingreports↔reports_htmlis circular. - [Axis 2]
EarlyStopInfo.gate_thresholdkeeps a hardcodeddefault=1.0although this PR introducesDEFAULT_STOP_EARLY_GATE_THRESHOLDand results.py already imports it (src/coder_eval/models/results.py:479) — This PR addsDEFAULT_STOP_EARLY_GATE_THRESHOLD: Final[float] = 1.0(models/limits.py:9) whose docstring says it exists 'so the field default below, the watcher'sfor_taskfallback, and the orchestrator's finalize fallback can never drift apart', and results.py imports it at line 24 and uses it at line 680 (def armed_criteria_passed(self, criteria, gate_threshold: float = DEFAULT_STOP_EARLY_GATE_THRESHOLD)). But the persisted-record default four lines away still readsgate_threshold: float = Field(/default=1.0,(results.py:478-479) — the one remaining literal, on the field that makestask.jsonself-describing. Change line 479 todefault=DEFAULT_STOP_EARLY_GATE_THRESHOLD,so a future change to the constant cannot leave persisted records describing a threshold the code no longer uses. - [Axis 2] Two new test helpers in the 1125-line early-stop test module carry no annotations, in a file pyright does not check (
tests/test_early_stop.py:921) —def _resolve_surface(task_file: Path, tmp_path: Path, *, overrides: dict[str, Any] | None = None):(line 921) has no return annotation even though it returnsresolve_all_tasks(...)'s two-tuple and 8+ tests destructure it asresolved, skipped = _resolve_surface(...); anddef counting(self_, criterion, records):(line 1984) — the replacement installed ontotype(checker).live_verdictat line 1988 — has no parameter or return annotations at all, so it silently need not matchBaseCriterion.live_verdict(self, criterion: C, turn_records: list[TurnRecord]) -> LiveVerdict. These are the only 2 of ~300defs in the module missing annotations (grep -c "^\s*\(async \)\?def .*):\s*$" tests/test_early_stop.py-> 2), and becausepyproject.toml:216excludestestsfrom pyright nothing will ever flag them. Annotate both (-> tuple[list[ResolvedTask], list[SkippedTask]], and(self_: SkillTriggeredChecker, criterion: SkillTriggeredCriterion, records: list[TurnRecord]) -> LiveVerdict) so the monkeypatch stays honest against the real signature. - [Axis 6] Cooperative-stop capability probed via stringly-typed getattr instead of the declared ClassVar on the Agent ABC (
src/coder_eval/orchestration/early_stop.py:236) — early_stop.py:236 reads the capability by name with a silent default:
if not bool(getattr(registration.agent_class, "supports_cooperative_stop", False)):registration.agent_class is statically typed type[Agent[Any]] (agents/registry.py:32) and Agent.supports_cooperative_stop: ClassVar[bool] = False is declared on the ABC (agent.py:80), with True on all three built-ins (claude_code_agent.py:657, codex_agent.py:640, antigravity_agent.py:190). So the getattr(..., default) + bool() buys nothing and defeats pyright: renaming the ClassVar would leave this reading False and every armed task would hard-fail with the actively misleading message "agent type 'claude-code' does not [support cooperative stopping]". Replace with the direct attribute access if not registration.agent_class.supports_cooperative_stop:. The same error string also hardcodes "(claude-code, codex, antigravity)", which will drift from the registry as soon as a plugin agent supports the seam — derive it from AgentRegistry instead.
5. [Axis 8] EarlyStopReason attribution between criterion_failed and decision_budget_exceeded is now YAML criterion-order dependent (was deterministic precedence on main) (src/coder_eval/orchestration/early_stop.py:600) — The fail-stop now picks its deciding criterion with a single order-scan — candidate_index = next((i for i, v in enumerate(verdicts) if v == "fail" and (self._fail_trigger[i] or self._budget_drove(i, verdicts, tool_call_index))), None) (early_stop.py:600-607) — and derives the reason from that same index (early_stop.py:609-613). On main the budget check was a separate loop that ran after the native fail-stop, so a native live-fail always won regardless of ordering. Failure scenario: a row with a latched distractor misfire (native fail) and a criterion whose decide_within expired on the same round reports criterion_failed if the distractor is listed first in success_criteria and decision_budget_exceeded if it is listed second — for identical agent behaviour. That value is persisted (EarlyStopInfo.reason), rendered (reports.py:456), and emitted as the EarlyStopReason telemetry dimension (orchestrator.py:264), so reordering criteria in a YAML shifts dashboard counts. No verdict impact (both reasons gate identically). Fix: restore explicit precedence — prefer a native live-fail candidate over a budget-driven one — and add a test with both orderings asserting the same reason.
What's Missing
Daily/nightly:
- 🟠 🟠 Pipeline blast radius of the hard error is unstated:
run_limits.stop_early: truewas the ONLY documented arming lever until this PR and is-D-reachable, yet it now raisesEarlyStopConfigErrorat resolution (abortsrun, flipsplan's exit code) with no deprecation window.action.yml'sextra-argsinput explicitly advertises-D overrides(action.yml:30) andrelease.ymlmaintains a movingv<major>tag, so any external caller (the coder-eval-uipath / eval-runner nightly, downstream experiment YAMLs, a workflow pinned to the moving tag) fails hard at resolution rather than degrading. Verified no in-repo config sets it (grep -rn stop_early .github/ experiments/ tasks/is clean oftrue) — the PR should say exactly that, plus name the external key to grep for. (trigger: src/coder_eval/models/limits.py) - 🟡 🟡 The fired-only gating flip changes
final_statusfor identical agent output and its trend impact is unstated: an armed run that completes naturally now gates strict-AND over the full criteria set (orchestrator.py:1600-1603), whereorigin/maingated the weighted armed subset for anystop_early-armed task whether or not the watcher fired. Pass rates for any armed suite therefore step discontinuously in the evalboard / App Insights series, and nothing in run.json versions or annotates the semantic change (gate_thresholdisnullon exactly those runs, indistinguishable from an unarmed run). The PR body should state the expected direction of the shift for the nightly, or note that no shipped nightly suite is armed today. (trigger: src/coder_eval/orchestrator.py)
Tests:
- 🟠 🟠 No test covers a pass-stop truncating a fail-only-decidable armed sibling, and the rewritten deferral rule does not protect it.
_evaluate_impl'soutside_pass_capable_undecided(early_stop.py:641-645) defers only on siblings whoselive_decidable_polarities()contains"pass", but acommand_executedwithmin_count>=1ANDmax_countset is decidable{"fail"}only (criteria.py:677-702) while its authoritative score still requires the command to run. Reproduced against the PR tree: A=min_count:1,max_count:None,on_pass:stop, B=min_count:1,max_count:3,stop_early:{}, both weight 1.0 — onepython app.pyToolEnd firescriterion_passedat tool call 1, andarmed_criteria_passed([A,B],1.0)on the frozen trajectory returns False, i.e. an unearned FAILURE on a run a full trajectory would have passed. This is exactly the outcome the deferral comment (early_stop.py:636-640: "an unearned fail on the armed gate that a full run would not have produced") claims to prevent. Pre-existing in shape, but the predicate was rewritten here; either defer on "not yet satisfied" rather than "pass-decidable", or pin the gap with a test. (trigger: src/coder_eval/orchestration/early_stop.py) (restates: Axis 8:decide_within-driven fail-stop is not verdict-preserving) - 🔵 🔵 The new shared
early_stop_gate_noteis exercised through the markdown renderer for both branches (test_early_stop.py:2624/2631) but through the HTML renderer only forcriterion_passed(test_html_header_shows_early_stop_badge, :2649). Thedecision_budget_exceededtooltip — and the newly added_esc(title)escaping at reports_html.py:352 — has no assertion; one extra line in that test would pin both. (trigger: src/coder_eval/reports_html.py)
Parallel paths:
- 🟡 🟡 The two lint registries that guard every sibling task-YAML config model were not extended for the new user-authored
StopEarlyPolicy:tests/lint/doc_schema_parity.py::DOCUMENTED_MODELS(CE030 — listsTaskDefinition/RunLimits/Dataset/SimulationConfig) andtests/lint/dead_config_fields.py::CONSUMED_MODELS(CE031 —SimulationConfig/RunLimits/Dataset). Both would pass today (on_passanddecide_withinare documented as inline code in docs/TASK_DEFINITION_GUIDE.md and read by name at early_stop.py:308/318), so this is a free 2-line addition that stops the nextstop_early:key from shipping undocumented or dead. (trigger: src/coder_eval/models/criteria.py) - 🟡 🟡
validate_early_stopdeletes all five instance-level dead-arm guards thatorigin/mainraised (unobservable type, undecidable requested polarity, emptyauto, budget on a fail-only instance, "armed but no criterion sets stop_when") in favour of "inert by design", but no user-visible replacement surface was added. The only signal that an armed criterion can never fire islogger.debug("all armed stop triggers are inert for this row")at early_stop.py:321, andplan— which still callsvalidate_early_stop(plan_command.py:138) — prints nothing about arming. Sostop_early: {decide_within: 5}on a fail-only distractor, or a block oncommand_executed(min_count: 0, max_count: None)(decides neither polarity), is now silent dead config on a non-fanned task, the class CE031 exists to prevent. Fix: warn (not debug) when the whole armed set is inert, and/or surface armed criteria + live triggers inplanoutput. (trigger: src/coder_eval/orchestration/early_stop.py) - 🔵 🔵
experiments/default.yaml— the baseline that deliberately enumerates everyrun_limitscap, including commented-out optional ones (max_usd,count_cached_input), so "users know the baseline" — was not extended with commentedstop_early(kill switch) /stop_early_gate_thresholdentries, even though the kill switch is now the primary run-level lever the A/B recipe depends on. (trigger: experiments/early-stop-ab.yaml)
Downstream consumers:
- 🟡 🟡 The suite-level classification path was not updated for the new arming model:
reports.py::write_suite_rollupsand thesuite_thresholdsgate (which drives the CLI exit code) aggregate per-row criterion results with zero early-stop awareness — nostopped_earlyrow count, no flag onSuiteRollup. A pass-stopped row biases recall/precision, and nothing rejects or warns when the same criterion carries bothstop_early:andsuite_thresholds:. Under the old design a run-level switch had to be typed to reach this; now a task file alone arms every consumer, so a dataset suite that reuses an armed task silently gates CI on truncated metrics. The caveat currently lives only in prose (early_stop.py:82, docs/AB_EXPERIMENTS.md:302). (trigger: src/coder_eval/models/criteria.py) - 🟡 🟡 Arming became reachable from exactly one layer and no override surface was extended to compensate:
stop_early:lives onsuccess_criteria, whichExperimentVariantcannot override (its fields are variant_id/description/agent/simulation/repeats/template_sources/prompt_mutations/initial_prompt[_file]/run_limits/driver), and-Donly reaches theagent/run_limits/sandboxroots. So after this PR you can DISARM from a variant or the CLI (-D run_limits.stop_early=false) but there is no way to ARM without editing the task YAML — asymmetric with the old-D run_limits.stop_early=truelever, and it means every suite sharing an armed task file inherits early stop plus fired-only gating by default. (trigger: src/coder_eval/models/limits.py)
Display & mapping dicts:
- 🟡 🟡 The only exhaustiveness guard over
EarlyStopReasonwas deleted and not replaced:assert_never(reason)(and thefrom typing import assert_neverimport) is gone from orchestrator.py, andgrep -rn assert_never src/coder_eval/now returns nothing. The single remaining reason-dependent surface isreports.py:169 early_stop_gate_note(reason: str), which special-cases"decision_budget_exceeded"and falls through to a generic sentence for everything else — including run.json's literal"unknown"(reports.py:456). A 4thEarlyStopReasonmember would therefore render the wrong gate prose in both the markdown note and the HTML badge tooltip with no pyright or lint signal. Cheap fix: type the parameter asEarlyStopReasonand dispatch viamatch+assert_never. (trigger: src/coder_eval/orchestrator.py) - 🟡 🟡 No evalboard surface consumes the early-stop keys this PR keeps writing into run.json.
eval_result_to_task_dictemitsstopped_early/early_stop_reason/turns_remaining_at_stop/gate_threshold(reports_experiment.py:208-216), butevalboard/lib/runs.ts'sRawTaskRowreadsexpected_turns/has_final_reply/visible_turnsand has no early-stop field at all — a truncated run is rendered, averaged and trended identically to a full one in the dashboard the nightly is read from. Pre-existing gap, but this PR removes the run-level opt-in that made truncation a deliberate, remembered act, so the misread risk is now much higher; astopped earlypill next to the existing badges would close it. (trigger: src/coder_eval/reports_experiment.py)
Harness & Lint Improvements
Static checks (lint / type):
- [ce-lint] CE034 — CLAUDE.md structural parity (doc-surface test class in
tests/test_custom_lint.py, alongside CE027–CE031; helper intests/lint/claude_md_parity.py). Three mechanical invariants over the repo-rootCLAUDE.md: (a) a checked-in manifest of required##sections (Project Overview,Directory Structure,Key Architectural Patterns,Success Criteria,Evaluation Flow,Development Commands,Configuration,Extension Points,Task Definition,Dependencies,Design Principles,Notes for AI Assistants) must all be present — deleting one then requires deleting its manifest entry, a visible reviewable act; (b) the## Success Criteria (N types)heading's N and the table's| \type` |row count must equal the member count of theSuccessCriterionunion inmodels/criteria.py(main has 14 rows / 14 members — verified bygit show origin/main:CLAUDE.md | grep -c '^| `'→ 14); (c) everymakenamed in the## Development Commandsblock must exist as a Makefile target, and every Makefile target carrying a##help string (help/install/format/check/lint/typecheck/test/test-cov/verify/docs-indexes/...) must appear in CLAUDE.md. Numbering note: implemented ids stop at CE031, but.claude/harness-candidates.mdreserves CE026/CE032/CE033, so claim CE034+ (the id-uniqueness assert intests/lint/runner.pyis the SSOT). _Prevents:_ A1/A5/A7 high —CLAUDE.md:146: 67 lines collaterally deleted (Success Criteria table → caught by (b), Development Commands incl.make verifyand the lint-rule standing rule → caught by (c), Evaluation Flow + the## Configurationheading → caught by (a)).uv run pytest tests/test_custom_lint.py -m lint` is 158 passed on the PR, i.e. nothing guards CLAUDE.md today. - [pyright] Type-check
tests/withenableTypeIgnoreComments = false. Remove"tests"from[tool.pyright] exclude(pyproject.toml:216), add it toinclude, and setenableTypeIgnoreComments = falseso mypy-style# type: ignore[...]comments (which pyright silently honors while ignoring the bracketed code) stop suppressing real errors. Measured on the PR worktree with exactly the repo's current baseline settings plus that flag:tests/test_early_stop.pyalone yields 26 errors, including verbatim823: reportCallIssue No parameter named "stop_on_pass",390: No parameter named "stop_early",409: No parameter named "on_fail", and1124: Declaration "should_stop" is obscured by a declaration of the same name— i.e. the check fires precisely on the masked defects. Phase it: land the flag plus atests/-scopedexecutionEnvironmentsentry, burn down the existing errors (or gate new/changed test modules first). Do NOT try ruffANNfor the annotation half — measured 4417 ANN001/ANN201/ANN202 hits acrosstests/, not adoptable. Prevents: A2 medium —tests/test_early_stop.py:823:# type: ignore[call-arg]masks a nonexistent kwarg, makingtest_guardrail3_unobservable_criterion_unrepresentablevacuous (it cannot distinguish the intended contract from any typo). Also the 5 decorative mypy-style ignores this diff adds, and the A2-low unannotated-helper/monkeypatch-signature issue at:1984. - [ce-lint] CE035 — no index-aligned parallel instance lists (and no sibling-derived instance lists) in one
__init__. NewBaseRuleintests/lint/rules/ce035_no_parallel_instance_lists.py, wired intoALL_RULESintests/lint/runner.py, scoped tosrc/coder_eval/. Two violation classes: (1) a single__init__assigns ≥4list-typedself._xattributes and the class shows positional coupling (azip(self._a, self._b, ..., strict=True)or ≥2 distinctself._x[i]subscripts sharing an index name) — measured repo-wide: fires only onEarlyStopWatcher.__init__(7 lists;zip(..., strict=True)at 309/319/473/583/642), while_CodexTurnState.__init__and_AntigravityTurnState.__init__(4 lists each, no positional coupling) stay clean; (2) an instance list assigned as a comprehension over a sibling attribute assigned in the same__init__— measured 3 hits repo-wide, all inside that same cluster (_pass_trigger308,_fail_trigger314,_budget318 over_decidable). Message points at the fix: one_ArmedState-style record per element.# noqa: CE035escape. Note ruffC901is NOT a substitute — I measured it and mccabe never flags this__init__(radon's C(14) comes from comprehensions, which mccabe does not count); at max-complexity 12 it produces 27 unrelated hits. Prevents: A1/A5 medium —orchestration/early_stop.py:296: 7 index-aligned parallel lists (radon A(3)→C(14)); and the derived-_fail_triggerhalf of the A1-medium finding at:545(_fail_triggercarries zero information beyond_decidable). - [ce-lint] CE036 — never probe a declared first-party capability flag by string. New
BaseRule(tests/lint/rules/ce036_no_stringly_capability_probe.py): AST-parsesrc/coder_eval/agent.pyfor names annotatedClassVar[...]onAgent(todaysupports_cooperative_stop,supports_cost_log_tags) and flag anygetattr(x, "<that name>", default)/hasattr(x, "<that name>")anywhere insrc/coder_eval/— the attribute is statically declared on the ABC, so direct access is both type-checked and rename-safe. Measured: exactly 1 hit repo-wide (orchestration/early_stop.py:236), so it is adoptable as-is with no noqa debt. A blanketgetattr-literal ban is not viable (145 legitimate duck-typing hits over SDK objects), which is why the name set is derived from the ABC. Prevents: A2/A6 low —orchestration/early_stop.py:236:bool(getattr(registration.agent_class, "supports_cooperative_stop", False))defeats pyright; a rename would silently readFalseand hard-fail every armed task with the misleading "'claude-code' does not support cooperative stopping". - [ce-lint] CE037 — a literal
Field(default=...)must not duplicate an already-importedDEFAULT_*constant. NewBaseRule(tests/lint/rules/ce037_field_default_uses_constant.py): collect module-levelDEFAULT_*constants and their literal values acrosssrc/coder_eval/(4 today:DEFAULT_LOG_TAIL_MAX_BYTES,DEFAULT_JUDGE_MODEL,DEFAULT_SIMULATION_STOP_TOKEN,DEFAULT_STOP_EARLY_GATE_THRESHOLD); in any module that imports one of them, flag aField(default=<literal>)whose value equals that constant's value (same type). Measured repo-wide: exactly 1 hit,models/results.py:478— zero false positives (themodel: str | None = Field(default=None)fields do not collide withDEFAULT_JUDGE_MODEL's string value, which is why value-equality rather than name-tail matching is the right predicate). Prevents: A2 low —models/results.py:479:EarlyStopInfo.gate_thresholdkeepsdefault=1.0although this PR addsDEFAULT_STOP_EARLY_GATE_THRESHOLD(whose own docstring says the field default must not drift from it) andresults.py:24already imports it. - [ce-lint] CE038 — agent-kind name literals may not appear in
src/coder_eval/outsideagents/andmodels/enums.py. NewBaseRule(or fold as a second class into CE036): flag string literals equal to a registered agent type ("claude-code","codex","antigravity","none") — including inside f-strings — outsidesrc/coder_eval/agents/**,models/enums.py, and doctest/docstring bodies; the fix is to derive the list fromAgentRegistry. This mechanizes the agnostic-core litmus CLAUDE.md already states ("grep -ri <agent-name> src/coder_eval/outside the agent's own package should be ~zero"). Measured: 1 real hit (orchestration/early_stop.py:239) plus the enum definitions and one docstring example, both exemptible by scope. Prevents: A6 low (second half) —orchestration/early_stop.py:239hardcodes"(claude-code, codex, antigravity)"in the guard's error message; it silently drifts the moment a plugin agent supports the cooperative-stop seam. - [ce-lint] CE039 — a second reducer over the event stream may not apply a
ToolEndStatusfilter thatEventCollectordoes not. NewBaseRule(tests/lint/rules/ce039_no_divergent_tool_end_filter.py): inside anisinstance(event, ToolEndEvent)handler located outsidesrc/coder_eval/streaming/andsrc/coder_eval/agents/, flag any comparison againstToolEndStatus.*(orevent.statusearly-return/continue) unless a# noqa: CE039names why the divergence is safe. Rationale:streaming/collector.pyrecords unresolved/force-closed tool ends as commands, so any other reducer that drops them builds a strictly smaller trajectory than the onecheck_all_async(turn_records=...)later scores — the exact mechanism behind the Axis-8 high. Measured: 1 hit (orchestration/early_stop.py:437-438). Prevents: A8 high —orchestrator.py:1586/early_stop.py:437: the watcher's trajectory ⊂ the checker's trajectory, so adecide_withinfail-stop is NOT verdict-preserving (armed criterion scores 1.0 on the frozen trajectory → run gated SUCCESS with advisory criteria demoted, where main forced FAILURE). - [ce-lint] CE040 — tests must not rebind a method on a class object directly. New
BaseRulescoped totests/: flag assignment to anast.Attributewhose value is atype(...)call (or a bare class reference) — e.g.type(checker).live_verdict = counting— and requiremonkeypatch.setattr(...)(auto-restoring,raising=Truevalidates the target exists) ormock.patch.object(..., autospec=True)(validates the replacement's signature against the real one). Measured: exactly 2 hits repo-wide, both introduced by this PR (tests/test_early_stop.py:1988,:1996), against 32 test modules already usingmonkeypatch.setattr— so the convention is established and the rule lands with zero debt. Prevents: A2 low —tests/test_early_stop.py:1984: the unannotateddef counting(self_, criterion, records)installed ontotype(checker).live_verdictneed not matchBaseCriterion.live_verdict(self, criterion, turn_records) -> LiveVerdict;autospec=Truemakes the signature mismatch fail loudly, and pyright does not covertests/. - [ce-lint] CE041 — removed-schema-name residue gate. Whole-tree text check wired as a
tests/test_custom_lint.pyclass (CE027-family, not aBaseRule): compute eachcoder_eval.modelsmodel's field-name set at the merge base (git show origin/main:src/coder_eval/models/*.py, AST-parsed — no imports needed) and at HEAD; any field name that disappeared must not survive as an identifier, kwarg, YAML key, or docs mention insrc/,tests/,docs/,tasks/unless listed in a smallRETIRED_NAMESallowlist with a reason. Offline fallback (no merge base available in the sandbox): a checked-intests/lint/retired_names.pylist the renaming PR must extend — the same shape as the deferred "retired-token grep gate" already in.claude/harness-candidates.md, generalized from subsystem tokens to model fields. Prevents: A1/A3/A7 medium —tests/test_early_stop.py:170:max_steps_to_decide(this PR's deletedCriterionEarlyStopfield, renamed todecide_within) survives at 47 occurrences including the test name at:706, and the_block(...)helper keeps a whole removed vocabulary alive across 162 call sites. - [ruff] Enable
PLC0415(import-outside-top-level) with the repo's existing debt-marker convention. Add"PLC0415"to[tool.ruff.lint] selectand bootstrap the 107 existing hits withruff check --add-noqa, exactly asPLR0915/PLR0912are already framed in pyproject.toml:187-192 ("gates NEW growth past these bounds … existing offenders are tracked"). The value is that every future function-local import must either move to the top or carry a visible# noqa: PLC0415, which is where the cycle reason belongs (comparereports_stats.py:3-4, which documents its cycle). If 107 noqas is judged too much churn, the narrower alternative is a CE rule scoped to first-party function-local imports (from .x/from coder_eval.xinside a function body) requiring an adjacent comment or noqa. Prevents: A1/A5 low —reports_html.py:348:from .reports import early_stop_gate_noteinside_render_headerwith no cycle to avoid (reports.pyimports only.modelsand.path_utils), which misleads readers into believingreports↔reports_htmlis circular.
Harness improvements (not statically reachable):
- Trajectory-parity test between the two event reducers. Feed one synthetic event sequence — including two
ToolStartEvents emitted before eitherToolEndEvent(Claude's parallel-tool-call shape,agents/claude_code_agent.py:344-376) and one force-closedstatus=UNRESOLVEDend — into bothstreaming.EventCollectorandEarlyStopWatcher, then assert the command set the watcher scores over equals the command set that lands in theTurnRecordthe checker will score (or that every deliberate difference is enumerated in the test). Parametrize over everyToolEndStatusmember so a new status can't quietly diverge one reducer from the other. Why not static: CE039 can forbid the status filter, but the invariant being protected is set-equality of two reductions over an interleaved stream (parallel starts, retry-drained crashed partials) — that needs an actual event sequence and both reducers running, not an AST shape. Prevents: A8 high — thedecide_withinfail-stop that is not verdict-preserving (orchestrator.py:1586,early_stop.py:437-438,:536-539). - Gate-consequence matrix test for every
EarlyStopReason. Add a test where the armed criterion's authoritative score is 1.0 while an unarmed/advisory criterion scores 0.0, per reason (criterion_failed,decision_budget_exceeded, ceiling-bound), asserting the resultingFinalStatus. Todaytests/test_early_stop.py:2330scores every criterion 1.0 and:2377scores the armed one 0.0, so the branch wherearmed_criteria_passedpasses whileall_criteria_passedfails — i.e. the advisory-demotion consequence of the single-gate rewrite — is never exercised. Why not static: The gap is a missing combination of runtime scores flowing throughOrchestratorfinalize; no lint rule can tell that an existing test's fixture happens to make both gates agree. Prevents: A8 high (the SUCCESS-where-main-FAILED outcome) and the stale "agree by construction" comment atorchestrator.py:1582-1587. - Criterion-order permutation invariance test. Run the same event stream twice with
success_criteriain both orders (a latched fail-armed distractor plus adecide_within-expired criterion resolving on the same round) and assertEarlyStopInfo.reason— which is persisted intask.json, rendered atreports.py:456, and emitted as theEarlyStopReasontelemetry dimension atorchestrator.py:264— is identical. Worth generalizing into a small "YAML-order invariance" helper for any persisted label derived from a first-match scan over criteria. Why not static: The order dependence lives in a runtimenext((i for i, v in enumerate(verdicts) ...))scan whose outcome depends on which verdicts co-occur on a round; a static rule can't distinguish a legitimate first-match scan from a precedence bug. Prevents: A8 low —early_stop.py:600-613: reason attribution flips betweencriterion_failedanddecision_budget_exceededpurely on YAML criterion order, shifting dashboard counts for identical agent behaviour. - Diff-coverage gate in
make verifyandpr-checks.yml. Adddiff-cover coverage.xml --compare-branch=origin/main --fail-under=100(or an equivalent changed-lines gate) after the existing coverage step. The repo only enforces a global--cov-fail-under=80(Makefile:57, pr-checks.yml:144), which cannot see 2 unexecuted statements inside a 673-line insertion: a full-suite run of this PR reportsearly_stop.py … 97.40% missing 231, 306, 436->443, 665->667. A changed-lines gate turns "new code is untested" into a red CI check instead of a reviewer catch, and forces the author to either test line 231 or mark it# pragma: no coverwith a reason (which itself surfaces that it is unreachable from any validatedTaskDefinition). Why not static: Needs runtime coverage data plus a git comparison against the merge base; unreachability of a defensiveraiseunder a registry-validated model is a semantic property, not an AST shape. Prevents: A3/A6 medium — the uncoveredregistration is Noneguard atearly_stop.py:231(and the untested defensiveraiseat:306), whose only reachable input (agent=None) yields the misleading "is the providing plugin installed and loaded?" diagnosis. - Fail-open invariant telemetry at the early-stop/finalize seam. When a fail-stop fired, have
Orchestratorcompare the watcher's live verdicts against the authoritativecheck_all_asyncresult at finalize and log a WARNING (with reason + criterion) whenever they disagree — e.g. a stopped-early run whose armed gate then passes. Promote the same comparison to a hard assert under an opt-in env flag (CODER_EVAL_STRICT_INVARIANTS=1) exercised by a nightly job, so the divergence shows up as one dashboard-visible counter instead of a silently mis-gated task. Why not static: The claim being audited ("the ceiling is an upper bound on the authoritative score") is only checkable by scoring the frozen trajectory at runtime; the harness already prefers fail-open behavior for verdict bugs, so this is an observability guard, not a gate. Prevents: A8 high — makes the broken verdict-preserving invariant self-reporting the first time it fires in the nightly, rather than surfacing as an unexplained SUCCESS. - Require every documented invariant claim to name its enforcing test. The
stop_earlydocs assert verdict-preservation in 3 places (docs/TASK_DEFINITION_GUIDE.md:432and the CLAUDE.md early-stop bullet) with nothing tying the prose to a test. Add a small checked-in claim→test map (docs/invariants.yaml: claim text anchor →tests/…::test_name) plus a lint-family test asserting each named test id actually exists (and, via--collect-only, is collected). Renaming or deleting the guard test then breaks the build instead of leaving a doc claim the code no longer honors. Why not static: Whether prose is true of the code is semantic judgment; the mechanizable part is only the existence/collection of the cited test, which requires a pytest collection pass. Prevents: A8 high (grouped doc half) — the three surviving "verdict-preserving" claims that thedecide_withinpath contradicts. - Extend the CE-id uniqueness assert to the reserved-id ledger.
tests/lint/runner.pyalready asserts no two implemented rules share an id, but.claude/harness-candidates.mdreserves CE026/CE032/CE033 for deferred candidates, so "next number after the last file intests/lint/rules/" (CE026) collides. Have the runner (or the CE-family test) parse the candidates file and fail if an implemented rule claims a reserved id, and print the true next-free id. Why not static: It is a static check, but it guards the review/authoring workflow rather than product code — listing it here so it isn't confused with a defect-preventing rule. Prevents: Pre-empts an id collision while landing CE034–CE041 above (the candidates file explicitly warns that two in-flight branches claiming one number is the likely failure).
Top 5 Priority Actions
- Fix the broken verdict-preserving invariant behind the single-gate branch at src/coder_eval/orchestrator.py:1586 — the watcher drops UNRESOLVED tool ends (src/coder_eval/orchestration/early_stop.py:437-438) and never latches an in-flight round (:539) while
check_all_asyncscores the fuller frozen trajectory, so adecide_withintimeout (:536-538) can pin a criterion at 0, firedecision_budget_exceeded, and still letarmed_criteria_passedscore it 1.0 and report SUCCESS with advisory criteria demoted (reproduced with parallel tool calls on tasks/early_stop_decision_budget_exceeded.yaml); make the watcher record unresolved tool ends like src/coder_eval/streaming/collector.py:80-81 does (or re-check the authoritative score before gating), delete the false "agree by construction" comment at orchestrator.py:1582-1587 plus the matching claim at docs/TASK_DEFINITION_GUIDE.md:432, and add the missing test where the timed-out criterion scores 1.0 and an unarmed criterion scores 0.0. - Restore deterministic precedence in the fail-stop candidate scan at src/coder_eval/orchestration/early_stop.py:600-613 so a native live-fail always wins over a budget-driven one, because the deciding index is currently picked by
success_criteriaYAML order and that value is persisted inEarlyStopInfo.reason, rendered at src/coder_eval/reports.py:456 and emitted as theEarlyStopReasontelemetry dimension (src/coder_eval/orchestrator.py:264), so a harmless criterion reorder shifts dashboard counts for identical agent behaviour. - Restore the ~67 lines of CLAUDE.md deleted collaterally around CLAUDE.md:146 — the
## Success Criteria (14 types)table,## Evaluation Flow, all of## Development Commands(includingmake verify, the "could a lint rule have prevented this?" standing rule, the CE030 doc-parity note, the docs-index SSOT note and the anchor-slugger convention) and the## Configurationheading, whose loss now leaves ruff/pyright/pytest settings reading as architectural patterns, keeping only the two intended early-stop/run-limits bullet rewrites and ideally adding a CEnnn heading-integrity rule sincemake lintdoes not catch this. - De-vacuum the guardrail tests: tests/test_early_stop.py:823 passes a nonexistent
stop_on_pass=True # type: ignore[call-arg]whoseextra_forbiddenerror is indistinguishable from any typo, so the test cannot fail — switch it tostop_early=StopEarlyPolicy()withpytest.raises(ValidationError, match="stop_early")as line 389 already does, and add the one genuinely missing guard test (armed criteria withtask.agent = None, the only reachable path to src/coder_eval/orchestration/early_stop.py:231, since an unregisteredagent.typeis rejected earlier by the registry validator). - Cut the accidental complexity this PR added in src/coder_eval/orchestration/early_stop.py by collapsing the seven index-aligned parallel lists keyed to
self._armed(:296, :308, :314, :318, :336, :337, :342 —__init__went from radon A(3) to C(14)) into one mutable_ArmedStatedataclass, dropping the derived_fail_trigger, and having_collect_verdictsreturn(verdict, budget_driven)pairs so the provably always-True conjunct at :564 disappears — while keeping_budget_drove's behaviour, which is load-bearing eligibility logic at :604 rather than a mere label; alongside, rename the stale test-helper kwargsmax_steps_to_decide/stop_on_failat tests/test_early_stop.py:170 to the shippeddecide_within/armedvocabulary, move the undocumented function-local import at src/coder_eval/reports_html.py:348 to module scope, useDEFAULT_STOP_EARLY_GATE_THRESHOLDat src/coder_eval/models/results.py:479, and replace the stringly-typedgetattr(..., "supports_cooperative_stop", False)probe at early_stop.py:236 with direct attribute access.
Stats: 0 🔴 · 2 🟠 · 5 🟡 · 5 🔵 across 8 axes reviewed.
uipreliga
left a comment
There was a problem hiding this comment.
Fix what you agree with and 🚢
There was a problem hiding this comment.
Pull request overview
This PR refactors early-stop so that arming is defined per criterion via a stop_early: block on live-observable criteria, while run_limits.stop_early becomes a kill switch (false to force-disarm; true is now a resolution error). This aligns early-stop behavior, gating semantics (“fired-only”), and reporting across the orchestrator, models, tasks, tests, and docs.
Changes:
- Replace
stop_when/max_steps_to_decidewithstop_early: StopEarlyPolicy(on_pass,decide_within) onLiveSuccessCriterion, plusis_stop_armedas the uniform arming predicate. - Update early-stop validation/activation (
early_stop_active,validate_early_stop) and orchestrator finalize gating to apply the armed weighted gate only when the watcher fired. - Unify early-stop report wording across Markdown + HTML via
early_stop_gate_note()and update shipped tasks/experiment/docs accordingly.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_threshold_enforcement.py | Updates validation coverage for zero-weight + early-stop arming via StopEarlyPolicy. |
| tests/test_early_stop.py | Reworks extensive test matrix for per-criterion arming, kill switch, fired-only gating, and timeout semantics. |
| tasks/early_stop_weighted_low_weight_absorbed.yaml | Migrates example task to stop_early: blocks and documents fired-only gating + weighted absorption. |
| tasks/early_stop_weighted_high_weight_kills_run.yaml | Same migration for the “high-weight distractor dooms the gate” example. |
| tasks/early_stop_decision_budget_exceeded.yaml | Migrates “decision budget exceeded” example to stop_early.decide_within. |
| src/coder_eval/reports.py | Adds early_stop_gate_note() and uses it in runtime notes to avoid renderer drift. |
| src/coder_eval/reports_html.py | Uses shared gate note for tooltip text and ensures it is HTML-escaped. |
| src/coder_eval/reports_experiment.py | Updates early-stop surface commentary to reflect criterion-level arming. |
| src/coder_eval/orchestrator.py | Activates watcher via early_stop_active() and applies fired-only gating in finalize. |
| src/coder_eval/orchestration/experiment.py | Clarifies early-stop validation timing relative to kill-switch overrides. |
| src/coder_eval/orchestration/early_stop.py | Implements criterion-level arming, inert-by-design triggers, latching, symmetric deferrals, and activation predicate. |
| src/coder_eval/models/results.py | Updates early-stop reason semantics/docs; defaults armed-gate threshold via constant. |
| src/coder_eval/models/limits.py | Adds DEFAULT_STOP_EARLY_GATE_THRESHOLD; changes RunLimits.stop_early to `bool |
| src/coder_eval/models/criteria.py | Introduces StopEarlyPolicy, removes stop_when, adds is_stop_armed, and wires weight/arming validation. |
| src/coder_eval/models/init.py | Re-exports StopEarlyPolicy and DEFAULT_STOP_EARLY_GATE_THRESHOLD. |
| src/coder_eval/cli/plan_command.py | Updates plan-surface comments to reflect per-criterion arming. |
| experiments/early-stop-ab.yaml | Updates “smoke vs e2e” recipe: smoke uses task arming; e2e uses kill switch. |
| docs/tutorials/04-writing-a-task.md | Updates tutorial link text to new arming/kill-switch semantics. |
| docs/TASK_DEFINITION_GUIDE.md | Updates the early-stop guide and run_limits table to the new model and semantics. |
| docs/REPORT_SCHEMA.md | Updates schema semantics for decision_budget_exceeded gating. |
| docs/EXTENDING.md | Updates agent extension docs for cooperative stop requirement under criterion-level arming. |
| docs/DIALOG_MODE.md | Updates dialog-mode note and kill-switch guidance. |
| docs/agents/CLAUDE_CODE.md | Updates agent doc wording to criterion-level arming. |
| docs/AB_EXPERIMENTS.md | Updates the smoke vs e2e recipe to use kill switch rather than a master arm. |
| CLAUDE.md | Updates the repository guide to reflect the new early-stop architecture and semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… criteria Replace the run-level early-stop master arm with per-criterion arming: a stop_early: block (StopEarlyPolicy) on a live-observable criterion (skill_triggered, command_executed) alone activates the EarlyStopWatcher. run_limits.stop_early is demoted to a run-level kill switch — false force-disarms every block (the one-line e2e/authoritative-P/R/F1 override); true (the removed master arm) is a hard EarlyStopConfigError at resolution. Semantics: - StopEarlyPolicy keys: on_pass (stop|continue, default continue) and decide_within (N tool-call steps; expiry latches an effective fail fed through the weighted ceiling fail-stop as decision_budget_exceeded). - Weighted stop rules: fail-stop fires when the armed set's ceiling can no longer reach run_limits.stop_early_gate_threshold (default 1.0 = strict AND); pass-stop fires when the on_pass:stop subset's floor already meets it. BOTH stops defer while a pass-capable armed criterion outside the deciding subset is undecided, so recall is never truncated. - FIRED-ONLY gating: a run the watcher actually cut gates on the weighted armed subset (EvaluationResult.armed_criteria_passed); a naturally completing run gates strict-AND over the full set — adding a block never changes the verdict of a run it didn't cut. - Verdicts latch on resolved rounds; polarity-inert arming supports dataset fan-out (one YAML line for positive and distractor rows); a raising live_verdict fails open to a full run. Hardening: allow_inf_nan=False on criterion weight; single-sourced DEFAULT_STOP_EARLY_GATE_THRESHOLD; shared early_stop_gate_note() for both report renderers; on_event fully behind the fail-open disarm wrapper; _prev_verdicts latched on resolved rounds only. Tests: 218 early-stop tests incl. real Orchestrator._setup activation, in-flight decide_within expiry, mixed-arming pass-stop deferral, load_experiment over experiments/early-stop-ab.yaml, fail-closed pins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…inism, doc restore Review blockers: - Restore the four CLAUDE.md sections (Success Criteria table, Evaluation Flow, Development Commands, Configuration heading) collaterally deleted by the early-stop bullet rewrite; only the criteria-support paragraph's stop_when mention is updated to the stop_early vocabulary. - Trajectory parity: the watcher now RECORDS UNRESOLVED tool ends into its collector (still never counted or evaluated on), so it reduces the same trajectory check_all_async scores — a decide_within timeout can no longer latch an effective fail on a criterion the frozen trajectory scores as a pass. The orchestrator's "agree by construction" comment now cites the parity mechanism instead of asserting it. Also from the review: - Deterministic fail-stop reason: a native live-fail candidate always wins over a budget-driven one, so EarlyStopInfo.reason no longer flips between criterion_failed and decision_budget_exceeded on YAML criterion order. - validate_early_stop: distinct agentless-task diagnosis (was the misleading "None is not registered" plugin hint); direct ClassVar access instead of a stringly getattr probe; supporting-agent list derived from AgentRegistry instead of a hardcoded literal. - early_stop_gate_note dispatches via exhaustiveness-checked match over EarlyStopReason (restores the assert_never guard the rewrite dropped). - EarlyStopInfo.gate_threshold field default uses DEFAULT_STOP_EARLY_GATE_THRESHOLD instead of a duplicate literal. - reports_html imports early_stop_gate_note at module scope (no cycle). - All-inert armed set logs at WARNING (visible dead-config signal). - Orchestrator distinguishes fail-open disarm from natural completion in the full-gate log line (Copilot inline comment). - De-vacuumed test_guardrail3_unobservable_criterion_unrepresentable (was asserting on a nonexistent kwarg's extra_forbidden error) and added tests: trajectory parity, budget-timeout-vs-orphan verdict preservation, reason order-invariance, advisory-demotion gate consequence, agentless/unregistered guard coverage (early_stop.py:231), decision-budget HTML tooltip, and a pinned documented-gap test for pass-stop cutting an undecided fail-only-decidable sibling (with a TASK_DEFINITION_GUIDE caveat). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eQL py/mixed-returns) CodeQL does not model assert_never's Never return, so the match arm read as an implicit-None fall-through mixed with explicit returns. Bind the note per arm and return once; the assert_never exhaustiveness guard is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7fcbe4f to
eb9fe79
Compare
… every path The mixed-returns fix traded alert 58 for py/uninitialized-local-variable (alert 59): CodeQL does not model the assert_never arm as raising, so `return note` looked reachable with `note` unbound. Default to the generic note before the match; exhaustiveness checking is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Replaces the run-level early-stop master arm with per-criterion arming: a
stop_early:block (StopEarlyPolicy) on a live-observable criterion (skill_triggered,command_executed) alone activates theEarlyStopWatcher— there is no run-level master switch anymore.stop_early: {}arms with the implicit fail trigger;on_pass: stopopts into pass-stops;decide_within: Nlatches an effective fail (decision_budget_exceeded) after N tool-call steps, fed through the ordinary weighted gate (not a force-fail).run_limits.stop_earlyis demoted to a kill switch:falseforce-disarms every block (the one-line e2e / authoritative-P/R/F1 override for experiment variants);true(the removed master arm) is a hardEarlyStopConfigErrorat resolution (plan exits 1).stop_early_gate_threshold(default 1.0 = strict AND); pass-stop fires when theon_pass: stopsubset's floor already meets it. Both stops defer while a pass-capable armed criterion outside the deciding subset is undecided — recall is never truncated, and mixed arming (on_pass: stop+ a sibling'sdecide_within) composes safely.live_verdictfails open to a full run.Hardening (from two structured review passes)
allow_inf_nan=Falseon criterionweight(.infno longer NaN-poisons the weighted math)DEFAULT_STOP_EARLY_GATE_THRESHOLD; sharedearly_stop_gate_note()for md+html rendererson_eventfully behind the fail-open disarm wrapper;_prev_verdictslatched on resolved rounds onlyValueErrorinstead of a-O-stripped narrowing assert; clearer unregistered-agent guardrail messageBreaking changes
run_limits.stop_early: trueis now a hard resolution error (use per-criterionstop_early:blocks)early_stop:→stop_early:;CriterionEarlyStop→StopEarlyPolicydecision_budget_exceededis now an ordinary weighted fail, not a force-FAILURETest plan
make verifygreen: 3850 passed, 3 skipped, coverage 91.34%Orchestrator._setupactivation seam, in-flightdecide_withinexpiry, mixed-arming pass-stop deferral,load_experimentoverexperiments/early-stop-ab.yaml, fail-closed gate pinscoder-eval planresolves all fixtures + the 2-variantearly-stop-abexperiment;stop_early: truefails plan with exit 1Breaking-change blast radius (
run_limits.stop_early: true)run_limits.stop_early: true— previously the ONLY documented arming lever, and-D-reachable — now raisesEarlyStopConfigErrorat resolution (abortsrun, flipsplan's exit code) with no deprecation window. Verified no in-repo config sets it (grep -rn stop_early .github/ experiments/ tasks/shows notrue). External callers (anything using the composite action'sextra-argswith-D run_limits.stop_early=true, downstream experiment YAMLs, workflows pinned to the movingv<major>tag) should grep for the keystop_early: true/-D run_limits.stop_early=trueand migrate to per-criterionstop_early:blocks. No shipped nightly suite is armed today, so no pass-rate trend step is expected from the fired-only gating flip.🤖 Generated with Claude Code