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

fix: address Copilot review round 5 on #2251

Browse source 
Two of these reverse round 4, which asked for the opposite. Where the
readings conflict I took the permissive one: a wrong tightening rejects
conforming servers, a wrong loosening only fails to report something.

- skills.ts: stop requiring the `skill:` scheme. SEP-2640 says a server
  SHOULD use `skill://` and explicitly allows a domain-native scheme, so
  round 4's requirement handed a conforming `github://…` skill a false
  `malformed-uri` and skipped its name and root checks. Containment is
  scheme-independent — it compares against THIS entry's own root — so the
  traversal fix that requirement came packaged with is unaffected.
- skills.ts: the 512-entry and 16 MiB limits are warnings. Both are
  SHOULD NOTs for a server and MAYs for a host, so calling them errors
  contradicted the "error = a MUST was broken" contract round 4
  established and told authors a permitted skill was invalid.
- skills.ts: `skillEntriesMatch` compares canonically — keys sorted,
  manifest sorted by URI. `JSON.stringify` treated key order and manifest
  order as differences, though neither carries meaning.
- SkillsScreen: a differing `skills/get` is "a different snapshot"
  (yellow), not a disagreement (red). The SEP defines it as a fresh
  point-in-time read, so a skill that changed since the listing
  legitimately differs.
- useServerCommands: route `onGetSkill` through the command-scoped OAuth
  recovery like every other server command. An expired authorization
  rendered an error and stopped there — no reauthorization, no retry.
- test-servers/skills.ts: stamp the modern base result envelope
  (`resultType` / `ttlMs` / `cacheScope`) on every skills result. The SDK
  stamps it only for methods in its own codec, and `skills/*` are
  consumer-owned, so a 2026-era connection was receiving results without
  it. Unconditional rather than per era: the modern leg builds a server
  per request, so there is no era to branch on at registration time, and
  on legacy they are three members no codec inspects.
- composable-test-server: remove the `skills.directoryRead` option. It was
  publicly settable and advertised a method nothing serves, producing the
  exact false capability the fixture exists to help catch. `skills` is now
  a plain boolean; the flag returns in phase 3 (#2248) with its handler.

Still not changed: whether a modern `skills/list` RESULT SCHEMA must
require that envelope. Stamping it server-side is free; requiring it
client-side would reject servers on a reading nobody has checked against
the normative text. #2248 owns settling it.

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 23:11:46 -04:00
commit 754ce5bd95
5 changed files with 272 additions and 81 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -437,7 +437,7 @@ describe("SkillsScreen", () => {
expect(within(issues).getAllByText("duplicate-resource")).toHaveLength(2);
});
it("fetches the selected entry through skills/get and reports agreement", async () => {
it("fetches the selected entry through skills/get and reports a match", async () => {
// The acceptance criterion this exists for: `skills/get` is one of the two
// methods the extension requires, and a server author's handler is only
// exercisable if something actually calls it.
@ -450,13 +450,36 @@ describe("SkillsScreen", () => {
);
expect(onGetSkill).toHaveBeenCalledWith(CLEAN_SKILL.uri);
expect(
await screen.findByText("skills/get agrees with skills/list"),
await screen.findByText("skills/get matches skills/list"),
).toBeInTheDocument();
});
it("reports a skills/get entry that disagrees with the listing", async () => {
// Both describe the same skill, so a disagreement is a server bug that
// only a side-by-side fetch can surface.
it("treats key and manifest order as immaterial when matching", async () => {
// The manifest is a set and JSON key order carries no meaning, so a server
// that enumerates either differently is not inconsistent — a
// `JSON.stringify` comparison would have called it one.
const user = userEvent.setup();
const onGetSkill = vi.fn().mockResolvedValue({
resources: [...CLEAN_SKILL.resources].reverse(),
frontmatter: {
description: CLEAN_SKILL.frontmatter.description,
name: CLEAN_SKILL.frontmatter.name,
},
uri: CLEAN_SKILL.uri,
});
renderWithMantine(<ControlledSkillsScreen onGetSkill={onGetSkill} />);
await user.click(screen.getByText("data-analysis"));
await user.click(
screen.getByRole("button", { name: /Fetch with skills\/get/ }),
);
expect(
await screen.findByText("skills/get matches skills/list"),
).toBeInTheDocument();
});
it("reports a skills/get entry that differs from the listing", async () => {
// Shown, but not called an error: `skills/get` is a fresh snapshot, so a
// skill that genuinely changed since the listing legitimately differs.
const user = userEvent.setup();
const onGetSkill = vi.fn().mockResolvedValue({
...CLEAN_SKILL,
@ -468,7 +491,7 @@ describe("SkillsScreen", () => {
screen.getByRole("button", { name: /Fetch with skills\/get/ }),
);
expect(
await screen.findByText("skills/get disagrees with skills/list"),
await screen.findByText("skills/get returned a different snapshot"),
).toBeInTheDocument();
// The fetched entry is rendered beside the verdict so the difference is
// inspectable rather than merely asserted. (Its JSON goes through

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

@ -23,6 +23,7 @@ import { DYNAMIC_RESOURCES } from "@inspector/core/mcp/skillsSchemas.js";
import {
checkSkillConformance,
skillDisplayName,
skillEntriesMatch,
totalSkillBytes,
verifySkillResource,
type SkillIssue,
@ -82,13 +83,13 @@ interface PreviewState {
/**
* The result of the on-demand `skills/get`, plus the manifest it belongs to.
* `agrees` records whether the fetched entry matched the one `skills/list`
* returned — the reason for making the call at all.
* `matches` records whether the fetched entry describes the same skill as the
* one `skills/list` returned — the reason for making the call at all.
*/
interface FetchedEntryState {
key: string | null;
entry?: SkillEntry;
agrees?: boolean;
matches?: boolean;
message?: string;
}
@ -483,12 +484,14 @@ export function SkillsScreen({
// started under so a late answer cannot land under another skill.
void onGetSkill(selected.uri)
.then((entry) => {
// Compared field-by-field against what `skills/list` advertised. The
// two describe the same skill, so a disagreement is a server bug that
// only shows up when both are fetched.
const agrees = JSON.stringify(entry) === JSON.stringify(selected);
// Compared semantically against what `skills/list` advertised — see
// `skillEntriesMatch` for why a `JSON.stringify` comparison would
// report key order and manifest order as differences.
const matches = skillEntriesMatch(entry, selected);
setFetchedEntry((prev) =>
prev.key !== null && prev.key !== key ? prev : { key, entry, agrees },
prev.key !== null && prev.key !== key
? prev
: { key, entry, matches },
);
})
.catch((err: unknown) => {
@ -767,20 +770,24 @@ export function SkillsScreen({
{fetched?.entry !== undefined && (
<Alert
data-testid="skills-get-result"
color={fetched.agrees ? "green" : "red"}
// Yellow, not red: `skills/get` is a fresh point-in-time
// snapshot, so a skill that genuinely changed since the
// listing legitimately differs. Worth showing, not an
// accusation.
color={fetched.matches ? "green" : "yellow"}
title={
fetched.agrees
? "skills/get agrees with skills/list"
: "skills/get disagrees with skills/list"
fetched.matches
? "skills/get matches skills/list"
: "skills/get returned a different snapshot"
}
>
<Stack gap={2}>
<Text size="sm">
{fetched.agrees
? "The entry this server returns for this URI is identical to the one it listed."
: "The entry this server returns for this URI differs from the one it listed; both describe the same skill, so one of them is wrong."}
{fetched.matches
? "The entry this server returns for this URI describes the same skill it listed (compared ignoring key and manifest order)."
: "The entry this server returns for this URI differs from the one it listed. `skills/get` is a fresh snapshot, so this is expected if the skill changed since the list was fetched — and a server inconsistency if it did not."}
</Text>
{!fetched.agrees && (
{!fetched.matches && (
<ContentViewer
block={{
type: "text",

51
clients/web/src/hooks/useServerCommands.test.tsx
View file

@ -936,6 +936,57 @@ describe("onGetSkill (#2234)", () => {
"Client is not connected",
);
});
it("retries once after a satisfied recovery", async () => {
// Every server command routes through the shared recovery; without it an
// expired authorization would render an error and stop there, with no
// reauthorization and no retry.
const recover = vi.fn().mockResolvedValue(true);
const getSkill = vi
.fn()
.mockRejectedValueOnce(authError())
.mockResolvedValue({
uri: "skill://demo/SKILL.md",
frontmatter: {},
resources: [],
});
const h = harness({
client: client({ getSkill }),
activeServerId: "a",
recovery: { handleCommandScopedAuthRecovery: recover },
});
await expect(
h.api().onGetSkill("skill://demo/SKILL.md"),
).resolves.toMatchObject({ uri: "skill://demo/SKILL.md" });
expect(getSkill).toHaveBeenCalledTimes(2);
});
it("rethrows when the recovery was not satisfied", async () => {
const recover = vi.fn().mockResolvedValue(false);
const h = harness({
client: client({ getSkill: vi.fn().mockRejectedValue(authError()) }),
activeServerId: "a",
recovery: { handleCommandScopedAuthRecovery: recover },
});
await expect(
h.api().onGetSkill("skill://demo/SKILL.md"),
).rejects.toBeInstanceOf(AuthRecoveryRequiredError);
});
it("rethrows a non-auth failure untouched", async () => {
const recover = vi.fn();
const h = harness({
client: client({
getSkill: vi.fn().mockRejectedValue(new Error("-32602")),
}),
activeServerId: "a",
recovery: { handleCommandScopedAuthRecovery: recover },
});
await expect(h.api().onGetSkill("skill://demo/SKILL.md")).rejects.toThrow(
"-32602",
);
expect(recover).not.toHaveBeenCalled();
});
});
describe("onRefreshSkills (#2234)", () => {

20
clients/web/src/hooks/useServerCommands.tsx
View file

@ -982,9 +982,25 @@ export function useServerCommands({
const onGetSkill = useCallback(
async (uri: string): Promise<SkillEntry> => {
if (!inspectorClient) throw new Error("Client is not connected");
return inspectorClient.getSkill(uri);
// Routed through the shared recovery like every other server command
// (#2174). Without it an expired authorization renders an error in the
// panel and stops there — no reauthorization, no retry — which is the
// one thing this hook exists to make impossible.
const get = () => inspectorClient.getSkill(uri);
try {
return await get();
} catch (err) {
if (err instanceof AuthRecoveryRequiredError && activeServerId) {
const satisfied = await handleCommandScopedAuthRecovery(err, {
serverId: activeServerId,
source: "resource",
});
if (satisfied) return get();
}
throw err;
}
},
[inspectorClient],
[inspectorClient, activeServerId, handleCommandScopedAuthRecovery],
);
const onRefreshSkills = useCallback(() => {

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

@ -107,16 +107,18 @@ describe("skillNameFromUri", () => {
);
});
it("reads the name off a domain-native scheme too", () => {
expect(skillNameFromUri("github://acme/repo/data-analysis/SKILL.md")).toBe(
"data-analysis",
);
});
it("returns undefined for a URI that does not end in /SKILL.md", () => {
expect(skillNameFromUri("skill://demo/other.md")).toBeUndefined();
// The suffix must include the separator: a bare "SKILL.md" has no segment.
expect(skillNameFromUri("SKILL.md")).toBeUndefined();
});
it("returns undefined for a non-skill scheme", () => {
expect(skillNameFromUri("https://demo/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.
@ -145,12 +147,16 @@ describe("normalizeSkillUri", () => {
expect(normalizeSkillUri("demo/SKILL.md")).toBeUndefined();
});
it("rejects a non-skill scheme", () => {
// Checking only that a URI is hierarchical would let this through and then
// pass the name and root checks — a manifest pointing anywhere on the web,
// reported as conforming.
expect(normalizeSkillUri("https://demo/SKILL.md")).toBeUndefined();
expect(normalizeSkillUri("file:///demo/SKILL.md")).toBeUndefined();
it("does not privilege the skill: scheme", () => {
// SEP-2640 only says a server SHOULD use `skill://`, and explicitly allows
// a domain-native scheme — so rejecting one would hand a conforming server
// a false `malformed-uri` and skip its name and root checks.
expect(normalizeSkillUri("github://acme/repo/SKILL.md")).toBe(
"github://acme/repo/SKILL.md",
);
expect(normalizeSkillUri("https://demo/a/../SKILL.md")).toBe(
"https://demo/SKILL.md",
);
});
it("rejects an opaque-path URI, which the parser does not normalize", () => {
@ -406,10 +412,13 @@ describe("checkSkillConformance", () => {
],
}),
);
expect(issues.map((i) => i.code)).toContain("size-limit-exceeded");
const finding = issues.find((i) => i.code === "size-limit-exceeded");
expect(finding?.severity).toBe("warning");
});
it("reports a manifest over the 512-entry limit", () => {
// Both limits are SHOULD NOTs for a server and MAYs for a host, so exceeding
// one makes a skill less portable rather than invalid.
it("reports a manifest over the 512-entry limit as a warning", () => {
const resources = [
{ uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 },
...Array.from({ length: SKILL_MAX_RESOURCE_ENTRIES }, (_unused, i) => ({
@ -419,10 +428,11 @@ describe("checkSkillConformance", () => {
})),
];
const issues = checkSkillConformance(entry({ resources }));
expect(issues.map((i) => i.code)).toContain("resource-limit-exceeded");
const finding = issues.find((i) => i.code === "resource-limit-exceeded");
expect(finding?.severity).toBe("warning");
});
it("reports a manifest over the 16 MiB limit", () => {
it("reports a manifest over the 16 MiB limit as a warning", () => {
const issues = checkSkillConformance(
entry({
resources: [