Skip to content

fix(plugin): correct the skill-reachability path — every generated activation suite reports recall 0.0 - #143

Merged
uipreliga merged 4 commits into
mainfrom
feat/reuse-pr109-cheap-wins
Aug 28, 2026
Merged

fix(plugin): correct the skill-reachability path — every generated activation suite reports recall 0.0#143
uipreliga merged 4 commits into
mainfrom
feat/reuse-pr109-cheap-wins

Conversation

@uipreliga

@uipreliga uipreliga commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

The bug

Six surfaces told users to point SKILL_SOURCE_PATH at .claude/skills. A local plugin path must be a plugin root — a directory holding skills/ — so the skill resolves at <path>/skills/<name>/SKILL.md. One level too deep loads nothing at all.

Probed against the real CLI, one skill at <root>/.claude/skills/probe-alpha/SKILL.md:

claude --plugin-dir <root>/.claude/skills   ->  NONE  (never loads)
claude --plugin-dir <root>/.claude          ->  `.claude:probe-alpha`

So every activation suite /coder-eval:check-skill generates today reports recall 0.0 — what the bundled template's own comment calls "reads exactly like a broken skill." And /coder-eval:ci wrote the same path into users' scheduled workflows, where it renders as a permanent red indistinguishable from the drift the schedule exists to detect.

Corrected across five files: check-skill, ci, activation.yaml, docs/PLUGIN.md, and tutorial 07 (its guidance line, plus its troubleshooting row, which had blamed only an unset variable). Review turned up three more surfaces with the same bug — see below.

Delivery note. plugin.json's explicit version pins the plugin: installed users get this only when that string moves. These are fix: commits, so semantic-release cuts a patch and the pin follows. A chore:/docs: fix to the plugin tree would not reach anyone.

Why it survived in six places at once — a harness divergence

docs/agents/HARNESS_PARITY.md gains a section. claude-code requires a plugin root; codex_agent._setup_skills and antigravity_agent._resolve_skills_paths both scan the bare and the nested layout and take whichever holds a <skill>/SKILL.md.

So .claude/skills works on two backends out of three and fails silently on the third. Per the parity rule a divergence is either fixed or documented — this one was neither.

CE045 keeps it fixed

Repo convention: a fixed bug becomes a lint rule when the root cause is mechanically detectable. SKILL_SOURCE_PATH must never name a directory whose last segment is skills.

It globs the surfaces rather than listing them, so a seventh is caught by existing rather than by being remembered. Mutation-guarded on both the rejected and accepted forms, and verified to fail with file and line when the pre-fix value is reintroduced.

Authored as CE044; renumbered on rebase after #141's merge claimed that id for the manifest-parity rule. The two are complementary — CE044 checks the manifests agree with each other, CE045 checks the path names a plugin root. Both green.

CE045 keys on SKILL_SOURCE_PATH only. That is a limit of the rule's reach, not a licence to use the deeper form elsewhere: $PLUGIN_PATH feeds experiments/plugin-comparison.yaml, whose default agent is claude-code, and is unlinted. The guard that reaches every user is the new runtime warning.

Salvaged from the closed PR #109

Only what stands alone — nothing depending on its optimize/ subsystem or Dataset.split_field.

Descriptions. Two carry evidence: analyze is #109's A/B-promoted a-regression variant (train 1.000 vs 0.667 non-overlapping, test 1.000 vs 0.909) and adds the "what regressed" trigger it deterministically missed; lint-tasks' trim was measured at ceiling. The check-skill/init/task trims are tighter prose, not individually A/B'd — stated so nobody later reads them as validated.

Listing budget 1,576 → 1,351 of 1,600. That headroom is the point: the budget is shared with every skill the user has installed.

A silent-wrong-measurement warning in check-skill. skill_triggered matches on the bare name and strips plugin: prefixes, and Claude Code ships its own unscoped init — a colliding skill_name does not error, it credits whichever skill fires.

Deliberately not taken. #109's --split guidance, its run_limits/setting_sources template notes, its threshold-currency warning, and its "plan expands the dataset" claim. Each was checked against main and is absent — porting the prose would document behavior this tree does not have.

One test unpinned from ordering

test_install_failure_is_not_latched patched signal.signal module-wide for its whole duration, so the async teardown's own signal.signal(SIGINT, default_int_handler) restore hit the refusing mock and raised ValueError out of teardown. Latent — it does not fail on today's sharding, but any change to how tests distribute surfaces it, and one did. The patch is now lifted right after the calls under test, and the count assertion became a comparison of the two calls' signal sets.

Not in this PR

The repo's litellm/ directory is importable as a namespace package, so import litellm succeeds and returns an empty module — which is why tests/test_judge_litellm.py fails instead of skipping without the optional extra. Ten failures on a clean checkout, invisible to CI because CI installs it. Left for a separate change; work parked on fix/litellm-namespace-shadowing.

Verification

  • make lint — exit 0, 382 passed (CE044 and CE045 both green)
  • make test — the same 10 pre-existing test_judge_litellm.py failures as clean main, and nothing else
  • ruff check + format clean
  • claude plugin validate --strict; coder-eval plan on the edited template

🤖 Generated with Claude Code

https://claude.ai/code/session_01V5WZ1BbtygK9YVvcgPfNGh


Review round (commit 754805b)

Every claim was reproduced before acting. Three proved my own text wrong.

Both blockers fixed. CE045's [^"'\s]+ stopped at the first space, so SKILL_SOURCE_PATH=${{ github.workspace }}/.claude/skills captured ${{ — the ci surface, which the rule's own docstring called the highest-cost one, was structurally unreachable, and a companion test asserted that truncation as correct. The mutation guard is now four verbatim pre-fix lines replayed through the full extract→predicate pipeline; verified to flag the reintroduced ci line. Writing it that way caught two further bugs of mine (findall reports unmatched groups as "", not None; the YAML walk mislabelled fixture paths).

A plugin root loads the whole plugin. Verified: a root with skills/probe-beta/, agents/probe-subagent.md and commands/probe-cmd.md offers all three. Pointing a suite at .claude hands the agent every project subagent — one that answers the request instead of the skill depresses recall for unrelated reasons and makes the number repo-dependent. check-skill and the template now prescribe a minimal staged root; ci carries the workflow step to build one.

Three claims of mine were false: PLUGIN_PATH is not Codex-only; the parity probe gave a false negative from a repo root (Claude Code discovers ./.claude/skills/ natively, so the deeper path still lists the skill — the namespace prefix is the real signal); and "tutorial 07 twice" was one line.

Three more surfaces carried the bug, including docs/AB_EXPERIMENTS.md's canonical A/B recipe, whose with-skill arm loaded nothing and silently compared two identical arms. Also fixed: experiments/default.yaml, experiments/plugin-comparison.yaml, plus docs/agents/CLAUDE_CODE.md and CLAUDE.md's divergence index.

A runtime guard. process_plugins warned only on an undefined env var; a path that exists but has no skills/ reached the SDK silently. Codex and Antigravity already log the equivalent; claude-code — where it is fatal — did not. Now it does, with four tests. That reaches user repos, which a repo-scoped lint rule structurally cannot.

Deferred: namespaced skill_name matching, the sibling tests' process-wide signal.signal patches, and the proposed CE046/CE047 meta-rules — real, but their own changes. The migration note for already-emitted user suites needs a release decision first.

On the red check

Live Integration Tests (Settings Enforcement + Cost Budget)test_broad_deny_limits_agent_to_sandbox failed with Agent did not attempt to read outside the sandbox. Calls: []. The test needs the agent to attempt the read so the deny rule can be shown to block it; the model declined on its own ("the file name and directory name … make it clear this is a sandbox escape test"). A model-behavior flake — main is green across its last six pr-checks runs and this diff touches nothing near settings enforcement. Separately worth noting: the test is fragile by construction, since it depends on the model choosing to misbehave.

uipreliga and others added 2 commits August 27, 2026 14:54
… measured descriptions

Salvages the parts of the closed PR #109 that stand alone — no dependency on
its `optimize/` subsystem or `Dataset.split_field`.

## The reachability bug (the reason this branch exists)

Six surfaces told users to point `SKILL_SOURCE_PATH` at `.claude/skills`. A
local plugin path must be a plugin ROOT — a directory holding `skills/` — so
the skill resolves at `<path>/skills/<name>/SKILL.md`. One level too deep loads
nothing at all.

Every activation suite `check-skill` generates today therefore reports recall
0.0, which the template's own comment calls "reads exactly like a broken skill".
`ci` was writing the broken path into users' scheduled CI workflows, where it
produces a permanent red indistinguishable from the drift the schedule exists to
detect.

Verified against the real CLI rather than taken from the closed PR, two layouts
over one probe skill at `<root>/.claude/skills/probe-alpha/SKILL.md`:

    claude --plugin-dir <root>/.claude/skills   ->  NONE (never loads)
    claude --plugin-dir <root>/.claude          ->  `.claude:probe-alpha`

Corrected in all six: `check-skill`, `ci`, the `activation.yaml` template,
`docs/PLUGIN.md`, and tutorial 07 (guidance + its troubleshooting row, which
blamed only an unset variable).

## Bare-name collision hazard (check-skill)

`skill_triggered` matches on the bare name and strips `plugin:` prefixes, and
Claude Code ships its own unscoped `init`. A colliding `skill_name` does not
error — it credits whichever skill fires. Ported PR #109's warning to say so.

## Descriptions

Took PR #109's six-skill set. Two carry evidence: `analyze` is its promoted
`a-regression` variant (train 1.000 vs 0.667 non-overlapping, test 1.000 vs
0.909) and adds the "what regressed" trigger it deterministically missed;
`lint-tasks`' trim was measured at ceiling (F1 1.000 both splits). The
`check-skill`/`init`/`task` trims are tighter prose, NOT individually A/B'd —
recorded here so nobody later reads them as validated.

Listing budget 1,576 -> 1,351 of 1,600. That headroom is the point: the budget
is shared with every skill the user has installed.

## Deliberately not taken

PR #109's `--split` guidance, its `run_limits`/`setting_sources` template notes,
its threshold-currency warning, and its "plan expands the dataset" claim all
describe code that PR added. Each was checked against `main` and is absent, so
porting the prose would document behavior this tree does not have.

Verified: `make lint` exit 0 (371 passed), `claude plugin validate --strict`,
`coder-eval plan` on the edited template. `make test` shows 10 pre-existing
failures in `tests/test_judge_litellm.py` that reproduce on clean `main` — the
repo's own `litellm/` directory shadows the installed package when pytest runs
from the root. This diff is Markdown and YAML only.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V5WZ1BbtygK9YVvcgPfNGh
…st from ordering

Three follow-ups to the skill-reachability fix in this branch's first commit.

## CE045 — a claude-code plugin path must be a plugin root

(Authored as CE044; renumbered on rebase — #141's merge claimed that id for the
manifest-parity rule, a different check on the same plugin tree.)

Repo convention says a fixed bug should become a lint rule when its root cause is
mechanically detectable. This one is: `SKILL_SOURCE_PATH` must never name a
directory whose last segment is `skills`, because a plugin's skills live at
`<path>/skills/<name>/SKILL.md` and one level deeper loads nothing at all.

Six surfaces drifted to the wrong value together precisely because nothing held
them in agreement. The rule globs the surfaces rather than enumerating them, so a
seventh is caught by existing rather than by being remembered. It carries a
mutation guard pinning both the rejected and the accepted forms, and it was
verified to fail with file and line when the pre-fix value is reintroduced.

## The plugin-path harness divergence

`docs/agents/HARNESS_PARITY.md` gains a section for it. claude-code REQUIRES a
plugin root; `codex_agent._setup_skills` and
`antigravity_agent._resolve_skills_paths` both scan the bare and the nested
layout and take whichever holds a `<skill>/SKILL.md`. So `.claude/skills` works
on two backends out of three and fails silently on the third — which is how the
wrong value survived in six places. Per the parity rule a divergence is either
fixed or documented, and this one was neither. That is also why CE045 keys on
`SKILL_SOURCE_PATH` and leaves the Codex docs' `PLUGIN_PATH` alone.

## test_install_failure_is_not_latched

A latent ordering hazard, found while investigating an unrelated schedule change.
It does not fail on today's `-n auto` sharding, and it is not a flake: any change
to how tests distribute across workers can surface it, and one did.

`signal.signal` was patched module-wide for the test's whole duration, so the
async teardown's own `signal.signal(SIGINT, default_int_handler)` restore hit the
refusing mock and raised `ValueError` out of teardown — failing the test for
something it does not test. The patch is now lifted as soon as the two calls
under test are made.

The count assertion is also replaced with a comparison of the two calls' signal
SETS. A running total of 4 cannot distinguish a latch bug from an unrelated call
landing in the same list; the set form tolerates a stray duplicate while still
proving the retry, since a latched install would record nothing on the second
call.

## Not in this PR

The repo's `litellm/` directory is importable as a namespace package, so
`import litellm` succeeds and returns an empty module — which is why
`tests/test_judge_litellm.py` fails with `ModuleNotFoundError: No module named
'litellm.types'` for anyone without the optional `litellm` extra, instead of
skipping. Ten failures on a clean checkout, invisible to CI because CI installs
the extra. Deliberately left for a separate change; the work is parked on
`fix/litellm-namespace-shadowing`.

Verified: `make lint` exit 0 (CE044 and CE045 both green); `make test` shows the
same pre-existing litellm failures as clean main and nothing else; ruff check and
format clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V5WZ1BbtygK9YVvcgPfNGh
@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

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


Code Review in Progress

Todo List:

  • Read .github/code_review.md for review standards
  • Read CLAUDE.md for project conventions
  • Review full diff (git diff origin/main...HEAD)
  • Read and analyze each changed file with context
  • Check cross-file consistency
  • Perform design-level scrutiny
  • Post comprehensive review feedback

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Review: coder_eval — pr:143

Scope: pr:143 · branch feat/reuse-pr109-cheap-wins · fa68920 · 2026-08-27T22:03Z · workflow variant

Change class: simple — corrects a documented plugin path (.claude/skills.claude) across six surfaces and adds a test-only CE045 lint rule that pins it; no src/ code path changes, and the rule's matcher carries its own mutation guard

Types, security, error handling, and API surface are effectively clean (four axes at 10.0) and the change itself is a genuine improvement, but the measurement layer is the real risk — the prescribed .claude plugin root injects sibling subagents into the evaluated sandbox, a misconfigured plugin path stays silent on claude-code alone, and the new CE045 rule's regex, globs, and tasks-walk each have a verified blind spot at the exact surface it was written to guard — so the bottom line is a healthy codebase whose eval-fidelity and lint-coverage gaps should be closed before more activation suites are generated from these templates.

Summary

Axis Score 🔴 🟠 🟡 🔵 Top Issue
1. Code Quality & Style 8.8 / 10 0 1 0 2 CE045's _ASSIGNMENT regex truncates at whitespace, so the ci skill's GitHub Actions ${{ ... }} form is never checked (and matcher tests bake the gap in)
2. Type Safety 10 / 10 0 0 0 0
3. Test Health 8.9 / 10 0 0 2 1 CE045's "globbed, not enumerated" claim overclaims: _GLOBS omits root-level *.md and .github/workflows, so canonical SKILL_DOC_SURFACES go unscanned
4. Security 10 / 10 0 0 0 0
5. Architecture & Design 9.5 / 10 0 0 1 0 Plugin-path ripple stopped at HARNESS_PARITY: CODEX.md and the per-agent config references a task author reads still describe a bare skills dir, and CE045's exemption keys on the variable NAME ($PLUGIN_PATH also feeds a claude-code experiment)
6. Error Handling & Resilience 10 / 10 0 0 0 0
7. API Surface & Maintainability 10 / 10 0 0 0 0
8. Evaluation Harness Quality 7.9 / 10 0 1 2 1 The prescribed plugin root (.claude) loads the entire plugin tree — commands/, agents/, hooks/, workflows/ — into the evaluated sandbox, and none of the six changed surfaces carries a caveat

Overall Score: 9.4 / 10 · Weakest Axis: Evaluation Harness Quality at 7.9 / 10
Totals: 🔴 0 · 🟠 2 · 🟡 5 · 🔵 4 across 8 axes.

Blockers

  1. [Axis 1] CE045's _ASSIGNMENT regex truncates at whitespace, so the ci skill's GitHub Actions ${{ ... }} form is never checked (and matcher tests bake the gap in) (tests/test_custom_lint.py:3392) — The rule's value extractor is _ASSIGNMENT = re.compile(r"""SKILL_SOURCE_PATH\s*=\s*[\"']?([^\"'\s]+)[\"']?""") (line 3392). [^"'\s]+ stops at the first whitespace, so the Actions form SKILL_SOURCE_PATH=${{ github.workspace }}/.claude yields the captured value ${{ — never the path. Replaying the rule verbatim against the pre-fix tree (origin/main) proves it: it CATCHES docs/PLUGIN.md:112, docs/tutorials/07-plugin-in-claude-code.md:137, plugins/coder-eval/reference/templates/activation.yaml:17 and plugins/coder-eval/skills/check-skill/SKILL.md:172, and MISSES plugins/coder-eval/skills/ci/SKILL.md:156 SKILL_SOURCE_PATH=${{ github.workspace }}/.claude/skills — which the class docstring itself names as the highest-cost surface ('ci wrote the same path into users' SCHEDULED workflows'). Two tests then cement the hole rather than expose it: line 3461 asserts self._is_skills_dir("${{ github.workspace }}/.claude/skills"), a string the matcher can never hand to _is_skills_dir; and line 3473 asserts the Actions line's captured value merely startswith("${{"), i.e. the truncation is written down as correct. Fix: strip ${{ ... }} expressions before capture (or make the value pattern whitespace-tolerant up to end-of-line / quote), then re-point line 3461 through the real pipeline — assert that _ASSIGNMENT.findall('SKILL_SOURCE_PATH=${{ github.workspace }}/.claude/skills') produces a value _is_skills_dir rejects, so the mutation guard exercises regex + predicate together instead of the predicate alone. Also consider adding .github/workflows/** to _GLOBS, since that is where the emitted snippet lands.
  2. [Axis 8] The prescribed plugin root (.claude) loads the entire plugin tree — commands/, agents/, hooks/, workflows/ — into the evaluated sandbox, and none of the six changed surfaces carries a caveat (plugins/coder-eval/reference/templates/activation.yaml:13) — The template now instructs # broken skill. \path` must be a PLUGIN ROOT: a directory holding a `skills/`(line 13) and# export SKILL_SOURCE_PATH=/abs/path/to/.claude(line 19), feedingpath: "$SKILL_SOURCE_PATH"(line 26).--plugin-dirloads an entire plugin, not just itsskills/. Verified against the real CLI (2.1.248): with a root containing skills/probe-beta/, agents/probe-subagent.mdandcommands/probe-cmd.md, claude --plugin-dir -p "list subagents and /probe commands"returnedroot:probe-subagentunder the Task tool and/root:probe-cmd. A standard user repo's .claude/holdsagents/andcommands/next toskills/, so every generated activation suite now offers the evaluated agent every project subagent. Concrete false negative: a repo with .claude/agents/pdf-expert.mdmeasuring.claude/skills/pdf-forms— the agent delegates toroot:pdf-expertinstead of callingSkill(pdf-forms), skill_triggeredrecordsobserved='no', and recall drops for a reason that has nothing to do with the skill's description. That is the same 'reads exactly like a broken skill' failure the fix is chasing, made partial rather than total, and it makes the recall number repo-dependent and non-comparable across suites. None of the six updated surfaces mentions it. Fix: have check-skillstage a MINIMAL plugin root — a scratch dir containing onlyskills/(symlink or copy) — and pointSKILL_SOURCE_PATHat that, so the plugin root contains exactly the unit under test; at minimum, state in the template comment and incheck-skill/SKILL.md` that everything else under the chosen root (subagents, commands, hooks) also becomes visible to the evaluated agent.

Non-blocking, but please consider before merge

  1. *[Axis 3] CE045's "globbed, not enumerated" claim overclaims: _GLOBS omits root-level .md and .github/workflows, so canonical SKILL_DOC_SURFACES go unscanned (tests/test_custom_lint.py:3396) — _GLOBS = ("plugins/coder-eval/**/*.md", "plugins/coder-eval/**/*.yaml", "docs/**/*.md") sits under the comment "Globbed, not enumerated: a seventh surface must be caught by existing, not by remembering to add it here" (lines 3394-3395), but the generated artifact lands outside all three patterns. plugins/coder-eval/skills/check-skill/SKILL.md:142-149 ("## Step 5 — Write the suite … Copy the two template files into the user's task tree") copies reference/templates/activation.yaml — including its # export SKILL_SOURCE_PATH=/abs/path/to/.claude comment at line 19 — into the repo's task tree. In this repo that is tasks/, which the first test never reads (the second test reads tasks/ but only parses agent.plugins[].path, never the SKILL_SOURCE_PATH comment). .github/workflows/** and root-level *.md are likewise unscanned. Fix: add tasks/**/*.yaml, tasks/**/*.yml, .github/workflows/*.y*ml and *.md to _GLOBS — the scan is a cheap line-regex, so widening it costs nothing and makes the "globbed, not enumerated" claim true.
  2. [Axis 3] CE045 only lints repo-shipped strings; a user-supplied SKILL_SOURCE_PATH pointing at a skills dir (or at a path that does not exist) stays silent on claude-code, while codex and antigravity both warn (tests/test_custom_lint.py:3357) — CE045 checks strings the repo itself ships. The failure it describes — "every activation suite the plugin generated reported recall 0.0" (docstring line 3369) — is triggered by a value the USER supplies via SKILL_SOURCE_PATH, and nothing in the harness detects it. src/coder_eval/utils.py::process_plugins (lines 70-82) warns only for an undefined env var: log.warning(f"Plugin path contains undefined environment variable ${var_name}: {path}"). A path that resolves and exists but has no skills/ subdirectory — precisely .claude/skills — passes through to the SDK with no signal at all. The other two backends already do the loud thing for the same condition: src/coder_eval/agents/codex_agent.py:1136 logs f"0 skills linked into {agents_skills_dir} despite …", and src/coder_eval/agents/antigravity_agent.py:302 logs "Plugin skills path did not resolve: …". Fix (additive, testable in one unit test): in process_plugins, when a type: local plugin path exists but Path(expanded)/"skills" is not a directory, emit a warning naming the plugin-root contract — then assert it with a tmp_path test that builds <root>/skills/demo/SKILL.md (no warning) and <root>/demo/SKILL.md (warning). That covers every user, which the lint rule structurally cannot.
  3. [Axis 5] Plugin-path ripple stopped at HARNESS_PARITY: CODEX.md and the per-agent config references a task author reads still describe a bare skills dir, and CE045's exemption keys on the variable NAME ($PLUGIN_PATH also feeds a claude-code experiment) (docs/agents/HARNESS_PARITY.md:139) — The new parity section closes with an exemption whose premise is false in this repo. docs/agents/HARNESS_PARITY.md:138-140:
`SKILL_SOURCE_PATH` — the variable `/coder-eval:check-skill` emits — is held to this by
lint rule CE045; `PLUGIN_PATH` in the Codex docs is deliberately outside that rule,
since a skills directory is valid there.

(the same claim is repeated in the lint rule's docstring, tests/test_custom_lint.py:3382-3384). PLUGIN_PATH is not Codex-only: experiments/plugin-comparison.yaml:22 sets type: claude-code in defaults.agent, and its with-plugin variant at line 37 sets path: "$PLUGIN_PATH". A reader who follows this page and exports PLUGIN_PATH=~/repo/.claude/skills loads no plugin at all on that experiment, and the with-plugin vs without-plugin comparison silently measures two identical arms — the same invisible failure the page was written to prevent, one env var over.

Two fixes, both cheap: (a) drop the blanket name-based exemption and scope it to the consuming harness (tasks/agents/codex_skills_test.yaml:15 is Codex; experiments/plugin-comparison.yaml:37 is not); (b) close the class at the shared runtime seam instead of in prose — utils.process_plugins (src/coder_eval/utils.py:42-86, reached from claude_code_agent.py:1184) already warns on an undefined env var but never checks the resolved directory, while BOTH tolerant harnesses already log this exact condition (codex_agent.py:1137-1142 "0 skills linked ... check the plugin path points at a skills repo root"; antigravity_agent.py:320-323 "0 skills discovered under %s"). claude-code — the one harness where the deep path is fatal — is the only one with no such warning, and a lint rule that runs only in this repo cannot help the user repos where /coder-eval:check-skill actually writes these suites.
4. [Axis 8] CE045's tasks/ walk checks nothing: the blanket $-in-path exemption skips the only eligible entry, so the whole YAML walk is unexercised and a literal .../.claude/skills tail behind a variable passes (tests/test_custom_lint.py:3446) — test_literal_plugin_paths_in_tasks_are_plugin_roots skips on if "$" in value: with the comment # an env var's value is covered by the assignment check above (lines 3446-3447), but the assignment check's globs are _GLOBS = ("plugins/coder-eval/**/*.md", "plugins/coder-eval/**/*.yaml", "docs/**/*.md") (line 3396) — neither tasks/** nor .github/workflows/** is scanned, so the claimed coverage does not exist. Two concrete recurrences pass CE045 today: (a) a task YAML writing path: "$REPO_ROOT/.claude/skills" — the literal /skills tail is visible and _is_skills_dir would flag it, but $ short-circuits the check; (b) a task YAML with path: "$SKILL_SOURCE_PATH" (the template's own shape, activation.yaml:26) whose assignment lives in a Makefile, a workflow, or a README — no surface CE045 reads. Fix: in the tasks walk, only skip when the value is a BARE variable reference (re.fullmatch(r'\\$\\{?\\w+\\}?/?', value)) and otherwise strip the $... prefix and apply _is_skills_dir to the literal tail; and add .github/workflows/**/*.yml to _GLOBS so a workflow-level assignment is covered.
5. [Axis 8] The HARNESS_PARITY reproduction command gives a false negative when run from a repo root — Claude Code discovers ./.claude/skills/ natively regardless of --plugin-dir, so both probes list the skill (docs/agents/HARNESS_PARITY.md:133) — The doc says Probe it in one command and gives claude --plugin-dir "$(pwd)/.claude" # loads .claude:<skill> / claude --plugin-dir "$(pwd)/.claude/skills" # loads nothing (lines 132-133). Run from the natural place — a repo root whose cwd contains .claude/skills/ — the second command still lists the skill, because Claude Code's own project-scoped skill discovery picks it up independently of --plugin-dir. Verified: in a dir containing .claude/skills/probe-alpha/SKILL.md, claude --plugin-dir "$(pwd)/.claude" listed both probe-alpha and .claude:probe-alpha, while claude --plugin-dir "$(pwd)/.claude/skills" still listed probe-alpha. Only with the skill moved outside a .claude cwd (root/skills/probe-beta, run from a sibling dir) does the contrast hold: root:probe-beta vs. nothing. So an engineer who doubts their config runs the documented probe, sees the skill under the deeper path, and concludes .claude/skills works — the opposite of what the page exists to teach. Fix: tell the reader the signal is the NAMESPACE prefix, not mere presence — e.g. # expect .claude:; a bare is project discovery, not the plugin — or make the probe run from a directory that is not the skill's parent repo.

Nits

  1. [Axis 1] Mid-test monkeypatch.undo() is not exception-safe, so the ordering/masking hazard it fixes reappears on any failing path (tests/test_reference_permissions.py:391) — The fix is a bare monkeypatch.undo() at line 391, after both registry.ensure_crash_handlers() calls and before the assertions at 394-398. Its comment states the purpose: 'Lift the patch HERE, not at teardown ... the async-test teardown restores SIGINT by calling signal.signal(SIGINT, default_int_handler) — which would hit _refuse and raise ValueError out of teardown, failing the test for a reason it does not test.' That holds only when everything before line 391 succeeds. If registry.ensure_crash_handlers() (line 378 or 381) raises — the regression shape this test exists to catch — line 391 is skipped, teardown hits _refuse, and the real error is masked by a teardown ValueError, which is precisely the symptom being fixed. Scope the two patches instead: with monkeypatch.context() as mp: around lines 360-382 (mp.setattr(...) for atexit.register and signal.signal), leaving the assertions outside the block — the restore then runs on every exit path with no explicit undo().
  2. [Axis 1] CE045 class docstring's incident record is inaccurate — tutorial 07 carried the wrong path once, not twice (tests/test_custom_lint.py:3368) — Line 3367-3368 reads: 'Six surfaces once said .claude/skills in unison: check-skill, ci, the bundled activation.yaml, docs/PLUGIN.md, and tutorial 07 twice.' git grep -n "\.claude/skills" origin/main -- docs plugins shows tutorial 07 contained the wrong value on exactly one line (docs/tutorials/07-plugin-in-claude-code.md:137); the file's second hunk in this PR edits the troubleshooting row, which on main said only 'SKILL_SOURCE_PATH is unset, so the skill was never offered' and never named .claude/skills. Commit 3b64a26 gets this right ('tutorial 07 (guidance + its troubleshooting row, which blamed only an unset variable)'); the docstring compresses it into a false claim. Since this docstring is the rule's rationale-of-record, restate it as the verified count — five wrong-value lines across five files (docs/PLUGIN.md:112, tutorial 07:137, activation.yaml:14 and :17, check-skill:168 and :172, ci:156), of which one form the matcher cannot see.
  3. [Axis 3] The de-flake immunizes one test but leaves the process-wide signal.signal patch it diagnosed in three sibling tests, two of which read a signum-keyed dict (tests/test_reference_permissions.py:391) — The new comment at lines 371-377 correctly diagnoses the cause — "signal.signal is patched module-wide for the duration of this test, so anything else in the process that reaches fs_permissions while it runs lands in attempts too" — because monkeypatch.setattr("coder_eval.fs_permissions.signal.signal", _refuse) resolves coder_eval.fs_permissions.signal to the stdlib signal module object and mutates its .signal attribute globally. The fix (set comparison + monkeypatch.undo() at line 391) is local to this one test; the same global patch remains at line 338 (_fake_signal), line 415 and line 443 (lambda s, h: captured.__setitem__(s, h)). The last two are the exposed shape: they later invoke captured[signal.SIGTERM](...) (line 424) and captured[signal.SIGINT](...) (line 452), so a stray in-process call landing in the same window overwrites the entry and the test exercises the wrong handler. Also, monkeypatch.undo() at line 391 is unguarded — if registry.ensure_crash_handlers() raised, undo is skipped and the teardown ValueError this change exists to prevent masks the real failure. Fix: give fs_permissions a module-local indirection to patch (or use pytest.MonkeyPatch.context() / try: … finally: monkeypatch.undo()) and apply it to all four sites, so the hazard is closed for the class rather than for one method.
  4. [Axis 8] skill_triggered name collisions are mitigated by advisory prose only, though the criterion already receives the namespace and throws it away (plugins/coder-eval/skills/check-skill/SKILL.md:190) — The new paragraph (lines 190-196) — **Because matching is by bare name, it cannot survive a name collision.** ... A collision does not error; it measures the wrong skill. — is accurate and a real improvement over silence, but it is a model-discretion instruction, so nothing in the harness enforces it and a colliding suite still emits a confident recall/precision number. The information needed to disambiguate is already present and discarded: src/coder_eval/criteria/skill_triggered.py:68 does names.add(skill.split(":")[-1]) on the Skill tool's parameters['skill'], which carries the full root:probe-beta form (confirmed against the real CLI). Since this PR makes a namespaced plugin skill the standard shape for every generated suite, the collision is now reachable in the default configuration rather than theoretical. Follow-up (out of this PR's scope, but the right harness answer): let SkillTriggeredCriterion.skill_name accept a namespaced value and match exactly when one is given, keeping bare-name matching as the fallback — then a colliding suite can be pinned instead of merely warned about.

What's Missing

Parallel paths:

  • 🟠 docs/AB_EXPERIMENTS.md:182-183 — the canonical "Recipe: A/B a Skill" still writes path: "../skills" under defaults.agent.type: claude-code (line 167). That is the exact broken depth this PR removed everywhere else: the with-skill arm loads nothing, so the A/B silently compares two identical bare arms. CE045 cannot see it (it scans only SKILL_SOURCE_PATH= assignments plus tasks/**/*.yaml values, never YAML inside docs code fences). (trigger: docs/agents/HARNESS_PARITY.md) (restates: Axis 5: Plugin-path ripple stopped at HARNESS_PARITY)
  • 🟡 docs/agents/CLAUDE_CODE.md — the per-harness reference a claude-code task author actually reads — was not updated: line 101 still defines plugins as "Local plugin/skill directories; $VAR in path is expanded…" and the "Skills & plugins" section (lines 179-190) repeats it, with no plugin-root requirement and no link to the new HARNESS_PARITY section. The one page stating the contract is the parity page nobody reaches from the field table. (trigger: docs/agents/HARNESS_PARITY.md) (restates: Axis 5: Plugin-path ripple stopped at HARNESS_PARITY)
  • 🟡 experiments/default.yaml:62 — the baseline layer every experiment inherits still documents # Example: [{"type": "local", "path": "/path/to/skills"}], i.e. the skills-dir form, for a file whose default agent is claude-code. experiments/ is outside CE045's _GLOBS and outside its tasks/** YAML walk, so this example can be copied indefinitely. (trigger: plugins/coder-eval/reference/templates/activation.yaml) (restates: Axis 5: Plugin-path ripple stopped at HARNESS_PARITY)
  • 🟡 CLAUDE.md:147 is the repo's index of harness divergences ("Known unfixed divergences: permission_mode on Codex and Antigravity, disallowed_tools on Codex, allowed_tools/disallowed_tools on Antigravity, turn_timeout on Antigravity"). The PR adds a sixth, higher-cost divergence to HARNESS_PARITY.md but does not add it to that list, so the index the next contributor greps is now incomplete for exactly the field that shipped broken across six surfaces. (trigger: docs/agents/HARNESS_PARITY.md)
  • 🔵 The three code sites that implement the divergence carry no back-reference to the new contract section, unlike the max_turns precedent (codex_agent.py:820 and antigravity_agent.py:539 both cite docs/agents/HARNESS_PARITY.md). Add the same one-line pointer at claude_code_agent.py's process_plugins call site (line ~1184), codex_agent._setup_skills, and antigravity_agent._resolve_skills_paths, so an edit to either tolerant scanner knows a documented contract exists. (trigger: docs/agents/HARNESS_PARITY.md)
  • 🔵 CE045 is implemented inline in tests/test_custom_lint.py against a hardcoded REPO_ROOT, while every sibling doc-surface rule factors its logic into a root-parameterized module under tests/lint/ (CE044 → tests/lint/plugin_manifest_parity.py, plus doc_env_parity.py, doc_schema_parity.py, plugin_reference.py, workflow_outputs.py). The divergence is what makes CE045's walk untestable against a fixture tree (see the Tests bucket). (trigger: tests/test_custom_lint.py)
  • 🟡 The runtime seam was left asymmetric: utils.process_plugins (src/coder_eval/utils.py:69-86) warns only on an undefined env var, while codex (codex_agent.py:1137-1142) and antigravity (antigravity_agent.py:302,320-323) both log a 0-skills/unresolved-path warning — so the one harness where the wrong depth is fatal is the only one that is silent, and no user repo (where check-skill actually writes these suites) is reachable by a repo-scoped lint rule. (trigger: docs/agents/HARNESS_PARITY.md) (restates: Axis 3: CE045 only lints repo-shipped strings; no runtime guard on claude-code)

Tests:

  • 🟡 CE045's surface walk has no fixture test and no non-vacuity guard: _surfaces() globs the live tree and test_no_surface_points_skill_source_path_at_a_skills_dir passes trivially if the globs ever match nothing (docs move, plugin dir renamed). The mutation guard at line 3455 exercises _is_skills_dir alone, never the regex+walk pipeline. Add a tmp_path tree containing one offending file and assert the walk reports it, plus assert self._surfaces() covers the known six surfaces. (trigger: tests/test_custom_lint.py)
  • 🟡 Nothing executable proves the plugin-root contract itself. tests/test_plugin_processing.py (11 tests) covers env-var expansion only — no case for a resolvable path whose layout has no skills/ — and the pr-checks plugin-validate job (.github/workflows/pr-checks.yml:253-278) copies the template but asserts only row count and labels, never the agent.plugins[0].path semantics. A fixture root laid out <root>/skills/<name>/SKILL.md plus a negative <root>/<name>/SKILL.md case would turn the entire fix from prose+regex into a real gate. (trigger: plugins/coder-eval/reference/templates/activation.yaml)
  • 🟡 CE045's second test (test_literal_plugin_paths_in_tasks_are_plugin_roots) has zero eligible inputs in tasks/ today and skips every $-bearing value, so the YAML walk is unexercised and cannot regress-fail; it needs at least a tmp_path fixture proving it flags path: ".claude/skills" and path: "$REPO_ROOT/.claude/skills". (trigger: tests/test_custom_lint.py) (restates: Axis 8: CE045's tasks/ walk checks nothing (blanket $ exemption))
  • 🟠 The GitHub Actions surface — the one the class docstring calls the highest-cost (ci writes it into users' scheduled workflows) — has no passing-path test: test_the_matcher_sees_every_framing_the_surfaces_use (line 3473) asserts the truncated ${{ capture as correct, so the Actions form is documented as covered while being structurally unreachable by _is_skills_dir. (trigger: plugins/coder-eval/skills/ci/SKILL.md) (restates: Axis 1: CE045's _ASSIGNMENT regex truncates at whitespace)
  • 🔵 The de-flake in tests/test_reference_permissions.py:391 lifts the patch imperatively rather than scoping it, and three sibling tests (lines 338, 415, 443) keep the same process-wide signal.signal patch — so neither the exception path nor the class-wide hazard is covered by the change. (trigger: tests/test_reference_permissions.py) (restates: Axis 1: Mid-test monkeypatch.undo() is not exception-safe)

Downstream consumers:

  • 🟡 docs/TASK_DEFINITION_GUIDE.md § skill_triggered (lines 1275-1298) is where a suite author learns how the criterion matches, and it still says nothing about how the skill becomes reachable — no plugin-root note, no link to the new parity section. An author who writes a skill_triggered suite from that page alone reproduces the recall-0.0 bug the PR is fixing. (trigger: docs/agents/HARNESS_PARITY.md)
  • 🔵 Six skill description: frontmatters were rewritten, but the paraphrase tables that advertise the same skills were not: docs/PLUGIN.md:53 still promises lint-tasks reports "fixtures with no cleanup and near-duplicates — each with a severity and a fix" and plugins/coder-eval/README.md:41-45 carries its own variants, while the trimmed descriptions dropped those claims. The derived surface tests check only name presence and the count word — nothing enforces description/blurb parity, so this drifts unobserved. (trigger: plugins/coder-eval/skills/lint-tasks/SKILL.md)
  • 🔵 The descriptions are described as "measured" (PR #109) but the measurement leaves no in-repo artifact: no activation suite, run record, or comment records the before/after triggering numbers, and SKILL_LISTING_BUDGET_CHARS (tests/test_custom_lint.py:1250) was not re-baselined after the total shrank. A future edit has no evidence to compare against. (trigger: plugins/coder-eval/skills/analyze/SKILL.md)

Daily/nightly:

  • 🟡 Blast radius on already-emitted user artifacts is unstated. Every suite check-skill scaffolded and every scheduled workflow ci emitted before this fix still carries .claude/skills, and the PR adds no migration note anywhere (docs/PLUGIN.md, tutorial 07, the skills themselves): those users' weekly jobs stay permanently red, and any suite_thresholds they lowered against a recall-0.0 baseline is now mis-calibrated in the other direction once they fix the path. (trigger: plugins/coder-eval/skills/ci/SKILL.md)
  • 🟡 No recurring in-repo guard exists for the plugin's own activation behavior: there is no skill_triggered task or scheduled workflow covering the six shipped skills (only plugin-validate, which is offline and schema-only), so a change to six description: fields — the field activation keys on — ships with no signal at all until users report mis-triggering. (trigger: plugins/coder-eval/skills/check-skill/SKILL.md)

Harness & Lint Improvements

Static checks (lint / type):

  • [ce-lint] CE046 — a text-scanning lint rule must carry a machine-checked known-bad corpus that exercises the FULL extract→predicate pipeline. New doc-surface-style @pytest.mark.lint class in tests/test_custom_lint.py (not a BaseRule; the runner's check_paths only walks src/, see tests/test_custom_lint.py:22,31). It walks the AST of tests/test_custom_lint.py itself and, for every lint class that defines a class-level re.compile(...) value extractor, requires (a) a KNOWN_BAD_LINES: tuple[str, ...] attribute holding the verbatim pre-fix source lines — one per historical offending surface — and (b) a test that feeds each line through extractor.findall(line) → predicate and asserts every one is flagged. Today CE045's mutation guard (test_the_rule_would_catch_the_bug_it_was_written_for, line 3458) calls _is_skills_dir on hand-written strings the matcher can never produce, so the predicate is proven and the matcher is not. Prevents: Finding 1 (_ASSIGNMENT at tests/test_custom_lint.py:3392 truncates at whitespace): the entry SKILL_SOURCE_PATH=${{ github.workspace }}/.claude/skills would fail immediately — _ASSIGNMENT.findall yields ${{ and _is_skills_dir returns False. It also kills the companion anti-test at line 3473 that writes the truncation down as expected. Finding 3 (docstring's "tutorial 07 twice" incident record) becomes moot: the corpus IS the incident record, so the count is derived rather than asserted in prose.
  • [ce-lint] CE047 — a text-scanning lint rule's file globs must cover every tracked file containing the token it guards. For each such rule, take its guarded token (SKILL_SOURCE_PATH), run git grep -l <token> over the tracked tree, and assert every hit matches at least one entry in the rule's _GLOBS (excluding the rule's own file). This makes CE045's comment at lines 3394-3395 — "Globbed, not enumerated: a seventh surface must be caught by existing, not by remembering to add it here" — true by construction instead of aspirational: a new surface either falls inside the globs or fails the build. Prevents: Finding 4: _GLOBS = ("plugins/coder-eval/**/*.md", "plugins/coder-eval/**/*.yaml", "docs/**/*.md") (line 3396) misses root-level *.md (README.md, CONTRIBUTING.md), .github/workflows/**, tasks/** and experiments/**. .github/workflows/pr-checks.yml already dogfoods the activation template (lines 261-262), so an in-repo workflow surface is a realistic next drift.
  • [ce-lint] CE048 — a file-walking lint scan must assert non-vacuity. AST rule over tests/test_custom_lint.py: a test method that walks files (for ... in ...glob(...)) and ends in assert not offenders must also assert a non-zero examined counter (assert scanned, ...). Mirrors CE036's existing registry-derived coverage requirement — a rule that examines nothing passes for the wrong reason and reads as green forever. Prevents: Finding 8: test_literal_plugin_paths_in_tasks_are_plugin_roots currently examines ZERO entries (the only local-plugin task, tasks/agents/codex_skills_test.yaml, is Codex and filtered out), so its if "$" in value: continue exemption (lines 3446-3447) removed the one shape the walk could ever see and nothing noticed. A non-vacuity assert forces either a fixture or a widened scan at authoring time.
  • [ce-lint] CE049 — no process-global stdlib patching or mid-test monkeypatch.undo() in tests. New BaseRule AST rule, wired via a dedicated check_paths([TESTS], rules=[NoProcessGlobalPatch]) test (the parametrized test_no_violations only walks SRC). Two shapes: (a) monkeypatch.setattr("<pkg>.<mod>.<stdlib>.<attr>", ...) where the second-to-last dotted segment is a stdlib module (os, signal, time, socket, subprocess, shutil) — pytest resolves coder_eval.fs_permissions.os to the shared stdlib module object and mutates it process-wide, not module-locally; require a module-local indirection or monkeypatch.context(). (b) any monkeypatch.undo() outside a finally: block — use with monkeypatch.context() as mp: so the restore runs on every exit path. Prevents: Findings 2 and 6. Would have flagged 11 sites in tests/test_reference_permissions.py (:176, :338, :367, :414-416, :442-444, :470, :1044) — including the two captured[signum] handler dicts at :415/:443 that the de-flake diagnosed but did not fix — plus both bare undo() calls at :183 and :391, where a raise from registry.ensure_crash_handlers() (:378/:381) skips the undo and masks the real failure behind a teardown ValueError.
  • [ce-lint] CE050 — every agent must resolve agent.plugins[].path through the single shared utils.process_plugins seam. AST rule over src/coder_eval/agents/**: reading plugin["path"] / plugin.get("path") and then calling expand_env_vars(...) or Path(...).resolve() on it, outside utils.process_plugins, is a violation (scoped to path resolution, not to each backend's skill-linking, which legitimately differs). One seam means one place to add the path diagnostic, so a warning added there covers all three backends at once. Prevents: Finding 5 (and the runtime half of finding 7): the divergence exists precisely because each backend resolves its own path — codex_agent.py:1137-1142 and antigravity_agent.py:302,320-323 both warn on a 0-skills root, while claude-code (claude_code_agent.py:1184utils.py:69-86) warns only on an undefined env var. claude-code is the one harness where the wrong path is fatal and the only one with no signal.
  • [ce-lint] CE045 widening — key the exemption on the CONSUMING harness, not the variable name, and stop short-circuiting on $. (i) Derive the guarded variable set instead of hardcoding SKILL_SOURCE_PATH: scan tasks/** and experiments/** for agent.type == claude-code (including inherited defaults.agent) and collect the $VAR names appearing in plugins[].path; require every assignment of those names, anywhere in the repo, to be a plugin root. (ii) In the tasks walk, replace if "$" in value: continue with a bare-reference-only skip (re.fullmatch(r"\$\{?\w+\}?/?", value)) and apply _is_skills_dir to the literal tail otherwise. (iii) Strip ${{ ... }} before capture so the GitHub Actions form is seen. Prevents: Finding 7: experiments/plugin-comparison.yaml sets type: claude-code (line 22) and path: "$PLUGIN_PATH" (line 37), while docs/agents/CODEX.md:266 documents export PLUGIN_PATH=~/path/to/skills — the exact banned shape, exempted only because the rule keys on a name. Also finding 8 case (a): path: "$REPO_ROOT/.claude/skills" in a task passes CE045 today (reproduced: 5 passed), even though the /skills tail is plainly visible and the sibling assignment walk would flag it.
  • [ce-lint] CE051 — a docs probe command that asserts an observable outcome must be pinned to a runner that executes it. In docs/agents/** (and docs/tutorials/**), a fenced shell block whose lines carry an expectation comment (# loads ..., # expect ..., # -> ...) must be preceded by a <!-- verified-by: <pytest nodeid | CI job> --> marker that resolves to a real test id or workflow job — same resolution style as CE035's needs.<job>.outputs walk and the same pinning pattern CE026 already uses for Action snippets (action-dogfood). The check verifies the pin exists; the claim's truth comes from the pinned runner (see harness item H2). Prevents: Finding 9: docs/agents/HARNESS_PARITY.md:133 states claude --plugin-dir "$(pwd)/.claude/skills" # loads nothing, which is FALSE when run from a repo root — project-scoped discovery lists the skill anyway (reproduced against claude 2.1.248). An unpinned expectation comment is an untested assertion in the one document written to stop people from mis-diagnosing this.

Harness improvements (not statically reachable):

  • Add the plugin-path diagnostic at the shared runtime seam (utils.process_plugins), with a caplog + tmp_path unit test. Warn when the expanded path (a) does not exist, or (b) exists, has no skills/ subdirectory, AND directly contains a <child>/SKILL.md — the precise skills-dir shape the bug produces. The narrow (b) condition matters: a legitimate plugin root may hold only commands/, agents/, or an MCP server (tests/test_plugin_processing.py already exercises a .../mcp root), so an unconditional "no skills/" warning would fire on valid configs. Test both arms: <root>/skills/demo/SKILL.md → silent, <root>/demo/SKILL.md → warns. Why not static: The offending value is supplied by the USER at run time via SKILL_SOURCE_PATH; no repo-scoped scan can ever see it. CE045 runs only in this repo, while /coder-eval:check-skill writes activation suites into downstream user repos where no lint of ours executes. Detecting the wrong layout requires stat-ing the resolved directory. Prevents: Findings 5 and 7 — and the entire failure class CE045 was written for, at the one place that reaches every user. It also closes the parity gap the PR's own HARNESS_PARITY row documents as "fails without an error".
  • Turn the plugin-path probe into an executable artifact the docs cite, not a hand-typed command. Add a @pytest.mark.live test (or make probe-plugin-dir) that builds a fixture skill in a tmp dir OUTSIDE any .claude cwd, runs claude --plugin-dir <root> and --plugin-dir <root>/skills, and asserts on the NAMESPACE prefix (<root-name>:<skill> present vs. absent) rather than on mere presence. Rewrite HARNESS_PARITY.md:129-133 to state the namespace signal and point at the pinned runner. Why not static: Needs the real claude CLI and a live agent invocation. The false negative is cwd-dependent — project-scoped skill discovery masks the failure only when the probe runs from the skill's own repo root, which is exactly where a reader runs it — so it is invisible to any text scan. Prevents: Finding 9. It also stops the CE045 docstring's "Probed against the real CLI" claim from rotting silently as the CLI evolves.
  • Have check-skill stage a MINIMAL scratch plugin root, and record the loaded plugin inventory in task.json. (i) The skill should create <tmp>/skills/<skill-name> (symlink or copy) and point SKILL_SOURCE_PATH there, so the plugin root contains exactly the unit under test. (ii) The claude-code agent should record, per --plugin-dir root, which components it contributed (skills/, agents/, commands/, hooks/, .mcp.json), so a confound is visible after the fact and two suites' recall numbers are comparable. At minimum, state the wider load in the template comment and in check-skill/SKILL.md. Why not static: What a plugin root contains is a property of the user's filesystem at run time — nothing in this repo can be scanned to learn that a given .claude/ also holds agents/. Verified against claude 2.1.248: a root with skills/, agents/ and commands/ registers all three (probe-root:probe-beta, probe-root:probe-subagent, /probe-root:probe-cmd); auto-delegable subagents are the load-bearing confound, slash commands are inert in a non-interactive eval. Note CE045 does not block the fix — a scratch root whose last segment is not skills passes. Prevents: Finding 11 — recall becoming repo-dependent and non-comparable across generated activation suites, for reasons unrelated to the skill's description.
  • Let SkillTriggeredCriterion.skill_name accept a namespaced value and match exactly, with bare-name matching as the fallback; add a collision note. criteria/skill_triggered.py:68 does names.add(skill.split(":")[-1]) on a Skill parameter that already carries the full <plugin-root>:<name> form — the disambiguating information is received and discarded. Also emit a criterion-level note when the trajectory shows one bare name under more than one namespace. Why not static: The collision is only observable in a recorded trajectory's Skill tool calls — it is a property of the user's installed skill set at run time, not of any repo text. The current mitigation is a prose paragraph in check-skill/SKILL.md (~line 190), i.e. model discretion with nothing enforcing it; a colliding suite still emits a confident recall/precision number. Prevents: Finding 10. This PR makes a namespaced plugin skill the DEFAULT shape for every generated suite, so the collision moved from theoretical to reachable in the standard configuration.
  • Add a session-level shared-state leak detector for the test suite. An autouse fixture that snapshots signal.getsignal(SIGINT/SIGTERM) and the identity of hot stdlib attributes (os.chmod, signal.signal, os.kill) before each test and fails the test that leaves any of them changed — attributing the leak to its source instead of to the next victim. Why not static: CE049 catches the patch SHAPE at the call site; only a runtime guard catches a leak that arrives through a helper or fixture, or an escape via a background thread — and fs_permissions owns exactly such a watchdog thread, which is what made the original failure -n auto-ordering-dependent rather than reproducible. Prevents: The residual half of findings 2 and 6 — the three sibling tests still holding a process-wide signal.signal patch, two of which read a signum-keyed dict (captured[SIGTERM] at :424, captured[SIGINT] at :452) that a stray in-process call can overwrite.
  • Add a would-have-caught replay target for new lint rules: make lint-replay RULE=<id> REV=<pre-fix-sha>. Check the pre-fix tree into a throwaway git worktree, run only the named rule against it, and require at least one violation — then paste the reported offender lines into the rule's KNOWN_BAD_LINES corpus (static check CE046). This is exactly the manual step that exposed the ${{ ... }} miss during this review; automating it makes the corpus a byproduct of authoring the rule rather than a discipline. Why not static: After the fix lands, the repo tree is clean by construction, so a new rule passes trivially against the only tree a static check can see. Proving a rule catches its motivating bug requires materializing a second, historical worktree — git state, not source text. Prevents: Findings 1, 4 and 8 share one root shape: the rule passes because it sees nothing (a truncated capture, an uncovered glob, an exemption that skips the only candidate). A replay makes the sighting mandatory before the rule is accepted.

Top 5 Priority Actions

  1. Stage a minimal plugin root in /coder-eval:check-skill — a scratch dir holding only skills/<skill> — instead of prescribing the user's whole .claude (plugins/coder-eval/reference/templates/activation.yaml:13,19,26), because --plugin-dir also loads sibling agents/, whose auto-delegable subagents can make the agent delegate instead of calling the skill, flipping skill_triggered to observed='no' and making recall repo-dependent; if the wider load is intentional, say so in the template and in check-skill/SKILL.md.
  2. Add a runtime guard in src/coder_eval/utils.py::process_plugins (lines 69-86; only warning is the undefined-env-var one at :77, reached from agents/claude_code_agent.py:1184) for a plugin path that does not exist, or that has no skills/ subdir while directly containing <child>/SKILL.md — today claude-code, the one harness where this is fatal, is silent and reports recall 0.0, while codex_agent.py:1137 and antigravity_agent.py:302 both warn.
  3. Let SkillTriggeredCriterion.skill_name accept and exactly match a namespaced value, keeping bare-name as fallback, since src/coder_eval/criteria/skill_triggered.py:68 already receives root:<skill> and discards the namespace via .split(":")[-1] — a name collision currently emits a confident but wrong recall number backed only by advisory prose in plugins/coder-eval/skills/check-skill/SKILL.md:190.
  4. Close CE045's three coverage holes in tests/test_custom_lint.py — strip ${{ ... }} before capture so the whitespace-truncating _ASSIGNMENT regex (:3392) stops missing the ci skill's GitHub Actions form (plugins/coder-eval/skills/ci/SKILL.md:156, the surface its own docstring calls highest-cost), widen _GLOBS (:3396) to root *.md, .github/workflows/*.y*ml and tasks/** so "globbed, not enumerated" is true, and replace the blanket if "$" in value: continue (:3446) with a bare-variable-only skip so an interpolated .../.claude/skills tail is still caught (reproduced: the tasks walk currently checks nothing).
  5. Fix the two misleading doc surfaces added in docs/agents/HARNESS_PARITY.md — the probe at :132-133 gives a false negative from a repo root because project-scoped discovery lists the skill anyway (key the reader on the .claude:<skill> namespace prefix, not mere presence), and the exemption at :139 claims a harness scope while CE045 is name-scoped, leaving the claude-code $PLUGIN_PATH consumer at experiments/plugin-comparison.yaml:37 and docs/agents/CODEX.md:266 unguarded.

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

uipreliga and others added 2 commits August 27, 2026 17:04
…allel surfaces, runtime guard

Every claim below was reproduced before acting on it. Three of them proved my own
text wrong.

## CE045's matcher could not see the surface it was written for

`[^"'\s]+` stops at the first space, so the GitHub Actions form
`SKILL_SOURCE_PATH=${{ github.workspace }}/.claude/skills` captured `${{` — never
the path. The `ci` skill, which the rule's own docstring calls the highest-cost
surface because it writes into users' SCHEDULED workflows, was structurally
unreachable. Worse, a companion test asserted that truncated capture as CORRECT,
so the hole was written down as covered.

The value now runs to end-of-line or a closing quote, and `${{ ... }}` expressions
are stripped before the last-segment check. Verified by reintroducing the pre-fix
`ci` line: the rule flags it with file and line.

The mutation guard is replaced by `KNOWN_BAD_LINES`, the four verbatim pre-fix
source lines, replayed through the FULL extract-then-predicate pipeline. The old
guard called the predicate on hand-written strings the matcher could never
produce, which is exactly how the matcher stayed unproven. That corpus is now the
incident record, so the docstring no longer miscounts it ("tutorial 07 twice" was
wrong — one line, verified against origin/main).

Two bugs my own new tests then caught: `findall` reports an unmatched alternation
group as `""` rather than `None`, so picking "the first non-None group" always
selected the empty quoted branch; and the YAML walk labelled fixture paths with
`relative_to(REPO_ROOT)`.

## The globs and the tasks walk both under-covered

`_GLOBS` missed root-level `*.md`, `.github/workflows/**`, `tasks/**` and
`experiments/**`. `test_globs_cover_every_file_naming_the_token` now derives the
expected set from `git grep -l`, so "globbed, not enumerated" is true by
construction rather than aspirational. A non-vacuity assert fails if the walk ever
matches nothing at all.

The YAML walk skipped every `$`-bearing value as "covered by the assignment
check", which those globs did not in fact cover — so it had zero eligible inputs
and could not regress-fail. It now skips only a BARE variable reference and judges
the literal tail otherwise, so `$REPO_ROOT/.claude/skills` is caught. It also
reads `defaults.agent` and `variants[].agent`, and has a fixture test.

## Three parallel surfaces carried the same bug

- `docs/AB_EXPERIMENTS.md` — the canonical "A/B a skill" recipe wrote
  `path: "../skills"` under `type: claude-code`. Its `with-skill` arm loaded
  nothing, so the experiment silently compared two identical arms.
- `experiments/default.yaml` — the baseline every experiment inherits documented
  `"path": "/path/to/skills"`.
- `experiments/plugin-comparison.yaml` — `$PLUGIN_PATH` under a claude-code
  default, with no statement of the contract.

That last one also falsifies a claim I shipped: HARNESS_PARITY and the rule's
docstring both said `PLUGIN_PATH` was "deliberately outside the rule, since a
skills directory is valid there". It is not Codex-only. Both now say the exemption
is a limit of the rule's scope, not a licence to use the deeper form.

`docs/agents/CLAUDE_CODE.md` — the reference a claude-code task author actually
reads — states the contract and links the parity section; `CLAUDE.md`'s index of
known divergences gains this one as its highest-cost entry.

## The parity probe taught the opposite of its lesson

Run from a repo root, `claude --plugin-dir "$(pwd)/.claude/skills"` STILL lists
the skill — Claude Code discovers `./.claude/skills/` natively, independent of
`--plugin-dir`. An engineer following the documented probe would have concluded
the deeper path works. The probe now runs from outside the skill's repo and tells
the reader the signal is the NAMESPACE prefix, not mere presence.

## A plugin root loads the whole plugin, not just its skills

Verified against the real CLI: a root holding `skills/probe-beta/`,
`agents/probe-subagent.md` and `commands/probe-cmd.md` offers all three as
`root:probe-beta`, `root:probe-subagent`, `root:probe-cmd`.

So pointing a suite at a repo's `.claude` hands the evaluated agent every project
subagent and command. With `.claude/agents/pdf-expert.md` present while measuring
`pdf-forms`, the agent may delegate instead of calling the skill; `skill_triggered`
records `no` and recall drops for a reason unrelated to the skill's description —
and the number becomes repo-dependent, so two suites stop being comparable.

`check-skill` and the bundled template now prescribe staging a MINIMAL root
containing only the skill under test, and `ci` carries the workflow step to build
one (a scheduled job inherits no shell state). The whole-tree form is still
offered, with the confound stated.

## A runtime guard, because a repo-scoped lint rule cannot reach users

`utils.process_plugins` warned only on an UNDEFINED env var. A path that resolves
and exists but holds no `skills/` — precisely `.claude/skills` — reached the SDK
with no signal at all. Codex and Antigravity already log the equivalent condition;
claude-code, the one harness where the depth is fatal, was silent. It now warns,
with four tests. That covers the user repos where `/coder-eval:check-skill`
actually writes these suites, which CE045 structurally cannot.

## Exception-safe patch scoping

`test_install_failure_is_not_latched` lifted its `signal.signal` patch with a bare
`monkeypatch.undo()` after the calls. If `ensure_crash_handlers` raised — the
regression the test exists to catch — the undo was skipped and the teardown
`ValueError` masked the real failure. Now `monkeypatch.context()`, which restores
on every exit path.

## Not addressed here

The reviewer's namespaced-`skill_name` matching, the sibling tests' process-wide
`signal.signal` patches, and the proposed CE046/CE047 meta-rules are real but are
their own changes. The migration note for already-emitted user artifacts needs a
release decision first.

Verified: `make lint` exit 0 (382 passed); `make test` shows the same 10
pre-existing litellm failures as clean main and nothing else; ruff clean;
`claude plugin validate --strict`; `coder-eval plan` on the edited template.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V5WZ1BbtygK9YVvcgPfNGh
pyright (`reportImplicitStringConcatenation`) rejected the multi-line warning
string added in 754805b. It failed two gates — Quality Gate and Windows Smoke,
both on the `Type check with pyright` step — and I had not caught it because
pyright could not install in this session, so the one gate my change could break
was the one gate I never ran.

Now matches the explicit `+` style the neighbouring warning in
`codex_agent._setup_skills` already uses.

Verified with CI's exact extras (`--extra dev --extra uipath --extra codex
--extra litellm`): pyright reports 0 errors, 1 pre-existing warning in
`antigravity_agent.py:442` that is not from this branch. `make lint` exit 0,
`make test` 4369 passed, ruff clean.

The third red gate, Evalboard, is unrelated: it failed on `Install dependencies
(lockfile-pinned)`, this branch touches no JS or lockfile, and the job passes on
main's last three runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01V5WZ1BbtygK9YVvcgPfNGh
@uipreliga
uipreliga merged commit c565ebb into main Aug 28, 2026
15 checks passed
@uipreliga
uipreliga deleted the feat/reuse-pr109-cheap-wins branch August 28, 2026 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants