From 775107776da75f871091ef762909e33659be1ee7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 7 Sep 2026 18:43:58 -0400 Subject: [PATCH] fix: address Copilot review round 1 on #2248 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings, all real. **Critical — `verifySkills` selected `contents[0]`.** These bytes are hashed against the file's advertised digest, so accepting a block the server labelled something else verifies one file's content against another file's digest, and can report that as `verified`. A false pass is worse than a missing check. `contentsFor(result, uri)` now selects by NORMALIZED identity, which keeps the canonicalized-echo case that motivated the original code while rejecting an unrelated block, and reports a read error when nothing answers for the URI. This is what `onReadSkillFile` already did; the two paths hash bytes against digests and must not disagree about which bytes. **`skills/get` was dispatched without the extension gate** that `skills/list` had. Hoisted into `assertSkillsSupported` so the two cannot drift — declaring the extension commits a server to both. Without it, an undeclared server's -32601 is indistinguishable to a script from the -32602 a declared server returns for a URI it does not serve. **The TUI could strand a user on a hidden Skills tab.** The tab left the bar when the gate went false but `activeTab` did not, so the pane kept rendering for a server that never declared the extension. Reset to `info`, following the Auth precedent — additionally gated on `connected`, because this gate reads a server declaration and would otherwise fire during a reconnect. **The TUI keyed a verdict by URI**, so a refresh replacing the manifest under the same URI left hashes computed for the previous snapshot describing the new one. Keyed on the entry now, as the web screen already was. **A failed "Load more" discarded the pages on screen and the retry cursor.** Both are preserved now, and the success and failure paths share one staleness-guarded `commit` helper so they cannot drift on which results they may write — which is how they came to disagree. Two nits: the six-skill counts. The docs sentence was more wrong than reported (five awkward skills of six, three outright violations) and is rewritten rather than renumbered. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc Signed-off-by: cliffhall --- .../cli/__tests__/run-method-skills.test.ts | 15 +++ clients/cli/src/handlers/run-method.ts | 36 +++++-- clients/tui/__tests__/App.test.tsx | 27 ++++++ clients/tui/__tests__/SkillsTab.test.tsx | 94 +++++++++++++++++++ clients/tui/src/App.tsx | 22 +++++ clients/tui/src/components/SkillsTab.tsx | 39 ++++++-- .../SkillsScreen/SkillsScreen.test.tsx | 65 +++++++++++++ .../screens/SkillsScreen/SkillsScreen.tsx | 73 +++++++++----- .../test/core/mcp/skillsVerification.test.ts | 76 ++++++++++++++- .../mcp/inspectorClient-skills.test.ts | 4 +- core/mcp/skillsVerification.ts | 47 +++++++--- docs/test-servers.md | 10 +- 12 files changed, 452 insertions(+), 56 deletions(-) diff --git a/clients/cli/__tests__/run-method-skills.test.ts b/clients/cli/__tests__/run-method-skills.test.ts index b496e30c..dc4da07a 100644 --- a/clients/cli/__tests__/run-method-skills.test.ts +++ b/clients/cli/__tests__/run-method-skills.test.ts @@ -78,6 +78,21 @@ describe("runMethod skills dispatch (#2248)", () => { ).rejects.toMatchObject({ exitCode: EXIT_CODES.USAGE }); }); + it("rejects skills/get too when the server declares no extension", async () => { + // Declaring the extension commits a server to BOTH methods, so gating one + // and not the other is inconsistent with the thing being checked — and an + // undeclared server's -32601 is indistinguishable to a script from the + // -32602 a declared server returns for a URI it does not serve. + const client = mockClient({ + getSkillsExtension: vi.fn().mockReturnValue(undefined), + getSkill: vi.fn(), + }); + await expect( + runMethod(client, { method: "skills/get", uri: "skill://x/SKILL.md" }), + ).rejects.toMatchObject({ exitCode: EXIT_CODES.USAGE }); + expect(client.getSkill).not.toHaveBeenCalled(); + }); + it("keeps the { skill } envelope on skills/get", async () => { // The client unwraps it for callers that want the entry; a CLI whose // contract is "print the result" must not quietly reshape the wire form. diff --git a/clients/cli/src/handlers/run-method.ts b/clients/cli/src/handlers/run-method.ts index 83a6bac7..c233f9bd 100644 --- a/clients/cli/src/handlers/run-method.ts +++ b/clients/cli/src/handlers/run-method.ts @@ -24,6 +24,28 @@ import type { MethodOutcome, } from "./method-types.js"; +/** + * Refuse a `skills/*` call against a server that never declared the extension. + * + * Shared by `skills/list` and `skills/get` so the two cannot drift: declaring + * the extension commits a server to both, so a client that gates one and not + * the other is inconsistent with the thing it is checking. Not needed for + * `resources/directory/read`, whose stricter `directoryRead` gate lives in + * `InspectorClient` itself. + */ +function assertSkillsSupported( + inspectorClient: InspectorClient, + method: string, +): void { + if (!inspectorClient.getSkillsExtension()) { + throw new CliExitCodeError( + EXIT_CODES.USAGE, + `Server does not declare the ${SKILLS_EXTENSION_KEY} extension, so ${method} is not available.`, + { code: "skills_unsupported" }, + ); + } +} + /** * Run one MCP method against a connected {@link InspectorClient}. * Core method dispatch used by the CLI (and other Inspector Node runners). @@ -299,13 +321,7 @@ export async function runMethod( // list, which is right for a UI that must render *something*, and wrong // for a CLI where "this server has no skills" and "this server does not // serve skills at all" are different answers a script has to tell apart. - if (!inspectorClient.getSkillsExtension()) { - throw new CliExitCodeError( - EXIT_CODES.USAGE, - `Server does not declare the ${SKILLS_EXTENSION_KEY} extension, so ${args.method} is not available.`, - { code: "skills_unsupported" }, - ); - } + assertSkillsSupported(inspectorClient, args.method); managedSkillsState = new ManagedSkillsState(inspectorClient); const skills = await managedSkillsState.refresh(args.metadata); if (args.verify) { @@ -330,6 +346,12 @@ export async function runMethod( "URI is required for skills/get method. Use --uri to specify the skill URI.", ); } + // Same gate as `skills/list`, and for the same reason. Without it an + // undeclared server answers `-32601`, which a script cannot tell apart + // from the `-32602` a *declared* server returns for a skill URI it does + // not serve — "this server has no Skills support" and "no such skill" + // are different answers (Copilot). + assertSkillsSupported(inspectorClient, args.method); const skill = await inspectorClient.getSkill(args.uri, args.metadata); if (args.verify) { const reports = await verifySkills( diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index f47cfaac..d259ccc5 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -806,6 +806,33 @@ describe("App (foundation)", () => { await expectFrame(r, "Select a skill to view details"); }); + it("leaves the Skills tab when the selected server does not serve it", async () => { + // The tab disappears from the bar when the gate goes false, but `activeTab` + // is independent of the bar — so without this the render branch keeps + // showing the pane for a server that never declared the extension, and the + // user is stranded on content they cannot navigate back to (Copilot). + h.ctrl.status = "connected"; + h.ctrl.skillsExtension = { directoryRead: false }; + const r = await mount(oneStdio()); + await expectFrame(r, "Skills"); + r.stdin.write("k"); + await expectFrame(r, "Select a skill to view details"); + + // The server stops declaring it — the shape of switching to one without + // the extension, since the declaration is read off the live client. + h.ctrl.skillsExtension = undefined; + r.rerender( + , + ); + await tick(); + await expectFrame(r, "Server Configuration"); + expect(r.lastFrame() ?? "").not.toContain("Select a skill to view details"); + }); + it("disconnects with 'd' when connected", async () => { h.ctrl.status = "connected"; const { stdin } = await mount(oneStdio()); diff --git a/clients/tui/__tests__/SkillsTab.test.tsx b/clients/tui/__tests__/SkillsTab.test.tsx index ce642e0e..2ef9ff49 100644 --- a/clients/tui/__tests__/SkillsTab.test.tsx +++ b/clients/tui/__tests__/SkillsTab.test.tsx @@ -470,6 +470,100 @@ describe("SkillsTab (#2248)", () => { await tick(); }); + it("drops a verdict when the entry changes under the same URI", async () => { + // A refresh can replace the manifest or the frontmatter without the URI + // moving. A URI-keyed verdict would then present hashes and findings + // computed for the PREVIOUS snapshot as if they described the new one. + const digest = await sha256Digest(textToBytes(SKILL_MD)); + const verifiable: SkillEntry = { + ...clean, + resources: [ + { + uri: "skill://clean/SKILL.md", + digest, + size: textToBytes(SKILL_MD).byteLength, + }, + ], + }; + const { lastFrame, stdin, rerender } = render( + , + ); + stdin.write(ENTER); + await tick(); + expect(lastFrame() ?? "").toContain("Verified — Enter to re-verify"); + + // Same URI, different manifest — the old verdict must not carry over. + rerender( + , + ); + await tick(); + expect(lastFrame() ?? "").toContain( + "[Enter to verify digests and frontmatter]", + ); + }); + + it("keeps a verdict across a reorder that leaves the entry unchanged", async () => { + // The reason the key is the entry rather than the list index: moving a + // skill down the list must not discard a verdict the user paid for. + const digest = await sha256Digest(textToBytes(SKILL_MD)); + const verifiable: SkillEntry = { + ...clean, + resources: [ + { + uri: "skill://clean/SKILL.md", + digest, + size: textToBytes(SKILL_MD).byteLength, + }, + ], + }; + const { lastFrame, stdin, rerender } = render( + , + ); + stdin.write(ENTER); + await tick(); + expect(lastFrame() ?? "").toContain("Verified — Enter to re-verify"); + rerender( + , + ); + await tick(); + expect(lastFrame() ?? "").toContain("Verified — Enter to re-verify"); + }); + it("falls back to the whole URI when a manifest entry has no path separator", async () => { const odd: SkillEntry = { uri: "skill://odd/SKILL.md", diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index cdffc67b..e95a1f0e 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -626,6 +626,28 @@ function App({ !!selectedInspectorClient?.getSkillsExtension() && inspectorStatus === "connected"; + // Switch away from the Skills tab when the selected server does not serve it. + // + // The same handling the Auth tab gets above, and needed for the same reason: + // the tab disappears from the bar when the gate goes false, but `activeTab` + // is independent of the bar, so the render branch would keep showing the pane + // for a server that never declared the extension — content the user can see + // but can no longer navigate back to (Copilot). + // + // Gated on `connected` rather than on the extension alone: the declaration is + // only knowable after the handshake, so resetting while a reconnect is in + // flight would bounce the user off the tab they were reading and not return + // them to it. + useEffect(() => { + if ( + activeTab === "skills" && + inspectorStatus === "connected" && + !showSkillsTab + ) { + setActiveTab("info"); + } + }, [activeTab, inspectorStatus, showSkillsTab]); + // Connect — on 401 or mid-session auth recovery, run OAuth then retry. type TuiOAuthRunResult = | "success" diff --git a/clients/tui/src/components/SkillsTab.tsx b/clients/tui/src/components/SkillsTab.tsx index 4fbc4a1e..f3bfe4e2 100644 --- a/clients/tui/src/components/SkillsTab.tsx +++ b/clients/tui/src/components/SkillsTab.tsx @@ -64,6 +64,18 @@ const FILE_COLOR: Record = { "read-error": "red", }; +/** + * What a verification result is a result *about*: the whole entry, serialized. + * + * `JSON.stringify` is enough here — this compares an entry against a later copy + * of *itself* from the same server, so key order is stable and there is no need + * for the canonical form `skillEntriesMatch` uses to compare two independently + * produced entries. + */ +function entryKey(entry: SkillEntry): string { + return JSON.stringify(entry); +} + /** The file name a manifest URI ends in, for a list that must fit 40 columns. */ function fileNameOf(uri: string): string { const cut = uri.lastIndexOf("/"); @@ -100,14 +112,23 @@ export function SkillsTab({ const [error, setError] = useState(null); const [verifying, setVerifying] = useState(false); /** - * The last verification, keyed by the skill URI it was run for. Keyed rather - * than cleared on selection change so moving off a skill and back does not - * silently discard a verdict the user just paid a round trip for — and keyed - * by URI rather than index so a refresh that reorders the list cannot show - * one skill's verdict under another's name. + * The last verification, keyed by the **entry it was computed against**. + * + * Keyed rather than cleared on selection change, so moving off a skill and + * back does not silently discard a verdict the user just paid a round trip + * for. Keyed by a serialization of the entry rather than by its index, so a + * refresh that reorders the list cannot show one skill's verdict under + * another's name — and rather than by its URI alone, because a refresh can + * replace the manifest or the frontmatter *under the same URI*, and a + * URI-keyed verdict would then present hashes and findings computed for the + * previous snapshot as if they described the new one (Copilot). + * + * The same key the web screen uses, for the same reason: re-verifying after a + * metadata-only refresh is the cheap direction to be wrong in; showing a + * verdict computed against a different entry is not. */ const [report, setReport] = useState<{ - uri: string; + key: string; result: SkillVerifyReport; } | null>(null); const scrollViewRef = useRef(null); @@ -124,7 +145,7 @@ export function SkillsTab({ void (async () => { try { const [result] = await verifySkills(inspectorClient, [skill]); - setReport({ uri: skill.uri, result }); + setReport({ key: entryKey(skill), result }); } catch (err) { if (err instanceof AuthRecoveryRequiredError) { onAuthRecoveryRequired?.(err); @@ -196,7 +217,9 @@ export function SkillsTab({ const detailWidth = width - listWidth; const issues = selectedSkill ? checkSkillConformance(selectedSkill) : []; const activeReport = - selectedSkill && report?.uri === selectedSkill.uri ? report.result : null; + selectedSkill && report?.key === entryKey(selectedSkill) + ? report.result + : null; const manifest = selectedSkill && selectedSkill.resources !== DYNAMIC_RESOURCES ? selectedSkill.resources diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index 2892530c..b33e422e 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -1784,6 +1784,27 @@ describe("SkillsScreen directory browsing (#2248)", () => { }); } + it("starts collapsed, since its content needs a round trip nobody has made", async () => { + // Open, it would hold a button and an empty frame — advertising content + // that is not there while taking height from the sections that have some. + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + expect(screen.getByRole("button", { name: /Directory/ })).toHaveAttribute( + "aria-expanded", + "false", + ); + // Still reachable, and the other sections are unaffected. + expect(screen.getByRole("button", { name: /Resources/ })).toHaveAttribute( + "aria-expanded", + "true", + ); + }); + it("renders no Directory section when the server did not declare directoryRead", async () => { const user = userEvent.setup(); renderWithMantine(); @@ -1808,6 +1829,7 @@ describe("SkillsScreen directory browsing (#2248)", () => { await user.click(screen.getByText("data-analysis")); expect(onReadResourceDirectory).not.toHaveBeenCalled(); + await user.click(screen.getByRole("button", { name: /Directory/ })); await user.click(screen.getByRole("button", { name: "Read directory" })); await waitFor(() => expect(screen.getByTestId("skill-directory")).toBeInTheDocument(), @@ -1836,6 +1858,8 @@ describe("SkillsScreen directory browsing (#2248)", () => { , ); await user.click(screen.getByText("data-analysis")); + // Directory starts collapsed — see `DEFAULT_OPEN_SECTIONS`. + await user.click(screen.getByRole("button", { name: /Directory/ })); await user.click(screen.getByRole("button", { name: "Read directory" })); await waitFor(() => expect(screen.getByTestId("skill-directory")).toBeInTheDocument(), @@ -1906,6 +1930,7 @@ describe("SkillsScreen directory browsing (#2248)", () => { />, ); await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Directory/ })); await user.click(screen.getByRole("button", { name: "Read directory" })); await waitFor(() => expect(screen.getByTestId("skill-directory")).toBeInTheDocument(), @@ -1934,6 +1959,7 @@ describe("SkillsScreen directory browsing (#2248)", () => { />, ); await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Directory/ })); await user.click(screen.getByRole("button", { name: "Read directory" })); await waitFor(() => expect( @@ -2024,6 +2050,7 @@ describe("SkillsScreen directory browsing (#2248)", () => { , ); await user.click(screen.getByText("dynamic-report")); + await user.click(screen.getByRole("button", { name: /Directory/ })); await user.click(screen.getByRole("button", { name: "Read directory" })); await waitFor(() => expect(screen.getByTestId("skill-directory")).toBeInTheDocument(), @@ -2044,6 +2071,7 @@ describe("SkillsScreen directory browsing (#2248)", () => { />, ); await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Directory/ })); await user.click(screen.getByRole("button", { name: "Read directory" })); await waitFor(() => expect(screen.getByText("This directory is empty.")).toBeInTheDocument(), @@ -2063,6 +2091,7 @@ describe("SkillsScreen directory browsing (#2248)", () => { />, ); await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Directory/ })); await user.click(screen.getByRole("button", { name: "Read directory" })); await waitFor(() => expect(screen.getByText(/Not a directory resource/)).toBeInTheDocument(), @@ -2072,6 +2101,41 @@ describe("SkillsScreen directory browsing (#2248)", () => { ).toBeInTheDocument(); }); + it("keeps the pages already shown when Load more fails, and can retry", async () => { + // Replacing the state outright made the table vanish and stranded the + // reader with no way back to that page short of restarting at the root. + const user = userEvent.setup(); + let fail = true; + const onReadResourceDirectory = vi.fn( + async (_uri: string, cursor?: string) => { + if (cursor === undefined) { + return { resources: [CHILD_FILE], nextCursor: "1" } as never; + } + if (fail) { + fail = false; + throw new Error("page two exploded"); + } + return { resources: [CHILD_DIR] } as never; + }, + ); + await openRoot(user, onReadResourceDirectory as never); + await user.click(screen.getByRole("button", { name: "Load more" })); + await waitFor(() => + expect(screen.getByText(/page two exploded/)).toBeInTheDocument(), + ); + // The first page is still on screen… + expect( + within(screen.getByTestId("skill-directory")).getByText(CHILD_FILE.uri), + ).toBeInTheDocument(); + // …and the cursor survived, so the same page can be retried. + await user.click(screen.getByRole("button", { name: "Load more" })); + await waitFor(() => { + const table = within(screen.getByTestId("skill-directory")); + expect(table.getByText(CHILD_FILE.uri)).toBeInTheDocument(); + expect(table.getByText(CHILD_DIR.uri)).toBeInTheDocument(); + }); + }); + it("drops a listing when the selection changes mid-read", async () => { // A read still in flight when the user switches skills must not land // afterwards and paint one skill's tree under another's name. @@ -2089,6 +2153,7 @@ describe("SkillsScreen directory browsing (#2248)", () => { />, ); await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Directory/ })); await user.click(screen.getByRole("button", { name: "Read directory" })); await user.click(screen.getByText("right-name")); release?.({ resources: [CHILD_FILE] }); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 0a34fea6..df83a133 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -495,6 +495,27 @@ const ALL_SECTIONS = [ "resource", ]; +/** + * The sections that open by default — everything except Directory. + * + * Directory is the one section whose content requires a round trip the user has + * not made yet, so open it holds a button and an empty frame: it advertises + * content that is not there, while taking height from the sections that do have + * some. With five open sections in a short pane each is squeezed to its floor + * and scrolls internally, which is the documented fallback but a poor first + * impression — and the one it costs most is the file viewer, the section + * `viewerFlex` exists to give the remainder to. + * + * The same argument as Conformance's auto-collapse, one step earlier: that one + * closes a section whose header already carries the whole answer, this one + * closes a section that has no answer yet. Both are defaults; neither prevents + * opening it, and `openSections` outlives a selection, so a user who opens + * Directory keeps it open across skills. + */ +const DEFAULT_OPEN_SECTIONS = ALL_SECTIONS.filter( + (section) => section !== "directory", +); + /** * The open set for the FIRST render. * @@ -508,13 +529,13 @@ function initialOpenSections( skills: SkillEntry[], selectedSkillUri: string | undefined, ): string[] { - if (selectedSkillUri === undefined) return ALL_SECTIONS; + if (selectedSkillUri === undefined) return DEFAULT_OPEN_SECTIONS; const wanted = skillUriIdentity(selectedSkillUri); const entry = skills.find((skill) => skillUriIdentity(skill.uri) === wanted); - if (entry === undefined) return ALL_SECTIONS; + if (entry === undefined) return DEFAULT_OPEN_SECTIONS; return checkSkillConformance(entry).length > 0 - ? ALL_SECTIONS - : ALL_SECTIONS.filter((section) => section !== "conformance"); + ? DEFAULT_OPEN_SECTIONS + : DEFAULT_OPEN_SECTIONS.filter((section) => section !== "conformance"); } /** @@ -1032,11 +1053,17 @@ export function SkillsScreen({ (uri: string, key: string, cursor?: string) => { if (!onReadResourceDirectory) return; const attempt = (nextAttempt.current += 1); - const write = (next: Omit) => + /** + * Commit a settled result, dropping it when it no longer belongs to the + * pane on screen — a different skill, or a newer read of this one. + */ + const commit = ( + next: (prev: DirectoryState) => Omit, + ) => setDirectory((prev) => { if (prev.key !== null && prev.key !== key) return prev; if (prev.attempt !== undefined && prev.attempt > attempt) return prev; - return { key, attempt, ...next }; + return { key, attempt, ...next(prev) }; }); // The path is claimed before the request goes out, so the header names // the directory being read rather than continuing to announce the @@ -1058,26 +1085,28 @@ export function SkillsScreen({ // `catch`, which surfaces the message in the section. void onReadResourceDirectory(uri, cursor) .then((page) => { - setDirectory((prev) => { - if (prev.key !== null && prev.key !== key) return prev; - if (prev.attempt !== undefined && prev.attempt > attempt) { - return prev; - } - const held = cursor === undefined ? [] : (prev.children ?? []); - return { - key, - attempt, - uri, - children: [...held, ...page.resources], - nextCursor: page.nextCursor, - }; - }); + commit((prev) => ({ + uri, + children: [ + ...(cursor === undefined ? [] : (prev.children ?? [])), + ...page.resources, + ], + nextCursor: page.nextCursor, + })); }) .catch((err: unknown) => { - write({ + // A FAILED page leaves what is already on screen where it is, and + // keeps the cursor that would retry it. Replacing the state outright + // made the table vanish and stranded the reader with no way back to + // that page short of restarting at the root (Copilot). Only a first + // read of a directory has nothing to preserve. + commit((prev) => ({ uri, + ...(cursor === undefined + ? {} + : { children: prev.children, nextCursor: cursor }), message: err instanceof Error ? err.message : String(err), - }); + })); }); }, [onReadResourceDirectory], diff --git a/clients/web/src/test/core/mcp/skillsVerification.test.ts b/clients/web/src/test/core/mcp/skillsVerification.test.ts index 57906b82..96054f87 100644 --- a/clients/web/src/test/core/mcp/skillsVerification.test.ts +++ b/clients/web/src/test/core/mcp/skillsVerification.test.ts @@ -137,13 +137,87 @@ describe("verifySkills (#2248)", () => { expect(report.ok).toBe(false); }); + it("refuses a block for a DIFFERENT uri rather than verifying it", async () => { + // The dangerous shape, and the reason positional selection is wrong: these + // bytes are about to be hashed against THIS file's advertised digest, so + // accepting a block the server labelled something else verifies one file's + // content against another file's digest — and can report that as + // `verified`. A false pass is worse than a missing check. + const skill = await entry(); + const readResource = vi.fn(async () => ({ + result: { + contents: [{ uri: "skill://demo/unrelated.md", text: "other bytes" }], + }, + })); + 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.files[0].reason).toMatch(/no content block for this URI/); + expect(report.ok).toBe(false); + }); + + it("finds the matching block when it is not the first one", async () => { + // A server may answer with more than one block, in any order; taking + // `contents[0]` would hash the wrong file's bytes. + const bytes = new TextEncoder().encode(REF); + const skill: SkillEntry = { + uri: "skill://demo/SKILL.md", + frontmatter: { name: "demo", description: "A demo" }, + resources: [ + { + uri: "skill://demo/ref.md", + digest: await sha256Digest(bytes), + size: bytes.byteLength, + }, + ], + }; + const readResource = vi.fn(async () => ({ + result: { + contents: [ + { uri: "skill://demo/decoy.md", text: "decoy" }, + { uri: "skill://demo/ref.md", text: REF }, + ], + }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(report.files[0].status).toBe("verified"); + }); + + it("ignores a malformed block while still finding the real one", async () => { + const bytes = new TextEncoder().encode(REF); + const skill: SkillEntry = { + uri: "skill://demo/SKILL.md", + frontmatter: { name: "demo", description: "A demo" }, + resources: [ + { + uri: "skill://demo/ref.md", + digest: await sha256Digest(bytes), + size: bytes.byteLength, + }, + ], + }; + const readResource = vi.fn(async () => ({ + result: { + contents: [ + null, + { uri: 42 }, + { uri: "skill://demo/ref.md", text: REF }, + ], + }, + })); + const client = { readResource } as unknown as InspectorClientProtocol; + const [report] = await verifySkills(client, [skill]); + expect(report.files[0].status).toBe("verified"); + }); + it("reports a response with no content blocks as a read failure", async () => { const skill = await entry(); const readResource = vi.fn(async () => ({ result: { contents: [] } })); const client = { readResource } as unknown as InspectorClientProtocol; const [report] = await verifySkills(client, [skill]); expect(report.files[0]).toMatchObject({ status: "read-error" }); - expect(report.files[0].reason).toMatch(/no content blocks/); + expect(report.files[0].reason).toMatch(/no content block for this URI/); }); it("reports a block carrying neither text nor blob as a read failure", async () => { diff --git a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts index 6e3df4ec..8c3eceef 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts @@ -121,8 +121,8 @@ describe("Skills extension over a real transport (#2234)", () => { // below — so a modern page missing the envelope surfaces here as a // rejection rather than as a missing property. // - // The fixture pages at two over five skills, so a client that stops - // here sees less than half. + // The fixture pages at two over six skills, so a client that stops + // here sees a third of the catalog. expect(first.skills).toHaveLength(2); expect(first.nextCursor).toBeDefined(); diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index 0ac9423f..650ae132 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -32,6 +32,7 @@ import { checkSkillFrontmatterMatch, skillDisplayName, skillFileBytes, + skillUriIdentity, verifySkillResource, type SkillIssue, type SkillVerification, @@ -85,19 +86,38 @@ interface ReadContents { } /** - * The first content block of a `resources/read` result. + * The block of a `resources/read` result that answers for `uri` — **selected by + * URI, never by position**. * - * `contents[0]` rather than a search by URI: a server may legitimately answer - * with a canonicalized spelling of the URI we asked for, and matching on the - * string would reject it. A result with no blocks is a read failure and is - * reported as one. + * `contents` is an array, and taking `contents[0]` is wrong in the one way that + * matters here: these bytes are about to be hashed against `uri`'s advertised + * digest, so accepting a block the server labelled something else would verify + * one file's content against another file's digest — and could report that as + * `verified`. A false pass from a positional read is worse than a missing + * check, because it is an affirmative statement about a file nobody looked at. + * + * A **normalized** match is accepted, because a server may echo the URI back in + * a different but equivalent form — a resolved `..`, a percent-encoding + * difference. That is what `skillUriIdentity` is for, and it is the same rule + * the whole module applies to every other URI comparison, so a server cannot be + * treated as conforming by one check and non-conforming by another. + * + * `undefined` when nothing answers for the URI, which the caller reports as a + * read failure. This mirrors `onReadSkillFile` in the web client, deliberately: + * two code paths that hash bytes against a digest must not disagree about which + * bytes they are. */ -function firstContents(result: unknown): ReadContents | undefined { +function contentsFor(result: unknown, uri: string): ReadContents | undefined { const contents = (result as { contents?: unknown })?.contents; - if (!Array.isArray(contents) || contents.length === 0) return undefined; - const first: unknown = contents[0]; - if (typeof first !== "object" || first === null) return undefined; - return first as ReadContents; + if (!Array.isArray(contents)) return undefined; + const wanted = skillUriIdentity(uri); + for (const block of contents) { + if (typeof block !== "object" || block === null) continue; + const got = (block as { uri?: unknown }).uri; + if (typeof got !== "string") continue; + if (skillUriIdentity(got) === wanted) return block as ReadContents; + } + return undefined; } /** @@ -136,7 +156,7 @@ export async function verifySkills( let contents: ReadContents | undefined; try { const invocation = await client.readResource(resource.uri, metadata); - contents = firstContents(invocation.result); + contents = contentsFor(invocation.result, resource.uri); } catch (err) { if (err instanceof AuthRecoveryRequiredError) throw err; files.push({ @@ -150,7 +170,8 @@ export async function verifySkills( files.push({ uri: resource.uri, status: "read-error", - reason: "resources/read returned no content blocks.", + reason: + "resources/read returned no content block for this URI, so there are no bytes that can be checked against its digest.", }); continue; } @@ -179,7 +200,7 @@ export async function verifySkills( if (entryText === undefined) { try { const invocation = await client.readResource(entry.uri, metadata); - const contents = firstContents(invocation.result); + const contents = contentsFor(invocation.result, entry.uri); if (typeof contents?.text === "string") entryText = contents.text; } catch (err) { // Left undefined: the frontmatter check is skipped below. When the diff --git a/docs/test-servers.md b/docs/test-servers.md index 71c1dacd..f33f3633 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -88,10 +88,14 @@ It works on **either era**: `skills/list`, `skills/get` and era codec defines, so the SDK's era gate skips them entirely — which is why this fixture, unlike the tasks ones, needs no per-era variant. -Four of the six skills are deliberately awkward, because the checks the Skills -tab runs are untestable without them. Only two are actual violations — the +Five of the six skills are deliberately awkward, because the checks the Skills +tab runs are untestable without them. Only **three** are outright violations +(`tampered-notes`, `lying-listing`, `wrong-folder` — the three `--verify` fails +on). The other two are subtler and neither is an error on its own: the `"dynamic"` form is **conforming**, and is here because "legal but unverifiable" -is the case most easily buried: +is the case most easily buried; and `stale-manifest`'s entry is fully conforming +too, with the defect living in the disagreement between its manifest and its +directory listing: | Skill | What it exercises | | --- | --- |