fix(sync): defer .gitignore write past first import, rebuild index, fail closed on unparseable db_only (#2964)

Fifth Codex review round (c17cd23) found the baseline-commit helper
interacting badly with the pre-existing db_only storage-tiering feature:

- P1 (data loss): createSyncBaselineCommit called manageGitignore BEFORE
  performFullSync's collectSyncableFiles ran. collectSyncableFiles
  enumerates via `git ls-files --cached --others --exclude-standard`, so
  writing db_only entries into .gitignore first would silently exclude
  those pages from the DATABASE, not just from git — on a brain's very
  first sync. This is the exact bug class runSync's existing "manage
  .gitignore ONLY on successful sync" ordering (this file, ~line 4540,
  itself a prior Codex P1 fix, comment literally says so) was written to
  prevent — my new code reintroduced it in a different spot. Fix: stopped
  calling manageGitignore inside the self-heal at all. db_only exclusion
  for the COMMIT still happens via the existing pathspec computation
  (independent of .gitignore); .gitignore itself gets written by the
  already-existing post-sync flow once this sync completes, same as any
  other sync.

- P1 (data leak): the unborn-HEAD recovery site can reach
  createSyncBaselineCommit with a repo whose INDEX already has entries
  staged from some prior operation (manual `git add`, interrupted
  workflow) before gbrain's self-heal ever touched it. `git add -A`
  only adds/updates — it doesn't drop an already-staged path our
  exclusion pathspecs now want excluded. Added `git read-tree --empty`
  to reset the index before staging (no-op on a freshly-`git init`-ed
  repo, whose index is already empty).

- P2: loadStorageConfig warns-and-returns an EMPTY config (not a throw)
  for syntactically-valid-but-unsupported YAML (e.g. flow-style
  `db_only: [dir/]` — the narrow custom parser only handles block-style
  lists), which would silently resolve zero exclusions from a gbrain.yml
  that clearly intended some. Added a sniff-test: if gbrain.yml exists
  and mentions db_only but nothing resolved from it, refuse the baseline
  commit rather than guess "genuinely empty" vs "syntax silently
  ignored" (git init may already have run by this point — same "unborn,
  retry on next sync" recovery path handles it, and will hit this same
  guard again until the user fixes gbrain.yml).

Tests: a positive regression proving db_only markdown IS imported into
the DB on first sync (the actual data-loss scenario), the sniff-test
refusal, and the stale-staged-content-gets-dropped case for the index
rebuild.

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

src/commands/sync.ts

@@ -972,17 +972,55 @@ export function discoverGitRoot(inputPath: string): string {
  * 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);
+function createSyncBaselineCommit(repoPath: string): void {
+  // #2964: db_only exclusion is computed directly from loadStorageConfig
+  // and passed to `git add` as pathspecs — deliberately NOT via
+  // manageGitignore/.gitignore, for two independent reasons:
+  //
+  // 1. Ordering (Codex review round 6, P1): `collectSyncableFiles` — the
+  //    file enumeration `performFullSync` runs right after this function
+  //    returns — honors `.gitignore` via `git ls-files --exclude-standard`.
+  //    Writing db_only entries into `.gitignore` BEFORE that first import
+  //    would silently exclude those pages from the database entirely.
+  //    That's the exact bug class `runSync`'s existing "manage .gitignore
+  //    ONLY on successful sync" ordering (this file, `manageGitignoreAtGitRoot`
+  //    callers below — itself a prior Codex P1 fix) exists to prevent. Leave
+  //    `.gitignore` untouched here; the existing post-sync flow writes it
+  //    once this sync completes, same as it does for every other sync.
+  // 2. Fail-closed (rounds 5-6): `manageGitignore`'s "warn and return" on a
+  //    broken gbrain.yml/unwritable .gitignore is the right default for its
+  //    OTHER callers (a side effect that must never kill the sync job), but
+  //    wrong for a commit we are creating ourselves — silently committing
+  //    db_only content into git history.
   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
+  const dbOnlyDirs = storageConfig?.db_only ?? [];
+  // Sniff-test fail-closed (round 6, P2): `loadStorageConfig` warns-and-
+  // returns an EMPTY config for syntactically-valid-but-unsupported YAML
+  // (e.g. flow-style `db_only: [dir/]` — the narrow custom parser only
+  // handles block-style lists), which would silently resolve zero
+  // exclusions from a file that clearly intended some. If gbrain.yml
+  // exists and mentions db_only but nothing resolved from it, refuse
+  // rather than guess "genuinely empty" vs "unsupported syntax ignored".
+  if (dbOnlyDirs.length === 0) {
+    const yamlPath = join(repoPath, 'gbrain.yml');
+    if (existsSync(yamlPath) && readFileSync(yamlPath, 'utf-8').includes('db_only')) {
+      throw new Error(
+        `${yamlPath} mentions db_only but no directories resolved from it — refusing to ` +
+          `auto-commit (cannot tell "genuinely empty" from "unsupported syntax silently ignored"). ` +
+          `Fix gbrain.yml's storage.db_only syntax, or git-init this directory manually.`,
+      );
+    }
+  }
+  // Only pathspec-exclude db_only dirs `.gitignore` does NOT already
+  // cover (a pre-existing `.gitignore` from before this brain lost its
+  // `.git` is possible even though we never write one here ourselves). 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 ?? [])
+  const excludePathspecs = dbOnlyDirs
     .filter((dir) => {
       try {
         git(repoPath, ['check-ignore', '-q', dir]);
@@ -992,6 +1030,15 @@ function createSyncBaselineCommit(repoPath: string, engineKind?: 'pglite' | 'pos
       }
     })
     .map((dir) => `:!${dir}`);
+  // Clear the index before staging (round 6, P1): the unborn-HEAD
+  // recovery site can reach this function with a repo whose index
+  // already has entries staged from some OTHER prior operation (a manual
+  // `git add`, an interrupted workflow) before gbrain ever touched it.
+  // `add -A` only adds/updates — it does not drop an already-staged path
+  // that our exclusion pathspecs above now want excluded. `read-tree
+  // --empty` resets the index without touching the working tree; a
+  // no-op on a freshly-`git init`-ed repo, whose index is already empty.
+  git(repoPath, ['read-tree', '--empty']);
   // #2964: 10 minutes, not the shared git() helper's 30s default — this
   // full-tree `git add -A` walks a legacy brain that may hold years of
   // accumulated content. A 30s timeout would abort staging after `git
@@ -1777,7 +1824,7 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
     }
     serr(`[gbrain] auto-recovery: git-initializing brain dir ${repoPath} (no git repo found).`);
     git(repoPath, ['init', '--quiet']);
-    createSyncBaselineCommit(repoPath, engine.kind);
+    createSyncBaselineCommit(repoPath);
     gitContextRoot = realpathSync(discoverGitRoot(repoPath));
   }
   const rawScopeRoot = opts.srcSubpath ? join(repoPath, opts.srcSubpath) : repoPath;
@@ -1926,7 +1973,7 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
       throw new Error(`No commits in repo ${repoPath}. Make at least one commit before syncing.`);
     }
     serr(`[gbrain] auto-recovery: repo has no commits yet, creating baseline commit ${gitContextRoot}.`);
-    createSyncBaselineCommit(gitContextRoot, engine.kind);
+    createSyncBaselineCommit(gitContextRoot);
     headCommit = git(gitContextRoot, ['rev-parse', 'HEAD']);
   }
 

test/sync-git-autoinit.test.ts

@@ -240,4 +240,68 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', ()
     const tracked = execSync('git ls-files', { cwd: dir }).toString();
     expect(tracked).not.toContain('private-cache');
   });
+
+  test('db_only markdown IS still imported into the DB on first sync (round 6 P1: ordering vs .gitignore)', async () => {
+    // If .gitignore had been written BEFORE the initial import (as an
+    // earlier version of this fix did via manageGitignore inside the
+    // self-heal), collectSyncableFiles's `git ls-files --exclude-standard`
+    // would have silently excluded this page from the database entirely
+    // — not just from git history, which is the only thing db_only is
+    // actually supposed to keep it out of.
+    const { performSync } = await import('../src/commands/sync.ts');
+    const { mkdirSync } = await import('fs');
+    mkdirSync(join(dir, 'private-cache'));
+    writeFileSync(join(dir, 'private-cache', 'note.md'), mdPage('DB-only note'));
+    writeFileSync(join(dir, 'gbrain.yml'), 'storage:\n  db_only:\n    - private-cache\n');
+
+    const result = await performSync(engine, { noPull: true, noEmbed: true, full: true });
+
+    expect(result.added).toBe(3); // page1, page2, private-cache/note
+    expect(await engine.getPage('private-cache/note')).not.toBeNull();
+  });
+
+  test('a gbrain.yml that mentions db_only but resolves no dirs refuses the baseline commit (round 6 P2: unsupported-syntax sniff test)', async () => {
+    const { performSync } = await import('../src/commands/sync.ts');
+    const { execSync } = await import('child_process');
+    // Flow-style array — valid YAML, but the narrow custom parser only
+    // handles block-style lists, so loadStorageConfig warns and resolves
+    // an empty db_only list rather than throwing.
+    writeFileSync(join(dir, 'gbrain.yml'), 'storage:\n  db_only: [private-cache/]\n');
+
+    await expect(
+      performSync(engine, { noPull: true, noEmbed: true, full: true }),
+    ).rejects.toThrow(/db_only/i);
+    // `git init` (site 1's first step) already ran before the sniff-test
+    // guard (inside createSyncBaselineCommit) refused — that's fine, it's
+    // the same "unborn repo" state the round-6-P1 index-rebuild test above
+    // recovers from on a later retry, which would hit this same guard and
+    // refuse again until gbrain.yml is fixed. What must NOT happen is a
+    // commit landing with unknown/unexcluded content.
+    expect(existsSync(join(dir, '.git'))).toBe(true);
+    let hasCommit = true;
+    try {
+      execSync('git rev-parse HEAD', { cwd: dir, stdio: 'pipe' });
+    } catch {
+      hasCommit = false;
+    }
+    expect(hasCommit).toBe(false);
+  });
+
+  test('unborn-HEAD recovery drops stale staged content the exclusion pathspec now wants excluded (round 6 P1: index rebuild)', 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');
+    // Simulate an interrupted workflow that left this file staged in an
+    // unborn repo BEFORE gbrain's self-heal ever ran.
+    execSync('git init -q', { cwd: dir });
+    execSync('git add private-cache/secret.bin', { cwd: dir });
+
+    await performSync(engine, { noPull: true, noEmbed: true, full: true });
+
+    const tracked = execSync('git ls-files', { cwd: dir }).toString();
+    expect(tracked).not.toContain('private-cache');
+  });
 });