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

Browse source 
**The depth guard covered only the YAML side.** The listing arrives over
JSON-RPC so it cannot be cyclic, but it is just as unbounded in depth — and
both `jsonLikeEqual` and its message formatter walk it, so a server
advertising a deeply nested value crashed the tool exactly as a cyclic served
one did. Reproduced first: 60,000 levels against a shallow served value gives
`RangeError: Maximum call stack size exceeded`. `jsonGraphError` is shared
now and runs over `entry.frontmatter` before the file is parsed — there is no
point reading a SKILL.md if the thing to compare it against is unusable.

**A frontmatter mismatch stayed behind a collapsed section.** Round 7 made
the badge count these findings; the alerts explaining a mandatory
verification failure were still one click away on a section that opens
collapsed for a structurally clean entry. Conformance now reveals itself when
they appear, keyed on the entry so it fires once rather than fighting a user
who collapses it again.

That reveal exposed three fixtures serving one skill's SKILL.md for every URI
— a genuine frontmatter mismatch the check was right to report. The stale-
batch invariant test and the Storybook fixtures now derive each file from the
frontmatter its own entry advertises, with digests computed from those bytes
rather than hard-coded, so the class of drift is closed rather than the
instances patched. `SAMPLE_FM` is the single source for both halves.

One story subtlety worth recording: only a skill's OWN SKILL.md is derived.
Serving it for `notes.md` too made the tampered fixture fail its SIZE check
first, which is a different finding from the digest mismatch that story
exists to demonstrate.

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 21:09:26 -04:00
commit 8ae1a1664d
6 changed files with 217 additions and 47 deletions
Download patch file Download diff file Expand all files Collapse all files

121
clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx
View file

@ -3,6 +3,7 @@ import type { ComponentProps } from "react";
import type { Meta, StoryObj } from "@storybook/react-vite";
import { expect, fn, userEvent, waitFor, within } from "storybook/test";
import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas";
import { sha256Bytes } from "@inspector/core/mcp/sha256";
import { SkillsScreen } from "./SkillsScreen";
import type { SkillsUiState } from "./SkillsScreen";
import { EMPTY_SKILLS_UI } from "../screenUiState";
@ -16,29 +17,74 @@ function StatefulSkillsScreen(args: ComponentProps<typeof SkillsScreen>) {
}
const REF_TEXT = "# Column rules\n";
const SELF_TEXT = "# skill\n";
// The real digests of those two strings, so the clean skill actually verifies
// when the "Verify all" story runs — a placeholder would demo a false green.
const REF_DIGEST =
"sha256:e201429aa2684958ca1a0537ab4eb4b7eb3a81c71e7cc7a11397eb500738e015";
const SELF_DIGEST =
"sha256:6504f2de0a1febf7492c3b98f93d9ab49558eb364607a706f02fe9a75aa7f75b";
/**
* A skill's own `SKILL.md`, built FROM the frontmatter its entry advertises.
*
* SEP-2640 requires the two to match field for field, and the screen now checks
* it — so a shared placeholder body with no frontmatter made every "conforming"
* story report a `frontmatter-absent` error. Deriving the file makes that class
* of drift impossible rather than merely fixed, which is the same discipline
* `test-servers/src/skills.ts` and the unit fixtures apply.
*/
const skillMd = (name: string, description: string) =>
`---\nname: ${name}\ndescription: ${description}\n---\n\n# ${name}\n`;
/** Real digests, computed from the very bytes served, so the clean skill
* actually verifies when the "Verify all" story runs — a hard-coded
* placeholder would demo a false green, and could not survive an edit to the
* text above. `sha256Bytes` is the repo's synchronous implementation, which is
* what lets this happen at module scope in a CSF file. */
const digestOf = (text: string) =>
`sha256:${[...sha256Bytes(new TextEncoder().encode(text))]
.map((b) => b.toString(16).padStart(2, "0"))
.join("")}`;
const REF_DIGEST = digestOf(REF_TEXT);
/** The frontmatter each sample skill advertises, keyed by its path segment. */
const SAMPLE_FM: Record<string, { name: string; description: string }> = {
"data-analysis": {
name: "data-analysis",
description: "Analyze a CSV and summarize its columns",
},
"tampered-notes": {
name: "tampered-notes",
description: "Advertises a digest its bytes do not match",
},
"wrong-folder": {
name: "right-name",
description: "Served from a folder that disagrees with its name",
},
"big-manifest": {
name: "big-manifest",
description: "A skill with a long manifest",
},
};
/** The file a given skill path serves, derived from its own frontmatter. */
const selfText = (path: string) => {
const fm = SAMPLE_FM[path];
return fm ? skillMd(fm.name, fm.description) : "# skill\n";
};
/** Every manifest lists the skill's own SKILL.md — a manifest is the complete
* file set, so one that omits it is a `manifest-missing-self` error. */
const selfEntry = (path: string) => ({
uri: `skill://${path}/SKILL.md`,
digest: SELF_DIGEST,
size: 8,
});
const selfEntry = (path: string) => {
const text = selfText(path);
return {
uri: `skill://${path}/SKILL.md`,
digest: digestOf(text),
size: new TextEncoder().encode(text).byteLength,
};
};
const sampleSkills: SkillEntry[] = [
{
uri: "skill://data-analysis/SKILL.md",
frontmatter: {
name: "data-analysis",
description: "Analyze a CSV and summarize its columns",
},
// Read from SAMPLE_FM, which is also what the served file is built from —
// so the entry and its SKILL.md agree by construction.
frontmatter: SAMPLE_FM["data-analysis"],
resources: [
selfEntry("data-analysis"),
{
@ -50,10 +96,7 @@ const sampleSkills: SkillEntry[] = [
},
{
uri: "skill://tampered-notes/SKILL.md",
frontmatter: {
name: "tampered-notes",
description: "Advertises a digest its bytes do not match",
},
frontmatter: SAMPLE_FM["tampered-notes"],
resources: [
selfEntry("tampered-notes"),
{
@ -76,10 +119,7 @@ const sampleSkills: SkillEntry[] = [
},
{
uri: "skill://wrong-folder/SKILL.md",
frontmatter: {
name: "right-name",
description: "URI path segment disagrees with frontmatter.name",
},
frontmatter: SAMPLE_FM["wrong-folder"],
resources: [selfEntry("wrong-folder")],
},
];
@ -94,11 +134,19 @@ const meta: Meta<typeof SkillsScreen> = {
ui: EMPTY_SKILLS_UI,
onUiChange: fn(),
onRefreshList: fn(),
onReadSkillFile: fn(async (uri: string) =>
uri.endsWith("reference.md")
? { text: REF_TEXT }
: { text: SELF_TEXT, mimeType: "text/markdown" },
),
onReadSkillFile: fn(async (uri: string) => {
if (uri.endsWith("reference.md")) return { text: REF_TEXT };
// Only a skill's OWN SKILL.md is derived from its frontmatter. Every
// other manifest file keeps the 8-byte placeholder its entry declares —
// serving the SKILL.md for `notes.md` made the tampered fixture fail its
// SIZE check first, which is a different finding from the digest
// mismatch that story exists to show.
if (!uri.endsWith("/SKILL.md")) {
return { text: "# skill\n", mimeType: "text/markdown" };
}
const path = uri.slice("skill://".length, -"/SKILL.md".length);
return { text: selfText(path), mimeType: "text/markdown" };
}),
// Echoes back the entry `skills/list` advertised, so "Fetch with
// skills/get" demonstrates the matching case rather than throwing.
onGetSkill: fn(async (uri: string) => {
@ -190,10 +238,7 @@ const LONG_SKILL_MD = [
// overflowing content.
const manyFilesSkill: SkillEntry = {
uri: "skill://big-manifest/SKILL.md",
frontmatter: {
name: "big-manifest",
description: "A conforming skill that declares a great many files",
},
frontmatter: SAMPLE_FM["big-manifest"],
resources: [
selfEntry("big-manifest"),
...Array.from({ length: 120 }, (_, i) => ({
@ -217,8 +262,10 @@ const manyFilesSkill: SkillEntry = {
export const LongManifest: Story = {
args: {
skills: [manyFilesSkill],
// Derived like every other fixture, so the frontmatter check stays silent
// and this story measures only the long-manifest layout it is about.
onReadSkillFile: fn(async () => ({
text: "---\nname: big-manifest\n---\n\n# Big manifest\n",
text: selfText("big-manifest"),
mimeType: "text/markdown",
})),
},
@ -276,10 +323,12 @@ const hostileHeaderSkill: SkillEntry = {
resources: [
{
uri: `skill://${"very-long-path-segment/".repeat(30)}SKILL.md`,
digest: SELF_DIGEST,
// This fixture exercises LAYOUT under hostile strings; its digests are
// never verified by the story, so a placeholder is honest here.
digest: `sha256:${"0".repeat(64)}`,
size: 8,
},
{ uri: HOSTILE_FILE_URI, digest: SELF_DIGEST, size: 8 },
{ uri: HOSTILE_FILE_URI, digest: `sha256:${"0".repeat(64)}`, size: 8 },
],
};

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

@ -155,6 +155,9 @@ const ALL_SKILLS = [
/** Everything `readFixtureFile` can serve a `SKILL.md` for. */
const SERVED_SKILLS = [...ALL_SKILLS, ACME, GLOBEX];
/** The many-row fixture's frontmatter, shared with the stub that serves it. */
const MANY_FM: Frontmatter = { name: "many", description: "Many rows" };
/**
* 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
@ -386,17 +389,22 @@ describe("SkillsScreen", () => {
const user = userEvent.setup();
// Held open so the batch is still in flight when the selection changes.
const releases: (() => void)[] = [];
// Each URI gets the file its OWN entry implies, so the frontmatter check
// stays silent and this test measures only the open-state invariant it is
// about. A stub serving one skill's text for every URI produces a genuine
// mismatch, which now reveals Conformance by design.
const onReadSkillFile = vi.fn(
() =>
(uri: string) =>
new Promise<{ text: string }>((resolve) => {
releases.push(() => resolve({ text: SELF_TEXT }));
const fm = uri.startsWith("skill://many/") ? MANY_FM : CLEAN_FM;
releases.push(() => resolve({ text: skillMdFor(fm) }));
}),
);
// More rows than the concurrency cap, so workers keep pulling.
const manyRows: SkillEntry = {
...CLEAN_SKILL,
uri: "skill://many/SKILL.md",
frontmatter: { name: "many", description: "Many rows" },
frontmatter: MANY_FM,
resources: Array.from({ length: 10 }, (_, i) => ({
uri: i === 0 ? "skill://many/SKILL.md" : `skill://many/f${i}.md`,
digest: SELF_DIGEST,
@ -2315,7 +2323,7 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => {
};
renderWithMantine(<ControlledSkillsScreen skills={[lying]} />);
await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /Conformance/ }));
// No click to expand: a frontmatter finding reveals the section itself.
await waitFor(() =>
expect(
screen.getByTestId("skill-frontmatter-issues"),
@ -2352,7 +2360,7 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => {
/>,
);
await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /Conformance/ }));
// No click to expand: a frontmatter finding reveals the section itself.
await waitFor(() =>
expect(
screen.getByTestId("skill-frontmatter-issues"),
@ -2377,7 +2385,7 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => {
/>,
);
await user.click(screen.getByText("data-analysis"));
await user.click(screen.getByRole("button", { name: /Conformance/ }));
// No click to expand: a frontmatter finding reveals the section itself.
await waitFor(() =>
expect(
screen.getByTestId("skill-frontmatter-issues"),
@ -2385,6 +2393,41 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => {
);
});
it("reveals Conformance when a frontmatter finding arrives", async () => {
// A structurally clean entry opens collapsed, and the frontmatter findings
// arrive later from the SKILL.md read — so the alerts explaining a
// mandatory verification failure sat behind a click the reader had no
// reason to make (Copilot).
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/ }),
).toHaveAttribute("aria-expanded", "true"),
);
expect(screen.getByTestId("skill-frontmatter-issues")).toBeInTheDocument();
});
it("leaves a clean entry's Conformance collapsed", async () => {
// The reveal must not fire when there is nothing to reveal, or it undoes
// the auto-collapse it sits next to.
const user = userEvent.setup();
renderWithMantine(<ControlledSkillsScreen />);
await user.click(screen.getByText("data-analysis"));
await waitFor(() =>
expect(readFixtureFile).toHaveBeenCalledWith(CLEAN_SKILL.uri),
);
expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute(
"aria-expanded",
"false",
);
});
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

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

@ -1482,6 +1482,27 @@ export function SkillsScreen({
* entry whose only finding is a banner one ended up showing an empty findings
* container instead of "no structural issues".
*/
/**
* Reveal Conformance when the frontmatter check finds something.
*
* A structurally clean entry opens with the section COLLAPSED — the header
* badge carries the whole answer — but the frontmatter findings arrive later,
* from the `SKILL.md` read, and land inside that collapsed section. The badge
* now counts them, so the number changes; the alerts explaining a mandatory
* verification failure were still a click away (Copilot).
*
* Keyed on the entry so it fires **once** per skill, when findings first
* appear, rather than fighting a user who deliberately collapses it again.
* `useValueChange` runs during render and does only `setState`, as that hook
* requires; the key is a primitive so `Object.is` cannot loop.
*/
useValueChange(frontmatterIssues.length > 0 ? manifestKey : "", (next) => {
if (next === "") return;
setOpenSections((prev) =>
prev.includes("conformance") ? prev : [...prev, "conformance"],
);
});
const listedIssues = useMemo(
() =>
issues.filter(

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

@ -1035,6 +1035,36 @@ describe("checkSkillFrontmatterMatch (#2248)", () => {
).toEqual([]);
});
it("bounds the LISTING side too, not only the served YAML", () => {
// The listing arrives over JSON-RPC so it cannot be cyclic, but it is just
// as unbounded in depth — and both the comparison and its message
// formatter walk it, so an absurdly nested advertised value crashed the
// tool exactly as a cyclic served one did (Copilot).
let deep: unknown = "leaf";
for (let i = 0; i < 5000; i += 1) deep = { a: deep };
const issues = checkSkillFrontmatterMatch(
entry({ x: deep as Record<string, unknown> }),
file("x: shallow"),
);
expect(issues).toEqual([
expect.objectContaining({
code: "frontmatter-unparsable",
severity: "error",
}),
]);
expect(issues[0].message).toMatch(/listing's own frontmatter/);
});
it("still compares an ordinarily nested listing value", () => {
// The bound must not reject anything a real skill would carry.
expect(
checkSkillFrontmatterMatch(
entry({ meta: { a: { b: { c: [1, 2] } } } }),
file("meta:\n a:\n b:\n c: [1, 2]"),
),
).toEqual([]);
});
it("reports nothing for two empty frontmatters", () => {
expect(checkSkillFrontmatterMatch(entry({}), file(""))).toEqual([]);
});

14
core/mcp/skillFile.ts
View file

@ -117,7 +117,13 @@ function hasContent(yamlText: string): boolean {
const MAX_FRONTMATTER_DEPTH = 64;
/**
* Why a parsed frontmatter cannot be compared, or `undefined` when it can.
* Why a frontmatter value cannot be compared, or `undefined` when it can.
*
* Exported because **both sides need it**. The served side can be cyclic; the
* listed side arrives over JSON-RPC and cannot be, but it is just as unbounded
* in DEPTH — a server can advertise a listing nested tens of thousands of
* levels deep, and the comparison and its message formatter both recurse. A
* guard on only the YAML side left that door open (Copilot).
*
* ⚠️ **A YAML document is a graph, not a tree, and JSON is a tree.** An alias
* can refer to its own ancestor — `meta: &m [*m]` parses without error into a
@ -136,10 +142,10 @@ const MAX_FRONTMATTER_DEPTH = 64;
* — which YAML aliases make ordinary and which JSON represents perfectly well —
* is not mistaken for a cycle.
*/
function jsonGraphError(
export function jsonGraphError(
value: unknown,
seen: Set<object>,
depth: number,
seen: Set<object> = new Set(),
depth = 0,
): string | undefined {
if (typeof value !== "object" || value === null) return undefined;
if (depth > MAX_FRONTMATTER_DEPTH) {

23
core/mcp/skills.ts
View file

@ -42,7 +42,11 @@ import {
type SkillResource,
} from "./skillsSchemas.js";
import { sha256Bytes } from "./sha256.js";
import { parseSkillFrontmatter, splitSkillFile } from "./skillFile.js";
import {
jsonGraphError,
parseSkillFrontmatter,
splitSkillFile,
} from "./skillFile.js";
/** Maximum resource entries a single skill may declare (SEP-2640). */
export const SKILL_MAX_RESOURCE_ENTRIES = 512;
@ -852,6 +856,23 @@ export function checkSkillFrontmatterMatch(
},
];
}
// The LISTING side is bounded too, before anything recurses over it. It
// arrives over JSON-RPC so it cannot be cyclic, but it is just as unbounded
// in depth — and both `jsonLikeEqual` and `displayValue` walk it, so a
// server advertising an absurdly nested value crashed the tool exactly as a
// cyclic served one did (Copilot). Checked before the file is parsed: there
// is no point reading one if the thing to compare it against is unusable.
const listedError = jsonGraphError(entry.frontmatter);
if (listedError) {
return [
{
code: "frontmatter-unparsable",
severity: "error",
message: `The listing's own frontmatter cannot be compared: ${listedError}`,
resourceUri: entry.uri,
},
];
}
const parsed = parseSkillFrontmatter(frontmatter);
if ("error" in parsed) {
return [