From e68791361b441d01f71332aec0c115b34fd13e38 Mon Sep 17 00:00:00 2001 From: masashiono0611 Date: Sun, 19 Jul 2026 02:51:40 +0900 Subject: [PATCH] fix(sync): defer .gitignore write past first import, rebuild index, fail closed on unparseable db_only (#2964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS --- src/commands/sync.ts | 61 ++++++++++++++++++++++++++++---- test/sync-git-autoinit.test.ts | 64 ++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 7 deletions(-) diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 3c057afb4..034f60e39 100644 --- a/src/commands/sync.ts +++ b/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 { + // 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'); + }); });