fix(sync): prove self-heal ownership by anchor VALUE, not field presence (#2964)

Third Codex review round (613ad5b) found the previous round's fix broke
the very call site it was meant to repair, plus a second scope gap:

- P1 (critical): !opts.repoPath rejected self-heal on the REAL production
  callers too. runPhaseSync (dream cycle's sync phase, cycle.ts) always
  passes `repoPath: brainDir` explicitly after resolving it upstream, and
  the CLI's bare `gbrain sync` resolves sourceId='default'. Both made the
  ownership gate rethrow, leaving `gbrain dream` and `gbrain sync`
  wedged on the exact non-git legacy brain this fix targets — only
  synthetic callers that omitted both fields ever healed.

  Fixed by proving ownership by VALUE instead of by field absence: a new
  isAnchorOwnedSyncPath() re-reads gbrain's own persisted
  sync.repo_path config and requires the resolved repoPath to equal it
  exactly, regardless of whether the caller passed it explicitly or let
  it default. An attacker-supplied arbitrary path (e.g. via
  submit_job({name:'sync', data:{repoPath}})) only self-heals if it
  happens to already equal gbrain's own anchor — which is the
  legitimate case, not an escalation. opts.sourceId and opts.srcSubpath
  still disqualify unconditionally (registered/subpath-scoped syncs are
  a different ownership context).

- P2: a --src-subpath sync with an unborn parent-repo HEAD would commit
  the whole ancestor root, capturing sibling files outside the scope.
  isAnchorOwnedSyncPath's opts.srcSubpath check closes this; the
  existing gitContextRoot === repoPath check stays as defense in depth.

- P2: manageGitignore's "warn and return" contract (a deliberate side-
  effect that must never kill the sync job for its OTHER callers) meant
  a broken gbrain.yml or unwritable .gitignore would silently let the
  baseline `git add -A` commit db_only content. createSyncBaselineCommit
  now recomputes db_only exclusion directly from loadStorageConfig and
  passes it to `git add` as pathspecs, independent of the .gitignore
  write's success — true fail-closed. (A redundant pathspec exclude for
  a path .gitignore ALREADY covers makes git's -A bail with "paths
  ignored, use -f" even though the negation is correct, so each dir is
  check-ignore'd first and only pathspec-excluded when NOT already
  covered.)

Tests rewritten around the anchor-VALUE model: the critical regression
case (explicit repoPath matching the anchor still heals — the exact
scenario Codex proved was broken) plus a true negative (a caller-supplied
path that does NOT match the anchor still throws), --src-subpath refusal,
and db_only fail-closed exclusion.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS

src/commands/sync.ts

@@ -958,10 +958,41 @@ export function discoverGitRoot(inputPath: string): string {
  * headless nightly cron/launchd invocation, which has no reason to have
  * git signing/identity configured, and must not block on an unavailable
  * signing agent or pinentry prompt.
+ *
+ * db_only exclusion is recomputed directly and passed to `git add` as
+ * negative pathspecs, rather than relying solely on `manageGitignore`
+ * having written `.gitignore` successfully: that helper is deliberately
+ * best-effort (a broken gbrain.yml parse, or an unwritable .gitignore,
+ * only warns and returns — the right default for its OTHER callers, where
+ * .gitignore management is a side effect that must never kill the sync
+ * job). For a commit we are about to create ourselves, "fail open" there
+ * would mean silently committing db_only content into git history. Fail
+ * closed instead: db_only exclusion doesn't depend on the .gitignore
+ * write having succeeded. `loadStorageConfig` throwing (unreadable
+ * gbrain.yml, or a semantic overlap) propagates — better to leave this
+ * self-heal wedged with a clear error than commit unknown content.
  */
 function createSyncBaselineCommit(repoPath: string, engineKind?: 'pglite' | 'postgres'): void {
   manageGitignore(repoPath, engineKind);
-  git(repoPath, ['add', '-A']);
+  const storageConfig = loadStorageConfig(repoPath);
+  // Only pathspec-exclude db_only dirs `.gitignore` did NOT already cover.
+  // A redundant negation pathspec for an already-ignored path makes git's
+  // `-A` bail with "paths ignored by .gitignore, use -f"
+  // (advice.addIgnoredFile) even though the negation is semantically
+  // correct and git would otherwise skip it silently. `check-ignore`
+  // throwing (not ignored, or the check itself failed) defaults to
+  // excluding explicitly — the fail-closed direction.
+  const excludePathspecs = (storageConfig?.db_only ?? [])
+    .filter((dir) => {
+      try {
+        git(repoPath, ['check-ignore', '-q', dir]);
+        return false;
+      } catch {
+        return true;
+      }
+    })
+    .map((dir) => `:!${dir}`);
+  git(repoPath, ['add', '-A', '--', '.', ...excludePathspecs]);
   git(
     repoPath,
     ['commit', '--quiet', '--allow-empty', '--no-gpg-sign', '-m', 'gbrain: initial commit (auto-init by sync)'],
@@ -1035,6 +1066,38 @@ async function readSyncAnchor(
   return await engine.getConfig(`sync.${which}`);
 }
 
+/**
+ * #2964: is `repoPath` gbrain's own legacy default anchor, as opposed to a
+ * path some caller merely happened to pass through unchanged?
+ *
+ * `!opts.sourceId` alone is NOT sufficient: `runPhaseSync` (dream cycle)
+ * and `submit_job({name:'sync', data:{repoPath}})` (MCP, jobs.ts) both
+ * reach `performSyncInner` with `sourceId` undefined AND `opts.repoPath`
+ * set explicitly — the former legitimately (it already resolved the
+ * anchor itself and is passing it through), the latter potentially with
+ * an attacker-controlled path. The two are indistinguishable from
+ * `opts.repoPath`'s mere presence/absence, so ownership has to be proven
+ * by VALUE: read the anchor fresh from config and require the resolved
+ * `repoPath` to equal it exactly. An arbitrary caller-supplied path only
+ * passes this check if it happens to already equal gbrain's own anchor —
+ * at which point self-healing it is exactly the legitimate case.
+ *
+ * `opts.srcSubpath` disqualifies unconditionally: a subpath-scoped sync
+ * only wants THAT subdirectory captured, but the self-heal baseline
+ * commit runs `git add -A` at the git root (there's no file list yet to
+ * scope it to — collection happens after this point) — see the P2 review
+ * finding on `createSyncBaselineCommit`'s callers.
+ */
+async function isAnchorOwnedSyncPath(
+  engine: BrainEngine,
+  opts: SyncOpts,
+  repoPath: string,
+): Promise<boolean> {
+  if (opts.sourceId || opts.srcSubpath) return false;
+  const anchor = await readSyncAnchor(engine, undefined, 'repo_path');
+  return anchor !== null && anchor === repoPath;
+}
+
 async function writeSyncAnchor(
   engine: BrainEngine,
   sourceId: string | undefined,
@@ -1654,30 +1717,23 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
   //   - syncScopeRoot:  file walking, imports, deletes, renames
   // In the common case (repoPath == git root, no subpath) they are identical.
   serr(`[gbrain phase] sync.discover_git_root`);
-  // #2964: a legacy `sync.repo_path`-anchored default brain (no `sources`
-  // row, no `opts.sourceId`, no caller-supplied `opts.repoPath`) can reach
-  // here having never been `git init`-ed — e.g. a brain-pages dir that
-  // predates git-backed sync, or one rsync'd from another machine without
-  // its `.git`. gbrain owns THAT directory outright (it's the one path
-  // resolved from gbrain's own persisted anchor, not a caller-named one),
-  // so self-heal by initializing it in place instead of failing the sync
-  // phase every single run. Mirrors the recloneIfMissing self-recovery
-  // above for owned remote clones.
-  //
-  // Ownership gate — `!opts.repoPath` is the load-bearing check, not just
-  // `!opts.sourceId`: `submit_job({name:'sync', data:{repoPath}})` reaches
-  // performSyncInner with sourceId undefined whenever the given path
-  // doesn't match a registered source's local_path (jobs.ts), so an
-  // admin-scope MCP caller could otherwise point this at an arbitrary
-  // directory and have it silently git-init + commit + ingest it. Only the
-  // anchor-resolved default path (`repoPath = opts.repoPath || anchor`,
-  // taking the anchor branch) is something gbrain chose for itself.
-  // `!opts.dryRun`: a dry-run preview must never write to disk.
+  // #2964: a legacy `sync.repo_path`-anchored default brain can reach here
+  // having never been `git init`-ed — e.g. a brain-pages dir that predates
+  // git-backed sync, or one rsync'd from another machine without its
+  // `.git`. gbrain owns that directory outright, so self-heal by
+  // initializing it in place instead of failing the sync phase every
+  // single run. Mirrors the recloneIfMissing self-recovery above for
+  // owned remote clones. Ownership is proven by VALUE (resolved repoPath
+  // equals gbrain's persisted anchor) via `isAnchorOwnedSyncPath`, not by
+  // the mere absence of `opts.sourceId`/`opts.repoPath` — see that
+  // function's docstring. `!opts.dryRun`: a preview must never write.
   let gitContextRoot: string;
   try {
     gitContextRoot = realpathSync(discoverGitRoot(repoPath));
   } catch (err) {
-    if (opts.sourceId || opts.repoPath || opts.dryRun || !existsSync(repoPath)) throw err;
+    if (opts.dryRun || !existsSync(repoPath) || !(await isAnchorOwnedSyncPath(engine, opts, repoPath))) {
+      throw err;
+    }
     serr(`[gbrain] auto-recovery: git-initializing brain dir ${repoPath} (no git repo found).`);
     git(repoPath, ['init', '--quiet']);
     createSyncBaselineCommit(repoPath, engine.kind);
@@ -1813,17 +1869,18 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
     // this brain permanently wedged on "No commits in repo" every night
     // thereafter. Finish the same baseline-commit self-heal the
     // discoverGitRoot catch above would have done, gated the same way
-    // (gbrain-owned default brain only, never on a dry-run preview) PLUS a
-    // scope check: `discoverGitRoot` walks UP from `repoPath`, so for a
-    // `--src-subpath`/subdir-as-repoPath sync it can resolve to an ANCESTOR
-    // repo, not `repoPath` itself. Committing there (`git add -A` at
-    // gitContextRoot) would capture sibling files well outside the sync
-    // scope — refuse instead of guessing.
+    // (ownership proven by value, never on a dry-run preview) PLUS a scope
+    // check: `discoverGitRoot` walks UP from `repoPath`, so it can resolve
+    // to an ANCESTOR repo, not `repoPath` itself (most plausible for a
+    // `--src-subpath` sync, but `isAnchorOwnedSyncPath` already refuses
+    // that case — kept here too as defense in depth against any other path
+    // where gitContextRoot could diverge from repoPath). Committing at an
+    // ancestor (`git add -A` at gitContextRoot) would capture sibling
+    // files well outside the sync scope — refuse instead of guessing.
     if (
-      opts.sourceId ||
-      opts.repoPath ||
       opts.dryRun ||
-      gitContextRoot !== realpathSync(repoPath)
+      gitContextRoot !== realpathSync(repoPath) ||
+      !(await isAnchorOwnedSyncPath(engine, opts, repoPath))
     ) {
       throw new Error(`No commits in repo ${repoPath}. Make at least one commit before syncing.`);
     }

test/sync-git-autoinit.test.ts

@@ -1,24 +1,30 @@
 /**
  * #2964 — sync phase self-heals a never-git-initialized default brain dir.
  *
- * A legacy `sync.repo_path`-anchored default brain (no `sources` row, no
- * `--source`, no caller-supplied `--repo`) can reach `performSync` pointed
- * at a directory that was never `git init`-ed (predates git-backed sync, or
- * was rsync'd from another machine without its `.git`). Before this fix,
- * `discoverGitRoot` threw unconditionally and the dream cycle's sync phase
- * failed every night with no self-recovery, even though `doctor`'s sync
- * checks reported "ok" (for an unrelated reason — they only look at the
- * `sources` table, which has no rows for this kind of brain).
+ * A legacy `sync.repo_path`-anchored default brain (no `sources` row) can
+ * reach `performSync` pointed at a directory that was never `git init`-ed
+ * (predates git-backed sync, or was rsync'd from another machine without
+ * its `.git`). Before this fix, `discoverGitRoot` threw unconditionally and
+ * the dream cycle's sync phase failed every night with no self-recovery,
+ * even though `doctor`'s sync checks reported "ok" (for an unrelated
+ * reason — they only look at the `sources` table, which has no rows for
+ * this kind of brain).
  *
- * gbrain owns THAT directory outright, so the fix self-heals by `git
+ * gbrain owns that directory outright, so the fix self-heals by `git
  * init`-ing it and capturing the current on-disk state as the sync
- * baseline — but ONLY when the path was resolved from gbrain's own
- * persisted `sync.repo_path` anchor, never `sourceId` and never a
- * caller-supplied `repoPath`. Either of those means someone else named the
- * directory (a registered local source, or — per Codex review — an
- * admin-scope `submit_job({name:'sync', data:{repoPath}})` caller with no
- * matching source row), which must keep failing loudly rather than being
- * silently git-initialized without consent.
+ * baseline. Ownership is proven by VALUE — the resolved `repoPath` must
+ * equal gbrain's own persisted `sync.repo_path` anchor — not by whether
+ * `opts.repoPath`/`opts.sourceId` happen to be set. That distinction
+ * matters because the real production caller (`runPhaseSync` in
+ * cycle.ts, i.e. `gbrain dream`'s sync phase) always passes the resolved
+ * anchor through explicitly as `opts.repoPath`; gating on
+ * `!opts.repoPath` (an earlier, insufficiently-reviewed version of this
+ * fix) would have made the self-heal never fire on the exact path it was
+ * written to repair (Codex review round 3 caught this). A caller-supplied
+ * path that does NOT match the anchor (a registered local source, or an
+ * admin-scope `submit_job({name:'sync', data:{repoPath}})` MCP call with
+ * an unrelated path) must keep failing loudly rather than being silently
+ * git-initialized without consent.
  */
 
 import { describe, test, expect, beforeAll, afterAll, beforeEach, afterEach } from 'bun:test';
@@ -51,8 +57,9 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
     dir = mkdtempSync(join(tmpdir(), 'gbrain-2964-'));
     writeFileSync(join(dir, 'page1.md'), mdPage('Page 1'));
     writeFileSync(join(dir, 'page2.md'), mdPage('Page 2'));
-    // The self-heal-eligible path: gbrain's own persisted anchor, not a
-    // caller-supplied --repo / job.data.repoPath.
+    // The self-heal-eligible anchor: gbrain's own persisted config, not a
+    // caller-supplied --repo / job.data.repoPath (those are proven by
+    // VALUE against this anchor, not by mere absence — see file docstring).
     await engine.setConfig('sync.repo_path', dir);
   });
 
@@ -64,11 +71,7 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
     const { performSync } = await import('../src/commands/sync.ts');
     expect(existsSync(join(dir, '.git'))).toBe(false);
 
-    const result = await performSync(engine, {
-      noPull: true,
-      noEmbed: true,
-      full: true,
-    });
+    const result = await performSync(engine, { noPull: true, noEmbed: true, full: true });
 
     expect(result.status).toBe('first_sync');
     expect(result.added).toBe(2);
@@ -77,6 +80,22 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
     expect(await engine.getPage('page2')).not.toBeNull();
   });
 
+  test('explicit repoPath matching the anchor still auto-inits (mirrors gbrain dream\'s sync phase)', async () => {
+    // cycle.ts's runPhaseSync (the actual dream-cycle call site this bug
+    // was filed against) always passes `repoPath: brainDir` explicitly —
+    // it already resolved the anchor itself upstream and threads it
+    // through. Gating self-heal on `!opts.repoPath` would silently never
+    // fire here; ownership must be proven by matching the anchor's VALUE.
+    const { performSync } = await import('../src/commands/sync.ts');
+    expect(existsSync(join(dir, '.git'))).toBe(false);
+
+    const result = await performSync(engine, { repoPath: dir, noPull: true, noEmbed: true, full: true });
+
+    expect(result.status).toBe('first_sync');
+    expect(result.added).toBe(2);
+    expect(existsSync(join(dir, '.git'))).toBe(true);
+  });
+
   test('a second sync after auto-init sees no changes (baseline commit captured current on-disk state)', async () => {
     const { performSync } = await import('../src/commands/sync.ts');
     const first = await performSync(engine, { noPull: true, noEmbed: true, full: true });
@@ -106,16 +125,34 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
     expect(existsSync(join(dir, '.git'))).toBe(false);
   });
 
-  test('a caller-supplied repoPath with no sourceId still throws — not auto-inited (P1: MCP submit_job arbitrary-path guard)', async () => {
+  test('a caller-supplied repoPath that does NOT match the anchor still throws (P1: MCP submit_job arbitrary-path guard)', async () => {
     // Mirrors jobs.ts: submit_job({name:'sync', data:{repoPath}}) reaches
     // performSyncInner with sourceId left undefined whenever repoPath
-    // doesn't match a registered source's local_path. Auto-init must not
-    // fire here even though sourceId is unset — only the anchor-resolved
-    // default path is gbrain's own.
+    // doesn't match a registered source's local_path. Self-heal must not
+    // fire for a path that isn't gbrain's own anchor, even with no
+    // sourceId set — only exact anchor-value equality (the previous test)
+    // is eligible.
+    const other = mkdtempSync(join(tmpdir(), 'gbrain-2964-other-'));
+    writeFileSync(join(other, 'unrelated.md'), mdPage('Unrelated'));
+    try {
+      const { performSync } = await import('../src/commands/sync.ts');
+      await expect(
+        performSync(engine, { repoPath: other, noPull: true, noEmbed: true, full: true }),
+      ).rejects.toThrow(/git repository/i);
+      expect(existsSync(join(other, '.git'))).toBe(false);
+    } finally {
+      rmSync(other, { recursive: true, force: true });
+    }
+  });
+
+  test('--src-subpath on the anchor-resolved path still throws — not auto-inited (P2: subpath scope guard)', async () => {
+    // A self-heal baseline commit runs `git add -A` at the git root before
+    // any subpath-scoped file collection happens, so it would capture
+    // sibling directories a --src-subpath sync never intended to touch.
     const { performSync } = await import('../src/commands/sync.ts');
     await expect(
       performSync(engine, {
-        repoPath: dir,
+        srcSubpath: 'wiki',
         noPull: true,
         noEmbed: true,
         full: true,
@@ -127,10 +164,10 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
   test('--dry-run on the anchor-resolved path throws without writing anything to disk', async () => {
     const { performSync } = await import('../src/commands/sync.ts');
     await expect(
-      performSync(engine, { dryRun: true, noPull: true, noEmbed: true, full: true }),
+      performSync(engine, { repoPath: dir, dryRun: true, noPull: true, noEmbed: true, full: true }),
     ).rejects.toThrow(/git repository/i);
     // The whole point of --dry-run is "preview only" — it must never git-init
-    // or commit on our behalf, even though we could self-heal.
+    // or commit on our behalf, even though this is otherwise self-heal-eligible.
     expect(existsSync(join(dir, '.git'))).toBe(false);
   });
 
@@ -142,10 +179,28 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
     // discoverGitRoot succeeds, but `git rev-parse HEAD` still fails.
     execSync('git init -q', { cwd: dir });
 
-    const result = await performSync(engine, { noPull: true, noEmbed: true, full: true });
+    const result = await performSync(engine, { repoPath: dir, noPull: true, noEmbed: true, full: true });
 
     expect(result.status).toBe('first_sync');
     expect(result.added).toBe(2);
     expect(execSync('git rev-parse HEAD', { cwd: dir }).toString().trim()).not.toBe('');
   });
+
+  test('db_only paths are excluded from the baseline commit even without gbrain.yml write support (P2: fail-closed exclusion)', async () => {
+    const { performSync } = await import('../src/commands/sync.ts');
+    const { mkdirSync } = await import('fs');
+    const { execSync } = await import('child_process');
+    mkdirSync(join(dir, 'private-cache'));
+    writeFileSync(join(dir, 'private-cache', 'secret.bin'), 'binary-ish content');
+    writeFileSync(
+      join(dir, 'gbrain.yml'),
+      'storage:\n  db_only:\n    - private-cache\n',
+    );
+
+    await performSync(engine, { noPull: true, noEmbed: true, full: true });
+
+    expect(existsSync(join(dir, '.git'))).toBe(true);
+    const tracked = execSync('git ls-files', { cwd: dir }).toString();
+    expect(tracked).not.toContain('private-cache');
+  });
 });