From 033600e4cafa2a1c312f5f8ac2cbcf08a226e931 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 19:24:30 -0400 Subject: [PATCH] fix: address Copilot review round 2 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings, two of them silent holes in the verification itself. **A blob-served SKILL.md skipped the frontmatter check entirely.** The entry's own file was captured as `contents.text`, so a server returning the markdown as a base64 `blob` — a legal `resources/read` shape this module already decodes for the digest — never reached the comparison, and the report still said `ok`. A MUST that quietly did not run. The file is now held as bytes and the text derived from them, which also guarantees the digest and the frontmatter describe one snapshot. **A dynamic skill whose SKILL.md could not be read reported `ok: true`.** It has no manifest rows, so `files` stayed empty, and its only static finding is a warning — a verification that could not be performed was reported as one that passed. The fallback read now records a `read-error` for all three ways it can fail. It is gated on `manifestListsSelf` (by normalized identity) so a self-entry the manifest listed and failed to read is not read, or reported, twice. **A served `.nan` compared equal to a listed `null`.** YAML expresses non-finite numbers, JSON does not, and `JSON.stringify` turns all of them into `null` — so the canonical comparison reported a real mismatch as agreement. Verified against the parser before fixing. They now canonicalize to a form no JSON scalar can equal, and the value is named in the finding. Also: the self-entry match is by normalized identity rather than raw string, which stops a second read and stops this function disagreeing with `checkSkillConformance`; and `SkillsTab.tsx` gains the file header AGENTS.md requires. The four `err instanceof Error` ternaries are one `reasonOf` helper now — extracted because the coverage gate found every one of their non-Error arms uncovered, and one honestly-tested branch beats four ignores. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- clients/tui/src/components/SkillsTab.tsx | 20 +++ clients/web/src/test/core/mcp/skills.test.ts | 37 +++++ .../test/core/mcp/skillsVerification.test.ts | 144 ++++++++++++++++++ core/mcp/skills.ts | 24 +++ core/mcp/skillsVerification.ts | 76 +++++++-- 5 files changed, 288 insertions(+), 13 deletions(-) diff --git a/clients/tui/src/components/SkillsTab.tsx b/clients/tui/src/components/SkillsTab.tsx index c583cd1e..65857ba9 100644 --- a/clients/tui/src/components/SkillsTab.tsx +++ b/clients/tui/src/components/SkillsTab.tsx @@ -1,3 +1,23 @@ +/** + * The TUI's Skills pane — the SEP-2640 catalog in a terminal (#2248). + * + * **Why the TUI owns a pane rather than reusing the web screen's logic.** It + * does reuse everything that decides an answer: `checkSkillConformance`, + * `checkSkillNameCollisions` and `verifySkills` all live in `core/` and are + * driven identically here, so a verdict cannot differ depending on which client + * you asked. What is local is presentation, and the terminal's constraints are + * genuinely different — two panes in 80 columns, no colour to rely on, and a + * keyboard rather than a pointer. + * + * ⚠️ **Severity is carried by a glyph as well as a colour** (`✓` / `!` / `✗`). + * This pane is read over ssh, inside tmux, and piped through `script(1)`, where + * colour may not survive; a row whose only signal was `red` would then be + * indistinguishable from a clean one. + * + * The pane is shown only when the connected server declares the extension — + * that gate, and the reset that leaves the tab when it goes false, live in + * `App.tsx` because they are navigation concerns rather than this pane's. + */ import React, { useCallback, useEffect, useRef, useState } from "react"; import { Box, Text, useInput, type Key } from "ink"; import { ScrollView, type ScrollViewRef } from "ink-scroll-view"; diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index 6b2aa069..551f86b3 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -975,6 +975,43 @@ describe("checkSkillFrontmatterMatch (#2248)", () => { ).toHaveLength(1); }); + it("does not let a YAML non-finite number match a listing's null", () => { + // `.nan` / `.inf` are YAML values JSON cannot express, and + // `JSON.stringify` turns every one of them into `null` — so a naive + // canonical comparison reported a served `.nan` as EQUAL to a listed + // `null`: a mismatch silently presented as agreement (Copilot). + const issues = checkSkillFrontmatterMatch( + entry({ threshold: null }), + file("threshold: .nan"), + ); + expect(issues).toHaveLength(1); + expect(issues[0].code).toBe("frontmatter-mismatch"); + // The value is named in the finding rather than hidden behind `null`. + expect(issues[0].message).toContain("NaN"); + }); + + it("distinguishes the three non-finite values from one another", () => { + expect( + checkSkillFrontmatterMatch(entry({ x: null }), file("x: .inf")), + ).toHaveLength(1); + // Infinity vs -Infinity: both stringify to `null`, so they would have + // compared equal to each other as well. + const both = checkSkillFrontmatterMatch( + entry({ a: 1, b: 2 }), + file("a: .inf\nb: -.inf"), + ); + expect(both).toHaveLength(2); + expect(both[0].message).toContain("Infinity"); + expect(both[1].message).toContain("-Infinity"); + }); + + it("still matches a null the served file also writes as null", () => { + // The fix must not turn a genuine agreement into a finding. + expect( + checkSkillFrontmatterMatch(entry({ x: null }), file("x: null")), + ).toEqual([]); + }); + it("reports nothing for two empty frontmatters", () => { expect(checkSkillFrontmatterMatch(entry({}), file(""))).toEqual([]); }); diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index 96054f87..db6d7aec 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -282,6 +282,150 @@ describe("verifySkills (#2248)", () => { reason: "expired", } as never); + it("runs the frontmatter check when the SKILL.md arrives as a blob", async () => { + // A base64 `blob` is a legal `resources/read` shape, and this module + // already decodes it for the digest. Reading `contents.text` skipped the + // MANDATORY frontmatter comparison for such a server while still reporting + // `ok` (Copilot). + const skillMd = "---\nname: demo\ndescription: Served\n---\n\n# D\n"; + const bytes = new TextEncoder().encode(skillMd); + const skill: SkillEntry = { + uri: "skill://demo/SKILL.md", + frontmatter: { name: "demo", description: "Listed" }, + resources: [ + { + uri: "skill://demo/SKILL.md", + digest: await sha256Digest(bytes), + size: bytes.byteLength, + }, + ], + }; + const readResource = vi.fn(async () => ({ + result: { + contents: [ + { + uri: "skill://demo/SKILL.md", + blob: Buffer.from(skillMd, "utf8").toString("base64"), + }, + ], + }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(report.files[0].status).toBe("verified"); + expect(report.frontmatter).toHaveLength(1); + expect(report.ok).toBe(false); + }); + + it("does not re-read a self-entry written in an equivalent URI form", async () => { + // `checkSkillConformance` accepts a normalized-equivalent self-entry, so a + // raw string comparison here would disagree with it and read the file twice. + const skillMd = "---\nname: demo\ndescription: A demo\n---\n\n# D\n"; + const bytes = new TextEncoder().encode(skillMd); + const skill: SkillEntry = { + uri: "skill://demo/SKILL.md", + frontmatter: { name: "demo", description: "A demo" }, + resources: [ + { + uri: "skill://demo/x/../SKILL.md", + digest: await sha256Digest(bytes), + size: bytes.byteLength, + }, + ], + }; + const readResource = vi.fn(async (uri: string) => ({ + result: { contents: [{ uri, text: skillMd }] }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(readResource).toHaveBeenCalledTimes(1); + expect(report.frontmatter).toEqual([]); + expect(report.ok).toBe(true); + }); + + it("fails a dynamic skill whose SKILL.md cannot be read", async () => { + // A dynamic skill has no manifest rows, so `files` stayed empty and its + // only static finding is a warning — an unreadable SKILL.md therefore + // reported `ok: true` for a skill whose mandatory frontmatter check never + // ran (Copilot). + const skill: SkillEntry = { + uri: "skill://gen/SKILL.md", + frontmatter: { name: "gen", description: "Generated" }, + resources: "dynamic", + }; + const readResource = vi.fn(async () => { + throw new Error("gone"); + }); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(report.files).toEqual([ + expect.objectContaining({ + uri: "skill://gen/SKILL.md", + status: "read-error", + reason: "gone", + }), + ]); + expect(report.ok).toBe(false); + }); + + it("fails a dynamic skill whose SKILL.md answers with no matching block", async () => { + const skill: SkillEntry = { + uri: "skill://gen/SKILL.md", + frontmatter: { name: "gen", description: "Generated" }, + resources: "dynamic", + }; + const readResource = vi.fn(async () => ({ + result: { contents: [{ uri: "skill://gen/other.md", text: "x" }] }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(report.files[0].status).toBe("read-error"); + expect(report.ok).toBe(false); + }); + + it("does not read a failed manifest self-entry a second time", async () => { + // Its failure is already recorded by the manifest loop; the fallback exists + // for a skill whose manifest never listed the file at all. + const skill = await entry(); + const readResource = vi.fn(async () => { + throw new Error("boom"); + }); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + // Two manifest entries, two reads — no third. + expect(readResource).toHaveBeenCalledTimes(2); + expect(report.files).toHaveLength(2); + }); + + it("stringifies a non-Error rejection rather than reading .message off it", async () => { + // A `throw "string"` anywhere in a transport reaches here; reading + // `.message` off one would put `undefined` where the diagnosis belongs. + const skill = await entry(); + const readResource = vi.fn(() => { + // A non-Error rejection is the point of the test. + throw "plainstring"; + }); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(report.files[0]).toMatchObject({ + status: "read-error", + reason: "plainstring", + }); + }); + + it("treats a result whose contents is not an array as no content", async () => { + // A server can return anything; `contents: "nope"` is not a block list, and + // hashing nothing against a digest would be a confident wrong answer. + const skill = await entry(); + const readResource = vi.fn(async () => ({ + result: { contents: "nope" }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(report.files.every((f) => f.status === "read-error")).toBe(true); + expect(report.ok).toBe(false); + }); + it("re-throws an auth-recovery error instead of recording it per file", async () => { // Not a property of the file in flight: the session's authorization // expired, so every remaining read fails the same way. Absorbing it would diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index 525ecd8a..be8d2748 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -561,6 +561,16 @@ function canonicalEntry(entry: SkillEntry): string { /** Object keys sorted recursively; array ORDER is preserved throughout. */ function canonicalize(value: unknown): unknown { + // YAML can express `.nan` and `.inf`; JSON cannot. `JSON.stringify` turns + // every one of them into `null`, so without this a served `x: .nan` would + // compare EQUAL to a listing declaring `x: null` — a mismatch silently + // reported as agreement (Copilot). The listing side arrived over JSON-RPC and + // can never hold a non-finite number, so one appearing here is always a real + // difference. Rendered as an object, which cannot equal any JSON scalar, and + // which names the value in the finding rather than hiding it. + if (typeof value === "number" && !Number.isFinite(value)) { + return { "#non-finite": String(value) }; + } if (Array.isArray(value)) return value.map(canonicalize); if (value === null || typeof value !== "object") return value; return sortKeys(value as Record); @@ -639,6 +649,20 @@ export function textToBytes(text: string): Uint8Array { return new TextEncoder().encode(text); } +/** + * A skill file's bytes as UTF-8 text — the inverse of {@link textToBytes}. + * + * Deliberately **non-fatal**: a `SKILL.md` that is not valid UTF-8 decodes with + * replacement characters rather than throwing. That is the more useful failure, + * because the frontmatter comparison then reports a concrete difference between + * what the listing claimed and what the file actually holds, instead of + * collapsing into "could not decode" and skipping the check the SEP makes + * mandatory. + */ +export function bytesToText(bytes: Uint8Array): string { + return new TextDecoder().decode(bytes); +} + /** * Raw bytes of a `resources/read` blob content block (standard base64). * Uses `atob`, which Node ≥22 and every browser provide, so this stays diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index a7ee2aac..84e0fdce 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -28,6 +28,7 @@ import { AuthRecoveryRequiredError } from "../auth/challenge.js"; import type { InspectorClientProtocol } from "./inspectorClientProtocol.js"; import type { RequestMetadata } from "./types.js"; import { + bytesToText, checkSkillConformance, checkSkillFrontmatterMatch, checkSkillNameCollisions, @@ -79,6 +80,18 @@ export interface SkillVerifyReport { ok: boolean; } +/** + * The reason string for a failed read. + * + * One helper rather than the same ternary at each of the four call sites: a + * rejection is not required to be an `Error` — a `throw "string"` anywhere in a + * transport or its dependencies reaches here — and reading `.message` off one + * would put `undefined` where the diagnosis belongs. + */ +function reasonOf(err: unknown): string { + return err instanceof Error ? err.message : String(err); +} + /** Result shape of one `resources/read`, narrowed to what a digest needs. */ interface ReadContents { text?: string; @@ -154,11 +167,27 @@ export async function verifySkills( // for the frontmatter cross-check. Reading it twice would double the load // on the server and, worse, could compare a digest against one snapshot // and frontmatter against another. - let entryText: string | undefined; + // + // Held as BYTES, not text. Taking `contents.text` skipped the whole + // frontmatter comparison whenever a server returned the markdown as a + // base64 `blob` — which is a legal `resources/read` shape, and which this + // module already decodes for the digest — so a mandatory check silently did + // not run while the report still said `ok` (Copilot). Deriving the text from + // the same verified bytes also guarantees the digest and the frontmatter + // describe one snapshot. + let entryBytes: Uint8Array | undefined; const files: SkillFileReport[] = []; const manifest = entry.resources === DYNAMIC_RESOURCES ? [] : entry.resources; + const entryIdentity = skillUriIdentity(entry.uri); + // Compared by NORMALIZED identity, like every other URI comparison here — + // `checkSkillConformance` already accepts a manifest self-entry written in + // an equivalent form, so a raw string test would disagree with it and read + // the same file a second time. + const manifestListsSelf = manifest.some( + (resource) => skillUriIdentity(resource.uri) === entryIdentity, + ); for (const resource of manifest) { let contents: ReadContents | undefined; try { @@ -169,7 +198,7 @@ export async function verifySkills( files.push({ uri: resource.uri, status: "read-error", - reason: err instanceof Error ? err.message : String(err), + reason: reasonOf(err), }); continue; } @@ -182,9 +211,6 @@ export async function verifySkills( }); continue; } - if (resource.uri === entry.uri && typeof contents.text === "string") { - entryText = contents.text; - } let bytes: Uint8Array; try { bytes = skillFileBytes(contents); @@ -192,10 +218,11 @@ export async function verifySkills( files.push({ uri: resource.uri, status: "read-error", - reason: err instanceof Error ? err.message : String(err), + reason: reasonOf(err), }); continue; } + if (skillUriIdentity(resource.uri) === entryIdentity) entryBytes = bytes; const verification = await verifySkillResource(resource, bytes); files.push({ uri: resource.uri, ...verification }); } @@ -203,21 +230,44 @@ export async function verifySkills( // A `"dynamic"` skill has no manifest, so the loop above read nothing — // but its SKILL.md is still served and still has to match the frontmatter // the listing advertised. That obligation is not waived by the file set - // being unenumerable; only integrity is. - if (entryText === undefined) { + // being unenumerable; only integrity is. The same applies to a skill whose + // manifest omits its own file. + // + // Gated on `manifestListsSelf` rather than on `entryBytes`, so a self-entry + // the loop already tried and FAILED to read is not read a second time — its + // failure is recorded there. + if (!manifestListsSelf) { + // Recorded as a file result, not swallowed. Because a dynamic skill has + // no manifest rows, `files` would otherwise stay empty and its only static + // finding is a warning — so an unreadable SKILL.md returned `ok: true` + // for a skill whose mandatory frontmatter check never ran (Copilot). + const fail = (reason: string) => + files.push({ uri: entry.uri, status: "read-error", reason }); try { const invocation = await client.readResource(entry.uri, metadata); const contents = contentsFor(invocation.result, entry.uri); - if (typeof contents?.text === "string") entryText = contents.text; + if (!contents) { + fail( + "resources/read returned no content block for this skill's own SKILL.md, so its frontmatter cannot be checked against the listing.", + ); + } else { + try { + entryBytes = skillFileBytes(contents); + } catch (err) { + fail(reasonOf(err)); + } + } } catch (err) { - // Left undefined: the frontmatter check is skipped below. When the - // manifest listed this file the failure is already reported there, and - // when it did not, `manifest-missing-self` is the finding that matters. - // An expired authorization is not that case — see the note above. + // An expired authorization is the one error that is not this file's + // problem — see the note on the function. if (err instanceof AuthRecoveryRequiredError) throw err; + fail(reasonOf(err)); } } + const entryText = + entryBytes === undefined ? undefined : bytesToText(entryBytes); + const collision = collisions.get(skillUriIdentity(entry.uri)); const conformance = [ ...checkSkillConformance(entry),