refactor: move safePushAll to internal utility, add call-site guard test

Address review feedback on test coverage and API surface:

- Move safePushAll from pipeline.ts to array-utils.ts so it is not
  exported from the pipeline module's public surface.
- Add a static analysis test that reads pipeline.ts source and fails
  if any chunk-accumulation array uses .push(...) instead of
  safePushAll(). This directly catches reintroduction at the call
  site, not just the helper.
- Verified: injecting a .push(...) line into pipeline.ts causes the
  test to fail with a clear message pointing to the exact line.
This commit is contained in:
Hardik Jain 2026-04-05 21:39:42 +05:30
parent 6cbd3b9a7b
commit 94ad1b7a18
3 changed files with 65 additions and 22 deletions

View file

@ -0,0 +1,11 @@
/**
* Append all elements from `source` into `target` without using the spread
* operator. `target.push(...source)` converts every element into a function
* argument which exceeds V8's call-stack limit when `source` has more than
* ~65 000 entries (common in large monoliths with many symbols).
*/
export function safePushAll<T>(target: T[], source: readonly T[]): void {
for (let i = 0; i < source.length; i++) {
target.push(source[i]);
}
}

View file

@ -1,4 +1,5 @@
import { createKnowledgeGraph } from '../graph/graph.js';
import { safePushAll } from './array-utils.js';
import { processStructure } from './structure-processor.js';
import { processMarkdown } from './markdown-processor.js';
import { processCobol, isCobolFile, isJclFile } from './cobol-processor.js';
@ -72,20 +73,6 @@ import { fileURLToPath, pathToFileURL } from 'node:url';
const isDev = process.env.NODE_ENV === 'development';
/**
* Append all elements from `source` into `target` without using the spread
* operator. `target.push(...source)` converts every element into a function
* argument which exceeds V8's call-stack limit when `source` has more than
* ~65 000 entries (common in large monoliths with many symbols).
*
* Exported for regression testing — see safe-push-all.test.ts.
*/
export function safePushAll<T>(target: T[], source: readonly T[]): void {
for (let i = 0; i < source.length; i++) {
target.push(source[i]);
}
}
const EXPO_NAV_PATTERNS = [
/router\.(push|replace|navigate)\(\s*['"`]([^'"`]+)['"`]/g,
/<Link\s+[^>]*href=\s*['"`]([^'"`]+)['"`]/g,

View file

@ -1,16 +1,21 @@
/**
* Regression test for safePushAll — the helper introduced to replace
* `array.push(...largeArray)` in the pipeline chunk-accumulation loop.
* Regression tests for the safePushAll helper and the pipeline call sites
* that must use it instead of `array.push(...largeArray)`.
*
* The spread operator converts every element into a V8 call-stack argument.
* At ~65k+ elements a single `push(...arr)` call overflows the stack.
* This test ensures safePushAll handles that size without crashing and
* will catch any future reintroduction of the spread pattern.
*
* Two layers of defense:
* 1. Unit tests — safePushAll itself works at scale.
* 2. Static analysis — pipeline.ts contains no `.push(...` in the
* chunk-accumulation section, catching future reintroductions.
*/
import { describe, it, expect } from 'vitest';
import { safePushAll } from '../../src/core/ingestion/pipeline.js';
import { safePushAll } from '../../src/core/ingestion/array-utils.js';
import fs from 'fs';
import path from 'path';
/** Size that reliably triggers the V8 stack overflow with push(...arr). */
/** Size well above V8's ~65k call-stack argument limit. */
const OVERFLOW_SIZE = 200_000;
describe('safePushAll', () => {
@ -37,12 +42,52 @@ describe('safePushAll', () => {
for (let i = 0; i < OVERFLOW_SIZE; i++) source[i] = i;
const target: number[] = [];
// This must not throw — push(...source) would RangeError here.
safePushAll(target, source);
expect(target.length).toBe(OVERFLOW_SIZE);
// Spot-check order is preserved at boundaries
expect(target[0]).toBe(0);
expect(target[OVERFLOW_SIZE - 1]).toBe(OVERFLOW_SIZE - 1);
});
});
describe('pipeline.ts has no spread-push in chunk accumulation', () => {
const pipelinePath = path.resolve(__dirname, '../../src/core/ingestion/pipeline.ts');
const source = fs.readFileSync(pipelinePath, 'utf-8');
const lines = source.split('\n');
it('does not use .push(...) on deferred accumulation arrays', () => {
// These are the arrays that accumulate per-chunk worker results.
// Any .push(...) on them will overflow on large repos.
const accumulatorNames = [
'deferredWorkerCalls',
'deferredWorkerHeritage',
'deferredConstructorBindings',
'deferredAssignments',
'workerTypeEnvBindings',
'allFetchCalls',
'allExtractedRoutes',
'allDecoratorRoutes',
'allToolDefs',
'allORMQueries',
];
const spreadPushPattern = /\.push\(\.\.\./;
const violations: string[] = [];
lines.forEach((line, idx) => {
if (spreadPushPattern.test(line)) {
const trimmed = line.trim();
// Check if the push target is one of the known accumulator arrays
if (accumulatorNames.some((name) => trimmed.startsWith(`${name}.push(`))) {
violations.push(`line ${idx + 1}: ${trimmed}`);
}
}
});
expect(
violations,
'Found .push(...) on chunk-accumulation arrays in pipeline.ts. ' +
'Use safePushAll() instead to avoid V8 stack overflow on large repos.',
).toEqual([]);
});
});