fix(skills,wizard,cli): clear the residue from the v2 removal and #823 - #838
Merged
Conversation
🦋 Changeset detectedLatest commit: e2382d2 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 |
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe PR updates wizard migration-sweep reporting, removes stale ChangesWizard migration reporting
Wizard output cleanup
Skill guidance reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/wizard/src/__tests__/format.test.ts`:
- Line 74: Update the relevant format test to assert that the second list item
contains “npx drizzle-kit migrate” and does not contain “stash db push,”
preserving the existing assertions for the other generated content.
🪄 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: 2fc4a901-a162-4efb-b3cf-cde61746aa71
📒 Files selected for processing (12)
.changeset/shipped-skills-v2-removal-residue.md.changeset/wizard-db-push-residue-and-sweep-report.mdpackages/cli/src/__tests__/rewrite-migrations.test.tspackages/wizard/src/__tests__/format.test.tspackages/wizard/src/__tests__/interface.test.tspackages/wizard/src/__tests__/post-agent.test.tspackages/wizard/src/agent/__tests__/interface.test.tspackages/wizard/src/bin/wizard.tspackages/wizard/src/lib/post-agent.tspackages/wizard/src/run.tsskills/stash-drizzle/SKILL.mdskills/stash-encryption/SKILL.md
freshtonic
requested changes
Jul 30, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Reviewed via git show/git grep against origin/fix/issue-837, with PR-relevant tests run in an isolated worktree.
Requesting changes — for one on-thesis missed residue. The actual code and test changes are correct and I'd otherwise approve.
What's right
- The partial-sweep reporting fix in
post-agent.ts(dropping the erroneouscontinue, gating the cross-directory summary on!unverified, presence-checkingerror) is sound, backed by two targeted tests that pass. - The two skill corrections are accurate against the code and CLI registry:
eql migration --drizzle[ --supabase]anddb validateresolve inregistry.ts, and the@cipherstash/stack/clientsubpath thestash-encryptionskill says was removed is indeed absent frompackages/stack/package.json. Two changesets present and correctly scoped (stashpatch for skills,@cipherstash/wizardpatch for code/strings). Prior CodeRabbit point (assert the second list item informat.test.ts) is resolved.
Blocker
packages/wizard/README.md:23,36(major): missed residue of exactly the kind this PR exists to kill. The README still says the wizard "shells out tostash eql install/db pushafter the agent finishes editing" (~L23) and lists post-agent steps as "package install,eql install,db push, framework-specific migrations" (~L36) — the same falsedb pushclaim this PR correctly removes fromrun.ts:243andwizard.ts:43.README.mdis in the wizard package'sfilesarray, so it ships in the@cipherstash/wizardtarball; leaving it recreates the "one copy updated, other left" defect #837 is about. The PR already bumps@cipherstash/wizard, so no extra changeset needed — just edit those two lines to name whatrunPostAgentStepsactually runs (package install,eql install, migrations).- Nit while you're in the file:
run.ts:17has a pre-existing unusedINTEGRATIONSimport (the lone biome warning, author-disclosed) — removing it leaves the changed file warning-free.
Caveat: 6 failures in the full wizard run are all in gateway-messages.test.ts (live-gateway suite, no network/creds here) — not a regression. The deferred err as Partial<RewriteSweepError> cast is legitimately out of scope (→ #836).
Fix the README lines and I'm happy to approve.
Every item here has the same root cause: a change updated one copy of something and left the other. Closes #837. **`stash-encryption` contradicted itself in the customer's repo.** `skills/stash-encryption/SKILL.md:12` pointed at an EQL v2 schema surface "with chainable capability builders" that "still exists", while `:201` and `:1015` of the same file said the v2 builders are gone and v2 is a decrypt-only read path — which is what the code does after #829. The opening callout now says that. It is the first thing an agent reads, and `SKILL_MAP.drizzle` installs this skill into every Drizzle project. **`db push` residue.** `stash db push` was retired with the Proxy lifecycle (#814 / #825) and `post-agent.ts` says it "must never run", but the "Post-agent steps complete" changelog line still claimed it had, and three tests used it as their example of an allowed `stash db` command. The tests now use `db validate`, which is in the manifest. Also dropped "no db pushes" from the `--plan` help text. The `'stash db'` allowlist prefix is unchanged — it still serves `db validate`, `db migrate` and `db test-connection`. **Wizard sweep reporting gap.** When a directory's sweep threw, the `continue` in `rewriteEncryptedMigrations` skipped the per-directory report, so files it had already rewritten on disk went unnamed — `sweepMigrationDirs` propagates that partial set precisely so the caller can report it, and the CLI twin does. Now reported as "Rewrote N migration file(s) in <dir>/ before the sweep stopped", with the file list. Behaviour-neutral otherwise: the SQL is additive and the wizard still throws before the migrate prompt. Two tests pin it, including one that the clean path does not borrow the partial wording; the first fails without the fix. Dropped in the rebase: the `stash-drizzle` add-only correction and the CLI rewriter test assertion this commit originally carried both landed on main independently (91452c1), so the tree already has them. The `stash-drizzle` bullet is gone from the changeset for the same reason. The `err as Partial<RewriteSweepError>` cast at `rewrite-migrations.ts:946` is deliberately untouched — it is a fail-closed correctness defect tracked as item 3 of #836, not cleanup.
…arker
The `stash db push` → `drizzle-kit migrate` fixture swap in the previous commit
was unasserted: the test pinned `pc.dim('1.')` and nothing about item 2, so the
retired command could drift back in and the suite would stay green — which is how
it survived the Proxy-lifecycle removal (#814 / #825) here in the first place.
Pins the rendered second item (`2. Run npx drizzle-kit migrate`) and the absence
of `stash db push`, leaving the existing heading/checkmark/inline-code
assertions untouched. Verified the new assertions fail when the fixture is
reverted to `stash db push`.
**Suppress the cross-directory summary when a sweep failed** (review finding 3). `runPostAgentSteps` prints "Rewrote N migration file(s) in the drizzle output to add staged encrypted columns while preserving the source columns" off `totals.rewritten`, which counts a clean directory and a partially-swept one alike. On the failure path that restated the reassuring framing the new per-directory report deliberately drops, immediately after it. Gated on the failure, not on `skipped` — a flagged statement means the sweep finished and the count is accurate. Test written first and watched fail on the second `info` call. **Cover the flagged statements a failed sweep leaves behind** (review finding 6). Removing the `continue` also let a failed directory report its `skipped` statements, which nothing asserted. Folded into the partial-set test, and verified load-bearing by gating the skipped block on `error === undefined` and watching the assertion fail. **Stop mocking the sweep for the clean path** (review findings 1 and 2). The clean-arm test overrode `sweepMigrationDirs` where a real fixture works, against the convention stated at the top of the file — and that override silently made the file's header comment false, which is exactly the defect class this branch exists to close. Deleted it; its two assertions now live on the existing real-fixture test, anchored on `in drizzle/` so the cross-directory summary cannot satisfy them instead. The header comment now names the two remaining overrides and why each is unreachable through the real filesystem, and each override repeats the reason at its call site. Verified by flipping the ternary: three tests fail, including the folded one. **Name all four skip reasons in the Drizzle skill** (review finding 4). The paragraph listed `source-unknown` and `target-exists` but not `already-encrypted` or `unrecognised-form`, so a reader could infer a hand-authored `SET DATA TYPE … USING …` gets repaired. Also cut the duplicated statement of the declaring-migration requirement. Verified: wizard 344 passed / 5 skipped, cli 895 passed, `tsc --noEmit` clean, biome clean on the changed files. Review finding 5 (skills also ship in the `@cipherstash/wizard` tarball, but the skills changeset is scoped to `stash` alone) is left as-is: it follows repo precedent (`adapter-split-skills.md`), and both tarballs are bumped here anyway. It is a question about the convention, not this branch.
The README still named `db push` in two places — as a `stash` prerequisite
("shells out to `stash eql install` / `db push`") and as a post-agent step.
`runPostAgentSteps` never invoked it, so both lines were the same false claim
this branch removes from `run.ts` and `wizard.ts`. README.md is in the wizard
package's `files` array, so it ships in the tarball: leaving it recreates the
one-copy-updated defect #837 is about.
Also removes the unused `INTEGRATIONS` import from `run.ts`, which was the
file's only biome warning.
The rebase onto main lands `sweep.staged` (#836, item 2) under the mocked `sweepMigrationDirs` result in the partial-set test, which predates it: the fixture had no `staged`, so `totals.staged.push(...staged)` threw "staged is not iterable" and the test failed on the wrong error. The twin is spelled out rather than stubbed to `[]` — the fixture's rewritten `0001_email.sql` is exactly a file that added one, and `rewriteEncryptedMigrations` accumulates twins from a directory whose sweep later threw precisely because they are already on disk and already divergent from `schema.ts` and the snapshot. A stub would have made the fixture contradict the code path it stands in for.
freshtonic
approved these changes
Jul 30, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed at head e2382d29, re-checking the prior findings and the four new commits.
Approving. The new commits cleanly resolve my prior major finding and the nit, and address the CodeRabbit review, without regressing the partial-sweep reporting fix or introducing new inaccuracies.
- ✅ [was major] README
db pushresidue — gone (0fc041ac). The prerequisite line now reads "shells out tostash eql installafter the agent finishes editing" and post-agent step 5 reads "package install,eql install, framework-specific migrations" — matching whatrunPostAgentStepsactually runs. No other shipped doc/string carries the false claim (remainingdb pushhits are the Supabase CLI's ownsupabase db push, tests asserting absence, the e2e retirement test, and comments). - ✅ [was nit] unused
INTEGRATIONSimport (run.ts:17) — removed; Biome now reports zero warnings on the changed files.
Verified the other new commits didn't regress anything
459cf55a("address review findings") changed no runtime behaviour beyond what was already approved — the reporting fix is intact (if (didStage && !unverified),continueremoved with the fall-through report,error === undefinedternary). Its runtime-adjacent change is test-only (dropped an unnecessarysweepMigrationDirsmock on the clean path, moving its assertions onto the real-fixture test — a net move away from mock-behaviour testing). Itsstash-drizzleskill edit adds the two previously-missing skip reasons; all four named reasons map exactly to the realSkipReasonunion atrewrite-migrations.ts:828-836.e2382d29is a legitimate test-only fixture fix (the rebase brought insweep.stagedfrom #836, so the pre-existing mock needed astagedentry to avoid "staged is not iterable"); the added shape matches the realDirRewriteResult/StagedColumn.6a616234is a genuine behavioural assertion (pins the rendered second step + absence ofstash db push).
All PR-relevant suites pass and Biome is clean; changesets present for both stash and @cipherstash/wizard. Note the branch was rebased onto a newer main that absorbed #836/#839/#840, so the authoritative diff is a tight 12 files / +220/-20 (the CLI rewrite-migrations.test.ts change landed via main, not here).
Nice, thorough turnaround.
This was referenced Jul 30, 2026
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
3 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 #837.
Every item in the checklist shares one root cause: a change updated one copy of something and left the other. All were verified present on
main(c8b1325a) before editing.Shipped skills that contradicted each other in the customer's repo
SKILL_MAP.drizzleinstallsstash-encryption,stash-drizzle,stash-indexingandstash-clitogether, andskills/ships inside thestashtarball — so both pairs below landed side by side in a customer's repo and disagreed.skills/stash-drizzle/SKILL.md:64called the sweep's repair "data-destroying", safe "only on an empty table", and told the reader not to apply a swept migration to a table with live data. #823 made the rewriter add-only, so that was false, andskills/stash-cli/SKILL.md:379states the opposite correctly — only thestash-clicopy was updated at the time.I re-verified the replacement text against the code rather than trusting the issue:
<column>_encrypted, preserves the sourcerenderSafeAlteremits a singleALTER TABLE … ADD COLUMN "<col>_encrypted"(packages/cli/src/commands/db/rewrite-migrations.ts:846-873)DROP COLUMN/RENAME COLUMNSkipReasonsource-unknown/target-exists, fail-closed ondeclaredColumnssweepIncomplete→throw new CliExit(1)(packages/cli/src/commands/eql/migration.ts:332-344)skills/stash-encryption/SKILL.md:12pointed at an older EQL v2 schema surface "with chainable capability builders" that "still exists for existing deployments".:201and:1015of the same file say the v2 builders and the@cipherstash/stack/clientsubpath were removed and that v2 is a decrypt-only read path — which matches the code after #829. The pointer resolved to a section asserting its opposite; it now describes the read-compatibility path it actually leads to.db pushresidue in wizard source and testsstash db pushwas retired with the Proxy lifecycle (#814 / #825), andpost-agent.ts:58already says it "must never run".packages/wizard/src/run.ts:243— the "Post-agent steps complete" changelog line claimeddb pushran. It now names whatrunPostAgentStepsactually does: package install,eql install, migrations.agent/__tests__/interface.test.ts:91,__tests__/interface.test.ts:148,__tests__/format.test.ts:74) now use live commands.db validateresolves againststash manifest --json.packages/wizard/src/bin/wizard.ts:43promised "No code or schema changes, no db pushes" in the--planhelp text — same residue, user-visible, so it is fixed here too.The
'stash db'allowlist prefix (packages/wizard/src/agent/interface.ts:58,81) is deliberately unchanged: it still servesdb validate,db migrateanddb test-connection.Wizard sweep: the reporting gap the CLI copy does not have
rewriteEncryptedMigrationswarned about a directory whose sweep threw and thencontinued past the per-directory report — so files it had already rewritten on disk were never named.sweepMigrationDirspropagates that partial set (rewrite-migrations.ts:944-952) precisely so the caller can report it, and the CLI does (eql/migration.ts:316-321).It now reports
Rewrote N migration file(s) in <dir>/ before the sweep stopped:followed by the file list, and falls through to the skipped-statement report as well. Nothing else changes: the rewritten SQL is additive, andpost-agent.ts:81-85still throws before the migrate prompt, so nothing unreviewed runs.Two tests cover it — one that the partial set is named, one that the clean path does not borrow the partial wording. Verified the first fails on the pre-fix
post-agent.tsand passes after.Test drift found while verifying
packages/cli/src/__tests__/rewrite-migrations.test.tslacked the'switch the application to the'assertion its wizard twin already had. The parity guard compares the two source files, not the two test files, so the drift was invisible to CI.Deliberately out of scope
The unchecked
err as Partial<RewriteSweepError>cast atpackages/wizard/src/lib/rewrite-migrations.ts:946is untouched — it is a fail-closed correctness defect (a non-object throw raises aTypeErrorinside the catch and escapessweepMigrationDirs), tracked as item 3 of #836.Per #837's note about the shared paragraph: this PR owns deleting the false "data-destroying" claim at
skills/stash-drizzle/SKILL.md:64. #836 later adds post-sweep reconciliation guidance to the same paragraph. This one must land first — the other order silently reverts the correction.Changesets
stashpatch — the two shipped skills (skills/ships in the tarball).@cipherstash/wizardpatch — thedb pushstrings and the sweep report.No
@cipherstash/stackchangeset: thestash-encryptionwording change is a skills-only edit, and skills ship in thestashand@cipherstash/wizardtarballs, both of which are bumped.Verification
biome checkreports one warning inrun.ts— an unusedINTEGRATIONSimport that predates this branch (confirmed by stashing the change and re-running). CI gates on errors only.Summary by CodeRabbit
Documentation
Bug Fixes
Tests