feat(criteria): let a run_command criterion opt in to post-failure grading

[rockymadden]

Why

Two runs in the UiPath/skills eval suite scored 0.00 on artifacts that are correct:

Task	Graders re-run by hand against the preserved sandbox
skill-flow-init-plain-default	project_profile.py --expect-profile Standard --expect-no-sentinel → exit 0; flow_contains.py → exit 0
skill-flow-hitl-smoke-node-placed	flow_contains.py → exit 0; check_simulated_hitl.py quick-form → exit 0

Both ended ERROR on a turn timeout. BaseSuccessCriterion.supports_post_failure_evaluation is
False, RunCommandCriterion inherits it, and the orchestrator skips every False criterion —
so all four graders recorded not_evaluated with "the criterion is not a deterministic,
read-only artifact check". The whole uipath-maestro-flow suite grades through run_command,
so a timeout there erases the artifact evidence entirely. The score was wrong, not the work.

The blanket default is still right: an arbitrary shell command can mutate state or hit a live
tenant. Several maestro-flow criteria shell out to uip maestro flow debug, which starts a real
cloud job. Flipping the type wholesale is not an option.

What

read_only: bool = False on RunCommandCriterion. When set, the criterion joins the post-failure
diagnostic pass that already runs file_exists, file_contains, json_check and friends while
the sandbox is still live.

Design

The flag lives on RunCommandCriterion, not on the base. A general override would let any
criterion type — a judge, a trajectory check — be declared post-failure-safe, which is a different
and much larger claim (a judge is neither deterministic nor free). run_command is the one type
whose safety genuinely depends on how the task author wrote it, so it is the one type that gets to
decide per instance.

supports_post_failure_evaluation stays a ClassVar. It has exactly one consumer
(orchestrator.py), confirmed by grepping all of src/. Rather than reshape it into an instance
field on all eight declaring classes, the orchestrator now asks a new property,
BaseSuccessCriterion.evaluable_after_agent_failure, which defaults to the ClassVar. The two names
carry two different statements: the ClassVar says "this criterion TYPE is always a read-only
artifact check"; the property says "THIS instance may run after a terminal failure".
RunCommandCriterion overrides only the property.

What the flag does NOT promise. Nothing stops someone marking a debug-invoking criterion
read_only, and nothing can: purity of a shell command is not statically decidable, and the flag
adds no sandboxing, no dry-run, no argv inspection. The command runs exactly as it always did. The
flag is a task author's declaration, the task author is accountable for it, and both the field
description and
the guide
say so in those words — "UNVERIFIED AND UNENFORCED", "declaration, not a restriction", with
uip maestro flow debug named as the case that must stay false. The name says what the author
asserts, not what the harness checked.

A timeout cannot become a pass

post_failure_criteria_results is a separate list from success_criteria_results.
calculate_weighted_score reads only the latter, which is empty on the terminal-error path, so
weighted_score stays 0.0 and final_status stays ERROR. The new test asserts all three
alongside the recovered score. Nothing about that contract changed — the flag only widens which
criteria produce evidence in the diagnostic list.

Also checked: read_only survives model_dump(exclude_unset=True) through the discriminated union
(so dataset fan-out in task_loader.py keeps it — pinned by a test); success_criteria is not a
config_merge layer, so there is no task/experiment inheritance path to get wrong; and
UiPathEvalCriterion subclasses the base, not RunCommandCriterion, so it inherits nothing.

Last touch: a skipped run_command now names the opt-in in its not_evaluated detail, because the
old message gave an author no way to discover it.

Version skew

Criterion models are extra="forbid", so a task YAML that sets read_only: true fails
validation on any coder-eval older than the release that ships this. Downstream suites adopting
the field must pin to at least that release — including pinned Docker images and older runners.

Tests

uv run pytest -n auto -m "not live and not lint" tests/   → 1 failed, 6091 passed, 2 skipped
uv run pytest tests/test_custom_lint.py                   → 677 passed
uv run ruff format --check / ruff check / pyright         → clean

The one failure is test_docker_runner_mounts.py::TestOutputMountWidenedBeforeLaunch, pre-existing
and unrelated — it fails identically on a stashed (clean) tree on this macOS host.

New coverage:

• test_read_only_run_command_is_graded_after_a_terminal_agent_error — a marked grader is
  evaluated with score 1.0 after a turn timeout; a marked grader that exits non-zero is
  evaluated with score 0.0 and not the not_evaluated placeholder (the two are otherwise
  indistinguishable by score); an unmarked one stays not_evaluated, provably never executes, and
  gets the opt-in hint, which a judge does not; and the run is still ERROR /
  weighted_score == 0.0.
• test_post_failure_property_tracks_the_type_answer — parametrized over every union member:
  the property may diverge from the ClassVar only for run_command, only via read_only.
• test_run_command_is_not_post_failure_evaluable_by_default
• test_read_only_survives_exclude_unset_round_trip

Docs: docs/TASK_DEFINITION_GUIDE.md#run_command, docs/REPORT_SCHEMA.md post-failure section,
and the generated plugins/coder-eval/reference/criteria.md (make plugin-reference).

Dependency

A companion PR in UiPath/skills sets read_only: true on the maestro-flow artifact checks. It
depends on this landing first.

🤖 Generated with Claude Code

[github-actions]

Claude finished @rockymadden's task in 1m 48s —— View job

---

Code Review In Progress

Todo List:

• Read .github/code_review.md for review standards
• Read CLAUDE.md for project conventions
• Get full diff with git diff origin/main...HEAD
• Read and analyze each changed file in full
• Perform cross-file consistency checks
• Check for missing elements and design-level issues
• Post comprehensive review feedback

[Copilot]

Copilot review overview

🟡 Changes recommended

A task-timeout cancellation can leave a grader subprocess running concurrently with sandbox teardown.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 · 3

Open (4)

• Cancellation can leave grader subprocess running during sandbox teardown · New
• Include budget exhaustion in diagnostic trigger documentation · New
• Document budget-exhaustion trigger and regenerate plugin reference · New
• Update docstring to reflect read-only sandbox command execution · New

What changed in this PR

Adds opt-in post-failure diagnostic grading for artifact-only run_command criteria without changing canonical scores or statuses.

Changes:

• Adds the read_only declaration and per-instance eligibility.
• Updates post-failure orchestration and tests.
• Documents the new configuration and report behavior.

File	Description
src/​coder_eval/​models/​criteria.py	Defines read_only and eligibility.
src/​coder_eval/​orchestrator.py	Selects eligible post-failure criteria.
tests/​test_timeout_orchestrator.py	Tests terminal-error grading behavior.
tests/​test_success_criterion_union.py	Tests defaults and serialization.
docs/​TASK_DEFINITION_GUIDE.md	Documents task configuration.
docs/​REPORT_SCHEMA.md	Documents diagnostic results.
plugins/​coder-eval/​reference/​criteria.md	Updates generated criterion reference.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[uipreliga]

Review: coder_eval — PR #197 (branch vs main, 7 files) axis:1,3,4,7,8

PR #197 · feat(criteria): let a run_command criterion opt in to post-failure grading · feat/read-only-run-command-post-failure → main · fbf44b8 · 2026-09-24T17:07Z

Change class: complex — adds a public schema field that changes which criteria execute sandbox commands on the post-failure recovery path

The read_only post-failure grading change is sound: it adds no correctness, data-loss or security defect, and no persisted score changes. The real risks are documentation that promises budget-breach diagnostics the code never writes, a string-literal type check that can fail silently, and a shell command that can outlive the task_timeout watchdog. Bottom line: merge after a short docs-and-tests follow-up.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Correctness & Logic	9.9 / 10	0	0	0	1	Budget-breach post-failure trigger is named in docs but no graded path reaches it and no test covers it
3. Types & Contracts	9.8 / 10	0	0	0	2	_unavailable_reason matches on a string literal where the repo's rule is isinstance narrowing
4. Test & Validation Health	9.9 / 10	0	0	0	1	Missing tests for read_only post-failure edge branches (failing command, non-run_command hint)
7. Error Handling & Resilience	9.9 / 10	0	0	0	1	A read_only run_command on the post-failure path is not stopped by the task_timeout watchdog, so it can still run while teardown deletes the sandbox
8. Interface, Docs & Compatibility	9.9 / 10	0	0	0	1	read_only Field description says the command runs on every failure path, but execute mode skips it; the text is also much longer and louder than sibling fields

Overall Score: 9.9 / 10 · Weakest Axis: Types & Contracts at 9.8 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 0 · 🔵 6 across 5 axes reviewed.

Blockers (0 🔴 Critical · 0 🟠 High)

None.

Non-blocking, but please consider before merge

None.

Nits

1. [Axis 1] Budget-breach post-failure trigger is named in docs but no graded path reaches it and no test covers it (docs/REPORT_SCHEMA.md:191) — The PR docs say that a budget breach is one of three triggers for the post-failure path. REPORT_SCHEMA.md:191 says "or a token/cost budget breach stops grading part-way". TASK_DEFINITION_GUIDE.md:908 and :923 say the same. The read_only Field description at models/criteria.py:419 says "a token or cost budget breach that stopped grading part-way. The command runs on every one of them". The code does not reach that path on a budget breach. In orchestrator.py, _run_evaluation_with_failure_evidence re-raises a BudgetExceededError when len(self.result.success_criteria_results) == len(self.task.success_criteria). On the graded single-shot path, _check_run_limits runs only after check_all_async fills the complete canonical vector ("# AFTER the criteria, so partial-credit visibility is preserved." then self._check_run_limits(iteration=iteration)). So the guard always re-raises. On the execute path (grade off), _evaluate_post_failure_criteria returns early. The dialog site catches BudgetExceededError locally (except BudgetExceededError: stop_reason = DialogStopReason.RUN_LIMIT_EXCEEDED). Also, check_all_async never raises it (grep for BudgetExceededError in src shows raise sites only in _check_run_limits). The result: a TOKEN/COST_BUDGET_EXCEEDED row does not have post_failure_criteria_results for a read_only command. It has the canonical result instead, and nothing is lost. The problem is only that a reader of the docs expects diagnostic evidence that is never written. Recommendation: remove the budget clause from the docs, or say that a budget breach happens after canonical grading, so the read_only command is already graded in success_criteria_results. Commit fbf44b8 is in this PR, so the change adds this claim.
2. [Axis 3] _unavailable_reason matches on a string literal where the repo's rule is isinstance narrowing (src/coder_eval/orchestrator.py:868) — if criterion.type == "run_command": compares a discriminated-union tag to a bare string. pyright runs in typeCheckingMode = "standard" (pyproject.toml:347), and in that mode reportUnnecessaryComparison is off. So if the tag is renamed, this check becomes False with no warning and the 'declare read_only: true' hint is lost without notice. The repo already names this hazard and uses isinstance for the same reason: regrade.py:217 if isinstance(c, RunCommandCriterion): and tasks.py:716 elif isinstance(c, RunCommandCriterion) and ..., with docstrings that say 'isinstance narrowing, never getattr ... a renamed field would silently degrade'. Use isinstance(criterion, RunCommandCriterion). An alternative is to move the hint onto the model, for example an overridable property beside evaluable_after_agent_failure, so the orchestrator does not special-case a type at all. The impact is only a missing hint in not_evaluated details, so the severity is Low.
3. [Axis 3] The public ClassVar supports_post_failure_evaluation now disagrees with the instance answer for run_command (src/coder_eval/models/criteria.py:142) — After this change, supports_post_failure_evaluation: ClassVar[bool] = False stays public, and it is still False on RunCommandCriterion. But RunCommandCriterion(read_only=True).evaluable_after_agent_failure returns True (criteria.py:428-430 return self.read_only). The only reader of the ClassVar is now the base property at line 153 (return self.supports_post_failure_evaluation; confirmed with grep -rn supports_post_failure_evaluation src/ tests/). The contract now has two public names for 'can this run post-failure', and one of them gives a different answer for run_command. A future class-level consumer (a docs generator, a registry listing, a plugin checker) that reads the ClassVar would under-report run_command. Rename the ClassVar to a private or explicitly type-level name (for example _always_post_failure_safe), or say in its docstring that callers must use evaluable_after_agent_failure. The docstring change at line 143 only partly does this. There is no consumer today, so the severity is Low.
4. [Axis 4] Missing tests for read_only post-failure edge branches (failing command, non-run_command hint) (tests/test_timeout_orchestrator.py:484) — The new test checks only the passing case: assert declared.evaluation_status == "evaluated" / assert declared.score == 1.0 (lines 483-484). No test runs a read_only command that exits non-zero on the post-failure path. So nothing shows that a failing diagnostic is recorded as evaluated with score 0.0, and not as not_evaluated (which also has score 0.0). Nothing shows that a failing diagnostic keeps final_status, error_message and weighted_score unchanged. Recommendation: add a case with command="test -f missing.txt", read_only=True. Assert evaluation_status == "evaluated", score == 0.0, and that the details do not contain "Not evaluated after terminal agent failure". This keeps a real failing verdict separate from the not-evaluated placeholder.
5. [Axis 7] A read_only run_command on the post-failure path is not stopped by the task_timeout watchdog, so it can still run while teardown deletes the sandbox (src/coder_eval/orchestrator.py:922) — This is bounded, with one gap. Each read_only command is capped by its own timeout (timeout: int = Field(default=30, description="Timeout in seconds"), criteria.py:394). On expiry Sandbox.run_command catches subprocess.TimeoutExpired and returns (-1, "", error_msg), so it does not raise. When a checker does raise, the except Exception as recovery_error branch (orchestrator.py:848) records not_evaluated and then re-raises the original terminal error, so the original error stays visible. The gap: _evaluate_post_failure_criteria calls self.success_checker.check_all_async( (line 922), and that function runs sync checkers through await asyncio.to_thread(self._check_single, ...) (evaluation/checker.py:206). When the ThreadedWatchdog fires during post-failure grading, the except asyncio.CancelledError branch (line 836) records "the task_timeout budget expired during post-failure grading" and continues to _run_post_run_commands() and _cleanup(). The worker thread and its shell subprocess keep running for up to criterion.timeout seconds in a sandbox that cleanup is deleting. With no task_timeout set, the delay after a TurnTimeoutError or AgentCrashError is the sum of all read_only timeouts, with no cap for this path. Normal grading has the same to_thread behaviour, so the risk is not new. This PR only extends it: before, the post-failure path ran only file checks, and now it can run shell commands. Low-impact fix: in the TASK_DEFINITION_GUIDE read_only section, tell authors to keep the timeout of a read_only criterion short. Or, on the post-failure path only, clamp the per-command timeout (for example to the lesser of criterion.timeout and the time left before task_timeout). Evidence ceiling: I did not test the concurrent-cleanup overlap at runtime, so this finding stays at low severity.
6. [Axis 8] read_only Field description says the command runs on every failure path, but execute mode skips it; the text is also much longer and louder than sibling fields (src/coder_eval/models/criteria.py:411-425) — The sentence "Three failures reach that path ... The command runs on every one of them" overclaims. _evaluate_post_failure_criteria (orchestrator.py:903) returns early when self.grade is False, so under coder-eval execute a read_only command never runs after a crash, turn timeout or budget breach. A budget breach that fires after every criterion has already graded (orchestrator.py:828-832) also skips it. Qualify the sentence, for example: "on a graded run, when one of these stops grading before every criterion was scored". Separately, the description is about 900 characters, where sibling fields such as score_from_stdout and stdout_match are about 100-250. It also uses all-caps wording ("DECLARATION", "UNVERIFIED AND UNENFORCED") that no sibling uses. This text is copied verbatim into plugins/coder-eval/reference/criteria.md and the JSON schema. Cut it to the contract: what the field declares, its one effect, and that nothing verifies it. The guide paragraph in TASK_DEFINITION_GUIDE.md already covers the rest. Regenerate with make plugin-reference.

What's Missing

Tests:

• 🔵 No test covers a failing read_only command on the post-failure path. The only read_only test (tests/test_timeout_orchestrator.py:433) asserts the passing case (score 1.0). Nothing shows that a non-zero exit is recorded as evaluated with score 0.0, and not as the not_evaluated placeholder. (trigger: tests/test_timeout_orchestrator.py) (restates: Axis 4: Missing tests for read_only post-failure edge branches (failing command, non-run_command hint))
• 🔵 The read_only test drives only TurnTimeoutError. The agent-crash trigger (AgentCrashError, parametrized for the generic path at tests/test_timeout_orchestrator.py:349) and the documented budget-breach trigger have no read_only case. The budget case cannot be reached on the graded path. (trigger: tests/test_timeout_orchestrator.py) (restates: Axis 1: Budget-breach post-failure trigger is named in docs but no graded path reaches it and no test covers it)
• 🔵 No unit test pins the new public property BaseSuccessCriterion.evaluable_after_agent_failure across the SuccessCriterion union. Each non-run_command type must return its supports_post_failure_evaluation ClassVar, and run_command must follow read_only, which defaults to False. tests/test_success_criterion_union.py adds only a round-trip test for read_only (line 125). If a subclass overrides the property by mistake, only the two orchestrator integration tests would catch it, and only for file_exists and run_command. (trigger: src/coder_eval/models/criteria.py) (restates: Axis 3: The public ClassVar supports_post_failure_evaluation now disagrees with the instance answer for run_command)

Docs & config:

• 🔵 Three places say a token/cost budget breach is a post-failure trigger: docs/REPORT_SCHEMA.md:191, TASK_DEFINITION_GUIDE.md (example comment at :908 and table row at :923), and the read_only Field description, which is copied verbatim into plugins/coder-eval/reference/criteria.md:252. No graded path reaches it. After you correct the Field text, run make plugin-reference again. (trigger: docs/REPORT_SCHEMA.md) (restates: Axis 1: Budget-breach post-failure trigger is named in docs but no graded path reaches it and no test covers it)
• 🔵 The /coder-eval:task authoring skill (plugins/coder-eval/skills/task/SKILL.md:98) sends authors to run_command for script graders. It does not mention the new read_only opt-in or the rule 'never on live-service graders', so authors who use the plugin do not learn of the field. Add one line there. If the lint-tasks skill should flag read_only: true on a command that calls a live service such as 'uip ... debug', add that check too. (trigger: plugins/coder-eval/skills/task/SKILL.md)

Parallel paths & mirrors:

• 🔵 The design rationale (why read_only is an unverified author declaration, and why the gate moved from a ClassVar to a per-instance property) is in the ~900-char Field description and in the orchestrator docstring. It is not in .claude/notes/. grep shows that no note covers post-failure evidence (only orchestration.md:44 names it in passing). CLAUDE.md requires the 'why' of grading changes to go in .claude/notes/. Add a short post-failure section to orchestration.md or contracts.md, and cut the Field text down to the contract. (trigger: .claude/notes/orchestration.md) (restates: Axis 8: read_only Field description says the command runs on every failure path, but execute mode skips it; the text is also much longer and louder than sibling fields)

Rollout impact:

• 🔵 The PR does not state the version-skew impact. Criterion models use extra="forbid" (models/criteria.py:103), so a task YAML that sets read_only: true fails validation on any coder-eval release older than this one. The same applies to a pinned Docker image or an older runner in a consumer repo. If downstream task suites (for example coder_eval_uipath) adopt the field, they must pin coder-eval to at least the release that ships it. Say this in the PR or release note. (trigger: src/coder_eval/models/criteria.py)

Guardrails & Automation

Static checks (lint / type / repo-local):

• [typechecker] Set reportUnnecessaryComparison = "error" in [tool.pyright] in pyproject.toml, beside the existing 'Catch real logic bugs' settings. tests/lint/pyright_config.py copies every rule setting into the second pass, so the tests/ contract engine gets it too. SuccessCriterion is a union of classes that each have a Literal type tag. If the run_command tag is renamed, criterion.type == "run_command" has no overlap with the union, and pyright flags it as always False. I ran a probe pyright pass over src/coder_eval with this setting on. It reports 9 existing hits, all redundant is not None / != None checks on non-Optional types (orchestrator.py:1093, 2593, 2717, 2735; experiment.py:508, 834; batch.py:206; reports/html.py:273; and one more). Fix or # pyright: ignore those 9 in the same change. This covers the whole class: any comparison against a discriminator literal that stops being a member of the union. Prevents: orchestrator.py:868 _unavailable_reason compares criterion.type == "run_command" as a bare string. With this setting, renaming the tag gives a pyright error, and the 'declare read_only: true' hint cannot disappear without notice. The isinstance fix is still the preferred code change. This setting is the guard that catches every other place with the same pattern.
• [repo-local-check] Alternative or addition to the pyright setting: widen CE012 (tests/lint/rules/no_type_name_string_dispatch.py) or add CE068 (the next free id, per the note in tests/lint/runner.py). The rule flags <recv>.type ==/!=/in <str literal or tuple of str literals> in src/coder_eval/ when <recv> has a criterion, template-source or route name. Use the same receiver-name scoping that CE050 (ce050_no_union_getattr_probe.py) already uses, and take the literal set from the SuccessCriterion / TemplateSource / ApiRoute members at collection time. The message is: use isinstance narrowing. Allow # noqa: CE068. Today src has one hit (orchestrator.py:868). The other .type == sites (harbor/atif_*, packager.py, tasks.py:625) have receivers that are not criteria, so the rule does not flag them. Add a TestCE068 class in tests/test_custom_lint.py, and add the origin entry to .claude/notes/lint-rules.md. Prevents: orchestrator.py:868 string-tag dispatch. This rule applies the repo's stated 'isinstance narrowing, never string/getattr probing' convention (regrade.py:217, tasks.py:716) when the pattern is written, not only after a rename.
• [linter-rule] Remove the sharp edge, then guard it. Rename the ClassVar supports_post_failure_evaluation to a private, type-level name (for example _post_failure_safe_by_type) in models/criteria.py (the base and its 7 overrides), so evaluable_after_agent_failure is the only public name for this question. Then add SLF001 (flake8-self, private-member access) to the ruff select list, so a consumer outside the class hierarchy cannot read the private ClassVar. Before you enable SLF001, count its repo-wide hits. The rule applies to the whole repo, and I did not measure its noise. If the count is too high, use a per-file-ignores entry for tests/, or use a narrow CE rule that flags any .supports_post_failure_evaluation / ._post_failure_safe_by_type attribute read outside models/criteria.py. Prevents: criteria.py:142. The public ClassVar says False for RunCommandCriterion, but RunCommandCriterion(read_only=True).evaluable_after_agent_failure is True. A future class-level reader (a docs generator, a registry listing, a plugin) would under-report run_command.
• [repo-local-check] Extend tests/lint/prose_budget.py (run by make docs-budget, which make verify runs) to measure Field(description=<str literal>) in src/coder_eval/models/. Use the same 150-word prose cap that the file already applies to docstrings (_DOCSTRING_ESSAY_WORDS). This text is copied verbatim into plugins/coder-eval/reference/criteria.md and the JSON schema, so it is user-facing prose and needs the same no-essays rule. I measured the current descriptions. 8 of 527 are above 150 words: telemetry.py:225 (257), results.py:351 (228), tasks.py:498 (198), telemetry.py:290 (173), results.py:570 (168), results.py:323 (166), limits.py:102 (162), criteria.py:411 (151, the new read_only). The repo rule is that a cap goes below the current value, so trim those 8 in the same change, then run make plugin-reference. The rule cannot find the overclaim ('the command runs on every one of them'), because that needs semantic judgment. It does make the description short enough that a reviewer can check each claim. Prevents: criteria.py:411-425. The read_only description has about 900 characters and all-caps essay text copied into the plugin reference. The overclaim was inside that length.

Guardrail improvements (not statically reachable):

• Add a test in tests/test_timeout_orchestrator.py that drives the graded single-shot path into TOKEN/COST_BUDGET_EXCEEDED with a read_only: true run_command. Assert that the command is scored in success_criteria_results and that post_failure_criteria_results does not contain it (or is empty). Add a dialog-mode case that asserts RUN_LIMIT_EXCEEDED does not route through _evaluate_post_failure_criteria. Then fix REPORT_SCHEMA.md:191, TASK_DEFINITION_GUIDE.md:908/923 and the Field description to agree with the test: a budget breach happens after canonical grading. Why not static: Whether a budget breach can reach the post-failure path depends on runtime control flow (where _check_run_limits is called relative to check_all_async, and the re-raise guard len(results) == len(criteria)). A linter cannot compare that ordering with a prose claim in the docs. A behavioural test that pins the ordering is the only check that fails when the docs and the code disagree. Prevents: The docs/REPORT_SCHEMA.md:191 budget-breach trigger that no graded path reaches, and the test gap at tests/test_timeout_orchestrator.py:454 where only turn timeout is covered.
• Parametrize the new post-failure test in tests/test_timeout_orchestrator.py over (passing command, failing command test -f missing.txt, run_command without read_only, a non-run_command criterion that is not post-failure-safe). For the failing read_only case, assert evaluation_status == "evaluated", score == 0.0, details without 'Not evaluated after terminal agent failure', and final_status / error_message / weighted_score the same as a run without the criterion. For the non-read_only cases, assert that the not_evaluated reason has the 'declare read_only: true' hint only for run_command. That also gives the orchestrator.py:868 branch a behavioural pin. Why not static: To tell a real failing verdict (evaluated, score 0.0) from the not-evaluated placeholder (also score 0.0), you must run the checker and read the recorded status. The two results have the same type and differ only in runtime values. Branch coverage of this kind is a test responsibility. The repo's 80% coverage gate is repo-wide and does not require these specific branches. Prevents: Missing tests for the read_only failing-command branch and the non-run_command hint branch (tests/test_timeout_orchestrator.py:484, :415).
• On the post-failure path only, clamp each read_only command's timeout to min(criterion.timeout, time left before task_timeout), with a small floor. Add an orchestrator test in which a sleep-based read_only command has a timeout longer than the time left before task_timeout. Assert that the subprocess stops before _cleanup() removes the sandbox (for example, the command's marker file is never written after cleanup, or the recorded duration is bounded). If no clamp is added, add one sentence to the TASK_DEFINITION_GUIDE read_only section that tells authors to keep a read_only criterion's timeout short. Why not static: The defect is a runtime concurrency overlap: an asyncio.to_thread worker and its shell subprocess keep running after the ThreadedWatchdog cancels the awaiting coroutine and while teardown runs. A static check could only enforce a fixed cap on timeout when read_only=True (a schema validator). That would reject valid long diagnostics and still not bound the sum across several criteria. Only a runtime clamp with a timing test shows the bound. Prevents: orchestrator.py:922. A read_only run_command on the post-failure path can run past the task_timeout watchdog while cleanup deletes the sandbox.

Top 5 Priority Actions

1. Fix the budget-breach claim at docs/REPORT_SCHEMA.md:191, docs/TASK_DEFINITION_GUIDE.md:908/:923 and src/coder_eval/models/criteria.py:419: a budget breach fires only after canonical grading, so no read_only post_failure_criteria_results row is ever written for it. Either remove the clause or say that the command is already graded in success_criteria_results.
2. Shorten the read_only Field description at src/coder_eval/models/criteria.py:411-425 to the contract only (what it declares, its one effect, and that nothing verifies it). Say that the command runs only on graded runs, where a crash or turn timeout stops grading early, so not under execute. Then regenerate the published reference with make plugin-reference.
3. Replace criterion.type == "run_command" at src/coder_eval/orchestrator.py:868 with isinstance(criterion, RunCommandCriterion), as regrade.py:217 and tasks.py:716 do, so that a renamed tag cannot silently drop the 'declare read_only: true' hint.
4. Bound the post-failure read_only command in src/coder_eval/orchestrator.py:922. On this path, clamp each command's timeout to the lesser of criterion.timeout and the time left before task_timeout, or at minimum tell authors in the guide to keep read_only timeouts short. The aim is that a worker thread cannot keep running in a sandbox that _cleanup is deleting.
5. Add tests to tests/test_timeout_orchestrator.py (near :454/:484). Add a failing read_only command (test -f missing.txt) that asserts evaluation_status == 'evaluated', score 0.0, no 'Not evaluated' placeholder, and final_status/weighted_score unchanged. Add a non-run_command hint branch. Also make the private-vs-instance contract clear: rename the ClassVar supports_post_failure_evaluation at src/coder_eval/models/criteria.py:142 or send callers to evaluable_after_agent_failure.

---

Change class: complex — adds a public schema field that changes which criteria execute sandbox commands on the post-failure recovery path

Stats: 0 🔴 · 0 🟠 · 0 🟡 · 6 🔵 across 5 axes reviewed.
Verification: 0 medium+ finding(s) adversarially re-checked · 0 dropped as false positives · 0 corrected in place · 8 low passed through unverified.

[uipreliga]

Fix what you agree with and 🚢

[rockymadden]

Thanks @uipreliga — this caught a real error, not a style point. Responses to all five priority actions, in e5f2bfe.

1. Budget-breach claim 🟢 taken, and it was my mistake

You are right and I verified the control flow myself rather than take it on trust:

• check_all_async at orchestrator.py:2275 fills the complete vector; _check_run_limits raises at :2300, deliberately after ("AFTER the criteria, so partial-credit visibility is preserved"). The guard at :828-832 therefore always re-raises.
• execute: _evaluate_post_failure_criteria returns early on not self.grade.
• Dialog: :2626-2627 catches BudgetExceededError locally.

The clause came from a Copilot finding I accepted after confirming only that the error type was in the terminal tuple at :827. Being in the tuple is not reaching the path. Removed from all four places. REPORT_SCHEMA.md now states the list stays empty under execute and on a budget breach (already scored canonically), and _evaluate_post_failure_criteria's docstring records why, so the claim cannot come back by the same route.

2. Field description 🟢 taken

~900 chars → 320, all-caps gone, execute overclaim gone:

Declares this command an artifact-only check: it writes nothing and reaches no live service. Its one effect is that a graded run also runs it after an agent crash or turn timeout, on the diagnostic path, where the result is recorded but never scored. Nothing verifies the declaration; see the Task Definition Guide for when to set it.

make plugin-reference regenerated. The rationale you noted was missing from .claude/notes/ is now a short Post-failure evidence is declared, not proven section in orchestration.md — which is where it belonged, not in a Field description.

3. isinstance 🟢 taken

isinstance(criterion, RunCommandCriterion), matching regrade.py:217 and tasks.py:716. I considered your alternative of moving the hint onto the model and rejected it: the string is a message about the orchestrator's recovery path, not criterion data, and putting it on the model adds a public surface to every criterion class to carry one diagnostic sentence.

4. Post-failure timeout 🟡 guidance taken, clamp declined

Your framing changed my answer — I had rejected this from Copilot as pre-existing, which was the wrong end of the argument. The part that is genuinely uncapped is yours: with no task_timeout, the pass is bounded only by the sum of every read_only timeout.

I declined the clamp for a specific reason: min(criterion.timeout, time left before task_timeout) needs a deadline, and in the unbounded case there is none — it does nothing exactly where the exposure is worst. It also needs an arbitrary floor and would silently apply a timeout other than the one the criterion declares. So the guide now says what an author can act on:

Keep a read_only criterion's timeout short. The diagnostic pass runs after the agent is gone, and the commands are not interruptible: a task_timeout that expires mid-pass cancels the await, not the shell subprocess, so it keeps running while the sandbox is torn down. Without a task_timeout the pass is bounded only by the sum of these timeouts.

Cancellation-aware Sandbox.run_command is still the real fix, still its own PR, and it closes the normal path too.

5. Tests + the ClassVar contract 🟢 taken

• The post-failure test now carries a failing read_only command (test -f missing.txt): evaluation_status == "evaluated", score == 0.0, and no "Not evaluated after terminal agent failure" placeholder — the distinction that two 0.0 scores otherwise hide.
• The non-run_command hint branch: an llm_judge gets not_evaluated without the read_only: true hint, pinning _unavailable_reason both ways.
• test_post_failure_property_tracks_the_type_answer, parametrized over every MINIMAL_PAYLOADS member: evaluable_after_agent_failure is supports_post_failure_evaluation for all of them at their defaults. A subclass that overrides the property by mistake now fails.
• ClassVar contract: documented rather than renamed. Its docstring now says it is type-level only, cannot answer for an instance, and that every caller asks the property. Renaming to _post_failure_safe_by_type touches the base plus 7 subclasses for a hazard with no consumer today; the parametrized test above is the part that actually catches a divergence.

Also added: one line in plugins/coder-eval/skills/task/SKILL.md so plugin authors learn the field and the never-on-live-service rule, and a version-skew note in the PR body (extra="forbid" means read_only: true fails validation on older releases).

Guardrails — not in this PR

Agreed they are follow-ups, and your own measurements are why: 9 existing reportUnnecessaryComparison hits and 8 over-length descriptions each need fixing in the same change, which would bury a default-false opt-in.

Ranked by what I think they are worth:

1. reportUnnecessaryComparison = "error" — highest value, since it covers every discriminator-literal comparison rather than the one I just fixed, and the 9 hits are all redundant is not None.
2. The Field(description=...) prose budget in tests/lint/prose_budget.py — this text ships verbatim into the plugin reference and the JSON schema, and the overclaim hid inside the length. A cap would not have found it, but it would have made it checkable.
3. CE068 — largely subsumed by (1) if that lands; worth it only if the pyright setting proves too noisy.
4. SLF001 / renaming the ClassVar — lowest; no consumer today, and the unmeasured repo-wide noise is the whole risk.

Happy to open (1) and (2) as separate PRs.

Verification

uv run pytest -n auto -m "not live and not lint" tests/   → 1 failed, 6091 passed, 2 skipped
uv run pytest tests/test_custom_lint.py                   → 677 passed
uv run ruff format --check / ruff check / pyright         → clean (0 errors)

The one failure is test_docker_runner_mounts.py::TestOutputMountWidenedBeforeLaunch, pre-existing and macOS-host-only — it fails identically on a stashed tree and passes in CI.