Conversation
Thread runner command through classifyError and classifyHttpError to replace hardcoded 'npx @cipherstash/cli auth login' strings with detected package manager (bunx, pnpm dlx, yarn dlx, npx). Update call sites: - agent/interface.ts: derive runner from session.detectedPackageManager (3 calls) - agent/fetch-prompt.ts: accept optional runner param, default 'npx' (1 call) - run.ts: pass packageManager.execCommand to fetchIntegrationPrompt All callers now get runner-aware auth failure messages per detected PM. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: f34fe9d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR extends package manager detection throughout the CLI initialization and wizard flows. It introduces a Changes
Sequence Diagram(s)This change does not introduce new feature flows or significantly alter control flow between multiple components—it primarily refactors command string generation to be parameterized by package manager. The interactions remain the same; only the command format changes based on detected manager. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 2 minutes and 31 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/init/providers/__tests__/supabase.test.ts`:
- Around line 23-35: Test suite is missing a Yarn case for the Supabase
provider: add a new test that calls provider.getNextSteps({}, 'yarn') and assert
the first step contains 'yarn dlx `@cipherstash/cli` db install --supabase'
(mirroring the pnpm case) and also ensure the returned steps still include the
raw Supabase CLI commands like 'supabase db reset' and 'supabase migration up';
implement this as a new it(...) block alongside the existing pnpm/bun tests
referencing provider.getNextSteps and the provider test variable.
In `@packages/cli/src/commands/init/utils.ts`:
- Around line 128-138: The runnerCommand currently returns "yarn dlx" for any
'yarn' value which breaks Yarn Classic (1.x); update the package manager
detection (packageManagerFromUserAgent() and detectPackageManager()) to
differentiate Yarn Classic vs Yarn Berry (e.g., return distinct token like
'yarn-classic' or include a version field) and then modify runnerCommand(pm,
ref) to handle the new Yarn-classic token by returning `npx ${ref}` while
keeping `yarn dlx ${ref}` for Yarn 2+ (and preserving existing cases for 'bun',
'pnpm', 'npm'). Ensure all call sites that expect 'yarn' are updated to handle
the new discriminator.
In `@packages/wizard/src/__tests__/post-agent.test.ts`:
- Around line 46-64: The test name asserts drizzle generation but the call to
runPostAgentSteps uses integration: 'supabase', so the drizzle branch is never
exercised; either change the test to reflect it verifies db push (rename the it
description to mention bunx db push) or update the test to use integration:
'drizzle' and ensure the interactive confirm is mocked/stubbed so the drizzle
path runs (mock the prompt used by runPostAgentSteps or the confirm function it
invokes) before asserting that the captured childProcess.execSync commands
include the `${runner} drizzle-kit generate` invocation.
In `@packages/wizard/src/agent/interface.ts`:
- Around line 351-353: The handler currently logging raw agent stderr via
p.log.warn(`[agent stderr] ${data.trim()}`) must be changed to avoid emitting
plaintext; replace the direct content log with metadata-only logging (e.g., log
the byte/character length, a redaction placeholder, and any relevant flags or
exit codes) in the stderr handler (the anonymous function assigned to ? (data:
string) => {...}) and apply the same redaction/metadata approach to the other
result/output logging code referenced around the other block (the result/agent
output logging at 485-501). Ensure you keep p.log.warn (or the existing logger
call) but pass only non-plaintext metadata such as length, stop reason, and
error code, not the raw data itself.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 318c7440-227e-4fd5-b1c3-b8164818697e
📒 Files selected for processing (22)
packages/cli/src/commands/init/__tests__/utils.test.tspackages/cli/src/commands/init/providers/__tests__/base.test.tspackages/cli/src/commands/init/providers/__tests__/drizzle.test.tspackages/cli/src/commands/init/providers/__tests__/supabase.test.tspackages/cli/src/commands/init/providers/base.tspackages/cli/src/commands/init/providers/drizzle.tspackages/cli/src/commands/init/providers/supabase.tspackages/cli/src/commands/init/steps/next-steps.tspackages/cli/src/commands/init/types.tspackages/cli/src/commands/init/utils.tspackages/wizard/src/__tests__/detect.test.tspackages/wizard/src/__tests__/errors-runner.test.tspackages/wizard/src/__tests__/post-agent.test.tspackages/wizard/src/__tests__/prerequisites.test.tspackages/wizard/src/agent/errors.tspackages/wizard/src/agent/fetch-prompt.tspackages/wizard/src/agent/interface.tspackages/wizard/src/lib/detect.tspackages/wizard/src/lib/post-agent.tspackages/wizard/src/lib/prerequisites.tspackages/wizard/src/lib/types.tspackages/wizard/src/run.ts
| export function runnerCommand(pm: PackageManager, ref: string): string { | ||
| switch (pm) { | ||
| case 'bun': | ||
| return `bunx ${ref}` | ||
| case 'pnpm': | ||
| return `pnpm dlx ${ref}` | ||
| case 'yarn': | ||
| return `yarn dlx ${ref}` | ||
| case 'npm': | ||
| return `npx ${ref}` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Yarn Classic (1.x) support yarn dlx? If not, what is the recommended one-off package execution command for Yarn 1.x compared to Yarn 2+?
💡 Result:
No. Yarn Classic (1.x) does not support the yarn dlx command. (In Yarn 2+ / Berry, yarn dlx is the “download and execute” command for one-off package execution.) For Yarn 1.x, the recommended one-off package execution is to use npx ... (the closest equivalent to Yarn 2+ yarn dlx, i.e., running a package without permanently installing it). Yarn 1.x’s built-in “local one-off” execution is via yarn run <script> or yarn exec , but those don’t provide the same “temporary install + run remote binary” behavior as yarn dlx.
Citations:
- 1: https://classic.yarnpkg.com/en/package/dlx
- 2: https://yarnpkg.com/cli/dlx
- 3: https://v2.yarnpkg.com/getting-started/migration
- 4: https://classic.yarnpkg.com/lang/en/docs/cli/run/
🏁 Script executed:
# Find and examine the detectPackageManager and packageManagerFromUserAgent functions
rg -A 15 "function detectPackageManager|function packageManagerFromUserAgent" packages/cli/src --type tsRepository: cipherstash/stack
Length of output: 2460
🏁 Script executed:
# Also search for where runnerCommand is used to understand impact
rg -B 2 -A 2 "runnerCommand" packages/cli/src --type tsRepository: cipherstash/stack
Length of output: 6210
🏁 Script executed:
# Check the type definition for PackageManager
rg -B 2 -A 2 "type PackageManager|PackageManager =" packages/cli/src --type tsRepository: cipherstash/stack
Length of output: 1496
🏁 Script executed:
# Check if there's any Yarn version detection elsewhere
rg "yarn.*1\.|yarn.*2\.|yarn.*berry|classic" packages/cli/src --type ts -iRepository: cipherstash/stack
Length of output: 43
runnerCommand generates yarn dlx for all Yarn versions, which fails on Yarn Classic (1.x).
The detection code (packageManagerFromUserAgent() and detectPackageManager()) returns a generic 'yarn' for all Yarn versions without distinguishing between 1.x and 2+. Since Yarn Classic does not support yarn dlx, generated next-step instructions will contain invalid commands for Yarn 1.x projects. This directly affects users' experience during init.
Update detection to distinguish Yarn Classic from Yarn 2+ Berry, and use npx as a fallback for Yarn 1.x.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/init/utils.ts` around lines 128 - 138, The
runnerCommand currently returns "yarn dlx" for any 'yarn' value which breaks
Yarn Classic (1.x); update the package manager detection
(packageManagerFromUserAgent() and detectPackageManager()) to differentiate Yarn
Classic vs Yarn Berry (e.g., return distinct token like 'yarn-classic' or
include a version field) and then modify runnerCommand(pm, ref) to handle the
new Yarn-classic token by returning `npx ${ref}` while keeping `yarn dlx ${ref}`
for Yarn 2+ (and preserving existing cases for 'bun', 'pnpm', 'npm'). Ensure all
call sites that expect 'yarn' are updated to handle the new discriminator.
| ? (data: string) => { | ||
| p.log.warn(`[agent stderr] ${data.trim()}`) | ||
| } |
There was a problem hiding this comment.
Avoid logging raw agent stderr/output content in debug mode
These debug logs emit plaintext from stderr and agent/result fields, which can contain sensitive values. Please redact content and log metadata only (lengths, flags, stop reason, error code).
As per coding guidelines: "Do not log plaintext; the library never logs plaintext by design and logs must not risk leaking sensitive data".
Also applies to: 485-501
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/wizard/src/agent/interface.ts` around lines 351 - 353, The handler
currently logging raw agent stderr via p.log.warn(`[agent stderr]
${data.trim()}`) must be changed to avoid emitting plaintext; replace the direct
content log with metadata-only logging (e.g., log the byte/character length, a
redaction placeholder, and any relevant flags or exit codes) in the stderr
handler (the anonymous function assigned to ? (data: string) => {...}) and apply
the same redaction/metadata approach to the other result/output logging code
referenced around the other block (the result/agent output logging at 485-501).
Ensure you keep p.log.warn (or the existing logger call) but pass only
non-plaintext metadata such as length, stop reason, and error code, not the raw
data itself.
Bootstraps a top-level `e2e/` workspace and ports the verification logic from the `verify-package-managers.sh` proof-of-concept into a vitest suite. Two suites: - CLI init providers: imports the production source and asserts each PM yields the right runner (`npx`/`bunx`/`pnpm dlx`/`yarn dlx`) in the rendered "Next Steps" copy. 12 cases. - Wizard binary: spawns the BUILT wizard binary in throwaway sandbox dirs (lockfile + npm_config_user_agent variations) and asserts the prerequisite "Run: ..." line uses the detected runner. 9 cases. Skipped when no auth is configured locally and no `CS_*` env vars are present in the runner environment. Adds a `test:e2e` script at the repo root that delegates to turbo, and a standalone `e2e-tests` job in `.github/workflows/tests.yml` that exposes the auth secrets so the wizard suite stays live in CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add the missing yarn case to supabase provider tests (mirrors the existing pnpm/bun cases that drizzle/base already had). - Rename the misleading post-agent test that claimed to verify drizzle-kit but actually used integration='supabase'. The test does exercise a useful path (hasStashConfig=true short-circuits db install while db push still runs with bunx), so the body is preserved with an honest name and an extra negative assertion that db install does NOT run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Make the
initand wizard print and execute commands under the correct package manager.When a user runs
bunx @cipherstash/cli init, they should should see "Next Steps" and wizard messages referencebunx/pnpm dlx, not alwaysnpx.The wizard's
db install/db pushcalls should also actually shell out to the right runner.Summary by CodeRabbit
New Features
Tests