Resolves PR #1693 review B3 — the final P0/P1 merge blocker. With this
test, all five doc-review blockers (B1-B5) are pinned by regression
coverage.
The PR's headline claim is "analyze no longer hangs on TS-root-shaped
loads". The existing suite pins each resilience layer (worker-pool-
resilience.test.ts), the deferred-extraction equivalence (U7), and
the chunk-concurrency contract (U1). What was missing: a single
end-to-end run that exercises the full chunked parse-and-resolve
path on a multi-chunk fixture, BOUNDED by a wall-clock budget so a
regression that re-introduces the hang fails this test loudly via
timeout rather than slipping past as a count drift.
Implementation:
- 17-file synthetic fixture: 15 small modules (one function each),
one "realistic dense" complex.ts (30 functions + class + interface),
and an index.ts re-exporting them. Forces cross-chunk import
chains.
- GITNEXUS_CHUNK_BYTE_BUDGET=64 via vi.resetModules forces multi-chunk
parsing on the small fixture.
- Promise.race with 30s timeout: a hang fails as
"exceeded WALL_CLOCK_BUDGET_MS — likely the hang B3 was meant to
prevent", not as a bounds-only inequality (DoD §2.7 distinction —
hang-detector via exception, not regression-mask via inequality).
- Exact .toBe(true) assertions on specific expected symbols
(fn0..fn14, Service, Config, configure, describe, complex0/15/29)
so a silent mid-chunk crash that exits 0 without producing graph
data also fails this test, not just the hang case.
Scope: runs the sequential-fallback path (skipWorkers: true) because
the full real-worker scenario requires a built dist/parse-worker.js
and ~60s wall-clock per run — appropriate for a CI-integration job,
not vitest. The load-bearing invariants pinned here catch the bulk
of B3's concern; the dist-worker swap is a Phase 2 follow-up
documented in the file header.
Resolves PR #1693 review B4: the deferred-extraction reorder (moving
processImportsFromExtracted / Heritage / Routes / Wildcard /
ReceiverTypes from per-chunk to end-of-loop) was proven observably
equivalent by Lane 4 of the production-readiness review. Until now,
the existing suite never asserted cross-chunk graph equivalence,
which lets a future refactor that accidentally tightens the per-chunk
vs end-of-loop coupling silently break cross-chunk resolution.
This test forces multi-chunk parsing on a small fixture by setting
GITNEXUS_CHUNK_BYTE_BUDGET=64 BEFORE the parse-impl module loads
(the budget is captured at module load via vi.resetModules — a future
move to function-scope env reads is U14 in Phase 2). Then runs the
same fixture under a 10MB budget (single chunk) and asserts the two
graphs are byte-identical: same nodeCount, same relationshipCount,
exact .toBe(N) per DoD §2.7.
Fixture: 3-file class hierarchy with cross-file inheritance — Animal
(a.ts) -> Dog extends Animal (b.ts) -> makeDog returns Dog (c.ts).
Forces the resolver to chain imports + heritage across chunks. A
second test pins specific symbol names (Animal, Dog, makeDog, speak,
bark) in the multi-chunk graph so a regression in chunk-boundary
resolution surfaces as a missing-symbol failure with a specific
diagnostic instead of a bare count mismatch.
Resolves PR #1693 review B5: the captures.ts ancestor-walk rewrite
(findSelfOrAncestorOfType[s] + pickFirstNode replacing the prior
findNodeAtRange-from-root path) was semantically equivalent to its
predecessor per Lane 4 of the production-readiness review, but the
existing typescript-captures.test.ts didn't pin the specific sharp
edges where an over-aggressive walk would silently break captures.
This file does.
Each test exercises a capture class whose anchor type is one the
rewrite explicitly handles:
- member call obj.foo() -> @reference.call.member (call_expression
anchor walks to self)
- dynamic import import("./helper") -> raw @import.dynamic gets
decomposed by splitImportStatement into @import.statement with
@import.kind=dynamic + @import.source stripped of quotes
- JSX <Foo /> in .tsx -> @reference.call.free emitted (TSX query
pattern, query.ts:899-905) but @declaration.parameter-count is
NOT synthesized because findSelfOrAncestorOfType('call_expression')
returns null on a jsx_self_closing_element anchor. Pre-rewrite the
range lookup also returned null. Pinning this contract catches
accidental "walk JSX -> outer call" refactors.
- constructor `new Foo(1,2)` -> @reference.call.constructor (new_expression
anchor walks to self)
- named/namespace import + re-export -> @import.statement (one each)
- class method override -> @declaration.method per class, no collapse
- member read obj.foo (no call) -> @reference.read.member
All assertions use exact .toBe(N) per DoD §2.7.
Resolves PR #1693 review M5 (Windows quarantine path-normalization
coverage). worker-pool.ts quarantines paths via a Set<string> keyed by
exact string equality. The existing suite never asserted this contract,
which lets a future "helpfully normalizing" refactor on one side of the
pipeline (caller, worker, or pool) silently break quarantine filtering
on Windows.
This file pins the contract from both directions:
1. Round-trip: a path the caller dispatches with backslashes
(src\bad.ts) flows through starting-file -> death -> quarantine ->
next-dispatch filter verbatim. The replacement worker never sees the
re-dispatched bad path because the pool's pre-dispatch filter
short-circuits it.
2. Non-normalization: quarantining src\poison.ts does NOT filter
src/poison.ts. Whoever changes that contract has to update this test
alongside (the load-bearing assertion catches accidental
path.normalize() calls in the quarantine path).
Runs on every platform — the path strings are test-injected, so the
test exercises the same code path regardless of the host's path.sep.
Used a self-contained FakeWorker that emits {type:'ready'} for U3's
waitForWorkerReady handshake, so the test doesn't depend on the larger
worker-pool-resilience.test.ts harness.
Resolves PR #1693 review L6: operator-facing env vars were either
mentioned inline (GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS) or only
documented via `gitnexus --help`, with no single place to look up
the full set. The new "Environment variables" subsection under the
Quick Start CLI block lists every operator-facing knob with default,
effect, and tuning guidance, matching the names in cli/index.ts
addHelpText post-U2 / U1.
Covers:
GITNEXUS_WORKER_POOL_SIZE (--workers)
GITNEXUS_PARSE_CHUNK_CONCURRENCY (newly real per U1)
GITNEXUS_VERBOSE (--verbose)
GITNEXUS_MAX_FILE_SIZE (--max-file-size)
GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS (--worker-timeout × 1000)
GITNEXUS_WORKER_SUB_BATCH_MAX_BYTES
GITNEXUS_CHUNK_BYTE_BUDGET
GITNEXUS_NO_GITIGNORE
GITNEXUS_SKIP_OPTIONAL_GRAMMARS
CLI flag vs env-var precedence is stated explicitly (CLI > env > default)
so operators running long-lived hosts (MCP server, eval-server) know
which channel wins.
Resolves PR #1693 review M6: the existing resilience suite asserts only
the *default value* of maxCumulativeTimeoutMs (5x subBatchIdleTimeoutMs),
not that dispatch actually aborts the offending job when the cumulative
wall-clock budget is exhausted. Without this test, a future refactor
could remove the exhaustion branch in requeueAfterTimeout and the suite
would stay green while the pool sat in retry loops for an hour on a
real production stall.
Scenario:
subBatchIdleTimeoutMs = 100ms
timeoutBackoffFactor = 10
maxCumulativeTimeoutMs = 300ms
Single file, HangingWorker that never responds. First attempt times
out at 100ms (cumulative=100). The next backoff (1000ms, cumulative
1100ms) exceeds the 300ms cap, so requeueAfterTimeout returns
give-up on the first timeout retry and the file goes to the session
quarantine. Asserts:
- pool.getQuarantinedPaths() includes 'src/stuck.ts' after dispatch
- if dispatch rejected, the error is a WorkerPoolDispatchError
(the typed surface that routes to sequential fallback)
Uses a local minimal HangingWorker double rather than the full
action-scripted FakeWorker from worker-pool-resilience.test.ts —
the inverse pattern (always hang) doesn't need the scripted-action
machinery and keeps the test file focused on the one behavior.
Resolves PR #1693 review B1 (GITNEXUS_PARSE_CHUNK_CONCURRENCY documented
in --help but unimplemented).
The chunk loop now pre-fetches chunk file contents up to
`parseChunkConcurrency` chunks ahead of the worker-dispatch cursor so
disk I/O overlaps with worker compute. Worker dispatch itself stays
serial because WorkerPool.dispatch is not reentrant — concurrent calls
would race on the shared per-slot busy/in-flight state, regressing the
hang/resilience work this PR is built on. The pre-fetch path is the
honest interpretation of "concurrent in-flight parse chunks" that the
help text advertises: I/O overlap, not parallel worker dispatch.
Concurrency value resolution:
1. PipelineOptions.parseChunkConcurrency (threaded from CLI)
2. GITNEXUS_PARSE_CHUNK_CONCURRENCY env var
3. Default 2 (matches the help text)
F4 (wildcard-synthesis ordering) is preserved: deferred-state
aggregation runs in chunkIdx order because the for-loop iterates
sequentially after awaiting each chunk's pre-fetched contents.
Cross-chunk processors (processImportsFromExtracted,
synthesizeWildcardImportBindings, etc.) still run only after all
chunks complete — they see deterministic input regardless of
file-read completion order.
Concurrency=1 produces behavior identical to the pure-serial loop;
that's the regression baseline.
New test: parse-impl-chunk-concurrency.test.ts
- Asserts graph output is identical (nodeCount + relationshipCount)
between parseChunkConcurrency=1 and =2 — the critical correctness
invariant. Exact .toBe(N) comparisons per DoD §2.7 (the second run's
counts must equal the first run's exactly).
- Pins specific fixture symbols (foo/bar/Baz) under both
parseChunkConcurrency=1 and the env-fallback (3) path.
- Env-fallback test confirms GITNEXUS_PARSE_CHUNK_CONCURRENCY is
honored when the option is undefined.
Resolves PR #1693 review M2, M3, L1, L5 in a single parse-impl.ts pass:
M2 - Monotonic progress through deferred phase (no more "stuck at 82%")
Previously the deferred resolution stages (imports, heritage, routes,
calls) all emitted percent: 82 — the UI looked frozen for the duration
of the deferred work, which on large repos is several seconds to minutes
and visually identical to the hang PR #1693 set out to fix.
Redistributed:
parse phase: 20-70 (was 20-82)
imports: 70-75
heritage: 75-80
routes: 80-85
calls: 85-95
Each deferred stage now advances through its own band via the existing
per-batch progress callback. Skipped stages (zero deferred input) leave
their band as a no-op jump - the next stage still starts at its own
band, preserving strict monotonicity. The "no parseable files" early
return now jumps to 95 (was 82), and the duplicate "Parsing N files..."
announcement is suppressed when totalParseable === 0 to avoid a
non-monotonic 95 -> 20 regression that pre-existed (uncovered by the
new monotonic test).
M3 - Throughput log gated on `--verbose`, not just NODE_ENV=development
The per-chunk files/s log was gated on `isDev`, so operators running
`gitnexus analyze --verbose` in a production install never saw it.
Now fires when (isDev || isVerboseIngestionEnabled()) — matches the
documented promise that `--verbose` shows tuning observability.
L1 - Typo rename: `chunkChunkStartMs` -> `chunkStartMs`
L5 - `buildExportedTypeMapFromGraph` runs BEFORE `seedCrossFileReceiverTypes`
Previously the seeding branch was reached with `exportedTypeMap.size === 0`
in the worker path (the map was only built far below, AFTER the seeding
branch), so the seed dead-coded itself silently and call resolution
never got the cross-file receiver-type enrichment. Now the map is
populated from the in-progress graph before the seed call; the
post-parse builder remains as a defensive sequential-path fallback,
guarded by `size === 0` so we don't pay the cost twice on the worker
path. Net win: cross-file CALLS edges that previously had no receiver
type now get enriched.
New test: parse-impl-progress-monotonic.test.ts
Asserts the emitted percent stream is strictly non-decreasing across
the parse + deferred phases, and that the deferred band (>=70) is
actually reached. Also pins the "no parseable files" path to exactly
[95] so the 95 -> 20 regression we just fixed can't re-emerge.
Resolves PR #1693 review H1, H2, M4:
H1 - messageerror handler at every dispatch site
V8 deserialization failure on postMessage previously left the message
silently lost; the pool would wait out the idle timeout (default 30s)
instead of treating it as worker death. The dispatch loop now wires
worker.once('messageerror', ...) alongside error/exit and routes through
recoverAndResume so the existing per-slot respawn budget, in-flight
file attribution, and circuit-breaker layers fire as designed.
H2 - resolveAutoPoolSize uses os.availableParallelism()
Mirrors the pattern at capabilities.ts:85 (defaultEmbeddingThreads).
os.cpus().length returns the host CPU count, which over-sizes the pool
on cgroup-limited containers, taskset-restricted runtimes, and CI
runners with explicit CPU quotas. Falls back to os.cpus().length on
Node < 18.14.
M4 - worker-side ready handshake replaces online-trust
parse-worker.ts now emits {type: 'ready'} after all top-of-script
initialization completes, BEFORE the message handler is attached. The
pool's renamed waitForWorkerReady listens for this message under a
bounded WORKER_READY_TIMEOUT_MS (5s) budget instead of trusting Node's
online event - which fires when the worker thread starts, BEFORE the
script body runs, letting init crashes slip past pool startup. ready
is added to WorkerOutgoingMessage with an exhaustiveness-checked
no-op branch in the dispatch handler (defensive: the message is
consumed by waitForWorkerReady before dispatch handlers attach).
messageerror is wired into waitForWorkerReady the same way.
Test scaffolding:
- FakeWorker emits {type: 'ready'} in addition to 'online' so
replacement workers in unit tests don't hit the 5s budget.
- Integration test ad-hoc worker scripts go through a writeReadyWorker
helper that prepends the ready handshake. Tests intending to script
"crash BEFORE ready" can bypass the helper.
61/61 worker-pool unit tests pass; 28/28 integration tests pass.
Resolves PR #1693 review B2 (env-var leak in long-running hosts):
- --workers is now threaded through AnalyzeOptions -> runFullAnalysis
-> PipelineOptions.workerPoolSize -> createWorkerPool's explicit
poolSize arg, bypassing the GITNEXUS_WORKER_POOL_SIZE env channel.
The env var remains as a back-compat fallback inside resolveAutoPoolSize
for operators who set it directly.
- analyzeCommand and wikiCommand snapshot the GITNEXUS_* env vars they
mutate at function entry and restore them in finally. Inner *Impl
extraction keeps the diff surgical (no body re-indent). process.exit(0)
on the CLI success path still terminates the process; restoration
matters for programmatic callers (tests, long-running hosts) reaching
early-return paths or the alreadyUpToDate fast path.
- Tests updated to assert the new behavior:
analyze-worker-pool-size.test.ts: workerPoolSize flows through
runFullAnalysis options; env is not mutated; back-to-back calls
see their own values, not the previous call's leak.
analyze-worker-timeout.test.ts: env IS set during the runFullAnalysis
call (captured via mockImplementation) and restored after, proving
the timeout reaches downstream while the leak fix holds.
- Also addresses L4: afterEach NODE_OPTIONS restore so back-to-back test
runs don't accumulate --max-old-space-size=8192 tokens.
Addresses PR #1693 review B2 (blocker) and L4 (test polish).
- Add afterEach to worker-pool-resilience.test.ts cleaning up the per-test temp
directory created by beforeEach (~25 stale dirs per CI run previously).
- Delete the duplicated describe('worker pool option resolution', ...) block.
Verified the first block (lines 490-532) is a strict superset (includes the
GITNEXUS_WORKER_MAX_CUMULATIVE_TIMEOUT_MS env test the second block omitted),
so deletion loses no test coverage.
Addresses PR #1693 review findings L2 (temp-dir leak) and L3 (duplicate block).
* fix(detect-changes): guard resolveWorktreeCwd against overriding a separately-indexed worktree
When the repo registry entry points to a linked worktree (both main
checkout and worktree indexed separately), resolveWorktreeCwd was
incorrectly replacing the correct worktree repoPath with the server's
main-checkout launch directory. Both share the same canonical root so
the existing same-repo check passed, causing git diff to run from the
wrong directory and return 0 changes (issue #1659).
Fix: early-exit guard — if tryRealpath(repoPath) differs from
tryRealpath(getCanonicalRepoRoot(repoPath)), repoPath is itself a
linked worktree and is returned unchanged. Auto-detection only fires
when repoPath equals the canonical main-checkout root.
Also normalises the launchCanonical comparison in the auto-detect path
to use tryRealpath for cross-platform consistency.
Regression test: 'returns worktreeDir unchanged when repoPath IS a
linked worktree and launchCwd is the main checkout'.
* test(detect-changes): add worktreeA→worktreeB case and assumption comment
Cover the missing case from the production-readiness review:
repoPath = wt-A (indexed), launchCwd = wt-B (server on a different
linked worktree). The guard fires on repoPath being a worktree
regardless of launchCwd, so wt-A is returned unchanged.
Also add an inline comment documenting the assumption that repoPath
is a git root or linked-worktree root (not an arbitrary subdirectory),
as noted in Finding 2 of the review.
* refactor(detect-changes): validate repoPath is a git root before canonical comparison
Instead of relying on a comment asserting repoPath is always a git
root, call getGitRoot(repoPath) first. Only if the result matches
repoPath itself do we call getCanonicalRepoRoot and apply the guard.
This eliminates the over-classification risk for subdirectory repoPath
values and makes the assumption explicit in code. repoCanonical is
shared across both the guard and the auto-detect block.
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
User reported 4-5% CPU utilization on a multi-core machine during
ingestion. Two structural reasons:
1. **Pool cap.** `createWorkerPool` resolved size as
`Math.min(8, max(1, os.cpus().length - 1))` — a 16-core box got 8
workers (50% theoretical max). U1 lifts the default to
`min(16, max(1, cores - 1))`, exposes `GITNEXUS_WORKER_POOL_SIZE`
env override, and adds `--workers <N>` CLI flag (`0` disables the
pool for sequential fallback).
2. **Per-chunk extraction serialized the loop.** Per chunk:
dispatch → await workers → main-thread `processImportsFromExtracted`
+ `processHeritageFromExtracted` + `processRoutesFromExtracted`
+ `synthesizeWildcardImportBindings` + `seedCrossFileReceiverTypes`
→ next chunk dispatch. Workers sat idle through every extraction
block. U2 (revised from the plan's pipelined-chunks design) defers
these passes to a single end-of-loop batch. Chunk loop becomes
parse + merge + accumulate. Resolution sees strictly-more-info
(full repo graph) so cross-chunk import/heritage targets resolve at
least as well as before. Memory cost: `deferredWorkerImports`
accumulates across chunks; bounded by total file count, acceptable.
Plan deviation note: the plan called for an in-flight chunk pipeline
(N concurrent dispatches with bounded memory). That design needed
either a `processParsing` API refactor or duplicating its catch-block
fallback in `parse-impl`. The deferred-extraction approach delivers
the same "workers stay busy" outcome with much smaller surface area
and zero changes to `processParsing`. The `GITNEXUS_PARSE_CHUNK_CONCURRENCY`
env var documented in U2 of the plan is therefore not implemented in
this commit; if memory growth from `deferredWorkerImports` becomes
a problem at very-large-repo scale, a bounded sliding-window variant
can land as a follow-up.
Tests:
- New `test/unit/analyze-worker-pool-size.test.ts` covers --workers
validation (5 invalid inputs rejected with exit code 1 + clear
error; valid integers set the env var; `--workers 0` routes to
sequential).
- Extended `worker-pool-resilience.test.ts` with `resolveAutoPoolSize`
scenarios: env override, env=0, env above cap, invalid env fallback,
auto-formula match, integer return type.
- Full unit suite: 6097 / 6127 passed / 30 skipped / 0 failed.
- Full integration suite (second run): 77 / 78 passed / 1 skipped /
0 failed. First run had a known cosmetic flake from an uncaught
worker exception bleeding into the test reporter.
Resilience contract from PR #1693 preserved: per-slot respawn budget,
circuit breaker, quarantine, authoritative in-flight tracking,
cumulative timeout budget — all unchanged.
New env vars surfaced in --help: GITNEXUS_WORKER_POOL_SIZE,
GITNEXUS_PARSE_CHUNK_CONCURRENCY (reserved for future bounded
pipelining).
* fix(lbug): probe-then-load FTS extension on Windows (#1690)
The Windows skip-on-process.platform==='win32' guard in pool-adapter.ts
hard-skipped loadFTSExtension() for every Windows host, even when the
FTS extension binary was already present locally at
~/.lbdb/extension/<version>/win_amd64/fts/libfts.lbug_extension.
That left BM25 silently degraded on Windows hosts that had a working
extension on disk, with no error path — `gitnexus doctor` still reported
FTS as available, but query returned 0 BM25 hits.
This patch adds hasLocalWinFtsExtension() which probes
~/.lbdb/extension/*/win_amd64/fts/ before the Windows skip. When a binary
is on disk we call loadFTSExtension(..., { policy: 'load-only' }); the
crashing install path documented in #1199 / #1217 is never exercised at
query time, and LadybugDB's version-specific resolution combined with
the ExtensionManager's tryLoad try/catch handles stale or zero-byte
sibling version dirs cleanly (no dlopen attempted on a stale binary).
When no binary is on disk at all, we fall back to the upstream skip so
install-time SIGSEGV continues to be avoided.
Verified on Windows 10 + Node 22.19.0 + gitnexus 1.6.5 +
@ladybugdb/core 0.16.1 with the FTS extension cached at 0.16.0:
* BM25 timing goes from 0 → ~250-326ms on previously-zero queries
* gitnexus context / impact / cypher unaffected
* Adversarial-mixed-state run (real 0.16.0 binary + zero-byte stubs at
0.15.0, 0.16.1, 0.17.0): exits 0, no SIGSEGV, FTS resolves to the
real 0.16.0 binary, BM25 returns real hits
* Stub-only state at the resolution path (0.16.0, zero-byte): exits 0,
emits "FTS extension unavailable; load-only policy: extension not
pre-installed", FTS marked unavailable cleanly via markUnavailable
in extension-loader.ts — no silent greenlight
Closes#1690
* test(lbug): cover hasLocalWinFtsExtension probe + format pool-adapter
- Export hasLocalWinFtsExtension and add lbug-pool-win-fts-probe.test.ts
with 7 cases against a real tmpdir + os.homedir spy:
* missing ~/.lbdb/extension dir -> false
* extension root present but no version dirs -> false
* one version dir with binary present -> true
* zero-byte stub at probe path -> true (LOAD failure handled downstream)
* multi-version with binary only in a non-first dir -> true
* multi-version with no binary anywhere (Nix/Bazel/MDM tree) -> false
* fs.readdir throws (EACCES) -> false
The Windows conditional in doInitLbug / initLbugWithDb is intentionally
not unit-isolated: it reduces to `probe ? load : true` over a fully
constructed lbug.Database + Connection pool, which the
test/integration/lbug-pool*.test.ts suites already exercise on the
windows-latest CI matrix.
- Apply prettier format to the fs.stat() call in pool-adapter.ts,
resolving the quality/format CI failure surfaced by gitnexus/autofix.
Addresses DoD §2.7 test-coverage blocker raised in the production-
readiness review on #1692, and the dir-exists-no-file regression case
raised on #1690.
Refs #1690.
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Adds 6 new real-worker integration tests covering the PR #1693
resilience layers + fixes 3 follow-on bugs surfaced while writing them.
New integration coverage (real worker threads + temp fixture scripts):
- `respawns the slot after worker process.exit and finishes the work on
the replacement` — exercises Layer 1 auto-respawn + Layer 3 quarantine
through real IPC.
- `attributes exactly via authoritative starting-file message on worker
crash` — Layer 4 end-to-end: starting-file message → exact quarantine
attribution (not the items[0] heuristic).
- `quarantine filters subsequent dispatches without sending to a worker`
— second dispatch's sub-batch payload audited via filesystem; the
quarantined path is never sent across the message channel.
- `drops a slot after maxRespawnsPerSlot and continues on the survivor`
— 2-slot pool, slot dies twice past budget, survivor finishes
re-queued remainder.
- `trips the circuit breaker on cascading per-slot consecutive failures`
— single-slot pool, dies on every job, breaker trips after
consecutiveFailureThreshold with WorkerPoolDispatchError carrying
the cumulative quarantine.
- `survives a worker error event (uncaught throw) the same as a
process.exit` — validates recoverAndResume on the errorHandler path
via a real worker `throw` (not just process.exit).
Bug fixes uncovered while writing these tests:
1. **Stack-overflow recursion in runWorker's no-worker branch** —
`if (!worker) { ...; wakeIdleSlots(); maybeDone(); }` recursed
indefinitely when multiple slots were mid-respawn simultaneously
(wakeIdleSlots → runWorker → no worker → wakeIdleSlots → …).
Removed the wakeIdleSlots call: the slot's own respawn IIFE owns
runWorker post-respawn, and other slots will pick up work via
finishJob's runWorker.
2. **requeueAfterTimeout dispatched work before respawn completed** —
the F2 fix had `requeueAfterTimeout` `void`-discarding
`handleWorkerDeath`, so the `!shouldContinue` IIFE had no way to
know when the respawn finished. New design: `requeueAfterTimeout`
returns a `TimeoutDecision` discriminated union; the IIFE owns
the death-and-respawn-and-dispatch orchestration in an async
closure so it can `await handleWorkerDeath` and then call
`runWorker` deterministically.
3. **Stalled-singleton + protocol-error + replacement-startup-crash
tests** had stale contracts predating the resilience refactor. The
stalled-singleton no longer rejects (it quarantines + resolves
`[]`); the protocol-error rejection message now mentions
"circuit breaker tripped"; the replacement-startup-crash test
documents the known `waitForWorkerOnline` race (online fires
before the worker's main script runs, so a top-level throw looks
like a successful spawn) — the test asserts the file is
quarantined via the second-idle-timeout give-up path.
Full suite: 334 files / 8982 passed / 43 skipped / 0 failed (second
run; first run had a Vitest-reported flake from an uncaught worker
exception bleeding into the test report — repeated runs are clean).
Walks through every finding from ce-code-review run
20260519-094648-3549cf5e. All 12 picked Apply.
Critical:
- F1 — Layer 5 cumulative-timeout exhaustion no longer silently drops
the rest of the job. `requeueRemainder` is now invoked before
`handleWorkerDeath` in both Layer 5 and singleton-final-fail give-up
paths so non-quarantined items get re-tried by another worker.
- F2 — idle-timer recovery overhaul. `!shouldContinue` branch no
longer calls `replaceWorker` (double-spawn race with the
`handleWorkerDeath` inside `requeueAfterTimeout`). `shouldContinue`
branch now enforces `maxRespawnsPerSlot` before respawning, closing
the budget-bypass for the timeout-retry path. Also fixes premature
`maybeDone` by simplifying the bookkeeping.
- F3 — `requeueRemainder` no longer pre-charges `cumulativeTimeoutMs`
by `job.timeoutMs`. The death itself consumed no budget, so the
next `requeueAfterTimeout` was double-billing the first attempt.
- F4 — `WorkerPool.getQuarantinedPaths` is now optional on the
interface, matching the defensive `?.()` call site and the existing
mocks. Removes the contract-vs-callsite contradiction.
- F5 — per-job unattributed-death tracking. When a worker dies with
no exclusion attribution, `requeueRemainder` tracks death count per
`startIndex`. First time: re-queue intact. Second time: quarantine
items[0] as best guess, or drop the job entirely when items lack
paths. Bounds the death loop the original design admitted to.
- F6 — per-slot consecutive-failure counter. Replaces the pool-wide
scalar so a chronically-failing slot trips the breaker on its own
streak instead of being masked by another slot's successes.
Smaller:
- F7 — exhaustiveness `never` check on `WorkerOutgoingMessage` union.
- F8 — recursive `runWorker` on fully-quarantined jobs converted to
a while-loop.
- F9 — `tripBreaker` calls `reject(err)` BEFORE awaiting
`worker.terminate()`. A stuck terminate no longer blocks the caller.
- F10 — `parsing-processor.ts` quarantine log de-duplicates per pool
instance via a `WeakMap`. Only newly-quarantined paths are logged
in each chunk; the per-chunk count still surfaces via progress.
- F11 — extract `firstPath` local in `requeueAfterTimeout`; eliminates
double `itemPath` call and the `unknown as string` cast.
Tests (F12, 6 new):
- crash-error event path (errorHandler).
- F5 drop-branch coverage via items without `.path`.
- Common-case unattributable crash falling back to items[0] heuristic.
- `replaceWorker` startup failure (workerFactory emits 'exit' before
'online').
- All-slots-dropped breaker trip.
- `GITNEXUS_WORKER_MAX_CUMULATIVE_TIMEOUT_MS` env override.
Residual gap (deferred): no unit test exercises the Layer 5
cumulative-budget runtime path — requires fake-timer interleaving
with FakeWorker that's too brittle for this iteration. Tracked.
Unit suite: 257 files / 6056 passed / 30 skipped / 0 failed.
Restructures `createWorkerPool` so a single bad file no longer kills the
pool for the rest of an analyze run. Five interlocking layers:
1. **Auto-respawn on error/exit** — worker death triggers `replaceWorker`
on the same slot, bounded by `maxRespawnsPerSlot` (default 3). The slot
is dropped from rotation when the budget is exhausted; other slots
keep running.
2. **Circuit breaker** — replaces the permanent `poolBroken=true` with a
consecutive-failure counter. The pool only trips after
`consecutiveFailureThreshold` deaths (default `max(3, poolSize)`) with
no successful job in between. A successful job resets the counter so
transient bursts of bad files don't escalate.
3. **Session-scoped file quarantine** — paths identified as the in-flight
file at the moment of a worker death are added to a `Set<string>` on
the pool. `dispatch()` filters quarantined items up front (they never
reach a worker again this pool lifetime). Exposed via the new
`WorkerPool.getQuarantinedPaths()` so callers can log/route them.
`processParsing` surfaces the per-chunk quarantine summary alongside
the existing fallback-exclusion log.
4. **Authoritative in-flight tracking** — `parse-worker.ts` emits
`{type:'starting-file', path}` before each file. The pool tracks this
per slot and uses it for crash attribution, falling back to the
`items[lastProgress]` heuristic only when no starting-file has been
observed (very-early crash, older worker build). Closes the
reorder/race concerns raised by reviewers C1 and R3 in the earlier
review run.
5. **Per-job cumulative timeout budget** — each `WorkerJob` tracks the
total wall time spent across attempts/splits/retries. When the budget
is exhausted (default 5x `subBatchIdleTimeoutMs`), the pool surfaces
the in-flight path instead of letting exponential backoff balloon
into multi-hour stalls.
Cross-layer wiring: a new `wakeIdleSlots` helper kicks any non-busy live
slot when items are requeued (after a death or split-retry), so a dropped
slot doesn't strand work in the queue. `recoverAndResume` consolidates
the per-job teardown shared by the three in-pool death sites (`error`,
`exit`, msg-channel `error`).
New env knobs: `GITNEXUS_WORKER_MAX_RESPAWNS_PER_SLOT`,
`GITNEXUS_WORKER_MAX_CUMULATIVE_TIMEOUT_MS`,
`GITNEXUS_WORKER_CONSECUTIVE_FAILURE_THRESHOLD`.
New `WorkerPoolOptions.workerFactory` injection point for unit tests.
Tests: 12 new unit tests using a FakeWorker mock cover quarantine
seeding, slot-respawn, slot-drop after budget, breaker trip + reset,
and quarantine filtering. Plus option-resolution tests for the three
new env vars. All 19 worker-pool/-fallback/-options tests pass; full
unit suite 6040 passed / 30 skipped / 0 failed.
- Use cause-neutral "worker-excluded" label in skip messages and tests now
that worker error/exit paths share the same exclusion contract as
singleton-timeout (correctness + maintainability reviewers).
- Add JSDoc to findSelfOrAncestorOfType{s} explaining the parent-walk
short-circuit vs root-DFS fallback (maintainability reviewer).
WorkerPoolDispatchError previously surfaced the stalled path only for the
singleton-timeout final-fail branch. Worker `error` and `exit` events (and
the msg-channel `error` reply) fell back to plain `Error`, so the sequential
fallback re-attempted every file in the active job — re-hanging on the same
pathological file when the worker crashed mid-parse.
Lift the in-flight-file inference into `inFlightExcludePath(job, lastProgress)`
and wire it into the three remaining in-pool failure sites. `lastProgress` is
already in `runWorker` scope, so `items[lastProgress]` (the next file the
worker was about to acknowledge) is the best single guess at the culprit;
earlier files are still re-tried sequentially. Returns `[]` when no path is
determinable (`lastProgress >= items.length`, or path missing/non-string) so
sequential retries the whole job.
Replacement-worker startup failures stay plain `Error` (no job context); the
result-before-flush protocol bug stays plain `Error` (code fault, not file).
Tests cover the three new exclusion paths plus a negative test confirming
non-WorkerPoolDispatchError throws fall through to full sequential retry.
* feat(eval-server): added --host for user configured host IP instead of system hardcoded IP (127.0.0.1)
* fix(eval-server): localhost value in --host now returns 127.0.0.1 instead of the raw input to fix wrong address, handled error for ipv6 disabled containers
* feat(eval-server): add --host flag with validation and error handling
Co-Authored-By: Val Vladescu <val.vladescu@thirdbridge.com>
* fix(eval-server): bracketed IPv6 addresses to remove ambiguity
* docs(eval-server): document --host flag, READY signal format, and parser migration note
* fix(eval-server): use actual bound port in READY signal; strengthen --host e2e tests
Co-Authored-By: Val Vladescu <val.vladescu@thirdbridge.com>
* feat(eval): wire eval-server --host through gitnexus_docker.py
* docs(eval): added guidance for docker user
* docs(eval): revise the imprecise documentation
* fix(e2e): updated original stdout for new format
* perf(scope-resolution): use owner-keyed lookup for Step 2 member resolution (#1656)
* chore(autofix): apply prettier + eslint fixes via /autofix command
* fix(scope-resolution): index Const/Static in FieldRegistry for Step 2 lookup
Extend FieldRegistry to hold multiple defs per (owner, name), reconcile Const and Static into the owner-keyed index, and wire lookupAllByOwner through the production hook so Step 2 does not drop field kinds the registry never indexed. Pass explicitReceiver on read/write reference sites and document undefined-vs-empty hook semantics for defs fallback.
Co-authored-by: Cursor <cursoragent@cursor.com>
* perf(scope-resolution): centralize O(1) owned-member hook and guard hot path
Extract lookupOwnedMembersByOwner for the production Step 2 hook so merges stay O(1) per registry with no defs.byId scan. Add a perf-contract unit test that throws if byId.values runs when the hook is wired. Reuse a frozen empty sentinel on double miss to avoid per-probe allocations.
Co-authored-by: Cursor <cursoragent@cursor.com>
* chore: drop unused buildFieldRegistry import
* chore(scope-resolution): apply ce-code-review safe_auto fixes
- Drop unreachable return + unused values() capture in perf-contract trap (Finding #7)
- Type lookupOwnedMembersByOwner ownerDefId as DefId (Finding #9)
- Add Static-kind Step 2 lookup test mirroring the Const case (Finding #11)
* docs(field-registry): document lookupFieldByOwner first-wins semantics
Audit of all 6 production callers (call-processor.ts:2279, walkers.ts:535,
receiver-bound-calls.ts:380+730, type-env.ts:627+631) confirms none depends
on last-wins precedence — all treat the return as a generic 'field with
this name owned by this class'. Clarify the JSDoc to surface the semantic
change introduced when FieldRegistry moved from last-wins to append-order
storage (ce-code-review finding #2).
* test(scope-resolution): extend Step 2 perf contract to implicit-self, MRO, field paths
Adds three sibling tests under the Step 2 perf contract describe block, each
asserting defs.byId.values() does NOT execute when ownedMembersByOwner is wired:
- implicit-self receiver via typeBindings.self (no explicitReceiver branch)
- 2-level MRO chain (Child extends Parent, save resolves on Parent at depth 1)
- FieldRegistry read via Step 2 (property lookup, separate registry path)
Pins the perf invariant on every distinct entry into walkReceiverTypeBinding
so a regression bypassing the hook on any sub-path now fails CI immediately
(ce-code-review finding #8).
* test(resolve-references): cover arity-overload filtering via resolveReferenceSites
Pins the orchestration-layer wiring of providers.arityCompatibility:
hook returns [save(arity 1), save(arity 2)], referenceSite.arity = 1,
arityCompatibility verdicts 'compatible'/'incompatible' by parameterCount,
exactly one reference emitted with toDef = the arity-1 overload.
registries.test.ts already covered arity at the buildMethodRegistry level;
this adds the missing entry-point check that resolveReferenceSites threads
providers correctly through to lookupCore.Step5 (ce-code-review finding #10).
* test(resolve-references): add hook-on vs hook-off parity test
Runs resolveReferenceSites twice on the same fixture (Parent.save method
hit + Child.name field hit, Child extends Parent MRO chain) — once with
ownedMembersByOwner wired to a synthetic registry, once with the hook
absent so collectOwnedMembers takes the defs.byId fallback. Asserts:
- stats are identical (sitesProcessed / referencesEmitted / unresolved)
- referenceIndex.bySourceScope entries have equal length
- toDef sets are equal
- each per-site reference (including evidence and depth) is .toEqual
Locks the semantic-parity claim in code while both paths still exist.
Will be removed alongside the fallback in finding #1 (ce-code-review #3).
* test(typescript): probe Step 2 MRO walk against ambient (declare class) base
Adds typescript-ambient-base-class fixture with an export declare class
AmbientBase + Derived extends AmbientBase and a call site d.ambientMethod().
Integration assertions:
- Both classes are detected
- EXTENDS edge Derived → AmbientBase emitted
- CALLS edge to ambient.ts:ambientMethod resolved via MRO walk
Probes the ce-code-review #6 concern that ambient-only owners (whose
bodies are never parsed) might be silently skipped by Step 2 after the
owner-keyed lookup change. Result: the call resolves correctly — the
method signature inside the declare class body still flows through
reconcileOwnership into model.methods, so the hook returns the right
ancestor hits. Residual risk is empirically closed.
* feat(scope-resolution): route nested types via owner-keyed TypeRegistry
Closes the Step 2 contract footgun where 'hook returns [] = authoritative
miss' silently dropped any owned def whose NodeLabel was outside the
method/field if-chain in reconcileOwnership.
- TypeRegistry: add nestedByOwner Map + lookupAllByOwner(owner, simple)
+ registerByOwner(owner, simple, def). Mirrors MethodRegistry/
FieldRegistry shape; cleared with the rest on cascade clear.
- reconcileOwnership: route class-like NodeLabels (Class/Interface/Enum/
Struct/Union/Trait/TypeAlias/Typedef/Record/Delegate/Annotation/
Template/Namespace) via types.registerByOwner. New nestedTypesRegistered
stat. Idempotent skip via nodeId match.
- validateOwnershipParity: extend the I9 invariant check to nested types.
- lookupOwnedMembersByOwner: merge methods + fields + nested-type hits;
short-circuit when any one source contributes the full result.
Unblocks future receiver-MRO registries that need to resolve 'Outer.Inner'
through the receiver's type-binding chain (ce-code-review finding #5a).
* refactor(scope-resolution): make ownedMembersByOwner required; delete byId fallback
Per ce-code-review finding #1, the optional-hook design encoded a silent
O(|defs|) perf cliff into the type system: any RegistryContext built
without the hook regressed Step 2 to scanning every def per probe with
no warning. Production wires the hook unconditionally; the fallback was
exercised only by tests.
- RegistryContext.ownedMembersByOwner: required, returns readonly
SymbolDefinition[] (no | undefined). Implementations MUST return [] on
authoritative miss.
- collectOwnedMembers in lookup-core.ts collapses to a one-line forward
to the hook; the defs.byId.values() scan and simpleNameOf helper are
deleted (simpleNameOf had no other consumers).
- ResolveReferencesInput.ownedMembersByOwner: required to match.
- Tests: drop three fallback-path tests (registries Const fallback,
resolveReferenceSites no-hook fallback, resolveReferenceSites Const-
undefined fallback) and the hook-vs-fallback parity test added by
finding #3. makeCtx in registries.test.ts now defaults to a real
owner-keyed scan over the test fixture defs so tests that don't care
about the hook keep working.
* perf(free-call-fallback): cache global callables by simple name once per pass
pickUniqueGlobalCallable scanned scopes.defs.byId.values() on every
free-call fallback site. After PR #1656 fixed Step 2, this scan became
the dominant remaining O(|defs|) hot path on large repos (ce-code-review
finding #4).
- buildGlobalCallableIndex builds a Map<simpleName, SymbolDefinition[]>
over scopes.defs once at the top of emitFreeCallFallback. Same filter
the per-site scan applied: Function / Method / Constructor, keyed by
the last .-segment of qualifiedName.
- pickUniqueGlobalCallable consumes the prebuilt index via O(1) Map.get
instead of iterating every def. Per-site complexity drops from
O(|defs|) to O(|defs with this simple name|).
- Cost: O(|defs|) once per pass instead of O(|defs| * |free-call sites|).
Subsequent narrowing (arity, conversion-rank) and the model-side fallback
(model.symbols.lookupCallableByName + model.methods.lookupMethodByName)
are unchanged.
* chore(autofix): apply prettier + eslint fixes via /autofix command
* ci: trigger build
---------
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Gergo Magyar <abhigyan1.patwari@gmail.com>
