Repository navigation
Skills M4 (8/9): @storybook/mcp runs on the shared docs toolset - #35735
Closed
valentinpalkovic wants to merge 1 commit into
Closed
valentinpalkovic wants to merge 1 commit into
valentinpalkovic wants to merge 1 commit into
Conversation
Contributor
|
1 of 9 tasks
Contributor
WalkthroughThe MCP package now delegates documentation tools, manifest contracts, instructions, and composition access to Storybook shared toolsets. It adds hosted registration outcome handling, provider-based tests, and published-package contract checks while removing duplicated local manifest and formatting implementations. ChangesShared documentation toolset migration
The local manifest fetching, formatting, adaptation, parsing, instruction, and documentation-tool modules are removed. Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant MCPServer
participant DocsRegistration
participant StorybookToolset
participant ManifestProvider
MCPClient->>MCPServer: invoke documentation tool
MCPServer->>DocsRegistration: dispatch request
DocsRegistration->>StorybookToolset: create and invoke shared tool
StorybookToolset->>ManifestProvider: fetch manifests for source
ManifestProvider-->>StorybookToolset: manifests or typed error
StorybookToolset-->>DocsRegistration: Markdown and structured outcome
DocsRegistration-->>MCPServer: validated MCP result
MCPServer-->>MCPClient: content and error status
Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
code/lib/mcp/src/tools/get-documentation-for-story.test.ts (1)
25-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfigure the provider mock in
beforeEach.These helpers create direct
vi.fn(async ...)implementations outsidebeforeEach. They also access the mock withoutvi.mocked().
code/lib/mcp/src/tools/get-documentation-for-story.test.ts#L25-L44: Declare the provider mock before the test cases. Configure its implementation throughvi.mocked(manifestProvider).mockImplementation(...)inbeforeEach.code/lib/mcp/src/tools/get-documentation.test.ts#L26-L45: Apply the same typed mock setup. Keep per-test manifest data inserved.code/lib/mcp/src/tools/list-all-documentation.test.ts#L31-L50: Apply the same typed mock setup. Updateservedfor source-specific cases instead of creating new provider mocks in test cases.As per coding guidelines, use
vi.mocked()to type and access mocked functions, implement mock behaviors inbeforeEachblocks, and avoid mock implementations outsidebeforeEachblocks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/mcp/src/tools/get-documentation-for-story.test.ts` around lines 25 - 44, Refactor the manifestProvider mocks in code/lib/mcp/src/tools/get-documentation-for-story.test.ts (lines 25-44), code/lib/mcp/src/tools/get-documentation.test.ts (lines 26-45), and code/lib/mcp/src/tools/list-all-documentation.test.ts (lines 31-50): declare each provider mock before the tests, configure it via vi.mocked(manifestProvider).mockImplementation(...) inside beforeEach, and keep per-test manifests in served. In list-all-documentation.test.ts, update served for source-specific cases instead of creating new provider mocks in individual tests; use vi.mocked() for all mock access and implementations.Source: Coding guidelines
code/lib/mcp/src/utils/error-to-mcp-content.test.ts (1)
39-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the
RequiresOwnMcpErrorbranch.The suite covers the
ManifestGetErrorand genericErrorbranches. It does not coverRequiresOwnMcpError. That branch is the only one that returns content withoutisError, and it now delegates text to the sharedformatRequiresOwnMcpNotice. A test pins that contract so a later change in the shared formatter does not silently flip the result to an error.💚 Suggested additional test
it('returns a requires-own-mcp notice without the error flag', () => { const error = new RequiresOwnMcpError( { id: 'design-system', title: 'Design System' }, 'https://example.com/mcp' ); const result = errorToMCPContent(error); expect(result.isError).toBeUndefined(); expect(result.content[0].text).toBe( formatRequiresOwnMcpNotice(error.source, error.endpoint) ); });Confirm the
RequiresOwnMcpErrorconstructor signature before use.As per coding guidelines: "For security-sensitive paths and legacy migration logic, prefer broader edge-case coverage".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/mcp/src/utils/error-to-mcp-content.test.ts` around lines 39 - 70, Add a test covering the RequiresOwnMcpError branch in errorToMCPContent, using its actual constructor signature and representative source and endpoint values. Assert the returned text matches formatRequiresOwnMcpNotice with the error’s source and endpoint, and verify isError is undefined to preserve the non-error response contract.Source: Coding guidelines
code/lib/mcp/src/dist-contract.test.ts (1)
11-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord why this test cannot use
memfs.The coding guidelines require filesystem tests that touch
node:fsto usememfswithvolreset inbeforeEach. This test reads the real build output, somemfswould make every assertion vacuous. The exception is correct, but it is not stated. Add one line to the file header explaining that this suite asserts against the realdistartifact and therefore reads the real filesystem. That prevents a later contributor or a lint rule from converting it tomemfsand silently disabling the guard.As per coding guidelines: "Filesystem tests touching
node:fsornode:fs/promisesmust usememfs; resetvolinbeforeEach, seed virtual files, and use spy redirects rather than full async factory mocks".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/mcp/src/dist-contract.test.ts` around lines 11 - 19, Add a file-header comment to the dist-contract test explaining that it intentionally reads the real filesystem and build artifacts, so it must not use memfs. Keep the existing imports and test behavior unchanged.Source: Coding guidelines
code/lib/mcp/src/tools/outcome-unwrap.test.ts (1)
21-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
vi.mockwithspy: trueand move the mock behavior intobeforeEach.The coding guidelines require
spy: truefor all package and file mocks,vi.mocked()for typed access, and mock behaviors implemented inbeforeEachblocks. This file uses an async factory withoutspy: true, and it mutates the real toolset object returned byactual.createDocsToolset. The mock behavior is then set inside each test case at lines 105, 124, 136-141, 158, 171-173, and 185-187, which the guidelines also prohibit.The
vi.hoistedholder is no longer needed oncevi.mockeddrives the implementation.♻️ Proposed restructure
-const listMethod = vi.hoisted(() => ({ - outputSchema: undefined as unknown, - handler: undefined as ((input: unknown, ctx: unknown) => unknown) | undefined, -})); - -vi.mock('storybook/internal/toolsets-docs', async (importOriginal) => { - const actual = await importOriginal<typeof import('storybook/internal/toolsets-docs')>(); - return { - ...actual, - createDocsToolset: (options: never) => { - const toolset = actual.createDocsToolset(options); - const list = toolset.methods.list as { outputSchema?: unknown; handler: unknown }; - if (listMethod.outputSchema) { - list.outputSchema = listMethod.outputSchema; - } - if (listMethod.handler) { - list.handler = listMethod.handler; - } - return toolset; - }, - }; -}); +vi.mock('storybook/internal/toolsets-docs', { spy: true });Then add a helper and drive it from
beforeEach:const listMethod: { outputSchema?: unknown; handler?: (input: unknown, ctx: unknown) => unknown; } = {}; beforeEach(() => { listMethod.outputSchema = undefined; listMethod.handler = undefined; vi.mocked(createDocsToolset).mockImplementation((options) => { const toolset = unmocked.createDocsToolset(options); const list = toolset.methods.list as { outputSchema?: unknown; handler: unknown }; if (listMethod.outputSchema) { list.outputSchema = listMethod.outputSchema; } if (listMethod.handler) { list.handler = listMethod.handler; } return toolset; }); });Each test then only sets
listMethod.outputSchemaandlistMethod.handleras data, not as an implementation.As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests", "Implement mock behaviors inbeforeEachblocks in Vitest tests", and "Avoid inline mock implementations within test cases in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/mcp/src/tools/outcome-unwrap.test.ts` around lines 21 - 42, Restructure the mock setup in outcome-unwrap.test.ts to use vi.mock with spy: true and vi.mocked(createDocsToolset), preserving the original implementation through an unmocked reference. Remove the vi.hoisted holder and async mock factory, define the listMethod data holder directly, and initialize/reset it plus install the createDocsToolset mockImplementation in beforeEach. Update each test to assign only listMethod.outputSchema or listMethod.handler values, with no inline mock implementations.Source: Coding guidelines
code/lib/mcp/src/tools/register.ts (1)
304-324: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHandle unknown entry kinds explicitly. Preserve the
ResolvedDocsEntryunion or checkentry.kind === 'doc'explicitly. The current fallback treats any future kind as a doc and can report a found entry as not found.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/mcp/src/tools/register.ts` around lines 304 - 324, Update the entry selection logic in the documentation callback to handle only the known ResolvedDocsEntry variants: use component for kind "component", doc for kind "doc", and treat unknown kinds as absent. Preserve the existing onGetDocumentation behavior while ensuring future entry kinds cannot be reported as found documentation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/lib/mcp/src/dist-contract.test.ts`:
- Around line 42-43: Update STORYBOOK_IMPORT_RE to allow optional whitespace
after import and before the require call’s opening parenthesis, while preserving
detection of standard, minified side-effect, dynamic import, and require forms.
Keep the existing storybook package-path matching behavior unchanged.
In `@code/lib/mcp/src/index.ts`:
- Around line 29-42: Update the exported resolveComponentStories API in the
index module to preserve its published Promise<ComponentManifest> return type
for TypeScript embedders, rather than exposing the migrated
Promise<ComponentManifestEntry> type from storybook/internal/toolsets-docs. Keep
the existing runtime export and other migrated signatures unchanged.
In `@code/lib/mcp/src/tools/register.ts`:
- Around line 276-283: Replace the unsafe result.content[0].text dereferences in
the documentation listing callback and addGetDocumentationTool with a shared
resultText helper defined next to call. Have the helper concatenate every
content block’s text with blank lines, returning an empty string when content is
empty, and pass its result to onListAllDocumentation.
- Around line 56-68: Align getSources metadata selection with the runtime
behavior in toolsetFor: when sources contain only URL-less entries, do not
configure metadata as multi-source requiring storybookId while runtime uses
unscoped single-source access. Reuse the runtime’s single-source mode for this
case, or explicitly reject the multiSource configuration before creating
composition documentation sources.
In `@code/lib/mcp/src/utils/error-to-mcp-content.ts`:
- Around line 19-33: Replace the copy-sensitive instanceof checks in
error-to-mcp-content.ts#19-33 with a shared copy-independent discriminator for
RequiresOwnMcpError and ManifestGetError, preserving the existing notice,
prefix, and cause handling. In code/lib/mcp/src/index.ts#36, document importing
RequiresOwnMcpError from `@storybook/mcp` or expose predicate helpers so embedders
avoid instanceof; apply equivalent guidance for ManifestGetError if embedders
construct it.
---
Nitpick comments:
In `@code/lib/mcp/src/dist-contract.test.ts`:
- Around line 11-19: Add a file-header comment to the dist-contract test
explaining that it intentionally reads the real filesystem and build artifacts,
so it must not use memfs. Keep the existing imports and test behavior unchanged.
In `@code/lib/mcp/src/tools/get-documentation-for-story.test.ts`:
- Around line 25-44: Refactor the manifestProvider mocks in
code/lib/mcp/src/tools/get-documentation-for-story.test.ts (lines 25-44),
code/lib/mcp/src/tools/get-documentation.test.ts (lines 26-45), and
code/lib/mcp/src/tools/list-all-documentation.test.ts (lines 31-50): declare
each provider mock before the tests, configure it via
vi.mocked(manifestProvider).mockImplementation(...) inside beforeEach, and keep
per-test manifests in served. In list-all-documentation.test.ts, update served
for source-specific cases instead of creating new provider mocks in individual
tests; use vi.mocked() for all mock access and implementations.
In `@code/lib/mcp/src/tools/outcome-unwrap.test.ts`:
- Around line 21-42: Restructure the mock setup in outcome-unwrap.test.ts to use
vi.mock with spy: true and vi.mocked(createDocsToolset), preserving the original
implementation through an unmocked reference. Remove the vi.hoisted holder and
async mock factory, define the listMethod data holder directly, and
initialize/reset it plus install the createDocsToolset mockImplementation in
beforeEach. Update each test to assign only listMethod.outputSchema or
listMethod.handler values, with no inline mock implementations.
In `@code/lib/mcp/src/tools/register.ts`:
- Around line 304-324: Update the entry selection logic in the documentation
callback to handle only the known ResolvedDocsEntry variants: use component for
kind "component", doc for kind "doc", and treat unknown kinds as absent.
Preserve the existing onGetDocumentation behavior while ensuring future entry
kinds cannot be reported as found documentation.
In `@code/lib/mcp/src/utils/error-to-mcp-content.test.ts`:
- Around line 39-70: Add a test covering the RequiresOwnMcpError branch in
errorToMCPContent, using its actual constructor signature and representative
source and endpoint values. Assert the returned text matches
formatRequiresOwnMcpNotice with the error’s source and endpoint, and verify
isError is undefined to preserve the non-error response contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdc189ce-792e-49f8-97f7-b8ed85a5fd77
⛔ Files ignored due to path filters (2)
code/lib/mcp/src/utils/manifest-formatter/__snapshots__/markdown.test.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (32)
code/lib/mcp/bin.tscode/lib/mcp/package.jsoncode/lib/mcp/src/dist-contract.test.tscode/lib/mcp/src/globals.d.tscode/lib/mcp/src/index.tscode/lib/mcp/src/instructions.mdcode/lib/mcp/src/tools/get-documentation-for-story.test.tscode/lib/mcp/src/tools/get-documentation-for-story.tscode/lib/mcp/src/tools/get-documentation.test.tscode/lib/mcp/src/tools/get-documentation.tscode/lib/mcp/src/tools/list-all-documentation.test.tscode/lib/mcp/src/tools/list-all-documentation.tscode/lib/mcp/src/tools/outcome-unwrap.test.tscode/lib/mcp/src/tools/register.tscode/lib/mcp/src/types.tscode/lib/mcp/src/utils/adapt-core-manifest.test.tscode/lib/mcp/src/utils/adapt-core-manifest.tscode/lib/mcp/src/utils/dedent.tscode/lib/mcp/src/utils/error-to-mcp-content.test.tscode/lib/mcp/src/utils/error-to-mcp-content.tscode/lib/mcp/src/utils/get-manifest.test.tscode/lib/mcp/src/utils/get-manifest.tscode/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.test.tscode/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.tscode/lib/mcp/src/utils/manifest-formatter/markdown.test.tscode/lib/mcp/src/utils/manifest-formatter/markdown.tscode/lib/mcp/src/utils/map-with-concurrency.tscode/lib/mcp/src/utils/multi-source-manifests.tscode/lib/mcp/src/utils/parse-react-docgen.test.tscode/lib/mcp/src/utils/parse-react-docgen.tscode/lib/mcp/src/utils/requires-own-mcp.test.tscode/lib/mcp/src/utils/requires-own-mcp.ts
💤 Files with no reviewable changes (19)
- code/lib/mcp/src/globals.d.ts
- code/lib/mcp/src/utils/requires-own-mcp.test.ts
- code/lib/mcp/src/utils/dedent.ts
- code/lib/mcp/src/utils/adapt-core-manifest.ts
- code/lib/mcp/src/tools/list-all-documentation.ts
- code/lib/mcp/src/utils/get-manifest.test.ts
- code/lib/mcp/src/utils/adapt-core-manifest.test.ts
- code/lib/mcp/src/utils/map-with-concurrency.ts
- code/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.test.ts
- code/lib/mcp/src/instructions.md
- code/lib/mcp/src/utils/manifest-formatter/markdown.ts
- code/lib/mcp/src/utils/parse-react-docgen.test.ts
- code/lib/mcp/src/utils/requires-own-mcp.ts
- code/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.ts
- code/lib/mcp/src/utils/parse-react-docgen.ts
- code/lib/mcp/src/utils/manifest-formatter/markdown.test.ts
- code/lib/mcp/src/utils/get-manifest.ts
- code/lib/mcp/src/tools/get-documentation-for-story.ts
- code/lib/mcp/src/tools/get-documentation.ts
Comment on lines
+42
to
+43
| const STORYBOOK_IMPORT_RE = | ||
| /(?:\bfrom\s*|\brequire\(\s*|\bimport\(\s*|\bimport\s+)["']storybook(?:["']|\/)/; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The regex misses a minified side-effect import.
\bimport\s+ requires at least one whitespace character after import. A minifier emits a side-effect import as import"storybook/internal/toolsets-docs"; with no space. That form matches none of the four alternatives, so the assertion at line 72 passes even though the artifact keeps an external storybook reference. This is the exact regression the test exists to catch.
\brequire\(\s* has the same class of gap for require ("storybook").
🐛 Proposed fix
const STORYBOOK_IMPORT_RE =
- /(?:\bfrom\s*|\brequire\(\s*|\bimport\(\s*|\bimport\s+)["']storybook(?:["']|\/)/;
+ /(?:\bfrom\s*|\brequire\s*\(\s*|\bimport\s*\(\s*|\bimport\s*)["']storybook(?:["']|\/)/;\bimport\s* covers both import "storybook" and the minified import"storybook", and it still covers import(.
Also applies to: 71-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/lib/mcp/src/dist-contract.test.ts` around lines 42 - 43, Update
STORYBOOK_IMPORT_RE to allow optional whitespace after import and before the
require call’s opening parenthesis, while preserving detection of standard,
minified side-effect, dynamic import, and require forms. Keep the existing
storybook package-path matching behavior unchanged.
Comment on lines
29
to
+42
| // Export manifest constants and utilities | ||
| export { | ||
| adaptCoreComponent, | ||
| adaptCoreDoc, | ||
| adaptCoreStories, | ||
| COMPONENT_MANIFEST_PATH, | ||
| DOCS_MANIFEST_PATH, | ||
| RequiresOwnMcpError, | ||
| getMultiSourceManifests, | ||
| resolveComponentEntry, | ||
| resolveComponentStories, | ||
| resolveDoc, | ||
| } from './utils/get-manifest.ts'; | ||
| resolveDocEntry as resolveDoc, | ||
| } from 'storybook/internal/toolsets-docs'; | ||
|
|
||
| // Adapters from Storybook's open-service ("core format") payloads to the internal shape. | ||
| export { adaptCoreComponent, adaptCoreDoc, adaptCoreStories } from './utils/adapt-core-manifest.ts'; | ||
| export { getMultiSourceManifests } from './utils/multi-source-manifests.ts'; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the previously exported local signatures against the shared implementations.
git show HEAD~1:code/lib/mcp/src/index.ts 2>/dev/null | rg -n 'export' || true
for sym in adaptCoreComponent adaptCoreDoc adaptCoreStories resolveComponentEntry resolveComponentStories resolveDocEntry; do
echo "=== $sym ==="
rg -nP --type=ts -A6 "export (async )?function $sym\b|export const $sym" code
doneRepository: storybookjs/storybook
Length of output: 6668
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '=== previous MCP exports and local implementations ==='
git show HEAD~1:code/lib/mcp/src/index.ts | sed -n '1,55p'
for path in \
code/lib/mcp/src/utils/adapt-core-manifest.ts \
code/lib/mcp/src/utils/resolve-component-entry.ts \
code/lib/mcp/src/utils/resolve-component-stories.ts \
code/lib/mcp/src/utils/resolve-doc-entry.ts
do
printf '\n=== %s ===\n' "$path"
git cat-file -e "HEAD~1:$path" 2>/dev/null && git show "HEAD~1:$path" | sed -n '1,240p' || echo 'not present at HEAD~1'
done
printf '\n=== current toolsets-docs export surface ===\n'
rg -n -A35 -B8 'toolsets-docs|adaptCoreComponent|resolveDocEntry|resolveComponentEntry|resolveComponentStories' \
code/core/src code/lib/mcp/src --glob '*.ts'Repository: storybookjs/storybook
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '=== previous get-manifest signatures and bodies ==='
git show HEAD~1:code/lib/mcp/src/utils/get-manifest.ts |
rg -n -A45 -B5 'export (async )?function (resolveComponentEntry|resolveComponentStories|resolveDoc)|export const (resolveComponentEntry|resolveComponentStories|resolveDoc)|type ManifestProvider|type Source'
printf '\n%s\n' '=== current shared resolver signatures and bodies ==='
for path in \
code/core/src/shared/open-service/toolsets/docs/access-provider.ts \
code/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.ts
do
printf '\n=== %s ===\n' "$path"
rg -n -A48 -B5 'export (async )?function (resolveComponentEntry|resolveComponentStories|resolveDocEntry|adaptCoreComponent|adaptCoreDoc|adaptCoreStories)' "$path"
done
printf '\n%s\n' '=== shared toolset export declarations ==='
rg -n -A5 -B5 'export .*from .*access-provider|export .*from .*adapt-core-manifest|resolveDocEntry|resolveComponentEntry|resolveComponentStories' \
code/core/src/shared/open-service/toolsets/docs --glob '*.ts'Repository: storybookjs/storybook
Length of output: 36297
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '=== previous MCP manifest type declarations ==='
git show HEAD~1:code/lib/mcp/src/types.ts |
rg -n -A35 -B5 'export type (ComponentManifest|ComponentManifestEntry|DocEntry|Doc|Story|CoreMdxDoc|CoreDocgenComponent)|from .storybook/internal/toolsets-docs'
printf '\n%s\n' '=== current shared manifest type declarations ==='
rg -n -A35 -B5 'export type (ComponentManifest|ComponentManifestEntry|DocEntry|Doc|MdxDoc|CoreDocgenComponent|Story)' \
code/core/src/shared/open-service/toolsets/docs --glob '*.ts'
printf '\n%s\n' '=== old and current adapter signatures ==='
git show HEAD~1:code/lib/mcp/src/utils/adapt-core-manifest.ts | rg -n 'export function'
rg -n 'export function' code/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.tsRepository: storybookjs/storybook
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '=== current working manifest types ==='
sed -n '150,215p' code/core/src/shared/open-service/toolsets/docs/manifest-formatter/manifest-types.ts
printf '\n%s\n' '=== current exported adapter and manifest types ==='
rg -n -A3 -B3 'export type .*ComponentManifest|export type .*DocEntry|export type .*MdxDoc|adaptCore' \
code/core/src/shared/open-service/toolsets/docs/public.ts \
code/core/src/shared/open-service/toolsets/docs/manifest-formatter/index.ts \
code/core/src/shared/open-service/toolsets/docs/manifest-formatter/*.ts
printf '\n%s\n' '=== previous adapter declarations and core payload declarations ==='
git show HEAD~1:code/lib/mcp/src/utils/adapt-core-manifest.ts | rg -n 'export function'
git show HEAD~1:code/lib/mcp/src/types.ts | sed -n '316,390p'Repository: storybookjs/storybook
Length of output: 20336
Preserve resolveComponentStories's published return type.
The other migrated signatures and the resolveDoc alias remain compatible. resolveComponentStories now returns Promise<ComponentManifestEntry> instead of the previous Promise<ComponentManifest>. Preserve the previous type for TypeScript embedders. This is a compile-time API change, not a runtime failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/lib/mcp/src/index.ts` around lines 29 - 42, Update the exported
resolveComponentStories API in the index module to preserve its published
Promise<ComponentManifest> return type for TypeScript embedders, rather than
exposing the migrated Promise<ComponentManifestEntry> type from
storybook/internal/toolsets-docs. Keep the existing runtime export and other
migrated signatures unchanged.
Comment on lines
+56
to
+68
| function getSources(context: StorybookContext | undefined): DocsSource[] | undefined { | ||
| const sources = context?.sources; | ||
| if (!sources?.some((source) => source.url)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| return createCompositionDocsSources({ | ||
| sources, | ||
| manifestProvider: context?.manifestProvider, | ||
| getRequest: () => context?.request, | ||
| resolveEntry: context?.resolveEntry, | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the shared toolset derives descriptions and schemas from sources.
rg -nP --type=ts -C10 'export function (createDocsToolset|resolveToolsetDescription)' code
rg -nP --type=ts -C4 'MCP_TOOL_NAMES|MCP_TOOL_TITLES' code -g '!**/mcp/**'Repository: storybookjs/storybook
Length of output: 16301
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- register.ts ---'
cat -n code/lib/mcp/src/tools/register.ts | sed -n '1,150p'
printf '%s\n' '--- docs definition ---'
cat -n code/core/src/shared/open-service/toolsets/docs/definition.ts | sed -n '238,520p'
printf '%s\n' '--- metadataToolset and source construction ---'
rg -n -C12 --type=ts 'metadataToolset|createCompositionDocsSources|multiSource|manifestProvider|resolveEntry' code/lib/mcp code/core/src/shared/open-serviceRepository: storybookjs/storybook
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- register.ts ---'
cat -n code/lib/mcp/src/tools/register.ts | sed -n '1,125p'
printf '%s\n' '--- docs definition 245-430 ---'
cat -n code/core/src/shared/open-service/toolsets/docs/definition.ts | sed -n '245,430p'
printf '%s\n' '--- source utilities ---'
rg -n -C8 --type=ts 'function createCompositionDocsSources|const createCompositionDocsSources|export .*createCompositionDocsSources|function listSources|export .*listSources' code/core/src code/lib/mcp/srcRepository: storybookjs/storybook
Length of output: 16337
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- docs descriptions and source selection ---'
cat -n code/core/src/shared/open-service/toolsets/docs/definition.ts | sed -n '1,120p'
cat -n code/core/src/shared/open-service/toolsets/docs/definition.ts | sed -n '120,245p'
printf '%s\n' '--- multi-source implementation ---'
cat -n code/core/src/shared/open-service/toolsets/docs/multi-source.ts | sed -n '1,115p'
printf '%s\n' '--- registration metadata and runtime calls ---'
cat -n code/lib/mcp/src/tools/register.ts | sed -n '119,280p'
printf '%s\n' '--- StorybookContext source type and callers ---'
rg -n -C10 --type=ts 'export type StorybookContext|interface StorybookContext|sources\??:|add.*Documentation|toolMetadata\\(' code/lib/mcp/srcRepository: storybookjs/storybook
Length of output: 24074
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- context and metadata option usage ---'
rg -n -C8 --type=ts 'StorybookContext|multiSource\\??:|addListAllDocumentationTool|getListAllDocumentationToolMetadata|getDocumentationToolMetadata|getStoryDocumentationToolMetadata' code/lib/mcp/src
printf '%s\n' '--- metadata and schema tests ---'
rg -n -C8 --type=ts 'metadata|storybookId|description|title|multiSource' code/lib/mcp/src/tools/*test.ts | head -n 500
printf '%s\n' '--- source definition ---'
rg -n -C12 --type=ts 'export (type|interface) Source|type Source =' code/core/src/shared/open-service/toolsets/docsRepository: storybookjs/storybook
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- StorybookContext and Source definitions ---'
cat -n code/lib/mcp/src/types.ts | sed -n '42,75p'
rg -n -C6 --type=ts 'export type Source|export interface Source' code/core/src/shared/open-service/toolsets/docs code/lib/mcp/src/types.ts
printf '%s\n' '--- targeted metadata assertions in tests ---'
rg -n --type=ts -e 'get(ListAllDocumentation|Documentation|StoryDocumentation)ToolMetadata' -e 'MCP_TOOL_TITLES' -e 'MCP_TOOL_NAMES' code/lib/mcp/src --glob '*test.ts'
printf '%s\n' '--- static verifier ---'
python3 - <<'PY'
from pathlib import Path
register = Path('code/lib/mcp/src/tools/register.ts').read_text()
definition = Path('code/core/src/shared/open-service/toolsets/docs/definition.ts').read_text()
assert "source: { id: 'local', title: 'Local' }" in register
assert "resolveToolsetDescription(definition.description, ctx)" in register
assert "function describeList(ctx: ToolsetCtx)" in definition
assert "source.id" not in definition[definition.index('function describeList'):definition.index('export function createDocsToolset')]
assert "source.title" not in definition[definition.index('function describeList'):definition.index('export function createDocsToolset')]
assert "storybookIdField" in definition
print('metadata source labels are not read by descriptions or schemas')
PYRepository: storybookjs/storybook
Length of output: 6487
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- provider access source handling ---'
cat -n code/core/src/shared/open-service/toolsets/docs/access-provider.ts | sed -n '1,180p'
printf '%s\n' '--- behavioral model verifier ---'
python3 - <<'PY'
from pathlib import Path
register = Path('code/lib/mcp/src/tools/register.ts').read_text()
definition = Path('code/core/src/shared/open-service/toolsets/docs/definition.ts').read_text()
# The metadata constructor supplies source values only to select the multi-source branch.
assert "sources: [{ source: { id: 'local', title: 'Local' }, access: unavailable }]" in register
assert "const multiSource = !!sources?.length;" in definition
assert "resolveToolsetDescription(definition.description, ctx)" in register
# Descriptions are resolved from ToolsetCtx, not from DocsSource values.
description_block = definition[definition.index('function describeList'):definition.index('const storybookIdField')]
assert 'source.id' not in description_block
assert 'source.title' not in description_block
assert 'storybookId' in description_block
# The only metadata schema difference is the presence of storybookId on lookup methods.
show_block = definition[definition.index('const showSchema'):definition.index('/** The access for a lookup')]
assert 'storybookIdField' in show_block
assert 'id:' in show_block
# With no URL, getSources selects the single-source branch; that branch still has a DocsAccess
# and its lookup handler does not use storybookId to select another source.
assert "if (!sources?.some((source) => source.url))" in register
runtime_block = register[register.index('function toolsetFor'):register.index('/** Metadata')]
assert 'docsAccess: createProviderDocsAccess' in runtime_block
assert 'createDocsToolset({ sources })' in runtime_block
print('descriptions and schemas do not embed synthetic source labels')
print('multi-source metadata adds storybookId; urlless runtime uses one provider access and does not route by storybookId')
PYRepository: storybookjs/storybook
Length of output: 7843
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- local-only source tests and source fixtures ---'
rg -n -C12 --type=ts 'sources =|const sources|url: undefined|id: .local.|multiSource: true' code/lib/mcp/src/tools/get-documentation.test.ts code/lib/mcp/src/tools/get-documentation-for-story.test.ts code/lib/mcp/src/tools/list-all-documentation.test.ts
printf '%s\n' '--- access factory ---'
rg -n -C12 --type=ts 'export function createProviderDocsAccess|function createProviderDocsAccess' code/core/src/shared/open-service/toolsets/docs/access-provider.tsRepository: storybookjs/storybook
Length of output: 12177
Align metadata mode with runtime source selection.
When multiSource: true is used with only URL-less sources, metadata requires storybookId, but toolsetFor() uses unscoped single-source access and ignores that value. Use the same mode for metadata and runtime, or reject this configuration. Synthetic source labels do not affect descriptions or schemas.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/lib/mcp/src/tools/register.ts` around lines 56 - 68, Align getSources
metadata selection with the runtime behavior in toolsetFor: when sources contain
only URL-less entries, do not configure metadata as multi-source requiring
storybookId while runtime uses unscoped single-source access. Reuse the
runtime’s single-source mode for this case, or explicitly reject the multiSource
configuration before creating composition documentation sources.
Comment on lines
+276
to
+283
| if (manifests) { | ||
| await context?.onListAllDocumentation?.({ | ||
| context: context!, | ||
| manifests, | ||
| resultText: result.content[0].text, | ||
| ...(listing?.sources ? { sources: listing.sources.map(toSourceManifests) } : {}), | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
result.content[0].text throws when a method returns no Markdown block.
Line 229 converts outcome.markdown with Array.isArray(outcome.markdown) ? outcome.markdown : [outcome.markdown]. An empty array passes through unchanged, so result.content becomes []. Line 280 then reads content[0].text and throws a TypeError inside the tool handler, after call already returned successfully. The MCP client receives a transport failure instead of a tool result.
The same dereference exists at line 320 in addGetDocumentationTool.
🐛 Proposed fix
Add a helper next to call:
function resultText(result: { content: Array<{ text: string }> }): string {
return result.content.map((block) => block.text).join('\n\n');
}Then use it at both call sites:
resultText: result.content[0].text,
+ resultText: resultText(result),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/lib/mcp/src/tools/register.ts` around lines 276 - 283, Replace the
unsafe result.content[0].text dereferences in the documentation listing callback
and addGetDocumentationTool with a shared resultText helper defined next to
call. Have the helper concatenate every content block’s text with blank lines,
returning an empty string when content is empty, and pass its result to
onListAllDocumentation.
Comment on lines
+19
to
+33
| if (error instanceof RequiresOwnMcpError) { | ||
| return { | ||
| content: [{ type: 'text', text: formatRequiresOwnMcpNotice(error.source, error.endpoint) }], | ||
| }; | ||
| } | ||
|
|
||
| const errorPrefix = | ||
| error instanceof ManifestGetError ? 'Error getting manifest' : 'Unexpected error'; | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
|
|
||
| let fullMessage = `${errorPrefix}: ${errorMessage}`; | ||
| if (error instanceof ManifestGetError && error.cause) { | ||
| const causeMessage = error.cause instanceof Error ? error.cause.message : String(error.cause); | ||
| fullMessage += `\nCaused by: ${causeMessage}`; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Toolset error classes have per-copy identity, so instanceof is unreliable in this package.
src/dist-contract.test.ts asserts that this package bundles its own copy of storybook/internal/toolsets-docs and declares no runtime dependency on storybook. Every error class from the toolset therefore exists once inside this bundle and again in any real storybook instance in the same process. register.ts lines 175-183 already act on this fact for the agent-facing trait, which uses a property read "because it travels across bundle copies." The two sites below still depend on class identity. Choose one discriminator strategy and apply it to both.
code/lib/mcp/src/utils/error-to-mcp-content.ts#L19-L33: replace theinstanceof RequiresOwnMcpErrorandinstanceof ManifestGetErrorchecks with a copy-independent discriminator, for example a stablenamevalue or a branded marker property. An embedder callback such asmanifestProviderorresolveEntrycan throw an instance built by a different copy; today that instance is reported to the agent asUnexpected error: ...and is also logged as a bug atregister.tsline 254.code/lib/mcp/src/index.ts#L36-L36: document that embedders must importRequiresOwnMcpErrorfrom@storybook/mcpand not fromstorybook, or expose a predicate helper such asisRequiresOwnMcpErroralongside the class so embedders never needinstanceof.
Apply the same treatment to ManifestGetError if embedders are expected to construct it.
📍 Affects 2 files
code/lib/mcp/src/utils/error-to-mcp-content.ts#L19-L33(this comment)code/lib/mcp/src/index.ts#L36-L36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/lib/mcp/src/utils/error-to-mcp-content.ts` around lines 19 - 33, Replace
the copy-sensitive instanceof checks in error-to-mcp-content.ts#19-33 with a
shared copy-independent discriminator for RequiresOwnMcpError and
ManifestGetError, preserving the existing notice, prefix, and cause handling. In
code/lib/mcp/src/index.ts#36, document importing RequiresOwnMcpError from
`@storybook/mcp` or expose predicate helpers so embedders avoid instanceof; apply
equivalent guidance for ManifestGetError if embedders construct it.
2 of 9 tasks
1 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
1 participant
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.
Closes #
Part 8 of 9 of the Skills M4 migration. Stacked on #35734, replaces #35677.
What I did
@storybook/mcpis the standalone MCP server you can point at a published Storybook without running a dev server, and it carries the second of the two hand-written tool engines this stack exists to remove. It becomes a thin adapter over the same docs toolset that core now owns, and its own engine is deleted.Nothing about the published surface changes: the same tool names, descriptions and output shapes, and the package still ships without pulling Storybook in as a runtime dependency, which is enforced by a test on the built output.
The diff is overwhelmingly deletion. The only part that needs a careful read is the adapter itself; the rest is "nothing imports these any more".
A few genuinely dead leftovers in that package are deliberately not cleaned up here, so the end of this stack matches the original change exactly. They are worth a follow-up.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
yarn nx run-many -t compile. The hosted package resolves core through its built output, so an unbuilt tree will mislead you here.list-all-documentation,get-documentationandget-documentation-for-story, and compare the Markdown against the same calls made with the version of the package on the base branch. They must match character for character.Documentation
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.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>