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

Browse source 
Round 10 raised no new inline comments; three suppressed findings, two fixed
and one declined with a reason.

**`Object.is` held `0` and `-0` distinct**, so a listing carrying JSON `0`
against a served YAML `-0` produced a mismatch — reported as "the listing
says 0 but the served SKILL.md says 0", a false finding with an
unintelligible explanation. JSON does not distinguish them, so neither may
this: `===` for finite numbers, `Object.is` kept only for the non-finite
cases it was introduced for.

**Browsing a supporting file erased an observed failure.** The frontmatter
check ran off whatever the viewer was showing, so opening a second file made
`showingSkillMd` false and dropped the finding and its error count, with the
skill unchanged. The entry's text is skill-scoped now, written by whichever
read produces it first — the on-selection preview or a verification — and
invalidated only with `manifestKey`. That also subsumes the earlier
same-fetch requirement rather than sitting beside it.

**Declined: the second stderr line on exit 7.** `--verify` writes its summary
and then the ordinary `ErrorEnvelope` that EVERY non-zero exit writes, which
is the documented contract and exactly what `--strict` does. Suppressing it
would make this one command's failure output unparseable by a caller
branching on `.code`. What was wrong is the README, which implied stderr
carries only the summary; it now states both lines and why.

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:33:34 -04:00
commit 50441f4d85
3 changed files with 128 additions and 13 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -2458,6 +2458,42 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => {
);
});
it("keeps a frontmatter finding when the reader opens another file", async () => {
// The check ran off whatever the viewer was showing, so opening a
// supporting file made `showingSkillMd` false and silently dropped the
// finding AND its error count — erasing an observed conformance failure
// because the reader browsed a second file, with the skill unchanged
// (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.getByTestId("skill-frontmatter-issues"),
).toBeInTheDocument(),
);
// Open a supporting file: the finding is about the SKILL, not the view.
await user.click(
screen.getByRole("button", {
name: "skill://data-analysis/reference.md",
}),
);
await waitFor(() =>
expect(readFixtureFile).toHaveBeenCalledWith(
"skill://data-analysis/reference.md",
),
);
expect(screen.getByTestId("skill-frontmatter-issues")).toBeInTheDocument();
expect(
screen.getByRole("button", { name: /Conformance/ }),
).toHaveTextContent("1 error(s)");
});
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

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

@ -882,7 +882,7 @@ export function SkillsScreen({
});
const fileStates = verification.key === manifestKey ? verification.files : {};
const verifiedEntryText =
const entrySourceText =
verification.key === manifestKey ? verification.entryText : undefined;
/**
@ -1037,7 +1037,7 @@ export function SkillsScreen({
* whichever one happened to be current when this callback was created.
*/
const showResource = useCallback(
(uri: string, key: string) => {
(uri: string, key: string, isEntryUri = false) => {
const attempt = (nextAttempt.current += 1);
// A click handler cannot await, and this chain terminates in its own
// `catch` that surfaces the message in the viewer. Both arms go through
@ -1056,7 +1056,32 @@ export function SkillsScreen({
// to announce the previous one for as long as the read takes.
writePreview({ uri });
void onReadSkillFile(uri)
.then((contents) => writePreview({ uri, contents }))
.then((contents) => {
writePreview({ uri, contents });
// A successful read of the skill's OWN file is recorded in the
// skill-scoped slot, so the frontmatter verdict it produces outlives
// the reader opening a supporting file — see
// `VerificationState.entryText`. Only on success, and only for that
// file: a failed or unrelated read must not overwrite an answer.
if (isEntryUri) {
let text: string | undefined;
try {
text = bytesToText(skillFileBytes(contents));
} catch {
return; // neither text nor blob; the viewer reports it
}
setVerification((prev) =>
prev.key !== null && prev.key !== key
? prev
: {
...prev,
key,
files: prev.key === key ? prev.files : {},
entryText: text,
},
);
}
})
.catch((err: unknown) => {
writePreview({
uri,
@ -1117,7 +1142,7 @@ export function SkillsScreen({
}
if (autoReadKey.current === manifestKey) return;
autoReadKey.current = manifestKey;
showResource(selectedUri, manifestKey);
showResource(selectedUri, manifestKey, true);
}, [manifestKey, selectedUri, showResource]);
/**
@ -1453,10 +1478,11 @@ export function SkillsScreen({
*/
const frontmatterIssues = useMemo(() => {
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);
// The skill-scoped text, whichever read produced it — so this verdict
// survives the reader opening another file, and agrees with the digest
// verdict when one verification produced both.
if (entrySourceText !== undefined) {
return checkSkillFrontmatterMatch(selected, entrySourceText);
}
if (!showingSkillMd || preview === undefined) return [];
// Run against the **raw fetched bytes**, not against `previewParts`.
@ -1479,7 +1505,7 @@ export function SkillsScreen({
return [];
}
return checkSkillFrontmatterMatch(selected, text);
}, [selected, showingSkillMd, preview, verifiedEntryText]);
}, [selected, showingSkillMd, preview, entrySourceText]);
/**
* The findings rendered as list items — everything except the two that are
@ -2001,7 +2027,11 @@ export function SkillsScreen({
variant={showing ? "light" : "subtle"}
aria-current={showing ? "true" : undefined}
onClick={() =>
showResource(resource.uri, manifestKey)
showResource(
resource.uri,
manifestKey,
isSelfResource(resource),
)
}
>
{resource.uri}

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

@ -1028,6 +1028,33 @@ describe("checkSkillFrontmatterMatch (#2248)", () => {
).toEqual([]);
});
it("does not report -0 against 0 as a difference", () => {
// `Object.is` holds them distinct; JSON does not (`JSON.stringify(-0)` is
// `"0"`), so this produced a false finding whose message read "the listing
// says 0 but the served SKILL.md says 0" (Copilot).
expect(checkSkillFrontmatterMatch(entry({ a: 0 }), file("a: -0"))).toEqual(
[],
);
expect(checkSkillFrontmatterMatch(entry({ a: -0 }), file("a: 0"))).toEqual(
[],
);
});
it("still holds NaN equal to NaN after the -0 fix", () => {
// `===` alone would hold NaN unequal to itself, which is why the two
// comparisons are combined rather than either used on its own.
expect(
checkSkillFrontmatterMatch(entry({ a: null }), file("a: .nan")),
).toHaveLength(1);
// Two served non-finite values of the SAME kind still agree with each
// other, so the combination did not trade one false finding for another.
const parsedBoth = checkSkillFrontmatterMatch(
entry({ a: 1 }),
file("a: 1"),
);
expect(parsedBoth).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(