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/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/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 296092f6..af7f352c 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,60 @@ 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 — 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 +# 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`. + +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 `tasks/run_limits/` holds one fixture per limit: `max_turns_cap.yaml` asks for more 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/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 a54c083d..6d91bdd5 100644 --- a/plugins/coder-eval/reference/templates/activation.yaml +++ b/plugins/coder-eval/reference/templates/activation.yaml @@ -10,11 +10,24 @@ 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 +# 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 # # 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..4cfab74a 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,14 +164,38 @@ 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. + +**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/skills" +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 @@ -182,6 +206,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..3d1b2729 100644 --- a/plugins/coder-eval/skills/ci/SKILL.md +++ b/plugins/coder-eval/skills/ci/SKILL.md @@ -153,11 +153,30 @@ 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. + +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 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"] --- diff --git a/src/coder_eval/utils.py b/src/coder_eval/utils.py index 1624dae6..2ec6cac2 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 1b3d7b7e..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 @@ -3350,3 +3351,239 @@ 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, 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 + + # 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/", + ) + + # 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: set[Path] = set() + for pattern in self._GLOBS: + 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?""" + # 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._values(line): + scanned += 1 + if self._is_skills_dir(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, " + "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_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 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 + for plugin in agent.get("plugins") or []: + if not isinstance(plugin, dict) or plugin.get("type") != "local": + continue + value = str(plugin.get("path") or "") + # 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_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_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 6d8babc6..733889ee 100644 --- a/tests/test_reference_permissions.py +++ b/tests/test_reference_permissions.py @@ -357,19 +357,45 @@ 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() - registry.ensure_crash_handlers() - registry.ensure_crash_handlers() - assert len(attempts) == 4, "a failed install must be retried on the next call, not latched" + # 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, ( + "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):