feat(early-stop): weighted ceiling/floor bounds + decision-step budget - #74
Conversation
#61) Generalizes early-stop from a strict boolean gate to a weighted one (run_limits.stop_early_gate_threshold) so a low-weight armed criterion's failure can be absorbed without truncating the run, while a high-weight failure still fail-stops immediately via a mathematically safe ceiling/floor bound. Adds a per-criterion decision-step budget (max_steps_to_decide) that force-fails a run if an armed criterion never reaches a verdict in time. Refactors "is this criterion live-observable" from a checker-side ClassVar into LiveSuccessCriterion model subclassing, the single source of truth CE025 and validate_early_stop now consult. Addresses a code-review pass on this diff: closes a gate_threshold=0.0 gating-bypass gap, fails closed on a defensive zero-weight guard, adds CommandExecutedCriterion coverage alongside SkillTriggeredCriterion, and updates CLAUDE.md/EXTENDING.md for the LiveSuccessCriterion refactor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:74 — feat(early-stop): weighted ceiling/floor bounds + decision-step budget (19 files, +1071/-383)
Scope: pr:74 — feat(early-stop): weighted ceiling/floor bounds + decision-step budget (19 files, +1071/-383) · branch akshaya/early_stop_improvements · 517c70a · 2026-08-03T20:07Z · workflow variant
Change class: complex — introduces a new LiveSuccessCriterion model layer with an abstract per-instance live_decidable_polarities() contract, replaces the boolean armed-criteria stop rule with weighted ceiling/floor bound arithmetic against a new stop_early_gate_threshold, adds a per-criterion max_steps_to_decide decision budget with a new EarlyStopReason and forced-FAILURE path, and DELETES lint rule CE025; touches models, criteria checkers, orchestration/early_stop.py and orchestrator.py together
The codebase remains in strong shape — flawless security and harness-quality axes, near-perfect typing, and disciplined architecture — but this PR's new weighted early-stop gate and per-criterion decision budget carry three confirmed scoring hazards (a polarity-blind max_steps_to_decide that force-fails clean fail-armed distractors, a weighted gate that silently displaces pass_threshold and diverges from the full-run gate, and a validator that breaks the shipped early-stop-ab variant while plan still exits 0), each of which can change a task's final_status for identical agent output and none of which the test suite catches; fix those four scoring/resolution paths and backfill their tests before merge, and the rest is polish.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 8.6 / 10 | 0 | 0 | 2 | 4 | EarlyStopWatcher._evaluate grew from C(17) to D(22) and to a 107-line body (53 comment / 49 code) holding three inlined decision rules |
| 2. Type Safety | 9.9 / 10 | 0 | 0 | 0 | 1 | EarlyStopInfo.gate_threshold drops the ge=0.0, le=1.0 bounds carried by the RunLimits field it mirrors |
| 3. Test Health | 8.8 / 10 | 0 | 0 | 2 | 2 | The new weighted early-stop gate is undertested: threshold plumbing hardcoded to 1.0 in tests and the ceiling-bound tie boundary survives mutation |
| 4. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 5. Architecture & Design | 9.4 / 10 | 0 | 0 | 1 | 1 | Weighted-gate documentation misstates criterion scoring: armed scores are claimed binary 0/1 and the per-criterion pass_threshold is silently ignored on early-stopped runs |
| 6. Error Handling & Resilience | 7.9 / 10 | 0 | 2 | 0 | 1 | max_steps_to_decide is polarity-blind: a fail-armed / fail-only-decidable criterion that correctly stays undecided is force-failed as decision_budget_exceeded |
| 7. API Surface & Maintainability | 9.4 / 10 | 0 | 0 | 1 | 1 | docs/REPORT_SCHEMA.md's EarlyStopInfo section (lines 188-194) still enumerates the pre-PR contract, omitting the new decision_budget_exceeded reason value and the new gate_threshold field that this PR serializes into task.json |
| 8. Evaluation Harness Quality | 10 / 10 | 0 | 0 | 0 | 0 | — |
Overall Score: 9.3 / 10 · Weakest Axis: Error Handling & Resilience at 7.9 / 10
Totals: 🔴 0 · 🟠 2 · 🟡 6 · 🔵 10 across 8 axes.
Blockers
- [Axis 6]
max_steps_to_decideis polarity-blind: a fail-armed / fail-only-decidable criterion that correctly stays undecided is force-failed asdecision_budget_exceeded(src/coder_eval/orchestration/early_stop.py:527) — The budget check treatsundecidedas "the criterion never resolved", which is only true for a PASS-armed criterion. For a fail-armed one (skill_triggereddistractor, orcommand_executedwithmax_countset — both resolve tofrozenset({'fail'})fromlive_decidable_polarities()),undecidedis the SUCCESS state: the forbidden skill/command never fired. early_stop.py:527 does not distinguish them:
for (criterion, _checker), verdict in zip(self._armed, verdicts, strict=True):
budget = criterion.max_steps_to_decide
if budget is not None and verdict == "undecided" and tool_call_index >= budget:
self._fire(EarlyStopReason.DECISION_BUDGET_EXCEEDED, criterion, tool_call_index=tool_call_index)and orchestrator.py:1579 then hard-codes all_passed = False for that reason, so the run lands on FinalStatus.FAILURE. Nothing rejects the config: the new validator at models/criteria.py:249 only checks if self.max_steps_to_decide is not None and self.stop_when is None: — it never asks whether the armed polarity set can ever leave undecided. This is the missing inverse half of the new guard.
Verified against PR HEAD (517c70a): a distractor SkillTriggeredCriterion(skill_name='weather-teller', expected_skill='date-teller', stop_when='auto', max_steps_to_decide=2) reports decidable: frozenset({'fail'}) and, fed three benign Bash tool-ends, prints step 2 should_stop: True reason: decision_budget_exceeded — i.e. a perfectly clean run force-failed. This is the exact idiom both new example tasks use for distractors (tasks/early_stop_weighted_low_weight_absorbed.yaml:50 stop_when: "auto" on the min_count: 0, max_count: 0 curl distractor), and auto is documented as the value for dataset-fanned criteria whose positive/distractor role flips per row — so one max_steps_to_decide on a fanned criterion force-fails every negative row.
Fix: either restrict max_steps_to_decide to instances whose live_decidable_polarities() contains "pass" (raise in _check_max_steps_requires_armed, which cannot see it — so do it in validate_early_stop, which already has the instance and raises EarlyStopConfigError), or make the budget check skip criteria whose resolved arming is fail-only ("pass" not in self._armed_polarities[i]). Add a test mirroring tests/test_early_stop.py:1446 test_decision_budget_exceeded_when_still_undecided for a fail-armed criterion — every existing budget test uses stop_when="pass".
2. [Axis 6] stop_early_gate_threshold model validator rejects partial merge layers, breaking the shipped early-stop-ab e2e variant and -D override paths (and plan exits 0) (src/coder_eval/models/limits.py:145) — models/limits.py:145 rejects an inert-but-harmless value:
if not self.stop_early and self.stop_early_gate_threshold != 1.0:
raise ValueError(
"run_limits.stop_early_gate_threshold is set but run_limits.stop_early is "
+ "False; the threshold has no effect unless stop_early is also True."
)Because run_limits is field-merged across the 5 layers, a variant/-D that flips only stop_early: false inherits the task's threshold and trips this. That is exactly the shipped recipe: experiments/early-stop-ab.yaml:20 sets stop_early: false for the e2e variant, and this PR's own tasks/early_stop_weighted_low_weight_absorbed.yaml:33 sets stop_early_gate_threshold: 0.7. Verified on PR HEAD:
uv run coder-eval plan tasks/early_stop_weighted_low_weight_absorbed.yaml --experiment experiments/early-stop-ab.yaml
→ Variant 'e2e': resolution failed - 1 validation error for RunLimits / Value error, run_limits.stop_early_gate_threshold is set but run_limits.stop_early is False... followed by All tasks are valid! and exit code 0.
The fail-loud/degrade decision is backwards twice over. (a) The condition itself should degrade — an inert field under a layered override is not a misconfiguration. (b) Because it raises a bare ValueError from a pydantic model_validator, it lands in cli/plan_command.py:149's generic except Exception as e: branch, which prints red text but never sets all_valid = False — unlike the deliberate except EarlyStopConfigError branch at plan_command.py:143 that exists precisely so early-stop arming errors flip the exit code. On the run path (orchestration/experiment.py:702) the same exception discards the whole task file (BOTH variants, since file_resolved is committed as a unit) into skipped with only a logger.warning. Either way the PR's stated "every unsupported combination is rejected at resolution time, never a silent no-op" contract is not met for its own new field.
Fix: drop the stop_early: False half of the validator (keep the <= 0.0 half, which is a genuine gate-neutralization guard), or move the check into validate_early_stop so it raises EarlyStopConfigError and gets the hard-stop treatment. Add a resolution test covering task-sets-threshold × variant-sets-stop_early: false.
Non-blocking, but please consider before merge
- [Axis 1]
EarlyStopWatcher._evaluategrew from C(17) to D(22) and to a 107-line body (53 comment / 49 code) holding three inlined decision rules (src/coder_eval/orchestration/early_stop.py:426) — radon on the merge-base copy reportsM 373:4 EarlyStopWatcher._evaluate - C (17); on PR head it isM 426:4 EarlyStopWatcher._evaluate - D (22)— this diff added the +5. The method now inlines three independent decision rules (deferred fail-stop + ceiling bound at lines 474-487, pass-stop floor bound + flip attribution at 505-514, decision-step budget loop at 525-529) on top of the verdict-collection loop. Its 107-line body (426-532) is 53 comment lines vs 49 code lines — e.g. a 20-line comment block at 455-473 introducing a 14-lineif, and a 16-line block at 489-504 introducing a 10-lineif. Extract the three rules into small predicates the way_ceiling/_flooralready are (e.g._fail_stop_candidate(verdicts, pass_armed) -> LiveSuccessCriterion | None,_pass_stop_deciding(verdicts, pass_armed) -> LiveSuccessCriterion | None,_budget_exceeded(verdicts, tool_call_index) -> LiveSuccessCriterion | None) and move their explanatory prose onto those docstrings;_evaluatethen reads as collect-verdicts + three guarded_firecalls. - [Axis 1]
stop_early_gate_threshold's weighted forgiveness only applies when the watcher actually fires; astop_early: truerun that completes naturally falls back to the strictall_criteria_passedgate, so the absorbed low-weight failure re-sinks the run (docs/TASK_DEFINITION_GUIDE.md:412) — The guide this PR adds states it outright: "This weighting applies only when the watcher itself fires the stop. ... Whether a low-weight criterion's failure gets forgiven can therefore depend on whether the watcher actually fired, not solely on the configured threshold — writemax_steps_to_decideor a tightmax_turnsif you need the weighted gate to be the one that always applies" (docs/TASK_DEFINITION_GUIDE.md:408-418). Concretely: a task withstop_early: true, stop_early_gate_threshold: 0.7and armed criteria of weight 0.8 (score 1.0) / 0.2 (score 0.0) gates PASS if the watcher fired (orchestrator.py:1589, weighted 0.8 >= 0.7) and FAIL if the agent simply finished first (orchestrator.py:1599, strictall_criteria_passed) — identical agent behaviour, oppositefinal_status. The PR then shipsmax_steps_to_decide(models/criteria.py:219) as the recommended way to force the first branch, i.e. a third knob added to make the second knob predictable. Prefer making the armed weighted gate apply to any task that resolved withstop_early: true(or, conversely, keep the gate strictly on the full criteria set and letstop_earlybe a pure budget optimisation) so one task config maps to one gate semantic; if the contingency is genuinely intended, it belongs in the anchor doc as a warning rather than as a knob recommendation. - [Axis 3] The new weighted early-stop gate is undertested: threshold plumbing hardcoded to 1.0 in tests and the ceiling-bound tie boundary survives mutation (
src/coder_eval/orchestrator.py:1589) — The PR's headline user-facing behavior is "setrun_limits.stop_early_gate_thresholdin YAML and an early-stopped run gates differently".armed_criteria_passed(criteria, gate_threshold)is well unit-tested (tests/test_early_stop.py:1111-1157), but the two hops that carry the configured value out ofRunLimitsinto a score and into the persisted record have NO assertion:
(a)src/coder_eval/orchestrator.py:1589—gate_threshold = self.task.run_limits.stop_early_gate_threshold if self.task.run_limits else 1.0
(b)src/coder_eval/orchestration/early_stop.py:548—gate_threshold=self._gate_threshold,(the new persistedEarlyStopInfo.gate_thresholdfield,models/results.py:475)
I verified this by mutation on the PR-head checkout: replacing (a) withgate_threshold = 1.0and (b) withgate_threshold=1.0simultaneously, the full suite still reports3849 passed, 14 skipped.TestOrchestratorEarlyStopWiringnever builds a task with a non-default threshold (_run_wiringat tests/test_early_stop.py:1689 has no threshold parameter), andgrep -rn "gate_threshold" tests/shows zero assertions oninfo.gate_threshold/early_stop.gate_thresholdanywhere. Add (1) an orchestrator wiring test withstop_early_gate_threshold=0.7and armed scores[1.0, 0.0]at weights 0.8/0.2 asserting_evaluation_loop()returns True (and returns False at the default 1.0 for the same scores), and (2) an assertion intest_decision_budget_exceeded_when_still_undecided/ a_watcher(..., gate_threshold=0.7)test thatwatcher.info.gate_threshold == 0.7, plusgate_threshold == 1.0intest_info_defaults(tests/test_early_stop.py:1040). - [Axis 3] CE025's rewrite drops all 9 of its detection tests; the new registry check's only test asserts the already-clean tree, so neither violation branch is ever exercised (
tests/test_custom_lint.py:563) — The PR deletestests/lint/rules/ce025_live_verdict_consistency.pyand its runner wiring, replacing the rule with a whole-tree registry check whose ONLY test istest_real_criteria_tree_is_clean(tests/test_custom_lint.py:563), which assertsnot violationsagainst the current, already-clean tree. The deleted class had eight positive/negative detection tests (test_flags_polarities_without_live_verdict,test_flags_live_verdict_without_polarities,test_flags_live_verdict_with_empty_polarities,test_allows_polarities_with_live_verdict,test_allows_neither_declared,test_allows_empty_polarities_without_live_verdict,test_detects_plain_assign_polarities,test_non_literal_arg_treated_nonempty,test_scope_exemptions). Nothing now proves the new detector can FAIL: if_type_to_model()silently returns a short mapping (e.g. a futureLiveSuccessCriterionsubclass not added to theSuccessCriterionunion is simply absent fromget_args(inner)) orchecker_cls.live_verdict is not BaseCriterion.live_verdictstops discriminating, the test passes vacuously and the invariant CLAUDE.md and docs/EXTENDING.md both advertise as enforced is silently gone. Add two synthetic-fixture tests that feed the same violation logic a deliberately mispaired (model, checker) pair — oneLiveSuccessCriterionmodel whose checker does not overridelive_verdict, and one checker that overrides it whose model is a plainBaseSuccessCriterion— and assert each produces a violation string. Factor the loop body into a helper taking adict[str, type]so both can be driven without touching the real registry. - [Axis 5] Weighted-gate documentation misstates criterion scoring: armed scores are claimed binary 0/1 and the per-criterion
pass_thresholdis silently ignored on early-stopped runs (src/coder_eval/models/results.py:718) — FAILURE SCENARIO: a task withrun_limits: {stop_early: true}(defaultstop_early_gate_threshold: 1.0) and two armed criteria — A =skill_triggeredpositive (stop_when: auto, weight 1.0, defaultpass_threshold: 0.9) and B =command_executeddistractor (min_count: 0, max_count: 0, stop_when: auto, weight 1.0,pass_threshold: 0.0, the author's way of saying "trigger a fail-stop but don't gate on it"). Agent output: engages the expected skill (A scores 1.0) and once touches the forbidden command (B scores 0.0), so the deferred fail-stop fires. Pre-PR gate:all(...)→ A1.0 >= 0.9True, B0.0 >= 0.0True → SUCCESS. Post-PR gate at the default threshold:(1.0*1 + 0.0*1)/2 = 0.5 < 1.0→ FAILURE. Identical agent output, flippedfinal_status, on the code path this PR documents as byte-for-byte unchanged at the default.
ROOT CAUSE: armed_criteria_passed changed from return all(r.score >= c.pass_threshold for r, c in armed) to:
weighted_score = sum(r.score * c.weight for r, c in armed) / total_weight
return weighted_score >= gate_thresholdpass_threshold is no longer read at all on this path, making run_limits.stop_early_gate_threshold a third scoring authority alongside the two documented per-criterion knobs (weight, pass_threshold — docs/TASK_DEFINITION_GUIDE.md:585-587). The PR is aware of the displacement and pins it with tests/test_early_stop.py:1128-1141 (test_armed_criteria_passed_ignores_pass_threshold), whose comment argues it is "Currently safe only because the two live-observable criteria always score binary 0.0/1.0 in practice". That safety argument has a hole it does not cover: binary scoring is not the only way the two rules diverge — a pass_threshold of 0.0 also diverges, and it is the only remaining way to arm a criterion for stop-triggering without making it gating, because check_weight_zero_is_not_gating (models/criteria.py:159-174) hard-rejects weight: 0 together with stop_when. So the docstring claim at models/results.py:686-689 — "gate_threshold defaults to 1.0, which reproduces the pre-weighting all(r.score >= c.pass_threshold) behavior exactly" — is not true, and the escape hatch is removed without a replacement or a note.
FIX: either restore the per-criterion threshold inside the weighted form (score each armed criterion as 1.0 if r.score >= c.pass_threshold else 0.0 before weighting, which makes the equivalence claim literally true), or, if the displacement is intended, correct the docstring at models/results.py:686-689, document in docs/TASK_DEFINITION_GUIDE.md that pass_threshold is inert on the armed gate, and name the supported replacement for a non-gating armed criterion.
6. [Axis 7] docs/REPORT_SCHEMA.md's EarlyStopInfo section (lines 188-194) still enumerates the pre-PR contract, omitting the new decision_budget_exceeded reason value and the new gate_threshold field that this PR serializes into task.json (src/coder_eval/models/results.py:430) — models/results.py:430 adds DECISION_BUDGET_EXCEEDED = "decision_budget_exceeded" and :475 adds gate_threshold: float, both of which are serialized into task.json. docs/REPORT_SCHEMA.md:191-194 still enumerates the old contract verbatim: "Fields: reason (criterion_passed / criterion_failed), deciding_criterion_type, deciding_criterion_description, armed_criteria, sdk_turn_index, tool_call_index (1-based, includes the in-flight call), elapsed_seconds, turns_remaining_at_stop." REPORT_SCHEMA.md is the cross-repo contract consumed by the external eval-runner / evalboard, so a downstream consumer reading it will neither know the third reason value exists (it may bucket it into an "unknown" bin) nor that gate_threshold is available. docs/TASK_DEFINITION_GUIDE.md:448-453's observability table is likewise unchanged. Update REPORT_SCHEMA.md's EarlyStopInfo section to list decision_budget_exceeded and gate_threshold, and confirm the external consumer's early_stop_reason mapping has an entry for the new value (src/coder_eval/reports.py:438 uses t.get("early_stop_reason") or "unknown", which tolerates it, but the doc is what downstream reads).
Nits
- [Axis 1] Orphaned migration comments in both live checkers describe a
ClassVarthat no longer exists in those files (src/coder_eval/criteria/command_executed.py:33) —command_executed.py:33-39andskill_triggered.py:112-119are now free-floating comment blocks betweencriterion_type = "..."and the next method, left behind after thelive_stop_polaritiesClassVar they annotated was deleted. Both end with a "moved elsewhere" pointer: "Per-instance decidability narrowing now lives on the model (CommandExecutedCriterion.live_decidable_polaritiesin models/criteria.py) — "is this criterion type live-observable" is itsLiveSuccessCriterionsubclassing, not a checker-side ClassVar." On a greenfield project with no backward-compatibility concern (CLAUDE.md "Clean Code: No dead code"), a where-it-used-to-live note has no reader; the substantive half ("command matches accumulate monotonically... so a min_count pass and a max_count exceedance are both decidable") already duplicatesCommandExecutedCriterion.live_decidable_polarities's docstring at models/criteria.py:632-646. Delete both blocks. - [Axis 1] Example task
early_stop_weighted_high_weight_kills_run.yamldescribes an immediate fail-stop, but the watcher defers it while a pass-armed criterion is undecided (tasks/early_stop_weighted_high_weight_kills_run.yaml:6) — The YAML says "the ceiling ... is only 0.2, which can never reach the 0.7 gate threshold ... so the watcher must fail-stop immediately instead of burning the rest of run_limits.max_turns on a doomed run" (lines 6-11). But the fail-stop is guarded byif not any(verdicts[i] == "undecided" for i in pass_armed):(early_stop.py:474), and this task's low-weight positive (command_executed,min_count: 1,max_countunset,stop_when: "auto"→ decidable{"pass"}, lines 34-40) is pass-armed. The prompt orders the distractor first ("As a first step, sanity-check network access by running: curl ...", line 21), so at the moment curl misfires the positive is stillundecidedand the fail-stop is deferred — it fires only afterpython app.pyruns. Reword to "fail-stops as soon as the pass-armed positive resolves" so the example teaches the deferral rather than contradicting it. - [Axis 1] New
LivePolarityliteral type is widened back tofrozenset[str]at the watcher's consumption point (src/coder_eval/orchestration/early_stop.py:272) —LivePolarity = Literal["pass", "fail"]is introduced with the explicit rationale "Typed here (not a bare frozenset[str]) so a live_decidable_polarities override returning a stray/typo'd string, or "undecided" itself, is a pyright error rather than a runtime-only lint gap" (models/criteria.py:190-196), and_requested_polaritieswas correctly tightened tofrozenset[LivePolarity](early_stop.py:97-98). But the watcher immediately discards it:self._armed_polarities: list[frozenset[str]](early_stop.py:272) anddef _resolve_armed_polarities(criterion: LiveSuccessCriterion) -> frozenset[str](early_stop.py:382). Change both tofrozenset[LivePolarity](LivePolarityis already imported at early_stop.py:70) so the membership tests at lines 454 and 482 are checked against the closed set too. - [Axis 1] The weighted ceiling/floor rationale is restated verbatim across five surfaces that must now drift together (
src/coder_eval/orchestration/early_stop.py:30) — The same explanation of the ceiling/floor bounds, the default-1.0 collapse, and the deferral is written out in full at: early_stop.py:30-61 (module docstring), early_stop.py:455-473 + 489-504 (inline comments in_evaluate), models/limits.py:111-123 (thestop_early_gate_thresholddescription=, 13 lines), CLAUDE.md:145, and docs/TASK_DEFINITION_GUIDE.md:394-418. Two of these already disagree with the code (see the finding on tasks/early_stop_weighted_high_weight_kills_run.yaml, and the "binary 0/1" premise at results.py:687). Keep the full derivation in exactly one place — the guide section, which is the linked "worked rationale" — and reduce the module docstring, thedescription=string, and the inline comments to a one-line summary plus a pointer, per CLAUDE.md's DRY / "Field descriptions ... defined once" principle.models/criteria.py:222-237(the 16-linemax_steps_to_decidedescription=) has the same shape and can point at the guide's Decision-step budget bullet instead of restating the retry-accumulation caveat. - [Axis 2]
EarlyStopInfo.gate_thresholddrops thege=0.0, le=1.0bounds carried by theRunLimitsfield it mirrors (src/coder_eval/models/results.py:475) — The persisted copy is declared asgate_threshold: float = Field(default=1.0, description=...)(src/coder_eval/models/results.py:475-480) with no numeric constraints, while its single source —stop_early_gate_threshold: float = Field(default=1.0, ge=0.0, le=1.0, ...)(src/coder_eval/models/limits.py:107-110) — is bounded on both ends.EarlyStopInfois serialized intotask.jsonand re-parsed (its own docstring at results.py:438-440 calls out the round-trip), so a hand-edited or externally-produced record carryinggate_threshold: 7.5or-1validates cleanly and is rendered as a self-describing threshold by downstream consumers. Impact is limited to reporting today (nothing recomputes the gate from this field —orchestrator.py:1589reads the threshold fromrun_limits, not fromEarlyStopInfo), hence Low. Addge=0.0, le=1.0to theField(...)so the persisted mirror cannot represent a value the authoritative field rejects. - [Axis 3] Documented cumulative-across-retries step counting for max_steps_to_decide has no test (
src/coder_eval/models/criteria.py:232) — The new field's description states the contract users must size against:"The step count is CUMULATIVE across every retry attempt of the " "turn (the same EarlyStopWatcher instance, and its counters, " "persist across retries) — including attempts that ultimately " "crashed or timed out"(models/criteria.py:232-236). This is an emergent property ofEarlyStopWatcher.on_eventnever resetting_tool_call_indexon a secondAgentStartEvent(early_stop.py:345-347, which only stamps_started_monotonic). The nearest test,test_second_agent_start_does_not_reset_origin(tests/test_early_stop.py:1634), asserts only that the wall-clock origin survives — it uses nomax_steps_to_decide. Add a test feeding[_agent_start(), _turn_start(), _tool_end(...)], then a second_agent_start()plus one more_tool_end(...), against a criterion withmax_steps_to_decide=2, assertingreason == DECISION_BUDGET_EXCEEDEDandtool_call_index == 2— otherwise a future per-attempt reset silently changes scoring with a green suite. - [Axis 3] New EarlyStopReason.DECISION_BUDGET_EXCEEDED value string is asserted nowhere, including the report/telemetry surfaces (
tests/test_early_stop.py:1036) —test_reason_valuesstill asserts only the two pre-existing members:assert EarlyStopReason.CRITERION_PASSED.value == "criterion_passed"/assert EarlyStopReason.CRITERION_FAILED.value == "criterion_failed"(tests/test_early_stop.py:1037-1038). The new member's wire string"decision_budget_exceeded"is a persistedtask.jsonvalue (early_stop_reason) and an App Insights telemetry dim (EarlyStopReason), butTestEarlyStopReportSurfacesexercises onlycriterion_passed—test_task_dict_keys_present_when_early_stopped(line 1962) andtest_telemetry_dims_reflect_early_stop(line 2002) both hardcode"criterion_passed". Addassert EarlyStopReason.DECISION_BUDGET_EXCEEDED.value == "decision_budget_exceeded"totest_reason_values, and one report/telemetry surface assertion for a budget-exceeded result (which would also surface that the runtime note text "gated on armed criteria only; other criteria are advisory", tests/test_early_stop.py:1976, is inaccurate for this reason). - [Axis 5] CLAUDE.md's "run_limits is the single namespace for all run-time caps" now contradicts the adjacent bullet's
max_steps_to_decide, and the new cap has no dedicated FinalStatus like its siblings (CLAUDE.md:144) — FAILURE SCENARIO: a contributor adding the next run-time cap reads CLAUDE.md:144, follows the stated invariant, and puts a per-criterion cap onRunLimits— where it cannot be attributed to a criterion — instead of following themax_steps_to_decideprecedent one line below. Separately, a nightly consumer bucketing runs byfinal_statuscannot tell a decision-budget abort from a genuine criteria failure without also parsingearly_stop.reason, unlike the three sibling cap breaches which are self-describing infinal_statusalone.
ROOT CAUSE: CLAUDE.md:144 was left verbatim by this PR (only line 145 changed, the +1/-1 edit):
TaskDefinition.run_limits(RunLimitsmodel) is the single namespace for all run-time caps —max_turns/task_timeout/turn_timeout(structural) andmax_input_tokens/ ... /max_usd(cumulative budget).
while the very next line now introduces "A per-criterion max_steps_to_decide (on LiveSuccessCriterion only, requires stop_when) caps tool-call steps ... before EarlyStopReason.DECISION_BUDGET_EXCEEDED force-fails the run outright" — a run-time cap that terminates the run, living outside that namespace. The two adjacent bullets contradict each other.
A second, related asymmetry: every sibling cap's breach gets its own terminal status with an explicit category and icon — MAX_TURNS_EXHAUSTED, TOKEN_BUDGET_EXCEEDED, COST_BUDGET_EXCEEDED (models/enums.py:15-17, :43-45, :57-59). max_steps_to_decide's breach instead collapses into generic FinalStatus.FAILURE at orchestrator.py:1579-1583 (all_passed = False), so on final_status alone a budget-exhausted run is indistinguishable from a run whose criteria genuinely failed. This is mitigated (reports.py:437-438 and reports_html.py:350 do surface early_stop.reason), which is why it is Low rather than higher.
FIX: amend CLAUDE.md:144 to carve out the per-criterion exception explicitly (e.g. "...the single namespace for all task-level run-time caps; the one per-criterion cap, max_steps_to_decide, lives on LiveSuccessCriterion because the watcher must attribute the breach to a specific criterion"), and consider whether DECISION_BUDGET_EXCEEDED warrants its own FinalStatus member for parity with the other cap breaches.
9. [Axis 6] Fail-open disarm covers only checker.live_verdict; the new bound arithmetic and _fire sit outside it, so a raise there degrades via a generic "Stream callback failed (ignored)" warning and re-raises every event (src/coder_eval/orchestration/early_stop.py:442) — Only one statement in _evaluate is protected:
try:
verdicts.append(checker.live_verdict(criterion, records))
except Exception:
# Fail-open: a raising verdict disarms; the run degrades to full.
self._disarmed = Truerecord = self._collector.build_turn_record() (line 427), the sorted(...) re-order (line 433), _ceiling/_floor (lines 485, 505) and _fire (line 534) are all outside it. I confirmed the obvious ZeroDivisionError in _ceiling (/ self._armed_weight, line 406) is NOT reachable — BaseSuccessCriterion.check_weight_zero_is_not_gating (models/criteria.py:155) rejects weight: 0 together with stop_when, and the candidate is not None and self._ceiling(...) short-circuit at line 485 means an empty armed set never divides — so this is robustness, not a live bug. But if anything there does raise, the exception escapes into CompositeStreamCallback/safe_emit (streaming/callbacks.py:56), which logs only logger.warning("Stream callback failed (ignored)") with no task or criterion context and, crucially, leaves self._disarmed = False — so the watcher keeps re-entering _evaluate and re-raising on every subsequent event, and nothing records that the smoke flavor silently became a full run. Move the try to wrap the whole _evaluate body (keeping the existing per-criterion criterion.type detail in the log) so the documented disarm-and-log-loudly path covers the new arithmetic too. Separately, the disarmed property (line 375) has zero consumers in src/ — a degraded run is invisible in EvaluationResult/reports.
10. [Axis 7] Guide's constraint column for stop_early_gate_threshold advertises a range that is never accepted, and omits the stop_early co-requirement (docs/TASK_DEFINITION_GUIDE.md:263) — The row reads | stop_early_gate_threshold|1.0|[0.0, 1.0] | Minimum weighted score over the armed subset required to gate as a pass. ... |. The field bounds are ge=0.0, le=1.0 (models/limits.py:109-110), but the validator makes 0.0 unreachable in BOTH states: with stop_early: True it hits models/limits.py:150 ("must be > 0.0 when stop_early is True"), and with stop_early: False it hits :145 (non-default with stop_early off). Verified: RunLimits(stop_early=True, stop_early_gate_threshold=0.0) and RunLimits(stop_early_gate_threshold=0.0) both raise. The effective range is (0.0, 1.0]. The Constraint column also never states the hard co-requirement that a non-default value requires stop_early: true, and neither does the Weighting bullet at lines 394-418. Change the column to (0.0, 1.0] and add "; requires stop_early: true" (or, if finding #1 is fixed by making the field inert-when-off, just fix the range).
What's Missing
Parallel paths:
- 🟠 The shipped A/B recipe was not updated for the new knobs:
experiments/early-stop-ab.yaml:20(e2evariant,stop_early: false) now hard-fails resolution against 2 of the 3 new fixture tasks (early_stop_weighted_low_weight_absorbed.yaml:34,early_stop_weighted_high_weight_kills_run.yaml:31both setstop_early_gate_threshold: 0.7), anddocs/AB_EXPERIMENTS.md:271-302still teaches that exact flip-one-boolean recipe with no caveat — so the documented A/B pattern and the PR's own example tasks are mutually incompatible. (trigger: tasks/early_stop_weighted_low_weight_absorbed.yaml) (restates: Axis 6:stop_early_gate_thresholdmodel validator rejects partial merge layers) - 🟡
validate_early_stop(src/coder_eval/orchestration/early_stop.py:150-231) is the single resolution-time gate bothplanandruncall, and it gained no case for either new knob — it still validates only observability and per-instance polarity decidability, nevermax_steps_to_decideagainst the armed polarity set and neverstop_early_gate_threshold(whose check was instead put on theRunLimitsmodel, where it cannot see the merge layer that produced the value). (trigger: src/coder_eval/orchestration/early_stop.py) (restates: Axis 6:max_steps_to_decideis polarity-blind) - 🟡 The two new knobs take opposite fail-loud decisions for the identical "inert without
stop_early" condition:stop_early_gate_thresholdhard-raises (src/coder_eval/models/limits.py:145), whilemax_steps_to_decide+stop_whenwithstop_early: falseis silently accepted and never fires — even though the field's own description (src/coder_eval/models/criteria.py:230) says "Requires run_limits.stop_early and this criterion's own stop_when" and its validator (:249) enforces only the second half. Pick one policy for both. (trigger: src/coder_eval/models/criteria.py)
Tests:
- 🟠 No test exercises
max_steps_to_decideon a fail-armed criterion — every budget test (tests/test_early_stop.py:1450-1507,:1827) usesstop_when="pass", which is exactly why the polarity-blind budget check shipped green. (trigger: src/coder_eval/orchestration/early_stop.py) (restates: Axis 6:max_steps_to_decideis polarity-blind) - 🟠 The headline user-facing hop — YAML
stop_early_gate_threshold-> final gate (src/coder_eval/orchestrator.py:1589-1590) and -> the persistedEarlyStopInfo.gate_threshold(src/coder_eval/orchestration/early_stop.py:548) — has no assertion; both sites can be replaced with a literal1.0and the suite stays green.TestOrchestratorEarlyStopWiring._run_wiring(tests/test_early_stop.py:1689) has no threshold parameter andgrep '\.gate_threshold' tests/returns zero hits. (trigger: src/coder_eval/orchestrator.py) (restates: Axis 3: The new weighted early-stop gate is undertested) - 🟠 No resolution/merge test for the layered shape the new
RunLimitsvalidator breaks — task setsstop_early_gate_thresholdx variant/-Dsetsstop_early: false.tests/test_early_stop.py:295covers only the direct model construction, andTestNewFixtureTasksResolve(:951-971) resolves each fixture without an experiment, so the shippedearly-stop-abe2evariant is never exercised against them. (trigger: src/coder_eval/models/limits.py) (restates: Axis 6:stop_early_gate_thresholdmodel validator rejects partial merge layers) - 🟠 The rewritten CE025 ships with no negative fixture:
tests/test_custom_lint.py:563asserts onlynot violationsagainst the already-clean tree, so neitherviolations.appendbranch (:574-583) is ever driven — unlike every sibling whole-tree rule (CE027/CE030/CE031), each of which has a synthetic test proving it can fail. Nine deleted detection tests went withtests/lint/rules/ce025_live_verdict_consistency.py. (trigger: tests/test_custom_lint.py) (restates: Axis 3: CE025's rewrite drops all of its detection tests) - 🟡 The three new example tasks get resolution-only coverage —
TestNewFixtureTasksResolve(tests/test_early_stop.py:951-971) asserts nothing beyondlimits.stop_early is True, not thestop_early_gate_threshold: 0.7/max_steps_to_decidevalues and not the stop behavior each file's header comment claims. Replaying each fixture's criteria through the_watcherhelper would have caught the high-weight fixture's "fail-stops immediately" claim, which the deferral rule contradicts. (trigger: tasks/early_stop_weighted_high_weight_kills_run.yaml) (restates: Axis 1: Example taskearly_stop_weighted_high_weight_kills_run.yamldescribes an immediate fail-stop) - 🟡 The new
EarlyStopReason.DECISION_BUDGET_EXCEEDEDwire string is asserted nowhere:test_reason_values(tests/test_early_stop.py:1036-1038) still lists only the two pre-existing members, and both report/telemetry surface tests (:1962,:2002) hardcode"criterion_passed", so the persistedtask.jsonvalue and the App Insights dim are untested for the new member. (trigger: src/coder_eval/models/results.py) (restates: Axis 3: NewEarlyStopReason.DECISION_BUDGET_EXCEEDEDvalue string is asserted nowhere) - 🔵
docs/EXTENDING.md:213-221now instructs third-party criterion authors to subclass the new publicLiveSuccessCriterionABC (newly exported fromcoder_eval.modelsalongsideLivePolarity), but no test walks a plugin-shaped criterion through that path end-to-end (subclass ->live_decidable_polarities->validate_early_stoparming -> watcher verdict); all coverage is against the two in-tree types. (trigger: docs/EXTENDING.md)
Downstream consumers:
- 🟡 The new
EarlyStopInfo.gate_thresholdis written intotask.jsonbut was not added to the flattened run.json task row —src/coder_eval/reports_experiment.py:208-211still emits onlystopped_early/early_stop_reason/turns_remaining_at_stop, so the external eval-runner / evalboard sees an early-stopped pass or fail with no way to reconstruct which threshold decided it. (trigger: src/coder_eval/models/results.py) - 🟡 Every score consumer still reads
weighted_score, computed over the FULL criteria set (src/coder_eval/orchestrator.py:1603), while an early-stopped run'sfinal_statusnow comes from a different weighted formula over the armed subset with a different denominator — and nothing persists that armed score. Soreports.py:326/386,reports_stats.py:327,reports_junit.py:265and theScoretelemetry dim (orchestrator.py:258) can visibly contradict the pass/fail (extreme case:decision_budget_exceededforce-fails a run whoseweighted_scoreis 1.0). (trigger: src/coder_eval/models/results.py)
Display & mapping dicts:
- 🟡 All three early-stop render strings hardcode the pre-PR semantics and are wrong for the new reason:
reports.py:441-442("gated on armed criteria only; other criteria are advisory"), the HTML badge titlereports_html.py:348-350(same wording), and the per-criterion labelreports_html.py:583("advisory — not gated (run stopped early)"). On adecision_budget_exceededrun NO criterion gated —orchestrator.py:1576-1584force-fails outright — so the report tells the reader the opposite of what happened. (trigger: src/coder_eval/models/results.py) (restates: Axis 3: NewEarlyStopReason.DECISION_BUDGET_EXCEEDEDvalue string is asserted nowhere) - 🟡 No rendering surface shows
gate_threshold, so a run forgiven by a sub-1.0 gate is unexplainable from its own report: the markdown note (reports.py:437-443), the HTML badge (reports_html.py:346-351) and the JUnit property list (reports_junit.py:263-269) all render a criterion row scored 0.0 next to an overall SUCCESS with nothing stating the threshold that absorbed it. (trigger: src/coder_eval/models/results.py) - 🔵
decision_budget_exceededcollapses into genericFinalStatus.FAILURE, so it gets no dedicated icon/label/category in the status dicts that every sibling run-limit breach has (MAX_TURNS_EXHAUSTED/TOKEN_BUDGET_EXCEEDED/COST_BUDGET_EXCEEDEDinmodels/enums.py); bucketing byfinal_statusalone cannot separate a budget abort from a genuine criteria failure. (trigger: src/coder_eval/orchestrator.py) (restates: Axis 5: CLAUDE.md's "run_limits is the single namespace for all run-time caps" now contradictsmax_steps_to_decide)
Daily/nightly:
- 🟠 The cross-repo contract doc was not touched:
docs/REPORT_SCHEMA.md:188-194still enumeratesreasonas (criterion_passed/criterion_failed) and omitsgate_threshold, both of which now ship intask.json— the externalcoder-eval-uipath/ eval-runner reads this doc, and nothing inmake lintguards it. (trigger: src/coder_eval/models/results.py) (restates: Axis 7: docs/REPORT_SCHEMA.md's EarlyStopInfo section still enumerates the pre-PR contract) - 🟠 Nightly blast radius is unstated for the
RunLimitsvalidator regression, and its failure mode is silent rather than loud: on therunpath the resolution error is caught atorchestration/experiment.py:706and the whole task file (both variants, committed as a unit) lands inskippedwith alogger.warning, so a nightly running theearly-stop-abe2evariant loses task coverage without failing; onplanit prints red and still exits 0. (trigger: src/coder_eval/models/limits.py) (restates: Axis 6:stop_early_gate_thresholdmodel validator rejects partial merge layers) - 🟡 A third value now flows onto the
EarlyStopReasonApp Insights dimension (src/coder_eval/orchestrator.py:264) and onto the run.jsonearly_stop_reasonfield, and the PR does not state whether the out-of-repo dashboards-as-code / evalboard bucketing coversdecision_budget_exceededor bins it as unknown —reports.py:438'sor "unknown"fallback only tolerates it in-repo. (trigger: src/coder_eval/models/results.py)
Harness & Lint Improvements
Static checks (lint / type):
- [ce-lint] CE026 — bounded ratio fields. New AST rule
tests/lint/rules/ce026_bounded_ratio_fields.py+ wiring intests/lint/runner.py: any Pydantic field insrc/coder_eval/models/whose name ends in_threshold/_ratio/_fractionand is annotatedfloat(orfloat | None) must declare BOTH a lower (ge=/gt=) and an upper (le=/lt=) bound in itsField(...), or carry# noqa: CE026with a reason. Measured baseline on PR head: every*_thresholdfield insrc/is already bounded except one — the rule ships at zero pre-existing violations. Prevents: A2-low:EarlyStopInfo.gate_threshold(src/coder_eval/models/results.py:475) drops thege=0.0, le=1.0bounds carried by its sourceRunLimits.stop_early_gate_threshold(models/limits.py:107), so a hand-edited or externally producedtask.jsonwithgate_threshold: 7.5round-trips cleanly. - [ce-lint] CE033 — persisted enum wire values must be tested and documented. New whole-tree check
tests/lint/enum_wire_parity.py, wired astests/test_custom_lint.py::TestCE033EnumWireParity(doc-surface style like CE027/CE030, not aBaseRule). For an explicit registry of enums serialized into the run record —{FinalStatus, EarlyStopReason}— every member's.valuestring must appear (a) as a string literal intests/and (b) verbatim indocs/REPORT_SCHEMA.md. Measured on PR head: exactly one violation across both clauses —decision_budget_exceeded.AgentState/ApiBackendare deliberately out of the registry (not part of the task.json contract; they would add 4-6 noise hits). Prevents: A7-medium (REPORT_SCHEMA.md:191-194 still enumerates onlycriterion_passed/criterion_failed, so the external eval-runner/evalboard buckets the new value as unknown) and A3-low (test_reason_values, tests/test_early_stop.py:1036, never asserts the new member's wire string and no report/telemetry surface test covers it). - [ce-lint] Extend CE030's registry to the run-record contract models. In
tests/lint/doc_schema_parity.py, add(EarlyStopInfo, "docs/REPORT_SCHEMA.md")(and, if cheap,EvaluationResult) toDOCUMENTED_MODELS, letting the existingEXEMPTmap absorb framework-only fields. The mechanism (field name must appear as inline code in the owning doc page) already exists and is already wired intomake lint; this is a two-line registry edit that extends the standing doc obligation from the authoring contract (TASK_DEFINITION_GUIDE.md) to the consumer contract (REPORT_SCHEMA.md) — which today has zero references fromtests/,src/, or theMakefile, i.e. nothing ties it to the code at all. Prevents: A7-medium: the newEarlyStopInfo.gate_thresholdfield ships intotask.jsonwhiledocs/REPORT_SCHEMA.mdstill lists the pre-PR field set, silently drifting the cross-repo contract consumed by the separate coder-eval-uipath / eval-runner pipeline. - [ce-lint] CE034 — every CEnnn check must have a positive-detection test. Meta-rule over
tests/test_custom_lint.py(AST, wired as its ownTestCE034LintRulesHaveDetectionTestsclass): eachTestCE*class must contain at least one assertion that a synthetic input PRODUCES a violation — anassertwhose expression is not aUnaryOp(Not)over the checker result and is not the clean-treeassert not violations. The file's own conventions make this mechanical: AST rules useassert self._run(...), doc-surface rules shiptest_detects_*/test_catches_*fixtures. Measured:TestCE025LiveVerdictConsistency(tests/test_custom_lint.py:534-585) is the outlier — its only test assertsnot violationsagainst the already-clean tree. Prevents: A3-medium: CE025's rewrite deleted all 9 detection tests along withtests/lint/rules/ce025_live_verdict_consistency.py, so neitherviolations.append(...)branch of the replacement is ever exercised — an inverted condition or a brokenchecker_cls.live_verdict is not BaseCriterion.live_verdictdiscriminator would ship green while CLAUDE.md and docs/EXTENDING.md keep advertising the invariant as enforced. - [ce-lint] CE035 — no migration/relocation prose in
src/comments. Line/AST rule: comment lines undersrc/coder_eval/matchingnow lives|used to be|formerly|renamed from|(has|was) moved|moved (to|elsewhere)|previously (called|named|lived)are rejected. On a greenfield project with no backward-compatibility surface (CLAUDE.md: "Clean Code: No dead code") a where-it-used-to-live note has no reader and rots silently. Measured baseline: 3 hits tree-wide — 2 are the finding, and the third (models/criteria.py:187) is itself already stale (it points atevaluator.py, a file that no longer exists), so the rule pays for itself on introduction. Prevents: A1-low: the orphaned migration comment blocks atsrc/coder_eval/criteria/command_executed.py:33-39andsrc/coder_eval/criteria/skill_triggered.py:112-119, left behind after thelive_stop_polaritiesClassVar they annotated was deleted. - [ce-lint] CE036 — no widening of a domain
Literalalias. Registry-driven AST rule (one entry per exportedLiteralalias inmodels/, e.g.{"LivePolarity": r"polarit"}): forbid annotating any attribute, parameter, or return whose name matches the alias's domain pattern with a barestrcontainer —set[str],frozenset[str],list[str]. Catchesself._armed_polarities: list[frozenset[str]](src/coder_eval/orchestration/early_stop.py:272) anddef _resolve_armed_polarities(...) -> frozenset[str](:382); both should befrozenset[LivePolarity], which is already imported atearly_stop.py:70. Prevents: A1-low / A2-low (cross-axis):LivePolarity = Literal["pass", "fail"]was introduced precisely so "a stray/typo'd string, orundecideditself, is a pyright error rather than a runtime-only lint gap" (models/criteria.py:190-196), then widened straight back tofrozenset[str]at the watcher's only consumption point, leaving the membership tests at early_stop.py:454 and :482 unchecked. - [pyright] Once CE036's tightening lands, enable
reportUnnecessaryContains = "error"andreportUnnecessaryComparison = "error"in[tool.pyright], scoped via[[tool.pyright.executionEnvironments]]tosrc/coder_eval/models+src/coder_eval/orchestrationif the whole-tree baseline is noisy. With polarity sets and verdicts typed as closedLiterals, a typo'd or out-of-domain literal in"pass" in self._armed_polarities[i]/verdict == "undecided"becomes a type error rather than a silently-always-false branch. Measure the baseline before flipping — I could not run pyright with these flags in this sandbox, so the whole-tree scope is unverified. Prevents: A1-low / A2-low: closes theLivePolaritywidening class at the use sites rather than only the declaration sites — which is the exact failure mode the alias was added to prevent. - [ce-lint] CE037 — CLI validation handlers must set the validity flag or re-raise. AST rule scoped to
src/coder_eval/cli/: inside any function that owns a validity flag (detected by an initialall_valid = True-style assignment plus a terminalraise typer.Exit(1)), everyExceptHandlermust either assign that flag orraise. Printing a red error and falling through is the forbidden shape. Measured: one violation on PR head —src/coder_eval/cli/plan_command.py:148(except Exception as e:prints "Variant '…': resolution failed" and leavesall_validuntouched), sitting directly beside theexcept EarlyStopConfigErrorhandler at:143that exists specifically to flip it. Prevents: A6-high (exit-code half):coder-eval plan tasks/early_stop_weighted_low_weight_absorbed.yaml --experiment experiments/early-stop-ab.yamlprints a resolution failure, then "All tasks are valid!", and exits 0 — so a CI plan gate cannot see the broken variant. - [ruff] Add
"C901"to[tool.ruff.lint].selectwith[tool.ruff.lint.mccabe] max-complexity = 15, following the policy already written intopyproject.toml:187-192forPLR0915/PLR0912("gates NEW growth past these bounds (a god-function must carry a visible# noqadebt marker). Existing offenders are tracked, not auto-decomposed"). The 18 functions currently at radon D+ take a# noqa: C901debt marker in the same change. Deliberate known limitation: a marked function can keep growing — see the radon ratchet in the harness bucket for the complement. Prevents: A1-medium:EarlyStopWatcher._evaluatewas already C(17) at the merge base and would have needed a visible debt marker before this PR pushed it to D(22) / 107 lines with three inlined decision rules — turning a silent slide into an explicit, reviewable decision (in practice, the_fail_stop_candidate/_pass_stop_deciding/_budget_exceededextraction).
Harness improvements (not statically reachable):
- Cartesian resolution gate. Add a test (and a
make verifystep) that resolves EVERYtasks/*.yamlagainst EVERY variant of EVERYexperiments/*.yamlthroughresolve_task_for_variant+validate_early_stop, asserting zero failures. Reproduced on PR head:experiments/early-stop-ab.yaml'se2evariant (stop_early: false) ×tasks/early_stop_weighted_low_weight_absorbed.yaml:34andtasks/early_stop_weighted_high_weight_kills_run.yaml:31(bothstop_early_gate_threshold: 0.7) both fail — 2 of the 3 new task fixtures are broken under a shipped variant. Why not static: No single file is wrong: the task YAML is valid, the experiment YAML is valid, and the validator atmodels/limits.py:145is syntactically fine. The defect exists only in the product of two files, after the 5-layer field-merge runs and pydantic validators fire on the merged object — that requires executing the resolver, which no AST rule can do. Prevents: A6-high (cross-axis 6/7/8):stop_early_gate_threshold's model validator rejects partial merge layers, breaking the shippedearly-stop-abe2e variant and every-D run_limits.stop_early=falseoverride path. - End-to-end exit-code assertion for
coder-eval plan. A CLI test invokingplanover a fixture whose only defect surfaces at variant resolution, asserting exit code 1 AND the absence of the "All tasks are valid!" line. Pair it with the same shape forrun—orchestration/experiment.py:706currently degrades a whole task file (both variants, sincefile_resolvedis committed as a unit) intoskippedwith a barelogger.warning. Why not static: CE037 can prove the handler shape is right; only a real invocation proves the process exit code and the printed verdict agree, since the flag, the success banner, andtyper.Exitare three separate statements a static rule cannot correlate into an observed exit status. Prevents: A6-high (silent-exit-0 half): the plan gate reports success on a task set it just failed to resolve. - Gate-equivalence property test. Assert the docstring's own claim mechanically: over a generated matrix of armed criteria (weight ∈ {0.2, 0.5, 1.0} × score ∈ {0.0, 0.5, 1.0} × pass_threshold ∈ {0.0, 0.5, 0.9}),
armed_criteria_passed(criteria, gate_threshold=1.0)must equalall(r.score >= c.pass_threshold for r, c in armed). It fails today for thepass_threshold: 0.0shape — the only supported way to arm a stop-triggering criterion without gating on it, sincecheck_weight_zero_is_not_gating(models/criteria.py:155) hard-rejectsweight: 0together withstop_when. Why not static: It is a semantic equivalence between two scoring formulas — a weighted mean versus an all-quantified per-criterion comparison. No AST shape distinguishes "these compute the same thing" from "these don't"; the claim is currently pinned only by an English comment (tests/test_early_stop.py:1128-1141) that itself concedes it holds "only because … binary 0.0/1.0 in practice". Prevents: A5-medium (cross-axis 5/7/8):pass_thresholdis silently displaced on the armed gate, so identical agent output yields SUCCESS on a naturally-completed run and FAILURE on an early-stopped one at the default threshold, and the equivalence claim at models/results.py:686-691 is false. - Polarity × stop_when × budget conformance matrix. A table-driven harness that, for every
LiveSuccessCriterionsubclass × everystop_whenvalue (pass/fail/decided/auto) × positive-vs-distractor instance shape, drivesEarlyStopWatcherover (a) a scripted CLEAN event stream, asserting it never fires a run-failing stop, and (b) a scripted VIOLATING stream, asserting the documented reason and step index. Add a retry leg (a secondAgentStartEventmid-trace) assertingtool_call_indexkeeps accumulating. Why not static: The bug is a runtime interaction betweenlive_decidable_polarities()— resolved per instance from field values likemax_count— and the budget loop's polarity-blindverdict == "undecided"test atearly_stop.py:527. Neither the criterion model nor the watcher is statically wrong in isolation; only replaying an event stream through the real watcher exposes it. Prevents: A6-high (cross-axis 5/6/8): a fail-armed criterion whoseundecidedIS the success state is force-failed asdecision_budget_exceeded— reproduced for both live criteria, and it force-fails every negative row of a dataset-fannedautocriterion. Also A3-low (documented cumulative-across-retries step counting is untested; every existing budget test usesstop_when="pass"). - Shipped example-task behavior fixtures. Each
tasks/early_stop_*.yamlgets a test that replays a scripted tool-event trace through the watcher and asserts the outcome its owndescription:advertises (stop reason, deciding criterion, approximate step). TodayTestNewFixtureTasksResolve(tests/test_early_stop.py:951-971) asserts onlylimits.stop_early is True; the prose is entirely unchecked. Why not static: The claim lives in a natural-languagedescription:field; proving it true requires executing the described scenario. A linter can at best check that the YAML parses. Prevents: A1-low / A8-low (cross-axis):tasks/early_stop_weighted_high_weight_kills_run.yaml:6-11promises the watcher "must fail-stop immediately", but the fail-stop is deferred while that task's own pass-armed positive is undecided (early_stop.py:474) — the example contradicts the feature it demonstrates. - Bounded-field boundary constructibility test. For every numeric field on
RunLimits/Dataset/SimulationConfigcarryingge=/le=, assert both advertised endpoints actually construct after allmodel_validators run, failing with a message naming the field and the rejecting validator. Fails today forRunLimits(stop_early=True, stop_early_gate_threshold=0.0)(rejected at limits.py:150) andRunLimits(stop_early_gate_threshold=0.0)(rejected at :145): the effective range is(0.0, 1.0], not the[0.0, 1.0]advertised in the guide's Constraint column. Why not static: The narrowing comes from imperativemodel_validatorbodies, not the field declaration. Static analysis readingField(ge=0.0, le=1.0)sees a range the model does not actually accept; only instantiation reveals the intersection of declared bounds and validator logic. Prevents: A7-low:docs/TASK_DEFINITION_GUIDE.md:263advertises a range forstop_early_gate_thresholdthat is unreachable at both settings ofstop_early, and omits the hardrequires stop_early: trueco-requirement. - Targeted mutation gate on changed
src/files. Add an opt-inmake mutate(mutmut or cosmic-ray, scoped to the diff's changed files, run in CI on PRs touchingorchestrator.py/orchestration/) that fails on surviving mutants in newly added lines. The gap was demonstrated directly: replacingorchestrator.py:1589(gate_threshold = self.task.run_limits.stop_early_gate_threshold …) andearly_stop.py:548(gate_threshold=self._gate_threshold) with the literal1.0leaves the suite fully green. Why not static: Detecting "this line's value never affects any assertion" requires running the test suite against mutated code and comparing outcomes — a property of the tests, not of the source text. The narrower name-level slice IS static (see CE032 below), but only mutation catches a wrong read at a correctly-named site. Prevents: A3-medium: the weighted early-stop gate's two plumbing hops — the orchestrator's final-gate read and the persistedEarlyStopInfo.gate_thresholdaudit write — carry the PR's headline user-facing behavior and are asserted nowhere. - CE032 — persisted-record fields must be asserted in
tests/. Newtests/lint/persisted_field_assertions.py, wired asTestCE032PersistedFieldAssertions: for an explicit registry of persisted models —{EvaluationResult, EarlyStopInfo, TurnRecord, RunLimits}— every field name must appear intests/as an attribute read (\.<field>\b) or a dict key ("<field>"), with anEXEMPTmap carrying reasons. Measured baseline on PR head: exactly 3 hits —EarlyStopInfo.gate_threshold(the finding),RunLimits.count_cache_creation,EvaluationResult.total_assistant_turns; the latter two are fixed or exempted at introduction. Why not static: The mechanism is a whole-tree grep, but what it enforces is not a code-shape invariant — it is "a human wrote an assertion about this field". Its floor is real and worth recording: a mere mention satisfies it, so it catches never-asserted fields, not weakly-asserted ones. That is why the mutation gate above is its complement, not its duplicate. Prevents: A3-medium (b) and A3-low:EarlyStopInfo.gate_thresholdhas zero attribute reads anywhere intests/, andtest_info_defaults(tests/test_early_stop.py:1040) never covers it. - Radon complexity ratchet. Check in
tests/lint/complexity_baseline.json(function → current radon CC) and add amake lintstep that fails when any function exceeds its recorded value, with the baseline only ever ratcheting down. Complements theruff C901proposal rather than duplicating it. Why not static: A single absolute-threshold ruff rule cannot express "no worse than before": once_evaluatecarries# noqa: C901it can grow unboundedly. The ratchet needs committed prior state plus a diff against it — a tool + artifact in the harness, not a rule over one file's AST. Prevents: A1-medium:EarlyStopWatcher._evaluateslid from C(17) to D(22) inside a tree that already tolerates 18 D+/E/F functions, so no absolute threshold anyone would realistically adopt today would have flagged the regression.
Top 5 Priority Actions
- Fix the polarity-blind decision budget at src/coder_eval/orchestration/early_stop.py:527 (skip criteria whose resolved arming is fail-only, or reject
max_steps_to_decideon fail-only instances invalidate_early_stop) — today a distractor that correctly never fires is force-failed asdecision_budget_exceededand hard-coded to FAILURE at src/coder_eval/orchestrator.py:1580, which on anauto-armed fanned criterion breaks every negative row. - Restore per-criterion
pass_thresholdinside the weighted armed gate at src/coder_eval/models/results.py:717-718 (score each armed criterion as1.0 if r.score >= c.pass_threshold else 0.0before weighting) or correct the false equivalence claim at src/coder_eval/models/results.py:686-691 — at the default threshold an armed criterion withpass_threshold: 0.0now flips SUCCESS to FAILURE on identical agent output, and disagrees withall_criteria_passedon the same result. - Relax the
stop_early_gate_thresholdvalidator at src/coder_eval/models/limits.py:145 (drop thestop_early is Falsehalf, or move it intovalidate_early_stopso it raisesEarlyStopConfigError) and makecli/plan_command.py:148setall_valid = False— the shippedexperiments/early-stop-ab.yaml:20e2evariant currently fails resolution for two of the three new task files whileplanstill exits 0 andrunsilently skips both variants. - Resolve the weighted-gate semantic split so one task config maps to one gate semantic — a
stop_early: truerun that completes naturally falls through to the strict gate at src/coder_eval/orchestrator.py:1600, so the low-weight failure the ceiling just absorbed re-fails the run (and the documentedmax_steps_to_decideworkaround at docs/TASK_DEFINITION_GUIDE.md:416-418 hard-fails instead of applying the weighted gate). - Close the coverage gaps that let all of the above ship green: add an orchestrator wiring test for a non-default
stop_early_gate_threshold(src/coder_eval/orchestrator.py:1589) and an assertion on the persistedEarlyStopInfo.gate_threshold(src/coder_eval/orchestration/early_stop.py:548) — both survive simultaneous mutation to1.0— plus fail-armedmax_steps_to_decideand synthetic-violation tests for the rewritten CE025 check (tests/test_custom_lint.py:563), whose only test asserts the already-clean tree.
Stats: 0 🔴 · 2 🟠 · 6 🟡 · 10 🔵 across 8 axes reviewed.
uipreliga
left a comment
There was a problem hiding this comment.
Fix what you agree with and 🚢
…shold displacement, gate-semantic split Fixes three scoring-correctness bugs found in review: - max_steps_to_decide was polarity-blind: a fail-only-decidable criterion (a distractor) whose "undecided" IS its success state got force-failed as decision_budget_exceeded. Now rejected at resolution time unless the instance can live-pass. - armed_criteria_passed's weighted formula silently displaced each criterion's own pass_threshold, so a criterion armed with pass_threshold: 0 (the only way to arm a non-gating criterion) could flip SUCCESS to FAILURE at the *default* gate_threshold=1.0. Each criterion's score is now converted to binary via its own pass_threshold before weighting, making the gate_threshold=1.0 default an exact equivalence with the pre-weighting all(...) rule, not an approximation. - A stop_early: true task that completed naturally (watcher never fired) fell back to the strict full-set gate, silently re-failing on a low-weight criterion the weighted gate was configured to forgive. The armed weighted gate now applies whenever stop_early is armed, regardless of whether the watcher physically fired — one task config, one gate semantic. Also: relaxed the stop_early_gate_threshold RunLimits validator (it broke the shipped early-stop-ab e2e variant and any -D stop_early=false override via field-merge); moved the degenerate-threshold (<=0.0) check into validate_early_stop so it gets the hard-stop CLI treatment; tightened LivePolarity typing at its remaining frozenset[str] consumption points; added CE025 synthetic-violation tests; wrapped EarlyStopWatcher's full evaluation round in fail-open handling (not just live_verdict); updated CLAUDE.md/REPORT_SCHEMA.md/TASK_DEFINITION_GUIDE.md for all of the above; and backfilled test coverage for every fix (fail-armed budget rejection, gate_threshold plumbing survives mutation, retry-accumulation, layered merge resolution, report/telemetry surfaces for the new reason value). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pip-audit flagged newly-published CVEs (aiohttp request-smuggling/DoS, cryptography Bleichenbacher oracle) that started failing the PR Quality Gate on the latest push, independent of the early-stop changes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks @uipreliga for the thorough review — caught two real scoring bugs I'd missed. Addressed:
Deliberately skipped (tracked as follow-up, not defects): the proposed new CE-lint rules, a radon complexity ratchet, and the full
|
Summary
Addresses GitHub issue #61 (the two items scoped for implementation, plus a decision-step budget requested mid-review):
run_limits.stop_early_gate_threshold(default1.0, byte-for-byte backward compatible) turns the armed pass/fail gate and the live stop trigger into weighted ceiling/floor bounds instead of a strict boolean — a low-weight armed criterion's failure can be absorbed without truncating the run, while a high-weight failure still fail-stops immediately once the gate is mathematically guaranteed unreachable.live_verdictcontract. Explicit determinism/monotonicity contract onLiveVerdict/live_verdict.max_steps_to_decide(per armed criterion) force-fails a run if the criterion never reaches a verdict within its budget, bypassing the weighted gate (nothing to weigh a criterion that never decided).Also refactors "is this criterion live-observable" from a checker-side
ClassVarintoLiveSuccessCriterionmodel subclassing — the single source of truthvalidate_early_stop/EarlyStopWatchernow consult directly, replacing the old paired-ClassVar/CE025 mechanism.A full 8-axis code review (
/coder-eval-code-review-full) ran against this diff before merge; findings addressed here:stop_early_gate_threshold: 0.0gaming gap that could trivially neutralize the armed gate (newRunLimitsvalidator).armed_criteria_passed's defensive zero-weight fallback now fails closed instead of open.CommandExecutedCriterioncoverage alongsideSkillTriggeredCriterionfor all new weight/budget mechanics.smoke/smoke-fail(their pass/fail outcome depends on non-deterministic live-agent behavior, unlike the existing deterministic smoke-fail fixtures) — added cheap resolution-only tests instead.CLAUDE.md/docs/EXTENDING.mdfor theLiveSuccessCriterionrefactor; documented the cross-retry decision-budget accumulation and the natural-completion/weighted-gate interaction indocs/TASK_DEFINITION_GUIDE.md.EarlyStopReasondispatch (assert_never) in the orchestrator finalize step; extracted a_floorhelper paralleling_ceiling.Test plan
make verify— 3808 passed, 91.29% coverage, ruff/pyright/lint cleancoder-eval planagainst the 3 new example task YAMLs — all resolve cleanlySkillTriggeredCriterion/CommandExecutedCriterion), ceiling/floor bound triggers, decision-step budget (single + multi-criterion attribution),RunLimitsvalidator edge cases,armed_criteria_passedpass_threshold-is-ignored regression testCo-Authored-By: Claude Sonnet 5 noreply@anthropic.com