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
Summary
Add a customer guide for upgrading existing Protect applications to Stack 1.2.1 and EQL v3 on DynamoDB and PostgreSQL. It preserves the tested procedure's supported mappings, verification steps and rollback boundaries, and adds an explicit checklist for rehearsal against the customer's own deployment.
Changes
/guides/protect-to-stackbeside the existing plaintext data migration guide in navigation.eql_v2_encryptedtype rule, since it must identify retained source columns. All other rules continue to apply.Verification
Fresh independently audited guide-only runs used source guide SHA-256
46106cc0a71dbeb757bcb0b3889c6fc5741bac201afe807ed8cde0a035d3d649and harness5261d3a5c0ae6a4750a7a1102d08436a1d314725:claude-sonnet-5-5on the existing Colima Linux host, outside CI.Latest synchronized page at
33e9218:bun run types:check, content and internal-link validation passed.next buildpassed on retry using the previously generated references. The first attempt reported an async panic without a source diagnostic; it did not reproduce.Initial publication preparation at
ad770c3:bun install --frozen-lockfilepassed; sandbox temp-directory denial required an approved retry.bun run buildpassed, including reference generation, content/diagram/link/redirect validation, production compilation and TypeScript. Unrelated tracked EQL generator output was restored afterward.bun run types:checkpassed on an approved retry. The initial sandbox run produced an empty generated module and consequently failed type checking.git diff --checkpass. Fullbun run lintstill reports two pre-existing formatting failures incontent/docs/security/meta.jsonandscripts/fixtures/stash-manifest.json; both were reproduced from unchanged HEAD contents.ad770c3and33e9218have verified signature statusG.Related
Refs cipherstash/stack#1540, cipherstash/stack#1604, cipherstash/stack#1620.
Review notes
The frozen source guide remains unchanged. This page targets Stack 1.2.1 and EQL v3; it makes no EQL v4 compatibility claim. The added rehearsal checklist does not expand the proven mappings or claim customer acceptance has completed. This is a draft publication PR; it has not been merged or deployed.
The repository's Docs workflow currently targets
v2, while its default branch ismain; the local validation above is explicit evidence rather than a claim that this PR triggered that workflow.