fix(cli): validate --name and pass --out in the v2 Drizzle migration generator#703
Conversation
…generator
`stash eql install --drizzle` (EQL v2) built a shell command string and ran it
via execSync, interpolating an unvalidated `--name`. A name carrying shell
metacharacters was executed:
stash eql install --drizzle --eql-version 2 --name 'x; rm -rf ~'
It also computed `outDir` from `--out`, searched it for the generated file, but
never passed `--out` to drizzle-kit — so any project whose drizzle.config.ts
writes elsewhere had the migration written in one place and looked for in
another, dying at the locate step.
Both fixes mirror the v3 generator (`stash eql migration --drizzle`), which
already had them: reuse its `SAFE_MIGRATION_NAME` guard and invoke drizzle-kit
through spawnSync with an argv array (no shell). `SAFE_MIGRATION_NAME` moves to
install.ts — migration.ts already imports from it, so the shared constant sits
on that side of the existing edge and no import cycle is introduced.
The generator's five `process.exit(1)` sites become `throw new CliExit(1)` so
the branches are unit testable and the telemetry `finally` in main.ts still
runs. `install-eql.ts` re-throws CliExit from its broad catch, so a hard stop
during `stash init` stays a hard stop instead of being reframed as a database
connection failure.
🦋 Changeset detectedLatest commit: 62b41d1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
📝 WalkthroughWalkthroughThe Drizzle migration generator now validates migration names, invokes project-local ChangesDrizzle migration hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant generateDrizzleMigration
participant spawnSync
participant drizzle-kit
participant MigrationFilesystem
CLI->>generateDrizzleMigration: provide --name and --out
generateDrizzleMigration->>generateDrizzleMigration: validate --name
generateDrizzleMigration->>spawnSync: invoke with argv tokens
spawnSync->>drizzle-kit: run project-local generate command
drizzle-kit->>MigrationFilesystem: write migration under --out
generateDrizzleMigration->>MigrationFilesystem: locate and rewrite migration
MigrationFilesystem-->>CLI: generated migration or CliExit
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
Review follow-ups on the v2 Drizzle migration generator fix. The `instanceof CliExit` re-throw in `init/steps/install-eql.ts` was the one edit outside the two main files and had no coverage. Without it a hard stop the installer already reported on gets reframed as "check your database connection" and init continues past it. Add a test for the re-throw plus its companion — an ordinary throw still being swallowed so init carries on — since the pair is what makes either meaningful. Tighten the argv assertion from `typeof command === 'string'` and `toContain` to a full `toEqual`. The loose form passed even with the runner prefix dropped, which would run drizzle-kit under the wrong resolver. `detectPackageManager` is pinned so the assertion is deterministic (detection reads the cwd lockfile and npm_config_user_agent); the runner mapping itself stays real. Also correct the stale doc comment on `installEqlStep` — `installCommand` now ends init via `process.exit(1)` or a thrown `CliExit`, not only the former. Verified by revert: removing the re-throw fails 1 of 7; dropping the `dlx` prefix fails 1 of 9. Suite: 52 files, 594 tests. code:check clean.
There was a problem hiding this comment.
Pull request overview
This PR hardens the v2 (stash eql install --drizzle --eql-version 2) Drizzle migration generator to match the already-fixed v3 generator, addressing a shell-injection vector in --name, ensuring --out is actually passed to drizzle-kit, and converting hard process.exit(1) paths into CliExit to preserve telemetry and enable unit testing.
Changes:
- Validate
--namewith a sharedSAFE_MIGRATION_NAMEregex and invokedrizzle-kitviaspawnSync(command, argv)(no shell interpolation) in the v2 generator. - Always forward
--outtodrizzle-kit generate, and improve the locate-failure hint when the configured output directory differs. - Replace several
process.exit(1)paths withthrow new CliExit(1)and add targeted unit coverage for both the migration generator and init’sCliExitrethrow behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| skills/stash-cli/SKILL.md | Documents the tightened --name character set and clarifies that --out is forwarded to drizzle-kit --out. |
| packages/cli/src/commands/init/steps/install-eql.ts | Re-throws CliExit so init does not reframe cooperative exits as connection failures or continue past hard stops. |
| packages/cli/src/commands/init/steps/tests/install-eql.test.ts | Adds tests verifying CliExit is re-thrown and non-CliExit errors are still swallowed with a generic message. |
| packages/cli/src/commands/eql/migration.ts | Removes the local copy of SAFE_MIGRATION_NAME and imports the shared constant from the v2 install module. |
| packages/cli/src/commands/db/install.ts | Fixes v2 Drizzle migration generation: validates --name, passes --out, uses spawnSync argv invocation, and throws CliExit on failures. |
| packages/cli/src/commands/db/tests/generate-drizzle-migration.test.ts | New unit tests covering unsafe-name rejection, dry-run ordering/preview, argv shape (no shell), --out forwarding, and non-zero drizzle-kit exit handling. |
| .changeset/khaki-pugs-play.md | Adds a patch changeset for the stash package describing the two v2 generator fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Relates to #707 (EQL v2 removal). This patches Also adjacent to #613 (v3 Drizzle migration install) — same command neighbourhood, opposite generation. |
auxesis
left a comment
auxesis
left a comment
There was a problem hiding this comment.
Verdict
The fix itself is sound. spawnSync with an argv array, no shell, gated on ^[\w-]+$ is the right shape for the injection defect, and the exact-argv assertion (rather than toContain) genuinely pins --out reaching drizzle-kit. Hoisting SAFE_MIGRATION_NAME into db/install.ts with the import-cycle rationale spelled out is the right call. Changeset and skills/stash-cli/SKILL.md are both updated. No blocking defects found.
Everything below is coverage or a design question, not a bug in the fix.
The theme: generateDrizzleMigration converted five process.exit(1) sites into throw new CliExit(1); only two are covered (bad name, non-zero exit). The three cleanup/abort paths — locate failure, SQL-load failure, write failure — are untested, even though the sibling v3 suite (packages/cli/src/commands/eql/__tests__/migration.test.ts:219) already tests the equivalent write-failure cleanup. Those paths matter more than usual here: a swallowed or mis-ordered CliExit leaves an empty scaffolded migration on disk that drizzle-kit migrate will happily apply and record as done.
The one item I'd genuinely resolve before merge is the runnerArgv / execArgv divergence (inline on install.ts:515) — not a regression, but the new exact-argv assertion cements it.
Caveat: the suite could not be executed in this checkout (vitest is not installed — pnpm install was never run here), so every finding is from reading, and the suggested test sketches are unexecuted.
Additional findings not posted inline
- Nothing asserts the two generators share one
SAFE_MIGRATION_NAME. The new doc comment says "there must not be a second copy", but that's unenforced — a re-introduced local copy incommands/eql/migration.tswould pass CI silently. A one-lineexpect(SAFE_MIGRATION_NAME.test('a b')).toBe(false)imported fromdb/install.jsin the v3 suite would make the drift fail. - The bundled-SQL failure branch (
install.ts:609) is the twin of the--latestbranch flagged inline. StubbingloadBundledEqlSqlgets you both cleanup arms for one mock, so this is nearly free once the--latesttest exists.
Review stats
| Source | Model | Type | Raw | Kept |
|---|---|---|---|---|
| claude | claude-opus-4-8 | test-gap | 6 | 6 (+1 promoted from its summary note) |
| codex | gpt-5.5 | test-gap | 1 | 1 (merged into the write-failure finding) |
- Total raw: 7 → 7 kept (7 inline, 2 overflow bullets in the body).
- Dropped as unsubstantiated: 0. Every finding verified against the diff and the surrounding code.
- Cross-model overlap: 1 kept finding corroborated by 2+ models — the write-failure cleanup gap (
install.ts:629), raised independently by both sources with near-identical test sketches. The remaining 6 are single-source (claude), all verified. - One claude finding was promoted from its summary's "out-of-scope note" to an inline comment (
install.ts:515, therunnerArgvvsexecArgvdivergence) — it verifies cleanly against the v3 generator's own comment and is the highest-value item in the set.
…execArgv Address the @auxesis review threads on #703 — all test-coverage gaps on the v2 generateDrizzleMigration failure arms, plus one documentation gap: - write-failure cleanup (delegating writeFileSync spy) - --latest fetch-failure cleanup (stubbed downloadEqlSql) - the --out remediation hint when the migration isn't where we looked - the defaulted --out arm (flag omitted -> <cwd>/drizzle) via dry-run - the ENOENT and exit-status arms of the error fallback chain - --name '' rejection boundary (empty is not nullish) Also comment why v2 keeps the download-and-run form (dlx) while v3 uses the project-local execArgv form, so the pinned-argv divergence is a conscious contract.
Switch the v2 generateDrizzleMigration invocation from the download-and-run form (runnerArgv -> pnpm dlx / npx / bunx) to the project-local form (execArgv -> pnpm exec / npx --no-install), so drizzle-kit resolves THIS project's drizzle.config.ts and schema and fails loudly if drizzle-kit is missing instead of surprise-downloading a possibly-different major. The dry-run preview (execCommand) and the drizzle-kit migrate hint follow suit. Updates the pinned-argv test from 'dlx' to 'exec'. Resolves review thread T2 by making the recommended change rather than documenting the divergence.
Closes #726
Closes #726
generateDrizzleMigration(the v2 Drizzle path incommands/db/install.ts) is a hand-copy of the v3 generator incommands/eql/migration.tsthat never received its fixes. Two defects, both already solved on the v3 side.Shell injection
install.tsbuilt a shell string interpolatingmigrationName— straight from--name, unvalidated — and ran it throughexecSync. So:executed the injected command. The v3 generator guards the identical value with
SAFE_MIGRATION_NAME = /^[\w-]+$/and invokes viaspawnSync(command, argv)with no shell. This applies the same guard and the same argv-based invocation.SAFE_MIGRATION_NAMEnow lives ininstall.tsandmigration.tsimports it — that direction was chosen deliberately:migration.tsalready imports three helpers frominstall.ts, so the constant sits on an existing edge. The reverse would have created a cycle.--outcomputed but never passedinstall.tscomputedoutDirand searched it for the generated file, but omitted--outfrom the drizzle-kit command. A project whosedrizzle.config.tswrites elsewhere had drizzle-kit write there while step 2 looked indrizzle/— the run then died with a bare exit. The v3 generator passes--out=${outDir}; this does too, and adds the same "pass --out if your config writes elsewhere" hint on locate failure.Also in this PR
The five
process.exit(1)sites in this function becamethrow new CliExit(1)— percli/exit.tsthose were untracked by telemetry and untestable, which is why this path had no coverage.That conversion needed one edit outside the two files:
init/steps/install-eql.tswrapsinstallCommandin a broadcatchthat reports "EQL install failed" and lets init continue. Without aninstanceof CliExitre-throw, a former hard exit would have become a misleading message plus continuation on thestash init --drizzlepath.Verification
__tests__/generate-drizzle-migration.test.ts— 9 tests covering name rejection (; rm -rf ~,$(...), backticks, spaces,../), rejection ordering vs the dry-run preview, argv shape, andCliExiton non-zero drizzle-kit exit.code:checkexits 0.Notes
SAFE_MIGRATION_NAMErejects., so a name likeinstall.eqlthat drizzle-kit previously accepted now errors. This matches v3 behaviour and the error names the allowed set. Widening both to/^[\w.-]+$/is a one-line alternative if the narrowing isn't wanted.runnerCommand(pnpm dlx/npx— download-and-run) where v3 usesexecArgv(pnpm exec/npx --no-install— project-local).npx drizzle-kitcan fetch a fresh drizzle-kit that won't resolve the project'sdrizzle.config.ts.skills/stash-cli/SKILL.mdupdated: the--namerows now state the allowed character set, and--outdocuments that it reachesdrizzle-kit --out.🤖 Generated with Claude Code
Summary by CodeRabbit
--namevalues and preventing shell-interpolated execution.--outis forwarded to Drizzle so generated files land where Drizzle expects them.--nameformats and that--outmust match the Drizzle configuration foreql install --drizzleandeql migration.