From 8c02dc6b7116527de012f3d46407efad6e6826ae Mon Sep 17 00:00:00 2001 From: uipreliga Date: Tue, 21 Jul 2026 16:31:42 -0700 Subject: [PATCH 1/3] fix: weight:0 un-gates criteria (informational criteria) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - weight: 0 now makes a criterion purely informational — excluded from both the weighted score AND the pass/fail gate (previously it could still flip a task to FAILURE). New BaseSuccessCriterion.is_gating drives the single-source gate (all_criteria_passed) and the `coder-eval evaluate` exit code. A validator rejects weight:0 combined with stop_when or suite_thresholds (arming a non-gating criterion for a pass/fail gate is incoherent). - reports.py failure-reason sampling uses pass_threshold, not score < 1.0. - Doc drift: dropped the non-existent `rephrase` mutation; corrected the stale "max_memory_mb NOT enforced" claim (docker --memory/--cpus/--pids-limit). Split out per review: the configurable retry policy moved to #44, and the evalboard OSS-edition change is dropped entirely (see @bai-uipath's review comment — the panels need core-produced fields that do not exist yet). Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 4 +- docs/AB_EXPERIMENTS.md | 4 +- docs/TASK_DEFINITION_GUIDE.md | 21 ++++++-- src/coder_eval/cli/evaluate_command.py | 19 +++++-- src/coder_eval/models/criteria.py | 44 ++++++++++++++-- src/coder_eval/models/results.py | 30 +++++++---- src/coder_eval/models/sandbox.py | 5 +- src/coder_eval/reports.py | 2 +- .../test_task_informational_criterion.yaml | 17 +++++++ tests/test_evaluate_command.py | 21 ++++++++ tests/test_threshold_enforcement.py | 51 +++++++++++++++++++ 11 files changed, 187 insertions(+), 31 deletions(-) create mode 100644 tests/fixtures/tasks/test_task_informational_criterion.yaml diff --git a/CLAUDE.md b/CLAUDE.md index 5c5cf392..c20bde4f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -35,7 +35,7 @@ coder_eval/ │ ├── criteria.py # 14 success criterion types + base + union │ ├── experiment.py # ExperimentDefinition, ExperimentVariant, ResolvedTask, result models │ ├── judge_defaults.py # DEFAULT_JUDGE_MODEL constant (cycle-free leaf) -│ ├── mutations.py # PromptMutation variants (prefix/suffix/replace/template/rephrase) +│ ├── mutations.py # PromptMutation variants (prefix/suffix/replace/template) │ ├── results.py # CriterionResult (+ ClassificationCriterionResult), TurnRecord, EvaluationResult, EarlyStopInfo/EarlyStopReason, CriterionAggregate, ThresholdCheck, SuiteRollup │ ├── routing.py # ApiRoute (DirectRoute/BedrockRoute) │ ├── sandbox.py # SandboxConfig, ResourceLimits @@ -294,7 +294,7 @@ Tasks are YAML files. See [docs/TASK_DEFINITION_GUIDE.md](docs/TASK_DEFINITION_G **Runtime (always)**: pydantic, pydantic-settings, pyyaml, typer, rich, python-dotenv, anthropic, claude-agent-sdk, anyio, radon, tqdm, jmespath, jsonschema -**Runtime (optional, `[uipath]` extra)**: uipath — the in-host `uipath` SDK (handy for local sandbox parity with tasks that invoke `uv run uipath eval ...`). Base installs without this extra still run end-to-end; UiPath-dependent paths fail at dispatch with a clear `pip install 'coder-eval[uipath]'` hint. The LLM judge and the `rephrase` mutation no longer use the LLM Gateway client — they route through the run's backend (Bedrock / Anthropic), so `uipath-llmgw-client` is no longer a dependency. +**Runtime (optional, `[uipath]` extra)**: uipath — the in-host `uipath` SDK (handy for local sandbox parity with tasks that invoke `uv run uipath eval ...`). Base installs without this extra still run end-to-end; UiPath-dependent paths fail at dispatch with a clear `pip install 'coder-eval[uipath]'` hint. The LLM judge no longer uses the LLM Gateway client — it routes through the run's backend (Bedrock / Anthropic), so `uipath-llmgw-client` is no longer a dependency. **Dev**: pytest, pytest-asyncio, pytest-mock, pytest-cov, ruff, pyright, pip-audit, bandit, pre-commit, mcp diff --git a/docs/AB_EXPERIMENTS.md b/docs/AB_EXPERIMENTS.md index 52c56269..0f437bc3 100644 --- a/docs/AB_EXPERIMENTS.md +++ b/docs/AB_EXPERIMENTS.md @@ -227,8 +227,8 @@ variants: text: "\n\nThink step by step and validate your work before finishing." ``` -The full mutation catalog (prefix / suffix / replace / template / rephrase) is -defined in `coder_eval/models/mutations.py`. +The full mutation catalog (prefix / suffix / replace / template) is defined in +`coder_eval/models/mutations.py`. ## Recipe: Smoke vs. e2e Flavors (Early Stop) diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index 3de0c196..2fe88b53 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -275,11 +275,17 @@ sandbox: - "!node_modules" - "*.bak" # bare entry adds an extra pattern limits: # Optional: resource limits - timeout: 300 # Enforced via subprocess timeout - max_memory_mb: 512 # NOT enforced (reserved for future use) - max_disk_mb: 1024 # NOT enforced (reserved for future use) + timeout: 300 # Enforced via subprocess timeout (both drivers) + max_memory_mb: 512 # driver:docker -> `--memory`; ignored under tempdir + max_cpus: 2 # driver:docker -> `--cpus`; ignored under tempdir + max_pids: 512 # driver:docker -> `--pids-limit`; ignored under tempdir + max_disk_mb: 1024 # NOT enforced (reserved: no portable docker knob) ``` +Under `driver: tempdir` only `timeout` is enforced — the agent can consume +arbitrary host memory, CPU, and PIDs. Use `driver: docker` when you need the +container limits above to actually bind. + ## Template Sources Tasks can start with preset files instead of an empty sandbox. Multiple sources are applied sequentially (last wins for conflicts). @@ -383,7 +389,7 @@ All criteria share these fields: | Field | Default | Description | |-------|---------|-------------| | `description` | — | Human-readable description (required) | -| `weight` | 1.0 | Relative importance for weighted score | +| `weight` | 1.0 | Relative importance for weighted score. `0` = **informational**: excluded from both the score and the pass/fail gate | | `pass_threshold` | 0.9 | Minimum score (0.0–1.0) to pass | | `stop_when` | `null` | Arms this criterion for early stop (`pass`/`fail`/`decided`); requires `run_limits.stop_early: true` and an observable criterion type (`skill_triggered`, `command_executed`). See [`stop_early`](#stop_early-opt-in-early-stop). | @@ -392,7 +398,12 @@ All criteria share these fields: - **Fractional** (0.0–1.0): `file_contains`, `file_check`, `json_check`, `command_executed`, `uipath_eval` - **Continuous** (0.0–1.0): `reference_comparison`, `llm_judge`, `agent_judge` -**Task success:** ALL criteria must score >= their `pass_threshold`. +**Task success:** all *gating* criteria must score >= their `pass_threshold`. A +criterion with `weight: 0` is informational — it is still checked, stored, and +rendered in reports, but it neither contributes to the score nor fails the task. +(A `weight: 0` criterion may not set `stop_when` or `suite_thresholds`: arming a +non-gating criterion for the early-stop or suite gate would let an +"informational" check flip a run to failure.) **Weighted score:** `weighted_score = sum(score * weight) / sum(weight)` — calculated regardless for quality assessment. diff --git a/src/coder_eval/cli/evaluate_command.py b/src/coder_eval/cli/evaluate_command.py index bad1d9c3..1e62d886 100644 --- a/src/coder_eval/cli/evaluate_command.py +++ b/src/coder_eval/cli/evaluate_command.py @@ -130,7 +130,12 @@ async def _setup_and_run() -> EvaluationResult: raise typer.Exit(1) for criterion, cr in zip(task.success_criteria, criteria_results, strict=True): - status = "[green]✓[/green]" if cr.score >= criterion.pass_threshold else "[red]✗[/red]" + if not criterion.is_gating: + # weight=0 is informational: it cannot pass/fail the task, so don't + # render it as ✓/✗ (that would contradict the gate and the exit code). + status = "[dim]○[/dim]" + else: + status = "[green]✓[/green]" if cr.score >= criterion.pass_threshold else "[red]✗[/red]" console.print(f"{status} {cr.criterion_type}") console.print(f" [dim]{cr.description}[/dim]") console.print(f" [dim]Score: {cr.score:.2f}[/dim]") @@ -140,15 +145,19 @@ async def _setup_and_run() -> EvaluationResult: console.print(f" [red]Error: {cr.error}[/red]") console.print() - passed = sum( - 1 for cr, c in zip(criteria_results, task.success_criteria, strict=True) if cr.score >= c.pass_threshold - ) - total = len(task.success_criteria) + # Gate over gating criteria only (weight=0 is informational and cannot fail + # the task) so this summary + the exit code below match final_status. + gating = [(cr, c) for cr, c in zip(criteria_results, task.success_criteria, strict=True) if c.is_gating] + passed = sum(1 for cr, c in gating if cr.score >= c.pass_threshold) + total = len(gating) failed = total - passed + informational = len(task.success_criteria) - total console.print("[bold]Summary:[/bold]") console.print(f" Passed: {passed}/{total}") console.print(f" Failed: {failed}/{total}") + if informational: + console.print(f" [dim]Informational (weight=0, not gated): {informational}[/dim]") console.print(f"\n[dim]Run directory: {prepared_run_dir}[/dim]") if result.sandbox_path: console.print(f"[dim]Artifacts: {result.sandbox_path}[/dim]") diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index 1bb59c8f..8786ac86 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -9,7 +9,7 @@ from __future__ import annotations from abc import ABC -from typing import Annotated, Any, ClassVar, Literal +from typing import Annotated, Any, ClassVar, Literal, Self from pydantic import BaseModel, ConfigDict, Field, model_validator @@ -101,10 +101,11 @@ class BaseSuccessCriterion(BaseModel, ABC): ge=0.0, description=( "Relative importance of this criterion in the weighted score (default: 1.0). Set to 0 to " - "exclude it from the weighted SCORE -- useful for informational or side-effect checks " - "(e.g. a setup command). NOTE: weight=0 excludes from the score but NOT from the pass/fail " - "gate -- a criterion scoring below its pass_threshold still flips the task to FAILURE " - "regardless of weight. To make a criterion truly non-gating, also set pass_threshold=0." + "make the criterion purely INFORMATIONAL -- useful for side-effect checks (e.g. a setup " + "command): it is excluded from the weighted score AND from the pass/fail gate, so scoring " + "below its pass_threshold no longer flips the task to FAILURE. The result is still " + "computed, stored, and rendered in reports. A weight=0 criterion may not set stop_when " + "or suite_thresholds (arming a non-gating criterion for a pass/fail gate is incoherent)." ), ) @@ -145,6 +146,39 @@ def model_post_init(self, context: Any, /) -> None: # the discriminated union rejects the dump with union_tag_not_found. self.__pydantic_fields_set__.add("type") + @model_validator(mode="after") + def check_weight_zero_is_not_gating(self) -> Self: + """Reject ``weight: 0`` combined with any gate-arming field. + + ``weight: 0`` makes a criterion informational — excluded from the score + and from the pass/fail gate (see ``is_gating``). Both ``stop_when`` (arms + the per-row early-stop gate) and ``suite_thresholds`` (arms the + across-row suite gate, which drives the run's exit code) would let an + "informational" criterion flip a run to failure — directly contradicting + the field's contract. So both combinations are authoring errors, caught + at load time rather than surfacing as a confusing exit code later. + """ + if self.weight == 0.0: + for field, name in (("stop_when", "stop_when"), ("suite_thresholds", "suite_thresholds")): + if getattr(self, field) is not None: + raise ValueError( + f"criterion {self.type!r}: weight=0 makes the criterion informational (non-gating), " + + f"so it cannot also set {name} (which arms it for a pass/fail gate). " + + f"Give it a non-zero weight, or drop {name}." + ) + return self + + @property + def is_gating(self) -> bool: + """True when this criterion participates in the task's pass/fail gate. + + Single source of truth for "does a low score here fail the task". A + ``weight: 0`` criterion is informational: it is excluded from the + weighted score (it contributes 0 to both numerator and denominator) and, + by the same token, from the gate — so the two never disagree. + """ + return self.weight > 0.0 + # Business logic (check operations) moved to SuccessChecker in evaluator.py diff --git a/src/coder_eval/models/results.py b/src/coder_eval/models/results.py index 05dbbb1d..dbc1e33a 100644 --- a/src/coder_eval/models/results.py +++ b/src/coder_eval/models/results.py @@ -593,20 +593,30 @@ def calculate_weighted_score(self, criteria: list[SuccessCriterion]) -> None: self.weighted_score = total_weighted_score / total_weight if total_weight > 0 else 0.0 def all_criteria_passed(self, criteria: list[SuccessCriterion]) -> bool: - """True iff every criterion result meets its pass_threshold. - - Single source of truth for the success gate. A results/criteria length - mismatch raises ``ValueError`` rather than silently truncating — the - ``len()`` pre-check is required because ``all()`` short-circuits on the - first failing pair, so ``zip(strict=True)`` alone would not reliably - reach the length check. + """True iff every GATING criterion result meets its pass_threshold. + + Single source of truth for the success gate. ``weight: 0`` criteria are + informational (``BaseSuccessCriterion.is_gating`` is False): they are + excluded from the weighted score, so they are excluded from the gate + too — otherwise a criterion that contributes nothing to the score could + still single-handedly flip the task to FAILURE. A task whose criteria + are ALL weight-0 has an empty gate and therefore passes. + + A results/criteria length mismatch raises ``ValueError`` rather than + silently truncating — the ``len()`` pre-check is required because + ``all()`` short-circuits on the first failing pair, so + ``zip(strict=True)`` alone would not reliably reach the length check. """ if len(self.success_criteria_results) != len(criteria): raise ValueError( f"Results/criteria length mismatch for task {self.task_id}: " + f"{len(self.success_criteria_results)} results vs {len(criteria)} criteria." ) - return all(r.score >= c.pass_threshold for r, c in zip(self.success_criteria_results, criteria, strict=True)) + return all( + r.score >= c.pass_threshold + for r, c in zip(self.success_criteria_results, criteria, strict=True) + if c.is_gating + ) def armed_criteria_passed(self, criteria: list[SuccessCriterion]) -> bool: """True iff every ARMED criterion (``stop_when`` set) meets its pass_threshold. @@ -618,7 +628,9 @@ def armed_criteria_passed(self, criteria: list[SuccessCriterion]) -> bool: pre-check as ``all_criteria_passed`` so the gate logic stays single-sourced. Raises ``ValueError`` on an empty armed set — unreachable when a stop actually fired (a stop requires an armed criterion), so this - is a defensive guard against misuse. + is a defensive guard against misuse. No ``is_gating`` filter is needed + here: ``BaseSuccessCriterion`` rejects ``weight: 0`` together with + ``stop_when``, so every armed criterion is gating by construction. """ if len(self.success_criteria_results) != len(criteria): raise ValueError( diff --git a/src/coder_eval/models/sandbox.py b/src/coder_eval/models/sandbox.py index bf86106e..282cdbdc 100644 --- a/src/coder_eval/models/sandbox.py +++ b/src/coder_eval/models/sandbox.py @@ -300,8 +300,9 @@ class SandboxConfig(BaseModel): network, process table, and filesystem outside the temp dir. ``driver: docker`` runs each task inside its own container; see :class:`DockerDriverConfig` for knobs. ``ResourceLimits.timeout`` is the - only limit enforced in tempdir mode; under docker, ``max_memory_mb`` also - maps to ``--memory`` when set. + only limit enforced in tempdir mode; under docker, ``max_memory_mb`` / + ``max_cpus`` / ``max_pids`` also map to ``--memory`` / ``--cpus`` / + ``--pids-limit`` when set. """ model_config = ConfigDict(populate_by_name=True, extra="forbid") diff --git a/src/coder_eval/reports.py b/src/coder_eval/reports.py index 6e5b9deb..3dac1371 100644 --- a/src/coder_eval/reports.py +++ b/src/coder_eval/reports.py @@ -815,7 +815,7 @@ def _compute_suite_rollup( break reasons: list[str] = [] for cr in row.result.success_criteria_results: - if cr.error is not None or cr.score < 1.0: + if cr.error is not None or cr.score < cr.pass_threshold: reason = cr.error or cr.details or f"{cr.criterion_type}: score={cr.score:.2f}" reasons.append(reason[:_FAILURE_REASON_MAX_LEN]) if len(reasons) >= _FAILURE_REASONS_PER_ROW: diff --git a/tests/fixtures/tasks/test_task_informational_criterion.yaml b/tests/fixtures/tasks/test_task_informational_criterion.yaml new file mode 100644 index 00000000..98a0f0df --- /dev/null +++ b/tests/fixtures/tasks/test_task_informational_criterion.yaml @@ -0,0 +1,17 @@ +task_id: test_task_informational +description: Gating criterion passes; a weight=0 informational criterion fails. +initial_prompt: Test +# agent block required by TaskDefinition schema but unused in evaluate-only mode +agent: + type: claude-code +sandbox: + driver: tempdir + python: null +success_criteria: + - type: file_exists + path: app.py + description: app.py must exist (gating) + - type: file_exists + path: missing.py + description: informational side-effect check (weight=0, must not gate) + weight: 0.0 diff --git a/tests/test_evaluate_command.py b/tests/test_evaluate_command.py index 24fd16ea..67a98046 100644 --- a/tests/test_evaluate_command.py +++ b/tests/test_evaluate_command.py @@ -132,6 +132,27 @@ def test_evaluate_command_failure(tmp_path): assert exc_info.value.exit_code == 1 +def test_evaluate_command_informational_criterion_does_not_fail_exit(tmp_path): + """A failing weight=0 criterion must not flip the exit code (it is non-gating).""" + task_file = FIXTURES_DIR / "tasks" / "test_task_informational_criterion.yaml" + + work_dir = tmp_path / "work" + work_dir.mkdir() + (work_dir / "app.py").write_text("print('hello')") # gating criterion passes; missing.py absent + + from coder_eval.cli.evaluate_command import evaluate_command + + run_dir = tmp_path / "run" + run_dir.mkdir() + + with patch("coder_eval.cli.console.console.print"), patch("coder_eval.logging_config.setup_logging"): + with pytest.raises(typer.Exit) as exc_info: + evaluate_command(task_file=task_file, work_dir=work_dir, run_dir=run_dir) + + # The only gating criterion passed → exit 0, despite the weight=0 miss. + assert exc_info.value.exit_code == 0 + + def test_evaluate_command_invalid_task_file(tmp_path): """Test evaluate command with invalid task file.""" # Use non-existent task file diff --git a/tests/test_threshold_enforcement.py b/tests/test_threshold_enforcement.py index ec789805..fa7a1c39 100644 --- a/tests/test_threshold_enforcement.py +++ b/tests/test_threshold_enforcement.py @@ -9,8 +9,10 @@ from datetime import datetime import pytest +from pydantic import ValidationError from coder_eval.models import ( + CommandExecutedCriterion, CriterionResult, EvaluationResult, FileExistsCriterion, @@ -103,6 +105,55 @@ def test_all_criteria_passed_raises_on_length_mismatch(self): with pytest.raises(ValueError, match="length mismatch"): result.all_criteria_passed(criteria) + def test_zero_weight_criterion_is_informational_and_does_not_gate(self): + """weight=0 excludes a criterion from the score AND from the pass/fail gate.""" + criteria = [ + FileExistsCriterion(path="f1.txt", description="crit-0", weight=1.0, pass_threshold=0.9), + FileExistsCriterion(path="f2.txt", description="crit-1", weight=0.0, pass_threshold=0.9), + ] + result = _make_result([1.0, 0.0]) # the informational criterion scores zero + + result.calculate_weighted_score(criteria) + assert result.weighted_score == 1.0 # weight-0 contributes to neither term + assert result.all_criteria_passed(criteria) # ...and cannot flip the task to FAILURE + + def test_zero_weight_does_not_rescue_a_failing_gating_criterion(self): + """Only the weight-0 criterion is exempt; real criteria still gate.""" + criteria = [ + FileExistsCriterion(path="f1.txt", description="crit-0", weight=1.0, pass_threshold=0.9), + FileExistsCriterion(path="f2.txt", description="crit-1", weight=0.0, pass_threshold=0.9), + ] + assert not _make_result([0.5, 1.0]).all_criteria_passed(criteria) + + def test_all_zero_weight_criteria_leave_an_empty_gate(self): + """A task of purely informational criteria has nothing to fail on.""" + criteria = [ + FileExistsCriterion(path="f1.txt", description="crit-0", weight=0.0, pass_threshold=0.9), + FileExistsCriterion(path="f2.txt", description="crit-1", weight=0.0, pass_threshold=0.9), + ] + assert _make_result([0.0, 0.0]).all_criteria_passed(criteria) + + def test_zero_weight_cannot_be_armed_for_early_stop(self): + """weight=0 + stop_when is incoherent: it would leave the early-stop gate empty.""" + with pytest.raises(ValidationError, match="weight=0"): + CommandExecutedCriterion( + description="informational + armed", + weight=0.0, + stop_when="pass", + tool_name="Bash", + command_pattern="pytest", + ) + + def test_zero_weight_cannot_declare_suite_thresholds(self): + """weight=0 + suite_thresholds is incoherent: the suite gate drives the run exit code.""" + with pytest.raises(ValidationError, match="weight=0"): + FileExistsCriterion( + path="f1.txt", + description="informational + suite-gated", + weight=0.0, + suite_thresholds={"mean": 0.8}, + ) + def test_empty_inputs_score_zero_without_raising(self): """Empty results/criteria still yield 0.0 (not a raise) and an empty gate passes.""" result = _make_result([]) From b2688756ddac8bbe55bb2469c0442c2557188097 Mon Sep 17 00:00:00 2001 From: uipreliga Date: Wed, 22 Jul 2026 14:51:03 -0700 Subject: [PATCH 2/3] fix: render weight:0 criteria as informational on every display surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the follow-up deferred in #34. `final_status` and both exit codes already ignored weight:0 criteria, but the display sites still labeled a below-threshold informational criterion as FAILED — a header-vs-list contradiction where the task reads SUCCESS while a row reads FAIL. The fix is one carried marker rather than four independent re-derivations: - `CriterionResult.gating` (default True) mirrors `BaseSuccessCriterion. is_gating`, stamped by `SuccessChecker` alongside `pass_threshold` on all three construction paths. Defaulting True means task.json files written before this field read back as gating. - checker log line says "below threshold (informational)" / "errored (informational)" instead of FAILED. - reports_html: rows tagged "informational — not gated (weight: 0)"; the "(n/m passed)" header counts gating criteria only and appends "+ k informational". - reports: suite failure-reason sampling skips non-gating criteria — an informational miss must not explain why a row failed. - evalboard: DTO carries `gating` + `passThreshold`; the pill renders INFO (gray) for non-gating criteria, and the criteria header counts them separately. Drive-by: the pill compared `score === 1`, so a fractional criterion passing at 0.95 with threshold 0.9 rendered FAIL — it now compares against the criterion's own threshold. Tests: gating is stamped from the criterion; results default to gating on read-back; the HTML header excludes informational criteria and labels the row; suite sampling omits an informational miss (verified failing without the fix). Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/TASK_DEFINITION_GUIDE.md | 8 +++ evalboard/app/runs/[id]/[...task]/_chips.tsx | 20 ++++++-- .../app/runs/[id]/[...task]/_sections.tsx | 15 +++++- evalboard/lib/runs.ts | 10 ++++ src/coder_eval/evaluation/checker.py | 11 +++- src/coder_eval/models/results.py | 11 ++++ src/coder_eval/reports.py | 4 ++ src/coder_eval/reports_html.py | 17 +++++-- tests/test_evaluator.py | 2 + tests/test_suite_rollup.py | 23 +++++++++ tests/test_threshold_enforcement.py | 51 +++++++++++++++++++ 11 files changed, 159 insertions(+), 13 deletions(-) diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index 2fe88b53..d0baa887 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -405,6 +405,14 @@ rendered in reports, but it neither contributes to the score nor fails the task. non-gating criterion for the early-stop or suite gate would let an "informational" check flip a run to failure.) +Every surface labels it as such rather than as a failure: the terminal shows `○` +instead of `✓`/`✗`, the HTML report tags the row "informational — not gated" and +excludes it from the *n*/*m* passed header, the evalboard renders an `INFO` pill, +and a below-threshold informational criterion is never sampled as the reason a +suite row failed. The persisted `CriterionResult.gating` field carries this to +every consumer, so no reader needs the original criterion to know whether a low +score mattered. + **Weighted score:** `weighted_score = sum(score * weight) / sum(weight)` — calculated regardless for quality assessment. ### `file_exists` diff --git a/evalboard/app/runs/[id]/[...task]/_chips.tsx b/evalboard/app/runs/[id]/[...task]/_chips.tsx index cfaa5c4f..09e1c44c 100644 --- a/evalboard/app/runs/[id]/[...task]/_chips.tsx +++ b/evalboard/app/runs/[id]/[...task]/_chips.tsx @@ -26,15 +26,25 @@ export function Expandable({ ); } -export function ResultPill({ passed }: { passed: boolean }) { - const cls = passed - ? "bg-green-50 text-green-700 border-green-200" - : "bg-red-50 text-red-700 border-red-200"; +// `gating: false` (weight: 0) criteria are informational — they cannot fail the +// task, so rendering PASS/FAIL for them would contradict the task's own status. +export function ResultPill({ + passed, + gating = true, +}: { + passed: boolean; + gating?: boolean; +}) { + const cls = !gating + ? "bg-gray-50 text-gray-600 border-gray-200" + : passed + ? "bg-green-50 text-green-700 border-green-200" + : "bg-red-50 text-red-700 border-red-200"; return ( - {passed ? "PASS" : "FAIL"} + {!gating ? "INFO" : passed ? "PASS" : "FAIL"} ); } diff --git a/evalboard/app/runs/[id]/[...task]/_sections.tsx b/evalboard/app/runs/[id]/[...task]/_sections.tsx index 6e7f4407..62a5bae0 100644 --- a/evalboard/app/runs/[id]/[...task]/_sections.tsx +++ b/evalboard/app/runs/[id]/[...task]/_sections.tsx @@ -108,16 +108,27 @@ export function CriteriaSection({ criteria }: { criteria: CriterionResult[] }) {

Success criteria ({criteria.length}) + {criteria.some((c) => !c.gating) && ( + + {criteria.filter((c) => !c.gating).length}{" "} + informational + + )}

{criteria.map((c, i) => { - const passed = c.score === 1; + // Compare against the criterion's own threshold, not === 1: + // fractional criteria pass below 1.0 (default threshold 0.9). + const passed = (c.score ?? 0) >= c.passThreshold; return ( - + {c.description ?? c.criterionType ?? diff --git a/evalboard/lib/runs.ts b/evalboard/lib/runs.ts index c8538f0d..c1d3f824 100644 --- a/evalboard/lib/runs.ts +++ b/evalboard/lib/runs.ts @@ -103,6 +103,12 @@ export interface CriterionResult { score: number | null; details: string | null; error: string | null; + // Mirrors the Python CriterionResult fields. `gating: false` (weight: 0) means + // the criterion is informational — measured, but excluded from the score and + // the pass/fail gate, so it must not render as PASS/FAIL. Both default the way + // pre-existing task.json files behave: gating, threshold 0.9. + passThreshold: number; + gating: boolean; } export interface ElementExecution { @@ -1823,6 +1829,8 @@ export async function readTaskDetail( score?: number; details?: string; error?: string | null; + pass_threshold?: number; + gating?: boolean; }>; iterations?: TurnEntry[]; environment_info?: RawRunJson["environment_info"]; @@ -1836,6 +1844,8 @@ export async function readTaskDetail( score: c.score ?? null, details: c.details ?? null, error: c.error ?? null, + passThreshold: c.pass_threshold ?? 0.9, + gating: c.gating ?? true, })); const artifactRoot = path.join(contentDir, "artifacts"); diff --git a/src/coder_eval/evaluation/checker.py b/src/coder_eval/evaluation/checker.py index 91f3b5da..60515ca0 100644 --- a/src/coder_eval/evaluation/checker.py +++ b/src/coder_eval/evaluation/checker.py @@ -214,12 +214,16 @@ def _check_single( context=context, ) result.pass_threshold = criterion.pass_threshold + result.gating = criterion.is_gating logger.debug(f"Criterion '{criterion_type}' score: {result.score:.2f}") if result.score < criterion.pass_threshold: + # An informational (weight: 0) criterion cannot fail the task, so + # logging it as FAILED contradicts the final status. Say what it is. logger.info( - "Criterion '%s' FAILED (score=%.2f, threshold=%.2f): %s", + "Criterion '%s' %s (score=%.2f, threshold=%.2f): %s", criterion_type, + "FAILED" if criterion.is_gating else "below threshold (informational)", result.score, criterion.pass_threshold, _short_failure_reason(result), @@ -237,6 +241,7 @@ def _check_single( details=f"No checker registered for criterion type '{criterion_type}'", error=f"Unsupported criterion type: '{criterion_type}'", pass_threshold=criterion.pass_threshold, + gating=criterion.is_gating, ) except JudgeInfrastructureError: raise # judge infra failure escalates to FinalStatus.ERROR; do not score it @@ -250,10 +255,12 @@ def _check_single( details="Error running checker", error=str(e), pass_threshold=criterion.pass_threshold, + gating=criterion.is_gating, ) logger.info( - "Criterion '%s' FAILED (score=0.00, threshold=%.2f): %s", + "Criterion '%s' %s (score=0.00, threshold=%.2f): %s", criterion_type, + "FAILED" if criterion.is_gating else "errored (informational)", criterion.pass_threshold, _short_failure_reason(failed), ) diff --git a/src/coder_eval/models/results.py b/src/coder_eval/models/results.py index dbc1e33a..9a424914 100644 --- a/src/coder_eval/models/results.py +++ b/src/coder_eval/models/results.py @@ -76,6 +76,17 @@ class CriterionResult(BaseModel): le=1.0, description="Score required to pass this criterion (mirrors BaseSuccessCriterion.pass_threshold).", ) + gating: bool = Field( + default=True, + description=( + "Whether a below-threshold score here fails the task (mirrors " + "BaseSuccessCriterion.is_gating, i.e. weight > 0). False marks an informational " + "criterion: it is measured and reported but excluded from the score and the " + "pass/fail gate, so every display surface must render it as informational rather " + "than failed. Defaults True so results persisted before this field existed — and " + "any result built without a source criterion — read back as gating." + ), + ) class ClassificationCriterionResult(CriterionResult): diff --git a/src/coder_eval/reports.py b/src/coder_eval/reports.py index 3dac1371..d5cce11f 100644 --- a/src/coder_eval/reports.py +++ b/src/coder_eval/reports.py @@ -815,6 +815,10 @@ def _compute_suite_rollup( break reasons: list[str] = [] for cr in row.result.success_criteria_results: + # Informational (weight: 0) criteria cannot fail a row, so they must + # not supply the reason the row is reported as failed. + if not cr.gating: + continue if cr.error is not None or cr.score < cr.pass_threshold: reason = cr.error or cr.details or f"{cr.criterion_type}: score={cr.score:.2f}" reasons.append(reason[:_FAILURE_REASON_MAX_LEN]) diff --git a/src/coder_eval/reports_html.py b/src/coder_eval/reports_html.py index 57184eff..7f0e9cad 100644 --- a/src/coder_eval/reports_html.py +++ b/src/coder_eval/reports_html.py @@ -575,7 +575,11 @@ def _render_criteria(results: list[CriterionResult], early_stop: EarlyStopInfo | rows: list[str] = [] for cr in results: advisory = "" - if early_stop is not None and f"{cr.criterion_type}: {cr.description}" not in armed: + if not cr.gating: + # weight: 0 — measured but excluded from the gate. Saying so here keeps + # the row from reading as a failure that the header/status contradicts. + advisory = '
informational — not gated (weight: 0)
' + elif early_stop is not None and f"{cr.criterion_type}: {cr.description}" not in armed: advisory = '
advisory — not gated (run stopped early)
' rows.append( f""" @@ -587,10 +591,15 @@ def _render_criteria(results: list[CriterionResult], early_stop: EarlyStopInfo | """ ) - passed = sum(1 for r in results if r.score >= r.pass_threshold) - total = len(results) + # Count over gating criteria only, so this header agrees with final_status + # and the CLI exit code (both of which ignore weight: 0 criteria). + gating_results = [r for r in results if r.gating] + passed = sum(1 for r in gating_results if r.score >= r.pass_threshold) + total = len(gating_results) + informational = len(results) - total + info_note = f' + {informational} informational' if informational else "" return f""" -

Success Criteria ({passed}/{total} passed)

+

Success Criteria ({passed}/{total} passed{info_note})

diff --git a/tests/test_evaluator.py b/tests/test_evaluator.py index adf4c23e..8d61cc0d 100644 --- a/tests/test_evaluator.py +++ b/tests/test_evaluator.py @@ -59,6 +59,7 @@ class _Fake: type = "unsupported_type" description = "fake" pass_threshold = 0.42 + is_gating = True result = checker._check_single(_Fake(), reference_code=None) # type: ignore[arg-type] assert result.pass_threshold == 0.42 @@ -206,6 +207,7 @@ def test_success_checker_unsupported_type(): bad_criterion.type = "invalid_type_that_does_not_exist" bad_criterion.description = "Test criterion with unsupported type" bad_criterion.pass_threshold = 0.9 + bad_criterion.is_gating = True # a bare Mock() yields a Mock here, not a bool # Should return failed result instead of raising result = checker.check(bad_criterion) diff --git a/tests/test_suite_rollup.py b/tests/test_suite_rollup.py index 20041251..425ee7d7 100644 --- a/tests/test_suite_rollup.py +++ b/tests/test_suite_rollup.py @@ -139,6 +139,29 @@ def test_mixed_outcomes(self, tmp_path: Path) -> None: ids = sorted(s.row_id for s in rollup.failed_samples if s.row_id is not None) assert ids == ["r2", "r3"] + def test_informational_criterion_is_not_a_failure_reason(self, tmp_path: Path) -> None: + """A weight-0 criterion cannot fail a row, so it must not explain one. + + The row here fails on its gating criterion; the informational one also + scored 0. Only the gating miss may appear in failure_reasons — otherwise + the sample tells a reviewer to go fix a criterion that never gated. + """ + row = _make_row( + suite_id="s", + row_id="r1", + final_status=FinalStatus.FAILURE, + weighted_score=0.0, + criteria=[("file_exists", 0.0, None), ("skill_triggered", 0.0, None)], + ) + row.result.success_criteria_results[1].gating = False + row.result.success_criteria_results[0].details = "gating miss" + row.result.success_criteria_results[1].details = "informational miss" + + rollup = _compute_suite_rollup("s", "v1", [row], tmp_path) + + reasons = rollup.failed_samples[0].failure_reasons + assert reasons == ["gating miss"] + def test_per_criterion_stats_averaging(self, tmp_path: Path) -> None: rows = [ _make_row( diff --git a/tests/test_threshold_enforcement.py b/tests/test_threshold_enforcement.py index fa7a1c39..3f59ffa4 100644 --- a/tests/test_threshold_enforcement.py +++ b/tests/test_threshold_enforcement.py @@ -160,3 +160,54 @@ def test_empty_inputs_score_zero_without_raising(self): result.calculate_weighted_score([]) assert result.weighted_score == 0.0 assert result.all_criteria_passed([]) # all([]) is True + + +class TestInformationalDisplayParity: + """A weight-0 criterion must render as informational everywhere, not as failed. + + ``final_status`` and both exit codes already ignore weight-0 criteria. These + pin the *display* surfaces to the same story, so a report header can't + contradict its own row list. + """ + + def test_checker_stamps_gating_from_the_criterion(self, tmp_path): + """The result mirrors ``is_gating`` so downstream renderers need no criterion.""" + from coder_eval.evaluation.checker import SuccessChecker + from coder_eval.models import SandboxConfig + from coder_eval.sandbox import Sandbox + + sandbox = Sandbox(SandboxConfig(driver="tempdir"), task_id="gating-display-test") + sandbox.sandbox_dir = tmp_path + checker = SuccessChecker(sandbox) + + gating = checker.check(FileExistsCriterion(path="missing.txt", description="gating", weight=1.0)) + informational = checker.check(FileExistsCriterion(path="missing.txt", description="informational", weight=0.0)) + + assert gating.gating is True + assert informational.gating is False + # Both genuinely scored zero — un-gating changes the label, never the measurement. + assert gating.score == 0.0 + assert informational.score == 0.0 + + def test_criterion_result_defaults_to_gating(self): + """Results persisted before the field existed must read back as gating.""" + assert CriterionResult(criterion_type="file_exists", description="d", score=0.0).gating is True + restored = CriterionResult.model_validate_json( + '{"criterion_type": "file_exists", "description": "d", "score": 0.0}' + ) + assert restored.gating is True + + def test_html_report_labels_informational_and_excludes_it_from_the_count(self): + """The HTML header counts gating criteria only, and the row says why.""" + from coder_eval.reports_html import _render_criteria + + html = _render_criteria( + [ + CriterionResult(criterion_type="file_exists", description="real", score=1.0, gating=True), + CriterionResult(criterion_type="file_exists", description="info", score=0.0, gating=False), + ] + ) + + assert "(1/1 passed" in html # NOT 1/2 — the informational miss isn't a failure + assert "informational — not gated (weight: 0)" in html + assert "1 informational" in html From 3ec8a058848dc6987346d051ea379908f1921d50 Mon Sep 17 00:00:00 2001 From: uipreliga Date: Wed, 22 Jul 2026 14:21:49 -0700 Subject: [PATCH 3/3] feat: configurable retry policy as a top-level `retry:` root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split out of #34 per review, with the placement feedback applied. - New RetryPolicy (models/retry.py): max_retries / initial_delay / backoff_multiplier, all optional. It lives at the task's TOP LEVEL (`retry:`), not under `run_limits` — `run_limits` holds caps that abort a run, while a retry policy is how hard the run tries to survive a transient error. - `retry` is a first-class 4th `-D` root alongside agent/run_limits/sandbox: `-D retry.max_retries=0` to fail fast while debugging. It merges through the SAME resolve_root engine at every layer (experiment defaults, task, variant, CLI), so the unification invariant holds — covered by new test_merge_unification cases. - All retry decisions resolve through one seam, errors/retry.py:: resolve_retry_config. A category that is non-retryable in the built-in table (auth/billing) is never made retryable by a per-run override — retryability is a property of the error, not the run. - Persisted error_details.is_retryable / retry_delay_seconds now reflect the run's actual policy instead of the built-in defaults. - Docs: TASK_DEFINITION_GUIDE gains a `retry` section that documents the placement rationale AND the in-place retry semantics raised in review — a retried communicate() re-sends into the same sandbox and session (the working directory is NOT reset), and the crashed attempt's partial turn is preserved, so trajectory-observing criteria see both attempts. Co-Authored-By: Claude Opus 4.8 (1M context) --- CLAUDE.md | 5 +- docs/TASK_DEFINITION_GUIDE.md | 42 +++++++++ docs/USER_GUIDE.md | 2 +- src/coder_eval/errors/executor.py | 14 +-- src/coder_eval/errors/retry.py | 45 ++++++++-- src/coder_eval/models/__init__.py | 5 ++ src/coder_eval/models/experiment.py | 15 ++++ src/coder_eval/models/retry.py | 66 ++++++++++++++ src/coder_eval/models/tasks.py | 9 ++ src/coder_eval/orchestration/config_merge.py | 24 ++++-- src/coder_eval/orchestration/experiment.py | 22 ++++- src/coder_eval/orchestration/overrides.py | 9 +- src/coder_eval/orchestrator.py | 13 +++ tests/test_error_handling.py | 91 +++++++++++++++++++- tests/test_merge_unification.py | 20 ++++- tests/test_overrides_engine.py | 21 +++++ 16 files changed, 375 insertions(+), 28 deletions(-) create mode 100644 src/coder_eval/models/retry.py diff --git a/CLAUDE.md b/CLAUDE.md index c20bde4f..89ce2263 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -36,6 +36,7 @@ coder_eval/ │ ├── experiment.py # ExperimentDefinition, ExperimentVariant, ResolvedTask, result models │ ├── judge_defaults.py # DEFAULT_JUDGE_MODEL constant (cycle-free leaf) │ ├── mutations.py # PromptMutation variants (prefix/suffix/replace/template) +│ ├── retry.py # RetryPolicy (top-level `retry:` block; -D root) │ ├── results.py # CriterionResult (+ ClassificationCriterionResult), TurnRecord, EvaluationResult, EarlyStopInfo/EarlyStopReason, CriterionAggregate, ThresholdCheck, SuiteRollup │ ├── routing.py # ApiRoute (DirectRoute/BedrockRoute) │ ├── sandbox.py # SandboxConfig, ResourceLimits @@ -130,8 +131,8 @@ templates/ # Sandbox template directories - **Separation of Concerns**: Data models (`models/`) are pure Pydantic; logic lives in `criteria/`, `evaluation/`, etc. - **Callback Streaming**: `StreamCallback` protocol with `TaskScopedCallback` wrapper for real-time LLM event output - **Experiment Layer**: Pre-processing config resolver (`ExperimentRunner`) that resolves task × variant combinations via 5-layer merge (default → experiment defaults → task → variant → CLI) before passing to `run_batch`. For running A/B comparisons (model vs. model, skill on vs. off, prompt vs. prompt), see [docs/AB_EXPERIMENTS.md](docs/AB_EXPERIMENTS.md). -- **Single declarative merge resolver**: All five config layers merge through ONE engine (`orchestration/config_merge.py::resolve_root`) for the three `-D`-reachable roots (`agent`/`run_limits`/`sandbox`). Each field declares *how it merges* once, on the model, via `MergeField(strategy="deep"|"append"|"replace")` (or a type-aware default: nested `BaseModel`/free-form `dict` → `deep`; `list`/scalar → `replace`). `resolve_task_for_variant` (layers 1–4) and `apply_overrides` (layer 5) build `Layer` lists and call the same `resolve_root`, so a field merges identically regardless of which layer supplied it (the unification invariant, enforced by `tests/test_merge_unification.py`). Lint rule CE014 forces every list field to declare its strategy explicitly. -- **Generic CLI overrides (`-D`/`--set`)**: Layer 5 is a thin wrapper (`orchestration/overrides.py`) over the resolver above. `coder-eval run -D agent.model=opus -D run_limits.max_turns=30` overrides any field on the resolved `TaskDefinition` (`agent`/`run_limits`/`sandbox` roots), schema-validated with did-you-mean. Only `--model` (→ `agent.model`) and `--driver` (→ `sandbox.driver`) survive as active thin aliases that emit the equivalent `-D` entry; an alias and `-D` targeting the same path is a hard error. `--type` (→ `agent.type`) is a separate, lighter alias that does NOT route through that collision check — `--type` and `-D agent.type=…` last-win rather than hard-error (the `-D` value wins). Tools, plugins, and SDK options are `-D`-only. +- **Single declarative merge resolver**: All five config layers merge through ONE engine (`orchestration/config_merge.py::resolve_root`) for the four `-D`-reachable roots (`agent`/`run_limits`/`retry`/`sandbox`). Each field declares *how it merges* once, on the model, via `MergeField(strategy="deep"|"append"|"replace")` (or a type-aware default: nested `BaseModel`/free-form `dict` → `deep`; `list`/scalar → `replace`). `resolve_task_for_variant` (layers 1–4) and `apply_overrides` (layer 5) build `Layer` lists and call the same `resolve_root`, so a field merges identically regardless of which layer supplied it (the unification invariant, enforced by `tests/test_merge_unification.py`). Lint rule CE014 forces every list field to declare its strategy explicitly. +- **Generic CLI overrides (`-D`/`--set`)**: Layer 5 is a thin wrapper (`orchestration/overrides.py`) over the resolver above. `coder-eval run -D agent.model=opus -D run_limits.max_turns=30` overrides any field on the resolved `TaskDefinition` (`agent`/`run_limits`/`retry`/`sandbox` roots), schema-validated with did-you-mean. Only `--model` (→ `agent.model`) and `--driver` (→ `sandbox.driver`) survive as active thin aliases that emit the equivalent `-D` entry; an alias and `-D` targeting the same path is a hard error. `--type` (→ `agent.type`) is a separate, lighter alias that does NOT route through that collision check — `--type` and `-D agent.type=…` last-win rather than hard-error (the `-D` value wins). Tools, plugins, and SDK options are `-D`-only. - **All core models importable from `coder_eval.models`** regardless of submodule - **Dataset fan-out**: `TaskDefinition.dataset` (inline rows or JSONL path) expands a single task into N row-tasks with `${row.}` substitution in `initial_prompt` and `success_criteria` string fields. Expansion runs in `task_loader.expand_dataset` **before** variant resolution, so variants cannot override the dataset. Row sampling: CLI `--sample N` (fixed-seed uniform-random N over the whole dataset) overrides `--sample-per-stratum N` / `dataset.sample_per_stratum` (stratified random N-per-stratum, keyed on `stratify_field`, default `expected_skill` — for classification suites like activation). Stratified sampling (whether the N-per-stratum count comes from the **CLI** `--sample-per-stratum` flag or **YAML** `dataset.sample_per_stratum`) is **nondeterministic** by default — it re-draws each run (so the nightly activation suite broadens coverage over time). Set `dataset.sample_seed` to pin a reproducible sample; an explicit seed always wins. (Only `--sample N` uses a fixed seed, since a smoke test wants the same N rows each run.) - **Per-criterion aggregation**: Each `BaseCriterion` subclass exposes `aggregate(criterion, per_row_results) -> CriterionAggregate | None`. Default emits `count / mean / median / std / min / max` so every criterion is suite-thresholdable for free. Classification-style criteria return `ClassificationCriterionResult` (subclass of `CriterionResult`) and layer accuracy / P/R/F1 / confusion via the shared `overlay_classification_metrics` utility. `BaseSuccessCriterion.suite_thresholds` gates the suite on those metrics; CLI exits non-zero on any gate failure. diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index d0baa887..257ae58c 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -103,6 +103,12 @@ run_limits: turn_timeout: 300 # Optional: per-communicate() timeout in seconds task_timeout: 600 # Optional: wall-clock cap across all iterations +# Retry policy is NOT a cap — it lives at the task's top level, not under run_limits +retry: # Optional: override the built-in retry policy + max_retries: 2 # attempts per retryable error category (0 = fail fast) + initial_delay: 5.0 # seconds before the first retry + backoff_multiplier: 2.0 # exponential backoff factor + agent: type: "claude-code" # Agent type — optional if supplied via experiment / --type permission_mode: "acceptEdits" # Permission mode (see below) @@ -184,6 +190,42 @@ model, etc.) is conceptually separate. > They must live under `run_limits:`. (A deprecation shim hoisted them > automatically until it was removed on 2026-06-01.) +### `retry` (transient-error policy) + +`retry:` is a **top-level** task block, deliberately not under `run_limits:` — +`run_limits` holds caps that *abort* a run, while `retry` is how hard the run +tries to survive a transient error (an API 5xx, a rate limit, a sandbox setup +hiccup). + +```yaml +retry: + max_retries: 0 # fail fast — no retries on any category +``` + +```bash +coder-eval run tasks/my_task.yaml -D retry.max_retries=0 # same, from the CLI +``` + +The baseline is a per-error-category table in `errors/categories.py::RETRY_CONFIG`. +Every field here is optional and applies uniformly to the categories that are +*already* retryable; omitting a field keeps the built-in value. Which categories +are retryable at all is a property of the error, not the run: a non-retryable +category (auth, billing) is **never** made retryable by an override. + +| Field | Default | Meaning | +|-------|---------|---------| +| `max_retries` | per-category | Attempts after the first failure. `0` disables retries entirely. | +| `initial_delay` | per-category | Seconds before the first retry (exponential from there, plus jitter). | +| `backoff_multiplier` | per-category | Exponential backoff factor between attempts. | + +**Retries resume in place.** A retried `communicate()` re-sends the prompt into +the *same* sandbox and the *same* agent session — the working directory is not +reset, so files the failed attempt already wrote survive into the retry. The +crashed attempt's partial turn is also preserved on the result (for crash +forensics), which means trajectory-observing criteria (`command_executed`, +`commands_efficiency`, `skill_triggered`) see **both** attempts. Set +`max_retries: 0` when you need a clean single-attempt trajectory. + ### `expected_turns` (soft efficiency budget) `run_limits.expected_turns` is a **soft target**, not a cap: the run is never diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index 844c040a..717a89ea 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -35,7 +35,7 @@ coder-eval run tasks/hello_date.yaml --stream full # live LLM output | `--max-parallel, -j` | Concurrent tasks (default: 1) | | `--preservation-mode` | Sandbox persistence: `NONE` / `MOVE_ON_WRITE` / `DIRECT_WRITE`. Default is driver-derived (docker → `DIRECT_WRITE`, else `MOVE_ON_WRITE`); explicit value always wins. | | `--run-dir` | Custom run directory (default: timestamped in `runs/`) | -| `-D path=value` / `--set` | Override any resolved task-config field (`agent`/`run_limits`/`sandbox` roots), e.g. `-D run_limits.max_turns=30 -D agent.permission_mode=plan -D agent.sdk_options.effort=high`. Repeatable; schema-validated. This is the way to set permission mode, turn/timeout limits, tools, plugins, and SDK options. | +| `-D path=value` / `--set` | Override any resolved task-config field (`agent`/`run_limits`/`retry`/`sandbox` roots), e.g. `-D run_limits.max_turns=30 -D agent.permission_mode=plan -D agent.sdk_options.effort=high`. Repeatable; schema-validated. This is the way to set permission mode, turn/timeout limits, tools, plugins, and SDK options. | | `--model, -m` | Shorthand alias for `-D agent.model=…` (e.g., `claude-sonnet-4-20250514`) | | `--driver` | Shorthand alias for `-D sandbox.driver=…` (`tempdir` or `docker`) | | `--exclude-tags` | Skip tasks matching any of these tags (comma-separated) | diff --git a/src/coder_eval/errors/executor.py b/src/coder_eval/errors/executor.py index 8949715b..4f8617cc 100644 --- a/src/coder_eval/errors/executor.py +++ b/src/coder_eval/errors/executor.py @@ -5,9 +5,10 @@ from collections.abc import Awaitable, Callable from typing import Any -from .categories import RETRY_CONFIG, RetryConfig +from coder_eval.models import RetryPolicy + from .categorization import categorize_error -from .retry import get_error_tip, get_retry_delay, should_retry +from .retry import get_error_tip, get_retry_delay, resolve_retry_config, should_retry logger = logging.getLogger(__name__) @@ -19,6 +20,7 @@ async def execute_with_retry( context: dict[str, Any], max_attempts: int | None = None, on_attempt_error: Callable[[Exception, int], Awaitable[None]] | None = None, + retry_policy: RetryPolicy | None = None, ) -> Any: """Execute an operation with automatic retry on transient errors. @@ -41,6 +43,8 @@ async def execute_with_retry( invoked after every failed attempt (including the final non-retryable one), for draining ``agent.pending_turn`` and calling ``agent.discard_pending_turn()``. Callback exceptions are logged and swallowed so they cannot mask the original. + retry_policy: Per-run overrides for the built-in per-category retry policy + (``retry``). None = built-in defaults. Returns: Result from operation @@ -92,10 +96,10 @@ async def execute_with_retry( # Categorize error category = categorize_error(e, context) - config = RETRY_CONFIG.get(category, RetryConfig()) + config = resolve_retry_config(category, retry_policy) # Check if we should retry (handles both non-retryable categories and exhausted attempts) - if not should_retry(category, attempt): + if not should_retry(category, attempt, retry_policy): if config.max_retries == 0: logger.error(f"[{task_id}] {operation_name} failed (non-retryable): {category.value} - {e}") else: @@ -105,7 +109,7 @@ async def execute_with_retry( raise # Calculate backoff with jitter - delay = get_retry_delay(category, attempt) + delay = get_retry_delay(category, attempt, retry_policy) # Enhanced warning with actionable tip tip = get_error_tip(category) diff --git a/src/coder_eval/errors/retry.py b/src/coder_eval/errors/retry.py index fc3e49cf..332b1f4d 100644 --- a/src/coder_eval/errors/retry.py +++ b/src/coder_eval/errors/retry.py @@ -10,6 +10,8 @@ from datetime import datetime from typing import Any +from coder_eval.models import RetryPolicy + from .categories import ERROR_TIPS, RETRY_CONFIG, ErrorCategory, ErrorContext, RetryConfig from .categorization import categorize_error @@ -17,18 +19,35 @@ logger = logging.getLogger(__name__) -def should_retry(category: ErrorCategory, attempt: int) -> bool: +def resolve_retry_config(category: ErrorCategory, policy: RetryPolicy | None = None) -> RetryConfig: + """Look up the effective retry config for ``category``. + + Single seam between the built-in per-category table (``RETRY_CONFIG``) and + the run's optional ``retry`` overrides — every retry decision + (should we? how long?) resolves through here so the two can't disagree. + + A non-retryable category (``max_retries == 0``) is returned untouched: + whether an error class is worth retrying is a property of the error, not a + per-run knob (see ``RetryPolicy``). + """ + config = RETRY_CONFIG.get(category, RetryConfig()) + if policy is None or config.max_retries == 0: + return config + return config.model_copy(update=policy.overrides()) + + +def should_retry(category: ErrorCategory, attempt: int, policy: RetryPolicy | None = None) -> bool: """Determine if an error should be retried. Args: category: Error category attempt: Current attempt number (0-indexed) + policy: Optional per-run overrides (the task's `retry` block) Returns: True if retry should be attempted """ - config = RETRY_CONFIG.get(category, RetryConfig()) - return attempt < config.max_retries + return attempt < resolve_retry_config(category, policy).max_retries def compute_backoff(config: RetryConfig, attempt: int) -> float: @@ -52,15 +71,16 @@ def compute_backoff(config: RetryConfig, attempt: int) -> float: return base_delay + jitter -def get_retry_delay(category: ErrorCategory, attempt: int) -> float: +def get_retry_delay(category: ErrorCategory, attempt: int, policy: RetryPolicy | None = None) -> float: """Calculate delay before retry with exponential backoff and jitter. - Convenience wrapper around ``compute_backoff`` that looks up the + Convenience wrapper around ``compute_backoff`` that resolves the effective ``RetryConfig`` for the given error category. Args: category: Error category attempt: Current attempt number (0-indexed) + policy: Optional per-run overrides (the task's `retry` block) Returns: Delay in seconds @@ -71,8 +91,7 @@ def get_retry_delay(category: ErrorCategory, attempt: int) -> float: >>> get_retry_delay(ErrorCategory.AGENT_API_ERROR, 1) # doctest: +SKIP 10.8 # 10.0 base + 0.8 jitter """ - config = RETRY_CONFIG.get(category, RetryConfig()) - return compute_backoff(config, attempt) + return compute_backoff(resolve_retry_config(category, policy), attempt) def get_error_tip(category: ErrorCategory) -> str: @@ -140,6 +159,7 @@ def create_error_context( attempt: int, component: str | None = None, agent_name: str | None = None, + retry_policy: RetryPolicy | None = None, **kwargs: Any, ) -> dict[str, Any]: """Create comprehensive error context for reporting. @@ -153,6 +173,9 @@ def create_error_context( attempt: Attempt number (1-indexed) component: Component that failed (agent/sandbox/evaluator) agent_name: Agent name (optional) + retry_policy: Per-run overrides (the task's `retry` block). Must be the SAME policy the + executor ran under, so the persisted is_retryable / retry_delay_seconds + cannot contradict the retry decision that was actually taken. **kwargs: Additional context fields: - disk_usage_gb: Disk usage in GB - memory_usage_gb: Memory usage in GB @@ -198,8 +221,12 @@ def create_error_context( component=component, agent_name=agent_name, attempt_number=attempt, - is_retryable=should_retry(category, attempt_index), - retry_delay_seconds=get_retry_delay(category, attempt_index) if should_retry(category, attempt_index) else 0.0, + is_retryable=should_retry(category, attempt_index, retry_policy), + retry_delay_seconds=( + get_retry_delay(category, attempt_index, retry_policy) + if should_retry(category, attempt_index, retry_policy) + else 0.0 + ), disk_usage_gb=kwargs.get("disk_usage_gb"), memory_usage_gb=kwargs.get("memory_usage_gb"), agent_stdout=agent_stdout, diff --git a/src/coder_eval/models/__init__.py b/src/coder_eval/models/__init__.py index 82354063..b5e9705b 100644 --- a/src/coder_eval/models/__init__.py +++ b/src/coder_eval/models/__init__.py @@ -129,6 +129,9 @@ TurnRecord, ) +# Retry +from coder_eval.models.retry import RetryPolicy + # Routing from coder_eval.models.routing import ( ROUTE_NAMES, @@ -293,6 +296,8 @@ "JudgeVerdict", # Limits "RunLimits", + # Retry + "RetryPolicy", # Merge strategy "MERGE_STRATEGY_KEY", "APPEND_ORDER_KEY", diff --git a/src/coder_eval/models/experiment.py b/src/coder_eval/models/experiment.py index eefe638f..696ea25f 100644 --- a/src/coder_eval/models/experiment.py +++ b/src/coder_eval/models/experiment.py @@ -12,6 +12,7 @@ from coder_eval.models.limits import RunLimits from coder_eval.models.mutations import PromptMutation from coder_eval.models.results import ConfigLineageEntry, EvaluationResult +from coder_eval.models.retry import RetryPolicy from coder_eval.models.sandbox import SandboxConfig from coder_eval.models.tasks import PostRunCommand, PreRunCommand, TaskDefinition from coder_eval.models.templates import TemplateSource @@ -63,6 +64,13 @@ class ExperimentVariant(BaseModel): "value intact." ), ) + retry: RetryPolicy | None = Field( + default=None, + description=( + "Per-variant overrides for the task's retry block. Field-merge — keys absent " + "here leave the task-level value intact." + ), + ) driver: Literal["tempdir", "docker"] | None = Field( default=None, description=( @@ -126,6 +134,13 @@ class ExperimentDefaults(BaseModel): "Field-merge — task and variant layers override individual keys without replacing the block." ), ) + retry: RetryPolicy | None = Field( + default=None, + description=( + "Default retry policy overrides applied to all tasks under this experiment. " + "Field-merge — task and variant layers override individual keys without replacing the block." + ), + ) driver: Literal["tempdir", "docker"] | None = Field( default=None, description=( diff --git a/src/coder_eval/models/retry.py b/src/coder_eval/models/retry.py new file mode 100644 index 00000000..4129371f --- /dev/null +++ b/src/coder_eval/models/retry.py @@ -0,0 +1,66 @@ +"""Per-run retry policy overrides (top-level ``retry:`` block).""" + +from __future__ import annotations + +from pydantic import BaseModel, ConfigDict, Field + + +class RetryPolicy(BaseModel): + """Per-run overrides for the built-in retry policy. + + Lives at the task's top level (``retry:``), not under ``run_limits``: a + retry policy is not a cap that aborts a run, it is how hard the run tries + to survive a transient error. It is its own ``-D`` root — ``-D + retry.max_retries=0``. + + The baseline policy lives in ``errors/categories.py::RETRY_CONFIG`` — a + per-error-category table (how many times an API error / rate limit / sandbox + setup failure is worth retrying, and with what backoff). Which categories + are retryable at all is a property of the error, not of the run, so it stays + in that table. What varies per run is *appetite*: a local debug loop wants + ``max_retries: 0`` to fail fast, a flaky-network nightly wants more attempts + and a longer initial delay than the defaults. + + Every field is optional and applies uniformly to the categories that are + already retryable; ``None`` means "keep the built-in value". A category with + ``max_retries=0`` in the table (non-retryable, e.g. auth errors) is never + made retryable by an override here — overriding a semantic classification + from a run config would turn a deterministic failure into a slow one. + + The overrides are applied in ``errors/retry.py::resolve_retry_config`` (the + single seam every retry decision resolves through); this model stays a pure + data holder so ``models`` keeps no dependency on ``errors``. + """ + + model_config = ConfigDict(extra="forbid") + + max_retries: int | None = Field( + default=None, + ge=0, + description=( + "Override the retry count for every retryable error category. 0 disables retries " + "entirely (fail on first error). None = per-category built-in defaults." + ), + ) + initial_delay: float | None = Field( + default=None, + gt=0.0, + description="Override the first backoff delay in seconds for every retryable category. None = built-in.", + ) + backoff_multiplier: float | None = Field( + default=None, + gt=0.0, + description="Override the exponential backoff multiplier for every retryable category. None = built-in.", + ) + + def overrides(self) -> dict[str, float | int]: + """The non-None overrides as a ``RetryConfig.model_copy(update=...)`` dict.""" + return { + name: value + for name, value in ( + ("max_retries", self.max_retries), + ("initial_delay", self.initial_delay), + ("backoff_multiplier", self.backoff_multiplier), + ) + if value is not None + } diff --git a/src/coder_eval/models/tasks.py b/src/coder_eval/models/tasks.py index c566d73d..ae22d0be 100644 --- a/src/coder_eval/models/tasks.py +++ b/src/coder_eval/models/tasks.py @@ -13,6 +13,7 @@ from coder_eval.models.enums import AgentKind from coder_eval.models.limits import RunLimits from coder_eval.models.merge_strategy import MergeField +from coder_eval.models.retry import RetryPolicy from coder_eval.models.sandbox import SandboxConfig @@ -370,6 +371,14 @@ class TaskDefinition(BaseModel): # noqa: CE009 -- soft-launch: see _warn_on_unk "Run-time caps (turns, wall-clock, tokens, USD) that abort the task when exceeded. See RunLimits." ), ) + retry: RetryPolicy | None = Field( + default=None, + description=( + "Per-run overrides for the built-in retry policy (attempts / backoff on transient " + "agent + sandbox errors). None = built-in per-category defaults. Set " + "`-D retry.max_retries=0` to fail fast while debugging. See RetryPolicy." + ), + ) reference: ReferenceSource | None = Field( default=None, description=( diff --git a/src/coder_eval/orchestration/config_merge.py b/src/coder_eval/orchestration/config_merge.py index 77c2355b..2ffc6aff 100644 --- a/src/coder_eval/orchestration/config_merge.py +++ b/src/coder_eval/orchestration/config_merge.py @@ -1,7 +1,7 @@ """The single generic config-merge resolver. -One implementation that merges any of the three ``-D``-reachable root models -(``agent`` / ``run_limits`` / ``sandbox``) across an ordered list of partial-dict +One implementation that merges any of the four ``-D``-reachable root models +(``agent`` / ``run_limits`` / ``retry`` / ``sandbox``) across an ordered list of partial-dict :class:`Layer` records, applying each field's declared merge strategy (:func:`coder_eval.models.merge_strategy.merge_strategy_of`) uniformly at every layer. Both the layers-1-4 path (``resolve_task_for_variant``) and the layer-5 @@ -32,6 +32,7 @@ ClaudeCodeAgentConfig, CodexAgentConfig, ConfigLineageEntry, + RetryPolicy, RunLimits, SandboxConfig, TaskDefinition, @@ -51,9 +52,9 @@ "cli", ] -ALLOWED_OVERRIDE_ROOTS: tuple[str, ...] = ("agent", "run_limits", "sandbox") +ALLOWED_OVERRIDE_ROOTS: tuple[str, ...] = ("agent", "run_limits", "retry", "sandbox") -RootName = Literal["agent", "run_limits", "sandbox"] +RootName = Literal["agent", "run_limits", "retry", "sandbox"] class MergeError(ValueError): @@ -101,7 +102,8 @@ def _root_model_types(root: str) -> tuple[type[BaseModel], ...]: plugin) plus ``BaseAgentConfig`` (so subclass-only fields like ``sdk_options`` and plugin fields are recognized for ``-D`` validation / did-you-mean) — read from the registry after plugin load, not a static annotation. ``run_limits`` / - ``sandbox`` derive from ``TaskDefinition`` via :func:`classify_annotation`. + ``retry`` / ``sandbox`` derive from ``TaskDefinition`` via + :func:`classify_annotation`. Raises :class:`MergeError` for an unknown root. """ if root not in ALLOWED_OVERRIDE_ROOTS: @@ -357,6 +359,13 @@ def resolve_root( """Resolve the ``run_limits`` root (None when no layer set anything).""" +@overload +def resolve_root( + root: Literal["retry"], layers: Sequence[Layer], *, lineage: dict[str, ConfigLineageEntry] | None = ... +) -> RetryPolicy | None: + """Resolve the ``retry`` root (None when no layer set anything).""" + + @overload def resolve_root( root: Literal["sandbox"], layers: Sequence[Layer], *, lineage: dict[str, ConfigLineageEntry] | None = ... @@ -375,13 +384,16 @@ def resolve_root( The single merge-and-build entry point both resolution paths call. Value validation (``extra="forbid"``, field validators, SandboxConfig's template-sources model_validator) happens in the constructor. Returns None - when no layer set anything for ``run_limits`` (an empty block is dropped).""" + when no layer set anything for ``run_limits`` / ``retry`` (an empty block is + dropped).""" model_types = _root_model_types(root) # raises MergeError for an unknown root merged = merge_layers(model_types, layers, lineage_root=root, lineage=lineage) if root == "agent": return parse_agent_config(**merged) if root == "run_limits": return RunLimits(**merged) if merged else None + if root == "retry": + return RetryPolicy(**merged) if merged else None # root == "sandbox" — the only remaining RootName member. return SandboxConfig(**merged) if merged else None diff --git a/src/coder_eval/orchestration/experiment.py b/src/coder_eval/orchestration/experiment.py index f84a4a59..fe37cf31 100644 --- a/src/coder_eval/orchestration/experiment.py +++ b/src/coder_eval/orchestration/experiment.py @@ -25,6 +25,7 @@ ExperimentVariant, FinalStatus, ResolvedTask, + RetryPolicy, RunLimits, SimulationConfig, SkippedTask, @@ -366,7 +367,7 @@ def resolve_task_for_variant( variant_agent_clean = variant.agent task_agent = task.agent.model_dump(exclude_unset=True) if task.agent else None - # Resolve all three `-D`-reachable roots through the SAME generic resolver + # Resolve all four `-D`-reachable roots through the SAME generic resolver # the CLI layer uses (config_merge.resolve_root) — one merge implementation, # one set of per-field strategies, lineage emitted as a side effect into the # shared `lineage` dict. Type is enforced after CLI overrides (layer 5) so @@ -412,6 +413,24 @@ def _add_rl(rl: RunLimits | None, source: ConfigSource) -> None: _add_rl(variant.run_limits, "variant") resolved_run_limits = resolve_root("run_limits", rl_layers, lineage=lineage) + # --- retry: same 4 layers, its own top-level root (a retry policy is not a cap). --- + retry_layers: list[Layer] = [] + + def _add_retry(policy: RetryPolicy | None, source: ConfigSource) -> None: + if policy is None: + return + patch = policy.model_dump(exclude_unset=True) + if patch: + retry_layers.append(Layer(source=source, patch=patch)) + + if default_experiment.defaults: + _add_retry(default_experiment.defaults.retry, "default") + if experiment.defaults: + _add_retry(experiment.defaults.retry, "experiment-defaults") + _add_retry(task.retry, "task") + _add_retry(variant.retry, "variant") + resolved_retry = resolve_root("retry", retry_layers, lineage=lineage) + # --- sandbox: synthetic-layer construction (driver precedence + task-first # template_sources ordering) lives in _build_sandbox_layers. --- sandbox_layers = _build_sandbox_layers(default_experiment, experiment, task, variant) @@ -452,6 +471,7 @@ def _add_rl(rl: RunLimits | None, source: ConfigSource) -> None: update={ "agent": resolved_agent, "run_limits": resolved_run_limits, + "retry": resolved_retry, "sandbox": resolved_sandbox, "post_run": resolved_post_run, "pre_run": resolved_pre_run, diff --git a/src/coder_eval/orchestration/overrides.py b/src/coder_eval/orchestration/overrides.py index de1f55d9..08ef1070 100644 --- a/src/coder_eval/orchestration/overrides.py +++ b/src/coder_eval/orchestration/overrides.py @@ -4,7 +4,7 @@ 1. parses a scalar value (YAML-safe, with the YAML-1.1 truthy-alias guard), 2. groups dotted ``path=value`` overrides into one nested ``-D`` patch per - root (``agent`` / ``run_limits`` / ``sandbox``), + root (``agent`` / ``run_limits`` / ``retry`` / ``sandbox``), 3. calls :func:`config_merge.resolve_root` with a lineage-silent value seed (the already-resolved model) + the ``-D`` patch layer, so a field merges under exactly the same strategy at layer 5 as at the variant layer. @@ -114,8 +114,9 @@ def apply_overrides( """ agent_patch: dict[str, Any] = {} rl_patch: dict[str, Any] = {} + retry_patch: dict[str, Any] = {} sandbox_patch: dict[str, Any] = {} - by_root = {"agent": agent_patch, "run_limits": rl_patch, "sandbox": sandbox_patch} + by_root = {"agent": agent_patch, "run_limits": rl_patch, "retry": retry_patch, "sandbox": sandbox_patch} for path, value in overrides.items(): root, _, rest = path.partition(".") @@ -153,6 +154,10 @@ def apply_overrides( seed = task.run_limits.model_dump(exclude_unset=True) if task.run_limits else {} task.run_limits = _resolve("run_limits", seed, rl_patch, detail=None, lineage=lineage) + if retry_patch: + seed = task.retry.model_dump(exclude_unset=True) if task.retry else {} + task.retry = _resolve("retry", seed, retry_patch, detail=None, lineage=lineage) + if sandbox_patch: # Full dump: the resolved sandbox already folded layers 1-4 (incl. the # append-semantics env_passthrough_extra/template_sources merges), so the diff --git a/src/coder_eval/orchestrator.py b/src/coder_eval/orchestrator.py index b1581470..efc5868a 100644 --- a/src/coder_eval/orchestrator.py +++ b/src/coder_eval/orchestrator.py @@ -41,6 +41,7 @@ PostRunResult, PreRunCommand, PreservationMode, + RetryPolicy, SimulationConfig, SimulationTelemetry, TaskConfigRecord, @@ -513,6 +514,7 @@ def _kill_agent_subprocess_sync() -> None: attempt=max(self.result.iteration_count, 1), component="orchestrator.task_timeout", agent_name=self._agent_name, + retry_policy=self._retry_policy, ) logger.error(f"Task timed out: {e}") @@ -532,6 +534,7 @@ def _kill_agent_subprocess_sync() -> None: attempt=max(self.result.iteration_count, 1), component=component, agent_name=self._agent_name, + retry_policy=self._retry_policy, ) logger.warning(f"Run limit exceeded: {e}") except Exception as e: @@ -552,6 +555,7 @@ def _kill_agent_subprocess_sync() -> None: attempt=max(self.result.iteration_count, 1), # Actual iteration attempt (1-indexed) component=failed_component, agent_name=self._agent_name, + retry_policy=self._retry_policy, ) logger.error(f"Evaluation failed: {e}", exc_info=True) @@ -632,6 +636,7 @@ def _finalize_result(self, start_time: float) -> None: attempt=max(self.result.iteration_count, 1), component="orchestrator.finalize.weighted_score", agent_name=self._agent_name, + retry_policy=self._retry_policy, ) # Command statistics @@ -866,6 +871,11 @@ def _aggregate_token_usage(self) -> None: total_cost_usd=sum(costs) if costs else None, ) + @property + def _retry_policy(self) -> RetryPolicy | None: + """The task's retry overrides (top-level ``retry:``), or None for built-in defaults.""" + return self.task.retry + async def _setup(self) -> None: """Set up all components for evaluation. @@ -949,6 +959,7 @@ async def _setup_sandbox() -> Any: operation=_setup_sandbox, operation_name="Sandbox setup", context={"task_id": self.task.task_id, "component": "sandbox"}, + retry_policy=self._retry_policy, ) # Assert result is initialized (set in run()) @@ -982,6 +993,7 @@ async def _start_agent() -> None: operation=_start_agent, operation_name="Agent start", context={"task_id": self.task.task_id, "component": "agent", "agent_name": self._agent_name}, + retry_policy=self._retry_policy, ) # Save agent config on result (copy to prevent mutation of shared reference) @@ -1269,6 +1281,7 @@ async def _communicate_attempt() -> TurnRecord: "agent_name": self._agent_name, }, on_attempt_error=_on_attempt_failure, + retry_policy=self._retry_policy, ) assert turn_record is not None # execute_with_retry returns the turn or raises return turn_record diff --git a/tests/test_error_handling.py b/tests/test_error_handling.py index db1b9692..b199b67d 100644 --- a/tests/test_error_handling.py +++ b/tests/test_error_handling.py @@ -12,7 +12,14 @@ from coder_eval.errors.categories import ERROR_TIPS, RETRY_CONFIG, ErrorCategory, RetryConfig from coder_eval.errors.categorization import categorize_error from coder_eval.errors.executor import execute_with_retry -from coder_eval.errors.retry import create_error_context, should_retry, truncate_log +from coder_eval.errors.retry import ( + create_error_context, + get_retry_delay, + resolve_retry_config, + should_retry, + truncate_log, +) +from coder_eval.models import RetryPolicy class TestErrorCategory: @@ -574,6 +581,88 @@ async def test_execute_with_retry_waits_between_retries(self): assert 3.75 <= sleep_duration <= 6.25 +class TestRetryPolicyOverrides: + """The top-level `retry:` block overrides the built-in per-category policy.""" + + def test_no_policy_keeps_builtin_config(self): + assert resolve_retry_config(ErrorCategory.AGENT_API_ERROR) == RETRY_CONFIG[ErrorCategory.AGENT_API_ERROR] + + def test_policy_overrides_only_the_fields_it_sets(self): + resolved = resolve_retry_config(ErrorCategory.AGENT_API_ERROR, RetryPolicy(max_retries=7)) + builtin = RETRY_CONFIG[ErrorCategory.AGENT_API_ERROR] + + assert resolved.max_retries == 7 + assert resolved.initial_delay == builtin.initial_delay + assert resolved.backoff_multiplier == builtin.backoff_multiplier + + def test_policy_cannot_make_a_non_retryable_category_retryable(self): + """Retryability is a property of the error class, not a per-run knob.""" + resolved = resolve_retry_config(ErrorCategory.AGENT_AUTH_ERROR, RetryPolicy(max_retries=9)) + assert resolved.max_retries == 0 + + def test_should_retry_and_delay_honor_the_policy(self): + policy = RetryPolicy(max_retries=1, initial_delay=1.0) + + assert should_retry(ErrorCategory.AGENT_API_ERROR, 0, policy) + assert not should_retry(ErrorCategory.AGENT_API_ERROR, 1, policy) # built-in would allow (3 retries) + assert 1.0 <= get_retry_delay(ErrorCategory.AGENT_API_ERROR, 0, policy) <= 1.25 + + @pytest.mark.asyncio + async def test_max_retries_zero_fails_fast_on_a_retryable_error(self): + """The debug-loop case: `-D retry.max_retries=0` disables retries.""" + mock_operation = AsyncMock(side_effect=Exception("503 Service Unavailable")) + + with ( + patch("asyncio.sleep", new_callable=AsyncMock) as mock_sleep, + pytest.raises(Exception, match="503 Service Unavailable"), + ): + await execute_with_retry( + operation=mock_operation, + operation_name="Test operation", + context={"component": "agent"}, + retry_policy=RetryPolicy(max_retries=0), + ) + + assert mock_operation.call_count == 1 + assert mock_sleep.call_count == 0 + + def test_error_context_reflects_the_run_policy(self): + """Persisted error_details must not contradict the executor's actual retry decision.""" + error = Exception("network connection error") + + default_ctx = create_error_context(error=error, task_id="t", attempt=1, component="agent") + assert default_ctx["is_retryable"] is True + assert default_ctx["retry_delay_seconds"] > 0 + + # Same error under a fail-fast run: the executor would NOT retry, so + # task.json must say so too. + no_retry_ctx = create_error_context( + error=error, + task_id="t", + attempt=1, + component="agent", + retry_policy=RetryPolicy(max_retries=0), + ) + assert no_retry_ctx["is_retryable"] is False + assert no_retry_ctx["retry_delay_seconds"] == 0.0 + + @pytest.mark.asyncio + async def test_policy_raises_the_attempt_ceiling(self): + """A policy above the built-in 3 retries keeps trying (and eventually succeeds).""" + mock_operation = AsyncMock(side_effect=[Exception("503 Service Unavailable")] * 4 + ["success"]) + + with patch("asyncio.sleep", new_callable=AsyncMock): + result = await execute_with_retry( + operation=mock_operation, + operation_name="Test operation", + context={"component": "agent"}, + retry_policy=RetryPolicy(max_retries=5), + ) + + assert result == "success" + assert mock_operation.call_count == 5 + + class TestCreateErrorContext: """Test error context creation and reporting.""" diff --git a/tests/test_merge_unification.py b/tests/test_merge_unification.py index a19e39c1..e8eb931d 100644 --- a/tests/test_merge_unification.py +++ b/tests/test_merge_unification.py @@ -3,7 +3,7 @@ Two gates: 1. **Unification invariant** — for a representative patch P, the resolved - ``agent``/``run_limits``/``sandbox`` is identical whether P arrives as a + ``agent``/``run_limits``/``retry``/``sandbox`` is identical whether P arrives as a higher config layer (layers 1-4) or as a ``-D`` override (layer 5). If this fails, the two paths are not one engine. 2. **Lineage parity** — applying a ``-D`` touching one field leaves every other @@ -20,6 +20,7 @@ ExperimentDefinition, ExperimentVariant, ResourceLimits, + RetryPolicy, RunLimits, SandboxConfig, TaskDefinition, @@ -87,6 +88,23 @@ def test_run_limits_max_turns(self): apply_overrides(b, {"run_limits.max_turns": 5}) assert a.run_limits.model_dump() == b.run_limits.model_dump() + def test_retry_max_retries(self): + a, _ = _resolve(_task(), variant=ExperimentVariant(variant_id="v", retry=RetryPolicy(max_retries=0))) + b, _ = _resolve(_task()) + apply_overrides(b, {"retry.max_retries": 0}) + assert a.retry is not None and b.retry is not None + assert a.retry.model_dump() == b.retry.model_dump() + assert a.retry.max_retries == 0 + + def test_retry_field_merge_keeps_lower_layer_keys(self): + """A variant setting one retry key must not replace the task's whole block.""" + task = _task(retry=RetryPolicy(initial_delay=9.0)) + a, _ = _resolve(task, variant=ExperimentVariant(variant_id="v", retry=RetryPolicy(max_retries=1))) + b, _ = _resolve(_task(retry=RetryPolicy(initial_delay=9.0))) + apply_overrides(b, {"retry.max_retries": 1}) + assert a.retry.model_dump() == b.retry.model_dump() + assert (a.retry.max_retries, a.retry.initial_delay) == (1, 9.0) + def test_sandbox_driver(self): a, _ = _resolve(_task(), variant=ExperimentVariant(variant_id="v", driver="docker")) b, _ = _resolve(_task()) diff --git a/tests/test_overrides_engine.py b/tests/test_overrides_engine.py index 6a1753f5..d19f0745 100644 --- a/tests/test_overrides_engine.py +++ b/tests/test_overrides_engine.py @@ -17,6 +17,7 @@ ConfigLineageEntry, DockerDriverConfig, FileExistsCriterion, + RetryPolicy, RunLimits, SandboxConfig, TaskDefinition, @@ -33,6 +34,7 @@ def _make_task( agent: object | None = None, run_limits: RunLimits | None = None, + retry: RetryPolicy | None = None, sandbox: SandboxConfig | None = None, ) -> TaskDefinition: return TaskDefinition( @@ -43,6 +45,7 @@ def _make_task( sandbox=sandbox if sandbox is not None else SandboxConfig(driver="tempdir"), success_criteria=[FileExistsCriterion(description="x", path="f.txt")], run_limits=run_limits, + retry=retry, ) @@ -152,6 +155,24 @@ def test_sdk_options_with_agent_type_codex_raises(self): with pytest.raises(OverrideError, match="only supported for claude-code"): apply_overrides(task, {"agent.sdk_options.effort": "high"}, agent_type="codex") + def test_retry_root_override(self): + """`-D retry.max_retries=0` — the fail-fast debug knob, on its own root.""" + task = _make_task() + assert task.retry is None + apply_overrides(task, {"retry.max_retries": 0}) + assert task.retry is not None + assert task.retry.max_retries == 0 + + def test_retry_root_field_merge_preserves_seed(self): + task = _make_task(retry=RetryPolicy(initial_delay=7.5)) + apply_overrides(task, {"retry.max_retries": 2}) + assert (task.retry.max_retries, task.retry.initial_delay) == (2, 7.5) + + def test_retry_unknown_key_raises(self): + task = _make_task() + with pytest.raises(OverrideError, match="did you mean 'max_retries'"): + apply_overrides(task, {"retry.max_retry": 1}) + def test_unknown_root_raises(self): task = _make_task() with pytest.raises(OverrideError, match="unknown override root 'foo'"):