Repository navigation
Skills M4 (7/9): dev-server docs tools run on the shared toolset, with composition - #35734
valentinpalkovic wants to merge 1 commit into
Conversation
The three documentation tools the dev server exposes over MCP now run on the docs toolset that lives in core, instead of on the separate hosted MCP package. That package is no longer a dependency of the addon, and the addon's parallel copy of the manifest reading and formatting code is gone with it. Composition support moves into the toolset too: the composed Storybooks, the provider that fetches their manifests and the local Storybook's own reader are now inputs to the toolset rather than plumbing spread across the addon. A composition still builds its toolset per request, since what it reads belongs to the request. Core gains a dependency-light entry point for the docs toolset. Its type declarations are produced as one flat self-contained file so the hosted package can bundle them, guarded by a test that fails on the producing side if the entry ever picks up an import it should not have. The tools that clients see are unchanged: same names, titles, descriptions, schemas and rendered output.
|
WalkthroughThe PR moves MCP documentation handling into a shared core docs toolset. It adds provider-backed and multi-source access, updates addon integration, exposes a portable public entry point, and removes legacy manifest access and dependency overrides. ChangesShared documentation toolset
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}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent-eval/lib/templates.test.ts (1)
91-92: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRestore an assertion for
@storybook/mcp.Lines 91-92 remove the only assertion for this dependency. The test can now pass if the generated manifest keeps a
workspace:spec or uses another incorrect value.If the fixture still uses
@storybook/mcp: "workspace:*", assert that the rewritten value equalspackageJson.version. Otherwise, add a fixture that verifies the dependency is absent.Proposed assertion for the existing workspace fixture
+ expect( + (rewritten.dependencies as Record<string, unknown>)['`@storybook/mcp`'] + ).toBe(packageJson.version);As per coding guidelines, test public contracts and externally observable side effects rather than private implementation details.
🤖 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 `@agent-eval/lib/templates.test.ts` around lines 91 - 92, Restore coverage for `@storybook/mcp` in the relevant rewritten manifest test: when the fixture declares it as workspace:*, assert its rewritten value equals packageJson.version, or add a fixture and assertion confirming the dependency is absent if that is the intended contract. Keep the existing workspace-spec assertion and other dependency checks unchanged.Source: Coding guidelines
🧹 Nitpick comments (8)
code/addons/mcp/src/tools/tool-registry-composition.test.ts (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock
../telemetry.tswithspy: trueand access it throughvi.mocked().The current mock replaces the module with a bare factory and uses the hoisted
vi.fn()directly. The repository rules require thespy: trueoption and typed access throughvi.mocked().♻️ Proposed refactor
-const collectTelemetry = vi.hoisted(() => vi.fn()); -vi.mock('../telemetry.ts', () => ({ collectTelemetry })); +vi.mock('../telemetry.ts', { spy: true });Then read the spy inside the suite:
import { collectTelemetry } from '../telemetry.ts'; const collectTelemetryMock = vi.mocked(collectTelemetry); beforeEach(() => { registerCoreToolsetsForTest(); collectTelemetryMock.mockClear(); collectTelemetryMock.mockResolvedValue(undefined); });Replace the
collectTelemetryassertions withcollectTelemetryMock.As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests" and "Usevi.mocked()to type and access the mocked functions 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/addons/mcp/src/tools/tool-registry-composition.test.ts` around lines 13 - 14, Update the telemetry mock in tool-registry-composition.test.ts to use vi.mock('../telemetry.ts', { spy: true }) and import collectTelemetry from that module. Create a typed collectTelemetryMock via vi.mocked(collectTelemetry), use it for mock setup and clearing in beforeEach, and replace all direct collectTelemetry assertions with collectTelemetryMock.Source: Coding guidelines
code/core/src/shared/open-service/toolsets/docs/access-provider.ts (1)
433-459: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOne broken story
$refdiscards every resolved row for the source.
mapWithConcurrencyusesPromise.all, so the first rejection rejects the whole batch. Thecatchthen drops all results, including components whose story$refresolved successfully. The comment states the intent is to lose story ids for the unreachable rows only.Isolate the failure per component instead.
♻️ Proposed refactor
- try { - const resolved = await mapWithConcurrency( - Object.entries(components), - STORY_REF_CONCURRENCY, - async ([id, component]) => - [ - id, - await resolveComponentStories(component, request(), manifestProvider, source), - ] as const - ); - for (const [id, component] of resolved) { - components[id] = component; - } - } catch (error) { - // One composed source must not lose its whole listing because a single story `$ref` was - // unreachable: leave its rows unresolved instead. A single Storybook has no other source - // to fall back on, so there the failure is the answer. - if (!source) { - throw error; - } - } + // One composed source must not lose its whole listing because a single story `$ref` was + // unreachable: leave the affected row unresolved instead. A single Storybook has no other + // source to fall back on, so there the failure is the answer. + const resolved = await mapWithConcurrency( + Object.entries(components), + STORY_REF_CONCURRENCY, + async ([id, component]) => { + try { + return [ + id, + await resolveComponentStories(component, request(), manifestProvider, source), + ] as const; + } catch (error) { + if (!source) { + throw error; + } + return [id, component] as const; + } + } + ); + for (const [id, component] of resolved) { + components[id] = component; + }🤖 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/core/src/shared/open-service/toolsets/docs/access-provider.ts` around lines 433 - 459, Update the withStoryIds resolution flow around mapWithConcurrency and resolveComponentStories to handle failures per component rather than rejecting the entire batch. Preserve successfully resolved components and leave only failed rows unresolved for composed sources; retain the existing behavior of rethrowing for a single Storybook source when source is absent.code/core/src/shared/open-service/toolsets/docs/sources.test.ts (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a path without a trailing slash.
getSourceMcpEndpointappends the missing trailing slash before resolvingmcp. The current cases cover the root URL and a path that already ends in a slash. A path without a trailing slash exercises that normalization.♻️ Proposed addition
['https://example.com', 'https://example.com/mcp'], + ['https://example.com/storybook', 'https://example.com/storybook/mcp'], ['https://example.com/storybook/', 'https://example.com/storybook/mcp'],🤖 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/core/src/shared/open-service/toolsets/docs/sources.test.ts` around lines 5 - 11, Add a test case to the existing getSourceMcpEndpoint parameterized suite using a URL with a non-root path that lacks a trailing slash, and assert the expected path-normalized /mcp endpoint. Keep the existing root, trailing-slash, and query/fragment cases unchanged.code/core/src/shared/open-service/toolsets/docs/sources.ts (1)
29-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEncode the "exactly one outcome" rule in the type.
The comment states that exactly one of
manifests,errorornoticecarries the outcome. The current shape allows all three at once, or none. A discriminated union removes the invalid states, and consumers such asformatMultiSourceManifestsToListsthen get exhaustive checking.♻️ Proposed refactor
-export type SourceListing = { - source: Source; - manifests?: AllManifests; - error?: string; - notice?: RequiresOwnMcpNotice; -}; +export type SourceListing = { source: Source } & ( + | { manifests: AllManifests; error?: never; notice?: never } + | { manifests?: never; error: string; notice?: never } + | { manifests?: never; error?: never; notice: RequiresOwnMcpNotice } +);As per coding guidelines: "Use static TypeScript types and existing lint rules to encode invariants whenever practical".
🤖 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/core/src/shared/open-service/toolsets/docs/sources.ts` around lines 29 - 38, Update the SourceListing type to a discriminated union requiring source plus exactly one outcome: manifests, error, or notice. Preserve the existing field types and source property while making the other outcome fields unavailable in each variant, so consumers such as formatMultiSourceManifestsToLists receive exhaustive type checking.Source: Coding guidelines
code/core/src/shared/open-service/toolsets/docs/definition.ts (1)
250-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared
idfield ofshowSchema.Both branches repeat the same
iddefinition. One shared constant keeps the description in one place.♻️ Proposed refactor
+ const idField = { + id: v.pipe(v.string(), v.description('The component or docs entry ID (e.g., "button")')), + }; + // A composition needs the caller to say which Storybook they mean; a single one must not ask. const showSchema = multiSource - ? v.object({ - id: v.pipe(v.string(), v.description('The component or docs entry ID (e.g., "button")')), - ...storybookIdField, - }) - : v.object({ - id: v.pipe(v.string(), v.description('The component or docs entry ID (e.g., "button")')), - }); + ? v.object({ ...idField, ...storybookIdField }) + : v.object({ ...idField });🤖 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/core/src/shared/open-service/toolsets/docs/definition.ts` around lines 250 - 261, Extract the repeated id field definition from showSchema into a shared constant, then reuse it in both the multiSource and non-multiSource object schemas while preserving the existing validation and description.code/core/src/shared/open-service/toolsets/docs/portable-dist.test.ts (1)
14-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the exemption from the memfs rule.
This test reads the real filesystem through
node:fs. The repository rule requiresmemfsfor filesystem tests. The rationale in the file header is sound, because the guarded property is the artifact on disk. Confirm the exemption with the maintainers, or record it where the rule is defined, so a later cleanup pass does not convert this suite tomemfsand remove its value.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/core/src/shared/open-service/toolsets/docs/portable-dist.test.ts` around lines 14 - 22, Record and confirm the intentional real-filesystem exemption for the portable distribution test using node:fs, preferably in the repository rule documentation or an appropriate maintainer-approved note. Preserve the existing artifact-on-disk test behavior and do not convert it to memfs.Source: Coding guidelines
code/core/src/shared/open-service/toolsets/docs/definition.test.ts (1)
159-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the multi-source paths.
This suite exercises only the single-source
toolsetfrom line 45. The newselectSourcebehavior is untested: a missingstorybookId, an unknownstorybookId, and thesourceCounttelemetry field on a composed listing. These are the branches most likely to regress.🤖 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/core/src/shared/open-service/toolsets/docs/definition.test.ts` around lines 159 - 196, Extend the usage-reporting tests around the existing run helper to exercise the composed multi-source toolset and its selectSource behavior: verify missing storybookId uses the default source, unknown storybookId follows the expected fallback or error path, and a composed list telemetry payload includes the correct sourceCount. Reuse the existing telemetry assertions and multi-source fixtures/symbols rather than changing single-source coverage.code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.ts (1)
1490-1513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining multi-source branches.
The new suite pins only the
requires-own-mcpnotice. Theerror:branch and the component/docs listing branch offormatMultiSourceManifestsToListsstay unpinned, including thewithStoryIdssub-bullets and the##heading level that differs from the single-source formatter. This file states it binds the Markdown every consumer receives, so add one listing case and one error case.🤖 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/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.ts` around lines 1490 - 1513, The test suite around formatMultiSourceManifestsToLists currently covers only requires-own-mcp notices. Add one test covering the component/docs listing branch, including withStoryIds sub-bullets and its ## heading level, and one test covering the error notice branch with the expected error: output; assert complete inline snapshots for both cases.
🤖 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/core/src/shared/open-service/toolsets/docs/access-provider-isolation.test.ts`:
- Around line 79-99: Update the manifestProvider fallback payload in the
access.list concurrency test so it contains the value requested by each story
ref, rather than always returning only c0. Ensure every cN pointer resolves
successfully, allowing the fan-out to complete while preserving the existing
concurrency assertions.
In `@code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts`:
- Around line 185-190: Update the tests around getManifests so rejection
assertions execute directly rather than only inside catch blocks. Ensure both
affected cases verify that the returned rejection error message does not contain
componentsManifest, while still failing if getManifests resolves.
In `@code/core/src/shared/open-service/toolsets/docs/access-provider.ts`:
- Around line 70-97: Update defaultManifestProvider to create a bounded-timeout
AbortSignal and pass it in the fetch call for each manifestUrl request. Ensure
the signal aborts stalled responses and preserves the existing ManifestGetError
handling for non-OK responses.
- Around line 122-135: Update the getUrl helper in fetchManifests to prefer
source.url when a remote source is provided, falling back to the request-derived
manifest URL for local manifests and to the existing unknown-source value when
neither is available. Ensure ManifestGetError instances use this corrected URL
for failures from composed remote sources.
In `@code/core/src/shared/open-service/toolsets/docs/definition.ts`:
- Around line 245-247: Update createDocsToolset to assert that options.sources
is non-empty whenever sources is provided, rejecting { sources: [] } before
handlers can dereference docsAccess; preserve the existing multiSource behavior
for valid non-empty sources and docsAccess configurations.
In `@code/core/src/shared/open-service/toolsets/docs/instructions.ts`:
- Around line 21-24: Update the “Multi-Source Requests” guidance in the
instructions template to mention that both ${MCP_TOOL_NAMES['docs.show']} and
${MCP_TOOL_NAMES['docs.showStory']} require storybookId when selecting a source,
while preserving the existing docs.list behavior.
---
Outside diff comments:
In `@agent-eval/lib/templates.test.ts`:
- Around line 91-92: Restore coverage for `@storybook/mcp` in the relevant
rewritten manifest test: when the fixture declares it as workspace:*, assert its
rewritten value equals packageJson.version, or add a fixture and assertion
confirming the dependency is absent if that is the intended contract. Keep the
existing workspace-spec assertion and other dependency checks unchanged.
---
Nitpick comments:
In `@code/addons/mcp/src/tools/tool-registry-composition.test.ts`:
- Around line 13-14: Update the telemetry mock in
tool-registry-composition.test.ts to use vi.mock('../telemetry.ts', { spy: true
}) and import collectTelemetry from that module. Create a typed
collectTelemetryMock via vi.mocked(collectTelemetry), use it for mock setup and
clearing in beforeEach, and replace all direct collectTelemetry assertions with
collectTelemetryMock.
In `@code/core/src/shared/open-service/toolsets/docs/access-provider.ts`:
- Around line 433-459: Update the withStoryIds resolution flow around
mapWithConcurrency and resolveComponentStories to handle failures per component
rather than rejecting the entire batch. Preserve successfully resolved
components and leave only failed rows unresolved for composed sources; retain
the existing behavior of rethrowing for a single Storybook source when source is
absent.
In `@code/core/src/shared/open-service/toolsets/docs/definition.test.ts`:
- Around line 159-196: Extend the usage-reporting tests around the existing run
helper to exercise the composed multi-source toolset and its selectSource
behavior: verify missing storybookId uses the default source, unknown
storybookId follows the expected fallback or error path, and a composed list
telemetry payload includes the correct sourceCount. Reuse the existing telemetry
assertions and multi-source fixtures/symbols rather than changing single-source
coverage.
In `@code/core/src/shared/open-service/toolsets/docs/definition.ts`:
- Around line 250-261: Extract the repeated id field definition from showSchema
into a shared constant, then reuse it in both the multiSource and
non-multiSource object schemas while preserving the existing validation and
description.
In
`@code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.ts`:
- Around line 1490-1513: The test suite around formatMultiSourceManifestsToLists
currently covers only requires-own-mcp notices. Add one test covering the
component/docs listing branch, including withStoryIds sub-bullets and its ##
heading level, and one test covering the error notice branch with the expected
error: output; assert complete inline snapshots for both cases.
In `@code/core/src/shared/open-service/toolsets/docs/portable-dist.test.ts`:
- Around line 14-22: Record and confirm the intentional real-filesystem
exemption for the portable distribution test using node:fs, preferably in the
repository rule documentation or an appropriate maintainer-approved note.
Preserve the existing artifact-on-disk test behavior and do not convert it to
memfs.
In `@code/core/src/shared/open-service/toolsets/docs/sources.test.ts`:
- Around line 5-11: Add a test case to the existing getSourceMcpEndpoint
parameterized suite using a URL with a non-root path that lacks a trailing
slash, and assert the expected path-normalized /mcp endpoint. Keep the existing
root, trailing-slash, and query/fragment cases unchanged.
In `@code/core/src/shared/open-service/toolsets/docs/sources.ts`:
- Around line 29-38: Update the SourceListing type to a discriminated union
requiring source plus exactly one outcome: manifests, error, or notice. Preserve
the existing field types and source property while making the other outcome
fields unavailable in each variant, so consumers such as
formatMultiSourceManifestsToLists receive exhaustive type checking.
🪄 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: c638c99c-715a-4691-a85d-e9d9d697e758
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (44)
agent-eval/lib/templates.test.tsagent-eval/lib/templates.tscode/addons/mcp/package.jsoncode/addons/mcp/src/auth/composition-auth.test.tscode/addons/mcp/src/auth/composition-auth.tscode/addons/mcp/src/auth/resolve-composition-sources.tscode/addons/mcp/src/instructions/build-server-instructions.tscode/addons/mcp/src/manifests/__fixtures__/core-manifest.fixture.jsoncode/addons/mcp/src/manifests/core-format-contract.test.tscode/addons/mcp/src/manifests/in-process-provider.test.tscode/addons/mcp/src/manifests/in-process-provider.tscode/addons/mcp/src/manifests/service-access.test.tscode/addons/mcp/src/manifests/service-access.tscode/addons/mcp/src/manifests/vendored.tscode/addons/mcp/src/mcp-handler.test.tscode/addons/mcp/src/mcp-handler.tscode/addons/mcp/src/preset.tscode/addons/mcp/src/storybook-ai-metadata.test.tscode/addons/mcp/src/tools/tool-registry-composition.test.tscode/addons/mcp/src/tools/tool-registry.tscode/addons/mcp/src/tools/toolset-tools.tscode/addons/mcp/src/types.tscode/core/build-config.tscode/core/package.jsoncode/core/src/shared/open-service/services/docgen/types.tscode/core/src/shared/open-service/toolsets/docs/access-provider-isolation.test.tscode/core/src/shared/open-service/toolsets/docs/access-provider.test.tscode/core/src/shared/open-service/toolsets/docs/access-provider.tscode/core/src/shared/open-service/toolsets/docs/definition.test.tscode/core/src/shared/open-service/toolsets/docs/definition.tscode/core/src/shared/open-service/toolsets/docs/instructions.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.tscode/core/src/shared/open-service/toolsets/docs/map-with-concurrency.tscode/core/src/shared/open-service/toolsets/docs/multi-source.test.tscode/core/src/shared/open-service/toolsets/docs/multi-source.tscode/core/src/shared/open-service/toolsets/docs/portable-dist.test.tscode/core/src/shared/open-service/toolsets/docs/public.tscode/core/src/shared/open-service/toolsets/docs/sources.test.tscode/core/src/shared/open-service/toolsets/docs/sources.tscode/core/src/shared/open-service/toolsets/estimate-tokens.test.tscode/core/src/shared/open-service/toolsets/estimate-tokens.tsscripts/build/utils/entry-utils.tsscripts/build/utils/generate-types-rolldown.ts
💤 Files with no reviewable changes (9)
- code/addons/mcp/src/manifests/fixtures/core-manifest.fixture.json
- code/addons/mcp/package.json
- code/addons/mcp/src/manifests/in-process-provider.ts
- code/addons/mcp/src/manifests/service-access.ts
- code/addons/mcp/src/auth/composition-auth.test.ts
- code/addons/mcp/src/manifests/core-format-contract.test.ts
- code/addons/mcp/src/manifests/service-access.test.ts
- code/addons/mcp/src/manifests/in-process-provider.test.ts
- code/addons/mcp/src/manifests/vendored.ts
| const access = createProviderDocsAccess({ | ||
| source: SOURCE, | ||
| manifestProvider: async (_request: unknown, path: string) => { | ||
| if (path.endsWith('components.json')) { | ||
| return JSON.stringify({ v: 1, components }); | ||
| } | ||
| if (path.endsWith('docs.json')) { | ||
| return JSON.stringify({ v: 1, docs: {} }); | ||
| } | ||
| inFlight += 1; | ||
| peak = Math.max(peak, inFlight); | ||
| await new Promise((resolve) => setTimeout(resolve, 1)); | ||
| inFlight -= 1; | ||
| return JSON.stringify({ c0: [] }); | ||
| }, | ||
| }); | ||
|
|
||
| await access.list({ withStoryIds: true }); | ||
|
|
||
| expect(peak).toBeLessThanOrEqual(16); | ||
| expect(peak).toBeGreaterThan(1); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The concurrency test measures a mostly failing fan-out.
The provider returns { "c0": [] } for every story $ref. The pointer for component c1 and later is c1, c2, and so on, so fetchRefValue throws for 59 of the 60 refs. list swallows those errors because source is set, and mapWithConcurrency rejects on the first failure. The assertion on peak therefore describes a short, aborted fan-out rather than sustained bounded concurrency.
Serve a payload that matches the requested pointer so every ref resolves.
💚 Proposed fix
inFlight += 1;
peak = Math.max(peak, inFlight);
await new Promise((resolve) => setTimeout(resolve, 1));
inFlight -= 1;
- return JSON.stringify({ c0: [] });
+ return JSON.stringify(
+ Object.fromEntries(Array.from({ length: total }, (_, index) => [`c${index}`, []]))
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const access = createProviderDocsAccess({ | |
| source: SOURCE, | |
| manifestProvider: async (_request: unknown, path: string) => { | |
| if (path.endsWith('components.json')) { | |
| return JSON.stringify({ v: 1, components }); | |
| } | |
| if (path.endsWith('docs.json')) { | |
| return JSON.stringify({ v: 1, docs: {} }); | |
| } | |
| inFlight += 1; | |
| peak = Math.max(peak, inFlight); | |
| await new Promise((resolve) => setTimeout(resolve, 1)); | |
| inFlight -= 1; | |
| return JSON.stringify({ c0: [] }); | |
| }, | |
| }); | |
| await access.list({ withStoryIds: true }); | |
| expect(peak).toBeLessThanOrEqual(16); | |
| expect(peak).toBeGreaterThan(1); | |
| const access = createProviderDocsAccess({ | |
| source: SOURCE, | |
| manifestProvider: async (_request: unknown, path: string) => { | |
| if (path.endsWith('components.json')) { | |
| return JSON.stringify({ v: 1, components }); | |
| } | |
| if (path.endsWith('docs.json')) { | |
| return JSON.stringify({ v: 1, docs: {} }); | |
| } | |
| inFlight += 1; | |
| peak = Math.max(peak, inFlight); | |
| await new Promise((resolve) => setTimeout(resolve, 1)); | |
| inFlight -= 1; | |
| return JSON.stringify( | |
| Object.fromEntries(Array.from({ length: total }, (_, index) => [`c${index}`, []])) | |
| ); | |
| }, | |
| }); | |
| await access.list({ withStoryIds: true }); | |
| expect(peak).toBeLessThanOrEqual(16); | |
| expect(peak).toBeGreaterThan(1); |
🤖 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/core/src/shared/open-service/toolsets/docs/access-provider-isolation.test.ts`
around lines 79 - 99, Update the manifestProvider fallback payload in the
access.list concurrency test so it contains the value requested by each story
ref, rather than always returning only c0. Ensure every cN pointer resolves
successfully, allowing the fan-out to complete while preserving the existing
concurrency assertions.
| try { | ||
| await getManifests(request); | ||
| } catch (error) { | ||
| expect((error as Error).message).not.toContain('componentsManifest'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Two assertions never run if the call resolves.
Both blocks assert inside catch. If getManifests resolves, the catch body is skipped and the test passes without checking anything. Assert on the rejection directly.
💚 Proposed fix
- try {
- await getManifests(request);
- } catch (error) {
- expect((error as Error).message).not.toContain('componentsManifest');
- }
+ await expect(getManifests(request)).rejects.not.toThrow('componentsManifest'); const request = createMockRequest('https://example.com/mcp');
- try {
- await getManifests(request);
- } catch (error) {
- expect(error).toBeInstanceOf(ManifestGetError);
- expect((error as ManifestGetError).url).toBe(
- 'https://example.com/manifests/components.json'
- );
- }
+ await expect(getManifests(request)).rejects.toMatchObject({
+ name: 'ManifestGetError',
+ url: 'https://example.com/manifests/components.json',
+ });Also applies to: 296-304
🤖 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/core/src/shared/open-service/toolsets/docs/access-provider.test.ts`
around lines 185 - 190, Update the tests around getManifests so rejection
assertions execute directly rather than only inside catch blocks. Ensure both
affected cases verify that the returned rejection error message does not contain
componentsManifest, while still failing if getManifests resolves.
| async function defaultManifestProvider( | ||
| request: Request | undefined, | ||
| path: string | ||
| ): Promise<string> { | ||
| if (!request) { | ||
| throw new ManifestGetError( | ||
| "Request is required when using the default manifest provider. You must either pass the original request forward to the server context, or set a custom manifestProvider that doesn't need the request." | ||
| ); | ||
| } | ||
| const manifestUrl = getManifestUrlFromRequest(request, path); | ||
| const response = await fetch(manifestUrl); | ||
|
|
||
| if (!response.ok) { | ||
| throw new ManifestGetError( | ||
| `Failed to fetch manifest: ${response.status} ${response.statusText}`, | ||
| manifestUrl | ||
| ); | ||
| } | ||
|
|
||
| const contentType = response.headers.get('content-type'); | ||
| if (!contentType?.includes('application/json')) { | ||
| throw new ManifestGetError( | ||
| `Invalid content type: expected application/json, got ${contentType}`, | ||
| manifestUrl | ||
| ); | ||
| } | ||
| return response.text(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the default manifest fetch.
defaultManifestProvider calls fetch with no abort signal. A composed Storybook that accepts the connection but never responds holds the MCP request open indefinitely. The listing path fans out to many $ref fetches, so one slow host can occupy the request for a long time.
Pass an AbortSignal so each manifest fetch fails within a bounded time.
🛡️ Proposed fix
+/** Upper bound on one manifest fetch, so an unresponsive host cannot hold the request open. */
+const MANIFEST_FETCH_TIMEOUT_MS = 15_000;
+
async function defaultManifestProvider(
request: Request | undefined,
path: string
): Promise<string> {
if (!request) {
throw new ManifestGetError(
"Request is required when using the default manifest provider. You must either pass the original request forward to the server context, or set a custom manifestProvider that doesn't need the request."
);
}
const manifestUrl = getManifestUrlFromRequest(request, path);
- const response = await fetch(manifestUrl);
+ const response = await fetch(manifestUrl, {
+ signal: AbortSignal.timeout(MANIFEST_FETCH_TIMEOUT_MS),
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function defaultManifestProvider( | |
| request: Request | undefined, | |
| path: string | |
| ): Promise<string> { | |
| if (!request) { | |
| throw new ManifestGetError( | |
| "Request is required when using the default manifest provider. You must either pass the original request forward to the server context, or set a custom manifestProvider that doesn't need the request." | |
| ); | |
| } | |
| const manifestUrl = getManifestUrlFromRequest(request, path); | |
| const response = await fetch(manifestUrl); | |
| if (!response.ok) { | |
| throw new ManifestGetError( | |
| `Failed to fetch manifest: ${response.status} ${response.statusText}`, | |
| manifestUrl | |
| ); | |
| } | |
| const contentType = response.headers.get('content-type'); | |
| if (!contentType?.includes('application/json')) { | |
| throw new ManifestGetError( | |
| `Invalid content type: expected application/json, got ${contentType}`, | |
| manifestUrl | |
| ); | |
| } | |
| return response.text(); | |
| } | |
| /** Upper bound on one manifest fetch, so an unresponsive host cannot hold the request open. */ | |
| const MANIFEST_FETCH_TIMEOUT_MS = 15_000; | |
| async function defaultManifestProvider( | |
| request: Request | undefined, | |
| path: string | |
| ): Promise<string> { | |
| if (!request) { | |
| throw new ManifestGetError( | |
| "Request is required when using the default manifest provider. You must either pass the original request forward to the server context, or set a custom manifestProvider that doesn't need the request." | |
| ); | |
| } | |
| const manifestUrl = getManifestUrlFromRequest(request, path); | |
| const response = await fetch(manifestUrl, { | |
| signal: AbortSignal.timeout(MANIFEST_FETCH_TIMEOUT_MS), | |
| }); | |
| if (!response.ok) { | |
| throw new ManifestGetError( | |
| `Failed to fetch manifest: ${response.status} ${response.statusText}`, | |
| manifestUrl | |
| ); | |
| } | |
| const contentType = response.headers.get('content-type'); | |
| if (!contentType?.includes('application/json')) { | |
| throw new ManifestGetError( | |
| `Invalid content type: expected application/json, got ${contentType}`, | |
| manifestUrl | |
| ); | |
| } | |
| return response.text(); | |
| } |
🤖 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/core/src/shared/open-service/toolsets/docs/access-provider.ts` around
lines 70 - 97, Update defaultManifestProvider to create a bounded-timeout
AbortSignal and pass it in the fetch call for each manifestUrl request. Ensure
the signal aborts stalled responses and preserves the existing ManifestGetError
handling for non-OK responses.
| export async function fetchManifests( | ||
| request?: Request, | ||
| manifestProvider?: ManifestProvider, | ||
| source?: Source | ||
| ): Promise<AllManifests> { | ||
| const provider = manifestProvider ?? defaultManifestProvider; | ||
|
|
||
| const [componentResult, docsResult] = await Promise.allSettled([ | ||
| provider(request, COMPONENT_MANIFEST_PATH, source), | ||
| provider(request, DOCS_MANIFEST_PATH, source), | ||
| ]); | ||
|
|
||
| const getUrl = (path: string) => | ||
| request ? getManifestUrlFromRequest(request, path) : 'Unknown manifest source'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Error URLs name the local origin for remote sources.
getUrl always derives the manifest URL from request.url. When source.url is set, the provider reads the manifest from the remote origin, as createManifestProvider in code/addons/mcp/src/auth/composition-auth.ts shows. The resulting ManifestGetError.url then reports the local Storybook origin for a remote failure, which misdirects debugging of composed sources.
🐛 Proposed fix
const getUrl = (path: string) =>
- request ? getManifestUrlFromRequest(request, path) : 'Unknown manifest source';
+ source?.url
+ ? `${source.url.replace(/\/$/, '')}/${path.replace(/^\.\//, '')}`
+ : request
+ ? getManifestUrlFromRequest(request, path)
+ : 'Unknown manifest source';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function fetchManifests( | |
| request?: Request, | |
| manifestProvider?: ManifestProvider, | |
| source?: Source | |
| ): Promise<AllManifests> { | |
| const provider = manifestProvider ?? defaultManifestProvider; | |
| const [componentResult, docsResult] = await Promise.allSettled([ | |
| provider(request, COMPONENT_MANIFEST_PATH, source), | |
| provider(request, DOCS_MANIFEST_PATH, source), | |
| ]); | |
| const getUrl = (path: string) => | |
| request ? getManifestUrlFromRequest(request, path) : 'Unknown manifest source'; | |
| export async function fetchManifests( | |
| request?: Request, | |
| manifestProvider?: ManifestProvider, | |
| source?: Source | |
| ): Promise<AllManifests> { | |
| const provider = manifestProvider ?? defaultManifestProvider; | |
| const [componentResult, docsResult] = await Promise.allSettled([ | |
| provider(request, COMPONENT_MANIFEST_PATH, source), | |
| provider(request, DOCS_MANIFEST_PATH, source), | |
| ]); | |
| const getUrl = (path: string) => | |
| source?.url | |
| ? `${source.url.replace(/\/$/, '')}/${path.replace(/^\.\//, '')}` | |
| : request | |
| ? getManifestUrlFromRequest(request, path) | |
| : 'Unknown manifest source'; |
🤖 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/core/src/shared/open-service/toolsets/docs/access-provider.ts` around
lines 122 - 135, Update the getUrl helper in fetchManifests to prefer source.url
when a remote source is provided, falling back to the request-derived manifest
URL for local manifests and to the existing unknown-source value when neither is
available. Ensure ManifestGetError instances use this corrected URL for failures
from composed remote sources.
| export function createDocsToolset(options: CreateDocsToolsetOptions) { | ||
| const { docsAccess, sources } = options; | ||
| const multiSource = !!sources?.length; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against an empty sources array.
multiSource is false when sources is [], and the union permits { sources: [] } with no docsAccess. The list handler then calls docsAccess!.list(...) on undefined and throws a TypeError. The MCP registry reaches this state, because custom?.sources ?? [{ id: 'local', title: 'Local' }] keeps an empty array instead of applying the default (see code/addons/mcp/src/tools/tool-registry.ts lines 133-138). Add a cheap runtime assertion at this boundary.
As per coding guidelines: "Use static TypeScript types and existing lint rules to encode invariants whenever practical; otherwise add a cheap runtime assertion near the boundary."
🛡️ Proposed guard
export function createDocsToolset(options: CreateDocsToolsetOptions) {
const { docsAccess, sources } = options;
const multiSource = !!sources?.length;
+
+ if (!multiSource && !docsAccess) {
+ throw new Error('createDocsToolset requires either a docsAccess or at least one source.');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function createDocsToolset(options: CreateDocsToolsetOptions) { | |
| const { docsAccess, sources } = options; | |
| const multiSource = !!sources?.length; | |
| export function createDocsToolset(options: CreateDocsToolsetOptions) { | |
| const { docsAccess, sources } = options; | |
| const multiSource = !!sources?.length; | |
| if (!multiSource && !docsAccess) { | |
| throw new Error('createDocsToolset requires either a docsAccess or at least one source.'); | |
| } |
🤖 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/core/src/shared/open-service/toolsets/docs/definition.ts` around lines
245 - 247, Update createDocsToolset to assert that options.sources is non-empty
whenever sources is provided, rejecting { sources: [] } before handlers can
dereference docsAccess; preserve the existing multiSource behavior for valid
non-empty sources and docsAccess configurations.
Source: Coding guidelines
| ## Multi-Source Requests | ||
|
|
||
| - With multiple sources configured, **${MCP_TOOL_NAMES['docs.list']}** returns entries from every source; pass \`storybookId\` to **${MCP_TOOL_NAMES['docs.show']}** to scope one. | ||
| `; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Mention storybookId for docs.showStory too.
In multi-source mode, showStorySchema also requires storybookId (see definition.ts lines 259-261). The instructions name only docs.show, so an agent can call docs.showStory without the field and get a source-selection error.
📝 Proposed wording
-- With multiple sources configured, **${MCP_TOOL_NAMES['docs.list']}** returns entries from every source; pass \`storybookId\` to **${MCP_TOOL_NAMES['docs.show']}** to scope one.
+- With multiple sources configured, **${MCP_TOOL_NAMES['docs.list']}** returns entries from every source; pass \`storybookId\` to **${MCP_TOOL_NAMES['docs.show']}** and **${MCP_TOOL_NAMES['docs.showStory']}** to scope one.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Multi-Source Requests | |
| - With multiple sources configured, **${MCP_TOOL_NAMES['docs.list']}** returns entries from every source; pass \`storybookId\` to **${MCP_TOOL_NAMES['docs.show']}** to scope one. | |
| `; | |
| ## Multi-Source Requests | |
| - With multiple sources configured, **${MCP_TOOL_NAMES['docs.list']}** returns entries from every source; pass \`storybookId\` to **${MCP_TOOL_NAMES['docs.show']}** and **${MCP_TOOL_NAMES['docs.showStory']}** to scope one. | |
| `; |
🤖 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/core/src/shared/open-service/toolsets/docs/instructions.ts` around lines
21 - 24, Update the “Multi-Source Requests” guidance in the instructions
template to mention that both ${MCP_TOOL_NAMES['docs.show']} and
${MCP_TOOL_NAMES['docs.showStory']} require storybookId when selecting a source,
while preserving the existing docs.list behavior.
1 participant
Closes #
Part 7 of 9 of the Skills M4 migration. Stacked on #35732, replaces #35677.
What I did
The three documentation tools the dev server exposes now run on the shared docs toolset in core, and the MCP addon drops its dependency on the hosted
@storybook/mcppackage together with the parallel copy of the manifest handling it had grown. Composition lands in the same change: a Storybook that composes other Storybooks can serve documentation from all of them, and it does so by composing the accessors introduced in the previous PR rather than by teaching each tool about composition.This is the largest PR in the stack and the seam is genuinely not splittable. Dropping that dependency requires all three tools, their shared types, their instructions and their formatting helpers to move at once.
Two things to expect while reading it. Core gains a published entry point that has to stay dependency-light, which required turning the shared type-declaration build step from one pass into several, so this touches build infrastructure used by every package in the repo and deserves a full compile before merging. And a large share of what looks like new code here is code moving in from the hosted package, whose other half is deleted in the next PR, so rename detection cannot connect the two sides.
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 -c production, not just the two obviously affected ones, since the type-declaration build step is shared. The production flag matters: type declarations are only emitted there, so a plain compile will not exercise this change at all and the guard test covering it silently skips.test-storybooks/mcp, runyarn installand thenyarn storybook. The MCP endpoint comes up athttp://localhost:6006/mcp.list-all-documentation,get-documentationandget-documentation-for-story. The Markdown they return must be identical to what the base branch returns, character for character.yarn storybook:compositioninstead, which serves a Storybook composing others on port 6007. Call the same three tools and confirm documentation from every composed source shows up, grouped by source, and that a source that is unreachable is reported as such instead of silently disappearing.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>