Repository navigation
Skills M2b: Rework the core toolsets into their intended shape - #35726
Merged
Merged
Conversation
kasperpeulen
added
maintenance
5 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.49 MB | 21.78 MB | 🚨 +292 KB 🚨 |
| Dependency size | 30.98 MB | 30.98 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 205 | 205 | 0 |
| Self size | 833 KB | 833 KB | 0 B |
| Dependency size | 85.97 MB | 86.27 MB | 🚨 +292 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 198 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 84.45 MB | 84.74 MB | 🚨 +292 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.48 MB | 52.77 MB | 🚨 +292 KB 🚨 |
| Bundle Size Analyzer | node | node |
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR updates Open Service toolset contracts, registry behavior, docs and story access, test and review execution, server and Vitest wiring, public exports, and portable declaration generation. ChangesOpen Service Toolsets and Integrations
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: 9
🧹 Nitpick comments (18)
code/core/src/shared/open-service/toolsets/stories/definition.ts (1)
324-335: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
matchedComponentCountcan exceed the number of components that matched.
resolveComponentStoriesdedupescomponentPaths, solookup.resultscan be shorter thaninput.componentPaths.unmatchedCountcounts deduped results, but the subtraction uses the raw input length. If a caller sends the same path twice and it has no stories, telemetry reportscomponentCount: 2andmatchedComponentCount: 1, although no component matched. Derive both counts fromlookup.results.📊 Proposed fix
- const unmatchedCount = lookup.results.filter( - (result) => !result.pathNotFound && result.matches.length === 0 - ).length; + const matchedComponentCount = lookup.results.filter( + (result) => result.matches.length > 0 + ).length; await ctx.telemetry?.('tool:getStoriesByComponent', { componentCount: input.componentPaths.length, - matchedComponentCount: input.componentPaths.length - unmatchedCount, + matchedComponentCount,🤖 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/definition.ts` around lines 324 - 335, Update the telemetry calculation in the resolveComponentStories flow to derive matchedComponentCount from the deduplicated lookup.results rather than input.componentPaths.length: use lookup.results.length minus unmatchedCount, while preserving componentCount as the raw input count.code/core/src/shared/open-service/toolsets/stories/unreachable-files.ts (1)
56-78: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA module-graph query failure fails the whole
stories.changedcall.Git failures degrade to an empty list (Lines 45-51), and a non-ready graph degrades as well (Lines 37-39). A rejected
storiesForFilesquery propagates instead.stories.changedawaits this helper inline (definition.tsLine 257), so the primary answer — the changed-story list — is lost because of a secondary coverage hint. Degrade the same way for consistency.🛡️ Proposed fix
const unreachable: string[] = []; + try { for ( let start = 0; start < relativeFiles.length && unreachable.length < maxFiles; start += CHUNK_SIZE ) { const chunk = relativeFiles.slice(start, start + CHUNK_SIZE); // One batched lookup per chunk; the result is positional. const hits = await moduleGraph.queries.storiesForFiles.loaded({ files: chunk.map((file) => resolvePath(repoRoot, file)), }); for (const [position, file] of chunk.entries()) { if (unreachable.length >= maxFiles) { break; } if ((hits[position]?.length ?? 0) === 0) { unreachable.push(file); } } } + } catch (error) { + logger.debug(`Unreachable-file detection skipped, module-graph query failed: ${error}`); + return []; + } return unreachable;🤖 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/unreachable-files.ts` around lines 56 - 78, Handle rejected storiesForFiles queries in the unreachable-file lookup by degrading to an empty unreachable list, matching the existing Git-failure and non-ready-graph behavior. Update the query flow around storiesForFiles.loaded so failures do not propagate into the stories.changed call, while preserving successful positional processing and maxFiles handling.code/core/src/shared/open-service/toolsets/stories/find-by-component.ts (1)
80-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese fallbacks cannot trigger.
resolveComponentStoriesalways returnsreasonwhenavailableisfalse, and always returnsresultswhenavailableistrue. The?? …fallbacks exist only becauseComponentStoriesResponsedeclares both fields optional. See the suggested union type inresolve-component-stories.tsLines 160-172; after that change these fallbacks can be removed.🤖 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/find-by-component.ts` around lines 80 - 87, Remove the unreachable nullish fallbacks in the `resolveComponentStories` result handling: return `lookup.reason` directly when `available` is false, and map `lookup.results` directly in the successful path. Rely on the discriminated `ComponentStoriesResponse` union defined by `resolve-component-stories.ts` to provide the required fields for each branch.code/core/src/shared/open-service/toolsets/stories/resolve-component-stories.ts (2)
258-267: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBatch size is unbounded here, while a sibling caller chunks at 50.
allTargetscan hold up to about seven paths per component path, andcomponentPathshas no upper length bound in thefindByComponentschema.unreachable-files.tssends the same query in chunks ofCHUNK_SIZE = 50. Confirm thatstoriesForFilesaccepts an arbitrarily longfilesarray over the channel, or chunk here for consistency.🤖 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/resolve-component-stories.ts` around lines 258 - 267, Bound the `storiesForFiles.loaded` request in the Phase 2 lookup around `allTargets` and `hitsByTarget`, using the established 50-item chunking pattern from the sibling caller unless the channel contract explicitly guarantees unbounded arrays. Preserve positional target-to-result mapping by processing each chunk in order and associating each returned hit list with its corresponding target.
160-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ComponentStoriesResponsedoes not encode the availability invariant.available: booleanwith optionalreasonandresultslets the type permit states the resolver never produces, which forces unreachable fallbacks in the consumer.
code/core/src/shared/open-service/toolsets/stories/resolve-component-stories.ts#L160-L172: replace the interface with a discriminated union:{ available: false; reason: string } | { available: true; results: ComponentStoriesResult[] }.code/core/src/shared/open-service/toolsets/stories/find-by-component.ts#L80-L87: after the union lands, drop the?? "Storybook's story module graph is unavailable."and?? []fallbacks.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/stories/resolve-component-stories.ts` around lines 160 - 172, Replace ComponentStoriesResponse in resolve-component-stories.ts with a discriminated union requiring reason when available is false and results when available is true. In find-by-component.ts, remove the nullish fallbacks for reason and results and consume the required union fields directly.Source: Coding guidelines
code/core/src/shared/open-service/toolsets/stories/format.ts (1)
228-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCLI output repeats the component path.
Line 231 pushes
## ${result.componentPath}, andserializeComponentSectionstarts its own block with${componentPath}:(Line 204). The CLI rendering therefore shows the path twice in sequence, asdefinition.test.tsLines 420-427 assert. Consider dropping the heading, or letting the serializer omit the leading path line when a heading precedes it.🤖 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 228 - 233, Update the non-MCP CLI formatting branch around serializeComponentSection so each component path appears only once. Remove the redundant `## ${result.componentPath}` heading or adjust the serializer call to omit its leading `${componentPath}:` line, while preserving the existing component sections and output structure.code/core/src/shared/open-service/toolset-definition.test-d.ts (1)
98-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the description type assertion.
not.toBeNever()passes for nearly every type, so this test does not check the resolved description type. Assert the exact type instead.♻️ Proposed change
it('resolves description functions against the toolset context', () => { - const description: string | ((context: ToolsetCtx) => string) = - reviewToolset.methods.create.description; - - expectTypeOf(description).not.toBeNever(); + expectTypeOf(reviewToolset.methods.create.description).toEqualTypeOf< + (ctx: ToolsetCtx) => string + >(); });🤖 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/toolset-definition.test-d.ts` around lines 98 - 103, Update the description type assertion in the resolves description functions test to verify the exact expected type, rather than only asserting that it is not never. Use expectTypeOf’s equality assertion against the intended resolved description type while preserving the existing ToolsetCtx-based description setup.scripts/build/utils/generate-types-rolldown.ts (1)
414-422: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winShared and portable declaration passes build sequentially instead of fully in parallel.
The first array element awaits
bundleDts(sharedInput, externalFn)to completion before thePromise.alloverportableInputsis even constructed. This serializes the shared pass ahead of all portable passes, even though the portable passes already run concurrently with each other. Combine both into a singlePromise.allto reduce total declaration-build time.♻️ Proposed refactor to parallelize all bundling passes
- const outputs = [ - ...(Object.keys(sharedInput).length > 0 ? [await bundleDts(sharedInput, externalFn)] : []), - // One isolated pass per portable entry, so nothing is shared between it and the rest. - ...(await Promise.all( - portableInputs.map(({ input: portableInput, allowedExternal }) => - bundleDts(portableInput, portableExternalFn(allowedExternal)) - ) - )), - ]; + const outputs = await Promise.all([ + ...(Object.keys(sharedInput).length > 0 ? [bundleDts(sharedInput, externalFn)] : []), + // One isolated pass per portable entry, so nothing is shared between it and the rest. + ...portableInputs.map(({ input: portableInput, allowedExternal }) => + bundleDts(portableInput, portableExternalFn(allowedExternal)) + ), + ]);🤖 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 `@scripts/build/utils/generate-types-rolldown.ts` around lines 414 - 422, Update the outputs construction around bundleDts so the shared pass and every portable pass are created as promises and awaited together in one Promise.all. Preserve the conditional omission of the shared pass when sharedInput is empty and keep each portable input paired with portableExternalFn(allowedExternal).code/core/src/shared/open-service/toolsets/docs/access-parity.test.ts (2)
147-177: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider comparing the whole outcome, not only
markdown.The helpers return
.markdownonly. The unifiedToolsetOutcomealso carriesokanddata. A mode that returns the same text with a differentokflag would still pass. Compareokas well to pin the full contract for both modes.♻️ Proposed refactor
async function renderShow(toolset: ReturnType<typeof createDocsToolset>, id: string) { - return (await toolset.methods.show.handler({ id }, ctx)).markdown; + const { ok, markdown } = await toolset.methods.show.handler({ id }, ctx); + return { ok, markdown }; }🤖 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-parity.test.ts` around lines 147 - 177, Update renderList, renderShow, and the showStory test helper to return the complete ToolsetOutcome from each handler instead of only .markdown, then compare the full outcomes between serviceToolset() and manifestToolset(), including ok and data.
85-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim and align the
core/story-docsfixture.Two points:
storyDocsForAllComponentsis never called.access-service.tsuses only the per-idstoryDocsquery. Remove the unused entry so the fixture states the real contract.storyDocs.loadedreturnsundefinedfor unknown ids. The real query rejects withOpenServiceDocgenMissingComponentError, asaccess-service.test.tsdocuments. The current assertions do not depend on this, but the stub can hide a future divergence.🤖 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-parity.test.ts` around lines 85 - 93, Update the core/story-docs fixture to remove the unused storyDocsForAllComponents query and make storyDocs.loaded reject with OpenServiceDocgenMissingComponentError for unknown component ids, while preserving the existing storyDocsPayload result for the button id.code/core/src/shared/open-service/toolsets/docs/access-local.test.ts (1)
14-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the registry mock with the repository mocking rules.
Two deviations exist:
vi.mock('../../service-registry.ts', ...)uses a full factory without thespy: trueoption.- The throwing
getServiceimplementation lives in the hoisted factory instead ofbeforeEach.Use
vi.mock('../../service-registry.ts', { spy: true }), access the functions throughvi.mocked(...), and set both implementations inbeforeEach.♻️ Proposed refactor
-const getRegisteredServices = vi.hoisted(() => vi.fn<() => Array<{ id: string }>>(() => [])); -const getService = vi.hoisted(() => - vi.fn(() => { - throw new Error('read from the docgen services'); - }) -); -vi.mock('../../service-registry.ts', () => ({ getRegisteredServices, getService })); +vi.mock('../../service-registry.ts', { spy: true }); + +import { getRegisteredServices, getService } from '../../service-registry.ts'; @@ beforeEach(() => { - getRegisteredServices.mockReturnValue([]); + vi.mocked(getRegisteredServices).mockReturnValue([]); + vi.mocked(getService).mockImplementation(() => { + throw new Error('read from the docgen services'); + }); });As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests", "Usevi.mocked()to type and access the mocked functions", and "Implement mock behaviors inbeforeEachblocks".🤖 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-local.test.ts` around lines 14 - 40, Update the service-registry mock to use vi.mock('../../service-registry.ts', { spy: true }) instead of a factory, and obtain getRegisteredServices and getService through vi.mocked(...). In beforeEach, reset both mocked functions’ implementations: return an empty array from getRegisteredServices and throw the existing docgen-services error from getService.Source: Coding guidelines
code/core/src/shared/open-service/toolsets/docs/access-local.ts (1)
35-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the
getServicecast.ServiceIdis an alias ofstring, and both signatures use the sameGetServiceOptions. PassinggetServicedirectly preserves compile-time contract checks.🤖 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-local.ts` around lines 35 - 47, In the service access setup, remove the type cast from the getService property passed to createServiceDocsAccess and pass getService directly. Preserve the existing GetServiceOptions contract and leave the manifest fallback logic unchanged.Source: Coding guidelines
code/core/src/shared/open-service/toolsets/docs/access-service.ts (1)
146-161: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound per-ID story-doc loads.
When
withStoryIdsis true, usemapWithConcurrencyforstoryDocs.loaded({ id })calls. Each call can start extraction, while the currentPromise.allstarts all component loads at once.🤖 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-service.ts` around lines 146 - 161, Update the story-based loading flow around storyBasedIds and storiesById to use mapWithConcurrency instead of Promise.all, limiting concurrent loadOptionalComponentPayload calls for storyDocs.queries.storyDocs.loaded({ id }). Preserve the existing id-to-payload Map result and the no-load behavior when withStoryIds is false.code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.ts (1)
395-418: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared component and docs section rendering.
Lines 395-418 repeat the loop in
formatManifestsToLists(lines 342-360). The two copies already differ in heading level and in empty-section handling. A shared helper that takes the components, the docs, the options, and the heading prefix keeps future formatting changes in one place.♻️ Sketch of the extracted helper
function pushListingSections( parts: string[], manifests: AllManifests | undefined, options: ListFormattingOptions, heading: '#' | '##' ): void { const components = Object.values(manifests?.componentManifest.components ?? {}); if (components.length > 0) { parts.push(`${heading} Components`, ''); for (const component of components) { parts.push(formatComponentLine(component)); if (options.withStoryIds && Array.isArray(component.stories)) { for (const story of component.stories) { parts.push(formatStorySubLine(story)); } } } parts.push(''); } const docs = Object.values(manifests?.docsManifest?.docs ?? {}); if (docs.length > 0) { parts.push(`${heading} Docs`, ''); for (const doc of docs) { parts.push(formatDocLine(doc)); } parts.push(''); } }🤖 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.ts` around lines 395 - 418, Extract the duplicated component and docs rendering from formatManifestsToLists and the shown listing flow into a shared pushListingSections helper. Have it accept parts, manifests, options, and a '#' or '##' heading prefix, preserve component story rendering and empty-section behavior, and replace both callers with the helper using their existing heading levels.code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.ts (1)
1490-1513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the error and listing branches of
formatMultiSourceManifestsToLists.The new suite covers only the
requires-own-mcpnotice. The formatter has two other branches: theerror:line and the per-source## Components/## Docslisting withwithStoryIds. Add one case that mixes a failed source with a successful one. That case pins both the error prefix and the heading level used inside a source section.🤖 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, Extend the `MarkdownFormatter - formatMultiSourceManifestsToLists` test suite with a mixed failed-and-successful source case. Exercise the formatter’s error notice branch and the successful source listing branch using `withStoryIds`, asserting the failed source includes the `error:` prefix and the successful source uses `## Components` and `## Docs` headings.code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts (1)
185-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the try/catch assertions fail when no error is thrown.
Both blocks assert only inside
catch. IfgetManifestsresolves, the test passes without running any assertion. Addexpect.hasAssertions()or userejectsmatchers so a missing rejection fails the test.♻️ Proposed change
+ expect.hasAssertions(); try { await getManifests(request); } catch (error) { expect((error as Error).message).not.toContain('componentsManifest'); }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 both try/catch test blocks around getManifests to require an assertion even when no error is thrown. Add expect.hasAssertions() at the start of each test or replace the try/catch with an appropriate rejects matcher, while preserving the existing error-message checks.code/core/src/shared/open-service/toolsets/docs/runtime-agnostic.test.ts (1)
1-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why this test reads the real filesystem.
The repository guideline requires
memfsfor tests that touchnode:fs. This test must read the real source tree, because the guarded property is the actual import graph. Add a short comment at the top that states this exception, asportable-dist.test.tsdoes on Lines 10-11. That keeps the deviation intentional and reviewable.As per coding guidelines: "Filesystem tests touching
node:fsornode:fs/promisesmust usememfs".🤖 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/runtime-agnostic.test.ts` around lines 1 - 26, Add a brief explanatory comment at the top of the runtime-agnostic test, near the node:fs imports, documenting that real filesystem access is intentional because the test validates the actual source import graph and therefore is exempt from the memfs guideline. Follow the precedent established by portable-dist.test.ts without changing the test behavior.Source: Coding guidelines
code/core/src/shared/open-service/toolsets/docs/sources.test.ts (1)
5-11: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for a sub-path URL without a trailing slash.
Add
['https://example.com/storybook', 'https://example.com/storybook/mcp']to confirm path normalization preserves thestorybooksegment.🤖 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 the missing sub-path case to the getSourceMcpEndpoint parameterized test: include https://example.com/storybook and expect https://example.com/storybook/mcp, preserving the existing cases and test structure.
🤖 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/core-server/utils/manifests/manifests.ts`:
- Around line 138-146: Update loadManifests to cache the extracted manifest data
across docs-tool calls rather than invoking getManifestEntries and reprocessing
manifests each time. Store the cache at session scope, reuse it while the story
index and source files remain unchanged, and invalidate it when either changes;
preserve the existing watch behavior and getManifests flow.
In `@code/core/src/server-errors.ts`:
- Around line 415-423: Update OpenServiceToolsetOutputMismatchError to safely
serialize data.issues without allowing circular references to throw from the
constructor, and bound the serialized message size to avoid embedding
excessively large rejected inputs; preserve the schema-mismatch error when
serialization fails.
In `@code/core/src/shared/open-service/README.md`:
- Around line 104-113: Update the defineToolset method field documentation to
include the required title field and change the count from four fields to five,
while preserving the existing descriptions of description, schema, outputSchema,
and handler.
In `@code/core/src/shared/open-service/toolsets/docs/access-provider.ts`:
- Around line 70-97: Update defaultManifestProvider to call fetch with a bounded
AbortSignal.timeout value, and catch timeout aborts so they are converted into
ManifestGetError using manifestUrl context. Preserve the existing response
validation and error handling for non-timeout fetch failures.
In
`@code/core/src/shared/open-service/toolsets/docs/manifest-formatter/manifest-types.ts`:
- Around line 68-92: Preserve top-level props throughout the v0 manifest
pipeline: add the field to ComponentManifestV0/BaseInlineComponentProperties
using the existing DocgenPayload.props shape, update the manifest adapter to map
it, and update markdown.ts to render it alongside the react* metadata. Add a
parity test covering both remote and in-process manifest paths to verify
identical props preservation and rendering.
- Around line 94-104: Update the manifest parsing/normalization used by
fetchManifests and createManifestDocsAccess to recognize legacy pre-v0
components.json files serialized as a direct component-ID map, converting them
into the v0 ComponentManifestMapV0 shape with v: 0 and components. Keep existing
wrapped-version handling intact, and add a composition test using a
representative legacy components manifest.
In `@code/core/src/shared/open-service/toolsets/test/definition.ts`:
- Around line 120-128: Clamp the effective a11y flag in the run handler using
a11yEnabled, so accessibility execution, formatting, and telemetry receive false
whenever the addon is disabled, even when runInputSchema.a11y defaults to true.
Update the related schema description to avoid promising accessibility testing
when unavailable, including the corresponding duplicate definition.
In `@code/core/src/shared/open-service/toolsets/test/format.ts`:
- Around line 56-76: Update getA11yViolations to defensively validate each
violation before accessing its fields: ensure violation is an object, verify
nodes is an array, and use an empty node list when it is absent or invalid so
malformed accessibility payloads never throw during mapping. Preserve the
existing normalized violation and node field handling for valid data.
In `@scripts/build/utils/generate-types-rolldown.ts`:
- Around line 411-412: Update portableExternalFn so its allowed-dependency
predicate also recognizes resolved absolute paths containing node_modules/<dep>,
matching the behavior already handled by externalFn. Preserve the existing
exact-package and subpath checks while adding the node_modules path form for
each allowed dependency.
---
Nitpick comments:
In `@code/core/src/shared/open-service/toolset-definition.test-d.ts`:
- Around line 98-103: Update the description type assertion in the resolves
description functions test to verify the exact expected type, rather than only
asserting that it is not never. Use expectTypeOf’s equality assertion against
the intended resolved description type while preserving the existing
ToolsetCtx-based description setup.
In `@code/core/src/shared/open-service/toolsets/docs/access-local.test.ts`:
- Around line 14-40: Update the service-registry mock to use
vi.mock('../../service-registry.ts', { spy: true }) instead of a factory, and
obtain getRegisteredServices and getService through vi.mocked(...). In
beforeEach, reset both mocked functions’ implementations: return an empty array
from getRegisteredServices and throw the existing docgen-services error from
getService.
In `@code/core/src/shared/open-service/toolsets/docs/access-local.ts`:
- Around line 35-47: In the service access setup, remove the type cast from the
getService property passed to createServiceDocsAccess and pass getService
directly. Preserve the existing GetServiceOptions contract and leave the
manifest fallback logic unchanged.
In `@code/core/src/shared/open-service/toolsets/docs/access-parity.test.ts`:
- Around line 147-177: Update renderList, renderShow, and the showStory test
helper to return the complete ToolsetOutcome from each handler instead of only
.markdown, then compare the full outcomes between serviceToolset() and
manifestToolset(), including ok and data.
- Around line 85-93: Update the core/story-docs fixture to remove the unused
storyDocsForAllComponents query and make storyDocs.loaded reject with
OpenServiceDocgenMissingComponentError for unknown component ids, while
preserving the existing storyDocsPayload result for the button id.
In `@code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts`:
- Around line 185-190: Update both try/catch test blocks around getManifests to
require an assertion even when no error is thrown. Add expect.hasAssertions() at
the start of each test or replace the try/catch with an appropriate rejects
matcher, while preserving the existing error-message checks.
In `@code/core/src/shared/open-service/toolsets/docs/access-service.ts`:
- Around line 146-161: Update the story-based loading flow around storyBasedIds
and storiesById to use mapWithConcurrency instead of Promise.all, limiting
concurrent loadOptionalComponentPayload calls for
storyDocs.queries.storyDocs.loaded({ id }). Preserve the existing id-to-payload
Map result and the no-load behavior when withStoryIds is false.
In
`@code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.ts`:
- Around line 1490-1513: Extend the `MarkdownFormatter -
formatMultiSourceManifestsToLists` test suite with a mixed failed-and-successful
source case. Exercise the formatter’s error notice branch and the successful
source listing branch using `withStoryIds`, asserting the failed source includes
the `error:` prefix and the successful source uses `## Components` and `## Docs`
headings.
In
`@code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.ts`:
- Around line 395-418: Extract the duplicated component and docs rendering from
formatManifestsToLists and the shown listing flow into a shared
pushListingSections helper. Have it accept parts, manifests, options, and a '#'
or '##' heading prefix, preserve component story rendering and empty-section
behavior, and replace both callers with the helper using their existing heading
levels.
In `@code/core/src/shared/open-service/toolsets/docs/runtime-agnostic.test.ts`:
- Around line 1-26: Add a brief explanatory comment at the top of the
runtime-agnostic test, near the node:fs imports, documenting that real
filesystem access is intentional because the test validates the actual source
import graph and therefore is exempt from the memfs guideline. Follow the
precedent established by portable-dist.test.ts without changing the test
behavior.
In `@code/core/src/shared/open-service/toolsets/docs/sources.test.ts`:
- Around line 5-11: Add the missing sub-path case to the getSourceMcpEndpoint
parameterized test: include https://example.com/storybook and expect
https://example.com/storybook/mcp, preserving the existing cases and test
structure.
In `@code/core/src/shared/open-service/toolsets/stories/definition.ts`:
- Around line 324-335: Update the telemetry calculation in the
resolveComponentStories flow to derive matchedComponentCount from the
deduplicated lookup.results rather than input.componentPaths.length: use
lookup.results.length minus unmatchedCount, while preserving componentCount as
the raw input count.
In `@code/core/src/shared/open-service/toolsets/stories/find-by-component.ts`:
- Around line 80-87: Remove the unreachable nullish fallbacks in the
`resolveComponentStories` result handling: return `lookup.reason` directly when
`available` is false, and map `lookup.results` directly in the successful path.
Rely on the discriminated `ComponentStoriesResponse` union defined by
`resolve-component-stories.ts` to provide the required fields for each branch.
In `@code/core/src/shared/open-service/toolsets/stories/format.ts`:
- Around line 228-233: Update the non-MCP CLI formatting branch around
serializeComponentSection so each component path appears only once. Remove the
redundant `## ${result.componentPath}` heading or adjust the serializer call to
omit its leading `${componentPath}:` line, while preserving the existing
component sections and output structure.
In
`@code/core/src/shared/open-service/toolsets/stories/resolve-component-stories.ts`:
- Around line 258-267: Bound the `storiesForFiles.loaded` request in the Phase 2
lookup around `allTargets` and `hitsByTarget`, using the established 50-item
chunking pattern from the sibling caller unless the channel contract explicitly
guarantees unbounded arrays. Preserve positional target-to-result mapping by
processing each chunk in order and associating each returned hit list with its
corresponding target.
- Around line 160-172: Replace ComponentStoriesResponse in
resolve-component-stories.ts with a discriminated union requiring reason when
available is false and results when available is true. In find-by-component.ts,
remove the nullish fallbacks for reason and results and consume the required
union fields directly.
In `@code/core/src/shared/open-service/toolsets/stories/unreachable-files.ts`:
- Around line 56-78: Handle rejected storiesForFiles queries in the
unreachable-file lookup by degrading to an empty unreachable list, matching the
existing Git-failure and non-ready-graph behavior. Update the query flow around
storiesForFiles.loaded so failures do not propagate into the stories.changed
call, while preserving successful positional processing and maxFiles handling.
In `@scripts/build/utils/generate-types-rolldown.ts`:
- Around line 414-422: Update the outputs construction around bundleDts so the
shared pass and every portable pass are created as promises and awaited together
in one Promise.all. Preserve the conditional omission of the shared pass when
sharedInput is empty and keep each portable input paired with
portableExternalFn(allowedExternal).
🪄 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: d0187f1f-071b-4724-9c15-30ec13c67705
📒 Files selected for processing (74)
code/addons/vitest/src/preset.test.tscode/addons/vitest/src/preset.tscode/core/build-config.tscode/core/package.jsoncode/core/src/core-server/index.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/utils/manifests/manifests.tscode/core/src/server-errors.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/index.tscode/core/src/shared/open-service/services/docgen/types.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/access-local.test.tscode/core/src/shared/open-service/toolsets/docs/access-local.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.test.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.tscode/core/src/shared/open-service/toolsets/docs/access-parity.test.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/access-service.test.tscode/core/src/shared/open-service/toolsets/docs/access-service.tscode/core/src/shared/open-service/toolsets/docs/access.tscode/core/src/shared/open-service/toolsets/docs/classify-services.test.tscode/core/src/shared/open-service/toolsets/docs/classify-services.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/adapt-core-manifest.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/manifest-types.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/manifest-formatter/parse-react-docgen.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/parse-react-docgen.tscode/core/src/shared/open-service/toolsets/docs/map-with-concurrency.tscode/core/src/shared/open-service/toolsets/docs/map.test.tscode/core/src/shared/open-service/toolsets/docs/map.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/runtime-agnostic.test.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.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/find-by-component.test.tscode/core/src/shared/open-service/toolsets/stories/find-by-component.tscode/core/src/shared/open-service/toolsets/stories/format.tscode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.test.tscode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.tscode/core/src/shared/open-service/toolsets/stories/story-input.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/open-service/toolsets/test/format.tscode/core/src/shared/open-service/toolsets/test/run.test.tscode/core/src/shared/open-service/toolsets/test/run.tscode/core/src/shared/review/features.test.tscode/core/src/shared/review/features.tscode/core/src/shared/review/review-state.tscode/core/src/storybook-error.tsscripts/build/utils/entry-utils.tsscripts/build/utils/generate-types-rolldown.ts
💤 Files with no reviewable changes (3)
- code/core/src/shared/open-service/toolsets/docs/map.test.ts
- code/core/src/shared/open-service/toolsets/docs/classify-services.test.ts
- code/core/src/shared/open-service/toolsets/docs/map.ts
1 of 9 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
code/core/src/shared/open-service/toolsets/stories/find-by-component.ts (1)
74-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEncode the resolver outcome as a discriminated union so the
?? []and?? reasonfallbacks become unnecessary.
ComponentStoriesResponseincode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.ts(lines 168-172) declaresavailable: booleanwith optionalreasonandresults. Because of that, this call site must guard both fields even though the resolver always setsreasonwhen unavailable andresultswhen available. Change the resolver type to a union, mirroringFindStoriesByComponentResulthere. Then the compiler narrowslookupand both fallbacks can be removed.♻️ Proposed change in resolve-component-stories.ts
-export interface ComponentStoriesResponse { - available: boolean; - reason?: string; - results?: ComponentStoriesResult[]; -} +export type ComponentStoriesResponse = + | { available: false; reason: string } + | { available: true; results: ComponentStoriesResult[] };Then in this file:
if (!lookup.available) { - return { - available: false, - reason: lookup.reason ?? "Storybook's story module graph is unavailable.", - }; + return { available: false, reason: lookup.reason }; } - const results = (lookup.results ?? []).map((entry) => { + const results = lookup.results.map((entry) => {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/stories/find-by-component.ts` around lines 74 - 87, Change ComponentStoriesResponse in resolve-component-stories.ts to a discriminated union keyed by available, requiring reason when unavailable and results when available. Update resolveComponentStories to return that precise union, then use the narrowed lookup in the current find-by-component flow to remove the ?? reason and ?? [] fallbacks while preserving its existing unavailable and available branches.Source: Coding guidelines
code/core/src/shared/open-service/toolsets/stories/resolve-component-stories.test.ts (1)
210-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the per-case
realpathSync.nativebehavior into a helper set frombeforeEach.Lines 212-216 and 278-283 define mock implementations inside test cases. The guidelines require mock behavior in
beforeEachblocks and forbid inline mock implementations in test cases. Define a mutable override map inbeforeEach, then let each test set the entry it needs.♻️ Sketch
+let realpathOverrides: Map<string, () => string>; + beforeEach(() => { vol.reset(); vol.fromNestedJSON({ [BADGE_ABS]: '' }); + realpathOverrides = new Map(); vi.mocked(existsSync).mockImplementation((filePath) => vol.existsSync(filePath)); - vi.mocked(realpathSync.native).mockImplementation((filePath) => - String(vol.realpathSync(filePath)) - ); + vi.mocked(realpathSync.native).mockImplementation((filePath) => { + const override = realpathOverrides.get(asGraphPath(String(filePath))); + return override ? override() : String(vol.realpathSync(filePath)); + }); });🤖 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/resolve-component-stories.test.ts` around lines 210 - 226, Move the per-test realpath behavior from inline realpathSync.native.mockImplementation calls in the affected tests into a mutable override map initialized by beforeEach. Configure the shared mock implementation there to return an override for matching paths and otherwise delegate to vol.realpathSync, then have each test populate its required path-to-result entry before invoking resolveComponentStories.Source: Coding guidelines
code/core/src/core-server/index.ts (1)
61-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider exporting the docs/stories toolset option and return types for parity with the test toolset.
createTestToolsetexports its options type (CreateTestToolsetOptions) and return type (TestToolset) alongside the factory.createDocsToolsetandcreateStoriesToolsetexport only the factory function (plusPreviewStoriesOutputfor stories). Consumers outside this file cannot nameCreateDocsToolsetOptionsorCreateStoriesToolsetOptionswhen constructing typed inputs, or annotate the return values without relying on inference. Since the MCP packages are expected to adopt this layer in the stacked PR, exporting these types now would avoid a follow-up API-surface patch.🤖 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/core-server/index.ts` around lines 61 - 83, Update the exports in the core-server toolset registration block to also re-export the docs and stories option and return types alongside createDocsToolset and createStoriesToolset. Use the existing CreateDocsToolsetOptions, DocsToolset, CreateStoriesToolsetOptions, and StoriesToolset symbols from their definition modules, preserving the current factory and PreviewStoriesOutput exports.code/addons/vitest/src/preset.test.ts (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the registered content, not only definedness.
toBeDefined()passes for any non-undefined value. Assert the toolset id and a stable fragment of the run description, so the test fails when registration wires the wrong toolset.♻️ Proposed refactor
it('registers the test toolset without a server channel', async () => { await services(undefined, options); - expect(getToolset('test').methods.run.description).toBeDefined(); + const toolset = getToolset('test'); + expect(toolset.id).toBe('test'); + expect(toolset.methods.run.description).toContain('Run story 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/vitest/src/preset.test.ts` around lines 25 - 29, Strengthen the test in the “registers the test toolset without a server channel” case by asserting the registered toolset’s expected id and a stable, meaningful fragment of run.description instead of only checking definedness. Keep the existing services(undefined, options) setup and use getToolset('test') as the registration target.code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts (2)
327-329: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAccess the stubbed
fetchthroughvi.mocked().The tests stub
fetchwithvi.stubGlobaland then assert onglobal.fetch. That access is untyped, sotoHaveBeenCalledWithis not type-checked against the mock. Capture the stub in a variable and wrap it withvi.mocked(), then assert on that reference.As per coding guidelines: "Use
vi.mocked()to type and access the mocked functions in Vitest tests".Also applies to: 402-402, 436-436, 466-466, 488-490
🤖 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 327 - 329, Update the affected tests to capture the function stubbed by vi.stubGlobal in a local variable, access it through vi.mocked(), and use that typed mock for all toHaveBeenCalledTimes and toHaveBeenCalledWith assertions instead of global.fetch, including the additional referenced cases.Source: Coding guidelines
250-253: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the inline snapshot with stable assertions
The snapshot includes Valibot’s default issue message. Assert separately on
ManifestGetErrorandFailed to parse component manifest:instead.🤖 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 250 - 253, Replace the inline snapshot assertion in the getManifests test with separate stable assertions: verify the rejection is a ManifestGetError and that its message contains “Failed to parse component manifest:”. Avoid asserting Valibot’s generated issue details.code/core/src/shared/open-service/toolsets/docs/access-provider.ts (1)
352-361: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAttached MDX refs resolve one at a time.
The loop awaits each
mdx.$reffetch in sequence. A component with many attached docs pays the full round-trip latency per doc.mapWithConcurrencyis already imported in this module and bounds fan-out for story refs. Reuse it here.♻️ Proposed change
if (docEntries.length > 0) { - const docs: Record<string, Doc> = {}; - for (const [docId, doc] of docEntries) { - const mdxRef = 'mdx' in doc ? doc.mdx?.$ref : undefined; - docs[docId] = mdxRef - ? await fetchRefValue<Doc>(mdxRef, request, provider, source, MdxRefPayload) - : (doc as Doc); - } - core.docs = docs; + const resolvedDocs = await mapWithConcurrency( + docEntries, + STORY_REF_CONCURRENCY, + async ([docId, doc]) => { + const mdxRef = 'mdx' in doc ? doc.mdx?.$ref : undefined; + return [ + docId, + mdxRef + ? await fetchRefValue<Doc>(mdxRef, request, provider, source, MdxRefPayload) + : (doc as Doc), + ] as const; + } + ); + core.docs = Object.fromEntries(resolvedDocs); }🤖 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 352 - 361, Update the doc resolution loop in the core.docs assignment to use the imported mapWithConcurrency helper, resolving attached mdx.$ref values concurrently with its bounded concurrency while preserving direct Doc values and the existing docs mapping.code/core/src/shared/open-service/toolsets/docs/access-local.test.ts (1)
14-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the
service-registry.tsmock with the repo's spy-mocking convention.This file mocks
../../service-registry.tswith a factory and nospy: trueoption. Use the spy-based form instead: mock with{ spy: true }, import the real bindings, and wrap them withvi.mocked()inbeforeEach. Set bothgetRegisteredServicesandgetServicebehaviors insidebeforeEach, not at module scope throughvi.hoisted.♻️ Proposed refactor to match the spy-mocking convention
-const getRegisteredServices = vi.hoisted(() => vi.fn<() => Array<{ id: string }>>(() => [])); -const getService = vi.hoisted(() => - vi.fn(() => { - throw new Error('read from the docgen services'); - }) -); -vi.mock('../../service-registry.ts', () => ({ getRegisteredServices, getService })); +import { getRegisteredServices, getService } from '../../service-registry.ts'; + +vi.mock('../../service-registry.ts', { spy: true });beforeEach(() => { - getRegisteredServices.mockReturnValue([]); + vi.mocked(getRegisteredServices).mockReturnValue([]); + vi.mocked(getService).mockImplementation(() => { + throw new Error('read from the docgen services'); + }); });As per coding guidelines, "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests" and "Implement mock behaviors inbeforeEachblocks in Vitest tests."Also applies to: 38-40
🤖 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-local.test.ts` around lines 14 - 20, Update the service-registry mock in this test to use vi.mock('../../service-registry.ts', { spy: true }), import the real getRegisteredServices and getService bindings, and remove the module-scope vi.hoisted mocks. In beforeEach, use vi.mocked() to configure both bindings with their current test behaviors, including the empty registered-services result and getService error.Source: Coding guidelines
code/core/src/shared/open-service/toolsets/docs/public.ts (1)
79-85: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExport
formatMultiSourceManifestsToListsfrom this entry.This entry exports the full composition toolkit:
createCompositionDocsSources,listSources, and theSourceListingtype (lines 37-45). It also exports the single-source rendererformatManifestsToLists. It does not exportformatMultiSourceManifestsToLists, which is the only shared renderer for aSourceListing[]. A consumer ofstorybook/internal/toolsets-docscan therefore build a composed listing but cannot render it with the shared formatter, and would have to duplicate the per-source Markdown layout.♻️ Proposed export
export { formatComponentManifest, formatDocsManifest, formatManifestsToLists, + formatMultiSourceManifestsToLists, formatStoryDocumentation, MAX_STORIES_TO_SHOW, } from './manifest-formatter/markdown.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/docs/public.ts` around lines 79 - 85, Update the public entry’s manifest-formatter export list to include formatMultiSourceManifestsToLists alongside formatManifestsToLists and the other shared renderers, so consumers of the composition toolkit can render SourceListing[] values through the shared formatter.code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.ts (1)
1490-1513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining branches of
formatMultiSourceManifestsToLists.This suite only exercises the
noticebranch. The formatter has three more behaviors that the docs toolset depends on for every composed listing: theerror: <message>branch, per-source## Components/## Docssections, and story sub-bullets underwithStoryIds: true. Add cases for those, including one input that mixes a failing source with a working source, to lock the section boundaries.🤖 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, Expand the formatMultiSourceManifestsToLists tests beyond the existing requires-own-mcp notice case: add coverage for the error notice branch, per-source “## Components” and “## Docs” sections, and story sub-bullets when withStoryIds is true. Include a mixed input containing one failing source and one successful source, and assert the complete output so section boundaries and source-specific content remain correct.
🤖 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-manifest.ts`:
- Around line 54-61: Update createManifestDocsAccess to omit docs manifests
whose docs object is empty, matching toShallowManifests while preserving
manifests containing entries. Add a regression test covering an
experimental_manifests value of { v: 1, docs: {} } and assert that no empty docs
manifest is exposed.
In
`@code/core/src/shared/open-service/toolsets/docs/access-provider-isolation.test.ts`:
- Around line 64-100: Update the manifestProvider stories.json response in the
concurrency test for createProviderDocsAccess so it includes a value for every
generated component reference c0 through c59, allowing all 60 fetches to
complete. Keep the existing in-flight tracking and concurrency assertions
unchanged.
In `@code/core/src/shared/open-service/toolsets/docs/definition.ts`:
- Around line 245-247: Update createDocsToolset at its factory boundary to
reject an empty sources array before computing multiSource or constructing
handlers, while preserving the existing single-source and docsAccess flows. Add
a cheap runtime assertion with the repository’s established assertion mechanism
so { sources: [] } fails immediately rather than allowing undefined access in
list, show, or showStory.
In `@code/core/src/shared/open-service/toolsets/docs/instructions.ts`:
- Around line 21-24: Update the multi-source guidance in the instructions
template to mention that both docs.show and docs.showStory require storybookId
for scoping; use the existing MCP_TOOL_NAMES symbols and preserve the docs.list
behavior.
In `@code/core/src/shared/open-service/toolsets/stories/definition.test.ts`:
- Around line 34-42: Update the fixture path declarations in definition.test.ts
to build repoRoot, storybookWorkingDir, componentPath, orphanPath, and themePath
with path.resolve so expected filesystem paths match Windows resolver output.
Keep changedComponentFile and changedThemeFile as repository-relative
forward-slash strings because they represent Git-reported paths.
In `@code/core/src/shared/open-service/toolsets/stories/definition.ts`:
- Around line 324-335: The matchedComponentCount telemetry currently derives
from raw input length and an incomplete unmatched count, causing missing and
duplicate paths to be over-reported. Update the telemetry calculation in the
lookup results block to count components directly from lookup.results, counting
only results that are found and have at least one match; leave unmatchedCount
and the other telemetry fields unchanged.
---
Nitpick comments:
In `@code/addons/vitest/src/preset.test.ts`:
- Around line 25-29: Strengthen the test in the “registers the test toolset
without a server channel” case by asserting the registered toolset’s expected id
and a stable, meaningful fragment of run.description instead of only checking
definedness. Keep the existing services(undefined, options) setup and use
getToolset('test') as the registration target.
In `@code/core/src/core-server/index.ts`:
- Around line 61-83: Update the exports in the core-server toolset registration
block to also re-export the docs and stories option and return types alongside
createDocsToolset and createStoriesToolset. Use the existing
CreateDocsToolsetOptions, DocsToolset, CreateStoriesToolsetOptions, and
StoriesToolset symbols from their definition modules, preserving the current
factory and PreviewStoriesOutput exports.
In `@code/core/src/shared/open-service/toolsets/docs/access-local.test.ts`:
- Around line 14-20: Update the service-registry mock in this test to use
vi.mock('../../service-registry.ts', { spy: true }), import the real
getRegisteredServices and getService bindings, and remove the module-scope
vi.hoisted mocks. In beforeEach, use vi.mocked() to configure both bindings with
their current test behaviors, including the empty registered-services result and
getService error.
In `@code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts`:
- Around line 327-329: Update the affected tests to capture the function stubbed
by vi.stubGlobal in a local variable, access it through vi.mocked(), and use
that typed mock for all toHaveBeenCalledTimes and toHaveBeenCalledWith
assertions instead of global.fetch, including the additional referenced cases.
- Around line 250-253: Replace the inline snapshot assertion in the getManifests
test with separate stable assertions: verify the rejection is a ManifestGetError
and that its message contains “Failed to parse component manifest:”. Avoid
asserting Valibot’s generated issue details.
In `@code/core/src/shared/open-service/toolsets/docs/access-provider.ts`:
- Around line 352-361: Update the doc resolution loop in the core.docs
assignment to use the imported mapWithConcurrency helper, resolving attached
mdx.$ref values concurrently with its bounded concurrency while preserving
direct Doc values and the existing docs mapping.
In
`@code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.ts`:
- Around line 1490-1513: Expand the formatMultiSourceManifestsToLists tests
beyond the existing requires-own-mcp notice case: add coverage for the error
notice branch, per-source “## Components” and “## Docs” sections, and story
sub-bullets when withStoryIds is true. Include a mixed input containing one
failing source and one successful source, and assert the complete output so
section boundaries and source-specific content remain correct.
In `@code/core/src/shared/open-service/toolsets/docs/public.ts`:
- Around line 79-85: Update the public entry’s manifest-formatter export list to
include formatMultiSourceManifestsToLists alongside formatManifestsToLists and
the other shared renderers, so consumers of the composition toolkit can render
SourceListing[] values through the shared formatter.
In `@code/core/src/shared/open-service/toolsets/stories/find-by-component.ts`:
- Around line 74-87: Change ComponentStoriesResponse in
resolve-component-stories.ts to a discriminated union keyed by available,
requiring reason when unavailable and results when available. Update
resolveComponentStories to return that precise union, then use the narrowed
lookup in the current find-by-component flow to remove the ?? reason and ?? []
fallbacks while preserving its existing unavailable and available branches.
In
`@code/core/src/shared/open-service/toolsets/stories/resolve-component-stories.test.ts`:
- Around line 210-226: Move the per-test realpath behavior from inline
realpathSync.native.mockImplementation calls in the affected tests into a
mutable override map initialized by beforeEach. Configure the shared mock
implementation there to return an override for matching paths and otherwise
delegate to vol.realpathSync, then have each test populate its required
path-to-result entry before invoking resolveComponentStories.
🪄 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: 1c1f8118-5907-49a0-ab99-75c6b049e5c4
📒 Files selected for processing (74)
code/addons/vitest/src/preset.test.tscode/addons/vitest/src/preset.tscode/core/build-config.tscode/core/package.jsoncode/core/src/core-server/index.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/utils/manifests/manifests.tscode/core/src/server-errors.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/index.tscode/core/src/shared/open-service/services/docgen/types.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/access-local.test.tscode/core/src/shared/open-service/toolsets/docs/access-local.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.test.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.tscode/core/src/shared/open-service/toolsets/docs/access-parity.test.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/access-service.test.tscode/core/src/shared/open-service/toolsets/docs/access-service.tscode/core/src/shared/open-service/toolsets/docs/access.tscode/core/src/shared/open-service/toolsets/docs/classify-services.test.tscode/core/src/shared/open-service/toolsets/docs/classify-services.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/adapt-core-manifest.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/manifest-types.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/manifest-formatter/parse-react-docgen.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/parse-react-docgen.tscode/core/src/shared/open-service/toolsets/docs/map-with-concurrency.tscode/core/src/shared/open-service/toolsets/docs/map.test.tscode/core/src/shared/open-service/toolsets/docs/map.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/runtime-agnostic.test.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.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/find-by-component.test.tscode/core/src/shared/open-service/toolsets/stories/find-by-component.tscode/core/src/shared/open-service/toolsets/stories/format.tscode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.test.tscode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.tscode/core/src/shared/open-service/toolsets/stories/story-input.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/open-service/toolsets/test/format.tscode/core/src/shared/open-service/toolsets/test/run.test.tscode/core/src/shared/open-service/toolsets/test/run.tscode/core/src/shared/review/features.test.tscode/core/src/shared/review/features.tscode/core/src/shared/review/review-state.tscode/core/src/storybook-error.tsscripts/build/utils/entry-utils.tsscripts/build/utils/generate-types-rolldown.ts
💤 Files with no reviewable changes (3)
- code/core/src/shared/open-service/toolsets/docs/classify-services.test.ts
- code/core/src/shared/open-service/toolsets/docs/map.test.ts
- code/core/src/shared/open-service/toolsets/docs/map.ts
This was referenced Aug 3, 2026
16 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/definition.test.ts`:
- Line 6: Add pathe to the dependencies declared in code/core/package.json,
matching the version convention used by the repository. Keep the existing import
in definition.test.ts unchanged.
🪄 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: 6e2e4736-35ff-4158-9a2b-e16f1197b57e
📒 Files selected for processing (5)
code/core/src/server-errors.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/toolsets/docs/access-provider-isolation.test.tscode/core/src/shared/open-service/toolsets/docs/definition.tscode/core/src/shared/open-service/toolsets/stories/definition.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- code/core/src/shared/open-service/toolsets/docs/access-provider-isolation.test.ts
- code/core/src/server-errors.ts
- code/core/src/shared/open-service/README.md
- code/core/src/shared/open-service/toolsets/docs/definition.ts
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
kasperpeulen
force-pushed
the
m2b-core-toolsets
branch
from
August 4, 2026 11:44
376092e to
c0a5c53
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts (3)
721-723: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert every authoritative index identity field.
The fixtures use matching index and payload identities. Both tests assert only
id. A regression that overridesname,description,summary, orerrorwould pass. Use valid conflicting payload values and assert that all index identity fields remain authoritative.
code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts#L721-L723: Assertid,name,description,summary, anderrorfrom the index entry.code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts#L835-L837: Assert the same contract for split docgen, story-docs, and MDX references.🤖 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 721 - 723, Strengthen the tests around the resolved stub identities by using payload values that conflict with the index entry, then assert that the index remains authoritative for id, name, description, summary, and error. Apply this contract at code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts lines 721-723 and 835-837, including the split docgen, story-docs, and MDX reference cases.
544-568: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the parallelism test prove concurrent execution.
The result assertions also pass when
listSourcesruns sources serially. Block one source response, startgetMultiSourceManifests, and assert that the other source starts before you release the block. This makes the test enforce the concurrent-source 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/core/src/shared/open-service/toolsets/docs/access-provider.test.ts` around lines 544 - 568, Update the “should fetch manifests from multiple sources in parallel” test around getMultiSourceManifests to gate one source’s manifest response with a promise, start the request, and wait until the other source’s provider call has begun before releasing the gate. Assert that the second source starts while the first remains blocked, then resolve the blocked response and retain the existing result assertions.
54-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCentralize Vitest mock configuration.
The test creates and configures mocks in helpers and
itblocks. This does not follow the requiredbeforeEachandvi.mocked()pattern. Declare reusable mock handles before tests. Configure behavior inbeforeEachfrom fixture data.
code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts#L54-L128: Replace mock factories with response-data helpers and reusable mock handles.code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts#L131-L137: Access the stubbedfetchfunction throughvi.mocked()while retaining tracked global cleanup.code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts#L152-L304: Move per-testfetchimplementations into fixture-drivenbeforeEachsetup.code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts#L544-L655: ConfiguremanifestProviderfrom suite fixture state instead of creating implementations in each test.code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts#L690-L890: Apply the same setup pattern to reference-resolution provider mocks.code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts#L887-L936: Configure the validation provider from arefBodyfixture inbeforeEach.As per coding guidelines: “Place all mocks at the top of the test file before any test cases”, “Implement mock behaviors in
beforeEachblocks 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/core/src/shared/open-service/toolsets/docs/access-provider.test.ts` around lines 54 - 128, Centralize Vitest mock setup in code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts:54-128 by replacing createFetchMock and createManifestProviderMock implementations with reusable response fixtures and top-level mock handles. At 131-137, access the global fetch stub through vi.mocked() while preserving cleanup. At 152-304 and 544-655, move fetch and manifestProvider behavior into beforeEach using suite fixture state; at 690-890, apply the same pattern to reference-resolution mocks; and at 887-936, configure the validation provider from the refBody fixture in beforeEach. Keep all mock declarations before tests and avoid per-test mock implementations.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/test/definition.ts`:
- Around line 65-66: Update the non-MCP branch of formatTestRunSummary to
include data.notFoundMessages when rendering a no-stories result. Preserve the
existing summary output while ensuring terminal consumers receive each unmatched
selector diagnostic.
- Around line 156-182: Update reportRunTelemetry to catch and isolate rejections
from ctx.telemetry, allowing test.run to continue returning the normal
formatTestRun outcome for both completed and no-stories statuses. Preserve the
existing telemetry payloads and status handling, and add tests using a rejecting
telemetry callback to verify both outcomes remain structured and successful.
- Around line 176-181: Update the telemetry payload in the tool:runStoryTests
flow to omit matchedStoryCount when data.result.storyIds is unavailable, rather
than falling back to inputStoryCount. Preserve reporting the storyIds length
when IDs are present, while keeping the remaining summarizeTestRun telemetry
unchanged.
- Around line 214-219: Update runStoryTests so the getIndex invocation is
wrapped in try-catch before its rejection can escape; convert any rejection into
the existing error result shape with status 'error' and an error message,
preserving the handler’s { ok: false, data, markdown } response path.
---
Nitpick comments:
In `@code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts`:
- Around line 721-723: Strengthen the tests around the resolved stub identities
by using payload values that conflict with the index entry, then assert that the
index remains authoritative for id, name, description, summary, and error. Apply
this contract at
code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts lines
721-723 and 835-837, including the split docgen, story-docs, and MDX reference
cases.
- Around line 544-568: Update the “should fetch manifests from multiple sources
in parallel” test around getMultiSourceManifests to gate one source’s manifest
response with a promise, start the request, and wait until the other source’s
provider call has begun before releasing the gate. Assert that the second source
starts while the first remains blocked, then resolve the blocked response and
retain the existing result assertions.
- Around line 54-128: Centralize Vitest mock setup in
code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts:54-128
by replacing createFetchMock and createManifestProviderMock implementations with
reusable response fixtures and top-level mock handles. At 131-137, access the
global fetch stub through vi.mocked() while preserving cleanup. At 152-304 and
544-655, move fetch and manifestProvider behavior into beforeEach using suite
fixture state; at 690-890, apply the same pattern to reference-resolution mocks;
and at 887-936, configure the validation provider from the refBody fixture in
beforeEach. Keep all mock declarations before tests and avoid per-test mock
implementations.
🪄 Autofix
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: 22128d5f-4c16-4920-93ff-e117192b5d85
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (74)
code/addons/vitest/src/preset.test.tscode/addons/vitest/src/preset.tscode/core/build-config.tscode/core/package.jsoncode/core/src/core-server/index.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/utils/manifests/manifests.tscode/core/src/server-errors.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/index.tscode/core/src/shared/open-service/services/docgen/types.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/access-local.test.tscode/core/src/shared/open-service/toolsets/docs/access-local.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.test.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.tscode/core/src/shared/open-service/toolsets/docs/access-parity.test.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/access-service.test.tscode/core/src/shared/open-service/toolsets/docs/access-service.tscode/core/src/shared/open-service/toolsets/docs/access.tscode/core/src/shared/open-service/toolsets/docs/classify-services.test.tscode/core/src/shared/open-service/toolsets/docs/classify-services.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/adapt-core-manifest.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/manifest-types.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/manifest-formatter/parse-react-docgen.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/parse-react-docgen.tscode/core/src/shared/open-service/toolsets/docs/map-with-concurrency.tscode/core/src/shared/open-service/toolsets/docs/map.test.tscode/core/src/shared/open-service/toolsets/docs/map.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/runtime-agnostic.test.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.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/find-by-component.test.tscode/core/src/shared/open-service/toolsets/stories/find-by-component.tscode/core/src/shared/open-service/toolsets/stories/format.tscode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.test.tscode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.tscode/core/src/shared/open-service/toolsets/stories/story-input.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/open-service/toolsets/test/format.tscode/core/src/shared/open-service/toolsets/test/run.test.tscode/core/src/shared/open-service/toolsets/test/run.tscode/core/src/shared/review/features.test.tscode/core/src/shared/review/features.tscode/core/src/shared/review/review-state.tscode/core/src/storybook-error.tsscripts/build/utils/entry-utils.tsscripts/build/utils/generate-types-rolldown.ts
💤 Files with no reviewable changes (3)
- code/core/src/shared/open-service/toolsets/docs/map.ts
- code/core/src/shared/open-service/toolsets/docs/classify-services.test.ts
- code/core/src/shared/open-service/toolsets/docs/map.test.ts
🚧 Files skipped from review as they are similar to previous changes (68)
- code/core/src/shared/open-service/services/docgen/types.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.ts
- code/core/package.json
- code/core/src/shared/open-service/toolsets/docs/instructions.ts
- code/core/src/shared/open-service/toolsets/docs/runtime-agnostic.test.ts
- code/core/src/shared/open-service/toolsets/docs/map-with-concurrency.ts
- code/addons/vitest/src/preset.test.ts
- code/core/build-config.ts
- scripts/build/utils/entry-utils.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/parse-react-docgen.ts
- code/core/src/shared/review/features.ts
- code/core/src/shared/open-service/toolsets/test/run.ts
- code/core/src/core-server/index.ts
- code/core/src/shared/open-service/toolsets/docs/access-service.test.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.ts
- code/core/src/shared/open-service/toolsets/test/definition.test.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.test.ts
- code/core/src/shared/open-service/toolsets/docs/multi-source.ts
- code/addons/vitest/src/preset.ts
- code/core/src/shared/open-service/toolset-names.ts
- code/core/src/shared/open-service/toolsets/test/run.test.ts
- code/core/src/shared/review/features.test.ts
- code/core/src/shared/open-service/toolsets/docs/access-parity.test.ts
- code/core/src/storybook-error.ts
- code/core/src/shared/open-service/toolsets/docs/access-provider-isolation.test.ts
- code/core/src/shared/open-service/toolsets/docs/access-manifest.test.ts
- code/core/src/shared/open-service/toolsets/docs/public.ts
- code/core/src/shared/open-service/toolsets/docs/sources.ts
- code/core/src/shared/open-service/toolsets/estimate-tokens.ts
- code/core/src/core-server/presets/common-preset.ts
- code/core/src/shared/open-service/toolset-registry.ts
- code/core/src/shared/open-service/toolsets/stories/story-input.ts
- code/core/src/shared/open-service/toolsets/estimate-tokens.test.ts
- code/core/src/shared/open-service/toolsets/stories/unreachable-files.ts
- code/core/src/shared/open-service/index.ts
- code/core/src/shared/open-service/toolsets/docs/portable-dist.test.ts
- code/core/src/shared/open-service/toolsets/docs/classify-services.ts
- code/core/src/shared/open-service/toolsets/docs/access-manifest.ts
- code/core/src/core-server/utils/manifests/manifests.ts
- code/core/src/shared/open-service/toolsets/docs/access-service.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.ts
- code/core/src/shared/open-service/toolsets/stories/find-by-component.ts
- code/core/src/shared/open-service/toolsets/stories/definition.test.ts
- code/core/src/shared/open-service/toolsets/stories/find-by-component.test.ts
- code/core/src/shared/review/review-state.ts
- code/core/src/shared/open-service/toolset-definition.test-d.ts
- code/core/src/shared/open-service/toolset-registry.test.ts
- code/core/src/shared/open-service/README.md
- code/core/src/shared/open-service/toolsets/docs/definition.ts
- code/core/src/shared/open-service/toolsets/stories/definition.ts
- code/core/src/shared/open-service/toolsets/stories/resolve-component-stories.test.ts
- code/core/src/shared/open-service/toolsets/docs/definition.test.ts
- code/core/src/shared/open-service/toolsets/docs/access-provider.ts
- code/core/src/shared/open-service/toolset-types.ts
- code/core/src/shared/open-service/toolsets/test/format.ts
- code/core/src/shared/open-service/toolsets/docs/sources.test.ts
- code/core/src/shared/open-service/toolsets/docs/access-local.ts
- code/core/src/shared/open-service/toolsets/stories/format.ts
- code/core/src/shared/open-service/toolset-definition.ts
- code/core/src/shared/open-service/toolsets/docs/multi-source.test.ts
- code/core/src/shared/open-service/toolsets/docs/access-local.test.ts
- code/core/src/shared/open-service/toolsets/docs/access.ts
- code/core/src/server-errors.ts
- code/core/src/shared/open-service/toolsets/review/definition.test.ts
- code/core/src/shared/open-service/toolsets/stories/resolve-component-stories.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/manifest-types.ts
- code/core/src/shared/open-service/toolsets/review/definition.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/parse-react-docgen.test.ts
2 of 9 tasks
Member
|
Naming nit on |
Closed
2 of 9 tasks
1 of 9 tasks
JReinhold
approved these changes
Aug 7, 2026
JReinhold
force-pushed
the
m2b-core-toolsets
branch
from
August 10, 2026 11:20
c0a5c53 to
0bd906a
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-local.test.ts`:
- Around line 14-20: Replace the manual factory mock for service-registry with a
spy mock using { spy: true }. Obtain getRegisteredServices and getService
through vi.mocked(), then reset both and configure their return/throw behavior
in beforeEach rather than during module initialization. Apply the same pattern
to the related mocked setup around the additional referenced lines.
🪄 Autofix
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: 19cf5c17-ecaf-4c56-8a23-017073134730
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (74)
code/addons/vitest/src/preset.test.tscode/addons/vitest/src/preset.tscode/core/build-config.tscode/core/package.jsoncode/core/src/core-server/index.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/utils/manifests/manifests.tscode/core/src/server-errors.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/index.tscode/core/src/shared/open-service/services/docgen/types.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/access-local.test.tscode/core/src/shared/open-service/toolsets/docs/access-local.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.test.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.tscode/core/src/shared/open-service/toolsets/docs/access-parity.test.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/access-service.test.tscode/core/src/shared/open-service/toolsets/docs/access-service.tscode/core/src/shared/open-service/toolsets/docs/access.tscode/core/src/shared/open-service/toolsets/docs/classify-services.test.tscode/core/src/shared/open-service/toolsets/docs/classify-services.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/adapt-core-manifest.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/manifest-types.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/manifest-formatter/parse-react-docgen.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/parse-react-docgen.tscode/core/src/shared/open-service/toolsets/docs/map-with-concurrency.tscode/core/src/shared/open-service/toolsets/docs/map.test.tscode/core/src/shared/open-service/toolsets/docs/map.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/runtime-agnostic.test.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.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/find-by-component.test.tscode/core/src/shared/open-service/toolsets/stories/find-by-component.tscode/core/src/shared/open-service/toolsets/stories/format.tscode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.test.tscode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.tscode/core/src/shared/open-service/toolsets/stories/story-input.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/open-service/toolsets/test/format.tscode/core/src/shared/open-service/toolsets/test/run.test.tscode/core/src/shared/open-service/toolsets/test/run.tscode/core/src/shared/review/features.test.tscode/core/src/shared/review/features.tscode/core/src/shared/review/review-state.tscode/core/src/storybook-error.tsscripts/build/utils/entry-utils.tsscripts/build/utils/generate-types-rolldown.ts
💤 Files with no reviewable changes (3)
- code/core/src/shared/open-service/toolsets/docs/classify-services.test.ts
- code/core/src/shared/open-service/toolsets/docs/map.ts
- code/core/src/shared/open-service/toolsets/docs/map.test.ts
🚧 Files skipped from review as they are similar to previous changes (67)
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/parse-react-docgen.ts
- code/core/package.json
- code/core/src/shared/open-service/services/docgen/types.ts
- code/core/build-config.ts
- code/core/src/shared/review/features.ts
- code/core/src/core-server/utils/manifests/manifests.ts
- code/core/src/shared/open-service/toolsets/docs/map-with-concurrency.ts
- code/core/src/shared/open-service/toolsets/docs/access-provider-isolation.test.ts
- code/addons/vitest/src/preset.test.ts
- code/core/src/shared/open-service/toolsets/docs/portable-dist.test.ts
- code/core/src/shared/open-service/toolsets/test/definition.test.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.ts
- code/core/src/shared/open-service/toolsets/docs/public.ts
- code/core/src/shared/open-service/toolsets/docs/access-local.ts
- code/core/src/shared/open-service/toolset-names.ts
- code/core/src/shared/open-service/toolsets/test/run.ts
- code/core/src/shared/open-service/toolsets/docs/access-manifest.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.ts
- code/core/src/shared/open-service/toolsets/docs/sources.test.ts
- code/core/src/shared/open-service/toolsets/docs/instructions.ts
- code/core/src/shared/open-service/toolsets/stories/find-by-component.test.ts
- code/core/src/shared/open-service/toolsets/docs/access-provider.test.ts
- scripts/build/utils/entry-utils.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.ts
- code/core/src/shared/open-service/toolsets/test/run.test.ts
- code/core/src/shared/review/features.test.ts
- code/core/src/shared/open-service/toolsets/docs/access-manifest.test.ts
- code/core/src/shared/open-service/toolsets/docs/multi-source.ts
- code/core/src/shared/open-service/toolset-types.ts
- code/core/src/shared/open-service/toolsets/docs/multi-source.test.ts
- code/core/src/shared/open-service/toolsets/stories/unreachable-files.ts
- code/core/src/shared/open-service/toolsets/docs/sources.ts
- code/core/src/shared/review/review-state.ts
- code/core/src/shared/open-service/toolsets/estimate-tokens.test.ts
- code/core/src/shared/open-service/toolsets/docs/access-service.ts
- code/core/src/server-errors.ts
- code/core/src/shared/open-service/toolsets/docs/runtime-agnostic.test.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.test.ts
- code/core/src/shared/open-service/toolsets/review/definition.test.ts
- code/core/src/shared/open-service/toolsets/docs/access-parity.test.ts
- code/core/src/shared/open-service/toolsets/test/format.ts
- code/core/src/storybook-error.ts
- code/core/src/shared/open-service/toolset-registry.test.ts
- code/core/src/shared/open-service/toolsets/stories/definition.ts
- code/core/src/shared/open-service/toolsets/stories/find-by-component.ts
- code/core/src/core-server/index.ts
- code/core/src/shared/open-service/toolset-definition.ts
- code/core/src/shared/open-service/toolsets/stories/resolve-component-stories.test.ts
- code/core/src/shared/open-service/toolsets/docs/definition.ts
- code/core/src/shared/open-service/toolsets/test/definition.ts
- code/core/src/shared/open-service/toolsets/estimate-tokens.ts
- code/core/src/shared/open-service/toolsets/review/definition.ts
- code/core/src/shared/open-service/toolsets/docs/classify-services.ts
- code/core/src/shared/open-service/toolsets/docs/access-service.test.ts
- code/core/src/shared/open-service/toolsets/stories/format.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/manifest-types.ts
- code/core/src/shared/open-service/toolsets/stories/definition.test.ts
- code/core/src/shared/open-service/index.ts
- code/core/src/core-server/presets/common-preset.ts
- code/core/src/shared/open-service/toolsets/stories/resolve-component-stories.ts
- code/core/src/shared/open-service/toolsets/docs/manifest-formatter/parse-react-docgen.test.ts
- code/core/src/shared/open-service/toolset-registry.ts
- code/core/src/shared/open-service/toolsets/stories/story-input.ts
- code/core/src/shared/open-service/toolsets/docs/definition.test.ts
- code/addons/vitest/src/preset.ts
- code/core/src/shared/open-service/README.md
- code/core/src/shared/open-service/toolsets/docs/access-provider.ts
…y, the frozen names
JReinhold
force-pushed
the
m2b-core-toolsets
branch
from
August 11, 2026 11:34
0bd906a to
a5ab56c
Compare
6 of 9 tasks
JReinhold
force-pushed
the
m2b-core-toolsets
branch
from
August 12, 2026 11:17
7c209cf to
a5ab56c
Compare
This was referenced Aug 12, 2026
This was referenced Aug 13, 2026
6 of 9 tasks
This was referenced Aug 24, 2026
JReinhold
added
qa:skip
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #35673. Stacked: #35677 is based on this branch — it switches
@storybook/addon-mcpand@storybook/mcpover to this layer and deletes their internal engines. Merge this PR first; GitHub retargets #35677 tonextautomatically.What I did
Storybook's agent-facing tools (story preview URLs, story lookup, test runs, docs, review) live inside
@storybook/addon-mcpand@storybook/mcptoday, as two separate engines. This PR moves that capability into core as toolsets: one definition per capability that every consumer — the MCP addon, the hosted MCP package, and the upcomingstorybook toolsCLI (#35719) — renders from.In this PR the two MCP packages are untouched and still run their own engines; the stacked #35677 is the switch.
A toolset is the public agent surface of an open service: where a service (
defineService) owns state, queries and commands, a toolset (defineToolset) owns how that capability is presented to an agent — input/output schemas, tool prose, telemetry, and the rendered Markdown.The toolsets
storiespreview,changed,findByComponentpreview-stories,get-changed-stories,get-stories-by-componentservicespreset hooktestrunrun-story-testsserviceshookreviewcreatedisplay-reviewservicespreset hookdocslist,show,showStorylist-all-documentation,get-documentation,get-documentation-for-storyservicespreset hookRegistration shares its gate with the feature itself: a disabled feature registers neither the service nor the toolset.
MCP_TOOL_NAMESintoolset-names.tsis the frozen wire contract — changing an entry is a breaking change — and both MCP adapters register from that one map, so description prose and registration cannot disagree.What a toolset looks like
Real code — the
testtoolset's definition, minus its valibot schemas:Methods that publish structured output additionally declare an
outputSchema; the adapter narrowsdatato it before putting it on the wire.Why this shape
The question underneath the format: one capability must serve MCP (a JSON-RPC reply) and the CLI (stdout + an exit code) without either consumer re-implementing what a result means.
content(text) andstructuredContent(JSON) in the same message, and a method liketest.runhas side effects — you cannot run it twice to produce the two renderings. So the singlehandlerreturns everything at once:{ ok, data, markdown }. Adapters unwrap mechanically —markdown→ text blocks,data→structuredContent,ok→ MCPisError/ CLI exit code — and never re-derive meaning from the data.{ ok: false, data, markdown }. Methods that cannot fail declareTFailure = never.MCP_TOOL_NAMES. The title is what UIs display — required prose on each method, freely editable. They live in different places because they change for different reasons.descriptionmay be a function ofctx({ consumer: 'cli' | 'mcp', origin?, uiRoot?, … }), and sibling tools are referenced throughgetRef(ctx)— the same sentence renderspreview-storiesfor an MCP client andnpx storybook tools stories previewfor the CLI.agentFacing: true; adapters surface those verbatim by reading the property — never via aninstanceoflist, which misclassifies across bundle copies.The docs engine
The docs toolset is runtime-agnostic behind one injected interface,
DocsAccess(list+resolve), with exactly three implementations: service (in-process, reading the open services when the docgen service registered), manifest (the manifest files core builds — the default), and provider (manifest files over any transport — HTTP, a static bundle, an authenticated proxy — following$refs and validating on arrival). Multi-Storybook composition composes accesses — one per source, astorybookIdinput, a failing source isolated instead of failing the listing — rather than being a second engine.The whole docs surface exports through
storybook/internal/toolsets-docs, a portable entry: its d.ts bundles into one flat self-contained file whose only import isvalibot, so@storybook/mcpcan inline it without depending onstorybookat runtime.portable-dist.test.tsguards flatness, the allowlist, and a size budget inside core's own build.Where the implementation comes from
The method bodies are the shipped engines, moved: story lookup and test-run handling from
@storybook/addon-mcp, the manifest formatter and multi-source handling from@storybook/mcp. They were ported, not rewritten — behavior-identical, with the evidence below. What is genuinely new in this PR is the API around them.How to review (~2½ hours)
75 files, +8,408/−1,855 — but more than half of the added lines are tests, and most of the rest is moved engine code. The real review is blocks 1–4: +2.6k lines of the diff, plus the 928-line README read in full.
The branch's six commits are these six blocks, in this order. Each block heading links straight to its commit's diff — review one commit at a time, and read the block's intro here before opening any file.
Block 1 · The contract — 25 min
Files:
open-service/README.md— the diff is only +48 −9, but read the full 928-line file; most of it predates this branch.This layer is spec-first: the README states every rule, and the code exists to implement them. Reviewing the README is reviewing the design — if a rule convinces you here, the remaining blocks reduce to checking conformance. The whole model fits in six lines:
What to check: every rule comes with its reason — do you buy them? Anything you would veto, veto now: after #35677 and the CLI PR #35719, three consumers stand on these rules and they harden into public contract.
Block 2 · The construct — 30 min
Files:
toolset-definition.ts,toolset-types.ts,toolset-registry.ts,toolset-names.ts(+403 −50 — the four source files are only 306 lines; the rest istoolset-definition.test-d.tspinning the type inference)The API every future toolset is written against, so it earns the most careful read per line. The runtime is deliberately thin — most of the value lives in the types:
What to check: the inference pinned in
toolset-definition.test-d.ts(schema types flow into the handler;TFailure = neverfor infallible methods); that the registry's failure modes are loud; that nothing here is speculative — every construct has a consumer in this stack.Block 3 · The tool surface — 45 min
Files:
toolsets/{stories,test,review,docs}/definition.ts(+810 −354)The 8 methods across the four toolsets are the actual product: exactly what an agent sees, in schema and prose. This is where a wrong decision costs the most later and is cheapest to fix now. A per-method checklist:
The "What a toolset looks like" section above shows one full real method (
test.run) — hold the other seven against that shape. Dip into imported helpers only where a definition makes you curious: the helper bodies are ported engine code (block 6).Block 4 · The docs machinery — 40 min
Files:
toolsets/docs/access*.ts,sources.ts,multi-source.ts,public.ts,instructions.ts(9 files, +1,306 — all new)The only genuinely new machinery in the PR: one docs toolset running against three interchangeable data accesses, and a composition that is access composition, not a second engine.
What to check: is
DocsAccessthe right seam — small enough to hold, big enough to serve all three?access-parity.test.tspins that the accesses answer identically. And the selection rule: which access serves the local Storybook is decided by what actually registered, not by the feature flag alone.Block 5 · Wiring and build — 15 min
Files:
common-preset.ts,core-server/index.ts,manifests.ts,build-config.ts,scripts/build/*,addons/vitest/preset.ts(7 files, +237 −52)Where the toolsets meet the running server and the published package:
What to check (skim): each registration site's gate matches the gate that decides whether the tool is offered; the PUSH_REVIEW channel adapter is deliberately kept alive here (released addon-mcp versions still emit it — #35677 deletes it); the portable d.ts pass in
scripts/build.Block 6 · The ported bodies and their tests — 10 min
Files: everything else under
toolsets/(49 files, +5,604 −1,390 — over half of it tests)The engine internals, moved from the MCP packages: story lookup, test-run handling, the manifest formatter, markdown rendering. Trust and spot-check at will — behavioral equality is pinned by the evidence below, not by line-reading five thousand lines.
What you do not need to review, and why that is safe:
next's own MCP e2e suite passes unchanged against this tree (43/43 — old engines, real dev servers)portable-dist.test.ts(flatness,valibot-only imports, size budget) fails core's build on regressionChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn nx run-many -t compilecd test-storybooks/mcp && yarn install && yarn storybook(port 6006)http://localhost:6006/mcp: the tool list and every tool response are unchanged — the addon still runs its own engine in this PR.display-reviewstill works end-to-end (the PUSH_REVIEW channel adapter is intentionally kept alive here; Skills M4: Run addon-mcp and @storybook/mcp on the shared core toolsets #35677 deletes it).code/core/dist/shared/open-service/toolsets/docs/public.d.tsis one flat file importing onlyvalibot(asserted byportable-dist.test.tsin CI).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.