fix: address Copilot review round 15 on #2251
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
11ac134975
commit
a561cd1daa
4 changed files with 176 additions and 20 deletions
|
|
@ -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(<SkillsScreen {...baseProps} skills={[]} />);
|
||||
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(<ControlledSkillsScreen />);
|
||||
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(
|
||||
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
|
||||
);
|
||||
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<SkillEntry>((resolve) => {
|
||||
resolvers.push(resolve);
|
||||
}),
|
||||
);
|
||||
renderWithMantine(<ControlledSkillsScreen onGetSkill={onGetSkill} />);
|
||||
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(<ControlledSkillsScreen />);
|
||||
|
|
|
|||
|
|
@ -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({
|
|||
</Alert>
|
||||
)}
|
||||
{filtered.length === 0 ? (
|
||||
<EmptyState>No skills</EmptyState>
|
||||
// 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.
|
||||
<EmptyState>No skills listed</EmptyState>
|
||||
) : (
|
||||
filtered.map((skill) => {
|
||||
const skillIssues = checkSkillConformance(skill);
|
||||
|
|
@ -783,8 +798,14 @@ export function SkillsScreen({
|
|||
{verificationLabel(state)}
|
||||
</CountBadge>
|
||||
<RowVerifyButton
|
||||
// Every row's button reads "Verify", so the
|
||||
// visible text alone gives a screen-reader
|
||||
// user no way to tell which file each one
|
||||
// checks; the URI cell is in the same row but
|
||||
// is not programmatically associated with it.
|
||||
aria-label={`Verify ${resource.uri}`}
|
||||
// A click handler cannot await, and
|
||||
// `verifyFile` owns its own failures — it
|
||||
// `verifyRow` owns its own failures — it
|
||||
// records them as this row's state.
|
||||
onClick={() =>
|
||||
void verifyRow(index, resource, manifestKey)
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Reference in a new issue