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 188 commits into
Open
feat(plugin): optimize-skill's execution track, measured end to end — plus the skill_triggered and --split fixes it surfaced#109uipreliga wants to merge 188 commits into
uipreliga wants to merge 188 commits into
Conversation
uipreliga
requested review from
akshaylive,
bai-uipath and
tmatup
as code owners
August 12, 2026 21:36
|
Claude finished @uipreliga's task in 1m 29s —— View job Code Review in Progress
|
| # 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 |
| match node: | ||
| case ast.Expression(): | ||
| return _compute(node.body) | ||
| case ast.Constant(value=value) if isinstance(value, int | float): |
| """ | ||
| import inspect | ||
|
|
||
| import coder_eval.optimize_gate as gate |
| def test_target_label_is_importable_from_models(self) -> None: | ||
| # A module-level constant that is not in `__all__` is a private import in disguise, so the | ||
| # move is only complete when both halves are done. | ||
| import coder_eval.models as models |
| assert "TARGET_LABEL" in models.__all__ | ||
|
|
||
| def test_the_gate_imports_it_rather_than_redeclaring_it(self) -> None: | ||
| import coder_eval.optimize_gate as gate |
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>
… 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>
…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>
… is auditable `weighted_score` is a weighted MEAN, so a criterion that scores identically on both arms on every row contributes its whole weight to that mean's denominator and nothing to its difference. The shipped outcome template does that by design — engagement and the `file_check` both saturate — so an effect confined to the grader reaches the gate multiplied by 1/2.05, and nothing anywhere said so. `ExecutionGateVerdict.dead_weight` is that share, with the naming sentence in the existing `notes` list rather than a second field. Computed beside the noise floor, before `_verdict` exists, so every return path reports it and the note is in the list pydantic copies at construction. **It is a READING and can never gate**, and that is a permanent decision rather than a deferral, measured rather than argued: a constant criterion scales the paired difference vector without changing its shape, so it scales the mean AND the standard deviation by the same factor — the paired *t* is identical to 1e-12 between the grader-only and blended scales while the mean difference scales by 1/2.05. Every conjunct of `promoted` is invariant to it, so wiring it into `integrity_checks` would force `promoted = False` on comparisons that are statistically sound. The one case where dead weight genuinely invalidates a comparison — every criterion constant — is already the zero-variance refusal. `None`, never `0.0`, on five distinct causes, each naming itself in `notes`: fewer than two rows paired, an arm with no criterion results, arms whose criteria lists disagree (a contaminated tree, whose diagnosis the reconciliation refusal owns), an unrecorded weight, and zero total weight. The rendered UNKNOWN names no cause, because only the note knows which of the five it was. Replicates collapse by MEAN before pairing, matching `paired_comparison` and `arm_row_scores` so the three surfaces agree about what a row scored, and the `== 0.0` test carries no tolerance — a tolerance would reclassify a small real effect as unmeasurable attenuation. Both are pinned, the second by a 1e-9 difference that must read ALIVE (verified by mutation: `abs(diff) < 1e-3` fails it). `_criterion_weights` is private beside `_row_score`, which is REUSED for the per-criterion extraction rather than re-derived. Pins regenerated: one key, one note and one rendered line each; no statistic moved. One ADDED `## Estimator changes` row (in the previous commit, with the schema ripple). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…st instrument provenance
The grader fingerprint covers the outcome track's script and its answer key. It cannot see a
criterion `weight` change that re-blends `weighted_score` — the number the execution gate's
paired *t* compares — and on the activation track it answers `None` every time, because
that track has no script grader at all. So the suite around the grader had no provenance,
and on activation nothing did.
`suite_fingerprint(task, rows)` is a SHA-256 over the criteria (each concrete subclass dump
minus a reason-carrying denylist of ONE, `description`), the prompt template as authored,
the expanded rows the round scored, and the whole `run_limits` block. `RoundScores` records
it; `optimize_store.suite_changed` compares it, three-valued and sharing one body with
`grader_changed` — `None` means NOT RECORDED, so an older sidecar cannot masquerade as a
suite that provably did not move.
**Its own module because the store PERSISTS and this COMPUTES**, the precedent
`leak_detection.py` and `reports_optimize.py` set. Not a cycle — `optimize_store` imports
`coder_eval.models`, `TaskDefinition` is in it, so a digest living there would close nothing.
The review of the first implementation changed what this covers, and the corrections matter
more than the addition:
- **The rows are hashed, not just their ids.** `activation.yaml` is
`initial_prompt: ${row.prompt}` with `expected_skill: "${row.expected_skill}"`, so every
prompt AND every label lives in the rows file. A digest over ids alone left a rewritten
prompt and a flipped label byte-identical — the commonest suite edit there is, on the one
track this digest is the sole provenance for. The grader half hashes `expectations/*.json`
because the answer key is part of the instrument; this is that rule on the other track.
- **`run_limits` is hashed whole.** Curating four caps was justified by a claim that is false
twice: the three token caps abort a run exactly as `max_usd` does, and `stop_early` is the
kill switch for every armed criterion, so it moves `f1.yes` itself.
- **The length prefix and the section tags are redundancy here, and the module says so.**
Verified by mutation: removing either leaves every test green, because canonical JSON per
part already escapes the delimiter and quotes strings. They stay for locality; the false
attribution does not.
- **The dump-settings tests asserted digest inequalities and caught nothing.** Each of
`exclude_none` / `exclude_defaults` / `exclude_unset` left all 41 tests green — dropping a
key from one side of an inequality leaves the sides unequal, and the real damage is
COLLISION (`exclude_defaults` drops the `type` discriminator, after which two criterion
types hash the same). They now assert the contract on `scoring_dump`, and all three
mutations fail.
- Step 11's snippet referenced a variable defined only inside a comment — a `NameError` in
the user's terminal after the round is paid for. It is live now, uses the UNION of arms'
row ids rather than `arms[0]` (a hole is absent, so one arm's vector is shorter), and was
executed end to end against the shipped suite.
- Step 11 also stops printing "comparability unknown" on every activation round: that track
has no script grader, which is not the same as not knowing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`GATE_RESAMPLES` is DERIVED from `GATE_P_PRECISION` at the strictest Holm threshold for `GATE_MAX_FAMILY = 5` survivors. Above that S the threshold tightens while the draw count does not, so the Monte-Carlo error of the p stops being the declared fraction of the threshold it is decided against — and nothing said so: the block printed the family size while the declared precision sat on a constant nobody reads at that moment. `_note_resolution_degraded` states the strictest threshold, the precision actually achieved, and the draw count that would restore it. It returns `str | None`, unlike `_note_holm_family` beside it: putting the threshold condition IN the function keeps it in one place, where an `if` duplicated at the two call sites is the shape that lets the two tracks drift — which is the whole reason these notes are shared. Emitted at both existing call sites, OUTSIDE the negative-result guard beside the family note: it is a statement about the draw count, not a claim about the candidate, so it stays true under a refusal. Both wrappers report the family's SMALLEST `n_resamples`, read off the verdicts rather than from the constant — the coarsest member is what bounds the family's resolution, and only members with a p were tested at any resolution. No constant moved and no decision changed: `gate_refusal`, `promoted` and `separated` are untouched, asserted directly. **No existing pinned fixture changed and no ledger row is owed.** Every pinned render carries a family of 1 or 2, so the note appears in none of them; it is pinned by a NEW family-of-8 fixture, which has no "before" and therefore no step to attribute. The test says so, so that if it ever starts modifying a sibling the reach is visible. Every figure in the tests is recomputed from the shipped constants. The table they reproduce was verified against the working tree first: S=6 → 0.1095 / 24,000; S=8 → 0.1265 / 32,000; S=10 → 0.1414 / 40,000. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A candidate is free to edit `scripts/` and reference files, and `reference/proposal-prompt.md` is about to say so out loud. A graded string bundled into one of those was invisible to `(arm / "SKILL.md").read_text()` — and the preflight then returned **clean**, byte-identical to a genuinely clean candidate, which is the worst shape a check of this kind can have. `skill_text(skill_dir)` concatenates every UTF-8-decodable file under the directory in sorted relative-path order, each preceded by its path — so a finding stays locatable and two files cannot concatenate into a phantom match across the boundary between them. `candidate_leaks` stays PURE: two strings and a row list. Widening the scan inside it would have made it read the filesystem, so the IO is a separate function and the module docstring says which half does what. Its boundary list is FIVE now; the fifth is that the caller decides what text is scanned and the checker cannot tell how much of the candidate it saw. Skipped, and each verified by execution rather than assumed: a non-UTF-8 file (a binary cannot carry a verbatim graded string in the form `graded_strings` produces), an `OSError` (a permission, or a file that vanished mid-walk — a preflight must not abort a round over one stray file), a symlinked FILE, and a symlinked DIRECTORY. The last two are stopped by DIFFERENT mechanisms — the explicit `is_symlink` check, which must precede `is_file` because that one follows the link, and `rglob` not recursing through a symlinked directory — so each has its own test. The flagged-in-scripts test asserts BOTH halves: the tree read flags it and the one-file read does not, so the test states the defect rather than only the fix. And a span already in the baseline's own reference file stays unflagged, so widening the scan cannot re-introduce the wolf-crying the diff was built to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er candidate space Stage C was prose: "report that block verbatim alongside the test F1s", leaving the reader to eyeball two intervals and decide whether one reproduced the other. It now has a verdict. `confirm_gate` (activation) / `confirm_gate_execution` (execution) run the track's own gate on the confirm run directory and classify the train→test delta as REPRODUCED, SHRANK, REVERSED or UNDECIDED. `ConfirmVerdict` carries both effects, the delta, the confirm split's own MDE and the full confirm-gate block; `render_confirm_markdown` prints it, with REVERSED directly below a refusal because a reversal is a headline rather than a footnote. Four things it deliberately does NOT do. It measures no floor — `execution_gate` already prices the replicate null split on whatever run dir it is handed, so a second estimator would be the CE037/CE040 defect class and would double every confirm's cost. It confirms exactly ONE candidate, raising on a list, because confirming a shortlist spends the held-out split on SELECTION. It applies Holm at m = 1 only so the carried block reads as a decision — there is no multiplicity at Stage C. And it REFUSES a confirm run that did not record `--split test`: a `train` confirm re-runs the rows the candidate was fitted to, reproduces by construction, and costs full price with no error anywhere. `classify_confirm` and `build_confirm_verdict` live at rank 1 with three more shared primitives, because the two rank-2 track modules may not import each other and per-track means two copies of promotion-relevant arithmetic. `FLOOR_RESOLUTION` moved there for the same reason — the classifier needs it, and `test_mde == 0.0` is the too-narrow test the execution track already had to widen once: a null split over a constant difference returns 2.8e-17, so an `== 0.0` check goes inert on exactly the degenerate suites it exists for. Value unchanged; ledger row added. `ExecutionGateVerdict` also records the PREDECLARED `primary_criterion_index` and its `primary_mean_diff` — the paired difference on that criterion alone, which is what converts the blended figure back into the grader's own unit beside `dead_weight`. Computed over the rows the statistic actually paired, because the field is sold as that conversion and the identity holds only over one sample. The reviews changed real behaviour, not just prose: - `classify_confirm` accepted ANY pair of signs, and produced actively misleading answers for three reachable inputs: a candidate that lost on train and lost harder read REPRODUCED, a loss that shrank read SHRANK, and an effect that APPEARED where Stage B measured none read REPRODUCED. Stage C confirms a WIN; all of them are UNDECIDED now, plus a `math.isfinite` guard, since a NaN fell through to the most permissive rung. - The activation split check DROPPED its refusal whenever any pooled dir lacked provenance — `SplitProvenance.value` collapses to UNRECORDED when one dir is unreadable, so three dirs recording `train` beside one unreadable `run.json` classified over train rows with only a note. The shared `confirm_split_check` reads `recorded` directly; the regression test was mutation-verified against the chained form. - `primary_mean_diff` was never rendered while four surfaces said the block reported it. - "It never affects `promoted`" was false in three places: an over-range index refuses. - A refusal was printed twice per block. `holm_promote`'s rule applies here too. Phase 7 rides along, being prose the confirm gate's own scope depends on: `proposal-prompt.md` now names scripts and reference files as legitimate edit targets (with the prose→determinism hypothesis, and the three constraints that follow — `allowed_tools`, the whole-directory leak scan, activation being untouched), supplies the PASSING rows as what must not regress with the honest sample-and-mixed-evidence caveats, and points at the regression corpus before the edit rather than after the run. `outcome.yaml` gains the `Bash` worked example on its `allowed_tools` union rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…surfaces The bootstrap is seeded, so a p near the Holm threshold can land on either side of it depending on the draw — and a single verdict cannot say whether that happened. `gate_seed_stability` gates at three seeds and reports the agreement, the p per seed and the spread. **It returns no single `promoted` field, and that is the whole design.** Collapsing three disagreeing seeds into one verdict is exactly what the reading exists to prevent, so there is nothing to read that way: `render_seed_stability` says "UNSTABLE — promoted at 2/3 seeds. This is a coin flip, not a result" in those words, because "2/3" on its own reads like a result to anyone skimming. A separate function rather than a `seeds=` parameter on the gate. The parameter would have moved the cost and the rendered output of every existing call site and every pinned fixture; this way `activation_gate` is untouched — asserted on its signature — and the reading costs three bootstraps over rows already loaded, so zero extra agent runs. The skill says so where it offers it, since the obvious assumption is that it triples the round. `SeedStability` is a NamedTuple beside its producer on `RuleCeiling`'s stated precedent — computed and rendered, never persisted — and is exported from nowhere. The layering test's deferred-import set is three modules now, and every member of it is a NamedTuple: a new name appearing there is the signal to ask which category it belongs to. No execution twin, and the docstring says why rather than leaving it as an omission: that track's primary is an analytic paired *t*, deterministic given the rows, so a seed moves only the MDE and the guardrails — the function would report a spread of zero on the number that decides. Also finishing Phase 7's remaining surfaces, all of them consequences of scripts becoming a legal candidate shape: Step 12 diffs the whole skill DIRECTORY on the execution track (a `SKILL.md`-only diff renders a scripts-only candidate as no change at all); the method file's control-arm section says that a short body is no longer a proxy for "no instruction", so read the directory rather than the line count when deciding which arm is which; and Step 13's stop rule prints the cumulative CANDIDATE count beside the round count, because the patience is a budget in hypotheses and two rounds of four have tested eight while two rounds of one have tested two. Four prose sensors pin the paragraphs that would otherwise be one careless edit from vanishing — scripts as edit targets, the passing rows with their honest caveats, the candidate count, and the tree diff. Their absence is silent in every case: nothing fails, the proposer simply stops being told. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`CriterionResult` is a consumer contract — `task.json` is read by the evalboard and by anything pointed at a run tree — and CE030's doc-parity family covers `TaskDefinition` / `RunLimits` / `Dataset` / `SimulationConfig` only. So adding `weight` to the model and forgetting `docs/REPORT_SCHEMA.md` broke nothing and failed nothing; a reviewer caught it, which is exactly the kind of catch a guard should be making instead. `TestPersistedCriterionResultFieldsAreDocumented` asserts every base field is named in that section. Deliberately not a numbered rule: it is one derived assertion over one section rather than CE030's configurable model→guide machinery, and a number would imply a generality it does not have. Verified by mutation — removing `weight` from the section fails it. Two candidates deferred to `.claude/harness-candidates.md` with why neither is cheap: a python fence in a shipped SKILL.md being executable as written (the cheap `ast` version cannot tell a legitimate placeholder from an undefined local, and this is the SECOND fence defect to ship past every sensor, so the pair argues for building it properly), and a moved WATCHED constant leaving the schema doc's boundary paragraph misattributing it (a module check was written and then removed for being unfailable — the limitation is now stated in the test itself). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…four blind digests Six Mediums from the cross-phase pass, every one a seam between phases rather than a defect inside one. **The three magnitudes on an execution block were measured over three different row sets, and a comment claimed they were one.** `mean_diff` comes from `experiment.json`, `primary_mean_diff` from the on-disk results over the paired rows, `dead_weight` from the on-disk results over the intersection — so the conversion the block invites, `mean_diff ≈ primary × (1 − dead_weight)`, is exact only while all three coincide. Reproduced at 2.2× off with one row that `experiment.json` scored and disk does not carry. `_sample_divergence_note` now says so on the block, reporting the primary's USABLE count rather than the row-id list it was handed — the ghost row is IN that list and contributes nothing, which is the divergence itself. The identity test states its precondition and a second test pins the note's absence on a healthy block. **The primary-index refusal was recorded AFTER `_execution_diagnostics`, defeating the guards the previous phase added.** It produced exactly the contradiction those guards exist to prevent: a `NOT A RESULT — primary_criterion_index=7 selected no usable row` headline above notes reading "this is an ordinary negative result and not a measurement problem". Resolved before the ladder. **`gate_seed_stability` decided at a family of ONE while the round it is offered beside decided at `alpha/m`.** Measured: `3/3, STABLE` for a candidate a family of three rejects. It cannot reproduce Holm without every sibling's p at every seed, so it now says what it measures — "would promote at 3/3 seeds" plus a line naming the family size and sending the reader to the p spread. **The suite fingerprint was blind to five fields that decide what a score means**, sharpest of them `reference` — the answer key the judge criteria score against, while the module's own justification for hashing criteria whole is that the answer key is part of the instrument. Also `initial_prompt_file` (the prompt section hashed `null` for every file-backed suite, so the docstring's claim to cover the prompt was vacuous there), `expected_commands`, `pre_run`/`post_run` and `simulation`. Dumped with `include=`, which makes "the agent and sandbox blocks do not reach the digest" structural rather than a matter of which keys a loop reads. **The "51.2% dead weight by design" example was false in five prose surfaces.** The template's `file_check` is a GRADED outcome check with its own `mean` threshold; only the engagement criterion saturates by design, so the by-design figure is ~2.4%, and 51.2% is a property of a RUN in which the artifact check also saturated. Corrected everywhere, with the distinction stated. **`test_the_p_spread_is_none_below_two_measured_values` asserted the value it passed in** — the `>= 2` guard could be mutated to `>= 1` with the whole class green. It drives the real computation now, through the single-seed input that reaches the branch, and fails under that mutation. Lows: the byte-identical `family_resamples` line is `_family_resamples` at rank 1 (the duplication `_note_resolution_degraded` was put there to avoid); four statements about the deferred imports said "two NamedTuples" over four across three modules; the activation confirm de-duplicates its run dirs, since that track passes the same list for both arms and the note read "missing from 2 of 6" for three directories; the cost line derives its count from `len(seeds)`; one unused test parameter. Not fixed, and deliberately: five pre-existing defects the multi-model pass surfaced in adjacent untouched code — `_recorded_replicates`' handling of a malformed `replicate_index`, `search_compare` passing a whole cross-suite corpus, `rule_row_map`'s partial attribution, `render_headroom_ceilings`' GAP verdict under incomplete attribution, and `checker._resolve_refs`' reference exclusivity. `git blame` puts all five outside this change; they are listed as next steps rather than folded in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… sensors derive their scope
The seven flat `optimize_*.py` modules move into `src/coder_eval/optimize/` — ranks 0-3 plus the
ladder-exempt `store` sidecar — with a docstring-only `__init__.py`. No re-export facade, and that
is machine-checked from both sides: one sensor asserts the `__init__` binds no non-module public
name, another that no module still answers at an old flat path. A facade would let every
pre-package import keep working and make the split cosmetic.
Behaviour-preserving throughout: every changed line under `src/` is an import path or a docstring
token, and no fixture under `tests/_fixtures/optimize_renders/` or `optimize_verdicts/` differs by
a byte.
The two phases land together because the tree is red between them — moving the files breaks three
sensors that keyed on the flat filenames, and re-scoping them is what makes it green again:
* the layering tests' module set is now DERIVED (`pkgutil.iter_modules`, private modules filtered),
with `_rank_coverage_gap` as the one predicate the module-scope assert and its test both run, so
a new module fails loudly with an actionable message instead of joining the family unranked. The
ranks stay a literal map: a rank computed from the import graph would pass over
`activation -> execution`, which is acyclic and still wrong, so the map IS the specification and
`pkgutil` supplies only the coverage check.
* CE053's path scope becomes the `optimize/` DIRECTORY, with a test asserting it matches the new
path and NOT the old flat one. Its failure mode is silence, so the negative half is the point.
* `WATCHED_CONSTANTS`' six paths are repointed, which trips the estimator-protocol job — hence one
`## Estimator changes` row recording a zero step: the values are unchanged, only the module each
is declared in moved.
Module names are dotted and package-relative to `coder_eval` (`"optimize.load"`), which keeps the
rank test's `removeprefix("coder_eval.")` comparable and lets one `_module_path` helper replace
three hand-rolled joins.
Two breakages a grep for `coder_eval.optimize_` could not have found: the holm-confinement test
keyed its results on the bare filename, where `gate.py` no longer says which gate; and the
`resolved_module` case whose whole subject was "a module sitting directly in the package" had
quietly moved into a subpackage and stopped testing that.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…keeps it that way One underscore cannot mark two boundaries. The family has three tiers — the skill-facing API the SKILL.md snippets import, the helpers several modules in the package share, and the names local to one file — and before this the middle tier was spelled like the innermost: 29 names carrying an underscore while four modules imported them, which tells a reader "safe to change this signature" about a helper a change here breaks in three other files. After the rename the middle tier is spelled like the outermost, which tells a reader "the skill might depend on this" about a helper nothing outside the package imports. The second error costs caution; the first costs a build the author did not expect. CE059 is the guard, and zero violations on the real tree is what says the 29 renames are complete rather than merely started. It routes through the one relative-import resolver and joins the CE051 parity table, because a rule matching `node.module` alone reads `from .load import _pool` as the bare module "load" and fails OPEN — the failure shape that once left four import rules blind at once while `make lint` stayed green. Ruff's PLC2701 is not a substitute: pointed at the module that imported 11 private names from two siblings it reports "All checks passed", since it fires only on private imports from an EXTERNAL module. Two names got a qualifying noun instead of a bare de-underscore, because the bare form says nothing at a cross-module call site: `pool_replicates` (it merges row maps APPENDING replicates) and `classification_metric`. A third left the package entirely — `reports_stats.median_or_none`, beside `mean` and `stddev`, since a median is not one of the questions a run-directory loader answers. `_family_resamples` could not simply de-underscore: `family_resamples` is already a local in both Holm wrappers and a keyword parameter on `_activation_notes`, so the FUNCTION became `resamples_for_family`. Ruff caught the shadow as F823; the plan's collision spike had scanned module-level names only. Moving the median helper into `reports_stats` also brought it under the repo's non-finite sweep, which it failed by returning `nan`. Exempting a numeric helper from that guard is the wrong direction, so it returns `None` — and the fix went upstream as well, because `None` reaches two branches whose text tells the user the data was ABSENT: `row_cost_levels` now drops a non-finite row exactly as it drops an empty one, which is what makes "guardrail not evaluated" true of it. Until now a single corrupt `total_cost_usd` propagated a `nan` into a guardrail's relative-change arithmetic, where every comparison answers neither way while the check still reports a number. No valid-input verdict changes, and no pinned fixture moves. Also repaired three copies of a harness assertion that could never fail: they probed `rule_id` on a rule class, an attribute `BaseRule` does not declare. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"It won and was vetoed" and "it lost" call for opposite next actions, and the set that separates them was spelled five times. One of those five read `guardrails` alone, so a candidate that separated, cleared Holm and was vetoed by a failing SIBLING check rendered as the ordinary `NOT PROMOTED` headline — indistinguishable from one that simply lost, which is the single confusion that rung exists to prevent. `failed_vetoes` is now a property on each verdict, both routing through one `_failed_names` predicate, so the `not check.passed` polarity is declared once. A property and deliberately not a `computed_field`: it must stay out of `model_dump()`, or every pinned `optimize_verdicts/*.json` gains a key for a value that measures nothing new. Behaviour-preserving, and the polarity is the whole risk — the activation site's old spelling was the POSITIVE `siblings_hold = all(...)` and the new one is a negative. Old was `rejected ∧ separated ∧ (∀ siblings pass) ∧ ¬(∃ guardrail fails) ∧ refusal is None`; De Morgan makes that identical to `rejected ∧ separated ∧ ¬failed_vetoes ∧ refusal is None` on every input, so four conjuncts becoming three is a fusion of two rather than a drop. Pinned by a test that fails under an inverted polarity, verified by mutation. Two sites deliberately NOT collapsed, both because they carry information the union cannot: * `siblings_hold` survives as a binding. It is not only a `promoted` conjunct — `_activation_notes` needs it to tell "this candidate moved the failure rather than fixing it" from "it costs too much", and deleting it as redundant makes both causes print the generic guardrail note. * the activation note loop stays guardrails-only, because the rung above it is already the single declaration for a sibling failure. Unioning it would double-report every sibling regression. That site had no sensor and now does: the new test asserts the check name is ABSENT from the notes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two renderers held parallel `if/elif` chains, kept in step by hand, and they drifted twice: the activation BLOCKED rung once read `guardrails` alone while its twin unioned both veto lists, and before that it keyed on `promoted`, which the veto had already made unsatisfiable. Neither drift deleted a token, so no presence sensor could see either — and both looked exactly like a rung being rebuilt at a call site, which is detectable. `_headline` is now the one chain, taking the three strings the tracks actually differ by. Not a rung table plus an evaluator: two call sites in one file do not earn an abstraction a reader has to unwind to answer "what does this print?". The execution ladder still READS as four rungs rather than five, and that is a property of one argument rather than a skipped rung — it passes `NOT A RESULT` as its `refusal_label` and reaches rung 3 with the text rung 2 produces, because it has no discreteness refusal to distinguish. Activation keeps `CANNOT SEPARATE AT THIS SIZE` there, since a refusal that DID compute a p is a statement about the suite's resolution and demoting it would tell the user the run was mis-wired. Behaviour-preserving, verified rather than asserted: every rendered byte is identical over 1296 field-state combinations per track against the original chains, all 143 pinned rung and refusal tests pass untouched, and no fixture moved. Rung ORDER is the contract — a refusal outranks BLOCKED and UNDECIDED outranks the refusal — and the separate `NOT A RESULT:` line each renderer prints when the headline could not carry the reason still appears exactly once. The rung rationale is single-sourced too, which is the same defect one indirection over: it had accumulated in three places, and both renderer docstrings still named a local this change deletes and described a twin rung that is now literally one chain. `_headline` argues the BLOCKED conjuncts once; each renderer documents only what is track-specific. A generated cross-product over tracks × rungs replaces the single-track version, and an AST sensor fails if a headline is ever built inside a branch again — with `render_confirm_markdown`'s own deliberate ladder as the standing positive control, so the sensor cannot go quiet without saying so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing into a test file `tests/lint/computed_claims.py` imported private helpers out of `tests/test_optimize_gate.py`. That is an inverted dependency: the lint layer reasons ABOUT the tree, so reaching into a test file for fixtures makes it depend on one — and it is what blocked splitting that file, since three claim functions named its internals. `tests/test_optimize_measurements.py` imported four more. `tests/optimize_fixtures.py` is the shared builder module, a non-numbered leaf on `tests/lint/import_resolution.py`'s precedent. What lives in it is DERIVED — a builder more than one of the coming per-module test files needs, plus everything the lint layer and the measurements suite import, plus the transitive closure over builder-to-builder references. That last step is not optional: `write_run_provenance` and `record_task_result` are referenced by no test body at all, only by `write_row`, so a grep over test bodies leaves them behind and breaks the module at import time. Names here are public, the same reasoning that took the underscore off the package's cross-module helpers. Two took a qualifying noun instead: `activation_verdict`, because `gate` is already 14 bindings across `tests/` including `import coder_eval.optimize.gate as gate`, and `arm_row_scores_for`, because `arm` is 4. Behaviour-identical: every moved body is byte-identical modulo the rename, and the collected node-id set is unchanged apart from four new sensor tests. Those sensors are the point — extraction fixes the inversion only while the new module stays a leaf, so one asserts it imports nothing from the test tree and one asserts no lint module reaches into a test file. Neither reads `node.module`: CE051 fired when the first draft did, on code minutes old, which is that meta-rule doing its job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e package Eight files, one per source module plus one for the cross-module contracts, so the rank ladder has a test-side counterpart. `tests/test_optimize_gate.py` is deleted rather than left holding leftovers: its name would imply it covers `optimize/gate.py` while being the residue of everything. The deliverable is that nothing changed, and it is verified as such rather than asserted: 803 collected tests before and after, with the node-id sets IDENTICAL modulo the file component. Whole top-level statements moved unmodified; coverage is 92.56%. Bucketing was DERIVED, not named — each of the 97 classes went to the module owning most of the optimize symbols it exercises, which is what caught four misfilings a name-based pass would not: `TestEveryWrongPathMessageDerivesFromTheGlob` and `TestGateResampleCount` are whole-FAMILY scans, so they belong with the cross-module contracts (which is also what keeps `_OPTIMIZE_RANKS` and `_family_source` in the one file where the ladder is declared), and `TestPromotionIsNotOverstated` says "the rendered block" in its own docstring. Seven class members became cross-FILE references the moment the classes parted, so they moved to the shared fixture module rather than being duplicated — and were renamed for what they build, because `_arms` meant a different builder in each of three classes. One dead ClassVar came out too: `TestExecutionDiagnostics._ROWS` had zero references in either revision, and only surfaced because it shadowed a module-level name in the sharing analysis. `test_optimize_activation.py` lands at 2,090 lines, 4.5% over the plan's ~2,000 guideline. Left as is: its two largest classes each have an execution-track twin, so splitting them out would break the by-track symmetry the layout exists for. Every prose reference to the deleted file now names the file holding its subject. Four keep the old name on purpose — they describe the past defect a sensor was written for, not the current tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLAUDE.md's optimize block was 16,766 bytes across eight tree entries — 2.2x the module docstrings in the same family, saying much of the same thing twice. It loads into every session for every task, while a docstring is read when the file is opened. It is 2,980 bytes now, one line per entry. Relocation was verified rather than assumed: every backticked identifier and every rationale sentence in each entry was checked against its module before the entry was cut, and only what had no home anywhere was merged in. Two things did. `optimize/gate.py` now states the estimator-ledger obligation its four watched constants carry — that touching one, including moving the file it is declared in, demands a row in `docs/REPORT_SCHEMA.md`, because a rendered statistic can step for identical data and nothing in a run artifact tells that apart from a real change in what is being measured. And `optimize/store.py` now states what the grader fingerprint COVERS — the script plus the `expectations/*.json` files it loads, because the answer key is part of the instrument, and not a `__pycache__` or an `archive/` of retired keys — along with the exit-code rule in the script it hashes, where a score-shaped line on a failed `--fingerprint` would be recorded AS the fingerprint. Everything else already lived in a module or function docstring. A strict "the rationale must be in the MODULE docstring" reading would have meant copying function docstrings upward, which puts the reasoning further from the code than it already is. CE059 also gets its entry in the CE-rule paragraph. That paragraph is the repo's only listing of CE001-CE058 and nothing mechanically enforces membership, so a rule absent from it is a rule the next author does not know exists — the same silent-omission shape the paragraph itself warns about for id collisions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g why Three independent reviews (gemini-3.1-pro, gpt-5.6-sol, and an Opus pass) found one real cross-phase defect, and both Highs were found twice independently. **The defect only a whole-diff view can see.** `row_cost_levels` gained a non-finite filter, so the cost guardrail's displayed medians and its incumbent mean skipped a corrupt row — while `cluster_bootstrap_diff_ci` still received the RAW clusters. A `nan` reached `ci_low`, and `nan <= materiality * mean` is False, which is a veto. On two IDENTICAL arms with one corrupt row the block rendered `BLOCKED BY A GUARDRAIL`, `1.000 -> 1.000`, a relative change of `0.000`, and no note: a promotion blocked with the page saying nothing changed. Each phase was safe alone; together they cleaned the displayed numbers and left the decision wrong. Fixed where the rows are chosen: a row carrying a non-finite figure on either arm is dropped where an unmeasured row is, so the medians, the mean and the interval see the same rows — and the discard is NAMED rather than silent. The existing note's wording is untouched, so every pinned render stays byte-identical. `cost_quality_points` had the same divergence, with its two coordinates on different row sets while `row_ids` claimed full coverage. Both reviewers called this reachable in production; it is not, and the code now says so. Pydantic serialises a non-finite `total_cost_usd` as `null` and the gates only ever read from disk, so the input reaches these functions from a CALLER — which still matters, because they are public and take the rows they are handed. The fronts test pins that mechanism rather than asserting a state I cannot build through a real artifact. **A Phase 7 defect the node-id parity check could not see.** The test-file splitter moved every top-level statement that defines a NAME, and a bare `assert` defines none — so two module-scope asserts guarding the rank ladder's coverage were dropped silently. An assert is not a collected test, so 803 identical node ids proved nothing about them. Restored (verified firing at COLLECTION), with a sensor so it cannot happen again to this file and a deferred candidate for the general rule. Sensor scopes tightened where they failed OPEN: CE053's regex missed a nested subpackage and any filename with a digit, both reporting zero violations exactly like a clean tree; CE059 had no non-vacuity assert, so a package rename would have left it green inspecting nothing; `_family_source` iterated the ranks, leaving the ladder-exempt `store.py` outside three whole-family scans; and `_LADDER_EXEMPT` is now asserted a singleton, since two surfaces call `store` "the ONE documented exception" and nothing checked it. Three documentation claims were false rather than merely stale. `pyproject.toml`'s complexity paragraph had its PATHS updated during the package move while keeping the baseline's numbers — it claimed no E-grade where radon reports two. CLAUDE.md said CE053 "lists all six module names" (it is a directory regex) and called the renderer free of any estimator (it imports `bootstrap_p_floor` at runtime, and a test REQUIRES that, because CE040 makes the value derived rather than respelled). A test fixture was quietly lying too: `set_split` replaced `run.json` wholesale and discarded `task_results`, so every fixture that changed a split was exercising the contamination path instead of a clean current run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mit work Integration fixes the rebase onto main exposed, in one place rather than smeared across 175 replayed commits. **The CE035 collision.** Main shipped CE035 as the workflow-output resolver (`tests/lint/workflow_outputs.py`) while this branch shipped CE035 as the split-label rule. A `# noqa` keys on the id, so one suppression would disarm both — exactly what `test_rule_ids_are_unique_across_baserules_and_test_classes` says. The unmerged one renumbers: the split-label rule is **CE060** now, in the test class, CLAUDE.md, `tasks/skills/ci-outcome.yaml`, tutorial 09 and `plan_command`'s pointer. Main's CE035 keeps its number and its file. Main's `.claude/harness-candidates.md` also reserved four numbers this branch has since implemented (CE036, CE037, CE040, CE041) plus CE042 for the sleep-loop candidate. Those reservations move to CE061–CE065, because the backlog is one of the two places CLAUDE.md tells an author to grep before claiming a number, and a reservation pointing at a shipped rule is worse than no reservation. **Main's new code under this branch's new rules.** `task_loader`'s `system_prompt_file` seam (main, #92) is a `model_copy(update={...})`, which CE048 now forbids outside `models/copy_with.py`; it becomes `copy_with(...)`, whose delegation to `model_copy` keeps the validator-skipping the two comments there describe — so the comments were corrected rather than left half-true. **Two conflicts resolved toward keeping both behaviours**, noted here because the replayed commits do not say so: `criteria/agent_judge.py` forces main's `system_prompt_file=None` / `system_prompt_mode="replace"` in the SAME `model_validate` payload as the branch's CE048 conversion (they are mutually exclusive under `validate_assignment=True`, so a sequential assignment raises), and `plan`'s per-variant `validate_run_limits` warnings (main) moved into the branch's `_report_variants`, escaped for CE050. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
uipreliga
force-pushed
the
feat/plugin-optimize-skill
branch
from
August 20, 2026 01:52
ecbabea to
8ea19ce
Compare
`ActivationGateVerdict` and `ExecutionGateVerdict` spelled all 18 of their shared fields twice, in parity maintained by hand — and the parity was already broken: 14 of the 18 carried different `description` text and four differed in required-ness. `separated` and `failed_vetoes` were spelled twice each too, on models whose whole job is to say what a promotion decision rests on. `GateVerdictBase` now holds the 18, with the four statistic fields required as the activation track has them. `ExecutionGateVerdict` re-declares six of them — the four statistics with `default=None`, plus `n_resamples` and `gate_refusal`, whose text this track genuinely says differently — and every re-declaration is licensed with its reason in `_FIELD_OVERRIDES`, the ONE place that set is written down. `ActivationGateVerdict` overrides nothing. A base class, deliberately not a track-discriminated union: each subclass declares only its own extras, so no field arrives as permanently-`None` noise on the track it does not belong to. Both docstrings say so, since the module already argued against the union and a reader would otherwise think the refactor contradicted it. The sensor is two-directional, on the CE038 `EXEMPT` pattern: an unrecorded re-declaration fails, and a recorded one that no longer differs from the base fails too, so a licence cannot outlive the trade it recorded. Detection is `cls.__annotations__`, which is class-local — `model_fields` merges the base in and would read as "declared" on both subclasses. Behaviour-preserving on every field's type, default and constraint. `model_dump()` key order does move, because subclass fields now follow every base field; the verdict pins compare parsed JSON and are left byte-untouched, so no estimator-ledger row is owed for a change that moved no number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both tracks carried their own copy of the promotion loop: `holm_promote` and `holm_promote_execution`, ~110 lines each, 700 lines apart, each with its own `promoted` conjunction. The two had already drifted in spelling — the refusal term read `refusal is None` on one side and `not refused` on the other — and the two trailing notes were appended from different places, one inside the note ladder and one inline. `gate.decide_family` is now the single loop. It owns the `holm_family` and `resamples_for_family` calls, the outside-the-family branch, the ONE `promoted` conjunction, both trailing notes and both `copy_with` calls. Each track supplies only a `decide(verdict, FamilyFacts) -> TrackDecision` hook, returning its refusal and its notes. The refusal is RETURNED from both because the two tracks disagree about where it comes from and agree about what it means: activation computes it from the family's rank-dependent threshold, execution reads the one `execution_gate` already set. Returning it from both is what makes the conjunction one expression rather than two spellings of one. `FamilyFacts` carries no `threshold`, as planned, and no `family_resamples` either — no hook reads it, since `decide_family` emits the resolution note itself and the activation ladder reads `verdict.n_resamples`, a different number. A field written every iteration and read by nobody is the speculative field this tuple already refuses a `threshold` for. Behaviour-preserving, and that claim was falsified rather than asserted: a 10,982-state differential against both old loops — every combination of p, mean_diff and interval sign, pre-set refusal, failing veto per list, discreteness floor, three family sizes, mixed draw counts and three alphas — is byte-identical on notes, order, count, `promoted`, `holm_rejected`, `holm_alpha` and `gate_refusal`. The one delta is `model_fields_set`, documented where it happens. The new sensor pins `family_size` to the MEASURED count, which needed a family with an unmeasured member to say anything: `len(family)` -> `len(verdicts)` passed the entire suite before this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both noise floors opened with the same three guards — reconcile the tree, load the rows, check the split provenance — in the same order, differing only in a rendered label. `gate.floor_preflight` owns them now, and owns the order with them: the reconcile runs BEFORE the load, so a contaminated tree costs no parse and a mistyped variant id still reports the path rather than a stale tree. Split across two functions, that ordering was a comment in each of them. It returns the per-invocation row maps and the split provenance. Both are load-bearing: the activation floor halves the invocations, and both floors key their cached `NoiseFloor` on `provenance.value`, so a preflight that swallowed the second value would serve a train floor to a test lookup. `gate.FirstCause` replaces four identical `nonlocal` refusal closures — one in `execution_gate`, one in each confirm gate, one in `_execution_diagnostics` — three of whose docstrings said they mirrored another, which is a tree asking for one declaration. A class rather than a closure factory because three of the four sites read the value at a distance, from inside a verdict builder called long after the last record; an attribute survives being passed and a captured cell does not. Every refusal message is byte-identical, and the render pins are the witness. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n function Three functions carried most of the family's complexity: `execution_gate` at radon cc 34 over 427 lines, `_execution_diagnostics` at 31, `_dead_weight` at 29. They are now 14, 8 and 8, and the file's worst function is the untouched `_integrity_checks` at 19. The gate's causes were already ordered by precedence in program order, and that order was the only declaration of the rule — a later cause is usually an earlier one's consequence, so reporting the wrong one sends the reader to the wrong remedy. Each cause is now a named stage returning its message, called in that same order, with `FirstCause` keeping the first. The order is still program order; it is now readable in five lines instead of four hundred. `_read_gate_experiment` returns either a resolved comparison or a refusal, and its refusal carries the row counts on the two paths that HAVE them — a verdict that dropped those would hide an eroded sample behind a message about a variant id. Nothing asserted that before; two tests do now. Behaviour-preserving, and falsified rather than claimed: a differential against the previous commit over 33 end-to-end gate scenarios, a 3,840-case diagnostics grid and 17 dead-weight cases is byte-identical on every message, note and number. Fifteen mutations killed every claim the new tests make except two, and both were real gaps: the untested row counts above, and an unreachable fallback arm in the dead-weight note — deleted, with its invariant asserted where the two lists are built rather than guarded where they are read. Extracting the reconcile into `_refuse_stale_tree` made CE053 fire on the gate, since the rule matches call names within one function body. The rule was taught the stage rather than suppressed on the gate — a `# noqa` there would have retired the check on the family's most consequential reader — and because that match is by bare name, a new test asserts every accepted wrapper really calls the primitive reconciler. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`load_and_pair` was radon cc 30 and interleaved five concerns in one body: loading both arms, pairing, dropping rows that scored on only one side, balancing replicate counts, and five notes about why the sample shrank. It is cc 9 now, composed from `_pair_rows`, `_balance_clusters` and two note builders, all private to the module — `_PairedRows` already crosses a boundary CE059 cannot see, and none of these widen that. `n_discordant` moved into the balancing stage because that is what it has to describe. Computed on the raw clusters, a row trimmed from three replicates to one can read as discordant on observations the comparison never saw, and the discreteness floor derived from it would then bound a sample that does not exist. `rows_excluded` stays where it was, at the end, since that is the only point that knows both of its causes. Every note is byte-identical and in the same order, which one comment now states rather than four separated appends implying. `pyproject.toml`'s complexity block is re-measured rather than extended. Under mccabe — the tool the ceiling enforces — the `optimize/` family now tops out at 10 where `execution_gate` alone was 23, and under radon it carries no E- or F-grade at all. The ratchet does not move: the binding function is `isolation/docker_runner.py::_build_argv` at 29, which this plan does not touch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five import rules read `node.module` and none read `node.level`, so `from ..models import X` — the spelling most of `src/` uses — evaded all of them, and the failure was silent: an import rule that never matches reports zero violations exactly like a clean tree. The fix at the time was to route each rule through `resolved_module`. This makes that the DEFAULT rather than a thing each rule remembers. `BaseRule.visit_ImportFrom` resolves the module once and hands it to `check_import`, which rules override. All six real definers moved onto the hook; `ce020` keeps its own `ast.walk` over `node.module` because it matches a third-party package, where a relative import can never resolve and there is no blindness to fix. CE051 is augmented rather than narrowed, because narrowing it would have been a weakening: its scope is all of `tests/`, and the same blindness lives in `test_optimize_layering.py::_coder_eval_imports`, which is not a `BaseRule` and can never use the hook. So the resolver check stays exactly as it was, and a second check under the same id forbids a rule in `tests/lint/rules/` from defining `visit_ImportFrom` at all — closing the opt-out the hook creates. That second check is anchored against vacuity on `base.py` itself: checking that file asserts it still defines both halves of the hook. Renaming `check_import` would otherwise leave a check that forbids the only shape there is, and every rule file in the tree would pass it while nothing was being verified. No caching was added. The resolver now runs for every rule on every import — measured at 0.6% of the lint suite, which `pytest -m lint` timings confirm unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…, and CE044 retires CE039's restricted expression evaluator declared its allowed operators twice: a tuple the AST walk checked membership against, and a `match` that computed them. Two halves of one decision, kept in step by CE044 — because a wildcard arm returning a value would compute an unhandled operator as something else, which is how `ast.Mod` in the whitelist would have been reported as division by the one sensor class whose entire purpose is catching arithmetic that lies. The halves are now one: two dicts mapping an operator type to the function that computes it. Admitting an operator and implementing it are a single edit, so there is no parity left to pin, and CE044 is retired rather than kept as a rule policing a shape that can no longer occur. Both halves converted — the unary arm too, which left hard-coded as `ast.USub()` would still have been a second declaration. What replaces the parity scan is behaviour. Every admitted operator must compute correctly, since a dict cannot be partially implemented but can be mis-mapped: `ast.Sub` pointing at `operator.add` type-checks and passes every other test in the file. An unadmitted operator must raise naming itself, which is what tells prose drift apart from a parser bug. And the two tables are pinned non-empty and disjoint by arity — the anti-vacuity guard for the other two, which would otherwise still pass against emptied tables by raising. The id stays reserved. `runner.py`'s uniqueness assert covers `ALL_RULES` only, so a class-wired rule could claim CE044 with nothing failing, and `make lint` would then report findings under a number whose documented meaning is something else. The guard is now a parametrized register of both directions — CE056 reserved, CE044 retired — beside its rationale in harness-candidates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ontract `tests/test_custom_lint.py` was 9,680 lines and 52 top-level classes, one of which was 3,226 — a third of the file. It is 95 lines now, holding the three runner-level invariants; the per-rule tests live in `tests/lint_tests/`, grouped by what each rule reasons over. `TestPluginArtifacts` became five classes over five modules, sharing a base for the class attributes and grader helpers its methods reached through `self.`. A pure move, and pinned as one: 518 qualified test functions before and after, and `pytest -m lint` collected the same 770 immediately after the split. `test_lint_suite_parity.py` is the standing witness — no test name claimed by two modules, every module importing and holding tests, and the count a floor rather than a literal so adding a lint test is not a chore. Three whole-tree scans had silently narrowed to their own file, which is what this kind of split does when nothing is watching. Each would have kept passing while checking a thirteenth of its subject: the cross-surface rule-id check found zero ids (caught by its own anti-vacuity guard), the normalization-idiom sensor policed one of thirteen modules, and ~25 path constants read `Path(__file__).parent.parent` — correct in `tests/`, one directory short in `tests/lint_tests/`. That last one surfaced as 178 anti-vacuity assertions firing at once, each reporting that the tree it scans had vanished. They now derive from one declared `REPO_ROOT`. `make lint` selected by path and now selects by marker. The two disagreed about what a lint test is, in both directions and invisibly: five classes — CE047, CE050 and CE051 among them — carried no `lint` marker but ran because the target named their file, while `tests/test_lint_no_top_level_run_limits.py` was marked and had never run under `make lint` at all. The five are marked, a test keeps them marked, and `make lint` runs 768 where it ran 749. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The optimize family's docstrings were 40-line essays, and an essay is not read: the contract a caller needs was buried in the history of how the code got there. Those are three different kinds of sentence and they now live in three places — contract in the docstring, why-not-the-obvious-alternative as a comment at the decision site, and what-broke-once in a dated file under `.claude/decisions/`, reached from the code by a one-line pointer. Docstrings over 25 lines in `optimize/` and `reports_optimize.py`: 25 down to 8. `Field(description=…)` blocks over 8 lines in `models/optimize.py`: 9 down to 4. Three of the eight surviving docstrings are module-level and are architectural contract, so they stayed. Nothing was deleted that a caller needs, and nothing was deleted that a sensor reads: `make lint`'s skipped count is identical and its passed count only grew. The field descriptions were trimmed rather than moved wherever they are user-visible in the JSON schema. The convention is prose, deliberately: "is this sentence a contract or a defect history" is a judgement no rule can make, and a heuristic for it would be a rule policing wording. What IS checked is mechanical — no orphaned decision file, no pointer to a file that does not exist, and two counted ratchets so the next essay is a visible decision rather than a drift. The ratchets are ceilings, not equalities: a docstring that resists trimming because every line really is contract should stay, and the README says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
89,090 characters down to 44,908. The file is prepended to the context of every request in every session, so a paragraph nobody reads is paid for on every call. One line was 33,247 characters — 37% of the file — naming 35 CE ids with a rationale essay for each, every one of which already existed in the rule's own module docstring. Duplicated prose is worse than absent prose there, because the copy drifts and a reader cannot tell which is current. It is now the standing instruction plus four bullets: where a rule lives by kind, where its tests live, how to claim a number, and the statement that each rule's own docstring is the authority on what it does and what it deliberately does not catch. Exactly one sentence in that line was a DERIVED surface — CE036's exemption list, bound in both directions by a test — and it is kept verbatim, now introduced by a clause saying why it lives here rather than in the rule, so the next person to trim does not move it. Nine more long lines point at their authoritative home instead of restating it. In every case that home already existed: the early-stop module and the task guide, the harness-parity doc, the module docstrings, and the new decision log. Two assertions guard it. A character ceiling, and — because a ceiling can be satisfied while one line still holds a third of the file — a per-line limit. The long line is also the shape that hides a derived sentence: nobody diffs 33,000 characters on one line, which is how CE036's list drifted from the code inside one in the first place. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The worst of it: Phase 9's docstring trim replaced the specification of `cost_quality_front`, `headroom_ceiling` and `candidate_leaks` with descriptions written from their NAMES rather than their bodies, and Phase 10 then canonized one of those in the decision log. `cost_quality_front` does not rank by a ratio — it is a 2-D Pareto filter gated on row-set coverage, and that conjunct is what stops an arm which crashed 5 of 6 rows from dominating the incumbent. `headroom_ceiling` cannot see other arms and never returns None, so a caller writing the documented `is None` guard got dead code; its real invariant, that the denominator is the FULL row count and not the selected subset, was the line that went missing. `candidate_leaks` takes strings: `skill_text` is the reader, and handed one file the preflight comes back CLEAN — byte-identical to a genuinely clean candidate, which is the worst shape a preflight can have. All three are restored from the code, and the decision log with them. The ratchet that caused it now has slack. It sat exactly at the count while its own README told authors to keep a docstring whose lines are contract — advice that reddened the build. A cost control must not be able to win an argument against a specification. Two sensors were open in the case they were written for. The reserved/retired-id register checked `ALL_RULES` and `tests/lint/rules/`, so a class-wired `TestCE044` — the exact shape its docstring names — passed everything; it now intersects the `TestCE<NNN>` class set too. And CE036's exemption parity read every backticked name in the sentence, so `skill_name` could be deleted from the list and still pass, because the sentence names it again downstream; it reads the parenthesised list now. Both verified by mutation. CE034, CE036 and CE060 hand-rolled `tasks/` discovery, so a `.yml` task was invisible to all three — a leaking `probe.yml` produced zero cases and a green `make lint`. They route through `task_yamls` now, from one declaration with a non-emptiness assert, so a path break fails instead of collecting nothing. The rest is prose that had stopped being true: six stale `optimize/` line counts in the very pyproject block that claims to be re-measured, a function attributed to the wrong module, `no_floor` "called four times" where Phase 3 left one, three disagreeing counts of the execution gate's refusal causes (none of them the right eight), "TWO consumers" of the leak primitive where there are three, and fifteen wiring pointers still naming a file the classes no longer live in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both were live after the lint split, and both fail OPEN — a stale pointer reads exactly like a correct one, which is why fifteen of them survived a phase whose whole subject was moving those files. `<path>::<Name>` wiring references now have to name the file that declares the name. Each is the "where is this rule wired" pointer a reader follows from a detection body to the test that runs it, and a wrong one sends them to a 95-line file holding none of the subject. The decision-log pointers next door were already checked in both directions; the asymmetry is what made this worth closing rather than deferring. And a module nested under `tests/` may no longer reach for the repo root with `Path(__file__).parent.parent`. The expression is CORRECT one directory up, which is what makes it dangerous: about 25 constants carried it into `tests/lint_tests/` unchanged, and the failure was not a red import but 178 anti-vacuity assertions firing at once, each reporting that the tree it scans had vanished. Depth-aware, so it says nothing about the files where the idiom is right. Both mutation-verified. The rest of the run's candidates are deferred with the measurement that shows why they are not cheap — the unread-tuple-field rule reports 15 false positives against 2 real hits, because a field read by tuple unpacking or by a consumer outside the package looks identical to a dead one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

What
Adds
Dataset.split_field+ a--splitrow 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 rowstrain/testand 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--splitis 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:description, measured against an activation suite. Does it fire when it should?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:
suite.jsoncannot report per replicate — hence three separate invocations.weighted_score, whichpaired_comparisonalready computes correctly over replicates it averages per row before pairing — hence--repeats 3on 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:skillat the top ofinitial_prompt— to hold activation constant; anddisable-model-invocation: truenow routes to the execution track rather than hard-stopping, soinitandciare optimizable after all.The slash form is not a stylistic preference. A
disable-model-invocationskill 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 realSkilltool call, and is detected byskill_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:
${CLAUDE_PLUGIN_ROOT}reference, slash command and CLI flag it emits resolves; the experiment YAML in its body validates againstExperimentDefinition.skill_triggereddetects 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 onlyskills/while the prose said "everything that source contained" (a diagram-faithful snapshot mounts skills whose bundledreference/files are missing — invisible on the activation track), and omitting.claude-plugin/plugin.jsonmakes 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
citest — "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.The sibling-regression gate read the wrong metric. Annexation makes the sibling's criterion
expected=yes, observed=no— a false negative. Sinceprecision = tp/(tp+fp), it stays pinned at 1.0 however many requests are stolen. The gate would have been gating on a constant. Now readsrecall.yes.A one-skill candidate snapshot empties the sandbox. A variant's
pluginsblock replaces the task's, so the snapshot is the arm's only skill source. Snapshot one skill and every sibling criterion silently observesno— the sibling gate "passes" by measuring nothing. Snapshots now carry the whole skills tree.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 suitecheck-skillgenerated would have reported recall 0.0 — exactly what the template's own comment calls "reads exactly like a broken skill". Corrected in 7 surfaces, includingci, 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-tasksmeasured 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.taskmisfire that reproduced on both splits turned out to be 2-in-3 variance under replicates. Two agreeing runs were not evidence.analyzemissed "what regressed" deterministically. Full three-stage A/B →a-regressionpromoted, 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).p = 0.038that was a billing limit eating one arm harder than the other.completion_rateis what caught both.skill_triggeredstripsplugin:prefixes, and Claude Code ships its own unscopedinit. Askill_name: "init"criterion would silently score a different skill. Documented incheck-skill.Notes for review
analyze's description change is the one behavioural edit to an existing skill, and it is measured..claude/harness-candidates.md— an all-skipped run exits 0, now reachable by a one-character--splittypo. Fixing it changes exit semantics for every skipped-task path, so it wants a decision rather than a drive-by.make verifygreen: 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/holdout→train/test. Docs and content only — split values are open strings, so no schema change and nothing to migrate;split_fieldstill 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 fromrow_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--splitsafe 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_triggeredthat invalidates two claims made earlier in this description, so those are corrected below rather than quietly edited.skill_triggeredno longer counts an erroredSkillcall as engagement (src/coder_eval/criteria/skill_triggered.py).Any suite where a
Skillcall failed previously scoredyesand will now scoreno. 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 theSKILL.mdwas 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.cisetsdisable-model-invocation: true, and theSkilltool refuses such a call outright:24 of 24 calls failed this way,
result_status: "error"on every one, and no row read theSKILL.mdoff 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: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
skill_triggereddetects a slash-command invocation (it does; scored 1.0)" — that 1.0 was the false positive. The call had errored.initandciare optimizable after all" — true, but only with the snapshot fix: delete thedisable-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
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_rate1.0). The three candidates were solving a problem that did not exist. Stages B and C were correctly never reached. ~55 Sonnet runs, ~$20.reference/templates/outcome.yaml) + a checked-in worked example (tasks/skills/ci-outcome.yaml), runnable in two documented lines.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.-ecommand lines) and restructured into two parts.docs/TASK_DEFINITION_GUIDE.mdupdated — it documented the pre-fix detection rule.make verifygreen: 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_triggerednow requires the signal to have actually delivered. The early-stop watcher evaluates on the tool call, whereresult_statusis stillNone, so an in-flight or crash-force-closedSkillcall live-passed while the frozen check scored itno— 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: aSkillcall counts only on"success"(for that tool the body is the tool result), and aRead/Glob/Grepthat errored or has not resolved does not count — the path is in its parameters, but nothing loaded.Bashstays ungated, becausecat … | grepexits 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.pypinned the opposite semantics as deliberate; each is inverted, not deleted, with the reasoning in its comment, plus aBashtwin for the path that still decides on the call. The cost is stated in the test that pays it: aSkillcall whose result never arrives no longer stops the run, so that run continues to its turn cap. The old behaviour stopped it and scoredyes— crediting a skill whose body never reached the agent.A mistyped
--splitnow exits 2 instead of 0. It used to raise, get caught like any load failure, and land inskipped_tasks: one yellow line, zero evaluations, exit 0 — a CI gate reporting success for a one-character typo.SplitSelectorErrorseparates 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 existingexcept ValueError -> typer.BadParametersupplies the exit.The rest
coder-eval planexpands 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.expand_dataset, a yellow⚠fromplan, and a check inlint-tasksfor adopters CE035 cannot reach. All three callrow_split_label, so the rule cannot drift even though the messages differ.--splitand sampling narrow it. A malformed row in thetesthalf used to validate under every--split trainrun and surface at promotion time — the most expensive moment to learn it.activation.yamlcaps 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.skill_nameexemption. 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_FIELDSis 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-lineSKILL.mdas 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.ci-outcome'sboth-triggersrow 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.optimize-skillentirely. 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.ci-outcome.yamlexcerpts 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-tasksstill holds at F1 1.000 on a suite three rows larger.analyzereads 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. Andhard-3is 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
live_verdictreads, which no AST rule can infer.MergeField; one new exception type (SplitSelectorError), one module logger, one CLI option on an existing command.make verifygreen: 4115 passed, 390 lint checks, coverage 91.63%.