From c20e4625fa3ef1b4369ae12259df17eb0708f122 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 9 Jul 2026 19:02:11 -0700 Subject: [PATCH] harden(gbrain-sources): route drift remove through #1734 guards + realpath drift check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Absorbing #2031 un-blocked a destructive remove that bypassed the #1734 data-loss guards: ensureSourceRegistered's drift path issued `gbrain sources remove` directly, without the detectAutopilot + decideSourceRemove checks every other remove routes through via safeSourcesRemove. gbrain >= 0.42's own prompt was accidentally blocking that path; with --confirm-destructive passed it is live again. - Drift remove now refuses LOUDLY (throws, actionable message) while an autopilot is active or when decideSourceRemove disallows; a silent changed=false would hide the drifted registration. - decideSourceRemove's extraArgs (--keep-storage when supported) propagate to the remove call, matching safeSourcesRemove. - Drift is realpath-normalized before being declared: a symlink alias of the same directory (macOS /tmp -> /private/tmp) is a match, not drift — the probable cause of #1985's reporter hitting the remove on an unmoved repo. - Drift fires a loud stderr line (old -> new path); perpetual drift in logs is the trigger for promoting #1985's reindex-in-place design. Tests: autopilot-active refusal (no remove in call log), fail-closed refusal on unreadable sources list, --keep-storage propagation, symlink-alias no-drift; existing drift tests pin the guard probes so a live autopilot on the dev machine can't flip them. Co-Authored-By: Claude Fable 5 --- lib/gbrain-sources.ts | 79 +++++++++++++++++++++++++--- test/gbrain-sources.test.ts | 102 +++++++++++++++++++++++++++++++++++- 2 files changed, 172 insertions(+), 9 deletions(-) diff --git a/lib/gbrain-sources.ts b/lib/gbrain-sources.ts index 9c5b2be82..826e905ac 100644 --- a/lib/gbrain-sources.ts +++ b/lib/gbrain-sources.ts @@ -10,8 +10,15 @@ */ import { execFileSync, spawnSync } from "child_process"; +import { realpathSync } from "fs"; import { withErrorContext } from "./gstack-memory-helpers"; import { execGbrainJson, NEEDS_SHELL_ON_WINDOWS } from "./gbrain-exec"; +import { + detectAutopilot, + decideSourceRemove, + type AutopilotProbe, + type DecideRemoveOpts, +} from "./gbrain-guards"; export interface SourceState { /** "absent" — id not registered. "match" — id at expected path. "drift" — id at different path. */ @@ -70,6 +77,33 @@ export interface EnsureOptions { * mutations of process.env.PATH unless env is passed explicitly). */ env?: NodeJS.ProcessEnv; + /** + * #1734 test hooks for the drift-remove guards. Production callers leave + * these unset (real autopilot detection + real remove decision). Tests pin + * them so a live autopilot on the dev machine can't flip test outcomes. + */ + autopilotProbe?: AutopilotProbe; + removeDecision?: DecideRemoveOpts; +} + +/** + * Path equality with realpath normalization (macOS /tmp -> /private/tmp, + * symlinked worktrees). A registered path that resolves to the same real + * directory is NOT drift — declaring it drift triggers a destructive + * remove+add and a full re-index for a no-op (#1985 reporter hit the remove + * on an unmoved repo). + */ +function samePath(registered: string | undefined, requested: string): boolean { + if (!registered) return false; + if (registered === requested) return true; + const real = (p: string): string => { + try { + return realpathSync(p); + } catch { + return p; + } + }; + return real(registered) === real(requested); } /** @@ -142,9 +176,10 @@ export async function ensureSourceRegistered( return withErrorContext(`ensureSourceRegistered:${id}`, () => { const probed = probeSource(id, env); - // Disambiguate match-but-different-path + // Disambiguate match-but-different-path (realpath-normalized: a symlink + // alias of the same directory is a match, not drift). let state: SourceState = probed; - if (probed.status === "match" && probed.registered_path !== path) { + if (probed.status === "match" && !samePath(probed.registered_path, path)) { state = { status: "drift", registered_path: probed.registered_path }; } @@ -165,12 +200,40 @@ export async function ensureSourceRegistered( // stage for any source that has drifted to a new path. This matches the // flag the orchestrator's own safeSourcesRemove() already passes. if (state.status === "drift") { - const rm = spawnSync("gbrain", ["sources", "remove", id, "--yes", "--confirm-destructive"], { - encoding: "utf-8", - timeout: 30_000, - env, - shell: NEEDS_SHELL_ON_WINDOWS, // #1731: gbrain is a .cmd shim on Windows - }); + // Loud drift observability: if this line shows up on every sync for some + // environment, drift is perpetual there and the reindex-in-place design + // from #1985 should be promoted (drop+rebuild re-embeds the full index). + console.error( + `[gbrain-sources] drift: ${id} registered at ${state.registered_path} -> re-registering at ${path}`, + ); + + // #1734: this remove deletes the source's pages/chunks/embeddings, so it + // runs only behind the same data-loss guards as the orchestrator's + // safeSourcesRemove(). A refusal is FATAL here (not best-effort): without + // the remove the add cannot proceed, and returning changed=false would + // silently hide the drifted registration. + const ap = detectAutopilot(env ?? process.env, options.autopilotProbe ?? {}); + if (ap.active) { + throw new Error( + `refusing drift re-register of ${id}: autopilot active (${ap.signal}). ` + + `Stop autopilot, then re-run /sync-gbrain.`, + ); + } + const decision = decideSourceRemove(id, env ?? process.env, options.removeDecision ?? {}); + if (!decision.allow) { + throw new Error(`refusing drift re-register of ${id}: ${decision.reason}`); + } + + const rm = spawnSync( + "gbrain", + ["sources", "remove", id, "--yes", "--confirm-destructive", ...decision.extraArgs], + { + encoding: "utf-8", + timeout: 30_000, + env, + shell: NEEDS_SHELL_ON_WINDOWS, // #1731: gbrain is a .cmd shim on Windows + }, + ); if (rm.status !== 0) { throw new Error(`gbrain sources remove ${id} failed: ${rm.stderr || rm.stdout || `exit ${rm.status}`}`); } diff --git a/test/gbrain-sources.test.ts b/test/gbrain-sources.test.ts index 63cbb65a7..810624755 100644 --- a/test/gbrain-sources.test.ts +++ b/test/gbrain-sources.test.ts @@ -8,7 +8,7 @@ */ import { describe, it, expect } from "bun:test"; -import { mkdtempSync, writeFileSync, readFileSync, existsSync, mkdirSync, rmSync, chmodSync } from "fs"; +import { mkdtempSync, writeFileSync, readFileSync, existsSync, mkdirSync, rmSync, chmodSync, symlinkSync } from "fs"; import { tmpdir } from "os"; import { join } from "path"; @@ -168,6 +168,13 @@ describe("ensureSourceRegistered", () => { fake.cleanup(); }); + // Drift-path tests pin the #1734 guard inputs (inactive autopilot, allowed + // remove) so a REAL autopilot running on the dev machine can't flip them. + const guardsPinnedInactive = { + autopilotProbe: { lockPaths: [], processRunning: () => false }, + removeDecision: { keepStorage: false }, + } as const; + it("recreates source when path differs (gbrain has no `sources update`), returns changed=true", async () => { const fake = makeFakeGbrain({ sources: [{ id: "gstack-code-foo", local_path: "/old/path" }], @@ -175,6 +182,7 @@ describe("ensureSourceRegistered", () => { const result = await ensureSourceRegistered("gstack-code-foo", "/new/path", { federated: true, env: fake.env, + ...guardsPinnedInactive, }); expect(result.changed).toBe(true); expect(result.state.status).toBe("match"); @@ -201,6 +209,7 @@ describe("ensureSourceRegistered", () => { const result = await ensureSourceRegistered("gstack-code-foo", "/new/path", { federated: true, env: fake.env, + ...guardsPinnedInactive, }); expect(result.changed).toBe(true); expect(result.state.status).toBe("match"); @@ -214,6 +223,97 @@ describe("ensureSourceRegistered", () => { fake.cleanup(); }); + // #1734 tripwire: the drift remove deletes pages/chunks/embeddings, so it + // must refuse while a gbrain autopilot is active — and refuse LOUDLY (throw), + // not silently return changed=false. Before the guard routing, this path + // issued the remove unconditionally. + it("REFUSES the drift remove while autopilot is active (throws, no remove issued)", async () => { + const fake = makeFakeGbrain({ + sources: [{ id: "gstack-code-foo", local_path: "/old/path" }], + }); + await expect( + ensureSourceRegistered("gstack-code-foo", "/new/path", { + env: fake.env, + autopilotProbe: { lockPaths: [], processRunning: () => true }, + removeDecision: { keepStorage: false }, + }), + ).rejects.toThrow(/autopilot active/); + + const log = readFileSync(fake.logPath, "utf-8"); + expect(log).not.toContain("sources remove"); + expect(log).not.toContain("sources add"); + fake.cleanup(); + }); + + it("REFUSES the drift remove when decideSourceRemove disallows (fail closed, throws)", async () => { + const fake = makeFakeGbrain({ + sources: [{ id: "gstack-code-foo", local_path: "/old/path" }], + }); + await expect( + ensureSourceRegistered("gstack-code-foo", "/new/path", { + env: fake.env, + autopilotProbe: { lockPaths: [], processRunning: () => false }, + // A sources-list read failure makes decideSourceRemove fail closed. + removeDecision: { + keepStorage: false, + fetchRows: () => { + throw new Error("sources list unavailable"); + }, + }, + }), + ).rejects.toThrow(/fail closed/); + + const log = readFileSync(fake.logPath, "utf-8"); + expect(log).not.toContain("sources remove"); + fake.cleanup(); + }); + + it("propagates decideSourceRemove extraArgs (--keep-storage) to the drift remove", async () => { + const fake = makeFakeGbrain({ + sources: [{ id: "gstack-code-foo", local_path: "/old/path" }], + }); + const result = await ensureSourceRegistered("gstack-code-foo", "/new/path", { + env: fake.env, + autopilotProbe: { lockPaths: [], processRunning: () => false }, + removeDecision: { keepStorage: true }, + }); + expect(result.changed).toBe(true); + + const log = readFileSync(fake.logPath, "utf-8"); + expect(log).toContain( + "sources remove gstack-code-foo --yes --confirm-destructive --keep-storage", + ); + fake.cleanup(); + }); + + // Realpath normalization: a registered path that is a symlink alias of the + // requested path is a MATCH, not drift. Declaring it drift triggers a + // destructive remove + full re-index for a no-op (#1985 reporter hit the + // remove on an unmoved repo; macOS /tmp -> /private/tmp is the usual cause). + it("does NOT declare drift when registered path is a symlink alias of the requested path", async () => { + const base = mkdtempSync(join(tmpdir(), "gbrain-sources-realpath-")); + const realDir = join(base, "real-repo"); + const linkDir = join(base, "link-repo"); + mkdirSync(realDir, { recursive: true }); + symlinkSync(realDir, linkDir); + + const fake = makeFakeGbrain({ + sources: [{ id: "gstack-code-foo", local_path: realDir }], + }); + const result = await ensureSourceRegistered("gstack-code-foo", linkDir, { + env: fake.env, + ...guardsPinnedInactive, + }); + expect(result.changed).toBe(false); + expect(result.state.status).toBe("match"); + + const log = readFileSync(fake.logPath, "utf-8"); + expect(log).not.toContain("sources remove"); + expect(log).not.toContain("sources add"); + fake.cleanup(); + rmSync(base, { recursive: true, force: true }); + }); + it("when reregister_on_drift=false and source is at different path, returns changed=false", async () => { const fake = makeFakeGbrain({ sources: [{ id: "gstack-code-foo", local_path: "/old/path" }],