Skip to content

feat(plugin): optimize-skill's execution track, measured end to end — plus the skill_triggered and --split fixes it surfaced - #109

Open
uipreliga wants to merge 45 commits into
mainfrom
feat/plugin-optimize-skill
Open

feat(plugin): optimize-skill's execution track, measured end to end — plus the skill_triggered and --split fixes it surfaced#109
uipreliga wants to merge 45 commits into
mainfrom
feat/plugin-optimize-skill

Conversation

@uipreliga

@uipreliga uipreliga commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

What

Adds Dataset.split_field + a --split row filter, ships a seventh plugin skill /coder-eval:optimize-skill, and dogfoods the whole loop in a new tutorial — which is where most of the value in this PR came from.

Implements c/2026-08-12-skill-optimize-p0-p1.md, phase by phase.

The feature

--split (P0). Label dataset rows train / test and select one at run time. The filter runs before either sampler — sampling first would leave an unpredictable (possibly zero) number of rows per split, destroying the comparison the split exists to protect. Unlabelled tasks pass through untouched, so --split is safe in a multi-task run; a labelled task with no matching row raises.

/coder-eval:optimize-skill (P1). A/B tests candidate edits to a skill as experiment variants, promoting only what beats run-to-run noise and then survives a held-out split. Two tracks:

  • Activation — the frontmatter description, measured against an activation suite. Does it fire when it should?
  • Execution — the skill body, measured against an outcome suite with real success criteria. Having fired, does it do the job?

Explicit-invocation only — it spends real money.

Why the two gates differ (worth a reviewer's eye)

The tracks share splits, snapshots, reachability, replicate discipline and the ledger. What differs is the instrument, and it is load-bearing:

  • Activation compares F1, which a pooled suite.json cannot report per replicate — hence three separate invocations.
  • Execution compares per-row weighted_score, which paired_comparison already computes correctly over replicates it averages per row before pairing — hence --repeats 3 on exactly two variants.

So the paired block that is mere corroboration on the activation track is the primary instrument on the execution track: tested code instead of arithmetic by hand. Unifying the gates would swap in an instrument that cannot see the metric, so both halves are pinned by sensors.

Two smaller inversions, also sensored: activation rows must never name the skill (that tests obedience, not activation) while execution rows must invoke it by slash command/plugin:skill at the top of initial_prompt — to hold activation constant; and disable-model-invocation: true now routes to the execution track rather than hard-stopping, so init and ci are optimizable after all.

The slash form is not a stylistic preference. A disable-model-invocation skill is not offered to the model at all, so asking in prose returns "no such skill is available" and the row measures nothing; the slash form loads it, emits a real Skill tool call, and is detected by skill_triggered. Both halves verified live (see below).

Validation

Beyond the test suite, the skill itself was live-tested in three layers, because most of its failure modes are silent rather than loud:

  1. Static — every ${CLAUDE_PLUGIN_ROOT} reference, slash command and CLI flag it emits resolves; the experiment YAML in its body validates against ExperimentDefinition.
  2. Cold agents — two agents with no context executed the skill end to end, one per track, on a zero-run budget, building real snapshots and experiment YAML for inspection.
  3. Live probe — one real agent run to settle whether skill_triggered detects a slash-command invocation (it does; scored 1.0).

That surfaced 15 problems, fixed in 9ac72f6. The two worth a reviewer's attention were silent confounds in the snapshot step: the diagram showed only skills/ while the prose said "everything that source contained" (a diagram-faithful snapshot mounts skills whose bundled reference/ files are missing — invisible on the activation track), and omitting .claude-plugin/plugin.json makes the namespace default to the arm's directory name, so arms would differ in the listing name as well as the text under test. Neither would have errored; both would have produced confident, wrong numbers.

It also produced one piece of guidance now in the skill: before proposing an A/B, check whether a lint rule already answers the question for zero runs. The complaint that prompted the ci test — "emits workflows that miss a step" — overlaps CE026 almost exactly, and ~300 agent runs is the wrong way to learn something a static rule catches permanently.

Three bugs caught before shipping

Two were errors in the plan, found by review; the third was already shipped on main.

  1. The sibling-regression gate read the wrong metric. Annexation makes the sibling's criterion expected=yes, observed=no — a false negative. Since precision = tp/(tp+fp), it stays pinned at 1.0 however many requests are stolen. The gate would have been gating on a constant. Now reads recall.yes.

  2. A one-skill candidate snapshot empties the sandbox. A variant's plugins block replaces the task's, so the snapshot is the arm's only skill source. Snapshot one skill and every sibling criterion silently observes no — the sibling gate "passes" by measuring nothing. Snapshots now carry the whole skills tree.

  3. The shipped reachability guidance was wrong (pre-existing). A local plugin path must be a plugin root holding skills/; the template told users to point at the bare skills directory, which loads nothing. Proven by probing three layouts against the real CLI. Every suite check-skill generated would have reported recall 0.0 — exactly what the template's own comment calls "reads exactly like a broken skill". Corrected in 7 surfaces, including ci, which was writing the broken path into users' CI workflows.

The tutorial (docs/tutorials/08)

Runs the loop for real against this repo's own skills, on Sonnet, and reports what happened rather than a tidied version:

  • lint-tasks measured at ceiling (F1 1.000 both splits), so the loop declined to spend ~224 runs chasing an unreachable number. That also closed the plan's open residual: the earlier 66-character description trim is now measured safe.
  • A task misfire that reproduced on both splits turned out to be 2-in-3 variance under replicates. Two agreeing runs were not evidence.
  • The real headroom was a sibling: analyze missed "what regressed" deterministically. Full three-stage A/B → a-regression promoted, gated on the train split (1.000 vs 0.667, non-overlapping, three invocations) and confirmed on the test split (1.000 vs 0.909).
  • Two comparisons were invalidated mid-run and thrown out — one candidate ranked on an eroded denominator, and a confident p = 0.038 that was a billing limit eating one arm harder than the other. completion_rate is what caught both.
  • Bare-name collision hazard: skill_triggered strips plugin: prefixes, and Claude Code ships its own unscoped init. A skill_name: "init" criterion would silently score a different skill. Documented in check-skill.

Notes for review

  • Listing budget held flat at 1,574/1,600 with a seventh skill added — five descriptions trimmed rather than raising the ceiling, since the budget is shared with every skill the user has installed.
  • analyze's description change is the one behavioural edit to an existing skill, and it is measured.
  • 3 new lint sensors (mutation-tested); 1 harness gap deferred to .claude/harness-candidates.md — an all-skipped run exits 0, now reachable by a one-character --split typo. Fixing it changes exit semantics for every skipped-task path, so it wants a decision rather than a drive-by.

make verify green: 4071 passed, coverage 91.57%.

🤖 Generated with Claude Code

Late changes (after the first review pass)

Two follow-ups from using the thing, both pushed since the PR opened:

Split values renamed tune/holdouttrain/test. Docs and content only — split values are open strings, so no schema change and nothing to migrate; split_field still defaults to "split". 215 replacements, followed by a pass disambiguating bare "test" back to "test split" wherever it named the data half rather than a check, since in a testing framework "without a test" parses exactly wrong. (Pedantic but recorded: nothing is trained here — you hand-select a candidate, which is a form of fitting, so "train" is defensible in the ML sense and far more recognisable than "tune".)

The skill now labels the splits itself rather than asking the user to. It previously offered to add labels, leaving a mechanical JSONL edit and a balance judgement to the reader. It now writes them and shows the counts for objection, under two rules that are easy to get wrong by eye and expensive when you do: stratify within each polarity so both halves carry positives and distractors, and assign deterministically so the split never re-rolls between rounds — a reshuffled split is not a test split, because rows already tuned against leak into it.

Not done, deliberately: a hash-based auto-split (--split train --split-ratio 0.6, membership derived from row_id, no labels in the file) would remove the last of the friction, but it conflicts with a load-bearing behaviour — unlabelled tasks currently pass through unfiltered, which is what makes --split safe in a multi-task run. Auto-splitting them would silently change that, so it needs an opt-in flag and its own tests. Left as a follow-up rather than smuggled in here.


Late changes (second pass): the execution track measured for real — and a criterion bug it uncovered

The execution track had never been run end to end. Doing that turned up a bug in skill_triggered that invalidates two claims made earlier in this description, so those are corrected below rather than quietly edited.

⚠️ Behaviour change reviewers should look at

skill_triggered no longer counts an errored Skill call as engagement (src/coder_eval/criteria/skill_triggered.py).

Any suite where a Skill call failed previously scored yes and will now score no. That is the correction — a refused call means the skill's body never loaded — but an existing green activation suite that was passing on refused calls will go red, and correctly so. The file-read signal is deliberately not gated the same way: a refused call loaded nothing, whereas a path reference means the SKILL.md was actually opened. Both cases are pinned by tests.

What the run found

An outcome suite was built to A/B ci's body: 4 arms differing only in that body, 24 rows. All four tied exactly on every criterion — which is a bug report, not a result.

ci sets disable-model-invocation: true, and the Skill tool refuses such a call outright:

<tool_use_error>Skill coder-eval:ci cannot be used with Skill tool
due to disable-model-invocation</tool_use_error>

24 of 24 calls failed this way, result_status: "error" on every one, and no row read the SKILL.md off disk either. The body never entered context; the agent answered from its own knowledge of GitHub Actions, plausibly enough that nothing downstream looked wrong. The tell was in the output the whole time:

body NOT loaded:  uses: anthropics/coder-eval-action@v1   <- does not exist
body loaded:    - uses: UiPath/coder_eval@v0             <- what the body specifies

Re-scoring those same artifacts with the fixed criterion gives 0/6 engagement per arm where the run reported 6/6.

Corrections to claims made above

  • "asking in prose returns no such skill is available"wrong. Measured across the same six rows, by how often the model attempted the call: slash form alone 3/6, prose instruction alone 5/6, slash + explicit imperative 6/6. Prose works better than the slash form. Nothing in coder-eval expands a slash command; it arrives as plain text the model may ignore.
  • "Live probe — … whether skill_triggered detects a slash-command invocation (it does; scored 1.0)" — that 1.0 was the false positive. The call had errored.
  • "init and ci are optimizable after all" — true, but only with the snapshot fix: delete the disable-model-invocation: line in every arm's snapshot, which is what a real user's slash command effectively does. Verified — the same rows then score 1.000 with the action reference correct.

Also in this pass

  • Verdict on ci: no promotion, a verified ceiling. With the body loaded, the train split scores 1.000 on every row and every criterion (18/18 assertions, completion_rate 1.0). The three candidates were solving a problem that did not exist. Stages B and C were correctly never reached. ~55 Sonnet runs, ~$20.
  • The outcome-suite template (reference/templates/outcome.yaml) + a checked-in worked example (tasks/skills/ci-outcome.yaml), runnable in two documented lines.
  • Engagement is now actually gated (recall.yes: 1.0) in both — they described it as a gate and carried no threshold, which is how a zero-engagement run exited 0.
  • CE035 (partly-labelled split datasets) and CE036 (a row's prompt must not contain the string its criteria grade — four scenarios here had that leak).
  • Tutorial 09, written from the real numbers, plus tutorial 08 made reproducible (snapshot layout, per-stage experiment YAML, the -e command lines) and restructured into two parts.
  • docs/TASK_DEFINITION_GUIDE.md updated — it documented the pre-fix detection rule.

make verify green: 4074 passed, coverage 91.57%.


Third pass — the review's P0/P1/P2 fixes, then a tutorial audit

Implements c/2026-08-13-optimize-skill-review-v2-fixes.md, one commit per phase.

Two behaviour changes to look at first

skill_triggered now requires the signal to have actually delivered. The early-stop watcher evaluates on the tool call, where result_status is still None, so an in-flight or crash-force-closed Skill call live-passed while the frozen check scored it no — live and final verdicts disagreed on the one criterion that decides whether a paid round measured the skill at all. Engagement is now an allowlist: a Skill call counts only on "success" (for that tool the body is the tool result), and a Read/Glob/Grep that errored or has not resolved does not count — the path is in its parameters, but nothing loaded. Bash stays ungated, because cat … | grep exits non-zero after genuinely reading the file. Historical activation P/R/F1 is not directly comparable if a run contained those shapes; two golden-corpus entries were appended to force that acknowledgement rather than editing existing ones.

This reverses a documented design decision, so it is worth a reviewer's eye. Seven tests in tests/test_early_stop.py pinned the opposite semantics as deliberate; each is inverted, not deleted, with the reasoning in its comment, plus a Bash twin for the path that still decides on the call. The cost is stated in the test that pays it: a Skill call whose result never arrives no longer stops the run, so that run continues to its turn cap. The old behaviour stopped it and scored yes — crediting a skill whose body never reached the agent.

A mistyped --split now exits 2 instead of 0. It used to raise, get caught like any load failure, and land in skipped_tasks: one yellow line, zero evaluations, exit 0 — a CI gate reporting success for a one-character typo. SplitSelectorError separates a malformed file (still demoted, so one bad task cannot abort a suite) from a malformed invocation (re-raised — the same selector applies to every task in the run). No new CLI code was needed: the existing except ValueError -> typer.BadParameter supplies the exit.

The rest

  • coder-eval plan expands datasets and takes --split — prints total/selected row counts and catches a bad ${row.*} at plan time rather than per-row after the sandbox is built. The pre-spend check now costs nothing. It is also louder: a task with a missing dataset JSONL now fails a plan it previously passed.
  • Partial split labelling stops being silent — a WARNING with the drop count from expand_dataset, a yellow from plan, and a check in lint-tasks for adopters CE035 cannot reach. All three call row_split_label, so the rule cannot drift even though the messages differ.
  • Row-id validation covers the whole dataset, before --split and sampling narrow it. A malformed row in the test half used to validate under every --split train run and surface at promotion time — the most expensive moment to learn it.
  • The bundled activation.yaml caps and isolates (run_limits, setting_sources: []), mirroring the checked-in worked example key for key. It preached both and shipped neither, so a user who copied it got the opposite of the advice they had just read.
  • CE036 gets real fixtures and a skill_name exemption. Its only test was a repo scan that passed whether or not the rule could still detect anything; detection now lives in a helper the scan and six fixtures share. CE036_LOCATOR_FIELDS is the source and CLAUDE.md the derived surface, with a both-directions parity sensor — that list had already drifted once.
  • reference/optimize-method.md — the track-invariant method extracted from an 859-line SKILL.md as a byte-identical move (verified against HEAD), the three named seams fixed, and the prose sensor split into procedure-vs-method sets that state which file each token belongs to and why.
  • Method caveats the review asked for: promotion verdicts apply to the flag-removed configuration, and Stage B now requires a predeclared primary criterion plus guardrails — scanning every per-criterion aggregate post hoc is uncorrected multiple testing in the rejection direction, so noisy criteria veto real wins.
  • ci-outcome's both-triggers row asked for two triggers and graded one, so a schedule-only workflow scored 1.000 — and it graded the same string as its sibling, discriminating nothing. Fixed without touching any train row, so the file's own measured "the train split scores 1.000 on every row" claim stays true by construction.

Tutorial audit

All nine checked mechanically — every path, relative link, cross-doc anchor, CLI flag against live --help, and every complete YAML snippet against the real models. All clean. By content, three defects, every one the same shape: a hand-maintained number describing a set the code derives.

  • "14 criterion types" (×3) against a registry of 15. CLAUDE.md's heading was right, which is why nothing noticed — it was the only guarded surface.
  • "six commands" in tutorial 07, omitting optimize-skill entirely. A skill-count sensor already existed; this file was not one of the four surfaces it read, and the tutorial's phrasing ("commands") was a fourth the matcher did not know.
  • Tutorial 09 quoting ci-outcome.yaml excerpts this PR had just changed — a reader copying the shown row would have built a suite that raises at expansion.

Each is now derived rather than asserted, and tutorial 07 joins SKILL_DOC_SURFACES.

Tutorial 08 Part 1 was re-measured rather than annotated — 62 Sonnet runs, three replicates of the train split plus one of the test split. lint-tasks still holds at F1 1.000 on a suite three rows larger. analyze reads 1.000 where Part 1 recorded 0.000, because the description this PR promotes is committed (4c7481c): the baseline is unreproducible precisely because the loop worked. And hard-3 is still unstable at exactly the 2-in-3 Step 6 measured — months later, on a different revision of the suite, which is the page's own "two agreeing runs are not evidence" lesson arriving unprompted. The page now states what a reader gets today and why it differs, instead of quietly refreshing the numbers.

Notes for review

  • 6 new sensors + 6 CE036 fixtures, all mutation-verified — each was confirmed to fail when the property it guards is reverted.
  • 1 harness gap deferred: "the ToolStart seam decides" is now a per-criterion property rather than a global invariant. Not mechanically detectable — it is about what a live_verdict reads, which no AST rule can infer.
  • No new Pydantic model, criterion, agent, config key or MergeField; one new exception type (SplitSelectorError), one module logger, one CLI option on an existing command.

make verify green: 4115 passed, 390 lint checks, coverage 91.63%.

uipreliga and others added 9 commits August 12, 2026 11:30
Label dataset rows with a split (tune / holdout / …) and select one at run
time with `coder-eval run --split <name>`. The filter runs BEFORE either
sampler: sampling first would leave an unpredictable (possibly zero) number
of rows per split, destroying the comparison the split exists to protect.

- `Dataset.split_field` (default "split") mirrors `stratify_field`'s shape.
- The filter is inlined in `expand_dataset` rather than extracted: one call
  site, a one-line comprehension, and a helper would re-declare the
  missing-field convention `_stratified_sample` already owns.
- A row is unlabelled when the field is absent, null, or "". A task whose
  rows are all unlabelled passes through unfiltered — `--split` is global to
  the invocation, so an unlabelled suite beside a labelled one must not fail.
  A labelled task with no matching row raises, naming the splits that exist;
  `resolve_all_tasks` records that as a skipped task, so a mistyped selector
  is a zero-task run that still exits 0. Documented rather than papered over.

`--split` unset leaves expansion byte-for-byte unchanged (pinned by test).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…activation template

`/coder-eval:optimize-skill` turns an activation suite's confusion matrix into
candidate description rewrites, A/B tests them as experiment variants, and
promotes only what beats run-to-run noise and then survives a held-out split.
Explicit-invocation only: it spends real money across three stages.

Two mismeasurements were designed out rather than discovered later:

- The sibling-regression gate reads the sibling's `recall.yes`, not its
  precision. Annexation makes the sibling's criterion expected=yes/observed=no —
  a false negative — and `precision = tp/(tp+fp)` stays pinned at 1.0 when the
  sibling never misfires, so a precision gate would gate on a constant.
- Each candidate snapshots the WHOLE skills directory, siblings copied
  unchanged. A variant's `plugins` block replaces the task's, so the snapshot is
  the arm's only skill source: snapshot one skill and every sibling criterion
  silently observes `no` in every arm, and the description is tested against a
  listing it will never face.

Supporting changes: the activation template gains `split_field` and per-row
tune/holdout labels (both splits carry positives and distractors; no
`stop_early:` — that would degrade sibling measurement); run-layout.md documents
the suite-rollup path, aggregate shape, `failed_samples` as the only row-identity
field, and replicate pooling — the contract that keeps someone from "simplifying"
Stage B's three invocations into `--repeats 3`, which pools into one suite.json
and leaves the gate nothing to read.

Five shipped descriptions trimmed so seven skills fit the listing budget
(1,524/1,600) without raising the ceiling: the budget is shared with every skill
the user has installed, so growing our own footprint evicts theirs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lity guidance it disproved

Runs the optimization loop end-to-end against this repo's own `lint-tasks`
skill and reports what actually happened, including the parts that did not go
to plan.

The shipped reachability guidance was wrong. Both the activation template and
`check-skill` said a local plugin `path` is "the directory containing the
skill's directory" — `.claude/skills`. Probing three layouts against the real
CLI shows a plugin path must be a PLUGIN ROOT holding a `skills/` subdirectory
(`<path>/skills/<name>/SKILL.md`; a manifest is optional, the namespace then
defaults to the directory name). A bare directory of skill directories loads
nothing at all. So the correct root for `.claude/skills/my-skill/SKILL.md` is
`.claude`. Every suite `check-skill` generated would have reported recall 0.0 —
precisely the symptom the template's own comment warns "reads exactly like a
broken skill". Corrected in the template, `check-skill` and `optimize-skill`,
and pinned by a new sensor.

The run itself: `lint-tasks` scores F1 1.000 on both splits, so the loop stops
at the diagnosis step rather than spending ~130 further runs chasing a number
the gate makes unreachable. That result closes an open question from the
previous commit — the 66-character description trim is now measured safe rather
than assumed. The headroom turned out to be in the sibling matrix: `task`
annexes setup requests on both splits, and `analyze` never fires on "what
regressed".

The tutorial ends on that, because a walkthrough that only works when the
result is positive teaches the wrong lesson about A/B testing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing --split

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reachability correction reached only half its surfaces. `ci`'s workflow
snippet still exported `SKILL_SOURCE_PATH=.../.claude/skills` — a shipped skill
writing the broken path straight into users' CI, where the failure mode is a
permanent red that looks exactly like the drift the schedule exists to detect.
Same stale form in docs/PLUGIN.md and tutorial 07, plus a self-contradicting
sentence in optimize-skill's own Step 8. All corrected, and the sensor now
covers seven surfaces and asserts the specific layout: its previous
`"skills/" in text` check passed on the pre-fix text via
`.claude/skills/my-skill/SKILL.md`, so it guarded nothing.

`check-skill` was never told about splits, so following it to the documented
row count produced a PARTLY labelled suite — the one state where `--split`
silently drops rows and shrinks the denominator the thresholds gate on. It now
labels every row it writes, and optimize-skill gained the missing branch.

Two rows of the new repo suite asserted `expected_skill: "init"`, a skill that
sets `disable-model-invocation: true` and so can never be engaged: an
unsatisfiable criterion that made every run of that suite exit 1. Relabelled to
`""`, which asks the question that has an answer.

Also: duplicate row ids are now rejected across the whole dataset before
`--split` narrows it, so a duplicate in an unselected split can no longer
validate under every split and surface only on a full run; run-layout documents
`rows_total` / `rows_excluded` / `completion_rate`; and the claim that an
excluded row shrinks a suite "silently" was wrong — the rollup reports it and
it is gateable.

Re-ran both splits after the label fix. `lint-tasks` holds F1 1.000. The
`task`-annexes-setup finding, which had reproduced on both splits, did NOT
survive the third run on byte-identical prompts — so the tutorial now teaches
that instead, which is a better lesson about why the gate demands replicates.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… two open findings

Both follow-ups from the previous round are now settled by measurement rather
than left as open questions.

**The `task` misfire was intermittent, not phantom.** Three tune invocations
show `hard-3` engaging `task` in two of three runs on byte-identical prompts.
The earlier pair of agreeing runs was luck. This is the clearest argument yet
for the replicate gate, and the tutorial now teaches it with the real trace.

**The `analyze` gap was real, stable, and fixable.** Recall 0.500 in all three
runs with precision 1.000 throughout — one row ("what regressed") missed every
time, no over-claiming. The suite gained analyze rows on both splits so the gap
could be measured at all, then the full loop ran:

- Stage A (68 runs): three candidates, all above the incumbent. `b-results`
  looked competitive but lost a row to an error (completion 0.941), so its
  recall came from 3 rows not 4 — ranked out rather than compared.
- Stage B (153 runs, three separate invocations): `a-regression` at F1 1.000 in
  every run against a rock-steady incumbent 0.667. Non-overlapping, no sibling
  regression, precision never off 1.000. One incumbent invocation dropped a row
  and was excluded rather than averaged in.
- Stage C: not obtained. The first holdout was uninformative — a flat tie,
  because every regression-phrased row had been put in tune. Fresh holdout rows
  were authored and the re-run then hit the org's monthly spend limit, which
  removed 11 rows from one arm and 6 from the other and produced a confident
  p = 0.038 pointing the WRONG way. A p-value over an asymmetrically eroded
  sample is not evidence.

`a-regression` is therefore promoted **gated on tune, unconfirmed on holdout** —
stated plainly in the tutorial rather than dressed up. Listing budget holds at
1,574/1,600.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d on holdout

Re-ran the holdout confirmation now that budget is available. Erosion was one
row against the candidate and none against the incumbent — near-symmetric, and
pointing the conservative way — so unlike the spend-limited attempt this run is
interpretable.

  incumbent      analyze recall 0.833  precision 1.000  F1 0.909
  a-regression   analyze recall 1.000  precision 1.000  F1 1.000

The direction reproduces on rows the candidate was never tuned against, which
is what Stage C is required to show. One row separates the arms, and it is one
of the fresh rows authored at promotion time:

  an-6  "Which of my tasks got worse after I switched the model?"
          incumbent     1 of 3      a-regression  3 of 3

No sibling regressed in either arm; the incumbent shows the intermittent `task`
misfire once more, consistent with the 2-in-3 rate measured earlier.

The paired comparison reads exactly zero (p = 1.000). That is the documented
limit of the block rather than a contradiction: it pairs per-row weighted_score
across all three criteria, so a gain confined to one criterion on one row out of
eleven is diluted below what 11 pairs resolve. F1 is the promotion metric; the
tutorial now reports both and says which one the decision rested on.

`a-regression` therefore ships gated AND confirmed, and the earlier
"unconfirmed" framing is gone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Stage C per-row data showed `init` being engaged on the setup rows in both
arms — which contradicted a comment claiming coder-eval's `init` "can never be
engaged" because it sets `disable-model-invocation: true`.

Both halves were wrong in an instructive way. Claude Code ships its own unscoped
`init` skill, and `skill_triggered` matches on the BARE name (it strips any
`plugin:` prefix), so the two are indistinguishable to the criterion. A
`skill_name: "init"` criterion would have silently scored a different skill's
activation as though it were the plugin's. That is a second, independent reason
not to have written one — beyond the unsatisfiability already fixed.

Documented where suite authors will hit it: `check-skill` (which already covers
bare-name matching), the tutorial's caveat section, and the suite's own comment.
A collision does not error; it measures the wrong skill.

Also marks the plan file complete and ticks its 52 acceptance criteria, with a
header recording the four deviations the implementation required.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @uipreliga's task in 1m 29s —— View job


Code Review in Progress

I'm reviewing PR #109 following the guidelines in .github/code_review.md.

Todo List

  • Read .github/code_review.md for review standards
  • Read CLAUDE.md for project conventions
  • Run git diff origin/main...HEAD to see full changes
  • Review changed files with full context
  • Perform cross-file consistency checks
  • Analyze what's missing
  • Provide comprehensive review feedback

Starting review now...

uipreliga and others added 5 commits August 12, 2026 14:46
… body, not just the description

A skill can fail two independent ways: it never gets reached, or it gets
reached and gives bad instructions. optimize-skill only measured the first.

Both tracks now live in the one skill, because they share almost everything —
splits, snapshots, reachability, replicate discipline, the ledger, the stop
rule. What differs is the instrument, and that difference is load-bearing:
`skill_triggered` is a binary one-turn probe that says nothing about the
quality of the work that follows, so an activation suite cannot grade a body.
The execution track uses an ordinary outcome suite with real success criteria.

Three consequences worth calling out, each pinned by a sensor:

- **The two gates differ on purpose.** Activation compares F1, which a pooled
  suite.json cannot report per replicate — hence three separate invocations.
  Execution compares per-row weighted_score, which `paired_comparison` already
  computes correctly over replicates it averages per row before pairing. So the
  paired block that is mere corroboration on the activation track is the
  PRIMARY instrument on the execution track: tested code instead of hand
  arithmetic. Unifying the gates would swap in an instrument that cannot see
  the metric.
- **The prompt rule inverts.** Activation rows must never name the skill (that
  tests obedience, not activation). Execution rows must, to hold activation
  constant so the body is the only variable.
- **`disable-model-invocation: true` now routes instead of stopping.** Such a
  skill's description never enters the activation decision, but its body still
  determines whether it does its job — so `init` and `ci` are optimizable after
  all, just not on the track that was previously the only one.

Also: one variable per round (never both tracks at once), regression coverage
for rows that already pass (a body edit breaks things silently, where the
confusion matrix shows activation regressions for free), and trajectory-based
diagnosis with the five failure modes a body edit actually addresses.

Tutorial 08 now says which track it walks. Listing budget holds at 1,577/1,600.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two agents executed the skill with no prior context and a zero-run budget, one
per track, building real artifacts. Between them they found 15 problems. The
substantive ones:

**The execution track never said how to engage the skill it routes to.** Step 2
sends every `disable-model-invocation` skill there, and Step 4 then said "name
the skill in the prompt" — which does not work: such a skill is not offered to
the model at all, so prose gets "no such skill is available" and the row
measures nothing. Verified live, and verified the fix: a slash command in
`initial_prompt` DOES load it, emits a real `Skill` tool call, and is detected
by `skill_triggered` (probe scored 1.0). The routing was right; the mechanism
was missing. Now specified, with a worked `initial_prompt`.

**Two silent confounds in the snapshot step.** The diagram showed only
`skills/` while the prose said "everything that source contained" — a
diagram-faithful snapshot mounts skills whose `${CLAUDE_PLUGIN_ROOT}/reference/`
files are gone, invisibly on the activation track. And dropping
`.claude-plugin/plugin.json` makes the namespace default to the arm's directory
name, so arms differ in the name shown in the listing as well as the text under
test — on the one track where activation is a competition between listings.

**A version string is not a capability check.** Two binaries reported the same
0.9.6 and differed in whether `--split` exists at all; the pinned-version rule
said "carry on" while every run would fail at load. Step 1 now requires
`coder-eval plan <suite>` to exit 0 before spending.

**The skill writes descriptions and never mentioned the length ceiling** every
natural fix pushes against. Now budgeted before candidates are written.

Also: the baseline is now a line item in the cost table with its
non-redundancy explained (it validates the snapshot wiring against the task's
own source); `completion_rate` is documented as per-criterion-aggregate rather
than top-level; the execution track's no-suite stop matches the activation
track's force and carries the split-label and slash-invocation requirements into
the hand-off; suite sizing resolves to a number rather than contradicting
itself; Step 3 gains a "check whether a lint rule answers this for zero runs"
pre-check; and repo-layout.md now prunes `tmp/` and any already-found run store,
which had been returning hundreds of agent-produced SKILL.md files.

Fixes two bugs this branch introduced: an off-by-one step reference from the
renumbering, and tutorial 08 claiming the skill hard-stops where it now routes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…vention

Tutorial 08 shipped without the YAML front-matter every other tutorial carries,
and with an H1 that did not follow `# Tutorial NN — Title Case`. The
front-matter is not decorative: mkdocs renders `description:` into the page's
`<meta name="description">`, so the page had no search or social-preview
summary at all.

- adds the folded `description: >-` block, matching 01–07's shape and voice
- retitles to `# Tutorial 08 — Optimizing a Skill Description`
- title-cases the nav and tutorials/README entries, which were the only
  lowercase ones in either list
- regenerates docs/llms.txt via `make docs-indexes` (verified idempotent)

Audited the rest of the tree while here: 24 of 24 docs pages now carry a
front-matter description; 08 was the only one missing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…, and label them for the user

Terminology: `train`/`test` is the vocabulary everyone already has, and split
values are open strings, so this is docs and content only — no schema change,
no migration. `Dataset.split_field` still defaults to `"split"`; only the values
this project's own suites and guidance use have changed.

215 replacements across 19 files, then a pass to disambiguate bare "test" back
to "test split" wherever it named the data half rather than a check — in a
testing framework "without a test" reads as "without a unit test", which is
exactly the wrong parse.

One honest imprecision worth noting: nothing is *trained* here. You hand-select
a description from candidates, which is a form of fitting, so "train" is
defensible in the ML sense of "the data you made your choices on" — and it is
far more recognisable than "tune". Keeping the familiar word.

Also removes the labelling friction that prompted this. Step 5 previously told
the skill to *offer* to add labels, which left the user with a mechanical JSONL
edit and the balance to get right by eye. It now does the labelling itself and
shows the resulting counts for objection, under two rules that are easy to get
wrong manually: stratify within each polarity so both halves carry positives and
distractors, and assign deterministically so the split never re-rolls between
rounds — a reshuffled split is not a test split, because rows already tuned
against leak into it.

Verified end to end: `--split train` -> 17 rows, `--split test` -> 11, unsplit
-> 28.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`!!! note` / `!!! danger` are mkdocs-material syntax. GitHub does not understand
them: it renders the marker as literal text and turns the indented body into an
accidental code block, so both callouts looked broken when read from the repo or
a PR diff.

Tutorials 01-07 use plain `>` blockquotes, which render correctly in both
places — I had introduced the admonitions without checking the convention.
Converted both callouts, and added a lint guard so the next tutorial cannot
reintroduce them (mutation-tested: it fires on an injected `!!! note`).

Scoped to tutorials deliberately. The one admonition under docs/ is in
DATASETS.md, is site-first reference material, and predates this branch.

Also drops `<sub>` tags from the skill's cost formula — a SKILL.md is read as
plain text by a model, so HTML there is noise rather than markup.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: coder_eval — pr:109

Scope: pr:109 · branch feat/plugin-optimize-skill · f1a684d · 2026-08-13T04:57Z · workflow variant

Change class: complex — introduces a new dataset row-filter control path (--split) with new validation/error semantics that interacts with the existing sampler win-order, plus a new published plugin skill and its lint/doc-surface enforcement

Architecture and security are flawless (both 10/10) and the --split feature is well-designed and thoroughly documented, but the real risk is a cluster of silent-green paths around the new row-selection stage — a mistyped or unmatched --split produces a zero-task run that still exits 0, a split_field naming no row key makes the filter an unannounced no-op over the full dataset, and the CLI plumbing for the flag is dead-code-tolerant (mutation-proven) — so the bottom line is: merge once the selector paths are made loud, since every one of these can report a confident train/test result that was never actually measured.

Summary

Axis Score 🔴 🟠 🟡 🔵 Top Issue
1. Code Quality & Style 8.6 / 10 0 0 2 4 expand_dataset absorbs the --split filter inline: cyclomatic complexity rises C(15) -> D(22)
2. Type Safety 9.3 / 10 0 0 1 2 A split_field that names no key present in the rows makes --split a silent no-op: the full dataset runs under every split name, with no warning (task_loader.py:437)
3. Test Health 7.9 / 10 0 1 2 1 New --split CLI flag is untested at the CLI layer — mutation-proven: nulling split=split at run_command.py:500 keeps the whole suite green (4301 passed, 14 skipped), and the documented exit-0-on-mistyped-split path is unasserted
4. Security 10 / 10 0 0 0 0
5. Architecture & Design 10 / 10 0 0 0 0
6. Error Handling & Resilience 9.5 / 10 0 0 1 0 load_task's blanket except Exception (task_loader.py:68, wrapping the try opened at line 60) relabels programming errors as ValueError, defeating the narrow-catch contract documented at experiment.py:632-637 and demoting first-party regressions to SkippedTask
7. API Surface & Maintainability 8.4 / 10 0 1 1 1 Mistyped/unmatched --split yields a zero-task run that still exits 0 (no run-level guard; asymmetric with the zero-task-files fatal path)
8. Evaluation Harness Quality 9.4 / 10 0 0 1 1 Published activation template ships no run_limits, while the PR's own new in-tree suite documents caps as load-bearing for its metric denominator

Overall Score: 9.1 / 10 · Weakest Axis: Test Health at 7.9 / 10
Totals: 🔴 0 · 🟠 2 · 🟡 8 · 🔵 9 across 8 axes.

Blockers

  1. [Axis 3] New --split CLI flag is untested at the CLI layer — mutation-proven: nulling split=split at run_command.py:500 keeps the whole suite green (4301 passed, 14 skipped), and the documented exit-0-on-mistyped-split path is unasserted (tests/test_dataset_expansion.py:742) — Two halves, both proven by direct experiment in the PR-HEAD worktree.

(a) PLUMBING. src/coder_eval/cli/run_command.py:500 reads split=split, (inside the config = BatchRunConfig( call at line 490). I mutated exactly that line to split=None, and ran the full suite: 4301 passed, 14 skipped in 39.06s — zero failures. So the flag can be completely dead (every run silently expands the whole dataset instead of the requested split, and the reported recall/precision/F1 is pooled over train+test while the operator believes they measured one half) with a green make verify. grep -rn 'split=' tests/*.py shows the deepest entry point any test exercises is BatchRunConfig(run_dir=..., split="train") (tests/test_dataset_expansion.py:690, :732, :756) — nothing calls run_command/_run_all_tasks with a split, unlike the CLI-level precedents in tests/test_cli_type_flag.py and tests/test_run_command_junit.py.

(b) EXIT CODE. test_unmatched_split_demotes_the_suite_to_a_skipped_task (line 742) stops at the resolver: it asserts resolved == [] and "no rows in split 'holdou'" in skipped[0].reason (lines 758-761). It does not assert the consequence CLAUDE.md and the skill both state: with zero tasks, summary.tasks_failed/tasks_error/failed_suite_gates are all 0, so run_command.py:560 (if summary.tasks_failed > 0 or summary.tasks_error > 0 or failed_suite_gates > 0:) never raises typer.Exit(1) → a green CI run over zero rows. The only user-visible signal is the console.print at run_command.py:691, which the routed coverage report lists as an uncovered line.

Fix: add a _run_all_tasks(..., split="test") test asserting the resolved/run task set (kills mutation (a)), plus one asserting a mistyped split exits 0 with the suite in run.json skipped_tasks (pins (b) as intended rather than accidental). Also pin the one prose defense in tests/test_custom_lint.py::test_optimize_skill_keeps_its_load_bearing_instructions — its ~18-token list omits plugins/coder-eval/skills/optimize-skill/SKILL.md:249 ("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."), which is the only thing standing between a typo and a fabricated result.
2. [Axis 7] Mistyped/unmatched --split yields a zero-task run that still exits 0 (no run-level guard; asymmetric with the zero-task-files fatal path) (src/coder_eval/cli/run_command.py:560) — The run's exit gate keys only on executed-task outcomes — run_command.py:559-561:

    # Exit with non-zero code if any tasks failed, errored, or any suite failed its thresholds.
    if summary.tasks_failed > 0 or summary.tasks_error > 0 or failed_suite_gates > 0:
        raise typer.Exit(1)

A labelled suite with no row in the requested split raises in expand_dataset (task_loader.py:441-446), which resolve_all_tasks catches into skipped (experiment.py:632-639). With every task file demoted, tasks_failed == tasks_error == failed_suite_gates == 0, so coder-eval run <suite> --split trian prints one yellow line and exits 0. The CLI is already inconsistent about this: zero task files is fatal — run_helpers.py:92-94 if not all_task_files: console.print("[red]No task files found![/red]"); raise typer.Exit(1) — while zero resolved tasks is green, so the same user-visible outcome ("nothing ran") returns two different exit codes depending on which stage produced the emptiness. The hazard is documented (docs/DATASETS.md:154-155: "a mistyped selector (--split holdou) produces a run of zero tasks that still exits 0") and deferred in .claude/harness-candidates.md:317-330, but --split is precisely what makes it reachable from a typo rather than a broken YAML, and the optimize-skill workflow it exists to serve passes --split on every one of its ~10 invocations (plugins/coder-eval/skills/optimize-skill/SKILL.md:466, 495-497, 548) — a truncated selector there reads as "the stage ran and nothing regressed". Narrow fix that avoids re-litigating skip: true semantics: after resolve_all_tasks, fail non-zero when a CLI selector (config.split, --tags/--exclude-tags) was supplied and resolved is empty — unambiguously a user error rather than repo state. Note line 691 (the skipped banner, the only on-screen signal) is itself uncovered per the routed coverage report, so add a CLI test for the all-skipped-under---split path either way.

Non-blocking, but please consider before merge

  1. [Axis 1] expand_dataset absorbs the --split filter inline: cyclomatic complexity rises C(15) -> D(22) (src/coder_eval/orchestration/task_loader.py:361) — Routed radon signal, quantified against the baseline: radon cc -s on origin/main's copy reports F 341:0 expand_dataset - C (15); on the PR HEAD copy it reports F 361:0 expand_dataset - D (22) — this PR added 7 of the 22 and pushed the function out of the C band. (The other routed offenders are untouched here: resolve_all_tasks is E (36) on BOTH main and PR HEAD — the PR adds only split=config.split at experiment.py:628 — and cli/run_command.py::run_command is C (12) on both.) The growth is a self-contained filter block that the module's own idiom says should be a helper: _load_dataset_rows, _stratified_sample, and the PR's own _reject_duplicate_row_ids (line 313) are all extracted, but this one is inlined at lines 431-446:
    if split is not None:
        field = task.dataset.split_field
        ...
        labelled = [r for r in rows if r.get(field) not in (None, "")]
        if labelled:
            rows = [r for r in labelled if str(r[field]) == split]
            if not rows:
                raise ValueError(...)

Extract it as _filter_by_split(rows, field, split, task_id) -> list[dict[str, Any]] beside _stratified_sample, which restores expand_dataset to roughly its pre-PR complexity, moves the 4-line 'unlabelled means absent/null/""' rationale comment onto the helper's docstring, and gives the semantics a unit-testable seam of its own. While there, consider narrowing _reject_duplicate_row_ids(rows, task) to (rows, id_field, task_id) so its assert task.dataset is not None disappears.
2. [Axis 1] New lint test duplicates the JSONL row-loader (and a function-local import json) verbatim from the test directly above it (tests/test_custom_lint.py:1422) — test_activation_rows_split_both_polarities_both_sides (line 1416) copies the loader from test_activation_rows_have_both_polarities (line 1404) character for character — lines 1422-1428 repeat lines 1405-1411:

        import json

        rows = [
            json.loads(line)
            for line in (self.TEMPLATES / "activation-rows.jsonl").read_text(encoding="utf-8").splitlines()
            if line.strip()
        ]

That is two copies of the same 6 lines in adjacent methods of one class, including a redundant function-local import json in each (the module imports re and Path at the top, lines 13-14, so there is no import-cost reason for the locals). Hoist a single def _template_rows(self) -> list[dict[str, Any]] helper onto TestPluginArtifacts (it already carries self.TEMPLATES) and move import json to the module header; both tests then read as the assertion they are about. Also note the pattern will recur — this is the second reader of that template file, so a third would copy it again.
3. [Axis 2] A split_field that names no key present in the rows makes --split a silent no-op: the full dataset runs under every split name, with no warning (task_loader.py:437) (src/coder_eval/orchestration/task_loader.py:437) — The unlabelled state is inferred from row data rather than declared, and the two very different causes collapse to the same value domain. task_loader.py:437-446:

labelled = [r for r in rows if r.get(field) not in (None, "")]
if labelled:
    rows = [r for r in labelled if str(r[field]) == split]
    ...
# else: no row in this task carries a split label -> --split does not apply here.

field comes from Dataset.split_field (src/coder_eval/models/tasks.py:261), a free str with no min_length and no cross-check against the row keys — pyright and pydantic both accept any value. Verified empirically against the PR worktree with a 2-row labelled dataset ({'id':'a','split':'train'}, {'id':'b','split':'test'}):

  • split_field='spilt' (typo) + --split test['a', 'b'] (both rows, filter silently inert)
  • split_field='' + --split test['a', 'b'] (same)

So a user whose JSONL column is set/fold, or who typos either the YAML value or the JSONL key, gets the full dataset under both --split train and --split test with no warning — exactly the tutorial-08 / optimize-skill workflow, where the two arms would then be identical full-dataset runs reported as "confirmed on held-out data". Contrast the mistyped-selector case, which DOES surface (documented in docs/DATASETS.md); the mistyped-field-name case fails in the opposite, silent direction.

Fix: make the inert case observable rather than inferred. Cheapest form — when split is not None and a dataset-backed task took the else branch at line 446, logger.warning("--split %r: task %r has no row carrying split_field %r; running all %d rows", split, task.task_id, field, len(rows)). Optionally add min_length=1 to Dataset.split_field so the empty-string form is rejected at load. A CE-style lint rule can also assert that any tasks/**.yaml setting split_field names a key present in its dataset.paths rows.
4. [Axis 3] Split label vocabulary (train/test) is pinned on the prose side but not the data side — renaming the shipped template's labels keeps all 344 lint tests green while every documented --split command hardcodes the old names (tests/test_custom_lint.py:1437) — The new guard deliberately accepts any two names: line 1437 is assert len(by_split) >= 2, f"template rows collapsed to a single split: {sorted(by_split)}", and line 1429 only checks assert all(r.get("split") for r in rows). Meanwhile the consuming surfaces hardcode the names: plugins/coder-eval/skills/optimize-skill/SKILL.md:247 (coder-eval run <suite> --split train -D run_limits.stop_early=false), :466, :495-497, :548, :581 (--split test --repeats 3); plugins/coder-eval/skills/check-skill/SKILL.md:159 ("every row's splittrain or test"); docs/tutorials/08-optimizing-a-skill.md. I verified the gap by rewriting the shipped template rows' labels to fit/eval (plugins/coder-eval/reference/templates/activation-rows.jsonl) and running uv run pytest tests/test_custom_lint.py: 344 passed. With that drift, every suite a user copies raises "no rows in split 'train'" → demoted to skipped_tasks → the zero-row green run of finding #1. This file already has the exact pattern to reuse — test_lint_tasks_does_not_flag_the_shipped_activation_template (line 1581) reads the template with yaml.safe_load and asserts its real contents match what the skill prose claims. Do the same here: assert set(by_split) == {"train", "test"}, or better, derive the expected names from the --split <name> tokens found in optimize-skill/SKILL.md so the two sides can never drift independently.
5. [Axis 3] The repo's own new activation suite gets none of the dataset guards the shipped template gets — five template-pinned guards, zero for tasks/skills/lint-tasks-activation* (tests/test_custom_lint.py:1416) — test_activation_rows_split_both_polarities_both_sides (line 1416) and the three sibling template guards (test_activation_template_expands_to_one_task_per_row line 1270, test_activation_template_thresholds_use_real_metric_keys line 1287, test_activation_template_makes_the_skill_reachable) are all hardcoded to self.TEMPLATES / "activation-rows.jsonl" / activation.yaml. The PR also adds a real suite — tasks/skills/lint-tasks-activation.yaml + tasks/skills/lint-tasks-activation-rows.jsonl (28 rows) — which drives the paid optimize-skill dogfood rounds and has zero guards. Proven: I deleted , "split": "train" from the pos-1 row of tasks/skills/lint-tasks-activation-rows.jsonl and the full suite still reported 4301 passed, 14 skipped — i.e. precisely the state the guard's own message calls "the one bad state: --split keeps the matching rows and drops the unlabelled ones, shrinking the suite the metrics are computed over" (line 1430-1432) is unguarded on the suite that actually runs. Note the only whole-tree task check, the CE034 parametrization at line 2661 (sorted(p for p in (... / "tasks").rglob("*.yaml") ...)), calls load_task (line 2666) but never expand_dataset, so a bad dataset.paths entry, a duplicate row id, or an unsubstituted ${row.…} in the new suite is also uncaught. Fix: parametrize the split-labelling/balance guard (and an expand_dataset smoke assertion) over both jsonl/yaml pairs instead of the template alone.
6. [Axis 6] load_task's blanket except Exception (task_loader.py:68, wrapping the try opened at line 60) relabels programming errors as ValueError, defeating the narrow-catch contract documented at experiment.py:632-637 and demoting first-party regressions to SkippedTask (src/coder_eval/orchestration/task_loader.py:68) — Pre-existing, but load-bearing for the error routing this PR extends, and it contradicts a comment in another in-scope file. task_loader.py:62-69:

try:
    task = TaskDefinition(**task_data)
    task = resolve_template_paths(task, task_file.parent)
    task = resolve_initial_prompt_file(task, task_file.parent)
    task = resolve_system_prompt_files(task, task_file.parent)
    task = resolve_dockerfile_path(task, task_file.parent)
    return task, raw_yaml
except Exception as e:
    raise ValueError(f"Invalid task definition: {e}") from e

Meanwhile experiment.py:632-636, the handler that consumes it, claims the opposite:

# 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
# silently demote every task to "skipped".

The narrowing is defeated for the whole load_task half: an AttributeError/TypeError/ImportError raised inside any of those four path resolvers is converted to ValueError at line 68, matches except (... ValueError ...) at experiment.py:637, and is demoted to a SkippedTask. With no all-skipped guard on that block (finding #1), a first-party regression in path resolution therefore produces a green, zero-task run rather than a crash — precisely the outcome the comment says is prevented.

Fix: narrow line 68 to the exception classes that genuinely mean "invalid task definition" — except (ValidationError, ValueError, FileNotFoundError, OSError) as e: — so programming errors propagate uncaught and reach the deliberately-unhandled path. This is grep/AST-shaped and a good CEnnn candidate: forbid except Exception in orchestration/ load paths whose callers document a narrow catch set. n/a
7. [Axis 7] optimize-skill cost table references undefined M_tune / M_holdout symbols after the train/test rename (SKILL.md:444) (plugins/coder-eval/skills/optimize-skill/SKILL.md:444) — Technique-1 rename grep over src/ tests/ docs/ tasks/ plugins/ experiments/ CLAUDE.md README.md .claude/ finds the rename otherwise complete, with three stragglers — the first is a real broken reference. SKILL.md:440 defines the symbols:

`M_train` train rows and `M_test` test rows:

but the table immediately below uses the retired names (SKILL.md:444-447):

| Step 6 baseline | `M_tune` |
| Stage A — triage | `(N+1) × M_tune` |
| Stage B — gate | `3 × (S+1) × M_tune` |
| Stage C — confirm | `6 × M_holdout` |

M_tune and M_holdout are defined nowhere in the file, and this table is load-bearing: the skill requires the agent to "State the projected run count before each stage and ask" before spending real money. Rename all four to M_train / M_test. Two lower-impact sites of the same class: src/coder_eval/models/tasks.py:264 offers a third vocabulary word the project no longer teaches — "Row field naming the row's split (e.g. 'train' / 'test' / 'holdback'). " (this Field description is the schema's public documentation; use 'train' / 'test'), and docs/DATASETS.md:154 illustrates a typo with `--split holdou`, a truncation of the retired holdout — switch to a truncation of a label the docs actually teach (e.g. --split trian) so a reader cannot mistake it for a valid split name.
8. [Axis 8] Published activation template ships no run_limits, while the PR's own new in-tree suite documents caps as load-bearing for its metric denominator (plugins/coder-eval/reference/templates/activation.yaml:41) — The in-tree suite added by this PR caps every row — run_limits: {max_turns: 2, turn_timeout: 120, task_timeout: 300} (tasks/skills/lint-tasks-activation.yaml:39-42) — and explains why in the comment above it: "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)." That mechanism is real: reports.py:848-850 slices row.result.success_criteria_results[i] only if i < len(...), so a row that errored before criteria ran silently leaves per_rows, and .claude/shared/run-layout.md:34-36 documents it as rows_excluded. The template that every check-skill-generated suite is copied from has an agent: block at line 41-44 and a dataset: block at 46, and no run_limits: anywhere — so generated suites inherit experiments/default.yaml's max_turns: 20 / task_timeout: 600 (10× the cost per activation probe) and gate recall.yes/precision.yes (lines 64-66) over a denominator that timeouts can erode with nothing failing. Add the same run_limits block to the template with the real task's comment, and consider adding completion_rate: 1.0 to its suite_thresholds — run-layout.md:39-42 documents that exact gate for this purpose. (setting_sources: [] is a second divergence — the real task calls the host CLAUDE.md "expensive and a confound" — but it needs type: "claude-code", which would pin the template to one agent, so that half is a judgement call.) n/a

Nits

  1. [Axis 1] "Three requirements specific to this track" is followed by four bullets (plugins/coder-eval/skills/optimize-skill/SKILL.md:163) — Line 163 reads "Three requirements specific to this track:" and the list under it has four top-level bullets: line 165 "Invoke the skill from the prompt.", line 187 "Assert engagement, do not assume it.", line 191 "Score outcomes, not prose.", line 196 "Cover what already works, not just what is broken.". An agent executing the skill counts requirements to check it has satisfied them all, so a miscount invites treating the fourth as optional commentary. Change to "Four requirements" (the parallel enumerations elsewhere in the file are correct — "Five facts" at line 414 has exactly five bullets, "Two rules" at line 219 has two), or drop the count and let the list speak for itself so a future bullet cannot make it stale again.
  2. [Axis 1] Two-topic comment block sits above the wrong statement, and a comment marks code that is no longer there (src/coder_eval/orchestration/task_loader.py:422) — Lines 422-428 are one unbroken 7-line comment covering two unrelated decisions, and the half that comes first describes a statement nine lines below it. The line it actually sits on top of (429) is the duplicate check, not the split filter:
    # --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
    # ...
    _reject_duplicate_row_ids(rows, task)

Split it: keep the duplicate-id rationale (lines 425-428) above line 429, and move the ordering rationale (lines 422-424) down onto the if split is not None: block at line 431 (or onto the _filter_by_split helper suggested in the complexity finding). Relatedly, lines 479-480 leave a comment where the deleted per-row check used to be — "# Uniqueness is already enforced across the whole dataset by / # _reject_duplicate_row_ids, before filtering narrowed rows." — inside the hot per-row loop, annotating absent code; the docstring's Raises section already states it, so this can go.
3. [Axis 1] The skill tells users to write split_field: "split", which only restates the model default — now duplicated across four surfaces (plugins/coder-eval/skills/optimize-skill/SKILL.md:217) — Dataset.split_field defaults to "split" (src/coder_eval/models/tasks.py:261-262), so the key is a no-op whenever rows use the conventional field name — yet line 217 instructs the agent to add it unconditionally: "Rows unlabelled → add split_field: \"split\" to the dataset: block and write a \"split\" into every row". Following that guidance has already seeded four copies of the redundant line — plugins/coder-eval/reference/templates/activation.yaml:51, tasks/skills/lint-tasks-activation.yaml:47, docs/tutorials/08-optimizing-a-skill.md:100 — all with the literal default value, and none of the repo's YAML sets a non-default (grep -rn split_field shows every occurrence is "split"). It also implies the key is required for --split to work, which it is not. Reword line 217 to "write a split into every row (dataset.split_field only needs setting if your rows name the field something else)" and drop the key from the template/task/tutorial snippets; if no dataset ever needs a custom name, the knob itself is a candidate for removal (it mirrors stratify_field, which likewise has no non-default user outside tests).
4. [Axis 1] New SKILL.md is 626 lines / 5,647 words — roughly double the next-largest skill, with no use of the plugin's reference/ progressive-disclosure pattern (plugins/coder-eval/skills/optimize-skill/SKILL.md:1) — wc -l plugins/coder-eval/skills/*/SKILL.md gives 626 for optimize-skill against 330 (analyze), 256 (check-skill), 243 (task), 220 (ci), 218 (lint-tasks), 132 (init); by wc -w it is 5,647 vs 2,444 for the next largest — 2.3x. The whole body loads into context on invocation, and several sections are self-contained deep detail that only one step needs: Step 10's three-stage gate reasoning (lines 437-596, ~160 lines, including the per-track gate rationale and the recall-vs-precision derivation at 520-527) and Step 8's snapshot-layout rules (lines 311-385). The plugin already has the mechanism for this — skills read ${CLAUDE_PLUGIN_ROOT}/reference/*.md on demand (this file does so at lines 39, 60, 194, 263). Consider moving the stage/gate detail to reference/optimize-gates.md and keeping the step body as the decision plus a pointer, which also shrinks what a reviewer must re-read when one stage changes. Not urgent — the file is well organized and readable as-is.
5. [Axis 2] split: str | None admits "", the exact sentinel the row side treats as unlabelled, making --split "" unsatisfiable by construction (src/coder_eval/cli/run_command.py:300) — src/coder_eval/cli/run_command.py:300 declares split: str | None = typer.Option(None, "--split", ...) and threads it unchanged through BatchRunConfig.split (src/coder_eval/orchestration/config.py:89, also str | None) into expand_dataset(split=...). The type therefore admits "", but task_loader.py:437 defines a row as unlabelled precisely when its value is None or "" — so no labelled row can ever match split="". Verified in the PR worktree: expand_dataset(mk([{'id':'a','split':'train'},{'id':'b','split':'test'}]), Path('.'), split='') raises Dataset for task 'suite' has no rows in split '' (split_field='split'); labelled splits present: ['test', 'train'], and since resolve_all_tasks catches ValueError into skipped_tasks, coder-eval run --split "" yields a zero-task run that exits 0. Narrow the value at the boundary instead of letting an impossible value travel the whole chain: either reject it in the CLI (a Typer callback raising typer.BadParameter("--split requires a non-empty name")) or normalize split or None before building BatchRunConfig.
6. [Axis 2] _reject_duplicate_row_ids takes a wider type than its precondition and re-narrows with assert, unlike its sibling helper one function up (src/coder_eval/orchestration/task_loader.py:313) — The new helper's signature is def _reject_duplicate_row_ids(rows: list[dict[str, Any]], task: TaskDefinition) -> None: (line 313) and its first statement is assert task.dataset is not None (line 321) purely to narrow before id_field = task.dataset.id_field. It uses only two things from task: task.dataset.id_field and task.task_id. The sibling helper directly above already models this correctly — def _load_dataset_rows(dataset: Dataset, task_file_dir: Path) -> list[dict[str, Any]]: (line 276) takes the narrowed Dataset so no narrowing assert is needed for the parameter itself. Prefer _reject_duplicate_row_ids(rows: list[dict[str, Any]], dataset: Dataset, task_id: str) -> None and call it as _reject_duplicate_row_ids(rows, task.dataset, task.task_id) at line 429, where task.dataset is already narrowed by the if task.dataset is None: return [task] guard at line 415. That deletes the assert (which python -O strips) and makes the precondition expressible in the type rather than at runtime.
7. [Axis 3] Dead assertion clauses in the new reachability guard — one can never fail, the other would raise IndexError if it ever fired (tests/test_custom_lint.py:1361) — In the PR-added test_reachability_guidance_names_the_plugin_root_layout:

  • Line 1361: if "SKILL_SOURCE_PATH=" in line or "SKILL_SOURCE_PATH=" in line.replace('"', ""): — the second clause can only be true when the first is false if the raw line embeds a quote inside the token (e.g. SKILL_SOURCE_PATH"="/x/skills). In that case line 1362 (value = line.split("SKILL_SOURCE_PATH=", 1)[1]...) splits the unmodified line, yielding a 1-element list and an IndexError instead of the intended assertion message. Drop the or clause, or normalize once (stripped = line.replace('"', "")) and split the same string you tested.
  • Line 1377: assert ".claude/skills" in text and ".claude" in text, ( — the second conjunct is implied by the first and can never fail independently; it reads as two checks but is one. Drop it (or make it a distinct check, e.g. that the surface also shows the correct .claude form on its own).
  1. [Axis 7] Skipped-tasks banner enumerates only two of the now-three causes, misdirecting the operator on a mistyped --split (src/coder_eval/cli/run_command.py:693) — run_command.py:690-694 is the only on-screen signal that a --split selector matched nothing:
    if skipped:
        console.print(
            f"[yellow]⚠[/] {len(skipped)} task file(s) skipped "
            + "(load errors or `skip: true` — see run.json `skipped_tasks` for reasons)"
        )

The parenthetical names two causes; this PR adds a third (a labelled dataset with no row in the requested split), so an operator who typo'd --split is told to look for a broken YAML or a quarantined task. Either add the third cause to the string, or drop the enumeration and print each SkippedTask.reason inline — the reason strings already carry the full ValueError text from expand_dataset (experiment.py:637: reason = f"{type(exc).__name__}: {exc}"[:500]), which names the splits that do exist. Line 691 is also flagged uncovered in the routed coverage report, so this whole branch is untested.
9. [Axis 8] The row-selection selector is absent from run.json / suite.json, the documented consumer contract (src/coder_eval/orchestration/config.py:89) — BatchRunConfig.split (config.py:89-96) joins max_rows and sample_per_stratum as a third knob that changes which rows enter a suite metric, and none of the three is echoed into RunSummary (models/results.py:992-1058, which does echo max_parallel at line 1044 as precedent) or into SuiteRollup. So a suite.json read in isolation — the shape an external consumer reads — cannot distinguish a --split test rollup from a full-suite one; only rows_total (a number) and the per-row task_ids hint at it. It is not lost entirely: compute_run_fingerprint dumps the whole config (return config.model_dump(mode="json") | {...}, batch.py:485) into resume_fingerprint.json, but that file is named for --resume and is not mentioned anywhere in .claude/shared/run-layout.md or its plugin mirror. Either surface split on SuiteRollup/RunSummary, or document resume_fingerprint.json in the run-layout contract as the run's config record. Note the tune/holdout → train/test rename is not a cross-repo risk: git grep holdout origin/main returns nothing, so the old labels never shipped and no downstream query can be pinned to them. n/a

What's Missing

Daily/nightly:

  • 🟠 Blast radius of the new 28-row PAID suite is unstated. It lands in tasks/ with no skip: true (the repo's existing convention for an environment-dependent suite — tasks/agents/codex_disallowed_tools_test.yaml) and no tag that excludes it from anything. Zero-arg coder-eval run is documented as recursive discovery over tasks/ (cli/run_helpers.py::discover_default_tasks), and any external nightly / eval-runner that globs tasks/** now picks up 28 full agent runs x 3 skill_triggered criteria. Worse, its plugin path is $SKILL_SOURCE_PATH: unset — the default anywhere the operator did not hand-export it — every row scores recall 0, the recall.yes/precision.yes: 0.7 gates fail, and the run exits non-zero. That is a permanent red indistinguishable from the real activation drift the suite exists to detect (the same failure mode the ci skill warns about at SKILL.md:159-168). The PR should say what the nightly/tasks/** consumers do with this suite, or gate it (skip: true / a tag) and document the opt-in. (trigger: tasks/skills/lint-tasks-activation.yaml)

Downstream consumers:

  • 🟡 suite_thresholds are evaluated over whatever denominator the split leaves, and nothing recalibrates them per split. The PR's own suite gates recall.yes: 0.7 (line 60-62) over 5 lint-tasks positives on train (4/5 = 0.80 passes) but only 3 on test — where 2/3 = 0.667 fails, so the held-out gate silently demands 100%. The shipped template is worse: after this PR's labelling it has 2 positives on train and exactly 1 on test, so activation.yaml's recall.yes: 0.7 on the test half is decided by a single coin flip. optimize-skill Stage C ("confirm on the test split") will therefore report a threshold failure at a level train tolerated, reading as a regression that is pure granularity. check-skill:132-134 does warn that "recall over 3 rows moves in 33-point jumps" and DATASETS.md:206-208 says budget double the rows, but neither the template's own gate values, the new in-tree suite, nor Stage C's promotion checklist tells the user to re-derive the thresholds for the smaller half. (trigger: tasks/skills/lint-tasks-activation.yaml)
  • 🟡 No output artifact records which split produced a run, so every downstream consumer of the changed row-selection logic silently conflates halves: (a) reports_junit.py names testcases per row-task and the suite gate as variant/suite_id (lines 253, 343), so a --split train run's JUnit XML shows the test rows as vanished tests and files its gate result into the same CI-history bucket as a full-suite or test-split run; (b) /coder-eval:analyze (SKILL.md, split-unaware — zero mentions) and the evalboard read suite.json and cannot tell an in-sample train rollup from a held-out one; (c) this PR's brand-new run-layout contract section (.claude/shared/run-layout.md:17-63 and its plugin mirror) documents rows_total / rows_excluded / completion_rate in detail but never mentions that a CLI selector may have chosen the rows. The only record is resume_fingerprint.json, which the contract doc does not list, plus whatever the operator encoded in --run-dir by hand. (trigger: .claude/shared/run-layout.md) (restates: Axis 8: split absent from run.json / suite.json)

Parallel paths:

  • 🟡 The CI surface was not extended for the split concept, so the workflow ci emits is in-sample after an optimize-skill round. optimize-skill tunes prose against --split train (SKILL.md:247, 466, 495-497, 548 — all six of its coder-eval run invocations pass --split), and ci then writes the weekly gate that runs the suite with no selector at all, i.e. train+test pooled — the gate's numbers partly re-measure the rows the description was fitted to, which is exactly what the train/test discipline exists to prevent. It is expressible today (extra-args: --split test via action.yml's extra-args input; there is no dedicated split input), but the ci skill's diff here touches only SKILL_SOURCE_PATH and says nothing about splits, and neither does check-skill's new split section when it hands off to CI. (trigger: plugins/coder-eval/skills/ci/SKILL.md)
  • 🟡 The two new split hazards are documented in prose only; the plugin's own linter surface — the natural mechanical home for them — was not extended. /coder-eval:lint-tasks (and the shared reference/task-rubric.md it reads with task) gained no rule for (a) a PARTIALLY split-labelled dataset, which check-skill:165-168 now calls "the one state to avoid" because --split silently drops the unlabelled rows and shrinks the gate's denominator, or (b) a split_field naming a key no row carries, which makes --split an inert no-op that runs the full dataset under both selectors. Both are decidable from the task YAML plus its dataset.paths rows — the same inputs lint-tasks already loads — so the linter that ships to users, not just an in-repo CE rule, should refuse them. (trigger: plugins/coder-eval/skills/lint-tasks/SKILL.md) (restates: Axis 2: split_field naming no key present in the rows makes --split a silent no-op)
  • 🟡 The new tasks/skills/ directory is not wired into either repo surface that inventories tasks. tasks/README.md calls itself "the map" and documents every sibling directory (agents/, datasets/, internal/, samples/, ...) including — for agents/ — precisely the caveat this suite needs ("several need a specific backend or extra ... so they are not in the CI smoke buckets"); it gained no skills/ entry and no note that the suite needs SKILL_SOURCE_PATH exported. And make run ("Run coder-eval on all tasks", Makefile:89-90) enumerates subdirectories explicitly as tasks/*.yaml tasks/agents/*.yaml and was not extended with tasks/skills/*.yaml — so either the target's "all tasks" claim is now false or the exclusion is deliberate and unrecorded. (trigger: tasks/skills/lint-tasks-activation.yaml)
  • 🔵 No cheap surface previews a split selection. coder-eval plan never expands datasets at all (it stops at load_task + resolve_task_for_variant — no dataset/row handling anywhere in plan_command.py) and gained no --split, so the verification optimize-skill Step 6 demands — "Check the resolved row count, not just the exit code" (SKILL.md:250) — is only obtainable from a paid run or a hand-written expand_dataset snippet like the one in pr-checks.yml. A plan --split <name> that printed the resolved row count per suite (or a run --dry-run) would make the mistyped-selector hazard visible for free. (trigger: src/coder_eval/cli/run_command.py)
  • 🔵 The skill's second track ships with no worked surface and nothing to dogfood it against. Tutorial 08 (535 new lines) explicitly walks the activation track only ("This page walks the activation track.", line 17), and the PR's in-tree dogfood suite tasks/skills/lint-tasks-activation.yaml is likewise activation-only — so the execution track (~160 lines of SKILL.md: Stage B's paired-comparison instrument, completion_rate parity, per-criterion regression rules) has neither a tutorial nor an outcome suite in the tree that has ever exercised it. At minimum, say in the tutorial or PLUGIN.md that the execution track is documented-but-unwalked, so a reader knows which half has been run for real. (trigger: docs/tutorials/08-optimizing-a-skill.md)

Tests:

  • 🟡 The out-of-tree scaffold assert — the one check that exercises the shipped template the way a user's copy sees it (pr-checks.yml:246-268, deliberately run with no source tree, no experiments/) — was not extended when the PR added split to that template. It still asserts only len(rows) == 6 and the expected_skill label set via expand_dataset(task, Path(".")) with no split= argument, so the headline user path this PR ships (copy template -> coder-eval run --split train) is verified nowhere end to end: a template shipped with absent, partial, or renamed split labels passes both that CI step and make lint. (trigger: plugins/coder-eval/reference/templates/activation-rows.jsonl) (restates: Axis 3: split label vocabulary is pinned on the prose side but not the data side)
  • 🔵 The resume drift guard is untested for the new field and has a hole for it. BatchRunConfig.split (config.py:89) is picked up by compute_run_fingerprint only because it dumps the whole config (batch.py:485), and no test in tests/test_resume.py asserts split participates in the drift warning. Worse, fingerprint_diff (batch.py:512-518) compares only keys present in the PRIOR stamp, so resuming a run directory stamped before this feature with --split test produces no warning at all and folds pooled prior results into a split subset's run.json. (trigger: src/coder_eval/orchestration/config.py)
  • 🔵 The exact invocation the new skill's Stage C prescribes — --split test --repeats 3 (SKILL.md:581) — has no test. Split filtering is covered alone (TestSplitFilter, lines 306-435) and replicate fan-out is covered alone (test_rows_fan_out_times_repeats, line 887), but nothing asserts row-filtering composes with replicate indexing (per-replicate run dirs, rows_total = repeats x filtered_rows in the pooled rollup), which is the combination every paid confirm run will use. (trigger: tests/test_dataset_expansion.py)

Display & mapping dicts:

  • 🔵 The hardcoded cause enumeration in the skipped-tasks banner (run_command.py:690-694, "load errors or skip: true") was not extended for the third cause this PR adds — a labelled dataset with no row in the requested split — so the one on-screen signal after a mistyped --split points the operator at broken YAML or a quarantined task instead of the selector they just typed. (trigger: src/coder_eval/cli/run_command.py) (restates: Axis 7: skipped-tasks banner enumerates only two of the now-three causes)

Harness & Lint Improvements

Static checks (lint / type):

  • [ce-lint] CE036 — a symbol-shaped token used inside a Markdown table in a shipped skill must be defined in prose outside any table in the same file. New whole-tree doc-surface rule (a @pytest.mark.lint class in tests/test_custom_lint.py, alongside CE026/CE033, not a BaseRule — it reasons over Markdown, not one .py AST). Implementation: for every plugins/coder-eval/**/*.md, extract inline-code spans, split on non-identifier chars, keep tokens matching ^[A-Za-z]_[A-Za-z0-9_]+$; the set used on lines starting with | must be a subset of the set appearing on non-table lines. VERIFIED by executing it on the tree at PR HEAD: exactly one hit — {M_holdout, M_tune} in plugins/coder-eval/skills/optimize-skill/SKILL.md — and zero false positives across every other bundled Markdown file, so migration cost is the fix itself. M_train/M_test are correctly seen as defined by the legend at SKILL.md:440. Prevents: The merged 4-axis finding at plugins/coder-eval/skills/optimize-skill/SKILL.md:444M_tune/M_holdout left undefined in the cost table by the tune/holdout→train/test rename (commit 2ebc522 renamed the legend line and missed the table body). That table is what the agent uses to project spend before each paid stage, so a stale symbol is a live cost-estimation surface. Also prevents the general rename-residue class in any bundled skill's legend/math prose, which no existing guard covers: the prose guard at tests/test_custom_lint.py:1614 asserts required tokens are PRESENT, never that used symbols are DEFINED.
  • [ce-lint] CE037 — a spelled-out enumeration count must match the number of top-level bullets that follow it. Same doc-surface lane as CE036. Implementation: a line matching ^\**(One|Two|…|Ten)\b[^.:]{0,70}:\**\s*$ opens an enumeration; count the immediately following top-level - /* /N. bullets (skipping indented continuation lines, blank lines and tables) and require equality. Scope plugins/coder-eval/**/*.md, docs/**/*.md, .claude/**/*.md. VERIFIED: the tightened form (the count word must START the line — the loose form false-positived on a prose sentence at docs/TASK_DEFINITION_GUIDE.md:1184) matches 8 enumeration headers across those trees and reports exactly one violation, the true positive. Zero migration cost. Prevents: optimize-skill/SKILL.md:163 — "Three requirements specific to this track:" followed by four bullets (165, 187, 191, 196). An agent executing a skill counts requirements to confirm it satisfied them, so a miscount invites treating the fourth ("Cover what already works, not just what is broken") as optional commentary. It also protects the correct enumerations in the same file ("Five facts" at 414, "Two rules" at 219) from going stale the next time a bullet is added.
  • [ce-lint] CE038 — no YAML key in tasks/** or plugins/coder-eval/reference/templates/*.yaml may set a value identical to its Pydantic field default. Whole-tree rule: yaml.safe_load the file, walk the top level against TaskDefinition.model_fields and the dataset: block against Dataset.model_fields, fail when field.get_default() == authored_value (skip None defaults). VERIFIED by executing the check: exactly two violations tree-wide — tasks/skills/lint-tasks-activation.yaml:47 and plugins/coder-eval/reference/templates/activation.yaml:51, both split_field: "split" — so the rule ships green after a two-line fix. It is the dual of CE031 (dead config = a field nothing reads; this = authored config that changes nothing). Prevents: The split_field: "split" finding — Dataset.split_field already defaults to "split" (models/tasks.py:261-262), so the key is a no-op that the skill's own instruction (optimize-skill/SKILL.md:217) tells agents to add unconditionally, and it has already been copied into four surfaces (template:51, tasks/skills/lint-tasks-activation.yaml:47, docs/tutorials/08-optimizing-a-skill.md:100) with the literal default value. It also implies the key is REQUIRED for --split to work, which is false. Caveat: the rule guards generated YAML only — the prose instruction that generates it must be reworded in the same change, or every new user suite re-adds the line and the rule fires on their tree instead of ours.
  • [ce-lint] CE039 — a dataset field-name knob must name a key that exists in the referenced rows. Whole-tree rule over tasks/**/*.yaml plus the shipped templates: resolve dataset.paths relative to the task file, load the rows, and assert split_field / stratify_field / id_field each name a key present in at least one row (and, for split_field, that labelling is all-or-nothing). Add min_length=1 to Dataset.split_field in the same change so the degenerate split_field: "" form is rejected at load. Prevents: The src/coder_eval/orchestration/task_loader.py:437 finding — a split_field naming no key present in the rows makes --split a SILENT no-op (reproduced: split_field='spilt' or '' with --split test returns both rows of a 2-row labelled dataset). In the optimize-skill / tutorial-08 workflow that means the train arm and the test arm are the same full-dataset run, reported as "confirmed on held-out data". Recorded boundary: this reaches in-repo suites only — a user's own JSONL is invisible to any lint pass, so it must be paired with harness item (D); and the min_length=1 half closes only the empty-string sub-case, never the typo.
  • [ce-lint] CE040 — every dataset-backed task under tasks/** must survive an expand_dataset smoke check. Extend the existing CE034-style tasks/ scanner (tests/test_custom_lint.py:2661), which today stops at load_task — as do the only two other whole-tree task scanners (test_tags.py:133, test_yaml_migration.py:25). Clauses per suite: expand_dataset succeeds; no ${row.…} placeholder survives substitution; row ids are unique; split labelling is all-or-nothing; a labelled suite carries both polarities on every split. Then parametrize the five template-pinned guards (tests/test_custom_lint.py:1270, 1287, 1382, 1404, 1416) over ALL jsonl/yaml pairs instead of self.TEMPLATES alone. Prevents: Two findings. (1) tests/test_custom_lint.py:1416 — the repo's own new paid-dogfood suite (tasks/skills/lint-tasks-activation.yaml + 28 rows) gets NONE of the guards the shipped template gets; mutation-proven twice (deleting "split": "train" from the pos-1 row, and separately breaking both dataset.paths and a ${row.prompt} placeholder, each left the suite green). (2) The data half of the label-vocabulary finding. The duplicate-id clause comes free: _reject_duplicate_row_ids (task_loader.py:313) already enforces it in the loader, it is simply never exercised for tasks/.
  • [ce-lint] CE041 — the split labels shipped in the template rows must equal the --split <name> tokens the shipped skills and tutorial hardcode. Derive the expected label set by parsing --split (\w+) out of plugins/coder-eval/skills/optimize-skill/SKILL.md (6 invocations: lines 247, 466, 495, 496, 497, 548) and check-skill/SKILL.md:159,162, then assert {r["split"] for r in activation-rows.jsonl} == that set. Deriving beats asserting the literal {"train","test"} because it makes the two sides unable to move independently. Prevents: tests/test_custom_lint.py:1437 — the new guard is deliberately value-agnostic (assert len(by_split) >= 2) while the PROSE side IS pinned (tests/test_custom_lint.py:1636-1638 asserts the literal --split train / --split test tokens). Reproduced: relabelling the shipped template rows to fit/eval leaves make lint fully green (344 passed) while every documented command still says train/test; a verbatim template copy then yields "no rows in split 'train'" → demoted to skipped_tasks → the zero-row green run of the merged high finding. This PR's own diff performed exactly that rename by hand, with no mechanical guard on either side.
  • [ce-lint] CE042 — no except Exception in src/coder_eval/orchestration/ whose handler raises a DIFFERENT exception type. AST rule in tests/lint/rules/, wired into tests/lint/runner.py: flag an ExceptHandler whose type is bare Exception and whose body contains raise <OtherType>(...). The fix at each site is to narrow to the consumer's own tuple — except (ValueError, FileNotFoundError, OSError, yaml.YAMLError), mirroring experiment.py:637 (pydantic v2's ValidationError is already a ValueError subclass, so listing it is redundant). VERIFIED cost: 8 handlers of this shape tree-wide, exactly 2 inside orchestration/ (task_loader.py:68 and experiment.py:123, the same laundering class), so the scoped rule ships after two edits. Log-and-continue handlers (early_stop.py:401/547, batch.py:201/263) are untouched by the raise clause. Prevents: The src/coder_eval/orchestration/task_loader.py:68 finding — except Exception around the four path resolvers relabels AttributeError/TypeError/ImportError as ValueError, which then matches the consumer's tuple at experiment.py:637 and is demoted to a SkippedTask, defeating the narrow-catch contract that consumer documents five lines above it ("we deliberately don't catch AttributeError / TypeError / ImportError — those signal a regression … and should crash loudly"). Combined with the un-reachable all-skipped guard (experiment.py:725 can never fire for load failures, since attempted += 1 is at line 652, after the handler), a first-party regression in path resolution yields a green exit-0 zero-task run instead of a crash.
  • [ce-lint] CE043 — a module-level helper must take the narrowed type, not re-narrow a parameter attribute with assert. AST rule: flag a module-level FunctionDef (methods asserting on self are exempt — that shape is genuinely unavoidable, cf. the six in sandbox.py) whose first statements include assert <param>.<attr> is not None. VERIFIED cost: 3 hits tree-wide — task_loader.py:321, task_loader.py:282, experiment.py:502 — so it is adoptable with at most one # noqa debt marker. Prevents: The src/coder_eval/orchestration/task_loader.py:313 finding: _reject_duplicate_row_ids takes TaskDefinition and re-narrows with assert task.dataset is not None (line 321) purely to reach task.dataset.id_field, while its sibling one function up (_load_dataset_rows, line 276) already models it correctly by taking the narrowed Dataset. The assert is stripped by python -O, so the precondition is expressible in the type but enforced at runtime instead. Moving to (rows, dataset, task_id) also removes the coupling flagged in the complexity finding.
  • [ce-lint] CE044 — a dataset-backed task gating classification metrics must cap its run time. Whole-tree rule over tasks/** plus plugins/coder-eval/reference/templates/*.yaml: a task whose suite_thresholds name a classification metric (recall.*, precision.*, f1.*, accuracy) must set run_limits.task_timeout, and should gate completion_rate. Mechanism the rule encodes: reports.py:849-851 slices row.result.success_criteria_results[i] only if i < len(...), so a row whose criteria never ran (the task-timeout watchdog cancel at orchestrator.py:492-540, plus crash paths) silently leaves per_rows — documented as rows_excluded in .claude/shared/run-layout.md:34-36. Prevents: The plugins/coder-eval/reference/templates/activation.yaml:41 finding — the published template every check-skill-generated suite is copied from ships NO run_limits, while the in-tree suite this PR added caps every row (max_turns: 2, turn_timeout: 120, task_timeout: 300) and explains in its own comment why the caps are load-bearing for the metric denominator. Generated suites therefore inherit the PACKAGED default_experiment.yaml's task_timeout: 600 (so wheel-installed users too, not just this repo) and gate recall.yes/precision.yes at 0.7 over a denominator timeouts can erode with nothing failing. Note the raised max_turns: 20 is a cost multiplier, not a denominator risk — MAX_TURNS_EXHAUSTED still scores the row; only the timeout path skips criteria, so task_timeout is the clause that matters.
  • [ce-lint] CE045 — tests must not hand-roll a reader for a bundled template/dataset file. Rule over tests/**: a path expression reaching plugins/coder-eval/reference/templates/ must come through the single shared fixture/helper (e.g. a _template_rows() on TestPluginArtifacts, which already carries self.TEMPLATES), not an inline json.loads(... .read_text(...).splitlines()). Recorded boundary on the ruff alternatives so this is a deliberate choice, not an omission: ruff ships no adoptable duplicate-code rule, and PLC0415 (import-outside-top-level — which WOULD have flagged the two redundant function-local import json) is unadoptable here, measured at 983 hits in tests/ and 106 in src/, and CE017 positively REQUIRES lazy agent imports in models/. Prevents: tests/test_custom_lint.py:1422 — the PR's new test_activation_rows_split_both_polarities_both_sides (line 1416) copies the 6-line JSONL loader byte-for-byte from the adjacent test_activation_rows_have_both_polarities (1405-1411), including a redundant function-local import json in each (the file's only two json. uses in ~2900 lines). It is the second raw reader of that file, so a third would copy it again. Forcing the shared helper also nudges the parametrization CE040 asks for, since a fixture generalizes over jsonl/yaml pairs where an inlined literal path cannot.
  • [ruff] Enable C901 (mccabe) alongside the existing PLR0915/PLR0912 size gates, and add a checked-in per-function complexity ratchet. MEASURED on the current tree: lint.mccabe.max-complexity = 30 → 0 violations (free to adopt today); = 18 → 5 (claude_code_agent.communicate 21, codex_agent._setup_skills 19, docker_runner._build_argv 29, orchestrator._simulation_dialog_loop 22, reports_experiment.generate_variant_report 24), each absorbable by the # noqa debt-marker policy pyproject.toml:188-192 already documents for function size. HONEST LIMIT, recorded rather than glossed: ruff's mccabe scores expand_dataset at 13 (radon cc -s says D(22) — radon counts boolean operators and comprehensions, mccabe does not), so C901 at ANY tree-passing threshold would NOT have fired on this PR. The form that would is a ratchet: a generated tests/lint/complexity_baseline.json of per-function radon scores plus a rule failing when a function exceeds its recorded value, refreshed by make complexity-baseline (same generated-surface pattern as make plugin-reference/CE033). That converts "the repo enforces no complexity gate" into "no function may get worse", at zero migration cost for the ~20 functions already at D or worse. Prevents: The merged A1/A5 finding at src/coder_eval/orchestration/task_loader.py:361expand_dataset went from C (15) on main to D (22) at PR HEAD by inlining the --split filter (lines 431-446) that the module's own idiom says should be a helper (_load_dataset_rows, _stratified_sample, and this PR's own _reject_duplicate_row_ids are all extracted). The ratchet catches it; the C901 threshold catches only the next unbounded-growth case. It also fires on the grouped comment/altitude nit at task_loader.py:422, which shares the root cause.
  • [pyright] Stop letting an unsatisfiable selector value travel the whole chain; narrow at the CLI boundary and type the selector into the run record. (a) --split is declared str | None at cli/run_command.py:300 and threaded unchanged through BatchRunConfig.split (orchestration/config.py:89) into expand_dataset, but task_loader.py:437 defines a row as UNLABELLED precisely when its value is None or "" — so "" is unsatisfiable by construction (verified: --split "" raises "has no rows in split ''", is caught into skipped_tasks, and exits 0). Reject it in a Typer callback (typer.BadParameter("--split requires a non-empty name")) or normalize split or None before building BatchRunConfig, and give Dataset.split_field min_length=1. (b) Echo split / max_rows / sample_per_stratum onto RunSummary (models/results.py:992-1058 already echoes max_parallel at line 1044 as precedent) and SuiteRollup, so the row-selection knobs are typed members of the consumer contract instead of being reachable only via resume_fingerprint.json (batch.py:485) — a file named for --resume and absent from .claude/shared/run-layout.md and its plugin mirror. Prevents: Directly: the src/coder_eval/cli/run_command.py:300 finding (--split "" unsatisfiable by construction) and the src/coder_eval/orchestration/config.py:89 finding (selector absent from run.json/suite.json, so a --split test rollup is indistinguishable from a full-suite one). Indirectly it is the enabling half of harness item (D): once split is on RunSummary, an inert-selector or zero-row run is attributable after the fact instead of leaving no artifact trace. Recorded boundary: no type system can catch the MAIN half of the split_field finding — distinguishing "field-name typo" from "dataset genuinely has no labels" needs the user's row data, since rows are dict[str, Any]; that half needs CE039 plus the runtime warning.

Harness improvements (not statically reachable):

  • Mutation-kill gate on the CLI/orchestration plumbing seam. Add a make mutate-diff target (and a PR CI job) that, for each changed line in src/coder_eval/cli/ and src/coder_eval/orchestration/, nulls keyword-argument values one at a time and asserts at least one test fails. Cheaper first step: a curated kill-list (tests/mutants.yaml) covering the high-traffic seams — BatchRunConfig(...) construction, _run_all_tasks(...) forwarding, the exit gate — checked by one -m slow test. Why not static: No AST pass can distinguish live plumbing from dead plumbing that happens to be spelled correctly; proving a line is OBSERVED by a test requires executing the suite with that line mutated. The obvious static substitute was measured and rejected: a "every typer.Option flag name appears somewhere in tests/" grep PASSES for --split today, because the string occurs in test docstrings and comments (tests/test_dataset_expansion.py:288, 315, 397, 411, 424) — it would have been a false green on exactly this finding. (A separate genuinely useful AST rule — every typer.Option parameter of a CLI command must be forwarded to some call — catches the adjacent DEAD-flag class but not this one; 5 of 26 flags are currently never named in tests at all: --exclude-tags, --input, --log-file, --tags, --task-dir.) Prevents: The high finding at tests/test_dataset_expansion.py:742 half (a): mutating split=split,split=None, at src/coder_eval/cli/run_command.py:500 leaves the whole suite green (4301 passed, 14 skipped — reproduced independently), i.e. the entire --split flag can be dead while make verify passes, with pooled train+test P/R/F1 reported as if measured on one half.
  • A CLI-layer test for every row-selection selector. Call _run_all_tasks(..., split="test") — the seam the repo already pins this way in tests/test_run_command_junit.py:45, tests/test_cli_telemetry.py:93 and tests/test_cli_empty_glob.py:44 — and assert the resolved/run task ids, not just the resolver's return value. Do the same for --sample, --sample-per-stratum, and --tags/--exclude-tags. Why not static: The property under test is which rows actually entered the run, which only exists after the resolver plus batch have executed. A lint rule can see that the kwarg is passed but not that passing it changed the row set. Prevents: The same high finding from the other direction, plus the untested-plumbing class for the other three selectors, whose deepest current coverage is BatchRunConfig(run_dir=..., split="train") handed straight to resolve_all_tasks (tests/test_dataset_expansion.py:690, 732, 756).
  • Exit-code matrix test for every "nothing ran" cause, plus the narrow selector-scoped fix. One table-driven test enumerating: zero task files / all skip: true / tag filter matched nothing / --split matched no labelled row / --split "" / every task demoted by a load error — each with its expected exit code AND expected banner text. Then implement the narrow fix already scoped in .claude/harness-candidates.md:320-333: fail non-zero when a CLI SELECTOR (--split, --tags) eliminated everything, which is unambiguously user error rather than repo state, leaving skip: true semantics untouched. While there, fix the banner at run_command.py:690-694, whose parenthetical still names only two of the now-three causes. Why not static: Exit semantics are runtime behavior, and the load-bearing part is a POLICY decision — which flavour of emptiness should be fatal — that no lint rule can make. The asymmetry is the tell: zero task FILES is already fatal (cli/run_helpers.py:92-94, pinned by tests/test_cli_empty_glob.py:15) while zero RESOLVED tasks is green (cli/run_command.py:559-561); same user-visible outcome, two exit codes, and only one of them has a test. Prevents: The merged A6/A7/A8 high finding — reproduced end to end: coder-eval run tasks/skills/lint-tasks-activation.yaml --split trian prints "1 task file(s) skipped" / "Running 0 task(s)" and EXITS 0. All six coder-eval run invocations in optimize-skill/SKILL.md pass --split, so a one-character typo reads as "the stage ran and nothing regressed". Also covers half (b) of the test-health high finding (the exit-0 consequence is unasserted at tests/test_dataset_expansion.py:758-761) and the uncovered banner branch at run_command.py:691.
  • Make an inert selector observable at runtime, and record it in the run artifacts. (a) When split is not None and a dataset-backed task takes the unlabelled fallback (task_loader.py:446), emit logger.warning("--split %r: task %r has no row carrying split_field %r; running all %d rows", ...) — note task_loader.py imports no logging at all today, so this is a new seam rather than a missing call. (b) Once split is echoed onto RunSummary/SuiteRollup (see the pyright static entry), add a golden-run assertion that suite.json carries the selector, and document the new fields (or resume_fingerprint.json) in .claude/shared/run-layout.md plus its plugin mirror. (c) Add the missing OVER-count instruction to optimize-skill Step 6: the existing guidance (SKILL.md:250-253, 476-481) warns only about a row count that is too LOW or zero, so a full-dataset run under --split test is named by no surface. Why not static: The inert case depends on the USER's row data at runtime — CE039 can only see in-repo suites, and no static pass can read a customer's JSONL. The artifact half is a property of a written file observed after a real run, not of the source. Prevents: The src/coder_eval/orchestration/task_loader.py:437 finding (silent full-dataset run under every split name — the failure that makes tutorial-08's two arms identical while being reported as "confirmed on held-out data") and the src/coder_eval/orchestration/config.py:89 finding (selector unrecoverable from suite.json, so a run made inert this way leaves no trace distinguishing it from a full run).
  • Denominator-integrity assertion on the dogfood classification suites. After the nightly/activation rollup, assert rows_excluded == 0 (equivalently completion_rate == 1.0) for any suite gating recall.*/precision.*, and add completion_rate: 1.0 to the shipped template's suite_thresholds so a copied suite inherits the gate rather than the hazard. Copy the in-tree suite's explanatory comment rather than its exact max_turns: 2, which risks false-negative recall for a skill that activates only after an exploratory tool call. Why not static: Whether rows were actually excluded is a property of a real run's rollup — CE044 can assert only that a CAP exists, never that no row hit it. The two are complementary: the lint rule bounds the exposure, this assertion detects when it materialized. Prevents: The live half of the plugins/coder-eval/reference/templates/activation.yaml:41 finding — a row cancelled by the task-timeout watchdog never reaches check_all_async, so reports.py:849-851 drops it from per_rows and the 0.7 recall/precision gates are computed over a shrunken denominator with nothing failing. The published surfaces offer no protection today: check-skill's SKILL.md contains no run_limits guidance at all, and optimize-skill only tells the user to inspect completion_rate/rows_excluded after the fact.
  • Recorded boundary — the findings static analysis deliberately cannot reach, so the omission is a decision rather than a gap. (i) The dead-assertion nit at tests/test_custom_lint.py:1361/1377: assert A and B where B is logically implied by A needs semantic reasoning, not an AST shape. Its sibling half IS mechanizable if it ever recurs — "the string you test membership of must be the string you .split() on" (if TOK in line or TOK in line.replace('"','') followed by line.split(TOK, 1)[1], which raises IndexError instead of the intended assertion message) — but a rule for a one-off in test code is over-fitting; fix by hand. (ii) The SKILL.md size finding is PARTLY mechanizable: add a SKILL_BODY_BUDGET_LINES cap (~400) with a declared per-skill exemption dict to the derived skill test that already owns SKILL_LISTING_BUDGET_CHARS (tests/test_custom_lint.py:1234) — measured 626 lines / 5,647 words versus 330 for the next largest, and the plugin already has the progressive-disclosure mechanism (${CLAUDE_PLUGIN_ROOT}/reference/*.md, used at lines 39, 60, 194, 263) — but WHICH sections to demote (Step 10's gate reasoning at 437-596, Step 8's snapshot rules at 311-385) is editorial judgment a budget cannot make. (iii) This finding set contains no security findings, so the bandit / pip-audit / CodeQL lane has nothing to enforce here. Why not static: Each sub-item names its own reason: implication between two boolean conjuncts is undecidable by pattern; a size budget can gate growth but cannot choose the split point; and the security lane has no finding to gate against. Prevents: The A3 low finding (dead assertion clauses in the new reachability guard) and the A1 low finding (SKILL.md size / no use of the reference/ progressive-disclosure pattern) — recorded as reviewer-fixed and budget-gated respectively, rather than silently dropped.

Top 5 Priority Actions

  1. Fail non-zero when a CLI selector (--split, --tags) eliminated every task — today src/coder_eval/cli/run_command.py:560 gates only on executed-task outcomes, so --split trian prints one yellow line and exits 0 while zero task files is fatal at src/coder_eval/cli/run_helpers.py:93, the same "nothing ran" outcome with two different exit codes.
  2. Make the inert-filter case observable at src/coder_eval/orchestration/task_loader.py:437: when --split is requested and no row carries split_field, log a warning naming the field and row count (verified: split_field='spilt' or '' silently returns the whole dataset under both --split train and --split test, so both arms of an optimize-skill run are identical full-dataset runs reported as held-out confirmation).
  3. Pin the --split seam in tests: add a _run_all_tasks(..., split="test") case (nulling split=split at src/coder_eval/cli/run_command.py:500 today leaves 4301 tests green), assert the mistyped-split exit-0/skipped_tasks consequence beyond the resolver-level tests/test_dataset_expansion.py:742, and pin the shipped label vocabulary at tests/test_custom_lint.py:1437 (>= 2 splits accepts any names, so renaming the template's train/test keeps all 344 lint tests green while every documented command hardcodes the old names).
  4. Narrow the blanket except Exception at src/coder_eval/orchestration/task_loader.py:68 to (ValueError, FileNotFoundError, OSError, yaml.YAMLError) — it relabels AttributeError/TypeError from the four path resolvers as ValueError, defeating the narrow-catch contract documented at src/coder_eval/orchestration/experiment.py:637 and demoting a first-party regression to a SkippedTask in an exit-0 zero-task run.
  5. Fix the two published plugin surfaces users copy: add a run_limits block (plus a completion_rate threshold) to plugins/coder-eval/reference/templates/activation.yaml:41, since generated suites inherit the packaged task_timeout: 600 and a timed-out row is excluded from the confusion matrix rather than scored (src/coder_eval/reports.py:849), and rename the undefined M_tune/M_holdout symbols in the pre-spend cost table at plugins/coder-eval/skills/optimize-skill/SKILL.md:444.

Stats: 0 🔴 · 2 🟠 · 8 🟡 · 9 🔵 across 8 axes reviewed.

uipreliga and others added 11 commits August 13, 2026 08:41
…table

The execution track's instructions produced silently-wrong measurements. Every
fix here is prose; no evaluation behaviour changes.

- The outcome suite must be ONE dataset-backed task, one row per scenario.
  suite.json is written only for tasks the dataset expander touched (rollups
  group on suite_id), and --split filters dataset ROWS — so a directory of
  separate task files gives Stage A no rollup to rank and makes Stage C's
  `--split test` silently re-run the train rows.
- Two consequences that decide how rows are written: criteria are copied to
  every row with ${row.*} substituted into every string leaf (so per-scenario
  assertions are parameterized, never written per scenario), and substitution
  never reaches sandbox.template_sources — every row shares ONE fixture, so
  variation lives in the prompt and the fixture must clear the skill's own hard
  stops or every arm ties at zero.
- Step 4's worked example described a repo with no .github/, which is exactly
  the hard stop `ci` refuses on; it now shows a repo that clears the
  precondition and names its output path.
- There is no --variant flag, so the arm set changes by authoring a file per
  stage: round<N>-triage/gate/confirm.yaml. Re-passing the triage file at
  Stage B/C costs (N+1)/2x the budgeted runs and renders no Paired Comparison
  block at all.
- The paired mean diff subtracts in variant declaration order, so with
  incumbent declared first a candidate win reads NEGATIVE. Stated in both
  Stage B and Stage C.
- Pin the tool policy under `defaults: agent:` and size the allowlist to the
  union of every arm's tools, or a candidate whose hypothesis is "use a
  different tool" is scored on the prohibition.
- Name the per-row max_usd brake, and say plainly that an activation suite's
  tight caps must not be carried over — a truncated outcome row scores as a
  body failure that never happened.
- Cost table: M_tune/M_holdout -> M_train/M_test, matching the split rename.

The deletion sensor gains one entry per new instruction, each with its why,
plus a counted assertion that the sign rule survives in BOTH stages and a
guard that the stale cost-table symbols stay gone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… track

optimize-skill's execution track had no artifact to hand over: Step 4 pointed
users at /coder-eval:task with a list of requirements attached, which came back
half-applied. It now hands over a template, the way the activation track has
always handed over activation.yaml.

outcome.yaml is the execution twin of activation.yaml and carries the two
constraints that are expensive to discover by running:

- ONE fixture serves every row (${row.*} never reaches sandbox:), so scenario
  variation lives in the prompt and the fixture must clear whatever
  precondition the skill checks before it will act — else every arm ties at
  the floor and reads as three bad candidates.
- Criteria are copied to every row, so per-scenario assertions are
  parameterized by row fields (path: "${row.expected_path}").

Its run_limits are deliberately unlike an activation suite's: an outcome row is
a full task run, so max_turns is 20 rather than 2 and max_usd is the per-row
brake. The run_command slot ships commented out — file_check already asserts
what it would, and a row-supplied shell command would drag quoting and
sandbox-tool-availability into every copy.

Tests are loader-backed, mirroring the activation template's. The structural
contract lives in ONE shared helper because Phase 4's checked-in sample asserts
the same four properties; the threshold vocabulary is the union of a real
aggregate() call and a real _attach_row_accounting() call, since completion_rate
comes from the latter and a check against aggregate() alone would fail on the
very template this ships.

Also closes a harness gap found reviewing 1/8: nothing verified that a
${CLAUDE_PLUGIN_ROOT}/... pointer in a skill resolves to a file that exists, so
the pointer at this template shipped one commit before the template did, past
344 green lint tests. Now 28 such references are checked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…issed them

docs/PLUGIN.md said "All six skills read it" while seven ship, and both READMEs
named three of the four skills that shell out to the CLI. The count sensor was
supposed to catch the first and did not: it substring-matched raw file text, and
the offending line reads "All six\n  skills read it", so the newline sat exactly
between the two words the check needed adjacent. 91 lint tests passed over it.

- docs/PLUGIN.md: six -> seven; both READMEs name all four members of
  SKILLS_REQUIRING_THE_CLI in the install paragraph; both `templates/` blurbs
  now name the outcome suite alongside the activation suite.
- Extract `_normalized()` and read every prose surface through it. The idiom
  was already hand-copied at three sites, all now converted.
- Extract the count matcher into `_wrong_skill_count_offenders` so its
  wrapped-phrase self-test can run the REAL matcher against a built file.
  Asserting only that `_normalized()` collapses whitespace left the sensor free
  to be reverted with every test still green — mutation-checked: reverting it
  now fails the self-test, and previously did not.
- New `test_cli_driving_skills_are_named_in_the_install_prose`, derived from
  SKILLS_REQUIRING_THE_CLI with no names in the assertion, so a fifth
  CLI-driving skill cannot ship with the prose still listing four. Scoped to the
  install paragraph, since every skill name appears somewhere in both files.

Note the strengthened sensor immediately caught this commit's own first draft
("the four skills that shell out to the CLI"). Reworded rather than exempted, as
the sensor is deliberately blunt about any count word next to "skills".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…orked example

Stands to the content track as tasks/skills/lint-tasks-activation.yaml stands to
the trigger track: a real, commented, runnable suite built from the bundled
outcome template. `ci` is the subject because it sets
disable-model-invocation: true (so the rows MUST use the slash form, dogfooding
the mechanism), its output is a real workflow file, and it needs no extra
credentials in the sandbox.

10 rows, 6 train / 4 test. Because row substitution never reaches `sandbox:`,
all ten run against ONE fixture, so each row is a different REQUEST against the
same repository and each names its own output path — which takes the agent's
filename choice out of the measurement without hinting at the content graded.

The fixture is not scenery. It carries `.github/workflows/lint.yml` because `ci`
stops outright on a repo with no `.github/` (an empty fixture ties every arm at
zero and reads as three bad candidates), and that workflow avoids the string
`coder_eval` or `ci` takes its don't-clobber branch instead. Its eval tree sits
at `evals/` rather than `tasks/`, at two depths, with one task interpolating
$SKILL_SOURCE_PATH, one experiment and a version pin — each making one
load-bearing body rule observable rather than unreachable.

The fixture lives under templates/, NOT tasks/ as drafted: tasks/**/*.yaml is
swept recursively by CE034, test_yaml_migration, test_tags and the CLI's own
default task discovery, all of which load every match as a TaskDefinition — and
the fixture must contain an experiment file. templates/ is the repo's existing
home for sandbox template dirs and is swept by nothing.

One row was smoke-run for real ($0.43, SUCCESS, skill_triggered observed 'yes',
file_check resolving a file the agent actually wrote) and it paid for itself
twice:

- max_usd was drafted at 0.50, which the measured row came within 15% of. A
  slightly longer row would have aborted COST_BUDGET_EXCEEDED and scored as a
  body failure that never happened. Raised to 2.00.
- The emitted workflow invented `anthropics/coder-eval-action@v1` and still
  scored 1.0. The real action reference is now asserted as its OWN criterion
  rather than a second `includes` entry: file_check scores found/total, so
  folding a constant sub-check into the gated criterion would put a fixed
  contribution in every row of every arm and quietly weaken `mean: 0.7` on the
  one dimension that discriminates.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
optimize-skill calls a partly-labelled dataset "the dangerous state, because it
does not look like one", and it is right: --split keeps the rows whose label
matches 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. Nothing in the output says how many rows went missing. That is
mechanically detectable, so per CLAUDE.md it becomes a rule rather than a
paragraph.

- Extract `row_split_label()` in task_loader.py as the single definition of the
  SPLIT-FILTER convention, and rewrite expand_dataset's inline predicate to use
  it in one pass. Behaviour-preserving: the old `x not in (None, "")` and the
  new `x is None or x == ""` were checked over None/""/0/0.0/False/True/[]/{}/
  b""/"0"/" "/"None"/missing-key and agree on every one, so a falsy 0 remains a
  real label compared via str().
- _stratified_sample keeps its DIFFERENT convention on purpose (it folds a
  missing key into the "" stratum, which turns an explicit None into "None") and
  now carries a comment naming the divergence — the old comment read as though
  the two agreed.
- CE035 as a @pytest.mark.lint class, mirroring CE034: every tasks/**/*.yaml
  with a dataset: block must label every row or none. Both legal states pass;
  only the mixture is a finding. It keys on the CONFIGURED split_field, not the
  literal "split", and covers inline rows as well as paths. Four dataset-backed
  repo tasks are exercised for real, not skipped.

Also closes a second definition of "labelled" that had survived in the harness:
the two template row tests used truthiness (`r.get("split")`), which would have
reported a legitimate `"split": 0` as unlabelled — the exact disagreement this
phase exists to remove.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rects the

 slash-form claim it disproved

Ran the execution track against `ci` for real. Four baselines on --split train,
~24 runs, ~$9. Stages A/B/C were NOT spent: the round never cleared its own
wiring check, and the method says a baseline that cannot be trusted is a stop,
not a starting point. Full ledger in .optimize-skill/ci/history.json.

Engagement never reached 1.0: 4/6, 4/5, 3/6, 4/6 across four runs, failing on
DIFFERENT rows each time. Three silent slips, all observed:
  - the model answers the slash command by dispatching a sub-agent, which reads
    the skill in the child so no Skill call reaches the parent stream;
  - it ignores the command and does the work itself, emitting no Skill call;
  - the scenario's wording routes it to a sibling — "so we find out if a skill
    quietly stops triggering" sent one row to check-skill instead of ci.
At that rate 20-50% of every arm would measure the absence of the thing under
test, and Stage B's own promotion rule ("the skill actually engaged on every
scored row") could not have been satisfied. Cost avoided: ~84 runs, ~$36.

On rows where `ci` DID engage the weighted score is 1.000 — a ceiling. The
emitted workflows carry the per-depth globs with their rationale, the real
UiPath/coder_eval action, the version pin, extra-args, both runtime prerequisite
steps and both hardening lines. So both no-go conditions hold at once.

The round's real output is a correction to shipped guidance. SKILL.md and the
bundled template both asserted the slash form "loads it, emits a real Skill tool
call, and is detected by skill_triggered" — presented as reliable. Measured at
50-80%. Both surfaces now say so, name the three failure modes, and make
engagement a GATE on the baseline rather than an afterthought. They also record
that skill_triggered counts READING the skill's SKILL.md as engagement, not only
a Skill call — one row reported engaged while the command it issued named a
different skill.

Suite and template fixes the run paid for:
  - `Skill` was missing from allowed_tools though it is the mechanism under
    test; it worked anyway, which is why the omission was invisible.
  - disallowed_tools: [Agent, Task] — an allowlist cannot suppress delegation,
    since those stay available whatever allowed_tools says. This one setting
    moved engaged-row scores from 0.333 to 1.000: with delegation on, the
    sub-agent did the work WITHOUT the skill and every row scored engagement
    only. The tool policy was not a detail, it decided the result.
  - turn_timeout 300 -> 900: 300 turned a row into an ERROR, which is excluded
    from the aggregate and surfaces only as completion_rate < 1.0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a null

Written from the run in 5/8, which produced no promotion. Tutorial 08 already
tells a "measure first, then decline to spend" story on the activation track;
this is its execution-track counterpart, and the reason it stopped is more
useful than a promotion would have been.

The page covers the outcome suite's shape (one dataset-backed task, rows as
scenarios), the one-fixture rule and why the fixture must clear the skill's own
preconditions, criteria copied per row with row-parameterized assertions, split
labelling, reachability, and the three checks to run before reading any score.
Then the go/no-go, which was no-go on two grounds at once: engagement never
reached 1.0 (4/6, 4/5, 3/6, 4/6 across four runs, failing on different rows
each time), and where `ci` did engage the score was already 1.000.

The sharpest exhibit is the same suite either side of one setting. With
sub-agent delegation available, every row scored 0.333 and the emitted workflows
named an action that does not exist, used the recursive glob the body forbids,
and misspelled the score-floor input. Denying delegation moved engaged rows to
1.000. The tool policy was not a detail; it decided the result.

It also carries the parts of the mechanics the round never reached but a reader
still needs: the snapshot layout as a full plugin root (from the real incumbent
snapshot, including why plugin.json is a trap), the per-stage experiment files,
and the paired mean diff's sign — it subtracts in variant declaration order, so
with incumbent declared first a candidate win reads negative.

Every figure traces to a preserved artifact; the cost line is the measured
$7.47, not an estimate. Two lint rules caught the draft and both were answered
by rewriting rather than exempting: the recursive-glob guard (which is blunt on
purpose, since integrators copy-paste out of docs) and CE026's runtime-prereq
check on the emitted-workflow excerpt.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…le it

The page reported ~350 runs of A/B results without ever showing how the arms
were wired: no snapshot layout, no experiment_id, no variants block, no `-e`
invocation. A reader could follow the reasoning and not reproduce any of it.

Adds, for the `analyze` round it already describes:
  - the .optimize-skill/analyze/1-<slug>/ snapshot tree, with the siblings and
    .claude-plugin/plugin.json, and why each is load-bearing — without the
    manifest the arms compete under different namespaces, and since a variant's
    plugins block REPLACES the task's, a snapshot missing its siblings makes
    every sibling criterion observe `no` in every arm;
  - the experiment YAML that mounts each snapshot by absolute path;
  - the exact command lines for Stages A, B and C, plus the rule that no flag
    selects a subset of an experiment's variants, so each stage needs its own
    file.

Restructures into Part 1 (`lint-tasks`, a ceiling result and when to stop) and
Part 2 (`analyze`, a full A/B that promotes), with the shared machinery under
Reference. `lint-tasks`'s test-split confirmation previously sat between
`analyze`'s Stage B and Stage C, so the page alternated subjects mid-argument;
it now closes Part 1. Every finding survives verbatim — the recall-0.0 wiring
trap, the 2-in-3 misfire, the billing-limit p = 0.038 artifact, and the
uninformative test split.

Now that the commands are shown, collapsing Stage B's three invocations into
`--repeats 3` is a one-line edit, so a sensor pins it: three distinct --run-dir
values, no --repeats in the fenced commands, and the warning itself retained.
Scoped to the code fences, since the section legitimately says "not --repeats 3"
in prose.

Also corrects "the other six skills" to seven — tutorials are not in
SKILL_DOC_SURFACES, so no count sensor covered that line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cross-phase review (two external models plus an Opus reader over the whole
diff) found three defects living in the seams between phases, where no
single-phase review could see them.

HIGH — the tool-policy instruction was shadowed by the artifacts shipped
alongside it. SKILL.md said to declare allowed_tools/permission_mode in the
experiment's `defaults: agent:`, but those fields merge by REPLACE and the task
layer outranks experiment defaults — so the template and the sample, both of
which declare them on the task, silently override it. Following the instruction
was a no-op, and it broke precisely the case the paragraph exists to enable:
widening the allowlist so a candidate whose hypothesis is "use a different tool"
is scored on the instruction rather than the prohibition. All three surfaces now
say to declare it on the suite, and note that a variant can still override
deliberately.

HIGH — four of the ten scenarios in the checked-in sample stated the behaviour
they graded ("listed explicitly rather than with a recursive wildcard" scoring
the explicit glob; naming the skill-source requirement and scoring its
passthrough). An arm whose body DELETED that rule would still have scored 1.0,
because the prompt supplied it — the answer-leak this repo's own task rubric and
lint-tasks flag. Rewritten to describe the situation and leave the method to the
body. Tutorial 09 now carries the caveat that its reported figures predate the
fix, which makes its ceiling reading softer rather than firmer.

MEDIUM — the template still shipped max_usd: 0.50, the cap a measured $0.43 row
came within 15% of, while the sample had been raised to 2.00 from that same
evidence; a comment claiming allowed_tools is a claude-code field when it is on
BaseAgentConfig (only setting_sources needs the discriminator); SKILL.md listing
"keep Skill in allowed_tools" as an engagement mitigation when tutorial 09
measured that it works either way; and SKILL.md conflating the two cap failures
— max_turns exhaustion scores low (a fabricated body failure) while a timeout
ERRORS and is excluded, surfacing only as completion_rate < 1.0.

Test harness: the fixture depth assertion counted evals/experiments/ as a depth,
so deleting the file it was entirely about still passed (mutation-verified fixed);
_normalized's docstring asserted a repo-wide invariant eight sensors do not
follow; _outcome_metric_vocabulary was documented as universal while derived from
file_check alone, which would have failed the next gate the docs ask for.

Also records why the fixture lives under templates/ rather than beside its suite,
and names reference/templates/ in CLAUDE.md's plugin inventory.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… grade

Closes the harness gap behind one of the final review's High findings. Four
scenarios in the checked-in outcome sample stated the behaviour they graded, so
an A/B arm whose body DELETED that rule would still have scored 1.0. This repo's
task rubric and the `lint-tasks` skill both call that out — for a USER's files.
Nothing applied it to this repository's own, so it shipped.

Guards the verbatim form: a criterion's content-asserting strings must not appear
in the prompt the row expands to. Location fields (path, agent_file, command) are
exempt, and that distinction is the rule — a prompt MAY say where to write, which
takes the agent's filename choice out of the measurement without revealing
anything graded; it may not say what the artifact must contain.

Scope is stated honestly in the docstring: this cannot catch a SEMANTIC leak,
where the prompt describes the graded behaviour in different words. That is what
actually happened here and it needs a reader. Guarding the blunt form is still
worth it — it is the easy mistake and it is silent. Mutation-verified: injecting
`minimum-task-score` into a scenario that grades it fails the rule.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
uipreliga and others added 7 commits August 13, 2026 13:01
`skill_triggered` read the Skill call's parameters and never its result, so a
call the tool REFUSED still scored as engagement. The case that matters is a
skill carrying `disable-model-invocation: true`:

  <tool_use_error>Skill coder-eval:ci cannot be used with Skill tool
  due to disable-model-invocation</tool_use_error>

The body is never loaded. The agent then answers from its own background
knowledge and produces output plausible enough that every criterion downstream
scores it as though the skill had written it — while the engagement criterion,
the one thing meant to catch exactly this, reports `yes`.

Found the expensive way. An outcome suite built to A/B the `ci` skill's body ran
24 rows across four arms differing only in that body; all four tied EXACTLY on
every criterion. Re-scoring those same artifacts with this fix gives 0/6
engagement per arm where the run reported 6/6: none of the arms had ever seen
the body they differed in, and the round measured the model's prior knowledge of
GitHub Actions. Confirming the diagnosis, removing that one frontmatter line
from the snapshot makes the call succeed and the same rows score 1.000 — with
the action reference the body specifies, which every no-body row got wrong.

The file-read signal is deliberately NOT gated on result_status: a failed Skill
CALL loaded nothing, whereas a path reference means the SKILL.md was actually
opened, which is genuine engagement and is how non-Claude agents engage a skill
at all. Both cases are pinned by tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…an reported

Tutorial 09 explained the no-promotion result as flaky 50-80% engagement plus a
ceiling where the skill did engage. Both were artifacts of the criterion bug
fixed in 59019dd. Engagement was never 50-80%; it was zero. The varying figure
came from counting refused Skill calls plus the occasional row that genuinely
read a SKILL.md off disk.

Rewritten around what actually happened:
  - `ci` sets disable-model-invocation: true, so the Skill tool refuses the
    call and the body never loads. 24 of 24 rows, result_status=error on every
    one, no row reading the file either.
  - The agent then answers from its own knowledge of GitHub Actions, plausibly
    enough that nothing downstream looks wrong — the tell being an action
    reference that does not exist, against the one its body specifies.
  - Four arms differing only in that body tied EXACTLY, which is a bug report
    rather than a result.
  - The fix belongs in the snapshot: delete that frontmatter line in every arm,
    which is what a real user's slash command effectively does. Verified — the
    same rows then score 1.000 with the action reference right.

The checks list grows a fourth item, and it is the one this round learned: the
Skill call SUCCEEDING is not the same question as engagement being reported, and
until the criterion was fixed the difference was invisible. It ships with the jq
line to check it directly.

The tool-policy section keeps its guidance but loses its causal story: the
0.333 -> 1.000 shift was measured with the body absent in both arms. Denying
sub-agent delegation is still right, for observability — a skill engaged inside
a child never reaches the parent's trajectory — which is the mirror image of the
bug above.

Also corrects the deletion sensor, which was pinning the superseded claim, and
the cost line (~55 runs, ~$20).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… on 6/6

With the body actually loading (snapshot minus disable-model-invocation), the
fixed criterion, de-leaked rows and the imperative prompt, all four pre-flight
checks pass and the train split scores 1.000 on every row and every criterion —
18 of 18 assertions, completion 1.0.

That settles the round. `ci`'s body is already correct on these scenarios, so
there was never anything for the three candidates to fix; Stages B and C were
rightly never reached. The page now contrasts the two tables directly: 1.000
with the body loaded against the 0.611 and "3/6 engaged" the same suite reported
while measuring the model's background knowledge. The distance between them is
the point of the page.

Adds the method's response to a ceiling — harder rows, not a looser gate and not
another round of candidates — and restores the emitted-workflow excerpt as the
evidence for what "correct" looks like here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both the bundled outcome template and the checked-in sample described the
engagement criterion as "a GATE on the baseline, not a diagnostic" and then
carried no suite_thresholds on it, so a run at zero engagement still exited 0.
After a round where exactly that happened — the skill never loaded, every row
scored the model's background knowledge, and nothing failed — leaving it
ungated was the one place these files did not practise what they teach.

recall.yes: 1.0 is the right gate: every row of an outcome suite is a positive,
so "of the rows that should have engaged the skill, how many did" must be 1.0 or
the round is a mixture of two effects. Against the real artifacts: the broken
Stage A reports 1.0 (which is how it passed), the same run rescored with the
fixed criterion gives 0.0 and now fails the gate, and the real baseline gives
1.0 and passes.

This also required the metric vocabulary in the threshold test to become
per-criterion-type — classification criteria emit recall/precision/f1 that the
default summary stats do not — which the final review predicted would bite the
first time anyone gated engagement. It did, immediately.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cord the dead end

Decision on whether to keep `ci` as the worked example's subject, having spiked
the alternatives rather than guessing.

Rejected — have the agent locate and read the SKILL.md itself. It should work:
a file read counts as engagement and genuinely loads the body, and it would need
no snapshot at all, for any skill. Tested against the unmodified plugin: 0 of 2
rows found the file, both scored 0.000. The plugin sits at a host path the
sandbox cannot discover. Recorded in both the skill and the template so nobody
re-explores it.

Rejected — retarget at a model-invokable skill. `lint-tasks` writes nothing
(disallowed-tools: Write/Edit), so there is no artifact to grade. `analyze`
writes analysis.md, but that is a prose report: grading it means brittle
file_check assertions on LLM phrasing, or llm_judge — which this very template
warns adds variance to the number the gate reads. Retargeting would make the
worked example less exemplary of its own advice, and costs a new fixture, rows
and baseline.

Kept `ci`, and removed the friction instead. Its output is the most gradeable of
the three (exact tokens in a workflow file, not prose), and the prep is not an
extra chore — it is the snapshot step every A/B round performs, so the header now
previews the real workflow in two copy-pasteable lines. Verified end to end: the
prepared copy is byte-identical to the shipped ci apart from that one line, the
Skill call succeeds, and the rows score 1.000. Skip the prep and engagement is
0/6 against a gate that now fails loudly and says why.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three items left open at the end of the plan's final review, now done.

Tutorial 08's restructure used mid-document H1s for its Part boundaries. Starlight
and mkdocs build the on-page TOC from H2/H3, so the Parts vanished from it while
their subsections rendered flat — the opposite of what the restructure was for.
Demoted one level throughout, with the page-level closers (Why this skill, What
to take away, Next) left at H2 where they belong.

That immediately broke the Stage B sensor, which pinned `### Stage B` — a sensor
failing for a reason unrelated to what it guards. It now matches the heading text
at any level.

Completed the `_normalized()` sweep: all 9 remaining sensors converted, so the
docstring's claim that every prose sensor reads through it is true again rather
than aspirational. `test_no_sensor_inlines_the_normalization_idiom` keeps it that
way, since a new sensor is usually copied from a neighbour and would inherit the
wrapped-phrase blind spot from an unconverted one. Mutation-checked: reinstating
a single inlined sensor fails it. Closes the harness candidate deferred earlier.

Plus the ragged rewrap in docs/PLUGIN.md left by the four-CLI-skills edit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`docs/TASK_DEFINITION_GUIDE.md` is the primary user-facing reference for this
criterion, and it still said engagement is "Claude's explicit Skill tool call
whose skill parameter matches" — the rule as it was before 59019dd, which now
also requires the call to have SUCCEEDED.

Adds the caveat and its reason: a refused call (the common cause being
disable-model-invocation) loads no body, so the agent continues on its own
background knowledge and produces output plausible enough that nothing
downstream looks wrong. Also notes why the file-read signal is deliberately not
gated the same way — a refused call loaded nothing, a path reference means the
SKILL.md was opened.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@uipreliga uipreliga changed the title feat(dataset,plugin): --split row filter + /coder-eval:optimize-skill, measured end to end feat(plugin): optimize-skill's execution track, measured end to end — and fix skill_triggered counting refused calls Aug 13, 2026
…skill

# Conflicts:
#	.claude/harness-candidates.md

Co-Authored-By: Claude <noreply@anthropic.com>
Comment thread tests/test_custom_lint.py
# skills' whole value is handing over an artifact rather than describing one.
# Caught for real: optimize-skill shipped a pointer at reference/templates/outcome.yaml
# one commit before that file existed, past 344 green lint tests.
import re
uipreliga and others added 10 commits August 13, 2026 18:39
…again

The early-stop watcher evaluates on the tool CALL, where result_status is still
None. `_engaged_skill_names` counted anything not "error" as engagement, so an
in-flight or crash-force-closed ("unknown") Skill call live-PASSED while the
frozen check scored it `no`. The two verdicts diverged on the one criterion that
decides whether a paid A/B round measured the skill at all.

Both signals now require the body to have actually been DELIVERED:

- Skill: an allowlist (result_status == "success"). For this tool the body IS
  the tool result, so a refused call (disable-model-invocation), one still in
  flight, or one force-closed by a turn crash loaded nothing. The branch also
  returns, so a call the gate excluded cannot be resurrected by the generic path
  scan below.
- Read/Glob/Grep: gated on "error"/None — the path is in the parameters, but
  nothing was read. Bash stays ungated: `cat SKILL.md | grep foo` exits non-zero
  AFTER genuinely reading the file, which is how an agent with no Skill tool
  engages a skill at all.

That makes engagement monotone (absent -> present as a call resolves, never the
reverse), which is what actually justifies latching a live verdict.

The cost, stated in the tests rather than hidden: a Skill call whose result never
arrives no longer stops the run, so it runs to its turn cap. The old behaviour
stopped it AND scored `yes` — crediting a skill whose body never loaded. The
ToolStart seam stays; which criteria can decide there is now per-criterion.

Seven tests in test_early_stop.py pinned the opposite semantics as deliberate
design. Each is inverted rather than deleted, with the reversal's reasoning in
its comment, plus a Bash twin for the path that still decides on the call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…exiting 0

`--split holdou` used to raise out of expand_dataset, get caught by
resolve_all_tasks like any load failure, and land in skipped_tasks: one yellow
line, zero evaluations, exit 0. A CI gate reporting success for a one-character
typo, and nothing in the output distinguishes it from a suite that genuinely
passed.

SplitSelectorError (a ValueError subclass) separates the two kinds of dataset
error. The others describe a malformed FILE, and demoting them is right — 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. resolve_all_tasks re-raises it; run_command.py's
existing `except ValueError -> typer.BadParameter` turns it into exit 2 with no
new CLI code.

Partial labelling stays legal — a run that selected something is a legitimate
run — but stops being silent: expand_dataset now warns with the drop count, on
the app logger that already reaches both the terminal and run.log. It fires only
when rows were actually dropped, never merely because --split was passed; the
two negative tests pin that, because a warning that fires when nothing happened
is a warning nobody reads.

lint-tasks gains the partly-labelled check for adopters, whom CE035 cannot
reach.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… own skill advises

check-skill tells users an activation row is a one-turn probe and that the host
project's CLAUDE.md is a confound. The template it hands them shipped neither
cap nor isolation, so a user who copied it got the opposite of the advice they
had just read.

Both additions mirror tasks/skills/lint-tasks-activation.yaml key for key rather
than inventing a shape. The caps are about signal, not only cost: a row that
runs on can time out, and a timed-out row is excluded from the confusion matrix
rather than scored — it never appears as a bad number, only as a denominator
that quietly shrank.

No max_usd: the worked example carries none, max_turns: 2 already bounds a
one-turn probe, and a third home for the same number is the first thing to rot.

The new loader-backed sensor is the guardrail — this finding IS "the template
drifted from the guidance", and an assertion that reads the real file is the
cheapest thing that keeps them together. Verified by mutation: dropping either
addition fails it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CE036's only test was a parametrized scan over tasks/ that passes when the rule
detects nothing — so it would have kept passing through any refactor that broke
the detection. The detection body moves into a `_offenders` classmethod that the
scan and six new fixtures both call, which is the point: the fixtures now fail
when the rule stops working.

skill_name joins the locator exemptions. It is a locator for the same reason a
path is: it names WHICH skill must engage, while 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
this, the first committed outcome suite for a skill whose name reaches the length
floor fails CE036 on its own engagement criterion.

command_pattern is documented as a collision rather than exempted: a regex
asserting *what ran* is graded behaviour, not a locator, and the `command`
exemption covers run_command.command — the command the checker itself runs. No
in-repo task has the collision, so an exemption would be an unused weakening.

The exemption list drifted once already (CLAUDE.md named three of the four fields
the code exempted), so CE036_LOCATOR_FIELDS is now the source and CLAUDE.md the
derived surface, with a both-directions parity sensor. CE036_MIN_LEAK_CHARS
replaces the inline 12, and the boundary fixture derives its strings from it — a
fixture that re-spelled the number would be the same drift one layer down.

One `_dataset_task` builder now serves CE035 and CE036; CE035's expected row sets
are unchanged, which is the evidence the extraction preserved behaviour.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Duplicate ids were already checked over the whole dataset; the missing-id and
malformed-id checks ran inside the expansion loop, i.e. over whatever survived
--split and sampling. So a malformed row sitting in the test split validated
under every --split train run and surfaced at promotion time — the most
expensive moment to learn it, and the documented optimize-skill workflow always
passes a split.

All three now run in one whole-dataset pass, before any narrowing. "The dataset
is well-formed" must not depend on what a given invocation happened to select.
Every message string is unchanged, and the `Dataset row {i}` index now counts
over the whole file, which is the more useful number anyway: it points at a line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
plan never called expand_dataset, so for a dataset-backed task it validated the
un-expanded shell. Two things followed. The resolved row count — the number
every cost estimate and every A/B comparison depends on — was knowable only by
paying for a run. And a ${row.<field>} naming a field no row carries failed at
run time, per row, after the sandbox was built.

Now plan prints `Dataset: <total> rows -> <selected> selected` and accepts
--split with run's semantics, so the check that decides whether to spend costs
nothing to make. The unfiltered total comes from a list length, not 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.

A partly labelled dataset gets a yellow ⚠ and keeps exit 0 — a warning, not an
error, because the run is legitimate, it is just measuring less than the file
suggests. It calls row_split_label rather than re-deriving what counts as a
label, so plan, expand_dataset and CE035 cannot drift apart on the rule.

The per-variant resolution now runs against the first expanded row, which is the
shape a run actually resolves.

This makes plan louder: a task with a missing dataset JSONL now fails a plan it
previously passed. That is the point — the run would have failed anyway, later
and more expensively. This repo's own tasks/ stays clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s true

Five things the optimize loop was silent about, and one sample row that graded
half its own request.

optimize-skill now says what removing `disable-model-invocation` costs in
external validity: results apply to the flag-removed configuration, so a skill
that ships with the flag must have its /name path re-checked before promoting.
The removal is still the right design — it is the only way to hold engagement
constant across arms — but the ledger's reader has to know which configuration
was measured.

Stage B's "no criterion regressed" becomes a predeclaration: name one primary
criterion plus the guardrails allowed to veto, before the stage runs, and record
them in the ledger before the numbers exist. Scanning every per-criterion
aggregate post hoc is uncorrected multiple testing in the rejection direction —
with enough criteria something always looks worse, so noisy criteria veto real
wins and which one "regressed" is unstable round to round.

ci-outcome's `both-triggers` row asked for a pull_request trigger AND a weekly
cron and graded only `cron:` — which its sibling `schedule-weekly` already
grades, so a schedule-only workflow scored 1.0 and the row discriminated nothing.
It now grades both. The other nine rows carry a doubled snippet so the criterion
keeps one shape: two identical includes score 0/2 or 2/2, numerically identical
to today, so no constant sub-check compresses the variance the mean: 0.7 gate
reads — and every train row's score is unchanged, which is what keeps the file's
own MEASURED "train split scores 1.000 on every row" claim true.

Tutorial 08 claimed 21 rows against a file holding 28, and "roughly 350 runs"
against stage headings summing to 407 over three Stage C attempts, not two. Both
are now derived: each stage names the suite revision it was computed at, the
Step-1 table describes the committed file, and a new sensor compares the table
against the JSONL — this exact drift, mechanically detectable, so it becomes a
rule rather than a fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
optimize-skill was 859 lines, and the review named three places where the length
had turned into structure problems.

Step 10 — the cost table, what each stage does and does not bound, why the two
tracks' gates use different machinery, and the paired-diff sign rule — moves
whole into reference/optimize-method.md. It is the same method on both tracks,
so it lives once; the skill keeps the procedure (which suite, which files, which
commands, in what order) and points at it. Same shape as reference/task-rubric.md,
which two skills already share.

This commit is MOVE-ONLY: every moved line is byte-identical to its pre-move
form, verified against HEAD, apart from the heading, which is no longer a
numbered step. Any rewording lands separately, by name.

The three seams:
- an editor's note ("this is a second reference on the same step, not a
  replacement") was addressed to whoever edits the file, not to the agent
  reading it. The two references are now disambiguated in the sentence itself.
- Step 7 switched from execution-track material to activation-only material with
  no headers, so a reader on one track read the other's without knowing. It now
  carries the same two track headers Step 4 uses, with the genuinely shared
  instruction hoisted above both.
- a 65-line disable-model-invocation block interrupted Step 4's five-requirement
  list mid-list. It moves above the list, where it belongs: it is a precondition
  that decides whether the execution track works on this skill at all, and it
  has to be read before the rows are written, not halfway through writing them.

The prose sensor is reworked rather than relaxed, into two sets that say which
surface each token belongs to and why: procedure tokens asserted against
SKILL.md ALONE — moving one would leave the step that must act on it pointing
elsewhere — and track-invariant method tokens against both files. The sign-rule
count sensor retargets to the method file, where both decision points now live,
and still demands the rule at each. A new assertion requires SKILL.md to point
at the extracted file: a reference nothing points at is a deleted reference.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… gate

The named reword the move-only extraction deferred, and a pre-existing bug the
extraction made visible by putting the cost table next to the two gates it
prices.

`3 × (S+1) × M_train` is the ACTIVATION arithmetic: S+1 arms through three
separate invocations. The execution gate is a different shape — exactly two arms,
incumbent plus one candidate, at --repeats 3 — so it costs 6 × M_train per
candidate gated, and gating three candidates in sequence costs three times that
rather than one pass with S = 3. A reader budgeting an execution round from the
old row overstated a single gate and understated a sequence of them.

Both rows are now in the table, with one sentence saying why they differ, since
the difference is the same one the two gates below already turn on.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nly assert

Three defects, all the same shape — a hand-maintained number describing a set
the code derives — plus the one the last commit left open.

- Tutorial 04 and the task guide said "14 criterion types" against a registry of
  15. CLAUDE.md's heading was right, which is why nothing noticed: it was the
  only guarded surface.
- Tutorial 07 told readers they would see "six commands" and listed six,
  omitting optimize-skill entirely. A skill-count sensor already existed; this
  file was not one of the four surfaces it read, and the tutorial's phrasing
  ("commands") was a fourth the matcher did not know.
- Tutorial 09 quoted ci-outcome.yaml excerpts that the previous commit changed,
  so a reader copying the shown row would build a suite that raises at expansion.

Each is now derived rather than asserted: a criterion-count sensor reads
CriterionRegistry, tutorial 07 joins SKILL_DOC_SURFACES, a tutorial-09 sensor
compares its excerpts against the committed suite, and the tutorial index is
checked against the directory listing.

TUTORIAL 08: measured instead of annotated. Part 1's numbers predate the change
Part 2 ends with, and the previous commit could only say so. Re-running settles
it — 62 agent runs on Sonnet, three replicates of the train split plus one of the
test split:

- lint-tasks holds at 1.000 F1 on all three train replicates and on the test
  split, so Part 1's finding survives a suite three rows larger.
- analyze reads 1.000 where Part 1 recorded 0.000, because the description this
  page promotes is committed (4c7481c). The baseline is unreproducible precisely
  because the loop worked.
- hard-3 is still unstable at 2-in-3 — the same rate Step 6 measured, months
  later, on a different revision of the suite. The page's own lesson, arriving
  unprompted.

So the numbers stay as the history they are, and the page now says what a reader
gets today and why it differs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@uipreliga uipreliga changed the title feat(plugin): optimize-skill's execution track, measured end to end — and fix skill_triggered counting refused calls feat(plugin): optimize-skill's execution track, measured end to end — plus the skill_triggered and --split fixes it surfaced Aug 14, 2026
uipreliga and others added 2 commits August 13, 2026 20:32
The page's own title has always said "Optimizing a Skill Description"; only the
filename said "a-skill". Next to its sibling 09-optimizing-a-skill-body that
reads as though 08 is the general case and 09 a specialization, when they are
two halves of one distinction — description versus body — and picking the wrong
one costs a reader a whole suite.

Free to do now and not later: 08 and 09 exist only on this branch, so
coder-eval.com has never served the old URL and there is nothing to redirect.
After this merges the same rename would break a published link, and mkdocs has
no redirects plugin configured.

Six internal references updated; the flat indexes regenerated with
`make docs-indexes` rather than hand-edited.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Part 1 already had a "what you get running this today" section; Part 2 did
not, so its numbers read as current when every one of them predates the change
the part ends with. Measured rather than asserted: Stage A re-run against the
committed 17-row train split, four arms, 68 agent runs, $3.84 on Sonnet.

The incumbent is now round 1's winner (4c7481c), so `a-regression` cannot be a
candidate; the arm set is fresh — `b-results` and `c-symptom` re-expressed
against the new incumbent, plus `c-compact`, which asks whether the length is
load-bearing given only 23 characters of listing headroom remain.

    incumbent   analyze F1 1.000   completion 1.000
    c-compact   analyze F1 1.000   completion 1.000
    a-results   analyze F1 0.857   completion 1.000
    b-symptom   analyze F1 0.857   completion 1.000

With the incumbent at ceiling, min(candidate) > max(incumbent) is unsatisfiable
and the round ends at Stage A — 68 runs against the 287 all three stages would
have cost. Stage B and C were not run.

Two findings worth the page. `c-compact` ties on F1 with 53 fewer characters,
which is a real result on a different budget and still not a promotion, because
the gate is `>` and not `>=`. And `hard-3` engaged `task` in two of four arms
whose `task` description is byte-identical — the instability the page has been
tracking since Part 1, this time with the confound excluded by construction
rather than by repetition.

The Stage A row in the Reference table said it "Decides nothing"; it decided
this round. Corrected to say what it actually does.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants