fix(cli): runner-aware help banners + wizard reference#384
Merged
Conversation
🦋 Changeset detectedLatest commit: 0560049 The changes in this PR will be included in the next version bump. 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 |
📝 WalkthroughWalkthroughThis PR makes CLI help banners and "Next steps" output dynamically use the package manager runner the user invoked (bunx, pnpm dlx, yarn dlx, or npx fallback) instead of hardcoding npx. Changes span message constants, detection/rendering logic, and integration points across help text and wizard output. ChangesRunner-Aware Help & Next Steps
Sequence DiagramsequenceDiagram
participant User as User
participant CLI as CLI Entry
participant Detect as Package Manager<br/>Detection
participant Render as Help Renderer
participant Output as Terminal Output
User->>CLI: bunx `@cipherstash/cli` --help<br/>(npm_config_user_agent=bunx)
CLI->>Detect: detectPackageManager()
Detect->>Detect: Check npm_config_user_agent
Detect-->>CLI: 'bunx'
CLI->>CLI: Compute runnerCommand('bunx', 'stash')
CLI->>Render: Build help with 'bunx stash'
Render->>Render: Inject 'bunx' into Usage:<br/>and Examples
Render-->>Output: Usage: bunx stash ...<br/>Examples: bunx stash init ...
Output-->>User: Display runner-aware help
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 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 60 minutes.Comment |
coderdan
force-pushed
the
dan/runner-aware-help-banners
branch
from
3a708f9 to
03d845e
Compare
tobyhede
approved these changes
May 4, 2026
Follow-up to PR #377 addressing Lindsay's feedback that the HELP banners and the wizard reference in `db install`'s "Next steps" panel still hard-coded `npx`. #379 already added `runnerCommand(pm, ref)` + `detectPackageManager()`; this sweep applies it to the remaining places. User-visible: a user who runs `bunx @cipherstash/cli --help` now sees `bunx @cipherstash/cli` in the Usage line and Examples, and the wizard suggestion at the end of `db install` matches the user's runner. Same for `pnpm dlx`, `yarn dlx`, default `npx`. Touches: - `bin/stash.ts` — `HELP` template now interpolates a `STASH` constant (`runnerCommand(PM, '@cipherstash/cli')`) for the Usage line, the six Examples, and the `requireStack` hint. - `commands/auth/index.ts` — same pattern for the auth HELP, with a `STASH_AUTH` constant for the Usage line + two examples. - `commands/db/install.ts` — the wizard line in `printNextSteps` uses `runnerCommand(pm, '@cipherstash/wizard')`. Also brings the `p.intro('npx @cipherstash/cli db install')` line in line with the runner-aware pattern (was already pinned for the same reason in PR #377; this branch is off main, so it lands here too). Messages: `messages.cli.usagePrefix` and `messages.auth.usagePrefix` become stable leaders (`'Usage: '`) — the runner+package suffix is appended at render time. E2E assertions stay runner-agnostic because they `toContain('Usage: ')` rather than the full prefix. Out of scope (still): the migrate stub message in `messages.db.migrateNotImplemented` keeps its hard-coded `npx` form. The stub is a placeholder and the inconsistency only shows when someone runs a not-yet-implemented command — not worth plumbing for.
Adds 8 new E2E tests in `tests/e2e/runner-aware-help.e2e.test.ts` asserting that `--help` and `auth --help` surface the right runner (`npx` / `bunx` / `pnpm dlx` / `yarn dlx`) based on `npm_config_user_agent`. Covers the gap between the existing unit tests for `detectPackageManager` / `runnerCommand` (which prove the helpers work in isolation) and the user-visible HELP rendering (which the smoke suite checks for existence but not runner-correctness). The Next-steps panel in `db install` and the `requireStack` hint are still uncovered by automation — they require either a real DB or a fixture without `@cipherstash/stack`. Documented as known gaps in the PR description.
coderdan
force-pushed
the
dan/runner-aware-help-banners
branch
from
03d845e to
0560049
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/cli/tests/e2e/runner-aware-help.e2e.test.ts (1)
35-36: ⚡ Quick winUse message constants for asserted user-facing strings.
These assertions hard-code usage text; prefer constants from
src/messages.tsto keep E2E aligned with copy changes.♻️ Proposed refactor
import { describe, expect, it } from 'vitest' import { render } from '../helpers/pty.js' +import { messages } from '../../src/messages.js' @@ - expect(r.output).toContain(`Usage: ${label} stash`) + expect(r.output).toContain(`${messages.cli.usagePrefix}${label} stash`) @@ - expect(r.output).toContain(`Usage: ${label} stash auth`) + expect(r.output).toContain(`${messages.auth.usagePrefix}${label} stash auth`)As per coding guidelines, "Extract user-facing strings that E2E tests assert on into
src/messages.tsas constants rather than hard-coding them in tests".Also applies to: 51-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/e2e/runner-aware-help.e2e.test.ts` around lines 35 - 36, Replace hard-coded usage and example strings in the assertions that inspect r.output (e.g., the expect(r.output).toContain(`Usage: ${label} stash`) and the similar assertions around lines 51-52) with constants exported from src/messages.ts: import the appropriate message constants (e.g., USAGE_STASH or analogous names) and use those constants in the expect(...).toContain(...) checks; update the test to reference the message constant(s) for both the Usage and Examples assertions so E2E stays in sync with copy changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/cli-runner-aware-help-banners.md:
- Around line 9-16: The fenced code block containing the CLI usage examples (the
block starting with "Usage: bunx `@cipherstash/cli` <command> [options]") is
missing a language identifier and fails markdown linting; update the opening
fence to include a language (e.g., add "text" after the three backticks) so it
reads ```text and leave the block contents unchanged.
In `@packages/cli/tests/e2e/runner-aware-help.e2e.test.ts`:
- Around line 19-20: The test data in
packages/cli/tests/e2e/runner-aware-help.e2e.test.ts currently includes an entry
{ ua: '', label: 'npx' } which is brittle because an empty npm_config_user_agent
can resolve via lockfile fallback; update the test by removing the hard-coded
'npx' expectation for the empty-ua case and instead either (A) add a separate
e2e test that sets up a controlled cwd with no lockfile and asserts the fallback
is 'npx', or (B) change the empty-ua case to assert a more flexible outcome
(e.g., expect no forced label or check that detection falls back to lockfile
resolution) so the test no longer assumes 'npx' unconditionally; locate the
array of UA test vectors in the runner-aware-help.e2e.test.ts file to apply the
change.
---
Nitpick comments:
In `@packages/cli/tests/e2e/runner-aware-help.e2e.test.ts`:
- Around line 35-36: Replace hard-coded usage and example strings in the
assertions that inspect r.output (e.g., the expect(r.output).toContain(`Usage:
${label} stash`) and the similar assertions around lines 51-52) with constants
exported from src/messages.ts: import the appropriate message constants (e.g.,
USAGE_STASH or analogous names) and use those constants in the
expect(...).toContain(...) checks; update the test to reference the message
constant(s) for both the Usage and Examples assertions so E2E stays in sync with
copy changes.
🪄 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: 99d22c1e-578e-4629-bfdd-2e9c84bb19fa
📒 Files selected for processing (6)
.changeset/cli-runner-aware-help-banners.mdpackages/cli/src/bin/stash.tspackages/cli/src/commands/auth/index.tspackages/cli/src/commands/db/install.tspackages/cli/src/messages.tspackages/cli/tests/e2e/runner-aware-help.e2e.test.ts
auxesis
added a commit
that referenced
this pull request
May 4, 2026
Reconciles the rebased branch with main's PR #384 (runner-aware help banners) and the @cipherstash/cli → stash rename. Changes: - Replace remaining `npx drizzle-kit` hardcodings in db/install.ts with the detected runner (Task 8 work that conflicted during rebase and was skipped — these spots weren't covered by main). - Make messages.db.migrateNotImplemented runner-aware. Main's hardcoded literal `"npx stash db migrate"` was caught by the lint; the factory takes a `stashRef` matching the existing `STASH` constant in bin/stash.ts. - Allowlist packages/cli/src/commands/init/lib/setup-prompt.ts in the lint script — its `case 'npm': return 'npx --no-install'` is the same canonical pattern we already allowlist in init/utils.ts. - Update supabase-migration.test.ts assertions to reference `stash` instead of `@cipherstash/cli`. - Revert the stale post-review fix to `printNextSteps`/ `writeSupabaseMigrationFile` — main's `printNextSteps()` is self-contained and doesn't need a runner threaded through.
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
2 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 #385.
Follow-up to #377 addressing Lindsay's feedback that the
--helpbanners and the wizard suggestion indb install's "Next steps" panel still hard-codednpx @cipherstash/cliregardless of how the user invoked the CLI.#379 already shipped
runnerCommand(pm, ref)+detectPackageManager(). This applies them to the remaining places.Before / after
Same for
pnpm dlx,yarn dlx, defaultnpx.Touches
bin/stash.ts—HELPinterpolates aSTASHconstant (runnerCommand(PM, '@cipherstash/cli')) for the usage line, six examples, and therequireStackhint shown when@cipherstash/stackisn't installed.commands/auth/index.ts— same pattern for the auth HELP (usage line + two examples).commands/db/install.ts— the wizard line inprintNextStepsusesrunnerCommand(pm, '@cipherstash/wizard').Messages
messages.cli.usagePrefixandmessages.auth.usagePrefixbecome stable leaders ('Usage: ') — the runner+package portion is appended at render time. The smoke E2E tests already usetoContainagainst these constants, so they stay runner-agnostic without test changes.Out of scope
messages.db.migrateNotImplementedkeeps its hard-codednpxform.db migrateis a not-yet-implemented stub; the inconsistency only surfaces when someone runs a command that doesn't exist yet. Not worth plumbing.Manual test plan
I ran each of the below locally; would appreciate reviewers running them too.
git checkout dan/runner-aware-help-banners pnpm install pnpm --filter @cipherstash/cli build STASH=$PWD/packages/cli/dist/bin/stash.jsDetection prefers
npm_config_user_agentover lockfile; overriding it directly is the easiest way to test each runner without installing them.1. Top-level
--help— Usage line and the six Examples should all use the runner:```sh
node $STASH --help # → npx (default)
npm_config_user_agent='bun/1.x' node $STASH --help # → bunx
npm_config_user_agent='pnpm/9.x' node $STASH --help # → pnpm dlx
npm_config_user_agent='yarn/4.x' node $STASH --help # → yarn dlx
```
2.
auth --help(or justauthwith no subcommand):```sh
npm_config_user_agent='bun/1.x' node $STASH auth
Expected: "Usage: bunx @cipherstash/cli auth [options]"
Examples: "bunx @cipherstash/cli auth login" / "...auth login --supabase"
```
3.
db install"Next steps" panel — the wizard line should match the runner:```sh
mkdir /tmp/runner-test && cd /tmp/runner-test
DATABASE_URL=postgresql://test/x npm_config_user_agent='bun/1.x' \
node $STASH db install --dry-run
When the panel prints, the wizard line should read:
bunx @cipherstash/wizard
(You'll also see "bunx @cipherstash/cli db install" as the intro — covered by #377.)
```
4.
requireStackhint — surfaces when runningdb push/validate/schema buildwithout@cipherstash/stackinstalled:```sh
cd /tmp/runner-test
DATABASE_URL=postgresql://test/x npm_config_user_agent='pnpm/9.x' \
node $STASH db push
Expected error:
@cipherstash/stack is required for this command.
Install it with: pnpm add @cipherstash/stack
Or run: pnpm dlx @cipherstash/cli init
```
5. Lockfile fallback — without
npm_config_user_agent, detection falls back to lockfile:```sh
cd /tmp/runner-test
touch bun.lock
node $STASH --help # without npm_config_user_agent override
Expected: bunx-flavoured output
rm bun.lock && touch pnpm-lock.yaml
node $STASH --help
Expected: pnpm dlx
rm pnpm-lock.yaml
```
Automated coverage
runnerCommand/detectPackageManagerthemselvessrc/commands/init/__tests__/utils.test.ts(from #379)--helpUsage line + Examplestests/e2e/runner-aware-help.e2e.test.ts— 4 cases (npx/bunx/pnpm dlx/yarn dlx) asserting the right runner appearsauth --helpUsage line + Examplesdb install"Next steps" panel (wizard line)requireStackhint@cipherstash/stackinstalled so the path doesn't fire in CI. Manual only.messages.cli.usagePrefixno longer hardcoded withnpxThe two gaps (Next-steps panel and requireStack hint) are awkward to E2E without standing up a real DB or surgically removing
@cipherstash/stackfrom the test environment. If a future PR adds either of those harnesses, both would be cheap to cover then.Test plan
pnpm --filter @cipherstash/cli buildcleanpnpm --filter @cipherstash/cli test— unit tests passpnpm --filter @cipherstash/cli test:e2e— 18 E2E tests pass (was 10; +8 from newrunner-aware-help.e2e.test.ts)biome checkclean on changed filesnode dist/bin/stash.js --helpwithnpm_config_user_agentcleared / set topnpm/9/ set tobun/1and confirmed the rendered Usage line + Examples match the runner.Summary by CodeRabbit
New Features
bunx,pnpm dlx,yarn dlxinstead of always showingnpx).Tests