fix(cli): mixed-table encrypt lifecycle, and a stash init scaffold that compiles (#772 review findings 6, 7)#787
fix(cli): mixed-table encrypt lifecycle, and a stash init scaffold that compiles (#772 review findings 6, 7)#787tobyhede wants to merge 2 commits into
Conversation
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 <guess>`. 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.
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 detectedLatest commit: 7fc55b6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Request changes — one blocking regression; the rest of the PR is strong.
The init-scaffold half (finding 6) is excellent: committing the generated output as byte-for-byte-pinned .generated.ts fixtures with tsconfig.scaffold.json and a typecheck:scaffold CI gate is exactly the right systemic fix for "nothing ever compiled the generated output." The cutover via:'sole' guard (finding 7) correctly mirrors drop.
Blocking — pure single-column EQL v2 tables backfilled with this release are now refused by cutover/drop with misleading guidance.
The unresolvedHint branch in resolveColumnLifecycle (resolve-eql.ts) has no candidates.length > 0 guard, and explainUnresolved checks unresolvedHint before the empty-candidates return null. Traced end-to-end against the branch:
backfill.tsrecordsencryptedColumnin.cipherstash/migrations.jsonunconditionally, v2 included (lines 138-139, 205-213).resolveColumnLifecyclereads that ashint. For a pure-v2 table,listEncryptedColumnsreturns[](classifyEqlDomainrecognizeseql_v3_*only), socandidatesis empty andhintedis null.hint && columnExists(hint)is true (theeql_v2_encryptedcolumn exists) → returns{ info: null, candidates: [], unresolvedHint }.explainUnresolvedreturns the "…legacy eql_v2 column, which this command no longer manages … use a stash release that still supports it" message →cutover/dropexit 1.
Pre-PR this fell through to the v2 rename/config machinery — which is still fully present in cutover.ts/drop.ts in this same release. So the message tells users to downgrade for a lifecycle this build still implements, and the changeset's "Pure-v2 … unaffected" is inaccurate for the backfill-with-this-release path.
The finding-7 protection is aimed at mixed tables (a v2 pair + an unrelated v3 column), which always have candidates. Gating the fail-closed on candidates.length > 0 preserves that protection exactly while restoring the safe pure-v2 fall-through. Please pick one:
- Preferred: gate the
columnExists/explainUnresolvedbranch oncandidates.length > 0; empty candidates → fall through to the v2 ladder as before. - Or, if the intent is genuinely to drop v2 lifecycle from
stash encrypt(this is on theremove-v2train, so that's plausible): do it coherently — remove the now-dead v2 code incutover.ts/drop.ts/backfill.ts, fix the message (don't say "use an older release" while shipping the code), and correct the changeset. - Either way, add a test for the untested shape:
candidates: []+ a recorded hint. The existingresolveColumnLifecycletests only covercandidates: [V3_OTHER], which is why this slipped through.
Non-blocking:
- The placeholder-client guard lives in
loadEncryptConfig, hit only bystash db push/validate.stash encrypt backfill(vialoadEncryptionContext/requireTable),cutover, anddroparen't gated, so they still emit the "table not found" error the changeset/skill say is eliminated. Either extend the guard or soften the wording. resolve-eql.test.tsspreadsawait importOriginal<@cipherstash/migrate>(), so it fails on a clean checkout until@cipherstash/migrateis built (CI is green only because turbo runs^buildfirst).packages/cli/AGENTS.mdwants the unit config self-contained — prefer a full explicit mock.packages/cli/package.jsonre-encodes em-dashes to—indescription/ the//optionalDependenciescomment — unrelated editor noise; revert to keep the diff focused.
2 participants
Review remediation for #772 — findings 6 and 7, both verified by executing the real commands.
Finding 7 —
encrypt cutover/dropacted on a guessed columnOn a table holding a legacy EQL v2 pair (
ssn/ssn_encrypted) plus one unrelated EQL v3 column, the v2 ciphertext column is not classified as an EQL column at all, so resolution fell through to the sole-EQL-column rule and claimed the unrelated v3 column. Three consequences:cutoverreported success for work it never did. Noviagate, and its v3 branchreturns without settingexitCode, sofinallynever firesprocess.exit— it printed "point your application atemail_enc" and exited 0 while the v2 rename never ran.drop.ts:106already gated onvia === 'sole'for exactly this reason.backfillwrites the trueencryptedColumnto the manifest, so the answer was on disk, but a hint that failed to resolve was dropped and the re-pick reached the guess. Now split by cause: a hint naming a column that is gone is stale and still falls back to convention; a hint naming a column that exists but is not EQL v3 is reported by name.drop's remedy prescribed the guess. Following--encrypted-column <guess>recorded it as fact, so the next run resolvedvia: 'hint', walked past the gate, and passed the coverage check vacuously — an unrelated but legitimately backfilled column is non-NULL on every row — then generated a liveDROP COLUMNon the plaintext at exit 0.Tested at the layer that had none:
resolve-eql.test.tscovered onlyexplainUnresolved, andencrypt-v3.test.tsstubsresolveColumnLifecycleoutright, so neither could see the hint discard. The new tests keeppickEncryptedColumnreal and replace only the two I/O boundaries.Finding 6 —
stash initscaffolded a file that does not compileBoth templates emitted
await Encryption({ schemas: [] }), a hardTS2769against both overloads. Everystash initleft a project failing its firsttsc.Relaxing the constraint is not an option — it is S-6 from #778, and catches a real mistake.
stash inithas no table names in scope by design. So the scaffold declares a sentinel__stash_placeholder__table, andloadEncryptConfigexits 1 while that is the only table, naming the file.The gap that let it ship matters more than the fix. Nothing compiled, parsed or executed the generated output:
packages/clihas no typecheck step, the codegen tests onlytoContain-match fragments, andbuild-schema.test.tsmocks the generator to'// placeholder'. Both templates are now committed as.generated.tsfixtures compiled by a scoped tsconfig in CI, pinned byte-for-byte to the generator by a unit test. Verified the gate reproduces the originalTS2769when reverted.Verification
pnpm --filter stash test→ 884 passed.typecheck:scaffoldclean. Biome: 0 errors.Changesets:
encrypt-lifecycle-mixed-table(stash patch),init-scaffold-compiles(stash patch).skills/stash-cliupdated for the new scaffold behaviour.