Repository navigation
Skills M5a: CLI: Add the public storybook tools command derived at runtime from the OSA toolsets - #35850
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (53)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (48)
WalkthroughThe pull request adds the ChangesStorybook tools CLI
Sequence Diagram(s)sequenceDiagram
participant ToolsCLI
participant Runtime
participant Storybook
participant VitestOrMCP
ToolsCLI->>Runtime: load toolsets and target configuration
Runtime->>Storybook: prepare channel and optional module graph
ToolsCLI->>VitestOrMCP: execute local test or proxy server-required method
VitestOrMCP-->>ToolsCLI: return structured result
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
code/core/src/shared/open-service/toolset-names.ts (1)
24-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared converter at every CLI boundary.
Line 31 exports the shared converter, but
code/core/src/cli/tools/run.tsandcode/core/src/cli/tools/help.tsstill define localtoCliMethodNamefunctions. Import this helper in those modules and remove the copies. This prevents help and command execution from diverging after later changes.🤖 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-names.ts` around lines 24 - 38, Import the shared toCliMethodName helper from toolset-names.ts into the CLI tools run and help modules, remove their local toCliMethodName implementations, and update their call sites to use the imported converter so command execution and help share one naming authority.code/addons/vitest/src/preset.test.ts (2)
45-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe assertion does not establish the Vite-specific claim in the test name.
wireTestRunResponderregisters theTRIGGER_TEST_RUN_REQUESTlistener for every builder. It usesviteBuilderonly to choose the response, not to decide whether to register. The first test at Line 37 passes nocore, so it also registers the listener. The assertion at Line 50 therefore holds for Webpack projects as well.Rename the test to state what it verifies, for example "wires the test-run responder next to the registration". The builder-specific behavior is already covered in
code/addons/vitest/src/node/test-run-responder.test.tsat Line 207.🤖 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 45 - 51, Rename the test case around services to remove the Vite-specific wording, since wireTestRunResponder registers the TRIGGER_TEST_RUN_REQUEST listener for all builders. Keep the existing assertion and setup unchanged; builder-specific response behavior is covered elsewhere.
9-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn the mock types from the factory so assertions can use
vi.mocked().
makeOptionscasts throughunknowntoOptions.options.channel.onis therefore typed as the real channel method. The assertion at Line 50 callstoHaveBeenCalledWithon a value that TypeScript does not know is a mock. Return the spies alongside the options, or assert throughvi.mocked().♻️ Proposed refactor to keep the spies typed
-function makeOptions({ core }: { core?: unknown } = {}): Options { - return { - channel: { on: vi.fn(), off: vi.fn(), emit: vi.fn() }, +function makeOptions({ core }: { core?: unknown } = {}) { + const channel = { on: vi.fn(), off: vi.fn(), emit: vi.fn() }; + const options = { + channel, presets: { apply: async (key: string, fallback?: unknown) => { switch (key) { case 'storyIndexGenerator': return { getIndex: async () => ({ v: 5, entries: {} }) }; case 'core': return core ?? fallback; default: return fallback; } }, }, } as unknown as Options; + return { options, channel }; }The spy-mocking rules require type-safe access through
vi.mocked()and discourage direct function mocking without 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/addons/vitest/src/preset.test.ts` around lines 9 - 25, Update makeOptions to expose the channel spies with mock-aware types alongside the Options object, then use those returned spies through vi.mocked() in the affected assertions. Preserve the existing channel behavior while ensuring calls such as toHaveBeenCalledWith operate on values TypeScript recognizes as Vitest mocks.Source: Learnings
code/addons/vitest/src/node/test-run-responder.test.ts (2)
15-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the
spy: trueoption to the module mocks.The four
vi.mockcalls at Line 15, Line 28, Line 40, and Line 44 supply factories withoutspy: true. The mocks at Line 40 and Line 44 replace the whole module, so any export added later to./boot-test-runner.tsor../logger.tsbecomesundefinedinside the responder.As per path instructions for
**/*.test.{ts,tsx,js,jsx}: "Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests" and "Avoid mocking without thespy: trueoption in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/vitest/src/node/test-run-responder.test.ts` around lines 15 - 46, Add the `spy: true` option to all four `vi.mock` calls in `test-run-responder.test.ts`, including the `storybook/internal/core-server`, `storybook/internal/common`, `./boot-test-runner.ts`, and `../logger.ts` mocks, while preserving their existing factories and mocked behavior.Source: Learnings
88-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
CANCEL_RUNandFATAL_ERRORresponse branches.The suite covers
completed, the non-Vite error, the setup-failure error, and the already-running error. It does not cover two branches of the responder subscription incode/addons/vitest/src/node/test-run-responder.tsat Lines 265-276:
FATAL_ERRORmaps tostatus: 'error'with the event payload.CANCEL_RUNmaps tostatus: 'cancelled'.Both statuses are part of
TriggerTestRunResponsePayloadincode/addons/vitest/src/constants.ts, and the CLI maps them to distinct exit codes. Each test can reuse the existing pattern: emit a request, take the store from therunTestRunnercall, thenstore.sendthe event.🤖 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/node/test-run-responder.test.ts` around lines 88 - 276, Add tests in the wireTestRunResponder suite covering the responder subscription’s FATAL_ERROR and CANCEL_RUN branches. Reuse the existing request and runTestRunner store setup, send each event through store.send, and assert the matching request receives status 'error' with the fatal-error payload and status 'cancelled' respectively.code/core/src/cli/tools/instances/registry.test.ts (1)
28-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
process.killbehavior intobeforeEach.Line 28 configures a direct mock. Line 109 changes its implementation inside a test case. Create the spy in
beforeEach, configure it throughvi.mocked(process.kill), and let each test set only a liveness outcome value.As per coding guidelines: “Implement mock behaviors in
beforeEachblocks in Vitest tests” and “Avoid direct function mocking withoutvi.mocked()in Vitest tests.”Also applies to: 108-113
🤖 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/cli/tools/instances/registry.test.ts` around lines 28 - 35, Move the process.kill spy setup into beforeEach, creating the spy there and configuring it via vi.mocked(process.kill). Refactor the affected test around the line-109 implementation change so each test only sets the desired liveness outcome, while preserving the ESRCH behavior for non-current PIDs and success for process.pid.Source: Coding guidelines
code/core/src/cli/tools/register.ts (1)
212-227: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFlush the confirmation log before the explicit exit.
The
--outputbranch writes the file, then callslogger.log. The action callsprocess.exit()right after. The stdout branch awaits the write callback for this reason. Iflogger.logbuffers on a piped stdout, the confirmation line can be truncated. Consider routing the confirmation through the same awaited write, or awaiting a flush before exit.🤖 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/cli/tools/register.ts` around lines 212 - 227, Update printResult’s outputPath branch so the “Output written to …” confirmation is emitted through an awaited stdout write, or otherwise explicitly flushed, before returning to the caller that invokes process.exit(). Preserve the existing file write and confirmation message while ensuring it cannot be truncated on piped stdout.
🤖 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/.oxlintrc.json`:
- Around line 98-102: Update the import restriction pattern in the patterns
configuration to reject all parent-relative paths resolving into cli/ai,
including ../ai/**, ../../ai/**, and deeper variants, while retaining the
existing restriction on paths matching **/cli/ai/**. Use a supported glob or
regex syntax that matches every parent-relative depth.
In `@code/addons/vitest/src/node/test-run-responder.ts`:
- Around line 97-101: Update the state-change listener around fsCache.set to
consume its promise rejection so cache-write failures do not become unhandled
rejections or terminate the dev server/CLI; preserve the existing state
comparison and write behavior while adding appropriate rejection handling.
- Around line 242-278: Add a unique request/run identifier to the TRIGGER_RUN
flow and propagate it through currentRun and TEST_RUN_COMPLETED so the
subscription in the responder only handles terminal events matching its own
request. Add child-runner exit, close, and error handling that emits an
appropriate terminal store event, then ensure every terminal path unsubscribes,
clears active-run state, resets requestInFlight, and responds with an error when
the runner terminates unexpectedly.
In `@code/builders/builder-vite/src/change-detection-adapter/headless.test.ts`:
- Around line 10-16: Update the headless test setup to use Vitest’s spy-mock
pattern: mock both modules with { spy: true }, import resolveConfig and
commonConfig, and access them via vi.mocked(). Move the mock implementations for
resolveConfig, commonConfig, and presets.apply into beforeEach, removing the
hoisted mock functions.
In `@code/core/src/cli/tools/register.ts`:
- Around line 190-209: Guard the telemetry operations in
reportToolsCommandTelemetry so failures from telemetry() or sendTelemetryError()
are caught and cannot propagate from the action’s finally block into command
failure handling. Apply the existing createMethodTelemetrySink pattern while
preserving telemetry reporting and the current outcome handling.
In `@code/core/src/shared/open-service/toolsets/review/definition.ts`:
- Around line 148-149: Update the toolset contract documentation and README
method-field description to include requiresDevServer as the sixth property.
Document that true requires a running Storybook dev server/instance for
publishing and UI linking, while false supports local execution without the
running instance.
---
Nitpick comments:
In `@code/addons/vitest/src/node/test-run-responder.test.ts`:
- Around line 15-46: Add the `spy: true` option to all four `vi.mock` calls in
`test-run-responder.test.ts`, including the `storybook/internal/core-server`,
`storybook/internal/common`, `./boot-test-runner.ts`, and `../logger.ts` mocks,
while preserving their existing factories and mocked behavior.
- Around line 88-276: Add tests in the wireTestRunResponder suite covering the
responder subscription’s FATAL_ERROR and CANCEL_RUN branches. Reuse the existing
request and runTestRunner store setup, send each event through store.send, and
assert the matching request receives status 'error' with the fatal-error payload
and status 'cancelled' respectively.
In `@code/addons/vitest/src/preset.test.ts`:
- Around line 45-51: Rename the test case around services to remove the
Vite-specific wording, since wireTestRunResponder registers the
TRIGGER_TEST_RUN_REQUEST listener for all builders. Keep the existing assertion
and setup unchanged; builder-specific response behavior is covered elsewhere.
- Around line 9-25: Update makeOptions to expose the channel spies with
mock-aware types alongside the Options object, then use those returned spies
through vi.mocked() in the affected assertions. Preserve the existing channel
behavior while ensuring calls such as toHaveBeenCalledWith operate on values
TypeScript recognizes as Vitest mocks.
In `@code/core/src/cli/tools/instances/registry.test.ts`:
- Around line 28-35: Move the process.kill spy setup into beforeEach, creating
the spy there and configuring it via vi.mocked(process.kill). Refactor the
affected test around the line-109 implementation change so each test only sets
the desired liveness outcome, while preserving the ESRCH behavior for
non-current PIDs and success for process.pid.
In `@code/core/src/cli/tools/register.ts`:
- Around line 212-227: Update printResult’s outputPath branch so the “Output
written to …” confirmation is emitted through an awaited stdout write, or
otherwise explicitly flushed, before returning to the caller that invokes
process.exit(). Preserve the existing file write and confirmation message while
ensuring it cannot be truncated on piped stdout.
In `@code/core/src/shared/open-service/toolset-names.ts`:
- Around line 24-38: Import the shared toCliMethodName helper from
toolset-names.ts into the CLI tools run and help modules, remove their local
toCliMethodName implementations, and update their call sites to use the imported
converter so command execution and help share one naming authority.
🪄 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: e432db81-d726-424a-8505-e2c4e5bf9de6
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (53)
AGENTS.mdcode/.oxlintrc.jsoncode/addons/mcp/src/constants.tscode/addons/vitest/src/node/test-run-responder.test.tscode/addons/vitest/src/node/test-run-responder.tscode/addons/vitest/src/preset.test.tscode/addons/vitest/src/preset.tscode/builders/builder-vite/src/change-detection-adapter/headless.test.tscode/builders/builder-vite/src/change-detection-adapter/headless.tscode/builders/builder-vite/src/index.tscode/core/package.jsoncode/core/src/bin/core.tscode/core/src/bin/dispatcher.tscode/core/src/cli/ai/mcp/intercepts.test.tscode/core/src/cli/ai/mcp/intercepts.tscode/core/src/cli/ai/mcp/local-metadata.test.tscode/core/src/cli/ai/mcp/local-metadata.tscode/core/src/cli/ai/mcp/register.tscode/core/src/cli/ai/mcp/run-tool.test.tscode/core/src/cli/ai/mcp/run-tool.tscode/core/src/cli/ai/mcp/types.tscode/core/src/cli/tools/bootstrap.test.tscode/core/src/cli/tools/bootstrap.tscode/core/src/cli/tools/config-dir.test.tscode/core/src/cli/tools/config-dir.tscode/core/src/cli/tools/discover-instance.tscode/core/src/cli/tools/help.tscode/core/src/cli/tools/instances/registry.test.tscode/core/src/cli/tools/instances/registry.tscode/core/src/cli/tools/instances/resolve.test.tscode/core/src/cli/tools/instances/resolve.tscode/core/src/cli/tools/instances/types.tscode/core/src/cli/tools/mcp-client.test.tscode/core/src/cli/tools/mcp-client.tscode/core/src/cli/tools/register.tscode/core/src/cli/tools/run.test.tscode/core/src/cli/tools/run.tscode/core/src/cli/tools/schema-lines.test.tscode/core/src/cli/tools/schema-lines.tscode/core/src/cli/tools/test-support/register-core-toolsets.tscode/core/src/cli/tools/tool-tokens.test.tscode/core/src/cli/tools/tool-tokens.tscode/core/src/core-server/index.tscode/core/src/core-server/load.tscode/core/src/core-server/utils/get-server-channel.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/toolset-definition.tscode/core/src/shared/open-service/toolset-names.tscode/core/src/shared/open-service/toolsets/review/definition.tscode/core/src/shared/open-service/toolsets/stories/definition.tscode/core/src/shared/open-service/toolsets/test/definition.tscode/core/src/telemetry/types.tscode/core/src/types/modules/core-common.ts
💤 Files with no reviewable changes (1)
- code/core/src/cli/ai/mcp/types.ts
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 21.78 MB | 21.83 MB | 🚨 +51 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 | 86.26 MB | 86.32 MB | 🚨 +51 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.74 MB | 84.79 MB | 🚨 +51 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.77 MB | 52.82 MB | 🚨 +51 KB 🚨 |
| Bundle Size Analyzer | node | node |
0a5ad88 to
b0ac819
Compare
b537099 to
d7c9d48
Compare
d7c9d48 to
7dce0de
Compare
28e9284 to
2a585e1
Compare
…server plumbing What the rest of the branch builds on, all of it small and declarative: - `ToolsetMethod` gains the optional `requiresDevServer` trait; `stories.preview` and `review.create` declare it (with their rationale), `test.run` explicitly does not. The CLI derives its one uniform dev-server contract from this trait alone. - `toolset-names.ts` exports `toCliMethodName` — the single authority for how a method is spelled on the CLI — and `getRef` renders CLI cross-references through it, so command names and description prose cannot disagree. - The `tools-command` telemetry event type, modeled on `ai-command`. - Core-server plumbing the CLI's own-process bootstrap needs: `experimental_loadStorybook` honors a caller-supplied channel, and `prepareHeadlessUniversalStores` returns the channel it prepared the UniversalStore singleton on. One bus from store preparation to presets is load-bearing for local test runs (review 4/6).
builder-vite implements its already-declared `changeDetectionAdapter` hook for the server-less case: the same `commonConfig` + `viteFinal` assembly the dev server uses, resolved with Vite's server-less `resolveConfig`, and a no-op watcher. This is what lets the CLI host the module graph in its own process, so `stories changed` and `stories find-by-component` work with no dev server on Vite projects. Non-Vite builders keep failing with the existing typed `OpenServiceModuleGraphUnavailableError`. The test pins the same three resolve-config fields the server-bound adapter is pinned on, so the two assemblies cannot drift apart silently.
The public, unflagged CLI, running the OSA core toolsets in its own process,
fully disconnected from any dev server:
- `run.ts` is the whole command behind the commander wiring: it derives the
surface from `getRegisteredToolsets()` at runtime, parses `--key value`
flags (JSON-coerced, `--input` escape hatch, `-o/--output`), validates
against the method's schema, and maps `ToolsetOutcome` mechanically —
markdown to stdout, `--json` prints `data`, `ok` drives the exit code,
agent-facing errors surface verbatim. The `requiresDevServer` trait renders
as one uniform contract; `stories preview` resolves its origin from the
runtime instance registry.
- `bootstrap.ts` loads the target Storybook configuration (registering every
service and toolset via the `services` hook) and hosts the module graph for
the graph-bound methods by repeating the dev server's change-detection
bootstrap against the review-2/6 adapter.
- `help.ts` renders commander's conventional shape — Usage, Options, a
Commands listing with per-command summaries and execution badges — followed
by the full per-tool reference derived from live descriptions and schemas
(valibot to JSON Schema, reusing the ai CLI's schema-line renderer, exported
rather than copied).
- `register.ts` + `bin/` wire `tools` into the program and dispatcher, with
one `tools-command` telemetry event per invocation and an explicit exit once
the result is printed (the vitest child's IPC pipe would otherwise hold the
event loop open forever).
The primary test seam is the run function: tokens in, `{ exitCode, output }`
out, against the real core toolsets with stubbed dependencies.
…vices hook `storybook tools test run` runs story tests end to end with no Storybook running, so `test.run` carries no `requiresDevServer` trait and shows as `[local]` in help. The only dev-server-bound piece was the responder answering `TRIGGER_TEST_RUN_REQUEST` over the channel, which lived exclusively in addon-vitest's `experimental_serverChannel` hook. It moves to `node/test-run-responder.ts` and is wired from the same `services` hook that registers the `test` toolset, behind the same gate — every consumer that can offer the tool can also answer it. The heavy machinery (fs cache, leader UniversalStore seeded with the story index, vitest child boot) is created lazily on first request; the dev server's channel hook runs the same memoized setup eagerly and keeps its dev-only extras, so dev behavior is unchanged. Gates: skipped inside the vitest child process (a run can never recursively boot another child); non-Vite builders get an immediate error response instead of an unanswered request. Includes the AGENTS.md section documenting the CLI-and-toolsets end state.
…e tools CLI
The tools CLI reached into `cli/ai/mcp/` for instance discovery, config-dir
resolution and the schema help renderer, so the surviving code lived inside
the tree that is slated for deletion. Relocate everything both CLIs need into
`cli/tools/` and have `storybook ai` import it from there, so removing the ai
CLI later is a pure deletion of its directory plus its registration lines.
ai/mcp/registry.ts -> tools/instances/registry.ts
ai/mcp/resolve-instance.ts -> tools/instances/resolve.ts
ai/mcp/types.ts -> split: instance records to tools/instances/types.ts,
MCP wire shapes into tools/mcp-client.ts,
JSON Schema nodes to tools/schema-lines.ts
ai/mcp/client.ts -> tools/mcp-client.ts
resolveStorybookConfigDir -> tools/config-dir.ts
schemaLines -> tools/schema-lines.ts
CommandFailureHandler -> tools/register.ts
Tests move with their modules. Behavior-neutral: no runtime change on either
CLI, and no `cli/tools` -> `cli/ai` import remains.
…er's MCP endpoint `review create` needs the running dev server's review state, which the tools CLI has no way to reach until connect mode (Milestone 5b). Until then it was the one tool in the surface that could not work at all. Forward it to that Storybook's `@storybook/addon-mcp` endpoint instead: the dispatch discovers the instance, validates the arguments locally against the method schema, calls the frozen `display-review` tool, and unwraps the reply mechanically — text to stdout, `structuredContent` for `--json`, `isError` to the exit code. The handler still runs exactly once, in the dev server, so its telemetry and side effects stay in the process that owns them. A running Storybook without the addon gets its own message naming the addon to install, rather than the generic cannot-attach one; nothing is sent until the input validates. Deliberately disposable: 5b deletes `PROXY_VIA_MCP_METHODS`, its dispatch branch and the injectable dependency, so nothing outside that branch knows a method is proxied. One consequence worth knowing: the handler executes server-side, where `ctx.consumer` is `mcp`, so a proxied method renders its MCP prose variant even on the CLI. Descriptions and `--help` are unaffected — those render locally with the CLI consumer.
7dce0de to
c56b642
Compare
|
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. |
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/cli/tools/instances/resolve.ts`:
- Around line 158-166: Update the agent selection logic around agentBuckets,
selectedAgent, and bucket so Claude and Claude-preview provenance values are
treated as a single candidate set whenever either value is currentAgent; filter
matches by both values, then apply byMostRecentlyStarted to choose the record.
Preserve existing single-agent behavior for other agents, and add
mixed-provenance recency and status coverage in resolve.test.ts.
🪄 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: 9013e181-76b2-426d-a21d-0665d034f216
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (53)
AGENTS.mdcode/.oxlintrc.jsoncode/addons/mcp/src/constants.tscode/addons/vitest/src/node/test-run-responder.test.tscode/addons/vitest/src/node/test-run-responder.tscode/addons/vitest/src/preset.test.tscode/addons/vitest/src/preset.tscode/builders/builder-vite/src/change-detection-adapter/headless.test.tscode/builders/builder-vite/src/change-detection-adapter/headless.tscode/builders/builder-vite/src/index.tscode/core/package.jsoncode/core/src/bin/core.tscode/core/src/bin/dispatcher.tscode/core/src/cli/ai/mcp/intercepts.test.tscode/core/src/cli/ai/mcp/intercepts.tscode/core/src/cli/ai/mcp/local-metadata.test.tscode/core/src/cli/ai/mcp/local-metadata.tscode/core/src/cli/ai/mcp/register.tscode/core/src/cli/ai/mcp/run-tool.test.tscode/core/src/cli/ai/mcp/run-tool.tscode/core/src/cli/ai/mcp/types.tscode/core/src/cli/tools/bootstrap.test.tscode/core/src/cli/tools/bootstrap.tscode/core/src/cli/tools/config-dir.test.tscode/core/src/cli/tools/config-dir.tscode/core/src/cli/tools/discover-instance.tscode/core/src/cli/tools/help.tscode/core/src/cli/tools/instances/registry.test.tscode/core/src/cli/tools/instances/registry.tscode/core/src/cli/tools/instances/resolve.test.tscode/core/src/cli/tools/instances/resolve.tscode/core/src/cli/tools/instances/types.tscode/core/src/cli/tools/mcp-client.test.tscode/core/src/cli/tools/mcp-client.tscode/core/src/cli/tools/register.tscode/core/src/cli/tools/run.test.tscode/core/src/cli/tools/run.tscode/core/src/cli/tools/schema-lines.test.tscode/core/src/cli/tools/schema-lines.tscode/core/src/cli/tools/test-support/register-core-toolsets.tscode/core/src/cli/tools/tool-tokens.test.tscode/core/src/cli/tools/tool-tokens.tscode/core/src/core-server/index.tscode/core/src/core-server/load.tscode/core/src/core-server/utils/get-server-channel.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/toolset-definition.tscode/core/src/shared/open-service/toolset-names.tscode/core/src/shared/open-service/toolsets/review/definition.tscode/core/src/shared/open-service/toolsets/stories/definition.tscode/core/src/shared/open-service/toolsets/test/definition.tscode/core/src/telemetry/types.tscode/core/src/types/modules/core-common.ts
💤 Files with no reviewable changes (1)
- code/core/src/cli/ai/mcp/types.ts
🚧 Files skipped from review as they are similar to previous changes (48)
- code/core/src/shared/open-service/toolsets/stories/definition.ts
- code/core/src/cli/tools/mcp-client.test.ts
- code/core/src/shared/open-service/toolsets/test/definition.ts
- code/core/src/cli/ai/mcp/run-tool.test.ts
- code/.oxlintrc.json
- code/core/src/telemetry/types.ts
- code/core/src/cli/tools/schema-lines.test.ts
- code/core/src/cli/tools/instances/resolve.test.ts
- code/core/src/shared/open-service/toolsets/review/definition.ts
- code/core/src/shared/open-service/toolset-names.ts
- code/core/src/cli/tools/discover-instance.ts
- code/core/src/shared/open-service/README.md
- code/core/src/shared/open-service/toolset-definition.ts
- code/core/src/cli/tools/config-dir.test.ts
- code/core/src/cli/tools/tool-tokens.test.ts
- code/builders/builder-vite/src/change-detection-adapter/headless.ts
- code/core/src/cli/ai/mcp/intercepts.test.ts
- code/core/src/cli/tools/instances/types.ts
- code/core/package.json
- code/core/src/cli/ai/mcp/local-metadata.test.ts
- code/core/src/cli/tools/bootstrap.test.ts
- code/builders/builder-vite/src/index.ts
- code/core/src/core-server/index.ts
- code/core/src/cli/ai/mcp/register.ts
- code/core/src/cli/tools/instances/registry.test.ts
- code/core/src/core-server/load.ts
- code/core/src/cli/tools/help.ts
- code/core/src/cli/tools/test-support/register-core-toolsets.ts
- code/core/src/types/modules/core-common.ts
- code/core/src/bin/core.ts
- code/core/src/cli/ai/mcp/intercepts.ts
- code/core/src/cli/ai/mcp/local-metadata.ts
- code/core/src/cli/tools/run.test.ts
- AGENTS.md
- code/addons/vitest/src/preset.ts
- code/builders/builder-vite/src/change-detection-adapter/headless.test.ts
- code/addons/vitest/src/node/test-run-responder.test.ts
- code/addons/vitest/src/preset.test.ts
- code/core/src/cli/tools/tool-tokens.ts
- code/core/src/cli/tools/bootstrap.ts
- code/core/src/cli/tools/run.ts
- code/addons/mcp/src/constants.ts
- code/core/src/cli/tools/config-dir.ts
- code/core/src/cli/tools/mcp-client.ts
- code/addons/vitest/src/node/test-run-responder.ts
- code/core/src/cli/tools/register.ts
- code/core/src/cli/tools/schema-lines.ts
- code/core/src/bin/dispatcher.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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/cli/tools/instances/resolve.ts`:
- Around line 158-166: Update the agent selection logic around agentBuckets,
selectedAgent, and bucket so Claude and Claude-preview provenance values are
treated as a single candidate set whenever either value is currentAgent; filter
matches by both values, then apply byMostRecentlyStarted to choose the record.
Preserve existing single-agent behavior for other agents, and add
mixed-provenance recency and status coverage in resolve.test.ts.
🪄 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: 9013e181-76b2-426d-a21d-0665d034f216
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (53)
AGENTS.mdcode/.oxlintrc.jsoncode/addons/mcp/src/constants.tscode/addons/vitest/src/node/test-run-responder.test.tscode/addons/vitest/src/node/test-run-responder.tscode/addons/vitest/src/preset.test.tscode/addons/vitest/src/preset.tscode/builders/builder-vite/src/change-detection-adapter/headless.test.tscode/builders/builder-vite/src/change-detection-adapter/headless.tscode/builders/builder-vite/src/index.tscode/core/package.jsoncode/core/src/bin/core.tscode/core/src/bin/dispatcher.tscode/core/src/cli/ai/mcp/intercepts.test.tscode/core/src/cli/ai/mcp/intercepts.tscode/core/src/cli/ai/mcp/local-metadata.test.tscode/core/src/cli/ai/mcp/local-metadata.tscode/core/src/cli/ai/mcp/register.tscode/core/src/cli/ai/mcp/run-tool.test.tscode/core/src/cli/ai/mcp/run-tool.tscode/core/src/cli/ai/mcp/types.tscode/core/src/cli/tools/bootstrap.test.tscode/core/src/cli/tools/bootstrap.tscode/core/src/cli/tools/config-dir.test.tscode/core/src/cli/tools/config-dir.tscode/core/src/cli/tools/discover-instance.tscode/core/src/cli/tools/help.tscode/core/src/cli/tools/instances/registry.test.tscode/core/src/cli/tools/instances/registry.tscode/core/src/cli/tools/instances/resolve.test.tscode/core/src/cli/tools/instances/resolve.tscode/core/src/cli/tools/instances/types.tscode/core/src/cli/tools/mcp-client.test.tscode/core/src/cli/tools/mcp-client.tscode/core/src/cli/tools/register.tscode/core/src/cli/tools/run.test.tscode/core/src/cli/tools/run.tscode/core/src/cli/tools/schema-lines.test.tscode/core/src/cli/tools/schema-lines.tscode/core/src/cli/tools/test-support/register-core-toolsets.tscode/core/src/cli/tools/tool-tokens.test.tscode/core/src/cli/tools/tool-tokens.tscode/core/src/core-server/index.tscode/core/src/core-server/load.tscode/core/src/core-server/utils/get-server-channel.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/toolset-definition.tscode/core/src/shared/open-service/toolset-names.tscode/core/src/shared/open-service/toolsets/review/definition.tscode/core/src/shared/open-service/toolsets/stories/definition.tscode/core/src/shared/open-service/toolsets/test/definition.tscode/core/src/telemetry/types.tscode/core/src/types/modules/core-common.ts
💤 Files with no reviewable changes (1)
- code/core/src/cli/ai/mcp/types.ts
🚧 Files skipped from review as they are similar to previous changes (48)
- code/core/src/shared/open-service/toolsets/stories/definition.ts
- code/core/src/cli/tools/mcp-client.test.ts
- code/core/src/shared/open-service/toolsets/test/definition.ts
- code/core/src/cli/ai/mcp/run-tool.test.ts
- code/.oxlintrc.json
- code/core/src/telemetry/types.ts
- code/core/src/cli/tools/schema-lines.test.ts
- code/core/src/cli/tools/instances/resolve.test.ts
- code/core/src/shared/open-service/toolsets/review/definition.ts
- code/core/src/shared/open-service/toolset-names.ts
- code/core/src/cli/tools/discover-instance.ts
- code/core/src/shared/open-service/README.md
- code/core/src/shared/open-service/toolset-definition.ts
- code/core/src/cli/tools/config-dir.test.ts
- code/core/src/cli/tools/tool-tokens.test.ts
- code/builders/builder-vite/src/change-detection-adapter/headless.ts
- code/core/src/cli/ai/mcp/intercepts.test.ts
- code/core/src/cli/tools/instances/types.ts
- code/core/package.json
- code/core/src/cli/ai/mcp/local-metadata.test.ts
- code/core/src/cli/tools/bootstrap.test.ts
- code/builders/builder-vite/src/index.ts
- code/core/src/core-server/index.ts
- code/core/src/cli/ai/mcp/register.ts
- code/core/src/cli/tools/instances/registry.test.ts
- code/core/src/core-server/load.ts
- code/core/src/cli/tools/help.ts
- code/core/src/cli/tools/test-support/register-core-toolsets.ts
- code/core/src/types/modules/core-common.ts
- code/core/src/bin/core.ts
- code/core/src/cli/ai/mcp/intercepts.ts
- code/core/src/cli/ai/mcp/local-metadata.ts
- code/core/src/cli/tools/run.test.ts
- AGENTS.md
- code/addons/vitest/src/preset.ts
- code/builders/builder-vite/src/change-detection-adapter/headless.test.ts
- code/addons/vitest/src/node/test-run-responder.test.ts
- code/addons/vitest/src/preset.test.ts
- code/core/src/cli/tools/tool-tokens.ts
- code/core/src/cli/tools/bootstrap.ts
- code/core/src/cli/tools/run.ts
- code/addons/mcp/src/constants.ts
- code/core/src/cli/tools/config-dir.ts
- code/core/src/cli/tools/mcp-client.ts
- code/addons/vitest/src/node/test-run-responder.ts
- code/core/src/cli/tools/register.ts
- code/core/src/cli/tools/schema-lines.ts
- code/core/src/bin/dispatcher.ts
🛑 Comments failed to post (1)
code/core/src/cli/tools/instances/resolve.ts (1)
158-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat Claude provenance values as one candidate bucket.
When both
claude-previewandclauderecords match, this code selects theclaude-previewbucket before it considersclaude. A stale preview record can override a newerclauderecord or returnmcp-startingwhen a readyclauderecord exists.Filter by both provenance values when either value matches, then apply
byMostRecentlyStarted. Add mixed-provenance recency and status cases toresolve.test.ts.Proposed fix
- const selectedAgent = agentBuckets.find((agent) => matches.some((r) => r.agent === agent)); - const bucket = selectedAgent ? matches.filter((r) => r.agent === selectedAgent) : matches; + const hasAgentMatch = matches.some((r) => agentBuckets.includes(r.agent)); + const bucket = hasAgentMatch + ? matches.filter((r) => agentBuckets.includes(r.agent)) + : matches;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// std-env reports Claude CLI as `claude`; preview-launched Storybooks record `claude-preview`. const agentBuckets = currentAgent === CLAUDE_AGENT_NAME ? [CLAUDE_PREVIEW_AGENT_NAME, CLAUDE_AGENT_NAME] : currentAgent ? [currentAgent] : []; const hasAgentMatch = matches.some((r) => agentBuckets.includes(r.agent)); const bucket = hasAgentMatch ? matches.filter((r) => agentBuckets.includes(r.agent)) : matches;🤖 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/cli/tools/instances/resolve.ts` around lines 158 - 166, Update the agent selection logic around agentBuckets, selectedAgent, and bucket so Claude and Claude-preview provenance values are treated as a single candidate set whenever either value is currentAgent; filter matches by both values, then apply byMostRecentlyStarted to choose the record. Preserve existing single-agent behavior for other agents, and add mixed-provenance recency and status coverage in resolve.test.ts.
Replaces #35719 after stack base retargeting from the accidental #35677 merge. Same commits on
m5a-tools-cli-osa; base restored tom4-addon-mcp-core-services(#35849).Closes #35716 · Closes #35724 · Closes #35747
Note
Stacked on #35677 (which stacks on #35726). This PR targets
m4-addon-mcp-core-services, so its diff is the CLI work only — 54 files, +3,384 −467. Review #35726 (the toolset layer) and #35677 (the MCP adapters) first; this PR is the third consumer of that same layer. Merge order: #35726 → #35677 → this one; GitHub retargets automatically.What I did
storybook tools— a public, unflagged CLI whose entire command surface is derived at runtime from the core toolsets, running in its own process, with no dev server. This is whatnpx storybook tools --helpprints in a react-vite sandbox:Not one of these commands is defined in the CLI. It loads the target project's Storybook configuration (
experimental_loadStorybook), which applies theservicespreset and registers every toolset in-process; commands, help text, schemas and summaries all come fromgetRegisteredToolsets()at runtime. Install addon-vitest andtest runappears; edit a description in a toolset and the help changes. The CLI's own job is mechanical:"Own process, no dev server" required solving three real problems, and each is its own commit:
stories changed,stories find-by-component): builder-vite implements its already-declaredchangeDetectionAdapterhook for the server-less case, and the CLI repeats the dev server's change-detection bootstrap against it.test run): the responder answering test-run requests moves out of addon-vitest's dev-server-only channel hook onto the sameserviceshook that registers thetesttoolset — every consumer that can offer the tool can now answer it.review create) genuinely needs the dev server's state. It is the one exception: the CLI forwards it to the running Storybook's@storybook/addon-mcpendpoint — the same mechanismstorybook aiuses — as an explicitly disposable stopgap until connect mode (Milestone 5b) exists.(
stories previewneeds no exception: it only needs a live origin, which the runtime instance registry provides.)Along the way, the modules both CLIs share (instance registry reader, instance matcher, MCP client, config-dir resolution, schema help renderer) move out of
cli/ai/intocli/tools/, becausestorybook aiis slated for removal and living code must not sit in a doomed tree. After this PR, deletingstorybook aiis a pure deletion of its directory plus two registration lines — and a lint rule fails any futurecli/tools→cli/aiimport.The one review question
Is the CLI a consumer with zero opinions — or does anything in
cli/tools/re-derive meaning that belongs in a toolset definition? If you find dispatch branching on domain data, help rendering prose a definition should own, or proxy-awareness leaking outside its one branch — that is the review finding to raise. Everything else should read as parsing, lookup, and mechanical mapping.How to review (~1½ hours)
The branch's six commits are six blocks, in dependency order — review one commit at a time; each heading links to that commit's diff.
storybook toolscommand — 40 minstorybook ai— 10 minreview createproxy — 10 minBlock 1 · The contract — 10 min
Files:
toolset-definition.ts,toolset-names.ts, the three definition annotations, telemetry types,core-server/load.ts(11 files, +76 −12)Everything the branch builds on, in ~90 lines: the optional
requiresDevServertrait onToolsetMethod(declared bystories.previewandreview.create; deliberately not bytest.run);toCliMethodNameas the single authority for how a method is spelled on the CLI (getRefrenders cross-references through it, so prose and dispatch cannot disagree); thetools-commandtelemetry event; andexperimental_loadStorybookhonoring a caller-supplied channel.What to check: is the trait on the right methods, and is one boolean enough to express "needs a dev server"? The channel parameter looks innocent but is load-bearing for block 4.
Block 2 · The headless module graph — 10 min
Files:
builder-vite/src/change-detection-adapter/headless.ts(+ test),builder-vite/src/index.ts(3 files, +129 −10)headless.tsis 38 lines: the samecommonConfig+viteFinalassembly the dev server uses, resolved with Vite's server-lessresolveConfig, and a no-op watcher. Non-Vite builders keep failing with the existing typed error.What to check: the test pins the same three resolved-config fields as the server-bound adapter, so the two assemblies cannot drift silently.
Block 3 · The
storybook toolscommand — 40 minFiles:
cli/tools/{run,help,bootstrap,register,tool-tokens,discover-instance}.ts+ tests +bin/wiring (15 files, +1,975 −10)The heart of the PR. Read
run.ts(355 lines — the whole command behind the commander wiring) andhelp.ts(209 lines) in full.run.test.tsdoubles as the behavior spec: tokens in,{ exitCode, output }out, against the real core toolsets with stubbed dependencies — including a parity test assertingdocs listprints byte-for-byte what an MCP client receives for the same call.Two deliberate oddities, so they don't read as bugs:
What to check: the one review question, applied line by line. Telemetry: one
tools-commandevent per invocation,helplookups excluded so they cannot skew success rates.Block 4 · Local story tests — 15 min
Files:
addons/vitest/src/node/test-run-responder.ts(+ test),preset.ts(+ test),AGENTS.md(5 files, +637 −185)The responder was the only dev-server-bound piece of
test run. It moves tonode/test-run-responder.ts, wired from the sameserviceshook that registers thetesttoolset, behind the same gate. Heavy machinery (fs cache, leader UniversalStore, vitest child) is created lazily on first request; the dev server's channel hook runs the same memoized setup eagerly and keeps its dev-only extras, so dev behavior is unchanged.What to check: the gates — skipped inside the vitest child (a run can never recursively boot another child); non-Vite builders get an immediate error response instead of an unanswered request. And the channel invariant: the responder relays the vitest child's store events onto the channel its preset hook received — leader stores only hear the channel they were prepared on, so a second channel on either side hangs the run silently (that is what block 1's channel parameter prevents).
Block 5 · The relocation out of
storybook ai— 10 minFiles:
cli/tools/{instances/,mcp-client,config-dir,schema-lines}.ts+ moved tests, thecli/ai/import rewires, the lint rule (27 files, +392 −265)Behavior-neutral by construction:
git mvplus import rewrites, no logic changes.cli/ai/mcp/types.tsis deleted, split across the new homes;schema-lines.tsgains the unit tests it never had (its only coverage used to live in the ai CLI's suite — the tree slated for deletion).What to check: skim for a logic change hiding among the moves (there are none); the new
no-restricted-importsrule incode/.oxlintrc.jsonthat fails anycli/tools→cli/aiimport — dependency direction is dying-code-depends-on-living-code, never the reverse.Block 6 · The
review createproxy — 10 minFiles:
run.ts,mcp-client.ts,run.test.ts,AGENTS.md(4 files, +193 −3)One self-contained branch in the dispatch:
The handler runs exactly once, in the dev server, so its telemetry and side effects stay in the process that owns them. Milestone 5b deletes the set, the branch, and the injectable dependency wholesale.
What to check: disposability — nothing outside the branch may know a method is proxied. Five seam tests pin the contract (frozen name + args,
--json,isError→ exit 1, validation before any send, missing-endpoint guidance).Known limitation, on purpose: the proxied handler executes server-side where
ctx.consumerismcp, soreview createprints its MCP prose variant on the CLI (descriptions and--helprender locally and are unaffected). Fixing it means mapping the header to acliconsumer in addon-mcp, which would also change whatstorybook aiand the Claude/Codex plugins receive — a separate call, and 5b removes the mismatch entirely. Noted on #35747.What you do not need to review, and why that is safe
docs listparity test asserts byte-equality with the handler markdown the MCP adapter renders verbatimtest run247/247 matching the manager UI,review createpublishing end-to-end with the review page visually verified (including the pending-review → Update flow), unknown-ID refusal, missing-addon guidance, no-dev-server contractstorybook aiis unchangedai display-review,ai list-all-documentation) behind its env flagOut of scope (per #35747): connect mode (5b), deprecating
storybook ai(a later PR — this PR only makes that deletion pure), proxying any method beyondreview.create, mapping the trusted-local-client header to acliconsumer.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn nx run-many -t compile, then in any project (a sandbox works):npx storybook tools— the Commands listing and full tool reference print from that project's configuration, with[local]/[requires running Storybook]badges.npx storybook tools docs listandnpx storybook tools stories changed— both work with no dev server (docs from manifests, the module graph hosted in-process).npx storybook tools test run --stories '[{"storyId":"example-button--primary"}]'— addon-vitest boots its vitest child, the report prints, the process exits with the mapped code.storybook dev(with@storybook/addon-mcp):npx storybook tools review create --input '<review object>'publishes the review; the page renders it at/?path=/review/;--jsonprints{ "reviewUrl": … }. Without the addon, the command names it and exits 1. Without a dev server, the uniform "start the dev server first" message.STORYBOOK_FEATURE_AI_CLI=true npx storybook ai list-all-documentation— the ai CLI still works identically on the relocated modules.Documentation
--helpis the CLI's documentation until the surface stabilizes, per the spec's out-of-scope list)Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Made with Cursor