fix(sync,import): global sync.* anchors only move for the brain repo they describe (#2114) (#3735)

Wave-assembled from PR #3735 by @smdesai27.

Co-Authored-By: smdesai27 <sanilmdesai@gmail.com>
This commit is contained in:
Garry Tan
2026-08-12 14:38:36 -07:00
committed by Sina Matian
co-authored by smdesai27
parent cb07cfda8d
commit 636628fdb2
4 changed files with 445 additions and 19 deletions
+31 -10
View File
@@ -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 };
+29 -7
View File
@@ -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<void> {
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<Sy
// (#1794): advance to the PINNED target, and clear any checkpoint (a resume
// whose remaining range turned out to have no syncable changes still
// completes cleanly here).
await writeSyncAnchor(engine, opts.sourceId, 'last_commit', pin, commitTimeMs(gitContextRoot, pin));
await writeSyncAnchor(engine, opts.sourceId, 'last_commit', pin, commitTimeMs(gitContextRoot, pin), gitContextRoot);
await engine.setConfig('sync.last_run', new Date().toISOString());
await writeChunkerVersion(engine, opts.sourceId, String(CHUNKER_VERSION));
await clearOpCheckpoint(engine, ckpt.paths);
@@ -3401,7 +3423,7 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
// "fresh". The checkpoint rows clear here — CONVERGENCE CONTRACT: sync
// convergence == IMPORT convergence; downstream extract/facts/embed is
// decoupled (its own resumable stale sweeps).
await writeSyncAnchor(engine, opts.sourceId, 'last_commit', pin, commitTimeMs(gitContextRoot, pin));
await writeSyncAnchor(engine, opts.sourceId, 'last_commit', pin, commitTimeMs(gitContextRoot, pin), 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));
@@ -3752,7 +3774,7 @@ async function performFullSync(
const advanceFull = async (): Promise<void> => {
// 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));
+82 -2
View File
@@ -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<GlobalAnchorOwnership> {
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 };
}
+303
View File
@@ -0,0 +1,303 @@
/**
* #2114 — `gbrain import <dir>` (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<Awaited<ReturnType<typeof runImport>>> {
let result!: Awaited<ReturnType<typeof runImport>>;
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<void>): Promise<string> {
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<ReturnType<typeof runImport>>;
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);
});
});