From a561cd1daa747cad59ed878d8b0c534a90a28ee2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 02:21:41 -0400 Subject: [PATCH] fix: address Copilot review round 15 on #2251 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 15 generated no new inline comments; all five suppressed findings were real and unaddressed. - SkillsScreen: claim the attempt BEFORE issuing the request, for both the SKILL.md preview and `skills/get`. Recording it only on settle left a window where an older request that happened to resolve first was still considered current and published while a newer one was in flight — `verifyRow` already claimed its row up front, these two did not. Tests answer the OLDER request first and assert nothing is published. - SkillsScreen: "No skills listed", not "No skills". SEP-2640 lets a server return an empty or partial catalog and says an empty result is not proof it has none — an unlisted skill is still fetchable by URI — so the old copy had the tool asserting something the protocol explicitly does not. - SkillsScreen: each row's Verify button gets an `aria-label` carrying its URI. Every row's visible text is "Verify" and the URI cell is not programmatically associated with the button, so a screen-reader user had no way to tell which file each control checked. - inspectorClient: attribute a rejected `skills/get` envelope with `markResponseRejected`, so the Protocol tab stops rendering it as a clean success while the screen shows an error. In the client rather than a store because `skills/get` has none. Gated on `isClientDecodeRejection`, with a test that a transport failure is NOT attributed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall --- .../SkillsScreen/SkillsScreen.test.tsx | 87 +++++++++++++++++-- .../screens/SkillsScreen/SkillsScreen.tsx | 25 +++++- .../core/mcp/inspectorClient-skills.test.ts | 47 ++++++++++ core/mcp/inspectorClient.ts | 37 ++++++-- 4 files changed, 176 insertions(+), 20 deletions(-) diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index b70067f5..88df0cbb 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -153,9 +153,14 @@ describe("SkillsScreen", () => { expect(root).toHaveAttribute("data-skill-page-count", "2"); }); - it("renders 'No skills' when the list is empty", () => { + it("says the list was empty without claiming there are no skills", () => { + // SEP-2640 lets a server return an empty or partial catalog and says an + // empty result is not proof it has none — an unlisted skill is still + // fetchable by URI — so "No skills" would be the tool asserting something + // the protocol explicitly does not. renderWithMantine(); - expect(screen.getByText("No skills")).toBeInTheDocument(); + expect(screen.getByText("No skills listed")).toBeInTheDocument(); + expect(screen.queryByText("No skills")).not.toBeInTheDocument(); }); it("renders a load failure above the list", () => { @@ -236,10 +241,13 @@ describe("SkillsScreen", () => { const user = userEvent.setup(); renderWithMantine(); await user.click(screen.getByText("data-analysis")); - const manifest = screen.getByTestId("skill-manifest"); - // One row at a time: the first row's own Verify button, not "Verify all". + // Addressed by its accessible name, which carries the URI — every row's + // visible text is just "Verify", so that name is what tells a + // screen-reader user (and this test) which file the button checks. await user.click( - within(manifest).getAllByRole("button", { name: "Verify" })[0], + screen.getByRole("button", { + name: "Verify skill://data-analysis/reference.md", + }), ); expect(await screen.findByText("verified")).toBeInTheDocument(); }); @@ -374,10 +382,9 @@ describe("SkillsScreen", () => { />, ); await user.click(screen.getByText("data-analysis")); - const manifest = screen.getByTestId("skill-manifest"); - const rowVerify = within(manifest).getAllByRole("button", { - name: "Verify", - })[0]; + const rowVerify = screen.getByRole("button", { + name: "Verify skill://data-analysis/SKILL.md", + }); await user.click(rowVerify); await user.click(rowVerify); expect(resolvers).toHaveLength(2); @@ -780,6 +787,68 @@ describe("SkillsScreen", () => { ).not.toBeInTheDocument(); }); + it("rejects an older preview read even when it resolves FIRST", async () => { + // The ordering hole: recording an attempt only when it settles leaves a + // window where the older request is still considered current. Claiming it + // before the request goes out is what makes the older callback stale + // immediately, whatever order the two resolve in. + const user = userEvent.setup(); + const resolvers: ((value: { text: string }) => void)[] = []; + const onReadSkillFile = vi.fn( + () => + new Promise<{ text: string }>((resolve) => { + resolvers.push(resolve); + }), + ); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + const view = screen.getByRole("button", { name: /View SKILL.md/ }); + await user.click(view); + await user.click(view); + expect(resolvers).toHaveLength(2); + + // The OLDER read answers first, while the newer one is still in flight. + resolvers[0]({ text: "# stale\n" }); + expect(screen.queryByTestId("skill-md-preview")).not.toBeInTheDocument(); + resolvers[1]({ text: "# newest\n" }); + expect(await screen.findByTestId("skill-md-preview")).toHaveTextContent( + "newest", + ); + }); + + it("rejects an older skills/get even when it resolves FIRST", async () => { + const user = userEvent.setup(); + const resolvers: ((value: SkillEntry) => void)[] = []; + const onGetSkill = vi.fn( + () => + new Promise((resolve) => { + resolvers.push(resolve); + }), + ); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + const fetchButton = screen.getByRole("button", { + name: /Fetch with skills\/get/, + }); + await user.click(fetchButton); + await user.click(fetchButton); + expect(resolvers).toHaveLength(2); + + // The older fetch answers first with a differing entry; it must not + // publish a verdict while the newer one is pending. + resolvers[0]({ + ...CLEAN_SKILL, + frontmatter: { ...CLEAN_SKILL.frontmatter, description: "stale" }, + }); + expect(screen.queryByTestId("skills-get-result")).not.toBeInTheDocument(); + resolvers[1](CLEAN_SKILL); + expect( + await screen.findByText("skills/get matches skills/list"), + ).toBeInTheDocument(); + }); + it("shows the SKILL.md preview on demand", async () => { const user = userEvent.setup(); renderWithMantine(); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 35a39f1f..909625ac 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -529,6 +529,13 @@ export function SkillsScreen({ setPreviewState((prev) => isStale(prev, key, attempt) ? prev : { key, attempt, ...next }, ); + // Claimed BEFORE the request goes out, the way `verifyRow` claims its row. + // Recording the attempt only on settle leaves a window where an older + // request that happens to resolve first is still considered current, and + // publishes its contents while a newer one is in flight. Clearing the + // previous result at the same time also means the pane doesn't keep + // showing the old file while the new read is running. + writePreview({}); void onReadSkillFile(selected.uri) .then((contents) => writePreview({ contents })) .catch((err: unknown) => { @@ -549,6 +556,9 @@ export function SkillsScreen({ setFetchedEntry((prev) => isStale(prev, key, attempt) ? prev : { key, attempt, ...next }, ); + // Claimed before the request goes out — see `showSkillMd` for why settling + // is too late. + writeFetched({}); void onGetSkill(selected.uri) .then((entry) => { // The fetched entry is checked ON ITS OWN before being compared. A @@ -628,7 +638,12 @@ export function SkillsScreen({ )} {filtered.length === 0 ? ( - No skills + // Not "No skills": SEP-2640 lets a server return an empty or + // partial catalog, and says an empty result must not be read as + // proof it has none — an unlisted skill can still be fetched by + // URI with `skills/get`. Claiming otherwise would be the tool + // asserting something the protocol explicitly does not. + No skills listed ) : ( filtered.map((skill) => { const skillIssues = checkSkillConformance(skill); @@ -783,8 +798,14 @@ export function SkillsScreen({ {verificationLabel(state)} void verifyRow(index, resource, manifestKey) 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 779fdaf6..989a1448 100644 --- a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi } from "vitest"; import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; import type { ServerCapabilities } from "@modelcontextprotocol/client"; import { SKILLS_EXTENSION_KEY } from "@inspector/core/mcp/skillsSchemas.js"; +import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client"; /** * Unit coverage for the Skills extension methods (#2234, SEP-2640). @@ -181,6 +182,52 @@ describe("InspectorClient skills methods (#2234)", () => { }); }); + it("attributes a rejected skills/get envelope to its Protocol entry", async () => { + // Without this the Skills screen shows an error while the Protocol tab + // renders the same exchange as a clean success. Done in the client rather + // than a store because `skills/get` has none — the screen calls it. + const client = makeClient(); + const marked: [string, string][] = []; + ( + client as unknown as { + markResponseRejected: (m: string, r: string) => void; + } + ).markResponseRejected = (method, reason) => { + marked.push([method, reason]); + }; + internals(client).client = { + request: async () => { + throw new SdkError( + SdkErrorCode.InvalidResult, + "Invalid result for skills/get", + ); + }, + }; + await expect(client.getSkill("skill://demo/SKILL.md")).rejects.toThrow(); + expect(marked).toEqual([["skills/get", "Invalid result for skills/get"]]); + }); + + it("does NOT attribute a transport failure on skills/get", async () => { + // No response frame arrived, so the last-answered id still points at an + // earlier, successful exchange; marking it would stamp that one. + const client = makeClient(); + const marked: string[] = []; + ( + client as unknown as { + markResponseRejected: (m: string, r: string) => void; + } + ).markResponseRejected = (method) => { + marked.push(method); + }; + internals(client).client = { + request: async () => { + throw new SdkError(SdkErrorCode.ConnectionClosed, "Connection closed"); + }, + }; + await expect(client.getSkill("skill://demo/SKILL.md")).rejects.toThrow(); + expect(marked).toEqual([]); + }); + it("rejects a skills/list result that is not a skills page", async () => { // The explicit result schema is the whole client-side mechanism for a // consumer-owned extension method, so a nonconforming result must fail diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 70b95eb7..7b2964f2 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -203,6 +203,7 @@ import { LIST_MAX_PAGES, ModernResultEnvelopeSchema, isSalvageableRejection, + isClientDecodeRejection, listPaginationExceeded, toolItemSchemaForEra, nextCursorOf, @@ -5603,15 +5604,33 @@ export class InspectorClient extends InspectorClientEventTarget { }; // `GetSkillResultSchema` unwraps the envelope, so there is nothing to // unwrap here. - return this.invokeMcpClient( - () => - this.client!.request( - { method: SKILLS_GET_METHOD, params }, - GetSkillResultSchema, - this.getRequestOptions(this.progressTokenOf(metadata)), - ), - { method: SKILLS_GET_METHOD }, - ); + try { + return await this.invokeMcpClient( + () => + this.client!.request( + { method: SKILLS_GET_METHOD, params }, + GetSkillResultSchema, + this.getRequestOptions(this.progressTokenOf(metadata)), + ), + { method: SKILLS_GET_METHOD }, + ); + } catch (err) { + // Attribute a rejected envelope to the exchange it came from, so the + // Protocol tab stops rendering it as a clean success while the Skills + // screen shows an error — the same handling every managed list gets + // (#1953). Done here rather than in a store because `skills/get` has + // none: the screen calls it directly. Must happen in this catch, while + // the correlation window is still current, and ONLY for a decode + // rejection — a request that never produced a response would otherwise + // stamp an earlier, successful exchange. + if (isClientDecodeRejection(err)) { + this.markResponseRejected( + SKILLS_GET_METHOD, + err instanceof Error ? err.message : String(err), + ); + } + throw err; + } } /**