diff --git a/.claude/notes/orchestration.md b/.claude/notes/orchestration.md index a37c65882..7a65e2241 100644 --- a/.claude/notes/orchestration.md +++ b/.claude/notes/orchestration.md @@ -108,6 +108,23 @@ VERDICT, never the facts — the seeding cannot restore a fact the execute phase captured. The budget gate runs AFTER the criteria on the graded path purely for partial-credit visibility, and there is no partial credit under `execute`. +### Post-failure evidence is declared, not proven + +A timed-out run used to score 0.00 on artifacts that every grader accepted, because the +whole uipath-maestro-flow suite grades through `run_command` and the path ran only file +checks. `read_only: true` admits a single named command. + +The gate could not stay a ClassVar: `run_command` is safe or unsafe per criterion, not +per type, and no analysis decides which — an arbitrary shell command can start a cloud +job (`uip maestro flow debug` does). So the flag is the AUTHOR's claim, the ClassVar +stayed the type-level answer, and `evaluable_after_agent_failure` became the per-instance +one every caller asks. Widening the flag to the base would let a judge declare itself +deterministic, which is a different and false claim. + +What keeps it honest: results land in `post_failure_criteria_results`, which +`calculate_weighted_score` never reads. A timed-out run stays ERROR at 0.0 no matter what +the diagnostic pass finds — the flag buys evidence, never a verdict. + ### Rates need verdict evidence, not bucket counts A published rate divides by rows that actually carry a verdict, not by a bucket count. The diff --git a/docs/REPORT_SCHEMA.md b/docs/REPORT_SCHEMA.md index 472287d9b..f7002e3ec 100644 --- a/docs/REPORT_SCHEMA.md +++ b/docs/REPORT_SCHEMA.md @@ -188,14 +188,17 @@ fields so subclass keys round-trip. ### Post-failure criterion evidence -When an agent crashes or its turn times out, coder-eval runs only deterministic, -read-only artifact criteria while the sandbox is still live: `file_exists`, +When an agent crashes or its turn times out on a graded run, coder-eval runs only +deterministic, read-only artifact criteria while the sandbox is still live: `file_exists`, `file_contains`, `file_matches_regex`, `file_check`, `json_check`, -`reference_comparison`, and `classification_match`. Judges, trajectory checks, -`run_command`, and `uipath_eval` are recorded with -`evaluation_status="not_evaluated"`; they are not invoked on this recovery path. +`reference_comparison`, and `classification_match`. A `run_command` criterion joins them +only when the task author sets [`read_only: true`](TASK_DEFINITION_GUIDE.md#run_command) +on it. Judges, trajectory checks, plain `run_command`, and `uipath_eval` are recorded +with `evaluation_status="not_evaluated"`; they are not invoked on this recovery path. The diagnostic list is additive evidence. An `ERROR` run remains `ERROR`, and its -canonical score remains 0.0. +canonical score remains 0.0. The list stays empty under `coder-eval execute`, and on a +token/cost budget breach, which fires only after every criterion is already scored in +`success_criteria_results`. ### TurnRecord diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index d0a300103..59d613a81 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -904,6 +904,12 @@ Runs a command and checks the exit code, with optional stdout matching. **Binary expected_stdout: "Hello, World!" # Optional: check stdout content stdout_match: "exact" # "exact" (default), "contains", or "regex" description: "Script must output the correct text" + +# Also graded when the turn times out or the agent crashes +- type: "run_command" + command: "python graders/check_flow.py" + read_only: true # declaration, not enforcement -- see below + description: "Flow must contain the approval node" ``` | Field | Default | Description | @@ -914,6 +920,19 @@ Runs a command and checks the exit code, with optional stdout matching. **Binary | `expected_stdout` | `null` | When set, stdout is also checked | | `stdout_match` | `"exact"` | Match mode: `exact` (stripped), `contains` (substring), `regex` (pattern) | | `score_from_stdout` | `false` | Read a float score (0.0–1.0) from the first stdout line (remaining lines become details); a non-zero exit code or a parse failure scores 0.0. Mutually exclusive with `expected_stdout`. | +| `read_only` | `false` | Declares the command inspects artifacts only. Its sole effect: a graded run also runs the criterion after a turn timeout or an agent crash — see [Post-failure criterion evidence](REPORT_SCHEMA.md#post-failure-criterion-evidence). | + +`read_only` is an author declaration, **not** a restriction. coder-eval cannot decide +whether a shell command is pure, so it verifies nothing and confines nothing: the +command runs exactly as it always does. Set it only when the command reads artifacts and +nothing else. Leave it `false` when the command writes state or calls a live service (a +`uip maestro flow debug` grader starts a real cloud job, so it must stay `false`). + +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. ### `file_matches_regex` diff --git a/plugins/coder-eval/reference/criteria.md b/plugins/coder-eval/reference/criteria.md index 4267c0e57..526e1fbdd 100644 --- a/plugins/coder-eval/reference/criteria.md +++ b/plugins/coder-eval/reference/criteria.md @@ -249,6 +249,7 @@ Optional: | `expected_stdout` | Expected stdout content. When set, stdout is also checked. | | `stdout_match` | How to match stdout: 'exact' (stripped), 'contains' (substring), 'regex' (pattern) | | `score_from_stdout` | When true, read a float score (0.0-1.0) from the first line of stdout. Remaining lines are captured as details. Non-zero exit code or parse failure -> score 0.0. Mutually exclusive with expected_stdout. | +| `read_only` | 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. | ### `skill_triggered` diff --git a/plugins/coder-eval/skills/task/SKILL.md b/plugins/coder-eval/skills/task/SKILL.md index 7226cdeaa..440de214f 100644 --- a/plugins/coder-eval/skills/task/SKILL.md +++ b/plugins/coder-eval/skills/task/SKILL.md @@ -120,6 +120,10 @@ Rules that matter: filename — a criterion matching that literal is a **smoke check**, not evidence: it only proves the agent typed back what it was told. Keep it if you like, at a low weight, and put the weight on a criterion that checks the resulting *behaviour*. +- Set `read_only: true` on a `run_command` grader that only inspects artifacts: a run + whose agent crashed or timed out then still records what the artifacts were worth, + instead of scoring nothing. Never set it on a command that writes state or calls a + live service — nothing verifies the claim, and the command runs unchanged. - `weight` reflects importance: `0.5` nice-to-have, `1.0` standard, `1.5`–`2.0` critical. `weight: 0` makes a criterion informational (reported, but excluded from the score and the pass/fail gate). diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index e4ee9764d..1e661aa73 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -140,7 +140,19 @@ class BaseSuccessCriterion(BaseModel, ABC): """True if this criterion requires agent turn records to evaluate correctly.""" supports_post_failure_evaluation: ClassVar[bool] = False - """True for deterministic, read-only artifact checks safe to run after agent failure.""" + """Type-level only: True for criterion TYPES that are always deterministic, read-only + artifact checks. It cannot answer for an instance -- ``run_command`` decides per + criterion -- so every caller asks ``evaluable_after_agent_failure`` instead.""" + + @property + def evaluable_after_agent_failure(self) -> bool: + """True when THIS criterion is safe to run after a terminal agent failure. + + The per-instance answer the orchestrator asks, so a type whose safety + depends on how the criterion is authored (``run_command``) can decide it + from its own fields instead of from the class. + """ + return self.supports_post_failure_evaluation @property def is_stop_armed(self) -> bool: @@ -398,6 +410,20 @@ class RunCommandCriterion(BaseSuccessCriterion): "Mutually exclusive with expected_stdout." ), ) + read_only: bool = Field( + default=False, + description=( + "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." + ), + ) + + @property + def evaluable_after_agent_failure(self) -> bool: + """Author-declared, not proven -- see ``read_only``.""" + return self.read_only @model_validator(mode="after") def check_score_from_stdout_exclusivity(self) -> RunCommandCriterion: diff --git a/src/coder_eval/orchestrator.py b/src/coder_eval/orchestrator.py index eccfd7f15..c10f1deb2 100644 --- a/src/coder_eval/orchestrator.py +++ b/src/coder_eval/orchestrator.py @@ -54,6 +54,7 @@ PreRunCommand, PreservationMode, ReferenceComparisonCriterion, + RunCommandCriterion, SimulationConfig, SimulationTelemetry, SuccessCriterion, @@ -862,6 +863,13 @@ def _post_failure_exception_reason(error: Exception) -> str: suffix = f": {message}" if message else "" return f"post-failure grading could not complete ({type(error).__name__}{suffix})" + @staticmethod + def _unavailable_reason(criterion: SuccessCriterion) -> str: + reason = "the criterion is not a deterministic, read-only artifact check" + if isinstance(criterion, RunCommandCriterion): + reason += " (declare 'read_only: true' on it if the command only inspects artifacts)" + return reason + @staticmethod def _not_evaluated_result(criterion: SuccessCriterion, reason: str) -> CriterionResult: return CriterionResult( @@ -886,8 +894,14 @@ async def _evaluate_post_failure_criteria(self) -> None: """Evaluate diagnostic criteria before the live sandbox is torn down. Results stay outside the canonical scored list. Only criteria that - declare themselves deterministic and read-only run on this path. This - excludes judges and checks that execute sandbox commands. + declare themselves deterministic and read-only run on this path. That + excludes judges and trajectory checks, and every ``run_command`` + criterion except one the task author marked ``read_only`` -- which DOES + execute a sandbox command here. + + Reached from an agent crash or a turn timeout only. A budget breach + raises after the canonical vector is already complete, so its guard in + ``_run_evaluation_with_failure_evidence`` re-raises before this runs. """ if self.result is None: return @@ -903,7 +917,7 @@ async def _evaluate_post_failure_criteria(self) -> None: runnable: list[SuccessCriterion] = [] unavailable_positions: set[int] = set() for position, criterion in enumerate(self.task.success_criteria): - if not criterion.supports_post_failure_evaluation: + if not criterion.evaluable_after_agent_failure: unavailable_positions.add(position) else: runnable.append(criterion) @@ -925,12 +939,7 @@ async def _evaluate_post_failure_criteria(self) -> None: recovered: CriteriaResults = [] for position, criterion in enumerate(self.task.success_criteria): if position in unavailable_positions: - recovered.append( - self._not_evaluated_result( - criterion, - "the criterion is not a deterministic, read-only artifact check", - ) - ) + recovered.append(self._not_evaluated_result(criterion, self._unavailable_reason(criterion))) else: recovered.append(next(checked_iter)) self.result.post_failure_criteria_results = recovered diff --git a/tests/test_success_criterion_union.py b/tests/test_success_criterion_union.py index 3cde2e9df..778a4dea9 100644 --- a/tests/test_success_criterion_union.py +++ b/tests/test_success_criterion_union.py @@ -122,6 +122,31 @@ def test_model_dump_exclude_unset_round_trip(): assert round_tripped.success_criteria[0].type == "file_exists" +def test_read_only_survives_exclude_unset_round_trip(): + """``read_only`` gates post-failure grading, so a dropped flag silently loses a score.""" + task = _make_task([{"type": "run_command", "description": "d", "command": "true", "read_only": True}]) + + round_tripped = TaskDefinition.model_validate(task.model_dump(exclude_unset=True)) + + criterion = round_tripped.success_criteria[0] + assert criterion.read_only is True + assert criterion.evaluable_after_agent_failure is True + + +def test_run_command_is_not_post_failure_evaluable_by_default(): + task = _make_task([{"type": "run_command", "description": "d", "command": "true"}]) + + assert task.success_criteria[0].evaluable_after_agent_failure is False + + +@pytest.mark.parametrize("tag", sorted(MINIMAL_PAYLOADS)) +def test_post_failure_property_tracks_the_type_answer(tag: str): + """Only ``run_command`` may diverge from its ClassVar, and only via ``read_only``.""" + criterion = _make_task([{"type": tag, **MINIMAL_PAYLOADS[tag]}]).success_criteria[0] + + assert criterion.evaluable_after_agent_failure is criterion.supports_post_failure_evaluation + + def test_validate_registry_passes(): CriterionRegistry.discover() validate_registry() diff --git a/tests/test_timeout_orchestrator.py b/tests/test_timeout_orchestrator.py index 080113c79..ec0565787 100644 --- a/tests/test_timeout_orchestrator.py +++ b/tests/test_timeout_orchestrator.py @@ -429,6 +429,89 @@ async def cleanup() -> None: ] +@pytest.mark.asyncio +async def test_read_only_run_command_is_graded_after_a_terminal_agent_error(tmp_path) -> None: + """A declared read-only grader records artifact truth without rescuing the run.""" + task = _make_task(turn_timeout=1200, task_timeout=1500) + task.success_criteria = [ + RunCommandCriterion( + type="run_command", + command="test -f artifact.txt", + read_only=True, + description="declared read-only grader", + ), + RunCommandCriterion( + type="run_command", + command="test -f missing.txt", + read_only=True, + description="declared read-only grader that fails", + ), + RunCommandCriterion( + type="run_command", + command="touch should-not-run", + description="undeclared sandbox command", + ), + LLMJudgeCriterion( + type="llm_judge", + prompt="Grade the artifact.", + description="paid judge", + ), + ] + run_dir = tmp_path / "run" / "read_only_post_failure" + run_dir.mkdir(parents=True) + orchestrator = Orchestrator(task=task, run_dir=run_dir, variant_id="test-variant") + orchestrator._setup = AsyncMock() # type: ignore[method-assign] + orchestrator._refresh_runtime_tool_versions = MagicMock() # type: ignore[method-assign] + terminal_error = TurnTimeoutError(1200, task_id=task.task_id, iteration=1) + orchestrator._evaluation_loop = AsyncMock(side_effect=terminal_error) # type: ignore[method-assign] + + sandbox = Sandbox(SandboxConfig(driver="tempdir"), task_id=task.task_id) + sandbox_dir = sandbox.setup() + (sandbox_dir / "artifact.txt").write_text("finished", encoding="utf-8") + orchestrator.sandbox = sandbox + orchestrator.success_checker = SuccessChecker(sandbox) + + # Recorded before teardown removes the directory; asserting inside `cleanup` + # would hide the failure in the orchestrator's finally block. + marker_seen: list[bool] = [] + + async def cleanup() -> None: + marker_seen.append((sandbox_dir / "should-not-run").exists()) + sandbox.cleanup() + + orchestrator._cleanup = cleanup # type: ignore[method-assign] + + mock_agent = MagicMock() + mock_agent.kill_sync = MagicMock() + mock_agent.get_sdk_options = MagicMock(return_value=None) + orchestrator.agent = mock_agent + + result = await orchestrator.run() + + assert marker_seen == [False], "an undeclared run_command must not execute on this path" + + passed, failed, undeclared, judge = result.post_failure_criteria_results + assert passed.evaluation_status == "evaluated" + assert passed.score == 1.0 + + # A real failing verdict, NOT the not_evaluated placeholder -- both score 0.0. + assert failed.evaluation_status == "evaluated" + assert failed.score == 0.0 + assert "Not evaluated after terminal agent failure" not in (failed.details or "") + + # The opt-in hint belongs only to the type that has the opt-in. + assert undeclared.evaluation_status == "not_evaluated" + assert "read_only: true" in (undeclared.details or "") + assert judge.evaluation_status == "not_evaluated" + assert "read_only: true" not in (judge.details or "") + + # The evidence is additive: the run is still the failure it was. + assert result.final_status == "ERROR" + assert result.error_message == str(terminal_error) + assert result.weighted_score == 0.0 + assert result.success_criteria_results == [] + + @pytest.mark.parametrize( "recovery_error", [