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 #2251

Browse source 
- skills.ts: validate `frontmatter.name` against the Agent Skills format
  (1–64 lowercase alphanumerics and hyphens, no leading, trailing or
  consecutive hyphen). Checking only for non-emptiness let `Bad Name` read
  as "Conforms" — and the name is not decorative: it must equal the URI
  path segment, so a name that cannot appear in a URI is a contradiction
  the entry cannot satisfy. Suppressed when the name is absent entirely,
  which `missing-name` already reports.
- skills.ts: detect duplicates on the NORMALIZED identity. Containment and
  the read that fetches the bytes both treat normalized-equivalents as one
  resource, so `skill://demo/SKILL.md` and `skill://demo/x/../SKILL.md`
  were passing as two distinct files while naming one. The finding still
  reports the raw URI, and two different unparseable URIs stay distinct.
- SkillsScreen: run `checkSkillConformance` on the fetched `skills/get`
  entry and treat an error — or an entry answering for a different URI —
  as invalid (red), not as "a different snapshot" (yellow). A fresh
  point-in-time read excuses a CHANGE; it does not excuse a violation, and
  answering with another skill is never a refresh of the one requested.

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-05 00:09:56 -04:00
commit 9d162f36c6
4 changed files with 208 additions and 18 deletions
Download patch file Download diff file Expand all files Collapse all files

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

@ -501,6 +501,39 @@ describe("SkillsScreen", () => {
expect(screen.getByTestId("skills-get-result")).toBeInTheDocument();
});
it("calls a non-conforming skills/get entry invalid, not a new snapshot", async () => {
// A fresh snapshot excuses a CHANGE; it does not excuse a violation. An
// entry missing a digest is invalid whether or not the skill moved on.
const user = userEvent.setup();
const onGetSkill = vi.fn().mockResolvedValue({
...CLEAN_SKILL,
resources: [{ uri: "skill://data-analysis/SKILL.md" }],
});
renderWithMantine(<ControlledSkillsScreen onGetSkill={onGetSkill} />);
await user.click(screen.getByText("data-analysis"));
await user.click(
screen.getByRole("button", { name: /Fetch with skills\/get/ }),
);
const result = await screen.findByTestId("skills-get-result");
expect(result).toHaveAttribute("data-verdict", "invalid");
expect(result).toHaveTextContent("missing-digest");
});
it("calls a skills/get answer for a different uri invalid", async () => {
// Answering with another skill is never a valid refresh of the one asked
// for, however much that other skill may have changed.
const user = userEvent.setup();
const onGetSkill = vi.fn().mockResolvedValue(TAMPERED_SKILL);
renderWithMantine(<ControlledSkillsScreen onGetSkill={onGetSkill} />);
await user.click(screen.getByText("data-analysis"));
await user.click(
screen.getByRole("button", { name: /Fetch with skills\/get/ }),
);
const result = await screen.findByTestId("skills-get-result");
expect(result).toHaveAttribute("data-verdict", "invalid");
expect(result).toHaveTextContent("different URI");
});
it("reports a failed skills/get", async () => {
const user = userEvent.setup();
const onGetSkill = vi.fn().mockRejectedValue(new Error("-32602"));

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

@ -97,7 +97,12 @@ interface FetchedEntryState {
/** The click this result belongs to — see {@link PreviewState.attempt}. */
attempt?: number;
entry?: SkillEntry;
/** Conformance findings for the FETCHED entry, in its own right. */
issues?: SkillIssue[];
/** True when the fetched entry describes the same skill as the listed one. */
matches?: boolean;
/** True when the fetched entry is for a different URI than was asked for. */
wrongUri?: boolean;
message?: string;
}
@ -530,10 +535,21 @@ export function SkillsScreen({
);
void onGetSkill(selected.uri)
.then((entry) => {
// Compared semantically against what `skills/list` advertised — see
// `skillEntriesMatch` for why a `JSON.stringify` comparison would
// report key order and manifest order as differences.
writeFetched({ entry, matches: skillEntriesMatch(entry, selected) });
// The fetched entry is checked ON ITS OWN before being compared. A
// snapshot is allowed to have moved on, but it is not allowed to be
// non-conforming: an entry missing a digest is invalid whether or not
// the skill changed, and an entry for a DIFFERENT uri is never a valid
// refresh of the one that was asked for. Only a conforming entry with
// the same identity gets the benign "the snapshot moved" reading.
writeFetched({
entry,
issues: checkSkillConformance(entry),
wrongUri: entry.uri !== selected.uri,
// Compared semantically — see `skillEntriesMatch` for why a
// `JSON.stringify` comparison would report key order and manifest
// order as differences.
matches: skillEntriesMatch(entry, selected),
});
})
.catch((err: unknown) => {
writeFetched({
@ -543,6 +559,16 @@ export function SkillsScreen({
}, [manifestKey, onGetSkill, selected]);
const fetched = fetchedEntry.key === manifestKey ? fetchedEntry : undefined;
// `invalid` outranks the snapshot comparison: an entry that breaks a
// requirement, or answers for a different URI, is wrong regardless of
// whether the skill it describes has changed since the listing.
const fetchedVerdict =
fetched?.wrongUri ||
(fetched?.issues ?? []).some((issue) => issue.severity === "error")
? "invalid"
: fetched?.matches
? "matches"
: "differs";
const batchRunning = batches.has(manifestKey);
const preview =
@ -810,24 +836,45 @@ export function SkillsScreen({
{fetched?.entry !== undefined && (
<Alert
data-testid="skills-get-result"
// 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"}
data-verdict={fetchedVerdict}
// Red only for a genuine violation — a non-conforming entry,
// or one answering with a different URI, neither of which a
// fresh snapshot excuses. A conforming entry that merely
// moved on is yellow: `skills/get` IS a point-in-time read,
// so a changed skill legitimately differs.
color={
fetchedVerdict === "invalid"
? "red"
: fetchedVerdict === "matches"
? "green"
: "yellow"
}
title={
fetched.matches
? "skills/get matches skills/list"
: "skills/get returned a different snapshot"
fetchedVerdict === "invalid"
? "skills/get returned a non-conforming entry"
: fetchedVerdict === "matches"
? "skills/get matches skills/list"
: "skills/get returned a different snapshot"
}
>
<Stack gap={2}>
<Text size="sm">
{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."}
{fetchedVerdict === "invalid"
? fetched.wrongUri
? "This entry is for a different URI than the one requested, which is never a valid refresh of it."
: "This entry breaks a requirement of its own, so the difference is not simply a newer snapshot."
: fetchedVerdict === "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.matches && (
{(fetched.issues ?? [])
.filter((issue) => issue.severity === "error")
.map((issue, index) => (
<MonoCaption key={`${index}:${issue.code}`}>
{issue.code}: {issue.message}
</MonoCaption>
))}
{fetchedVerdict !== "matches" && (
<ContentViewer
block={{
type: "text",

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

@ -266,6 +266,52 @@ describe("checkSkillConformance", () => {
expect(issues[0].severity).toBe("error");
});
it("reports a name that is not a valid Agent Skills name", () => {
// The name is not decorative: it must equal the URI path segment, so a
// name that cannot appear in a URI is a contradiction the entry cannot
// satisfy. Checking only for non-emptiness let these read as "Conforms".
for (const name of [
"Bad Name",
"UPPER",
"-leading",
"trailing-",
"double--hyphen",
"under_score",
"a".repeat(65),
]) {
const issues = checkSkillConformance(
entry({ frontmatter: { name, description: "d" } }),
);
expect(issues.map((i) => i.code)).toContain("malformed-name");
expect(issues.find((i) => i.code === "malformed-name")?.severity).toBe(
"error",
);
}
});
it("accepts the names the Agent Skills format allows", () => {
for (const name of ["a", "demo", "data-analysis", "a1-b2-c3"]) {
const issues = checkSkillConformance(
entry({
uri: `skill://${name}/SKILL.md`,
frontmatter: { name, description: "d" },
resources: [
{ uri: `skill://${name}/SKILL.md`, digest: DIGEST, size: 1 },
],
}),
);
expect(issues.map((i) => i.code)).not.toContain("malformed-name");
}
});
it("does not report a malformed name when there is no name at all", () => {
// `missing-name` already says it; two findings would read as two defects.
const issues = checkSkillConformance(
entry({ frontmatter: { description: "d" } }),
);
expect(issues.map((i) => i.code)).toEqual(["missing-name"]);
});
it("reports a missing description as an error", () => {
// SEP-2640 requires `description`, so an absent one is a format violation
// and must not read as "0 errors" in the conformance summary.
@ -385,6 +431,37 @@ describe("checkSkillConformance", () => {
expect(issues.map((i) => i.code)).toEqual(["resource-outside-skill-root"]);
});
it("detects a duplicate that differs only before normalization", () => {
// Containment and the read that fetches the bytes both treat these as one
// resource, so the uniqueness check must too — otherwise a manifest naming
// one file twice passes as two distinct files.
const issues = checkSkillConformance(
entry({
resources: [
{ uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 },
{ uri: "skill://demo/x/../SKILL.md", digest: DIGEST, size: 20 },
],
}),
);
expect(issues.map((i) => i.code)).toEqual(["duplicate-resource"]);
// Reported against the raw URI, so the diagnostic points at what the
// server actually sent.
expect(issues[0].resourceUri).toBe("skill://demo/x/../SKILL.md");
});
it("does not fold two different unparseable URIs into one duplicate", () => {
const issues = checkSkillConformance(
entry({
resources: [
{ uri: "skill://demo/SKILL.md", digest: DIGEST, size: 20 },
{ uri: "not a uri", digest: DIGEST, size: 1 },
{ uri: "also not a uri", digest: DIGEST, size: 1 },
],
}),
);
expect(issues.map((i) => i.code)).not.toContain("duplicate-resource");
});
it("reports a manifest entry outside the skill root", () => {
const issues = checkSkillConformance(
entry({

37
core/mcp/skills.ts
View file

@ -55,6 +55,19 @@ export const SKILL_FILE_SUFFIX = "/SKILL.md";
/** `sha256:` followed by exactly 64 lowercase hex characters. */
const DIGEST_PATTERN = /^sha256:[0-9a-f]{64}$/;
/**
* The Agent Skills name format SEP-2640 requires of `frontmatter.name`: 1–64
* characters of lowercase alphanumerics and hyphens, with no leading, trailing
* or consecutive hyphen.
*
* Checking only that the name is non-empty let `Bad Name` reach the UI as
* "Conforms" — and the name is not decorative here: it must equal the URI path
* segment, so a name that cannot appear in a URI is a contradiction the entry
* cannot satisfy.
*/
const SKILL_NAME_PATTERN = /^[a-z0-9]+(?:-[a-z0-9]+)*$/;
const SKILL_NAME_MAX_LENGTH = 64;
/**
* What the server declared under `io.modelcontextprotocol/skills`. The only
* sub-option SEP-2640 defines is `directoryRead`, which gates
@ -162,6 +175,7 @@ export function skillDisplayName(entry: SkillEntry): string {
export type SkillIssueCode =
| "dynamic-resources"
| "missing-name"
| "malformed-name"
| "missing-description"
| "malformed-uri"
| "name-path-mismatch"
@ -213,6 +227,15 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] {
severity: "error",
message: "frontmatter.name is required but missing or empty.",
});
} else if (
declaredName.length > SKILL_NAME_MAX_LENGTH ||
!SKILL_NAME_PATTERN.test(declaredName)
) {
issues.push({
code: "malformed-name",
severity: "error",
message: `frontmatter.name "${declaredName}" is not a valid Agent Skills name: 1–${SKILL_NAME_MAX_LENGTH} lowercase alphanumerics and hyphens, with no leading, trailing or consecutive hyphen.`,
});
}
if (!entry.frontmatter.description?.trim()) {
// An error, not a warning: SEP-2640 requires `description` on every skill,
@ -301,7 +324,17 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] {
: undefined;
for (const resource of entry.resources) {
if (seenUris.has(resource.uri)) {
// Compared on the NORMALIZED identity, because everything else here treats
// normalized-equivalents as the same resource — containment does, and so
// does the read that fetches the bytes. On the raw string,
// `skill://demo/SKILL.md` and `skill://demo/x/../SKILL.md` would pass as
// two distinct files while naming one. The raw URI is still what the
// finding reports, so the diagnostic points at what the server actually
// sent. Unparseable URIs fall back to the raw string: they are already
// reported by the root check, and normalizing them all to `undefined`
// would make two different bad URIs look like one duplicate.
const identity = normalizeSkillUri(resource.uri) ?? resource.uri;
if (seenUris.has(identity)) {
issues.push({
code: "duplicate-resource",
severity: "error",
@ -310,7 +343,7 @@ export function checkSkillConformance(entry: SkillEntry): SkillIssue[] {
resourceUri: resource.uri,
});
}
seenUris.add(resource.uri);
seenUris.add(identity);
if (root !== undefined) {
const normalized = normalizeSkillUri(resource.uri);
// An unparseable entry URI is outside the root by construction: nothing