mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
Some checks failed
CodeQL / Analyze (javascript-typescript) (push) Has been cancelled
CodeQL / Analyze (python) (push) Has been cancelled
Gitleaks / gitleaks (push) Has been cancelled
Publish / Classify release event (push) Has been cancelled
Scorecard / Scorecard analysis (push) Has been cancelled
Skill copy sync / shipped skills drift guard (push) Has been cancelled
Trivy Image Scan / Trivy (gitnexus-cli) (push) Has been cancelled
Trivy Image Scan / Trivy (gitnexus-web) (push) Has been cancelled
Publish / RC guard (marker + release-PR skip) (push) Has been cancelled
Publish / ci (push) Has been cancelled
Publish / Publish to npm (push) Has been cancelled
Publish / Build & Push RC Docker images (push) Has been cancelled
* fix(mcp): key the empty-ascent note on CALL_SUMMARY data, not language (#2802) `pdg-impact.ts` decided whether to append a "return-value ascent is TypeScript/JavaScript-only" caveat to the `impact(mode:'pdg')` note by looking up the criterion file's language. That put language-specific logic in a layer that must be language-agnostic, and it was a lossy proxy for a fact the graph already holds. Whether the ascent can fire is a property of the persisted CALL_SUMMARY edges. The descent already computes it, so thread the resolved-callee and return-flowing counts out of `interproceduralDescent` and key the note on those instead. Three defects the language proxy carried, all gone: - Wrong for `.mjs`/`.cjs`/`.mts`/`.cts`: the provider registry's extension arrays omit them while the ingestion pipeline parses them as TS/JS, so those files were harvested but the note claimed their ascent was empty. - Silently stale: any language whose harvester started recording formal indices would keep getting the caveat until someone edited the list. - Wrong in reverse: a TS/JS callee with no return-flow got no caveat, so an ascent that found nothing read like one that covered the slice. `pdg-impact.ts` now names no language and imports nothing from the language layer, which also drops the analyze-only provider closure from MCP server startup. Measured on overlayfs against a full build: import mcp/local/local-backend.js before 565-648 ms / 548 modules import mcp/local/local-backend.js after 458-463 ms / 170 modules Tests hold CALL_SUMMARY content fixed while varying the file extension across nine languages and assert the note text is identical, then hold the extension fixed and vary the summary to show the note tracks the data. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(mcp): guard MCP startup against the language-provider closure returning The eager `pdg-impact.ts -> core/ingestion/languages` edge was found and lost once already during #2793 before #2802 re-derived it, so it gets a test rather than a comment. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(lbug): record why csv-generator is not lazy-imported #2802 proposed cutting `csv-generator.js` out of the adapter chain to shorten MCP server startup. Measured on a native filesystem, the marginal cost is small relative to the siblings this module already imports, and `core/search/bm25-index.ts` statically imports `normalizeFtsText` from the same module on a path `local-backend.ts` reaches dynamically for FTS — so deferring would relocate the cost to first query, not remove it. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(pdg): pin chained receiver calls reaching BasicBlock.calleeIds The PDG inter-procedural descent hops through `BasicBlock.calleeIds`, so it can only cross a call boundary the resolver resolved. Chained receiver calls reach `calleeIds` through the receiver-typing pass's own `calleeIdSink` — a separate path from plain calls. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(analyze): drop the stale per-language cross-reference (#2802 review P3-4) `pdgModeMismatch`'s comment told readers to keep "the diagnostic per-language refinement in the impact CONSUMER (see pdg-impact.ts assemblePdgImpactResult)". That refinement is no longer per-language — removing it is the point of #2802, which now keys the empty-ascent note on the persisted CALL_SUMMARY data instead. The comment's real invariant is untouched and still correct: the values in `resolvePdgConfig` must stay scalar, because the comparison below is a shallow `!==` and an object would compare by reference. Only the cross-reference was stale. Comment-only; no executable line changes. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(mcp): probe the real module loader for the startup language closure (#2802 review P1-2) The previous guard hand-rolled a regex walk over TypeScript source to assert `core/ingestion/languages` was not statically reachable from MCP startup. Four bypasses were reproduced against it, any one of which let the exact 226-module regression return while the test stayed green: a. Wrong entry root. It walked from `mcp/local/local-backend.ts`, but the server module is `mcp/server.ts` — which imports LocalBackend as `import type`, so the guard's anchor was not even on server.ts's runtime closure. Ten real startup modules sat outside it. b. A top-level `await import(...)` executes during module evaluation, so it is eager at startup — but the walker skipped every `import(...)` by construction. c. The `import type` strip deleted a 16,445-character window of `pdg-impact.ts`: an `export type X =` matched lazily to the next `from "…"`, which lives inside a string literal. Any import in that window was invisible. d. The comment strip treated a `/*` inside a string literal as a comment opener. Replace the approximation with a real module-load probe: spawn a child node process per entry, import the built `dist/` entry, and report what the loader actually pulled in. Rooted at `dist/mcp/server.js` and `dist/cli/mcp.js` (the real startup entries) plus `dist/mcp/local/local-backend.js`. Syntax cannot fool it. One deviation from the two existing sibling probes is load-bearing: `dist/` is ESM, so a `require.cache` diff alone cannot see the first-party `dist/**` graph — it only catches CJS and native modules, which is why `import-closure.test.ts` gets away with it (it asserts on `@ladybugdb/core`). A pure cache diff here would have reported zero language modules unconditionally, i.e. a new vacuous guard. This probe unions `module.registerHooks({ load })` with the cache diff, and each entry carries a non-vacuity anchor and a module floor so an empty result fails loudly. Verified load-bearing: adding a top-level `await import('../core/ingestion/languages/index.js')` to `src/mcp/resources.ts` and rebuilding turns `dist/mcp/server.js` red with 70+ named offenders, while the `local-backend` and `cli/mcp` cases stay green — which is bypass (a) demonstrated directly. The old guard passed that poisoned tree entirely. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(lbug): drop the unreproducible 9p multiplier from the csv-generator note (#2802 review P3-2) The comment justifying why `csv-generator.js` is NOT lazy-imported carried a hard "~40x" figure for how much a 9p mount inflates per-file ESM resolve. Three independent measurements during review produced ~40x, ~7.3x and ~30x, so the multiplier is not a reproducible quantity and had no business being stated as one in a durable comment. Reworked so the STRUCTURAL argument leads and the numbers only support it. That argument is what actually settles the question and it does not rot: `core/search/bm25-index.ts` statically imports `normalizeFtsText` from `csv-generator.js`, and `local-backend.ts` reaches bm25-index through a dynamic import on the FTS query path — so deferring here relocates the cost to first query rather than removing it. Both verified again at `bm25-index.ts:15` and `local-backend.ts:2756`. Remaining figures are re-measured, attributed to a date and issue, and labelled by filesystem: ~1.6 ms marginal (median of 45 cold imports on local disk) versus ~50 ms for the same import on a network mount, stated as environment-bound rather than as a property of the module. The provider-registry cost is given as "several hundred modules" — the static walk, the runtime hook, and the reviewer's probe each counted it differently (375 / 439 / 407), so no single number was picked to go stale. The old "226 modules" was real but counted only the `languages/` subtree and undercounted the win. Also repoints the trailing reference to the guard's new home at `test/integration/mcp/startup-language-closure.test.ts` (same comment block, inseparable from this rewrite). Comment-only; no executable line changes. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(mcp): stop the empty-ascent note asserting a fact an undecodable summary contradicts (#2802 review P2-2) The note claimed "this is a property of the persisted summaries" whenever the descent resolved callees and none carried a return-flow. But `decodeCallSummary` never throws by design: a version-skewed (`2|r:1`), corrupt (`1|r:zz`), or NULL `reason` yields no entry, which was indistinguishable from a cleanly-decoded empty summary. So the note could assert "no formal parameter is recorded as flowing to its return value" about a callee whose CALL_SUMMARY actually records `p0 -> return`. `meta.pdg.hasCallSummary` is a plain boolean and stores no codec version, so nothing else caught it. `calleesWithReturnFlow` now reports three outcomes instead of two — flowing, decoded-empty, and undecodable — and the undecodable count is threaded through the descent to the note. When it is non-zero the note says so and points at a re-index; when every summary decoded, the persisted-summaries claim is kept and now explicitly conditioned on that. Soundness is unchanged: an undecodable summary still licenses no ascent and never enters the return-flowing set, so the ascent path is byte-identical. Only the note's wording moves. Tests drive all three undecodable forms through the mock and assert the false claim is gone, the remedy is reported, and the ascent is still withheld. A companion assertion pins that the all-decoded case KEEPS the persisted-summaries claim, so the fix cannot degenerate into deleting the sentence. Verified load-bearing: reverting the source alone fails 6 of 34. Impact analysis: `calleesWithReturnFlow` upstream LOW (2 callers, both in this file); `assemblePdgImpactResult` upstream LOW (1 caller). Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(pdg): cover every chained-receiver shape and pin the inference gap (#2802 review P2-1, P3-1) The fixture proved chained receiver calls reach `BasicBlock.calleeIds` using exactly one receiver form — a local `const`. That is the shape that works, so a single-shape fixture implied general support the resolver does not have. This repo has been burned by that before: a drop-count gate blind to fixed shapes. Measuring nine forms against the real pipeline also corrects how the gap was originally characterised. It is NOT local-versus-field. An annotated field resolves fine, including the constructor-assigned variant: private p: Outer = new Outer(); -> both links private p: Outer; this.p = new Outer(); -> both links private p = new Outer(); -> EMPTY CELL private p; this.p = new Outer(); -> EMPTY CELL The discriminator is the type ANNOTATION. When a field's type must be inferred from its initializer the whole `calleeIds` cell empties — so even `Outer.inner`, an ordinary named-receiver call, is lost, and the inter-procedural descent cannot cross the boundary at all. Pre-existing; independent of #2802, which does not touch receiver resolution. The fixture is now table-driven over seven working forms (local const, local in a method, annotated field, ctor-assigned annotated, ctor-param assigned, call-result receiver, three-link chain) plus the two inference-typed forms, each row carrying its expected chain-link ids. Assertions moved from substring to exact id membership, split with the production `splitCalleeIds` reader — so `Inner.compute` can no longer be satisfied by `Inner.computeExtra` or `OtherInner.compute`, which matters because the descent keys on exact ids for span and CALL_SUMMARY lookup. The two known-gap rows are pinned with `it.fails` plus a hard assertion on the exact gap-row set, so a resolver fix turns them red instead of passing silently, and an anti-vacuity guard requires every shape to match exactly one block — without it a drifted fixture matching zero blocks would let `it.fails` pass for the wrong reason. Proven by mutation: relabelling a working row as a known gap fails both pins. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(mcp): qualify the empty-ascent note when the examined callee set is incomplete (#2802 review P2-4) The note asserted "none of the N resolved callees carry a CALL_SUMMARY return-flow", and on the all-decoded path that this is "a property of the persisted summaries". Both are universal claims over the callees the descent actually examined, and two mechanisms can leave that set incomplete without the note saying so: 1. Budget truncation. The descent stops on depth/limit/node-cap, so a callee that DOES carry a return-flow can sit in a hop never reached. A 4-deep chain reported "none of the 3 resolved callees" while link 4 held the only summary. 2. Emit-time capping. When a block's `calleeIds` cell was capped, `splitCalleeIds` strips CALLEES_TRUNCATED_SENTINEL, so the dropped callees are invisible to both the scan and the counters — even though the callgraph bridge in this same file already treats such a block as callee-incomplete. Add `calleeIdsWereTruncated`, the counterpart to the sentinel strip, read from the raw cell before splitting so a block whose entire list was capped away still raises the flag. Thread it through the descent to the note. Case 1 needs no new plumbing — the aggregate `truncated` is already on the input object. Using the aggregate rather than a descent-only flag is deliberate: seed truncation and intra-BFS depth truncation also shrink the initial slice, so their callees are never gathered either. It is a sound superset that never under-hedges. When either mechanism fired, one clause naming the reasons is appended and the whole-slice assertion softens to "every summary examined decoded … a property of those summaries". When the set is complete both branches stay byte-identical to before, so this does not become a blanket hedge. Tests pin truncated, untruncated, emit-capped-alone, both-mechanisms, and undecodable+truncated, asserting the truncation premise rather than assuming it. Verified load-bearing: reverting the source alone fails 6 of 42, and the HEAD note printed in those failures is the bug verbatim. Impact analysis: `assemblePdgImpactResult`, `calleeIdsByBlock`, `interproceduralDescent` all upstream LOW; every caller is in this file and `runImpactPDG`'s exported signature is unchanged. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(mcp): stop the empty-ascent note calling call-site references "resolved callees" (#2802 review P3-7) The note printed "none of the N resolved callees carry a CALL_SUMMARY return-flow (no formal parameter is recorded as flowing to its return value)". N counted the raw `BasicBlock.calleeIds` cell, which carries ids `resolveCalleeSpans` never enters — out-of-repo targets, interface methods, and the `Class:` id a `new X()` emits. On the chained-receiver fixture that inflated N from 1 to 3. Two defects, both in the wording rather than the arithmetic: "resolved" implies a symbol-table lookup that did not happen for those ids, and the parenthetical asserted a FORMALS-level property about symbols never resolved to a body. Reworded rather than re-seeded, deliberately. `calleesWithReturnFlow` scans the RAW id set, so the claim "none of these carries a return-flow" is exactly established for all N — the scan really did check the `Class:` id. Re-seeding N from the resolved spans would make the sentence quantify over a strict SUBSET of what was checked, silently dropping the un-enterable references from a claim that genuinely covers them, and would desync N from `calleesUndecodable`, which is derived from the same scan population. none of the N resolved callees carry ... none of the N call-site callee references carry ... and the formals parenthetical is dropped. The note gets shorter, not longer. `calleesResolved` is renamed `calleeReferences` end-to-end (file-local; nothing outside referenced it), and the descent's return-type doc — which called them "callee symbols the descent resolved" and reinforced the wrong reading — now states that un-enterable ids ride the same cell, are scanned, and are never entered. The `> 0` gate is unchanged, so no slice that previously produced the note stops producing one. A test pins that explicitly: an all-un-enterable cell resolves no span, takes no hop, and emits no ascent sentence despite a non-zero count — so a future re-seeding cannot silently move when the note fires. Tests also pin the quoted number and singular/plural against a mixed cell, with a discriminator asserting `reachableBlocks` is byte-identical while the count moves 1 -> 3. Verified load-bearing: reverting the source alone fails 6 of 7 new tests, printing the finding verbatim. Impact analysis: `assemblePdgImpactResult` and `interproceduralDescent` upstream LOW, sole caller `runImpactPDG` in the same file; exported signature unchanged. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(mcp): pin cross-hop callee accumulation and the mixed return-flow contract (#2802 review P2-5) Every case in this file drove a single hop, so the Set union the descent performs across hops (`calleeReferencesSeen` / `calleesReturnFlowingSeen`) was never proven to accumulate rather than overwrite — a one-hop descent cannot tell the two apart. And although a sibling commit added a three-id cell, none of those ids return-flowed, so the "some callees flow, some do not" boundary was entirely unpinned. Extends the mock with a `secondSummary` knob that drives a genuine second hop: `helper2` is named only in `helper`'s own body block, so the descent must cross a second boundary to reach it. Three mock handlers are made faithful to the parameters they already bind — `calleeIdsByBlock` now routes on the asked `$ids`, and the CALL_SUMMARY scan and span resolve answer per asked id — which is what makes a second callee answerable at all. Existing cases are behavior-identical. Five tests: the union count across two hops; a return-flow on hop 0 surviving a later empty hop; a return-flow found only on hop 1; mixed callees in one examined set going silent rather than partial; and a flowing callee alongside an undecodable sibling staying silent including the decode remedy. The mixed case pins a deliberate contract rather than proposing one. The production condition is `calleesReturnFlowing === 0`, so partial coverage is reported as silence. A reviewer considered and dropped "report partial coverage" as a product change; this makes flipping it a conscious edit instead of an accident. Verified load-bearing against three separate source mutations: accumulating only on hop 0 (2 fail), each hop overwriting instead of unioning (3 fail), and flipping the gate to partial-coverage reporting (4 fail). In all three every PRE-EXISTING test still passed — which is the finding restated as evidence. Test-only; `pdg-impact.ts` is byte-identical to HEAD. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(mcp): consolidate the empty-ascent rationale to one canonical site (#2802 review P3-6) The "keyed on observed CALL_SUMMARY data, never on the criterion's language" rationale was restated in full at four comment sites. It exists because a reviewer asked "why not just look up the language?", so it has to stay findable — but not four times. The canonical explanation now lives in `interproceduralDescent`'s return-type doc, where the counters are actually computed, organised as POPULATION (why the raw `calleeIds` tally is the right set to quantify over) and OBSERVED DATA, NEVER THE CRITERION'S LANGUAGE (the full answer, including the producer-change argument and the no-language-naming rule). The other three sites keep only what is locally load-bearing and point here. Deliberately preserved, because each carries a non-obvious fact: why an undecodable summary licenses no ascent, why the aggregate `truncated` is used rather than a descent-only flag, and the raw-id-tally population argument. Net comment delta -11 lines. The reviewer also flagged the local/field naming asymmetry (`calleeReferencesSeen` vs `calleeReferences`). Keeping the suffix, with a comment recording why so it is not re-raised: the premise that every other local matches its field is true, but those locals are identity-returned, whereas these are `Set<string>` accumulators returned as `.size`. Dropping the suffix would give one identifier two types in one file — a `Set` at the accumulation site and a `number` where the note does arithmetic and pluralisation on it ~900 lines away. The Set-ness is also load-bearing: the dedup is why a callee invoked from two hops is not double-counted, which is what makes the note's count correct. Comment-only. Verified mechanically: every added and removed line in `git diff -U0` matches a comment pattern, so the note's template literals are untouched and its rendered text is byte-identical. 89 tests unchanged. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(mcp): collapse the ascent plumbing accreted across 13 fix commits Quality cleanup, no behavior change. Four independent review passes converged on the same root cause: thirteen commits each fixed one review finding in isolation, and the ascent facts grew one loose field at a time until 62% of the changed region was comments explaining plumbing. Five changes: - `calleeIdsFromBlocks` deleted. Zero call sites anywhere in src/ or test/ — already dead on main, and this branch had edited it to keep it compiling. Its only reference was a stale `{@link}` in a neighbour's doc, now rewritten to stand alone. - `parseCalleeIdsCell` replaces the two-pass read. `calleeIdsWereTruncated` and `splitCalleeIds` were splitting the same cell on adjacent lines, which measured ~2x the parse cost (0.82 -> 1.59 ms at a realistic hop, 57.7 -> 92.7 ms at the per-statement site cap) and was a second independent encoding of the sentinel format — exactly what `splitCalleeIds` was extracted to prevent. One pass classifies as it walks; `splitCalleeIds` stays as a wrapper so its two external callers are untouched. The single-use `export` is gone. - `AscentCoverage` replaces four fields threaded through three signatures. ~12 declaration sites become 3, and the canonical rationale now lives on the type by construction — which is why the earlier doc-consolidation commit was needed at all. - `calleesReturnFlowing` becomes a boolean. Its only reads were `=== 0`, twice; it cost a Set sized to every callee in the slice plus a per-hop union loop. The flag is set inside the existing `returnFlowing.size > 0` branch — equivalent, since the cross-hop union is non-empty iff some hop's was. - The duplicated empty-ascent note head is collapsed to one gate and one head with per-arm tails. Both arms had been edited in lockstep twice in this branch's own history. The rendered note text is byte-identical. Verified structurally and then empirically: both expressions reconstructed standalone and diffed across the full cross product of references x returnFlowing x undecodable x truncated x listTruncated — 288 combinations, 0 mismatches. Net -53 lines. 102 tests pass unedited; the unused-symbol lint warning is gone. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(mcp): parallelise the startup probes, drop a redundant pin, name the mock knobs Quality cleanup from the same review passes. The set of verified behaviors is unchanged except where noted. **Startup probes run concurrently.** `spawnSync` blocks the event loop and vitest runs a file's tests in order, so the three probes strictly serialised. Launching all three with async `spawn` in `beforeAll` and asserting over the collected outcomes cuts the file from ~12.7 s to ~3.9 s wall (-69%). Every promise is caught before `Promise.all`, so all three children are reaped and failures report per entry rather than surfacing only the first rejection. Preserved and each proven by mutation: the missing-dist error names its entry, a raised module floor fails only its own row, and a bogus anchor still reports the loaded-module count. **The two `it.fails` rows are removed.** They pinned the inference-typed receiver gap that the strict `toEqual` pin beside them already covers — and they were the weaker of the two, because `it.fails` passes when the body throws for ANY reason, including `idsFor`'s own non-vacuity guard. A renamed fixture marker would have kept them green on a rotted premise. The strict pin is self-diffing and was verified load-bearing on its own: pointing a known-gap marker at a resolving shape fails it with the two newly-present ids listed. The file header now carries the gap's durable description. **The ascent-note mock takes options objects.** `descentExec` and `run` had grown to five and seven positional parameters in the order five agents added them, so call sites read `run(FILE, true, null, 3, false, undefined, null)` — several carrying `undefined` purely to reach a later argument. All 34 call sites are converted; nine that used only defaults are now bare `run(file)`. No knob renamed — they are orthogonal and correctly named. Code lines are exactly neutral (353 -> 353); the win is at the call sites. Also refreshes five comments that still described `calleesReturnFlowingSeen` and the two-branch note, both of which the preceding commit replaced. 102 unit and 10 integration tests pass; test count moves 9 -> 7 in the chained-receiver file, exactly the two redundant rows. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(mcp): publish return-value-ascent coverage on the PDG impact result `impact(mode:'pdg')` computed four facts about ascent coverage and used them exactly once — to interpolate an English sentence. They never reached the result object, so an agent consuming this MCP output could only ask "was the ascent complete, and if not why" by regexing prose. The cost was already demonstrated: a pure rewording commit earlier in this branch broke ~30 assertions and would have silently broken any consumer keying on the old phrase. Adds `pdgEvidence.ascent`: referencesScanned how many call-site callee references were scanned returnFlowFound did the ascent fire anywhere in this slice undecodableSummaryCount summaries the codec could not decode examinedComplete was the examined set the whole callee list incompleteReasons 'traversal-truncated' | 'callee-list-capped' callSummaryLayerPresent false => pre-FU-C (v3) index Nested under `pdgEvidence` because that is the established counts-and- classification namespace, and `composeUnifiedPdgImpactResult` already spreads it, so the member survives the unified compose untouched. `incompleteReasons` carries CODES, following the existing `truncatedByReasons: ('depth'|'limit')[]` precedent. The prose clause and the structured field now render from one array computed once, so an agent branching on codes and a human reading the note cannot disagree, and a third reason becomes a rendering decision rather than a contract change. Two shape decisions worth recording. `callSummaryLayerPresent` exists because without it a v3 index publishes `referencesScanned: N, returnFlowFound: false`, which reads as "these callees record no return-flow" when the truth is "the layer that records it is absent" — the note already distinguishes those, and the structured surface must not be less honest than the prose. And the field is ABSENT rather than zeroed when the descent never ran (upstream slices): "nothing was scanned" is a different fact from "we scanned and found nothing". `pdgResultVersion` stays 2. The documented trigger is a BREAKING change to the result shape; this removes nothing, renames nothing, and changes no existing field's meaning. Confirmed mechanically: zero top-level key drift across 2304 cases. The historical v2 bump was for changing an existing field's semantics (startLine 0- to 1-based). The note prose is byte-identical, proven across the same 2304 cases with a negative control — perturbing one character of the phrase table produces 60 drifts, so the harness demonstrably detects what it asserts. 14 new tests cover the structured surface and all 14 fail when the source is reverted, while the 54 prose tests pass unchanged. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(helpers): share one module-load probe, and fix two guards that passed on broken builds Three tests independently spawned a child node process to inspect what a built `dist/` entry loads, duplicating the REPO_ROOT derivation, the probe source, the missing-dist guard, the spawn with NODE_OPTIONS cleared, the status-vs-signal rendering, and the payload parse. The newest copy was also the only correct one, so the next author had 2-in-3 odds of copying a weaker probe. The two older probes diff `require.cache` only, which is structurally blind to the first-party ESM `dist/**` graph. That is not theoretical — both were demonstrated passing on genuinely broken builds: - Severing `dist/cli/mcp.js -> stdio-context.js` (a pure ESM change) leaves the require.cache diff EMPTY, so `import-closure.test.ts`'s two assertions reduce to `[].filter(...) === []`. It reported 2 passed on a severed graph. - Severing `registry -> swift/query.js` leaves 76 unrelated CJS entries, which satisfied `registry-import-closure.test.ts`'s indirect guard. The Swift half of its headline had gone vacuous and it reported 1 passed. Both now fail on those same builds, naming the missing anchor. `test/helpers/module-load-probe.ts` unions the ESM `registerHooks({ load })` channel with the cache diff, probes entries concurrently, and makes non-vacuity STRUCTURAL: `anchor` and `minModules` are required fields and the helper throws when either fails. A vacuous probe is a harness failure, not a silently green test, so it cannot be forgotten. Forbidden patterns and remedy text stay per-test — the harness is the shared part, the policy is not. Also fixes `toRepoRelativePosix` resolving non-absolute specifiers against `process.cwd()`, and dedupes modules a CJS-from-ESM import reported once per channel. Faster despite doing more: the registry file goes 12.4s -> 6.75s, because `spawnSync` burned the parent thread polling while the child loaded native grammars. `import-closure` drops to one spawn from two. The `local-backend.js` entry is kept although its closure is currently a strict subset of `server.js`'s: that is an observation, not an invariant. If `server.js` ever stops eagerly reaching the local backend, the server probe stays green while the module #2802 actually changed goes unobserved — and now that anchors are mandatory, that entry is what pins `pdg-impact.js`. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(lbug): trim the csv-generator note and fix the claim it got wrong Two reviewers split on this comment: one wanted it cut to the structural argument, the other said a comment is the right depth for documenting a rejected change since there is no invariant to guard. Both are right, so it stays a comment and gets shorter — 13 lines to 6. Trimmed because it had already taken two corrections (an unreproducible "~40x" figure, and a pointer to a test file that no longer exists), and its tail had drifted from its own guard: the comment said "several hundred modules, ~150 ms" where `startup-language-closure.test.ts` says "~226 extra modules and ~130 ms". Two numbers for one fact. That tail is documented better in the guard's own header, so deleting it loses nothing. It also stated the load-bearing claim inaccurately. The old text said bm25-index imports `normalizeFtsText` "from here" — but `lbug-adapter.ts` neither exports nor re-exports it; the only occurrence of the identifier in this file WAS the comment. Anyone verifying would have grepped, found nothing, and concluded the note was stale. Now names `csv-generator.js` explicitly, re-verified at `bm25-index.ts:15` (static) and `local-backend.ts:2756` (dynamic, on the FTS query path). Comment-only, proven two ways: every changed line matches a comment pattern, and stripping all `//` lines from HEAD and from the working tree yields byte-identical text. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(helpers): extract the temp-repo lifecycle, collapsing five hand-rolled cleanups into one Four cfg integration tests each hand-rolled a `tmpDirs` array, a mkdtemp-and-register step, and an `afterAll` rmSync. It is actually five registrations across six creation sites — `pipeline-pdg.test.ts` keeps a second pool for its C-family fixtures. Seeding genuinely varies four ways (recursive cpSync, single copyFileSync, inline mkdir+writeFile, and nothing at all), so a fixture-copier helper would have fitted about half the sites and made things worse. Extracted the LIFECYCLE instead — mkdtemp, register, afterAll cleanup — which is byte-identical at all five registrations and is the correctness-critical part. `dir()` returns an empty registered directory for callers that seed themselves; `fromFixture()` covers the common case. That fits 6/6. The duplication had already produced a latent defect: `cFamilyTmpDirs` was cleaned by TWO `afterAll` blocks, harmless only because `rmSync` was called with `force: true`. Now one hook. `createTempDirPool` is a function called from each test file's module scope rather than a top-level hook in the helper, because under ESM caching a module-level `afterAll` would register once, against whichever file imported it first. That hazard is documented in the helper. Raw line count is roughly neutral (-44 across the tests, +62 for the helper, 29 of which are the rationale). The win is that a cleanup invariant went from five copies to one. Cleanup verified empirically, including the failure path: a throwaway suite whose `beforeAll` throws still has its directory removed, and every temp directory created by the four migrated files is gone after a run. 46 tests pass across the four files. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(resolvers): pin the inference-typed field receiver gap at the resolver level The gap was pinned only in a PDG test, asserting on `BasicBlock.calleeIds` behind the full `--pdg` pipeline. But it is a resolver fact: when a class field's type must be inferred from its initializer, chained receiver calls resolve to nothing. Whoever closes it will be working in the resolver suite and would have got a red CFG/PDG test with no resolver-side signal. Asserts CALLS edges directly, alongside `python-constructor-field-receiver.test.ts`. Nine receiver shapes run the identical statement; seven resolve, two do not: const o = new Outer() resolves private p: Outer = new Outer() resolves private p: Outer; this.p = new Outer() resolves private p: Outer; this.p = p (ctor arg) resolves constructor(private p: Outer) {} resolves makeOuter().inner().compute() resolves o.inner().mid().compute() (three links) resolves private p = new Outer() NO EDGES private p; this.p = new Outer() NO EDGES Two things the fixture establishes that the PDG-side pin could not. The discriminator is the type ANNOTATION, not local-versus-field — the parameter-property form resolves fine. And the initializer is NOT invisible to the resolver: `new Outer()` still emits its own constructor CALLS edge, byte-identical to the annotated twin. Only the initializer-to-field-type binding is missing, which narrows where a fix belongs. Assertions key on exact node ids rather than names, because `compute` is ambiguous across two classes and keying on the source name collides with `Object.prototype.constructor`. No `describe.skip` and no `it.fails` — the latter passes when the body throws for ANY reason, so it can go green on a rotted premise. The gap is pinned as its explicit current value, which self-diffs: simulating the fix fails one test showing the two newly-resolved ids, and renaming a fixture symbol fails the non-vacuity guard. Runtime is comparable to the PDG-side pin (~9-11s, both dominated by worker startup), so this is an altitude and scope win, not a speed one. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(mcp): replace the extension sweeps with a stronger language-agnosticism pin Two `it.each` sweeps over nine file extensions asserted that the empty-ascent caveat was present (or absent) for each. They looked like the pin for the property the whole change exists for — `pdg-impact.ts` must name no language and its output must not vary by extension — but they were the weakest available form of it. They asserted substring presence/absence, so a language dependence that ADDS text while leaving the caveat intact passes them. Demonstrated, not assumed: injecting a `.py`-only hedge inside the caveat sentence and replaying the two sweeps verbatim against that source gives 18 passed. The byte-identity test beside them caught it. So the sweeps are deleted and the identity test carries the property alone, hardened in two ways: - Two rows instead of one, covering BOTH sides of the caveat gate. The silent (return-flow present) branch previously had no identity counterpart at all — nine runs proving one fact, with nothing checking that its rendering was extension-invariant. - The fingerprint spans the note AND the reachable blocks, not just the note. Strictly more than the sweeps verified. Entailment is exact: identity across the extension set, plus the two existing single-extension content assertions, gives "every extension gets the caveat" and "no extension gets it". Reducing a sweep to one extension was rejected because it reproduces an assertion already present verbatim. Also converts the incompleteness block from six near-identical bodies to a 3-row premise table crossed with two assertions. Each row now names the exact phrase set its clause must contain, so presence and absence are asserted together — which adds three checks the longhand version lacked (the budget row now also proves the emit-cap phrase is absent). And three tests that re-rendered one fixture to make one assertion each are hoisted to a single render. 97 tests, down from 116: -18 sweep cases, -2 from the hoist, +1 identity row. No assertion was lost; several were added. Verified by injection: a `.py`-only note change fails the identity pin, and a dependence in the shared hop sentence fails BOTH rows, confirming the second row is load-bearing rather than decorative. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(mcp): lazy-import syncGroup so MCP startup skips the group extractor closure `core/group/service.ts` statically imported `./sync.js`, which pulls all six contract extractors, five of which statically import the native `tree-sitter` binding. That put the whole parser stack on every MCP server start, for a server that never syncs. Only `groupSync` needs it. The other seven group tools — `group_list`, `group_impact`, `group_query`, `group_contracts`, `group_status`, `group_trace`, `group_context` — do not, and now never load it. `syncGroup` has a single call site, already inside an `async` method, so this is a lazy `await import(...)` at that call site and nothing else: no signature change, no async ripple, no change to `local-backend.ts`. The pattern is already established on this exact module — `cli/group.ts`'s sync command lazy-imports `sync.js` the same way. `service.ts` was the outlier. Measured on a native filesystem (overlayfs; /workspace is a 9p mount that inflates ESM resolve, so it is not a valid measurement surface), 5 cold runs, medians: dist/mcp/server.js 521 ms -> 133 ms (-75%) dist/mcp/local/local-backend.js 453 ms -> 66 ms (-85%) tree-sitter modules at both entries: 11 -> 0 Same defect class as #2802, which cut the language-provider registry from the same startup path; this is what remained. The cost is moved rather than deleted: the first `group_sync` call now pays the module load. That is the right trade — `group_sync` is already a long-running operation, and sessions that never sync pay nothing. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(mcp): guard MCP startup against the group extractor closure returning Sibling forbidden-pattern case in the #2802 startup guard, reusing the concurrent probes it already collects — no new spawn, no new harness. Asserts that none of `dist/mcp/server.js`, `dist/cli/mcp.js`, or `dist/mcp/local/local-backend.js` loads a `core/group/extractors/` module or the native `tree-sitter` package. The parser is matched by package prefix rather than a bare substring, so a source file that merely mentions the word can neither satisfy nor trip it. Verified load-bearing rather than assumed: restoring the static `import { syncGroup }` in `core/group/service.ts` and rebuilding turns `dist/mcp/server.js` red and names all seven offenders — http-route, grpc, thrift, topic, include, manifest and workspace extractors. Reverted and re-confirmed green. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(mcp): keep the analyze-only CFG closure off MCP server startup (#2802 review) `mcp/local/pdg-impact.ts` imported `CALLEES_TRUNCATED_SENTINEL` and `CALLEE_ID_SEP` from `core/ingestion/cfg/emit.ts`. ESM evaluates a module to import any binding from it, so those two strings dragged the whole analyze-only CFG closure into every MCP server start. Measured against a clean build, per entry point: 8 modules — `emit`, `reaching-defs`, `reaching-defs-graph`, `control-dependence`, `post-dominators`, `synthetic-escape`, `call-site-harvest`, `reaching-def-reason-codec` — present at `dist/mcp/server.js`, `dist/mcp/local/local-backend.js` and `dist/mcp/http-transport.js`. Same defect class as the language-provider closure this branch already removed, and the guard could not see it: `FORBIDDEN_RE` covers `core/ingestion/languages/` and `FORBIDDEN_GROUP_RE` covers `core/group/extractors/|node_modules/tree-sitter`, neither of which matches `core/ingestion/cfg/`. The format constants move to a new LEAF module `cfg/callee-cell-format.ts` that imports nothing; `emit.ts` re-exports both names so every existing importer is untouched, and producer and consumer still resolve to one definition — the drift the shared constant exists to prevent stays impossible. Deleted, not deferred — the same bar #2802 held its own csv-generator proposal to. After: cfg modules at startup 8 -> 2, and both survivors (`callee-cell-format`, `reaching-def-reason-codec`) are leaves that import nothing. Totals: `server.js` 387 -> 380, `local-backend.js` 163 -> 156, `http-transport.js` 523 -> 516. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(mcp): stop pdgEvidence.ascent claiming a completeness it cannot have (#2802 review) `examinedComplete` is the field a consumer reads to decide whether `returnFlowFound: false` is a whole-slice claim. It could be published `true` over a callee set the descent never finished examining — the exact false all-clear the field was added to prevent. Root cause: `bfsReachableBlocks` sets `truncatedByDepth` when its frontier is still non-empty at the budget, but both call sites inside `interproceduralDescent` folded only the row-limit flag and dropped the depth flag. The top-level intra BFS's copy of that same flag was already propagated, so the asymmetry was unintended — one `if`-pair folding limit-but-not-depth, within a merge that already folds the node cap too. Reproduced at `maxDepth: 3`, the shipped default: a criterion calling a helper whose body is a 5-block dependence chain, with the return-flowing callee on the block past the clamp. Result reported `truncated: undefined`, `examinedComplete: true`, `incompleteReasons: []` and an unqualified universal note sentence. Fixed by propagating the dropped flags rather than inventing a parallel channel: `intraDepthBudget` is documented in-file as the SAME clamp the top-level intra BFS applies, and that one's depth truncation is already result-level. So the result's own `truncated`/`truncatedBy` were under-reporting for the same reason, and both surfaces are corrected together. Four further honesty fixes to the same published record: - Blocks reached only by the U-C4 ascent went into `reachable` but never `hopReached`, so their `calleeIds` cells were never scanned, never counted, and could not raise `callee-list-capped`. They are slice blocks; they now enter the hop set and get the same treatment as every other one. - `pdgEvidence.ascent` was absent on the empty-slice early return even though the descent had already run and scanned, contradicting the "present iff the descent ran" contract this branch itself added to `tools.ts`. Both exits now classify through one shared helper so they cannot disagree. - A block carrying call sites in `callees` but no resolved ids in `calleeIds` (the whole-file case where `emit.ts` has no fileMap) silently shrank the population while `examinedComplete` still reported `true`. That now raises a third reason, `callee-ids-unrecorded`. - `referencesScanned` is a distinct-callee tally and both surfaces described it as a call-site count. Field name kept — a rename is breaking at `pdgResultVersion: 2` — and the prose corrected instead. `PdgAscentIncompleteReason` gains a member, which is additive, so `pdgResultVersion` stays 2. Visible output change worth knowing: slices whose callee chain outruns `maxDepth` now report `truncatedBy: 'depth'` where they previously reported none, and a repo with id-less call sites now reports `examinedComplete: false`. Both are strictly more honest. Every behavioural change carries a mutation proof — revert the source, watch the new test go red, restore. One exception is documented inline rather than faked: the ascent-side fold cannot be observed independently, because the re-seed shares the caller's `visited` set and so can only reach past the budget when the traversal that covered that closure was already cut and had already raised a flag. Suite: 49 -> 59 tests. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(mcp): anchor each import-closure policy on the edge it polices (#2802 review) `module-load-probe.ts` makes non-vacuity structural via a required `anchor` — but the anchor was one per ENTRY while `startup-language-closure.test.ts` now runs TWO independent policies. The group-extractor policy added in83e8cf7c5therefore had no anchor of its own, and one of its three rows was already vacuous: `cli/mcp.js` loads four leaf modules and reaches no `core/group/` module at all, so its group assertion could not fail for any policy-related reason while its `dist/mcp/stdio-context.js` anchor stayed green. Proven, not argued. `dist/mcp/local/local-backend.js` is the only static importer of `core/group/service.js` in the whole build; severing that one edge — the exact next lazy-load step — and re-probing: OLD shape (anchor per entry): server 385, http-transport 521, local-backend 161 reaches group/service = false, group offenders 0 -> GREEN on all three NEW shape (anchor per policy): -> RED on all three, each naming the missing dist/core/group/service.js Counts fell only 387->385 and 163->161, so `minModules` was structurally blind to the severance; the anchor is the only thing that catches it. `anchor` accepts `string | readonly string[]` and every listed anchor must load. Existing single-anchor call sites are unchanged. `anchorsOf()` lets the group `it.each` DERIVE its entries by filtering on the group anchor, with a test pinning that derivation, so the policy cannot silently register zero cases. `cli/mcp.js` is dropped from the group policy — it cannot honestly carry that anchor — and the doc-comment now states the invariant: an anchor is per-POLICY, not per-entry. Also: - `mcp/http-transport.js` gets a row. It is the largest startup entry (516 modules) and `src/cli/mcp.ts` imports it directly rather than through `server.js`, so nothing about the server row constrained it. Measured clean today; the gap was coverage, not a broken claim. - The three spawn-based closure tests are registered in `SPAWN_CLI`, so the Windows-safety plumbing this branch wrote for them (POSIX normalisation, `pathToFileURL`, `NODE_OPTIONS` clearing, array-form `spawn`) is finally exercised on the Windows/macOS matrix. Measured cost ~11.7s on Linux; budget ~60s on Windows against a 25-minute job. - `PROBE_TARGET` now wins over `extraEnv`, which was spread last and could have silently redirected a probe while `anchor`/`minModules` stayed keyed on `entry`. - The child's JSON payload is validated through a type predicate instead of a bare `as string[]`, and the spawn timeout escalates SIGTERM to SIGKILL so a child stalled in native code is reaped rather than orphaned. - Recorded baselines re-measured (server 380, local-backend 156, cli/mcp 4) and relabelled a snapshot rather than a contract — they moved twice inside this branch alone. The subset claim was re-verified exactly: 0 of local-backend's 156 modules are absent from server's 380. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(helpers): survive a failing temp-dir removal instead of leaking the rest (#2802 review) `createTempDirPool`'s `afterAll` ran a bare `for (const d of created) fs.rmSync(d, {recursive, force})`. `force` suppresses only `ENOENT` — not the `EBUSY`/`EPERM`/`ENOTEMPTY` class a Windows runner produces when a pipeline test still holds a handle — so the FIRST failure threw out of the loop and leaked every directory registered after it. Pre-existing: all four hand-rolled cleanups this helper consolidated had the same shape. But the blast radius is now shared across four consumers, which is exactly why it is worth fixing at the point of consolidation. Cleanup is now per-directory best-effort via `removeTempDirs`, plus Node's own documented mitigation for that error class (`maxRetries: 3, retryDelay: 50`), which costs nothing on the happy path. Warn rather than swallow or rethrow, and the reasoning is in the doc comment, not just here: rethrowing would fail an otherwise green suite from `afterAll` over housekeeping the OS reclaims anyway, where it reads as a test failure and buries the real result — a Windows EBUSY on a temp dir is not a defect in the code under test. Silence is the opposite hazard: a systematic leak would be invisible with nothing naming the responsible suite. The warning carries the path, and the `mkdtemp` prefix is per-pool, so it names the suite that made it. Failure is injected through a scripted remover keyed by path (a Map lookup, so no `if` in a test body and no dependence on producing a real locked handle). Beyond the three behavioural pins there is a wiring pin — a nested `describe` creates a real pool and a sibling `it` declared after it asserts the dirs are gone — so the tested function cannot drift into "tested helper plus an untested copy of the loop". Mutation proof: restoring the abort-on-first-failure loop turns 3 of the 5 tests red, the throw escaping `removeTempDirs` outright so the third real directory is never attempted. With the fix, `[first, blocked, last].map(existsSync)` is `[false, true, false]` — the injected failure survives and the directory after it is really gone, through the remover that actually ships. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(pdg): point the self-diffing receiver pins at #2807, not at this PR (#2802 review) Both pins named the gap "(#2802 follow-up)". The gap has its own tracking issue — #2807, "Inference-typed field receivers resolve to no CALLS edges at all" (open, labeled bug) — and PR #2810 is already open against it. As written, after merge the gap was discoverable only by reading a KNOWN GAP marker inside a test file, not from the issue tracker. Both describe names now read "(known gap: #2807)" and both KNOWN GAP test names carry the number. #2802 is kept only as provenance: the gap was FOUND during #2802 work but is pre-existing and independent of it. Each header gains an explicit "this pin is self-diffing: it will go red on purpose" section naming #2807 with its exact title, noting #2810 is open against it at the time of writing, and stating that the pin asserts the gap EXISTS — so closing #2807 fails it by design, and the correct response is to update the expected value, not to relax the assertion. The same note is repeated inline above each KNOWN GAP test, where a maintainer editing it will actually see it. No pin is weakened. Both deliberately reject `it.fails` in favour of exact `toEqual` assertions with a non-vacuity probe, and that design is left untouched. Refs #2802, #2807 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(group): cover the lazy syncGroup import that no test reached (#2802 review)9ea9676dcturned `GroupService.groupSync`'s `syncGroup` into `await import('./sync.js')` — this branch's one changed control-flow line in production code — and nothing exercised it. Every existing test stopped short: `service.test.ts` returns at the empty-name guard; `group-service-not-found.test.ts` mocks `loadGroupConfig` to reject and never invokes its `syncGroupMock`; `group-sync.test.ts` imports `syncGroup` directly, bypassing `GroupService`; and the startup guard asserts only the negative, that `sync.js` is absent at startup. `tsc` catches a path typo, but nothing verified the import resolves and hands off correctly — while every production `group_sync` call goes through that line. No production change was needed; the reviewed design was sound. This is the missing coverage. The happy-path test mocks nothing: it points `GITNEXUS_HOME` at a pool temp dir, seeds a real `group.yaml`, and calls `groupSync`, so `loadGroupConfig` resolves, `groupDir` is found, and execution falls through into the REAL `syncGroup`. What makes a real sync reachable with no indexed repo: an empty registry puts both members in `missingRepos`, but one declared manifest link still yields synthetic-UID contracts. It asserts the returned counts AND reads back the `contracts.json` that real `syncGroup` wrote into `groupDir` via the production `readContractRegistry`, which pins the option handoff too. Two further tests use `vi.doMock` to re-evaluate the service against a `sync.js` whose load throws: one asserts the call rejects with the load failure in its `cause` chain — so the caller gets a catchable rejection, not a floating unhandled one — and one asserts both pre-import guards still answer with `sync.js` unloadable, which is also a structural pin that the module has no STATIC import of it (a static one would throw at re-import, before any call). Mutation proofs: pointing the specifier at `./sync-nope.js` turns 2 of 3 red ("Cannot find module .../sync-nope.js ... at GroupService.groupSync service.ts:349"); aliasing a real-but-wrong export turns 1 red. Restored, all 3 green, and `service.ts` verified byte-identical to HEAD. Out of scope, stated rather than glossed: the final `isError: true` MCP envelope is produced above `GroupService` and needs a full `LocalBackend`; the rejection test is the in-scope half of that claim. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(mcp): close the gaps a cleanup pass found in the #2802 review fixes Quality pass over the review-response series (reuse / simplification / efficiency / altitude). No behaviour change except where noted. The two that mattered: - **The cfg/emit fix had no guard.** `FORBIDDEN_RE` covers `core/ingestion/languages/` and `FORBIDDEN_GROUP_RE` covers `core/group/extractors/|tree-sitter`; neither matches `core/ingestion/cfg/`. Because `emit.ts` re-exports the constants, pointing `pdg-impact.ts` back at `cfg/emit.js` typechecks identically and silently restores all 7 modules. Verified: with the import reverted, `tsc --noEmit` still exits 0 and every test stayed green before this commit; after it, 3 rows go red naming the offenders. Written as an ALLOWLIST of genuine leaves rather than a denylist of the 7 already-suffered modules, because the next regression is a module nobody has thought of yet. - **`FORBIDDEN_GROUP_RE`'s parser matcher was forward-slash only** while both sibling probe regexes spell the separator `[\\/]`. Native bindings arrive via the `require.cache` channel as absolute paths and `toRepoRelativePosix` only normalises paths inside the repo root, so a hoisted `node_modules` renders as `…\node_modules\tree-sitter\…` on Windows and matched nothing. The same series put this file on the Windows matrix, where that half of the assertion would have been vacuous. Reuse — three re-implementations of existing helpers: - `removeTempDirRecursive` re-rolled `fs.rmSync` retries; it now delegates to `cleanupTempDirSync` (`test-db.ts`), the repo's Windows-lock-aware remover. The copy had already drifted on both knobs that matter — 3 retries at 50 ms vs 5 at 100–400 ms, and warn-on-everything vs swallow-lock-codes-rethrow-rest — which is how one half of a suite goes green-with-a-warning on the same `EBUSY` the other half fails on. The per-directory try/warn loop, which is the actual fix, is unchanged. - `errorChainText` re-rolled the cause-chain walk that `causeChain` (`src/lib/utils.ts`) exists to be the single copy of — its own doc asks callers not to. - The SIGKILL escalation (a timer, an `unref`, and two `clearTimeout`s) is `spawn`'s own `killSignal` option, which Node's `timeout` already delivers. Simplification and altitude: - `'callee-ids-unrecorded'` documented ONE of its three producer paths. The unnamed common one is a call site that did not RESOLVE — exactly the receiver gaps this repo pins (#2807) — so on a real index the reason fires broadly, driven by resolution quality rather than a missing `--pdg` layer, and "re-run analyze --pdg" is the wrong remedy for it. Doc now names all three and states the consequence: `examinedComplete: true` is the strong, rare signal. - The derived policy-entry list was re-pinned against a hand-written 3-element literal, reinstating one layer down the list the derivation removes. Now asserts the properties that are actually at risk — non-emptiness (a policy going silent) and `cli/mcp.js` staying excluded (a row that cannot fail). - A test fixture spread `ascentBlockCell: 'idless'` and then overrode it to `'capped'` in both runs, so the id-less shape never reached the mock while reading as though it did. - `idlessCallSites` is sticky, so its per-row string allocation now short-circuits once set. - Dropped an unused `export` on `CleanupWarner`. Refs #2802 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * revert(ci): unregister the module-load closure guards from the Windows matrix Registering the three `dist/` closure guards in `SPAWN_CLI` turned the Windows `platform-sensitive 1/3` shard red at the 20-minute watchdog. Baseline83e8cf7c5was green on all three shards;a4245119c(which added them) failed 1/3;d0b201442failed the same way. It is not the files themselves. On the Windows runner they are among the cheapest in the suite — `registry-import-closure` 448 ms, `import-closure` 53 ms — and both passed. vitest shards this list by file COUNT, not runtime, so adding three files RESHUFFLED the split: shard 1 went to 32 files against 26 and 29, concentrating the heavy CLI e2e suites. It timed out with `cli-e2e`, `group/cross-trace-e2e`, `lbug-orphan-sidecar-recovery` and `server-http-startup` still queued — `cli-e2e` being the ~50-spawn suite whose setup flakiness already needed fixing once (PR #2000). That clustering fragility is pre-existing and this file's own header documents it (#2449: "the heaviest spawn suites can cluster on one shard", busiest Windows shard already at 14m57s against the old watchdog). These three files only tipped it over, and unblocking the PR beats holding it for a CI-infra fix that belongs in its own change. Reverted rather than worked around: raising the shard count would keep the coverage but is a repo-wide CI change made on a 25-minute feedback loop with no guarantee the reshuffle balances, and this PR is about MCP startup. The removed entries are replaced by a comment recording WHY they are absent, what they were measured to cost, and the precondition for re-landing them — so the gap is documented at the point someone would otherwise re-add them blind. Verified: the emitted file list is byte-identical to 83e8cf7c5's, so the shard split returns to the configuration that was green. The Windows-specific bug this series found is unaffected — `FORBIDDEN_GROUP_RE` now spells its separator `[\\/]` like its siblings, which was a real forward-slash-only vacuity, and that fix stays. Refs #2802, #2449 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(ci): shard the cross-platform matrix by measured weight, not file count Restores the three `dist/` module-load closure guards to the Windows/macOS matrix, and fixes the reason they could not stay there. They must run on every OS — the shared probe in `test/helpers/module-load-probe.ts` IS the platform-varying code (array-form `process.execPath` spawn, cleared NODE_OPTIONS, `pathToFileURL` because Windows rejects a bare absolute path as an ESM specifier, and a `path.sep`→POSIX normalisation the anchors and offender regexes depend on). Ubuntu-only coverage of a platform guard is no coverage. The earlier attempt turned Windows `platform-sensitive 1/3` red at the 20-minute watchdog, and the reflex fix — unregistering them — treated the symptom. The files are among the cheapest in the suite (measured 448 ms, 53 ms, sub-second, and both that completed passed). The defect is that `run-cross-platform.ts` handed vitest all 84 files plus `--shard=i/n`, and vitest partitions by file COUNT. Runtimes here span three orders of magnitude, so a count-split is blind to the thing that decides the budget, AND re-partitions on every insertion: adding three free files reshuffled the list and happened to co-locate `cli-e2e` (361 s) with `cli-limit-e2e` (75 s) and `analyze-heap-oom-e2e` (23 s) — 32 files against 26 and 29 — which timed out with four still queued. The split now happens in `scripts/cross-platform-shard.ts`, longest-processing- time first over measured Windows runtimes, and only the chosen shard's files are passed to vitest (`--shard` is consumed, never forwarded — forwarding would re-partition the slice a second time and silently drop most of it). Weights are measured, from the last green matrix run plus the timed files of the failing one, and every file also carries an 8 s per-file floor. That floor is calibrated, not guessed: the last green busiest shard ran 736 s of wall clock over ~511 s of attributed file time. Without it the balancer isolates the two monsters and then piles every light file onto the remaining shards — trading a runtime imbalance for a count imbalance that costs the same. Result at TOTAL=3, with the three guards back in: 521 s / 527 s / 519 s across 20 / 33 / 34 files. The previous green configuration's busiest shard was 736 s, so this is better balanced than the state before any of this, and the busiest shard is now bounded by construction rather than by sort-order luck. `test/unit/cross-platform-shard.test.ts` pins the properties, and the load-bearing one is not "the split is even" — it is "adding a cheap file cannot move a heavy one", the property whose absence caused the outage. Two details in that test are themselves load-bearing, and earlier drafts got both wrong and were vacuous: the inserted names must sort EARLY (names sorting last disturb nothing under any scheme) and the count must not be a multiple of the shard total (adding exactly `total` files leaves an equal-weight round-robin in the same rotation). Mutation-proved: replacing `weightOf` with a constant — i.e. count-based sharding — turns that test and the per-file-floor test red; restored, all 8 pass. Refs #2802, #2449 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
110 lines
4.7 KiB
TypeScript
110 lines
4.7 KiB
TypeScript
/**
|
|
* Cross-platform test runner.
|
|
*
|
|
* Runs the platform-sensitive test subset defined in cross-platform-tests.ts
|
|
* via vitest. Used by `npm run test:cross-platform` and by the CI cross-
|
|
* platform matrix (ci-tests.yml).
|
|
*
|
|
* The main vitest.config.ts is used, so lbug-db project files get
|
|
* sequential execution and other safety constraints are preserved.
|
|
*/
|
|
|
|
import { execFileSync } from 'child_process';
|
|
import fs from 'fs';
|
|
import path from 'path';
|
|
import { fileURLToPath } from 'url';
|
|
import { ALL_CROSS_PLATFORM } from './cross-platform-tests.js';
|
|
import { parseShardArg } from './shard-arg.js';
|
|
import { shardFiles, shardWeight } from './cross-platform-shard.js';
|
|
|
|
const __dirname = path.dirname(fileURLToPath(import.meta.url));
|
|
const ROOT = path.resolve(__dirname, '..');
|
|
|
|
// Verify all files exist
|
|
const missing = ALL_CROSS_PLATFORM.filter((f) => !fs.existsSync(path.resolve(ROOT, f)));
|
|
if (missing.length > 0) {
|
|
console.error(`Cross-platform test files not found (${missing.length}):`);
|
|
for (const f of missing) console.error(` ${f}`);
|
|
console.error('\nUpdate scripts/cross-platform-tests.ts if files were moved or removed.');
|
|
process.exit(1);
|
|
}
|
|
|
|
// Optional sharding (CI): `--shard=<i>/<n>` splits the fixed file list across
|
|
// parallel matrix shards. The split is computed HERE, by measured weight, and
|
|
// only this shard's files are handed to vitest — it is NOT passed through,
|
|
// because vitest partitions by file COUNT and this suite's runtimes span three
|
|
// orders of magnitude (see cross-platform-shard.ts). The
|
|
// Windows runner is ~5x slower than macOS/Linux on this spawn-heavy suite (~50
|
|
// CLI/worker process spawns), so a single shard was creeping past the watchdog
|
|
// below; sharding keeps each runner well under it (see ci-tests.yml matrix).
|
|
// Fail loud on a malformed --shard arg (mirrors the missing-files check above):
|
|
// a silently-dropped shard flag would run the full unsharded suite and re-trip
|
|
// the watchdog. Kept outside the execFileSync try/catch below so the message
|
|
// isn't swallowed by that catch's watchdog-only branch.
|
|
let shardArg: string | undefined;
|
|
try {
|
|
shardArg = parseShardArg(process.argv.slice(2));
|
|
} catch (err) {
|
|
console.error(err instanceof Error ? err.message : String(err));
|
|
process.exit(1);
|
|
}
|
|
|
|
// Per-shard watchdog, default 15 min. Sharding splits the file list by COUNT, not
|
|
// runtime, so the heaviest spawn suites can cluster on one shard — what this
|
|
// bounds is the *busiest* shard, not an even 1/n of wall-clock. The busiest
|
|
// Windows shard has grown to the default (14m57s on the v1.6.10-rc.19 green
|
|
// run, one observed timeout since — #2449), so CI raises the budget to 20
|
|
// minutes via GITNEXUS_CROSS_PLATFORM_TIMEOUT_MINUTES; the default stays 15
|
|
// for local runs.
|
|
const DEFAULT_TIMEOUT_MIN = 15;
|
|
const timeoutMinutes = Number.parseInt(
|
|
process.env.GITNEXUS_CROSS_PLATFORM_TIMEOUT_MINUTES ?? String(DEFAULT_TIMEOUT_MIN),
|
|
10,
|
|
);
|
|
const timeoutMs =
|
|
Number.isFinite(timeoutMinutes) && timeoutMinutes > 0
|
|
? timeoutMinutes * 60 * 1000
|
|
: DEFAULT_TIMEOUT_MIN * 60 * 1000;
|
|
|
|
// Resolve the shard to an explicit file list. `--shard=i/n` is consumed here,
|
|
// never forwarded: forwarding it as well would re-partition this slice a second
|
|
// time and silently drop most of it.
|
|
const shardParts = shardArg?.replace('--shard=', '').split('/');
|
|
const shardIndex = shardParts ? Number(shardParts[0]) : 1;
|
|
const shardTotal = shardParts ? Number(shardParts[1]) : 1;
|
|
const files = shardFiles(ALL_CROSS_PLATFORM, shardIndex, shardTotal);
|
|
|
|
console.log(
|
|
`Running ${files.length} of ${ALL_CROSS_PLATFORM.length} platform-sensitive tests` +
|
|
`${shardArg ? ` (shard ${shardIndex}/${shardTotal}, ~${shardWeight(files)}s measured weight)` : ''}...\n`,
|
|
);
|
|
|
|
const startedAt = Date.now();
|
|
try {
|
|
execFileSync('npx', ['vitest', 'run', ...files], {
|
|
cwd: ROOT,
|
|
stdio: 'inherit',
|
|
timeout: timeoutMs,
|
|
shell: true,
|
|
});
|
|
} catch (err) {
|
|
// execFileSync sets `killed`/`signal` when the watchdog above kills vitest.
|
|
const e = err as {
|
|
killed?: boolean;
|
|
signal?: NodeJS.Signals | null;
|
|
status?: number | null;
|
|
code?: string;
|
|
};
|
|
if (e.killed || e.signal) {
|
|
console.error(`vitest timed out after ${Math.round(timeoutMs / 60_000)} minutes`);
|
|
}
|
|
// #2449: Windows shards have died with a bare `status: null`, empty stderr
|
|
// and nothing to triage from. Always leave the child's exit facts behind.
|
|
const elapsedSec = Math.round((Date.now() - startedAt) / 1000);
|
|
console.error(
|
|
`vitest exited abnormally: status=${e.status ?? 'null'} signal=${e.signal ?? 'none'} ` +
|
|
`killed=${e.killed === true} spawnCode=${e.code ?? 'none'} elapsed=${elapsedSec}s ` +
|
|
`budget=${Math.round(timeoutMs / 60_000)}min`,
|
|
);
|
|
process.exit(1);
|
|
}
|