fix(wizard,cli): report a part-way-failed ALTER COLUMN sweep as destructive (#786) - #812
fix(wizard,cli): report a part-way-failed ALTER COLUMN sweep as destructive (#786)#812tobyhede wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 90ca56b 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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/sweep-partial-rewrite-destructive.md:
- Around line 35-42: Update the second paragraph to describe failures occurring
before recording any rewrite or skipped statement, rather than before attempting
any write or reporting zero counts. Preserve the distinction that a failure
after skipped statements retains flagged-statement guidance and does not use the
softer no-known-data wording.
In `@skills/stash-cli/SKILL.md`:
- Around line 395-396: Update the sweep failure documentation to state that the
reported file list includes both successfully rewritten and attempted files,
including the file whose write failed. Instruct users to inspect every listed
file because any may be truncated or partially replaced, while preserving the
warning that the directory requires review before running drizzle-kit migrate.
🪄 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 Plus
Run ID: 5ce03ded-1e9f-4627-8da0-7ac371b734ab
📒 Files selected for processing (12)
.changeset/sweep-partial-rewrite-destructive.mdpackages/cli/src/__tests__/rewrite-migrations.test.tspackages/cli/src/commands/db/__tests__/generate-drizzle-migration.test.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/db/rewrite-migrations.tspackages/cli/src/commands/eql/__tests__/migration.test.tspackages/cli/src/commands/eql/migration.tspackages/wizard/src/__tests__/post-agent.test.tspackages/wizard/src/__tests__/rewrite-migrations.test.tspackages/wizard/src/lib/post-agent.tspackages/wizard/src/lib/rewrite-migrations.tsskills/stash-cli/SKILL.md
| If a wizard sweep flags a raw ALTER and then fails before rewriting anything, | ||
| the prompt now preserves both facts: it keeps the flagged-statement guidance | ||
| and names the migration directory that the sweep could not finish checking. | ||
|
|
||
| A sweep that fails before attempting any write is unchanged: it rejects with | ||
| the original error, reports zeros, and keeps the softer "nothing is known about | ||
| this directory" wording, because claiming data destruction there would be a | ||
| guess. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Qualify the “reports zeros” failure case.
This contradicts the preceding paragraph: a sweep can record skipped statements, then fail before attempting any rewrite. In that case rewritten is zero but skipped is not, and the prompt retains the flagged-statement guidance. Change “before attempting any write” / “reports zeros” to “before recording any rewrite or skipped statement.”
Suggested wording
-A sweep that fails before attempting any write is unchanged: it rejects with
-the original error, reports zeros, and keeps the softer "nothing is known about
-this directory" wording, because claiming data destruction there would be a
-guess.
+A sweep that fails before recording any rewrite or skipped statement is
+unchanged: it rejects with the original error, reports zero rewrite/skip
+results, and keeps the softer "nothing is known about this directory" wording,
+because claiming data destruction there would be a guess.📝 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.
| If a wizard sweep flags a raw ALTER and then fails before rewriting anything, | |
| the prompt now preserves both facts: it keeps the flagged-statement guidance | |
| and names the migration directory that the sweep could not finish checking. | |
| A sweep that fails before attempting any write is unchanged: it rejects with | |
| the original error, reports zeros, and keeps the softer "nothing is known about | |
| this directory" wording, because claiming data destruction there would be a | |
| guess. | |
| If a wizard sweep flags a raw ALTER and then fails before rewriting anything, | |
| the prompt now preserves both facts: it keeps the flagged-statement guidance | |
| and names the migration directory that the sweep could not finish checking. | |
| A sweep that fails before recording any rewrite or skipped statement is | |
| unchanged: it rejects with the original error, reports zero rewrite/skip | |
| results, and keeps the softer "nothing is known about this directory" wording, | |
| because claiming data destruction there would be a guess. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/sweep-partial-rewrite-destructive.md around lines 35 - 42, Update
the second paragraph to describe failures occurring before recording any rewrite
or skipped statement, rather than before attempting any write or reporting zero
counts. Preserve the distinction that a failure after skipped statements retains
flagged-statement guidance and does not use the softer no-known-data wording.
| If the sweep itself fails part way through — an unwritable file, a full disk, a lock held by an editor or `drizzle-kit` — it still lists the files it rewrote before the failure, *and* warns that it did not fully complete. Both matter: the listed files already contain the `DROP COLUMN`, and the rest of the directory went unchecked. Treat that directory as needing review before you run `drizzle-kit migrate`. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document attempted files as potentially partially rewritten.
The error contract includes the file whose write failed because it may already be truncated or partially replaced. Therefore, not every listed path is guaranteed to have completed the rewrite or already contain DROP COLUMN. Tell users that the list includes rewritten/attempted files and that every listed file must be inspected.
Suggested wording
-If the sweep itself fails part way through — an unwritable file, a full disk, a lock held by an editor or `drizzle-kit` — it still lists the files it rewrote before the failure, *and* warns that it did not fully complete. Both matter: the listed files already contain the `DROP COLUMN`, and the rest of the directory went unchecked. Treat that directory as needing review before you run `drizzle-kit migrate`.
+If the sweep itself fails part way through — an unwritable file, a full disk, a lock held by an editor or `drizzle-kit` — it still lists files it rewrote or attempted before the failure, *and* warns that it did not fully complete. An attempted write may already have partially modified its destination, so inspect every listed file and the rest of the directory before you run `drizzle-kit migrate`.📝 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.
| If the sweep itself fails part way through — an unwritable file, a full disk, a lock held by an editor or `drizzle-kit` — it still lists the files it rewrote before the failure, *and* warns that it did not fully complete. Both matter: the listed files already contain the `DROP COLUMN`, and the rest of the directory went unchecked. Treat that directory as needing review before you run `drizzle-kit migrate`. | |
| If the sweep itself fails part way through — an unwritable file, a full disk, a lock held by an editor or `drizzle-kit` — it still lists files it rewrote or attempted before the failure, *and* warns that it did not fully complete. An attempted write may already have partially modified its destination, so inspect every listed file and the rest of the directory before you run `drizzle-kit migrate`. |
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 35: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 36: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 43: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 44: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 45: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 51: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 52: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 64: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 65: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 73: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 91: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 179: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 181: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 182: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 348: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 367: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 625: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 312: [MP2] Context Window Stuffing: Skill attempts to fill the context window with filler content, displacing legitimate instructions and safety constraints. This can degrade agent performance or bypass safety boundaries.
Remediation: Implement context-window management that detects and rejects padding or stuffing attempts. Prioritize system instructions over user-injected content.
(Memory Poisoning (MP2))
[warning] 12: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@skills/stash-cli/SKILL.md` around lines 395 - 396, Update the sweep failure
documentation to state that the reported file list includes both successfully
rewritten and attempted files, including the file whose write failed. Instruct
users to inspect every listed file because any may be truncated or partially
replaced, while preserving the warning that the directory requires review before
running drizzle-kit migrate.
|
Review complete: no findings. The patch preserves partial rewrite state and reports it consistently across CLI and wizard paths. Targeted tests and builds pass without regressions. |
…uctive (#786) The Drizzle migration rewriter writes one file at a time. When it threw *after* rewriting a file — ENOSPC, a read-only file, a lock held by an editor or drizzle-kit — the accumulated `rewritten` array died with the stack frame. `sweepMigrationDirs` then hard-coded `rewritten: []` on the error result, and `rewriteEncryptedMigrations` `continue`d past the whole reporting block, so the wizard printed "the sweep could not check 1 directory (drizzle/)" over a prompt that never mentioned data loss — while a live DROP COLUMN sat on disk. The last fail-open path in the A-2 sweep, in exactly the subsystem where it costs the most. `rewriteEncryptedAlterColumns` now rejects with a `PartialRewriteError` carrying `rewritten` and `skipped` whenever it fails part way through a directory it has already changed, preserving the underlying message verbatim and keeping the original as `cause`. A failure with no work behind it still rejects with the original error untouched, so the "nothing is known about this directory" arm is byte-for-byte unchanged. `sweepMigrationDirs` propagates those arrays onto the error result, and the wizard's report no longer skips a directory that errored: the rewritten files are listed with the existing data-destroying warning, the "did not fully complete" warning still fires, and the prompt takes the destructive arm (initialValue: false, "DESTROYS data"). Both facts are true; the destructive one wins because it is the one the user cannot afford to miss. `stash eql migration --drizzle` and `stash eql install --drizzle` likewise name any files the sweep rewrote before it failed, instead of pointing at the directory and leaving the user to guess. The rewriter change lands identically in both mirrored copies (scripts/__tests__/rewriter-copies-in-sync.test.mjs is green). Tests use a delegating `node:fs/promises` writeFile spy that fails the Nth call — deterministic, unlike chmod, which passes silently as root. Coverage spans the rewriter (both copies), the directory sweep, the wizard report driven end to end through a real failing write, and both CLI callers. Skill updated: skills/stash-cli documents what a part-way failure now reports.
…shes mid-walk `lint-no-hardcoded-runners.mjs` walks the packages tree with a recursive `readdir`. The repo's own script self-tests create and delete probe package directories in that same tree while other suites run, and vitest ran those suites in parallel — so the walk could enumerate a probe directory and find it gone before recursing into it. Unhandled, that rejects with ENOENT and Node exits 1: the same code the linter uses for "found a hardcoded npx". CI then failed naming nothing, on a tree the identical script had just passed one step earlier, and a re-run went green. The script already draws this distinction for a missing target — "Exit 2 means the linter could not run" — but only at the top level, not one frame down. Observed on three branches before this one (30316404472, 30317589094), which is why it reads as a flake rather than a defect. Two changes: - `walk` treats ENOENT as "nothing here to lint" and skips. A directory that no longer exists cannot contain a violation, and crashing with the violation exit code is the actual bug. - `scripts/vitest.config.mjs` sets `fileParallelism: false`. Several of these suites mutate the real repo tree, so cross-file parallelism is unsound here regardless of this particular pair. Eleven small files; it costs a second. Verified by reproducing the race locally — churning a probe directory while running the linter failed 6/30 before, 0/40 after. One wrinkle worth recording: the first draft of these comments named the probe directories with their `packages/` prefix, which the sibling `lint-no-dead-package-paths` linter correctly flagged as references to directories that do not exist. The names are now written bare.
ad403cb to
90ca56b
Compare
2 participants
Closes #786
The Drizzle migration rewriter writes one file at a time. When it threw after rewriting a file — ENOSPC, a read-only file, an editor or
drizzle-kitholding a lock — the accumulatedrewrittenarray died with the stack frame.sweepMigrationDirsthen hard-codedrewritten: []on the error result, andrewriteEncryptedMigrationscontinued past the whole reporting block, so the wizard printedover a prompt that never mentioned data loss — while a live
DROP COLUMNsat on disk. This was the last fail-open path left by the A-2 sweep (#783, #785), in the subsystem where it costs the most.What changed
rewriteEncryptedAlterColumnsrejects with a newPartialRewriteErrorcarryingrewrittenandskippedwhenever it fails part way through a directory it has already changed. The underlying message is preserved verbatim and the original is kept ascause. The whole per-file loop is wrapped, not just thewriteFilecall — a throw from the post-write near-miss scan would otherwise still lose the file written in that same iteration.sweepMigrationDirspropagates those arrays onto the error result instead of emptying them.packages/wizard/src/lib/post-agent.tsno longercontinues past the report for a directory that errored: the rewritten files are listed with the existing data-destroying warning, the "did not fully complete" warning still fires, and the prompt takes the destructive arm (initialValue: false, "DESTROYS data"). Both facts are true; destruction wins the wording because it is the one a user cannot afford to miss.stash eql migration --drizzleandstash eql install --drizzlenow name any files the sweep rewrote before it failed, instead of pointing at the directory and leaving the user to guess. The shared reporting blocks were hoisted out of thetryso success and partial failure use one path; message wording, ordering, andsweepIncompletesemantics are unchanged.Design note
PartialRewriteErroris thrown only when there is partial work to report. A sweep that fails before changing or flagging anything rejects with the original error object,codeand identity intact — so the "threw on the first file" arm is provably unchanged rather than merely tested-unchanged, and the fail-open-risk surface stays limited to the case that actually gained information.The alternative (returning a result carrying
error) would have been a breaking signature change for both CLI callers and would have removed the throw contractsweepMigrationDirsrelies on. Cost of the chosen shape:PartialRewriteErroris new exported surface in both mirrored rewriter copies.Tests
Both pre-existing failure tests use the EISDIR-on-read trick, which throws before any write — which is why this was invisible. New tests use a delegating
node:fs/promiseswriteFilespy that fails the Nth call, deterministic wherechmodpasses silently as root.Coverage: the rewriter (both mirrored copies), the directory sweep, the wizard report driven end to end through a real failing write, and both CLI callers. No existing test was weakened or deleted —
post-agent.test.ts's empty-error-message test and the two EISDIR tests pass unmodified.The rewriter change lands identically in
packages/wizard/src/lib/rewrite-migrations.tsandpackages/cli/src/commands/db/rewrite-migrations.ts;scripts/__tests__/rewriter-copies-in-sync.test.mjsis green.Changeset:
@cipherstash/wizardpatch +stashpatch.skills/stash-cli/SKILL.mddocuments what a part-way failure now reports.Base
Branched off
remove-v2, notmain— it targets code that only exists there.Summary by CodeRabbit
Bug Fixes
Documentation