docs: address Copilot review round 1 on #2203
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 <cliff@futurescale.com>
This commit is contained in:
parent
906e970897
commit
50f3360d65
6 changed files with 104 additions and 8 deletions
|
|
@ -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
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
},
|
||||
{
|
||||
|
|
|
|||
11
AGENTS.md
11
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**.
|
||||
|
||||
|
|
|
|||
|
|
@ -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 -- "<prompt>" \
|
||||
| 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' "<prompt>" \
|
||||
| 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
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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, []);
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue