diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index c45515f0..39c3683e 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -16,9 +16,15 @@ the `Skill` tool fired with the expected name. Four properties of that harness drive everything below: -- **`--max-turns 1`.** The skill must be the model's **first** tool call. If it - opens a file first — even on its way to a perfect answer — the sample is a - miss. This is the single most important fact about writing cases. +- **`--max-turns 1`.** The skill must fire in the model's **first assistant + turn**. Ordering _within_ that turn does not matter — `collectSkillInvocations` + scans every `tool_use` block in the event, so a turn that calls `Read` and + `Skill` together still scores a hit. What loses the sample is needing a tool + *result* first: read a file, look at what came back, then decide — that + decision lands in turn two, which never runs. So a prompt that invites the + model to go look at something before answering is a miss even when its + eventual answer would have been perfect. This is the single most important + fact about writing cases. - **The session is fresh, but not empty.** `CLAUDE.md` → `AGENTS.md` is loaded in full on every turn, so anything the rules file already answers gets answered without a skill. diff --git a/scripts/lib/skill-manifest.mjs b/scripts/lib/skill-manifest.mjs index e4521a5a..fe8f6147 100644 --- a/scripts/lib/skill-manifest.mjs +++ b/scripts/lib/skill-manifest.mjs @@ -176,6 +176,13 @@ export function parseSkill(dirName, text) { // `typeof node.value !== "string"` also covers the non-scalar cases: a map // or list node has no `.value`, and its own checks above already report it. if (!node || typeof node.value !== "string") continue; + // Only on a PLAIN scalar is a comment evidence of loss. A quoted or block + // scalar can carry a perfectly legitimate trailing one — `description: + // "Complete value" # note` keeps its whole value — and flagging that told + // the author to quote a value they had already quoted (Copilot). The regex + // this replaced tested the same thing by reading the first character; the + // node states it outright. + if (node.type !== "PLAIN") continue; const dropped = node.comment; if (typeof dropped !== "string") continue; errors.push( diff --git a/scripts/lib/skill-manifest.test.mjs b/scripts/lib/skill-manifest.test.mjs index 3218ffcd..7a00aae8 100644 --- a/scripts/lib/skill-manifest.test.mjs +++ b/scripts/lib/skill-manifest.test.mjs @@ -602,6 +602,41 @@ test("the positive floor counts distinct prompts, not entries", () => { ); }); +test("a quoted or block scalar may carry a trailing comment", () => { + // `yaml` attaches a legitimate trailing comment to these nodes too, so keying + // on `.comment` alone rejected valid frontmatter and told the author to quote + // a value that was already quoted. Only a PLAIN scalar can lose text to one. + for (const value of ['"Complete value" # note', "'Complete value' # note"]) { + const parsed = parseSkill( + "alpha", + [ + "---", + "name: alpha", + `description: ${value}`, + "disable-model-invocation: false", + "---", + "body", + ].join("\n"), + ); + assert.deepEqual(parsed.errors, [], value); + assert.equal(parsed.description, "Complete value"); + } + + const block = parseSkill( + "alpha", + [ + "---", + "name: alpha", + "description: | # note", + " Complete value", + "disable-model-invocation: false", + "---", + "body", + ].join("\n"), + ); + assert.deepEqual(block.errors, []); +}); + test("the truncation guard covers noncanonical key forms", () => { // `"description":` and `description :` both populate the mapping, so a guard // that located the field by matching a `^description:` LINE skipped them