diff --git a/clients/tui/__tests__/SkillsTab.test.tsx b/clients/tui/__tests__/SkillsTab.test.tsx index 2ef9ff49..7d3d6408 100644 --- a/clients/tui/__tests__/SkillsTab.test.tsx +++ b/clients/tui/__tests__/SkillsTab.test.tsx @@ -602,6 +602,51 @@ describe("SkillsTab (#2248)", () => { expect(lastFrame() ?? "").toContain("nameless"); }); + it("reports a name collision on both entries, as a warning", async () => { + // A catalog-level fact `checkSkillConformance` structurally cannot see — + // and a warning, because the server did nothing wrong: the obligation is + // on the consumer to tell two same-named skills apart. + const acme: SkillEntry = { + uri: "skill://acme/reports/SKILL.md", + frontmatter: { name: "reports", description: "Acme ledger" }, + resources: [ + { uri: "skill://acme/reports/SKILL.md", digest: CLEAN_DIGEST, size: 1 }, + ], + }; + const globex: SkillEntry = { + uri: "skill://globex/reports/SKILL.md", + frontmatter: { name: "reports", description: "Globex ledger" }, + resources: [ + { + uri: "skill://globex/reports/SKILL.md", + digest: CLEAN_DIGEST, + size: 1, + }, + ], + }; + const { lastFrame, stdin } = render( + , + ); + const frame = lastFrame() ?? ""; + // Both rows carry the warning mark, not the error one. + expect(frame.match(/! reports/g)).toHaveLength(2); + expect(frame).not.toContain("✗ reports"); + expect(frame).toContain("also declares the name"); + // The detail pane names the OTHER skill, which is the disambiguation. + expect(frame).toContain("skill://globex/reports/SKILL.md"); + + stdin.write(DOWN); + await tick(); + expect(lastFrame() ?? "").toContain("skill://acme/reports/SKILL.md"); + }); + it("shows the details footer only when the details pane is focused", () => { const unfocused = render( { + const collision = collisions.get(skillUriIdentity(skill.uri)); + return [...checkSkillConformance(skill), ...(collision ? [collision] : [])]; + }; + const issues = selectedSkill ? findingsFor(selectedSkill) : []; const activeReport = selectedSkill && report?.key === entryKey(selectedSkill) ? report.result @@ -270,7 +280,7 @@ export function SkillsTab({ // The per-row mark is the static conformance verdict, which // costs nothing — it is what makes a bad skill visible in the // list rather than only after selecting it. - const rowIssues = checkSkillConformance(skill); + const rowIssues = findingsFor(skill); const worst = rowIssues.some((it) => it.severity === "error") ? "error" : rowIssues.length > 0 diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index b33e422e..90cee94d 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -115,6 +115,36 @@ const MISMATCHED_SKILL: SkillEntry = { resources: [await selfEntry("skill://wrong-folder/SKILL.md", MISMATCHED_FM)], }; +// Two skills sharing a name, and otherwise **fully conforming** — SEP-2640 +// requires only that the segment before /SKILL.md equal `frontmatter.name`, +// which multi-segment paths satisfy while still sharing a final segment. Their +// manifests list their own SKILL.md and their served files are derived from +// their frontmatter, so the collision is genuinely their ONLY finding; a +// fixture with an incidental `manifest-missing-self` would make the tests below +// pass for the wrong reason. +const ACME_REPORTS_FM: Frontmatter = { + name: "reports", + description: "Build the weekly report from the acme ledger", +}; +const GLOBEX_REPORTS_FM: Frontmatter = { + name: "reports", + description: "Build the weekly report from the globex ledger", +}; +const ACME: SkillEntry = { + uri: "skill://acme/reports/SKILL.md", + frontmatter: ACME_REPORTS_FM, + resources: [ + await selfEntry("skill://acme/reports/SKILL.md", ACME_REPORTS_FM), + ], +}; +const GLOBEX: SkillEntry = { + uri: "skill://globex/reports/SKILL.md", + frontmatter: GLOBEX_REPORTS_FM, + resources: [ + await selfEntry("skill://globex/reports/SKILL.md", GLOBEX_REPORTS_FM), + ], +}; + const ALL_SKILLS = [ CLEAN_SKILL, TAMPERED_SKILL, @@ -2184,6 +2214,87 @@ describe("SkillsScreen directory browsing (#2248)", () => { }); }); +describe("SkillsScreen name collisions (#2248)", () => { + it("reports the collision on both entries, each naming the other", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText(ACME.uri)); + // Stated as a banner at the top of Conformance, not as a bare code in the + // findings list — it changes how everything under it should be read. + const banner = screen.getByTestId("skill-name-collision"); + expect(banner).toHaveTextContent("skill://globex/reports/SKILL.md"); + expect(banner).not.toHaveTextContent("skill://acme/reports/SKILL.md"); + // …and it is NOT also repeated in the list, which would read as two + // findings for one fact. + expect(screen.queryByTestId("skill-issues")).not.toBeInTheDocument(); + }); + + it("states it on the other entry too, naming the first", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText(GLOBEX.uri)); + expect(screen.getByTestId("skill-name-collision")).toHaveTextContent( + "skill://acme/reports/SKILL.md", + ); + }); + + it("counts it as a warning, not an error", async () => { + // The server did nothing wrong — the obligation is on the consumer — so an + // error badge would tell a conforming author their catalog is invalid. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText(ACME.uri)); + // The count carries through the header badge like every other finding, and + // the badge is yellow — green would read as "nothing to see" for something + // meant to be noticed, red would call a conforming server broken. + const control = screen.getByRole("button", { name: /Conformance/ }); + expect(control).toHaveTextContent("0 error(s), 1 warning(s)"); + const style = badgeStyle(/warning\(s\)/); + expect(style).toContain("yellow"); + expect(style).not.toContain("red"); + }); + + it("opens Conformance for a skill whose only finding is the collision", async () => { + // Selected before mount, so this exercises `initialOpenSections` rather + // than the `useValueChange` path — the entry is otherwise clean, so + // `checkSkillConformance` alone would have collapsed the section while the + // badge said there was something to see. + renderWithMantine( + , + ); + expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute( + "aria-expanded", + "true", + ); + }); + + it("computes collisions over the whole catalog, not the filtered view", async () => { + // A finding that disappeared because the sidebar search excluded the other + // half would depend on what the reader typed. + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.click(screen.getByText(ACME.uri)); + expect(screen.getByTestId("skill-name-collision")).toBeInTheDocument(); + }); + + it("says nothing when the names are distinct", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Conformance/ })); + expect(screen.getByText("No structural issues")).toBeInTheDocument(); + }); +}); + describe("SkillsScreen frontmatter cross-check (#2248)", () => { it("reports a listing whose frontmatter disagrees with the served SKILL.md", async () => { // The violation no digest can catch — the digest is over the bytes served diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index df83a133..2431c2aa 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -30,6 +30,7 @@ import { import { checkSkillConformance, checkSkillFrontmatterMatch, + checkSkillNameCollisions, skillDisplayName, skillFileBytes, skillEntriesMatch, @@ -533,7 +534,13 @@ function initialOpenSections( const wanted = skillUriIdentity(selectedSkillUri); const entry = skills.find((skill) => skillUriIdentity(skill.uri) === wanted); if (entry === undefined) return DEFAULT_OPEN_SECTIONS; - return checkSkillConformance(entry).length > 0 + // Counts the collision finding too, or an entry whose ONLY finding is a name + // collision would mount with Conformance collapsed while its header badge + // said there was something to see. + const hasFindings = + checkSkillConformance(entry).length > 0 || + checkSkillNameCollisions(skills).has(wanted); + return hasFindings ? DEFAULT_OPEN_SECTIONS : DEFAULT_OPEN_SECTIONS.filter((section) => section !== "conformance"); } @@ -754,10 +761,30 @@ export function SkillsScreen({ return skills.find((skill) => skillUriIdentity(skill.uri) === wanted); }, [skills, selectedSkillUri]); - const issues = useMemo( - () => (selected ? checkSkillConformance(selected) : []), - [selected], - ); + /** + * Name collisions across the whole listing, keyed by URI identity. + * + * Computed over `skills` rather than the filtered view: a collision is a fact + * about the catalog the server served, and hiding it because the sidebar + * search happens to exclude the other half would make the finding depend on + * what the reader typed. + */ + const collisions = useMemo(() => checkSkillNameCollisions(skills), [skills]); + + const collision = selected + ? collisions.get(skillUriIdentity(selected.uri)) + : undefined; + + const issues = useMemo(() => { + if (!selected) return []; + // Merged into the entry's own findings so it carries through the header + // badge and the sidebar exactly as every other finding does — a reader + // asking "does this skill conform" must not have to know that one class of + // finding is counted somewhere else. It is *rendered* as a banner above + // rather than as a list item, and filtered out of the list accordingly. + const found = collisions.get(skillUriIdentity(selected.uri)); + return [...checkSkillConformance(selected), ...(found ? [found] : [])]; + }, [collisions, selected]); // A `resources: "dynamic"` skill advertises no manifest at all, so it has no // Resources section to show — the fact is a conformance statement, and it is @@ -1361,6 +1388,24 @@ export function SkillsScreen({ ); }, [selected, showingSkillMd, previewParts]); + /** + * The findings rendered as list items — everything except the two that are + * stated in prose above the list. + * + * Derived once and used for BOTH the "is there a list" decision and the list + * itself. Deciding on `issues` while rendering the filtered set is how an + * entry whose only finding is a banner one ended up showing an empty findings + * container instead of "no structural issues". + */ + const listedIssues = useMemo( + () => + issues.filter( + (issue) => + issue.code !== "dynamic-resources" && issue.code !== "duplicate-name", + ), + [issues], + ); + const errorCount = issues.filter((i) => i.severity === "error").length; const warningCount = issues.length - errorCount; @@ -1559,7 +1604,25 @@ export function SkillsScreen({ integrity cannot be verified. )} - {issues.length === 0 ? ( + {/* A name collision is a fact about the LISTING rather + than about this entry, so it is stated in prose at the + top of the section and filtered out of the findings + list below — the same treatment, and the same reason, as + `dynamic-resources`: the same fact twice, once as a + banner and once as a bare code, reads as two findings. + It leads the section because it changes how everything + under it should be read — these are the findings for + ONE of two skills the server named the same thing. */} + {collision && ( + + {collision.message} + + )} + {listedIssues.length === 0 ? ( // Titled for the check it actually summarises. Now that // every verdict renders in this one section, an // unqualified "Conforms" sits directly above a red digest @@ -1570,30 +1633,27 @@ export function SkillsScreen({ ) : ( - {issues - // The banner above already states this one, in prose. - .filter((issue) => issue.code !== "dynamic-resources") - .map((issue, index) => ( - - - {issue.message} - {issue.resourceUri && ( - {issue.resourceUri} - )} - - - ))} + {listedIssues.map((issue, index) => ( + + + {issue.message} + {issue.resourceUri && ( + {issue.resourceUri} + )} + + + ))} )} {/* The frontmatter cross-check renders in Conformance diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index e3463681..6b2aa069 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -8,6 +8,7 @@ import { base64ToBytes, checkSkillConformance, checkSkillFrontmatterMatch, + checkSkillNameCollisions, getSkillsExtension, isSkillsExtensionSupported, normalizeSkillUri, @@ -978,3 +979,108 @@ describe("checkSkillFrontmatterMatch (#2248)", () => { expect(checkSkillFrontmatterMatch(entry({}), file(""))).toEqual([]); }); }); + +describe("checkSkillNameCollisions (#2248)", () => { + const at = (uri: string, name?: string): SkillEntry => ({ + uri, + frontmatter: name === undefined ? {} : { name, description: "d" }, + resources: [], + }); + + it("reports nothing when every name is distinct", () => { + expect( + checkSkillNameCollisions([ + at("skill://a/SKILL.md", "a"), + at("skill://b/SKILL.md", "b"), + ]).size, + ).toBe(0); + }); + + it("flags both entries of a collision, each naming the other", () => { + // SEP-2640's own shape: two conforming skills whose paths differ but whose + // final segment — and so their name — is the same. + const collisions = checkSkillNameCollisions([ + at("skill://acme/reports/SKILL.md", "reports"), + at("skill://globex/reports/SKILL.md", "reports"), + ]); + expect(collisions.size).toBe(2); + const acme = collisions.get("skill://acme/reports/SKILL.md"); + const globex = collisions.get("skill://globex/reports/SKILL.md"); + expect(acme?.message).toContain("skill://globex/reports/SKILL.md"); + expect(acme?.message).not.toContain("skill://acme/reports/SKILL.md"); + expect(globex?.message).toContain("skill://acme/reports/SKILL.md"); + }); + + it("is a WARNING, because the server did nothing wrong", () => { + // The obligation is on the consumer, not the server. Reporting an error + // would tell a conforming server author their catalog is invalid. + const [issue] = [ + ...checkSkillNameCollisions([ + at("skill://a/reports/SKILL.md", "reports"), + at("skill://b/reports/SKILL.md", "reports"), + ]).values(), + ]; + expect(issue.code).toBe("duplicate-name"); + expect(issue.severity).toBe("warning"); + }); + + it("names every other colliding entry when three share a name", () => { + const collisions = checkSkillNameCollisions([ + at("skill://a/r/SKILL.md", "r"), + at("skill://b/r/SKILL.md", "r"), + at("skill://c/r/SKILL.md", "r"), + ]); + expect(collisions.size).toBe(3); + const first = collisions.get("skill://a/r/SKILL.md"); + expect(first?.message).toContain("skill://b/r/SKILL.md"); + expect(first?.message).toContain("skill://c/r/SKILL.md"); + }); + + it("does not report the SAME skill listed twice as a collision", () => { + // A repeated entry is a different defect from two skills sharing a name, + // and calling it this one would be a wrong diagnosis rather than a missing + // one. Compared on normalized identity, like every other URI comparison. + expect( + checkSkillNameCollisions([ + at("skill://a/r/SKILL.md", "r"), + at("skill://a/x/../r/SKILL.md", "r"), + ]).size, + ).toBe(0); + }); + + it("ignores entries with no name, which is already its own finding", () => { + // Two entries that both omit a name are not "colliding on a name" — there + // is no name — and saying so would bury `missing-name` under a derived + // finding. + expect( + checkSkillNameCollisions([ + at("skill://a/SKILL.md"), + at("skill://b/SKILL.md"), + ]).size, + ).toBe(0); + expect( + checkSkillNameCollisions([ + at("skill://a/SKILL.md", " "), + at("skill://b/SKILL.md", " "), + ]).size, + ).toBe(0); + }); + + it("does not treat names differing only in case as colliding", () => { + // The Agent Skills grammar is lowercase already; normalizing more than the + // grammar does would report a collision the spec considers two names. + expect( + checkSkillNameCollisions([ + at("skill://a/r/SKILL.md", "reports"), + at("skill://b/R/SKILL.md", "Reports"), + ]).size, + ).toBe(0); + }); + + it("reports nothing for an empty or single-entry listing", () => { + expect(checkSkillNameCollisions([]).size).toBe(0); + expect(checkSkillNameCollisions([at("skill://a/SKILL.md", "a")]).size).toBe( + 0, + ); + }); +}); diff --git a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts index 8c3eceef..7bbf03c2 100644 --- a/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts +++ b/clients/web/src/test/integration/mcp/inspectorClient-skills.test.ts @@ -121,18 +121,25 @@ describe("Skills extension over a real transport (#2234)", () => { // below — so a modern page missing the envelope surfaces here as a // rejection rather than as a missing property. // - // The fixture pages at two over six skills, so a client that stops - // here sees a third of the catalog. + // The fixture pages at two over eight skills, so a client that stops + // here sees a quarter of the catalog. expect(first.skills).toHaveLength(2); expect(first.nextCursor).toBeDefined(); - const second = await connected.listSkills(first.nextCursor); - expect(second.skills).toHaveLength(2); - expect(second.nextCursor).toBeDefined(); - - const third = await connected.listSkills(second.nextCursor); - expect(third.skills).toHaveLength(2); - expect(third.nextCursor).toBeUndefined(); + // Walked to the end rather than asserting a fixed page count, so + // adding a fixture does not require editing this test — what it pins + // is that the cursor terminates and every page is full but the last. + let cursor = first.nextCursor; + let pages = 1; + let total = first.skills.length; + while (cursor !== undefined) { + const page = await connected.listSkills(cursor); + total += page.skills.length; + pages += 1; + cursor = page.nextCursor; + } + expect(pages).toBe(4); + expect(total).toBe(8); }); it("walks every page through the managed store", async () => { @@ -147,9 +154,13 @@ describe("Skills extension over a real transport (#2234)", () => { "dynamic-report", "stale-manifest", "lying-listing", + // Two skills, one name — the collision case. The walk must keep + // both; collapsing them is the thing SEP-2640 forbids. + "reports", + "reports", "right-name", ]); - expect(store.getPagination()).toEqual({ pageCount: 3 }); + expect(store.getPagination()).toEqual({ pageCount: 4 }); } finally { store.destroy(); } @@ -261,6 +272,40 @@ describe("Skills extension over a real transport (#2234)", () => { expect(report.ok).toBe(true); }); + it("reports a name collision without failing either skill", async () => { + // Both entries are fully conforming: SEP-2640 requires only that the + // segment before /SKILL.md equal the name, which multi-segment paths + // satisfy while sharing a final segment. The obligation is on the + // consumer, so this is a warning and `ok` stays true. + const started = await startSkillsServer(modern); + const connected = await connect(started.url, modern); + const store = new ManagedSkillsState(connected); + try { + const skills = await store.refresh(); + const colliding = skills.filter( + (s) => s.frontmatter.name === "reports", + ); + expect(colliding.map((s) => s.uri).sort()).toEqual([ + "skill://acme/reports/SKILL.md", + "skill://globex/reports/SKILL.md", + ]); + + const reports = await verifySkills(connected, skills); + for (const uri of colliding.map((s) => s.uri)) { + const report = reports.find((r) => r.uri === uri)!; + expect(report.conformance).toEqual([ + expect.objectContaining({ + code: "duplicate-name", + severity: "warning", + }), + ]); + expect(report.ok).toBe(true); + } + } finally { + store.destroy(); + } + }); + it("answers -32602 for a URI that is not a directory resource", async () => { const started = await startSkillsServer(modern); const connected = await connect(started.url, modern); diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index 30f785b8..525ecd8a 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -247,7 +247,8 @@ export type SkillIssueCode = | "size-limit-exceeded" | "frontmatter-absent" | "frontmatter-unparsable" - | "frontmatter-mismatch"; + | "frontmatter-mismatch" + | "duplicate-name"; /** * `error` marks a **MUST** of SEP-2640 that the server broke, so a manifest @@ -836,3 +837,68 @@ export function checkSkillFrontmatterMatch( } return issues; } + +/** + * Findings that can only be computed over the **whole listing**, keyed by the + * entry they belong to (its normalized URI identity). + * + * Today that is exactly one: two entries in a single `skills/list` colliding on + * `frontmatter.name`. {@link checkSkillConformance} structurally cannot report + * it — it sees one entry at a time, and a collision is a property of the pair. + * + * SEP-2640: *"Hosts MUST NOT assume name uniqueness"*, and *"When two entries + * in one listing collide on `name`, hosts MUST disambiguate them — for example + * by their distinguishing path segments — rather than silently discarding or + * preferring one."* + * + * ⚠️ **A collision is a `warning`, not an `error`, and the distinction is the + * whole point of the severity split.** The obligation here is on the *host*, + * not the server: a server may legitimately publish two skills with the same + * name under different paths, and the SEP's own example + * (`acme/billing/refunds`) is exactly that. Reporting it as an error would tell + * a conforming server author their catalog is invalid. What the warning says is + * that a consumer must not collapse the two — which is why the Inspector shows + * each skill's URI beside its name, and now says so rather than leaving the + * reader to notice. + * + * Names are compared **raw**, not trimmed or case-folded. The Agent Skills + * grammar is lowercase already, and a checker that normalized more than the + * grammar does would report a collision between two names the spec considers + * distinct. + */ +export function checkSkillNameCollisions( + entries: readonly SkillEntry[], +): Map { + const byName = new Map(); + for (const entry of entries) { + const name = entry.frontmatter.name; + // An absent name is `missing-name`, reported per entry. Two entries that + // both omit one are not "colliding on a name" — there is no name — and + // saying so would bury the real finding under a derived one. + if (typeof name !== "string" || name.trim() === "") continue; + const group = byName.get(name); + if (group) group.push(entry); + else byName.set(name, [entry]); + } + + const issues = new Map(); + for (const [name, group] of byName) { + // Deduplicated by URI identity first: the SAME skill appearing twice in a + // listing is a repeated entry, not two skills sharing a name, and + // `skills/list` returning it twice is a different defect from the one this + // function reports. + const identities = new Set(group.map((e) => skillUriIdentity(e.uri))); + if (identities.size < 2) continue; + const uris = [...identities].sort(); + for (const entry of group) { + const self = skillUriIdentity(entry.uri); + const others = uris.filter((uri) => uri !== self); + issues.set(self, { + code: "duplicate-name", + severity: "warning", + message: `Another skill in this listing also declares the name "${name}" (${others.join(", ")}). This is legal — a consumer must tell them apart by their URIs rather than collapsing or preferring one.`, + }); + } + } + return issues; +} diff --git a/core/mcp/skillsVerification.ts b/core/mcp/skillsVerification.ts index 650ae132..a7ee2aac 100644 --- a/core/mcp/skillsVerification.ts +++ b/core/mcp/skillsVerification.ts @@ -30,6 +30,7 @@ import type { RequestMetadata } from "./types.js"; import { checkSkillConformance, checkSkillFrontmatterMatch, + checkSkillNameCollisions, skillDisplayName, skillFileBytes, skillUriIdentity, @@ -141,6 +142,12 @@ export async function verifySkills( entries: readonly SkillEntry[], metadata?: RequestMetadata, ): Promise { + // Computed once over the whole set, because a name collision is a property + // of the listing rather than of an entry — `checkSkillConformance` sees one + // at a time and structurally cannot report it. Note this is scoped to the + // entries passed in, so `--method skills/get --verify` on a single skill + // reports no collision: there is no listing to collide within. + const collisions = checkSkillNameCollisions(entries); const reports: SkillVerifyReport[] = []; for (const entry of entries) { // The entry's own SKILL.md, read once and used twice — for its digest and @@ -211,7 +218,11 @@ export async function verifySkills( } } - const conformance = checkSkillConformance(entry); + const collision = collisions.get(skillUriIdentity(entry.uri)); + const conformance = [ + ...checkSkillConformance(entry), + ...(collision ? [collision] : []), + ]; const frontmatter = entryText === undefined ? [] diff --git a/docs/test-servers.md b/docs/test-servers.md index f33f3633..3fdca7d6 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -64,7 +64,7 @@ as a missing capability rather than an error. ## Skills (SEP-2640) -`skills-http.json` sets `"skills": true` and serves six skills over three +`skills-http.json` sets `"skills": true` and serves eight skills over four `skills/list` pages. Since [#2248](https://github.com/modelcontextprotocol/inspector/issues/2248) that one flag also declares **`directoryRead: true`** and registers the @@ -88,14 +88,15 @@ It works on **either era**: `skills/list`, `skills/get` and era codec defines, so the SDK's era gate skips them entirely — which is why this fixture, unlike the tasks ones, needs no per-era variant. -Five of the six skills are deliberately awkward, because the checks the Skills -tab runs are untestable without them. Only **three** are outright violations -(`tampered-notes`, `lying-listing`, `wrong-folder` — the three `--verify` fails -on). The other two are subtler and neither is an error on its own: the +Seven of the eight skills are deliberately awkward, because the checks the +Skills tab runs are untestable without them. Only **three** are outright +violations (`tampered-notes`, `lying-listing`, `wrong-folder` — the three +`--verify` fails on). The rest are subtler and none is an error on its own: the `"dynamic"` form is **conforming**, and is here because "legal but unverifiable" -is the case most easily buried; and `stale-manifest`'s entry is fully conforming +is the case most easily buried; `stale-manifest`'s entry is fully conforming too, with the defect living in the disagreement between its manifest and its -directory listing: +directory listing; and the two `reports` skills are both entirely valid, with +the obligation falling on whoever consumes them: | Skill | What it exercises | | --- | --- | @@ -104,6 +105,7 @@ directory listing: | `dynamic-report` | `resources: "dynamic"` — a **legal** form for generated content. No manifest is advertised, so integrity cannot be verified at all; reported as a warning, not an error. | | `wrong-folder` | A URI path segment (`wrong-folder`) that disagrees with `frontmatter.name` (`right-name`), the one structural invariant SEP-2640 states outright. | | `stale-manifest` | A skill that **serves and directory-lists a file its `resources` manifest does not declare**. Its entry is otherwise fully conforming and verifies clean, so the disagreement between the two views is the only defect — and only a directory read can see it. SEP-2640 calls a directory result "a live observation" and says hosts MUST NOT treat it as extending the manifest, so the Directory section marks the extra child **not listed** rather than showing it as one of the skill's files. | +| `acme/reports` + `globex/reports` | **Two conforming skills sharing the name `reports`.** SEP-2640 requires only that the segment before `/SKILL.md` equal `frontmatter.name`, which multi-segment paths satisfy while still sharing a final segment — its own `acme/billing/refunds` example is this shape. Hosts MUST NOT assume name uniqueness and MUST tell the two apart rather than collapsing or preferring one, so the Inspector reports a `duplicate-name` **warning** on both and shows each skill's URI beside its name. Also the only fixture with a multi-segment skill path. | | `lying-listing` | A `skills/list` entry advertising one `description` while the served `SKILL.md` carries another. **Its digest verifies** — a digest is taken over the bytes the server served and says nothing about whether the listing described them honestly — so this is the one violation only the frontmatter cross-check can catch. | Connection Info's **Skills Extension Options** section shows the `directoryRead` diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts index 82679e53..95b1c468 100644 --- a/test-servers/src/skills.ts +++ b/test-servers/src/skills.ts @@ -25,6 +25,14 @@ * observation" and says hosts MUST NOT treat it as extending the manifest, * so this is the fixture for that rule: the Inspector must show the extra * child as *not listed* rather than as one of the skill's files (#2248). + * - `acme/reports` and `globex/reports` collide on `frontmatter.name`. Both + * are **fully conforming** — SEP-2640 requires only that the segment before + * `/SKILL.md` equal the name, which multi-segment paths satisfy while still + * sharing a final segment, and the SEP's own `acme/billing/refunds` example + * is this shape. The obligation is on the *consumer*: hosts MUST NOT assume + * name uniqueness and MUST tell two same-named skills apart rather than + * collapsing or preferring one. This pair is also the only fixture with a + * multi-segment skill path, which nothing else here exercises (#2248). * - `lying-listing` advertises one `description` in its `skills/list` entry * and serves a different one in its `SKILL.md` — the violation no digest can * catch, because the digest is over the bytes the server served and says @@ -283,6 +291,26 @@ const LYING_MD = skillMd( "# Lying listing\n\nThe description this file carries is not the one the listing advertised.", ); +// Same `name`, different paths — see the module header. Their `SKILL.md` files +// are derived from these objects like every other fixture's, so each entry is +// internally consistent and the ONLY thing to report is the collision. +const ACME_REPORTS_FM: Frontmatter = { + name: "reports", + description: "Build the weekly report from the acme ledger", +}; +const ACME_REPORTS_MD = skillMd( + ACME_REPORTS_FM, + "# Reports (acme)\n\nOne of two skills named `reports`; tell them apart by URI.", +); +const GLOBEX_REPORTS_FM: Frontmatter = { + name: "reports", + description: "Build the weekly report from the globex ledger", +}; +const GLOBEX_REPORTS_MD = skillMd( + GLOBEX_REPORTS_FM, + "# Reports (globex)\n\nThe other skill named `reports`; same name, different server path.", +); + const FIXTURE_SKILLS: FixtureSkill[] = [ { path: "data-analysis", @@ -358,6 +386,28 @@ const FIXTURE_SKILLS: FixtureSkill[] = [ }, ], }, + { + path: "acme/reports", + frontmatter: ACME_REPORTS_FM, + files: [ + { + uri: "skill://acme/reports/SKILL.md", + text: ACME_REPORTS_MD, + mimeType: "text/markdown", + }, + ], + }, + { + path: "globex/reports", + frontmatter: GLOBEX_REPORTS_FM, + files: [ + { + uri: "skill://globex/reports/SKILL.md", + text: GLOBEX_REPORTS_MD, + mimeType: "text/markdown", + }, + ], + }, { path: "wrong-folder", frontmatter: MISMATCHED_FM,