diff --git a/src/commands/sync.ts b/src/commands/sync.ts index f17acfc09..36180d823 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -1,4 +1,4 @@ -import { existsSync, readFileSync, writeFileSync, statSync, realpathSync, unlinkSync } from 'fs'; +import { existsSync, readFileSync, writeFileSync, statSync, realpathSync } from 'fs'; import { execFileSync } from 'child_process'; import { join, relative } from 'path'; import type { BrainEngine } from '../core/engine.ts'; @@ -1003,6 +1003,20 @@ function createSyncBaselineCommit(repoPath: string): void { // `supabase_only` — same keep-out-of-git semantics, still a supported // backward-compat key per storage-config.ts) but nothing resolved from // it, refuse rather than guess "genuinely empty" vs "syntax ignored". + // + // Known false-positive (round 8 review): a genuinely, intentionally + // empty `db_only: []` mentioning the word also refuses, and can't be + // told apart from the unsupported-syntax case — `loadStorageConfig` + // returns the IDENTICAL `{db_tracked:[],db_only:[]}` for both (verified + // directly: flow-style `[dir/]` and literal `[]` both collapse to that + // same shape). Distinguishing them would mean teaching this function + // about the parser's internal line-recognition rules, which belongs in + // storage-config.ts, not here. Accepted trade-off: the false-positive + // cost is low and self-resolving (the brain stays wedged with a clear, + // actionable error until the user drops the pointless empty stanza or + // fixes their syntax; retried on every subsequent sync); the + // false-negative this guards against — silently committing db_only + // content into permanent git history — is high-cost and hard to undo. if (dbOnlyDirs.length === 0) { const yamlPath = join(repoPath, 'gbrain.yml'); const yamlContent = existsSync(yamlPath) ? readFileSync(yamlPath, 'utf-8') : ''; @@ -1052,15 +1066,24 @@ function createSyncBaselineCommit(repoPath: string): void { git(repoPath, ['add', '-A', '--', '.', ...excludePathspecs], [], 600_000); git( repoPath, - // --no-verify: skip pre-commit/commit-msg hooks. An operator's global - // core.hooksPath or init.templateDir can wire hooks that expect - // project tooling, prompt interactively, or reject the snapshot — - // none of which a headless self-heal commit can satisfy. + // --no-verify only skips pre-commit/commit-msg — prepare-commit-msg + // and (worse, since it runs AFTER the commit object already exists, + // synchronously inside this same git invocation) post-commit are + // NOT covered by it. An operator's global core.hooksPath or + // init.templateDir can wire either, expecting project tooling, + // prompting interactively, or hanging — none of which a headless + // self-heal commit can satisfy, and a hanging post-commit hook would + // burn the 600s budget above without even being the slow step. + // `-c core.hooksPath=/dev/null` (in configs, below) makes git look + // for hook scripts inside a location that can't contain any, + // disabling the entire hooks path for this one invocation — the + // complete form of what --no-verify only partially covers, kept for + // explicitness on the two hooks it does name. [ 'commit', '--quiet', '--allow-empty', '--no-gpg-sign', '--no-verify', '-m', 'gbrain: initial commit (auto-init by sync)', ], - ['user.name=gbrain', 'user.email=gbrain@localhost'], + ['user.name=gbrain', 'user.email=gbrain@localhost', 'core.hooksPath=/dev/null'], ); } @@ -1819,7 +1842,6 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise { - // #2964 (round 7, P1 — Codex caught this with an actual repro run): a - // brain rsync'd from another machine without its `.git` can still - // retain that machine's auto-managed `.gitignore`. collectSyncableFiles - // (inside performFullSync) enumerates via `git ls-files - // --exclude-standard`, so a leftover db_only ignore rule would - // silently omit those pages from THIS first sync's DB import — the - // same data-loss bug class the ordering fix above prevents for a - // .gitignore gbrain itself would have written, just from a - // pre-existing file instead. Neutralize any existing .gitignore for - // the duration of this ONE first-sync call only, byte-for-byte - // restoring it immediately after (even on error) — matching exactly - // what a truly fresh brain with no .gitignore at all already does on - // its first sync (nothing to suppress collection there either). - // db_only content still stays out of the COMMIT independently - // (createSyncBaselineCommit's pathspec exclusion doesn't depend on - // .gitignore); manageGitignore below re-merges the managed block onto - // the restored original content afterward, so any of the user's own - // unrelated ignore rules in that file survive intact. - const gitignorePath = join(roots.gitContextRoot, '.gitignore'); - const savedGitignore = didSelfHeal && existsSync(gitignorePath) - ? readFileSync(gitignorePath, 'utf-8') - : null; - if (savedGitignore !== null) unlinkSync(gitignorePath); - try { - const result = await performFullSync(engine, roots, headCommit, opts); - if (savedGitignore !== null) writeFileSync(gitignorePath, savedGitignore); - if (didSelfHeal && result.status !== 'blocked_by_failures' && result.status !== 'partial') { - // repoPath is `string` by construction (guarded at function - // entry), but the guard's narrowing doesn't carry into this - // nested closure — reassert via roots, realpath-derived from it. - manageGitignore(roots.gitContextRoot, engine.kind); - } - return result; - } catch (err) { - if (savedGitignore !== null && !existsSync(gitignorePath)) writeFileSync(gitignorePath, savedGitignore); - throw err; - } - } + // #2964: self-heal deliberately does NOT special-case db_only/.gitignore + // interaction beyond the COMMIT itself (createSyncBaselineCommit's + // pathspec exclusion, which stands on its own regardless of what + // .gitignore says). db_only content is documented as DB-sourced ("bulk + // machine-generated content... written to disk as a local cache", see + // docs/storage-tiering.md) — it reaches the database via ingest-specific + // paths, never via gbrain sync's git-diff-based file collection, and + // `.gitignore` management there is entirely about keeping db_only out of + // git history, not about what sync imports. An earlier version of this + // fix (Codex review rounds 6-7) tried to also guarantee db_only markdown + // gets imported on this first sync and that .gitignore gets written + // post-success even when called outside runSync — solving a problem + // that, per the docs above, isn't actually in scope for what sync is + // for. Reverted in round 8 review discussion in favor of this simpler + // design: after self-heal, the import + any subsequent .gitignore + // management behave EXACTLY the same as for any other brain, self-healed + // or not (runSync's existing post-success manageGitignoreAtGitRoot call + // covers the CLI path identically either way; the dream cycle not + // calling it is a separate, pre-existing characteristic of the dream + // cycle in general, not something this fix introduces or worsens). // #1970: bookmark reachability. The ONLY thing that should force a full // reconcile is a truly-absent object; a present-but-non-ancestor bookmark @@ -2071,7 +2056,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'); @@ -305,53 +286,4 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', () expect(tracked).not.toContain('private-cache'); }); - test('a self-heal via bare performSync (mirrors cycle.ts:runPhaseSync — no runSync CLI wrapper) still writes .gitignore for db_only dirs (round 6 P2)', async () => { - // The dream cycle calls performSync directly and never runs runSync's - // post-success manageGitignoreAtGitRoot. Without performSyncInner doing - // this itself after a self-heal, a brain that's only ever synced via - // the dream cycle would have db_only content correctly excluded from - // the baseline commit but .gitignore never written — leaving the - // user's own future manual `git add`/`commit` unprotected. - const { performSync } = await import('../src/commands/sync.ts'); - const { mkdirSync } = await import('fs'); - 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'); - - const result = await performSync(engine, { noPull: true, noEmbed: true, full: true }); - - expect(result.status).toBe('first_sync'); - const gitignore = existsSync(join(dir, '.gitignore')) ? readFileSync(join(dir, '.gitignore'), 'utf-8') : ''; - expect(gitignore).toContain('private-cache'); - }); - - test('a leftover .gitignore from before the brain lost its .git does not suppress db_only import (round 7 P1: rsync scenario)', async () => { - // The exact primary motivating scenario: a brain rsync'd from another - // machine keeps its OLD auto-managed .gitignore (already listing - // db_only dirs from before) even though `.git` itself never made the - // trip. Codex's round-7 review caught this with a live repro: without - // neutralizing the pre-existing file for this one first-sync call, - // collectSyncableFiles's `git ls-files --exclude-standard` silently - // omits db_only pages from the DATABASE — same bug class as round 6's - // ordering fix, just triggered by a file gbrain didn't write itself. - 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'); - // A leftover .gitignore with an UNRELATED custom rule too, proving the - // restore preserves the user's own content rather than discarding it. - writeFileSync( - join(dir, '.gitignore'), - '.DS_Store\n\n# Auto-managed by gbrain (db_only directories)\nprivate-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(); - const gitignore = readFileSync(join(dir, '.gitignore'), 'utf-8'); - expect(gitignore).toContain('.DS_Store'); - expect(gitignore).toContain('private-cache'); - }); });