Repository navigation
Conversation
Contributor
|
Contributor
Author
|
Superseding with a fresh branch name to clear false CONFLICTING state left over from the accidental #35677 merge on this head branch name. |
5 of 9 tasks
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (101)
WalkthroughThe PR moves addon and hosted MCP tools onto shared Storybook toolsets. It adds thin MCP adapters, request-scoped documentation access, outcome and error handling, availability and telemetry wiring, package-boundary tests, and end-to-end coverage. Obsolete MCP implementations and review-channel plumbing are removed. ChangesShared MCP toolset integration
Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MCPRegistry
participant ToolsetAdapter
participant CoreToolset
participant ManifestProvider
MCPClient->>MCPRegistry: invoke MCP tool
MCPRegistry->>ToolsetAdapter: resolve registered method
ToolsetAdapter->>CoreToolset: execute with request context
CoreToolset->>ManifestProvider: read documentation manifest when required
ManifestProvider-->>CoreToolset: return manifest or failure
CoreToolset-->>ToolsetAdapter: return outcome
ToolsetAdapter-->>MCPClient: return MCP content and structured result
✨ Finishing Touches📝 Generate docstrings
Comment |
5 of 9 tasks
2 of 6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces accidentally closed #35677 (a mergeability probe via the GitHub merges API marked that PR merged and deleted this branch; the merge commit on
m2b-core-toolsetswas reverted and this branch tip was restored to0a5ad88a6a4).No intentional code change versus #35677.
Closes #35673 · Closes #35725
Stacked: this PR's base is #35726 — the core toolsets layer this PR consumes; review that one first. #35719 (the
storybook toolsCLI) stacks on this one. Merge #35726 first; GitHub retargets this PR tonextautomatically.What I did
#35726 built Storybook's agent-facing capabilities as core toolsets. This PR is the switch: both MCP surfaces —
@storybook/addon-mcp(the dev-server MCP) and@storybook/mcp(the hosted/composition MCP) — now render their tools from those shared definitions, and the engines they each carried are deleted. Net: −11,199 lines.What remains in each package is an adapter. It resolves a toolset method and maps the outcome onto the MCP reply mechanically: schemas and prose come from the definition,
markdownbecomes the text blocks,databecomesstructuredContent,okbecomesisError, and agent-facing errors surface verbatim. Meaning lives in the toolset — the adapter is not allowed to re-derive it from the data.Concretely, an MCP tool is now a registry row pointing at a toolset method (real code):
The one review question
Does the way the MCP uses the toolsets make sense — and is there business logic still in the MCP that belongs in a toolset? Everything in commits 1–3 should read as mechanical: naming, gating, unwrapping, transport. If you find an adapter interpreting a result — branching on domain data, rewriting prose, computing anything the future CLI consumer would have to duplicate — that is the review finding to raise.
You do not need to review the deleted engines: commit 4/7 is −12,570 lines of pure deletions, and behavioral equality with them is pinned by the e2e wire snapshots (evidence below), not by reading them.
How to review (~1½ hours)
The branch's seven commits are these seven blocks, in this order. Each block heading links straight to its commit's diff — review one commit at a time, and read the block's intro here before opening any file.
Block 1 · The addon adapter — 30 min
Files:
toolset-tools.ts,tool-registry.ts,ui-root.ts,tool-names.ts(4 files, +425 −109)The entire addon-side integration is two ideas. The first is one generic unwrap that turns any toolset outcome into an MCP result —
toolset-tools.ts, 187 lines, read it in full:The second is a declarative registry where every MCP tool is a row pointing at a toolset method — the
docsRowsnippet above is one real row.tool-registry.tswalks the rows; the availability gate × thetoolsetsconfig decides what registers.What to check: nothing in this block may interpret
data— no branching on domain fields, no re-deriving prose from the payload. If you find the adapter deciding what a result means, that is the finding to raise.Block 2 · The hosted adapter — 25 min
Files:
register.ts,error-to-mcp-content.ts,multi-source-manifests.ts,index.ts,types.ts,bin.ts,package.json(7 files, +497 −225)The same unwrap for
@storybook/mcp, with one structural difference: the hosted server builds its docs toolset per request, because the provider and sources belong to the request:What to check:
register.ts(346 lines) in full — the per-request construction, the twin error mapping, and that the package boundary holds.Block 3 · Addon rewiring — 15 min
Files: preset, mcp-handler, availability, telemetry, instructions, auth (16 files, +131 −386)
Mechanical re-pointing: everything that used to call an engine now consults the registry. Net −255 lines. Skim with one question: did any logic sneak in here, or is it all naming and plumbing?
Block 4 · Delete the replaced engines — 0 min
Files: the old tools, manifest pipeline, formatter and utils of both packages (47 files, −12,570, zero insertions)
Don't review. This is the point of the PR: every deleted line is an engine the core toolsets replaced. Behavioral equality is proven by block 6, not by reading these.
Block 5 · Retire the PUSH_REVIEW channel adapter — 5 min
Files:
common-preset.ts,review-channel.ts(+ test),events.ts(4 files, +3 −200)#35726 deliberately kept this adapter alive so released addon-mcp versions kept working against the new core; with the addon switched over in this PR, it retires. One glance.
Block 6 · The adapter tests and the e2e proof — 15 min
Files: adapter unit tests,
dist-contract.test.ts,test-storybooks/mcp(22 files, +1,569 −366)Skim as evidence, not as code under review: the contract tests pin the unwrap (outcome →
isError,structuredContentnarrowing), gating (including the preview app resource), telemetry, andagentFacingerror surfacing; the e2e suite exercises the real wire against real servers.Block 7 · Docs and bookkeeping — 5 min
Files:
AGENTS.md, agent-eval templates,yarn.lock(4 files, +89 −57)The AGENTS.md architecture section documents the end state of the whole layer — worth an actual read: it is also the text future agents working in this repo obey.
Why skipping the deletions is safe:
isErrormapping,structuredContentnarrowing, telemetry,agentFacingerror surfacing, and toolset gating (including the preview app resource)@storybook/mcpships withoutstorybookat runtimedist-contract.test.tspins the published dependency surface; the docs toolset arrives through the portablestorybook/internal/toolsets-docsentryChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn nx run-many -t compilecd test-storybooks/mcp && yarn install && yarn vitest run --project=e2e— 46 tests, inline snapshots byte-identical to the old enginesyarn storybookintest-storybooks/mcp, connect an MCP client tohttp://localhost:6006/mcp— the tool list and every tool response match the released addondisplay-review→ UI) — now served by the review toolset; the PUSH_REVIEW channel event no longer exists@storybook/mcpagainst the Chromatic-hosted Storybooks — listing groups per source, lookups takestorybookIdDocumentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.Made with Cursor
Made with Cursor