From 5854d122d33cea51ec5d13e3fa1342fcb63bd683 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 16 Aug 2026 08:56:42 -0700 Subject: [PATCH] fix: resolve GBRAIN_HOME with gbrain's parent-dir semantics (#2521) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gstack treated GBRAIN_HOME as the config directory; gbrain's configDir() treats it as the PARENT and always appends `.gbrain` itself (the contract is explicit in gbrain's source: GBRAIN_HOME=/tmp/x → /tmp/x/.gbrain/ config.json). With GBRAIN_HOME set, gstack classified engine status from a file gbrain never reads — the probe's two halves (file checks vs the spawned `gbrain sources list`) looked at DIFFERENT installs, so any resulting status was arbitrary: missing-config/broken-config against healthy installs, or a thin-client marker gstack saw that gbrain itself reported as "No brain configured". New shared resolver `gbrainConfigDir()` in lib/gbrain-exec.ts is the single source of truth. All seven gstack sites route through the contract: - lib/gbrain-local-status.ts gbrainConfigPath (the classifier's file half) - bin/gstack-gbrain-detect GBRAIN_CONFIG + readRemoteMcpUrl - lib/gbrain-exec.ts buildGbrainEnv (the probe's DATABASE_URL seed — fixing only the classifier would have left the split-brain in the spawn half, flagged by the reporter) - lib/gbrain-guards.ts gbrainHome (clones-dir + autopilot-lock paths) - lib/gstack-memory-helpers.ts gbrainConfigPath (engine-tier fallback) - bin/gstack-gbrain-install pre-doctor config check (shell) Unit tests cover GBRAIN_HOME set (config found at $GBRAIN_HOME/.gbrain), the old flat layout explicitly NOT read (both classifier and buildGbrainEnv), and unset (~/.gbrain unchanged). Existing fixtures that encoded the deviant flat layout are updated to gbrain's contract. Root-cause analysis by @d-danielsun in #2521. Deviation from the 3-site plan spec: the same deviant resolution existed in four more sites (buildGbrainEnv, gbrain-guards, memory-helpers, gbrain-install); fixing only three would have left gstack disagreeing with itself as well as with gbrain, so the whole class moved to the shared resolver in one change. Fixes #2521 Co-Authored-By: Claude Fable 5 --- bin/gstack-gbrain-detect | 18 +++++++-------- bin/gstack-gbrain-install | 9 +++++++- lib/gbrain-exec.ts | 31 +++++++++++++++++-------- lib/gbrain-guards.ts | 4 +++- lib/gbrain-local-status.ts | 11 +++++---- lib/gstack-memory-helpers.ts | 8 ++++--- test/build-gbrain-env.test.ts | 25 +++++++++++++++++---- test/code-intelligence.test.ts | 4 +++- test/gbrain-local-status.test.ts | 36 +++++++++++++++++++++++++----- test/gstack-memory-helpers.test.ts | 10 ++++++--- 10 files changed, 115 insertions(+), 41 deletions(-) diff --git a/bin/gstack-gbrain-detect b/bin/gstack-gbrain-detect index 4ec4f239d..3774f6bac 100755 --- a/bin/gstack-gbrain-detect +++ b/bin/gstack-gbrain-detect @@ -43,18 +43,17 @@ import { resolveGbrainBin, readGbrainVersion, } from "../lib/gbrain-local-status"; -import { isTransactionModePooler } from "../lib/gbrain-exec"; +import { gbrainConfigDir, isTransactionModePooler } from "../lib/gbrain-exec"; const STATE_DIR = process.env.GSTACK_HOME || join(userHome(), ".gstack"); const SCRIPT_DIR = __dirname; const CONFIG_BIN = join(SCRIPT_DIR, "gstack-config"); -// Honors GBRAIN_HOME — must stay consistent with lib/gbrain-local-status's -// config resolution, or the detect JSON reports gbrain_local_status "ok" -// alongside gbrain_config_exists false for relocated-home users. -const GBRAIN_CONFIG = join( - process.env.GBRAIN_HOME || join(userHome(), ".gbrain"), - "config.json", -); +// Honors GBRAIN_HOME with gbrain's own configDir() semantics (#2521: +// GBRAIN_HOME is a parent dir, `.gbrain` is appended) — must stay consistent +// with lib/gbrain-local-status's config resolution, or the detect JSON +// reports gbrain_local_status "ok" alongside gbrain_config_exists false for +// relocated-home users. Both route through gbrainConfigDir. +const GBRAIN_CONFIG = join(gbrainConfigDir(), "config.json"); const CLAUDE_JSON = join(userHome(), ".claude.json"); function userHome(): string { @@ -232,8 +231,7 @@ function detectMcpMode(): "local-stdio" | "remote-http" | "none" { /** remote_mcp.mcp_url from gbrain's own config (thin-client marker, #2051). */ function readRemoteMcpUrl(): string { - const gbrainHome = process.env.GBRAIN_HOME || join(userHome(), ".gbrain"); - const cfg = tryReadJSON(join(gbrainHome, "config.json")) as + const cfg = tryReadJSON(join(gbrainConfigDir(), "config.json")) as | { remote_mcp?: { mcp_url?: string } } | null; return cfg?.remote_mcp?.mcp_url || ""; diff --git a/bin/gstack-gbrain-install b/bin/gstack-gbrain-install index 60c8f86b6..35aab4b08 100755 --- a/bin/gstack-gbrain-install +++ b/bin/gstack-gbrain-install @@ -235,7 +235,14 @@ fi # a hard gate so a broken gbrain is caught at setup, not at data-loss time. # Pre-init installs skip this (config not written yet); the full # `/sync-gbrain --dry-run` self-test runs from /setup-gbrain after `gbrain init`. -_GBRAIN_HOME_CHECK="${GBRAIN_HOME:-$HOME/.gbrain}" +# #2521: GBRAIN_HOME is a PARENT dir per gbrain's configDir() contract — +# gbrain appends `.gbrain` itself, so the config lives at +# $GBRAIN_HOME/.gbrain/config.json (or ~/.gbrain/config.json when unset). +if [ -n "${GBRAIN_HOME:-}" ]; then + _GBRAIN_HOME_CHECK="$GBRAIN_HOME/.gbrain" +else + _GBRAIN_HOME_CHECK="$HOME/.gbrain" +fi if [ -f "$_GBRAIN_HOME_CHECK/config.json" ]; then if ! gbrain doctor --fast >/dev/null 2>&1; then echo "" >&2 diff --git a/lib/gbrain-exec.ts b/lib/gbrain-exec.ts index f13ea4a88..a7d32dea7 100644 --- a/lib/gbrain-exec.ts +++ b/lib/gbrain-exec.ts @@ -21,10 +21,12 @@ * spawn. This is the central bug the helper exists to prevent * regressing on. * - * 3. **`GBRAIN_HOME` honored consistently.** Other gstack helpers - * (`detectEngineTier`) already honor `GBRAIN_HOME`. `buildGbrainEnv` - * reads from `${GBRAIN_HOME:-$HOME/.gbrain}/config.json` so all - * gstack-side gbrain calls agree on which config file matters. + * 3. **`GBRAIN_HOME` honored consistently — with gbrain's own semantics + * (#2521).** gbrain's configDir() treats `GBRAIN_HOME` as a PARENT + * directory and always appends `.gbrain` itself (GBRAIN_HOME=/tmp/x + * → /tmp/x/.gbrain/config.json). Every gstack-side read goes through + * `gbrainConfigDir()` below so gstack and gbrain agree on which + * config file matters. * * **Escape hatch:** `GSTACK_RESPECT_ENV_DATABASE_URL=1` returns the * caller's env unchanged. Use only when the brain intentionally lives in @@ -75,8 +77,21 @@ export function isTransactionModePooler(url: string): boolean { } /** - * Build an env dict with DATABASE_URL seeded from - * `${GBRAIN_HOME:-$HOME/.gbrain}/config.json`. Returns the base env + * gbrain's config directory, matching gbrain's own configDir() contract + * (#2521): `GBRAIN_HOME` is a PARENT directory — gbrain always appends + * `.gbrain` itself, so GBRAIN_HOME=/tmp/x reads /tmp/x/.gbrain/config.json. + * Unset → ~/.gbrain. Every gstack-side gbrain-config read MUST resolve + * through this helper, or gstack classifies engine status from a file + * gbrain never reads. + */ +export function gbrainConfigDir(env: NodeJS.ProcessEnv = process.env): string { + if (env.GBRAIN_HOME) return join(env.GBRAIN_HOME, ".gbrain"); + return join(env.HOME || homedir(), ".gbrain"); +} + +/** + * Build an env dict with DATABASE_URL seeded from gbrain's config.json + * (resolved via `gbrainConfigDir`). Returns the base env * unchanged when: * - `GSTACK_RESPECT_ENV_DATABASE_URL=1` (intentional opt-out), * - the config file is missing or unparseable, @@ -98,9 +113,7 @@ export function buildGbrainEnv(opts: BuildGbrainEnvOptions = {}): NodeJS.Process const out: NodeJS.ProcessEnv = { ...baseEnv }; if (baseEnv.GSTACK_RESPECT_ENV_DATABASE_URL === "1") return out; - const homeBase = baseEnv.HOME || homedir(); - const gbrainHome = baseEnv.GBRAIN_HOME || join(homeBase, ".gbrain"); - const configPath = join(gbrainHome, "config.json"); + const configPath = join(gbrainConfigDir(baseEnv), "config.json"); if (!existsSync(configPath)) return out; let cfg: GbrainConfig = {}; diff --git a/lib/gbrain-guards.ts b/lib/gbrain-guards.ts index e983de260..1c46b4ad4 100644 --- a/lib/gbrain-guards.ts +++ b/lib/gbrain-guards.ts @@ -36,7 +36,9 @@ import { execGbrainJson, execGbrainText, NEEDS_SHELL_ON_WINDOWS } from "./gbrain import { parseSourcesList, type GbrainSourceRow } from "./gbrain-sources"; export function gbrainHome(env: NodeJS.ProcessEnv = process.env): string { - return env.GBRAIN_HOME || join(homedir(), ".gbrain"); + // #2521: GBRAIN_HOME is a PARENT dir per gbrain's configDir() contract — + // gbrain appends `.gbrain` itself, so gstack must too. + return env.GBRAIN_HOME ? join(env.GBRAIN_HOME, ".gbrain") : join(homedir(), ".gbrain"); } /** diff --git a/lib/gbrain-local-status.ts b/lib/gbrain-local-status.ts index e2f7b2879..f2ca41cb1 100644 --- a/lib/gbrain-local-status.ts +++ b/lib/gbrain-local-status.ts @@ -48,7 +48,7 @@ import { import { atomicWriteSync } from "./fs-atomic"; import { homedir } from "os"; import { dirname, join } from "path"; -import { buildGbrainEnv, NEEDS_SHELL_ON_WINDOWS } from "./gbrain-exec"; +import { buildGbrainEnv, gbrainConfigDir, NEEDS_SHELL_ON_WINDOWS } from "./gbrain-exec"; export type LocalEngineStatus = | "ok" @@ -122,11 +122,14 @@ export function cacheFilePath(): string { ); } -/** Honors GBRAIN_HOME (codex D11) — same resolution as buildGbrainEnv. */ +/** + * Honors GBRAIN_HOME (codex D11) with gbrain's own configDir() semantics + * (#2521): GBRAIN_HOME is a parent dir, `.gbrain` is appended. Same + * resolution as buildGbrainEnv — both route through gbrainConfigDir. + */ function gbrainConfigPath(env?: NodeJS.ProcessEnv): string { const e = env ?? process.env; - const gbrainHome = e.GBRAIN_HOME || join(userHome(e), ".gbrain"); - return join(gbrainHome, "config.json"); + return join(gbrainConfigDir(e), "config.json"); } /** diff --git a/lib/gstack-memory-helpers.ts b/lib/gstack-memory-helpers.ts index cca8515a1..91a786665 100644 --- a/lib/gstack-memory-helpers.ts +++ b/lib/gstack-memory-helpers.ts @@ -19,6 +19,7 @@ import { existsSync, readFileSync, writeFileSync, mkdirSync, statSync } from "fs"; import { appendJsonl } from "./jsonl-store"; +import { gbrainConfigDir } from "./gbrain-exec"; import { dirname, join } from "path"; import { execFileSync } from "child_process"; import { homedir } from "os"; @@ -256,12 +257,13 @@ export function detectEngineTier(): EngineDetect { } // 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 +// fallback to ~/.gbrain. Resolution matches gbrain's own configDir() +// contract (#2521): GBRAIN_HOME is a parent dir, `.gbrain` is appended. +// 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"); + return join(gbrainConfigDir(process.env), "config.json"); } // Best-effort JSONL append to ~/.gstack/.gbrain-errors.jsonl. Never throws. diff --git a/test/build-gbrain-env.test.ts b/test/build-gbrain-env.test.ts index 46403ba41..d97e3a8d2 100644 --- a/test/build-gbrain-env.test.ts +++ b/test/build-gbrain-env.test.ts @@ -77,17 +77,34 @@ describe("buildGbrainEnv", () => { expect(result.DATABASE_URL).toBe("postgresql://app/db"); }); - it("honors GBRAIN_HOME when set (config aligned with detectEngineTier)", () => { - // Move the config to an alternate dir; set GBRAIN_HOME to point at it. + it("honors GBRAIN_HOME when set, with gbrain's parent-dir semantics (#2521)", () => { + // Move the config to an alternate dir; set GBRAIN_HOME to point at its + // PARENT — gbrain's configDir() appends `.gbrain` itself, so + // GBRAIN_HOME=/x reads /x/.gbrain/config.json. const altGbrainHome = join(home, "alt-gbrain"); - mkdirSync(altGbrainHome, { recursive: true }); - writeFileSync(join(altGbrainHome, "config.json"), JSON.stringify({ database_url: "postgresql://alt/db" })); + mkdirSync(join(altGbrainHome, ".gbrain"), { recursive: true }); + writeFileSync( + join(altGbrainHome, ".gbrain", "config.json"), + JSON.stringify({ database_url: "postgresql://alt/db" }), + ); // No file at the default ~/.gbrain location. const baseEnv = { HOME: home, GBRAIN_HOME: altGbrainHome }; const result = buildGbrainEnv({ baseEnv }); expect(result.DATABASE_URL).toBe("postgresql://alt/db"); }); + it("ignores a config at $GBRAIN_HOME/config.json — gbrain never reads that file (#2521)", () => { + const altGbrainHome = join(home, "alt-gbrain-flat"); + mkdirSync(altGbrainHome, { recursive: true }); + writeFileSync( + join(altGbrainHome, "config.json"), + JSON.stringify({ database_url: "postgresql://alt/db" }), + ); + const baseEnv = { HOME: home, GBRAIN_HOME: altGbrainHome }; + const result = buildGbrainEnv({ baseEnv }); + expect(result.DATABASE_URL).toBeUndefined(); + }); + it("returns a fresh env object — never the caller's env by identity", () => { // Codex review #11: object-identity equality lets later mutation of the // returned env leak back into the caller's view. The helper MUST clone. diff --git a/test/code-intelligence.test.ts b/test/code-intelligence.test.ts index aac388f71..8d9705f32 100644 --- a/test/code-intelligence.test.ts +++ b/test/code-intelligence.test.ts @@ -953,7 +953,9 @@ exit 1 ...process.env, GSTACK_HOME: home, HOME: home, - GBRAIN_HOME: gbrainHome, + // #2521: GBRAIN_HOME is the PARENT of .gbrain per gbrain's configDir() + // contract — pointing it at `home` resolves to home/.gbrain/config.json. + GBRAIN_HOME: home, PATH: `${shimDir}:${process.env.PATH}`, // Dead loopback port → the Sourcebot probe fails fast + deterministically // (connection refused) instead of poking whatever operator dev server diff --git a/test/gbrain-local-status.test.ts b/test/gbrain-local-status.test.ts index a9aa8c7de..299f32f44 100644 --- a/test/gbrain-local-status.test.ts +++ b/test/gbrain-local-status.test.ts @@ -269,20 +269,46 @@ describe("lib/gbrain-local-status — status classification", () => { expect(localEngineStatus({ noCache: true })).toBe("timeout"); }); - it("honors GBRAIN_HOME for config detection (codex D11)", () => { - // Config lives ONLY at the alternate GBRAIN_HOME; ~/.gbrain has none. + it("honors GBRAIN_HOME for config detection (codex D11) with gbrain's parent-dir semantics (#2521)", () => { + // Config lives ONLY under the alternate GBRAIN_HOME; ~/.gbrain has none. + // gbrain's configDir() treats GBRAIN_HOME as a PARENT dir and appends + // `.gbrain` itself: GBRAIN_HOME=/x → /x/.gbrain/config.json. env = makeEnv({ withGbrain: true, gbrainBehavior: "ok", withConfig: false }); restoreEnv = applyEnv(env); const altHome = join(env.tmp, "alt-gbrain"); + mkdirSync(join(altHome, ".gbrain"), { recursive: true }); + writeFileSync( + join(altHome, ".gbrain", "config.json"), + JSON.stringify({ engine: "pglite", database_url: "pglite:///fake" }), + ); + // Without GBRAIN_HOME: misclassified as missing-config. + expect(localEngineStatus({ noCache: true })).toBe("missing-config"); + // With GBRAIN_HOME: the relocated config is found at $GBRAIN_HOME/.gbrain. + process.env.GBRAIN_HOME = altHome; + expect(localEngineStatus({ noCache: true })).toBe("ok"); + }); + + it("does NOT read $GBRAIN_HOME/config.json directly — gbrain never reads that file (#2521)", () => { + // A config placed at gstack's OLD (wrong) resolution must be invisible: + // gbrain itself would report "No brain configured" for this layout, so + // gstack classifying "ok" from it is the #2521 split-brain. + env = makeEnv({ withGbrain: true, gbrainBehavior: "ok", withConfig: false }); + restoreEnv = applyEnv(env); + const altHome = join(env.tmp, "alt-gbrain-flat"); mkdirSync(altHome, { recursive: true }); writeFileSync( join(altHome, "config.json"), JSON.stringify({ engine: "pglite", database_url: "pglite:///fake" }), ); - // Without GBRAIN_HOME: misclassified as missing-config. - expect(localEngineStatus({ noCache: true })).toBe("missing-config"); - // With GBRAIN_HOME: the relocated config is found. process.env.GBRAIN_HOME = altHome; + expect(localEngineStatus({ noCache: true })).toBe("missing-config"); + }); + + it("with GBRAIN_HOME unset, config resolution stays at ~/.gbrain (#2521 unset half)", () => { + env = makeEnv({ withGbrain: true, gbrainBehavior: "ok", withConfig: true }); + restoreEnv = applyEnv(env); + // applyEnv deletes GBRAIN_HOME; config was written at $HOME/.gbrain. + expect(process.env.GBRAIN_HOME).toBeUndefined(); expect(localEngineStatus({ noCache: true })).toBe("ok"); }); }); diff --git a/test/gstack-memory-helpers.test.ts b/test/gstack-memory-helpers.test.ts index 2bc89c83f..48a3dbd81 100644 --- a/test/gstack-memory-helpers.test.ts +++ b/test/gstack-memory-helpers.test.ts @@ -462,10 +462,13 @@ describe("detectEngineTier", () => { // 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. + // synthetic config under GBRAIN_HOME so the fallback path is + // deterministic. Per gbrain's configDir() contract (#2521), GBRAIN_HOME + // is a parent dir — the config lives at $GBRAIN_HOME/.gbrain/config.json. process.env.PATH = "/nonexistent-no-gbrain-here"; + mkdirSync(join(testGbrainHome, ".gbrain"), { recursive: true }); writeFileSync( - join(testGbrainHome, "config.json"), + join(testGbrainHome, ".gbrain", "config.json"), JSON.stringify({ engine: "postgres", database_url: "postgresql://test/example" }), "utf-8" ); @@ -500,8 +503,9 @@ exit 0 { mode: 0o755 } ); process.env.PATH = `${binDir}:${process.env.PATH || ""}`; + mkdirSync(join(testGbrainHome, ".gbrain"), { recursive: true }); writeFileSync( - join(testGbrainHome, "config.json"), + join(testGbrainHome, ".gbrain", "config.json"), JSON.stringify({ engine: "pglite" }), "utf-8" );