From ff60e57ad4d3d85893e721a00d720a5126abd9da Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Tue, 4 Aug 2026 13:43:42 +0200 Subject: [PATCH 1/6] =?UTF-8?q?Review=201/6=20=E2=80=94=20the=20contract:?= =?UTF-8?q?=20the=20open-service=20README?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code/core/src/shared/open-service/README.md | 57 +++++++++++++++++---- 1 file changed, 48 insertions(+), 9 deletions(-) diff --git a/code/core/src/shared/open-service/README.md b/code/core/src/shared/open-service/README.md index 6d4b692db1da..0aa72c9dc29a 100644 --- a/code/core/src/shared/open-service/README.md +++ b/code/core/src/shared/open-service/README.md @@ -31,8 +31,8 @@ client/server registration API. The environment-agnostic API consists of: - `defineService` -- `defineToolset`, `registerToolset`, `getRegisteredToolsets` — the public toolset construct (see - [Toolset](#toolset)) +- `defineToolset`, `registerToolset`, `getToolset`, `getRegisteredToolsets` — the public toolset + construct (see [Toolset](#toolset)) - the exported type aliases from [types.ts](./types.ts) The server-only API consists of: @@ -64,7 +64,8 @@ Internal tests and implementation code may import from the individual modules di - [types.ts](./types.ts): core type model for definitions, contexts, runtime instances, and static build data - [service-definition.ts](./service-definition.ts): `defineService()` typing that preserves inline inference when declaring services - [toolset-definition.ts](./toolset-definition.ts): `defineToolset()` typing for public toolsets and their handler context -- [toolset-registry.ts](./toolset-registry.ts): `registerToolset` / `getRegisteredToolsets` — the realm-global toolset inventory adapters read +- [toolset-registry.ts](./toolset-registry.ts): `registerToolset` / `getToolset` / `getRegisteredToolsets` — the realm-global toolset inventory adapters read +- [toolset-names.ts](./toolset-names.ts): `MCP_TOOL_NAMES` — the frozen public MCP tool names — and `getRef` for consumer-aware cross-references (display titles are editable prose on each method, not part of the frozen contract) - `services/`: core OSA service definitions and per-runtime registration helpers - `toolsets/`: public toolsets (docs, stories, test, review), mirroring the services tree - [service-validation.ts](./service-validation.ts): sync + async schema validation helpers and error wrapping @@ -100,15 +101,53 @@ Use `defineService()` to preserve the concrete query and command map types. Services and toolsets are sibling constructs behind this one entry: **services** own internal state and synchronization; **toolsets** are the public agent surface for CLI and MCP adapters. A toolset -(`defineToolset`) has an `id`, a description, and methods carrying only `schema`, `description`, and -`handler`. Handlers receive `(input, ctx)` where `ctx` has `consumer` (`'cli' | 'mcp'`), an optional -`origin`, a required `format` (`'markdown' | 'json'` — adapters own the mapping; methods never -declare the format), and `getService` for calling OSA services (with `{ internal: true }`). +(`defineToolset`) has an `id`, a description, a `telemetryGroup` (`'dev' | 'test' | 'docs'` — +stories and review report under `dev`), and methods carrying five fields: + +- `title` — required short display label shown by client UIs; editable prose, not part of the + frozen wire contract (`MCP_TOOL_NAMES` owns the invokable tool name) +- `description` — `string`, or a function of `ctx` when the prose differs per consumer +- `schema` — the input schema +- `outputSchema` — optional; published as the MCP `outputSchema`, and `structuredContent` is + 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 + +`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 → +**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 +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[]`: +`preview-stories` renders one text block per URL; the CLI's `--json` (Milestone 5) becomes "return +`data`, skip `markdown`". + +An error whose message speaks to the agent and names its own recovery declares `agentFacing: true` +(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 `{ consumer: 'cli' | 'mcp', origin?, uiRoot?, getService, telemetry? }`. `uiRoot` is where +the consumer's Storybook UI is reachable when that differs from `origin` (a sub-path-hosted dev +server); methods that link into the UI prefer it, and the adapter derives it from the request. +Descriptions that name a sibling tool must render it through `getRef(ctx)` rather than hardcoding +either spelling, so the same sentence reads as the MCP tool name or the CLI command per consumer. +Facts that are fixed at boot (whether review or a11y is enabled) are factory options on the +toolset, not `ctx` fields. Toolsets register imperatively via `registerToolset`, called from the same place the paired service registers (for core and addons, the `services` preset hook — the mechanism itself is -preset-independent so manager- or preview-realm toolsets can use it later). Adapters obtain the -registered set via `getRegisteredToolsets()`; the MCP server consumes it from Milestone 4 and the +preset-independent so manager- or preview-realm toolsets can use it later). A toolset must be +registered wherever a consumer resolves it, including consumers that only read its descriptions and +schemas: `getToolset(id)` throws on an unregistered id rather than silently dropping a tool. +Adapters resolve one toolset with `getToolset(id)` or take the whole set via +`getRegisteredToolsets()`; `@storybook/addon-mcp` and `@storybook/mcp` consume them today, and the `storybook tools` CLI from Milestone 5. ### Query From 5bc15c9b41b7786bb5405b18efa15224fb36b3d6 Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Tue, 4 Aug 2026 13:43:54 +0200 Subject: [PATCH 2/6] =?UTF-8?q?Review=202/6=20=E2=80=94=20the=20construct:?= =?UTF-8?q?=20defineToolset,=20toolset=20types,=20the=20registry,=20the=20?= =?UTF-8?q?frozen=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../open-service/toolset-definition.test-d.ts | 217 +++++++++++++++--- .../shared/open-service/toolset-definition.ts | 139 +++++++++-- .../src/shared/open-service/toolset-names.ts | 42 ++++ .../shared/open-service/toolset-registry.ts | 37 ++- .../src/shared/open-service/toolset-types.ts | 18 ++ 5 files changed, 403 insertions(+), 50 deletions(-) create mode 100644 code/core/src/shared/open-service/toolset-names.ts create mode 100644 code/core/src/shared/open-service/toolset-types.ts diff --git a/code/core/src/shared/open-service/toolset-definition.test-d.ts b/code/core/src/shared/open-service/toolset-definition.test-d.ts index cd203207fd59..effba734cd7e 100644 --- a/code/core/src/shared/open-service/toolset-definition.test-d.ts +++ b/code/core/src/shared/open-service/toolset-definition.test-d.ts @@ -1,20 +1,27 @@ import * as v from 'valibot'; import { describe, expectTypeOf, it } from 'vitest'; -import { defineToolset, type ToolsetDefinition } from './index.ts'; +import { + defineToolset, + type ToolsetCtx, + type ToolsetDefinition, + type ToolsetOutcome, +} from './index.ts'; const exampleToolset = defineToolset({ id: 'example', description: 'Example API', + telemetryGroup: 'dev', methods: { greet: { + title: 'Greet', description: 'Greets a person.', schema: v.object({ name: v.string() }), - handler: async ({ name }) => { - expectTypeOf(name).toEqualTypeOf(); - - return `Hello ${name}`; - }, + handler: async ({ name }): Promise> => ({ + ok: true, + data: { greeting: `Hello ${name}` }, + markdown: `Hello ${name}`, + }), }, }, }); @@ -22,35 +29,191 @@ const exampleToolset = defineToolset({ const reviewToolset = defineToolset({ id: 'review', description: 'Create a review', + telemetryGroup: 'dev', methods: { create: { - description: 'Create a review', + title: 'Create review', + description: (ctx) => `Create a review (${ctx.consumer})`, schema: v.object({ title: v.string() }), - handler: async (input, ctx) => { - expectTypeOf(input.title).toEqualTypeOf(); - expectTypeOf(ctx.consumer).toEqualTypeOf<'cli' | 'mcp'>(); - expectTypeOf(ctx.origin).toEqualTypeOf(); - expectTypeOf(ctx.format).toEqualTypeOf<'markdown' | 'json'>(); - expectTypeOf( - ctx.getService<{ ok: true }>('core/review', { internal: true }) - ).toEqualTypeOf<{ - ok: true; - }>(); - - return input.title; - }, + outputSchema: v.object({ title: v.string() }), + handler: async ( + input, + ctx + ): Promise< + ToolsetOutcome<{ title: string; origin?: string }, { title: string; reason: string }> + > => + input.title + ? { ok: true, data: { title: input.title, origin: ctx.origin }, markdown: input.title } + : { + ok: false, + data: { title: input.title, reason: 'missing title' }, + markdown: 'missing title', + }, }, }, }); describe('defineToolset types', () => { - it('preserves method schema output types in handlers', () => { + // Assertions live outside the definition literal: inside it, the first contextual-typing pass + // sees `any`/`unknown` params, so exact-type checks there would report on the wrong pass. + it('types handler input from the method schema', () => { + const greet: ( + input: { name: string }, + context: ToolsetCtx + ) => Promise> = + exampleToolset.methods.greet.handler; + const create: ( + input: { title: string }, + context: ToolsetCtx + ) => Promise< + ToolsetOutcome<{ title: string; origin?: string }, { title: string; reason: string }> + > = reviewToolset.methods.create.handler; + + expectTypeOf(greet).toBeFunction(); + expectTypeOf(create).toBeFunction(); expectTypeOf(exampleToolset).toMatchTypeOf(); - expectTypeOf(exampleToolset.methods.greet.handler).parameter(0).toEqualTypeOf<{ - name: string; - }>(); - expectTypeOf(reviewToolset.methods.create.handler).parameter(0).toEqualTypeOf<{ - title: string; - }>(); + }); + + it('narrows both branches of an outcome on its tag', async () => { + const outcome = await reviewToolset.methods.create.handler( + { title: 'x' }, + { consumer: 'cli', getService: () => ({}) as never } + ); + + if (outcome.ok) { + expectTypeOf(outcome.data).toEqualTypeOf<{ title: string; origin?: string }>(); + } else { + expectTypeOf(outcome.data).toEqualTypeOf<{ title: string; reason: string }>(); + } + }); + + it('makes the failure branch unreachable for infallible methods', async () => { + const outcome = await exampleToolset.methods.greet.handler({ name: 'x' }); + + if (!outcome.ok) { + expectTypeOf(outcome.data).toBeNever(); + } + }); + + it('resolves description functions against the toolset context', () => { + const description: string | ((context: ToolsetCtx) => string) = + reviewToolset.methods.create.description; + + expectTypeOf(description).not.toBeNever(); + }); +}); + +describe('method titles', () => { + it('rejects a method that omits its display title', () => { + defineToolset({ + id: 'untitled', + description: 'Rejected', + telemetryGroup: 'dev', + methods: { + // @ts-expect-error — every method must declare its display `title` + create: { + description: 'Has no title.', + schema: v.object({}), + handler: async (): Promise> => ({ + ok: true, + data: { done: true }, + markdown: 'x', + }), + }, + }, + }); + }); +}); + +describe('schema-bound outcomes', () => { + // `reviewToolset` above is the acceptance case: its success data carries `origin`, which the + // output schema does not declare — outcome data may be a superset of the published contract. + + it('rejects data that renames a schema-declared field', () => { + defineToolset({ + id: 'renamed-field', + description: 'Rejected', + telemetryGroup: 'dev', + methods: { + create: { + title: 'Create', + description: 'Renames title to heading.', + schema: v.object({}), + outputSchema: v.object({ title: v.string() }), + // @ts-expect-error — `data` lacks the schema-declared `title` field + handler: async (): Promise> => ({ + ok: true, + data: { heading: 'x' }, + markdown: 'x', + }), + }, + }, + }); + }); + + it('rejects data that drops a schema-declared field', () => { + defineToolset({ + id: 'dropped-field', + description: 'Rejected', + telemetryGroup: 'dev', + methods: { + create: { + title: 'Create', + description: 'Publishes title but returns nothing.', + schema: v.object({}), + outputSchema: v.object({ title: v.string() }), + // @ts-expect-error — `data` is missing the schema-declared `title` field + handler: async (): Promise, never>> => ({ + ok: true, + data: {}, + markdown: 'x', + }), + }, + }, + }); + }); + + it('rejects data that mistypes a schema-declared field', () => { + defineToolset({ + id: 'mistyped-field', + description: 'Rejected', + telemetryGroup: 'dev', + methods: { + create: { + title: 'Create', + description: 'Returns a number where the schema declares a string.', + schema: v.object({}), + outputSchema: v.object({ title: v.string() }), + // @ts-expect-error — `title` is a number where the schema declares a string + handler: async (): Promise> => ({ + ok: true, + data: { title: 1 }, + markdown: 'x', + }), + }, + }, + }); + }); + + it('constrains the failure branch to the schema too', () => { + defineToolset({ + id: 'unbound-failure', + description: 'Rejected', + telemetryGroup: 'dev', + methods: { + create: { + title: 'Create', + description: 'Failure data skips the published contract.', + schema: v.object({}), + outputSchema: v.object({ title: v.string() }), + // @ts-expect-error — failure `data` lacks the schema-declared `title` field + handler: async (): Promise> => ({ + ok: false, + data: { reason: 'x' }, + markdown: 'x', + }), + }, + }, + }); }); }); diff --git a/code/core/src/shared/open-service/toolset-definition.ts b/code/core/src/shared/open-service/toolset-definition.ts index f747a8b68d28..41ac65a0b7d0 100644 --- a/code/core/src/shared/open-service/toolset-definition.ts +++ b/code/core/src/shared/open-service/toolset-definition.ts @@ -6,12 +6,6 @@ type AnySchema = StandardSchemaV1; export type ToolsetConsumer = 'cli' | 'mcp'; -/** - * Output representation requested by the adapter. Methods never declare the format themselves; - * adapters own the mapping (CLI `--json` flag, MCP `json` tool input). - */ -export type ToolsetFormat = 'markdown' | 'json'; - /** * Service lookup for toolset handlers. Intentionally not keyed to ServerCoreServices: * toolsets may call core OSA (with `{ internal: true }`) or optional addon services by id. @@ -20,23 +14,100 @@ export type ToolsetGetService = { (serviceId: string, options?: GetServiceOptions): TInstance; }; +/** + * Emits one telemetry event for a toolset method. + * + * Adapters supply the sink so surface-specific fields (MCP session id, client info) stay with the + * adapter while the event name and payload — the part that describes the capability — stay in the + * method. Absent when the consumer has telemetry disabled. + */ +export type ToolsetTelemetry = (event: string, payload: Record) => Promise; + export type ToolsetCtx = { consumer: ToolsetConsumer; /** Storybook server origin. Absent when running from a CLI without a live Storybook. */ origin?: string; - format: ToolsetFormat; + /** + * Where this consumer's Storybook UI is reachable, when that differs from `origin` — a + * sub-path-hosted dev server answers MCP at `` while its UI lives at ``. + * Methods that link into the UI prefer this over `origin`; the adapter derives it from the + * request it is serving. + */ + uiRoot?: string; getService: ToolsetGetService; + telemetry?: ToolsetTelemetry; }; -export type ToolsetMethod = { - description: string; +/** + * A method description, resolved per consumer. + * + * The function form exists because descriptions cross-reference sibling methods, and each surface + * spells those differently (`get-changed-stories` on MCP, `npx storybook tools stories changed` on + * the CLI). Use `getRef(ctx)` to render a reference rather than hardcoding either spelling. + */ +export type ToolsetMethodDescription = string | ((context: ToolsetCtx) => string); + +/** + * The result of one method run: the tag, the structured data, and the rendered Markdown, all from a + * single execution. + * + * The failure model in one line each: could not do the job → throw; did the job and the answer is + * bad news → return `{ ok: false, data, markdown }`. Adapters unwrap mechanically — text blocks + * from `markdown`, `structuredContent` from `data`, MCP `isError` (and later CLI exit codes) from + * `ok` — so everything a method means lives on its definition, never re-derived outside it. + * + * Declare `TFailure = never` for infallible methods; the signature then documents fallibility and + * TypeScript narrows both branches. Return plain object literals: contextual typing against this + * union does the narrowing, no factory helpers needed. + * + * `markdown` may be multiple strings: MCP renders each as its own text block (preview-stories + * renders one block per URL), the CLI joins them with newlines. + */ +export type ToolsetOutcome = + | { readonly ok: true; readonly data: TSuccess; readonly markdown: string | string[] } + | { readonly ok: false; readonly data: TFailure; readonly markdown: string | string[] }; + +// `any` permits heterogeneous outcome maps. Each individual method remains typed by `defineToolset`. +// eslint-disable-next-line @typescript-eslint/no-explicit-any +export type AnyToolsetOutcome = ToolsetOutcome; + +/** + * One public method: description, input schema, optional output schema, and one handler. + * + * The handler produces the whole {@link ToolsetOutcome} — data, side effects, telemetry, and the + * rendered Markdown — because one MCP response carries `content` (text) and `structuredContent` + * (JSON) at once, and both must come from a single run: re-running a method with side effects + * would repeat them. Usage telemetry reports inline in the handler, with the rendered text in + * hand, so no consumer can forget it. + */ +export type ToolsetMethod< + TSchema extends AnySchema = AnySchema, + TOutcome extends AnyToolsetOutcome = AnyToolsetOutcome, +> = { + /** + * Short display label shown by client UIs (e.g. an MCP client's tool list). Editable prose like + * `description`, not a frozen contract — the invokable tool name lives in `MCP_TOOL_NAMES`. + */ + title: string; + description: ToolsetMethodDescription; schema: TSchema; - handler: (input: StandardSchemaV1.InferOutput, context: ToolsetCtx) => unknown; + /** Published as the MCP tool's `outputSchema`. Declare it only where the JSON is contractual. */ + outputSchema?: AnySchema; + handler: ( + input: StandardSchemaV1.InferOutput, + context: ToolsetCtx + ) => TOutcome | Promise; }; // `any` permits a heterogeneous method map. Each individual method remains typed by `defineToolset`. // eslint-disable-next-line @typescript-eslint/no-explicit-any -type ToolsetMethods = Record>; +type ToolsetMethods = Record>; + +/** + * Which surface group a toolset's telemetry reports under. Part of the definition — not adapter + * wiring — so the grouping cannot drift between consumers. Stories and review report under `dev`. + */ +export type ToolsetTelemetryGroup = 'dev' | 'test' | 'docs'; export type ToolsetDefinition< TId extends string = string, @@ -44,22 +115,58 @@ export type ToolsetDefinition< > = { id: TId; description: string; + telemetryGroup: ToolsetTelemetryGroup; methods: TMethods; }; export type AnyToolsetDefinition = ToolsetDefinition; -type DefinedToolsetMethods> = { - [TKey in keyof TSchemas]: ToolsetMethod; +/** + * What a handler may return when its method publishes an `outputSchema`: outcomes whose `data` — + * on both branches, since adapters validate failure data into `structuredContent` too — carries at + * least the schema's declared shape. The open record keeps the data-superset pattern legal: the + * rendered Markdown may use fields the public contract does not ship. The schema's *input* type is + * the right side of the contract, because that is what the adapters' runtime validation accepts. + */ +type SchemaBoundData = StandardSchemaV1.InferInput & + Record; + +type MethodOutcomeContract = TMethod extends { outputSchema: infer TOut extends AnySchema } + ? ToolsetOutcome> | Promise>> + : unknown; + +/** + * Second contextual-typing pass for the methods literal: `handler` input comes from that method's + * own `schema`, and its outcome data from the method's `outputSchema` where one is declared — so + * renaming or removing a published field is a compile error at the definition site. Intersecting + * this with the inferred map is what makes the flow work on both the stable and the native + * TypeScript compiler — inferring a separate record does not. + */ +type MethodContracts = { + [TKey in keyof TMethods]: { + handler: ( + input: StandardSchemaV1.InferOutput, + context: ToolsetCtx + ) => MethodOutcomeContract; + }; }; export function defineToolset< const TId extends string, - const TSchemas extends Record, + const TMethods extends ToolsetMethods, >(definition: { id: TId; description: string; - methods: DefinedToolsetMethods; -}): ToolsetDefinition> { + telemetryGroup: ToolsetTelemetryGroup; + methods: TMethods & MethodContracts; +}): ToolsetDefinition { return definition; } + +/** Resolves a method description for one consumer. */ +export function resolveToolsetDescription( + description: ToolsetMethodDescription, + context: ToolsetCtx +): string { + return typeof description === 'function' ? description(context) : description; +} diff --git a/code/core/src/shared/open-service/toolset-names.ts b/code/core/src/shared/open-service/toolset-names.ts new file mode 100644 index 000000000000..d151629b2084 --- /dev/null +++ b/code/core/src/shared/open-service/toolset-names.ts @@ -0,0 +1,42 @@ +import type { ToolsetCtx } from './toolset-definition.ts'; + +/** + * Maps a toolset method to the MCP tool name that exposes it. + * + * These names are the public MCP contract of `@storybook/addon-mcp` and `@storybook/mcp`: clients + * and downstream integrations call them by name, so they are frozen independently of the method + * names they point at. Both adapters register from this map, and descriptions render + * cross-references through {@link getRef}, so description prose and registration cannot disagree. + */ +export const MCP_TOOL_NAMES = { + 'stories.preview': 'preview-stories', + 'stories.changed': 'get-changed-stories', + 'stories.findByComponent': 'get-stories-by-component', + 'test.run': 'run-story-tests', + 'review.create': 'display-review', + 'docs.list': 'list-all-documentation', + 'docs.show': 'get-documentation', + 'docs.showStory': 'get-documentation-for-story', +} as const; + +export type ToolsetMethodRef = keyof typeof MCP_TOOL_NAMES; + +/** `stories.findByComponent` -> `stories find-by-component`. */ +function toCliPath(method: ToolsetMethodRef): string { + const [toolsetId, methodName] = method.split('.'); + const kebabMethod = methodName.replace(/[A-Z]/g, (letter) => `-${letter.toLowerCase()}`); + return `${toolsetId} ${kebabMethod}`; +} + +/** + * Renders a reference to a sibling method in the consumer's own vocabulary. + * + * Descriptions that tell an agent to call another tool must name it the way that agent can call it, + * so never hardcode a tool name or a command in description prose — use this instead. + */ +export function getRef(context: Pick) { + return (method: ToolsetMethodRef): string => + context.consumer === 'mcp' + ? MCP_TOOL_NAMES[method] + : `npx storybook tools ${toCliPath(method)}`; +} diff --git a/code/core/src/shared/open-service/toolset-registry.ts b/code/core/src/shared/open-service/toolset-registry.ts index f0b7aa4b0e32..4fa51960d1ff 100644 --- a/code/core/src/shared/open-service/toolset-registry.ts +++ b/code/core/src/shared/open-service/toolset-registry.ts @@ -1,4 +1,9 @@ +import { + OpenServiceDuplicateToolsetError, + OpenServiceMissingToolsetError, +} from '../../server-errors.ts'; import type { AnyToolsetDefinition } from './toolset-definition.ts'; +import type { KnownToolsets } from './toolset-types.ts'; const TOOLSET_REGISTRY_SYMBOL = Symbol.for('storybook.open-service.toolset-registry'); @@ -24,23 +29,41 @@ function getToolsetRegistry(): Map { * * Call it from the same place the paired OSA service registers (for core, the `services` preset * hook). Registration is deliberately independent of the Node preset system so manager- or - * preview-realm toolsets can use the same API later. Idempotent by id, mirroring `registerService`: - * a repeated registration is a no-op. + * preview-realm toolsets can use the same API later. + * + * Unlike `registerService`, a repeated id throws instead of being ignored: services re-register + * legitimately (HMR, repeated composition), while every toolset registration site is a one-shot + * preset hook, so a duplicate id can only mean two hosts claimed the same public surface. */ export function registerToolset(toolset: AnyToolsetDefinition): void { const registry = getToolsetRegistry(); - if (!registry.has(toolset.id)) { - registry.set(toolset.id, toolset); + if (registry.has(toolset.id)) { + throw new OpenServiceDuplicateToolsetError({ toolsetId: toolset.id }); } + + registry.set(toolset.id, toolset); } /** - * Returns the registered toolsets in registration order. + * Returns one registered toolset by id, typed for the core toolsets. * - * Adapter-facing: the MCP server (Milestone 4) and the `storybook tools` CLI (Milestone 5) read the - * public tool surface from here; nothing consumes it before then. + * Throws when the id is not registered — an adapter asking for a toolset that no host wired up is + * a configuration error, not an empty result. */ +export function getToolset(toolsetId: TId): KnownToolsets[TId]; +export function getToolset(toolsetId: string): AnyToolsetDefinition; +export function getToolset(toolsetId: string): AnyToolsetDefinition { + const toolset = getToolsetRegistry().get(toolsetId); + + if (!toolset) { + throw new OpenServiceMissingToolsetError({ toolsetId }); + } + + return toolset; +} + +/** Returns the registered toolsets in registration order. */ export function getRegisteredToolsets(): AnyToolsetDefinition[] { return Array.from(getToolsetRegistry().values()); } diff --git a/code/core/src/shared/open-service/toolset-types.ts b/code/core/src/shared/open-service/toolset-types.ts new file mode 100644 index 000000000000..3133ddbe70b4 --- /dev/null +++ b/code/core/src/shared/open-service/toolset-types.ts @@ -0,0 +1,18 @@ +import type { DocsToolset } from './toolsets/docs/definition.ts'; +import type { ReviewToolset } from './toolsets/review/definition.ts'; +import type { StoriesToolset } from './toolsets/stories/definition.ts'; +import type { TestToolset } from './toolsets/test/definition.ts'; + +/** + * The public toolsets core can register, keyed by id. + * + * Mirrors `core-service-types.ts` for services: it is what makes `getToolset('stories')` return a + * typed toolset instead of an opaque definition. A toolset registered by an addon under its own id + * is still resolvable — it falls through to the untyped `getToolset` overload. + */ +export type KnownToolsets = { + docs: DocsToolset; + stories: StoriesToolset; + test: TestToolset; + review: ReviewToolset; +}; From 1b8f71960899499a095eac2af8304cfd53ea6982 Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Tue, 4 Aug 2026 13:43:59 +0200 Subject: [PATCH 3/6] =?UTF-8?q?Review=203/6=20=E2=80=94=20the=20tool=20sur?= =?UTF-8?q?face:=20all=20four=20toolset=20definitions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../open-service/toolsets/docs/definition.ts | 603 ++++++++++-------- .../toolsets/review/definition.ts | 185 +++++- .../toolsets/stories/definition.ts | 231 +++++-- .../open-service/toolsets/test/definition.ts | 145 ++++- 4 files changed, 810 insertions(+), 354 deletions(-) diff --git a/code/core/src/shared/open-service/toolsets/docs/definition.ts b/code/core/src/shared/open-service/toolsets/docs/definition.ts index e7a4af5a74b4..c5c7752f9648 100644 --- a/code/core/src/shared/open-service/toolsets/docs/definition.ts +++ b/code/core/src/shared/open-service/toolsets/docs/definition.ts @@ -1,297 +1,372 @@ import * as v from 'valibot'; -import { - OpenServiceDocgenMissingComponentError, - OpenServiceMissingServiceError, -} from '../../../../server-errors.ts'; -import type { DocgenService } from '../../services/docgen/definition.ts'; -import type { StoryDocsService } from '../../services/story-docs/definition.ts'; -import { defineToolset, type ToolsetCtx } from '../../toolset-definition.ts'; -import { classifyServices } from './classify-services.ts'; -import { - adaptCoreComponent, - adaptCoreDoc, - adaptCoreStories, -} from './manifest-formatter/adapt-core-manifest.ts'; -import type { - AllManifests, - ComponentManifestV1, - DocV1, -} from './manifest-formatter/manifest-types.ts'; +import { defineToolset, type ToolsetCtx, type ToolsetOutcome } from '../../toolset-definition.ts'; +import { getRef } from '../../toolset-names.ts'; +import type { DocsAccess, ResolvedDocsEntry } from './access.ts'; import { formatComponentManifest, formatDocsManifest, formatManifestsToLists, + formatMultiSourceManifestsToLists, formatStoryDocumentation, + MAX_STORIES_TO_SHOW, } from './manifest-formatter/markdown.ts'; -import { - mapDocsList, - mapDocsShow, - mapDocsShowStory, - mapStoryDocsEntries, - resolveImportStatement, - selectAttachedDocs, - type MdxPayload, -} from './map.ts'; - -/** Stable addon-docs MDX service id. Kept local so the docs toolset does not import core-server. */ -const MDX_SERVICE_ID = 'addon-docs/mdx'; - -type MdxService = { - queries: { - mdxForAllComponents: { - loaded: () => Promise>; - }; - mdxForComponent: { - loaded: (input: { id: string }) => Promise; +import type { AllManifests } from './manifest-formatter/manifest-types.ts'; +import { listSources, type DocsSource } from './multi-source.ts'; +import type { SourceListing } from './sources.ts'; +import { estimateTokens } from '../estimate-tokens.ts'; + +/** + * Which Storybooks these tools serve — exactly one of the two. + * + * Modelled as an exclusive union so neither "no access at all" nor "both, one silently winning" can + * be constructed: a composition takes a `storybookId` on every lookup and a single Storybook must + * not ask for one, and that difference is decided here. + */ +export type CreateDocsToolsetOptions = + | { + /** Reads the one Storybook these tools serve. */ + docsAccess: DocsAccess; + sources?: never; + } + | { + /** The composed Storybooks these tools serve; ids are only unique within a source. */ + sources: DocsSource[]; + docsAccess?: never; }; - }; + +export type DocsListOutput = { + withStoryIds: boolean; + /** Single-source listing. */ + manifests?: AllManifests; + /** Per-source listings, in composition. */ + sources?: SourceListing[]; }; -function tryGetService(ctx: ToolsetCtx, serviceId: string): T | undefined { - try { - return ctx.getService(serviceId, { internal: true }); - } catch (error) { - if (error instanceof OpenServiceMissingServiceError) { - return undefined; - } - throw error; +export type DocsShowOutput = { + id: string; + entry?: ResolvedDocsEntry; + storybookId?: string; + /** Set when the request named no source, or one that does not exist. */ + sourceError?: string; +}; + +export type DocsShowStoryOutput = { + componentId: string; + storyName: string; + entry?: ResolvedDocsEntry; + storybookId?: string; + sourceError?: string; +}; + +/** + * The manifests a listing should be reported against. + * + * Reporting predates composition and describes one Storybook, so a composed listing is reported + * against the first source that produced one. Nothing is returned when no source did — a listing of + * nothing but errors is not a usage signal. + */ +export function selectReportedManifests({ + manifests, + sources, +}: DocsListOutput): AllManifests | undefined { + return manifests ?? sources?.find((listing) => listing.manifests)?.manifests; +} + +/** + * One classification of a `show` lookup, shared by the failure predicate and the renderer so the + * outcome tag and the rendered prose cannot disagree. + */ +type ShowResolution = + | { kind: 'source-error'; message: string } + | { kind: 'entry-missing' } + | { kind: 'found'; entry: ResolvedDocsEntry }; + +function resolveShow({ entry, sourceError }: DocsShowOutput): ShowResolution { + if (sourceError !== undefined) { + return { kind: 'source-error', message: sourceError }; } + if (entry === undefined) { + return { kind: 'entry-missing' }; + } + return { kind: 'found', entry }; +} + +type ComponentEntry = Extract; + +/** The `showStory` counterpart of {@link ShowResolution}. */ +type ShowStoryResolution = + | { kind: 'source-error'; message: string } + | { kind: 'component-missing' } + | { kind: 'story-missing'; component: ComponentEntry['component'] } + | { kind: 'found'; component: ComponentEntry['component'] }; + +function resolveShowStory({ + entry, + storyName, + sourceError, +}: DocsShowStoryOutput): ShowStoryResolution { + if (sourceError !== undefined) { + return { kind: 'source-error', message: sourceError }; + } + if (entry === undefined || entry.kind !== 'component') { + return { kind: 'component-missing' }; + } + const { component } = entry; + return component.stories?.some((story) => story.name === storyName) + ? { kind: 'found', component } + : { kind: 'story-missing', component }; } /** - * Component payload loads throw when an id has no component entry in the index. Standalone docs - * ids and ids that don't exist at all both land there, and both are answered by absence rather - * than a failure, so the typed error becomes `undefined`. + * Whether `docs.show` failed: an unusable source, or an id that resolved to nothing. + * + * The handlers encode this in the outcome tag; the predicate stays exported because it is part of + * the frozen `@storybook/mcp` API. */ -async function loadOptionalComponentPayload( - load: Promise -): Promise { - try { - return await load; - } catch (error) { - if (error instanceof OpenServiceDocgenMissingComponentError) { - return undefined; +export function isDocsShowError(output: DocsShowOutput): boolean { + return resolveShow(output).kind !== 'found'; +} + +/** Whether `docs.showStory` failed: an unusable source, a missing component, or a missing story. */ +export function isDocsShowStoryError(output: DocsShowStoryOutput): boolean { + return resolveShowStory(output).kind !== 'found'; +} + +function describeList(ctx: ToolsetCtx): string { + const ref = getRef(ctx); + return `List all available UI components and documentation entries from the Storybook, returning the IDs the other documentation tools take as input. Call this first for any UI task — before writing a new component, check what the design system already provides and build on it instead of hand-rolling a duplicate; before answering any question about props, API, or usage, discover the relevant IDs here rather than reading component source. Then fetch the entries with ${ref('docs.show')}, referencing only IDs returned here — never guess IDs. When multiple Storybook sources are configured, entries from every source are included; scope follow-up calls to one source via their \`storybookId\` input. Pass \`withStoryIds: true\` when you need story IDs for other tools.`; +} + +function describeShow(ctx: ToolsetCtx): string { + return `Get documentation for a UI component or docs entry. + +Returns the first ${MAX_STORIES_TO_SHOW} stories (including story IDs) with code snippets showing how props are used, plus TypeScript prop definitions. Call this before using a component to avoid hallucinating prop names, types, or valid combinations, and to answer any question about a component's props, API, or usage — reading or grepping the component source is not a substitute. Stories reveal real prop usage patterns, interactions, and edge cases that type definitions alone don't show. If the example stories don't show the prop you need, use the ${getRef(ctx)('docs.showStory')} tool to fetch the story documentation for the specific story variant you need. + +Example: id="button" returns Primary, Secondary, Large stories with code like ', + impact: 'critical', + failureSummary: '2.5:1 (required: 4.5:1)', + linkPath: '/inspect/button--primary?inspectPath=button.0', + }, + ], + }, + ], + }, + ], + }, + }) + ); + + expect((await runForMcp()).markdown).toBe(`## Passing Stories + +- button--primary + +## Accessibility Violations + +### button--primary - color-contrast + +Color contrast ratio is insufficient + +#### Affected Elements +- **Impact**: critical + **Message**: 2.5:1 (required: 4.5:1) + **Element**: + **Inspect**: http://localhost:6006/inspect/button--primary?inspectPath=button.0`); + }); + + it('omits accessibility violations when the run disabled a11y', async () => { + vi.mocked(runStoryTests).mockResolvedValue( + completed({ + componentTestStatuses: [componentTest('button--primary', 'status-value:success')], + a11yReports: { + 'button--primary': [ + { + violations: [ + { + id: 'color-contrast', + description: 'Color contrast ratio is insufficient', + nodes: [{ html: '', impact: 'critical' }], + }, + ], + }, + ], + }, + }) + ); + + expect((await runForMcp({ a11y: false })).markdown).toBe(`## Passing Stories + +- button--primary`); + }); + + it('reports unhandled errors without claiming any story passed', async () => { + vi.mocked(runStoryTests).mockResolvedValue( + completed({ + unhandledErrors: [ + { + name: 'ReferenceError', + message: 'foo is not defined', + stack: 'ReferenceError: foo is not defined\n at Button.tsx:10:5', + VITEST_TEST_PATH: '/src/Button.stories.tsx', + VITEST_TEST_NAME: 'Button > Primary', + }, + ], + }) + ); + + expect((await runForMcp()).markdown).toBe(`## Unhandled Errors + +### ReferenceError + +**Error message**: foo is not defined +**Path**: /src/Button.stories.tsx +**Test name**: Button > Primary +**Stack trace**: +ReferenceError: foo is not defined + at Button.tsx:10:5`); + }); + + it('returns the per-selector lookup failures when nothing matched', async () => { + vi.mocked(runStoryTests).mockResolvedValue({ + status: 'no-stories', + notFoundMessages: [ + 'No story found for story ID "missing--story"', + 'No story found for story ID "gone--story"', + ], + }); + + const outcome = await runForMcp({ + stories: [{ storyId: 'missing--story' }, { storyId: 'gone--story' }], + }); + + expect(outcome.ok).toBe(true); + expect(outcome.markdown).toBe(`No stories found matching the provided input. + +No story found for story ID "missing--story" +No story found for story ID "gone--story"`); + }); + + it('flags a failed run as a failure while still rendering the error line', async () => { + vi.mocked(runStoryTests).mockResolvedValue({ + status: 'error', + error: { message: 'Vitest failed to start' }, + }); + + const outcome = await runForMcp(); + + expect(outcome.ok).toBe(false); + expect(outcome.markdown).toBe('Error: Vitest failed to start'); + }); + + it('flags a cancelled run as a failure while still rendering the error line', async () => { + vi.mocked(runStoryTests).mockResolvedValue({ status: 'cancelled' }); + + const outcome = await runForMcp(); + + expect(outcome.ok).toBe(false); + expect(outcome.markdown).toBe('Error: Test run was cancelled'); + }); + }); + + describe('telemetry', () => { + it('reports result counts for a completed run', async () => { + vi.mocked(runStoryTests).mockResolvedValue( + completed({ + storyIds: ['button--primary'], + componentTestCount: { success: 1, error: 0 }, + a11yCount: { success: 0, warning: 1, error: 0 }, + componentTestStatuses: [componentTest('button--primary', 'status-value:success')], + a11yReports: { + 'button--primary': [ + { + violations: [ + { + id: 'color-contrast', + description: 'Color contrast ratio is insufficient', + nodes: [{ html: '', impact: 'critical' }], + }, + ], + }, + ], + }, + }) + ); + + await runTests({ stories: [{ storyId: 'button--primary' }] }); + + expect(telemetry).toHaveBeenCalledWith('tool:runStoryTests', { + runA11y: true, + inputStoryCount: 1, + matchedStoryCount: 1, + passingStoryCount: 1, + failingStoryCount: 0, + a11yViolationCount: 1, + unhandledErrorCount: 0, + }); + }); + + it('reports zeroed counts when no story matched', async () => { + vi.mocked(runStoryTests).mockResolvedValue({ + status: 'no-stories', + notFoundMessages: ['No story found for story ID "missing--story"'], + }); + + await runTests({ stories: [{ storyId: 'missing--story' }], a11y: false }); + + expect(telemetry).toHaveBeenCalledWith('tool:runStoryTests', { + 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(); + + expect(telemetry).not.toHaveBeenCalled(); + }); + }); }); diff --git a/code/core/src/shared/open-service/toolsets/test/format.ts b/code/core/src/shared/open-service/toolsets/test/format.ts index fa0afa991f77..b5b701cc8837 100644 --- a/code/core/src/shared/open-service/toolsets/test/format.ts +++ b/code/core/src/shared/open-service/toolsets/test/format.ts @@ -1,16 +1,280 @@ -import type { TestRunOutput } from './definition.ts'; +import type { ToolsetCtx } from '../../toolset-definition.ts'; +import type { TestRunData, TestRunResult } from './definition.ts'; -/** Formats a test run result as a compact summary for humans and agents. */ -export function formatTestRun(output: TestRunOutput): string { - switch (output.status) { +type ComponentTestStatus = TestRunResult['componentTestStatuses'][number]; + +/** + * Shapes of the addon-vitest / addon-a11y payloads that travel over the channel. The run result is + * not validated on arrival, so these describe what the formatter reads rather than what it is + * promised — every access below stays defensive for that reason. + */ +type A11yViolationNode = { + impact?: string; + failureSummary?: string; + html?: string; + linkPath?: string; +}; + +type A11yViolation = { + id: string; + description: string; + nodes: A11yViolationNode[]; +}; + +type A11yReport = { + error?: { message: string }; + violations?: unknown; +}; + +type UnhandledError = { + name?: string; + message?: string; + stack?: string; + VITEST_TEST_PATH?: string; + VITEST_TEST_NAME?: string; +}; + +export type TestRunSummary = { + passingStoryCount: number; + failingStoryCount: number; + a11yViolationCount: number; + unhandledErrorCount: number; +}; + +function isPassing(status: ComponentTestStatus): boolean { + return status.value === 'status-value:success'; +} + +function isFailing(status: ComponentTestStatus): boolean { + return status.value === 'status-value:error'; +} + +function getA11yReports(result: TestRunResult): Record { + return result.a11yReports as Record; +} + +function getA11yViolations(report: A11yReport): A11yViolation[] { + if (!('violations' in report)) { + return []; + } + + const { violations } = report; + if (!Array.isArray(violations)) { + return []; + } + + return violations.map((violation) => ({ + id: violation.id, + description: violation.description, + nodes: violation.nodes.map((node: Record) => ({ + impact: typeof node.impact === 'string' ? node.impact : undefined, + failureSummary: typeof node.failureSummary === 'string' ? node.failureSummary : undefined, + html: typeof node.html === 'string' ? node.html : undefined, + linkPath: typeof node.linkPath === 'string' ? node.linkPath : undefined, + })), + })); +} + +function countA11yViolations(a11yReports: Record): number { + let count = 0; + + for (const reports of Object.values(a11yReports ?? {})) { + for (const report of reports) { + if ('error' in report && report.error) { + continue; + } + + count += getA11yViolations(report).length; + } + } + + return count; +} + +/** Result counts, shared with the handler so telemetry reports the numbers the text shows. */ +export function summarizeTestRun(result: TestRunResult, a11y: boolean): TestRunSummary { + return { + passingStoryCount: result.componentTestStatuses.filter(isPassing).length, + failingStoryCount: result.componentTestStatuses.filter(isFailing).length, + a11yViolationCount: a11y ? countA11yViolations(getA11yReports(result)) : 0, + unhandledErrorCount: result.unhandledErrors.length, + }; +} + +function formatPassingStoriesSection(passingStories: ComponentTestStatus[]): string { + return `## Passing Stories + +- ${passingStories.map((status) => status.storyId).join('\n- ')}`; +} + +function formatFailingStoriesSection(statuses: ComponentTestStatus[]): string { + const entries = statuses.map( + (status) => + `### ${status.storyId} + +${status.description || 'No failure details available.'}` + ); + + return `## Failing Stories + +${entries.join('\n\n')}`; +} + +function formatA11yReportsSection({ + a11yReports, + origin, +}: { + a11yReports: Record; + origin?: string; +}): string | undefined { + const a11yViolationSections: string[] = []; + + for (const [storyId, reports] of Object.entries(a11yReports)) { + for (const report of reports) { + if ('error' in report && report.error) { + a11yViolationSections.push(`### ${storyId} - Error + +${report.error.message}`); + continue; + } + + const violations = getA11yViolations(report); + if (violations.length === 0) { + continue; + } + + for (const violation of violations) { + const nodes = violation.nodes + .map((node) => { + const inspectLink = origin && node.linkPath ? `${origin}${node.linkPath}` : undefined; + const parts: string[] = []; + + if (node.impact) { + parts.push(`- **Impact**: ${node.impact}`); + } + + if (node.failureSummary) { + parts.push(` **Message**: ${node.failureSummary}`); + } + + parts.push(` **Element**: ${node.html || '(no html available)'}`); + + if (inspectLink) { + parts.push(` **Inspect**: ${inspectLink}`); + } + + return parts.join('\n'); + }) + .join('\n'); + + a11yViolationSections.push(`### ${storyId} - ${violation.id} + +${violation.description} + +#### Affected Elements +${nodes}`); + } + } + } + + if (a11yViolationSections.length === 0) { + return undefined; + } + + return `## Accessibility Violations + +${a11yViolationSections.join('\n\n')}`; +} + +function formatUnhandledErrorsSection(errors: UnhandledError[]): string { + const formattedErrors = errors.map( + (unhandledError) => + `### ${unhandledError.name || 'Unknown Error'} + +**Error message**: ${unhandledError.message || 'No message available'} +**Path**: ${unhandledError.VITEST_TEST_PATH || 'No path available'} +**Test name**: ${unhandledError.VITEST_TEST_NAME || 'No test name available'} +**Stack trace**: +${unhandledError.stack || 'No stack trace available'}` + ); + + return `## Unhandled Errors + +${formattedErrors.join('\n\n')}`; +} + +/** + * Only the sections that carry information are emitted, so a run with nothing to report renders as + * an empty string. + */ +function formatCompletedRun( + result: TestRunResult, + { a11y, origin }: { a11y: boolean; origin?: string } +): string { + const sections: string[] = []; + const passingStories = result.componentTestStatuses.filter(isPassing); + const failingStories = result.componentTestStatuses.filter(isFailing); + + if (passingStories.length > 0) { + sections.push(formatPassingStoriesSection(passingStories)); + } + + if (failingStories.length > 0) { + sections.push(formatFailingStoriesSection(failingStories)); + } + + const a11yReports = getA11yReports(result); + if (a11y && a11yReports && Object.keys(a11yReports).length > 0) { + const a11ySection = formatA11yReportsSection({ a11yReports, origin }); + if (a11ySection) { + sections.push(a11ySection); + } + } + + if (result.unhandledErrors.length > 0) { + sections.push(formatUnhandledErrorsSection(result.unhandledErrors as UnhandledError[])); + } + + return sections.join('\n\n'); +} + +/** + * Agent-facing rendering, kept identical to the `run-story-tests` MCP tool. + * + * Failed and cancelled runs read as `Error: …` because that tool surfaced them by throwing; the + * `isError` flag that accompanied them belongs to the adapter, not to text. + */ +function formatTestRunForAgent(data: TestRunData, ctx: ToolsetCtx): string { + switch (data.status) { + case 'no-stories': + return `No stories found matching the provided input. + +${data.notFoundMessages.join('\n')}`; + case 'completed': + return formatCompletedRun(data.result, { a11y: data.a11y, origin: ctx.origin }); + case 'error': + return `Error: ${data.error.message}`; + case 'cancelled': + return 'Error: Test run was cancelled'; + default: { + // Type-only: a new outcome must get its own agent-facing text rather than falling through. + const _exhaustive: never = data; + return _exhaustive; + } + } +} + +/** Compact summary for a human reading terminal output. */ +function formatTestRunSummary(data: TestRunData): string { + switch (data.status) { case 'no-stories': return '# No stories matched\nNo story tests were run.'; case 'error': - return `# Test run failed\n${output.error.message}`; + return `# Test run failed\n${data.error.message}`; case 'cancelled': return '# Test run cancelled\nThe test run was cancelled.'; case 'completed': { - const { componentTestCount, a11yCount, totalTestCount } = output.result; + const { componentTestCount, a11yCount, totalTestCount } = data.result; const total = totalTestCount ?? componentTestCount.success + @@ -26,5 +290,14 @@ export function formatTestRun(output: TestRunOutput): string { `- Accessibility tests: ${a11yCount.success} passed, ${a11yCount.warning} warnings, ${a11yCount.error} failed`, ].join('\n'); } + default: { + // Type-only: a new outcome must get its own summary rather than falling through. + const _exhaustive: never = data; + return _exhaustive; + } } } + +export function formatTestRun(data: TestRunData, ctx: ToolsetCtx): string { + return ctx.consumer === 'mcp' ? formatTestRunForAgent(data, ctx) : formatTestRunSummary(data); +} diff --git a/code/core/src/shared/open-service/toolsets/test/run.test.ts b/code/core/src/shared/open-service/toolsets/test/run.test.ts index 1f5fb78449a9..656f7900278e 100644 --- a/code/core/src/shared/open-service/toolsets/test/run.test.ts +++ b/code/core/src/shared/open-service/toolsets/test/run.test.ts @@ -92,7 +92,7 @@ describe('createAsyncQueue', () => { }); describe('runStoryTests', () => { - it('returns an error when any focused selector is unresolved', async () => { + it('returns no-stories with the per-selector messages when nothing matched', async () => { const channel = createMockChannel(); const emitSpy = vi.spyOn(channel, 'emit'); @@ -103,8 +103,8 @@ describe('runStoryTests', () => { }); expect(result).toEqual({ - status: 'error', - error: { message: expect.stringContaining('missing--story') }, + status: 'no-stories', + notFoundMessages: ['No story found for story ID "missing--story"'], }); expect(emitSpy).not.toHaveBeenCalled(); }); @@ -133,7 +133,7 @@ describe('runStoryTests', () => { stories: [], }); - expect(result).toEqual({ status: 'no-stories' }); + expect(result).toEqual({ status: 'no-stories', notFoundMessages: [] }); expect(emitSpy).not.toHaveBeenCalled(); }); diff --git a/code/core/src/shared/open-service/toolsets/test/run.ts b/code/core/src/shared/open-service/toolsets/test/run.ts index 2ba20799cce3..06bc1953c920 100644 --- a/code/core/src/shared/open-service/toolsets/test/run.ts +++ b/code/core/src/shared/open-service/toolsets/test/run.ts @@ -172,24 +172,26 @@ export async function runStoryTests({ if (stories) { const index = await getIndex(); const resolved = findStoryIds(index, stories); - const unresolved = resolved.filter((story): story is NotFoundStory => !('entry' in story)); + const found = resolved.filter((story): story is FoundStory => 'entry' in story); + const notFoundMessages = resolved + .filter((story): story is NotFoundStory => !('entry' in story)) + .map((story) => story.errorMessage); + + // Nothing to run: the per-selector failures are the answer, so they travel with the outcome. + if (found.length === 0) { + return { status: 'no-stories', notFoundMessages }; + } - if (unresolved.length > 0) { + // Running the matched subset would report a verdict for stories that were never run, so a + // partially resolved selector list fails instead. + if (notFoundMessages.length > 0) { return { status: 'error', - error: { - message: unresolved.map((story) => story.errorMessage).join('; '), - }, + error: { message: notFoundMessages.join('; ') }, }; } - storyIds = resolved - .filter((story): story is FoundStory => 'entry' in story) - .map((s) => s.entry.id); - - if (storyIds.length === 0) { - return { status: 'no-stories' }; - } + storyIds = found.map((story) => story.entry.id); } let response: TriggerTestRunResponse; diff --git a/code/core/src/shared/review/features.test.ts b/code/core/src/shared/review/features.test.ts index 739b6dc83c5b..b1a5a9913261 100644 --- a/code/core/src/shared/review/features.test.ts +++ b/code/core/src/shared/review/features.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it } from 'vitest'; import type { StorybookFeatures } from '../../types/modules/core-common.ts'; import { features as defaultFeaturesPreset } from '../../core-server/presets/common-preset.ts'; -import { isReviewFeatureEnabled } from './features.ts'; +import { isReviewExplicitlyEnabled, isReviewFeatureEnabled } from './features.ts'; describe('isReviewFeatureEnabled', () => { it('is enabled with the untouched default features preset', async () => { @@ -40,3 +40,25 @@ describe('isReviewFeatureEnabled', () => { expect(isReviewFeatureEnabled(undefined)).toBe(false); }); }); + +describe('isReviewExplicitlyEnabled', () => { + // The flag-unset default is where the two gates differ, and prose gated on the wrong one + // advertises `display-review` to MCP clients that cannot see it: the infrastructure gate is on + // by default, the direct-client gate only on explicit opt-in. + it('stays off when the flag is unset, unlike the infrastructure gate', () => { + const defaults = { changeDetection: true }; + expect(isReviewFeatureEnabled(defaults)).toBe(true); + expect(isReviewExplicitlyEnabled(defaults)).toBe(false); + }); + + it('is enabled only on explicit opt-in with change detection', () => { + expect(isReviewExplicitlyEnabled({ changeDetection: true, experimentalReview: true })).toBe( + true + ); + expect(isReviewExplicitlyEnabled({ changeDetection: true, experimentalReview: false })).toBe( + false + ); + expect(isReviewExplicitlyEnabled({ experimentalReview: true })).toBe(false); + expect(isReviewExplicitlyEnabled(undefined)).toBe(false); + }); +}); diff --git a/code/core/src/shared/review/features.ts b/code/core/src/shared/review/features.ts index 18638eadda73..223a3152987f 100644 --- a/code/core/src/shared/review/features.ts +++ b/code/core/src/shared/review/features.ts @@ -16,3 +16,14 @@ import type { StorybookFeatures } from '../../types/modules/core-common.ts'; */ export const isReviewFeatureEnabled = (features: StorybookFeatures | undefined): boolean => features?.experimentalReview !== false && !!features?.changeDetection; + +/** + * Whether review is available to direct MCP clients: the explicit `experimentalReview` opt-in. + * + * Contrast with {@link isReviewFeatureEnabled}: with the flag unset the review infrastructure + * still mounts (the `storybook ai` CLI channel turns review on per request), but direct MCP + * clients never see `display-review` — so tool descriptions and result prose must not advertise + * it. Mirrors `getReviewStatus().available` in `@storybook/addon-mcp`. + */ +export const isReviewExplicitlyEnabled = (features: StorybookFeatures | undefined): boolean => + !!features?.experimentalReview && !!features?.changeDetection; diff --git a/code/core/src/shared/review/review-state.ts b/code/core/src/shared/review/review-state.ts index 119c955a04b8..0b0c81d77b57 100644 --- a/code/core/src/shared/review/review-state.ts +++ b/code/core/src/shared/review/review-state.ts @@ -1,14 +1,11 @@ /** - * The review payload an agent pushes via the `display-review` MCP tool. + * The review payload an agent publishes via the `review.create` toolset method. * - * Flow: - * MCP `display-review` tool → emit PUSH_REVIEW on the Storybook channel - * → core-server adapts it into the `core/review` service - * → manager tabs subscribe to the service's current-review query. + * Flow: `review.create` calls the `core/review` service's `setReview` command; manager tabs + * subscribe to the service's current-review query. * - * This mirrors the canonical valibot schema in `@storybook/addon-mcp` → - * `tools/display-review.ts`. The manager only renders the data — it does - * not validate — so it needs the type, not the validator. Keep `title` / + * This mirrors the canonical valibot schema in the review toolset definition. The manager only + * renders the data — it does not validate — so it needs the type, not the validator. Keep `title` / * `description` / `collections` in sync with that schema. */ @@ -24,7 +21,7 @@ export interface ReviewState { collections: ReviewCollection[]; changedFiles?: string[]; /** - * Server-side creation timestamp (unix ms) assigned when PUSH_REVIEW is + * Server-side creation timestamp (unix ms) assigned when the review is * received; used for live "Created x minutes ago" UI in the summary. */ createdAt?: number; diff --git a/code/core/src/storybook-error.ts b/code/core/src/storybook-error.ts index 6fe3d142e022..f78a879d1e42 100644 --- a/code/core/src/storybook-error.ts +++ b/code/core/src/storybook-error.ts @@ -50,6 +50,14 @@ export abstract class StorybookError extends Error { /** Flag used to easily determine if the error originates from Storybook. */ readonly fromStorybook: true = true as const; + /** + * Marks an error whose message is written for the agent that triggered the call and names its + * own recovery. Adapters surface such errors verbatim instead of wrapping them as unexpected + * failures. A property rather than a class list, so the trait travels with the instance across + * bundle copies where `instanceof` would misclassify. + */ + public readonly agentFacing: boolean; + /** * Flag used to determine if the error is handled by us and should therefore not be shown to the * user. @@ -98,6 +106,8 @@ export abstract class StorybookError extends Error { cause?: unknown; documentation?: boolean | string | string[]; isHandledError?: boolean; + /** See {@link StorybookError.agentFacing}. Defaults to false. */ + agentFacing?: boolean; name: string; /** * Optional array of sub-errors that are related to this error. When this error is sent to @@ -113,6 +123,7 @@ export abstract class StorybookError extends Error { this.documentation = props.documentation ?? false; this.code = props.code; this.isHandledError = props.isHandledError ?? false; + this.agentFacing = props.agentFacing ?? false; this.name = props.name; this.subErrors = props.subErrors ?? []; } diff --git a/yarn.lock b/yarn.lock index 41c1c5ce8295..48a512d3ba2e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -36614,6 +36614,7 @@ __metadata: oxc-resolver: "npm:^11.19.1" p-limit: "npm:^7.2.0" package-manager-detector: "npm:^1.1.0" + pathe: "npm:^1.1.2" picocolors: "npm:^1.1.0" picomatch: "npm:^2.3.0" picoquery: "npm:^2.5.0"