From 9d162f36c6a054b4958be92f5cd5ce5ef3ea4ea1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 00:09:56 -0400 Subject: [PATCH] fix: address Copilot review round 10 on #2251 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - skills.ts: validate `frontmatter.name` against the Agent Skills format (1–64 lowercase alphanumerics and hyphens, no leading, trailing or consecutive hyphen). Checking only for non-emptiness let `Bad Name` read as "Conforms" — and the name is not decorative: it must equal the URI path segment, so a name that cannot appear in a URI is a contradiction the entry cannot satisfy. Suppressed when the name is absent entirely, which `missing-name` already reports. - skills.ts: detect duplicates on the NORMALIZED identity. Containment and the read that fetches the bytes both treat normalized-equivalents as one resource, so `skill://demo/SKILL.md` and `skill://demo/x/../SKILL.md` were passing as two distinct files while naming one. The finding still reports the raw URI, and two different unparseable URIs stay distinct. - SkillsScreen: run `checkSkillConformance` on the fetched `skills/get` entry and treat an error — or an entry answering for a different URI — as invalid (red), not as "a different snapshot" (yellow). A fresh point-in-time read excuses a CHANGE; it does not excuse a violation, and answering with another skill is never a refresh of the one requested. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 33 ++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 79 +++++++++++++++---- clients/web/src/test/core/mcp/skills.test.ts | 77 ++++++++++++++++++ core/mcp/skills.ts | 37 ++++++++- 4 files changed, 208 insertions(+), 18 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 739ae968..b97608cc 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -501,6 +501,39 @@ describe("SkillsScreen", () => { expect(screen.getByTestId("skills-get-result")).toBeInTheDocument(); }); + it("calls a non-conforming skills/get entry invalid, not a new snapshot", async () => { + // A fresh snapshot excuses a CHANGE; it does not excuse a violation. An + // entry missing a digest is invalid whether or not the skill moved on. + const user = userEvent.setup(); + const onGetSkill = vi.fn().mockResolvedValue({ + ...CLEAN_SKILL, + resources: [{ uri: "skill://data-analysis/SKILL.md" }], + }); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click( + screen.getByRole("button", { name: /Fetch with skills\/get/ }), + ); + const result = await screen.findByTestId("skills-get-result"); + expect(result).toHaveAttribute("data-verdict", "invalid"); + expect(result).toHaveTextContent("missing-digest"); + }); + + it("calls a skills/get answer for a different uri invalid", async () => { + // Answering with another skill is never a valid refresh of the one asked + // for, however much that other skill may have changed. + const user = userEvent.setup(); + const onGetSkill = vi.fn().mockResolvedValue(TAMPERED_SKILL); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click( + screen.getByRole("button", { name: /Fetch with skills\/get/ }), + ); + const result = await screen.findByTestId("skills-get-result"); + expect(result).toHaveAttribute("data-verdict", "invalid"); + expect(result).toHaveTextContent("different URI"); + }); + it("reports a failed skills/get", async () => { const user = userEvent.setup(); const onGetSkill = vi.fn().mockRejectedValue(new Error("-32602")); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index bd7b09e7..2fd0fa08 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -97,7 +97,12 @@ interface FetchedEntryState { /** The click this result belongs to — see {@link PreviewState.attempt}. */ attempt?: number; entry?: SkillEntry; + /** Conformance findings for the FETCHED entry, in its own right. */ + issues?: SkillIssue[]; + /** True when the fetched entry describes the same skill as the listed one. */ matches?: boolean; + /** True when the fetched entry is for a different URI than was asked for. */ + wrongUri?: boolean; message?: string; } @@ -530,10 +535,21 @@ export function SkillsScreen({ ); void onGetSkill(selected.uri) .then((entry) => { - // Compared semantically against what `skills/list` advertised — see - // `skillEntriesMatch` for why a `JSON.stringify` comparison would - // report key order and manifest order as differences. - writeFetched({ entry, matches: skillEntriesMatch(entry, selected) }); + // The fetched entry is checked ON ITS OWN before being compared. A + // snapshot is allowed to have moved on, but it is not allowed to be + // non-conforming: an entry missing a digest is invalid whether or not + // the skill changed, and an entry for a DIFFERENT uri is never a valid + // refresh of the one that was asked for. Only a conforming entry with + // the same identity gets the benign "the snapshot moved" reading. + writeFetched({ + entry, + issues: checkSkillConformance(entry), + wrongUri: entry.uri !== selected.uri, + // Compared semantically — see `skillEntriesMatch` for why a + // `JSON.stringify` comparison would report key order and manifest + // order as differences. + matches: skillEntriesMatch(entry, selected), + }); }) .catch((err: unknown) => { writeFetched({ @@ -543,6 +559,16 @@ export function SkillsScreen({ }, [manifestKey, onGetSkill, selected]); const fetched = fetchedEntry.key === manifestKey ? fetchedEntry : undefined; + // `invalid` outranks the snapshot comparison: an entry that breaks a + // requirement, or answers for a different URI, is wrong regardless of + // whether the skill it describes has changed since the listing. + const fetchedVerdict = + fetched?.wrongUri || + (fetched?.issues ?? []).some((issue) => issue.severity === "error") + ? "invalid" + : fetched?.matches + ? "matches" + : "differs"; const batchRunning = batches.has(manifestKey); const preview = @@ -810,24 +836,45 @@ export function SkillsScreen({ {fetched?.entry !== undefined && ( - {fetched.matches - ? "The entry this server returns for this URI describes the same skill it listed (compared ignoring key and manifest order)." - : "The entry this server returns for this URI differs from the one it listed. `skills/get` is a fresh snapshot, so this is expected if the skill changed since the list was fetched — and a server inconsistency if it did not."} + {fetchedVerdict === "invalid" + ? fetched.wrongUri + ? "This entry is for a different URI than the one requested, which is never a valid refresh of it." + : "This entry breaks a requirement of its own, so the difference is not simply a newer snapshot." + : fetchedVerdict === "matches" + ? "The entry this server returns for this URI describes the same skill it listed (compared ignoring key and manifest order)." + : "The entry this server returns for this URI differs from the one it listed. `skills/get` is a fresh snapshot, so this is expected if the skill changed since the list was fetched — and a server inconsistency if it did not."} - {!fetched.matches && ( + {(fetched.issues ?? []) + .filter((issue) => issue.severity === "error") + .map((issue, index) => ( + + {issue.code}: {issue.message} + + ))} + {fetchedVerdict !== "matches" && ( { expect(issues[0].severity).toBe("error"); }); + it("reports a name that is not a valid Agent Skills name", () => { + // The name is not decorative: it must equal the URI path segment, so a + // name that cannot appear in a URI is a contradiction the entry cannot + // satisfy. Checking only for non-emptiness let these read as "Conforms". + for (const name of [ + "Bad Name", + "UPPER", + "-leading", + "trailing-", + "double--hyphen", + "under_score", + "a".repeat(65), + ]) { + const issues = checkSkillConformance( + entry({ frontmatter: { name, description: "d" } }), + ); + expect(issues.map((i) => i.code)).toContain("malformed-name"); + expect(issues.find((i) => i.code === "malformed-name")?.severity).toBe( + "error", + ); + } + }); + + it("accepts the names the Agent Skills format allows", () => { + for (const name of ["a", "demo", "data-analysis", "a1-b2-c3"]) { + const issues = checkSkillConformance( + entry({ + uri: `skill://${name}/SKILL.md`, + frontmatter: { name, description: "d" }, + resources: [ + { uri: `skill://${name}/SKILL.md`, digest: DIGEST, size: 1 }, + ], + }), + ); + expect(issues.map((i) => i.code)).not.toContain("malformed-name"); + } + }); + + it("does not report a malformed name when there is no name at all", () => { + // `missing-name` already says it; two findings would read as two defects. + const issues = checkSkillConformance( + entry({ frontmatter: { description: "d" } }), + ); + expect(issues.map((i) => i.code)).toEqual(["missing-name"]); + }); + it("reports a missing description as an error", () => { // SEP-2640 requires `description`, so an absent one is a format violation // and must not read as "0 errors" in the conformance summary. @@ -385,6 +431,37 @@ describe("checkSkillConformance", () => { expect(issues.map((i) => i.code)).toEqual(["resource-outside-skill-root"]); }); + it("detects a duplicate that differs only before normalization", () => { + // Containment and the read that fetches the bytes both treat these as one + // resource, so the uniqueness check must too — otherwise a manifest naming + // one file twice passes as two distinct files. + const issues = checkSkillConformance( + entry({ + resources: [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + { uri: "skill://demo/x/../SKILL.md", digest: DIGEST, size: 20 }, + ], + }), + ); + expect(issues.map((i) => i.code)).toEqual(["duplicate-resource"]); + // Reported against the raw URI, so the diagnostic points at what the + // server actually sent. + expect(issues[0].resourceUri).toBe("skill://demo/x/../SKILL.md"); + }); + + it("does not fold two different unparseable URIs into one duplicate", () => { + const issues = checkSkillConformance( + entry({ + resources: [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + { uri: "not a uri", digest: DIGEST, size: 1 }, + { uri: "also not a uri", digest: DIGEST, size: 1 }, + ], + }), + ); + expect(issues.map((i) => i.code)).not.toContain("duplicate-resource"); + }); + it("reports a manifest entry outside the skill root", () => { const issues = checkSkillConformance( entry({ diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index 926c683a..d7378d86 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -55,6 +55,19 @@ export const SKILL_FILE_SUFFIX = "/SKILL.md"; /** `sha256:` followed by exactly 64 lowercase hex characters. */ const DIGEST_PATTERN = /^sha256:[0-9a-f]{64}$/; +/** + * The Agent Skills name format SEP-2640 requires of `frontmatter.name`: 1–64 + * characters of lowercase alphanumerics and hyphens, with no leading, trailing + * or consecutive hyphen. + * + * Checking only that the name is non-empty let `Bad Name` reach the UI as + * "Conforms" — and the name is not decorative here: it must equal the URI path + * segment, so a name that cannot appear in a URI is a contradiction the entry + * cannot satisfy. + */ +const SKILL_NAME_PATTERN = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; +const SKILL_NAME_MAX_LENGTH = 64; + /** * What the server declared under `io.modelcontextprotocol/skills`. The only * sub-option SEP-2640 defines is `directoryRead`, which gates @@ -162,6 +175,7 @@ export function skillDisplayName(entry: SkillEntry): string { export type SkillIssueCode = | "dynamic-resources" | "missing-name" + | "malformed-name" | "missing-description" | "malformed-uri" | "name-path-mismatch" @@ -213,6 +227,15 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { severity: "error", message: "frontmatter.name is required but missing or empty.", }); + } else if ( + declaredName.length > SKILL_NAME_MAX_LENGTH || + !SKILL_NAME_PATTERN.test(declaredName) + ) { + issues.push({ + code: "malformed-name", + severity: "error", + message: `frontmatter.name "${declaredName}" is not a valid Agent Skills name: 1–${SKILL_NAME_MAX_LENGTH} lowercase alphanumerics and hyphens, with no leading, trailing or consecutive hyphen.`, + }); } if (!entry.frontmatter.description?.trim()) { // An error, not a warning: SEP-2640 requires `description` on every skill, @@ -301,7 +324,17 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { : undefined; for (const resource of entry.resources) { - if (seenUris.has(resource.uri)) { + // Compared on the NORMALIZED identity, because everything else here treats + // normalized-equivalents as the same resource — containment does, and so + // does the read that fetches the bytes. On the raw string, + // `skill://demo/SKILL.md` and `skill://demo/x/../SKILL.md` would pass as + // two distinct files while naming one. The raw URI is still what the + // finding reports, so the diagnostic points at what the server actually + // sent. Unparseable URIs fall back to the raw string: they are already + // reported by the root check, and normalizing them all to `undefined` + // would make two different bad URIs look like one duplicate. + const identity = normalizeSkillUri(resource.uri) ?? resource.uri; + if (seenUris.has(identity)) { issues.push({ code: "duplicate-resource", severity: "error", @@ -310,7 +343,7 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { resourceUri: resource.uri, }); } - seenUris.add(resource.uri); + seenUris.add(identity); if (root !== undefined) { const normalized = normalizeSkillUri(resource.uri); // An unparseable entry URI is outside the root by construction: nothing