diff --git a/.claude/harness-candidates.md b/.claude/harness-candidates.md index a47e5b44..434cc186 100644 --- a/.claude/harness-candidates.md +++ b/.claude/harness-candidates.md @@ -457,14 +457,21 @@ with the two `action.yml` items above — one considered change to the action's depend on, mirroring how CE030 pins doc/schema parity. ## From the split-field / optimize-skill plan (2026-08-12) -- [ ] **A run whose every task is skipped exits 0 — a green run of zero tasks.** When - `resolve_all_tasks` demotes every task to `skipped_tasks` (a load failure, `skip: true`, - or now a `--split` selector matching no labelled row), the run reports success: nothing +- [ ] **A run whose every task is skipped exits 0 — a green run of zero tasks.** *(Narrow case + CLOSED 2026-08-13: a `--split` selector that matches no labelled row now raises + `SplitSelectorError` out of `expand_dataset`, which `resolve_all_tasks` re-raises and the CLI + turns into a `typer.BadParameter` — exit 2. The GENERAL case below stays open: `skip: true`, + load failures and tag filters that match nothing all keep today's exit-0 behaviour, because + making those fatal changes exit semantics for deliberate quarantine workflows and needs its own + decision plus tests per case.)* When + `resolve_all_tasks` demotes every task to `skipped_tasks` (a load failure or `skip: true` — + no longer a `--split` typo, see above), the run reports success: nothing failed, so the exit gate in `cli/run_command.py` — which keys only on failed/errored tasks - and suite gates — passes. Verified directly: `coder-eval run --split holdou` - prints one yellow "1 task file(s) skipped" line and exits 0. This is pre-existing, but - `--split` makes it reachable by a one-character CLI typo rather than a broken file, and - the whole point of a test confirmation is that you trust its verdict. Not guarded, and + and suite gates — passes. Verified directly before the narrow fix: `coder-eval run + --split holdou` printed one yellow "1 task file(s) skipped" line and exited 0. `--split` was + what made it reachable by a one-character CLI typo rather than a broken file, and + the whole point of a test confirmation is that you trust its verdict. Still unguarded for + the remaining paths, and not a five-minute fix: making an all-skipped run non-green changes exit semantics for every skipped-task path (including deliberate `skip: true` suites and tag filters that match nothing), so it needs a decision about which of those should be fatal, plus tests @@ -474,3 +481,7 @@ with the two `action.yml` items above — one considered change to the action's - [ ] **Semantic answer-leak in a task prompt** — a prompt that describes the graded behaviour in *different words* ("list the paths explicitly rather than with a recursive wildcard" while grading an explicit glob) scores well whether or not the behaviour happened, and in an A/B an arm that deleted the rule still passes. CE061 catches only the verbatim form; the semantic form needs an LLM judge or a `lint-tasks` pass over this repo's own `tasks/`, neither of which is cheap or deterministic. — caught in the final review of c/2026-08-13-optimize-skill-fixes.md, where 4 of 10 rows in a shipped worked example had it. - [ ] **A doc claim that contradicts merge semantics** — `optimize-skill` told users to declare `allowed_tools` in an experiment's `defaults: agent:`, which is a silent no-op because those fields merge by `replace` and the task layer outranks experiment defaults. Detecting "this prose recommends a config location that the merge order makes ineffective" would need the rule to model the layer stack against prose, which no existing rule shape supports. — caught in the final review of c/2026-08-13-optimize-skill-fixes.md. - [x] **`_normalized()` not used by every prose sensor** — CLOSED: all 9 sites converted, and `test_no_sensor_inlines_the_normalization_idiom` now forbids the raw form. Original note: — 8 sensors in `tests/test_custom_lint.py` still inline `" ".join(path.read_text().split())`, so a future one copied from the wrong neighbour is defeated by a line wrap (the bug that let a stale skill count ship past 91 green tests). A rule forbidding the raw idiom in that file is easy; the conversion sweep was out of scope. — caught in the final review of c/2026-08-13-optimize-skill-fixes.md. + +## From the optimize-skill review v2 plan (2026-08-13) + +- [ ] **"The ToolStart seam decides" is now a PER-CRITERION property, not a global invariant.** `command_executed`'s verdict is decidable from the tool call's inputs; `skill_triggered`'s is not (for the `Skill` tool the body is delivered AS the result, so an in-flight call engaged nothing). A new `LiveSuccessCriterion` must state which seam its `live_verdict` is decidable at, and a criterion that decides at the ToolStart on information only the result carries silently diverges from its own frozen check. Not mechanically detectable today: the property is about what a `live_verdict` implementation *reads*, which no AST rule can infer — a rule would have to know that `result_status` is the field distinguishing the two seams. A cheaper partial guard would be a test-level convention (every live criterion has a "not decided before the result" or "decided on the call" test), which is a sweep rather than a rule. — caught implementing Phase 1 of c/2026-08-13-optimize-skill-review-v2-fixes.md. diff --git a/CLAUDE.md b/CLAUDE.md index ca904a97..c5220f35 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -90,7 +90,7 @@ coder_eval/ ├── cli/ # CLI commands (Typer + Rich) │ ├── __init__.py # Typer app setup (core commands) │ ├── run_command.py # `coder-eval run` -│ ├── plan_command.py # `coder-eval plan` +│ ├── plan_command.py # `coder-eval plan` (also expands datasets: previews total/selected row counts, takes `--split`, warns on partial labelling) │ ├── report_command.py # `coder-eval report` │ ├── run_helpers.py # CLI helper functions │ ├── console.py # Rich console instance @@ -124,7 +124,7 @@ tests/ # Test suite docs/ # Documentation templates/ # Sandbox template directories .claude-plugin/marketplace.json # Makes this repo a Claude Code plugin marketplace (`/plugin marketplace add UiPath/coder_eval`); lists the one plugin below. -plugins/coder-eval/ # The published Claude Code plugin: `.claude-plugin/plugin.json` (its `version` is a derived pin of pyproject's, bumped by release.yml, guarded by tests/test_action_version_pin.py), `skills//SKILL.md` × 7 (`/coder-eval:init`, `/coder-eval:check-skill`, `/coder-eval:optimize-skill`, `/coder-eval:task`, `/coder-eval:lint-tasks`, `/coder-eval:analyze`, `/coder-eval:ci`), and `reference/` — everything a skill reads must live here, since an installed plugin is copied to ~/.claude/plugins/cache/ WITHOUT its parent dirs (address it via `${CLAUDE_PLUGIN_ROOT}`). `reference/criteria.md` is generated (`make plugin-reference`, CE033); `reference/run-layout.md` is a verbatim mirror of `.claude/shared/run-layout.md`; `reference/templates/` holds the two suites the skills copy into a user's repo — `activation.yaml` (+ rows) that `check-skill` writes, and `outcome.yaml` (+ rows) that `optimize-skill`'s execution track starts from; both are loader-backed-tested in `tests/test_custom_lint.py`. `reference/task-rubric.md` is the shared task-quality rubric that `task` and `lint-tasks` both read (plugin-only — no repo-side twin); `reference/repo-layout.md` is the eval-tree DISCOVERY policy every skill reads (`SKILL_NEEDS_EVAL_ROOT_DISCOVERY`, which a new skill must declare a stance in) — glob for `task_id:` files and `run.json`, never assume `tasks/`/`runs/latest` — as distinct from `run-layout.md`, which describes what is inside a run directory. Every skill must appear in all four surfaces in `SKILL_DOC_SURFACES` (derived test), and their combined frontmatter `description` length is capped (`SKILL_LISTING_BUDGET_CHARS`) because the skill listing's budget is shared with every skill the user has installed. **Skill naming is verb-first imperative** — a skill is a command you issue (`/coder-eval:`) and every one of them takes an action, so name it for the action: a bare verb where that is unambiguous (`init`, `analyze` — the object comes from the argument), otherwise `-` (`lint-tasks`, `check-skill`, `optimize-skill`). Never `-`: `skill-check` was renamed to `check-skill` precisely because it read backwards next to `lint-tasks`, and `optimize-skill` was named that way rather than `skill-optimize` for the same reason — the rule is applied at authoring time now, not repaired later. `task` and `ci` predate the rule and stay — renaming a published skill breaks every user's muscle memory for no functional gain, since activation keys on the `description`, never the name. Distinct from `.claude/commands/`, which stays repo-local contributor tooling. +plugins/coder-eval/ # The published Claude Code plugin: `.claude-plugin/plugin.json` (its `version` is a derived pin of pyproject's, bumped by release.yml, guarded by tests/test_action_version_pin.py), `skills//SKILL.md` × 7 (`/coder-eval:init`, `/coder-eval:check-skill`, `/coder-eval:optimize-skill`, `/coder-eval:task`, `/coder-eval:lint-tasks`, `/coder-eval:analyze`, `/coder-eval:ci`), and `reference/` — everything a skill reads must live here, since an installed plugin is copied to ~/.claude/plugins/cache/ WITHOUT its parent dirs (address it via `${CLAUDE_PLUGIN_ROOT}`). `reference/criteria.md` is generated (`make plugin-reference`, CE033); `reference/run-layout.md` is a verbatim mirror of `.claude/shared/run-layout.md`; `reference/templates/` holds the two suites the skills copy into a user's repo — `activation.yaml` (+ rows) that `check-skill` writes, and `outcome.yaml` (+ rows) that `optimize-skill`'s execution track starts from; both are loader-backed-tested in `tests/test_custom_lint.py`. `reference/task-rubric.md` is the shared task-quality rubric that `task` and `lint-tasks` both read (plugin-only — no repo-side twin); `reference/optimize-method.md` is the track-invariant method behind `optimize-skill`'s three stages (cost table, what each stage bounds, why the two gates differ, the paired-diff sign rule) — one reader, `optimize-skill`, which keeps the procedure and points at it via `${CLAUDE_PLUGIN_ROOT}`; the prose sensor asserts procedure tokens against `SKILL.md` alone and method tokens against the pair; `reference/repo-layout.md` is the eval-tree DISCOVERY policy every skill reads (`SKILL_NEEDS_EVAL_ROOT_DISCOVERY`, which a new skill must declare a stance in) — glob for `task_id:` files and `run.json`, never assume `tasks/`/`runs/latest` — as distinct from `run-layout.md`, which describes what is inside a run directory. Every skill must appear in all four surfaces in `SKILL_DOC_SURFACES` (derived test), and their combined frontmatter `description` length is capped (`SKILL_LISTING_BUDGET_CHARS`) because the skill listing's budget is shared with every skill the user has installed. **Skill naming is verb-first imperative** — a skill is a command you issue (`/coder-eval:`) and every one of them takes an action, so name it for the action: a bare verb where that is unambiguous (`init`, `analyze` — the object comes from the argument), otherwise `-` (`lint-tasks`, `check-skill`, `optimize-skill`). Never `-`: `skill-check` was renamed to `check-skill` precisely because it read backwards next to `lint-tasks`, and `optimize-skill` was named that way rather than `skill-optimize` for the same reason — the rule is applied at authoring time now, not repaired later. `task` and `ci` predate the rule and stay — renaming a published skill breaks every user's muscle memory for no functional gain, since activation keys on the `description`, never the name. Distinct from `.claude/commands/`, which stays repo-local contributor tooling. action.yml # Published composite GitHub Action (coder-eval as a CI gate). release.yml's `release` job maintains its `version:` default; its `promote` job (gated on publish-pypi) moves the `v` tag + cuts the Release, so nothing consumer-visible moves before the wheel is on PyPI. verify-published-action.yml then verifies the published composite (tag/pin/PyPI/Marketplace parity, plus a real consumer run) after each Release and nightly. Runbook: CONTRIBUTING.md § Releasing. ``` @@ -139,7 +139,7 @@ action.yml # Published composite GitHub Action (coder-ev - **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. - **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 selection is filter-then-sample: CLI `--split ` (keep only rows whose `dataset.split_field` value matches, default field `split`) runs **first** and is orthogonal to the sampler win-order — a row is unlabelled when the field is absent/`null`/`""` and a task whose rows are all unlabelled passes through unfiltered; partial labelling drops the unlabelled rows. A *labelled* task with no matching row raises a `ValueError` listing the splits that exist — which `resolve_all_tasks` catches into `skipped_tasks` like any load failure, so a mistyped selector yields a zero-task run that still exits 0. Filtering before sampling is a correctness requirement: sampling first would leave an unpredictable (possibly zero) number of rows per split, destroying the train/test comparison. Then 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.) +- **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 selection is filter-then-sample: CLI `--split ` (keep only rows whose `dataset.split_field` value matches, default field `split`) runs **first** and is orthogonal to the sampler win-order — a row is unlabelled when the field is absent/`null`/`""` and a task whose rows are all unlabelled passes through unfiltered; partial labelling drops the unlabelled rows. A *labelled* task with no matching row raises `SplitSelectorError` (a `ValueError` subclass in `task_loader.py`) listing the splits that exist — and `resolve_all_tasks` **re-raises** this one rather than demoting it to `skipped_tasks`, so a mistyped selector aborts the run with a non-zero exit instead of yielding a green zero-task run. It is the one dataset error treated that way: the others describe a malformed FILE (one bad task must not abort a suite), this one a malformed INVOCATION applied to every task in the run. Partial labelling stays legal but is no longer silent — `expand_dataset` logs a WARNING naming the drop count, and `coder-eval plan --split ` shows it pre-spend. Filtering before sampling is a correctness requirement: sampling first would leave an unpredictable (possibly zero) number of rows per split, destroying the train/test comparison. Then 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. - **Sub-agent token accounting**: There is NO separate per-sub-agent field. Every sub-agent generation is captured as a `parent_tool_use_id`-tagged `AssistantMessage` in the turn transcript, so per-sub-agent usage is derived by grouping those messages on that id (the evalboard's `aggregateSubAgentUsage` does exactly this). Claude bubbles its sub-agent's intermediate generations into the parent stream natively, and the **terminal** generation (delivered as the Agent tool result, never streamed) is synthesized into one via `_synthesize_subagent_terminal_message` from `tool_use_result.usage`. Codex reconstructs all child generations from the child rollout (`_recover_subagent_tool_calls`). The turn total already includes sub-agent cost — Claude via the SDK's cumulative `model_usage`; Codex via `_fold_subagent_tokens`, which folds the child messages (their real per-generation tokens) into the parent total. `CommandTelemetry.result_summary` is stored **untruncated** (no 200-char cap) so sub-agent returns are preserved whole. Set `CODER_EVAL_RAW_SDK_LOG=1` to dump every raw SDK event to the task log for inspection. - **Reconciliation message (stream self-reconciles to the turn total)**: The per-message stream consistently under-reports the authoritative turn total — a fixed prompt slice (~512 input tokens on Claude) is billed on no SDK-emitted message, and sub-agent input/cache only partially bubbles up. So `EventCollector.build_turn_record` appends one synthetic `ReconciliationMessage` (`role="reconciliation"`, in the `TranscriptMessage` union) per turn, carrying the per-bucket residual = `token_usage` − Σ(assistant message buckets). The invariant: **summing the four token buckets across `TurnRecord.messages` (assistant + reconciliation) equals `token_usage` exactly**, for both Claude and Codex (Codex's stream is already complete after `_recover_subagent_tool_calls`, so its residual is usually 0 and no entry is emitted). This is what lets the evalboard SUM the message stream as the source of truth instead of reading a separate aggregate ("agent tokens"): `selectTokenTotals` returns the stream sum whenever a reconciliation entry is present, and the timeline renders it as its own row. It is agent-agnostic (booked at the single `EventCollector` seam), carries no cost (cost stays on `token_usage`), and is excluded from generation/turn counts and the cost simulator. The LiteLLM open-weight actual-cost join (`litellm_cost.apply_actual_cost`) deliberately writes cost at the TURN level only (`token_usage.total_cost_usd` = the real OpenRouter bill) plus the per-call `TurnRecord.provider_call_costs` audit record; it does NOT touch the message token buckets, so `EventCollector` stays the single writer and this invariant holds on every backend. The Python `token_usage`/`total_token_usage` aggregate is unchanged and still authoritative for budget/judges/reports. @@ -218,7 +218,7 @@ Editing `src/coder_eval/pricing.py` means editing `evalboard/lib/pricing.ts` too Recent additions, each traceable to a shipped defect: **CE037** (no unreferenced module-level private helper in `src/` — a helper whose docstring documents a bug the live code still has is worse than none), **CE038** (in an `@asynccontextmanager`, the acquire must sit INSIDE the `try` whose `finally` releases it — `asyncio.shield` protects the inner task, NOT the await, so a cancel on `__aenter__` skips the unwind while the work completes), **CE039** (a criterion checker must not return a gating `score=0.0` from an `except OSError` over a path the *task author* named — that books an eval-config error as an agent failure; raise `CheckerMisuseError` instead, and `# noqa: CE039` the cases that really are the agent's). -When fixing a bug, ask: *could a custom lint rule have prevented this?* If the root cause is a mechanically detectable pattern (e.g., "always import from `coder_eval.models`", "never call blocking IO in async"), add a rule to `tests/lint/rules/` following the CE001+ pattern and wire it up in `tests/lint/runner.py`. This turns a one-time fix into permanent enforcement. See `tests/test_custom_lint.py` for how rules are tested. (Doc-surface / whole-tree rules that reason over Markdown/YAML or the entire `src/` tree rather than one `.py` AST at a time — CE026–CE031, CE033–CE036, CE060–CE061 — are not `BaseRule`s in the runner; they are wired as dedicated `@pytest.mark.lint` test classes. CE036 enforces the `live_verdict` determinism + monotonicity contract (`criteria/base.py`) that `EarlyStopWatcher`'s latching, deferred fail-stop, and flip-attribution silently depend on: monotonicity over arbitrary Python is undecidable, so instead of a static check it REPLAYS each live criterion against every prefix of recorded trajectories (`tests/lint/live_verdict_contract.py::CASES`) — on the authored ordering AND under seeded shuffles (`permuted_violations`, which catch order-sensitive bugs the authored walk misses) — and asserts the property directly, plus registry-derived coverage — every `LiveSuccessCriterion` in the union must have cases, and every polarity its instances claim via `live_decidable_polarities()` must actually be reached by one (otherwise a single always-`undecided` fixture would "cover" a type while proving nothing). Adding a live criterion therefore means adding `ContractCase`s in the same change. CE035 resolves every `steps..outputs.` / `needs..outputs.` reference in `.github/workflows/**` to a writer that actually produces that key — GitHub expands an unwritten output to the empty string, so a typo degrades a gate silently and actionlint models `steps.*.outputs` as an open string map. CE034 scans `tasks/` and forces an armed, live-*passable* `command_executed` to set `require_success` — a crashed invocation would otherwise latch a live PASS, fire `on_pass: stop`, and let FIRED-ONLY armed gating report SUCCESS without ever consulting the unarmed criteria (negative assertions are fail-only and are exempt). CE060 scans `tasks/` and forces a dataset's `split_field` to be present on **every** row or on **none** — a *partly* labelled dataset is the one genuinely dangerous state, because `--split` keeps the matching rows and **silently drops the unlabelled ones**, so the run succeeds, the report renders, and every metric is computed over a smaller suite than the file suggests. It shares one definition of "labelled" with the runtime via `task_loader.row_split_label()` (deliberately NOT `_stratified_sample`'s convention, which folds a missing key to the `""` stratum and would turn an explicit `null` into `"None"`). CE061 scans `tasks/` and forbids a dataset row's prompt from containing, verbatim, a string one of its criteria grades it on — a prompt that supplies its own answer scores well whether or not the behaviour under test happened, and in an A/B an arm that DELETED that behaviour still passes. Location fields (`path`, `agent_file`, `command`) are exempt: naming WHERE to write removes filename nondeterminism from the measurement without revealing WHAT is graded. It catches the verbatim form only; a semantic leak (the prompt describing the graded behaviour in other words) still needs `lint-tasks` or a reviewer. CE033 keeps the plugin's bundled `reference/criteria.md` in parity with the `SuccessCriterion` union that generates it (`make plugin-reference` writes it; the rule re-renders and diffs — never hand-edit the file). CE031 guards against dead config: a behavior-driving field on `SimulationConfig`/`RunLimits`/`Dataset` that no code reads by name. CE026 keeps the GitHub Action's onboarding surfaces honest — `README.md`, `docs/CI_GATE.md`, `docs/tutorials/02-ci-pipeline.md`, and the plugin's `ci` skill, whose emitted workflow users copy into their own repos: a page's *first* Action snippet must show the agent-runtime prerequisite steps (pinned to the `action-dogfood` job that proves them in CI), a zero-install absolute next to such a snippet must name the channel it means, every `github.com/marketplace/actions/` link plus the shields badge label must match `action.yml`'s `name:`, and every `with:` key on a snippet's action step must be a real `action.yml` input (GitHub ignores unknown inputs, so a rename would silently degrade every copied workflow). Renaming an action input or changing its runtime prerequisites therefore means updating the skill too.) +When fixing a bug, ask: *could a custom lint rule have prevented this?* If the root cause is a mechanically detectable pattern (e.g., "always import from `coder_eval.models`", "never call blocking IO in async"), add a rule to `tests/lint/rules/` following the CE001+ pattern and wire it up in `tests/lint/runner.py`. This turns a one-time fix into permanent enforcement. See `tests/test_custom_lint.py` for how rules are tested. (Doc-surface / whole-tree rules that reason over Markdown/YAML or the entire `src/` tree rather than one `.py` AST at a time — CE026–CE031, CE033–CE036, CE060–CE061 — are not `BaseRule`s in the runner; they are wired as dedicated `@pytest.mark.lint` test classes. CE036 enforces the `live_verdict` determinism + monotonicity contract (`criteria/base.py`) that `EarlyStopWatcher`'s latching, deferred fail-stop, and flip-attribution silently depend on: monotonicity over arbitrary Python is undecidable, so instead of a static check it REPLAYS each live criterion against every prefix of recorded trajectories (`tests/lint/live_verdict_contract.py::CASES`) — on the authored ordering AND under seeded shuffles (`permuted_violations`, which catch order-sensitive bugs the authored walk misses) — and asserts the property directly, plus registry-derived coverage — every `LiveSuccessCriterion` in the union must have cases, and every polarity its instances claim via `live_decidable_polarities()` must actually be reached by one (otherwise a single always-`undecided` fixture would "cover" a type while proving nothing). Adding a live criterion therefore means adding `ContractCase`s in the same change. CE035 resolves every `steps..outputs.` / `needs..outputs.` reference in `.github/workflows/**` to a writer that actually produces that key — GitHub expands an unwritten output to the empty string, so a typo degrades a gate silently and actionlint models `steps.*.outputs` as an open string map. CE034 scans `tasks/` and forces an armed, live-*passable* `command_executed` to set `require_success` — a crashed invocation would otherwise latch a live PASS, fire `on_pass: stop`, and let FIRED-ONLY armed gating report SUCCESS without ever consulting the unarmed criteria (negative assertions are fail-only and are exempt). CE060 scans `tasks/` and forces a dataset's `split_field` to be present on **every** row or on **none** — a *partly* labelled dataset is the one genuinely dangerous state, because `--split` keeps the matching rows and **silently drops the unlabelled ones**, so the run succeeds, the report renders, and every metric is computed over a smaller suite than the file suggests. It shares one definition of "labelled" with the runtime via `task_loader.row_split_label()` (deliberately NOT `_stratified_sample`'s convention, which folds a missing key to the `""` stratum and would turn an explicit `null` into `"None"`). CE061 scans `tasks/` and forbids a dataset row's prompt from containing, verbatim, a string one of its criteria grades it on — a prompt that supplies its own answer scores well whether or not the behaviour under test happened, and in an A/B an arm that DELETED that behaviour still passes. Location fields (`path`, `agent_file`, `file_path`, `command`, `skill_name`) are exempt: naming WHERE to write removes filename nondeterminism from the measurement without revealing WHAT is graded — and `skill_name` is a locator for the same reason, naming WHICH skill must engage while the graded thing is the engagement EVENT, which no prompt can supply. The list lives in `CE061_LOCATOR_FIELDS` and this sentence is the derived surface (`test_ce061_exemption_list_matches_claude_md` fails if either side is edited alone). A literal `command_executed.command_pattern` echoed in a prompt is deliberately NOT exempt — a pattern asserting *what ran* is graded behaviour, and the `command` exemption covers `run_command.command`, the command the checker itself runs. It catches the verbatim form only; a semantic leak (the prompt describing the graded behaviour in other words) still needs `lint-tasks` or a reviewer. CE033 keeps the plugin's bundled `reference/criteria.md` in parity with the `SuccessCriterion` union that generates it (`make plugin-reference` writes it; the rule re-renders and diffs — never hand-edit the file). CE031 guards against dead config: a behavior-driving field on `SimulationConfig`/`RunLimits`/`Dataset` that no code reads by name. CE026 keeps the GitHub Action's onboarding surfaces honest — `README.md`, `docs/CI_GATE.md`, `docs/tutorials/02-ci-pipeline.md`, and the plugin's `ci` skill, whose emitted workflow users copy into their own repos: a page's *first* Action snippet must show the agent-runtime prerequisite steps (pinned to the `action-dogfood` job that proves them in CI), a zero-install absolute next to such a snippet must name the channel it means, every `github.com/marketplace/actions/` link plus the shields badge label must match `action.yml`'s `name:`, and every `with:` key on a snippet's action step must be a real `action.yml` input (GitHub ignores unknown inputs, so a rename would silently degrade every copied workflow). Renaming an action input or changing its runtime prerequisites therefore means updating the skill too.) Adding a user-facing field to one of the models CE030 tracks (`TaskDefinition`, `RunLimits`, `Dataset`, `SimulationConfig` — see `tests/lint/doc_schema_parity.py`) means documenting it in its guide (mention the field name as inline code) or adding an `EXEMPT` entry with a reason it is not user-authored. `make lint` fails otherwise. diff --git a/docs/DATASETS.md b/docs/DATASETS.md index 5768f8ac..943018c2 100644 --- a/docs/DATASETS.md +++ b/docs/DATASETS.md @@ -149,11 +149,16 @@ Three behaviours are worth knowing before you rely on it: safe direction (an unlabelled row never leaks into a named split), but during an incremental migration it silently *shrinks* the suite, which moves the aggregate metrics `suite_thresholds` gates on. Finish labelling before you compare two runs. -- **A labelled task with no row in the requested split is skipped, not fatal.** Expansion raises, - naming the splits that do exist, and the run records it in `run.json`'s `skipped_tasks` and carries - on with whatever else resolved. So a mistyped selector (`--split holdou`) produces a run of **zero - tasks that still exits 0** — check the skipped-task count, not just the exit code, when a split run - comes back suspiciously clean. +- **A labelled task with no row in the requested split aborts the run.** Expansion raises + `SplitSelectorError`, naming the splits that do exist, and — unlike every other dataset error — + `resolve_all_tasks` re-raises it rather than recording a `skipped_tasks` entry. So a mistyped + selector (`--split holdou`) fails loudly with a non-zero exit instead of producing a green run of + zero tasks. The distinction is deliberate: the other errors describe a malformed *file*, and one + bad task must not abort a suite; this one describes a malformed *invocation*, and the same + selector applies to every task in the run. +- **A partly labelled dataset logs a WARNING** naming how many rows were dropped, because that run + is legitimate but is measuring a smaller suite than the file suggests. `coder-eval plan --split + ` shows the same thing before you spend anything. **Stage 2 — the samplers**, over whatever survived the filter: diff --git a/docs/PLUGIN.md b/docs/PLUGIN.md index e112fbcf..b53c7acc 100644 --- a/docs/PLUGIN.md +++ b/docs/PLUGIN.md @@ -49,7 +49,7 @@ Running a suite additionally needs credentials for whichever agent the tasks use | --- | --- | | `/coder-eval:init` | Scans the repository for what is worth evaluating (Claude Code skills, an MCP server, a CLI), reports the findings, then scaffolds a task directory with one real task. | | `/coder-eval:check-skill` | Builds and runs an activation suite for one of your skills — does the agent engage it when it should, and leave it alone when it shouldn't? | -| `/coder-eval:optimize-skill` | Improves a skill along either of two tracks — the **description**, measured against an activation suite, or the **body**, measured against an outcome suite. Candidate edits become experiment variants; only what beats run-to-run noise and survives a held-out split is promoted. Walked end to end in [tutorial 08](tutorials/08-optimizing-a-skill.md) (description) and [tutorial 09](tutorials/09-optimizing-a-skill-body.md) (body). | +| `/coder-eval:optimize-skill` | Improves a skill along either of two tracks — the **description**, measured against an activation suite, or the **body**, measured against an outcome suite. Candidate edits become experiment variants; only what beats run-to-run noise and survives a held-out split is promoted. Walked end to end in [tutorial 08](tutorials/08-optimizing-a-skill-description.md) (description) and [tutorial 09](tutorials/09-optimizing-a-skill-body.md) (body). | | `/coder-eval:task` | Turns a natural-language description into task YAML with criteria that check output *content*, validated through `coder-eval plan`. | | `/coder-eval:lint-tasks` | Reviews task YAML that already exists and reports, per task, criteria that cannot fail, prompts that leak the answer, fixtures with no cleanup and near-duplicates — each with a severity and a fix. Read-only. | | `/coder-eval:analyze` | Reads a finished run directory and writes `analysis.md`: systemic failure patterns, per-task findings, and concrete fixes. | @@ -190,6 +190,11 @@ directories, so every file a skill reads travels with it under `reference/`: writes and `lint-tasks` applies to task files already on disk: could this task pass for the wrong reason, does it grade behavior or a self-report, do its fixtures reset and clean up. +- `optimize-method.md` — the track-invariant method behind `optimize-skill`'s three stages: + the cost table, what each stage does and does not bound, why the activation and execution + gates use different machinery, and the paired-diff sign rule. Split out of `SKILL.md` for + the same reason as `task-rubric.md` — the method is identical on both tracks, so it lives + once and the skill keeps the procedure. - `cli-setup.md` — how the CLI-driving skills handle a missing `coder-eval` binary: offer the install, ask first, verify it worked. - `run-layout.md` — the on-disk run-directory contract `analyze` reads: what is *inside* diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index 4b5e883d..2577604a 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -94,7 +94,7 @@ dataset: | `sample_per_stratum` | `null` | Stratified random sample: keep up to N rows per stratum. Overridden by CLI `--sample`. | | `stratify_field` | `"expected_skill"` | Row field whose value defines the stratum for `sample_per_stratum`. | | `sample_seed` | `null` | Seed for the stratified draw. Unset means the sample is **re-drawn every run**; set an integer to pin it. CLI `--sample` is separately fixed-seed and always reproducible. | -| `split_field` | `"split"` | Row field naming the row's split (e.g. `train` / `test`). CLI `--split ` keeps only rows whose value here matches, **before** any sampling. A task whose rows never set this field is unaffected by `--split`. Splits are open strings. | +| `split_field` | `"split"` | Row field naming the row's split (e.g. `train` / `test`). CLI `--split ` keeps only rows whose value here matches, **before** any sampling. A task whose rows never set this field is unaffected by `--split`; a task that sets it on *some* rows keeps the matching ones, drops the rest, and logs a WARNING naming the drop count; a task that sets it on every row but has none in the requested split aborts the run. Splits are open strings. | Full guide — row sources, substitution rules, sampling precedence, suite-level scoring, and worked examples: **[Bring Your Own Dataset](DATASETS.md)**. @@ -663,7 +663,7 @@ This pattern is especially useful for A/B testing whether additional context imp ## Success Criteria -Every task needs at least one success criterion. The framework supports 14 criterion types. +Every task needs at least one success criterion. The framework supports 15 criterion types. ### Continuous Scoring @@ -1272,7 +1272,11 @@ Like `skill_triggered`, this criterion emits a `ClassificationCriterionResult`, ### `skill_triggered` -Binary classifier: **did the agent engage the target skill during the run?** Agent-agnostic — scans the run's `turn_records` for either signal: Claude's explicit `Skill` tool call whose `skill` parameter matches `skill_name` (namespace prefixes like `plugin:skill` are stripped, so `skill_name: uipath-agents` matches `Skill(skill="uipath-coded-agents:uipath-agents")`), or — for an agent with no `Skill` tool, e.g. Codex — a command that reads the skill's files off disk (a parameter contains `skills//`, matching both the repo path and the `.agents/skills/` symlink). +Binary classifier: **did the agent engage the target skill during the run?** Agent-agnostic — scans the run's `turn_records` for either signal: Claude's explicit `Skill` tool call whose `skill` parameter matches `skill_name` **and which succeeded** (namespace prefixes like `plugin:skill` are stripped, so `skill_name: uipath-agents` matches `Skill(skill="uipath-coded-agents:uipath-agents")`), or — for an agent with no `Skill` tool, e.g. Codex — a command that **successfully** reads the skill's files off disk (a parameter contains `skills//`, matching both the repo path and the `.agents/skills/` symlink). + +> **Only a successful `Skill` call is engagement**, and the distinction matters more than it sounds. The common cause of a failed one is a skill carrying `disable-model-invocation: true`: the tool refuses the call outright (`cannot be used with Skill tool due to disable-model-invocation`), so the skill's body never loads and the agent continues on its own background knowledge — producing output plausible enough that nothing downstream looks wrong. Counting the attempt would report `yes` for a run the skill took no part in. The same reasoning excludes a call that is still in flight or was force-closed by a turn crash: for the `Skill` tool the body IS the tool result, so anything short of a delivered result loaded nothing. +> +> The file-read signal is gated too, but only where a failure proves nothing was read: a `Read`, `Glob` or `Grep` that errored (or has not resolved yet) names the path in its parameters while loading nothing, so it does not count. `Bash` is deliberately left ungated — `cat skills/x/SKILL.md | grep foo` exits non-zero *after* genuinely reading the file, which is exactly how an agent with no `Skill` tool engages a skill. Observed label is `"yes"` when either signal is found, else `"no"`. Expected label is `"yes"` iff `expected_skill == skill_name`. **Binary scoring:** `1.0` when observed matches expected, else `0.0`. @@ -1288,7 +1292,7 @@ Observed label is `"yes"` when either signal is found, else `"no"`. Expected lab | Field | Default | Description | |-------|---------|-------------| -| `skill_name` | *required* | The skill to detect — a `Skill` call whose `skill` parameter matches, or a file read under `skills//` | +| `skill_name` | *required* | The skill to detect — a **successful** `Skill` call whose `skill` parameter matches, or a **successful** file read under `skills//` | | `expected_skill` | *required* | The row's expected skill (after `${row.*}` substitution); empty string `""` for negative rows where the skill should **not** fire | **Requires agent telemetry.** This criterion reads `turn_records`, so it only works against a real agent run (not a static check). With no turn records it reports `score=0.0` and an `error`. diff --git a/docs/llms.txt b/docs/llms.txt index 24e123c1..d81edb06 100644 --- a/docs/llms.txt +++ b/docs/llms.txt @@ -50,7 +50,7 @@ and A/B plumbing. - [05 · Comparing two models](https://coder-eval.com/docs/tutorials/05-comparing-models) - [06 · Docker isolation](https://coder-eval.com/docs/tutorials/06-use-docker-isolation) - [07 · Driving Coder Eval from Claude Code](https://coder-eval.com/docs/tutorials/07-plugin-in-claude-code) -- [08 · Optimizing a Skill Description](https://coder-eval.com/docs/tutorials/08-optimizing-a-skill) +- [08 · Optimizing a Skill Description](https://coder-eval.com/docs/tutorials/08-optimizing-a-skill-description) - [09 · Optimizing a Skill Body](https://coder-eval.com/docs/tutorials/09-optimizing-a-skill-body) diff --git a/docs/tutorials/04-writing-a-task.md b/docs/tutorials/04-writing-a-task.md index 83d15c4e..9f46d3c8 100644 --- a/docs/tutorials/04-writing-a-task.md +++ b/docs/tutorials/04-writing-a-task.md @@ -69,7 +69,7 @@ What each block does: (default 1.0) and `pass_threshold` (default 0.9). `run_command` here only checks the exit code; it can also match stdout (`expected_stdout`) or read a continuous score from stdout — see the - [Task Definition Guide](../TASK_DEFINITION_GUIDE.md) for all 14 criterion + [Task Definition Guide](../TASK_DEFINITION_GUIDE.md) for all 15 criterion types and every field. ## 2. Validate without spending tokens @@ -101,7 +101,7 @@ created are preserved under `runs/latest////` (`task.json`, ## Where to go deeper -- **All 14 criterion types, weights, thresholds** → [Task Definition Guide](../TASK_DEFINITION_GUIDE.md) +- **All 15 criterion types, weights, thresholds** → [Task Definition Guide](../TASK_DEFINITION_GUIDE.md) - **Stop a run early once the key criteria are decided** (an opt-in `stop_early:` block on a criterion; `run_limits.stop_early: false` is the run-level kill switch) → [Task Definition Guide → `stop_early`](../TASK_DEFINITION_GUIDE.md#stop_early-opt-in-early-stop) - **Fan one task out over a dataset of rows** → [Bring Your Own Dataset](../DATASETS.md) - **Full CLI & config reference** → [User Guide](../USER_GUIDE.md) diff --git a/docs/tutorials/07-plugin-in-claude-code.md b/docs/tutorials/07-plugin-in-claude-code.md index 532304bd..857802fd 100644 --- a/docs/tutorials/07-plugin-in-claude-code.md +++ b/docs/tutorials/07-plugin-in-claude-code.md @@ -41,12 +41,18 @@ Code prompt, not in your shell: /plugin install coder-eval@coder-eval ``` -Verify by typing `/coder-eval:`. You should see six commands: `init`, -`check-skill`, `task`, `lint-tasks`, `analyze`, `ci`. Three of them — `init`, -`check-skill` and `task` — drive the same `coder-eval` CLI you would type by hand, -so what they write is a normal file you can commit, diff and run in CI. The other -three never invoke it: `analyze` reads a finished run directory, `ci` writes a -workflow, and `lint-tasks` only reads task files and reports. +Verify by typing `/coder-eval:`. You should see seven commands: `init`, +`check-skill`, `task`, `optimize-skill`, `lint-tasks`, `analyze`, `ci`. Four of them +— `init`, `check-skill`, `task` and `optimize-skill` — drive the same `coder-eval` +CLI you would type by hand, so what they write is a normal file you can commit, diff +and run in CI. The remaining three never invoke it: `analyze` reads a finished run +directory, `ci` writes a workflow, and `lint-tasks` only reads task files and reports. + +Three of the seven are deliberately **not** model-invokable — `init`, `ci` and +`optimize-skill` carry `disable-model-invocation: true`, so you reach them by typing the +command rather than by describing the job. That matters most for `optimize-skill`: it +spends real money across a baseline and three A/B stages, and is never something to start +because a message happened to mention a skill's wording. ## 2. Scaffold a suite @@ -170,6 +176,13 @@ remove it, `/plugin uninstall`. - `/coder-eval:ci` emits the CI workflow from [Tutorial 02](02-ci-pipeline.md) for you — least-privilege by default, and it provides the agent runtime (Node plus the Claude CLI) that the Action deliberately does not install. +- `/coder-eval:optimize-skill` is the follow-on from step 5. `check-skill` tells you + *whether* a skill fires; this A/B-tests edits to its description — or to its body — as + experiment variants, and promotes one only when it beats run-to-run noise on rows it was + never fitted to. It is the expensive one: a baseline plus three stages of agent runs, so + it states the projected count and asks before each. Walked end to end in + [Tutorial 08](08-optimizing-a-skill-description.md) (description) and + [Tutorial 09](09-optimizing-a-skill-body.md) (body). - [Claude Code plugin](../PLUGIN.md) — the reference page: every skill, what ships in the plugin, and the activation-budget mechanics in full. - [Writing a task](04-writing-a-task.md) — the same authoring loop by hand, worth diff --git a/docs/tutorials/08-optimizing-a-skill.md b/docs/tutorials/08-optimizing-a-skill-description.md similarity index 72% rename from docs/tutorials/08-optimizing-a-skill.md rename to docs/tutorials/08-optimizing-a-skill-description.md index 108e70bc..d12c0557 100644 --- a/docs/tutorials/08-optimizing-a-skill.md +++ b/docs/tutorials/08-optimizing-a-skill-description.md @@ -29,8 +29,8 @@ it actually produced — including the part where the first attempt measured not test rows, baseline it, read the confusion matrix, and decide whether to spend anything. For `lint-tasks` the answer turns out to be no — about 20 agent runs settle it. The suite then points at a sibling that *does* have headroom, `analyze`, and the full three-stage A/B -runs against that: roughly 350 further runs, ending in a promotion that survives the -test. +runs against that: **407 further runs** — the sum of the per-stage counts in the headings +below, three Stage C attempts included — ending in a promotion that survives the test. All of it on Sonnet. Never Opus for a suite this size. @@ -68,7 +68,7 @@ Do not hand-author it. Run the sibling skill: /coder-eval:check-skill lint-tasks ``` -That produces a task YAML plus a JSONL row file. The suite used here has **21 rows**, +That produces a task YAML plus a JSONL row file. The committed suite has **28 rows**, counted by `expected_skill` — the field that actually decides a row's polarity: | Kind | `expected_skill` | Total | train | test | @@ -76,7 +76,18 @@ counted by `expected_skill` — the field that actually decides a row's polarity | Positive | `lint-tasks` | 8 | 5 | 3 | | Distractor | `""` | 9 | 6 | 3 | | Sibling-owned | `task` | 3 | 2 | 1 | -| Sibling-owned | `analyze` | 1 | 1 | 0 | +| Sibling-owned | `analyze` | 8 | 4 | 4 | + +> **The table describes the file as committed; Part 1's runs did not use it.** Part 1 was +> run against an earlier 14-train-row revision, and the `analyze` rows were added later, in +> Part 2, before the file was committed. Every result block on this page names the revision +> it was computed at, which is what reconciles Step 5's `analyze` recall of 0.000 (one row, +> unlucky) with Step 6's two `analyze` train rows sitting at a stable 0.500. +> +> Re-running Part 1 today gives different numbers for a second and more interesting reason: +> the description Part 2 promotes is already committed, so a re-run measures the *improved* +> skill. [What you get running Part 1 today](#what-you-get-running-part-1-today) +> has the measured comparison. **Label sibling rows by what should *fire*, not by whose territory it is.** Two rows here ask for project setup — work that belongs to `init`, which sets @@ -213,10 +224,11 @@ before a later sibling misfire is observable, and authoritative precision needs trajectory. A stock `check-skill` suite arms nothing, so on that suite this flag changes nothing. -**Check the resolved row count, not just the exit code.** A mistyped split (`--split holdou`) -is reported as a skipped task and the run still **exits 0** — a green run over zero rows. +**Check the resolved row count.** A mistyped split (`--split holdou`) now aborts the run with an +error naming the splits that exist, so it cannot pass silently — but a *partial* row loss still +can, and only the count shows it. -Result, 14 train rows: +Result (**14 train rows — the revision these runs used**; the committed file has 17): | Skill | recall.yes | precision.yes | f1.yes | | --- | --- | --- | --- | @@ -236,13 +248,15 @@ Python code"* — the two probes designed to catch it over-claiming. promotion gate — `min(candidate F1) > max(incumbent F1)` — cannot be satisfied by any candidate. Running the three A/B stages anyway, on this suite with three candidates and two survivors, would have cost `(3+1)×14 + 3×(2+1)×14 + 6×7` = **224 further agent runs** to -chase a number that is not reachable. +chase a number that is not reachable. (Arithmetic at the 14-train / 7-test revision these +runs used — every stage count on this page is computed at the revision that stage actually +ran against, which is why they do not all divide the same way.) The first finding is therefore about the change that prompted this: **the 66-character trim was safe.** That is now measured rather than assumed. The second finding is that **a ceiling result is as much a statement about the suite as about -the description.** Fourteen well-separated rows could not distinguish a good description from +the description.** Those fourteen well-separated rows could not distinguish a good description from a better one. If you need to tell those apart, the fix is more rows and harder ones — not a looser gate. @@ -279,7 +293,9 @@ ranges.** Had this been a candidate description rather than an incumbent's quirk agreeing runs would have "proved" an improvement worth shipping. Report what replicates; treat anything else as a hypothesis. -Note what the same three runs said about `analyze`: +Note what the same three runs said about `analyze` (still the 14-train-row revision — two +`analyze` rows then, four in the committed file, which is why re-running today gives a +different baseline): ``` an-1 expected=analyze ['analyze', 'analyze', 'analyze'] @@ -311,6 +327,53 @@ two splits, three sittings — it never missed a positive and never took a distr about as much as a suite this size can say, and it is enough to conclude the trim did no harm. +### What you get running Part 1 today + +Every number above was measured **before** the change Part 2 ends with. Re-running Part 1 +against the committed suite therefore does *not* reproduce them, and the reason is the +point of the whole page rather than a defect in it. + +Re-run, three times, on the committed 17-row train split — 51 agent runs, minutes on Sonnet: + +```bash +export SKILL_SOURCE_PATH="$(pwd)/plugins/coder-eval" +coder-eval run tasks/skills/lint-tasks-activation.yaml \ + --split train -D run_limits.stop_early=false +``` + +| Skill | recall.yes | precision.yes | f1.yes (3 runs) | +| --- | --- | --- | --- | +| `lint-tasks` | 1.000 | 1.000 | **1.000, 1.000, 1.000** | +| `analyze` | 1.000 | 1.000 | **1.000, 1.000, 1.000** | +| `task` | 1.000 | 0.667 / 0.667 / 1.000 | 0.800, 0.800, 1.000 | + +`completion_rate` is 1.000 on all three; no row was excluded. + +Three things to read off it: + +- **`lint-tasks` is still at ceiling.** Part 1's finding holds on a suite 3 train rows + larger than the one that produced it, which is the strongest form the claim can take here. +- **`analyze`'s headroom is gone — because Part 2 closed it.** The description this page + promotes is committed (`4c7481c`), so the skill a re-run measures is the *improved* one. + Recall on `analyze` reads 1.000 where Part 1 recorded 0.000. The suite also grew from 2 + `analyze` train rows to 4, so both the instrument and the subject changed; what is not in + doubt is the direction, and that the fix shipped. +- **`hard-3` is still unstable, at exactly the rate Step 6 measured.** The `task` sibling + takes that distractor in **2 of 3** runs — the same two-in-three the earlier sitting + found, months later and on a different revision of the suite. A single run would have + shown precision 1.000 or 0.667 and either would have looked like a fact. + +That last one is the page's own lesson arriving unprompted: the instability is a property +of the row, not of the sitting that first caught it. It is also why the suite gate reports +a failure on two of the three runs — `precision.yes` dips under its 0.7 floor — and why the +honest baseline for this suite is a range, not a number. + +The **test** split (now 11 rows, up from 7), run once, tells the same story: `lint-tasks` 1.000 / +1.000 / **1.000**, `analyze` likewise, and `task` at precision 0.500 on one misfire. So +Step 7's conclusion — that `lint-tasks` holds at ceiling on rows it was never checked +against — survives a suite half again as large, and the `task` misfire is visible on both +halves, which is what makes it a property of the skill rather than of one split. + --- ## Part 2 — A full A/B that promotes (`analyze`) @@ -322,6 +385,11 @@ hypothesis, phrased as a claim about specific rows: **the description never name regression or comparison between runs**, so requests about things getting *worse* find nothing to match. +> As in Part 1, every number below was measured **before** the change this part ends with, +> and for the same reason: the winner is committed, so it is the incumbent now. +> [What you get running Part 2 today](#what-you-get-running-part-2-today) re-runs Stage A +> against the committed suite and reports where it stops. + Three candidates, one per hypothesis, each differing from the incumbent only in `analyze`'s frontmatter `description`, each a full seven-skill snapshot: @@ -441,10 +509,11 @@ precision. One incumbent invocation dropped a row and was excluded from the gate rather than averaged in. A gate computed over a shifting denominator is not a gate. -#### Stage C, and a test split that could not answer the question +#### Stage C, and a test split that could not answer the question (54 runs) The winner went to test as a **two-variant** experiment at `--repeats 3`, which is the one -place `--repeats` is correct: +place `--repeats` is correct. This attempt ran against the **9-row** test half — before the +two fresh rows below were written — so 2 arms × 9 rows × 3 replicates = 54 runs: ```bash coder-eval run tasks/skills/lint-tasks-activation.yaml \ @@ -475,7 +544,7 @@ exercise the failure mode, at promotion time, when you know what needs confirmin Write them as requests a real user would send, and commit to whatever they say — rows authored to flatter a candidate confirm nothing. -#### And then the infrastructure lied to us +#### And then the infrastructure lied to us (66 runs) The re-run returned a result that looked publishable and was worthless: @@ -483,7 +552,10 @@ The re-run returned a result that looked publishable and was worthless: **Paired mean diff (incumbent - a-regression)**: +0.162 [95% CI +0.011, +0.312], d = 0.72, p = 0.038 ``` -Read the sign: that says the **incumbent** was better, significantly. It is an artifact. +Read the sign: that says the **incumbent** was better, significantly. (The header subtracts +in variant declaration order, not better-minus-worse — with `incumbent` declared first, a +candidate win reads negative. `/coder-eval:optimize-skill`'s Stage B and Stage C state the +full rule; never resolve a direction from memory.) It is an artifact. `completion_rate` gives it away — `a-regression` lost 11 of 33 rows, the incumbent 6: ``` @@ -504,7 +576,7 @@ computed over an eroded, asymmetric sample is not evidence of anything.** This i `completion_rate` is a gateable metric, and why the first thing to read in a rollup is the denominator, not the effect. The run was discarded, not interpreted. -#### Stage C, run properly +#### Stage C, run properly (66 runs) With budget restored, the same experiment ran again over the 11-row test. Erosion this time was one row against `a-regression` and none against the incumbent — near-symmetric, and @@ -551,6 +623,74 @@ never measuring. confirmed in direction on the test split (1.000 vs 0.909), no sibling regression anywhere, and precision never off 1.000 in any run of either stage. +### What you get running Part 2 today + +Re-running Stage A against the committed suite measures a **different incumbent** — the one +this page promotes — and the answer it returns is Part 1's stop condition, arriving on the +skill that used to be the counter-example to it. + +The re-run is the same shape as the original Stage A: four arms over the same 17-row train +split, **68 agent runs**, $3.84 and 12 minutes on Sonnet. Only the arms' contents changed. +`a-regression` won round 1 and is committed, so it *is* the incumbent; the candidate set is +therefore fresh — two of round 1's hypotheses re-expressed against the new incumbent, plus +one the first round had no reason to ask: + +| Arm | Hypothesis | chars | +| --- | --- | --- | +| `incumbent` | round 1's promoted `a-regression`, as committed at `4c7481c` | 269 | +| `a-results` | users say "results" and "scores"; the description says "run" (round 1's `b-results`) | 281 | +| `b-symptom` | lead the trigger clause with symptoms, not the operation (round 1's `c-symptom`) | 242 | +| `c-compact` | the length is not load-bearing — same trigger tokens, 53 fewer characters | 216 | + +```bash +export SKILL_SOURCE_PATH="$(pwd)/plugins/coder-eval" +coder-eval run tasks/skills/lint-tasks-activation.yaml \ + -e .optimize-skill/analyze/round2-triage.yaml \ + --split train -D run_limits.stop_early=false +``` + +| Arm | analyze recall | precision | F1 | completion | `task` precision | +| --- | --- | --- | --- | --- | --- | +| **incumbent** | 1.000 | 1.000 | **1.000** | 1.000 | 0.667 | +| `c-compact` | 1.000 | 1.000 | **1.000** | 1.000 | 1.000 | +| `a-results` | 0.750 | 1.000 | 0.857 | 1.000 | 1.000 | +| `b-symptom` | 0.750 | 1.000 | 0.857 | 1.000 | 0.667 | + +`lint-tasks` held recall and precision at 1.000 in every arm, and every arm scored all 17 +rows — so unlike the original Stage A, nothing here was ranked against a shifting +denominator. + +**The round stops at Stage A, and that is the result.** With the incumbent at F1 1.000, +`min(candidate F1) > max(incumbent F1)` cannot be satisfied by anything: the best a candidate +can do is tie. Stage B and Stage C were not run. Establishing that cost 68 runs against the +`(3+1)×17 + 3×(2+1)×17 + 2×11×3` = **287** the full three stages would have — which is what +Stage A is for. + +Three things to read off it: + +- **The headroom Part 2 found is closed, measured by the instrument that found it.** Part 1 + caught `analyze` missing "what regressed" in every run; the promoted description takes all + four `analyze` train rows here, in the same suite, at precision 1.000. Part 1's re-run says + this from the other side; this says it against three live challengers. +- **`c-compact` ties at 1.000 with 53 fewer characters, and a tie does not promote.** On F1 + the gate is indifferent to it. It is still a real finding about a *different* budget: these + seven descriptions total 1,577 of the 1,600-character listing cap, and `c-compact` hands 53 + back. Acting on that means gating on length with F1 held constant — a different comparison + with its own replicates, not this one's leftovers. As it stands it is a single run, which + is exactly the evidence Step 6 says not to ship on. +- **One row separates every arm that differs, and it is `an-4`** — *"Compare last week's eval + results with this week's and tell me what got worse."* Both losing candidates drop it and + nothing else; all sixteen other rows are identical across all four arms. The suite's entire + discriminating power sits in one prompt, which is the small-suite cost Step 1 warned about + and the reason a 0.857 here should not be read as a candidate being meaningfully worse. + +And the page's own instability turned up again, in a form that settles it further. `hard-3` +engaged `task` in the `incumbent` and `b-symptom` arms and not in `a-results` or `c-compact` +— arms whose `task` description is **byte-identical**, since only `analyze`'s description +varies. A difference across arms that cannot differ in the relevant text is not an effect of +the arm. Two in four, where Part 1 measured two in three, and this time with the confound +ruled out by construction rather than by repetition. + --- ## Reference @@ -562,7 +702,7 @@ for `analyze`, above. In summary: | Stage | What it does | Replicates | | --- | --- | --- | -| **A — triage** | All candidates + incumbent, one invocation, `--split train`. Rank by F1, discard anything at or below the incumbent. Decides nothing. | one run | +| **A — triage** | All candidates + incumbent, one invocation, `--split train`. Rank by F1, discard anything at or below the incumbent. Promotes nothing on its own — but when every candidate lands at or below, it ends the round there, which against a ceiling incumbent it always will. | one run | | **B — gate** | Survivors + incumbent, `--split train`, **three separate `coder-eval run` invocations**. Promote only on `min(candidate) > max(incumbent)`. | three runs | | **C — confirm** | Best survivor vs. incumbent only, `--split test --repeats 3`. Read the rendered `## Paired Comparison` block. | `--repeats 3` | @@ -616,8 +756,22 @@ options are to rename the skill, or to measure it somewhere the collision is abs - **A ceiling baseline means stop.** `lint-tasks` was perfect, so the loop declined to spend 224 runs chasing a number the gate makes unreachable. Then it found the real headroom next door. +- **A tie is not a promotion — and a promoted change puts its own skill at that ceiling.** + Re-running Part 2's Stage A today measures the description this page shipped: F1 1.000, so + the best of three fresh candidates could only match it and the round ended at Stage A for + 68 runs of the 287 all three stages would have cost. The gate is `>`, not `≥`, exactly so + that a coin-flip tie cannot ship a change — including one that is genuinely better on some + other axis, as `c-compact` is on length. - **Two agreeing runs are not evidence.** The `task` misfire reproduced on both splits and - still turned out to be 2-in-3 variance. Only replicates told the difference. + still turned out to be 2-in-3 variance. Only replicates told the difference — and it is + still 2-in-3 today, on a later revision of the suite, which is about as clean a + demonstration as this page could ask for that the instability belongs to the row rather + than to the sitting that caught it. +- **A promoted change makes its own baseline unreproducible, and that is success.** Re-run + Part 1 against this repository now and `analyze` reads 1.000 where it read 0.000, because + the description this page promotes is committed. Numbers in a walkthrough date the moment + the walkthrough works; say which revision each was measured at rather than quietly + refreshing them. - **A test only confirms failure modes it contains.** The first one here was a flat tie because every regression-phrased row sat in the train half. Fresh rows, authored to test the hypothesis rather than to flatter the candidate, are the fix. diff --git a/docs/tutorials/09-optimizing-a-skill-body.md b/docs/tutorials/09-optimizing-a-skill-body.md index fa23286d..14101b41 100644 --- a/docs/tutorials/09-optimizing-a-skill-body.md +++ b/docs/tutorials/09-optimizing-a-skill-body.md @@ -53,7 +53,7 @@ dataset: ``` ```json -{"id": "pr-gate", "split": "train", "expected_skill": "ci", "scenario": "...", "expected_path": ".github/workflows/evals.yml", "expected_snippet": "minimum-task-score"} +{"id": "pr-gate", "split": "train", "expected_skill": "ci", "scenario": "...", "expected_path": ".github/workflows/evals.yml", "expected_snippet": "minimum-task-score", "expected_snippet_2": "minimum-task-score"} ``` Label every row or none. A *partly* labelled dataset is the one genuinely bad state: `--split` @@ -110,11 +110,22 @@ success_criteria: path: "${row.expected_path}" includes: - "${row.expected_snippet}" + - "${row.expected_snippet_2}" suite_thresholds: mean: 0.7 completion_rate: 1.0 ``` +**Why two snippet slots when most rows need only one.** A scenario may ask for two things, +and grading only the first lets a half-answer score full marks: `both-triggers` asks for a +pull_request trigger *and* a weekly cron, and graded on `cron:` alone a schedule-only +workflow scored 1.000 — while `cron:` is also what its sibling `schedule-weekly` grades, so +the row discriminated nothing its sibling did not. Rows with a single requirement set +`expected_snippet_2` to the **same value** as `expected_snippet`: two identical entries +score `0/2` or `2/2`, numerically identical to a single include, so the uniform shape costs +nothing. Every row must carry the field regardless — criteria are copied to every row, and a +`${row.*}` naming a field some row lacks raises at expansion. + Keep anything **constant** out of the gated criterion. `file_check` scores `found / total` over its `includes`, so folding a universal check into it puts a fixed contribution in every row of every arm, compressing the variance the gate reads. This suite asserts the action @@ -231,16 +242,20 @@ coder-eval run tasks/skills/ci-outcome.yaml --split train -D run_limits.stop_ear In order, because each one makes every number below it meaningless if it fails: -1. **The resolved row count is what you expect.** A mistyped split is reported as a skipped - task and the run still exits 0 — a green run of zero rows. +1. **The resolved row count is what you expect.** A mistyped split now aborts the run with an + error naming the splits that exist, so it cannot slip through — but a *partial* row loss + (a half-labelled dataset) still can, and only the count shows it. 2. **Engagement passes on every row.** Not most rows. A row where the skill never ran measures nothing, and Stage B's own promotion rule requires the skill to have engaged on every scored row. 3. **The Skill calls actually succeeded.** This is the one this round learned, and it is not the same as check 2. Grep a `task.json` for the tool's `result_status`; an errored call - means the body never loaded, whatever the criterion says. On an older criterion that - distinction was invisible, so it is worth confirming directly the first time you run a - suite: + means the body never loaded, whatever the criterion says. The criterion now requires + `success` for a `Skill` call to count as engagement — so on a current coder-eval this grep + *confirms* what check 2 already enforces rather than working around it, and it is what + tells you **why** a row reports `no` (refused, still in flight, or crash-force-closed to + `unknown`). On an older criterion the distinction was invisible, which is what hid the + whole failure here: ```bash jq -r '.iterations[].commands[] | select(.tool_name=="Skill") @@ -384,6 +399,6 @@ whole method exists to prevent. ## Next -- [Tutorial 08 — Optimizing a Skill Description](08-optimizing-a-skill.md) — the activation +- [Tutorial 08 — Optimizing a Skill Description](08-optimizing-a-skill-description.md) — the activation track, and another honest stop. - [Plugin guide](../PLUGIN.md) — every skill the plugin ships. diff --git a/docs/tutorials/README.md b/docs/tutorials/README.md index 1805abc8..f758fc69 100644 --- a/docs/tutorials/README.md +++ b/docs/tutorials/README.md @@ -1,8 +1,9 @@ --- description: >- Step-by-step Coder Eval tutorials — run your first AI coding-agent evaluation, - wire it into CI, browse results, write tasks, compare models, and isolate runs - in Docker. + wire it into CI, browse results, write tasks, compare models, isolate runs in + Docker, drive the whole loop from Claude Code, and A/B-test a skill's + description or body. --- # Tutorials @@ -20,7 +21,7 @@ the task-file schema see the [Task Definition Guide](../TASK_DEFINITION_GUIDE.md | 05 | [Comparing two models](05-comparing-models.md) | Use the experiment layer to A/B two configurations | | 06 | [Running tasks in Docker isolation](06-use-docker-isolation.md) | Run each task in a fresh container; add task-specific dependencies | | 07 | [Driving Coder Eval from Claude Code](07-plugin-in-claude-code.md) | Install the plugin and drive the whole loop — scaffold, author, review, run, analyze — from slash commands | -| 08 | [Optimizing a Skill Description](08-optimizing-a-skill.md) | Measure whether a skill's description can be improved — train/test splits, the reachability check that decides everything, and when to stop | +| 08 | [Optimizing a Skill Description](08-optimizing-a-skill-description.md) | Measure whether a skill's description can be improved — train/test splits, the reachability check that decides everything, and when to stop | | 09 | [Optimizing a Skill Body](09-optimizing-a-skill-body.md) | Measure whether a skill's body can be improved — outcome suites, one fixture per suite, and checking the instrument works before spending | > Contributions welcome — add a numbered `NN-title.md` file and link it in the diff --git a/mkdocs.yml b/mkdocs.yml index 5d9df14b..a2fd24a1 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -105,7 +105,7 @@ nav: - 05 · Comparing two models: tutorials/05-comparing-models.md - 06 · Docker isolation: tutorials/06-use-docker-isolation.md - 07 · Driving Coder Eval from Claude Code: tutorials/07-plugin-in-claude-code.md - - 08 · Optimizing a Skill Description: tutorials/08-optimizing-a-skill.md + - 08 · Optimizing a Skill Description: tutorials/08-optimizing-a-skill-description.md - 09 · Optimizing a Skill Body: tutorials/09-optimizing-a-skill-body.md - Guides: - User Guide: USER_GUIDE.md diff --git a/plugins/coder-eval/README.md b/plugins/coder-eval/README.md index e72dc7a8..aa60f7b1 100644 --- a/plugins/coder-eval/README.md +++ b/plugins/coder-eval/README.md @@ -58,6 +58,10 @@ real money across several rounds of runs. `check-skill`, `task`, `lint-tasks`, a `SuccessCriterion` model union (regenerated by `make plugin-reference`; do not hand-edit). - `task-rubric.md` — the adversarial task-quality checklist ("could this pass for the wrong reason?", fixture lifecycle, scope match) that `task` and `lint-tasks` both apply. +- `optimize-method.md` — the track-invariant half of `optimize-skill`: the cost table, what + each of the three stages does and does not bound, why the two tracks' gates use different + machinery, and how to read the paired-diff sign. Its one reader is `optimize-skill`, which + keeps the procedure and points here for the method. - `cli-setup.md` — the missing-CLI policy the CLI-driving skills follow: offer, ask, verify. - `run-layout.md` — the on-disk run-directory contract that `analyze` reads: what is diff --git a/plugins/coder-eval/reference/criteria.md b/plugins/coder-eval/reference/criteria.md index ba5441d4..5905fef2 100644 --- a/plugins/coder-eval/reference/criteria.md +++ b/plugins/coder-eval/reference/criteria.md @@ -257,7 +257,7 @@ Binary classifier: did the agent engage the target skill during the run? | Field | What it is | | --- | --- | | `expected_skill` | The row's expected skill (after substitution); empty string '' for negatives. | -| `skill_name` | Only count Skill invocations whose 'skill' parameter matches this name. | +| `skill_name` | Only count SUCCESSFUL Skill invocations whose 'skill' parameter matches this name (or a successful file read under skills//). | ### `uipath_eval` diff --git a/plugins/coder-eval/reference/optimize-method.md b/plugins/coder-eval/reference/optimize-method.md new file mode 100644 index 00000000..5227ded5 --- /dev/null +++ b/plugins/coder-eval/reference/optimize-method.md @@ -0,0 +1,205 @@ +# The optimize-skill method + +The track-invariant half of `/coder-eval:optimize-skill`: what the three stages cost, what +each one does and does not bound, why the two gates use different machinery, and how to read +the paired-diff sign. The skill itself carries the *procedure* — which suite, which files, +which commands, in what order. This carries the *method*, because it is the same method on +both tracks and it is what has to be right for a verdict to mean anything. + +Read it before Stage A, and again before stating any verdict. + +## Three stages, and the gate + +State the projected run count before each stage and ask. With N candidates, S survivors, +`M_train` train rows and `M_test` test rows: + +| Spend | Runs | +| --- | --- | +| Step 6 baseline | `M_train` | +| Stage A — triage | `(N+1) × M_train` | +| Stage B — gate, activation track | `3 × (S+1) × M_train` | +| Stage B — gate, execution track | `6 × M_train` per candidate gated | +| Stage C — confirm | `6 × M_test` | + +The two Stage B rows are the two gates below, priced. Activation runs `S+1` arms through +**three separate invocations**, so the survivor count multiplies. Execution runs exactly +**two** arms — incumbent plus one candidate — at `--repeats 3`, which is `2 × 3 = 6` runs per +train row *per candidate you choose to gate*; gating three candidates one after another is +three times that, not one pass with `S = 3`. Reading the activation formula on the execution +track overstates a single gate and understates a sequence of them. + +**The baseline is a real line item, and it is not redundant with Stage A's incumbent arm.** +It looks like the same measurement on the same rows, and that is precisely its value: the +baseline runs through the *task's own* skill source, while Stage A's incumbent runs through a +*snapshot you built*. Agreement between them is the wiring check — it is what proves the +snapshot machinery did not change what is being measured. Quote both numbers when you +compare them. + +**Every stage runs the suite through the experiment.** The suite is the positional +argument; the experiment carrying the arms is passed with `-e`. Passing the experiment file +positionally instead would treat it as a task, which resolves to a skipped task and a green +run of zero rows. + +### Stage A — triage (cheap) + +All candidates plus the incumbent — that is `round-triage.yaml` — in **one** invocation, +`--split train`: + +```bash +coder-eval run -e --split train +``` + +Rank each variant from its `suite.json`, then discard anything at or below the incumbent: + +- **Activation track** — the target label's F1, `metrics["f1.yes"]`. +- **Execution track** — `average_weighted_score`, or `pass_rate` when the criteria are all + binary. Rank on the suite-level number, then look at which *criteria* moved: a candidate + that gains on one criterion while losing another is not ahead, it has traded. + +Check `completion_rate` on every arm before ranking anything. An arm that lost rows computed +its score over a different denominator and is not comparable — that is a re-run, not a +ranking. + +It is **not** a top-level `suite.json` key: it sits in each entry of `criterion_aggregates`, +alongside that entry's `rows_total` and `rows_excluded`. A suite stacking several criteria +therefore has one per criterion. Read the one belonging to the criterion you are gating on, +and if two disagree, say so rather than picking — divergent denominators across criteria on +the same rows means something dropped mid-run. + +This decides nothing — it only narrows. Replicate pooling is irrelevant here because +nothing is being gated on. + +### Stage B — the gate (replicates) + +**Activation track.** Incumbent plus survivors, `--split train`, **invoked three separate +times** — three `coder-eval run` commands, **not** `--repeats 3`: + +```bash +coder-eval run -e --split train --run-dir /round-gate-1 +coder-eval run -e --split train --run-dir /round-gate-2 +coder-eval run -e --split train --run-dir /round-gate-3 +``` + +**This is the instruction most likely to be "simplified" into a bug.** Suite rollups are +keyed on `(variant, suite)` only, so `--repeats 3` pools all three replicates into a single +`suite.json` with one confusion matrix — the per-replicate F1 this gate reads would not +exist in that file. Three invocations produce three run directories and three `suite.json` +files. The cost is identical. + +Read `metrics["f1.yes"]` per invocation per arm. **Never recompute F1 from the confusion +counts** — the criterion layer owns that arithmetic including its division-by-zero +convention, and a re-derivation will disagree with the gate the run already applied. + +Promote only when all of these hold: + +- **`min(candidate F1) > max(incumbent F1)`** across the three invocations — the ranges do + not overlap. A difference smaller than the run-to-run spread cannot clear it. +- **F1 improves, not just recall.** Widening recall while shedding precision is not an + improvement, it is a different trade. +- **No sibling regression.** For every other `skill_triggered` criterion in the suite, its + **`recall.yes`** must not drop. A candidate that wins by annexing a sibling's requests has + moved the failure, not fixed it. + + Read `recall.yes`, not `precision.yes`, and the reason is worth keeping: annexation makes + the sibling's criterion `expected=yes, observed=no` on that row — a false negative. That + lowers recall. Precision is `tp/(tp+fp)`, and annexation lowers `tp` while leaving `fp` + untouched, so on a suite where the sibling never misfires precision reads 1.0 however many + requests are stolen — right up until the last one, where `tp` and `fp` are both 0 and the + div-by-zero convention drops it to 0.0. Either way it is blind to the thing you are + watching for: gating on precision here would be gating on a constant that finally moves + only when the sibling has already lost everything. +- **Print per-invocation F1 and confusion counts for every arm.** The verdict is never + reported without the numbers behind it. + +Why replicates rather than a fixed threshold: each arm's spread measures the noise floor +for *this* suite at *this* size. A hardcoded "≥ 0.05 F1" would be far too lax on a six-row +suite and needlessly strict on a forty-row one. **Do not introduce one.** + +**Be precise about what this bounds, because it is easy to claim more.** The replicate +spread measures agent stochasticity over a *fixed* set of rows. It does not measure +row-sampling variance, and it does not correct for the fact that the survivors were already +chosen on these same train rows in Stage A — so with S survivors each tested independently, +some separation by luck is expected. Stage B bounds run noise. **The test is what bounds +the fit**, and it is why Stage C is not optional. Report the gate as "separated beyond +run-to-run noise on the train rows", never as "proven better". + +**Execution track — gate pairwise, with `--repeats 3`, and let the reporter do the +statistics.** Take the single best Stage A survivor against the incumbent as a **two-variant** +experiment: + +```bash +coder-eval run -e --split train --repeats 3 +``` + +This is a deliberate departure from the activation gate above, for one reason: the +activation gate compares **F1**, which the pooled `suite.json` cannot report per replicate — +hence three invocations. The execution gate compares per-row **`weighted_score`**, which is +exactly what `paired_comparison` already computes, correctly, over replicates it averages +per row before pairing. So the same `## Paired Comparison` block that is only *corroboration* +on the activation track is the **primary instrument** here, with a mean difference, +a 95% confidence interval, Cohen's *d* and a paired *t*-test — tested code instead of +arithmetic done by hand. + +That constrains the shape: it fires only for exactly two variants, so gate one candidate at +a time, in `round-gate.yaml`. With expensive rows that is the right trade anyway — Stage A +already ranked them. + +**Read the sign off the header, and never state a direction without it.** The block renders +as `**Paired mean diff ( - )**`, subtracting +in **variant declaration order** — not incumbent-minus-candidate, and not +better-minus-worse. With `incumbent` declared first, as in the example above, **a candidate +win reads negative**. Quote the header verbatim next to the number, and resolve the direction +from it every time rather than from memory; a reversed reading promotes the arm that lost, +and every subsequent number in the ledger corroborates it. + +Promote only when all of these hold: + +- **The paired mean difference favours the candidate and its 95% CI excludes zero.** +- **The PREDECLARED primary and its guardrails hold.** Before Stage B runs, name **one** + primary criterion (or the suite score) plus the specific guardrail criteria allowed to veto + a win, and record them in the ledger — before the numbers exist, which is what makes it a + predeclaration rather than a claim. Then evaluate only those. The reason to compare + per-criterion aggregates at all is unchanged: a candidate that lifts the average while + breaking a row that used to pass has traded, and on a body edit that trade is usually the + thing you least want. But scanning *every* per-criterion aggregate post hoc for a + regression is uncorrected multiple testing in the rejection direction — with enough + criteria something always looks worse, so noisy criteria veto real wins and *which* + criterion "regressed" is unstable from round to round. +- **`completion_rate` is equal across arms**, or the difference favours the incumbent. An + eroded, asymmetric sample produces confident nonsense — a *p*-value computed over rows that + vanished from one arm is not evidence. +- The skill **actually engaged** on every scored row; otherwise part of the sample measured + the absence of the thing under test. + +Print the paired block verbatim alongside the per-criterion table. A body change is a +behavioural change, and the numbers behind it are the whole argument. + +### Stage C — confirm on the test split + +Only the best candidate that already passed Stage B, as a **two-variant** experiment +(incumbent + that candidate) at `--split test --repeats 3`. That is `round-confirm.yaml`, +a third file — re-passing the triage file here is the mistake Step 9 describes, and it costs +more while producing no paired block at all. + +Here `--repeats` is correct and required. The experiment reporter renders a +`## Paired Comparison` block in `experiment.md` — mean difference, 95% confidence interval, +Cohen's *d*, and a paired-*t* p-value — but it fires **only** for exactly two variants, and +it averages replicates per row *before* pairing, which is precisely what pooling breaks in +`suite.json` and exactly what is wanted here. + +**The same sign rule applies, and it is easiest to get wrong here** — this is the number the +promotion is reported on. It is restated rather than cross-referenced because each stage is +read on its own, and a lint sensor requires it in both. The header subtracts in +**variant declaration order** +(` - `), so with `incumbent` declared first **a candidate +win reads negative**. Quote the header verbatim beside the figure and read the direction off +it, every time. + +Report that block verbatim alongside the test F1s, and state its limit honestly: it +pairs per-row `weighted_score` — row-level correctness, an accuracy-flavoured quantity — +**not** F1. F1 remains the promotion metric; the paired block corroborates the direction on +the paired rows, it does not re-test the promotion metric. + +Require the F1 direction to reproduce on the test split. Do **not** require replicate separation +there — a test split is usually smaller and the separation usually weaker — and say that, +rather than implying a stronger result than was obtained. diff --git a/plugins/coder-eval/reference/templates/activation.yaml b/plugins/coder-eval/reference/templates/activation.yaml index 15fbe90e..db0d3a0e 100644 --- a/plugins/coder-eval/reference/templates/activation.yaml +++ b/plugins/coder-eval/reference/templates/activation.yaml @@ -39,9 +39,28 @@ tags: [activation] # `skill_name` below stays the BARE name even when a plugin namespaces the skill as # `plugin:skill` — the checker strips the namespace before comparing. agent: + # The discriminator is required for `setting_sources` to be in schema — it is a + # claude-code field, not a field on the shared agent base. Running this suite with + # `--type codex` therefore fails resolution and reports the task as skipped; a Codex + # user should drop both this line and `setting_sources` below. + type: "claude-code" plugins: - type: "local" path: "$SKILL_SOURCE_PATH" + # The agent must not need the host project's CLAUDE.md or settings — this measures the + # skill listing, and injecting a 20 KB project guide into every call is both expensive + # and a confound. + setting_sources: [] + +# Activation is decided in the first assistant turn: either the skill is engaged or it is +# not. Without caps the agent spends turns exploring a sandbox that deliberately holds no +# eval files, and a row that times out is EXCLUDED from the confusion matrix rather than +# scored — shrinking the denominator the metrics are computed over (watch `completion_rate` +# in the rollup). Cap tightly: it buys signal, not just cost. +run_limits: + max_turns: 2 + turn_timeout: 120 + task_timeout: 300 dataset: paths: diff --git a/plugins/coder-eval/reference/templates/outcome.yaml b/plugins/coder-eval/reference/templates/outcome.yaml index d67f797c..f153be2f 100644 --- a/plugins/coder-eval/reference/templates/outcome.yaml +++ b/plugins/coder-eval/reference/templates/outcome.yaml @@ -12,6 +12,13 @@ # Replace every `my-skill` with the bare name of the skill under test (the directory name # of its SKILL.md), every `/my-plugin:my-skill` with its real slash command, and rewrite # outcome-rows.jsonl as real scenarios. Every REPLACE: marker is a placeholder. +# +# SIZING: the four rows in outcome-rows.jsonl are the illustrative MINIMUM, not a target. +# A split halves each side, so four rows leave a two-row test half — exactly the token test +# `/coder-eval:optimize-skill` warns against, which confirms nothing while reading as if it +# did. Write as many real scenarios as the skill has distinct jobs before running a stage. +# (The disclaimer lives here rather than beside the rows because a JSONL cannot carry a +# comment — the same reason activation.yaml carries its twin of this note.) task_id: "my-skill-outcome" description: "Given that `my-skill` fired, does the agent produce the right outcome?" tags: [outcome] @@ -134,7 +141,8 @@ dataset: # tmp/arm/skills//SKILL.md # # Do NOT instead ask the agent to find and read the SKILL.md itself. That looks like it -# should work — a file read counts as engagement and genuinely loads the body — but the +# should work — a SUCCESSFUL file read counts as engagement and genuinely loads the body — +# but the # plugin lives at a host path the sandbox cannot discover. Tested: 0 of 2 rows located it, # both scored 0.000. # @@ -163,9 +171,12 @@ success_criteria: # here, not in the criteria below. If it is not 1.0 on every row, fix the suite before # spending a stage. # - # Necessary but not sufficient: it counts READING the skill's SKILL.md as engagement, not - # only a Skill tool call, so a row can report engaged while the command it issued named a - # different skill. Check the trajectory when a number surprises you. + # Necessary but not sufficient: it counts SUCCESSFULLY READING the skill's SKILL.md as + # engagement, not only a Skill tool call, so a row can report engaged while the command it + # issued named a different skill. Check the trajectory when a number surprises you. + # + # A Skill call must have SUCCEEDED to count: a refused call (disable-model-invocation), one + # still in flight, or one force-closed by a turn crash delivered no body at all. - type: "skill_triggered" description: "my-skill engaged for row ${row.id}" skill_name: "my-skill" diff --git a/plugins/coder-eval/skills/check-skill/SKILL.md b/plugins/coder-eval/skills/check-skill/SKILL.md index b28b8ba9..8bc9a165 100644 --- a/plugins/coder-eval/skills/check-skill/SKILL.md +++ b/plugins/coder-eval/skills/check-skill/SKILL.md @@ -168,6 +168,15 @@ your `suite_thresholds` gate on are computed over fewer rows than you think. Eit every row or label none — never some. (With no labels anywhere, `--split` leaves the task untouched, which is safe.) +**Keep the template's `run_limits:` block and its `agent.setting_sources: []`.** Both are +there for the measurement, not just the bill. The caps buy *signal*: activation is decided in +the first assistant turn, and a row that runs on is a row that can time out — and a timed-out +row is excluded from the confusion matrix rather than scored, so it shrinks the denominator +instead of showing up as a bad number. The empty `setting_sources` keeps the host project's +`CLAUDE.md` out of every call, which is both a cost and a confound on a suite whose subject is +the skill listing. Do not restate the numbers here; the template owns them. (Both are +claude-code fields, so a Codex user drops them along with `agent.type`.) + **Then make the skill reachable, which is the step that decides whether the suite measures anything.** The task runs in a fresh sandbox that contains none of the user's files, so the agent is offered no skills unless the task says where they live. That is the `agent.plugins` diff --git a/plugins/coder-eval/skills/lint-tasks/SKILL.md b/plugins/coder-eval/skills/lint-tasks/SKILL.md index 93988af8..85aab1d3 100644 --- a/plugins/coder-eval/skills/lint-tasks/SKILL.md +++ b/plugins/coder-eval/skills/lint-tasks/SKILL.md @@ -65,12 +65,20 @@ a framework fixture. The rubric is the single declaration of those checks. Do not restate or count them here; read it at runtime, so a rubric that gains a section or a check reaches this skill with no edit. -Then add the **one** axis that exists only at review time, because it needs neighbours: - -- **Near-duplicate.** Name the most similar sibling and say what overlaps. Carve-out: - **scaffold reuse is not duplication.** Tasks sharing a YAML skeleton while exercising - materially distinct operations are good template reuse — that is what a template is for. - Raise this only when the *operation under test* overlaps, not when the boilerplate does. +Then add the **two** axes that exist only at review time: + +- **Near-duplicate.** Needs neighbours. Name the most similar sibling and say what overlaps. + Carve-out: **scaffold reuse is not duplication.** Tasks sharing a YAML skeleton while + exercising materially distinct operations are good template reuse — that is what a template + is for. Raise this only when the *operation under test* overlaps, not when the boilerplate + does. +- **A partly labelled split dataset.** Needs the row file, which the rubric does not read. If a + dataset's `split_field` (default `split`) is set on *some* rows and absent, `null` or `""` on + others, flag it: `--split` keeps the matching rows and **silently drops the rest**, so the run + succeeds, the report renders, and every metric is computed over a smaller suite than the file + suggests. Label every row or none — and say which, since "or none" is a legitimate answer for + a suite that is never split. Two states that look similar are *fine* and must not be flagged: + every row labelled, and no row labelled. ### Do not flag an activation suite diff --git a/plugins/coder-eval/skills/optimize-skill/SKILL.md b/plugins/coder-eval/skills/optimize-skill/SKILL.md index ff83bd04..68055ffb 100644 --- a/plugins/coder-eval/skills/optimize-skill/SKILL.md +++ b/plugins/coder-eval/skills/optimize-skill/SKILL.md @@ -42,8 +42,11 @@ also covers whether this project pins a version and what to do when the installe disagrees. **A version string is not a capability check, and this loop needs a capability.** Once you -have a suite (step 4), run `coder-eval plan ` and require it to exit 0 *before* -spending. Two binaries can report the same version and differ in whether `--split` and +have a suite (step 4), run `coder-eval plan --split ` +and require it to exit 0 *before* spending — then read the printed row count, which is the +number every cost estimate below depends on. A binary whose `plan` does not accept `--split` +is an older coder-eval, which makes this a sharper capability check than the version string. +Two binaries can report the same version and differ in whether `--split` and `dataset.split_field` exist at all — a repository whose working tree is ahead of its last release has exactly that shape, and then the pinned-version rule says "carry on" while every run fails at load. If `plan` rejects a field this skill relies on, prefer a project-local @@ -138,7 +141,9 @@ train half is all you get to develop against. Roughly double what a one-shot che want. Apply that as a number, not a feeling: compute the *train half's* count for the polarity you -are gating on. Below `check-skill`'s un-doubled minimum, **hand back rather than gate** — a +are gating on, and price it — the row count is the only input to the cost table in +`${CLAUDE_PLUGIN_ROOT}/reference/optimize-method.md`, so sizing the suite IS the budget +decision, made here rather than discovered at Stage B. Below `check-skill`'s un-doubled minimum, **hand back rather than gate** — a metric over three or four rows moves in 25–33 point jumps and cannot separate a real gain from noise. Between the un-doubled minimum and the doubled target, you may proceed, but say in the report that the suite is under-sized and that a non-result may simply be a suite too @@ -165,9 +170,9 @@ produces a green run that measures nothing: So the suite carries a `dataset:` block naming a JSONL in `paths:` plus `split_field: "split"`, and each row is one scenario. Start from -`${CLAUDE_PLUGIN_ROOT}/reference/templates/outcome.yaml`, the worked example of everything -in this section (Step 4 already points at `reference/criteria.md` for the criterion types; -this is a second reference on the same step, not a replacement). +`${CLAUDE_PLUGIN_ROOT}/reference/templates/outcome.yaml` — the worked example of everything +in this section, and a different reference from `reference/criteria.md` above: that one lists +the criterion TYPES available, this one is the suite shape to fill in. Glob the eval tree for tasks that exercise this skill's job. @@ -181,6 +186,74 @@ two that go missing first are that the rows must **carry `train`/`test` split la that each must **invoke the skill by slash command** (below). Neither is `/coder-eval:task`'s default. +**Before writing any rows, check this precondition — it decides whether the execution +track works on this skill at all.** + +### A `disable-model-invocation: true` skill cannot be reached at all — read this first + +If the skill under test sets that flag, **the execution track does not work on it +unmodified**, and the way it fails is the worst possible one. The `Skill` tool refuses the +call outright: + +``` +Skill plugin:my-skill cannot be used with Skill tool +due to disable-model-invocation +``` + +The body is never loaded. The agent, given a capable model and a plausible request, +carries on from its own background knowledge and produces confident, wrong-in-detail +output — and every criterion downstream scores that output as though the skill had +written it. A whole round measured this way tells you nothing about the body: in a real +case, four arms differing only in that body tied *exactly*, because none of them ever saw +it. + +**Fix it in the snapshot, not in the suite.** Step 8's arms are already modified copies of +the plugin, so remove the `disable-model-invocation:` line from the target skill's +frontmatter in **every** arm, incumbent included. That reproduces what a real user gets — +typing the slash command *does* inject the body — while keeping the arms identical in +everything but the text under test. Verified: with the flag removed the call succeeds and +the body loads; with it present, 24 of 24 rows failed silently. + +**Say what this costs in external validity.** Results therefore apply to the +flag-removed configuration. If you ship with the flag kept, the body only ever loads via +explicit `/name` invocation, so re-check that path before promoting. The removal is still +the right experimental design — it is the only way to hold engagement constant across +arms — but the reader of your ledger must know which configuration was measured. + +Do **not** try to route around it by telling the agent to locate and read the `SKILL.md` +itself. It is a reasonable idea — a *successful* file read counts as engagement and does +load the body — but the plugin sits at a host path the sandbox cannot discover. Tested: 0 +of 2 rows found the file, both scored zero. (And a read that fails to find it is not +engagement either, so those rows score zero rather than reporting a phantom `yes`.) + +**Then confirm engagement is genuinely 1.0 before spending.** And confirm it against a +version of `skill_triggered` that requires a **successful** call — an older one counted +the refused attempt as engagement, which is precisely what hid all of this. The current +rule is wider than "ignore errored calls": a call that is still in flight, or was +force-closed by a turn crash, delivered no body and does not count either. + +**Engagement below 1.0 on every row is the single biggest threat to an outcome round.** +Three ways it slips, all silent: + +- the model answers the command by **dispatching a sub-agent**, which reads the skill in + the child instead of emitting a `Skill` call in the parent stream; +- it ignores the command and simply **does the work itself**, emitting no `Skill` call; +- the scenario's own wording **routes it to a sibling skill** — a request phrased around + another skill's subject matter can beat the explicit command. + +So deny sub-agent delegation (`disallowed_tools: ["Agent", "Task"]` — an *allowlist* +cannot do it, because those tools stay available whatever `allowed_tools` says), name +`Skill` in `allowed_tools` so the mechanism under test is visible (the tool is available +either way, so this documents rather than fixes), phrase scenarios in the vocabulary of +*this* skill's job, and +above all **read the engagement rate before the scores**. A round whose engagement is not +1.0 on every row is measuring a mixture, and no amount of replicates fixes it. + +One subtlety when you read it: `skill_triggered` counts **successfully reading the skill's +`SKILL.md`** as engagement, not only a `Skill` call. A row can therefore report engaged while the +command it actually issued named a different skill. Treat the criterion as necessary, not +sufficient, and check the trajectory when a number surprises you. + **Two consequences of being dataset-backed, and both decide how the rows are written.** - **Criteria are copied to every row**, with `${row.}` substituted into every string @@ -244,62 +317,6 @@ Five requirements specific to this track: two: *"Use the `plugin:skill` skill to handle this request. Invoke it with the Skill tool and follow it before writing anything."* - ### A `disable-model-invocation: true` skill cannot be reached at all — read this first - - If the skill under test sets that flag, **the execution track does not work on it - unmodified**, and the way it fails is the worst possible one. The `Skill` tool refuses the - call outright: - - ``` - Skill plugin:my-skill cannot be used with Skill tool - due to disable-model-invocation - ``` - - The body is never loaded. The agent, given a capable model and a plausible request, - carries on from its own background knowledge and produces confident, wrong-in-detail - output — and every criterion downstream scores that output as though the skill had - written it. A whole round measured this way tells you nothing about the body: in a real - case, four arms differing only in that body tied *exactly*, because none of them ever saw - it. - - **Fix it in the snapshot, not in the suite.** Step 8's arms are already modified copies of - the plugin, so remove the `disable-model-invocation:` line from the target skill's - frontmatter in **every** arm, incumbent included. That reproduces what a real user gets — - typing the slash command *does* inject the body — while keeping the arms identical in - everything but the text under test. Verified: with the flag removed the call succeeds and - the body loads; with it present, 24 of 24 rows failed silently. - - Do **not** try to route around it by telling the agent to locate and read the `SKILL.md` - itself. It is a reasonable idea — a file read counts as engagement and does load the body — - but the plugin sits at a host path the sandbox cannot discover. Tested: 0 of 2 rows found - the file, both scored zero. - - **Then confirm engagement is genuinely 1.0 before spending.** And confirm it against a - version of `skill_triggered` that ignores errored calls — an older one counted the refused - attempt as engagement, which is precisely what hid all of this. - - **Engagement below 1.0 on every row is the single biggest threat to an outcome round.** - Three ways it slips, all silent: - - - the model answers the command by **dispatching a sub-agent**, which reads the skill in - the child instead of emitting a `Skill` call in the parent stream; - - it ignores the command and simply **does the work itself**, emitting no `Skill` call; - - the scenario's own wording **routes it to a sibling skill** — a request phrased around - another skill's subject matter can beat the explicit command. - - So deny sub-agent delegation (`disallowed_tools: ["Agent", "Task"]` — an *allowlist* - cannot do it, because those tools stay available whatever `allowed_tools` says), name - `Skill` in `allowed_tools` so the mechanism under test is visible (the tool is available - either way, so this documents rather than fixes), phrase scenarios in the vocabulary of - *this* skill's job, and - above all **read the engagement rate before the scores**. A round whose engagement is not - 1.0 on every row is measuring a mixture, and no amount of replicates fixes it. - - One subtlety when you read it: `skill_triggered` counts **reading the skill's `SKILL.md`** - as engagement, not only a `Skill` call. A row can therefore report engaged while the - command it actually issued named a different skill. Treat the criterion as necessary, not - sufficient, and check the trajectory when a number surprises you. - - **Assert engagement, do not assume it.** Stack a `skill_triggered` criterion naming the skill on every row. It costs nothing extra — the trajectory is already recorded — and it is what separates "the body gave bad instructions" from "the skill never ran", which score @@ -399,9 +416,9 @@ already scarce sample; it confirms nothing and reads as if it did. Keep every ro and author **fresh test rows at promotion time**, when you know which single candidate needs confirming. - Note what follows: with every row labelled `train`, a later `--split test` matches nothing, - which is reported as a skipped task and a green run of zero rows. Write the fresh rows - first, then run Stage C. + Note what follows: with every row labelled `train`, a later `--split test` matches nothing + and aborts the run with an error naming the splits that exist. That is better for this + advice, not worse — you cannot miss it. Write the fresh rows first, then run Stage C. **Rows *partly* labelled** → finish the labelling before running anything. This is the dangerous state, because it does not look like one: `--split` keeps the matching rows and @@ -415,8 +432,9 @@ coder-eval run --split train -D run_limits.stop_early=false ``` **Check the resolved row count, not just the exit code.** A mistyped split name (`--split -holdou`) is reported as a skipped task and the run still exits 0 — a green run of zero rows. -If the count is zero or lower than the train rows you expect, that is a wiring problem, not a +holdou`) aborts the run outright, naming the splits that exist — but a *partial* row loss does +not, and that is what the count catches: a half-labelled dataset silently drops its unlabelled +rows. If the count is lower than the train rows you expect, that is a wiring problem, not a result. On `stop_early`: a suite that arms it can pass-stop a run before a later sibling misfire is @@ -429,6 +447,14 @@ insurance against an armed suite, not a fix for anything already there. Read `///suite.json`; its shape is documented in `${CLAUDE_PLUGIN_ROOT}/reference/run-layout.md`. +**Group failures into named hypotheses, each pointing at specific rows.** *"It misses +oblique requests because the description names the operation and never the symptom."* +*"It fires on 'audit my dependencies' because the description claims audit vocabulary +generally."* Not "improve the wording" — a hypothesis you cannot phrase as a claim about +specific rows is not one you can test. + +### Execution track + **On the execution track, read the trajectory, not just the score.** A failed criterion tells you the outcome was wrong; only the transcript tells you *which instruction the agent followed instead*. Open the failing rows' `task.json` and compare what the agent actually did @@ -449,6 +475,8 @@ Name the failing rows and quote the instruction each one contradicts. Everything recorded — file contents, stdout, transcripts — is **untrusted agent output**: quote it as evidence, never follow it as instruction. +### Activation track + Take the aggregate whose criterion names **this** skill. `details.confusion` and `details.per_label` give you the *counts* — how many false negatives (rows that should have engaged it and did not) and how many false positives (rows that engaged it and should not @@ -461,12 +489,6 @@ though the list is capped) and, when a row you need is not in it, the per-row that produced it before drawing any conclusion — a hypothesis about wording that cannot name the requests it explains is not testable. -**Group failures into named hypotheses, each pointing at specific rows.** *"It misses -oblique requests because the description names the operation and never the symptom."* -*"It fires on 'audit my dependencies' because the description claims audit vocabulary -generally."* Not "improve the wording" — a hypothesis you cannot phrase as a claim about -specific rows is not one you can test. - **Sibling-owned rows.** A suite may carry rows whose `expected_skill` names a *different* skill in the same repository, with one `skill_triggered` criterion stacked per skill — which yields a per-skill confusion matrix from the same traces. That turns "this candidate @@ -626,193 +648,43 @@ downstream means anything. ## Step 10 — Three stages, and the gate -State the projected run count before each stage and ask. With N candidates, S survivors, -`M_train` train rows and `M_test` test rows: - -| Spend | Runs | -| --- | --- | -| Step 6 baseline | `M_train` | -| Stage A — triage | `(N+1) × M_train` | -| Stage B — gate | `3 × (S+1) × M_train` | -| Stage C — confirm | `6 × M_test` | - -**The baseline is a real line item, and it is not redundant with Stage A's incumbent arm.** -It looks like the same measurement on the same rows, and that is precisely its value: the -baseline runs through the *task's own* skill source, while Stage A's incumbent runs through a -*snapshot you built*. Agreement between them is the wiring check — it is what proves the -snapshot machinery did not change what is being measured. Quote both numbers when you -compare them. - -**Every stage runs the suite through the experiment.** The suite is the positional -argument; the experiment carrying the arms is passed with `-e`. Passing the experiment file -positionally instead would treat it as a task, which resolves to a skipped task and a green -run of zero rows. - -### Stage A — triage (cheap) - -All candidates plus the incumbent — that is `round-triage.yaml` — in **one** invocation, -`--split train`: - -```bash -coder-eval run -e --split train -``` - -Rank each variant from its `suite.json`, then discard anything at or below the incumbent: - -- **Activation track** — the target label's F1, `metrics["f1.yes"]`. -- **Execution track** — `average_weighted_score`, or `pass_rate` when the criteria are all - binary. Rank on the suite-level number, then look at which *criteria* moved: a candidate - that gains on one criterion while losing another is not ahead, it has traded. - -Check `completion_rate` on every arm before ranking anything. An arm that lost rows computed -its score over a different denominator and is not comparable — that is a re-run, not a -ranking. +**Read `${CLAUDE_PLUGIN_ROOT}/reference/optimize-method.md` before Stage A, and again before +stating any verdict.** It carries the method this step executes — the cost table, what each +stage does and does not bound, why the two tracks' gates use different machinery, the +promotion conditions, and how to read the paired-diff sign. It is track-invariant, which is +why it lives once rather than twice. -It is **not** a top-level `suite.json` key: it sits in each entry of `criterion_aggregates`, -alongside that entry's `rows_total` and `rows_excluded`. A suite stacking several criteria -therefore has one per criterion. Read the one belonging to the criterion you are gating on, -and if two disagree, say so rather than picking — divergent denominators across criteria on -the same rows means something dropped mid-run. +**State the projected run count before each stage and ask.** The arithmetic is the method +file's cost table; the experiment files are the ones Step 9 named, one per stage: -This decides nothing — it only narrows. Replicate pooling is irrelevant here because -nothing is being gated on. - -### Stage B — the gate (replicates) - -**Activation track.** Incumbent plus survivors, `--split train`, **invoked three separate -times** — three `coder-eval run` commands, **not** `--repeats 3`: - -```bash -coder-eval run -e --split train --run-dir /round-gate-1 -coder-eval run -e --split train --run-dir /round-gate-2 -coder-eval run -e --split train --run-dir /round-gate-3 -``` - -**This is the instruction most likely to be "simplified" into a bug.** Suite rollups are -keyed on `(variant, suite)` only, so `--repeats 3` pools all three replicates into a single -`suite.json` with one confusion matrix — the per-replicate F1 this gate reads would not -exist in that file. Three invocations produce three run directories and three `suite.json` -files. The cost is identical. - -Read `metrics["f1.yes"]` per invocation per arm. **Never recompute F1 from the confusion -counts** — the criterion layer owns that arithmetic including its division-by-zero -convention, and a re-derivation will disagree with the gate the run already applied. - -Promote only when all of these hold: - -- **`min(candidate F1) > max(incumbent F1)`** across the three invocations — the ranges do - not overlap. A difference smaller than the run-to-run spread cannot clear it. -- **F1 improves, not just recall.** Widening recall while shedding precision is not an - improvement, it is a different trade. -- **No sibling regression.** For every other `skill_triggered` criterion in the suite, its - **`recall.yes`** must not drop. A candidate that wins by annexing a sibling's requests has - moved the failure, not fixed it. - - Read `recall.yes`, not `precision.yes`, and the reason is worth keeping: annexation makes - the sibling's criterion `expected=yes, observed=no` on that row — a false negative. That - lowers recall. Precision is `tp/(tp+fp)`, and annexation lowers `tp` while leaving `fp` - untouched, so on a suite where the sibling never misfires precision reads 1.0 however many - requests are stolen — right up until the last one, where `tp` and `fp` are both 0 and the - div-by-zero convention drops it to 0.0. Either way it is blind to the thing you are - watching for: gating on precision here would be gating on a constant that finally moves - only when the sibling has already lost everything. -- **Print per-invocation F1 and confusion counts for every arm.** The verdict is never - reported without the numbers behind it. - -Why replicates rather than a fixed threshold: each arm's spread measures the noise floor -for *this* suite at *this* size. A hardcoded "≥ 0.05 F1" would be far too lax on a six-row -suite and needlessly strict on a forty-row one. **Do not introduce one.** - -**Be precise about what this bounds, because it is easy to claim more.** The replicate -spread measures agent stochasticity over a *fixed* set of rows. It does not measure -row-sampling variance, and it does not correct for the fact that the survivors were already -chosen on these same train rows in Stage A — so with S survivors each tested independently, -some separation by luck is expected. Stage B bounds run noise. **The test is what bounds -the fit**, and it is why Stage C is not optional. Report the gate as "separated beyond -run-to-run noise on the train rows", never as "proven better". - -**Execution track — gate pairwise, with `--repeats 3`, and let the reporter do the -statistics.** Take the single best Stage A survivor against the incumbent as a **two-variant** -experiment: +| Stage | Experiment file | How it runs | +| --- | --- | --- | +| A — triage | `round-triage.yaml` | one invocation, all candidates + incumbent, `--split train` | +| B — gate, activation track | `round-gate.yaml` | **three separate invocations**, `--split train`, distinct `--run-dir` each | +| B — gate, execution track | `round-gate.yaml` | one invocation, exactly two variants, `--split train --repeats 3` | +| C — confirm | `round-confirm.yaml` | exactly two variants, `--split test --repeats 3` | -```bash -coder-eval run -e --split train --repeats 3 -``` +**Every stage runs the suite through the experiment.** The suite is the positional argument; +the experiment carrying the arms is passed with `-e`. Passing the experiment file +positionally instead would treat it as a task, which resolves to a skipped task and a green +run of zero rows. -This is a deliberate departure from the activation gate above, for one reason: the -activation gate compares **F1**, which the pooled `suite.json` cannot report per replicate — -hence three invocations. The execution gate compares per-row **`weighted_score`**, which is -exactly what `paired_comparison` already computes, correctly, over replicates it averages -per row before pairing. So the same `## Paired Comparison` block that is only *corroboration* -on the activation track is the **primary instrument** here, with a mean difference, -a 95% confidence interval, Cohen's *d* and a paired *t*-test — tested code instead of -arithmetic done by hand. - -That constrains the shape: it fires only for exactly two variants, so gate one candidate at -a time, in `round-gate.yaml`. With expensive rows that is the right trade anyway — Stage A -already ranked them. - -**Read the sign off the header, and never state a direction without it.** The block renders -as `**Paired mean diff ( - )**`, subtracting -in **variant declaration order** — not incumbent-minus-candidate, and not -better-minus-worse. With `incumbent` declared first, as in the example above, **a candidate -win reads negative**. Quote the header verbatim next to the number, and resolve the direction -from it every time rather than from memory; a reversed reading promotes the arm that lost, -and every subsequent number in the ledger corroborates it. - -Promote only when all of these hold: - -- **The paired mean difference favours the candidate and its 95% CI excludes zero.** -- **No criterion regressed.** Compare per-criterion aggregates, not just the suite score: a - candidate that lifts the average while breaking a row that used to pass has traded, and on - a body edit that trade is usually the thing you least want. -- **`completion_rate` is equal across arms**, or the difference favours the incumbent. An - eroded, asymmetric sample produces confident nonsense — a *p*-value computed over rows that - vanished from one arm is not evidence. -- The skill **actually engaged** on every scored row; otherwise part of the sample measured - the absence of the thing under test. - -Print the paired block verbatim alongside the per-criterion table. A body change is a -behavioural change, and the numbers behind it are the whole argument. - -### Stage C — confirm on the test split - -Only the best candidate that already passed Stage B, as a **two-variant** experiment -(incumbent + that candidate) at `--split test --repeats 3`. That is `round-confirm.yaml`, -a third file — re-passing the triage file here is the mistake Step 9 describes, and it costs -more while producing no paired block at all. - -Here `--repeats` is correct and required. The experiment reporter renders a -`## Paired Comparison` block in `experiment.md` — mean difference, 95% confidence interval, -Cohen's *d*, and a paired-*t* p-value — but it fires **only** for exactly two variants, and -it averages replicates per row *before* pairing, which is precisely what pooling breaks in -`suite.json` and exactly what is wanted here. - -**The same sign rule applies, and it is easiest to get wrong here** — this is the number the -promotion is reported on. It is restated rather than cross-referenced because each stage is -read on its own, and a lint sensor requires it in both. The header subtracts in -**variant declaration order** -(` - `), so with `incumbent` declared first **a candidate -win reads negative**. Quote the header verbatim beside the figure and read the direction off -it, every time. - -Report that block verbatim alongside the test F1s, and state its limit honestly: it -pairs per-row `weighted_score` — row-level correctness, an accuracy-flavoured quantity — -**not** F1. F1 remains the promotion metric; the paired block corroborates the direction on -the paired rows, it does not re-test the promotion metric. - -Require the F1 direction to reproduce on the test split. Do **not** require replicate separation -there — a test split is usually smaller and the separation usually weaker — and say that, -rather than implying a stronger result than was obtained. +The two Stage B rows differ on purpose and the method file says why — do not unify them. +Neither the promotion conditions nor the sign rule is restated here: read them there, at the +moment you apply them, rather than from memory. ## Step 11 — Ledger Append to `.optimize-skill//history.json`: round, **track**, candidate slug, -hypothesis, the train numbers (per-invocation F1 on activation; the paired block on -execution), the test numbers, per-criterion or sibling movement, `completion_rate` per -arm, verdict, and the run ids. Append-only — never rewrite an earlier round. An existing +hypothesis, **the predeclared primary criterion and its guardrails** (written before the +stage runs — see Stage B), the train numbers (per-invocation F1 on activation; the paired +block on execution), the test numbers, per-criterion or sibling movement, `completion_rate` +per arm, verdict, and the run ids. Append-only — never rewrite an earlier round. An existing directory means read `history.json` first and continue the numbering. +Writing the primary down *before* the numbers exist is what separates a predeclaration from +a rationalization, and it is the only part of this ledger that has to be recorded early. + Recording the track is what keeps the history readable: two rounds with the same skill name and incomparable metrics are otherwise indistinguishable a month later. diff --git a/src/coder_eval/cli/plan_command.py b/src/coder_eval/cli/plan_command.py index 25318717..b58e6e27 100644 --- a/src/coder_eval/cli/plan_command.py +++ b/src/coder_eval/cli/plan_command.py @@ -5,13 +5,60 @@ import typer -from ..models.tasks import UnknownTaskFieldWarning -from ..orchestration.task_loader import load_task +from ..models.tasks import TaskDefinition, UnknownTaskFieldWarning +from ..orchestration.task_loader import expand_dataset, load_dataset_rows, load_task, row_split_label from .console import console from .run_helpers import discover_default_tasks from .utils import check_api_keys, check_tools +def _preview_dataset(task: TaskDefinition, task_file: Path, split_name: str | None) -> list[TaskDefinition]: + """Expand a dataset-backed task, print its row accounting, and return the rows. + + Runs at the one surface that costs nothing, so a suite's resolved row count — the + number every cost estimate and every A/B comparison depends on — is knowable before + any money is spent. Expansion also validates row ids and every ``${row.*}`` + substitution, which previously failed per-row at run time after the sandbox was built. + + Returns ``[task]`` unchanged when the task carries no ``dataset:`` block; a "1 row" + line there would be noise about a concept the task does not have. + """ + if task.dataset is None: + return [task] + + expanded = expand_dataset(task, task_file.parent, split=split_name) + # The unfiltered total, from a list length rather than a second expand_dataset call: + # that would re-run whole-dataset id validation and full substitution over every row + # for a number already in hand. + rows = load_dataset_rows(task.dataset, task_file.parent) + # `row_split_label` is the runtime's single definition of "labelled" (expand_dataset + # and CE060 call it too) — do not re-derive the rule here. + labels = [row_split_label(r, task.dataset.split_field) for r in rows] + labelled = [x for x in labels if x is not None] + + if split_name is None: + suffix = "" + elif not labelled: + # Do not imply the selector did something. It did not: an unlabelled task passes + # through --split untouched, because --split is global to the invocation. + suffix = f" (--split {split_name}: not labelled, all rows kept)" + else: + suffix = f" (--split {split_name})" + console.print(f" [dim]Dataset: {len(rows)} rows -> {len(expanded)} selected{suffix}[/dim]") + + # The pre-spend half of the partial-labelling signal (expand_dataset also logs a + # WARNING). `plan` is the loudest of the two because it is free to run — and this is + # the state that silently shrinks every metric. + if split_name is not None and labelled and len(labelled) != len(labels): + console.print( + f" [yellow]⚠[/yellow] [yellow]{len(labels) - len(labelled)} of {len(labels)} rows carry " + + f"no '{task.dataset.split_field}' label and are DROPPED by --split; every metric would be " + + "computed over the smaller set[/yellow]" + ) + assert expanded, "expand_dataset returned no rows (the empty-dataset raise should have fired)" + return expanded + + def plan_command( task_files: list[Path] | None = typer.Argument( # noqa: B008 None, @@ -23,6 +70,17 @@ def plan_command( "-e", help="Experiment definition YAML (default: experiments/default.yaml)", ), + split: str | None = typer.Option( + None, + "--split", + help=( + "For dataset-backed tasks, preview only rows whose dataset.split_field value " + "(default field: split) matches this name — e.g. --split train / --split test, " + "matching `coder-eval run --split`. Tasks whose rows are all unlabelled are " + "unaffected; a labelled task with no row in this split is an error naming the " + "splits that exist." + ), + ), ) -> None: """Validate task files without executing (dry-run). @@ -33,6 +91,8 @@ def plan_command( - Required CLI tools are available (claude, uv) - API keys are configured - Task configuration is reasonable + - Dataset-backed tasks expand: row ids validate, ``${row.*}`` substitutions resolve, + and the selected row count is printed BEFORE any money is spent When an experiment is provided (or experiments/default.yaml exists), also shows experiment info and resolved agent configs per variant. @@ -46,9 +106,14 @@ def plan_command( coder-eval plan tasks/hello_date.yaml coder-eval plan tasks/*.yaml coder-eval plan tasks/*.yaml -e experiments/model-comparison.yaml + coder-eval plan evals/outcome.yaml --split test """ # Default to discovering all tasks under tasks/ when none provided resolved_task_files = task_files if task_files else discover_default_tasks() + # Tests call plan_command(...) directly rather than through CliRunner, so an unpassed + # option arrives as a typer OptionInfo rather than its declared default — the same + # guard `experiment` already carries below. + split_name = split if isinstance(split, str) else None console.print("\n[bold]Task Validation (Dry-Run)[/bold]\n") @@ -119,6 +184,8 @@ def plan_command( console.print(f" [dim]Success criteria: {len(task.success_criteria)}[/dim]") + expanded = _preview_dataset(task, task_file, split_name) + # Surface unknown-field warnings as inline notices (non-blocking; # catches stale top-level fields like max_iterations / llm_reviewer # that the soft-launch validator otherwise drops silently). Match @@ -134,7 +201,10 @@ def plan_command( # Show resolved agent per variant for variant in exp_def.variants: try: - resolved, _lineage, _ = resolve_task_for_variant(default_exp, task, exp_def, variant) + # Resolve the FIRST expanded row for a dataset-backed task: that is the + # shape a run actually resolves, and it is where a criterion that only + # becomes invalid after ${row.*} substitution shows up. + resolved, _lineage, _ = resolve_task_for_variant(default_exp, expanded[0], 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): diff --git a/src/coder_eval/cli/run_command.py b/src/coder_eval/cli/run_command.py index 613de08f..3eb9eac5 100644 --- a/src/coder_eval/cli/run_command.py +++ b/src/coder_eval/cli/run_command.py @@ -305,7 +305,8 @@ def run_command( "(default field: split) matches this name — e.g. --split train / --split test. " "Applied BEFORE --sample / --sample-per-stratum, so a sampled split keeps a " "predictable size. Tasks whose rows are all unlabelled are unaffected; a " - "labelled task with no row in this split is reported in skipped_tasks." + "labelled task with no row in this split aborts the run with an error naming " + "the splits that exist." ), ), repeats: int | None = typer.Option( diff --git a/src/coder_eval/criteria/skill_triggered.py b/src/coder_eval/criteria/skill_triggered.py index 50e89d20..fad835aa 100644 --- a/src/coder_eval/criteria/skill_triggered.py +++ b/src/coder_eval/criteria/skill_triggered.py @@ -3,7 +3,10 @@ Agent-agnostic. Claude Code engages a skill via an explicit ``Skill`` tool call; Codex has no such tool — it auto-discovers skills under ``.agents/skills/`` and engages one by reading its ``SKILL.md`` / references off disk via shell. Both -signals are detected here so the criterion scores identically across agents. +signals are detected here so the criterion scores identically across agents, and +both require the signal to have actually DELIVERED the body: a refused, in-flight +or crash-force-closed ``Skill`` call, and a failed or unresolved +``Read``/``Glob``/``Grep``, loaded nothing and are not engagement. """ from __future__ import annotations @@ -40,6 +43,14 @@ # lookahead permits overlapping matches such as ``.../skills/skills//...``. _SKILL_PATH_RE = re.compile(r"(?=skills[\\/]+([A-Za-z0-9][A-Za-z0-9_-]*)[\\/]+)") +# File-read tools whose FAILURE means nothing was loaded. ``Bash`` is deliberately absent: +# ``cat skills/x/SKILL.md | grep foo`` exits 1 after genuinely reading the file, so a status +# gate there would drop real Codex engagement. Antigravity IS covered — its tool map +# (``antigravity_agent.py``) renames ``view_file``/``search_directory``/``find_file`` to +# ``Read``/``Grep``/``Glob``, so its file-read engagement decides at the ToolEnd rather than +# the ToolStart: one evaluation round later, not a lost signal. +_FILE_READ_TOOLS = frozenset({"Read", "Glob", "Grep"}) + def _engaged_skill_names(cmd: CommandTelemetry) -> set[str]: """All skill names engaged by ONE command, agent-agnostically (any-skill). @@ -51,7 +62,7 @@ def _engaged_skill_names(cmd: CommandTelemetry) -> set[str]: ``parameters['skill']``, optionally namespaced (e.g. ``plugin:uipath-agents``); the namespace is stripped via ``.split(":")[-1]``. - Codex (and any non-Claude agent): no ``Skill`` tool exists, so a skill is - engaged by reading its files off disk via shell. Both the repo layout + engaged by successfully reading its files off disk. Both the repo layout (``.../skills//...``) and the sandbox symlink (``.agents/skills//...``) contain the substring ``skills//``, matched here in any string parameter (Bash ``parameters['command']`` or a @@ -63,31 +74,51 @@ def _engaged_skill_names(cmd: CommandTelemetry) -> set[str]: """ names: set[str] = set() if cmd.tool_name == "Skill": - # An ERRORED Skill call is not engagement. The most important case is a skill - # carrying ``disable-model-invocation: true``: the tool refuses it outright - # ("cannot be used with Skill tool due to disable-model-invocation"), the body is - # never loaded, and the agent proceeds on its own prior knowledge. Counting the - # attempt would report `yes` for a run in which the skill did not participate at - # all — and because the agent still produces plausible output, nothing downstream - # looks wrong. Observed on 24 of 24 rows of a real outcome suite, where it made an - # entire A/B round measure the model's background knowledge instead of the skill. - if cmd.result_status == "error": + # ALLOWLIST, not a denylist: engagement counts only on a SUCCESSFUL call. For the + # ``Skill`` tool the skill body IS the tool result, so anything other than a + # delivered result means nothing loaded. The three excluded states share that: + # - "error" — refused, typically ``disable-model-invocation: true`` ("cannot be + # used with Skill tool due to disable-model-invocation"). The agent + # proceeds on prior knowledge and still produces plausible output, + # so nothing downstream looks wrong. Observed on 24 of 24 rows of a + # real outcome suite, where it made an entire A/B round measure the + # model's background knowledge instead of the skill. + # - None — still in flight: the early-stop watcher evaluates on + # ``ToolStartEvent``, before any result exists. + # - "unknown" — force-closed by a turn crash before any result arrived. + if cmd.result_status != "success": logger.debug( - "Skill call for %r errored (%s); not counting it as engagement", + "Skill call for %r did not deliver a body (result_status=%r, %s); not counting it as engagement", cmd.parameters.get("skill"), + cmd.result_status, (cmd.result_summary or "")[:120], ) else: skill = cmd.parameters.get("skill", "") if isinstance(skill, str) and skill: names.add(skill.split(":")[-1]) + # This branch is AUTHORITATIVE for a ``Skill`` call — return rather than fall + # through to the file-read scan below, which would otherwise resurrect a call the + # gate just excluded the moment one of its own parameters contained a + # ``skills//``-shaped substring. That would restore the false `yes` and break + # the monotonicity the latch depends on. + return names # The file-read signal (Codex, or any agent reading a SKILL.md off disk) is scanned on - # every command regardless of tool, since it is a genuine engagement: the body really - # did reach the agent. This is deliberately NOT gated on result_status above — a failed - # *Skill tool call* loaded nothing, whereas a path reference means the file was opened. - for value in cmd.parameters.values(): - if isinstance(value, str): - names.update(_SKILL_PATH_RE.findall(value)) + # every non-``Skill`` command's string parameters, because the body really did reach + # the agent. + # It is gated only for the three tools whose failure means nothing was loaded: a failed + # or in-flight ``Read``/``Glob``/``Grep`` puts the path in ``parameters`` while loading + # nothing (a ``Grep`` that matched nothing exits non-zero and did not deliver a body + # either — that is intended, not collateral damage). ``None`` is excluded for these + # tools too: counting it would reintroduce the live/frozen divergence one branch over, + # with the watcher live-passing on the ToolStart of a ``Read`` that then ENOENTs. + # ``"unknown"`` still counts here — Codex reconstructs genuinely-executed calls with + # that status — and every other tool, ``Bash`` included, is ungated, because a non-zero + # exit does not imply the file was not read. + if not (cmd.tool_name in _FILE_READ_TOOLS and cmd.result_status in ("error", None)): + for value in cmd.parameters.values(): + if isinstance(value, str): + names.update(_SKILL_PATH_RE.findall(value)) return names @@ -184,9 +215,17 @@ def live_verdict( ``"pass"`` for a positive criterion (the expected skill loaded), ``"fail"`` for a distractor/negative one (a wrong skill loaded). - Because engagement is monotonic (a skill, once engaged, stays engaged), a - latched verdict never flips, so it agrees with ``_check_impl`` on the - frozen trajectory by construction — whether or not the run stopped early. + Latching is safe because engagement is computed only from RESOLVED + telemetry for the tools whose failure means nothing loaded (a ``Skill`` + call must have succeeded; a ``Read``/``Glob``/``Grep`` must not have + failed or be in flight — see ``_engaged_skill_names``). A command's + contribution can therefore only go absent -> present as it resolves, + never present -> absent, so a latched verdict never flips and agrees with + ``_check_impl`` on the frozen trajectory — whether or not the run stopped + early. The cost is that a ``Skill`` engagement decides at its + ``ToolEndEvent`` rather than its ``ToolStartEvent``: one evaluation round + later, and no stop at all if the turn is cut between the two — in which + case the frozen check scores that call ``no`` as well, which is the point. A positive criterion can therefore only ever live-``pass`` and a distractor/negative one only ever live-``fail``; their *absence* is never decidable mid-run (see ``SkillTriggeredCriterion.live_decidable_polarities`` diff --git a/src/coder_eval/models/criteria.py b/src/coder_eval/models/criteria.py index 7e54637c..bbfbb92b 100644 --- a/src/coder_eval/models/criteria.py +++ b/src/coder_eval/models/criteria.py @@ -1172,9 +1172,14 @@ class SkillTriggeredCriterion(LiveSuccessCriterion): """Binary classifier: did the agent engage the target skill during the run? Agent-agnostic. Observed label is ``"yes"`` when ``turn_records`` show the - skill engaged under ``skill_name`` — Claude via an explicit ``Skill`` tool - call, or any agent without that tool (e.g. Codex) by reading the skill's - files off disk (a command parameter contains ``skills//``). + skill engaged under ``skill_name`` — Claude via a **successful** ``Skill`` + tool call (the body is delivered AS the tool result, so a refused, in-flight + or crash-force-closed call loaded nothing), or any agent without that tool + (e.g. Codex) by reading the skill's files off disk (a command parameter + contains ``skills//``). A ``Read``/``Glob``/``Grep`` that failed + or has not resolved does not count — the path is in its parameters, but + nothing was loaded; ``Bash`` stays ungated, since ``cat … | grep`` exits + non-zero after genuinely reading the file. Otherwise ``"no"``. Expected label is ``"yes"`` iff ``expected_skill == skill_name``. Stack one criterion per skill against a single dataset labeled with @@ -1201,7 +1206,10 @@ class SkillTriggeredCriterion(LiveSuccessCriterion): description="The row's expected skill (after substitution); empty string '' for negatives.", ) skill_name: str = Field( - description="Only count Skill invocations whose 'skill' parameter matches this name.", + description=( + "Only count SUCCESSFUL Skill invocations whose 'skill' parameter matches this name " + "(or a successful file read under skills//)." + ), ) def live_decidable_polarities(self) -> frozenset[LivePolarity]: diff --git a/src/coder_eval/orchestration/early_stop.py b/src/coder_eval/orchestration/early_stop.py index 68fa2f21..673abe7d 100644 --- a/src/coder_eval/orchestration/early_stop.py +++ b/src/coder_eval/orchestration/early_stop.py @@ -415,12 +415,17 @@ def _on_event_impl(self, event: StreamEvent) -> None: the FIRST ``AgentStartEvent`` only (a retry's second AgentStart does not reset it). - The decision is evaluated on the tool *call* (``ToolStartEvent``): for an - observable criterion the verdict is fully determined by the call's inputs, - so latching here lets the agent's post-dispatch ``should_stop`` poll break - the loop before a cut-short turn can strip the result. The call is not in - the collector yet (it reduces commands from ``ToolEndEvent``), so it is - passed to ``_evaluate_impl`` as the in-flight command, reported at + The decision is evaluated on the tool *call* (``ToolStartEvent``), which + lets the agent's post-dispatch ``should_stop`` poll break the loop before a + cut-short turn can strip the result. Whether a given criterion can actually + decide there is **per-criterion, not global**: ``command_executed``'s + verdict is fully determined by the call's inputs, whereas + ``skill_triggered``'s is not — for the ``Skill`` tool the body is delivered + AS the result, so an in-flight call has engaged nothing and that criterion + deliberately stays undecided until its ``ToolEndEvent``. A new live + criterion must state which seam its verdict is decidable at. The call is + not in the collector yet (it reduces commands from ``ToolEndEvent``), so it + is passed to ``_evaluate_impl`` as the in-flight command, reported at ``tool_call_index + 1`` (it has no ``ToolEndEvent`` to count yet). The matching ``ToolEndEvent`` still evaluates, which covers a verdict that only becomes decidable once the result is known and is a no-op once a call has diff --git a/src/coder_eval/orchestration/experiment.py b/src/coder_eval/orchestration/experiment.py index afd7de41..294abd7e 100644 --- a/src/coder_eval/orchestration/experiment.py +++ b/src/coder_eval/orchestration/experiment.py @@ -40,6 +40,7 @@ from .config import BatchRunConfig from .config_merge import ConfigSource, Layer, merge_layers, resolve_root from .task_loader import ( + SplitSelectorError, expand_dataset, load_task, resolve_agent_system_prompt, @@ -554,7 +555,10 @@ def resolve_all_tasks( Task YAMLs that fail to load (YAML parse error, Pydantic validation, dataset expansion error) are recorded in the returned ``skipped`` list and - excluded from the resolved set rather than aborting the suite. + excluded from the resolved set rather than aborting the suite. The one + exception is ``SplitSelectorError``, which is re-raised: it describes a + malformed INVOCATION (a ``--split`` matching no labelled row) rather than a + malformed file, and demoting it produces a green run of zero rows. Per-task config-resolution failures (a task whose own YAML is incompatible with the resolved run — e.g. Claude-only ``sdk_options`` surviving a @@ -630,6 +634,11 @@ def resolve_all_tasks( sample_per_stratum=config.sample_per_stratum, split=config.split, ) + except SplitSelectorError: + # A CLI selector that matched nothing is user error, not repo state: demoted to + # skipped_tasks it yields a green run of zero rows, which is the loudest failure + # mode in the split workflow and the one a CI gate cannot see. Let it abort. + raise # Narrow set: real load failures only. We deliberately don't catch # AttributeError / TypeError / ImportError — those signal a regression # in load_task / expand_dataset and should crash loudly rather than diff --git a/src/coder_eval/orchestration/task_loader.py b/src/coder_eval/orchestration/task_loader.py index 3f812c29..f5e2d071 100644 --- a/src/coder_eval/orchestration/task_loader.py +++ b/src/coder_eval/orchestration/task_loader.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import logging import os import random import re @@ -30,6 +31,28 @@ _ROW_ID_PATTERN = re.compile(r"^[A-Za-z0-9_][A-Za-z0-9_.\-]*$") _ENV_VAR_PATTERN = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)\}|\$([A-Za-z_][A-Za-z0-9_]*)") +logger = logging.getLogger(__name__) + + +class SplitSelectorError(ValueError): + """A CLI ``--split`` selector eliminated every row of a labelled dataset. + + Distinct from the other dataset errors on purpose. Those describe a malformed + FILE and are demoted to ``skipped_tasks`` so one bad task cannot abort a suite; + this one describes a malformed INVOCATION — the user asked for a split that does + not exist — and there is no per-task isolation argument for it, because the same + selector is applied to every task in the run. Demoted, it produces a yellow + warning and exit 0: a CI gate that ran zero evals and reported success. + + That the abort is run-wide is the point, not a side effect: ``--split`` is global + to the invocation, so one labelled suite with no matching row means the selector + itself is wrong, and finishing the other suites would hide it. + + A ``ValueError`` subclass so every existing ``except ValueError`` caller keeps + working; ``resolve_all_tasks`` re-raises it explicitly and the CLI turns it into + a ``typer.BadParameter``. + """ + def load_task(task_file: Path) -> tuple[TaskDefinition, str]: """Load a task definition from a YAML file. @@ -305,7 +328,7 @@ def _resolve_path(p: str, task_file_dir: Path) -> Path: return path if path.is_absolute() else (task_file_dir / path).resolve() -def _load_dataset_rows(dataset: Dataset, task_file_dir: Path) -> list[dict[str, Any]]: +def load_dataset_rows(dataset: Dataset, task_file_dir: Path) -> list[dict[str, Any]]: """Load dataset rows from inline list or one or more JSONL files.""" if dataset.rows is not None: return [dict(r) for r in dataset.rows] @@ -369,21 +392,30 @@ def row_split_label(row: dict[str, Any], field: str) -> str | None: return None if value is None or value == "" else str(value) -def _reject_duplicate_row_ids(rows: list[dict[str, Any]], task: TaskDefinition) -> None: - """Raise if two rows share an id, considering the dataset as a whole. +def _validate_row_ids(rows: list[dict[str, Any]], task: TaskDefinition) -> None: + """Validate every row's id against the dataset AS A WHOLE, before any narrowing. - Called before ``--split`` filtering and sampling narrow the row set: a duplicate id is - malformed data regardless of which rows a given invocation happens to select. Rows - missing the id field are skipped here — that is reported per-row during expansion, - where the row index is known. + All three id checks live here on purpose: "the dataset is well-formed" must not + depend on what a given invocation selected. Split the checks — duplicates whole-set, + missing/malformed per-selected-row — and a malformed row sitting in the ``test`` split + validates under every ``--split train`` run and surfaces only at promotion time, which + is the most expensive moment to learn it. + + The ``Dataset row {i}`` index therefore counts over the whole dataset rather than the + selected subset, which is the more useful number: it points at the line in the file. """ assert task.dataset is not None id_field = task.dataset.id_field seen: set[str] = set() - for row in rows: + for i, row in enumerate(rows): if id_field not in row: - continue + raise ValueError(f"Dataset row {i} for task '{task.task_id}' missing id_field '{id_field}': {row}") row_id = str(row[id_field]) + if not _ROW_ID_PATTERN.match(row_id): + raise ValueError( + f"Dataset row id {row_id!r} must match {_ROW_ID_PATTERN.pattern}" + + " (letters, digits, underscore, hyphen, dot)" + ) if row_id in seen: raise ValueError(f"Duplicate dataset row id for task '{task.task_id}': {row_id!r}") seen.add(row_id) @@ -457,35 +489,40 @@ def expand_dataset( unlabelled when the field is absent, ``None``, or ``""``. A task whose rows are all unlabelled passes through unfiltered (``--split`` is global to the invocation, so an unlabelled task in a multi-task - run must not fail); partial labelling keeps the matching rows and - drops the unlabelled ones; a *labelled* task with no matching row - raises. Note the raise is caught by ``resolve_all_tasks`` into - ``skipped_tasks``, so at the run level a mistyped split name is a - skipped suite rather than an aborted run. + run must not fail); partial labelling keeps the matching rows, drops + the unlabelled ones and logs a WARNING naming the drop count; a + *labelled* task with no matching row raises ``SplitSelectorError``. + That one is NOT demoted to ``skipped_tasks`` — ``resolve_all_tasks`` + re-raises it, so a mistyped split name aborts the run instead of + producing a green run of zero rows. Returns: Expanded list of TaskDefinitions. Length is 1 when dataset is None. Raises: - ValueError: Empty dataset, duplicate row ids, missing id_field, - malformed row id, or a labelled dataset with no row in ``split``. + ValueError: Empty dataset, duplicate row ids, missing id_field, or a + malformed row id — all malformed-FILE errors, which + ``resolve_all_tasks`` demotes to ``skipped_tasks``. + SplitSelectorError: A labelled dataset has no row in ``split``. A + ``ValueError`` subclass, but ``resolve_all_tasks`` re-raises this one + (it describes a malformed INVOCATION, not a malformed file). FileNotFoundError: Dataset path does not exist. """ if task.dataset is None: return [task] - rows = _load_dataset_rows(task.dataset, task_file_dir) + rows = load_dataset_rows(task.dataset, task_file_dir) if not rows: raise ValueError(f"Dataset for task '{task.task_id}' is empty") # --split filters BEFORE either sampler below: sampling first would leave an # unpredictable (possibly zero) number of rows per split, destroying the # train/test comparison the split exists to protect. - # Duplicate ids are a property of the DATASET, so check the whole row set BEFORE any - # filtering or sampling narrows it. Checking only what survives would let a duplicate - # sitting in an unselected split validate under every --split and surface only on a - # full run — and the split workflow always passes one. - _reject_duplicate_row_ids(rows, task) + # Row ids are a property of the DATASET, so check the whole row set BEFORE any + # filtering or sampling narrows it. Checking only what survives would let a malformed, + # id-less or duplicate row sitting in an unselected split validate under every --split + # and surface only on a full run — and the split workflow always passes one. + _validate_row_ids(rows, task) if split is not None: field = task.dataset.split_field @@ -493,9 +530,26 @@ def expand_dataset( # value, so they cannot drift apart into two definitions of "labelled". labelled = [(r, label) for r in rows if (label := row_split_label(r, field)) is not None] if labelled: - rows = [r for r, label in labelled if label == split] + # A PARTLY labelled dataset is the dangerous state: the unlabelled rows are + # dropped (the safe direction), so the run succeeds and every metric is + # computed over a smaller suite than the file suggests. Say so. Guarded on an + # actual drop, not on --split being set — a fully labelled dataset drops + # nothing and must stay quiet, or the warning becomes noise and gets ignored. + matching = [r for r, label in labelled if label == split] + if len(labelled) != len(rows): + logger.warning( + "Task '%s': --split %r kept %d of %d rows; %d row(s) carry no %r label and were " + + "DROPPED. Every metric below is computed over the smaller set.", + task.task_id, + split, + len(matching), + len(rows), + len(rows) - len(labelled), + field, + ) + rows = matching if not rows: - raise ValueError( + raise SplitSelectorError( f"Dataset for task '{task.task_id}' has no rows in split {split!r} " + f"(split_field={field!r}); labelled splits present: " + f"{sorted({label for _, label in labelled})}" @@ -524,17 +578,10 @@ def expand_dataset( id_field = task.dataset.id_field expanded: list[TaskDefinition] = [] - for i, row in enumerate(rows): - if id_field not in row: - raise ValueError(f"Dataset row {i} for task '{task.task_id}' missing id_field '{id_field}': {row}") + for row in rows: + # No validation here: all three id checks ran over the whole dataset in + # _validate_row_ids, before filtering and sampling narrowed `rows`. row_id = str(row[id_field]) - if not _ROW_ID_PATTERN.match(row_id): - raise ValueError( - f"Dataset row id {row_id!r} must match {_ROW_ID_PATTERN.pattern}" - + " (letters, digits, underscore, hyphen, dot)" - ) - # Uniqueness is already enforced across the whole dataset by - # _reject_duplicate_row_ids, before filtering narrowed `rows`. data = task.model_dump(exclude_unset=True) if isinstance(data.get("initial_prompt"), str): diff --git a/tasks/skills/ci-outcome-rows.jsonl b/tasks/skills/ci-outcome-rows.jsonl index de7438be..bb39bb8a 100644 --- a/tasks/skills/ci-outcome-rows.jsonl +++ b/tasks/skills/ci-outcome-rows.jsonl @@ -1,10 +1,10 @@ -{"id": "pr-gate", "expected_skill": "ci", "split": "train", "scenario": "Wire our evals into GitHub Actions so they gate pull requests, and fail the build if any task scores below 0.7. Put it in .github/workflows/evals.yml.", "expected_path": ".github/workflows/evals.yml", "expected_snippet": "minimum-task-score"} -{"id": "schedule-weekly", "expected_skill": "ci", "split": "train", "scenario": "Set up a weekly scheduled run of our eval suite so regressions surface before users hit them. Write it to .github/workflows/eval-schedule.yml.", "expected_path": ".github/workflows/eval-schedule.yml", "expected_snippet": "cron:"} -{"id": "tasks-two-depths", "expected_skill": "ci", "split": "train", "scenario": "Add CI that runs every eval in this repo — note they are not all at the same depth. Write it to .github/workflows/eval-all.yml.", "expected_path": ".github/workflows/eval-all.yml", "expected_snippet": "evals/suite/*.yaml"} -{"id": "skill-source-path", "expected_skill": "ci", "split": "train", "scenario": "Set up CI for our evals at .github/workflows/eval-activation.yml. One thing to watch: last time someone tried this, the activation rows failed in CI every time while passing locally.", "expected_path": ".github/workflows/eval-activation.yml", "expected_snippet": "SKILL_SOURCE_PATH"} -{"id": "regression-node-runtime", "expected_skill": "ci", "split": "train", "scenario": "Add a pull-request CI gate for our evals at .github/workflows/eval-pr.yml.", "expected_path": ".github/workflows/eval-pr.yml", "expected_snippet": "@anthropic-ai/claude-code"} -{"id": "regression-least-privilege", "expected_skill": "ci", "split": "train", "scenario": "Add a workflow at .github/workflows/eval-guard.yml that runs our eval suite on pull requests. This repo takes outside contributions.", "expected_path": ".github/workflows/eval-guard.yml", "expected_snippet": "persist-credentials: false"} -{"id": "both-triggers", "expected_skill": "ci", "split": "test", "scenario": "I want our evals to run both on every pull request and on a weekly cron, in one workflow. Write it to .github/workflows/eval-both.yml.", "expected_path": ".github/workflows/eval-both.yml", "expected_snippet": "cron:"} -{"id": "junit-report", "expected_skill": "ci", "split": "test", "scenario": "Set up CI for our evals and make the per-task results show up in GitHub's test report UI. Put it at .github/workflows/eval-junit.yml.", "expected_path": ".github/workflows/eval-junit.yml", "expected_snippet": "junit-path"} -{"id": "experiment-passthrough", "expected_skill": "ci", "split": "test", "scenario": "Our suite under evals/suite/ resolves through an experiment. Add CI for it at .github/workflows/eval-suite.yml.", "expected_path": ".github/workflows/eval-suite.yml", "expected_snippet": "extra-args"} -{"id": "regression-setup-node", "expected_skill": "ci", "split": "test", "scenario": "Add a scheduled weekly eval run at .github/workflows/eval-weekly.yml.", "expected_path": ".github/workflows/eval-weekly.yml", "expected_snippet": "actions/setup-node"} +{"id": "pr-gate", "expected_skill": "ci", "split": "train", "scenario": "Wire our evals into GitHub Actions so they gate pull requests, and fail the build if any task scores below 0.7. Put it in .github/workflows/evals.yml.", "expected_path": ".github/workflows/evals.yml", "expected_snippet": "minimum-task-score", "expected_snippet_2": "minimum-task-score"} +{"id": "schedule-weekly", "expected_skill": "ci", "split": "train", "scenario": "Set up a weekly scheduled run of our eval suite so regressions surface before users hit them. Write it to .github/workflows/eval-schedule.yml.", "expected_path": ".github/workflows/eval-schedule.yml", "expected_snippet": "cron:", "expected_snippet_2": "cron:"} +{"id": "tasks-two-depths", "expected_skill": "ci", "split": "train", "scenario": "Add CI that runs every eval in this repo — note they are not all at the same depth. Write it to .github/workflows/eval-all.yml.", "expected_path": ".github/workflows/eval-all.yml", "expected_snippet": "evals/suite/*.yaml", "expected_snippet_2": "evals/suite/*.yaml"} +{"id": "skill-source-path", "expected_skill": "ci", "split": "train", "scenario": "Set up CI for our evals at .github/workflows/eval-activation.yml. One thing to watch: last time someone tried this, the activation rows failed in CI every time while passing locally.", "expected_path": ".github/workflows/eval-activation.yml", "expected_snippet": "SKILL_SOURCE_PATH", "expected_snippet_2": "SKILL_SOURCE_PATH"} +{"id": "regression-node-runtime", "expected_skill": "ci", "split": "train", "scenario": "Add a pull-request CI gate for our evals at .github/workflows/eval-pr.yml.", "expected_path": ".github/workflows/eval-pr.yml", "expected_snippet": "@anthropic-ai/claude-code", "expected_snippet_2": "@anthropic-ai/claude-code"} +{"id": "regression-least-privilege", "expected_skill": "ci", "split": "train", "scenario": "Add a workflow at .github/workflows/eval-guard.yml that runs our eval suite on pull requests. This repo takes outside contributions.", "expected_path": ".github/workflows/eval-guard.yml", "expected_snippet": "persist-credentials: false", "expected_snippet_2": "persist-credentials: false"} +{"id": "both-triggers", "expected_skill": "ci", "split": "test", "scenario": "I want our evals to run both on every pull request and on a weekly cron, in one workflow. Write it to .github/workflows/eval-both.yml.", "expected_path": ".github/workflows/eval-both.yml", "expected_snippet": "cron:", "expected_snippet_2": "pull_request:"} +{"id": "junit-report", "expected_skill": "ci", "split": "test", "scenario": "Set up CI for our evals and make the per-task results show up in GitHub's test report UI. Put it at .github/workflows/eval-junit.yml.", "expected_path": ".github/workflows/eval-junit.yml", "expected_snippet": "junit-path", "expected_snippet_2": "junit-path"} +{"id": "experiment-passthrough", "expected_skill": "ci", "split": "test", "scenario": "Our suite under evals/suite/ resolves through an experiment. Add CI for it at .github/workflows/eval-suite.yml.", "expected_path": ".github/workflows/eval-suite.yml", "expected_snippet": "extra-args", "expected_snippet_2": "extra-args"} +{"id": "regression-setup-node", "expected_skill": "ci", "split": "test", "scenario": "Add a scheduled weekly eval run at .github/workflows/eval-weekly.yml.", "expected_path": ".github/workflows/eval-weekly.yml", "expected_snippet": "actions/setup-node", "expected_snippet_2": "actions/setup-node"} diff --git a/tasks/skills/ci-outcome.yaml b/tasks/skills/ci-outcome.yaml index 499d68ac..cac31fd6 100644 --- a/tasks/skills/ci-outcome.yaml +++ b/tasks/skills/ci-outcome.yaml @@ -181,11 +181,24 @@ success_criteria: # `found / total` over its `includes` — a constant sub-check would put a fixed # contribution in every row's score in every arm, compressing the variance the gate # reads and making `mean: 0.7` quietly weaker than it looks. + # + # Two `includes` slots because a scenario may ask for two things. `both-triggers` asks + # for a pull_request trigger AND a weekly cron, and graded on `cron:` alone a + # schedule-only workflow scored a clean 1.0 — while `cron:` is also what its sibling + # `schedule-weekly` grades, so the row discriminated nothing its sibling did not. + # + # The other nine rows set `expected_snippet_2` to the SAME value as `expected_snippet`. + # That is deliberate and is the part not to "improve": two identical entries score + # `0/2` or `2/2`, i.e. numerically identical to a single include, so no constant + # sub-check is introduced and the variance the `mean: 0.7` gate reads is untouched. + # A row with a genuinely second requirement is where the slot earns its place. + # (The note lives here because a JSONL cannot carry a comment.) - type: "file_check" description: "the workflow row ${row.id} asked for" path: "${row.expected_path}" includes: - "${row.expected_snippet}" + - "${row.expected_snippet_2}" suite_thresholds: mean: 0.7 completion_rate: 1.0 diff --git a/tests/test_custom_lint.py b/tests/test_custom_lint.py index c4e54c42..a5e3c293 100644 --- a/tests/test_custom_lint.py +++ b/tests/test_custom_lint.py @@ -1299,7 +1299,17 @@ def test_drift_is_detected(self, tmp_path: Path): # The surfaces that must name every shipped skill, so a new one cannot ship # undocumented. Adding a surface is one edit here. -SKILL_DOC_SURFACES = ("plugins/coder-eval/README.md", "docs/PLUGIN.md", "README.md", "CLAUDE.md") +SKILL_DOC_SURFACES = ( + "plugins/coder-eval/README.md", + "docs/PLUGIN.md", + "README.md", + "CLAUDE.md", + # The tutorial enumerates the commands a reader will see after installing, so a skill + # missing here is a skill they are told does not exist. Added after it shipped a stale + # "six commands" list omitting `optimize-skill`: the count sensor below already existed, + # but this file was not one of the surfaces it read. + "docs/tutorials/07-plugin-in-claude-code.md", +) # Claude Code loads a listing of every skill's name and description into context. # The listing's character budget scales at ~1% of the model's context window and is @@ -1370,7 +1380,10 @@ def _wrong_skill_count_offenders(surfaces: dict[str, Path], *, count: int, auto: offenders += [ f"{name}: '{word} {noun}'" for word in wrong_total - for noun in ("skills", "slash commands") + # "commands" is how the tutorial phrases it, and its absence here is exactly + # why a stale "six commands" survived: the count was guarded in three + # phrasings, and the surface used a fourth. + for noun in ("skills", "slash commands", "commands") if f"{word} {noun}" in text ] offenders += [f"{name}: 'The other {word}'" for word in wrong_subset if f"The other {word}" in text] @@ -1648,11 +1661,39 @@ def test_outcome_template_thresholds_use_real_metric_keys(self): f"{criterion.type!r} (available: {sorted(available)})" ) + def test_activation_template_caps_turns_and_isolates(self): + # The template preaches both of these and used to ship neither, so a user who copied + # it got the opposite of the advice they were reading. + # + # The cap is about SIGNAL, not only cost: activation is decided in the first + # assistant turn, and an uncapped row spends turns exploring a sandbox that + # deliberately holds no eval files. A row that times out is EXCLUDED from the + # confusion matrix rather than scored, so it never shows up as a bad number — only + # as a denominator that quietly shrank. + from coder_eval.orchestration.task_loader import load_task + + task, _ = load_task(self.TEMPLATES / "activation.yaml") + limits = task.run_limits + assert limits is not None and limits.max_turns == 2, ( + "the activation template must cap max_turns at 2, matching the worked example in " + "tasks/skills/lint-tasks-activation.yaml — activation is decided in the first " + "assistant turn, and an uncapped row erodes the confusion-matrix denominator" + ) + assert limits.turn_timeout == 120 and limits.task_timeout == 300, ( + "the activation template's timeouts must match the worked example key for key" + ) + assert task.agent is not None and task.agent.setting_sources == [], ( + "the activation template must set `agent.setting_sources: []`. This suite measures " + "the skill LISTING; inheriting the host project's CLAUDE.md injects a large project " + "guide into every call — expensive, and a confound on the thing being measured" + ) + def test_outcome_template_caps_cost(self): - # An outcome row is a FULL task run, so the template needs a per-row brake the - # activation template does not. An absolute floor, deliberately not a comparison - # against activation.yaml: that file ships no `run_limits:` block at all, so there - # is nothing to compare against. + # An outcome row is a FULL task run, so the template needs a per-row COST brake the + # activation template does not. Both templates now carry `run_limits:`, but they cap + # for opposite reasons — activation for signal (see the test above), outcome for + # spend — so this stays an ABSOLUTE floor rather than a comparison against the other + # file, which would encode a relationship that does not exist. from coder_eval.orchestration.task_loader import load_task task, _ = load_task(self.TEMPLATES / "outcome.yaml") @@ -1729,6 +1770,30 @@ def test_checked_in_outcome_sample_matches_the_shipped_template_shape(self): "asserts only engagement is an activation suite with a bigger bill" ) + # Every row must supply every ${row.*} field the criteria reference. Criteria are + # copied to EVERY row, so a field only some rows carry raises KeyError mid-expansion + # — a failure that costs nothing here and a whole stage's setup at run time. This is + # also what makes a second `includes` slot safe to add: it is only optional-looking. + import json + import re + + referenced = set( + re.findall( + r"\$\{row\.([A-Za-z_][A-Za-z0-9_]*)\}", + (self.REPO_ROOT / "tasks" / "skills" / "ci-outcome.yaml").read_text(), + ) + ) + rows = [ + json.loads(line) + for line in (sample.parent / "ci-outcome-rows.jsonl").read_text().splitlines() + if line.strip() + ] + for row in rows: + missing = referenced - set(row) + assert not missing, ( + f"row {row.get('id')!r} is missing {sorted(missing)}, referenced as ${{row.*}} in the suite" + ) + def test_checked_in_outcome_fixture_lets_the_skill_act(self): # The fixture is load-bearing, and every way it can be wrong is SILENT at full cost. # `ci` stops outright on a repo with no `.github/`, so a fixture missing it scores @@ -1819,7 +1884,7 @@ def test_reachability_guidance_names_the_plugin_root_layout(self): "ci": PLUGIN_ROOT / "skills" / "ci" / "SKILL.md", "docs/PLUGIN.md": self.REPO_ROOT / "docs" / "PLUGIN.md", "tutorial 07": self.REPO_ROOT / "docs" / "tutorials" / "07-plugin-in-claude-code.md", - "tutorial 08": self.REPO_ROOT / "docs" / "tutorials" / "08-optimizing-a-skill.md", + "tutorial 08": self.REPO_ROOT / "docs" / "tutorials" / "08-optimizing-a-skill-description.md", "tutorial 09": self.REPO_ROOT / "docs" / "tutorials" / "09-optimizing-a-skill-body.md", } for name, path in surfaces.items(): @@ -2104,7 +2169,17 @@ def test_optimize_skill_keeps_its_load_bearing_instructions(self): # every one of these instructions is something a well-meaning edit would "simplify" # away, leaving a skill that still reads plausibly and measures nothing. Same # deletion-sensor shape as the lint-tasks read-only guard above. - text = _normalized(PLUGIN_ROOT / "skills" / "optimize-skill" / "SKILL.md") + # TWO surfaces, and which one a token belongs to is the reviewable part of the + # split. `SKILL.md` holds the PROCEDURE — which suite, which files, which commands, + # in what order — so a procedure token asserted anywhere else would mean the step + # that needs it no longer states it. `reference/optimize-method.md` holds the + # track-invariant METHOD — the cost table, what each stage bounds, why the two gates + # differ, the sign rule — which is identical on both tracks and is what has to be + # right for a verdict to mean anything. Tokens that may legitimately live in either + # are asserted against the concatenation. + skill = _normalized(PLUGIN_ROOT / "skills" / "optimize-skill" / "SKILL.md") + method = _normalized(PLUGIN_ROOT / "reference" / "optimize-method.md") + text = skill + " " + method # The single most important invariant. Suite rollups pool replicates, so --repeats # writes ONE pooled suite.json and the per-replicate F1 the Stage B gate reads would @@ -2255,6 +2330,35 @@ def test_optimize_skill_keeps_its_load_bearing_instructions(self): ): assert token in text, f"optimize-skill lost {token!r} — {why}" + # The PROCEDURE half, asserted against `SKILL.md` alone. Moving any of these into + # the method file would leave the step that must act on it silently pointing + # elsewhere — which is the failure mode the extraction itself could introduce. + for token, why in ( + ("route to the execution track", "Step 2's routing decision"), + ("Never run both tracks in one round", "Step 3's one-variable-per-round rule"), + ("ONE dataset-backed task", "Step 4's hard constraint on the suite's shape"), + ("Invoke the skill from the prompt", "Step 4's activation-held-constant rule"), + ("Use the slash form", "Step 4's only way to reach a disable-model-invocation skill"), + ("Hand over the template itself", "Step 4's handover to /coder-eval:task"), + ("copied unchanged", "Step 8's snapshot must carry the sibling skills"), + ("/coder-eval:check-skill", "the sibling this skill hands control back to"), + ("round-triage.yaml", "Step 9's per-stage experiment file"), + ("round-gate.yaml", "Step 9's per-stage experiment file"), + ("round-confirm.yaml", "Step 9's per-stage experiment file"), + ): + assert token in skill, ( + f"optimize-skill's SKILL.md lost {token!r} — {why}. This is PROCEDURE: it must stay " + f"in the skill, not move to reference/optimize-method.md" + ) + + # An extracted reference nothing points at is a deleted reference. Mirrors the + # reference/task-rubric.md pointer sensor. + assert "${CLAUDE_PLUGIN_ROOT}/reference/optimize-method.md" in skill, ( + "optimize-skill's SKILL.md no longer points at reference/optimize-method.md, so the " + "cost table, the gate rules and the sign rule are unreachable from the procedure " + "that has to apply them" + ) + # The paired-diff sign rule has to appear in BOTH gates that read the block — # Stage B (execution) and Stage C — because each is a separate decision point and a # reader lands on one or the other. Counted rather than `in text`: a presence check @@ -2263,10 +2367,15 @@ def test_optimize_skill_keeps_its_load_bearing_instructions(self): # variant_ids[0]/[1]), so with `incumbent` declared first a candidate win is NEGATIVE # — and a reversed reading promotes the arm that lost, with every later number in the # ledger corroborating it. + # Counted against the METHOD file, because both decision points moved there together + # (Stage B and Stage C are adjacent sections of it). The reason for TWO is unchanged + # by the move: each stage is a separate decision point a reader lands on + # independently, so a cross-reference from one to the other is not good enough. for phrase, expected in (("variant declaration order", 2), ("a candidate win reads negative", 2)): - assert text.count(phrase) >= expected, ( - f"optimize-skill states {phrase!r} {text.count(phrase)} time(s); both the execution " - f"Stage B gate and Stage C must carry the paired-diff sign rule, so it needs {expected}" + assert method.count(phrase) >= expected, ( + f"reference/optimize-method.md states {phrase!r} {method.count(phrase)} time(s); both " + f"the execution Stage B gate and Stage C must carry the paired-diff sign rule, so it " + f"needs {expected}" ) # The cost table's symbols must match the prose that defines them. It shipped @@ -2332,6 +2441,95 @@ def test_skill_docs_surfaces_list_every_skill(self, skill: Path): ] assert not missing, f"{name} is not documented in {missing} — a shipped skill nobody can discover" + def test_tutorial_index_lists_every_tutorial(self): + # `docs/tutorials/README.md` is the index a reader lands on, and it is hand-written + # while the set of tutorials is a directory listing. mkdocs' nav is guarded by CE028, + # but nothing read this table — so a tutorial added without a row here is invisible + # to anyone who does not already know its filename. + tut_dir = self.REPO_ROOT / "docs" / "tutorials" + index = (tut_dir / "README.md").read_text(encoding="utf-8") + pages = sorted(p.name for p in tut_dir.glob("*.md") if p.name != "README.md") + missing = [name for name in pages if f"({name})" not in index] + assert not missing, ( + f"docs/tutorials/README.md's table does not link {missing}. A tutorial nobody links " + f"is a tutorial nobody finds." + ) + + def test_tutorial_09_excerpts_match_the_committed_suite(self): + # Tutorial 09 quotes `tasks/skills/ci-outcome.yaml` and its rows file as excerpts. + # An excerpt is a derived surface: when the suite gained a second `includes` slot + # and an `expected_snippet_2` field, the page kept showing the old shape and a + # reader copying it would build a suite that raises at expansion. Compared against + # the real files rather than pinned, so the suite stays free to change. + import json + import re + + page = (self.REPO_ROOT / "docs" / "tutorials" / "09-optimizing-a-skill-body.md").read_text(encoding="utf-8") + rows = [ + json.loads(ln) + for ln in (self.REPO_ROOT / "tasks" / "skills" / "ci-outcome-rows.jsonl") + .read_text(encoding="utf-8") + .splitlines() + if ln.strip() + ] + real_fields = set(rows[0]) + + # 1. The illustrative JSONL row must carry exactly the fields a real row does. + shown = [json.loads(m) for m in re.findall(r'^\{"id".*\}$', page, re.M)] + assert shown, "tutorial 09 no longer shows a sample dataset row; this sensor reads it" + for row in shown: + assert set(row) == real_fields, ( + f"tutorial 09's sample row has fields {sorted(set(row))}, but a committed row has " + f"{sorted(real_fields)}. A reader copies that shape — a missing ${{row.*}} field " + f"raises at expansion." + ) + + # 2. Every ${row.*} the page's YAML excerpt references must exist on a real row. + referenced = set(re.findall(r"\$\{row\.([A-Za-z_][A-Za-z0-9_]*)\}", page)) + missing = referenced - real_fields + assert not missing, f"tutorial 09 references ${{row.{sorted(missing)}}}, which no committed row carries" + + # 3. The excerpt must not UNDER-state the gated criterion: if the suite grades two + # snippets, showing one teaches a shape that scores differently from the file. + suite = (self.REPO_ROOT / "tasks" / "skills" / "ci-outcome.yaml").read_text(encoding="utf-8") + for field in ("expected_snippet", "expected_snippet_2"): + if f"${{row.{field}}}" in suite: + assert f"${{row.{field}}}" in page, ( + f"the committed suite grades ${{row.{field}}} but tutorial 09's excerpt omits it" + ) + + def test_docs_state_the_right_criterion_type_count(self): + # Same drift class as the skill count below, one layer down: a hand-maintained + # number describing a set the registry derives. It had already rotted — three sites + # said 14 against a registry of 15 — and nothing noticed, because the only guarded + # surface was CLAUDE.md's heading, which happened to be right. + # + # Derived from the registry, never from a literal here: adding a criterion must not + # require editing this test, only the prose it points at. + import re + + from coder_eval.criteria import CriterionRegistry, init_criteria + + init_criteria(validate=False) + count = len(CriterionRegistry.list_types()) + # \b on the number, or "5 criterion types" matches inside a correct "15 criterion + # types" and the sensor reports a failure that is not there. + patterns = (r"\b(\d+) criterion types", r"\b(\d+) success criteria types", r"Success Criteria \((\d+) types\)") + offenders = [] + for rel in ( + "docs/TASK_DEFINITION_GUIDE.md", + "CLAUDE.md", + "README.md", + *sorted(str(p.relative_to(self.REPO_ROOT)) for p in (self.REPO_ROOT / "docs" / "tutorials").glob("*.md")), + ): + text = _normalized(self.REPO_ROOT / rel) + for pat in patterns: + offenders += [f"{rel}: {m.group(0)!r}" for m in re.finditer(pat, text) if int(m.group(1)) != count] + assert not offenders, ( + f"the registry has {count} criterion types, but these surfaces state another count: " + f"{offenders}. The count is derived from `CriterionRegistry.list_types()` — update the prose." + ) + def test_skill_docs_surfaces_state_the_right_count(self): # The companion to the test above, which only checks that each NAME appears. These # surfaces also state the count in prose, and adding the sixth skill meant hand-editing @@ -2430,13 +2628,62 @@ def test_cli_driving_skills_are_named_in_the_install_prose(self): "bare `command not found` mid-task." ) + def test_tutorial_08_row_table_matches_the_committed_jsonl(self): + # The page's Step-1 table claimed 21 rows against a file holding 28, with the + # `analyze` row count off by seven — added in Part 2 and never back-propagated. A + # reader sizing their own suite from that table sizes it from a number that has not + # been true for two revisions, and nothing in the build noticed. + # + # Derived, not duplicated: the JSONL is the source and the table is the surface, so + # this compares the two rather than pinning either. + import collections + import json + import re + + page = (self.REPO_ROOT / "docs" / "tutorials" / "08-optimizing-a-skill-description.md").read_text( + encoding="utf-8" + ) + rows_file = self.REPO_ROOT / "tasks" / "skills" / "lint-tasks-activation-rows.jsonl" + rows = [json.loads(ln) for ln in rows_file.read_text(encoding="utf-8").splitlines() if ln.strip()] + + actual = collections.Counter(r["expected_skill"] for r in rows) + by_split = collections.Counter((r["expected_skill"], r.get("split")) for r in rows) + + # | Kind | `expected_skill` | Total | train | test | + table = re.findall( + r"^\|\s*(?:Positive|Distractor|Sibling-owned)\s*\|\s*`([^`]*)`\s*\|\s*(\d+)\s*\|\s*(\d+)\s*\|\s*(\d+)\s*\|", + page, + re.M, + ) + assert table, "tutorial 08's Step-1 row table is gone or reshaped; this sensor reads it by column" + + stated_total = 0 + for raw_skill, total, train, test in table: + skill = "" if raw_skill == '""' else raw_skill + stated_total += int(total) + assert (int(total), int(train), int(test)) == ( + actual[skill], + by_split[(skill, "train")], + by_split[(skill, "test")], + ), ( + f"tutorial 08's table says {skill!r} has {total} rows ({train} train / {test} test); " + f"{rows_file.name} has {actual[skill]} ({by_split[(skill, 'train')]}/{by_split[(skill, 'test')]}). " + "Recompute the table from the file — a reader sizes their own suite from it." + ) + assert stated_total == len(rows), ( + f"tutorial 08's table rows sum to {stated_total}; the committed JSONL has {len(rows)} rows" + ) + assert f"**{len(rows)} rows**" in page, ( + f"tutorial 08 no longer states the committed row count as **{len(rows)} rows** in prose" + ) + def test_tutorial_08_shows_stage_b_as_three_separate_invocations(self): # The single most dangerous edit in this whole area. Suite rollups are keyed on # (variant, suite), so `--repeats 3` pools all three replicates into ONE suite.json # with one confusion matrix — the per-replicate F1 the activation gate compares # would not exist, and the gate would silently compare a number against itself. # Now that the page shows real command lines, "simplifying" them is a one-line edit. - page = self.REPO_ROOT / "docs" / "tutorials" / "08-optimizing-a-skill.md" + page = self.REPO_ROOT / "docs" / "tutorials" / "08-optimizing-a-skill-description.md" lines = page.read_text(encoding="utf-8").splitlines() # Matched on heading TEXT at any level: pinning `###` broke the first time the page @@ -4208,9 +4455,9 @@ class TestCE060SplitLabelsAllOrNothing: @staticmethod def _split_labels(task, task_file_dir: Path) -> list[str | None]: """Each row's split label, using the runtime's own definition of "labelled".""" - from coder_eval.orchestration.task_loader import _load_dataset_rows, row_split_label + from coder_eval.orchestration.task_loader import load_dataset_rows, row_split_label - rows = _load_dataset_rows(task.dataset, task_file_dir) + rows = load_dataset_rows(task.dataset, task_file_dir) # The CONFIGURED field name, never the literal "split" — a dataset may name it # anything, and keying on the default would silently pass every such suite. return [row_split_label(row, task.dataset.split_field) for row in rows] @@ -4243,52 +4490,36 @@ def test_repo_tasks_are_fully_labelled_or_not_at_all(self, path: Path): f"nothing in the run reporting it. Label the remaining rows (do not exempt)." ) - def _task_from_rows(self, tmp_path: Path, rows: list[dict], split_field: str = "split"): - """A minimal dataset-backed task over inline rows.""" - from coder_eval.models import Dataset, FileExistsCriterion, TaskDefinition - - return TaskDefinition( - task_id="t", - description="split-label fixture", - initial_prompt="${row.id}", - success_criteria=[FileExistsCriterion(description="d", path="out.txt")], - dataset=Dataset(rows=rows, split_field=split_field), - ) - def test_detects_a_partly_labelled_dataset(self, tmp_path: Path): - task = self._task_from_rows( - tmp_path, [{"id": "a", "split": "train"}, {"id": "b", "split": "test"}, {"id": "c"}] - ) + task = _dataset_task([{"id": "a", "split": "train"}, {"id": "b", "split": "test"}, {"id": "c"}]) assert self._offenders(task, tmp_path) == "2 of 3 rows carry a split label" def test_fully_labelled_dataset_is_not_flagged(self, tmp_path: Path): - task = self._task_from_rows(tmp_path, [{"id": "a", "split": "train"}, {"id": "b", "split": "test"}]) + task = _dataset_task([{"id": "a", "split": "train"}, {"id": "b", "split": "test"}]) assert self._offenders(task, tmp_path) is None def test_fully_unlabelled_dataset_is_not_flagged(self, tmp_path: Path): # Legal and safe: `--split` then does not apply to this task at all. - task = self._task_from_rows(tmp_path, [{"id": "a"}, {"id": "b"}]) + task = _dataset_task([{"id": "a"}, {"id": "b"}]) assert self._offenders(task, tmp_path) is None def test_zero_counts_as_a_label(self, tmp_path: Path): # A falsy 0 is a real label, not a missing value — the split filter compares via # str(), so `--split 0` selects it. Treating it as unlabelled would make a fully # labelled dataset read as partly labelled. - task = self._task_from_rows(tmp_path, [{"id": "a", "split": 0}, {"id": "b", "split": 1}]) + task = _dataset_task([{"id": "a", "split": 0}, {"id": "b", "split": 1}]) assert self._offenders(task, tmp_path) is None def test_explicit_null_and_empty_string_count_as_unlabelled(self, tmp_path: Path): # Pins the (None, "") convention. A half-labelled JSONL carries explicit nulls, and # an empty string is the same "no value here" state. - task = self._task_from_rows( - tmp_path, [{"id": "a", "split": "train"}, {"id": "b", "split": None}, {"id": "c", "split": ""}] - ) + task = _dataset_task([{"id": "a", "split": "train"}, {"id": "b", "split": None}, {"id": "c", "split": ""}]) assert self._offenders(task, tmp_path) == "1 of 3 rows carry a split label" def test_rule_keys_on_the_configured_split_field(self, tmp_path: Path): # Not the literal "split". A dataset naming its field anything else would otherwise # read as fully unlabelled and pass no matter how it was labelled. - task = self._task_from_rows(tmp_path, [{"id": "a", "fold": "train"}, {"id": "b"}], split_field="fold") + task = _dataset_task([{"id": "a", "fold": "train"}, {"id": "b"}], split_field="fold") assert self._offenders(task, tmp_path) == "1 of 2 rows carry a split label" def test_expand_dataset_keeps_exactly_the_rows_the_convention_names(self, tmp_path: Path): @@ -4308,12 +4539,25 @@ def test_expand_dataset_keeps_exactly_the_rows_the_convention_names(self, tmp_pa {"id": "d", "split": None}, {"id": "e", "split": ""}, ] - task = self._task_from_rows(tmp_path, rows) + task = _dataset_task(rows) for split, expected in (("train", {"a"}), ("test", {"b"}), ("0", {"c"})): kept = {t.task_id.split("/")[-1] for t in expand_dataset(task, tmp_path, split=split)} assert kept == expected, f"split={split!r}: expand_dataset kept {kept}, expected {expected}" +# Fields naming WHERE an artifact goes, not WHAT it must contain. A prompt may say +# "write it to .github/workflows/evals.yml" — that removes filename nondeterminism from +# the measurement without revealing the graded behaviour. `skill_name` is a locator for +# the same reason: it names WHICH skill must engage, while the graded thing is the +# engagement EVENT, which no prompt can supply. The outcome pattern this plugin +# prescribes puts the skill name in every prompt by design. +CE061_LOCATOR_FIELDS = ("path", "agent_file", "file_path", "command", "skill_name") + +# Shorter values collide by chance ("ci", "0.7"); a leak worth flagging is a substantive +# string the author put in both places. +CE061_MIN_LEAK_CHARS = 12 + + @pytest.mark.lint class TestCE061RowPromptsDoNotLeakWhatTheyGrade: """CE061 — a dataset row's prompt must not contain the value a criterion grades it on. @@ -4329,23 +4573,28 @@ class TestCE061RowPromptsDoNotLeakWhatTheyGrade: with a recursive wildcard" while grading an explicit glob). That form needs a reader, and is what `lint-tasks` and code review are for. Guarding the blunt case is still worth it: it is the easy mistake, and it is silent. - """ - @pytest.mark.parametrize( - "path", - sorted(p for p in (Path(__file__).parent.parent / "tasks").rglob("*.yaml") if p.name != "metadata.yaml"), - ids=lambda p: p.relative_to(Path(__file__).parent.parent).as_posix(), - ) - def test_repo_task_prompts_do_not_contain_the_graded_string(self, path: Path): + One deliberate collision, documented rather than exempted: a literal (non-regex) + `command_executed.command_pattern` of >= CE061_MIN_LEAK_CHARS echoed verbatim in a + prompt IS flagged. That is correct — a pattern asserting *what ran* is graded + behaviour, not a locator. The `command` exemption covers `run_command.command`, the + command the CHECKER runs, which is a different field on a different criterion. No + in-repo task has the collision, so exempting `command_pattern` would be an unused + exemption weakening a real check. + """ - from coder_eval.orchestration.task_loader import expand_dataset, load_task + @classmethod + def _offenders(cls, task, task_file_dir: Path) -> list[str]: + """Every verbatim leak in the task's expanded rows. - task, _ = load_task(path) - if task.dataset is None: - pytest.skip("no dataset: block — nothing is row-substituted") + The rule's whole detection body lives here so the fixtures below and the repo scan + exercise the SAME code. Split, the repo scan would keep passing identically whether + or not the rule could still detect anything. + """ + from coder_eval.orchestration.task_loader import expand_dataset offenders: list[str] = [] - for row in expand_dataset(task, path.parent): + for row in expand_dataset(task, task_file_dir): prompt = (row.initial_prompt or "").lower() if not prompt: continue @@ -4354,18 +4603,26 @@ def test_repo_task_prompts_do_not_contain_the_graded_string(self, path: Path): # it is a label, routinely echoes the scenario, and grades nothing. dumped = criterion.model_dump() dumped.pop("description", None) - # Location fields are exempt, and the distinction is the whole rule: a - # prompt MAY say WHERE to write ("call it .github/workflows/evals.yml"), - # which removes the agent's filename choice from the measurement without - # revealing anything graded. It may not say WHAT the artifact must contain. - for locator in ("path", "agent_file", "file_path", "command"): + for locator in CE061_LOCATOR_FIELDS: dumped.pop(locator, None) for value in _string_leaves(dumped): - # Short values collide by chance ("ci", "0.7"); a leak worth flagging is - # a substantive string the author put in both places. - if len(value) >= 12 and value.lower() in prompt: + if len(value) >= CE061_MIN_LEAK_CHARS and value.lower() in prompt: offenders.append(f"{row.task_id}: prompt contains {value!r} ({criterion.type})") + return offenders + + @pytest.mark.parametrize( + "path", + sorted(p for p in (Path(__file__).parent.parent / "tasks").rglob("*.yaml") if p.name != "metadata.yaml"), + ids=lambda p: p.relative_to(Path(__file__).parent.parent).as_posix(), + ) + def test_repo_task_prompts_do_not_contain_the_graded_string(self, path: Path): + from coder_eval.orchestration.task_loader import load_task + + task, _ = load_task(path) + if task.dataset is None: + pytest.skip("no dataset: block — nothing is row-substituted") + offenders = self._offenders(task, path.parent) assert not offenders, ( f"{path}: the prompt hands the agent the exact string a criterion grades it on, so " f"the row scores well whether or not the behaviour under test happened — and an " @@ -4373,6 +4630,128 @@ def test_repo_task_prompts_do_not_contain_the_graded_string(self, path: Path): f"let the skill or the agent supply the method.\n\n" + "\n".join(f" {o}" for o in offenders) ) + def test_detects_a_leaking_row(self, tmp_path: Path): + from coder_eval.models import FileCheckCriterion + + task = _dataset_task( + [{"id": "a"}], + prompt="Write a workflow that sets minimum-task-score to 0.8", + criteria=[FileCheckCriterion(description="d", path="out.yml", includes=["minimum-task-score"])], + ) + offenders = self._offenders(task, tmp_path) + assert len(offenders) == 1 and "minimum-task-score" in offenders[0] + + def test_locator_fields_are_exempt(self, tmp_path: Path): + # Naming WHERE the artifact goes removes filename nondeterminism from the + # measurement; it reveals nothing about what the artifact must contain. + from coder_eval.models import FileCheckCriterion + + task = _dataset_task( + [{"id": "a"}], + prompt="Write it to .github/workflows/evals.yml", + criteria=[FileCheckCriterion(description="d", path=".github/workflows/evals.yml")], + ) + assert self._offenders(task, tmp_path) == [] + + def test_skill_name_is_exempt(self, tmp_path: Path): + # THE regression this exemption exists for. `skill_name` names WHICH skill must + # engage; the graded thing is the engagement EVENT, which no prompt can supply — + # and the outcome-suite pattern this plugin prescribes puts the skill name in every + # prompt by design. Without the exemption, the first repo-committed outcome suite + # for a skill whose name reaches the length floor fails CE061 on its own engagement + # criterion. + from coder_eval.models import SkillTriggeredCriterion + + assert len("optimize-skill") >= CE061_MIN_LEAK_CHARS, "fixture no longer exercises the floor" + task = _dataset_task( + [{"id": "a"}], + prompt="Use the optimize-skill skill to improve this description", + criteria=[SkillTriggeredCriterion(description="d", skill_name="optimize-skill", expected_skill="")], + ) + assert self._offenders(task, tmp_path) == [] + + @pytest.mark.parametrize(("delta", "flagged"), [(-1, False), (0, True)]) + def test_the_length_floor_is_inclusive(self, tmp_path: Path, delta: int, flagged: bool): + # Both sides of the `>=`, which is the part a refactor actually breaks. The strings + # are DERIVED from CE061_MIN_LEAK_CHARS rather than spelled out: a hardcoded 11 and + # 12 would be a second declaration of the same number, which is the drift this + # fixture exists to prevent. (The literal VALUE of the floor is not pinned here on + # purpose — it is a tuning knob; what must not move silently is the comparison.) + from coder_eval.models import FileCheckCriterion + + value = "x" * (CE061_MIN_LEAK_CHARS + delta) + task = _dataset_task( + [{"id": "a"}], + prompt=f"The answer is {value}", + criteria=[FileCheckCriterion(description="d", path="out.yml", includes=[value])], + ) + assert bool(self._offenders(task, tmp_path)) is flagged + + def test_description_is_not_scanned(self, tmp_path: Path): + # `description` is a label. It routinely echoes the scenario and grades nothing, so + # scanning it would flag every well-named criterion in the repo. + from coder_eval.models import FileCheckCriterion + + task = _dataset_task( + [{"id": "a"}], + prompt="emit the deployment manifest", + criteria=[FileCheckCriterion(description="emit the deployment manifest", path="out.yml")], + ) + assert self._offenders(task, tmp_path) == [] + + def test_nested_string_leaves_are_scanned(self, tmp_path: Path): + # Pins `_string_leaves`' recursion: a leak in the SECOND entry of a list must be + # caught, or a rule that only looked at scalar fields would pass this repo's suites + # while missing every `includes:` leak — the commonest shape there is. + from coder_eval.models import FileCheckCriterion + + task = _dataset_task( + [{"id": "a"}], + prompt="the file must mention permissions-boundary", + criteria=[ + FileCheckCriterion(description="d", path="out.yml", includes=["harmless", "permissions-boundary"]) + ], + ) + offenders = self._offenders(task, tmp_path) + assert len(offenders) == 1 and "permissions-boundary" in offenders[0] + + def test_ce061_exemption_list_matches_claude_md(self): + # CE061_LOCATOR_FIELDS is the single source; CLAUDE.md's CE061 sentence is derived. + # Both directions, because the list already drifted once: CLAUDE.md named three of + # the four fields the code exempted, and nothing noticed. The repo automates exactly + # this class elsewhere (CE028 for the docs indexes, CE033 for the plugin reference). + import re + + text = _normalized(Path(__file__).parent.parent / "CLAUDE.md") + sentence = next((s for s in text.split(". ") if "Location fields" in s), None) + assert sentence is not None, "CLAUDE.md no longer states CE061's exemption list" + backticked = set(re.findall(r"`([a-z_]+)`", sentence)) + assert set(CE061_LOCATOR_FIELDS) <= backticked, ( + f"CLAUDE.md's CE061 sentence omits {sorted(set(CE061_LOCATOR_FIELDS) - backticked)}" + ) + assert backticked <= set(CE061_LOCATOR_FIELDS), ( + f"CLAUDE.md's CE061 sentence names {sorted(backticked - set(CE061_LOCATOR_FIELDS))} as exempt, " + f"which the rule does not exempt" + ) + + +def _dataset_task(rows: list[dict], *, prompt: str = "${row.id}", criteria=None, split_field: str = "split"): + """A minimal dataset-backed task over inline rows, for the CE060/CE061 fixtures. + + One builder for both rules: CE060 needs varying rows, CE061 varying prompts AND + criteria, and a per-class copy would fork the moment either grew a parameter. + Inline ``rows`` deliberately — no JSONL file, so the fixtures never touch disk. + """ + from coder_eval.models import Dataset, FileExistsCriterion, TaskDefinition + + return TaskDefinition( + task_id="t", + description="dataset-rule fixture", + initial_prompt=prompt, + success_criteria=criteria or [FileExistsCriterion(description="d", path="out.txt")], + dataset=Dataset(rows=rows, split_field=split_field), + ) + def _string_leaves(node: object) -> list[str]: """Every string in a nested dict/list, flattened.""" diff --git a/tests/test_dataset_expansion.py b/tests/test_dataset_expansion.py index c2fc72b5..2bf7ae0f 100644 --- a/tests/test_dataset_expansion.py +++ b/tests/test_dataset_expansion.py @@ -18,7 +18,7 @@ ) from coder_eval.orchestration.config import BatchRunConfig from coder_eval.orchestration.experiment import resolve_all_tasks -from coder_eval.orchestration.task_loader import expand_dataset +from coder_eval.orchestration.task_loader import SplitSelectorError, expand_dataset def _base_task_dict() -> dict[str, Any]: @@ -329,6 +329,40 @@ def test_partially_labelled_excludes_unlabelled_rows(self, tmp_path: Path) -> No expanded = expand_dataset(_make_task_with_dataset(rows=rows), tmp_path, split="train") assert [t.row_id for t in expanded] == ["a"] + def test_partial_labelling_warns_with_the_drop_count(self, tmp_path: Path, caplog) -> None: + # Dropping the unlabelled rows is the safe direction, but it must not be SILENT: + # every metric below is then computed over a smaller suite than the file suggests. + rows = [ + {"id": "a", "prompt": "p", "expected": "e", "split": "train"}, + {"id": "b", "prompt": "p", "expected": "e"}, + ] + with caplog.at_level("WARNING", logger="coder_eval.orchestration.task_loader"): + expand_dataset(_make_task_with_dataset(rows=rows), tmp_path, split="train") + assert len(caplog.records) == 1 + assert "1 row(s) carry no" in caplog.text + + def test_fully_labelled_split_warns_nothing(self, tmp_path: Path, caplog) -> None: + # The first of the two negatives, and they matter more than the positive: a warning + # that fires when nothing was dropped trains the reader to ignore it. + rows = [ + {"id": "a", "prompt": "p", "expected": "e", "split": "train"}, + {"id": "b", "prompt": "p", "expected": "e", "split": "test"}, + ] + with caplog.at_level("WARNING", logger="coder_eval.orchestration.task_loader"): + expand_dataset(_make_task_with_dataset(rows=rows), tmp_path, split="train") + assert caplog.records == [] + + def test_partial_labelling_without_a_split_warns_nothing(self, tmp_path: Path, caplog) -> None: + # No selector, no drop — the state is only dangerous when --split acts on it. + rows = [ + {"id": "a", "prompt": "p", "expected": "e", "split": "train"}, + {"id": "b", "prompt": "p", "expected": "e"}, + ] + with caplog.at_level("WARNING", logger="coder_eval.orchestration.task_loader"): + expanded = expand_dataset(_make_task_with_dataset(rows=rows), tmp_path) + assert [t.row_id for t in expanded] == ["a", "b"] + assert caplog.records == [] + def test_labelled_but_unmatched_raises_listing_available_splits(self, tmp_path: Path) -> None: task = _make_task_with_dataset(rows=self._split_rows()) with pytest.raises(ValueError, match="no rows in split 'Train'") as exc: @@ -432,6 +466,47 @@ def test_duplicate_ids_across_splits_are_caught_under_a_filter(self, tmp_path: P with pytest.raises(ValueError, match="Duplicate dataset row id"): expand_dataset(task, tmp_path, split="train") + def test_malformed_row_id_in_an_unselected_split_still_raises(self, tmp_path: Path) -> None: + # Same argument as the duplicate-id test above, for the other two id checks: a + # malformed id is malformed data whichever split you asked for. Validated only over + # the SELECTED rows, it would pass under every `--split train` and surface at + # promotion time — the most expensive moment to learn it. + rows = [ + {"id": "ok", "prompt": "p", "expected": "e", "split": "train"}, + {"id": "bad id!", "prompt": "p", "expected": "e", "split": "test"}, + ] + with pytest.raises(ValueError, match="must match"): + expand_dataset(_make_task_with_dataset(rows=rows), tmp_path, split="train") + + def test_missing_id_field_in_an_unselected_split_still_raises(self, tmp_path: Path) -> None: + rows = [ + {"id": "ok", "prompt": "p", "expected": "e", "split": "train"}, + {"prompt": "p", "expected": "e", "split": "test"}, + ] + with pytest.raises(ValueError, match="missing id_field 'id'"): + expand_dataset(_make_task_with_dataset(rows=rows), tmp_path, split="train") + + def test_malformed_row_id_beyond_the_sample_still_raises(self, tmp_path: Path) -> None: + # The sampler narrows the row set too, and a malformed row the sample happened to + # drop is still a malformed row. Validation runs over the whole dataset first. + # + # `--sample` uses a FIXED seed, so which row it drops is deterministic — but the + # test must not silently stop exercising the case if that seed ever changes. So + # first establish, on a well-formed twin of the same shape, that row 0 is the one + # the sampler excludes; that is the premise the assertion below depends on. + shape_twin = [{"id": "first", "prompt": "p", "expected": "e"}, {"id": "second", "prompt": "p", "expected": "e"}] + sampled = expand_dataset(_make_task_with_dataset(rows=shape_twin), tmp_path, max_rows=1) + assert [t.row_id for t in sampled] == ["second"], ( + "the fixed sample seed no longer drops row 0; re-pick the malformed row's position" + ) + + rows = [ + {"id": "bad id!", "prompt": "p", "expected": "e"}, + {"id": "ok", "prompt": "p", "expected": "e"}, + ] + with pytest.raises(ValueError, match="must match"): + expand_dataset(_make_task_with_dataset(rows=rows), tmp_path, max_rows=1) + def test_split_field_defaults_to_split(self) -> None: task = _make_task_with_dataset(rows=[{"id": "a"}]) assert task.dataset is not None @@ -739,26 +814,73 @@ def test_split_leaves_an_unlabelled_sibling_suite_whole(self, tmp_path: Path) -> "plain/row-2", ] - def test_unmatched_split_demotes_the_suite_to_a_skipped_task(self, tmp_path: Path) -> None: - # A labelled suite with no row in the requested split raises out of - # expand_dataset, but resolve_all_tasks catches ValueError and records a - # SkippedTask rather than aborting — so the run reports it in run.json's - # skipped_tasks instead of failing. Pinned because the reason string is the - # ONLY place a mistyped --split selector surfaces: nothing else distinguishes - # it from an empty run. + def test_unmatched_split_aborts_instead_of_demoting_to_a_skipped_task(self, tmp_path: Path) -> None: + # A labelled suite with no row in the requested split ABORTS the run. It used to + # demote to a SkippedTask like any load failure, which produced the worst outcome + # in the whole split workflow: one yellow line, zero evals run, exit 0 — a CI gate + # reporting success for a one-character typo. The other dataset errors describe a + # malformed FILE and stay demoted (one bad task must not abort a suite); this one + # describes a malformed INVOCATION, and the same selector applies to every task in + # the run, so there is no per-task isolation argument for it. + # + # The message is still the only place a user learns what they should have typed, + # so both halves stay pinned. task_file = self._write_split_suite(tmp_path, "labelled", ["train", "test"]) default_exp, experiment = self._make_experiment(["v1"]) + with pytest.raises(SplitSelectorError) as exc: + resolve_all_tasks( + task_files=[task_file], + experiment=experiment, + default_experiment=default_exp, + config=BatchRunConfig(run_dir=tmp_path / "runs", split="holdou"), + ) + assert "no rows in split 'holdou'" in str(exc.value) + assert "'test', 'train'" in str(exc.value) + + def test_split_selector_error_is_a_value_error(self) -> None: + # Every existing `except ValueError` caller depends on this, including the CLI + # seam in run_command.py that turns it into a typer.BadParameter. + assert issubclass(SplitSelectorError, ValueError) + + def test_unlabelled_task_in_a_multi_task_run_is_unaffected(self, tmp_path: Path) -> None: + # Guards the `if labelled:` placement: the abort must fire only for a task that + # CARRIES split labels. A suite with none passes through unfiltered even while a + # sibling suite is being filtered by the same selector. + labelled = self._write_split_suite(tmp_path, "labelled", ["train", "test"]) + plain = self._write_split_suite(tmp_path, "plain", [None, None]) + default_exp, experiment = self._make_experiment(["v1"]) + resolved, skipped = resolve_all_tasks( - task_files=[task_file], + task_files=[labelled, plain], experiment=experiment, default_experiment=default_exp, - config=BatchRunConfig(run_dir=tmp_path / "runs", split="holdou"), + config=BatchRunConfig(run_dir=tmp_path / "runs", split="train"), ) - assert resolved == [] - assert len(skipped) == 1 - assert "no rows in split 'holdou'" in skipped[0].reason - assert "'test', 'train'" in skipped[0].reason + assert skipped == [] + assert sorted(rt.task.task_id for rt in resolved) == ["labelled/row-0", "plain/row-0", "plain/row-1"] + + def test_unmatched_split_surfaces_as_a_cli_bad_parameter(self, tmp_path: Path) -> None: + # The CLI seam, at the cheaper of the two levels: `run_command.py` wraps its + # resolve_all_tasks call in `except ValueError -> typer.BadParameter`, and a full + # `coder-eval run` invocation would need credentials and a sandbox. typer.BadParameter + # exits **2**, not 1 — do not "fix" a later assertion to 1. + import typer + + task_file = self._write_split_suite(tmp_path, "labelled", ["train", "test"]) + default_exp, experiment = self._make_experiment(["v1"]) + + with pytest.raises(typer.BadParameter) as exc: + try: + resolve_all_tasks( + task_files=[task_file], + experiment=experiment, + default_experiment=default_exp, + config=BatchRunConfig(run_dir=tmp_path / "runs", split="holdou"), + ) + except ValueError as e: # exactly what cli/run_command.py does + raise typer.BadParameter(str(e)) from e + assert "'test', 'train'" in str(exc.value) def test_non_dataset_task_unaffected(self, tmp_path: Path) -> None: task_file = self._write_task_yaml(tmp_path, "plain", with_dataset=False) diff --git a/tests/test_early_stop.py b/tests/test_early_stop.py index 5f647392..e1a7d421 100644 --- a/tests/test_early_stop.py +++ b/tests/test_early_stop.py @@ -2160,31 +2160,41 @@ def test_decision_latched_after_fire(self) -> None: assert watcher.info is not None assert watcher.info.reason == EarlyStopReason.CRITERION_PASSED - def test_tool_call_fires_before_result(self) -> None: - # The decision latches on the tool CALL (ToolStartEvent): a Skill call - # whose result never arrives (a cut-short turn would strip it) still stops. - # No ToolEndEvent is ever fed. + def test_tool_call_does_not_fire_before_the_result(self) -> None: + # The ToolStart evaluation seam STAYS — but which criteria it can decide is + # now PER-CRITERION, not global. `skill_triggered`'s verdict is not decidable + # from the call: for the Skill tool the body IS the tool result, so an + # in-flight call (result_status=None) has delivered nothing and must not + # pass-stop. It decides one round later, at its ToolEnd (below). The seam + # still serves criteria whose verdict IS decidable from the call, such as + # `command_executed`. watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_on_pass=True)]) _feed(watcher, [_agent_start(), _turn_start(), _skill_start("date-teller")]) + assert watcher.should_stop() is False + assert watcher.info is None + # The matching resolved ToolEnd is what decides it. + _feed(watcher, [_tool_end(_skill_cmd("date-teller", tool_id="sk-1"))]) assert watcher.should_stop() is True assert watcher.info is not None assert watcher.info.reason == EarlyStopReason.CRITERION_PASSED - # The in-flight call reports as the 1st tool call even without a ToolEnd. - assert watcher.info.tool_call_index == 1 - def test_tool_call_distractor_fail_fires(self) -> None: - # A distractor (armed fail) fail-stops on the tool CALL that engages its - # skill, before any result arrives. + def test_tool_call_distractor_fail_does_not_fire_before_the_result(self) -> None: + # Same narrowing on the fail side: a distractor's engagement is only real + # once the body was actually delivered, so no fail-stop until the ToolEnd. watcher = _watcher([_skill_crit("weather-teller", "date-teller", stop_on_fail=True)]) _feed(watcher, [_agent_start(), _turn_start(), _skill_start("weather-teller")]) + assert watcher.info is None + _feed(watcher, [_tool_end(_skill_cmd("weather-teller", tool_id="sk-1"))]) assert watcher.info is not None assert watcher.info.reason == EarlyStopReason.CRITERION_FAILED - def test_tool_call_latches_on_file_read_engagement(self) -> None: + def test_tool_call_does_not_latch_on_an_unresolved_file_read(self) -> None: # Off-Claude agents (antigravity/codex) engage a skill by READING its files - # (skills//...), not via a Skill tool call. The watcher must latch on - # that Read ToolStart — the file-path parameter carries the signal on the - # call itself, so early-stop fires off-Claude just as it does for Claude. + # (skills//...), not via a Skill tool call. `Read` is one of the three + # tools whose FAILURE means nothing was loaded, so an unresolved Read + # ToolStart — the path is in `parameters`, but the file may yet ENOENT — is + # not engagement. Antigravity is the agent this costs a round: its tool map + # renames view_file/search_directory/find_file to Read/Grep/Glob. watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_on_pass=True)]) read = CommandTelemetry( tool_name="Read", @@ -2193,30 +2203,74 @@ def test_tool_call_latches_on_file_read_engagement(self) -> None: parameters={"file_path": "/repo/skills/date-teller/SKILL.md"}, ) _feed(watcher, [_agent_start(), _turn_start(), ToolStartEvent(task_id="t", tool=read)]) + assert watcher.should_stop() is False + assert watcher.info is None + # Resolved successfully, it decides. + read.result_status = "success" + _feed(watcher, [ToolEndEvent(task_id="t", tool=read)]) assert watcher.should_stop() is True assert watcher.info is not None assert watcher.info.reason == EarlyStopReason.CRITERION_PASSED - def test_tool_call_latches_before_unresolved_end(self) -> None: - # The call fires the stop in-loop; a later finalize() UNRESOLVED end for - # the SAME call is short-circuited (decision already latched) — no relabel, - # no double count. + def test_in_flight_then_errored_skill_call_never_stops_the_run(self) -> None: + # The refusal path end to end on the watcher, over ONE telemetry object + # mutated in place exactly as the agent mutates it on resolution. A skill + # carrying `disable-model-invocation: true` is refused: the call is + # dispatched (result_status=None), then resolves to "error". Neither seam + # may cut the run — the body never loaded, so the frozen check will score + # this row `no`, and a stop here would credit a skill that never ran. watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_on_pass=True)]) - _feed(watcher, [_agent_start(), _turn_start(), _skill_start("date-teller", tool_id="sk-1")]) - fired = watcher.info - _feed(watcher, [_unresolved_skill_end("date-teller", tool_id="sk-1")]) - assert watcher.info is fired + start = _skill_start("date-teller", tool_id="sk-1") + _feed(watcher, [_agent_start(), _turn_start(), start]) + assert watcher.should_stop() is False + start.tool.result_status = "error" + _feed(watcher, [ToolEndEvent(task_id="t", tool=start.tool, status=ToolEndStatus.ERROR)]) + assert watcher.should_stop() is False + assert watcher.info is None + + def test_tool_call_latches_on_an_unresolved_bash_file_read(self) -> None: + # The Bash twin of the test above, and the path that still latches on the + # CALL: Codex reads a SKILL.md through the shell, and `cat … | grep foo` + # exits non-zero AFTER genuinely reading the file — so a status gate on Bash + # would drop real engagement. Bash is therefore ungated, and its ToolStart + # still decides. Which tool name carries the signal is agent-specific: + # Bash for Codex, Read/Grep/Glob for antigravity, the Skill tool for Claude. + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_on_pass=True)]) + bash = CommandTelemetry( + tool_name="Bash", + tool_id="b1", + timestamp=_TS, + parameters={"command": "cat /repo/skills/date-teller/SKILL.md"}, + ) + _feed(watcher, [_agent_start(), _turn_start(), ToolStartEvent(task_id="t", tool=bash)]) + assert watcher.should_stop() is True assert watcher.info is not None + assert watcher.info.reason == EarlyStopReason.CRITERION_PASSED assert watcher.info.tool_call_index == 1 + def test_unresolved_end_after_an_in_flight_skill_call_fires_nothing(self) -> None: + # The inverse of the old "the call latches, the UNRESOLVED end is a no-op": + # nothing latches on the call now, and the finalize() UNRESOLVED end must not + # fire one either — an "unknown" Skill call was force-closed by a crash + # before any body arrived, so it is not engagement on either seam. This is + # what keeps the live verdict and the frozen check agreeing: both score `no`. + watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_on_pass=True)]) + _feed(watcher, [_agent_start(), _turn_start(), _skill_start("date-teller", tool_id="sk-1")]) + assert watcher.info is None + _feed(watcher, [_unresolved_skill_end("date-teller", tool_id="sk-1")]) + assert watcher.info is None + assert watcher.should_stop() is False + def test_tool_call_index_counts_prior_resolved_calls(self) -> None: # A prior resolved, non-deciding tool is counted at its ToolEnd; the - # deciding in-flight call is then reported as the next (2nd) call. + # deciding call is then reported as the next (2nd) call. Pointed at a + # RESOLVED Skill ToolEnd so it keeps testing the INDEX rather than the + # in-flight deferral (which its own test above owns). watcher = _watcher([_skill_crit("date-teller", "date-teller", stop_on_pass=True)]) prior = _cmd("Bash", {"command": "ls"}) # not a skill engagement _feed(watcher, [_agent_start(), _turn_start(), _tool_end(prior)]) assert watcher.info is None - _feed(watcher, [_skill_start("date-teller", tool_id="sk-1", sequence_number=1)]) + _feed(watcher, [_tool_end(_skill_cmd("date-teller", tool_id="sk-1"))]) assert watcher.info is not None assert watcher.info.tool_call_index == 2 @@ -2232,8 +2286,9 @@ def test_second_agent_start_does_not_reset_origin(self) -> None: # A second AgentStart (as on a retry) must leave the origin untouched. _feed(watcher, [_agent_start(), _turn_start()]) assert watcher._started_monotonic == origin - # The stop that follows anchors elapsed_seconds to that first origin. - _feed(watcher, [_skill_start("date-teller")]) + # The stop that follows anchors elapsed_seconds to that first origin. A + # RESOLVED Skill ToolEnd, since an in-flight call no longer decides. + _feed(watcher, [_tool_end(_skill_cmd("date-teller", tool_id="sk-1"))]) assert watcher.info is not None assert watcher.info.elapsed_seconds >= 0.0 @@ -2611,9 +2666,14 @@ async def test_completed_run_with_orphan_tool_not_early_stopped(self, tmp_path) assert result.all_criteria_passed(self._criteria()) is False async def test_tool_call_cut_without_tool_end(self, tmp_path) -> None: - # End-to-end: the deciding Skill CALL (a ToolStart with no ToolEnd) cuts - # the stream and records an early stop — the case that would otherwise run - # to the turn cap when a cut-short turn strips the result. + # THE TEST THAT STATES THE TRADE. A Skill CALL whose result never arrives + # (a cut-short turn strips it) no longer stops the run, so this run + # continues to its turn cap. That is the price, and it is the correct one: + # the old behaviour stopped the run AND the frozen check scored `yes` — a + # run credited to a skill whose body never reached the agent. Both rules + # are internally consistent; they disagree on whether "dispatched and never + # returned" is engagement. It is not. Do NOT "restore" the old behaviour to + # buy the stop back: the stop and the score would diverge again. events = [_agent_start(), _turn_start(), _skill_start(self._SKILL), _turn_start()] result, agent, _success = await _run_wiring( criteria=self._criteria(), @@ -2621,10 +2681,30 @@ async def test_tool_call_cut_without_tool_end(self, tmp_path) -> None: scores=[1.0, 0.0], tmp_path=tmp_path, ) + assert result.early_stop is None + assert agent.delivered == 4 # nothing cut: the whole stream was consumed + + async def test_tool_call_with_a_resolved_end_still_cuts(self, tmp_path) -> None: + # The twin of the test above, and what makes it "one round later, not + # never": the SAME stream with a successful ToolEnd for that call still + # cuts and still reports CRITERION_PASSED. + events = [ + _agent_start(), + _turn_start(), + _skill_start(self._SKILL), + _tool_end(_skill_cmd(self._SKILL, tool_id="sk-1")), + _turn_start(), + ] + result, agent, _success = await _run_wiring( + criteria=self._criteria(), + events=events, + scores=[1.0, 0.0], + tmp_path=tmp_path, + ) assert result.early_stop is not None assert result.early_stop.reason == EarlyStopReason.CRITERION_PASSED - # Cut at the ToolStart: the trailing turn_start is never delivered. - assert agent.delivered == 3 + # Cut at the ToolEnd: the trailing turn_start is never delivered. + assert agent.delivered == 4 async def test_fail_open_wiring_degrades_to_full_run(self, tmp_path) -> None: with patch.object(SkillTriggeredChecker, "live_verdict", side_effect=RuntimeError("boom")): diff --git a/tests/test_plan_command.py b/tests/test_plan_command.py index 16a8d663..4b49da9f 100644 --- a/tests/test_plan_command.py +++ b/tests/test_plan_command.py @@ -311,3 +311,121 @@ def test_plan_exits_on_explicit_experiment_load_failure(self, tmp_path: Path) -> printed = " ".join(str(call) for call in mock_console.print.call_args_list) assert "Bad experiment" in printed + + +def _make_dataset_task( + rows: list[dict], + *, + prompt: str = "Do ${row.id}", + split_field: str = "split", +) -> TaskDefinition: + """A dataset-backed task over INLINE rows, so the fixtures never touch disk.""" + from coder_eval.models import Dataset + + return TaskDefinition( + task_id="dataset-task", + description="A dataset task", + initial_prompt=prompt, + sandbox={"driver": "tempdir"}, + success_criteria=[{"type": "file_exists", "description": "check", "path": "out.txt"}], + dataset=Dataset(rows=rows, split_field=split_field), + ) + + +class TestPlanCommandDatasetPreview: + """`plan` expands datasets, so the pre-spend surface reports what a run will do. + + Without this, the only way to learn a suite's resolved row count — the number every + cost estimate and every A/B comparison depends on — was to pay for a run. + """ + + def _run(self, task, task_file: Path, **kwargs): + experiment = _make_experiment(variants=[ExperimentVariant(variant_id="default")]) + resolved = _make_task(agent=parse_agent_config(type=AgentKind.CLAUDE_CODE)) + 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, {}, 1)), + patch("coder_eval.cli.plan_command.console") as mock_console, + ): + exit_code = 0 + try: + plan_command(task_files=[task_file], **kwargs) + except typer.Exit as e: + exit_code = e.exit_code + return " ".join(str(call) for call in mock_console.print.call_args_list), exit_code + + def test_plan_prints_dataset_row_counts(self, tmp_path: Path) -> None: + task_file = tmp_path / "task.yaml" + task_file.write_text("placeholder") + task = _make_dataset_task([{"id": f"r{i}"} for i in range(4)]) + printed, exit_code = self._run(task, task_file) + assert exit_code == 0 + assert "4 rows" in printed and "4 selected" in printed + + def test_plan_split_filters_the_previewed_rows(self, tmp_path: Path) -> None: + task_file = tmp_path / "task.yaml" + task_file.write_text("placeholder") + task = _make_dataset_task( + [ + {"id": "a", "split": "train"}, + {"id": "b", "split": "train"}, + {"id": "c", "split": "test"}, + {"id": "d", "split": "test"}, + ] + ) + printed, exit_code = self._run(task, task_file, split="test") + assert exit_code == 0 + assert "4 rows" in printed and "2 selected" in printed and "--split test" in printed + + def test_plan_reports_an_unmatched_split_as_an_error_and_exits_1(self, tmp_path: Path) -> None: + # The pre-spend surface this phase exists to provide: learn the selector is wrong + # for free, rather than by watching a run abort. + task_file = tmp_path / "task.yaml" + task_file.write_text("placeholder") + task = _make_dataset_task([{"id": "a", "split": "train"}, {"id": "b", "split": "test"}]) + printed, exit_code = self._run(task, task_file, split="typo") + assert exit_code == 1 + assert "'test', 'train'" in printed + + def test_plan_leaves_a_non_dataset_task_unannotated(self, tmp_path: Path) -> None: + # A "1 row" line on a task with no dataset: block would be noise, and would imply + # a concept the task does not have. + task_file = tmp_path / "task.yaml" + task_file.write_text("placeholder") + printed, exit_code = self._run(_make_task(), task_file) + assert exit_code == 0 + assert "Dataset:" not in printed + + def test_plan_leaves_an_unlabelled_dataset_unfiltered_under_split(self, tmp_path: Path) -> None: + # --split is global to the invocation, so an unlabelled task in a multi-task run + # must pass through whole — and the line must not imply the selector did anything. + task_file = tmp_path / "task.yaml" + task_file.write_text("placeholder") + task = _make_dataset_task([{"id": "a"}, {"id": "b"}, {"id": "c"}]) + printed, exit_code = self._run(task, task_file, split="test") + assert exit_code == 0 + assert "3 rows" in printed and "3 selected" in printed + assert "not labelled, all rows kept" in printed + + def test_plan_catches_a_bad_row_substitution(self, tmp_path: Path) -> None: + # The value the expansion buys beyond a row count: a ${row.*} naming a field no row + # carries used to fail at run time, per row, after the sandbox was built. + task_file = tmp_path / "task.yaml" + task_file.write_text("placeholder") + task = _make_dataset_task([{"id": "a"}], prompt="Do ${row.missing}") + printed, exit_code = self._run(task, task_file) + assert exit_code == 1 + assert "missing" in printed + + def test_plan_warns_on_a_partly_labelled_dataset(self, tmp_path: Path) -> None: + # A warning, not an error: the run is legitimate, it is just measuring less than + # the file suggests. Exit stays 0. + task_file = tmp_path / "task.yaml" + task_file.write_text("placeholder") + task = _make_dataset_task([{"id": "a", "split": "train"}, {"id": "b", "split": "train"}, {"id": "c"}]) + printed, exit_code = self._run(task, task_file, split="train") + assert exit_code == 0 + assert "⚠" in printed and "1 of 3 rows carry no 'split' label" in printed diff --git a/tests/test_skill_triggered.py b/tests/test_skill_triggered.py index 081aa611..4ccdc802 100644 --- a/tests/test_skill_triggered.py +++ b/tests/test_skill_triggered.py @@ -21,7 +21,7 @@ def _cmd( tool_name: str, parameters: dict[str, Any] | None = None, tool_id: str = "t1", - result_status: str = "success", + result_status: str | None = "success", result_summary: str | None = None, ) -> CommandTelemetry: return CommandTelemetry( @@ -106,6 +106,85 @@ def test_errored_skill_call_still_counts_when_the_file_was_read(self) -> None: ) assert result.observed_label == "yes" and result.score == 1.0 + def test_pending_skill_call_is_not_engagement(self) -> None: + # `result_status=None` is an IN-FLIGHT call: the early-stop watcher evaluates on + # ToolStartEvent, before any result exists. For the Skill tool the body IS the tool + # result, so a call with no result yet has delivered nothing. Counting it made the + # live verdict pass on a call the frozen check would later score `no`. + result = _check( + expected_skill="uipath-flow", + skill_name="uipath-flow", + commands=[_cmd("Skill", {"skill": "uipath-flow"}, result_status=None)], + ) + assert result.observed_label == "no" and result.score == 0.0 + + def test_unknown_skill_call_is_not_engagement(self) -> None: + # A turn that crashes force-closes its open tool calls to "unknown". A Skill call + # that never returned a result never delivered a body — including the refusal case + # where the crash beat the error result to the recorder. + result = _check( + expected_skill="uipath-flow", + skill_name="uipath-flow", + commands=[_cmd("Skill", {"skill": "uipath-flow"}, result_status="unknown")], + ) + assert result.observed_label == "no" and result.score == 0.0 + + def test_excluded_skill_call_is_not_resurrected_by_its_own_parameters(self) -> None: + # The Skill branch is AUTHORITATIVE for a Skill call. Without that, a call the + # status gate excluded would fall through to the generic path scan and be counted + # again the moment any of its own parameters happened to contain a + # `skills//`-shaped substring — reintroducing exactly the false `yes` the + # gate exists to remove, and breaking monotonicity (absent -> present on a call + # that delivered nothing). + result = _check( + expected_skill="uipath-flow", + skill_name="uipath-flow", + commands=[_cmd("Skill", {"skill": "x", "hint": "see skills/uipath-flow/SKILL.md"}, result_status="error")], + ) + assert result.observed_label == "no" and result.score == 0.0 + + def test_failed_read_of_a_skill_path_is_not_engagement(self) -> None: + # A failed Read puts the path in `parameters` while loading nothing. The path + # reference alone is not evidence the body reached the agent. + result = _check( + expected_skill="my-skill", + skill_name="my-skill", + commands=[_cmd("Read", {"file_path": "/x/.agents/skills/my-skill/SKILL.md"}, result_status="error")], + ) + assert result.observed_label == "no" and result.score == 0.0 + + def test_pending_read_of_a_skill_path_is_not_engagement(self) -> None: + # Same reason as the pending Skill call: counting the in-flight Read would + # reintroduce the live/frozen divergence one branch over. + result = _check( + expected_skill="my-skill", + skill_name="my-skill", + commands=[_cmd("Read", {"file_path": "/x/.agents/skills/my-skill/SKILL.md"}, result_status=None)], + ) + assert result.observed_label == "no" and result.score == 0.0 + + def test_failed_bash_read_still_counts(self) -> None: + # `Bash` is deliberately NOT gated: `cat SKILL.md | grep foo` exits non-zero AFTER + # genuinely reading the file. A blanket status gate would drop real off-Claude + # engagement, which is the whole file-read signal. + result = _check( + expected_skill="my-skill", + skill_name="my-skill", + commands=[_cmd("Bash", {"command": "cat skills/my-skill/SKILL.md | grep foo"}, result_status="error")], + ) + assert result.observed_label == "yes" and result.score == 1.0 + + def test_unknown_read_still_counts(self) -> None: + # Codex reconstructs genuinely-executed calls from the rollout with status + # "unknown"; for a READ that is a real read, so it keeps counting. Only "error" + # and the in-flight None are evidence that nothing loaded. + result = _check( + expected_skill="my-skill", + skill_name="my-skill", + commands=[_cmd("Read", {"file_path": "skills/my-skill/SKILL.md"}, result_status="unknown")], + ) + assert result.observed_label == "yes" and result.score == 1.0 + def test_skill_invoked_fp(self) -> None: result = _check(expected_skill="", skill_name="uipath-flow", commands=[_cmd("Skill", {"skill": "uipath-flow"})]) assert result.score == 0.0 and result.observed_label == "yes" @@ -301,6 +380,26 @@ class TestSkillTriggeredGoldenCorpus: "yes", 1.0, ), + # Appended (not edited) when engagement narrowed to RESOLVED, SUCCESSFUL + # signals. Historical activation P/R/F1 computed before that change is not + # directly comparable if any run contained these two shapes — which is what + # these entries exist to make explicit. + ( + "errored-skill-call-with-no-other-signal", + "uipath-admin", + "uipath-admin", + [_cmd("Skill", {"skill": "uipath-admin"}, tool_id="s1", result_status="error")], + "no", + 0.0, + ), + ( + "failed-read-of-skill-path", + "uipath-admin", + "uipath-admin", + [_cmd("Read", {"file_path": "skills/uipath-admin/SKILL.md"}, result_status="error")], + "no", + 0.0, + ), ], ) def test_golden_corpus_scores_are_pinned(