From 3b64a26141ee2a3b01481d48c0b7880498f4fefc Mon Sep 17 00:00:00 2001 From: uipreliga Date: Thu, 27 Aug 2026 13:41:55 -0700 Subject: [PATCH 1/4] fix(plugin): correct the skill-reachability path, and reuse PR #109's measured descriptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `/skills//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 `/.claude/skills/probe-alpha/SKILL.md`: claude --plugin-dir /.claude/skills -> NONE (never loads) claude --plugin-dir /.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) Claude-Session: https://claude.ai/code/session_01V5WZ1BbtygK9YVvcgPfNGh --- docs/PLUGIN.md | 6 +++-- docs/tutorials/07-plugin-in-claude-code.md | 9 +++++--- .../reference/templates/activation.yaml | 10 ++++---- plugins/coder-eval/skills/analyze/SKILL.md | 2 +- .../coder-eval/skills/check-skill/SKILL.md | 23 +++++++++++++++---- plugins/coder-eval/skills/ci/SKILL.md | 10 ++++++-- plugins/coder-eval/skills/init/SKILL.md | 2 +- plugins/coder-eval/skills/lint-tasks/SKILL.md | 2 +- plugins/coder-eval/skills/task/SKILL.md | 2 +- 9 files changed, 46 insertions(+), 20 deletions(-) diff --git a/docs/PLUGIN.md b/docs/PLUGIN.md index 9b118482..3c353612 100644 --- a/docs/PLUGIN.md +++ b/docs/PLUGIN.md @@ -106,10 +106,12 @@ It then: One prerequisite the suite cannot infer: the evaluated agent runs in a fresh sandbox holding none of your files, so it is offered no skills unless the task says where they live. The template reads that location from an environment -variable — point it at the directory *containing* the skill's own directory: +variable — point it at a **plugin root**: a directory holding a `skills/` +subdirectory, so the skill sits at `/skills//SKILL.md`. For +`.claude/skills/pdf-forms/SKILL.md` that root is `.claude`, not `.claude/skills`: ```bash -export SKILL_SOURCE_PATH="$(pwd)/.claude/skills" +export SKILL_SOURCE_PATH="$(pwd)/.claude" ``` Leave it unset and the skill is simply absent, every positive row scores 0, and diff --git a/docs/tutorials/07-plugin-in-claude-code.md b/docs/tutorials/07-plugin-in-claude-code.md index a5d7c527..1d265a95 100644 --- a/docs/tutorials/07-plugin-in-claude-code.md +++ b/docs/tutorials/07-plugin-in-claude-code.md @@ -131,10 +131,13 @@ reaches for one at the right moment. **Export the skill location first** — the evaluated agent runs in a fresh sandbox holding none of your files, so it is offered no skills unless the task says where -they live. Point at the directory *containing* the skill's own directory: +they live. Point at a **plugin root**: a directory holding a `skills/` subdirectory, +so the skill sits at `/skills//SKILL.md`. For +`.claude/skills/pdf-forms/SKILL.md` that root is `.claude`, not `.claude/skills` — +one level too deep loads nothing at all: ```bash -export SKILL_SOURCE_PATH="$(pwd)/.claude/skills" +export SKILL_SOURCE_PATH="$(pwd)/.claude" ``` ``` @@ -158,7 +161,7 @@ covers telling them apart with `/doctor` and `/context`. | No `/coder-eval:` commands after installing | Check `/plugin`; re-run the install | | A skill offers to install the CLI, or Bash reports `command not found` | The CLI isn't installed or isn't on `PATH` — accept the offer, or install it yourself | | `coder-eval run` matches nothing | Wrong directory — use the path `init` reported in step 2 | -| Every positive row in step 5 scores 0 | `SKILL_SOURCE_PATH` is unset, so the skill was never offered | +| Every positive row in step 5 scores 0 | `SKILL_SOURCE_PATH` is unset, or points one level too deep (`.claude/skills` rather than the `.claude` plugin root), so the skill was never offered | To update after the marketplace moves, `/plugin marketplace update coder-eval`; to remove it, `/plugin uninstall`. diff --git a/plugins/coder-eval/reference/templates/activation.yaml b/plugins/coder-eval/reference/templates/activation.yaml index a54c083d..3432096e 100644 --- a/plugins/coder-eval/reference/templates/activation.yaml +++ b/plugins/coder-eval/reference/templates/activation.yaml @@ -10,11 +10,13 @@ tags: [activation] # REPLACE: the skill under test must be REACHABLE by the sandboxed agent, or every # positive row scores 0 and the suite reports recall 0.0 — which reads exactly like a -# broken skill. `path` is the directory CONTAINING the skill's directory (for -# `.claude/skills/my-skill/SKILL.md` that is `.claude/skills`), supplied through an -# environment variable so the committed suite stays portable across machines and CI: +# broken skill. `path` must be a PLUGIN ROOT: a directory holding a `skills/` +# subdirectory, so the skill sits at `/skills//SKILL.md`. For +# `.claude/skills/my-skill/SKILL.md` that root is `.claude`, NOT `.claude/skills` — +# pointing one level too deep loads nothing at all. Supply it through an environment +# variable so the committed suite stays portable across machines and CI: # -# export SKILL_SOURCE_PATH=/abs/path/to/.claude/skills +# export SKILL_SOURCE_PATH=/abs/path/to/.claude # # An unset variable is logged as a warning and leaves the skill unreachable, so check # the first run's recall before trusting a low score. diff --git a/plugins/coder-eval/skills/analyze/SKILL.md b/plugins/coder-eval/skills/analyze/SKILL.md index c9d84242..39e548ac 100644 --- a/plugins/coder-eval/skills/analyze/SKILL.md +++ b/plugins/coder-eval/skills/analyze/SKILL.md @@ -1,5 +1,5 @@ --- -description: Analyze a finished coder-eval run and write analysis.md — cluster failures into systemic patterns, diagnose prompts, criteria, config, environment and cost, and recommend concrete fixes. Use when the user wants to know why a run failed, what to fix, or what a run says about their tasks. +description: Analyze a finished coder-eval run and write analysis.md — cluster failures into systemic patterns and recommend fixes. Use when the user wants to know why a run failed, what regressed or got worse since a previous run, what to fix, or what a run says about their tasks. allowed-tools: ["Read", "Glob", "Grep", "Write", "Bash"] --- diff --git a/plugins/coder-eval/skills/check-skill/SKILL.md b/plugins/coder-eval/skills/check-skill/SKILL.md index e451a7af..380be20f 100644 --- a/plugins/coder-eval/skills/check-skill/SKILL.md +++ b/plugins/coder-eval/skills/check-skill/SKILL.md @@ -1,5 +1,5 @@ --- -description: Generate and run a coder-eval activation suite for a Claude Code skill — does the agent actually engage it when it should, and leave it alone when it shouldn't? Use when the user asks whether a skill triggers, wants to test skill activation, or worries a skill has silently stopped firing. +description: Generate and run a coder-eval activation suite for a Claude Code skill. Use when the user asks whether a skill triggers, wants to test skill activation, or worries a skill has silently stopped firing. allowed-tools: ["Read", "Glob", "Grep", "Write", "Bash"] --- @@ -164,12 +164,17 @@ block in the template — and it is the template's job only **when nothing alrea skill**: if step 3 found an experiment supplying that block, inherit it and delete the template's copy rather than writing a second declaration. -Otherwise, fill it in: `path` is the directory **containing** the skill's own directory — -for `.claude/skills/pdf-forms/SKILL.md` that is `.claude/skills`. Tell the user to export it -before running, and to use the same variable in CI: +Otherwise, fill it in. **`path` must be a plugin root: a directory holding a `skills/` +subdirectory**, so that the skill sits at `/skills//SKILL.md`. A +`.claude-plugin/plugin.json` is optional — without one the namespace defaults to the +directory's own name. + +For `.claude/skills/pdf-forms/SKILL.md` that root is **`.claude`**, not `.claude/skills`. +Pointing at a bare directory of skill directories loads nothing at all. Tell the user to +export it before running, and to use the same variable in CI: ```bash -export SKILL_SOURCE_PATH="$(pwd)/.claude/skills" +export SKILL_SOURCE_PATH="$(pwd)/.claude" ``` Keep it an environment variable rather than baking an absolute path into the YAML — the @@ -182,6 +187,14 @@ invoked as `plugin:skill` — the checker strips the namespace before comparing. namespaced value here silently scores zero recall on every row, which reads exactly like a broken skill. +**Because matching is by bare name, it cannot survive a name collision.** Two skills called +`init` — one from a plugin, one built in — are the same string to the criterion, so it will +credit whichever fires as though it were the one under test. Check the name is unique across +everything installed before trusting a result (`/context` and `/doctor` list the active set). +A collision does not error; it measures the wrong skill. If one exists, say so rather than +reporting the number — renaming the skill, or measuring where the collision is absent, are +the only honest fixes. + For criterion fields beyond this template, read `${CLAUDE_PLUGIN_ROOT}/reference/criteria.md`. diff --git a/plugins/coder-eval/skills/ci/SKILL.md b/plugins/coder-eval/skills/ci/SKILL.md index 9b367b7e..c5bd203a 100644 --- a/plugins/coder-eval/skills/ci/SKILL.md +++ b/plugins/coder-eval/skills/ci/SKILL.md @@ -153,11 +153,17 @@ find**, resolved against the checkout: ```yaml env: | ANTHROPIC_API_KEY=${{ secrets.ANTHROPIC_API_KEY }} - SKILL_SOURCE_PATH=${{ github.workspace }}/.claude/skills + SKILL_SOURCE_PATH=${{ github.workspace }}/.claude ``` Use the directory the repository actually keeps skills in, from step 1, not the path -above. This is the one omission the scheduled trigger cannot survive: unset, the skill is +above — and note **what level that variable points at**. A local plugin path must be a +**plugin root**: a directory holding a `skills/` subdirectory, so the skill sits at +`/skills//SKILL.md`. For `.claude/skills/my-skill/SKILL.md` that is `.claude`, +**not** `.claude/skills`. Pointing one level too deep loads nothing at all and produces the +same permanent red as leaving it unset. + +This is the one omission the scheduled trigger cannot survive: unset, the skill is never offered to the sandboxed agent, every positive row scores 0, and the job fails its `recall` threshold every week — a permanent red that looks exactly like the drift the schedule exists to detect, so the real thing goes unnoticed when it arrives. diff --git a/plugins/coder-eval/skills/init/SKILL.md b/plugins/coder-eval/skills/init/SKILL.md index 1ea3819f..ce1b5d26 100644 --- a/plugins/coder-eval/skills/init/SKILL.md +++ b/plugins/coder-eval/skills/init/SKILL.md @@ -1,5 +1,5 @@ --- -description: Set up coder-eval in this repository — scan for what is worth evaluating (Claude Code skills, an MCP server, a CLI), then scaffold a task directory with one real, passing-or-failing task and the exact command to run it. +description: Set up coder-eval in this repository — scan for what is worth evaluating (Claude Code skills, an MCP server, a CLI), then scaffold a task directory with one real task and the command to run it. disable-model-invocation: true allowed-tools: ["Read", "Glob", "Grep", "Write", "Bash"] --- diff --git a/plugins/coder-eval/skills/lint-tasks/SKILL.md b/plugins/coder-eval/skills/lint-tasks/SKILL.md index 03b8efc1..93988af8 100644 --- a/plugins/coder-eval/skills/lint-tasks/SKILL.md +++ b/plugins/coder-eval/skills/lint-tasks/SKILL.md @@ -1,5 +1,5 @@ --- -description: Review coder-eval task YAML that already exists — find criteria that cannot fail, prompts that give away the answer, fixtures with no cleanup, and near-duplicate tasks, each with a severity and a concrete fix. Read-only. Use when the user wants existing tasks reviewed, linted, audited, or checked for gaps. +description: Review existing coder-eval task YAML — find criteria that cannot fail, prompts that leak the answer, and near-duplicate tasks, each with a fix. Read-only. Use when the user wants existing tasks reviewed, linted, audited, or checked for gaps. allowed-tools: ["Read", "Glob", "Grep"] disallowed-tools: ["Write", "Edit", "NotebookEdit"] --- diff --git a/plugins/coder-eval/skills/task/SKILL.md b/plugins/coder-eval/skills/task/SKILL.md index 037cfe31..7226cdea 100644 --- a/plugins/coder-eval/skills/task/SKILL.md +++ b/plugins/coder-eval/skills/task/SKILL.md @@ -1,5 +1,5 @@ --- -description: Turn a natural-language description into one or more coder-eval task YAML files — minimal prompts, weighted success criteria that check output content, validated with `coder-eval plan`. Use when the user wants to write, add, or generate an evaluation task. +description: Turn a natural-language description into coder-eval task YAML — minimal prompts, weighted criteria that check output content, validated with `coder-eval plan`. Use when the user wants to write, add, or generate an evaluation task. allowed-tools: ["Read", "Glob", "Grep", "Write", "Bash"] --- From fa6892061f875e5f1160633c51a28d30006bcc82 Mon Sep 17 00:00:00 2001 From: uipreliga Date: Thu, 27 Aug 2026 14:30:37 -0700 Subject: [PATCH 2/4] fix(test): add CE045, document the plugin-path divergence, unpin a test from ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `/skills//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.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) Claude-Session: https://claude.ai/code/session_01V5WZ1BbtygK9YVvcgPfNGh --- docs/agents/HARNESS_PARITY.md | 39 ++++++++- tests/test_custom_lint.py | 126 ++++++++++++++++++++++++++++ tests/test_reference_permissions.py | 28 ++++++- 3 files changed, 191 insertions(+), 2 deletions(-) diff --git a/docs/agents/HARNESS_PARITY.md b/docs/agents/HARNESS_PARITY.md index 296092f6..d8302bdb 100644 --- a/docs/agents/HARNESS_PARITY.md +++ b/docs/agents/HARNESS_PARITY.md @@ -5,7 +5,8 @@ was the field that broke that promise hardest: Claude Code enforced it, and Code Antigravity accepted it and never read it, so `max_turns: 6` ran capped on one backend and unbounded on the other two. -This page is the contract for what each run limit means per harness. +This page is the contract for what each run limit means per harness, plus the shared +`agent` fields whose meaning still differs across them. ## The table @@ -102,6 +103,42 @@ A timeout is a *failure* (partial turn captured, error status); the turn cap is *clean stop*. Conflating them is the mistake this page exists to prevent: a task whose cap fires should not look like a task whose harness hung. +## `agent.plugins[].path` accepts different depths per harness + +Not a run limit, but the same promise: one task file, three harnesses, same meaning. +This field breaks it silently. + +| | claude-code | codex | antigravity | +|---|---|---|---| +| `/skills//SKILL.md` (plugin root) | **required** | accepted | accepted | +| `//SKILL.md` (bare skills dir) | **loads nothing** | accepted | accepted | + +claude-code hands the value to the SDK as a *plugin directory*, and a plugin's skills +live at `/skills//SKILL.md`. Point it at the directory that directly +parents the skill directories and no skill loads. Codex +(`codex_agent._setup_skills`) and Antigravity (`antigravity_agent._resolve_skills_paths`) +both scan **both** layouts and take whichever actually holds a `/SKILL.md`. + +So `.claude/skills` works on two backends out of three and fails on the third — and +fails without an error. The agent simply is not offered the skill, every positive row +of an activation suite scores 0, and the suite reports recall 0.0. That is +indistinguishable from a skill that never triggers, which is the finding such a suite +exists to produce. It shipped in six documentation surfaces at once for exactly this +reason. + +Probe it in one command, against whichever harness you doubt: + +```bash +claude --plugin-dir "$(pwd)/.claude" # loads .claude: +claude --plugin-dir "$(pwd)/.claude/skills" # loads nothing +``` + +**Write the plugin root.** It is correct on all three, so there is never a reason to +write the deeper form. For `.claude/skills/my-skill/SKILL.md` that is `.claude`. +`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. + ## Reproducing `tasks/run_limits/` holds one fixture per limit: `max_turns_cap.yaml` asks for more diff --git a/tests/test_custom_lint.py b/tests/test_custom_lint.py index 1b3d7b7e..169476c4 100644 --- a/tests/test_custom_lint.py +++ b/tests/test_custom_lint.py @@ -3350,3 +3350,129 @@ def _write_pair(root: Path, entry_extra: dict | None = None) -> None: market_dir = root / ".claude-plugin" market_dir.mkdir(parents=True) (market_dir / "marketplace.json").write_text(json.dumps({"name": "demo", "plugins": [entry]}), encoding="utf-8") + + +@pytest.mark.lint +class TestCE045PluginPathIsAPluginRoot: + """CE045 — a claude-code local plugin path must name a plugin ROOT, not a skills dir. + + `agent.plugins: [{type: local, path: X}]` reaches the Claude Code SDK as a + plugin directory, so a skill is found at `X/skills//SKILL.md`. Point X + one level deeper — at the directory that holds the skill directories — and + NOTHING loads. Probed against the real CLI: + + claude --plugin-dir /.claude/skills -> no skills + claude --plugin-dir /.claude -> `.claude:probe-alpha` + + Six surfaces once said `.claude/skills` in unison: `check-skill`, `ci`, the + bundled `activation.yaml`, `docs/PLUGIN.md`, and tutorial 07 twice. The cost + of that is invisible and total — every activation suite the plugin generated + reported recall 0.0, which the template's own comment calls "reads exactly + like a broken skill", and `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. + + Nothing held those six in agreement, which is why they drifted together. The + unit under test is the VALUE, not the prose: a path whose last segment is + `skills` cannot be a plugin root, whatever the sentence around it claims. + + Codex is deliberately out of scope. `codex_agent._setup_skills` scans BOTH + layouts (`//` and `/skills//`), so a skills + directory is valid there — see the plugin-path row in + docs/agents/HARNESS_PARITY.md. The rule keys on `SKILL_SOURCE_PATH`, the + variable the claude-code plugin emits, so `PLUGIN_PATH` in the Codex docs is + untouched. + """ + + REPO_ROOT = Path(__file__).parent.parent + + # `SKILL_SOURCE_PATH=`, with or without `export`, quotes, or YAML `key: value` + # framing — the same assignment appears as shell, as a GitHub Actions `env:` line, and + # as a comment in the bundled template. + _ASSIGNMENT = re.compile(r"""SKILL_SOURCE_PATH\s*=\s*["']?([^"'\s]+)["']?""") + + # Only the surfaces a claude-code user reads. Globbed, not enumerated: a seventh + # surface must be caught by existing, not by remembering to add it here. + _GLOBS = ("plugins/coder-eval/**/*.md", "plugins/coder-eval/**/*.yaml", "docs/**/*.md") + + def _surfaces(self) -> list[Path]: + found: list[Path] = [] + for pattern in self._GLOBS: + found.extend(self.REPO_ROOT.glob(pattern)) + return sorted(found) + + @staticmethod + def _is_skills_dir(value: str) -> bool: + """Does this path's last segment name a skills directory rather than a plugin root?""" + return value.rstrip("/").rsplit("/", 1)[-1] == "skills" + + def test_no_surface_points_skill_source_path_at_a_skills_dir(self): + offenders: list[str] = [] + for path in self._surfaces(): + for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + for value in self._ASSIGNMENT.findall(line): + if self._is_skills_dir(value): + rel = path.relative_to(self.REPO_ROOT) + offenders.append(f"{rel}:{lineno}: {value}") + + assert not offenders, ( + "SKILL_SOURCE_PATH must name a PLUGIN ROOT — a directory holding `skills/` — so the " + "skill resolves at `/skills//SKILL.md`. These point one level too deep, " + "which loads no skills at all and reports recall 0.0 on every positive row:\n " + + "\n ".join(offenders) + + "\nFor `.claude/skills/my-skill/SKILL.md` the root is `.claude`, not `.claude/skills`." + ) + + def test_literal_plugin_paths_in_tasks_are_plugin_roots(self): + # The env-var form is the plugin's convention, but a task may hardcode a path. + # Same failure, no variable to inspect, so check the YAML value directly. + import yaml + + offenders: list[str] = [] + for path in sorted(self.REPO_ROOT.glob("tasks/**/*.yaml")): + try: + doc = yaml.safe_load(path.read_text(encoding="utf-8")) + except yaml.YAMLError: + continue # malformed YAML is another rule's problem + if not isinstance(doc, dict): + continue + agent = doc.get("agent") + if not isinstance(agent, dict) or str(agent.get("type", "claude-code")) != "claude-code": + continue # Codex tolerates either layout; see the class docstring + for plugin in agent.get("plugins") or []: + if not isinstance(plugin, dict) or plugin.get("type") != "local": + continue + value = str(plugin.get("path") or "") + if "$" in value: + continue # an env var's value is covered by the assignment check above + if value and self._is_skills_dir(value): + offenders.append(f"{path.relative_to(self.REPO_ROOT)}: {value}") + + assert not offenders, ( + "A local plugin `path` must be a plugin root holding `skills/`, not the skills " + "directory itself:\n " + "\n ".join(offenders) + ) + + def test_the_rule_would_catch_the_bug_it_was_written_for(self): + # Mutation guard: the pre-fix value must be rejected and the fixed one accepted, + # so a loosened matcher fails here rather than passing a regression through. + assert self._is_skills_dir(".claude/skills") + assert self._is_skills_dir("$(pwd)/.claude/skills/") + assert self._is_skills_dir("${{ github.workspace }}/.claude/skills") + assert not self._is_skills_dir(".claude") + assert not self._is_skills_dir("$(pwd)/.claude") + assert not self._is_skills_dir("/abs/path/to/.claude") + # A directory merely CONTAINING the word is a plugin root, not an offender. + assert not self._is_skills_dir("my-skills") + assert not self._is_skills_dir(".claude/skills/pdf-forms") + + def test_the_matcher_sees_every_framing_the_surfaces_use(self): + # The three real shapes: shell export, Actions `env:` line, template comment. + for line, expected in ( + ('export SKILL_SOURCE_PATH="$(pwd)/.claude"', "$(pwd)/.claude"), + (" SKILL_SOURCE_PATH=${{ github.workspace }}/.claude", "${{"), + ("# export SKILL_SOURCE_PATH=/abs/path/to/.claude", "/abs/path/to/.claude"), + ): + found = self._ASSIGNMENT.findall(line) + assert found, f"matcher missed {line!r}" + assert found[0].startswith(expected), (found, expected) diff --git a/tests/test_reference_permissions.py b/tests/test_reference_permissions.py index 6d8babc6..4c76e8bf 100644 --- a/tests/test_reference_permissions.py +++ b/tests/test_reference_permissions.py @@ -366,10 +366,36 @@ def _refuse(signum, _handler): monkeypatch.setattr("coder_eval.fs_permissions.signal.signal", _refuse) registry = _PermissionStack() + + # Compare the two calls' signal SETS, not a running total of 4. `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 — and + # under `-n auto` that depends on which tests share this worker. A count assertion + # therefore failed on a schedule change with `attempts == [INT, TERM, INT, TERM, + # INT]`, reporting a latch bug that did not exist. The set form is insensitive to + # a stray duplicate while still proving the retry: if the install were latched, + # the second call would record nothing at all. registry.ensure_crash_handlers() + first = set(attempts) + attempts.clear() registry.ensure_crash_handlers() + second = set(attempts) + + # Lift the patch HERE, not at teardown. `signal.signal` is patched module-wide, + # and 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 surfaced only when a scheduling change moved this test to a + # different `-n auto` worker, so it read as a flake rather than as the fixed + # ordering hazard it is. + monkeypatch.undo() - assert len(attempts) == 4, "a failed install must be retried on the next call, not latched" + expected = {signal.SIGINT, signal.SIGTERM} + assert first == expected, f"first install did not attempt both signals: {sorted(first)}" + assert second == expected, ( + "a failed install must be retried on the next call, not latched — the second " + f"call attempted {sorted(second)}" + ) @pytest.mark.parametrize("previous_is_callable", [True, False]) async def test_signal_handler_restores_then_chains(self, guarded_dir, monkeypatch, previous_is_callable): From 754805b196a23f399d88c862ce48bdc25a9cec73 Mon Sep 17 00:00:00 2001 From: uipreliga Date: Thu, 27 Aug 2026 17:04:05 -0700 Subject: [PATCH 3/4] =?UTF-8?q?fix(plugin):=20close=20the=20review's=20ver?= =?UTF-8?q?ified=20gaps=20=E2=80=94=20regex=20blind=20spot,=20parallel=20s?= =?UTF-8?q?urfaces,=20runtime=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01V5WZ1BbtygK9YVvcgPfNGh --- CLAUDE.md | 2 +- docs/AB_EXPERIMENTS.md | 10 +- docs/agents/CLAUDE_CODE.md | 12 +- docs/agents/HARNESS_PARITY.md | 30 +- experiments/default.yaml | 4 +- experiments/plugin-comparison.yaml | 3 + .../reference/templates/activation.yaml | 13 +- .../coder-eval/skills/check-skill/SKILL.md | 25 +- plugins/coder-eval/skills/ci/SKILL.md | 13 + src/coder_eval/utils.py | 19 +- tests/test_custom_lint.py | 271 ++++++++++++------ tests/test_plugin_processing.py | 54 ++++ tests/test_reference_permissions.py | 50 ++-- 13 files changed, 385 insertions(+), 121 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f4320f48..70552191 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -144,7 +144,7 @@ action.yml # Published composite GitHub Action (coder-ev - **Sub-agent token accounting**: There is NO separate per-sub-agent field. Every sub-agent generation is captured as a `parent_tool_use_id`-tagged `AssistantMessage` in the turn transcript, so per-sub-agent usage is derived by grouping those messages on that id (the evalboard's `aggregateSubAgentUsage` does exactly this). Claude bubbles its sub-agent's intermediate generations into the parent stream natively, and the **terminal** generation (delivered as the Agent tool result, never streamed) is synthesized into one via `_synthesize_subagent_terminal_message` from `tool_use_result.usage`. Codex reconstructs all child generations from the child rollout (`_recover_subagent_tool_calls`). The turn total already includes sub-agent cost — Claude via the SDK's cumulative `model_usage`; Codex via `_fold_subagent_tokens`, which folds the child messages (their real per-generation tokens) into the parent total. `CommandTelemetry.result_summary` is stored **untruncated** (no 200-char cap) so sub-agent returns are preserved whole. Set `CODER_EVAL_RAW_SDK_LOG=1` to dump every raw SDK event to the task log for inspection. - **Reconciliation message (stream self-reconciles to the turn total)**: The per-message stream consistently under-reports the authoritative turn total — a fixed prompt slice (~512 input tokens on Claude) is billed on no SDK-emitted message, and sub-agent input/cache only partially bubbles up. So `EventCollector.build_turn_record` appends one synthetic `ReconciliationMessage` (`role="reconciliation"`, in the `TranscriptMessage` union) per turn, carrying the per-bucket residual = `token_usage` − Σ(assistant message buckets). The invariant: **summing the four token buckets across `TurnRecord.messages` (assistant + reconciliation) equals `token_usage` exactly**, for both Claude and Codex (Codex's stream is already complete after `_recover_subagent_tool_calls`, so its residual is usually 0 and no entry is emitted). This is what lets the evalboard SUM the message stream as the source of truth instead of reading a separate aggregate ("agent tokens"): `selectTokenTotals` returns the stream sum whenever a reconciliation entry is present, and the timeline renders it as its own row. It is agent-agnostic (booked at the single `EventCollector` seam), carries no cost (cost stays on `token_usage`), and is excluded from generation/turn counts and the cost simulator. The LiteLLM open-weight actual-cost join (`litellm_cost.apply_actual_cost`) deliberately writes cost at the TURN level only (`token_usage.total_cost_usd` = the real OpenRouter bill) plus the per-call `TurnRecord.provider_call_costs` audit record; it does NOT touch the message token buckets, so `EventCollector` stays the single writer and this invariant holds on every backend. The Python `token_usage`/`total_token_usage` aggregate is unchanged and still authoritative for budget/judges/reports. - **Reference solutions are directory-only, and shielded (partially) from the agent**: `task.reference` is a single required `directory:` (relative to the task YAML) — the inline `code:` / single-file `file:` forms are gone, because a directory is the only shape that can be permission-gated as a unit; a `model_validator(mode="before")` gives the removed forms a migration error. The orchestrator stages a **per-run private copy** (`orchestration/evaluation.py::stage_reference_dir`, symlinks stripped) into a tempdir, removed in `_cleanup` via `path_utils.rmtree_restrictive` (keyed on `_reference_staging_root`, recorded BEFORE the copy so a failed copy still cleans up; `rmtree(ignore_errors=True)` silently declines on a tree left at 000) and deliberately never preserved into `run_dir/artifacts`. That copy is held at mode `000` for the whole of every `agent.communicate` call via **`Sandbox.set_permissions`**, the driver-aware wrapper over `fs_permissions.py::set_permissions`. Windows **stack**: exiting restores the *enclosing* window's mode, only the outermost exit restores the pre-window mode — that is what makes a mid-turn re-grant (`mode=READ_ONLY_MODE`) expressible, and it covers two windows at the same mode so no refcount is needed. The window is enforced **only inside a docker container** (`Sandbox.enforces_permission_windows`) and is a no-op on the host, where the agent shares our uid. **That gate keys on the `CODER_EVAL_IN_CONTAINER` env var, NOT `sandbox.driver`** — `run_task_internal_command` rewrites `driver: docker` → `tempdir` before building the in-container Orchestrator, so a driver-based gate would silently disable the anti-cheat on exactly the path that needs it (regression-guarded by `TestSandboxDriverGate`); `resolve_reference_dir` gates its `/work/references` branch on the same var for the same reason. The task directory is **not** shielded (`:ro` mount → EROFS, and the same YAML is readable at `/work/input`). Criteria address reference files with the `$REFERENCE_DIR` token (same resolver as `$TASK_DIR`) and the `REFERENCE_DIR` env var for `run_command`; `reference_comparison` names one file via `reference_file`. Docker mounts a throwaway **read-write** copy at `/work/references` (a `:ro` mount cannot be chmod'd — EROFS), masks the in-task-dir original with an empty tmpfs, and drops `DAC_OVERRIDE`/`DAC_READ_SEARCH`. `FOWNER`/`CHOWN` are deliberately **NOT** dropped: the in-container orchestrator that applies the window is the same root process with the same caps, so dropping `FOWNER` breaks *the harness's own* chmod wherever the bind mount preserves a non-root owner (native Linux — verified: `chmod: Operation not permitted`), i.e. exactly where the drop would otherwise bite. A window that cannot be applied is now a hard error, not a warning: `Sandbox.set_permissions` passes `strict=True` whenever it enforces, so an unprotected run fails instead of producing a normal-looking score. **KNOWN GAP — this is defense-in-depth, not a boundary**: (a) `chmod(2)` is gated on owner-or-`CAP_FOWNER` and the container runs as root owning the copy, so a deliberate `chmod 755 /work/references` restores access; (b) the window spans `agent.communicate` only, and nothing reaps agent child processes at turn end, so a backgrounded read loop succeeds once the window closes. The **write** half of (b) is closed — `path_utils.digest_tree` hashes the tree at staging and `Orchestrator._verify_reference_integrity` re-checks before grading, raising `ReferenceTamperedError` (→ `FinalStatus.ERROR`) on a mismatch so an agent cannot overwrite the reference to drive `reference_comparison` to 1.0. Passive reads are blocked; an adversarial agent is not. Full containment requires running the agent as a non-root uid AND holding the window for the agent's whole lifetime — follow-up. `tasks/anti_cheat_reference` probes the passive-read half. -- **Harness run-limit parity**: a shared `BaseAgentConfig` field must mean the same thing on every backend, so a divergence is either fixed or documented — never silent. **`run_limits.max_turns` on Codex/Antigravity counts VISIBLE turns** (resolved tool calls, read live off the shared `EventCollector.visible_turn_count`, the same list `TurnRecord.commands` holds) because one `communicate()` is a single SDK turn on both, so a native counter would clamp at 1; claude-code keeps its native SDK cap, whose unit (an agent-loop turn) absorbs arbitrarily many parallel calls — the same number is NOT the same budget across harnesses. The cap is enforced on the same loop boundary as the cooperative early stop and finalizes cleanly as `max_turns_exhausted` (no crash, no retry); on Antigravity that boundary lives in `_drain()`, so the background-work poll loop honors it too. Known unfixed divergences: `permission_mode` on Codex and Antigravity (both run unconfined — the sandbox driver is the isolation boundary), `disallowed_tools` on Codex (forwarded, not SDK-enforced), `allowed_tools`/`disallowed_tools` on Antigravity (not read at all), and `turn_timeout` on Antigravity (bounded by an earlier internal poll deadline at 80% of it). Full table + rationale: docs/agents/HARNESS_PARITY.md. +- **Harness run-limit parity**: a shared `BaseAgentConfig` field must mean the same thing on every backend, so a divergence is either fixed or documented — never silent. **`run_limits.max_turns` on Codex/Antigravity counts VISIBLE turns** (resolved tool calls, read live off the shared `EventCollector.visible_turn_count`, the same list `TurnRecord.commands` holds) because one `communicate()` is a single SDK turn on both, so a native counter would clamp at 1; claude-code keeps its native SDK cap, whose unit (an agent-loop turn) absorbs arbitrarily many parallel calls — the same number is NOT the same budget across harnesses. The cap is enforced on the same loop boundary as the cooperative early stop and finalizes cleanly as `max_turns_exhausted` (no crash, no retry); on Antigravity that boundary lives in `_drain()`, so the background-work poll loop honors it too. Known unfixed divergences: `permission_mode` on Codex and Antigravity (both run unconfined — the sandbox driver is the isolation boundary), `disallowed_tools` on Codex (forwarded, not SDK-enforced), `allowed_tools`/`disallowed_tools` on Antigravity (not read at all), `turn_timeout` on Antigravity (bounded by an earlier internal poll deadline at 80% of it), and **`agent.plugins[].path` depth** — claude-code REQUIRES a plugin root holding `skills/` and silently loads NOTHING from a bare skills directory, while Codex and Antigravity scan both depths and accept either. That is the costly direction: the wrong depth produces no error, every positive row of an activation suite scores 0, and the suite reports recall 0.0, which reads exactly like a skill that never triggers. Held to the plugin-root shape (for `SKILL_SOURCE_PATH` only) by lint rule CE045. Full table + rationale: docs/agents/HARNESS_PARITY.md. - **sandbox isolation**: Tasks that don't need MCP servers should set `setting_sources: []` in their `agent:` block to isolate the sandbox from the host project's CLAUDE.md and settings. Without this, the host project's CLAUDE.md (often 20 KB+) is injected into every API call, inflating cache-creation tokens and cost significantly. - **Run-time caps (non-criterion enforcement)**: `TaskDefinition.run_limits` (`RunLimits` model) is the single namespace for all *task-level* run-time caps — `max_turns` / `task_timeout` / `turn_timeout` (structural) and `max_input_tokens` / `max_output_tokens` / `max_total_tokens` / `max_usd` (cumulative budget). Token/USD breaches abort with `FinalStatus.TOKEN_BUDGET_EXCEEDED` or `COST_BUDGET_EXCEEDED` (both `category == "failed"`). Structural caps are set from the CLI via `-D run_limits.max_turns=…` / `-D run_limits.task_timeout=…` / `-D run_limits.turn_timeout=…` (field-merged into `run_limits`); budget caps via `-D run_limits.max_usd=…` etc. or YAML. Layered config uses field-merge — a variant block overrides individual keys without replacing the task's block. The one *per-criterion* cap, `stop_early.decide_within`, deliberately lives on `LiveSuccessCriterion` instead (see below) — the watcher must attribute a decision-step timeout to a specific criterion, which `RunLimits` (task-scoped, criterion-agnostic) cannot express. - **Early stop on criterion (opt-in, per-criterion arming)**: a `stop_early:` block (`StopEarlyPolicy`) on a criterion ends a single-shot run early once the run's **armed** criteria decide the outcome, so a raised `max_turns` isn't wasted on the smoke flavor. The block's PRESENCE is the arming and alone activates the watcher — there is **no run-level master switch**: `run_limits.stop_early: false` is the run-level KILL SWITCH that force-disarms every block (the one-line experiment-variant/`-D` override for an authoritative full run), and `run_limits.stop_early: true` (the removed master arm) is a hard `EarlyStopConfigError` at resolution. The block exists on `LiveSuccessCriterion` only (currently `skill_triggered`, `command_executed` — so arming an unobservable criterion is unrepresentable, a pydantic extra-forbid error). Arming carries one implicit trigger (a native live-fail may fail-stop the run); its keys refine it: `on_pass: stop` (pass-stop the moment the criterion live-passes; default `continue` just latches) and `decide_within: N` (still undecided after N tool-call steps latches an **effective fail**, fed through the same fail-stop rule, reported as `decision_budget_exceeded` — an ordinary weighted fail, NOT a gate-bypassing force-fail; cumulative across retry attempts of the same turn). A trigger whose polarity the instance can't decide (per the abstract, checker-independent `live_decidable_polarities()`, a pure function of the criterion's own fields, paired with the checker's `live_verdict` override by lint rule CE025, a registry-based whole-tree check) is **inert by design** — one dataset-fanned YAML line serves both positive rows (pass/timeout live) and distractor rows (fail live). Verdicts **latch**: once a criterion decides, its `live_verdict` is never polled again. Stop rule is weighted, not strict-boolean: `run_limits.stop_early_gate_threshold` (default `1.0`, reproducing strict-AND behavior exactly) is the minimum weighted score (`Σ weight·score / Σ weight` over the armed subset) required to pass; a fail-stop fires once the armed set's **ceiling** (best case for everything still undecided) can no longer reach the threshold — so a low-weight fail or timeout that can't doom the gate is absorbed and the run continues — and is **deferred while any pass-capable armed criterion is undecided** (a distractor misfire never truncates a positive row's recall signal); a pass-stop fires once the `on_pass: stop` subset's **floor** (worst case) already meets the threshold, and is symmetrically **deferred while any pass-capable armed criterion outside the `on_pass: stop` subset is undecided** (so an early pass never freezes a sibling `on_pass: continue` criterion's signal out of the trajectory). A fail-stop is therefore verdict-preserving; a pass-stop can miss a *later* distractor misfire, so authoritative P/R/F1 comes from a kill-switched (`stop_early: false`) run. Driven by `orchestration/early_stop.py::EarlyStopWatcher` (built when `early_stop_active(task)`: ≥1 armed criterion, kill switch not thrown) through the agent's cooperative `should_stop` seam (tool-call granularity, no SIGKILL); live verdicts only *trigger* the stop — the standard `check_all_async` on the frozen trajectory is authoritative. Gating is **FIRED-ONLY**: a run the watcher actually cut gates on the **armed subset** via the weighted `EvaluationResult.armed_criteria_passed`; a run that completes naturally — armed or not — gates strict-AND via `all_criteria_passed`, so adding a block never changes the verdict of a run it didn't cut. Note the gate keys on the watcher having FIRED (`result.early_stop is not None`), not on confirmed truncation — an agent that ignores `should_stop`, or a stop firing on the final message, still gates armed-only. Every resolution-time guardrail violation is a hard error at resolution (plan *and* run); the one load-time case — a `stop_early:` block on a non-live criterion — is a pydantic schema error at task load, which the run surface reports as a skipped task like any other malformed task. A runtime verdict bug **fails open** to a full run. Surfaces: `EarlyStopInfo` (incl. `gate_threshold` at stop time), report notes/badges, `stopped_early` run.json rows, `EarlyStopped`/`EarlyStopReason` telemetry dims. Worked rationale: docs/TASK_DEFINITION_GUIDE.md § `stop_early`. No blocks anywhere ⇒ behavior byte-for-byte unchanged. diff --git a/docs/AB_EXPERIMENTS.md b/docs/AB_EXPERIMENTS.md index 0e73d6a3..4efb0dde 100644 --- a/docs/AB_EXPERIMENTS.md +++ b/docs/AB_EXPERIMENTS.md @@ -181,7 +181,7 @@ variants: agent: plugins: - type: "local" - path: "../skills" # skill available + path: ".." # PLUGIN ROOT holding skills/ — see note below ``` Notes: @@ -193,8 +193,14 @@ Notes: it. Pair the experiment with a [`skill_triggered`](TASK_DEFINITION_GUIDE.md#skill_triggered) criterion to measure _whether it fired_ alongside your real success criteria that measure _whether outcomes improved_. +- **`path` must be a plugin ROOT — a directory holding `skills/`** — so the skill + resolves at `/skills//SKILL.md`. Point one level deeper, at the + directory of skill directories, and claude-code loads **nothing**: the `with-skill` + arm then silently matches the baseline and the A/B compares two identical arms. + Codex and Antigravity accept either depth, so this fails on claude-code alone — + see [Harness parity](agents/HARNESS_PARITY.md#agentpluginspath-accepts-different-depths-per-harness). - Plugin paths are environment-dependent. The shipped example expects a - `$PLUGIN_PATH` env var pointing at your plugin directory. See + `$PLUGIN_PATH` env var pointing at your plugin **root**. See `experiments/plugin-comparison.yaml`. Run it: diff --git a/docs/agents/CLAUDE_CODE.md b/docs/agents/CLAUDE_CODE.md index c2ccc453..84e2146c 100644 --- a/docs/agents/CLAUDE_CODE.md +++ b/docs/agents/CLAUDE_CODE.md @@ -98,7 +98,7 @@ agent: | `permission_mode` | default **`acceptEdits`** | `default` / `acceptEdits` / `plan` / `bypassPermissions` — semantics come from the Claude Code SDK. `plan` is read-only; `bypassPermissions` grants full autonomy. | | `allowed_tools` | `list[str] \| null` | Tool allowlist. Unset ⇒ all tools allowed. | | `disallowed_tools` | `list[str] \| null` | Tool denylist. (`ToolSearch` is always appended for Bedrock parity.) | -| `plugins` | `list[{type: local, path}]` | Local plugin/skill directories; `$VAR` in `path` is expanded and resolved to an absolute path. | +| `plugins` | `list[{type: local, path}]` | Local plugin roots; `$VAR` in `path` is expanded and resolved to an absolute path. **`path` must hold a `skills/` subdirectory** — a bare directory of skill directories loads nothing here, though Codex and Antigravity accept it. See [Harness parity](HARNESS_PARITY.md#agentpluginspath-accepts-different-depths-per-harness). | | `system_prompt` | `str \| null` | **Appended** to the default Claude Code system prompt (via the SDK's `claude_code` preset) — the default's behavioral guidance is kept unless `system_prompt_mode: replace` opts out. Mutually exclusive with `system_prompt_file`. An empty or whitespace-only value is treated as unset. | | `system_prompt_mode` | `"append"` (default) / `"replace"` | `replace` sends `system_prompt` as the **entire** system prompt (no preset) and requires a non-blank `system_prompt` / `system_prompt_file` (validated at load). Used by judge sub-agents and the user simulator, which must not carry the coding-agent persona; rarely needed in tasks — see [the migration note](#migrating-tasks-that-set-system_prompt). | | `system_prompt_file` | `str \| null` | Path (relative to the task YAML) loaded into `system_prompt` at resolution. Works with either `system_prompt_mode`. | @@ -183,6 +183,16 @@ simulator force `[]` for the same reason.) that call and scores skill-activation suites. - **Plugins** are supplied as `plugins: [{type: local, path: …}]`; the path is env-expanded and resolved to an absolute path before being handed to the SDK. + It must name a **plugin root** — a directory holding `skills/`, so the skill + resolves at `/skills//SKILL.md`. One level deeper loads nothing, with + no error: every positive row of an activation suite then scores 0 and the suite + reports recall 0.0, which reads exactly like a skill that never triggers. Codex + and Antigravity scan both depths, so this is claude-code-only — see + [Harness parity](HARNESS_PARITY.md#agentpluginspath-accepts-different-depths-per-harness). +- A plugin root loads **everything** the plugin declares, not only `skills/`: an + `agents/`, `commands/` or `hooks/` directory beside it becomes visible to the + evaluated agent too. Point at a minimal root when the suite must measure one + skill in isolation. - **`PLUGIN_TOOLS_DIR`** pins the canonical `node_modules/@uipath` directory for UiPath CLI plugin discovery; when unset the sandbox derives it from the resolved `uip` binary. See [User Guide → Environment Variables](../USER_GUIDE.md#environment-variables). diff --git a/docs/agents/HARNESS_PARITY.md b/docs/agents/HARNESS_PARITY.md index d8302bdb..af7f352c 100644 --- a/docs/agents/HARNESS_PARITY.md +++ b/docs/agents/HARNESS_PARITY.md @@ -126,18 +126,36 @@ indistinguishable from a skill that never triggers, which is the finding such a exists to produce. It shipped in six documentation surfaces at once for exactly this reason. -Probe it in one command, against whichever harness you doubt: +Probe it — but **read the namespace, not the presence**. Claude Code discovers a +project's own `./.claude/skills/` natively, independent of `--plugin-dir`, so run +from a repo root and BOTH commands list the skill: the deeper one only looks +correct. The plugin loaded iff the name carries the root's prefix. ```bash -claude --plugin-dir "$(pwd)/.claude" # loads .claude: -claude --plugin-dir "$(pwd)/.claude/skills" # loads nothing +# Run from a directory that is NOT the skill's own repo root. +claude --plugin-dir /path/to/root # lists `root:` <- plugin loaded +claude --plugin-dir /path/to/root/skills # lists nothing <- loaded nothing ``` +A bare `` with no prefix is project discovery, not your plugin. + **Write the plugin root.** It is correct on all three, so there is never a reason to write the deeper form. For `.claude/skills/my-skill/SKILL.md` that is `.claude`. -`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. + +Note what else that pulls in: a plugin root loads the **whole** plugin, so an +`agents/`, `commands/` or `hooks/` directory sitting beside `skills/` becomes visible +to the evaluated agent as well. Verified — 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` and `root:probe-cmd`. Pointing a suite at a +repo's `.claude` therefore hands the agent every project subagent, which can answer a +request the skill was supposed to answer. Stage a minimal root when the suite must +isolate one skill. + +`SKILL_SOURCE_PATH` — the variable `/coder-eval:check-skill` emits — is held to the +plugin-root shape by lint rule CE045. The rule keys on that variable name only; it is +**not** a statement that other variables may use the deeper form. `$PLUGIN_PATH`, for +one, feeds `experiments/plugin-comparison.yaml`, whose default agent is claude-code, +so the same requirement applies there and is unlinted. ## Reproducing diff --git a/experiments/default.yaml b/experiments/default.yaml index 5af7eef2..ebf627b5 100644 --- a/experiments/default.yaml +++ b/experiments/default.yaml @@ -59,7 +59,9 @@ defaults: allowed_tools: ["Bash", "Read", "Write", "Edit", "Glob", "Grep", "Skill"] # Claude Code plugins to load (null = none) - # Example: [{"type": "local", "path": "/path/to/skills"}] + # `path` must be a PLUGIN ROOT: a directory holding skills/, so the skill sits at + # /skills//SKILL.md. One level deeper loads nothing on claude-code. + # Example: [{"type": "local", "path": "/path/to/plugin-root"}] plugins: null # Additional patterns to ignore during file change detection (beyond defaults) diff --git a/experiments/plugin-comparison.yaml b/experiments/plugin-comparison.yaml index 5aa4f7c9..9faab36c 100644 --- a/experiments/plugin-comparison.yaml +++ b/experiments/plugin-comparison.yaml @@ -33,5 +33,8 @@ variants: - variant_id: with-plugin agent: plugins: + # $PLUGIN_PATH must be a PLUGIN ROOT (a directory holding skills/). The default + # agent here is claude-code, which loads nothing from a bare skills directory — + # so a wrong depth makes this arm identical to `without-plugin` with no error. - type: "local" path: "$PLUGIN_PATH" diff --git a/plugins/coder-eval/reference/templates/activation.yaml b/plugins/coder-eval/reference/templates/activation.yaml index 3432096e..6d91bdd5 100644 --- a/plugins/coder-eval/reference/templates/activation.yaml +++ b/plugins/coder-eval/reference/templates/activation.yaml @@ -14,7 +14,18 @@ tags: [activation] # subdirectory, so the skill sits at `/skills//SKILL.md`. For # `.claude/skills/my-skill/SKILL.md` that root is `.claude`, NOT `.claude/skills` — # pointing one level too deep loads nothing at all. Supply it through an environment -# variable so the committed suite stays portable across machines and CI: +# variable so the committed suite stays portable across machines and CI. +# +# PREFER A MINIMAL ROOT holding only the skill under test. A plugin root loads the whole +# plugin, so agents/, commands/ and hooks/ sitting beside skills/ reach the evaluated +# agent too — a project subagent that can answer the request will depress recall for +# reasons unrelated to the skill, and makes the number repo-dependent: +# +# SKILL_ROOT=$(mktemp -d) && mkdir -p "$SKILL_ROOT/skills" +# ln -s /abs/path/to/.claude/skills/my-skill "$SKILL_ROOT/skills/my-skill" +# export SKILL_SOURCE_PATH="$SKILL_ROOT" +# +# The whole-tree form works too, with that caveat: # # export SKILL_SOURCE_PATH=/abs/path/to/.claude # diff --git a/plugins/coder-eval/skills/check-skill/SKILL.md b/plugins/coder-eval/skills/check-skill/SKILL.md index 380be20f..4cfab74a 100644 --- a/plugins/coder-eval/skills/check-skill/SKILL.md +++ b/plugins/coder-eval/skills/check-skill/SKILL.md @@ -170,13 +170,32 @@ subdirectory**, so that the skill sits at `/skills//SKILL.md`. directory's own name. For `.claude/skills/pdf-forms/SKILL.md` that root is **`.claude`**, not `.claude/skills`. -Pointing at a bare directory of skill directories loads nothing at all. Tell the user to -export it before running, and to use the same variable in CI: +Pointing at a bare directory of skill directories loads nothing at all. + +**Stage a minimal root rather than pointing at `.claude` itself.** A plugin root loads +the WHOLE plugin, not just its skills: an `agents/`, `commands/` or `hooks/` directory +sitting beside `skills/` becomes visible to the evaluated agent too. Point at a repo's +`.claude` and you hand the agent every project subagent and command — and a subagent +that can answer the request is a confound, not a detail. If `.claude/agents/pdf-expert.md` +exists while you measure `pdf-forms`, the agent may delegate to it instead of calling the +skill; `skill_triggered` records `no`, and recall drops for a reason that has nothing to +do with the skill's description. It also makes the number repo-dependent, so two suites +are no longer comparable. + +So build a root that contains exactly the unit under test, and point at that: ```bash -export SKILL_SOURCE_PATH="$(pwd)/.claude" +SKILL_ROOT="$(mktemp -d)" +mkdir -p "$SKILL_ROOT/skills" +ln -s "$(pwd)/.claude/skills/pdf-forms" "$SKILL_ROOT/skills/pdf-forms" +export SKILL_SOURCE_PATH="$SKILL_ROOT" ``` +Use `cp -R` instead of `ln -s` where symlinks are awkward (Windows, some CI images). If +the user prefers the one-liner, `export SKILL_SOURCE_PATH="$(pwd)/.claude"` still works — +say plainly that it also exposes everything else under `.claude`, so a low recall may be +the siblings rather than the skill. + Keep it an environment variable rather than baking an absolute path into the YAML — the suite is committed and re-run on other machines. If the variable is unset the skill is simply absent, every positive row scores 0, and the result is indistinguishable from a skill that diff --git a/plugins/coder-eval/skills/ci/SKILL.md b/plugins/coder-eval/skills/ci/SKILL.md index c5bd203a..3d1b2729 100644 --- a/plugins/coder-eval/skills/ci/SKILL.md +++ b/plugins/coder-eval/skills/ci/SKILL.md @@ -163,6 +163,19 @@ above — and note **what level that variable points at**. A local plugin path m **not** `.claude/skills`. Pointing one level too deep loads nothing at all and produces the same permanent red as leaving it unset. +If the suite stages a minimal root (which `/coder-eval:check-skill` recommends, so that +sibling subagents and commands under `.claude` cannot confound the measurement), the +workflow has to build it before the run — a scheduled job has no shell history to +inherit it from: + +```yaml +- name: Stage the skill under test as a minimal plugin root + run: | + mkdir -p "$RUNNER_TEMP/skill-root/skills" + cp -R "${{ github.workspace }}/.claude/skills/my-skill" "$RUNNER_TEMP/skill-root/skills/" + echo "SKILL_SOURCE_PATH=$RUNNER_TEMP/skill-root" >> "$GITHUB_ENV" +``` + This is the one omission the scheduled trigger cannot survive: unset, the skill is never offered to the sandboxed agent, every positive row scores 0, and the job fails its `recall` threshold every week — a permanent red that looks exactly like the drift the diff --git a/src/coder_eval/utils.py b/src/coder_eval/utils.py index 1624dae6..dccd84ae 100644 --- a/src/coder_eval/utils.py +++ b/src/coder_eval/utils.py @@ -79,7 +79,24 @@ def process_plugins( # Expand all env vars in the path, then resolve relative paths # against the process cwd (not the sandbox cwd) so plugins are found expanded = expand_env_vars(path) - processed_plugin["path"] = str(Path(expanded).resolve()) + resolved = Path(expanded).resolve() + processed_plugin["path"] = str(resolved) + + # Loud: claude-code loads a local plugin as a PLUGIN ROOT, so its skills must + # sit at /skills//SKILL.md. Point one level deeper — at the bare + # directory of skill directories — and the SDK loads NOTHING, with no error: + # every positive row of an activation suite scores 0 and the suite reports + # recall 0.0, which reads exactly like a skill that never triggers. This + # function is claude-code-only (see the module docstring); Codex and + # Antigravity scan both depths and warn for themselves. Warn rather than + # raise — a plugin may legitimately ship only agents/, commands/ or hooks/. + if plugin.get("type") == "local" and resolved.is_dir() and not (resolved / "skills").is_dir(): + log.warning( + f"Plugin path has no skills/ subdirectory, so it loads no skills: {resolved}. " + "A local plugin path must be a PLUGIN ROOT holding skills/ " + "(for .claude/skills/my-skill/SKILL.md that is .claude, not .claude/skills). " + "See docs/agents/HARNESS_PARITY.md." + ) processed.append(processed_plugin) diff --git a/tests/test_custom_lint.py b/tests/test_custom_lint.py index 169476c4..3693dbae 100644 --- a/tests/test_custom_lint.py +++ b/tests/test_custom_lint.py @@ -12,6 +12,7 @@ import json import re +import subprocess from pathlib import Path import pytest @@ -3356,65 +3357,115 @@ def _write_pair(root: Path, entry_extra: dict | None = None) -> None: class TestCE045PluginPathIsAPluginRoot: """CE045 — a claude-code local plugin path must name a plugin ROOT, not a skills dir. - `agent.plugins: [{type: local, path: X}]` reaches the Claude Code SDK as a - plugin directory, so a skill is found at `X/skills//SKILL.md`. Point X - one level deeper — at the directory that holds the skill directories — and - NOTHING loads. Probed against the real CLI: - - claude --plugin-dir /.claude/skills -> no skills - claude --plugin-dir /.claude -> `.claude:probe-alpha` - - Six surfaces once said `.claude/skills` in unison: `check-skill`, `ci`, the - bundled `activation.yaml`, `docs/PLUGIN.md`, and tutorial 07 twice. The cost - of that is invisible and total — every activation suite the plugin generated - reported recall 0.0, which the template's own comment calls "reads exactly - like a broken skill", and `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. - - Nothing held those six in agreement, which is why they drifted together. The - unit under test is the VALUE, not the prose: a path whose last segment is - `skills` cannot be a plugin root, whatever the sentence around it claims. - - Codex is deliberately out of scope. `codex_agent._setup_skills` scans BOTH - layouts (`//` and `/skills//`), so a skills - directory is valid there — see the plugin-path row in - docs/agents/HARNESS_PARITY.md. The rule keys on `SKILL_SOURCE_PATH`, the - variable the claude-code plugin emits, so `PLUGIN_PATH` in the Codex docs is - untouched. + `agent.plugins: [{type: local, path: X}]` reaches the Claude Code SDK as a plugin + directory, so a skill is found at `X/skills//SKILL.md`. Point X one level + deeper — at the directory that holds the skill directories — and NOTHING loads. + Probed against the real CLI, from a cwd that is not the skill's own repo (project + discovery would otherwise find it regardless of `--plugin-dir`, and the namespace + prefix is the real signal): + + claude --plugin-dir /skills -> nothing + claude --plugin-dir -> `root:probe-beta` + + The cost is invisible and total: every activation suite the plugin generated + reported recall 0.0, which the bundled template's own comment calls "reads exactly + like a broken skill", and `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. + + INCIDENT RECORD — the corpus below is that record, not this prose. Six wrong-value + lines across five files shipped at once: docs/PLUGIN.md, tutorial 07, + activation.yaml (comment and example), check-skill, and ci. Nothing held them in + agreement, which is why they drifted together. + + The unit under test is the VALUE, not the sentence around it: a path whose last + segment is `skills` cannot be a plugin root, whatever the prose claims. + + SCOPE. The rule keys on `SKILL_SOURCE_PATH`, the variable the plugin emits. That is + a limit, NOT a statement that other variables may use the deeper form — `$PLUGIN_PATH` + feeds `experiments/plugin-comparison.yaml`, whose default agent is claude-code, and + is unlinted. The guard that reaches every user, including the repos where + `/coder-eval:check-skill` actually writes suites, is the runtime warning in + `utils.process_plugins`; this rule only keeps THIS repo's shipped strings honest. """ REPO_ROOT = Path(__file__).parent.parent - # `SKILL_SOURCE_PATH=`, with or without `export`, quotes, or YAML `key: value` - # framing — the same assignment appears as shell, as a GitHub Actions `env:` line, and - # as a comment in the bundled template. - _ASSIGNMENT = re.compile(r"""SKILL_SOURCE_PATH\s*=\s*["']?([^"'\s]+)["']?""") + # Verbatim pre-fix lines, one per surface that shipped the wrong value. The mutation + # guard replays these through the FULL extract-then-predicate pipeline. An earlier + # revision asserted the predicate against hand-written strings the matcher could + # never produce, which is how the Actions form below stayed unreachable while the + # rule looked covered. + KNOWN_BAD_LINES = ( + 'export SKILL_SOURCE_PATH="$(pwd)/.claude/skills"', + "# export SKILL_SOURCE_PATH=/abs/path/to/.claude/skills", + " SKILL_SOURCE_PATH=${{ github.workspace }}/.claude/skills", + "export SKILL_SOURCE_PATH=$HOME/repo/.claude/skills/", + ) - # Only the surfaces a claude-code user reads. Globbed, not enumerated: a seventh - # surface must be caught by existing, not by remembering to add it here. - _GLOBS = ("plugins/coder-eval/**/*.md", "plugins/coder-eval/**/*.yaml", "docs/**/*.md") + # Value runs to end-of-line or a closing quote, NOT to the first space: the GitHub + # Actions form `${{ github.workspace }}/.claude/skills` contains spaces INSIDE the + # expression, and a whitespace-terminated pattern captured a bare `${{` — silently + # exempting the highest-cost surface, the one `ci` writes into users' workflows. + _ASSIGNMENT = re.compile(r"""SKILL_SOURCE_PATH\s*=\s*(?:"([^"]*)"|'([^']*)'|([^"'\n]*))""") + + # Every tracked file that can carry the token. Kept in step with the tree by + # `test_globs_cover_every_file_naming_the_token`, so "globbed, not enumerated" is + # true by construction rather than aspirational. + _GLOBS = ( + "*.md", + "docs/**/*.md", + "plugins/**/*.md", + "plugins/**/*.yaml", + "tasks/**/*.yaml", + "tasks/**/*.yml", + "experiments/**/*.yaml", + ".github/workflows/*.yml", + ".github/workflows/*.yaml", + ) def _surfaces(self) -> list[Path]: - found: list[Path] = [] + found: set[Path] = set() for pattern in self._GLOBS: - found.extend(self.REPO_ROOT.glob(pattern)) + found.update(self.REPO_ROOT.glob(pattern)) return sorted(found) + @classmethod + def _values(cls, line: str) -> list[str]: + """Every SKILL_SOURCE_PATH value on one line, whichever quoting it uses. + + `finditer`, not `findall`: an unmatched alternation group is None here but an + empty STRING in findall's tuples, so picking "the first non-None group" off a + findall tuple silently selects the empty quoted branch every time. + """ + values = [] + for match in cls._ASSIGNMENT.finditer(line): + captured = next((g for g in match.groups() if g is not None), "") + if captured.strip(): + values.append(captured.strip()) + return values + @staticmethod def _is_skills_dir(value: str) -> bool: """Does this path's last segment name a skills directory rather than a plugin root?""" - return value.rstrip("/").rsplit("/", 1)[-1] == "skills" + # Drop `${{ ... }}` expressions first: their inner spaces are not path separators, + # and what matters is the literal tail written after them. + literal = re.sub(r"\$\{\{.*?\}\}", "", value) + return literal.rstrip("/").rsplit("/", 1)[-1] == "skills" def test_no_surface_points_skill_source_path_at_a_skills_dir(self): offenders: list[str] = [] + scanned = 0 for path in self._surfaces(): for lineno, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): - for value in self._ASSIGNMENT.findall(line): + for value in self._values(line): + scanned += 1 if self._is_skills_dir(value): - rel = path.relative_to(self.REPO_ROOT) - offenders.append(f"{rel}:{lineno}: {value}") + offenders.append(f"{path.relative_to(self.REPO_ROOT)}:{lineno}: {value}") + # Non-vacuity: a rule that silently matches nothing passes forever. If the docs + # move or the plugin directory is renamed, fail here rather than go quiet. + assert scanned, "CE045 found no SKILL_SOURCE_PATH assignment at all — the globs have gone stale" assert not offenders, ( "SKILL_SOURCE_PATH must name a PLUGIN ROOT — a directory holding `skills/` — so the " "skill resolves at `/skills//SKILL.md`. These point one level too deep, " @@ -3423,56 +3474,116 @@ def test_no_surface_points_skill_source_path_at_a_skills_dir(self): + "\nFor `.claude/skills/my-skill/SKILL.md` the root is `.claude`, not `.claude/skills`." ) - def test_literal_plugin_paths_in_tasks_are_plugin_roots(self): - # The env-var form is the plugin's convention, but a task may hardcode a path. - # Same failure, no variable to inspect, so check the YAML value directly. - import yaml + def test_globs_cover_every_file_naming_the_token(self): + """The "globbed, not enumerated" claim, made checkable. + A surface that names the token but sits outside `_GLOBS` is unscanned, and the + rule reports success over it. Deriving the expected set from the tracked tree + means a new surface either falls inside the globs or fails the build. + """ + tracked = subprocess.run( + ["git", "grep", "-l", "SKILL_SOURCE_PATH", "--", "."], + cwd=self.REPO_ROOT, + capture_output=True, + text=True, + check=False, + ).stdout.split() + covered = {p.relative_to(self.REPO_ROOT).as_posix() for p in self._surfaces()} + # This file holds the corpus, so it names the token by construction. + missed = [f for f in tracked if f not in covered and not f.endswith("tests/test_custom_lint.py")] + assert not missed, ( + "these files name SKILL_SOURCE_PATH but no CE045 glob reaches them, so the rule " + f"reports success over them: {missed}. Widen `_GLOBS`." + ) + + def test_literal_plugin_paths_are_plugin_roots(self): + """Config surfaces that write a `path:` literal rather than the env var.""" offenders: list[str] = [] - for path in sorted(self.REPO_ROOT.glob("tasks/**/*.yaml")): - try: - doc = yaml.safe_load(path.read_text(encoding="utf-8")) - except yaml.YAMLError: - continue # malformed YAML is another rule's problem - if not isinstance(doc, dict): + for pattern in ("tasks/**/*.yaml", "experiments/**/*.yaml"): + for path in sorted(self.REPO_ROOT.glob(pattern)): + offenders.extend(self._offending_paths_in(path)) + assert not offenders, ( + "A local plugin `path` must be a plugin root holding `skills/`, not the skills " + "directory itself:\n " + "\n ".join(offenders) + ) + + def _offending_paths_in(self, path: Path) -> list[str]: + import yaml + + try: + doc = yaml.safe_load(path.read_text(encoding="utf-8")) + except yaml.YAMLError: + return [] # malformed YAML is another rule's problem + if not isinstance(doc, dict): + return [] + blocks = [doc.get("agent")] + blocks.append((doc.get("defaults") or {}).get("agent") if isinstance(doc.get("defaults"), dict) else None) + for variant in doc.get("variants") or []: + if isinstance(variant, dict): + blocks.append(variant.get("agent")) + + found: list[str] = [] + for agent in blocks: + if not isinstance(agent, dict): continue - agent = doc.get("agent") - if not isinstance(agent, dict) or str(agent.get("type", "claude-code")) != "claude-code": - continue # Codex tolerates either layout; see the class docstring for plugin in agent.get("plugins") or []: if not isinstance(plugin, dict) or plugin.get("type") != "local": continue value = str(plugin.get("path") or "") - if "$" in value: - continue # an env var's value is covered by the assignment check above - if value and self._is_skills_dir(value): - offenders.append(f"{path.relative_to(self.REPO_ROOT)}: {value}") - - assert not offenders, ( - "A local plugin `path` must be a plugin root holding `skills/`, not the skills " - "directory itself:\n " + "\n ".join(offenders) - ) + # Skip ONLY a bare variable reference, whose value lives elsewhere. A value + # that merely STARTS with a variable still has a visible literal tail — + # `$REPO_ROOT/.claude/skills` is exactly the bug, and a blanket `$` skip + # waved it through. + if not value or re.fullmatch(r"\$\{?\w+\}?/?", value): + continue + if self._is_skills_dir(value): + # A fixture tree lives outside the repo, so relativize only when it applies. + label = path.relative_to(self.REPO_ROOT) if path.is_relative_to(self.REPO_ROOT) else path + found.append(f"{label}: {value}") + return found + + def test_every_known_bad_line_is_caught_end_to_end(self): + """The mutation guard: replay the real incident lines through extract + predicate. + + Asserting the predicate alone proved half the rule and hid the other half — the + Actions form was structurally unreachable while a companion test wrote its + truncated capture down as correct. + """ + for line in self.KNOWN_BAD_LINES: + values = self._values(line) + assert values, f"matcher extracted nothing from {line!r}" + assert any(self._is_skills_dir(v) for v in values), ( + f"CE045 would NOT flag the historical offender {line!r} (extracted {values!r})" + ) - def test_the_rule_would_catch_the_bug_it_was_written_for(self): - # Mutation guard: the pre-fix value must be rejected and the fixed one accepted, - # so a loosened matcher fails here rather than passing a regression through. - assert self._is_skills_dir(".claude/skills") - assert self._is_skills_dir("$(pwd)/.claude/skills/") - assert self._is_skills_dir("${{ github.workspace }}/.claude/skills") - assert not self._is_skills_dir(".claude") - assert not self._is_skills_dir("$(pwd)/.claude") - assert not self._is_skills_dir("/abs/path/to/.claude") + def test_the_fixed_forms_are_accepted(self): + for line in ( + 'export SKILL_SOURCE_PATH="$(pwd)/.claude"', + "# export SKILL_SOURCE_PATH=/abs/path/to/.claude", + " SKILL_SOURCE_PATH=${{ github.workspace }}/.claude", + ): + values = self._values(line) + assert values, f"matcher extracted nothing from {line!r}" + assert not any(self._is_skills_dir(v) for v in values), f"false positive on {line!r}" # A directory merely CONTAINING the word is a plugin root, not an offender. assert not self._is_skills_dir("my-skills") assert not self._is_skills_dir(".claude/skills/pdf-forms") - def test_the_matcher_sees_every_framing_the_surfaces_use(self): - # The three real shapes: shell export, Actions `env:` line, template comment. - for line, expected in ( - ('export SKILL_SOURCE_PATH="$(pwd)/.claude"', "$(pwd)/.claude"), - (" SKILL_SOURCE_PATH=${{ github.workspace }}/.claude", "${{"), - ("# export SKILL_SOURCE_PATH=/abs/path/to/.claude", "/abs/path/to/.claude"), - ): - found = self._ASSIGNMENT.findall(line) - assert found, f"matcher missed {line!r}" - assert found[0].startswith(expected), (found, expected) + def test_yaml_walk_flags_a_literal_tail_behind_a_variable(self, tmp_path: Path): + """The tasks/experiments walk has no offender in-tree, so prove it on a fixture.""" + for value in (".claude/skills", "$REPO_ROOT/.claude/skills", "${REPO_ROOT}/skills/"): + task = tmp_path / "t.yaml" + task.write_text( + f'task_id: t\nagent:\n type: claude-code\n plugins:\n - type: local\n path: "{value}"\n', + encoding="utf-8", + ) + assert self._offending_paths_in(task), f"walk missed {value!r}" + + # A bare variable reference carries no literal tail to judge. + task = tmp_path / "bare.yaml" + task.write_text( + "task_id: t\nagent:\n type: claude-code\n plugins:\n" + ' - type: local\n path: "$SKILL_SOURCE_PATH"\n', + encoding="utf-8", + ) + assert not self._offending_paths_in(task) diff --git a/tests/test_plugin_processing.py b/tests/test_plugin_processing.py index 9cd7f9f0..a6468b69 100644 --- a/tests/test_plugin_processing.py +++ b/tests/test_plugin_processing.py @@ -1,5 +1,6 @@ """Tests for Claude Code plugin processing.""" +import logging from pathlib import Path from coder_eval.utils import process_plugins @@ -101,3 +102,56 @@ def test_braced_syntax_not_set(self, monkeypatch): result = process_plugins(plugins) # Path unchanged (braced var not expanded when not set) assert "${UNDEFINED_VAR}" in result[0]["path"] + + +class TestPluginRootWarning: + """A local plugin path must be a plugin ROOT, and claude-code says nothing when it isn't. + + `--plugin-dir ` resolves a skill at `/skills//SKILL.md`. Aim one + level deeper — at the bare directory of skill directories — and the SDK loads + nothing at all, with no error. Every positive row of an activation suite then + scores 0 and the suite reports recall 0.0, indistinguishable from a skill that + never triggers. That shipped in six documentation surfaces before anyone noticed. + + Codex (`codex_agent._setup_skills`) and Antigravity + (`antigravity_agent._resolve_skills_paths`) accept BOTH depths and already log + when they link zero skills. claude-code — the one harness where the wrong depth is + fatal — was the only one that stayed silent, and a repo-scoped lint rule cannot + reach the user repos where `/coder-eval:check-skill` writes these suites. + """ + + def test_plugin_root_with_skills_dir_is_quiet(self, tmp_path, caplog): + (tmp_path / "skills" / "demo").mkdir(parents=True) + (tmp_path / "skills" / "demo" / "SKILL.md").write_text("---\n---\n", encoding="utf-8") + + with caplog.at_level(logging.WARNING): + process_plugins([{"type": "local", "path": str(tmp_path)}]) + + assert "no skills/ subdirectory" not in caplog.text + + def test_bare_skills_dir_warns(self, tmp_path, caplog): + # The exact shape six surfaces once prescribed: point at `.claude/skills` itself. + skills_dir = tmp_path / "skills" + (skills_dir / "demo").mkdir(parents=True) + (skills_dir / "demo" / "SKILL.md").write_text("---\n---\n", encoding="utf-8") + + with caplog.at_level(logging.WARNING): + process_plugins([{"type": "local", "path": str(skills_dir)}]) + + assert "no skills/ subdirectory" in caplog.text + assert "PLUGIN ROOT" in caplog.text + + def test_missing_directory_does_not_warn_about_layout(self, tmp_path, caplog): + # A path that does not exist is a different failure with its own signal; claiming + # a layout problem about it would send the reader looking in the wrong place. + with caplog.at_level(logging.WARNING): + process_plugins([{"type": "local", "path": str(tmp_path / "nope")}]) + + assert "no skills/ subdirectory" not in caplog.text + + def test_non_local_plugin_is_not_checked(self, tmp_path, caplog): + # The plugin-root contract is about `type: local`; leave other source types alone. + with caplog.at_level(logging.WARNING): + process_plugins([{"type": "github", "path": str(tmp_path)}]) + + assert "no skills/ subdirectory" not in caplog.text diff --git a/tests/test_reference_permissions.py b/tests/test_reference_permissions.py index 4c76e8bf..733889ee 100644 --- a/tests/test_reference_permissions.py +++ b/tests/test_reference_permissions.py @@ -357,39 +357,39 @@ async def test_install_failure_is_not_latched(self, guarded_dir, monkeypatch): latch ``_handlers_installed = True`` regardless, so the one retry that could have succeeded (from the main thread) never happened. """ - monkeypatch.setattr("coder_eval.fs_permissions.atexit.register", lambda _fn: None) attempts: list[int] = [] def _refuse(signum, _handler): attempts.append(signum) raise ValueError("not the main thread") - monkeypatch.setattr("coder_eval.fs_permissions.signal.signal", _refuse) registry = _PermissionStack() - # Compare the two calls' signal SETS, not a running total of 4. `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 — and - # under `-n auto` that depends on which tests share this worker. A count assertion - # therefore failed on a schedule change with `attempts == [INT, TERM, INT, TERM, - # INT]`, reporting a latch bug that did not exist. The set form is insensitive to - # a stray duplicate while still proving the retry: if the install were latched, - # the second call would record nothing at all. - registry.ensure_crash_handlers() - first = set(attempts) - attempts.clear() - registry.ensure_crash_handlers() - second = set(attempts) - - # Lift the patch HERE, not at teardown. `signal.signal` is patched module-wide, - # and 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 surfaced only when a scheduling change moved this test to a - # different `-n auto` worker, so it read as a flake rather than as the fixed - # ordering hazard it is. - monkeypatch.undo() - + # SCOPE the patches, do not lift them by hand. `signal.signal` is patched + # process-wide, and the async teardown restores SIGINT by calling + # `signal.signal(SIGINT, default_int_handler)` — which hits `_refuse` and raises + # `ValueError` out of teardown, failing the test for something it does not test. + # An explicit `undo()` after the calls fixed the happy path only: if + # `ensure_crash_handlers` itself raised — the very regression this test exists to + # catch — the undo would be skipped and the teardown ValueError would MASK the + # real failure. A context manager restores on every exit path. + with monkeypatch.context() as mp: + mp.setattr("coder_eval.fs_permissions.atexit.register", lambda _fn: None) + mp.setattr("coder_eval.fs_permissions.signal.signal", _refuse) + + registry.ensure_crash_handlers() + first = set(attempts) + attempts.clear() + registry.ensure_crash_handlers() + second = set(attempts) + + # Compare the two calls' signal SETS, not a running total of 4. Anything else in + # the process that reaches fs_permissions inside the window lands in `attempts` + # too, and under `-n auto` that depends on which tests share this worker — a count + # assertion failed on a schedule change with `[INT, TERM, INT, TERM, INT]`, + # reporting a latch bug that did not exist. The set form tolerates a stray + # duplicate while still proving the retry: a latched install records nothing at + # all on the second call. expected = {signal.SIGINT, signal.SIGTERM} assert first == expected, f"first install did not attempt both signals: {sorted(first)}" assert second == expected, ( From fa33f6f8cea30f9dc87d3278cdb0ddfe05e66267 Mon Sep 17 00:00:00 2001 From: uipreliga Date: Thu, 27 Aug 2026 18:20:42 -0700 Subject: [PATCH 4/4] fix(utils): use explicit concatenation in the plugin-root warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01V5WZ1BbtygK9YVvcgPfNGh --- src/coder_eval/utils.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coder_eval/utils.py b/src/coder_eval/utils.py index dccd84ae..2ec6cac2 100644 --- a/src/coder_eval/utils.py +++ b/src/coder_eval/utils.py @@ -93,9 +93,9 @@ def process_plugins( if plugin.get("type") == "local" and resolved.is_dir() and not (resolved / "skills").is_dir(): log.warning( f"Plugin path has no skills/ subdirectory, so it loads no skills: {resolved}. " - "A local plugin path must be a PLUGIN ROOT holding skills/ " - "(for .claude/skills/my-skill/SKILL.md that is .claude, not .claude/skills). " - "See docs/agents/HARNESS_PARITY.md." + + "A local plugin path must be a PLUGIN ROOT holding skills/ " + + "(for .claude/skills/my-skill/SKILL.md that is .claude, not .claude/skills). " + + "See docs/agents/HARNESS_PARITY.md." ) processed.append(processed_plugin)