From 45768b74e61c190a82126fe2ae8e757112926867 Mon Sep 17 00:00:00 2001 From: tmatup <51425734+tmatup@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:31:03 +0000 Subject: [PATCH] feat(evaluation): preserve criteria after agent failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit πŸ€– Generated with Codex Co-Authored-By: [Codex](mailto:noreply@openai.com) --- src/coder_eval/cli/plan_command.py | 5 + .../evaluation/judge_persistence.py | 93 +++++---- src/coder_eval/models/limits.py | 3 + src/coder_eval/models/results.py | 26 ++- src/coder_eval/orchestration/run_limits.py | 34 ++++ src/coder_eval/orchestrator.py | 177 ++++++++++++++++-- tests/test_cost_accounting_paths.py | 20 ++ tests/test_criterion_result_round_trip.py | 49 +++++ tests/test_judge_persistence.py | 31 ++- tests/test_plan_command.py | 30 ++- tests/test_run_limits_models.py | 35 ++++ tests/test_run_limits_orchestrator.py | 2 + tests/test_timeout_orchestrator.py | 117 ++++++++++++ 13 files changed, 556 insertions(+), 66 deletions(-) create mode 100644 src/coder_eval/orchestration/run_limits.py diff --git a/src/coder_eval/cli/plan_command.py b/src/coder_eval/cli/plan_command.py index 264d863d..25318717 100644 --- a/src/coder_eval/cli/plan_command.py +++ b/src/coder_eval/cli/plan_command.py @@ -61,6 +61,7 @@ def plan_command( # Lazy import to avoid circular dependency at module level from ..orchestration.early_stop import EarlyStopConfigError, validate_early_stop from ..orchestration.experiment import DEFAULT_EXPERIMENT_PATH, load_experiment, resolve_task_for_variant + from ..orchestration.run_limits import validate_run_limits # Always load experiment (defaults to experiments/default.yaml) exp_path = experiment if isinstance(experiment, Path) else DEFAULT_EXPERIMENT_PATH @@ -136,6 +137,10 @@ def plan_command( resolved, _lineage, _ = resolve_task_for_variant(default_exp, task, exp_def, variant) # Early-stop guardrails (no-op unless a criterion carries a stop_early: block). validate_early_stop(resolved) + for message in validate_run_limits(resolved): + console.print( + f" [yellow]⚠[/yellow] [yellow]Variant '{variant.variant_id}': {message}[/yellow]" + ) agent_type = str(resolved.agent.type) if resolved.agent else "unknown" agent_model = resolved.agent.model if resolved.agent else None model_str = f" ({agent_model})" if agent_model else "" diff --git a/src/coder_eval/evaluation/judge_persistence.py b/src/coder_eval/evaluation/judge_persistence.py index d600cfb7..10aefcf0 100644 --- a/src/coder_eval/evaluation/judge_persistence.py +++ b/src/coder_eval/evaluation/judge_persistence.py @@ -3,9 +3,9 @@ The full judge transcript (tool calls, raw verdict, rendered prompt and system prompt) can run 10-100 KB. Inlining it into every ``task.json`` inflates the row record for consumers (suite rollups, report renderers) -that don't need it. Spilling each transcript to a sibling -``judge-.yaml`` next to ``task.json`` keeps the row record lean and -lets reviewers grep transcripts independently. +that don't need it. Spilling each transcript to a sibling YAML file next to +``task.json`` keeps the row record lean and lets reviewers grep transcripts +independently. YAML (over JSON) for the sibling: the transcript carries multi-line text (``judge_prompt``, ``judge_system_prompt``, ``raw_verdict``) which YAML's @@ -121,50 +121,48 @@ def _ordered_transcript_dict(transcript_dump: dict[str, Any]) -> dict[str, Any]: def spill_judge_transcripts(result: EvaluationResult, output_dir: Path) -> int: """Write each judge result's inline transcript to a sibling YAML file. - For each ``JudgeCriterionResult`` in ``result.success_criteria_results`` - that carries a non-None ``transcript``, writes ``judge-.yaml`` in - ``output_dir`` (creating the directory if needed) and sets + For each ``JudgeCriterionResult`` in the canonical or post-failure result + list that carries a non-None ``transcript``, writes a distinct sibling YAML + file in ``output_dir`` (creating the directory if needed) and sets ``transcript_path`` on the result to the sibling filename. The inline ``transcript`` is **left in place** so in-memory consumers (HTML rendering at the end of the orchestrator run) still see it. - Callers writing ``task.json`` should pass - ``exclude={"success_criteria_results": {"__all__": {"transcript"}}}`` - to ``model_dump_json`` so the on-disk record carries only the path. + Callers writing ``task.json`` should exclude ``transcript`` from both result + lists so the on-disk record carries only the path. Returns the count of transcripts spilled (informational; no-op when 0). """ output_dir.mkdir(parents=True, exist_ok=True) spilled = 0 - # ORDER IS LOAD-BEARING. ``judge-{idx}.yaml`` is keyed off the criterion's - # position in ``success_criteria_results``; ``load_judge_transcripts`` reads - # ``transcript_path`` (which we set below) to find each sibling, so the - # filename naming scheme itself can change freely. What MUST stay stable is - # the indexβ†’file mapping for the lifetime of any task.json that references - # these siblings: writers that reorder ``success_criteria_results`` between - # spill and read would break the binding. Today's only writer is the - # orchestrator and the order is preserved through model_dump_json/ - # model_validate_json, so this is safe β€” keep it that way. - for idx, cr in enumerate(result.success_criteria_results): - if not isinstance(cr, JudgeCriterionResult): - continue - if cr.transcript is None: - continue - sibling_name = f"judge-{idx}.yaml" - sibling_path = output_dir / sibling_name - ordered = _ordered_transcript_dict(cr.transcript.model_dump()) - sibling_path.write_text( - yaml.dump( - ordered, - Dumper=_BlockLiteralDumper, - sort_keys=False, - allow_unicode=True, - width=100, - ), - encoding="utf-8", - ) - cr.transcript_path = sibling_name - spilled += 1 + # ORDER IS LOAD-BEARING. Each filename is keyed off the criterion's + # position in its result list; ``load_judge_transcripts`` reads the stored + # path, so each list must retain its order through persistence. + result_groups = ( + ("judge", result.success_criteria_results), + ("post-failure-judge", result.post_failure_criteria_results), + ) + for prefix, criteria_results in result_groups: + for idx, cr in enumerate(criteria_results): + if not isinstance(cr, JudgeCriterionResult): + continue + if cr.transcript is None: + continue + sibling_name = f"{prefix}-{idx}.yaml" + sibling_path = output_dir / sibling_name + ordered = _ordered_transcript_dict(cr.transcript.model_dump()) + sibling_path.write_text( + yaml.dump( + ordered, + Dumper=_BlockLiteralDumper, + sort_keys=False, + allow_unicode=True, + width=100, + ), + encoding="utf-8", + ) + cr.transcript_path = sibling_name + spilled += 1 if spilled: logger.debug("spilled %d judge transcript(s) to %s", spilled, output_dir) return spilled @@ -173,11 +171,11 @@ def spill_judge_transcripts(result: EvaluationResult, output_dir: Path) -> int: def load_judge_transcripts(result: EvaluationResult, task_dir: Path) -> int: """Read sibling judge transcript files and attach them to each result. - For each criterion result in ``result.success_criteria_results`` that has - a ``transcript_path`` set (and no inline ``transcript`` β€” already-loaded + For each criterion result in either result list that has a + ``transcript_path`` set (and no inline ``transcript`` β€” already-loaded results are left alone), reads the sibling file relative to ``task_dir`` - and attaches the parsed dict on ``transcript`` so HTML / markdown - renderers see the same shape they get during the original run. + and attaches the parsed dict on ``transcript`` so HTML / markdown renderers + see the same shape they get during the original run. Missing sibling files are skipped silently and logged at debug level β€” runs predating this feature have no sibling files and render fine via @@ -187,7 +185,8 @@ def load_judge_transcripts(result: EvaluationResult, task_dir: Path) -> int: Returns the count of transcripts loaded. """ loaded = 0 - for cr in result.success_criteria_results: + criterion_results = result.success_criteria_results + result.post_failure_criteria_results + for cr in criterion_results: path = getattr(cr, "transcript_path", None) if not path: continue @@ -198,8 +197,8 @@ def load_judge_transcripts(result: EvaluationResult, task_dir: Path) -> int: continue # SECURITY: transcript_path comes from task.json, which may travel across # trust boundaries (CI artifacts, shared eval bundles). spill_judge_transcripts - # only ever writes the literal ``f"judge-{idx}.yaml"`` β€” a basename, no - # separators, no ``..``. Allowlist the basename shape directly so a tampered + # only ever writes generated basename-only paths, with no separators or + # ``..``. Allowlist that shape directly so a tampered # ``transcript_path: '/etc/passwd'`` or ``../../secrets`` is refused at the # door rather than relying on ``is_relative_to`` to catch it after a join. # Check BOTH PurePosixPath (forward-slash separator) AND PureWindowsPath @@ -275,8 +274,8 @@ def load_judge_transcripts(result: EvaluationResult, task_dir: Path) -> int: # which (depending on model_config of the loaded subclass) might # validate or reject. The HTML renderer accepts both typed # JudgeTranscript and dict-shape so either shape works downstream. - # NOTE: With the ``CriterionResultUnion`` discriminator on - # ``EvaluationResult.success_criteria_results``, ``cr`` is now a + # NOTE: With the ``CriterionResultUnion`` discriminator on both + # ``EvaluationResult`` criterion-result lists, ``cr`` is now a # properly-typed ``JudgeCriterionResult`` after reload (not a base # ``CriterionResult`` with the field in ``__pydantic_extra__``), so # the assignment lands on the declared field directly. diff --git a/src/coder_eval/models/limits.py b/src/coder_eval/models/limits.py index e6febfea..6cafb5f6 100644 --- a/src/coder_eval/models/limits.py +++ b/src/coder_eval/models/limits.py @@ -154,3 +154,6 @@ class RunLimits(BaseModel): # plan_command's generic per-variant "resolution failed" branch, which # prints red text but does NOT flip the exit code by design (unlike # EarlyStopConfigError), so a model-level raise would silently pass CI. + # Other cross-field semantics that are warnings rather than errors live in + # orchestration/run_limits.py::validate_run_limits for the same post-merge + # visibility without rejecting or mutating the resolved values. diff --git a/src/coder_eval/models/results.py b/src/coder_eval/models/results.py index 2f03222e..212ab667 100644 --- a/src/coder_eval/models/results.py +++ b/src/coder_eval/models/results.py @@ -82,6 +82,15 @@ class CriterionResult(BaseModel): ) details: str | None = Field(default=None, description="Additional details about the result") error: str | None = Field(default=None, description="Error message if the check failed") + evaluation_status: Literal["evaluated", "not_evaluated"] = Field( + default="evaluated", + description=( + "Whether the criterion ran. ``not_evaluated`` is distinct from an evaluated " + "criterion whose score is 0.0 or whose checker returned an error. Defaults to " + "``evaluated`` so task.json files written before this field existed retain their " + "original meaning." + ), + ) pass_threshold: float = Field( default=0.9, ge=0.0, @@ -531,6 +540,16 @@ class EvaluationResult(BaseModel): "files without ``result_kind`` are inferred from ``criterion_type``." ), ) + post_failure_criteria_results: list[CriterionResultUnion] = Field( + default_factory=list, + description=( + "Diagnostic criterion evidence collected after a terminal agent failure while the " + "sandbox is still readable. These results are intentionally separate from " + "success_criteria_results: they do not affect weighted_score, task gating, or suite " + "aggregation. A result with evaluation_status='not_evaluated' records that its " + "required inputs or remaining task-timeout budget were unavailable." + ), + ) # Detailed transcript iterations: list[TurnRecord] = Field( @@ -952,9 +971,12 @@ def judge_cost_usd(result: EvaluationResult) -> float | None: Covers both flavors: ``llm_judge`` prices its own one-shot call from the criterion's model, ``agent_judge`` inherits the SDK's cost on the sub-agent's - turn. ``None`` when no criterion reported cost. + turn. Post-failure diagnostic judges are included because their calls still + incur real spend even though their results cannot affect the canonical score. + ``None`` when no criterion reported cost. """ - usages = [u for cr in result.success_criteria_results if (u := getattr(cr, "token_usage", None)) is not None] + criterion_results = result.success_criteria_results + result.post_failure_criteria_results + usages = [u for cr in criterion_results if (u := getattr(cr, "token_usage", None)) is not None] return sum_costs(*(u.total_cost_usd for u in usages)) diff --git a/src/coder_eval/orchestration/run_limits.py b/src/coder_eval/orchestration/run_limits.py new file mode 100644 index 00000000..6a0161b3 --- /dev/null +++ b/src/coder_eval/orchestration/run_limits.py @@ -0,0 +1,34 @@ +"""Post-merge validation for cross-field run-limit semantics.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + + +if TYPE_CHECKING: + from coder_eval.models import TaskDefinition + + +INEFFECTIVE_TASK_TIMEOUT_WARNING = ( + "A larger task_timeout cannot extend the agent's single iteration; the agent budget is turn_timeout." +) + + +def validate_run_limits(task: TaskDefinition) -> tuple[str, ...]: + """Return non-blocking warnings for the fully resolved run limits. + + The comparison belongs after config merge because either timeout may come + from any of the five layers. The warning is about one agent call: even when + dialog simulation makes several calls, a larger task-wide timeout cannot + extend any call beyond its turn timeout. + """ + limits = task.run_limits + if limits is None or limits.task_timeout is None or limits.turn_timeout is None: + return () + if limits.task_timeout <= limits.turn_timeout: + return () + return ( + f"run_limits.task_timeout ({limits.task_timeout}s) exceeds " + + f"run_limits.turn_timeout ({limits.turn_timeout}s). " + + INEFFECTIVE_TASK_TIMEOUT_WARNING, + ) diff --git a/src/coder_eval/orchestrator.py b/src/coder_eval/orchestrator.py index 83f67da3..7054aa76 100644 --- a/src/coder_eval/orchestrator.py +++ b/src/coder_eval/orchestrator.py @@ -22,6 +22,8 @@ from .errors import ( AgentCrashError, BudgetExceededError, + CheckerMisuseError, + JudgeInfrastructureError, TaskTimeoutError, TurnTimeoutError, ) @@ -36,6 +38,7 @@ ApiRoute, BedrockRoute, ConfigLineageEntry, + CriteriaResults, CriterionResult, DirectRoute, EvaluationResult, @@ -48,6 +51,7 @@ PreservationMode, SimulationConfig, SimulationTelemetry, + SuccessCriterion, TaskConfigRecord, TaskDefinition, TokenUsage, @@ -58,6 +62,7 @@ ) from .orchestration.early_stop import EarlyStopWatcher, early_stop_active, validate_early_stop from .orchestration.evaluation import load_reference +from .orchestration.run_limits import validate_run_limits from .path_utils import format_task_log_id, task_log_path from .sandbox import Sandbox from .simulation import DialogStopReason, SimulatorResult, UserSimulator, evaluate_stop @@ -403,6 +408,10 @@ def __init__( # task run even though _check_expected_turns is called after every turn. self._expected_turns_warning_emitted: bool = False + # One-shot flag: a resolved task may be inspected more than once during + # setup, but its ineffective timeout relationship should be logged once. + self._run_limits_warning_emitted: bool = False + # Canonical id shared with run_dir layout, tqdm label, and streaming events. self._log_task_id = format_task_log_id(variant_id, task.task_id, replicate_index) @@ -494,16 +503,11 @@ def _kill_agent_subprocess_sync() -> None: asyncio_task_to_cancel=asyncio.current_task(), label=f"task_timeout ({self.task.task_id})", ) as wd: - try: - success = await self._evaluation_loop() - except asyncio.CancelledError: - if wd.fired: - raise TaskTimeoutError( - task_timeout or 0, - task_id=self.task.task_id, - elapsed_seconds=time.time() - start_time, - ) from None - raise + success = await self._run_evaluation_with_failure_evidence( + watchdog=wd, + task_timeout=task_timeout, + start_time=start_time, + ) # Belt-and-suspenders: if the loop returned normally but the # watchdog fired during post-loop work or the inner coro # swallowed the cancel, still classify as TIMEOUT. @@ -628,6 +632,139 @@ def _kill_agent_subprocess_sync() -> None: return self.result + async def _run_evaluation_with_failure_evidence( + self, + *, + watchdog: ThreadedWatchdog, + task_timeout: int | None, + start_time: float, + ) -> bool: + """Run the loop and collect diagnostics before its watchdog closes.""" + try: + return await self._evaluation_loop() + except asyncio.CancelledError: + if watchdog.fired: + self._record_post_failure_not_evaluated( + "the task_timeout budget was exhausted before post-failure grading could run" + ) + raise TaskTimeoutError( + task_timeout or 0, + task_id=self.task.task_id, + elapsed_seconds=time.time() - start_time, + ) from None + raise + except TaskTimeoutError: + self._record_post_failure_not_evaluated( + "the task_timeout budget was exhausted before post-failure grading could run" + ) + raise + except (AgentCrashError, TurnTimeoutError, BudgetExceededError) as terminal_error: + if ( + isinstance(terminal_error, BudgetExceededError) + and self.result is not None + and len(self.result.success_criteria_results) == len(self.task.success_criteria) + ): + raise + try: + await self._evaluate_post_failure_criteria() + except asyncio.CancelledError: + if watchdog.fired: + self._record_post_failure_not_evaluated( + "the task_timeout budget expired during post-failure grading" + ) + raise TaskTimeoutError( + task_timeout or 0, + task_id=self.task.task_id, + elapsed_seconds=time.time() - start_time, + ) from None + raise + except (JudgeInfrastructureError, CheckerMisuseError): + raise + except Exception as recovery_error: + self._record_post_failure_not_evaluated( + f"post-failure grading could not complete ({type(recovery_error).__name__})" + ) + logger.warning( + "[%s] Post-failure criteria evaluation failed; preserving the original terminal error", + self.task.task_id, + exc_info=True, + ) + raise + + @staticmethod + def _not_evaluated_result(criterion: SuccessCriterion, reason: str) -> CriterionResult: + return CriterionResult( + criterion_type=criterion.type, + description=criterion.description, + score=0.0, + details=f"Not evaluated after terminal agent failure: {reason}.", + evaluation_status="not_evaluated", + pass_threshold=criterion.pass_threshold, + gating=criterion.is_gating, + ) + + def _record_post_failure_not_evaluated(self, reason: str) -> None: + """Record a full diagnostic result vector when recovery cannot run.""" + if self.result is None: + return + self.result.post_failure_criteria_results = [ + self._not_evaluated_result(criterion, reason) for criterion in self.task.success_criteria + ] + + 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. Agent-dependent checks + require at least one preserved turn; artifact-only checks can still run + when the agent failed before producing trajectory evidence. + """ + if self.result is None: + return + if self.success_checker is None or self.sandbox is None: + self._record_post_failure_not_evaluated("the sandbox or success checker was unavailable") + return + + runnable: list[SuccessCriterion] = [] + unavailable_positions: set[int] = set() + for position, criterion in enumerate(self.task.success_criteria): + if criterion.requires_agent and not self.result.iterations: + unavailable_positions.add(position) + else: + runnable.append(criterion) + + checked: CriteriaResults = [] + if runnable: + reference_code, reference_dir, self._reference_code = load_reference( + task=self.task, + task_file=self.task_file, + cached_reference=self._reference_code, + ) + checked = await self.success_checker.check_all_async( + runnable, + reference_code=reference_code, + reference_dir=reference_dir, + turn_records=self.result.iterations, + ) + + if len(checked) != len(runnable): + raise ValueError( + f"Post-failure checker returned {len(checked)} results for {len(runnable)} runnable criteria" + ) + + checked_iter = iter(checked) + recovered: CriteriaResults = [] + for position, criterion in enumerate(self.task.success_criteria): + if position in unavailable_positions: + recovered.append( + self._not_evaluated_result( + criterion, + "no turn record survived for this agent-dependent criterion", + ) + ) + else: + recovered.append(next(checked_iter)) + self.result.post_failure_criteria_results = recovered + async def _drain_killed_turn(self) -> None: """Move a hard-killed turn's partial record from the agent onto the result. @@ -773,7 +910,7 @@ def _finalize_result(self, start_time: float) -> None: # Persist self.report_path.parent.mkdir(parents=True, exist_ok=True) # noqa: CE002 β€” mkdir on local FS is nanoseconds - # Spill any judge transcripts to sibling judge-.yaml files BEFORE + # Spill any judge transcripts to sibling YAML files BEFORE # we dump task.json, so transcript_path is set on each judge result. # The inline `transcript` field stays in memory β€” HTML rendering below # uses it directly. We strip it from the JSON dump via `exclude=...`. @@ -791,11 +928,14 @@ def _finalize_result(self, start_time: float) -> None: report_tmp.write_text( # noqa: CE002 β€” small JSON write at end of run self.result.model_dump_json( indent=2, - # Strip inline transcripts: they live in sibling judge-.yaml + # Strip inline transcripts: they live in sibling YAML files # next to task.json, referenced by transcript_path. Excluding # `transcript` here avoids ~20-100 KB of bloat per judge result # in the row record without losing any data. - exclude={"success_criteria_results": {"__all__": {"transcript"}}}, + exclude={ + "success_criteria_results": {"__all__": {"transcript"}}, + "post_failure_criteria_results": {"__all__": {"transcript"}}, + }, ), encoding="utf-8", ) @@ -907,6 +1047,16 @@ def _check_expected_turns(self, *, iteration: int) -> None: ) self._expected_turns_warning_emitted = True + def _warn_on_ineffective_task_timeout(self) -> None: + """Log resolved cross-field run-limit warnings once per task run.""" + if self._run_limits_warning_emitted: + return + messages = validate_run_limits(self.task) + for message in messages: + logger.warning("[%s] %s", self.task.task_id, message) + if messages: + self._run_limits_warning_emitted = True + @property def _cost_correlation_run_id(self) -> str: """The LiteLLM cost-log correlation run id β€” a stable hash of the run dir. @@ -988,6 +1138,7 @@ async def _setup(self) -> None: # paths (the CLI already validated during resolution). No-op unless # some criterion carries a stop_early: block. validate_early_stop(self.task) + self._warn_on_ineffective_task_timeout() # Build the early-stop watcher once, up front, when armed (>= 1 criterion # with a stop_early: block and the run_limits.stop_early kill switch not diff --git a/tests/test_cost_accounting_paths.py b/tests/test_cost_accounting_paths.py index 9193f7dc..98ac1b6f 100644 --- a/tests/test_cost_accounting_paths.py +++ b/tests/test_cost_accounting_paths.py @@ -188,6 +188,26 @@ def test_judge_cost_rolls_up_onto_the_row(self): assert row["total_cost_usd"] == pytest.approx(0.15) assert row["agent_cost_usd"] == pytest.approx(0.1) + def test_post_failure_judge_cost_rolls_up_without_affecting_score(self): + result = _result([_turn(1, TokenUsage(uncached_input_tokens=10, output_tokens=1, total_cost_usd=0.1))]) + result.final_status = FinalStatus.ERROR + result.weighted_score = 0.0 + result.total_token_usage = TokenUsage(uncached_input_tokens=10, output_tokens=1, total_cost_usd=0.1) + result.post_failure_criteria_results = [ + JudgeCriterionResult( + criterion_type="llm_judge", + description="diagnostic", + score=1.0, + token_usage=TokenUsage(uncached_input_tokens=5000, output_tokens=500, total_cost_usd=0.02), + ) + ] + + row = eval_result_to_task_dict(result) + + assert row["judge_cost_usd"] == pytest.approx(0.02) + assert row["total_cost_usd"] == pytest.approx(0.12) + assert row["weighted_score"] == 0.0 + def test_no_judge_means_no_judge_cost(self): """None, not 0.0 β€” 'no judge ran' must stay distinct from 'a judge ran free'.""" result = _result([_turn(1, TokenUsage(uncached_input_tokens=10, output_tokens=1, total_cost_usd=0.1))]) diff --git a/tests/test_criterion_result_round_trip.py b/tests/test_criterion_result_round_trip.py index 4d03a828..26d882dc 100644 --- a/tests/test_criterion_result_round_trip.py +++ b/tests/test_criterion_result_round_trip.py @@ -198,3 +198,52 @@ def test_mixed_result_types_in_one_list() -> None: reloaded = EvaluationResult.model_validate_json(er.model_dump_json()) types = [type(r).__name__ for r in reloaded.success_criteria_results] assert types == ["JudgeCriterionResult", "ClassificationCriterionResult", "CriterionResult"] + + +def test_legacy_result_defaults_to_evaluated_and_no_post_failure_evidence() -> None: + """Old task.json payloads keep their meaning when the new fields are absent.""" + legacy_payload = { + "task_id": "t", + "task_description": "d", + "agent_type": "claude-code", + "started_at": "2026-05-12T00:00:00", + "final_status": "ERROR", + "iteration_count": 1, + "success_criteria_results": [ + {"criterion_type": "file_exists", "description": "f", "score": 0.0}, + ], + } + + result = EvaluationResult.model_validate(legacy_payload) + + assert result.success_criteria_results[0].evaluation_status == "evaluated" + assert result.post_failure_criteria_results == [] + + +def test_post_failure_evaluation_status_round_trip() -> None: + result = _make_eval([]) + result.final_status = FinalStatus.ERROR + result.weighted_score = 0.0 + result.post_failure_criteria_results = [ + CriterionResult( + criterion_type="file_exists", + description="artifact exists", + score=1.0, + evaluation_status="evaluated", + ), + CriterionResult( + criterion_type="command_executed", + description="agent ran validator", + score=0.0, + details="Not evaluated after terminal agent failure: no turn record survived.", + evaluation_status="not_evaluated", + ), + ] + + reloaded = EvaluationResult.model_validate_json(result.model_dump_json()) + + assert reloaded.weighted_score == 0.0 + assert [r.evaluation_status for r in reloaded.post_failure_criteria_results] == [ + "evaluated", + "not_evaluated", + ] diff --git a/tests/test_judge_persistence.py b/tests/test_judge_persistence.py index 4d02bb03..7eb183cd 100644 --- a/tests/test_judge_persistence.py +++ b/tests/test_judge_persistence.py @@ -1,6 +1,6 @@ """Tests for the spill/load helpers in ``coder_eval.evaluation.judge_persistence``. -The orchestrator spills judge transcripts to ``judge-.json`` next to +The orchestrator spills judge transcripts to sibling YAML files next to ``task.json`` so the row record stays lean. Re-render paths reload them. These tests verify the round-trip and back-compat with old runs that inlined the transcript. @@ -133,6 +133,31 @@ def test_spill_preserves_index_for_multiple_judges(tmp_path: Path) -> None: assert (tmp_path / "judge-1.yaml").is_file() +def test_post_failure_judge_uses_distinct_sibling_and_round_trips(tmp_path: Path) -> None: + judge = _make_judge_result(transcript=_make_transcript()) + result = _make_evaluation_result(criteria=[]) + result.final_status = FinalStatus.ERROR + result.post_failure_criteria_results = [judge] + + assert spill_judge_transcripts(result, tmp_path) == 1 + assert judge.transcript_path == "post-failure-judge-0.yaml" + + raw = result.model_dump_json( + exclude={ + "success_criteria_results": {"__all__": {"transcript"}}, + "post_failure_criteria_results": {"__all__": {"transcript"}}, + } + ) + assert "raw_verdict" not in raw + + reloaded = EvaluationResult.model_validate_json(raw) + assert load_judge_transcripts(reloaded, tmp_path) == 1 + recovered = reloaded.post_failure_criteria_results[0] + assert isinstance(recovered, JudgeCriterionResult) + assert recovered.transcript is not None + assert recovered.transcript.raw_verdict == '{"score":0.75,"rationale":"ok"}' + + def test_spill_skips_non_judge_results(tmp_path: Path) -> None: """Plain CriterionResult instances are no-ops β€” no sibling file written.""" plain = CriterionResult( @@ -306,8 +331,8 @@ def test_load_rejects_dotdot_traversal(tmp_path: Path) -> None: def test_load_rejects_subdir_path(tmp_path: Path) -> None: """A path with a separator (even within task_dir) is rejected β€” the spill helper - only ever writes ``judge-.yaml`` as a basename, so anything with a slash is - by definition not from us.""" + only ever writes basenames, so anything with a slash is by definition not + from us.""" judge = _make_judge_result(transcript=None) judge.transcript_path = "subdir/judge-0.yaml" result = _make_evaluation_result(criteria=[judge]) diff --git a/tests/test_plan_command.py b/tests/test_plan_command.py index 51896846..16a8d663 100644 --- a/tests/test_plan_command.py +++ b/tests/test_plan_command.py @@ -7,7 +7,14 @@ import typer from coder_eval.cli.plan_command import plan_command -from coder_eval.models import AgentConfig, ExperimentDefinition, ExperimentVariant, TaskDefinition, parse_agent_config +from coder_eval.models import ( + AgentConfig, + ExperimentDefinition, + ExperimentVariant, + RunLimits, + TaskDefinition, + parse_agent_config, +) from coder_eval.models.enums import AgentKind @@ -209,6 +216,27 @@ def test_plan_with_default_experiment(self, tmp_path: Path) -> None: printed = " ".join(str(call) for call in mock_console.print.call_args_list) assert "test-exp" in printed + def test_plan_warns_when_task_timeout_cannot_extend_single_iteration(self, tmp_path: Path) -> None: + task_file = tmp_path / "task.yaml" + task_file.write_text("placeholder") + experiment = _make_experiment(variants=[ExperimentVariant(variant_id="default")]) + task = _make_task(agent=parse_agent_config(type=AgentKind.CLAUDE_CODE)) + resolved_task = task.model_copy(update={"run_limits": RunLimits(task_timeout=1500, turn_timeout=1200)}) + + with ( + patch("coder_eval.cli.plan_command.check_tools"), + patch("coder_eval.cli.plan_command.check_api_keys"), + patch("coder_eval.cli.plan_command.load_task", return_value=(task, "mock yaml")), + patch(f"{_EXP}.load_experiment", return_value=experiment), + patch(f"{_EXP}.resolve_task_for_variant", return_value=(resolved_task, {}, 1)), + patch("coder_eval.cli.plan_command.console") as mock_console, + ): + plan_command(task_files=[task_file]) + + printed = " ".join(str(call) for call in mock_console.print.call_args_list) + assert "A larger task_timeout cannot extend the agent's single iteration" in printed + assert "the agent budget is turn_timeout" in printed + def test_plan_exits_when_default_experiment_missing(self, tmp_path: Path) -> None: """When default experiment file is missing and no --experiment given, plan should exit.""" task_file = tmp_path / "task.yaml" diff --git a/tests/test_run_limits_models.py b/tests/test_run_limits_models.py index e6d6d6be..12241a1c 100644 --- a/tests/test_run_limits_models.py +++ b/tests/test_run_limits_models.py @@ -11,6 +11,7 @@ RunLimits, TaskDefinition, ) +from coder_eval.orchestration.run_limits import INEFFECTIVE_TASK_TIMEOUT_WARNING, validate_run_limits def _minimal_task(**overrides) -> TaskDefinition: @@ -129,6 +130,40 @@ def test_extra_forbid_still_rejects_unknowns(self): RunLimits.model_validate({"expected_turn": 5}) +class TestRunLimitsCrossFieldWarnings: + def test_warning_wording_states_the_single_iteration_semantic(self): + assert INEFFECTIVE_TASK_TIMEOUT_WARNING == ( + "A larger task_timeout cannot extend the agent's single iteration; the agent budget is turn_timeout." + ) + + @pytest.mark.parametrize( + ("task_timeout", "turn_timeout", "warns"), + [ + (121, 120, True), + (120, 120, False), + (119, 120, False), + (None, 120, False), + (120, None, False), + ], + ) + def test_warns_only_when_task_timeout_exceeds_turn_timeout(self, task_timeout, turn_timeout, warns): + task = _minimal_task(run_limits={"task_timeout": task_timeout, "turn_timeout": turn_timeout}) + + messages = validate_run_limits(task) + + assert bool(messages) is warns + if warns: + assert INEFFECTIVE_TASK_TIMEOUT_WARNING in messages[0] + + def test_dialog_simulation_still_warns_for_each_agent_call(self): + task = _minimal_task( + run_limits={"task_timeout": 121, "turn_timeout": 120}, + simulation={"enabled": True, "persona": "user", "goal": "finish"}, + ) + + assert INEFFECTIVE_TASK_TIMEOUT_WARNING in validate_run_limits(task)[0] + + class TestRunLimitsOnTaskDefinition: def test_default_is_none(self): assert _minimal_task().run_limits is None diff --git a/tests/test_run_limits_orchestrator.py b/tests/test_run_limits_orchestrator.py index 228168f5..2245d25f 100644 --- a/tests/test_run_limits_orchestrator.py +++ b/tests/test_run_limits_orchestrator.py @@ -281,6 +281,8 @@ async def test_run_arm_maps_budget_to_status( assert "budget exceeded" in (result.error_message or "") # Captured error_log_tail key allowlist must include both new statuses. assert result.error_details == {} + assert len(result.post_failure_criteria_results) == 1 + assert result.post_failure_criteria_results[0].evaluation_status == "not_evaluated" # Inspect the actual create_error_context call to confirm the component label. assert mock_ctx.call_args.kwargs["component"] == expected_component diff --git a/tests/test_timeout_orchestrator.py b/tests/test_timeout_orchestrator.py index 2bca85b1..de36073f 100644 --- a/tests/test_timeout_orchestrator.py +++ b/tests/test_timeout_orchestrator.py @@ -7,10 +7,12 @@ import pytest +from coder_eval.errors import JudgeInfrastructureError from coder_eval.errors.timeout import TaskTimeoutError, TurnTimeoutError from coder_eval.models import ( AgentKind, ClaudeCodeAgentConfig, + CommandExecutedCriterion, CriterionResult, EvaluationResult, FileExistsCriterion, @@ -147,6 +149,8 @@ async def slow_loop(): result = await orchestrator.run() assert result.final_status == "TIMEOUT" assert f"Task timed out after {task_timeout}s" in (result.error_message or "") + assert len(result.post_failure_criteria_results) == 1 + assert result.post_failure_criteria_results[0].evaluation_status == "not_evaluated" @pytest.mark.asyncio @@ -331,6 +335,119 @@ async def turn_out_communicate(_prompt, **kwargs): await orchestrator._evaluation_loop() +@pytest.mark.asyncio +async def test_turn_timeout_records_post_failure_evidence_without_rescoring(tmp_path) -> None: + """A terminal turn timeout preserves artifact truth without changing the ERROR score.""" + task = _make_task(turn_timeout=1200, task_timeout=1500) + task.success_criteria = [ + FileExistsCriterion(type="file_exists", path="artifact.txt", description="artifact exists"), + CommandExecutedCriterion( + type="command_executed", + tool_name="Bash", + description="agent ran validator", + ), + ] + run_dir = tmp_path / "run" / "post_failure_evidence" + 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._cleanup = AsyncMock() # type: ignore[method-assign] + orchestrator._refresh_runtime_tool_versions = MagicMock() # type: ignore[method-assign] + orchestrator._evaluation_loop = AsyncMock( # type: ignore[method-assign] + side_effect=TurnTimeoutError(1200, task_id=task.task_id, iteration=1) + ) + + mock_sandbox = MagicMock() + mock_sandbox.sandbox_dir = tmp_path / "sandbox" + mock_sandbox.sandbox_dir.mkdir() + orchestrator.sandbox = mock_sandbox + + mock_checker = MagicMock() + mock_checker.check_all_async = AsyncMock( + return_value=[ + CriterionResult( + criterion_type="file_exists", + description="artifact exists", + score=1.0, + ) + ] + ) + orchestrator.success_checker = mock_checker + + mock_agent = MagicMock() + mock_agent.kill_sync = MagicMock() + mock_agent.get_sdk_options = MagicMock(return_value=None) + orchestrator.agent = mock_agent + + with patch("coder_eval.orchestrator.load_reference", return_value=(None, None, None)): + result = await orchestrator.run() + + assert result.final_status == "ERROR" + assert result.weighted_score == 0.0 + assert result.success_criteria_results == [] + assert len(result.post_failure_criteria_results) == 2 + artifact, agent_dependent = result.post_failure_criteria_results + assert artifact.score == 1.0 + assert artifact.evaluation_status == "evaluated" + assert agent_dependent.score == 0.0 + assert agent_dependent.evaluation_status == "not_evaluated" + assert "no turn record survived" in (agent_dependent.details or "") + + checked_criteria = mock_checker.check_all_async.await_args.args[0] + assert [criterion.type for criterion in checked_criteria] == ["file_exists"] + + persisted = EvaluationResult.model_validate_json((run_dir / "task.json").read_text()) + assert persisted.final_status == "ERROR" + assert persisted.weighted_score == 0.0 + assert [r.evaluation_status for r in persisted.post_failure_criteria_results] == [ + "evaluated", + "not_evaluated", + ] + + +@pytest.mark.asyncio +async def test_post_failure_judge_infrastructure_error_still_escalates(tmp_path) -> None: + task = _make_task(turn_timeout=1200, task_timeout=1500) + task.success_criteria = [ + FileExistsCriterion(type="file_exists", path="artifact.txt", description="artifact exists") + ] + orchestrator = Orchestrator(task=task, run_dir=tmp_path / "run", variant_id="test-variant") + orchestrator._setup = AsyncMock() # type: ignore[method-assign] + orchestrator._cleanup = AsyncMock() # type: ignore[method-assign] + orchestrator._refresh_runtime_tool_versions = MagicMock() # type: ignore[method-assign] + orchestrator._evaluation_loop = AsyncMock( # type: ignore[method-assign] + side_effect=TurnTimeoutError(1200, task_id=task.task_id, iteration=1) + ) + orchestrator.sandbox = MagicMock() + orchestrator.success_checker = MagicMock() + orchestrator.success_checker.check_all_async = AsyncMock(side_effect=JudgeInfrastructureError("judge unavailable")) + orchestrator.agent = MagicMock() + orchestrator.agent.get_sdk_options.return_value = None + + with patch("coder_eval.orchestrator.load_reference", return_value=(None, None, None)): + result = await orchestrator.run() + + assert result.final_status == "ERROR" + assert result.error_message == "judge unavailable" + assert result.weighted_score == 0.0 + + +def test_runtime_timeout_warning_is_emitted_once(tmp_path, caplog) -> None: + import logging + + task = _make_task(turn_timeout=1200, task_timeout=1500) + orchestrator = Orchestrator(task=task, run_dir=tmp_path / "run", variant_id="test-variant") + + with caplog.at_level(logging.WARNING, logger="coder_eval.orchestrator"): + orchestrator._warn_on_ineffective_task_timeout() + orchestrator._warn_on_ineffective_task_timeout() + + messages = [record.message for record in caplog.records if "single iteration" in record.message] + assert len(messages) == 1 + assert "A larger task_timeout cannot extend the agent's single iteration" in messages[0] + assert "the agent budget is turn_timeout" in messages[0] + + @pytest.mark.asyncio async def test_task_timeout_fires_when_inner_coro_swallows_cancel(tmp_path) -> None: """Belt-and-suspenders: if ``_evaluation_loop`` catches ``CancelledError``