From b61015693ef83f0e6ddf7742dd9c47e2dfd1a945 Mon Sep 17 00:00:00 2001 From: jelsco <58397194+jelsco@users.noreply.github.com> Date: Wed, 20 May 2026 07:37:58 -0600 Subject: [PATCH] fix(ingestion): surface skipped large-file paths by default (#1659) (#1661) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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= 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=..." 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 --- .../src/core/ingestion/filesystem-walker.ts | 27 ++- .../integration/filesystem-walker.test.ts | 167 ++++++++++++++++++ 2 files changed, 190 insertions(+), 4 deletions(-) diff --git a/gitnexus/src/core/ingestion/filesystem-walker.ts b/gitnexus/src/core/ingestion/filesystem-walker.ts index 4d6725e246..c30ea43216 100644 --- a/gitnexus/src/core/ingestion/filesystem-walker.ts +++ b/gitnexus/src/core/ingestion/filesystem-walker.ts @@ -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= to include files above the default cap.`); } } diff --git a/gitnexus/test/integration/filesystem-walker.test.ts b/gitnexus/test/integration/filesystem-walker.test.ts index 2a1257ccc0..1d743a8989 100644 --- a/gitnexus/test/integration/filesystem-walker.test.ts +++ b/gitnexus/test/integration/filesystem-walker.test.ts @@ -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=')); + 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=')); + 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=')); + 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; + + 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', + ]); + }); }); });