feat(skills): add stash-deployment, install it for every integration - #826
Conversation
🦋 Changeset detectedLatest commit: b95b3e9 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds the ChangesDeployment skill rollout
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant DeployedApp
participant BackfillJob
participant Database
Operator->>DeployedApp: Deploy dual-write code
DeployedApp->>Database: Write plaintext and encrypted twin
Operator->>BackfillJob: Run coverage backfill
BackfillJob->>Database: Populate missing ciphertext
Operator->>DeployedApp: Deploy read cutover
DeployedApp->>Database: Read encrypted twin
Operator->>DeployedApp: Deploy stop-dual-write code
Operator->>Database: Apply guarded plaintext drop
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
ab65046 to
3d21324
Compare
The rollout/cutover lifecycle was documented in `stash-encryption` and `stash-cli` as a command sequence, with the deploy boundaries described in passing. The boundaries are the hard part. An agent that reads the lifecycle as one unit of work — twin column, dual-write, backfill, read switch, drop — produces a plan that loses data, because ciphertext can only be written by the application and the plaintext column has to stay authoritative until every row has a ciphertext twin and the deployed code reads it. `stash-deployment` makes the deploy shape the subject: - The three-deploy ladder with its two out-of-band steps and two human gates, and what each gate is actually verifying. - A failure table: for each way of collapsing the ladder, the data that is lost. - Rollback per stage, making explicit that only the final drop is irreversible. - `CS_*` as a build-time input wherever the encryption client is constructed at module load, and the keyset rule for backfills — ciphertext written under credentials the deployed app does not resolve to fails only at read time. - A Prisma Postgres / Prisma Compute section: EQL installing through the Prisma Next migration graph, one merge deploying one stage, the additive-only deploy policy that makes the plaintext drop fail the build (and the apply-before-merge sequence that avoids it), preview-branch databases masking destructive migrations and inviting a wrong-database apply, and one-off jobs against a hosted database. Scoped to EQL v3 throughout, with the v2 tail pointed back at `stash-encryption`. Joins `stash-encryption`, `stash-indexing` and `stash-cli` in the set every integration installs, so `SKILL_MAP` (both the CLI's and the wizard's), `BASE_SKILLS`, `SKILL_PURPOSES`, and the doctrine's skill list gain an entry, and the tests that pin those arrays are updated.
3d21324 to
126faef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@skills/stash-deployment/SKILL.md`:
- Around line 58-70: Add a language identifier such as text to both fenced code
blocks containing the deployment rollout diagram in the deployment
documentation, including the additionally referenced block, so Markdown linting
and deterministic syntax highlighting are satisfied.
- Around line 301-305: Pin the Prisma CLI versions for all production operation
commands in this section, including database list, connection create, connection
remove, database show, and db update. Use the exact tested versions for both
`@prisma/cli` and prisma-next, or invoke the repository’s installed local binaries
consistently.
🪄 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: 07960dd6-431a-4a14-b6b4-c3e8231ae5a2
📒 Files selected for processing (9)
.changeset/stash-deployment-skill.mdAGENTS.mdpackages/cli/src/commands/init/doctrine/AGENTS-doctrine.mdpackages/cli/src/commands/init/lib/__tests__/install-skills.test.tspackages/cli/src/commands/init/lib/install-skills.tspackages/cli/src/commands/init/lib/setup-prompt.tspackages/wizard/src/__tests__/install-skills.test.tspackages/wizard/src/lib/install-skills.tsskills/stash-deployment/SKILL.md
Review notes —
|
|
@freshtonic to review |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed the new skill and its install wiring in an isolated worktree, cross-checking every technical claim against the CLI registry, main.ts, and the co-installed sibling skills.
Requesting changes. The wiring is correct and complete — SKILL_MAP + BASE_SKILLS in both CLI and wizard, the SKILL_PURPOSES blurb, the doctrine + AGENTS.md skill lists, and all pinned test arrays are updated consistently; the two unit suites pass (CLI 98, wizard 3), Biome is clean, a valid changeset is present, and the skill is a self-sufficient single SKILL.md (nothing dropped by readBundledSkill inlining). But the shipped content — which lands verbatim in customer repos, where a wrong sentence is a wrong line of code — has four confirmed accuracy defects. These independently reproduce coderdan's own review notes.
Major (block merge)
SKILL.md:158— presentsstash encrypt cutoveras a live EQL v2 command that just "does not apply to a v3 column." It was removed:packages/cli/src/bin/main.ts:378throwsCliExit(1)unconditionally on every project, and it's absent from the registry. An agent told it still exists will run it and hit an exit-1 removal error.SKILL.md:21— same dead command in the intro's v2 path ("a rename swap viastash encrypt cutover, then dropping<col>_plaintext"). The cross-reference is also dead —stash-encryptiondocuments no<col>_plaintext/cutover path. Point the v2 case at the manual path that skill actually documents.SKILL.md:172-178, 304, 342— Deploy 3 write-failure window: the Prisma sequence applies the destructive plaintext-drop migration to production before merging the code that stops dual-writing (repeated at :342). During that window the still-deployed app dual-writes to a dropped column → every insert/update fails withcolumn "..." does not exist. The ":291 no outage" claim only holds for the failed-db initcase. Split into code-first (stop writing plaintext → deploy → drop) or scope the window explicitly.SKILL.md:142— instructs creatingeql_v3.*extractor indexes out-of-band, directly contradictingskills/stash-prisma-next/SKILL.md:157("Never run index DDL out-of-band") — a skill this PR now co-installs with it on the prisma-next path. Make the index step integration-conditional (its own migration/PR for Prisma) and add it to the stage table at :338-342, which currently omits it.
Minor / nits
SKILL.md:86, 358— the bundling prerequisite offers only native-module externalization (serverExternalPackages), no@cipherstash/stack/wasm-inlinepath. This skill is installed for thesupabaseand genericpostgresqlintegrations whose flagship targets (Edge Functions, Workers, Deno) can't load the native module at all. Add the WASM branch and linkstash-edge(co-installed but absent from this skill's Related list). Partly mitigated bystash-edgebeing installed alongside.- Nit:
SKILL.md:~58-70ASCII ladder fenced blocks lack a language identifier (use ```text) for markdown-lint. - Nit:
SKILL.md:301-305some Prisma production commands are unpinned while others in the skill are version-stamped; be consistent.
Verified correct: the claimed-live commands plan, impl, status, env (--name/--json), eql install, encrypt backfill (--confirm-dual-writes-deployed/--force), and encrypt drop all resolve in registry.ts — only encrypt cutover is absent. Changeset (stash minor + wizard patch) satisfies the skills-only requirement; AGENTS.md skills list/table updated; SECURITY.md needs no change.
Happy to re-review once the four content items are corrected — the structure and wiring are otherwise solid.
…loy 3 to close the write-failure window - stash encrypt cutover was removed from the CLI (exits 1 on every project); the intro's legacy-v2 path and the Deploy 2 parenthetical now describe the manual v3-shaped finish (backfill -> switch reads by name -> encrypt drop) - Restructure the ladder to four deploys: read cutover -> stop dual-writes (Deploy 3, code-only) -> drop plaintext (Deploy 4, migration-only), with a Gate 3 between them. Dropping the column while dual-writing code still serves traffic fails every insert/update; the Prisma apply-before-merge sequence is now explicitly gated on Deploy 3 being live - Index DDL ships through the integration's migration flow, never ad-hoc — aligns with stash-prisma-next's 'never run index DDL out-of-band' and stash-indexing's placement guidance; added as a stage in the Prisma PR table - Add the wasm-inline bundling branch for edge runtimes and stash-edge to Related skills; new troubleshooting rows for both failure modes - text language tags on the ladder and PN-CLI-4020 fences; scope the 'no outage' claim to the failed build itself
…dentity Same-credentials was the hard-line shorthand; the real requirement is that the backfill encrypts under the keyset the deployed app resolves — any client granted that keyset works. Defer the model to stash-zerokms and the credential resolution order to stash-auth (stacked above this PR), and add both to Related skills. Also disambiguate 'keyset-paginated' (SQL sense) from ZeroKMS keysets.
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Re-reviewed at head b95b3e920 (commits 9823eab9 + b95b3e92), re-checking each prior finding and watching for regressions.
All four of my prior MAJOR findings are resolved cleanly:
- ✅ Dead
stash encrypt cutover(:20-24, :182-184) — now accurately described as removed ("exits with an error on every project"), matchingmain.ts:378; the v2 case points at the real v3 flow (backfill → switch reads →stash encrypt drop), no dead<col>_plaintextcross-ref. - ✅ Deploy 3 write-failure window (
:197-223, :239-249, :349-368) — now code-first: Deploy 3 stops dual-writes (relaxingNOT NULL), Deploy 4 drops the column in a separate migration-only deploy, and the "no outage" claim is correctly scoped to the failed-build case. A "Why the boundaries" row names the exact hazard. - ✅ Out-of-band index DDL (
:163-169, :396) — now integration-conditional ("never out-of-band there — seestash-prisma-next"), consistent withstash-prisma-next:157, with its own row in the stage table. - ✅ WASM/edge path (
:100-104, :448, :464) —@cipherstash/stack/wasm-inlinebranch added, troubleshooting row, andstash-edgein Related skills.
Also confirmed: the backfill-invariant rephrase in b95b3e92 is technically correct — the invariant turns on keyset/grant identity ("any client granted access to that keyset works"), not credential identity, matching ZeroKMS semantics. Every stash command/flag the edits touched still resolves against registry.ts, and all opening fenced blocks now carry language ids. Install wiring is byte-identical to the head I tested last round (98 CLI + 3 wizard unit tests passing, Biome clean).
However — one NEW blocker introduced by the rewrite:
- [major]
SKILL.md:158-159, 294, 451, 462-463— six references tostash-zerokmsandstash-auth, neither of which exists on the branch, in anySKILL_MAP, or in the bundledskills/directory (only cli, deployment, drizzle, dynamodb, edge, encryption, indexing, postgres, prisma-next, supabase, supply-chain-security exist). The "Related skills" section lists both as real, so an agent in a customer repo following these pointers finds nothing. Fix (trivial): drop them, or redirect to skills that exist and are co-installed —CS_*/credential resolution is covered bystash-edge; auth strategies live instash-encryption; there is no keyset/grant skill, so fold that pointer intostash-encryptionor remove it.
Nit (non-blocking, defensible deferral): :353-362, :382 — Prisma CLI commands remain unpinned; the PR reasons about this (Prisma Compute beta volatility) and the section advises verifying against current CLI help.
Nice turnaround on the four majors — this is the only thing standing between here and approve.
There IS! They are stacked on this PR. |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approving because my agent does not apparently understand stacked PRs yet and didn't notice that the blocker is in fact done.
4 participants
Why
The rollout/cutover lifecycle is documented in
stash-encryptionandstash-clias a command sequence, with the deploy boundaries described in passing. The boundaries are the hard part.An agent that reads the lifecycle as one unit of work — twin column, dual-write, backfill, read switch, drop — produces a plan that loses data. Ciphertext can only be written by the application, client-side, so no
UPDATEand no migration can encrypt an existing row; the plaintext column has to stay authoritative until every row has a ciphertext twin and the deployed code reads it. Nothing in the tooling makes that shape obvious from the command list alone.This came out of a real staged rollout onto Prisma Compute, where every interesting failure was a deployment failure rather than an API one.
What
New
skills/stash-deployment/SKILL.md:CS_*as a build-time input wherever the encryption client is constructed at module load (local builds mask this via the device profile), plus the keyset rule for backfills: ciphertext written under credentials the deployed app does not resolve to fails only at read time.prisma-next migrate), one merge deploying one stage, the additive-only deploy policy that makes the plaintext drop fail the build and the apply-before-merge sequence that avoids it, preview-branch databases masking destructive migrations and inviting a wrong-database apply, and running one-off jobs against a hosted database.Scoped to EQL v3 throughout, with the v2 tail (
encrypt cutoverrename swap,<col>_plaintext) pointed back atstash-encryption.Registration
stash-deploymentjoinsstash-encryption,stash-indexingandstash-cliin the set every integration installs — deployment safety is cross-cutting in the same way indexing is (#753).SKILL_MAP+BASE_SKILLSinpackages/cli/.../install-skills.ts, and the wizard's copySKILL_PURPOSESblurb insetup-prompt.tsAGENTS.mdskill list and the "if you change… check…" tableAGENTS-doctrine.mdcommon-skills lineVerification
pnpm --filter stash exec vitest run— 857 passedpnpm --filter @cipherstash/wizard exec vitest run— 290 passedpackages/cli/src/cli/registry.tson this branch; the Prisma commands againstprisma-next0.14.0 and@prisma/cli3.0.0-beta.29Changeset:
stashminor,@cipherstash/wizardpatch.Notes for review
remove-v2rather thanmain, since it describes the post-removal v3-only world.database listproduction/preview ambiguity in particular) is feedback we have open with Prisma.Summary by CodeRabbit
stash-deploymentskill with guidance for safely rolling out encryption changes through staged deployments, verification gates, backfills, rollback procedures, and credential handling.Summary by CodeRabbit
New Features
stash-deploymentskill to supported integrations and default installations.Documentation
Tests