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

Browse source 
Eight findings. Two were visible in a terminal capture I had already taken
and read past.

**A digest mismatch rendered with no digests.** `verifySkillResource` sets
`reason` for a SIZE mismatch — which short-circuits before hashing — while a
digest mismatch carries `expectedDigest` / `actualDigest` and no `reason`, so
the TUI printed a bare `✗ notes.md` with nothing to act on. `failureDetail`
now falls back to the expected/actual pair, truncated to fit the pane.

**A dynamic skill's read failure rendered nowhere.** Its synthetic
`read-error` row lives in the report, not the manifest, and the manifest is
empty for such a skill by definition — so the pane said only "Verification
FAILED". There is a Read failures block for any report file the manifest does
not cover, matched on normalized identity.

**The non-finite sentinel could be aliased.** A listing whose value genuinely
was `{"#non-finite":"NaN"}` canonicalized identically to a served `.nan`, so
the round-3 fix moved the bug rather than closing it. Any encoding into the
value space can be aliased by a document containing the encoding, so the
comparison is structural now (`jsonLikeEqual`) and there is no sentinel at
all; `Object.is` gives NaN === NaN while keeping ±Infinity distinct.
Serialization is used only for the message.

**`parseSkillFrontmatter("null")` returned `{ fields: {} }`,** contradicting
its own non-mapping contract: an explicit null scalar parses to exactly what
an empty block does. They are told apart by the source, since the parsed
value cannot.

Also: the `files` doc comment still claimed it is empty for a dynamic skill,
which the round-2 fix made untrue; and three inline Mantine elements with two
static styling props each are extracted to `.withProps()` constants, per the
convention the rest of the file follows.

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 19:55:17 -04:00
commit 799763543f
8 changed files with 313 additions and 33 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -670,6 +670,88 @@ describe("SkillsTab (#2248)", () => {
expect(lastFrame() ?? "").toContain("skill://acme/reports/SKILL.md");
});
it("shows the digests for a mismatch, not just the failed mark", async () => {
// `verifySkillResource` sets `reason` for a SIZE mismatch but not a digest
// one, so a pane rendering only `reason` left a bare `✗` with no diagnosis
// — a failed verification the reader cannot act on (Copilot).
// The declared size must be RIGHT, or the cheaper size cross-check
// short-circuits before hashing and reports its own `reason` instead —
// which is the path that already rendered.
const digestOnly: SkillEntry = {
...clean,
resources: [
{
uri: "skill://clean/SKILL.md",
digest: CLEAN_DIGEST,
size: textToBytes(SKILL_MD).byteLength,
},
],
};
const { lastFrame, stdin } = render(
<SkillsTab
skills={[digestOnly]}
pageCount={1}
inspectorClient={mockClient()}
width={160}
height={30}
focusedPane="list"
/>,
);
stdin.write(ENTER);
await tick();
const frame = lastFrame() ?? "";
expect(frame).toContain("Verification FAILED");
// Truncated to keep the line inside a narrow pane; the CLI report carries
// the digests in full.
expect(frame).toMatch(/expected sha256:0+…/);
expect(frame).toMatch(/got sha256:[0-9a-f]+…/);
});
it("shows the reason for a size mismatch, which carries no digest", async () => {
// The other arm: a length disagreement fails before the hash, so there is
// no actual digest to print and the reason is the whole diagnosis.
const { lastFrame, stdin } = render(
<SkillsTab
skills={[clean]}
pageCount={1}
inspectorClient={mockClient()}
width={160}
height={30}
focusedPane="list"
/>,
);
stdin.write(ENTER);
await tick();
expect(lastFrame() ?? "").toContain(
"Manifest declares 51 bytes but the fetched file is 52.",
);
});
it("shows a read failure the manifest does not cover", async () => {
// A dynamic skill has no manifest rows, so the synthetic read-error row
// `verifySkills` records for its own SKILL.md was rendered nowhere and the
// pane said only "Verification FAILED".
const { lastFrame, stdin } = render(
<SkillsTab
skills={[dynamic]}
pageCount={1}
inspectorClient={mockClient(
vi.fn().mockRejectedValue(new Error("upstream gone")),
)}
width={160}
height={30}
focusedPane="list"
/>,
);
stdin.write(ENTER);
await tick();
const frame = lastFrame() ?? "";
expect(frame).toContain("Read failures:");
expect(frame).toContain("SKILL.md");
expect(frame).toContain("upstream gone");
expect(frame).toContain("Verification FAILED");
});
it("shows the details footer only when the details pane is focused", () => {
const unfocused = render(
<SkillsTab

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

@ -36,6 +36,7 @@ import {
} from "@inspector/core/mcp/skillsSchemas.js";
import {
verifySkills,
type SkillFileReport,
type SkillVerifyReport,
} from "@inspector/core/mcp/skillsVerification.js";
import { useSelectableList } from "../hooks/useSelectableList.js";
@ -98,6 +99,23 @@ function entryKey(entry: SkillEntry): string {
return JSON.stringify(entry);
}
/**
* The explanation printed under a failed file row.
*
* `verifySkillResource` sets `reason` for a SIZE mismatch but not for a digest
* one — that carries `expectedDigest` / `actualDigest` instead — so a pane that
* rendered only `reason` showed a bare `✗ notes.md` and never said why, leaving
* the failure unactionable (Copilot). Digests are truncated because the pane is
* 40-odd columns wide and the first bytes are enough to see that two differ;
* the CLI report carries them in full.
*/
function failureDetail(file: SkillFileReport): string | undefined {
if (file.reason) return file.reason;
if (file.status !== "mismatch") return undefined;
const short = (d: string | undefined) => (d ? `${d.slice(0, 23)}…` : "—");
return `expected ${short(file.expectedDigest)}, got ${short(file.actualDigest)}`;
}
/** The file name a manifest URI ends in, for a list that must fit 40 columns. */
function fileNameOf(uri: string): string {
const cut = uri.lastIndexOf("/");
@ -254,6 +272,14 @@ export function SkillsTab({
selectedSkill && selectedSkill.resources !== DYNAMIC_RESOURCES
? selectedSkill.resources
: [];
// Compared on normalized identity, like every other URI comparison here, so a
// manifest entry written in an equivalent form is not reported twice.
const manifestIdentities = new Set(
manifest.map((resource) => skillUriIdentity(resource.uri)),
);
const extraReportFiles = (activeReport?.files ?? []).filter(
(file) => !manifestIdentities.has(skillUriIdentity(file.uri)),
);
return (
<Box flexDirection="row" width={width} height={height}>
@ -410,15 +436,48 @@ export function SkillsTab({
<Text dimColor> ({resource.size} B)</Text>
) : null}
</Text>
{fileReport?.reason && (
{fileReport && failureDetail(fileReport) && (
<Box paddingLeft={4} flexShrink={0}>
<Text color="red">{fileReport.reason}</Text>
<Text color="red">{failureDetail(fileReport)}</Text>
</Box>
)}
</Box>
);
})}
{/* A report can carry a file the MANIFEST does not — a dynamic
skill has no rows at all, yet a failed read of its own
SKILL.md is recorded so the failure is visible. Rendering only
manifest rows left "Verification FAILED" with no diagnosis
anywhere on screen (Copilot). */}
{extraReportFiles.length > 0 && (
<>
<Box marginTop={1} flexShrink={0}>
<Text bold>Read failures:</Text>
</Box>
{extraReportFiles.map((file, idx) => (
<Box
key={`extra-${idx}`}
paddingLeft={2}
flexShrink={0}
flexDirection="column"
>
<Text>
<Text color={FILE_COLOR[file.status] ?? "white"}>
{FILE_MARK[file.status] ?? "?"}{" "}
</Text>
{fileNameOf(file.uri)}
</Text>
{failureDetail(file) && (
<Box paddingLeft={4} flexShrink={0}>
<Text color="red">{failureDetail(file)}</Text>
</Box>
)}
</Box>
))}
</>
)}
{activeReport && activeReport.frontmatter.length > 0 && (
<>
<Box marginTop={1} flexShrink={0}>

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

@ -380,6 +380,26 @@ const ResourceNameCaption = Text.withProps({
maw: "50%",
});
/** A read that failed, in the Directory section. */
const ReadFailureAlert = Alert.withProps({
color: "red",
variant: "light",
title: "Read failed",
});
/**
* The directory-vs-manifest divergence banner. Yellow: the server is not
* necessarily wrong — its listing may simply be newer than the held entry.
*/
const UnlistedChildrenAlert = Alert.withProps({
color: "yellow",
variant: "light",
title: "This directory lists files the entry does not",
});
/** The em dash standing in for a verdict that does not apply to a row. */
const NoVerdictText = Text.withProps({ size: "xs", c: "dimmed" });
const IssueStack = Stack.withProps({
gap: "xs",
});
@ -1984,9 +2004,7 @@ export function SkillsScreen({
</Group>
</SectionControlsRow>
{directoryError !== undefined && (
<Alert color="red" variant="light" title="Read failed">
{directoryError}
</Alert>
<ReadFailureAlert>{directoryError}</ReadFailureAlert>
)}
{/* Stated in prose the first time the two views
disagree, because the per-row chip alone does not say
@ -1994,12 +2012,7 @@ export function SkillsScreen({
re-approval" is what SEP-2640 asks a host to present
here, rather than a read error. */}
{unlistedChildren.length > 0 && (
<Alert
color="yellow"
variant="light"
title="This directory lists files the entry does not"
data-testid="skill-directory-unlisted"
>
<UnlistedChildrenAlert data-testid="skill-directory-unlisted">
The server is serving {unlistedChildren.length} file
{unlistedChildren.length === 1 ? "" : "s"} here that
the held <Code>skills/list</Code> entry does not
@ -2009,7 +2022,7 @@ export function SkillsScreen({
verification failure equivalent to a digest mismatch.
Re-fetch the entry with <Code>skills/get</Code> to see
whether the skill has changed.
</Alert>
</UnlistedChildrenAlert>
)}
{directoryChildren !== undefined &&
(directoryChildren.length === 0 ? (
@ -2074,9 +2087,7 @@ export function SkillsScreen({
<Table.Td>{child.mimeType ?? "—"}</Table.Td>
<Table.Td>
{isDir || isDynamic ? (
<Text size="xs" c="dimmed">
—
</Text>
<NoVerdictText>—</NoVerdictText>
) : (
<CountBadge
color={listed ? "green" : "yellow"}

27
clients/web/src/test/core/mcp/skillFile.test.ts
View file

@ -96,6 +96,33 @@ describe("parseSkillFrontmatter (#2248)", () => {
expect(parseSkillFrontmatter("# just a comment")).toEqual({ fields: {} });
});
it("reports an explicit null scalar as an error, not as no fields", () => {
// `null` and `~` parse to the same value an EMPTY block does, but only the
// empty one is a degenerate mapping — returning `{ fields: {} }` for an
// explicit null scalar contradicts this function's own contract (Copilot).
for (const src of ["null", "~", " null ", "# lead\nnull"]) {
expect(parseSkillFrontmatter(src)).toEqual({
error: expect.stringContaining("mapping"),
});
}
});
it("still reads a comment-only block as a mapping of no fields", () => {
// The distinction is made on the SOURCE, so this must not regress.
expect(parseSkillFrontmatter("# just a comment\n\n # another")).toEqual({
fields: {},
});
});
it("does not mistake a leading # inside a value for a comment", () => {
// Comments are stripped only at the start of a line; a `#` inside a value
// is part of it, and treating it as a comment would call a block with real
// content empty.
expect(parseSkillFrontmatter('name: "#hashtag"')).toEqual({
fields: { name: "#hashtag" },
});
});
it("reports a scalar block as an error rather than as no fields", () => {
// `just a string` parses successfully as a scalar. Reporting it as an
// empty mapping would present a malformed file as one that merely omitted

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

@ -1005,6 +1005,29 @@ describe("checkSkillFrontmatterMatch (#2248)", () => {
expect(both[1].message).toContain("-Infinity");
});
it("cannot be fooled by a listing that looks like an encoding", () => {
// The regression this guards: encoding non-finite numbers as a sentinel
// object let a listing whose value genuinely WAS that object alias it and
// match a served `.nan` (Copilot). The comparison is structural now, so
// there is no encoding to alias.
const issues = checkSkillFrontmatterMatch(
entry({ x: { "#non-finite": "NaN" } }),
file("x: .nan"),
);
expect(issues).toHaveLength(1);
expect(issues[0].code).toBe("frontmatter-mismatch");
});
it("still matches a listing object that equals the served mapping", () => {
// …and the guard must not make a genuine agreement look like a difference.
expect(
checkSkillFrontmatterMatch(
entry({ x: { "#non-finite": "NaN" } }),
file('x:\n "#non-finite": NaN'),
),
).toEqual([]);
});
it("still matches a null the served file also writes as null", () => {
// The fix must not turn a genuine agreement into a finding.
expect(

28
core/mcp/skillFile.ts
View file

@ -93,6 +93,21 @@ export type ParsedFrontmatter =
* everything. An *empty* block (`fields: {}`) is a different fact and is
* reported as a successful parse of nothing.
*/
/**
* Whether a frontmatter block holds anything but whitespace and comments.
*
* Comments are stripped only from the start of a line: a `#` inside a value is
* part of that value, and treating it as a comment would call a block with real
* content empty. That is the safe direction — mistaking content for emptiness
* here would turn a malformed document back into "no fields", which is the bug
* this exists to prevent.
*/
function hasContent(yamlText: string): boolean {
return yamlText
.split("\n")
.some((line) => line.trim() !== "" && !line.trimStart().startsWith("#"));
}
export function parseSkillFrontmatter(yamlText: string): ParsedFrontmatter {
let parsed: unknown;
try {
@ -100,9 +115,16 @@ export function parseSkillFrontmatter(yamlText: string): ParsedFrontmatter {
} catch (err) {
return { error: err instanceof Error ? err.message : String(err) };
}
// `null` is what an empty (or comment-only) block parses to — a real, if
// degenerate, mapping of no fields rather than a malformed one.
if (parsed === null || parsed === undefined) return { fields: {} };
// An empty (or comment-only) block parses to `null`, and so does an explicit
// `null` / `~` scalar — but only the first is a degenerate mapping of no
// fields. The second is a non-mapping document, and returning `{ fields: {} }`
// for it contradicts this function's own contract (Copilot). They are told
// apart by the SOURCE, since the parsed value cannot distinguish them.
if (parsed === null || parsed === undefined) {
return hasContent(yamlText)
? { error: "Frontmatter is not a YAML mapping of fields." }
: { fields: {} };
}
if (typeof parsed !== "object" || Array.isArray(parsed)) {
return {
error: "Frontmatter is not a YAML mapping of fields.",

75
core/mcp/skills.ts
View file

@ -561,16 +561,6 @@ function canonicalEntry(entry: SkillEntry): string {
/** Object keys sorted recursively; array ORDER is preserved throughout. */
function canonicalize(value: unknown): unknown {
// YAML can express `.nan` and `.inf`; JSON cannot. `JSON.stringify` turns
// every one of them into `null`, so without this a served `x: .nan` would
// compare EQUAL to a listing declaring `x: null` — a mismatch silently
// reported as agreement (Copilot). The listing side arrived over JSON-RPC and
// can never hold a non-finite number, so one appearing here is always a real
// difference. Rendered as an object, which cannot equal any JSON scalar, and
// which names the value in the finding rather than hiding it.
if (typeof value === "number" && !Number.isFinite(value)) {
return { "#non-finite": String(value) };
}
if (Array.isArray(value)) return value.map(canonicalize);
if (value === null || typeof value !== "object") return value;
return sortKeys(value as Record<string, unknown>);
@ -760,6 +750,65 @@ export async function verifySkillResource(
};
}
/**
* Structural equality for JSON-like values, with YAML's extra scalars handled.
*
* ⚠️ **Comparison is structural rather than serialized, and that is the point.**
* `JSON.stringify` is not injective over what a YAML parser produces: `.nan`,
* `.inf` and `-.inf` all serialize to `null`, so a served `x: .nan` compared
* EQUAL to a listing declaring `x: null` — and to each other. An earlier fix
* encoded non-finite numbers as a sentinel object, which merely moved the
* problem: a listing whose value genuinely *was* that object aliased the
* sentinel and matched a served `.nan` (Copilot). Any encoding into the value
* space can be aliased by a document containing the encoding, so there is no
* sentinel here at all.
*
* `Object.is` on the number path is what makes it work: it holds `NaN` equal to
* `NaN`, keeps `Infinity` and `-Infinity` distinct, and never equates either
* with `null`.
*/
function jsonLikeEqual(a: unknown, b: unknown): boolean {
if (typeof a === "number" || typeof b === "number") return Object.is(a, b);
if (a === null || b === null) return a === b;
if (typeof a !== "object" || typeof b !== "object") return Object.is(a, b);
const aArray = Array.isArray(a);
if (aArray !== Array.isArray(b)) return false;
if (aArray) {
const x = a as unknown[];
const y = b as unknown[];
// Array ORDER is significant — a YAML sequence is ordered.
return x.length === y.length && x.every((v, i) => jsonLikeEqual(v, y[i]));
}
const x = a as Record<string, unknown>;
const y = b as Record<string, unknown>;
const keys = Object.keys(x);
// Key order is not meaningful in either JSON or YAML, so only the key SET and
// the values matter.
return (
keys.length === Object.keys(y).length &&
keys.every((k) => Object.hasOwn(y, k) && jsonLikeEqual(x[k], y[k]))
);
}
/**
* A frontmatter value as it should READ in a finding.
*
* `JSON.stringify` renders every non-finite number as `null`, which would print
* "the listing says null but the served file says null" for a real difference.
* Only the display is special-cased; the comparison above never goes through a
* string, so this cannot reintroduce an aliasing bug.
*/
function displayValue(value: unknown): string {
if (typeof value === "number" && !Number.isFinite(value)) {
return String(value);
}
return JSON.stringify(canonicalize(value), (_key, member: unknown) =>
typeof member === "number" && !Number.isFinite(member)
? `<${String(member)}>`
: member,
);
}
/**
* Compare the fetched `SKILL.md`'s own frontmatter against the frontmatter the
* entry advertised, field by field — the SEP-2640 obligation a digest cannot
@ -848,13 +897,11 @@ export function checkSkillFrontmatterMatch(
});
continue;
}
const listedJson = JSON.stringify(canonicalize(listed));
const servedJson = JSON.stringify(canonicalize(served));
if (listedJson !== servedJson) {
if (!jsonLikeEqual(listed, served)) {
issues.push({
code: "frontmatter-mismatch",
severity: "error",
message: `Field "${field}" differs: the listing says ${listedJson} but the served SKILL.md says ${servedJson}.`,
message: `Field "${field}" differs: the listing says ${displayValue(listed)} but the served SKILL.md says ${displayValue(served)}.`,
resourceUri: entry.uri,
});
}

11
core/mcp/skillsVerification.ts
View file

@ -67,7 +67,16 @@ export interface SkillVerifyReport {
* frontmatter discrepancy.
*/
frontmatter: SkillIssue[];
/** One entry per manifest file, in manifest order. Empty for `"dynamic"`. */
/**
* One entry per manifest file, in manifest order.
*
* ⚠️ **Not necessarily empty for a `"dynamic"` skill.** Such a skill has no
* manifest rows, but a failed read of its own `SKILL.md` — the file the
* mandatory frontmatter comparison needs — is recorded here as a synthetic
* `read-error` row against the entry's URI, so the failure is visible and
* fails the report rather than passing silently. A consumer must not assume
* `files` mirrors the manifest one-for-one (Copilot).
*/
files: SkillFileReport[];
/**
* False when anything the SEP makes a MUST was broken: an error-severity