fix: finish v2 removal release gates - #828
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: c62d23c 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 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:
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 |
tobyhede
marked this pull request as ready for review
July 29, 2026 02:55
…) 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.
coderdan
approved these changes
Jul 29, 2026
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
2 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.
Summary
referencedTablewhile encrypting only encrypted leavesVerification
pnpm --filter @cipherstash/stack-supabase test— 499 passedpnpm --filter @cipherstash/stack-supabase run test:types— 44 passedpnpm --filter @cipherstash/stack-drizzle run test:types— 7 passedCloses #816