CLI: Fix agentic check#34678
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA new test case validates that Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches📝 Generate docstrings
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/telemetry/ai-setup-utils.ts (1)
92-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid emitting raw pending telemetry context on normal log level.
Line 107 logs the full
pendingrecord, and multiplelogger.logcalls (Line 92, Line 98, Line 106, Line 115, Line 125, Line 130, Line 140-141) will print in standard CLI output. This is noisy and risks exposing session/file-path context.Suggested tightening
- logger.log('collectAiSetupEvidence'); + logger.debug('collectAiSetupEvidence'); @@ - logger.log('collectAiSetupEvidence - yes agent'); + logger.debug('collectAiSetupEvidence - agent detected'); @@ - logger.log('collectAiSetupEvidence - no pending'); - logger.log(JSON.stringify({ configDir, pending }, null, 2)); + logger.debug('collectAiSetupEvidence - pending record found'); @@ - logger.log(`timeSinceSetup: ${timeSinceSetup}`); + logger.debug(`collectAiSetupEvidence - timeSinceSetup=${timeSinceSetup}`); @@ - logger.log(`eventType: ${eventType}`); + logger.debug(`collectAiSetupEvidence - eventType=${eventType}`); @@ - logger.log(`calling event`); + logger.debug('collectAiSetupEvidence - firing telemetry event'); @@ - logger.log(`event data`); - logger.log(JSON.stringify({ previewChanged, aiAuthoredStories, timeSinceSetup }, null, 2)); + logger.debug( + `collectAiSetupEvidence - event data previewChanged=${previewChanged} aiAuthoredStories=${aiAuthoredStories ?? 'undefined'}` + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/telemetry/ai-setup-utils.ts` around lines 92 - 141, The current collect/telemetry flow logs sensitive and noisy details at normal log level (multiple logger.log calls and a full JSON.stringify of pending), so update the function to stop emitting raw pending/session/configDir data to CLI: remove or change logger.log(JSON.stringify({ configDir, pending })) and other noisy logger.log calls to a lower-severity/debug/trace channel (e.g., logger.debug or logger.trace) or redact file paths and timestamps before logging; keep only minimal non-sensitive indicators (e.g., "agent detected", "pending present", "session expired") and ensure checks around detectAgent(), getAiSetupPending(), SESSION_TIMEOUT, checkPreviewChanged, and the telemetry('ai-setup-evidence', ...) block no longer print full pending or configDir values to standard output.
🧹 Nitpick comments (1)
code/core/src/telemetry/ai-setup-utils.test.ts (1)
240-245: ⚡ Quick winMove the new mock behavior setup out of the test body.
This block introduces inline mock implementations in the
itcase. Please move these into abeforeEachin a scopeddescribeto match repository Vitest mocking rules.As per coding guidelines: "Implement mock behaviors in
beforeEachblocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/telemetry/ai-setup-utils.test.ts` around lines 240 - 245, The inline mock setup inside the failing it case (calls to vi.mocked(detectAgent).mockReturnValue, vi.mocked(getAiSetupPending).mockResolvedValue with makePendingRecord using absoluteConfigDir, and vi.mocked(findConfigFile).mockReturnValue) should be moved into a beforeEach within a surrounding describe block that scopes this test; create a describe(...) around the test, add a beforeEach that sets those vi.mocked(...) behaviors (using the same values including absoluteConfigDir and makePendingRecord) and remove the inline mock calls from the it body so Vitest mock rules are followed and the test uses the shared beforeEach setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/telemetry/ai-setup-utils.test.ts`:
- Around line 241-243: The test constructs absoluteConfigDir with string
concatenation which yields mixed path separators on Windows; update the test to
build absoluteConfigDir using path resolution (e.g., call resolve(process.cwd(),
'.storybook') or path.resolve(process.cwd(), '.storybook')) before passing it to
makePendingRecord so it matches the code under test that normalizes with
resolve(configDir). Change the absoluteConfigDir variable in the snippet
referencing getAiSetupPending and makePendingRecord accordingly.
---
Outside diff comments:
In `@code/core/src/telemetry/ai-setup-utils.ts`:
- Around line 92-141: The current collect/telemetry flow logs sensitive and
noisy details at normal log level (multiple logger.log calls and a full
JSON.stringify of pending), so update the function to stop emitting raw
pending/session/configDir data to CLI: remove or change
logger.log(JSON.stringify({ configDir, pending })) and other noisy logger.log
calls to a lower-severity/debug/trace channel (e.g., logger.debug or
logger.trace) or redact file paths and timestamps before logging; keep only
minimal non-sensitive indicators (e.g., "agent detected", "pending present",
"session expired") and ensure checks around detectAgent(), getAiSetupPending(),
SESSION_TIMEOUT, checkPreviewChanged, and the telemetry('ai-setup-evidence',
...) block no longer print full pending or configDir values to standard output.
---
Nitpick comments:
In `@code/core/src/telemetry/ai-setup-utils.test.ts`:
- Around line 240-245: The inline mock setup inside the failing it case (calls
to vi.mocked(detectAgent).mockReturnValue,
vi.mocked(getAiSetupPending).mockResolvedValue with makePendingRecord using
absoluteConfigDir, and vi.mocked(findConfigFile).mockReturnValue) should be
moved into a beforeEach within a surrounding describe block that scopes this
test; create a describe(...) around the test, add a beforeEach that sets those
vi.mocked(...) behaviors (using the same values including absoluteConfigDir and
makePendingRecord) and remove the inline mock calls from the it body so Vitest
mock rules are followed and the test uses the shared beforeEach setup.
🪄 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: a4dce0da-fa21-43c3-852b-bb251b59ea75
📒 Files selected for processing (2)
code/core/src/telemetry/ai-setup-utils.test.tscode/core/src/telemetry/ai-setup-utils.ts
Contributor
|
Failed to publish canary version of this pull request, triggered by @yannbf. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/25203705813 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/telemetry/ai-setup-utils.ts (1)
92-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the user-visible evidence logging from this path.
These
logger.log()calls run on a normal agent flow and print the full pending record plus event payload details. That leaks internal telemetry/session fields and absolute paths into terminal or CI logs, and it will also clutter the CLI output for every matched run. Please either delete these logs or downgrade them to a single redactedlogger.debug()/logger.trace()message.Suggested cleanup
- logger.log('collectAiSetupEvidence'); // Gate 1: Is this an agent? (cheapest check) const agent = detectAgent(); if (!agent) { return; } - logger.log('collectAiSetupEvidence - yes agent'); // Gate 2: Is there a pending ai-setup record? const pending = await getAiSetupPending(); if (!pending) { return; } - logger.log('collectAiSetupEvidence - no pending'); - logger.log(JSON.stringify({ configDir, pending }, null, 2)); // Gate 3: Does the configDir match? (cross-project guard) if (configDir && pending.configDir !== resolve(configDir)) { return; } // Gate 4: Is it within the session window? const timeSinceSetup = Date.now() - pending.timestamp; - logger.log(`timeSinceSetup: ${timeSinceSetup}`); if (timeSinceSetup > SESSION_TIMEOUT) { await flushAiSetupPending(); return; } - logger.log(`eventType: ${eventType}`); if (eventType === 'ai-setup') { return; } - logger.log(`calling event`); await telemetry( 'ai-setup-evidence', async () => { const previewChanged = await checkPreviewChanged(pending.configDir, pending); const aiAuthoredStories = storyIndex ? countAiAuthoredStories(storyIndex) : undefined; - logger.log(`event data`); - logger.log(JSON.stringify({ previewChanged, aiAuthoredStories, timeSinceSetup }, null, 2)); return { previewChanged, aiAuthoredStories, sessionId: pending.sessionId, timeSinceSetup, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/telemetry/ai-setup-utils.ts` around lines 92 - 141, The current collect-ai-setup flow emits multiple user-visible logger.log() calls that leak sensitive session/telemetry fields and absolute paths (e.g., the JSON.stringify of { configDir, pending } and event payloads); locate the logging calls inside the ai-setup evidence path (around detectAgent(), getAiSetupPending(), the SESSION_TIMEOUT check, and before telemetry() where previewChanged/aiAuthoredStories/timeSinceSetup are logged) and remove or downgrade them to non-user-facing logs (use logger.debug() or logger.trace()), and when logging pending or configDir ensure you redact or omit sensitive fields (e.g., do not print pending.raw/session fields or absolute paths—use a redacted summary instead) so only safe, minimal diagnostic info is emitted.
♻️ Duplicate comments (1)
code/core/src/telemetry/ai-setup-utils.test.ts (1)
236-243:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBuild the absolute config path with
resolve()instead of string concatenation.
`${process.cwd()}/.storybook`produces mixed separators on Windows, while the implementation now compares againstresolve(configDir). That makes this regression test flaky cross-platform.Cross-platform fix
+import { resolve } from 'node:path'; @@ - const absoluteConfigDir = `${process.cwd()}/.storybook`; + const absoluteConfigDir = resolve('.storybook');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/telemetry/ai-setup-utils.test.ts` around lines 236 - 243, The test builds an absolute config path using string concatenation which is OS-dependent; change the absoluteConfigDir assignment in the test (the variable used with makePendingRecord) to use path.resolve instead (e.g., resolve(process.cwd(), '.storybook')) and add an import for resolve from 'path' at the top of the test file so the path separators are correct cross-platform; keep the rest of the test (detectAgent, getAiSetupPending, makePendingRecord) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/lib/create-storybook/src/initiate.ts`:
- Around line 207-212: The callback is re-enabling telemetry after
withTelemetry() has already determined the correct state; remove or guard the
explicit await setTelemetryEnabled(!options.disableTelemetry) so it doesn't
override an existing project setting (which withTelemetry() already honors via
core.disableTelemetry/main.ts). Fix by deleting the await
setTelemetryEnabled(...) call in the async callback (or wrap it with a check
against the resolved telemetry state from withTelemetry() so it only runs when
no prior config exists), referencing the withTelemetry() invocation and the
setTelemetryEnabled and options.disableTelemetry symbols to locate and update
the code.
---
Outside diff comments:
In `@code/core/src/telemetry/ai-setup-utils.ts`:
- Around line 92-141: The current collect-ai-setup flow emits multiple
user-visible logger.log() calls that leak sensitive session/telemetry fields and
absolute paths (e.g., the JSON.stringify of { configDir, pending } and event
payloads); locate the logging calls inside the ai-setup evidence path (around
detectAgent(), getAiSetupPending(), the SESSION_TIMEOUT check, and before
telemetry() where previewChanged/aiAuthoredStories/timeSinceSetup are logged)
and remove or downgrade them to non-user-facing logs (use logger.debug() or
logger.trace()), and when logging pending or configDir ensure you redact or omit
sensitive fields (e.g., do not print pending.raw/session fields or absolute
paths—use a redacted summary instead) so only safe, minimal diagnostic info is
emitted.
---
Duplicate comments:
In `@code/core/src/telemetry/ai-setup-utils.test.ts`:
- Around line 236-243: The test builds an absolute config path using string
concatenation which is OS-dependent; change the absoluteConfigDir assignment in
the test (the variable used with makePendingRecord) to use path.resolve instead
(e.g., resolve(process.cwd(), '.storybook')) and add an import for resolve from
'path' at the top of the test file so the path separators are correct
cross-platform; keep the rest of the test (detectAgent, getAiSetupPending,
makePendingRecord) 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: 0d791bc6-81b6-43e4-918a-be675949df18
📒 Files selected for processing (8)
code/core/src/core-server/build-dev.tscode/core/src/core-server/build-static.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/withTelemetry.test.tscode/core/src/core-server/withTelemetry.tscode/core/src/telemetry/ai-setup-utils.test.tscode/core/src/telemetry/ai-setup-utils.tscode/lib/create-storybook/src/initiate.ts
This reverts commit e248e58.
…into yann/ai-steps-fix
JReinhold
approved these changes
May 1, 2026
3 tasks
This was referenced May 4, 2026
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.
Closes #
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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.tsMake 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 pull request has been released as version
0.0.0-pr-34678-sha-e248e58e. Try it out in a new sandbox by runningnpx storybook@0.0.0-pr-34678-sha-e248e58e sandboxor in an existing project withnpx storybook@0.0.0-pr-34678-sha-e248e58e upgrade.More information
0.0.0-pr-34678-sha-e248e58eyann/ai-steps-fixe248e58e1777613564)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=34678Summary by CodeRabbit
Tests
Bug Fixes