Skip to content

Commit fd5aafe

Browse files
committed
fix(cli,migrate): close gaps found verifying the #787 review fixes
Follow-up to 1d14412, 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.
1 parent a55788b commit fd5aafe

21 files changed

Lines changed: 343 additions & 72 deletions

File tree

.changeset/encrypt-lifecycle-mixed-table.md

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,25 @@ all now fixed:
3535
column that actually encrypts the named one, and says explicitly not to record
3636
the guess.
3737

38-
The new refusal is scoped to the mixed table it was written for: it applies only
39-
when the table actually holds EQL v3 columns that a guess could wrongly claim. A
40-
pure-v2 table has none, so it still falls through to the EQL v2 lifecycle exactly
41-
as before — including when `encrypt backfill` recorded an `encryptedColumn` for
42-
it, which it does for v2 columns too. Pure-v2 and pure-v3 tables are therefore
43-
unaffected, as are tables with two or more EQL v3 columns (resolution already
44-
failed closed there).
38+
The `unresolvedHint` refusal is scoped to tables that actually hold EQL v3
39+
columns a guess could wrongly claim. A **pure-v2** table has none, so it still
40+
falls through to the EQL v2 lifecycle exactly as before — including when
41+
`encrypt backfill` recorded an `encryptedColumn` for it, which it does for v2
42+
columns too.
43+
44+
Two cases DO newly exit 1, both deliberately:
45+
46+
- Any table with at least one EQL v3 column where the manifest records an
47+
`encryptedColumn` that exists but is not one of them — not only the
48+
v2-pair-plus-one-v3 shape. The recorded pairing is authoritative and
49+
disagrees with every candidate, so guessing past it is the bug.
50+
51+
- A column whose encrypted counterpart could only be identified **by
52+
elimination** — no recorded `encryptedColumn`, no `<col>_encrypted` name
53+
match, one EQL column left once the plaintext column itself is excluded.
54+
`cutover` now refuses this as `drop` already did. Note `.cipherstash/` is
55+
gitignored, so `migrations.json` is machine-local: a fresh clone or CI runner
56+
can hit this on a **pure-v3** table whose encrypted column is named
57+
unconventionally, where `cutover` previously exited 0 with "not applicable".
58+
Re-run `stash encrypt backfill --table T --column C --encrypted-column <name>`
59+
to record the pairing.
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
---
2+
'@cipherstash/migrate': minor
3+
'stash': patch
4+
---
5+
6+
Add `columnExists(client, tableName, columnName)` — a case-exact "does this
7+
column exist at all?" catalog probe, distinct from `detectColumnEqlVersion`'s
8+
"and is it an EQL column?".
9+
10+
Callers need that difference to tell a STALE column reference (it is gone) from
11+
a live one the domain classifier simply does not recognise — most often a legacy
12+
`eql_v2_encrypted` counterpart.
13+
14+
`stash encrypt cutover` / `drop` had a private copy of this probe built on a bare
15+
`to_regclass($1)`. That form *parses* its argument and case-folds unquoted
16+
identifiers, so on a Prisma-style `"User"` table it resolved `user`, reported the
17+
column missing, and treated a valid recorded pairing as stale — silently skipping
18+
the fail-closed that stops those commands acting on a guessed encrypted column.
19+
The shared implementation quotes with `format('%I')` first, like every other
20+
catalog probe in this package, so the lookup is case-exact while still honouring
21+
`search_path` for unqualified names.

.github/workflows/tests.yml

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,8 +199,14 @@ jobs:
199199
# emitting a project that fails its first `tsc` — with CI green (#772
200200
# review). The fixtures are pinned byte-for-byte to the generator by
201201
# `placeholder-client-fixture.test.ts`.
202+
# Through turbo, not `pnpm run` — the fixtures import `@cipherstash/stack/v3`,
203+
# so this needs that package BUILT. Invoked directly it passed only because
204+
# earlier steps in this job happen to build it via their own `^build`; drop
205+
# or reorder those (they read as guards for other packages, so they look
206+
# independently removable) and this fails `TS2307`, which reads as "the
207+
# scaffold is broken" rather than "you forgot to build" (#787 review).
202208
- name: Typecheck (stash init's scaffolded client)
203-
run: pnpm --filter stash run typecheck:scaffold
209+
run: pnpm exec turbo run typecheck:scaffold --filter stash
204210

205211
- name: Lint — no hardcoded package-manager runners
206212
run: pnpm run lint:runners

packages/cli/AGENTS.md

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,22 @@ This package has **two** Vitest configs. Run the right one for the change.
66

77
| Command | Config | Scope | Needs build? |
88
| --- | --- | --- | --- |
9-
| `pnpm --filter stash test` | `vitest.config.ts` | Unit tests under `src/__tests__/**` and `src/**/__tests__/**` | No |
9+
| `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. |
1010
| `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`. |
1111

1212
The unit config explicitly excludes `tests/e2e/**` so the default `pnpm test`
13-
stays fast and self-contained.
13+
stays fast.
14+
15+
It is **not** fully self-contained, despite running standalone in CI. Some `src`
16+
modules import workspace packages that publish `./dist` only, so an unbuilt
17+
workspace fails at collection with `Failed to resolve entry for package …`
18+
rather than at an assertion. `vitest.config.ts` aliases `@cipherstash/migrate`
19+
to its source to remove one such coupling; `@cipherstash/stack` remains, reached
20+
via `packages/migrate/src/backfill.ts` and a direct import in
21+
`init/lib/introspect.test.ts`. Deleting `packages/stack/dist` fails 10 files.
22+
Closing it needs `vitest.shared.ts`'s `stackSourceAlias`, which cannot be spread
23+
into this config — its `'@/'` entry points at `packages/stack/src` and would
24+
clobber this package's own `'@/'` (#787 review).
1425

1526
## When to add or update an E2E test
1627

packages/cli/__fixtures__/scaffold/drizzle.generated.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,10 @@
88
* to reference the encrypted tables you declared there.
99
*
1010
* Until that happens, the encryption client is initialised with a single
11-
* placeholder table so that this file compiles, and `stash encrypt`
12-
* commands refuse to run and point back here.
11+
* placeholder table so that this file compiles, and `stash db push`,
12+
* `stash db validate` and `stash encrypt backfill` refuse to run and point
13+
* back here. (`stash encrypt cutover` / `drop` resolve against the database
14+
* and never read this file.)
1315
*
1416
* This project uses EQL v3. Encrypted columns are concrete Postgres domains
1517
* built with the `types.*` factories from `@cipherstash/stack-drizzle`.
@@ -56,8 +58,8 @@ import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3'
5658

5759
// REPLACE THIS. It exists only so this file compiles before you have declared
5860
// any encrypted tables — `Encryption` requires at least one. Swap it for your
59-
// real tables (see the patterns above); `stash encrypt` refuses to run while
60-
// the placeholder is still here.
61+
// real tables (see the patterns above); `stash db push`, `stash db validate`
62+
// and `stash encrypt backfill` refuse to run while the placeholder is still here.
6163
export const placeholderTable = encryptedTable('__stash_placeholder__', {
6264
replace_me: types.Text('replace_me'),
6365
})

packages/cli/__fixtures__/scaffold/generic.generated.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,10 @@
77
* `Encryption({ schemas: [...] })` call below to reference them.
88
*
99
* Until that happens, the encryption client is initialised with a single
10-
* placeholder table so that this file compiles, and `stash encrypt`
11-
* commands refuse to run and point back here.
10+
* placeholder table so that this file compiles, and `stash db push`,
11+
* `stash db validate` and `stash encrypt backfill` refuse to run and point
12+
* back here. (`stash encrypt cutover` / `drop` resolve against the database
13+
* and never read this file.)
1214
*
1315
* This project uses EQL v3. Encrypted columns are concrete Postgres domains
1416
* built with the `types.*` factories from `@cipherstash/stack/eql/v3`
@@ -51,8 +53,8 @@ import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3'
5153

5254
// REPLACE THIS. It exists only so this file compiles before you have declared
5355
// any encrypted tables — `Encryption` requires at least one. Swap it for your
54-
// real tables (see the patterns above); `stash encrypt` refuses to run while
55-
// the placeholder is still here.
56+
// real tables (see the patterns above); `stash db push`, `stash db validate`
57+
// and `stash encrypt backfill` refuse to run while the placeholder is still here.
5658
export const placeholderTable = encryptedTable('__stash_placeholder__', {
5759
replace_me: types.Text('replace_me'),
5860
})

packages/cli/src/commands/encrypt/__tests__/context-placeholder.test.ts

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,12 @@ describe('loadEncryptionContext — the un-replaced init scaffold', () => {
5353
})
5454

5555
it('exits 1 naming the sentinel when it is the only table declared', async () => {
56+
// The scaffold's real shape: the sentinel is both exported AND the sole
57+
// entry in the built encrypt config, because it was passed to `Encryption`.
5658
writeProject(
57-
`export const encryptionClient = { getEncryptConfig: () => ({}) }
59+
`export const encryptionClient = {
60+
getEncryptConfig: () => ({ tables: { '${PLACEHOLDER_TABLE_NAME}': {} } }),
61+
}
5862
export const placeholderTable = ${table(PLACEHOLDER_TABLE_NAME)}`,
5963
)
6064
const error = vi.spyOn(console, 'error').mockImplementation(() => {})
@@ -72,6 +76,54 @@ describe('loadEncryptionContext — the un-replaced init scaffold', () => {
7276
expect(message).not.toContain('was not found in the encryption client')
7377
})
7478

79+
/**
80+
* #787 review. The guard originally read the harvested EXPORT map, while the
81+
* `db push` / `db validate` guard it mirrors reads `getEncryptConfig().tables`.
82+
* Those disagree in both directions on the same client file, so the two
83+
* commands gave different answers for identical input.
84+
*/
85+
it('fires when the placeholder is passed to Encryption but never exported', async () => {
86+
// The false NEGATIVE. `schemas: [placeholderTable]` with a bare `const` —
87+
// the scaffold minus one `export` keyword. Reading exports, the guard saw
88+
// no tables and fell through to `requireTable`'s "table not found …
89+
// Available: (none)" — the very error this guard exists to replace.
90+
writeProject(
91+
`export const encryptionClient = {
92+
getEncryptConfig: () => ({ tables: { '${PLACEHOLDER_TABLE_NAME}': {} } }),
93+
}`,
94+
)
95+
const error = vi.spyOn(console, 'error').mockImplementation(() => {})
96+
vi.spyOn(process, 'exit').mockImplementation(() => {
97+
throw new Error('process.exit')
98+
})
99+
100+
const { loadEncryptionContext } = await import('../context.js')
101+
await expect(loadEncryptionContext()).rejects.toThrow('process.exit')
102+
103+
const message = error.mock.calls.flat().join('\n')
104+
expect(message).toContain('still contains the placeholder table')
105+
expect(message).not.toContain('was not found in the encryption client')
106+
})
107+
108+
it('stays silent when a stale placeholder export sits beside real configured tables', async () => {
109+
// The false POSITIVE, and the mirror of the case above: the user replaced
110+
// the schema set but left the sentinel `export` behind (or imports their
111+
// real tables without re-exporting them). Reading exports, the guard fired
112+
// and told them to declare columns they had already declared. `db push`
113+
// passes on this same file.
114+
writeProject(
115+
`export const encryptionClient = {
116+
getEncryptConfig: () => ({ tables: { users: {} } }),
117+
}
118+
export const placeholderTable = ${table(PLACEHOLDER_TABLE_NAME)}`,
119+
)
120+
121+
const { loadEncryptionContext } = await import('../context.js')
122+
const ctx = await loadEncryptionContext()
123+
124+
expect(ctx.tables.has(PLACEHOLDER_TABLE_NAME)).toBe(true)
125+
})
126+
75127
it('allows the sentinel through once a real table sits alongside it', async () => {
76128
// Only the SOLE-placeholder case is the un-replaced scaffold. A user who
77129
// has added real tables must not be blocked by a leftover sentinel export.

packages/cli/src/commands/encrypt/context.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,14 +142,25 @@ export async function loadEncryptionContext(): Promise<EncryptionContext> {
142142
process.exit(1)
143143
}
144144

145-
// Same guard `loadEncryptConfig` applies for `stash db push` / `db validate`,
145+
// The guard `loadEncryptConfig` applies for `stash db push` / `db validate`,
146146
// repeated here because `stash encrypt` does not go through that loader. The
147147
// scaffold `stash init` writes declares one sentinel table so the file
148148
// compiles; reaching here with only that table means it was never replaced.
149149
// Without this, `requireTable` reported `Table "users" was not found …
150150
// Available: __stash_placeholder__`, which names the symptom and not the
151151
// cause (#787 review).
152-
if (tables.size === 1 && tables.has(PLACEHOLDER_TABLE_NAME)) {
152+
//
153+
// Read from `getEncryptConfig()` — the SAME source `loadEncryptConfig` uses —
154+
// not from the harvested export map, or the two commands disagree on one
155+
// file. `schemas: [placeholderTable]` with the `export` keyword dropped is
156+
// still the un-replaced scaffold but exports nothing; conversely a stale
157+
// `export const placeholderTable` beside real tables that are imported
158+
// rather than re-exported is NOT (#787 review).
159+
const configuredTables = Object.keys(client.getEncryptConfig()?.tables ?? {})
160+
if (
161+
configuredTables.length === 1 &&
162+
configuredTables[0] === PLACEHOLDER_TABLE_NAME
163+
) {
153164
console.error(
154165
`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.`,
155166
)

packages/cli/src/commands/encrypt/cutover.ts

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -95,23 +95,32 @@ export async function cutoverCommand(options: CutoverCommandOptions) {
9595
}
9696
const state = await progress(client, options.table, options.column)
9797

98+
// `via: 'sole'` means only that this is the table's one remaining EQL
99+
// candidate once the plaintext column itself is excluded — nothing ties it
100+
// to the plaintext column the user named. On a mixed table (a v2 pair the
101+
// classifier no longer sees, plus one unrelated v3 column) that guess is
102+
// simply wrong, and reporting "nothing to do for EQL v3" for it told a
103+
// scripted rollout the cut-over had succeeded when the v2 rename never ran
104+
// (#772 review, finding 7).
105+
//
106+
// Deliberately at TOP LEVEL, not inside the `version === 3` branch, so it
107+
// mirrors `drop.ts` exactly. Equivalent today — `classifyEqlDomain`
108+
// recognises `eql_v3_*` only, so a non-null `info` is always version 3 —
109+
// but the v2 ladder below performs an irreversible rename plus config
110+
// promotion. Were v2 classification restored, or a v4 family added, a
111+
// nested guard would let `cutover` rename on a guess that `drop` refuses
112+
// (#787 review).
113+
if (info?.via === 'sole') {
114+
p.log.error(
115+
`${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 <the column that actually encrypts ${options.column}>\`.`,
116+
)
117+
exitCode = 1
118+
return
119+
}
120+
98121
if (info?.version === 3) {
99122
const encryptedColumn = info.column
100123

101-
// `via: 'sole'` means only that this is the table's ONE EQL v3 column —
102-
// nothing ties it to the plaintext column the user named. On a mixed
103-
// table (a v2 pair the classifier no longer sees, plus one unrelated v3
104-
// column) that guess is simply wrong, and reporting "nothing to do for
105-
// EQL v3" for it told a scripted rollout the cut-over had succeeded when
106-
// the v2 rename never ran. `drop.ts` already refuses a `'sole'` match for
107-
// the same reason (#772 review, finding 7).
108-
if (info.via === 'sole') {
109-
p.log.error(
110-
`${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 <the column that actually encrypts ${options.column}>\`.`,
111-
)
112-
exitCode = 1
113-
return
114-
}
115124
if (state?.phase === 'dropped') {
116125
// Terminal phase — the lifecycle already finished. Not an error and
117126
// not "finish the backfill": there is nothing left to backfill.

packages/cli/src/commands/encrypt/drop.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ export async function dropCommand(options: DropCommandOptions) {
113113
// live `DROP COLUMN` on the plaintext at exit 0 (#772 review, finding 7).
114114
if (info?.via === 'sole') {
115115
p.log.error(
116-
`${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 <name>\` (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}.`,
116+
`${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 <name>\` (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}.`,
117117
)
118118
exitCode = 1
119119
return

0 commit comments

Comments
 (0)