modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: address Copilot review round 3 on #2251

Browse source 
- skills.ts: decide root containment on NORMALIZED URIs. A raw prefix
  check passed `skill://acme/billing/refunds/../other.md`, which starts
  with the advertised root but resolves outside it — a traversal the
  conformance report was reporting as clean.
- skills.ts: `normalizeSkillUri` also rejects a relative string, so
  `demo/SKILL.md` is `malformed-uri` rather than a skill path, and an
  opaque-path URI (`skill:demo/..`), which the parser leaves un-normalized
  and on which containment cannot be decided.
- skills.ts: `missing-description` is an error. SEP-2640 requires it, so
  an absent one must not read as "0 errors" in the conformance summary.
- managedSkillsState: call `markResponseRejected` for a decode rejection,
  as every managed list does. An invalid `skills/list` result was showing
  in the Protocol tab as a clean success.
- SkillsScreen: per-row attempt token. The manifest key cannot tell two
  verifications of the SAME row apart, so a double click (or a row button
  pressed during "Verify all") let an older read finish last and overwrite
  the newer verdict.
- SkillsScreen: disable "Verify all" while a batch runs. The concurrency
  cap is per invocation, so repeated clicks stacked pools — 4, then 8,
  then 12.
- SkillsScreen: include the finding index in each issue alert's key. Three
  identical URIs produce two `duplicate-resource` findings with the same
  code and URI, and React was free to drop the extras — hiding findings in
  exactly the malformed input this view exists to inspect.
- test-servers/skills.ts: register `skills/list` and `skills/get` through
  the PUBLIC `setRequestHandler` with explicit param schemas. The private
  `_requestHandlers` map is now reached only to wrap `resources/read`,
  which has to chain onto the SDK's handler rather than replace it — the
  one thing the public API cannot express, and the comment now says so
  instead of citing the tasks fixture.
- test-servers/skills.ts: raise `-32602` for an unknown `skills/get` URI.
  A plain Error mapped to a generic server failure, making the fixture
  non-conforming outside its three documented bad cases.
- clients/web/README.md: the paragraph named four contracts while still
  saying the smoke "drives all three".

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 2026-09-04 22:06:36 -04:00
commit 6a08c77c92
4 changed files with 376 additions and 59 deletions
Download patch file Download diff file Expand all files Collapse all files

95
clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx
View file

@ -6,6 +6,7 @@ import { sha256Digest, textToBytes } from "@inspector/core/mcp/skills";
import {
renderWithMantine,
screen,
waitFor,
within,
} from "../../../test/renderWithMantine";
import {
@ -335,6 +336,100 @@ describe("SkillsScreen", () => {
expect(preview).toHaveTextContent("from a blob");
});
it("keeps the newest verdict when two verifications of one row overlap", async () => {
// Same row, same manifest — so the manifest key cannot tell these apart.
// Without a per-row attempt token the older read finishing last would
// overwrite the newer verdict and leave the UI reporting stale bytes.
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}
skills={[
{
...CLEAN_SKILL,
resources: [
{
uri: "skill://data-analysis/SKILL.md",
digest: SELF_DIGEST,
size: textToBytes(SELF_TEXT).byteLength,
},
],
},
]}
/>,
);
await user.click(screen.getByText("data-analysis"));
const manifest = screen.getByTestId("skill-manifest");
const rowVerify = within(manifest).getAllByRole("button", {
name: "Verify",
})[0];
await user.click(rowVerify);
await user.click(rowVerify);
expect(resolvers).toHaveLength(2);
// The SECOND read answers first with the matching bytes, then the first
// read answers with bytes that would verify as a mismatch.
resolvers[1]({ text: SELF_TEXT });
expect(await screen.findByText("verified")).toBeInTheDocument();
resolvers[0]({ text: "stale bytes\n" });
// Still the newer verdict.
expect(await screen.findByText("verified")).toBeInTheDocument();
expect(screen.queryByText("mismatch")).not.toBeInTheDocument();
});
it("disables Verify all while a batch is running", async () => {
// The concurrency cap is per invocation, so a second click would start a
// second pool of four rather than reusing the first.
const user = userEvent.setup();
const pending: ((value: { text: string }) => void)[] = [];
const onReadSkillFile = vi.fn(
() =>
new Promise<{ text: string }>((resolve) => {
pending.push(resolve);
}),
);
renderWithMantine(
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
);
await user.click(screen.getByText("data-analysis"));
const verifyAll = screen.getByRole("button", { name: /Verify all/ });
await user.click(verifyAll);
expect(verifyAll).toBeDisabled();
// Release every read the batch started; the button frees only once the
// whole batch settles, not once the first file does.
await waitFor(() => expect(pending.length).toBeGreaterThan(0));
for (const resolve of pending) resolve({ text: SELF_TEXT });
await waitFor(() => expect(verifyAll).not.toBeDisabled());
});
it("renders every duplicate finding rather than collapsing them", async () => {
// Three identical URIs produce two `duplicate-resource` findings with the
// same code and URI. A key built from those alone would make React drop
// the extras — hiding findings in exactly the malformed input this view is
// for.
const user = userEvent.setup();
const dup = {
uri: "skill://data-analysis/SKILL.md",
digest: SELF_DIGEST,
size: textToBytes(SELF_TEXT).byteLength,
};
renderWithMantine(
<ControlledSkillsScreen
skills={[{ ...CLEAN_SKILL, resources: [dup, dup, dup] }]}
/>,
);
await user.click(screen.getByText("data-analysis"));
const issues = screen.getByTestId("skill-issues");
expect(within(issues).getAllByText("duplicate-resource")).toHaveLength(2);
});
it("shows the SKILL.md preview on demand", async () => {
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);

54
clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx
View file

@ -1,4 +1,4 @@
import { useCallback, useMemo, useState } from "react";
import { useCallback, useMemo, useRef, useState } from "react";
import {
Alert,
Badge,
@ -42,11 +42,18 @@ import {
*/
const VERIFY_CONCURRENCY = 4;
/** Per-row verification progress. */
type FileState =
/**
* Per-row verification progress. `attempt` is the click that produced it: two
* verifications of the SAME row in the SAME manifest (a double click, or a row
* button pressed while "Verify all" is running) are not distinguished by the
* manifest key, so without it an older read finishing last would overwrite the
* newer verdict and leave the UI reporting bytes it no longer fetched.
*/
type FileState = { attempt: number } & (
| { status: "pending" }
| { status: "done"; verification: SkillVerification }
| { status: "error"; message: string };
| { status: "error"; message: string }
);
/**
* Verification verdicts plus the manifest they belong to. Rows are keyed by
@ -277,6 +284,12 @@ export function SkillsScreen({
files: {},
});
const [previewState, setPreviewState] = useState<PreviewState>({ key: null });
// True while a "Verify all" batch is in flight; disables the button so a
// second click cannot stack another pool of workers on top.
const [batchRunning, setBatchRunning] = useState(false);
// Monotonic per-row attempt token. A ref because it is claimed inside an
// event handler, never during render.
const nextAttempt = useRef(0);
const filtered = useMemo(() => {
const needle = search.trim().toLowerCase();
@ -342,24 +355,32 @@ export function SkillsScreen({
*/
const verifyRow = useCallback(
async (index: number, resource: SkillResource, key: string) => {
// Claimed synchronously, so two verifications of this row are ordered
// before either read starts.
const attempt = (nextAttempt.current += 1);
const write = (state: FileState) =>
setVerification((prev) => {
// `null` is the un-adopted initial manifest; any other mismatch is a
// continuation from a manifest that has since been invalidated.
if (prev.key !== null && prev.key !== key) return prev;
const files = prev.key === key ? prev.files : {};
// A newer attempt for this row already wrote — an older read
// finishing last must not overwrite it.
const held = files[index];
if (held !== undefined && held.attempt > attempt) return prev;
return { key, files: { ...files, [index]: state } };
});
write({ status: "pending" });
write({ attempt, status: "pending" });
try {
const contents = await onReadSkillFile(resource.uri);
const result = await verifySkillResource(
resource,
skillFileBytes(contents),
);
write({ status: "done", verification: result });
write({ attempt, status: "done", verification: result });
} catch (err) {
write({
attempt,
status: "error",
message: err instanceof Error ? err.message : String(err),
});
@ -386,7 +407,13 @@ export function SkillsScreen({
}
};
const workers = Math.min(VERIFY_CONCURRENCY, manifest.length);
void Promise.all(Array.from({ length: workers }, () => worker()));
setBatchRunning(true);
// The concurrency cap is per invocation, so without the button being
// disabled below, a second click would start a second pool of four and a
// third would make it twelve — the flood the cap exists to prevent.
void Promise.all(Array.from({ length: workers }, () => worker())).finally(
() => setBatchRunning(false),
);
}, [manifest, manifestKey, verifyRow]);
const showSkillMd = useCallback(() => {
@ -509,9 +536,15 @@ export function SkillsScreen({
</Alert>
) : (
<IssueStack data-testid="skill-issues">
{issues.map((issue) => (
{issues.map((issue, index) => (
<Alert
key={`${issue.code}:${issue.resourceUri ?? ""}`}
// The index is load-bearing, not decoration: a manifest
// repeating one URI three times yields three
// `duplicate-resource` findings with identical code and
// URI, and a key built from those alone would make
// React drop the extras — hiding findings in exactly
// the malformed input this view exists to inspect.
key={`${index}:${issue.code}:${issue.resourceUri ?? ""}`}
color={issueColor(issue)}
title={issue.code}
>
@ -542,7 +575,8 @@ export function SkillsScreen({
</VerifyButton>
<VerifyButton
onClick={verifyAll}
disabled={manifest.length === 0}
disabled={manifest.length === 0 || batchRunning}
loading={batchRunning}
>
Verify all
</VerifyButton>

72
clients/web/src/test/core/mcp/skills.test.ts
View file

@ -9,6 +9,7 @@ import {
checkSkillConformance,
getSkillsExtension,
isSkillsExtensionSupported,
normalizeSkillUri,
sha256Digest,
skillDisplayName,
skillNameFromUri,
@ -112,11 +113,47 @@ describe("skillNameFromUri", () => {
expect(skillNameFromUri("SKILL.md")).toBeUndefined();
});
it("returns undefined for a relative string", () => {
// SEP-2640 requires a full resource URI; treating `demo/SKILL.md` as one
// would let a non-conforming entry report a name and pass the path check.
expect(skillNameFromUri("demo/SKILL.md")).toBeUndefined();
});
it("reads the name off the RESOLVED path, not the raw string", () => {
expect(skillNameFromUri("skill://acme/wrong/../demo/SKILL.md")).toBe(
"demo",
);
});
it("returns undefined when the segment before the suffix is empty", () => {
expect(skillNameFromUri("skill:///SKILL.md")).toBeUndefined();
});
});
describe("normalizeSkillUri", () => {
it("resolves traversal segments", () => {
expect(normalizeSkillUri("skill://acme/billing/refunds/../other.md")).toBe(
"skill://acme/billing/other.md",
);
});
it("rejects a relative string, which is not a resource URI", () => {
expect(normalizeSkillUri("demo/SKILL.md")).toBeUndefined();
});
it("rejects an opaque-path URI, which the parser does not normalize", () => {
// `skill:demo/../x.md` parses but keeps its `..` verbatim, so containment
// could not be decided on it — accepting it would reopen the hole.
expect(normalizeSkillUri("skill:demo/SKILL.md")).toBeUndefined();
});
it("leaves an already-normal URI alone", () => {
expect(normalizeSkillUri("skill://demo/SKILL.md")).toBe(
"skill://demo/SKILL.md",
);
});
});
describe("skillDisplayName", () => {
it("prefers the declared frontmatter name", () => {
expect(skillDisplayName(entry())).toBe("demo");
@ -148,12 +185,14 @@ describe("checkSkillConformance", () => {
expect(issues[0].severity).toBe("error");
});
it("reports a missing description as a warning", () => {
it("reports a missing description as an error", () => {
// SEP-2640 requires `description`, so an absent one is a format violation
// and must not read as "0 errors" in the conformance summary.
const issues = checkSkillConformance(
entry({ frontmatter: { name: "demo" } }),
);
expect(issues.map((i) => i.code)).toEqual(["missing-description"]);
expect(issues[0].severity).toBe("warning");
expect(issues[0].severity).toBe("error");
});
it("reports a URI that does not carry a skill path", () => {
@ -235,6 +274,35 @@ describe("checkSkillConformance", () => {
expect(issues[0].resourceUri).toBe("skill://demo/ref.md");
});
it("reports an entry that traverses out of the skill root", () => {
// The raw string starts with the root; the resolved path does not. A
// prefix check alone would miss this and report `Conforms`.
const issues = checkSkillConformance(
entry({
uri: "skill://demo/refunds/SKILL.md",
frontmatter: { name: "refunds", description: "d" },
resources: [
{ uri: "skill://demo/refunds/SKILL.md", digest: DIGEST, size: 1 },
{ uri: "skill://demo/refunds/../other.md", digest: DIGEST, size: 1 },
],
}),
);
expect(issues.map((i) => i.code)).toEqual(["resource-outside-skill-root"]);
});
it("reports an unparseable manifest entry as outside the root", () => {
// Nothing can establish that a non-URI is inside a root.
const issues = checkSkillConformance(
entry({
resources: [
{ uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 },
{ uri: "not a uri", digest: DIGEST, size: 1 },
],
}),
);
expect(issues.map((i) => i.code)).toEqual(["resource-outside-skill-root"]);
});
it("reports a manifest entry outside the skill root", () => {
const issues = checkSkillConformance(
entry({

31
clients/web/src/test/core/mcp/state/managedSkillsState.test.ts
View file

@ -6,6 +6,7 @@ import {
SKILLS_MAX_PAGES,
SKILLS_PAGE_LIMIT_MESSAGE,
} from "@inspector/core/mcp/state/managedSkillsState";
import { SdkError, SdkErrorCode } from "@modelcontextprotocol/client";
import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient";
function skill(name: string): SkillEntry {
@ -216,6 +217,36 @@ describe("ManagedSkillsState", () => {
expect(state.getError()).toBe(failure);
});
describe("Protocol-entry attribution", () => {
it("attributes a decode rejection to its skills/list Protocol entry", async () => {
// Without this the Protocol tab renders an invalid `skills/list` result
// as a clean success — the same gap every managed list closed in #1953.
client.setStatus("connected");
const rejection = new SdkError(
SdkErrorCode.InvalidResult,
"Invalid result for skills/list: skills required",
);
client.listSkills.mockRejectedValueOnce(rejection);
await expect(state.refresh()).rejects.toThrow(rejection);
expect(client.markResponseRejected).toHaveBeenCalledWith(
"skills/list",
rejection.message,
);
});
it("does NOT attribute a transport failure", async () => {
// No response frame arrived, so the last-answered id still points at an
// EARLIER successful call; marking it would stamp "Rejected by the
// Inspector" onto an exchange that worked.
client.setStatus("connected");
client.listSkills.mockRejectedValueOnce(
new SdkError(SdkErrorCode.ConnectionClosed, "Connection closed"),
);
await expect(state.refresh()).rejects.toThrow();
expect(client.markResponseRejected).not.toHaveBeenCalled();
});
});
it("wraps a non-Error rejection", async () => {
client.setStatus("connected");
client.listSkills.mockRejectedValueOnce("just a string");