From c87e57e1502e7c21b69db9f92d17e2caa82b9070 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 30 May 2026 10:39:26 -0700 Subject: [PATCH] refactor(gbrain-sources): centralize sources-list shape handling in parseSourcesList (#1576) #1576's crash in sourceLocalPath was already fixed in v1.42.0.0 (dual-shape handling). But the readers disagreed: sourceLocalPath accepted both the wrapped {sources:[...]} object (v0.20+) and a bare array, while probeSource and sourcePageCount accepted only the wrapped shape. Extract one parseSourcesList() normalizer and route all three through it, so the shape assumption lives in a single place. This is also the base the #1734 remote_url audit builds on. parseSourcesList returns [] for null/garbage rather than throwing; callers treat 'no rows' as absent. New test/gbrain-sources-parse.test.ts pins both shapes plus the garbage paths and confirms config.remote_url survives for the audit. #1576 is closeable as already-fixed in v1.42.0.0. Co-Authored-By: Claude Opus 4.8 (1M context) --- bin/gstack-gbrain-sync.ts | 7 ++--- lib/gbrain-sources.ts | 38 +++++++++++++++++++++--- test/gbrain-sources-parse.test.ts | 49 +++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 9 deletions(-) create mode 100644 test/gbrain-sources-parse.test.ts diff --git a/bin/gstack-gbrain-sync.ts b/bin/gstack-gbrain-sync.ts index c3708a090..f84e6b2ab 100644 --- a/bin/gstack-gbrain-sync.ts +++ b/bin/gstack-gbrain-sync.ts @@ -37,7 +37,7 @@ import { createHash } from "crypto"; import "../lib/conductor-env-shim"; import { detectEngineTier, withErrorContext, canonicalizeRemote } from "../lib/gstack-memory-helpers"; -import { ensureSourceRegistered, sourcePageCount } from "../lib/gbrain-sources"; +import { ensureSourceRegistered, sourcePageCount, parseSourcesList } from "../lib/gbrain-sources"; import { localEngineStatus, type LocalEngineStatus } from "../lib/gbrain-local-status"; import { buildGbrainEnv, spawnGbrain, execGbrainJson } from "../lib/gbrain-exec"; @@ -407,10 +407,7 @@ export function sourceLocalPath(sourceId: string, env?: NodeJS.ProcessEnv): stri { baseEnv: env }, ); if (!raw) return null; - const list: Array<{ id?: string; local_path?: string }> = Array.isArray(raw) - ? (raw as Array<{ id?: string; local_path?: string }>) - : ((raw as { sources?: Array<{ id?: string; local_path?: string }> }).sources ?? []); - const found = list.find((s) => s.id === sourceId); + const found = parseSourcesList(raw).find((s) => s.id === sourceId); return found?.local_path ?? null; } diff --git a/lib/gbrain-sources.ts b/lib/gbrain-sources.ts index c8ffbad5a..1e8309171 100644 --- a/lib/gbrain-sources.ts +++ b/lib/gbrain-sources.ts @@ -26,6 +26,37 @@ export interface EnsureResult { state: SourceState; } +/** + * One row of `gbrain sources list --json`. `config.remote_url` distinguishes + * URL-managed sources (gbrain owns the clone, may auto-reclone) from + * path-managed ones (user owns the working tree) — load-bearing for the #1734 + * destructive-op guards. + */ +export interface GbrainSourceRow { + id?: string; + local_path?: string; + page_count?: number; + config?: { remote_url?: string | null } | null; +} + +/** + * Normalize `gbrain sources list --json` output to an array of source rows. + * + * gbrain has shipped two shapes: a wrapped `{ sources: [...] }` object (v0.20+) + * and, in older/other variants, a bare top-level array. #1576 was a crash when a + * reader assumed one shape; the parse is centralized here so every reader + * (probeSource, sourcePageCount, sourceLocalPath, the #1734 remote_url audit) + * agrees on the shape in ONE place. Returns [] for null/garbage rather than + * throwing — callers treat "no rows" as absent. + */ +export function parseSourcesList(raw: unknown): GbrainSourceRow[] { + if (Array.isArray(raw)) return raw as GbrainSourceRow[]; + if (raw && typeof raw === "object" && Array.isArray((raw as { sources?: unknown }).sources)) { + return (raw as { sources: GbrainSourceRow[] }).sources; + } + return []; +} + export interface EnsureOptions { /** Pass --federated to `gbrain sources add`. Default false. */ federated?: boolean; @@ -69,14 +100,14 @@ export function probeSource(id: string, env?: NodeJS.ProcessEnv): SourceState { throw err; } - let parsed: { sources?: Array<{ id?: string; local_path?: string }> }; + let parsed: unknown; try { parsed = JSON.parse(stdout); } catch (err) { throw new Error(`gbrain sources list returned non-JSON output: ${(err as Error).message}`); } - const sources = parsed.sources || []; + const sources = parseSourcesList(parsed); const match = sources.find((s) => s.id === id); if (!match) return { status: "absent" }; return { @@ -173,8 +204,7 @@ export function sourcePageCount(id: string, env?: NodeJS.ProcessEnv): number | n } try { - const parsed = JSON.parse(stdout) as { sources?: Array<{ id?: string; page_count?: number }> }; - const match = (parsed.sources || []).find((s) => s.id === id); + const match = parseSourcesList(JSON.parse(stdout)).find((s) => s.id === id); if (!match) return null; if (typeof match.page_count !== "number") return null; return match.page_count; diff --git a/test/gbrain-sources-parse.test.ts b/test/gbrain-sources-parse.test.ts new file mode 100644 index 000000000..ce198b8e5 --- /dev/null +++ b/test/gbrain-sources-parse.test.ts @@ -0,0 +1,49 @@ +import { describe, test, expect } from "bun:test"; +import { parseSourcesList } from "../lib/gbrain-sources"; + +// #1576 hardening: `gbrain sources list --json` has shipped two shapes — a +// wrapped `{ sources: [...] }` object (v0.20+) and a bare top-level array. +// parseSourcesList is the single place that normalizes both, so every reader +// (probeSource, sourcePageCount, sourceLocalPath, the #1734 remote_url audit) +// agrees on the shape. These tests pin both shapes plus the garbage paths. +describe("parseSourcesList", () => { + const rows = [ + { id: "a", local_path: "/x", page_count: 3 }, + { id: "b", local_path: "/y", config: { remote_url: "https://example.com/r.git" } }, + ]; + + test("wrapped { sources: [...] } shape", () => { + expect(parseSourcesList({ sources: rows })).toEqual(rows); + }); + + test("bare top-level array shape", () => { + expect(parseSourcesList(rows)).toEqual(rows); + }); + + test("both shapes yield identical rows (shape-independent)", () => { + expect(parseSourcesList({ sources: rows })).toEqual(parseSourcesList(rows)); + }); + + test("null / undefined → empty array (no throw)", () => { + expect(parseSourcesList(null)).toEqual([]); + expect(parseSourcesList(undefined)).toEqual([]); + }); + + test("object without sources key → empty array", () => { + expect(parseSourcesList({ pages: [] })).toEqual([]); + }); + + test("sources key present but not an array → empty array", () => { + expect(parseSourcesList({ sources: "oops" })).toEqual([]); + }); + + test("scalar garbage → empty array", () => { + expect(parseSourcesList("nope")).toEqual([]); + expect(parseSourcesList(42)).toEqual([]); + }); + + test("preserves config.remote_url for the #1734 audit", () => { + const parsed = parseSourcesList({ sources: rows }); + expect(parsed.find((r) => r.id === "b")?.config?.remote_url).toBe("https://example.com/r.git"); + }); +});