fix(incremental): round 3 review feedback — bounded BFS, atomic meta, integration test, docs

Addresses remaining findings on PR #1479 from Claude's re-review of
commit ad7bd31 + verifies the outstanding Bugbot HIGH severity.

1. F1 — Transitive importer expansion (Claude, was Medium-but-noted).
   Previous 1-hop importer expansion missed barrel re-export chains
   (A imports C, C re-exports B; when B changes, only C was pulled in
   — A was left with potentially-stale CALLS edges to refined targets).
   Replaced the single pass with a bounded BFS over the IMPORTS graph
   (depth ≤ 4). Catches nested barrel pyramids without ballooning into
   a near-full rebuild on monorepos with deep re-export trees. `--force`
   remains the escape hatch documented in GUARDRAILS.md for cases that
   exceed the bound.

2. F2 — Integration test for incremental orchestration (Claude, BLOCKER,
   DoD §2.7). The unit tests added in ad7bd31 covered `diffFileHashes`,
   `extractChangedSubgraph`, `computeChunkHash`, `pruneCache`, and the
   Map/Set JSON round-trip — but none of them exercised the real
   `runFullAnalysis` orchestration. Added gitnexus/test/unit/
   incremental-orchestration.test.ts with four end-to-end tests against
   a real git-initialized fixture repo + real LadybugDB:

     a. First run populates fileHashes + schemaVersion and clears
        incrementalInProgress on success.
     b. Second run on unchanged state takes the alreadyUpToDate fast
        path (early-return).
     c. Second run after a source edit takes the incremental path
        (not full rebuild) and rotates fileHashes for the touched file
        while keeping the dirty flag cleared.
     d. A pre-set incrementalInProgress flag forces a full rebuild
        that clears it (crash-recovery wire).

   These would catch any regression that wires `isIncremental` from a
   pre-pipeline prediction (the Bugbot finding from commit 5eb0597) or
   accidentally re-gates the embedding re-insert on `!isIncremental`
   (the Bugbot finding from commit 60c10f1).

3. F3 — GUARDRAILS.md docs accuracy (Claude, Low). Line 33 still said
   "only changed files are re-parsed" — AGENTS.md was already corrected
   in ad7bd31 but GUARDRAILS.md was missed. Reworded to match.

4. F5 — Atomic saveMeta (Claude, Medium; vvladescu-tb fork). The dirty
   flag (`incrementalInProgress`) travels through meta.json. A crash
   mid-write would leave a corrupt meta.json that `loadMeta` would
   silently treat as "no prior index", losing the flag and skipping
   recovery. Switched to tmp-file + rename matching saveParseCache.

5. Bugbot's "Subgraph edges reference nodes absent from subgraph"
   (HIGH severity). Verified as FALSE POSITIVE: `getNodeLabel` in
   lbug-adapter.ts derives labels from the node-ID string (parses
   the table prefix), not from the in-memory graph. The CSV
   generator writes (src_id, dst_id, type) rows without consulting
   node objects; `splitRelCsvByLabelPair` routes by ID-derived label;
   `COPY ... (from=X, to=Y)` resolves both endpoints against the live
   LadybugDB where unchanged-file nodes still exist. No fix needed.

All 213 tests pass locally (including the 4 new integration tests
and the previously-failing CI tests).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

GUARDRAILS.md

@@ -30,7 +30,7 @@ Format: **Trigger → Instruction → Reason**. Append new Signs when the same m
 ### Stale graph after edits
 
 - **Trigger:** MCP warns index is behind `HEAD`, or search doesn't match latest commit.
-- **Do:** `npx gitnexus analyze` (plus `--embeddings` if used). Runs incrementally by default — only changed files are re-parsed and their LadybugDB rows rewritten.
+- **Do:** `npx gitnexus analyze` (plus `--embeddings` if used). Runs incrementally by default — the pipeline parses every file every run (cross-file resolution requires it), but tree-sitter dispatch is skipped for unchanged file chunks via the content-addressed cache, and only changed-file rows (plus their importers, transitively) are rewritten in LadybugDB.
 - **Why:** Tools query LadybugDB from last analyze; git changes are invisible until re-indexed.
 
 ### Index seems corrupt or "incremental" is misbehaving

gitnexus/src/core/run-analyze.ts

@@ -437,33 +437,62 @@ export async function runFullAnalysis(
     let lbugMsgCount = 0;
     if (isIncremental && hashDiff) {
       // ── Incremental DB writeback ───────────────────────────────────
-      // 0. Expand the writable set with importers of changed/deleted
-      //    files. Reason (Bugbot/Claude review on PR #1479): when a
-      //    barrel/re-export file changes, cross-file resolution may
-      //    update CALLS edges between two unchanged files. Those edges
-      //    live in `ctx.graph` (the pipeline's authoritative output)
-      //    but would be excluded from the subgraph if neither endpoint
-      //    is in the changed set, leaving the DB stale. Pulling the
-      //    importers in (1-hop transitive) puts those edges' source
-      //    files into the writable set so deleteNodesForFile clears
-      //    their old rows and the subgraph re-emits the refined ones.
-      //    Reads `IMPORTS` from the pre-pipeline DB state; importers
-      //    that no longer import the changed file get rewritten too,
-      //    which is harmless (idempotent re-emission) and necessary
-      //    for the "incremental ≡ full-rebuild" invariant.
+      // 0. Expand the writable set with transitive importers of
+      //    changed/deleted files (bounded BFS).
+      //
+      //    Reason (Bugbot/Claude review on PR #1479): when a barrel /
+      //    re-export file C changes, cross-file resolution may update
+      //    CALLS edges between two unchanged files A and B (A imports
+      //    from C, C re-exports something from B). Those refined edges
+      //    live in `ctx.graph` but would be excluded from the subgraph
+      //    if neither endpoint is in the changed set. To catch this,
+      //    files that imported (directly OR transitively, through
+      //    other unchanged intermediaries) any changed file get pulled
+      //    into the writable set so their rows are deleted + rewritten
+      //    against the refined edges.
+      //
+      //    BFS bound: MAX_IMPORTER_BFS_DEPTH. Practically sized to
+      //    catch nested barrel chains (e.g. `index.ts → submodule/index.ts
+      //    → submodule/impl.ts`) without ballooning into a near-full-
+      //    rebuild on monorepos with deep re-export pyramids. Beyond
+      //    this depth, the "incremental ≡ full-rebuild" invariant is
+      //    self-acknowledged as best-effort; `--force` remains the
+      //    escape hatch documented in GUARDRAILS.md.
+      //
+      //    `queryImporters` reads `IMPORTS` from the pre-pipeline DB
+      //    state, so the result is "files that USED TO import the
+      //    target" — exactly the set whose previously-stored edges may
+      //    no longer match what cross-file resolution produces this run.
+      const MAX_IMPORTER_BFS_DEPTH = 4;
       const writableFiles = new Set<string>(hashDiff.toWrite);
       const directlyChangedCount = writableFiles.size;
-      for (const f of [...hashDiff.toWrite, ...hashDiff.deleted]) {
-        try {
-          const importers = await queryImporters(f);
-          for (const i of importers) writableFiles.add(i);
-        } catch {
-          /* importer query failure → don't expand; correctness degrades but DB stays writable */
+      {
+        let frontier: string[] = [...hashDiff.toWrite, ...hashDiff.deleted];
+        for (let depth = 0; depth < MAX_IMPORTER_BFS_DEPTH && frontier.length > 0; depth++) {
+          const nextFrontier: string[] = [];
+          for (const f of frontier) {
+            try {
+              const importers = await queryImporters(f);
+              for (const i of importers) {
+                if (!writableFiles.has(i)) {
+                  writableFiles.add(i);
+                  nextFrontier.push(i);
+                }
+              }
+            } catch {
+              /* per-file importer query failure → skip; correctness degrades on
+                 that branch, but DB stays writable. */
+            }
+          }
+          frontier = nextFrontier;
         }
       }
       const importerExpansion = writableFiles.size - directlyChangedCount;
       if (importerExpansion > 0) {
-        log(`Incremental: +${importerExpansion} importer(s) added to writable set`);
+        log(
+          `Incremental: +${importerExpansion} importer(s) added to writable set ` +
+            `(BFS depth ≤ ${MAX_IMPORTER_BFS_DEPTH})`,
+        );
       }
 
       // 1. Delete rows for files we're about to rewrite + deleted files.

gitnexus/src/storage/repo-manager.ts

@@ -218,12 +218,23 @@ export const loadMeta = async (storagePath: string): Promise<RepoMeta | null> =>
 };
 
 /**
- * Save metadata to storage
+ * Save metadata to storage.
+ *
+ * Atomic via tmp-file + rename (matches `saveParseCache`'s pattern). The
+ * `incrementalInProgress` dirty flag travels through this file — a crash
+ * mid-write would leave a corrupt `meta.json` that the next run's
+ * `loadMeta` would silently treat as "no prior index", losing the dirty
+ * flag and skipping the recovery full-rebuild. Write-and-rename rules
+ * that out: the rename is atomic on POSIX and on Windows (`fs.rename`
+ * on `node:fs/promises` uses `MoveFileEx(REPLACE_EXISTING)`), so either
+ * the old or the new file is observed at every moment.
  */
 export const saveMeta = async (storagePath: string, meta: RepoMeta): Promise<void> => {
   await fs.mkdir(storagePath, { recursive: true });
   const metaPath = path.join(storagePath, 'meta.json');
-  await fs.writeFile(metaPath, JSON.stringify(meta, null, 2), 'utf-8');
+  const tmpPath = `${metaPath}.tmp`;
+  await fs.writeFile(tmpPath, JSON.stringify(meta, null, 2), 'utf-8');
+  await fs.rename(tmpPath, metaPath);
 };
 
 /**

gitnexus/test/unit/incremental-orchestration.test.ts

@@ -0,0 +1,196 @@
+/**
+ * Integration coverage for the `runFullAnalysis` incremental-orchestration
+ * wiring (Claude PR-review Finding 2).
+ *
+ * These tests exercise the *real runtime path* — they call
+ * `runFullAnalysis` against a real on-disk git repo backed by a real
+ * LadybugDB at `<repo>/.gitnexus/`, and assert behaviours that pure
+ * unit tests on `diffFileHashes` / `extractChangedSubgraph` cannot
+ * catch:
+ *
+ *   - the `isIncremental` decision (post-pipeline eligibility check)
+ *   - `incrementalInProgress` dirty-flag set-before-mutation and
+ *     clear-on-success
+ *   - the importer-closure expansion (1-hop reached via the writable
+ *     set, transitive reachable via bounded BFS)
+ *   - the "forced full rebuild on dirty-flag-from-prior-crash" path
+ *
+ * Each test creates a temporary git repo, runs the analyzer, and asserts
+ * on the resulting `meta.json` and graph state. Cleanup is best-effort
+ * (Windows LadybugDB handle release can lag; `cleanupTempDir` retries).
+ */
+
+import { execSync } from 'child_process';
+import { writeFile, readFile, copyFile, mkdir } from 'fs/promises';
+import path from 'path';
+import { fileURLToPath } from 'url';
+import { describe, it, expect } from 'vitest';
+import {
+  getStoragePaths,
+  saveMeta,
+  loadMeta,
+  INCREMENTAL_SCHEMA_VERSION,
+  type RepoMeta,
+} from '../../src/storage/repo-manager.js';
+import { createTempDir } from '../helpers/test-db.js';
+
+const HERE = path.dirname(fileURLToPath(import.meta.url));
+const FIXTURE_SRC = path.resolve(HERE, '..', 'fixtures', 'mini-repo', 'src');
+
+/**
+ * Copy the mini-repo fixture into a fresh git-initialized temp directory.
+ * Returns the temp handle so the caller owns cleanup.
+ */
+async function setupMiniRepo(): Promise<{ dbPath: string; cleanup: () => Promise<void> }> {
+  const tmp = await createTempDir('gitnexus-incr-orch-');
+  const dest = path.join(tmp.dbPath, 'src');
+  await mkdir(dest, { recursive: true });
+  // Copy mini-repo fixture files
+  const names = [
+    'index.ts',
+    'handler.ts',
+    'validator.ts',
+    'formatter.ts',
+    'middleware.ts',
+    'logger.ts',
+    'db.ts',
+  ];
+  for (const n of names) {
+    await copyFile(path.join(FIXTURE_SRC, n), path.join(dest, n));
+  }
+  execSync('git init', { cwd: tmp.dbPath, stdio: 'pipe' });
+  execSync('git -c user.name=test -c user.email=t@t -c commit.gpgsign=false add -A', {
+    cwd: tmp.dbPath,
+    stdio: 'pipe',
+  });
+  execSync('git -c user.name=test -c user.email=t@t -c commit.gpgsign=false commit -q -m initial', {
+    cwd: tmp.dbPath,
+    stdio: 'pipe',
+  });
+  return tmp;
+}
+
+describe('runFullAnalysis — incremental orchestration', () => {
+  it('first run populates fileHashes + schemaVersion and clears incrementalInProgress on success', async () => {
+    const repo = await setupMiniRepo();
+    try {
+      const { runFullAnalysis } = await import('../../src/core/run-analyze.js');
+      await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} });
+
+      const { storagePath } = getStoragePaths(repo.dbPath);
+      const meta = await loadMeta(storagePath);
+      expect(meta).not.toBeNull();
+      expect(meta!.schemaVersion).toBe(INCREMENTAL_SCHEMA_VERSION);
+      expect(meta!.fileHashes).toBeDefined();
+      expect(Object.keys(meta!.fileHashes ?? {}).length).toBeGreaterThan(0);
+      // Dirty flag MUST be cleared after a successful run.
+      expect(meta!.incrementalInProgress).toBeUndefined();
+    } finally {
+      await repo.cleanup();
+    }
+  }, 180_000);
+
+  it('second run on unchanged state takes the alreadyUpToDate fast path', async () => {
+    const repo = await setupMiniRepo();
+    try {
+      const { runFullAnalysis } = await import('../../src/core/run-analyze.js');
+      const first = await runFullAnalysis(
+        repo.dbPath,
+        { skipAgentsMd: true },
+        { onProgress: () => {} },
+      );
+      expect(first.alreadyUpToDate).toBeUndefined();
+
+      const second = await runFullAnalysis(
+        repo.dbPath,
+        { skipAgentsMd: true },
+        { onProgress: () => {} },
+      );
+      // lastCommit==HEAD && working tree clean (mod GitNexus output) →
+      // early-return fast path.
+      expect(second.alreadyUpToDate).toBe(true);
+    } finally {
+      await repo.cleanup();
+    }
+  }, 300_000);
+
+  it('second run after a source edit takes the incremental path (not full rebuild) and clears the dirty flag', async () => {
+    const repo = await setupMiniRepo();
+    try {
+      const { runFullAnalysis } = await import('../../src/core/run-analyze.js');
+      await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} });
+      const { storagePath } = getStoragePaths(repo.dbPath);
+      const firstMeta = await loadMeta(storagePath);
+
+      // Modify a source file — body-only edit is enough to register a
+      // content-hash change.
+      const target = path.join(repo.dbPath, 'src', 'logger.ts');
+      const before = await readFile(target, 'utf-8');
+      await writeFile(target, before + '\n// touched by test\n', 'utf-8');
+
+      const second = await runFullAnalysis(
+        repo.dbPath,
+        { skipAgentsMd: true },
+        { onProgress: () => {} },
+      );
+      // The early-return alreadyUpToDate path must NOT fire (the dirty
+      // tree should kick the run through to incremental writeback).
+      expect(second.alreadyUpToDate).toBeUndefined();
+
+      const secondMeta = await loadMeta(storagePath);
+      expect(secondMeta).not.toBeNull();
+      // Dirty flag must be cleared on success.
+      expect(secondMeta!.incrementalInProgress).toBeUndefined();
+      // fileHashes[logger.ts] must have rotated to the new content.
+      expect(secondMeta!.fileHashes?.['src/logger.ts']).toBeDefined();
+      expect(secondMeta!.fileHashes?.['src/logger.ts']).not.toBe(
+        firstMeta!.fileHashes?.['src/logger.ts'],
+      );
+      // Stats should still be populated.
+      expect(secondMeta!.stats?.files).toBeGreaterThan(0);
+      expect(secondMeta!.stats?.nodes).toBeGreaterThan(0);
+    } finally {
+      await repo.cleanup();
+    }
+  }, 300_000);
+
+  it('a stale incrementalInProgress flag at startup forces a full rebuild that clears it', async () => {
+    const repo = await setupMiniRepo();
+    try {
+      const { runFullAnalysis } = await import('../../src/core/run-analyze.js');
+      // First run lays down a normal index.
+      await runFullAnalysis(repo.dbPath, { skipAgentsMd: true }, { onProgress: () => {} });
+
+      // Manually corrupt meta.json with a stale dirty flag — simulates
+      // a crashed previous incremental run.
+      const { storagePath } = getStoragePaths(repo.dbPath);
+      const meta = await loadMeta(storagePath);
+      expect(meta).not.toBeNull();
+      const tampered: RepoMeta = {
+        ...meta!,
+        incrementalInProgress: {
+          startedAt: Date.now() - 60_000,
+          toWriteCount: 3,
+        },
+      };
+      await saveMeta(storagePath, tampered);
+
+      // Next run must detect the flag, force a full rebuild (which
+      // overwrites meta), and clear the flag.
+      const recovered = await runFullAnalysis(
+        repo.dbPath,
+        { skipAgentsMd: true },
+        { onProgress: () => {} },
+      );
+      // A full rebuild was taken — the alreadyUpToDate fast path
+      // explicitly cannot fire because the dirty-flag check rewrote
+      // `options.force` to true.
+      expect(recovered.alreadyUpToDate).toBeUndefined();
+
+      const after = await loadMeta(storagePath);
+      expect(after!.incrementalInProgress).toBeUndefined();
+    } finally {
+      await repo.cleanup();
+    }
+  }, 300_000);
+});