From bd28a36f237757843804923ea3a0e027324fc7b0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 00:46:18 -0400 Subject: [PATCH] fix: address Copilot review round 12 on #2251 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three findings were the same underlying inconsistency: URI comparisons on raw strings, in a flow that elsewhere deliberately treats normalized equivalents as the same resource. One `skillUriIdentity` helper now backs every comparison, so they cannot disagree. - skills.ts: `manifest-missing-self` compares identities, so a manifest listing the RFC-equivalent `skill://demo/%53KILL.md` is recognized as the entry's own file — it is fetchable as that file, so reporting it missing was the tool disagreeing with itself. - skills.ts: `skillEntriesMatch` normalizes the entry URI and every manifest URI before comparing, so a server that canonicalizes an escape between the listing and the fetch is not reported as a changed snapshot. - SkillsScreen: the `wrongUri` check uses the same identity, so a canonicalizing server is not accused of answering for a different skill — the read path already accepts exactly that equivalence. - skills.ts: apply the Agent Skills name grammar to the RAW value. Trimming first let `" demo "` pass, and whitespace is not in the grammar — so an entry could report "Conforms" with a name that can never equal its URI path segment. The trimmed copy now only distinguishes absent from invalid. - skills.ts: add `malformed-description` for a description above the 1024-character Agent Skills limit, with boundary coverage. - inspectorClient-skills.test: justify the `as unknown as` per AGENTS.md — both fields are `private` with no public setter, the asserted shape is exactly what the class declares, and the alternative is a live connection. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall --- .../screens/SkillsScreen/SkillsScreen.tsx | 8 ++- .../core/mcp/inspectorClient-skills.test.ts | 15 ++++ clients/web/src/test/core/mcp/skills.test.ts | 69 +++++++++++++++++++ core/mcp/skills.ts | 62 +++++++++++++++-- 4 files changed, 146 insertions(+), 8 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 2fd0fa08..b882942b 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -24,6 +24,7 @@ import { checkSkillConformance, skillDisplayName, skillEntriesMatch, + skillUriIdentity, totalSkillBytes, verifySkillResource, type SkillIssue, @@ -544,7 +545,12 @@ export function SkillsScreen({ writeFetched({ entry, issues: checkSkillConformance(entry), - wrongUri: entry.uri !== selected.uri, + // Compared by identity, not raw string: a server that canonicalizes + // an escape has answered for the same resource, and calling that + // "a different URI" would be the tool disagreeing with the read + // path, which accepts exactly that equivalence. + wrongUri: + skillUriIdentity(entry.uri) !== skillUriIdentity(selected.uri), // Compared semantically — see `skillEntriesMatch` for why a // `JSON.stringify` comparison would report key order and manifest // order as differences. diff --git a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts index 1ad5250c..a68b2379 100644 --- a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts @@ -45,6 +45,21 @@ describe("InspectorClient skills methods (#2234)", () => { ); } + /** + * A structural view onto two private fields, so the tests can stub the SDK + * client and set `capabilities` without connecting. + * + * The double cast is justified rather than incidental: `InspectorClient` + * declares both members `private`, so no single `as` relates it to a type + * that exposes them, and there is no public setter for either — the public + * path is `connect()`, which needs a transport, a live server and a + * handshake to reach the same state. It is safe because the shape asserted + * here is exactly the shape the class declares (`client` is the SDK client; + * `capabilities` is `ServerCapabilities | undefined`), so a rename or a type + * change on either field breaks these tests at the first use rather than + * silently passing. The same seam is used by + * `inspectorClient-raw-wire.test.ts`. + */ function internals(client: InspectorClient): SkillsInternals { return client as unknown as SkillsInternals; } diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index 8686a171..6bea4241 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -11,6 +11,7 @@ import { isSkillsExtensionSupported, normalizeSkillUri, skillEntriesMatch, + skillUriIdentity, sha256Digest, skillDisplayName, skillNameFromUri, @@ -191,6 +192,21 @@ describe("normalizeSkillUri", () => { }); }); +describe("skillUriIdentity", () => { + it("is the normalized form when the URI parses", () => { + expect(skillUriIdentity("skill://demo/a/../SKILL.md")).toBe( + "skill://demo/SKILL.md", + ); + }); + + it("falls back to the raw string, keeping two bad URIs distinct", () => { + expect(skillUriIdentity("not a uri")).toBe("not a uri"); + expect(skillUriIdentity("not a uri")).not.toBe( + skillUriIdentity("also not a uri"), + ); + }); +}); + describe("skillEntriesMatch", () => { const base = (): SkillEntry => ({ uri: "skill://demo/SKILL.md", @@ -237,6 +253,20 @@ describe("skillEntriesMatch", () => { ).toBe(true); }); + it("treats RFC-equivalent URI spellings as the same entry", () => { + // A server that canonicalizes an escape between the listing and the fetch + // has not changed the skill, so it must not read as a new snapshot. + const encoded: SkillEntry = { + ...base(), + uri: "skill://demo/%53KILL.md", + resources: [ + { uri: "skill://demo/%53KILL.md", digest: DIGEST, size: 20 }, + { uri: "skill://demo/x/../ref.md", digest: DIGEST, size: 10 }, + ], + }; + expect(skillEntriesMatch(base(), encoded)).toBe(true); + }); + it("still sees a real difference", () => { expect( skillEntriesMatch(base(), { @@ -307,6 +337,16 @@ describe("checkSkillConformance", () => { } }); + it("applies the name grammar to the RAW value, not a trimmed copy", () => { + // Trimming first would let `" demo "` through — and whitespace is not in + // the grammar, so the entry would report "Conforms" with a name that can + // never equal its URI path segment. + const issues = checkSkillConformance( + entry({ frontmatter: { name: " demo ", description: "d" } }), + ); + expect(issues.map((i) => i.code)).toContain("malformed-name"); + }); + it("accepts the names the Agent Skills format allows", () => { for (const name of ["a", "demo", "data-analysis", "a1-b2-c3"]) { const issues = checkSkillConformance( @@ -330,6 +370,21 @@ describe("checkSkillConformance", () => { expect(issues.map((i) => i.code)).toEqual(["missing-name"]); }); + it("reports a description above the 1024-character limit", () => { + const issues = checkSkillConformance( + entry({ frontmatter: { name: "demo", description: "d".repeat(1025) } }), + ); + expect(issues.map((i) => i.code)).toEqual(["malformed-description"]); + expect(issues[0].severity).toBe("error"); + }); + + it("accepts a description exactly at the limit", () => { + const issues = checkSkillConformance( + entry({ frontmatter: { name: "demo", description: "d".repeat(1024) } }), + ); + expect(issues).toEqual([]); + }); + 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. @@ -388,6 +443,20 @@ describe("checkSkillConformance", () => { expect(issues[0].resourceUri).toBe("skill://demo/ref.md"); }); + it("accepts a self-entry listed in an RFC-equivalent spelling", () => { + // The manifest names the same file the entry does, and it is fetchable as + // that file — reporting it missing would be the tool disagreeing with + // itself about which URIs are the same resource. + const issues = checkSkillConformance( + entry({ + resources: [ + { uri: "skill://demo/%53KILL.md", digest: DIGEST, size: 20 }, + ], + }), + ); + expect(issues.map((i) => i.code)).not.toContain("manifest-missing-self"); + }); + it("reports a manifest that omits the skill's own SKILL.md", () => { // A manifest is the complete file set, so one without the entry file is // not "a skill with no extras" — it cannot be checked against the skill. diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index e13de763..02e3ab6c 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -68,6 +68,9 @@ const DIGEST_PATTERN = /^sha256:[0-9a-f]{64}$/; const SKILL_NAME_PATTERN = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; const SKILL_NAME_MAX_LENGTH = 64; +/** The Agent Skills limit on `frontmatter.description`. */ +const SKILL_DESCRIPTION_MAX_LENGTH = 1024; + /** * What the server declared under `io.modelcontextprotocol/skills`. The only * sub-option SEP-2640 defines is `directoryRead`, which gates @@ -180,6 +183,19 @@ export function skillNameFromUri(uri: string): string | undefined { return segment.length > 0 ? segment : undefined; } +/** + * The comparison identity of a resource URI: its normalized form, falling back + * to the raw string when it does not parse. + * + * Every URI comparison in this module and in the screen goes through this, so + * they cannot disagree about whether two spellings name the same file. The raw + * fallback is deliberate: two *different* unparseable URIs must stay distinct + * rather than both collapsing to one `undefined` identity. + */ +export function skillUriIdentity(uri: string): string { + return normalizeSkillUri(uri) ?? uri; +} + /** * The label a UI shows for a skill: the declared name, falling back to the URI * path segment, falling back to the raw URI. Never empty, so a list row is @@ -197,6 +213,7 @@ export type SkillIssueCode = | "missing-name" | "malformed-name" | "missing-description" + | "malformed-description" | "malformed-uri" | "name-path-mismatch" | "missing-digest" @@ -238,7 +255,11 @@ export interface SkillIssue { */ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { const issues: SkillIssue[] = []; - const declaredName = entry.frontmatter.name?.trim(); + // The RAW value is what the grammar is applied to — trimming first would let + // `" demo "` pass, and whitespace is not in the Agent Skills name grammar. + // The trimmed copy exists only to tell "absent" from "present but invalid". + const rawName = entry.frontmatter.name; + const declaredName = rawName?.trim() ? rawName : undefined; const uriName = skillNameFromUri(entry.uri); if (!declaredName) { @@ -251,13 +272,15 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { declaredName.length > SKILL_NAME_MAX_LENGTH || !SKILL_NAME_PATTERN.test(declaredName) ) { + // Reaches here for `" demo "` too: the grammar sees the untrimmed value. 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()) { + const rawDescription = entry.frontmatter.description; + if (!rawDescription?.trim()) { // An error, not a warning: SEP-2640 requires `description` on every skill, // so an absent one is a format violation and must not read as "0 errors". issues.push({ @@ -265,6 +288,12 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { severity: "error", message: "frontmatter.description is required but missing or empty.", }); + } else if (rawDescription.length > SKILL_DESCRIPTION_MAX_LENGTH) { + issues.push({ + code: "malformed-description", + severity: "error", + message: `frontmatter.description is ${rawDescription.length} characters, above the ${SKILL_DESCRIPTION_MAX_LENGTH}-character limit.`, + }); } if (uriName === undefined) { issues.push({ @@ -320,7 +349,16 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { // therefore not "a skill with no extra files" — it is a manifest that cannot // be checked against what the skill actually is, and reporting `Conforms` // for it would be a wrong answer rather than a missing one. - if (!entry.resources.some((resource) => resource.uri === entry.uri)) { + // Compared on the normalized identity, like every other URI comparison here: + // a manifest listing the RFC-equivalent `skill://demo/%53KILL.md` names the + // same file the entry does, and is fetchable as that file, so calling it a + // missing self-entry would be the tool disagreeing with itself. + const entryIdentity = skillUriIdentity(entry.uri); + if ( + !entry.resources.some( + (resource) => skillUriIdentity(resource.uri) === entryIdentity, + ) + ) { issues.push({ code: "manifest-missing-self", severity: "error", @@ -353,7 +391,7 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { // 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; + const identity = skillUriIdentity(resource.uri); if (seenUris.has(identity)) { issues.push({ code: "duplicate-resource", @@ -478,13 +516,23 @@ export function skillEntriesMatch(a: SkillEntry, b: SkillEntry): boolean { * a set; every other array keeps its order. */ function canonicalEntry(entry: SkillEntry): string { - const { resources, ...rest } = entry; + const { resources, uri, ...rest } = entry; + // URIs are compared by IDENTITY, so a server that canonicalizes an escape + // between the listing and the fetch is not reported as a changed snapshot. const manifest = Array.isArray(resources) ? [...resources] - .sort((x, y) => String(x?.uri).localeCompare(String(y?.uri))) + .map((resource) => ({ + ...resource, + uri: skillUriIdentity(String(resource?.uri)), + })) + .sort((x, y) => x.uri.localeCompare(y.uri)) .map(canonicalize) : resources; - return JSON.stringify({ ...sortKeys(rest), resources: manifest }); + return JSON.stringify({ + ...sortKeys(rest), + uri: skillUriIdentity(uri), + resources: manifest, + }); } /** Object keys sorted recursively; array ORDER is preserved throughout. */