diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx index da17cf50..dfdc4581 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx @@ -3,6 +3,7 @@ import type { ComponentProps } from "react"; import type { Meta, StoryObj } from "@storybook/react-vite"; import { expect, fn, userEvent, waitFor, within } from "storybook/test"; import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas"; +import { sha256Bytes } from "@inspector/core/mcp/sha256"; import { SkillsScreen } from "./SkillsScreen"; import type { SkillsUiState } from "./SkillsScreen"; import { EMPTY_SKILLS_UI } from "../screenUiState"; @@ -16,29 +17,74 @@ function StatefulSkillsScreen(args: ComponentProps) { } const REF_TEXT = "# Column rules\n"; -const SELF_TEXT = "# skill\n"; -// The real digests of those two strings, so the clean skill actually verifies -// when the "Verify all" story runs — a placeholder would demo a false green. -const REF_DIGEST = - "sha256:e201429aa2684958ca1a0537ab4eb4b7eb3a81c71e7cc7a11397eb500738e015"; -const SELF_DIGEST = - "sha256:6504f2de0a1febf7492c3b98f93d9ab49558eb364607a706f02fe9a75aa7f75b"; + +/** + * A skill's own `SKILL.md`, built FROM the frontmatter its entry advertises. + * + * SEP-2640 requires the two to match field for field, and the screen now checks + * it — so a shared placeholder body with no frontmatter made every "conforming" + * story report a `frontmatter-absent` error. Deriving the file makes that class + * of drift impossible rather than merely fixed, which is the same discipline + * `test-servers/src/skills.ts` and the unit fixtures apply. + */ +const skillMd = (name: string, description: string) => + `---\nname: ${name}\ndescription: ${description}\n---\n\n# ${name}\n`; + +/** Real digests, computed from the very bytes served, so the clean skill + * actually verifies when the "Verify all" story runs — a hard-coded + * placeholder would demo a false green, and could not survive an edit to the + * text above. `sha256Bytes` is the repo's synchronous implementation, which is + * what lets this happen at module scope in a CSF file. */ +const digestOf = (text: string) => + `sha256:${[...sha256Bytes(new TextEncoder().encode(text))] + .map((b) => b.toString(16).padStart(2, "0")) + .join("")}`; + +const REF_DIGEST = digestOf(REF_TEXT); + +/** The frontmatter each sample skill advertises, keyed by its path segment. */ +const SAMPLE_FM: Record = { + "data-analysis": { + name: "data-analysis", + description: "Analyze a CSV and summarize its columns", + }, + "tampered-notes": { + name: "tampered-notes", + description: "Advertises a digest its bytes do not match", + }, + "wrong-folder": { + name: "right-name", + description: "Served from a folder that disagrees with its name", + }, + "big-manifest": { + name: "big-manifest", + description: "A skill with a long manifest", + }, +}; + +/** The file a given skill path serves, derived from its own frontmatter. */ +const selfText = (path: string) => { + const fm = SAMPLE_FM[path]; + return fm ? skillMd(fm.name, fm.description) : "# skill\n"; +}; /** Every manifest lists the skill's own SKILL.md — a manifest is the complete * file set, so one that omits it is a `manifest-missing-self` error. */ -const selfEntry = (path: string) => ({ - uri: `skill://${path}/SKILL.md`, - digest: SELF_DIGEST, - size: 8, -}); +const selfEntry = (path: string) => { + const text = selfText(path); + return { + uri: `skill://${path}/SKILL.md`, + digest: digestOf(text), + size: new TextEncoder().encode(text).byteLength, + }; +}; const sampleSkills: SkillEntry[] = [ { uri: "skill://data-analysis/SKILL.md", - frontmatter: { - name: "data-analysis", - description: "Analyze a CSV and summarize its columns", - }, + // Read from SAMPLE_FM, which is also what the served file is built from — + // so the entry and its SKILL.md agree by construction. + frontmatter: SAMPLE_FM["data-analysis"], resources: [ selfEntry("data-analysis"), { @@ -50,10 +96,7 @@ const sampleSkills: SkillEntry[] = [ }, { uri: "skill://tampered-notes/SKILL.md", - frontmatter: { - name: "tampered-notes", - description: "Advertises a digest its bytes do not match", - }, + frontmatter: SAMPLE_FM["tampered-notes"], resources: [ selfEntry("tampered-notes"), { @@ -76,10 +119,7 @@ const sampleSkills: SkillEntry[] = [ }, { uri: "skill://wrong-folder/SKILL.md", - frontmatter: { - name: "right-name", - description: "URI path segment disagrees with frontmatter.name", - }, + frontmatter: SAMPLE_FM["wrong-folder"], resources: [selfEntry("wrong-folder")], }, ]; @@ -94,11 +134,19 @@ const meta: Meta = { ui: EMPTY_SKILLS_UI, onUiChange: fn(), onRefreshList: fn(), - onReadSkillFile: fn(async (uri: string) => - uri.endsWith("reference.md") - ? { text: REF_TEXT } - : { text: SELF_TEXT, mimeType: "text/markdown" }, - ), + onReadSkillFile: fn(async (uri: string) => { + if (uri.endsWith("reference.md")) return { text: REF_TEXT }; + // Only a skill's OWN SKILL.md is derived from its frontmatter. Every + // other manifest file keeps the 8-byte placeholder its entry declares — + // serving the SKILL.md for `notes.md` made the tampered fixture fail its + // SIZE check first, which is a different finding from the digest + // mismatch that story exists to show. + if (!uri.endsWith("/SKILL.md")) { + return { text: "# skill\n", mimeType: "text/markdown" }; + } + const path = uri.slice("skill://".length, -"/SKILL.md".length); + return { text: selfText(path), mimeType: "text/markdown" }; + }), // Echoes back the entry `skills/list` advertised, so "Fetch with // skills/get" demonstrates the matching case rather than throwing. onGetSkill: fn(async (uri: string) => { @@ -190,10 +238,7 @@ const LONG_SKILL_MD = [ // overflowing content. const manyFilesSkill: SkillEntry = { uri: "skill://big-manifest/SKILL.md", - frontmatter: { - name: "big-manifest", - description: "A conforming skill that declares a great many files", - }, + frontmatter: SAMPLE_FM["big-manifest"], resources: [ selfEntry("big-manifest"), ...Array.from({ length: 120 }, (_, i) => ({ @@ -217,8 +262,10 @@ const manyFilesSkill: SkillEntry = { export const LongManifest: Story = { args: { skills: [manyFilesSkill], + // Derived like every other fixture, so the frontmatter check stays silent + // and this story measures only the long-manifest layout it is about. onReadSkillFile: fn(async () => ({ - text: "---\nname: big-manifest\n---\n\n# Big manifest\n", + text: selfText("big-manifest"), mimeType: "text/markdown", })), }, @@ -276,10 +323,12 @@ const hostileHeaderSkill: SkillEntry = { resources: [ { uri: `skill://${"very-long-path-segment/".repeat(30)}SKILL.md`, - digest: SELF_DIGEST, + // This fixture exercises LAYOUT under hostile strings; its digests are + // never verified by the story, so a placeholder is honest here. + digest: `sha256:${"0".repeat(64)}`, size: 8, }, - { uri: HOSTILE_FILE_URI, digest: SELF_DIGEST, size: 8 }, + { uri: HOSTILE_FILE_URI, digest: `sha256:${"0".repeat(64)}`, size: 8 }, ], }; diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx index cd659f70..aaa2064b 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -155,6 +155,9 @@ const ALL_SKILLS = [ /** Everything `readFixtureFile` can serve a `SKILL.md` for. */ const SERVED_SKILLS = [...ALL_SKILLS, ACME, GLOBEX]; +/** The many-row fixture's frontmatter, shared with the stub that serves it. */ +const MANY_FM: Frontmatter = { name: "many", description: "Many rows" }; + /** * A `resources/read` that serves the fixture bytes for any known URI. A skill's * own `SKILL.md` comes from {@link skillMdFor}, so it agrees with the entry's @@ -386,17 +389,22 @@ describe("SkillsScreen", () => { const user = userEvent.setup(); // Held open so the batch is still in flight when the selection changes. const releases: (() => void)[] = []; + // Each URI gets the file its OWN entry implies, so the frontmatter check + // stays silent and this test measures only the open-state invariant it is + // about. A stub serving one skill's text for every URI produces a genuine + // mismatch, which now reveals Conformance by design. const onReadSkillFile = vi.fn( - () => + (uri: string) => new Promise<{ text: string }>((resolve) => { - releases.push(() => resolve({ text: SELF_TEXT })); + const fm = uri.startsWith("skill://many/") ? MANY_FM : CLEAN_FM; + releases.push(() => resolve({ text: skillMdFor(fm) })); }), ); // More rows than the concurrency cap, so workers keep pulling. const manyRows: SkillEntry = { ...CLEAN_SKILL, uri: "skill://many/SKILL.md", - frontmatter: { name: "many", description: "Many rows" }, + frontmatter: MANY_FM, resources: Array.from({ length: 10 }, (_, i) => ({ uri: i === 0 ? "skill://many/SKILL.md" : `skill://many/f${i}.md`, digest: SELF_DIGEST, @@ -2315,7 +2323,7 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => { }; renderWithMantine(); await user.click(screen.getByText("data-analysis")); - await user.click(screen.getByRole("button", { name: /Conformance/ })); + // No click to expand: a frontmatter finding reveals the section itself. await waitFor(() => expect( screen.getByTestId("skill-frontmatter-issues"), @@ -2352,7 +2360,7 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => { />, ); await user.click(screen.getByText("data-analysis")); - await user.click(screen.getByRole("button", { name: /Conformance/ })); + // No click to expand: a frontmatter finding reveals the section itself. await waitFor(() => expect( screen.getByTestId("skill-frontmatter-issues"), @@ -2377,7 +2385,7 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => { />, ); await user.click(screen.getByText("data-analysis")); - await user.click(screen.getByRole("button", { name: /Conformance/ })); + // No click to expand: a frontmatter finding reveals the section itself. await waitFor(() => expect( screen.getByTestId("skill-frontmatter-issues"), @@ -2385,6 +2393,41 @@ describe("SkillsScreen frontmatter cross-check (#2248)", () => { ); }); + it("reveals Conformance when a frontmatter finding arrives", async () => { + // A structurally clean entry opens collapsed, and the frontmatter findings + // arrive later from the SKILL.md read — so the alerts explaining a + // mandatory verification failure sat behind a click the reader had no + // reason to make (Copilot). + const user = userEvent.setup(); + const lying: SkillEntry = { + ...CLEAN_SKILL, + frontmatter: { name: "data-analysis", description: "Disagrees" }, + }; + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await waitFor(() => + expect( + screen.getByRole("button", { name: /Conformance/ }), + ).toHaveAttribute("aria-expanded", "true"), + ); + expect(screen.getByTestId("skill-frontmatter-issues")).toBeInTheDocument(); + }); + + it("leaves a clean entry's Conformance collapsed", async () => { + // The reveal must not fire when there is nothing to reveal, or it undoes + // the auto-collapse it sits next to. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await waitFor(() => + expect(readFixtureFile).toHaveBeenCalledWith(CLEAN_SKILL.uri), + ); + expect(screen.getByRole("button", { name: /Conformance/ })).toHaveAttribute( + "aria-expanded", + "false", + ); + }); + 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 diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx index 8b999d6b..135a922d 100644 --- a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -1482,6 +1482,27 @@ export function SkillsScreen({ * entry whose only finding is a banner one ended up showing an empty findings * container instead of "no structural issues". */ + /** + * Reveal Conformance when the frontmatter check finds something. + * + * A structurally clean entry opens with the section COLLAPSED — the header + * badge carries the whole answer — but the frontmatter findings arrive later, + * from the `SKILL.md` read, and land inside that collapsed section. The badge + * now counts them, so the number changes; the alerts explaining a mandatory + * verification failure were still a click away (Copilot). + * + * Keyed on the entry so it fires **once** per skill, when findings first + * appear, rather than fighting a user who deliberately collapses it again. + * `useValueChange` runs during render and does only `setState`, as that hook + * requires; the key is a primitive so `Object.is` cannot loop. + */ + useValueChange(frontmatterIssues.length > 0 ? manifestKey : "", (next) => { + if (next === "") return; + setOpenSections((prev) => + prev.includes("conformance") ? prev : [...prev, "conformance"], + ); + }); + const listedIssues = useMemo( () => issues.filter( diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts index 67132a11..ee3778ec 100644 --- a/clients/web/src/test/core/mcp/skills.test.ts +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -1035,6 +1035,36 @@ describe("checkSkillFrontmatterMatch (#2248)", () => { ).toEqual([]); }); + it("bounds the LISTING side too, not only the served YAML", () => { + // The listing arrives over JSON-RPC so it cannot be cyclic, but it is just + // as unbounded in depth — and both the comparison and its message + // formatter walk it, so an absurdly nested advertised value crashed the + // tool exactly as a cyclic served one did (Copilot). + let deep: unknown = "leaf"; + for (let i = 0; i < 5000; i += 1) deep = { a: deep }; + const issues = checkSkillFrontmatterMatch( + entry({ x: deep as Record }), + file("x: shallow"), + ); + expect(issues).toEqual([ + expect.objectContaining({ + code: "frontmatter-unparsable", + severity: "error", + }), + ]); + expect(issues[0].message).toMatch(/listing's own frontmatter/); + }); + + it("still compares an ordinarily nested listing value", () => { + // The bound must not reject anything a real skill would carry. + expect( + checkSkillFrontmatterMatch( + entry({ meta: { a: { b: { c: [1, 2] } } } }), + file("meta:\n a:\n b:\n c: [1, 2]"), + ), + ).toEqual([]); + }); + it("reports nothing for two empty frontmatters", () => { expect(checkSkillFrontmatterMatch(entry({}), file(""))).toEqual([]); }); diff --git a/core/mcp/skillFile.ts b/core/mcp/skillFile.ts index 76a098d8..42f79d41 100644 --- a/core/mcp/skillFile.ts +++ b/core/mcp/skillFile.ts @@ -117,7 +117,13 @@ function hasContent(yamlText: string): boolean { const MAX_FRONTMATTER_DEPTH = 64; /** - * Why a parsed frontmatter cannot be compared, or `undefined` when it can. + * Why a frontmatter value cannot be compared, or `undefined` when it can. + * + * Exported because **both sides need it**. The served side can be cyclic; the + * listed side arrives over JSON-RPC and cannot be, but it is just as unbounded + * in DEPTH — a server can advertise a listing nested tens of thousands of + * levels deep, and the comparison and its message formatter both recurse. A + * guard on only the YAML side left that door open (Copilot). * * ⚠️ **A YAML document is a graph, not a tree, and JSON is a tree.** An alias * can refer to its own ancestor — `meta: &m [*m]` parses without error into a @@ -136,10 +142,10 @@ const MAX_FRONTMATTER_DEPTH = 64; * — which YAML aliases make ordinary and which JSON represents perfectly well — * is not mistaken for a cycle. */ -function jsonGraphError( +export function jsonGraphError( value: unknown, - seen: Set, - depth: number, + seen: Set = new Set(), + depth = 0, ): string | undefined { if (typeof value !== "object" || value === null) return undefined; if (depth > MAX_FRONTMATTER_DEPTH) { diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts index d33847b9..54032198 100644 --- a/core/mcp/skills.ts +++ b/core/mcp/skills.ts @@ -42,7 +42,11 @@ import { type SkillResource, } from "./skillsSchemas.js"; import { sha256Bytes } from "./sha256.js"; -import { parseSkillFrontmatter, splitSkillFile } from "./skillFile.js"; +import { + jsonGraphError, + parseSkillFrontmatter, + splitSkillFile, +} from "./skillFile.js"; /** Maximum resource entries a single skill may declare (SEP-2640). */ export const SKILL_MAX_RESOURCE_ENTRIES = 512; @@ -852,6 +856,23 @@ export function checkSkillFrontmatterMatch( }, ]; } + // The LISTING side is bounded too, before anything recurses over it. It + // arrives over JSON-RPC so it cannot be cyclic, but it is just as unbounded + // in depth — and both `jsonLikeEqual` and `displayValue` walk it, so a + // server advertising an absurdly nested value crashed the tool exactly as a + // cyclic served one did (Copilot). Checked before the file is parsed: there + // is no point reading one if the thing to compare it against is unusable. + const listedError = jsonGraphError(entry.frontmatter); + if (listedError) { + return [ + { + code: "frontmatter-unparsable", + severity: "error", + message: `The listing's own frontmatter cannot be compared: ${listedError}`, + resourceUri: entry.uri, + }, + ]; + } const parsed = parseSkillFrontmatter(frontmatter); if ("error" in parsed) { return [