modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

docs: address Copilot review round 8 on #2203

Browse source 
Ask the YAML parser what it discarded instead of re-deriving it from the raw
frontmatter text. `yaml` hangs the comment it stripped on the scalar node, so
`parseDocument(...).get(field, true).comment` IS the truncation signal: a plain
scalar that lost text has one, a quoted or block scalar never does, and
`Issue#28` (no preceding space, so no comment) keeps its full value.

That closes both findings at once. The line-locating regex only ever matched
the canonical `description:` key, so `"description":` and `description :`
populated the mapping while the guard skipped them and left verify:skills green
on a truncated listing; and the continuation scan stopped at the first blank
line, inspecting only the opening paragraph of a plain scalar that YAML happily
continues past one.

It also retires the scanning itself — three rounds of regex here each fixed one
shape and missed the next (tags and anchors, then indented continuations, now
key forms and blank lines). The node carries all of them for free, so 65 lines
of hand-rolled parsing come out. Parsing once with `parseDocument` keeps the
node view and the value view from ever disagreeing.

Tests cover the three noncanonical key forms, the blank-line continuation, and
the standalone comments that must NOT read as truncation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SyBPs7caoMcyvnp9ArMBhV
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-01 12:46:22 -04:00
commit c66793bfd8
2 changed files with 102 additions and 65 deletions
Download patch file Download diff file Expand all files Collapse all files

98
scripts/lib/skill-manifest.mjs
View file

@ -11,7 +11,7 @@
// So every check here is deny-by-default: a file we cannot read the way Claude // So every check here is deny-by-default: a file we cannot read the way Claude
// Code reads it is an error, not a skip. // Code reads it is an error, not a skip.
import { parse as parseYaml } from "yaml"; import { parseDocument } from "yaml";
/** /**
* Per-entry cap Claude Code applies to a skill listing entry. It covers * Per-entry cap Claude Code applies to a skill listing entry. It covers
@ -84,9 +84,15 @@ export function parseSkill(dirName, text) {
const { frontmatter, error } = splitFrontmatter(text); const { frontmatter, error } = splitFrontmatter(text);
if (error) return { errors: [error] }; if (error) return { errors: [error] };
let doc;
let meta; let meta;
try { try {
meta = parseYaml(frontmatter); // `parseDocument` rather than `parse`: the truncation guard below needs the
// scalar NODES, not just their values, and parsing once keeps the two views
// of the frontmatter from ever disagreeing.
doc = parseDocument(frontmatter);
if (doc.errors.length > 0) throw new Error(doc.errors[0].message);
meta = doc.toJS();
} catch (e) { } catch (e) {
// The whole point of the guard: this is what strips the description. // The whole point of the guard: this is what strips the description.
return { return {
@ -154,71 +160,33 @@ export function parseSkill(dirName, text) {
// truncated at that point — silently, and *not* to empty, which is why the // 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 // "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 // 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 // and the option-deletion hazard cut off this way.
// against what YAML actually kept and make any loss an error.
// //
// The comparison has to span the WHOLE plain scalar, not its first physical // Ask the PARSER what it discarded rather than re-deriving it from the raw
// line. A plain scalar continues onto more-indented lines, so // text. `yaml` hangs the comment it stripped on the scalar node itself, so a
// // plain scalar with a `.comment` is exactly the truncation case, and a quoted
// description: Covers boards // or block scalar never has one. Three rounds of regex here each fixed one
// #28 and #11 // shape and missed the next — a tag or anchor before the value, a scalar
// // continued onto indented lines, then a continuation across a blank line, and
// parses to `Covers boards` — and a first-line-only check compares that // key forms (`"description":`, `description :`) that populate the mapping
// against `Covers boards` and sees no loss, passing the exact truncation this // while matching no `^description:` line at all (Copilot). The node carries
// guard exists to reject (Copilot). // all of them for free, so none of that scanning survives.
const lines = frontmatter.split("\n"); for (const field of ["description", "when_to_use"]) {
for (const [field, parsed] of [ const node = doc.get(field, true);
["description", description], // `typeof node.value !== "string"` also covers the non-scalar cases: a map
["when_to_use", whenToUse], // or list node has no `.value`, and its own checks above already report it.
]) { if (!node || typeof node.value !== "string") continue;
if (typeof parsed !== "string") continue; const dropped = node.comment;
const start = lines.findIndex((l) => if (typeof dropped !== "string") continue;
new RegExp("^" + field + ":([ \\t]|$)").test(l), errors.push(
"`" +
field +
"` is truncated by an unquoted `#` — YAML kept `" +
node.value +
"` and dropped `#" +
dropped +
"`; quote the value",
); );
if (start === -1) continue;
const head = lines[start].slice(field.length + 1).trim();
// A scalar may carry YAML properties — a tag, an anchor, or both, in either
// order — before its value. Testing the first character alone read
// `&copy "Counts # safely"` as unquoted and then rejected the `#` that the
// quotes make harmless, so strip the properties before deciding (Copilot).
const value = head.replace(
/^(?:(?:!(?:!?[\w:.-]*|<[^>]*>)|&[^\s]+)\s+)+/,
"",
);
// Only an unquoted scalar can lose text to a comment; a quoted one is safe,
// and a block scalar (`|`/`>`) is read literally rather than comment-stripped.
if (/^["'|>]/.test(value)) continue;
// Continuation lines are indented and are not the next `key:` of the map.
const parts = [value];
for (let i = start + 1; i < lines.length; i++) {
const l = lines[i];
if (!/^[ \t]/.test(l) || l.trim() === "") break;
parts.push(l.trim());
}
// YAML folds a plain scalar's newlines to single spaces, so compare on
// whitespace-collapsed text rather than on the raw characters.
const collapse = (s) => s.replace(/\s+/g, " ").trim();
const rawText = collapse(parts.join(" "));
const keptText = collapse(parsed);
// A raw scalar can legitimately be longer than the parsed value without any
// text being lost: YAML strips presentation syntax such as an anchor
// (`&name`) or a tag (`!!str`) from the value. Comparing lengths alone
// reported those as truncation, with a message blaming a `#` that is not
// there. Only look at the length when the scalar actually contains a comment
// marker — a `#` at the start or preceded by whitespace, which is precisely
// where YAML begins a comment (Copilot).
if (!/(^|\s)#/.test(rawText)) continue;
if (rawText.length > keptText.length) {
errors.push(
"`" +
field +
"` is truncated by an unquoted `#` — YAML kept " +
keptText.length +
" of " +
rawText.length +
" characters; quote the value",
);
}
} }
// A skill nothing can reach is dead weight that still costs a directory. // A skill nothing can reach is dead weight that still costs a directory.

69
scripts/lib/skill-manifest.test.mjs
View file

@ -601,3 +601,72 @@ test("the positive floor counts distinct prompts, not entries", () => {
[], [],
); );
}); });
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
// entirely and left `verify:skills` green on a truncated listing (Copilot).
const mk = (key) =>
[
"---",
"name: alpha",
`${key} Covers board #28 and the hazard.`,
"disable-model-invocation: false",
"---",
"body",
].join("\n");
for (const key of ['"description":', "'description':", "description :"]) {
const parsed = parseSkill("alpha", mk(key));
assert.equal(parsed.description, "Covers board", `key form: ${key}`);
assert.match(
parsed.errors.join(" "),
/`description` is truncated/,
`key form: ${key}`,
);
}
});
test("the truncation guard spans a blank line inside a plain scalar", () => {
// A plain scalar continues across empty lines too, so a scan that stopped at
// the first blank one inspected only the opening paragraph and reported no
// loss (Copilot).
const parsed = parseSkill(
"alpha",
[
"---",
"name: alpha",
"description: Covers boards",
"",
" #28 and #11 and the hazard.",
"disable-model-invocation: false",
"---",
"body",
].join("\n"),
);
assert.equal(parsed.description, "Covers boards");
assert.match(parsed.errors.join(" "), /`description` is truncated/);
});
test("a standalone comment line is not read as truncation", () => {
// The guard keys on the comment the parser hung on the SCALAR. A comment on
// its own line belongs to the document, not to the description, and flagging
// it would fail a frontmatter that loses nothing.
for (const frontmatter of [
["# leading", "name: alpha", "description: Covers boards"],
["name: alpha", "description: Covers boards", "# standalone"],
["name: alpha", "description: Issue#28 needs no quoting"],
]) {
const parsed = parseSkill(
"alpha",
[
"---",
...frontmatter,
"disable-model-invocation: false",
"---",
"body",
].join("\n"),
);
assert.deepEqual(parsed.errors, [], frontmatter.join(" | "));
}
});