From 16782aee7f267fdd71be3b069537ae64b6df0ed7 Mon Sep 17 00:00:00 2001 From: arisgysel-design Date: Mon, 27 Jul 2026 23:16:05 +0200 Subject: [PATCH] fix(sources): recover corrupted config shapes (#3420) * fix(sources): recover corrupted config shapes (#3401) Use one canonical normalizer for nested string and array-shaped source configs across federation reads, config writes, archive/restore, and doctor remediation.\n\nFixes #3401\nFixes #3402\nFixes #3403 Signed-off-by: arisgysel-design * fix(sources): bind restoreSource federated patch via ::text::jsonb (#2339 class) restoreSource bound a JS JSON string to a bare $1::jsonb placeholder; postgres.js double-encodes that into a jsonb string scalar, so on the Postgres engine the coerced object || string-scalar concat evaluates as array-concat and restore RE-CORRUPTS the exact config shape this PR repairs. PGLite masks the bug (its driver parses the bind natively). Fix: bind through $1::text::jsonb per the repo JSONB rule. Adds the DATABASE_URL-gated Postgres regression (test/e2e/restore-source-config-jsonb-postgres.test.ts): seeds a corrupted string-scalar config, runs archive -> restore, asserts jsonb_typeof(config) = 'object' with the federated flag applied and pre-existing keys preserved. Verified red on the bare ::jsonb bind (config became a jsonb array) and green on the fix against a real pgvector Postgres; skips cleanly without DATABASE_URL. Co-authored-by: arisgysel-design Co-Authored-By: Claude Fable 5 --------- Signed-off-by: arisgysel-design Co-authored-by: arisgysel-design Co-authored-by: Garry Tan Co-authored-by: Claude Fable 5 --- docs/architecture/KEY_FILES.md | 1 + src/commands/doctor.ts | 7 +- src/core/destructive-guard.ts | 5 +- src/core/pglite-engine.ts | 8 +-- src/core/postgres-engine.ts | 27 ++----- src/core/source-config-sql.ts | 65 +++++++++++++++++ src/core/source-resolver.ts | 19 +++-- src/core/sources-load.ts | 53 ++++++++++++-- test/destructive-guard.test.ts | 71 +++++++++++++++++++ test/doctor-source-config-shape.test.ts | 2 + ...store-source-config-jsonb-postgres.test.ts | 68 ++++++++++++++++++ test/list-all-sources.test.ts | 70 +++++++----------- test/local-federated-search-scope.test.ts | 18 ++++- test/sources-load.test.ts | 23 ++++++ 14 files changed, 349 insertions(+), 88 deletions(-) create mode 100644 src/core/source-config-sql.ts create mode 100644 test/e2e/restore-source-config-jsonb-postgres.test.ts diff --git a/docs/architecture/KEY_FILES.md b/docs/architecture/KEY_FILES.md index afb12fdc4..a7924bcf6 100644 --- a/docs/architecture/KEY_FILES.md +++ b/docs/architecture/KEY_FILES.md @@ -202,6 +202,7 @@ per-release `**vX.Y.Z:**` narration — CI enforces this - `src/commands/graph-query.ts` — `gbrain graph-query [--type T] [--depth N] [--direction in|out|both] [--include-foreign]`: typed-edge relationship traversal (renders indented tree). Foreign-edge footer always present (`X foreign edges (use --include-foreign to traverse)`) so cross-source edges never disappear silently; `--include-foreign` widens the SQL filter to walk them. Pinned by `test/graph-query.test.ts`. - `src/commands/sources.ts` — `gbrain sources {list,add,remove,archive,restore,archived,purge,current,status,audit}`. `current [--json]` calls `resolveSourceWithTier()` and prints `source_id`, `tier` (`flag | env | dotfile | local_path | brain_default | seed_default`), and optional `detail` (decision table in `skills/conventions/brain-routing.md`). `status [--json]` — read-only per-source dashboard (last sync, staleness, page count, embedding coverage, unacked failures); thin wrapper around `buildSyncStatusReport` + `printSyncStatusReport` from `src/commands/sync.ts`; `--json` emits stable `{schema_version: 1, sources, ...}` on stdout; filters input to `local_path IS NOT NULL AND archived IS NOT TRUE`. `audit [--json]` — read-only dry-run disk scan for size distribution + would-blocks + junk-pattern hits WITHOUT touching the DB; walks `sources.local_path`, reads each markdown file, runs `assessContent()` from `src/core/content-sanity.ts`, aggregates by verdict (`ok | warn_oversize | hard_block_junk_pattern`). The live `runStatus` health table gains a `BACKFILL` column between `EMBED` and `FAILS` (`active(N)` beats `queued(N)` beats `idle`, from `SourceMetrics.backfill_active` / `backfill_queued` in `src/core/source-health.ts`) so operators see deferred `embed-backfill` minion work after `sync --all` exits 0; `jobCountsBySource` in `source-health.ts` widens its `minion_jobs` SQL with two `COUNT(*) FILTER (WHERE name = 'embed-backfill' AND ...)` aggregates (best-effort, all-0 on pre-minions brains). Pinned by `test/content-sanity.test.ts`, `test/import-file-content-sanity.test.ts`, `test/source-health.test.ts`. - `src/commands/reindex-frontmatter.ts` — `gbrain reindex-frontmatter`. Query path wrapped in the standard `withEngine(...)` lifecycle so `engine.connect()` runs before the first SQL call. Pinned by `test/reindex-frontmatter-connect.test.ts`. +- `src/core/source-config-sql.ts` + `src/core/sources-load.ts` — canonical recovery for historical non-object `sources.config` values. The application reader unwraps nested JSON strings and merges recoverable array fragments left-to-right; the shared SQL expression mirrors that policy atomically for both engines, source config updates, archive/restore, and the paste-ready `source_config_shape` doctor repair. `localFederatedSourceIds` reads config through the same parser so stdio/CLI federation cannot silently disagree with `sources list`. Invalid fragments degrade to `{}` rather than throwing. Pinned by `test/sources-load.test.ts`, `test/list-all-sources.test.ts`, `test/local-federated-search-scope.test.ts`, `test/destructive-guard.test.ts`, and `test/doctor-source-config-shape.test.ts`. - `src/core/source-resolver.ts` — 6-tier source resolution. `resolveSourceWithTier(engine, explicit, cwd)` returns `{ source_id, tier: SourceTier, detail? }` alongside `resolveSourceId()` (unchanged). `SOURCE_TIER_NAMES = ['flag', 'env', 'dotfile', 'local_path', 'sole_non_default', 'brain_default', 'seed_default']` (7 entries; order matches priority). Tier `sole_non_default` slots between `local_path` and `brain_default`: when NO `sources.default` config is set AND exactly one registered source has `local_path` AND isn't `'default'`, auto-route to it; archived sources excluded (try/catch for pre-v34 brains); private `pickSoleNonDefaultSource(engine)` shared by both resolver entry points so they cannot drift. Exported `formatSoleNonDefaultNudge(sourceId): string | null` builds the user-facing stderr nudge (null when `GBRAIN_NO_SOLE_NON_DEFAULT_NUDGE=1`). `src/commands/sync.ts:1497-1519` calls `resolveSourceWithTier` unconditionally so the tier fires; `src/commands/import.ts:96-128` mirrors with the tier-gated nudge. Consumed by `gbrain sources current`, `import --source-id`, `extract --source-id`, and the `source_routing_health` doctor check. Pinned by `test/source-resolver-with-tier.test.ts` (`withEnv()` per test-isolation lint), `test/source-resolver-sole-non-default.test.ts` (14 cases), `test/sync-sole-non-default-routing.test.ts` (3 PGLite cases driving real `runSync`). - `src/core/sync.ts` extension — `isSyncable` factored through private `classifySync(path, opts): SyncableReason | null`; exported companion `unsyncableReason(path, opts)` returns the same tagged reason or null when syncable. `SYNC_SKIP_FILES` is a named export (the four canonical metafile basenames `schema.md`, `index.md`, `log.md`, `README.md`). `SyncableReason` union: `'metafile' | 'strategy' | 'pruned-dir' | 'include-glob-miss' | 'exclude-glob-hit'`. The `commands/sync.ts` cleanup loop guards on `unsyncableReason(path)` being `'metafile'` OR `'pruned-dir'` (#2404) so previously-indexed metafile pages AND deliberately put-created pages under pruned dirs survive every re-sync. Does NOT cover `manifest.deleted` (the upstream filter already strips metafiles). Pinned by `test/sync-isSyncable-shape.test.ts` (15 cases, duality contract) + `test/sync-metafile-skip.serial.test.ts` (3 PGLite cases incl. the renamed `.md → .txt` negative). - `src/core/import-file.ts` extension — identity-based dedup pre-check at `:427-490`. Calls `engine.findDuplicatePage?.(sourceId, {hash, frontmatterId})` (optional `?` so test doubles compile). Posture: SKIP when `frontmatter.id` matches (true external duplicate from overlapping ingest roots), WARN-ALWAYS on content_hash collision with different/missing `frontmatter.id` (templates and daily logs may legitimately share text), FAIL CLOSED on lookup error, bypass via `--force-rechunk`. Soft-deleted pages excluded at the engine layer so tombstones don't block legitimate re-imports under new slugs. Pinned by `test/import-dedup-frontmatter-id.test.ts` (11 cases). diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 18add8c76..656444408 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -1,4 +1,5 @@ import type { BrainEngine } from '../core/engine.ts'; +import { REPAIR_SOURCE_CONFIG_SQL } from '../core/source-config-sql.ts'; import { setCliExitVerdict } from '../core/cli-force-exit.ts'; import * as db from '../core/db.ts'; import { LATEST_VERSION, getIdleBlockers } from '../core/migrate.ts'; @@ -616,10 +617,8 @@ export async function checkSourceConfigShape(engine: BrainEngine): Promise> '{}')::jsonb ` + - `WHERE jsonb_typeof(config) <> 'object';`, + `Repair by running any 'gbrain sources' config write (self-heals nested ` + + `strings and recoverable arrays), or in SQL: ${REPAIR_SOURCE_CONFIG_SQL}`, }; } catch (e) { const msg = e instanceof Error ? e.message : String(e); diff --git a/src/core/destructive-guard.ts b/src/core/destructive-guard.ts index 5a8c2e18d..c35d55b4b 100644 --- a/src/core/destructive-guard.ts +++ b/src/core/destructive-guard.ts @@ -14,6 +14,7 @@ */ import type { BrainEngine } from './engine.ts'; +import { SOURCE_CONFIG_OBJECT_SQL } from './source-config-sql.ts'; // ── Types ─────────────────────────────────────────────────── @@ -190,7 +191,7 @@ export async function softDeleteSource( SET archived = true, archived_at = now(), archive_expires_at = ${expiresClause}, - config = COALESCE(config, '{}'::jsonb) || '{"federated": false}'::jsonb + config = ${SOURCE_CONFIG_OBJECT_SQL} || '{"federated": false}'::jsonb WHERE id = $1 AND archived = false RETURNING id, name, archived_at, archive_expires_at`, [sourceId], @@ -232,7 +233,7 @@ export async function restoreSource( SET archived = false, archived_at = NULL, archive_expires_at = NULL, - config = COALESCE(config, '{}'::jsonb) || $1::jsonb + config = ${SOURCE_CONFIG_OBJECT_SQL} || $1::text::jsonb WHERE id = $2 AND archived = true RETURNING id`, [federatedPatch, sourceId], diff --git a/src/core/pglite-engine.ts b/src/core/pglite-engine.ts index c842a08b4..453486116 100644 --- a/src/core/pglite-engine.ts +++ b/src/core/pglite-engine.ts @@ -23,6 +23,7 @@ import { runMigrations } from './migrate.ts'; import { PGLITE_SCHEMA_SQL, getPGLiteSchema } from './pglite-schema.ts'; import { DEFAULT_EMBEDDING_MODEL, DEFAULT_EMBEDDING_DIMENSIONS } from './ai/defaults.ts'; import { DELETE_BATCH_SIZE } from './engine-constants.ts'; +import { SOURCE_CONFIG_OBJECT_SQL } from './source-config-sql.ts'; import { MARKDOWN_CHUNKER_VERSION } from './chunkers/recursive.ts'; import { acquireLock, releaseLock, type LockHandle } from './pglite-lock.ts'; import { getFtsLanguage } from './fts-language.ts'; @@ -1362,12 +1363,11 @@ export class PGLiteEngine implements BrainEngine { } async updateSourceConfig(sourceId: string, patch: Record): Promise { - // v0.38: parity with postgres-engine.updateSourceConfig. JSONB `||` - // concat operator (overrides same-key, no deep merge). PGLite passes - // `JSON.stringify(patch)` as the param; cast to jsonb on the SQL side. + // Parity with postgres-engine.updateSourceConfig: normalize historical + // string/array shapes atomically before the JSONB patch merge. const result = await this.db.query<{ id: string }>( `UPDATE sources - SET config = COALESCE(config, '{}'::jsonb) || $1::jsonb + SET config = ${SOURCE_CONFIG_OBJECT_SQL} || $1::jsonb WHERE id = $2 RETURNING id`, [JSON.stringify(patch), sourceId], diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts index 173e8bd2f..1f8657e23 100644 --- a/src/core/postgres-engine.ts +++ b/src/core/postgres-engine.ts @@ -67,6 +67,7 @@ import { resolveBoostMap, resolveHardExcludes } from './search/source-boost.ts'; import { buildSourceFactorCase, buildHardExcludeClause, buildVisibilityClause, buildRecencyComponentSql, buildBestPerPagePoolCte, buildOrFallbackWebsearchQuery } from './search/sql-ranking.ts'; import { DEFAULT_EMBEDDING_MODEL, DEFAULT_EMBEDDING_DIMENSIONS } from './ai/defaults.ts'; import { DELETE_BATCH_SIZE } from './engine-constants.ts'; +import { SOURCE_CONFIG_OBJECT_SQL } from './source-config-sql.ts'; import { shouldExcludeFromOrphanReporting, loadOrphanPolicyOverrides } from './orphan-policy.ts'; import { LINK_EXTRACTOR_VERSION_TS } from './link-extraction.ts'; @@ -1407,9 +1408,10 @@ export class PostgresEngine implements BrainEngine { // paths, so the merge must happen inside the UPDATE (parity with // pglite-engine.updateSourceConfig, which already uses JSONB `||`). // - // The CASE normalizes historical bad shapes inline (so `config` is re-read - // against the row-locked latest version — a CTE/subquery snapshot would - // reintroduce the lost-update race under READ COMMITTED): older code paths + // The shared SQL coercion normalizes historical bad shapes inline (so + // `config` is re-read against the row-locked latest version — a detached + // read/normalize/write cycle would reintroduce the lost-update race under + // READ COMMITTED): older code paths // could store config as a JSONB string (double-encoded) or as a JSONB array // of patch objects. We coerce those to a flat object before the `||` merge // so doctor and source routing keep getting flat keys. @@ -1435,24 +1437,7 @@ export class PostgresEngine implements BrainEngine { const sql = this.sql; const result = await sql` UPDATE sources - SET config = - CASE - WHEN jsonb_typeof(config) = 'object' THEN config - WHEN jsonb_typeof(config) = 'string' - THEN CASE - WHEN (config #>> '{}') IS JSON - THEN COALESCE(NULLIF((config #>> '{}'), '')::jsonb, '{}'::jsonb) - ELSE '{}'::jsonb - END - WHEN jsonb_typeof(config) = 'array' - THEN COALESCE( - (SELECT jsonb_object_agg(kv.key, kv.value) - FROM jsonb_array_elements(config) elem, - jsonb_each(elem) kv), - '{}'::jsonb - ) - ELSE '{}'::jsonb - END + SET config = ${sql.unsafe(SOURCE_CONFIG_OBJECT_SQL)} || ${sql.json(patch as Parameters[0])} WHERE id = ${sourceId} `; diff --git a/src/core/source-config-sql.ts b/src/core/source-config-sql.ts new file mode 100644 index 000000000..644ffeddf --- /dev/null +++ b/src/core/source-config-sql.ts @@ -0,0 +1,65 @@ +/** + * Canonical SQL coercion for historical `sources.config` shapes. + * + * Config is meant to be a JSONB object. Older writers could leave nested JSON + * strings or arrays of config fragments. The recursive CTE unwraps strings up + * to the same depth as the application reader, then merges recoverable array + * fragments left-to-right. Invalid fragments are ignored instead of making a + * repair or archive operation fail. + * + * This expression is static SQL: it contains no user input. + */ +export const SOURCE_CONFIG_OBJECT_SQL = `( + WITH RECURSIVE + root_layers(value, depth) AS ( + SELECT COALESCE(config, '{}'::jsonb), 0 + UNION ALL + SELECT (value #>> '{}')::jsonb, depth + 1 + FROM root_layers + WHERE depth < 10 + AND jsonb_typeof(value) = 'string' + AND (value #>> '{}') IS JSON + ), + root(value) AS ( + SELECT value FROM root_layers ORDER BY depth DESC LIMIT 1 + ), + fragment_seeds(ordinality, value) AS ( + SELECT 0::bigint, value FROM root WHERE jsonb_typeof(value) = 'object' + UNION ALL + SELECT item.ordinality, item.value + FROM root, + LATERAL jsonb_array_elements( + CASE WHEN jsonb_typeof(root.value) = 'array' THEN root.value ELSE '[]'::jsonb END + ) WITH ORDINALITY AS item(value, ordinality) + ), + fragment_layers(ordinality, value, depth) AS ( + SELECT ordinality, value, 0 FROM fragment_seeds + UNION ALL + SELECT ordinality, (value #>> '{}')::jsonb, depth + 1 + FROM fragment_layers + WHERE depth < 10 + AND jsonb_typeof(value) = 'string' + AND (value #>> '{}') IS JSON + ), + fragments AS ( + SELECT DISTINCT ON (ordinality) ordinality, value + FROM fragment_layers + ORDER BY ordinality, depth DESC + ) + SELECT COALESCE( + jsonb_object_agg(entry.key, entry.value ORDER BY fragments.ordinality), + '{}'::jsonb + ) + FROM fragments + CROSS JOIN LATERAL jsonb_each( + CASE WHEN jsonb_typeof(fragments.value) = 'object' + THEN fragments.value + ELSE '{}'::jsonb + END + ) AS entry(key, value) +)`; + +/** Paste-ready repair used by `gbrain doctor`. */ +export const REPAIR_SOURCE_CONFIG_SQL = + `UPDATE sources SET config = ${SOURCE_CONFIG_OBJECT_SQL} ` + + `WHERE jsonb_typeof(config) <> 'object';`; diff --git a/src/core/source-resolver.ts b/src/core/source-resolver.ts index 03f9eb0c0..fc2587de0 100644 --- a/src/core/source-resolver.ts +++ b/src/core/source-resolver.ts @@ -16,6 +16,7 @@ import { readFileSync, lstatSync, type Stats } from 'fs'; import { join, dirname, resolve } from 'path'; import type { BrainEngine } from './engine.ts'; +import { isSourceFederated } from './sources-load.ts'; import { SOURCE_ID_RE, isValidSourceId } from './source-id.ts'; import { isTrustedDotfile, realpathOrResolve } from './path-confine.ts'; @@ -405,17 +406,23 @@ export async function localFederatedSourceIds( tier: SourceTier, ): Promise { if (tier === 'flag' || tier === 'env' || tier === 'dotfile') return undefined; - let rows: Array<{ id: string }>; + let rows: Array<{ id: string; config: unknown; archived?: boolean }>; try { - rows = await engine.executeRaw<{ id: string }>( - `SELECT id FROM sources WHERE config->>'federated' = 'true' AND archived = false ORDER BY id`, + rows = await engine.executeRaw<{ id: string; config: unknown; archived?: boolean }>( + `SELECT id, config, archived FROM sources WHERE archived = false ORDER BY id`, ); } catch { - rows = await engine.executeRaw<{ id: string }>( - `SELECT id FROM sources WHERE config->>'federated' = 'true' ORDER BY id`, + rows = await engine.executeRaw<{ id: string; config: unknown }>( + `SELECT id, config FROM sources ORDER BY id`, ); } - const ids = [sourceId, ...rows.map((r) => r.id).filter((id) => id !== sourceId)]; + const ids = [ + sourceId, + ...rows + .filter((row) => row.archived !== true && isSourceFederated(row.config)) + .map((row) => row.id) + .filter((id) => id !== sourceId), + ]; return ids.length > 1 ? ids : undefined; } diff --git a/src/core/sources-load.ts b/src/core/sources-load.ts index 92c10ffd2..3c3b3aa9a 100644 --- a/src/core/sources-load.ts +++ b/src/core/sources-load.ts @@ -72,6 +72,44 @@ function unwrapConfigLayers(config: unknown): { value: unknown; layers: number } return { value, layers }; } +/** + * Recover the canonical object from historical config shapes. + * + * A naive JSONB `||` merge could turn a string-shaped config plus an object + * patch into an array. Those arrays are an ordered sequence of config + * fragments, so merge recoverable object fragments left-to-right. This keeps + * the latest patch authoritative while preserving keys from older fragments. + */ +function coerceSourceConfigObject(config: unknown): { + value: Record | null; + layers: number; + recoveredArray: boolean; +} { + const root = unwrapConfigLayers(config); + if (isPlainObject(root.value)) { + return { value: root.value, layers: root.layers, recoveredArray: false }; + } + if (!Array.isArray(root.value)) { + return { value: null, layers: root.layers, recoveredArray: false }; + } + + const merged: Record = {}; + let objectFragments = 0; + let layers = root.layers; + for (const fragment of root.value) { + const unwrapped = unwrapConfigLayers(fragment); + layers += unwrapped.layers; + if (!isPlainObject(unwrapped.value)) continue; + Object.assign(merged, unwrapped.value); + objectFragments++; + } + return { + value: objectFragments > 0 ? merged : null, + layers, + recoveredArray: objectFragments > 0, + }; +} + /** * #2829: coerce a config value to the underlying plain object before it is * written back, fully unwrapping any accidental JSON-string nesting so a @@ -82,10 +120,10 @@ function unwrapConfigLayers(config: unknown): { value: unknown; layers: number } * object. */ export function normalizeSourceConfig(config: unknown): Record { - const { value } = unwrapConfigLayers(config); - if (isPlainObject(value)) return value; + const { value } = coerceSourceConfigObject(config); + if (value) return value; console.warn( - `[gbrain] source config was not a JSON object (got ${value === null ? 'null' : typeof value}); ` + + `[gbrain] source config was not a recoverable JSON object; ` + `storing {} instead. Run 'gbrain doctor' to find affected sources.`, ); return {}; @@ -99,14 +137,15 @@ export function normalizeSourceConfig(config: unknown): Record * path; two or more means the value was re-wrapped and should be repaired). */ export function parseSourceConfig(config: unknown): Record { - const { value, layers } = unwrapConfigLayers(config); - if (layers > 1) { + const { value, layers, recoveredArray } = coerceSourceConfigObject(config); + if (layers > 1 || recoveredArray) { + const shape = recoveredArray ? 'historical JSON array' : `${layers}-layer nested JSON string`; console.warn( - `[gbrain] source config was stored as a ${layers}-layer nested JSON string; ` + + `[gbrain] source config was stored as a ${shape}; ` + `it will be repaired on the next config write. Run 'gbrain doctor' to find affected sources.`, ); } - return isPlainObject(value) ? value : {}; + return value ?? {}; } /** True iff the source's config.federated field is the literal boolean true. */ diff --git a/test/destructive-guard.test.ts b/test/destructive-guard.test.ts index 8c5efce84..76d612cd2 100644 --- a/test/destructive-guard.test.ts +++ b/test/destructive-guard.test.ts @@ -53,6 +53,22 @@ async function seedSource(engine: PGLiteEngine, id: string, opts?: { withPages?: } } +async function setRawSourceConfig(engine: PGLiteEngine, id: string, rawJson: string): Promise { + await engine.executeRaw( + `UPDATE sources SET config = $2::text::jsonb WHERE id = $1`, + [id, rawJson], + ); +} + +async function readSourceConfig(engine: PGLiteEngine, id: string): Promise> { + const rows = await engine.executeRaw<{ config: unknown }>( + `SELECT config FROM sources WHERE id = $1`, + [id], + ); + const config = rows[0].config; + return typeof config === 'string' ? JSON.parse(config) : config as Record; +} + describe('assessDestructiveImpact', () => { let engine: PGLiteEngine; @@ -209,6 +225,38 @@ describe('soft-delete + restore lifecycle (column-based v0.26.5)', () => { expect(config.archived_at).toBeUndefined(); }); + test('softDeleteSource normalizes nested-string config without dropping keys', async () => { + const id = 'sd-string-config'; + await seedSource(engine, id); + await setRawSourceConfig( + engine, + id, + JSON.stringify(JSON.stringify({ federated: true, remote_url: 'https://example.invalid/repo' })), + ); + await softDeleteSource(engine, id); + expect(await readSourceConfig(engine, id)).toEqual({ + federated: false, + remote_url: 'https://example.invalid/repo', + }); + }); + + test('softDeleteSource flattens recoverable array config without dropping keys', async () => { + const id = 'sd-array-config'; + await seedSource(engine, id); + await setRawSourceConfig(engine, id, JSON.stringify([ + '{"remote_url":"https://example.invalid/repo"}', + { tracked_branch: 'main' }, + { federated: true }, + 'not-json', + ])); + await softDeleteSource(engine, id); + expect(await readSourceConfig(engine, id)).toEqual({ + federated: false, + remote_url: 'https://example.invalid/repo', + tracked_branch: 'main', + }); + }); + test('restoreSource clears the column state and re-federates by default', async () => { const id = 'sd-restore-fed'; await seedSource(engine, id, { withPages: 1 }); @@ -237,6 +285,29 @@ describe('soft-delete + restore lifecycle (column-based v0.26.5)', () => { expect(config.federated).toBe(false); }); + test('restoreSource repairs array config and preserves recoverable keys', async () => { + const id = 'sd-restore-array'; + await seedSource(engine, id); + await engine.executeRaw( + `UPDATE sources + SET archived = true, + archived_at = now(), + archive_expires_at = now() + interval '1 hour' + WHERE id = $1`, + [id], + ); + await setRawSourceConfig(engine, id, JSON.stringify([ + { remote_url: 'https://example.invalid/repo' }, + '{"federated":false,"tracked_branch":"main"}', + ])); + expect(await restoreSource(engine, id)).toBe(true); + expect(await readSourceConfig(engine, id)).toEqual({ + federated: true, + remote_url: 'https://example.invalid/repo', + tracked_branch: 'main', + }); + }); + test('restoreSource is idempotent-as-false on already-active', async () => { const id = 'sd-active'; await seedSource(engine, id); diff --git a/test/doctor-source-config-shape.test.ts b/test/doctor-source-config-shape.test.ts index 40519a306..57fd599f4 100644 --- a/test/doctor-source-config-shape.test.ts +++ b/test/doctor-source-config-shape.test.ts @@ -39,6 +39,8 @@ describe('checkSourceConfigShape (#2829)', () => { expect(result.message).toContain('#2829'); // Paste-ready repair SQL is part of the hint. expect(result.message).toContain('UPDATE sources SET config'); + expect(result.message).toContain('jsonb_array_elements'); + expect(result.message).toContain('IS JSON'); }); test('detection query targets the exact jsonb_typeof predicate', async () => { diff --git a/test/e2e/restore-source-config-jsonb-postgres.test.ts b/test/e2e/restore-source-config-jsonb-postgres.test.ts new file mode 100644 index 000000000..ea8cc9b69 --- /dev/null +++ b/test/e2e/restore-source-config-jsonb-postgres.test.ts @@ -0,0 +1,68 @@ +/** + * Postgres-only regression for archive/restore config recovery (#3420). + * + * restoreSource patches `config.federated` by binding a JS JSON string to a + * jsonb placeholder. With a bare `$n::jsonb` bind, postgres.js double-encodes + * the value into a jsonb STRING scalar (#2339 class); the coerced object then + * gets `object || string` concatenated, which Postgres evaluates as + * array-concat — so restore RE-CORRUPTS the exact shape this path repairs. + * PGLite cannot reproduce this (its driver parses the bind natively), so this + * is DATABASE_URL-gated per the engine-parity convention. Pins the + * `$n::text::jsonb` bind shape: after archive → restore of a corrupted + * string-scalar config, config must be jsonb_typeof = 'object', the federated + * flag must be applied, and pre-existing keys must survive. + */ +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { setupDB, teardownDB, hasDatabase } from './helpers.ts'; +import type { PostgresEngine } from '../../src/core/postgres-engine.ts'; +import { softDeleteSource, restoreSource } from '../../src/core/destructive-guard.ts'; + +const skip = !hasDatabase(); +const describeIfDB = skip ? describe.skip : describe; + +let engine: PostgresEngine; + +beforeAll(async () => { + if (skip) return; + engine = await setupDB(); +}); + +afterAll(async () => { + if (skip) return; + await engine.executeRaw(`DELETE FROM sources WHERE id = 'restore-corrupt-cfg'`); + await teardownDB(); +}); + +describeIfDB('restoreSource config jsonb encoding — Postgres regression (#3420)', () => { + test('archive → restore of a string-scalar config yields an object with federated preserved', async () => { + const id = 'restore-corrupt-cfg'; + await engine.executeRaw( + `INSERT INTO sources (id, name) VALUES ($1, $1) ON CONFLICT (id) DO NOTHING`, + [id], + ); + // Corrupted shape: config is a jsonb STRING scalar whose text is valid JSON. + await engine.executeRaw( + `UPDATE sources SET config = $2::text::jsonb, archived = false WHERE id = $1`, + [id, JSON.stringify(JSON.stringify({ federated: false, remote_url: 'https://example.invalid/repo' }))], + ); + const seeded = await engine.executeRaw<{ kind: string }>( + `SELECT jsonb_typeof(config) AS kind FROM sources WHERE id = $1`, + [id], + ); + expect(seeded[0]!.kind).toBe('string'); + + expect(await softDeleteSource(engine, id)).not.toBeNull(); + expect(await restoreSource(engine, id, true)).toBe(true); + + const rows = await engine.executeRaw<{ kind: string; federated: string | null; remote_url: string | null }>( + `SELECT jsonb_typeof(config) AS kind, + config->>'federated' AS federated, + config->>'remote_url' AS remote_url + FROM sources WHERE id = $1`, + [id], + ); + expect(rows[0]!.kind).toBe('object'); + expect(rows[0]!.federated).toBe('true'); + expect(rows[0]!.remote_url).toBe('https://example.invalid/repo'); + }); +}); diff --git a/test/list-all-sources.test.ts b/test/list-all-sources.test.ts index fddfc9b0a..18e444bfc 100644 --- a/test/list-all-sources.test.ts +++ b/test/list-all-sources.test.ts @@ -43,6 +43,13 @@ async function seedSource( ); } +async function setRawConfig(id: string, rawJson: string): Promise { + await engine.executeRaw( + `UPDATE sources SET config = $2::text::jsonb WHERE id = $1`, + [id, rawJson], + ); +} + describe('engine.listAllSources', () => { test('returns empty array on fresh brain with only seeded default', async () => { // 'default' source seeded by migration; we'll just check it appears @@ -136,50 +143,27 @@ describe('engine.updateSourceConfig', () => { expect(all.find(s => s.id === 'delta')!.config.last_full_cycle_at).toBe('2026-05-22T11:00:00.000Z'); }); - // IS JSON guard: the postgres-engine atomic merge gates its `::jsonb` cast - // behind the SQL `IS JSON` predicate so a historical bad row whose config is - // a JSONB string of NON-JSON text normalizes to `{}` instead of raising - // `invalid input syntax for type json` and aborting the UPDATE. - // - // We exercise the SQL CASE expression directly via executeRaw against PGLite - // (which ships Postgres 17 + IS JSON parity) rather than calling - // PostgresEngine.updateSourceConfig (which requires a live Postgres pool). test('IS-JSON guard: non-JSON string config normalizes to {} on merge', async () => { - const patch = { merged_key: 'v' }; + await seedSource('bad-string'); + await setRawConfig('bad-string', JSON.stringify('garbage text')); + expect(await engine.updateSourceConfig('bad-string', { merged_key: 'v' })).toBe(true); + const all = await engine.listAllSources(); + expect(all.find(source => source.id === 'bad-string')!.config).toEqual({ merged_key: 'v' }); + }); - const guarded = (configExpr: string) => ` - SELECT ( - CASE - WHEN jsonb_typeof(${configExpr}) = 'object' THEN ${configExpr} - WHEN jsonb_typeof(${configExpr}) = 'string' - THEN CASE - WHEN (${configExpr} #>> '{}') IS JSON - THEN COALESCE(NULLIF((${configExpr} #>> '{}'), '')::jsonb, '{}'::jsonb) - ELSE '{}'::jsonb - END - WHEN jsonb_typeof(${configExpr}) = 'array' - THEN COALESCE( - (SELECT jsonb_object_agg(kv.key, kv.value) - FROM jsonb_array_elements(${configExpr}) elem, - jsonb_each(elem) kv), - '{}'::jsonb - ) - ELSE '{}'::jsonb - END || $1::jsonb - ) AS result`; - - // 1. JSONB string holding NON-JSON text → normalizes to {} then merges patch. - const bad = await engine.executeRaw<{ result: Record }>( - guarded(`to_jsonb('garbage text'::text)`), - [JSON.stringify(patch)], - ); - expect(bad[0].result).toEqual({ merged_key: 'v' }); - - // 2. JSONB string holding double-encoded valid JSON object → parsed + merged. - const good = await engine.executeRaw<{ result: Record }>( - guarded(`to_jsonb('{"x":1}'::text)`), - [JSON.stringify(patch)], - ); - expect(good[0].result).toEqual({ x: 1, merged_key: 'v' }); + test('historical array config is flattened safely before merge', async () => { + await seedSource('array-config'); + await setRawConfig('array-config', JSON.stringify([ + JSON.stringify(JSON.stringify({ remote_url: 'https://example.invalid/repo' })), + { federated: true }, + 'garbage', + ])); + expect(await engine.updateSourceConfig('array-config', { tracked_branch: 'main' })).toBe(true); + const all = await engine.listAllSources(); + expect(all.find(source => source.id === 'array-config')!.config).toEqual({ + remote_url: 'https://example.invalid/repo', + federated: true, + tracked_branch: 'main', + }); }); }); diff --git a/test/local-federated-search-scope.test.ts b/test/local-federated-search-scope.test.ts index 2a7dd1b64..d1f5945ad 100644 --- a/test/local-federated-search-scope.test.ts +++ b/test/local-federated-search-scope.test.ts @@ -102,9 +102,25 @@ describe('localFederatedSourceIds — CLI-side scope computation', () => { }); test('single federated source (the resolved one) keeps the scalar fast path', async () => { - const solo = { executeRaw: async () => [{ id: 'default' }] } as any; + const solo = { executeRaw: async () => [{ id: 'default', config: { federated: true } }] } as any; expect(await localFederatedSourceIds(solo, 'default', 'seed_default')).toBeUndefined(); }); + + test('historical string and array config shapes remain federated', async () => { + const historical = { + executeRaw: async () => [ + { id: 'default', config: { federated: true }, archived: false }, + { id: 'nested', config: JSON.stringify(JSON.stringify({ federated: true })), archived: false }, + { id: 'array', config: ['{"remote_url":"x"}', { federated: true }], archived: false }, + { id: 'archived', config: ['{}', { federated: true }], archived: true }, + ], + } as any; + expect(await localFederatedSourceIds(historical, 'default', 'seed_default')).toEqual([ + 'default', + 'nested', + 'array', + ]); + }); }); describe('federatedSearchScope — trust + explicitness matrix', () => { diff --git a/test/sources-load.test.ts b/test/sources-load.test.ts index d6d392e53..cd436a48b 100644 --- a/test/sources-load.test.ts +++ b/test/sources-load.test.ts @@ -126,6 +126,18 @@ describe('parseSourceConfig', () => { const wrapped = JSON.stringify(JSON.stringify({ federated: true })); expect(parseSourceConfig(wrapped)).toEqual({ federated: true }); }); + + test('recovers ordered object fragments from a historical JSONB array', () => { + expect(parseSourceConfig([ + '{"remote_url":"https://example.invalid/repo"}', + { federated: true }, + { tracked_branch: 'main' }, + ])).toEqual({ + remote_url: 'https://example.invalid/repo', + federated: true, + tracked_branch: 'main', + }); + }); }); describe('normalizeSourceConfig (#2829)', () => { @@ -156,6 +168,16 @@ describe('normalizeSourceConfig (#2829)', () => { expect(normalizeSourceConfig(JSON.stringify(['a']))).toEqual({}); }); + test('normalizes a historical config-fragment array without dropping valid keys', () => { + expect(normalizeSourceConfig([ + JSON.stringify(JSON.stringify({ remote_url: 'https://example.invalid/repo' })), + { federated: false }, + ])).toEqual({ + remote_url: 'https://example.invalid/repo', + federated: false, + }); + }); + 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 @@ -171,5 +193,6 @@ describe('isSourceFederated', () => { expect(isSourceFederated({ federated: 1 })).toBe(false); expect(isSourceFederated({})).toBe(false); expect(isSourceFederated(null)).toBe(false); + expect(isSourceFederated(['{"remote_url":"x"}', { federated: true }])).toBe(true); }); });