diff --git a/code/addons/mcp/scripts/generate-tools-api-doc.ts b/code/addons/mcp/scripts/generate-tools-api-doc.ts index 6dde5dc643bd..10e5febe1164 100644 --- a/code/addons/mcp/scripts/generate-tools-api-doc.ts +++ b/code/addons/mcp/scripts/generate-tools-api-doc.ts @@ -16,6 +16,7 @@ import { fileURLToPath } from 'node:url'; import { promisify } from 'node:util'; import { ValibotJsonSchemaAdapter } from '@tmcp/adapter-valibot'; import { buildServerInstructions } from '../src/instructions/build-server-instructions.ts'; +import { registerCoreToolsetsForTest } from '../src/test-support/register-core-toolsets.ts'; import { getAddonToolMetadata, type ToolMetadata } from '../src/tools/tool-registry.ts'; import type { ToolAvailability } from '../src/utils/get-tool-availability.ts'; @@ -41,6 +42,15 @@ const availability = (reviewEnabled: boolean): ToolAvailability => ({ docgenServer: false, }); +/** + * Metadata is resolved from the live toolset registry, so each rendered mode registers the real + * toolsets first — review-off and review-on descriptions genuinely differ per registration. + */ +function toolMetadataFor(reviewEnabled: boolean): ToolMetadata[] { + registerCoreToolsetsForTest({ reviewEnabled }); + return getAddonToolMetadata({ availability: availability(reviewEnabled) }); +} + const instructions = (reviewEnabled: boolean) => buildServerInstructions({ devEnabled: true, @@ -218,7 +228,7 @@ async function renderTool(tool: ToolMetadata): Promise { } async function renderServerSection(title: string, reviewEnabled: boolean): Promise { - const tools = getAddonToolMetadata({ availability: availability(reviewEnabled) }); + const tools = toolMetadataFor(reviewEnabled); const toolSections = await Promise.all(tools.map(renderTool)); return [ `## ${title}`, @@ -236,12 +246,8 @@ async function renderServerSection(title: string, reviewEnabled: boolean): Promi } function diffSummary(): string { - const off = new Map( - getAddonToolMetadata({ availability: availability(false) }).map((t) => [t.name, t]) - ); - const on = new Map( - getAddonToolMetadata({ availability: availability(true) }).map((t) => [t.name, t]) - ); + const off = new Map(toolMetadataFor(false).map((t) => [t.name, t])); + const on = new Map(toolMetadataFor(true).map((t) => [t.name, t])); const added = [...on.keys()].filter((name) => !off.has(name)); const removed = [...off.keys()].filter((name) => !on.has(name)); const changed = [...on.keys()].filter( @@ -306,7 +312,7 @@ async function runCliHelp(args: string[]): Promise { } async function renderCliSection(): Promise { - const toolNames = getAddonToolMetadata({ availability: availability(true) }).map((t) => t.name); + const toolNames = toolMetadataFor(true).map((t) => t.name); const commands = ['setup', ...toolNames]; const [topLevel, ...commandHelps] = await Promise.all([ runCliHelp([]), diff --git a/code/addons/mcp/src/mcp-handler.test.ts b/code/addons/mcp/src/mcp-handler.test.ts index 090519ff79c0..276550e816e5 100644 --- a/code/addons/mcp/src/mcp-handler.test.ts +++ b/code/addons/mcp/src/mcp-handler.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { registerCoreToolsetsForTest } from './test-support/register-core-toolsets.ts'; import { incomingMessageToWebRequest, webResponseToServerResponse, @@ -205,6 +206,8 @@ describe('mcpServerHandler', () => { beforeEach(async () => { // Reset modules and get fresh handler for each test to avoid state pollution vi.resetModules(); + // The `services` preset hook does this in a real Storybook, before the MCP server boots. + registerCoreToolsetsForTest(); const handler = await import('./mcp-handler.ts'); mcpServerHandler = handler.mcpServerHandler; }); diff --git a/code/addons/mcp/src/preset.test.ts b/code/addons/mcp/src/preset.test.ts index 1978190dea32..e73087fd78ed 100644 --- a/code/addons/mcp/src/preset.test.ts +++ b/code/addons/mcp/src/preset.test.ts @@ -4,6 +4,7 @@ import { experimental_devServer } from './preset.ts'; import * as mcpHandlerModule from './mcp-handler.ts'; import * as runStoryTests from './tools/run-story-tests.ts'; import * as moduleGraph from './utils/module-graph.ts'; +import { registerCoreToolsetsForTest } from './test-support/register-core-toolsets.ts'; describe('experimental_devServer', () => { let mockApp: any; @@ -11,6 +12,8 @@ describe('experimental_devServer', () => { let mcpHandler: any; beforeEach(() => { + // The `services` preset hook does this in a real Storybook before the dev server mounts /mcp. + registerCoreToolsetsForTest(); vi.restoreAllMocks(); mockApp = { diff --git a/code/addons/mcp/src/storybook-ai-metadata.test.ts b/code/addons/mcp/src/storybook-ai-metadata.test.ts index 363ee747cfab..5fe831157e34 100644 --- a/code/addons/mcp/src/storybook-ai-metadata.test.ts +++ b/code/addons/mcp/src/storybook-ai-metadata.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { registerCoreToolsetsForTest } from './test-support/register-core-toolsets.ts'; import { McpServer } from 'tmcp'; import { ValibotJsonSchemaAdapter } from '@tmcp/adapter-valibot'; import { GET_TOOL_NAME } from '@storybook/mcp'; @@ -47,6 +48,8 @@ vi.mock('./tools/run-story-tests.ts', async (importActual) => ({ describe('buildStorybookAiMetadata', () => { beforeEach(() => { + // The `services` preset hook does this in a real Storybook before metadata is built. + registerCoreToolsetsForTest(); vi.clearAllMocks(); vi.stubGlobal('fetch', vi.fn(mockManifestFetch(true))); vi.mocked(isModuleGraphSupported).mockResolvedValue(true); diff --git a/code/addons/mcp/src/test-support/register-core-toolsets.ts b/code/addons/mcp/src/test-support/register-core-toolsets.ts new file mode 100644 index 000000000000..0d505efb101a --- /dev/null +++ b/code/addons/mcp/src/test-support/register-core-toolsets.ts @@ -0,0 +1,35 @@ +/** + * Registers the core toolsets the MCP tools are adapters over. + * + * Test support: in a real Storybook the `services` preset hook registers these before the MCP + * server boots, so a test that exercises tool registration has to stand that step up too. The real + * toolsets are used (with stub runtime dependencies) rather than fakes, so the adapter is tested + * against the definitions it ships with. + */ + +import type { StoryIndex } from 'storybook/internal/types'; +import { clearToolsetRegistry } from 'storybook/open-service'; +import { createStoriesToolset, registerToolset } from 'storybook/internal/core-server'; + +const EMPTY_INDEX: StoryIndex = { v: 5, entries: {} }; + +export function registerCoreToolsetsForTest({ + index = EMPTY_INDEX, + reviewEnabled = true, +}: { index?: StoryIndex; reviewEnabled?: boolean } = {}) { + clearToolsetRegistry(); + + const storyIndex = { getIndex: async () => index }; + + registerToolset( + createStoriesToolset({ + storyIndex, + git: { + getRepoRoot: async () => process.cwd(), + getChangedFiles: async () => ({ changed: new Set(), new: new Set() }), + }, + changeStatuses: { getAll: () => ({}) }, + reviewEnabled, + }) + ); +} diff --git a/code/addons/mcp/src/tools/get-changed-stories.test.ts b/code/addons/mcp/src/tools/get-changed-stories.test.ts deleted file mode 100644 index 66c4d7cb7729..000000000000 --- a/code/addons/mcp/src/tools/get-changed-stories.test.ts +++ /dev/null @@ -1,516 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { McpServer } from 'tmcp'; -import { ValibotJsonSchemaAdapter } from '@tmcp/adapter-valibot'; -import { addGetChangedStoriesTool } from './get-changed-stories.ts'; -import type { AddonContext } from '../types.ts'; -import * as getStoryIndexModule from '../utils/get-story-index.ts'; -import smallStoryIndexFixture from '../../fixtures/small-story-index.fixture.json' with { type: 'json' }; -import { GET_CHANGED_STORIES_TOOL_NAME } from './tool-names.ts'; -import type { StoryIndex } from 'storybook/internal/types'; - -const { mockGetStatusStore, mockGetService, mockExecSync } = vi.hoisted(() => ({ - mockGetStatusStore: vi.fn<(...args: any[]) => any>(), - // Resolves the `core/module-graph` open service. Defaults to "service inactive" - // (returns undefined); tests exercising the unreachable-files path override it - // with `mockGetService.mockReturnValue(moduleGraphStub(...))`. - mockGetService: vi.fn<(...args: any[]) => any>(), - // Hoisted because node:child_process is loaded inside - // detect-unreachable-changes.ts at module-eval time; ESM forbids - // retroactive vi.spyOn. - mockExecSync: vi.fn<(...args: any[]) => any>(), -})); - -vi.mock('storybook/internal/core-server', () => ({ - experimental_getStatusStore: (...args: unknown[]) => mockGetStatusStore(...args), - // `get-changed-stories` resolves this via `detectUnreachableChanges` to surface - // modified working-tree files that aren't reached from any story file. - getService: (...args: unknown[]) => mockGetService(...args), -})); - -/** - * Builds a `core/module-graph` runtime stub. `storiesForFiles` maps the batched input files to - * positional hit lists — an empty list marks a file as unreachable from every story. - */ -function moduleGraphStub(storiesForFiles: (files: string[]) => Array>) { - return { - queries: { - status: { loaded: async () => ({ value: 'ready' as const }) }, - storiesForFiles: { - loaded: async ({ files }: { files: string[] }) => storiesForFiles(files), - }, - }, - }; -} - -vi.mock('node:child_process', async () => { - const actual = await vi.importActual('node:child_process'); - return { ...actual, execSync: (...args: unknown[]) => mockExecSync(...(args as [])) }; -}); - -describe('getChangedStoriesTool', () => { - let server: McpServer; - const testContext: AddonContext = { - origin: 'http://localhost:6006', - options: {} as AddonContext['options'], - disableTelemetry: true, - }; - - beforeEach(async () => { - mockGetStatusStore.mockReset(); - mockGetService.mockReset(); - mockGetService.mockReturnValue(undefined); - mockExecSync.mockReset(); - mockExecSync.mockReturnValue(''); - const adapter = new ValibotJsonSchemaAdapter(); - server = new McpServer( - { - name: 'test-server', - version: '1.0.0', - description: 'Test server for get-changed-stories tool', - }, - { - adapter, - capabilities: { - tools: { listChanged: true }, - }, - } - ).withContext(); - - await server.receive( - { - jsonrpc: '2.0', - id: 1, - method: 'initialize', - params: { - protocolVersion: '2025-06-18', - capabilities: {}, - clientInfo: { name: 'test', version: '1.0.0' }, - }, - }, - { - sessionId: 'test-session', - } - ); - - await addGetChangedStoriesTool(server); - vi.spyOn(getStoryIndexModule, 'getStoryIndex').mockResolvedValue( - smallStoryIndexFixture as unknown as StoryIndex - ); - }); - - async function callTool() { - return server.receive( - { - jsonrpc: '2.0' as const, - id: 1, - method: 'tools/call', - params: { - name: GET_CHANGED_STORIES_TOOL_NAME, - arguments: {}, - }, - }, - { - sessionId: 'test-session', - custom: testContext, - } - ); - } - - function getResultText(response: unknown): string { - if (!response || typeof response !== 'object') return ''; - const result = (response as { result?: { content?: Array<{ text?: string }> } }).result; - return result?.content?.[0]?.text ?? ''; - } - - it('returns grouped markdown text with changed story metadata', async () => { - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'button--primary': { - 'storybook/change-detection': { - value: 'status-value:new', - storyId: 'button--primary', - }, - }, - 'button--secondary': { - 'storybook/change-detection': { - value: 'status-value:modified', - storyId: 'button--secondary', - }, - }, - 'input--default': { - 'storybook/change-detection': { - value: 'status-value:affected', - storyId: 'input--default', - }, - }, - }), - }); - - const response = await callTool(); - const text = getResultText(response); - - expect(getStoryIndexModule.getStoryIndex).toHaveBeenCalledWith(testContext.options); - expect(text).toMatchInlineSnapshot(` - "Detected 3 changed stories (1 new, 1 modified, 1 related). - - New stories: - - \`button--primary\`: Button / Primary (\`./src/Button.stories.tsx\`) - - Modified stories: - - \`button--secondary\`: Button / Secondary (\`./src/Button.stories.tsx\`) - - Related stories: - - \`input--default\`: Input / Default (\`./src/Input.stories.tsx\`)" - `); - }); - - it('filters out unsupported status values', async () => { - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'button--primary': { - 'storybook/change-detection': { - value: 'status-value:new', - storyId: 'button--primary', - }, - }, - 'button--secondary': { - 'storybook/change-detection': { - value: 'status-value:success', - storyId: 'button--secondary', - }, - }, - }), - }); - - const response = await callTool(); - const text = getResultText(response); - - expect(text).toMatchInlineSnapshot(` - "Detected 1 changed story (1 new, 0 modified, 0 related). - - New stories: - - \`button--primary\`: Button / Primary (\`./src/Button.stories.tsx\`)" - `); - }); - - it('groups by new, modified, related, then sorts by storyId', async () => { - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'input--default': { - 'storybook/change-detection': { - value: 'status-value:affected', - storyId: 'input--default', - }, - }, - 'button--secondary': { - 'storybook/change-detection': { - value: 'status-value:new', - storyId: 'button--secondary', - }, - }, - 'button--primary': { - 'storybook/change-detection': { - value: 'status-value:new', - storyId: 'button--primary', - }, - }, - }), - }); - - const response = await callTool(); - const text = getResultText(response); - - expect(text).toMatchInlineSnapshot(` - "Detected 3 changed stories (2 new, 0 modified, 1 related). - - New stories: - - \`button--primary\`: Button / Primary (\`./src/Button.stories.tsx\`) - - \`button--secondary\`: Button / Secondary (\`./src/Button.stories.tsx\`) - - Related stories: - - \`input--default\`: Input / Default (\`./src/Input.stories.tsx\`)" - `); - }); - - // Registers the tool on a fresh server with the review feature enabled, so - // the "publish the review now" next-step hint paths can be exercised. - async function createReviewEnabledServer() { - const adapter = new ValibotJsonSchemaAdapter(); - const reviewServer = new McpServer( - { - name: 'test-server-review', - version: '1.0.0', - description: 'Test server for get-changed-stories with review enabled', - }, - { adapter, capabilities: { tools: { listChanged: true } } } - ).withContext(); - - await reviewServer.receive( - { - jsonrpc: '2.0', - id: 1, - method: 'initialize', - params: { - protocolVersion: '2025-06-18', - capabilities: {}, - clientInfo: { name: 'test', version: '1.0.0' }, - }, - }, - { sessionId: 'test-session' } - ); - - await addGetChangedStoriesTool(reviewServer, undefined, { reviewEnabled: true }); - return reviewServer; - } - - async function callToolOn(targetServer: McpServer) { - return targetServer.receive( - { - jsonrpc: '2.0' as const, - id: 1, - method: 'tools/call', - params: { name: GET_CHANGED_STORIES_TOOL_NAME, arguments: {} }, - }, - { sessionId: 'test-session', custom: testContext } - ); - } - - it('appends the display-review next-step hint when review is enabled and stories were detected', async () => { - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'button--primary': { - 'storybook/change-detection': { - value: 'status-value:new', - storyId: 'button--primary', - }, - }, - }), - }); - - const text = getResultText(await callToolOn(await createReviewEnabledServer())); - - expect(text).toContain('Detected 1 changed story'); - expect(text).toContain('publish the review now — call **display-review**'); - }); - - it('omits the display-review hint when review is enabled but no stories resolve', async () => { - // Status store entries that don't exist in the index resolve to zero - // stories — exactly the case where the agent should fall back to - // get-stories-by-component instead of being pointed at an empty list. - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'not-in-index--at-all': { - 'storybook/change-detection': { - value: 'status-value:new', - storyId: 'not-in-index--at-all', - }, - }, - }), - }); - - const text = getResultText(await callToolOn(await createReviewEnabledServer())); - - expect(text).toContain('Detected 0 changed stories'); - expect(text).not.toContain('publish the review now'); - }); - - it('omits the display-review hint when review is disabled', async () => { - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'button--primary': { - 'storybook/change-detection': { - value: 'status-value:new', - storyId: 'button--primary', - }, - }, - }), - }); - - const text = getResultText(await callTool()); - - expect(text).toContain('Detected 1 changed story'); - expect(text).not.toContain('publish the review now'); - }); - - it('supports getAll() status-store API', async () => { - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'button--primary': { - 'storybook/change-detection': { - value: 'status-value:new', - storyId: 'button--primary', - }, - }, - }), - }); - - const response = await callTool(); - const text = getResultText(response); - - expect(text).toContain('Detected 1 changed story (1 new, 0 modified, 0 related).'); - expect(text).toContain('- `button--primary`: Button / Primary (`./src/Button.stories.tsx`)'); - }); - - it('supports getAll() for modified stories', async () => { - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'button--secondary': { - 'storybook/change-detection': { - value: 'status-value:modified', - storyId: 'button--secondary', - }, - }, - }), - }); - - const response = await callTool(); - const text = getResultText(response); - - expect(text).toContain('Detected 1 changed story (0 new, 1 modified, 0 related).'); - expect(text).toContain( - '- `button--secondary`: Button / Secondary (`./src/Button.stories.tsx`)' - ); - }); - - it('omits changed stories that are not in the index', async () => { - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'missing--story': { - 'storybook/change-detection': { - value: 'status-value:modified', - storyId: 'missing--story', - }, - }, - }), - }); - - const response = await callTool(); - const text = getResultText(response); - - expect(text).toMatchInlineSnapshot( - `"Detected 0 changed stories (0 new, 0 modified, 0 related)."` - ); - }); - - it('returns an MCP error when status store cannot be read', async () => { - mockGetStatusStore.mockReturnValue({}); - - const response = await server.receive( - { - jsonrpc: '2.0' as const, - id: 1, - method: 'tools/call', - params: { name: GET_CHANGED_STORIES_TOOL_NAME, arguments: {} }, - }, - { sessionId: 'test-session', custom: testContext } - ); - - expect(response.result).toEqual({ - content: [ - { - type: 'text', - text: 'Error: statusStore.getAll is not a function', - }, - ], - isError: true, - }); - }); - - it('returns early without fetching index when there are no relevant changed statuses', async () => { - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'button--primary': { - 'storybook/change-detection': { - value: 'status-value:success', - storyId: 'button--primary', - }, - }, - }), - }); - - const callCountBefore = vi.mocked(getStoryIndexModule.getStoryIndex).mock.calls.length; - const response = await callTool(); - const text = getResultText(response); - - expect(text).toBe('No new, modified, or related stories detected.'); - expect(vi.mocked(getStoryIndexModule.getStoryIndex).mock.calls.length).toBe(callCountBefore); - }); - - it('appends an unreachable-files hint to the empty response when the working tree has uncommitted source files outside the story graph', async () => { - // The "theme-token edit" case: the agent changed a file that isn't - // reached from any story root, so the status store is empty. Without - // this hint the agent reads "no impact" and stops — the original - // hallucination this whole feature exists to prevent. - mockGetStatusStore.mockReturnValue({ getAll: () => ({}) }); - // theme.ts is modified but no story file imports it. - mockGetService.mockReturnValue(moduleGraphStub((files) => files.map(() => []))); - mockExecSync.mockReturnValue(' M src/styles/theme.ts\n'); - - const response = await callTool(); - const text = getResultText(response); - - expect(text).toContain('No new, modified, or related stories detected.'); - expect(text).toContain('src/styles/theme.ts'); - expect(text).toMatch(/unreachable/i); - expect(text).toContain('get-stories-by-component'); - }); - - it('front-loads a coverage-gap banner AND appends a sanity-check hint when results coexist with unreachable working-tree files', async () => { - // Belt-and-braces: long story lists (Chromatic-scale) can run past - // host-side tool-output truncation caps, dropping the trailing hint. - // The leading banner is the short, survivable salience aid; the tail - // hint stays for agents that read in full. - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'button--primary': { - 'storybook/change-detection': { - value: 'status-value:modified', - storyId: 'button--primary', - }, - }, - }), - }); - mockGetService.mockReturnValue(moduleGraphStub((files) => files.map(() => []))); - mockExecSync.mockReturnValue(' M .storybook/main.ts\n M src/server.ts\n'); - - const response = await callTool(); - const text = getResultText(response); - - const bannerIdx = text.indexOf('Coverage gap'); - const headlineIdx = text.indexOf('Detected'); - expect(bannerIdx).toBeGreaterThanOrEqual(0); - expect(bannerIdx).toBeLessThan(headlineIdx); - expect(text).toContain('.storybook/main.ts'); - expect(text).toContain('src/server.ts'); - // And the long-form sanity-check hint still trails the bullet list, - // so agents that read in full get the explanatory paragraph. - expect(text).toMatch(/coverage sanity check/i); - }); - - it('omits both callouts when nothing in the working tree is unreachable', async () => { - mockGetStatusStore.mockReturnValue({ - getAll: () => ({ - 'button--primary': { - 'storybook/change-detection': { - value: 'status-value:modified', - storyId: 'button--primary', - }, - }, - }), - }); - // Every modified file IS in the graph — no unreachable callout fires. - mockGetService.mockReturnValue( - moduleGraphStub((files) => - files.map(() => [{ storyFile: './src/Button.stories.tsx', depth: 1 }]) - ) - ); - mockExecSync.mockReturnValue(' M src/Button.tsx\n'); - - const response = await callTool(); - const text = getResultText(response); - - expect(text).not.toContain('Coverage gap'); - expect(text).not.toMatch(/coverage sanity check/i); - expect(text.startsWith('Detected')).toBe(true); - }); -}); diff --git a/code/addons/mcp/src/tools/get-changed-stories.ts b/code/addons/mcp/src/tools/get-changed-stories.ts deleted file mode 100644 index 72a77f401692..000000000000 --- a/code/addons/mcp/src/tools/get-changed-stories.ts +++ /dev/null @@ -1,221 +0,0 @@ -import type { McpServer } from 'tmcp'; -import { experimental_getStatusStore } from 'storybook/internal/core-server'; -import { collectTelemetry } from '../telemetry.ts'; -import type { AddonContext } from '../types.ts'; -import { errorToMCPContent } from '../utils/errors.ts'; -import { getStoryIndex } from '../utils/get-story-index.ts'; -import { - detectUnreachableChanges, - formatPartialCoverageBanner, - formatPartialCoverageHint, - formatUnreachableHint, -} from '../utils/detect-unreachable-changes.ts'; -import { GET_CHANGED_STORIES_TOOL_NAME } from './tool-names.ts'; - -export const GET_CHANGED_STORIES_TOOL_DESCRIPTION = `Get Storybook stories marked as new, modified, or related. Returns story metadata only (no URLs). - -The result reflects the cumulative working-tree diff, not just your latest edit — after multiple edits in one session, a non-empty result may cover an earlier sub-change and miss your most recent one. Check that every file you touched is represented; for any that isn't, find its consumer components and pass their paths to get-stories-by-component instead. The response surfaces this gap with a "coverage sanity check" hint when it detects unreachable working-tree files.`; - -const CHANGE_DETECTION_TYPE = 'storybook/change-detection'; -const INCLUDED_STATUS_VALUES = new Set([ - 'status-value:new', - 'status-value:modified', - 'status-value:affected', -]); - -type StatusValue = - | 'status-value:pending' - | 'status-value:success' - | 'status-value:new' - | 'status-value:modified' - | 'status-value:affected' - | 'status-value:warning' - | 'status-value:error' - | 'status-value:unknown'; - -type StatusesByStoryIdAndTypeId = Record>; - -interface Status { - value: StatusValue; - typeId: string; - storyId: string; - title: string; - description: string; - data?: any; - sidebarContextMenu?: boolean; -} - -interface ChangedStory { - storyId: string; - statusValue: StatusValue; - title: string; - name: string; - importPath: string; -} - -function statusPriority(statusValue: StatusValue): number { - if (statusValue === 'status-value:new') return 0; - if (statusValue === 'status-value:modified') return 1; - return 2; -} - -export function getChangedStoriesToolMetadata() { - return { - name: GET_CHANGED_STORIES_TOOL_NAME, - title: 'Get changed stories metadata', - description: GET_CHANGED_STORIES_TOOL_DESCRIPTION, - }; -} - -export async function addGetChangedStoriesTool( - server: McpServer, - enabled: Parameters['tool']>[0]['enabled'] = () => - server.ctx.custom?.toolsets?.dev ?? true, - { reviewEnabled = false }: { reviewEnabled?: boolean } = {} -) { - server.tool( - { - ...getChangedStoriesToolMetadata(), - enabled, - }, - async () => { - try { - const { options, disableTelemetry } = server.ctx.custom ?? {}; - if (!options) { - throw new Error('Storybook options are required in addon context'); - } - - const statusStore = experimental_getStatusStore(CHANGE_DETECTION_TYPE); - const allStatuses = statusStore.getAll() as StatusesByStoryIdAndTypeId; - const changedStoriesFromStatusStore: Status[] = []; - for (const byType of Object.values(allStatuses)) { - const status = byType?.[CHANGE_DETECTION_TYPE]; - if (status?.value && INCLUDED_STATUS_VALUES.has(status.value)) { - changedStoriesFromStatusStore.push(status); - } - } - - if (changedStoriesFromStatusStore.length === 0) { - if (!disableTelemetry) { - await collectTelemetry({ - event: 'tool:getChangedStories', - server, - toolset: 'dev', - storyCount: 0, - newStoryCount: 0, - modifiedStoryCount: 0, - affectedStoryCount: 0, - }); - } - - // Empty result doesn't mean "nothing to review" — files may be - // modified outside the story graph (theme tokens, decorators, - // utilities consumed at preview-runtime). Surface those so the - // agent knows to fall back to grep + get-stories-by-component - // rather than reporting "no impact". - const unreachable = await detectUnreachableChanges(); - const hint = formatUnreachableHint(unreachable); - - return { - content: [ - { - type: 'text' as const, - text: `No new, modified, or related stories detected.${hint}`, - }, - ], - }; - } - - const index = await getStoryIndex(options); - const stories = changedStoriesFromStatusStore.flatMap( - ({ storyId, value }) => { - const entry = index.entries[storyId]; - if (!entry) { - return []; - } - return [ - { - storyId, - statusValue: value, - title: entry.title, - name: entry.name, - importPath: entry.importPath, - }, - ]; - } - ); - - stories.sort((a, b) => { - const priorityDelta = statusPriority(a.statusValue) - statusPriority(b.statusValue); - return priorityDelta !== 0 ? priorityDelta : a.storyId.localeCompare(b.storyId); - }); - - const buckets = { - new: stories.filter((story) => story.statusValue === 'status-value:new'), - modified: stories.filter((story) => story.statusValue === 'status-value:modified'), - affected: stories.filter((story) => story.statusValue === 'status-value:affected'), - }; - const counts = { - new: buckets.new.length, - modified: buckets.modified.length, - affected: buckets.affected.length, - }; - - if (!disableTelemetry) { - await collectTelemetry({ - event: 'tool:getChangedStories', - server, - toolset: 'dev', - storyCount: stories.length, - newStoryCount: counts.new, - modifiedStoryCount: counts.modified, - affectedStoryCount: counts.affected, - }); - } - - // Detect unreachable working-tree files first so the banner can - // front-load the warning. With a long story list (Chromatic-scale) - // the tail-positioned `formatPartialCoverageHint` can land past - // host-side tool-output truncation caps; the leading banner is the - // salience aid that survives. Tail hint also stays for agents that - // read the full response. - const unreachable = await detectUnreachableChanges(); - const banner = formatPartialCoverageBanner(unreachable); - - let text = `${banner}Detected ${stories.length} changed stor${stories.length === 1 ? 'y' : 'ies'} (${counts.new} new, ${counts.modified} modified, ${counts.affected} related).`; - - // Front-loaded like the banner above: host-side tool-output caps - // can cut the tail of a long story list, and this next-step nudge - // is the product fix that keeps agents from ending visual work at - // preview URLs instead of the review. Only when there are story - // IDs to curate — a zero-result is the moment to fall back to - // get-stories-by-component instead. - if (reviewEnabled && stories.length > 0) { - text += `\n\nNext: if the change is visually observable, publish the review now — call **display-review** curating these story IDs. That review link is how you finish; do not substitute individual preview URLs for it.`; - } - - const serializeStory = ({ storyId, title, name, importPath }: ChangedStory) => - `- \`${storyId}\`: ${title} / ${name} (\`${importPath}\`)`; - - if (buckets.new.length > 0) { - text += `\n\nNew stories:\n`; - text += buckets.new.map(serializeStory).join('\n'); - } - if (buckets.modified.length > 0) { - text += `\n\nModified stories:\n`; - text += buckets.modified.map(serializeStory).join('\n'); - } - if (buckets.affected.length > 0) { - text += `\n\nRelated stories:\n`; - text += buckets.affected.map(serializeStory).join('\n'); - } - - text += formatPartialCoverageHint(unreachable); - - return { content: [{ type: 'text' as const, text }] }; - } catch (error) { - return errorToMCPContent(error); - } - } - ); -} diff --git a/code/addons/mcp/src/tools/tool-names.ts b/code/addons/mcp/src/tools/tool-names.ts index 4d20527ab9a7..ce1cd3530c12 100644 --- a/code/addons/mcp/src/tools/tool-names.ts +++ b/code/addons/mcp/src/tools/tool-names.ts @@ -1,10 +1,15 @@ +import { MCP_TOOL_NAMES } from 'storybook/open-service'; + /** - * Tool name constants extracted to avoid circular dependencies. + * Tool names used in this addon's own prose and resource URIs. + * + * Derived from core's frozen map so there is one source of truth for the public MCP contract. The + * instructions tool is the exception: it has no toolset method, so its name lives here. */ +export const PREVIEW_STORIES_TOOL_NAME = MCP_TOOL_NAMES['stories.preview']; +export const GET_CHANGED_STORIES_TOOL_NAME = MCP_TOOL_NAMES['stories.changed']; +export const GET_STORIES_BY_COMPONENT_TOOL_NAME = MCP_TOOL_NAMES['stories.findByComponent']; +export const RUN_STORY_TESTS_TOOL_NAME = MCP_TOOL_NAMES['test.run']; +export const DISPLAY_REVIEW_TOOL_NAME = MCP_TOOL_NAMES['review.create']; -export const PREVIEW_STORIES_TOOL_NAME = 'preview-stories'; -export const GET_CHANGED_STORIES_TOOL_NAME = 'get-changed-stories'; -export const GET_STORIES_BY_COMPONENT_TOOL_NAME = 'get-stories-by-component'; export const GET_UI_BUILDING_INSTRUCTIONS_TOOL_NAME = 'get-storybook-story-instructions'; -export const RUN_STORY_TESTS_TOOL_NAME = 'run-story-tests'; -export const DISPLAY_REVIEW_TOOL_NAME = 'display-review'; diff --git a/code/addons/mcp/src/tools/tool-registry.test.ts b/code/addons/mcp/src/tools/tool-registry.test.ts new file mode 100644 index 000000000000..35ef15624032 --- /dev/null +++ b/code/addons/mcp/src/tools/tool-registry.test.ts @@ -0,0 +1,117 @@ +/** + * Registry-level contracts: what survives when a backing toolset is broken. A regression here was + * invisible to the e2e happy paths, so it is pinned at the adapter seam. How an outcome's tag maps + * onto the MCP `isError` flag is the generic unwrap's contract, pinned in `toolset-tools.test.ts`. + */ + +import { clearToolsetRegistry, defineToolset, registerToolset } from 'storybook/open-service'; +import * as v from 'valibot'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { registerCoreToolsetsForTest } from '../test-support/register-core-toolsets.ts'; +import { getAddonToolMetadata, registerAddonMcpTools } from './tool-registry.ts'; + +const collectTelemetry = vi.hoisted(() => vi.fn()); +vi.mock('../telemetry.ts', () => ({ collectTelemetry })); + +const loggerError = vi.hoisted(() => vi.fn()); +vi.mock('storybook/internal/node-logger', () => ({ + logger: { error: loggerError, warn: vi.fn(), info: vi.fn(), debug: vi.fn() }, +})); + +/** Registers a stub `stories` toolset whose metadata resolution throws the given error. */ +function registerStoriesToolsetThrowing(error: Error) { + registerToolset( + defineToolset({ + id: 'stories', + description: 'stub', + telemetryGroup: 'dev', + methods: { + changed: { + schema: v.object({}), + description: () => { + throw error; + }, + handler: async () => ({ ok: true, data: {}, markdown: '' }), + }, + }, + }) as any + ); +} + +function makeServer() { + const tools = new Map Promise>(); + const server = { + ctx: { custom: { origin: 'http://localhost:6006' }, sessionId: 'session-1' }, + tool: (metadata: { name: string }, handler: (input: unknown) => Promise) => { + tools.set(metadata.name, handler); + }, + resource: () => {}, + } as any; + + return { server, tools }; +} + +describe('a broken tool row', () => { + // The availability gate claims change detection while the `stories` toolset never registered — + // the wiring bug this containment exists for. + const context = { + availability: { changeDetectionEnabled: true, docsEnabled: true, a11yEnabled: false }, + } as never; + + beforeEach(() => { + clearToolsetRegistry(); + loggerError.mockClear(); + }); + + it('is dropped from registration with an error log, keeping every other tool served', async () => { + const { server, tools } = makeServer(); + + await expect(registerAddonMcpTools(server, context)).resolves.toBeUndefined(); + + expect([...tools.keys()]).not.toContain('get-changed-stories'); + expect([...tools.keys()]).toEqual( + expect.arrayContaining(['preview-stories', 'list-all-documentation', 'get-documentation']) + ); + expect(loggerError).toHaveBeenCalledWith(expect.stringContaining('get-changed-stories')); + }); + + it('is dropped from the storybook ai metadata instead of failing the build', () => { + const metadata = getAddonToolMetadata(context); + const names = metadata.map((tool) => tool.name); + + expect(names).not.toContain('get-changed-stories'); + expect(names).toEqual(expect.arrayContaining(['preview-stories', 'list-all-documentation'])); + expect(loggerError).toHaveBeenCalledWith(expect.stringContaining('get-changed-stories')); + }); + + it('contains only the missing-toolset case: any other adapter failure still fails fast', () => { + registerStoriesToolsetThrowing(new Error('broken description')); + + expect(() => getAddonToolMetadata(context)).toThrow('broken description'); + }); +}); + +describe('get-changed-stories over the registry', () => { + beforeEach(() => { + clearToolsetRegistry(); + }); + + // One integration probe through the real `stories` toolset: the row resolves off the registry + // and publishes the frozen name and title an MCP client sees. + it('serves the migrated row once its toolset is registered', async () => { + registerCoreToolsetsForTest(); + const { server, tools } = makeServer(); + + await registerAddonMcpTools(server, { + availability: { changeDetectionEnabled: true, docsEnabled: false, a11yEnabled: false }, + } as never); + + expect(tools.has('get-changed-stories')).toBe(true); + expect( + getAddonToolMetadata({ + availability: { changeDetectionEnabled: true, docsEnabled: false, a11yEnabled: false }, + } as never).find((tool) => tool.name === 'get-changed-stories') + ).toMatchObject({ title: 'Get changed stories metadata' }); + }); +}); diff --git a/code/addons/mcp/src/tools/tool-registry.ts b/code/addons/mcp/src/tools/tool-registry.ts index 55171853616c..11ddad47ee69 100644 --- a/code/addons/mcp/src/tools/tool-registry.ts +++ b/code/addons/mcp/src/tools/tool-registry.ts @@ -15,7 +15,6 @@ import { import type { AddonContext } from '../types.ts'; import type { ToolAvailability } from '../utils/get-tool-availability.ts'; import { getDisplayReviewToolMetadata, addDisplayReviewTool } from './display-review.ts'; -import { getChangedStoriesToolMetadata, addGetChangedStoriesTool } from './get-changed-stories.ts'; import { getStoriesByComponentToolMetadata, addGetStoriesByComponentTool, @@ -27,14 +26,22 @@ import { } from './get-storybook-story-instructions.ts'; import { getPreviewStoriesToolMetadata, addPreviewStoriesTool } from './preview-stories.ts'; import { getRunStoryTestsToolMetadata, addRunStoryTestsTool } from './run-story-tests.ts'; +// The error class must come from the same entry as `getToolset` (which throws it, via +// `toolset-tools.ts`); a copy from another core entry is a different constructor and +// `instanceof` would silently fail. +import { MCP_TOOL_NAMES, OpenServiceMissingToolsetError } from 'storybook/open-service'; import { DISPLAY_REVIEW_TOOL_NAME, - GET_CHANGED_STORIES_TOOL_NAME, GET_STORIES_BY_COMPONENT_TOOL_NAME, GET_UI_BUILDING_INSTRUCTIONS_TOOL_NAME, PREVIEW_STORIES_TOOL_NAME, RUN_STORY_TESTS_TOOL_NAME, } from './tool-names.ts'; +import { + getToolsetToolMetadata, + registerToolsetTool, + type ToolsetToolOptions, +} from './toolset-tools.ts'; export type ToolMetadata = { name: string; @@ -94,6 +101,36 @@ const createToolsetEnabled = () => server.ctx.custom?.toolsets?.[toolset] ?? true; +/** + * Declares an MCP tool that is backed by a core toolset method. + * + * The addon contributes only what is specific to this surface: the MCP toolset grouping, the + * availability gate, and any MCP-only metadata. Name, title, description, schemas, behaviour and + * telemetry all come from the method. + */ +function fromToolset( + definition: Omit & { + options: ToolsetToolOptions; + available?: (context: AddonToolRegistryContext) => boolean; + } +): AddonToolDefinition { + const { options, available, ...rest } = definition; + return { + ...rest, + // Read from the constant, not the registry: this array is built at import time, while toolsets + // register later from their preset hooks. Each availability gate is written to match the + // condition under which its toolset registers; if they still disagree, resolution fails loudly + // (getToolset throws) and the registry drops that one row with an error log rather than taking + // down the whole server (see resolveDefinitionOrDrop). + name: MCP_TOOL_NAMES[options.method], + available: (context) => available?.(context) ?? true, + getMetadata: () => getToolsetToolMetadata(options), + register: async (server, context, enabled) => { + registerToolsetTool(server, options, enabled); + }, + }; +} + const addonToolDefinitions: AddonToolDefinition[] = [ { name: PREVIEW_STORIES_TOOL_NAME, @@ -130,14 +167,11 @@ const addonToolDefinitions: AddonToolDefinition[] = [ }, }), }, - { - name: GET_CHANGED_STORIES_TOOL_NAME, + fromToolset({ toolset: 'dev', available: ({ availability }) => availability.changeDetectionEnabled, - getMetadata: () => getChangedStoriesToolMetadata(), - register: (server, { availability }, enabled) => - addGetChangedStoriesTool(server, enabled, { reviewEnabled: availability.reviewEnabled }), - }, + options: { method: 'stories.changed' }, + }), { name: GET_STORIES_BY_COMPONENT_TOOL_NAME, toolset: 'dev', @@ -208,10 +242,40 @@ const addonToolDefinitions: AddonToolDefinition[] = [ }, ]; +/** + * Logs and drops one tool row when its availability gate said yes but the backing toolset never + * registered. + * + * That mismatch is a wiring bug (each gate is written to match its toolset's registration + * condition), but it must cost the user one tool, not the whole MCP server or the `storybook ai` + * metadata build — the error log keeps it loud. Only this one error is contained: every other + * failure rethrows, so a genuinely broken adapter still fails fast. + */ +function dropRowIfToolsetMissing(name: string, error: unknown): undefined { + if (!(error instanceof OpenServiceMissingToolsetError)) { + throw error; + } + logger.error(`Skipping MCP tool "${name}", its backing toolset is not registered: ${error}`); + return undefined; +} + +function resolveDefinitionOrDrop(name: string, resolve: () => T): T | undefined { + try { + return resolve(); + } catch (error) { + return dropRowIfToolsetMissing(name, error); + } +} + export function getAddonToolMetadata(context: AddonToolRegistryContext): ToolMetadata[] { return addonToolDefinitions .filter((definition) => isMetadataToolEnabled(definition, context)) - .map((definition) => definition.getMetadata(context)); + .flatMap((definition) => { + const metadata = resolveDefinitionOrDrop(definition.name, () => + definition.getMetadata(context) + ); + return metadata ? [metadata] : []; + }); } export function getAddonLocalTools( @@ -221,7 +285,9 @@ export function getAddonLocalTools( addonToolDefinitions .filter((definition) => isMetadataToolEnabled(definition, context)) .flatMap((definition) => { - const localTool = definition.getLocalTool?.(context); + const localTool = resolveDefinitionOrDrop(definition.name, () => + definition.getLocalTool?.(context) + ); return localTool ? [[definition.name, localTool]] : []; }) ); @@ -236,7 +302,15 @@ export async function registerAddonMcpTools( isToolsetEnabled(definition.toolset, context.toolsets) && isToolAvailable(definition, context) ) { - await definition.register(server, context, createToolsetEnabled(server, definition.toolset)); + try { + await definition.register( + server, + context, + createToolsetEnabled(server, definition.toolset) + ); + } catch (error) { + dropRowIfToolsetMissing(definition.name, error); + } } } } diff --git a/code/addons/mcp/src/tools/toolset-tools.test.ts b/code/addons/mcp/src/tools/toolset-tools.test.ts new file mode 100644 index 000000000000..a18b81f6717d --- /dev/null +++ b/code/addons/mcp/src/tools/toolset-tools.test.ts @@ -0,0 +1,237 @@ +import { logger } from 'storybook/internal/node-logger'; +import { clearToolsetRegistry, defineToolset, registerToolset } from 'storybook/open-service'; +import * as v from 'valibot'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { callToolsetMethod, getToolsetToolMetadata } from './toolset-tools.ts'; + +vi.mock('storybook/internal/core-server', () => ({ getService: vi.fn() })); + +const collectTelemetry = vi.hoisted(() => vi.fn()); +vi.mock('../telemetry.ts', () => ({ collectTelemetry })); + +/** + * Stands in for the real `stories` toolset so these tests assert the adapter's contract — what an + * MCP client sees — rather than any one method's behaviour. + */ +function registerStubStoriesToolset( + overrides: { + handler?: (input: any, ctx: any) => any; + } = {} +) { + registerToolset( + defineToolset({ + id: 'stories', + description: 'stub', + telemetryGroup: 'dev', + methods: { + preview: { + schema: v.object({ id: v.string() }), + outputSchema: v.object({ stories: v.array(v.object({ previewUrl: v.string() })) }), + description: (ctx) => `describes ${ctx.consumer}`, + handler: + overrides.handler ?? + (async (input: { id: string }, ctx) => { + await ctx.telemetry?.('tool:previewStories', { inputStoryCount: 1 }); + const stories = [{ previewUrl: `${ctx.origin}/?path=/story/${input.id}` }]; + return { + ok: true, + data: { stories, extraNotInContract: 'internal' }, + markdown: stories.map((story) => story.previewUrl).join('\n'), + }; + }), + }, + }, + }) as any + ); +} + +function makeServer(custom: Record = {}) { + return { + ctx: { custom: { origin: 'http://localhost:6006', ...custom }, sessionId: 'session-1' }, + } as any; +} + +const previewOptions = { method: 'stories.preview' } as const; + +describe('toolset-backed MCP tools', () => { + beforeEach(() => { + clearToolsetRegistry(); + collectTelemetry.mockClear(); + vi.mocked(logger.error).mockClear(); + }); + + afterEach(() => { + clearToolsetRegistry(); + }); + + it('publishes the frozen tool name and title with the mcp-resolved description', () => { + registerStubStoriesToolset(); + + const metadata = getToolsetToolMetadata(previewOptions); + + expect(metadata.name).toBe('preview-stories'); + expect(metadata.title).toBe('Get story preview URLs'); + expect(metadata.description).toBe('describes mcp'); + }); + + it('ships the outcome markdown and narrows structuredContent to the published output schema', async () => { + registerStubStoriesToolset(); + + const result = await callToolsetMethod(makeServer(), previewOptions, { id: 'button--primary' }); + + expect(result.content).toEqual([ + { type: 'text', text: 'http://localhost:6006/?path=/story/button--primary' }, + ]); + expect(result.structuredContent).toEqual({ + stories: [{ previewUrl: 'http://localhost:6006/?path=/story/button--primary' }], + }); + expect(result.isError).toBeUndefined(); + }); + + it('maps a failure outcome to an MCP error result that still carries the markdown', async () => { + registerStubStoriesToolset({ + handler: async () => ({ + ok: false, + data: { stories: [] }, + markdown: 'Component or Docs Entry not found: "nope".', + }), + }); + + const result = await callToolsetMethod(makeServer(), previewOptions, { id: 'nope' }); + + expect(result.isError).toBe(true); + expect(result.content).toEqual([ + { type: 'text', text: 'Component or Docs Entry not found: "nope".' }, + ]); + }); + + it('renders one text block per markdown entry', async () => { + registerStubStoriesToolset({ + handler: async () => ({ + ok: true, + data: { stories: [] }, + markdown: ['http://localhost:6006/one', 'http://localhost:6006/two'], + }), + }); + + const result = await callToolsetMethod(makeServer(), previewOptions, { id: 'x' }); + + expect(result.content).toEqual([ + { type: 'text', text: 'http://localhost:6006/one' }, + { type: 'text', text: 'http://localhost:6006/two' }, + ]); + }); + + it('omits structuredContent when the method publishes no output schema', async () => { + registerToolset( + defineToolset({ + id: 'stories', + description: 'stub', + telemetryGroup: 'dev', + methods: { + changed: { + schema: v.object({}), + description: 'changed', + handler: async () => ({ ok: true, data: { stories: [] }, markdown: 'no changes' }), + }, + }, + }) as any + ); + + const result = await callToolsetMethod(makeServer(), { method: 'stories.changed' }, {}); + + expect(result.content).toEqual([{ type: 'text', text: 'no changes' }]); + expect(result.structuredContent).toBeUndefined(); + }); + + it('runs the handler once, so a method with side effects cannot double-publish', async () => { + const handler = vi.fn(async () => ({ + ok: true, + data: { stories: [{ previewUrl: 'http://localhost:6006/' }] }, + markdown: 'http://localhost:6006/', + })); + registerStubStoriesToolset({ handler }); + + await callToolsetMethod(makeServer(), previewOptions, { id: 'button--primary' }); + + expect(handler).toHaveBeenCalledTimes(1); + }); + + it('selects verbatim surfacing by the agentFacing trait, not an adapter class list', async () => { + // Any error instance carrying the trait qualifies — including one from a different bundle + // copy of a class, which is exactly when an instanceof list would misclassify it. + const traitError = Object.assign(new Error('Do X, then retry the tool.'), { + agentFacing: true, + }); + registerStubStoriesToolset({ + handler: () => { + throw traitError; + }, + }); + + const result = await callToolsetMethod(makeServer(), previewOptions, { id: 'x' }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toBe('Do X, then retry the tool.'); + // A designed answer for the agent is not an incident for the maintainer. + expect(logger.error).not.toHaveBeenCalled(); + }); + + it('wraps every other failure as an error result and logs it server-side', async () => { + registerStubStoriesToolset({ + handler: () => { + throw new Error('boom'); + }, + }); + + const result = await callToolsetMethod(makeServer(), previewOptions, { id: 'x' }); + + expect(result.isError).toBe(true); + expect(result.content[0].text).toBe('Error: boom'); + // Without the log, the only evidence of a bug would be the calling agent's transcript. + expect(logger.error).toHaveBeenCalledTimes(1); + expect(vi.mocked(logger.error).mock.calls[0][0]).toContain('boom'); + }); + + it('forwards method telemetry with the surface fields the adapter owns', async () => { + registerStubStoriesToolset(); + + await callToolsetMethod(makeServer(), previewOptions, { id: 'button--primary' }); + + expect(collectTelemetry).toHaveBeenCalledWith( + expect.objectContaining({ + event: 'tool:previewStories', + toolset: 'dev', + inputStoryCount: 1, + }) + ); + }); + + it('emits no telemetry when the session disabled it', async () => { + registerStubStoriesToolset(); + + await callToolsetMethod(makeServer({ disableTelemetry: true }), previewOptions, { + id: 'button--primary', + }); + + expect(collectTelemetry).not.toHaveBeenCalled(); + }); + + it('derives the uiRoot fact from the request path for every method', async () => { + registerStubStoriesToolset({ + handler: async (_input, ctx) => ({ ok: true, data: { stories: [] }, markdown: ctx.uiRoot }), + }); + + const result = await callToolsetMethod( + makeServer({ + endpoint: '/mcp', + request: new Request('http://localhost:6006/nested/mcp', { method: 'POST' }), + }), + previewOptions, + { id: 'button--primary' } + ); + + expect(result.content[0].text).toBe('http://localhost:6006/nested'); + }); +}); diff --git a/code/addons/mcp/src/tools/toolset-tools.ts b/code/addons/mcp/src/tools/toolset-tools.ts new file mode 100644 index 000000000000..19e4e01cc6fb --- /dev/null +++ b/code/addons/mcp/src/tools/toolset-tools.ts @@ -0,0 +1,170 @@ +/** + * Adapter from core's public toolsets to MCP tools. + * + * Everything an MCP client can observe stays here — the frozen tool names, the transport, the + * session, and the app resource — while the data, prose and telemetry payloads come from the + * toolset the method belongs to. + */ + +import type { StandardSchemaV1 } from '@standard-schema/spec'; +import { getService } from 'storybook/internal/core-server'; +import { logger } from 'storybook/internal/node-logger'; +import { OpenServiceToolsetOutputMismatchError } from 'storybook/internal/server-errors'; +import { + MCP_TOOL_NAMES, + MCP_TOOL_TITLES, + getToolset, + resolveToolsetDescription, + type AnyToolsetDefinition, + type AnyToolsetOutcome, + type ToolsetCtx, + type ToolsetMethod, + type ToolsetMethodRef, +} from 'storybook/open-service'; +import type { McpServer } from 'tmcp'; + +import { collectTelemetry } from '../telemetry.ts'; +import type { AddonContext } from '../types.ts'; +import { errorToMCPContent } from '../utils/errors.ts'; +import { resolveUiRoot } from './ui-root.ts'; +import type { StorybookAiToolCallResult } from './tool-registry.ts'; + +type Server = McpServer; +type ToolEnabled = Parameters[0]['enabled']; + +export type ToolsetToolOptions = { + /** Which toolset method backs this MCP tool. */ + method: ToolsetMethodRef; +}; + +function resolveToolset(options: ToolsetToolOptions): AnyToolsetDefinition { + const [toolsetId] = options.method.split('.'); + return getToolset(toolsetId); +} + +function resolveMethod( + toolset: AnyToolsetDefinition, + options: ToolsetToolOptions +): ToolsetMethod { + const [, methodName] = options.method.split('.'); + return toolset.methods[methodName]; +} + +/** + * Whether an error's prose speaks to the agent and names its own recovery. + * + * The trait is a property read, not a class list: it travels with the instance even across bundle + * copies. + */ +function isAgentFacingError(error: unknown): error is Error { + return error instanceof Error && (error as { agentFacing?: boolean }).agentFacing === true; +} + +/** + * Narrows outcome data to the published output contract. + * + * Outcomes may carry more data than the contract declares (the rendered Markdown needs it); only + * the declared shape reaches `structuredContent`. + */ +async function toStructuredContent( + outputSchema: StandardSchemaV1 | undefined, + data: unknown +): Promise | undefined> { + if (!outputSchema || data === undefined || data === null) { + return undefined; + } + const result = await outputSchema['~standard'].validate(data); + if (result.issues) { + throw new OpenServiceToolsetOutputMismatchError({ issues: result.issues }); + } + return result.value as Record; +} + +function buildContext(server: Server, toolset: AnyToolsetDefinition): ToolsetCtx { + const custom = server.ctx.custom; + return { + consumer: 'mcp', + origin: custom?.origin, + // Derived generically for every method: where this request's Storybook UI is reachable, which + // differs from the origin for a sub-path-hosted dev server. + uiRoot: resolveUiRoot(custom ?? {}), + getService: (serviceId, serviceOptions) => getService(serviceId as any, serviceOptions) as any, + telemetry: custom?.disableTelemetry + ? undefined + : async (event, payload) => { + await collectTelemetry({ + event, + server, + toolset: toolset.telemetryGroup, + ...payload, + }); + }, + }; +} + +/** Runs one toolset method and unwraps its outcome into an MCP tool result. */ +export async function callToolsetMethod( + server: Server, + options: ToolsetToolOptions, + input: unknown +): Promise { + const toolset = resolveToolset(options); + const method = resolveMethod(toolset, options); + const ctx = buildContext(server, toolset); + + try { + const outcome = await method.handler(input as never, ctx); + const structuredContent = await toStructuredContent(method.outputSchema, outcome.data); + const blocks = Array.isArray(outcome.markdown) ? outcome.markdown : [outcome.markdown]; + + return { + content: blocks.map((text) => ({ type: 'text' as const, text })), + ...(structuredContent ? { structuredContent } : {}), + ...(outcome.ok ? {} : { isError: true }), + }; + } catch (error) { + // An agent-facing error is surfaced as-is instead of being wrapped as an unexpected failure. + if (isAgentFacingError(error)) { + return { content: [{ type: 'text', text: error.message }], isError: true }; + } + // Everything else is a bug whose only other evidence would be the calling agent's transcript; + // the terminal is the maintainer's channel. + logger.error(`MCP tool "${MCP_TOOL_NAMES[options.method]}" failed: ${error}`); + return errorToMCPContent(error); + } +} + +/** Metadata for one toolset-backed MCP tool, with the frozen name and title. */ +export function getToolsetToolMetadata(options: ToolsetToolOptions) { + const method = resolveMethod(resolveToolset(options), options); + const descriptionCtx: ToolsetCtx = { + consumer: 'mcp', + getService: (serviceId, serviceOptions) => getService(serviceId as any, serviceOptions) as any, + }; + + // A zero-input method publishes no input schema, matching the hand-written tools it replaced. + const entries = (method.schema as { entries?: Record }).entries; + const hasInput = !entries || Object.keys(entries).length > 0; + + return { + name: MCP_TOOL_NAMES[options.method], + title: MCP_TOOL_TITLES[options.method], + description: resolveToolsetDescription(method.description, descriptionCtx), + ...(hasInput ? { schema: method.schema } : {}), + ...(method.outputSchema ? { outputSchema: method.outputSchema } : {}), + }; +} + +/** Registers one toolset-backed MCP tool. */ +export function registerToolsetTool( + server: Server, + options: ToolsetToolOptions, + enabled: ToolEnabled +): void { + // tmcp types the handler from the literal schema generic; toolset schemas resolve at runtime, + // so both sides step out of that inference. + server.tool( + { ...getToolsetToolMetadata(options), enabled } as never, + ((input: unknown) => callToolsetMethod(server, options, input)) as never + ); +} diff --git a/code/addons/mcp/src/tools/ui-root.ts b/code/addons/mcp/src/tools/ui-root.ts new file mode 100644 index 000000000000..f73116bb425b --- /dev/null +++ b/code/addons/mcp/src/tools/ui-root.ts @@ -0,0 +1,50 @@ +import { DEFAULT_MCP_ENDPOINT } from '../constants.ts'; + +/** + * Derives the Storybook root from the MCP request path. + * + * A Storybook served under a sub-path answers MCP at ``, so links into the UI have + * to be built from the request's own path rather than the bare origin. + */ +function storybookRootFromRequest( + request: Request | undefined, + trustedOrigin: string, + endpoint: string +): string | undefined { + if (!request?.url) { + return undefined; + } + try { + const url = new URL(request.url); + const normalizedEndpoint = endpoint.replace(/\/$/, ''); + const normalizedPathname = url.pathname.replace(/\/$/, ''); + const rootPath = normalizedPathname.endsWith(normalizedEndpoint) + ? normalizedPathname.slice(0, -normalizedEndpoint.length) + : normalizedPathname.replace(/\/[^/]+$/, ''); + return `${trustedOrigin.replace(/\/$/, '')}${rootPath}`; + } catch { + return undefined; + } +} + +/** + * Where this request's Storybook UI is reachable — the `uiRoot` context fact. The HTTP request + * parsing lives here in the adapter; which methods prefer it over `origin` is decided on the + * method definitions. + */ +export function resolveUiRoot(context: { + origin?: string; + request?: Request; + endpoint?: string; +}): string | undefined { + if (!context.origin) { + return undefined; + } + return context.request + ? (storybookRootFromRequest( + context.request, + context.origin, + context.endpoint ?? DEFAULT_MCP_ENDPOINT + ) ?? context.origin) + : context.origin; +} diff --git a/code/addons/mcp/src/utils/detect-unreachable-changes.test.ts b/code/addons/mcp/src/utils/detect-unreachable-changes.test.ts deleted file mode 100644 index a6bdca220814..000000000000 --- a/code/addons/mcp/src/utils/detect-unreachable-changes.test.ts +++ /dev/null @@ -1,234 +0,0 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { - formatPartialCoverageBanner, - formatPartialCoverageHint, - formatUnreachableHint, -} from './detect-unreachable-changes.ts'; -import type { ModuleGraphStatus, ModuleGraphStoryHit } from './module-graph.ts'; - -describe('formatUnreachableHint', () => { - it('returns empty string when there are no unreachable files', () => { - expect(formatUnreachableHint([])).toBe(''); - }); - - it('lists files and tells the agent what to do next', () => { - const out = formatUnreachableHint(['src/styles/theme.ts', 'src/utils/format.ts']); - expect(out).toContain('src/styles/theme.ts'); - expect(out).toContain('src/utils/format.ts'); - expect(out).toMatch(/unreachable/i); - expect(out).toContain('get-stories-by-component'); - expect(out).toMatch(/grep/i); - }); -}); - -describe('formatPartialCoverageHint', () => { - it('returns empty string when there are no unreachable files', () => { - expect(formatPartialCoverageHint([])).toBe(''); - }); - - it('warns about partial coverage and points at get-stories-by-component', () => { - const out = formatPartialCoverageHint(['src/styles/theme.ts']); - expect(out).toContain('src/styles/theme.ts'); - expect(out).toMatch(/coverage sanity check/i); - expect(out).toContain('get-stories-by-component'); - expect(out).toMatch(/never invent/i); - }); - - it('uses different framing from the empty-response hint', () => { - const partial = formatPartialCoverageHint(['src/styles/theme.ts']); - const empty = formatUnreachableHint(['src/styles/theme.ts']); - // The partial-coverage case is specifically about stale-but-non-empty - // responses; the empty-response case is about no results at all. The - // hints must be distinguishable so the agent reacts differently. - expect(partial).not.toBe(empty); - expect(partial).toMatch(/stale/i); - }); -}); - -describe('formatPartialCoverageBanner', () => { - it('returns empty string when there are no unreachable files', () => { - expect(formatPartialCoverageBanner([])).toBe(''); - }); - - it('inlines the file list when 3 or fewer files are flagged', () => { - const out = formatPartialCoverageBanner(['.storybook/main.ts', 'src/server.ts']); - expect(out).toMatch(/^⚠ Coverage gap:/); - expect(out).toContain('2 modified files'); - expect(out).toContain('.storybook/main.ts'); - expect(out).toContain('src/server.ts'); - expect(out).toContain('full sanity-check note at end'); - expect(out.endsWith('\n\n')).toBe(true); - }); - - it('summarises overflow as `+N more` past the inline limit', () => { - const out = formatPartialCoverageBanner(['a.ts', 'b.ts', 'c.ts', 'd.ts', 'e.ts']); - // First three inlined, remainder collapsed. - expect(out).toContain('a.ts, b.ts, c.ts, +2 more'); - expect(out).toContain('5 modified files'); - }); - - it('singularises when exactly one file is unreachable', () => { - const out = formatPartialCoverageBanner(['src/server.ts']); - expect(out).toContain('1 modified file unreachable'); - }); - - it('is short enough to survive realistic tool-output truncation', () => { - // The whole point of the banner is to outlast aggressive truncation / - // compaction passes. Five-file case with realistic-length paths is the - // upper bound we care about; cap at 250 chars so the leading line stays - // in the "definitely-not-dropped" budget of every host we've seen. - const out = formatPartialCoverageBanner([ - '.storybook/main.ts', - 'services/webapp/server.ts', - 'services/webapp/lib/auth.ts', - 'services/webapp/lib/cache.ts', - 'services/webapp/lib/feature-flags.ts', - ]); - expect(out.length).toBeLessThanOrEqual(250); - }); -}); - -describe('detectUnreachableChanges', () => { - beforeEach(() => { - vi.resetModules(); - }); - - afterEach(() => { - vi.resetModules(); - vi.doUnmock('storybook/internal/core-server'); - vi.doUnmock('node:child_process'); - }); - - function mockService(opts: { - status?: ModuleGraphStatus; - /** Maps the batched input files to positional hit lists. */ - storiesForFiles: (files: string[]) => ModuleGraphStoryHit[][]; - }) { - const status = opts.status ?? { value: 'ready' }; - vi.doMock('storybook/internal/core-server', () => ({ - getService: () => ({ - queries: { - status: { loaded: async () => status }, - storiesForFiles: { - loaded: async ({ files }: { files: string[] }) => opts.storiesForFiles(files), - }, - }, - }), - })); - } - - function mockGit(porcelain: string) { - vi.doMock('node:child_process', async (importOriginal) => ({ - ...(await importOriginal()), - execSync: () => porcelain, - })); - } - - it('lists working-tree files that the reverse-graph does not reach', async () => { - mockService({ - // reverse-graph knows about Badge.tsx but NOT theme.ts - storiesForFiles: (files) => - files.map((f) => - f.endsWith('Badge.tsx') ? [{ storyFile: './src/Badge.stories.tsx', depth: 1 }] : [] - ), - }); - mockGit(' M src/styles/theme.ts\n M src/components/Badge/Badge.tsx\n'); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges()).toEqual(['src/styles/theme.ts']); - }); - - it('returns [] when every modified file IS in the graph', async () => { - mockService({ - storiesForFiles: (files) => files.map(() => [{ storyFile: './src/x.stories.tsx', depth: 1 }]), - }); - mockGit(' M src/components/Badge/Badge.tsx\n'); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges()).toEqual([]); - }); - - it('returns [] when the service is not active', async () => { - vi.doMock('storybook/internal/core-server', () => ({ - getService: () => undefined, - })); - mockGit(' M src/styles/theme.ts\n'); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges()).toEqual([]); - }); - - it('returns [] on older Storybook versions that lack the open-service API', async () => { - // Backwards-compat: the `getService` export is missing entirely. - vi.doMock('storybook/internal/core-server', () => ({})); - mockGit(' M src/styles/theme.ts\n'); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges()).toEqual([]); - }); - - it('returns [] when the module graph is not ready', async () => { - mockService({ - status: { value: 'booting' }, - storiesForFiles: (files) => files.map(() => []), - }); - mockGit(' M src/styles/theme.ts\n'); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges()).toEqual([]); - }); - - it('returns [] when the working tree is clean', async () => { - mockService({ storiesForFiles: (files) => files.map(() => []) }); - mockGit(''); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges()).toEqual([]); - }); - - it('ignores non-source files (css, json, lockfiles, …)', async () => { - mockService({ storiesForFiles: (files) => files.map(() => []) }); - mockGit(' M src/styles/theme.css\n M package-lock.json\n M src/styles/theme.ts\n'); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges()).toEqual(['src/styles/theme.ts']); - }); - - it('handles rename lines (`R old -> new`) by keeping the new path', async () => { - mockService({ storiesForFiles: (files) => files.map(() => []) }); - mockGit('R src/old.ts -> src/new.ts\n'); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges()).toEqual(['src/new.ts']); - }); - - it('returns [] gracefully when git fails (not a repo, etc.)', async () => { - mockService({ storiesForFiles: (files) => files.map(() => []) }); - vi.doMock('node:child_process', async (importOriginal) => ({ - ...(await importOriginal()), - execSync: () => { - throw new Error('not a git repository'); - }, - })); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges()).toEqual([]); - }); - - it('caps the list at maxFiles to keep the agent response small', async () => { - mockService({ storiesForFiles: (files) => files.map(() => []) }); - const many = Array.from({ length: 50 }, (_, i) => ` M src/f${i}.ts\n`).join(''); - mockGit(many); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges(5)).toHaveLength(5); - }); - - it('queries in chunks and stops early once maxFiles unreachable files are found', async () => { - // 200 modified files, all unreachable. With a chunk size of 50 and maxFiles=5, the very - // first chunk already satisfies the cap, so we must not query the remaining ~150 files. - const queried: string[] = []; - mockService({ - storiesForFiles: (files) => { - queried.push(...files); - return files.map(() => []); - }, - }); - const many = Array.from({ length: 200 }, (_, i) => ` M src/f${i}.ts\n`).join(''); - mockGit(many); - const { detectUnreachableChanges } = await import('./detect-unreachable-changes.ts'); - expect(await detectUnreachableChanges(5)).toHaveLength(5); - // Only the first chunk should have been queried, not the whole working tree. - expect(queried.length).toBeLessThanOrEqual(50); - }); -}); diff --git a/code/addons/mcp/src/utils/detect-unreachable-changes.ts b/code/addons/mcp/src/utils/detect-unreachable-changes.ts deleted file mode 100644 index 5a10194839d7..000000000000 --- a/code/addons/mcp/src/utils/detect-unreachable-changes.ts +++ /dev/null @@ -1,138 +0,0 @@ -import { execSync } from 'node:child_process'; -import path from 'node:path'; -import { getModuleGraphService } from './module-graph.ts'; -import { slash } from './slash.ts'; - -const SOURCE_EXT_RE = /\.(?:tsx?|jsx?|mjs|cjs)$/i; - -/** - * Module-graph query batch size. Bounds the per-call file list so a large working tree doesn't - * issue one oversized reverse-index query; we early-exit between chunks once `maxFiles` is reached. - */ -const CHUNK_SIZE = 50; - -/** - * git status --porcelain produces lines like `XY filename`, where XY are two - * status characters (M, A, ?, etc). The pattern is rigid; we slice the prefix. - * Rename lines are formatted as `R old -> new` — keep only the new path. - */ -function parsePorcelain(output: string): string[] { - return output - .split('\n') - .filter(Boolean) - .map((line) => { - const rest = line.slice(3); // strip "XY " - const arrow = rest.indexOf(' -> '); - return arrow >= 0 ? rest.slice(arrow + 4).trim() : rest.trim(); - }); -} - -/** - * Lists working-tree modified files that are NOT reached from any story file - * via Storybook's reverse dependency graph. This is the "your edit isn't in - * the graph; you'll need to grep" case — typical for theme tokens, decorator - * config, and other infrastructure files consumed via Storybook's preview - * runtime rather than story-file imports. - * - * Returns workspace-relative paths, suitable for inlining into a tool - * response. Empty when: - * - change detection is inactive, - * - the working tree is clean, - * - or every modified file IS in the graph (the caller should use the - * status-store result directly). - */ -export async function detectUnreachableChanges(maxFiles = 10): Promise { - const moduleGraph = await getModuleGraphService(); - if (!moduleGraph) return []; - const status = await moduleGraph.queries.status.loaded(undefined); - if (status.value !== 'ready') return []; - // Matches what the dev server uses, so paths line up with the reverse-index keys. - const workingDir = process.cwd(); - - let porcelain: string; - try { - porcelain = execSync('git status --porcelain', { - cwd: workingDir, - encoding: 'utf-8', - stdio: ['ignore', 'pipe', 'ignore'], - }); - } catch { - return []; - } - - const relFiles = parsePorcelain(porcelain).filter((f) => SOURCE_EXT_RE.test(f)); - if (relFiles.length === 0) return []; - - // Query in chunks and stop once we've collected `maxFiles` unreachable files, so a huge working - // tree doesn't build an oversized module-graph query for results we'd discard anyway. - const unreachable: string[] = []; - for ( - let start = 0; - start < relFiles.length && unreachable.length < maxFiles; - start += CHUNK_SIZE - ) { - const chunk = relFiles.slice(start, start + CHUNK_SIZE); - // The module graph accepts absolute paths but normalizes them with forward slashes; - // `path.resolve` emits backslashes on Windows, which would miss every key and wrongly flag - // reachable files. - const absChunk = chunk.map((rel) => slash(path.resolve(workingDir, rel))); - // One batched lookup per chunk; output is positional (result `i` corresponds to `absChunk[i]`). - const hits = await moduleGraph.queries.storiesForFiles.loaded({ files: absChunk }); - for (const [i, rel] of chunk.entries()) { - if (unreachable.length >= maxFiles) break; - if ((hits[i]?.length ?? 0) === 0) unreachable.push(rel); - } - } - return unreachable; -} - -/** - * Maximum unreachable files listed inline in the short header banner. Beyond - * this we summarise the overflow as `+N more` to keep the banner one-line-ish. - */ -const BANNER_INLINE_LIMIT = 3; - -/** - * Short "front-loaded" version of {@link formatPartialCoverageHint}, designed - * to survive truncation and compaction. The full hint still trails the - * response body unchanged — this banner is purely a salience aid for the - * non-empty case where 1000+ story bullets would otherwise bury the tail. - * - * Empty when no unreachable files were detected. - */ -export function formatPartialCoverageBanner(unreachable: string[]): string { - if (unreachable.length === 0) return ''; - const fileList = - unreachable.length <= BANNER_INLINE_LIMIT - ? unreachable.join(', ') - : `${unreachable.slice(0, BANNER_INLINE_LIMIT).join(', ')}, +${unreachable.length - BANNER_INLINE_LIMIT} more`; - const noun = unreachable.length === 1 ? 'file' : 'files'; - return `⚠ Coverage gap: ${unreachable.length} modified ${noun} unreachable from any story (${fileList}) — full sanity-check note at end of this response.\n\n`; -} - -/** - * Formats the unreachable-files list into a hint appended to - * `get-changed-stories`' empty response. Empty string when no hint applies. - */ -export function formatUnreachableHint(unreachable: string[]): string { - if (unreachable.length === 0) return ''; - const lines = unreachable.map((f) => `- ${f}`).join('\n'); - return `\n\nThe following working-tree file(s) are modified but unreachable from any story (no static import path connects them — they are likely theme tokens, decorators, or other Storybook-preview-runtime files):\n${lines}\n\nFor these, grep the codebase for their exports (e.g. specific tokens or symbols) to find runtime consumers, then call \`get-stories-by-component\` with those consumer file paths.`; -} - -/** - * Formats a sanity-check hint for the *non-empty* response case: there ARE - * changed stories, but the working tree also contains modified files that - * aren't reachable from any story (typically because the diff bundles a prior - * sub-change on a component with a follow-up edit to shared infrastructure). - * In that situation, the changed-stories list is real but incomplete w.r.t. - * the agent's most recent sub-edit — the agent has to actively check coverage - * rather than trust the list. - * - * Empty string when no hint applies. - */ -export function formatPartialCoverageHint(unreachable: string[]): string { - if (unreachable.length === 0) return ''; - const lines = unreachable.map((f) => `- ${f}`).join('\n'); - return `\n\nCoverage sanity check: the working tree also contains modified file(s) that aren't reachable from any story above (no static import path connects them — typically theme tokens, decorators, or other preview-runtime files):\n${lines}\n\nThe list above is real but may be stale w.r.t. these files — they're often left over from an earlier sub-change in the same diff. Before composing a review, grep the codebase for their exports and call \`get-stories-by-component\` with the runtime consumers' file paths. Do not assume the list above already covers them, and never invent story IDs to fill the gap.`; -} diff --git a/code/core/src/core-server/index.ts b/code/core/src/core-server/index.ts index 96d4ea12447f..f31c58a0dda9 100644 --- a/code/core/src/core-server/index.ts +++ b/code/core/src/core-server/index.ts @@ -58,6 +58,15 @@ export { registerService as experimental_registerService, } from '../shared/open-service/server.ts'; +/** + * Toolset registration for addons that host a public toolset. + * + * Exposed from the Node entry rather than `storybook/open-service`: toolset factories reach + * server-only code, and that entry is built for the browser. + */ +export { registerToolset } from '../shared/open-service/toolset-registry.ts'; +export { createStoriesToolset } from '../shared/open-service/toolsets/stories/definition.ts'; + export { UniversalStore as experimental_UniversalStore } from '../shared/universal-store/index.ts'; export { MockUniversalStore as experimental_MockUniversalStore } from '../shared/universal-store/mock.ts'; export { diff --git a/code/core/src/core-server/presets/common-preset.ts b/code/core/src/core-server/presets/common-preset.ts index 398498b32653..7df1282c4d39 100644 --- a/code/core/src/core-server/presets/common-preset.ts +++ b/code/core/src/core-server/presets/common-preset.ts @@ -19,15 +19,16 @@ import { StoryIndexGenerator } from 'storybook/internal/core-server'; import { loadCsf } from 'storybook/internal/csf-tools'; import { logger } from 'storybook/internal/node-logger'; import { telemetry } from 'storybook/internal/telemetry'; -import type { - CoreConfig, - DocgenProviderDescriptor, - Indexer, - Options, - PresetProperty, - PresetPropertyFn, - StoryDocsProvider, - StorybookConfigRaw, +import { + CHANGE_DETECTION_STATUS_TYPE_ID, + type CoreConfig, + type DocgenProviderDescriptor, + type Indexer, + type Options, + type PresetProperty, + type PresetPropertyFn, + type StoryDocsProvider, + type StorybookConfigRaw, } from 'storybook/internal/types'; import { OpenServiceServicesAppliedTwiceError } from '../../server-errors.ts'; @@ -39,6 +40,9 @@ import { registerStoryDocsService } from '../../shared/open-service/services/sto import { registerToolset } from '../../shared/open-service/toolset-registry.ts'; import { docsToolset } from '../../shared/open-service/toolsets/docs/definition.ts'; import { reviewToolset } from '../../shared/open-service/toolsets/review/definition.ts'; +import { createStoriesToolset } from '../../shared/open-service/toolsets/stories/definition.ts'; +import { GitDiffProvider } from '../change-detection/GitDiffProvider.ts'; +import { getStatusStoreByTypeId } from '../stores/status.ts'; import * as pathe from 'pathe'; import { isAbsolute, join } from 'pathe'; @@ -50,7 +54,7 @@ import { initCreateNewStoryChannel } from '../server-channel/create-new-story-ch import { initFileSearchChannel } from '../server-channel/file-search-channel.ts'; import { initGhostStoriesChannel } from '../server-channel/ghost-stories-channel.ts'; import { initOpenInEditorChannel } from '../server-channel/open-in-editor-channel.ts'; -import { isReviewFeatureEnabled } from '../../shared/review/features.ts'; +import { isReviewExplicitlyEnabled, isReviewFeatureEnabled } from '../../shared/review/features.ts'; import { initReviewChannel } from '../server-channel/review-channel.ts'; import { initTelemetryChannel } from '../server-channel/telemetry-channel.ts'; import { initializeChecklist } from '../utils/checklist.ts'; @@ -367,13 +371,33 @@ export const services = async (_value: void, options: Options): Promise => presets: options.presets, }); - // Toolsets register imperatively alongside their services: addons contribute both from their own - // `services` hook. The stories and test toolsets join once their boot-time dependencies are wired - // (stories factory; test moves to addon-vitest). registerToolset(docsToolset); const features = await options.presets.apply('features'); + // Toolsets register imperatively alongside their services: addons contribute both from their own + // `services` hook. The test toolset registers from addon-vitest, which owns the channel it needs. + const storyIndex = { getIndex: () => storyIndexGenerator.getIndex() }; + const gitDiffProvider = new GitDiffProvider(process.cwd()); + + registerToolset( + createStoriesToolset({ + storyIndex, + git: { + getRepoRoot: () => gitDiffProvider.getRepoRoot(), + getChangedFiles: () => gitDiffProvider.getChangedFiles(), + }, + changeStatuses: { + getAll: () => getStatusStoreByTypeId(CHANGE_DETECTION_STATUS_TYPE_ID).getAll(), + }, + // The explicit opt-in gate, not `isReviewFeatureEnabled`: with the flag unset the review + // infrastructure below still registers (the `storybook ai` CLI channel enables the tool per + // request), but direct MCP clients never see `display-review`, so the stories prose must + // not point at it. + reviewEnabled: isReviewExplicitlyEnabled(features), + }) + ); + if (isReviewFeatureEnabled(features)) { registerReviewService({ getIndex: () => storyIndexGenerator.getIndex(), diff --git a/code/core/src/server-errors.ts b/code/core/src/server-errors.ts index f0aa52a70db7..04773787e422 100644 --- a/code/core/src/server-errors.ts +++ b/code/core/src/server-errors.ts @@ -356,6 +356,45 @@ export class OpenServiceServicesAppliedTwiceError extends StorybookError { } } +export class OpenServiceMissingToolsetError extends StorybookError { + constructor(public data: { toolsetId: string }) { + super({ + name: 'OpenServiceMissingToolsetError', + category: Category.CORE_COMMON, + code: 23, + message: `No registered toolset with id "${data.toolsetId}" exists in this environment.`, + }); + } +} + +export class OpenServiceDuplicateToolsetError extends StorybookError { + constructor(public data: { toolsetId: string }) { + super({ + name: 'OpenServiceDuplicateToolsetError', + category: Category.CORE_COMMON, + code: 24, + message: `A toolset with id "${data.toolsetId}" is already registered. Each public toolset must be registered exactly once.`, + }); + } +} + +/** + * A toolset method returned data its own published `outputSchema` rejects. + * + * Always a bug in the method: the schema is the contract adapters publish to their clients, so the + * mismatch is raised instead of quietly shipping unvalidated data. + */ +export class OpenServiceToolsetOutputMismatchError extends StorybookError { + constructor(public data: { issues: readonly unknown[] }) { + super({ + name: 'OpenServiceToolsetOutputMismatchError', + category: Category.CORE_COMMON, + code: 26, + message: `Toolset output did not match its published output schema: ${JSON.stringify(data.issues)}`, + }); + } +} + export class WebpackMissingStatsError extends StorybookError { constructor() { super({ diff --git a/code/core/src/shared/open-service/index.ts b/code/core/src/shared/open-service/index.ts index 6e1213469514..92b6b18bd89b 100644 --- a/code/core/src/shared/open-service/index.ts +++ b/code/core/src/shared/open-service/index.ts @@ -8,17 +8,36 @@ export { defineService } from './service-definition.ts'; export { seedQueryState } from './query-state.ts'; -export { defineToolset } from './toolset-definition.ts'; +export { defineToolset, resolveToolsetDescription } from './toolset-definition.ts'; export type { AnyToolsetDefinition, + AnyToolsetOutcome, ToolsetConsumer, ToolsetCtx, ToolsetDefinition, - ToolsetFormat, ToolsetGetService, ToolsetMethod, + ToolsetMethodDescription, + ToolsetOutcome, + ToolsetTelemetry, + ToolsetTelemetryGroup, } from './toolset-definition.ts'; -export { getRegisteredToolsets, registerToolset } from './toolset-registry.ts'; +export { getRef, MCP_TOOL_NAMES, MCP_TOOL_TITLES } from './toolset-names.ts'; +export type { ToolsetMethodRef } from './toolset-names.ts'; +export { + clearToolsetRegistry, + getRegisteredToolsets, + getToolset, + registerToolset, +} from './toolset-registry.ts'; +// The errors the registry functions above throw. Catchers must import them from this same entry: +// each core entry bundles its own copy of a class, so an error class imported from another entry +// is a different constructor and `instanceof` silently fails. +export { + OpenServiceDuplicateToolsetError, + OpenServiceMissingToolsetError, +} from '../../server-errors.ts'; +export type { KnownToolsets } from './toolset-types.ts'; export type { DocgenService } from './services/docgen/definition.ts'; export type { DocgenPayload } from './services/docgen/types.ts'; diff --git a/code/core/src/shared/open-service/toolset-definition.test-d.ts b/code/core/src/shared/open-service/toolset-definition.test-d.ts index cd203207fd59..1315b2e7c085 100644 --- a/code/core/src/shared/open-service/toolset-definition.test-d.ts +++ b/code/core/src/shared/open-service/toolset-definition.test-d.ts @@ -1,20 +1,26 @@ import * as v from 'valibot'; import { describe, expectTypeOf, it } from 'vitest'; -import { defineToolset, type ToolsetDefinition } from './index.ts'; +import { + defineToolset, + type ToolsetCtx, + type ToolsetDefinition, + type ToolsetOutcome, +} from './index.ts'; const exampleToolset = defineToolset({ id: 'example', description: 'Example API', + telemetryGroup: 'dev', methods: { greet: { description: 'Greets a person.', schema: v.object({ name: v.string() }), - handler: async ({ name }) => { - expectTypeOf(name).toEqualTypeOf(); - - return `Hello ${name}`; - }, + handler: async ({ name }): Promise> => ({ + ok: true, + data: { greeting: `Hello ${name}` }, + markdown: `Hello ${name}`, + }), }, }, }); @@ -22,35 +28,164 @@ const exampleToolset = defineToolset({ const reviewToolset = defineToolset({ id: 'review', description: 'Create a review', + telemetryGroup: 'dev', methods: { create: { - description: 'Create a review', + description: (ctx) => `Create a review (${ctx.consumer})`, schema: v.object({ title: v.string() }), - handler: async (input, ctx) => { - expectTypeOf(input.title).toEqualTypeOf(); - expectTypeOf(ctx.consumer).toEqualTypeOf<'cli' | 'mcp'>(); - expectTypeOf(ctx.origin).toEqualTypeOf(); - expectTypeOf(ctx.format).toEqualTypeOf<'markdown' | 'json'>(); - expectTypeOf( - ctx.getService<{ ok: true }>('core/review', { internal: true }) - ).toEqualTypeOf<{ - ok: true; - }>(); - - return input.title; - }, + outputSchema: v.object({ title: v.string() }), + handler: async ( + input, + ctx + ): Promise< + ToolsetOutcome<{ title: string; origin?: string }, { title: string; reason: string }> + > => + input.title + ? { ok: true, data: { title: input.title, origin: ctx.origin }, markdown: input.title } + : { + ok: false, + data: { title: input.title, reason: 'missing title' }, + markdown: 'missing title', + }, }, }, }); describe('defineToolset types', () => { - it('preserves method schema output types in handlers', () => { + // Assertions live outside the definition literal: inside it, the first contextual-typing pass + // sees `any`/`unknown` params, so exact-type checks there would report on the wrong pass. + it('types handler input from the method schema', () => { + const greet: ( + input: { name: string }, + context: ToolsetCtx + ) => Promise> = + exampleToolset.methods.greet.handler; + const create: ( + input: { title: string }, + context: ToolsetCtx + ) => Promise< + ToolsetOutcome<{ title: string; origin?: string }, { title: string; reason: string }> + > = reviewToolset.methods.create.handler; + + expectTypeOf(greet).toBeFunction(); + expectTypeOf(create).toBeFunction(); expectTypeOf(exampleToolset).toMatchTypeOf(); - expectTypeOf(exampleToolset.methods.greet.handler).parameter(0).toEqualTypeOf<{ - name: string; - }>(); - expectTypeOf(reviewToolset.methods.create.handler).parameter(0).toEqualTypeOf<{ - title: string; - }>(); + }); + + it('narrows both branches of an outcome on its tag', async () => { + const outcome = await reviewToolset.methods.create.handler( + { title: 'x' }, + { consumer: 'cli', getService: () => ({}) as never } + ); + + if (outcome.ok) { + expectTypeOf(outcome.data).toEqualTypeOf<{ title: string; origin?: string }>(); + } else { + expectTypeOf(outcome.data).toEqualTypeOf<{ title: string; reason: string }>(); + } + }); + + it('makes the failure branch unreachable for infallible methods', async () => { + const outcome = await exampleToolset.methods.greet.handler({ name: 'x' }); + + if (!outcome.ok) { + expectTypeOf(outcome.data).toBeNever(); + } + }); + + it('resolves description functions against the toolset context', () => { + const description: string | ((context: ToolsetCtx) => string) = + reviewToolset.methods.create.description; + + expectTypeOf(description).not.toBeNever(); + }); +}); + +describe('schema-bound outcomes', () => { + // `reviewToolset` above is the acceptance case: its success data carries `origin`, which the + // output schema does not declare — outcome data may be a superset of the published contract. + + it('rejects data that renames a schema-declared field', () => { + defineToolset({ + id: 'renamed-field', + description: 'Rejected', + telemetryGroup: 'dev', + methods: { + create: { + description: 'Renames title to heading.', + schema: v.object({}), + outputSchema: v.object({ title: v.string() }), + // @ts-expect-error — `data` lacks the schema-declared `title` field + handler: async (): Promise> => ({ + ok: true, + data: { heading: 'x' }, + markdown: 'x', + }), + }, + }, + }); + }); + + it('rejects data that drops a schema-declared field', () => { + defineToolset({ + id: 'dropped-field', + description: 'Rejected', + telemetryGroup: 'dev', + methods: { + create: { + description: 'Publishes title but returns nothing.', + schema: v.object({}), + outputSchema: v.object({ title: v.string() }), + // @ts-expect-error — `data` is missing the schema-declared `title` field + handler: async (): Promise, never>> => ({ + ok: true, + data: {}, + markdown: 'x', + }), + }, + }, + }); + }); + + it('rejects data that mistypes a schema-declared field', () => { + defineToolset({ + id: 'mistyped-field', + description: 'Rejected', + telemetryGroup: 'dev', + methods: { + create: { + description: 'Returns a number where the schema declares a string.', + schema: v.object({}), + outputSchema: v.object({ title: v.string() }), + // @ts-expect-error — `title` is a number where the schema declares a string + handler: async (): Promise> => ({ + ok: true, + data: { title: 1 }, + markdown: 'x', + }), + }, + }, + }); + }); + + it('constrains the failure branch to the schema too', () => { + defineToolset({ + id: 'unbound-failure', + description: 'Rejected', + telemetryGroup: 'dev', + methods: { + create: { + description: 'Failure data skips the published contract.', + schema: v.object({}), + outputSchema: v.object({ title: v.string() }), + // @ts-expect-error — failure `data` lacks the schema-declared `title` field + handler: async (): Promise> => ({ + ok: false, + data: { reason: 'x' }, + markdown: 'x', + }), + }, + }, + }); }); }); diff --git a/code/core/src/shared/open-service/toolset-definition.ts b/code/core/src/shared/open-service/toolset-definition.ts index f747a8b68d28..0c6174f0d81c 100644 --- a/code/core/src/shared/open-service/toolset-definition.ts +++ b/code/core/src/shared/open-service/toolset-definition.ts @@ -6,12 +6,6 @@ type AnySchema = StandardSchemaV1; export type ToolsetConsumer = 'cli' | 'mcp'; -/** - * Output representation requested by the adapter. Methods never declare the format themselves; - * adapters own the mapping (CLI `--json` flag, MCP `json` tool input). - */ -export type ToolsetFormat = 'markdown' | 'json'; - /** * Service lookup for toolset handlers. Intentionally not keyed to ServerCoreServices: * toolsets may call core OSA (with `{ internal: true }`) or optional addon services by id. @@ -20,23 +14,95 @@ export type ToolsetGetService = { (serviceId: string, options?: GetServiceOptions): TInstance; }; +/** + * Emits one telemetry event for a toolset method. + * + * Adapters supply the sink so surface-specific fields (MCP session id, client info) stay with the + * adapter while the event name and payload — the part that describes the capability — stay in the + * method. Absent when the consumer has telemetry disabled. + */ +export type ToolsetTelemetry = (event: string, payload: Record) => Promise; + export type ToolsetCtx = { consumer: ToolsetConsumer; /** Storybook server origin. Absent when running from a CLI without a live Storybook. */ origin?: string; - format: ToolsetFormat; + /** + * Where this consumer's Storybook UI is reachable, when that differs from `origin` — a + * sub-path-hosted dev server answers MCP at `` while its UI lives at ``. + * Methods that link into the UI prefer this over `origin`; the adapter derives it from the + * request it is serving. + */ + uiRoot?: string; getService: ToolsetGetService; + telemetry?: ToolsetTelemetry; }; -export type ToolsetMethod = { - description: string; +/** + * A method description, resolved per consumer. + * + * The function form exists because descriptions cross-reference sibling methods, and each surface + * spells those differently (`get-changed-stories` on MCP, `npx storybook tools stories changed` on + * the CLI). Use `getRef(ctx)` to render a reference rather than hardcoding either spelling. + */ +export type ToolsetMethodDescription = string | ((context: ToolsetCtx) => string); + +/** + * The result of one method run: the tag, the structured data, and the rendered Markdown, all from a + * single execution. + * + * The failure model in one line each: could not do the job → throw; did the job and the answer is + * bad news → return `{ ok: false, data, markdown }`. Adapters unwrap mechanically — text blocks + * from `markdown`, `structuredContent` from `data`, MCP `isError` (and later CLI exit codes) from + * `ok` — so everything a method means lives on its definition, never re-derived outside it. + * + * Declare `TFailure = never` for infallible methods; the signature then documents fallibility and + * TypeScript narrows both branches. Return plain object literals: contextual typing against this + * union does the narrowing, no factory helpers needed. + * + * `markdown` may be multiple strings: MCP renders each as its own text block (preview-stories + * renders one block per URL), the CLI joins them with newlines. + */ +export type ToolsetOutcome = + | { readonly ok: true; readonly data: TSuccess; readonly markdown: string | string[] } + | { readonly ok: false; readonly data: TFailure; readonly markdown: string | string[] }; + +// `any` permits heterogeneous outcome maps. Each individual method remains typed by `defineToolset`. +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export type AnyToolsetOutcome = ToolsetOutcome; + +/** + * One public method: description, input schema, optional output schema, and one handler. + * + * The handler produces the whole {@link ToolsetOutcome} — data, side effects, telemetry, and the + * rendered Markdown — because one MCP response carries `content` (text) and `structuredContent` + * (JSON) at once, and both must come from a single run: re-running a method with side effects + * would repeat them. Usage telemetry reports inline in the handler, with the rendered text in + * hand, so no consumer can forget it. + */ +export type ToolsetMethod< + TSchema extends AnySchema = AnySchema, + TOutcome extends AnyToolsetOutcome = AnyToolsetOutcome, +> = { + description: ToolsetMethodDescription; schema: TSchema; - handler: (input: StandardSchemaV1.InferOutput, context: ToolsetCtx) => unknown; + /** Published as the MCP tool's `outputSchema`. Declare it only where the JSON is contractual. */ + outputSchema?: AnySchema; + handler: ( + input: StandardSchemaV1.InferOutput, + context: ToolsetCtx + ) => TOutcome | Promise; }; // `any` permits a heterogeneous method map. Each individual method remains typed by `defineToolset`. // eslint-disable-next-line @typescript-eslint/no-explicit-any -type ToolsetMethods = Record>; +type ToolsetMethods = Record>; + +/** + * Which surface group a toolset's telemetry reports under. Part of the definition — not adapter + * wiring — so the grouping cannot drift between consumers. Stories and review report under `dev`. + */ +export type ToolsetTelemetryGroup = 'dev' | 'test' | 'docs'; export type ToolsetDefinition< TId extends string = string, @@ -44,22 +110,58 @@ export type ToolsetDefinition< > = { id: TId; description: string; + telemetryGroup: ToolsetTelemetryGroup; methods: TMethods; }; export type AnyToolsetDefinition = ToolsetDefinition; -type DefinedToolsetMethods> = { - [TKey in keyof TSchemas]: ToolsetMethod; +/** + * What a handler may return when its method publishes an `outputSchema`: outcomes whose `data` — + * on both branches, since adapters validate failure data into `structuredContent` too — carries at + * least the schema's declared shape. The open record keeps the data-superset pattern legal: the + * rendered Markdown may use fields the public contract does not ship. The schema's *input* type is + * the right side of the contract, because that is what the adapters' runtime validation accepts. + */ +type SchemaBoundData = StandardSchemaV1.InferInput & + Record; + +type MethodOutcomeContract = TMethod extends { outputSchema: infer TOut extends AnySchema } + ? ToolsetOutcome> | Promise>> + : unknown; + +/** + * Second contextual-typing pass for the methods literal: `handler` input comes from that method's + * own `schema`, and its outcome data from the method's `outputSchema` where one is declared — so + * renaming or removing a published field is a compile error at the definition site. Intersecting + * this with the inferred map is what makes the flow work on both the stable and the native + * TypeScript compiler — inferring a separate record does not. + */ +type MethodContracts = { + [TKey in keyof TMethods]: { + handler: ( + input: StandardSchemaV1.InferOutput, + context: ToolsetCtx + ) => MethodOutcomeContract; + }; }; export function defineToolset< const TId extends string, - const TSchemas extends Record, + const TMethods extends ToolsetMethods, >(definition: { id: TId; description: string; - methods: DefinedToolsetMethods; -}): ToolsetDefinition> { + telemetryGroup: ToolsetTelemetryGroup; + methods: TMethods & MethodContracts; +}): ToolsetDefinition { return definition; } + +/** Resolves a method description for one consumer. */ +export function resolveToolsetDescription( + description: ToolsetMethodDescription, + context: ToolsetCtx +): string { + return typeof description === 'function' ? description(context) : description; +} diff --git a/code/core/src/shared/open-service/toolset-names.ts b/code/core/src/shared/open-service/toolset-names.ts new file mode 100644 index 000000000000..a5670c642594 --- /dev/null +++ b/code/core/src/shared/open-service/toolset-names.ts @@ -0,0 +1,59 @@ +import type { ToolsetCtx } from './toolset-definition.ts'; + +/** + * Maps a toolset method to the MCP tool name that exposes it. + * + * These names are the public MCP contract of `@storybook/addon-mcp` and `@storybook/mcp`: clients + * and downstream integrations call them by name, so they are frozen independently of the method + * names they point at. Both adapters register from this map, and descriptions render + * cross-references through {@link getRef}, so description prose and registration cannot disagree. + */ +export const MCP_TOOL_NAMES = { + 'stories.preview': 'preview-stories', + 'stories.changed': 'get-changed-stories', + 'stories.findByComponent': 'get-stories-by-component', + 'test.run': 'run-story-tests', + 'review.create': 'display-review', + 'docs.list': 'list-all-documentation', + 'docs.show': 'get-documentation', + 'docs.showStory': 'get-documentation-for-story', +} as const; + +export type ToolsetMethodRef = keyof typeof MCP_TOOL_NAMES; + +/** + * MCP tool titles, frozen alongside {@link MCP_TOOL_NAMES}. + * + * A title is display metadata for one surface rather than a description of the capability, which is + * why it lives with the names instead of on the method. + */ +export const MCP_TOOL_TITLES: Record = { + 'stories.preview': 'Get story preview URLs', + 'stories.changed': 'Get changed stories metadata', + 'stories.findByComponent': 'Get stories for component files', + 'test.run': 'Storybook Tests', + 'review.create': 'Display Storybook review', + 'docs.list': 'List All Documentation', + 'docs.show': 'Get Documentation', + 'docs.showStory': 'Get Documentation for Story', +}; + +/** `stories.findByComponent` -> `stories find-by-component`. */ +function toCliPath(method: ToolsetMethodRef): string { + const [toolsetId, methodName] = method.split('.'); + const kebabMethod = methodName.replace(/[A-Z]/g, (letter) => `-${letter.toLowerCase()}`); + return `${toolsetId} ${kebabMethod}`; +} + +/** + * Renders a reference to a sibling method in the consumer's own vocabulary. + * + * Descriptions that tell an agent to call another tool must name it the way that agent can call it, + * so never hardcode a tool name or a command in description prose — use this instead. + */ +export function getRef(context: Pick) { + return (method: ToolsetMethodRef): string => + context.consumer === 'mcp' + ? MCP_TOOL_NAMES[method] + : `npx storybook tools ${toCliPath(method)}`; +} diff --git a/code/core/src/shared/open-service/toolset-registry.test.ts b/code/core/src/shared/open-service/toolset-registry.test.ts index 1e8c2e9600c6..f6c2c6e23108 100644 --- a/code/core/src/shared/open-service/toolset-registry.test.ts +++ b/code/core/src/shared/open-service/toolset-registry.test.ts @@ -2,10 +2,15 @@ import { beforeEach, describe, expect, it } from 'vitest'; import * as v from 'valibot'; +import { + OpenServiceDuplicateToolsetError, + OpenServiceMissingToolsetError, +} from '../../server-errors.ts'; import { defineToolset } from './toolset-definition.ts'; import { clearToolsetRegistry, getRegisteredToolsets, + getToolset, registerToolset, } from './toolset-registry.ts'; @@ -13,11 +18,12 @@ const makeToolset = (id: string, description = `${id} toolset`) => defineToolset({ id, description, + telemetryGroup: 'dev', methods: { noop: { description: 'No-op method.', schema: v.object({}), - handler: () => undefined, + handler: () => ({ ok: true, data: undefined, markdown: '' }) as const, }, }, }); @@ -37,17 +43,28 @@ describe('registerToolset', () => { expect(getRegisteredToolsets()).toEqual([docs, review]); }); - it('is idempotent by id: the first registration wins', () => { - const first = makeToolset('docs', 'first'); - const second = makeToolset('docs', 'second'); - - registerToolset(first); - registerToolset(second); + it('throws on a duplicate id: two hosts claiming one public surface is mis-wiring', () => { + registerToolset(makeToolset('docs', 'first')); - expect(getRegisteredToolsets()).toEqual([first]); + expect(() => registerToolset(makeToolset('docs', 'second'))).toThrow( + OpenServiceDuplicateToolsetError + ); }); it('returns an empty list before any registration', () => { expect(getRegisteredToolsets()).toEqual([]); }); }); + +describe('getToolset', () => { + it('returns a registered toolset by id', () => { + const docs = makeToolset('docs'); + registerToolset(docs); + + expect(getToolset('docs')).toBe(docs); + }); + + it('throws for an unregistered id instead of returning an empty result', () => { + expect(() => getToolset('stories')).toThrow(OpenServiceMissingToolsetError); + }); +}); diff --git a/code/core/src/shared/open-service/toolset-registry.ts b/code/core/src/shared/open-service/toolset-registry.ts index f0b7aa4b0e32..4fa51960d1ff 100644 --- a/code/core/src/shared/open-service/toolset-registry.ts +++ b/code/core/src/shared/open-service/toolset-registry.ts @@ -1,4 +1,9 @@ +import { + OpenServiceDuplicateToolsetError, + OpenServiceMissingToolsetError, +} from '../../server-errors.ts'; import type { AnyToolsetDefinition } from './toolset-definition.ts'; +import type { KnownToolsets } from './toolset-types.ts'; const TOOLSET_REGISTRY_SYMBOL = Symbol.for('storybook.open-service.toolset-registry'); @@ -24,23 +29,41 @@ function getToolsetRegistry(): Map { * * Call it from the same place the paired OSA service registers (for core, the `services` preset * hook). Registration is deliberately independent of the Node preset system so manager- or - * preview-realm toolsets can use the same API later. Idempotent by id, mirroring `registerService`: - * a repeated registration is a no-op. + * preview-realm toolsets can use the same API later. + * + * Unlike `registerService`, a repeated id throws instead of being ignored: services re-register + * legitimately (HMR, repeated composition), while every toolset registration site is a one-shot + * preset hook, so a duplicate id can only mean two hosts claimed the same public surface. */ export function registerToolset(toolset: AnyToolsetDefinition): void { const registry = getToolsetRegistry(); - if (!registry.has(toolset.id)) { - registry.set(toolset.id, toolset); + if (registry.has(toolset.id)) { + throw new OpenServiceDuplicateToolsetError({ toolsetId: toolset.id }); } + + registry.set(toolset.id, toolset); } /** - * Returns the registered toolsets in registration order. + * Returns one registered toolset by id, typed for the core toolsets. * - * Adapter-facing: the MCP server (Milestone 4) and the `storybook tools` CLI (Milestone 5) read the - * public tool surface from here; nothing consumes it before then. + * Throws when the id is not registered — an adapter asking for a toolset that no host wired up is + * a configuration error, not an empty result. */ +export function getToolset(toolsetId: TId): KnownToolsets[TId]; +export function getToolset(toolsetId: string): AnyToolsetDefinition; +export function getToolset(toolsetId: string): AnyToolsetDefinition { + const toolset = getToolsetRegistry().get(toolsetId); + + if (!toolset) { + throw new OpenServiceMissingToolsetError({ toolsetId }); + } + + return toolset; +} + +/** Returns the registered toolsets in registration order. */ export function getRegisteredToolsets(): AnyToolsetDefinition[] { return Array.from(getToolsetRegistry().values()); } diff --git a/code/core/src/shared/open-service/toolset-types.ts b/code/core/src/shared/open-service/toolset-types.ts new file mode 100644 index 000000000000..3133ddbe70b4 --- /dev/null +++ b/code/core/src/shared/open-service/toolset-types.ts @@ -0,0 +1,18 @@ +import type { DocsToolset } from './toolsets/docs/definition.ts'; +import type { ReviewToolset } from './toolsets/review/definition.ts'; +import type { StoriesToolset } from './toolsets/stories/definition.ts'; +import type { TestToolset } from './toolsets/test/definition.ts'; + +/** + * The public toolsets core can register, keyed by id. + * + * Mirrors `core-service-types.ts` for services: it is what makes `getToolset('stories')` return a + * typed toolset instead of an opaque definition. A toolset registered by an addon under its own id + * is still resolvable — it falls through to the untyped `getToolset` overload. + */ +export type KnownToolsets = { + docs: DocsToolset; + stories: StoriesToolset; + test: TestToolset; + review: ReviewToolset; +}; diff --git a/code/core/src/shared/open-service/toolsets/docs/definition.test.ts b/code/core/src/shared/open-service/toolsets/docs/definition.test.ts index 4f1f945a283d..998ae9a0185f 100644 --- a/code/core/src/shared/open-service/toolsets/docs/definition.test.ts +++ b/code/core/src/shared/open-service/toolsets/docs/definition.test.ts @@ -45,7 +45,6 @@ beforeEach(() => { ctx = { consumer: 'cli', origin: 'http://localhost:6006', - format: 'markdown', getService: vi.fn((id) => { if (id === 'addon-docs/mdx' && !mdxAvailable) { throw new OpenServiceMissingServiceError({ serviceId: 'addon-docs/mdx' }); @@ -110,13 +109,13 @@ beforeEach(() => { }); describe('docs API', () => { - it('returns the MCP list Markdown by default after loading services through context', async () => { - await expect( - docsToolset.methods.list.handler( - v.parse(docsToolset.methods.list.schema, { withStoryIds: true }), - ctx - ) - ).resolves.toBe( + it('renders the list and its structured data after loading services through context', async () => { + const outcome = await docsToolset.methods.list.handler( + v.parse(docsToolset.methods.list.schema, { withStoryIds: true }), + ctx + ); + + expect(outcome.markdown).toBe( [ '# Components', '', @@ -128,19 +127,7 @@ describe('docs API', () => { '- Guide (guide--docs): Intro', ].join('\n') ); - expect(ctx.getService).toHaveBeenCalledWith('core/docgen', { internal: true }); - expect(ctx.getService).toHaveBeenCalledWith('core/story-docs', { internal: true }); - expect(ctx.getService).toHaveBeenCalledWith('addon-docs/mdx', { internal: true }); - }); - - it('returns structured JSON when the adapter requests it', async () => { - ctx.format = 'json'; - await expect( - docsToolset.methods.list.handler( - v.parse(docsToolset.methods.list.schema, { withStoryIds: true }), - ctx - ) - ).resolves.toEqual({ + expect(outcome.data).toEqual({ components: [ { id: 'button', @@ -151,27 +138,30 @@ describe('docs API', () => { ], docs: [{ id: 'guide--docs', name: 'Guide', title: 'Getting started', summary: 'Intro' }], }); + expect(ctx.getService).toHaveBeenCalledWith('core/docgen', { internal: true }); + expect(ctx.getService).toHaveBeenCalledWith('core/story-docs', { internal: true }); + expect(ctx.getService).toHaveBeenCalledWith('addon-docs/mdx', { internal: true }); }); it('shows component and story documentation via per-id loaders', async () => { - await expect( - docsToolset.methods.show.handler( - v.parse(docsToolset.methods.show.schema, { id: 'button' }), - ctx - ) - ).resolves.toContain('Button'); + const shown = await docsToolset.methods.show.handler( + v.parse(docsToolset.methods.show.schema, { id: 'button' }), + ctx + ); + + expect(shown.markdown).toContain('Button'); expect(docgen).toHaveBeenCalledWith({ id: 'button' }); expect(storyDocs).toHaveBeenCalledWith({ id: 'button' }); - await expect( - docsToolset.methods.showStory.handler( - v.parse(docsToolset.methods.showStory.schema, { - componentId: 'button', - storyName: 'Primary', - }), - ctx - ) - ).resolves.toBe( + const shownStory = await docsToolset.methods.showStory.handler( + v.parse(docsToolset.methods.showStory.schema, { + componentId: 'button', + storyName: 'Primary', + }), + ctx + ); + + expect(shownStory.markdown).toBe( [ '# Button - Primary', '', @@ -187,55 +177,54 @@ describe('docs API', () => { }); it('returns the existing not-found result for unknown ids', async () => { - ctx.format = 'json'; - await expect( - docsToolset.methods.show.handler( - v.parse(docsToolset.methods.show.schema, { id: 'missing' }), - ctx - ) - ).resolves.toEqual({ kind: 'not-found', id: 'missing' }); + const outcome = await docsToolset.methods.show.handler( + v.parse(docsToolset.methods.show.schema, { id: 'missing' }), + ctx + ); + + expect(outcome.data).toEqual({ kind: 'not-found', id: 'missing' }); }); it('shows a standalone docs entry, whose id has no component payloads', async () => { - ctx.format = 'json'; - - await expect( - docsToolset.methods.show.handler( - v.parse(docsToolset.methods.show.schema, { id: 'guide--docs' }), - ctx - ) - ).resolves.toMatchObject({ kind: 'docs', id: 'guide--docs' }); + const outcome = await docsToolset.methods.show.handler( + v.parse(docsToolset.methods.show.schema, { id: 'guide--docs' }), + ctx + ); + + expect(outcome.data).toMatchObject({ kind: 'docs', id: 'guide--docs' }); }); it('mirrors the @storybook/mcp miss messages for the MCP consumer', async () => { ctx.consumer = 'mcp'; - await expect( - docsToolset.methods.show.handler( - v.parse(docsToolset.methods.show.schema, { id: 'missing' }), - ctx - ) - ).resolves.toBe( + const shown = await docsToolset.methods.show.handler( + v.parse(docsToolset.methods.show.schema, { id: 'missing' }), + ctx + ); + + expect(shown.markdown).toBe( 'Component or Docs Entry not found: "missing". Use the list-all-documentation tool to see available components and documentation entries.' ); - await expect( - docsToolset.methods.showStory.handler( - v.parse(docsToolset.methods.showStory.schema, { componentId: 'missing', storyName: 'X' }), - ctx - ) - ).resolves.toBe( + const shownStory = await docsToolset.methods.showStory.handler( + v.parse(docsToolset.methods.showStory.schema, { componentId: 'missing', storyName: 'X' }), + ctx + ); + + expect(shownStory.markdown).toBe( 'Component not found: "missing". Use the list-all-documentation tool to see available components.' ); }); it('continues without MDX when the optional service is unavailable', async () => { - ctx.format = 'json'; mdxAvailable = false; - await expect( - docsToolset.methods.list.handler(v.parse(docsToolset.methods.list.schema, {}), ctx) - ).resolves.toEqual({ + const outcome = await docsToolset.methods.list.handler( + v.parse(docsToolset.methods.list.schema, {}), + ctx + ); + + expect(outcome.data).toEqual({ components: [{ id: 'button', name: 'Button', summary: 'A button' }], docs: [], }); diff --git a/code/core/src/shared/open-service/toolsets/docs/definition.ts b/code/core/src/shared/open-service/toolsets/docs/definition.ts index e7a4af5a74b4..7978a1753280 100644 --- a/code/core/src/shared/open-service/toolsets/docs/definition.ts +++ b/code/core/src/shared/open-service/toolsets/docs/definition.ts @@ -106,6 +106,7 @@ function formatEntryNotFound(id: string, ctx: ToolsetCtx): string { export const docsToolset = defineToolset({ id: 'docs', description: 'Storybook component and docs documentation.', + telemetryGroup: 'docs', methods: { list: { schema: v.object({ @@ -119,15 +120,13 @@ export const docsToolset = defineToolset({ handler: async (input, ctx) => { const { classification, allDocgen, allStoryDocs, allMdx } = await loadDocsListServices(ctx); - if (ctx.format === 'json') { - return mapDocsList({ - classification, - allDocgen, - allStoryDocs, - allMdx, - withStoryIds: input.withStoryIds, - }); - } + const data = mapDocsList({ + classification, + allDocgen, + allStoryDocs, + allMdx, + withStoryIds: input.withStoryIds, + }); // Mirrors the manifest index addon-mcp builds in-process for `list-all-documentation`: // shallow component rows, stories inlined only when story ids are requested. @@ -161,7 +160,11 @@ export const docsToolset = defineToolset({ componentManifest: { v: 1, components }, ...(Object.keys(docs).length > 0 ? { docsManifest: { v: 1, docs } } : {}), }; - return formatManifestsToLists(manifests, { withStoryIds: input.withStoryIds }); + return { + ok: true, + data, + markdown: formatManifestsToLists(manifests, { withStoryIds: input.withStoryIds }), + }; }, }, show: { @@ -185,42 +188,44 @@ export const docsToolset = defineToolset({ allMdx: mdxPayload ? { [input.id]: mdxPayload } : {}, }); - if (ctx.format === 'json') { - return mapDocsShow({ - id: input.id, - classification, - docgen: docgenPayload, - storyDocs: storyDocsPayload, - mdx: mdxPayload, - }); - } + const data = mapDocsShow({ + id: input.id, + classification, + docgen: docgenPayload, + storyDocs: storyDocsPayload, + mdx: mdxPayload, + }); // Mirrors addon-mcp's in-process `resolveEntry`: standalone docs render through the docs // formatter, components assemble docgen + story-docs + attached MDX. if (classification.unattachedDocs.has(input.id)) { const doc = mdxPayload?.docs?.[input.id]; if (!doc) { - return formatEntryNotFound(input.id, ctx); + return { ok: true, data, markdown: formatEntryNotFound(input.id, ctx) }; } - return formatDocsManifest(adaptCoreDoc(doc)); + return { ok: true, data, markdown: formatDocsManifest(adaptCoreDoc(doc)) }; } if (!classification.componentIds.includes(input.id)) { - return formatEntryNotFound(input.id, ctx); + return { ok: true, data, markdown: formatEntryNotFound(input.id, ctx) }; } const docs = selectAttachedDocs(classification, input.id, mdxPayload); - return formatComponentManifest( - adaptCoreComponent({ - ...docgenPayload, - id: input.id, - name: docgenPayload?.name ?? input.id, - ...(storyDocsPayload?.stories ? { stories: storyDocsPayload.stories } : {}), - ...(storyDocsPayload?.import ? { import: storyDocsPayload.import } : {}), - ...(docs ? { docs } : {}), - }) - ); + return { + ok: true, + data, + markdown: formatComponentManifest( + adaptCoreComponent({ + ...docgenPayload, + id: input.id, + name: docgenPayload?.name ?? input.id, + ...(storyDocsPayload?.stories ? { stories: storyDocsPayload.stories } : {}), + ...(storyDocsPayload?.import ? { import: storyDocsPayload.import } : {}), + ...(docs ? { docs } : {}), + }) + ), + }; }, }, showStory: { @@ -239,40 +244,34 @@ export const docsToolset = defineToolset({ loadOptionalComponentPayload(docgen.queries.docgen.loaded({ id: input.componentId })), ]); - if (ctx.format === 'json') { - if (!storyDocsPayload && !docgenPayload) { - return mapDocsShowStory({ + // Mirrors `@storybook/mcp`'s `get-documentation-for-story`, including its miss messages. + if (!storyDocsPayload && !docgenPayload) { + return { + ok: true, + data: mapDocsShowStory({ componentId: input.componentId, storyName: input.storyName, show: { kind: 'not-found', id: input.componentId }, - }); - } - - const stories = storyDocsPayload?.stories - ? mapStoryDocsEntries(storyDocsPayload.stories) - : []; - - const importStatement = resolveImportStatement(storyDocsPayload, docgenPayload); - - return mapDocsShowStory({ - componentId: input.componentId, - storyName: input.storyName, - show: { - kind: 'component', - id: input.componentId, - name: docgenPayload?.name ?? storyDocsPayload?.name ?? input.componentId, - ...(importStatement !== undefined ? { import: importStatement } : {}), - stories, - }, - }); + }), + markdown: + ctx.consumer === 'mcp' + ? `Component not found: "${input.componentId}". Use the list-all-documentation tool to see available components.` + : `Component not found: "${input.componentId}".`, + }; } - // Mirrors `@storybook/mcp`'s `get-documentation-for-story`, including its miss messages. - if (!storyDocsPayload && !docgenPayload) { - return ctx.consumer === 'mcp' - ? `Component not found: "${input.componentId}". Use the list-all-documentation tool to see available components.` - : `Component not found: "${input.componentId}".`; - } + const importStatement = resolveImportStatement(storyDocsPayload, docgenPayload); + const data = mapDocsShowStory({ + componentId: input.componentId, + storyName: input.storyName, + show: { + kind: 'component', + id: input.componentId, + name: docgenPayload?.name ?? storyDocsPayload?.name ?? input.componentId, + ...(importStatement !== undefined ? { import: importStatement } : {}), + stories: storyDocsPayload?.stories ? mapStoryDocsEntries(storyDocsPayload.stories) : [], + }, + }); const component = adaptCoreComponent({ ...docgenPayload, @@ -285,10 +284,18 @@ export const docsToolset = defineToolset({ const story = component.stories?.find((entry) => entry.name === input.storyName); if (!story) { const availableStories = component.stories?.map((entry) => entry.name).join(', '); - return `Story "${input.storyName}" not found for component "${input.componentId}". Available stories: ${availableStories || 'none'}`; + return { + ok: true, + data, + markdown: `Story "${input.storyName}" not found for component "${input.componentId}". Available stories: ${availableStories || 'none'}`, + }; } - return formatStoryDocumentation(component, input.storyName); + return { + ok: true, + data, + markdown: formatStoryDocumentation(component, input.storyName), + }; }, }, }, diff --git a/code/core/src/shared/open-service/toolsets/review/definition.test.ts b/code/core/src/shared/open-service/toolsets/review/definition.test.ts index 3b1adb0f3eb3..6333fa40a649 100644 --- a/code/core/src/shared/open-service/toolsets/review/definition.test.ts +++ b/code/core/src/shared/open-service/toolsets/review/definition.test.ts @@ -43,7 +43,6 @@ describe('review API', () => { ctx = { consumer: 'cli', origin: 'http://localhost:6006/', - format: 'markdown', getService: vi.fn(() => ({ commands: { setReview } })) as ToolsetCtx['getService'], }; }); @@ -61,10 +60,14 @@ describe('review API', () => { await expect(createReview()).rejects.toBe(serviceError); }); - it('sets review state and returns Markdown by default', async () => { - await expect(createReview()).resolves.toBe( - 'Review created: http://localhost:6006/?path=/review/' - ); + it('sets review state and returns the review page URL', async () => { + const outcome = await createReview(); + + expect(outcome).toEqual({ + ok: true, + data: { reviewUrl: 'http://localhost:6006/?path=/review/' }, + markdown: 'Review created: http://localhost:6006/?path=/review/', + }); expect(setReview).toHaveBeenCalledWith(input); expect(ctx.getService).toHaveBeenCalledWith('core/review', { internal: true }); }); @@ -74,18 +77,9 @@ describe('review API', () => { ctx.consumer = 'mcp'; const mcpResult = await createReview(); - expect(cliResult).not.toContain('Show this review URL'); - expect(mcpResult).toBe( - `${cliResult}\n\nShow this review URL to the user in your final response.` + expect(cliResult.markdown).not.toContain('Show this review URL'); + expect(mcpResult.markdown).toBe( + `${cliResult.markdown}\n\nShow this review URL to the user in your final response.` ); }); - - it('returns structured data when the adapter requests JSON', async () => { - ctx.format = 'json'; - - await expect(createReview()).resolves.toEqual({ - reviewUrl: 'http://localhost:6006/?path=/review/', - }); - expect(setReview).toHaveBeenCalledWith(input); - }); }); diff --git a/code/core/src/shared/open-service/toolsets/review/definition.ts b/code/core/src/shared/open-service/toolsets/review/definition.ts index 49c00d3aab0f..c3e238ea3739 100644 --- a/code/core/src/shared/open-service/toolsets/review/definition.ts +++ b/code/core/src/shared/open-service/toolsets/review/definition.ts @@ -17,6 +17,7 @@ const reviewCreateInputSchema = v.object({ export const reviewToolset = defineToolset({ id: 'review', description: 'Create a curated Storybook review.', + telemetryGroup: 'dev', methods: { create: { schema: reviewCreateInputSchema, @@ -34,14 +35,13 @@ export const reviewToolset = defineToolset({ .commands.setReview(review); const reviewUrl = `${ctx.origin.replace(/\/$/, '')}/?path=/review/`; - if (ctx.format === 'json') { - return { reviewUrl }; - } + const created = `Review created: ${reviewUrl}`; + const markdown = + ctx.consumer === 'mcp' + ? `${created}\n\nShow this review URL to the user in your final response.` + : created; - const markdown = `Review created: ${reviewUrl}`; - return ctx.consumer === 'mcp' - ? `${markdown}\n\nShow this review URL to the user in your final response.` - : markdown; + return { ok: true, data: { reviewUrl }, markdown }; }, }, }, diff --git a/code/core/src/shared/open-service/toolsets/stories/definition.test.ts b/code/core/src/shared/open-service/toolsets/stories/definition.test.ts index bf0b469f4ecf..466555a9e076 100644 --- a/code/core/src/shared/open-service/toolsets/stories/definition.test.ts +++ b/code/core/src/shared/open-service/toolsets/stories/definition.test.ts @@ -6,9 +6,9 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'; import * as v from 'valibot'; import { vol } from 'memfs'; -import type { ToolsetCtx } from '../../toolset-definition.ts'; +import { resolveToolsetDescription, type ToolsetCtx } from '../../toolset-definition.ts'; import { CHANGE_DETECTION_STATUS_TYPE_ID } from '../../../status-store/index.ts'; -import { createStoriesToolset } from './definition.ts'; +import { createStoriesToolset, type StoriesToolset } from './definition.ts'; vi.mock('node:fs', { spy: true }); @@ -31,12 +31,19 @@ const repoRoot = '/repo'; const storybookWorkingDir = '/repo/packages/ui'; const componentPath = `${storybookWorkingDir}/src/Button.tsx`; const themePath = `${storybookWorkingDir}/src/theme.ts`; +// Git reports paths relative to the repository root, and the response echoes them in that form. +const changedComponentFile = 'packages/ui/src/Button.tsx'; +const changedThemeFile = 'packages/ui/src/theme.ts'; + +const buttonStoryHit = { storyFile: './src/Button.stories.tsx', depth: 1 }; + const getIndex = vi.fn(); const getChangedFiles = vi.fn(); const getRepoRoot = vi.fn(); const getStatuses = vi.fn(); const graphStatus = vi.fn(); const storiesForFiles = vi.fn(); +const telemetry = vi.fn(); const cwd = vi.spyOn(process, 'cwd'); const moduleGraph = { queries: { @@ -48,16 +55,30 @@ const moduleGraph = { const storyIndex = { getIndex }; const git = { getChangedFiles, getRepoRoot }; const changeStatuses = { getAll: getStatuses }; + let statusesFixture: Record>; let graphMatchesByFile: Map>; -let ctx: ToolsetCtx; +let cliCtx: ToolsetCtx; +let mcpCtx: ToolsetCtx; +let toolset: StoriesToolset; -function createToolset() { - return createStoriesToolset({ - storyIndex, - git, - changeStatuses, - }); +function createToolset({ reviewEnabled = false } = {}): StoriesToolset { + return createStoriesToolset({ storyIndex, git, changeStatuses, reviewEnabled }); +} + +function runChanged(ctx: ToolsetCtx = cliCtx, target: StoriesToolset = toolset) { + return target.methods.changed.handler(v.parse(target.methods.changed.schema, {}), ctx); +} + +/** Marks a changed file as reachable from a story, which keeps it out of `unreachableFiles`. */ +function markReachable(absolutePath: string) { + graphMatchesByFile.set(absolutePath, [buttonStoryHit]); +} + +function markChanged(storyId: string, value: string) { + statusesFixture[storyId] = { + [CHANGE_DETECTION_STATUS_TYPE_ID]: { storyId, value }, + }; } beforeEach(async () => { @@ -69,19 +90,18 @@ beforeEach(async () => { vi.mocked(existsSync).mockImplementation(memfs.fs.existsSync as typeof existsSync); cwd.mockReturnValue(storybookWorkingDir); statusesFixture = {}; - graphMatchesByFile = new Map([ - [componentPath, [{ storyFile: './src/Button.stories.tsx', depth: 1 }]], - ]); - ctx = { + graphMatchesByFile = new Map([[componentPath, [buttonStoryHit]]]); + cliCtx = { consumer: 'cli', origin: 'http://localhost:6006', - format: 'markdown', getService: vi.fn(() => moduleGraph) as ToolsetCtx['getService'], + telemetry, }; + mcpCtx = { ...cliCtx, consumer: 'mcp' }; getIndex.mockResolvedValue(index); getChangedFiles.mockResolvedValue({ - changed: new Set(['packages/ui/src/Button.tsx']), - new: new Set(['packages/ui/src/theme.ts']), + changed: new Set([changedComponentFile]), + new: new Set([changedThemeFile]), }); getRepoRoot.mockResolvedValue(repoRoot); getStatuses.mockImplementation(() => statusesFixture); @@ -89,6 +109,7 @@ beforeEach(async () => { storiesForFiles.mockImplementation(async ({ files }: { files: string[] }) => files.map((file) => graphMatchesByFile.get(file) ?? []) ); + toolset = createToolset(); }); afterAll(() => { @@ -96,18 +117,33 @@ afterAll(() => { vol.reset(); }); -describe('stories API', () => { - it('returns compact Markdown preview URLs by default', async () => { - const storiesToolset = createToolset(); - - await expect( - storiesToolset.methods.preview.handler( - v.parse(storiesToolset.methods.preview.schema, { - stories: [{ storyId: 'button--primary' }], - }), - ctx - ) - ).resolves.toBe( +describe('stories.preview', () => { + it('resolves story ids against the live index', async () => { + const outcome = await toolset.methods.preview.handler( + v.parse(toolset.methods.preview.schema, { stories: [{ storyId: 'button--primary' }] }), + cliCtx + ); + + expect(outcome.ok).toBe(true); + expect(outcome.data).toEqual({ + stories: [ + { + title: 'Button', + name: 'Primary', + previewUrl: 'http://localhost:6006/?path=/story/button--primary', + }, + ], + }); + expect(getIndex).toHaveBeenCalledOnce(); + }); + + it('renders compact Markdown preview URLs', async () => { + const outcome = await toolset.methods.preview.handler( + v.parse(toolset.methods.preview.schema, { stories: [{ storyId: 'button--primary' }] }), + cliCtx + ); + + expect(outcome.markdown).toBe( [ '# Story previews', '- Button - Primary', @@ -115,60 +151,152 @@ describe('stories API', () => { ].join('\n') ); }); +}); + +describe('stories.changed', () => { + it('enriches change-detection statuses and lists unreachable working-tree files', async () => { + markChanged('button--primary', 'status-value:modified'); - it('returns the structured preview result when the adapter requests JSON', async () => { - ctx.format = 'json'; - const storiesToolset = createToolset(); - - await expect( - storiesToolset.methods.preview.handler( - v.parse(storiesToolset.methods.preview.schema, { - stories: [{ storyId: 'button--primary' }], - }), - ctx - ) - ).resolves.toEqual({ + const outcome = await runChanged(); + + expect(outcome.ok).toBe(true); + expect(outcome.data).toEqual({ stories: [ { + storyId: 'button--primary', + statusValue: 'status-value:modified', title: 'Button', name: 'Primary', - previewUrl: 'http://localhost:6006/?path=/story/button--primary', + importPath: './src/Button.stories.tsx', }, ], + counts: { new: 0, modified: 1, affected: 0 }, + unreachableFiles: [changedThemeFile], }); - expect(getIndex).toHaveBeenCalledOnce(); + expect(getStatuses).toHaveBeenCalledOnce(); + expect(cliCtx.getService).toHaveBeenCalledWith('core/module-graph', { internal: true }); }); - it('formats component matches using the module graph from context', async () => { - const storiesToolset = createToolset(); + it('degrades to "no changes detected" when git is unusable, as the pre-toolset tool did', async () => { + getChangedFiles.mockRejectedValue(new Error('not a git repository')); + getRepoRoot.mockRejectedValue(new Error('not a git repository')); - await expect( - storiesToolset.methods.findByComponent.handler( - v.parse(storiesToolset.methods.findByComponent.schema, { componentPaths: [componentPath] }), - ctx - ) - ).resolves.toBe( - [ - '# Stories by component', - `## ${componentPath}`, - '- Button - Primary (button--primary, distance 1)', - ' ./src/Button.stories.tsx', - ].join('\n') - ); - expect(ctx.getService).toHaveBeenCalledWith('core/module-graph', { internal: true }); - expect(storiesForFiles).toHaveBeenCalledWith({ files: [componentPath] }); + const outcome = await runChanged(mcpCtx); + + expect(outcome.data.stories).toEqual([]); + expect(outcome.data.unreachableFiles).toEqual([]); + expect(outcome.markdown).toBe('No new, modified, or related stories detected.'); + }); + + it('anchors Git-relative paths at the repository root, not the Storybook working directory', async () => { + await runChanged(); + + expect(process.cwd()).toBe(storybookWorkingDir); + expect(storiesForFiles).toHaveBeenCalledWith({ files: [componentPath, themePath] }); }); - it('returns structured component matches when the adapter requests JSON', async () => { - ctx.format = 'json'; - const storiesToolset = createToolset(); + it('reports the per-status counts', async () => { + markChanged('button--primary', 'status-value:new'); + + await runChanged(); + + expect(telemetry).toHaveBeenCalledWith('tool:getChangedStories', { + storyCount: 1, + newStoryCount: 1, + modifiedStoryCount: 0, + affectedStoryCount: 0, + }); + }); + + describe('rendering', () => { + it('summarizes counts and unreachable files for the CLI', async () => { + markChanged('button--primary', 'status-value:new'); + const outcome = await runChanged(); + + expect(outcome.markdown).toBe( + [ + '# Changed stories', + 'New: 1, modified: 0, affected: 0', + '- [new] Button - Primary', + '', + '## Unreachable files', + `- ${changedThemeFile}`, + ].join('\n') + ); + }); - await expect( - storiesToolset.methods.findByComponent.handler( - v.parse(storiesToolset.methods.findByComponent.schema, { componentPaths: [componentPath] }), - ctx - ) - ).resolves.toEqual({ + it('buckets stories by status for MCP', async () => { + markChanged('button--primary', 'status-value:new'); + markReachable(themePath); + const outcome = await runChanged(mcpCtx); + + expect(outcome.markdown).toBe( + `Detected 1 changed story (1 new, 0 modified, 0 related). + +New stories: +- \`button--primary\`: Button / Primary (\`./src/Button.stories.tsx\`)` + ); + }); + + it('points MCP at the review tool as the next step when reviews are enabled', async () => { + markChanged('button--primary', 'status-value:new'); + markReachable(themePath); + const withReviews = createToolset({ reviewEnabled: true }); + const outcome = await runChanged(mcpCtx, withReviews); + + expect(outcome.markdown).toBe( + `Detected 1 changed story (1 new, 0 modified, 0 related). + +Next: if the change is visually observable, publish the review now — call **display-review** curating these story IDs. That review link is how you finish; do not substitute individual preview URLs for it. + +New stories: +- \`button--primary\`: Button / Primary (\`./src/Button.stories.tsx\`)` + ); + }); + + it('brackets a non-empty MCP result with a coverage banner and a sanity-check note', async () => { + markChanged('button--primary', 'status-value:new'); + const outcome = await runChanged(mcpCtx); + + expect(outcome.markdown).toBe( + `⚠ Coverage gap: 1 modified file unreachable from any story (${changedThemeFile}) — full sanity-check note at end of this response. + +Detected 1 changed story (1 new, 0 modified, 0 related). + +New stories: +- \`button--primary\`: Button / Primary (\`./src/Button.stories.tsx\`) + +Coverage sanity check: the working tree also contains modified file(s) that aren't reachable from any story above (no static import path connects them — typically theme tokens, decorators, or other preview-runtime files): +- ${changedThemeFile} + +The list above is real but may be stale w.r.t. these files — they're often left over from an earlier sub-change in the same diff. Before composing a review, grep the codebase for their exports and call \`get-stories-by-component\` with the runtime consumers' file paths. Do not assume the list above already covers them, and never invent story IDs to fill the gap.` + ); + }); + + it('tells MCP how to recover when nothing changed but files are unreachable', async () => { + const outcome = await runChanged(mcpCtx); + + expect(outcome.markdown).toBe( + `No new, modified, or related stories detected. + +The following working-tree file(s) are modified but unreachable from any story (no static import path connects them — they are likely theme tokens, decorators, or other Storybook-preview-runtime files): +- ${changedThemeFile} + +For these, grep the codebase for their exports (e.g. specific tokens or symbols) to find runtime consumers, then call \`get-stories-by-component\` with those consumer file paths.` + ); + }); + }); +}); + +describe('stories.findByComponent', () => { + it('returns index-backed matches from the module graph in context', async () => { + const outcome = await toolset.methods.findByComponent.handler( + v.parse(toolset.methods.findByComponent.schema, { componentPaths: [componentPath] }), + cliCtx + ); + + expect(outcome.ok).toBe(true); + expect(outcome.data).toEqual({ results: [ { componentPath, @@ -184,86 +312,37 @@ describe('stories API', () => { }, ], }); + expect(cliCtx.getService).toHaveBeenCalledWith('core/module-graph', { internal: true }); + expect(storiesForFiles).toHaveBeenCalledWith({ files: [componentPath] }); }); - it('returns compact Markdown for changed stories by default', async () => { - statusesFixture = { - 'button--primary': { - [CHANGE_DETECTION_STATUS_TYPE_ID]: { - storyId: 'button--primary', - value: 'status-value:new', - }, - }, - }; - const storiesToolset = createToolset(); - - await expect( - storiesToolset.methods.changed.handler( - v.parse(storiesToolset.methods.changed.schema, {}), - ctx - ) - ).resolves.toBe( + it('renders a headed section per component', async () => { + const outcome = await toolset.methods.findByComponent.handler( + v.parse(toolset.methods.findByComponent.schema, { componentPaths: [componentPath] }), + cliCtx + ); + + expect(outcome.markdown).toBe( [ - '# Changed stories', - 'New: 1, modified: 0, affected: 0', - '- [new] Button - Primary', - '', - '## Unreachable files', - `- ${themePath}`, + '# Stories by component', + `## ${componentPath}`, + '- Button - Primary (button--primary, distance 1)', + ' ./src/Button.stories.tsx', ].join('\n') ); - expect(getStatuses).toHaveBeenCalledOnce(); - expect(getChangedFiles).toHaveBeenCalledOnce(); - expect(getRepoRoot).toHaveBeenCalledOnce(); - expect(ctx.getService).toHaveBeenCalledWith('core/module-graph', { internal: true }); - expect(storiesForFiles).toHaveBeenCalledWith({ - files: [componentPath, themePath], - }); }); +}); - it('anchors Git-relative paths when Storybook runs below the repository root', async () => { - const storiesToolset = createToolset(); - - await storiesToolset.methods.changed.handler( - v.parse(storiesToolset.methods.changed.schema, {}), - ctx +describe('descriptions', () => { + it('names sibling tools the way an MCP client calls them', () => { + expect(resolveToolsetDescription(toolset.methods.changed.description, mcpCtx)).toContain( + 'get-stories-by-component' ); - - expect(process.cwd()).toBe(storybookWorkingDir); - expect(storiesForFiles).toHaveBeenCalledWith({ - files: [componentPath, themePath], - }); }); - it('returns structured changed stories when the adapter requests JSON', async () => { - ctx.format = 'json'; - statusesFixture = { - 'button--primary': { - [CHANGE_DETECTION_STATUS_TYPE_ID]: { - storyId: 'button--primary', - value: 'status-value:modified', - }, - }, - }; - const storiesToolset = createToolset(); - - await expect( - storiesToolset.methods.changed.handler( - v.parse(storiesToolset.methods.changed.schema, {}), - ctx - ) - ).resolves.toEqual({ - stories: [ - { - storyId: 'button--primary', - statusValue: 'status-value:modified', - title: 'Button', - name: 'Primary', - importPath: './src/Button.stories.tsx', - }, - ], - counts: { new: 0, modified: 1, affected: 0 }, - unreachableFiles: [themePath], - }); + it('names sibling tools as CLI commands', () => { + expect(resolveToolsetDescription(toolset.methods.changed.description, cliCtx)).toContain( + 'npx storybook tools stories find-by-component' + ); }); }); diff --git a/code/core/src/shared/open-service/toolsets/stories/definition.ts b/code/core/src/shared/open-service/toolsets/stories/definition.ts index f2e2b85d7048..e4a755fd5423 100644 --- a/code/core/src/shared/open-service/toolsets/stories/definition.ts +++ b/code/core/src/shared/open-service/toolsets/stories/definition.ts @@ -1,17 +1,18 @@ import type { StoryIndex } from 'storybook/internal/types'; -import { resolve as resolvePath } from 'pathe'; import * as v from 'valibot'; import { OpenServiceMissingOriginError } from '../../../../server-errors.ts'; import type { ModuleGraphService } from '../../services/module-graph/definition.ts'; -import { defineToolset } from '../../toolset-definition.ts'; +import { defineToolset, type ToolsetCtx, type ToolsetOutcome } from '../../toolset-definition.ts'; +import { getRef } from '../../toolset-names.ts'; import type { StatusesByStoryIdAndTypeId } from '../../../status-store/index.ts'; import { getChangedStories } from './changed.ts'; import { findStoriesByComponent } from './find-by-component.ts'; import { formatChangedStories, formatFindByComponent, formatPreviewStories } from './format.ts'; import { previewStories } from './preview-stories.ts'; import { storyInputArraySchema, storyInputSchema } from './story-input.ts'; +import { detectUnreachableFiles } from './unreachable-files.ts'; const previewSuccessSchema = v.object({ title: v.string(), @@ -110,17 +111,30 @@ export type CreateStoriesToolsetOptions = { git: StoriesGitAccess; /** Change-detection status snapshot; wired by the server host, not imported from core-server. */ changeStatuses: StoriesChangeStatusesAccess; + /** + * Whether curated reviews are available in this Storybook. Reviews are the intended end of visual + * work, so when they exist several methods steer the agent there instead of at raw preview links. + */ + reviewEnabled?: boolean; }; +function describeChanged(ctx: ToolsetCtx): string { + return `Get Storybook stories marked as new, modified, or related. Returns story metadata only (no URLs). + +The result reflects the cumulative working-tree diff, not just your latest edit — after multiple edits in one session, a non-empty result may cover an earlier sub-change and miss your most recent one. Check that every file you touched is represented; for any that isn't, find its consumer components and pass their paths to ${getRef(ctx)('stories.findByComponent')} instead. The response surfaces this gap with a "coverage sanity check" hint when it detects unreachable working-tree files.`; +} + /** Creates the public stories API with request-local access to Storybook runtime dependencies. */ export function createStoriesToolset({ storyIndex, git, changeStatuses, + reviewEnabled = false, }: CreateStoriesToolsetOptions) { return defineToolset({ id: 'stories', description: 'Story discovery, change detection, and preview URL generation.', + telemetryGroup: 'dev', methods: { preview: { schema: v.object({ @@ -130,7 +144,7 @@ export function createStoriesToolset({ ), }), description: 'Resolves story selectors to preview URLs.', - handler: async (input, ctx) => { + handler: async (input, ctx): Promise> => { const origin = ctx.origin; if (!origin) { throw new OpenServiceMissingOriginError({ @@ -143,39 +157,36 @@ export function createStoriesToolset({ index: await storyIndex.getIndex(), stories: input.stories, }); - return ctx.format === 'json' ? data : formatPreviewStories(data); + return { ok: true, data, markdown: formatPreviewStories(data) }; }, }, changed: { schema: v.object({}), - description: - 'Returns new, modified, and related stories from change detection, plus unreachable working-tree files.', - handler: async (input, ctx) => { + description: describeChanged, + handler: async (_input, ctx): Promise> => { const moduleGraph = ctx.getService('core/module-graph', { internal: true, }); - const [statuses, index, changedFiles, repoRoot] = await Promise.all([ + const [statuses, index] = await Promise.all([ Promise.resolve(changeStatuses.getAll()), storyIndex.getIndex(), - git.getChangedFiles(), - git.getRepoRoot(), ]); - const files = [...new Set([...changedFiles.changed, ...changedFiles.new])].map((file) => - resolvePath(repoRoot, file) - ); - let unreachableFiles: string[] = []; - const status = await moduleGraph.queries.status.loaded(undefined); - if (status.value === 'ready') { - const storiesForFiles = await moduleGraph.queries.storiesForFiles.loaded({ files }); - unreachableFiles = files.filter( - (_file, fileIndex) => storiesForFiles[fileIndex]?.length === 0 - ); - } + const data = { ...getChangedStories({ statuses, index }), - unreachableFiles, + // Files outside the story graph are why an empty or partial result can still be wrong, + // so they are part of the answer rather than a separate lookup. + unreachableFiles: await detectUnreachableFiles({ git, moduleGraph }), }; - return ctx.format === 'json' ? data : formatChangedStories(data); + + await ctx.telemetry?.('tool:getChangedStories', { + storyCount: data.stories.length, + newStoryCount: data.counts.new, + modifiedStoryCount: data.counts.modified, + affectedStoryCount: data.counts.affected, + }); + + return { ok: true, data, markdown: formatChangedStories(data, ctx, { reviewEnabled }) }; }, }, findByComponent: { @@ -191,7 +202,7 @@ export function createStoriesToolset({ ), }), description: 'Finds stories that import the given component paths via the module graph.', - handler: async (input, ctx) => { + handler: async (input, ctx): Promise> => { const moduleGraph = ctx.getService('core/module-graph', { internal: true, }); @@ -202,7 +213,7 @@ export function createStoriesToolset({ index, moduleGraph, }); - return ctx.format === 'json' ? data : formatFindByComponent(data); + return { ok: true, data, markdown: formatFindByComponent(data) }; }, }, }, diff --git a/code/core/src/shared/open-service/toolsets/stories/format.ts b/code/core/src/shared/open-service/toolsets/stories/format.ts index c3eee4b44ac4..03a0c5969704 100644 --- a/code/core/src/shared/open-service/toolsets/stories/format.ts +++ b/code/core/src/shared/open-service/toolsets/stories/format.ts @@ -1,9 +1,15 @@ +import type { ToolsetCtx } from '../../toolset-definition.ts'; +import { getRef } from '../../toolset-names.ts'; import type { ChangedStoriesOutput, FindByComponentOutput, PreviewStoriesOutput, } from './definition.ts'; +function pluralize(count: number, singular: string, plural = `${singular}s`): string { + return count === 1 ? singular : plural; +} + /** Formats preview URLs as compact Markdown for humans and agents. */ export function formatPreviewStories({ stories }: PreviewStoriesOutput): string { const lines = ['# Story previews']; @@ -19,28 +25,104 @@ export function formatPreviewStories({ stories }: PreviewStoriesOutput): string return lines.join('\n'); } -/** Formats changed stories as compact Markdown for humans and agents. */ -export function formatChangedStories({ - stories, - counts, - unreachableFiles, -}: ChangedStoriesOutput): string { - const lines = [ - '# Changed stories', - `New: ${counts.new}, modified: ${counts.modified}, affected: ${counts.affected}`, - ]; +const BANNER_INLINE_LIMIT = 3; - for (const story of stories) { - lines.push( - `- [${story.statusValue.replace('status-value:', '')}] ${story.title} - ${story.name}` - ); +/** + * Front-loaded coverage warning. + * + * Duplicates {@link formatPartialCoverageHint}'s information on purpose: with a long story list the + * tail hint can land past a host's tool-output truncation cap, and the leading banner is the part + * that survives. + */ +function formatPartialCoverageBanner(unreachable: string[]): string { + if (unreachable.length === 0) { + return ''; } + const fileList = + unreachable.length <= BANNER_INLINE_LIMIT + ? unreachable.join(', ') + : `${unreachable.slice(0, BANNER_INLINE_LIMIT).join(', ')}, +${unreachable.length - BANNER_INLINE_LIMIT} more`; + return `⚠ Coverage gap: ${unreachable.length} modified ${pluralize(unreachable.length, 'file')} unreachable from any story (${fileList}) — full sanity-check note at end of this response.\n\n`; +} - if (unreachableFiles.length > 0) { - lines.push('', '## Unreachable files', ...unreachableFiles.map((file) => `- ${file}`)); +/** Hint appended to an empty changed-stories response. */ +function formatUnreachableHint(unreachable: string[], ctx: ToolsetCtx): string { + if (unreachable.length === 0) { + return ''; } + const lines = unreachable.map((file) => `- ${file}`).join('\n'); + return `\n\nThe following working-tree file(s) are modified but unreachable from any story (no static import path connects them — they are likely theme tokens, decorators, or other Storybook-preview-runtime files):\n${lines}\n\nFor these, grep the codebase for their exports (e.g. specific tokens or symbols) to find runtime consumers, then call \`${getRef(ctx)('stories.findByComponent')}\` with those consumer file paths.`; +} - return lines.join('\n'); +/** + * Hint for the non-empty case: the changed list is real but may be stale with respect to files the + * diff also touched that no story reaches, so the agent has to check coverage rather than trust it. + */ +function formatPartialCoverageHint(unreachable: string[], ctx: ToolsetCtx): string { + if (unreachable.length === 0) { + return ''; + } + const lines = unreachable.map((file) => `- ${file}`).join('\n'); + return `\n\nCoverage sanity check: the working tree also contains modified file(s) that aren't reachable from any story above (no static import path connects them — typically theme tokens, decorators, or other preview-runtime files):\n${lines}\n\nThe list above is real but may be stale w.r.t. these files — they're often left over from an earlier sub-change in the same diff. Before composing a review, grep the codebase for their exports and call \`${getRef(ctx)('stories.findByComponent')}\` with the runtime consumers' file paths. Do not assume the list above already covers them, and never invent story IDs to fill the gap.`; +} + +export function formatChangedStories( + { stories, counts, unreachableFiles }: ChangedStoriesOutput, + ctx: ToolsetCtx, + { reviewEnabled = false }: { reviewEnabled?: boolean } = {} +): string { + if (ctx.consumer !== 'mcp') { + const lines = [ + '# Changed stories', + `New: ${counts.new}, modified: ${counts.modified}, affected: ${counts.affected}`, + ...stories.map( + (story) => + `- [${story.statusValue.replace('status-value:', '')}] ${story.title} - ${story.name}` + ), + ]; + if (unreachableFiles.length > 0) { + lines.push('', '## Unreachable files', ...unreachableFiles.map((file) => `- ${file}`)); + } + return lines.join('\n'); + } + + if (stories.length === 0) { + return `No new, modified, or related stories detected.${formatUnreachableHint(unreachableFiles, ctx)}`; + } + + const buckets = { + new: stories.filter((story) => story.statusValue === 'status-value:new'), + modified: stories.filter((story) => story.statusValue === 'status-value:modified'), + affected: stories.filter((story) => story.statusValue === 'status-value:affected'), + }; + + let text = `${formatPartialCoverageBanner(unreachableFiles)}Detected ${stories.length} changed stor${pluralize(stories.length, 'y', 'ies')} (${counts.new} new, ${counts.modified} modified, ${counts.affected} related).`; + + // Front-loaded like the banner: host-side output caps can cut the tail of a long story list, and + // this next step is what keeps agents from ending visual work at preview URLs. + if (reviewEnabled) { + text += `\n\nNext: if the change is visually observable, publish the review now — call **${getRef(ctx)('review.create')}** curating these story IDs. That review link is how you finish; do not substitute individual preview URLs for it.`; + } + + const serializeStory = ({ + storyId, + title, + name, + importPath, + }: ChangedStoriesOutput['stories'][number]) => + `- \`${storyId}\`: ${title} / ${name} (\`${importPath}\`)`; + + if (buckets.new.length > 0) { + text += `\n\nNew stories:\n${buckets.new.map(serializeStory).join('\n')}`; + } + if (buckets.modified.length > 0) { + text += `\n\nModified stories:\n${buckets.modified.map(serializeStory).join('\n')}`; + } + if (buckets.affected.length > 0) { + text += `\n\nRelated stories:\n${buckets.affected.map(serializeStory).join('\n')}`; + } + + return text + formatPartialCoverageHint(unreachableFiles, ctx); } /** Formats component-to-story matches as compact Markdown for humans and agents. */ diff --git a/code/core/src/shared/open-service/toolsets/stories/unreachable-files.ts b/code/core/src/shared/open-service/toolsets/stories/unreachable-files.ts new file mode 100644 index 000000000000..30429f0f8d1d --- /dev/null +++ b/code/core/src/shared/open-service/toolsets/stories/unreachable-files.ts @@ -0,0 +1,78 @@ +import { logger } from 'storybook/internal/node-logger'; + +import { resolve as resolvePath } from 'pathe'; + +import type { ModuleGraphService } from '../../services/module-graph/definition.ts'; +import type { StoriesGitAccess } from './definition.ts'; + +const SOURCE_EXT_RE = /\.(?:tsx?|jsx?|mjs|cjs)$/i; + +/** + * Module-graph query batch size. Bounds the per-call file list so a large working tree doesn't + * issue one oversized reverse-index query; we early-exit between chunks once `maxFiles` is reached. + */ +const CHUNK_SIZE = 50; + +/** Cap on files reported inline, so a large working tree can't dominate the response. */ +const DEFAULT_MAX_FILES = 10; + +/** + * Lists working-tree files that no story reaches through the import graph. + * + * This is the "your edit isn't in the graph; you'll need to grep" case — typical for theme tokens, + * decorator config, and other files consumed through Storybook's preview runtime rather than a + * story-file import. Paths stay repo-root-relative because they are shown to an agent that will + * grep for them. + */ +export async function detectUnreachableFiles({ + git, + moduleGraph, + maxFiles = DEFAULT_MAX_FILES, +}: { + git: StoriesGitAccess; + moduleGraph: ModuleGraphService; + maxFiles?: number; +}): Promise { + const status = await moduleGraph.queries.status.loaded(undefined); + if (status.value !== 'ready') { + return []; + } + + let changedFiles: Awaited>; + let repoRoot: string; + try { + [changedFiles, repoRoot] = await Promise.all([git.getChangedFiles(), git.getRepoRoot()]); + } catch (error) { + // Not a git repository, or git itself is unusable. Change detection legitimately answers + // "no changes detected" here — the pre-toolset tool degraded the same way — so the failure + // must not turn the whole tool into an error for the agent. + logger.debug(`Unreachable-file detection skipped, git is unavailable: ${error}`); + return []; + } + const relativeFiles = [...new Set([...changedFiles.changed, ...changedFiles.new])].filter( + (file) => SOURCE_EXT_RE.test(file) + ); + + const unreachable: string[] = []; + for ( + let start = 0; + start < relativeFiles.length && unreachable.length < maxFiles; + start += CHUNK_SIZE + ) { + const chunk = relativeFiles.slice(start, start + CHUNK_SIZE); + // One batched lookup per chunk; the result is positional. + const hits = await moduleGraph.queries.storiesForFiles.loaded({ + files: chunk.map((file) => resolvePath(repoRoot, file)), + }); + for (const [position, file] of chunk.entries()) { + if (unreachable.length >= maxFiles) { + break; + } + if ((hits[position]?.length ?? 0) === 0) { + unreachable.push(file); + } + } + } + + return unreachable; +} diff --git a/code/core/src/shared/open-service/toolsets/test/definition.test.ts b/code/core/src/shared/open-service/toolsets/test/definition.test.ts index 18236d767591..f8609a236b21 100644 --- a/code/core/src/shared/open-service/toolsets/test/definition.test.ts +++ b/code/core/src/shared/open-service/toolsets/test/definition.test.ts @@ -3,7 +3,6 @@ import type { StoryIndex } from 'storybook/internal/types'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import * as v from 'valibot'; -import type { ToolsetCtx } from '../../toolset-definition.ts'; import type { TestRunOutput } from './definition.ts'; import { createTestToolset } from './definition.ts'; import { runStoryTests } from './run.ts'; @@ -13,13 +12,6 @@ vi.mock('./run.ts', { spy: true }); const index = { v: 5, entries: {} } as StoryIndex; const getIndex = vi.fn(); const storyIndex = { getIndex }; -const ctx = { - consumer: 'cli', - origin: 'http://localhost:6006', - format: 'json', - getService: vi.fn() as ToolsetCtx['getService'], -} satisfies ToolsetCtx; - const completedRun: TestRunOutput = { status: 'completed', result: { @@ -45,23 +37,24 @@ beforeEach(() => { }); describe('test API', () => { - it('returns a useful Markdown summary by default', async () => { + it('renders a run summary alongside the structured result', async () => { const channel = {} as never; const testToolset = createTestToolset({ channel, storyIndex }); - await expect( - testToolset.methods.run.handler(v.parse(testToolset.methods.run.schema, {}), { - ...ctx, - format: 'markdown', - }) - ).resolves.toBe( - [ + const outcome = await testToolset.methods.run.handler( + v.parse(testToolset.methods.run.schema, {}) + ); + + expect(outcome).toEqual({ + ok: true, + data: completedRun, + markdown: [ '# Test run completed', '- Total tests: 3', '- Component tests: 2 passed, 0 failed', '- Accessibility tests: 1 passed, 0 warnings, 0 failed', - ].join('\n') - ); + ].join('\n'), + }); expect(runStoryTests).toHaveBeenCalledWith({ channel, getIndex, @@ -70,14 +63,6 @@ describe('test API', () => { }); }); - it('returns the existing TestRunOutput when the adapter requests JSON', async () => { - const testToolset = createTestToolset({ channel: {} as never, storyIndex }); - - await expect( - testToolset.methods.run.handler(v.parse(testToolset.methods.run.schema, {}), ctx) - ).resolves.toEqual(completedRun); - }); - it('serializes concurrent test runs for one API registration', async () => { const testToolset = createTestToolset({ channel: {} as never, storyIndex }); let completePendingRun!: () => void; @@ -86,16 +71,16 @@ describe('test API', () => { }); const input = v.parse(testToolset.methods.run.schema, {}); - const firstRun = testToolset.methods.run.handler(input, ctx); + const firstRun = testToolset.methods.run.handler(input); await vi.waitFor(() => expect(runStoryTests).toHaveBeenCalledOnce()); - const secondRun = testToolset.methods.run.handler(input, ctx); + const secondRun = testToolset.methods.run.handler(input); await Promise.resolve(); expect(runStoryTests).toHaveBeenCalledOnce(); completePendingRun(); await firstRun; - await expect(secondRun).resolves.toEqual(completedRun); + await expect(secondRun).resolves.toMatchObject({ ok: true, data: completedRun }); expect(runStoryTests).toHaveBeenCalledTimes(2); }); }); diff --git a/code/core/src/shared/open-service/toolsets/test/definition.ts b/code/core/src/shared/open-service/toolsets/test/definition.ts index 7fab504ea9d9..e690f73e6398 100644 --- a/code/core/src/shared/open-service/toolsets/test/definition.ts +++ b/code/core/src/shared/open-service/toolsets/test/definition.ts @@ -1,6 +1,6 @@ import * as v from 'valibot'; -import { defineToolset } from '../../toolset-definition.ts'; +import { defineToolset, type ToolsetOutcome } from '../../toolset-definition.ts'; import type { StoryIndexAccess } from '../stories/definition.ts'; import { storyInputArraySchema } from '../stories/story-input.ts'; import { formatTestRun } from './format.ts'; @@ -97,6 +97,7 @@ export function createTestToolset({ channel, storyIndex }: CreateTestToolsetOpti return defineToolset({ id: 'test', description: 'Run Storybook story tests via addon-vitest.', + telemetryGroup: 'test', methods: { run: { schema: v.object({ @@ -116,7 +117,7 @@ export function createTestToolset({ channel, storyIndex }: CreateTestToolsetOpti }), description: 'Runs story tests for the given selectors, or all stories when stories is omitted.', - handler: async (input, ctx) => { + handler: async (input): Promise> => { const done = await queue.wait(); try { const result = await runStoryTests({ @@ -125,7 +126,7 @@ export function createTestToolset({ channel, storyIndex }: CreateTestToolsetOpti stories: input.stories, a11y: input.a11y, }); - return ctx.format === 'json' ? result : formatTestRun(result); + return { ok: true, data: result, markdown: formatTestRun(result) }; } finally { done(); } diff --git a/code/core/src/shared/review/features.test.ts b/code/core/src/shared/review/features.test.ts index 739b6dc83c5b..b1a5a9913261 100644 --- a/code/core/src/shared/review/features.test.ts +++ b/code/core/src/shared/review/features.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it } from 'vitest'; import type { StorybookFeatures } from '../../types/modules/core-common.ts'; import { features as defaultFeaturesPreset } from '../../core-server/presets/common-preset.ts'; -import { isReviewFeatureEnabled } from './features.ts'; +import { isReviewExplicitlyEnabled, isReviewFeatureEnabled } from './features.ts'; describe('isReviewFeatureEnabled', () => { it('is enabled with the untouched default features preset', async () => { @@ -40,3 +40,25 @@ describe('isReviewFeatureEnabled', () => { expect(isReviewFeatureEnabled(undefined)).toBe(false); }); }); + +describe('isReviewExplicitlyEnabled', () => { + // The flag-unset default is where the two gates differ, and prose gated on the wrong one + // advertises `display-review` to MCP clients that cannot see it: the infrastructure gate is on + // by default, the direct-client gate only on explicit opt-in. + it('stays off when the flag is unset, unlike the infrastructure gate', () => { + const defaults = { changeDetection: true }; + expect(isReviewFeatureEnabled(defaults)).toBe(true); + expect(isReviewExplicitlyEnabled(defaults)).toBe(false); + }); + + it('is enabled only on explicit opt-in with change detection', () => { + expect(isReviewExplicitlyEnabled({ changeDetection: true, experimentalReview: true })).toBe( + true + ); + expect(isReviewExplicitlyEnabled({ changeDetection: true, experimentalReview: false })).toBe( + false + ); + expect(isReviewExplicitlyEnabled({ experimentalReview: true })).toBe(false); + expect(isReviewExplicitlyEnabled(undefined)).toBe(false); + }); +}); diff --git a/code/core/src/shared/review/features.ts b/code/core/src/shared/review/features.ts index 18638eadda73..223a3152987f 100644 --- a/code/core/src/shared/review/features.ts +++ b/code/core/src/shared/review/features.ts @@ -16,3 +16,14 @@ import type { StorybookFeatures } from '../../types/modules/core-common.ts'; */ export const isReviewFeatureEnabled = (features: StorybookFeatures | undefined): boolean => features?.experimentalReview !== false && !!features?.changeDetection; + +/** + * Whether review is available to direct MCP clients: the explicit `experimentalReview` opt-in. + * + * Contrast with {@link isReviewFeatureEnabled}: with the flag unset the review infrastructure + * still mounts (the `storybook ai` CLI channel turns review on per request), but direct MCP + * clients never see `display-review` — so tool descriptions and result prose must not advertise + * it. Mirrors `getReviewStatus().available` in `@storybook/addon-mcp`. + */ +export const isReviewExplicitlyEnabled = (features: StorybookFeatures | undefined): boolean => + !!features?.experimentalReview && !!features?.changeDetection; diff --git a/code/core/src/storybook-error.ts b/code/core/src/storybook-error.ts index 6fe3d142e022..f78a879d1e42 100644 --- a/code/core/src/storybook-error.ts +++ b/code/core/src/storybook-error.ts @@ -50,6 +50,14 @@ export abstract class StorybookError extends Error { /** Flag used to easily determine if the error originates from Storybook. */ readonly fromStorybook: true = true as const; + /** + * Marks an error whose message is written for the agent that triggered the call and names its + * own recovery. Adapters surface such errors verbatim instead of wrapping them as unexpected + * failures. A property rather than a class list, so the trait travels with the instance across + * bundle copies where `instanceof` would misclassify. + */ + public readonly agentFacing: boolean; + /** * Flag used to determine if the error is handled by us and should therefore not be shown to the * user. @@ -98,6 +106,8 @@ export abstract class StorybookError extends Error { cause?: unknown; documentation?: boolean | string | string[]; isHandledError?: boolean; + /** See {@link StorybookError.agentFacing}. Defaults to false. */ + agentFacing?: boolean; name: string; /** * Optional array of sub-errors that are related to this error. When this error is sent to @@ -113,6 +123,7 @@ export abstract class StorybookError extends Error { this.documentation = props.documentation ?? false; this.code = props.code; this.isHandledError = props.isHandledError ?? false; + this.agentFacing = props.agentFacing ?? false; this.name = props.name; this.subErrors = props.subErrors ?? []; } diff --git a/scripts/verdaccio.yaml b/scripts/verdaccio.yaml index ca2064f8f25a..1854b60bfc41 100644 --- a/scripts/verdaccio.yaml +++ b/scripts/verdaccio.yaml @@ -186,14 +186,6 @@ packages: access: $all publish: $all proxy: npmjs - '@storybook/addon-mcp': - access: $all - publish: $all - proxy: npmjs - '@storybook/mcp': - access: $all - publish: $all - proxy: npmjs # storybook packages are NOT proxied to global registry # allowing us to republish any version during tests diff --git a/test-storybooks/mcp/tests/helpers.ts b/test-storybooks/mcp/tests/helpers.ts index e4cfa3606260..322419a3f201 100644 --- a/test-storybooks/mcp/tests/helpers.ts +++ b/test-storybooks/mcp/tests/helpers.ts @@ -19,6 +19,21 @@ export async function parseMCPResponse(response: Response) { return JSON.parse(jsonText); } +/** One JSON-RPC request against an MCP endpoint, with the SSE framing parsed away. */ +export async function mcpRequest(endpoint: string, method: string, params: any = {}, id: number = 1) { + const response = await fetch(endpoint, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(createMCPRequestBody(method, params, id)), + }); + + if (!response.ok) { + throw new Error(`HTTP error! status: ${response.status}`); + } + + return parseMCPResponse(response); +} + export async function waitForMcpEndpoint( endpoint: string, options: { maxAttempts?: number; interval?: number; acceptStatuses?: number[] } = {}, @@ -69,10 +84,15 @@ export async function killPort(port: number): Promise { } } -export function startStorybook(configDir: string, port: number): ReturnType { +export function startStorybook( + configDir: string, + port: number, + env?: Record, +): ReturnType { return x('yarn', ['storybook', '--config-dir', configDir, '--port', String(port)], { nodeOptions: { cwd: STORYBOOK_DIR, + ...(env ? { env: { ...process.env, ...env } } : {}), }, }); } diff --git a/test-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts b/test-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts new file mode 100644 index 000000000000..8a0db31af34b --- /dev/null +++ b/test-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts @@ -0,0 +1,41 @@ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { x } from 'tinyexec'; +import { mcpRequest, waitForMcpEndpoint, killPort, startStorybook, stopStorybook } from './helpers'; + +/** + * The git-unusable scenario (not a git repository, or git itself broken) with `changeDetection` + * on: change detection legitimately answers "no changes detected", exactly as it did before the + * toolset swap — the tool must stay in the agent's repertoire instead of erroring. + */ + +const PORT = 6011; +const MCP_ENDPOINT = `http://localhost:${PORT}/mcp`; +const STARTUP_TIMEOUT = 60_000; + +let storybookProcess: ReturnType | null = null; + +describe('MCP endpoint when git is unusable', () => { + beforeAll(async () => { + await killPort(PORT); + // GIT_DIR pointing nowhere makes every git invocation fail, without touching the repo. + storybookProcess = startStorybook('.storybook', PORT, { GIT_DIR: '/nonexistent' }); + await waitForMcpEndpoint(MCP_ENDPOINT); + }, STARTUP_TIMEOUT); + + afterAll(async () => { + await stopStorybook(storybookProcess); + storybookProcess = null; + }); + + it('answers get-changed-stories with the no-changes sentence instead of an error', async () => { + const response = await mcpRequest(MCP_ENDPOINT, 'tools/call', { + name: 'get-changed-stories', + arguments: {}, + }); + + expect(response.result.isError).toBeUndefined(); + expect(response.result.content[0].text).toBe( + 'No new, modified, or related stories detected.', + ); + }); +});