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 2 on #2251

Browse source 
- SkillsScreen: key verdicts and React elements by manifest ROW INDEX,
  not URI. The checker deliberately tolerates a duplicated URI so it can
  report `duplicate-resource`; a URI key collided those rows into one
  verdict, so verifying either updated both and "Verify all" raced two
  different digest/size declarations into the same slot.
- SkillsScreen: hold the invalidation generation in React state keyed by
  the manifest, not a ref bumped during render. `useValueChange` runs its
  callback during render and requires setState-only purity — an abandoned
  render left the ref incremented and silently discarded a live
  verification.
- SkillsScreen: title a size disagreement "Size mismatch" and render its
  reason. `verifySkillResource` catches it before hashing, so the alert
  was showing "actual undefined" under "Digest mismatch".
- SkillsScreen: pass `contents` to ContentViewer so a base64 SKILL.md
  renders. The text-block form substituted "" and painted a blank preview
  for a file verification had just read correctly.
- skills.ts: add `malformed-size` for a size that is not a non-negative
  safe integer, and exclude such values from the 16 MiB total. A negative
  one could pull the sum back under the limit and hide a violation.
- managedSkillsState: make the overlap guard per-session instead of a
  boolean. A disconnect during an in-flight walk left it set, so the
  reconnect's own load no-oped and was never retried — permanently, if
  the stale request never settled. A stale `finally` can no longer clear
  the live session's guard either.
- skillsSchemas: require the `{ skill }` envelope for `skills/get`. The
  accepted SEP settles it, and normalizing an inline entry would let a
  non-conforming response past the one place that could report it.
- #2248 and the PR description: corrected — they said
  `ReadResourceDirectoryResultSchema` was already declared, which round 1
  removed. #2248 now owns defining it, and records the frontmatter
  cross-check gap too.

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 21:45:49 -04:00
commit bfeebd440d
6 changed files with 406 additions and 143 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -259,6 +259,82 @@ describe("SkillsScreen", () => {
expect(await screen.findAllByText("plain string")).toHaveLength(2);
});
it("titles a size disagreement a size mismatch, not a digest one", async () => {
// `verifySkillResource` catches a size disagreement BEFORE hashing, so
// there is no `actualDigest` — labelling it "Digest mismatch" would render
// "actual undefined" and hide the real failure.
const user = userEvent.setup();
renderWithMantine(
<ControlledSkillsScreen
skills={[
{
...CLEAN_SKILL,
resources: [{ uri: "skill://data-analysis/SKILL.md", size: 9999 }],
},
]}
/>,
);
await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /Verify all/ }));
expect(await screen.findByText("Size mismatch")).toBeInTheDocument();
expect(screen.queryByText("Digest mismatch")).not.toBeInTheDocument();
// The alert states both lengths; the manifest row also shows the declared
// one, hence `getAllByText`.
expect(screen.getAllByText(/9999 bytes/).length).toBeGreaterThan(0);
});
it("gives duplicated manifest URIs their own row and their own verdict", async () => {
// The conformance checker reports `duplicate-resource` rather than
// collapsing the rows, so the verdicts must not collapse either: the two
// entries declare different digests and only one of them is right.
const user = userEvent.setup();
renderWithMantine(
<ControlledSkillsScreen
skills={[
{
...CLEAN_SKILL,
resources: [
{
uri: "skill://data-analysis/SKILL.md",
digest: SELF_DIGEST,
size: textToBytes(SELF_TEXT).byteLength,
},
{
uri: "skill://data-analysis/SKILL.md",
digest: `sha256:${"d".repeat(64)}`,
size: textToBytes(SELF_TEXT).byteLength,
},
],
},
]}
/>,
);
await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /Verify all/ }));
// One row verifies and the other does not — a shared key would have made
// both show whichever landed last.
expect(await screen.findByText("verified")).toBeInTheDocument();
expect(screen.getByText("mismatch")).toBeInTheDocument();
});
it("renders a base64 SKILL.md preview instead of a blank one", async () => {
// `onReadSkillFile` supports blob content, and verification reads it
// correctly; dropping it in the preview would paint an empty box for a
// file the screen had just checked.
const user = userEvent.setup();
const onReadSkillFile = vi.fn().mockResolvedValue({
blob: btoa("# from a blob\n"),
mimeType: "text/markdown",
});
renderWithMantine(
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
);
await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /View SKILL.md/ }));
const preview = await screen.findByTestId("skill-md-preview");
expect(preview).toHaveTextContent("from a blob");
});
it("shows the SKILL.md preview on demand", async () => {
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);

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

@ -1,4 +1,4 @@
import { useCallback, useMemo, useRef, useState } from "react";
import { useCallback, useMemo, useState } from "react";
import {
Alert,
Badge,
@ -42,12 +42,37 @@ import {
*/
const VERIFY_CONCURRENCY = 4;
/** Per-file verification progress, keyed by the manifest entry's URI. */
/** Per-row verification progress. */
type FileState =
| { status: "pending" }
| { status: "done"; verification: SkillVerification }
| { status: "error"; message: string };
/**
* Verification verdicts plus the manifest they belong to. Rows are keyed by
* their **index**, not their URI: the conformance checker deliberately tolerates
* a duplicated URI so it can report `duplicate-resource`, and a URI key would
* collide those two rows into one verdict.
*/
interface VerificationState {
/**
* The manifest these verdicts belong to, or `null` before anything has been
* verified. `useValueChange` deliberately does not fire on the first render,
* so `null` stands in for "the initial manifest, not yet adopted" — the first
* write claims it. Once set it is only ever replaced by an invalidation, so a
* stale continuation can never be mistaken for an initial one.
*/
key: string | null;
files: Record<number, FileState>;
}
/** The SKILL.md preview, plus the manifest it belongs to (`null` as above). */
interface PreviewState {
key: string | null;
contents?: SkillFileContents;
message?: string;
}
export interface SkillsScreenProps {
skills: SkillEntry[];
/** Pages the last `skills/list` walk took; shown so pagination is visible. */
@ -239,15 +264,19 @@ export function SkillsScreen({
onReadSkillFile,
}: SkillsScreenProps) {
const { selectedSkillUri, search } = ui;
const [fileStates, setFileStates] = useState<Record<string, FileState>>({});
const [preview, setPreview] = useState<SkillFileContents | null>(null);
const [previewError, setPreviewError] = useState<string | null>(null);
// Bumped every time the verdicts are invalidated. A read that was already in
// flight captures the value it started under and discards its result when
// this has moved on — otherwise a slow fetch for the previous selection (or
// the previous manifest) lands afterwards and writes a verdict for content
// nobody is looking at, or worse, one that was never checked.
const epoch = useRef(0);
// Both slices carry the manifest key they belong to, and every async
// continuation writes through a functional update that compares it. That is
// what discards a read still in flight when the selection changes or a
// Refresh replaces the manifest — without it, a slow fetch lands afterwards
// and writes a verdict for content nobody is looking at, or one that was
// never checked. Storing the key IN the state (rather than bumping a ref
// during render) keeps the `useValueChange` callback to `setState` calls
// only, which is the purity that hook documents and requires.
const [verification, setVerification] = useState<VerificationState>({
key: null,
files: {},
});
const [previewState, setPreviewState] = useState<PreviewState>({ key: null });
const filtered = useMemo(() => {
const needle = search.trim().toLowerCase();
@ -295,43 +324,45 @@ export function SkillsScreen({
// Adjusted DURING RENDER via `useValueChange` rather than in an effect, so a
// new selection (or a refreshed manifest) never paints a frame carrying the
// previous one's verification results. The epoch bump is a ref write, which
// is why it is done in the callback alongside the state resets rather than
// during the render body itself.
useValueChange(manifestKey, () => {
epoch.current += 1;
setFileStates({});
setPreview(null);
setPreviewError(null);
// previous one's verification results. `setState` calls only — the hook
// replays this callback whenever React replays the render.
useValueChange(manifestKey, (next) => {
setVerification({ key: next, files: {} });
setPreviewState({ key: next });
});
const verifyFile = useCallback(
async (resource: SkillResource) => {
const started = epoch.current;
setFileStates((prev) => ({
...prev,
[resource.uri]: { status: "pending" },
}));
const fileStates = verification.key === manifestKey ? verification.files : {};
/**
* Verify one manifest ROW. Keyed by row index, not by URI: the checker
* deliberately tolerates a duplicated URI so it can report
* `duplicate-resource`, and two rows sharing a key would share one verdict —
* verifying either would update both, and "Verify all" would race two
* different digest/size declarations into the same slot.
*/
const verifyRow = useCallback(
async (index: number, resource: SkillResource, key: string) => {
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 : {};
return { key, files: { ...files, [index]: state } };
});
write({ status: "pending" });
try {
const contents = await onReadSkillFile(resource.uri);
const verification = await verifySkillResource(
const result = await verifySkillResource(
resource,
skillFileBytes(contents),
);
if (epoch.current !== started) return;
setFileStates((prev) => ({
...prev,
[resource.uri]: { status: "done", verification },
}));
write({ status: "done", verification: result });
} catch (err) {
if (epoch.current !== started) return;
setFileStates((prev) => ({
...prev,
[resource.uri]: {
status: "error",
message: err instanceof Error ? err.message : String(err),
},
}));
write({
status: "error",
message: err instanceof Error ? err.message : String(err),
});
}
},
[onReadSkillFile],
@ -348,34 +379,42 @@ export function SkillsScreen({
// records them as per-row state), and this handler cannot be async, so the
// settled promise is discarded explicitly at one place instead of per file.
let next = 0;
const key = manifestKey;
const worker = async (): Promise<void> => {
for (let i = next++; i < manifest.length; i = next++) {
await verifyFile(manifest[i]);
await verifyRow(i, manifest[i], key);
}
};
const workers = Math.min(VERIFY_CONCURRENCY, manifest.length);
void Promise.all(Array.from({ length: workers }, () => worker()));
}, [manifest, verifyFile]);
}, [manifest, manifestKey, verifyRow]);
const showSkillMd = useCallback(() => {
if (!selected) return;
const started = epoch.current;
const key = manifestKey;
// A click handler cannot await, and this chain terminates in its own
// `catch` that surfaces the message in the preview slot. Both arms are
// epoch-guarded: a read that resolves after the selection moved on would
// otherwise show one skill's SKILL.md under another's heading.
// `catch` that surfaces the message in the preview slot. Both arms compare
// the manifest key they started under: a read that resolves after the
// selection moved on would otherwise show one skill's SKILL.md under
// another's heading.
void onReadSkillFile(selected.uri)
.then((contents) => {
if (epoch.current !== started) return;
setPreview(contents);
setPreviewError(null);
setPreviewState((prev) =>
prev.key !== null && prev.key !== key ? prev : { key, contents },
);
})
.catch((err: unknown) => {
if (epoch.current !== started) return;
setPreview(null);
setPreviewError(err instanceof Error ? err.message : String(err));
const message = err instanceof Error ? err.message : String(err);
setPreviewState((prev) =>
prev.key !== null && prev.key !== key ? prev : { key, message },
);
});
}, [onReadSkillFile, selected]);
}, [manifestKey, onReadSkillFile, selected]);
const preview =
previewState.key === manifestKey ? previewState.contents : undefined;
const previewError =
previewState.key === manifestKey ? previewState.message : undefined;
const errorCount = issues.filter((i) => i.severity === "error").length;
const warningCount = issues.length - errorCount;
@ -527,8 +566,8 @@ export function SkillsScreen({
</Table.Tr>
</Table.Thead>
<Table.Tbody>
{manifest.map((resource) => {
const state = fileStates[resource.uri];
{manifest.map((resource, index) => {
const state = fileStates[index];
const color =
state?.status === "done"
? verificationColor(state.verification.status)
@ -536,7 +575,10 @@ export function SkillsScreen({
? "red"
: "gray";
return (
<Table.Tr key={resource.uri}>
// Index-keyed for the same reason the verdicts are:
// a duplicated URI is a case this screen reports, so
// it must not also collide two rows into one.
<Table.Tr key={index}>
<Table.Td>{resource.uri}</Table.Td>
<Table.Td>{resource.size ?? "—"}</Table.Td>
<Table.Td>{shortDigest(resource.digest)}</Table.Td>
@ -549,7 +591,9 @@ export function SkillsScreen({
// A click handler cannot await, and
// `verifyFile` owns its own failures — it
// records them as this row's state.
onClick={() => void verifyFile(resource)}
onClick={() =>
void verifyRow(index, resource, manifestKey)
}
>
Verify
</RowVerifyButton>
@ -561,35 +605,45 @@ export function SkillsScreen({
</Table.Tbody>
</ManifestTable>
)}
{manifest.map((resource) => {
const state = fileStates[resource.uri];
{manifest.map((resource, index) => {
const state = fileStates[index];
if (state?.status === "done") {
const { verification } = state;
if (verification.status === "mismatch") {
return (
<Alert
key={`mismatch:${resource.uri}`}
color="red"
title="Digest mismatch"
>
<Stack gap={2}>
<MonoCaption>{resource.uri}</MonoCaption>
<MonoCaption>
expected {verification.expectedDigest}
</MonoCaption>
<MonoCaption>
actual {verification.actualDigest}
</MonoCaption>
</Stack>
</Alert>
);
}
return null;
const result = state.verification;
if (result.status !== "mismatch") return null;
// A size disagreement is caught BEFORE hashing, so it has
// no `actualDigest` — titling it "Digest mismatch" and
// rendering "actual undefined" would hide the real failure.
const sizeFailure = result.actualDigest === undefined;
return (
<Alert
key={`mismatch:${index}`}
color="red"
title={
sizeFailure ? "Size mismatch" : "Digest mismatch"
}
>
<Stack gap={2}>
<MonoCaption>{resource.uri}</MonoCaption>
{sizeFailure ? (
<Text size="sm">{result.reason}</Text>
) : (
<>
<MonoCaption>
expected {result.expectedDigest}
</MonoCaption>
<MonoCaption>
actual {result.actualDigest}
</MonoCaption>
</>
)}
</Stack>
</Alert>
);
}
if (state?.status === "error") {
return (
<Alert
key={`read:${resource.uri}`}
key={`read:${index}`}
color="red"
title="Could not read file"
>
@ -612,9 +666,24 @@ export function SkillsScreen({
{preview && (
<Stack gap="xs" data-testid="skill-md-preview">
<SectionHeading>SKILL.md</SectionHeading>
{/* `contents`, not a text `block`: a server may serve
SKILL.md as a base64 `blob`, and the block form would
substitute an empty string and paint a blank preview for
a file verification just read correctly. */}
<ContentViewer
block={{ type: "text", text: preview.text ?? "" }}
mimeType={preview.mimeType ?? "text/markdown"}
contents={
typeof preview.text === "string"
? {
uri: selected.uri,
text: preview.text,
mimeType: preview.mimeType ?? "text/markdown",
}
: {
uri: selected.uri,
blob: preview.blob ?? "",
mimeType: preview.mimeType ?? "text/markdown",
}
}
copyable
/>
</Stack>

12
clients/web/src/test/core/mcp/inspectorClient-skills.test.ts
View file

@ -115,18 +115,20 @@ describe("InspectorClient skills methods (#2234)", () => {
expect(request.mock.calls[0][0].params.uri).toBe("skill://demo/SKILL.md");
});
it("normalizes the enveloped skills/get result to the entry", async () => {
it("unwraps the skills/get envelope to the entry", async () => {
const client = makeClient();
stubRequest(client, { skill: ENTRY });
expect(await client.getSkill("skill://demo/SKILL.md")).toEqual(ENTRY);
});
it("normalizes the inline skills/get result to the entry", async () => {
// The SEP settles the entry shape but not the envelope; a server that
// returns the entry at the top level must not fail here.
it("rejects a skills/get result returned without its envelope", async () => {
// Normalizing it would let a non-conforming server through the one place
// that could have reported it.
const client = makeClient();
stubRequest(client, ENTRY);
expect(await client.getSkill("skill://demo/SKILL.md")).toEqual(ENTRY);
await expect(
client.getSkill("skill://demo/SKILL.md"),
).rejects.toBeDefined();
});
it("rejects a skills/list result that is not a skills page", async () => {

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

@ -295,6 +295,39 @@ describe("checkSkillConformance", () => {
}
});
it("reports a size that is not a non-negative integer byte length", () => {
for (const size of [-1, 1.5, Number.NaN, Number.MAX_SAFE_INTEGER + 2]) {
const issues = checkSkillConformance(
entry({
resources: [
{ uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 },
{ uri: "skill://demo/ref.md", digest: DIGEST, size },
],
}),
);
expect(issues.map((i) => i.code)).toEqual(["malformed-size"]);
expect(issues[0].severity).toBe("error");
}
});
it("a negative size cannot pull the total back under the 16 MiB limit", () => {
// The reason `malformed-size` is an error and not just noise: summing a
// negative would hide a genuine `size-limit-exceeded`.
const issues = checkSkillConformance(
entry({
resources: [
{
uri: "skill://demo/SKILL.md",
digest: DIGEST,
size: SKILL_MAX_TOTAL_BYTES + 1,
},
{ uri: "skill://demo/ref.md", digest: DIGEST, size: -1000 },
],
}),
);
expect(issues.map((i) => i.code)).toContain("size-limit-exceeded");
});
it("reports a manifest over the 512-entry limit", () => {
const resources = [
{ uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 },
@ -350,6 +383,20 @@ describe("totalSkillBytes", () => {
]),
).toBe(15);
});
it("excludes an unusable size rather than summing it", () => {
// An incomplete manifest may only ever *understate* the total, which is
// what keeps the limit check free of false positives. A negative or
// fractional value would break that.
expect(
totalSkillBytes([
{ uri: "a", size: 10 },
{ uri: "b", size: -100 },
{ uri: "c", size: 2.5 },
{ uri: "d", size: Number.NaN },
]),
).toBe(10);
});
});
describe("byte helpers", () => {

21
clients/web/src/test/core/mcp/skillsSchemas.test.ts
View file

@ -7,7 +7,6 @@ import {
SKILLS_GET_METHOD,
SKILLS_LIST_METHOD,
SkillEntrySchema,
normalizeGetSkillResult,
} from "@inspector/core/mcp/skillsSchemas";
const ENTRY = {
@ -89,20 +88,24 @@ describe("ListSkillsResultSchema", () => {
});
describe("GetSkillResultSchema", () => {
it("normalizes the enveloped form to the entry", () => {
it("unwraps the envelope to the entry", () => {
expect(GetSkillResultSchema.parse({ skill: ENTRY })).toEqual(ENTRY);
});
it("normalizes the inline form to the entry", () => {
expect(GetSkillResultSchema.parse(ENTRY)).toEqual(ENTRY);
});
it("normalizeGetSkillResult accepts either shape directly", () => {
expect(normalizeGetSkillResult({ skill: ENTRY })).toEqual(ENTRY);
expect(normalizeGetSkillResult(ENTRY)).toEqual(ENTRY);
it("rejects an entry returned inline rather than normalizing it", () => {
// The envelope is required. Accepting the inline form would silently
// normalize a non-conforming response, which is the failure this
// extension's support exists to report.
expect(() => GetSkillResultSchema.parse(ENTRY)).toThrow();
});
it("rejects a result that is neither shape", () => {
expect(() => GetSkillResultSchema.parse({ nothing: true })).toThrow();
});
it("rejects an envelope whose skill is not an entry", () => {
expect(() =>
GetSkillResultSchema.parse({ skill: { frontmatter: {} } }),
).toThrow();
});
});

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

@ -142,6 +142,52 @@ describe("ManagedSkillsState", () => {
expect(state.getPagination()).toEqual({ pageCount: 0 });
});
it("lets a reconnect load its skills while a stale walk is still hanging", async () => {
client.setStatus("connected");
// The first walk never settles. A boolean overlap guard would stay set,
// so the reconnect's own load would no-op and never be retried — the
// reconnect would show an empty Skills tab forever.
client.listSkills.mockImplementationOnce(() => new Promise(() => {}));
void state.refresh().catch(() => {});
await client.disconnect();
client.skillPages = [{ skills: [skill("fresh")] }];
await client.connect();
expect(state.getSkills().map((s) => s.frontmatter.name)).toEqual(["fresh"]);
});
it("a stale walk settling later cannot release the live session's guard", async () => {
client.setStatus("connected");
let release: ((value: { skills: SkillEntry[] }) => void) | undefined;
client.listSkills.mockImplementationOnce(
async () =>
new Promise((resolve) => {
release = resolve;
}),
);
const stale = state.refresh();
await client.disconnect();
client.setStatus("connected");
// A live walk is now in flight under the new generation.
let releaseLive: ((value: { skills: SkillEntry[] }) => void) | undefined;
client.listSkills.mockImplementationOnce(
async () =>
new Promise((resolve) => {
releaseLive = resolve;
}),
);
const live = state.refresh();
// The stale one settles first; its `finally` must not free the guard.
release?.({ skills: [] });
await stale;
const blocked = await state.refresh();
expect(blocked).toEqual([]);
releaseLive?.({ skills: [skill("live")] });
await live;
expect(state.getSkills().map((s) => s.frontmatter.name)).toEqual(["live"]);
});
it("does not surface a dead session's failure in the live one", async () => {
client.setStatus("connected");
let fail: ((err: Error) => void) | undefined;