mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-29 06:32:28 +00:00
* fix(ingestion): surface skipped large-file paths by default (#1659) The 512 KB skip threshold in filesystem-walker is necessary, but the existing warning only said "Skipped N large files" with no paths unless GITNEXUS_VERBOSE=1 was set. In a repo with one or two oversized first- party source files (e.g. a 17K-line cron handler), every IMPORTS/CALLS edge from that file silently disappeared and the surface looked like a Python resolver bug. Issue #1659 was filed against the resolver for exactly that reason, but the resolver was fine; the file was being dropped before parse. Changes: * Always print up to 5 skipped paths after the count line. * If more than 5 were skipped, append "...and N more" with a hint to set GITNEXUS_VERBOSE=1 for the full list. * When running at the default threshold, emit a one-line hint about GITNEXUS_MAX_FILE_SIZE=<KB> so operators know how to widen it. * Cover the new behavior with three additional tests in the existing filesystem-walker integration suite, plus a new describe block for the >5 preview-cap case. Verified end-to-end on a 680-file Python repo that hit #1659: before the patch, "Skipped 3 large files (>512KB, ...)" was the only signal and impact upstream of a function called from cron.py returned 1 of 5 real callers; after the patch the cron file is listed by name with the hint, and running with GITNEXUS_MAX_FILE_SIZE=1024 brings the missing callers back (impactedCount 1 -> 9). * fix(ingestion): address #1661 adversarial review follow-ups (F1/F2/F3) Three non-blocking nits flagged by the adversarial review on #1661: F1 (output stability) — skippedLargePaths was populated by concurrent fs.stat callbacks in batches of 32, so push order within a batch was completion-order rather than input-order. The default preview's "first 5" could vary across runs on the same repo. Fix: sort the array before slicing. New test asserts the verbose output is in sorted order. F2 (boundary coverage) — the preview-cap describe block created 8 large files, so the SKIPPED_PREVIEW_CAP = 5 comparison was never exercised at the exact <= boundary. A future off-by-one (<= → <) would not fail the suite. Fix: add two tests, one with exactly 5 files (all listed, no truncation) and one with exactly 6 files (5 listed plus "...and 1 more"). F3 (hint accuracy) — isDefault compared effective bytes, so an operator who explicitly set GITNEXUS_MAX_FILE_SIZE=512 (the same KB as the default) would still see the "Set GITNEXUS_MAX_FILE_SIZE=<KB>..." hint. Fix: gate the hint on whether the env var is unset, not on the resulting byte value. New test pins the explicit-default-value case. All 34 filesystem-walker tests pass (was 30; +4 new). Prettier clean, typecheck clean for the changed files. --------- Co-authored-by: scotjelinski <58397194+scotjelinski@users.noreply.github.com> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
This commit is contained in:
jelsco
GitHub
parent
b9f7b39850
commit
b61015693e
2 changed files with 190 additions and 4 deletions
|
|
@ -74,12 +74,31 @@ export const walkRepositoryPaths = async (
|
|||
|
||||
if (skippedLarge > 0) {
|
||||
const isDefault = maxFileSizeBytes === DEFAULT_MAX_FILE_SIZE_BYTES;
|
||||
const isOverrideUnset = !process.env.GITNEXUS_MAX_FILE_SIZE;
|
||||
const suffix = isDefault ? ', likely generated/vendored' : '';
|
||||
logger.warn(` Skipped ${skippedLarge} large files (>${maxFileSizeBytes / 1024}KB${suffix})`);
|
||||
if (isVerboseIngestionEnabled()) {
|
||||
for (const p of skippedLargePaths) {
|
||||
logger.warn(` - ${p}`);
|
||||
}
|
||||
|
||||
// Always show at least the first few paths so users can diagnose why
|
||||
// edges are missing from a specific file (issue #1659). The full list is
|
||||
// gated behind GITNEXUS_VERBOSE=1 to avoid flooding output on repos with
|
||||
// many generated/vendored blobs. Sort before slicing so the preview is
|
||||
// stable across runs (fs.stat callbacks race within each batch).
|
||||
skippedLargePaths.sort();
|
||||
const SKIPPED_PREVIEW_CAP = 5;
|
||||
const showAll = isVerboseIngestionEnabled() || skippedLargePaths.length <= SKIPPED_PREVIEW_CAP;
|
||||
const preview = showAll ? skippedLargePaths : skippedLargePaths.slice(0, SKIPPED_PREVIEW_CAP);
|
||||
for (const p of preview) {
|
||||
logger.warn(` - ${p}`);
|
||||
}
|
||||
if (!showAll) {
|
||||
const remaining = skippedLargePaths.length - SKIPPED_PREVIEW_CAP;
|
||||
logger.warn(` ...and ${remaining} more (set GITNEXUS_VERBOSE=1 to list them all)`);
|
||||
}
|
||||
// Only hint about the env var when the user has not set it at all. An
|
||||
// explicit GITNEXUS_MAX_FILE_SIZE=512 happens to resolve to the same
|
||||
// bytes as the default but the operator clearly already knows the knob.
|
||||
if (isDefault && isOverrideUnset) {
|
||||
logger.warn(` Set GITNEXUS_MAX_FILE_SIZE=<KB> to include files above the default cap.`);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -398,5 +398,172 @@ describe('filesystem-walker', () => {
|
|||
expect(skipWarnings.length).toBeGreaterThan(0);
|
||||
expect(String(skipWarnings[0].msg ?? '')).toContain('generated/vendored');
|
||||
});
|
||||
|
||||
// Regression: issue #1659. The skipped-paths list and the
|
||||
// GITNEXUS_MAX_FILE_SIZE hint must appear by default, otherwise users
|
||||
// see "Skipped N large files" with no actionable detail and misdiagnose
|
||||
// missing IMPORTS/CALLS edges as a resolver bug.
|
||||
it('lists the skipped path by default (not gated behind GITNEXUS_VERBOSE)', async () => {
|
||||
await walkRepositoryPaths(sizeDir);
|
||||
const pathWarnings = cap.records().filter((r) => String(r.msg ?? '').includes(BIG_FILE));
|
||||
expect(pathWarnings.length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it('emits a GITNEXUS_MAX_FILE_SIZE hint when running with the default cap', async () => {
|
||||
await walkRepositoryPaths(sizeDir);
|
||||
const hint = cap
|
||||
.records()
|
||||
.filter((r) => String(r.msg ?? '').includes('GITNEXUS_MAX_FILE_SIZE=<KB>'));
|
||||
expect(hint.length).toBe(1);
|
||||
});
|
||||
|
||||
it('omits the GITNEXUS_MAX_FILE_SIZE hint when an override is active', async () => {
|
||||
process.env.GITNEXUS_MAX_FILE_SIZE = '1';
|
||||
await walkRepositoryPaths(sizeDir);
|
||||
const hint = cap
|
||||
.records()
|
||||
.filter((r) => String(r.msg ?? '').includes('GITNEXUS_MAX_FILE_SIZE=<KB>'));
|
||||
expect(hint.length).toBe(0);
|
||||
});
|
||||
|
||||
// Edge case from the #1661 adversarial review: setting GITNEXUS_MAX_FILE_SIZE
|
||||
// to the same value as the default (512KB) used to still print the hint
|
||||
// because the byte comparison resolved to equal. The hint should care
|
||||
// about whether the operator set the env var, not what value they chose.
|
||||
it('omits the GITNEXUS_MAX_FILE_SIZE hint when the override equals the default value', async () => {
|
||||
process.env.GITNEXUS_MAX_FILE_SIZE = '512';
|
||||
await walkRepositoryPaths(sizeDir);
|
||||
const hint = cap
|
||||
.records()
|
||||
.filter((r) => String(r.msg ?? '').includes('GITNEXUS_MAX_FILE_SIZE=<KB>'));
|
||||
expect(hint.length).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('large file skip preview cap (#1659)', () => {
|
||||
let manyDir: string;
|
||||
const ORIGINAL_ENV = process.env.GITNEXUS_MAX_FILE_SIZE;
|
||||
const ORIGINAL_VERBOSE = process.env.GITNEXUS_VERBOSE;
|
||||
let cap: ReturnType<typeof _captureLogger>;
|
||||
|
||||
beforeAll(async () => {
|
||||
manyDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-walker-size-many-'));
|
||||
await fs.mkdir(path.join(manyDir, 'src'), { recursive: true });
|
||||
// 8 files >512KB so the preview-cap path (5) is exercised.
|
||||
for (let i = 0; i < 8; i++) {
|
||||
await fs.writeFile(path.join(manyDir, 'src', `big${i}.ts`), 'x'.repeat(600 * 1024));
|
||||
}
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await fs.rm(manyDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
delete process.env.GITNEXUS_MAX_FILE_SIZE;
|
||||
delete process.env.GITNEXUS_VERBOSE;
|
||||
_resetMaxFileSizeWarnings();
|
||||
cap = _captureLogger();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
if (ORIGINAL_ENV === undefined) {
|
||||
delete process.env.GITNEXUS_MAX_FILE_SIZE;
|
||||
} else {
|
||||
process.env.GITNEXUS_MAX_FILE_SIZE = ORIGINAL_ENV;
|
||||
}
|
||||
if (ORIGINAL_VERBOSE === undefined) {
|
||||
delete process.env.GITNEXUS_VERBOSE;
|
||||
} else {
|
||||
process.env.GITNEXUS_VERBOSE = ORIGINAL_VERBOSE;
|
||||
}
|
||||
cap.restore();
|
||||
});
|
||||
|
||||
it('truncates the path list to 5 and mentions GITNEXUS_VERBOSE when over the cap', async () => {
|
||||
await walkRepositoryPaths(manyDir);
|
||||
const pathLines = cap.records().filter((r) => /^\s*-\s/.test(String(r.msg ?? '')));
|
||||
expect(pathLines.length).toBe(5);
|
||||
const more = cap
|
||||
.records()
|
||||
.filter((r) => String(r.msg ?? '').includes('and 3 more (set GITNEXUS_VERBOSE=1'));
|
||||
expect(more.length).toBe(1);
|
||||
});
|
||||
|
||||
// Boundary check from the #1661 adversarial review: the SKIPPED_PREVIEW_CAP
|
||||
// comparison is `<=`, so 5 paths should list all five without a truncation
|
||||
// line and 6 paths should list exactly five plus "...and 1 more". Tested
|
||||
// explicitly so a future off-by-one refactor (`<=` → `<`) fails fast.
|
||||
it('lists all paths and omits the truncation line at exactly 5 skipped files', async () => {
|
||||
const fiveDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-walker-size-five-'));
|
||||
try {
|
||||
await fs.mkdir(path.join(fiveDir, 'src'), { recursive: true });
|
||||
for (let i = 0; i < 5; i++) {
|
||||
await fs.writeFile(path.join(fiveDir, 'src', `big${i}.ts`), 'x'.repeat(600 * 1024));
|
||||
}
|
||||
await walkRepositoryPaths(fiveDir);
|
||||
const pathLines = cap.records().filter((r) => /^\s*-\s/.test(String(r.msg ?? '')));
|
||||
expect(pathLines.length).toBe(5);
|
||||
const more = cap.records().filter((r) => String(r.msg ?? '').includes('...and '));
|
||||
expect(more.length).toBe(0);
|
||||
} finally {
|
||||
await fs.rm(fiveDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('lists exactly 5 paths plus "...and 1 more" at exactly 6 skipped files', async () => {
|
||||
const sixDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gn-walker-size-six-'));
|
||||
try {
|
||||
await fs.mkdir(path.join(sixDir, 'src'), { recursive: true });
|
||||
for (let i = 0; i < 6; i++) {
|
||||
await fs.writeFile(path.join(sixDir, 'src', `big${i}.ts`), 'x'.repeat(600 * 1024));
|
||||
}
|
||||
await walkRepositoryPaths(sixDir);
|
||||
const pathLines = cap.records().filter((r) => /^\s*-\s/.test(String(r.msg ?? '')));
|
||||
expect(pathLines.length).toBe(5);
|
||||
const more = cap
|
||||
.records()
|
||||
.filter((r) => String(r.msg ?? '').includes('and 1 more (set GITNEXUS_VERBOSE=1'));
|
||||
expect(more.length).toBe(1);
|
||||
} finally {
|
||||
await fs.rm(sixDir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('lists every skipped path when GITNEXUS_VERBOSE=1', async () => {
|
||||
process.env.GITNEXUS_VERBOSE = '1';
|
||||
await walkRepositoryPaths(manyDir);
|
||||
const pathLines = cap.records().filter((r) => /^\s*-\s/.test(String(r.msg ?? '')));
|
||||
expect(pathLines.length).toBe(8);
|
||||
const more = cap.records().filter((r) => String(r.msg ?? '').includes('and '));
|
||||
expect(more.length).toBe(0);
|
||||
});
|
||||
|
||||
// Issue #1659 follow-up (PR #1661 review): paths were pushed in fs.stat
|
||||
// completion order, so the default preview could vary between runs on
|
||||
// the same repo. The implementation sorts skippedLargePaths before
|
||||
// slicing, so the listed paths come out in sorted order, which is the
|
||||
// stable contract operators can rely on.
|
||||
it('lists skipped paths in sorted order (deterministic preview)', async () => {
|
||||
process.env.GITNEXUS_VERBOSE = '1';
|
||||
await walkRepositoryPaths(manyDir);
|
||||
const pathLines = cap
|
||||
.records()
|
||||
.map((r) => String(r.msg ?? ''))
|
||||
.filter((m) => /^\s*-\s/.test(m))
|
||||
.map((m) => m.replace(/^\s*-\s*/, ''));
|
||||
expect(pathLines).toEqual([...pathLines].sort());
|
||||
// sanity-check we actually saw all 8 of the manyDir fixture
|
||||
expect(pathLines).toEqual([
|
||||
'src/big0.ts',
|
||||
'src/big1.ts',
|
||||
'src/big2.ts',
|
||||
'src/big3.ts',
|
||||
'src/big4.ts',
|
||||
'src/big5.ts',
|
||||
'src/big6.ts',
|
||||
'src/big7.ts',
|
||||
]);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue