diff --git a/GUARDRAILS.md b/GUARDRAILS.md index 8521a9ac9..c09f0319a 100644 --- a/GUARDRAILS.md +++ b/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 diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 91150b2e8..1dbf85b10 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/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(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. diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index c6fef8e2a..456a6c143 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -218,12 +218,23 @@ export const loadMeta = async (storagePath: string): Promise => }; /** - * 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 => { 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); }; /** diff --git a/gitnexus/test/unit/incremental-orchestration.test.ts b/gitnexus/test/unit/incremental-orchestration.test.ts new file mode 100644 index 000000000..e07bcd589 --- /dev/null +++ b/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 `/.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 }> { + 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); +});