From c17cd23bd3fb879953ab2bb9d755015bb65cb313 Mon Sep 17 00:00:00 2001 From: masashiono0611 Date: Sun, 19 Jul 2026 02:36:18 +0900 Subject: [PATCH] fix(sync): allow default-source ownership, realpath compare, generous timeout, --no-verify (#2964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth Codex review round (9c1d461) found the previous round's ownership gate still didn't match the REAL installed-brain shape, plus 3 more gaps: - P1 (critical): rejecting all non-empty opts.sourceId meant self-heal still never fired on a real brain. Migration sources_table_additive seeds a 'default' source row whose local_path mirrors sync.repo_path on every brain that's run it (virtually all of them), so resolveSourceForDir (dream cycle) and bare `gbrain sync` both resolve sourceId:'default' in practice, never undefined. isAnchorOwnedSyncPath now permits sourceId undefined OR exactly 'default' (gbrain's own bootstrap identity, never something a caller names) and proves ownership by rereading the LIVE anchor for that same identity (sources.default.local_path vs config.sync.repo_path). - P2: compared raw anchor/repoPath strings, so a cosmetic difference (trailing slash, ..) between the stored anchor and dream.ts's path.resolve()-normalized brainDir would defeat the match. Now realpath-compares both sides (fail-closed on ENOENT/dangling). - P2: the shared git() helper's 30s timeout could abort the baseline `git add -A` on a large legacy brain mid-way, after `git init` already created `.git` — leaving an unborn repo every subsequent sync would retry and time out identically forever. Added an optional timeoutMs param (default unchanged at 30s); the baseline add call uses 10min. - P2: the baseline commit could trigger an operator's global core.hooksPath/init.templateDir hooks (pre-commit/commit-msg), breaking headless recovery if those hooks need project tooling or prompt. Added --no-verify. Tests: rewrote the mis-scoped "registered source" test (it used sourceId:'default', which is now correctly permitted) into two — a new regression test proving sourceId='default' + matching local_path heals (the actual production shape), and a corrected non-default-sourceId refusal test. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS --- src/commands/sync.ts | 79 ++++++++++++++++++++++++++-------- test/sync-git-autoinit.test.ts | 79 +++++++++++++++++++++++++--------- 2 files changed, 118 insertions(+), 40 deletions(-) diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 5bd1a05ad..3c057afb4 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -919,10 +919,10 @@ export function buildAutoEmbedArgs(slugs: string[], sourceId?: string): string[] * 100 MiB is generous but still bounded — a 100K-file diff with long * paths tops out around 10–20 MiB in practice. */ -function git(repoPath: string, args: string[], configs: string[] = []): string { +function git(repoPath: string, args: string[], configs: string[] = [], timeoutMs = 30000): string { return execFileSync('git', buildGitInvocation(repoPath, args, configs), { encoding: 'utf-8', - timeout: 30000, + timeout: timeoutMs, maxBuffer: 100 * 1024 * 1024, }).trim(); } @@ -992,10 +992,24 @@ function createSyncBaselineCommit(repoPath: string, engineKind?: 'pglite' | 'pos } }) .map((dir) => `:!${dir}`); - git(repoPath, ['add', '-A', '--', '.', ...excludePathspecs]); + // #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 + // init` already created `.git`, leaving an unborn repo that every + // subsequent sync would retry (and time out identically) forever; + // the unborn-HEAD recovery path exists for OTHER causes of that state, + // not to be this one's normal first outcome. + git(repoPath, ['add', '-A', '--', '.', ...excludePathspecs], [], 600_000); git( repoPath, - ['commit', '--quiet', '--allow-empty', '--no-gpg-sign', '-m', 'gbrain: initial commit (auto-init by sync)'], + // --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. + [ + 'commit', '--quiet', '--allow-empty', '--no-gpg-sign', '--no-verify', + '-m', 'gbrain: initial commit (auto-init by sync)', + ], ['user.name=gbrain', 'user.email=gbrain@localhost'], ); } @@ -1067,20 +1081,39 @@ async function readSyncAnchor( } /** - * #2964: is `repoPath` gbrain's own legacy default anchor, as opposed to a + * #2964: is `repoPath` gbrain's own default-brain 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.sourceId` alone is NOT sufficient — and neither is rejecting + * `opts.sourceId` outright: migration `sources_table_additive` (v20) + * seeds a `'default'` source row whose `local_path` is copied FROM + * `config.sync.repo_path` on every brain that has ever run it (i.e. + * effectively all of them by now), and `writeSyncAnchor` keeps that row's + * `local_path` current on every sync thereafter. So on a real installed + * brain, `resolveSourceForDir` (dream cycle) and the CLI's bare `gbrain + * sync` both resolve `sourceId: 'default'`, NOT `undefined` — rejecting + * all non-empty `sourceId` (an earlier, insufficiently-reviewed version + * of this check) made self-heal never fire on that real path either, + * masked in tests only because a freshly-`initSchema()`'d test brain's + * `'default'` row has a null `local_path` (Codex review round 5). + * + * The actual boundary: `'default'` is gbrain's own bootstrap identity, + * not something a caller names — a DIFFERENT, non-default `sourceId` is + * what an explicit `sources add --path ` registration (a + * user's own external directory) looks like, and that's what must keep + * failing loudly. So: permit `sourceId` when it's exactly `undefined` or + * `'default'`, reject any other id, and for BOTH permitted cases prove + * ownership by VALUE — reread the live anchor for that same identity + * (`sources.default.local_path` when sourceId='default', else + * `config.sync.repo_path`) and require the resolved `repoPath` to + * REALPATH-equal it (not raw string equality: `dream`'s `resolveBrainDir` + * normalizes via `path.resolve`, so a trailing slash or `..` in the + * stored anchor must not defeat the match — Codex review round 5, P2). + * An arbitrary caller-supplied path (e.g. an admin-scope + * `submit_job({name:'sync', data:{repoPath}})`) only passes this check + * if it already equals gbrain's own anchor by realpath identity — at + * which point self-healing it is exactly the legitimate case, not an + * escalation. * * `opts.srcSubpath` disqualifies unconditionally: a subpath-scoped sync * only wants THAT subdirectory captured, but the self-heal baseline @@ -1093,9 +1126,17 @@ async function isAnchorOwnedSyncPath( 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; + if (opts.srcSubpath) return false; + if (opts.sourceId && opts.sourceId !== 'default') return false; + const anchor = await readSyncAnchor(engine, opts.sourceId, 'repo_path'); + if (anchor === null) return false; + try { + return realpathSync(anchor) === realpathSync(repoPath); + } catch { + // Anchor or repoPath doesn't realpath-resolve (dangling/nonexistent) — + // can't prove identity, so don't self-heal. + return false; + } } async function writeSyncAnchor( diff --git a/test/sync-git-autoinit.test.ts b/test/sync-git-autoinit.test.ts index 6bfaf67a6..23e87fcf8 100644 --- a/test/sync-git-autoinit.test.ts +++ b/test/sync-git-autoinit.test.ts @@ -1,29 +1,40 @@ /** * #2964 — sync phase self-heals a never-git-initialized default brain dir. * - * 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). + * A legacy `sync.repo_path`-anchored default brain 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 in a way this brain shape doesn't hit). * * 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. 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 + * realpath-equal gbrain's own anchor — not by whether + * `opts.repoPath`/`opts.sourceId` happen to be set: + * + * - Gating on `!opts.repoPath` (round 3) would have made self-heal never + * fire on `runPhaseSync` (dream cycle), which always resolves the + * anchor itself and passes it through explicitly as `opts.repoPath`. + * - Gating on `!opts.sourceId` (round 4) would ALSO never fire in + * practice: migration `sources_table_additive` seeds a `'default'` + * source row whose `local_path` mirrors `sync.repo_path` on every + * brain that's run it (i.e. virtually all installed brains today), so + * both the dream cycle and bare `gbrain sync` resolve + * `sourceId: 'default'`, never `undefined`, in reality — a fresh test + * brain's null `local_path` masked this (Codex review round 5). + * + * The actual boundary implemented by `isAnchorOwnedSyncPath`: `sourceId` + * must be `undefined` OR exactly `'default'` (gbrain's own bootstrap + * identity — a DIFFERENT id is what an explicit `sources add --path + * ` registration of a user's own external directory looks like), + * AND the resolved `repoPath` must realpath-equal the LIVE anchor for + * that same identity. A caller-supplied path that does not match (a + * registered non-default 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. */ @@ -111,12 +122,38 @@ describe('#2964: sync auto-inits a never-git-initialized default brain dir', () expect(second.modified).toBe(0); }); - test('a registered local source (sourceId set, no remote_url) on a non-git dir still throws — not auto-inited', async () => { + test("sourceId='default' whose local_path mirrors the anchor still auto-inits (P1: the real installed-brain shape)", async () => { + // Migration sources_table_additive seeds a 'default' source row with + // local_path copied from sync.repo_path on every brain that's run it + // — i.e. this, not a bare no-sourceId call, is what runPhaseSync/CLI + // `gbrain sync` actually resolve to on a real installed brain. + await engine.executeRaw(`UPDATE sources SET local_path = $1 WHERE id = 'default'`, [dir]); + const { performSync } = await import('../src/commands/sync.ts'); + expect(existsSync(join(dir, '.git'))).toBe(false); + + const result = await performSync(engine, { + repoPath: dir, + sourceId: 'default', + 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 registered non-default local source (sourceId != default, no remote_url) on a non-git dir still throws — not auto-inited', async () => { + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config) VALUES ('mysource', 'mysource', $1, '{}'::jsonb)`, + [dir], + ); const { performSync } = await import('../src/commands/sync.ts'); await expect( performSync(engine, { repoPath: dir, - sourceId: 'default', + sourceId: 'mysource', noPull: true, noEmbed: true, full: true,