Conversation
🦋 Changeset detectedLatest commit: bf7be09 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 skippedToo many files! This PR contains 365 files, which is 265 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (470)
You can disable this status message by setting the ✨ 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 |
7 tasks
This was referenced Jul 24, 2026
tobyhede
added a commit
that referenced
this pull request
Jul 24, 2026
…tion fix: review remediation for #772 — rewriter data loss, supabase row types, shipped-doc corrections
Contributor
Code review of #780's content, retargeted hereThis review ran against One finding changed materially on re-verification — see the correction under Type surface below. Two candidates were dropped before this list as pre-existing on The headline is a cluster, not a listFindings 1–4 are all 1. Quote-parity drift makes a commented
|
tobyhede
added a commit
that referenced
this pull request
Jul 25, 2026
…icit schema Two ways a corpus could still lose ciphertext to the ADD+DROP+RENAME, both found by the #772 review. Neither is caught by the fail-closed `declared` rule, because in both the column really is declared — as plaintext, by the original CREATE TABLE. Finding 3 — chained conversions. `indexColumnDeclarations` reads CREATE TABLE, ADD COLUMN and RENAME, but never the strict matcher's own target. A corpus carrying an earlier `SET DATA TYPE eql_v2_encrypted` (a stack version that predates this sweep) left the column looking plaintext, so a later domain change dropped its ciphertext. Record the conversion as the sweep walks, in file-sorted order — not in the corpus-wide index, which has no order and would flag the legitimate first conversion too. Finding 4 — `"users"` and `"public"."users"` are the same table; Postgres resolves the unqualified name through `search_path`. drizzle-kit emits unqualified, while hand-written SQL and this sweep's own renderSafeAlter output are qualified, so a corpus mixing the two split one column across two keys: the already-encrypted guard missed, and the ALTER dropped ciphertext. Collapse the implicit schema in `columnKey`. Only that one — `"app"."users"` stays distinct, and there is no case folding, since TABLE_REF matches quoted identifiers only. Both fixes land in both copies of the rewriter. 7 tests each: the two destructive cases, the wrong-reason case finding 4 also produced (`source-unknown` for a column declared two files up), and four guards for the ways a naive fix breaks — the first conversion in a chain, a chain across different columns, a commented-out earlier ALTER, and a real non-public schema.
tobyhede
added a commit
that referenced
this pull request
Jul 25, 2026
…ls CI The rewriter lives twice — packages/wizard/src/lib and packages/cli/src/commands/db — at ~99% similarity, and neither package depends on the other. Four #772 review findings each needed the identical fix in both places; a fix landing in one still passes that package's own suite, and this code emits DROP COLUMN. Extraction is not available this cycle: packages/utils has no package.json, @cipherstash/test-kit is private and build-less, and both bin entries set skipNodeModulesBundle, so a shared workspace package would survive into dist as an unresolvable bare specifier. That leaves publishing a new package into a fixed mid-RC release group or adding noExternal to two CLI bundles — both worse than the drift they would prevent, for now. So compare instead. Everything outside an explicit `#region wizard-only` marker must match byte-for-byte modulo comments, imports and the tool name in the emitted header. Verified the guard actually fires: perturbing either copy fails with a message naming both files and pointing at the region marker.
This was referenced Jul 25, 2026
tobyhede
added a commit
that referenced
this pull request
Jul 26, 2026
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.
freshtonic
requested changes
Jul 27, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the umbrella branch structurally, verified against source, and ran build/lint/unit tests in an isolated worktree (commit f7b8220).
Requesting changes — but to be clear up front: this means "not ready to merge as intended," not "the completed work is wrong." The landed work is genuinely strong (green build/lint/unit tests, a correctly preserved and tested v2 read path, no dangling references to the deleted packages). The reasons it can't merge are inherent to it being a draft umbrella branch mid-stack.
Blockers
- Stated end-state unmet — v2 authoring/emission still present.
packages/stack/src/index.ts:9still re-exports the v2 DSL (encryptedColumn,encryptedField,encryptedTable), andresolveEqlVersion(encryption/index.ts:97-131) returnsundefinedfor an all-v2 schema, which protect-ffi 0.30 defaults to wire v2 — soEncryption({ schemas: [v2Table] })still emits v2 with no native-entry rejection (onlywasm-inline.tsrejects). By design (PRs 9–12 outstanding), but it blocks merge of the umbrella. - CLI still carries the entire v2 surface (
--eql-version 2,--latest, v2 SQL, v2 drizzle/supabase routing). PR 8 outstanding. - Branch is
CONFLICTING, 9 commits behind main / 57 ahead (the body's "one commit behind" is stale). Needs rebase + conflict resolution (packages/stack-drizzle/README.md) before it can merge. - Phantom changeset bump —
.changeset/remove-eql-v2-packages.mdbumps@cipherstash/nextjs: patch, butpackages/nextjsdepends only onjoseand imports no@cipherstash/*package. It would ship a nextjs CHANGELOG entry about removing three packages it never used. Drop the line.
Major
- Several unconsumed changesets (
adapter-package-split.md,eql-v3-drizzle.md, and 6 others) advertise@cipherstash/stack-drizzle/v3/@cipherstash/stack/eql/v3/drizzleas migration targets, but this PR removes the./v3subpath (a hard break). They render into the same 1.0.0-rc.5 CHANGELOG asremove-eql-v2-drizzle-root.md, producing directly contradictory guidance. Pre-existing on main, but this PR makes them false — reconcile before release.
Minor / nits
.changeset/remove-eql-v2-migrate-classifier.mdispatchbut the change makeslistEncryptedColumns/resolveEncryptedColumnsilently omit v2 columns under an unchanged signature —minorcommunicates it better (moot inside migrate's own major).- PR description is stale vs HEAD — several "Fix before merge" items are already resolved (root README swept to v3;
packages/stack/README.md:446-451now usesencryptedSupabase;drizzle-encrypted-indexes.mdno longer advertises/v3). Refresh so reviewers/CHANGELOG readers aren't chasing fixed items.
What's done right (worth calling out)
- v2 decrypt preservation is correct and tested:
integration/shared/v2-decrypt-compat.integration.test.tscase #1c (a v3-configured client that never registered the v2 table decrypts v2 ciphertext — the actual customer invariant), plus a credential-free envelope-split path (23/23). The test documents its own fragility and instructs future PRs to keep #1c alive. - No dangling references to the deleted
@cipherstash/protect/@cipherstash/schema/@cipherstash/protect-dynamodbin source/config. - Export-map integrity intact (every changed package keeps
import+require; the only removal,stack-drizzle./v3, is the intended documented hard break with sound reasoning). - Meta files kept honest per AGENTS.md (
AGENTS.md,SECURITY.md,README.md, all seven skills); thestash-cliskill still accurately documents the v2 flags the not-yet-removed CLI ships, so skill/manifest stay consistent for the current state.
Caveat: I could not run the live ZeroKMS suite (no credentials), so the #1c v3-decrypts-v2 invariant is verified by inspection only — confirm it green in CI before merge.
tobyhede
added a commit
that referenced
this pull request
Jul 27, 2026
Follow-up to 1d14412, from an adversarial cross-check of that commit. Blocking, and self-inflicted: `git checkout origin/main -- packages/cli/package.json` (the em-dash revert) also imported main's Dependabot bump of posthog-node to ^5.41.0, which this branch's lockfile predates. Every CI job starts with `pnpm install --frozen-lockfile`, so the PR died at step 1 before a test ran. Back to ^5.40.0, matching the lockfile and packages/wizard. 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.
tobyhede
added a commit
that referenced
this pull request
Jul 27, 2026
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.
tobyhede
added a commit
that referenced
this pull request
Jul 27, 2026
Follow-up to 1d14412, from an adversarial cross-check of that commit. Blocking, and self-inflicted: `git checkout origin/main -- packages/cli/package.json` (the em-dash revert) also imported main's Dependabot bump of posthog-node to ^5.41.0, which this branch's lockfile predates. Every CI job starts with `pnpm install --frozen-lockfile`, so the PR died at step 1 before a test ran. Back to ^5.40.0, matching the lockfile and packages/wizard. 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.
tobyhede
added a commit
that referenced
this pull request
Jul 27, 2026
fix(stack,bench): wasm-inline DynamoDB v2 reads, and a bench seed that never encrypted (#772 review findings 10, 12)
tobyhede
added a commit
that referenced
this pull request
Jul 27, 2026
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.
tobyhede
added a commit
that referenced
this pull request
Jul 27, 2026
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.
tobyhede
added a commit
that referenced
this pull request
Jul 27, 2026
fix(cli): mixed-table encrypt lifecycle, and a stash init scaffold that compiles (#772 review findings 6, 7)
tobyhede
added a commit
that referenced
this pull request
Jul 27, 2026
…tch their own rot Follow-up to the #772 review findings, addressing what the review of that review turned up. `stash init`'s setup prompt named `createEncryptionOperators` unconditionally. That symbol is exported by `@cipherstash/stack-drizzle` alone, so a Supabase or Prisma Next project was sent after a package that is not in its dependency tree — the previous `protectOps.eq` was wrong for everyone, this was wrong for three integrations out of four. Step 5 now branches through `queryOperatorGuidance()`, following the `migrationCommands()` pattern already in the file: `ops.eq` for Drizzle, the `encryptedSupabase` wrapper's own filters for Supabase, the `eql*` column operators for Prisma Next, and `client.encryptQuery(...)` for plain Postgres — which is also pointed at `stash-encryption`, since it is the default integration and installs no integration skill, making "see the integration skill" a dangling pointer. The package-path linter reported an untracked-but-present package as "does not exist" — finding 15's false alarm pointed the other way, at a directory sitting right there on disk. The live set now unions `git ls-files --others --exclude-standard`. `--directory` is deliberately not passed: it collapses an all-ignored directory to one entry and would resurrect exactly the `dist/`-and- `node_modules/` shells the linter exists to catch (verified both ways). git failing now exits 2 with an actionable message instead of a raw ENOENT stack trace and exit 1, which was indistinguishable from a genuine lint failure, and an empty live set refuses to run rather than flagging every reference at once. The `scans scripts/ but not its fixtures` test asserted the repo was clean — byte-for-byte what the suite's first test already asserted, and passing whether or not `scripts` was in TARGETS. It now plants an offender in the scanned directory. Verified by mutation: it dies when `scripts` is dropped, as the untracked-package test dies without the union and the git-failure test dies when the exit code is flipped. The runners linter now requires each allowlist entry to exist and to still contain an unexcused `npx` literal. It immediately found a second stale entry the sibling linter structurally cannot see, since it only matches the `packages/<name>` shape: `setup-prompt.ts` has had no `npx` since its switch moved to `utils.ts`. Removed. Also corrects two claims: the test comment saying `packages/drizzle` never existed in git history (it was added speculatively by c671560, became load-bearing 31 minutes later with 9d259e6, and rotted when 413ca39 deleted the package), and the `AnyEncryptedTable` doc comment still promising v3 is "purely additive and no existing caller has to change" — true of decrypt, false of encrypt since the v2 write overloads were removed.
tobyhede
added a commit
that referenced
this pull request
Jul 27, 2026
…tch their own rot Follow-up to the #772 review findings, addressing what the review of that review turned up. `stash init`'s setup prompt named `createEncryptionOperators` unconditionally. That symbol is exported by `@cipherstash/stack-drizzle` alone, so a Supabase or Prisma Next project was sent after a package that is not in its dependency tree — the previous `protectOps.eq` was wrong for everyone, this was wrong for three integrations out of four. Step 5 now branches through `queryOperatorGuidance()`, following the `migrationCommands()` pattern already in the file: `ops.eq` for Drizzle, the `encryptedSupabase` wrapper's own filters for Supabase, the `eql*` column operators for Prisma Next, and `client.encryptQuery(...)` for plain Postgres — which is also pointed at `stash-encryption`, since it is the default integration and installs no integration skill, making "see the integration skill" a dangling pointer. The package-path linter reported an untracked-but-present package as "does not exist" — finding 15's false alarm pointed the other way, at a directory sitting right there on disk. The live set now unions `git ls-files --others --exclude-standard`. `--directory` is deliberately not passed: it collapses an all-ignored directory to one entry and would resurrect exactly the `dist/`-and- `node_modules/` shells the linter exists to catch (verified both ways). git failing now exits 2 with an actionable message instead of a raw ENOENT stack trace and exit 1, which was indistinguishable from a genuine lint failure, and an empty live set refuses to run rather than flagging every reference at once. The `scans scripts/ but not its fixtures` test asserted the repo was clean — byte-for-byte what the suite's first test already asserted, and passing whether or not `scripts` was in TARGETS. It now plants an offender in the scanned directory. Verified by mutation: it dies when `scripts` is dropped, as the untracked-package test dies without the union and the git-failure test dies when the exit code is flipped. The runners linter now requires each allowlist entry to exist and to still contain an unexcused `npx` literal. It immediately found a second stale entry the sibling linter structurally cannot see, since it only matches the `packages/<name>` shape: `setup-prompt.ts` has had no `npx` since its switch moved to `utils.ts`. Removed. Also corrects two claims: the test comment saying `packages/drizzle` never existed in git history (it was added speculatively by c671560, became load-bearing 31 minutes later with 9d259e6, and rotted when 413ca39 deleted the package), and the `AnyEncryptedTable` doc comment still promising v3 is "purely additive and no existing caller has to change" — true of decrypt, false of encrypt since the v2 write overloads were removed.
tobyhede
added a commit
that referenced
this pull request
Jul 27, 2026
fix(skills,stack,scripts): shipped decrypt guidance, and a sound package-path linter (#772 review findings 14, 15)
tobyhede
added a commit
that referenced
this pull request
Jul 28, 2026
…icit schema Two ways a corpus could still lose ciphertext to the ADD+DROP+RENAME, both found by the #772 review. Neither is caught by the fail-closed `declared` rule, because in both the column really is declared — as plaintext, by the original CREATE TABLE. Finding 3 — chained conversions. `indexColumnDeclarations` reads CREATE TABLE, ADD COLUMN and RENAME, but never the strict matcher's own target. A corpus carrying an earlier `SET DATA TYPE eql_v2_encrypted` (a stack version that predates this sweep) left the column looking plaintext, so a later domain change dropped its ciphertext. Record the conversion as the sweep walks, in file-sorted order — not in the corpus-wide index, which has no order and would flag the legitimate first conversion too. Finding 4 — `"users"` and `"public"."users"` are the same table; Postgres resolves the unqualified name through `search_path`. drizzle-kit emits unqualified, while hand-written SQL and this sweep's own renderSafeAlter output are qualified, so a corpus mixing the two split one column across two keys: the already-encrypted guard missed, and the ALTER dropped ciphertext. Collapse the implicit schema in `columnKey`. Only that one — `"app"."users"` stays distinct, and there is no case folding, since TABLE_REF matches quoted identifiers only. Both fixes land in both copies of the rewriter. 7 tests each: the two destructive cases, the wrong-reason case finding 4 also produced (`source-unknown` for a column declared two files up), and four guards for the ways a naive fix breaks — the first conversion in a chain, a chain across different columns, a commented-out earlier ALTER, and a real non-public schema.
Contributor
Author
|
Thanks Dan — I audited both current-head reports, verified the findings against The result is 16 distinct remaining items. I’ve consolidated them into the existing trackers:
The issue bodies now contain explicit acceptance criteria for each item:
Open-PR audit:
The broader remediation remains tracked by #778 under the #707 removal umbrella. |
coderdan
approved these changes
Jul 29, 2026
coderdan
left a comment
coderdan
left a comment
Contributor
There was a problem hiding this comment.
Approved based on follow-ups tracked in separate issues.
`detectColumnEqlVersion` returns null both for a column whose domain isn't `eql_v3_*` and for a column that isn't there at all, so backfill's single error accused a user who had merely not added `<col>_encrypted` yet of running legacy EQL v2 — and handed them a remedy (migrate the domain) for a column that does not exist. Extract the guard into `assertEqlV3Target` and probe `columnExists` on the failure path only; a v3 answer already proves existence, so the happy path still makes one catalog query. Also mark the retired surface in two internal design plans (`stash db push`, `stash encrypt cutover`, the `eql_v2.*` wrappers, `commands/db/push.ts`) so a future reader doesn't chase files that were deleted with EQL v2. The historical narrative is left intact.
test(scripts): cover repository-linter race guards
…) recursion
Two guards in the string-form `.or()` path had no test that could see them
fail.
`substituteOrStringLeaves` sorts its replacements right-to-left so an earlier
source span stays valid after a later one grew — and every v3 envelope is far
longer than the plaintext operand it replaces. Sorting ascending instead
splices the second envelope INSIDE the first and leaks the trailing condition
verbatim: `email.eq."{\"created_at.gte."{…}"…}",createdAt.gte.2026-01-01`.
Only one existing test replaced two leaves at all (the `is`-on-an-encrypted-
column case, where the second replacement is `created_at.is.null` — shorter
than the span it fills), so the growth case that motivates the sort was
untested.
`parseOrStringAt`'s group regex admits a `not.` prefix. Drop it and
`not.and(email.eq.ada,…)` stops being a group: the leaf parser splits it at
the first dot into the pseudo-column `not`, which matches no encrypted
column, so the whole or-string takes the verbatim branch and `ada` goes to
PostgREST in the clear against an encrypted column.
Both were verified red against those exact mutations and green on restore.
… it explains `reconstructDate` was deleted from `wasm-inline.ts` when date reconstruction moved to `eql/v3/date-reconstruction.ts`, but its doc comment survived and came to rest above `INTERNAL_CONSTRUCT` — two JSDoc blocks stacked over one constant, the first documenting nothing. The explanation it carried is still the reason `reconstructDateValue` guards on `Number.isNaN` at all, so it lands there: an unparseable stored value must come back as the raw value, not as an Invalid Date whose later `.toISOString()` throws far from the column that produced it (#742 review).
…and correct the skill
`csv()` throws unconditionally — PostgREST serializes rows server-side, so a
CSV response carries ciphertext the adapter never gets to decrypt. Nothing can
therefore push `{ kind: 'csv' }`, yet the transform survived in three places:
the `TransformOp` union, the `toDbSpace` pass-through switch, and the
`buildAndExecuteQuery` apply switch. All three go; the fail-closed throw stays
exactly as it is.
`skills/stash-supabase/SKILL.md` still listed `.csv()` among the transforms
"passed through to Supabase directly". That ships inside the `stash` tarball
and gets copied into customer repos, so it was about to become wrong guidance
in someone else's codebase. It now documents the throw and shows serializing
the decrypted rows instead.
`postprocessDecryptedRow` called `reconstructDatePaths(out, [key])` once per dotted date key, and that helper allocates a fresh shallow row copy on every call — so a row with N nested date columns paid N clones to produce one result. Collect the dotted keys and make a single call. The batching is also the correctness-preserving form for two paths under a shared prefix: inside one call the second path clones the intermediate the first already rebuilt (`targetChild` wins over `sourceChild`), so both Dates and their plaintext siblings survive. Covered by a new `profile.createdAt` / `profile.updatedAt` test, which goes red if that preference is dropped. The flat-key branch is unchanged.
…checks `checkedInsertRows` reads as though it validates the rows it returns. It validates `rowKey` and `testRunId` are strings and then does `return rows as T[]` — the encrypted columns, the ones that could plausibly be malformed, are never looked at. Renamed to `assertScopeKeys`, with a doc comment saying plainly that it re-establishes the Drizzle insert type after asserting only that the two plaintext scope keys survived encryption. Verified through `pnpm --filter @cipherstash/stack-drizzle run test:types`, which is the script driving `tsconfig.typecheck.json` — now widened to `integration/**/*.ts`. Confirmed the gate really compiles this file by planting a deliberate type error in it and watching the run exit 1.
… declared
The parity assertion lived in the CLI's unit suite and reached the wizard
through `../../../../../../wizard/src/lib/install-skills.js` — a deep relative
path into a package `packages/cli` declares no dependency on, targeting a
non-published internal module. If the wizard relocated that file the only
signal was a module-not-found in a different package's test run.
Chose option (a), moving it to `e2e/`, over adding a devDependency and a
comment (option (b)), for three reasons:
- `e2e/` already declares BOTH `stash` and `@cipherstash/wizard` as workspace
dependencies, so the layering is honest without widening the CLI's dep graph
for a single assertion.
- `e2e` has a `typecheck` script over `tests/**` and CI runs it ("Typecheck
(e2e)"). `packages/cli` has no typecheck script at all, so this is the only
placement where a relocated module is a COMPILE error. Verified: pointing the
import at a non-existent path fails `pnpm --filter @cipherstash/e2e run
typecheck` with TS2307.
- Cross-package behaviour is what that workspace is for, per its own
description — and reading source rather than a built binary is an existing
idiom there (`package-managers.e2e.test.ts` Suite A), so no build is needed.
`SKILL_MAP` stays unexported from the wizard's package entry — the published
surface should not grow for a test.
The assertion also got stronger in the move: the wizard→CLI name mapping is now
a `Record<WizardIntegration, CliIntegration>`, so a new integration on either
side fails to compile or fails the key-coverage test, instead of being silently
omitted from a hand-listed set of four expectations.
…the nested-or() fix was Two changesets had been edited to delete the statement that `encryptedSupabaseV3` survives as a type-identical `@deprecated` alias. It does — `packages/stack-supabase/src/index.ts:275`, alongside the `*V3` type aliases at 282-297 — the repo's own tests import it, and `AGENTS.md` documents it. A changeset that contradicts the shipped code is worse than a terse one, so both get a tight sentence back. `adapter-release-readiness` compressed the nested-group fix into "preserves nested PostgREST boolean expressions", which reads as a formatting nicety. It was a disclosure fix: with no group recursion, `and(createdAt.gte.…,…)` was cut at the first dot into the pseudo-column `and(createdAt`, matched no encrypted column, and took the verbatim branch — so the operand reached PostgREST as plaintext against an encrypted column, under the JS property name rather than the DB column name. Users need that stated outright. Also records the `stash-supabase` skill correction (csv() throws, it is not a pass-through transform) on the changeset that already bumps `stash`.
…mments Widening `stack-drizzle`'s typecheck `include` to `integration/**/*.ts` made the surrounding comments stale, and rewriting them dropped the two things that made them actionable: the per-package error counts, and the `#778` root cause (`V3_MATRIX`'s `indexes` union in `@cipherstash/test-kit`). Those counts are the mechanism the original comment describes — "widen this include as that count comes down" is not a usable instruction once the count is gone, and "tracked separately" does not say where. Restore both, keeping the corrected scope: drizzle now type-tests every `integration/**` source, so only its `src` and runtime tests remain ungated. Comment-only; no build or gate behaviour changes.
…oduction runs `parseOrStringWithSpans` became the only parser any production path calls when the nested-group fix landed, but `parseOrString` survived alongside it and kept the entire helpers suite — including assertions that pinned the NON-recursing behaviour, e.g. `and(a.eq.1,b.eq.2),c.eq.3` yielding two conditions, one of them a junk leaf on a column literally named `and(a`. The parser with the coverage was the dead one; the live one's group recursion and source spans were exercised only indirectly, through the builder. Delete the wrapper, repoint the suite at `parseOrStringWithSpans`, and drop the now-constant `recurseGroups` parameter. A local `parseConditions` strips `sourceSpan` so every existing `toEqual` stays exactly as strict about the condition shape as it was. Adds the span-level coverage that existed nowhere: a leaf one and two groups deep, inside `not.and(`, after array and jsonb containment literals whose commas are not delimiters, with surrounding whitespace excluded, and two identical leaves getting distinct spans — plus `substituteOrStringLeaves` replacing leaves at two nesting depths. Both are real regressions tests: flipping the splice to left-to-right fails two of them, and shifting the group base offset by one fails seven. Nothing in the suite caught either mutation before.
`parseLikeNeedle` decides which `like`/`ilike` patterns can be delegated to
encrypted fuzzy matching and what literal term the search receives, but its only
coverage was two end-to-end cases in the builder suite, which pin the wire
envelope rather than the parse. Its predecessor (`replace(/^%+/, '')` plus
`pattern.includes('_')`) got the escaping wrong in both directions, so the rules
are worth holding directly: 9 of the 22 new tests fail against that predecessor.
Pins in particular that `hasUnsupportedWildcard` tracks UNESCAPED wildcards only
— an escaped `\_` is a literal and must not reject the query — and that only
unescaped leading/trailing `%` are strippable, so a literal `\%` at either end
stops the strip instead of being eaten by it.
A trailing lone backslash stays a literal backslash rather than throwing;
Postgres rejects such a pattern outright, but this needle is an approximation
and the plaintext `like` path never reaches here. Documented as deliberate in
the JSDoc so it does not get "fixed" into a throw.
Also corrects the throw's parenthetical, which still read `an internal "%" or
any "_"` — escaped metacharacters are honoured now, so it names unescaped ones.
…ate-shape break Three things this release changes that a reader could only find at runtime. `csv()` throws on every encrypted query, but the interface member is still typed `Self` with no `@deprecated` tag, so an existing call site got no editor or compile signal and would discover the removal on its first request. Tag it; the `Self` return type stays, since it is a deliberate choice to keep the chain shape. `docs/reference/supabase-sdk.md` still advertised `.csv` among the transforms passed through to Supabase — the bundled skill was corrected in this PR, the reference was not. Date reconstruction now returns the raw value when a stored value does not parse. Before this branch the native typed client pushed every date-like column through `new Date(...)` unconditionally, so an unparseable value came back as an Invalid `Date`: `instanceof Date` held and `.getTime()` yielded `NaN`. It now comes back as the raw string and that call throws a `TypeError`, while the declared column type is still `Date`. The changeset said only "leaving invalid values unchanged", which undersells it. Both `stash-encryption` and `stash-drizzle` asserted flatly that `Date` columns are reconstructed to real `Date` instances — Drizzle decrypts through the same typed client, so both now carry the caveat. Bumps `@cipherstash/stack-supabase` and `@cipherstash/stack` from patch to minor. `csv()` and `matches(column, null)` throwing where they previously did something are behavioural breaks, and the date-shape change originates in `stack`; the migrate classifier changeset was raised for the same reason. The emitted version is unchanged under rc prerelease mode with a fixed group — this is signal, not version math. Also names the parser that actually runs in the nested-group bullet, since `parseOrString` no longer exists.
fix: finish v2 removal release gates
…rate-v2 feat(cli): remove the EQL v2 migration leaf
freshtonic
self-requested a review
July 29, 2026 06:54
freshtonic
approved these changes
Jul 29, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
I'm happy to approve. Outstanding feedback has been capture in Linear.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Umbrella for #707: remove the EQL v2 authoring and emission surface repo-wide while preserving reads of existing v2 ciphertext.
Current state
The removal work is integrated on
remove-v2. This status supersedes the earlier in-flight branch snapshot in this issue's edit history.@cipherstash/stack.Encryptionfrom@cipherstash/stack/v3.@cipherstash/stack-drizzlepackage root with unsuffixed exports.encryptedSupabasefrom@cipherstash/stack-supabase..audit().Final release gate
#816 owns the remaining adapter correctness and release-verification pass: fail-closed Supabase filters, Date reconstruction parity, examples/e2e/integration typechecks, final changeset and shipped-skill reconciliation, and CLI/wizard skill parity.
Already-consumed RC changesets remain historical records. Final pending changesets and migration guidance are reconciled to the canonical 1.0 surface rather than rewriting published RC history.
Closes #707 when the final release gate is merged.