fix(early-stop): add stop_when 'auto' — per-instance arming + pass-armed-subset stop rule#51
Conversation
…med-subset stop rule Under v0.8.9 any-engagement scoring, each skill_triggered instance can decide only one polarity mid-run (a positive can only live-pass, a distractor only live-fail), so 'decided' — the only arming value that fit a dataset-fanned suite — was rejected at validation for every instance. - stop_when: auto arms whichever polarities the instance can decide (live_decidable_polarities); a dead arm is a hard error at resolution. - Pass-stop now requires only the pass-armed subset to live-pass, with a vacuous guard so a row with zero pass-armed criteria never pass-stops. Fail-stop is unchanged and evaluated first. - The watcher resolves arming once into _armed_polarities; the stop rule never compares raw stop_when. - Precision trade-off (pass-stop can hide a later distractor misfire) documented in module docstring, CLAUDE.md, and the task guide.
|
Claude finished @mohsen-uipath's task in 1m 18s —— View job Code Review in Progress
|
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:51 (6 files) axis:1,2,3,4,5,6,7,8
Scope: pr:51 (6 files) axis:1,2,3,4,5,6,7,8 · branch fix/early-stop-auto-polarity (PR #51 head; local main at 947acd3) · f1a89dc · 2026-07-24T17:32Z · workflow variant
Change class: complex — adds a new stop_when: auto arming value and rewrites the early-stop pass-stop rule to a pass-armed subset, changing when a run is cut and therefore which frozen trajectory gets scored
coder_eval is in strong health — type safety, security, and error handling score a clean 10/10 and the architecture (single declarative merge resolver, plugin SPI, one EventCollector capture seam) is genuinely well-factored — but the real risk is concentrated in the new opt-in early-stop path, where stop_when: auto lets a fail-stop truncate a positive row into a false negative and lets truncated rows silently feed the suite_thresholds CI gate, so identical agent output can score differently under stop_early: true; bottom line: the code is production-grade, and what needs fixing is the harness's own measurement integrity, not its quality.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 9 / 10 | 0 | 0 | 2 | 0 | stop_when: decided is unsatisfiable for every shipping criterion, yet it is the guide's only copy-paste stop_early example and the validator's own suggested fix (hard resolution error) |
| 2. Type Safety | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 3. Test Health | 9.5 / 10 | 0 | 0 | 1 | 0 | New early-stop arming/stop-rule branches lack mutation-killing tests: decided arm never executed, pass_armed index remap only tested at index 0, fail-before-pass precedence untested |
| 4. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 5. Architecture & Design | 9.8 / 10 | 0 | 0 | 0 | 2 | Rewritten Verdict bullet asserts "identical verdicts" and negates it in the next sentence |
| 6. Error Handling & Resilience | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 7. API Surface & Maintainability | 9.7 / 10 | 0 | 0 | 0 | 3 | New auto dead-arm EarlyStopConfigError identifies the criterion only by c.type (and drops the sibling error's concrete fix hint), so it is unactionable in stacked/fanned suites |
| 8. Evaluation Harness Quality | 8 / 10 | 0 | 2 | 0 | 0 | stop_when: auto fail-arms every distractor instance, so on a positive row an exploratory read of a wrong skill fail-stops the run before the expected skill can load — the frozen row scores TP→FN and per-skill recall.yes/f1.yes in the suite rollup is deflated (effectively reverting the any-engagement policy to first-engagement whenever stop_early: true) |
Overall Score: 9.5 / 10 · Weakest Axis: Evaluation Harness Quality at 8 / 10
Totals: 🔴 0 · 🟠 2 · 🟡 3 · 🔵 5 across 8 axes.
Blockers
- [Axis 8]
stop_when: autofail-arms every distractor instance, so on a positive row an exploratory read of a wrong skill fail-stops the run before the expected skill can load — the frozen row scores TP→FN and per-skillrecall.yes/f1.yesin the suite rollup is deflated (effectively reverting the any-engagement policy to first-engagement wheneverstop_early: true) (src/coder_eval/orchestration/early_stop.py:378) —autoresolves tolive_decidable_polarities, which for everyskill_triggereddistractor instance (skill_name != expected_skill) is exactlyfrozenset({"fail"})(src/coder_eval/criteria/skill_triggered.py:210-211:return frozenset({"pass"}) if expected_yes else frozenset({"fail"})). The stop rule then fires on it unconditionally:if verdict == "fail" and "fail" in armed_pol:(early_stop.py:378), evaluated BEFORE the pass-stop. On the PR's motivating activation suite (~23 stacked criteria per row, all armed with one fannedstop_when: auto), a positive row where the agent reads.agents/skills/<wrong-skill>/SKILL.mdat tool call 1 fail-stops the run right there —_engaged_skill_namesmatches anyskills/<name>/substring in any string parameter, andlive_verdictreturns"fail"for that distractor. The frozen trajectory then scores the row's POSITIVE criterionscore=0.0,observed_label='no',expected_label='yes'(skill_triggered.py:145-152), i.e. a false negative, where the same trajectory understop_early: falsewould have reached the expected skill and scored 1.0 / TP.overlay_classification_metricscomputesrecall = tp / (tp + fn)(criteria/_classification_aggregate.py:62) from exactly those labels, so per-skill recall/F1 in the suite rollup collapses in the smoke flavor. This directly defeats the documented intent of the criterion the feature was built for — skill_triggered.py'slive_verdictdocstring states "a wrong skill engaged first no longer live-fails a positive row — the run keeps going so the expected skill can still load" — and it is NOT covered by the new trade-off note, which discloses only the pass-stop precision loss (early_stop.py:30-36). Fix options (pick one, the choice is an open question): (a) suppress the fail-stop while any pass-armed criterion on the row is stillundecided, so the recall signal is always allowed to resolve; (b) giveautoa narrowing form (e.g.auto-pass) so a fanned suite can arm per-instance on the pass side only; (c) at minimum, extend the module docstring + guide to state that a fail-stop truncates the recall signal and recorddeciding+ the undecided pass-armed criteria inEarlyStopInfoso those rows can be excluded from recall. - [Axis 8]
suite_thresholdsgate (drives CLI exit code) pools truncated early-stopped rows with complete rows and carries no suite-level marker — the trade-offautonewly makes reachable is recorded only in prose (early_stop.py:30-36, TASK_DEFINITION_GUIDE.md:259-274) (src/coder_eval/orchestration/early_stop.py:30) — The new note says "the frozen trajectory then scores that row as a clean pass ... the authoritative precision/recall must come from a non-early-stop (stop_early: false) run" (early_stop.py:30-36), but nothing mechanically records or separates truncated rows at the aggregate level, andautois precisely what first makes an armed dataset-fanned suite possible (pre-PR, one staticstop_whenon a fanned criterion whose role flips per row was rejected at resolution for most rows — see tests/test_early_stop.py:488-493). Verified:_compute_suite_rollupfeeds EVERY row intochecker.aggregate(criterion, per_rows)with no early-stop filter (src/coder_eval/reports.py:778-805),SuiteRolluphas no truncated-row field (src/coder_eval/models/results.py:786-814), andSuiteRollup.passedis documented as "Drives CLI exit code for dataset-backed tasks" (results.py:808-813) and is consumed as such (failed_gates = [r for r in rollups if not r.passed], src/coder_eval/cli/run_command.py:700-708). So asuite_thresholds: {precision.yes: 0.9}gate can go green on a smoke run that a full run fails. Compounding it, the armed-subset verdict gates on criteria that were never observed:armed_criteria_passedfilters onc.stop_when is not None(results.py:656-658), so on a pass-stop the ~22 fail-armed distractors are gating and trivially score 1.0, andreports_html._render_criteriamarks only NON-armed criteria advisory (elif early_stop is not None and f"{cr.criterion_type}: {cr.description}" not in armed:, reports_html.py:574-582), so an unobserved distractor renders as a genuine armed pass. Add a suite-level surface (e.g.SuiteRollup.rows_stopped_early+ aCriterionAggregatemetric/marker, mirrored in suite.md) and either exclude truncated rows from classification metrics or fail the gate when a threshold-gated criterion was unobserved on any row; the guide's own observability table (docs/TASK_DEFINITION_GUIDE.md:266-274, "every early-stopped run is flagged everywhere so analysis never compares a truncated run against a full one") lists only per-row markers and should be extended.
Non-blocking, but please consider before merge
- [Axis 1]
stop_when: decidedis unsatisfiable for every shipping criterion, yet it is the guide's only copy-pastestop_earlyexample and the validator's own suggested fix (hard resolution error) (src/coder_eval/models/criteria.py:126) — Both observable checkers narrowlive_decidable_polaritiesto at most ONE polarity per instance —skill_triggered.py:210-211returnsfrozenset({"pass"}) if expected_yes else frozenset({"fail"})andcommand_executed.py:61-67adds"pass"xor"fail"(or neither). Soneeded = {"pass", "fail"} if polarity == "decided" else {polarity}(early_stop.py:175) can never be satisfied:stop_when: decidedALWAYS raisesEarlyStopConfigErrortoday, which the PR's own suite asserts (tests/test_early_stop.py:488-491, :519, :523-525). Consequences the diff leaves in place: (a) the guardrail message at early_stop.py:132-135 still advertises the guaranteed-invalid value —"arming requires at least one stop criterion (e.g. stop_when: decided)."— so a user following the error hits a second error; (b) the widened Literal now ships four values of which, for every in-tree criterion instance, exactly one is valid andautoalways resolves to it (pass/faildiffer fromautoonly as a strict-intent assertion,decidedis dead). Per CLAUDE.md's greenfield rule ("No worries about backward compatibility") the KISS option is to redefinedecidedas the per-instance decidable set (i.e. whatautonow does) and drop the fourth member; ifautois kept, at minimum change the example in the early_stop.py:134 message tostop_when: autoand note in thestop_whendescription + docs/TASK_DEFINITION_GUIDE.md:233 that no shipped criterion instance can satisfydecided. - [Axis 1]
stop_when→ armed-polarity mapping duplicated betweenvalidate_early_stopandEarlyStopWatcher._resolve_armed_polarities(and validate_early_stop grows to D(21)) (src/coder_eval/orchestration/early_stop.py:162) — radon on the PR-head file reportsF 81:0 validate_early_stop - D (21)(wasC (19)on main) — the newautoblock at lines 162-174 with itscontinueis what tips it over. The same block also creates a second source of truth for the polarity mapping: the validator computesneeded = {"pass", "fail"} if polarity == "decided" else {polarity}(line 175) while the watcher re-derives the identical mapping in_resolve_armed_polarities(lines 336-343:if sw == "auto": ... if sw == "decided": return frozenset({"pass", "fail"}) ... if sw in ("pass", "fail"): return frozenset({sw})). Adding or renaming a polarity value now requires editing both. Hoist one module-level helper, e.g.def _requested_polarities(stop_when: str, decidable: frozenset[str]) -> frozenset[str]returningdecidableforauto, and have the validator dorequested = _requested_polarities(polarity, polarities)thenif not requested: <dead-arm error>/if requested - polarities: <undecidable error>— that removes theautospecial-case branch (dropping the function back under D) and makesEarlyStopWatcher._armed_polaritiesa call to the same function. It also removes the third restatement of the mapping in prose: the same rule is currently written out in the__init__comment (lines 218-223) and again in the_resolve_armed_polaritiesdocstring (lines 328-335) directly above the eight lines of code that say it. - [Axis 3] New early-stop arming/stop-rule branches lack mutation-killing tests:
decidedarm never executed, pass_armed index remap only tested at index 0, fail-before-pass precedence untested (tests/test_early_stop.py:1117) — The new module docstring at early_stop.py:34-35 makes the ordering load-bearing for the documented trade-off:trade of the opt-in "smoke" flavor (an already-visible misfire still fail-stops,/since fail-stop is evaluated before pass-stop each round), and CLAUDE.md + docs/TASK_DEFINITION_GUIDE.md repeat it. Nothing pins it: physically swapping the fail-stop block (early_stop.py:373-380) with the pass-stop block (:382-398) still passes 129/129 in tests/test_early_stop.py. The uncovered case is a single_evaluateround in which a pass-armed criterion live-passes AND a fail-armed criterion live-fails — under the swapped order that row recordsEarlyStopReason.CRITERION_PASSEDinstead ofCRITERION_FAILED, corruptingEvaluationResult.early_stop.reason, the report badge/notes, and theEarlyStopReasontelemetry dim for exactly the misfire rows the activation suite exists to count. The nearest existing tests do not reach it:test_stacked_wrong_skill_fail_stop(line 1045) leaves the positiveundecided, andtest_auto_negative_row_misfire_fail_stops(line 1102) has zero pass-armed criteria, so in both the pass-stop branch cannot fire regardless of order. Add a test where both polarities are decided in one round (criteria[_skill_crit("date-teller","date-teller",stop_when="auto"), _skill_crit("weather-teller","date-teller",stop_when="auto")], fed so both skills are engaged before the deciding evaluation) and assertreason == EarlyStopReason.CRITERION_FAILED.
Nits
- [Axis 5] Rewritten Verdict bullet asserts "identical verdicts" and negates it in the next sentence (
docs/TASK_DEFINITION_GUIDE.md:258) — The bullet keeps the pre-PR invariant claim — lines 256-258: "This is what lets one file serve both asmokeflavor (stop_early: true) and ane2eflavor (stop_early: false) with identical verdicts" — and then immediately contradicts it in the sentence the PR appended: "Note: on a pass-stop the run is cut once the positive is decided, so a distractor that would misfire on a later tool call is not observed (the frozen row scores as a clean pass)". Those cannot both hold: a smoke row that pass-stops scores PASS where the e2e row would score FAIL. The claim was true before this change for the observable criteria — a distractor can never live-pass, so the oldall_permit_pass = all(c.stop_when in ("pass", "decided") ...)veto meant a mixed pass/fail arming could only ever fail-stop, and a fail-stop is verdict-preserving under the monotone any-engagement latch. The new pass-armed-subset rule is what breaks it (and not only forauto— the PR's owntest_mixed_static_arming_pass_stops_ignoring_fail_armedshows staticpass+failarming now pass-stops where it previously could not, contradicting the PR description's "pass/fail/decided semantics untouched"). Delete or qualify the "identical verdicts" clause so the guide states one rule: verdict-preserving on a fail-stop, precision-optimistic on a pass-stop. - [Axis 5] Third index-aligned parallel list in EarlyStopWatcher, with raw positional indexing replacing the zip(strict=True) guard (
src/coder_eval/orchestration/early_stop.py:224) — The watcher now carries three lists that must stay positionally aligned —self._armed(line 217),self._armed_polarities(line 224, built by a comprehension overarmed), andself._prev_verdicts(line 236,["undecided"] * len(armed)) — plus the locally builtverdicts. The fail-stop loop keeps the safety net (zip(self._armed, verdicts, self._armed_polarities, strict=True), lines 375-377), but the new pass-stop branch drops it for raw index arithmetic: line 389pass_armed = [i for i, pol in enumerate(self._armed_polarities) if "pass" in pol], thenverdicts[i],self._prev_verdicts[i],self._armed[i][0]andself._armed[pass_armed[-1]][0](lines 390-396). Lengths are structurally guaranteed today (all three derive fromarmed, and_prev_verdictsis only reassigned to a same-lengthverdictsat line 401), so there is no bug — but this is thelist[i] ⟷ list[i]coupling shape this repo treats as a lint-rule-worthy smell, and it is now three-wide with nostrict=Trueon the hot branch. Collapse to one list of records (a small frozen dataclass or NamedTuple_ArmedEntry(criterion, checker, polarities)plus a mutableprev_verdict), sopass_armedbecomes a filter over entries and the alignment is unrepresentable-if-wrong rather than assert-if-remembered. - [Axis 7] New
autodead-arm EarlyStopConfigError identifies the criterion only byc.type(and drops the sibling error's concrete fix hint), so it is unactionable in stacked/fanned suites (src/coder_eval/orchestration/early_stop.py:169) — The new branch raisesf"criterion type {c.type!r} is armed (stop_when='auto') but this instance can " + "decide no polarity mid-run; 'auto' requires at least one live-decidable " + "polarity (its decidability can depend on the criterion's fields)."(early_stop.py:169-173). Executed against a task stacking twocommand_executedcriteria — a valid one (description="ran pytest", min_count=1) and a dead arm (description="ran ruff (BAD ARM)", min_count=0, max_count=None), bothstop_when: auto— the message emitted is exactly:criterion type 'command_executed' is armed (stop_when='auto') but this instance can decide no polarity mid-run; 'auto' requires at least one live-decidable polarity (its decidability can depend on the criterion's fields).It names neither the offending criterion'sdescription(a required field on every criterion, and the natural identifier — the watcher already usesf"{c.type}: {c.description}"forEarlyStopInfo.armed_criteria, early_stop.py:412) nor its index, so with stacked same-type criteria the author must bisect by hand. It is also less actionable than the pre-existing polarity error 12 lines below, which does spell out the fix ("command_executed can live-pass only with max_count unset + min_count>0, and live-fail only with max_count set", early_stop.py:182-184). Fix: includec.description!rin theautomessage and append the same concrete field guidance (e.g. "set min_count>0 with max_count unset to make pass decidable, or set max_count to make fail decidable"). - [Axis 7]
_resolve_armed_polarities's non-exhaustive fall-through silently returns an empty (inert) polarity set instead of failing loud (src/coder_eval/orchestration/early_stop.py:343) —_resolve_armed_polaritiesends with a barereturn frozenset()(early_stop.py:343) after an if-chain over"auto"/"decided"/("pass", "fail"), documented as covering onlyNone("Noneis never armed, but is mapped to the empty set defensively", :333-334). Because the parameter is a stringly-typed read ofcriterion.stop_whenintofrozenset[str], pyright gives no exhaustiveness check: adding a fifth value toLiteral["pass", "fail", "decided", "auto"](models/criteria.py:126) plus avalidate_early_stopacceptance branch — precisely the two-site edit this PR just performed — and forgetting this mapper yields an empty polarity set, so the criterion is armed, passes validation, appears inEarlyStopInfo.armed_criteria, and can never fire either branch ("fail" in armed_polat :378 and"pass" in polat :389 are both False). That is exactly the silent no-op the module's own contract forbids ("Rejects every configuration v1 cannot honor as a hard error, so an unsupported arming is never a silent no-op", :7-9). Fix: replace the trailingreturn frozenset()with an explicitif sw is None: return frozenset()followed bytyping.assert_never(sw)(or a raise naming the unhandled value), so a future value fails loudly at the point of omission instead of degrading to a dead arm. - [Axis 7] The CLAUDE.md early-stop bullet grew to 2589 chars — 1.9x the next-longest bullet — becoming a fourth near-verbatim copy of the polarity semantics with no single source of truth (
CLAUDE.md:144) — Measured at PR HEAD:awk 'NR==144{print length($0)}' CLAUDE.md= 2589 chars, up from 1438 onmain(+80%), against 1395 for the next-longest bullet in the file (line 141). The polarity/pass-armed-subset/precision-trade-off text now exists in four places that must be kept in sync by hand:models/criteria.py:128-146(the field description, the repo's declared SSOT),CLAUDE.md:144,docs/TASK_DEFINITION_GUIDE.md:231-262, and theearly_stop.py:30-36module docstring. Finding #2 is direct evidence the drift is real, not hypothetical — this PR updated three of those copies and left the downstream "identical verdicts" restatements stale in the same commit. Since CLAUDE.md is prepended to every assistant session, the cost is also per-call context. Fix: compress CLAUDE.md:144 back toward its neighbours' length — keep the invariant (armed subset gates an early-stopped run;autoresolves per instance; pass-stop needs only the pass-armed subset; authoritative P/R needsstop_early: false) and delegate the workedskill_triggeredpositive/distractor rationale to the single detailed home at docs/TASK_DEFINITION_GUIDE.md#stop_early-opt-in-early-stop, which the bullet should link rather than restate.
What's Missing
Parallel paths:
- 🟠 The PR added the pass-stop precision caveat to docs/TASK_DEFINITION_GUIDE.md, CLAUDE.md and the early_stop.py docstring but left the two A/B surfaces that make the opposite promise untouched: docs/AB_EXPERIMENTS.md:259-264 still says "Expect identical pass/fail verdicts between the two variants ... so the
smokeflavor can't 'pass for free'", and the shipped recipe experiments/early-stop-ab.yaml:10-12 repeats it verbatim in itsdescription. Under the new pass-armed-subset rule a mixed-arming row pass-stops with its fail-armed distractors unobserved (scoring 1.0), so the smoke flavor CAN pass for free — both surfaces need the same qualification (verdict-preserving on a fail-stop, precision-optimistic on a pass-stop). (trigger: docs/TASK_DEFINITION_GUIDE.md) (restates: Axis 5: Rewritten Verdict bullet asserts "identical verdicts" and negates it in the next sentence) - 🟡
RunLimits.stop_early's own field description (src/coder_eval/models/limits.py:90-99) — the user-facing SSOT rendered in schema/-Dhelp and a fifth copy of the semantics — was not updated: it still says the run ends "as soon as the armed criteria (those with stop_when set) are decided mid-run - on pass or on a definitive fail", which no longer describes the pass-armed-subset pass-stop, and it never mentionsauto. The PR updated four of the five copies (criteria.py description, CLAUDE.md, the guide, the module docstring) and missed this one — concrete evidence that the duplicated prose drifts on every edit. (trigger: src/coder_eval/models/criteria.py) (restates: Axis 7: The CLAUDE.md early-stop bullet grew to a fourth near-verbatim copy of the polarity semantics with no single source of truth) - 🟡
coder-eval planwas not taught about dataset fan-out, even thoughautoexists specifically for fanned suites: plan_command.py:105-138 callsload_task→resolve_task_for_variant→validate_early_stopon the unexpanded task, whereas the run path validates post-expansion (orchestration/experiment.py:625 then :673). Verified empirically: a fannedskill_triggeredwithskill_name: ${row.skill}/expected_skill: ${row.expected_skill}resolves tolive_decidable_polarities == frozenset({'fail'})because the two literal placeholders differ — so plan validates a fictitious distractor. Consequences: plan can never catch a per-row dead arm nor preview which polarity a row will arm (the one thing an author of a 1150-row suite needs), and a fanned criterion armed with staticpassspuriously fails plan with exit 1 while the real run is valid. (trigger: src/coder_eval/orchestration/early_stop.py)
Tests:
- 🟠 tests/test_early_stop.py contains zero occurrences of
dataset(verifiedgrep -c dataset= 0), so the PR's entire motivating scenario — ONE criterion fanned over rows whose positive/distractor role flips per row — has no test. The closest new case,test_auto_stacked_activation_accepts(:597), hand-stacks static criteria in a single task. Missing: an expand_dataset → per-rowvalidate_early_stop→ per-rowEarlyStopWatcher._armed_polaritiestest asserting the SAME criterion resolves pass-armed on a positive row and fail-armed on a distractor row (the exact behaviorautowas added to provide, and the only place the placeholder-vs-expanded distinction above would be caught). (trigger: tests/test_early_stop.py) - 🟡
automakes the documentedlive_decidable_polarities ⊆ live_stop_polaritiesinvariant (criteria/base.py:247) load-bearing —_resolve_armed_polaritiesarms straight from the per-instance set, bypassing the class-levellive_stop_polaritiesgate at early_stop.py:150 — yet the invariant is still only asserted by two hand-written per-criterion loops (tests/test_early_stop.py:418 for command_executed, :449 for skill_triggered). No registry-wide parametrized test over every registered checker was added, so a future/plugin criterion whose override widens the set would arm a polarity itslive_verdictnever emits: validation accepts it and the arm is a silent no-op, precisely what the module contract forbids. (trigger: src/coder_eval/orchestration/early_stop.py) - 🟡
TestOrchestratorEarlyStopWiring(tests/test_early_stop.py:1355-1500) gained no case usingautoor mixed arming, so nothing pins the end-to-end consequence of the new pass-stop rule: on a pass-stop the never-observed fail-armed distractors are still counted as gating passes byarmed_criteria_passed(models/results.py:656-664) and render as genuine armed passes in HTML (reports_html.py:582, which marks only NON-armed criteria advisory). The new behavior is asserted only at watcher level (verdict + reason), never at the verdict/report level where it actually changes a row's meaning. (trigger: tests/test_early_stop.py) (restates: Axis 8: suite_thresholds gate pools truncated early-stopped rows with complete rows and carries no suite-level marker)
Downstream consumers:
- 🟠 The pass-armed-subset rule changes which trajectories the classification labels are computed from, but no consumer of those labels was updated:
_compute_suite_rollupstill feeds every row intochecker.aggregatewith no early-stop filter (reports.py:775-807),overlay_classification_metricscomputesrecall = tp/(tp+fn)/ precision from the truncated labels (criteria/_classification_aggregate.py:62), andSuiteRollup.passed— documented as driving the CLI exit code (models/results.py:808-814) — is consumed unchanged at cli/run_command.py:701. So asuite_thresholdsgate can go green (or red) on truncated evidence with nothing in suite.json / suite.md indicating any row was cut. (trigger: src/coder_eval/orchestration/early_stop.py) (restates: Axis 8: suite_thresholds gate pools truncated early-stopped rows with complete rows and carries no suite-level marker) - 🟡 Arming is now a two-notion concept and only one notion was updated: the watcher resolves per-instance polarities into
_armed_polarities(early_stop.py:224), while the frozen-trajectory gate still derives "armed" from a raw polarity-blindc.stop_when is not Noneread (models/results.py:656-657), as doesEarlyStopWatcher.for_task(:264) and the HTML advisory set (reports_html.py:574). The gate therefore cannot tell that a fail-armed distractor was structurally unobservable at the cut — the same raw-.stop_when-read class the PR's own new harness-candidate proposes banning inside the stop rule, left in place in the gate that decidesfinal_status. (trigger: src/coder_eval/orchestration/early_stop.py) (restates: Axis 8: suite_thresholds gate pools truncated early-stopped rows with complete rows and carries no suite-level marker)
Display & mapping dicts:
- 🟡 No display surface was extended for the new per-instance arming, so
autois invisible everywhere after the run:EarlyStopInfo.armed_criteriais still a flat list of"type: description"strings (models/results.py:425-427) with no polarity, run.json rows carry onlystopped_early+early_stop_reason(reports_experiment.py:145-146), and the run.md note (reports.py:421-427), HTML badge (reports_html.py:348) and advisory marker (reports_html.py:574-582) all treat "armed" as one undifferentiated set. A grep forstop_whenoutside models/criteria.py + early_stop.py finds no renderer at all, so there is no way — in any report, JSON row, or telemetry dim — to see which polarity a criterion actually armed or that a listed armed criterion was never observable. (trigger: src/coder_eval/orchestration/early_stop.py)
Daily/nightly:
- 🟠 The stated motivation is the out-of-tree nightly activation suite (~23 criteria × ~1150 rows), yet the blast radius on that pipeline is unstated and unwired: no in-tree task or experiment adopts
stop_when: auto(grep over tasks/ finds nostop_whenat all; experiments/early-stop-ab.yaml only flipsstop_early), so nothing exercises the new value through the run path, and the PR does not say whether the nightly switches tostop_early: true+auto. It matters because the run-record schema is unchanged while the meaning of the classification columns changes for stop_early runs — every positive row whose agent touches a wrong skill first now fail-stops and books TP→FN, deflating the per-skill recall/F1 the nightly rollup (and anysuite_thresholdsCI gate reading it) reports. (trigger: CLAUDE.md) (restates: Axis 8:stop_when: autofail-arms every distractor instance, deflating per-skill recall/f1)
Harness & Lint Improvements
Static checks (lint / type):
- [ce-lint] CE032 — doc YAML examples must survive the RESOLUTION-time validators, not just Pydantic. Extend tests/lint/doc_examples.py (today's CE029 only calls
model_validate) with a second pass: any fenced yaml block containingstop_when:/stop_early:is synthesized into a minimal TaskDefinition (inject placeholdertask_id/description/initial_prompt, a per-criteriondescription, andagent: {type: claude-code}when absent — the guide's snippets are fragments, which is why CE029 skips them today) and then run throughorchestration/early_stop.validate_early_stop; any raise is a lint failure. Wire asTestCE032DocExamplesResolvein tests/test_custom_lint.py besideTestCE029DocYamlExamples(tests/lint/runner.py is AST/.py-only, so this belongs in the doc-surface pytest family with CE027–CE031). EMPIRICALLY VERIFIED against PR head f1a89dc: exactly one block fires — docs/TASK_DEFINITION_GUIDE.md:214-225 -> "criterion type 'skill_triggered' cannot decide polarity ['fail'] mid-run (stop_when='decided') … it supports ['pass']" — and zero other doc blocks, so false-positive load is 0. Prevents: Finding 1 (stop_when: decidedis unsatisfiable for every in-tree criterion, yet it is the guide's only copy-pasteablestop_earlyexample). Generalizes to any future config key whose real gate is a resolution-time validator rather than a Pydantic validator. - [ce-lint] CE033 — an advertised config value must be satisfiable by some in-tree criterion (arming matrix as SSOT). Add a curated arming-matrix fixture (observable criterion shapes x stop_when values:
skill_triggeredpositive AND distractor,command_executedwith min_count>0 / max_count-set / neither) that derives the satisfiablestop_whenset by runningvalidate_early_stop; then scan user-facing message string literals in src/ and docs prose for thestop_when: <value>pattern and fail when a named value is not in that set. Would have caught src/coder_eval/orchestration/early_stop.py:134 — "arming requires at least one stop criterion (e.g. stop_when: decided)." — i.e. the validator's own suggested fix is a guaranteed second error. DESIGN NOTE: the existingMINIMAL_PAYLOADSdict (tests/test_success_criterion_union.py:31, already parity-asserted against the union) is NOT sufficient alone — itscommand_executedpayload decides no polarity and itsskill_triggeredpayload is a positive, sofail(legitimately satisfiable by a distractor) would false-positive; hence the curated matrix, which also serves as CE032's fixture set. Prevents: Finding 1's error-message half (early_stop.py:134 advertisingdecided), plus any future stop_when/backend/driver value named in an error hint or doc that no shipped implementation can honor. - [ce-lint] CE034 —
Literal-field string dispatch must be centralized in one resolver. Registry-driven AST rule (same shape as CE018's closed-enum denylist rule): given a table of (model Literal field -> owning function), forbid any==/!=/in (...)comparison of that field, or of a local directly assigned from it, against a string constant outside the owner. Seed entry:BaseSuccessCriterion.stop_when->orchestration/early_stop._requested_polarities(the hoisted helper Finding 2 recommends). At PR head this fires on early_stop.py:162 (polarity == "auto"), :175 (polarity == "decided") and :336-341 (_resolve_armed_polarities's chain); after the DRY fix only the owner remains.stop_when is not Nonearming filters (orchestrator.py:1428, models/results.py:657, early_stop.py:130/251) are identity checks and unaffected. New file tests/lint/rules/ce034_literal_dispatch_centralized.py, wired in tests/lint/runner.py + the[tool.ruff.lint].externallist. CONSIDERED AND REJECTED alongside it: a ruffC901gate for the radon D(21) growth the finding cites — measured, ruff scoresvalidate_early_stopat 10 (up from 8) while the tree max is 24 (_build_argv), so no tree-passing threshold would fire; the duplication rule is the enforceable part. Prevents: Finding 2 (stop_when->polarity mapping duplicated betweenvalidate_early_stopandEarlyStopWatcher._resolve_armed_polarities, plus two prose restatements). Also a precondition for the pyright check below: it guarantees exactly ONE exhaustive chain exists to terminate inassert_never. - [pyright] Terminate the single stop_when resolver with
typing.assert_never— no config change required — and optionally addreportMatchNotExhaustive = "error"to[tool.pyright]if it is written as amatch. Replace the barereturn frozenset()fall-through at src/coder_eval/orchestration/early_stop.py:343 with an explicitif sw is None: return frozenset()followed byassert_never(sw). EMPIRICALLY VERIFIED with this repo's own pyright andtypeCheckingMode = "standard": the current bare-return form still type-checks clean after adding a 5th member to the Literal, while theassert_neverform errors immediately — 'Argument of type "Literal['newvalue']" cannot be assigned to parameter "arg" of type "Never" in function "assert_never"'. So the exhaustiveness gate is available today at zero config cost. Prevents: Finding 7 (non-exhaustive fall-through silently returns an inert empty polarity set — a criterion that is armed, passes validation, is listed inEarlyStopInfo.armed_criteria, and can never fire, exactly the silent no-op the module docstring forbids). Also guards the two-site edit pattern this PR performed (widen the Literal + add a validator branch, forget the mapper). - [ce-lint] CE035 — no two parallel containers indexed by the same integer variable. AST rule: inside one function, flag >=2 distinct container expressions subscripted by the same Name when that Name is bound as an integer index (a
range()/enumerate()loop target, or a loop over a list comprehended from anenumerateindex — the one-hop dataflow needed to seepass_armed = [i for i, pol in enumerate(...)]thenfor i in pass_armed). MEASURED false-positive load on main: exactly 1 site — src/coder_eval/agents/claude_code_agent.py:99_distribute_output_tokens(floors/order/out/raw/weightsall indexed byi), a genuine local parallel-array algorithm that takes a# noqa: CE035debt marker. On PR head it additionally fires on src/coder_eval/orchestration/early_stop.py:345_evaluate(self._armed/self._prev_verdicts/verdictsindexed byi). This is the complement to bugbearB905(already active viaselect = ["B"]), which guardszip()withoutstrict=but is silent the moment code abandons zip for raw index arithmetic — precisely what the new pass-stop branch did. Prevents: Finding 5 (third index-aligned parallel list inEarlyStopWatcher: the fail-stop loop keepszip(..., strict=True)but the new pass-stop branch drops it forverdicts[i]/self._prev_verdicts[i]/self._armed[i][0]). Pushes the fix toward one record type where misalignment is unrepresentable. - [ce-lint] CE036 — a validation error raised while iterating user-authored items must name the offending item. AST rule: a
raiseinside aforoversuccess_criteria(direct, or via a name bound from a comprehension over it) whose message interpolates neither the item's.descriptionnor its index is a violation. MEASURED: fires on 4 in-tree sites at PR head — src/coder_eval/orchestration/early_stop.py:151, :169 (the newautodead-arm message), :179, and src/coder_eval/models/tasks.py:572 (success_criteria[{c.type!r}].suite_thresholds requires a dataset) — 3 pre-existing plus the new one, and every one is unactionable in a stacked or dataset-fanned suite where N same-type criteria coexist (the watcher already usesf"{c.type}: {c.description}"as the identity at early_stop.py:412, so the identifier exists). Slots in as tests/lint/rules/ce036_validation_error_names_item.py. Prevents: Finding 6 (the newstop_when='auto'dead-armEarlyStopConfigErroridentifies the criterion only byc.type, forcing hand-bisection in stacked suites), plus the same defect in the 3 pre-existing messages it drags in. - [ce-lint] CE037 — cap CLAUDE.md top-level bullet length so detail is delegated, not duplicated. Markdown check: fail when any line starting with
-in CLAUDE.md exceeds ~1500 chars, with a message telling the author to keep the invariant in CLAUDE.md and link the docs page for the worked rationale. MEASURED: main's longest bullet is 1434 chars, PR head's is 2577 (CLAUDE.md:144, +80%), against 1390 for the next-longest — so a 1500 cap is green today and fires exactly on this change. Wire as a lint-marked pytest class beside CE028's docs-index test (markdown, not AST). STRETCH (noisier, propose separately): n-gram overlap detection between CLAUDE.md and docs/*.md to flag near-verbatim prose duplication — that is the mechanical form of the 4-copy drift, but the length cap is the deterministic part. Prevents: Finding 10 (the early-stop bullet became a fourth near-verbatim copy of the polarity semantics, 1.9x the next-longest bullet, in a file prepended to every session). Indirectly reduces the drift surface behind Findings 1 and 4, where this PR updated three of four copies and left the fourth stale.
Harness improvements (not statically reachable):
- Smoke-vs-e2e parity replay test with an explicitly declared divergence table. Take a recorded event-stream fixture for an activation-shaped dataset-fanned suite (positive + distractors), drive
EarlyStopWatcherover it to produce the truncated trajectory, then runSuccessChecker.check_all+overlay_classification_metrics+reports._compute_suite_rollupover BOTH the truncated and the full trajectory, and assert every per-criterion label flip appears in a checked-in declared-divergence table — failing on any UNdeclared flip. This makes the parity claim executable instead of prose. Note a FIFTH copy of that claim the review did not flag: experiments/early-stop-ab.yaml still says "Expect identical pass/fail verdicts between the two variants", which the new pass-armed-subset rule falsifies — and that experiment file is the natural fixture for this test. Why not static: The divergence is a runtime property of a specific trajectory: it requires replaying an event stream through the watcher's latch and then through the whole scoring/aggregation path. No AST or grep can see that a truncated trajectory scores a row TP->FN. Prevents: Findings 8 (autofail-arms distractors -> TP->FN, recall/F1 deflation), 9 (truncated rows pooled into the suite gate), and 4 (docs + experiment YAML assert "identical verdicts" while the appended note negates it). - Measurement-integrity guard on the suite gate: (a) surface truncation at the aggregate level —
SuiteRollup.rows_stopped_earlyplus aCriterionAggregatemarker, mirrored into suite.json/suite.md — since_compute_suite_rollup(reports.py:775-807) currently feeds every row intoaggregate()with no early-stop notion whileSuiteRollup.passeddrives the CLI exit code (cli/run_command.py:701); (b) either exclude truncated rows from classification metrics or fail the gate when asuite_thresholds-gated criterion was unobserved on any row; (c) record the still-undecided pass-armed criteria inEarlyStopInfoso consumers can exclude those rows; (d) makerun_limits.stop_early+suite_thresholdsan explicit resolution-time error or loud warning invalidate_early_stop, backed by a test asserting astop_early: truerun cannot report a greenprecision.yesgate on truncated evidence. Why not static: Truncation is per-row runtime state, and the suite that motivated the feature lives out-of-tree (the activation suite is in coder-eval-uipath), so a lint rule over in-tree tasks/*.yaml would reach nothing — measured: zero in-tree YAML files setstop_earlyandsuite_thresholdstogether today (experiments/early-stop-ab.yaml sets only the former, tasks/sentiment_classification.yaml only the latter). Only a runtime guardrail plus a report/model field protects an external consumer. Prevents: Findings 9 (gate can go green on truncated evidence with no suite-level marker; armed-subset verdict counts never-observed distractors as armed passes) and 8 (deflatedrecall.yes/f1.yesgated in CI). - Add a curated ordering/precedence mutation smoke gate — e.g.
make mutate-smoke— that applies a small hand-written set of source mutations to hot invariant-bearing modules and fails if the module's own test file stays green. Seed mutations for orchestration/early_stop.py: swap the fail-stop block (:373-380) with the pass-stop block (:382-398); off-by-one on thepass_armedindex remap; delete thepass_armed and …vacuity guard. The reviewer ran mutation #1 by hand and tests/test_early_stop.py stayed at 129/129 (tests/test_threshold_enforcement.py at 15/15) — the docstring's load-bearing claim "fail-stop is evaluated before pass-stop each round" has no test owner. Pair it with the one missing direct test: both polarities decided in a SINGLE_evaluateround via one tool call engaging both skills, assertingreason == EarlyStopReason.CRITERION_FAILED. Why not static: Mutation survival is a property of the test suite's assertions, not of the source AST — no lint rule can tell whether an existing test would notice a semantically different but syntactically valid reordering. Prevents: Finding 3 (new arming/stop-rule branches exercised only in shapes where mutations survive: thedecidedarm is never executed, the pass_armed index remap is tested only at index 0, and fail-before-pass precedence is untested). - Give every prose-asserted behavioral invariant an executable owner: for claims like "identical verdicts", "fail-stop is evaluated before pass-stop each round", and "never a silent no-op", require the prose to name the test that pins it (
see tests/test_early_stop.py::test_…) and add a cheap collect-only check that every referenced test id exists. The existence half is mechanical (a lint-marked test); the truth half is the named test itself. This converts the current failure mode — an invariant stated in four places, updated in three — into a build break the moment behavior diverges from the claim. Why not static: Detecting that a paragraph asserts an invariant and then negates it two sentences later is semantic judgment; only the back-reference existence check is grep-able, and the invariant's truth can only be established by running the behavior. Prevents: Findings 4 ("identical verdicts" asserted and immediately contradicted), 3 (precedence ordering with no test owner), and the drift dimension of 1 and 10 (four hand-synced copies of the polarity semantics).
Top 5 Priority Actions
- Stop
auto's fail-stop from truncating the recall signal at src/coder_eval/orchestration/early_stop.py:378 — suppress the fail-stop while any pass-armed criterion on the row is stillundecided(or add anauto-pass-only narrowing) — because today a positive row where the agent reads a wrong skill's SKILL.md first freezes asobserved_label='no', flipping a TP to an FN and deflating per-skillrecall.yes/f1.yesfor identical agent output. - Make truncated rows visible and non-gating in the suite rollup: add a
rows_stopped_earlymarker toSuiteRollup(src/coder_eval/models/results.py:778) and either exclude early-stopped rows fromchecker.aggregate(...)at src/coder_eval/reports.py:791 or fail the gate when a threshold-gated criterion was unobserved, sinceSuiteRollup.passeddrives the CLI exit code (src/coder_eval/cli/run_command.py:701) and can go green on a smoke run that a full run fails. - Tighten the armed-subset verdict so never-observed criteria cannot score a pass —
armed_criteria_passedfilters only onc.stop_when is not None(src/coder_eval/models/results.py:656), so on a pass-stop the ~22 unobserved fail-armed distractors gate the run at a trivial 1.0 and render as genuine armed passes in the HTML report (src/coder_eval/reports_html.py:582). - Fix the two surfaces that hand users a guaranteed-invalid value by switching
stop_when: decidedtostop_when: autoin the guide's only copy-pastestop_earlysnippet (docs/TASK_DEFINITION_GUIDE.md:222) and in the guardrail hint at src/coder_eval/orchestration/early_stop.py:134, because no in-tree criterion instance can satisfydecidedand following the error message produces a secondEarlyStopConfigError. - Pin the load-bearing fail-before-pass precedence with a test that decides both polarities in one
_evaluateround (tests/test_early_stop.py:1117 — physically swapping the blocks at early_stop.py:373-398 leaves all 129 tests green), and while in that file collapse the duplicatedstop_when→polarity mapping (early_stop.py:162 vs :336) into one_requested_polaritieshelper, replace the silentreturn frozenset()fall-through at early_stop.py:343 withassert_never, and name the offending criterion'sdescriptionin theautodead-arm error at early_stop.py:169.
Stats: 0 🔴 · 2 🟠 · 3 🟡 · 5 🔵 across 8 axes reviewed.
…cided Review follow-up (PR #51): a distractor misfire on an early tool call no longer truncates a positive row before its expected signal can appear — cutting there froze a would-be TP as an FN and deflated suite recall/F1 under stop_early. The misfire is latched by the criteria's monotone semantics, so the deferred fail-stop still fires the moment every pass-armed criterion decides (fail-stop keeps precedence over pass-stop, now pinned by a same-round test); a row with zero pass-armed criteria (negative rows) defers nothing and fail-stops on the first misfire. Also from the review: - hoist the stop_when->polarity mapping into one _requested_polarities helper (validator + watcher), terminated by assert_never so widening the Literal without updating the mapping is a type error - name the offending criterion's description in the arming errors and add the concrete field guidance to the auto dead-arm message - replace the unsatisfiable 'stop_when: decided' example/hint with 'auto' (guide + validator hint) - state verdict parity as one-sided (fail-stop verdict-preserving, pass-stop precision-optimistic) in the guide, AB_EXPERIMENTS.md, experiments/early-stop-ab.yaml, RunLimits.stop_early, and a compressed CLAUDE.md bullet
|
Addressed the review in 5a9ba3b (plus a merge of main for the required up-to-date check): Blocker 1 (recall truncation) — fixed. The fail-stop is now deferred while any pass-armed criterion is still undecided ( Also from the review:
Blocker 2 (suite-rollup truncation marker / gate on truncated evidence) is deferred to a follow-up PR — it's a separate surface ( Validation: |
…cided Review follow-up (PR #51): a distractor misfire on an early tool call no longer truncates a positive row before its expected signal can appear — cutting there froze a would-be TP as an FN and deflated suite recall/F1 under stop_early. The misfire is latched by the criteria's monotone semantics, so the deferred fail-stop still fires the moment every pass-armed criterion decides (fail-stop keeps precedence over pass-stop, now pinned by a same-round test); a row with zero pass-armed criteria (negative rows) defers nothing and fail-stops on the first misfire. Also from the review: - hoist the stop_when->polarity mapping into one _requested_polarities helper (validator + watcher), terminated by assert_never so widening the Literal without updating the mapping is a type error - name the offending criterion's description in the arming errors and add the concrete field guidance to the auto dead-arm message - replace the unsatisfiable 'stop_when: decided' example/hint with 'auto' (guide + validator hint) - state verdict parity as one-sided (fail-stop verdict-preserving, pass-stop precision-optimistic) in the guide, AB_EXPERIMENTS.md, experiments/early-stop-ab.yaml, RunLimits.stop_early, and a compressed CLAUDE.md bullet
5a9ba3b to
415c910
Compare
CodeQL does not model typing.assert_never's Never return, so the bare call read as an implicit fall-through return mixed with explicit returns. 'return assert_never(...)' keeps the pyright exhaustiveness guarantee and makes every path explicit.

Problem
Since v0.8.9,
skill_triggeredscores any engagement of the target skill, which narrows each criterion instance to a single mid-run-decidable polarity: a positive instance (skill_name == expected_skill) can only live-pass, and a distractor instance can only live-fail.That broke early-stop arming for dataset-fanned suites. The criteria are written once in the task YAML but expanded per row, and a given instance's positive/distractor role flips from row to row — so no static
stop_whenvalue fits every row:decidedrequires the instance to be able to decide both polarities → now rejected at validation for every instance.pass/faileach fit only one role → wrong (and rejected) for the rows where the role flips.Net effect: activation-style suites (e.g. the skills-repo suite, 23 stacked
skill_triggeredcriteria × ~1,150 rows) could not use early stop at all.Change
stop_when: auto— arms whichever polarities this instance can actually decide (itslive_decidable_polarities). On a positive row the criterion arms pass; on a distractor row it arms fail. Anautothat resolves to zero decidable polarities (a dead arm) stays a hard error at resolution, same as the other values._armed_polaritiesat construction; the stop rule never inspects rawstop_whenvalues.stop_early: falserun. Noted in the module docstring, CLAUDE.md, and TASK_DEFINITION_GUIDE.Defaults unchanged:
stop_earlyremains opt-in;pass/fail/decidedsemantics are untouched.Testing
tests/test_early_stop.py(+119 lines): auto-arming resolution per instance, pass-armed-subset stop rule, vacuous-pass guard on negative rows, dead-arm resolution errors.skill_triggeredcriteria,stop_when: auto,max_turns: 3, real claude-code agent): 8/8 positive rows pass-stopped at SDK turn 1 / tool call 1 with verdicts identical to a full run (23/23 criteria, score 1.000, ~$0.05/row); the negative row correctly never armed a pass-stop and ran to its caps with a clean 23/23 classification.