test: ensure no hard coded package managers#401
Merged
Conversation
🦋 Changeset detectedLatest commit: 17b63dd 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/next-step strings and shell-outs package-manager-aware by detecting the local package manager (npx / bunx / pnpm dlx / yarn dlx) and threading that runner into displayed examples and exec calls across wizard, cli, protect, and drizzle. It also adds a lint script and CI checks to prevent reintroduction of hardcoded ChangesPackage-Manager-Aware Runner Integration
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/User
participant CLI as CLI/Wizard/Drizzle/Protect entrypoint
participant Detector as Runner Detector (fromUserAgent/fromLockfile)
participant Shell as Exec (drizzle-kit / supabase / other)
Dev->>CLI: run --help or trigger migration/install
CLI->>Detector: detectRunner()
Detector-->>CLI: returns runner (e.g., "pnpm dlx")
CLI->>Dev: render help/next-steps using runner
CLI->>Shell: execute command prefixed with runner
Shell-->>CLI: command output/exit
CLI-->>Dev: show output/status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace dual NPX_INLINE and NPX_INDENTED regexes with a single NPX_TOKEN
that properly catches:
- Quoted 'npx' literals (e.g., `runner = 'npx'`)
- Command invocations like `npx @cipherstash/cli` (including inside
multi-line template literals where the line doesn't start with npx)
- Does not flag npx used as a JS identifier (npxResult, etc.)
Add test fixtures and tests for:
- Multi-line template literals (wizard-style.ts, Test A)
- Default function parameters (default-param.ts, Test B)
- JS identifiers (identifier.ts, Test D)
- Verify fallback expressions still allowed (Test C)
Pre-allowlist helper files for Tasks 11 and 13 to land without lint breaks.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace three hardcoded npx @cipherstash/cli strings in the env command with dynamically detected package manager runners (npx, bunx, pnpm dlx, yarn dlx). Updates formatEnvBlock to accept the detected cliRef as a parameter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Convert MIGRATION_HEADER from a module-level const to a migrationHeader() function that resolves the runner at call time, ensuring the SQL migration header reflects the detected package manager (npx, bunx, pnpm dlx, yarn dlx). This migration file ends up in users' repos and should correctly document how it was installed. Update the test assertion to match any of the four runners rather than assuming npx. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace hardcoded 'npx --no-install' with the detected package manager's dlx runner (npx/bunx/pnpm dlx/yarn dlx). The --no-install flag has no equivalent in bunx/pnpm dlx/yarn dlx, so we drop it and rely on the local-binary candidate ('supabase' command) to short-circuit the common case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/tests.yml (1)
25-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNode.js version mismatch with
package.jsonengines requirement.CI uses
node-version: 20butpackage.jsonspecifies"engines": { "node": ">=22" }. This inconsistency means tests run on an unsupported Node version, potentially masking compatibility issues. As per coding guidelines, Node.js version >= 22 should be enforced.🔧 Proposed fix
- name: Install Node.js uses: actions/setup-node@v4 with: - node-version: 20 + node-version: 22 cache: 'pnpm'Apply the same change to line 124 in the
e2e-testsjob.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 25 - 29, Update the GitHub Actions setup-node steps to use Node.js 22 to match package.json engines: change the node-version value from 20 to 22 in the "Install Node.js" step that uses actions/setup-node@v4, and make the identical change in the e2e-tests job's Install Node.js step so both jobs run on Node >=22 (leave cache: 'pnpm' as-is).
🧹 Nitpick comments (5)
packages/drizzle/src/bin/runner.ts (1)
1-25: 💤 Low valueConsider extracting shared runner detection logic.
This implementation is identical to
packages/protect/src/bin/runner.ts. While package independence in a monorepo is valid, maintaining two copies increases the risk of drift. Consider extracting to a shared internal package or keeping both in sync via a lint/test assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/drizzle/src/bin/runner.ts` around lines 1 - 25, The runner detection logic (functions fromUserAgent, fromLockfile and detectRunner) is duplicated with packages/protect/src/bin/runner.ts; extract these into a shared module (e.g., packages/utils or packages/internal/runner) and replace the copies in both packages with imports of that module, or alternatively add a unit test or lint rule that asserts the two implementations stay identical; update imports in packages/drizzle/src/bin/runner.ts and packages/protect/src/bin/runner.ts to call the shared function (detectRunner) and remove the duplicated local functions.packages/cli/src/__tests__/supabase-migration.test.ts (1)
16-36: ⚡ Quick winThe local
migrationHeadercopy tests itself, not production code.
migrationHeaderis not exported frompackages/cli/src/commands/db/supabase-migration.ts, so the newdescribe('migrationHeader')suite (lines 250–271) exercises only the local duplicate. If the production function's wording, URL, or flags change, these tests will still pass unmodified. The genuinely useful coverage is already provided by thewriteSupabaseEqlMigrationintegration assertion at line 138.Two options to fix this:
- Export
migrationHeaderfromsupabase-migration.tsand import it here, deleting the local copy.- Remove the
describe('migrationHeader')suite entirely and rely on the integration test at line 138 (which already validates all four runner variants via regex).♻️ Option 1 – export and import instead of duplicating
In
packages/cli/src/commands/db/supabase-migration.ts:-function migrationHeader(runner: string): string { +export function migrationHeader(runner: string): string {In
packages/cli/src/__tests__/supabase-migration.test.ts:import { SUPABASE_EQL_MIGRATION_FILENAME, writeSupabaseEqlMigration, + migrationHeader, } from '../commands/db/supabase-migration.js' - -/** - * Generate the migration header for testing purposes. - * Mirrors the production function but imported for testing. - */ -function migrationHeader(runner: string): string { - return `-- CipherStash EQL — installed by \`${runner} `@cipherstash/cli` db install --supabase --migration\`. -... -` -}As per coding guidelines: "Prefer testing via public API; avoid reaching into private internals in tests."
Also applies to: 250-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/__tests__/supabase-migration.test.ts` around lines 16 - 36, The test file contains a local duplicate of migrationHeader which tests the copy instead of the production function; either export migrationHeader from the production module (packages/cli/src/commands/db/supabase-migration.ts) and import that symbol into the test (packages/cli/src/__tests__/supabase-migration.test.ts) removing the local duplicate, or delete the describe('migrationHeader') suite and rely on the existing writeSupabaseEqlMigration integration assertion (which already validates the runners); update imports and test assertions accordingly so tests exercise the real migrationHeader implementation rather than a local copy.packages/wizard/src/__tests__/errors-runner.test.ts (1)
4-21: 💤 Low valueMissing
yarn dlxtest case for completeness.The suite covers
npx,bunx, andpnpm dlxbut omitsyarn dlx, which is one of the four supported runners introduced by this PR. Adding a symmetric case would close the gap.➕ Proposed addition
it('uses pnpm dlx when runner=pnpm dlx for auth failure', () => { expect(classifyError('authentication_failed', '', 'pnpm dlx')).toContain( 'Run: pnpm dlx `@cipherstash/cli` auth login', ) }) + + it('uses yarn dlx when runner=yarn dlx for auth failure', () => { + expect(classifyError('authentication_failed', '', 'yarn dlx')).toContain( + 'Run: yarn dlx `@cipherstash/cli` auth login', + ) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wizard/src/__tests__/errors-runner.test.ts` around lines 4 - 21, The test suite for classifyError is missing a symmetric case for the 'yarn dlx' runner; add a new it(...) test in packages/wizard/src/__tests__/errors-runner.test.ts mirroring the existing cases for 'npx', 'bunx', and 'pnpm dlx' that calls classifyError('authentication_failed', '', 'yarn dlx') and asserts the returned message contains 'Run: yarn dlx `@cipherstash/cli` auth login', keeping the same structure and style as the other tests.e2e/tests/package-managers.e2e.test.ts (2)
199-217: ⚡ Quick winAdd a
beforeAllexistence guard for the Suite C binaries.Suite B guards against missing binaries with an explicit
beforeAllcheck (line 109–113) that surfaces a clear error. Suite C skips this and relies onspawnSyncreturning a non-zeroresult.status, which produces a less actionable failure message (e.g.,ENOENTburied inresult.error). At minimum, the error string passed toexpectalready surfacesresult.stderr, which helps, but a pre-test guard is more developer-friendly.🔧 Suggested addition
describe('binaries — help text uses detected runner', () => { + beforeAll(() => { + for (const [name, bin] of Object.entries(BIN)) { + if (!existsSync(bin)) { + throw new Error( + `Binary "${name}" not found at ${bin}. Run the build step first.`, + ) + } + } + }) + for (const pm of PMS) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/package-managers.e2e.test.ts` around lines 199 - 217, Add a beforeAll existence guard for Suite C that mirrors Suite B: iterate over BIN entries and verify each binary file exists before running tests (using the same check logic used in Suite B), and if any are missing call fail/throw a clear error that includes the binary name and expected path; update the describe block "binaries — help text uses detected runner" to include this beforeAll so tests using spawnSync, UA, PMS, RUNNER, BIN and the spawnSync invocations fail fast with an actionable message rather than relying on spawnSync ENOENT.
1-6: 💤 Low valueMerge duplicate
node:child_processimports.
spawnSync(line 6) andexecFileSync(line 1) can be combined into a single import.♻️ Proposed fix
-import { execFileSync } from 'node:child_process' +import { execFileSync, spawnSync } from 'node:child_process' import { existsSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { dirname, join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' -import { spawnSync } from 'node:child_process'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/package-managers.e2e.test.ts` around lines 1 - 6, Merge the duplicate imports from 'node:child_process' by combining execFileSync and spawnSync into a single import statement; update the top of the file to import both execFileSync and spawnSync from 'node:child_process' (replace the separate imports referencing execFileSync and spawnSync) so only one child_process import remains and remove the redundant import line.
🤖 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/db/supabase-migration.ts`:
- Line 108: The option excludeOperatorFamily is being forced to true by the
expression "excludeOperatorFamily || true"; update the call that sets this
option (the parameter passed as excludeOperatorFamily in the supabase migration
logic) to pass the actual variable directly (or rely on the destructuring
default) instead of using "|| true" so the caller's provided value is respected;
locate the usage of excludeOperatorFamily in the supabase-migration.ts call site
and remove the "|| true" fallback.
In `@packages/cli/tests/e2e/smoke.e2e.test.ts`:
- Around line 75-89: Replace the hard-coded strings 'bunx `@cipherstash/cli`' and
'bunx `@cipherstash/cli` auth' in the two tests with the exported message
factories from src/messages.ts (the factory that produces the full "Usage:" line
for a detected runner and the factory that produces the runner+command usage);
import the appropriate factory functions/constants into
packages/cli/tests/e2e/smoke.e2e.test.ts, call them with the 'bun' runner (and
with 'auth' for the second test) and assert the full produced Usage string
against r.output instead of the hard-coded substrings, leaving the existing
render(...) and exit assertions unchanged.
In `@packages/wizard/src/agent/interface.ts`:
- Around line 83-91: The function isAllowedDlxCommand currently uses
rest.startsWith(t) which allows variants like "drizzle-kit-malicious"; update
the check inside isAllowedDlxCommand to ensure the matched tool name is a whole
token by verifying rest === t or rest starts with the tool plus a space (i.e.,
rest === t || rest.startsWith(`${t} `)); keep the existing RUNNER_PREFIXES loop
and ALLOWED_DLX_TOOLS list but replace the loose startsWith check with this
stricter token-boundary logic so only the exact tool name or the tool name
followed by arguments is allowed.
---
Outside diff comments:
In @.github/workflows/tests.yml:
- Around line 25-29: Update the GitHub Actions setup-node steps to use Node.js
22 to match package.json engines: change the node-version value from 20 to 22 in
the "Install Node.js" step that uses actions/setup-node@v4, and make the
identical change in the e2e-tests job's Install Node.js step so both jobs run on
Node >=22 (leave cache: 'pnpm' as-is).
---
Nitpick comments:
In `@e2e/tests/package-managers.e2e.test.ts`:
- Around line 199-217: Add a beforeAll existence guard for Suite C that mirrors
Suite B: iterate over BIN entries and verify each binary file exists before
running tests (using the same check logic used in Suite B), and if any are
missing call fail/throw a clear error that includes the binary name and expected
path; update the describe block "binaries — help text uses detected runner" to
include this beforeAll so tests using spawnSync, UA, PMS, RUNNER, BIN and the
spawnSync invocations fail fast with an actionable message rather than relying
on spawnSync ENOENT.
- Around line 1-6: Merge the duplicate imports from 'node:child_process' by
combining execFileSync and spawnSync into a single import statement; update the
top of the file to import both execFileSync and spawnSync from
'node:child_process' (replace the separate imports referencing execFileSync and
spawnSync) so only one child_process import remains and remove the redundant
import line.
In `@packages/cli/src/__tests__/supabase-migration.test.ts`:
- Around line 16-36: The test file contains a local duplicate of migrationHeader
which tests the copy instead of the production function; either export
migrationHeader from the production module
(packages/cli/src/commands/db/supabase-migration.ts) and import that symbol into
the test (packages/cli/src/__tests__/supabase-migration.test.ts) removing the
local duplicate, or delete the describe('migrationHeader') suite and rely on the
existing writeSupabaseEqlMigration integration assertion (which already
validates the runners); update imports and test assertions accordingly so tests
exercise the real migrationHeader implementation rather than a local copy.
In `@packages/drizzle/src/bin/runner.ts`:
- Around line 1-25: The runner detection logic (functions fromUserAgent,
fromLockfile and detectRunner) is duplicated with
packages/protect/src/bin/runner.ts; extract these into a shared module (e.g.,
packages/utils or packages/internal/runner) and replace the copies in both
packages with imports of that module, or alternatively add a unit test or lint
rule that asserts the two implementations stay identical; update imports in
packages/drizzle/src/bin/runner.ts and packages/protect/src/bin/runner.ts to
call the shared function (detectRunner) and remove the duplicated local
functions.
In `@packages/wizard/src/__tests__/errors-runner.test.ts`:
- Around line 4-21: The test suite for classifyError is missing a symmetric case
for the 'yarn dlx' runner; add a new it(...) test in
packages/wizard/src/__tests__/errors-runner.test.ts mirroring the existing cases
for 'npx', 'bunx', and 'pnpm dlx' that calls
classifyError('authentication_failed', '', 'yarn dlx') and asserts the returned
message contains 'Run: yarn dlx `@cipherstash/cli` auth login', keeping the same
structure and style as the other tests.
🪄 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: 9fc9ef69-2b14-4106-9a1e-f05faa39e0d1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
.changeset/package-manager-aware-output.md.github/workflows/tests.ymle2e/package.jsone2e/tests/package-managers.e2e.test.tspackage.jsonpackages/cli/src/__tests__/messages.test.tspackages/cli/src/__tests__/supabase-migration.test.tspackages/cli/src/bin/stash.tspackages/cli/src/commands/auth/index.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/db/supabase-migration.tspackages/cli/src/commands/env/index.tspackages/cli/src/config/database-url.tspackages/cli/src/messages.tspackages/cli/tests/e2e/smoke.e2e.test.tspackages/drizzle/src/bin/generate-eql-migration.tspackages/drizzle/src/bin/runner.tspackages/protect/src/bin/runner.tspackages/protect/src/bin/stash.tspackages/wizard/src/__tests__/errors-runner.test.tspackages/wizard/src/agent/__tests__/interface.test.tspackages/wizard/src/agent/errors.tspackages/wizard/src/agent/fetch-prompt.tspackages/wizard/src/agent/interface.tspackages/wizard/src/bin/wizard.tspackages/wizard/src/lib/detect.tsscripts/__tests__/fixtures/__tests__/inside.test.tsscripts/__tests__/fixtures/allowed-comment.tsscripts/__tests__/fixtures/allowed-fallback.tsscripts/__tests__/fixtures/clean.tsscripts/__tests__/fixtures/default-param.tsscripts/__tests__/fixtures/identifier.tsscripts/__tests__/fixtures/multiline-offender.tsscripts/__tests__/fixtures/offender.tsscripts/__tests__/fixtures/wizard-style.tsscripts/__tests__/lint-no-hardcoded-runners.test.mjsscripts/lint-no-hardcoded-runners.mjsscripts/vitest.config.mjs
Comment on lines
75
to
89
| it('--help renders detected runner (bunx) when npm_config_user_agent=bun', async () => { | ||
| const r = render(['--help'], { env: { npm_config_user_agent: 'bun/1.0.0' } }) | ||
| const { exitCode } = await r.exit | ||
| expect(exitCode).toBe(0) | ||
| expect(r.output).toContain('bunx @cipherstash/cli') | ||
| expect(r.output).not.toMatch(/\bnpx\b/) | ||
| }) | ||
|
|
||
| it('auth renders detected runner (bunx) when npm_config_user_agent=bun', async () => { | ||
| const r = render(['auth'], { env: { npm_config_user_agent: 'bun/1.0.0' } }) | ||
| const { exitCode } = await r.exit | ||
| expect(exitCode).toBe(0) | ||
| expect(r.output).toContain('bunx @cipherstash/cli auth') | ||
| expect(r.output).not.toMatch(/\bnpx\b/) | ||
| }) |
There was a problem hiding this comment.
Use message factories instead of hard-coded runner strings.
Lines 79 and 87 hard-code 'bunx @cipherstash/cli' / 'bunx @cipherstash/cli auth' directly rather than using the message factories that already exist for this purpose. This violates the guideline: "Extract user-facing strings that E2E tests assert on into src/messages.ts as constants rather than hard-coding them in tests."
Using the factories also makes the assertions more precise (full Usage: prefix rather than a substring) and ensures they automatically reflect any copy-edit to messages.ts.
🛡️ Proposed fix
it('--help renders detected runner (bunx) when npm_config_user_agent=bun', async () => {
const r = render(['--help'], { env: { npm_config_user_agent: 'bun/1.0.0' } })
const { exitCode } = await r.exit
expect(exitCode).toBe(0)
- expect(r.output).toContain('bunx `@cipherstash/cli`')
+ expect(r.output).toContain(messages.cli.usagePrefix('bunx'))
expect(r.output).not.toMatch(/\bnpx\b/)
})
it('auth renders detected runner (bunx) when npm_config_user_agent=bun', async () => {
const r = render(['auth'], { env: { npm_config_user_agent: 'bun/1.0.0' } })
const { exitCode } = await r.exit
expect(exitCode).toBe(0)
- expect(r.output).toContain('bunx `@cipherstash/cli` auth')
+ expect(r.output).toContain(messages.auth.usagePrefix('bunx'))
expect(r.output).not.toMatch(/\bnpx\b/)
})As per coding guidelines: "Extract user-facing strings that E2E tests assert on into src/messages.ts as constants rather than hard-coding them in tests."
📝 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.
Suggested change
| it('--help renders detected runner (bunx) when npm_config_user_agent=bun', async () => { | |
| const r = render(['--help'], { env: { npm_config_user_agent: 'bun/1.0.0' } }) | |
| const { exitCode } = await r.exit | |
| expect(exitCode).toBe(0) | |
| expect(r.output).toContain('bunx @cipherstash/cli') | |
| expect(r.output).not.toMatch(/\bnpx\b/) | |
| }) | |
| it('auth renders detected runner (bunx) when npm_config_user_agent=bun', async () => { | |
| const r = render(['auth'], { env: { npm_config_user_agent: 'bun/1.0.0' } }) | |
| const { exitCode } = await r.exit | |
| expect(exitCode).toBe(0) | |
| expect(r.output).toContain('bunx @cipherstash/cli auth') | |
| expect(r.output).not.toMatch(/\bnpx\b/) | |
| }) | |
| it('--help renders detected runner (bunx) when npm_config_user_agent=bun', async () => { | |
| const r = render(['--help'], { env: { npm_config_user_agent: 'bun/1.0.0' } }) | |
| const { exitCode } = await r.exit | |
| expect(exitCode).toBe(0) | |
| expect(r.output).toContain(messages.cli.usagePrefix('bunx')) | |
| expect(r.output).not.toMatch(/\bnpx\b/) | |
| }) | |
| it('auth renders detected runner (bunx) when npm_config_user_agent=bun', async () => { | |
| const r = render(['auth'], { env: { npm_config_user_agent: 'bun/1.0.0' } }) | |
| const { exitCode } = await r.exit | |
| expect(exitCode).toBe(0) | |
| expect(r.output).toContain(messages.auth.usagePrefix('bunx')) | |
| expect(r.output).not.toMatch(/\bnpx\b/) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/tests/e2e/smoke.e2e.test.ts` around lines 75 - 89, Replace the
hard-coded strings 'bunx `@cipherstash/cli`' and 'bunx `@cipherstash/cli` auth' in
the two tests with the exported message factories from src/messages.ts (the
factory that produces the full "Usage:" line for a detected runner and the
factory that produces the runner+command usage); import the appropriate factory
functions/constants into packages/cli/tests/e2e/smoke.e2e.test.ts, call them
with the 'bun' runner (and with 'auth' for the second test) and assert the full
produced Usage string against r.output instead of the hard-coded substrings,
leaving the existing render(...) and exit assertions unchanged.
- Add detectPackageManager import to wizard.ts and compute RUNNER for dynamic usage string - Remove default 'npx' parameter values from classifyError, classifyHttpError, and fetchIntegrationPrompt - callers must now pass runner explicitly - Refactor ALLOWED_BASH_COMMANDS in interface.ts to support all four DLX runners (npx, bunx, pnpm dlx, yarn dlx) via new isAllowedDlxCommand predicate - Add comprehensive test coverage for DLX command allowlist (package-managers.e2e.test.ts already covers wizard binary) - Allowlist packages/wizard/src/agent/interface.ts in lint-no-hardcoded-runners.mjs for legitimate comparison data All 7 wizard offenders now resolved; 5 drizzle offenders remain for Task 13. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…table Tighten lint allowlist scope by deriving RUNNER_PREFIXES from the canonical PACKAGE_MANAGERS table in detect.ts. This prevents future hardcoded runner strings in interface.ts from slipping past the linter. The observable behavior is unchanged — RUNNER_PREFIXES is still a string array with the same values. - Export PACKAGE_MANAGERS from detect.ts - Replace hardcoded RUNNER_PREFIXES array with derivation via Object.values().map() - Remove overbroad interface.ts allowlist entry, tightening lint scope Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
auxesis
force-pushed
the
test/ensure-no-hard-coded-package-managers
branch
from
abf314d to
6cc47d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/db/install.ts (1)
353-353:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded
npm installon the drizzle-kit error message — contradicts PR goalLine 353 was not touched by this PR but directly contradicts its objective: a
bunx/pnpm dlx/yarn dlxuser who hits this error will be told to runnpm install -D drizzle-kitinstead of the right command for their package manager.🐛 Proposed fix (depends on `pm` variable from the refactor above)
- p.log.info('Make sure drizzle-kit is installed: npm install -D drizzle-kit') + const installCmd = + pm === 'yarn' ? 'yarn add -D drizzle-kit' + : pm === 'pnpm' ? 'pnpm add -D drizzle-kit' + : pm === 'bun' ? 'bun add -D drizzle-kit' + : 'npm install -D drizzle-kit' + p.log.info(`Make sure drizzle-kit is installed: ${installCmd}`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/db/install.ts` at line 353, Replace the hardcoded npm instruction in the p.log.info call with a dynamic install suggestion that uses the existing pm variable introduced by the refactor; locate the p.log.info('Make sure drizzle-kit is installed: npm install -D drizzle-kit') message and build the appropriate command string from pm (e.g., use pm to format the install invocation for npm/pnpm/bun/yarn/dlx) and log that computed command so users see the correct install command for their package manager.
♻️ Duplicate comments (1)
packages/wizard/src/agent/interface.ts (1)
83-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse token-boundary matching for DLX tool names.
Line 87 still accepts any command whose tool merely starts with an allowed prefix, so
npx drizzle-kit-maliciousoryarn dlx stash dbxwould pass this Bash allowlist. Since this gate is what limits agent shell execution, that widens the allowed command surface beyond the intended tool set.Suggested fix
function isAllowedDlxCommand(cmd: string): boolean { for (const prefix of RUNNER_PREFIXES) { if (cmd.startsWith(`${prefix} `)) { const rest = cmd.slice(prefix.length + 1) - return ALLOWED_DLX_TOOLS.some((t) => rest.startsWith(t)) + return ALLOWED_DLX_TOOLS.some( + (t) => rest === t || rest.startsWith(`${t} `), + ) } } return false }🤖 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 83 - 87, The isAllowedDlxCommand function currently treats any rest that startsWith an ALLOWED_DLX_TOOLS entry as allowed, which permits names like "drizzle-kit-malicious"; update the check in isAllowedDlxCommand (which iterates RUNNER_PREFIXES and computes rest) to use token-boundary matching instead of simple startsWith — e.g., replace ALLOWED_DLX_TOOLS.some((t) => rest.startsWith(t)) with a test that ensures rest is exactly the tool token or the tool token followed by whitespace (or use a RegExp like ^tool(\s|$) for each t) so only exact tool names (not prefixes) are allowed.
🧹 Nitpick comments (1)
packages/wizard/src/agent/__tests__/interface.test.ts (1)
34-52: ⚡ Quick winAdd a regression case for near-match tool names.
This suite checks unknown tools, but it still won't catch the current
startsWithbypass for names likenpx drizzle-kit-maliciousoryarn dlx stash dbx. A focused negative case here would keep this security boundary from regressing silently.Suggested test addition
describe('rejects unknown tools regardless of runner', () => { + it('rejects tool names that only share an allowed prefix', () => { + expect( + wizardCanUseTool('Bash', { + command: 'npx drizzle-kit-malicious generate', + }), + ).not.toBe(true) + + expect( + wizardCanUseTool('Bash', { + command: 'yarn dlx stash dbx install', + }), + ).not.toBe(true) + }) + it('rejects curl with any runner prefix', () => { for (const runner of ['npx', 'bunx', 'pnpm dlx', 'yarn dlx']) { const result = wizardCanUseTool('Bash', { command: `${runner} curl https://evil.example`, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wizard/src/agent/__tests__/interface.test.ts` around lines 34 - 52, Add a negative regression test to the existing suite that ensures near-match tool names cannot bypass the check: in the same describe block add an it case that loops the same runners array (['npx','bunx','pnpm dlx','yarn dlx']) and calls wizardCanUseTool('Bash', { command: `${runner} drizzle-kit-malicious ...` }) and similarly for a near-match like `${runner} stash dbx ...` asserting the result is not true; place the new test alongside the existing "rejects curl" and "rejects rm" tests so it fails if wizardCanUseTool incorrectly uses startsWith matching for tool names.
🤖 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/db/install.ts`:
- Around line 309-311: The migrationName (from options.name) is interpolated
directly into drizzleCmd and then executed via a shell, which allows command
injection; validate and sanitize migrationName before use by applying an
allowlist regexp (e.g. only permit [A-Za-z0-9._-] and reject or error on
anything else) and refuse or normalize invalid input, then construct drizzleCmd
using the validated migrationName; alternatively, avoid shell interpolation
altogether by invoking the command with child_process.spawn/execFile with an
args array (use runnerCommand/detectPackageManager to locate the binary and pass
["drizzle-kit","generate","--custom","--name", migrationName]) so user input is
never injected into a shell.
- Line 311: In generateDrizzleMigration, avoid calling detectPackageManager()
twice and pass the real package name into runnerCommand: compute and store const
pkgManager = detectPackageManager() once at the top of the function, then build
drizzleCmd using runnerCommand(pkgManager, 'drizzle-kit') (instead of
runnerCommand(detectPackageManager(), '')). Update any other usages around the
later block (previously calling detectPackageManager() again on lines ~443-445)
to reuse pkgManager so the command uses a cached package manager and correctly
specifies 'drizzle-kit'; keep using migrationName for the --name flag and
preserve trimming/spacing as needed.
In `@packages/cli/src/messages.ts`:
- Around line 34-35: The test is asserting messages.db.migrateNotImplemented
without invoking it; update the E2E test to call the factory with the stashRef
argument (e.g. messages.db.migrateNotImplemented('stash')) so the assertion
compares against the produced string. Locate the failing assertion and replace
the reference to migrateNotImplemented with an invocation that passes the
expected stashRef value used in the test.
---
Outside diff comments:
In `@packages/cli/src/commands/db/install.ts`:
- Line 353: Replace the hardcoded npm instruction in the p.log.info call with a
dynamic install suggestion that uses the existing pm variable introduced by the
refactor; locate the p.log.info('Make sure drizzle-kit is installed: npm install
-D drizzle-kit') message and build the appropriate command string from pm (e.g.,
use pm to format the install invocation for npm/pnpm/bun/yarn/dlx) and log that
computed command so users see the correct install command for their package
manager.
---
Duplicate comments:
In `@packages/wizard/src/agent/interface.ts`:
- Around line 83-87: The isAllowedDlxCommand function currently treats any rest
that startsWith an ALLOWED_DLX_TOOLS entry as allowed, which permits names like
"drizzle-kit-malicious"; update the check in isAllowedDlxCommand (which iterates
RUNNER_PREFIXES and computes rest) to use token-boundary matching instead of
simple startsWith — e.g., replace ALLOWED_DLX_TOOLS.some((t) =>
rest.startsWith(t)) with a test that ensures rest is exactly the tool token or
the tool token followed by whitespace (or use a RegExp like ^tool(\s|$) for each
t) so only exact tool names (not prefixes) are allowed.
---
Nitpick comments:
In `@packages/wizard/src/agent/__tests__/interface.test.ts`:
- Around line 34-52: Add a negative regression test to the existing suite that
ensures near-match tool names cannot bypass the check: in the same describe
block add an it case that loops the same runners array (['npx','bunx','pnpm
dlx','yarn dlx']) and calls wizardCanUseTool('Bash', { command: `${runner}
drizzle-kit-malicious ...` }) and similarly for a near-match like `${runner}
stash dbx ...` asserting the result is not true; place the new test alongside
the existing "rejects curl" and "rejects rm" tests so it fails if
wizardCanUseTool incorrectly uses startsWith matching for tool names.
🪄 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: 5ffb0c2b-d1eb-4e34-90ea-bd192a8810ea
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
.changeset/package-manager-aware-output.md.github/workflows/tests.ymle2e/package.jsone2e/tests/package-managers.e2e.test.tspackage.jsonpackages/cli/src/__tests__/supabase-migration.test.tspackages/cli/src/bin/stash.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/db/supabase-migration.tspackages/cli/src/commands/env/index.tspackages/cli/src/config/database-url.tspackages/cli/src/messages.tspackages/drizzle/src/bin/generate-eql-migration.tspackages/drizzle/src/bin/runner.tspackages/protect/src/bin/runner.tspackages/protect/src/bin/stash.tspackages/wizard/src/__tests__/errors-runner.test.tspackages/wizard/src/agent/__tests__/interface.test.tspackages/wizard/src/agent/errors.tspackages/wizard/src/agent/fetch-prompt.tspackages/wizard/src/agent/interface.tspackages/wizard/src/bin/wizard.tspackages/wizard/src/lib/detect.tsscripts/__tests__/fixtures/__tests__/inside.test.tsscripts/__tests__/fixtures/allowed-comment.tsscripts/__tests__/fixtures/allowed-fallback.tsscripts/__tests__/fixtures/clean.tsscripts/__tests__/fixtures/default-param.tsscripts/__tests__/fixtures/identifier.tsscripts/__tests__/fixtures/multiline-offender.tsscripts/__tests__/fixtures/offender.tsscripts/__tests__/fixtures/wizard-style.tsscripts/__tests__/lint-no-hardcoded-runners.test.mjsscripts/lint-no-hardcoded-runners.mjsscripts/vitest.config.mjs
✅ Files skipped from review due to trivial changes (19)
- scripts/vitest.config.mjs
- scripts/tests/fixtures/identifier.ts
- scripts/tests/fixtures/offender.ts
- scripts/tests/fixtures/allowed-fallback.ts
- scripts/tests/fixtures/allowed-comment.ts
- scripts/tests/fixtures/tests/inside.test.ts
- scripts/tests/fixtures/multiline-offender.ts
- scripts/tests/fixtures/default-param.ts
- scripts/tests/lint-no-hardcoded-runners.test.mjs
- scripts/tests/fixtures/clean.ts
- packages/cli/src/tests/supabase-migration.test.ts
- packages/drizzle/src/bin/generate-eql-migration.ts
- e2e/tests/package-managers.e2e.test.ts
- packages/wizard/src/lib/detect.ts
- packages/wizard/src/tests/errors-runner.test.ts
- packages/cli/src/config/database-url.ts
- scripts/tests/fixtures/wizard-style.ts
- package.json
- packages/wizard/src/bin/wizard.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- e2e/package.json
- packages/wizard/src/agent/errors.ts
- .github/workflows/tests.yml
- packages/drizzle/src/bin/runner.ts
- scripts/lint-no-hardcoded-runners.mjs
- packages/protect/src/bin/runner.ts
- packages/wizard/src/agent/fetch-prompt.ts
- .changeset/package-manager-aware-output.md
- packages/cli/src/commands/db/supabase-migration.ts
Comment on lines
309
to
+311
| const migrationName = options.name ?? DEFAULT_MIGRATION_NAME | ||
| const outDir = resolve(options.out ?? DEFAULT_DRIZZLE_OUT) | ||
| const drizzleCmd = `${runnerCommand(detectPackageManager(), '').trim()} drizzle-kit generate --custom --name=${migrationName}` |
There was a problem hiding this comment.
migrationName is interpolated unquoted into a shell command — validate before use
options.name (a raw CLI flag value) flows directly into the drizzleCmd string without sanitization. execSync with a string argument spawns a shell, so it is important to never pass unsanitized user input to this function. A user who passes --name "foo; rm -rf ." (intentionally or accidentally) will have the injected command executed. Even a plain space in the name causes silent argument-splitting failures.
Add an allowlist check before constructing drizzleCmd:
🛡️ Proposed fix — validate `migrationName` early
const migrationName = options.name ?? DEFAULT_MIGRATION_NAME
+ if (!/^[a-zA-Z0-9][a-zA-Z0-9_-]*$/.test(migrationName)) {
+ p.log.error(
+ `Invalid migration name "${migrationName}". Only alphanumerics, hyphens, and underscores are allowed.`,
+ )
+ p.outro('Migration aborted.')
+ process.exit(1)
+ }
const outDir = resolve(options.out ?? DEFAULT_DRIZZLE_OUT)
const drizzleCmd = `${runnerCommand(detectPackageManager(), '').trim()} drizzle-kit generate --custom --name=${migrationName}`Also applies to: 332-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/db/install.ts` around lines 309 - 311, The
migrationName (from options.name) is interpolated directly into drizzleCmd and
then executed via a shell, which allows command injection; validate and sanitize
migrationName before use by applying an allowlist regexp (e.g. only permit
[A-Za-z0-9._-] and reject or error on anything else) and refuse or normalize
invalid input, then construct drizzleCmd using the validated migrationName;
alternatively, avoid shell interpolation altogether by invoking the command with
child_process.spawn/execFile with an args array (use
runnerCommand/detectPackageManager to locate the binary and pass
["drizzle-kit","generate","--custom","--name", migrationName]) so user input is
never injected into a shell.
There was a problem hiding this comment.
@coderdan, glad the finding resonates! Would you like me to go ahead and plan the code change? The fix involves two parts:
- Validate
migrationNameearly (before constructingdrizzleCmd) with an allowlist regex so invalid names are rejected with a clear error. - Optionally, switch from
execSync(string)toexecSyncwith a pre-built array-style invocation orexecFileto avoid shell interpolation entirely — though input validation alone is the minimal safe fix.
Just say the word and I'll kick off ``.
( ˆ＿ˆ)⊃━ ✉
Failed to handle agent chat message. Please try again.
| ) { | ||
| const migrationName = options.name ?? DEFAULT_MIGRATION_NAME | ||
| const outDir = resolve(options.out ?? DEFAULT_DRIZZLE_OUT) | ||
| const drizzleCmd = `${runnerCommand(detectPackageManager(), '').trim()} drizzle-kit generate --custom --name=${migrationName}` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate and display the runnerCommand implementation
ast-grep --pattern 'function runnerCommand($$$) { $$$ }'
ast-grep --pattern 'const runnerCommand = ($$$) => $$$'
ast-grep --pattern 'export function runnerCommand($$$) { $$$ }'
rg -n "runnerCommand" --type ts -A 5 -B 2Repository: cipherstash/stack
Length of output: 29999
Cache detectPackageManager() once and pass 'drizzle-kit' to runnerCommand for clarity
Two issues in generateDrizzleMigration:
-
API misuse:
runnerCommandexpects a package name as its second argument. Passing''and trimming the trailing space works today but is fragile — any internal change torunnerCommand(e.g., formatting differences) silently breaks the command. Pass'drizzle-kit'directly instead. -
Redundant calls:
detectPackageManager()is called separately on line 311 and again on line 443. Cache it once at the function level.
Suggested fix
const migrationName = options.name ?? DEFAULT_MIGRATION_NAME
const outDir = resolve(options.out ?? DEFAULT_DRIZZLE_OUT)
+ const pm = detectPackageManager()
- const drizzleCmd = `${runnerCommand(detectPackageManager(), '').trim()} drizzle-kit generate --custom --name=${migrationName}`
+ const drizzleCmd = `${runnerCommand(pm, 'drizzle-kit')} generate --custom --name=${migrationName}` p.note(
- `Run your Drizzle migrations to install EQL:\n\n ${runnerCommand(detectPackageManager(), '').trim()} drizzle-kit migrate`,
+ `Run your Drizzle migrations to install EQL:\n\n ${runnerCommand(pm, 'drizzle-kit')} migrate`,
'Next Steps',
)Also applies to: 443-445
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/db/install.ts` at line 311, In
generateDrizzleMigration, avoid calling detectPackageManager() twice and pass
the real package name into runnerCommand: compute and store const pkgManager =
detectPackageManager() once at the top of the function, then build drizzleCmd
using runnerCommand(pkgManager, 'drizzle-kit') (instead of
runnerCommand(detectPackageManager(), '')). Update any other usages around the
later block (previously calling detectPackageManager() again on lines ~443-445)
to reuse pkgManager so the command uses a cached package manager and correctly
specifies 'drizzle-kit'; keep using migrationName for the --name flag and
preserve trimming/spacing as needed.
There was a problem hiding this comment.
@coderdan, glad it's useful! Here's the planned fix:
- Cache
detectPackageManager()once aspmat the top ofgenerateDrizzleMigration - Replace both
runnerCommand(detectPackageManager(), '').trim()usages withrunnerCommand(pm, 'drizzle-kit')
Failed to handle agent chat message. Please try again.
`migrateNotImplemented` was converted from a string to a runner-aware factory in 6cc47d2; the existing E2E assertion still passed it as a string to `toContain`, which then matched against the printed function name instead of the warning text. Local unit tests use a separate vitest config and didn't catch it; the CLI E2E job did. Switch the assertion to the stable runner-agnostic suffix `'stash db migrate" is not yet implemented.'` — works under any runner the test environment detects (pnpm dlx in CI, npm/npx locally).
Three small follow-ups from the latest review pass:
- isAllowedDlxCommand used \`rest.startsWith(t)\`, which would let
\`bunx drizzle-kit-malicious\` slip through on \`drizzle-kit\`.
Tighten to a token-boundary match (\`rest === t || rest.startsWith(\`\${t} \`)\`)
so only the exact tool or the tool followed by a space matches.
- errors-runner.test.ts covered npx/bunx/pnpm dlx but not yarn dlx —
add the missing case to both \`classifyError\` and \`classifyHttpError\`
suites for symmetry.
- e2e/tests/package-managers.e2e.test.ts had two separate
\`node:child_process\` imports (\`execFileSync\` and \`spawnSync\`).
Merge into one.
Out of scope from the same review pass: the pre-existing
\`excludeOperatorFamily || true\` in supabase-migration.ts (introduced
2026-04-28, before this branch); the duplicate \`migrationHeader\` in
the test file (already triaged in prior review); the runner.ts
duplication between protect/drizzle (explicitly out-of-scope per the
plan).
coderdan
reviewed
May 4, 2026
| "dependencies": { | ||
| "stash": "workspace:*", | ||
| "@cipherstash/drizzle": "workspace:*", | ||
| "@cipherstash/protect": "workspace:*", |
Contributor
There was a problem hiding this comment.
Is this just for legacy testing?
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/wizard/src/agent/interface.ts (1)
392-398:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the remaining hardcoded
npxfallbacks from the error paths.Lines 392-398, 527-529, and 547-548 still default to
'npx'whensession.detectedPackageManageris missing, so bun/pnpm/yarn users will still get npm-specific remediation on failures. Please route these through the same runner source as the allowlist, or fall back to a neutral placeholder instead.Also applies to: 527-529, 547-548
🤖 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 392 - 398, Replace the hardcoded 'npx' fallback used when building the classifyError call with the same runner source used for the allowlist resolution instead of a npm-specific default: read the runner from session.detectedPackageManager?.execCommand (or the common helper used elsewhere to resolve the package manager/runner) and pass that into classifyError; if no runner can be resolved, use a neutral placeholder string (e.g. '<package-manager>') rather than 'npx'. Update the three spots that call classifyError in this file (where the current code uses session.detectedPackageManager?.execCommand ?? 'npx') so they rely on the shared resolution logic and pass that value into classifyError.
♻️ Duplicate comments (1)
packages/cli/tests/e2e/smoke.e2e.test.ts (1)
72-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid hard-coded CLI copy in this E2E assertion.
At Line 74, the test reintroduces a literal user-facing string. Keep the runner-agnostic behavior, but derive the asserted text from
messages.db.migrateNotImplemented(...)so copy changes remain centralized.Proposed fix
- // `migrateNotImplemented` is a runner-aware factory; the runner-agnostic - // suffix is the stable assertion target. - expect(r.output).toContain('stash db migrate" is not yet implemented.') + // `migrateNotImplemented` is runner-aware; derive a stable suffix from + // the source-of-truth message factory instead of hard-coding copy. + const runnerToken = '__RUNNER__' + const stableSuffix = messages.db + .migrateNotImplemented(runnerToken) + .split(`${runnerToken} `)[1] + expect(r.output).toContain(stableSuffix)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."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/e2e/smoke.e2e.test.ts` around lines 72 - 74, Replace the hard-coded CLI copy in the assertion with the centralized message constant: import messages (from src/messages) into the test and change the assertion to expect(r.output).toContain(messages.db.migrateNotImplemented('migrate')) (or the appropriate runner-agnostic suffix argument required by messages.db.migrateNotImplemented), so the test derives its expected text from messages.db.migrateNotImplemented rather than a literal string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/wizard/src/agent/interface.ts`:
- Around line 392-398: Replace the hardcoded 'npx' fallback used when building
the classifyError call with the same runner source used for the allowlist
resolution instead of a npm-specific default: read the runner from
session.detectedPackageManager?.execCommand (or the common helper used elsewhere
to resolve the package manager/runner) and pass that into classifyError; if no
runner can be resolved, use a neutral placeholder string (e.g.
'<package-manager>') rather than 'npx'. Update the three spots that call
classifyError in this file (where the current code uses
session.detectedPackageManager?.execCommand ?? 'npx') so they rely on the shared
resolution logic and pass that value into classifyError.
---
Duplicate comments:
In `@packages/cli/tests/e2e/smoke.e2e.test.ts`:
- Around line 72-74: Replace the hard-coded CLI copy in the assertion with the
centralized message constant: import messages (from src/messages) into the test
and change the assertion to
expect(r.output).toContain(messages.db.migrateNotImplemented('migrate')) (or the
appropriate runner-agnostic suffix argument required by
messages.db.migrateNotImplemented), so the test derives its expected text from
messages.db.migrateNotImplemented rather than a literal string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b75f58b-8f68-4f56-8ad9-138084d4dbb8
📒 Files selected for processing (4)
e2e/tests/package-managers.e2e.test.tspackages/cli/tests/e2e/smoke.e2e.test.tspackages/wizard/src/__tests__/errors-runner.test.tspackages/wizard/src/agent/interface.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/tests/package-managers.e2e.test.ts
coderdan
approved these changes
May 4, 2026
Contributor
coderdan
left a comment
coderdan
left a comment
There was a problem hiding this comment.
Some valid feedback from Coderabbit which should be addressed but otherwise LGTM
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.
What
#379 added the ability to detect which package manager is in use, and uses it in CLI output.
However,
npxwas still hardcoded in several places in the code base:bunx,pnpm dlx, oryarn dlx.This PR closes those gaps and adds a CI lint that prevents regressions.
Why
bunx @cipherstash/cli initshould never see instructions telling them to runnpx @cipherstash/cli db installHow
packages/**for hardcodednpxreferences and fails on regressions.runnerCommandin@cipherstash/cli,detectPackageManagerin@cipherstash/wizard).--helpunder all four package managers and asserts the rendered runner matches.Summary by CodeRabbit
New Features
Tests
Chores