From f84ac7d947f9c3aece5973d1ae6a72451699d3ca Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:29:40 +1000 Subject: [PATCH 1/6] fix(cli): stop encrypt cutover/drop acting on a guessed encrypted column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a mixed table — a legacy v2 pair the classifier no longer sees, plus one unrelated EQL v3 column — pickEncryptedColumn's sole-EQL-column rule claims the v3 column for the v2 plaintext. Three separate defects turned that guess into wrong outcomes. cutover had no `via` gate at all. Its v3 branch returns from inside try with exitCode untouched, so the finally never fires process.exit: it printed "point your application at email_enc" and exited 0 while the v2 rename never ran. drop.ts:106 already gated on `via === 'sole'` for exactly this reason. The manifest hint was discarded. backfill records the true pairing, so the answer was on disk, but resolveColumnLifecycle dropped a hint that failed to resolve and re-picked without it — reaching the sole rule. Recording the pairing changed nothing. Split the two reasons a hint fails: a column that is GONE is stale and still falls through to convention; a column that EXISTS but is not EQL v3 (the legacy eql_v2_encrypted case) is reported by name. drop's remedy prescribed the guess — `--encrypted-column `. Recording it makes the next resolution `via: 'hint'`, which walks past the gate, and the coverage check passes vacuously because an unrelated backfilled column is non-NULL on every row. The message was the instruction manual for generating a live DROP COLUMN on the plaintext at exit 0. Tested at the layer that had none: resolve-eql.test.ts covered only explainUnresolved, and encrypt-v3.test.ts stubs resolveColumnLifecycle outright, so neither could see the hint discard. The new tests keep pickEncryptedColumn real and replace only the two I/O boundaries. --- .changeset/encrypt-lifecycle-mixed-table.md | 39 +++++ .../encrypt/__tests__/encrypt-v3.test.ts | 44 +++++- packages/cli/src/commands/encrypt/cutover.ts | 18 ++- packages/cli/src/commands/encrypt/drop.ts | 12 +- .../encrypt/lib/__tests__/resolve-eql.test.ts | 149 +++++++++++++++++- .../src/commands/encrypt/lib/resolve-eql.ts | 64 +++++++- 6 files changed, 315 insertions(+), 11 deletions(-) create mode 100644 .changeset/encrypt-lifecycle-mixed-table.md diff --git a/.changeset/encrypt-lifecycle-mixed-table.md b/.changeset/encrypt-lifecycle-mixed-table.md new file mode 100644 index 000000000..78d41244e --- /dev/null +++ b/.changeset/encrypt-lifecycle-mixed-table.md @@ -0,0 +1,39 @@ +--- +'stash': patch +--- + +`stash encrypt cutover` and `stash encrypt drop` no longer act on — or report +success for — an encrypted column they only guessed at. + +On a table holding both a legacy EQL v2 pair (`ssn` / `ssn_encrypted`) and one +unrelated EQL v3 column, the v2 ciphertext column is not classified as an EQL +column at all, so column resolution fell through to the "this is the table's +only EQL column" rule and claimed the unrelated v3 column. Three consequences, +all now fixed: + +- **`cutover` reported success for work it never did.** Its EQL v3 branch had no + guard on how the column was resolved, and returned without setting an exit + code — so it printed "point your application at `email_enc`" and exited 0 + while the v2 rename never ran. A scripted rollout read that as complete. It + now refuses, and exits 1, exactly as `drop` already did. + +- **The recorded pairing was discarded.** `encrypt backfill` writes the true + `encryptedColumn` to `.cipherstash/migrations.json`, so the answer was already + on disk — but a hint that failed to resolve was dropped entirely and the + re-resolution reached the guess. A hint naming a column that still exists but + is not an EQL v3 column is now reported as what it is (most often a legacy + `eql_v2_encrypted` counterpart) instead of being replaced by a guess. A + genuinely stale hint — one naming a column that is gone — still falls back to + the naming convention as before. + +- **`drop`'s refusal message prescribed the guess.** It told the user to re-run + `backfill --encrypted-column `. Following it recorded the + guess as fact, so the next run resolved "by hint", walked past the refusal, + and passed the coverage check vacuously — an unrelated but legitimately + backfilled column is non-NULL on every row — then generated a live + `DROP COLUMN` on the plaintext and exited 0. The message now asks for the + column that actually encrypts the named one, and says explicitly not to record + the guess. + +Pure-v2 and pure-v3 tables are unaffected, as are tables with two or more EQL v3 +columns (resolution already failed closed there). diff --git a/packages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.ts b/packages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.ts index 45def1ef2..a0d583294 100644 --- a/packages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.ts +++ b/packages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.ts @@ -216,6 +216,36 @@ describe('encrypt cutover — EQL version awareness', () => { ) }) + // #772 review, finding 7. `drop` gated on `via === 'sole'`; `cutover` did + // not, and its v3 branch `return`s without setting exitCode — so a mixed + // table (a v2 pair the classifier no longer sees, plus one unrelated v3 + // column) produced a success-shaped message and exit 0 while the v2 rename + // never ran. A scripted rollout read that as done. + it("refuses a by-elimination ('sole') match rather than reporting success", async () => { + lifecycleMock.mockResolvedValue( + resolved( + { column: 'email_enc', domain: 'eql_v3_text_search', version: 3 }, + 'sole', + ), + ) + migrateMocks.progress.mockResolvedValue({ phase: 'backfilled' }) + const exitSpy = spyExit() + + await cutoverCommand({ table: 'users', column: 'ssn' }) + + expect(p.log.error).toHaveBeenCalledWith( + expect.stringContaining('nothing confirms it encrypts "ssn"'), + ) + // The old message told the user to point their application at the guessed + // column, as though the lifecycle were complete. + expect(p.log.info).not.toHaveBeenCalledWith( + expect.stringContaining('point your application at email_enc'), + ) + expect(migrateMocks.renameEncryptedColumns).not.toHaveBeenCalled() + expect(exitSpy).toHaveBeenCalledWith(1) + exitSpy.mockRestore() + }) + it('fails closed when EQL columns exist but none is identifiable', async () => { lifecycleMock.mockResolvedValue({ info: null, @@ -372,9 +402,21 @@ describe('encrypt drop — EQL version awareness', () => { expect(p.log.error).toHaveBeenCalledWith( expect.stringContaining('nothing confirms it encrypts "email"'), ) - expect(p.log.error).toHaveBeenCalledWith( + // The remedy must not prescribe the GUESS. Recording `secret_blob` makes + // the next resolution `via: 'hint'`, which walks past this very gate — and + // the coverage check then passes vacuously, because an unrelated but + // legitimately-backfilled column is non-NULL on every row. Following that + // advice generated a live DROP COLUMN on the plaintext at exit 0 + // (#772 review, finding 7). + expect(p.log.error).not.toHaveBeenCalledWith( expect.stringContaining('--encrypted-column secret_blob'), ) + expect(p.log.error).toHaveBeenCalledWith( + expect.stringContaining('--encrypted-column '), + ) + expect(p.log.error).toHaveBeenCalledWith( + expect.stringContaining('do not record secret_blob'), + ) expect(migrateMocks.countUnencrypted).not.toHaveBeenCalled() expect(writeFileMock).not.toHaveBeenCalled() expect(exitSpy).toHaveBeenCalledWith(1) diff --git a/packages/cli/src/commands/encrypt/cutover.ts b/packages/cli/src/commands/encrypt/cutover.ts index 676c4e244..5b9906134 100644 --- a/packages/cli/src/commands/encrypt/cutover.ts +++ b/packages/cli/src/commands/encrypt/cutover.ts @@ -70,7 +70,7 @@ export async function cutoverCommand(options: CutoverCommandOptions) { // DOMAIN TYPES (manifest name as a hint; the `_encrypted` naming is // a convention, never relied upon) before any phase/config checks so v3 // users get the real answer, not a confusing precondition error. - const { info, candidates } = await resolveColumnLifecycle( + const { info, candidates, unresolvedHint } = await resolveColumnLifecycle( client, options.table, options.column, @@ -86,6 +86,7 @@ export async function cutoverCommand(options: CutoverCommandOptions) { options.table, options.column, candidates, + unresolvedHint, ) if (!info && unresolved) { p.log.error(unresolved) @@ -96,6 +97,21 @@ export async function cutoverCommand(options: CutoverCommandOptions) { if (info?.version === 3) { const encryptedColumn = info.column + + // `via: 'sole'` means only that this is the table's ONE EQL v3 column — + // nothing ties it to the plaintext column the user named. On a mixed + // table (a v2 pair the classifier no longer sees, plus one unrelated v3 + // column) that guess is simply wrong, and reporting "nothing to do for + // EQL v3" for it told a scripted rollout the cut-over had succeeded when + // the v2 rename never ran. `drop.ts` already refuses a `'sole'` match for + // the same reason (#772 review, finding 7). + if (info.via === 'sole') { + p.log.error( + `${options.table}.${encryptedColumn} (${info.domain}) is the table's only EQL v3 column, but nothing confirms it encrypts "${options.column}" — refusing to report a cut-over outcome on that guess. If "${options.column}" pairs with a legacy eql_v2_encrypted column, this release no longer manages that lifecycle. Otherwise record the pairing: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column \`.`, + ) + exitCode = 1 + return + } if (state?.phase === 'dropped') { // Terminal phase — the lifecycle already finished. Not an error and // not "finish the backfill": there is nothing left to backfill. diff --git a/packages/cli/src/commands/encrypt/drop.ts b/packages/cli/src/commands/encrypt/drop.ts index 9beb6953a..ae5181e4f 100644 --- a/packages/cli/src/commands/encrypt/drop.ts +++ b/packages/cli/src/commands/encrypt/drop.ts @@ -74,7 +74,7 @@ export async function dropCommand(options: DropCommandOptions) { // column, droppable straight after `backfilled`. The version and the // encrypted column's name are resolved from the DOMAIN TYPES (manifest // name as a hint) — the `_encrypted` naming is a convention only. - const { info, candidates } = await resolveColumnLifecycle( + const { info, candidates, unresolvedHint } = await resolveColumnLifecycle( client, options.table, options.column, @@ -91,6 +91,7 @@ export async function dropCommand(options: DropCommandOptions) { options.table, options.column, candidates, + unresolvedHint, ) if (!info && unresolved) { p.log.error(unresolved) @@ -103,9 +104,16 @@ export async function dropCommand(options: DropCommandOptions) { // that destroys the only copy of this data. Dropping is the single // irreversible step in the lifecycle, so it demands a positively // asserted pairing (manifest hint or the naming convention). + // + // The remedy must NOT name `info.column`. That is the guess itself, and + // recording it turns the next resolution into `via: 'hint'`, which walks + // straight past this gate — while the coverage check below passes + // vacuously, because a legitimately-backfilled unrelated column is + // non-NULL on every row. Following the old message verbatim generated a + // live `DROP COLUMN` on the plaintext at exit 0 (#772 review, finding 7). if (info?.via === 'sole') { p.log.error( - `${options.table}.${info.column} (${info.domain}) is the table's only encrypted column, but nothing confirms it encrypts "${options.column}" — refusing to generate an irreversible drop on that guess. Record the pairing and retry: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column ${info.column}\` (which writes it to the manifest), or set "encryptedColumn": "${info.column}" for this column in .cipherstash/migrations.json.`, + `${options.table}.${info.column} (${info.domain}) is the table's only encrypted column, but nothing confirms it encrypts "${options.column}" — refusing to generate an irreversible drop on that guess. Identify the column that actually encrypts "${options.column}" and record that pairing: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column \` (which writes it to the manifest), or set "encryptedColumn" for this column in .cipherstash/migrations.json. If "${options.column}" pairs with a legacy eql_v2_encrypted column, this release no longer manages that lifecycle — do not record ${info.column}.`, ) exitCode = 1 return diff --git a/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts b/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts index 1e5bc6cae..0310891c5 100644 --- a/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts +++ b/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts @@ -13,8 +13,40 @@ */ import type { EncryptedColumnInfo } from '@cipherstash/migrate' -import { describe, expect, it } from 'vitest' -import { explainUnresolved } from '../resolve-eql.js' +import type pg from 'pg' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +// Only the two I/O boundaries are replaced. `pickEncryptedColumn` stays real — +// it IS the resolution rule under test, and stubbing it (as `encrypt-v3.test.ts` +// stubs `resolveColumnLifecycle`) is why the hint-discard below had no coverage. +const readManifest = vi.hoisted(() => + vi.fn(async () => null as { tables: Record } | null), +) +const listEncryptedColumns = vi.hoisted(() => + vi.fn(async () => [] as EncryptedColumnInfo[]), +) +vi.mock('@cipherstash/migrate', async (importOriginal) => ({ + ...(await importOriginal()), + readManifest, + listEncryptedColumns, +})) + +const { explainUnresolved, resolveColumnLifecycle } = await import( + '../resolve-eql.js' +) + +/** + * A `pg` double answering only the `pg_attribute` existence probe, from a set + * of column names the table is pretended to have. That probe is the whole + * point: it is what separates "the hint is stale" from "the hint names a real + * column that simply is not EQL v3". + */ +const clientWithColumns = (...columns: string[]): pg.ClientBase => + ({ + query: async (_sql: string, params: unknown[]) => ({ + rows: [{ exists: columns.includes(String(params[1])) }], + }), + }) as unknown as pg.ClientBase const v3 = ( column: string, @@ -57,3 +89,116 @@ describe('explainUnresolved', () => { expect(message).toContain('Cannot identify which encrypted column') }) }) + +/** + * #772 review, finding 7 — the mixed-table dead end. + * + * `classifyEqlDomain` recognises `eql_v3_*` only, so on a table holding a v2 + * pair (`ssn` / `ssn_encrypted`) plus one unrelated v3 column, the v2 ciphertext + * column is not a candidate at all. `pickEncryptedColumn`'s sole-EQL-column rule + * then claims the unrelated v3 column for `ssn`. + * + * `encrypt backfill` records the true pairing in the manifest, so the answer is + * on disk — but the hint was thrown away whenever it failed to resolve, and the + * re-pick without it reached the sole rule. Recording the pairing changed + * nothing, and `drop`'s remedy told the user to record the guess instead. + */ +describe('resolveColumnLifecycle — a recorded hint that is not a v3 candidate', () => { + const V3_OTHER: EncryptedColumnInfo = { + column: 'email_enc', + domain: 'eql_v3_text_search', + version: 3, + } + + beforeEach(() => { + vi.clearAllMocks() + readManifest.mockResolvedValue(null) + listEncryptedColumns.mockResolvedValue([]) + }) + + it('does not fall back to the sole-column guess when the manifest names a counterpart', async () => { + // The v2 pair is invisible to the classifier; only email_enc is a candidate. + listEncryptedColumns.mockResolvedValue([V3_OTHER]) + readManifest.mockResolvedValue({ + tables: { users: [{ column: 'ssn', encryptedColumn: 'ssn_encrypted' }] }, + }) + + const { info, unresolvedHint } = await resolveColumnLifecycle( + clientWithColumns('ssn', 'ssn_encrypted', 'email_enc'), + 'users', + 'ssn', + ) + + expect(info).toBeNull() + expect(unresolvedHint).toBe('ssn_encrypted') + }) + + it('explains the recorded counterpart by name rather than listing candidates', async () => { + const message = explainUnresolved( + 'users', + 'ssn', + [V3_OTHER], + 'ssn_encrypted', + ) + + expect(message).toContain('ssn_encrypted') + expect(message).toContain('not an EQL v3 column') + // Naming the guess here is what sent users to `--encrypted-column email_enc`. + expect(message).not.toContain('--encrypted-column email_enc') + }) + + // The fallback the comment actually describes — a hint naming a column that + // is simply gone — must still resolve through convention. + it('still falls back to convention when the hint is genuinely stale', async () => { + listEncryptedColumns.mockResolvedValue([ + { column: 'ssn_encrypted', domain: 'eql_v3_text_eq', version: 3 }, + ]) + readManifest.mockResolvedValue({ + tables: { users: [{ column: 'ssn', encryptedColumn: 'ssn_old' }] }, + }) + + // `ssn_old` is gone from the table entirely — the genuinely stale case. + const { info } = await resolveColumnLifecycle( + clientWithColumns('ssn', 'ssn_encrypted'), + 'users', + 'ssn', + ) + + expect(info?.column).toBe('ssn_encrypted') + expect(info?.via).toBe('convention') + }) + + it('resolves through the hint when it does name a candidate', async () => { + listEncryptedColumns.mockResolvedValue([ + { column: 'ssn_enc', domain: 'eql_v3_text_eq', version: 3 }, + ]) + readManifest.mockResolvedValue({ + tables: { users: [{ column: 'ssn', encryptedColumn: 'ssn_enc' }] }, + }) + + const { info, unresolvedHint } = await resolveColumnLifecycle( + clientWithColumns('ssn', 'ssn_enc'), + 'users', + 'ssn', + ) + + expect(info?.column).toBe('ssn_enc') + expect(info?.via).toBe('hint') + expect(unresolvedHint).toBeUndefined() + }) + + // No manifest entry at all is the ordinary case; the sole rule still applies + // and `drop`'s own `via === 'sole'` gate is what refuses it. + it('leaves the sole-column rule alone when nothing was recorded', async () => { + listEncryptedColumns.mockResolvedValue([V3_OTHER]) + + const { info, unresolvedHint } = await resolveColumnLifecycle( + clientWithColumns('ssn', 'email_enc'), + 'users', + 'ssn', + ) + + expect(info?.via).toBe('sole') + expect(unresolvedHint).toBeUndefined() + }) +}) diff --git a/packages/cli/src/commands/encrypt/lib/resolve-eql.ts b/packages/cli/src/commands/encrypt/lib/resolve-eql.ts index b4e595a10..df8f765b2 100644 --- a/packages/cli/src/commands/encrypt/lib/resolve-eql.ts +++ b/packages/cli/src/commands/encrypt/lib/resolve-eql.ts @@ -23,6 +23,17 @@ export interface ResolvedLifecycle { * identifiable — callers name them instead of erroring blind. */ candidates: EncryptedColumnInfo[] + /** + * The manifest's recorded `encryptedColumn` when it named a column that is + * NOT among `candidates` — i.e. the pairing is on record but the column it + * names is not an EQL v3 column (typically legacy `eql_v2_encrypted`, which + * `classifyEqlDomain` no longer recognises). + * + * Distinct from a stale hint. It means the answer IS known and disagrees + * with anything resolution could otherwise guess, so callers must fail closed + * naming it rather than fall through to `via: 'sole'` (#772 review, finding 7). + */ + unresolvedHint?: string } /** @@ -59,11 +70,45 @@ export async function resolveColumnLifecycle( )?.encryptedColumn const candidates = await listEncryptedColumns(client, table) - let info = hint ? pickEncryptedColumn(candidates, column, hint) : null - // A stale hint (column since renamed/retyped) must not mask a resolvable - // counterpart — fall back to convention + sole-EQL-column resolution. - if (!info) info = pickEncryptedColumn(candidates, column) - return { info, candidates } + const hinted = hint ? pickEncryptedColumn(candidates, column, hint) : null + if (hinted) return { info: hinted, candidates } + + // The hint named a column that is not a candidate. Two very different + // reasons, and they must not share an outcome: + // + // - the column no longer exists (renamed, dropped) — a genuinely STALE hint, + // which must not mask a resolvable counterpart, so fall through; + // - the column exists but is not an EQL v3 column — the usual shape being a + // legacy `eql_v2_encrypted` counterpart, which `classifyEqlDomain` stopped + // recognising. Here the pairing IS known and falling through would discard + // it in favour of a guess: on a mixed table the sole-EQL-column rule then + // claims an unrelated v3 column, `cutover` reports success for a rename it + // never performed, and `drop`'s remedy tells the user to record the guess + // (#772 review, finding 7). + if (hint && (await columnExists(client, table, hint))) { + return { info: null, candidates, unresolvedHint: hint } + } + + return { info: pickEncryptedColumn(candidates, column), candidates } +} + +/** Whether `column` exists on `table` at all, whatever its type. */ +async function columnExists( + client: pg.ClientBase, + table: string, + column: string, +): Promise { + const { rows } = await client.query<{ exists: boolean }>( + `SELECT EXISTS ( + SELECT 1 FROM pg_attribute + WHERE attrelid = to_regclass($1) + AND attname = $2 + AND attnum > 0 + AND NOT attisdropped + ) AS exists`, + [table, column], + ) + return rows[0]?.exists === true } /** @@ -87,7 +132,16 @@ export function explainUnresolved( table: string, column: string, candidates: readonly EncryptedColumnInfo[], + unresolvedHint?: string, ): string | null { + // The recorded pairing points at a real column that is not an EQL v3 column + // — almost always a legacy `eql_v2_encrypted` counterpart. Say exactly that: + // listing the v3 candidates here would invite the user to record one of them, + // which is how the guess used to get laundered into a `via: 'hint'` match. + if (unresolvedHint !== undefined) { + return `${table}.${column} is recorded as pairing with "${unresolvedHint}", but ${unresolvedHint} is not an EQL v3 column — it is most likely a legacy eql_v2_encrypted column, which this command no longer manages. Finish the EQL v2 lifecycle for this column with a stash release that still supports it, or drop the recorded pairing from .cipherstash/migrations.json if it is wrong.` + } + if (candidates.length === 0) return null const listed = candidates .map((c) => ` - ${c.column} (${c.domain})`) From 342ec6233e46c81a48bbbe75c6a99c3358df80a6 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:34:07 +1000 Subject: [PATCH 2/6] fix(cli): make stash init's scaffolded client compile, and gate it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both placeholder templates emitted `await Encryption({ schemas: [] })`. An empty schema set is a hard TS2769 against both overloads — deliberately, per S-6 — so every `stash init` left a project failing its first tsc, in the one file the CLI tells the user not to hand-edit. The old scaffold called EncryptionV3, whose `readonly AnyV3Table[]` bound accepted `[]`; collapsing it into an alias of Encryption tightened that away. Relaxing the constraint is not an option (it exists to catch a real mistake), and `stash init` has no table names in scope by design — it stopped introspecting, and `build-schema.ts` sets `schemas: []` on its own state. So the scaffold declares a sentinel table instead, which keeps the file compiling and keeps the "you haven't declared anything yet" signal: loadEncryptConfig exits 1 when `__stash_placeholder__` is the only table left, naming the file. The gap that let this ship is the more important half. packages/cli has no typecheck step (21 pre-existing errors), utils-codegen*.test.ts only `toContain`-matches fragments, and build-schema.test.ts mocks generatePlaceholderClient to '// placeholder' — so nothing anywhere compiled, parsed or executed the generated output. Both templates are now committed as `.generated.ts` fixtures compiled by a scoped tsconfig in CI, and pinned byte-for-byte to the generator by a unit test. Verified the gate reproduces the original TS2769 when the fixture is reverted. The `.generated.ts` suffix is load-bearing: biome.json already excludes it, so formatting cannot rewrite template output and break the byte comparison. --- .changeset/init-scaffold-compiles.md | 24 ++++++ .github/workflows/tests.yml | 9 +++ .../scaffold/drizzle.generated.ts | 65 ++++++++++++++++ .../scaffold/generic.generated.ts | 60 +++++++++++++++ packages/cli/package.json | 5 +- .../placeholder-client-fixture.test.ts | 74 +++++++++++++++++++ packages/cli/src/commands/init/utils.ts | 36 ++++++--- packages/cli/src/config/index.ts | 22 ++++++ packages/cli/tsconfig.scaffold.json | 25 +++++++ skills/stash-cli/SKILL.md | 2 +- 10 files changed, 309 insertions(+), 13 deletions(-) create mode 100644 .changeset/init-scaffold-compiles.md create mode 100644 packages/cli/__fixtures__/scaffold/drizzle.generated.ts create mode 100644 packages/cli/__fixtures__/scaffold/generic.generated.ts create mode 100644 packages/cli/src/commands/init/__tests__/placeholder-client-fixture.test.ts create mode 100644 packages/cli/tsconfig.scaffold.json diff --git a/.changeset/init-scaffold-compiles.md b/.changeset/init-scaffold-compiles.md new file mode 100644 index 000000000..84587fa34 --- /dev/null +++ b/.changeset/init-scaffold-compiles.md @@ -0,0 +1,24 @@ +--- +'stash': patch +--- + +The client file `stash init` writes now compiles. + +Both placeholder templates emitted `await Encryption({ schemas: [] })`, and +`Encryption` requires at least one table — an empty schema set is a deliberate +compile error, so it cannot be relaxed. Every `stash init` therefore left a +project whose first `tsc` or `next build` failed, in a file the CLI had just +told the user not to hand-edit. (The previous scaffold called `EncryptionV3`, +whose looser bound accepted `[]`; collapsing that into an alias of `Encryption` +tightened it.) + +The scaffold now declares a single sentinel table, `__stash_placeholder__`, so +the file typechecks as written. `stash encrypt` commands refuse to run while +that table is still the only one declared, and say so — rather than failing +later with a confusing "table not found". + +Nothing in the repo compiled this output before: `packages/cli` has no +typecheck step, the codegen tests only string-match fragments of the template, +and the step test stubs the generator out entirely. Both templates are now +committed as fixtures that CI typechecks, pinned byte-for-byte to the generator +so they cannot drift. diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 369eecd61..7caeba077 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -193,6 +193,15 @@ jobs: - name: Typecheck (stack — emitted declarations, not source) run: pnpm exec turbo run test:types:dist --filter @cipherstash/stack + # `stash init` writes a client file into the user's project and tells them + # not to hand-edit it. Nothing compiled that file, so tightening + # `Encryption` to require a non-empty schema set left every `stash init` + # emitting a project that fails its first `tsc` — with CI green (#772 + # review). The fixtures are pinned byte-for-byte to the generator by + # `placeholder-client-fixture.test.ts`. + - name: Typecheck (stash init's scaffolded client) + run: pnpm --filter stash run typecheck:scaffold + - name: Lint — no hardcoded package-manager runners run: pnpm run lint:runners diff --git a/packages/cli/__fixtures__/scaffold/drizzle.generated.ts b/packages/cli/__fixtures__/scaffold/drizzle.generated.ts new file mode 100644 index 000000000..a0f9b77f0 --- /dev/null +++ b/packages/cli/__fixtures__/scaffold/drizzle.generated.ts @@ -0,0 +1,65 @@ +/** + * CipherStash encryption client — placeholder. + * + * `stash init` wrote this file. It is intentionally NOT a real Drizzle + * schema. Your existing schema files (typically under `src/db/`) remain + * authoritative — your agent will edit those directly when you encrypt a + * column, then update the `Encryption({ schemas: [...] })` call below + * to reference the encrypted tables you declared there. + * + * Until that happens, the encryption client is initialised with a single + * placeholder table so that this file compiles, and `stash encrypt` + * commands refuse to run and point back here. + * + * This project uses EQL v3. Encrypted columns are concrete Postgres domains + * built with the `types.*` factories from `@cipherstash/stack-drizzle`. + * Each domain's query capabilities are FIXED by the type you pick — there is + * no capability config object. Choose the factory whose capabilities you need: + * types.Text / types.Integer / … storage only (encrypt/decrypt, no queries) + * types.TextEq / types.IntegerEq equality (eq, inArray) + * types.IntegerOrd / types.DateOrd equality + order/range (gt/lt/between/sort) + * types.TextMatch free-text match only + * types.TextSearch equality + order/range + free-text + * types.Json encrypted-JSONB containment + selectors + * + * --- Pattern reference (copy into your real schema, do NOT use as-is) --- + * + * Encrypted twin column for an existing populated column (path 3 — lifecycle): + * + * import { pgTable, integer, text } from 'drizzle-orm/pg-core' + * import { types } from '@cipherstash/stack-drizzle' + * + * export const users = pgTable('users', { + * id: integer('id').primaryKey().generatedAlwaysAsIdentity(), + * email: text('email').notNull(), // existing plaintext, unchanged for now + * email_encrypted: types.TextSearch('email_encrypted'), // encrypted twin, NULLABLE — never .notNull() + * }) + * + * Net-new encrypted column (path 1 — declare encrypted from the start): + * + * export const orders = pgTable('orders', { + * id: integer('id').primaryKey().generatedAlwaysAsIdentity(), + * billing_address: types.TextEq('billing_address'), + * }) + * + * Once you have encrypted tables declared, harvest them and pass to Encryption(): + * + * import { extractEncryptionSchema } from '@cipherstash/stack-drizzle' + * import { Encryption } from '@cipherstash/stack/v3' + * import { users, orders } from './db/schema' + * + * export const encryptionClient = await Encryption({ + * schemas: [extractEncryptionSchema(users), extractEncryptionSchema(orders)], + * }) + */ +import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' + +// REPLACE THIS. It exists only so this file compiles before you have declared +// any encrypted tables — `Encryption` requires at least one. Swap it for your +// real tables (see the patterns above); `stash encrypt` refuses to run while +// the placeholder is still here. +export const placeholderTable = encryptedTable('__stash_placeholder__', { + replace_me: types.Text('replace_me'), +}) + +export const encryptionClient = await Encryption({ schemas: [placeholderTable] }) diff --git a/packages/cli/__fixtures__/scaffold/generic.generated.ts b/packages/cli/__fixtures__/scaffold/generic.generated.ts new file mode 100644 index 000000000..4f5831847 --- /dev/null +++ b/packages/cli/__fixtures__/scaffold/generic.generated.ts @@ -0,0 +1,60 @@ +/** + * CipherStash encryption client — placeholder. + * + * `stash init` wrote this file. It is intentionally NOT a real schema + * definition. Your existing schema files remain authoritative — your + * agent will declare encrypted columns there and update the + * `Encryption({ schemas: [...] })` call below to reference them. + * + * Until that happens, the encryption client is initialised with a single + * placeholder table so that this file compiles, and `stash encrypt` + * commands refuse to run and point back here. + * + * This project uses EQL v3. Encrypted columns are concrete Postgres domains + * built with the `types.*` factories from `@cipherstash/stack/eql/v3` + * (also re-exported from `@cipherstash/stack/v3`). Each domain's query + * capabilities are FIXED by the type you pick — there is no chainable + * capability tuner. Choose the factory whose capabilities you need: + * types.Text / types.Integer / … storage only (encrypt/decrypt, no queries) + * types.TextEq / types.IntegerEq equality + * types.IntegerOrd / types.DateOrd equality + order/range + * types.TextMatch free-text match only + * types.TextSearch equality + order/range + free-text + * types.Json encrypted-JSONB containment + selectors + * + * --- Pattern reference (copy into your real schema, do NOT use as-is) --- + * + * Encrypted twin column for an existing populated column (path 3 — lifecycle): + * + * import { encryptedTable, types } from '@cipherstash/stack/v3' + * + * export const users = encryptedTable('users', { + * email_encrypted: types.TextSearch('email_encrypted'), + * }) + * + * Net-new encrypted column (path 1 — declare encrypted from the start): + * + * export const orders = encryptedTable('orders', { + * billing_address: types.TextEq('billing_address'), + * }) + * + * Once you have encrypted tables declared, pass them to Encryption(): + * + * import { Encryption } from '@cipherstash/stack/v3' + * import { users, orders } from './db/schema' + * + * export const encryptionClient = await Encryption({ + * schemas: [users, orders], + * }) + */ +import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' + +// REPLACE THIS. It exists only so this file compiles before you have declared +// any encrypted tables — `Encryption` requires at least one. Swap it for your +// real tables (see the patterns above); `stash encrypt` refuses to run while +// the placeholder is still here. +export const placeholderTable = encryptedTable('__stash_placeholder__', { + replace_me: types.Text('replace_me'), +}) + +export const encryptionClient = await Encryption({ schemas: [placeholderTable] }) diff --git a/packages/cli/package.json b/packages/cli/package.json index 5b95ae381..ea8ea97ef 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,7 +1,7 @@ { "name": "stash", "version": "1.0.0-rc.4", - "description": "CipherStash CLI — the one stash command for auth, init, encryption schema, database setup, and secrets.", + "description": "CipherStash CLI \u2014 the one stash command for auth, init, encryption schema, database setup, and secrets.", "repository": { "type": "git", "url": "git+https://github.com/cipherstash/stack.git", @@ -41,6 +41,7 @@ "postbuild": "chmod +x ./dist/bin/stash.js", "dev": "tsup --watch", "test": "vitest run", + "typecheck:scaffold": "tsc -p tsconfig.scaffold.json", "test:e2e": "vitest run --config vitest.integration.config.ts", "lint": "biome check ." }, @@ -56,7 +57,7 @@ "posthog-node": "^5.40.0", "zod": "^3.25.76" }, - "//optionalDependencies": "@cipherstash/auth ships per-platform native bindings as optional peerDependencies. pnpm does not auto-install platform-matched optional peer deps, so we declare them here as optionalDependencies — pnpm then picks the binary matching the host's os/cpu (from each sub-package's own package.json) and ignores the rest. All seven names share a single catalog entry to keep them in lockstep.", + "//optionalDependencies": "@cipherstash/auth ships per-platform native bindings as optional peerDependencies. pnpm does not auto-install platform-matched optional peer deps, so we declare them here as optionalDependencies \u2014 pnpm then picks the binary matching the host's os/cpu (from each sub-package's own package.json) and ignores the rest. All seven names share a single catalog entry to keep them in lockstep.", "optionalDependencies": { "@cipherstash/auth-darwin-arm64": "catalog:repo", "@cipherstash/auth-darwin-x64": "catalog:repo", diff --git a/packages/cli/src/commands/init/__tests__/placeholder-client-fixture.test.ts b/packages/cli/src/commands/init/__tests__/placeholder-client-fixture.test.ts new file mode 100644 index 000000000..c3ddbf548 --- /dev/null +++ b/packages/cli/src/commands/init/__tests__/placeholder-client-fixture.test.ts @@ -0,0 +1,74 @@ +/** + * Binds `__fixtures__/scaffold/*.ts` to what `generatePlaceholderClient` + * actually emits. + * + * The fixtures are the only thing in the repo that COMPILES the file + * `stash init` writes into a user's project — `tsconfig.scaffold.json` and the + * CI step that runs it exist for that. But a typechecked fixture is worthless + * if the generator can drift away from it, and the existing codegen tests only + * `toContain`-match fragments while `build-schema.test.ts` stubs the generator + * out entirely. That combination is how `Encryption({ schemas: [] })` shipped: + * every test was green and no compiler ever saw the output (#772 review, + * finding 6). + * + * So: byte-for-byte, both directions. Change a template, regenerate the + * fixture; the compiler then has an opinion about it. + */ +import { readFileSync } from 'node:fs' +import path from 'node:path' +import { fileURLToPath } from 'node:url' +import { describe, expect, it } from 'vitest' +import { PLACEHOLDER_TABLE_NAME } from '@/config/index.js' +import { generatePlaceholderClient } from '../utils.js' + +const FIXTURE_DIR = path.resolve( + fileURLToPath(import.meta.url), + '../../../../../__fixtures__/scaffold', +) + +const fixture = (name: string) => + readFileSync(path.join(FIXTURE_DIR, name), 'utf-8') + +describe('the scaffolded client fixtures match the generator', () => { + // Named `.generated.ts` so biome.json's existing exclusion leaves them alone: + // they are template OUTPUT, and reformatting them would break the byte-for-byte + // comparison below (which is the only thing tying the compiler to the generator). + it.each([ + ['generic.generated.ts', 'postgresql'], + ['drizzle.generated.ts', 'drizzle'], + ] as const)('%s', (file, integration) => { + expect(fixture(file)).toBe(generatePlaceholderClient(integration)) + }) + + // `supabase` shares the generic template; pin that so a future split does not + // silently leave the supabase path ungated. + it('supabase reuses the generic template', () => { + expect(generatePlaceholderClient('supabase')).toBe( + fixture('generic.generated.ts'), + ) + }) +}) + +describe('the scaffold compiles because it declares a table', () => { + it.each([ + 'generic.generated.ts', + 'drizzle.generated.ts', + ])('%s passes a non-empty schema set', (file) => { + const body = fixture(file) + // The empty form is a hard TS2769 against both overloads — `Encryption` + // requires at least one table by design (S-6), so the scaffold cannot go + // back to `schemas: []` without breaking every project it is written into. + expect(body).not.toContain('Encryption({ schemas: [] })') + expect(body).toContain('schemas: [placeholderTable]') + }) + + it.each([ + 'generic.generated.ts', + 'drizzle.generated.ts', + ])('%s uses the sentinel name the config loader refuses', (file) => { + // `loadEncryptConfig` exits 1 when this is the only table left, so the + // two must agree — otherwise the user gets a confusing "table not found" + // from whichever command runs next instead of "you never replaced this". + expect(fixture(file)).toContain(`'${PLACEHOLDER_TABLE_NAME}'`) + }) +}) diff --git a/packages/cli/src/commands/init/utils.ts b/packages/cli/src/commands/init/utils.ts index fc0e6aec1..282a0320b 100644 --- a/packages/cli/src/commands/init/utils.ts +++ b/packages/cli/src/commands/init/utils.ts @@ -384,9 +384,9 @@ const DRIZZLE_PLACEHOLDER = `/** * column, then update the \`Encryption({ schemas: [...] })\` call below * to reference the encrypted tables you declared there. * - * Until that happens, the encryption client is initialised with no - * schemas, and \`stash encrypt\` commands will surface a clear error - * pointing at this file. + * Until that happens, the encryption client is initialised with a single + * placeholder table so that this file compiles, and \`stash encrypt\` + * commands refuse to run and point back here. * * This project uses EQL v3. Encrypted columns are concrete Postgres domains * built with the \`types.*\` factories from \`@cipherstash/stack-drizzle\`. @@ -429,9 +429,17 @@ const DRIZZLE_PLACEHOLDER = `/** * schemas: [extractEncryptionSchema(users), extractEncryptionSchema(orders)], * }) */ -import { Encryption } from '@cipherstash/stack/v3' +import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' + +// REPLACE THIS. It exists only so this file compiles before you have declared +// any encrypted tables — \`Encryption\` requires at least one. Swap it for your +// real tables (see the patterns above); \`stash encrypt\` refuses to run while +// the placeholder is still here. +export const placeholderTable = encryptedTable('__stash_placeholder__', { + replace_me: types.Text('replace_me'), +}) -export const encryptionClient = await Encryption({ schemas: [] }) +export const encryptionClient = await Encryption({ schemas: [placeholderTable] }) ` const GENERIC_PLACEHOLDER = `/** @@ -442,9 +450,9 @@ const GENERIC_PLACEHOLDER = `/** * agent will declare encrypted columns there and update the * \`Encryption({ schemas: [...] })\` call below to reference them. * - * Until that happens, the encryption client is initialised with no - * schemas, and \`stash encrypt\` commands will surface a clear error - * pointing at this file. + * Until that happens, the encryption client is initialised with a single + * placeholder table so that this file compiles, and \`stash encrypt\` + * commands refuse to run and point back here. * * This project uses EQL v3. Encrypted columns are concrete Postgres domains * built with the \`types.*\` factories from \`@cipherstash/stack/eql/v3\` @@ -483,7 +491,15 @@ const GENERIC_PLACEHOLDER = `/** * schemas: [users, orders], * }) */ -import { Encryption } from '@cipherstash/stack/v3' +import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' + +// REPLACE THIS. It exists only so this file compiles before you have declared +// any encrypted tables — \`Encryption\` requires at least one. Swap it for your +// real tables (see the patterns above); \`stash encrypt\` refuses to run while +// the placeholder is still here. +export const placeholderTable = encryptedTable('__stash_placeholder__', { + replace_me: types.Text('replace_me'), +}) -export const encryptionClient = await Encryption({ schemas: [] }) +export const encryptionClient = await Encryption({ schemas: [placeholderTable] }) ` diff --git a/packages/cli/src/config/index.ts b/packages/cli/src/config/index.ts index 7103d7baa..c304f7d58 100644 --- a/packages/cli/src/config/index.ts +++ b/packages/cli/src/config/index.ts @@ -238,5 +238,27 @@ export async function loadEncryptConfig( ) process.exit(1) } + + // `stash init` scaffolds a client holding one placeholder table, because + // `Encryption` requires a non-empty schema set and the scaffold has no real + // tables to name yet. Reaching here with only that table means the user never + // replaced it — which used to surface as a confusing "table not found" from + // whichever command ran next. + const tables = Object.keys(config.tables ?? {}) + if (tables.length === 1 && tables[0] === PLACEHOLDER_TABLE_NAME) { + console.error( + `Error: ${encryptClientPath} still contains the placeholder table \`${PLACEHOLDER_TABLE_NAME}\` that \`stash init\` wrote.\n\nDeclare your encrypted columns and pass those tables to Encryption({ schemas: [...] }) in that file, then re-run this command.`, + ) + process.exit(1) + } + return config } + +/** + * The table name `stash init`'s scaffold uses so the file it writes compiles. + * + * Kept in sync with the templates in `commands/init/utils.ts` by + * `__tests__/placeholder-client-fixture.test.ts`, which also typechecks them. + */ +export const PLACEHOLDER_TABLE_NAME = '__stash_placeholder__' diff --git a/packages/cli/tsconfig.scaffold.json b/packages/cli/tsconfig.scaffold.json new file mode 100644 index 000000000..1a11f6286 --- /dev/null +++ b/packages/cli/tsconfig.scaffold.json @@ -0,0 +1,25 @@ +{ + // Compiles the exact files `stash init` writes into a user's project. + // + // Nothing else in the repo did. `packages/cli` has no typecheck script (21 + // pre-existing errors), the codegen tests only string-match the templates, + // and `build-schema.test.ts` stubs the generator out entirely — so when + // `Encryption` was tightened to require a non-empty schema set, both + // placeholders started emitting a file that fails `tsc` on first build, in a + // file the CLI had just told the user not to hand-edit, and CI stayed green + // (#772 review, finding 6). + // + // Deliberately scoped to the fixtures so it gates the scaffold without + // waiting on the rest of the package to compile. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + "module": "ESNext", + "moduleResolution": "bundler", + "target": "ESNext", + "lib": ["ESNext"], + "strict": true, + "skipLibCheck": true + }, + "include": ["__fixtures__/scaffold/*.ts"] +} diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index aa1c03c27..49873523e 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -221,7 +221,7 @@ Seven mechanical steps, no agent handoff. It prompts only when it can't pick a s 1. **Authenticate** — silent when a valid token exists. 2. **Resolve database** — per the resolution order above; verifies the connection. 3. **Resolve proxy choice** — CipherStash Proxy or direct SDK access (the default). Stored as `usesProxy` in `context.json`. Set by `--proxy` / `--no-proxy`; non-TTY without a flag defaults to SDK. -4. **Build schema** — auto-detects Drizzle (`drizzle.config.*`, `drizzle-orm`/`drizzle-kit`), Supabase (from the `DATABASE_URL` host), and Prisma Next. Writes a placeholder encryption client; prompts only if a file already exists there. +4. **Build schema** — auto-detects Drizzle (`drizzle.config.*`, `drizzle-orm`/`drizzle-kit`), Supabase (from the `DATABASE_URL` host), and Prisma Next. Writes a placeholder encryption client; prompts only if a file already exists there. The placeholder declares one sentinel table, `__stash_placeholder__`, because `Encryption` requires at least one — replace it with the tables you actually encrypt. Until you do, `stash encrypt` commands exit 1 and point back at that file. 5. **Install dependencies** — one combined prompt for `@cipherstash/stack` and `stash`. Skipped when both are present. 6. **Install EQL** — always EQL v3. **Drizzle** projects generate a v3 install migration (the same output as `eql migration --drizzle`, including the `cs_migrations` tracking schema) so the install lands in your migration history — apply it with `drizzle-kit migrate`; requires `drizzle-kit` to be installed and configured. **Prisma Next** is skipped (it installs EQL via `prisma-next migrate`). Everything else runs `eql install` directly against the resolved database, and is skipped when EQL is already installed. 7. **Gather context** — detects available coding agents and writes `.cipherstash/context.json`. From a55788b4a283855db091bb88841f108991d0f8fb Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Mon, 27 Jul 2026 09:51:31 +1000 Subject: [PATCH 3/6] fix(cli): keep pure-v2 tables on the v2 ladder (#787 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `unresolvedHint` fail-closed added for #772 finding 7 had no `candidates.length > 0` guard, so it fired on pure EQL v2 tables too. `encrypt backfill` records `encryptedColumn` in migrations.json unconditionally, v2 included. On a pure-v2 table `listEncryptedColumns` returns [] (the classifier recognises `eql_v3_*` only), so the hint failed to resolve, `columnExists` found the real `eql_v2_encrypted` column, and `cutover` / `drop` exited 1 — telling users to downgrade for a lifecycle this same build still fully implements in cutover.ts / drop.ts. Gate the fail-closed on a non-empty candidate list. Finding 7's protection is unchanged: the mixed table it targets always has candidates. Order `explainUnresolved`'s empty-candidates fall-through ahead of the hint branch so both agree for direct callers. Also drop the "this release no longer manages that lifecycle" claim from the cutover/drop `via: 'sole'` messages — the build does still implement it; the command simply resolves EQL v3 counterparts only. Tests cover the previously untested shape: candidates [] + a recorded hint. Review follow-ups: - Extend the placeholder-table guard to `encrypt backfill` via loadEncryptionContext; correct the changeset/skill wording to name the commands that actually refuse (cutover/drop never read the client file). - Alias `@cipherstash/migrate` to source in the CLI unit vitest config, so `pnpm --filter stash test` no longer needs a prior workspace build (verified: 888 tests pass with packages/migrate/dist removed). - Revert the unrelated em-dash re-encoding in packages/cli/package.json. --- .changeset/encrypt-lifecycle-mixed-table.md | 9 +- .changeset/init-scaffold-compiles.md | 9 +- packages/cli/package.json | 4 +- .../__tests__/context-placeholder.test.ts | 89 +++++++++++++++++++ packages/cli/src/commands/encrypt/context.ts | 20 ++++- packages/cli/src/commands/encrypt/cutover.ts | 2 +- packages/cli/src/commands/encrypt/drop.ts | 2 +- .../encrypt/lib/__tests__/resolve-eql.test.ts | 56 ++++++++++-- .../src/commands/encrypt/lib/resolve-eql.ts | 77 +++++++++++----- packages/cli/vitest.config.ts | 7 ++ skills/stash-cli/SKILL.md | 2 +- 11 files changed, 237 insertions(+), 40 deletions(-) create mode 100644 packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts diff --git a/.changeset/encrypt-lifecycle-mixed-table.md b/.changeset/encrypt-lifecycle-mixed-table.md index 78d41244e..95bebb711 100644 --- a/.changeset/encrypt-lifecycle-mixed-table.md +++ b/.changeset/encrypt-lifecycle-mixed-table.md @@ -35,5 +35,10 @@ all now fixed: column that actually encrypts the named one, and says explicitly not to record the guess. -Pure-v2 and pure-v3 tables are unaffected, as are tables with two or more EQL v3 -columns (resolution already failed closed there). +The new refusal is scoped to the mixed table it was written for: it applies only +when the table actually holds EQL v3 columns that a guess could wrongly claim. A +pure-v2 table has none, so it still falls through to the EQL v2 lifecycle exactly +as before — including when `encrypt backfill` recorded an `encryptedColumn` for +it, which it does for v2 columns too. Pure-v2 and pure-v3 tables are therefore +unaffected, as are tables with two or more EQL v3 columns (resolution already +failed closed there). diff --git a/.changeset/init-scaffold-compiles.md b/.changeset/init-scaffold-compiles.md index 84587fa34..f20ae5d4d 100644 --- a/.changeset/init-scaffold-compiles.md +++ b/.changeset/init-scaffold-compiles.md @@ -13,9 +13,12 @@ whose looser bound accepted `[]`; collapsing that into an alias of `Encryption` tightened it.) The scaffold now declares a single sentinel table, `__stash_placeholder__`, so -the file typechecks as written. `stash encrypt` commands refuse to run while -that table is still the only one declared, and say so — rather than failing -later with a confusing "table not found". +the file typechecks as written. Every command that reads the encryption client +— `stash db push`, `stash db validate`, and `stash encrypt backfill` — refuses +to run while that table is still the only one declared, and names it, rather +than failing later with a confusing "table not found". (`stash encrypt cutover` +and `stash encrypt drop` do not read the client file at all; they resolve +against the database.) Nothing in the repo compiled this output before: `packages/cli` has no typecheck step, the codegen tests only string-match fragments of the template, diff --git a/packages/cli/package.json b/packages/cli/package.json index ea8ea97ef..afbf3e98b 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -1,7 +1,7 @@ { "name": "stash", "version": "1.0.0-rc.4", - "description": "CipherStash CLI \u2014 the one stash command for auth, init, encryption schema, database setup, and secrets.", + "description": "CipherStash CLI — the one stash command for auth, init, encryption schema, database setup, and secrets.", "repository": { "type": "git", "url": "git+https://github.com/cipherstash/stack.git", @@ -57,7 +57,7 @@ "posthog-node": "^5.40.0", "zod": "^3.25.76" }, - "//optionalDependencies": "@cipherstash/auth ships per-platform native bindings as optional peerDependencies. pnpm does not auto-install platform-matched optional peer deps, so we declare them here as optionalDependencies \u2014 pnpm then picks the binary matching the host's os/cpu (from each sub-package's own package.json) and ignores the rest. All seven names share a single catalog entry to keep them in lockstep.", + "//optionalDependencies": "@cipherstash/auth ships per-platform native bindings as optional peerDependencies. pnpm does not auto-install platform-matched optional peer deps, so we declare them here as optionalDependencies — pnpm then picks the binary matching the host's os/cpu (from each sub-package's own package.json) and ignores the rest. All seven names share a single catalog entry to keep them in lockstep.", "optionalDependencies": { "@cipherstash/auth-darwin-arm64": "catalog:repo", "@cipherstash/auth-darwin-x64": "catalog:repo", diff --git a/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts b/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts new file mode 100644 index 000000000..ad32cded8 --- /dev/null +++ b/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts @@ -0,0 +1,89 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { PLACEHOLDER_TABLE_NAME } from '@/config/index.js' + +/** + * `stash encrypt` does not load its client through `loadEncryptConfig`, so the + * placeholder guard that command group needs lives in `loadEncryptionContext`. + * Both must refuse the un-replaced scaffold; otherwise `requireTable` reports + * `Table "users" was not found … Available: __stash_placeholder__`, which names + * the symptom instead of the cause (#787 review). + * + * Runs against the real jiti runtime — the guard reads the tables harvested + * from an actually-evaluated client module, which is the part worth pinning. + */ +describe('loadEncryptionContext — the un-replaced init scaffold', () => { + let tmpDir: string + let originalCwd: () => string + + const writeProject = (clientBody: string) => { + fs.writeFileSync( + path.join(tmpDir, 'stash.config.ts'), + `export default { + databaseUrl: 'postgresql://u:p@127.0.0.1:5432/db', + client: './client.ts', + }`, + ) + fs.writeFileSync(path.join(tmpDir, 'client.ts'), clientBody) + process.cwd = () => tmpDir + } + + /** + * The duck-typed shapes `loadEncryptionContext` harvests: any export with a + * `getEncryptConfig()` method is the client, and any with `tableName` + + * `build()` is a table. Hand-rolled rather than imported from + * `@cipherstash/stack` so the test needs no native module. + */ + const table = (name: string) => + `{ tableName: '${name}', build: () => ({ tableName: '${name}', columns: {} }) }` + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'stash-encrypt-ctx-')) + originalCwd = process.cwd + }) + + afterEach(() => { + process.cwd = originalCwd + vi.restoreAllMocks() + if (tmpDir && fs.existsSync(tmpDir)) { + fs.rmSync(tmpDir, { recursive: true, force: true }) + } + }) + + it('exits 1 naming the sentinel when it is the only table declared', async () => { + writeProject( + `export const encryptionClient = { getEncryptConfig: () => ({}) } + export const placeholderTable = ${table(PLACEHOLDER_TABLE_NAME)}`, + ) + const error = vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit') + }) + + const { loadEncryptionContext } = await import('../context.js') + await expect(loadEncryptionContext()).rejects.toThrow('process.exit') + + const message = error.mock.calls.flat().join('\n') + expect(message).toContain(PLACEHOLDER_TABLE_NAME) + // Names the cause, not `requireTable`'s "table not found" symptom. + expect(message).toContain('still contains the placeholder table') + expect(message).not.toContain('was not found in the encryption client') + }) + + it('allows the sentinel through once a real table sits alongside it', async () => { + // Only the SOLE-placeholder case is the un-replaced scaffold. A user who + // has added real tables must not be blocked by a leftover sentinel export. + writeProject( + `export const encryptionClient = { getEncryptConfig: () => ({}) } + export const placeholderTable = ${table(PLACEHOLDER_TABLE_NAME)} + export const users = ${table('users')}`, + ) + + const { loadEncryptionContext } = await import('../context.js') + const ctx = await loadEncryptionContext() + + expect(ctx.tables.has('users')).toBe(true) + }) +}) diff --git a/packages/cli/src/commands/encrypt/context.ts b/packages/cli/src/commands/encrypt/context.ts index 1975e2d96..e37dd8a31 100644 --- a/packages/cli/src/commands/encrypt/context.ts +++ b/packages/cli/src/commands/encrypt/context.ts @@ -1,7 +1,11 @@ import fs from 'node:fs' import path from 'node:path' import type { EncryptionClient } from '@cipherstash/stack/encryption' -import { loadStashConfig, type ResolvedStashConfig } from '@/config/index.js' +import { + loadStashConfig, + PLACEHOLDER_TABLE_NAME, + type ResolvedStashConfig, +} from '@/config/index.js' /** * Structural shape of `@cipherstash/stack`'s `EncryptedTable` class. @@ -138,6 +142,20 @@ export async function loadEncryptionContext(): Promise { process.exit(1) } + // Same guard `loadEncryptConfig` applies for `stash db push` / `db validate`, + // repeated here because `stash encrypt` does not go through that loader. The + // scaffold `stash init` writes declares one sentinel table so the file + // compiles; reaching here with only that table means it was never replaced. + // Without this, `requireTable` reported `Table "users" was not found … + // Available: __stash_placeholder__`, which names the symptom and not the + // cause (#787 review). + if (tables.size === 1 && tables.has(PLACEHOLDER_TABLE_NAME)) { + console.error( + `Error: ${stashConfig.client} still contains the placeholder table \`${PLACEHOLDER_TABLE_NAME}\` that \`stash init\` wrote.\n\nDeclare your encrypted columns and pass those tables to Encryption({ schemas: [...] }) in that file, then re-run this command.`, + ) + process.exit(1) + } + return { stashConfig, client, tables } } diff --git a/packages/cli/src/commands/encrypt/cutover.ts b/packages/cli/src/commands/encrypt/cutover.ts index 5b9906134..7a5cd7f1c 100644 --- a/packages/cli/src/commands/encrypt/cutover.ts +++ b/packages/cli/src/commands/encrypt/cutover.ts @@ -107,7 +107,7 @@ export async function cutoverCommand(options: CutoverCommandOptions) { // the same reason (#772 review, finding 7). if (info.via === 'sole') { p.log.error( - `${options.table}.${encryptedColumn} (${info.domain}) is the table's only EQL v3 column, but nothing confirms it encrypts "${options.column}" — refusing to report a cut-over outcome on that guess. If "${options.column}" pairs with a legacy eql_v2_encrypted column, this release no longer manages that lifecycle. Otherwise record the pairing: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column \`.`, + `${options.table}.${encryptedColumn} (${info.domain}) is the table's only EQL v3 column, but nothing confirms it encrypts "${options.column}" — refusing to report a cut-over outcome on that guess. If "${options.column}" pairs with a legacy eql_v2_encrypted column, resolution cannot see it (this command resolves EQL v3 counterparts only) — drive that column's v2 lifecycle against its own encrypted column directly. Otherwise record the pairing: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column \`.`, ) exitCode = 1 return diff --git a/packages/cli/src/commands/encrypt/drop.ts b/packages/cli/src/commands/encrypt/drop.ts index ae5181e4f..49b0bff8c 100644 --- a/packages/cli/src/commands/encrypt/drop.ts +++ b/packages/cli/src/commands/encrypt/drop.ts @@ -113,7 +113,7 @@ export async function dropCommand(options: DropCommandOptions) { // live `DROP COLUMN` on the plaintext at exit 0 (#772 review, finding 7). if (info?.via === 'sole') { p.log.error( - `${options.table}.${info.column} (${info.domain}) is the table's only encrypted column, but nothing confirms it encrypts "${options.column}" — refusing to generate an irreversible drop on that guess. Identify the column that actually encrypts "${options.column}" and record that pairing: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column \` (which writes it to the manifest), or set "encryptedColumn" for this column in .cipherstash/migrations.json. If "${options.column}" pairs with a legacy eql_v2_encrypted column, this release no longer manages that lifecycle — do not record ${info.column}.`, + `${options.table}.${info.column} (${info.domain}) is the table's only encrypted column, but nothing confirms it encrypts "${options.column}" — refusing to generate an irreversible drop on that guess. Identify the column that actually encrypts "${options.column}" and record that pairing: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column \` (which writes it to the manifest), or set "encryptedColumn" for this column in .cipherstash/migrations.json. If "${options.column}" pairs with a legacy eql_v2_encrypted column, resolution cannot see it (this command resolves EQL v3 counterparts only) — drive that column's v2 lifecycle against its own encrypted column directly, and do not record ${info.column}.`, ) exitCode = 1 return diff --git a/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts b/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts index 0310891c5..9c45d78b7 100644 --- a/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts +++ b/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts @@ -4,12 +4,17 @@ * * `listEncryptedColumns` can no longer emit `version: 2` — a legacy * `eql_v2_encrypted` column is not classified as an EQL column at all, so it - * never reaches this function as a candidate. The post-cutover v2 state (the - * ciphertext renamed onto the plaintext column's own name) therefore arrives - * here as an EMPTY candidate list, which the first guard already falls through - * on. These tests exist so removing the now-unreachable `version === 2` branch - * is provably behaviour-preserving, and so a future v2 sweep cannot delete the - * empty-list guard the v2 lifecycle actually depends on. + * never reaches this function as a candidate. Every pure-v2 table therefore + * arrives here as an EMPTY candidate list, both pre-cutover (`` / + * `_encrypted`) and post-cutover (the ciphertext renamed onto the + * plaintext column's own name), and the first guard falls through on it. + * + * These tests exist so removing the now-unreachable `version === 2` branch is + * provably behaviour-preserving, and so a future v2 sweep cannot delete the + * empty-list guard the v2 lifecycle actually depends on. That guard has to hold + * even when the manifest recorded an `encryptedColumn` — `backfill` records one + * for v2 columns too — which is the `candidates.length > 0` gate on the + * fail-closed path (#787 review). */ import type { EncryptedColumnInfo } from '@cipherstash/migrate' @@ -64,6 +69,15 @@ describe('explainUnresolved', () => { expect(explainUnresolved('users', 'email', [])).toBeNull() }) + it('still falls through when no EQL v3 columns exist BUT a hint was recorded', () => { + // The pure-v2 table. `encrypt backfill` records `encryptedColumn` for v2 + // columns too, so a hint is present on every table backfilled with this + // release — it must not flip the empty-candidate fall-through into a + // refusal, because `cutover` / `drop` in this same build still implement + // the v2 ladder this falls through to (#787 review). + expect(explainUnresolved('users', 'ssn', [], 'ssn_encrypted')).toBeNull() + }) + it('fails closed, naming every candidate, when none is identifiable', () => { const message = explainUnresolved('users', 'email', [ v3('a_enc'), @@ -133,6 +147,36 @@ describe('resolveColumnLifecycle — a recorded hint that is not a v3 candidate' expect(unresolvedHint).toBe('ssn_encrypted') }) + // The pure-v2 shape, and the regression the `candidates.length > 0` gate + // exists to prevent (#787 review). `backfill` records `encryptedColumn` + // unconditionally — v2 included — so EVERY pure-v2 table backfilled with this + // release carries a hint naming a real, existing, non-v3 column. Without the + // gate, `columnExists` returned true, `unresolvedHint` was set, and + // `cutover` / `drop` refused a lifecycle this same build still performs. + it('does not fail closed on a pure-v2 table, where no v3 column can be mis-claimed', async () => { + // No v3 columns at all: just the `ssn` / `ssn_encrypted` v2 pair, which the + // classifier does not see. + listEncryptedColumns.mockResolvedValue([]) + readManifest.mockResolvedValue({ + tables: { users: [{ column: 'ssn', encryptedColumn: 'ssn_encrypted' }] }, + }) + + const { info, candidates, unresolvedHint } = await resolveColumnLifecycle( + clientWithColumns('ssn', 'ssn_encrypted'), + 'users', + 'ssn', + ) + + expect(info).toBeNull() + expect(candidates).toEqual([]) + // The fall-through signal: no hint reported, so `explainUnresolved` returns + // null and the caller reaches its own v2 preconditions. + expect(unresolvedHint).toBeUndefined() + expect( + explainUnresolved('users', 'ssn', candidates, unresolvedHint), + ).toBeNull() + }) + it('explains the recorded counterpart by name rather than listing candidates', async () => { const message = explainUnresolved( 'users', diff --git a/packages/cli/src/commands/encrypt/lib/resolve-eql.ts b/packages/cli/src/commands/encrypt/lib/resolve-eql.ts index df8f765b2..6c4834d0b 100644 --- a/packages/cli/src/commands/encrypt/lib/resolve-eql.ts +++ b/packages/cli/src/commands/encrypt/lib/resolve-eql.ts @@ -24,14 +24,23 @@ export interface ResolvedLifecycle { */ candidates: EncryptedColumnInfo[] /** - * The manifest's recorded `encryptedColumn` when it named a column that is - * NOT among `candidates` — i.e. the pairing is on record but the column it - * names is not an EQL v3 column (typically legacy `eql_v2_encrypted`, which - * `classifyEqlDomain` no longer recognises). + * The manifest's recorded `encryptedColumn` when it named a real column that + * is NOT among `candidates`, AND `candidates` is non-empty — i.e. the pairing + * is on record, the column it names is not an EQL v3 column (typically legacy + * `eql_v2_encrypted`, which `classifyEqlDomain` no longer recognises), and + * there are v3 columns here that a guess could wrongly claim. * * Distinct from a stale hint. It means the answer IS known and disagrees * with anything resolution could otherwise guess, so callers must fail closed * naming it rather than fall through to `via: 'sole'` (#772 review, finding 7). + * + * Deliberately NOT set when `candidates` is empty. That is the pure-v2 table + * — a `` / `_encrypted` pair and nothing else — where there is no + * v3 column to mis-claim and so nothing to protect against. The v2 lifecycle + * is still implemented in `cutover.ts` / `drop.ts`, and those commands must + * keep reaching it; failing closed here would refuse a lifecycle this same + * build still performs, and tell the user to downgrade to reach it (#787 + * review). */ unresolvedHint?: string } @@ -73,19 +82,29 @@ export async function resolveColumnLifecycle( const hinted = hint ? pickEncryptedColumn(candidates, column, hint) : null if (hinted) return { info: hinted, candidates } - // The hint named a column that is not a candidate. Two very different + // The hint named a column that is not a candidate. Three very different // reasons, and they must not share an outcome: // // - the column no longer exists (renamed, dropped) — a genuinely STALE hint, // which must not mask a resolvable counterpart, so fall through; - // - the column exists but is not an EQL v3 column — the usual shape being a - // legacy `eql_v2_encrypted` counterpart, which `classifyEqlDomain` stopped - // recognising. Here the pairing IS known and falling through would discard - // it in favour of a guess: on a mixed table the sole-EQL-column rule then - // claims an unrelated v3 column, `cutover` reports success for a rename it - // never performed, and `drop`'s remedy tells the user to record the guess - // (#772 review, finding 7). - if (hint && (await columnExists(client, table, hint))) { + // - the column exists, is not an EQL v3 column, and the table HAS v3 columns + // — the mixed table. The usual shape is a legacy `eql_v2_encrypted` + // counterpart, which `classifyEqlDomain` stopped recognising. Here the + // pairing IS known and falling through would discard it in favour of a + // guess: the sole-EQL-column rule claims an unrelated v3 column, `cutover` + // reports success for a rename it never performed, and `drop`'s remedy + // tells the user to record the guess (#772 review, finding 7). Fail closed; + // - the column exists, is not an EQL v3 column, and there are NO v3 columns + // on the table — the pure-v2 table. Nothing here can be mis-claimed, and + // `cutover` / `drop` still implement the v2 ladder, so fall through to it + // exactly as before. Gating on `candidates.length` is what keeps the + // protection above scoped to the mixed table it was written for (#787 + // review). + if ( + hint && + candidates.length > 0 && + (await columnExists(client, table, hint)) + ) { return { info: null, candidates, unresolvedHint: hint } } @@ -115,12 +134,16 @@ async function columnExists( * Explain a failed resolution (`info === null`) to the user, or return * `null` when the failure is fine to fall through to the v2 lifecycle. * - * The one fall-through case is "no EQL columns at all", which the v2 + * The one fall-through case is "no EQL v3 columns at all", which the v2 * phase/config preconditions turn into an accurate error ("not backfilled", * "no pending config", …). Since `classifyEqlDomain` recognises `eql_v3_*` - * only, that case now also covers the post-cutover v2 state — `` was - * renamed onto the ciphertext, and its `eql_v2_encrypted` domain is no longer - * classified, so the column never appears as a candidate. + * only, that case covers every pure-v2 table — both the pre-cutover pair + * (`` / `_encrypted`) and the post-cutover state where `` was + * renamed onto the ciphertext. Neither column is ever a candidate, so a pure-v2 + * table reaches the v2 ladder here regardless of what the manifest recorded; + * a recorded `encryptedColumn` must NOT turn that into a refusal, because + * `cutover.ts` / `drop.ts` in this same build still implement that ladder + * (#787 review). * * A non-empty candidate list therefore means EQL v3 columns exist but none is * identifiable — the caller must fail closed with this message rather than @@ -134,15 +157,23 @@ export function explainUnresolved( candidates: readonly EncryptedColumnInfo[], unresolvedHint?: string, ): string | null { - // The recorded pairing points at a real column that is not an EQL v3 column - // — almost always a legacy `eql_v2_encrypted` counterpart. Say exactly that: - // listing the v3 candidates here would invite the user to record one of them, - // which is how the guess used to get laundered into a `via: 'hint'` match. + // "No EQL v3 columns at all" always falls through, even with a recorded hint. + // That is the pure-v2 table, and the v2 ladder in `cutover.ts` / `drop.ts` + // still handles it — the caller's own preconditions produce the accurate + // error. Ordered ahead of the hint branch deliberately: `resolveColumnLifecycle` + // already declines to set `unresolvedHint` on an empty candidate list, and this + // keeps the two agreeing for direct callers of this function (#787 review). + if (candidates.length === 0) return null + + // The recorded pairing points at a real column that is not an EQL v3 column, + // on a table that does have v3 columns — almost always a legacy + // `eql_v2_encrypted` counterpart alongside an unrelated v3 one. Say exactly + // that: listing the v3 candidates here would invite the user to record one of + // them, which is how the guess used to get laundered into a `via: 'hint'` match. if (unresolvedHint !== undefined) { - return `${table}.${column} is recorded as pairing with "${unresolvedHint}", but ${unresolvedHint} is not an EQL v3 column — it is most likely a legacy eql_v2_encrypted column, which this command no longer manages. Finish the EQL v2 lifecycle for this column with a stash release that still supports it, or drop the recorded pairing from .cipherstash/migrations.json if it is wrong.` + return `${table}.${column} is recorded as pairing with "${unresolvedHint}", but ${unresolvedHint} is not an EQL v3 column — it is most likely a legacy eql_v2_encrypted column. ${table} also holds EQL v3 columns, and none of them is a confirmed counterpart for ${column}, so this command cannot tell which lifecycle applies and will not guess.\n\nIf that pairing is wrong, correct or remove "encryptedColumn" for ${column} in .cipherstash/migrations.json and re-run. If it is right, ${column} is on the EQL v2 lifecycle: drive it against ${unresolvedHint} directly rather than through this command, which resolves EQL v3 counterparts only.` } - if (candidates.length === 0) return null const listed = candidates .map((c) => ` - ${c.column} (${c.domain})`) .join('\n') diff --git a/packages/cli/vitest.config.ts b/packages/cli/vitest.config.ts index 53df05899..195a6a156 100644 --- a/packages/cli/vitest.config.ts +++ b/packages/cli/vitest.config.ts @@ -9,6 +9,13 @@ export default defineConfig({ resolve: { alias: { '@/': `${resolve(__dirname, './src')}/`, + // `@cipherstash/migrate` publishes `./dist` only, so importing it — or + // spreading `importOriginal()` inside a partial `vi.mock` — makes the + // UNIT suite require a prior workspace build. CI only hides that because + // turbo runs `^build` first; on a clean checkout `pnpm --filter stash + // test` would fail to resolve it. Resolving to source keeps the unit + // config self-contained, as `AGENTS.md` says it is (#787 review). + '@cipherstash/migrate': resolve(__dirname, '../migrate/src/index.ts'), }, }, }) diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 49873523e..e2541e40f 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -221,7 +221,7 @@ Seven mechanical steps, no agent handoff. It prompts only when it can't pick a s 1. **Authenticate** — silent when a valid token exists. 2. **Resolve database** — per the resolution order above; verifies the connection. 3. **Resolve proxy choice** — CipherStash Proxy or direct SDK access (the default). Stored as `usesProxy` in `context.json`. Set by `--proxy` / `--no-proxy`; non-TTY without a flag defaults to SDK. -4. **Build schema** — auto-detects Drizzle (`drizzle.config.*`, `drizzle-orm`/`drizzle-kit`), Supabase (from the `DATABASE_URL` host), and Prisma Next. Writes a placeholder encryption client; prompts only if a file already exists there. The placeholder declares one sentinel table, `__stash_placeholder__`, because `Encryption` requires at least one — replace it with the tables you actually encrypt. Until you do, `stash encrypt` commands exit 1 and point back at that file. +4. **Build schema** — auto-detects Drizzle (`drizzle.config.*`, `drizzle-orm`/`drizzle-kit`), Supabase (from the `DATABASE_URL` host), and Prisma Next. Writes a placeholder encryption client; prompts only if a file already exists there. The placeholder declares one sentinel table, `__stash_placeholder__`, because `Encryption` requires at least one — replace it with the tables you actually encrypt. Until you do, the commands that read the client file — `db push`, `db validate`, and `encrypt backfill` — exit 1 and point back at it. 5. **Install dependencies** — one combined prompt for `@cipherstash/stack` and `stash`. Skipped when both are present. 6. **Install EQL** — always EQL v3. **Drizzle** projects generate a v3 install migration (the same output as `eql migration --drizzle`, including the `cs_migrations` tracking schema) so the install lands in your migration history — apply it with `drizzle-kit migrate`; requires `drizzle-kit` to be installed and configured. **Prisma Next** is skipped (it installs EQL via `prisma-next migrate`). Everything else runs `eql install` directly against the resolved database, and is skipped when EQL is already installed. 7. **Gather context** — detects available coding agents and writes `.cipherstash/context.json`. From fd5aafe9c2e71fedbe9e4ba4f8c130e09b21d30c Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Mon, 27 Jul 2026 10:52:38 +1000 Subject: [PATCH 4/6] fix(cli,migrate): close gaps found verifying the #787 review fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 1d144128, from an adversarial cross-check of that commit. resolve-eql's private `columnExists` used a bare `to_regclass($1)`. That form parses and case-folds unquoted identifiers, so on a Prisma-style "User" table the probe reported the column missing, the recorded pairing was treated as stale, and the #772 fail-closed silently did not fire — falling through to the sole/convention rules and resolving the guess it exists to prevent. migrate already documents this exact anti-pattern (REGCLASS_SQL) and its test asserts `not.toMatch(/to_regclass\(\$1\)/)`. Moved to a shared, case-exact `columnExists` export and deleted the CLI copy; the test double is now case-exact too, so it cannot hide a regression. The placeholder guard read the harvested export map while the `db push` / `db validate` guard it mirrors reads `getEncryptConfig().tables`. Those disagree in both directions on one file: `schemas: [placeholderTable]` minus the `export` keyword fell through to "table not found … Available: (none)" — the error the guard replaces — and a stale placeholder export beside real tables wrongly fired it. Now reads the same source. cutover's `via:'sole'` refusal was nested inside `version === 3`; drop's is top-level. Equivalent today, but cutover's v2 ladder does an irreversible rename plus config promotion, so a restored v2 classification or a v4 family would let cutover rename on a guess drop refuses. Hoisted to match. Text that was false: - the scaffold `stash init` writes into every customer project (and both fixtures) claimed `stash encrypt` commands refuse to run; only backfill does - skills/stash-cli listed the client as loaded by `schema build` and `encrypt *`, and omitted `db push` - skills/stash-cli and skills/stash-encryption still said cutover on a backfilled v3 column exits 0. Since `.cipherstash/` is gitignored, a clone or CI runner hits the new `sole` refusal on a pure-v3 table with an unconventional column name — the changeset's "pure-v3 unaffected" was wrong too - both `sole` messages said "the table's only EQL column"; pickEncryptedColumn excludes the plaintext column first, so it fires with two - the remedy said to drive the v2 lifecycle "directly"; there is no CLI route, so it now says to run the eql_v2 SQL yourself - vitest.config.ts and packages/cli/AGENTS.md claimed the unit suite is self-contained. It is not: @cipherstash/stack is still reached via migrate/src/backfill.ts. The alias removed one of two couplings typecheck:scaffold ran `tsc` directly, bypassing turbo, so it needed a prior build and passed only by accident of step ordering behind steps that read as independently droppable. Now a turbo task with dependsOn ^build. Added scripts/__tests__/cli-vitest-alias.test.mjs — the CLI's alias map sits outside the vitest.shared.ts guard and must, since stackSourceAlias's '@/' would clobber the CLI's own. Runs in test:scripts, ahead of the build-dependent suite. Verified: frozen-lockfile install clean; 891 CLI unit, 41 migrate, 93 scripts, 76 pty e2e; code:check 0 errors; scaffold gate green through turbo with packages/stack/dist absent. --- .changeset/encrypt-lifecycle-mixed-table.md | 29 +++++++--- .changeset/migrate-column-exists.md | 21 ++++++++ .github/workflows/tests.yml | 8 ++- packages/cli/AGENTS.md | 15 +++++- .../scaffold/drizzle.generated.ts | 10 ++-- .../scaffold/generic.generated.ts | 10 ++-- .../__tests__/context-placeholder.test.ts | 54 ++++++++++++++++++- packages/cli/src/commands/encrypt/context.ts | 15 +++++- packages/cli/src/commands/encrypt/cutover.ts | 37 ++++++++----- packages/cli/src/commands/encrypt/drop.ts | 2 +- .../encrypt/lib/__tests__/resolve-eql.test.ts | 30 ++++++++++- .../src/commands/encrypt/lib/resolve-eql.ts | 22 +------- packages/cli/src/commands/init/utils.ts | 20 ++++--- packages/cli/vitest.config.ts | 18 +++++-- .../migrate/src/__tests__/version.test.ts | 34 ++++++++++++ packages/migrate/src/index.ts | 1 + packages/migrate/src/version.ts | 32 +++++++++++ scripts/__tests__/cli-vitest-alias.test.mjs | 45 ++++++++++++++++ skills/stash-cli/SKILL.md | 4 +- skills/stash-encryption/SKILL.md | 4 +- turbo.json | 4 ++ 21 files changed, 343 insertions(+), 72 deletions(-) create mode 100644 .changeset/migrate-column-exists.md create mode 100644 scripts/__tests__/cli-vitest-alias.test.mjs diff --git a/.changeset/encrypt-lifecycle-mixed-table.md b/.changeset/encrypt-lifecycle-mixed-table.md index 95bebb711..3b3dab41a 100644 --- a/.changeset/encrypt-lifecycle-mixed-table.md +++ b/.changeset/encrypt-lifecycle-mixed-table.md @@ -35,10 +35,25 @@ all now fixed: column that actually encrypts the named one, and says explicitly not to record the guess. -The new refusal is scoped to the mixed table it was written for: it applies only -when the table actually holds EQL v3 columns that a guess could wrongly claim. A -pure-v2 table has none, so it still falls through to the EQL v2 lifecycle exactly -as before — including when `encrypt backfill` recorded an `encryptedColumn` for -it, which it does for v2 columns too. Pure-v2 and pure-v3 tables are therefore -unaffected, as are tables with two or more EQL v3 columns (resolution already -failed closed there). +The `unresolvedHint` refusal is scoped to tables that actually hold EQL v3 +columns a guess could wrongly claim. A **pure-v2** table has none, so it still +falls through to the EQL v2 lifecycle exactly as before — including when +`encrypt backfill` recorded an `encryptedColumn` for it, which it does for v2 +columns too. + +Two cases DO newly exit 1, both deliberately: + +- Any table with at least one EQL v3 column where the manifest records an + `encryptedColumn` that exists but is not one of them — not only the + v2-pair-plus-one-v3 shape. The recorded pairing is authoritative and + disagrees with every candidate, so guessing past it is the bug. + +- A column whose encrypted counterpart could only be identified **by + elimination** — no recorded `encryptedColumn`, no `_encrypted` name + match, one EQL column left once the plaintext column itself is excluded. + `cutover` now refuses this as `drop` already did. Note `.cipherstash/` is + gitignored, so `migrations.json` is machine-local: a fresh clone or CI runner + can hit this on a **pure-v3** table whose encrypted column is named + unconventionally, where `cutover` previously exited 0 with "not applicable". + Re-run `stash encrypt backfill --table T --column C --encrypted-column ` + to record the pairing. diff --git a/.changeset/migrate-column-exists.md b/.changeset/migrate-column-exists.md new file mode 100644 index 000000000..d88867430 --- /dev/null +++ b/.changeset/migrate-column-exists.md @@ -0,0 +1,21 @@ +--- +'@cipherstash/migrate': minor +'stash': patch +--- + +Add `columnExists(client, tableName, columnName)` — a case-exact "does this +column exist at all?" catalog probe, distinct from `detectColumnEqlVersion`'s +"and is it an EQL column?". + +Callers need that difference to tell a STALE column reference (it is gone) from +a live one the domain classifier simply does not recognise — most often a legacy +`eql_v2_encrypted` counterpart. + +`stash encrypt cutover` / `drop` had a private copy of this probe built on a bare +`to_regclass($1)`. That form *parses* its argument and case-folds unquoted +identifiers, so on a Prisma-style `"User"` table it resolved `user`, reported the +column missing, and treated a valid recorded pairing as stale — silently skipping +the fail-closed that stops those commands acting on a guessed encrypted column. +The shared implementation quotes with `format('%I')` first, like every other +catalog probe in this package, so the lookup is case-exact while still honouring +`search_path` for unqualified names. diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 7caeba077..aadfe5aa5 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -199,8 +199,14 @@ jobs: # emitting a project that fails its first `tsc` — with CI green (#772 # review). The fixtures are pinned byte-for-byte to the generator by # `placeholder-client-fixture.test.ts`. + # Through turbo, not `pnpm run` — the fixtures import `@cipherstash/stack/v3`, + # so this needs that package BUILT. Invoked directly it passed only because + # earlier steps in this job happen to build it via their own `^build`; drop + # or reorder those (they read as guards for other packages, so they look + # independently removable) and this fails `TS2307`, which reads as "the + # scaffold is broken" rather than "you forgot to build" (#787 review). - name: Typecheck (stash init's scaffolded client) - run: pnpm --filter stash run typecheck:scaffold + run: pnpm exec turbo run typecheck:scaffold --filter stash - name: Lint — no hardcoded package-manager runners run: pnpm run lint:runners diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index 73795c3bc..ced2efbff 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -6,11 +6,22 @@ This package has **two** Vitest configs. Run the right one for the change. | Command | Config | Scope | Needs build? | | --- | --- | --- | --- | -| `pnpm --filter stash test` | `vitest.config.ts` | Unit tests under `src/__tests__/**` and `src/**/__tests__/**` | No | +| `pnpm --filter stash test` | `vitest.config.ts` | Unit tests under `src/__tests__/**` and `src/**/__tests__/**` | **Partly** — needs `@cipherstash/stack` built (see below). Turbo's `^build` supplies it in CI. | | `pnpm --filter stash test:e2e` | `vitest.integration.config.ts` | E2E tests under `tests/e2e/**.e2e.test.ts` driving the built `dist/bin/stash.js` through a real pty (`node-pty`) | **Yes** — run `pnpm --filter stash build` first, or use the turbo `test:e2e` task which depends on `build`. | The unit config explicitly excludes `tests/e2e/**` so the default `pnpm test` -stays fast and self-contained. +stays fast. + +It is **not** fully self-contained, despite running standalone in CI. Some `src` +modules import workspace packages that publish `./dist` only, so an unbuilt +workspace fails at collection with `Failed to resolve entry for package …` +rather than at an assertion. `vitest.config.ts` aliases `@cipherstash/migrate` +to its source to remove one such coupling; `@cipherstash/stack` remains, reached +via `packages/migrate/src/backfill.ts` and a direct import in +`init/lib/introspect.test.ts`. Deleting `packages/stack/dist` fails 10 files. +Closing it needs `vitest.shared.ts`'s `stackSourceAlias`, which cannot be spread +into this config — its `'@/'` entry points at `packages/stack/src` and would +clobber this package's own `'@/'` (#787 review). ## When to add or update an E2E test diff --git a/packages/cli/__fixtures__/scaffold/drizzle.generated.ts b/packages/cli/__fixtures__/scaffold/drizzle.generated.ts index a0f9b77f0..755815874 100644 --- a/packages/cli/__fixtures__/scaffold/drizzle.generated.ts +++ b/packages/cli/__fixtures__/scaffold/drizzle.generated.ts @@ -8,8 +8,10 @@ * to reference the encrypted tables you declared there. * * Until that happens, the encryption client is initialised with a single - * placeholder table so that this file compiles, and `stash encrypt` - * commands refuse to run and point back here. + * placeholder table so that this file compiles, and `stash db push`, + * `stash db validate` and `stash encrypt backfill` refuse to run and point + * back here. (`stash encrypt cutover` / `drop` resolve against the database + * and never read this file.) * * This project uses EQL v3. Encrypted columns are concrete Postgres domains * built with the `types.*` factories from `@cipherstash/stack-drizzle`. @@ -56,8 +58,8 @@ import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' // REPLACE THIS. It exists only so this file compiles before you have declared // any encrypted tables — `Encryption` requires at least one. Swap it for your -// real tables (see the patterns above); `stash encrypt` refuses to run while -// the placeholder is still here. +// real tables (see the patterns above); `stash db push`, `stash db validate` +// and `stash encrypt backfill` refuse to run while the placeholder is still here. export const placeholderTable = encryptedTable('__stash_placeholder__', { replace_me: types.Text('replace_me'), }) diff --git a/packages/cli/__fixtures__/scaffold/generic.generated.ts b/packages/cli/__fixtures__/scaffold/generic.generated.ts index 4f5831847..9f0187adc 100644 --- a/packages/cli/__fixtures__/scaffold/generic.generated.ts +++ b/packages/cli/__fixtures__/scaffold/generic.generated.ts @@ -7,8 +7,10 @@ * `Encryption({ schemas: [...] })` call below to reference them. * * Until that happens, the encryption client is initialised with a single - * placeholder table so that this file compiles, and `stash encrypt` - * commands refuse to run and point back here. + * placeholder table so that this file compiles, and `stash db push`, + * `stash db validate` and `stash encrypt backfill` refuse to run and point + * back here. (`stash encrypt cutover` / `drop` resolve against the database + * and never read this file.) * * This project uses EQL v3. Encrypted columns are concrete Postgres domains * built with the `types.*` factories from `@cipherstash/stack/eql/v3` @@ -51,8 +53,8 @@ import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' // REPLACE THIS. It exists only so this file compiles before you have declared // any encrypted tables — `Encryption` requires at least one. Swap it for your -// real tables (see the patterns above); `stash encrypt` refuses to run while -// the placeholder is still here. +// real tables (see the patterns above); `stash db push`, `stash db validate` +// and `stash encrypt backfill` refuse to run while the placeholder is still here. export const placeholderTable = encryptedTable('__stash_placeholder__', { replace_me: types.Text('replace_me'), }) diff --git a/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts b/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts index ad32cded8..f1dacb93a 100644 --- a/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts +++ b/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts @@ -53,8 +53,12 @@ describe('loadEncryptionContext — the un-replaced init scaffold', () => { }) it('exits 1 naming the sentinel when it is the only table declared', async () => { + // The scaffold's real shape: the sentinel is both exported AND the sole + // entry in the built encrypt config, because it was passed to `Encryption`. writeProject( - `export const encryptionClient = { getEncryptConfig: () => ({}) } + `export const encryptionClient = { + getEncryptConfig: () => ({ tables: { '${PLACEHOLDER_TABLE_NAME}': {} } }), + } export const placeholderTable = ${table(PLACEHOLDER_TABLE_NAME)}`, ) const error = vi.spyOn(console, 'error').mockImplementation(() => {}) @@ -72,6 +76,54 @@ describe('loadEncryptionContext — the un-replaced init scaffold', () => { expect(message).not.toContain('was not found in the encryption client') }) + /** + * #787 review. The guard originally read the harvested EXPORT map, while the + * `db push` / `db validate` guard it mirrors reads `getEncryptConfig().tables`. + * Those disagree in both directions on the same client file, so the two + * commands gave different answers for identical input. + */ + it('fires when the placeholder is passed to Encryption but never exported', async () => { + // The false NEGATIVE. `schemas: [placeholderTable]` with a bare `const` — + // the scaffold minus one `export` keyword. Reading exports, the guard saw + // no tables and fell through to `requireTable`'s "table not found … + // Available: (none)" — the very error this guard exists to replace. + writeProject( + `export const encryptionClient = { + getEncryptConfig: () => ({ tables: { '${PLACEHOLDER_TABLE_NAME}': {} } }), + }`, + ) + const error = vi.spyOn(console, 'error').mockImplementation(() => {}) + vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit') + }) + + const { loadEncryptionContext } = await import('../context.js') + await expect(loadEncryptionContext()).rejects.toThrow('process.exit') + + const message = error.mock.calls.flat().join('\n') + expect(message).toContain('still contains the placeholder table') + expect(message).not.toContain('was not found in the encryption client') + }) + + it('stays silent when a stale placeholder export sits beside real configured tables', async () => { + // The false POSITIVE, and the mirror of the case above: the user replaced + // the schema set but left the sentinel `export` behind (or imports their + // real tables without re-exporting them). Reading exports, the guard fired + // and told them to declare columns they had already declared. `db push` + // passes on this same file. + writeProject( + `export const encryptionClient = { + getEncryptConfig: () => ({ tables: { users: {} } }), + } + export const placeholderTable = ${table(PLACEHOLDER_TABLE_NAME)}`, + ) + + const { loadEncryptionContext } = await import('../context.js') + const ctx = await loadEncryptionContext() + + expect(ctx.tables.has(PLACEHOLDER_TABLE_NAME)).toBe(true) + }) + it('allows the sentinel through once a real table sits alongside it', async () => { // Only the SOLE-placeholder case is the un-replaced scaffold. A user who // has added real tables must not be blocked by a leftover sentinel export. diff --git a/packages/cli/src/commands/encrypt/context.ts b/packages/cli/src/commands/encrypt/context.ts index e37dd8a31..0eab28db1 100644 --- a/packages/cli/src/commands/encrypt/context.ts +++ b/packages/cli/src/commands/encrypt/context.ts @@ -142,14 +142,25 @@ export async function loadEncryptionContext(): Promise { process.exit(1) } - // Same guard `loadEncryptConfig` applies for `stash db push` / `db validate`, + // The guard `loadEncryptConfig` applies for `stash db push` / `db validate`, // repeated here because `stash encrypt` does not go through that loader. The // scaffold `stash init` writes declares one sentinel table so the file // compiles; reaching here with only that table means it was never replaced. // Without this, `requireTable` reported `Table "users" was not found … // Available: __stash_placeholder__`, which names the symptom and not the // cause (#787 review). - if (tables.size === 1 && tables.has(PLACEHOLDER_TABLE_NAME)) { + // + // Read from `getEncryptConfig()` — the SAME source `loadEncryptConfig` uses — + // not from the harvested export map, or the two commands disagree on one + // file. `schemas: [placeholderTable]` with the `export` keyword dropped is + // still the un-replaced scaffold but exports nothing; conversely a stale + // `export const placeholderTable` beside real tables that are imported + // rather than re-exported is NOT (#787 review). + const configuredTables = Object.keys(client.getEncryptConfig()?.tables ?? {}) + if ( + configuredTables.length === 1 && + configuredTables[0] === PLACEHOLDER_TABLE_NAME + ) { console.error( `Error: ${stashConfig.client} still contains the placeholder table \`${PLACEHOLDER_TABLE_NAME}\` that \`stash init\` wrote.\n\nDeclare your encrypted columns and pass those tables to Encryption({ schemas: [...] }) in that file, then re-run this command.`, ) diff --git a/packages/cli/src/commands/encrypt/cutover.ts b/packages/cli/src/commands/encrypt/cutover.ts index 7a5cd7f1c..1080e9524 100644 --- a/packages/cli/src/commands/encrypt/cutover.ts +++ b/packages/cli/src/commands/encrypt/cutover.ts @@ -95,23 +95,32 @@ export async function cutoverCommand(options: CutoverCommandOptions) { } const state = await progress(client, options.table, options.column) + // `via: 'sole'` means only that this is the table's one remaining EQL + // candidate once the plaintext column itself is excluded — nothing ties it + // to the plaintext column the user named. On a mixed table (a v2 pair the + // classifier no longer sees, plus one unrelated v3 column) that guess is + // simply wrong, and reporting "nothing to do for EQL v3" for it told a + // scripted rollout the cut-over had succeeded when the v2 rename never ran + // (#772 review, finding 7). + // + // Deliberately at TOP LEVEL, not inside the `version === 3` branch, so it + // mirrors `drop.ts` exactly. Equivalent today — `classifyEqlDomain` + // recognises `eql_v3_*` only, so a non-null `info` is always version 3 — + // but the v2 ladder below performs an irreversible rename plus config + // promotion. Were v2 classification restored, or a v4 family added, a + // nested guard would let `cutover` rename on a guess that `drop` refuses + // (#787 review). + if (info?.via === 'sole') { + p.log.error( + `${options.table}.${info.column} (${info.domain}) is the only EQL column left on ${options.table} once "${options.column}" itself is excluded, but nothing confirms it encrypts "${options.column}" — refusing to report a cut-over outcome on that guess. If "${options.column}" pairs with a legacy eql_v2_encrypted column, resolution cannot see it (this command resolves EQL v3 counterparts only): complete that column's v2 lifecycle yourself with the eql_v2 SQL — \`SELECT eql_v2.rename_encrypted_columns();\` plus the config promotion — since no stash command can drive it here. Otherwise record the pairing: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column \`.`, + ) + exitCode = 1 + return + } + if (info?.version === 3) { const encryptedColumn = info.column - // `via: 'sole'` means only that this is the table's ONE EQL v3 column — - // nothing ties it to the plaintext column the user named. On a mixed - // table (a v2 pair the classifier no longer sees, plus one unrelated v3 - // column) that guess is simply wrong, and reporting "nothing to do for - // EQL v3" for it told a scripted rollout the cut-over had succeeded when - // the v2 rename never ran. `drop.ts` already refuses a `'sole'` match for - // the same reason (#772 review, finding 7). - if (info.via === 'sole') { - p.log.error( - `${options.table}.${encryptedColumn} (${info.domain}) is the table's only EQL v3 column, but nothing confirms it encrypts "${options.column}" — refusing to report a cut-over outcome on that guess. If "${options.column}" pairs with a legacy eql_v2_encrypted column, resolution cannot see it (this command resolves EQL v3 counterparts only) — drive that column's v2 lifecycle against its own encrypted column directly. Otherwise record the pairing: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column \`.`, - ) - exitCode = 1 - return - } if (state?.phase === 'dropped') { // Terminal phase — the lifecycle already finished. Not an error and // not "finish the backfill": there is nothing left to backfill. diff --git a/packages/cli/src/commands/encrypt/drop.ts b/packages/cli/src/commands/encrypt/drop.ts index 49b0bff8c..d016a0c63 100644 --- a/packages/cli/src/commands/encrypt/drop.ts +++ b/packages/cli/src/commands/encrypt/drop.ts @@ -113,7 +113,7 @@ export async function dropCommand(options: DropCommandOptions) { // live `DROP COLUMN` on the plaintext at exit 0 (#772 review, finding 7). if (info?.via === 'sole') { p.log.error( - `${options.table}.${info.column} (${info.domain}) is the table's only encrypted column, but nothing confirms it encrypts "${options.column}" — refusing to generate an irreversible drop on that guess. Identify the column that actually encrypts "${options.column}" and record that pairing: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column \` (which writes it to the manifest), or set "encryptedColumn" for this column in .cipherstash/migrations.json. If "${options.column}" pairs with a legacy eql_v2_encrypted column, resolution cannot see it (this command resolves EQL v3 counterparts only) — drive that column's v2 lifecycle against its own encrypted column directly, and do not record ${info.column}.`, + `${options.table}.${info.column} (${info.domain}) is the only EQL column left on ${options.table} once "${options.column}" itself is excluded, but nothing confirms it encrypts "${options.column}" — refusing to generate an irreversible drop on that guess. Identify the column that actually encrypts "${options.column}" and record that pairing: re-run \`stash encrypt backfill --table ${options.table} --column ${options.column} --encrypted-column \` (which writes it to the manifest), or set "encryptedColumn" for this column in .cipherstash/migrations.json. If "${options.column}" pairs with a legacy eql_v2_encrypted column, resolution cannot see it (this command resolves EQL v3 counterparts only): complete that column's v2 lifecycle yourself with the eql_v2 SQL, since no stash command can drive it here — and do not record ${info.column}.`, ) exitCode = 1 return diff --git a/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts b/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts index 9c45d78b7..154c5e563 100644 --- a/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts +++ b/packages/cli/src/commands/encrypt/lib/__tests__/resolve-eql.test.ts @@ -48,8 +48,13 @@ const { explainUnresolved, resolveColumnLifecycle } = await import( */ const clientWithColumns = (...columns: string[]): pg.ClientBase => ({ + // `columnExists` binds [table, schema, column] — it splits schema-qualified + // names and quotes with `format('%I')` so the lookup is case-EXACT. This + // double matches case-exactly for the same reason: a case-insensitive + // fixture would keep passing if the probe regressed to a bare + // `to_regclass($1)`, which case-folds (#787 review). query: async (_sql: string, params: unknown[]) => ({ - rows: [{ exists: columns.includes(String(params[1])) }], + rows: [{ exists: columns.includes(String(params[2])) }], }), }) as unknown as pg.ClientBase @@ -177,6 +182,29 @@ describe('resolveColumnLifecycle — a recorded hint that is not a v3 candidate' ).toBeNull() }) + // #787 review. `columnExists` used to be a local copy using a bare + // `to_regclass($1)`, which PARSES and case-folds its argument — so on a + // Prisma-style `"User"` table the probe reported "missing", the hint was + // treated as stale, and the fail-closed above silently did not fire. It fell + // through to the sole/convention rules and resolved a guess, which is exactly + // what #772 finding 7 exists to prevent. Now delegated to + // `@cipherstash/migrate`'s shared, case-exact probe. + it('fires the fail-closed on a mixed-case table name too', async () => { + listEncryptedColumns.mockResolvedValue([V3_OTHER]) + readManifest.mockResolvedValue({ + tables: { User: [{ column: 'ssn', encryptedColumn: 'ssn_encrypted' }] }, + }) + + const { info, unresolvedHint } = await resolveColumnLifecycle( + clientWithColumns('ssn', 'ssn_encrypted', 'email_enc'), + 'User', + 'ssn', + ) + + expect(info).toBeNull() + expect(unresolvedHint).toBe('ssn_encrypted') + }) + it('explains the recorded counterpart by name rather than listing candidates', async () => { const message = explainUnresolved( 'users', diff --git a/packages/cli/src/commands/encrypt/lib/resolve-eql.ts b/packages/cli/src/commands/encrypt/lib/resolve-eql.ts index 6c4834d0b..d2b0ac889 100644 --- a/packages/cli/src/commands/encrypt/lib/resolve-eql.ts +++ b/packages/cli/src/commands/encrypt/lib/resolve-eql.ts @@ -1,4 +1,5 @@ import { + columnExists, type EncryptedColumnInfo, listEncryptedColumns, pickEncryptedColumn, @@ -111,25 +112,6 @@ export async function resolveColumnLifecycle( return { info: pickEncryptedColumn(candidates, column), candidates } } -/** Whether `column` exists on `table` at all, whatever its type. */ -async function columnExists( - client: pg.ClientBase, - table: string, - column: string, -): Promise { - const { rows } = await client.query<{ exists: boolean }>( - `SELECT EXISTS ( - SELECT 1 FROM pg_attribute - WHERE attrelid = to_regclass($1) - AND attname = $2 - AND attnum > 0 - AND NOT attisdropped - ) AS exists`, - [table, column], - ) - return rows[0]?.exists === true -} - /** * Explain a failed resolution (`info === null`) to the user, or return * `null` when the failure is fine to fall through to the v2 lifecycle. @@ -171,7 +153,7 @@ export function explainUnresolved( // that: listing the v3 candidates here would invite the user to record one of // them, which is how the guess used to get laundered into a `via: 'hint'` match. if (unresolvedHint !== undefined) { - return `${table}.${column} is recorded as pairing with "${unresolvedHint}", but ${unresolvedHint} is not an EQL v3 column — it is most likely a legacy eql_v2_encrypted column. ${table} also holds EQL v3 columns, and none of them is a confirmed counterpart for ${column}, so this command cannot tell which lifecycle applies and will not guess.\n\nIf that pairing is wrong, correct or remove "encryptedColumn" for ${column} in .cipherstash/migrations.json and re-run. If it is right, ${column} is on the EQL v2 lifecycle: drive it against ${unresolvedHint} directly rather than through this command, which resolves EQL v3 counterparts only.` + return `${table}.${column} is recorded as pairing with "${unresolvedHint}", but ${unresolvedHint} is not an EQL v3 column — it is most likely a legacy eql_v2_encrypted column. ${table} also holds EQL v3 columns, and none of them is a confirmed counterpart for ${column}, so this command cannot tell which lifecycle applies and will not guess.\n\nIf that pairing is wrong, correct or remove "encryptedColumn" for ${column} in .cipherstash/migrations.json and re-run. If it is right, ${column} is on the EQL v2 lifecycle, which no stash command can drive on this table — complete it yourself against ${unresolvedHint} with the eql_v2 SQL.` } const listed = candidates diff --git a/packages/cli/src/commands/init/utils.ts b/packages/cli/src/commands/init/utils.ts index 282a0320b..9750af055 100644 --- a/packages/cli/src/commands/init/utils.ts +++ b/packages/cli/src/commands/init/utils.ts @@ -385,8 +385,10 @@ const DRIZZLE_PLACEHOLDER = `/** * to reference the encrypted tables you declared there. * * Until that happens, the encryption client is initialised with a single - * placeholder table so that this file compiles, and \`stash encrypt\` - * commands refuse to run and point back here. + * placeholder table so that this file compiles, and \`stash db push\`, + * \`stash db validate\` and \`stash encrypt backfill\` refuse to run and point + * back here. (\`stash encrypt cutover\` / \`drop\` resolve against the database + * and never read this file.) * * This project uses EQL v3. Encrypted columns are concrete Postgres domains * built with the \`types.*\` factories from \`@cipherstash/stack-drizzle\`. @@ -433,8 +435,8 @@ import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' // REPLACE THIS. It exists only so this file compiles before you have declared // any encrypted tables — \`Encryption\` requires at least one. Swap it for your -// real tables (see the patterns above); \`stash encrypt\` refuses to run while -// the placeholder is still here. +// real tables (see the patterns above); \`stash db push\`, \`stash db validate\` +// and \`stash encrypt backfill\` refuse to run while the placeholder is still here. export const placeholderTable = encryptedTable('__stash_placeholder__', { replace_me: types.Text('replace_me'), }) @@ -451,8 +453,10 @@ const GENERIC_PLACEHOLDER = `/** * \`Encryption({ schemas: [...] })\` call below to reference them. * * Until that happens, the encryption client is initialised with a single - * placeholder table so that this file compiles, and \`stash encrypt\` - * commands refuse to run and point back here. + * placeholder table so that this file compiles, and \`stash db push\`, + * \`stash db validate\` and \`stash encrypt backfill\` refuse to run and point + * back here. (\`stash encrypt cutover\` / \`drop\` resolve against the database + * and never read this file.) * * This project uses EQL v3. Encrypted columns are concrete Postgres domains * built with the \`types.*\` factories from \`@cipherstash/stack/eql/v3\` @@ -495,8 +499,8 @@ import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' // REPLACE THIS. It exists only so this file compiles before you have declared // any encrypted tables — \`Encryption\` requires at least one. Swap it for your -// real tables (see the patterns above); \`stash encrypt\` refuses to run while -// the placeholder is still here. +// real tables (see the patterns above); \`stash db push\`, \`stash db validate\` +// and \`stash encrypt backfill\` refuse to run while the placeholder is still here. export const placeholderTable = encryptedTable('__stash_placeholder__', { replace_me: types.Text('replace_me'), }) diff --git a/packages/cli/vitest.config.ts b/packages/cli/vitest.config.ts index 195a6a156..6f2cf7f76 100644 --- a/packages/cli/vitest.config.ts +++ b/packages/cli/vitest.config.ts @@ -12,9 +12,21 @@ export default defineConfig({ // `@cipherstash/migrate` publishes `./dist` only, so importing it — or // spreading `importOriginal()` inside a partial `vi.mock` — makes the // UNIT suite require a prior workspace build. CI only hides that because - // turbo runs `^build` first; on a clean checkout `pnpm --filter stash - // test` would fail to resolve it. Resolving to source keeps the unit - // config self-contained, as `AGENTS.md` says it is (#787 review). + // turbo runs `^build` first; without this alias and without a build, 10 + // files / 177 tests fail to collect with `Failed to resolve entry for + // package "@cipherstash/migrate"` — the transitive `src` importers + // (`status/index.ts`, `db/install.ts`, `encrypt/lib/db-readers.ts`), not + // just the one mocked test. A full-factory `vi.mock` does not help: + // Vite's import analysis fails on the source module's import statement + // before mocking runs. + // + // This removes the `@cipherstash/migrate` build coupling ONLY. The suite + // is still not self-contained: `packages/migrate/src/backfill.ts` imports + // `@cipherstash/stack`, so removing `packages/stack/dist` still fails 10 + // files. Closing that needs `stackSourceAlias`, which cannot be spread + // here — its `'@/'` entry (pointing at `packages/stack/src`) would + // clobber this package's own `'@/'`. That is why `vitest.shared.ts` is + // not imported by this config (#787 review). '@cipherstash/migrate': resolve(__dirname, '../migrate/src/index.ts'), }, }, diff --git a/packages/migrate/src/__tests__/version.test.ts b/packages/migrate/src/__tests__/version.test.ts index 96e5cfbb6..140b12738 100644 --- a/packages/migrate/src/__tests__/version.test.ts +++ b/packages/migrate/src/__tests__/version.test.ts @@ -2,6 +2,7 @@ import type { ClientBase } from 'pg' import { describe, expect, it, vi } from 'vitest' import { classifyEqlDomain, + columnExists, detectColumnEqlVersion, type EncryptedColumnInfo, listEncryptedColumns, @@ -202,3 +203,36 @@ describe('resolveEncryptedColumn', () => { }) }) }) + +describe('columnExists', () => { + it('returns true when the catalog reports the column', async () => { + const { client } = mockClient([{ exists: true }]) + expect(await columnExists(client, 'users', 'ssn_encrypted')).toBe(true) + }) + + it('returns false when it does not', async () => { + const { client } = mockClient([{ exists: false }]) + expect(await columnExists(client, 'users', 'gone')).toBe(false) + }) + + it('preserves identifier case — no bare to_regclass($1)', async () => { + // Same guard as `detectColumnEqlVersion` above. A bare `to_regclass($1)` + // parses its argument and case-folds it, so a Prisma-style `"User"` table + // silently resolves to `user` and the probe reports "column missing" for a + // column that plainly exists. In `resolveColumnLifecycle` that turns the + // #772 fail-closed into a silent no-op (#787 review). + const { client, query } = mockClient([{ exists: true }]) + await columnExists(client, 'User', 'ssn_encrypted') + const [sql, values] = query.mock.calls[0] as [string, unknown[]] + expect(sql).toContain("format('%I'") + expect(sql).not.toMatch(/to_regclass\(\$1\)/) + expect(values).toEqual(['User', null, 'ssn_encrypted']) + }) + + it('splits schema-qualified names on the first dot, like qualifyTable', async () => { + const { client, query } = mockClient([{ exists: true }]) + await columnExists(client, 'app.users', 'ssn_encrypted') + const [, values] = query.mock.calls[0] as [string, unknown[]] + expect(values).toEqual(['users', 'app', 'ssn_encrypted']) + }) +}) diff --git a/packages/migrate/src/index.ts b/packages/migrate/src/index.ts index 65bd0db15..026b5f6e5 100644 --- a/packages/migrate/src/index.ts +++ b/packages/migrate/src/index.ts @@ -73,6 +73,7 @@ export { } from './state.js' export { classifyEqlDomain, + columnExists, detectColumnEqlVersion, type EncryptedColumnInfo, type EncryptedColumnResolution, diff --git a/packages/migrate/src/version.ts b/packages/migrate/src/version.ts index 60e1d9d41..88c496170 100644 --- a/packages/migrate/src/version.ts +++ b/packages/migrate/src/version.ts @@ -105,6 +105,38 @@ export async function detectColumnEqlVersion( return domain === undefined ? null : classifyEqlDomain(domain) } +/** + * Whether `columnName` exists on `tableName` at all, whatever its type. + * + * Distinct from {@link detectColumnEqlVersion}, which answers "and is it an EQL + * column?". Callers need the difference to tell a STALE reference (the column + * is gone) from a live one the classifier simply does not recognise — most + * often a legacy `eql_v2_encrypted` counterpart. + * + * Lives here rather than in the CLI so it shares {@link REGCLASS_SQL} with the + * other catalog probes: a hand-rolled `to_regclass($1)` case-folds unquoted + * identifiers and silently reports "missing" for a Prisma-style `"User"` table + * (#787 review). + */ +export async function columnExists( + client: ClientBase, + tableName: string, + columnName: string, +): Promise { + const { schema, table } = splitTableName(tableName) + const { rows } = await client.query<{ exists: boolean }>( + `SELECT EXISTS ( + SELECT 1 FROM pg_attribute + WHERE attrelid = ${REGCLASS_SQL} + AND attname = $3 + AND attnum > 0 + AND NOT attisdropped + ) AS exists`, + [table, schema, columnName], + ) + return rows[0]?.exists === true +} + /** * Every EQL-domain column on a table, classified. The EQL types are * self-describing, so this is the ground truth for "which columns on this diff --git a/scripts/__tests__/cli-vitest-alias.test.mjs b/scripts/__tests__/cli-vitest-alias.test.mjs new file mode 100644 index 000000000..881ca11af --- /dev/null +++ b/scripts/__tests__/cli-vitest-alias.test.mjs @@ -0,0 +1,45 @@ +import { existsSync } from 'node:fs' +import { describe, expect, it } from 'vitest' +import cliVitestConfig from '../../packages/cli/vitest.config.ts' + +/** + * `packages/cli/vitest.config.ts` carries its own alias map, outside the one + * `vitest-shared-alias.test.mjs` guards. It has to: closing the CLI suite's + * remaining build coupling would need `stackSourceAlias`, whose `'@/'` entry + * points at `packages/stack/src` and would clobber the CLI's own `'@/'`. So the + * map is package-local and needs its own guard, for the same reason — an alias + * pointing at a moved file fails LATE, with a "failed to resolve import" naming + * a path nobody wrote. + * + * `@cipherstash/migrate` is pinned by name because it is load-bearing, not + * cosmetic: without it, 10 files / 177 tests fail to COLLECT on an unbuilt + * workspace (the transitive `src` importers, not just the one mocked test). + * `pnpm run test:scripts` runs ahead of the build-dependent suite in CI, so + * this fires before that failure would (#787 review). + */ + +const aliases = cliVitestConfig.resolve.alias + +/** Directory aliases are written with a trailing slash (`'@/'`). */ +const targets = Object.entries(aliases).map(([specifier, target]) => [ + specifier, + target.endsWith('/') ? target.slice(0, -1) : target, +]) + +describe('packages/cli vitest alias map', () => { + it('still aliases @cipherstash/migrate to source', () => { + // migrate publishes `./dist` only. Drop this entry — or re-introduce an + // `importOriginal()` spread that needs the built package — and the unit + // suite silently regains a dependency on a prior `turbo run build`. + expect(Object.keys(aliases)).toContain('@cipherstash/migrate') + expect(aliases['@cipherstash/migrate']).toMatch( + /packages[/\\]migrate[/\\]src[/\\]index\.ts$/, + ) + }) + + it.each( + targets, + )('%s resolves to a file that exists', (_specifier, target) => { + expect(existsSync(target), `missing alias target: ${target}`).toBe(true) + }) +}) diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index e2541e40f..849763fb9 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -199,7 +199,7 @@ export default defineConfig({ | Option | Required | Default | Purpose | |---|---|---|---| | `databaseUrl` | yes | — | PostgreSQL connection string | -| `client` | no | `./src/encryption/index.ts` | Encryption client, loaded by `db validate`, `schema build`, `encrypt *` | +| `client` | no | `./src/encryption/index.ts` | Encryption client, loaded by `db push`, `db validate`, `encrypt backfill` (`schema build` only *writes* here; `encrypt cutover`/`drop` resolve against the database) | Resolved by walking up from `process.cwd()`, like `tsconfig.json`. `stash init` scaffolds it; `stash eql install` offers to. @@ -473,7 +473,7 @@ Backfill **detects a `public.eql_v3_*` target column as EQL v3** from its Postgr stash encrypt cutover --table users --column email ``` -**EQL v2 only** — v3 has no cut-over: the application switches to the encrypted column by name. Running this command on a **backfilled** v3 column reports "not applicable" (exit 0) with the next step; before backfill completes it exits 1 and says to finish the backfill (never "switch now" onto a half-populated column). For v2, the preconditions are: the column is in the `backfilled` phase, **and** a pending EQL configuration exists (on a v3-only database — no `eql_v2_configuration` table — it explains that and exits 1). +**EQL v2 only** — v3 has no cut-over: the application switches to the encrypted column by name. Running this command on a **backfilled** v3 column reports "not applicable" (exit 0) with the next step — *provided the encrypted column can be identified*. If it was resolved only by elimination (the table's one remaining EQL column, with no `encryptedColumn` recorded in `.cipherstash/migrations.json` and no `_encrypted` name match), it refuses and exits 1 rather than report an outcome for a column it guessed. `.cipherstash/` is gitignored, so a clone or CI runner without that manifest can hit this on a table whose encrypted column is named unconventionally; re-run `stash encrypt backfill … --encrypted-column ` to record the pairing; before backfill completes it exits 1 and says to finish the backfill (never "switch now" onto a half-populated column). For v2, the preconditions are: the column is in the `backfilled` phase, **and** a pending EQL configuration exists (on a v3-only database — no `eql_v2_configuration` table — it explains that and exits 1). In one transaction it renames `` → `_plaintext` and `_encrypted` → ``, advances the pending config to `encrypting`, activates it, and appends a `cut_over` event. With a Proxy URL configured (`--proxy-url` or `CIPHERSTASH_PROXY_URL`) it then calls `eql_v2.reload_config()` so Proxy picks up the new shape. diff --git a/skills/stash-encryption/SKILL.md b/skills/stash-encryption/SKILL.md index d766079df..da2f11581 100644 --- a/skills/stash-encryption/SKILL.md +++ b/skills/stash-encryption/SKILL.md @@ -822,7 +822,7 @@ try { ## Rolling Encryption Out to Production -> **EQL version note.** The rollout tooling (`stash encrypt *`, `@cipherstash/migrate`) works with **both EQL versions** and detects a column's generation from its Postgres domain type — there is no flag. Detection is one-sided: a `public.eql_v3_*` domain classifies as **v3**; anything else — a plaintext column, or a legacy `eql_v2_encrypted` one — classifies as *unknown* and falls through to the **v2** lifecycle, which is the correct default for a v2 column. Only a v3 column has its version recorded in the migration manifest. The lifecycles differ at the end: **v2** finishes with `stash encrypt cutover` (a rename swap plus a config promotion in `eql_v2_configuration`), then drops `_plaintext`. **v3 has no cut-over and no configuration table** — after backfill you point the application at `_encrypted` *by name*, verify reads, then `stash encrypt drop` generates the drop of the original plaintext ``. Running `encrypt cutover` on a v3 column safely reports "not applicable" with the next step. `stash db push`/`db activate` remain v2-only (they manage `eql_v2_configuration`). +> **EQL version note.** The rollout tooling (`stash encrypt *`, `@cipherstash/migrate`) works with **both EQL versions** and detects a column's generation from its Postgres domain type — there is no flag. Detection is one-sided: a `public.eql_v3_*` domain classifies as **v3**; anything else — a plaintext column, or a legacy `eql_v2_encrypted` one — classifies as *unknown* and falls through to the **v2** lifecycle, which is the correct default for a v2 column — *unless* the table also holds EQL v3 columns and `.cipherstash/migrations.json` records an `encryptedColumn` that is one of the unclassified ones. That combination is ambiguous, so `cutover`/`drop` fail closed and name the recorded column instead of guessing. Only a v3 column has its version recorded in the migration manifest. The lifecycles differ at the end: **v2** finishes with `stash encrypt cutover` (a rename swap plus a config promotion in `eql_v2_configuration`), then drops `_plaintext`. **v3 has no cut-over and no configuration table** — after backfill you point the application at `_encrypted` *by name*, verify reads, then `stash encrypt drop` generates the drop of the original plaintext ``. Running `encrypt cutover` on a v3 column safely reports "not applicable" with the next step. `stash db push`/`db activate` remain v2-only (they manage `eql_v2_configuration`). Adding a fresh encrypted column to a table you don't yet write to is the easy case — declare it in the schema, run the migration, start writing. The harder case is taking an **existing plaintext column with live data** and turning it into an encrypted one without dropping a write or returning the wrong value mid-cutover. @@ -873,7 +873,7 @@ Once dual-writes are recorded as live in `cs_migrations`: |---|---| | `stash encrypt backfill` | Walks the table in keyset-pagination order, encrypts each chunk, writes a single transactional `UPDATE` per chunk plus a `cs_migrations` checkpoint. SIGINT-safe; idempotent re-runs converge. | | Schema rename (**v2 only**) | Update the schema file: drop the `_encrypted` suffix; switch the original column declaration onto the encrypted type. **v3:** there is no rename — leave `_encrypted` under its own name and point the schema/queries at that name instead. | -| `stash encrypt cutover` (**v2 only**) | One transaction: renames `` → `_plaintext`, `_encrypted` → ``, and promotes the `eql_v2_configuration` row `pending` → `active`. Reads of `` through **CipherStash Proxy** are decrypted on the wire; SDK/ORM reads still return ciphertext and need the decrypt path in the next row. **v3:** cutover does not apply — on a backfilled v3 column it reports "not applicable" and exits 0 without changing anything; skip this row. | +| `stash encrypt cutover` (**v2 only**) | One transaction: renames `` → `_plaintext`, `_encrypted` → ``, and promotes the `eql_v2_configuration` row `pending` → `active`. Reads of `` through **CipherStash Proxy** are decrypted on the wire; SDK/ORM reads still return ciphertext and need the decrypt path in the next row. **v3:** cutover does not apply — on a backfilled v3 column it reports "not applicable" and exits 0 without changing anything; skip this row. Exception: if the encrypted column was identified only by elimination (no recorded `encryptedColumn`, no `_encrypted` name match), it exits 1 rather than report an outcome for a guess — record the pairing with `stash encrypt backfill … --encrypted-column `. | | Wire reads through the encryption client | Read paths must decrypt before returning the value to callers (`decryptModel(row, table)` for Drizzle; the Supabase wrapper for Supabase; `decrypt`/`bulkDecryptModels` otherwise). Without this step, reads return raw EQL payloads to end users (a `public.eql_v3_*` jsonb document on v3; an `eql_v2_encrypted` composite on a legacy v2 column). | | Remove dual-write code | The plaintext column is no longer authoritative — **v2:** it is now `_plaintext` (the cutover renamed it); **v3:** it is still the original ``, since nothing was renamed. Either way, delete the dual-write logic once reads are served from the encrypted column. | | `stash encrypt drop` | Emits a migration that drops the plaintext column — and *which* column that is depends on the generation. **v2** (precondition: phase `cut-over`): a plain `ALTER TABLE … DROP COLUMN "_plaintext"`. **v3** (precondition: phase `backfilled`): it drops the **original ``** — there is no `_plaintext` — and the generated SQL is a `DO` block that takes `ACCESS EXCLUSIVE` on the table, re-counts rows with `` set and `_encrypted` NULL *at apply time*, and raises instead of dropping if any remain. Apply with the project's normal migration tooling. | diff --git a/turbo.json b/turbo.json index f86fa0a04..0acfb8b38 100644 --- a/turbo.json +++ b/turbo.json @@ -55,6 +55,10 @@ "dependsOn": ["^build"], "inputs": ["$TURBO_DEFAULT$"] }, + "typecheck:scaffold": { + "dependsOn": ["^build"], + "inputs": ["$TURBO_DEFAULT$"] + }, "test:e2e": { "dependsOn": ["^build", "build"], "inputs": ["$TURBO_DEFAULT$", ".env*"], From 39060e65f323aeae41d3c841c8832347da129272 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Mon, 27 Jul 2026 11:06:55 +1000 Subject: [PATCH 5/6] test(ci): pin the turbo routing that the #787 review's minor finding exposed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer's two findings were both already fixed at bbade2c4 — this adds the guards that would have caught them, plus two accuracy fixes. `typecheck:scaffold` declares `dependsOn: ["^build"]`, so turbo has to be the thing invoking it. #787 first added it as a bare `pnpm --filter stash run typecheck:scaffold` and CI went green anyway: an earlier step in the same job happened to build the workspace via its own `^build`. Those earlier steps read as independent guards for other packages, so they look freely removable — delete one and the scaffold step fails `TS2307`, which reads as "the scaffold is broken" rather than "you skipped the build". Nothing caught that. scripts/__tests__/workflow-turbo-build-deps.test.mjs derives the protected set from turbo.json (every task declaring `^build`) rather than hardcoding names, and asserts tests.yml routes them through turbo. Mutation-tested against three regressions: reverting the step to the bare form, deleting the step outright, and adding a new bare build-dependent step — each fails with a message naming the step and the fix. Two pre-existing bare `typecheck` invocations (prisma-next, wizard) carry the same latent trap; they are recorded in KNOWN_BARE rather than silently skipped, and a fourth test fails if an entry goes stale so the list gets worked down instead of accumulating. turbo.json is JSONC, so reading it needs the string-aware comment stripper #782 wrote for `turbo-skills-inputs.test.mjs` — a naive regex eats the `//` in `"$schema": "https://turbo.build/schema.json"`. Rather than hand-copy 30 lines of parser into an adjacent file, that function moves to `scripts/__tests__/lib/read-jsonc.mjs` and both guards import it. Two accuracy fixes: - vitest.config.ts claimed the suite's residual `@cipherstash/stack` coupling runs through `migrate/src/backfill.ts`. That is only one of two routes: `init/lib/__tests__/introspect.test.ts:1` imports `@cipherstash/stack/eql/v3` directly, never touching migrate. As written, someone could decouple backfill.ts and expect the suite to go standalone. It would not. - cli-vitest-alias.test.mjs called `.endsWith()` on every alias value, so Vite's array form (`[{find, replacement}]`) would throw an opaque TypeError from a helper — reading as "the guard is broken" rather than "the config changed shape". It now fails cleanly, naming the offender, and says to update the guard rather than delete it. No changeset: repo tooling and a comment, no published surface touched. --- packages/cli/vitest.config.ts | 17 +- scripts/__tests__/cli-vitest-alias.test.mjs | 28 ++- scripts/__tests__/lib/read-jsonc.mjs | 49 ++++++ .../__tests__/turbo-skills-inputs.test.mjs | 37 +--- .../workflow-turbo-build-deps.test.mjs | 166 ++++++++++++++++++ 5 files changed, 251 insertions(+), 46 deletions(-) create mode 100644 scripts/__tests__/lib/read-jsonc.mjs create mode 100644 scripts/__tests__/workflow-turbo-build-deps.test.mjs diff --git a/packages/cli/vitest.config.ts b/packages/cli/vitest.config.ts index 6f2cf7f76..76b3903f4 100644 --- a/packages/cli/vitest.config.ts +++ b/packages/cli/vitest.config.ts @@ -21,12 +21,17 @@ export default defineConfig({ // before mocking runs. // // This removes the `@cipherstash/migrate` build coupling ONLY. The suite - // is still not self-contained: `packages/migrate/src/backfill.ts` imports - // `@cipherstash/stack`, so removing `packages/stack/dist` still fails 10 - // files. Closing that needs `stackSourceAlias`, which cannot be spread - // here — its `'@/'` entry (pointing at `packages/stack/src`) would - // clobber this package's own `'@/'`. That is why `vitest.shared.ts` is - // not imported by this config (#787 review). + // is still not self-contained: removing `packages/stack/dist` still + // fails 10 files, via TWO independent routes — + // 1. `packages/migrate/src/backfill.ts` imports `@cipherstash/stack`, + // so this alias reaches it transitively; and + // 2. `init/lib/__tests__/introspect.test.ts` imports + // `@cipherstash/stack/eql/v3` directly, never touching migrate. + // Route 2 means decoupling `backfill.ts` would NOT make the suite + // standalone. Closing both needs `stackSourceAlias`, which cannot be + // spread here — its `'@/'` entry (pointing at `packages/stack/src`) + // would clobber this package's own `'@/'`. That is why + // `vitest.shared.ts` is not imported by this config (#787 review). '@cipherstash/migrate': resolve(__dirname, '../migrate/src/index.ts'), }, }, diff --git a/scripts/__tests__/cli-vitest-alias.test.mjs b/scripts/__tests__/cli-vitest-alias.test.mjs index 881ca11af..5d93db5e7 100644 --- a/scripts/__tests__/cli-vitest-alias.test.mjs +++ b/scripts/__tests__/cli-vitest-alias.test.mjs @@ -20,13 +20,33 @@ import cliVitestConfig from '../../packages/cli/vitest.config.ts' const aliases = cliVitestConfig.resolve.alias +/** + * Vite also accepts the array form (`[{ find, replacement }]`). This guard + * reads the object-of-strings form; on the array form every check below would + * throw `TypeError: target.endsWith is not a function` from a helper, which + * reads as "the guard is broken" rather than "the config changed shape". + * Fail here instead, naming the offender. + */ +const nonStringTargets = Object.entries(aliases).filter( + ([, target]) => typeof target !== 'string', +) + /** Directory aliases are written with a trailing slash (`'@/'`). */ -const targets = Object.entries(aliases).map(([specifier, target]) => [ - specifier, - target.endsWith('/') ? target.slice(0, -1) : target, -]) +const targets = Object.entries(aliases) + .filter(([, target]) => typeof target === 'string') + .map(([specifier, target]) => [ + specifier, + target.endsWith('/') ? target.slice(0, -1) : target, + ]) describe('packages/cli vitest alias map', () => { + it('is the object-of-strings form this guard can read', () => { + expect( + nonStringTargets.map(([specifier]) => specifier), + 'packages/cli/vitest.config.ts switched away from the object-of-strings alias form. Update this guard to walk the new shape — do not delete it.', + ).toEqual([]) + }) + it('still aliases @cipherstash/migrate to source', () => { // migrate publishes `./dist` only. Drop this entry — or re-introduce an // `importOriginal()` spread that needs the built package — and the unit diff --git a/scripts/__tests__/lib/read-jsonc.mjs b/scripts/__tests__/lib/read-jsonc.mjs new file mode 100644 index 000000000..3ecff8b80 --- /dev/null +++ b/scripts/__tests__/lib/read-jsonc.mjs @@ -0,0 +1,49 @@ +import { readFileSync } from 'node:fs' + +/** + * Parse a JSONC file (`turbo.json` and friends allow comments and trailing + * commas; `JSON.parse` does not). + * + * String-aware by necessity, not fussiness: `turbo.json` opens with + * `"$schema": "https://turbo.build/schema.json"`, so a regex that strips `//` + * without tracking string state eats the URL and yields invalid JSON — or, + * worse, silently valid JSON with the wrong contents. + * + * Extracted from `turbo-skills-inputs.test.mjs` when a second guard needed it. + * Two hand-rolled copies of this would drift, and a subtly wrong copy fails as + * a confusing `SyntaxError` at a byte offset rather than as a clear defect. + */ +export function readJsonc(path) { + const raw = readFileSync(path, 'utf8') + let out = '' + let inString = false + let escaped = false + for (let i = 0; i < raw.length; i++) { + const ch = raw[i] + if (inString) { + out += ch + if (escaped) escaped = false + else if (ch === '\\') escaped = true + else if (ch === '"') inString = false + continue + } + if (ch === '"') { + inString = true + out += ch + continue + } + if (ch === '/' && raw[i + 1] === '/') { + while (i < raw.length && raw[i] !== '\n') i++ + out += '\n' + continue + } + if (ch === '/' && raw[i + 1] === '*') { + i += 2 + while (i < raw.length && !(raw[i] === '*' && raw[i + 1] === '/')) i++ + i++ + continue + } + out += ch + } + return JSON.parse(out.replace(/,(\s*[}\]])/g, '$1')) +} diff --git a/scripts/__tests__/turbo-skills-inputs.test.mjs b/scripts/__tests__/turbo-skills-inputs.test.mjs index 7706ca767..a2413a121 100644 --- a/scripts/__tests__/turbo-skills-inputs.test.mjs +++ b/scripts/__tests__/turbo-skills-inputs.test.mjs @@ -21,45 +21,10 @@ import { existsSync, readdirSync, readFileSync } from 'node:fs' import { join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' +import { readJsonc } from './lib/read-jsonc.mjs' const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') -/** Strip comments so `JSON.parse` accepts turbo.json (it is JSONC). */ -function readJsonc(path) { - const raw = readFileSync(path, 'utf8') - let out = '' - let inString = false - let escaped = false - for (let i = 0; i < raw.length; i++) { - const ch = raw[i] - if (inString) { - out += ch - if (escaped) escaped = false - else if (ch === '\\') escaped = true - else if (ch === '"') inString = false - continue - } - if (ch === '"') { - inString = true - out += ch - continue - } - if (ch === '/' && raw[i + 1] === '/') { - while (i < raw.length && raw[i] !== '\n') i++ - out += '\n' - continue - } - if (ch === '/' && raw[i + 1] === '*') { - i += 2 - while (i < raw.length && !(raw[i] === '*' && raw[i + 1] === '/')) i++ - i++ - continue - } - out += ch - } - return JSON.parse(out.replace(/,(\s*[}\]])/g, '$1')) -} - /** Packages whose tsup config copies the repo-root `skills/` into `dist/`. */ function packagesCopyingSkills() { const pkgsDir = join(REPO_ROOT, 'packages') diff --git a/scripts/__tests__/workflow-turbo-build-deps.test.mjs b/scripts/__tests__/workflow-turbo-build-deps.test.mjs new file mode 100644 index 000000000..1de2124d1 --- /dev/null +++ b/scripts/__tests__/workflow-turbo-build-deps.test.mjs @@ -0,0 +1,166 @@ +import { readFileSync } from 'node:fs' +import { resolve } from 'node:path' +import { fileURLToPath } from 'node:url' +import yaml from 'js-yaml' +import { describe, expect, it } from 'vitest' +import { readJsonc } from './lib/read-jsonc.mjs' + +/** + * A turbo task declaring `dependsOn: ["^build"]` gets its workspace + * dependencies built before it runs — but ONLY when turbo is the one invoking + * it. Run the same task as a bare package script (`pnpm --filter run + * `) and the dependency graph is skipped silently. + * + * That failure mode is invisible in CI, because a bare step usually still + * passes: some earlier step in the same job built the workspace via its own + * `^build`. The step is then load-bearing on an ordering nobody declared. + * Reorder or delete that earlier step — and they read as independent guards + * for other packages, so they look freely removable — and the bare step fails + * with a module-resolution error (`TS2307`, `Failed to resolve entry for + * package`) that reads as "the code is broken" rather than "you skipped the + * build". + * + * This is not hypothetical: #787 added `typecheck:scaffold` as a bare + * `pnpm --filter stash run typecheck:scaffold` and it went green for exactly + * that reason. The fixtures it typechecks import `@cipherstash/stack/v3`, so + * the step needs that package built; it was routed through turbo in review. + * This test pins that routing so it cannot be quietly "simplified" back. + */ + +const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') +const WORKFLOW = '.github/workflows/tests.yml' + +/** + * Bare invocations that predate this guard. Each is the same latent trap: it + * passes today only because an earlier step in its job builds the workspace. + * They are recorded rather than ignored so the list can be worked down — do + * not add to it. Route new steps through `turbo run` instead. + */ +const KNOWN_BARE = new Set([ + 'pnpm --filter @cipherstash/prisma-next run typecheck', + 'pnpm --filter @cipherstash/wizard run typecheck', +]) + +const turboJson = readJsonc(resolve(REPO_ROOT, 'turbo.json')) +const rootScripts = + JSON.parse(readFileSync(resolve(REPO_ROOT, 'package.json'), 'utf8')) + .scripts ?? {} + +/** Turbo tasks that build their workspace dependencies first. */ +const buildDependentTasks = new Set( + Object.entries(turboJson.tasks ?? {}) + .filter(([, task]) => (task?.dependsOn ?? []).includes('^build')) + .map(([name]) => name), +) + +/** pnpm's own verbs — never package scripts. */ +const PNPM_VERBS = new Set([ + 'install', + 'exec', + 'dlx', + 'add', + 'why', + 'store', + 'run', +]) + +/** + * The task a command line runs, and whether turbo is the thing running it. + * Returns null when the line runs no recognisable task. + * + * Two shapes matter: + * `pnpm exec turbo run --filter ` / `pnpm turbo ` → routed + * `pnpm [--filter ] [run] ` → bare + */ +function invokedTask(line) { + const turbo = line.match(/\bturbo\b\s+(?:run\s+)?([\w:.-]+)/) + if (turbo) return { task: turbo[1], routed: true } + + const pnpm = line.match( + /\bpnpm\b(?:\s+(?:--filter|-F)\s+\S+|\s+--if-present)*\s+(?:run\s+)?([\w:.-]+)/, + ) + if (!pnpm) return null + const task = pnpm[1] + if (PNPM_VERBS.has(task)) return null + return { task, routed: false } +} + +/** Root scripts may delegate to turbo themselves (`"test": "turbo test ..."`). */ +const rootScriptDelegatesToTurbo = (task) => + typeof rootScripts[task] === 'string' && /\bturbo\b/.test(rootScripts[task]) + +function workflowRunLines(path) { + const doc = yaml.load(readFileSync(resolve(REPO_ROOT, path), 'utf8')) + const lines = [] + for (const [jobName, job] of Object.entries(doc?.jobs ?? {})) { + for (const step of job?.steps ?? []) { + if (typeof step?.run !== 'string') continue + for (const raw of step.run.split('\n')) { + const line = raw.trim() + if (line) lines.push({ jobName, stepName: step.name, line }) + } + } + } + return lines +} + +describe('tests.yml routes build-dependent turbo tasks through turbo', () => { + it('turbo.json declares the tasks this guard protects', () => { + // If `^build` is dropped from these, the guard below silently stops + // checking anything — assert the premise rather than trusting it. + expect(buildDependentTasks).toContain('typecheck:scaffold') + expect(buildDependentTasks).toContain('typecheck') + expect(buildDependentTasks).toContain('build') + }) + + it('typecheck:scaffold is invoked via turbo, never bare', () => { + const invocations = workflowRunLines(WORKFLOW).filter( + ({ line }) => invokedTask(line)?.task === 'typecheck:scaffold', + ) + + // A bare `pnpm --filter stash run typecheck:scaffold` matches + // `invokedTask` but not `routedThroughTurbo`, so it would fail here. + // Deleting the step entirely is caught by this length assertion. + expect( + invocations.length, + `${WORKFLOW} must run typecheck:scaffold — the scaffold fixtures are only typechecked there`, + ).toBeGreaterThan(0) + + for (const { jobName, stepName, line } of invocations) { + expect( + invokedTask(line).routed, + `${WORKFLOW} job "${jobName}" step "${stepName}" runs typecheck:scaffold without turbo:\n ${line}\nThe scaffold fixtures import @cipherstash/stack/v3, so this needs that package BUILT. Use \`pnpm exec turbo run typecheck:scaffold --filter stash\`.`, + ).toBe(true) + } + }) + + it('no new bare invocation of a build-dependent turbo task', () => { + const offenders = workflowRunLines(WORKFLOW) + .filter(({ line }) => { + const invoked = invokedTask(line) + if (!invoked || invoked.routed) return false + if (!buildDependentTasks.has(invoked.task)) return false + if (rootScriptDelegatesToTurbo(invoked.task)) return false + return !KNOWN_BARE.has(line) + }) + .map(({ jobName, stepName, line }) => `${jobName} / ${stepName}: ${line}`) + + expect( + offenders, + `These steps run a turbo task declaring \`dependsOn: ["^build"]\` without turbo, so nothing guarantees their workspace dependencies are built. They pass only while an earlier step in the same job happens to build them. Route them through \`pnpm exec turbo run --filter \`.`, + ).toEqual([]) + }) + + it('the grandfathered bare invocations still exist as written', () => { + // KNOWN_BARE entries are matched by exact string. If a step is reworded or + // fixed, its entry goes stale and silently exempts nothing — or worse, + // masks a different line later. Fail so the list gets pruned. + const lines = new Set(workflowRunLines(WORKFLOW).map(({ line }) => line)) + for (const bare of KNOWN_BARE) { + expect( + lines.has(bare), + `KNOWN_BARE entry is stale — no step in ${WORKFLOW} runs:\n ${bare}\nRemove it from the allowlist.`, + ).toBe(true) + } + }) +}) From 937c6f3663fb435949499181a63dc576f75965e3 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Mon, 27 Jul 2026 14:27:33 +1000 Subject: [PATCH 6/6] test(cli,ci): close the coverage and guard gaps found auditing #787 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the #787 review. Five gaps, none raised by the reviewer, all found by auditing what the PR's own tests actually pin. A guard clause that no test held. `context-placeholder.test.ts` named a case it did not construct: its fixture returned `getEncryptConfig: () => ({})`, so `configuredTables` was empty and the guard short-circuited on the arity check before the sentinel-name comparison ever ran. Deleting `configuredTables.length === 1` left all 891 tests green — measured, not argued. The fixture now declares the sentinel FIRST alongside a real table, because the guard indexes `[0]`, and both tests spy `process.exit` so a regression fails as an assertion instead of killing the worker. One guard, not two. The same refusal was hand-copied into `loadEncryptConfig` and `loadEncryptionContext`, sharing only the constant — and it had already drifted: on a client whose `getEncryptConfig()` returns nothing, `db push` named the cause while `encrypt backfill` fell through to `Table "..." was not found`, the symptom-not-cause message the guard exists to replace. Both now call `requireUsableEncryptConfig`, pinned by a parity test asserting the two seams emit byte-identical text. The resolver/command seam, tested. `encrypt-v3.test.ts` stubs `resolveColumnLifecycle` and hand-writes its return value; `resolve-eql.test.ts` proves the pure-v2 shape produces that value. Nothing ran the real producer into the real consumer. The new composition test mocks no resolution at all — only pg, fs, prompts and the effectful migrate helpers — so the v2 verdict is derived from a real manifest hint and a real catalog read. Mutation-checked: the two #787 defences are independent, so removing either alone is masked by the other; removing both fails this test and nothing else. Its sibling types are now imported rather than re-declared, so the shape cannot drift silently. The workflow guard, applied to all workflows. It scanned only `tests.yml` while enforcing a rule its own docblock generalises. Widened to all 13, which surfaced six real bare invocations of `^build`-dependent tasks — five `test:integration`, plus a `test:e2e` that `rootScriptDelegatesToTurbo` was wrongly exempting (a `--filter` invocation runs the PACKAGE's script, so the root script's delegation says nothing about it). All six routed through turbo with `--env-mode=loose`: turbo defaults to strict and would otherwise withhold the step env these suites need, which `passThroughEnv` could only fix by enumerating ten variables across workflows this change cannot execute. Also corrects `AGENTS.md`'s path to `introspect.test.ts` and states the `stackSourceAlias` collision symmetrically — spreading it either way breaks one package's `'@/'`. Its "fails 10 files" figure was re-measured by deleting `packages/stack/dist`: exactly 10. `stash` unit suite 896/896, scripts 112/112, typecheck:scaffold clean, Biome 0 errors. --- .changeset/encrypt-client-guard-parity.md | 20 ++ .github/workflows/integration-drizzle.yml | 10 +- .github/workflows/integration-prisma-next.yml | 2 +- .github/workflows/integration-supabase.yml | 4 +- .github/workflows/prisma-next-e2e.yml | 2 +- packages/cli/AGENTS.md | 10 +- .../placeholder-guard-parity.test.ts | 121 +++++++++++ .../__tests__/context-placeholder.test.ts | 33 ++- .../encrypt/__tests__/encrypt-v3.test.ts | 16 +- ...-lifecycle-composition.integration.test.ts | 202 ++++++++++++++++++ packages/cli/src/commands/encrypt/context.ts | 33 +-- packages/cli/src/config/index.ts | 34 ++- .../workflow-turbo-build-deps.test.mjs | 85 ++++++-- 13 files changed, 508 insertions(+), 64 deletions(-) create mode 100644 .changeset/encrypt-client-guard-parity.md create mode 100644 packages/cli/src/__tests__/placeholder-guard-parity.test.ts create mode 100644 packages/cli/src/commands/encrypt/__tests__/v2-lifecycle-composition.integration.test.ts diff --git a/.changeset/encrypt-client-guard-parity.md b/.changeset/encrypt-client-guard-parity.md new file mode 100644 index 000000000..63c3a2664 --- /dev/null +++ b/.changeset/encrypt-client-guard-parity.md @@ -0,0 +1,20 @@ +--- +'stash': patch +--- + +`stash encrypt backfill` now names the cause when the encryption client has no +initialized encrypt config, instead of reporting a missing table. + +The guard that refuses an unusable client file existed twice — once in +`loadEncryptConfig` (`stash db push` / `db validate`) and once, hand-copied, in +`loadEncryptionContext` (`stash encrypt backfill`). The copies had already +drifted: for a client whose `getEncryptConfig()` returns nothing, `db push` +exited 1 with `Encryption client in has no initialized encrypt config`, +while `encrypt backfill` fell through to `Table "users" was not found in the +encryption client exports. Available: (none)` — naming the symptom rather than +the cause, which is precisely the failure the guard was added to eliminate. + +Both loaders now call one shared guard, so a single file cannot produce two +different diagnoses, and the refusals are pinned at both public entry points. +The un-replaced `stash init` scaffold is unaffected — it was already refused by +both, with the same message. diff --git a/.github/workflows/integration-drizzle.yml b/.github/workflows/integration-drizzle.yml index 7dd8310ba..c4b3e7669 100644 --- a/.github/workflows/integration-drizzle.yml +++ b/.github/workflows/integration-drizzle.yml @@ -153,8 +153,14 @@ jobs: # # Step env, not a `.env` file: `dotenv/config` does not override an # already-set `process.env`, so these win and no secret is written to disk. + # Through turbo, not a bare `pnpm --filter`: `test:integration` declares + # `dependsOn: ["^build", "build"]`, and the only build in this job is the + # setup action's `--filter stash`, which reaches these packages purely by + # coincidence of stash's own dependency graph. `--env-mode=loose` because + # turbo defaults to strict and would otherwise withhold the step env below + # from the test process (#787 review follow-up). - name: Drizzle v3 integration suites - run: pnpm --filter @cipherstash/stack-drizzle run test:integration + run: pnpm exec turbo run test:integration --filter @cipherstash/stack-drizzle --env-mode=loose env: DATABASE_URL: ${{ steps.db.outputs.database-url }} PGRST_URL: ${{ steps.db.outputs.pgrest-url }} @@ -164,7 +170,7 @@ jobs: # `isInstalled` short-circuits against the DB the first invocation already # provisioned — a fast no-op check, not a second schema apply. - name: Shared core + identity + wasm integration suites - run: pnpm --filter @cipherstash/stack run test:integration + run: pnpm exec turbo run test:integration --filter @cipherstash/stack --env-mode=loose env: DATABASE_URL: ${{ steps.db.outputs.database-url }} PGRST_URL: ${{ steps.db.outputs.pgrest-url }} diff --git a/.github/workflows/integration-prisma-next.yml b/.github/workflows/integration-prisma-next.yml index acfb99b64..6b7e19134 100644 --- a/.github/workflows/integration-prisma-next.yml +++ b/.github/workflows/integration-prisma-next.yml @@ -114,7 +114,7 @@ jobs: # `stash eql install --eql-version 3`, so an installer regression fails # here rather than hiding behind a test-only SQL apply. - name: prisma-next v3 family suites - run: pnpm --filter @cipherstash/prisma-next run test:integration + run: pnpm exec turbo run test:integration --filter @cipherstash/prisma-next --env-mode=loose env: # Step env, not a `.env` file: `dotenv/config` does not override an # already-set `process.env`, so these win and no secret hits disk. diff --git a/.github/workflows/integration-supabase.yml b/.github/workflows/integration-supabase.yml index 98b281655..f5eff3093 100644 --- a/.github/workflows/integration-supabase.yml +++ b/.github/workflows/integration-supabase.yml @@ -129,7 +129,7 @@ jobs: # Step env, not a `.env` file: `dotenv/config` does not override an # already-set `process.env`, so these win and no secret is written to disk. - name: Supabase v3 integration suites - run: pnpm --filter @cipherstash/stack-supabase run test:integration + run: pnpm exec turbo run test:integration --filter @cipherstash/stack-supabase --env-mode=loose env: DATABASE_URL: ${{ steps.db.outputs.database-url }} PGRST_URL: ${{ steps.db.outputs.pgrest-url }} @@ -139,7 +139,7 @@ jobs: # `isInstalled` short-circuits against the DB the first invocation already # provisioned — so this is a fast no-op check, not a second schema apply. - name: Shared core integration suites (against Supabase Postgres) - run: pnpm --filter @cipherstash/stack run test:integration + run: pnpm exec turbo run test:integration --filter @cipherstash/stack --env-mode=loose env: DATABASE_URL: ${{ steps.db.outputs.database-url }} PGRST_URL: ${{ steps.db.outputs.pgrest-url }} diff --git a/.github/workflows/prisma-next-e2e.yml b/.github/workflows/prisma-next-e2e.yml index 8c2e260ae..1455cad60 100644 --- a/.github/workflows/prisma-next-e2e.yml +++ b/.github/workflows/prisma-next-e2e.yml @@ -121,7 +121,7 @@ jobs: done - name: Run E2E suite - run: pnpm --filter @cipherstash/prisma-next-example test:e2e + run: pnpm exec turbo run test:e2e --filter @cipherstash/prisma-next-example --env-mode=loose - name: Stop E2E Postgres container if: always() diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index ced2efbff..aa65e4d00 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -18,10 +18,12 @@ workspace fails at collection with `Failed to resolve entry for package …` rather than at an assertion. `vitest.config.ts` aliases `@cipherstash/migrate` to its source to remove one such coupling; `@cipherstash/stack` remains, reached via `packages/migrate/src/backfill.ts` and a direct import in -`init/lib/introspect.test.ts`. Deleting `packages/stack/dist` fails 10 files. -Closing it needs `vitest.shared.ts`'s `stackSourceAlias`, which cannot be spread -into this config — its `'@/'` entry points at `packages/stack/src` and would -clobber this package's own `'@/'` (#787 review). +`init/lib/__tests__/introspect.test.ts`. Deleting `packages/stack/dist` fails 10 +files. Closing it needs `vitest.shared.ts`'s `stackSourceAlias`, which cannot be +spread into this config: its `'@/'` points at `packages/stack/src` while this +package's points at `packages/cli/src`, and a flat alias map admits only one — +spread it after and stack's entry clobbers the CLI's, spread it before and the +CLI's breaks stack's own source imports (#787 review). ## When to add or update an E2E test diff --git a/packages/cli/src/__tests__/placeholder-guard-parity.test.ts b/packages/cli/src/__tests__/placeholder-guard-parity.test.ts new file mode 100644 index 000000000..80563e9c2 --- /dev/null +++ b/packages/cli/src/__tests__/placeholder-guard-parity.test.ts @@ -0,0 +1,121 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { PLACEHOLDER_TABLE_NAME } from '@/config/index.js' + +/** + * `stash db push` / `db validate` reach the user's encryption client through + * `loadEncryptConfig`; `stash encrypt backfill` reaches the same file through + * `loadEncryptionContext`. Both must refuse the un-replaced `stash init` + * scaffold, and — because it is one file and one mistake — both must say the + * same thing about it. + * + * The two guards were hand-copied rather than shared, so nothing held them + * together: the message text could drift on one side, and the nullish-config + * case HAD already drifted (#787 review follow-up). This pins the agreement at + * both public seams rather than testing the shared helper directly, which + * would prove only that a function calls itself. + * + * Real jiti, real temp project, real filesystem — the only doubles are + * `process.exit` and `console.error`. + */ +describe('the un-replaced init scaffold, refused identically by both loaders', () => { + let tmpDir: string + let originalCwd: () => string + + const writeProject = (clientBody: string) => { + fs.writeFileSync( + path.join(tmpDir, 'stash.config.ts'), + `export default { + databaseUrl: 'postgresql://u:p@127.0.0.1:5432/db', + client: './client.ts', + }`, + ) + fs.writeFileSync(path.join(tmpDir, 'client.ts'), clientBody) + process.cwd = () => tmpDir + } + + /** The duck-typed table shape `loadEncryptionContext` harvests. */ + const table = (name: string) => + `{ tableName: '${name}', build: () => ({ tableName: '${name}', columns: {} }) }` + + /** Run one loader, capturing whatever it wrote to stderr before exiting. */ + const captureRefusal = async (load: () => Promise) => { + const error = vi.spyOn(console, 'error').mockImplementation(() => {}) + const exit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit') + }) + let exited = false + try { + await load() + } catch (err) { + exited = (err as Error).message === 'process.exit' + if (!exited) throw err + } + const message = error.mock.calls.flat().join('\n') + error.mockRestore() + exit.mockRestore() + return { exited, message } + } + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'stash-guard-parity-')) + originalCwd = process.cwd + }) + + afterEach(() => { + process.cwd = originalCwd + vi.restoreAllMocks() + if (tmpDir && fs.existsSync(tmpDir)) { + fs.rmSync(tmpDir, { recursive: true, force: true }) + } + }) + + it('gives db push and encrypt backfill the same refusal for the same file', async () => { + writeProject( + `export const encryptionClient = { + getEncryptConfig: () => ({ tables: { '${PLACEHOLDER_TABLE_NAME}': {} } }), + } + export const placeholderTable = ${table(PLACEHOLDER_TABLE_NAME)}`, + ) + + const { loadEncryptConfig } = await import('@/config/index.js') + const dbPush = await captureRefusal(() => loadEncryptConfig('./client.ts')) + + const { loadEncryptionContext } = await import( + '../commands/encrypt/context.js' + ) + const backfill = await captureRefusal(() => loadEncryptionContext()) + + expect(dbPush.exited).toBe(true) + expect(backfill.exited).toBe(true) + expect(dbPush.message).toContain('still contains the placeholder table') + expect(backfill.message).toEqual(dbPush.message) + }) + + it('names the cause, not the symptom, when the client has no encrypt config', async () => { + // A client whose `getEncryptConfig()` returns nothing is the same class of + // un-finished setup. `db push` already named it; `encrypt backfill` used to + // fall through to `requireTable`'s `Table "users" was not found … + // Available: (none)` — the symptom-not-cause message this guard exists to + // replace (#787 review follow-up). + writeProject( + `export const encryptionClient = { getEncryptConfig: () => undefined } + export const users = ${table('users')}`, + ) + + const { loadEncryptConfig } = await import('@/config/index.js') + const dbPush = await captureRefusal(() => loadEncryptConfig('./client.ts')) + + const { loadEncryptionContext } = await import( + '../commands/encrypt/context.js' + ) + const backfill = await captureRefusal(() => loadEncryptionContext()) + + expect(dbPush.exited).toBe(true) + expect(backfill.exited).toBe(true) + expect(backfill.message).toContain('no initialized encrypt config') + expect(backfill.message).not.toContain('was not found') + }) +}) diff --git a/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts b/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts index f1dacb93a..9a0ad2440 100644 --- a/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts +++ b/packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts @@ -118,24 +118,53 @@ describe('loadEncryptionContext — the un-replaced init scaffold', () => { export const placeholderTable = ${table(PLACEHOLDER_TABLE_NAME)}`, ) + const error = vi.spyOn(console, 'error').mockImplementation(() => {}) + const exit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit') + }) + const { loadEncryptionContext } = await import('../context.js') const ctx = await loadEncryptionContext() + // The silence this test is named for. Without the `exit` spy the guard + // firing here would kill the worker instead of failing an assertion. + expect(exit).not.toHaveBeenCalled() + expect(error.mock.calls.flat().join('\n')).not.toContain( + 'still contains the placeholder table', + ) expect(ctx.tables.has(PLACEHOLDER_TABLE_NAME)).toBe(true) }) it('allows the sentinel through once a real table sits alongside it', async () => { // Only the SOLE-placeholder case is the un-replaced scaffold. A user who - // has added real tables must not be blocked by a leftover sentinel export. + // has added real tables must not be blocked by a leftover sentinel. + // + // The sentinel is declared FIRST in the config deliberately: the guard + // reads `configuredTables[0]`, so a placeholder-last fixture passes even + // with the arity check deleted. Ordered this way, dropping + // `configuredTables.length === 1` makes this test fail — which is the only + // thing that pins that clause anywhere in the suite (#787 review follow-up). writeProject( - `export const encryptionClient = { getEncryptConfig: () => ({}) } + `export const encryptionClient = { + getEncryptConfig: () => ({ + tables: { '${PLACEHOLDER_TABLE_NAME}': {}, users: {} }, + }), + } export const placeholderTable = ${table(PLACEHOLDER_TABLE_NAME)} export const users = ${table('users')}`, ) + const error = vi.spyOn(console, 'error').mockImplementation(() => {}) + const exit = vi.spyOn(process, 'exit').mockImplementation(() => { + throw new Error('process.exit') + }) const { loadEncryptionContext } = await import('../context.js') const ctx = await loadEncryptionContext() + expect(exit).not.toHaveBeenCalled() + expect(error.mock.calls.flat().join('\n')).not.toContain( + 'still contains the placeholder table', + ) expect(ctx.tables.has('users')).toBe(true) }) }) diff --git a/packages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.ts b/packages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.ts index a0d583294..2b8255e79 100644 --- a/packages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.ts +++ b/packages/cli/src/commands/encrypt/__tests__/encrypt-v3.test.ts @@ -1,4 +1,9 @@ +import type { + EncryptedColumnInfo, + ResolvedEncryptedColumn, +} from '@cipherstash/migrate' import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { ResolvedLifecycle } from '../lib/resolve-eql.js' // The EQL-v3 lifecycle branches (#648/#649): v3 has no cut-over rename — // `encrypt cutover` must short-circuit with guidance instead of running the @@ -23,9 +28,14 @@ vi.mock('pg', () => ({ }, })) -type ColumnInfo = { column: string; domain: string; version: 2 | 3 } -type ResolvedInfo = ColumnInfo & { via: 'hint' | 'convention' | 'sole' } -type Lifecycle = { info: ResolvedInfo | null; candidates: ColumnInfo[] } +// Imported, never re-declared. These stubs stand in for the real +// `resolveColumnLifecycle`, so a structural copy would let a renamed or added +// field compile on both sides while the commands read something the resolver no +// longer returns — the exact seam `v2-lifecycle-composition.integration.test.ts` +// exercises at runtime, pinned here at compile time (#787 review follow-up). +type ColumnInfo = EncryptedColumnInfo +type ResolvedInfo = ResolvedEncryptedColumn +type Lifecycle = ResolvedLifecycle const migrateMocks = vi.hoisted(() => ({ listEncryptedColumns: vi.fn(async (): Promise => []), diff --git a/packages/cli/src/commands/encrypt/__tests__/v2-lifecycle-composition.integration.test.ts b/packages/cli/src/commands/encrypt/__tests__/v2-lifecycle-composition.integration.test.ts new file mode 100644 index 000000000..15ea2b01d --- /dev/null +++ b/packages/cli/src/commands/encrypt/__tests__/v2-lifecycle-composition.integration.test.ts @@ -0,0 +1,202 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +/** + * The seam between lifecycle RESOLUTION and the commands that act on it. + * + * `encrypt-v3.test.ts` stubs `resolveColumnLifecycle` and hand-writes the value + * it returns; `resolve-eql.test.ts` separately proves a pure-v2 table produces + * that value. Both can stay green while the shape passed between them changes, + * because neither runs the real producer into the real consumer — and the + * commands' v2/v3 branch is chosen entirely from that shape. + * + * So here `resolve-eql.js` is NOT mocked at all: `resolveColumnLifecycle`, + * `explainUnresolved`, `pickEncryptedColumn`, `listEncryptedColumns` and + * `columnExists` all run for real, and the pure-v2 verdict is derived from a + * real manifest hint plus a real catalog read rather than asserted into + * existence. Only genuine boundaries are faked: `pg`, the filesystem, prompts, + * and the effectful migrate helpers that write to the database (#787 review + * follow-up). + */ + +const queryMock = vi.hoisted(() => + vi.fn(async (_sql: string, _params?: unknown[]) => ({ + rows: [] as unknown[], + })), +) +vi.mock('pg', () => ({ + default: { + Client: class { + connect = vi.fn(async () => {}) + end = vi.fn(async () => {}) + query = queryMock + }, + }, +})) + +/** + * Spread the real module and override only the functions that TOUCH something — + * the manifest file and the database. Everything resolution actually reasons + * with (`listEncryptedColumns`, `columnExists`, `pickEncryptedColumn`, + * `classifyEqlDomain`) stays real and runs against `queryMock` below. + */ +const migrateMocks = vi.hoisted(() => ({ + readManifest: vi.fn(async () => null as unknown), + countUnencrypted: vi.fn(async () => 0), + progress: vi.fn( + async () => ({ phase: 'cut-over' }) as { phase: string } | null, + ), + appendEvent: vi.fn(async () => {}), + setManifestTargetPhase: vi.fn(async () => {}), + renameEncryptedColumns: vi.fn(async () => {}), + migrateConfig: vi.fn(async () => {}), + activateConfig: vi.fn(async () => {}), + reloadConfig: vi.fn(async () => {}), +})) +vi.mock('@cipherstash/migrate', async (importOriginal) => ({ + ...(await importOriginal()), + ...migrateMocks, +})) + +vi.mock('@clack/prompts', () => ({ + intro: vi.fn(), + outro: vi.fn(), + confirm: vi.fn(async () => true), + isCancel: vi.fn(() => false), + log: { + info: vi.fn(), + success: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + step: vi.fn(), + }, + note: vi.fn(), +})) +vi.mock('@/config/index.js', () => ({ + loadStashConfig: vi.fn(async () => ({ databaseUrl: 'postgres://test' })), +})) +vi.mock('@/commands/db/detect.js', () => ({ + detectDrizzle: vi.fn(() => false), +})) +vi.mock('@/commands/init/utils.js', () => ({ + detectPackageManager: vi.fn(() => 'npm'), + runnerCommand: vi.fn((_pm: string, ref: string) => `npx ${ref}`), +})) +vi.mock('../drizzle-helper.js', () => ({ + scaffoldDrizzleMigration: vi.fn(async ({ name }: { name: string }) => ({ + path: `drizzle/${name}.sql`, + })), +})) +const writeFileMock = vi.hoisted(() => vi.fn()) +vi.mock('node:fs', () => ({ + default: { mkdirSync: vi.fn(), writeFileSync: writeFileMock }, +})) + +import * as p from '@clack/prompts' +import { cutoverCommand } from '../cutover.js' +import { dropCommand } from '../drop.js' + +function spyExit() { + return vi + .spyOn(process, 'exit') + .mockImplementation((() => undefined) as never) +} + +/** Columns the faked catalog reports as EQL-domain typed. Empty = pure v2. */ +let eqlColumns: { column: string; domain_name: string }[] = [] +/** Columns the faked catalog reports as existing at all. */ +let existingColumns: string[] = [] + +/** + * Route on the catalog statements these paths issue. Each is matched on text + * unique to it, and ORDER MATTERS: three of them end in `AS exists`, and + * `columnExists` and `listEncryptedColumns` both embed the same shared + * `to_regclass` expression. Matching loosely silently answers the wrong probe — + * routing the v2 pending-config check into `columnExists` made cutover report + * "no pending EQL configuration" instead of running the ladder. + */ +function fakeCatalog() { + queryMock.mockImplementation(async (sql: string, params?: unknown[]) => { + if (typeof sql !== 'string') return { rows: [] } + // The v2 config machine exists… + if (sql.includes("to_regclass('public.eql_v2_configuration')")) { + return { rows: [{ exists: 'eql_v2_configuration' }] } + } + // …and holds a pending row to promote. + if (sql.includes("state = 'pending'")) return { rows: [{ exists: true }] } + if (sql.includes('typname AS domain_name')) return { rows: eqlColumns } + if (sql.includes('pg_attribute') && sql.includes('AS exists')) { + return { + rows: [{ exists: existingColumns.includes(String(params?.[2])) }], + } + } + return { rows: [] } + }) +} + +beforeEach(() => { + vi.clearAllMocks() + // The hint `encrypt backfill` records for EVERY column, v2 included. It is + // what used to be discarded into a guess, and what must now be ignored + // harmlessly on a table with no v3 columns to mis-claim. + migrateMocks.readManifest.mockResolvedValue({ + tables: { + users: [{ column: 'ssn', encryptedColumn: 'ssn_encrypted' }], + }, + }) + migrateMocks.progress.mockResolvedValue({ phase: 'cut-over' }) + eqlColumns = [] + existingColumns = ['ssn', 'ssn_encrypted'] + fakeCatalog() +}) + +describe('the pure-v2 lifecycle, resolved for real end to end', () => { + it('generates the v2 plaintext drop for a table whose encrypted column is legacy v2', async () => { + const exitSpy = spyExit() + + await dropCommand({ table: 'users', column: 'ssn' }) + + expect(exitSpy).not.toHaveBeenCalled() + + // Reached the v2 ladder: `_plaintext`, not the v3 target ``. + const sql = writeFileMock.mock.calls[0]?.[1] as string + expect(sql).toContain('DROP COLUMN "ssn_plaintext"') + // The v3-only coverage gate must not run on a v2 column. + expect(migrateMocks.countUnencrypted).not.toHaveBeenCalled() + }) + + it('runs the v2 rename and config promotion for a table with no EQL v3 columns', async () => { + migrateMocks.progress.mockResolvedValue({ phase: 'backfilled' }) + + const exitSpy = spyExit() + + await cutoverCommand({ table: 'users', column: 'ssn' }) + + expect(exitSpy).not.toHaveBeenCalled() + expect(migrateMocks.renameEncryptedColumns).toHaveBeenCalled() + expect(migrateMocks.migrateConfig).toHaveBeenCalled() + expect(migrateMocks.activateConfig).toHaveBeenCalled() + expect(p.log.error).not.toHaveBeenCalled() + }) + + it('refuses both commands when the table also holds an unidentifiable EQL v3 column', async () => { + // The mixed table from #772 finding 7. Same manifest hint, same v2 pair — + // the ONLY difference is an unrelated v3 column, which is what makes the + // recorded pairing meaningful and a fall-through a guess. Crossing this + // boundary is what neither existing half of the coverage can see. + eqlColumns = [{ column: 'email_enc', domain_name: 'eql_v3_text_search' }] + const exitSpy = spyExit() + + await dropCommand({ table: 'users', column: 'ssn' }) + await cutoverCommand({ table: 'users', column: 'ssn' }) + + // Refusing is the point: both must exit non-zero rather than act on + // `email_enc`, the unrelated v3 column the sole-EQL-column rule would + // otherwise claim. + expect(exitSpy).toHaveBeenCalledWith(1) + expect(writeFileMock).not.toHaveBeenCalled() + expect(migrateMocks.renameEncryptedColumns).not.toHaveBeenCalled() + const errors = vi.mocked(p.log.error).mock.calls.flat().join('\n') + expect(errors).toContain('is not an EQL v3 column') + expect(errors).toContain('ssn_encrypted') + }) +}) diff --git a/packages/cli/src/commands/encrypt/context.ts b/packages/cli/src/commands/encrypt/context.ts index 0eab28db1..682328eed 100644 --- a/packages/cli/src/commands/encrypt/context.ts +++ b/packages/cli/src/commands/encrypt/context.ts @@ -3,8 +3,8 @@ import path from 'node:path' import type { EncryptionClient } from '@cipherstash/stack/encryption' import { loadStashConfig, - PLACEHOLDER_TABLE_NAME, type ResolvedStashConfig, + requireUsableEncryptConfig, } from '@/config/index.js' /** @@ -142,30 +142,13 @@ export async function loadEncryptionContext(): Promise { process.exit(1) } - // The guard `loadEncryptConfig` applies for `stash db push` / `db validate`, - // repeated here because `stash encrypt` does not go through that loader. The - // scaffold `stash init` writes declares one sentinel table so the file - // compiles; reaching here with only that table means it was never replaced. - // Without this, `requireTable` reported `Table "users" was not found … - // Available: __stash_placeholder__`, which names the symptom and not the - // cause (#787 review). - // - // Read from `getEncryptConfig()` — the SAME source `loadEncryptConfig` uses — - // not from the harvested export map, or the two commands disagree on one - // file. `schemas: [placeholderTable]` with the `export` keyword dropped is - // still the un-replaced scaffold but exports nothing; conversely a stale - // `export const placeholderTable` beside real tables that are imported - // rather than re-exported is NOT (#787 review). - const configuredTables = Object.keys(client.getEncryptConfig()?.tables ?? {}) - if ( - configuredTables.length === 1 && - configuredTables[0] === PLACEHOLDER_TABLE_NAME - ) { - console.error( - `Error: ${stashConfig.client} still contains the placeholder table \`${PLACEHOLDER_TABLE_NAME}\` that \`stash init\` wrote.\n\nDeclare your encrypted columns and pass those tables to Encryption({ schemas: [...] }) in that file, then re-run this command.`, - ) - process.exit(1) - } + // The same refusal `stash db push` / `db validate` get from + // `loadEncryptConfig`, applied here because `stash encrypt` does not go + // through that loader. Called, not re-implemented: it guards one file, so the + // two commands must say one thing about it. Without this, `requireTable` + // reported `Table "users" was not found … Available: __stash_placeholder__`, + // naming the symptom and not the cause (#787 review). + requireUsableEncryptConfig(client.getEncryptConfig(), stashConfig.client) return { stashConfig, client, tables } } diff --git a/packages/cli/src/config/index.ts b/packages/cli/src/config/index.ts index c304f7d58..4442c43e0 100644 --- a/packages/cli/src/config/index.ts +++ b/packages/cli/src/config/index.ts @@ -231,7 +231,31 @@ export async function loadEncryptConfig( process.exit(1) } - const config = encryptClient.getEncryptConfig() + return requireUsableEncryptConfig( + encryptClient.getEncryptConfig(), + encryptClientPath, + ) +} + +/** + * Refuse an encryption client that cannot drive any command yet, naming the + * cause. + * + * Shared rather than duplicated because it guards ONE file reached by two + * loaders — `loadEncryptConfig` for `stash db push` / `db validate`, and + * `loadEncryptionContext` for `stash encrypt backfill`. When the copies were + * separate they had already drifted on the nullish-config case, so one command + * named the cause while the other fell through to `requireTable`'s `Table + * "users" was not found … Available: (none)` — the symptom-not-cause message + * this guard exists to replace (#787 review follow-up). + * + * Both refusals are hard exits: there is no partially-usable state here, and + * every caller would otherwise have to re-derive that. + */ +export function requireUsableEncryptConfig( + config: EncryptConfig | undefined, + encryptClientPath: string, +): EncryptConfig { if (!config) { console.error( `Error: Encryption client in ${encryptClientPath} has no initialized encrypt config.`, @@ -242,8 +266,12 @@ export async function loadEncryptConfig( // `stash init` scaffolds a client holding one placeholder table, because // `Encryption` requires a non-empty schema set and the scaffold has no real // tables to name yet. Reaching here with only that table means the user never - // replaced it — which used to surface as a confusing "table not found" from - // whichever command ran next. + // replaced it. + // + // Read from the built encrypt config, never from the module's export map: a + // scaffold whose `export` keyword was dropped is still un-replaced, while a + // stale `export const placeholderTable` beside real tables that are imported + // rather than re-exported is not (#787 review). const tables = Object.keys(config.tables ?? {}) if (tables.length === 1 && tables[0] === PLACEHOLDER_TABLE_NAME) { console.error( diff --git a/scripts/__tests__/workflow-turbo-build-deps.test.mjs b/scripts/__tests__/workflow-turbo-build-deps.test.mjs index 1de2124d1..82386574d 100644 --- a/scripts/__tests__/workflow-turbo-build-deps.test.mjs +++ b/scripts/__tests__/workflow-turbo-build-deps.test.mjs @@ -1,4 +1,4 @@ -import { readFileSync } from 'node:fs' +import { readdirSync, readFileSync } from 'node:fs' import { resolve } from 'node:path' import { fileURLToPath } from 'node:url' import yaml from 'js-yaml' @@ -28,7 +28,26 @@ import { readJsonc } from './lib/read-jsonc.mjs' */ const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') -const WORKFLOW = '.github/workflows/tests.yml' + +/** + * The workflow that must carry the `typecheck:scaffold` step specifically. + * Scoped, because "that step exists" is a claim about this file, not about + * workflows in general. + */ +const SCAFFOLD_WORKFLOW = '.github/workflows/tests.yml' + +/** + * Every workflow, not just `tests.yml`. A bare build-dependent step is the same + * latent trap wherever it runs, and the integration workflows are where it is + * least visible: they build only `stash` (via the shared `integration-setup` + * action) and reach every other package through that one task's `^build`. + * Narrowing the guard to one file left five real bare invocations unchecked + * (#787 review follow-up). + */ +const WORKFLOWS = readdirSync(resolve(REPO_ROOT, '.github/workflows')) + .filter((file) => /\.ya?ml$/.test(file)) + .map((file) => `.github/workflows/${file}`) + .sort() /** * Bare invocations that predate this guard. Each is the same latent trap: it @@ -74,7 +93,7 @@ const PNPM_VERBS = new Set([ */ function invokedTask(line) { const turbo = line.match(/\bturbo\b\s+(?:run\s+)?([\w:.-]+)/) - if (turbo) return { task: turbo[1], routed: true } + if (turbo) return { task: turbo[1], routed: true, filtered: false } const pnpm = line.match( /\bpnpm\b(?:\s+(?:--filter|-F)\s+\S+|\s+--if-present)*\s+(?:run\s+)?([\w:.-]+)/, @@ -82,10 +101,19 @@ function invokedTask(line) { if (!pnpm) return null const task = pnpm[1] if (PNPM_VERBS.has(task)) return null - return { task, routed: false } + return { task, routed: false, filtered: /\s(?:--filter|-F)\s/.test(line) } } -/** Root scripts may delegate to turbo themselves (`"test": "turbo test ..."`). */ +/** + * Root scripts may delegate to turbo themselves (`"test": "turbo test ..."`). + * + * Only ever consult this for an UNFILTERED invocation. `pnpm --filter + * ` runs that package's script, so the root script's delegation says + * nothing about it — reading it either way exempted a genuinely bare + * `pnpm --filter @cipherstash/prisma-next-example test:e2e` purely because the + * root happens to define `"test:e2e": "turbo run test:e2e"` (#787 review + * follow-up). + */ const rootScriptDelegatesToTurbo = (task) => typeof rootScripts[task] === 'string' && /\bturbo\b/.test(rootScripts[task]) @@ -104,7 +132,7 @@ function workflowRunLines(path) { return lines } -describe('tests.yml routes build-dependent turbo tasks through turbo', () => { +describe('workflows route build-dependent turbo tasks through turbo', () => { it('turbo.json declares the tasks this guard protects', () => { // If `^build` is dropped from these, the guard below silently stops // checking anything — assert the premise rather than trusting it. @@ -114,7 +142,7 @@ describe('tests.yml routes build-dependent turbo tasks through turbo', () => { }) it('typecheck:scaffold is invoked via turbo, never bare', () => { - const invocations = workflowRunLines(WORKFLOW).filter( + const invocations = workflowRunLines(SCAFFOLD_WORKFLOW).filter( ({ line }) => invokedTask(line)?.task === 'typecheck:scaffold', ) @@ -123,27 +151,35 @@ describe('tests.yml routes build-dependent turbo tasks through turbo', () => { // Deleting the step entirely is caught by this length assertion. expect( invocations.length, - `${WORKFLOW} must run typecheck:scaffold — the scaffold fixtures are only typechecked there`, + `${SCAFFOLD_WORKFLOW} must run typecheck:scaffold — the scaffold fixtures are only typechecked there`, ).toBeGreaterThan(0) for (const { jobName, stepName, line } of invocations) { expect( invokedTask(line).routed, - `${WORKFLOW} job "${jobName}" step "${stepName}" runs typecheck:scaffold without turbo:\n ${line}\nThe scaffold fixtures import @cipherstash/stack/v3, so this needs that package BUILT. Use \`pnpm exec turbo run typecheck:scaffold --filter stash\`.`, + `${SCAFFOLD_WORKFLOW} job "${jobName}" step "${stepName}" runs typecheck:scaffold without turbo:\n ${line}\nThe scaffold fixtures import @cipherstash/stack/v3, so this needs that package BUILT. Use \`pnpm exec turbo run typecheck:scaffold --filter stash\`.`, ).toBe(true) } }) - it('no new bare invocation of a build-dependent turbo task', () => { - const offenders = workflowRunLines(WORKFLOW) - .filter(({ line }) => { - const invoked = invokedTask(line) - if (!invoked || invoked.routed) return false - if (!buildDependentTasks.has(invoked.task)) return false - if (rootScriptDelegatesToTurbo(invoked.task)) return false - return !KNOWN_BARE.has(line) - }) - .map(({ jobName, stepName, line }) => `${jobName} / ${stepName}: ${line}`) + it('no new bare invocation of a build-dependent turbo task in any workflow', () => { + const offenders = WORKFLOWS.flatMap((file) => + workflowRunLines(file) + .filter(({ line }) => { + const invoked = invokedTask(line) + if (!invoked || invoked.routed) return false + if (!buildDependentTasks.has(invoked.task)) return false + // A filtered invocation runs the package's own script, so the root + // script's turbo delegation is irrelevant to it. + if (!invoked.filtered && rootScriptDelegatesToTurbo(invoked.task)) + return false + return !KNOWN_BARE.has(line) + }) + .map( + ({ jobName, stepName, line }) => + `${file} / ${jobName} / ${stepName}: ${line}`, + ), + ) expect( offenders, @@ -155,11 +191,18 @@ describe('tests.yml routes build-dependent turbo tasks through turbo', () => { // KNOWN_BARE entries are matched by exact string. If a step is reworded or // fixed, its entry goes stale and silently exempts nothing — or worse, // masks a different line later. Fail so the list gets pruned. - const lines = new Set(workflowRunLines(WORKFLOW).map(({ line }) => line)) + // Searched across every workflow, not just tests.yml: the allowlist now + // exempts lines found anywhere, so a narrower staleness check would let an + // entry keep exempting a step it no longer describes. + const lines = new Set( + WORKFLOWS.flatMap((file) => workflowRunLines(file)).map( + ({ line }) => line, + ), + ) for (const bare of KNOWN_BARE) { expect( lines.has(bare), - `KNOWN_BARE entry is stale — no step in ${WORKFLOW} runs:\n ${bare}\nRemove it from the allowlist.`, + `KNOWN_BARE entry is stale — no workflow step runs:\n ${bare}\nRemove it from the allowlist.`, ).toBe(true) } })