From 66942a078b857dfa73e4a2dd8dcd8d82542c2282 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:43:55 +1000 Subject: [PATCH 1/5] fix(skills,stack,scripts): correct shipped decrypt guidance, and make the package-path linter sound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 14 — decryptModel/bulkDecryptModels return an AuditableDecryptModelOperation: thenable, with .withLockContext() and .audit(). Three sites in skills/stash-encryption and four in packages/stack/README.md said the opposite ("a plain Promise>", "no .withLockContext() to chain"), steering agents away from the very .audit() chain the audit-on-decrypt changeset advertises. The skill contradicted its own reference table, which was already right — 8b744309 fixed that table and nothing else, despite a commit message claiming otherwise. Both files ship: the skill inside the stash tarball and thence into customer repos via installSkills(), the README inside the @cipherstash/stack tarball. The equivalent statement about the WASM entry is CORRECT and deliberately untouched — that client really does return a bare promise. Also `protectOps.eq` in the setup prompt stash init writes for coding agents. One occurrence repo-wide, and no such export exists; the real API is createEncryptionOperators(client), conventionally `ops`. Finding 15 — the package-path linter had a false positive and a false negative, both fixed with the fixture-driven self-tests the suite already uses: - The name capture had no right anchor, so a sentence-final `packages/stack.` swallowed the period and reported a LIVE package as dead. Uppercase was also excluded from the class, so a capitalised name was never checked at all. - livePackages came from readdirSync, i.e. the working tree. Deleting a package leaves dist/ and node_modules/ behind, so every reference to it kept passing — the exact case the linter was commissioned to catch, silently unenforced on any checkout that had built the package. Now derived from `git ls-files`. Deliberately not "has a package.json": packages/utils has none and is live. - scripts/ was not scanned, so the linters never checked themselves. Adding it immediately surfaced a dead allowlist entry in lint-no-hardcoded-runners for a path that never existed in git history. Self-test fixtures stay exempt — they must name dead packages. Severity note for the record: CI was never affected (fresh checkout, caching disabled), and main carries no required status checks — this was a local developer papercut plus a real soundness hole, not a broken build. Finally, resolves two changesets that contradicted each other in the same release: dynamodb-eql-v3 claimed "EQL v2 tables continue to work unchanged" and "no existing caller needs to change" while stack-dynamodb-v2-write-removal announced the v2 encrypt overloads as removed. The code sides with removal; the claims are now scoped to the decrypt path. --- .changeset/decrypt-chaining-docs.md | 27 +++++++++++ .changeset/dynamodb-eql-v3.md | 13 +++-- .../cli/src/commands/init/lib/setup-prompt.ts | 2 +- packages/stack/README.md | 12 +++-- .../sentence-final.md | 7 +++ .../lint-no-dead-package-paths/uppercase.md | 3 ++ .../lint-no-dead-package-paths.test.mjs | 28 +++++++++++ scripts/lint-no-dead-package-paths.mjs | 47 ++++++++++++++++--- scripts/lint-no-hardcoded-runners.mjs | 1 - skills/stash-encryption/SKILL.md | 6 +-- 10 files changed, 125 insertions(+), 21 deletions(-) create mode 100644 .changeset/decrypt-chaining-docs.md create mode 100644 scripts/__tests__/fixtures/lint-no-dead-package-paths/sentence-final.md create mode 100644 scripts/__tests__/fixtures/lint-no-dead-package-paths/uppercase.md diff --git a/.changeset/decrypt-chaining-docs.md b/.changeset/decrypt-chaining-docs.md new file mode 100644 index 000000000..f2a40d9f7 --- /dev/null +++ b/.changeset/decrypt-chaining-docs.md @@ -0,0 +1,27 @@ +--- +'stash': patch +'@cipherstash/stack': patch +--- + +Correct the shipped documentation for `decryptModel` / `bulkDecryptModels`. + +Three places in `skills/stash-encryption` and four in `packages/stack/README.md` +said these return "a plain `Promise>` (not a chainable operation)" +and that there is therefore "no `.withLockContext()` to chain". They return an +`AuditableDecryptModelOperation`, which is thenable and carries both +`.withLockContext()` and `.audit()` — the same `.audit()` chain the +audit-on-decrypt work advertises. The skill contradicted itself: its own +reference table already listed the correct return type. + +The skill ships inside the `stash` tarball and `installSkills()` copies it into +customer repos, so this was steering agents away from an API that exists. The +README ships in the `@cipherstash/stack` tarball. + +The equivalent statement about the **WASM entry** is correct and unchanged — +`@cipherstash/stack/wasm-inline` really does return a plain promise from decrypt, +with no lock-context argument. + +Also fixes the setup prompt `stash init` writes for coding agents, which +referenced `protectOps.eq` — an API that does not exist anywhere in the repo. +The operators come from `createEncryptionOperators(client)`, conventionally +bound to `ops`. diff --git a/.changeset/dynamodb-eql-v3.md b/.changeset/dynamodb-eql-v3.md index c6303c997..3763c2ee5 100644 --- a/.changeset/dynamodb-eql-v3.md +++ b/.changeset/dynamodb-eql-v3.md @@ -11,10 +11,13 @@ Pass a table built with `encryptedTable` + the `types.*` domains from the typed client from `EncryptionV3` and the nominal client from `Encryption({ config: { eqlVersion: 3 } })` are accepted. -EQL v2 tables continue to work unchanged — this is additive, and no existing -caller needs to change. The table decides which wire format is used, so a -DynamoDB table populated under one version must keep being read with that -version. +EQL v2 tables continue to be **readable** — `decryptModel` / +`bulkDecryptModels` still accept one, so existing items stay accessible. Writing +through a v2 table is a separate matter: the encrypt overloads for it are +removed in this same release (see the DynamoDB v2 write-removal entry), so a +caller that still encrypts through a v2 table does need to change. The table +decides which wire format is used, so a DynamoDB table populated under one +version must keep being read with that version. This fixes a latent bug that made v3 unusable: the write path detected an encrypted value by its `k: 'ct'` tag, but EQL v3 scalars carry no `k` @@ -71,4 +74,4 @@ type, where a declared column `email` becomes `email__source` (plus `email`. `decryptModel` / `bulkDecryptModels` invert it via `DecryptedAttributes`. `AnyEncryptedTable`, `DynamoDBEncryptionClient` and `AuditConfig` are now exported from `@cipherstash/stack/dynamodb` so these signatures can be named. -The EQL v2 overloads are unchanged. +The EQL v2 **decrypt** overloads are unchanged; the v2 encrypt overloads are removed in this release. diff --git a/packages/cli/src/commands/init/lib/setup-prompt.ts b/packages/cli/src/commands/init/lib/setup-prompt.ts index c0e22ba21..12b149aae 100644 --- a/packages/cli/src/commands/init/lib/setup-prompt.ts +++ b/packages/cli/src/commands/init/lib/setup-prompt.ts @@ -280,7 +280,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { "2. Edit the user's real schema file (`src/db/schema.ts` or wherever they keep it) to declare the new encrypted column. Use the patterns in the integration skill — the `types.*` domain factories from `@cipherstash/stack-drizzle` for Drizzle, and the `types.*` factories from `@cipherstash/stack/eql/v3` (via `encryptedTable`, passed as `schemas`) for Supabase. Encrypted columns must be **nullable `jsonb`** at creation time. Never `.notNull()`.", `3. Generate the schema migration${migration ? ` — \`${migration.generate}\` (${migration.tool})` : " using the project's existing migration tooling"}.`, `4. Show the user the generated SQL before applying${migration ? ` — \`${migration.apply}\`` : ''}.`, - '5. Wire the column through the application code: insert paths encrypt before write, select paths decrypt after read, query paths use the right operator (`protectOps.eq`, etc. — see the integration skill).', + '5. Wire the column through the application code: insert paths encrypt before write, select paths decrypt after read, query paths use the right operator (`ops.eq`, from `createEncryptionOperators(client)` — see the integration skill).', '6. Verify with a round-trip: insert a record, select it back, confirm the value decrypts and the search ops work.', '', '### Migrate an existing column to encrypted', diff --git a/packages/stack/README.md b/packages/stack/README.md index ae45f1ea6..9e5710eda 100644 --- a/packages/stack/README.md +++ b/packages/stack/README.md @@ -528,8 +528,10 @@ same claim must be supplied to encrypt and decrypt. Lock contexts work with all operations: `encrypt`, `decrypt`, `encryptModel`, `decryptModel`, `bulkEncryptModels`, `bulkDecryptModels`, `bulkEncrypt`, `bulkDecrypt`, `encryptQuery`. `.withLockContext()` also accepts a `LockContext` instance. -On the typed client, `decryptModel` / `bulkDecryptModels` take the lock -context as an optional third argument instead of chaining. +On the typed client, `decryptModel` / `bulkDecryptModels` additionally accept +the lock context as an optional third argument. Use that or `.withLockContext()`, +not both — chaining onto a decrypt that already took a positional lock context +throws. > **Deprecated: `LockContext.identify()`.** Per-operation CTS tokens were removed > in `protect-ffi` 0.25; the token `identify()` fetches is no longer used by @@ -707,14 +709,14 @@ Method signatures are derived from your schemas: plaintext arguments are pinned `returnType` controls the encrypted query term's shape: `'eql'` (default, the EQL JSON payload for the ORM adapters), `'composite-literal'` (a Postgres composite string for `.eq()`/string-based APIs), or `'escaped-composite-literal'` (the same, escaped for embedding). Most users take the default; the adapters set it as needed. | `encryptQuery` | `(terms: ScalarQueryTerm[])` | `BatchEncryptQueryOperation` (thenable) | | `encryptModel` | `(model, table)` | `EncryptModelOperation` (thenable) | -| `decryptModel` | `(encryptedModel, table, lockContext?)` | `Promise>` | +| `decryptModel` | `(encryptedModel, table, lockContext?)` | `AuditableDecryptModelOperation` (thenable) | | `bulkEncryptModels` | `(models, table)` | `BulkEncryptModelsOperation` (thenable) | -| `bulkDecryptModels` | `(encryptedModels, table, lockContext?)` | `Promise>` | +| `bulkDecryptModels` | `(encryptedModels, table, lockContext?)` | `AuditableDecryptModelOperation` (thenable) | | `bulkEncrypt` | `(plaintexts, { column, table })` | `BulkEncryptOperation` (thenable) | | `bulkDecrypt` | `(encryptedPayloads)` | `BulkDecryptOperation` (thenable) | | `getEncryptConfig` | `()` | The resolved encrypt config | -The thenable operations support `.withLockContext(lockContext)` for identity-aware encryption. `decryptModel` / `bulkDecryptModels` return a plain `Promise` instead — pass the lock context as the optional third argument. `decrypt` of a single value cannot be strongly typed (a lone ciphertext carries no column identity), and `encryptQuery` rejects storage-only columns at compile time. +The thenable operations support `.withLockContext(lockContext)` for identity-aware encryption, and `decryptModel` / `bulkDecryptModels` also support `.audit({ metadata })`. Those two additionally accept the lock context as an optional third argument — use one form or the other. `decrypt` of a single value cannot be strongly typed (a lone ciphertext carries no column identity), and `encryptQuery` rejects storage-only columns at compile time. ### `LockContext` (legacy) diff --git a/scripts/__tests__/fixtures/lint-no-dead-package-paths/sentence-final.md b/scripts/__tests__/fixtures/lint-no-dead-package-paths/sentence-final.md new file mode 100644 index 000000000..7ae81cfb4 --- /dev/null +++ b/scripts/__tests__/fixtures/lint-no-dead-package-paths/sentence-final.md @@ -0,0 +1,7 @@ +# Sentence-final package paths + +The client lives in packages/stack. +A hyphen at a wrap: packages/migrate- +An underscore: packages/wizard_ +An ellipsis: packages/cli... +Parenthesised (packages/stack) and comma'd packages/cli, both fine. diff --git a/scripts/__tests__/fixtures/lint-no-dead-package-paths/uppercase.md b/scripts/__tests__/fixtures/lint-no-dead-package-paths/uppercase.md new file mode 100644 index 000000000..a2060a9c9 --- /dev/null +++ b/scripts/__tests__/fixtures/lint-no-dead-package-paths/uppercase.md @@ -0,0 +1,3 @@ +# Uppercase package names must still be checked + +See packages/Foo/does/not/exist for details. diff --git a/scripts/__tests__/lint-no-dead-package-paths.test.mjs b/scripts/__tests__/lint-no-dead-package-paths.test.mjs index bc9ea927f..c0dacca7b 100644 --- a/scripts/__tests__/lint-no-dead-package-paths.test.mjs +++ b/scripts/__tests__/lint-no-dead-package-paths.test.mjs @@ -43,6 +43,34 @@ describe('lint-no-dead-package-paths', () => { expect(r.output).toMatch(/packages\/protect/) }) + // #772 review, finding 15. The name capture had no right anchor, so a + // sentence-final `packages/stack.` swallowed the period and the linter + // reported a LIVE package as dead — failing the build with a message naming a + // directory that plainly exists. Never fired in 400 commits only because the + // repo's backtick convention happened to dodge it. + it('does not flag a live package followed by sentence punctuation', () => { + const r = run(fx('sentence-final.md')) + expect(r.output).toBe('') + expect(r.exitCode).toBe(0) + }) + + // The character class excluded uppercase, so `packages/Foo` was never checked + // at all — a silent hole rather than a false alarm. + it('checks a package name containing uppercase', () => { + const r = run(fx('uppercase.md')) + expect(r.exitCode).toBe(1) + expect(r.output).toMatch(/packages\/Foo/) + }) + + // The linters carry package paths of their own; `scripts/` was not scanned, + // so a `packages/drizzle` allowlist entry for a package that never existed in + // git history sat there unnoticed. Its own fixtures must stay exempt. + it('scans scripts/ but not its fixtures', () => { + const r = run() + expect(r.exitCode).toBe(0) + expect(r.output).toBe('') + }) + it('names the file and line of each offender', () => { const r = run(fx('dead-ref.md')) expect(r.output).toMatch(/dead-ref\.md:3/) diff --git a/scripts/lint-no-dead-package-paths.mjs b/scripts/lint-no-dead-package-paths.mjs index 8ca468d27..47f7497a9 100644 --- a/scripts/lint-no-dead-package-paths.mjs +++ b/scripts/lint-no-dead-package-paths.mjs @@ -1,3 +1,4 @@ +import { execFileSync } from 'node:child_process' import { readdirSync, readFileSync, statSync } from 'node:fs' import { join, relative, resolve } from 'node:path' @@ -24,6 +25,10 @@ const TARGETS = process.argv.slice(2).length 'skills', 'e2e/README.md', 'packages/cli/AGENTS.md', + // The linters themselves carry package paths — an allowlist entry for a + // deleted package sat here unnoticed because `scripts/` was not scanned. + // `__tests__` is excluded below: its fixtures MUST name dead packages. + 'scripts', ] const SKIP_DIRS = new Set([ @@ -32,20 +37,50 @@ const SKIP_DIRS = new Set([ 'plans', 'superpowers', '.git', + // Fixtures for this linter's own self-tests deliberately reference deleted + // packages; scanning them would make the suite unrunnable. + '__tests__', ]) const SKIP_FILES = new Set(['CHANGELOG.md']) const TEXT_EXT = /\.(md|ya?ml|json|mjs|ts|txt)$/ // `packages/` where `` is a real directory name. The character // class excludes `*`, so workspace globs (`packages/*`, `./packages/*`) are -// left alone, and `+` is greedy so `packages/stack-forge` is never excused by -// the live `packages/stack`. -const PACKAGE_REF = /packages\/([a-z0-9][a-z0-9._-]*)/g +// left alone, and it is greedy so a longer directory name is never excused by +// a live package whose name is a prefix of it. +// +// The name must END on an alphanumeric. Without that anchor a sentence-final +// `packages/stack.` — or a hyphen at a line wrap, or a trailing underscore — +// captured the punctuation too and reported a LIVE package as dead, failing +// the build with a message naming a directory that plainly exists. Uppercase +// is admitted so a capitalised directory name is checked rather than silently +// skipped; no package uses one today, which is exactly why nothing noticed +// (#772 review, finding 15). +const PACKAGE_REF = /packages\/([a-zA-Z0-9](?:[a-zA-Z0-9._-]*[a-zA-Z0-9])?)/g +// Live packages come from what git TRACKS, not from what is on disk. +// +// `readdirSync` was wrong in the direction that matters: deleting a package +// leaves its `dist/` and `node_modules/` behind, so the directory still exists +// and every reference to the deleted package passed. That is the exact failure +// this linter was written to catch, and it silently stopped catching it on any +// checkout where the package had previously been built — two packages deleted +// by this very stack are sitting on `main` right now as exactly such shells +// (#772 review, finding 15). +// +// Note this deliberately does NOT require a `package.json`: `packages/utils` has +// none (it is two loose files consumed by relative path from `packages/nextjs`) +// yet is tracked, live, and referenced from AGENTS.md. const livePackages = new Set( - readdirSync(resolve(REPO_ROOT, 'packages'), { withFileTypes: true }) - .filter((e) => e.isDirectory()) - .map((e) => e.name), + execFileSync('git', ['ls-files', '-z', 'packages'], { + cwd: REPO_ROOT, + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + }) + .split('\0') + .filter(Boolean) + .map((file) => file.split('/')[1]) + .filter(Boolean), ) function* walk(abs) { diff --git a/scripts/lint-no-hardcoded-runners.mjs b/scripts/lint-no-hardcoded-runners.mjs index e8bbc8b49..29c453fe8 100644 --- a/scripts/lint-no-hardcoded-runners.mjs +++ b/scripts/lint-no-hardcoded-runners.mjs @@ -10,7 +10,6 @@ const ALLOWLISTED_PATHS = new Set([ 'packages/wizard/src/lib/detect.ts', // npm row of the PM table 'packages/cli/src/commands/init/utils.ts', // runnerCommand `case 'npm'` 'packages/cli/src/commands/init/lib/setup-prompt.ts', // execCommand `case 'npm':` switch - 'packages/drizzle/src/bin/runner.ts', // Pre-allowlisted: helper for Task 13 'scripts/lint-no-hardcoded-runners.mjs', // this script's own docs ]) diff --git a/skills/stash-encryption/SKILL.md b/skills/stash-encryption/SKILL.md index 010e63d4d..6865ebd2f 100644 --- a/skills/stash-encryption/SKILL.md +++ b/skills/stash-encryption/SKILL.md @@ -390,7 +390,7 @@ if (!enc.failure) { Typed-client model notes: -- `decryptModel` / `bulkDecryptModels` take the **table as a second argument** and return a plain `Promise>` (not a chainable operation) — pass a lock context as the optional third argument instead of chaining `.withLockContext()`. +- `decryptModel` / `bulkDecryptModels` take the **table as a second argument** and return a chainable `AuditableDecryptModelOperation` — await it for the `Result`, or chain `.audit({ metadata })` / `.withLockContext(lockContext)` first. A lock context may instead be passed as the optional third argument; use one form or the other, not both (chaining `.withLockContext()` onto a decrypt that already took a positional lock context throws). - `Date` columns are reconstructed to real `Date` instances on decrypt; `bigint` columns round-trip as native `bigint`. - Nullable schema fields stay nullable through the round trip. @@ -698,7 +698,7 @@ Every operation returns a `Result`. Narrow on `.failure` before touching `.data` `identityClaim` is an array of JWT claim *names*, not values: `["sub"]` (the default) or `["sub", "org_id"]`. ZeroKMS resolves each claim's value from the JWT the strategy federated. **The same claim must be supplied to encrypt and decrypt** — it is baked into the data key's tag, so decrypting without it fails with `Failed to retrieve key`. -Lock contexts work with every operation: `encrypt`, `decrypt`, `encryptModel`, `decryptModel`, `bulkEncrypt`, `bulkDecrypt`, `bulkEncryptModels`, `bulkDecryptModels`, `encryptQuery`. On the typed client, `decryptModel` and `bulkDecryptModels` take the lock context as their optional **third argument** (they return a plain `Promise>`, so there is no `.withLockContext()` to chain): +Lock contexts work with every operation: `encrypt`, `decrypt`, `encryptModel`, `decryptModel`, `bulkEncrypt`, `bulkDecrypt`, `bulkEncryptModels`, `bulkDecryptModels`, `encryptQuery`. On the typed client, `decryptModel` and `bulkDecryptModels` additionally accept the lock context as an optional **third argument**, which is the form shown here — `.withLockContext()` chains on them too, but use one or the other, not both: ```typescript const dec = await client.decryptModel(enc.data, users, IDENTITY) @@ -747,7 +747,7 @@ const result = await client .audit({ metadata: { action: "create" } }) // optional: audit trail ``` -(The typed client's `decryptModel` / `bulkDecryptModels` are the exception: they return a plain `Promise>` and take the lock context as an argument — see "Model Operations".) +(The typed client's `decryptModel` / `bulkDecryptModels` may also take the lock context as a positional argument instead of chaining — see "Model Operations".) ## Error Handling From 69b754c929bb9d5a633350cb76000ab874261f00 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 19:50:20 +1000 Subject: [PATCH 2/5] fix(cli,scripts): per-integration query guidance, and linters that catch their own rot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the #772 review findings, addressing what the review of that review turned up. `stash init`'s setup prompt named `createEncryptionOperators` unconditionally. That symbol is exported by `@cipherstash/stack-drizzle` alone, so a Supabase or Prisma Next project was sent after a package that is not in its dependency tree — the previous `protectOps.eq` was wrong for everyone, this was wrong for three integrations out of four. Step 5 now branches through `queryOperatorGuidance()`, following the `migrationCommands()` pattern already in the file: `ops.eq` for Drizzle, the `encryptedSupabase` wrapper's own filters for Supabase, the `eql*` column operators for Prisma Next, and `client.encryptQuery(...)` for plain Postgres — which is also pointed at `stash-encryption`, since it is the default integration and installs no integration skill, making "see the integration skill" a dangling pointer. The package-path linter reported an untracked-but-present package as "does not exist" — finding 15's false alarm pointed the other way, at a directory sitting right there on disk. The live set now unions `git ls-files --others --exclude-standard`. `--directory` is deliberately not passed: it collapses an all-ignored directory to one entry and would resurrect exactly the `dist/`-and- `node_modules/` shells the linter exists to catch (verified both ways). git failing now exits 2 with an actionable message instead of a raw ENOENT stack trace and exit 1, which was indistinguishable from a genuine lint failure, and an empty live set refuses to run rather than flagging every reference at once. The `scans scripts/ but not its fixtures` test asserted the repo was clean — byte-for-byte what the suite's first test already asserted, and passing whether or not `scripts` was in TARGETS. It now plants an offender in the scanned directory. Verified by mutation: it dies when `scripts` is dropped, as the untracked-package test dies without the union and the git-failure test dies when the exit code is flipped. The runners linter now requires each allowlist entry to exist and to still contain an unexcused `npx` literal. It immediately found a second stale entry the sibling linter structurally cannot see, since it only matches the `packages/` shape: `setup-prompt.ts` has had no `npx` since its switch moved to `utils.ts`. Removed. Also corrects two claims: the test comment saying `packages/drizzle` never existed in git history (it was added speculatively by c6715608, became load-bearing 31 minutes later with 9d259e6e, and rotted when 413ca396 deleted the package), and the `AnyEncryptedTable` doc comment still promising v3 is "purely additive and no existing caller has to change" — true of decrypt, false of encrypt since the v2 write overloads were removed. --- .changeset/decrypt-chaining-docs.md | 8 ++- .changeset/dynamodb-eql-v3.md | 10 +-- .../init/lib/__tests__/setup-prompt.test.ts | 39 +++++++++++ .../cli/src/commands/init/lib/setup-prompt.ts | 26 ++++++- packages/stack/src/dynamodb/types.ts | 11 ++- .../untracked-package.md | 3 + .../lint-no-dead-package-paths.test.mjs | 69 +++++++++++++++++-- .../lint-no-hardcoded-runners.test.mjs | 57 ++++++++++++++- scripts/lint-no-dead-package-paths.mjs | 64 ++++++++++++++--- scripts/lint-no-hardcoded-runners.mjs | 47 ++++++++++++- 10 files changed, 304 insertions(+), 30 deletions(-) create mode 100644 scripts/__tests__/fixtures/lint-no-dead-package-paths/untracked-package.md diff --git a/.changeset/decrypt-chaining-docs.md b/.changeset/decrypt-chaining-docs.md index f2a40d9f7..3e81a244d 100644 --- a/.changeset/decrypt-chaining-docs.md +++ b/.changeset/decrypt-chaining-docs.md @@ -23,5 +23,9 @@ with no lock-context argument. Also fixes the setup prompt `stash init` writes for coding agents, which referenced `protectOps.eq` — an API that does not exist anywhere in the repo. -The operators come from `createEncryptionOperators(client)`, conventionally -bound to `ops`. +The step now names the query API each integration can actually import: +`createEncryptionOperators(client)` (conventionally `ops`) for Drizzle, the +`encryptedSupabase` wrapper's own filters for Supabase, the `eql*` column +operators for Prisma Next, and `client.encryptQuery(...)` for a plain Postgres +project — which is also pointed at `stash-encryption`, since it is installed +with no integration skill. diff --git a/.changeset/dynamodb-eql-v3.md b/.changeset/dynamodb-eql-v3.md index 3763c2ee5..54b960c92 100644 --- a/.changeset/dynamodb-eql-v3.md +++ b/.changeset/dynamodb-eql-v3.md @@ -13,11 +13,11 @@ the typed client from `EncryptionV3` and the nominal client from EQL v2 tables continue to be **readable** — `decryptModel` / `bulkDecryptModels` still accept one, so existing items stay accessible. Writing -through a v2 table is a separate matter: the encrypt overloads for it are -removed in this same release (see the DynamoDB v2 write-removal entry), so a -caller that still encrypts through a v2 table does need to change. The table -decides which wire format is used, so a DynamoDB table populated under one -version must keep being read with that version. +through a v2 table is a separate matter: `encryptModel` / `bulkEncryptModels` +narrowed to EQL v3 tables in this same release, so a caller that still encrypts +through a v2 table does need to change. The table decides which wire format is +used, so a DynamoDB table populated under one version must keep being read with +that version. This fixes a latent bug that made v3 unusable: the write path detected an encrypted value by its `k: 'ct'` tag, but EQL v3 scalars carry no `k` diff --git a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts index b1b684910..4a76304cc 100644 --- a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts @@ -105,6 +105,45 @@ describe('renderSetupPrompt — orient + route (implement mode)', () => { expect(out).toContain('pnpm exec drizzle-kit migrate') }) + // The "wire the column through" step names the query API by hand. It used to + // name `protectOps.eq`, which exists nowhere; naming `createEncryptionOperators` + // unconditionally is the same mistake one step smaller — that symbol is + // exported only by `@cipherstash/stack-drizzle`, which a Supabase or Prisma + // project does not even have in its dependency tree. Each integration gets + // the API it can actually import. + describe('query-operator guidance is per-integration', () => { + const render = (integration: SetupPromptContext['integration']) => + renderSetupPrompt({ ...baseCtx, integration }) + + it('names the Drizzle operators for drizzle', () => { + const out = render('drizzle') + expect(out).toContain('createEncryptionOperators(client)') + expect(out).toContain('ops.eq') + }) + + it('names the wrapper builder for supabase, not the Drizzle operators', () => { + const out = render('supabase') + expect(out).not.toContain('createEncryptionOperators') + expect(out).toContain('encryptedSupabase') + }) + + it('names the eql* column operators for prisma-next', () => { + const out = render('prisma-next') + expect(out).not.toContain('createEncryptionOperators') + expect(out).toContain('eqlEq') + }) + + // `postgresql` is the default when nothing is detected, and it gets no + // integration skill at all (install-skills.ts) — so "see the integration + // skill" is a dangling pointer for it too. + it('names the core encryptQuery path for plain postgresql', () => { + const out = render('postgresql') + expect(out).not.toContain('createEncryptionOperators') + expect(out).toContain('encryptQuery') + expect(out).toContain('stash-encryption') + }) + }) + it('emits supabase migration commands for supabase integration', () => { const out = renderSetupPrompt({ ...baseCtx, diff --git a/packages/cli/src/commands/init/lib/setup-prompt.ts b/packages/cli/src/commands/init/lib/setup-prompt.ts index 12b149aae..5a22d42a6 100644 --- a/packages/cli/src/commands/init/lib/setup-prompt.ts +++ b/packages/cli/src/commands/init/lib/setup-prompt.ts @@ -68,6 +68,30 @@ function migrationCommands( return undefined } +/** + * How this integration filters on an encrypted column. Named per-integration + * rather than generically because the APIs are not interchangeable and only one + * of them is importable from any given project: `createEncryptionOperators` is + * exported by `@cipherstash/stack-drizzle` alone, so naming it for a Supabase + * or Prisma project sends the agent after a package that is not installed. + * + * `postgresql` is the fallback integration and is installed with no integration + * skill (see `SKILL_MAP` in `install-skills.ts`), so it is pointed at + * `stash-encryption` instead of "the integration skill". + */ +function queryOperatorGuidance(integration: Integration): string { + if (integration === 'supabase') { + return 'query paths filter through the `encryptedSupabase` wrapper (`es.from("users").select(...).eq("email", value)`) — it encrypts filter operands for encrypted columns automatically; see the integration skill' + } + if (integration === 'prisma-next') { + return 'query paths use the `eql*` operators on the column inside `.where()` (`u.email.eqlEq(value)`, `eqlMatch`, `eqlGt`, …) — see the integration skill' + } + if (integration === 'postgresql') { + return 'query paths encrypt the search term first with `client.encryptQuery(value, { table, column, queryType })` and compare against the encrypted column — see the `stash-encryption` skill (a plain Postgres project gets no integration skill)' + } + return 'query paths use the right operator (`ops.eq`, from `createEncryptionOperators(client)`) — see the integration skill' +} + function bullet(line: string): string { return `- ${line}` } @@ -280,7 +304,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { "2. Edit the user's real schema file (`src/db/schema.ts` or wherever they keep it) to declare the new encrypted column. Use the patterns in the integration skill — the `types.*` domain factories from `@cipherstash/stack-drizzle` for Drizzle, and the `types.*` factories from `@cipherstash/stack/eql/v3` (via `encryptedTable`, passed as `schemas`) for Supabase. Encrypted columns must be **nullable `jsonb`** at creation time. Never `.notNull()`.", `3. Generate the schema migration${migration ? ` — \`${migration.generate}\` (${migration.tool})` : " using the project's existing migration tooling"}.`, `4. Show the user the generated SQL before applying${migration ? ` — \`${migration.apply}\`` : ''}.`, - '5. Wire the column through the application code: insert paths encrypt before write, select paths decrypt after read, query paths use the right operator (`ops.eq`, from `createEncryptionOperators(client)` — see the integration skill).', + `5. Wire the column through the application code: insert paths encrypt before write, select paths decrypt after read, ${queryOperatorGuidance(ctx.integration)}.`, '6. Verify with a round-trip: insert a record, select it back, confirm the value decrypts and the search ops work.', '', '### Migrate an existing column to encrypted', diff --git a/packages/stack/src/dynamodb/types.ts b/packages/stack/src/dynamodb/types.ts index 83aac0414..c84af7b2c 100644 --- a/packages/stack/src/dynamodb/types.ts +++ b/packages/stack/src/dynamodb/types.ts @@ -21,9 +21,14 @@ import type { EncryptModelOperation } from './operations/encrypt-model' * `encryptedColumn`/`encryptedField` from `@cipherstash/stack/schema`) or an * EQL v3 one (`encryptedTable` + `types.*` from `@cipherstash/stack/eql/v3`). * - * Both are supported deliberately. DynamoDB shares none of the v2 Postgres - * machinery — there is no EQL extension to install and no migration to run — - * so accepting v3 is purely additive and no existing caller has to change. + * This union is the adapter's widest input type — the erased view the internal + * `CallableEncryptionClient` is declared against. It is NOT the public contract: + * the surface split the two versions apart. `encryptModel` / + * `bulkEncryptModels` narrowed to `AnyV3Table` (the v2 write overloads were + * removed, so a v2 encrypt call site does have to change); `decryptModel` / + * `bulkDecryptModels` still take either, so items stored under v2 stay + * readable. See {@link EncryptedDynamoDBInstance} for the overloads that decide + * this per method. */ export type AnyEncryptedTable = | EncryptedTable diff --git a/scripts/__tests__/fixtures/lint-no-dead-package-paths/untracked-package.md b/scripts/__tests__/fixtures/lint-no-dead-package-paths/untracked-package.md new file mode 100644 index 000000000..4f808752e --- /dev/null +++ b/scripts/__tests__/fixtures/lint-no-dead-package-paths/untracked-package.md @@ -0,0 +1,3 @@ +# A package that exists on disk but is not yet tracked + +The new thing lives in packages/lint-untracked-probe, scaffolded but not staged. diff --git a/scripts/__tests__/lint-no-dead-package-paths.test.mjs b/scripts/__tests__/lint-no-dead-package-paths.test.mjs index c0dacca7b..89e855dd6 100644 --- a/scripts/__tests__/lint-no-dead-package-paths.test.mjs +++ b/scripts/__tests__/lint-no-dead-package-paths.test.mjs @@ -1,4 +1,5 @@ import { execFileSync } from 'node:child_process' +import { mkdirSync, rmSync, writeFileSync } from 'node:fs' import { resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' @@ -8,9 +9,18 @@ const SCRIPT = resolve( '../../lint-no-dead-package-paths.mjs', ) +const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') + function run(...targets) { + return runWith({}, ...targets) +} + +function runWith(opts, ...targets) { try { - execFileSync('node', [SCRIPT, ...targets], { encoding: 'utf8' }) + execFileSync(process.execPath, [SCRIPT, ...targets], { + encoding: 'utf8', + ...opts, + }) return { exitCode: 0, output: '' } } catch (err) { return { @@ -63,12 +73,59 @@ describe('lint-no-dead-package-paths', () => { }) // The linters carry package paths of their own; `scripts/` was not scanned, - // so a `packages/drizzle` allowlist entry for a package that never existed in - // git history sat there unnoticed. Its own fixtures must stay exempt. + // so lint-no-hardcoded-runners' `packages/drizzle/src/bin/runner.ts` allowlist + // entry — added speculatively by c6715608, load-bearing 31 minutes later once + // 9d259e6e created the file — sat dead for the two months after 413ca396 + // deleted the package. Its sibling entry for `packages/protect` was removed + // with its package; this one was simply missed. + // + // Asserting the repo is clean would NOT pin this: the suite's first test + // already does that, and both pass whether or not `scripts` is in TARGETS. + // Plant an offender in the scanned directory instead. it('scans scripts/ but not its fixtures', () => { - const r = run() - expect(r.exitCode).toBe(0) - expect(r.output).toBe('') + const probe = resolve(REPO_ROOT, 'scripts/dead-path-probe.md') + try { + writeFileSync(probe, 'A reference to `packages/protect`, long gone.\n') + const r = run() + expect(r.exitCode).toBe(1) + expect(r.output).toMatch(/scripts\/dead-path-probe\.md:1/) + // `__tests__` stays skipped — the fixtures name dead packages on purpose, + // and so do the comments in this very file. + expect(r.output).not.toMatch(/__tests__/) + } finally { + rmSync(probe, { force: true }) + } + }) + + // A package scaffolded a minute ago is live, but nothing about it is tracked + // yet. Deriving the live set from `git ls-files` alone reported it as "does + // not exist" — the same species of false alarm as the sentence-final one + // above, pointed the other way, at a directory sitting right there on disk. + it('treats a package that exists but is not yet tracked as live', () => { + const pkg = resolve(REPO_ROOT, 'packages/lint-untracked-probe') + try { + mkdirSync(resolve(pkg, 'src'), { recursive: true }) + writeFileSync(resolve(pkg, 'src/index.ts'), 'export const x = 1\n') + const r = run(fx('untracked-package.md')) + expect(r.output).toBe('') + expect(r.exitCode).toBe(0) + } finally { + rmSync(pkg, { recursive: true, force: true }) + } + }) + + // The live set is derived by shelling out to git, so git failing is a mode + // this linter has to own. Exiting 1 with a raw ENOENT stack trace would be + // indistinguishable from a genuine lint failure; exit 2 says "the linter + // could not run", not "your docs are wrong". + it('exits 2 with an actionable message when git is unavailable', () => { + const r = runWith({ + env: { PATH: resolve(REPO_ROOT, 'scripts/nonexistent') }, + }) + expect(r.exitCode).toBe(2) + expect(r.output).toMatch(/git/) + expect(r.output).toMatch(/checkout/) + expect(r.output).not.toMatch(/at ModuleJob/) }) it('names the file and line of each offender', () => { diff --git a/scripts/__tests__/lint-no-hardcoded-runners.test.mjs b/scripts/__tests__/lint-no-hardcoded-runners.test.mjs index 7bb1202dc..96531ea0e 100644 --- a/scripts/__tests__/lint-no-hardcoded-runners.test.mjs +++ b/scripts/__tests__/lint-no-hardcoded-runners.test.mjs @@ -1,4 +1,5 @@ import { execFileSync } from 'node:child_process' +import { readFileSync, rmSync, writeFileSync } from 'node:fs' import { resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' @@ -8,9 +9,9 @@ const SCRIPT = resolve( '../../lint-no-hardcoded-runners.mjs', ) -function run(target) { +function runScript(script, ...targets) { try { - execFileSync('node', [SCRIPT, target], { encoding: 'utf8' }) + execFileSync(process.execPath, [script, ...targets], { encoding: 'utf8' }) return { exitCode: 0, output: '' } } catch (err) { return { @@ -20,6 +21,10 @@ function run(target) { } } +function run(target) { + return runScript(SCRIPT, target) +} + describe('lint-no-hardcoded-runners', () => { const fx = (name) => resolve(fileURLToPath(import.meta.url), `../fixtures/${name}`) @@ -70,3 +75,51 @@ describe('lint-no-hardcoded-runners', () => { expect(run(fx('identifier.ts')).exitCode).toBe(0) }) }) + +// An allowlist entry is a standing exemption. When the file it names is deleted +// — or stops carrying the `npx` literal it was excused for — the entry becomes +// silent dead weight, and the next reader takes it as evidence that the file +// still needs an exemption. `packages/drizzle/src/bin/runner.ts` sat here for +// the two months after 413ca396 deleted its package, and surfaced only because +// a sibling linter happened to start scanning `scripts/` (#772 review, finding +// 15). That sibling can only ever catch the `packages/` shape; this check +// covers every entry, including a stale path inside a live package. +describe('lint-no-hardcoded-runners — allowlist hygiene', () => { + // A copy alongside the original so `REPO_ROOT` still resolves to the repo. + const PROBE = resolve( + fileURLToPath(import.meta.url), + '../../allowlist-probe.mjs', + ) + + function runWithExtraEntry(entry) { + const src = readFileSync(SCRIPT, 'utf8').replace( + 'const ALLOWLISTED_PATHS = new Set([', + `const ALLOWLISTED_PATHS = new Set([\n '${entry}',`, + ) + try { + writeFileSync(PROBE, src) + return runScript(PROBE) + } finally { + rmSync(PROBE, { force: true }) + } + } + + it('rejects an entry whose file no longer exists', () => { + const r = runWithExtraEntry('scripts/deleted-helper.mjs') + expect(r.exitCode).toBe(2) + expect(r.output).toMatch(/scripts\/deleted-helper\.mjs/) + expect(r.output).toMatch(/no such file/) + }) + + it('rejects an entry whose file no longer needs the exemption', () => { + const r = runWithExtraEntry('scripts/vitest.config.mjs') + expect(r.exitCode).toBe(2) + expect(r.output).toMatch(/scripts\/vitest\.config\.mjs/) + expect(r.output).toMatch(/no longer contains/) + }) + + it('accepts the allowlist as it stands', () => { + const r = runScript(SCRIPT) + expect(r.exitCode).toBe(0) + }) +}) diff --git a/scripts/lint-no-dead-package-paths.mjs b/scripts/lint-no-dead-package-paths.mjs index 47f7497a9..9fe1a4984 100644 --- a/scripts/lint-no-dead-package-paths.mjs +++ b/scripts/lint-no-dead-package-paths.mjs @@ -37,8 +37,9 @@ const SKIP_DIRS = new Set([ 'plans', 'superpowers', '.git', - // Fixtures for this linter's own self-tests deliberately reference deleted - // packages; scanning them would make the suite unrunnable. + // This linter's own self-tests deliberately reference deleted packages — + // in the fixtures, and in the prose comments of the test files themselves. + // Scanning them would make the suite unrunnable. '__tests__', ]) const SKIP_FILES = new Set(['CHANGELOG.md']) @@ -58,7 +59,7 @@ const TEXT_EXT = /\.(md|ya?ml|json|mjs|ts|txt)$/ // (#772 review, finding 15). const PACKAGE_REF = /packages\/([a-zA-Z0-9](?:[a-zA-Z0-9._-]*[a-zA-Z0-9])?)/g -// Live packages come from what git TRACKS, not from what is on disk. +// Live packages come from git, not from what is on disk. // // `readdirSync` was wrong in the direction that matters: deleting a package // leaves its `dist/` and `node_modules/` behind, so the directory still exists @@ -71,18 +72,61 @@ const PACKAGE_REF = /packages\/([a-zA-Z0-9](?:[a-zA-Z0-9._-]*[a-zA-Z0-9])?)/g // Note this deliberately does NOT require a `package.json`: `packages/utils` has // none (it is two loose files consumed by relative path from `packages/nextjs`) // yet is tracked, live, and referenced from AGENTS.md. +// +// Shelling out to git is a dependency this linter has to own: git missing, or a +// tree with no `.git`, must not read as "every package is dead". +function gitPackagePaths(...args) { + try { + return execFileSync('git', args, { + cwd: REPO_ROOT, + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + }) + .split('\0') + .filter(Boolean) + } catch (err) { + const detail = String(err.stderr || err.message || '').trim() + console.error( + `Could not list packages via \`git ${args.join(' ')}\`:\n\n ${detail}\n\n` + + 'This linter derives the live package set from git, so it cannot run\n' + + 'without git on PATH or outside a git checkout.', + ) + // Exit 2, not 1: the linter failed to run. Exit 1 means it ran and found + // dead references, which is a different thing to go and fix. + process.exit(2) + } +} + const livePackages = new Set( - execFileSync('git', ['ls-files', '-z', 'packages'], { - cwd: REPO_ROOT, - encoding: 'utf8', - maxBuffer: 64 * 1024 * 1024, - }) - .split('\0') - .filter(Boolean) + [ + ...gitPackagePaths('ls-files', '-z', 'packages'), + // Untracked but not ignored. A package scaffolded a minute ago is live + // even though nothing about it is staged yet, and reporting it as "does + // not exist" is the sentence-final false alarm all over again, pointed the + // other way. `--directory` is deliberately NOT passed: it collapses an + // all-ignored directory to a single entry, which would resurrect exactly + // the `dist/`-and-`node_modules/` shells this linter exists to catch. + ...gitPackagePaths( + 'ls-files', + '--others', + '--exclude-standard', + '-z', + 'packages', + ), + ] .map((file) => file.split('/')[1]) .filter(Boolean), ) +if (livePackages.size === 0) { + console.error( + 'git reported no packages at all under `packages/`. Refusing to run —\n' + + 'every reference would be flagged. Check that `packages/` is present and\n' + + 'not wholly ignored.', + ) + process.exit(2) +} + function* walk(abs) { const stat = statSync(abs) if (stat.isFile()) { diff --git a/scripts/lint-no-hardcoded-runners.mjs b/scripts/lint-no-hardcoded-runners.mjs index 29c453fe8..95b216570 100644 --- a/scripts/lint-no-hardcoded-runners.mjs +++ b/scripts/lint-no-hardcoded-runners.mjs @@ -9,7 +9,6 @@ const REPO_ROOT = resolve(import.meta.dirname, '..') const ALLOWLISTED_PATHS = new Set([ 'packages/wizard/src/lib/detect.ts', // npm row of the PM table 'packages/cli/src/commands/init/utils.ts', // runnerCommand `case 'npm'` - 'packages/cli/src/commands/init/lib/setup-prompt.ts', // execCommand `case 'npm':` switch 'scripts/lint-no-hardcoded-runners.mjs', // this script's own docs ]) @@ -68,6 +67,52 @@ function isAllowedRunnerSwitch(line) { return /\bcase\s+['"]npm['"]/.test(line) || /name:\s*['"]npm['"]/.test(line) } +// An allowlist entry is a standing exemption, so it has to keep earning its +// place. Two ways it rots: the file is deleted (which is how the legacy Drizzle +// package's `src/bin/runner.ts` entry outlived its package by two months), or +// the file survives but the `npx` literal it was excused for moves elsewhere — +// leaving an entry that reads as evidence the exemption is still needed. Check +// both before scanning anything. +const staleAllowlist = [] +for (const rel of ALLOWLISTED_PATHS) { + let source + try { + source = readFileSync(resolve(REPO_ROOT, rel), 'utf8') + } catch { + staleAllowlist.push(`${rel}: no such file`) + continue + } + const stillNeeded = source + .split('\n') + .some( + (line) => + NPX_TOKEN.test(line) && + !isCommentLine(line) && + !isAllowedFallback(line) && + !isAllowedRunnerSwitch(line), + ) + if (!stillNeeded) { + staleAllowlist.push( + `${rel}: no longer contains an unexcused \`npx\` literal`, + ) + } +} + +if (staleAllowlist.length > 0) { + console.error( + `Found ${staleAllowlist.length} stale allowlist entr(ies) in this script:\n`, + ) + for (const s of staleAllowlist) console.error(` ${s}`) + console.error( + '\nDrop the entry. An exemption for a file that no longer exists, or that\n' + + 'no longer contains the literal it was excused for, is dead weight that\n' + + 'reads as deliberate.', + ) + // Exit 2, not 1: the linter's own configuration is wrong, which is a + // different thing to fix than an `npx` literal in the codebase. + process.exit(2) +} + const offenders = [] for (const target of TARGETS) { const abs = resolve(REPO_ROOT, target) From 57f323ef2f33226ef35d999c005110423571db1f Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Mon, 27 Jul 2026 15:24:39 +1000 Subject: [PATCH 3/5] fix(cli,scripts): close the review nits, and the same defect where it repeats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remediation for the two review rounds on #789, plus what verifying them turned up in the same code. **The linter fix the review asked for twice.** `livePackages` moving from `readdirSync` to `git ls-files` was the highest-value change in the PR and had no test that pinned it. It still doesn't fail under the revert anyone would actually write: keeping the git call and unioning the filesystem back in passes every existing test while fully restoring the false negative. The new fixture builds the discriminating case — a package deleted from git whose gitignored `dist/` and `node_modules/` shells survive on disk — and is the only test that fails under both a naive `readdirSync` revert and that hybrid. The `git status` guard is scoped to the probe, not to `packages/`, so an unrelated uncommitted file doesn't turn it into an assertion about nothing. **A missing target no longer passes in silence.** A linter whose entire job is catching dead paths in configuration skipped dead paths in its own: rename a target and it dropped out of coverage forever, green. Its sibling already exits 2 for a stale allowlist entry. Both now do, and both report a target outside the repo by name instead of as a `../../../../..` chain climbing out of the root — the review's cosmetic nit, which turned out to sit in both files. **Every step that names an integration-specific API now branches.** Step 5 was fixed for this; steps 1, 2 and the read-path step were not. A Prisma Next project was sent at `types.*` / `encryptedTable` — the client `stash schema build` explicitly refuses to scaffold for it — and a plain-Postgres project was pointed three times at "the integration skill" it never gets installed. `encryptQuery` is shown taking the schema objects rather than an object-shorthand that read as three required strings. `queryOperatorGuidance` is a switch with a neutral default rather than an if-chain ending in Drizzle's answer: tsup transpiles without type-checking and this package has no typecheck script, so nothing would have caught a fifth integration inheriting it. One test asserted nothing: it scoped to a `#### Encryption cutover` heading that does not exist, and `substring(-1)` returns the whole document. --- .changeset/decrypt-chaining-docs.md | 30 ++++-- .../init/lib/__tests__/setup-prompt.test.ts | 79 +++++++++++++++- .../cli/src/commands/init/lib/setup-prompt.ts | 92 ++++++++++++++++--- .../lint-no-dead-package-paths/dead-shell.md | 4 + .../lint-no-dead-package-paths.test.mjs | 83 ++++++++++++++++- .../lint-no-hardcoded-runners.test.mjs | 31 ++++++- scripts/lint-no-dead-package-paths.mjs | 28 +++++- scripts/lint-no-hardcoded-runners.mjs | 22 ++++- 8 files changed, 338 insertions(+), 31 deletions(-) create mode 100644 scripts/__tests__/fixtures/lint-no-dead-package-paths/dead-shell.md diff --git a/.changeset/decrypt-chaining-docs.md b/.changeset/decrypt-chaining-docs.md index 3e81a244d..5345a1e9c 100644 --- a/.changeset/decrypt-chaining-docs.md +++ b/.changeset/decrypt-chaining-docs.md @@ -23,9 +23,27 @@ with no lock-context argument. Also fixes the setup prompt `stash init` writes for coding agents, which referenced `protectOps.eq` — an API that does not exist anywhere in the repo. -The step now names the query API each integration can actually import: -`createEncryptionOperators(client)` (conventionally `ops`) for Drizzle, the -`encryptedSupabase` wrapper's own filters for Supabase, the `eql*` column -operators for Prisma Next, and `client.encryptQuery(...)` for a plain Postgres -project — which is also pointed at `stash-encryption`, since it is installed -with no integration skill. +Every step naming an integration-specific API now branches on the project's +actual integration, instead of naming Drizzle's and Supabase's and leaving the +other two to guess: + +- **Query paths.** `createEncryptionOperators(client)` (conventionally `ops`) + for Drizzle, the `encryptedSupabase` wrapper's own filters for Supabase, the + `eql*` column operators for Prisma Next, and `client.encryptQuery(...)` for a + plain Postgres project. +- **Schema authoring.** The `types.*` column factories for Drizzle, the + `eql_v3_encrypted` domain in migration SQL for Supabase, the `cipherstash.*` + field constructors in `schema.prisma` for Prisma Next, and `encryptedTable` + for plain Postgres. Prisma Next was previously sent at `types.*` / + `encryptedTable` — the client `stash schema build` explicitly refuses to + scaffold for that integration. +- **Read paths.** `decryptModel(row, usersSchema)` where that applies, and the + wrapper's transparent decryption where it does not. +- **Skill pointers.** A plain Postgres project installs no integration-specific + skill, so each "see the integration skill" was a pointer at a file that was + never written. Those now point at `stash-encryption`, which it does get. + +`client.encryptQuery` is also shown taking the schema objects themselves +(`{ table: usersSchema, column: usersSchema.email }`) rather than an +object-shorthand that read as three required strings — `queryType` is inferred +from the column's configured indexes. diff --git a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts index 4a76304cc..8ea972d9f 100644 --- a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts @@ -142,6 +142,78 @@ describe('renderSetupPrompt — orient + route (implement mode)', () => { expect(out).toContain('encryptQuery') expect(out).toContain('stash-encryption') }) + + // `EncryptQueryOptions` takes the schema OBJECTS — `usersSchema` and + // `usersSchema.email` — not their names as strings, and `queryType` is + // inferred from the column's indexes when omitted. The object-shorthand + // `{ table, column, queryType }` reads as three required string fields: + // the same species of plausible-but-wrong API as the `protectOps.eq` this + // PR exists to have removed. + it('shows encryptQuery taking schema objects, not string names', () => { + const out = render('postgresql') + expect(out).toContain('column: usersSchema.email') + expect(out).not.toContain('{ table, column, queryType }') + }) + + // `packages/cli` builds with tsup, which transpiles without type-checking, + // and has no `typecheck` script — so `Record` buys nothing + // at build time here. An if-chain ending in a bare Drizzle `return` hands a + // future fifth integration the exact string this helper exists to stop it + // getting. Degrade to neutral guidance, as `skillsFor()` degrades to + // BASE_SKILLS. + it('degrades to neutral guidance for an unrecognised integration', () => { + const out = renderSetupPrompt({ + ...baseCtx, + integration: 'mystery-orm' as SetupPromptContext['integration'], + }) + expect(out).not.toContain('createEncryptionOperators') + expect(out).not.toContain('encryptedSupabase') + expect(out).toContain('encryptQuery') + }) + }) + + // Step 2 named the Drizzle and Supabase schema APIs unconditionally — the + // identical defect step 5 above was just fixed for. A prisma-next project was + // sent at `types.*` / `encryptedTable`, which `stash schema build` explicitly + // refuses to scaffold for that integration; a plain-Postgres project was + // given no named path at all. + describe('schema-authoring guidance is per-integration', () => { + const render = (integration: SetupPromptContext['integration']) => + renderSetupPrompt({ ...baseCtx, integration }) + + it('names the Drizzle column factories for drizzle', () => { + const out = render('drizzle') + expect(out).toContain('`types.*` column factories') + expect(out).toContain('@cipherstash/stack-drizzle') + }) + + it('names the eql_v3 domain for supabase', () => { + expect(render('supabase')).toContain('eql_v3_encrypted') + }) + + it('names the cipherstash.* field constructors for prisma-next', () => { + const out = render('prisma-next') + expect(out).toContain('cipherstash.TextSearch()') + expect(out).toContain('prisma/schema.prisma') + // The `types.*` client is precisely what `stash schema build` refuses to + // emit for prisma-next. + expect(out).not.toContain('`types.*` column factories') + }) + + it('names encryptedTable for plain postgresql', () => { + const out = render('postgresql') + expect(out).toContain('encryptedTable') + expect(out).toContain('@cipherstash/stack/v3') + }) + }) + + // `postgresql` installs no integration-specific skill (SKILL_MAP), so every + // unconditional "see the integration skill" is a pointer at a file that was + // never written. Step 5's was fixed; two more survived elsewhere. + it('never points plain postgresql at "the integration skill"', () => { + const out = renderSetupPrompt({ ...baseCtx, integration: 'postgresql' }) + expect(out).not.toMatch(/the integration skill/i) + expect(out).toContain('stash-encryption') }) it('emits supabase migration commands for supabase integration', () => { @@ -477,7 +549,12 @@ describe('renderSetupPrompt — no db push recommendations', () => { expect(out).toMatch(/1\.\s*\*\*Schema-add/) expect(out).toMatch(/2\.\s*\*\*Dual-write/) // Cutover is still covered, just without a db push workaround note. - const cutoverSection = out.substring(out.indexOf('#### Encryption cutover')) + // The heading has to be one that exists: `indexOf` returning -1 makes + // `substring(-1)` the whole document, so this scoped assertion was + // silently asserting nothing at all. + const heading = '#### Backfill and switch' + expect(out).toContain(heading) + const cutoverSection = out.substring(out.indexOf(heading)) expect(cutoverSection).toMatch(/encrypt cutover/) }) diff --git a/packages/cli/src/commands/init/lib/setup-prompt.ts b/packages/cli/src/commands/init/lib/setup-prompt.ts index 5a22d42a6..03d9a7e3b 100644 --- a/packages/cli/src/commands/init/lib/setup-prompt.ts +++ b/packages/cli/src/commands/init/lib/setup-prompt.ts @@ -68,6 +68,46 @@ function migrationCommands( return undefined } +/** + * Where this integration's rules were actually written. + * + * `postgresql` is the fallback integration and installs no integration-specific + * skill (see `SKILL_MAP` in `install-skills.ts`), so "the integration skill" is + * a pointer at a file that was never created. Point it at `stash-encryption`, + * which it does get. + */ +function integrationSkillRef(integration: Integration): string { + switch (integration) { + case 'drizzle': + case 'supabase': + case 'prisma-next': + return 'the integration skill' + default: + return 'the `stash-encryption` skill' + } +} + +/** + * How this integration declares an encrypted column in the schema. + * + * Per-integration for the same reason the query operators are: the APIs are not + * interchangeable, and `stash schema build` (`utils.ts`) refuses outright to + * scaffold a `types.*` client for prisma-next — that integration authors its + * columns with `cipherstash.*` constructors in `schema.prisma` instead. + */ +function schemaAuthoringGuidance(integration: Integration): string { + switch (integration) { + case 'drizzle': + return 'Declare it with the `types.*` column factories from `@cipherstash/stack-drizzle` on the existing `pgTable`' + case 'supabase': + return 'Declare the column with the `eql_v3_encrypted` domain in the migration SQL (`encryptedSupabase` derives its encryption config by introspecting those domains)' + case 'prisma-next': + return 'Declare the field with the `cipherstash.*` constructors in `prisma/schema.prisma` (`cipherstash.TextSearch()`, `cipherstash.DoubleOrd()`, …), with the `cipherstash` extension pack wired up per `@cipherstash/prisma-next/control`' + default: + return 'Declare the table with `encryptedTable` and the `types.*` domain factories from `@cipherstash/stack/v3`, then pass it to `Encryption({ schemas })`' + } +} + /** * How this integration filters on an encrypted column. Named per-integration * rather than generically because the APIs are not interchangeable and only one @@ -75,21 +115,45 @@ function migrationCommands( * exported by `@cipherstash/stack-drizzle` alone, so naming it for a Supabase * or Prisma project sends the agent after a package that is not installed. * - * `postgresql` is the fallback integration and is installed with no integration - * skill (see `SKILL_MAP` in `install-skills.ts`), so it is pointed at - * `stash-encryption` instead of "the integration skill". + * A `switch` with a neutral `default`, not an if-chain ending in the Drizzle + * string: `packages/cli` is built by tsup, which transpiles without + * type-checking, and the package has no `typecheck` script — so nothing would + * catch a fifth `Integration` variant silently inheriting Drizzle's answer. + * `skillsFor()` in `install-skills.ts` degrades the same way, for the same + * reason. */ function queryOperatorGuidance(integration: Integration): string { - if (integration === 'supabase') { - return 'query paths filter through the `encryptedSupabase` wrapper (`es.from("users").select(...).eq("email", value)`) — it encrypts filter operands for encrypted columns automatically; see the integration skill' + switch (integration) { + case 'supabase': + return 'query paths filter through the `encryptedSupabase` wrapper (`es.from("users").select(...).eq("email", value)`) — it encrypts filter operands for encrypted columns automatically; see the integration skill' + case 'prisma-next': + return 'query paths use the `eql*` operators on the column inside `.where()` (`u.email.eqlEq(value)`, `eqlMatch`, `eqlGt`, …) — see the integration skill' + case 'drizzle': + return 'query paths use the right operator (`ops.eq`, from `createEncryptionOperators(client)`) — see the integration skill' + default: + // `table` and `column` are the schema OBJECTS, not their names as + // strings, and `queryType` is inferred from the column's configured + // indexes unless it is passed to override the inference. + return 'query paths encrypt the search term first — `client.encryptQuery(value, { table: usersSchema, column: usersSchema.email })`, passing the schema objects themselves rather than their names — and compare that against the encrypted column; see the `stash-encryption` skill (a plain Postgres project gets no integration-specific skill)' } - if (integration === 'prisma-next') { - return 'query paths use the `eql*` operators on the column inside `.where()` (`u.email.eqlEq(value)`, `eqlMatch`, `eqlGt`, …) — see the integration skill' - } - if (integration === 'postgresql') { - return 'query paths encrypt the search term first with `client.encryptQuery(value, { table, column, queryType })` and compare against the encrypted column — see the `stash-encryption` skill (a plain Postgres project gets no integration skill)' +} + +/** + * How this integration turns ciphertext back into values on the read path. + * + * Named per-integration for the third time in this file, and for the third + * time because the answer is not portable: `decryptModel` is the typed + * client's, transparent decryption is the Supabase wrapper's. + */ +function readPathGuidance(integration: Integration): string { + switch (integration) { + case 'supabase': + return 'selects through the `encryptedSupabase` wrapper decrypt transparently' + case 'prisma-next': + return 'the encrypted fields decrypt through the Prisma Next client' + default: + return 'call `decryptModel(row, usersSchema)` — or `bulkDecryptModels` for a set — before returning the value to callers' } - return 'query paths use the right operator (`ops.eq`, from `createEncryptionOperators(client)`) — see the integration skill' } function bullet(line: string): string { @@ -298,10 +362,10 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { '', '### Add a new encrypted column', '', - 'Use when the column **does not yet exist** in the database (no plaintext predecessor to preserve). This is normal Drizzle / Supabase work plus the encryption client patterns from the integration skill.', + `Use when the column **does not yet exist** in the database (no plaintext predecessor to preserve). This is normal schema work in the project's own ORM or migration tooling, plus the encryption client patterns from ${integrationSkillRef(ctx.integration)}.`, '', "1. **If this is the first encrypted column in the project, configure the bundler exclusion first.** `@cipherstash/stack` cannot be bundled (it wraps a native FFI module). Next.js: add `serverExternalPackages: ['@cipherstash/stack', '@cipherstash/protect-ffi']` to `next.config.*`. Webpack: `externals`. esbuild: `external`. Vite SSR: `ssr.external`. Without this, the encryption client crashes at runtime with `Cannot find module '@cipherstash/protect-ffi-*'`. See the `stash-encryption` skill's Installation section for the full snippets.", - "2. Edit the user's real schema file (`src/db/schema.ts` or wherever they keep it) to declare the new encrypted column. Use the patterns in the integration skill — the `types.*` domain factories from `@cipherstash/stack-drizzle` for Drizzle, and the `types.*` factories from `@cipherstash/stack/eql/v3` (via `encryptedTable`, passed as `schemas`) for Supabase. Encrypted columns must be **nullable `jsonb`** at creation time. Never `.notNull()`.", + `2. Edit the user's real schema file (\`src/db/schema.ts\` or wherever they keep it) to declare the new encrypted column. ${schemaAuthoringGuidance(ctx.integration)} — the patterns are in ${integrationSkillRef(ctx.integration)}. Encrypted columns must be **nullable \`jsonb\`** at creation time (the \`eql_v3_*\` domains are over \`jsonb\`). Never \`.notNull()\`.`, `3. Generate the schema migration${migration ? ` — \`${migration.generate}\` (${migration.tool})` : " using the project's existing migration tooling"}.`, `4. Show the user the generated SQL before applying${migration ? ` — \`${migration.apply}\`` : ''}.`, `5. Wire the column through the application code: insert paths encrypt before write, select paths decrypt after read, ${queryOperatorGuidance(ctx.integration)}.`, @@ -328,7 +392,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { '', `3. **Backfill.** Run \`${cli} encrypt backfill --table --column \`. The CLI prompts the user (or accepts \`--confirm-dual-writes-deployed\` non-interactively) to confirm dual-writes are live, then chunks through the existing rows. Resumable; checkpoints to \`cs_migrations\` after every chunk. SIGINT-safe.`, `4. **Switch reads to the encrypted column.** The step depends on the EQL version (\`${cli} encrypt backfill\` prints it; \`${cli} encrypt status\` shows it). **EQL v3 (the default):** there is no rename — update the schema and queries to read/write the encrypted column by its own name, and wire decryption through the encryption client. **EQL v2 (legacy data only):** update the schema file to declare the encrypted column under its final name (drop the twin suffix), then \`${cli} encrypt cutover --table --column \` runs the rename in one transaction (\`\` → \`_plaintext\`, twin → \`\`). Do **not** declare a v2 column with a \`types.*\` domain — those are EQL v3 only. The adapters no longer author v2 (\`@cipherstash/stack-drizzle\` removed \`encryptedType\`), so a v2 column is a read path: declare it with the deprecated \`@cipherstash/stack/schema\` builders and decrypt through \`@cipherstash/stack\`.`, - '5. **Wire the read path through the encryption client.** The read column now holds ciphertext. Read code paths must decrypt before returning the value to callers — `decryptModel(row, table)` for Drizzle, the `encryptedSupabase` wrapper for Supabase, or the equivalent `decrypt`/`bulkDecryptModels` calls. Without this step, your read paths return raw encrypted payloads to end users. The integration skill has the exact API.', + `5. **Wire the read path through the encryption client.** The read column now holds ciphertext — ${readPathGuidance(ctx.integration)}. Without this step, your read paths return raw encrypted payloads to end users. See ${integrationSkillRef(ctx.integration)} for the exact API.`, '6. **Remove the dual-write code.** The plaintext column (still `` on v3; renamed `_plaintext` on v2) is no longer authoritative. Delete the dual-write logic from the persistence layer.', `7. **Drop.** Run \`${cli} encrypt drop --table --column \`. Generates a migration that removes the now-unused plaintext column (on v3 it first verifies no rows are still plaintext-only). Apply with the project's normal migration tooling.`, '', diff --git a/scripts/__tests__/fixtures/lint-no-dead-package-paths/dead-shell.md b/scripts/__tests__/fixtures/lint-no-dead-package-paths/dead-shell.md new file mode 100644 index 000000000..9b25c6550 --- /dev/null +++ b/scripts/__tests__/fixtures/lint-no-dead-package-paths/dead-shell.md @@ -0,0 +1,4 @@ +# A deleted package whose build output survives on disk + +The old thing lived in packages/lint-dead-shell-probe, deleted from git but +still sitting there as a `dist/` shell on any checkout that once built it. diff --git a/scripts/__tests__/lint-no-dead-package-paths.test.mjs b/scripts/__tests__/lint-no-dead-package-paths.test.mjs index 89e855dd6..92f9d7aff 100644 --- a/scripts/__tests__/lint-no-dead-package-paths.test.mjs +++ b/scripts/__tests__/lint-no-dead-package-paths.test.mjs @@ -1,6 +1,7 @@ import { execFileSync } from 'node:child_process' -import { mkdirSync, rmSync, writeFileSync } from 'node:fs' -import { resolve } from 'node:path' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' @@ -114,6 +115,49 @@ describe('lint-no-dead-package-paths', () => { } }) + // The whole reason `livePackages` comes from git rather than from + // `readdirSync`, pinned in the discriminating direction (#772 review, + // finding 15). + // + // Deleting a package leaves its gitignored `dist/` and `node_modules/` + // behind, so the directory is still on disk and any filesystem-derived live + // set calls it live — silently excusing every reference to it. The test + // above, `treats a package that exists but is not yet tracked as live`, does + // NOT pin this: it builds a divergence that `readdirSync` and git agree on, + // so it passes under a revert. This one is the only test that fails under + // both a plain `readdirSync` revert and the more plausible + // `readdirSync`-unioned-with-git hybrid. + // + // The shell's contents MUST be gitignored. If they aren't, the + // `--others --exclude-standard` arm legitimately counts the package as live + // and this test would assert the opposite of what it claims — so guard on a + // clean `git status` and fail loudly rather than silently degrading. + it('flags a deleted package whose gitignored dist/ shell survives on disk', () => { + const shell = resolve(REPO_ROOT, 'packages/lint-dead-shell-probe') + try { + mkdirSync(resolve(shell, 'dist'), { recursive: true }) + writeFileSync(resolve(shell, 'dist/index.js'), 'exports.x = 1\n') + mkdirSync(resolve(shell, 'node_modules'), { recursive: true }) + writeFileSync(resolve(shell, 'node_modules/.keep'), '') + + // Scoped to the probe alone: asserting the whole `packages/` tree is + // clean would couple this to whatever else is uncommitted in the + // working copy, which is nobody's business but the developer's. + const visible = execFileSync( + 'git', + ['status', '--porcelain', 'packages/lint-dead-shell-probe'], + { cwd: REPO_ROOT, encoding: 'utf8' }, + ) + expect(visible).toBe('') + + const r = run(fx('dead-shell.md')) + expect(r.exitCode).toBe(1) + expect(r.output).toMatch(/packages\/lint-dead-shell-probe/) + } finally { + rmSync(shell, { recursive: true, force: true }) + } + }) + // The live set is derived by shelling out to git, so git failing is a mode // this linter has to own. Exiting 1 with a raw ENOENT stack trace would be // indistinguishable from a genuine lint failure; exit 2 says "the linter @@ -128,6 +172,41 @@ describe('lint-no-dead-package-paths', () => { expect(r.output).not.toMatch(/at ModuleJob/) }) + // A linter whose whole job is catching dead paths in configuration silently + // ignored dead paths in its OWN configuration: a target that did not exist + // was skipped, so a renamed or moved entry dropped out of coverage forever + // with a green build. Its sibling `lint-no-hardcoded-runners.mjs` exits 2 on + // a stale allowlist entry — same rot, opposite handling, same PR. + // + // Exit 2, not 1: the linter could not check what it was asked to check, + // which is not the same as "your docs are wrong". + it('exits 2 when a target does not exist rather than skipping it', () => { + const r = run(fx('no-such-fixture.md')) + expect(r.exitCode).toBe(2) + expect(r.output).toMatch(/no-such-fixture\.md/) + expect(r.output).not.toMatch(/at ModuleJob/) + }) + + // `relative(REPO_ROOT, file)` renders a target outside the repo as a + // `../../../../..` chain climbing out of the root — technically resolvable, + // unreadable in practice, and it buries the one thing the line is for. + // Unreachable via the default target list, which is repo-relative + // throughout; reachable the moment anyone passes an argv override. + it('renders an absolute path for an offender outside the repo root', () => { + const dir = mkdtempSync(join(tmpdir(), 'lint-dead-package-paths-')) + try { + const file = join(dir, 'outside.md') + writeFileSync(file, 'Points at packages/lint-dead-shell-probe, gone.\n') + const r = run(file) + expect(r.exitCode).toBe(1) + expect(r.output).toMatch(/packages\/lint-dead-shell-probe/) + expect(r.output).toContain(file) + expect(r.output).not.toMatch(/\.\.\//) + } finally { + rmSync(dir, { recursive: true, force: true }) + } + }) + it('names the file and line of each offender', () => { const r = run(fx('dead-ref.md')) expect(r.output).toMatch(/dead-ref\.md:3/) diff --git a/scripts/__tests__/lint-no-hardcoded-runners.test.mjs b/scripts/__tests__/lint-no-hardcoded-runners.test.mjs index 96531ea0e..709ac63ed 100644 --- a/scripts/__tests__/lint-no-hardcoded-runners.test.mjs +++ b/scripts/__tests__/lint-no-hardcoded-runners.test.mjs @@ -1,6 +1,7 @@ import { execFileSync } from 'node:child_process' -import { readFileSync, rmSync, writeFileSync } from 'node:fs' -import { resolve } from 'node:path' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' @@ -40,6 +41,32 @@ describe('lint-no-hardcoded-runners', () => { expect(r.output).toMatch(/\bnpx\b/) }) + // `statSync` on a missing target threw uncaught: exit 1 plus a raw stack + // trace, indistinguishable from "found a hardcoded npx" to anything reading + // the exit code. Exit 2 means the linter could not run. + it('exits 2 when a target does not exist', () => { + const r = run(fx('no-such-fixture.ts')) + expect(r.exitCode).toBe(2) + expect(r.output).toContain('no-such-fixture.ts') + expect(r.output).not.toMatch(/at ModuleJob/) + }) + + // Matches the sibling linter: a target outside the repo rendered as a + // `../../../../..` chain out of the root instead of naming the file. + it('renders an absolute path for an offender outside the repo root', () => { + const dir = mkdtempSync(join(tmpdir(), 'lint-hardcoded-runners-')) + try { + const file = join(dir, 'outside.ts') + writeFileSync(file, "export const cmd = 'npx drizzle-kit generate'\n") + const r = run(file) + expect(r.exitCode).toBe(1) + expect(r.output).toContain(file) + expect(r.output).not.toMatch(/\.\.\//) + } finally { + rmSync(dir, { recursive: true, force: true }) + } + }) + it("ignores `?? 'npx'` fallback expressions", () => { expect(run(fx('allowed-fallback.ts')).exitCode).toBe(0) }) diff --git a/scripts/lint-no-dead-package-paths.mjs b/scripts/lint-no-dead-package-paths.mjs index 9fe1a4984..321b30e04 100644 --- a/scripts/lint-no-dead-package-paths.mjs +++ b/scripts/lint-no-dead-package-paths.mjs @@ -152,19 +152,39 @@ for (const target of TARGETS) { } catch { exists = false } - // A default target that doesn't exist is not an error — the list covers - // optional files (CONTRIBUTE.md, per-package AGENTS.md). - if (!exists) continue + // A target that isn't there used to be skipped in silence, on the theory + // that the default list covered optional files. It doesn't — all of TARGETS + // is tracked and present — and the silence was the same rot this linter + // exists to catch, one level up: rename a target and it drops out of + // coverage forever with a green build. `lint-no-hardcoded-runners.mjs` + // already exits 2 on a stale allowlist entry; this is that rule applied to + // this linter's own configuration. + if (!exists) { + console.error( + `Target \`${target}\` does not exist.\n\n` + + 'Either it was renamed or removed — in which case update TARGETS in\n' + + 'this script, since a target that is silently skipped is coverage\n' + + 'quietly lost — or it was mistyped on the command line.', + ) + // Exit 2, not 1: the linter could not check what it was asked to check. + process.exit(2) + } for (const file of walk(abs)) { + // A target outside the repo (only reachable via an argv override — every + // default is repo-relative) renders as a `../../../../..` chain climbing + // out of the root, which buries the filename it exists to point at. const rel = relative(REPO_ROOT, file) + const shown = rel.startsWith('..') ? file : rel if (SKIP_FILES.has(rel.split('/').pop())) continue const lines = readFileSync(file, 'utf8').split('\n') lines.forEach((line, idx) => { PACKAGE_REF.lastIndex = 0 for (const m of line.matchAll(PACKAGE_REF)) { if (livePackages.has(m[1])) continue - offenders.push(`${rel}:${idx + 1}: \`packages/${m[1]}\` does not exist`) + offenders.push( + `${shown}:${idx + 1}: \`packages/${m[1]}\` does not exist`, + ) } }) } diff --git a/scripts/lint-no-hardcoded-runners.mjs b/scripts/lint-no-hardcoded-runners.mjs index 95b216570..3070963c8 100644 --- a/scripts/lint-no-hardcoded-runners.mjs +++ b/scripts/lint-no-hardcoded-runners.mjs @@ -116,10 +116,28 @@ if (staleAllowlist.length > 0) { const offenders = [] for (const target of TARGETS) { const abs = resolve(REPO_ROOT, target) - const stat = statSync(abs) + let stat + try { + stat = statSync(abs) + } catch { + // Was an uncaught throw: exit 1 plus a raw stack trace, which reads to + // anything checking the exit code exactly like a genuine `npx` finding. + // Exit 2 says the linter could not run — the same contract the allowlist + // self-check above already uses. + console.error( + `Target \`${target}\` does not exist.\n\n` + + 'Update the scan roots in this script if it was renamed or removed,\n' + + 'or check the path passed on the command line.', + ) + process.exit(2) + } const files = stat.isDirectory() ? walk(abs) : [abs] for await (const file of files) { + // `rel` stays repo-relative for the allowlist lookup; only the rendering + // falls back to the absolute path, for a target outside the repo root that + // would otherwise print a `../../../../..` chain instead of a filename. const rel = relative(REPO_ROOT, file) + const shown = rel.startsWith('..') ? file : rel if (ALLOWLISTED_PATHS.has(rel)) continue if (/\.(test|spec)\.(ts|tsx|mts|cts)$/.test(file)) continue const lines = readFileSync(file, 'utf8').split('\n') @@ -129,7 +147,7 @@ for (const target of TARGETS) { if (isCommentLine(line)) return if (isAllowedFallback(line)) return if (isAllowedRunnerSwitch(line)) return - offenders.push(`${rel}:${idx + 1}: ${line.trim()}`) + offenders.push(`${shown}:${idx + 1}: ${line.trim()}`) }) } } From 24f66c610380700e90788bc9732ddbe1126a8738 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Mon, 27 Jul 2026 17:09:47 +1000 Subject: [PATCH 4/5] fix(cli): stop the plan templates drafting a v2-only cutover on a v3 default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stash plan` writes one of three templates. Two of them — the cutover plan and the `--complete-rollout` plan — described the EQL v2 rename swap as the only way to switch reads: "a single transaction renames `` → `_plaintext` and `_encrypted` → ``", with the steps after it referring to `_plaintext` as though the rename had certainly happened. On EQL v3, which is the default, none of that is true. `stash encrypt cutover` refuses a v3 column outright — "Cut-over is not applicable to EQL v3 columns … there is no rename step" (`encrypt/cutover.ts:119`) — and refuses entirely on a v3-only database, where the `eql_v2_configuration` relation it needs does not exist (`:142`). So on a default install the agent was being asked to draft a plan around a command that will decline to run, and to name a `_plaintext` column that will never exist. The implement prompt in this same file already splits the two correctly. This copies that split into the templates that were missing it. The EQL version is per-column — one database can hold both — so the templates tell the agent to establish it per column (`stash encrypt status`) rather than deciding once for the whole plan; a context-level flag would have been wrong for a mixed database. Also: the "this column is already encrypted" stop-and-ask keyed on the `eql_v2_encrypted` udt alone. v3 columns carry `eql_v3_*` domains, so on the default path the check could never fire. The agent reads the schema itself, so naming both is sufficient here. `introspect.ts:88` has the same v2-only assumption in code (`isEqlEncrypted: row.udt_name === 'eql_v2_encrypted'`). Left alone deliberately: it decides which columns `stash init` pre-selects as already-managed, so widening it changes setup behaviour and wants its own tests rather than riding along here. --- .changeset/decrypt-chaining-docs.md | 14 +++++ .../init/lib/__tests__/setup-prompt.test.ts | 60 +++++++++++++++++++ .../cli/src/commands/init/lib/setup-prompt.ts | 25 ++++---- 3 files changed, 85 insertions(+), 14 deletions(-) diff --git a/.changeset/decrypt-chaining-docs.md b/.changeset/decrypt-chaining-docs.md index 5345a1e9c..e955fd495 100644 --- a/.changeset/decrypt-chaining-docs.md +++ b/.changeset/decrypt-chaining-docs.md @@ -47,3 +47,17 @@ other two to guess: (`{ table: usersSchema, column: usersSchema.email }`) rather than an object-shorthand that read as three required strings — `queryType` is inferred from the column's configured indexes. + +The cutover and complete-rollout **plan templates** now split EQL v3 from v2. +Both described the v2 rename swap (`` → `_plaintext`, twin → ``) +as the only cutover path, so on the default v3 install `stash plan` drafted a +plan built around `stash encrypt cutover` — a command that refuses v3 columns +outright ("there is no rename step") and refuses entirely on a v3-only +database, where `eql_v2_configuration` does not exist. The implement prompt +already carried this split; the plan templates did not. The version is +per-column, so the templates tell the agent to establish it per column rather +than deciding once for the whole plan. + +The "already encrypted" stop-and-ask now recognises `eql_v3_*` domains +alongside the legacy `eql_v2_encrypted` udt, so it can fire on the default +path at all. diff --git a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts index 8ea972d9f..ee1c302b7 100644 --- a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts @@ -566,6 +566,66 @@ describe('renderSetupPrompt — no db push recommendations', () => { }) }) +// The rename swap is an EQL v2 operation. `stash encrypt cutover` refuses a v3 +// column outright ("Cut-over is not applicable to EQL v3 columns … there is no +// rename step", `encrypt/cutover.ts`) and refuses entirely on a v3-only +// install, where `eql_v2_configuration` does not exist. v3 is the default, so +// a template presenting the rename as the only path drafts a plan around a +// command that will decline to run. The implement prompt already splits the +// two; these templates did not. +describe('renderSetupPrompt — plan templates split EQL v3 from v2', () => { + const plan = (planStep: 'cutover' | 'complete') => + renderSetupPrompt({ ...baseCtx, mode: 'plan', planStep }) + + for (const planStep of ['cutover', 'complete'] as const) { + describe(`${planStep} plan`, () => { + it('names both versions and states v3 has no rename', () => { + const out = plan(planStep) + expect(out).toContain('EQL v3') + expect(out).toContain('EQL v2') + expect(out).toMatch(/no rename/i) + }) + + it('does not present the rename swap unconditionally', () => { + const out = plan(planStep) + // Every mention of the rename swap has to sit next to the v2 label. + for (const line of out.split('\n')) { + if (/_plaintext/.test(line)) { + expect(line).toMatch(/v2|v3/) + } + } + }) + + it('tells the agent how to determine the version per column', () => { + // The version is per-column — a database can hold both — so this + // cannot be decided once for the whole plan. + expect(plan(planStep)).toMatch(/encrypt status|encrypt backfill/) + }) + }) + } + + // The stop-and-ask trigger for "already encrypted" named the v2 UDT alone. + // v3 is the default and its columns carry `eql_v3_*` domains, so on the + // default path the check silently never fired. The agent reads the schema + // itself, so this half is fixable here; `introspect.ts`'s `isEqlEncrypted` + // has the same gap and is a separate behavioural change. + it('recognises v3 domains in the already-encrypted stop-and-ask', () => { + for (const mode of ['implement', 'plan'] as const) { + const out = renderSetupPrompt({ ...baseCtx, mode }) + expect(out).toMatch(/eql_v3_/) + } + }) + + it('keeps the cutover invocation scoped to v2', () => { + const out = plan('cutover') + const cutoverLine = out + .split('\n') + .find((l) => l.includes('encrypt cutover --table')) + expect(cutoverLine).toBeDefined() + expect(cutoverLine).toMatch(/v2/) + }) +}) + describe('renderSetupPrompt — honours what the handoff actually wrote', () => { for (const mode of ['implement', 'plan'] as const) { it(`claude-code with no skills points at neither a skills dir nor AGENTS.md (${mode})`, () => { diff --git a/packages/cli/src/commands/init/lib/setup-prompt.ts b/packages/cli/src/commands/init/lib/setup-prompt.ts index 03d9a7e3b..ac97a3687 100644 --- a/packages/cli/src/commands/init/lib/setup-prompt.ts +++ b/packages/cli/src/commands/init/lib/setup-prompt.ts @@ -414,7 +414,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { "The user asks to convert a populated column in place. Explain why it doesn't work and offer the migrate-existing-column flow instead.", ), bullet( - "A column the user names is already encrypted (`eql_v2_encrypted` udt) but with a different EQL config than they've described. This is the post-cutover re-encryption case (`stash encrypt update`, not yet shipped) — surface it instead of guessing.", + "A column the user names is already encrypted — an `eql_v3_*` domain (`eql_v3_text_search`, `eql_v3_integer_ord`, …) on the default path, or the legacy `eql_v2_encrypted` udt — but with a different EQL config than they've described. This is the post-cutover re-encryption case (`stash encrypt update`, not yet shipped) — surface it instead of guessing.", ), bullet( 'The schema migration would change the data type of a column the user has already filled.', @@ -516,7 +516,7 @@ function planSharedStopAndAsk(): string[] { "The user asks to convert a populated column in place. Explain why it doesn't work and offer the migrate-existing-column flow instead.", ), bullet( - "A column the user names is already encrypted (`eql_v2_encrypted` udt) but with a different EQL config than they've described. This is the post-cutover re-encryption case (`stash encrypt update`, not yet shipped) — surface it in the plan as a flagged risk.", + "A column the user names is already encrypted — an `eql_v3_*` domain (`eql_v3_text_search`, `eql_v3_integer_ord`, …) on the default path, or the legacy `eql_v2_encrypted` udt — but with a different EQL config than they've described. This is the post-cutover re-encryption case (`stash encrypt update`, not yet shipped) — surface it in the plan as a flagged risk.", ), bullet( 'You discover existing partial CipherStash setup that disagrees with what the user is describing — someone else may have run `stash init` earlier with different choices. Note this in the plan and ask the user to clarify before writing prescriptive steps.', @@ -644,19 +644,16 @@ function renderCutoverPlanPrompt(ctx: SetupPromptContext): string { '**Backfill.** Encrypt the historical rows that pre-date the rollout deploy. Resumable; chunked; SIGINT-safe.', ), bullet( - '**Schema rename.** Update the schema declaration so the original column points at the encrypted type.', + `**Switch reads to the encrypted column.** This step depends on the column's EQL version, so establish it per column first — \`${cli} encrypt backfill\` prints it and \`${cli} encrypt status\` shows it. **EQL v3 (the default):** there is no rename. Update the schema declaration and the queries to read and write \`_encrypted\` under its own name. **EQL v2 (legacy data only):** declare the encrypted column under its final name (drop the twin suffix), then a single \`${cli} encrypt cutover\` transaction renames \`\` → \`_plaintext\` and \`_encrypted\` → \`\`.`, ), bullet( - '**Cutover.** A single transaction renames `` → `_plaintext` and `_encrypted` → ``.', + '**Read path.** Reads of the encrypted column return ciphertext until the read path decrypts via the encryption client. The plan must specify what changes per read site.', ), bullet( - '**Read path.** Application reads of `` now return ciphertext until the read path decrypts via the encryption client. The plan must specify what changes per read site.', + '**Remove dual-writes.** The plaintext column — still `` on v3, renamed `_plaintext` on v2 — is no longer authoritative. Delete the dual-write code paths.', ), bullet( - '**Remove dual-writes.** The plaintext column is now `_plaintext` and is no longer authoritative. Delete the dual-write code paths.', - ), - bullet( - "**Drop plaintext.** `stash encrypt drop` emits a migration that removes `_plaintext`. Apply with the project's normal migration tooling.", + `**Drop plaintext.** \`${cli} encrypt drop\` emits a migration that removes the now-unused plaintext column; on v3 it first verifies no rows are still plaintext-only. Apply with the project's normal migration tooling.`, ), '', '## Your task: produce the cutover plan file', @@ -679,12 +676,12 @@ function renderCutoverPlanPrompt(ctx: SetupPromptContext): string { ' encrypt backfill` invocation with concrete `--table` / `--column` values.', ), bullet( - 'The schema-edit step, with the exact rename pattern (drop `_encrypted` suffix on the encrypted column, switch the original column declaration off `text`/`varchar` and onto the encrypted type).', + 'The schema-edit step, stated per column against its EQL version. **v3:** point the declaration and queries at `_encrypted` under its own name — there is no rename, and no `_encrypted` suffix to drop. **v2:** drop the `_encrypted` suffix and switch the original column declaration off `text`/`varchar` and onto the encrypted type.', ), bullet( - 'The cutover invocation per column: `' + + 'For EQL v2 columns only, the cutover invocation per column: `' + cli + - ' encrypt cutover --table --column `.', + ' encrypt cutover --table --column `. It refuses v3 columns — there is nothing to rename — so do not schedule it for them.', ), bullet( 'Read-path code changes: every site that reads `` from this table must decrypt via the encryption client. Enumerate the sites you can find via grep so the user can verify nothing was missed.', @@ -696,7 +693,7 @@ function renderCutoverPlanPrompt(ctx: SetupPromptContext): string { ' encrypt drop --table --column `, plus the schema-migration apply step that follows.', ), bullet( - `Risks specific to cutover: row-count for the backfill (use \`${cli} eql status\` to estimate if helpful), tables under heavy write load (cutover holds a brief lock on the rename), application code that constructs SQL by string (those reads won't transparently decrypt).`, + `Risks specific to cutover: row-count for the backfill (use \`${cli} eql status\` to estimate if helpful), tables under heavy write load (on v2, cutover holds a brief lock for the rename), application code that constructs SQL by string (those reads won't transparently decrypt).`, ), bullet( "Open questions for the user — anything you can't determine from the schema, context.json, or the skills.", @@ -741,7 +738,7 @@ function renderCompletePlanPrompt(ctx: SetupPromptContext): string { '**Add new encrypted columns** — declared encrypted from the start; single-deploy.', ), bullet( - '**Migrate existing columns** — schema-add → dual-write code → backfill → schema rename → cutover → read-path switch → remove dual-write code → drop plaintext. No deploy gate between rollout and cutover steps because there is no deployed application to gate on.', + `**Migrate existing columns** — schema-add → dual-write code → backfill → switch reads to the encrypted column → remove dual-write code → drop plaintext. The switch step depends on the column's EQL version (\`${cli} encrypt status\` shows it): on **EQL v3**, the default, there is no rename — point the schema and queries at \`_encrypted\` by name; on **EQL v2** only, \`${cli} encrypt cutover\` renames the twin into \`\` first. No deploy gate between rollout and cutover steps because there is no deployed application to gate on.`, ), '', '## Your task: produce the complete-rollout plan file', From 0585f4d21f28139e8f7457199ceee779cab0e8f6 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Mon, 27 Jul 2026 17:51:46 +1000 Subject: [PATCH 5/5] fix(cli): detect already-encrypted columns on EQL v3 during init MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `introspectDatabase` marked a column as CipherStash-managed only when its udt was exactly `eql_v2_encrypted`: isEqlEncrypted: row.udt_name === 'eql_v2_encrypted' v3 is the default generation and its columns carry per-domain types — `eql_v3_text_search`, `eql_v3_integer_ord`, and so on — so on the default path this was false for every encrypted column. The consequences are all in the column picker: the table hint never said "N already encrypted", the pre-selection notice never fired, and each encrypted column was displayed with its plaintext `dataType` and left unticked. An encrypted column presented as plaintext invites the user to encrypt it a second time, which is the direction of wrongness that costs data rather than time. `packages/wizard` already had the correct predicate, with the reasoning written down (`isEqlEncryptedDomain`, `wizard-tools.ts:167`). This mirrors it rather than inventing a second answer, and exports it so it is testable — the old inline comparison sat inside a function that needs a live pg connection, which is why nothing covered it. Deliberately not `classifyEqlDomain` from `@cipherstash/migrate`, despite the dependency already being present: that answers "which generation authors this" and returns null for `eql_v2_encrypted`, since v2 is no longer authorable. The question here is "is this encrypted at all", and v2 answers yes. The per-column hint was also the hardcoded string `eql_v2_encrypted` for any encrypted column, which mislabels a v3 column with a domain it does not have. It now reports the column's actual udt, and the pre-selection notice lists the domains it found instead of naming one it may not have seen. Checked the rest of the CLI for the same assumption: every other site either already handles both generations (`rewrite-migrations.ts` matches `eql_v2_encrypted|eql_v3_[a-z0-9_]+`, `db-readers.ts`, `backfill.ts`) or is legitimately v2-only (`cutover.ts`, the installer's CREATE TYPE permission messages). Introspection was the sole outlier. --- .changeset/decrypt-chaining-docs.md | 9 +++ .../init/lib/__tests__/introspect.test.ts | 71 +++++++++++++++++++ .../cli/src/commands/init/lib/introspect.ts | 38 ++++++++-- 3 files changed, 111 insertions(+), 7 deletions(-) diff --git a/.changeset/decrypt-chaining-docs.md b/.changeset/decrypt-chaining-docs.md index e955fd495..19bcbb8ea 100644 --- a/.changeset/decrypt-chaining-docs.md +++ b/.changeset/decrypt-chaining-docs.md @@ -61,3 +61,12 @@ than deciding once for the whole plan. The "already encrypted" stop-and-ask now recognises `eql_v3_*` domains alongside the legacy `eql_v2_encrypted` udt, so it can fire on the default path at all. + +**`stash init` now detects already-encrypted columns on EQL v3.** Database +introspection marked a column as CipherStash-managed only when its udt was +exactly `eql_v2_encrypted`. v3 columns carry per-domain types +(`eql_v3_text_search`, `eql_v3_integer_ord`, …), so on the default path every +encrypted column was reported as plaintext — shown with its `dataType` and left +unticked in the column picker, inviting a re-run to encrypt it a second time. +The picker also labelled any encrypted column with the literal string +`eql_v2_encrypted`; it now shows the column's real domain. diff --git a/packages/cli/src/commands/init/lib/__tests__/introspect.test.ts b/packages/cli/src/commands/init/lib/__tests__/introspect.test.ts index 40a011e95..1efca50e3 100644 --- a/packages/cli/src/commands/init/lib/__tests__/introspect.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/introspect.test.ts @@ -5,6 +5,7 @@ import type { DataType } from '../../types.js' import { candidateDomains, defaultDomain, + isEqlEncryptedDomain, pgTypeToDataType, selectTableColumns, } from '../introspect.js' @@ -103,6 +104,44 @@ describe('defaultDomain', () => { }) }) +// The marker used to be `udt_name === 'eql_v2_encrypted'` alone. v3 is the +// default generation and its columns carry `eql_v3_*` domains, so on the +// default path an already-encrypted column was reported as plaintext: shown +// with its `dataType` hint and left unticked. `packages/wizard` already made +// this call, and its comment names the consequence — misreporting encrypted +// columns as plaintext lets an agent clobber real ciphertext. +describe('isEqlEncryptedDomain', () => { + it('recognises every eql_v3_* domain', () => { + for (const udt of [ + 'eql_v3_encrypted', + 'eql_v3_text_search', + 'eql_v3_integer_ord', + 'eql_v3_date_ord', + 'eql_v3_json', + ]) { + expect(isEqlEncryptedDomain(udt)).toBe(true) + } + }) + + it('still recognises the legacy v2 udt', () => { + expect(isEqlEncryptedDomain('eql_v2_encrypted')).toBe(true) + }) + + it('does not claim plaintext types', () => { + for (const udt of ['text', 'int4', 'jsonb', 'bool', 'timestamptz']) { + expect(isEqlEncryptedDomain(udt)).toBe(false) + } + }) + + // The trailing underscore is load-bearing: a bare `startsWith('eql_v3')` + // would also claim a hypothetical future `eql_v30_*` generation. Same + // reasoning as `classifyEqlDomain` in `@cipherstash/migrate`. + it('does not claim a future generation by prefix', () => { + expect(isEqlEncryptedDomain('eql_v30_text_search')).toBe(false) + expect(isEqlEncryptedDomain('eql_v3')).toBe(false) + }) +}) + describe('selectTableColumns', () => { beforeEach(() => { selectMock.mockReset() @@ -214,6 +253,38 @@ describe('selectTableColumns', () => { expect(schema?.columns).toEqual([{ name: 'ssn', domain: 'TextSearch' }]) }) + // The per-column hint was the literal string 'eql_v2_encrypted' for any + // encrypted column, so a v3 column was labelled with a domain it does not + // have. Report the column's actual udt. + it('hints an encrypted column with its real domain, not a hardcoded v2 udt', async () => { + const withV3 = [ + { + tableName: 'accounts', + columns: [ + { + columnName: 'ssn', + dataType: 'jsonb', + udtName: 'eql_v3_text_search', + isEqlEncrypted: true, + }, + ], + }, + ] + selectMock + .mockResolvedValueOnce('accounts') + .mockResolvedValueOnce('TextSearch') + multiselectMock.mockResolvedValueOnce(['ssn']) + + await selectTableColumns(withV3) + + const opts = multiselectMock.mock.calls[0][0].options + expect(opts).toEqual([ + expect.objectContaining({ value: 'ssn', hint: 'eql_v3_text_search' }), + ]) + // And it starts ticked, so a re-run does not offer to re-encrypt it. + expect(multiselectMock.mock.calls[0][0].initialValues).toEqual(['ssn']) + }) + it('pre-selects the widest searchable domain as the per-column default', async () => { // Asserts the ARGS to the domain prompt, not just its return: the picker // must pass initialValue: defaultDomain(options) so the widest searchable diff --git a/packages/cli/src/commands/init/lib/introspect.ts b/packages/cli/src/commands/init/lib/introspect.ts index 2e4a4c732..31b7bb398 100644 --- a/packages/cli/src/commands/init/lib/introspect.ts +++ b/packages/cli/src/commands/init/lib/introspect.ts @@ -42,12 +42,36 @@ export function pgTypeToDataType(udtName: string): DataType { } } +/** + * Is this column already managed by CipherStash? + * + * Both generations count. v3 is the sole generation this workspace authors, + * and its columns carry per-domain types (`eql_v3_text_search`, + * `eql_v3_integer_ord`, …) rather than v2's single `eql_v2_encrypted` udt — + * so keying on the v2 name alone, as this did, reported every column on the + * default path as plaintext. That is the dangerous direction to be wrong in: + * an encrypted column shown as plaintext invites the caller to encrypt it a + * second time. `packages/wizard` already carries this predicate; the two + * should agree. + * + * Deliberately not `classifyEqlDomain` from `@cipherstash/migrate`, despite + * the dependency being present: that answers "which generation authors this", + * and returns `null` for `eql_v2_encrypted` because v2 is no longer authorable. + * The question here is "is this encrypted at all", which v2 answers yes to. + * + * The trailing underscore matters — a bare `eql_v3` prefix would also claim a + * hypothetical future `eql_v30_*`. + */ +export function isEqlEncryptedDomain(udtName: string): boolean { + return udtName === 'eql_v2_encrypted' || udtName.startsWith('eql_v3_') +} + /** * Read every base table in the `public` schema along with its columns. * - * The `eql_v2_encrypted` UDT marker tells us a column is already managed by - * CipherStash — useful for re-runs against a partially set up DB so we can - * pre-select those columns rather than asking the user to reconfirm. + * The EQL domain markers tell us a column is already managed by CipherStash — + * useful for re-runs against a partially set up DB so we can pre-select those + * columns rather than asking the user to reconfirm. */ export async function introspectDatabase( databaseUrl: string, @@ -85,7 +109,7 @@ export async function introspectDatabase( columnName: row.column_name, dataType: row.data_type, udtName: row.udt_name, - isEqlEncrypted: row.udt_name === 'eql_v2_encrypted', + isEqlEncrypted: isEqlEncryptedDomain(row.udt_name), }) tableMap.set(row.table_name, cols) } @@ -203,7 +227,7 @@ export function defaultDomain( * Returns `undefined` if the user cancels at any prompt — callers should * propagate the cancellation rather than treating it as "no columns selected". * - * Pre-selects columns that are already `eql_v2_encrypted` so re-running on a + * Pre-selects columns that already carry an EQL domain so re-running on a * partially encrypted DB is a no-op by default. */ export async function selectTableColumns( @@ -230,7 +254,7 @@ export async function selectTableColumns( if (eqlColumns.length > 0) { p.log.info( - `Detected ${eqlColumns.length} column${eqlColumns.length !== 1 ? 's' : ''} with eql_v2_encrypted type — pre-selected for you.`, + `Detected ${eqlColumns.length} already-encrypted column${eqlColumns.length !== 1 ? 's' : ''} (${[...new Set(eqlColumns.map((c) => c.udtName))].join(', ')}) — pre-selected for you.`, ) } @@ -239,7 +263,7 @@ export async function selectTableColumns( options: table.columns.map((col) => ({ value: col.columnName, label: col.columnName, - hint: col.isEqlEncrypted ? 'eql_v2_encrypted' : col.dataType, + hint: col.isEqlEncrypted ? col.udtName : col.dataType, })), required: true, initialValues: eqlColumns.map((c) => c.columnName),