Repository navigation
fix: recover grading failures without replaying agents - #248
Conversation
osv-scanner fails the Quality Gate on pyasn1 0.6.4 (GHSA-2qm8-x737-gmw5, GHSA-9m75-vc3g-4x39, GHSA-fm9c-5rcf-2q5p, GHSA-mm57-4v3p-rchx), on main as well. 0.6.5 fixes all four. Lockfile only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit ffa1713)
|
Claude finished @cosminacho's task in 1m 11s —— View job Code Review in Progress
|
uipreliga
left a comment
There was a problem hiding this comment.
Review: fix: recover grading failures without replaying agents (#248)
PR #248 by @cosminacho · codex/grading-error-recovery → main · OPEN · reviewed against 1ab2acd
Scope: pr:248 · branch codex/grading-error-recovery (PR #248 head; local ref pr-248) · 1ab2acd · 2026-11-05T11:35Z · workflow variant
Change class: complex — changes grading/resume control flow, FinalStatus preservation and the persisted task.json result schema
PR 248 has a sound goal and its type, security and error-handling quality is good (9.4 to 9.9), but it must not merge as is: a grading crash is now repairable before the execution-phase work (budget checks, remaining dialog turns) has finished, so --resume or evaluate can turn a TOKEN_BUDGET_EXCEEDED row into SUCCESS for the same agent output, and a failed regrade of a row with an execution failure (a fact --resume must not overwrite) writes an unmeasured weighted_score of 0.0 that --resume never repairs.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 6.8 / 10 | 0 | 2 | 2 | 2 | Grading-origin ERROR predicate inlined at ~7 sites with no named property; overlapping public execution-fact predicates |
| 2. Type Safety | 9.4 / 10 | 0 | 0 | 1 | 1 | New persisted public model GradingErrorInfo adds a dict[str, Any] escape hatch, although a typed ErrorContext already exists |
| 3. Test Health | 6.3 / 10 | 0 | 2 | 3 | 2 | Grading failure under evaluate --format harbor writes task.json into the user's WORK_DIR (restore_pre_grade_record lost its run-dir guard) |
| 4. Security | 9.9 / 10 | 0 | 0 | 0 | 1 | New CLI error prints put grading_error.message into Rich markup without escape(); a message containing '[/...' raises MarkupError before the prior verdict is restored |
| 5. Architecture & Design | 9.4 / 10 | 0 | 0 | 1 | 1 | One grading incident is stored twice (error_message/error_details/error_log_tail and grading_error), and the two copies do not agree |
| 6. Error Handling & Resilience | 9.9 / 10 | 0 | 0 | 0 | 1 | Failed-regrade paths in run_command ignore the restore result and print inaccurate operator messages |
| 7. API Surface & Maintainability | 9.8 / 10 | 0 | 0 | 0 | 2 | New public persisted model GradingErrorInfo has no field descriptions |
| 8. Evaluation Harness Quality | 5.5 / 10 | 1 | 1 | 1 | 0 | Making grading ERROR repairable skips execution-phase work the crash cut short, so --resume/evaluate can turn a budget stop or an unfinished dialog into SUCCESS |
Overall Score: 8.4 / 10 · Weakest Axis: Evaluation Harness Quality at 5.5 / 10
Totals: 🔴 1 · 🟠 5 · 🟡 8 · 🔵 10 across 8 axes.
Blockers
- [Axis 1] Grading-origin ERROR predicate inlined at ~7 sites with no named property; overlapping public execution-fact predicates (
src/coder_eval/orchestrator.py:682) — The same two-term testX.final_status is FinalStatus.ERROR and X.error_phase == "grading"is written by hand at 7 places: orchestrator.py:682 (elif prior.final_status is FinalStatus.ERROR and prior.error_phase == "grading":), orchestrator.py:691 (the same test on the nextif, which repeats theelifabove it), models/results.py:731 (insidehas_execution_failure), orchestration/batch.py:414 (if result.final_status is FinalStatus.ERROR and result.error_phase == "grading":), cli/run_command.py:913 and :920, and cli/evaluate_command.py:527. This matches the Axis-1 High anchor (logic duplicated across 4+ sites). It is also the classifier that decides whether a row can be repaired, so if one copy drifts, one command treats a row as recoverable and another treats it as an execution fact. Fix: add oneEvaluationResult.is_grading_errorproperty. Makehas_execution_failurereturnself.final_status.is_execution_fact and not self.is_grading_error, and call the property at all six other sites. In_seed_from_prior_result, compute it once into a local variable rather than evaluating it twice in a row (lines 682 and 691). - [Axis 1] Unreachable grading-phase branches in the evaluate CLI and the legacy/backup-read restore paths (
src/coder_eval/cli/evaluate_command.py:541) — Members of this finding: (1) evaluate_command.py:541-543,if result.error_phase == "grading": restore_pre_grade_record(...); console.print("[yellow]⚠[/] Grading errored; the prior verdict is preserved."). This code runs only after line 524 (if result.grading_error is not None: ... raise typer.Exit(1)) has not matched. The orchestrator never writeserror_phase="grading"without also settinggrading_error(orchestrator.py:934-941 and :950), so this branch cannot run. Its comment ("Restore explicit grader faults only...") makes it look like a required path. (2) run_command.py:919-921, the clauseor (result.final_status is FinalStatus.ERROR and result.error_phase == "grading"), is redundant for the same reason. (3) regrade.py:590/594/596-597, theprior_result is Nonepath inrestore_pre_grade_record, which readstask.execute.json(backup.read_text(...), thenEvaluationResult.model_validate_json(text)). All three production callers (run_command.py:926, evaluate_command.py:532 and :542) pass a non-Noneprior_result. Only tests reach this path (test_detached_grading_boundaries.py:456/467/476, test_regrade.py:1260). A reader still sees a function namedrestore_pre_grade_recordwith anOptionalprior_resultand concludes that crash recovery falls back to the pre-grade backup. That is now false. This is close to the Axis-1 Critical anchor (dead code on a live path that misleads readers). I filed it as High because the branches cannot change behavior today. Fix: delete (1) and (2). Makeprior_resultandgrading_resultrequired inrestore_pre_grade_record, delete the backup-read branch, and rename the function (for examplerestore_prior_result) so the name matches its new contract. Then update or delete the tests that pin the removed path. - [Axis 3] Grading failure under
evaluate --format harborwrites task.json into the user's WORK_DIR (restore_pre_grade_record lost its run-dir guard) (src/coder_eval/cli/evaluate_command.py:532) — Line 532 isrestore_pre_grade_record(target.target, prior_result=prior, grading_result=result)and line 542 is the same call. Neither has anEvaluateMode.RUN_DIRguard. The success path at line 595 has one (if prior is not None and target.mode is EvaluateMode.RUN_DIR:), and its comment at lines 591-593 says the harbor synthetic prior 'is not a run directory'. On main,restore_pre_grade_record(target.target)returned False when notask.execute.jsonbackup existed. With this PR, regrade.py:590 skips the backup check wheneverprior_resultis passed (prior_result is None and (...)), so the function now writes. I confirmed this with a probe in a scratch copy: I patchedSuccessChecker.check_all_asyncto raise, then ranevaluate task.yaml workdir --format harbor --trajectory t.json. It exited 1, printed 'Grading errored; prior verdict restored with grading provenance.', and leftWORKDIR FILES ['task.json', 'test.txt']. The planted task.json makes the nextevaluateof that directory try the RUN_DIR detection (evaluate_command.py:141-153), and the graded workspace now holds a file the agent did not write. No test exercises this branch: coverage at PR HEAD shows526->534and528->530partial, and tests/test_evaluate_format_harbor.py has no grading-failure case. Fix: guard both restore calls (532, 542) withtarget.mode is EvaluateMode.RUN_DIR. Add a harbor test that makes the checker raise and assertsnot (work_dir / 'task.json').exists()and exit 1. Cross-axis: 6 (error handling), 8 (harness). - [Axis 3]
EvaluationResult.has_execution_failurehas no direct test, and a mutant that ignores the status in its grading exemption survives the suite. The changed input never occurs in practice (src/coder_eval/models/results.py:728) — Every caller now uses this property instead ofFinalStatus.is_execution_fact(orchestrator.py:467/632/942/947, evaluate_command.py:522/602, run_command.py:912). It decides whether a grading pass may overwrite a recorded status. The body isreturn self.final_status.is_execution_fact and not (self.final_status is FinalStatus.ERROR and self.error_phase == "grading"). I replaced it withself.final_status.is_execution_fact and self.error_phase != "grading", which lets a TIMEOUT / COST_BUDGET_EXCEEDED / TOKEN_BUDGET_EXCEEDED / BUILD_FAILED row taggederror_phase: gradingbe regraded into SUCCESS. All 361 targeted tests still passed. The CLI tests cover only ERROR/grading, ERROR/execution, ERROR/None and TIMEOUT/execution. tests/test_seed_from_prior_result.py:286 still pins only the context-free enum. Fix: add a parametrized table test over everyFinalStatusx {"execution", "grading", None}. It must assert that only (ERROR, "grading") returns False among the execution-fact statuses, that TIMEOUT/BUILD_FAILED/budget with phase "grading" stay True, and that NOT_GRADED/SUCCESS/FAILURE/MAX_TURNS_EXHAUSTED stay False. This is the test that trips the gate. - [Axis 8] Making grading ERROR repairable skips execution-phase work the crash cut short, so
--resume/evaluatecan turn a budget stop or an unfinished dialog into SUCCESS (src/coder_eval/orchestration/batch.py:414) — The new_owes_a_gradearmif result.final_status is FinalStatus.ERROR and result.error_phase == "grading": return True(batch.py:414-415) sends every fresh-run grading crash to the detached re-grade. That re-grade uses the evaluate-only path (agent None, orchestrator.py:2190-2221), which runs the criteria once and never calls_check_run_limits. But on a fresh run a criteria crash happens BEFORE execution-phase work that decides the status: (a) the post-criteria budget checkself._check_run_limits(iteration=iteration)at orchestrator.py:2314 is skipped; (b) in dialog mode,criteria_results = await self._run_dialog_criteria_check(judge_usage_accum)(orchestrator.py:2628, whencheck_every_turn) comes before the budget gate at 2636 and before the remaining simulated turns, so a crash cuts the dialog short; (c) the budget-gate fallback check at 2642 sits insideexcept BudgetExceededError:, so a crash there drops the BudgetExceededError. I reproduced (a) on the PR head withOrchestrator._check_run_limitspatched to raise BudgetExceededError. A cleanrungives TOKEN_BUDGET_EXCEEDED. The same run with one transientJudgeInfrastructureErrorfromcheck_all_asyncgivesERROR/error_phase=grading, and the nextrun --resumegives SUCCESS with weighted_score 1.0. So the same agent output gets a different final_status. On main this row stayed ERROR, so the status was never laundered into a pass. Under the Axis 8 scoring-correctness rule this blocks the merge. Fix: mark a fresh-run crash as repairable (error_phase="grading") only when all execution-phase work has finished. Option 1: run_check_run_limitsbefore the criteria, or again in the detached re-grade over the seedediterations. Option 2: when grading fails before the post-turn work, do not set error_phase "grading" on a simulation task (or on any task whose remaining loop work did not run), because_reject_simulation_under_executeexists for exactly this 'trajectoryrunwould not have produced' reason. Add a test that patches the run-limit check to trip, crashes the grader once, resumes, and asserts the budget status survives. Also add the same test for acheck_criteria: every_turndialog. Nightly impact: the change is in the gradedrunpath and the--resumepartition. Any eval-runner job that resumes a partial nightly would re-grade these rows and publish a different status. n/a - [Axis 8] A failed re-grade of an execution-fact row writes weighted_score 0.0, and
--resumethen never re-grades it (src/coder_eval/cli/run_command.py:911) — In_grade_resumed_tasks, the new first branchif result.grading_error is not None and (prior.has_execution_failure or ...)(run_command.py:911-918) only prints a message. It keeps the row that_finalize_resultalready wrote. In that row_record_errorkept the prior status (self.result.final_status = self.prior_result.final_status; return, orchestrator.py:942-946), and then_finalize_weighted_scorerancalculate_weighted_scoreover an empty results list, which writesself.weighted_score = 0.0(results.py:742-744). I reproduced it on the PR head:execute, then edit the row to TIMEOUT/execution, thenrun --resumewith a transient judge error. The row on disk isTIMEOUT execution 0.0 []withgrading_error='transient', and_owes_a_grade(...)returns False, because ofif result.weighted_score is not None or result.success_criteria_results: return False(batch.py:416). So a transient grader fault leaves the row with a 0.0 score that was never measured. That row now counts intasks_measured('Rows carrying a criteria verdict (weighted_score is not None)'), and no later--resumerepairs it. On main the ERROR result tookrestore_pre_grade_record(rt.run_dir), so the row stayed re-gradeable.evaluate <run_dir>has the same fault: theinheritedarm (evaluate_command.py:527-529) calls_write_back(target.target, result, prepared_run_dir), which writes the 0.0 and refreshes run.json. Fix: when grading fails, keepweighted_score = Noneand keepsuccess_criteria_resultsempty (or restore the prior record with the grading provenance added) for execution-fact priors too. A row with an opengrading_errormust not count as measured. Add arun --resumecase totest_failed_regrade_preserves_execution_fact_and_records_the_grader_incident, which today testsevaluateonly and does not checkweighted_score. n/a
Non-blocking, but please consider before merge
- [Axis 1] _record_error is C(14) and its interacting booleans are hard to follow; one branch records a reference tamper as a grading_error (
src/coder_eval/orchestrator.py:930) —_record_error(self, error, *, component, grading: bool = False)(radon C(14), in the hotorchestratormodule) combines three overlapping booleans: thegradingparameter (used only by the weighted-score call at line 1027),checker_failure = self._publish_grading_failures(error)(a call that mutates state and also returns a value), andis_grading = not isinstance(error, ReferenceTamperedError) and (checker_failure or self.prior_result is not None). Their combinations then pass through three early-exit branches. The cost to readability is visible at lines 942-944:if self.prior_result is not None and self.prior_result.has_execution_failure: if not is_grading: self.result.grading_error = self._grading_error_info(error, component). When a prior result exists,is_gradingis False only forReferenceTamperedError. So this branch records an answer-key tamper as a grading incident. That contradicts the notes ("an agent changing its answer key is an execution failure") and contradicts the NOT_GRADED-prior path, which a test pins tograding_error is None(test_execute_evaluate_loop.py:1042)._record_limit_error(905-909) also calls back into_record_errorunder its own condition, so a reader must trace both functions to know how one timeout is classified. Fix: classify once, up front, into an explicit outcome (for examplephase = self._classify(error)returning execution / grading / reference), then branch once on that value. Remove thegrading=flag parameter by having the weighted-score site raise or record through the same classifier. - [Axis 1] _report_and_exit rises to D(30); the failed-regrade disposition is duplicated in run_command (
src/coder_eval/cli/evaluate_command.py:524) — radon at the PR head:_report_and_exitis D(30), up from D(22) on the base (+8 in this PR)._grade_resumed_tasks(run_command.py:836) is C(14). Each function now makes the same three-way decision after a failed regrade: keep the prior execution fact and write back the grading incident, restore the prior verdict with provenance, or write back a new execution error. They do it with different branch structures: evaluate_command.py:524-546 (if result.grading_error is not None: ... if inherited or (prior.final_status is FinalStatus.ERROR and prior.error_phase == "grading"): ... else: restore_pre_grade_record(...)) and run_command.py:911-934 (if result.grading_error is not None and (prior.has_execution_failure or (...)): ... elif result.grading_error is not None or (...): restore_pre_grade_record(...); prior.grading_error = ...). The function is outside the hot modules and above CC 20, and the same logic appears at 2 sites, so the Axis-1 Medium anchor applies. Fix: move the decision into one function in orchestration/regrade.py, for examplesettle_failed_regrade(run_dir, prior, result) -> Outcome, that returns the row to publish. Each CLI then only prints and sets the exit code. - [Axis 2] New persisted public model GradingErrorInfo adds a dict[str, Any] escape hatch, although a typed ErrorContext already exists (
src/coder_eval/models/results.py:535) —details: dict[str, Any] | None = None(results.py:535) is a new field on a model thatcoder_eval.models.__all__exports and that task.json persists for downstream consumers. Its only producer is_grading_error_info(orchestrator.py:864-876), which storescreate_error_context(...). That helper builds a typedErrorContext(errors/categories.py:85) and then flattens it todict[str, Any]. Its docstring says the flattening exists only "for backward compatibility with EvaluationResult.error_details", and CLAUDE.md says the project is greenfield with no back-compat burden. errors/categories.py imports only enum and pydantic, so models/ can import it without a cycle. Type the field asErrorContext | Noneand build it directly. At the one dict-sourced site (details=prior.error_details, orchestrator.py:686) useErrorContext.model_validate(...), or record None for a legacy row. Also,ErrorContextalready haserror_messageandtimestamp, somessageandrecorded_atrepeat them. Choose one source for each. - [Axis 3] Resume arm that preserves execution facts on a failed regrade is untested. The evaluate twin has a test; the resume twin has none (run_command.py:911-918). (
src/coder_eval/cli/run_command.py:911) — Lines 911-918 handle a resumed regrade that fails over an execution-fact or grading-ERROR prior:if result.grading_error is not None and (prior.has_execution_failure or (prior.final_status is FinalStatus.ERROR and prior.error_phase == "grading")):. Line 915 is uncovered at PR HEAD. A mutant that replaced the condition withif False and (...)survived all 361 tests. With that mutant, a TIMEOUT row whose resume regrade fails goes to theelifarm instead, and line 931 rewrites its message in run.json toGrading errored during --resume: .... The evaluate-side twin is pinned by tests/test_execute_evaluate_loop.py:844 (test_failed_regrade_preserves_execution_fact_and_records_the_grader_incident), which only runsevaluate. Fix: parametrize that test (or add a sibling) withcommand in ("evaluate", "resume"), as test_repaired_grading_error_reaches_a_verdict_without_replaying_execution already does. Assert the run.json row and task.json keeperror_message == "original execution incident"and the original status, and carry the newgrading_error. Also add a resume case for a repeated failure over an already-grading-ERROR row. - [Axis 3] Resume fold-back of grading provenance into the run.json row is unasserted (
src/coder_eval/cli/run_command.py:932) — Lines 932-933 areprior.grading_error = result.grading_errorandprior.error_history = result.error_history. They copy the new incident onto the in-memory row that becomes the run.json entry. A mutant that deleted both lines survived all 361 tests. The only test of this arm, tests/test_detached_grading_boundaries.py:480 with phase="grading", asserts onlyfolded.final_statusand"Grading errored during --resume" in folded.error_message. It asserts nothing onfolded.grading_errororfolded.error_history, or on the on-disk copy. run.json is the downstream consumer contract (docs/REPORT_SCHEMA.md now documentsgrading_error/error_history), so this field can drop with no failing test. Fix: in that test, assertfolded.grading_error.message == "mock failure". Also assert that the on-disktask.jsongrading_errorequals the in-memory one, so run.json and task.json cannot disagree. - [Axis 3] New log-tail routing (keep the prior error_log_tail; attach the tail to grading_error) is untested; both mutants survive (
src/coder_eval/orchestrator.py:630) — Lines 629-633 aretail = log_tail.get_text() or None/if self.result.grading_error is not None: self.result.grading_error.log_tail = tail/if self.prior_result is None or not self.prior_result.has_execution_failure: self.result.error_log_tail = tail. I ran two mutants. (a) M2 always overwriteserror_log_tail, so a failed regrade replaces the original TIMEOUT/BUILD_FAILED tail, which carries the Docker build-log tail. (b) M3 drops thegrading_error.log_tailassignment. Both survived all 361 tests. orchestration.md says 'the run keeps its original status, message, structured context and log tail', and REPORT_SCHEMA.md listslog_tailas agrading_errorfield. test_failed_regrade_preserves_execution_fact_and_records_the_grader_incident (tests/test_execute_evaluate_loop.py:862-867) never seedserror_log_tailin the prior, and no new test readsgrading_error.log_tail. Fix: seederror_log_tail="original tail"in that prior. Assertfailed["error_log_tail"] == "original tail"andfailed["grading_error"]["log_tail"]is a non-empty string. - [Axis 5] One grading incident is stored twice (error_message/error_details/error_log_tail and grading_error), and the two copies do not agree (
src/coder_eval/orchestrator.py:951) — Fresh-run path: a criterion raises,_check_criteriarecordsGradingErrorInfowith component "orchestrator.grading" (line 890), and the outer handler callsself._record_error(e, component=failed_component)withfailed_component = f"orchestrator.iteration_{self.result.iteration_count}"(lines 593-595)._publish_grading_failuresthen setsself.result.grading_error = selected[1](line 900), and lines 949-958 write the same exception again into the terminal fields:self.result.final_status = FinalStatus.ERROR,self.result.error_phase = "grading" if is_grading else "execution",self.result.error_message = str(error),self.result.error_details = create_error_context(... component=component ...). The log tail is also written twice (lines 631 and 633). So one event has two records, and they disagree on which component failed. The model has no validator that tieserror_phase,grading_errorandfinal_statustogether (results.py:625-636). Because of this,_seed_from_prior_resultneeds special cases. It archivesprior.grading_errorAND, at lines 682-690, keeps anelifthat builds aGradingErrorInfofromprior.error_messagefor a grading-ERROR row that has nograding_error. The new writers cannot produce that shape, and no legacy row can haveerror_phase == "grading"because the field is new in this PR, so the branch is dead. Fix: choose ONE authoritative record. Either the terminal error fields are the incident andgrading_erroris set only when an execution fact outranks it, or the reverse. Add a model validator that enforces this invariant. Delete the unreachable synthesis branch. - [Axis 8] Grading-provenance fields are task.json-only; run.json rows and reports cannot show the grading incident or tell a grading ERROR from an execution ERROR (
docs/REPORT_SCHEMA.md:163) — REPORT_SCHEMA.md:163-170 addserror_phase,grading_erroranderror_historyto the task record, and resume/evaluate behaviour now depends on them. Buteval_result_to_task_dict(src/coder_eval/run_record.py, the run.json row serializer) still writes onlystatus,error_messageanderror_category, and no renderer inreports/readsgrading_error. The downstream eval-runner/dashboard reads run.json as its contract. It therefore cannot tell a repairable grading ERROR from a permanent execution ERROR. It also cannot see that a SUCCESS/FAILURE/NOT_GRADED row was restored with an opengrading_error, becauserestore_pre_grade_record(..., prior_result=prior, grading_result=result)writes the old verdict plus provenance to task.json only. Cross-run error-rate comparisons will count grader outages as agent errors. Fix: adderror_phaseand ahas_grading_errorboolean (orgrading_error.message) to the run.json row, and document them in REPORT_SCHEMA's run.json section. Optionally show a grading-incident marker in the markdown and HTML reports. The schema change is additive (EvaluationResult does not forbid extra keys), so older consumers and container images under version skew still load. No image rebuild is needed. n/a
Nits
- [Axis 1] restore_pre_grade_record round-trips JSON for no reason, and its log and console messages are out of date (
src/coder_eval/orchestration/regrade.py:593) — Lines 593-600 serialize the prior result (text = restored.model_dump_json(indent=2) if restored is not None else backup.read_text(...)), then, ifrestored is None, parse the text back (restored = EvaluationResult.model_validate_json(text)), set two fields, and serialize again (text = restored.model_dump_json(indent=2)). Build the model once, change it, and dump it once. The success log at line 607 still says"Restored the ungraded record at %s after a grading failure.", but the function now restores the immediate prior verdict, which can be SUCCESS or FAILURE. run_command.py:928-929 has the same problem: it prints"keeping the ungraded row so it stays re-gradeable."and interpolatesresult.error_message, while the sibling branch at :916 usesresult.grading_error.message. Usegrading_error.messagein both branches and reword both messages to say "prior record". - [Axis 1] New tests repeat the check_all_async patch block 9 times and the evaluate-or-resume argv block twice (
tests/test_execute_evaluate_loop.py:777) — The tests this PR adds writepatch("coder_eval.evaluation.checker.SuccessChecker.check_all_async", new=AsyncMock(side_effect=...))at lines 777, 809, 815, 870, 897, 951, 969, 981 and 1033. They also build the evaluate-or-resume argv block (["evaluate", str(task_dir)] if command == "evaluate" else ["run", str(AGENTLESS_TASK), "--run-dir", str(run_dir), "--resume"]) twice, at lines 790 and 1029. Replace these with two small helpers,_checker_raises(error)and_regrade_args(command, run_dir, task_dir). - [Axis 2] restore_pre_grade_record keeps a backup-only optional signature that no production caller uses, and the unit tests pin it (
src/coder_eval/orchestration/regrade.py:580) — The signature at regrade.py:580-581 isprior_result: EvaluationResult | None = None, grading_result: EvaluationResult | None = None. All three call sites pass both arguments (evaluate_command.py:532, evaluate_command.py:542, run_command.py:926). So theprior_result is Nonebranches (read the backup,EvaluationResult.model_validate_json(text), and theValueErrorcatch they need) cannot be reached from production code, and pyright cannot show that. Makeprior_result: EvaluationResultrequired. Pass only the two values used (grading_error: GradingErrorInfo | None,error_history: list[GradingErrorInfo]) and not a whole second EvaluationResult. Then delete the backup-read fallback, or keep it only if a caller still needs it. - [Axis 3] error_history accumulation branches are uncovered; an incident can be dropped from history with no failing test (
src/coder_eval/orchestrator.py:898) — Lines 898-899 areif self.result.grading_error is not None: self.result.error_history.append(self.result.grading_error). Also uncovered at PR HEAD: 939-941 (the same archival in_record_error), 944 (if not is_grading: self.result.grading_error = self._grading_error_info(error, component), which covers a ReferenceTamperedError during a regrade of an execution-fact prior) and 966 (the ReferenceTamperedError re-raise in_start_grading). Mutants that deleted 898-899 (M6) or 943-944 (M10) both survived. Fix: add an orchestrator-level test with two grading failures in one pass, for example a checker failure followed by acalculate_weighted_scoreValueError. Assert that both incidents appear acrossgrading_error+error_history. Add a regrade test that raises ReferenceTamperedError over a TIMEOUT prior and asserts the status stays TIMEOUT andgrading_erroris set. - [Axis 3] Test name contradicts two of its three parametrizations (
tests/test_detached_grading_boundaries.py:480) —async def test_the_resume_error_arm_restores_and_folds_back_ungraded(is now parametrized@pytest.mark.parametrize("phase", ["grading", "execution", None]). For "execution" and None it asserts that the row is NOT restored (expected = FinalStatus.NOT_GRADED if phase == "grading" else FinalStatus.ERROR). Split it into two tests: one that restores the ungraded row for a grading-origin ERROR, and one that keeps an execution-origin or unclassified ERROR. - [Axis 4] New CLI error prints put grading_error.message into Rich markup without escape(); a message containing '[/...' raises MarkupError before the prior verdict is restored (
src/coder_eval/cli/evaluate_command.py:525) — Line 525console.print(f"\n[red]✗ Evaluation error: {result.grading_error.message}[/red]")runs beforerestore_pre_grade_record(...)at line 532. Line 916 in run_command.py does the same:f"[yellow]⚠[/] Grading {rt.task.task_id} errored ({result.grading_error.message}); ". The message isstr(exception)from a criterion, from the before_grading hook, or from a timeout. A criterion error can include text from the graded workspace, which the agent controls. I checked with the repo's Rich:Console().print(f'[red]x Evaluation error: bad value [/tmp/x] in file[/red]')raisesMarkupError: closing tag '[/tmp/x] ... doesn't match any open tag'. When that happens,evaluatecrashes with a traceback and the prior SUCCESS/FAILURE/NOT_GRADED record is not restored, so the ERROR row stays on disk. Inrun --resumethe error propagates out of_grade_resumed_tasksand stops grading of the rows that follow. Balanced markup such as[link=...]can also inject terminal hyperlinks. Sibling handlers in this file already useescape(str(e))(lines 407, 431, 496). Fix: wrap the message inescape(...)at both new sites, and also at the existingresult.error_messageprints at evaluate_command.py:537 and run_command.py:928. CVSS:3.1/AV:L/AC:H/PR:N/UI:R/S:U/C:N/I:L/A:L - [Axis 5] restore_pre_grade_record becomes a fourth task.json serializer that does not use the shared TASK_JSON_TRANSCRIPT_EXCLUDE (
src/coder_eval/orchestration/regrade.py:594) — On main, restore copied the backup bytes verbatim. Now it re-serializes:text = restored.model_dump_json(indent=2) if restored is not None else backup.read_text(encoding="utf-8")(line 594) andtext = restored.model_dump_json(indent=2)(line 600). Each of the other three task.json writers passesexclude=TASK_JSON_TRANSCRIPT_EXCLUDE:Orchestrator._finalize_result(orchestrator.py:1195),evaluate_command._write_back(line 640) anddocker_runner(line 869). Because the restored row was loaded from a transcript-stripped task.json, today the only drift is extra"transcript": nullkeys. But the path now writes a different on-disk shape from its siblings. If a future caller passes an in-memory result, inline judge transcripts will be written into task.json. Fix: send every task.json write through one helper (for examplewrite_task_json(path, result)next to the exclusion constant) and call it here. - [Axis 6] Failed-regrade paths in run_command ignore the restore result and print inaccurate operator messages (
src/coder_eval/cli/run_command.py:926) — This change widensrestore_pre_grade_recordto catch(OSError, ValueError)and return False. Neither caller reads the result. run_command.py:926 callsrestore_pre_grade_record(rt.run_dir, prior_result=prior, grading_result=result), prints 'keeping the ungraded row', and setsresult = prior. evaluate_command.py:532 and 542 print 'prior verdict restored'/'preserved'. When the write fails or the target is a symlink, the ERROR-grading row stays on disk, but run.json is rebuilt from the in-memory prior. That gives exactly the run.json vs task.json mismatch the old docstring warned about. The row can still be re-graded through_owes_a_grade, so the failure is not severe. Fix: branch on the return value. On False, print a warning that names the file left as ERROR, and fold the on-disk result back in instead of the prior. - [Axis 7] New public persisted model GradingErrorInfo has no field descriptions (
src/coder_eval/models/results.py:531) —class GradingErrorInfo(BaseModel)declaresmessage: str,details: dict[str, Any] | None = None,log_tail: str | None = Noneandrecorded_at: datetimewith noField(description=...). The model is exported fromcoder_eval.modelsand serialized into task.json. Its siblingEarlyStopInfo(line 475) describes every field. The CLAUDE.md DRY principle says field descriptions are defined once in the Pydantic models. As a result, the field meanings exist only in docs/REPORT_SCHEMA.md:165-167, and the model's JSON schema has none. Fix: add descriptions to the four fields. State thatrecorded_atis naive local time, the same asstarted_at. - [Axis 7] _owes_a_grade docstring now contradicts its body (
src/coder_eval/orchestration/batch.py:414) — The docstring says "EVIDENCE, not label ... A row with a criteria vector or a score HAS been graded, whatever its status." The new first checkif result.final_status is FinalStatus.ERROR and result.error_phase == "grading": return Trueis a label test, and it returns True even when a score or a criteria vector exists. The docstring states the contract callers rely on, so update it to name this exception.
What's Missing
Parallel paths:
- 🟡 The new
_owes_a_gradearm sends a grading-origin ERROR to grade-in-place, but the resume contract on every other surface still says the opposite. Not updated: docs/USER_GUIDE.md § Resuming a run (table row "Any other status, including FAILURE / ERROR -> already complete" and "Resume never retries failures. FAILURE and ERROR count as complete"), thepartition_for_resumedocstring ("FAILURE and ERROR rows that DO carry a verdict stay complete under both commands"), thetest_partition_still_never_retries_failuresdocstring in tests/test_resume.py, and thetask.execute.jsonline in .claude/shared/run-layout.md / plugins/coder-eval/reference/run-layout.md ("over a NOT_GRADED row"). A user who reads the guide expects an ERROR row to stay final, but a laterrun --resumenow changes it. (trigger: src/coder_eval/orchestration/batch.py) _(restates: Axis 7: owes_a_grade docstring now contradicts its body) - 🟡 The provenance contract (
grading_error+error_history) applies only to the in-process grading path. Thedriver: dockerdetached-grade path is not updated._grade_in_containerturns a container failure intoRegradeError. On resume (run_command.py:895-908), that arm only appends "Grading failed during --resume" toerror_messageand records noGradingErrorInfoand no history. Inevaluate(evaluate_command.py:491), it exits 1 and records nothing. A grading-ERROR docker row that fails again in its grading container therefore loses the audit trail the PR promises, and its run.json message changes. No test drives the RegradeError arm over anerror_phase: gradingprior. (trigger: src/coder_eval/cli/run_command.py)
Tests:
- 🟡 The unit tests for
partition_for_resumein tests/test_resume.py were not extended for the new_owes_a_gradearm. Missing: a direct case where ERROR +error_phase="grading"withgraded=True(score/criteria present) goes toto_gradeundergrade=Trueand stays inprior_resultsundergrade=False, plus companion cases that show ERROR/"execution", ERROR/None and TIMEOUT/"grading" stay complete. Today only the CLI integration tests pin the new routing, andtest_partition_still_never_retries_failureskeeps passing only because its fixture rows set no phase. (trigger: src/coder_eval/orchestration/batch.py)
Downstream consumers:
- 🟡 The PR adds an execution-vs-grading classification (
error_phase,has_execution_failure), but the consumers that compute rates and scores from status were not updated.orchestration/experiment.py::_measured_scoresstill scores everycategory == "error"row as 0.0, so a grader outage counts as an agent miss in A/B means and the nightly headline. The run-leveltasks_error/ error-share rates and the suite rolluprows_error(reports/markdown.py:951) do the same. The_pick_worst_statusdocstring still says a failed resume grade folds back "still ungraded", which is no longer true: the restored prior can now be SUCCESS/FAILURE with an opengrading_error. (trigger: src/coder_eval/models/results.py) (restates: Axis 8: Grading-provenance fields are task.json-only; run.json rows and reports cannot show the grading incident or tell a grading ERROR from an execution ERROR)
Display & mapping dicts:
- 🔵 No new enum value was added, but the new
grading_error/error_historyfields are not shown anywhere.reports/html.py::_render_error_detailsand the markdown failure sampling (reports/markdown.py:1058/1144) read onlyerror_message/error_details/error_log_tail. A SUCCESS/FAILURE/NOT_GRADED row restored with an opengrading_errorshows no error section. An ERROR row does not say whether it is a repairable grading incident. Archived incidents inerror_historyare never shown. The evalboard task shape (evalboard/lib/runs.ts) does not read these fields either. (trigger: src/coder_eval/models/results.py) (restates: Axis 8: Grading-provenance fields are task.json-only; run.json rows and reports cannot show the grading incident or tell a grading ERROR from an execution ERROR)
Nightly pipeline:
- 🟡 The PR description says no saved records change, but it does not state the effect on the nightly pipeline. (a) task.json gains
error_phase/grading_error/error_history, which the external coder-eval-uipath / eval-runner consumers read. (b) Fordriver: docker, the orchestrator that runs inside the container writeserror_phase, so an image built before this change writes untagged ERROR rows. The host then treats these as legacy execution facts that cannot be repaired, until the nightly image is rebuilt. (c) Any nightly job that resumes a partial run now re-grades fresh grading-ERROR rows and publishes a different final_status and pass rate than the original run.json. The PR must state whether an image rebuild is required and what a resumed nightly will report. (trigger: src/coder_eval/orchestrator.py)
Harness & Lint Improvements
Static checks (lint / type):
- [ce-lint] CE069 (next free number after CE068; CE067 has no rule file, so check .claude/notes/lint-rules.md before you reuse it)
no_inline_grading_error_predicate. The rule forbids anast.Comparewhose left side is an attribute ending in.error_phaseand whose right side is a string literal, anywhere under src/coder_eval/ except models/results.py. It also forbids.final_status.is_execution_factoutside models/results.py and models/enums.py. Callers must use the newEvaluationResult.is_grading_error/has_execution_failureproperties. Put the rule in tests/lint/rules/ce069_no_inline_grading_error_predicate.py and register it in tests/lint/runner.py. Add a module docstring that names the blind spot: a getattr or a rebound local escapes the rule. Prevents: The 7 hand-written copies offinal_status is ERROR and error_phase == "grading"(orchestrator.py:682/:691, results.py:731, batch.py:414, run_command.py:913/:920, evaluate_command.py:527). The variant at evaluate_command.py:541 that already drifted (error_phase == "grading"with no ERROR check). The overlapping is_execution_fact/has_execution_failure pair (run_command.py:912-913). Finding 1, and part of finding 2's dead branch at evaluate_command.py:541. - [pyright] Make the illegal error states unrepresentable, so that pyright proves the dead branches dead. Replace the three independent fields
error_phase: Literal[...] | None,error_message/error_detailsandgrading_erroron EvaluationResult with one discriminated fieldterminal_error: ExecutionError | GradingIncident | None(discriminatorphase). Keepgrading_erroronly as an "outranked by an execution fact" slot. Thenif result.grading_error is None and <phase is grading>cannot be expressed, and pyright standard mode reports the narrowed-to-Never arm. You can also turn onreportUnnecessaryComparisonfor cli/ and orchestration/regrade.py. Prevents: Finding 2 (the unreachable grading-phase branch at evaluate_command.py:541-543 and the redundant clause at run_command.py:919-921). Finding A5 (one grading incident stored twice with components that disagree, orchestrator.py:949-958 vs :890/:900). The dead synthesiselifat orchestrator.py:682-690, which builds a GradingErrorInfo from error_message for a shape that no writer produces. - [ce-lint] CE070
rich_markup_must_escape_dynamic_text, scoped to src/coder_eval/cli/ and any module that imports rich. It fires onconsole.print(f"...")and onConsole.print/.log/.statuswith a JoinedStr argument when a FormattedValue interpolates an expression whose trailing name matchesmessage|error|error_message|reason|detail|stderr|output|^e$or is astr(<exc>)call, and that expression is not wrapped inescape(...)from rich.markup or amarkup=Falsekwarg. Use# noqa: CE070for text that is known to be static. Prevents: Finding A4:grading_error.message(agent-influenced criterion text) put into Rich markup at evaluate_command.py:525 and run_command.py:916, and the existing unescapederror_messageprints at evaluate_command.py:537 and run_command.py:928. A message that contains[/...raises MarkupError before restore_pre_grade_record runs, and it can stop grading of later rows on--resume. Sibling handlers in the same file already use escape() (lines 407/431/496), so the pattern is known and only remembered by hand. No ruff or bandit rule covers Rich markup. - [ce-lint] CE071
task_json_single_writer. First add onewrite_task_json(path, result)helper next to TASK_JSON_TRANSCRIPT_EXCLUDE in evaluation/judge_persistence.py, which always passesexclude=TASK_JSON_TRANSCRIPT_EXCLUDE. Then forbid anywrite_text_atomic(...)/Path.write_text(...)whose payload is anEvaluationResult.model_dump_json(...)call outside that helper. The AST shape is amodel_dump_jsoncall that is a direct argument of a write call, or that is assigned to a local which a write call then uses. This sits beside CE053, which already owns the TASK_JSON filename literal. Prevents: Finding A5-low: restore_pre_grade_record became a fourth task.json serializer (regrade.py:594/:600) and dropped TASK_JSON_TRANSCRIPT_EXCLUDE. The three siblings at orchestrator.py:1195, evaluate_command.py:640 and docker_runner.py:869 each pass the exclusion by hand. It also removes the needless JSON round-trip in finding A1-low (regrade.py:593-600). - [ce-lint] Extend CE049 (no_score_or_zero) from
score or 0.0to assignments. Fire on<target>.weighted_score = <numeric literal>and on<score-named attr> = ... if ... else 0.0in src/. The intent is the same as CE049: an unmeasured row is None, never 0.0. Use# noqa: CE049for the one legitimate case, which is 0.0 when a task really has zero criteria and has been graded. Prevents: Finding A8-high.calculate_weighted_scorewritesself.weighted_score = 0.0over an empty results list (results.py:742-744) after a failed regrade of an execution-fact prior._owes_a_grade(batch.py:416) then treats the row as measured, so--resumenever repairs it, and tasks_measured counts a score that was never measured. - [ce-lint] CE072
run_json_projection_parity, a whole-tree@pytest.mark.lintclass in tests/test_custom_lint.py that follows the CE030 doc_schema_parity pattern. Every EvaluationResult field must either be emitted byrun_record.eval_result_to_task_dictor be listed in aRUN_JSON_EXEMPT: dict[str, str](field -> reason) in tests/lint/. It must also be named in docs/REPORT_SCHEMA.md's run.jsontask_results[]section when it is emitted. The rule also fails when a field thatorchestration/batch.py(_owes_a_grade) reads is exempt, because resume behaviour must not depend on a field that run.json consumers cannot see. Prevents: Finding A8-medium: error_phase, grading_error and error_history are documented only for task.json (REPORT_SCHEMA.md:163-170), resume branches on them, and run.json (the downstream contract) cannot tell a repairable grading ERROR from an execution ERROR or show a restored verdict with an open grading_error. - [ce-lint] CE073
no_untyped_dict_on_persisted_model. In src/coder_eval/models/, a field annotation ofdict[str, Any](ordict[str, Any] | None) on a BaseModel class is forbidden unless the class.field pair is in an allowlist that holds today's offenders (for example EvaluationResult.error_details). The allowlist can only get smaller. In the same rule, also requireField(description=...)on every field of a class exported fromcoder_eval.models.__all__, with the same frozen allowlist. This applies the CLAUDE.md DRY rule that "field descriptions are defined once in the Pydantic models". Prevents: Finding A2-medium:GradingErrorInfo.details: dict[str, Any] | None(results.py:535) when the typedErrorContextalready exists. Finding A7-low: GradingErrorInfo's four fields have no descriptions (results.py:531), while the sibling EarlyStopInfo describes every field. - [ce-lint] CE074
no_always_passed_optional_param, a whole-tree rule in the same family as CE037 (dead private helper). It flags a function in src/ that has a parameter annotatedX | None = Nonewhen every call site in src/ passes that parameter by keyword with an expression that is not the literalNone. A test-only call does not count. This means the None-branch is reachable only from tests. Use# noqa: CE074for a public SPI entry point. Its blind spots:**kwargsforwarding and dynamic calls. Prevents: Finding 2(3) and finding A2-low:restore_pre_grade_record(..., prior_result: EvaluationResult | None = None, grading_result: ... | None = None)(regrade.py:580-581). All three production callers pass both arguments, so the backup-read fallback (regrade.py:590-597) is live only in tests and misleads readers about crash recovery. - [ci-gate] Add a complexity ratchet:
make complexity-ratchet, wired intomake verify. It runsradon cc -jon the files changed against origin/main. It fails when a function that is already rank C or worse (CC >= 11) increases its CC, or when a new function exceeds CC 15 in the hot modules (orchestrator.py, orchestration/, cli/). Ruff C901 alone is impractical here, because about 200 functions are already rank C or worse and about 47 are D or worse. That many noqa markers would dilute it, so a diff-relative ratchet follows the "cap below the current value" rule without a flag day. Prevents: Finding 1-medium:_record_erroris C(14) with three interacting booleans (orchestrator.py:930). Finding A1/A7-medium:_report_and_exitrose from D(22) to D(30) and_grade_resumed_tasksfrom B(8) to C(14), with the failed-regrade disposition duplicated in two shapes (evaluate_command.py:524-546, run_command.py:911-934).
Harness improvements (not statically reachable):
- Add a crash-then-resume equivalence matrix test. For each interruption point in the graded
runpath, inject a single JudgeInfrastructureError at that point, then runrun --resume. Assert that the final_status and weighted_score match an uninterruptedrunof the same agent output. The interruption points are: a criteria crash before the post-criteria_check_run_limits(orchestrator.py:2314), a dialogcheck_every_turncrash at :2628 before the budget gate at :2636, and a crash inside theexcept BudgetExceededErrorfallback at :2642. Patch_check_run_limitsto trip in the budget cases. Why not static: Whether execution-phase work (run-limit checks, remaining dialog turns) was skipped depends on the order of runtime events and on which statuses the detached evaluate-only path re-derives. Only running both paths and diffing the results shows this. Prevents: Finding A8-critical: a transient grader crash makes the row repairable (_owes_a_gradeat batch.py:414-415), and--resumethen launders TOKEN_BUDGET_EXCEEDED or an unfinished dialog into SUCCESS. - Add evaluate/resume twin parity with a run.json == task.json assertion. Put a shared helper
assert_row_agrees(run_dir, task_id)in tests/ that compares status, error_message, weighted_score, grading_error and error_history between the run.json task_results row and the on-disk task.json. Every failed-regrade test must be parametrized overcommand in ("evaluate", "resume")and must call the helper. Start with test_failed_regrade_preserves_execution_fact_and_records_the_grader_incident (tests/test_execute_evaluate_loop.py:844) and test_the_resume_error_arm_restores_and_folds_back_ungraded (tests/test_detached_grading_boundaries.py:480). Add one case that seeds error_log_tail and asserts it survives while grading_error.log_tail is set. Why not static: Run.json and task.json disagree only after a CLI run writes both files, through different code paths (in-memory fold-back in run_command vs. the _write_back in evaluate). A lint rule cannot see whether two runtime artifacts agree. Prevents: Finding A3-medium (run_command.py:911-918 preserve arm is untested; a mutant of it survives). Finding A3-medium (the fold-back at run_command.py:932-933 is unasserted). Finding A3-medium (the log-tail routing at orchestrator.py:629-633). Finding A6-low (the restore return value is ignored, which causes the run.json/task.json mismatch). Finding A8-high (weighted_score 0.0 after a failed resume regrade). - Add a runtime model invariant plus a classifier table test. Add an EvaluationResult
model_validator(mode="after"):error_phase == "grading"impliesfinal_status is ERRORandgrading_error is not None. Also add a parametrized test over every FinalStatus x {"execution", "grading", None} that pinshas_execution_failure/is_grading_error. In that test, only (ERROR, "grading") is exempt among the execution-fact statuses, and TIMEOUT, BUILD_FAILED and the budget statuses stay True with phase "grading". Why not static: These are invariants between the values of three fields, so they hold or fail at construction and load time (including on hand-edited task.json). A lint rule cannot reason about values. The pyright discriminated-union redesign in the static bucket is the long-term fix, and this validator is the guard until that ships. Prevents: Finding A3-high: thehas_execution_failuremutant that ignores the status survives every test. Finding A5-medium: one incident is stored twice and the model enforces no invariant. Finding 2: the dead branches stay hidden because nothing states that the shape cannot occur. - Add a Harbor WORK_DIR immutability guard. In a parametrized test, run
evaluate --format harborunder every grading failure mode (checker raises, before_grading hook fails, grading timeout, weighted-score ValueError). Assert with path_utils' tree digest that the WORK_DIR digest is the same before and after, and that the exit code is 1. Why not static: Whether a write lands in the user's directory depends on the runtime EvaluateMode and on whether restore_pre_grade_record decides to write. A rule that requires every write call to be under a RUN_DIR guard would be a dominance analysis over semantic conditions, and that is not reliable with AST matching. Prevents: Finding A3-high: a grading failure underevaluate --format harborplants task.json in the user's WORK_DIR, because the restore calls at evaluate_command.py:532 and :542 have no RUN_DIR guard. - Add a diff-scoped mutation-testing target.
make mutate-diffruns mutmut (or cosmic-ray) only on lines changed in orchestrator.py, orchestration/batch.py, orchestration/regrade.py, cli/run_command.py, cli/evaluate_command.py and models/results.py, against the tests that touch them, and lists the surviving mutants. Run it in the review workflow for PRs that change grading, resume or status classification, as an advisory step, not a blocking gate. Why not static: A surviving mutant shows that tests are weak, and that can be known only by running the suite against changed code. Coverage alone said these lines were partly covered. Prevents: This review needed hand-made mutants to show six untested branches: has_execution_failure (results.py:728), run_command.py:911 and :932-933, orchestrator.py:629-633, :898-899 and :943-944 (finding A3-low error_history accumulation and the ReferenceTamperedError-over-TIMEOUT path). - Record the decision about what static analysis cannot reach. These findings need human review or a targeted test, not a rule: the _owes_a_grade docstring that contradicts its body (batch.py:414), the stale operator and log messages ("ungraded record" at regrade.py:607, run_command.py:928-929), the test name that contradicts its parametrization (test_detached_grading_boundaries.py:480), and a ReferenceTamperedError recorded as grading_error over an execution-fact prior (orchestrator.py:942-944). For the last one, add the regrade test that raises ReferenceTamperedError over a TIMEOUT prior and asserts that the tamper is classified as an execution fact. The repeated check_all_async patch block (9 copies) and the evaluate/resume argv block (2 copies) in tests/test_execute_evaluate_loop.py should become shared
_checker_raises/_regrade_argshelpers when the twin-parity parametrization is added. A rule is not worth it for test duplication of this size. Why not static: Docstring and message accuracy, test-name semantics and whether a tamper is a grading or an execution failure need a semantic judgment about intent, and no AST pattern expresses that. Prevents: Finding A7-low (docstring drift), finding A1-low (stale messages and test duplication), finding A3-low (misleading test name) and finding 1-medium (tamper misclassified in _record_error).
Top 5 Priority Actions
- Stop the critical status laundering at src/coder_eval/orchestration/batch.py:414. Set error_phase="grading" on a fresh-run crash only after all execution-phase work is done, or run
_check_run_limitsagain over the seeded iterations in the detached regrade (orchestrator.py:2314, 2628, 2642). Add tests: a budget stop plus one grader crash and then--resumemust keep TOKEN_BUDGET_EXCEEDED, and acheck_criteria: every_turndialog needs the same test. - Fix the unmeasured 0.0 score at src/coder_eval/cli/run_command.py:911 and evaluate_command.py:527. When a regrade fails over an execution-fact prior, keep weighted_score None and success_criteria_results empty, or restore the prior record with the grading provenance added, so that
_owes_a_grade(batch.py:416) still sends the row to regrade and tasks_measured does not count it. Add arun --resumecase that asserts weighted_score is None. - Add an
EvaluateMode.RUN_DIRguard to bothrestore_pre_grade_recordcalls at src/coder_eval/cli/evaluate_command.py:532 and :542, so that a grading failure underevaluate --format harbordoes not write task.json into the user's WORK_DIR. Add a harbor test where the checker raises and assert that no task.json is left behind. - Add one
EvaluationResult.is_grading_errorproperty (src/coder_eval/models/results.py:728) and use it at all 7 hand-written copies (orchestrator.py:682/691, batch.py:414, run_command.py:913/920, evaluate_command.py:527). Delete the unreachable branches at evaluate_command.py:541-543 and run_command.py:919-921, and the backup-read fallback in regrade.py:590. Makeprior_resultrequired and rename the function so its name matches what it now does. - Close the test gaps in the new contract. Add a parametrized FinalStatus x error_phase table test for
has_execution_failure(results.py:728). Add aresumeparametrization to the preserve-execution-fact test for run_command.py:911-918. Add asserts on grading_error and error_history fold-back (run_command.py:932-933) and on log-tail routing (orchestrator.py:629-633). Then add error_phase and grading_error to the run.json row in run_record.py and REPORT_SCHEMA.md:163, so downstream consumers can tell a grading ERROR from an execution ERROR.
Stats: 1 🔴 · 5 🟠 · 8 🟡 · 10 🔵 across 8 axes reviewed.
|
Claude finished @cosminacho's task in 1m 40s —— View job Review in Progress
|
bai-uipath
left a comment
There was a problem hiding this comment.
Right goal, but please restructure before merge. Most of the complexity comes from deciding after the fact whether an exception was execution or grading, and from writing task.json and then undoing it.
- Settle execution facts before grading: check run limits right after the turn without raising, grade, then raise. A grading crash can then never skip a budget or timeout fact, and the seeded limit error and the re-checks in the error path go away.
- One grading error type: wrap the grading sites so failures surface as a single typed error handled in one place, instead of matching exceptions by identity across several flags.
- A mid-dialog grading crash is an execution failure: the remaining turns never ran, so treat it as unfinished execution; the dialog special cases in the status check and in regrade go away.
- Two fields, not three: keep
error_phaseanderror_history; dropgrading_error, which duplicateserror_message/error_details. - Write once on regrade: don't persist a failed regrade; the caller appends the incident to the prior record and writes once. The stale-backup overwrite becomes impossible and the restore branches in run and evaluate collapse into one.
- Split out the
weighted_score0.0 to None change: it's a scoring change unrelated to recovery, and averages that drop None (the Flow V2 comparison in coder_eval_uipath) would stop counting errored rows.
If you want the Discovery benchmark unblocked sooner, the typed error, the two fields and the owes-a-grade change are enough for a first PR.
1e2496b to
4608e6b
Compare
There was a problem hiding this comment.
Approve: all six restructure points landed, and the settle, grade, then write-once shape is much easier to follow.
Before merge:
- Delegate smoke prompt: reframing FizzBuzz as a UiPath Python Scope helper changes what the Delegate live E2E measures and has nothing to do with grading recovery.
- Resume docs: the user guide still says resume never retries FAILURE or ERROR, and the owes-a-grade docstring still says any row with a score has been graded. Fix: update both for grading-phase ERROR.
Consider as follow-up:
- Docker failure marker: the container signals a failed grade through a magic key in the incident's untyped details dict, which has to be popped in three places; if one is ever missed, every later container regrade of that row reads as failed. Fix: a sidecar file or a distinct exit code.
- run.json can't see the phase: reports and evalboard still can't tell a grading ERROR from an execution ERROR.
Minor: an is_grading_error property would replace the four hand-written copies of the check (one carries a clause that is always true); telemetry fires for a failed regrade that is never persisted.

A grader failure can hide an execution limit or replace a previously measured result during detached grading. This change records execution limits before grading, keeps execution failures as the primary outcome, and distinguishes completed grading failures from interrupted execution.
Grading failures use one typed boundary. Results retain
error_phaseanderror_history; a failed detached grade preserves the immediate prior verdict, answers, usage and judge artifacts, then appends one incident with one authoritative write. When evaluating an external work directory, recovery does not write run records into it. Completed runs whose grading failed remain eligible for grading without replaying the agent. The original weighted-score behavior is preserved.The inherited OAuthlib minimum and lock now require 4.0.1 for CVE-2026-49272.
Validation:
The targeted dependency update preserves all other locked packages. A hash-checked OAuthlib 4.0.1 installation passed a focused package audit; full automatic CI remains the publication check.