Skip to content

Commit 11fe4ec

Browse files
committed
fix(stack): address code-review findings on the v3 integration harness (round 2)
- types.ts: exclude ORE (`*_ord_ore`) columns from `V3OrderableKeys` at COMPILE time (they are orderAndRange-capable but not sortable through a jsonb path), so `.order(oreCol)` is a type error matching the runtime rejection — no more green-typecheck-then-500. Pinned with a `@ts-expect-error` in the type tests. - relational: re-add the deleted `not(between())` precedence test — `between` emits a two-clause conjunction and the passthrough `not` relies on `v3Dialect.range` parenthesising it; the only surviving `not` test wrapped a single-clause `eq` and could not catch a paren regression. Asserts ROW_C is kept, which the buggy `value < bound` form would drop. - relational: run-scope the table names (`_${RID}`) and DROP them in teardown, like the family suites — a fixed name races a concurrent run's `beforeAll` DROP on a shared/persistent DB. - supabase adapter `expectRejected`: stop counting an arbitrary throw as a valid rejection (it matched its own sentinel by message substring). Now a thrown rejection must carry the `[supabase v3]` marker; a stray TypeError/network error rethrows and fails the test. - query-builder-v3: drop the cached `selectKeyToDb` field (a side effect of `buildSelectString` read later by `postprocessDecryptedRow`) — derive it inline from `this.selectColumns` so a reused builder can't postprocess with a stale select map. - vitest.shared.ts: add the bare `@cipherstash/stack` runtime alias (last, so the subpaths still win) — both sibling tsconfigs map it, and without it a bare- specifier importer would resolve to `dist/`, re-coupling `pnpm test` to a build. - integration workflows: broaden the path filters to the source layers the adapters' encoding rests on (`src/encryption`, `src/schema`, `packages/schema`) so a break there triggers the live wire/round-trip suites. Note: the `orderColumnName` bare-fallback finding was investigated and found to be a non-issue — `validateTransforms` runs before the query executes and rejects the column with a domain-specific message, so the bare name is only an intermediate value on an about-to-be-rejected request. Corrected the misleading comment instead of adding a throw (which regressed that message). Claude-Session: https://claude.ai/code/session_01MxTTPaPP16m6br7Hoab94w
1 parent 02750c6 commit 11fe4ec

9 files changed

Lines changed: 140 additions & 58 deletions

File tree

.changeset/supabase-v3-order-by-ope-term.md

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@ The guard is now on the ordering FLAVOUR, not on encryption:
2626
- **`ore` present → rejected.** The `ob` term is an array of ORE blocks whose
2727
comparison needs the superuser-only operator class, which no jsonb path can
2828
reach. (Such a column cannot hold data on managed Postgres anyway: its domain
29-
CHECK raises `ore_domain_unavailable`.)
29+
CHECK raises `ore_domain_unavailable`.) ORE columns are now excluded from
30+
`order()` at COMPILE time too, not only at runtime — `.order(oreColumn)` is a
31+
type error, matching the rejection.
3032
- **neither → rejected.** Storage-only, equality-only and match-only columns
3133
carry no ordering term.
3234

@@ -38,6 +40,8 @@ numeric and date domains, and per-character (16 hex chars each) for text, so
3840
lexicographic order reproduces plaintext order including the prefix case
3941
(`ada` < `adam`). `ope-term.integration.test.ts` pins that shape.
4042

41-
`V3OrderableKeys` widens to match, so `order()` on an ordering column
42-
typechecks. `is(col, true)` is unaffected — it stays plaintext-only, and now has
43-
its own `V3PlaintextKeys` rather than borrowing the orderable set.
43+
`V3OrderableKeys` widens to admit OPE-backed ordering columns (`*_ord`,
44+
`text_ord`, `text_search`) while still excluding ORE (`*_ord_ore`) columns, so
45+
`order()` typechecks exactly where it works. `is(col, true)` is unaffected — it
46+
stays plaintext-only, and now has its own `V3PlaintextKeys` rather than
47+
borrowing the orderable set.

.github/workflows/integration-drizzle.yml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,12 @@ on:
1818
branches: [main]
1919
paths:
2020
- 'packages/stack/src/eql/v3/**'
21+
# Source layers the adapter's encoding/round-trip rests on: a break here
22+
# (not just under src/eql/v3) can produce wrong rows, so trigger the live
23+
# suite that would catch it.
24+
- 'packages/stack/src/encryption/**'
25+
- 'packages/stack/src/schema/**'
26+
- 'packages/schema/**'
2127
- 'packages/stack/integration/**'
2228
- 'packages/test-kit/**'
2329
- 'packages/cli/src/installer/**'
@@ -31,6 +37,12 @@ on:
3137
# Repeated verbatim: GitHub Actions does not support YAML anchors/aliases.
3238
paths:
3339
- 'packages/stack/src/eql/v3/**'
40+
# Source layers the adapter's encoding/round-trip rests on: a break here
41+
# (not just under src/eql/v3) can produce wrong rows, so trigger the live
42+
# suite that would catch it.
43+
- 'packages/stack/src/encryption/**'
44+
- 'packages/stack/src/schema/**'
45+
- 'packages/schema/**'
3446
- 'packages/stack/integration/**'
3547
- 'packages/test-kit/**'
3648
- 'packages/cli/src/installer/**'

.github/workflows/integration-supabase.yml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,12 @@ on:
1414
paths:
1515
- 'packages/stack/src/supabase/**'
1616
- 'packages/stack/src/eql/v3/**'
17+
# Source layers the adapter's encoding/round-trip rests on: a break here
18+
# (not just under src/supabase) can produce wrong wire output or rows, so
19+
# trigger the live suite that would catch it.
20+
- 'packages/stack/src/encryption/**'
21+
- 'packages/stack/src/schema/**'
22+
- 'packages/schema/**'
1723
- 'packages/stack/integration/**'
1824
- 'packages/test-kit/**'
1925
- 'packages/cli/src/installer/**'
@@ -26,6 +32,12 @@ on:
2632
paths:
2733
- 'packages/stack/src/supabase/**'
2834
- 'packages/stack/src/eql/v3/**'
35+
# Source layers the adapter's encoding/round-trip rests on: a break here
36+
# (not just under src/supabase) can produce wrong wire output or rows, so
37+
# trigger the live suite that would catch it.
38+
- 'packages/stack/src/encryption/**'
39+
- 'packages/stack/src/schema/**'
40+
- 'packages/schema/**'
2941
- 'packages/stack/integration/**'
3042
- 'packages/test-kit/**'
3143
- 'packages/cli/src/installer/**'

packages/stack/__tests__/supabase-v3.test-d.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ const users = encryptedTable('users', {
1818
active: types.Boolean('active'),
1919
nickname: types.TextEq('nickname'),
2020
bio: types.TextMatch('bio'),
21+
score: types.IntegerOrdOre('score'),
2122
})
2223

2324
type UserRow = InferPlaintext<typeof users>
@@ -130,6 +131,11 @@ describe('encryptedSupabaseV3 typed surface (with schemas)', () => {
130131
builder.order('nickname')
131132
// @ts-expect-error — bio is public.eql_v3_text_match: match only
132133
builder.order('bio')
134+
// @ts-expect-error — score is public.eql_v3_integer_ord_ore: ORE-backed, so
135+
// orderAndRange-capable but NOT sortable through a jsonb path (its `ob` term
136+
// needs the superuser-only ORE opclass). Excluded at compile time to match
137+
// the runtime rejection.
138+
builder.order('score')
133139
})
134140

135141
it('still allows order() on a plaintext row key', async () => {

packages/stack/integration/drizzle-v3/relational.integration.test.ts

Lines changed: 41 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,15 @@ import { makeEqlV3Column } from '@/eql/v3/drizzle/column'
5252

5353
const sqlClient = postgres(databaseUrl(), { prepare: false })
5454

55-
const TABLE_NAME = 'protect_ci_v3_drizzle_matrix'
56-
const ACCOUNT_TABLE_NAME = 'protect_ci_v3_drizzle_matrix_accounts'
57-
const RUN = `run-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`
55+
// Per-run table suffix so two runs sharing a database (a persistent/reused CI
56+
// database, a developer's local DB, or re-enabled file parallelism) never
57+
// operate on the same physical table — one run's `beforeAll` DROP would
58+
// otherwise blow away a table another run is mid-query on. Mirrors the family
59+
// suites' run-scoped naming (`rows.ts` `planTable`).
60+
const RID = Math.random().toString(36).slice(2, 8)
61+
const TABLE_NAME = `protect_ci_v3_drizzle_matrix_${RID}`
62+
const ACCOUNT_TABLE_NAME = `protect_ci_v3_drizzle_matrix_accounts_${RID}`
63+
const RUN = `run-${Date.now()}-${RID}`
5864
const ROW_A = 'row-a'
5965
const ROW_B = 'row-b'
6066
// A third row. With only two rows every predicate can return just [A], [B],
@@ -103,7 +109,7 @@ const accountsTable = pgTable(ACCOUNT_TABLE_NAME, {
103109
// known at compile time, so it exercises A3 end-to-end with ZERO casts: the
104110
// insert takes envelope rows and the select yields `Encrypted` values ready for
105111
// decrypt.
106-
const BIGINT_TABLE_NAME = 'protect_ci_v3_drizzle_bigint'
112+
const BIGINT_TABLE_NAME = `protect_ci_v3_drizzle_bigint_${RID}`
107113
const bigintTable = pgTable(BIGINT_TABLE_NAME, {
108114
id: integer('id').primaryKey().generatedAlwaysAsIdentity(),
109115
rowKey: text('row_key').notNull(),
@@ -218,12 +224,13 @@ beforeAll(async () => {
218224
.map(([eqlType]) => `"${slug(eqlType)}" ${eqlType} NOT NULL`)
219225
.join(',\n ')
220226

221-
// DROP, not `CREATE TABLE IF NOT EXISTS`. A table left by an earlier run keeps
222-
// its old columns, so a change to the catalog silently reuses the stale schema.
223-
// That bit: after the `_ord_ore` domains were filtered out of `matrixEntries`,
224-
// the leftover table still carried its nine ORE columns, and on managed
225-
// Postgres every INSERT raised `ore_domain_unavailable` — even leaving those
226-
// columns NULL, because the domain CHECK calls a function that RAISEs.
227+
// Table names are run-scoped (see RID), so DROP IF EXISTS is normally a no-op;
228+
// it stays as a belt-and-braces guard against an id collision. Recreating from
229+
// scratch each run also means a catalog change can never silently reuse a
230+
// stale schema — the bug that bit once when the `_ord_ore` domains were
231+
// filtered out of `matrixEntries` but a leftover fixed-name table kept its nine
232+
// ORE columns, making every INSERT raise `ore_domain_unavailable` on managed
233+
// Postgres (the domain CHECK RAISEs even for a NULL value).
227234
await sqlClient.unsafe(`
228235
DROP TABLE IF EXISTS ${TABLE_NAME}
229236
`)
@@ -299,9 +306,11 @@ beforeAll(async () => {
299306
}, 120000)
300307

301308
afterAll(async () => {
302-
await sqlClient`DELETE FROM ${sqlClient(TABLE_NAME)} WHERE test_run_id = ${RUN}`
303-
await sqlClient`DELETE FROM ${sqlClient(ACCOUNT_TABLE_NAME)} WHERE test_run_id = ${RUN}`
304-
await sqlClient`DELETE FROM ${sqlClient(BIGINT_TABLE_NAME)} WHERE test_run_id = ${RUN}`
309+
// Tables are run-scoped, so drop them outright rather than DELETE-ing rows —
310+
// no other run shares them, and this leaves nothing behind on a persistent DB.
311+
await sqlClient.unsafe(`DROP TABLE IF EXISTS ${TABLE_NAME}`)
312+
await sqlClient.unsafe(`DROP TABLE IF EXISTS ${ACCOUNT_TABLE_NAME}`)
313+
await sqlClient.unsafe(`DROP TABLE IF EXISTS ${BIGINT_TABLE_NAME}`)
305314
await sqlClient.end()
306315
}, 30000)
307316

@@ -449,6 +458,25 @@ describe('v3 drizzle — relational, needle guards, pagination', () => {
449458
expect(rows).toEqual([ROW_B, ROW_C])
450459
}, 30000)
451460

461+
it('not(between()) negates the whole range, not just the lower bound', async () => {
462+
// integer_ord: ROW_A=0, ROW_B=-42, ROW_C=2147483647. `not(between(0, 0))`
463+
// must return every row whose value != 0 → ROW_B and ROW_C.
464+
//
465+
// `between` emits a TWO-clause conjunction, and Drizzle's passthrough `not`
466+
// renders a bare `NOT <cond>`. Postgres binds NOT tighter than AND, so this
467+
// only works because `v3Dialect.range` already parenthesises `(gte AND lte)`.
468+
// Without those parens, `NOT gte(0) AND lte(0)` parses as
469+
// `value < 0 AND value <= 0` = `value < 0` = ROW_B alone, silently dropping
470+
// ROW_C. Asserting ROW_C is present is what discriminates the two — a
471+
// single-bound complement would satisfy the buggy form too.
472+
const rows = await selectRowKeys(
473+
ops.not(
474+
await ops.between(matrixColumn('public.eql_v3_integer_ord'), 0, 0),
475+
),
476+
)
477+
expect(rows).toEqual([ROW_B, ROW_C])
478+
}, 30000)
479+
452480
it('isNull and isNotNull work on nullable encrypted columns', async () => {
453481
expect(await selectRowKeys(ops.isNull(matrixTable.nullableTextEq))).toEqual(
454482
[ROW_A],

packages/stack/integration/supabase/adapter.ts

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -205,26 +205,27 @@ export function makeSupabaseAdapter(): IntegrationAdapter {
205205
},
206206

207207
async expectRejected(_table: TableSpec, op: QueryOp) {
208-
// The guards fire in two places: capability checks throw synchronously at
209-
// call time, while a term that reaches `execute` surfaces as a Result
210-
// error. Accept either; the point is that the query never runs.
208+
// The rejection fires in one of two places: a capability check throws
209+
// synchronously while collecting terms (marked `[supabase v3]`), or a term
210+
// that reaches `execute` comes back as a Result error. Accept either — but
211+
// NOT an arbitrary throw. A `TypeError` while assembling the term, or a
212+
// network failure, is not the refusal this test asserts; counting it would
213+
// let the test pass for the wrong reason if the real capability guard were
214+
// removed. The `try` wraps only `applyOp`, and the "not rejected" error is
215+
// thrown OUTSIDE it, so it can never be swallowed as a rejection.
216+
let error: { message: string } | null
211217
try {
212-
const { error } = await applyOp(op)
213-
if (!error) {
214-
throw new Error(
215-
`Expected ${op.kind}("${op.column}") to be rejected, but it succeeded`,
216-
)
217-
}
218+
;({ error } = await applyOp(op))
218219
} catch (cause) {
219-
if (
220-
cause instanceof Error &&
221-
cause.message.startsWith('Expected ') &&
222-
cause.message.includes('to be rejected')
223-
) {
224-
throw cause
220+
if (cause instanceof Error && cause.message.includes('[supabase v3]')) {
221+
return // a modeled capability rejection
225222
}
226-
// A thrown guard is a rejection.
223+
throw cause // an unexpected error — fail loudly
227224
}
225+
if (error) return // an execute-time DB refusal (e.g. a PostgREST error)
226+
throw new Error(
227+
`Expected ${op.kind}("${op.column}") to be rejected, but it succeeded`,
228+
)
228229
},
229230
}
230231
}

packages/stack/src/supabase/query-builder-v3.ts

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -146,15 +146,6 @@ export class EncryptedQueryBuilderV3Impl<
146146
private columnSchemas: Record<string, ColumnSchema>
147147
/** Column builders keyed by BOTH property name and DB name. */
148148
private v3Columns: Record<string, V3ColumnLike>
149-
/**
150-
* Result-row key → DB column name for the columns the current select
151-
* produces, including caller-chosen PostgREST aliases (`ts:createdAt` keys
152-
* rows by `ts`). Populated by {@link buildSelectString}; consumed by
153-
* {@link postprocessDecryptedRow} so aliased date columns still get `Date`
154-
* reconstruction. Empty on paths with no recorded select (an insert or update
155-
* that returns rows), which fall back to the static property/DB names.
156-
*/
157-
private selectKeyToDb: Record<string, string> = Object.create(null)
158149

159150
constructor(
160151
tableName: string,
@@ -300,8 +291,13 @@ export class EncryptedQueryBuilderV3Impl<
300291
* `text_search`) — and every collation orders digits before letters and hex
301292
* letters among themselves. `match-bloom`'s sibling assertion pins that shape.
302293
*
303-
* `validateTransforms` has already rejected every encrypted column that lacks
304-
* an `ope` index, so reaching the jsonb path here implies the term exists.
294+
* This runs at column-name-mapping time (`transformToDbSpace`), BEFORE
295+
* `buildAndExecuteQuery` calls `validateTransforms`. For an encrypted column
296+
* with no `ope` index it therefore returns a bare `dbName` here — a name that
297+
* would sort by `jsonb_cmp` over the ciphertext if it reached PostgREST — but
298+
* it never does: `validateTransforms` throws (with a domain-specific reason)
299+
* before the query executes, so the bare name is only ever an intermediate
300+
* value on a request that is about to be rejected.
305301
*/
306302
protected override orderColumnName(column: string): DbName {
307303
const dbName = this.dbNameFor(column)
@@ -348,7 +344,6 @@ export class EncryptedQueryBuilderV3Impl<
348344

349345
protected override buildSelectString(): DbSelect | null {
350346
if (this.selectColumns === null) return null
351-
this.selectKeyToDb = selectKeyToDbV3(this.selectColumns, this.propToDb)
352347
return addJsonbCastsV3(this.selectColumns, this.propToDb)
353348
}
354349

@@ -603,10 +598,14 @@ export class EncryptedQueryBuilderV3Impl<
603598
// Every key an encrypted column can appear under: the keys this select
604599
// actually produces (including caller-chosen aliases like `ts:createdAt`),
605600
// plus the static property and DB names as a fallback for paths that record
606-
// no select. Aliases win — `selectKeyToDb` describes the row in hand.
601+
// no select. Aliases win. Derived here from `this.selectColumns` (the row in
602+
// hand) rather than cached from `buildSelectString`, so a reused builder can
603+
// never postprocess a row with a previous operation's stale select map.
607604
const keyToDb: Record<string, string> = Object.assign(
608605
Object.create(null),
609-
this.selectKeyToDb,
606+
this.selectColumns === null
607+
? undefined
608+
: selectKeyToDbV3(this.selectColumns, this.propToDb),
610609
)
611610
for (const [property, dbName] of Object.entries(this.propToDb)) {
612611
keyToDb[property] ??= dbName

packages/stack/src/supabase/types.ts

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
import type { EncryptionClient } from '@/encryption'
22
import type { AuditConfig } from '@/encryption/operations/base-operation'
3-
import type { AnyV3Table, InferPlaintext, QueryTypesForColumn } from '@/eql/v3'
3+
import type {
4+
AnyV3Table,
5+
EqlTypeForColumn,
6+
InferPlaintext,
7+
QueryTypesForColumn,
8+
} from '@/eql/v3'
49
import type { EncryptionError } from '@/errors'
510
import type { LockContext } from '@/identity'
611
import type { EncryptedTable, EncryptedTableColumn } from '@/schema'
@@ -188,16 +193,26 @@ export type V3ContainsValue<
188193
: string
189194

190195
/**
191-
* JS property names of a v3 table's columns that carry NO `orderAndRange`
192-
* capability — storage-only, equality-only and match-only domains. They hold no
193-
* ordering term, so there is nothing for `order()` to sort by.
196+
* JS property names of a v3 table's columns that `order()` cannot sort by. Two
197+
* cases:
198+
*
199+
* 1. Columns with NO `orderAndRange` capability — storage-only, equality-only
200+
* and match-only domains hold no ordering term.
201+
* 2. ORE-backed (`*_ord_ore`) columns. They ARE `orderAndRange`-capable, but the
202+
* builder sorts encrypted columns through a jsonb path (`col->op`), and the
203+
* OPE term that path selects does not exist on an ORE domain — its `ob` term
204+
* needs the superuser-only ORE opclass no jsonb path can reach. So they are
205+
* excluded here to match the runtime rejection in `validateTransforms`,
206+
* rather than type-checking clean and throwing at execute time.
194207
*/
195208
export type NonOrderableV3Keys<Table extends AnyV3Table> = {
196209
[K in Extract<
197210
keyof V3ColumnsOfTable<Table>,
198211
string
199212
>]: 'orderAndRange' extends QueryTypesForColumn<V3ColumnsOfTable<Table>[K]>
200-
? never
213+
? EqlTypeForColumn<V3ColumnsOfTable<Table>[K]> extends `${string}_ord_ore`
214+
? K
215+
: never
201216
: K
202217
}[Extract<keyof V3ColumnsOfTable<Table>, string>]
203218

@@ -212,12 +227,11 @@ export type NonOrderableV3Keys<Table extends AnyV3Table> = {
212227
* path `col->op`, which selects the OPE term, and OPE is order-preserving. See
213228
* `EncryptedQueryBuilderV3Impl.orderColumnName`.
214229
*
215-
* ORE-backed (`*_ord_ore`) columns are `orderAndRange`-capable and so pass this
216-
* type, but are rejected at runtime: their `ob` term needs the superuser-only
217-
* ORE opclass, which no jsonb path can reach. Encoding the ordering FLAVOUR in
218-
* the type system would mean threading it through `QueryTypesForColumn`, and
219-
* such a column cannot hold data on managed Postgres anyway (its domain CHECK
220-
* raises `ore_domain_unavailable`), so the runtime guard is where it belongs.
230+
* ORE-backed (`*_ord_ore`) columns are excluded at compile time by
231+
* {@link NonOrderableV3Keys} — the builder sorts through a jsonb path that
232+
* cannot reach their superuser-only ORE opclass, so `.order()` on one is a type
233+
* error, matching the runtime rejection in `validateTransforms` (defense in
234+
* depth for the untyped `.order(someString)` path).
221235
*/
222236
export type V3OrderableKeys<
223237
Table extends AnyV3Table,

vitest.shared.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,4 +46,10 @@ export const sharedAlias: Record<string, string> = {
4646
repoRoot,
4747
'packages/stack/src/supabase/index.ts',
4848
),
49+
// Bare entry LAST: it is a prefix of every subpath above, and Vite matches in
50+
// order, so the subpaths must win first. Both sibling tsconfigs map bare
51+
// `@cipherstash/stack` → `src/index.ts`; without the runtime match here a
52+
// consumer importing the bare specifier would fall through to
53+
// `packages/stack/dist`, re-coupling `pnpm test` to a prior `pnpm build`.
54+
'@cipherstash/stack': resolve(repoRoot, 'packages/stack/src/index.ts'),
4955
}

0 commit comments

Comments
 (0)