diff --git a/scripts/check-jsonb-params.mjs b/scripts/check-jsonb-params.mjs new file mode 100644 index 000000000..f3be2ddab --- /dev/null +++ b/scripts/check-jsonb-params.mjs @@ -0,0 +1,128 @@ +#!/usr/bin/env node +/** + * CI guard for the POSITIONAL jsonb double-encode footgun (#2339 / #2324 class). + * + * The legacy scripts/check-jsonb-pattern.sh only catches the template-tag form + * (`${JSON.stringify(x)}::jsonb`). It MISSES the positional-param form: + * + * engine.executeRaw(`... $3::jsonb ...`, [a, b, JSON.stringify(x)]) + * + * Under postgres.js `.unsafe(sql, params)` 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*. PGLite parses it silently, so the bug is invisible in + * unit tests and only bites on real Postgres (it aborted every sync in #2339). + * + * This scanner flags any executeRaw / executeRawDirect / .unsafe(...) call whose + * balanced argument span contains BOTH a positional `$N::jsonb` cast + * (NOT `$N::text::jsonb`, NOT `$N::text[]`) AND a `JSON.stringify(` — the exact + * double-encode shape. It is heuristic by design (whole-span correlation); the + * real backstop is the DATABASE_URL-gated e2e parity test. Keep both. + * + * Allowed forms (NOT flagged): + * - `$N::text::jsonb` + JSON.stringify (the fix: binds as text, cast parses it) + * - `$N::text[]` (the unnest path — arrays bind fine) + * - executeRawJsonb(...) (passes raw objects, not strings) + * - sql.json(x) (postgres.js native jsonb serializer) + * - a `jsonb-guard-ok` comment anywhere in the call span (explicit opt-out) + * + * Exit 0 = clean, 1 = violations found. Runs under node or bun. + */ +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join } from 'node:path'; + +// Default scan roots; overridable via argv so the guard's own test can point it +// at a fixture dir (e.g. `node check-jsonb-params.mjs /tmp/fixtures`). +const ROOTS = process.argv.slice(2).length > 0 ? process.argv.slice(2) : ['src', 'scripts']; +// executeRawDirect must precede executeRaw in the alternation so the longer name +// wins; executeRawJsonb is deliberately excluded (it passes objects). The +// optional `<...>` handles generic type args, e.g. `executeRaw<{ id: string }>(`. +// +// Only the postgres.js raw path is scanned (executeRaw/executeRawDirect/.unsafe). +// PGLite's native `this.db.query(...)` is intentionally NOT matched: its driver +// parses a text→jsonb cast natively, so the double-encode that bites postgres.js +// `.unsafe()` does not occur there (the `pglite-masks` invariant). The engine +// parity test pins that the resulting jsonb_typeof agrees across both engines. +const CALL_RE = /\b(executeRawDirect|executeRaw|unsafe)\s*(?:<[^>;]*>)?\s*\(/g; + +/** Walk from the '(' at openIdx and return [start,end) of the balanced span, + * respecting strings, template literals, and comments. */ +function findSpan(src, openIdx) { + let depth = 0; + let mode = 'code'; // code | line | block | sq | dq | tpl + for (let i = openIdx; i < src.length; i++) { + const c = src[i]; + const n = src[i + 1]; + if (mode === 'line') { if (c === '\n') mode = 'code'; continue; } + if (mode === 'block') { if (c === '*' && n === '/') { mode = 'code'; i++; } continue; } + if (mode === 'sq') { if (c === '\\') { i++; continue; } if (c === "'") mode = 'code'; continue; } + if (mode === 'dq') { if (c === '\\') { i++; continue; } if (c === '"') mode = 'code'; continue; } + if (mode === 'tpl') { if (c === '\\') { i++; continue; } if (c === '`') mode = 'code'; continue; } + // mode === 'code' + if (c === '/' && n === '/') { mode = 'line'; i++; continue; } + if (c === '/' && n === '*') { mode = 'block'; i++; continue; } + if (c === "'") { mode = 'sq'; continue; } + if (c === '"') { mode = 'dq'; continue; } + if (c === '`') { mode = 'tpl'; continue; } + if (c === '(') depth++; + else if (c === ')') { depth--; if (depth === 0) return [openIdx + 1, i]; } + } + return [openIdx + 1, src.length]; +} + +/** Blank out comments so a commented-out example doesn't trip the JSON.stringify probe. */ +function stripComments(s) { + return s.replace(/\/\/[^\n]*/g, '').replace(/\/\*[\s\S]*?\*\//g, ''); +} + +const violations = []; + +function scanFile(file) { + const src = readFileSync(file, 'utf8'); + CALL_RE.lastIndex = 0; + let m; + while ((m = CALL_RE.exec(src))) { + const method = m[1]; + const openIdx = m.index + m[0].length - 1; // index of the '(' + const [s, e] = findSpan(src, openIdx); + const span = src.slice(s, e); + if (/jsonb-guard-ok/.test(span)) continue; + if (!/JSON\.stringify\s*\(/.test(stripComments(span))) continue; + // A positional `$N::jsonb` that is NOT `$N::text::jsonb`. + const jsonbRe = /\$\d+\s*::\s*jsonb\b/g; + let j; + let badText = ''; + while ((j = jsonbRe.exec(span))) { + const pre = span.slice(Math.max(0, j.index - 12), j.index); + if (/::\s*text\s*$/.test(pre)) continue; // $N::text::jsonb is the fix — allowed + badText = j[0].replace(/\s+/g, ''); + break; + } + if (!badText) continue; + const line = src.slice(0, s).split('\n').length; + violations.push( + `${file}:${line} ${method}(...) binds JSON.stringify into ${badText} — use $N::text::jsonb or pass a raw object (executeRawJsonb / sql.json)`, + ); + } +} + +function walk(dir) { + let ents; + try { ents = readdirSync(dir); } catch { return; } + for (const ent of ents) { + if (ent === 'node_modules') continue; + const p = join(dir, ent); + const st = statSync(p); + if (st.isDirectory()) walk(p); + else if (p.endsWith('.ts') && !p.endsWith('.test.ts')) scanFile(p); + } +} + +for (const root of ROOTS) walk(root); + +if (violations.length) { + console.error('JSONB positional double-encode violations (#2339 class):\n'); + for (const v of violations) console.error(' ' + v); + console.error(`\n${violations.length} violation(s). Fix: bind through $N::text::jsonb (keeping JSON.stringify), or pass a raw object via executeRawJsonb / sql.json. See docs/ENGINES.md.`); + process.exit(1); +} +console.log('check-jsonb-params: clean (no positional $N::jsonb + JSON.stringify double-encodes)'); diff --git a/scripts/check-jsonb-pattern.sh b/scripts/check-jsonb-pattern.sh index 2403f7b83..d26acb0e2 100755 --- a/scripts/check-jsonb-pattern.sh +++ b/scripts/check-jsonb-pattern.sh @@ -44,3 +44,17 @@ if grep -rEn "$MAX_STALLED_PATTERN" src/schema.sql src/core/migrate.ts src/core/ fi echo "OK: max_stalled defaults are 5 in all schema sources" + +# v0.42.x (#2339 / #2324): positional `$N::jsonb` + JSON.stringify double-encode. +# The template-string grep above only catches `${JSON.stringify(x)}::jsonb`. It +# MISSES the positional-param form — executeRaw(`... $N::jsonb ...`, +# [JSON.stringify(x)]) — which is the exact shape that double-encoded the +# op_checkpoints pin and aborted every sync in #2339. The AST-lite scanner below +# catches it. `set -e` propagates its non-zero exit. +if command -v node >/dev/null 2>&1; then + node scripts/check-jsonb-params.mjs +elif command -v bun >/dev/null 2>&1; then + bun scripts/check-jsonb-params.mjs +else + echo "WARN: neither node nor bun on PATH; skipping check-jsonb-params.mjs" >&2 +fi diff --git a/src/commands/agent.ts b/src/commands/agent.ts index 4a5ae5500..771c981ac 100644 --- a/src/commands/agent.ts +++ b/src/commands/agent.ts @@ -251,7 +251,7 @@ async function runFanout(engine: BrainEngine, queue: MinionQueue, flags: RunFlag // do this after submission because each add() returns the committed // row's id; the aggregator's seed started with an empty array. await engine.executeRaw( - `UPDATE minion_jobs SET data = jsonb_set(data, '{children_ids}', $1::jsonb) WHERE id = $2`, + `UPDATE minion_jobs SET data = jsonb_set(data, '{children_ids}', $1::text::jsonb) WHERE id = $2`, [JSON.stringify(childIds), aggregator.id], ); diff --git a/src/commands/sources.ts b/src/commands/sources.ts index e6bac3b37..7e3b26deb 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -692,7 +692,7 @@ async function runFederate(engine: BrainEngine, args: string[], value: boolean): const config = parseConfig(src.config); config.federated = value; await engine.executeRaw( - `UPDATE sources SET config = $1::jsonb WHERE id = $2`, + `UPDATE sources SET config = $1::text::jsonb WHERE id = $2`, [JSON.stringify(config), id], ); console.log(`Source "${id}" is now ${value ? 'federated (appears in cross-source default search)' : 'isolated (only searched when explicitly named)'}.`); @@ -854,7 +854,7 @@ async function runWebhookSet(engine: BrainEngine, args: string[]): Promise cfg.webhook_secret = secret; cfg.github_repo = githubRepo; await engine.executeRaw( - `UPDATE sources SET config = $1::jsonb WHERE id = $2`, + `UPDATE sources SET config = $1::text::jsonb WHERE id = $2`, [JSON.stringify(cfg), id], ); @@ -910,7 +910,7 @@ async function runWebhookRotate(engine: BrainEngine, args: string[]): Promise( `INSERT INTO code_traversal_cache (symbol_qualified, depth, source_id, response_json, max_chunk_updated_at, xmin_max, cluster_generation) - VALUES ($1, $2, $3, $4::jsonb, $5::timestamptz, $6, $7) + VALUES ($1, $2, $3, $4::text::jsonb, $5::timestamptz, $6, $7) ON CONFLICT (symbol_qualified, depth, source_id) DO UPDATE SET response_json = EXCLUDED.response_json, diff --git a/src/core/conversation-parser/llm-base.ts b/src/core/conversation-parser/llm-base.ts index 78d0af205..6671b2cb0 100644 --- a/src/core/conversation-parser/llm-base.ts +++ b/src/core/conversation-parser/llm-base.ts @@ -266,13 +266,13 @@ async function writeDbCache( ): Promise { const [, , contentSha] = splitCacheKey(key); if (!contentSha) return; - // executeRaw with positional binding for JSONB. Per the sql-query.ts - // contract: object values passed via positional params reach the - // wire as proper jsonb when cast. + // #2339 class: this binds JSON.stringify(value) (a STRING) positionally, so it + // must cast through $4::text::jsonb — a bare $4::jsonb double-encodes under + // postgres.js .unsafe() (PGLite hides it). Pass a raw object to use $N::jsonb. await engine.executeRaw( `INSERT INTO conversation_parser_llm_cache (content_sha256, model_id, call_shape, value_json) - VALUES ($1, $2, $3, $4::jsonb) + VALUES ($1, $2, $3, $4::text::jsonb) ON CONFLICT (content_sha256, model_id, call_shape) DO NOTHING`, [contentSha, modelStr, shape, JSON.stringify(value)], ); diff --git a/src/core/cycle/calibration-profile.ts b/src/core/cycle/calibration-profile.ts index 05aff0234..f07724f48 100644 --- a/src/core/cycle/calibration-profile.ts +++ b/src/core/cycle/calibration-profile.ts @@ -356,7 +356,7 @@ class CalibrationProfilePhase extends BaseCyclePhase { active_bias_tags, model_id, cost_usd, judge_model_agreement ) VALUES ($1, $2, now(), false, $3, $4, $5, $6, $7, - $8::jsonb, $9::text[], + $8::text::jsonb, $9::text[], $10, $11, $12::text[], $13, NULL, NULL)`, [ diff --git a/src/core/minions/handlers/subagent.ts b/src/core/minions/handlers/subagent.ts index fcead52f3..450dc8da0 100644 --- a/src/core/minions/handlers/subagent.ts +++ b/src/core/minions/handlers/subagent.ts @@ -878,7 +878,7 @@ async function runSubagentViaGateway(args: GatewayRunArgs): Promise( `INSERT INTO subagent_tool_executions (job_id, message_idx, tool_use_id, tool_name, input, status, schema_version, ordinal, gbrain_tool_use_id, provider_id) - VALUES ($1, $2, $3, $4, $5::jsonb, 'pending', 2, $6, $7, $8) + VALUES ($1, $2, $3, $4, $5::text::jsonb, 'pending', 2, $6, $7, $8) ON CONFLICT (job_id, message_idx, ordinal) DO UPDATE SET status = subagent_tool_executions.status RETURNING gbrain_tool_use_id::text AS gbrain_tool_use_id`, @@ -891,7 +891,7 @@ async function runSubagentViaGateway(args: GatewayRunArgs): Promise { await engine.executeRaw( `UPDATE subagent_tool_executions - SET status = 'complete', output = $1::jsonb, ended_at = now() + SET status = 'complete', output = $1::text::jsonb, ended_at = now() WHERE gbrain_tool_use_id::text = $2`, [JSON.stringify(output ?? null), gbrainToolUseId], ); @@ -1107,7 +1107,7 @@ async function persistMessage(engine: BrainEngine, jobId: number, msg: Persisted await engine.executeRaw( `INSERT INTO subagent_messages (job_id, message_idx, role, content_blocks, tokens_in, tokens_out, tokens_cache_read, tokens_cache_create, model) - VALUES ($1, $2, $3, $4::jsonb, $5, $6, $7, $8, $9) + VALUES ($1, $2, $3, $4::text::jsonb, $5, $6, $7, $8, $9) ON CONFLICT (job_id, message_idx) DO NOTHING`, [ jobId, @@ -1131,13 +1131,15 @@ async function persistToolExecPending( toolName: string, input: unknown, ): Promise { - // Serialize to JSON string for the ::jsonb cast. When `input` is already a - // string (e.g. pre-serialized), avoid double-encoding which produces a jsonb - // scalar string instead of a jsonb object — breaking `input->>'key'` lookups. + // Serialize to a JSON string, then bind through $5::text::jsonb. The value is + // ALWAYS a string here (pre-serialized input, or JSON.stringify) — binding a + // string to a bare $5::jsonb double-encodes it into a jsonb scalar string under + // postgres.js .unsafe() (#2339 class; PGLite hides it). The ::text cast makes + // the text→jsonb parse produce a real jsonb object. const jsonStr = typeof input === 'string' ? input : JSON.stringify(input); await engine.executeRaw( `INSERT INTO subagent_tool_executions (job_id, message_idx, tool_use_id, tool_name, input, status) - VALUES ($1, $2, $3, $4, $5::jsonb, 'pending') + VALUES ($1, $2, $3, $4, $5::text::jsonb, 'pending') ON CONFLICT (job_id, tool_use_id) DO NOTHING`, [jobId, messageIdx, toolUseId, toolName, jsonStr], ); @@ -1151,7 +1153,7 @@ async function persistToolExecComplete( ): Promise { await engine.executeRaw( `UPDATE subagent_tool_executions - SET status = 'complete', output = $3::jsonb, ended_at = now() + SET status = 'complete', output = $3::text::jsonb, ended_at = now() WHERE job_id = $1 AND tool_use_id = $2`, [jobId, toolUseId, typeof output === 'string' ? output : JSON.stringify(output)], ); @@ -1170,7 +1172,7 @@ async function persistToolExecFailed( // rejected upfront) and "pending row exists" (tool threw mid-execute). await engine.executeRaw( `INSERT INTO subagent_tool_executions (job_id, message_idx, tool_use_id, tool_name, input, status, error, ended_at) - VALUES ($1, $2, $3, $4, $5::jsonb, 'failed', $6, now()) + VALUES ($1, $2, $3, $4, $5::text::jsonb, 'failed', $6, now()) ON CONFLICT (job_id, tool_use_id) DO UPDATE SET status = 'failed', error = EXCLUDED.error, ended_at = now()`, [jobId, messageIdx, toolUseId, toolName, typeof input === 'string' ? input : JSON.stringify(input), error], diff --git a/src/core/onboard/impact-capture.ts b/src/core/onboard/impact-capture.ts index 7db93a772..f1f622af7 100644 --- a/src/core/onboard/impact-capture.ts +++ b/src/core/onboard/impact-capture.ts @@ -120,7 +120,7 @@ export async function writeImpactLogRow( remediation_id, metric_name, metric_before, metric_after, job_id, source_id, brain_id, started_at, idempotency_key, applied_by, details - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11::jsonb)`, + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11::text::jsonb)`, [ attribution.remediation_id, metricName, diff --git a/src/core/search/query-cache.ts b/src/core/search/query-cache.ts index dd68a12e8..6d0558264 100644 --- a/src/core/search/query-cache.ts +++ b/src/core/search/query-cache.ts @@ -236,7 +236,7 @@ export class SemanticQueryCache { // the v0.40.3.0 IRON-RULE). await this.engine.executeRaw( `INSERT INTO query_cache (id, query_text, source_id, knobs_hash, embedding, results, meta, ttl_seconds, page_generations, max_generation_at_store, created_at) - VALUES ($1, $2, $3, $4, $5::vector, $6::jsonb, $7::jsonb, $8, $9::jsonb, $10, now()) + VALUES ($1, $2, $3, $4, $5::vector, $6::text::jsonb, $7::text::jsonb, $8, $9::text::jsonb, $10, now()) ON CONFLICT (id) DO UPDATE SET query_text = EXCLUDED.query_text, knobs_hash = EXCLUDED.knobs_hash, diff --git a/src/core/sources-ops.ts b/src/core/sources-ops.ts index 3cb1c0a9a..a7d95ee77 100644 --- a/src/core/sources-ops.ts +++ b/src/core/sources-ops.ts @@ -408,7 +408,7 @@ export async function addSource( try { await engine.executeRaw( `INSERT INTO sources (id, name, local_path, config) - VALUES ($1, $2, $3, $4::jsonb)`, + VALUES ($1, $2, $3, $4::text::jsonb)`, [opts.id, displayName, finalPath, JSON.stringify(config)], ); } catch (e) { @@ -454,7 +454,7 @@ export async function addSource( const displayName = opts.name ?? opts.id; await engine.executeRaw( `INSERT INTO sources (id, name, local_path, config) - VALUES ($1, $2, $3, $4::jsonb)`, + VALUES ($1, $2, $3, $4::text::jsonb)`, [opts.id, displayName, finalPath, JSON.stringify(config)], ); } diff --git a/src/core/takes-quality-eval/receipt-write.ts b/src/core/takes-quality-eval/receipt-write.ts index 5bb77f719..637670dae 100644 --- a/src/core/takes-quality-eval/receipt-write.ts +++ b/src/core/takes-quality-eval/receipt-write.ts @@ -32,8 +32,8 @@ export async function writeReceiptToDb(engine: BrainEngine, receipt: TakesQualit receipt_json, receipt_disk_path, created_at ) VALUES ( $1, $2, $3, $4, - $5, $6, $7, $8::jsonb, $9, - $10::jsonb, $11, $12::timestamptz + $5, $6, $7, $8::text::jsonb, $9, + $10::text::jsonb, $11, $12::timestamptz ) ON CONFLICT (receipt_sha8_corpus, receipt_sha8_prompt, receipt_sha8_models, receipt_sha8_rubric) DO NOTHING`, diff --git a/test/check-jsonb-params.test.ts b/test/check-jsonb-params.test.ts new file mode 100644 index 000000000..26e1b43ed --- /dev/null +++ b/test/check-jsonb-params.test.ts @@ -0,0 +1,83 @@ +/** + * Self-test for scripts/check-jsonb-params.mjs — the positional jsonb + * double-encode guard (#2339 / #2324 class). Verifies it catches the bug shape + * (including generic-typed calls and the `jsonStr` variable case is acknowledged + * as out of scope) and does NOT false-positive on the sanctioned forms. + * + * Fixtures are written to a temp dir and the scanner is pointed at it via argv, + * so this never touches the real src/ tree. + */ +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; + +const SCRIPT = join(import.meta.dir, '..', 'scripts', 'check-jsonb-params.mjs'); + +let root: string; +let badDir: string; +let goodDir: string; + +function runGuard(dir: string): { code: number; err: string } { + const res = Bun.spawnSync([process.execPath, SCRIPT, dir]); + return { code: res.exitCode, err: res.stderr.toString() + res.stdout.toString() }; +} + +beforeAll(() => { + root = mkdtempSync(join(tmpdir(), 'jsonb-guard-')); + badDir = join(root, 'bad'); + goodDir = join(root, 'good'); + mkdirSync(badDir, { recursive: true }); + mkdirSync(goodDir, { recursive: true }); + + // BAD: positional $1::jsonb bound to a JSON.stringify'd value. + writeFileSync( + join(badDir, 'bad.ts'), + "await engine.executeRaw(`INSERT INTO t (a) VALUES ($1::jsonb)`, [JSON.stringify(x)]);\n", + ); + // BAD: generic-typed executeRaw(...) must still be caught. + writeFileSync( + join(badDir, 'bad_generic.ts'), + "await engine.executeRaw<{ id: string }>(`UPDATE t SET a = $2::jsonb WHERE id = $1`, [id, JSON.stringify(x)]);\n", + ); + + // GOOD: the fix — $1::text::jsonb. + writeFileSync( + join(goodDir, 'good_text.ts'), + "await engine.executeRaw(`INSERT INTO t (a) VALUES ($1::text::jsonb)`, [JSON.stringify(x)]);\n", + ); + // GOOD: text[] array path (the appendCompleted unnest shape). + writeFileSync( + join(goodDir, 'good_array.ts'), + "await engine.executeRaw(`INSERT INTO t (a) SELECT unnest($1::text[])`, [JSON.stringify(arr)]);\n", + ); + // GOOD: executeRawJsonb passes a raw object, not a string — excluded. + writeFileSync( + join(goodDir, 'good_helper.ts'), + "await executeRawJsonb(engine, `INSERT INTO t (a) VALUES ($1::jsonb)`, [], [JSON.stringify(x)]);\n", + ); + // GOOD: explicit opt-out for a rare legitimate object-binding case. + writeFileSync( + join(goodDir, 'good_optout.ts'), + "await engine.executeRaw(`INSERT INTO t (a) VALUES ($1::jsonb)` /* jsonb-guard-ok */, [JSON.stringify(x)]);\n", + ); +}); + +afterAll(() => { + rmSync(root, { recursive: true, force: true }); +}); + +describe('check-jsonb-params guard', () => { + test('flags positional $N::jsonb + JSON.stringify (incl. generic-typed calls)', () => { + const { code, err } = runGuard(badDir); + expect(code).toBe(1); + expect(err).toContain('bad.ts'); + expect(err).toContain('bad_generic.ts'); + }); + + test('passes the sanctioned forms (::text::jsonb, ::text[], executeRawJsonb, opt-out)', () => { + const { code, err } = runGuard(goodDir); + expect(code).toBe(0); + expect(err).toContain('clean'); + }); +});