scriptoriumadmin/GitNexus
1
0
Fork 0
mirror of https://github.com/abhigyanpatwari/GitNexus.git synced 2026-10-03 02:21:44 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 8

fix(incremental): F1 cross-file edge consistency + F4 stable chunk sort + unit coverage (#1511)

Browse source 
Patch addressing two of the still-open changes-requested findings on PR
#1479, rebased onto the current feat/incremental-indexing head. F3
(parser fingerprint in the cache key), F5 (atomic saveMeta), and F6
(AGENTS.md phrasing) were already handled on the branch, so the
corresponding parts of the original patch were dropped as redundant.

  F1 (Blocker) — Cross-file edges between unchanged files
    Adds `computeEffectiveWriteSet(graph, toWriteSet)` to
    subgraph-extract.ts: a single pass over the new graph's edges that
    pulls the unchanged-side file of every writable-boundary-crossing
    edge into the write set. run-analyze composes it ON TOP of the
    existing importer-BFS expansion and feeds the combined set to BOTH
    `deleteNodesForFile` and `extractChangedSubgraph`, so the delete
    cascade and the writeback subgraph cover identical files (asymmetry
    would leave stale rows or PK-conflict at COPY time). The BFS reads
    IMPORTS from the pre-pipeline DB (catches files that *stopped*
    importing a changed file); the edge walk reads the new graph
    (catches refined CALLS edges the pre-run DB couldn't predict, e.g.
    a barrel re-export shifting a symbol from B to D). `extractChangedSubgraph`
    stays a pure filter — all expansion is the orchestrator's job.

  F4 (Medium) — Restore alphabetical chunk sort
    `parseableScanned` is sorted before chunking. Filesystem-scan order
    isn't stable enough across runs/platforms (notably macOS APFS) to
    keep chunk hashes consistent, so the parse cache thrashes without
    it. The pre-existing Ruby cross-file resolution order-dependency the
    old comment cited is independent — the sort surfaces it but doesn't
    cause it; tracked separately rather than leaving the cache cold.

  Tests — incremental-subgraph-extract.test.ts
    Locks the F1 invariants: `extractChangedSubgraph` is a pure filter
    (includes only the set it's given, plus graph-wide nodes; edges
    fire on one writable endpoint), and `computeEffectiveWriteSet`
    covers the barrel-re-export scenario, the symmetric edge-into-
    changed-file case, the no-boundary-crossed no-op, graph-wide-node
    edges, and input-immutability. Supersedes the prior
    extractChangedSubgraph-only test file on the branch.

Co-authored-by: Val Vladescu <vvladescu-tb@users.noreply.github.com>
This commit is contained in:
Val Vladescu 2026-05-12 09:03:09 +02:00 • committed by GitHub
parent 6afc70d00f
commit e2badb22d9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 257 additions and 99 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

72
gitnexus/src/core/incremental/subgraph-extract.ts
View file

@ -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<nodeId, filePath> for every File-bound node in the graph.
* Graph-wide nodes (Community/Process) have no filePath and are filtered.
*/
const indexNodeFilePaths = (fullGraph: KnowledgeGraph): Map<string, string> => {
const idx = new Map<string, string>();
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<string>,
@ -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<string>,
): Set<string> => {
const nodeFilePaths = indexNodeFilePaths(fullGraph);
const expanded = new Set<string>(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;
};

22
gitnexus/src/core/ingestion/pipeline-phases/parse-impl.ts
View file

@ -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;

34
gitnexus/src/core/run-analyze.ts
View file

@ -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));

228
gitnexus/test/unit/incremental-subgraph-extract.test.ts
View file

@ -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']);
});
});