mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-24 00:51:53 +00:00
fix(incremental): address Bugbot round-4 findings (added-file shadow seed + dedupe)
Bugbot review on commit e23e4400 surfaced two new findings against the
incremental writeback in run-analyze.ts:
HIGH — Incremental BFS misses importers of newly added files.
queryImporters() reads the pre-pipeline DB. For a NEWLY ADDED
file there are no IMPORTS rows pointing to it yet, so unchanged
files whose pre-existing import statements now resolve to the
newcomer keep stale CALLS edges pointing at the OLD resolution
target.
LOW — Deleted files double-counted in filesToDelete.
hashDiff.deleted entries can reappear in writableFiles via the
BFS expansion (queryImporters can return a now-deleted path),
so deleteNodesForFile() ran twice for the same file.
Fixes:
- Add gitnexus/src/core/incremental/shadow-candidates.ts: derive
the pre-existing file paths whose JS/TS module-resolution claim
an added file can steal. Pattern catalogue: same-basename/
different-extension, bare-file-beats-directory-index, and
directory-index-beats-bare-file. Emit both POSIX and Windows
separators because the prior fileHashes map may have been
written from either OS.
- In run-analyze.ts, seed the BFS frontier with shadow candidates
that exist in the prior meta.fileHashes. Their importers — found
via queryImporters — get pulled into the writable set so their
CALLS edges re-resolve against the new file.
- Dedupe filesToDelete via Set to avoid the double-call.
Tests: gitnexus/test/unit/incremental-shadow-candidates.test.ts —
8 cases covering each shadow pattern, separator handling, .d.ts as
a single extension token, deduplication, and the no-self-shadow
invariant. All 40 incremental tests (file-hash, parse-cache,
subgraph-extract, shadow-candidates, orchestration) pass locally.
Note on the third Bugbot finding ("Subgraph edges reference nodes
absent from subgraph"): re-anchored from a prior review pass — the
code at subgraph-extract.ts:48 is unchanged. Already verified as a
false positive: getNodeLabel parses labels from ID strings, CSV
write is by ID, and COPY resolves against the live DB.
This commit is contained in:
parent
e23e4400b0
commit
42302bc7d6
3 changed files with 188 additions and 3 deletions
76
gitnexus/src/core/incremental/shadow-candidates.ts
Normal file
76
gitnexus/src/core/incremental/shadow-candidates.ts
Normal file
|
|
@ -0,0 +1,76 @@
|
|||
/**
|
||||
* Shadow-candidate path derivation for incremental indexing.
|
||||
*
|
||||
* Background — Bugbot review on PR #1479:
|
||||
* queryImporters() on a NEWLY ADDED file returns 0 importers in the
|
||||
* pre-pipeline DB, because the new file's IMPORTS rows haven't been
|
||||
* written yet. But pre-existing files may have IMPORTS edges that
|
||||
* *resolved to a sibling path*, and the newcomer can now steal that
|
||||
* resolution under standard JS/TS module-resolution rules. Without
|
||||
* pulling those pre-existing files into the writable set, their
|
||||
* stale CALLS edges remain pointing at the OLD resolution target.
|
||||
*
|
||||
* Given an added file path, this helper enumerates the pre-existing
|
||||
* file paths whose import-resolution claim the newcomer can steal.
|
||||
* Caller filters the candidates against the prior-run `fileHashes`
|
||||
* map so we only query importers of paths that actually existed.
|
||||
*
|
||||
* Shadow patterns covered (resolution-priority-aware):
|
||||
*
|
||||
* (a) Same basename, different extension —
|
||||
* added `foo/bar.ts` shadows `foo/bar.{tsx,js,jsx,mjs,cjs,d.ts}`.
|
||||
* (b) Bare-file beats directory-style index —
|
||||
* added `foo/bar.ts` shadows `foo/bar/index.{ts,tsx,...}`.
|
||||
* (c) Directory-index beats bare-file —
|
||||
* added `foo/index.ts` shadows `foo.{ts,tsx,...}` (rare but real,
|
||||
* e.g. converting a single-file module into a directory module).
|
||||
*
|
||||
* Resolution-order priority is conservatively wide: we enumerate ALL
|
||||
* common extensions because we don't know which the importer actually
|
||||
* specified, and over-seeding is harmless (extra BFS work, but the
|
||||
* subgraph extract still gates write-back by file membership).
|
||||
*
|
||||
* Cross-platform path separators: candidates are emitted with both `/`
|
||||
* and `\` for shadow pattern (b), since the caller's prior fileHashes
|
||||
* map may use either depending on the OS that wrote it.
|
||||
*/
|
||||
|
||||
const SHADOW_EXTS = ['.d.ts', '.tsx', '.ts', '.jsx', '.js', '.mjs', '.cjs'];
|
||||
|
||||
/**
|
||||
* Enumerate pre-existing paths whose import-resolution `added` can steal.
|
||||
*
|
||||
* @param added — repo-relative path of a newly-added file
|
||||
* @returns deduplicated list of candidate paths (NOT filtered against
|
||||
* any known-files set — caller does that)
|
||||
*/
|
||||
export const shadowCandidatesFor = (added: string): string[] => {
|
||||
const ext = SHADOW_EXTS.find((e) => added.endsWith(e));
|
||||
if (!ext) return [];
|
||||
|
||||
const noExt = added.slice(0, -ext.length);
|
||||
const out = new Set<string>();
|
||||
|
||||
// (a) Same basename, different extension.
|
||||
for (const alt of SHADOW_EXTS) {
|
||||
if (alt !== ext) out.add(noExt + alt);
|
||||
}
|
||||
|
||||
// (b) Bare file beats sibling directory-style index.
|
||||
for (const idx of SHADOW_EXTS) {
|
||||
out.add(`${noExt}/index${idx}`);
|
||||
out.add(`${noExt}\\index${idx}`);
|
||||
}
|
||||
|
||||
// (c) New `foo/index.ext` shadows old `foo.ext`.
|
||||
const idxSuffixSlash = '/index';
|
||||
const idxSuffixBack = '\\index';
|
||||
let dir: string | null = null;
|
||||
if (noExt.endsWith(idxSuffixSlash)) dir = noExt.slice(0, -idxSuffixSlash.length);
|
||||
else if (noExt.endsWith(idxSuffixBack)) dir = noExt.slice(0, -idxSuffixBack.length);
|
||||
if (dir !== null) {
|
||||
for (const alt of SHADOW_EXTS) out.add(dir + alt);
|
||||
}
|
||||
|
||||
return [...out];
|
||||
};
|
||||
|
|
@ -37,6 +37,7 @@ import {
|
|||
} from '../storage/repo-manager.js';
|
||||
import { computeFileHashes, diffFileHashes } from '../storage/file-hash.js';
|
||||
import { extractChangedSubgraph } from './incremental/subgraph-extract.js';
|
||||
import { shadowCandidatesFor } from './incremental/shadow-candidates.js';
|
||||
import { loadParseCache, saveParseCache, pruneCache } from '../storage/parse-cache.js';
|
||||
import {
|
||||
getCurrentCommit,
|
||||
|
|
@ -466,8 +467,35 @@ export async function runFullAnalysis(
|
|||
const MAX_IMPORTER_BFS_DEPTH = 4;
|
||||
const writableFiles = new Set<string>(hashDiff.toWrite);
|
||||
const directlyChangedCount = writableFiles.size;
|
||||
|
||||
// Shadow-seed: for ADDED files, queryImporters returns 0 (the new
|
||||
// file has no IMPORTS rows in the pre-pipeline DB yet). But pre-
|
||||
// existing unchanged files may have IMPORTS edges whose module-
|
||||
// resolution claim the newcomer can steal under standard JS/TS
|
||||
// resolution (Bugbot review on PR #1479). For each added file we
|
||||
// derive the shadow candidates and, if the candidate was a known
|
||||
// file in the prior meta, seed it into the BFS frontier so its
|
||||
// importers — surfaced via queryImporters — get their CALLS edges
|
||||
// re-resolved against the new file. See shadow-candidates.ts for
|
||||
// the full pattern catalogue.
|
||||
const priorFileSet = new Set<string>(
|
||||
existingMeta?.fileHashes ? Object.keys(existingMeta.fileHashes) : [],
|
||||
);
|
||||
const shadowSeed: string[] = [];
|
||||
for (const added of hashDiff.added) {
|
||||
for (const cand of shadowCandidatesFor(added)) {
|
||||
if (priorFileSet.has(cand) && !writableFiles.has(cand)) {
|
||||
shadowSeed.push(cand);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
{
|
||||
let frontier: string[] = [...hashDiff.toWrite, ...hashDiff.deleted];
|
||||
let frontier: string[] = [
|
||||
...hashDiff.toWrite,
|
||||
...hashDiff.deleted,
|
||||
...shadowSeed,
|
||||
];
|
||||
for (let depth = 0; depth < MAX_IMPORTER_BFS_DEPTH && frontier.length > 0; depth++) {
|
||||
const nextFrontier: string[] = [];
|
||||
for (const f of frontier) {
|
||||
|
|
@ -491,12 +519,18 @@ export async function runFullAnalysis(
|
|||
if (importerExpansion > 0) {
|
||||
log(
|
||||
`Incremental: +${importerExpansion} importer(s) added to writable set ` +
|
||||
`(BFS depth ≤ ${MAX_IMPORTER_BFS_DEPTH})`,
|
||||
`(BFS depth ≤ ${MAX_IMPORTER_BFS_DEPTH}` +
|
||||
(shadowSeed.length > 0 ? `, ${shadowSeed.length} shadow-seed(s)` : '') +
|
||||
`)`,
|
||||
);
|
||||
}
|
||||
|
||||
// 1. Delete rows for files we're about to rewrite + deleted files.
|
||||
const filesToDelete = [...writableFiles, ...hashDiff.deleted];
|
||||
// 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])];
|
||||
for (let i = 0; i < filesToDelete.length; i++) {
|
||||
const f = filesToDelete[i];
|
||||
try {
|
||||
|
|
|
|||
75
gitnexus/test/unit/incremental-shadow-candidates.test.ts
Normal file
75
gitnexus/test/unit/incremental-shadow-candidates.test.ts
Normal file
|
|
@ -0,0 +1,75 @@
|
|||
import { describe, it, expect } from 'vitest';
|
||||
import { shadowCandidatesFor } from '../../src/core/incremental/shadow-candidates.js';
|
||||
|
||||
describe('shadowCandidatesFor', () => {
|
||||
it('returns an empty list when the input has no recognised module extension', () => {
|
||||
expect(shadowCandidatesFor('README.md')).toEqual([]);
|
||||
expect(shadowCandidatesFor('src/foo')).toEqual([]);
|
||||
expect(shadowCandidatesFor('binary.so')).toEqual([]);
|
||||
});
|
||||
|
||||
it('enumerates same-basename / different-extension candidates (pattern a)', () => {
|
||||
const out = shadowCandidatesFor('src/foo/bar.ts');
|
||||
// All non-.ts module extensions on the same path should appear.
|
||||
expect(out).toContain('src/foo/bar.tsx');
|
||||
expect(out).toContain('src/foo/bar.js');
|
||||
expect(out).toContain('src/foo/bar.jsx');
|
||||
expect(out).toContain('src/foo/bar.mjs');
|
||||
expect(out).toContain('src/foo/bar.cjs');
|
||||
expect(out).toContain('src/foo/bar.d.ts');
|
||||
// ...but NOT the same .ts (you can't shadow yourself).
|
||||
expect(out).not.toContain('src/foo/bar.ts');
|
||||
});
|
||||
|
||||
it('enumerates directory-style index candidates (pattern b) for both path separators', () => {
|
||||
const out = shadowCandidatesFor('src/foo/bar.ts');
|
||||
// POSIX form
|
||||
expect(out).toContain('src/foo/bar/index.ts');
|
||||
expect(out).toContain('src/foo/bar/index.tsx');
|
||||
expect(out).toContain('src/foo/bar/index.js');
|
||||
// Windows form
|
||||
expect(out).toContain('src/foo/bar\\index.ts');
|
||||
expect(out).toContain('src/foo/bar\\index.js');
|
||||
});
|
||||
|
||||
it('enumerates bare-file shadows when the added file is a directory index (pattern c)', () => {
|
||||
const out = shadowCandidatesFor('src/foo/index.ts');
|
||||
// Adding foo/index.ts can shadow foo.{ext} (rare but real — converting
|
||||
// a single-file module into a directory module).
|
||||
expect(out).toContain('src/foo.ts');
|
||||
expect(out).toContain('src/foo.tsx');
|
||||
expect(out).toContain('src/foo.js');
|
||||
expect(out).toContain('src/foo.jsx');
|
||||
expect(out).toContain('src/foo.mjs');
|
||||
expect(out).toContain('src/foo.cjs');
|
||||
});
|
||||
|
||||
it('also handles the Windows-separator form of `foo\\index.ts`', () => {
|
||||
const out = shadowCandidatesFor('src\\foo\\index.ts');
|
||||
expect(out).toContain('src\\foo.ts');
|
||||
expect(out).toContain('src\\foo.tsx');
|
||||
expect(out).toContain('src\\foo.js');
|
||||
});
|
||||
|
||||
it('handles `.d.ts` as a single extension token (not `.ts`)', () => {
|
||||
// The longest-match scan in shadowCandidatesFor puts `.d.ts` first.
|
||||
// For `foo.d.ts`, the noExt portion is "foo" (not "foo.d"), so the
|
||||
// pattern (a) candidates should be the non-.d.ts module variants.
|
||||
const out = shadowCandidatesFor('types/foo.d.ts');
|
||||
expect(out).toContain('types/foo.ts');
|
||||
expect(out).toContain('types/foo.tsx');
|
||||
expect(out).toContain('types/foo.js');
|
||||
// Not the .d.ts itself.
|
||||
expect(out).not.toContain('types/foo.d.ts');
|
||||
});
|
||||
|
||||
it('deduplicates output (no candidate appears twice)', () => {
|
||||
const out = shadowCandidatesFor('src/foo/bar.ts');
|
||||
expect(out.length).toBe(new Set(out).size);
|
||||
});
|
||||
|
||||
it('never includes the input path itself', () => {
|
||||
const input = 'src/foo/bar.ts';
|
||||
expect(shadowCandidatesFor(input)).not.toContain(input);
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue