From da8cc166ceb553155f8122242cba2b667c03c839 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 21:23:58 -0400 Subject: [PATCH] fix: address Copilot review round 1 on #2251 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - skills.ts: report a manifest that omits the skill's own SKILL.md (an empty list included), duplicate URIs, entries outside the skill root, and a missing size. `Conforms` was reachable for manifests that break invariants SEP-2640 states. - skills.ts: cross-check the declared byte length before hashing. A size that disagrees fails verification on its own — the digest is taken over the bytes the server served, so agreeing with it says nothing about whether the manifest describes them. - skills.ts: copy the view instead of slicing its backing store in `sha256Digest`. `SharedArrayBuffer.prototype.slice()` returns another SharedArrayBuffer, which `crypto.subtle.digest` rejects — the cast claimed to handle the exact input that would have thrown. No cast now. - skills.ts: state the one obligation NOT checked here — that an entry's frontmatter matches the fetched SKILL.md's. The digest cannot cover it, and closing it needs a YAML parser, so it is tracked on #2248. - managedSkillsState: cap the walk at LIST_MAX_PAGES. The repeated-cursor guard only catches a server stuck on one cursor; endlessly unique ones walked forever. Raises rather than truncating, like the salvage walk. - managedSkillsState: gate every write on a session generation, so a walk that resolves after a disconnect or destroy cannot repopulate a cleared store or deliver the previous session's skills into the next. - inspectorClient: send a cursor when it is `!== undefined`, not when it is truthy. An opaque cursor may be `""`, and dropping it re-requested page one — which the store then reported as a repeated-cursor failure. - SkillsScreen: key verdict invalidation on the manifest (URI + digests + sizes), not the URI alone. A Refresh that changed the manifest left a green badge attached to a digest nothing had checked. - SkillsScreen: epoch-guard every read continuation, so a fetch that resolves after the selection moved on cannot write into the new one. - SkillsScreen: bound "Verify all" to 4 concurrent reads. A conforming manifest may hold 512 files. - skillsSchemas: drop the guessed `resources/directory/read` result schema. Nothing calls it, so an unverified shape could sit wrong indefinitely; phase 3 adds it against the normative text. - skills-http.json: declare the extension bare. It advertised `directoryRead: true` with no handler, so Connection Info reported "Supported" for a method that answers -32601. - types.ts: restore the Tasks doc comment the Skills interface displaced. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.stories.tsx | 28 ++- .../SkillsScreen/SkillsScreen.test.tsx | 166 +++++++++++++++--- .../screens/SkillsScreen/SkillsScreen.tsx | 80 +++++++-- .../components/views/InspectorView/types.ts | 2 +- clients/web/src/test/core/mcp/skills.test.ts | 157 +++++++++++++++-- .../src/test/core/mcp/skillsSchemas.test.ts | 13 -- .../core/mcp/state/managedSkillsState.test.ts | 55 ++++++ core/mcp/inspectorClient.ts | 6 +- core/mcp/skills.ts | 110 +++++++++++- core/mcp/skillsSchemas.ts | 28 +-- core/mcp/state/managedSkillsState.ts | 43 ++++- docs/test-servers.md | 10 +- test-servers/configs/skills-http.json | 4 +- test-servers/src/composable-test-server.ts | 9 +- 14 files changed, 604 insertions(+), 107 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx index 101d7383..b1a97355 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx @@ -16,10 +16,21 @@ function StatefulSkillsScreen(args: ComponentProps) { } const REF_TEXT = "# Column rules\n"; -// The digest of REF_TEXT, so the clean skill really does verify when the -// "Verify all" story runs — a placeholder here would demo a false green. +const SELF_TEXT = "# skill\n"; +// The real digests of those two strings, so the clean skill actually verifies +// when the "Verify all" story runs — a placeholder would demo a false green. const REF_DIGEST = "sha256:e201429aa2684958ca1a0537ab4eb4b7eb3a81c71e7cc7a11397eb500738e015"; +const SELF_DIGEST = + "sha256:6504f2de0a1febf7492c3b98f93d9ab49558eb364607a706f02fe9a75aa7f75b"; + +/** Every manifest lists the skill's own SKILL.md — a manifest is the complete + * file set, so one that omits it is a `manifest-missing-self` error. */ +const selfEntry = (path: string) => ({ + uri: `skill://${path}/SKILL.md`, + digest: SELF_DIGEST, + size: 8, +}); const sampleSkills: SkillEntry[] = [ { @@ -29,10 +40,11 @@ const sampleSkills: SkillEntry[] = [ description: "Analyze a CSV and summarize its columns", }, resources: [ + selfEntry("data-analysis"), { uri: "skill://data-analysis/reference.md", digest: REF_DIGEST, - size: REF_TEXT.length, + size: 15, }, ], }, @@ -43,10 +55,14 @@ const sampleSkills: SkillEntry[] = [ description: "Advertises a digest its bytes do not match", }, resources: [ + selfEntry("tampered-notes"), { + // A well-formed digest of bytes the fake read does not return, with a + // size that agrees — so the reported failure is a *digest* mismatch + // rather than the cheaper size cross-check. uri: "skill://tampered-notes/notes.md", digest: `sha256:${"b".repeat(64)}`, - size: 12, + size: 8, }, ], }, @@ -64,7 +80,7 @@ const sampleSkills: SkillEntry[] = [ name: "right-name", description: "URI path segment disagrees with frontmatter.name", }, - resources: [], + resources: [selfEntry("wrong-folder")], }, ]; @@ -81,7 +97,7 @@ const meta: Meta = { onReadSkillFile: fn(async (uri: string) => uri.endsWith("reference.md") ? { text: REF_TEXT } - : { text: `# ${uri}\n`, mimeType: "text/markdown" }, + : { text: SELF_TEXT, mimeType: "text/markdown" }, ), }, render: (args) => , diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 90de63c1..9ee2532c 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -16,11 +16,17 @@ import { import { EMPTY_SKILLS_UI } from "../screenUiState"; const REF_TEXT = "# Column rules\n"; -// Computed once at module load so the fixture's advertised digest really is the -// digest of the bytes the fake read returns — a hard-coded constant here would +const SELF_TEXT = "# data-analysis\n"; +const NOTES_TEXT = "different\n"; +// Computed once at module load so each fixture's advertised digest really is +// the digest of the bytes the fake read returns — a hard-coded constant would // make the "verified" test pass for the wrong reason if the encoder changed. const REF_DIGEST = await sha256Digest(textToBytes(REF_TEXT)); +const SELF_DIGEST = await sha256Digest(textToBytes(SELF_TEXT)); +// Every manifest lists the skill's own SKILL.md: a manifest is the complete +// file set, so one that omits it is a `manifest-missing-self` error and no +// fixture here would be "clean". const CLEAN_SKILL: SkillEntry = { uri: "skill://data-analysis/SKILL.md", frontmatter: { @@ -28,10 +34,15 @@ const CLEAN_SKILL: SkillEntry = { description: "Analyze a CSV and summarize its columns", }, resources: [ + { + uri: "skill://data-analysis/SKILL.md", + digest: SELF_DIGEST, + size: textToBytes(SELF_TEXT).byteLength, + }, { uri: "skill://data-analysis/reference.md", digest: REF_DIGEST, - size: REF_TEXT.length, + size: textToBytes(REF_TEXT).byteLength, }, ], }; @@ -41,9 +52,17 @@ const TAMPERED_SKILL: SkillEntry = { frontmatter: { name: "tampered", description: "Bad digest" }, resources: [ { + uri: "skill://tampered/SKILL.md", + digest: SELF_DIGEST, + size: textToBytes(SELF_TEXT).byteLength, + }, + { + // A well-formed digest of bytes the fake read does not return, and a + // size that agrees — so the failure reported is a *digest* mismatch and + // not the cheaper size cross-check. uri: "skill://tampered/notes.md", digest: `sha256:${"b".repeat(64)}`, - size: 4, + size: textToBytes(NOTES_TEXT).byteLength, }, ], }; @@ -57,7 +76,13 @@ const DYNAMIC_SKILL: SkillEntry = { const MISMATCHED_SKILL: SkillEntry = { uri: "skill://wrong-folder/SKILL.md", frontmatter: { name: "right-name", description: "Name disagreement" }, - resources: [], + resources: [ + { + uri: "skill://wrong-folder/SKILL.md", + digest: SELF_DIGEST, + size: textToBytes(SELF_TEXT).byteLength, + }, + ], }; const ALL_SKILLS = [ @@ -70,8 +95,8 @@ const ALL_SKILLS = [ /** A `resources/read` that serves the fixture bytes for any known URI. */ const readFixtureFile = vi.fn(async (uri: string) => { if (uri === "skill://data-analysis/reference.md") return { text: REF_TEXT }; - if (uri === "skill://tampered/notes.md") return { text: "different\n" }; - return { text: `# ${uri}\n`, mimeType: "text/markdown" }; + if (uri === "skill://tampered/notes.md") return { text: NOTES_TEXT }; + return { text: SELF_TEXT, mimeType: "text/markdown" }; }); const baseProps: SkillsScreenProps = { @@ -184,7 +209,7 @@ describe("SkillsScreen", () => { renderWithMantine(); await user.click(screen.getByText("data-analysis")); await user.click(screen.getByRole("button", { name: /Verify all/ })); - expect(await screen.findByText("verified")).toBeInTheDocument(); + expect(await screen.findAllByText("verified")).toHaveLength(2); }); it("reports a digest mismatch loudly, with both digests", async () => { @@ -203,7 +228,10 @@ describe("SkillsScreen", () => { renderWithMantine(); await user.click(screen.getByText("data-analysis")); const manifest = screen.getByTestId("skill-manifest"); - await user.click(within(manifest).getByRole("button", { name: "Verify" })); + // One row at a time: the first row's own Verify button, not "Verify all". + await user.click( + within(manifest).getAllByRole("button", { name: "Verify" })[0], + ); expect(await screen.findByText("verified")).toBeInTheDocument(); }); @@ -215,8 +243,9 @@ describe("SkillsScreen", () => { ); await user.click(screen.getByText("data-analysis")); await user.click(screen.getByRole("button", { name: /Verify all/ })); - expect(await screen.findByText("Could not read file")).toBeInTheDocument(); - expect(screen.getByText("403")).toBeInTheDocument(); + // One alert per file in the manifest — both reads failed. + expect(await screen.findAllByText("Could not read file")).toHaveLength(2); + expect(screen.getAllByText("403")).toHaveLength(2); }); it("wraps a non-Error read rejection", async () => { @@ -227,7 +256,7 @@ describe("SkillsScreen", () => { ); await user.click(screen.getByText("data-analysis")); await user.click(screen.getByRole("button", { name: /Verify all/ })); - expect(await screen.findByText("plain string")).toBeInTheDocument(); + expect(await screen.findAllByText("plain string")).toHaveLength(2); }); it("shows the SKILL.md preview on demand", async () => { @@ -267,13 +296,13 @@ describe("SkillsScreen", () => { renderWithMantine(); await user.click(screen.getByText("data-analysis")); await user.click(screen.getByRole("button", { name: /Verify all/ })); - expect(await screen.findByText("verified")).toBeInTheDocument(); + expect(await screen.findAllByText("verified")).toHaveLength(2); // A verdict belongs to the skill it was computed for; carrying it across a // selection change would attribute one skill's result to another. await user.click(screen.getByText("tampered")); expect(screen.queryByText("verified")).not.toBeInTheDocument(); - expect(screen.getByText("—")).toBeInTheDocument(); + expect(screen.getAllByText("—")).toHaveLength(2); }); it("drops the SKILL.md preview when the selection changes", async () => { @@ -286,22 +315,22 @@ describe("SkillsScreen", () => { expect(screen.queryByTestId("skill-md-preview")).not.toBeInTheDocument(); }); - it("renders an em dash for a manifest entry with no size", async () => { + it("renders an em dash for a manifest entry with no size or digest", async () => { const user = userEvent.setup(); renderWithMantine( , ); await user.click(screen.getByText("data-analysis")); const manifest = screen.getByTestId("skill-manifest"); - // Three em dashes in the row: the size cell, the digest cell, and the - // not-yet-run verification badge — which stays distinct from + // Three em dashes in the single row: the size cell, the digest cell, and + // the not-yet-run verification badge — which stays distinct from // "unverifiable" so an absent digest is never mistaken for an unrun check. expect(within(manifest).getAllByText("—")).toHaveLength(3); }); @@ -314,7 +343,7 @@ describe("SkillsScreen", () => { { ...CLEAN_SKILL, resources: [ - { uri: "skill://data-analysis/a.md", digest: "sha256:short" }, + { uri: "skill://data-analysis/SKILL.md", digest: "sha256:short" }, ], }, ]} @@ -331,7 +360,7 @@ describe("SkillsScreen", () => { skills={[ { ...CLEAN_SKILL, - resources: [{ uri: "skill://data-analysis/reference.md" }], + resources: [{ uri: "skill://data-analysis/SKILL.md" }], }, ]} />, @@ -340,4 +369,101 @@ describe("SkillsScreen", () => { await user.click(screen.getByRole("button", { name: /Verify all/ })); expect(await screen.findByText("unverifiable")).toBeInTheDocument(); }); + + it("drops verdicts when a refresh replaces the manifest for the same skill", async () => { + // The selection never changes, so keying invalidation on the URI alone + // would leave a green `verified` badge attached to a digest the refresh + // replaced — the UI vouching for content it has never checked. + const user = userEvent.setup(); + const { rerender } = renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + expect(await screen.findAllByText("verified")).toHaveLength(2); + + rerender( + , + ); + expect(screen.queryByText("verified")).not.toBeInTheDocument(); + }); + + it("discards a verification that resolves after the selection moved on", async () => { + // A read still in flight when the user switches skills must not write its + // verdict into the newly selected skill's rows. + const user = userEvent.setup(); + let release: ((value: { text: string }) => void) | undefined; + const onReadSkillFile = vi.fn( + () => + new Promise<{ text: string }>((resolve) => { + release = resolve; + }), + ); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + await user.click(screen.getByText("tampered")); + release?.({ text: SELF_TEXT }); + // Nothing from the abandoned read reaches the new selection's rows. + expect(screen.queryByText("verified")).not.toBeInTheDocument(); + expect(screen.queryByText("mismatch")).not.toBeInTheDocument(); + }); + + it("discards a SKILL.md read that resolves after the selection moved on", async () => { + const user = userEvent.setup(); + let release: ((value: { text: string }) => void) | undefined; + const onReadSkillFile = vi.fn( + () => + new Promise<{ text: string }>((resolve) => { + release = resolve; + }), + ); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); + await user.click(screen.getByText("tampered")); + release?.({ text: SELF_TEXT }); + expect(screen.queryByTestId("skill-md-preview")).not.toBeInTheDocument(); + }); + + it("discards a failed SKILL.md read that resolves after the selection moved on", async () => { + const user = userEvent.setup(); + let fail: ((err: Error) => void) | undefined; + const onReadSkillFile = vi.fn( + () => + new Promise<{ text: string }>((_resolve, reject) => { + fail = reject; + }), + ); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); + await user.click(screen.getByText("tampered")); + fail?.(new Error("too late")); + expect(screen.queryByText("too late")).not.toBeInTheDocument(); + }); }); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index d325c1a6..1f313cd0 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -1,4 +1,4 @@ -import { useCallback, useMemo, useState } from "react"; +import { useCallback, useMemo, useRef, useState } from "react"; import { Alert, Badge, @@ -35,6 +35,13 @@ import { type SkillFileContents, } from "../../../utils/skillFileBytes"; +/** + * How many skill files are read at once by "Verify all". A conforming manifest + * may hold 512 entries, so this is what keeps one click from becoming 512 + * simultaneous `resources/read` calls. + */ +const VERIFY_CONCURRENCY = 4; + /** Per-file verification progress, keyed by the manifest entry's URI. */ type FileState = | { status: "pending" } @@ -235,16 +242,12 @@ export function SkillsScreen({ const [fileStates, setFileStates] = useState>({}); const [preview, setPreview] = useState(null); const [previewError, setPreviewError] = useState(null); - - // Changing the selection invalidates every verdict and the SKILL.md preview: - // they belong to the skill that was selected. Adjusted DURING RENDER via - // `useValueChange` rather than in an effect, so the new skill never paints - // for a frame carrying the previous one's verification results. - useValueChange(selectedSkillUri, () => { - setFileStates({}); - setPreview(null); - setPreviewError(null); - }); + // Bumped every time the verdicts are invalidated. A read that was already in + // flight captures the value it started under and discards its result when + // this has moved on — otherwise a slow fetch for the previous selection (or + // the previous manifest) lands afterwards and writes a verdict for content + // nobody is looking at, or worse, one that was never checked. + const epoch = useRef(0); const filtered = useMemo(() => { const needle = search.trim().toLowerCase(); @@ -274,8 +277,37 @@ export function SkillsScreen({ [selected], ); + // What every verdict on screen is a verdict *about*: the selected skill AND + // the manifest it advertised. Keying invalidation on the URI alone would + // leave a green `verified` badge attached to a digest the Refresh replaced, + // so the UI would vouch for content it has never checked. A primitive string + // rather than the manifest object, because `useValueChange` compares with + // `Object.is` and a fresh array every render would loop. + const manifestKey = useMemo( + () => + [ + selectedSkillUri ?? "", + selected?.resources === DYNAMIC_RESOURCES ? "dynamic" : "", + ...manifest.map((r) => `${r.uri}|${r.digest ?? ""}|${r.size ?? ""}`), + ].join("\n"), + [manifest, selected, selectedSkillUri], + ); + + // Adjusted DURING RENDER via `useValueChange` rather than in an effect, so a + // new selection (or a refreshed manifest) never paints a frame carrying the + // previous one's verification results. The epoch bump is a ref write, which + // is why it is done in the callback alongside the state resets rather than + // during the render body itself. + useValueChange(manifestKey, () => { + epoch.current += 1; + setFileStates({}); + setPreview(null); + setPreviewError(null); + }); + const verifyFile = useCallback( async (resource: SkillResource) => { + const started = epoch.current; setFileStates((prev) => ({ ...prev, [resource.uri]: { status: "pending" }, @@ -286,11 +318,13 @@ export function SkillsScreen({ resource, skillFileBytes(contents), ); + if (epoch.current !== started) return; setFileStates((prev) => ({ ...prev, [resource.uri]: { status: "done", verification }, })); } catch (err) { + if (epoch.current !== started) return; setFileStates((prev) => ({ ...prev, [resource.uri]: { @@ -304,22 +338,40 @@ export function SkillsScreen({ ); const verifyAll = useCallback(() => { + // Bounded concurrency, not `Promise.all` over the whole manifest: a + // conforming skill may declare 512 files, and firing 512 simultaneous + // `resources/read` calls would bury the transport and the server for no + // gain. Workers pull from a shared cursor so each row still flips to + // `checking…` and then to its verdict as it lands, rather than all at once. + // // Held rather than floated: each `verifyFile` owns its own failures (it - // records them as per-file state), and this handler cannot be async, so the + // records them as per-row state), and this handler cannot be async, so the // settled promise is discarded explicitly at one place instead of per file. - void Promise.all(manifest.map((resource) => verifyFile(resource))); + let next = 0; + const worker = async (): Promise => { + for (let i = next++; i < manifest.length; i = next++) { + await verifyFile(manifest[i]); + } + }; + const workers = Math.min(VERIFY_CONCURRENCY, manifest.length); + void Promise.all(Array.from({ length: workers }, () => worker())); }, [manifest, verifyFile]); const showSkillMd = useCallback(() => { if (!selected) return; + const started = epoch.current; // A click handler cannot await, and this chain terminates in its own - // `catch` that surfaces the message in the preview slot. + // `catch` that surfaces the message in the preview slot. Both arms are + // epoch-guarded: a read that resolves after the selection moved on would + // otherwise show one skill's SKILL.md under another's heading. void onReadSkillFile(selected.uri) .then((contents) => { + if (epoch.current !== started) return; setPreview(contents); setPreviewError(null); }) .catch((err: unknown) => { + if (epoch.current !== started) return; setPreview(null); setPreviewError(err instanceof Error ? err.message : String(err)); }); diff --git a/clients/web/src/components/views/InspectorView/types.ts b/clients/web/src/components/views/InspectorView/types.ts index 02a9ee86..4950765c 100644 --- a/clients/web/src/components/views/InspectorView/types.ts +++ b/clients/web/src/components/views/InspectorView/types.ts @@ -311,7 +311,6 @@ export interface AppsPanelProps { onRefreshApps: () => void; } -/** The Tasks monitor: the task list, its progress map, and actions. */ /** The Skills screen (SEP-2640): the enumerated skills and their verification. */ export interface SkillsPanelProps { skills: SkillEntry[]; @@ -325,6 +324,7 @@ export interface SkillsPanelProps { onReadSkillFile: (uri: string) => Promise; } +/** The Tasks monitor: the task list, its progress map, and actions. */ export interface TasksPanelProps { tasks: Task[]; progressByTaskId?: Record; diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index 46afa98d..ace626b1 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -22,16 +22,20 @@ import { const HELLO_SHA256 = "sha256:2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824"; +const DIGEST = `sha256:${"a".repeat(64)}`; + +/** + * A conforming entry: the manifest is complete (it lists the skill's own + * SKILL.md), unique, inside the skill root, and every row carries a digest and + * a size. Overrides make exactly one of those false, one test at a time. + */ function entry(overrides: Partial = {}): SkillEntry { return { uri: "skill://demo/SKILL.md", frontmatter: { name: "demo", description: "A demo skill" }, resources: [ - { - uri: "skill://demo/ref.md", - digest: `sha256:${"a".repeat(64)}`, - size: 10, - }, + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + { uri: "skill://demo/ref.md", digest: DIGEST, size: 10 }, ], ...overrides, }; @@ -188,12 +192,90 @@ describe("checkSkillConformance", () => { it("reports a manifest entry with no digest as unverifiable", () => { const issues = checkSkillConformance( - entry({ resources: [{ uri: "skill://demo/ref.md", size: 1 }] }), + entry({ + resources: [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + { uri: "skill://demo/ref.md", size: 1 }, + ], + }), ); expect(issues.map((i) => i.code)).toEqual(["missing-digest"]); expect(issues[0].resourceUri).toBe("skill://demo/ref.md"); }); + 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. + const issues = checkSkillConformance( + entry({ + resources: [{ uri: "skill://demo/ref.md", digest: DIGEST, size: 1 }], + }), + ); + expect(issues.map((i) => i.code)).toEqual(["manifest-missing-self"]); + expect(issues[0].severity).toBe("error"); + }); + + it("reports an empty manifest through the same finding", () => { + const issues = checkSkillConformance(entry({ resources: [] })); + expect(issues.map((i) => i.code)).toEqual(["manifest-missing-self"]); + }); + + it("reports a duplicated manifest URI", () => { + const dup = { uri: "skill://demo/ref.md", digest: DIGEST, size: 1 }; + const issues = checkSkillConformance( + entry({ + resources: [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + dup, + dup, + ], + }), + ); + expect(issues.map((i) => i.code)).toEqual(["duplicate-resource"]); + expect(issues[0].resourceUri).toBe("skill://demo/ref.md"); + }); + + it("reports a manifest entry outside the skill root", () => { + const issues = checkSkillConformance( + entry({ + resources: [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + { uri: "skill://other/ref.md", digest: DIGEST, size: 1 }, + ], + }), + ); + expect(issues.map((i) => i.code)).toEqual(["resource-outside-skill-root"]); + expect(issues[0].resourceUri).toBe("skill://other/ref.md"); + }); + + it("does not check the skill root when the entry URI is malformed", () => { + // There is no root to measure against, and `malformed-uri` already says so; + // a second finding per resource would present one defect as many. + const issues = checkSkillConformance( + entry({ + uri: "skill://demo/other.md", + resources: [{ uri: "skill://elsewhere/a.md", digest: DIGEST, size: 1 }], + }), + ); + expect(issues.map((i) => i.code)).not.toContain( + "resource-outside-skill-root", + ); + expect(issues.map((i) => i.code)).toContain("malformed-uri"); + }); + + it("reports a manifest entry with no size as a warning", () => { + const issues = checkSkillConformance( + entry({ + resources: [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + { uri: "skill://demo/ref.md", digest: DIGEST }, + ], + }), + ); + expect(issues.map((i) => i.code)).toEqual(["missing-size"]); + expect(issues[0].severity).toBe("warning"); + }); + it("reports a digest that is not sha256 + 64 lowercase hex", () => { for (const digest of [ "sha256:XYZ", @@ -202,20 +284,26 @@ describe("checkSkillConformance", () => { `sha256:${"a".repeat(63)}`, ]) { const issues = checkSkillConformance( - entry({ resources: [{ uri: "skill://demo/ref.md", digest }] }), + entry({ + resources: [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + { uri: "skill://demo/ref.md", digest, size: 1 }, + ], + }), ); expect(issues.map((i) => i.code)).toEqual(["malformed-digest"]); } }); it("reports a manifest over the 512-entry limit", () => { - const resources = Array.from( - { length: SKILL_MAX_RESOURCE_ENTRIES + 1 }, - (_unused, i) => ({ + const resources = [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, + ...Array.from({ length: SKILL_MAX_RESOURCE_ENTRIES }, (_unused, i) => ({ uri: `skill://demo/f${i}.md`, - digest: `sha256:${"a".repeat(64)}`, - }), - ); + digest: DIGEST, + size: 1, + })), + ]; const issues = checkSkillConformance(entry({ resources })); expect(issues.map((i) => i.code)).toContain("resource-limit-exceeded"); }); @@ -224,10 +312,11 @@ describe("checkSkillConformance", () => { const issues = checkSkillConformance( entry({ resources: [ + { uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 }, { uri: "skill://demo/big.bin", - digest: `sha256:${"a".repeat(64)}`, - size: SKILL_MAX_TOTAL_BYTES + 1, + digest: DIGEST, + size: SKILL_MAX_TOTAL_BYTES, }, ], }), @@ -240,8 +329,8 @@ describe("checkSkillConformance", () => { entry({ resources: [ { - uri: "skill://demo/big.bin", - digest: `sha256:${"a".repeat(64)}`, + uri: "skill://demo/SKILL.md", + digest: DIGEST, size: SKILL_MAX_TOTAL_BYTES, }, ], @@ -310,6 +399,40 @@ describe("verifySkillResource", () => { expect(result.actualDigest).toBe(HELLO_SHA256); }); + it("fails on a declared size that disagrees with the fetched bytes", async () => { + // A size disagreement is a real inconsistency even when the digest would + // match: the digest is taken over the bytes the server served, so agreeing + // with it says nothing about whether the manifest describes those bytes. + const result = await verifySkillResource( + { uri: "skill://demo/a.md", digest: HELLO_SHA256, size: 999 }, + textToBytes("hello"), + ); + expect(result.status).toBe("mismatch"); + expect(result.expectedSize).toBe(999); + expect(result.actualSize).toBe(5); + expect(result.reason).toMatch(/999 bytes/); + }); + + it("checks the size before hashing, so a bad size never reports verified", async () => { + const result = await verifySkillResource( + { uri: "skill://demo/a.md", size: 1 }, + textToBytes("hello"), + ); + // No digest at all, and still a mismatch — the length alone settles it. + expect(result.status).toBe("mismatch"); + expect(result.actualDigest).toBeUndefined(); + }); + + it("echoes both sizes on a verified result when one was declared", async () => { + const result = await verifySkillResource( + { uri: "skill://demo/a.md", digest: HELLO_SHA256, size: 5 }, + textToBytes("hello"), + ); + expect(result.status).toBe("verified"); + expect(result.expectedSize).toBe(5); + expect(result.actualSize).toBe(5); + }); + it("reports unverifiable when no digest is advertised", async () => { const result = await verifySkillResource( { uri: "skill://demo/a.md" }, diff --git a/clients/web/src/test/core/mcp/skillsSchemas.test.ts b/clients/web/src/test/core/mcp/skillsSchemas.test.ts index 1a19eb2a..e0a09226 100644 --- a/clients/web/src/test/core/mcp/skillsSchemas.test.ts +++ b/clients/web/src/test/core/mcp/skillsSchemas.test.ts @@ -3,7 +3,6 @@ import { DYNAMIC_RESOURCES, GetSkillResultSchema, ListSkillsResultSchema, - ReadResourceDirectoryResultSchema, SKILLS_EXTENSION_KEY, SKILLS_GET_METHOD, SKILLS_LIST_METHOD, @@ -107,15 +106,3 @@ describe("GetSkillResultSchema", () => { expect(() => GetSkillResultSchema.parse({ nothing: true })).toThrow(); }); }); - -describe("ReadResourceDirectoryResultSchema", () => { - it("parses directory children including the directory mime type", () => { - const parsed = ReadResourceDirectoryResultSchema.parse({ - contents: [ - { uri: "skill://demo/sub", mimeType: "inode/directory" }, - { uri: "skill://demo/ref.md", mimeType: "text/markdown", size: 3 }, - ], - }); - expect(parsed.contents).toHaveLength(2); - }); -}); diff --git a/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts b/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts index 79f00351..7a9ecfda 100644 --- a/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts +++ b/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts @@ -3,6 +3,8 @@ import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas"; import { ManagedSkillsState, REPEATED_CURSOR_MESSAGE, + SKILLS_MAX_PAGES, + SKILLS_PAGE_LIMIT_MESSAGE, } from "@inspector/core/mcp/state/managedSkillsState"; import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient"; @@ -105,6 +107,59 @@ describe("ManagedSkillsState", () => { expect(state.getError()?.message).toBe(REPEATED_CURSOR_MESSAGE); }); + it("stops and reports when a server hands back endlessly unique cursors", async () => { + client.setStatus("connected"); + // The repeated-cursor guard cannot see this shape: every cursor is new, so + // the walk would grow without bound. The cap raises rather than truncating + // — returning what we have would present a partial list as a complete one. + let n = 0; + client.listSkills.mockImplementation(async () => ({ + skills: [skill(`s${n}`)], + nextCursor: String(++n), + })); + await expect(state.refresh()).rejects.toThrow(SKILLS_PAGE_LIMIT_MESSAGE); + expect(client.listSkills).toHaveBeenCalledTimes(SKILLS_MAX_PAGES); + // The truncated list is NOT committed. + expect(state.getSkills()).toEqual([]); + }); + + it("abandons a walk whose session ended mid-flight", async () => { + client.setStatus("connected"); + let release: ((value: { skills: SkillEntry[] }) => void) | undefined; + client.listSkills.mockImplementationOnce( + async () => + new Promise((resolve) => { + release = resolve; + }), + ); + const walk = state.refresh(); + // A disconnect lands while the request is still out. + await client.disconnect(); + release?.({ skills: [skill("stale")] }); + await walk; + // The continuation must not repopulate a store the disconnect cleared. + expect(state.getSkills()).toEqual([]); + expect(state.getPagination()).toEqual({ pageCount: 0 }); + }); + + it("does not surface a dead session's failure in the live one", async () => { + client.setStatus("connected"); + let fail: ((err: Error) => void) | undefined; + client.listSkills.mockImplementationOnce( + async () => + new Promise((_resolve, reject) => { + fail = reject; + }), + ); + const walk = state.refresh(); + await client.disconnect(); + fail?.(new Error("from the old session")); + // Still rejected — the caller's auth-recovery wrapper keys off that — but + // the store's observable error is left alone. + await expect(walk).rejects.toThrow("from the old session"); + expect(state.getError()).toBeNull(); + }); + it("records a failure as observable state and re-throws it", async () => { client.setStatus("connected"); const failure = new Error("boom"); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 763beac5..ca59952d 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -5555,7 +5555,11 @@ export class InspectorClient extends InspectorClientEventTarget { const effectiveMeta = this.mergeMeta(metadata); const params: Record = { ...(effectiveMeta ? { _meta: effectiveMeta } : {}), - ...(cursor ? { cursor } : {}), + // `!== undefined`, not truthiness: a cursor is opaque and the empty + // string is a legal value. Dropping `""` would silently re-request page + // one, which the store then reports as a repeated-cursor failure — a + // conforming server made to look broken. + ...(cursor !== undefined ? { cursor } : {}), }; const response = await this.invokeMcpClient( () => diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index eab9770d..0a1aeead 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -22,6 +22,16 @@ * is explicitly not a load and confers no standing, so none of the SEP's host * machinery (activation, per-skill consent, content-bound approval) is * implemented here. Surface and verify. + * + * ⚠️ **One SEP-2640 obligation is deliberately NOT checked here: that an entry's + * `frontmatter` matches the fetched `SKILL.md`'s frontmatter field by field.** + * The digest check does not cover it — a digest is taken over the bytes the + * server served, so it proves the file was not tampered with in transit and + * says nothing about whether the *listing* described that file honestly. A + * server can therefore advertise one description, serve a different one, and + * pass every check in this module. Closing it needs a YAML parser, which is a + * new runtime dependency and a placement decision of its own, so it is tracked + * on #2248 rather than half-done here. */ import type { ServerCapabilities } from "@modelcontextprotocol/client"; @@ -115,6 +125,10 @@ export type SkillIssueCode = | "name-path-mismatch" | "missing-digest" | "malformed-digest" + | "missing-size" + | "duplicate-resource" + | "resource-outside-skill-root" + | "manifest-missing-self" | "resource-limit-exceeded" | "size-limit-exceeded"; @@ -206,7 +220,48 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { }); } + // A manifest is the *complete* file set, and the skill's own SKILL.md is one + // of those files. An empty list, or one that omits the entry's own URI, is + // 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)) { + issues.push({ + code: "manifest-missing-self", + severity: "error", + message: `Manifest does not list the skill's own entry file (${entry.uri}); a manifest must be the complete file set.`, + }); + } + + const seenUris = new Set(); + // Relative references resolve against the skill root, so every manifest entry + // must live under it. A URI outside that prefix is either a typo or a server + // claiming integrity over a file that is not part of this skill. Left + // `undefined` for a malformed entry URI — there is no root to measure + // against, and `malformed-uri` already reports that. + const root = entry.uri.endsWith(SKILL_FILE_SUFFIX) + ? `${entry.uri.slice(0, -SKILL_FILE_SUFFIX.length)}/` + : undefined; + for (const resource of entry.resources) { + if (seenUris.has(resource.uri)) { + issues.push({ + code: "duplicate-resource", + severity: "error", + message: + "Manifest lists this URI more than once; entries must be unique.", + resourceUri: resource.uri, + }); + } + seenUris.add(resource.uri); + if (root !== undefined && !resource.uri.startsWith(root)) { + issues.push({ + code: "resource-outside-skill-root", + severity: "error", + message: `Manifest entry is outside the skill root "${root}".`, + resourceUri: resource.uri, + }); + } if (resource.digest === undefined) { issues.push({ code: "missing-digest", @@ -222,6 +277,18 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { resourceUri: resource.uri, }); } + if (resource.size === undefined) { + // A warning, not an error: an absent size costs the length cross-check in + // `verifySkillResource` and silently understates the 16 MiB total, but + // the digest still verifies the bytes. + issues.push({ + code: "missing-size", + severity: "warning", + message: + "Manifest entry declares no size, so it is excluded from the 16 MiB total and its length cannot be cross-checked.", + resourceUri: resource.uri, + }); + } } return issues; @@ -250,6 +317,10 @@ export interface SkillVerification { actualDigest?: string; /** The manifest's digest, echoed so a mismatch renders both halves. */ expectedDigest?: string; + /** The manifest's declared byte length, when it declared one. */ + expectedSize?: number; + /** The fetched file's actual byte length, when it was measured. */ + actualSize?: number; /** Why the file could not be verified or fetched. */ reason?: string; } @@ -271,13 +342,16 @@ function toHex(bytes: Uint8Array): string { * context, so `subtle` is present there too. */ export async function sha256Digest(bytes: Uint8Array): Promise { - // `BufferSource` wants a plain ArrayBuffer; a Uint8Array over a SharedArrayBuffer - // (or a view into a larger buffer) would otherwise hash the wrong range. - const buffer = bytes.buffer.slice( - bytes.byteOffset, - bytes.byteOffset + bytes.byteLength, - ) as ArrayBuffer; - const hash = await crypto.subtle.digest("SHA-256", buffer); + // Copy the VIEW into a fresh typed array rather than slicing its backing + // store. Two things depend on that: a `Uint8Array` can be a window into a + // larger buffer, so hashing the buffer would digest neighbouring bytes; and + // `SharedArrayBuffer.prototype.slice()` returns another `SharedArrayBuffer`, + // which `crypto.subtle.digest` rejects — so slicing-and-casting would have + // failed at runtime for the exact input a cast claimed to handle. + // `new Uint8Array(view)` always allocates a plain `ArrayBuffer`, which is + // also why no cast is needed here. + const copy = new Uint8Array(bytes); + const hash = await crypto.subtle.digest("SHA-256", copy.buffer); return `sha256:${toHex(new Uint8Array(hash))}`; } @@ -307,11 +381,30 @@ export function base64ToBytes(blob: string): Uint8Array { * says. `"unverifiable"` means the manifest advertised no digest (or advertised * a malformed one, already reported by {@link checkSkillConformance}); nothing * about the file itself is wrong, we simply have nothing to compare against. + * + * The declared `size` is cross-checked **before** the digest and fails + * verification on its own. A length that disagrees with the manifest is a real + * inconsistency even when the digest matches — the digest is taken over the + * bytes the server served, so agreeing with it says nothing about whether the + * manifest describes those bytes — and it is the cheaper check, so a + * 16 MiB file that was never going to verify is not hashed first. */ export async function verifySkillResource( resource: SkillResource, bytes: Uint8Array, ): Promise { + const expectedSize = resource.size; + if (expectedSize !== undefined && expectedSize !== bytes.byteLength) { + return { + status: "mismatch", + expectedSize, + actualSize: bytes.byteLength, + ...(resource.digest !== undefined + ? { expectedDigest: resource.digest } + : {}), + reason: `Manifest declares ${expectedSize} bytes but the fetched file is ${bytes.byteLength}.`, + }; + } const expectedDigest = resource.digest; if (expectedDigest === undefined) { return { @@ -332,5 +425,8 @@ export async function verifySkillResource( status: actualDigest === expectedDigest ? "verified" : "mismatch", actualDigest, expectedDigest, + ...(expectedSize !== undefined + ? { expectedSize, actualSize: bytes.byteLength } + : {}), }; } diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts index 8ee49cbc..bf2d36a5 100644 --- a/core/mcp/skillsSchemas.ts +++ b/core/mcp/skillsSchemas.ts @@ -142,23 +142,13 @@ export const GetSkillResultSchema = z export type GetSkillResult = SkillEntry; /** - * `resources/directory/read` result — the direct (non-recursive) children of a - * directory resource. Present for completeness of the wire surface; the - * Inspector surfaces the `directoryRead` sub-flag today and calls the method in - * phase 3. + * ⚠️ **No `resources/directory/read` result schema here yet, on purpose.** + * + * The method name and the directory MIME type above are stated in SEP-2640; + * the shape of the result it returns is not something this PR verified against + * the normative text, and the Inspector does not call the method (phase 3, + * #2248). Declaring a guessed schema would put an unverified claim in the one + * module that is supposed to be the authority on the wire format — and one + * nothing exercises, so it could be wrong indefinitely without failing + * anything. Phase 3 adds it against the spec, alongside the call that uses it. */ -export const ReadResourceDirectoryResultSchema = z.looseObject({ - contents: z.array( - z.looseObject({ - uri: z.string(), - name: z.string().optional(), - mimeType: z.string().optional(), - size: z.number().optional(), - }), - ), - nextCursor: z.string().optional(), -}); - -export type ReadResourceDirectoryResult = z.infer< - typeof ReadResourceDirectoryResultSchema ->; diff --git a/core/mcp/state/managedSkillsState.ts b/core/mcp/state/managedSkillsState.ts index b28a784d..d45aa115 100644 --- a/core/mcp/state/managedSkillsState.ts +++ b/core/mcp/state/managedSkillsState.ts @@ -21,6 +21,7 @@ import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; import type { SkillEntry } from "../skillsSchemas.js"; +import { LIST_MAX_PAGES } from "../listSalvage.js"; import { isTerminalStatus } from "../types.js"; import type { RequestMetadata } from "../types.js"; import { TypedEventTarget } from "../typedEventTarget.js"; @@ -46,6 +47,16 @@ export interface ManagedSkillsStateEventMap { export const REPEATED_CURSOR_MESSAGE = "Server repeated a pagination cursor in skills/list; stopped to avoid an infinite walk."; +/** + * Page cap for the `skills/list` walk. Mirrors `LIST_MAX_PAGES`, the bound the + * SDK's aggregate verbs and the #1909 salvage re-walks already use, so the two + * pagination paths in this repo cannot drift to different limits. + */ +export const SKILLS_MAX_PAGES = LIST_MAX_PAGES; + +/** The error a walk raises when it hits {@link SKILLS_MAX_PAGES}. */ +export const SKILLS_PAGE_LIMIT_MESSAGE = `skills/list exceeded ${SKILLS_MAX_PAGES} pages without the server's pagination converging`; + export class ManagedSkillsState extends TypedEventTarget { private skills: SkillEntry[] = []; private pageCount = 0; @@ -55,6 +66,9 @@ export class ManagedSkillsState extends TypedEventTarget this.generation === generation; try { const collected: SkillEntry[] = []; const seen = new Set(); @@ -143,9 +167,20 @@ export class ManagedSkillsState extends TypedEventTarget= SKILLS_MAX_PAGES) { + throw new Error(SKILLS_PAGE_LIMIT_MESSAGE); + } if (seen.has(page.nextCursor)) { throw new Error(REPEATED_CURSOR_MESSAGE); } @@ -156,7 +191,12 @@ export class ManagedSkillsState extends TypedEventTarget