Repository navigation
Conversation
Customers upgrading existing Protect data need a store-specific procedure distinct from plaintext backfill. Port the verified Stack guide at its immutable source revision, preserve its compatibility and rollback boundaries, and add explicit customer staging acceptance checks. Refs cipherstash/stack#1540, cipherstash/stack#1604.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Pin the customer page to the source revision that includes dependency checks directly and explains safe refusal on repeated conversion. Keep the added customer rehearsal checklist separate from the verified procedure. Refs cipherstash/stack#1620.
tobyhede
left a comment
tobyhede
left a comment
There was a problem hiding this comment.
Review attestation
Reviewed 33e9218 · posted from Claude Code · claude-opus-5-5 · effort medium
Sources: code-review · claude-opus-5-5
Skills: code-review
Findings (8): 8 minor — 8 inline, 0 below.
| ```typescript filename="migration-example-8.ts" | ||
| import postgres from 'postgres' | ||
|
|
||
| const sql = postgres(process.env.DATABASE_URL) |
There was a problem hiding this comment.
minor postgres(process.env.DATABASE_URL) with an unset URL falls back to the libpq PG* defaults, so the script can take ACCESS EXCLUSIVE and run DDL against whatever database those point at. Throw if DATABASE_URL is missing before connecting.
Source: code-review · claude-opus-5-5
| p.ob != null || p.bf != null || | ||
| (column === 'notes' ? p.hm != null : typeof p.hm !== 'string')) | ||
| throw new Error(`Unsupported legacy payload in ${column}`) | ||
| const plain = ok(await client.decrypt(p), 'decrypt source') |
There was a problem hiding this comment.
minor Under ACCESS EXCLUSIVE the script makes about 11 serial ZeroKMS calls per row (three columns), and step 6 decrypts every source value again even though step 2 already produced plain. At 10k rows and ~50 ms per call that is roughly 90 minutes of table lock. Keep the step-2 plaintexts for the step-6 comparison and use bulkDecrypt and batch encryptQuery.
Source: code-review · claude-opus-5-5
| const term = ok(await client.encryptQuery(plain, { | ||
| table: customers, column: customers[column], | ||
| }), 'recompute term') | ||
| if (term?.hm !== p.hm) throw new Error(`Equality term mismatch in ${column}`) |
There was a problem hiding this comment.
minor term?.hm reads a member of the encryptQuery union without narrowing, which the guide's own hmacOf example says to do, so strict tsc rejects it; function ok(result, step) is also implicit any. Narrow with hmacOf and type ok.
Source: code-review · claude-opus-5-5
|
|
||
| The executable example accepts one primary key and the three source columns above. It refuses secondary indexes, non-primary constraints, dependent views, user triggers, row-level security and inheritance. Those objects need a reviewed migration of their own before using this example. Do not drop them merely to make preflight pass (`postgres-upgrade › refuses customer views before changing source data or columns`). | ||
|
|
||
| For an existing database, retain its v2 SQL installation. Install v3 alongside it using the current `stash eql install` against the rehearsed database. Do not uninstall v2 or drop its schemas during this procedure. Capture and verify existing indexes and dependencies around installation, as well as around data conversion. |
There was a problem hiding this comment.
minor The guide pins Stack 1.2.1 and EQL v3 but installs with an unpinned "current stash eql install" (also lines 269 and 482), so a later stash release can install SQL this procedure was never tested against. Pin the CLI, e.g. npx --package=stash@X.Y.Z stash eql install.
Source: code-review · claude-opus-5-5
| EXISTS (SELECT 1 FROM pg_trigger WHERE tgrelid = 'customers'::regclass AND NOT tgisinternal) | ||
| OR EXISTS (SELECT 1 FROM pg_constraint WHERE (conrelid = 'customers'::regclass OR confrelid = 'customers'::regclass) | ||
| -- PostgreSQL 18 also lists the primary key column's NOT NULL here. | ||
| AND NOT (contype IN ('p', 'n') AND conrelid = 'customers'::regclass AND conkey = ARRAY[1]::smallint[])) |
There was a problem hiding this comment.
minor The preflight hardcodes the primary key as attribute 1 (conkey = ARRAY[1], and attnum > 1 at line 361). On a table where a column before pk was dropped, pk has attnum 2 and a valid table is refused as having unsupported constraints. Compare against pk's actual attnum.
Source: code-review · claude-opus-5-5
| - `postgres-upgrade` is `e2e/tests/protect-postgres-upgrade.e2e.test.ts`. | ||
| - `example` is `e2e/tests/protect-guide-example.e2e.test.ts`. | ||
|
|
||
| Both suites produce item objects with the real Protect.js versions above, installed from npm, and read them back with the in-tree Stack package (currently 1.2.1). They test live encryption, but do not contact DynamoDB: item persistence and HMAC matching are simulated in memory. The crossover suite also runs real Postgres inserts and SQL queries. This verifies adapter compatibility, not AWS expressions, GSI configuration or a deployed customer application. They run on every pull request in the `e2e-tests` job of `.github/workflows/tests.yml`. Both need `CS_WORKSPACE_CRN`, `CS_CLIENT_ID`, `CS_CLIENT_KEY` and `CS_CLIENT_ACCESS_KEY`, supplied in the environment, `e2e/.env` or the repository-root `.env`. The crossover suite also needs `DATABASE_URL` for a disposable Postgres database where it can install EQL and create tables. From an installed repository checkout, build the native binding and run through Turbo so workspace dependencies are built: |
There was a problem hiding this comment.
minor "Both suites produce item objects…" follows a list of four suites, and postgres-upgrade produces no DynamoDB items, so the in-memory-persistence claim appears to cover it. Name the suites each sentence applies to.
Source: code-review · claude-opus-5-5
| Both suites produce item objects with the real Protect.js versions above, installed from npm, and read them back with the in-tree Stack package (currently 1.2.1). They test live encryption, but do not contact DynamoDB: item persistence and HMAC matching are simulated in memory. The crossover suite also runs real Postgres inserts and SQL queries. This verifies adapter compatibility, not AWS expressions, GSI configuration or a deployed customer application. They run on every pull request in the `e2e-tests` job of `.github/workflows/tests.yml`. Both need `CS_WORKSPACE_CRN`, `CS_CLIENT_ID`, `CS_CLIENT_KEY` and `CS_CLIENT_ACCESS_KEY`, supplied in the environment, `e2e/.env` or the repository-root `.env`. The crossover suite also needs `DATABASE_URL` for a disposable Postgres database where it can install EQL and create tables. From an installed repository checkout, build the native binding and run through Turbo so workspace dependencies are built: | ||
|
|
||
| ```bash | ||
| pnpm --filter @cipherstash/protect-ffi run build:native |
There was a problem hiding this comment.
minor A developer-audience page publishes maintainer-only commands (build:native with Rust and mise, Turbo tasks in the stack repo) that fail in a customer's app; line 47 itself says they are for maintainers. Move them to the stack repo and link from here.
Source: code-review · claude-opus-5-5
| { | ||
| id: "eql-v2-encrypted-type", | ||
| // This upgrade guide must identify the retained legacy source columns. | ||
| exempt: ["content/docs/guides/protect-to-stack.mdx"], |
There was a problem hiding this comment.
minor exempt turns eql-v2-encrypted-type off for the whole page, against the Rule docstring that limits it to pages documenting a component that still ships the API. A later edit teaching eql_v2_encrypted for new columns would pass. Use the line-level RETIRED_MARKER on the lines naming legacy source columns instead.
Source: code-review · claude-opus-5-5
Keep the customer page aligned with the generated and executable source guide. Source plaintexts now come from Protect, the standalone migration tool retains both pinned SDKs, and the code is checked for strict types and drift instead of maintained as a handwritten copy. Refs cipherstash/stack#1604, cipherstash/stack#1540
Summary
Add the customer Protect-to-Stack guide for DynamoDB and PostgreSQL, targeting Stack 1.2.1 and EQL v3. The PostgreSQL script is a verbatim port of the canonical executable conversion, with independent Protect-to-Stack plaintext verification.
Changes
e7d6a02d3d92f466e57175711efb56e9f4bdf544, guide SHA-2566e730d48fdba975c55273f3c9d58f27c21928babe8607c862a6b0eca996bc242.21631ecf68fd5356fdd4c2631550a46931123dc4.eql_v2_encrypted.Verification
5259b44995b09ab7d3cac62003e77d3b68c58c00is signed and verified.content/docs/security/meta.jsonandscripts/fixtures/stash-manifest.json.46106cc0…; they are historical and do not validate this revised script. No new model run, customer staging migration or AWS deployment test was performed for this port.Related
Refs cipherstash/stack#1540, cipherstash/stack#1604, cipherstash/stack#1620.
Review notes
The page targets EQL v3, not v4. Publication and customer-specific staging/cutover/rollback rehearsal remain separate gates. This PR has not been merged or deployed.