diff --git a/clients/web/README.md b/clients/web/README.md index a72ed8df..8a427970 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -93,7 +93,7 @@ Nothing _enforces_ the boundary — no path alias keys off it, and the coverage ## Core tab automation contract -The Tools, Resources, and Prompts screens each expose a `data-testid` plus a +The Tools, Resources, Prompts and Skills screens each expose a `data-testid` plus a small set of `data-*` attributes, so a headless driver can `waitForSelector` on a deterministic signal rather than on visible copy. `scripts/smoke-web-tabs.mjs` drives all three against `test-servers/configs/web-tabs-http.json` ([#2148](https://github.com/modelcontextprotocol/inspector/issues/2148)). @@ -114,6 +114,10 @@ Treat them as a public contract, for the same reason as the Apps ones below: | `data-prompt-count` | on `prompts-screen`| Entries from `prompts/list`. | | `data-get-status` | on `prompts-screen`| `idle` → `pending` → `ok` / `error` for the current `prompts/get`. | | `data-testid="prompt-messages"` | messages panel | The fetched prompt's **rendered** messages — the `prompts/get` counterpart of `resource-preview`, and asserted alongside `data-get-status` for the same reason. | +| `data-testid="skills-screen"` | Skills root | The element carrying the two attributes below ([#2234](https://github.com/modelcontextprotocol/inspector/issues/2234)). Not driven by `smoke-web-tabs.mjs` yet — the attributes exist so it can be, and so a rename fails in the screen's unit test rather than later. | +| `data-skill-count` | on `skills-screen` | Entries accumulated from `skills/list`. | +| `data-skill-page-count` | on `skills-screen` | Pages the last `skills/list` walk took — a **separate** fact from the count, and the one that shows pagination actually happened. | +| `data-testid="skill-manifest"` | detail pane | The selected skill's resource manifest. Absent for a `resources: "dynamic"` skill, which has no manifest to render. | Why attributes rather than text: a smoke that waited on a label fails the next time the label is reworded, which is noise rather than signal — and it fails as diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 03bcda5d..930b52a1 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -75,6 +75,7 @@ import type { ResourcesPanelProps, ServerListProps, ShellProps, + SkillsPanelProps, TasksPanelProps, ToolsPanelProps, } from "./components/views/InspectorView/types"; @@ -458,6 +459,10 @@ function App() { tasks, refreshTasks, clearCompletedTasks, + skills, + skillsPageCount, + skillsLoadError, + refreshSkills, subscriptions, subscriptionStreamState, messages, @@ -828,6 +833,8 @@ function App() { onRefreshTools, onRefreshPrompts, onRefreshResources, + onRefreshSkills, + onReadSkillFile, onRefreshTasks, onTogglePaginatedLists, onLoadMoreTools, @@ -851,6 +858,7 @@ function App() { activeToolCallTaskIdRef, clearCompletedTasks, refreshTasks, + refreshSkills, paginatedLists, paginatedListsOverride, toolsPagination, @@ -1816,6 +1824,16 @@ function App() { onRefreshApps: onRefreshTools, }; + const skillsPanelProps: SkillsPanelProps = { + skills, + skillsPageCount, + skillsLoadError, + skillsUi: ui.skillsUi, + onSkillsUiChange: setUi.setSkillsUi, + onRefreshSkills, + onReadSkillFile, + }; + const tasksPanelProps: TasksPanelProps = { tasks, progressByTaskId, @@ -1889,6 +1907,7 @@ function App() { prompts={promptsPanelProps} resources={resourcesPanelProps} apps={appsPanelProps} + skills={skillsPanelProps} tasks={tasksPanelProps} logs={logsPanelProps} protocol={protocolPanelProps} diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index 0ebb740f..d495f8f2 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -452,6 +452,64 @@ describe("ConnectionInfoContent", () => { expect(screen.getAllByText("—")).toHaveLength(2); }); + it("hides the Skills section when the server declares no skills extension (#2234)", () => { + renderWithMantine( + , + ); + expect(screen.queryByText("Skills Extension")).not.toBeInTheDocument(); + }); + + it("shows the Skills extension and its directoryRead sub-flag (#2234)", () => { + // The generic "Server Extensions" row lists the identifier; the sub-flag + // that gates `resources/directory/read` is what this section adds, and it + // is the fact a server author opens the modal to confirm. + renderWithMantine( + , + ); + expect(screen.getByText("Skills Extension")).toBeInTheDocument(); + expect(screen.getByTestId("skills-directory-read")).toHaveTextContent( + "Supported", + ); + }); + + it("reports directory read as unsupported for a bare skills declaration (#2234)", () => { + renderWithMantine( + , + ); + expect(screen.getByTestId("skills-directory-read")).toHaveTextContent( + "Not supported", + ); + }); + it("renders client registration kind when provided", () => { renderWithMantine( + {/* Skills (SEP-2640). The generic "Server Extensions" row above lists the + identifier, but not the one sub-option the extension defines — + `directoryRead`, which gates `resources/directory/read`. That flag is + exactly what a server author opens this modal to confirm, so it gets a + row of its own rather than being flattened into a key list (#2234). */} + {skillsExtension && ( + + + Skills Extension + {SKILLS_EXTENSION_KEY} + + + Directory Read + + {skillsExtension.directoryRead ? "Supported" : "Not supported"} + + + + )} + {instructions && ( Server Instructions diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx new file mode 100644 index 00000000..101d7383 --- /dev/null +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.stories.tsx @@ -0,0 +1,136 @@ +import { useState } from "react"; +import type { ComponentProps } from "react"; +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { expect, fn, userEvent, within } from "storybook/test"; +import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas"; +import { SkillsScreen } from "./SkillsScreen"; +import type { SkillsUiState } from "./SkillsScreen"; +import { EMPTY_SKILLS_UI } from "../screenUiState"; + +// SkillsScreen is controlled (selection and search live in the parent as one +// `ui` object — see #1417). This wrapper holds that state so the play-driven +// clicks drive the detail pane, mirroring how App owns it in the real app. +function StatefulSkillsScreen(args: ComponentProps) { + const [ui, setUi] = useState(args.ui ?? EMPTY_SKILLS_UI); + return ; +} + +const REF_TEXT = "# Column rules\n"; +// The digest of REF_TEXT, so the clean skill really does verify when the +// "Verify all" story runs — a placeholder here would demo a false green. +const REF_DIGEST = + "sha256:e201429aa2684958ca1a0537ab4eb4b7eb3a81c71e7cc7a11397eb500738e015"; + +const sampleSkills: SkillEntry[] = [ + { + uri: "skill://data-analysis/SKILL.md", + frontmatter: { + name: "data-analysis", + description: "Analyze a CSV and summarize its columns", + }, + resources: [ + { + uri: "skill://data-analysis/reference.md", + digest: REF_DIGEST, + size: REF_TEXT.length, + }, + ], + }, + { + uri: "skill://tampered-notes/SKILL.md", + frontmatter: { + name: "tampered-notes", + description: "Advertises a digest its bytes do not match", + }, + resources: [ + { + uri: "skill://tampered-notes/notes.md", + digest: `sha256:${"b".repeat(64)}`, + size: 12, + }, + ], + }, + { + uri: "skill://dynamic-report/SKILL.md", + frontmatter: { + name: "dynamic-report", + description: "Generated files, so integrity cannot be verified", + }, + resources: "dynamic", + }, + { + uri: "skill://wrong-folder/SKILL.md", + frontmatter: { + name: "right-name", + description: "URI path segment disagrees with frontmatter.name", + }, + resources: [], + }, +]; + +const meta: Meta = { + title: "Screens/SkillsScreen", + component: SkillsScreen, + parameters: { layout: "fullscreen" }, + args: { + skills: sampleSkills, + pageCount: 2, + ui: EMPTY_SKILLS_UI, + onUiChange: fn(), + onRefreshList: fn(), + onReadSkillFile: fn(async (uri: string) => + uri.endsWith("reference.md") + ? { text: REF_TEXT } + : { text: `# ${uri}\n`, mimeType: "text/markdown" }, + ), + }, + render: (args) => , +}; + +export default meta; +type Story = StoryObj; + +export const Default: Story = {}; + +export const Empty: Story = { + args: { skills: [], pageCount: 0 }, +}; + +export const LoadFailed: Story = { + args: { loadError: new Error("skills/list failed: -32601 Method not found") }, +}; + +export const ConformingSkill: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByText("data-analysis")); + await expect(canvas.getByText("Conforms")).toBeInTheDocument(); + }, +}; + +export const NameMismatch: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByText("right-name")); + await expect(canvas.getByText("name-path-mismatch")).toBeInTheDocument(); + }, +}; + +export const DynamicResources: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByText("dynamic-report")); + await expect(canvas.getByText("Dynamic resources")).toBeInTheDocument(); + }, +}; + +export const DigestMismatch: Story = { + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + await userEvent.click(canvas.getByText("tampered-notes")); + await userEvent.click(canvas.getByRole("button", { name: /Verify all/ })); + await expect( + await canvas.findByText("Digest mismatch"), + ).toBeInTheDocument(); + }, +}; diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx new file mode 100644 index 00000000..90de63c1 --- /dev/null +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.test.tsx @@ -0,0 +1,343 @@ +import { useState } from "react"; +import { describe, it, expect, vi } from "vitest"; +import userEvent from "@testing-library/user-event"; +import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas"; +import { sha256Digest, textToBytes } from "@inspector/core/mcp/skills"; +import { + renderWithMantine, + screen, + within, +} from "../../../test/renderWithMantine"; +import { + SkillsScreen, + type SkillsScreenProps, + type SkillsUiState, +} from "./SkillsScreen"; +import { EMPTY_SKILLS_UI } from "../screenUiState"; + +const REF_TEXT = "# Column rules\n"; +// Computed once at module load so the fixture's advertised digest really is the +// digest of the bytes the fake read returns — a hard-coded constant here would +// make the "verified" test pass for the wrong reason if the encoder changed. +const REF_DIGEST = await sha256Digest(textToBytes(REF_TEXT)); + +const CLEAN_SKILL: SkillEntry = { + uri: "skill://data-analysis/SKILL.md", + frontmatter: { + name: "data-analysis", + description: "Analyze a CSV and summarize its columns", + }, + resources: [ + { + uri: "skill://data-analysis/reference.md", + digest: REF_DIGEST, + size: REF_TEXT.length, + }, + ], +}; + +const TAMPERED_SKILL: SkillEntry = { + uri: "skill://tampered/SKILL.md", + frontmatter: { name: "tampered", description: "Bad digest" }, + resources: [ + { + uri: "skill://tampered/notes.md", + digest: `sha256:${"b".repeat(64)}`, + size: 4, + }, + ], +}; + +const DYNAMIC_SKILL: SkillEntry = { + uri: "skill://dynamic-report/SKILL.md", + frontmatter: { name: "dynamic-report", description: "Generated files" }, + resources: "dynamic", +}; + +const MISMATCHED_SKILL: SkillEntry = { + uri: "skill://wrong-folder/SKILL.md", + frontmatter: { name: "right-name", description: "Name disagreement" }, + resources: [], +}; + +const ALL_SKILLS = [ + CLEAN_SKILL, + TAMPERED_SKILL, + DYNAMIC_SKILL, + MISMATCHED_SKILL, +]; + +/** A `resources/read` that serves the fixture bytes for any known URI. */ +const readFixtureFile = vi.fn(async (uri: string) => { + if (uri === "skill://data-analysis/reference.md") return { text: REF_TEXT }; + if (uri === "skill://tampered/notes.md") return { text: "different\n" }; + return { text: `# ${uri}\n`, mimeType: "text/markdown" }; +}); + +const baseProps: SkillsScreenProps = { + skills: ALL_SKILLS, + pageCount: 2, + ui: EMPTY_SKILLS_UI, + onUiChange: vi.fn(), + onRefreshList: vi.fn(), + onReadSkillFile: readFixtureFile, +}; + +// SkillsScreen is controlled: the selection and the sidebar search live in the +// parent (App) as one `ui` object so they persist across tab navigation +// (#1417). This host holds that state so clicking a skill actually selects it. +function ControlledSkillsScreen(props: Partial = {}) { + const [ui, setUi] = useState({ + ...EMPTY_SKILLS_UI, + ...props.ui, + }); + return ( + { + setUi(next); + props.onUiChange?.(next); + }} + /> + ); +} + +describe("SkillsScreen", () => { + it("renders the empty state until a skill is selected", () => { + renderWithMantine(); + expect( + screen.getByText("Select a skill to view details"), + ).toBeInTheDocument(); + }); + + it("exposes the readiness contract the headless tab smoke keys off", () => { + renderWithMantine(); + const root = screen.getByTestId("skills-screen"); + expect(root).toHaveAttribute("data-skill-count", "4"); + expect(root).toHaveAttribute("data-skill-page-count", "2"); + }); + + it("renders 'No skills' when the list is empty", () => { + renderWithMantine(); + expect(screen.getByText("No skills")).toBeInTheDocument(); + }); + + it("renders a load failure above the list", () => { + renderWithMantine( + , + ); + expect(screen.getByText("Could not load skills")).toBeInTheDocument(); + expect(screen.getByText("nope")).toBeInTheDocument(); + }); + + it("filters the sidebar by name and by URI", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.type(screen.getByLabelText("Search skills"), "wrong-folder"); + // The matching skill's *name* is `right-name`, so a hit here proves the URI + // is searched too and not just the display name. + expect(screen.getByText("right-name")).toBeInTheDocument(); + expect(screen.queryByText("data-analysis")).not.toBeInTheDocument(); + }); + + it("calls onRefreshList when Refresh is clicked", async () => { + const user = userEvent.setup(); + const onRefreshList = vi.fn(); + renderWithMantine( + , + ); + await user.click(screen.getByRole("button", { name: "Refresh" })); + expect(onRefreshList).toHaveBeenCalled(); + }); + + it("reports a conforming skill as conforming", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + expect(screen.getByText("Conforms")).toBeInTheDocument(); + expect(screen.queryByTestId("skill-issues")).not.toBeInTheDocument(); + }); + + it("shows the name/path mismatch as a distinct, named finding", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("right-name")); + const issues = screen.getByTestId("skill-issues"); + expect(within(issues).getByText("name-path-mismatch")).toBeInTheDocument(); + }); + + it("shows the dynamic warning and no manifest table", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("dynamic-report")); + expect(screen.getByText("Dynamic resources")).toBeInTheDocument(); + expect(screen.queryByTestId("skill-manifest")).not.toBeInTheDocument(); + // "Verify all" has nothing to verify, so it is disabled rather than a + // button that silently does nothing. + expect(screen.getByRole("button", { name: /Verify all/ })).toBeDisabled(); + }); + + it("verifies a file whose bytes match its digest", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + expect(await screen.findByText("verified")).toBeInTheDocument(); + }); + + it("reports a digest mismatch loudly, with both digests", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("tampered")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + expect(await screen.findByText("Digest mismatch")).toBeInTheDocument(); + expect( + screen.getByText(`expected sha256:${"b".repeat(64)}`), + ).toBeInTheDocument(); + }); + + it("verifies a single file from its own row button", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + const manifest = screen.getByTestId("skill-manifest"); + await user.click(within(manifest).getByRole("button", { name: "Verify" })); + expect(await screen.findByText("verified")).toBeInTheDocument(); + }); + + it("reports a failed read as a read failure, not a mismatch", async () => { + const user = userEvent.setup(); + const onReadSkillFile = vi.fn().mockRejectedValue(new Error("403")); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + expect(await screen.findByText("Could not read file")).toBeInTheDocument(); + expect(screen.getByText("403")).toBeInTheDocument(); + }); + + it("wraps a non-Error read rejection", async () => { + const user = userEvent.setup(); + const onReadSkillFile = vi.fn().mockRejectedValue("plain string"); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + expect(await screen.findByText("plain string")).toBeInTheDocument(); + }); + + it("shows the SKILL.md preview on demand", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); + expect(await screen.findByTestId("skill-md-preview")).toBeInTheDocument(); + }); + + it("reports a failed SKILL.md read", async () => { + const user = userEvent.setup(); + const onReadSkillFile = vi.fn().mockRejectedValue(new Error("gone")); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); + expect( + await screen.findByText("Could not read SKILL.md"), + ).toBeInTheDocument(); + }); + + it("wraps a non-Error SKILL.md rejection", async () => { + const user = userEvent.setup(); + const onReadSkillFile = vi.fn().mockRejectedValue("bare"); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); + expect(await screen.findByText("bare")).toBeInTheDocument(); + }); + + it("drops verification results when the selection changes", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + expect(await screen.findByText("verified")).toBeInTheDocument(); + + // A verdict belongs to the skill it was computed for; carrying it across a + // selection change would attribute one skill's result to another. + await user.click(screen.getByText("tampered")); + expect(screen.queryByText("verified")).not.toBeInTheDocument(); + expect(screen.getByText("—")).toBeInTheDocument(); + }); + + it("drops the SKILL.md preview when the selection changes", async () => { + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /View SKILL.md/ })); + expect(await screen.findByTestId("skill-md-preview")).toBeInTheDocument(); + await user.click(screen.getByText("tampered")); + expect(screen.queryByTestId("skill-md-preview")).not.toBeInTheDocument(); + }); + + it("renders an em dash for a manifest entry with no size", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + const manifest = screen.getByTestId("skill-manifest"); + // Three em dashes in the row: the size cell, the digest cell, and the + // not-yet-run verification badge — which stays distinct from + // "unverifiable" so an absent digest is never mistaken for an unrun check. + expect(within(manifest).getAllByText("—")).toHaveLength(3); + }); + + it("truncates a long digest but shows a short one whole", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + expect(screen.getByText("sha256:short")).toBeInTheDocument(); + }); + + it("reports a file with no advertised digest as unverifiable, not verified", async () => { + const user = userEvent.setup(); + renderWithMantine( + , + ); + await user.click(screen.getByText("data-analysis")); + await user.click(screen.getByRole("button", { name: /Verify all/ })); + expect(await screen.findByText("unverifiable")).toBeInTheDocument(); + }); +}); diff --git a/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx new file mode 100644 index 00000000..d325c1a6 --- /dev/null +++ b/clients/web/src/components/screens/SkillsScreen/SkillsScreen.tsx @@ -0,0 +1,588 @@ +import { useCallback, useMemo, useState } from "react"; +import { + Alert, + Badge, + Button, + Card, + Code, + Flex, + Group, + NavLink, + ScrollArea, + Stack, + Table, + Text, + TextInput, +} from "@mantine/core"; +import { MdRefresh, MdSearch, MdVerifiedUser } from "react-icons/md"; +import type { + SkillEntry, + SkillResource, +} from "@inspector/core/mcp/skillsSchemas.js"; +import { DYNAMIC_RESOURCES } from "@inspector/core/mcp/skillsSchemas.js"; +import { + checkSkillConformance, + skillDisplayName, + totalSkillBytes, + verifySkillResource, + type SkillIssue, + type SkillVerification, +} from "@inspector/core/mcp/skills.js"; +import { ContentViewer } from "../../elements/ContentViewer/ContentViewer"; +import { useValueChange } from "../../../hooks/useValueChange"; +import { + skillFileBytes, + type SkillFileContents, +} from "../../../utils/skillFileBytes"; + +/** Per-file verification progress, keyed by the manifest entry's URI. */ +type FileState = + | { status: "pending" } + | { status: "done"; verification: SkillVerification } + | { status: "error"; message: string }; + +export interface SkillsScreenProps { + skills: SkillEntry[]; + /** Pages the last `skills/list` walk took; shown so pagination is visible. */ + pageCount: number; + /** A failed list walk, rendered above the sidebar list. */ + loadError?: Error | null; + ui: SkillsUiState; + onUiChange: (next: SkillsUiState) => void; + onRefreshList: () => void; + /** Fetch one skill file's contents via `resources/read`, on demand. */ + onReadSkillFile: (uri: string) => Promise; +} + +/** + * Selection and the sidebar search — controlled by the parent (App) as one + * object so they persist across tab navigation within a live session (#1417). + * Verification results stay local to the screen: they are derived from a live + * `resources/read` round trip that is torn down with the screen, so persisting + * them would restore a verdict without the fetch that produced it. + */ +export interface SkillsUiState { + selectedSkillUri?: string; + search: string; +} + +const ScreenLayout = Flex.withProps({ + variant: "screen", + h: "calc(100dvh - var(--app-shell-header-height, 0px) - var(--app-shell-footer-height, 0px))", + gap: "md", + p: "xl", + align: "flex-start", +}); + +const Sidebar = Stack.withProps({ + w: 340, + flex: "0 0 auto", +}); + +const SidebarCard = Card.withProps({ + withBorder: true, + padding: "lg", +}); + +const DetailCard = Card.withProps({ + withBorder: true, + padding: "lg", + flex: 1, + h: "100%", +}); + +const DetailScroll = ScrollArea.withProps({ + type: "auto", + scrollbars: "y", + offsetScrollbars: true, + h: "100%", +}); + +const EmptyState = Text.withProps({ + c: "dimmed", + ta: "center", + py: "xl", +}); + +const ControlsRow = Group.withProps({ + justify: "space-between", + wrap: "nowrap", + gap: "sm", +}); + +const SearchInput = TextInput.withProps({ + size: "xs", + flex: 1, + leftSection: , +}); + +const RefreshButton = Button.withProps({ + variant: "subtle", + size: "compact-xs", + leftSection: , +}); + +const VerifyButton = Button.withProps({ + variant: "light", + size: "compact-sm", + leftSection: , +}); + +// A `Text` renders a `

`, so a section heading must never *wrap* the count +// badge beside it — a `

` inside a `

` is invalid HTML that React reports +// as a hydration error and the Storybook run fails on. Heading and badge sit +// side by side in an `InlineRow` instead. +const SectionHeading = Text.withProps({ + fw: 600, + size: "sm", +}); + +const MonoCaption = Text.withProps({ + size: "xs", + c: "dimmed", + ff: "monospace", +}); + +const DetailStack = Stack.withProps({ + gap: "md", +}); + +const IssueStack = Stack.withProps({ + gap: "xs", +}); + +const ManifestTable = Table.withProps({ + striped: true, + withTableBorder: true, + fz: "xs", + verticalSpacing: "xs", +}); + +const CountBadge = Badge.withProps({ + size: "xs", + variant: "light", +}); + +// A tight, non-wrapping row — used for the detail-pane action pair and for the +// badge + Verify button inside a manifest cell. +const InlineRow = Group.withProps({ + gap: "xs", + wrap: "nowrap", +}); + +const RowVerifyButton = Button.withProps({ + variant: "subtle", + size: "compact-xs", +}); + +const SkillTitle = Text.withProps({ + fw: 600, + size: "lg", + truncate: true, +}); + +/** Colour token for a finding's severity — errors read as failures. */ +function issueColor(issue: SkillIssue): string { + return issue.severity === "error" ? "red" : "yellow"; +} + +/** Colour token for a per-file verification verdict. */ +function verificationColor(status: SkillVerification["status"]): string { + if (status === "verified") return "green"; + if (status === "mismatch") return "red"; + return "yellow"; +} + +/** + * The short label a manifest row shows for its verdict. `—` (not yet run) is + * deliberately distinct from `unverifiable` (run, but nothing to compare + * against): conflating them would hide the fact that a server advertised no + * digest. + */ +function verificationLabel(state: FileState | undefined): string { + if (!state) return "—"; + if (state.status === "pending") return "checking…"; + if (state.status === "error") return "read failed"; + return state.verification.status; +} + +/** `sha256:abcd…wxyz`, so a long digest stays readable in a table cell. */ +function shortDigest(digest: string | undefined): string { + if (!digest) return "—"; + return digest.length <= 24 ? digest : `${digest.slice(0, 16)}…`; +} + +/** + * The Skills screen (SEP-2640) — a conformance view, not just a list. + * + * The sidebar lists the skills the server enumerated; the detail pane shows the + * entry's frontmatter, every conformance finding + * (`checkSkillConformance`), and the resource manifest with a per-file + * verification verdict. Verification is on demand: SEP-2640 says a + * `resources/read` of a skill file is not a load and confers no standing, so + * the Inspector fetches only what the user asks it to. + */ +export function SkillsScreen({ + skills, + pageCount, + loadError, + ui, + onUiChange, + onRefreshList, + onReadSkillFile, +}: SkillsScreenProps) { + const { selectedSkillUri, search } = ui; + const [fileStates, setFileStates] = useState>({}); + const [preview, setPreview] = useState(null); + const [previewError, setPreviewError] = useState(null); + + // Changing the selection invalidates every verdict and the SKILL.md preview: + // they belong to the skill that was selected. Adjusted DURING RENDER via + // `useValueChange` rather than in an effect, so the new skill never paints + // for a frame carrying the previous one's verification results. + useValueChange(selectedSkillUri, () => { + setFileStates({}); + setPreview(null); + setPreviewError(null); + }); + + const filtered = useMemo(() => { + const needle = search.trim().toLowerCase(); + if (!needle) return skills; + return skills.filter( + (skill) => + skillDisplayName(skill).toLowerCase().includes(needle) || + skill.uri.toLowerCase().includes(needle), + ); + }, [skills, search]); + + const selected = useMemo( + () => skills.find((skill) => skill.uri === selectedSkillUri), + [skills, selectedSkillUri], + ); + + const issues = useMemo( + () => (selected ? checkSkillConformance(selected) : []), + [selected], + ); + + const manifest: SkillResource[] = useMemo( + () => + selected && selected.resources !== DYNAMIC_RESOURCES + ? selected.resources + : [], + [selected], + ); + + const verifyFile = useCallback( + async (resource: SkillResource) => { + setFileStates((prev) => ({ + ...prev, + [resource.uri]: { status: "pending" }, + })); + try { + const contents = await onReadSkillFile(resource.uri); + const verification = await verifySkillResource( + resource, + skillFileBytes(contents), + ); + setFileStates((prev) => ({ + ...prev, + [resource.uri]: { status: "done", verification }, + })); + } catch (err) { + setFileStates((prev) => ({ + ...prev, + [resource.uri]: { + status: "error", + message: err instanceof Error ? err.message : String(err), + }, + })); + } + }, + [onReadSkillFile], + ); + + const verifyAll = useCallback(() => { + // Held rather than floated: each `verifyFile` owns its own failures (it + // records them as per-file state), and this handler cannot be async, so the + // settled promise is discarded explicitly at one place instead of per file. + void Promise.all(manifest.map((resource) => verifyFile(resource))); + }, [manifest, verifyFile]); + + const showSkillMd = useCallback(() => { + if (!selected) return; + // A click handler cannot await, and this chain terminates in its own + // `catch` that surfaces the message in the preview slot. + void onReadSkillFile(selected.uri) + .then((contents) => { + setPreview(contents); + setPreviewError(null); + }) + .catch((err: unknown) => { + setPreview(null); + setPreviewError(err instanceof Error ? err.message : String(err)); + }); + }, [onReadSkillFile, selected]); + + const errorCount = issues.filter((i) => i.severity === "error").length; + const warningCount = issues.length - errorCount; + + return ( + // `data-*` readiness contract for the headless tab smoke (#2148); see + // clients/web/README.md#core-tab-automation-contract. + + + + + + + onUiChange({ ...ui, search: event.currentTarget.value }) + } + /> + Refresh + + {loadError && ( + + {loadError.message} + + )} + {filtered.length === 0 ? ( + No skills + ) : ( + filtered.map((skill) => { + const skillIssues = checkSkillConformance(skill); + const errors = skillIssues.filter( + (i) => i.severity === "error", + ).length; + return ( + + onUiChange({ ...ui, selectedSkillUri: skill.uri }) + } + rightSection={ + skillIssues.length > 0 ? ( + 0 ? "red" : "yellow"}> + {skillIssues.length} + + ) : undefined + } + /> + ); + }) + )} + + {skills.length} skill(s) over {pageCount} page(s) + + + + + + + {!selected ? ( + Select a skill to view details + ) : ( + + + + {skillDisplayName(selected)} + {selected.uri} + + + {selected.frontmatter.description && ( + {selected.frontmatter.description} + )} + + + + Conformance + 0 ? "red" : "green"}> + {errorCount} error(s), {warningCount} warning(s) + + + {issues.length === 0 ? ( + + No structural issues found in this entry. + + ) : ( + + {issues.map((issue) => ( + + + {issue.message} + {issue.resourceUri && ( + {issue.resourceUri} + )} + + + ))} + + )} + + + + + + Resources + + {manifest.length} file(s), {totalSkillBytes(manifest)}{" "} + bytes + + + + + View SKILL.md + + + Verify all + + + + {selected.resources === DYNAMIC_RESOURCES ? ( + + This skill declares{" "} + resources: "dynamic" — its files are + generated, so no manifest is advertised and integrity cannot + be verified. + + ) : ( + + + + URI + Size + Digest + Verification + + + + {manifest.map((resource) => { + const state = fileStates[resource.uri]; + const color = + state?.status === "done" + ? verificationColor(state.verification.status) + : state?.status === "error" + ? "red" + : "gray"; + return ( + + {resource.uri} + {resource.size ?? "—"} + {shortDigest(resource.digest)} + + + + {verificationLabel(state)} + + void verifyFile(resource)} + > + Verify + + + + + ); + })} + + + )} + {manifest.map((resource) => { + const state = fileStates[resource.uri]; + if (state?.status === "done") { + const { verification } = state; + if (verification.status === "mismatch") { + return ( + + + {resource.uri} + + expected {verification.expectedDigest} + + + actual {verification.actualDigest} + + + + ); + } + return null; + } + if (state?.status === "error") { + return ( + + + {resource.uri} + {state.message} + + + ); + } + return null; + })} + + + {previewError && ( + + {previewError} + + )} + {preview && ( + + SKILL.md + + + )} + + + Frontmatter + + + + + )} + + + ); +} diff --git a/clients/web/src/components/screens/screenUiState.ts b/clients/web/src/components/screens/screenUiState.ts index 1da027cf..2e52459f 100644 --- a/clients/web/src/components/screens/screenUiState.ts +++ b/clients/web/src/components/screens/screenUiState.ts @@ -7,6 +7,7 @@ import type { ToolsUiState } from "./ToolsScreen/ToolsScreen"; import type { PromptsUiState } from "./PromptsScreen/PromptsScreen"; import type { ResourcesUiState } from "./ResourcesScreen/ResourcesScreen"; import type { AppsUiState } from "./AppsScreen/AppsScreen"; +import type { SkillsUiState } from "./SkillsScreen/SkillsScreen"; import type { TasksUiState } from "./TasksScreen/TasksScreen"; import type { LogsUiState } from "./LoggingScreen/LoggingScreen"; import type { ProtocolUiState } from "./ProtocolScreen/ProtocolScreen"; @@ -43,6 +44,11 @@ export const EMPTY_APPS_UI: AppsUiState = { search: "", }; +export const EMPTY_SKILLS_UI: SkillsUiState = { + selectedSkillUri: undefined, + search: "", +}; + export const EMPTY_TASKS_UI: TasksUiState = { search: "", statusFilter: undefined, @@ -74,6 +80,7 @@ export const TAB_UI_REGISTRY = { Tools: { empty: EMPTY_TOOLS_UI }, Prompts: { empty: EMPTY_PROMPTS_UI }, Resources: { empty: EMPTY_RESOURCES_UI }, + Skills: { empty: EMPTY_SKILLS_UI }, Tasks: { empty: EMPTY_TASKS_UI }, Logs: { empty: EMPTY_LOGS_UI }, Protocol: { empty: EMPTY_PROTOCOL_UI }, diff --git a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx index 24054fda..0c227379 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx @@ -29,6 +29,7 @@ import type { ResourcesPanelProps, ServerListProps, ShellProps, + SkillsPanelProps, TasksPanelProps, ToolsPanelProps, } from "./types"; @@ -38,6 +39,7 @@ import { EMPTY_APPS_UI, EMPTY_PROMPTS_UI, EMPTY_RESOURCES_UI, + EMPTY_SKILLS_UI, EMPTY_TASKS_UI, EMPTY_LOGS_UI, EMPTY_PROTOCOL_UI, @@ -424,6 +426,15 @@ const appsArgs: AppsPanelProps = { onRefreshApps: fn(), }; +const skillsArgs: SkillsPanelProps = { + skills: [], + skillsPageCount: 0, + skillsUi: EMPTY_SKILLS_UI, + onSkillsUiChange: fn(), + onRefreshSkills: fn(), + onReadSkillFile: fn(async () => ({ text: "" })), +}; + const tasksArgs: TasksPanelProps = { tasks: demoTasks, progressByTaskId: demoProgressByTaskId, @@ -484,6 +495,7 @@ const meta: Meta = { prompts: promptsArgs, resources: resourcesArgs, apps: appsArgs, + skills: skillsArgs, tasks: tasksArgs, logs: logsArgs, protocol: protocolArgs, diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index 20fd24db..d720d1e4 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -39,6 +39,7 @@ import { EMPTY_APPS_UI, EMPTY_PROMPTS_UI, EMPTY_RESOURCES_UI, + EMPTY_SKILLS_UI, EMPTY_TASKS_UI, EMPTY_LOGS_UI, EMPTY_PROTOCOL_UI, @@ -170,6 +171,15 @@ function makeProps(...overrides: PropOverrides[]): InspectorViewProps { onRefreshApps: vi.fn(), ...mergeBundle("apps", overrides), }, + skills: { + skills: [], + skillsPageCount: 0, + skillsUi: EMPTY_SKILLS_UI, + onSkillsUiChange: vi.fn(), + onRefreshSkills: vi.fn(), + onReadSkillFile: vi.fn().mockResolvedValue({ text: "" }), + ...mergeBundle("skills", overrides), + }, tasks: { tasks: [], tasksUi: EMPTY_TASKS_UI, @@ -1369,6 +1379,50 @@ describe("InspectorView", () => { expect(labels).not.toContain("Tasks"); }); + it("hides the Skills tab when the server declares no skills extension (#2234)", async () => { + renderWithMantine( + , + ); + const radios = await screen.findAllByRole("radio"); + const labels = radios.map((r) => r.getAttribute("value")); + expect(labels).toContain("Tools"); + expect(labels).not.toContain("Skills"); + }); + + it("shows the Skills tab on a LEGACY connection that declares the extension (#2234)", async () => { + // Unlike Tasks, Skills is not era-gated: `skills/*` are not spec method + // names in either codec, so a legacy-era server that declares the + // extension is serving it and must get the tab. + renderWithMantine( + , + ); + const radios = await screen.findAllByRole("radio"); + const labels = radios.map((r) => r.getAttribute("value")); + expect(labels).toContain("Skills"); + }); + it("shows the Tasks tab when the server advertises tasks even with no tasks yet", async () => { renderWithMantine( { if (t === NETWORK_TAB && isStdio) return false; // Console is the stdio process's stderr stream — shown only for stdio @@ -663,6 +683,7 @@ export function InspectorView({ if (t === "Apps" && !hasApps) return false; if (t === "Prompts" && !hasPrompts) return false; if (t === "Resources" && !hasResources) return false; + if (t === SKILLS_TAB && !hasSkills) return false; if (t === "Tasks" && !hasTasks) return false; if (t === "Logs" && !hasLogging) return false; return true; @@ -1032,6 +1053,15 @@ export function InspectorView({ sortDirection: consoleSort, onSortChange: setConsoleSort, }; + const skillsScreenProps = { + skills, + pageCount: skillsPageCount, + loadError: skillsLoadError, + ui: skillsUi, + onUiChange: onSkillsUiChange, + onRefreshList: onRefreshSkills, + onReadSkillFile, + }; const tasksScreenProps = { tasks, progressByTaskId, @@ -1240,6 +1270,9 @@ export function InspectorView({ onCompactChange={setResourcesCompact} /> + + + diff --git a/clients/web/src/components/views/InspectorView/types.ts b/clients/web/src/components/views/InspectorView/types.ts index e4b7ce72..02a9ee86 100644 --- a/clients/web/src/components/views/InspectorView/types.ts +++ b/clients/web/src/components/views/InspectorView/types.ts @@ -59,6 +59,9 @@ import type { ResourcesUiState, } from "../../screens/ResourcesScreen/ResourcesScreen"; import type { LogsUiState } from "../../screens/LoggingScreen/LoggingScreen"; +import type { SkillsUiState } from "../../screens/SkillsScreen/SkillsScreen"; +import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas.js"; +import type { SkillFileContents } from "../../../utils/skillFileBytes"; import type { TasksUiState } from "../../screens/TasksScreen/TasksScreen"; import type { ProtocolUiState } from "../../screens/ProtocolScreen/ProtocolScreen"; import type { NetworkUiState } from "../../screens/NetworkScreen/NetworkScreen"; @@ -309,6 +312,19 @@ export interface AppsPanelProps { } /** The Tasks monitor: the task list, its progress map, and actions. */ +/** The Skills screen (SEP-2640): the enumerated skills and their verification. */ +export interface SkillsPanelProps { + skills: SkillEntry[]; + /** Pages the last `skills/list` walk took. */ + skillsPageCount: number; + skillsLoadError?: Error | null; + skillsUi: SkillsUiState; + onSkillsUiChange: (next: SkillsUiState) => void; + onRefreshSkills: () => void; + /** Read one skill file (`resources/read`) so its digest can be checked. */ + onReadSkillFile: (uri: string) => Promise; +} + export interface TasksPanelProps { tasks: Task[]; progressByTaskId?: Record; diff --git a/clients/web/src/hooks/useInspectorStores.test.tsx b/clients/web/src/hooks/useInspectorStores.test.tsx index 6e7c2955..2ec027e2 100644 --- a/clients/web/src/hooks/useInspectorStores.test.tsx +++ b/clients/web/src/hooks/useInspectorStores.test.tsx @@ -63,6 +63,9 @@ vi.mock("@inspector/core/mcp/state/managedResourceTemplatesState.js", () => ({ vi.mock("@inspector/core/mcp/state/managedRequestorTasksState.js", () => ({ ManagedRequestorTasksState: fakeState("managedRequestorTasksState"), })); +vi.mock("@inspector/core/mcp/state/managedSkillsState.js", () => ({ + ManagedSkillsState: fakeState("managedSkillsState"), +})); vi.mock("@inspector/core/mcp/state/resourceSubscriptionsState.js", () => ({ ResourceSubscriptionsState: fakeState("resourceSubscriptionsState"), })); @@ -111,6 +114,14 @@ vi.mock("@inspector/core/react/useManagedRequestorTasks.js", () => ({ clearCompleted: vi.fn(), }), })); +vi.mock("@inspector/core/react/useManagedSkills.js", () => ({ + useManagedSkills: () => ({ + skills: [{ uri: "skill://s/SKILL.md" }], + pageCount: 1, + error: null, + refresh: vi.fn().mockResolvedValue(undefined), + }), +})); vi.mock("@inspector/core/react/useResourceSubscriptions.js", () => ({ useResourceSubscriptions: () => ({ subscriptions: [{ uri: "sub://r" }], @@ -181,6 +192,7 @@ const STORE_NAMES = [ "pagedResourcesState", "managedResourceTemplatesState", "managedRequestorTasksState", + "managedSkillsState", "resourceSubscriptionsState", "messageLogState", "fetchRequestLogState", @@ -201,7 +213,7 @@ describe("useInspectorStores", () => { expect(built).toHaveLength(0); }); - it("builds all twelve stores against the client", () => { + it("builds all thirteen stores against the client", () => { const h = harness(); const c = client(); h.run((api) => api.createStores(c, fetchLogOptions)); diff --git a/clients/web/src/hooks/useInspectorStores.ts b/clients/web/src/hooks/useInspectorStores.ts index ae98d8be..3ad32fad 100644 --- a/clients/web/src/hooks/useInspectorStores.ts +++ b/clients/web/src/hooks/useInspectorStores.ts @@ -13,6 +13,7 @@ import type { ResourceSubscriptionStreamState, StderrLogEntry, } from "@inspector/core/mcp/types.js"; +import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas.js"; import type { InspectorClient } from "@inspector/core/mcp/index.js"; import { ManagedToolsState } from "@inspector/core/mcp/state/managedToolsState.js"; import { ManagedPromptsState } from "@inspector/core/mcp/state/managedPromptsState.js"; @@ -22,6 +23,7 @@ import { PagedPromptsState } from "@inspector/core/mcp/state/pagedPromptsState.j import { PagedResourcesState } from "@inspector/core/mcp/state/pagedResourcesState.js"; import { ManagedResourceTemplatesState } from "@inspector/core/mcp/state/managedResourceTemplatesState.js"; import { ManagedRequestorTasksState } from "@inspector/core/mcp/state/managedRequestorTasksState.js"; +import { ManagedSkillsState } from "@inspector/core/mcp/state/managedSkillsState.js"; import { ResourceSubscriptionsState } from "@inspector/core/mcp/state/resourceSubscriptionsState.js"; import { MessageLogState } from "@inspector/core/mcp/state/messageLogState.js"; import { @@ -37,6 +39,7 @@ import { usePagedPrompts } from "@inspector/core/react/usePagedPrompts.js"; import { usePagedResources } from "@inspector/core/react/usePagedResources.js"; import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js"; import { useManagedRequestorTasks } from "@inspector/core/react/useManagedRequestorTasks.js"; +import { useManagedSkills } from "@inspector/core/react/useManagedSkills.js"; import { useResourceSubscriptions } from "@inspector/core/react/useResourceSubscriptions.js"; import { useMessageLog } from "@inspector/core/react/useMessageLog.js"; import { useFetchRequestLog } from "@inspector/core/react/useFetchRequestLog.js"; @@ -44,9 +47,9 @@ import { useStderrLog } from "@inspector/core/react/useStderrLog.js"; import { usePaginatedList, type PaginatedListModel } from "./usePaginatedList"; /** - * The twelve per-session state managers. They are created together (one + * The thirteen per-session state managers. They are created together (one * `InspectorClient`, one set of stores) and torn down together, so they are - * held as one slot rather than twelve — a partially-replaced set would leave + * held as one slot rather than thirteen — a partially-replaced set would leave * some stores listening to a client the others had already left. */ export interface InspectorStores { @@ -58,6 +61,7 @@ export interface InspectorStores { pagedResourcesState: PagedResourcesState; managedResourceTemplatesState: ManagedResourceTemplatesState; managedRequestorTasksState: ManagedRequestorTasksState; + managedSkillsState: ManagedSkillsState; resourceSubscriptionsState: ResourceSubscriptionsState; messageLogState: MessageLogState; fetchRequestLogState: FetchRequestLogState; @@ -131,6 +135,12 @@ export interface UseInspectorStoresResult { tasks: Task[]; refreshTasks: () => Promise; clearCompletedTasks: () => void; + /** The server's skills (SEP-2640); empty when it declared no extension. */ + skills: SkillEntry[]; + /** Pages the last `skills/list` walk took. */ + skillsPageCount: number; + skillsLoadError: Error | null; + refreshSkills: () => Promise; subscriptions: InspectorResourceSubscription[]; subscriptionStreamState: ResourceSubscriptionStreamState; messages: MessageEntry[]; @@ -192,6 +202,7 @@ export function useInspectorStores({ client, ), managedRequestorTasksState: new ManagedRequestorTasksState(client), + managedSkillsState: new ManagedSkillsState(client), resourceSubscriptionsState: new ResourceSubscriptionsState( client, managedResourcesState, @@ -310,6 +321,15 @@ export function useInspectorStores({ inspectorClient, stores?.managedRequestorTasksState ?? null, ); + // Skills (SEP-2640). The store no-ops when the server declared no extension, + // so this hook is unconditional like the rest — the Skills *tab* is what is + // gated, in `InspectorView`. + const { + skills, + pageCount: skillsPageCount, + error: skillsLoadError, + refresh: refreshSkills, + } = useManagedSkills(inspectorClient, stores?.managedSkillsState ?? null); const { subscriptions, streamState: subscriptionStreamState } = useResourceSubscriptions(stores?.resourceSubscriptionsState ?? null); const { messages } = useMessageLog(stores?.messageLogState ?? null); @@ -344,6 +364,10 @@ export function useInspectorStores({ tasks, refreshTasks, clearCompletedTasks, + skills, + skillsPageCount, + skillsLoadError, + refreshSkills, subscriptions, subscriptionStreamState, messages, diff --git a/clients/web/src/hooks/useServerCommands.test.tsx b/clients/web/src/hooks/useServerCommands.test.tsx index b8de9e2d..a675a284 100644 --- a/clients/web/src/hooks/useServerCommands.test.tsx +++ b/clients/web/src/hooks/useServerCommands.test.tsx @@ -162,6 +162,7 @@ function spies() { setModernLogLevel: vi.fn(), clearCompletedTasks: vi.fn(), refreshTasks: vi.fn().mockResolvedValue(undefined), + refreshSkills: vi.fn().mockResolvedValue(undefined), refreshTools: vi.fn().mockResolvedValue(undefined), refreshPrompts: vi.fn().mockResolvedValue(undefined), refreshResources: vi.fn().mockResolvedValue(undefined), @@ -271,6 +272,7 @@ function harness(initial: HarnessProps = {}): Harness { activeToolCallTaskIdRef, clearCompletedTasks: s.clearCompletedTasks, refreshTasks: s.refreshTasks, + refreshSkills: s.refreshSkills, paginatedLists: p.paginatedLists ?? false, paginatedListsOverride: { record: s.record, valueFor: s.valueFor }, toolsPagination, @@ -840,6 +842,88 @@ describe("onReadResourceContents", () => { }); }); +describe("onReadSkillFile (#2234)", () => { + const skillUri = "skill://demo/reference.md"; + + it("returns the block whose uri matches exactly", async () => { + const c = client({ + readResource: vi.fn().mockResolvedValue({ + result: { + contents: [ + { uri: "skill://demo/other.md", text: "wrong" }, + { uri: skillUri, text: "right", mimeType: "text/markdown" }, + ], + }, + }), + }); + const h = harness({ client: c }); + await expect(h.api().onReadSkillFile(skillUri)).resolves.toEqual({ + text: "right", + mimeType: "text/markdown", + }); + }); + + it("accepts a sole block whose uri the server echoed back differently", async () => { + // `resources/read` answers the URI it was asked for, so a single-block + // response IS that block even when the echo differs in form. + const c = client({ + readResource: vi.fn().mockResolvedValue({ + result: { contents: [{ uri: "SKILL://DEMO/reference.md", text: "x" }] }, + }), + }); + const h = harness({ client: c }); + await expect(h.api().onReadSkillFile(skillUri)).resolves.toEqual({ + text: "x", + }); + }); + + it("passes a blob block through as a blob", async () => { + const c = client({ + readResource: vi.fn().mockResolvedValue({ + result: { contents: [{ uri: skillUri, blob: "aGk=" }] }, + }), + }); + const h = harness({ client: c }); + await expect(h.api().onReadSkillFile(skillUri)).resolves.toEqual({ + blob: "aGk=", + }); + }); + + it("throws when no block answers the uri", async () => { + // Returning an empty payload here would let the caller hash zero bytes and + // report a confident digest *mismatch* for a response that carried nothing. + const c = client({ + readResource: vi.fn().mockResolvedValue({ + result: { + contents: [ + { uri: "skill://demo/a.md", text: "a" }, + { uri: "skill://demo/b.md", text: "b" }, + ], + }, + }), + }); + const h = harness({ client: c }); + await expect(h.api().onReadSkillFile(skillUri)).rejects.toThrow( + /returned no content/, + ); + }); + + it("throws when there is no client", async () => { + const h = harness(); + await expect(h.api().onReadSkillFile(skillUri)).rejects.toThrow( + "Client is not connected", + ); + }); +}); + +describe("onRefreshSkills (#2234)", () => { + it("drives the store refresh in the background", () => { + const h = harness(); + h.api().onRefreshSkills(); + expect(h.spies.refreshSkills).toHaveBeenCalled(); + }); +}); + describe("subscriptions and completion", () => { it("subscribes and unsubscribes through the client", async () => { const c = client(); diff --git a/clients/web/src/hooks/useServerCommands.tsx b/clients/web/src/hooks/useServerCommands.tsx index 61aabae8..9488e352 100644 --- a/clients/web/src/hooks/useServerCommands.tsx +++ b/clients/web/src/hooks/useServerCommands.tsx @@ -34,6 +34,7 @@ import type { } from "../components/screens/ToolsScreen/ToolsScreen"; import type { GetPromptState } from "../components/screens/PromptsScreen/PromptsScreen"; import type { ReadResourceState } from "../components/screens/ResourcesScreen/ResourcesScreen"; +import type { SkillFileContents } from "../utils/skillFileBytes"; import { UrlElicitationErrorToastMessage } from "../components/elements/Toasts/UrlElicitationErrorToastMessage"; import { errorCodeOf, @@ -161,6 +162,8 @@ export interface UseServerCommandsOptions { activeToolCallTaskIdRef: { current: string | undefined }; clearCompletedTasks: () => void; refreshTasks: () => Promise; + /** Re-walk `skills/list` (SEP-2640). */ + refreshSkills: () => Promise; // --- The list stores and their two fetch modes (#1721). --- paginatedLists: boolean; @@ -210,6 +213,12 @@ export interface ServerCommands { onReadResourceContents: ( uri: string, ) => Promise>["result"]>; + /** + * Read one skill file's contents (SEP-2640), for digest verification. Returns + * the single content block that answers the URI, narrowed to the two fields + * the digest is taken over. + */ + onReadSkillFile: (uri: string) => Promise; onSubscribeResource: (uri: string) => void; onUnsubscribeResource: (uri: string) => void; onCompleteArgument: ( @@ -228,6 +237,7 @@ export interface ServerCommands { onRefreshTools: () => void; onRefreshPrompts: () => void; onRefreshResources: () => void; + onRefreshSkills: () => void; onRefreshTasks: () => void; onTogglePaginatedLists: (value: boolean) => void; onLoadMoreTools: () => void; @@ -269,6 +279,7 @@ export function useServerCommands({ activeToolCallTaskIdRef, clearCompletedTasks, refreshTasks, + refreshSkills, paginatedLists, paginatedListsOverride, toolsPagination, @@ -932,6 +943,44 @@ export function useServerCommands({ runCommandInBackground(() => resourcesPagination.onLoadMore(), "ambient"), [resourcesPagination, runCommandInBackground], ); + // Skill files are fetched on demand, never pre-fetched: SEP-2640 is explicit + // that a `resources/read` of a skill file is not a load and confers no + // standing, so the Inspector reads only what the user asks it to verify. + // Routed through `onReadResourceContents` so a skill read gets the same + // auth-recovery retry every other read does. + const onReadSkillFile = useCallback( + async (uri: string): Promise => { + const result = await onReadResourceContents(uri); + // `resources/read` answers the URI it was asked for, so a single-block + // response is that block even when the server echoes the URI back in a + // slightly different form; an exact match wins when there are several. + const block = + result.contents.find((c) => c.uri === uri) ?? + (result.contents.length === 1 ? result.contents[0] : undefined); + if (!block) { + throw new Error(`resources/read returned no content for ${uri}`); + } + // `contents` is a union of the text and blob shapes, each with its own + // payload field required — so `in` is what narrows it, not a `typeof` on + // a property one arm does not declare. + return { + ...("text" in block ? { text: block.text } : { blob: block.blob }), + ...(typeof block.mimeType === "string" + ? { mimeType: block.mimeType } + : {}), + }; + }, + [onReadResourceContents], + ); + + const onRefreshSkills = useCallback(() => { + runCommandInBackground( + () => refreshSkills(), + "ambient", + "Failed to refresh skills", + ); + }, [refreshSkills, runCommandInBackground]); + const onRefreshTasks = useCallback(() => { runCommandInBackground( () => refreshTasks(), @@ -947,6 +996,7 @@ export function useServerCommands({ onGetPrompt, onReadResource, onReadResourceContents, + onReadSkillFile, onSubscribeResource, onUnsubscribeResource, onCompleteArgument, @@ -958,6 +1008,7 @@ export function useServerCommands({ onRefreshTools, onRefreshPrompts, onRefreshResources, + onRefreshSkills, onRefreshTasks, onTogglePaginatedLists, onLoadMoreTools, diff --git a/clients/web/src/hooks/useTabUiState.ts b/clients/web/src/hooks/useTabUiState.ts index 88bd5cdd..dab9af51 100644 --- a/clients/web/src/hooks/useTabUiState.ts +++ b/clients/web/src/hooks/useTabUiState.ts @@ -8,6 +8,7 @@ import { EMPTY_PROMPTS_UI, EMPTY_PROTOCOL_UI, EMPTY_RESOURCES_UI, + EMPTY_SKILLS_UI, EMPTY_TASKS_UI, EMPTY_TOOLS_UI, } from "../components/screens/screenUiState"; @@ -67,6 +68,7 @@ export function useTabUiState(): TabUiStateResult { const [promptsUi, setPromptsUi] = useState(EMPTY_PROMPTS_UI); const [resourcesUi, setResourcesUi] = useState(EMPTY_RESOURCES_UI); const [appsUi, setAppsUi] = useState(EMPTY_APPS_UI); + const [skillsUi, setSkillsUi] = useState(EMPTY_SKILLS_UI); const [tasksUi, setTasksUi] = useState(EMPTY_TASKS_UI); const [logsUi, setLogsUi] = useState(EMPTY_LOGS_UI); const [protocolUi, setProtocolUi] = useState(EMPTY_PROTOCOL_UI); @@ -85,6 +87,7 @@ export function useTabUiState(): TabUiStateResult { promptsUi, resourcesUi, appsUi, + skillsUi, tasksUi, logsUi, protocolUi, @@ -96,6 +99,7 @@ export function useTabUiState(): TabUiStateResult { promptsUi, resourcesUi, appsUi, + skillsUi, tasksUi, logsUi, protocolUi, @@ -112,6 +116,7 @@ export function useTabUiState(): TabUiStateResult { setPromptsUi, setResourcesUi, setAppsUi, + setSkillsUi, setTasksUi, setLogsUi, setProtocolUi, @@ -139,6 +144,7 @@ export function useTabUiState(): TabUiStateResult { setPromptsUi(EMPTY_PROMPTS_UI); setResourcesUi(EMPTY_RESOURCES_UI); setAppsUi(EMPTY_APPS_UI); + setSkillsUi(EMPTY_SKILLS_UI); setTasksUi(EMPTY_TASKS_UI); setLogsUi(EMPTY_LOGS_UI); setProtocolUi(EMPTY_PROTOCOL_UI); diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index fb8d1bd9..6977bafa 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/clients/web/src/lib/oauthResume.test.ts @@ -19,6 +19,7 @@ import { EMPTY_PROMPTS_UI, EMPTY_RESOURCES_UI, EMPTY_APPS_UI, + EMPTY_SKILLS_UI, EMPTY_TASKS_UI, EMPTY_LOGS_UI, EMPTY_PROTOCOL_UI, @@ -107,6 +108,7 @@ describe("oauthResume", () => { promptsUi: EMPTY_PROMPTS_UI, resourcesUi: EMPTY_RESOURCES_UI, appsUi: EMPTY_APPS_UI, + skillsUi: EMPTY_SKILLS_UI, tasksUi: EMPTY_TASKS_UI, logsUi: EMPTY_LOGS_UI, protocolUi: EMPTY_PROTOCOL_UI, @@ -118,6 +120,7 @@ describe("oauthResume", () => { setPromptsUi: vi.fn(), setResourcesUi: vi.fn(), setAppsUi: vi.fn(), + setSkillsUi: vi.fn(), setTasksUi: vi.fn(), setLogsUi: vi.fn(), setProtocolUi: vi.fn(), @@ -147,6 +150,7 @@ describe("oauthResume", () => { setPromptsUi: vi.fn(), setResourcesUi: vi.fn(), setAppsUi: vi.fn(), + setSkillsUi: vi.fn(), setTasksUi: vi.fn(), setLogsUi: vi.fn(), setProtocolUi: vi.fn(), @@ -244,6 +248,7 @@ describe("oauthResume", () => { setPromptsUi: vi.fn(), setResourcesUi: vi.fn(), setAppsUi: vi.fn(), + setSkillsUi: vi.fn(), setTasksUi: vi.fn(), setLogsUi: vi.fn(), setProtocolUi: vi.fn(), @@ -553,6 +558,7 @@ describe("oauthResume", () => { setPromptsUi: vi.fn(), setResourcesUi: vi.fn(), setAppsUi: vi.fn(), + setSkillsUi: vi.fn(), setTasksUi: vi.fn(), setLogsUi: vi.fn(), setProtocolUi: vi.fn(), @@ -567,6 +573,7 @@ describe("oauthResume", () => { setPromptsUi: vi.fn(), setResourcesUi: vi.fn(), setAppsUi: vi.fn(), + setSkillsUi: vi.fn(), setTasksUi: vi.fn(), setLogsUi: vi.fn(), setProtocolUi: vi.fn(), @@ -587,6 +594,7 @@ describe("oauthResume", () => { setPromptsUi: vi.fn(), setResourcesUi: vi.fn(), setAppsUi: vi.fn(), + setSkillsUi: vi.fn(), setTasksUi: vi.fn(), setLogsUi: vi.fn(), setProtocolUi: vi.fn(), @@ -621,6 +629,7 @@ describe("oauthResume", () => { setPromptsUi: vi.fn(), setResourcesUi: vi.fn(), setAppsUi: vi.fn(), + setSkillsUi: vi.fn(), setTasksUi: vi.fn(), setLogsUi: vi.fn(), setProtocolUi: vi.fn(), diff --git a/clients/web/src/lib/oauthResume.ts b/clients/web/src/lib/oauthResume.ts index ee2bebf3..14517108 100644 --- a/clients/web/src/lib/oauthResume.ts +++ b/clients/web/src/lib/oauthResume.ts @@ -11,6 +11,7 @@ import { EMPTY_NETWORK_UI, EMPTY_PROMPTS_UI, EMPTY_RESOURCES_UI, + EMPTY_SKILLS_UI, EMPTY_TASKS_UI, EMPTY_TOOLS_UI, } from "../components/screens/screenUiState.js"; @@ -20,6 +21,7 @@ import type { LogsUiState } from "../components/screens/LoggingScreen/LoggingScr import type { NetworkUiState } from "../components/screens/NetworkScreen/NetworkScreen.js"; import type { PromptsUiState } from "../components/screens/PromptsScreen/PromptsScreen.js"; import type { ResourcesUiState } from "../components/screens/ResourcesScreen/ResourcesScreen.js"; +import type { SkillsUiState } from "../components/screens/SkillsScreen/SkillsScreen.js"; import type { TasksUiState } from "../components/screens/TasksScreen/TasksScreen.js"; import type { ToolsUiState } from "../components/screens/ToolsScreen/ToolsScreen.js"; import { @@ -77,6 +79,7 @@ export interface LiftedTabUiState { promptsUi: PromptsUiState; resourcesUi: ResourcesUiState; appsUi: AppsUiState; + skillsUi: SkillsUiState; tasksUi: TasksUiState; logsUi: LogsUiState; protocolUi: ProtocolUiState; @@ -88,6 +91,7 @@ export interface TabUiSetters { setPromptsUi: (next: PromptsUiState) => void; setResourcesUi: (next: ResourcesUiState) => void; setAppsUi: (next: AppsUiState) => void; + setSkillsUi: (next: SkillsUiState) => void; setTasksUi: (next: TasksUiState) => void; setLogsUi: (next: LogsUiState) => void; setProtocolUi: (next: ProtocolUiState) => void; @@ -102,6 +106,7 @@ export function buildTabUiSnapshot( Tools: state.toolsUi, Prompts: state.promptsUi, Resources: state.resourcesUi, + Skills: state.skillsUi, Tasks: state.tasksUi, Logs: state.logsUi, Protocol: state.protocolUi, @@ -155,6 +160,11 @@ export function restoreTabUiFromSnapshot( case "Apps": setters.setAppsUi((value as AppsUiState | undefined) ?? EMPTY_APPS_UI); break; + case "Skills": + setters.setSkillsUi( + (value as SkillsUiState | undefined) ?? EMPTY_SKILLS_UI, + ); + break; case "Tasks": setters.setTasksUi( (value as TasksUiState | undefined) ?? EMPTY_TASKS_UI, diff --git a/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts new file mode 100644 index 00000000..d60b16db --- /dev/null +++ b/clients/web/src/test/core/mcp/inspectorClient-skills.test.ts @@ -0,0 +1,140 @@ +import { describe, it, expect, vi } from "vitest"; +import { InspectorClient } from "@inspector/core/mcp/inspectorClient.js"; +import type { ServerCapabilities } from "@modelcontextprotocol/client"; +import { SKILLS_EXTENSION_KEY } from "@inspector/core/mcp/skillsSchemas.js"; + +/** + * Unit coverage for the Skills extension methods (#2234, SEP-2640). + * + * The SDK client is stubbed rather than connected: what these assert is the + * shape of the outbound request and the normalization of the result, both of + * which are decided entirely in `InspectorClient` — and the point worth pinning + * is that `skills/*` go out through the ordinary `client.request` path with an + * explicit result schema, NOT through the raw-wire channel modern `tasks/*` + * needs. + */ +describe("InspectorClient skills methods (#2234)", () => { + const ENTRY = { + uri: "skill://demo/SKILL.md", + frontmatter: { name: "demo", description: "A demo skill" }, + resources: [ + { + uri: "skill://demo/ref.md", + digest: `sha256:${"a".repeat(64)}`, + size: 3, + }, + ], + }; + + interface SkillsInternals { + client: { + request: ( + req: { method: string; params: Record }, + schema: { parse: (value: unknown) => unknown }, + ) => Promise; + } | null; + capabilities: ServerCapabilities | undefined; + } + + function makeClient(): InspectorClient { + return new InspectorClient( + { type: "stdio", command: "noop", args: [] }, + // `environment.transport` is only used on connect(); these tests never + // connect, they stub the SDK client directly. + { environment: { transport: () => ({}) as never } }, + ); + } + + function internals(client: InspectorClient): SkillsInternals { + return client as unknown as SkillsInternals; + } + + /** Stub the SDK client so `request` parses through the supplied schema. */ + function stubRequest(client: InspectorClient, result: unknown) { + const request = vi.fn( + async ( + _req: { method: string; params: Record }, + schema: { parse: (value: unknown) => unknown }, + ) => schema.parse(result), + ); + internals(client).client = { request }; + return request; + } + + it("getSkillsExtension reads the server's declaration", () => { + const client = makeClient(); + expect(client.getSkillsExtension()).toBeUndefined(); + internals(client).capabilities = { + extensions: { [SKILLS_EXTENSION_KEY]: { directoryRead: true } }, + } as ServerCapabilities; + expect(client.getSkillsExtension()).toEqual({ directoryRead: true }); + }); + + it("listSkills throws when not connected", async () => { + await expect(makeClient().listSkills()).rejects.toThrow(/not connected/i); + }); + + it("getSkill throws when not connected", async () => { + await expect(makeClient().getSkill("skill://x/SKILL.md")).rejects.toThrow( + /not connected/i, + ); + }); + + it("sends skills/list with no cursor on the first page", async () => { + const client = makeClient(); + const request = stubRequest(client, { skills: [ENTRY] }); + const page = await client.listSkills(); + expect(request.mock.calls[0][0].method).toBe("skills/list"); + expect(request.mock.calls[0][0].params).not.toHaveProperty("cursor"); + expect(page.skills).toEqual([ENTRY]); + expect(page.nextCursor).toBeUndefined(); + }); + + it("forwards a cursor and returns the server's nextCursor", async () => { + const client = makeClient(); + const request = stubRequest(client, { skills: [], nextCursor: "4" }); + const page = await client.listSkills("2"); + expect(request.mock.calls[0][0].params.cursor).toBe("2"); + expect(page.nextCursor).toBe("4"); + }); + + it("stamps call metadata onto skills/list as _meta", async () => { + const client = makeClient(); + const request = stubRequest(client, { skills: [] }); + await client.listSkills(undefined, { trace: "abc" }); + expect(request.mock.calls[0][0].params._meta).toMatchObject({ + trace: "abc", + }); + }); + + it("sends skills/get with the requested uri", async () => { + const client = makeClient(); + const request = stubRequest(client, { skill: ENTRY }); + await client.getSkill("skill://demo/SKILL.md"); + expect(request.mock.calls[0][0].method).toBe("skills/get"); + expect(request.mock.calls[0][0].params.uri).toBe("skill://demo/SKILL.md"); + }); + + it("normalizes the enveloped skills/get result to the entry", async () => { + const client = makeClient(); + stubRequest(client, { skill: ENTRY }); + expect(await client.getSkill("skill://demo/SKILL.md")).toEqual(ENTRY); + }); + + it("normalizes the inline skills/get result to the entry", async () => { + // The SEP settles the entry shape but not the envelope; a server that + // returns the entry at the top level must not fail here. + const client = makeClient(); + stubRequest(client, ENTRY); + expect(await client.getSkill("skill://demo/SKILL.md")).toEqual(ENTRY); + }); + + it("rejects a skills/list result that is not a skills page", async () => { + // The explicit result schema is the whole client-side mechanism for a + // consumer-owned extension method, so a nonconforming result must fail + // here rather than reaching the UI as a half-parsed shape. + const client = makeClient(); + stubRequest(client, { notSkills: true }); + await expect(client.listSkills()).rejects.toBeDefined(); + }); +}); diff --git a/clients/web/src/test/core/mcp/skills.test.ts b/clients/web/src/test/core/mcp/skills.test.ts new file mode 100644 index 00000000..46afa98d --- /dev/null +++ b/clients/web/src/test/core/mcp/skills.test.ts @@ -0,0 +1,333 @@ +import { describe, it, expect } from "vitest"; +import type { ServerCapabilities } from "@modelcontextprotocol/client"; +import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas"; +import { SKILLS_EXTENSION_KEY } from "@inspector/core/mcp/skillsSchemas"; +import { + SKILL_MAX_RESOURCE_ENTRIES, + SKILL_MAX_TOTAL_BYTES, + base64ToBytes, + checkSkillConformance, + getSkillsExtension, + isSkillsExtensionSupported, + sha256Digest, + skillDisplayName, + skillNameFromUri, + textToBytes, + totalSkillBytes, + verifySkillResource, +} from "@inspector/core/mcp/skills"; + +/** The digest of the string "hello", precomputed so the assertion is a fact + * about SHA-256 rather than a restatement of what the code just did. */ +const HELLO_SHA256 = + "sha256:2cf24dba5fb0a30e26e83b2ac5b9e29e1b161e5c1fa7425e73043362938b9824"; + +function entry(overrides: Partial = {}): SkillEntry { + return { + uri: "skill://demo/SKILL.md", + frontmatter: { name: "demo", description: "A demo skill" }, + resources: [ + { + uri: "skill://demo/ref.md", + digest: `sha256:${"a".repeat(64)}`, + size: 10, + }, + ], + ...overrides, + }; +} + +function caps(extensions?: Record): ServerCapabilities { + return { ...(extensions ? { extensions } : {}) } as ServerCapabilities; +} + +describe("getSkillsExtension", () => { + it("returns undefined when the server declared no extensions at all", () => { + expect(getSkillsExtension(undefined)).toBeUndefined(); + expect(getSkillsExtension(caps())).toBeUndefined(); + }); + + it("returns undefined when other extensions are declared but not skills", () => { + expect( + getSkillsExtension(caps({ "io.modelcontextprotocol/tasks": {} })), + ).toBeUndefined(); + }); + + it("reports directoryRead false for a bare declaration", () => { + expect(getSkillsExtension(caps({ [SKILLS_EXTENSION_KEY]: {} }))).toEqual({ + directoryRead: false, + }); + }); + + it("reports directoryRead only for a literal true", () => { + expect( + getSkillsExtension( + caps({ [SKILLS_EXTENSION_KEY]: { directoryRead: true } }), + ), + ).toEqual({ directoryRead: true }); + // A truthy non-`true` value is a non-conforming advertisement; treating it + // as support would make the Inspector call a method the server may not + // serve, so it reads as unsupported. + expect( + getSkillsExtension( + caps({ [SKILLS_EXTENSION_KEY]: { directoryRead: "yes" } }), + ), + ).toEqual({ directoryRead: false }); + }); + + it("treats a declared-but-null value as no declaration", () => { + expect( + getSkillsExtension(caps({ [SKILLS_EXTENSION_KEY]: null })), + ).toBeUndefined(); + }); + + it("treats a non-object declaration as declared with no sub-options", () => { + expect(getSkillsExtension(caps({ [SKILLS_EXTENSION_KEY]: true }))).toEqual({ + directoryRead: false, + }); + }); + + it("isSkillsExtensionSupported mirrors presence", () => { + expect(isSkillsExtensionSupported(caps())).toBe(false); + expect( + isSkillsExtensionSupported(caps({ [SKILLS_EXTENSION_KEY]: {} })), + ).toBe(true); + }); +}); + +describe("skillNameFromUri", () => { + it("returns the segment before /SKILL.md, not the filename", () => { + expect(skillNameFromUri("skill://a/b/data-analysis/SKILL.md")).toBe( + "data-analysis", + ); + }); + + it("returns undefined for a URI that does not end in /SKILL.md", () => { + expect(skillNameFromUri("skill://demo/other.md")).toBeUndefined(); + // The suffix must include the separator: a bare "SKILL.md" has no segment. + expect(skillNameFromUri("SKILL.md")).toBeUndefined(); + }); + + it("returns undefined when the segment before the suffix is empty", () => { + expect(skillNameFromUri("skill:///SKILL.md")).toBeUndefined(); + }); +}); + +describe("skillDisplayName", () => { + it("prefers the declared frontmatter name", () => { + expect(skillDisplayName(entry())).toBe("demo"); + }); + + it("falls back to the URI segment when the name is blank", () => { + expect(skillDisplayName(entry({ frontmatter: { name: " " } }))).toBe( + "demo", + ); + }); + + it("falls back to the raw URI when neither is available", () => { + expect(skillDisplayName(entry({ uri: "skill://x", frontmatter: {} }))).toBe( + "skill://x", + ); + }); +}); + +describe("checkSkillConformance", () => { + it("reports nothing for a conforming entry", () => { + expect(checkSkillConformance(entry())).toEqual([]); + }); + + it("reports a missing name as an error", () => { + const issues = checkSkillConformance( + entry({ frontmatter: { description: "d" } }), + ); + expect(issues.map((i) => i.code)).toEqual(["missing-name"]); + expect(issues[0].severity).toBe("error"); + }); + + it("reports a missing description as a warning", () => { + const issues = checkSkillConformance( + entry({ frontmatter: { name: "demo" } }), + ); + expect(issues.map((i) => i.code)).toEqual(["missing-description"]); + expect(issues[0].severity).toBe("warning"); + }); + + it("reports a URI that does not carry a skill path", () => { + const issues = checkSkillConformance(entry({ uri: "skill://demo/x.md" })); + expect(issues.map((i) => i.code)).toContain("malformed-uri"); + // The name/path check is suppressed: there is no path segment to compare, + // and reporting both would present one defect as two. + expect(issues.map((i) => i.code)).not.toContain("name-path-mismatch"); + }); + + it("reports a path segment that disagrees with frontmatter.name", () => { + const issues = checkSkillConformance( + entry({ uri: "skill://wrong-folder/SKILL.md" }), + ); + const mismatch = issues.find((i) => i.code === "name-path-mismatch"); + expect(mismatch?.severity).toBe("error"); + expect(mismatch?.message).toContain("wrong-folder"); + expect(mismatch?.message).toContain("demo"); + }); + + it("does not report a mismatch when the name is missing entirely", () => { + // The missing name is already an error of its own; a second finding + // comparing against an absent value would be noise. + const issues = checkSkillConformance( + entry({ uri: "skill://other/SKILL.md", frontmatter: {} }), + ); + expect(issues.map((i) => i.code)).not.toContain("name-path-mismatch"); + expect(issues.map((i) => i.code)).toContain("missing-name"); + }); + + it("reports dynamic resources as a warning and checks nothing further", () => { + const issues = checkSkillConformance(entry({ resources: "dynamic" })); + expect(issues.map((i) => i.code)).toEqual(["dynamic-resources"]); + expect(issues[0].severity).toBe("warning"); + }); + + it("reports a manifest entry with no digest as unverifiable", () => { + const issues = checkSkillConformance( + entry({ resources: [{ uri: "skill://demo/ref.md", size: 1 }] }), + ); + expect(issues.map((i) => i.code)).toEqual(["missing-digest"]); + expect(issues[0].resourceUri).toBe("skill://demo/ref.md"); + }); + + it("reports a digest that is not sha256 + 64 lowercase hex", () => { + for (const digest of [ + "sha256:XYZ", + `sha256:${"A".repeat(64)}`, + `sha512:${"a".repeat(64)}`, + `sha256:${"a".repeat(63)}`, + ]) { + const issues = checkSkillConformance( + entry({ resources: [{ uri: "skill://demo/ref.md", digest }] }), + ); + expect(issues.map((i) => i.code)).toEqual(["malformed-digest"]); + } + }); + + it("reports a manifest over the 512-entry limit", () => { + const resources = Array.from( + { length: SKILL_MAX_RESOURCE_ENTRIES + 1 }, + (_unused, i) => ({ + uri: `skill://demo/f${i}.md`, + digest: `sha256:${"a".repeat(64)}`, + }), + ); + const issues = checkSkillConformance(entry({ resources })); + expect(issues.map((i) => i.code)).toContain("resource-limit-exceeded"); + }); + + it("reports a manifest over the 16 MiB limit", () => { + const issues = checkSkillConformance( + entry({ + resources: [ + { + uri: "skill://demo/big.bin", + digest: `sha256:${"a".repeat(64)}`, + size: SKILL_MAX_TOTAL_BYTES + 1, + }, + ], + }), + ); + expect(issues.map((i) => i.code)).toContain("size-limit-exceeded"); + }); + + it("does not report the size limit at exactly the boundary", () => { + const issues = checkSkillConformance( + entry({ + resources: [ + { + uri: "skill://demo/big.bin", + digest: `sha256:${"a".repeat(64)}`, + size: SKILL_MAX_TOTAL_BYTES, + }, + ], + }), + ); + expect(issues).toEqual([]); + }); +}); + +describe("totalSkillBytes", () => { + it("sums declared sizes and treats a missing size as zero", () => { + expect( + totalSkillBytes([ + { uri: "a", size: 10 }, + { uri: "b" }, + { uri: "c", size: 5 }, + ]), + ).toBe(15); + }); +}); + +describe("byte helpers", () => { + it("textToBytes produces UTF-8, not code units", () => { + // "é" is two bytes in UTF-8 and one JS code unit — the digest is over the + // former, so a naive per-char encoding would verify the wrong thing. + expect(Array.from(textToBytes("é"))).toEqual([0xc3, 0xa9]); + }); + + it("base64ToBytes decodes standard base64", () => { + expect(Array.from(base64ToBytes("aGVsbG8="))).toEqual([ + 104, 101, 108, 108, 111, + ]); + }); + + it("sha256Digest matches the known digest of 'hello'", async () => { + expect(await sha256Digest(textToBytes("hello"))).toBe(HELLO_SHA256); + }); + + it("sha256Digest hashes only the view, not the whole backing buffer", async () => { + // A Uint8Array can be a window into a larger ArrayBuffer. Hashing the + // buffer instead of the view would silently digest neighbouring bytes. + const backing = new Uint8Array([0xff, ...textToBytes("hello"), 0xff]); + const view = backing.subarray(1, 6); + expect(await sha256Digest(view)).toBe(HELLO_SHA256); + }); +}); + +describe("verifySkillResource", () => { + it("verifies matching bytes", async () => { + const result = await verifySkillResource( + { uri: "skill://demo/a.md", digest: HELLO_SHA256 }, + textToBytes("hello"), + ); + expect(result.status).toBe("verified"); + expect(result.actualDigest).toBe(HELLO_SHA256); + }); + + it("reports a mismatch with both digests instead of throwing", async () => { + const expected = `sha256:${"b".repeat(64)}`; + const result = await verifySkillResource( + { uri: "skill://demo/a.md", digest: expected }, + textToBytes("hello"), + ); + expect(result.status).toBe("mismatch"); + expect(result.expectedDigest).toBe(expected); + expect(result.actualDigest).toBe(HELLO_SHA256); + }); + + it("reports unverifiable when no digest is advertised", async () => { + const result = await verifySkillResource( + { uri: "skill://demo/a.md" }, + textToBytes("hello"), + ); + expect(result.status).toBe("unverifiable"); + expect(result.actualDigest).toBeUndefined(); + }); + + it("reports unverifiable — not a mismatch — for a malformed digest", async () => { + // A malformed digest is already a conformance finding; calling it a + // mismatch would accuse the file's bytes of being wrong when the manifest + // is what is broken. + const result = await verifySkillResource( + { uri: "skill://demo/a.md", digest: "sha256:nope" }, + textToBytes("hello"), + ); + expect(result.status).toBe("unverifiable"); + expect(result.expectedDigest).toBe("sha256:nope"); + }); +}); diff --git a/clients/web/src/test/core/mcp/skillsSchemas.test.ts b/clients/web/src/test/core/mcp/skillsSchemas.test.ts new file mode 100644 index 00000000..1a19eb2a --- /dev/null +++ b/clients/web/src/test/core/mcp/skillsSchemas.test.ts @@ -0,0 +1,121 @@ +import { describe, it, expect } from "vitest"; +import { + DYNAMIC_RESOURCES, + GetSkillResultSchema, + ListSkillsResultSchema, + ReadResourceDirectoryResultSchema, + SKILLS_EXTENSION_KEY, + SKILLS_GET_METHOD, + SKILLS_LIST_METHOD, + SkillEntrySchema, + normalizeGetSkillResult, +} from "@inspector/core/mcp/skillsSchemas"; + +const ENTRY = { + uri: "skill://demo/SKILL.md", + frontmatter: { name: "demo", description: "A demo skill" }, + resources: [ + { uri: "skill://demo/ref.md", digest: `sha256:${"a".repeat(64)}`, size: 3 }, + ], +}; + +describe("skills wire constants", () => { + it("names the extension and its two required methods", () => { + expect(SKILLS_EXTENSION_KEY).toBe("io.modelcontextprotocol/skills"); + expect(SKILLS_LIST_METHOD).toBe("skills/list"); + expect(SKILLS_GET_METHOD).toBe("skills/get"); + }); +}); + +describe("SkillEntrySchema", () => { + it("parses a full manifest entry", () => { + expect(SkillEntrySchema.parse(ENTRY)).toEqual(ENTRY); + }); + + it("parses the dynamic form", () => { + const dynamic = { ...ENTRY, resources: DYNAMIC_RESOURCES }; + expect(SkillEntrySchema.parse(dynamic).resources).toBe("dynamic"); + }); + + it("passes unknown frontmatter fields through untouched", () => { + // The skill *format* versions independently of this extension, so an + // unrecognized frontmatter field is a future Agent Skills field, not junk. + const parsed = SkillEntrySchema.parse({ + ...ENTRY, + frontmatter: { ...ENTRY.frontmatter, license: "MIT" }, + }); + expect(parsed.frontmatter.license).toBe("MIT"); + }); + + it("accepts a malformed digest rather than rejecting the entry", () => { + // Rejecting here would turn a reportable server bug into a parse failure, + // and the Inspector exists to report it. See `checkSkillConformance`. + const parsed = SkillEntrySchema.parse({ + ...ENTRY, + resources: [{ uri: "skill://demo/ref.md", digest: "nope" }], + }); + expect(parsed.resources).toEqual([ + { uri: "skill://demo/ref.md", digest: "nope" }, + ]); + }); + + it("rejects an entry with no uri", () => { + expect(() => + SkillEntrySchema.parse({ frontmatter: {}, resources: [] }), + ).toThrow(); + }); + + it("rejects a resources value that is neither a list nor 'dynamic'", () => { + expect(() => + SkillEntrySchema.parse({ ...ENTRY, resources: "static" }), + ).toThrow(); + }); +}); + +describe("ListSkillsResultSchema", () => { + it("parses a page with a cursor", () => { + const parsed = ListSkillsResultSchema.parse({ + skills: [ENTRY], + nextCursor: "2", + }); + expect(parsed.skills).toHaveLength(1); + expect(parsed.nextCursor).toBe("2"); + }); + + it("parses a final page with no cursor", () => { + expect( + ListSkillsResultSchema.parse({ skills: [] }).nextCursor, + ).toBeUndefined(); + }); +}); + +describe("GetSkillResultSchema", () => { + it("normalizes the enveloped form to the entry", () => { + expect(GetSkillResultSchema.parse({ skill: ENTRY })).toEqual(ENTRY); + }); + + it("normalizes the inline form to the entry", () => { + expect(GetSkillResultSchema.parse(ENTRY)).toEqual(ENTRY); + }); + + it("normalizeGetSkillResult accepts either shape directly", () => { + expect(normalizeGetSkillResult({ skill: ENTRY })).toEqual(ENTRY); + expect(normalizeGetSkillResult(ENTRY)).toEqual(ENTRY); + }); + + it("rejects a result that is neither shape", () => { + expect(() => GetSkillResultSchema.parse({ nothing: true })).toThrow(); + }); +}); + +describe("ReadResourceDirectoryResultSchema", () => { + it("parses directory children including the directory mime type", () => { + const parsed = ReadResourceDirectoryResultSchema.parse({ + contents: [ + { uri: "skill://demo/sub", mimeType: "inode/directory" }, + { uri: "skill://demo/ref.md", mimeType: "text/markdown", size: 3 }, + ], + }); + expect(parsed.contents).toHaveLength(2); + }); +}); diff --git a/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts b/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts new file mode 100644 index 00000000..79f00351 --- /dev/null +++ b/clients/web/src/test/core/mcp/state/managedSkillsState.test.ts @@ -0,0 +1,178 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas"; +import { + ManagedSkillsState, + REPEATED_CURSOR_MESSAGE, +} from "@inspector/core/mcp/state/managedSkillsState"; +import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient"; + +function skill(name: string): SkillEntry { + return { + uri: `skill://${name}/SKILL.md`, + frontmatter: { name, description: `${name} skill` }, + resources: [], + }; +} + +function waitFor( + state: ManagedSkillsState, + event: "skillsChange" | "errorChange" | "paginationChange", +): Promise { + return new Promise((resolve) => { + state.addEventListener( + event, + // The union of detail types across the three events is wider than any one + // caller wants, so the resolve is typed at the call site. + (e) => resolve(e.detail as T), + { once: true }, + ); + }); +} + +describe("ManagedSkillsState", () => { + let client: FakeInspectorClient; + let state: ManagedSkillsState; + + beforeEach(() => { + client = new FakeInspectorClient(); + client.skillsExtension = { directoryRead: false }; + state = new ManagedSkillsState(client); + }); + + it("starts empty and returns defensive copies", () => { + expect(state.getSkills()).toEqual([]); + expect(state.getSkills()).not.toBe(state.getSkills()); + expect(state.getPagination()).toEqual({ pageCount: 0 }); + expect(state.getError()).toBeNull(); + }); + + it("refresh no-ops while disconnected", async () => { + await state.refresh(); + expect(client.listSkills).not.toHaveBeenCalled(); + }); + + it("returns an empty list without calling the server when the extension is absent", async () => { + // Calling `skills/list` against a server that never declared the extension + // gets -32601 and spams the console for a question already answered. + client.setStatus("connected"); + client.skillsExtension = undefined; + await state.refresh(); + expect(client.listSkills).not.toHaveBeenCalled(); + expect(state.getSkills()).toEqual([]); + }); + + it("walks every page and reports how many it took", async () => { + client.setStatus("connected"); + client.skillPages = [ + { skills: [skill("a"), skill("b")], nextCursor: "2" }, + { skills: [skill("c")], nextCursor: undefined }, + ]; + const skills = await state.refresh(); + expect(skills.map((s) => s.frontmatter.name)).toEqual(["a", "b", "c"]); + expect(state.getPagination()).toEqual({ pageCount: 2 }); + expect(client.listSkills).toHaveBeenCalledTimes(2); + }); + + it("dispatches skillsChange and paginationChange on a successful walk", async () => { + client.setStatus("connected"); + client.skillPages = [{ skills: [skill("a")] }]; + const skillsEvent = waitFor(state, "skillsChange"); + const paginationEvent = waitFor<{ pageCount: number }>( + state, + "paginationChange", + ); + await state.refresh(); + expect(await skillsEvent).toHaveLength(1); + expect(await paginationEvent).toEqual({ pageCount: 1 }); + }); + + it("loads on connect", async () => { + client.skillPages = [{ skills: [skill("a")] }]; + const skillsEvent = waitFor(state, "skillsChange"); + await client.connect(); + expect(await skillsEvent).toHaveLength(1); + }); + + it("stops and reports when the server repeats a cursor", async () => { + client.setStatus("connected"); + // A server stuck on one cursor would otherwise walk forever, so the guard + // is what keeps a server bug from becoming a hang. + client.listSkills.mockResolvedValue({ + skills: [skill("a")], + nextCursor: "same", + }); + await expect(state.refresh()).rejects.toThrow(REPEATED_CURSOR_MESSAGE); + expect(state.getError()?.message).toBe(REPEATED_CURSOR_MESSAGE); + }); + + it("records a failure as observable state and re-throws it", async () => { + client.setStatus("connected"); + const failure = new Error("boom"); + client.listSkills.mockRejectedValueOnce(failure); + const errorEvent = waitFor(state, "errorChange"); + await expect(state.refresh()).rejects.toThrow("boom"); + expect(await errorEvent).toBe(failure); + expect(state.getError()).toBe(failure); + }); + + it("wraps a non-Error rejection", async () => { + client.setStatus("connected"); + client.listSkills.mockRejectedValueOnce("just a string"); + await expect(state.refresh()).rejects.toBeDefined(); + expect(state.getError()?.message).toBe("just a string"); + }); + + it("clears the error once a later walk succeeds", async () => { + client.setStatus("connected"); + client.listSkills.mockRejectedValueOnce(new Error("boom")); + await expect(state.refresh()).rejects.toThrow(); + client.skillPages = [{ skills: [skill("a")] }]; + await state.refresh(); + expect(state.getError()).toBeNull(); + }); + + it("swallows the connect-time load's rejection rather than leaking it", async () => { + // Nobody awaits the connect-time load, so an unhandled rejection would + // fail an unrelated test file. The failure still lands on `getError`. + client.listSkills.mockRejectedValueOnce(new Error("connect boom")); + const errorEvent = waitFor(state, "errorChange"); + await client.connect(); + expect((await errorEvent)?.message).toBe("connect boom"); + }); + + it("makes a second refresh a no-op while one is in flight", async () => { + client.setStatus("connected"); + let release: (() => void) | undefined; + client.listSkills.mockImplementationOnce( + async () => + new Promise((resolve) => { + release = () => resolve({ skills: [skill("a")] }); + }), + ); + const first = state.refresh(); + const second = await state.refresh(); + expect(second).toEqual([]); + expect(client.listSkills).toHaveBeenCalledTimes(1); + release?.(); + await first; + expect(state.getSkills()).toHaveLength(1); + }); + + it("clears the list and the error on disconnect", async () => { + client.setStatus("connected"); + client.skillPages = [{ skills: [skill("a")] }]; + await state.refresh(); + await client.disconnect(); + expect(state.getSkills()).toEqual([]); + expect(state.getPagination()).toEqual({ pageCount: 0 }); + expect(state.getError()).toBeNull(); + }); + + it("destroy unsubscribes so a later connect does not refetch", async () => { + state.destroy(); + await client.connect(); + expect(client.listSkills).not.toHaveBeenCalled(); + // Idempotent. + state.destroy(); + }); +}); diff --git a/clients/web/src/test/core/react/useManagedSkills.test.tsx b/clients/web/src/test/core/react/useManagedSkills.test.tsx new file mode 100644 index 00000000..a9c744e4 --- /dev/null +++ b/clients/web/src/test/core/react/useManagedSkills.test.tsx @@ -0,0 +1,114 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import type { SkillEntry } from "@inspector/core/mcp/skillsSchemas"; +import { FakeInspectorClient } from "@inspector/core/mcp/__tests__/fakeInspectorClient"; +import { ManagedSkillsState } from "@inspector/core/mcp/state/managedSkillsState"; +import { useManagedSkills } from "@inspector/core/react/useManagedSkills"; + +function skill(name: string): SkillEntry { + return { + uri: `skill://${name}/SKILL.md`, + frontmatter: { name, description: `${name} skill` }, + resources: [], + }; +} + +describe("useManagedSkills", () => { + let client: FakeInspectorClient; + let state: ManagedSkillsState; + + beforeEach(() => { + client = new FakeInspectorClient({ status: "connected" }); + client.skillsExtension = { directoryRead: false }; + state = new ManagedSkillsState(client); + }); + + it("reports the store's current snapshot on first render", async () => { + client.skillPages = [{ skills: [skill("a"), skill("b")] }]; + await state.refresh(); + + const { result } = renderHook(() => useManagedSkills(client, state)); + expect(result.current.skills.map((s) => s.frontmatter.name)).toEqual([ + "a", + "b", + ]); + expect(result.current.pageCount).toBe(1); + expect(result.current.error).toBeNull(); + }); + + it("degrades to empty values when no store is attached", async () => { + const { result } = renderHook(() => useManagedSkills(client, null)); + expect(result.current.skills).toEqual([]); + expect(result.current.pageCount).toBe(0); + expect(result.current.error).toBeNull(); + // The refresh is still callable and simply resolves to the empty list. + await expect(result.current.refresh()).resolves.toEqual([]); + }); + + it("updates when the store dispatches", async () => { + const { result } = renderHook(() => useManagedSkills(client, state)); + expect(result.current.skills).toEqual([]); + + client.skillPages = [ + { skills: [skill("a")], nextCursor: "1" }, + { skills: [skill("b")] }, + ]; + await act(async () => { + await state.refresh(); + }); + expect(result.current.skills).toHaveLength(2); + expect(result.current.pageCount).toBe(2); + }); + + it("holds the snapshot identity stable across renders with no dispatch", async () => { + client.skillPages = [{ skills: [skill("a")] }]; + await state.refresh(); + const { result, rerender } = renderHook(() => + useManagedSkills(client, state), + ); + const first = result.current.skills; + rerender(); + // `getSkills()` returns a fresh copy per call, so an uncached snapshot + // would hand back a new array every render and defeat every downstream memo. + expect(result.current.skills).toBe(first); + }); + + it("surfaces the store's error", async () => { + client.listSkills.mockRejectedValueOnce(new Error("boom")); + const { result } = renderHook(() => useManagedSkills(client, state)); + await act(async () => { + await state.refresh().catch(() => {}); + }); + expect(result.current.error?.message).toBe("boom"); + }); + + it("refresh drives the store", async () => { + const { result } = renderHook(() => useManagedSkills(client, state)); + client.skillPages = [{ skills: [skill("a")] }]; + await act(async () => { + await result.current.refresh(); + }); + expect(result.current.skills).toHaveLength(1); + }); + + it("swaps to another server's store in the same render", async () => { + const other = new FakeInspectorClient({ status: "connected" }); + other.skillsExtension = { directoryRead: false }; + const otherState = new ManagedSkillsState(other); + other.skillPages = [{ skills: [skill("z")] }]; + await otherState.refresh(); + + client.skillPages = [{ skills: [skill("a")] }]; + await state.refresh(); + + const { result, rerender } = renderHook( + ({ s }: { s: ManagedSkillsState }) => useManagedSkills(client, s), + { initialProps: { s: state } }, + ); + expect(result.current.skills[0].frontmatter.name).toBe("a"); + rerender({ s: otherState }); + // Read during render, so the swap lands in the same frame — no frame of + // the previous server's skills. + expect(result.current.skills[0].frontmatter.name).toBe("z"); + }); +}); diff --git a/clients/web/src/utils/inspectorTabs.test.ts b/clients/web/src/utils/inspectorTabs.test.ts index db6ab064..181b088e 100644 --- a/clients/web/src/utils/inspectorTabs.test.ts +++ b/clients/web/src/utils/inspectorTabs.test.ts @@ -17,6 +17,7 @@ describe("inspectorTabs", () => { "Tools", "Prompts", "Resources", + "Skills", "Tasks", "Logs", "Protocol", diff --git a/clients/web/src/utils/inspectorTabs.ts b/clients/web/src/utils/inspectorTabs.ts index be69af73..a9d69afe 100644 --- a/clients/web/src/utils/inspectorTabs.ts +++ b/clients/web/src/utils/inspectorTabs.ts @@ -11,6 +11,7 @@ export const INSPECTOR_TAB_IDS = [ "Tools", "Prompts", "Resources", + "Skills", "Tasks", "Logs", "Protocol", diff --git a/clients/web/src/utils/skillFileBytes.test.ts b/clients/web/src/utils/skillFileBytes.test.ts new file mode 100644 index 00000000..be27497d --- /dev/null +++ b/clients/web/src/utils/skillFileBytes.test.ts @@ -0,0 +1,37 @@ +import { describe, it, expect } from "vitest"; +import { skillFileBytes } from "./skillFileBytes"; + +describe("skillFileBytes", () => { + it("encodes a text content block as UTF-8", () => { + expect(Array.from(skillFileBytes({ text: "hé" }))).toEqual([ + 0x68, 0xc3, 0xa9, + ]); + }); + + it("decodes a blob content block from base64", () => { + expect(Array.from(skillFileBytes({ blob: "aGVsbG8=" }))).toEqual([ + 104, 101, 108, 108, 111, + ]); + }); + + it("prefers text when a server sends both", () => { + expect(Array.from(skillFileBytes({ text: "a", blob: "Yg==" }))).toEqual([ + 97, + ]); + }); + + it("treats an empty string as content, not as absence", () => { + // A zero-byte file is legal and has a perfectly good digest; falling + // through to the throw here would report a real file as unreadable. + expect(skillFileBytes({ text: "" })).toHaveLength(0); + }); + + it("throws for a response carrying neither field", () => { + // Not an empty array: an empty array hashes to the digest of nothing, so a + // silent fallback would report a confident digest *mismatch* rather than + // the truth, which is that the server returned no content. + expect(() => skillFileBytes({ mimeType: "text/markdown" })).toThrow( + /neither text nor blob/, + ); + }); +}); diff --git a/clients/web/src/utils/skillFileBytes.ts b/clients/web/src/utils/skillFileBytes.ts new file mode 100644 index 00000000..a1fd0e14 --- /dev/null +++ b/clients/web/src/utils/skillFileBytes.ts @@ -0,0 +1,35 @@ +/** + * Decoding a `resources/read` payload back to the bytes its digest was taken + * over (SEP-2640, #2234). + * + * A pure transform with no I/O and no subsystem ownership, so it belongs in + * `utils/` rather than `lib/` — the screen that verifies a skill file does the + * fetching; this only turns what came back into bytes. + */ + +import { base64ToBytes, textToBytes } from "@inspector/core/mcp/skills.js"; + +/** + * The content a `resources/read` returned for one skill file. Either `text` (a + * `TextResourceContents`) or `blob` (base64, a `BlobResourceContents`). + */ +export interface SkillFileContents { + text?: string; + blob?: string; + mimeType?: string; +} + +/** + * The raw bytes of a skill file, as fetched. + * + * Throws for a result carrying neither `text` nor `blob`. That is a server bug, + * and it must not be quietly treated as empty content: an empty `Uint8Array` + * has a perfectly good SHA-256, so a silent fallback would report a *digest + * mismatch* — a confident, wrong diagnosis — instead of "this response carried + * no content at all". Callers surface the throw as a per-file read failure. + */ +export function skillFileBytes(contents: SkillFileContents): Uint8Array { + if (typeof contents.text === "string") return textToBytes(contents.text); + if (typeof contents.blob === "string") return base64ToBytes(contents.blob); + throw new Error("resources/read returned neither text nor blob content."); +} diff --git a/core/mcp/__tests__/fakeInspectorClient.ts b/core/mcp/__tests__/fakeInspectorClient.ts index 8e3519e9..49232ad6 100644 --- a/core/mcp/__tests__/fakeInspectorClient.ts +++ b/core/mcp/__tests__/fakeInspectorClient.ts @@ -40,6 +40,8 @@ import type { } from "../types.js"; import { INACTIVE_SUBSCRIPTION_STREAM_STATE } from "../types.js"; import type { MalformedListItem } from "../listSalvage.js"; +import type { SkillEntry, SkillResource } from "../skillsSchemas.js"; +import type { SkillsExtensionSupport } from "../skills.js"; import type { JsonValue } from "../../json/jsonUtils.js"; type ListResult = { @@ -104,6 +106,7 @@ export class FakeInspectorClient ListResult<"resourceTemplates", ResourceTemplate> > = []; taskPages: Array> = []; + skillPages: Array> = []; listTools = vi.fn(async () => this.toolPages.shift() ?? { tools: [] }); listPrompts = vi.fn(async () => this.promptPages.shift() ?? { prompts: [] }); @@ -116,6 +119,13 @@ export class FakeInspectorClient listRequestorTasks = vi.fn( async () => this.taskPages.shift() ?? { tasks: [] }, ); + listSkills = vi.fn(async () => this.skillPages.shift() ?? { skills: [] }); + // `skills/get` echoes a minimal entry; tests that care override the mock. + getSkill = vi.fn(async (uri: string) => ({ + uri, + frontmatter: {}, + resources: [] as SkillResource[], + })); // Modern task poll (#1631): defaults to echoing back a minimal task; tests // override the mock to drive status transitions. Dispatches nothing by // default — tests that exercise the merge path dispatch requestorTaskUpdated @@ -135,6 +145,14 @@ export class FakeInspectorClient return this.tasksExtensionNegotiated; } + // The Skills extension (SEP-2640) this fake presents. `undefined` means the + // server declared none, which is what `getSkillsExtension` returns then — + // tests assign a support object to exercise the skills paths. + skillsExtension: SkillsExtensionSupport | undefined = undefined; + getSkillsExtension(): SkillsExtensionSupport | undefined { + return this.skillsExtension; + } + // Attributes a failed load back to its Protocol entry (#1953). A `vi.fn` so // tests can assert the method name and reason a failing refresh reported. markResponseRejected = vi.fn((_method: string, _reason: string) => {}); diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 8fdaebe9..763beac5 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -144,6 +144,14 @@ import { type ModernDetailedTask, } from "./modernTaskSchemas.js"; import { buildClientExtensions } from "./extensions.js"; +import { + GetSkillResultSchema, + ListSkillsResultSchema, + SKILLS_GET_METHOD, + SKILLS_LIST_METHOD, + type SkillEntry, +} from "./skillsSchemas.js"; +import { getSkillsExtension, type SkillsExtensionSupport } from "./skills.js"; import { getElicitationUiResourceUri, isFormElicitation, @@ -5516,6 +5524,81 @@ export class InspectorClient extends InspectorClientEventTarget { return { prompts }; } + /** + * The Skills extension (SEP-2640) the server declared, or `undefined` when it + * declared none. Gates the Skills tab and the skills store the same way + * {@link isTasksExtensionNegotiated} gates Tasks — but deliberately without an + * era check: `skills/*` are not spec method names in either codec, so a + * legacy-era server that declares the extension is serving it (#2234). + */ + getSkillsExtension(): SkillsExtensionSupport | undefined { + return getSkillsExtension(this.capabilities); + } + + /** + * One page of `skills/list` (SEP-2640). + * + * An ordinary `client.request` with an explicit result schema — the SDK's era + * gate skips methods neither codec defines, and `assertCapabilityForMethod` + * falls through to a no-op for them, so this is all the extension needs. The + * raw-wire path modern `tasks/*` uses would be wrong here: it exists for spec + * names the 2026 codec deleted, and taking it would bypass the SDK's response + * correlation for nothing (#2234). + */ + async listSkills( + cursor?: string, + metadata?: RequestMetadata, + ): Promise<{ skills: SkillEntry[]; nextCursor?: string }> { + if (!this.client) { + throw new Error("Client is not connected"); + } + const effectiveMeta = this.mergeMeta(metadata); + const params: Record = { + ...(effectiveMeta ? { _meta: effectiveMeta } : {}), + ...(cursor ? { cursor } : {}), + }; + const response = await this.invokeMcpClient( + () => + this.client!.request( + { method: SKILLS_LIST_METHOD, params }, + ListSkillsResultSchema, + this.getRequestOptions(this.progressTokenOf(metadata)), + ), + { method: SKILLS_LIST_METHOD }, + ); + return { + skills: response.skills, + nextCursor: response.nextCursor, + }; + } + + /** + * One skill entry by URI (`skills/get`, SEP-2640). The SEP settles the entry + * shape but not the envelope around it, so the result is normalized through + * {@link normalizeGetSkillResult} rather than assuming one form. + */ + async getSkill(uri: string, metadata?: RequestMetadata): Promise { + if (!this.client) { + throw new Error("Client is not connected"); + } + const effectiveMeta = this.mergeMeta(metadata); + const params: Record = { + uri, + ...(effectiveMeta ? { _meta: effectiveMeta } : {}), + }; + // `GetSkillResultSchema` normalizes both accepted envelopes to the entry, + // so there is nothing to unwrap here. + return this.invokeMcpClient( + () => + this.client!.request( + { method: SKILLS_GET_METHOD, params }, + GetSkillResultSchema, + this.getRequestOptions(this.progressTokenOf(metadata)), + ), + { method: SKILLS_GET_METHOD }, + ); + } + /** * Get a prompt by name * @param name Prompt name diff --git a/core/mcp/inspectorClientProtocol.ts b/core/mcp/inspectorClientProtocol.ts index ef290f9e..675bf677 100644 --- a/core/mcp/inspectorClientProtocol.ts +++ b/core/mcp/inspectorClientProtocol.ts @@ -40,6 +40,8 @@ import type { import type { JsonValue } from "../json/jsonUtils.js"; import type { MalformedListItem } from "./listSalvage.js"; import type { InspectorClientEventTarget } from "./inspectorClientEventTarget.js"; +import type { SkillEntry } from "./skillsSchemas.js"; +import type { SkillsExtensionSupport } from "./skills.js"; import type { SamplingCreateMessage } from "./samplingCreateMessage.js"; import type { ElicitationCreateMessage } from "./elicitationCreateMessage.js"; @@ -110,6 +112,17 @@ export interface InspectorClientProtocol extends InspectorClientEventTarget { * and the modern task store's poll-based refresh. */ isTasksExtensionNegotiated(): boolean; + /** The Skills extension (SEP-2640) the server declared, or `undefined`. + * Gates the Skills tab and the managed skills store (#2234). */ + getSkillsExtension(): SkillsExtensionSupport | undefined; + /** One page of `skills/list`; the managed skills store walks the cursor. */ + listSkills( + cursor?: string, + metadata?: RequestMetadata, + ): Promise<{ skills: SkillEntry[]; nextCursor?: string }>; + /** One skill entry by URI (`skills/get`). */ + getSkill(uri: string, metadata?: RequestMetadata): Promise; + /** * Mark the response that most recently answered `method` as rejected by the * client, so its Protocol entry shows the reason instead of rendering as a diff --git a/core/mcp/skills.ts b/core/mcp/skills.ts new file mode 100644 index 00000000..eab9770d --- /dev/null +++ b/core/mcp/skills.ts @@ -0,0 +1,336 @@ +/** + * Skills extension (SEP-2640) detection, conformance checking, and digest + * verification — the part of the extension that makes the Inspector more than a + * viewer. + * + * SEP-2640 puts real obligations on whoever consumes a skill: verify each + * fetched file against the digest its manifest advertised, treat a mismatch as + * a failure, and honor the per-skill limits. Every one of those is a check a + * server author wants run against their implementation, which is the same + * argument the CLI's `--strict` tool-schema lint makes. So the checks live here, + * shared by every client, and produce a structured finding list rather than a + * boolean — a report is useful, "invalid" is not. + * + * ⚠️ Skills is a **server-declared** extension, read off the connecting server's + * `capabilities.extensions`. It deliberately does NOT belong in + * `ADVERTISABLE_EXTENSIONS` (`core/mcp/extensions.ts`), which is the catalog of + * extensions the *Inspector* advertises and the user toggles in Server Settings. + * The precedent is `appElicitation.ts`, which reads the server side the same + * way; getting it backwards would put a meaningless toggle in Server Settings. + * + * The Inspector is an inspector, not a host: a `resources/read` of a `SKILL.md` + * is explicitly not a load and confers no standing, so none of the SEP's host + * machinery (activation, per-skill consent, content-bound approval) is + * implemented here. Surface and verify. + */ + +import type { ServerCapabilities } from "@modelcontextprotocol/client"; +import { + DYNAMIC_RESOURCES, + SKILLS_EXTENSION_KEY, + type SkillEntry, + type SkillResource, +} from "./skillsSchemas.js"; + +/** Maximum resource entries a single skill may declare (SEP-2640). */ +export const SKILL_MAX_RESOURCE_ENTRIES = 512; + +/** Maximum total size, in bytes, of a single skill's resources (16 MiB). */ +export const SKILL_MAX_TOTAL_BYTES = 16 * 1024 * 1024; + +/** The suffix every skill URI ends with; the segment before it is the name. */ +export const SKILL_FILE_SUFFIX = "/SKILL.md"; + +/** `sha256:` followed by exactly 64 lowercase hex characters. */ +const DIGEST_PATTERN = /^sha256:[0-9a-f]{64}$/; + +/** + * What the server declared under `io.modelcontextprotocol/skills`. The only + * sub-option SEP-2640 defines is `directoryRead`, which gates + * `resources/directory/read`. + */ +export interface SkillsExtensionSupport { + /** True when the server declared `directoryRead: true`. */ + directoryRead: boolean; +} + +/** + * Read the Skills extension off a server's advertised capabilities, or + * `undefined` when the server did not declare it. + * + * Not era-gated, unlike `isTasksExtensionNegotiated()`: `skills/*` are not spec + * method names in either codec, so nothing about the negotiated era changes + * whether the extension can be served or called. A legacy-era server that + * declares it is serving it. + */ +export function getSkillsExtension( + capabilities: ServerCapabilities | undefined, +): SkillsExtensionSupport | undefined { + const declared = capabilities?.extensions?.[SKILLS_EXTENSION_KEY]; + if (declared === undefined || declared === null) return undefined; + const directoryRead = + typeof declared === "object" && + (declared as { directoryRead?: unknown }).directoryRead === true; + return { directoryRead }; +} + +/** True when the connected server declared the Skills extension. */ +export function isSkillsExtensionSupported( + capabilities: ServerCapabilities | undefined, +): boolean { + return getSkillsExtension(capabilities) !== undefined; +} + +/** + * The final `` segment of a skill URI — the segment *before* + * `/SKILL.md`, not the filename. SEP-2640 requires it to equal + * `frontmatter.name`, which is what makes a skill's name recoverable from its + * URI alone. Returns `undefined` when the URI does not have that shape, which + * is itself a conformance finding. + */ +export function skillNameFromUri(uri: string): string | undefined { + if (!uri.endsWith(SKILL_FILE_SUFFIX)) return undefined; + const path = uri.slice(0, -SKILL_FILE_SUFFIX.length); + const segment = path.slice(path.lastIndexOf("/") + 1); + return segment.length > 0 ? segment : undefined; +} + +/** + * The label a UI shows for a skill: the declared name, falling back to the URI + * path segment, falling back to the raw URI. Never empty, so a list row is + * always addressable even for a badly non-conforming entry. + */ +export function skillDisplayName(entry: SkillEntry): string { + const declared = entry.frontmatter.name?.trim(); + if (declared) return declared; + return skillNameFromUri(entry.uri) ?? entry.uri; +} + +/** Machine-readable identity of a conformance finding. */ +export type SkillIssueCode = + | "dynamic-resources" + | "missing-name" + | "missing-description" + | "malformed-uri" + | "name-path-mismatch" + | "missing-digest" + | "malformed-digest" + | "resource-limit-exceeded" + | "size-limit-exceeded"; + +/** + * `error` marks a stated requirement of SEP-2640 that the server broke. + * `warning` marks something that is legal but leaves the Inspector unable to + * verify integrity — `"dynamic"` resources above all, which is the case most + * worth surfacing and the one most easily buried. + */ +export type SkillIssueSeverity = "error" | "warning"; + +export interface SkillIssue { + code: SkillIssueCode; + severity: SkillIssueSeverity; + /** Human-readable statement of what is wrong. */ + message: string; + /** The manifest entry the finding is about, when it is a per-file finding. */ + resourceUri?: string; +} + +/** + * Run every structural check SEP-2640 states against one skill entry, returning + * the findings in a stable order (skill-level first, then per-resource in + * manifest order). An empty array means the entry conforms. + * + * This is the static half. Digest *verification* needs the file's bytes and so + * lives in {@link verifySkillResource}, which the UI runs on demand. + */ +export function checkSkillConformance(entry: SkillEntry): SkillIssue[] { + const issues: SkillIssue[] = []; + const declaredName = entry.frontmatter.name?.trim(); + const uriName = skillNameFromUri(entry.uri); + + if (!declaredName) { + issues.push({ + code: "missing-name", + severity: "error", + message: "frontmatter.name is required but missing or empty.", + }); + } + if (!entry.frontmatter.description?.trim()) { + issues.push({ + code: "missing-description", + severity: "warning", + message: "frontmatter.description is missing or empty.", + }); + } + if (uriName === undefined) { + issues.push({ + code: "malformed-uri", + severity: "error", + message: `Skill URI must end with "${SKILL_FILE_SUFFIX}" and carry a non-empty path segment before it.`, + }); + } else if (declaredName && uriName !== declaredName) { + // The one structural invariant the spec states outright: the segment before + // /SKILL.md must equal frontmatter.name, so the name is recoverable from + // the URI alone. Only checked when both halves exist — a missing name is + // already reported above, and reporting it twice reads as two defects. + issues.push({ + code: "name-path-mismatch", + severity: "error", + message: `URI path segment "${uriName}" does not match frontmatter.name "${declaredName}".`, + }); + } + + if (entry.resources === DYNAMIC_RESOURCES) { + issues.push({ + code: "dynamic-resources", + severity: "warning", + message: + 'resources is "dynamic": the file set is generated, so no digest is advertised and integrity cannot be verified.', + }); + return issues; + } + + if (entry.resources.length > SKILL_MAX_RESOURCE_ENTRIES) { + issues.push({ + code: "resource-limit-exceeded", + severity: "error", + message: `Skill declares ${entry.resources.length} resource entries, above the ${SKILL_MAX_RESOURCE_ENTRIES}-entry limit.`, + }); + } + const totalBytes = totalSkillBytes(entry.resources); + if (totalBytes > SKILL_MAX_TOTAL_BYTES) { + issues.push({ + code: "size-limit-exceeded", + severity: "error", + message: `Skill resources total ${totalBytes} bytes, above the ${SKILL_MAX_TOTAL_BYTES}-byte (16 MiB) limit.`, + }); + } + + for (const resource of entry.resources) { + if (resource.digest === undefined) { + issues.push({ + code: "missing-digest", + severity: "warning", + message: "Manifest entry declares no digest, so it cannot be verified.", + resourceUri: resource.uri, + }); + } else if (!DIGEST_PATTERN.test(resource.digest)) { + issues.push({ + code: "malformed-digest", + severity: "error", + message: `Digest "${resource.digest}" is not "sha256:" followed by 64 lowercase hex characters.`, + resourceUri: resource.uri, + }); + } + } + + return issues; +} + +/** + * Sum of the manifest's declared `size` fields. An entry that omits `size` + * contributes nothing rather than failing the sum — the limit check is about + * catching a server that is demonstrably over, and an incomplete manifest can + * only ever understate the total, so this never produces a false positive. + */ +export function totalSkillBytes(resources: readonly SkillResource[]): number { + return resources.reduce((sum, r) => sum + (r.size ?? 0), 0); +} + +/** Outcome of comparing a fetched file against its advertised digest. */ +export type SkillVerificationStatus = + | "verified" + | "mismatch" + | "unverifiable" + | "error"; + +export interface SkillVerification { + status: SkillVerificationStatus; + /** The digest computed over the fetched bytes, when one was computed. */ + actualDigest?: string; + /** The manifest's digest, echoed so a mismatch renders both halves. */ + expectedDigest?: string; + /** Why the file could not be verified or fetched. */ + reason?: string; +} + +/** Lowercase hex of a byte array — the form SEP-2640 digests are written in. */ +function toHex(bytes: Uint8Array): string { + let out = ""; + for (const byte of bytes) out += byte.toString(16).padStart(2, "0"); + return out; +} + +/** + * `sha256:<64 hex>` over the given bytes, in the exact form a manifest digest + * takes, so a caller can compare strings rather than re-deriving the prefix. + * + * Uses WebCrypto (`crypto.subtle`), which both Node ≥22 and the browser provide + * — no dependency, and per [Dependency placement] this module adds nothing to + * any manifest. The Inspector's web client is served over localhost, a secure + * context, so `subtle` is present there too. + */ +export async function sha256Digest(bytes: Uint8Array): Promise { + // `BufferSource` wants a plain ArrayBuffer; a Uint8Array over a SharedArrayBuffer + // (or a view into a larger buffer) would otherwise hash the wrong range. + const buffer = bytes.buffer.slice( + bytes.byteOffset, + bytes.byteOffset + bytes.byteLength, + ) as ArrayBuffer; + const hash = await crypto.subtle.digest("SHA-256", buffer); + return `sha256:${toHex(new Uint8Array(hash))}`; +} + +/** UTF-8 bytes of a `resources/read` text content block. */ +export function textToBytes(text: string): Uint8Array { + return new TextEncoder().encode(text); +} + +/** + * Raw bytes of a `resources/read` blob content block (standard base64). + * Uses `atob`, which Node ≥22 and every browser provide, so this stays + * dependency-free and works unchanged in both. + */ +export function base64ToBytes(blob: string): Uint8Array { + const binary = atob(blob); + const bytes = new Uint8Array(binary.length); + for (let i = 0; i < binary.length; i += 1) bytes[i] = binary.charCodeAt(i); + return bytes; +} + +/** + * Verify one fetched skill file against its manifest entry. + * + * A mismatch is reported as `"mismatch"` with both digests attached rather than + * thrown — the whole value proposition is showing a digest mismatch loudly, and + * a thrown error would collapse into whatever the caller's generic failure UI + * says. `"unverifiable"` means the manifest advertised no digest (or advertised + * a malformed one, already reported by {@link checkSkillConformance}); nothing + * about the file itself is wrong, we simply have nothing to compare against. + */ +export async function verifySkillResource( + resource: SkillResource, + bytes: Uint8Array, +): Promise { + const expectedDigest = resource.digest; + if (expectedDigest === undefined) { + return { + status: "unverifiable", + reason: "The manifest entry advertises no digest.", + }; + } + if (!DIGEST_PATTERN.test(expectedDigest)) { + return { + status: "unverifiable", + expectedDigest, + reason: + 'The advertised digest is not "sha256:" followed by 64 lowercase hex characters.', + }; + } + const actualDigest = await sha256Digest(bytes); + return { + status: actualDigest === expectedDigest ? "verified" : "mismatch", + actualDigest, + expectedDigest, + }; +} diff --git a/core/mcp/skillsSchemas.ts b/core/mcp/skillsSchemas.ts new file mode 100644 index 00000000..8ee49cbc --- /dev/null +++ b/core/mcp/skillsSchemas.ts @@ -0,0 +1,164 @@ +/** + * Skills extension wire schemas — SEP-2640 (`io.modelcontextprotocol/skills`). + * + * There is no `@modelcontextprotocol/ext-skills` package (404 on npm) and the + * SDK's era codecs define none of these methods, so the Inspector drives them + * as ordinary `client.request(…, ResultSchema)` calls with the explicit result + * schemas below. That is exactly what the SDK prescribes for a consumer-owned + * extension method: `Protocol._assertOutboundRequestInEra` only fires for names + * one of the era codecs knows, so `skills/list` and `skills/get` are era-blind + * and go out unchanged on both the 2025- and 2026-era legs. The raw-wire escape + * hatch that modern `tasks/*` needs is deliberately NOT used here — `tasks/*` + * are spec names the 2026 codec deleted, which is a different problem. + * + * **This module is the whole wire surface.** SEP-2640 is Accepted, so the method + * names and the entry shape are settled, but the skill *format* is delegated to + * the independently-versioned Agent Skills specification and the SEP leaves the + * `skills/get` caching attributes (SEP-2549 `ttlMs` / `cacheScope`) open. Keeping + * every wire type here makes a spec revision a single-file edit (#2234). + * + * Schemas are deliberately permissive (`looseObject`, and a `digest` typed as a + * plain string rather than a hex-constrained one) so a non-conforming server is + * *surfaced* rather than rejected — the Inspector is a conformance tool, and a + * malformed digest is a finding to report, not a parse error to swallow. The + * structural checks live in `skills.ts`. + */ + +import { z } from "zod/v4"; + +/** SEP-2133 extension identifier for the Skills extension (SEP-2640). */ +export const SKILLS_EXTENSION_KEY = "io.modelcontextprotocol/skills"; + +/** The `skills/list` JSON-RPC method name. */ +export const SKILLS_LIST_METHOD = "skills/list"; + +/** The `skills/get` JSON-RPC method name. */ +export const SKILLS_GET_METHOD = "skills/get"; + +/** + * The `resources/directory/read` method name, gated on the server declaring + * `directoryRead: true` in its extension advertisement. Declared here so the + * one wire-surface module names every method the extension defines, even though + * the Inspector does not call it yet (phase 3 of #2234). + */ +export const RESOURCES_DIRECTORY_READ_METHOD = "resources/directory/read"; + +/** `mimeType` marking a resource as a directory rather than a file. */ +export const DIRECTORY_MIME_TYPE = "inode/directory"; + +/** + * The literal `resources` value meaning "this skill's file set is generated and + * cannot be enumerated". Integrity verification is impossible for such a skill, + * which is why it is a reported finding rather than a silent absence. + */ +export const DYNAMIC_RESOURCES = "dynamic"; + +/** + * The verbatim YAML frontmatter of a `SKILL.md`, expressed as JSON. `name` and + * `description` are the two fields SEP-2640 requires; everything else the Agent + * Skills format defines passes through untouched, since that format versions + * independently of this extension. + */ +export const SkillFrontmatterSchema = z.looseObject({ + name: z.string().optional(), + description: z.string().optional(), +}); + +export type SkillFrontmatter = z.infer; + +/** + * One file in a skill's manifest. `digest` is `sha256:` + 64 lowercase hex per + * the SEP, but it is typed as a bare string so a server that gets the format + * wrong still parses and can be *reported* — see `checkSkillConformance`. + */ +export const SkillResourceSchema = z.looseObject({ + uri: z.string(), + digest: z.string().optional(), + size: z.number().optional(), +}); + +export type SkillResource = z.infer; + +/** + * A skill entry as returned by `skills/list` and `skills/get`. `resources` is + * either the full file manifest or the literal `"dynamic"`; the union is + * preserved on the type rather than normalized away, because which one a server + * sent is itself the finding. + */ +export const SkillEntrySchema = z.looseObject({ + uri: z.string(), + frontmatter: SkillFrontmatterSchema, + resources: z.union([ + z.literal(DYNAMIC_RESOURCES), + z.array(SkillResourceSchema), + ]), +}); + +export type SkillEntry = z.infer; + +/** `skills/list` result: a page of entries plus the opaque cursor. */ +export const ListSkillsResultSchema = z.looseObject({ + skills: z.array(SkillEntrySchema), + nextCursor: z.string().optional(), +}); + +export type ListSkillsResult = z.infer; + +/** + * The envelope form of a `skills/get` result: the entry wrapped under `skill`. + */ +const GetSkillEnvelopeSchema = z.looseObject({ skill: SkillEntrySchema }); + +/** + * Collapse either accepted `skills/get` shape to the entry it carries. + * + * Written as a parse rather than an `in` check because both accepted shapes are + * loose objects — they carry an index signature, so `"skill" in result` narrows + * nothing and would leave the extracted value `unknown`. Parsing the envelope + * is what proves its `skill` really is an entry, with no cast anywhere. + */ +export function normalizeGetSkillResult(result: unknown): SkillEntry { + const enveloped = GetSkillEnvelopeSchema.safeParse(result); + return enveloped.success + ? enveloped.data.skill + : SkillEntrySchema.parse(result); +} + +/** + * `skills/get` result, normalized to the entry. + * + * ⚠️ The SEP settles the *entry* shape but not the envelope this result wraps it + * in, so both plausible forms are accepted: `{ skill: }` and the entry + * returned inline at the top level. Being permissive here costs nothing (the + * two are structurally distinguishable — an inline entry carries `uri` and + * `frontmatter`, the envelope carries neither) and spares a server author a + * failure whose cause is a spec ambiguity rather than their code. The transform + * means every caller receives the entry and none of them branches. + */ +export const GetSkillResultSchema = z + .union([GetSkillEnvelopeSchema, SkillEntrySchema]) + .transform(normalizeGetSkillResult); + +export type GetSkillResult = SkillEntry; + +/** + * `resources/directory/read` result — the direct (non-recursive) children of a + * directory resource. Present for completeness of the wire surface; the + * Inspector surfaces the `directoryRead` sub-flag today and calls the method in + * phase 3. + */ +export const ReadResourceDirectoryResultSchema = z.looseObject({ + contents: z.array( + z.looseObject({ + uri: z.string(), + name: z.string().optional(), + mimeType: z.string().optional(), + size: z.number().optional(), + }), + ), + nextCursor: z.string().optional(), +}); + +export type ReadResourceDirectoryResult = z.infer< + typeof ReadResourceDirectoryResultSchema +>; diff --git a/core/mcp/state/index.ts b/core/mcp/state/index.ts index af1e9f65..ab66e34e 100644 --- a/core/mcp/state/index.ts +++ b/core/mcp/state/index.ts @@ -50,3 +50,8 @@ export type { } from "./pagedRequestorTasksState.js"; export { ResourceSubscriptionsState } from "./resourceSubscriptionsState.js"; export type { ResourceSubscriptionsStateEventMap } from "./resourceSubscriptionsState.js"; +export { ManagedSkillsState } from "./managedSkillsState.js"; +export type { + ManagedSkillsStateEventMap, + SkillsPaginationState, +} from "./managedSkillsState.js"; diff --git a/core/mcp/state/managedSkillsState.ts b/core/mcp/state/managedSkillsState.ts new file mode 100644 index 00000000..b28a784d --- /dev/null +++ b/core/mcp/state/managedSkillsState.ts @@ -0,0 +1,181 @@ +/** + * ManagedSkillsState: holds the full skill list (SEP-2640), in sync with the + * server. Loaded on connect, cleared on disconnect, re-walked on refresh. + * + * Deliberately NOT a `ManagedListState` subclass, despite the family + * resemblance. That base is built around two things the Skills extension does + * not have: a top-level `ServerCapabilities` key to gate on (skills is a + * *server-declared extension*, read from `capabilities.extensions`), and a + * per-list `list_changed` notification to debounce and turn into a sidebar + * indicator + * (SEP-2640 defines none). Subclassing would mean widening the base's + * capability gate and inventing a list-changed event nothing dispatches — two + * changes to shared machinery to serve one caller. The cursor walk below is the + * only behavior actually shared, and it is nine lines. + * + * The walk is done here rather than through an SDK `listAll*` verb for the same + * reason the request is a plain `client.request`: the SDK has no high-level verb + * for a consumer-owned extension method, so there is no cache-aware wrapper to + * delegate to and no `cacheMode` to honor. + */ + +import type { InspectorClientProtocol } from "../inspectorClientProtocol.js"; +import type { SkillEntry } from "../skillsSchemas.js"; +import { isTerminalStatus } from "../types.js"; +import type { RequestMetadata } from "../types.js"; +import { TypedEventTarget } from "../typedEventTarget.js"; + +export interface SkillsPaginationState { + /** Pages walked on the last completed refresh (a one-page list is 1). */ + pageCount: number; +} + +export interface ManagedSkillsStateEventMap { + skillsChange: SkillEntry[]; + paginationChange: SkillsPaginationState; + /** The last walk's failure, or `null` once one succeeds. */ + errorChange: Error | null; +} + +/** + * Thrown when a server hands back a cursor it already handed back, which would + * otherwise walk forever. Surfaced as the store's error so the panel reports the + * server bug instead of hanging — the same "report, don't swallow" posture the + * conformance checks take. + */ +export const REPEATED_CURSOR_MESSAGE = + "Server repeated a pagination cursor in skills/list; stopped to avoid an infinite walk."; + +export class ManagedSkillsState extends TypedEventTarget { + private skills: SkillEntry[] = []; + private pageCount = 0; + private error: Error | null = null; + private client: InspectorClientProtocol | null = null; + private unsubscribe: (() => void) | null = null; + // Overlap guard: a walk in flight makes a second one a no-op so a slow older + // walk can't clobber a newer list via last-write-wins. + private running = false; + + constructor(client: InspectorClientProtocol) { + super(); + this.client = client; + const onConnect = (): void => { + // No caller to await the connect-time load, so its rejection is caught + // here rather than left to become an unhandled rejection. Not a swallow: + // `refresh` has already recorded the failure via `setError`, and the + // panel renders it. + void this.refresh().catch(() => {}); + }; + const onStatusChange = (): void => { + if (isTerminalStatus(this.client?.getStatus())) { + this.reset(); + } + }; + this.client.addEventListener("connect", onConnect); + this.client.addEventListener("statusChange", onStatusChange); + this.unsubscribe = () => { + if (this.client) { + this.client.removeEventListener("connect", onConnect); + this.client.removeEventListener("statusChange", onStatusChange); + } + this.client = null; + }; + } + + /** Defensive copy of the current list. */ + getSkills(): SkillEntry[] { + return [...this.skills]; + } + + getPagination(): SkillsPaginationState { + return { pageCount: this.pageCount }; + } + + /** The last walk's failure, or `null` when it succeeded. */ + getError(): Error | null { + return this.error; + } + + // Compared by identity rather than message: two distinct failures with the + // same text are still two events, and a re-render on a repeat failure is + // cheap next to silently coalescing them. + private setError(value: Error | null): void { + if (this.error === value) return; + this.error = value; + this.dispatchTypedEvent("errorChange", value); + } + + private reset(): void { + this.skills = []; + this.pageCount = 0; + this.dispatchTypedEvent("skillsChange", this.getSkills()); + this.dispatchTypedEvent("paginationChange", this.getPagination()); + // A disconnect ends the session the error belonged to — a stale + // "couldn't load skills" must not outlive it into the next connect. + this.setError(null); + } + + /** + * Walk every page of `skills/list` and commit the result. + * + * A failure is recorded as observable state (`getError`) AND re-thrown: the + * state drives the panel's error rendering, while the rejection is what a + * caller's auth-recovery wrapper keys off to detect a 401 and start a + * re-authorization. The connect-time load, which has no such caller, catches + * it above. + */ + async refresh(metadata?: RequestMetadata): Promise { + const client = this.client; + if (!client || client.getStatus() !== "connected") return this.getSkills(); + // A server that never declared the extension answers `skills/list` with + // -32601, which would spam the console for a question we already know the + // answer to. An empty list is the right semantics. + if (!client.getSkillsExtension()) { + this.applyPages([], 0); + return this.getSkills(); + } + if (this.running) return this.getSkills(); + this.running = true; + try { + const collected: SkillEntry[] = []; + const seen = new Set(); + let cursor: string | undefined; + let pages = 0; + for (;;) { + const page = await client.listSkills(cursor, metadata); + collected.push(...page.skills); + pages += 1; + if (page.nextCursor === undefined) break; + if (seen.has(page.nextCursor)) { + throw new Error(REPEATED_CURSOR_MESSAGE); + } + seen.add(page.nextCursor); + cursor = page.nextCursor; + } + this.setError(null); + this.applyPages(collected, pages); + return this.getSkills(); + } catch (err) { + this.setError(err instanceof Error ? err : new Error(String(err))); + throw err; + } finally { + this.running = false; + } + } + + private applyPages(skills: SkillEntry[], pageCount: number): void { + this.skills = skills; + this.pageCount = pageCount; + this.dispatchTypedEvent("skillsChange", this.getSkills()); + this.dispatchTypedEvent("paginationChange", this.getPagination()); + } + + /** Unsubscribe from the client and drop the list; idempotent. */ + destroy(): void { + this.unsubscribe?.(); + this.unsubscribe = null; + this.skills = []; + this.pageCount = 0; + this.error = null; + } +} diff --git a/core/react/useManagedSkills.ts b/core/react/useManagedSkills.ts new file mode 100644 index 00000000..e02e1219 --- /dev/null +++ b/core/react/useManagedSkills.ts @@ -0,0 +1,76 @@ +import { useCallback } from "react"; +import type { InspectorClientProtocol } from "../mcp/inspectorClientProtocol.js"; +import type { + ManagedSkillsState, + SkillsPaginationState, +} from "../mcp/state/managedSkillsState.js"; +import type { SkillEntry } from "../mcp/skillsSchemas.js"; +import { useListError } from "./useListError.js"; +import { useStoreSnapshot } from "./useStoreSnapshot.js"; + +/** + * Shared stable empty values for the no-server case. Module scope so the + * snapshots don't change identity every render — see `useStoreSnapshot`. + * Read-only by contract: nothing mutates a value this hook returns. + */ +const NO_SKILLS: SkillEntry[] = []; +const NO_SKILLS_PAGINATION: SkillsPaginationState = Object.freeze({ + pageCount: 0, +}); + +const readSkills = (state: ManagedSkillsState): SkillEntry[] => + state.getSkills(); +const readPagination = (state: ManagedSkillsState): SkillsPaginationState => + state.getPagination(); + +export interface UseManagedSkillsResult { + skills: SkillEntry[]; + /** Pages walked on the last completed refresh (a one-page list is 1). */ + pageCount: number; + /** + * The last walk's failure, or `null` when it succeeded. Includes the + * connect-time load, whose failure has no caller to surface it. + */ + error: Error | null; + refresh: () => Promise; +} + +/** + * React hook over `ManagedSkillsState` (SEP-2640): the full skill list, the + * page count the walk took, the last failure, and a refresh. + * + * Read during render via `useStoreSnapshot`, never `useState` + a subscribing + * effect — that shape re-seeds local state from the store prop, so switching + * servers would paint one frame of the previous server's skills, and an event + * dispatched between render and subscribe would be lost outright. + */ +export function useManagedSkills( + client: InspectorClientProtocol | null, + managedSkillsState: ManagedSkillsState | null, +): UseManagedSkillsResult { + const skills = useStoreSnapshot( + managedSkillsState, + "skillsChange", + readSkills, + NO_SKILLS, + ); + const { pageCount } = useStoreSnapshot( + managedSkillsState, + "paginationChange", + readPagination, + NO_SKILLS_PAGINATION, + ); + + const error = useListError(managedSkillsState); + + const refresh = useCallback(async (): Promise => { + if (!managedSkillsState || !client) return NO_SKILLS; + // The store dispatches `skillsChange` as it commits, so the snapshot above + // updates on its own. No `cacheMode`: `skills/list` is a consumer-owned + // extension method with no SDK cache-aware verb behind it, so every walk is + // already a real round trip. + return managedSkillsState.refresh(); + }, [client, managedSkillsState]); + + return { skills, pageCount, error, refresh }; +} diff --git a/docs/test-servers.md b/docs/test-servers.md index 93e699c1..a0b4419c 100644 --- a/docs/test-servers.md +++ b/docs/test-servers.md @@ -56,6 +56,29 @@ as a missing capability rather than an error. | `subscriptions-never-acknowledged-http.json` **(modern era)** | A `subscriptions/listen` answered with a bare result | [#2097](https://github.com/modelcontextprotocol/inspector/issues/2097) | | `tasks-{legacy,modern}-http.json` **(era per file)** | Tasks, both eras | [#1631](https://github.com/modelcontextprotocol/inspector/issues/1631) | | `cancellation-modern-http.json` **(modern era)** | Cancelling a call by closing its response stream | [#2140](https://github.com/modelcontextprotocol/inspector/issues/2140) | +| `skills-http.json` **(either era)** | Skills tab: `skills/list`, digest verification, and the non-conforming cases | [#2234](https://github.com/modelcontextprotocol/inspector/issues/2234) | + +## Skills (SEP-2640) + +`skills-http.json` advertises the `io.modelcontextprotocol/skills` extension +with `directoryRead: true` and serves four skills over two `skills/list` pages. +It works on **either era**: `skills/list`, `skills/get` and +`resources/directory/read` are consumer-owned extension methods that neither +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. + +Three of the four skills are deliberately non-conforming, because the checks +the Skills tab runs are untestable without them: + +| Skill | What it exercises | +| --- | --- | +| `data-analysis` | The clean case — **Verify all** reports `verified` for every file. | +| `tampered-notes` | An advertised digest that does not match the bytes served, so verification reports a **digest mismatch** with both digests shown. | +| `dynamic-report` | `resources: "dynamic"` — a generated file set, so integrity cannot be verified at all and the tab says so rather than staying silent. | +| `wrong-folder` | A URI path segment (`wrong-folder`) that disagrees with `frontmatter.name` (`right-name`), the one structural invariant SEP-2640 states outright. | + +Connection Info shows the extension and its `directoryRead` sub-flag; the +Inspector surfaces that flag but does not call `resources/directory/read` yet. ## Cancelling a call diff --git a/test-servers/configs/skills-http.json b/test-servers/configs/skills-http.json new file mode 100644 index 00000000..dab5ae2c --- /dev/null +++ b/test-servers/configs/skills-http.json @@ -0,0 +1,15 @@ +{ + "serverInfo": { + "name": "skills", + "version": "1.0.0" + }, + "tools": [{ "preset": "echo" }], + "resources": [], + "skills": { + "directoryRead": true + }, + "transport": { + "type": "streamable-http", + "port": 3230 + } +} diff --git a/test-servers/src/composable-test-server.ts b/test-servers/src/composable-test-server.ts index eddb56d5..3bbfb3a0 100644 --- a/test-servers/src/composable-test-server.ts +++ b/test-servers/src/composable-test-server.ts @@ -49,6 +49,7 @@ import { createModernTaskTools, wireModernTaskHandlers, } from "./modern-tasks.js"; +import { SKILLS_EXTENSION_KEY, wireSkillsHandlers } from "./skills.js"; /** * MCP Apps extension id. Hardcoded for the same reason the Inspector's @@ -556,6 +557,12 @@ export interface ServerConfig { * `modern: true`. See `modern-tasks.ts`. */ tasksExtension?: boolean; + /** + * Advertise the Skills extension (SEP-2640) and serve `skills/list` / + * `skills/get` plus the `skill://` files those entries name. The fixture set + * deliberately includes non-conforming skills — see `skills.ts`. + */ + skills?: { directoryRead?: boolean }; /** * Advertise the MCP Apps `io.modelcontextprotocol/ui` extension with the * nested `elicitation` setting — the server-side half of the app-rendered @@ -824,6 +831,23 @@ export function createMcpServer(config: ServerConfig): McpServer { }; } + // Skills extension (SEP-2640): a server-declared extension, advertised with + // its one sub-option. `directoryRead` is opt-in per config so a client can be + // exercised against both a server that offers `resources/directory/read` and + // one that does not. + if (config.skills) { + capabilities.extensions = { + ...(capabilities.extensions ?? {}), + [SKILLS_EXTENSION_KEY]: { + ...(config.skills.directoryRead ? { directoryRead: true } : {}), + }, + }; + // Skill files are fetched through ordinary `resources/read`, so the + // resources capability has to be advertised even when the config registers + // no ordinary resources of its own. + capabilities.resources = capabilities.resources ?? {}; + } + // MCP Apps app-rendered elicitation (#1854): the server-side half of the // negotiation, on the same extension the Apps work already uses. if (config.appElicitation) { @@ -1503,6 +1527,13 @@ export function createMcpServer(config: ServerConfig): McpServer { wireModernTaskHandlers(mcpServer, modernTaskRuntime); } + // Skills extension (SEP-2640): raw skills/list + skills/get, and the + // `skill://` half of resources/read. Wired after the SDK's own handlers so + // the resources/read wrapper can delegate non-skill URIs to them. + if (config.skills) { + wireSkillsHandlers(mcpServer); + } + // Extension-gated tools (#1739): start each gated tool disabled, then enable // it on `initialized` iff the connected client declared its extension. if (config.extensionGatedTools) { diff --git a/test-servers/src/load-config.ts b/test-servers/src/load-config.ts index dc68bb8d..1b136a8c 100644 --- a/test-servers/src/load-config.ts +++ b/test-servers/src/load-config.ts @@ -67,6 +67,9 @@ export interface ConfigFile { * and wire its handlers + `modern_task` / `modern_input_task` tools. Pair with * `transport.modern`. */ tasksExtension?: boolean; + /** Advertise the Skills extension (SEP-2640) and serve its fixture skills. + * `directoryRead` advertises the `resources/directory/read` sub-option. */ + skills?: { directoryRead?: boolean }; /** Advertise the MCP Apps `io.modelcontextprotocol/ui` extension with the nested * `elicitation` setting — the server half of app-rendered form elicitation * (#1854). Pair with the `app_choose_option` tool + `choose_option_app` resource. */ diff --git a/test-servers/src/resolve-config.ts b/test-servers/src/resolve-config.ts index 693c0bc3..1448da03 100644 --- a/test-servers/src/resolve-config.ts +++ b/test-servers/src/resolve-config.ts @@ -90,6 +90,7 @@ export function resolveConfig(config: ConfigFile): ServerConfig { subscriptions: config.subscriptions, tasks: config.tasks, tasksExtension: config.tasksExtension, + skills: config.skills, appElicitation: config.appElicitation, maxPageSize: config.maxPageSize, duplicateToolNames: config.duplicateToolNames, diff --git a/test-servers/src/skills.ts b/test-servers/src/skills.ts new file mode 100644 index 00000000..03f0810c --- /dev/null +++ b/test-servers/src/skills.ts @@ -0,0 +1,280 @@ +/** + * Skills extension test fixture — SEP-2640 (`io.modelcontextprotocol/skills`). + * + * Serves `skills/list` (paginated) and `skills/get`, plus `resources/read` for + * the `skill://` URIs those entries name, so an Inspector connected here can + * exercise the whole flow: enumerate, fetch a file, and verify its digest. + * + * **The non-conforming skills are the point.** A fixture that only served a + * clean skill would leave every verification and conformance path in the + * Inspector untestable, so the set below deliberately includes one `"dynamic"` + * skill, one whose advertised digest does not match the bytes served, and one + * whose URI path segment disagrees with `frontmatter.name`. Each is the exact + * shape one of the checks in `core/mcp/skills.ts` exists to catch. + * + * Handlers are installed straight into the low-level `_requestHandlers` map + * rather than through `setRequestHandler`, the same seam `modern-tasks.ts` + * uses. `skills/*` are consumer-owned extension methods that neither era codec + * defines, so they need no schemas and are era-blind in both directions — + * which is what lets one fixture serve both the legacy and modern legs. + */ + +import { createHash } from "node:crypto"; +import type { McpServer } from "@modelcontextprotocol/server"; + +/** SEP-2133 extension identifier for the Skills extension (SEP-2640). */ +export const SKILLS_EXTENSION_KEY = "io.modelcontextprotocol/skills"; + +/** + * Entries per `skills/list` page. Two, deliberately: the fixture serves four + * skills, so a client that stops after page one sees half the set — which is + * what makes a broken cursor walk visible rather than merely slower. + */ +export const SKILLS_PAGE_SIZE = 2; + +/** `sha256:<64 lowercase hex>` over a UTF-8 string, the SEP's digest form. */ +function digestOf(text: string): string { + return `sha256:${createHash("sha256").update(text, "utf8").digest("hex")}`; +} + +/** Byte length of a UTF-8 string, for the manifest's `size`. */ +function sizeOf(text: string): number { + return Buffer.byteLength(text, "utf8"); +} + +interface FixtureFile { + uri: string; + text: string; + mimeType: string; + /** + * Digest to *advertise*, when it should differ from the real one. The + * tampered skill sets this; everywhere else the advertised digest is + * computed from the very bytes served, so a clean skill verifies. + */ + advertisedDigest?: string; +} + +interface FixtureSkill { + /** The `` segment; `skill:///SKILL.md` is the entry URI. */ + path: string; + frontmatter: Record; + /** `"dynamic"` for a generated skill with no enumerable manifest. */ + files: FixtureFile[] | "dynamic"; +} + +function skillMd(name: string, description: string, body: string): string { + return `---\nname: ${name}\ndescription: ${description}\n---\n\n${body}\n`; +} + +const DATA_ANALYSIS_MD = skillMd( + "data-analysis", + "Analyze a CSV and summarize its columns", + "# Data analysis\n\nLoad the CSV, then follow `reference.md` for the column rules.", +); +const DATA_ANALYSIS_REF = + "# Column rules\n\nNumeric columns get min/max/mean; text columns get a value count.\n"; + +const TAMPERED_MD = skillMd( + "tampered-notes", + "A skill whose manifest digest does not match its served bytes", + "# Tampered notes\n\nThe digest advertised for `notes.md` is wrong on purpose.", +); +const TAMPERED_NOTES = + "# Notes\n\nThese bytes hash to something other than what the manifest claims.\n"; + +const DYNAMIC_MD = skillMd( + "dynamic-report", + "A skill whose files are generated per request", + "# Dynamic report\n\nThis skill's file set is generated, so it advertises no manifest.", +); + +// The frontmatter says `right-name` while the URI segment says `wrong-folder`, +// breaking the one structural invariant SEP-2640 states outright: the segment +// before /SKILL.md must equal frontmatter.name. +const MISMATCHED_MD = skillMd( + "right-name", + "A skill whose URI path segment disagrees with its frontmatter name", + "# Mismatched name\n\nServed from `wrong-folder/` while claiming the name `right-name`.", +); + +const FIXTURE_SKILLS: FixtureSkill[] = [ + { + path: "data-analysis", + frontmatter: { + name: "data-analysis", + description: "Analyze a CSV and summarize its columns", + }, + files: [ + { + uri: "skill://data-analysis/SKILL.md", + text: DATA_ANALYSIS_MD, + mimeType: "text/markdown", + }, + { + uri: "skill://data-analysis/reference.md", + text: DATA_ANALYSIS_REF, + mimeType: "text/markdown", + }, + ], + }, + { + path: "tampered-notes", + frontmatter: { + name: "tampered-notes", + description: "A skill whose manifest digest does not match its bytes", + }, + files: [ + { + uri: "skill://tampered-notes/SKILL.md", + text: TAMPERED_MD, + mimeType: "text/markdown", + }, + { + uri: "skill://tampered-notes/notes.md", + text: TAMPERED_NOTES, + mimeType: "text/markdown", + // A syntactically valid digest of the *wrong* bytes, so the failure the + // Inspector reports is a mismatch rather than a malformed-digest + // finding — those are different checks and must stay distinguishable. + advertisedDigest: digestOf("not the bytes this server serves"), + }, + ], + }, + { + path: "dynamic-report", + frontmatter: { + name: "dynamic-report", + description: "A skill whose files are generated per request", + }, + files: "dynamic", + }, + { + path: "wrong-folder", + frontmatter: { + name: "right-name", + description: "A skill whose URI segment disagrees with its name", + }, + files: [ + { + uri: "skill://wrong-folder/SKILL.md", + text: MISMATCHED_MD, + mimeType: "text/markdown", + }, + ], + }, +]; + +/** Every servable `skill://` file, by URI. `dynamic` skills contribute their + * `SKILL.md` too, so the screen's "View SKILL.md" works there as well. */ +const FILES_BY_URI = new Map(); +for (const skill of FIXTURE_SKILLS) { + if (skill.files === "dynamic") { + FILES_BY_URI.set(`skill://${skill.path}/SKILL.md`, { + uri: `skill://${skill.path}/SKILL.md`, + text: DYNAMIC_MD, + mimeType: "text/markdown", + }); + continue; + } + for (const file of skill.files) FILES_BY_URI.set(file.uri, file); +} + +/** The wire entry for one fixture skill. */ +function toEntry(skill: FixtureSkill): Record { + return { + uri: `skill://${skill.path}/SKILL.md`, + frontmatter: skill.frontmatter, + resources: + skill.files === "dynamic" + ? "dynamic" + : skill.files.map((file) => ({ + uri: file.uri, + digest: file.advertisedDigest ?? digestOf(file.text), + size: sizeOf(file.text), + })), + }; +} + +/** One `skills/list` page starting at `cursor` (an index, as a string). */ +export function listSkillsPage(cursor?: string): Record { + const start = cursor ? Number.parseInt(cursor, 10) : 0; + // A cursor the fixture never issued is answered as an empty final page + // rather than an error: the Inspector's walk should terminate, and a thrown + // error here would read as a transport failure instead. + const from = Number.isFinite(start) && start > 0 ? start : 0; + const page = FIXTURE_SKILLS.slice(from, from + SKILLS_PAGE_SIZE); + const next = from + SKILLS_PAGE_SIZE; + return { + skills: page.map(toEntry), + ...(next < FIXTURE_SKILLS.length ? { nextCursor: String(next) } : {}), + }; +} + +/** The `skills/get` result for one entry URI. */ +export function getSkillEntry(uri: string): Record { + const skill = FIXTURE_SKILLS.find( + (candidate) => `skill://${candidate.path}/SKILL.md` === uri, + ); + if (!skill) throw new Error(`Unknown skill uri: ${uri}`); + // The envelope form (`{ skill }`) is served deliberately: the SEP settles the + // entry shape but not this wrapper, and the Inspector accepts both — serving + // the wrapped one keeps that tolerance exercised. + return { skill: toEntry(skill) }; +} + +/** The `resources/read` result for a `skill://` file, or `undefined`. */ +export function readSkillFile( + uri: string, +): Record | undefined { + const file = FILES_BY_URI.get(uri); + if (!file) return undefined; + return { + contents: [{ uri: file.uri, mimeType: file.mimeType, text: file.text }], + }; +} + +/** The private handler registry the SDK dispatches through. */ +interface RawHandlerHost { + _requestHandlers: Map< + string, + (request: unknown, ctx: unknown) => Promise + >; +} + +interface UriRequest { + params?: { uri?: string; cursor?: string }; +} + +/** + * Wire `skills/list`, `skills/get` and the `skill://` half of `resources/read` + * onto an `McpServer`. + * + * `resources/read` is wrapped rather than replaced: a `skill://` URI is + * answered here and everything else falls through to whatever the SDK + * registered, so a config can serve ordinary resources alongside its skills. + */ +export function wireSkillsHandlers(mcpServer: McpServer): void { + const registry = (mcpServer.server as unknown as RawHandlerHost) + ._requestHandlers; + + registry.set("skills/list", async (request) => { + const req = request as UriRequest; + return listSkillsPage(req.params?.cursor); + }); + + registry.set("skills/get", async (request) => { + const req = request as UriRequest; + return getSkillEntry(req.params?.uri ?? ""); + }); + + const sdkResourcesRead = registry.get("resources/read"); + registry.set("resources/read", async (request, ctx) => { + const req = request as UriRequest; + const skillFile = readSkillFile(req.params?.uri ?? ""); + if (skillFile) return skillFile; + if (!sdkResourcesRead) { + throw new Error(`Unknown resource: ${req.params?.uri}`); + } + return sdkResourcesRead(request, ctx); + }); +}