diff --git a/code/addons/mcp/src/tools/tool-registry.ts b/code/addons/mcp/src/tools/tool-registry.ts index f67df499c379..5a56d469b727 100644 --- a/code/addons/mcp/src/tools/tool-registry.ts +++ b/code/addons/mcp/src/tools/tool-registry.ts @@ -23,6 +23,7 @@ import { GET_UI_BUILDING_INSTRUCTIONS_TOOL_NAME } from './tool-names.ts'; import { getToolsetToolMetadata, registerToolsetTool, + type McpToolsetGroup, type ToolsetToolOptions, } from './toolset-tools.ts'; @@ -52,12 +53,11 @@ export type AddonToolRegistryContext = { options?: Options; }; -type AddonToolset = keyof NonNullable; type ToolEnabled = Parameters['tool']>[0]['enabled']; type AddonToolDefinition = { name: string; - toolset: AddonToolset; + toolset: McpToolsetGroup; available?: (context: AddonToolRegistryContext) => boolean; getMetadata: (context: AddonToolRegistryContext) => ToolMetadata; register: ( @@ -68,8 +68,10 @@ type AddonToolDefinition = { getLocalTool?: (context: AddonToolRegistryContext & { options: Options }) => StorybookAiLocalTool; }; -const isToolsetEnabled = (toolset: AddonToolset, toolsets: AddonContext['toolsets'] | undefined) => - toolsets?.[toolset] ?? true; +const isToolsetEnabled = ( + toolset: McpToolsetGroup, + toolsets: AddonContext['toolsets'] | undefined +) => toolsets?.[toolset] ?? true; const isToolAvailable = (definition: AddonToolDefinition, context: AddonToolRegistryContext) => definition.available?.(context) ?? true; @@ -80,7 +82,7 @@ const isMetadataToolEnabled = ( ) => isToolsetEnabled(definition.toolset, context.toolsets) && isToolAvailable(definition, context); const createToolsetEnabled = - (server: McpServer, toolset: AddonToolset): ToolEnabled => + (server: McpServer, toolset: McpToolsetGroup): ToolEnabled => () => server.ctx.custom?.toolsets?.[toolset] ?? true; @@ -139,14 +141,19 @@ function compositionDocsToolset(server?: McpServer): DocsTool }); } +const DOCS_EVENT_NAMES = { + 'docs.list': 'tool:listAllDocumentation', + 'docs.show': 'tool:getDocumentation', + 'docs.showStory': 'tool:getDocumentationForStory', +} as const; + /** The docs tools, in the two shapes the registry needs: registered toolset, or per-request one. */ -function docsToolDefinition( - method: 'docs.list' | 'docs.show' | 'docs.showStory' -): AddonToolDefinition { - const forContext = (context: AddonToolRegistryContext): ToolsetToolOptions => - context.multiSource - ? { method, resolveToolset: (server) => compositionDocsToolset(server) } - : { method }; +function docsToolDefinition(method: keyof typeof DOCS_EVENT_NAMES): AddonToolDefinition { + const forContext = (context: AddonToolRegistryContext): ToolsetToolOptions => ({ + method, + mcpEventName: DOCS_EVENT_NAMES[method], + ...(context.multiSource ? { resolveToolset: (server) => compositionDocsToolset(server) } : {}), + }); return { name: toMcpToolName(method), @@ -170,6 +177,7 @@ const addonToolDefinitions: AddonToolDefinition[] = [ toolset: 'dev', options: { method: 'stories.preview', + mcpEventName: 'tool:previewStories', extras: { _meta: { ui: { resourceUri: PREVIEW_STORIES_RESOURCE_URI } } }, }, }), @@ -204,12 +212,12 @@ const addonToolDefinitions: AddonToolDefinition[] = [ fromToolset({ toolset: 'dev', available: ({ availability }) => availability.changeDetectionEnabled, - options: { method: 'stories.changed' }, + options: { method: 'stories.changed', mcpEventName: 'tool:getChangedStories' }, }), fromToolset({ toolset: 'dev', available: ({ availability }) => availability.moduleGraphSupported, - options: { method: 'stories.findByComponent' }, + options: { method: 'stories.findByComponent', mcpEventName: 'tool:getStoriesByComponent' }, }), fromToolset({ toolset: 'dev', @@ -224,13 +232,14 @@ const addonToolDefinitions: AddonToolDefinition[] = [ (server.ctx.custom?.reviewEnabled ?? availability.reviewEnabled), options: { method: 'review.create', + mcpEventName: 'tool:displayReview', wrapSchema: withFriendlyErrors, }, }), fromToolset({ toolset: 'test', available: ({ availability }) => availability.testSupported, - options: { method: 'test.run' }, + options: { method: 'test.run', mcpEventName: 'tool:runStoryTests' }, }), // Docs run on the core docs toolset in both modes. A composition builds its toolset per request, // because the sources it reads and the provider that fetches them belong to the request. diff --git a/code/addons/mcp/src/tools/toolset-tools.test.ts b/code/addons/mcp/src/tools/toolset-tools.test.ts index fa2e0de43122..1d52383e80e5 100644 --- a/code/addons/mcp/src/tools/toolset-tools.test.ts +++ b/code/addons/mcp/src/tools/toolset-tools.test.ts @@ -34,15 +34,12 @@ function registerStubStoriesToolset( handler: overrides.handler ?? (async (input: { id: string }, ctx) => { - await ctx.telemetry?.('tool:previewStories', { - toolset: 'dev', - 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'), + telemetry: { payload: { inputStoryCount: 1 } }, }; }), }, @@ -57,7 +54,7 @@ function makeServer(custom: Record = {}) { } as any; } -const previewOptions = { method: 'stories.preview' } as const; +const previewOptions = { method: 'stories.preview', mcpEventName: 'tool:previewStories' } as const; describe('toolset-backed MCP tools', () => { beforeEach(() => { @@ -146,7 +143,11 @@ describe('toolset-backed MCP tools', () => { }) as any ); - const result = await callToolsetMethod(makeServer(), { method: 'stories.changed' }, {}); + const result = await callToolsetMethod( + makeServer(), + { method: 'stories.changed', mcpEventName: 'tool:getChangedStories' }, + {} + ); expect(result.content).toEqual([{ type: 'text', text: 'no changes' }]); expect(result.structuredContent).toBeUndefined(); @@ -219,18 +220,28 @@ describe('toolset-backed MCP tools', () => { expect(vi.mocked(logger.error).mock.calls[0][0]).toContain('boom'); }); - it('forwards method telemetry with the surface fields the adapter owns', async () => { + it('sends the report on the outcome under the legacy event name the addon keeps', async () => { registerStubStoriesToolset(); await callToolsetMethod(makeServer(), previewOptions, { id: 'button--primary' }); - expect(collectTelemetry).toHaveBeenCalledWith( - expect.objectContaining({ - event: 'tool:previewStories', - toolset: 'dev', - inputStoryCount: 1, - }) - ); + expect(collectTelemetry).toHaveBeenCalledWith({ + event: 'tool:previewStories', + server: expect.anything(), + toolset: 'stories', + tool: 'preview', + inputStoryCount: 1, + }); + }); + + it('emits no telemetry for an outcome without a report', async () => { + registerStubStoriesToolset({ + handler: async () => ({ ok: true, data: { stories: [] }, markdown: '' }), + }); + + await callToolsetMethod(makeServer(), previewOptions, { id: 'button--primary' }); + + expect(collectTelemetry).not.toHaveBeenCalled(); }); it('emits no telemetry when the session disabled it', async () => { diff --git a/code/addons/mcp/src/tools/toolset-tools.ts b/code/addons/mcp/src/tools/toolset-tools.ts index 88743a1e79ba..c73267d385a2 100644 --- a/code/addons/mcp/src/tools/toolset-tools.ts +++ b/code/addons/mcp/src/tools/toolset-tools.ts @@ -12,6 +12,8 @@ import { logger } from 'storybook/internal/node-logger'; import { OpenServiceToolsetOutputMismatchError } from 'storybook/internal/server-errors'; import { getToolset, + invokeToolsetMethod, + parseToolsetMethodId, resolveToolsetDescription, toMcpToolName, type AnyToolsetDefinition, @@ -19,6 +21,7 @@ import { type ToolsetCtx, type ToolsetMethod, type ToolsetMethodId, + type ToolsetMethodReport, } from 'storybook/open-service'; import type { McpServer } from 'tmcp'; @@ -31,9 +34,18 @@ import type { StorybookAiToolCallResult } from './tool-registry.ts'; type Server = McpServer; type ToolEnabled = Parameters[0]['enabled']; +// The grouping behind the enable gate and the `X-MCP-Toolsets` header; not a telemetry field. +export type McpToolsetGroup = keyof NonNullable; + export type ToolsetToolOptions = { /** Which toolset method backs this MCP tool. */ method: ToolsetMethodId; + /** + * The `event` of this tool's `addon-mcp` record. These names predate the toolsets + * (`tool:getChangedStories`) and replace the generated `tool:stories_changed` on this surface + * only, so MCP usage data stays continuous across versions. The toolsets know nothing of them. + */ + mcpEventName: string; /** Extra MCP-only tool metadata, e.g. the preview app resource. */ extras?: Record; /** Wraps the input schema before publishing it (used for friendlier validation errors). */ @@ -52,16 +64,14 @@ function resolveToolset(options: ToolsetToolOptions, server?: Server): AnyToolse if (options.resolveToolset) { return options.resolveToolset(server); } - const [toolsetId] = options.method.split('.'); - return getToolset(toolsetId); + return getToolset(parseToolsetMethodId(options.method).toolsetId); } function resolveMethod( toolset: AnyToolsetDefinition, options: ToolsetToolOptions ): ToolsetMethod { - const [, methodName] = options.method.split('.'); - return toolset.methods[methodName]; + return toolset.methods[parseToolsetMethodId(options.method).methodName]; } /** @@ -95,24 +105,26 @@ async function toStructuredContent( } function buildContext(server: Server): ToolsetCtx { - const custom = server.ctx.custom; return { transport: 'mcp', // The toolset origin is the complete UI base URL, including any deployment subpath. - origin: resolveToolsetOrigin(custom ?? {}), + origin: resolveToolsetOrigin(server.ctx.custom ?? {}), getService: (serviceId, serviceOptions) => getService(serviceId as any, serviceOptions) as any, - telemetry: custom?.disableTelemetry - ? undefined - : async (event, payload) => { - await collectTelemetry({ - event, - server, - ...payload, - }); - }, }; } +async function reportToolsetTelemetry( + server: Server, + event: string, + report: ToolsetMethodReport | undefined +): Promise { + if (!report || server.ctx.custom?.disableTelemetry) { + return; + } + const { payload, toolset, tool } = report; + await collectTelemetry({ event, server, ...payload, toolset, tool }); +} + /** Runs one toolset method and unwraps its outcome into an MCP tool result. */ export async function callToolsetMethod( server: Server, @@ -120,11 +132,12 @@ export async function callToolsetMethod( input: unknown ): Promise { const toolset = resolveToolset(options, server); - const method = resolveMethod(toolset, options); - const ctx = buildContext(server); + const { methodName } = parseToolsetMethodId(options.method); + const method = toolset.methods[methodName]; try { - const outcome = await method.handler(input as never, ctx); + const outcome = await invokeToolsetMethod(toolset, methodName, input, buildContext(server)); + await reportToolsetTelemetry(server, options.mcpEventName, outcome.telemetry); const structuredContent = await toStructuredContent(method.output, outcome.data); const blocks = Array.isArray(outcome.markdown) ? outcome.markdown : [outcome.markdown]; diff --git a/code/addons/vitest/src/node/toolset/definition.test.ts b/code/addons/vitest/src/node/toolset/definition.test.ts index f48ca5aae4aa..ae2de58a7aa7 100644 --- a/code/addons/vitest/src/node/toolset/definition.test.ts +++ b/code/addons/vitest/src/node/toolset/definition.test.ts @@ -1,5 +1,5 @@ import type { StoryIndex } from 'storybook/internal/types'; -import type { ToolsetCtx } from 'storybook/open-service'; +import { invokeToolsetMethod, type ToolsetCtx } from 'storybook/open-service'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import * as v from 'valibot'; @@ -13,14 +13,12 @@ vi.mock('./run.ts', { spy: true }); const index = { v: 5, entries: {} } as StoryIndex; const getIndex = vi.fn(); const storyIndex = { getIndex }; -const telemetry = vi.fn(); const channel = {} as never; const ctx = { transport: 'cli', origin: 'http://localhost:6006', getService: vi.fn() as ToolsetCtx['getService'], - telemetry, } satisfies ToolsetCtx; const mcpCtx = { ...ctx, transport: 'mcp' } satisfies ToolsetCtx; @@ -75,7 +73,7 @@ function runTests( input: v.InferInput = {}, runCtx: ToolsetCtx = ctx ) { - return toolset.methods.run.handler(v.parse(toolset.methods.run.input, input), runCtx); + return invokeToolsetMethod(toolset, 'run', v.parse(toolset.methods.run.input, input), runCtx); } /** Runs and renders the way the MCP adapter does: one handler call, markdown from the outcome. */ @@ -445,17 +443,21 @@ No story found for story ID "gone--story"`); }) ); - await runTests({ stories: [{ storyId: 'button--primary' }] }); + const outcome = await runTests({ stories: [{ storyId: 'button--primary' }] }); - expect(telemetry).toHaveBeenCalledWith('tool:runStoryTests', { + expect(outcome.telemetry).toEqual({ toolset: 'test', - runA11y: true, - inputStoryCount: 1, - matchedStoryCount: 1, - passingStoryCount: 1, - failingStoryCount: 0, - a11yViolationCount: 1, - unhandledErrorCount: 0, + tool: 'run', + event: 'tool:test_run', + payload: { + runA11y: true, + inputStoryCount: 1, + matchedStoryCount: 1, + passingStoryCount: 1, + failingStoryCount: 0, + a11yViolationCount: 1, + unhandledErrorCount: 0, + }, }); }); @@ -465,50 +467,30 @@ No story found for story ID "gone--story"`); notFoundMessages: ['No story found for story ID "missing--story"'], }); - await runTests({ stories: [{ storyId: 'missing--story' }], a11y: false }); + const outcome = await runTests({ stories: [{ storyId: 'missing--story' }], a11y: false }); - expect(telemetry).toHaveBeenCalledWith('tool:runStoryTests', { + expect(outcome.telemetry).toEqual({ toolset: 'test', - runA11y: false, - inputStoryCount: 1, - matchedStoryCount: 0, - passingStoryCount: 0, - failingStoryCount: 0, - a11yViolationCount: 0, - unhandledErrorCount: 0, + tool: 'run', + event: 'tool:test_run', + payload: { + runA11y: false, + inputStoryCount: 1, + matchedStoryCount: 0, + passingStoryCount: 0, + failingStoryCount: 0, + a11yViolationCount: 0, + unhandledErrorCount: 0, + }, }); }); it('stays silent for a run that never reached a verdict', async () => { vi.mocked(runStoryTests).mockResolvedValue({ status: 'cancelled' }); - await runTests(); + const outcome = await runTests(); - expect(telemetry).not.toHaveBeenCalled(); - }); - - it.each([ - ['completed', completedRun], - [ - 'no-stories', - { - status: 'no-stories' as const, - notFoundMessages: ['No story found for story ID "missing--story"'], - }, - ], - ])('does not fail a %s result when telemetry rejects', async (_status, result) => { - vi.mocked(runStoryTests).mockResolvedValue(result); - const rejectingCtx: ToolsetCtx = { - ...ctx, - telemetry: async () => { - throw new Error('telemetry unavailable'); - }, - }; - - const outcome = await runTests({}, rejectingCtx); - - expect(outcome.ok).toBe(true); - expect(outcome.data.status).toBe(result.status); + expect(outcome.telemetry).toBeUndefined(); }); }); }); diff --git a/code/addons/vitest/src/node/toolset/definition.ts b/code/addons/vitest/src/node/toolset/definition.ts index 96d28cc8253e..7d91be2ddcfb 100644 --- a/code/addons/vitest/src/node/toolset/definition.ts +++ b/code/addons/vitest/src/node/toolset/definition.ts @@ -3,8 +3,7 @@ import * as v from 'valibot'; import { storyInputArraySchema, type StoryIndexAccess } from 'storybook/internal/core-server'; import { defineToolset, - reportToolsetTelemetry, - type ToolsetCtx, + type ToolsetTelemetryReport, type ToolsetOutcome, } from 'storybook/open-service'; @@ -158,38 +157,39 @@ For visual/design accessibility violations (for example color contrast), ask the } /** - * Reports a run that reached a verdict. A run that never got one — a channel error, a cancellation — - * stays silent, so the event counts runs whose numbers mean something. + * The report for a run that reached a verdict. A run that never got one — a channel error, a + * cancellation — reports nothing, so the event counts runs whose numbers mean something. */ -async function reportRunTelemetry(data: TestRunData, input: RunInput, ctx: ToolsetCtx) { +function runTelemetry(data: TestRunData, input: RunInput): ToolsetTelemetryReport | undefined { const inputStoryCount = input.stories?.length ?? 0; if (data.status === 'no-stories') { - await reportToolsetTelemetry(ctx, 'tool:runStoryTests', { - toolset: 'test', - runA11y: data.a11y, - inputStoryCount, - matchedStoryCount: 0, - passingStoryCount: 0, - failingStoryCount: 0, - a11yViolationCount: 0, - unhandledErrorCount: 0, - }); - return; + return { + payload: { + runA11y: data.a11y, + inputStoryCount, + matchedStoryCount: 0, + passingStoryCount: 0, + failingStoryCount: 0, + a11yViolationCount: 0, + unhandledErrorCount: 0, + }, + }; } if (data.status !== 'completed') { - return; + return undefined; } - await reportToolsetTelemetry(ctx, 'tool:runStoryTests', { - toolset: 'test', - runA11y: data.a11y, - inputStoryCount, - // A partially resolved selector list never reaches a run, so every input matched by this point. - matchedStoryCount: data.result.storyIds?.length ?? inputStoryCount, - ...summarizeTestRun(data.result, data.a11y), - }); + return { + payload: { + runA11y: data.a11y, + inputStoryCount, + // A partially resolved selector list never reaches a run, so every input matched by this point. + matchedStoryCount: data.result.storyIds?.length ?? inputStoryCount, + ...summarizeTestRun(data.result, data.a11y), + }, + }; } function isFailedRun(data: TestRunData): data is TestRunFailureData { @@ -248,11 +248,11 @@ export function createTestToolset({ channel, storyIndex, a11yEnabled }: CreateTe a11y: input.a11y, }); const data: TestRunData = { ...output, a11y: input.a11y }; - - await reportRunTelemetry(data, input, ctx); - const markdown = formatTestRun(data, ctx); - return isFailedRun(data) ? { ok: false, data, markdown } : { ok: true, data, markdown }; + const telemetry = runTelemetry(data, input); + return isFailedRun(data) + ? { ok: false, data, markdown, telemetry } + : { ok: true, data, markdown, telemetry }; } finally { done(); } diff --git a/code/core/src/cli/tools/architecture.md b/code/core/src/cli/tools/architecture.md index 26c5efd5abaa..ce7f617b897d 100644 --- a/code/core/src/cli/tools/architecture.md +++ b/code/core/src/cli/tools/architecture.md @@ -109,9 +109,12 @@ See [Load](../../shared/open-service/README.md#load). In local mode, `requiresDevServer` intercepts with start-your-Storybook guidance. In attached mode those methods run in the caller. `stories.preview` reads origin from the instance record. -The CLI fires a `tools-command` invocation event after a run. The payload includes `attachMode`. -Per-method toolset telemetry (`ctx.telemetry`) fires in the caller when the CLI passes a sink into -`tools.call`. Command-level side effects and their telemetry run on the instance. +The CLI and the SDK fire one `tools-command` record per invocation after a run; help lookups are +excluded so they cannot skew success rates. The record is the handler's usage report — `toolset` +and `tool` in CLI spelling, the generated `event` (`tool:docs_list`), and its payload, all returned +on the outcome by `invokeToolsetMethod` — plus `success`, `outcome`, `duration`, and the attach fields +(`attachMode` among them). A child host's report rides inside the outcome that already crosses +IPC. Command-level side effects run on the instance. ## SDK diff --git a/code/core/src/cli/tools/register.test.ts b/code/core/src/cli/tools/register.test.ts index da8d77955136..ec2379bca565 100644 --- a/code/core/src/cli/tools/register.test.ts +++ b/code/core/src/cli/tools/register.test.ts @@ -57,10 +57,7 @@ function parse(program: Command, argv: string[]) { function toolsCommandPayloads(): unknown[] { return vi .mocked(telemetry) - .mock.calls.filter( - ([eventType, payload]) => - eventType === 'tools-command' && payload !== undefined && !('event' in payload) - ) + .mock.calls.filter(([eventType]) => eventType === 'tools-command') .map(([, payload]) => payload); } @@ -119,7 +116,8 @@ describe('tools-command telemetry', () => { expect(toolsCommandPayloads()).toEqual([ { - command: 'docs list', + toolset: 'docs', + tool: 'list', success: true, outcome: 'success', client: 'cli', @@ -133,6 +131,66 @@ describe('tools-command telemetry', () => { expect(sendTelemetryError).not.toHaveBeenCalled(); }); + it('merges the handler report into the one tools-command record', async () => { + const { program } = buildProgram(); + vi.mocked(runToolsCommand).mockResolvedValue( + successResult({ + attachMode: 'local', + report: { + toolset: 'stories', + tool: 'changed', + event: 'tool:stories_changed', + payload: { + storyCount: 4, + newStoryCount: 1, + modifiedStoryCount: 3, + affectedStoryCount: 0, + }, + }, + }) + ); + await parse(program, ['tools', 'stories', 'changed']); + + expect(toolsCommandPayloads()).toEqual([ + { + toolset: 'stories', + tool: 'changed', + event: 'tool:stories_changed', + success: true, + outcome: 'success', + duration: expect.any(Number), + client: 'cli', + requestedMode: 'auto', + resolvedMode: 'local', + attachMode: 'local', + host: 'in-process', + storyCount: 4, + newStoryCount: 1, + modifiedStoryCount: 3, + affectedStoryCount: 0, + }, + ]); + }); + + it('names the tool by its registered spelling, not by what the agent typed', async () => { + const { program } = buildProgram(); + vi.mocked(runToolsCommand).mockResolvedValue( + successResult({ + report: { + toolset: 'stories', + tool: 'find-by-component', + event: 'tool:stories_findByComponent', + payload: { componentCount: 1 }, + }, + }) + ); + await parse(program, ['tools', 'stories', 'findByComponent']); + + expect(toolsCommandPayloads()).toEqual([ + expect.objectContaining({ toolset: 'stories', tool: 'find-by-component', componentCount: 1 }), + ]); + }); + it('reports an attach-gate outcome from --attach', async () => { const { program } = buildProgram(); vi.mocked(runToolsCommand).mockResolvedValue( @@ -149,7 +207,8 @@ describe('tools-command telemetry', () => { expect(toolsCommandPayloads()).toEqual([ expect.objectContaining({ - command: 'docs list', + toolset: 'docs', + tool: 'list', success: false, outcome: 'attach-gate', client: 'cli', @@ -163,6 +222,27 @@ describe('tools-command telemetry', () => { expect(sendTelemetryError).not.toHaveBeenCalled(); }); + it('names no toolset or tool when the attach gate fired before any was parsed', async () => { + const { program } = buildProgram(); + vi.mocked(runToolsCommand).mockResolvedValue( + successResult({ + exitCode: 1, + output: 'No running Storybook', + outcome: { kind: 'attach-gate', reason: 'no-instance' }, + requestedMode: 'attached', + attachMode: 'attached', + host: undefined, + }) + ); + await parse(program, ['tools', '--attach']); + + expect(toolsCommandPayloads()).toEqual([ + expect.objectContaining({ success: false, outcome: 'attach-gate' }), + ]); + expect(toolsCommandPayloads()[0]).not.toHaveProperty('toolset'); + expect(toolsCommandPayloads()[0]).not.toHaveProperty('tool'); + }); + it('keeps requestedMode auto and attachGate on a successful local fallback', async () => { const { program } = buildProgram(); vi.mocked(runToolsCommand).mockResolvedValue( @@ -178,7 +258,8 @@ describe('tools-command telemetry', () => { expect(toolsCommandPayloads()).toEqual([ expect.objectContaining({ - command: 'docs list', + toolset: 'docs', + tool: 'list', success: true, outcome: 'success', client: 'cli', @@ -204,7 +285,8 @@ describe('tools-command telemetry', () => { expect(toolsCommandPayloads()).toEqual([ expect.objectContaining({ - command: 'nope list', + toolset: 'nope', + tool: 'list', success: false, outcome: 'intercept', interceptReason: 'unknown-toolset', @@ -232,7 +314,8 @@ describe('tools-command telemetry', () => { expect(toolsCommandPayloads()).toEqual([ expect.objectContaining({ - command: 'docs list', + toolset: 'docs', + tool: 'list', success: false, outcome: 'error', client: 'cli', @@ -272,7 +355,7 @@ describe('tools-command telemetry', () => { await parse(program, ['tools', './projects/secret-app', 'list']); expect(toolsCommandPayloads()).toEqual([ - expect.objectContaining({ command: '(invalid) list' }), + expect.objectContaining({ toolset: '(invalid)', tool: 'list' }), ]); }); @@ -283,7 +366,8 @@ describe('tools-command telemetry', () => { expect(runToolsCommand).not.toHaveBeenCalled(); expect(toolsCommandPayloads()).toEqual([ expect.objectContaining({ - command: 'docs list', + toolset: 'docs', + tool: 'list', success: false, outcome: 'intercept', interceptReason: 'invalid-arguments', @@ -299,8 +383,7 @@ describe('tools-command telemetry', () => { await parse(program, ['tools', '--port', '6006', 'docs', 'list']); expect(runToolsCommand).toHaveBeenCalledWith( - expect.objectContaining({ toolset: 'docs', tool: 'list', port: '6006' }), - expect.anything() + expect.objectContaining({ toolset: 'docs', tool: 'list', port: '6006' }) ); }); diff --git a/code/core/src/cli/tools/register.ts b/code/core/src/cli/tools/register.ts index caa2069e9414..045003a09fb0 100644 --- a/code/core/src/cli/tools/register.ts +++ b/code/core/src/cli/tools/register.ts @@ -3,14 +3,13 @@ import { resolve } from 'node:path'; import { sendTelemetryError, withTelemetry } from 'storybook/internal/core-server'; import { logger } from 'storybook/internal/node-logger'; -import { telemetry } from 'storybook/internal/telemetry'; import type { CLIOptions } from 'storybook/internal/types'; import { Option, type Command } from 'commander'; -import type { ToolsetTelemetry } from '../../shared/open-service/toolset-definition.ts'; import { resolveStorybookConfigDir } from './config-dir.ts'; import { runToolsCommand, type ToolsRunResult } from './run.ts'; +import { reportToolsCommandEvent, sanitizeNamePart } from './sdk/command-telemetry.ts'; import { isJsonToolsRun, TOOLS_OPTION_SPECS, type ToolsOutputFlags } from './tool-tokens.ts'; /** `handleCommandFailure` from `bin/core.ts`, passed in to avoid an import cycle. */ @@ -127,18 +126,15 @@ export function registerToolsPassthrough( attachMode: 'auto', }; } else { - result = await runToolsCommand( - { - toolset, - tool, - tokens, - target: { cwd: options.cwd, configDir: options.configDir }, - port: options.port, - attach: options.noAttach ? false : options.attach, - flags, - }, - { methodTelemetry: createMethodTelemetrySink(cliOptions) } - ); + result = await runToolsCommand({ + toolset, + tool, + tokens, + target: { cwd: options.cwd, configDir: options.configDir }, + port: options.port, + attach: options.noAttach ? false : options.attach, + flags, + }); } } finally { clearInterval(keepAlive); @@ -150,7 +146,7 @@ export function registerToolsPassthrough( // The tool has executed either way, so a failed `--output` write must not lose the // event. Reporting after printing keeps a slow telemetry endpoint from ever // delaying the user's result. - await reportToolsCommandTelemetry(toolset, tool, result, duration, cliOptions); + await reportToolsCommandTelemetry({ toolset, tool, result, duration }, cliOptions); } } ).catch(handleCommandFailure(options.logfile)); @@ -183,58 +179,31 @@ function pickCliOptions(options: ToolsPassthroughOptions): CLIOptions { }; } -/** - * The per-method toolset telemetry sink. Events pass through `telemetry()`, which honors the - * resolved opt-out state, so the sink itself never has to know whether telemetry is enabled. They - * report under the same `tools-command` event as the per-invocation record, distinguished by the - * `event` field the method supplies (mirroring how MCP method events report under `addon-mcp`). - */ -function createMethodTelemetrySink(cliOptions: CLIOptions): ToolsetTelemetry { - return async (event, payload) => { - try { - await telemetry('tools-command', { event, ...payload }, { configDir: cliOptions.configDir }); - } catch (error) { - logger.debug(`Error collecting telemetry: ${String(error)}`); - } - }; -} - -/** - * Tool and toolset names are a fixed, project-defined vocabulary of short identifiers. Anything - * else is arbitrary agent input (a typo'd path, a stray flag value) that must not be sent - * verbatim, so it is collapsed to a placeholder. The intercept reason still tells the failure - * class apart. - */ -function sanitizeNamePart(part: string): string { - return /^[\w-]{1,64}$/.test(part) ? part : '(invalid)'; -} - /** * Fire the `tools-command` event, once per executed tool, modeled on the `ai-command` event - * (storybookjs/storybook#35131). Help lookups are excluded so they cannot skew success rates. - * Unexpected failures additionally go through the standard sanitized error path; `failure` - * outcomes (the tool ran and reported bad news) do not. + * (storybookjs/storybook#35131). The handler's own report, returned on its outcome, is merged + * into the same record. Help lookups are excluded so they cannot skew success rates. Unexpected + * failures additionally go through the standard sanitized error path; `failure` outcomes (the + * tool ran and reported bad news) do not. */ async function reportToolsCommandTelemetry( - toolset: string | undefined, - tool: string | undefined, - result: ToolsRunResult, - duration: number, + run: { + toolset: string | undefined; + tool: string | undefined; + result: ToolsRunResult; + duration: number; + }, cliOptions: CLIOptions ): Promise { + const { toolset, tool, result, duration } = run; const { outcome } = result; if (outcome.kind === 'help') { return; } - const command = - [toolset, tool] - .filter((part): part is string => part !== undefined) - .map(sanitizeNamePart) - .join(' ') || '(none)'; - await telemetry( - 'tools-command', + await reportToolsCommandEvent( { - command, + ...(toolset !== undefined ? { toolset: sanitizeNamePart(toolset) } : {}), + ...(tool !== undefined ? { tool: sanitizeNamePart(tool) } : {}), success: outcome.kind === 'success', outcome: outcome.kind, client: 'cli', @@ -251,7 +220,7 @@ async function reportToolsCommandTelemetry( duration, }, // Metadata must describe the target project, consistent with the opt-out resolution. - { configDir: cliOptions.configDir } + { report: result.report, configDir: cliOptions.configDir } ); if (outcome.kind === 'error') { await sendTelemetryError(outcome.error, 'tools-command', { cliOptions }); diff --git a/code/core/src/cli/tools/run.test.ts b/code/core/src/cli/tools/run.test.ts index 8860b9e24ee4..96d02e4de951 100644 --- a/code/core/src/cli/tools/run.test.ts +++ b/code/core/src/cli/tools/run.test.ts @@ -21,6 +21,7 @@ import { import type { DocsAccess } from '../../shared/open-service/toolsets/docs/access.ts'; import type { StorybookInstanceRecord } from './instances/types.ts'; import { runToolsCommand, type ToolsInvocation, type ToolsRunDeps } from './run.ts'; +import { invokeToolsetMethod } from '../../shared/open-service/toolset-definition.ts'; import { parseToolsetMethodId } from '../../shared/open-service/toolset-names.ts'; import { toCatalogEntry } from './sdk/catalog.ts'; import { @@ -127,10 +128,9 @@ function makeLocalTools(runtimeOverrides: Partial = {}): LocalTool issues: validation.issues, }); } - return method.handler(validation.value, { + return invokeToolsetMethod(toolset, methodName, validation.value, { ...ctx, ...(options.origin ? { origin: options.origin } : {}), - ...(options.telemetry ? { telemetry: options.telemetry } : {}), }); }, close: async () => {}, @@ -178,7 +178,6 @@ function makeAttachedTools(runtimeOverrides: Partial = {}): Attach const callCtx: ToolsetCtx = { ...ctx, ...(options?.origin !== undefined ? { origin: options.origin } : {}), - ...(options?.telemetry ? { telemetry: options.telemetry } : {}), }; const { toolsetId, methodName } = parseToolsetMethodId(ref); const toolset = local.runtime.toolsets.find((candidate) => candidate.id === toolsetId); @@ -194,7 +193,7 @@ function makeAttachedTools(runtimeOverrides: Partial = {}): Attach issues: validation.issues, }); } - return method.handler(validation.value, callCtx); + return invokeToolsetMethod(toolset, methodName, validation.value, callCtx); }, }; } @@ -216,36 +215,15 @@ describe('local tools', () => { // Parity claim: the CLI must print byte-for-byte what MCP clients receive. The MCP adapter // itself lives in addon-mcp (core tests cannot reach it); its own suite asserts it renders - // handler markdown verbatim as text blocks, so comparing against the handler's markdown under - // an MCP context is the same contract expressed from this side of the package boundary. - const mcpCtx: ToolsetCtx = { transport: 'mcp', getService: () => ({}) as never }; - const mcpOutcome = await getToolset('docs').methods.list.handler({}, mcpCtx); + // handler markdown verbatim as text blocks, so comparing against the handler's markdown is + // the same contract expressed from this side of the package boundary. + const mcpOutcome = await getToolset('docs').methods.list.handler({}); expect(result.exitCode).toBe(0); expect(result.outcome).toEqual({ kind: 'success' }); expect(result.output).toContain('Button'); expect(result.output).toBe(mcpOutcome.markdown); }); - it('stamps tools-command dimensions onto per-method telemetry for a local host', async () => { - const methodTelemetry = vi.fn(async () => {}); - const { deps } = makeDeps({ methodTelemetry }); - - const result = await run(['docs', 'list'], deps); - - expect(result.outcome).toEqual({ kind: 'success' }); - expect(methodTelemetry).toHaveBeenCalledWith( - 'tool:listAllDocumentation', - expect.objectContaining({ - toolset: 'docs', - client: 'cli', - requestedMode: 'local', - resolvedMode: 'local', - attachMode: 'local', - host: 'in-process', - }) - ); - }); - it('round-trips the show-story --storyId flag through token parsing to the handler', async () => { const { deps } = makeDeps(); @@ -792,32 +770,26 @@ describe('outcome mapping', () => { }); }); -describe('telemetry sink', () => { - it('forwards per-method events with the toolset’s telemetry group', async () => { - const methodTelemetry = vi.fn(async () => {}); - const { deps } = makeDeps({ methodTelemetry }); +describe('usage report', () => { + it('carries the handler report out of a local run', async () => { + const { deps } = makeDeps(); - await run(['docs', 'list'], deps); + const result = await run(['docs', 'list'], deps); - expect(methodTelemetry).toHaveBeenCalledWith( - 'tool:listAllDocumentation', - expect.objectContaining({ toolset: 'docs' }) - ); + expect(result.report).toEqual({ + toolset: 'docs', + tool: 'list', + event: 'tool:docs_list', + payload: expect.objectContaining({ componentCount: expect.any(Number) }), + }); }); - it('forwards per-method events on attached dispatch', async () => { - const methodTelemetry = vi.fn(async () => {}); - const { deps } = makeDeps({ - methodTelemetry, - createTools: vi.fn(async () => makeAttachedTools()), - }); + it('carries the handler report out of an attached run', async () => { + const { deps } = makeDeps({ createTools: vi.fn(async () => makeAttachedTools()) }); - await run(['docs', 'list'], deps, { attach: true }); + const result = await run(['docs', 'list'], deps, { attach: true }); - expect(methodTelemetry).toHaveBeenCalledWith( - 'tool:listAllDocumentation', - expect.objectContaining({ toolset: 'docs' }) - ); + expect(result.report).toEqual(expect.objectContaining({ toolset: 'docs', tool: 'list' })); }); }); diff --git a/code/core/src/cli/tools/run.ts b/code/core/src/cli/tools/run.ts index 137b448ba4ea..fa47de1d4be4 100644 --- a/code/core/src/cli/tools/run.ts +++ b/code/core/src/cli/tools/run.ts @@ -1,6 +1,6 @@ import { versions } from 'storybook/internal/common'; -import type { ToolsetTelemetry } from '../../shared/open-service/toolset-definition.ts'; +import type { ToolsetMethodReport } from '../../shared/open-service/toolset-definition.ts'; import { parseToolsetMethodId, toCliMethodName } from '../../shared/open-service/toolset-names.ts'; import type { StorybookInstanceRecord } from './instances/types.ts'; import { @@ -8,8 +8,6 @@ import { createTools, formatMultiInstanceNotice, isAttachGateError, - toolsCommandDimensions, - wrapMethodTelemetry, ToolsRuntimeError, type CreateToolsDeps, type CreateToolsOptions, @@ -80,6 +78,8 @@ export type ToolsRunResult = { multiInstanceNotice?: string; /** True when the attached host chose among several matching instances; drives telemetry. */ multipleMatches?: boolean; + /** The handler's usage report, from the outcome of a run that reached the handler. */ + report?: ToolsetMethodReport; }; export type ToolsInvocation = { @@ -107,8 +107,6 @@ const CLI_CLIENT_INFO: ToolsClientInfo = { export type ToolsRunDeps = { createTools?: (options?: CreateToolsOptions, deps?: CreateToolsDeps) => Promise; discoverInstance?: typeof discoverRunningInstance; - /** Sink for the per-method toolset telemetry events; absent when telemetry is disabled. */ - methodTelemetry?: ToolsetTelemetry; }; /** `find-by-component` -> `findByComponent`, accepting an already-camelCase spelling unchanged. */ @@ -231,25 +229,11 @@ export async function runToolsCommand( } try { - const methodTelemetry = - tools.mode === 'local' && deps.methodTelemetry - ? wrapMethodTelemetry( - deps.methodTelemetry, - toolsCommandDimensions({ - clientInfo: tools.clientInfo, - requestedMode: tools.requestedMode, - resolvedMode: tools.mode, - host: tools.host, - fallbackReason: tools.fallbackReason, - }) - ) - : deps.methodTelemetry; - const dispatchDeps: ToolsRunDeps = { ...deps, methodTelemetry }; const dispatched = await dispatchTools( tools, { ...normalized, target }, parsed, - dispatchDeps, + deps, requestedMode, result ); @@ -354,7 +338,6 @@ async function dispatchTools( try { const outcome = await tools.call(method.ref, parsed.args, { ...(tools.storybook.url ? { origin: tools.storybook.url } : {}), - ...(deps.methodTelemetry ? { telemetry: deps.methodTelemetry } : {}), }); const output = parsed.json ? JSON.stringify(outcome.data, null, 2) @@ -363,6 +346,7 @@ async function dispatchTools( exitCode: outcome.ok ? 0 : 1, output, outcome: { kind: outcome.ok ? 'success' : 'failure' }, + ...(outcome.telemetry ? { report: outcome.telemetry } : {}), }); } catch (error) { if (isInvalidInputError(error)) { diff --git a/code/core/src/cli/tools/sdk/child-client.test.ts b/code/core/src/cli/tools/sdk/child-client.test.ts index f3fa0372c0cb..dbd2b0d3a5a6 100644 --- a/code/core/src/cli/tools/sdk/child-client.test.ts +++ b/code/core/src/cli/tools/sdk/child-client.test.ts @@ -257,97 +257,60 @@ describe('spawnChildHost', () => { ); }); - it('forwards child method telemetry to the call sink without resolving the waiter', async () => { + it('reports the invocation from the report the child outcome carries', async () => { + const report = { + toolset: 'docs', + tool: 'list', + event: 'tool:docs_list', + payload: { componentCount: 3 }, + }; child.send.mockImplementation((message: { type: string; id?: string }) => { if (message.type === 'init') { queueMicrotask(() => child.emit('message', HELLO)); } if (message.type === 'call') { - queueMicrotask(() => { - child.emit('message', { - type: 'telemetry', - id: message.id, - event: 'tool:listAllDocumentation', - payload: { toolset: 'docs' }, - }); + queueMicrotask(() => child.emit('message', { type: 'result', id: message.id, - value: { ok: true, data: { ran: true }, markdown: 'ok' }, - }); - }); + value: { ok: true, data: { ran: true }, markdown: 'ok', telemetry: report }, + }) + ); } return true; }); - const sink = vi.fn(async () => {}); const tools = await spawn(OPTIONS, 'auto'); - await expect(tools.call('docs.list', {}, { telemetry: sink })).resolves.toEqual({ + await expect(tools.call('docs.list', {})).resolves.toEqual({ ok: true, data: { ran: true }, markdown: 'ok', + telemetry: report, }); expect(tools.requestedMode).toBe('auto'); - expect(sink).toHaveBeenCalledWith( - 'tool:listAllDocumentation', - expect.objectContaining({ - toolset: 'docs', - client: 'sdk', - requestedMode: 'auto', - resolvedMode: 'attached', - attachMode: 'attached', - host: 'child', - }) - ); - expect(telemetry).toHaveBeenCalledWith( - 'tools-command', - expect.objectContaining({ - command: 'docs list', - success: true, - outcome: 'success', - client: 'sdk', - requestedMode: 'auto', - host: 'child', - }), - expect.anything() - ); - }); - - it('does not reject the call when a forwarded telemetry sink fails', async () => { - child.send.mockImplementation((message: { type: string; id?: string }) => { - if (message.type === 'init') { - queueMicrotask(() => child.emit('message', HELLO)); - } - if (message.type === 'call') { - queueMicrotask(() => { - child.emit('message', { - type: 'telemetry', - id: message.id, - event: 'tool:listAllDocumentation', - payload: { toolset: 'docs' }, - }); - child.emit('message', { - type: 'result', - id: message.id, - value: { ok: true, data: { ran: true }, markdown: 'ok' }, - }); - }); - } - return true; - }); - const sink = vi.fn(async () => { - throw new Error('telemetry down'); - }); - const tools = await spawn(); - - await expect(tools.call('docs.list', {}, { telemetry: sink })).resolves.toEqual({ - ok: true, - data: { ran: true }, - markdown: 'ok', - }); + expect(vi.mocked(telemetry).mock.calls).toEqual([ + [ + 'tools-command', + { + toolset: 'docs', + tool: 'list', + event: 'tool:docs_list', + componentCount: 3, + success: true, + outcome: 'success', + client: 'sdk', + requestedMode: 'auto', + resolvedMode: 'attached', + attachMode: 'attached', + host: 'child', + duration: expect.any(Number), + }, + expect.anything(), + ], + ]); }); - it('does not reject the call when a forwarded telemetry sink throws synchronously', async () => { + it('ignores the telemetry envelope an older child host sends before its result', async () => { child.send.mockImplementation((message: { type: string; id?: string }) => { if (message.type === 'init') { queueMicrotask(() => child.emit('message', HELLO)); @@ -358,7 +321,7 @@ describe('spawnChildHost', () => { type: 'telemetry', id: message.id, event: 'tool:listAllDocumentation', - payload: { toolset: 'docs' }, + payload: { componentCount: 3 }, }); child.emit('message', { type: 'result', @@ -369,27 +332,21 @@ describe('spawnChildHost', () => { } return true; }); - const sink = vi.fn(() => { - throw new Error('telemetry down'); - }); const tools = await spawn(); - await expect(tools.call('docs.list', {}, { telemetry: sink })).resolves.toEqual({ + await expect(tools.call('docs.list', {})).resolves.toEqual({ ok: true, data: { ran: true }, markdown: 'ok', }); - expect(sink).toHaveBeenCalledWith( - 'tool:listAllDocumentation', - expect.objectContaining({ - toolset: 'docs', - client: 'sdk', - requestedMode: 'attached', - resolvedMode: 'attached', - attachMode: 'attached', - host: 'child', - }) - ); + expect(vi.mocked(telemetry).mock.calls).toEqual([ + [ + 'tools-command', + expect.objectContaining({ toolset: 'docs', tool: 'list', success: true }), + expect.anything(), + ], + ]); + expect(vi.mocked(telemetry).mock.calls[0][1]).not.toHaveProperty('componentCount'); }); it('sends a cancel envelope keyed by the call id when the signal aborts', async () => { diff --git a/code/core/src/cli/tools/sdk/child-client.ts b/code/core/src/cli/tools/sdk/child-client.ts index 58a43215fa67..efb0a399293e 100644 --- a/code/core/src/cli/tools/sdk/child-client.ts +++ b/code/core/src/cli/tools/sdk/child-client.ts @@ -6,21 +6,14 @@ import { deserializeError, type SerializedError, } from '../../../shared/open-service/service-error-serialization.ts'; -import type { - AnyToolsetOutcome, - ToolsetTelemetry, -} from '../../../shared/open-service/toolset-definition.ts'; +import type { InvokedToolsetOutcome } from '../../../shared/open-service/toolset-definition.ts'; import { CHILD_HOST_PROTOCOL_VERSION, isChildMessage, type ChildHelloMessage, type ParentMessage, } from './child-protocol.ts'; -import { - reportSdkInvocation, - resolveCallTelemetry, - toolsCommandDimensions, -} from './command-telemetry.ts'; +import { reportSdkInvocation } from './command-telemetry.ts'; import { AttachUnavailableError, EnvironmentMismatchError, @@ -140,7 +133,6 @@ export async function spawnChildHost( string, { resolve: (value: unknown) => void; reject: (error: unknown) => void } >(); - const pendingTelemetry = new Map(); let closed = false; let nextId = 0; @@ -180,15 +172,10 @@ export async function spawnChildHost( void disconnected.catch(() => {}); child.on('message', (raw: unknown) => { - if (!isChildMessage(raw) || raw.type === 'hello') { - return; - } - if (raw.type === 'telemetry') { - void Promise.resolve() - .then(() => pendingTelemetry.get(raw.id)?.(raw.event, raw.payload)) - .catch(() => { - // Method telemetry is never part of the call result. - }); + // Only a result or an error settles a call. A child host from an older Storybook still sends a + // per-call `telemetry` envelope first; treating it as the reply would reject the call and drop + // the result that follows. + if (!isChildMessage(raw) || (raw.type !== 'result' && raw.type !== 'error')) { return; } const waiter = pending.get(raw.id); @@ -273,13 +260,6 @@ export async function spawnChildHost( } }; - const dimensions = toolsCommandDimensions({ - clientInfo: args.clientInfo, - requestedMode: args.requestedMode, - resolvedMode, - host: 'child', - }); - return { mode: resolvedMode, host: 'child', @@ -295,17 +275,10 @@ export async function spawnChildHost( ref: string, input: Record = {}, options: ToolsCallOptions = {} - ): Promise { + ): Promise { assertOpen(); options.signal?.throwIfAborted(); const id = String(++nextId); - const telemetry = resolveCallTelemetry(options, dimensions, { - clientInfo: args.clientInfo, - configDir: hello.storybook.configDir, - }); - if (telemetry) { - pendingTelemetry.set(id, telemetry); - } let onAbort: (() => void) | undefined; const aborted = options.signal ? new Promise((_, reject) => { @@ -325,7 +298,7 @@ export async function spawnChildHost( const work = request({ type: 'call', id, ref, input }); const outcome = (await (aborted ? Promise.race([work, aborted]) - : work)) as AnyToolsetOutcome; + : work)) as InvokedToolsetOutcome; await reportSdkInvocation({ ref, clientInfo: args.clientInfo, @@ -333,6 +306,7 @@ export async function spawnChildHost( resolvedMode, host: 'child', result: outcome, + report: outcome.telemetry, duration: Date.now() - start, configDir: hello.storybook.configDir, }); @@ -350,7 +324,6 @@ export async function spawnChildHost( }); throw error; } finally { - pendingTelemetry.delete(id); if (onAbort) { options.signal?.removeEventListener('abort', onAbort); } diff --git a/code/core/src/cli/tools/sdk/child-host.test.ts b/code/core/src/cli/tools/sdk/child-host.test.ts index 78ab7162a13b..060c46630ef7 100644 --- a/code/core/src/cli/tools/sdk/child-host.test.ts +++ b/code/core/src/cli/tools/sdk/child-host.test.ts @@ -17,10 +17,7 @@ describe('runChildHost', () => { async ( _ref: string, _input?: Record, - options?: { - signal?: AbortSignal; - telemetry?: (event: string, payload: Record) => Promise; - } + options?: { signal?: AbortSignal } ): Promise => { if (options?.signal?.aborted) { throw new Error('aborted'); @@ -169,11 +166,14 @@ describe('runChildHost', () => { exit.mockRestore(); }); - it('forwards method telemetry over IPC keyed by the call id', async () => { - call.mockImplementation(async (_ref, _input, options) => { - await options?.telemetry?.('tool:listAllDocumentation', { toolset: 'docs' }); - return { ok: true, data: { ran: true }, markdown: 'ok' }; - }); + it('sends the outcome, report included, as the call result', async () => { + const outcome = { + ok: true as const, + data: { ran: true }, + markdown: 'ok', + telemetry: { payload: { componentCount: 3 } }, + }; + call.mockResolvedValue(outcome); await runChildHost({ send, subscribe: (handler) => { @@ -186,15 +186,7 @@ describe('runChildHost', () => { handlers[0]({ type: 'call', id: 'call-7', ref: 'docs.list', input: {} }); await vi.waitFor(() => - expect(send).toHaveBeenCalledWith({ - type: 'telemetry', - id: 'call-7', - event: 'tool:listAllDocumentation', - payload: { toolset: 'docs' }, - }) - ); - await vi.waitFor(() => - expect(send).toHaveBeenCalledWith(expect.objectContaining({ type: 'result', id: 'call-7' })) + expect(send).toHaveBeenCalledWith({ type: 'result', id: 'call-7', value: outcome }) ); }); diff --git a/code/core/src/cli/tools/sdk/child-host.ts b/code/core/src/cli/tools/sdk/child-host.ts index 70586429e948..2d50e410871c 100644 --- a/code/core/src/cli/tools/sdk/child-host.ts +++ b/code/core/src/cli/tools/sdk/child-host.ts @@ -59,12 +59,7 @@ export async function runChildHost({ controllers.set(message.id, controller); try { await reply(message.id, tools, () => - tools!.call(message.ref, message.input, { - signal: controller.signal, - telemetry: async (event, payload) => { - send({ type: 'telemetry', id: message.id, event, payload }); - }, - }) + tools!.call(message.ref, message.input, { signal: controller.signal }) ); } finally { controllers.delete(message.id); diff --git a/code/core/src/cli/tools/sdk/child-protocol.ts b/code/core/src/cli/tools/sdk/child-protocol.ts index 899b2046f0ee..02810adfddd8 100644 --- a/code/core/src/cli/tools/sdk/child-protocol.ts +++ b/code/core/src/cli/tools/sdk/child-protocol.ts @@ -27,18 +27,7 @@ export type ChildErrorMessage = { error: SerializedError; }; -export type ChildTelemetryMessage = { - type: 'telemetry'; - id: string; - event: string; - payload: Record; -}; - -export type ChildMessage = - | ChildHelloMessage - | ChildResultMessage - | ChildErrorMessage - | ChildTelemetryMessage; +export type ChildMessage = ChildHelloMessage | ChildResultMessage | ChildErrorMessage; export type ParentInitMessage = { type: 'init'; diff --git a/code/core/src/cli/tools/sdk/command-telemetry.test.ts b/code/core/src/cli/tools/sdk/command-telemetry.test.ts index 3885da9ee9d5..dc90a35861ba 100644 --- a/code/core/src/cli/tools/sdk/command-telemetry.test.ts +++ b/code/core/src/cli/tools/sdk/command-telemetry.test.ts @@ -1,12 +1,16 @@ -import { afterEach, describe, expect, it, vi } from 'vitest'; +import { telemetry } from 'storybook/internal/telemetry'; + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { - commandNameFromRef, + commandPartsFromRef, + reportToolsCommandEvent, shouldReportSdkInvocation, toolsCommandDimensions, - wrapMethodTelemetry, } from './command-telemetry.ts'; +vi.mock('storybook/internal/telemetry', { spy: true }); + describe('toolsCommandDimensions', () => { it('uses resolvedMode for attachMode when a host exists', () => { expect( @@ -41,38 +45,106 @@ describe('toolsCommandDimensions', () => { }); }); -describe('commandNameFromRef', () => { - it('turns a dotted method id into the CLI spelling', () => { - expect(commandNameFromRef('docs.list')).toBe('docs list'); - expect(commandNameFromRef('stories.findByComponent')).toBe('stories find-by-component'); +describe('commandPartsFromRef', () => { + it('splits a dotted method id into the CLI spelling of both parts', () => { + expect(commandPartsFromRef('docs.list')).toEqual({ toolset: 'docs', tool: 'list' }); + expect(commandPartsFromRef('stories.findByComponent')).toEqual({ + toolset: 'stories', + tool: 'find-by-component', + }); }); it('collapses a malformed reference', () => { - expect(commandNameFromRef('not-a-ref')).toBe('(invalid)'); + expect(commandPartsFromRef('not-a-ref')).toEqual({ toolset: '(invalid)', tool: '(invalid)' }); }); -}); -describe('wrapMethodTelemetry', () => { - it('merges host dimensions under the method payload', async () => { - const sink = vi.fn(async () => {}); - const wrapped = wrapMethodTelemetry(sink, { - client: 'sdk', - requestedMode: 'auto', - resolvedMode: 'attached', - attachMode: 'attached', - host: 'child', + it('collapses a part that is not a name-shaped token', () => { + expect(commandPartsFromRef('my project.list')).toEqual({ + toolset: '(invalid)', + tool: 'list', }); + }); +}); - await wrapped('tool:listAllDocumentation', { toolset: 'docs' }); +describe('reportToolsCommandEvent', () => { + beforeEach(() => { + vi.mocked(telemetry).mockReset(); + vi.mocked(telemetry).mockResolvedValue(undefined); + }); - expect(sink).toHaveBeenCalledWith('tool:listAllDocumentation', { - client: 'sdk', + it('merges the report under the record and names the tool by the report', async () => { + await reportToolsCommandEvent( + { + toolset: 'stories', + tool: 'findByComponent', + success: true, + outcome: 'success', + client: 'cli', + requestedMode: 'auto', + attachMode: 'local', + }, + { + report: { + toolset: 'stories', + tool: 'find-by-component', + event: 'tool:stories_findByComponent', + payload: { componentCount: 2, success: 'not a record field' }, + }, + configDir: '/repo/.storybook', + } + ); + + expect(telemetry).toHaveBeenCalledWith( + 'tools-command', + { + toolset: 'stories', + tool: 'find-by-component', + event: 'tool:stories_findByComponent', + componentCount: 2, + success: true, + outcome: 'success', + client: 'cli', + requestedMode: 'auto', + attachMode: 'local', + }, + { configDir: '/repo/.storybook' } + ); + }); + + it('sends the record alone when the run produced no report', async () => { + await reportToolsCommandEvent({ + success: false, + outcome: 'intercept', + client: 'cli', requestedMode: 'auto', - resolvedMode: 'attached', - attachMode: 'attached', - host: 'child', - toolset: 'docs', + attachMode: 'auto', }); + + expect(telemetry).toHaveBeenCalledWith( + 'tools-command', + { + success: false, + outcome: 'intercept', + client: 'cli', + requestedMode: 'auto', + attachMode: 'auto', + }, + { configDir: undefined } + ); + }); + + it('never lets a telemetry failure escape', async () => { + vi.mocked(telemetry).mockRejectedValue(new Error('offline')); + + await expect( + reportToolsCommandEvent({ + success: true, + outcome: 'success', + client: 'sdk', + requestedMode: 'local', + attachMode: 'local', + }) + ).resolves.toBeUndefined(); }); }); diff --git a/code/core/src/cli/tools/sdk/command-telemetry.ts b/code/core/src/cli/tools/sdk/command-telemetry.ts index 6e3627683fd5..d14a67bb6ef1 100644 --- a/code/core/src/cli/tools/sdk/command-telemetry.ts +++ b/code/core/src/cli/tools/sdk/command-telemetry.ts @@ -1,12 +1,12 @@ import { telemetry } from 'storybook/internal/telemetry'; -import type { ToolsetTelemetry } from '../../../shared/open-service/toolset-definition.ts'; +import type { ToolsetMethodReport } from '../../../shared/open-service/toolset-definition.ts'; import { parseToolsetMethodId, toCliMethodName, } from '../../../shared/open-service/toolset-names.ts'; import { attachGateReasonFromError, type ToolsAttachGateReason } from './errors.ts'; -import type { ToolsCallOptions, ToolsClientInfo, ToolsHostKind, ToolsMode } from './types.ts'; +import type { ToolsClientInfo, ToolsHostKind, ToolsMode } from './types.ts'; export type ToolsCommandOutcomeKind = 'success' | 'failure' | 'intercept' | 'error' | 'attach-gate'; @@ -19,14 +19,29 @@ export type ToolsCommandDimensions = { attachGate?: ToolsAttachGateReason; }; +// One record per CLI or SDK invocation; `toolset` and `tool` are absent when no part was parsed. export type ToolsCommandTelemetryPayload = ToolsCommandDimensions & { - command: string; + toolset?: string; + tool?: string; success: boolean; outcome: ToolsCommandOutcomeKind; interceptReason?: string; + multipleMatches?: boolean; duration?: number; }; +/** The record as sent: the run's fields plus the handler's event name and payload, when it ran. */ +export type ToolsCommandTelemetryRecord = ToolsCommandTelemetryPayload & { + event?: string; + [field: string]: unknown; +}; + +// Names are a fixed vocabulary of short identifiers; anything else is arbitrary agent input (a +// typo'd path, a stray flag value) that must not be sent verbatim. +export function sanitizeNamePart(part: string): string { + return /^[\w-]{1,64}$/.test(part) ? part : '(invalid)'; +} + export function toolsCommandDimensions(args: { clientInfo: Pick, 'kind'>; requestedMode: ToolsMode; @@ -44,53 +59,36 @@ export function toolsCommandDimensions(args: { }; } -export function commandNameFromRef(ref: string): string { +export function commandPartsFromRef(ref: string): { toolset: string; tool: string } { try { const { toolsetId, methodName } = parseToolsetMethodId(ref); - return `${toolsetId} ${toCliMethodName(methodName)}`; + return { + toolset: sanitizeNamePart(toolsetId), + tool: sanitizeNamePart(toCliMethodName(methodName)), + }; } catch { - return '(invalid)'; - } -} - -export function wrapMethodTelemetry( - sink: ToolsetTelemetry, - dimensions: ToolsCommandDimensions -): ToolsetTelemetry { - return async (event, payload) => { - await sink(event, { ...dimensions, ...payload }); - }; -} - -export function defaultMethodTelemetrySink(configDir?: string): ToolsetTelemetry { - return async (event, payload) => { - await telemetry('tools-command', { event, ...payload }, { configDir }); - }; -} - -export function resolveCallTelemetry( - options: ToolsCallOptions, - dimensions: ToolsCommandDimensions, - args: { clientInfo: Pick, 'kind'>; configDir?: string } -): ToolsetTelemetry | undefined { - const isChildHost = process.env.STORYBOOK_TOOLS_CHILD_HOST === 'true'; - const sink = - options.telemetry ?? - (!isChildHost && shouldReportSdkInvocation(args.clientInfo.kind) - ? defaultMethodTelemetrySink(args.configDir) - : undefined); - if (!sink) { - return undefined; + return { toolset: '(invalid)', tool: '(invalid)' }; } - return isChildHost ? sink : wrapMethodTelemetry(sink, dimensions); } +// The record describes the run and wins over the handler's payload; the report names the method +// and wins over whatever the caller parsed, so a record always carries the registered spelling. export async function reportToolsCommandEvent( - payload: ToolsCommandTelemetryPayload, - options?: { configDir?: string } + record: ToolsCommandTelemetryPayload, + options: { report?: ToolsetMethodReport; configDir?: string } = {} ): Promise { + const { report, configDir } = options; + const payload: ToolsCommandTelemetryRecord = report + ? { + ...report.payload, + ...record, + event: report.event, + toolset: report.toolset, + tool: report.tool, + } + : record; try { - await telemetry('tools-command', payload, options); + await telemetry('tools-command', payload, { configDir }); } catch { // Telemetry is never part of the tool's result contract. } @@ -112,7 +110,6 @@ export async function reportSdkAttachGate(args: { const attachGate = attachGateReasonFromError(args.error); await reportToolsCommandEvent( { - command: '(none)', success: false, outcome: 'attach-gate', ...toolsCommandDimensions({ @@ -133,6 +130,7 @@ export async function reportSdkInvocation(args: { host: ToolsHostKind; fallbackReason?: ToolsAttachGateReason; result: { ok: boolean } | { error: unknown }; + report?: ToolsetMethodReport; duration: number; configDir?: string; }): Promise { @@ -140,30 +138,32 @@ export async function reportSdkInvocation(args: { return; } const dimensions = toolsCommandDimensions(args); + const invoked = commandPartsFromRef(args.ref); + const options = { report: args.report, configDir: args.configDir }; if (!('ok' in args.result)) { const attachGate = attachGateReasonFromError(args.result.error); await reportToolsCommandEvent( { - command: commandNameFromRef(args.ref), + ...invoked, success: false, outcome: attachGate ? 'attach-gate' : 'error', duration: args.duration, ...dimensions, ...(attachGate ? { attachGate } : {}), }, - { configDir: args.configDir } + options ); return; } const success = args.result.ok; await reportToolsCommandEvent( { - command: commandNameFromRef(args.ref), + ...invoked, success, outcome: success ? 'success' : 'failure', duration: args.duration, ...dimensions, }, - { configDir: args.configDir } + options ); } diff --git a/code/core/src/cli/tools/sdk/create-tools.test.ts b/code/core/src/cli/tools/sdk/create-tools.test.ts index 9e4c51bc5615..4b33b9ae8c0b 100644 --- a/code/core/src/cli/tools/sdk/create-tools.test.ts +++ b/code/core/src/cli/tools/sdk/create-tools.test.ts @@ -55,6 +55,17 @@ const echo = defineToolset({ input: v.object({}), handler: async () => ({ ok: true as const, data: {}, markdown: '' }), }, + counted: { + title: 'Report a count', + description: 'Reports usage, then succeeds.', + input: v.object({}), + handler: async () => ({ + ok: true as const, + data: {}, + markdown: '', + telemetry: { payload: { itemCount: 2 } }, + }), + }, slow: { title: 'Delay', description: 'Resolves after a tick unless aborted.', @@ -547,8 +558,7 @@ describe('createTools', () => { await expect(spawnFailed).rejects.toThrow('Could not resolve the `storybook` package'); }); - it('applies per-call origin and telemetry to the method context', async () => { - const sink = vi.fn(async () => {}); + it('applies the per-call origin and returns the named report on the outcome', async () => { vi.mocked(bootstrapToolsRuntime).mockResolvedValue( makeRuntime({ toolsets: [ @@ -560,14 +570,12 @@ describe('createTools', () => { title: 'Ping', description: 'ping', input: v.object({}), - handler: async (_input, ctx) => { - await ctx.telemetry?.('tool:ping', { toolset: 'probe' }); - return { - ok: true as const, - data: { origin: ctx.origin }, - markdown: ctx.origin ?? '', - }; - }, + handler: async (_input, ctx) => ({ + ok: true as const, + data: { origin: ctx.origin }, + markdown: ctx.origin ?? '', + telemetry: { payload: { pingCount: 1 } }, + }), }, }, }), @@ -576,24 +584,18 @@ describe('createTools', () => { ); const tools = await createTools({ mode: 'local' }); - const outcome = await tools.call( - 'probe.ping', - {}, - { origin: 'http://localhost:9', telemetry: sink } - ); + const outcome = await tools.call('probe.ping', {}, { origin: 'http://localhost:9' }); expect(outcome).toMatchObject({ data: { origin: 'http://localhost:9' } }); - expect(sink).toHaveBeenCalledWith( - 'tool:ping', - expect.objectContaining({ - toolset: 'probe', - client: 'sdk', - requestedMode: 'local', - resolvedMode: 'local', - attachMode: 'local', - host: 'in-process', - }) - ); + expect(outcome.telemetry).toEqual({ + toolset: 'probe', + tool: 'ping', + event: 'tool:probe_ping', + payload: { pingCount: 1 }, + }); + expect(invocationPayloads()).toEqual([ + expect.objectContaining({ toolset: 'probe', tool: 'ping', pingCount: 1 }), + ]); }); it('wraps a configuration that cannot be loaded', async () => { @@ -647,6 +649,7 @@ describe('describe', () => { ['echo.bad', false], ['echo.live', true], ['echo.sibling', false], + ['echo.counted', false], ['echo.slow', false], ]); }); @@ -840,10 +843,7 @@ describe('call', () => { function invocationPayloads(): unknown[] { return vi .mocked(telemetry) - .mock.calls.filter( - ([eventType, payload]) => - eventType === 'tools-command' && payload !== undefined && !('event' in payload) - ) + .mock.calls.filter(([eventType]) => eventType === 'tools-command') .map(([, payload]) => payload); } @@ -854,8 +854,9 @@ describe('tools-command telemetry', () => { await tools.call('echo.ok', { value: 'hello' }); expect(invocationPayloads()).toEqual([ - expect.objectContaining({ - command: 'echo ok', + { + toolset: 'echo', + tool: 'ok', success: true, outcome: 'success', client: 'sdk', @@ -864,7 +865,40 @@ describe('tools-command telemetry', () => { attachMode: 'local', host: 'in-process', duration: expect.any(Number), - }), + }, + ]); + }); + + it('merges the handler report into the one invocation record', async () => { + const tools = await createTools({ mode: 'local' }); + + await tools.call('echo.counted'); + + expect(invocationPayloads()).toEqual([ + { + toolset: 'echo', + tool: 'counted', + event: 'tool:echo_counted', + itemCount: 2, + success: true, + outcome: 'success', + client: 'sdk', + requestedMode: 'local', + resolvedMode: 'local', + attachMode: 'local', + host: 'in-process', + duration: expect.any(Number), + }, + ]); + }); + + it('spells the tool the way the CLI does', async () => { + const tools = await createTools({ mode: 'local' }); + + await tools.call('echo.findByComponent').catch(() => {}); + + expect(invocationPayloads()).toEqual([ + expect.objectContaining({ toolset: 'echo', tool: 'find-by-component', outcome: 'error' }), ]); }); @@ -875,7 +909,8 @@ describe('tools-command telemetry', () => { expect(invocationPayloads()).toEqual([ expect.objectContaining({ - command: 'echo bad', + toolset: 'echo', + tool: 'bad', success: false, outcome: 'failure', client: 'sdk', @@ -917,7 +952,8 @@ describe('tools-command telemetry', () => { expect(invocationPayloads()).toEqual([ expect.objectContaining({ - command: 'echo ok', + toolset: 'echo', + tool: 'ok', success: true, outcome: 'success', client: 'sdk', @@ -944,15 +980,14 @@ describe('tools-command telemetry', () => { ); expect(invocationPayloads()).toEqual([ - expect.objectContaining({ - command: '(none)', + { success: false, outcome: 'attach-gate', client: 'sdk', requestedMode: 'attached', attachMode: 'attached', attachGate: 'connection-failed', - }), + }, ]); }); diff --git a/code/core/src/cli/tools/sdk/create-tools.ts b/code/core/src/cli/tools/sdk/create-tools.ts index 3f1c3acf74db..1e9460427474 100644 --- a/code/core/src/cli/tools/sdk/create-tools.ts +++ b/code/core/src/cli/tools/sdk/create-tools.ts @@ -4,12 +4,13 @@ import { versions } from 'storybook/internal/common'; import { StorybookDevServerDisconnectedError } from '../../../server-errors.ts'; import { formatIssues } from '../../../shared/open-service/errors.ts'; -import type { - AnyToolsetDefinition, - AnyToolsetMethod, - AnyToolsetOutcome, - ToolsetCtx, - ToolsetTransport, +import { + invokeToolsetMethod, + type AnyToolsetDefinition, + type AnyToolsetMethod, + type InvokedToolsetOutcome, + type ToolsetCtx, + type ToolsetTransport, } from '../../../shared/open-service/toolset-definition.ts'; import { parseToolsetMethodId } from '../../../shared/open-service/toolset-names.ts'; import { projectPathsEqual } from '../instances/project-path.ts'; @@ -18,12 +19,7 @@ import type { AttachedBootstrapResult } from './attached-runtime.ts'; import { toCatalogEntry } from './catalog.ts'; import { formatAttachFallback } from './attach-messages.ts'; import { spawnChildHost } from './child-client.ts'; -import { - reportSdkAttachGate, - reportSdkInvocation, - resolveCallTelemetry, - toolsCommandDimensions, -} from './command-telemetry.ts'; +import { reportSdkAttachGate, reportSdkInvocation } from './command-telemetry.ts'; import { AttachUnavailableError, SpawnFailedError, @@ -378,11 +374,12 @@ function createToolsHost(args: { ref: string, input: Record, options: ToolsCallOptions - ): Promise => { + ): Promise => { options.signal?.throwIfAborted(); const { toolsetId, methodName } = splitRef(ref); - const method = findMethod(findToolset(runtime, toolsetId), methodName); + const toolset = findToolset(runtime, toolsetId); + const method = findMethod(toolset, methodName); if (mode === 'local' && method.requiresDevServer) { throw new AttachUnavailableError({ @@ -403,22 +400,13 @@ function createToolsHost(args: { return raceAbort( options.signal, - method.handler(validation.value, { + invokeToolsetMethod(toolset, methodName, validation.value, { ...baseCtx, ...(options.origin !== undefined ? { origin: options.origin } : {}), - ...(options.telemetry ? { telemetry: options.telemetry } : {}), }) ); }; - const dimensions = toolsCommandDimensions({ - clientInfo, - requestedMode, - resolvedMode: mode, - host, - fallbackReason: args.fallbackReason, - }); - return { mode, host, @@ -443,21 +431,13 @@ function createToolsHost(args: { ref: string, input: Record = {}, options: ToolsCallOptions = {} - ): Promise { + ): Promise { assertOpen(); - const telemetry = resolveCallTelemetry(options, dimensions, { - clientInfo, - configDir: runtime.configDir, - }); - const callOptions: ToolsCallOptions = { - ...options, - ...(telemetry ? { telemetry } : {}), - }; const start = Date.now(); try { const outcome = args.disconnected - ? await Promise.race([invoke(ref, input, callOptions), args.disconnected]) - : await invoke(ref, input, callOptions); + ? await Promise.race([invoke(ref, input, options), args.disconnected]) + : await invoke(ref, input, options); await reportSdkInvocation({ ref, clientInfo, @@ -466,6 +446,7 @@ function createToolsHost(args: { host, fallbackReason: args.fallbackReason, result: outcome, + report: outcome.telemetry, duration: Date.now() - start, configDir: runtime.configDir, }); diff --git a/code/core/src/cli/tools/sdk/index.ts b/code/core/src/cli/tools/sdk/index.ts index a6659357e98f..cc71f4750203 100644 --- a/code/core/src/cli/tools/sdk/index.ts +++ b/code/core/src/cli/tools/sdk/index.ts @@ -10,7 +10,6 @@ export { type ToolsAttachGateReason, type ToolsRuntimeErrorReason, } from './errors.ts'; -export { toolsCommandDimensions, wrapMethodTelemetry } from './command-telemetry.ts'; export type { ToolsRuntime } from './local-runtime.ts'; export type { ToolsetJsonSchema } from './json-schema.ts'; export { formatMultiInstanceNotice } from './attach-messages.ts'; diff --git a/code/core/src/cli/tools/sdk/types.ts b/code/core/src/cli/tools/sdk/types.ts index 36a353fffcf8..52fb2689f3dc 100644 --- a/code/core/src/cli/tools/sdk/types.ts +++ b/code/core/src/cli/tools/sdk/types.ts @@ -1,7 +1,4 @@ -import type { - AnyToolsetOutcome, - ToolsetTelemetry, -} from '../../../shared/open-service/toolset-definition.ts'; +import type { InvokedToolsetOutcome } from '../../../shared/open-service/toolset-definition.ts'; import type { ToolsetMethodId } from '../../../shared/open-service/toolset-names.ts'; import type { ToolsAttachGateReason } from './errors.ts'; import type { ToolsetJsonSchema } from './json-schema.ts'; @@ -101,7 +98,6 @@ export type ToolsCallOptions = { signal?: AbortSignal; /** Overrides the host's Storybook origin for this call. */ origin?: string; - telemetry?: ToolsetTelemetry; }; type ToolsBase = { @@ -133,7 +129,7 @@ type ToolsBase = { ref: string, input?: Record, options?: ToolsCallOptions - ): Promise; + ): Promise; close(): Promise; }; diff --git a/code/core/src/shared/open-service/README.md b/code/core/src/shared/open-service/README.md index 20e5a970b705..bc0d6a42e54f 100644 --- a/code/core/src/shared/open-service/README.md +++ b/code/core/src/shared/open-service/README.md @@ -110,18 +110,18 @@ and synchronization; **toolsets** are the public agent surface for CLI and MCP a narrowed to it. An outcome's `data` may carry more than this declares; only the declared shape reaches the wire - `handler(input, ctx)` — the one execution: produces the data, renders the text, and owns side - effects and telemetry + effects and the usage report `handler` returns a `ToolsetOutcome` — a discriminated union of -`{ ok: true, data, markdown }` and `{ ok: false, data, markdown }`, written as plain object -literals (no factory helpers). The failure model is one line each: could not do the job → +`{ ok: true, data, markdown }` and `{ ok: false, data, markdown }`, each with an optional +`telemetry` usage report, written as plain object literals (no factory helpers). The failure model is one line each: could not do the job → **throw**; did the job and the answer is bad news (a failed test run, a not-found lookup) → **return `{ ok: false, data, markdown }`**. Infallible methods declare `TFailure = never`. One handler owns data and rendering because one MCP reply 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. Adapters unwrap outcomes mechanically — text blocks +side effects would repeat them. The usage report is part of the returned outcome, built with the +rendered text in hand, so no consumer can forget it. Adapters unwrap outcomes mechanically — text blocks from `markdown`, `structuredContent` from `data`, MCP `isError` (and later CLI exit codes) from `ok` — and must not re-derive meaning from the data. `markdown` may be `string | string[]`: the CLI joins the blocks with blank lines, and its `--json` flag means "print `data`, skip @@ -131,7 +131,7 @@ An error whose message speaks to the agent and names its own recovery declares ` (a `StorybookError` constructor prop); adapters surface such errors verbatim by reading that property — never by keeping a class list, which misclassifies across bundle copies. -`ctx` is `{ transport: 'cli' | 'mcp' | 'sdk', origin?, getService, telemetry? }`. `origin` is the complete +`ctx` is `{ transport: 'cli' | 'mcp' | 'sdk', origin?, getService }`. `origin` is the complete Storybook UI base URL, including a deployment subpath; the MCP adapter derives it from the request. Descriptions that name a sibling tool must render it through `getToolName(ctx)` rather than hardcoding a spelling, so the same sentence reads as the derived MCP tool name, the CLI command, or the SDK @@ -151,8 +151,14 @@ Adapters resolve one toolset with `getToolset(id)` or take the whole set via consume them today. Telemetry classification belongs in Storybook-owned telemetry calls, not on the generic toolset -definition. Use `reportToolsetTelemetry` so a rejected analytics sink cannot fail a tool call. -Third-party toolsets do not need to participate in Storybook's telemetry taxonomy. +definition. A handler returns at most one usage report on its outcome, `telemetry: { payload }`. +`invokeToolsetMethod` — the one way every surface runs a method — names the report after the +registration: `toolset`, `tool` in CLI spelling, and the generated `event` +(`tool:stories_findByComponent`). The CLI and SDK forward that report as their `tools-command` +record. The MCP adapter forwards it as its `addon-mcp` event, but under the pre-toolset event name +the addon keeps per tool (`tool:getStoriesByComponent`), so MCP usage data stays continuous across +versions. Toolsets never name events themselves. Third-party toolsets do not need to participate in +Storybook's telemetry taxonomy. Core owns `docs`, `stories`, and `review`. Addon-vitest owns the complete `test` toolset—its schemas, channel protocol, formatting, telemetry, and tests—and registers it beside its responder from the diff --git a/code/core/src/shared/open-service/index.ts b/code/core/src/shared/open-service/index.ts index b32a49107925..cef11cfabd79 100644 --- a/code/core/src/shared/open-service/index.ts +++ b/code/core/src/shared/open-service/index.ts @@ -10,20 +10,22 @@ export { seedQueryState } from './query-state.ts'; export { defineToolset, - reportToolsetTelemetry, + invokeToolsetMethod, resolveToolsetDescription, } from './toolset-definition.ts'; export type { AnyToolsetDefinition, AnyToolsetOutcome, + InvokedToolsetOutcome, ToolsetCtx, ToolsetDefinition, ToolsetGetService, ToolsetMethod, ToolsetMethodDescription, ToolsetObjectOutputSchema, + ToolsetMethodReport, ToolsetOutcome, - ToolsetTelemetry, + ToolsetTelemetryReport, ToolsetTransport, } from './toolset-definition.ts'; export { getToolName, parseToolsetMethodId, toMcpToolName } from './toolset-names.ts'; diff --git a/code/core/src/shared/open-service/toolset-definition.ts b/code/core/src/shared/open-service/toolset-definition.ts index 9e752525ec84..58d392c10d97 100644 --- a/code/core/src/shared/open-service/toolset-definition.ts +++ b/code/core/src/shared/open-service/toolset-definition.ts @@ -1,5 +1,6 @@ import type { StandardSchemaV1 } from '@standard-schema/spec'; +import { toCliMethodName } from './toolset-names.ts'; import type { GetServiceOptions } from './types.ts'; type AnySchema = StandardSchemaV1; @@ -14,15 +15,6 @@ 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 transport has telemetry disabled. - */ -export type ToolsetTelemetry = (event: string, payload: Record) => Promise; - export type ToolsetCtx = { transport: ToolsetTransport; /** @@ -31,9 +23,16 @@ export type ToolsetCtx = { */ origin?: string; getService: ToolsetGetService; - telemetry?: ToolsetTelemetry; }; +/** + * A handler's usage report: the payload describing what the call did, counts and flags alike. It + * travels on the outcome, so a handler reports at most once and in the same object as its data. + * The surface that ran the method turns it into its own telemetry record and names the event; a + * handler never names one. + */ +export type ToolsetTelemetryReport = { payload: Record }; + /** * A method description, resolved per transport. * @@ -45,8 +44,8 @@ export type ToolsetCtx = { 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 result of one method run: the tag, the structured data, the rendered Markdown, and the usage + * report, 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 @@ -60,16 +59,22 @@ export type ToolsetMethodDescription = string | ((context: ToolsetCtx) => string * `markdown` may be multiple strings: MCP renders each as its own text block (`stories-preview` * renders one block per URL), the CLI joins them with newlines. */ -export type ToolsetOutcome = +export type ToolsetOutcome< + TSuccess, + TFailure = TSuccess, + TReport extends ToolsetTelemetryReport = ToolsetTelemetryReport, +> = | { readonly ok: true; readonly data: TSuccess; readonly markdown: string | string[]; + readonly telemetry?: TReport; } | { readonly ok: false; readonly data: TFailure; readonly markdown: string | string[]; + readonly telemetry?: TReport; }; // `any` permits heterogeneous outcome maps. Each individual method remains typed by `defineToolset`. @@ -85,11 +90,11 @@ export type ToolsetObjectOutputSchema = StandardSchemaV1< /** * 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. + * The handler produces the whole {@link ToolsetOutcome} — data, side effects, the usage report, + * 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. The usage report is part of the returned object, with the + * rendered text in hand, so no consumer can forget it. */ export type ToolsetMethod< TSchema extends AnySchema = AnySchema, @@ -187,21 +192,42 @@ export function resolveToolsetDescription( } /** - * Reports best-effort telemetry without allowing analytics failures to fail the tool call. - * - * Analytics event names (`tool:previewStories`, …) and payload classifiers (`toolset: 'dev' | - * 'docs' | 'test'`) are a frozen cross-version contract. Keep them aligned with older Storybook - * releases even when MCP wire tool names or toolset ids change. The channel field is `transport` - * (`'cli' | 'mcp' | 'sdk'`), matching the toolset API. + * A handler's report completed from where the method is registered: the CLI spelling of the + * invoked names (`stories`, `find-by-component`) and the generated event name + * (`tool:stories_findByComponent`). */ -export async function reportToolsetTelemetry( - context: ToolsetCtx, - event: string, - payload: Record -): Promise { - try { - await context.telemetry?.(event, payload); - } catch { - // Telemetry is never part of the tool's result contract. +export type ToolsetMethodReport = ToolsetTelemetryReport & { + toolset: string; + tool: string; + event: string; +}; + +// `any` for the same reason as {@link AnyToolsetOutcome}: surfaces dispatch over every method. +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export type InvokedToolsetOutcome = ToolsetOutcome; + +/** + * Runs one method the way every surface does. The report is named after the registration, never + * by the handler, so a method cannot report a toolset that disagrees with where it lives. Every + * surface forwards the report as is and adds only its own fields. + */ +export async function invokeToolsetMethod( + toolset: AnyToolsetDefinition, + methodName: string, + input: unknown, + context: ToolsetCtx +): Promise { + const { telemetry, ...outcome } = await toolset.methods[methodName].handler(input, context); + if (!telemetry) { + return outcome; } + return { + ...outcome, + telemetry: { + ...telemetry, + toolset: toolset.id, + tool: toCliMethodName(methodName), + event: `tool:${toolset.id}_${methodName}`, + }, + }; } diff --git a/code/core/src/shared/open-service/toolsets/docs/access-parity.test.ts b/code/core/src/shared/open-service/toolsets/docs/access-parity.test.ts index 007ff3bc426c..6b619223c0e0 100644 --- a/code/core/src/shared/open-service/toolsets/docs/access-parity.test.ts +++ b/code/core/src/shared/open-service/toolsets/docs/access-parity.test.ts @@ -173,7 +173,7 @@ function manifestToolset() { } async function renderList(toolset: ReturnType, withStoryIds: boolean) { - return (await toolset.methods.list.handler({ withStoryIds }, ctx)).markdown; + return (await toolset.methods.list.handler({ withStoryIds })).markdown; } async function renderShow(toolset: ReturnType, id: string) { 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 ec8967e4732a..e5322411a131 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 @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; -import type { ToolsetCtx } from '../../toolset-definition.ts'; +import { invokeToolsetMethod, type ToolsetCtx } from '../../toolset-definition.ts'; import type { DocsAccess } from './access.ts'; import { createDocsToolset } from './definition.ts'; @@ -49,7 +49,7 @@ const cliCtx: ToolsetCtx = { transport: 'cli', getService: () => ({}) as never } describe('docs.list', () => { it('returns the manifests from the access and renders the list Markdown', async () => { - const outcome = await toolset.methods.list.handler({ withStoryIds: false }, mcpCtx); + const outcome = await toolset.methods.list.handler({ withStoryIds: false }); expect(outcome.ok).toBe(true); expect(Object.keys(outcome.data.manifests!.componentManifest.components)).toEqual(['button']); @@ -59,7 +59,7 @@ describe('docs.list', () => { }); it('includes story ids only when requested', async () => { - const outcome = await toolset.methods.list.handler({ withStoryIds: true }, mcpCtx); + const outcome = await toolset.methods.list.handler({ withStoryIds: true }); expect(outcome.markdown).toContain('button--primary'); }); @@ -272,40 +272,84 @@ describe('docs.showStory in a composition', () => { }); describe('usage reporting', () => { - /** Runs a method the way a transport does; the handler reports usage inline. */ - async function run(methodName: 'list' | 'show', input: unknown, transport: 'cli' | 'mcp') { - const events: Array<[string, Record]> = []; - const ctx: ToolsetCtx = { - transport, - getService: () => ({}) as never, - telemetry: async (event, payload) => { - events.push([event, payload]); + async function run(methodName: 'list' | 'show' | 'showStory', input: unknown) { + return (await invokeToolsetMethod(toolset, methodName, input, mcpCtx)).telemetry; + } + + it('reports a listing', async () => { + const report = await run('list', { withStoryIds: false }); + + expect(report).toEqual({ + toolset: 'docs', + tool: 'list', + event: 'tool:docs_list', + payload: { + componentCount: 1, + docsCount: 1, + resultTokenCount: expect.any(Number), + sourceCount: undefined, }, - }; + }); + }); - await toolset.methods[methodName].handler(input as never, ctx); + it('reports a lookup', async () => { + const report = await run('show', { id: 'button' }); - return events; - } + expect(report).toEqual({ + toolset: 'docs', + tool: 'show', + event: 'tool:docs_show', + payload: { componentId: 'button', found: true, resultTokenCount: expect.any(Number) }, + }); + }); - it.each(['cli', 'mcp'] as const)('reports a listing on %s', async (transport) => { - const [[event, payload] = []] = await run('list', { withStoryIds: false }, transport); + it('reports a miss as not found', async () => { + const report = await run('show', { id: 'nope' }); - expect(event).toBe('tool:listAllDocumentation'); - expect(payload).toMatchObject({ componentCount: 1, docsCount: 1 }); - expect(payload!.resultTokenCount).toBeGreaterThan(0); + expect(report?.payload).toMatchObject({ componentId: 'nope', found: false }); }); - it.each(['cli', 'mcp'] as const)('reports a lookup on %s', async (transport) => { - const [[event, payload] = []] = await run('show', { id: 'button' }, transport); + it('reports a story lookup by id', async () => { + const report = await run('showStory', { storyId: 'button--primary' }); + + expect(report).toEqual({ + toolset: 'docs', + tool: 'show-story', + event: 'tool:docs_showStory', + payload: { + found: true, + storyId: 'button--primary', + lookup: 'storyId', + resultTokenCount: expect.any(Number), + }, + }); + }); + + it('reports a story lookup by name with the resolved story id', async () => { + const report = await run('showStory', { componentId: 'button', storyName: 'Primary' }); - expect(event).toBe('tool:getDocumentation'); - expect(payload).toMatchObject({ componentId: 'button', found: true }); + expect(report?.payload).toEqual({ + found: true, + storyId: 'button--primary', + lookup: 'name', + resultTokenCount: expect.any(Number), + }); }); - it('reports a miss as not found', async () => { - const [[, payload] = []] = await run('show', { id: 'nope' }, 'mcp'); + it('reports a missing story with the requested id', async () => { + const report = await run('showStory', { storyId: 'button--nope' }); + + expect(report?.payload).toMatchObject({ + found: false, + storyId: 'button--nope', + lookup: 'storyId', + }); + }); + + it('reports a missing component without a story id', async () => { + const report = await run('showStory', { componentId: 'nope', storyName: 'Primary' }); - expect(payload).toMatchObject({ componentId: 'nope', found: false }); + expect(report?.payload).toMatchObject({ found: false, lookup: 'name' }); + expect(report?.payload.storyId).toBeUndefined(); }); }); 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 9e7b567cc20b..07fd925592b9 100644 --- a/code/core/src/shared/open-service/toolsets/docs/definition.ts +++ b/code/core/src/shared/open-service/toolsets/docs/definition.ts @@ -1,11 +1,6 @@ import * as v from 'valibot'; -import { - defineToolset, - reportToolsetTelemetry, - type ToolsetCtx, - type ToolsetOutcome, -} from '../../toolset-definition.ts'; +import { defineToolset, type ToolsetCtx, type ToolsetOutcome } from '../../toolset-definition.ts'; import { getToolName, type ToolsetMethodId } from '../../toolset-names.ts'; import type { DocsAccess, ResolvedDocsEntry } from './access.ts'; import { @@ -111,6 +106,7 @@ function resolveShow({ entry, sourceError }: DocsShowOutput): ShowResolution { } type ComponentEntry = Extract; +type ComponentStory = NonNullable[number]; /** The `showStory` counterpart of {@link ShowResolution}. */ type ShowStoryResolution = @@ -118,7 +114,7 @@ type ShowStoryResolution = | { kind: 'source-error'; message: string } | { kind: 'component-missing' } | { kind: 'story-missing'; component: ComponentEntry['component'] } - | { kind: 'found'; component: ComponentEntry['component']; storyName: string }; + | { kind: 'found'; component: ComponentEntry['component']; story: ComponentStory }; /** Whether a `showStory` input names a story at all: a story id, or a complete name pair. */ function isShowStorySelector({ storyId, componentId, storyName }: DocsShowStoryOutput): boolean { @@ -151,9 +147,7 @@ function resolveShowStory(data: DocsShowStoryOutput): ShowStoryResolution { storyId !== undefined ? component.stories?.find((candidate) => candidate.id === storyId) : component.stories?.find((candidate) => candidate.name === storyName); - return story - ? { kind: 'found', component, storyName: story.name } - : { kind: 'story-missing', component }; + return story ? { kind: 'found', component, story } : { kind: 'story-missing', component }; } /** @@ -217,8 +211,11 @@ function formatAvailableStories(stories: ComponentEntry['component']['stories']) } /** Pure renderer for `showStory`. */ -function renderShowStory(data: DocsShowStoryOutput, ctx: ToolsetCtx): string { - const resolution = resolveShowStory(data); +function renderShowStory( + resolution: ShowStoryResolution, + data: DocsShowStoryOutput, + ctx: ToolsetCtx +): string { switch (resolution.kind) { case 'input-invalid': return `Provide either \`storyId\`, or both \`componentId\` and \`storyName\`. Story ids are listed by the ${getToolName(ctx)(DOCS_METHOD_REFS.list)} tool with \`withStoryIds: true\` and in ${getToolName(ctx)(DOCS_METHOD_REFS.show)} output.`; @@ -235,7 +232,7 @@ function renderShowStory(data: DocsShowStoryOutput, ctx: ToolsetCtx): string { : `Story "${data.storyName}" not found for component "${data.componentId}". Available stories: ${availableStories}`; } case 'found': - return formatStoryDocumentation(resolution.component, resolution.storyName); + return formatStoryDocumentation(resolution.component, resolution.story.name); default: { const exhaustive: never = resolution; return exhaustive; @@ -359,7 +356,7 @@ export function createDocsToolset(options: CreateDocsToolsetOptions) { }), title: 'List All Documentation', description: describeList, - handler: async (input, ctx): Promise> => { + handler: async (input): Promise> => { const { withStoryIds } = input; const data: DocsListOutput = multiSource ? { withStoryIds, sources: await listSources(sources!, { withStoryIds }) } @@ -371,17 +368,23 @@ export function createDocsToolset(options: CreateDocsToolsetOptions) { // A listing of nothing but errors is not a usage signal, so nothing is counted then. const counted = selectReportedManifests(data); - if (counted) { - await reportToolsetTelemetry(ctx, 'tool:listAllDocumentation', { - toolset: 'docs', - componentCount: Object.keys(counted.componentManifest.components).length, - docsCount: Object.keys(counted.docsManifest?.docs ?? {}).length, - resultTokenCount: estimateTokens(markdown), - sourceCount: data.sources?.length, - }); - } - - return { ok: true, data, markdown }; + return { + ok: true, + data, + markdown, + ...(counted + ? { + telemetry: { + payload: { + componentCount: Object.keys(counted.componentManifest.components).length, + docsCount: Object.keys(counted.docsManifest?.docs ?? {}).length, + resultTokenCount: estimateTokens(markdown), + sourceCount: data.sources?.length, + }, + }, + } + : {}), + }; }, }, [DOCS_METHOD_NAMES.show]: { @@ -397,16 +400,16 @@ export function createDocsToolset(options: CreateDocsToolsetOptions) { const markdown = renderShow(data, ctx); - await reportToolsetTelemetry(ctx, 'tool:getDocumentation', { - toolset: 'docs', - componentId: id, - found: data.entry !== undefined, - resultTokenCount: estimateTokens(markdown), - }); - + const telemetry = { + payload: { + componentId: id, + found: data.entry !== undefined, + resultTokenCount: estimateTokens(markdown), + }, + }; return isDocsShowError(data) - ? { ok: false, data, markdown } - : { ok: true, data, markdown }; + ? { ok: false, data, markdown, telemetry } + : { ok: true, data, markdown, telemetry }; }, }, [DOCS_METHOD_NAMES.showStory]: { @@ -437,11 +440,20 @@ export function createDocsToolset(options: CreateDocsToolsetOptions) { ? { ...request, entry: await selected.access.resolve(resolveId) } : { ...request, sourceError: selected.sourceError }; - const markdown = renderShowStory(data, ctx); + const resolution = resolveShowStory(data); + const markdown = renderShowStory(resolution, data, ctx); - return isDocsShowStoryError(data) - ? { ok: false, data, markdown } - : { ok: true, data, markdown }; + const telemetry = { + payload: { + found: resolution.kind === 'found', + storyId: resolution.kind === 'found' ? resolution.story.id : storyId, + lookup: storyId !== undefined ? 'storyId' : 'name', + resultTokenCount: estimateTokens(markdown), + }, + }; + return resolution.kind === 'found' + ? { ok: true, data, markdown, telemetry } + : { ok: false, data, markdown, telemetry }; }, }, }, 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 504637c2b45e..07f6ed413120 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 @@ -5,7 +5,11 @@ import { OpenServiceMissingOriginError, OpenServiceUnknownStoryIdsError, } from '../../../../server-errors.ts'; -import { resolveToolsetDescription, type ToolsetCtx } from '../../toolset-definition.ts'; +import { + invokeToolsetMethod, + resolveToolsetDescription, + type ToolsetCtx, +} from '../../toolset-definition.ts'; import { reviewToolset } from './definition.ts'; const reviewUrl = 'http://localhost:6006/?path=/review/'; @@ -32,7 +36,9 @@ function createReview( overrides: Partial> = {}, ctx: ToolsetCtx = cliCtx ) { - return reviewToolset.methods.create.handler( + return invokeToolsetMethod( + reviewToolset, + 'create', v.parse(reviewToolset.methods.create.input, { ...input, ...overrides }), ctx ); @@ -55,6 +61,17 @@ beforeEach(() => { }); describe('review.create', () => { + it('reports what the published review contains', async () => { + const outcome = await createReview(); + + expect(outcome.telemetry).toEqual({ + toolset: 'review', + tool: 'create', + event: 'tool:review_create', + payload: { collectionCount: 1, storyCount: 1, changedFileCount: 1 }, + }); + }); + it('publishes the review and returns its page URL plus what it contains', async () => { const outcome = await createReview(); 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 fc55dcf5e379..a629d249057f 100644 --- a/code/core/src/shared/open-service/toolsets/review/definition.ts +++ b/code/core/src/shared/open-service/toolsets/review/definition.ts @@ -5,12 +5,7 @@ import { describeUnknownStoryIds, OpenServiceUnknownStoryIdsError, } from '../../../../server-errors.ts'; -import { - defineToolset, - reportToolsetTelemetry, - type ToolsetCtx, - type ToolsetOutcome, -} from '../../toolset-definition.ts'; +import { defineToolset, type ToolsetCtx, type ToolsetOutcome } from '../../toolset-definition.ts'; import { getToolName } from '../../toolset-names.ts'; import type { ReviewService } from '../../services/review/definition.ts'; @@ -176,20 +171,20 @@ export const reviewToolset = defineToolset({ 0 ); - await reportToolsetTelemetry(ctx, 'tool:displayReview', { - toolset: 'dev', - collectionCount, - storyCount, - changedFileCount: review.changedFiles.length, - }); - const data: ReviewCreateOutput = { reviewUrl: `${ctx.origin.replace(/\/$/, '')}/?path=${REVIEW_PAGE_PATH}`, collectionCount, storyCount, }; - return { ok: true, data, markdown: formatReviewApplied(data, ctx) }; + return { + ok: true, + data, + markdown: formatReviewApplied(data, ctx), + telemetry: { + payload: { collectionCount, storyCount, changedFileCount: review.changedFiles.length }, + }, + }; }, }, }, 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 16e3152cfb5a..4de7f06b0ebc 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 @@ -13,7 +13,11 @@ import { OpenServiceModuleGraphUnavailableError, } from '../../../../server-errors.ts'; import { CHANGE_DETECTION_STATUS_TYPE_ID } from '../../../status-store/index.ts'; -import { resolveToolsetDescription, type ToolsetCtx } from '../../toolset-definition.ts'; +import { + invokeToolsetMethod, + resolveToolsetDescription, + type ToolsetCtx, +} from '../../toolset-definition.ts'; import { createStoriesToolset, type StoriesToolset } from './definition.ts'; vi.mock('node:fs', { spy: true }); @@ -55,7 +59,6 @@ const getStatuses = vi.fn(); const graphStatus = vi.fn(); const changeDetectionReadiness = vi.fn(); const storiesForFiles = vi.fn(); -const telemetry = vi.fn(); const cwd = vi.spyOn(process, 'cwd'); const moduleGraph = { queries: { @@ -89,11 +92,16 @@ function runPreview( ctx: ToolsetCtx = cliCtx, target: StoriesToolset = toolset ) { - return target.methods.preview.handler(v.parse(target.methods.preview.input, { stories }), ctx); + return invokeToolsetMethod( + target, + 'preview', + v.parse(target.methods.preview.input, { stories }), + ctx + ); } function runChanged(ctx: ToolsetCtx = cliCtx, target: StoriesToolset = toolset) { - return target.methods.changed.handler(v.parse(target.methods.changed.input, {}), ctx); + return invokeToolsetMethod(target, 'changed', v.parse(target.methods.changed.input, {}), ctx); } function runFindByComponent( @@ -101,7 +109,9 @@ function runFindByComponent( ctx: ToolsetCtx = cliCtx, target: StoriesToolset = toolset ) { - return target.methods.findByComponent.handler( + return invokeToolsetMethod( + target, + 'findByComponent', v.parse(target.methods.findByComponent.input, input), ctx ); @@ -133,7 +143,6 @@ beforeEach(() => { transport: 'cli', origin: 'http://localhost:6006', getService: vi.fn(() => moduleGraph) as ToolsetCtx['getService'], - telemetry, }; mcpCtx = { ...cliCtx, transport: 'mcp' }; getIndex.mockResolvedValue(index); @@ -188,12 +197,13 @@ describe('stories.preview', () => { }); it('reports the story counts it resolved', async () => { - await runPreview([{ storyId: 'button--primary' }, { storyId: 'gone--story' }]); + const outcome = await runPreview([{ storyId: 'button--primary' }, { storyId: 'gone--story' }]); - expect(telemetry).toHaveBeenCalledWith('tool:previewStories', { - toolset: 'dev', - inputStoryCount: 2, - outputStoryCount: 2, + expect(outcome.telemetry).toEqual({ + toolset: 'stories', + tool: 'preview', + event: 'tool:stories_preview', + payload: { inputStoryCount: 2, outputStoryCount: 2 }, }); }); @@ -319,14 +329,13 @@ describe('stories.changed', () => { it('reports the per-status counts', async () => { markChanged('button--primary', 'status-value:new'); - await runChanged(); + const outcome = await runChanged(); - expect(telemetry).toHaveBeenCalledWith('tool:getChangedStories', { - toolset: 'dev', - storyCount: 1, - newStoryCount: 1, - modifiedStoryCount: 0, - affectedStoryCount: 0, + expect(outcome.telemetry).toEqual({ + toolset: 'stories', + tool: 'changed', + event: 'tool:stories_changed', + payload: { storyCount: 1, newStoryCount: 1, modifiedStoryCount: 0, affectedStoryCount: 0 }, }); }); @@ -460,14 +469,13 @@ describe('stories.findByComponent', () => { }); it('reports how many of the requested components matched', async () => { - await runFindByComponent({ componentPaths: [componentPath, orphanPath] }); + const outcome = await runFindByComponent({ componentPaths: [componentPath, orphanPath] }); - expect(telemetry).toHaveBeenCalledWith('tool:getStoriesByComponent', { - toolset: 'dev', - componentCount: 2, - matchedComponentCount: 1, - totalMatchCount: 1, - maxDistance: 3, + expect(outcome.telemetry).toEqual({ + toolset: 'stories', + tool: 'find-by-component', + event: 'tool:stories_findByComponent', + payload: { componentCount: 2, matchedComponentCount: 1, totalMatchCount: 1, maxDistance: 3 }, }); }); 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 9f2282d7ac15..e11d507e807a 100644 --- a/code/core/src/shared/open-service/toolsets/stories/definition.ts +++ b/code/core/src/shared/open-service/toolsets/stories/definition.ts @@ -11,12 +11,7 @@ import type { ModuleGraphService, } from '../../services/module-graph/definition.ts'; import type { ModuleGraphIndexService } from '../../services/module-graph-index/definition.ts'; -import { - defineToolset, - reportToolsetTelemetry, - type ToolsetCtx, - type ToolsetOutcome, -} from '../../toolset-definition.ts'; +import { defineToolset, type ToolsetCtx, type ToolsetOutcome } from '../../toolset-definition.ts'; import { getToolName } from '../../toolset-names.ts'; import type { StatusesByStoryIdAndTypeId } from '../../../status-store/index.ts'; import { getChangedStories } from './changed.ts'; @@ -293,13 +288,17 @@ Use { absoluteStoryPath + exportName } only when you're already working in a spe stories: input.stories, }); - await reportToolsetTelemetry(ctx, 'tool:previewStories', { - toolset: 'dev', - inputStoryCount: input.stories.length, - outputStoryCount: data.stories.length, - }); - - return { ok: true, data, markdown: formatPreviewStories(data, ctx, { reviewEnabled }) }; + return { + ok: true, + data, + markdown: formatPreviewStories(data, ctx, { reviewEnabled }), + telemetry: { + payload: { + inputStoryCount: input.stories.length, + outputStoryCount: data.stories.length, + }, + }, + }; }, }, changed: { @@ -325,17 +324,18 @@ Use { absoluteStoryPath + exportName } only when you're already working in a spe if (changeDetection.status !== 'ready') { if (isGitUnusableReadiness(changeDetection)) { const data = emptyChangedStories(); - await reportToolsetTelemetry(ctx, 'tool:getChangedStories', { - toolset: 'dev', - storyCount: 0, - newStoryCount: 0, - modifiedStoryCount: 0, - affectedStoryCount: 0, - }); return { ok: true, data, markdown: formatChangedStories(data, ctx, { reviewEnabled }), + telemetry: { + payload: { + storyCount: 0, + newStoryCount: 0, + modifiedStoryCount: 0, + affectedStoryCount: 0, + }, + }, }; } throw new OpenServiceModuleGraphUnavailableError({ @@ -355,15 +355,19 @@ Use { absoluteStoryPath + exportName } only when you're already working in a spe unreachableFiles: await detectUnreachableFiles({ git, moduleGraph }), }; - await reportToolsetTelemetry(ctx, 'tool:getChangedStories', { - toolset: 'dev', - 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 }) }; + return { + ok: true, + data, + markdown: formatChangedStories(data, ctx, { reviewEnabled }), + telemetry: { + payload: { + storyCount: data.stories.length, + newStoryCount: data.counts.new, + modifiedStoryCount: data.counts.modified, + affectedStoryCount: data.counts.affected, + }, + }, + }; }, }, findByComponent: { @@ -407,19 +411,23 @@ Defaults to ${DEFAULT_MAX_DISTANCE}; raise it to widen recall, lower it to tight const unmatchedCount = lookup.results.filter( (result) => !result.pathNotFound && result.matches.length === 0 ).length; - await reportToolsetTelemetry(ctx, 'tool:getStoriesByComponent', { - toolset: 'dev', - componentCount: input.componentPaths.length, - matchedComponentCount: input.componentPaths.length - unmatchedCount, - totalMatchCount: lookup.results.reduce( - (total, result) => total + result.matches.length, - 0 - ), - maxDistance, - }); - const data: FindByComponentOutput = { results: lookup.results, maxDistance }; - return { ok: true, data, markdown: formatFindByComponent(data) }; + return { + ok: true, + data, + markdown: formatFindByComponent(data), + telemetry: { + payload: { + componentCount: input.componentPaths.length, + matchedComponentCount: input.componentPaths.length - unmatchedCount, + totalMatchCount: lookup.results.reduce( + (total, result) => total + result.matches.length, + 0 + ), + maxDistance, + }, + }, + }; }, }, },