Skip to content

feat(plugin): add optimize-skill's execution track and the outcome-suite template - #130

Open
uipreliga wants to merge 1 commit into
pr/split-mvpfrom
pr/split-execution-track
Open

feat(plugin): add optimize-skill's execution track and the outcome-suite template#130
uipreliga wants to merge 1 commit into
pr/split-mvpfrom
pr/split-execution-track

Conversation

@uipreliga

Copy link
Copy Markdown
Collaborator

PR 2 of 3 extracted from #109. Based on pr/split-mvp (#129), not main, so the diff
above is exactly this PR's own slice rather than a cumulative one. It merges after #129.

⚠️ This PR has no CI until it is rebased

Every gating workflow filters on the base branch (branches: [main, develop], and
develop does not exist here), so a PR based on another PR's branch triggers no CI at
all
. That is deliberate — the alternative was basing on main and showing a ~4.7k-line
cumulative diff.

What makes it safe to review now: this exact tree was verified locally against the full
gate — 376 files already formatted, ruff check clean, pyright 0 errors, 0 warnings,
and 4759 passed, 0 failed.

Before merging, this branch gets rebased onto main and force-pushed. The repo's
strict_required_status_checks_policy requires being up to date anyway, and that
force-push is the synchronize event that finally runs all the required checks. Do not
merge until they are green.

Blast radius

One product change. criteria/skill_triggered.py: an errored Skill call is no longer
engagement
(3b2802b). A refused or failed call loaded no skill body, so counting it inflates
the activation metric on precisely the rows where the measurement itself failed. This matters for
ci, which sets disable-model-invocation: true and therefore makes the Skill tool refuse.

Everything else is skill prose, a bundled template, a sample task suite, a fixture and two
tutorials:

optimize-skill/SKILL.md the execution track
reference/templates/outcome.yaml + outcome-rows.jsonl the bundled suite users copy
tasks/skills/ci-outcome.yaml + -rows.jsonl, templates/ci-outcome-fixture/** the worked example, run against our own ci skill
docs/tutorials/09-optimizing-a-skill-body.md the round, reported as a null result
orchestration/task_loader.py the tune/holdouttrain/test split-value rename
.gitignore ignores .optimize-skill/, the skill's multi-megabyte per-arm working tree

The split-value rename (6e9c1b4) landed before any of this shipped — tune/holdout were
never in a release, so reviewers seeing those values disappear are seeing a rename, not a
breaking change.

The correction — two lint rules were squatting on main's numbers

The branch numbered its two new dataset rules CE035 and CE036. main already owns both:
TestCE035WorkflowOutputParity (tests/lint/workflow_outputs.py) and
TestCE036LiveVerdictContract (tests/lint/live_verdict_contract.py). This PR renumbers the new
pair to CE060 and CE061 — the numbers they carry at the branch tip, so no later PR in the
stack has to move an id.

Nothing in the build could have caught this, which is the more important half. From
CLAUDE.md:

Claiming a number. tests/lint/runner.py's id-uniqueness assert covers ALL_RULES ONLY, so
a class-wired id can collide with a BaseRule's — or with a number that is RESERVED (CE056) or
RETIRED (CE044) — without failing anything.

So this PR also adds test_rule_ids_are_unique_across_baserules_and_test_classes, which reads
the TestCE<NNN> class names — the one place every rule surfaces, since a BaseRule has a test
class and a class-wired rule is one. It was observed failing on ['CE035', 'CE036'] before the
renumber and passing after.

Why it matters beyond tidiness: a # noqa keys on the id string, so while two rules shared a
number one suppression would have disarmed both — including main's live-verdict contract.

The renumber reaches five files, because two of them are user-facing: tutorial 09 tells the
reader CE035 / CE036 fails the build on a partly-labelled dataset and on a verbatim leak, and
tasks/skills/ci-outcome.yaml names the rules that load tasks/**/*.yaml. Left at the old
numbers, each of those sentences would point a reader at an unrelated rule on main.

The renumber was applied by explicit occurrence, never by pattern. A global s/CE036/CE061/g
would have renamed main's TestCE036LiveVerdictContract, orphaning its contract file and
silently retiring a live rule. main's six CE034/CE035/CE036 mentions in CLAUDE.md sit on one
line; exactly three moved. Of the six mentions in .claude/harness-candidates.md, exactly one —
the "Semantic answer-leak" bullet — refers to this branch's rule, and only that one moved.

What this PR does not do

  • Tutorial 09 is a null result, and says so. The execution-track A/B did not produce a
    significant winner. It is published as a documented method with an honest outcome, not as a
    success story.
  • No gate. There is still no significance test and no promotion rule behind either track —
    Stage B remains a human reading runs. The gate is not in this wave.
  • No coder_eval.optimize package.

Verification

ruff format --check (375 files), ruff check, pyright 0 errors, and the full suite green:
4753 passed, 104 skipped — the checkpoint's 4752 plus exactly one, the new guard.

The 90-skip rise versus PR1 is benign and expected: CE061 parametrises per task file and skips
every task that carries no dataset: block.

…ite template

Adds the second half of `/coder-eval:optimize-skill`: the EXECUTION track, which asks
whether a skill's BODY produces better outcomes, next to the activation track's
question of whether its DESCRIPTION gets the skill engaged.

Like PR1 this is mostly prose — the skill's execution track, the bundled
`outcome.yaml` template, the `ci-outcome` sample suite and its fixture, and
tutorial 09. Tutorial 09 reports a NULL result and says so.

One product change: an errored `Skill` call no longer counts as engagement
(3b2802b). A refused or failed call loaded no skill body, so scoring it as
engagement inflates the activation metric on exactly the rows where the
measurement failed.

Corrections applied during the split:

  * The two lint rules this range introduces were numbered CE035 and CE036 on the
    branch, and `main` already owns both — `TestCE035WorkflowOutputParity` and
    `TestCE036LiveVerdictContract`. They are renumbered CE060 and CE061 here, the
    numbers they carry at the branch tip, so no later PR in the stack has to move
    an id. The rename reaches five files, two of them user-facing: tutorial 09 and
    the ci-outcome sample both name the rules by number.

  * `test_rule_ids_are_unique_across_baserules_and_test_classes` is pulled forward
    into this PR — the one that introduces the two ids. `runner.py`'s import-time
    uniqueness assert covers `ALL_RULES` only, and roughly a third of the CE rules
    are `@pytest.mark.lint` classes whose id lives in a class NAME, so nothing in
    the build could see this collision. The guard was observed failing on
    ['CE035', 'CE036'] before the renumber and passing after.

Squashed from feat/plugin-optimize-skill:

  35e7714 feat(plugin): add an execution track to optimize-skill — optimize the body, not just the description
  9526d54 fix(plugin): repairs from live-testing optimize-skill with cold agents
  2a0d72d docs(tutorial): conform 08 to the tutorial front-matter and title convention
  6e9c1b4 refactor(dataset): rename the split values tune/holdout to train/test, and label them for the user
  6ebe2d9 docs(tutorial): use blockquotes, not mkdocs admonitions, in tutorial 08
  a50a356 fix(plugin): 1/8 — correct optimize-skill's execution track and cost table
  a90f849 feat(plugin): 2/8 — ship the outcome-suite template for the execution track
  17aa999 fix(docs): 3/8 — repair the stale skill counts, and the sensor that missed them
  b910adc feat(tasks): 4/8 — add the ci-outcome sample, the execution track's worked example
  f252e6e feat(lint): 8/8 — CE035 catches the partly-labelled split dataset
  b375fb8 fix(plugin): 5/8 — execution-track A/B returns a NULL result, and corrects the  slash-form claim it disproved
  c0d6959 docs(tutorial): 6/8 — add tutorial 09, the content track, reported as a null
  eb758e4 docs(tutorial): 7/8 — make tutorial 08's A/B reproducible, and untangle it
  d5b4fb0 fix: code review fixes for the optimize-skill plan
  c2315ed test(lint): CE036 — a row's prompt must not contain what its criteria grade
  95553ec docs(harness): defer three gaps found reviewing the optimize-skill plan
  3b2802b fix(criteria): an errored Skill call is not engagement
  2ddb737 docs: correct tutorial 09 — the round's null had a different cause than reported
  5f14f73 docs(tutorial): 09 gets the real baseline — a verified ceiling, 1.000 on 6/6
  c40cdd4 fix(tasks): gate engagement, in the artifacts that call it a gate
  bc9125b docs(tasks): make the ci-outcome sample runnable in two lines, and record the dead end
  3ce116b chore: close the last loose ends from the optimize-skill plan

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

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown

Claude finished @uipreliga's task in 2m 8s —— View job


Code Review for PR #130

Review Checklist

  • Read .github/code_review.md for review standards
  • Review full diff against origin/pr/split-mvp
  • Check product change: errored Skill calls no longer count as engagement
  • Verify lint rule renumbering: CE035/CE036 → CE060/CE061
  • Review optimize-skill implementation and execution track
  • Check outcome-suite template and related files
  • Review tutorials and documentation updates
  • Verify cross-file consistency
  • Check for missing components or edge cases

Reviewing changes now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant