From 9ae98e9d4d4a6fc97032772426dc58395b49f8b6 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 13 May 2026 11:22:15 -0700 Subject: [PATCH] =?UTF-8?q?fix(gbrain):=20engine=20detection=20survives=20?= =?UTF-8?q?gbrain=20=E2=89=A50.25=20schema=20+=20non-zero=20doctor=20exit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit freshDetectEngineTier() in lib/gstack-memory-helpers.ts returned engine: "unknown" for every Supabase user on gbrain ≥0.25. Two stacking bugs: 1. execSync("gbrain doctor --json --fast 2>/dev/null") threw on non-zero exit. gbrain doctor exits 1 whenever health_score < 100, which is essentially every fresh install due to resolver_health warnings. The JSON output never reached the parser. 2. gbrain ≥0.25 shipped schema_version:2 doctor output that dropped the top-level 'engine' field entirely. Result: every /sync-gbrain on Supabase logged 'engine=unknown' and skipped all sync stages silently. Fix: - Replace execSync with execFileSync (no shell, no bash-specific 2>/dev/null redirect; portable to Windows). - Recover stdout from the thrown error object so non-zero exits still parse. - Fall back to reading gbrain's config.json (respecting GBRAIN_HOME env var, defaulting to ~/.gbrain/config.json) when doctor output doesn't surface an engine field. - Add logGbrainError() helper that appends one-line JSONL to ~/.gstack/.gbrain-errors.jsonl on parse failure, so future regressions leave a forensic trail. The "supabase" tier here means "remote postgres" in practice — gbrain config uses engine:"postgres" for both real Supabase and any other remote postgres (e.g. local-postgres-for-testing). Downstream sync code treats them identically, so the label compression is intentional and documented inline. Regression test: existing detectEngineTier suite now isolates HOME + GBRAIN_HOME + PATH to temp dirs (closes a flake source where the prior tests would read whatever was on the reviewer's machine). New test forces gbrain off PATH, writes a synthetic config.json with engine:"postgres", asserts detectEngineTier() returns engine:"supabase". Fixes #1415. Patch shape contributed by Shiv @shivasymbl (tested on gstack v1.31.0.0 + gbrain v0.31.3 + Supabase). --- lib/gstack-memory-helpers.ts | 83 ++++++++++++++++++++++++++---- test/gstack-memory-helpers.test.ts | 34 ++++++++++++ 2 files changed, 106 insertions(+), 11 deletions(-) diff --git a/lib/gstack-memory-helpers.ts b/lib/gstack-memory-helpers.ts index 8b20f3203..c76a5f179 100644 --- a/lib/gstack-memory-helpers.ts +++ b/lib/gstack-memory-helpers.ts @@ -244,21 +244,82 @@ export function detectEngineTier(): EngineDetect { return fresh; } +// Returns gbrain's config.json path, honoring GBRAIN_HOME env var with a +// fallback to ~/.gbrain. gbrain >=0.25 dropped the top-level `engine` field +// from doctor output, so this file is the only reliable source for engine +// detection on that version. See #1415. +function gbrainConfigPath(): string { + const root = process.env.GBRAIN_HOME || join(homedir(), ".gbrain"); + return join(root, "config.json"); +} + +// Best-effort JSONL append to ~/.gstack/.gbrain-errors.jsonl. Never throws. +function logGbrainError(kind: string, detail: string): void { + try { + const path = errorLogPath(); + mkdirSync(dirname(path), { recursive: true }); + appendFileSync( + path, + JSON.stringify({ ts: new Date().toISOString(), kind, detail: detail.slice(0, 500) }) + "\n", + "utf-8" + ); + } catch { /* logging is best-effort */ } +} + function freshDetectEngineTier(): EngineDetect { const now = Date.now(); + let parsed: Record | null = null; + + // execFileSync (not execSync) avoids shell redirection — portable to + // environments where `2>/dev/null` is bash-specific. The stdio array + // suppresses stderr without invoking a shell. try { - const out = execSync("gbrain doctor --json --fast 2>/dev/null", { encoding: "utf-8", timeout: 5000 }); - const parsed = JSON.parse(out); - const engine: EngineTier = parsed?.engine === "supabase" ? "supabase" : parsed?.engine === "pglite" ? "pglite" : "unknown"; - return { - engine, - supabase_url: parsed?.supabase_url || undefined, - detected_at: now, - schema_version: 1, - }; - } catch { - return { engine: "unknown", detected_at: now, schema_version: 1 }; + const out = execFileSync("gbrain", ["doctor", "--json", "--fast"], { + encoding: "utf-8", + timeout: 5000, + stdio: ["ignore", "pipe", "ignore"], + }); + parsed = JSON.parse(out); + } catch (err: unknown) { + // execFileSync throws on non-zero exit; stdout is still on the error + // object. gbrain doctor exits 1 whenever health_score < 100, which is + // essentially always on fresh installs (resolver_health warnings are + // normal). Recover stdout and re-parse. See #1415. + try { + const stdout = (err as { stdout?: Buffer | string })?.stdout ?? ""; + const stdoutStr = typeof stdout === "string" ? stdout : stdout.toString("utf-8"); + if (stdoutStr) parsed = JSON.parse(stdoutStr); + } catch (parseErr) { + logGbrainError("doctor_parse_failure", String(parseErr)); + } } + + let engine: EngineTier = + parsed?.engine === "supabase" ? "supabase" : + parsed?.engine === "pglite" ? "pglite" : "unknown"; + + // gbrain >=0.25 ships schema_version:2 doctor output which dropped the + // top-level `engine` field. Fall back to gbrain's config.json (respects + // GBRAIN_HOME). "supabase" here means "remote postgres" — gbrain config + // uses engine:"postgres" for real Supabase AND any other remote postgres + // (e.g. local-postgres-for-testing). Downstream sync code treats them the + // same, so the label compression is intentional. + if (engine === "unknown") { + try { + const cfg = JSON.parse(readFileSync(gbrainConfigPath(), "utf-8")); + if (cfg?.engine === "pglite") engine = "pglite"; + else if (cfg?.engine === "postgres" || cfg?.database_url) engine = "supabase"; + } catch (cfgErr) { + logGbrainError("config_read_failure", String(cfgErr)); + } + } + + return { + engine, + supabase_url: parsed?.supabase_url as string | undefined, + detected_at: now, + schema_version: 1, + }; } // ── Public: parseSkillManifest ──────────────────────────────────────────── diff --git a/test/gstack-memory-helpers.test.ts b/test/gstack-memory-helpers.test.ts index 864fde635..f1d2bf379 100644 --- a/test/gstack-memory-helpers.test.ts +++ b/test/gstack-memory-helpers.test.ts @@ -272,17 +272,36 @@ describe("withErrorContext", () => { describe("detectEngineTier", () => { let savedHome: string | undefined; + let savedGbrainHome: string | undefined; + let savedRealHome: string | undefined; + let savedPath: string | undefined; let testHome: string; + let testGbrainHome: string; beforeEach(() => { savedHome = process.env.GSTACK_HOME; + savedGbrainHome = process.env.GBRAIN_HOME; + savedRealHome = process.env.HOME; + savedPath = process.env.PATH; testHome = mkdtempSync(join(tmpdir(), "gstack-test-engine-")); + testGbrainHome = mkdtempSync(join(tmpdir(), "gstack-test-gbrain-")); process.env.GSTACK_HOME = testHome; + process.env.GBRAIN_HOME = testGbrainHome; + // Isolate HOME too — even though gbrainConfigPath() prefers GBRAIN_HOME + // when set, defense-in-depth against future code reading ~/.gbrain + // directly. See #1415 codex review finding #6. + process.env.HOME = testHome; }); afterAll(() => { if (savedHome === undefined) delete process.env.GSTACK_HOME; else process.env.GSTACK_HOME = savedHome; + if (savedGbrainHome === undefined) delete process.env.GBRAIN_HOME; + else process.env.GBRAIN_HOME = savedGbrainHome; + if (savedRealHome === undefined) delete process.env.HOME; + else process.env.HOME = savedRealHome; + if (savedPath === undefined) delete process.env.PATH; + else process.env.PATH = savedPath; }); it("returns a valid EngineDetect shape (engine, detected_at, schema_version)", () => { @@ -307,4 +326,19 @@ describe("detectEngineTier", () => { const second = detectEngineTier(); expect(second.detected_at).toBe(first.detected_at); }); + + it("falls back to GBRAIN_HOME/config.json when gbrain doctor omits engine (schema_version:2 case)", () => { + // Regression test for #1415: gbrain >=0.25 doctor output dropped the + // top-level `engine` field. The detect path must fall back to config.json. + // We force the doctor call to fail (PATH stripped of gbrain) and write a + // synthetic config to GBRAIN_HOME so the fallback path is deterministic. + process.env.PATH = "/nonexistent-no-gbrain-here"; + writeFileSync( + join(testGbrainHome, "config.json"), + JSON.stringify({ engine: "postgres", database_url: "postgresql://test/example" }), + "utf-8" + ); + const result = detectEngineTier(); + expect(result.engine).toBe("supabase"); + }); });