feat: report a skill-name collision across the listing (#2248)
SEP-2640: hosts MUST NOT assume name uniqueness, and when two entries in one listing collide on `name` a host MUST disambiguate them rather than silently discarding or preferring one. `checkSkillConformance` structurally cannot see this — it takes one entry and a collision is a property of the pair — so `checkSkillNameCollisions` walks the listing and returns a finding per colliding entry, each naming the others. All three clients merge it into the entry's own findings, so it carries through the header badge, the CLI report and the TUI row marks with no new surface. **It is a `warning`, not an `error`,** and that is the severity split doing its job. The obligation is on the *consumer*: a server may legitimately publish two skills with the same name under different paths, and the SEP's own `acme/billing/refunds` example is exactly that shape. Calling it an error would tell a conforming author their catalog is invalid. `--verify` therefore still exits 0 for a collision. In the web screen it renders as a banner directly under the Conformance header and is filtered out of the findings list, the same treatment `dynamic-resources` gets: it changes how everything below it should be read, and stating one fact twice reads as two findings. Fixed while adding it: an entry whose only finding was a banner one rendered an EMPTY findings container instead of "no structural issues", because the list's presence was decided on the unfiltered set while its contents were filtered. `listedIssues` is now derived once and used for both. The fixture gains `acme/reports` + `globex/reports`, both fully conforming and sharing the name `reports` — also the only fixture with a multi-segment skill path, which nothing else exercised. Eight skills over four pages now, and the integration test walks the cursor to exhaustion rather than asserting a fixed page count, so a future fixture does not require editing it. 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
parent
775107776d
commit
4fa00d1cfd
10 changed files with 557 additions and 51 deletions
|
|
@ -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(
|
||||
<SkillsTab
|
||||
skills={[acme, globex]}
|
||||
pageCount={1}
|
||||
inspectorClient={null}
|
||||
width={160}
|
||||
height={30}
|
||||
focusedPane="list"
|
||||
/>,
|
||||
);
|
||||
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(
|
||||
<SkillsTab
|
||||
|
|
|
|||
|
|
@ -5,7 +5,9 @@ import type { InspectorClient } from "@inspector/core/mcp/index.js";
|
|||
import { AuthRecoveryRequiredError } from "@inspector/core/auth/challenge.js";
|
||||
import {
|
||||
checkSkillConformance,
|
||||
checkSkillNameCollisions,
|
||||
skillDisplayName,
|
||||
skillUriIdentity,
|
||||
type SkillIssue,
|
||||
} from "@inspector/core/mcp/skills.js";
|
||||
import {
|
||||
|
|
@ -215,7 +217,15 @@ export function SkillsTab({
|
|||
|
||||
const listWidth = Math.floor(width * 0.4);
|
||||
const detailWidth = width - listWidth;
|
||||
const issues = selectedSkill ? checkSkillConformance(selectedSkill) : [];
|
||||
// A name collision is a property of the LISTING, not of an entry, so it is
|
||||
// computed once here and merged into each entry's own findings — which is
|
||||
// what carries it into the row marks below as well as the detail pane.
|
||||
const collisions = checkSkillNameCollisions(skills);
|
||||
const findingsFor = (skill: SkillEntry): SkillIssue[] => {
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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(<ControlledSkillsScreen skills={[ACME, GLOBEX]} />);
|
||||
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(<ControlledSkillsScreen skills={[ACME, GLOBEX]} />);
|
||||
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(<ControlledSkillsScreen skills={[ACME, GLOBEX]} />);
|
||||
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(
|
||||
<SkillsScreen
|
||||
{...baseProps}
|
||||
skills={[ACME, GLOBEX]}
|
||||
ui={{ ...EMPTY_SKILLS_UI, selectedSkillUri: ACME.uri }}
|
||||
/>,
|
||||
);
|
||||
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(
|
||||
<ControlledSkillsScreen
|
||||
skills={[ACME, GLOBEX]}
|
||||
ui={{ ...EMPTY_SKILLS_UI, search: "acme" }}
|
||||
/>,
|
||||
);
|
||||
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(<ControlledSkillsScreen />);
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
</Alert>
|
||||
)}
|
||||
{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 && (
|
||||
<Alert
|
||||
color="yellow"
|
||||
title="Another skill shares this name"
|
||||
data-testid="skill-name-collision"
|
||||
>
|
||||
{collision.message}
|
||||
</Alert>
|
||||
)}
|
||||
{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({
|
|||
</Alert>
|
||||
) : (
|
||||
<IssueStack data-testid="skill-issues">
|
||||
{issues
|
||||
// The banner above already states this one, in prose.
|
||||
.filter((issue) => issue.code !== "dynamic-resources")
|
||||
.map((issue, index) => (
|
||||
<Alert
|
||||
// The index is load-bearing, not decoration: a
|
||||
// manifest repeating one URI three times yields
|
||||
// three `duplicate-resource` findings with
|
||||
// identical code and URI, and a key built from
|
||||
// those alone would make React drop the extras —
|
||||
// hiding findings in exactly the malformed input
|
||||
// this view exists to inspect.
|
||||
key={`${index}:${issue.code}:${issue.resourceUri ?? ""}`}
|
||||
color={issueColor(issue)}
|
||||
title={issue.code}
|
||||
>
|
||||
<Stack gap={2}>
|
||||
<Text size="sm">{issue.message}</Text>
|
||||
{issue.resourceUri && (
|
||||
<MonoCaption>{issue.resourceUri}</MonoCaption>
|
||||
)}
|
||||
</Stack>
|
||||
</Alert>
|
||||
))}
|
||||
{listedIssues.map((issue, index) => (
|
||||
<Alert
|
||||
// The index is load-bearing, not decoration: a
|
||||
// manifest repeating one URI three times yields
|
||||
// three `duplicate-resource` findings with
|
||||
// identical code and URI, and a key built from
|
||||
// those alone would make React drop the extras —
|
||||
// hiding findings in exactly the malformed input
|
||||
// this view exists to inspect.
|
||||
key={`${index}:${issue.code}:${issue.resourceUri ?? ""}`}
|
||||
color={issueColor(issue)}
|
||||
title={issue.code}
|
||||
>
|
||||
<Stack gap={2}>
|
||||
<Text size="sm">{issue.message}</Text>
|
||||
{issue.resourceUri && (
|
||||
<MonoCaption>{issue.resourceUri}</MonoCaption>
|
||||
)}
|
||||
</Stack>
|
||||
</Alert>
|
||||
))}
|
||||
</IssueStack>
|
||||
)}
|
||||
{/* The frontmatter cross-check renders in Conformance
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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<string, SkillIssue> {
|
||||
const byName = new Map<string, SkillEntry[]>();
|
||||
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<string, SkillIssue>();
|
||||
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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<SkillVerifyReport[]> {
|
||||
// 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
|
||||
? []
|
||||
|
|
|
|||
|
|
@ -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`
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in a new issue