diff --git a/src/commands/import.ts b/src/commands/import.ts index bb08b7a93..e4fe8dc53 100644 --- a/src/commands/import.ts +++ b/src/commands/import.ts @@ -545,8 +545,9 @@ export async function runImport( // Import → sync continuity: write sync checkpoint if this is a git repo. // Bug 9 — gate last_commit on "no failures" so import doesn't silently - // stomp on the sync bookmark when parsing broke. We still write - // last_run + repo_path either way (those are progress indicators). + // stomp on the sync bookmark when parsing broke. last_run + repo_path are + // written alongside it, but ONLY when this import owns the globals (#2114 + // guard below) — a foreign directory must not repoint the brain repo. let gitHead: string | null = null; try { if (existsSync(join(dir, '.git'))) { @@ -568,17 +569,37 @@ export async function runImport( const { recordFailures } = await import('../core/sync.ts'); recordFailures(opts.sourceId ?? 'default', failures, gitHead); } - if (failures.length === 0) { - await engine.setConfig('sync.last_commit', gitHead); - } else { + + // #2114 guard: the global sync.* keys describe THE brain repo (the + // default source's working tree). Pre-fix this block rewrote them on + // every git-repo import, silently repointing put_page write-through + // and poisoning the incremental sync anchor. Ownership + the bootstrap + // rule live in ownsGlobalSyncAnchor (shared with writeSyncAnchor's + // legacy branch in sync.ts, so the two layers cannot drift). + const { ownsGlobalSyncAnchor } = await import('../core/sync.ts'); + const { owns, configured } = await ownsGlobalSyncAnchor(engine, sourceId, dir); + + if (owns) { + if (failures.length === 0) { + await engine.setConfig('sync.last_commit', gitHead); + } else { + console.error( + `\nImport completed with ${failures.length} failure(s). ` + + `sync.last_commit NOT advanced — re-run 'gbrain sync' to retry, or ` + + `'gbrain sync --skip-failed' to acknowledge and move past them.`, + ); + } + await engine.setConfig('sync.last_run', new Date().toISOString()); + await engine.setConfig('sync.repo_path', dir); + } else if ((sourceId ?? 'default') === 'default') { console.error( - `\nImport completed with ${failures.length} failure(s). ` + - `sync.last_commit NOT advanced — re-run 'gbrain sync' to retry, or ` + - `'gbrain sync --skip-failed' to acknowledge and move past them.`, + `\n[import] sync.repo_path stays at ${configured ?? '(unset)'} — NOT repointing to "${dir}". ` + + `Sync bookmarks were not advanced. If this directory IS your brain repo, run: ` + + `gbrain config set sync.repo_path "${dir}"`, ); } - await engine.setConfig('sync.last_run', new Date().toISOString()); - await engine.setConfig('sync.repo_path', dir); + // Non-default sources: deliberately silent no-op — the globals are not + // this import's to move (its sync anchors live on the `sources` row). } return { imported, skipped, errors, chunksCreated, failures }; diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 16de5133b..09cd57910 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -19,6 +19,7 @@ import { isSkippablePath, resolveAutoSkipThreshold, DEFAULT_SOURCE_ID, + ownsGlobalSyncAnchor, } from '../core/sync.ts'; import { computeSyncDelta, @@ -1311,7 +1312,7 @@ async function isAnchorOwnedSyncPath( } } -async function writeSyncAnchor( +export async function writeSyncAnchor( engine: BrainEngine, sourceId: string | undefined, which: 'repo_path' | 'last_commit', @@ -1323,6 +1324,11 @@ async function writeSyncAnchor( // git-intrinsic committer time of the HEAD we just synced). `undefined` keeps // the legacy 2-column write; `null` clears the column (git unavailable). newestContentEpochMs?: number | null, + // #2114: the repo dir this anchor write is FOR. Required to guard the + // legacy branch's `last_commit` writes (where `value` is a hash, not a + // dir). `repo_path` writes self-describe via `value`. Callers that omit + // it on a legacy-path last_commit write keep pre-#2114 behavior. + repoDir?: string, ): Promise { if (sourceId) { const col = which === 'repo_path' ? 'local_path' : 'last_commit'; @@ -1350,9 +1356,25 @@ async function writeSyncAnchor( } return; } - // Legacy no-sourceId path (pre-v0.18 global config). Modern sync always - // resolves a sourceId (incl. 'default'), so newest_content_at is written via - // the sourceId branch above; the default source is not stuck on NULL. + // Legacy no-sourceId path (pre-v0.18 global config; also reached when a + // caller could not resolve a source for the dir — dream --dir on an + // unregistered directory, minion sync with an unmatched repoPath). #2114: + // these globals describe THE brain repo, and this branch used to write + // them unconditionally — a full-sync fallback against a foreign directory + // silently repointed put_page write-through and poisoned the incremental + // anchor. Refuse to move them for a directory that isn't the brain repo. + const anchorDir = which === 'repo_path' ? value : repoDir; + if (anchorDir !== undefined) { + const { owns, configured } = await ownsGlobalSyncAnchor(engine, undefined, anchorDir); + if (!owns) { + serr( + `[sync] sync.${which} stays at ${configured ?? '(unset)'} — not moving the ` + + `global anchor for "${anchorDir}". To make that directory the brain repo: ` + + `gbrain config set sync.repo_path "${anchorDir}"`, + ); + return; + } + } await engine.setConfig(`sync.${which}`, value); } @@ -2493,7 +2515,7 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise => { // Persist sync state so the next sync is incremental. Routed through // writeSyncAnchor so --source pins the right sources row. - await writeSyncAnchor(engine, opts.sourceId, 'last_commit', headCommit, newestCommitMs(gitContextRoot)); + await writeSyncAnchor(engine, opts.sourceId, 'last_commit', headCommit, newestCommitMs(gitContextRoot), gitContextRoot); await engine.setConfig('sync.last_run', new Date().toISOString()); await writeSyncAnchor(engine, opts.sourceId, 'repo_path', anchorPath); await writeChunkerVersion(engine, opts.sourceId, String(CHUNKER_VERSION)); diff --git a/src/core/sync.ts b/src/core/sync.ts index 1dea00c56..9519c7660 100644 --- a/src/core/sync.ts +++ b/src/core/sync.ts @@ -15,8 +15,9 @@ import { SLUG_WORD_CHARS } from './cjk.ts'; // v0.37.7.0 #1169 submodule-detection helpers. Bottom-of-file already // aliases existsSync as `_existsSync` for other purposes; the top-of-file // import keeps the pruneDir helper's deps near its callsite. -import { existsSync, statSync } from 'fs'; -import { join as pathJoin } from 'path'; +import { existsSync, statSync, realpathSync } from 'fs'; +import { join as pathJoin, resolve as pathResolve } from 'path'; +import type { BrainEngine } from './engine.ts'; export interface SyncManifest { added: string[]; @@ -568,3 +569,82 @@ export type { SyncGateInput, SyncGateOutcome, } from './sync-failure-ledger.ts'; + +/** + * #2114 — same-directory check for the global sync-anchor ownership guard. + * Best-effort realpath so symlinked spellings (macOS `/tmp` vs `/private/tmp`) + * and relative invocations compare as the same repo. `realpathSync.native` + * first: unlike the JS implementation it canonicalizes path CASE on + * case-insensitive filesystems (APFS `/users/x` vs `/Users/x`), so a + * case-variant spelling cannot false-refuse the user's own brain repo. + */ +export function sameRepoDir(a: string, b: string): boolean { + const norm = (p: string): string => { + const abs = pathResolve(p); + try { + return realpathSync.native(abs); + } catch { + // fall through + } + try { + return realpathSync(abs); + } catch { + return abs; + } + }; + return norm(a) === norm(b); +} + +export interface GlobalAnchorOwnership { + owns: boolean; + /** The path ownership was judged against (global key first, else the + * default source's local_path), or null on a truly fresh brain. */ + configured: string | null; +} + +/** + * #2114 — may an import/sync of `dir` move the GLOBAL sync anchors + * (`sync.repo_path` / `sync.last_commit`)? + * + * The globals describe THE brain repo — the default source's working tree + * (source-resolver.ts documents `sync.repo_path` as the legacy pre-v0.18 + * default-source key; migration v16 seeds the `default` source row FROM it). + * Ownership therefore requires BOTH: + * 1. the operation resolves to the default source (or the legacy + * no-sourceId path, which is default-by-definition), AND + * 2. `dir` matches the brain repo's configured identity — the global key + * when set, else the default source row's `local_path` when set + * (modern sync writes its anchor THERE, leaving the global unset, so + * an unset global alone must not green-light a bootstrap). + * Only when neither identity exists is this a fresh brain, and bootstrap + * is allowed. + */ +export async function ownsGlobalSyncAnchor( + engine: BrainEngine, + sourceId: string | undefined, + dir: string, +): Promise { + if (sourceId && sourceId !== 'default') { + return { owns: false, configured: null }; + } + const globalPath = await engine.getConfig('sync.repo_path'); + if (globalPath) { + return { owns: sameRepoDir(globalPath, dir), configured: globalPath }; + } + // Global unset — the brain identity may still live on the default source + // row (modern sync writes anchors there). Best-effort: a query failure + // falls through to bootstrap, preserving pre-#2114 behavior on exotic + // engines rather than refusing writes. + try { + const rows = await engine.executeRaw<{ local_path: string | null }>( + `SELECT local_path FROM sources WHERE id = 'default'`, + ); + const defaultLocal = rows[0]?.local_path ?? null; + if (defaultLocal) { + return { owns: sameRepoDir(defaultLocal, dir), configured: defaultLocal }; + } + } catch { + // sources table unavailable (pre-v0.18 brain mid-upgrade) — treat as fresh. + } + return { owns: true, configured: null }; +} diff --git a/test/import-repo-path-guard.test.ts b/test/import-repo-path-guard.test.ts new file mode 100644 index 000000000..d6a61305d --- /dev/null +++ b/test/import-repo-path-guard.test.ts @@ -0,0 +1,303 @@ +/** + * #2114 — `gbrain import ` (and the sync layer's legacy anchor path) + * must not silently repoint the global brain repo. + * + * Pre-fix, the sync-checkpoint block at the end of `runImport` wrote + * `sync.repo_path` and `sync.last_run` unconditionally (and `sync.last_commit` + * when the import had no failures) whenever the imported directory was a git + * repo — and `writeSyncAnchor`'s legacy no-sourceId branch did the same for + * sync-driven full-reimport fallbacks. Importing ANY other directory silently + * repointed `put_page` write-through at that directory (write-through.ts falls + * back to `sync.repo_path` when the source row has no local_path) and + * poisoned the incremental sync anchor. Nothing logged the change. + * + * The guard (shared `ownsGlobalSyncAnchor` in core/sync.ts, used by BOTH + * layers): only the default source may move the globals, and only for the + * configured brain repo — the global key when set, else the default source + * row's local_path when set. Only a truly fresh brain bootstraps. + * + * Hermetic: PGLite in-memory; `GBRAIN_HOME` overridden via `withEnv` so + * runImport's checkpoint file NEVER touches the real `~/.gbrain` (the + * pattern documented in test/import-resume.test.ts); `GBRAIN_SOURCE` + * cleared so a dev-shell source override can't reroute resolution; git + * fixtures run with global/system config disabled so user hooks and + * templates can't fire. + */ + +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { mkdtempSync, writeFileSync, rmSync, realpathSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { execFileSync } from 'child_process'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { runImport } from '../src/commands/import.ts'; +import { writeSyncAnchor } from '../src/commands/sync.ts'; +import { withEnv } from './helpers/with-env.ts'; + +let engine: PGLiteEngine; +let workspace: string; // GBRAIN_HOME target — keeps the import checkpoint out of ~/.gbrain + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); + workspace = mkdtempSync(join(tmpdir(), 'gbrain-2114-home-')); +}); + +afterAll(async () => { + await engine.disconnect(); + rmSync(workspace, { recursive: true, force: true }); +}); + +/** runImport with the hermetic env: isolated GBRAIN_HOME, no source override. */ +async function run(args: string[]): Promise>> { + let result!: Awaited>; + await withEnv({ GBRAIN_HOME: workspace, GBRAIN_SOURCE: undefined }, async () => { + result = await runImport(engine, args); + }); + return result; +} + +/** git isolated from the user's global/system config (hooks, templates, signing). */ +const GIT_ENV = { + ...process.env, + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_SYSTEM: '/dev/null', + GIT_AUTHOR_NAME: 'test', + GIT_AUTHOR_EMAIL: 'test@example.com', + GIT_COMMITTER_NAME: 'test', + GIT_COMMITTER_EMAIL: 'test@example.com', +}; + +function git(dir: string, ...args: string[]): void { + execFileSync('git', ['-C', dir, ...args], { encoding: 'utf-8', env: GIT_ENV }); +} + +/** + * Throwaway git repo with one committed markdown page. The page filename is + * derived from the prefix so two fixture repos never collide on slug when + * imported into different sources of the same brain. + */ +function makeGitRepo(prefix: string): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + writeFileSync( + join(dir, `${prefix}note.md`), + `---\ntype: note\n---\n# Note ${prefix}\n\nContent of ${prefix}.`, + ); + git(dir, 'init', '-q'); + git(dir, 'add', '-A'); + git(dir, 'commit', '-qm', 'init'); + // runImport canonicalizes the target dir (resolveImportTargetDir), so + // return the realpath here to make equality assertions exact. + return realpathSync(dir); +} + +function headOf(dir: string): string { + return execFileSync('git', ['-C', dir, 'rev-parse', 'HEAD'], { + encoding: 'utf-8', + env: GIT_ENV, + }).trim(); +} + +function commitNewFile(dir: string, name: string): void { + writeFileSync(join(dir, name), `---\ntype: note\n---\n# ${name}\n\nMore content.`); + git(dir, 'add', '-A'); + git(dir, 'commit', '-qm', `add ${name}`); +} + +/** + * Capture everything written to stderr while `fn` runs. Bun's console.error + * does NOT route through process.stderr.write, so two layers are patched: + * globalThis.console.error (catches the guard notices fired from src modules) + * and process.stderr.write (catches the progress reporter). Extends the + * stream-patch pattern from test/sync-sole-non-default-routing.test.ts with + * the console layer bun requires. + */ +async function captureStderr(fn: () => Promise): Promise { + const captured: string[] = []; + const origWrite = process.stderr.write.bind(process.stderr); + const origError = globalThis.console.error; + (process.stderr as unknown as { write: typeof origWrite }).write = (( + chunk: string | Uint8Array, + ): boolean => { + captured.push(typeof chunk === 'string' ? chunk : new TextDecoder().decode(chunk)); + return true; + }) as typeof origWrite; + globalThis.console.error = (...args: unknown[]): void => { + captured.push(args.map(String).join(' ') + '\n'); + }; + try { + await fn(); + } finally { + (process.stderr as unknown as { write: typeof origWrite }).write = origWrite; + globalThis.console.error = origError; + } + return captured.join(''); +} + +describe('import sync-bookmark guard (#2114)', () => { + let repoA: string; + let repoB: string; + const cleanups: string[] = []; + + beforeEach(async () => { + // Reset the global sync bookmarks and imported pages between cases. + await (engine as any).db.exec(`DELETE FROM config WHERE key LIKE 'sync.%'`); + for (const t of ['content_chunks', 'links', 'tags', 'raw_data', 'page_versions', 'ingest_log', 'pages']) { + await (engine as any).db.exec(`DELETE FROM ${t}`); + } + await (engine as any).db.exec(`DELETE FROM sources WHERE id <> 'default'`); + await (engine as any).db.exec(`UPDATE sources SET local_path = NULL WHERE id = 'default'`); + repoA = makeGitRepo('gbrain-2114-a-'); + repoB = makeGitRepo('gbrain-2114-b-'); + cleanups.push(repoA, repoB); + }); + + afterAll(() => { + for (const d of cleanups) rmSync(d, { recursive: true, force: true }); + }); + + test('first import bootstraps sync.repo_path (fresh-brain flow unchanged)', async () => { + expect(await engine.getConfig('sync.repo_path')).toBeFalsy(); + + const result = await run([repoA, '--no-embed', '--json']); + expect(result.imported).toBeGreaterThanOrEqual(1); + + expect(await engine.getConfig('sync.repo_path')).toBe(repoA); + expect(await engine.getConfig('sync.last_commit')).toBe(headOf(repoA)); + expect(await engine.getConfig('sync.last_run')).toBeTruthy(); + }); + + test('importing a DIFFERENT git repo does not clobber the configured brain repo', async () => { + await run([repoA, '--no-embed', '--json']); + const anchorBefore = await engine.getConfig('sync.last_commit'); + expect(anchorBefore).toBe(headOf(repoA)); // premise: clean first import took the anchor + const lastRunBefore = await engine.getConfig('sync.last_run'); + + let result!: Awaited>; + const notices = await captureStderr(async () => { + result = await run([repoB, '--no-embed', '--json']); + }); + // The import itself still succeeds — only the bookmark writes are guarded. + expect(result.imported).toBeGreaterThanOrEqual(1); + + // The #2114 clobber: pre-fix, repo_path + last_run moved to repoB + // unconditionally (and last_commit with them on a clean import). + expect(await engine.getConfig('sync.repo_path')).toBe(repoA); + expect(await engine.getConfig('sync.last_commit')).toBe(anchorBefore); + expect(await engine.getConfig('sync.last_run')).toBe(lastRunBefore); + + // The refusal is loud, and names the intentional-repoint command. + expect(notices).toContain('NOT repointing'); + expect(notices).toContain('gbrain config set sync.repo_path'); + }); + + test('re-importing the SAME repo still advances the bookmark', async () => { + await run([repoA, '--no-embed', '--json']); + const firstHead = headOf(repoA); + expect(await engine.getConfig('sync.last_commit')).toBe(firstHead); + + commitNewFile(repoA, 'second.md'); + const secondHead = headOf(repoA); + expect(secondHead).not.toBe(firstHead); + + await run([repoA, '--no-embed', '--json']); + expect(await engine.getConfig('sync.last_commit')).toBe(secondHead); + expect(await engine.getConfig('sync.repo_path')).toBe(repoA); + }); + + test('non-canonical CONFIGURED spelling still counts as the same repo (realpath compare)', async () => { + // The import target is canonicalized by resolveImportTargetDir before the + // guard runs, but the CONFIGURED value can be any spelling the user gave + // `gbrain config set sync.repo_path` (macOS: /var/... vs /private/var/..., + // or a symlink). The guard must canonicalize BOTH sides — a plain + // string/resolve() compare would false-refuse the user's own brain repo. + const altA = repoA.startsWith('/private/') ? repoA.slice('/private'.length) : repoA; + if (altA !== repoA) { + expect(realpathSync(altA)).toBe(repoA); // sanity: same dir, different spelling + } + await engine.setConfig('sync.repo_path', altA); + await engine.setConfig('sync.last_commit', 'stale-anchor'); + + const notices = await captureStderr(async () => { + await run([repoA, '--no-embed', '--json']); + }); + + // Same repo → no refusal, bookmark advances past the stale anchor. + expect(notices).not.toContain('NOT repointing'); + expect(await engine.getConfig('sync.last_commit')).toBe(headOf(repoA)); + }); + + test('unset global does NOT green-light bootstrap when the default source row holds the brain repo', async () => { + // Modern sync writes the default source's anchor to sources.local_path and + // leaves the global unset. An unset global alone must not let a foreign + // import claim the brain-repo identity (#2114 false-accept). + await (engine as any).db.exec( + `UPDATE sources SET local_path = '${repoA}' WHERE id = 'default'`, + ); + expect(await engine.getConfig('sync.repo_path')).toBeFalsy(); + + const notices = await captureStderr(async () => { + await run([repoB, '--no-embed', '--json']); + }); + expect(notices).toContain('NOT repointing'); + expect(await engine.getConfig('sync.repo_path')).toBeFalsy(); + expect(await engine.getConfig('sync.last_commit')).toBeFalsy(); + + // The REAL brain repo may align the global with the source row. + await run([repoA, '--no-embed', '--json']); + expect(await engine.getConfig('sync.repo_path')).toBe(repoA); + }); + + test('non-default source import never touches the global sync.* keys', async () => { + await engine.executeRaw( + `INSERT INTO sources (id, name) VALUES ('work-code', 'work-code') ON CONFLICT DO NOTHING`, + ); + + // Case 1: globals unset — a non-default import must NOT bootstrap them + // (the globals describe the default source's repo, not this one). + await run([repoB, '--source-id', 'work-code', '--no-embed', '--json']); + expect(await engine.getConfig('sync.repo_path')).toBeFalsy(); + expect(await engine.getConfig('sync.last_commit')).toBeFalsy(); + + // Case 2: globals configured — a non-default import must leave them alone. + await run([repoA, '--no-embed', '--json']); + expect(await engine.getConfig('sync.repo_path')).toBe(repoA); + const anchorBefore = await engine.getConfig('sync.last_commit'); + expect(anchorBefore).toBe(headOf(repoA)); // premise: clean default import took the anchor + + commitNewFile(repoB, 'more.md'); + await run([repoB, '--source-id', 'work-code', '--no-embed', '--json']); + + expect(await engine.getConfig('sync.repo_path')).toBe(repoA); + expect(await engine.getConfig('sync.last_commit')).toBe(anchorBefore); + }); + + test('writeSyncAnchor legacy (no-sourceId) branch refuses foreign dirs too', async () => { + // The original #2114 incident came through the sync layer: a full-reimport + // fallback against a staging dir hit writeSyncAnchor's legacy branch and + // clobbered the globals. The same ownership guard must hold there. + await engine.setConfig('sync.repo_path', repoA); + await engine.setConfig('sync.last_commit', 'anchor-a'); + + const notices = await captureStderr(async () => { + // Foreign dir: both writes must be refused. + await writeSyncAnchor(engine, undefined, 'repo_path', repoB); + await writeSyncAnchor(engine, undefined, 'last_commit', 'anchor-b', undefined, repoB); + }); + expect(await engine.getConfig('sync.repo_path')).toBe(repoA); + expect(await engine.getConfig('sync.last_commit')).toBe('anchor-a'); + expect(notices).toContain('not moving the global anchor'); + + // The configured repo itself still advances (repoDir threaded for last_commit). + await writeSyncAnchor(engine, undefined, 'last_commit', 'anchor-a2', undefined, repoA); + expect(await engine.getConfig('sync.last_commit')).toBe('anchor-a2'); + + // A resolved sourceId keeps writing to its own sources row, untouched by the guard. + await writeSyncAnchor(engine, 'default', 'repo_path', repoA); + const rows = await engine.executeRaw<{ local_path: string | null }>( + `SELECT local_path FROM sources WHERE id = 'default'`, + ); + expect(rows[0]?.local_path).toBe(repoA); + }); +});