diff --git a/code/addons/vitest/src/preset.test.ts b/code/addons/vitest/src/preset.test.ts new file mode 100644 index 000000000000..ce77142db42a --- /dev/null +++ b/code/addons/vitest/src/preset.test.ts @@ -0,0 +1,30 @@ +import { beforeEach, describe, expect, it } from 'vitest'; + +import { clearToolsetRegistry, getToolset } from 'storybook/open-service'; +import type { Options } from 'storybook/internal/types'; + +import { services } from './preset.ts'; + +const options = { + channel: { on: () => {}, off: () => {}, emit: () => {} }, + presets: { + apply: async (key: string, fallback?: unknown) => + key === 'storyIndexGenerator' ? { getIndex: async () => ({ v: 5, entries: {} }) } : fallback, + }, +} as unknown as Options; + +describe('services preset hook', () => { + beforeEach(() => { + clearToolsetRegistry(); + }); + + // Consumers resolve the `test` toolset for its descriptions and schemas alone, without a dev + // server: `storybook ai` metadata generation applies `services` but never starts one, and a + // non-Vite dev server returns from `experimental_serverChannel` before it could register. Both + // used to hit an unregistered toolset and fail hard. + it('registers the test toolset without a server channel', async () => { + await services(undefined, options); + + expect(getToolset('test').methods.run.description).toBeDefined(); + }); +}); diff --git a/code/addons/vitest/src/preset.ts b/code/addons/vitest/src/preset.ts index 0b17ab781ddc..e2e2f439fc4e 100644 --- a/code/addons/vitest/src/preset.ts +++ b/code/addons/vitest/src/preset.ts @@ -9,8 +9,10 @@ import { } from 'storybook/internal/common'; import { type StoryIndexGenerator, + createTestToolset, experimental_UniversalStore, experimental_getTestProviderStore, + registerToolset, } from 'storybook/internal/core-server'; import { logger } from 'storybook/internal/node-logger'; import { cleanPaths, oneWayHash, sanitizeError, telemetry } from 'storybook/internal/telemetry'; @@ -43,6 +45,12 @@ import { runTestRunner } from './node/boot-test-runner.ts'; import type { CachedState, ErrorLike, StoreState } from './types.ts'; import type { StoreEvent } from './types.ts'; +/** + * Preset marker: true exactly when this addon is enabled, since only enabled addons' presets load. + * addon-mcp's availability gate reads it through `presets.apply('isAddonVitestEnabled', false)`. + */ +export const isAddonVitestEnabled = true; + type Event = | { type: 'test-discrepancy'; @@ -58,6 +66,32 @@ type Event = payload: StoreState['currentRun']; }; +/** + * Registers the public `test` toolset. + * + * This addon owns the toolset because running stories needs its channel protocol, but it must + * register from the `services` hook rather than `experimental_serverChannel`: consumers resolve + * the toolset for its descriptions and schemas alone, and the two places that do so — `storybook + * ai` metadata generation (which never starts a dev server) and a non-Vite dev server (where the + * channel hook returns early) — would otherwise ask for a toolset that was never registered and + * fail hard. Registering here matches the availability gate that decides whether the tool is + * offered at all: that gate reads the `isAddonVitestEnabled` marker this preset exports, which is + * true exactly when the addon is enabled — the same condition under which this hook runs. The + * channel is used only when a run is actually triggered. + */ +export const services = async (_value: void, options: Options): Promise => { + const storyIndexGenerator = + await options.presets.apply>('storyIndexGenerator'); + + registerToolset( + createTestToolset({ + channel: options.channel as Channel, + storyIndex: { getIndex: () => storyIndexGenerator.getIndex() }, + a11yEnabled: await options.presets.apply('isAddonA11yEnabled', false), + }) + ); +}; + export const experimental_serverChannel = async (channel: Channel, options: Options) => { const core = await options.presets.apply('core'); diff --git a/code/core/build-config.ts b/code/core/build-config.ts index 341ead7bc67d..a84e4c491830 100644 --- a/code/core/build-config.ts +++ b/code/core/build-config.ts @@ -206,6 +206,15 @@ const config: BuildEntries = { exportEntries: ['./open-service'], entryPoint: './src/shared/open-service/index.ts', }, + { + // Dependency-light docs toolset surface bundled into `@storybook/mcp`'s dist (core is a + // dev dependency there), replacing that package's own manifest-formatter copies. Portable: + // its d.ts is one flat self-contained file so that consumer can inline it through standard + // resolution; guarded by `portable-dist.test.ts` next to the entry source. + exportEntries: ['./internal/toolsets-docs'], + entryPoint: './src/shared/open-service/toolsets/docs/public.ts', + portable: { external: ['valibot'] }, + }, ], runtime: [ { diff --git a/code/core/package.json b/code/core/package.json index 67ccd09a6484..c6429907b493 100644 --- a/code/core/package.json +++ b/code/core/package.json @@ -198,6 +198,11 @@ "code": "./src/telemetry/index.ts", "default": "./dist/telemetry/index.js" }, + "./internal/toolsets-docs": { + "types": "./dist/shared/open-service/toolsets/docs/public.d.ts", + "code": "./src/shared/open-service/toolsets/docs/public.ts", + "default": "./dist/shared/open-service/toolsets/docs/public.js" + }, "./internal/types": { "types": "./dist/types/index.d.ts", "code": "./src/types/index.ts", @@ -371,6 +376,7 @@ "npmlog": "^7.0.0", "p-limit": "^7.2.0", "package-manager-detector": "^1.1.0", + "pathe": "^1.1.2", "picocolors": "^1.1.0", "picomatch": "^2.3.0", "picoquery": "^2.5.0", diff --git a/code/core/src/core-server/index.ts b/code/core/src/core-server/index.ts index 96d4ea12447f..a8874d0dcd92 100644 --- a/code/core/src/core-server/index.ts +++ b/code/core/src/core-server/index.ts @@ -58,6 +58,29 @@ export { registerService as experimental_registerService, } from '../shared/open-service/server.ts'; +/** + * Toolset registration for addons that host a public toolset. + * + * Exposed from the Node entry rather than `storybook/open-service`: toolset factories reach + * server-only code, and that entry is built for the browser. + */ +export { registerToolset } from '../shared/open-service/toolset-registry.ts'; +export { createTestToolset } from '../shared/open-service/toolsets/test/definition.ts'; +export type { + CreateTestToolsetOptions, + TestToolset, +} from '../shared/open-service/toolsets/test/definition.ts'; +export { createDocsToolset } from '../shared/open-service/toolsets/docs/definition.ts'; +export { emptyManifests } from '../shared/open-service/toolsets/docs/access.ts'; +export type { DocsAccess, ResolvedDocsEntry } from '../shared/open-service/toolsets/docs/access.ts'; +export { createServiceDocsAccess } from '../shared/open-service/toolsets/docs/access-service.ts'; +export { createManifestDocsAccess } from '../shared/open-service/toolsets/docs/access-manifest.ts'; +export { createLocalDocsAccess } from '../shared/open-service/toolsets/docs/access-local.ts'; +export { loadManifests } from './utils/manifests/manifests.ts'; +export { createStoriesToolset } from '../shared/open-service/toolsets/stories/definition.ts'; +export type { PreviewStoriesOutput } from '../shared/open-service/toolsets/stories/definition.ts'; +export { reviewToolset } from '../shared/open-service/toolsets/review/definition.ts'; + export { UniversalStore as experimental_UniversalStore } from '../shared/universal-store/index.ts'; export { MockUniversalStore as experimental_MockUniversalStore } from '../shared/universal-store/mock.ts'; export { diff --git a/code/core/src/core-server/presets/common-preset.ts b/code/core/src/core-server/presets/common-preset.ts index 398498b32653..d5a51c1680ff 100644 --- a/code/core/src/core-server/presets/common-preset.ts +++ b/code/core/src/core-server/presets/common-preset.ts @@ -19,15 +19,16 @@ import { StoryIndexGenerator } from 'storybook/internal/core-server'; import { loadCsf } from 'storybook/internal/csf-tools'; import { logger } from 'storybook/internal/node-logger'; import { telemetry } from 'storybook/internal/telemetry'; -import type { - CoreConfig, - DocgenProviderDescriptor, - Indexer, - Options, - PresetProperty, - PresetPropertyFn, - StoryDocsProvider, - StorybookConfigRaw, +import { + CHANGE_DETECTION_STATUS_TYPE_ID, + type CoreConfig, + type DocgenProviderDescriptor, + type Indexer, + type Options, + type PresetProperty, + type PresetPropertyFn, + type StoryDocsProvider, + type StorybookConfigRaw, } from 'storybook/internal/types'; import { OpenServiceServicesAppliedTwiceError } from '../../server-errors.ts'; @@ -36,9 +37,14 @@ import { createDocgenWorkerClient } from '../../shared/open-service/services/doc import { registerModuleGraphService } from '../../shared/open-service/services/module-graph/server.ts'; import { registerReviewService } from '../../shared/open-service/services/review/server.ts'; import { registerStoryDocsService } from '../../shared/open-service/services/story-docs/server.ts'; +import { createLocalDocsAccess } from '../../shared/open-service/toolsets/docs/access-local.ts'; import { registerToolset } from '../../shared/open-service/toolset-registry.ts'; -import { docsToolset } from '../../shared/open-service/toolsets/docs/definition.ts'; +import { createDocsToolset } from '../../shared/open-service/toolsets/docs/definition.ts'; import { reviewToolset } from '../../shared/open-service/toolsets/review/definition.ts'; +import { createStoriesToolset } from '../../shared/open-service/toolsets/stories/definition.ts'; +import { GitDiffProvider } from '../change-detection/GitDiffProvider.ts'; +import { getStatusStoreByTypeId } from '../stores/status.ts'; +import { loadManifests } from '../utils/manifests/manifests.ts'; import * as pathe from 'pathe'; import { isAbsolute, join } from 'pathe'; @@ -50,7 +56,7 @@ import { initCreateNewStoryChannel } from '../server-channel/create-new-story-ch import { initFileSearchChannel } from '../server-channel/file-search-channel.ts'; import { initGhostStoriesChannel } from '../server-channel/ghost-stories-channel.ts'; import { initOpenInEditorChannel } from '../server-channel/open-in-editor-channel.ts'; -import { isReviewFeatureEnabled } from '../../shared/review/features.ts'; +import { isReviewExplicitlyEnabled, isReviewFeatureEnabled } from '../../shared/review/features.ts'; import { initReviewChannel } from '../server-channel/review-channel.ts'; import { initTelemetryChannel } from '../server-channel/telemetry-channel.ts'; import { initializeChecklist } from '../utils/checklist.ts'; @@ -367,13 +373,31 @@ export const services = async (_value: void, options: Options): Promise => presets: options.presets, }); - // Toolsets register imperatively alongside their services: addons contribute both from their own - // `services` hook. The stories and test toolsets join once their boot-time dependencies are wired - // (stories factory; test moves to addon-vitest). - registerToolset(docsToolset); - const features = await options.presets.apply('features'); + // Toolsets register imperatively alongside their services: addons contribute both from their own + // `services` hook. The test toolset registers from addon-vitest, which owns the channel it needs. + const storyIndex = { getIndex: () => storyIndexGenerator.getIndex() }; + const gitDiffProvider = new GitDiffProvider(process.cwd()); + + registerToolset( + createStoriesToolset({ + storyIndex, + git: { + getRepoRoot: () => gitDiffProvider.getRepoRoot(), + getChangedFiles: () => gitDiffProvider.getChangedFiles(), + }, + changeStatuses: { + getAll: () => getStatusStoreByTypeId(CHANGE_DETECTION_STATUS_TYPE_ID).getAll(), + }, + // The explicit opt-in gate, not `isReviewFeatureEnabled`: with the flag unset the review + // infrastructure below still registers (the `storybook ai` CLI channel enables the tool per + // request), but direct MCP clients never see `display-review`, so the stories prose must + // not point at it. + reviewEnabled: isReviewExplicitlyEnabled(features), + }) + ); + if (isReviewFeatureEnabled(features)) { registerReviewService({ getIndex: () => storyIndexGenerator.getIndex(), @@ -410,12 +434,27 @@ export const services = async (_value: void, options: Options): Promise => }); } + // Story-docs registers whenever this block runs, docgen only when a worker is available, so + // docgen registered ⇒ story-docs registered. `createLocalDocsAccess` requires both before + // reading from the services and degrades to the manifests otherwise, so reordering or gating + // these registrations cannot make the docs toolset throw — only fall back. registerStoryDocsService({ getIndex: () => storyIndexGenerator.getIndex(), storyDocsProvider, workingDir: process.cwd(), }); } + + registerToolset( + createDocsToolset({ + // Registration-based selection between the docgen services and the inline manifests, shared + // with addon-mcp's composed local source so both read this Storybook the same way. + docsAccess: createLocalDocsAccess({ + storyIndex, + getManifests: () => loadManifests(options.presets), + }), + }) + ); }; // Store the promise (not the result) to prevent race conditions. diff --git a/code/core/src/core-server/utils/manifests/manifests.ts b/code/core/src/core-server/utils/manifests/manifests.ts index 3096be75a6dd..e759c2c68f8d 100644 --- a/code/core/src/core-server/utils/manifests/manifests.ts +++ b/code/core/src/core-server/utils/manifests/manifests.ts @@ -70,6 +70,18 @@ function mergeServicePayloads( ); } +/** + * Whether the dev server should 404 `manifests/components.json` and `manifests/docs.json` because + * docgen-server mode owns that data. + * + * Deliberately flag-based, while the docs toolset's engine selection (`createLocalDocsAccess`) is + * registration-based. The two can disagree: with the flag on but the docgen services unregistered + * (manager-only build, no docgen worker), the toolset falls back to reading the inline manifests + * while this route keeps 404ing them — so a composing parent Storybook fetching manifests over + * HTTP sees nothing even though the local MCP tools still serve docs. Accepted for now: switching + * this gate to registration would change composition behavior that only the live-fixture suites + * cover, so it is deferred to its own change. + */ function isDocgenServerManifestMode(features: { experimentalDocgenServer?: boolean; componentsManifest?: boolean; @@ -123,6 +135,16 @@ async function getManifests( ); } +/** + * Loads the live manifests, the same way the dev-server manifest routes do. + * + * Exposed for the docs toolset, which reads manifest data in-process instead of fetching its own + * server over loopback HTTP. + */ +export async function loadManifests(presets: Presets) { + return getManifests(presets, await getManifestEntries(presets), { watch: true }); +} + /** * Resolves the docgen `meta` for the components HTML debugger. * diff --git a/code/core/src/server-errors.ts b/code/core/src/server-errors.ts index a614343cacef..93324e303619 100644 --- a/code/core/src/server-errors.ts +++ b/code/core/src/server-errors.ts @@ -319,15 +319,23 @@ export class OpenServiceMissingOriginError extends StorybookError { } } +/** + * Why a review was refused. The toolset renders this as the opening of a longer, coaching message, + * so it lives here rather than in both places — the two copies had already drifted apart once. + */ +export function describeUnknownStoryIds(unknownIds: string[]): string { + const plural = unknownIds.length === 1 ? 'ID is' : 'IDs are'; + return `Refusing to publish review: ${unknownIds.length} story ${plural} not backed by a story entry in the live Storybook index (docs entries cannot be review slots):`; +} + export class OpenServiceUnknownStoryIdsError extends StorybookError { constructor(public data: { unknownIds: string[] }) { const list = data.unknownIds.map((id) => `- ${id}`).join('\n'); - const plural = data.unknownIds.length === 1 ? 'ID is' : 'IDs are'; super({ name: 'OpenServiceUnknownStoryIdsError', category: Category.CORE_COMMON, code: 18, - message: `Refusing to publish review: ${data.unknownIds.length} story ${plural} not backed by a story entry in the live Storybook index (docs entries cannot be review slots):\n${list}`, + message: `${describeUnknownStoryIds(data.unknownIds)}\n${list}`, }); } } @@ -356,6 +364,73 @@ export class OpenServiceServicesAppliedTwiceError extends StorybookError { } } +export class OpenServiceMissingToolsetError extends StorybookError { + constructor(public data: { toolsetId: string }) { + super({ + name: 'OpenServiceMissingToolsetError', + category: Category.CORE_COMMON, + code: 23, + message: `No registered toolset with id "${data.toolsetId}" exists in this environment.`, + }); + } +} + +export class OpenServiceDuplicateToolsetError extends StorybookError { + constructor(public data: { toolsetId: string }) { + super({ + name: 'OpenServiceDuplicateToolsetError', + category: Category.CORE_COMMON, + code: 24, + message: `A toolset with id "${data.toolsetId}" is already registered. Each public toolset must be registered exactly once.`, + }); + } +} + +/** + * The story module graph cannot answer a query right now (still building, unsupported builder, or + * a build failure). + * + * Its `message` is written for the agent that triggered the lookup and names the recovery, which + * is what `agentFacing` declares: adapters surface it verbatim rather than wrapping it as an + * unexpected failure. + */ +export class OpenServiceModuleGraphUnavailableError extends StorybookError { + constructor(public data: { reason: string }) { + super({ + name: 'OpenServiceModuleGraphUnavailableError', + category: Category.CORE_COMMON, + code: 25, + message: data.reason, + agentFacing: true, + }); + } +} + +/** + * A toolset method returned data its own published `outputSchema` rejects. + * + * Always a bug in the method: the schema is the contract adapters publish to their clients, so the + * mismatch is raised instead of quietly shipping unvalidated data. + */ +export class OpenServiceToolsetOutputMismatchError extends StorybookError { + constructor(public data: { issues: readonly unknown[] }) { + // Validation issues embed the rejected input, which can be large or circular — the diagnostic + // must never throw or balloon while reporting the real bug. + let serialized: string; + try { + serialized = JSON.stringify(data.issues)?.slice(0, 2000) ?? String(data.issues); + } catch { + serialized = `${data.issues.length} issue(s) that could not be serialized`; + } + super({ + name: 'OpenServiceToolsetOutputMismatchError', + category: Category.CORE_COMMON, + code: 26, + message: `Toolset output did not match its published output schema: ${serialized}`, + }); + } +} + export class WebpackMissingStatsError extends StorybookError { constructor() { super({ 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 diff --git a/code/core/src/shared/open-service/index.ts b/code/core/src/shared/open-service/index.ts index 6e1213469514..490f84bd0602 100644 --- a/code/core/src/shared/open-service/index.ts +++ b/code/core/src/shared/open-service/index.ts @@ -8,17 +8,36 @@ export { defineService } from './service-definition.ts'; export { seedQueryState } from './query-state.ts'; -export { defineToolset } from './toolset-definition.ts'; +export { defineToolset, resolveToolsetDescription } from './toolset-definition.ts'; export type { AnyToolsetDefinition, + AnyToolsetOutcome, ToolsetConsumer, ToolsetCtx, ToolsetDefinition, - ToolsetFormat, ToolsetGetService, ToolsetMethod, + ToolsetMethodDescription, + ToolsetOutcome, + ToolsetTelemetry, + ToolsetTelemetryGroup, } from './toolset-definition.ts'; -export { getRegisteredToolsets, registerToolset } from './toolset-registry.ts'; +export { getRef, MCP_TOOL_NAMES } from './toolset-names.ts'; +export type { ToolsetMethodRef } from './toolset-names.ts'; +export { + clearToolsetRegistry, + getRegisteredToolsets, + getToolset, + registerToolset, +} from './toolset-registry.ts'; +// The errors the registry functions above throw. Catchers must import them from this same entry: +// each core entry bundles its own copy of a class, so an error class imported from another entry +// is a different constructor and `instanceof` silently fails. +export { + OpenServiceDuplicateToolsetError, + OpenServiceMissingToolsetError, +} from '../../server-errors.ts'; +export type { KnownToolsets } from './toolset-types.ts'; export type { DocgenService } from './services/docgen/definition.ts'; export type { DocgenPayload } from './services/docgen/types.ts'; diff --git a/code/core/src/shared/open-service/services/docgen/types.ts b/code/core/src/shared/open-service/services/docgen/types.ts index 21edf6585d21..7ba94fba0678 100644 --- a/code/core/src/shared/open-service/services/docgen/types.ts +++ b/code/core/src/shared/open-service/services/docgen/types.ts @@ -1,4 +1,7 @@ -import type { StrictArgTypes } from '../../../../types/modules/csf.ts'; +// From the defining module, not the `types/modules/csf.ts` barrel: that barrel re-exports through +// the bare `storybook/internal/csf` specifier, which the portable `toolsets-docs` d.ts (this file +// is in its closure) must not reference — its guard allows no `storybook/*` imports. +import type { StrictArgTypes } from '../../../../csf/story.ts'; import type { IndexEntry } from '../../../../types/modules/indexer.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.test.ts b/code/core/src/shared/open-service/toolset-registry.test.ts index 1e8c2e9600c6..c21b17163b91 100644 --- a/code/core/src/shared/open-service/toolset-registry.test.ts +++ b/code/core/src/shared/open-service/toolset-registry.test.ts @@ -2,10 +2,15 @@ import { beforeEach, describe, expect, it } from 'vitest'; import * as v from 'valibot'; +import { + OpenServiceDuplicateToolsetError, + OpenServiceMissingToolsetError, +} from '../../server-errors.ts'; import { defineToolset } from './toolset-definition.ts'; import { clearToolsetRegistry, getRegisteredToolsets, + getToolset, registerToolset, } from './toolset-registry.ts'; @@ -13,11 +18,13 @@ const makeToolset = (id: string, description = `${id} toolset`) => defineToolset({ id, description, + telemetryGroup: 'dev', methods: { noop: { + title: 'No-op', description: 'No-op method.', schema: v.object({}), - handler: () => undefined, + handler: () => ({ ok: true, data: undefined, markdown: '' }) as const, }, }, }); @@ -37,17 +44,28 @@ describe('registerToolset', () => { expect(getRegisteredToolsets()).toEqual([docs, review]); }); - it('is idempotent by id: the first registration wins', () => { - const first = makeToolset('docs', 'first'); - const second = makeToolset('docs', 'second'); - - registerToolset(first); - registerToolset(second); + it('throws on a duplicate id: two hosts claiming one public surface is mis-wiring', () => { + registerToolset(makeToolset('docs', 'first')); - expect(getRegisteredToolsets()).toEqual([first]); + expect(() => registerToolset(makeToolset('docs', 'second'))).toThrow( + OpenServiceDuplicateToolsetError + ); }); it('returns an empty list before any registration', () => { expect(getRegisteredToolsets()).toEqual([]); }); }); + +describe('getToolset', () => { + it('returns a registered toolset by id', () => { + const docs = makeToolset('docs'); + registerToolset(docs); + + expect(getToolset('docs')).toBe(docs); + }); + + it('throws for an unregistered id instead of returning an empty result', () => { + expect(() => getToolset('stories')).toThrow(OpenServiceMissingToolsetError); + }); +}); diff --git a/code/core/src/shared/open-service/toolset-registry.ts b/code/core/src/shared/open-service/toolset-registry.ts index f0b7aa4b0e32..4fa51960d1ff 100644 --- a/code/core/src/shared/open-service/toolset-registry.ts +++ b/code/core/src/shared/open-service/toolset-registry.ts @@ -1,4 +1,9 @@ +import { + OpenServiceDuplicateToolsetError, + OpenServiceMissingToolsetError, +} from '../../server-errors.ts'; import type { AnyToolsetDefinition } from './toolset-definition.ts'; +import type { KnownToolsets } from './toolset-types.ts'; const TOOLSET_REGISTRY_SYMBOL = Symbol.for('storybook.open-service.toolset-registry'); @@ -24,23 +29,41 @@ function getToolsetRegistry(): Map { * * Call it from the same place the paired OSA service registers (for core, the `services` preset * hook). Registration is deliberately independent of the Node preset system so manager- or - * preview-realm toolsets can use the same API later. Idempotent by id, mirroring `registerService`: - * a repeated registration is a no-op. + * preview-realm toolsets can use the same API later. + * + * Unlike `registerService`, a repeated id throws instead of being ignored: services re-register + * legitimately (HMR, repeated composition), while every toolset registration site is a one-shot + * preset hook, so a duplicate id can only mean two hosts claimed the same public surface. */ export function registerToolset(toolset: AnyToolsetDefinition): void { const registry = getToolsetRegistry(); - if (!registry.has(toolset.id)) { - registry.set(toolset.id, toolset); + if (registry.has(toolset.id)) { + throw new OpenServiceDuplicateToolsetError({ toolsetId: toolset.id }); } + + registry.set(toolset.id, toolset); } /** - * Returns the registered toolsets in registration order. + * Returns one registered toolset by id, typed for the core toolsets. * - * Adapter-facing: the MCP server (Milestone 4) and the `storybook tools` CLI (Milestone 5) read the - * public tool surface from here; nothing consumes it before then. + * Throws when the id is not registered — an adapter asking for a toolset that no host wired up is + * a configuration error, not an empty result. */ +export function getToolset(toolsetId: TId): KnownToolsets[TId]; +export function getToolset(toolsetId: string): AnyToolsetDefinition; +export function getToolset(toolsetId: string): AnyToolsetDefinition { + const toolset = getToolsetRegistry().get(toolsetId); + + if (!toolset) { + throw new OpenServiceMissingToolsetError({ toolsetId }); + } + + return toolset; +} + +/** Returns the registered toolsets in registration order. */ export function getRegisteredToolsets(): AnyToolsetDefinition[] { return Array.from(getToolsetRegistry().values()); } diff --git a/code/core/src/shared/open-service/toolset-types.ts b/code/core/src/shared/open-service/toolset-types.ts new file mode 100644 index 000000000000..3133ddbe70b4 --- /dev/null +++ b/code/core/src/shared/open-service/toolset-types.ts @@ -0,0 +1,18 @@ +import type { DocsToolset } from './toolsets/docs/definition.ts'; +import type { ReviewToolset } from './toolsets/review/definition.ts'; +import type { StoriesToolset } from './toolsets/stories/definition.ts'; +import type { TestToolset } from './toolsets/test/definition.ts'; + +/** + * The public toolsets core can register, keyed by id. + * + * Mirrors `core-service-types.ts` for services: it is what makes `getToolset('stories')` return a + * typed toolset instead of an opaque definition. A toolset registered by an addon under its own id + * is still resolvable — it falls through to the untyped `getToolset` overload. + */ +export type KnownToolsets = { + docs: DocsToolset; + stories: StoriesToolset; + test: TestToolset; + review: ReviewToolset; +}; diff --git a/code/core/src/shared/open-service/toolsets/docs/access-local.test.ts b/code/core/src/shared/open-service/toolsets/docs/access-local.test.ts new file mode 100644 index 000000000000..2d0d80cb74a7 --- /dev/null +++ b/code/core/src/shared/open-service/toolsets/docs/access-local.test.ts @@ -0,0 +1,77 @@ +/** + * Selection contract for the local docs access: which engine serves is decided by whether the + * docgen services actually registered, never by the feature flag alone. Composed and uncomposed + * consumers share this helper, so this is the single place the choice is pinned. + */ + +import type { StoryIndex } from 'storybook/internal/types'; + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { createLocalDocsAccess } from './access-local.ts'; +import type { RawManifests } from './access-manifest.ts'; + +const getRegisteredServices = vi.hoisted(() => vi.fn<() => Array<{ id: string }>>(() => [])); +const getService = vi.hoisted(() => + vi.fn(() => { + throw new Error('read from the docgen services'); + }) +); +vi.mock('../../service-registry.ts', () => ({ getRegisteredServices, getService })); + +const manifests: RawManifests = { + components: { + v: 0, + components: { button: { id: 'button', name: 'Button' } }, + meta: { docgen: 'react-docgen', durationMs: 1 }, + }, +}; + +const emptyIndex = { v: 5, entries: {} } as StoryIndex; + +const createAccess = () => + createLocalDocsAccess({ + storyIndex: { getIndex: async () => emptyIndex }, + getManifests: async () => manifests, + }); + +beforeEach(() => { + getRegisteredServices.mockReturnValue([]); +}); + +describe('createLocalDocsAccess', () => { + it('serves from the inline manifests when the docgen services never registered', async () => { + const listing = await createAccess().list({ withStoryIds: false }); + + expect(Object.keys(listing.componentManifest.components)).toEqual(['button']); + expect(getService).not.toHaveBeenCalled(); + }); + + it('serves from the open services once the docgen services registered', async () => { + getRegisteredServices.mockReturnValue([{ id: 'core/docgen' }, { id: 'core/story-docs' }]); + + await expect(createAccess().list({ withStoryIds: false })).rejects.toThrow( + 'read from the docgen services' + ); + }); + + it('falls back to the inline manifests when docgen registered without story-docs', async () => { + getRegisteredServices.mockReturnValue([{ id: 'core/docgen' }]); + + const listing = await createAccess().list({ withStoryIds: false }); + + expect(Object.keys(listing.componentManifest.components)).toEqual(['button']); + expect(getService).not.toHaveBeenCalled(); + }); + + it('probes per call, because it can be created before the services preset hooks run', async () => { + const access = createAccess(); + + await expect(access.list({ withStoryIds: false })).resolves.toBeDefined(); + + getRegisteredServices.mockReturnValue([{ id: 'core/docgen' }, { id: 'core/story-docs' }]); + await expect(access.list({ withStoryIds: false })).rejects.toThrow( + 'read from the docgen services' + ); + }); +}); diff --git a/code/core/src/shared/open-service/toolsets/docs/access-local.ts b/code/core/src/shared/open-service/toolsets/docs/access-local.ts new file mode 100644 index 000000000000..97bb6f22753d --- /dev/null +++ b/code/core/src/shared/open-service/toolsets/docs/access-local.ts @@ -0,0 +1,53 @@ +/** + * Local docs access with registration-based selection between the two engines. + * + * Docgen-server mode moves docs data out of the served manifests and into the open services, but + * the `experimentalDocgenServer` flag alone is not enough to read from them: the docgen + * registrations are skipped for manager-only builds and when no docgen worker is available. Every + * consumer that reads the local Storybook — core's own docs toolset and a composition's local + * source alike — must make the same choice, so it lives here: the services when they actually + * registered, and otherwise the manifests, which every mode still writes. + * + * The registry probe runs per call, because some consumers (addon-mcp's dev-server hook) create + * this access before the `services` preset hooks have necessarily run. + */ + +import type { StoryIndex } from 'storybook/internal/types'; + +import { DOCGEN_SERVICE_ID } from '../../services/docgen/paths.ts'; +import { STORY_DOCS_SERVICE_ID } from '../../services/story-docs/paths.ts'; +import { getRegisteredServices, getService } from '../../service-registry.ts'; +import type { ToolsetGetService } from '../../toolset-definition.ts'; +import { createManifestDocsAccess, type RawManifests } from './access-manifest.ts'; +import { createServiceDocsAccess } from './access-service.ts'; +import type { DocsAccess } from './access.ts'; + +export type LocalDocsAccessOptions = { + storyIndex: { getIndex: () => Promise }; + /** Reads the live inline manifests, e.g. core-server's `loadManifests`. */ + getManifests: () => Promise; +}; + +export function createLocalDocsAccess({ + storyIndex, + getManifests, +}: LocalDocsAccessOptions): DocsAccess { + const serviceAccess = createServiceDocsAccess({ + storyIndex, + getService: getService as ToolsetGetService, + }); + const manifestAccess = createManifestDocsAccess({ getManifests }); + // The service access hard-requires every one of these; a partial registration would make every + // call throw, so it degrades to the manifests instead of trusting that core's registration block + // always registers them together. + const requiredServiceIds = [DOCGEN_SERVICE_ID, STORY_DOCS_SERVICE_ID]; + const pick = () => { + const registered = new Set(getRegisteredServices().map((service) => service.id)); + return requiredServiceIds.every((id) => registered.has(id)) ? serviceAccess : manifestAccess; + }; + + return { + list: (options) => pick().list(options), + resolve: (id) => pick().resolve(id), + }; +} diff --git a/code/core/src/shared/open-service/toolsets/docs/access-manifest.test.ts b/code/core/src/shared/open-service/toolsets/docs/access-manifest.test.ts new file mode 100644 index 000000000000..0cd579d8bb00 --- /dev/null +++ b/code/core/src/shared/open-service/toolsets/docs/access-manifest.test.ts @@ -0,0 +1,100 @@ +import { describe, expect, it, vi } from 'vitest'; + +import { createManifestDocsAccess, type RawManifests } from './access-manifest.ts'; + +const button = { + id: 'button', + name: 'Button', + summary: 'A button', + stories: [{ id: 'button--primary', name: 'Primary', snippet: '', + 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/definition.ts b/code/core/src/shared/open-service/toolsets/test/definition.ts index 7fab504ea9d9..e23a1cd64cfd 100644 --- a/code/core/src/shared/open-service/toolsets/test/definition.ts +++ b/code/core/src/shared/open-service/toolsets/test/definition.ts @@ -1,9 +1,9 @@ import * as v from 'valibot'; -import { defineToolset } from '../../toolset-definition.ts'; +import { defineToolset, type ToolsetCtx, type ToolsetOutcome } from '../../toolset-definition.ts'; import type { StoryIndexAccess } from '../stories/definition.ts'; import { storyInputArraySchema } from '../stories/story-input.ts'; -import { formatTestRun } from './format.ts'; +import { formatTestRun, summarizeTestRun } from './format.ts'; import { createAsyncQueue, runStoryTests, type TestChannel } from './run.ts'; const errorLikeSchema: v.GenericSchema = v.object({ @@ -62,6 +62,8 @@ const testRunResultSchema = v.object({ const testRunOutputSchema = v.variant('status', [ v.object({ status: v.literal('no-stories'), + /** Per-selector lookup failures. When nothing matched, they are the whole answer. */ + notFoundMessages: v.array(v.string()), }), v.object({ status: v.literal('completed'), @@ -82,50 +84,147 @@ const testRunOutputSchema = v.variant('status', [ export type TestRunResult = v.InferOutput; export type TestRunOutput = v.InferOutput; +/** + * What `run` renders: the run result plus whether accessibility tests were part of this run, which + * the result payload itself does not state. + */ +export type TestRunData = TestRunOutput & { a11y: boolean }; + +/** + * The outcome split for `test.run`: a crashed or cancelled run is a failure that still carries its + * full report, so clients keying on the tag cannot count it as a pass while agents keep the + * diagnostic detail. + */ +export type TestRunSuccessData = Extract & { + a11y: boolean; +}; + +export type TestRunFailureData = Extract & { + a11y: boolean; +}; + +const runInputSchema = v.object({ + stories: v.optional( + v.pipe( + storyInputArraySchema, + v.description( + `Stories to test for focused feedback. Omit this field to run tests for all available stories. +Prefer running tests for specific stories while developing to get faster feedback, +and only omit this when you explicitly need to run all tests for comprehensive verification. +Prefer { storyId } when you don't already have story file context, since this avoids filesystem discovery. +Use { storyId } when IDs were discovered from documentation tools. +Use { absoluteStoryPath + exportName } only when you're currently working in a story file and already know those values.` + ) + ) + ), + a11y: v.optional( + v.pipe( + v.boolean(), + v.description( + 'Whether to run accessibility tests. Defaults to true. Disable if you only need component test results.' + ) + ), + true + ), +}); + +type RunInput = v.InferOutput; + +/** + * The accessibility half of this tool's contract only holds when addon-a11y is enabled, so the + * promise is dropped from the description rather than made and then broken. + */ +function describeRun(a11yEnabled: boolean): string { + return ( + `Run story tests. +Run them after editing anything that changes how the UI looks — components, stories, styles, CSS, themes, colors, or design tokens — shell-level substitutes like typecheck, lint, or package.json test scripts do not replace this. +Provide stories for focused runs (faster while iterating), +or omit stories to run all tests for full-project verification. +Use this continuously to monitor test results as you work on your UI components and stories. +Results will include passing/failing status` + + (a11yEnabled + ? `, and accessibility violation reports. +For visual/design accessibility violations (for example color contrast), ask the user before changing styles.` + : '.') + ); +} + +/** + * 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. + */ +async function reportRunTelemetry(data: TestRunData, input: RunInput, ctx: ToolsetCtx) { + const inputStoryCount = input.stories?.length ?? 0; + + if (data.status === 'no-stories') { + await ctx.telemetry?.('tool:runStoryTests', { + runA11y: data.a11y, + inputStoryCount, + matchedStoryCount: 0, + passingStoryCount: 0, + failingStoryCount: 0, + a11yViolationCount: 0, + unhandledErrorCount: 0, + }); + return; + } + + if (data.status !== 'completed') { + return; + } + + await ctx.telemetry?.('tool:runStoryTests', { + 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), + }); +} + export type CreateTestToolsetOptions = { channel: TestChannel; storyIndex: StoryIndexAccess; + /** Whether accessibility tests run alongside component tests (addon-a11y enabled). */ + a11yEnabled: boolean; }; /** * Creates the public test API. Each registration owns a queue because addon-vitest supports one * live test run at a time. */ -export function createTestToolset({ channel, storyIndex }: CreateTestToolsetOptions) { +export function createTestToolset({ channel, storyIndex, a11yEnabled }: CreateTestToolsetOptions) { const queue = createAsyncQueue(); return defineToolset({ id: 'test', description: 'Run Storybook story tests via addon-vitest.', + telemetryGroup: 'test', methods: { run: { - schema: v.object({ - stories: v.optional( - v.pipe( - storyInputArraySchema, - v.description('Stories to test. Omit to run all available stories.') - ) - ), - a11y: v.optional( - v.pipe( - v.boolean(), - v.description('Whether to include accessibility tests. Defaults to true.') - ), - true - ), - }), - description: - 'Runs story tests for the given selectors, or all stories when stories is omitted.', - handler: async (input, ctx) => { + schema: runInputSchema, + title: 'Storybook Tests', + description: describeRun(a11yEnabled), + handler: async ( + input, + ctx + ): Promise> => { const done = await queue.wait(); try { - const result = await runStoryTests({ + const output = await runStoryTests({ channel, getIndex: storyIndex.getIndex, stories: input.stories, a11y: input.a11y, }); - return ctx.format === 'json' ? result : formatTestRun(result); + const data: TestRunData = { ...output, a11y: input.a11y }; + + await reportRunTelemetry(data, input, ctx); + + const markdown = formatTestRun(data, ctx); + return data.status === 'error' || data.status === 'cancelled' + ? { ok: false, data, markdown } + : { ok: true, data, markdown }; } finally { done(); } 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/scripts/build/utils/entry-utils.ts b/scripts/build/utils/entry-utils.ts index 3988b533101b..d503cbb414d0 100644 --- a/scripts/build/utils/entry-utils.ts +++ b/scripts/build/utils/entry-utils.ts @@ -11,6 +11,16 @@ export type BuildEntry = { entryPoint: `./src/${string}`; // the source file to bundle, e.g. "./src/manager-api/index.ts", external?: string[]; // the list of external dependencies to exclude from the bundle dts?: false; // default to generating d.ts files for all entries, except if set to false + /** + * Bundle this entry's d.ts in an isolated single-entry pass, producing one flat self-contained + * file instead of sharing type chunks with the package's other entries. For entries other + * packages bundle into their own dist, where a shared chunk would drag unrelated type surfaces + * along. `external` lists the only imports the flat file may keep — packages every consumer of + * the entry declares itself (it only affects this d.ts pass, never the JS bundle). A producer-side + * test guards each portable artifact: flatness, the import allowlist (kept there as a deliberate + * second copy, so widening it is a reviewer-visible edit), and a size budget. + */ + portable?: { external: string[] }; }; export type BuildEntriesByPlatform = Partial>; diff --git a/scripts/build/utils/generate-types-rolldown.ts b/scripts/build/utils/generate-types-rolldown.ts index bdb867a68e07..cbbc24355e94 100644 --- a/scripts/build/utils/generate-types-rolldown.ts +++ b/scripts/build/utils/generate-types-rolldown.ts @@ -266,10 +266,10 @@ export async function generateTypesFiles( ) { const DIR_REL = relative(DIR_CODE, cwd); - const dtsEntries = Object.values(data.entries) + const dtsBuildEntries = Object.values(data.entries) .flat() - .filter((entry) => entry.dts !== false) - .map((e) => e.entryPoint); + .filter((entry) => entry.dts !== false); + const dtsEntries = dtsBuildEntries.map((e) => e.entryPoint); if (dtsEntries.length === 0) { return; @@ -285,14 +285,25 @@ export async function generateTypesFiles( id.includes(`${sep}node_modules${sep}${dep}${sep}`) ); + // ./src/client-logger/index.ts -> client-logger/index + const entryName = (entryPoint: string) => + entryPoint.replace(/^\.\/src\//, '').replace(/\.tsx?$/, ''); + // Build entry map: { 'client-logger/index': '/absolute/path/src/client-logger/index.ts', ... } const entryMap: Record = {}; for (const entry of dtsEntries) { - // ./src/client-logger/index.ts -> client-logger/index - const name = entry.replace(/^\.\/src\//, '').replace(/\.tsx?$/, ''); - entryMap[name] = join(cwd, entry); + entryMap[entryName(entry)] = join(cwd, entry); } + // Portable entries leave the shared pass: bundled alone, everything outside the entry's own + // external allowlist inlines into one flat file instead of importing shared type chunks (see + // BuildEntry.portable). + const portableByName = new Map( + dtsBuildEntries + .filter((entry) => entry.portable) + .map((entry) => [entryName(entry.entryPoint), entry.portable!]) + ); + // The dts plugin and tsgo derive rootDir from path.dirname(tsconfig). // When rootDir is the package dir, declarations for cross-package imports // would be emitted next to their source files. Fix: create a temporary @@ -304,6 +315,9 @@ export async function generateTypesFiles( const useTsgo = options?.tsgo ?? false; const resolver = options?.resolver ?? 'hybrid'; + const include = [`${relative(DIR_ROOT, cwd)}/src/**/*`]; + const exclude = DTS_EXCLUDES; + // tsgo removed support for `baseUrl`, and ts.getParsedCommandLineOfConfigFile // needs concrete options anyway: resolve the tsconfig chain and write a flat // config. @@ -313,8 +327,8 @@ export async function generateTypesFiles( wrapperTsconfig, JSON.stringify({ compilerOptions, - include: [`${relative(DIR_ROOT, cwd)}/src/**/*`], - exclude: DTS_EXCLUDES, + include, + exclude, }) ); @@ -332,8 +346,8 @@ export async function generateTypesFiles( emitTsconfig, JSON.stringify({ compilerOptions: { ...compilerOptions, ...dtsEmitCompilerOptions(emitDir) }, - include: [`${relative(DIR_ROOT, cwd)}/src/**/*`], - exclude: DTS_EXCLUDES, + include, + exclude, }) ); emitPackageDeclarationsNative(emitTsconfig, wrapperTsconfig, emitDir); @@ -348,38 +362,72 @@ export async function generateTypesFiles( ]) ); - const out = await rolldown({ - input, - external: externalFn, - plugins: [ - createTextAssetStubPlugin(), - ...(resolver === 'hybrid' ? [createTypesFallbackResolverPlugin(externalFn)] : []), - dts({ - cwd, - tsconfig: wrapperTsconfig, - dtsInput: true, - emitDtsOnly: true, - resolver: resolver === 'tsc' ? 'tsc' : 'oxc', - }), - ], - logLevel: 'warn', - }); - - const { output } = await out.write({ - dir: join(cwd, 'dist'), - format: 'es', - // Name shared chunks purely by content hash: the default `[name]-[hash]` - // inherits a base name from whichever module rolldown happens to pick, - // which varies across runs and would make the output non-deterministic. - chunkFileNames: 'chunk-[hash].js', - }); + const sharedInput = Object.fromEntries( + Object.entries(input).filter(([name]) => !portableByName.has(name)) + ); + const portableInputs = Object.entries(input) + .filter(([name]) => portableByName.has(name)) + .map(([name, emitted]) => ({ + input: { [name]: emitted }, + allowedExternal: portableByName.get(name)!.external, + })); + + const bundleDts = async ( + bundleInput: Record, + isExternal: (id: string) => boolean + ) => { + const out = await rolldown({ + input: bundleInput, + external: isExternal, + plugins: [ + createTextAssetStubPlugin(), + ...(resolver === 'hybrid' ? [createTypesFallbackResolverPlugin(isExternal)] : []), + dts({ + cwd, + tsconfig: wrapperTsconfig, + dtsInput: true, + emitDtsOnly: true, + resolver: resolver === 'tsc' ? 'tsc' : 'oxc', + }), + ], + logLevel: 'warn', + }); + + const { output } = await out.write({ + dir: join(cwd, 'dist'), + format: 'es', + // Name shared chunks purely by content hash: the default `[name]-[hash]` + // inherits a base name from whichever module rolldown happens to pick, + // which varies across runs and would make the output non-deterministic. + chunkFileNames: 'chunk-[hash].js', + }); + return output; + }; + + // A portable pass bundles with the package-wide externals plus the entry's allowlist: an + // external that nothing in the kept types references is tree-shaken from the output, so the + // flat file ends up importing only what its closure genuinely uses — which the entry's + // producer-side guard then asserts is exactly the allowlist. + const portableExternalFn = (allowed: string[]) => (id: string) => + externalFn(id) || allowed.some((dep) => id === dep || id.startsWith(`${dep}/`)); + + const outputs = [ + ...(Object.keys(sharedInput).length > 0 ? [await bundleDts(sharedInput, externalFn)] : []), + // One isolated pass per portable entry, so nothing is shared between it and the rest. + ...(await Promise.all( + portableInputs.map(({ input: portableInput, allowedExternal }) => + bundleDts(portableInput, portableExternalFn(allowedExternal)) + ) + )), + ]; // Everything meaningful in this build is a .d.ts file; rolldown still // emits its runtime helper chunk as plain JS, which nothing references. // Only touch our own chunk namespace: the JS bundle writes real entry // files into the same dist directory in parallel. await Promise.all( - output + outputs + .flat() .filter((chunk) => /^chunk-[^/]+\.js$/.test(chunk.fileName)) .map((chunk) => rm(join(cwd, 'dist', chunk.fileName), { force: true })) ); diff --git a/yarn.lock b/yarn.lock index c1da5d4eec6b..2cb38c5adca2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -36599,6 +36599,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"