From 50f3360d652afac0205c79057baf2b2606de7694 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Tue, 1 Sep 2026 10:09:52 -0400 Subject: [PATCH] docs: address Copilot review round 1 on #2203 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes an unquoted `#` in `board-ops`'s description. YAML reads ` #28` onward as a comment, so the parser — and Claude Code, which parses the same way — only ever saw the description up to `Covers board`. Both board numbers and the option-deletion hazard were absent from the listing that decides whether the skill fires, and every check stayed green because they look for an *empty* description and a truncated one is still non-empty. Quoting restores all 271 characters, which is the whole of the 3,144 vs 3,234 discrepancy the review flagged. `parseSkill` now compares the raw scalar against what YAML kept and errors on any loss, so the next one fails the gate rather than shipping. Tests cover the truncated case and a legitimately quoted `#`. Re-aims the `project-structure` Node-backend case, which `AGENTS.md` answers via its tree and which had oscillated 80 -> 100% across runs — the partial-overlap signature this PR documents. The replacement names no internals and probed 4/4. Makes the documented probe reproduce the harness: the same `--disallowedTools` flags as `runPrompt` rather than `--allowedTools`, `printf` instead of zsh's `print -r`, and `jq` over `tool_use` blocks instead of a `grep` that also matched MCP server names in the init event. The first-tool-call table is relabelled as an unrestricted probe, since the eval cannot reach for Bash at all. RUNS=5 skills:eval: 63/63, 62 at 100%; restoring those 90 characters displaced nothing. Signed-off-by: cliffhall --- .claude/skills/board-ops/SKILL.md | 2 +- .../skills/project-structure/evals/evals.json | 2 +- AGENTS.md | 11 ++++- docs/skill-authoring.md | 22 ++++++++-- scripts/lib/skill-manifest.mjs | 32 ++++++++++++++ scripts/lib/skill-manifest.test.mjs | 43 +++++++++++++++++++ 6 files changed, 104 insertions(+), 8 deletions(-) diff --git a/.claude/skills/board-ops/SKILL.md b/.claude/skills/board-ops/SKILL.md index 8344172e..0a8f5825 100644 --- a/.claude/skills/board-ops/SKILL.md +++ b/.claude/skills/board-ops/SKILL.md @@ -1,6 +1,6 @@ --- name: board-ops -description: gh recipes for the Inspector project boards — add a card, move its Status, set or re-score its Priority, delete a card, and recover from a deleted single-select option. Covers board #28 (v2) and board #11 (v1), their node/field/option IDs, and the option-deletion hazard. +description: "gh recipes for the Inspector project boards — add a card, move its Status, set or re-score its Priority, delete a card, and recover from a deleted single-select option. Covers board #28 (v2) and board #11 (v1), their node/field/option IDs, and the option-deletion hazard." disable-model-invocation: false --- diff --git a/.claude/skills/project-structure/evals/evals.json b/.claude/skills/project-structure/evals/evals.json index e8bfc42b..7c1520d5 100644 --- a/.claude/skills/project-structure/evals/evals.json +++ b/.claude/skills/project-structure/evals/evals.json @@ -4,7 +4,7 @@ "expect": "project-structure" }, { - "prompt": "Does a new Node-only backend piece for the web client belong in core or in the web client itself?", + "prompt": "I am adding a module that both the browser side and the Node side need to run. How do I decide where it goes in this repo?", "expect": "project-structure" }, { diff --git a/AGENTS.md b/AGENTS.md index 627d136e..a5af8932 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -154,7 +154,14 @@ that from happening: Code does and fails on anything that would strip the metadata — most importantly **malformed YAML**, which loads the body with an *empty* description, so `/skill-name` still works and a manual spot check passes while the skill can - never auto-fire again. An unquoted colon in a description is enough. It also + never auto-fire again. An unquoted colon in a description is enough — and so + is an unquoted **`#`**, which YAML reads as a comment and which truncates the + description *silently* from that point on rather than emptying it. `board-ops` + shipped that way: `Covers board #28 (v2) and board #11 (v1), their + node/field/option IDs, and the option-deletion hazard` was cut at `#28`, so 90 + characters — including both board numbers — were absent from the listing while + every check stayed green, because a truncated description is still a non-empty + one. **Quote any description containing `#` or `:`.** It also runs `claude plugin validate` — the authoritative schema — when the installed CLI is **exactly** the pinned version, and otherwise says so and moves on. That best-effort hand-off keeps `validate` fast and offline, but it also @@ -236,7 +243,7 @@ that from happening: overflows, and drops the least-invoked entries **first** — which are exactly the model-invoked skills that must fire on their own. `verify:skills` prints the current cost against the budget recorded in `scripts/lib/skill-manifest.mjs` - (3,144/4,000 characters as of this writing) and fails when it is exceeded. Raise + (3,234/4,000 characters as of this writing) and fails when it is exceeded. Raise the budget deliberately, or tighten a description; each entry is capped at 1,536 characters regardless, so **put the key use case first**. diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 34947428..6ba91097 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -76,6 +76,14 @@ Measured, one probe each, on this repo: | "Which parts of the gate only ever run locally and never in CI?" | `Read` | | "The gate failed at the lint stage. How do I work out what is wrong?" | `Skill` ✅ | +⚠️ Those rows come from an **unrestricted** probe, run before the snippet below +was aligned with the harness. The eval itself forbids `Bash` (and `Write`, +`Edit`, `NotebookEdit`), so a scored case cannot actually reach for one — what +the rows show is the *pull*: given a free choice, these prompts send the model +to read the repo rather than to load a skill, and under the harness's flags that +same pull simply produces no `Skill` call at all. Read them as a diagnosis of +why a prompt scores badly, never as a claim about which tools the eval offers. + Shapes that reliably fire: - **"How do I …"** — a procedure, not an explanation. @@ -152,10 +160,16 @@ of metered model calls; a single probe is one. Iterate with probes until a prompt fires at all, and only then spend a full run on its rate: ```sh -# One sample, printing which tool actually fired first. -print -r -- "" \ - | claude -p --output-format stream-json --verbose --max-turns 1 --allowedTools "Skill" \ - | grep -o '"name":"[^"]*"' | head -3 +# One sample, printing which tool actually fired first. The flags are the +# harness's own (see `runPrompt` in `scripts/skill-eval.mjs`), so a probe and a +# scored case see the same tool policy — a probe that may call a tool the eval +# forbids predicts nothing. `printf` rather than zsh's `print -r`, so the +# snippet also runs under bash. +printf '%s' "" \ + | claude -p --output-format stream-json --verbose --max-turns 1 \ + --disallowedTools Bash,Write,Edit,NotebookEdit \ + | jq -r 'select(.message.content?) | .message.content[]? + | select(.type == "tool_use") | .name' | head -3 ``` **Probe a marginal case more than once.** A prompt that fires on a single probe diff --git a/scripts/lib/skill-manifest.mjs b/scripts/lib/skill-manifest.mjs index e7f084ca..76c7b584 100644 --- a/scripts/lib/skill-manifest.mjs +++ b/scripts/lib/skill-manifest.mjs @@ -150,6 +150,38 @@ export function parseSkill(dirName, text) { errors.push("`paths` must be a list of glob strings"); } } + // An unquoted `#` is a YAML comment, so a description containing one is + // truncated at that point — silently, and *not* to empty, which is why the + // "malformed YAML" checks sail past it: what survives still parses as a + // non-empty string. `board-ops` shipped for a while with its two board numbers + // and the option-deletion hazard cut off this way. Compare the raw scalar + // against what YAML actually kept and make any loss an error. + for (const [field, parsed] of [ + ["description", description], + ["when_to_use", whenToUse], + ]) { + if (typeof parsed !== "string") continue; + const raw = new RegExp("^" + field + ":[ \\t]*(.*)$", "m").exec( + frontmatter, + )?.[1]; + if (raw === undefined) continue; + const trimmed = raw.trim(); + // Only an unquoted scalar can lose text to a comment; a quoted one is safe, + // and a block scalar (`|`/`>`) puts nothing on this line to compare. + if (/^["'|>]/.test(trimmed)) continue; + if (trimmed.length > parsed.trim().length) { + errors.push( + "`" + + field + + "` is truncated by an unquoted `#` — YAML kept " + + parsed.trim().length + + " of " + + trimmed.length + + " characters; quote the value", + ); + } + } + // A skill nothing can reach is dead weight that still costs a directory. if (!modelInvoked && meta["user-invocable"] === false) { errors.push( diff --git a/scripts/lib/skill-manifest.test.mjs b/scripts/lib/skill-manifest.test.mjs index a9bb3481..8136d174 100644 --- a/scripts/lib/skill-manifest.test.mjs +++ b/scripts/lib/skill-manifest.test.mjs @@ -356,3 +356,46 @@ test("an eval file may only expect its own skill", () => { [], ); }); + +test("an unquoted `#` truncates a description, and that is an error", () => { + // The dangerous part is that YAML leaves a *non-empty* string behind, so the + // "description is required and non-empty" check passes and the skill keeps + // auto-firing — just with its most distinctive words missing from the + // listing. `board-ops` shipped this way, losing both board numbers. + const withHash = [ + "---", + "name: board-ops", + "description: Recipes. Covers board #28 (v2) and board #11 (v1).", + "disable-model-invocation: false", + "---", + "body", + ].join("\n"); + const parsed = parseSkill("board-ops", withHash); + assert.equal(parsed.description, "Recipes. Covers board"); + assert.match(parsed.errors.join(" "), /truncated by an unquoted `#`/); + + // Quoting is the fix, and it must come back clean. + const quoted = withHash.replace( + /^description: (.*)$/m, + (_m, d) => `description: "${d}"`, + ); + const ok = parseSkill("board-ops", quoted); + assert.deepEqual(ok.errors, []); + assert.equal( + ok.description, + "Recipes. Covers board #28 (v2) and board #11 (v1).", + ); +}); + +test("a `#` inside a quoted description is not treated as truncation", () => { + // Guard against the check firing on every legitimately quoted description. + const quoted = [ + "---", + "name: alpha", + 'description: "counts # and : safely"', + "disable-model-invocation: false", + "---", + "body", + ].join("\n"); + assert.deepEqual(parseSkill("alpha", quoted).errors, []); +});