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 7 on #2248

Browse source 
Four findings.

**The CLI dropped what it promised to print.** `getSkill` transforms the
`skills/get` envelope to a `SkillEntry`, so every other member the loose
schema accepted was gone before the CLI saw it — including the `ttlMs` /
`cacheScope` SEP-2640 explicitly leaves open. The code contradicted a comment
two lines above it saying a CLI whose contract is "print the result" must not
reshape one. `getSkillResult` returns the envelope whole; `getSkill` is a
one-line unwrap over it, which matches the callers: the UIs want the entry,
the CLI wants the result.

**The frontmatter verdict and the digest came from different reads.** The
preview fetch and the Verify fetch are separate `resources/read` calls, so a
resource changing between them could pair a verified digest with a
frontmatter verdict for other bytes. `VerificationState.entryText` now holds
the SKILL.md as the verification read it, written in the SAME state update as
the verdict — a separate write was clobbered, because `write` returns a fresh
object and drops anything not named in it. The preview stays the fallback so
a reader who has not clicked Verify still gets the check.

**The Conformance badge excluded the frontmatter findings** it renders, so it
said `0 error(s)` above a red mismatch. Digest and size mismatches stay in
their own badge: "the listing is wrong" and "the bytes are wrong" are
different answers.

**A repeated skill URI collided two React keys** in the TUI list, in a pane
whose job is to show both entries. Index-keyed like the manifest rows.

The web fixture also gains its collision pair in the served-file lookup —
without it they were handed another skill's SKILL.md, which the badge fix
correctly began reporting as a frontmatter error.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ND46Ci64DC3tzBy9aG81Nc
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-07 20:44:51 -04:00
commit d6b1e36e7e
8 changed files with 257 additions and 31 deletions
Download patch file Download diff file Expand all files Collapse all files

34
clients/cli/__tests__/run-method-skills.test.ts
View file

@ -41,6 +41,7 @@ function mockClient(overrides: Record<string, unknown> = {}): InspectorClient {
getSkillsExtension: vi.fn().mockReturnValue({ directoryRead: true }),
listSkills: vi.fn().mockResolvedValue({ skills: [] }),
getSkill: vi.fn(),
getSkillResult: vi.fn(),
readResourceDirectory: vi.fn(),
readResource: vi.fn().mockResolvedValue({
result: { contents: [{ uri: "skill://demo/SKILL.md", text: SKILL_MD }] },
@ -85,19 +86,21 @@ describe("runMethod skills dispatch (#2248)", () => {
// -32602 a declared server returns for a URI it does not serve.
const client = mockClient({
getSkillsExtension: vi.fn().mockReturnValue(undefined),
getSkill: vi.fn(),
getSkillResult: vi.fn(),
});
await expect(
runMethod(client, { method: "skills/get", uri: "skill://x/SKILL.md" }),
).rejects.toMatchObject({ exitCode: EXIT_CODES.USAGE });
expect(client.getSkill).not.toHaveBeenCalled();
expect(client.getSkillResult).not.toHaveBeenCalled();
});
it("keeps the { skill } envelope on skills/get", async () => {
// The client unwraps it for callers that want the entry; a CLI whose
// contract is "print the result" must not quietly reshape the wire form.
const entry = await cleanEntry();
const client = mockClient({ getSkill: vi.fn().mockResolvedValue(entry) });
const client = mockClient({
getSkillResult: vi.fn().mockResolvedValue({ skill: entry }),
});
const outcome = await runMethod(client, {
method: "skills/get",
uri: entry.uri,
@ -105,6 +108,27 @@ describe("runMethod skills dispatch (#2248)", () => {
expect(outcome).toMatchObject({ result: { skill: entry } });
});
it("prints the whole skills/get envelope, not just the entry", async () => {
// SEP-2640 leaves it open whether this result carries `ttlMs`/`cacheScope`,
// so a server may send them — and unwrapping to the entry discarded exactly
// those, from a path whose contract is "print the result" (Copilot).
const entry = await cleanEntry();
const envelope = {
skill: entry,
resultType: "complete",
ttlMs: 60,
cacheScope: "public",
};
const client = mockClient({
getSkillResult: vi.fn().mockResolvedValue(envelope),
});
const outcome = await runMethod(client, {
method: "skills/get",
uri: entry.uri,
});
expect(outcome).toMatchObject({ result: envelope });
});
it("requires --uri for skills/get", async () => {
await expect(
runMethod(mockClient(), { method: "skills/get" }),
@ -179,7 +203,9 @@ describe("runMethod skills dispatch (#2248)", () => {
it("--verify works on a single skills/get", async () => {
const entry = await cleanEntry();
const client = mockClient({ getSkill: vi.fn().mockResolvedValue(entry) });
const client = mockClient({
getSkillResult: vi.fn().mockResolvedValue({ skill: entry }),
});
const outcome = await runMethod(client, {
method: "skills/get",
uri: entry.uri,

16
clients/cli/src/handlers/run-method.ts
View file

@ -352,7 +352,15 @@ export async function runMethod(
// not serve — "this server has no Skills support" and "no such skill"
// are different answers (Copilot).
assertSkillsSupported(inspectorClient, args.method);
const skill = await inspectorClient.getSkill(args.uri, args.metadata);
// The ENVELOPE, not the unwrapped entry. `getSkill` discards every other
// member the result carried — including the `ttlMs` / `cacheScope` that
// SEP-2640 explicitly leaves open — and a CLI whose contract is "print
// the result" must not drop what the server actually sent (Copilot).
const envelope = await inspectorClient.getSkillResult(
args.uri,
args.metadata,
);
const skill = envelope.skill;
if (args.verify) {
const reports = await verifySkills(
inspectorClient,
@ -368,11 +376,7 @@ export async function runMethod(
: { exitCode: EXIT_CODES.SKILL_NONCONFORMANT }),
};
}
// The `{ skill }` envelope is restored here because it is what the wire
// carries: `GetSkillResultSchema` unwraps it for callers that want the
// entry, and a CLI whose contract is "print the result" must not quietly
// reshape one.
result = { skill };
result = envelope;
} else if (args.method === "resources/directory/read") {
if (!args.uri) {
throw new Error(

23
clients/tui/__tests__/SkillsTab.test.tsx
View file

@ -605,6 +605,29 @@ describe("SkillsTab (#2248)", () => {
expect(lastFrame() ?? "").toContain("urn:opaque");
});
it("renders both rows when a listing repeats a URI", () => {
// A malformed listing can carry the same skill twice, and this pane exists
// to show BOTH — a URI-keyed row would collide them and let React drop or
// reuse one (Copilot).
const dup: SkillEntry = {
uri: "skill://twice/SKILL.md",
frontmatter: { name: "twice", description: "Listed twice" },
resources: [],
};
const { lastFrame } = render(
<SkillsTab
skills={[dup, dup]}
pageCount={1}
inspectorClient={null}
width={140}
height={30}
/>,
);
const frame = lastFrame() ?? "";
expect(frame).toContain("Skills (2)");
expect(frame.match(/twice/g)?.length).toBeGreaterThanOrEqual(2);
});
it("keys a row by its index when the entry carries no URI", () => {
// A URI-less entry is a `malformed-uri` finding this pane reports, so it
// must still render a addressable row rather than colliding React keys.

11
clients/tui/src/components/SkillsTab.tsx
View file

@ -333,7 +333,16 @@ export function SkillsTab({
? "warning"
: null;
return (
<Box key={skill.uri || index} paddingY={0} flexShrink={0}>
// Index-keyed like the manifest and finding rows, and for
// the same reason: a malformed listing can repeat a URI, and
// this pane exists to show BOTH entries — a URI key would
// collide them and let React drop or reuse the wrong row
// (Copilot).
<Box
key={`${index}:${skill.uri}`}
paddingY={0}
flexShrink={0}
>
<Text>
{isSelected ? "▶ " : " "}
{worst ? (

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

@ -152,6 +152,9 @@ const ALL_SKILLS = [
MISMATCHED_SKILL,
];
/** Everything `readFixtureFile` can serve a `SKILL.md` for. */
const SERVED_SKILLS = [...ALL_SKILLS, ACME, GLOBEX];
/**
* A `resources/read` that serves the fixture bytes for any known URI. A skill's
* own `SKILL.md` comes from {@link skillMdFor}, so it agrees with the entry's
@ -161,7 +164,10 @@ const ALL_SKILLS = [
const readFixtureFile = vi.fn(async (uri: string) => {
if (uri === "skill://data-analysis/reference.md") return { text: REF_TEXT };
if (uri === "skill://tampered/notes.md") return { text: NOTES_TEXT };
const owner = ALL_SKILLS.find((skill) => skill.uri === uri);
// Every fixture, not only the four in the default catalog — the collision
// pair is served here too, or it would be handed another skill's SKILL.md and
// report a frontmatter mismatch that the fixture never meant to demonstrate.
const owner = SERVED_SKILLS.find((skill) => skill.uri === uri);
return {
text: owner ? skillMdFor(owner.frontmatter as Frontmatter) : SELF_TEXT,
mimeType: "text/markdown",
@ -2379,6 +2385,64 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => {
);
});
it("counts frontmatter findings in the Conformance badge", async () => {
// The findings render inside this section, so counting only the static
// listing issues left the badge saying `0 error(s)` above a red
// `frontmatter-mismatch` — the section contradicting its own output.
const user = userEvent.setup();
const lying: SkillEntry = {
...CLEAN_SKILL,
frontmatter: { name: "data-analysis", description: "Disagrees" },
};
renderWithMantine(<ControlledSkillsScreen skills={[lying]} />);
await user.click(screen.getByText("data-analysis"));
await waitFor(() =>
expect(
screen.getByRole("button", { name: /Conformance/ }),
).toHaveTextContent("1 error(s), 0 warning(s)"),
);
});
it("prefers the bytes verification read over the preview read", async () => {
// The two verdicts came from separate `resources/read` calls, so a resource
// that changed between them could pair a verified digest with a frontmatter
// verdict computed for different bytes (Copilot). After Verify, the
// frontmatter check reads what the verification hashed.
const user = userEvent.setup();
let served = skillMdFor(CLEAN_FM); // agrees with the listing…
const onReadSkillFile = vi.fn(async (uri: string) => {
if (uri === "skill://data-analysis/reference.md")
return { text: REF_TEXT };
return { text: served, mimeType: "text/markdown" };
});
renderWithMantine(
<ControlledSkillsScreen onReadSkillFile={onReadSkillFile} />,
);
await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /Conformance/ }));
await waitFor(() =>
expect(screen.getByText("No structural issues")).toBeInTheDocument(),
);
// …and then the server starts serving something else.
served = skillMdFor({ ...CLEAN_FM, description: "Changed underneath" });
await user.click(
screen.getByRole("button", {
name: "Verify skill://data-analysis/SKILL.md",
}),
);
await waitFor(() =>
expect(
screen.getByTestId("skill-frontmatter-issues"),
).toBeInTheDocument(),
);
expect(
within(screen.getByTestId("skill-frontmatter-issues")).getByText(
/Changed underneath/,
),
).toBeInTheDocument();
});
it("reports nothing when the served frontmatter agrees", async () => {
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);

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

@ -91,6 +91,18 @@ interface VerificationState {
*/
key: string | null;
files: Record<number, FileState>;
/**
* The text of the skill's own `SKILL.md` **as the verification read it**.
*
* Held so the frontmatter comparison and the digest describe the *same*
* fetch. They were derived from two separate `resources/read` calls — the
* on-selection preview and the Verify click — so a resource that changed
* between them could pair a verified digest with a frontmatter verdict for
* different bytes (Copilot). Set only when the verified row is the entry's
* own file; the preview remains the fallback until then, since a reader who
* has not clicked Verify should still get the check.
*/
entryText?: string;
}
/**
@ -869,6 +881,8 @@ export function SkillsScreen({
});
const fileStates = verification.key === manifestKey ? verification.files : {};
const verifiedEntryText =
verification.key === manifestKey ? verification.entryText : undefined;
/**
* Verify one manifest ROW. Keyed by row index, not by URI: the checker
@ -893,8 +907,26 @@ export function SkillsScreen({
);
}, []);
/**
* Whether a manifest row IS the skill's own `SKILL.md`.
*
* By normalized identity, like every other URI comparison here — a manifest
* that spells its self-entry equivalently still names the same file.
*/
const isSelfResource = useCallback(
(resource: SkillResource) =>
selected !== undefined &&
skillUriIdentity(resource.uri) === skillUriIdentity(selected.uri),
[selected],
);
const verifyRow = useCallback(
async (index: number, resource: SkillResource, key: string) => {
async (
index: number,
resource: SkillResource,
key: string,
isSelfRow = false,
) => {
// NOTE: opening the Conformance section deliberately does NOT happen
// here. `verifyRow` is called once per row by every "Verify all" worker
// as it advances, so a batch begun on one skill keeps calling it after
@ -907,7 +939,7 @@ export function SkillsScreen({
// Claimed synchronously, so two verifications of this row are ordered
// before either read starts.
const attempt = (nextAttempt.current += 1);
const write = (state: FileState) =>
const write = (state: FileState, entryText?: string) =>
setVerification((prev) => {
// `null` is the un-adopted initial manifest; any other mismatch is a
// continuation from a manifest that has since been invalidated.
@ -917,16 +949,31 @@ export function SkillsScreen({
// finishing last must not overwrite it.
const held = files[index];
if (held !== undefined && held.attempt > attempt) return prev;
return { key, files: { ...files, [index]: state } };
return {
key,
files: { ...files, [index]: state },
// Carried through explicitly: this returns a FRESH state object, so
// anything not named here is dropped — which silently discarded the
// verified `SKILL.md` text the frontmatter check depends on.
...(entryText !== undefined
? { entryText }
: prev.key === key && prev.entryText !== undefined
? { entryText: prev.entryText }
: {}),
};
});
write({ attempt, status: "pending" });
try {
const contents = await onReadSkillFile(resource.uri);
const result = await verifySkillResource(
resource,
skillFileBytes(contents),
const bytes = skillFileBytes(contents);
const result = await verifySkillResource(resource, bytes);
// The entry's own file is captured in the SAME write as its verdict, so
// the frontmatter check reads the very bytes that were just hashed —
// see `VerificationState.entryText`.
write(
{ attempt, status: "done", verification: result },
isSelfRow ? bytesToText(bytes) : undefined,
);
write({ attempt, status: "done", verification: result });
} catch (err) {
write({
attempt,
@ -957,7 +1004,7 @@ export function SkillsScreen({
const key = manifestKey;
const worker = async (): Promise<void> => {
for (let i = next++; i < manifest.length; i = next++) {
await verifyRow(i, manifest[i], key);
await verifyRow(i, manifest[i], key, isSelfResource(manifest[i]));
}
};
const workers = Math.min(VERIFY_CONCURRENCY, manifest.length);
@ -978,7 +1025,7 @@ export function SkillsScreen({
return next;
}),
);
}, [manifest, manifestKey, openConformance, verifyRow]);
}, [manifest, manifestKey, openConformance, verifyRow, isSelfResource]);
/**
* Put one of the skill's files in the viewer. Driven both by the effect that
@ -1397,7 +1444,13 @@ export function SkillsScreen({
* first time a file happened to be fetched.
*/
const frontmatterIssues = useMemo(() => {
if (!selected || !showingSkillMd || preview === undefined) return [];
if (!selected) return [];
// Prefer the text the VERIFICATION read, so the digest verdict above and
// this one describe one fetch rather than two.
if (verifiedEntryText !== undefined) {
return checkSkillFrontmatterMatch(selected, verifiedEntryText);
}
if (!showingSkillMd || preview === undefined) return [];
// Run against the **raw fetched bytes**, not against `previewParts`.
//
// `previewParts` is a *presentation* value: it only exists when the
@ -1418,7 +1471,7 @@ export function SkillsScreen({
return [];
}
return checkSkillFrontmatterMatch(selected, text);
}, [selected, showingSkillMd, preview]);
}, [selected, showingSkillMd, preview, verifiedEntryText]);
/**
* The findings rendered as list items — everything except the two that are
@ -1438,8 +1491,23 @@ export function SkillsScreen({
[issues],
);
const errorCount = issues.filter((i) => i.severity === "error").length;
const warningCount = issues.length - errorCount;
/**
* Everything the Conformance section reports, for the header badge.
*
* The frontmatter findings render inside this section, so counting only the
* static listing issues left the badge saying `0 error(s)` above a red
* `frontmatter-mismatch` alert — the section contradicting its own output
* (Copilot). Digest and size mismatches stay OUT: they have their own
* `mismatch(es)` badge, because "the listing is wrong" and "the bytes are
* wrong" are different answers and merging them would hide which failed.
*/
const countedIssues = useMemo(
() => [...issues, ...frontmatterIssues],
[issues, frontmatterIssues],
);
const errorCount = countedIssues.filter((i) => i.severity === "error").length;
const warningCount = countedIssues.length - errorCount;
return (
// `data-*` readiness contract for the headless tab smoke (#2148); see
@ -1940,6 +2008,7 @@ export function SkillsScreen({
index,
resource,
manifestKey,
isSelfResource(resource),
);
}}
>

27
core/mcp/inspectorClient.ts
View file

@ -146,13 +146,14 @@ import {
import { buildClientExtensions } from "./extensions.js";
import {
DirectoryReadResultSchema,
GetSkillResultSchema,
GetSkillEnvelopeSchema,
ListSkillsResultSchema,
ModernDirectoryReadResultSchema,
ModernListSkillsResultSchema,
RESOURCES_DIRECTORY_READ_METHOD,
SKILLS_EXTENSION_KEY,
type DirectoryReadResult,
type GetSkillEnvelope,
SKILLS_GET_METHOD,
SKILLS_LIST_METHOD,
type SkillEntry,
@ -5609,11 +5610,28 @@ export class InspectorClient extends InspectorClientEventTarget {
/**
* One skill entry by URI (`skills/get`, SEP-2640). The result envelope is
* required — `GetSkillResultSchema` unwraps `{ skill }` and rejects an entry
* required — `GetSkillEnvelopeSchema` requires `{ skill }` and rejects an entry
* returned inline, so a non-conforming shape fails here rather than being
* silently normalized past the conformance checks.
*/
async getSkill(uri: string, metadata?: RequestMetadata): Promise<SkillEntry> {
return (await this.getSkillResult(uri, metadata)).skill;
}
/**
* `skills/get` as the server sent it — the `{ skill }` envelope **and any
* other members it carried**.
*
* Separate from {@link getSkill} because the callers differ: the UIs want the
* entry, while the CLI prints the result and must not reshape it. SEP-2640
* explicitly leaves open whether this result carries `ttlMs` / `cacheScope`,
* so a server may send them — and unwrapping to the entry discards exactly
* those (Copilot).
*/
async getSkillResult(
uri: string,
metadata?: RequestMetadata,
): Promise<GetSkillEnvelope> {
if (!this.client) {
throw new Error("Client is not connected");
}
@ -5622,14 +5640,13 @@ export class InspectorClient extends InspectorClientEventTarget {
uri,
...(effectiveMeta ? { _meta: effectiveMeta } : {}),
};
// `GetSkillResultSchema` unwraps the envelope, so there is nothing to
// unwrap here.
// The envelope is returned whole; `getSkill` is the one that unwraps.
try {
return await this.invokeMcpClient(
() =>
this.client!.request(
{ method: SKILLS_GET_METHOD, params },
GetSkillResultSchema,
GetSkillEnvelopeSchema,
this.getRequestOptions(this.progressTokenOf(metadata)),
),
{ method: SKILLS_GET_METHOD },

16
core/mcp/skillsSchemas.ts
View file

@ -167,7 +167,21 @@ export const ModernListSkillsResultSchema = ListSkillsResultSchema.extend({
* — which is exactly the failure this extension's support exists to *report*.
* A server that returns the entry inline now fails the parse, loudly.
*/
const GetSkillEnvelopeSchema = z.looseObject({ skill: SkillEntrySchema });
export const GetSkillEnvelopeSchema = z.looseObject({
skill: SkillEntrySchema,
});
/**
* The `skills/get` result as the server sent it, envelope and all.
*
* Exported alongside the unwrapping schema below because the two callers want
* different things: the UIs want the entry, while the CLI's job is to print
* **the result** — and the caching attributes SEP-2640 leaves open are members
* a `looseObject` accepts and the transform then discards, so unwrapping for
* everyone silently dropped them from a contract that promised not to reshape
* anything (Copilot).
*/
export type GetSkillEnvelope = z.infer<typeof GetSkillEnvelopeSchema>;
/**
* `skills/get` result, unwrapped to the entry it carries.