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;
+ }
}
/**