From 95acb2f360f0f18b37d7bdb2a1ef36cf2d4e3338 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 23 Jun 2026 15:04:31 -0700 Subject: [PATCH] =?UTF-8?q?fix(sync):=20op=5Fcheckpoints=20pin=20write=20d?= =?UTF-8?q?ouble-encodes=20jsonb=20=E2=80=94=20every=20sync=20aborts=20(#2?= =?UTF-8?q?339)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit recordCompleted bound JSON.stringify(array) to a $3::jsonb param via postgres.js .unsafe(), double-encoding it into a jsonb string scalar that violates the v119 op_checkpoints_completed_keys_array CHECK — aborting every multi-source sync on real Postgres at the first checkpoint write. PGLite parses the string silently, which is why unit tests stayed green and it shipped. Cast through $3::text::jsonb so the text->jsonb cast parses a genuine array. Adds a DATABASE_URL-gated parity test + a dedicated Postgres CI job so the guard can never silently skip. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/e2e.yml | 43 +++++++++++ src/core/op-checkpoint.ts | 17 +++-- src/core/sql-query.ts | 25 ++++--- test/e2e/op-checkpoint-jsonb-parity.test.ts | 80 +++++++++++++++++++++ 4 files changed, 151 insertions(+), 14 deletions(-) create mode 100644 test/e2e/op-checkpoint-jsonb-parity.test.ts diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 74717c3ea..5142a47c2 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -20,6 +20,49 @@ concurrency: cancel-in-progress: true jobs: + jsonb-parity: + # Dedicated required guard for the JSONB double-encode bug-class (#2339). + # PGLite parses a double-encoded jsonb string silently, so this assertion can + # ONLY be made on real Postgres — a normal gated e2e file would skip without + # DATABASE_URL and let the bug ship green (as #2339 did). This job provisions + # Postgres and HARD-FAILS if DATABASE_URL is missing, so the guard can never + # silently skip. + name: JSONB parity (#2339 regression guard) + runs-on: ubuntu-latest + timeout-minutes: 15 + services: + postgres: + image: pgvector/pgvector:pg16 + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: gbrain_test + ports: + - 5432:5432 + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 + steps: + - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 + - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 + with: + bun-version: 1.3.13 + - run: bun install + - name: Require DATABASE_URL (no silent skip) + env: + DATABASE_URL: postgresql://postgres:postgres@localhost:5432/gbrain_test + run: | + if [ -z "$DATABASE_URL" ]; then + echo "::error::DATABASE_URL must be set for the jsonb-parity job — the #2339 guard would silently skip (the exact failure PGLite hides). Failing the job." >&2 + exit 1 + fi + - name: Run JSONB double-encode parity tests on real Postgres + env: + DATABASE_URL: postgresql://postgres:postgres@localhost:5432/gbrain_test + run: bun test test/e2e/op-checkpoint-jsonb-parity.test.ts test/e2e/jsonb-roundtrip.test.ts + tier1: name: Tier 1 (Mechanical) runs-on: ubuntu-latest diff --git a/src/core/op-checkpoint.ts b/src/core/op-checkpoint.ts index f1c0c1e2e..8705bc73b 100644 --- a/src/core/op-checkpoint.ts +++ b/src/core/op-checkpoint.ts @@ -179,15 +179,22 @@ export async function recordCompleted( // REPLACE semantics (kept deliberately — #1794 V3). Callers like // extract-conversation-facts serialize a MUTABLE map through here and rely on // stale keys being REMOVED; an append would make them unremovable. The full - // set lands in the parent `completed_keys` JSONB column via a single UPSERT — - // exactly as before. JSON.stringify into `$3::jsonb` is correct (the text→jsonb - // cast yields a proper array; NOT the double-encode trap, which is the template - // form). Sync uses `appendCompleted` (below) instead, never this. + // set lands in the parent `completed_keys` JSONB column via a single UPSERT. + // #2339: bind through `$3::text::jsonb`, NOT `$3::jsonb`. Under postgres.js + // `.unsafe(sql, params)` (executeRawDirect's path) a JS string bound to a + // `$N::jsonb` param double-encodes — the text→jsonb cast wraps the already-JSON + // string into a jsonb *string scalar*, which fails the v119 + // `op_checkpoints_completed_keys_array CHECK (jsonb_typeof = 'array')` and aborts + // every sync on real Postgres (PGLite parses it silently, which hid the bug). + // Casting through `text` first binds it as a plain text param so the text→jsonb + // cast parses it into a genuine jsonb array. This is the positional-param form of + // the CLAUDE.md double-encode trap (the grep guard only caught the template form). + // Sync uses `appendCompleted` (below, `unnest($3::text[])`) instead, never this. const sorted = [...keys].sort(); return durableWrite(engine, key, 'write', () => engine.executeRawDirect( `INSERT INTO op_checkpoints (op, fingerprint, completed_keys, updated_at) - VALUES ($1, $2, $3::jsonb, now()) + VALUES ($1, $2, $3::text::jsonb, now()) ON CONFLICT (op, fingerprint) DO UPDATE SET completed_keys = EXCLUDED.completed_keys, updated_at = now()`, diff --git a/src/core/sql-query.ts b/src/core/sql-query.ts index 2fcae4556..320820877 100644 --- a/src/core/sql-query.ts +++ b/src/core/sql-query.ts @@ -79,15 +79,22 @@ function assertSqlValue(value: unknown): asserts value is SqlValue { * auth/admin surface that a focused helper preserves the contract without * forcing every call site to remember which positions hold JSONB. * - * Why this is safe vs the v0.12.0 double-encode bug: the bug was specific - * to postgres.js's template-tag auto-stringify path interacting with - * sql.json() — not to positional binding through `unsafe()`. JS objects - * passed as positional params reach the wire protocol with the correct - * type oid (jsonb when cast in the SQL string), so there is no double- - * encode. The CI guard (scripts/check-jsonb-pattern.sh) doesn't fire - * because the source pattern is a method call (`executeRawJsonb(...)`), - * not the banned literal-template-tag interpolation pattern with - * JSON.stringify cast to jsonb. + * Why this is safe vs the double-encode bug: this helper binds a JS **object** + * (not a pre-stringified string) to each `$N::jsonb` position. postgres.js + * `unsafe()` and PGLite both serialize a JS object to the jsonb wire type + * correctly, so there is no double-encode. + * + * IMPORTANT (the #2339 distinction): positional binding is NOT universally safe. + * Binding `JSON.stringify(x)` (a **string**) to a `$N::jsonb` position via + * `unsafe()`/`executeRawDirect` DOES double-encode — the text→jsonb cast wraps + * the already-JSON string into a jsonb *string scalar* (PGLite hides it; real + * Postgres exposes it, and it broke every sync in #2339). The fixes are: pass a + * raw object (this helper), or cast through `$N::text::jsonb` so the string is + * parsed, never `$N::jsonb` + JSON.stringify. The legacy grep guard + * (scripts/check-jsonb-pattern.sh) only caught the template-tag form; the + * positional `$N::jsonb` + JSON.stringify form is caught by the AST guard + * scripts/check-jsonb-params.mjs. This helper's `executeRawJsonb(...)` method-call + * shape trips neither guard because it passes objects, which is correct. * * Usage: * await executeRawJsonb( diff --git a/test/e2e/op-checkpoint-jsonb-parity.test.ts b/test/e2e/op-checkpoint-jsonb-parity.test.ts new file mode 100644 index 000000000..42f8abdc7 --- /dev/null +++ b/test/e2e/op-checkpoint-jsonb-parity.test.ts @@ -0,0 +1,80 @@ +/** + * E2E: op_checkpoints.completed_keys JSONB parity — #2339 regression guard. + * + * #2339: `recordCompleted` bound `JSON.stringify(array)` to a `$3::jsonb` param + * via postgres.js `.unsafe()` (executeRawDirect). That double-encodes the value + * into a jsonb *string scalar*, which violates the v119 + * `op_checkpoints_completed_keys_array CHECK (jsonb_typeof = 'array')` and aborts + * EVERY sync on real Postgres at the first checkpoint write. PGLite's driver + * parses the string silently, which is exactly why the unit suite stayed green + * and the bug shipped — so this assertion can ONLY be made on real Postgres. + * + * This file uses the standard `hasDatabase()` skip gate (consistent with the + * other e2e tests). The X2-A guarantee that it actually RUNS lives in a dedicated + * CI job (.github/workflows) that provisions a Postgres service so DATABASE_URL + * is always present there — rather than a fail-on-skip hack inside this file, + * which would red-fail legitimate DB-less local runs. + * + * Fix under test: `$3::text::jsonb` binds the value as text, so the text→jsonb + * cast parses it into a genuine jsonb array. + */ +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { hasDatabase, setupDB, teardownDB, getEngine, getConn } from './helpers.ts'; +import { recordCompleted, loadOpCheckpoint } from '../../src/core/op-checkpoint.ts'; + +const describeE2E = hasDatabase() ? describe : describe.skip; + +describeE2E('E2E: op_checkpoints completed_keys jsonb parity (#2339)', () => { + beforeAll(async () => { + await setupDB(); + }); + afterAll(async () => { + await teardownDB(); + }); + + const key = { op: 'sync-target', fingerprint: 'jsonb-parity-2339' }; + + test('recordCompleted stores completed_keys as a jsonb ARRAY, not a double-encoded scalar', async () => { + const engine = getEngine(); + + // Pre-fix this throws SQLSTATE 23514 (the CHECK), durableWrite exhausts its + // retries, and recordCompleted returns false. Post-fix it stores a real array. + const ok = await recordCompleted(engine, key, ['b/2.md', 'a/1.md', 'c/3.md']); + expect(ok).toBe(true); + + const sql = getConn(); + const [row] = await sql` + SELECT jsonb_typeof(completed_keys) AS t, + jsonb_array_length(completed_keys) AS len, + completed_keys ->> 0 AS first + FROM op_checkpoints + WHERE op = ${key.op} AND fingerprint = ${key.fingerprint} + `; + expect(row.t).toBe('array'); // pre-fix: 'string' (scalar) — the bug + expect(Number(row.len)).toBe(3); + expect(row.first).toBe('a/1.md'); // recordCompleted sorts the set + }, 30_000); + + test('loadOpCheckpoint round-trips the recorded set', async () => { + const engine = getEngine(); + const got = await loadOpCheckpoint(engine, key); + expect(new Set(got)).toEqual(new Set(['a/1.md', 'b/2.md', 'c/3.md'])); + }, 30_000); + + test('REPLACE semantics: re-recording a smaller set drops stale keys (stays an array)', async () => { + const engine = getEngine(); + const ok = await recordCompleted(engine, key, ['only/1.md']); + expect(ok).toBe(true); + + const got = await loadOpCheckpoint(engine, key); + expect(new Set(got)).toEqual(new Set(['only/1.md'])); + + const sql = getConn(); + const [row] = await sql` + SELECT jsonb_typeof(completed_keys) AS t + FROM op_checkpoints + WHERE op = ${key.op} AND fingerprint = ${key.fingerprint} + `; + expect(row.t).toBe('array'); + }, 30_000); +});