From 9c1d46114691d02c7c7600956f4cd67c73234f1c Mon Sep 17 00:00:00 2001 From: masashiono0611 Date: Sun, 19 Jul 2026 02:24:18 +0900 Subject: [PATCH] fix(sync): prove self-heal ownership by anchor VALUE, not field presence (#2964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS --- src/commands/sync.ts | 117 ++++++++++++++++++++++++--------- test/sync-git-autoinit.test.ts | 117 ++++++++++++++++++++++++--------- 2 files changed, 173 insertions(+), 61 deletions(-) diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 60a3a2dc6..5bd1a05ad 100644 --- a/src/commands/sync.ts +++ b/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 { + 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 { + // 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'); + }); });