fix(sync): failed git pull with zero imports reports partial (pull_failed) instead of up_to_date (#3068) (#3253)

* fix(sync): report partial (pull_failed) instead of up_to_date when git pull fails with zero imports (#3068)

A warn-and-continue internal git pull failure (e.g. a local-path origin
rejected by protocol.file.allow=never) combined with a zero-import run
previously reported `up_to_date`, exited 0, and bumped the last_sync_at
freshness heartbeat. A permanently-failing pull was therefore invisible
forever: doctor's sync_freshness never fired and every scheduled sync
looked clean while the source silently went stale.

Now, when the pull failed and the run imported nothing, sync returns
`partial` with the new reason `pull_failed`, leaves last_commit AND
last_sync_at untouched (so staleness monitoring fires), and prints a
dedicated non-success message. The fall-through-to-working-tree design
is unchanged: local commits still import when the remote is unreachable,
and the anchor still advances over commits that were actually imported.

Addresses the report in #3068.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sync): surface pull_failed to CLI exit codes, sync --all JSON, and the cycle phase (#3068 review round)

Codex review round 1 follow-ups:

- Single-source `gbrain sync` sets exit code 1 on partial/pull_failed
  (timeout-class partials keep exit 0 — they converge on retry; a failing
  pull does not).
- `sync --all` exits 1 when any source reports pull_failed, and the
  --json envelope carries the per-source partial `reason`.
- The autopilot cycle's sync phase maps partial/pull_failed to `warn`
  with a dedicated summary and a `syncReason` detail, so a scheduled
  cycle no longer reports a clean run over a wedged source.
- The regression test now isolates GBRAIN_HOME to a temp dir so the
  first full sync cannot touch the real sync-failure ledger.
- Current-state docs: KEY_FILES.md sync.ts entry + TESTING.md inventory
  describe the pull_failed contract and the new test.

Addresses the report in #3068.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(sync): route pull_failed exit through the owned verdict channel; make the regression test serial (#3068 review round 2)

Codex review round 2 follow-ups:

- Single-source exit now uses setCliExitVerdict(1) instead of a raw
  process.exitCode assignment, which the CLI teardown deliberately
  ignores (PGLite's Emscripten runtime clobbers process.exitCode
  mid-run; the owned channel in src/core/cli-force-exit.ts is the only
  trusted verdict). Pinned by test/cli-exit-verdict-pin.test.ts.
- The regression test is renamed to *.serial.test.ts because it pins
  GBRAIN_HOME for the whole file (scripts/check-test-isolation.sh R1);
  docs updated to the new name.

Addresses the report in #3068.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

docs/TESTING.md

@@ -189,6 +189,7 @@ Unit tests and what they cover:
 - `test/orphans.test.ts` — orphans command: detection, pseudo filtering, text/json/count outputs, MCP op.
 - `test/postgres-engine.test.ts` — `statement_timeout` scoping: `sql.begin` + `SET LOCAL` shape, source-level grep guardrail against a reintroduced bare `SET statement_timeout`.
 - `test/sync.test.ts` — sync logic + regression guard asserting top-level `engine.transaction` is not called.
+- `test/sync-pull-failed-anchor.serial.test.ts` — #3068 regression: a failed internal `git pull` (local-path origin vs `protocol.file.allow=never`) with zero imports returns `partial`/`pull_failed` (not `up_to_date`), freezes `last_commit` + `last_sync_at`, recovers after a manual pull; fall-through import of local commits preserved. Serial: pins `GBRAIN_HOME` to a temp dir for the whole file.
 - `test/sync-concurrency.test.ts` — `autoConcurrency()` thresholds + PGLite-forces-serial + explicit-override clamping; `shouldRunParallel()` explicit-bypasses-floor contract; `parseWorkers()` validation rejecting `'0'`/`'-3'`/`'foo'`/`'1.5'`/trailing chars.
 - `test/sync-parallel.test.ts` — PGLite-routed coverage of the bookmark gate under concurrency, head-drift gate, vanished-file failure capture, PGLite-stays-serial, and the `gbrain-sync` writer-lock contract.
 - `test/sync-failures.test.ts` — `classifyErrorCode` regex coverage for all 12 codes against literal production message strings from `markdown.ts` and `import-file.ts`; `summarizeFailuresByCode` sort + pre-classified-honor; `recordSyncFailures` code-field persistence; `acknowledgeSyncFailures` `AcknowledgeResult` shape + backfill on legacy entries.

src/commands/sync.ts

@@ -213,7 +213,7 @@ export interface SyncResult {
    * cron operators can disambiguate timeout vs pull-timeout in monitoring.
    */
   filesImported?: number;
-  reason?: 'timeout' | 'pull_timeout' | 'stall_timeout' | 'checkpoint_unavailable';
+  reason?: 'timeout' | 'pull_timeout' | 'pull_failed' | 'stall_timeout' | 'checkpoint_unavailable';
   /**
    * v0.42.x (#1794): cumulative file paths durably banked to the checkpoint
    * across THIS run + prior resumed runs. Surfaced on every partial/blocked
@@ -1761,7 +1761,7 @@ function buildPartialResult(opts: {
   modified: number;
   deleted: number;
   renamed: number;
-  reason: 'timeout' | 'pull_timeout' | 'stall_timeout' | 'checkpoint_unavailable';
+  reason: 'timeout' | 'pull_timeout' | 'pull_failed' | 'stall_timeout' | 'checkpoint_unavailable';
   bankedFiles?: number;
 }): SyncResult {
   return {
@@ -1992,6 +1992,15 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
     });
   }
 
+  // #3068: remember a warn-and-continue pull failure. The fall-through-to-
+  // working-tree design stays (local commits still import when the remote is
+  // unreachable), but a ZERO-import sync after a failed pull must not report
+  // `up_to_date` / bump the freshness heartbeat — that is what made a
+  // permanently-failing pull (e.g. a local-path origin rejected by
+  // protocol.file.allow=never, #1315) invisible forever: every nightly run
+  // exited 0 with "Already up to date" and doctor's sync_freshness never
+  // fired because last_sync_at kept advancing.
+  let pullFailed = false;
   if (!opts.noPull && !detachedHead && originRemotePresent) {
     const _t0 = Date.now();
     serr(`[gbrain phase] sync.git_pull start`);
@@ -2034,6 +2043,7 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
           reason: 'pull_timeout',
         });
       }
+      pullFailed = true;
       if (msg.includes('non-fast-forward') || msg.includes('diverged')) {
         serr(`Warning: git pull failed (remote diverged). Syncing from local state.`);
       } else {
@@ -2208,6 +2218,29 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
       detachedWorkingTreeManifest.renamed.length > 0);
 
   if (lastCommit === headCommit && !versionMismatch && !versionNeverSet && !hasDetachedWorkingTreeChanges) {
+    // #3068: the pull failed and nothing local advanced — this run imported
+    // NOTHING and the remote may hold commits we could not fetch. Reporting
+    // `up_to_date` here (and bumping the heartbeat below) is exactly the
+    // silent-wedge from the issue: every scheduled sync exits 0 forever while
+    // the source is stale. Return `partial` instead (not a clean status, and
+    // last_sync_at stays frozen so doctor/sources-status staleness fires).
+    // The anchor is untouched; the next sync retries the pull from the same
+    // bookmark.
+    if (pullFailed) {
+      serr(
+        `[sync] git pull failed and no local changes imported — reporting partial ` +
+        `(not up_to_date); sync anchor unchanged at ${lastCommit.slice(0, 8)}.`,
+      );
+      return buildPartialResult({
+        fromCommit: lastCommit,
+        toCommit: lastCommit,
+        filesImported: 0,
+        pagesAffected: [],
+        chunksCreated: 0,
+        added: 0, modified: 0, deleted: 0, renamed: 0,
+        reason: 'pull_failed',
+      });
+    }
     // v0.42.52.0 (PR #22xx): bump last_sync_at as a heartbeat on every successful
     // 0-changes sync. D4 invariant ("never advance last_commit on partial") is
     // preserved: last_sync_at is a monitoring signal (doctor sync_freshness
@@ -2392,6 +2425,27 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise<Sy
   }
 
   if (totalChanges === 0) {
+    // #3068: same guard as the git-HEAD-equality gate above — a failed pull
+    // plus zero imports must not produce a clean `up_to_date` (and must not
+    // advance the anchor past commits this run never looked at remotely).
+    // Reached when local-only commits landed with no syncable content while
+    // the pull kept failing. Nothing is written; the next sync re-diffs the
+    // same trivial range and retries the pull.
+    if (pullFailed) {
+      serr(
+        `[sync] git pull failed and no syncable changes imported — reporting partial ` +
+        `(not up_to_date); sync anchor unchanged at ${lastCommit.slice(0, 8)}.`,
+      );
+      return buildPartialResult({
+        fromCommit: lastCommit,
+        toCommit: lastCommit,
+        filesImported: 0,
+        pagesAffected: [],
+        chunksCreated: 0,
+        added: 0, modified: 0, deleted: 0, renamed: 0,
+        reason: 'pull_failed',
+      });
+    }
     // Update sync state even with no syncable changes (git advanced). v0.42.x
     // (#1794): advance to the PINNED target, and clear any checkpoint (a resume
     // whose remaining range turned out to have no syncable changes still
@@ -4616,6 +4670,9 @@ See also:
           status: r.status,
           ...(r.result ? {
             sync_status: r.result.status,
+            // #3068: surface the partial reason (e.g. pull_failed) so JSON
+            // consumers can distinguish a self-healing timeout from a wedge.
+            ...(r.result.reason ? { reason: r.result.reason } : {}),
             added: r.result.added,
             modified: r.result.modified,
             deleted: r.result.deleted,
@@ -4637,7 +4694,14 @@ See also:
     // Best-effort, stderr-only; skipped on dry-run.
     if (!dryRun) await maybeExtractionNudge(engine);
 
-    if (errCount > 0) process.exit(1);
+    // #3068: any source wedged on a failed pull (partial/pull_failed) makes
+    // the whole --all run non-zero — it will not self-heal on retry, so a
+    // green exit would hide it from cron/monitoring. Timeout-class partials
+    // keep the pre-existing exit-0 behavior (they converge on retry).
+    const pullFailedCount = perSourceResults.filter(
+      (r) => r.status === 'ok' && r.result?.status === 'partial' && r.result.reason === 'pull_failed',
+    ).length;
+    if (errCount > 0 || pullFailedCount > 0) process.exit(1);
     return;
   }
 
@@ -4725,6 +4789,16 @@ See also:
       process.off('SIGINT', onSingleSourceSigint);
     }
     printSyncResult(result);
+    // #3068: a pull_failed partial is NOT a success — unlike timeout-class
+    // partials (which converge on retry), a failing pull will not self-heal.
+    // Exit non-zero so cron/monitoring sees the wedge instead of a green run.
+    // Routed through the owned verdict channel (NOT bare `process.exitCode`,
+    // which PGLite's Emscripten runtime clobbers mid-run — see
+    // src/core/cli-force-exit.ts).
+    if (result.status === 'partial' && result.reason === 'pull_failed') {
+      const { setCliExitVerdict } = await import('../core/cli-force-exit.ts');
+      setCliExitVerdict(1);
+    }
     // v0.42.7 (#1696, D5): extraction-lag nudge after a completed single-source
     // sync. Fire on every non-error completion (synced | first_sync | up_to_date)
     // — NOT just 'synced'; a fresh/--full import (`first_sync`) is the biggest
@@ -5430,6 +5504,17 @@ function printSyncResult(result: SyncResult, sink: NodeJS.WriteStream = process.
       write(`  Fix the files then re-run 'gbrain sync', or 'gbrain sync --skip-failed' to move on.`);
       break;
     case 'partial':
+      // #3068: a failed (non-timeout) pull with zero imports gets its own
+      // message — "imported 0 of 0" reads like success, but the local
+      // checkout may be behind a remote we could not fetch.
+      if (result.reason === 'pull_failed') {
+        write(
+          `Sync INCOMPLETE at ${result.fromCommit?.slice(0, 8) ?? '<initial>'}: ` +
+          `git pull failed — the local checkout may be behind its remote.`,
+        );
+        write(`  Fix the pull (see the warning above), then re-run 'gbrain sync' (last_commit unchanged; safe to retry).`);
+        break;
+      }
       // v0.41.13.0 (T7 / D-V3-5): --timeout fired before the bookmark write
       // so last_commit is UNCHANGED. The next sync re-walks the same diff
       // and content_hash short-circuits already-imported files at ~10ms each.

src/core/cycle.ts

@@ -935,13 +935,21 @@ async function runPhaseSync(
                                            // sync's inline extract still runs to preserve prior behavior.
     });
     const syncedCount = result.added + result.modified;
+    // #3068: a pull_failed partial means the internal git pull failed and the
+    // run imported nothing — the source may be silently behind its remote and
+    // will not self-heal. Surface it as 'warn' (not 'ok') so a scheduled cycle
+    // doesn't report a clean run over a wedged source. Timeout-class partials
+    // keep the pre-existing 'ok' mapping (they converge on retry by design).
+    const pullFailedPartial = result.status === 'partial' && result.reason === 'pull_failed';
     return {
       phase: 'sync',
-      status: result.status === 'blocked_by_failures' ? 'warn' : 'ok',
+      status: result.status === 'blocked_by_failures' || pullFailedPartial ? 'warn' : 'ok',
       duration_ms: 0,
       summary: dryRun
         ? `${syncedCount} page(s) would sync, ${result.deleted} would delete`
-        : `+${result.added} added, ~${result.modified} modified, -${result.deleted} deleted`,
+        : pullFailedPartial
+          ? `git pull failed, nothing imported — source may be behind its remote (sync anchor unchanged)`
+          : `+${result.added} added, ~${result.modified} modified, -${result.deleted} deleted`,
       details: {
         added: result.added,
         modified: result.modified,
@@ -950,6 +958,7 @@ async function runPhaseSync(
         chunksCreated: result.chunksCreated,
         failedFiles: result.failedFiles ?? 0,
         syncStatus: result.status,
+        ...(result.reason ? { syncReason: result.reason } : {}),
         dryRun,
       },
       pagesAffected: result.pagesAffected,

test/sync-pull-failed-anchor.serial.test.ts

@@ -0,0 +1,190 @@
+import { describe, test, expect, beforeAll, afterAll, beforeEach, afterEach } from 'bun:test';
+import { mkdtempSync, writeFileSync, rmSync, mkdirSync } from 'fs';
+import { join } from 'path';
+import { execSync } from 'child_process';
+import { tmpdir } from 'os';
+import { PGLiteEngine } from '../src/core/pglite-engine.ts';
+import { resetPgliteState } from './helpers/reset-pglite.ts';
+
+/**
+ * #3068 regression: a failed internal `git pull` (warn-and-continue class,
+ * e.g. a local-filesystem-path origin rejected by the SSRF flag
+ * `protocol.file.allow=never`) combined with a zero-import run must NOT be
+ * reported as a clean `up_to_date` sync:
+ *
+ *   - status must be `partial` with reason `pull_failed` (not `up_to_date`),
+ *   - `last_commit` (the sync anchor) must not move,
+ *   - `last_sync_at` (the freshness heartbeat doctor/sources-status read)
+ *     must not be bumped — otherwise a permanently-failing pull keeps the
+ *     source looking fresh forever while it silently goes stale.
+ *
+ * The fall-through-to-working-tree design is unchanged: local commits still
+ *  import when the remote is unreachable, and once the operator repairs the
+ * checkout (e.g. a manual `git pull`, which does not carry the SSRF flags)
+ * the next sync imports the missed content from the untouched anchor.
+ *
+ * The local-path-origin topology below reproduces the pull failure
+ * deterministically: `pullRepo` always passes `-c protocol.file.allow=never`,
+ * so its internal pull fails on every cycle while plain `git pull` succeeds.
+ */
+describe('#3068: failed git pull + zero imports must not report up_to_date', () => {
+  let engine: PGLiteEngine;
+  const dirs: string[] = [];
+  // Hermetic GBRAIN_HOME: performFullSync (first sync) reads/writes the
+  // sync-failure ledger under the gbrain home — never touch the real one.
+  let isolatedHome: string;
+  let origGbrainHome: string | undefined;
+
+  beforeAll(async () => {
+    origGbrainHome = process.env.GBRAIN_HOME;
+    isolatedHome = mkdtempSync(join(tmpdir(), 'gbrain-3068-home-'));
+    process.env.GBRAIN_HOME = isolatedHome;
+    engine = new PGLiteEngine();
+    await engine.connect({});
+    await engine.initSchema();
+  });
+
+  afterAll(async () => {
+    await engine.disconnect();
+    if (origGbrainHome !== undefined) process.env.GBRAIN_HOME = origGbrainHome;
+    else delete process.env.GBRAIN_HOME;
+    rmSync(isolatedHome, { recursive: true, force: true });
+  });
+
+  beforeEach(async () => {
+    await resetPgliteState(engine);
+  });
+
+  afterEach(() => {
+    while (dirs.length) {
+      const d = dirs.pop();
+      if (d) rmSync(d, { recursive: true, force: true });
+    }
+  });
+
+  function personMd(title: string, body: string): string {
+    return ['---', 'type: person', `title: ${title}`, '---', '', body].join('\n');
+  }
+
+  function mkUpstream(files: Record<string, string>): string {
+    const dir = mkdtempSync(join(tmpdir(), 'gbrain-3068-upstream-'));
+    dirs.push(dir);
+    execSync('git init', { cwd: dir, stdio: 'pipe' });
+    execSync('git config user.email "test@test.com"', { cwd: dir, stdio: 'pipe' });
+    execSync('git config user.name "Test"', { cwd: dir, stdio: 'pipe' });
+    for (const [rel, content] of Object.entries(files)) {
+      mkdirSync(join(dir, rel, '..'), { recursive: true });
+      writeFileSync(join(dir, rel), content);
+    }
+    execSync('git add -A && git commit -m "initial"', { cwd: dir, stdio: 'pipe' });
+    return dir;
+  }
+
+  /** Clone `upstream` to a sibling temp dir — origin is a local filesystem path. */
+  function mkMirror(upstream: string): string {
+    const dir = mkdtempSync(join(tmpdir(), 'gbrain-3068-mirror-'));
+    dirs.push(dir);
+    rmSync(dir, { recursive: true, force: true });
+    execSync(`git clone ${JSON.stringify(upstream)} ${JSON.stringify(dir)}`, { stdio: 'pipe' });
+    return dir;
+  }
+
+  async function sourceRow(): Promise<{ last_commit: string | null; last_sync_at: string | null }> {
+    const rows = await engine.executeRaw<{ last_commit: string | null; last_sync_at: string | null }>(
+      `SELECT last_commit, last_sync_at FROM sources WHERE id = 'default'`,
+    );
+    return rows[0] ?? { last_commit: null, last_sync_at: null };
+  }
+
+  // Pull ENABLED (no noPull) — the internal pull must actually run and fail.
+  const SYNC_OPTS = { noEmbed: true, noExtract: true, sourceId: 'default' } as const;
+
+  test('zero-import sync after failed pull reports partial/pull_failed; anchor and heartbeat frozen; manual pull recovers', async () => {
+    const { performSync } = await import('../src/commands/sync.ts');
+    const upstream = mkUpstream({ 'people/alice.md': personMd('Alice', 'Alice is a person.') });
+    const mirror = mkMirror(upstream);
+
+    // First sync: the internal pull already fails (local-path origin), but the
+    // fall-through imports the working tree — unchanged behavior.
+    const first = await performSync(engine, { repoPath: mirror, ...SYNC_OPTS });
+    expect(first.status).toBe('first_sync');
+    const afterFirst = await sourceRow();
+    expect(afterFirst.last_commit).not.toBeNull();
+    expect(afterFirst.last_sync_at).not.toBeNull();
+
+    // New content lands upstream; the mirror's internal pull can never fetch it.
+    writeFileSync(join(upstream, 'people/bob.md'), personMd('Bob', 'zzuniquetoken99 new content.'));
+    execSync('git add -A && git commit -m "new page"', { cwd: upstream, stdio: 'pipe' });
+
+    // Wait so a (buggy) heartbeat bump would be observable as a changed timestamp.
+    await new Promise((r) => setTimeout(r, 1100));
+
+    // Pre-fix this reported `up_to_date`, bumped last_sync_at, and exited clean
+    // forever — the #3068 silent wedge.
+    const wedged = await performSync(engine, { repoPath: mirror, ...SYNC_OPTS });
+    expect(wedged.status).toBe('partial');
+    expect(wedged.reason).toBe('pull_failed');
+    expect(wedged.added + wedged.modified + wedged.deleted + wedged.renamed).toBe(0);
+    expect(wedged.fromCommit).toBe(afterFirst.last_commit);
+    expect(wedged.toCommit).toBe(afterFirst.last_commit ?? '');
+
+    const afterWedged = await sourceRow();
+    expect(afterWedged.last_commit).toBe(afterFirst.last_commit); // anchor unchanged
+    expect(afterWedged.last_sync_at).toEqual(afterFirst.last_sync_at); // heartbeat NOT bumped
+
+    // Recovery: a manual pull (no SSRF flags) fast-forwards the mirror; the
+    // next sync imports the missed content from the untouched anchor.
+    execSync('git pull', { cwd: mirror, stdio: 'pipe' });
+    const recovered = await performSync(engine, { repoPath: mirror, ...SYNC_OPTS });
+    expect(recovered.status).toBe('synced');
+    expect(recovered.added).toBe(1);
+    expect(await engine.getPage('people/bob')).not.toBeNull();
+
+    const afterRecovered = await sourceRow();
+    expect(afterRecovered.last_commit).not.toBe(afterFirst.last_commit); // anchor advanced with the import
+  });
+
+  test('failed pull with local syncable commits still imports them (fall-through preserved)', async () => {
+    const { performSync } = await import('../src/commands/sync.ts');
+    const upstream = mkUpstream({ 'people/alice.md': personMd('Alice', 'Alice is a person.') });
+    const mirror = mkMirror(upstream);
+
+    const first = await performSync(engine, { repoPath: mirror, ...SYNC_OPTS });
+    expect(first.status).toBe('first_sync');
+
+    // A local commit in the mirror itself — importable without any pull.
+    execSync('git config user.email "test@test.com"', { cwd: mirror, stdio: 'pipe' });
+    execSync('git config user.name "Test"', { cwd: mirror, stdio: 'pipe' });
+    writeFileSync(join(mirror, 'people/carol.md'), personMd('Carol', 'Carol is local.'));
+    execSync('git add -A && git commit -m "local carol"', { cwd: mirror, stdio: 'pipe' });
+
+    // The internal pull still fails, but there is real local work — the
+    // warn-and-continue design imports it and advances the anchor over the
+    // commits that were actually imported.
+    const synced = await performSync(engine, { repoPath: mirror, ...SYNC_OPTS });
+    expect(synced.status).toBe('synced');
+    expect(synced.added).toBe(1);
+    expect(await engine.getPage('people/carol')).not.toBeNull();
+  });
+
+  test('local no-syncable-content commit after failed pull also reports partial without advancing the anchor', async () => {
+    const { performSync } = await import('../src/commands/sync.ts');
+    const upstream = mkUpstream({ 'people/alice.md': personMd('Alice', 'Alice is a person.') });
+    const mirror = mkMirror(upstream);
+
+    const first = await performSync(engine, { repoPath: mirror, ...SYNC_OPTS });
+    expect(first.status).toBe('first_sync');
+    const anchor = (await sourceRow()).last_commit;
+
+    // A local commit with no syncable content (non-markdown file).
+    execSync('git config user.email "test@test.com"', { cwd: mirror, stdio: 'pipe' });
+    execSync('git config user.name "Test"', { cwd: mirror, stdio: 'pipe' });
+    writeFileSync(join(mirror, 'notes.txt'), 'not syncable');
+    execSync('git add -A && git commit -m "local txt"', { cwd: mirror, stdio: 'pipe' });
+
+    const result = await performSync(engine, { repoPath: mirror, ...SYNC_OPTS });
+    expect(result.status).toBe('partial');
+    expect(result.reason).toBe('pull_failed');
+    expect((await sourceRow()).last_commit).toBe(anchor); // anchor unchanged
+  });
+});