mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-11 03:38:07 +00:00
Walks the full set of findings from a multi-agent code review (11 reviewers, 1 maintainability dispatch lost to tool-permission denial) of the PR #1693 branch. All 16 actionable findings — 4 P1, 4 P2, 8 P3 — applied in a single pass against a consistent tree. Tests pass (269/269 unit files, 29/29 integration). P1 — bounds-only / disguised-bounds assertions across 4 test files (per user-memory DoD §2.7): - worker-pool.test.ts: 5 sites — `nodes.length > 0` dropped (redundant after `.toContain('validateInput')`); `files.length >= 4` pinned to `.toBe(7)` (mini-repo/src has exactly 7 .ts files); `results.length > 0` pinned to `.toHaveLength(1)` (default sub-batch absorbs all 7); `result.fileCount >= 0` pinned to `.toBe(1)` (empty file is still "processed"); `warnRecords.length > 0` replaced with content- predicate `/respawn|dropping|replacement|did not report ready/` (catches silenced warnings); `fallbackExcludePaths.length > 0` pinned to exact `['one.ts', 'two.ts']` (deterministic given the single-slot pool + 2 items + per-item starting-file). - parse-impl-fallback.test.ts: 3 sites — `astCacheClearCalls >= 1` pinned to exact 4 (per-chunk × 2 + finally × 2); the two error-path delta checks pinned to exact +2 and +3 (verified empirically). - parse-impl-progress-monotonic.test.ts: `percents.length > 0` → `.not.toEqual([])`; per-element `Math.max(prev, cur)` tautology replaced with direct `if (cur < prev) throw`; final-percent `Math.min(last, 95)` tautology pinned to exact `.toBe(70)` (3-file skipWorkers fixture's deferred band lands at the band start). - parse-impl-large-fixture.test.ts: `Math.min(elapsedMs, BUDGET)` tautology removed; Promise.race rejection is the load-bearing wall-clock check. P1 — terminate() lacks `.catch` mask: - worker-pool.ts terminate() now matches the `.catch(() => undefined)` pattern used at every other internal terminate site. Prevents a hung/OOM worker's terminate rejection from masking the original pipeline error when called from parse-impl.ts's finally block, and guarantees `workers.length = 0` / `activeSlots.clear()` always run. P1 — hybrid envelope length-mismatch + null-payload silent data loss: - parse-worker.ts decodeIncomingMessage: explicit non-null-and-typed check before `.type` access (decodeMessage permits null payloads per encodeMessage contract); explicit length-equality assertion between `decoded.files` and `contents` before zipping. Without these, `TextDecoder.decode(undefined)` silently returns "" and produces empty-content graph nodes — a contract violation that used to be undetectable. Both throws route through the outer try/catch → worker `error` reply → pool's recoverAndResume. P1 — unsafe casts at the IPC boundary: - buildDispatchMessage now uses a properly-typed `isParseWorkerItemArray` type guard. The narrowed branch accesses `item.path` and `item.content` as statically-typed strings — a future rename of `ParseWorkerInput.content` would fail to compile inside the branch instead of silently mismatching at runtime. The remaining decodeMessage payload casts are bounded by the F3/F6 runtime guards. P2 — idle-timeout retry bypasses circuit breaker: - worker-pool.ts timeout-retry IIFE now increments `consecutiveFailuresPerSlot[workerIndex]` alongside `respawnCount`. A slot that consistently times out (vs crashes) now trips the per-slot breaker, instead of consuming its full respawn budget over potentially tens of minutes without the breaker firing. P2 — null/non-object worker message crashes pool handler: - Dispatch handler in worker-pool.ts now guards `null / non-object / no string type discriminant` before `msg.type` access and routes through recoverAndResume on violation. Previously a legitimate `null` payload would throw TypeError out of the EventEmitter listener → uncaughtException on main, crashing the analyze. P2 — workerPoolSize === 0 creates unusable pool: - parse-impl.ts now treats `workerPoolSize === 0` as `skipWorkers` at the gate. Matches the PipelineOptions docstring contract ("0 disables the pool entirely — equivalent to skipWorkers"); avoids constructing a pool that rejects every dispatch and logs "Worker pool parsing stopped" per chunk. P2 — encodeMessage 2-buffer allocation per frame: - protocol.ts encodeMessage coalesced to a single `Buffer.allocUnsafe + writeUInt8 + writeUInt32LE + buf.write (string, offset, 'utf8')`. Drops the intermediate `Buffer.from(JSON.stringify(...), 'utf8')` allocation + memcpy. Length pre-check via `Buffer.byteLength(string, 'utf8')` surfaces the uint32 cap before any allocation. P3 — slotGenerations made optional on WorkerPoolStats so external implementations of getStats() that predate U12 don't compile-break; in-repo callers already use optional chaining. P3 — buildDispatchMessage marked `@internal` so it isn't surfaced as public API by typedoc / api-extractor (it's a test-only export). P3 — verboseThroughputLog hoisted above the chunk loop (env vars can't change mid-run; one O(env-read) per analyze, not per chunk). P3 — corrected the messageerror routing comment in worker-pool.ts dispatch handler. `ProtocolDecodeError` is caught by the surrounding try/catch — distinct from `messageerror`, which fires for V8 structured-clone failures before the message body would reach the handler. P3 — initial pool spawn now uses a `Promise.allSettled` ready-handshake gate symmetric with `replaceWorker`. Dispatch awaits this gate before selecting slots, so an init-crashing initial worker is dropped from `activeSlots` and a downstream OOM/missing-native-binding failure surfaces in seconds (bounded by WORKER_READY_TIMEOUT_MS) rather than waiting for the first idle timeout (30s default). P3 — `GITNEXUS_WORKER_MAX_RESPAWNS_PER_SLOT`, `GITNEXUS_WORKER_MAX_CUMULATIVE_TIMEOUT_MS`, `GITNEXUS_WORKER_CONSECUTIVE_FAILURE_THRESHOLD` added to: - CLI `--help` text in src/cli/index.ts - Root README env-var table - gitnexus/README troubleshooting section (new "Worker pool resilience tuning" subsection) P3 — CLI `catch (e: any)` / `catch (err: any)` in analyze.ts replaced with `catch (err: unknown)` + narrowed access; matches modern TS best practice and the codebase pattern at other catch sites. P3 — `WorkerPoolStats.terminated: boolean` field added (optional, for backward compatibility). `terminate()` sets it true; `getStats()` surfaces it. Distinguishes graceful shutdown from a circuit-breaker trip in observability surfaces. Coverage / advisory items not addressed in this commit (kept in the report only): - maintainability reviewer failed (Read/Bash denied) — god-module audit on worker-pool.ts (~1400 LOC) carried as residual risk - quarantine case-sensitivity contract unpinned (adversarial #8) - WORKER_READY_TIMEOUT_MS env-configurability (adversarial #2) - chunk-byte-budget × parseChunkConcurrency memory multiplier doc (adversarial #5) - MCP discoverability gaps for env vars / verbose (agent-native W1/W2) - bench/parse-throughput.md scaffold-with-TBD-rows (PS RR-003)
132 lines
4.9 KiB
TypeScript
132 lines
4.9 KiB
TypeScript
/**
|
|
* U4 (M2) — Monotonic progress through the parse + deferred-extraction phases.
|
|
*
|
|
* Before this fix, parse-impl emitted `percent: 82` for every progress
|
|
* update during the deferred resolution stages (imports, heritage, routes,
|
|
* calls). The UI sat at 82 for the duration of the deferred work — on real
|
|
* repos, several seconds to minutes — looking exactly like a hang, which is
|
|
* the user-facing symptom PR #1693 set out to fix.
|
|
*
|
|
* After M2, parse phase covers 20-70 and deferred extraction covers 70-95
|
|
* across four labelled sub-bands. This test runs `runChunkedParseAndResolve`
|
|
* on a small temp repo via the deterministic sequential-fallback path
|
|
* (`skipWorkers: true`) and asserts the recorded percent stream is strictly
|
|
* non-decreasing AND reaches the deferred band (>=70) before returning.
|
|
*/
|
|
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
|
import fs from 'node:fs';
|
|
import os from 'node:os';
|
|
import path from 'node:path';
|
|
|
|
import { runChunkedParseAndResolve } from '../../src/core/ingestion/pipeline-phases/parse-impl.js';
|
|
import { createKnowledgeGraph } from '../../src/core/graph/graph.js';
|
|
|
|
function makeTempRepo(files: Record<string, string>): string {
|
|
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'parse-impl-progress-'));
|
|
for (const [rel, content] of Object.entries(files)) {
|
|
const abs = path.join(dir, rel);
|
|
fs.mkdirSync(path.dirname(abs), { recursive: true });
|
|
fs.writeFileSync(abs, content);
|
|
}
|
|
return dir;
|
|
}
|
|
|
|
function scanned(repo: string, files: string[]) {
|
|
return files.map((rel) => ({
|
|
path: rel,
|
|
size: fs.statSync(path.join(repo, rel)).size,
|
|
}));
|
|
}
|
|
|
|
describe('parse-impl progress monotonicity (U4 M2)', () => {
|
|
let repoPath = '';
|
|
|
|
beforeEach(() => {
|
|
repoPath = makeTempRepo({
|
|
'a.ts': `export function foo() { return 1; }\n`,
|
|
'b.ts': `import { foo } from './a';\nexport function bar() { return foo(); }\n`,
|
|
'c.ts': `import { bar } from './b';\nexport class Baz { run() { return bar(); } }\n`,
|
|
});
|
|
});
|
|
|
|
afterEach(() => {
|
|
if (repoPath && fs.existsSync(repoPath)) {
|
|
fs.rmSync(repoPath, { recursive: true, force: true });
|
|
}
|
|
});
|
|
|
|
it('emits a strictly non-decreasing percent stream and reaches the deferred band', async () => {
|
|
const graph = createKnowledgeGraph();
|
|
const files = ['a.ts', 'b.ts', 'c.ts'];
|
|
const percents: number[] = [];
|
|
|
|
await runChunkedParseAndResolve(
|
|
graph,
|
|
scanned(repoPath, files),
|
|
files,
|
|
files.length,
|
|
repoPath,
|
|
Date.now(),
|
|
(p) => {
|
|
if (typeof p.percent === 'number') percents.push(p.percent);
|
|
},
|
|
{ skipWorkers: true },
|
|
);
|
|
|
|
// The stream MUST be non-empty (a regression that stops emitting
|
|
// progress should fail this test). Express via exact-equality
|
|
// negation rather than a bound.
|
|
expect(percents).not.toEqual([]);
|
|
|
|
// Strict monotonic non-decreasing across the whole stream. Direct
|
|
// comparison — the previous `Math.max(prev, cur)` form resolved to
|
|
// `expect(cur).toBe(cur)` which is a tautology.
|
|
for (let i = 1; i < percents.length; i++) {
|
|
if (percents[i] < percents[i - 1]) {
|
|
throw new Error(
|
|
`progress regressed: percents[${i}]=${percents[i]} < percents[${i - 1}]=${percents[i - 1]}`,
|
|
);
|
|
}
|
|
}
|
|
|
|
// The parse phase advances through 20-70; the deferred extraction band
|
|
// covers 70-95. On a 3-file fixture with imports + heritage + calls,
|
|
// we should observe at least one percent value in the 70-95 band so the
|
|
// monotonic-advance behavior is exercised, not just the parse half.
|
|
const reachedDeferredBand = percents.some((p) => p >= 70 && p <= 95);
|
|
expect(reachedDeferredBand).toBe(true);
|
|
|
|
// On this 3-file fixture in skipWorkers mode the deferred band
|
|
// advances exactly to 70 (the start of the band). The orchestrator
|
|
// (run-analyze) drives 70-100 itself once cross-chunk extraction
|
|
// finishes. Pinning the exact observed value catches both an
|
|
// upper-bound regression (anything >70 would unexpectedly land in
|
|
// the band) AND a lower-bound regression (anything <70 would mean
|
|
// the parse phase didn't complete).
|
|
expect(percents[percents.length - 1]).toBe(70);
|
|
});
|
|
|
|
it('emits percent 95 (not 82) when there are no parseable files to skip past the parse band', async () => {
|
|
const graph = createKnowledgeGraph();
|
|
// No parseable files: empty scanned list, empty parseable list.
|
|
const percents: number[] = [];
|
|
|
|
await runChunkedParseAndResolve(
|
|
graph,
|
|
[],
|
|
[],
|
|
0,
|
|
repoPath,
|
|
Date.now(),
|
|
(p) => {
|
|
if (typeof p.percent === 'number') percents.push(p.percent);
|
|
},
|
|
{ skipWorkers: true },
|
|
);
|
|
|
|
// The early-return path must emit 95 (the new post-deferred ceiling),
|
|
// not the stale 82 it used before M2 — otherwise downstream phases
|
|
// would visibly regress percent on the next update.
|
|
expect(percents).toEqual([95]);
|
|
});
|
|
});
|