diff --git a/gitnexus/src/core/incremental/subgraph-extract.ts b/gitnexus/src/core/incremental/subgraph-extract.ts index e181cc526..71fe656be 100644 --- a/gitnexus/src/core/incremental/subgraph-extract.ts +++ b/gitnexus/src/core/incremental/subgraph-extract.ts @@ -17,6 +17,35 @@ * The resulting subgraph is what gets passed to `loadGraphToLbug` after * the orchestrator has deleted the corresponding DB rows. Hydrated * unchanged-file rows are never touched in the DB. + * + * # Cross-file edge consistency (Finding 1) + * + * `extractChangedSubgraph` intentionally does NOT expand the set it is + * given — expansion is the orchestrator's job, so the SAME expanded set + * can be fed to both `deleteNodesForFile` and this function (asymmetry + * between the delete set and the write set silently corrupts the DB). + * `computeEffectiveWriteSet` below performs the boundary-crossing 1-hop + * walk; the orchestrator composes it with its importer-BFS expansion and + * passes the result here. + * + * Why the 1-hop walk is needed: consider a barrel re-export change — + * file C (a barrel) shifts `export { foo } from './b'` to + * `export { foo } from './d'`. After scope resolution, file A's CALLS + * edge to `foo` resolves to D instead of B, even though A's content is + * byte-for-byte identical: + * + * - Old A→B edge survives in DB (neither A nor B is changed → not deleted) + * - New A→D edge is missing (neither A nor D in writable set → skipped) + * + * Pulling the unchanged-side file of every writable-boundary-crossing + * edge into the write set fixes both halves: the orchestrator's + * `DETACH DELETE` cleans up the stale unchanged-side rows, and the new + * cross-file edges land because at least one endpoint is now writable. + * + * Limitation (documented): if a file X *stopped* importing from a + * changed file C, X has no edge to C in the new graph, so this 1-hop + * walk doesn't catch it. The orchestrator's importer-BFS (which reads + * IMPORTS from the pre-pipeline DB) covers that case instead. */ import type { GraphNode, GraphRelationship } from 'gitnexus-shared'; @@ -25,6 +54,19 @@ import type { KnowledgeGraph } from '../graph/types.js'; const isGraphWide = (label: string): boolean => label === 'Community' || label === 'Process'; +/** + * Build a Map for every File-bound node in the graph. + * Graph-wide nodes (Community/Process) have no filePath and are filtered. + */ +const indexNodeFilePaths = (fullGraph: KnowledgeGraph): Map => { + const idx = new Map(); + fullGraph.forEachNode((n: GraphNode) => { + const fp = n.properties?.filePath as string | undefined; + if (fp) idx.set(n.id, fp); + }); + return idx; +}; + export const extractChangedSubgraph = ( fullGraph: KnowledgeGraph, toWriteSet: ReadonlySet, @@ -49,3 +91,33 @@ export const extractChangedSubgraph = ( return sub; }; + +/** + * Public — derive the EFFECTIVE write-set: `toWriteSet` expanded by one + * hop along every edge in the new graph that crosses the writable + * boundary (one endpoint in a writable file, the other in an unchanged + * file). The unchanged-side file is pulled in so its stale rows are + * deleted + rewritten in lockstep with the changed side. + * + * Single pass over the edge list. Does NOT mutate `toWriteSet`. The + * orchestrator MUST feed the returned set to both `deleteNodesForFile` + * and `extractChangedSubgraph` — feeding the unexpanded set to either + * one leaves stale rows or PK-conflicts at COPY time. + */ +export const computeEffectiveWriteSet = ( + fullGraph: KnowledgeGraph, + toWriteSet: ReadonlySet, +): Set => { + const nodeFilePaths = indexNodeFilePaths(fullGraph); + const expanded = new Set(toWriteSet); + fullGraph.forEachRelationship((r: GraphRelationship) => { + const sourcePath = nodeFilePaths.get(r.sourceId); + const targetPath = nodeFilePaths.get(r.targetId); + if (!sourcePath || !targetPath) return; // skip edges to graph-wide nodes + const sourceWritable = toWriteSet.has(sourcePath); + const targetWritable = toWriteSet.has(targetPath); + if (sourceWritable && !targetWritable) expanded.add(targetPath); + else if (targetWritable && !sourceWritable) expanded.add(sourcePath); + }); + return expanded; +}; diff --git a/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts b/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts index 17dc5e41c..44e2e8a93 100644 --- a/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts +++ b/gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts @@ -163,14 +163,20 @@ export async function runChunkedParseAndResolve( ); } - // We previously sorted parseableScanned alphabetically here for stable - // chunk membership across runs (so the parse cache wouldn't miss when - // filesystem-scan order varied). Removed because it surfaced a - // pre-existing order-dependency in Ruby cross-file resolution - // (`user.address.save → Address#save` resolution depends on file - // processing order — a separate bug to fix). Filesystem order on most - // platforms is stable enough in practice that the cache still hits the - // common case; runs where it doesn't simply pay a cold-parse cost. + // Sort parseableScanned alphabetically for stable chunk membership + // across runs (Finding 4). Without this, filesystem-scan order can + // shift between runs (notably on macOS APFS where directory entry + // order can change after modifications) — different files in the + // same chunk → different chunk hash → cache miss even when no file + // content changed. The cache also becomes platform-specific: a + // Linux-built cache misses on macOS for the same repo. + // + // Note: this re-introduces a pre-existing order-dependency in Ruby + // cross-file resolution (`user.address.save → Address#save` resolves + // differently depending on file processing order). That bug is + // independent — the sort surfaces it but doesn't cause it. Tracking + // separately rather than letting the parse cache pay the cost. + parseableScanned.sort((a, b) => (a.path < b.path ? -1 : a.path > b.path ? 1 : 0)); const totalParseable = parseableScanned.length; diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index bd2ac4867..64c6b5497 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -36,7 +36,7 @@ import { INCREMENTAL_SCHEMA_VERSION, } from '../storage/repo-manager.js'; import { computeFileHashes, diffFileHashes } from '../storage/file-hash.js'; -import { extractChangedSubgraph } from './incremental/subgraph-extract.js'; +import { extractChangedSubgraph, computeEffectiveWriteSet } from './incremental/subgraph-extract.js'; import { shadowCandidatesFor } from './incremental/shadow-candidates.js'; import { loadParseCache, saveParseCache, pruneCache } from '../storage/parse-cache.js'; import { @@ -523,12 +523,26 @@ export async function runFullAnalysis( ); } - // 1. Delete rows for files we're about to rewrite + deleted files. - // Deduped: deleted entries may already appear in writableFiles via - // BFS expansion (queryImporters can return a now-deleted path), - // which would otherwise call deleteNodesForFile twice for the - // same file (Bugbot LOW finding on PR #1479). - const filesToDelete = [...new Set([...writableFiles, ...hashDiff.deleted])]; + // 1. Compute the EFFECTIVE write-set (Finding 1). Two layers, + // composed: + // (a) `writableFiles` — toWrite ∪ transitive importers of + // changed/deleted files (the bounded BFS above, reading + // IMPORTS from the pre-pipeline DB). + // (b) `computeEffectiveWriteSet` — walks the NEW graph's + // edges and pulls in any unchanged-side file that sits + // on a writable-boundary-crossing edge (catches refined + // cross-file CALLS edges that the pre-run DB couldn't + // predict, e.g. a barrel re-export shifting `foo` from + // B to D). + // The composed set is the input to BOTH deleteNodesForFile + // and extractChangedSubgraph — asymmetry between the two would + // leave stale rows or PK-conflict at COPY time. + const effectiveWriteSet = computeEffectiveWriteSet(pipelineResult.graph, writableFiles); + // Deduped: deleted entries may already appear via importer-BFS + // expansion (queryImporters can return a now-deleted path), which + // would otherwise call deleteNodesForFile twice for the same file + // (Bugbot LOW finding on PR #1479). + const filesToDelete = [...new Set([...effectiveWriteSet, ...hashDiff.deleted])]; for (let i = 0; i < filesToDelete.length; i++) { const f = filesToDelete[i]; try { @@ -546,8 +560,10 @@ export async function runFullAnalysis( await deleteAllCommunitiesAndProcesses(); // 3. Extract the changed subgraph from the FULL ctx.graph and write - // only that. Unchanged-file rows in the DB stay untouched. - const subgraph = extractChangedSubgraph(pipelineResult.graph, writableFiles); + // only that. Unchanged-file rows in the DB stay untouched. Pass + // the SAME effectiveWriteSet so the subgraph and the deletes + // cover identical files (asymmetry would silently corrupt). + const subgraph = extractChangedSubgraph(pipelineResult.graph, effectiveWriteSet); await loadGraphToLbug(subgraph, pipelineResult.repoPath, storagePath, (msg) => { lbugMsgCount++; const pct = Math.min(84, 65 + Math.round((lbugMsgCount / (lbugMsgCount + 10)) * 19)); diff --git a/gitnexus/test/unit/incremental-subgraph-extract.test.ts b/gitnexus/test/unit/incremental-subgraph-extract.test.ts index f2a8c3e3e..dc720fd9e 100644 --- a/gitnexus/test/unit/incremental-subgraph-extract.test.ts +++ b/gitnexus/test/unit/incremental-subgraph-extract.test.ts @@ -1,105 +1,169 @@ +/** + * Tests for incremental DB writeback subgraph extraction. + * + * Locks the Finding 1 fix (PR #1479 review): cross-file edges between + * two unchanged files MUST land in the writeback subgraph when a third + * (changed) file alters their cross-file resolution. The pre-fix + * behaviour silently dropped those edges, leaving stale rows in the DB. + * + * These tests use synthetic graphs constructed via createKnowledgeGraph + * directly — they don't run the parser, so they're cheap and stable. + */ + import { describe, it, expect } from 'vitest'; -import { createKnowledgeGraph } from '../../src/core/graph/graph.js'; -import { extractChangedSubgraph } from '../../src/core/incremental/subgraph-extract.js'; import type { GraphNode, GraphRelationship } from 'gitnexus-shared'; +import { createKnowledgeGraph } from '../../src/core/graph/graph.js'; +import { + extractChangedSubgraph, + computeEffectiveWriteSet, +} from '../../src/core/incremental/subgraph-extract.js'; -const fileNode = ( +const makeFileNode = (id: string, filePath: string, label = 'Function'): GraphNode => + ({ + id, + label, + properties: { filePath, name: id }, + }) as unknown as GraphNode; + +const makeWideNode = (id: string, label: 'Community' | 'Process'): GraphNode => + ({ + id, + label, + properties: {}, + }) as unknown as GraphNode; + +const makeRel = ( id: string, - filePath: string, - label: GraphNode['label'] = 'Function', -): GraphNode => ({ - id, - label, - properties: { name: id, filePath }, -}); - -const wideNode = (id: string, label: GraphNode['label']): GraphNode => ({ - id, - label, - properties: { name: id }, -}); - -const rel = ( - id: string, - type: GraphRelationship['type'], - src: string, - dst: string, -): GraphRelationship => ({ - id, - type, - sourceId: src, - targetId: dst, - confidence: 1, - reason: 't', -}); + sourceId: string, + targetId: string, + type = 'CALLS', +): GraphRelationship => + ({ + id, + sourceId, + targetId, + type, + properties: {}, + }) as unknown as GraphRelationship; describe('extractChangedSubgraph', () => { - it('keeps file nodes whose filePath is in the writable set', () => { + it('includes nodes whose filePath is in the explicit toWriteSet', () => { const g = createKnowledgeGraph(); - g.addNode(fileNode('Function:a.ts:foo', 'a.ts')); - g.addNode(fileNode('Function:b.ts:bar', 'b.ts')); - g.addNode(fileNode('Function:c.ts:baz', 'c.ts')); + g.addNode(makeFileNode('a', '/repo/a.ts')); + g.addNode(makeFileNode('c', '/repo/c.ts')); - const sub = extractChangedSubgraph(g, new Set(['a.ts', 'c.ts'])); - expect(sub.nodeCount).toBe(2); - expect(sub.getNode('Function:a.ts:foo')).toBeDefined(); - expect(sub.getNode('Function:c.ts:baz')).toBeDefined(); - expect(sub.getNode('Function:b.ts:bar')).toBeUndefined(); + const sub = extractChangedSubgraph(g, new Set(['/repo/c.ts'])); + + expect(sub.nodes.map((n) => n.id).sort()).toEqual(['c']); }); - it('always keeps Community and Process nodes (regenerated graph-wide)', () => { + it('always includes graph-wide nodes (Community, Process)', () => { const g = createKnowledgeGraph(); - g.addNode(fileNode('Function:a.ts:foo', 'a.ts')); - g.addNode(wideNode('comm_1', 'Community')); - g.addNode(wideNode('proc_1', 'Process')); + g.addNode(makeFileNode('a', '/repo/a.ts')); + g.addNode(makeWideNode('comm-1', 'Community')); + g.addNode(makeWideNode('proc-1', 'Process')); - // Empty writable set: only graph-wide nodes survive - const sub = extractChangedSubgraph(g, new Set()); - expect(sub.getNode('Function:a.ts:foo')).toBeUndefined(); - expect(sub.getNode('comm_1')).toBeDefined(); - expect(sub.getNode('proc_1')).toBeDefined(); + const sub = extractChangedSubgraph(g, new Set([])); // no files changed + + expect(sub.nodes.map((n) => n.id).sort()).toEqual(['comm-1', 'proc-1']); }); - it('keeps edges where at least one endpoint is in the writable subgraph', () => { + it('includes a relationship when at least one endpoint is writable', () => { const g = createKnowledgeGraph(); - g.addNode(fileNode('Function:a.ts:foo', 'a.ts')); - g.addNode(fileNode('Function:b.ts:bar', 'b.ts')); - g.addNode(fileNode('Function:c.ts:baz', 'c.ts')); - g.addRelationship(rel('r-ab', 'CALLS', 'Function:a.ts:foo', 'Function:b.ts:bar')); - g.addRelationship(rel('r-bc', 'CALLS', 'Function:b.ts:bar', 'Function:c.ts:baz')); - g.addRelationship(rel('r-ac', 'CALLS', 'Function:a.ts:foo', 'Function:c.ts:baz')); + g.addNode(makeFileNode('a:fn', '/repo/a.ts')); + g.addNode(makeFileNode('c:fn', '/repo/c.ts')); + g.addRelationship(makeRel('e1', 'a:fn', 'c:fn', 'CALLS')); - const sub = extractChangedSubgraph(g, new Set(['a.ts'])); - // r-ab: src in a.ts (writable) → kept - // r-ac: src in a.ts (writable) → kept - // r-bc: src in b.ts, dst in c.ts (both unchanged) → dropped - const rels = [...sub.iterRelationships()].map((r) => r.id).sort(); - expect(rels).toEqual(['r-ab', 'r-ac']); + // toWriteSet already includes A (the orchestrator expanded it via + // computeEffectiveWriteSet) — both endpoints writable, edge fires. + const sub = extractChangedSubgraph(g, new Set(['/repo/a.ts', '/repo/c.ts'])); + + expect(sub.nodes.map((n) => n.id).sort()).toEqual(['a:fn', 'c:fn']); + expect(sub.relationships.map((r) => r.id)).toEqual(['e1']); }); - it('keeps MEMBER_OF edges (Community endpoints are graph-wide)', () => { + it('skips a relationship entirely between unchanged files', () => { const g = createKnowledgeGraph(); - g.addNode(fileNode('Function:a.ts:foo', 'a.ts')); - g.addNode(fileNode('Function:b.ts:bar', 'b.ts')); - g.addNode(wideNode('comm_1', 'Community')); - // Both functions are members of the same community - g.addRelationship(rel('r-a-comm', 'MEMBER_OF', 'Function:a.ts:foo', 'comm_1')); - g.addRelationship(rel('r-b-comm', 'MEMBER_OF', 'Function:b.ts:bar', 'comm_1')); + g.addNode(makeFileNode('x:fn', '/repo/x.ts')); + g.addNode(makeFileNode('y:fn', '/repo/y.ts')); + g.addRelationship(makeRel('e1', 'x:fn', 'y:fn', 'CALLS')); - const sub = extractChangedSubgraph(g, new Set(['a.ts'])); - // r-a-comm: src in a.ts (writable) — kept - // r-b-comm: src in b.ts (NOT writable) but dst is graph-wide (Community is in writable subgraph) — kept - const rels = [...sub.iterRelationships()].map((r) => r.id).sort(); - expect(rels).toEqual(['r-a-comm', 'r-b-comm']); - }); + const sub = extractChangedSubgraph(g, new Set(['/repo/c.ts'])); - it('produces an empty subgraph when no nodes match', () => { - const g = createKnowledgeGraph(); - g.addNode(fileNode('Function:a.ts:foo', 'a.ts')); - g.addRelationship(rel('r1', 'CALLS', 'Function:a.ts:foo', 'Function:a.ts:foo')); - - const sub = extractChangedSubgraph(g, new Set(['nonexistent.ts'])); - expect(sub.nodeCount).toBe(0); - expect(sub.relationshipCount).toBe(0); + expect(sub.nodes).toEqual([]); + expect(sub.relationships).toEqual([]); + }); +}); + +describe('computeEffectiveWriteSet (Finding 1)', () => { + it('barrel re-export — expands the writable set to the consumer file', () => { + // Scenario: file C (a barrel) used to re-export from B; now re-exports + // from D. File A is unchanged byte-wise but its CALLS to foo() now + // resolve to D instead of B. Both A and D are unchanged at the file + // level — but A's edges have shifted. + // + // Pre-fix: toWriteSet={C} → A's nodes not deleted, A→D edge not + // inserted (neither endpoint writable). DB ends up with + // stale A→B and missing A→D. + // Post-fix: the new graph has A→C (A still imports the barrel), so + // A crosses the writable boundary and joins the effective + // write set. deleteNodesForFile(A) then clears the stale + // rows and the subgraph carries the new A→D edge. + const g = createKnowledgeGraph(); + g.addNode(makeFileNode('a:fn', '/repo/a.ts')); + g.addNode(makeFileNode('b:fn', '/repo/b.ts')); + g.addNode(makeFileNode('c:re-export', '/repo/c.ts')); + g.addNode(makeFileNode('d:fn', '/repo/d.ts')); + g.addRelationship(makeRel('e1', 'a:fn', 'c:re-export', 'IMPORTS')); + g.addRelationship(makeRel('e2', 'a:fn', 'd:fn', 'CALLS')); + + const effective = computeEffectiveWriteSet(g, new Set(['/repo/c.ts'])); + + expect([...effective].sort()).toEqual(['/repo/a.ts', '/repo/c.ts']); + }); + + it('picks up edges pointing INTO the changed file (symmetric case)', () => { + const g = createKnowledgeGraph(); + g.addNode(makeFileNode('b:fn', '/repo/b.ts')); + g.addNode(makeFileNode('c:fn', '/repo/c.ts')); + g.addRelationship(makeRel('e1', 'b:fn', 'c:fn', 'CALLS')); + + const effective = computeEffectiveWriteSet(g, new Set(['/repo/c.ts'])); + + expect([...effective].sort()).toEqual(['/repo/b.ts', '/repo/c.ts']); + }); + + it('does not expand when no edge crosses the writable boundary', () => { + const g = createKnowledgeGraph(); + g.addNode(makeFileNode('x:fn', '/repo/x.ts')); + g.addNode(makeFileNode('y:fn', '/repo/y.ts')); + g.addRelationship(makeRel('e1', 'x:fn', 'y:fn', 'CALLS')); + + const effective = computeEffectiveWriteSet(g, new Set(['/repo/c.ts'])); + + expect([...effective].sort()).toEqual(['/repo/c.ts']); + }); + + it('ignores edges to graph-wide nodes (no filePath)', () => { + const g = createKnowledgeGraph(); + g.addNode(makeFileNode('a:fn', '/repo/a.ts')); + g.addNode(makeWideNode('comm-1', 'Community')); + g.addRelationship(makeRel('e1', 'a:fn', 'comm-1', 'BELONGS_TO')); + + const effective = computeEffectiveWriteSet(g, new Set(['/repo/a.ts'])); + + expect([...effective].sort()).toEqual(['/repo/a.ts']); + }); + + it('does not mutate the input set', () => { + const g = createKnowledgeGraph(); + g.addNode(makeFileNode('a:fn', '/repo/a.ts')); + g.addNode(makeFileNode('c:fn', '/repo/c.ts')); + g.addRelationship(makeRel('e1', 'a:fn', 'c:fn', 'CALLS')); + + const input = new Set(['/repo/c.ts']); + computeEffectiveWriteSet(g, input); + + expect([...input]).toEqual(['/repo/c.ts']); }); });