Repository navigation
Skills M4 (1/9): shared toolset contract, with changed-stories migrated end to end - #35727
Closed
valentinpalkovic wants to merge 1 commit into
Closed
valentinpalkovic wants to merge 1 commit into
valentinpalkovic wants to merge 1 commit into
Conversation
Toolset methods now return a single tagged outcome carrying both the structured data and the rendered Markdown, instead of branching on a `format` flag and returning one or the other. Every toolset also declares the telemetry group it reports under, so the grouping cannot drift between surfaces. A new adapter in addon-mcp turns a toolset method into an MCP tool: name, title, description, schemas, behaviour and telemetry all come from the method, while the addon contributes only the MCP grouping and the availability gate. The shipped get-changed-stories tool is the first to run on it and its hand-written implementation is deleted. If a row's backing toolset is ever missing, that one tool is dropped with an error log instead of taking the whole server down. Also drops the @storybook/mcp and @storybook/addon-mcp proxy entries from the Verdaccio config, which have been failing the Linux build since those packages were first published.
valentinpalkovic
added
maintenance
Contributor
|
1 of 9 tasks
Contributor
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 21.42 MB | 21.52 MB | 🚨 +95 KB 🚨 |
| Dependency size | 31.21 MB | 31.21 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 205 | 205 | 0 |
| Self size | 829 KB | 829 KB | 0 B |
| Dependency size | 86.10 MB | 86.20 MB | 🚨 +95 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 198 | 0 |
| Self size | 32 KB | 32 KB | 🚨 +36 B 🚨 |
| Dependency size | 84.58 MB | 84.68 MB | 🚨 +95 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 74 | 74 | 0 |
| Self size | 1.09 MB | 1.09 MB | 🎉 -66 B 🎉 |
| Dependency size | 52.63 MB | 52.73 MB | 🚨 +95 KB 🚨 |
| Bundle Size Analyzer | node | node |
Contributor
WalkthroughThe PR migrates MCP tools to shared Open Service toolsets. It adds typed outcomes, registry APIs, core stories registration, MCP adapters, UI-root resolution, review gating, resilient missing-toolset handling, and updated unit and end-to-end tests. ChangesShared toolset contracts and registry
MCP integration
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: 1
🧹 Nitpick comments (5)
code/core/src/server-errors.ts (1)
381-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider identifying the failing toolset/method in the mismatch error.
OpenServiceToolsetOutputMismatchErrorstores onlyissues. When this error surfaces in logs, nothing on the error identifies which toolset or method produced the invalid output, so diagnosing the bug requires cross-referencing theissuespayload against every method with anoutputSchema. Add atoolsetIdand/ormethodIdfield so the error is self-describing.♻️ Proposed addition of identifying context
export class OpenServiceToolsetOutputMismatchError extends StorybookError { - constructor(public data: { issues: readonly unknown[] }) { + constructor(public data: { toolsetId: string; methodId: string; issues: readonly unknown[] }) { super({ name: 'OpenServiceToolsetOutputMismatchError', category: Category.CORE_COMMON, code: 26, - message: `Toolset output did not match its published output schema: ${JSON.stringify(data.issues)}`, + message: `Toolset "${data.toolsetId}" method "${data.methodId}" output did not match its published output schema: ${JSON.stringify(data.issues)}`, }); } }🤖 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/server-errors.ts` around lines 381 - 397, Update OpenServiceToolsetOutputMismatchError to accept and store identifying context for the failing toolset and/or method, such as toolsetId and methodId, and include that context in the constructed error message so surfaced errors are self-describing. Preserve the existing issues payload and validation-mismatch behavior.test-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the explicit
.tsextension to the relative import.♻️ Proposed change
-import { mcpRequest, waitForMcpEndpoint, killPort, startStorybook, stopStorybook } from './helpers'; +import { mcpRequest, waitForMcpEndpoint, killPort, startStorybook, stopStorybook } from './helpers.ts';As per coding guidelines: "For relative TypeScript imports and exports targeting repository TS/JS modules, prefer explicit extensions such as
./foo.tsor./bar.tsx."🤖 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 `@test-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts` at line 3, Update the relative import from `./helpers` in the test file to include the explicit `.ts` extension, while preserving the existing imported symbols.Source: Coding guidelines
code/addons/mcp/src/tools/toolset-tools.ts (2)
45-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a boundary assertion for an unresolved method.
resolveMethodreturnstoolset.methods[methodName]without checking that the key exists. IfToolsetMethodRefand the registered toolset'smethodsmap ever drift (a typo, a renamed method, a partially-registered stub toolset in tests), this returnsundefinedtyped as a validToolsetMethod. The failure then surfaces later as an opaqueTypeErrorinsidemethod.handler(...), instead of a clear error at the resolution boundary.Add a runtime check next to the existing
getToolsetboundary check, so a missing method fails as loudly and specifically as a missing toolset.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 boundary assertion
function resolveMethod( toolset: AnyToolsetDefinition, options: ToolsetToolOptions ): ToolsetMethod<any, AnyToolsetOutcome> { const [, methodName] = options.method.split('.'); - return toolset.methods[methodName]; + const method = toolset.methods[methodName]; + if (!method) { + throw new Error( + `Toolset "${toolset.id}" has no method "${methodName}" (requested via "${options.method}").` + ); + } + return method; }🤖 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/toolset-tools.ts` around lines 45 - 51, Update resolveMethod to validate that toolset.methods[methodName] exists before returning it, using the same runtime assertion style as the existing getToolset boundary check. Preserve the current method-name extraction and return the resolved ToolsetMethod when present; otherwise fail immediately with a clear missing-method error.Source: Coding guidelines
138-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid relying on
.entriesfor zero-input detection.
ToolsetMethod.schemaaccepts anyStandardSchemaV1. Valibot object schemas expose.entries, but piped schemas and other Standard Schema implementations do not. Store explicit input metadata in the method definition, or restrict and enforce object schemas in the core contract.🤖 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/toolset-tools.ts` around lines 138 - 156, Update getToolsetToolMetadata and the ToolsetMethod definition to determine zero-input methods through explicit input metadata rather than inspecting method.schema.entries. Ensure every method definition supplies that metadata, including piped and non-Valibot StandardSchemaV1 schemas, and preserve omission of schema only for methods explicitly marked as having no inputs.code/addons/mcp/src/tools/toolset-tools.test.ts (1)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
vi.mock()calls omit the requiredspy: trueoption. Both test files replace modules withvi.mock()factories but none pass{ spy: true }, and the coding guidelines require it for every package and file mock in Vitest tests. Notespy: trueis documented to preserve the original export behavior while automocking, which conflicts with these tests' intent to fully replacecollectTelemetry,getService, andlogger— confirm the intended pattern per.cursor/rules/spy-mocking.mdcbefore applying a mechanical fix.
code/addons/mcp/src/tools/toolset-tools.test.ts#L1-L12: addspy: true(or confirm the repo's documented alternative pattern) to thevi.mock('storybook/internal/core-server', ...)andvi.mock('../telemetry.ts', ...)calls.code/addons/mcp/src/tools/tool-registry.test.ts#L14-L20: addspy: true(or confirm the repo's documented alternative pattern) to thevi.mock('../telemetry.ts', ...)andvi.mock('storybook/internal/node-logger', ...)calls.As per coding guidelines, "Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests" and "Avoid mocking without the spy: true option 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/toolset-tools.test.ts` around lines 1 - 12, Update the vi.mock calls in code/addons/mcp/src/tools/toolset-tools.test.ts:1-12 and code/addons/mcp/src/tools/tool-registry.test.ts:14-20 to follow the documented spy-mocking pattern, adding spy: true where appropriate while preserving the tests’ intended replacement behavior for getService, collectTelemetry, and logger; if the repository guidance specifies an alternative for full factory replacements, apply that documented pattern instead.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/core/src/shared/open-service/toolsets/stories/format.ts`:
- Around line 37-46: Update formatPartialCoverageBanner and its caller to
receive the true unreachable-file total separately from the capped file list, so
the banner reports the full count while still previewing only available entries.
Use neutral “changed” wording (or otherwise avoid labeling new files as
modified), preserve the existing +N preview behavior, and update the
corresponding expectation in the definition test.
---
Nitpick comments:
In `@code/addons/mcp/src/tools/toolset-tools.test.ts`:
- Around line 1-12: Update the vi.mock calls in
code/addons/mcp/src/tools/toolset-tools.test.ts:1-12 and
code/addons/mcp/src/tools/tool-registry.test.ts:14-20 to follow the documented
spy-mocking pattern, adding spy: true where appropriate while preserving the
tests’ intended replacement behavior for getService, collectTelemetry, and
logger; if the repository guidance specifies an alternative for full factory
replacements, apply that documented pattern instead.
In `@code/addons/mcp/src/tools/toolset-tools.ts`:
- Around line 45-51: Update resolveMethod to validate that
toolset.methods[methodName] exists before returning it, using the same runtime
assertion style as the existing getToolset boundary check. Preserve the current
method-name extraction and return the resolved ToolsetMethod when present;
otherwise fail immediately with a clear missing-method error.
- Around line 138-156: Update getToolsetToolMetadata and the ToolsetMethod
definition to determine zero-input methods through explicit input metadata
rather than inspecting method.schema.entries. Ensure every method definition
supplies that metadata, including piped and non-Valibot StandardSchemaV1
schemas, and preserve omission of schema only for methods explicitly marked as
having no inputs.
In `@code/core/src/server-errors.ts`:
- Around line 381-397: Update OpenServiceToolsetOutputMismatchError to accept
and store identifying context for the failing toolset and/or method, such as
toolsetId and methodId, and include that context in the constructed error
message so surfaced errors are self-describing. Preserve the existing issues
payload and validation-mismatch behavior.
In `@test-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts`:
- Line 3: Update the relative import from `./helpers` in the test file to
include the explicit `.ts` extension, while preserving the existing imported
symbols.
🪄 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: e9842f99-8ca3-4d60-92d6-d7143afcb4ec
📒 Files selected for processing (41)
code/addons/mcp/scripts/generate-tools-api-doc.tscode/addons/mcp/src/mcp-handler.test.tscode/addons/mcp/src/preset.test.tscode/addons/mcp/src/storybook-ai-metadata.test.tscode/addons/mcp/src/test-support/register-core-toolsets.tscode/addons/mcp/src/tools/get-changed-stories.test.tscode/addons/mcp/src/tools/get-changed-stories.tscode/addons/mcp/src/tools/tool-names.tscode/addons/mcp/src/tools/tool-registry.test.tscode/addons/mcp/src/tools/tool-registry.tscode/addons/mcp/src/tools/toolset-tools.test.tscode/addons/mcp/src/tools/toolset-tools.tscode/addons/mcp/src/tools/ui-root.tscode/addons/mcp/src/utils/detect-unreachable-changes.test.tscode/addons/mcp/src/utils/detect-unreachable-changes.tscode/core/src/core-server/index.tscode/core/src/core-server/presets/common-preset.tscode/core/src/server-errors.tscode/core/src/shared/open-service/index.tscode/core/src/shared/open-service/toolset-definition.test-d.tscode/core/src/shared/open-service/toolset-definition.tscode/core/src/shared/open-service/toolset-names.tscode/core/src/shared/open-service/toolset-registry.test.tscode/core/src/shared/open-service/toolset-registry.tscode/core/src/shared/open-service/toolset-types.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/review/definition.test.tscode/core/src/shared/open-service/toolsets/review/definition.tscode/core/src/shared/open-service/toolsets/stories/definition.test.tscode/core/src/shared/open-service/toolsets/stories/definition.tscode/core/src/shared/open-service/toolsets/stories/format.tscode/core/src/shared/open-service/toolsets/stories/unreachable-files.tscode/core/src/shared/open-service/toolsets/test/definition.test.tscode/core/src/shared/open-service/toolsets/test/definition.tscode/core/src/shared/review/features.test.tscode/core/src/shared/review/features.tscode/core/src/storybook-error.tsscripts/verdaccio.yamltest-storybooks/mcp/tests/helpers.tstest-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts
💤 Files with no reviewable changes (5)
- scripts/verdaccio.yaml
- code/addons/mcp/src/utils/detect-unreachable-changes.ts
- code/addons/mcp/src/utils/detect-unreachable-changes.test.ts
- code/addons/mcp/src/tools/get-changed-stories.test.ts
- code/addons/mcp/src/tools/get-changed-stories.ts
Comment on lines
+37
to
+46
| function formatPartialCoverageBanner(unreachable: string[]): string { | ||
| if (unreachable.length === 0) { | ||
| return ''; | ||
| } | ||
| const fileList = | ||
| unreachable.length <= BANNER_INLINE_LIMIT | ||
| ? unreachable.join(', ') | ||
| : `${unreachable.slice(0, BANNER_INLINE_LIMIT).join(', ')}, +${unreachable.length - BANNER_INLINE_LIMIT} more`; | ||
| return `⚠ Coverage gap: ${unreachable.length} modified ${pluralize(unreachable.length, 'file')} unreachable from any story (${fileList}) — full sanity-check note at end of this response.\n\n`; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The banner count can understate the coverage gap, and the label can be wrong for new files.
detectUnreachableFiles caps its result at DEFAULT_MAX_FILES (10) in unreachable-files.ts, and it includes entries from changedFiles.new. As a result the banner reports at most 10 files as the whole gap, and it calls untracked new files "modified". Pass the true total to the formatter, or use a neutral wording such as "changed" plus a "+N more" total marker.
♻️ Proposed change
-function formatPartialCoverageBanner(unreachable: string[]): string {
+function formatPartialCoverageBanner(unreachable: string[], total = unreachable.length): string {
if (unreachable.length === 0) {
return '';
}
const fileList =
unreachable.length <= BANNER_INLINE_LIMIT
? unreachable.join(', ')
: `${unreachable.slice(0, BANNER_INLINE_LIMIT).join(', ')}, +${unreachable.length - BANNER_INLINE_LIMIT} more`;
- return `⚠ Coverage gap: ${unreachable.length} modified ${pluralize(unreachable.length, 'file')} unreachable from any story (${fileList}) — full sanity-check note at end of this response.\n\n`;
+ return `⚠ Coverage gap: ${total}${total > unreachable.length ? '+' : ''} changed ${pluralize(total, 'file')} unreachable from any story (${fileList}) — full sanity-check note at end of this response.\n\n`;
}This change also requires updating the banner expectation in code/core/src/shared/open-service/toolsets/stories/definition.test.ts.
🤖 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/stories/format.ts` around lines 37
- 46, Update formatPartialCoverageBanner and its caller to receive the true
unreachable-file total separately from the capped file list, so the banner
reports the full count while still previewing only available entries. Use
neutral “changed” wording (or otherwise avoid labeling new files as modified),
preserve the existing +N preview behavior, and update the corresponding
expectation in the definition test.
This was referenced Aug 3, 2026
This was referenced Aug 5, 2026
1 of 9 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 1 of 9 of the Skills M4 migration. Targets
next, replaces #35677.What I did
Storybook exposes agent tools through two different MCP servers today: the one the dev server serves via the MCP addon, and the hosted
@storybook/mcppackage. Each of them carries its own hand-written implementation of those tools, which is why the same tool can drift in two places. This stack replaces both with one shared set of toolsets owned by Storybook's core, and this first PR lands the contract those toolsets are written against, together with a single tool migrated all the way through, so the whole path is reviewable on something real rather than on an abstraction.Two things about the contract are worth understanding before reading the rest of the stack. A tool handler now produces its structured data, its human-readable text and its telemetry in a single run, instead of being invoked once per output format with telemetry reported somewhere else entirely. And failure is deliberately split in two: a method that cannot do its job at all throws, while a method that did its job and simply has bad news returns a failure outcome that the agent is meant to read.
The migrated tool is the changed-stories tool, which reports which stories a working-tree diff affects. It is now served from core and the hand-written copy is deleted. The other three toolsets are converted in the same commit, but mechanically: removing the per-call output-format flag is source-breaking for every toolset definition at once, so they all have to move together. That mechanical conversion is the bulk of the diff, and a compatibility shim would only have to be built and then torn down again.
One unrelated fix rides along, because the stack is red in CI without it: two stale package proxy entries in the local test registry break the Linux build on every branch since those packages were first published, and they are removed here.
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.test-storybooks/mcp, runyarn installand thenyarn storybook. The MCP endpoint comes up athttp://localhost:6006/mcp.tools/listJSON-RPC request to it) and confirm the tool list still containsget-changed-storieswith an unchanged name, title, description and input schema.get-changed-stories. The result should read exactly as it did before this PR, including the coverage-gap banner and the New/Modified/Related grouping.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>