From e0a208d7b7ee41a0735fb5a47b0fd2fc94deadd4 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 23 Jul 2026 12:02:36 -0700 Subject: [PATCH] Revert "fix(sources): stop source config re-wrapping into a growing JSON string scalar (#2829) (#2837)" This reverts commit e36251c023c1eb809991e450bf8a6a54fe9ac86a. --- src/commands/doctor.ts | 48 ------------------ src/commands/sources.ts | 13 +++-- src/core/doctor-categories.ts | 1 - src/core/sources-load.ts | 65 ++----------------------- test/doctor-source-config-shape.test.ts | 63 ------------------------ test/sources-load.test.ts | 41 ---------------- 6 files changed, 11 insertions(+), 220 deletions(-) delete mode 100644 test/doctor-source-config-shape.test.ts diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index f49bf68fd..a4c822a1a 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -528,48 +528,6 @@ export async function childTableOrphansCheck(engine: BrainEngine): Promise 'object'` is corrupted — federation and ACL settings - * on that source are read off a string instead of the settings object. Surface - * the affected sources with the repair path. The `gbrain sources` config writers - * now normalize before write, so any config-writing command self-heals the row - * (the app unwraps up to 10 nested layers); the SQL below repairs one layer - * directly for the common case. - */ -export async function checkSourceConfigShape(engine: BrainEngine): Promise { - try { - const rows = await engine.executeRaw<{ id: string; typ: string | null }>( - `SELECT id, jsonb_typeof(config) AS typ FROM sources WHERE jsonb_typeof(config) <> 'object'`, - ); - if (rows.length === 0) { - return { - name: 'source_config_shape', - status: 'ok', - message: 'All source config values are JSON objects', - }; - } - const affected = rows.map((r) => `${r.id} (${r.typ ?? 'null'})`).join(', '); - return { - name: 'source_config_shape', - status: 'warn', - message: - `${rows.length} source(s) have a non-object config — a JSON string/scalar ` + - `instead of an object (the #2829 re-wrapping bug): ${affected}. ` + - `Federation and ACL settings on these sources won't be read correctly. ` + - `Repair by running any 'gbrain sources' config write (self-heals up to 10 ` + - `nested layers), or in SQL: ` + - `UPDATE sources SET config = (config #>> '{}')::jsonb ` + - `WHERE jsonb_typeof(config) <> 'object';`, - }; - } catch (e) { - const msg = e instanceof Error ? e.message : String(e); - return { name: 'source_config_shape', status: 'warn', message: `Check failed: ${msg}` }; - } -} - export async function doctorReportRemote(engine: BrainEngine): Promise { const checks: Check[] = []; @@ -6243,12 +6201,6 @@ export async function buildChecks( progress.heartbeat('child_table_orphans'); checks.push(await childTableOrphansCheck(engine)); - // #2829: detect sources whose jsonb `config` was re-wrapped into a string - // scalar (grows a layer per read→write cycle). Non-object configs break - // federation + ACL reads; surface them with the repair path. - progress.heartbeat('source_config_shape'); - checks.push(await checkSourceConfigShape(engine)); - // v0.33: whoknows_health — fixture presence + row count. The eval // gate itself runs via `gbrain eval whoknows`; this check is the // "did you do the assignment?" signal. diff --git a/src/commands/sources.ts b/src/commands/sources.ts index 02182ddd0..cb855b3f6 100644 --- a/src/commands/sources.ts +++ b/src/commands/sources.ts @@ -53,7 +53,6 @@ import { import { loadAllSources, parseSourceConfig, - normalizeSourceConfig, isSourceFederated, type SourceRow as LoadedSourceRow, } from '../core/sources-load.ts'; @@ -712,7 +711,7 @@ async function runFederate(engine: BrainEngine, args: string[], value: boolean): config.federated = value; await engine.executeRaw( `UPDATE sources SET config = $1::text::jsonb WHERE id = $2`, - [JSON.stringify(normalizeSourceConfig(config)), id], + [JSON.stringify(config), id], ); console.log(`Source "${id}" is now ${value ? 'federated (appears in cross-source default search)' : 'isolated (only searched when explicitly named)'}.`); @@ -899,7 +898,7 @@ async function runWebhookSet(engine: BrainEngine, args: string[]): Promise cfg.github_repo = githubRepo; await engine.executeRaw( `UPDATE sources SET config = $1::text::jsonb WHERE id = $2`, - [JSON.stringify(normalizeSourceConfig(cfg)), id], + [JSON.stringify(cfg), id], ); console.log(`Webhook configured for source "${id}":`); @@ -955,7 +954,7 @@ async function runWebhookRotate(engine: BrainEngine, args: string[]): Promise = new Set([ 'flagged_pages', 'salience_health', 'scraper_junk_pages', - 'source_config_shape', 'source_routing_health', 'stub_guard_24h', 'sync_failures', diff --git a/src/core/sources-load.ts b/src/core/sources-load.ts index 92c10ffd2..a72219f03 100644 --- a/src/core/sources-load.ts +++ b/src/core/sources-load.ts @@ -45,68 +45,13 @@ export interface LoadAllSourcesOpts { federatedOnly?: boolean; } -/** - * #2829: max JSON.parse passes when unwrapping a possibly multiply-stringified - * `sources.config`. A re-wrapping bug could store config as a JSON *string - * scalar* ("{}", "\"{}\"", ...) that grows one layer per read→write cycle; the - * bound keeps a pathological value from spinning forever. - */ -const MAX_CONFIG_UNWRAP_DEPTH = 10; - -function isPlainObject(v: unknown): v is Record { - return typeof v === 'object' && v !== null && !Array.isArray(v); -} - -/** Unwrap a value that may be JSON-stringified 0..N times. Bounded; never throws. */ -function unwrapConfigLayers(config: unknown): { value: unknown; layers: number } { - let value = config; - let layers = 0; - while (typeof value === 'string' && layers < MAX_CONFIG_UNWRAP_DEPTH) { - try { - value = JSON.parse(value); - } catch { - break; - } - layers++; - } - return { value, layers }; -} - -/** - * #2829: coerce a config value to the underlying plain object before it is - * written back, fully unwrapping any accidental JSON-string nesting so a - * re-wrapping bug can't keep growing a layer on every write. Returns {} (with a - * warning) when the value never resolves to a plain object. Every `sources` - * config writer runs its config through this before `JSON.stringify` + the - * `$1::text::jsonb` cast, which converges the stored value back to a jsonb - * object. - */ -export function normalizeSourceConfig(config: unknown): Record { - const { value } = unwrapConfigLayers(config); - if (isPlainObject(value)) return value; - console.warn( - `[gbrain] source config was not a JSON object (got ${value === null ? 'null' : typeof value}); ` + - `storing {} instead. Run 'gbrain doctor' to find affected sources.`, - ); - return {}; -} - -/** - * Parse `sources.config` to a plain object regardless of driver shape (Postgres - * returns an object; PGLite returns a JSON string). #2829: also unwraps a config - * that was accidentally stored as a nested JSON string scalar, and warns once - * when more than one unwrap layer is needed (one layer is the normal PGLite - * path; two or more means the value was re-wrapped and should be repaired). - */ +/** Parse `sources.config` to a plain object regardless of driver shape. */ export function parseSourceConfig(config: unknown): Record { - const { value, layers } = unwrapConfigLayers(config); - if (layers > 1) { - console.warn( - `[gbrain] source config was stored as a ${layers}-layer nested JSON string; ` + - `it will be repaired on the next config write. Run 'gbrain doctor' to find affected sources.`, - ); + if (typeof config === 'string') { + try { return JSON.parse(config) as Record; } catch { return {}; } } - return isPlainObject(value) ? value : {}; + if (typeof config === 'object' && config !== null) return config as Record; + return {}; } /** True iff the source's config.federated field is the literal boolean true. */ diff --git a/test/doctor-source-config-shape.test.ts b/test/doctor-source-config-shape.test.ts deleted file mode 100644 index 40519a306..000000000 --- a/test/doctor-source-config-shape.test.ts +++ /dev/null @@ -1,63 +0,0 @@ -/** - * Test: `checkSourceConfigShape` (#2829 — source config string-scalar re-wrapping). - * - * Pure-helper surface — the check only consumes `engine.executeRaw`, so a - * structurally-typed mock satisfies the contract (same pattern as - * `doctor-child-orphans.test.ts`). No PGLite spin-up required. - */ - -import { describe, test, expect } from 'bun:test'; -import { checkSourceConfigShape } from '../src/commands/doctor.ts'; -import type { BrainEngine } from '../src/core/engine.ts'; - -/** Build a structurally-typed BrainEngine whose executeRaw returns per-SQL results. */ -function makeMockEngine(handler: (sql: string) => Promise): BrainEngine { - return { - executeRaw: handler, - } as unknown as BrainEngine; -} - -describe('checkSourceConfigShape (#2829)', () => { - test('all configs are objects → status:ok', async () => { - const engine = makeMockEngine(async () => []); - const result = await checkSourceConfigShape(engine); - expect(result.name).toBe('source_config_shape'); - expect(result.status).toBe('ok'); - expect(result.message).toContain('JSON objects'); - }); - - test('non-object configs → warn naming affected sources + repair hint', async () => { - const engine = makeMockEngine(async () => [ - { id: 'default', typ: 'string' }, - { id: 'wiki', typ: 'string' }, - ]); - const result = await checkSourceConfigShape(engine); - expect(result.status).toBe('warn'); - expect(result.message).toContain('2 source(s)'); - expect(result.message).toContain('default (string)'); - expect(result.message).toContain('wiki (string)'); - expect(result.message).toContain('#2829'); - // Paste-ready repair SQL is part of the hint. - expect(result.message).toContain('UPDATE sources SET config'); - }); - - test('detection query targets the exact jsonb_typeof predicate', async () => { - let captured = ''; - const engine = makeMockEngine(async (sql: string) => { - captured = sql; - return []; - }); - await checkSourceConfigShape(engine); - expect(captured).toContain('jsonb_typeof(config) AS typ'); - expect(captured).toContain("WHERE jsonb_typeof(config) <> 'object'"); - }); - - test('engine error → warn, never a false ok', async () => { - const engine = makeMockEngine(async () => { - throw new Error('relation "sources" does not exist'); - }); - const result = await checkSourceConfigShape(engine); - expect(result.status).toBe('warn'); - expect(result.message).toContain('Check failed'); - }); -}); diff --git a/test/sources-load.test.ts b/test/sources-load.test.ts index d6d392e53..ad202ba89 100644 --- a/test/sources-load.test.ts +++ b/test/sources-load.test.ts @@ -11,7 +11,6 @@ import { loadAllSources, fetchSource, parseSourceConfig, - normalizeSourceConfig, isSourceFederated, } from '../src/core/sources-load.ts'; @@ -121,46 +120,6 @@ describe('parseSourceConfig', () => { test('returns empty object on malformed JSON string', () => { expect(parseSourceConfig('{')).toEqual({}); }); - - test('#2829: unwraps an accidental multi-layer nested string (self-heal read path)', () => { - const wrapped = JSON.stringify(JSON.stringify({ federated: true })); - expect(parseSourceConfig(wrapped)).toEqual({ federated: true }); - }); -}); - -describe('normalizeSourceConfig (#2829)', () => { - test('passes a plain object through unchanged', () => { - expect(normalizeSourceConfig({ federated: true, webhook_secret: 'x' })).toEqual({ - federated: true, - webhook_secret: 'x', - }); - }); - - test('unwraps a single JSON-string layer', () => { - expect(normalizeSourceConfig('{"federated":true}')).toEqual({ federated: true }); - }); - - test('unwraps a 5-layer nested JSON string back to the object', () => { - let v: unknown = { federated: true, tracked_branch: 'main' }; - for (let i = 0; i < 5; i++) v = JSON.stringify(v); // 5 stringify passes = 5 layers - expect(normalizeSourceConfig(v)).toEqual({ federated: true, tracked_branch: 'main' }); - }); - - test('non-object garbage resolves to {}', () => { - expect(normalizeSourceConfig('not json')).toEqual({}); - expect(normalizeSourceConfig('42')).toEqual({}); // parses to a number - expect(normalizeSourceConfig('"just a string"')).toEqual({}); - expect(normalizeSourceConfig(null)).toEqual({}); - expect(normalizeSourceConfig(undefined)).toEqual({}); - expect(normalizeSourceConfig(['a', 'b'])).toEqual({}); // array is not a plain object - expect(normalizeSourceConfig(JSON.stringify(['a']))).toEqual({}); - }); - - test('respects the unwrap bound instead of spinning forever', () => { - let v: unknown = { federated: true }; - for (let i = 0; i < 12; i++) v = JSON.stringify(v); // 12 layers, past the bound of 10 - expect(normalizeSourceConfig(v)).toEqual({}); // gives up to {} once the bound is hit - }); }); describe('isSourceFederated', () => {