diff --git a/.github/workflows/ci-tests.yml b/.github/workflows/ci-tests.yml index 36a07fbbc..d59709dd6 100644 --- a/.github/workflows/ci-tests.yml +++ b/.github/workflows/ci-tests.yml @@ -482,6 +482,14 @@ jobs: working-directory: gitnexus - name: Cross-language scope-capture fingerprint + scaling guards + # Runs even after an earlier guard fails (#2895). Every step here was + # fail-fast, so the FIRST failing --check aborted the job and every guard + # after it reported `skipped` — which reads identically to "nothing to do". + # Audited across 13 benchmark runs on #2856: the job succeeded zero times + # and the last two guards executed zero times for the life of the PR, while + # two reviews read the checks summary and saw nothing wrong. `!cancelled()` + # rather than `always()` so an explicit cancel still stops the job. + if: ${{ !cancelled() }} # Build-free: asserts emitScopeCaptures output is unchanged # (fingerprint) and stays linear (scaling < 1.5) for go/csharp/rust/php/ # ruby/cobol. Catches an O(n^2) re-regression without the worker pool. @@ -489,6 +497,7 @@ jobs: working-directory: gitnexus - name: Callable-value-flow target-index guards (#2693) + if: ${{ !cancelled() }} # Build-free: asserts buildGraphTargetIndex resolves an unchanged target # set (fingerprint), stays linear in def count, and that the #2693 # widened gate — which now considers VALUE bindings, a population that @@ -501,6 +510,7 @@ jobs: working-directory: gitnexus - name: C++ qualified-namespace resolution guards (#2788) + if: ${{ !cancelled() }} # Build-free: asserts resolveCppQualifiedNamespaceMember resolves an # unchanged symbol set (fingerprint) and that per-call-site cost stays # independent of corpus size. Rationale and history: see the header of @@ -555,6 +565,7 @@ jobs: working-directory: gitnexus - name: Kotlin import-resolution identity + scaling guards + if: ${{ !cancelled() }} # Build-free: asserts resolveKotlinImportTarget resolves an unchanged # file set (fingerprint, in both file-set iteration orders — every # tie-break in that resolver is expressed only through iteration order) @@ -566,6 +577,7 @@ jobs: working-directory: gitnexus - name: Receiver-resolution drop guards + if: ${{ !cancelled() }} # NOT build-free: this one runs the real pipeline, so it needs dist/ # (the setup action above builds). ~2m15s. # @@ -591,6 +603,7 @@ jobs: working-directory: gitnexus - name: Scope-emission guards (#2699) + if: ${{ !cancelled() }} # Build-free: asserts the JS/TS scope set is unchanged. Block scopes are # what make `let`/`const` in sibling blocks distinct bindings, but a # scope per `statement_block` triples the count and deepens every @@ -603,6 +616,7 @@ jobs: working-directory: gitnexus - name: CFG construction time / disk / memory guards (#2081 M1) + if: ${{ !cancelled() }} # Build-free: asserts collectFunctionCfgs output is unchanged # (fingerprint) and that wall-time, cfgSideChannel disk bytes, AND # retained heap all stay sub-quadratic for the straight-line / @@ -613,6 +627,7 @@ jobs: working-directory: gitnexus - name: Emit-persistence throughput / byte-identity guards (#2203) + if: ${{ !cancelled() }} # Build-free: asserts streamAllCSVsToDisk output is byte-identical # (order-independent CSV-line fingerprint — the #2203 U2/U3 emit # optimisations must not change graph content) and that emit wall-time @@ -622,6 +637,7 @@ jobs: working-directory: gitnexus - name: Streaming PDG-emit byte-identity / bounded-RSS guards (#2202) + if: ${{ !cancelled() }} # Build-free: asserts the streaming PdgEmitSink emits a CSV row SET # byte-identical to the whole-graph streamAllCSVsToDisk emit, AND that # the in-memory graph retains zero BasicBlock nodes (the O(chunk) peak-RSS @@ -631,6 +647,7 @@ jobs: working-directory: gitnexus - name: Cross-language pipeline benchmarks (GITNEXUS_BENCH, serial) + if: ${{ !cancelled() }} # cpp-adl-benchmark.test.ts is not a `*-pipeline-benchmark.test.ts` but # belongs here for the same reason: it is skipIf-gated on GITNEXUS_BENCH, # so it had never run in CI and the PR #1990 ADL emit-scaling guard it diff --git a/gitnexus/bench/receiver-resolution/baseline.json b/gitnexus/bench/receiver-resolution/baseline.json index 7a427598f..cf8b8abeb 100644 --- a/gitnexus/bench/receiver-resolution/baseline.json +++ b/gitnexus/bench/receiver-resolution/baseline.json @@ -200,11 +200,11 @@ }, "countArm": { "callDrops": 102, - "totalDropsAllKinds": 140, + "totalDropsAllKinds": 148, "bySiteKind": { "call": 102, "read": 27, - "write": 11 + "write": 19 }, "callDropsByExtension": { ".java": 49, diff --git a/gitnexus/bench/scope-capture/baselines.json b/gitnexus/bench/scope-capture/baselines.json index 5b8de09a9..aee412fc0 100644 --- a/gitnexus/bench/scope-capture/baselines.json +++ b/gitnexus/bench/scope-capture/baselines.json @@ -159,7 +159,7 @@ "_rebaselined_2766_receiver_chain_wire_v2": "#2766: receiver-chain wire format v1 -> v2 (name-free `await` / `index` step kinds). The VERSION prefix is part of every emitted `@reference.receiver-chain` capture, so every chain-minting language's capture text changed. WIRE-FORMAT CHANGE, NOT A CAPTURE-SET CHANGE: the same chains are minted for the same sites, spelled `2|…` instead of `1|…`. Exactly the 12 chain-minting languages drifted; c, cobol and dart did not, which is the check that this is the prefix and not a capture regression. Accompanied by SCHEMA_BUMP 34 -> 37 and INCREMENTAL_SCHEMA_VERSION 28 -> 31 so a stale index is rejected rather than replaying chains a v2 decoder refuses. Prior 3ca67847ea2b9a71b0a41e09f943767e5a2d3a113d3e203499ee364e37f40236 -> 8c50bbc83dff4f7f5abd06078aa6abc6b64af05fddb17ee826b5f3df3d346633." }, "typescript": { - "fingerprint": "f66a3e6f1e096431e7046505129a627deaa00ca0de5bc846b080591b397248f7", + "fingerprint": "c2fbf8a89e5686dd1ff3659b20d41d8b05ebcc9790356e3653ee0c8ca5d365c8", "scaling_budget": 1.5, "_rebaselined_callable_flow_2522_review": "PR #2522 review hardening: callable operands retain expression/qualified identity and formals retain signature metadata. Prior 27f937bfb47d4bded316ea3c785ff659c8cd88a5761d928f113477a08c802c78 -> e05446620c5b80b7aae291cfdf32f693580fada2ae687124769b04a0c03bfe63; scaling 0.983 < 1.5.", "_rebaselined_callable_flow_2522_followup": "PR #2522 follow-up: lexical callable bindings, direct-callee argument metadata, and invocation-result suppression. Prior db5933cc6760234ed7d495123410feba6de243646d583f20d43032b9459f81fd -> 27f937bfb47d4bded316ea3c785ff659c8cd88a5761d928f113477a08c802c78; scaling 0.975 < 1.5.", @@ -175,9 +175,11 @@ "_rebaselined_ts_heritage_2842": "#2842 review: TypeScript heritage capture now emits `@reference.inherits` for `interface_declaration` (bases on `extends_type_clause`) and `abstract_class_declaration` (bases on `class_heritage`), which were both silently skipped — so `interface B extends A` and `abstract class X implements I` produced no edge and every interface-dispatch walk dead-ended on a bodiless declaration. Verified before re-baselining by diffing the capture-name histogram over this same fixture corpus (145 files) with and without the change: the ONLY deltas are @reference.inherits 17 -> 20 (+3) and its paired @reference.name 245 -> 248 (+3), emitted together by emitTsInheritanceBase. Every other capture count is byte-identical, so no existing capture moved. The +3 is the three `interface X extends BasePayload` declarations in typescript-generic-calls/src/{auth,admin,guest}.ts. javascript is unchanged (no interfaces in the language). Prior 248b56f0d7a0a6fc7a949dc7afb8611e135ed642bccc2631b96ebb9d686bb965 -> 7a960908031331360ce582f5b55b7681e1cd7f8a2eabfd73c00982cb17f2a949.", "capture_groups_small": 4503, "capture_groups_large": 14403, - "capture_groups_fp": 2338, - "fixture_count": 151, - "_rebaselined_blind_spots_2856": "#2856 blind-spots series: the JS/TS SCOPE queries gained capture rules, so fingerprint drift is expected and additive. Verified before re-baselining by diffing the capture-name sets in both scope queries against origin/main: TypeScript gained exactly @reference.read.identifier (A2 bare-identifier reads in value positions) and @reference.type (R2-2 type references, so a declared contract stops reporting incoming:{}); JavaScript gained exactly @reference.read.identifier, @reference.read.destructured (R2-1c) and @reference.write.property-key (R2-1b record-construction writes). NOTHING was removed on either side — the delta is a pure superset, which is the check that no existing capture moved. capture_groups_small/large are unchanged (4503/14403) because those measure the SYNTHETIC scaling source, which this branch does not touch; only the fixture-corpus count moves. capture_groups_fp 2097 -> 2338 and fixture_count 146 -> 151 from 21 new lang-resolution fixtures. Scaling stayed linear and inside budget: typescript 1.116 < 1.5, javascript 1.010 < 1.5. Prior typescript ed92588e0fc7b28b3a0174339ac378b4dd85965fe007db1208dea97a65ce0571 -> f66a3e6f1e096431e7046505129a627deaa00ca0de5bc846b080591b397248f7; prior javascript 806f70ad3cce5fc849f6d06a08ace8a95f92a1ea84a2418fddabb1eef5846594 -> 2026993b81b873839dd2ef8797d9c14d9c48516b2b57b05ac17d8d43f2f4eba3." + "capture_groups_fp": 2414, + "fixture_count": 155, + "_rebaselined_blind_spots_2856": "#2856 blind-spots series: the JS/TS SCOPE queries gained capture rules, so fingerprint drift is expected and additive. Verified before re-baselining by diffing the capture-name sets in both scope queries against origin/main: TypeScript gained exactly @reference.read.identifier (A2 bare-identifier reads in value positions) and @reference.type (R2-2 type references, so a declared contract stops reporting incoming:{}); JavaScript gained exactly @reference.read.identifier, @reference.read.destructured (R2-1c) and @reference.write.property-key (R2-1b record-construction writes). NOTHING was removed on either side — the delta is a pure superset, which is the check that no existing capture moved. capture_groups_small/large are unchanged (4503/14403) because those measure the SYNTHETIC scaling source, which this branch does not touch; only the fixture-corpus count moves. capture_groups_fp 2097 -> 2338 and fixture_count 146 -> 151 from 21 new lang-resolution fixtures. Scaling stayed linear and inside budget: typescript 1.116 < 1.5, javascript 1.010 < 1.5. Prior typescript ed92588e0fc7b28b3a0174339ac378b4dd85965fe007db1208dea97a65ce0571 -> f66a3e6f1e096431e7046505129a627deaa00ca0de5bc846b080591b397248f7; prior javascript 806f70ad3cce5fc849f6d06a08ace8a95f92a1ea84a2418fddabb1eef5846594 -> 2026993b81b873839dd2ef8797d9c14d9c48516b2b57b05ac17d8d43f2f4eba3.", + "_rebaselined_type_parameter_shadowing_w2_8": "W2-8: `@declaration.type-parameters` is now captured on generic FUNCTIONS, generator functions and type ALIASES, not only on class/interface declarations. NO NEW CAPTURE NAME — verified by diffing the capture-name sets against the wave-1 branch, which returns empty; the tag already existed and simply fires on more declarations. That is the whole delta: capture_groups_fp 2338 -> 2371 (+33 occurrences of an existing tag) and fixture_count 151 -> 152 (one new fixture, typescript-type-parameters). capture_groups_small/large unchanged at 4503/14403, since those measure the synthetic scaling source this does not touch. Scaling 1.06 < 1.5. JavaScript is untouched — it has no type parameters — and its fingerprint does not move, which is the check that this is the TS declaration rules and not something broader. Prior f66a3e6f1e096431e7046505129a627deaa00ca0de5bc846b080591b397248f7 -> 62c7f1bfbe568eed927fb78f00061ed5e49d12511fd8260648b876df386f3b4c.", + "_rebaselined_2899_review_type_parameter_scope_fixtures": "PR #2899 review follow-up: FIXTURE-CORPUS GROWTH ONLY — no query rule changed and no capture name was added or removed. `typescript/query.ts` is byte-identical to the previous baseline; the type-parameter shadowing defect was fixed on the RESOLUTION side (`walkers.ts` gains a `declarationOpenedScope` gate so a declaration's `typeParameters` bind only inside the scope that declaration opened, and the `USES` guard moved from `graph-bridge/references-to-edges.ts` to `resolve-references.ts` where the spelled `site.name` is in hand). The fingerprint moves because measure.mjs fingerprints the whole `lang-resolution/typescript-*` fixture corpus and the regression tests add three files to `typescript-type-parameters/src/` (values.ts, aliased.ts, namespaced.ts) plus two scope-less generic aliases in shapes.ts. Per-file accounting sums exactly to the delta: shapes.ts 33->35 (+2), values.ts +11, aliased.ts +10, namespaced.ts +20 = +43. capture_groups_fp 2371 -> 2414; fixture_count 152 -> 155. capture_groups_small/large unchanged at 4503/14403 (they measure the SYNTHETIC scaling source, untouched). JAVASCRIPT IS THE CONTROL AND DID NOT MOVE (fingerprint 2026993b..., 43 fixtures) — which is the check that this is corpus growth and not a capture regression; all 14 other languages report `ok`. Scaling 0.976 < 1.5. Prior 62c7f1bfbe568eed927fb78f00061ed5e49d12511fd8260648b876df386f3b4c -> c2fbf8a89e5686dd1ff3659b20d41d8b05ebcc9790356e3653ee0c8ca5d365c8." }, "javascript": { "fingerprint": "2026993b81b873839dd2ef8797d9c14d9c48516b2b57b05ac17d8d43f2f4eba3", diff --git a/gitnexus/src/cli/ai-context.ts b/gitnexus/src/cli/ai-context.ts index f3ab38cf1..e78130a14 100644 --- a/gitnexus/src/cli/ai-context.ts +++ b/gitnexus/src/cli/ai-context.ts @@ -211,6 +211,7 @@ This project is indexed by GitNexus as **${projectName}**${noStats ? '' : ` (${s } - **MUST analyze graph changes before committing.** Use \`detect_changes({scope: "all"})\` (MCP) or \`${runner} detect-changes --scope all --repo .\` (CLI fallback). For regression review: \`detect_changes({scope: "compare", base_ref: ${JSON.stringify(markdownSafeBranch(defaultBranch))}})\` or \`${runner} detect-changes --scope compare --base-ref ${JSON.stringify(markdownSafeBranch(defaultBranch))} --repo .\`. - **MUST warn the user** if impact analysis returns HIGH or CRITICAL risk before proceeding with edits. +- **MUST treat \`risk: UNKNOWN\` as unresolved, not as low.** An empty caller set is not evidence the symbol is unused — it can also mean the callers are not resolvable by the index (plain-object property access, dynamic dispatch, cross-language calls). \`impact\` pairs \`UNKNOWN\` with a \`riskNote\` saying so. Confirm with a text search before treating the symbol as safe to change or delete; do not proceed on the strength of a zero. - When exploring unfamiliar code, use \`query({search_query: "concept"})\` to find execution flows instead of grepping. It returns process-grouped results ranked by relevance. - When you need full context on a specific symbol — callers, callees, which execution flows it participates in — use \`context({name: "symbolName"})\`. - For security review, \`explain({target: "fileOrSymbol"})\` lists taint findings (source→sink flows; needs \`analyze --pdg\`).${ @@ -222,7 +223,7 @@ This project is indexed by GitNexus as **${projectName}**${noStats ? '' : ` (${s ## Never Do - NEVER edit a function, class, or method before MCP/CLI impact analysis. -- NEVER ignore HIGH or CRITICAL risk warnings from impact analysis. +- NEVER ignore HIGH or CRITICAL risk warnings from impact analysis, and never read \`UNKNOWN\` as an all-clear — it means the walk could not answer, which is the one verdict that requires confirming by other means. - NEVER rename symbols with find-and-replace — use \`rename\` which understands the call graph. - NEVER commit before MCP/CLI graph change analysis. diff --git a/gitnexus/src/core/index-freshness.ts b/gitnexus/src/core/index-freshness.ts index e34d371c2..ea577f834 100644 --- a/gitnexus/src/core/index-freshness.ts +++ b/gitnexus/src/core/index-freshness.ts @@ -24,6 +24,32 @@ export const GRAPH_WRITE_COLLAPSE_RATIO = 0.5; */ export const GRAPH_WRITE_COLLAPSE_MIN_EDGES = 100; +/** Why {@link detectGraphWriteCollapse} could reach no verdict at all. */ +export type GraphWriteCollapseUnmeasurableReason = + /** The pipeline's own total was not a usable number (or was zero). */ + | 'expected-unavailable' + /** The DB-side count could not be READ — a query that threw, no connection. */ + | 'persisted-unreadable' + /** Set by the CALLER: an incremental write persists only the changed + * subgraph, so whole-scope counts are not comparable to it. */ + | 'incremental-write'; + +/** + * The three outcomes of the collapse check, kept APART because two of them used + * to share `undefined` and the conflation erased a stamp recording real, + * unrepaired edge loss. + * + * `'healthy'` is a POSITIVE all-clear — the counts were both taken and enough + * rows persisted — and is the only outcome that licenses clearing a previous + * `graph-write-collapsed` stamp. `'unmeasurable'` says the comparison never + * happened; the previous stamp must survive it, because nothing has repaired + * whatever it recorded. + */ +export type GraphWriteCollapseVerdict = + | { verdict: 'collapsed'; expected: number; persisted: number } + | { verdict: 'healthy' } + | { verdict: 'unmeasurable'; reason: GraphWriteCollapseUnmeasurableReason }; + /** * Decide whether a finished write collapsed, comparing what the pipeline * produced against what the DB hands back. @@ -34,7 +60,15 @@ export const GRAPH_WRITE_COLLAPSE_MIN_EDGES = 100; * * FAIL-SAFE at `expected === 0`: an implementation that offloads relationships * out of memory may not be able to report a total, and a false "your index is - * broken" is worse than a missed one. + * broken" is worse than a missed one. That case is `'unmeasurable'`, NOT + * `'healthy'` — nothing was compared, so nothing was cleared. + * + * Returns a THREE-WAY verdict rather than `{...} | undefined`. The absent value + * meant both "measured, fine" and "could not measure", and the caller — which + * decides whether to keep or erase the persisted `graph-write-collapsed` stamp — + * cannot tell those apart from a shared `undefined`. It guessed by write mode + * instead, so a full run whose structural count threw took the + * "no collapse ⇒ clear it" branch and deleted a stamp recording real loss. */ export function detectGraphWriteCollapse( expected: number, @@ -50,7 +84,7 @@ export function detectGraphWriteCollapse( * same confident-zero error it exists to catch. */ persisted: number | undefined, -): { expected: number; persisted: number } | undefined { +): GraphWriteCollapseVerdict { // Both sides must be REAL NUMBERS before any comparison. A non-numeric // `expected` (a graph implementation that reports no total, a lightweight // pipeline result) does not merely skip the guards — it INVERTS them: @@ -59,23 +93,38 @@ export function detectGraphWriteCollapse( // "passes" too and a healthy run is reported as a total collapse. Comparing // against a non-number is the one way this check can manufacture the exact // false certainty it was written to prevent. - if (!Number.isFinite(expected) || typeof persisted !== 'number' || !Number.isFinite(persisted)) { - return undefined; + if (!Number.isFinite(expected)) { + return { verdict: 'unmeasurable', reason: 'expected-unavailable' }; + } + if (typeof persisted !== 'number' || !Number.isFinite(persisted)) { + return { verdict: 'unmeasurable', reason: 'persisted-unreadable' }; } const expectedCount = expected; const persistedCount = persisted; + // FAIL-SAFE, and `'unmeasurable'` rather than `'healthy'`: a zero expectation + // is the documented "could not report a total" case, not evidence the write + // went well. Reporting it as an all-clear would let a run that measured + // nothing erase a stamp recording a previous run's real loss. + if (expectedCount === 0) { + return { verdict: 'unmeasurable', reason: 'expected-unavailable' }; + } // A TOTAL loss is never small enough to excuse. The min-edges exemption // exists for "a handful of edges lost to legitimate filtering", which its own // docstring says — it does not describe a persisted count of zero. Evaluated // before the exemption because the exemption looked only at `expected`: // `expected = 99, persisted = 0` lost every single edge and still returned - // `undefined`, leaving the metadata fresh and the CLI reporting success. + // no verdict, leaving the metadata fresh and the CLI reporting success. if (expectedCount > 0 && persistedCount === 0) { - return { expected: expectedCount, persisted: persistedCount }; + return { verdict: 'collapsed', expected: expectedCount, persisted: persistedCount }; } - if (expectedCount < GRAPH_WRITE_COLLAPSE_MIN_EDGES) return undefined; - if (persistedCount >= expectedCount * GRAPH_WRITE_COLLAPSE_RATIO) return undefined; - return { expected: expectedCount, persisted: persistedCount }; + // The small-repo exemption and the cleared ratio are both `'healthy'`, not + // `'unmeasurable'`: both counts WERE taken, and the comparison ran. Calling + // the exemption a non-verdict would make a stamp unclearable on any repo that + // shrank below the threshold — a permanent forced-rebuild wedge, which is the + // failure this taxonomy exists to avoid rather than to relocate. + if (expectedCount < GRAPH_WRITE_COLLAPSE_MIN_EDGES) return { verdict: 'healthy' }; + if (persistedCount >= expectedCount * GRAPH_WRITE_COLLAPSE_RATIO) return { verdict: 'healthy' }; + return { verdict: 'collapsed', expected: expectedCount, persisted: persistedCount }; } /** Stable machine-readable reasons an index cannot be certified complete. */ diff --git a/gitnexus/src/core/ingestion/languages/typescript/query.ts b/gitnexus/src/core/ingestion/languages/typescript/query.ts index f2d2f22cf..babdb273b 100644 --- a/gitnexus/src/core/ingestion/languages/typescript/query.ts +++ b/gitnexus/src/core/ingestion/languages/typescript/query.ts @@ -175,17 +175,20 @@ export const TYPESCRIPT_SCOPE_QUERY = ` ;; to no label and TypeScript aliases produced NO scope-resolution def at all. ;; Kotlin and Dart already spell it this way. (type_alias_declaration - name: (type_identifier) @declaration.name) @declaration.type_alias + name: (type_identifier) @declaration.name + type_parameters: (type_parameters)? @declaration.type-parameters) @declaration.type_alias (internal_module name: (identifier) @declaration.name) @declaration.namespace ;; Declarations — methods / functions / constructors (function_declaration - name: (identifier) @declaration.name) @declaration.function + name: (identifier) @declaration.name + type_parameters: (type_parameters)? @declaration.type-parameters) @declaration.function (generator_function_declaration - name: (identifier) @declaration.name) @declaration.function + name: (identifier) @declaration.name + type_parameters: (type_parameters)? @declaration.type-parameters) @declaration.function ;; Function overload signatures (declaration-only; body in a separate ;; function_declaration). Extractors dedup by (name, parameterTypes). diff --git a/gitnexus/src/core/ingestion/pipeline-phases/processes.ts b/gitnexus/src/core/ingestion/pipeline-phases/processes.ts index eefce05ea..24117d532 100644 --- a/gitnexus/src/core/ingestion/pipeline-phases/processes.ts +++ b/gitnexus/src/core/ingestion/pipeline-phases/processes.ts @@ -124,6 +124,65 @@ export const processesPhase: PipelinePhase = { ); } + // Not gated on `isDev`: this is the one line that tells a reader the process + // list is a SAMPLE. "823 flows" presented without it reads as the complete + // set, which is the confident-empty failure in its other direction — a + // confident-COMPLETE one. + // + // But it is only a `warn` when a ceiling removed WHOLE FLOWS from the + // report. That split is the one `ProcessTruncationStats` already documents — + // "unexplored entry points mean whole flows are missing, while a + // depth-capped trace means a flow is present but shorter than it really is" + // — and it is what keeps the line worth reading. Warning on every counter + // meant warning on every run: this phase overrides only `maxProcesses`, so + // at the shipped defaults (`maxBranching: 4`, `maxTraceDepth: 10`, + // per-entry trace budget 12) `calleesDropped` fires for any function with + // five callees, `tracesDepthCapped` for any chain deeper than ten, and + // `walksCutByBudget` for any entry point with twelve paths under it. All + // three are true of every non-trivial repository — and none of them removes + // an entry point or a completed flow from the list, they only bound how far + // an already-represented region was walked. A warning that always fires is a + // warning nobody reads, so those three go to `debug`. + // + // `entryPointCandidatesDropped` IS in the loud set even though it fires on + // any repository with more than 200 candidates, because it is the only + // ceiling that grows with the repository: `entryPointsUnexplored` and + // `processesDropped` can only fire while `maxProcesses` (symbols / 10) is + // small enough to bind, so gating on those two alone would go quiet on + // exactly the large repositories where 200 of several thousand entry points + // is the thinnest sample. The message leads with that ratio so the line + // carries a fact rather than an alarm. + // + // `stats.truncation` on the RESULT is untouched and still reports all six + // counters; this only decides which of them are loud. + const { truncation } = processResult.stats; + const entryPointCandidates = + processResult.stats.entryPointsFound + truncation.entryPointCandidatesDropped; + const flowsMissing = + truncation.entryPointCandidatesDropped > 0 || + truncation.entryPointsUnexplored > 0 || + truncation.processesDropped > 0; + const shape = + `${truncation.entryPointCandidatesDropped} of ${entryPointCandidates} candidate entry point(s) never ranked in, ` + + `${truncation.entryPointsUnexplored} ranked entry point(s) never traced, ` + + `${truncation.processesDropped} deduplicated flow(s) dropped at maxProcesses, ` + + `${truncation.tracesDepthCapped} trace(s) cut at maxTraceDepth, ` + + `${truncation.calleesDropped} callee(s) skipped at maxBranching, ` + + `${truncation.walksCutByBudget} walk(s) cut by the per-entry trace budget.`; + if (flowsMissing) { + logger.warn( + { truncation }, + `[processes] ${processResult.stats.totalProcesses} flows reported, but whole flows are MISSING: ` + + `${shape} An absent flow does NOT mean the code path does not exist.`, + ); + } else if (truncation.truncated) { + logger.debug( + { truncation }, + `[processes] ${processResult.stats.totalProcesses} flows reported; every flow found is present, ` + + `but some are shorter than the code path they describe: ${shape}`, + ); + } + processResult.processes.forEach((proc) => { ctx.graph.addNode({ id: proc.id, diff --git a/gitnexus/src/core/ingestion/process-processor.ts b/gitnexus/src/core/ingestion/process-processor.ts index 845232b5e..67804d268 100644 --- a/gitnexus/src/core/ingestion/process-processor.ts +++ b/gitnexus/src/core/ingestion/process-processor.ts @@ -58,6 +58,52 @@ export interface ProcessStep { step: number; // 1-indexed position in trace } +/** + * What the detection ceilings dropped, so a partial answer cannot present + * itself as a complete one. + * + * Every field here was previously either a `logger.debug` line or nothing at + * all: the caps fired, the result came back looking whole, and no consumer + * could tell. A silently truncating cap reads as "this is everything", which is + * the same class of confident-empty answer the rest of this work is about. + * `dispatchFanoutSkipped` / `propertyDispatch.skippedKeys` are the precedent. + * + * The counters are kept SEPARATE rather than summed because they mean different + * things and a reader acts on them differently: unexplored entry points mean + * whole flows are missing, while a depth-capped trace means a flow is present + * but shorter than it really is. `truncated` is the single boolean to branch on. + * That distinction is not decoration — `pipeline-phases/processes.ts` uses it to + * decide which of these are worth a `warn` and which belong at `debug`. + * + * `entryPointCandidatesDropped` was MISSED on the first pass, which is worth + * recording because the comment deriving `truncated` claimed "a new ceiling + * added later cannot be forgotten here" while an EXISTING one already had been: + * `findEntryPoints` ranks every scoring candidate and then keeps 200, so + * `entryPointsUnexplored` — computed over the list it RETURNS — could only ever + * see the survivors. On any repository with more than 200 candidate entry + * points that slice is the dominant ceiling, and it was invisible. + */ +export interface ProcessTruncationStats { + /** True when any ceiling below fired. */ + truncated: boolean; + /** + * Scoring candidates that never reached the trace loop because + * `findEntryPoints` keeps only the top `ENTRY_POINT_CANDIDATE_LIMIT`. + * Counted BEFORE the slice, so it sees what `entryPointsFound` cannot. + */ + entryPointCandidatesDropped: number; + /** Entry points never traced at all — the trace-collection loop stopped first. */ + entryPointsUnexplored: number; + /** Entry-point walks abandoned with branches still on the stack. */ + walksCutByBudget: number; + /** Traces that end at `maxTraceDepth`, i.e. are a PREFIX of a longer flow. */ + tracesDepthCapped: number; + /** Callees never followed because a call site exceeded `maxBranching`. */ + calleesDropped: number; + /** Deduplicated traces discarded because `maxProcesses` was already full. */ + processesDropped: number; +} + export interface ProcessDetectionResult { processes: ProcessNode[]; steps: ProcessStep[]; @@ -66,9 +112,22 @@ export interface ProcessDetectionResult { crossCommunityCount: number; avgStepCount: number; entryPointsFound: number; + /** Additive — existing consumers read the four counters above unchanged. */ + truncation: ProcessTruncationStats; }; } +/** Zeroed counters, mutated in place by the walk. */ +const emptyTruncation = (): ProcessTruncationStats => ({ + truncated: false, + entryPointCandidatesDropped: 0, + entryPointsUnexplored: 0, + walksCutByBudget: 0, + tracesDepthCapped: 0, + calleesDropped: 0, + processesDropped: 0, +}); + // ============================================================================ // MAIN PROCESSOR // ============================================================================ @@ -105,8 +164,12 @@ export const processProcesses = async ( const nodeMap = new Map(); for (const n of knowledgeGraph.iterNodes()) nodeMap.set(n.id, n); + // Declared before Step 1 because `findEntryPoints` has a ceiling of its own + // (see `ENTRY_POINT_CANDIDATE_LIMIT`) and reports it through the same record. + const truncation = emptyTruncation(); + // Step 1: Find entry points (functions that call others but have few callers) - const entryPoints = findEntryPoints(knowledgeGraph, reverseCallsEdges, callsEdges); + const entryPoints = findEntryPoints(knowledgeGraph, reverseCallsEdges, callsEdges, truncation); onProgress?.(`Found ${entryPoints.length} entry points, tracing flows...`, 20); @@ -115,9 +178,11 @@ export const processProcesses = async ( // Step 2: Trace processes from each entry point const allTraces: string[][] = []; + let tracedEntryPoints = 0; for (let i = 0; i < entryPoints.length && allTraces.length < cfg.maxProcesses * 2; i++) { const entryId = entryPoints[i]; - const traces = traceFromEntryPoint(entryId, callsEdges, cfg, isSink); + const traces = traceFromEntryPoint(entryId, callsEdges, cfg, isSink, truncation); + tracedEntryPoints = i + 1; // Filter out traces that are too short traces.filter((t) => t.length >= cfg.minSteps).forEach((t) => allTraces.push(t)); @@ -129,6 +194,14 @@ export const processProcesses = async ( ); } } + // The loop exits on the TRACE quota, not on running out of entry points, so + // the remainder are not "no flows found" — they were never looked at. + // + // Counted over the list `findEntryPoints` RETURNS, which is already capped at + // `ENTRY_POINT_CANDIDATE_LIMIT`; candidates beyond that cap are invisible here + // by construction and are reported separately as + // `entryPointCandidatesDropped`. + truncation.entryPointsUnexplored = entryPoints.length - tracedEntryPoints; onProgress?.(`Found ${allTraces.length} traces, deduplicating...`, 60); @@ -187,12 +260,47 @@ export const processProcesses = async ( // fetch/ORM extraction fires (see `buildSinkFunctionSet`), so a codebase whose // outward calls are not detected as such still sees leaf-terminated traces // only. + // DETERMINISM. The comparator below ranks by sink-ness then by depth, and for + // two flows equal on both it returned 0. `Array.prototype.sort` is stable, so + // a 0 preserves INPUT order — which traces back to `graph.iterNodes()`, i.e. + // the order files happened to be inserted. Under `maxProcesses` capping that + // decided which `Process` and `STEP_IN_PROCESS` nodes were persisted at all. + // + // Reproduced: two equal three-step flows with `maxProcesses: 1` select + // `handleAlpha`; inserting the identical nodes and CALLS edges in reverse + // select `handleBeta`. Same repository, same commit, different persisted + // graph — so an incremental run that reorders assembly, or a filesystem that + // enumerates differently, silently changes what the tool reports. + // + // The id is the tiebreak because it is the only totally-ordered, content-derived + // key available here; comparing the whole path keeps it stable when two traces + // share a terminal. + // + // MUTATION STATUS, recorded so nobody mistakes this for a verified guard: + // removing THIS tiebreak alone fails nothing, because the two dedup sorts + // below already impose a total order on the list that reaches here. The + // entry-point sort and the dedup sorts each ARE individually verified. This + // one is kept as defence in depth — it cannot misbehave (it only makes an + // already-deterministic order explicit) and it is what stops a future change + // to dedup ordering from silently re-opening the defect. + // + // Decorated once and sorted on the precomputed key rather than joining inside + // the comparator — see `traceOrderKey` for why the key exists at all and + // `sortByDepthThenPath` for why it is built exactly once per trace. The sink + // test is hoisted for the same reason: it ran twice per comparison. const tracesByTerminal = new Map(); - const rankedByInterest = [...endpointDeduped].sort((a, b) => { - const aSink = Number(isSink(a[a.length - 1] ?? '')); - const bSink = Number(isSink(b[b.length - 1] ?? '')); - return bSink - aSink || b.length - a.length; - }); + const rankedByInterest = ((): string[][] => { + const decorated = endpointDeduped.map((trace) => ({ + trace, + sink: isSink(trace[trace.length - 1] ?? '') ? 1 : 0, + key: traceOrderKey(trace), + })); + decorated.sort( + (a, b) => + b.sink - a.sink || b.trace.length - a.trace.length || compareOrderKeys(a.key, b.key), + ); + return decorated.map((d) => d.trace); + })(); for (const trace of rankedByInterest) { const terminalId = trace[trace.length - 1]; if (terminalId === undefined) continue; @@ -215,6 +323,10 @@ export const processProcesses = async ( } if (!addedAny) break; } + // Counted against the DEDUPED input, not `allTraces`: the difference between + // those two is deduplication doing its job, which is not truncation. + const dedupedAvailable = [...tracesByTerminal.values()].reduce((n, t) => n + t.length, 0); + truncation.processesDropped = dedupedAvailable - limitedTraces.length; onProgress?.(`Creating ${limitedTraces.length} process nodes...`, 80); @@ -278,6 +390,26 @@ export const processProcesses = async ( ? processes.reduce((sum, p) => sum + p.stepCount, 0) / processes.length : 0; + // Derived last, from the counters the walk accumulated, so a new ceiling added + // later cannot be forgotten here — it only has to increment its own counter. + // + // That claim was wrong when it was written: `findEntryPoints`' 200-candidate + // slice was an EXISTING ceiling with no counter, so it was not merely + // forgettable, it had already been forgotten. Adding a counter is only half + // the discipline; the other half is checking, when you write a line like this, + // that every cap in the file actually has one. + truncation.truncated = + truncation.entryPointCandidatesDropped > 0 || + truncation.entryPointsUnexplored > 0 || + truncation.walksCutByBudget > 0 || + truncation.tracesDepthCapped > 0 || + truncation.calleesDropped > 0 || + truncation.processesDropped > 0; + + if (truncation.truncated) { + logger.debug({ truncation }, 'process-processor: detection was truncated by one or more caps'); + } + return { processes, steps, @@ -286,6 +418,7 @@ export const processProcesses = async ( crossCommunityCount, avgStepCount: Math.round(avgStepCount * 10) / 10, entryPointsFound: entryPoints.length, + truncation, }, }; }; @@ -330,6 +463,13 @@ const buildReverseCallsGraph = (graph: KnowledgeGraph): AdjacencyList => { return adj; }; +/** + * How many ranked candidates survive to be traced. Everything below this line + * is discarded — see `ProcessTruncationStats.entryPointCandidatesDropped`, the + * counter that exists because this cap spent a release being invisible. + */ +const ENTRY_POINT_CANDIDATE_LIMIT = 200; + /** * Find functions/methods that are good entry points for tracing. * @@ -344,6 +484,14 @@ const findEntryPoints = ( graph: KnowledgeGraph, reverseCallsEdges: AdjacencyList, callsEdges: AdjacencyList, + /** + * Mutated in place when the candidate cap fires. Optional and reported + * through an out-parameter rather than a richer return value, matching + * `traceFromEntryPoint`: the `string[]` contract every caller already uses is + * unchanged, and a caller that does not care about completeness does not have + * to unwrap a counter to ask for entry points. + */ + truncation?: ProcessTruncationStats, ): string[] => { const symbolTypes = new Set(['Function', 'Method']); const entryPointCandidates: { @@ -388,8 +536,14 @@ const findEntryPoints = ( } } - // Sort by score descending and return top candidates - const sorted = entryPointCandidates.sort((a, b) => b.score - a.score); + // Sort by score descending, then by node id. Ties on score are common — most + // candidates share a heuristic bucket — and a stable sort resolves them by + // `iterNodes()` order, so which entry points survive the `slice` below became + // a function of file insertion order. See the determinism note on + // `rankedByInterest`. + const sorted = entryPointCandidates.sort( + (a, b) => b.score - a.score || (a.id < b.id ? -1 : a.id > b.id ? 1 : 0), + ); // DEBUG: Log top candidates with new scoring details if (sorted.length > 0 && isDev) { @@ -403,9 +557,17 @@ const findEntryPoints = ( }); } - return sorted - .slice(0, 200) // Limit to prevent explosion - .map((c) => c.id); + // Limit to prevent explosion — and SAY SO. This is the ceiling that decides + // how much of a repository process detection ever looks at: on anything with + // more than 200 scoring candidates the reported flows are a sample of the + // top-ranked ones, and every downstream count (`entryPointsFound`, + // `entryPointsUnexplored`) is computed over the survivors, so none of them can + // see what was cut here. + if (truncation !== undefined && sorted.length > ENTRY_POINT_CANDIDATE_LIMIT) { + truncation.entryPointCandidatesDropped = sorted.length - ENTRY_POINT_CANDIDATE_LIMIT; + } + + return sorted.slice(0, ENTRY_POINT_CANDIDATE_LIMIT).map((c) => c.id); }; // ============================================================================ @@ -432,6 +594,12 @@ export const traceFromEntryPoint = ( * bottomed out in. */ isSink: (nodeId: string) => boolean = () => false, + /** + * Mutated in place when a ceiling fires. Optional so the many direct callers + * in tests are unchanged, and because a caller that does not care about + * completeness should not have to invent a counter to ask for a trace. + */ + truncation?: ProcessTruncationStats, ): string[][] => { const traces: string[][] = []; @@ -468,7 +636,10 @@ export const traceFromEntryPoint = ( traces.push([...path]); } } else if (path.length >= config.maxTraceDepth) { - // Max depth reached - save what we have + // Max depth reached - save what we have. The trace is kept, but it is a + // PREFIX of a longer flow rather than a flow that ended, and only this + // counter distinguishes the two downstream. + if (truncation !== undefined) truncation.tracesDepthCapped++; if (path.length >= config.minSteps) { traces.push([...path]); } @@ -482,6 +653,9 @@ export const traceFromEntryPoint = ( } // Continue tracing - limit branching const limitedCallees = callees.slice(0, config.maxBranching); + if (truncation !== undefined && callees.length > limitedCallees.length) { + truncation.calleesDropped += callees.length - limitedCallees.length; + } let addedBranch = false; // PUSHED IN REVERSE so the stack POPS them in source order. `slice` @@ -511,7 +685,12 @@ export const traceFromEntryPoint = ( // class of confident-empty answer this work is about. The repo already sets // this precedent for `dispatchFanoutSkipped` and // `propertyDispatch.skippedKeys`. + // + // The debug line stays for the per-entry-point detail (which entry, how many + // branches); the counter is what escapes to a CONSUMER. A log nobody has + // enabled is not a disclosure. if (stack.length > 0) { + if (truncation !== undefined) truncation.walksCutByBudget++; logger.debug( { entryId, traceBudget, unexploredBranches: stack.length }, 'process-processor: trace budget exhausted; unexplored branches remain for this entry point', @@ -590,6 +769,57 @@ export function buildSinkFunctionSet( return sinks; } +// ============================================================================ +// HELPER: Deterministic trace ordering +// ============================================================================ + +/** + * Total-order key for a trace — the TIEBREAK every trace sort in this file uses. + * + * NUL is the separator, and that is load-bearing rather than cosmetic. Node ids + * embed file paths and a path may contain a SPACE, so a space-joined key is + * ambiguous in exactly the way `traceKey`'s unpadded `->` join was (#2894): + * `['A B', 'C']` and `['A', 'B C']` both render as `A B C`, the comparator + * returns 0, and `Array.prototype.sort` — being stable — falls straight back to + * the input order the tiebreak exists to remove. Two of the three trace sorts + * here joined on a space and had that hole; all three now share this key. + * + * NUL cannot occur in a node id (not in a POSIX path, not in a source + * identifier) and sorts below every character that can, so joining on it is + * order-equivalent to comparing the two arrays element by element. That + * equivalence is what makes it a drop-in for the space-joined keys: on any + * corpus without the collision above the resulting order is IDENTICAL, which is + * asserted directly in `process-processor.test.ts`. + */ +const traceOrderKey = (trace: readonly string[]): string => trace.join('\u0000'); + +/** Lexicographic compare of two `traceOrderKey` results. */ +const compareOrderKeys = (a: string, b: string): number => (a < b ? -1 : a > b ? 1 : 0); + +/** + * Sort traces deepest-first, breaking ties on the path key. + * + * A Schwartzian transform: the key is built ONCE per trace and the comparator + * only compares two strings. Written the obvious way — `a.join(sep) < + * b.join(sep)` inside the comparator — each comparison allocates up to FOUR + * joined strings, so an O(n log n) sort performs O(n log n) joins of + * O(depth x id-length) characters each. + * + * `n` is bounded here (`ENTRY_POINT_CANDIDATE_LIMIT` entry points x the + * per-entry trace budget), so the cost is small and once-per-analyze: measured + * at the ceiling, 23,851 comparisons performed 70,524 joins, and end-to-end + * `processProcesses` at 80,000 functions / 640k CALLS went 456 -> 555 ms. It is + * fixed anyway because it is the same allocation-in-the-comparator shape that + * was just removed from `deduplicateTraces` below (deep_chain 1233 -> 102 ms), + * and leaving one instance of it standing next to that comment invites the next + * one. + */ +const sortByDepthThenPath = (traces: readonly string[][]): string[][] => { + const decorated = traces.map((trace) => ({ trace, key: traceOrderKey(trace) })); + decorated.sort((a, b) => b.trace.length - a.trace.length || compareOrderKeys(a.key, b.key)); + return decorated.map((d) => d.trace); +}; + // ============================================================================ // HELPER: Deduplicate traces // ============================================================================ @@ -598,15 +828,18 @@ export function buildSinkFunctionSet( * Merge traces that are subsets of other traces. * Keep longer traces, remove redundant shorter ones. */ -const deduplicateTraces = ( +export const deduplicateTraces = ( traces: string[][], /** See `buildSinkFunctionSet` — a sink-terminated trace survives subsumption. */ isSink: (nodeId: string) => boolean = () => false, ): string[][] => { if (traces.length === 0) return []; - // Sort by length descending - const sorted = [...traces].sort((a, b) => b.length - a.length); + // Sort by length descending, then by path, so equal-length traces have a + // total order instead of inheriting graph-traversal order (determinism note + // on `rankedByInterest`). Which of two equal traces is kept as the + // representative is otherwise decided by insertion order. + const sorted = sortByDepthThenPath(traces); const unique: string[][] = []; // Keys for `unique`, built ONCE per surviving trace rather than once per // COMPARISON. The join used to sit inside the `some()` callback below, so @@ -630,7 +863,23 @@ const deduplicateTraces = ( // from ever being processes. Emitting one at the walk and deleting it one // step later would have been a no-op fix. const terminal = trace[trace.length - 1]; - const traceKey = trace.join('->'); + // PADDED with the separator on both ends, so `includes` can only match whole + // steps (#2894). Unpadded, the test is not anchored to a step boundary and a + // match may begin in the MIDDLE of a node id: + // + // 'X->AA->B'.includes('A->B') -> true + // + // which discards `A -> B` as redundant against a chain `A` is not a step of + // at all. Measured inert on every corpus tried — the collision needs one id + // to be a strict suffix of another, which real ids + // (`Function::`) do not produce — but the predicate did not mean + // what the surrounding code says it means, and this is a function whose + // entire job is deciding what to delete. + // + // Note the encoding assumes `->` never appears IN a node id. A C++ + // `operator->` would defeat the join regardless of padding; out of scope + // here, but the assumption is real. + const traceKey = `->${trace.join('->')}->`; if (terminal !== undefined && isSink(terminal)) { unique.push(trace); uniqueKeys.push(traceKey); @@ -660,8 +909,10 @@ const deduplicateByEndpoints = (traces: string[][]): string[][] => { if (traces.length === 0) return []; const byEndpoints = new Map(); - // Sort longest first so the first seen per key is the longest - const sorted = [...traces].sort((a, b) => b.length - a.length); + // Sort longest first so the first seen per key is the longest; the path + // tiebreak makes "which of two equal-length traces represents this endpoint + // pair" independent of insertion order. + const sorted = sortByDepthThenPath(traces); for (const trace of sorted) { const key = `${trace[0]}::${trace[trace.length - 1]}`; diff --git a/gitnexus/src/core/ingestion/resolve-references.ts b/gitnexus/src/core/ingestion/resolve-references.ts index b34bff49d..a7a2cf0a3 100644 --- a/gitnexus/src/core/ingestion/resolve-references.ts +++ b/gitnexus/src/core/ingestion/resolve-references.ts @@ -59,6 +59,7 @@ import { type ScopeId, } from 'gitnexus-shared'; import type { ScopeResolutionIndexes } from './model/scope-resolution-indexes.js'; +import { bindsTypeParameter } from './scope-resolution/scope/walkers.js'; // ─── Public API ───────────────────────────────────────────────────────────── @@ -73,7 +74,12 @@ export interface ResolveReferencesInput { export interface ResolveStats { readonly sitesProcessed: number; readonly referencesEmitted: number; - /** Sites where `Registry.lookup` returned no candidates. */ + /** + * Sites that produced no `Reference`. Almost always "the registry returned no + * candidates", but it also counts a site declined before lookup because the + * name is bound as a type parameter here (#2899) — a shadowed annotation names + * no symbol in the graph, so "resolved to nothing" is the honest bucket for it. + */ readonly unresolved: number; } @@ -128,6 +134,7 @@ export function resolveReferenceSites(input: ResolveReferencesInput): ResolveRef methodRegistry, fieldRegistry, macroRegistry, + scopes, ); if (resolutions.length === 0) { unresolved++; @@ -176,7 +183,7 @@ export function resolveReferenceSites(input: ResolveReferencesInput): ResolveRef * |------------------|-------------------|------------------------------| * | `call` | MethodRegistry | METHOD_KINDS (Method/Func/Ctor) * | `inherits` | ClassRegistry | CLASS_KINDS | - * | `type-reference` | ClassRegistry | CLASS_KINDS | + * | `type-reference` | ClassRegistry | CLASS_KINDS (type-parameter shadow guard, #2899) | * | `read`/`write` | FieldRegistry | FIELD_KINDS | * | `import-use` | tiered fallback | METHOD ∪ CLASS ∪ FIELD | * | `value-ref` | (skipped here) | post-finalize walker in `emitPropertyDispatchCalls` | @@ -199,6 +206,7 @@ function lookupForSite( methodRegistry: MethodRegistry, fieldRegistry: FieldRegistry, macroRegistry: MacroRegistry, + scopes: ScopeResolutionIndexes, ): readonly Resolution[] { switch (site.kind) { case 'call': { @@ -208,8 +216,49 @@ function lookupForSite( }; return methodRegistry.lookup(site.name, site.inScope, opts); } - case 'inherits': + case 'inherits': { + return classRegistry.lookup(site.name, site.inScope); + } case 'type-reference': { + // A TYPE PARAMETER SHADOWS A DECLARED TYPE OF THE SAME NAME (#2899). + // + // `export function unwrap(value: Result): Result` names the + // parameter, not the `interface Result` next to it — tsc resolves BOTH + // annotations to the parameter. Nothing in the type-reference path knew + // that a parameter binds a name, so each annotation minted a `USES` edge + // into the interface at the same confidence as a real consumer and + // indistinguishable from one. Blast radius is every generic whose + // parameter name collides with a declared type — `Result`, `Key`, + // `Value`, `Item`, `Node`, `Options`, `Config`, `Props`, `State`. + // + // ASKED HERE, ON `site.name`, BECAUSE SHADOWING IS A PROPERTY OF THE NAME + // WRITTEN AT THE SITE. This is the last point that still holds the + // spelling — a `Reference` keeps only the resolved def — so a guard placed + // after resolution has to substitute the DEF's name for the written one + // and is then wrong in BOTH directions from the same substitution. It + // deletes a genuine edge wherever the two differ and the def's name + // happens to match a parameter (`import { Payload as ApiPayload }` written + // inside `pluck`), and it keeps a false one wherever the def's + // name is qualified or position-suffixed and the written name is the + // parameter (`Inner` resolving to `Host.Inner`, or a function-local + // `Result@12:4`). Neither failure is recoverable from the resolved id, + // because the information the question needs was never in it. + // + // TYPE REFERENCES ONLY, which is what asking on `kind` rather than on the + // emitted EDGE TYPE buys. `mapReferenceKindToEdgeType` folds `value-ref` + // (#2437) and `macro` (#1934) into the same `USES` edge, and neither is a + // type annotation — a value or a macro whose name collides with an + // enclosing type parameter is a different construct in a different + // namespace, and dropping it would be a second false-negative class + // bought with the fix for the first. + // + // Reuses the predicate #2833 introduced for the CALL-receiver path, which + // stopped a workspace `class T` answering for `` but never reached type + // references. Absence is not evidence there and is not here: + // `typeParameters` is populated only by languages whose captures were + // extended for it, so a POSITIVE match declines and an absent list changes + // nothing — which is what keeps every unconverted language unchanged. + if (bindsTypeParameter(site.inScope, site.name, scopes)) return []; return classRegistry.lookup(site.name, site.inScope); } case 'read': diff --git a/gitnexus/src/core/ingestion/route-extractors/dispatch-guard.ts b/gitnexus/src/core/ingestion/route-extractors/dispatch-guard.ts index a322a608a..d4a46a012 100644 --- a/gitnexus/src/core/ingestion/route-extractors/dispatch-guard.ts +++ b/gitnexus/src/core/ingestion/route-extractors/dispatch-guard.ts @@ -342,7 +342,7 @@ function verbFromComparison(node: SyntaxNode): string | null { } /** - * Find the verb that governs a path comparison, by walking outward. + * Find the verbs that govern a path comparison, by walking outward. * * Two idioms, both common and both handled: * `if (req.method === 'GET' && pathname === '/x')` — a sibling in the same @@ -355,8 +355,14 @@ function verbFromComparison(node: SyntaxNode): string | null { * `if (req.method === 'POST') {…} else if (pathname === '/x')` the path * comparison is reached precisely when the method is NOT POST, so attributing * POST to it would be exactly backwards. + * + * Returns a LIST because one guard can serve several methods: + * `if ((req.method === 'GET' || req.method === 'POST') && pathname === '/x')` is + * two routes, and returning the first verb reported it as GET-only — a route + * that silently loses its other methods reads as a narrower contract than the + * code implements. Empty means "no verb is guaranteed", which stays verb-less. */ -function governingVerb(comparison: SyntaxNode): string | null { +function governingVerbs(comparison: SyntaxNode): readonly string[] { let current: SyntaxNode = comparison; let parent = current.parent; @@ -369,8 +375,8 @@ function governingVerb(comparison: SyntaxNode): string | null { parent.childForFieldName('left')?.id === current.id ? parent.childForFieldName('right') : parent.childForFieldName('left'); - const verb = sibling === null ? null : findVerbInSubtree(sibling); - if (verb !== null) return verb; + const verbs = sibling === null ? [] : findVerbsInSubtree(sibling); + if (verbs.length > 0) return verbs; } if (parent.type === 'if_statement') { const alternative = parent.childForFieldName('alternative'); @@ -379,29 +385,163 @@ function governingVerb(comparison: SyntaxNode): string | null { // A comparison inside the condition itself is handled by the `&&` rule // above; here we only inherit from an ENCLOSING if we are governed by. if (!inElseBranch && condition !== null && condition.id !== current.id) { - const verb = findVerbInSubtree(condition); - if (verb !== null) return verb; + const verbs = findVerbsInSubtree(condition); + if (verbs.length > 0) return verbs; } } current = parent; parent = current.parent; } - return null; + return []; } -/** First verb comparison anywhere in this subtree. */ -function findVerbInSubtree(node: SyntaxNode): string | null { - // A verb under a `!` is the verb the branch EXCLUDES. Returning null keeps the - // route (the path evidence is unaffected) and leaves it verb-less, which is - // the honest answer: this branch does not tell us which method it serves. - if (isNegation(node)) return null; - const direct = verbFromComparison(node); - if (direct !== null) return direct; - for (const child of node.namedChildren) { - const found = findVerbInSubtree(child); - if (found !== null) return found; +/** + * The verb a subtree GUARANTEES when it evaluates truthy. + * + * `negated` counts whether an odd number of `!` stands between the question and + * this node. It is PARITY, the same rule `isNegatedContext` states — and the + * rule the previous presence-based check contradicted: it returned null at the + * first `!` it saw, so `!!(req.method === 'GET')` lost a verb the source states + * outright. A stated invariant with half an implementation, in the same module + * that had already been fixed for exactly that once. + * + * A verb reached at odd parity is the verb the branch EXCLUDES, so it yields + * nothing — the route survives, verb-less, which is the honest answer: this + * branch does not say which method it serves. Siblings are still searched, + * because excluding one verb says nothing about the next. + */ +function findVerbsInSubtree(node: SyntaxNode, negated = false): readonly string[] { + if (isNegation(node)) { + const operand = node.childForFieldName('argument'); + return operand === null ? [] : findVerbsInSubtree(operand, !negated); } - return null; + + // A ternary SELECTS between its arms, so a verb inside one is not reached + // merely because the whole is truthy — see `verbsFromTernary`. + if (node.type === 'ternary_expression') return verbsFromTernary(node, negated); + + if (isDisjunction(node)) return verbsFromDisjunction(node, negated); + + const direct = verbFromComparison(node); + if (direct !== null) return negated ? [] : [direct]; + + // Generic descent keeps FIRST-match rather than unioning across children: an + // arbitrary node says nothing about how its children combine, and two verbs + // found under one are far more likely to be unrelated than alternatives. The + // one construct that genuinely means "either of these" is `||`, handled above. + for (const child of node.namedChildren) { + const found = findVerbsInSubtree(child, negated); + if (found.length > 0) return found; + } + return []; +} + +/** A logical `||`. */ +function isDisjunction(node: SyntaxNode): boolean { + return node.type === 'binary_expression' && node.childForFieldName('operator')?.text === '||'; +} + +/** + * The verbs a disjunction guarantees — ALL of them, or none. + * + * `req.method === 'GET' || req.method === 'POST'` is the multi-method guard, and + * every operand names a verb, so the guard serves exactly those two. + * + * `req.method === 'GET' || isAdmin` is not: the branch is reached for ANY method + * when `isAdmin` holds, so the honest answer is no verb at all. Reporting `GET` + * — which is what taking the first match did — presents a route open to every + * method as one restricted to a single method, and this module's whole bar is + * that a wrong answer costs more than a missing one. + * + * So: every operand must yield at least one verb, or the whole disjunction + * yields none. At odd parity `!(A || B)` is `!A && !B`, which excludes verbs + * rather than offering them, so nothing is guaranteed either. + */ +function verbsFromDisjunction(node: SyntaxNode, negated: boolean): readonly string[] { + if (negated) return []; + const operands = [node.childForFieldName('left'), node.childForFieldName('right')]; + const collected: string[] = []; + for (const operand of operands) { + if (operand === null) return []; + const verbs = findVerbsInSubtree(operand, false); + if (verbs.length === 0) return []; + for (const verb of verbs) if (!collected.includes(verb)) collected.push(verb); + } + return collected; +} + +/** + * The verbs BOTH operands of a conjunction guarantee — their INTERSECTION. + * + * `A && B` is reached only when each side holds, so the methods it serves are + * the methods they agree on. Taking the first non-empty side instead — which is + * what {@link verbsFromTernary} did — reports one operand's set unintersected: + * `(GET || POST) ? (POST || PUT) : false` emitted GET and POST where only POST + * can reach the body, so the GET route was invented outright. + * + * An EMPTY side is "this operand names no method", not "this operand admits + * none", so it yields to the other rather than annihilating it — that is the + * `isAdmin && req.method === 'POST'` shape, and it is the whole reason the + * fallthrough existed. An empty INTERSECTION of two non-empty sides is the + * opposite: two conflicting method assertions, a guard nothing can satisfy. No + * verb is honest there, and the route survives verb-less, which is this + * module's stated direction for "cannot prove it". + */ +function intersectVerbs(a: readonly string[], b: readonly string[]): readonly string[] { + if (a.length === 0) return b; + if (b.length === 0) return a; + return a.filter((verb) => b.includes(verb)); +} + +/** + * The verb a ternary guarantees — which is one only when an arm is a boolean + * literal, because that is what collapses the selection into a conjunction: + * + * c ? A : false ≡ c && A both hold, so INTERSECT both + * c ? false : B ≡ !c && B c must NOT hold, so search it at flipped parity + * c ? true : B ≡ c || B a disjunction guarantees neither operand + * c ? A : true ≡ !c || A likewise + * + * With two non-literal arms the verb is chosen by a condition whose value is + * unknown, so the ternary guarantees nothing. + * + * The two conjunctions intersect rather than take the first side that names a + * verb — see {@link intersectVerbs} for the route that mistake invented. The + * `||` rule below is the mirror image and already had it right: a disjunction + * UNIONS its operands, all-or-nothing. + * + * Measured before fixing: `(req.method === 'GET' ? false : true) && pathname === + * '/api/i'` emitted `GET /api/i` — the one method that branch guarantees the + * request does NOT have, the same inversion `!` produced before `d4dcba8c`. The + * three shapes that were already right stay right; refusing every ternary would + * have been safe but would have dropped them. + * + * At odd parity every conjunction above becomes a disjunction (De Morgan) and + * guarantees nothing, so a negated ternary yields no verb. `!(c ? false : true)` + * is really `c` and could be read, but it needs BOTH arms folded as literals to + * see that, and no such condition has been observed in a real dispatcher. + * Declining is the safe direction: a missing verb, not an inverted one. + */ +function verbsFromTernary(node: SyntaxNode, negated: boolean): readonly string[] { + if (negated) return []; + const condition = unparenthesize(node.childForFieldName('condition')); + const consequence = unparenthesize(node.childForFieldName('consequence')); + const alternative = unparenthesize(node.childForFieldName('alternative')); + if (condition === null || consequence === null || alternative === null) return []; + + if (alternative.type === 'false') { + return intersectVerbs( + findVerbsInSubtree(condition, false), + findVerbsInSubtree(consequence, false), + ); + } + if (consequence.type === 'false') { + return intersectVerbs( + findVerbsInSubtree(condition, true), + findVerbsInSubtree(alternative, false), + ); + } + return []; } /** @@ -413,41 +553,128 @@ function findVerbInSubtree(node: SyntaxNode): string | null { * repo's route modules are written. */ function enclosingHandlerName(node: SyntaxNode): string | undefined { - let current: SyntaxNode | null = node.parent; - while (current !== null) { - if (FUNCTION_NODE_TYPES.has(current.type)) { - const own = current.childForFieldName('name'); - if (own !== null) return own.text; - const parent = current.parent; - if (parent === null) return undefined; - if (parent.type === 'variable_declarator' || parent.type === 'pair') { - const bound = parent.childForFieldName('name') ?? parent.childForFieldName('key'); - return bound?.text; - } - if (parent.type === 'assignment_expression') { - const left = parent.childForFieldName('left'); - if (left === null) return undefined; - return left.type === 'member_expression' - ? (left.childForFieldName('property')?.text ?? undefined) - : left.text; - } - return undefined; - } - current = current.parent; + const fn = enclosingFunction(node); + if (fn === null) return undefined; + const own = fn.childForFieldName('name'); + if (own !== null) return own.text; + const parent = fn.parent; + if (parent === null) return undefined; + if (parent.type === 'variable_declarator' || parent.type === 'pair') { + const bound = parent.childForFieldName('name') ?? parent.childForFieldName('key'); + return bound?.text; + } + if (parent.type === 'assignment_expression') { + const left = parent.childForFieldName('left'); + if (left === null) return undefined; + return left.type === 'member_expression' + ? (left.childForFieldName('property')?.text ?? undefined) + : left.text; } return undefined; } +/** + * The function this node sits in, or `null` at module scope. + * + * ONE traversal, three readers: the handler name above, the scope half of a + * match-binding key, and the chain an assignment can rebind. They have to agree + * on where a function begins or "the same name in the same function" stops + * meaning one thing, so they share the walk rather than each re-deriving it. + */ +function enclosingFunction(node: SyntaxNode): SyntaxNode | null { + let current: SyntaxNode | null = node.parent; + while (current !== null) { + if (FUNCTION_NODE_TYPES.has(current.type)) return current; + current = current.parent; + } + return null; +} + +/** Names bound outside every function share this scope. */ +const MODULE_SCOPE_ID = -1; + +/** The scope half of a binding key: the id of the function this node lives in. */ +function enclosingScopeId(node: SyntaxNode): number { + return enclosingFunction(node)?.id ?? MODULE_SCOPE_ID; +} + +/** + * Every scope an assignment written here could be rebinding — its own function, + * then outward. `m = x` inside a callback rebinds the `m` of whichever enclosing + * function declared it, and this module does not resolve which, so an assignment + * is taken to reach all of them. + */ +function enclosingScopeIds(node: SyntaxNode): number[] { + const ids: number[] = []; + for (let fn = enclosingFunction(node); fn !== null; fn = enclosingFunction(fn)) ids.push(fn.id); + ids.push(MODULE_SCOPE_ID); + return ids; +} + +/** + * A binding key: the function a name is bound in, plus the name. + * + * The scope id is a number and NUL cannot appear in an identifier, so the two + * halves cannot run together into a collision. Written as the `\u0000` ESCAPE, + * never a raw NUL byte: a literal NUL makes the source a binary file to git, + * grep and every other line-oriented tool. + */ +function bindingKey(scopeId: number, name: string): string { + return `${scopeId}\u0000${name}`; +} + +/** + * Every name a binding pattern introduces — `m`, `{ m }`, `{ a: m }`, `[m]`, + * `...m`, `m = fallback`. + * + * Destructuring is here because it SHADOWS: `{ const { m } = req.body }` in a + * block below a real `const m = pathname.match(…)` binds a different `m` in the + * same function scope, and a shadow this module cannot see is a shadow it would + * mint a route from. Over-collecting a name only ever refuses one, so the + * recursion is deliberately blunt about the shapes it does not name. + */ +function patternNames(pattern: SyntaxNode, out: string[] = []): string[] { + if (pattern.type === 'identifier' || pattern.type === 'shorthand_property_identifier_pattern') { + out.push(pattern.text); + return out; + } + if (pattern.type === 'pair_pattern' || pattern.type === 'assignment_pattern') { + const bound = pattern.childForFieldName('value') ?? pattern.childForFieldName('left'); + if (bound !== null) patternNames(bound, out); + return out; + } + for (const child of pattern.namedChildren) patternNames(child, out); + return out; +} + +/** + * A single URL segment, capturing or not: `[^/]+`, `[^\/]*`, `([^/]+)`. + * + * The CAPTURING form is the one real dispatchers write, and it was the one form + * this converter refused. `(` fell through to the metacharacter bail below, so + * `^\/api\/research-runs\/([^/]+)$` translated to nothing — while the + * non-capturing twin translated fine, which is why every test for this rule + * passed. The tests were written against the implementation instead of against + * the corpus, and the reporting repo does not contain a single non-capturing + * path wildcard: a dispatcher captures the segment because it needs the id. + * + * The alternatives are balanced on purpose — `([^/]+` unclosed is not a segment, + * and matching it would leave a stray `)` to be read as a literal. + */ +const SEGMENT_WILDCARD = /^(?:\(\[\^\\?\/\][+*]\)|\[\^\\?\/\][+*])/; + /** * Convert an anchored regex used as a path test into a route path, or `null` if * any part of it is not cleanly representable. * - * `^\/api\/research-runs\/[^/]+$` → `/api/research-runs/{param}` + * `^\/api\/research-runs\/([^/]+)$` → `/api/research-runs/{param1}` * - * Only two wildcard atoms are recognised, both single-segment (`[^/]+` and - * `[^/]*`, with or without the slash escaped). Anything else — an optional - * group, an alternation, a bare `.*` — bails, because a route path is a claim - * about what the server serves and a mistranslated pattern is a wrong one. + * Only single-segment wildcards are recognised — see {@link SEGMENT_WILDCARD}. + * Anything else — an optional group, an alternation, a bare `.*` — bails, + * because a route path is a claim about what the server serves and a + * mistranslated pattern is a wrong one. A capture group around anything OTHER + * than a segment wildcard still bails: `(.+)` spans slashes, so it is not one + * segment and cannot be one `{param}`. */ export function regexToRoutePath(source: string): string | null { if (!source.startsWith('^') || !source.endsWith('$')) return null; @@ -459,7 +686,7 @@ export function regexToRoutePath(source: string): string | null { let paramIndex = 0; while (i < body.length) { const rest = body.slice(i); - const wildcard = /^\[\^\\?\/\][+*]/.exec(rest); + const wildcard = SEGMENT_WILDCARD.exec(rest); if (wildcard !== null) { paramIndex += 1; out += `{param${paramIndex}}`; @@ -515,15 +742,29 @@ export function extractDispatchGuardRoutes( const found: GuardRoute[] = []; const constants = buildConstantMap(tree.rootNode); + const regexes = buildRegexConstantMap(tree.rootNode); + const matches: MatchBindingState = { bindings: new Map(), declarations: new Map() }; + // Declarations and assignments are noted on the SAME walk that records the + // bindings, and every emission happens after it, so a shadow or a rebinding + // written below the match still refuses the name it would have poisoned. const visit = (node: SyntaxNode): void => { if (node.type === 'binary_expression') collectFromComparison(node, found, constants); - else if (node.type === 'call_expression') collectFromRegexTest(node, found); + else if (node.type === 'call_expression') + collectFromRegexDispatch(node, found, regexes, matches); else if (node.type === 'switch_statement') collectFromSwitch(node, found, constants); + else if (node.type === 'variable_declarator') noteDeclaration(node, matches); + else if ( + node.type === 'assignment_expression' || + node.type === 'augmented_assignment_expression' + ) + noteReassignment(node, matches); for (const child of node.namedChildren) visit(child); }; visit(tree.rootNode); + collectFromMatchBindings(tree.rootNode, matches.bindings, found); + return dedupeWithinFile(found).map((route) => ({ filePath, routePath: route.url, @@ -552,12 +793,11 @@ function collectFromComparison(node: SyntaxNode, out: GuardRoute[], constants: C if (!isPathExpression(expr)) continue; const value = literalValue(literal, constants); if (value === null || !isPathLiteral(value)) continue; - const verb = governingVerb(node); + const verbs = governingVerbs(node); // A bare `/` is only a route when a verb says so — see the module header. - if (value === '/' && verb === null) continue; - out.push({ + if (value === '/' && verbs.length === 0) continue; + pushPerVerb(out, verbs, { url: value, - verb, handlerName: enclosingHandlerName(node), line: node.startPosition.row + 1, }); @@ -565,6 +805,24 @@ function collectFromComparison(node: SyntaxNode, out: GuardRoute[], constants: C } } +/** + * Emit one route per governing verb, or a single verb-less route when the guard + * guarantees none. A multi-method guard is genuinely several routes: they share + * a path and a handler but not a method, and `(method, url)` is the key every + * downstream consumer dedups and looks up on. + */ +function pushPerVerb( + out: GuardRoute[], + verbs: readonly string[], + route: Omit, +): void { + if (verbs.length === 0) { + out.push({ ...route, verb: null }); + return; + } + for (const verb of verbs) out.push({ ...route, verb }); +} + /** * `switch (pathname) { case '/api/health': … }` — the other way to write the * same dispatch, and the reason this module is not a rule about `if`. The @@ -584,9 +842,9 @@ function collectFromSwitch(node: SyntaxNode, out: GuardRoute[], constants: Const const body = node.childForFieldName('body'); if (body === null) return; - // The verb governing the whole switch, if any (`if (req.method === 'GET') - // switch (pathname) { … }`). Read once — every arm shares it. - const verb = governingVerb(node); + // The verbs governing the whole switch, if any (`if (req.method === 'GET') + // switch (pathname) { … }`). Read once — every arm shares them. + const verbs = governingVerbs(node); for (const arm of body.namedChildren) { if (arm.type !== 'switch_case') continue; @@ -594,40 +852,370 @@ function collectFromSwitch(node: SyntaxNode, out: GuardRoute[], constants: Const if (caseValue === null) continue; const value = literalValue(caseValue, constants); if (value === null || !isPathLiteral(value)) continue; - if (value === '/' && verb === null) continue; - out.push({ + if (value === '/' && verbs.length === 0) continue; + pushPerVerb(out, verbs, { url: value, - verb, handlerName: enclosingHandlerName(arm), line: arm.startPosition.row + 1, }); } } -function collectFromRegexTest(node: SyntaxNode, out: GuardRoute[]): void { - if (isNegatedContext(node)) return; +/** A name bound to the result of an anchored-regex match against the path. */ +interface MatchBinding { + readonly name: string; + readonly url: string; + readonly line: number; + readonly handlerName: string | undefined; +} + +/** + * Match bindings, keyed by the FUNCTION a name is bound in as well as the name. + * + * The bare name is not enough, and settling for it invented routes. `m`, `match` + * and `result` are the three most common local names in dispatcher code, so a + * file with two handlers routinely binds `m` twice to unrelated things: + * + * function handleReplay(req) { const m = pathname.match(REPLAY_RE) + * if (req.method === 'GET' && m) … } + * function handleSettings(req) { const m = req.headers['x-mode'] + * if (req.method === 'DELETE' && m) … } + * + * Keyed by name alone, the second function's `m` resolved to the FIRST + * function's binding and minted `DELETE /api/live/positions/{param1}/replay` — + * wrong in its verb, its handler and its line, for a path that handler never + * serves. The poison rule did not catch it because poisoning only ran when a + * second REGEX MATCH bound the name; a binding to anything else never reached + * that code at all. And the loss compounded: the fabricated route carries a + * verb, so {@link reconcileDispatchGuardRoutes} treats it as the authoritative + * claim on that URL and EVICTS the honest verb-less one. + * + * Two names in two functions are now two keys, so neither can see the other. + * Within ONE scope the module still refuses rather than resolves, the way + * {@link buildConstantMap} does: a second declarator for the same key is a + * shadow this walk cannot order, and an assignment can rebind a name from any + * function nested inside the one that declared it. + */ +interface MatchBindingState { + /** Binding key -> the binding, or `null` once the name is ambiguous there. */ + readonly bindings: Map; + /** Binding key -> how many declarators bind it. A second one is a shadow. */ + readonly declarations: Map; +} + +/** + * Count a declarator against its key, and refuse the key once a second one + * binds it. + * + * Refusing rather than ordering costs the real route in + * `const m = pathname.match(RE); { const m = other() }` — the honest GET is + * dropped alongside the shadow that would have fabricated a DELETE. That is the + * cheaper failure by this module's own bar, and it is the same trade + * {@link buildConstantMap} makes for a name declared twice. + */ +function noteDeclaration(node: SyntaxNode, matches: MatchBindingState): void { + const name = node.childForFieldName('name'); + if (name === null) return; + const scopeId = enclosingScopeId(node); + for (const bound of patternNames(name)) { + const key = bindingKey(scopeId, bound); + const count = (matches.declarations.get(key) ?? 0) + 1; + matches.declarations.set(key, count); + if (count > 1) matches.bindings.set(key, null); + } +} + +/** + * Refuse a name that is ASSIGNED anywhere it could reach. + * + * `let m = pathname.match(RE); m = fallback()` leaves `m` holding something this + * walk never saw, and the declaration alone is no longer evidence of what the + * later `if (m)` tests. Poisoning pre-emptively — before the binding is even + * recorded — is what makes the order of the two statements not matter. + * + * Only a REBINDING counts. `m.index = 0` and `m[1] = x` assign THROUGH the name + * and leave it bound to the same match, so refusing on them would drop routes + * for writes that change nothing this module reads. + */ +const REBINDABLE_TARGETS: ReadonlySet = new Set([ + 'identifier', + 'array_pattern', + 'object_pattern', +]); + +function noteReassignment(node: SyntaxNode, matches: MatchBindingState): void { + const left = node.childForFieldName('left'); + if (left === null || !REBINDABLE_TARGETS.has(left.type)) return; + const names = patternNames(left); + if (names.length === 0) return; + for (const scopeId of enclosingScopeIds(node)) { + for (const name of names) matches.bindings.set(bindingKey(scopeId, name), null); + } +} + +/** + * Same-file `const NAME = /re/` bindings, so a regex named once and used by name + * still yields its route. + * + * Verbatim from the reporting repo: `positionReplayRoutes.js` declares + * `const POSITION_REPLAY_RE = /^\/api\/live\/positions\/([^/]+)\/replay$/` at + * module scope and then uses it BOTH ways — `POSITION_REPLAY_RE.test(pathname)` + * and `pathname.match(POSITION_REPLAY_RE)`. Keying only on inline literals + * loses the whole file. + * + * Ambiguity is refused the same way {@link buildConstantMap} refuses it: a name + * bound twice to different patterns is dropped rather than resolved to the + * first, because a half-right regex is a wrong route. + * + * That refusal only ever SAW regex literals, which left the two rebindings that + * matter walking straight past it. `let RE = /^\/api\/re\/([^/]+)$/` followed by + * `RE = buildDynamic(req)` still minted the literal's route, and so did a + * `const RE = new RegExp(userPrefix + '/x')` twin in another function — the map + * is flat, so a same-named binding anywhere in the file is exactly the ambiguity + * the doc claims to refuse. A name bound to ANYTHING that is not a regex + * literal, or assigned at all, is now dropped. + */ +function buildRegexConstantMap(root: SyntaxNode): ReadonlyMap { + const patterns = new Map(); + const ambiguous = new Set(); + + const visit = (node: SyntaxNode): void => { + if (node.type === 'variable_declarator') { + const name = node.childForFieldName('name'); + const value = unparenthesize(node.childForFieldName('value')); + if (name !== null && name.type === 'identifier') { + const pattern = + value !== null && value.type === 'regex' + ? (value.childForFieldName('pattern')?.text ?? null) + : null; + if (pattern === null) ambiguous.add(name.text); + else { + const existing = patterns.get(name.text); + if (existing !== undefined && existing !== pattern) ambiguous.add(name.text); + else patterns.set(name.text, pattern); + } + } + } + if (node.type === 'assignment_expression' || node.type === 'augmented_assignment_expression') { + const left = node.childForFieldName('left'); + if (left !== null && left.type === 'identifier') ambiguous.add(left.text); + } + for (const child of node.namedChildren) visit(child); + }; + visit(root); + + for (const name of ambiguous) patterns.delete(name); + return patterns; +} + +/** The regex pattern this expression denotes — inline literal or named const. */ +function regexPatternOf( + node: SyntaxNode | null, + regexes: ReadonlyMap, +): string | null { + const expr = unparenthesize(node); + if (expr === null) return null; + if (expr.type === 'regex') return expr.childForFieldName('pattern')?.text ?? null; + if (expr.type === 'identifier') return regexes.get(expr.text) ?? null; + return null; +} + +/** + * A route declared by matching the path against an anchored regex. + * + * Two spellings of the same test, with the operands swapped: + * + * if (RE.test(pathname)) — receiver is the regex + * const m = pathname.match(RE) — receiver is the path + * + * Only `.test` was read, which is why 28 of the reporting repo's 75 routes still + * named the shared route table as their handler rather than the module that + * actually serves them: their modules dispatch with `.match`. + * + * `.match` differs in one way that matters. Its result is USED — it carries the + * captured segments — so it is almost always BOUND, and the verb then lives in a + * later `if` rather than around the call: + * + * const runMatch = pathname.match(/^\/api\/research-runs\/([^/]+)$/) + * if (req.method === 'GET' && runMatch) { … } + * + * Reading the verb off the CALL would report every one of those verb-less. So a + * bound match is recorded rather than emitted, and {@link collectFromMatchBindings} + * emits it where the binding is actually tested. An unbound match is a plain + * predicate and is emitted here, exactly like `.test`. + */ +function collectFromRegexDispatch( + node: SyntaxNode, + out: GuardRoute[], + regexes: ReadonlyMap, + matches: MatchBindingState, +): void { const callee = node.childForFieldName('function'); if (callee === null || callee.type !== 'member_expression') return; - if (callee.childForFieldName('property')?.text !== 'test') return; + const method = callee.childForFieldName('property')?.text; + if (method !== 'test' && method !== 'match') return; + const receiver = callee.childForFieldName('object'); - if (receiver === null || receiver.type !== 'regex') return; + const argument = node.childForFieldName('arguments')?.namedChildren[0] ?? null; + if (receiver === null || argument === null) return; - const argument = node.childForFieldName('arguments')?.namedChildren[0]; - if (argument === undefined || !isPathExpression(argument)) return; - - const pattern = receiver.childForFieldName('pattern'); + // `RE.test(pathname)` vs `pathname.match(RE)` — the regex and the path swap + // sides with the method, so each spelling is checked in its own orientation + // rather than accepting any pairing. + const pattern = + method === 'test' + ? isPathExpression(argument) + ? regexPatternOf(receiver, regexes) + : null + : isPathExpression(receiver) + ? regexPatternOf(argument, regexes) + : null; if (pattern === null) return; - const url = regexToRoutePath(pattern.text); + + const url = regexToRoutePath(pattern); if (url === null) return; - out.push({ + const boundName = boundDeclaratorName(node); + if (boundName !== null) { + // Keyed by the function this name is bound in — see MatchBindingState for + // the routes the bare name invented. Already-refused keys stay refused, and + // a key bound twice to DIFFERENT routes is poisoned rather than resolved to + // the first. + const key = bindingKey(enclosingScopeId(node), boundName); + const existing = matches.bindings.get(key); + if (existing !== undefined && (existing === null || existing.url !== url)) { + matches.bindings.set(key, null); + return; + } + matches.bindings.set(key, { + name: boundName, + url, + line: node.startPosition.row + 1, + handlerName: enclosingHandlerName(node), + }); + return; + } + + // Unbound: the call IS the predicate, so its own context carries the verb. + if (isNegatedContext(node)) return; + pushPerVerb(out, governingVerbs(node), { url, - verb: governingVerb(node), handlerName: enclosingHandlerName(node), line: node.startPosition.row + 1, }); } +/** The name this call's result is bound to by `const NAME = `, if any. */ +function boundDeclaratorName(call: SyntaxNode): string | null { + const parent = call.parent; + if (parent === null || parent.type !== 'variable_declarator') return null; + if (parent.childForFieldName('value')?.id !== call.id) return null; + const name = parent.childForFieldName('name'); + return name !== null && name.type === 'identifier' ? name.text : null; +} + +/** + * Emit a route wherever a recorded match binding is TESTED. + * + * The binding's declaration proves a path; the test site proves the method, and + * one binding can be tested more than once. A reference counts only in a + * truthiness position — see {@link isTruthinessPosition} — which is what + * separates `if (m && …)` from `m[1]`, a read of the captured segment that says + * nothing about dispatch and would otherwise mint a duplicate route per capture + * group used. + * + * A binding that is never tested still emits ONE verb-less route: the code did + * compute an anchored match against the request path, which is the same evidence + * an unbound `.test` carries, and dropping it would trade a known path for + * nothing. + * + * The DECLARATION's own name identifier needs no special case: its parent is a + * `variable_declarator`, which is not a truthiness position, so the same + * predicate that rejects `m[1]` rejects it. An explicit skip was written here + * first and removed once it proved unreachable — it read as though the + * declaration were a hazard, which sends the next reader looking for one. + * + * A use counts only against a binding in ITS OWN function. `tested` is keyed the + * same way, and that half matters as much as the emission: keyed by bare name, a + * same-named local in another handler marked the name tested and SUPPRESSED the + * real binding's own verb-less route from the tail loop below — so the honest + * route was not merely joined by a fabricated one, it was replaced by it, down + * to reporting the wrong handler and the wrong line. + */ +function collectFromMatchBindings( + root: SyntaxNode, + matchBindings: ReadonlyMap, + out: GuardRoute[], +): void { + // Resolving a scope costs a walk to the function boundary, and this visits + // every identifier in the file. Names that no live binding uses are rejected + // on a set lookup first, so files without a bound match pay nothing. + const liveNames = new Set(); + for (const binding of matchBindings.values()) if (binding !== null) liveNames.add(binding.name); + if (liveNames.size === 0) return; + const tested = new Set(); + + const visit = (node: SyntaxNode): void => { + if (node.type === 'identifier' && liveNames.has(node.text)) { + const key = bindingKey(enclosingScopeId(node), node.text); + const binding = matchBindings.get(key); + if ( + binding !== undefined && + binding !== null && + isTruthinessPosition(node) && + !isNegatedContext(node) + ) { + tested.add(key); + pushPerVerb(out, governingVerbs(node), { + url: binding.url, + handlerName: enclosingHandlerName(node) ?? binding.handlerName, + line: node.startPosition.row + 1, + }); + } + } + for (const child of node.namedChildren) visit(child); + }; + visit(root); + + for (const [key, binding] of matchBindings) { + if (binding === null || tested.has(key)) continue; + out.push({ + url: binding.url, + verb: null, + handlerName: binding.handlerName, + line: binding.line, + }); + } +} + +/** + * Is this reference read for its TRUTH — an operand of `&&`/`||`, or the whole + * condition of an `if`? + * + * Deliberately narrow. `runMatch[1]` (a `subscript_expression` parent) reads a + * captured segment, and `validate(runMatch)` passes it along; neither asserts + * that the request took this route, and counting them would emit one duplicate + * route per use of the captured id. Parentheses are transparent, so + * `if ((runMatch))` and `if (verb && (runMatch))` both count. + */ +function isTruthinessPosition(node: SyntaxNode): boolean { + let current: SyntaxNode = node; + let parent = current.parent; + while (parent !== null && parent.type === 'parenthesized_expression') { + current = parent; + parent = current.parent; + } + if (parent === null) return false; + if (parent.type === 'binary_expression') { + const operator = parent.childForFieldName('operator')?.text; + return operator === '&&' || operator === '||'; + } + if (parent.type === 'if_statement') { + return parent.childForFieldName('condition')?.id === current.id; + } + return false; +} + /** * Collapse duplicate `(url, verb)` findings within one file, keeping the first — * matching the routes phase's own first-writer-wins. The same comparison can diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/return-shape-members.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/return-shape-members.ts index 1ffb2da65..c03365a9b 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/return-shape-members.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/passes/return-shape-members.ts @@ -36,12 +36,16 @@ * name inference, and keep being reported when it declines. */ -import type { ParsedFile } from 'gitnexus-shared'; +import type { CallableFlowOperand, ParsedFile, Scope, ScopeId } from 'gitnexus-shared'; import type { KnowledgeGraph } from '../../../graph/types.js'; import type { ScopeResolutionIndexes } from '../../model/scope-resolution-indexes.js'; import type { GraphNodeLookup } from '../graph-bridge/node-lookup.js'; import { resolveCallerGraphId } from '../graph-bridge/ids.js'; -import { findCallableBindingInScope, findReceiverTypeBinding } from '../scope/walkers.js'; +import { + findCallableBindingInScope, + findClassBindingInScope, + findReceiverTypeBinding, +} from '../scope/walkers.js'; import { callableFlowSiteKey } from './callable-value-flow.js'; import type { PropertyNameIndex } from './unique-name-properties.js'; @@ -82,6 +86,236 @@ function idNamesMember(id: string, owner: string, member: string): boolean { return after.length === 0 || after.startsWith('@'); } +/** + * Key a parameter cell by the scope it BINDS IN plus its name. + * + * Not by its definition id, which is what the first attempt used: a parameter + * is not reachable through `findValueBindingInScope` (its predicate + * `isOwnableValueLabel` lists Const / Variable / Property / Static, because it + * exists for OWNERSHIP registration and a parameter is owned by nothing), and + * measured, it is not reachable as a `local` binding either — the join found the + * formal and then resolved no def at all. + * + * The scope plus the name is enough and needs no def: the `formal` site already + * states the scope its parameter binds in, and a read of that name anywhere + * inside that scope's subtree refers to it unless something nearer shadows it — + * which {@link parameterProducerFor} handles by stopping at the first scope that + * BINDS the name. + */ +function parameterCellKey(scope: ScopeId, name: string): string { + return `${scope}\u0000${name}`; +} + +/** + * Does `scope` BIND `name` itself — with or without a type or a definition? + * + * The question the parameter walk has to ask before it climbs, and it is NOT + * "does this scope hold a parameter producer", which is what the first attempt + * asked. A `const`, a `for…of` binder, a catch binding and a parameter are all + * nearer declarations of the name, and none of them is in the producer map — so + * a walk that consults only that map climbs straight past the nearer binding and + * types the shadow from an enclosing parameter's callers. + * + * Reads the scope's OWN tables rather than `lookupBindingsAt`, for the same + * reason `isNamespaceNameShadowed` does: the question here is what this scope + * declares LOCALLY, and the finalized/augmented import channels answer a + * different one — routing through them would let a module-level import of the + * name count as a shadow of itself. + * + * `ownedDefs` is consulted alongside `bindings` because a language may register + * a declaration without a binding entry of its own; the sibling guard reads both + * for that reason, and here an extra STOP only ever costs an edge. + */ +function scopeBindsName(scope: Scope, name: string): boolean { + return ( + scope.bindings.has(name) || + scope.typeBindings.has(name) || + scope.lexicalNames?.has(name) === true || + scope.ownedDefs.some((def) => { + const qualifiedName = def.qualifiedName; + if (qualifiedName === undefined) return false; + const dot = qualifiedName.lastIndexOf('.'); + return (dot === -1 ? qualifiedName : qualifiedName.slice(dot + 1)) === name; + }) + ); +} + +/** + * The caller-derived producer for `name` as read at `startScope`, or undefined. + * + * A cell is keyed by the scope its parameter BINDS IN, so a read nested below + * that scope has to climb to reach it — through a nested block, a class body, a + * `catch`. The climb is the whole reason this walk exists, and it is also the + * whole risk: every scope crossed is a scope that might declare the name itself. + * + * So the walk stops at the FIRST scope that binds the name at all, not at the + * first scope that happens to hold a producer. `{ const item = rows[0]; … + * item.wickRatio }` inside `f(item, rows)` is the shape that separates the two: + * the block declares `item`, the producer map does not know that name, and a + * producer-only walk climbs past it to the formal and types a value the callers + * never supplied. + * + * A CALLABLE boundary stops the walk even when nothing visible binds the name, + * because a parameter list is the one binder this pass cannot see through: an + * anonymous arrow is dropped by `collectFunctions` (it cannot be named), so it + * emits no `formal` site and `items.map((spike) => spike.wickRatio)` presents a + * scope that looks EMPTY while in fact rebinding `spike`. Crossing it types an + * array element from the enclosing parameter's callers, at 0.9. The price is a + * closure that genuinely reads an enclosing parameter, which now declines — the + * trade this pass is built to make, since a wrong answer at the precise tier is + * one no `minConfidence` floor can filter out, while a missing one still falls + * through to the 0.5 name tier. + * + * NO VISITED SET, deliberately, unlike the sibling walks in `walkers.ts`. Those + * fail closed on a parent cycle; this one cannot meet a cycle to fail on. Both + * constructions of this tree (`buildScopeTree`, and `TransitionalScopeTree` + * which validates through it) enforce that a parent's range STRICTLY contains + * its child's and throw otherwise, and strict containment is well-founded — a + * cycle would need a scope strictly containing itself. A per-site `Set` here + * would be defence against a state the builder rejects, allocated once for every + * read/write site in the repo. + */ +function parameterProducerFor( + startScope: ScopeId, + name: string, + parameterProducers: ReadonlyMap, + indexes: ScopeResolutionIndexes, +): string | undefined { + let cursor: ScopeId | null = startScope; + while (cursor !== null) { + const producer = parameterProducers.get(parameterCellKey(cursor, name)); + if (producer !== undefined && producer.length > 0) return producer; + const scope = indexes.scopeTree.getScope(cursor); + if (scope === undefined) return undefined; + if (scopeBindsName(scope, name)) return undefined; + if (scope.kind === 'Function') return undefined; + cursor = scope.parent; + } + return undefined; +} + +/** + * Producer names for PARAMETERS, derived from what their callers pass (W2-2). + * + * `function f(spike) { return spike.wickRatio }` has nothing to type `spike` + * from — that is the standing limit of R3-5 and the reason the 0.5 name tier + * exists at all. Measured on the reporting repo, it is also the LARGEST one: + * 11,012 of 13,672 property edges (81%) rest on that name guess. + * + * The two facts needed to answer it were already being extracted, for a + * different purpose. `callable-flow-captures` synthesizes, for JS and TS among + * others: + * + * formal owner=f binding=spike parameter-index=0 + * argument source=s parameter-index=0 direct-callee-name=f + * + * so joining them on `(callee, parameterIndex)` says which cell reaches which + * parameter, and the argument's own binding is typed by the same + * `findReceiverTypeBinding` used for a directly-bound receiver. No new capture, + * no parse-time change, and deliberately NOT a change to the callable-value-flow + * solver that owns these sites — that pass is guarded by a fingerprint + * correctness gate, so this reads the same facts and computes its own map. + * + * A parameter with callers passing DIFFERENT producers resolves to nothing. + * Picking one would fabricate at the 0.9 PRECISE tier, which no `minConfidence` + * floor can filter out — the same reason `buildConstantMap` drops an ambiguous + * constant instead of taking the first. + * + * COVERAGE, measured rather than assumed. The synthesis skips an argument that + * is itself a call result (`f(makeSignal())` emits no argument site, by an + * explicit `continue` in `callable-flow-captures`), so only the bound spelling + * `const s = makeSignal(); f(s)` is served. That looked fatal until counted: in + * the reporting repo bare-identifier arguments outnumber call-result arguments + * 2,563 to 50. The captured spelling is the dominant one by 51:1. + */ +function buildParameterProducers( + indexes: ScopeResolutionIndexes, + parsedFiles: readonly ParsedFile[], +): ReadonlyMap { + /** parameter def id -> producer name, or CONFLICT once callers disagree. */ + const producers = new Map(); + const conflicted = new Set(); + + // Formals keyed by the file that declares them, so two same-named functions + // in different files cannot answer for each other — the same file identity + // the member join below relies on. + // + // AMBIGUITY IS REFUSED HERE TOO, not settled by arrival order. The file is + // only one of the two axes a name can collide on: `ownerName` is a BARE + // identifier, and `emitFormalFacts` emits one `formal` per parameter of every + // callable it collects — nested functions and class methods included. So a + // free `parse` and a `parse` nested inside it, or a free `apply` and + // `Runner.apply`, key this map identically within ONE file. A plain `.set` + // lets the last one visited win, which hands a caller's producer to a + // parameter that caller never reached; the edge that follows is emitted at the + // 0.9 PRECISE tier, above every `minConfidence` floor, while the genuine + // consumer is left untyped. Poisoning the key costs both callables their edge + // and fabricates neither — the same discipline the `producers` map applies to + // disagreeing callers thirty lines below. + const formals = new Map(); + /** Formal keys claimed by two DIFFERENT parameters — unable to answer. */ + const ambiguousFormals = new Set(); + for (const parsed of parsedFiles) { + for (const flow of parsed.callableFlowSites ?? []) { + if (flow.kind !== 'formal') continue; + const formalKey = `${parsed.filePath}\u0000${flow.ownerName}\u0000${flow.parameterIndex}`; + if (ambiguousFormals.has(formalKey)) continue; + const claimed = formals.get(formalKey); + if (claimed !== undefined) { + // The same cell restated is not a disagreement — only a formal naming a + // DIFFERENT parameter leaves the key unable to answer. + if (claimed.inScope === flow.binding.inScope && claimed.name === flow.binding.name) { + continue; + } + formals.delete(formalKey); + ambiguousFormals.add(formalKey); + continue; + } + formals.set(formalKey, flow.binding); + } + } + if (formals.size === 0) return producers; + + for (const parsed of parsedFiles) { + for (const flow of parsed.callableFlowSites ?? []) { + if (flow.kind !== 'argument') continue; + const callee = flow.directCalleeName; + if (callee === undefined || callee.length === 0) continue; + + // Resolve the callee from the CALL SITE, so the formal is looked up in the + // file that actually declares the function rather than the one calling it. + const calleeDef = findCallableBindingInScope(flow.source.inScope, callee, indexes); + if (calleeDef?.filePath === undefined) continue; + + const binding = formals.get( + `${calleeDef.filePath}\u0000${callee}\u0000${flow.parameterIndex}`, + ); + if (binding === undefined) continue; + + const cell = parameterCellKey(binding.inScope, binding.name); + if (conflicted.has(cell)) continue; + + const producer = findReceiverTypeBinding( + flow.source.inScope, + flow.source.name, + indexes, + )?.rawName; + if (producer === undefined || producer.length === 0) continue; + + const existing = producers.get(cell); + if (existing !== undefined && existing !== producer) { + // Two callers, two producers. Which shape this parameter holds depends + // on the call, and this pass answers at the precise tier or not at all. + producers.delete(cell); + conflicted.add(cell); + continue; + } + producers.set(cell, producer); + } + } + return producers; +} + export function emitReturnShapeMemberAccesses( graph: KnowledgeGraph, indexes: ScopeResolutionIndexes, @@ -110,6 +344,9 @@ export function emitReturnShapeMemberAccesses( // own files is what actually closes it. const ownFilePaths = new Set(parsedFiles.map((p) => p.filePath)); + // Caller-derived parameter types (W2-2) — see `buildParameterProducers`. + const parameterProducers = buildParameterProducers(indexes, parsedFiles); + for (const parsed of parsedFiles) { for (const site of parsed.referenceSites) { if (site.kind !== 'read' && site.kind !== 'write') continue; @@ -122,22 +359,20 @@ export function emitReturnShapeMemberAccesses( // whole point: `formatSpikeAlert` is a function, and before R3-4 there // was nothing named after it to look a member up on. const typeRef = findReceiverTypeBinding(site.inScope, receiver, indexes); - const producerRef = typeRef?.rawName; + let producerRef = typeRef?.rawName; + + // W2-2. A receiver with no binding of its own may still be a PARAMETER + // whose callers all pass the same producer. Consulted only where the + // direct binding declined, so a receiver that already had a type keeps it. + if (producerRef === undefined || producerRef.length === 0) { + producerRef = parameterProducerFor(site.inScope, receiver, parameterProducers, indexes); + } if (producerRef === undefined || producerRef.length === 0) continue; // R3-4 qualifies a returned key by the producing function's own name, so // the owner segment to match is the LAST one. For a plain producer this is // a no-op. - // - // A MEMBER-CALL producer (`const r = svc.make()`) binds `svc.make`, and - // that spelling resolves to no value binding below, so this pass DECLINES - // rather than resolving it. That is a known coverage limit, not a fix: - // answering it means typing `svc` first and then finding `make` on that - // type, which is a different (and larger) piece of work. Declining is the - // correct behaviour in the meantime — the alternative, matching - // `make.` by name across the graph, is precisely the fabrication - // the file guard below exists to stop. - const producer = producerRef.slice(producerRef.lastIndexOf('.') + 1); + let producer = producerRef.slice(producerRef.lastIndexOf('.') + 1); if (producer.length === 0) continue; // Resolve the producer to a real definition and keep only members that @@ -177,7 +412,42 @@ export function emitReturnShapeMemberAccesses( // legitimately in that same file. File equality passes // there; only the language restriction closes it. const producerDef = findCallableBindingInScope(site.inScope, producerRef, indexes); - const producerFile = producerDef?.filePath; + let producerFile = producerDef?.filePath; + + // MEMBER-CALL PRODUCERS (W2-1). Tried only where the callable lookup above + // DECLINED, so every reference that resolved before resolves identically — + // this adds a case, it does not reroute the existing one. + // + // `const r = svc.make()` binds the spelling `svc.make`. Slicing that to its + // last segment leaves `make`, which is a METHOD and so never a callable + // binding in scope; the lookup failed and the pass declined. The limit was + // documented as needing inter-procedural receiver typing, but measured, the + // pipeline had already done the hard part: `svc.make()` resolves to its + // Method node as an ordinary CALLS edge, and R3-4 anchors the returned + // literal's keys to that method, so `SignalService.make.secretFlag` already + // existed as a node. Only this join was missing. + // + // Nothing new is inferred. The receiver is typed by the SAME predicate that + // typed `r` above, and it must resolve to a class of its own — a receiver + // that cannot be typed still declines. The owner segment is then TWO parts + // (`SignalService.make`) rather than one, which is exactly how R3-4 + // qualifies a key returned from a method, and it is what separates two + // methods on one class that return the same key name from each other and + // from a free function of that name. + if (producerFile === undefined) { + const dotAt = producerRef.lastIndexOf('.'); + if (dotAt <= 0) continue; + const receiverExpr = producerRef.slice(0, dotAt); + const methodName = producerRef.slice(dotAt + 1); + if (methodName.length === 0) continue; + const ownerType = findReceiverTypeBinding(site.inScope, receiverExpr, indexes)?.rawName; + if (ownerType === undefined || ownerType.length === 0) continue; + const ownerDef = findClassBindingInScope(site.inScope, ownerType, indexes); + if (ownerDef === undefined) continue; + producer = `${ownerType}.${methodName}`; + producerFile = ownerDef.filePath; + } + if (producerFile === undefined) continue; if (!ownFilePaths.has(producerFile)) continue; diff --git a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts index b973d5357..0525171be 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts @@ -23,6 +23,7 @@ import type { BindingRef, ParsedFile, + Scope, ScopeId, SymbolDefinition, TypeParameter, @@ -40,6 +41,7 @@ import { extractTemplateArguments, stripTemplateArguments, } from '../../utils/template-arguments.js'; +import { definitionIdPosition } from '../utils/definition-id.js'; const EMPTY_BINDINGS: readonly BindingRef[] = Object.freeze([]); @@ -501,6 +503,44 @@ const typeParameterNamesByBundle = new WeakMap< const NO_TYPE_PARAMETERS: ReadonlySet = Object.freeze(new Set()); +/** + * Did `def` OPEN `scope` — i.e. is this scope the declaration's own body? + * + * The gate on reading a declaration's `typeParameters` as a lexical binding. + * `ownedDefs` answers "which scope was this declaration written in", which is a + * DIFFERENT question: a declaration that opens no scope of its own is owned by + * whatever encloses it, and reading its parameters there binds them across that + * entire enclosing region. + * + * Measured (#2899): a TypeScript `type Maybe = Result | null` opens no + * scope — only the `object_type` alias form does — so its parameter list landed + * in the MODULE's `ownedDefs`. Since each scope's answer is built from its + * parent's, `Result` was then bound as a type parameter in every scope in the + * file, and the `USES` guard downstream deleted every genuine edge to the + * `interface Result` beside it, imported ones included. One un-anchored + * parameter list silently emptied an entire file of the edge class that answers + * "what breaks if I remove this field?". + * + * Compares the def's declaration position with the scope's start — the same + * alignment test `pickCallerCallableDef` uses to tell a closure from a nested + * function, and sound for the same reason: when a declaration is itself the + * scope node, both sides are built from one `Range`. Every language that + * populates `typeParameters` today anchors them on a declaration that IS a scope + * node (TS class/interface/function, Java/C#/Kotlin/Rust type declarations, the + * C++ `class_specifier` inside a `template_declaration`), so the alignment holds + * wherever the parameters were meant to bind. + * + * A `Module` scope is excluded outright rather than left to the position test: + * a module is opened by the file, never by a declaration, and a declaration + * written on the file's first line shares its start coordinates. + */ +function declarationOpenedScope(def: SymbolDefinition, scope: Scope): boolean { + if (scope.kind === 'Module') return false; + const position = definitionIdPosition(def.nodeId, def.filePath); + if (position === undefined) return false; + return position.line === scope.range.startLine && position.column === scope.range.startCol; +} + /** * Every name the scope chain above `scopeId` (inclusive) binds as a declared * TYPE PARAMETER. @@ -509,6 +549,10 @@ const NO_TYPE_PARAMETERS: ReadonlySet = Object.freeze(new Set()) * chain is walked once and every scope on it is O(own defs) rather than * O(depth × defs). That matters because the caller runs on every class-binding * lookup, and a module scope's `ownedDefs` is the whole file. + * + * That parent-inheriting fold is also why {@link declarationOpenedScope} gates + * every read: a parameter list picked up one scope too high does not merely + * over-reach by one scope, it reaches every scope below it as well. */ function typeParameterNamesInScope( scopeId: ScopeId, @@ -545,7 +589,9 @@ function typeParameterNamesInScope( const scope = scopes.scopeTree.getScope(id); let own: Set | undefined; for (const def of scope?.ownedDefs ?? []) { - for (const parameter of def.typeParameters ?? []) { + if (def.typeParameters === undefined) continue; + if (scope === undefined || !declarationOpenedScope(def, scope)) continue; + for (const parameter of def.typeParameters) { if (parameter.name.length === 0) continue; own ??= new Set(inherited); own.add(parameter.name); @@ -581,7 +627,7 @@ function typeParameterNamesInScope( * yet. So only a POSITIVE match declines; an absent list changes nothing, which * is what keeps every unconverted language behaving exactly as it does today. */ -function bindsTypeParameter( +export function bindsTypeParameter( scopeId: ScopeId, name: string, scopes: ScopeResolutionIndexes, diff --git a/gitnexus/src/core/ingestion/tree-sitter-queries.ts b/gitnexus/src/core/ingestion/tree-sitter-queries.ts index 7a6dee3df..0e56f7c7b 100644 --- a/gitnexus/src/core/ingestion/tree-sitter-queries.ts +++ b/gitnexus/src/core/ingestion/tree-sitter-queries.ts @@ -562,11 +562,18 @@ export const TYPESCRIPT_QUERIES = ` ; HTTP consumers: fetch('/path'), axios.get('/path'), $.get('/path'), etc. ; fetch() — global function +; The URL alternation is OPTIONAL (#2897). Requiring a literal made the rule +; blind to fetch(url) with a variable argument -- measured on this repo, 44 of +; 47 fetch calls pass one, so 94% of outward calls produced no site at all. The +; R3-6 sink set needs only WHERE the program reaches outward, not the URL; route +; linking still needs the URL and already skips an entry without one +; (normalizeFetchURL returns nothing and processNextjsFetchRoutes continues), so +; widening here adds sink sites without inventing a single FETCHES edge. (call_expression function: (identifier) @_fetch_fn (#eq? @_fetch_fn "fetch") arguments: (arguments [(string (string_fragment) @route.url) - (template_string) @route.template_url])) @route.fetch + (template_string) @route.template_url]?)) @route.fetch ; Custom fetch wrappers: apiFetch('/path'), fetchJSON('/api/data'), httpGet('/users'), etc. (call_expression @@ -1058,11 +1065,18 @@ export const JAVASCRIPT_QUERIES = ` right: (_)) @assignment ; HTTP consumers: fetch('/path'), axios.get('/path'), $.get('/path'), etc. +; The URL alternation is OPTIONAL (#2897). Requiring a literal made the rule +; blind to fetch(url) with a variable argument -- measured on this repo, 44 of +; 47 fetch calls pass one, so 94% of outward calls produced no site at all. The +; R3-6 sink set needs only WHERE the program reaches outward, not the URL; route +; linking still needs the URL and already skips an entry without one +; (normalizeFetchURL returns nothing and processNextjsFetchRoutes continues), so +; widening here adds sink sites without inventing a single FETCHES edge. (call_expression function: (identifier) @_fetch_fn (#eq? @_fetch_fn "fetch") arguments: (arguments [(string (string_fragment) @route.url) - (template_string) @route.template_url])) @route.fetch + (template_string) @route.template_url]?)) @route.fetch ; Custom fetch wrappers: apiFetch('/path'), fetchJSON('/api/data'), httpGet('/users'), etc. (call_expression diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index 95b7d37b1..f139c2be1 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -1772,13 +1772,20 @@ const processFileGroup = ( // Extract HTTP consumer URLs: fetch(), axios.get(), $.get(), requests.get(), etc. if (captureMap['route.fetch']) { const urlNode = captureMap['route.url'] ?? captureMap['route.template_url']; - if (urlNode) { - result.fetchCalls.push({ - filePath: file.path, - fetchURL: urlNode.text, - lineNumber: captureMap['route.fetch'].startPosition.row + lineOffset, - }); - } + // A fetch whose URL is not a literal is still an OUTWARD CALL, and that + // is the whole of what the R3-6 sink set needs — where the program + // reaches out, not where to. Recorded with an empty `fetchURL` (#2897): + // route linking normalizes the URL first and skips anything that yields + // nothing, so these add sink sites without inventing a FETCHES edge. + // + // Measured before this: 44 of 47 fetch calls in this repo pass a + // variable, so the sink signal was absent from 94% of them and + // sink-terminated flows could effectively never fire. + result.fetchCalls.push({ + filePath: file.path, + fetchURL: urlNode ? urlNode.text : '', + lineNumber: captureMap['route.fetch'].startPosition.row + lineOffset, + }); continue; } diff --git a/gitnexus/src/core/lbug/graph-emit-sink.ts b/gitnexus/src/core/lbug/graph-emit-sink.ts index f1527c6d2..640ee11f7 100644 --- a/gitnexus/src/core/lbug/graph-emit-sink.ts +++ b/gitnexus/src/core/lbug/graph-emit-sink.ts @@ -104,6 +104,7 @@ import { splitRelPairKey, } from './rel-pair-routing.js'; import { DEFAULT_EMIT_CHUNK_ROWS, SyncCsvWriter } from './sync-csv-writer.js'; +import { PDG_EDGE_TYPES } from './pdg-emit-sink.js'; /** * Relationship types that MUST stay in the in-memory graph because a phase @@ -167,6 +168,22 @@ export interface GraphEmitManifest { readonly relsByPair: Map; /** Total streamed rows, for the buffer-pool size hint (#2631 path). */ readonly totalRows: number; + /** + * Streamed rows EXCLUDING `PDG_EDGE_TYPES`, for the graph-write-collapse + * check — which counts persisted STRUCTURAL rows and so needs a structural + * expectation to compare against. + * + * Not derivable from `relsByPair`: a pair key is `From|To` NODE LABELS, and + * a PDG edge shares `Function|Function` with `CALLS`. Only the write path + * sees `relationship.type`, so the split has to be counted here. + * + * This existed as a bug first. `totalRows` is a buffer-pool size hint and + * counts every row; the collapse check reused it as the expectation while + * measuring structural rows on the other side. On a `--pdg` run that compared + * ~200k against ~65k and declared a healthy index INCOMPLETE — then the + * collapse stamp forced a rebuild on the next run, which did it again. + */ + readonly structuralRows: number; } /** @@ -448,6 +465,7 @@ export class GraphEmitSink implements KnowledgeGraph, GraphEmitControl { this.streamedIds.add(key); writer.addRow(buildRelRow(relationship)); + if (!PDG_EDGE_TYPES.has(relationship.type)) this.structuralRows++; this.srcIx.push(srcIx); this.tgtIx.push(tgtIx); this.relTypes.push(relationship.type); @@ -459,6 +477,9 @@ export class GraphEmitSink implements KnowledgeGraph, GraphEmitControl { * a final-flush failure, or a writer-open failure (EMFILE) — is surfaced * loudly here so a disk-full / out-of-fds run never hands a truncated CSV to * the bulk COPY. */ + /** Streamed rows that are not PDG — see `GraphEmitManifest.structuralRows`. */ + private structuralRows = 0; + finalize(): GraphEmitManifest { if (this.finalized) throw new Error('GraphEmitSink.finalize() called twice'); this.finalized = true; @@ -486,7 +507,7 @@ export class GraphEmitSink implements KnowledgeGraph, GraphEmitControl { ); } - return { relsByPair, totalRows }; + return { relsByPair, totalRows, structuralRows: this.structuralRows }; } /** Best-effort fd release for the error path — when the pipeline throws diff --git a/gitnexus/src/core/lbug/lbug-adapter.ts b/gitnexus/src/core/lbug/lbug-adapter.ts index 3c85eaf39..d081f87b3 100644 --- a/gitnexus/src/core/lbug/lbug-adapter.ts +++ b/gitnexus/src/core/lbug/lbug-adapter.ts @@ -28,6 +28,7 @@ import { import { streamAllCSVsToDisk, type StreamedCSVResult } from './csv-generator.js'; import type { GraphEmitManifest } from './graph-emit-sink.js'; import type { PdgEmitManifest } from './pdg-emit-sink.js'; +import { PDG_EDGE_TYPES } from './pdg-emit-sink.js'; import { getNodeLabel as deriveNodeLabel, type WriteStreamFactory } from './rel-pair-routing.js'; import { EMBEDDABLE_LABELS, type CachedEmbedding } from '../embeddings/types.js'; import { @@ -1839,9 +1840,40 @@ export const executeWithReusedStatement = async ( export const getLbugStats = async (): Promise<{ nodes: number; edges: number | undefined; + /** + * Edges EXCLUDING the streamed PDG layers, or `undefined` when the count could + * not be taken (same distinction `edges` makes — an unmeasurable count is not + * a measured zero). + * + * The graph-write-collapse check compares what the pipeline produced against + * what the database holds, and `edges` counts every `CodeRelation` row — PDG + * writes into that same table. On a `--pdg` run the expected side is + * structural-only, so comparing it against the total let PDG volume mask + * structural loss outright: 1,000 structural edges expected, 4,000 PDG rows + * persisted, every structural edge gone, and the ratio still clears. This is + * the like-for-like counterpart. + */ + structuralEdges: number | undefined; + /** + * Why `structuralEdges` is absent, when it is; `undefined` once the count was + * taken. Recorded rather than swallowed because this query is NEWER and + * NARROWER than `edges` — it filters on `r.type` with an `IN` predicate — and + * the collapse check consults only it, so a throw here disables the guard and + * (since the guard is now also the automatic-rebuild trigger) the repair it + * drives. A caller that cannot see the difference between "measured" and + * "could not measure" has no way to say so in its log or its metadata. + */ + structuralEdgesError?: string; }> => { const c = conn; - if (!c) return { nodes: 0, edges: undefined }; + if (!c) { + return { + nodes: 0, + edges: undefined, + structuralEdges: undefined, + structuralEdgesError: 'no open connection', + }; + } // Called during analyze finalize while the WAL-checkpoint driver is still // running; each count read takes the connection lock so it cannot execute @@ -1876,7 +1908,36 @@ export const getLbugStats = async (): Promise<{ // here is what made a throwing query indistinguishable from an empty table. } - return { nodes: totalNodes, edges: totalEdges }; + // Structural-only count for the collapse check. `TAINT_PATH` is deliberately + // NOT in `PDG_EDGE_TYPES` — it is a whole-program Function→Function edge that + // lives in the in-memory graph and is persisted by the normal emit, so it IS + // structural and must stay counted on both sides. + let structuralEdges: number | undefined; + let structuralEdgesError: string | undefined; + try { + const excluded = [...PDG_EDGE_TYPES].map((t) => `'${t}'`).join(', '); + structuralEdges = await withConnLock(async () => { + const queryResult = await c.query( + `MATCH ()-[r:${REL_TABLE_NAME}]->() WHERE NOT r.type IN [${excluded}] RETURN count(r) AS cnt`, + ); + const rows = await readQueryRows(queryResult); + return rows.length > 0 ? Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0) : 0; + }); + } catch (err) { + // Same contract as `edges`: leave undefined rather than report a zero the + // collapse check would read as a total wipeout. But NOT silent — the reason + // travels back on the result and is logged by the caller, so "the guard + // declined because it could not measure" is visible instead of looking + // identical to "the guard ran and found nothing wrong". + structuralEdgesError = err instanceof Error ? err.message : String(err); + logger.warn( + { err }, + 'Structural relationship count failed; the graph-write-collapse check will have no ' + + 'structural measurement from this run.', + ); + } + + return { nodes: totalNodes, edges: totalEdges, structuralEdges, structuralEdgesError }; }; /** diff --git a/gitnexus/src/core/lbug/pdg-emit-sink.ts b/gitnexus/src/core/lbug/pdg-emit-sink.ts index 78d337816..12db53ac4 100644 --- a/gitnexus/src/core/lbug/pdg-emit-sink.ts +++ b/gitnexus/src/core/lbug/pdg-emit-sink.ts @@ -70,7 +70,7 @@ import { type NodeTableName } from './schema.js'; * complete CALLS graph, and stays in the in-memory graph (it is small and is * persisted by the normal whole-graph emit). */ -const PDG_EDGE_TYPES: ReadonlySet = new Set([ +export const PDG_EDGE_TYPES: ReadonlySet = new Set([ 'CFG', 'REACHING_DEF', 'CDG', diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 70e4a6fe5..ea9f386d2 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -9,7 +9,8 @@ * wrapper or server worker) is responsible for process lifecycle. */ -import { detectGraphWriteCollapse } from './index-freshness.js'; +import { detectGraphWriteCollapse, type GraphWriteCollapseVerdict } from './index-freshness.js'; +import { PDG_EDGE_TYPES } from './lbug/pdg-emit-sink.js'; import path from 'path'; import fs from 'fs/promises'; import { randomUUID } from 'node:crypto'; @@ -535,6 +536,113 @@ import { deriveEmbeddingCap, DEFAULT_EMBEDDING_NODE_LIMIT, } from './embedding-mode.js'; +import type { GraphEmitManifest } from './lbug/graph-emit-sink.js'; + +/** + * Relationships RESIDENT in the in-memory graph, excluding the PDG layers — + * the heap-side counterpart of the sink's `structuralRows` subtotal and of + * `getLbugStats().structuralEdges`, counted by the same `PDG_EDGE_TYPES` + * predicate so all three measure one population. + * + * A type-aware scan rather than `graph.relationshipCount`, because that count is + * PDG-INCLUSIVE on every run that does not stream. `resolveStreamPdgEmit` and + * `resolveStreamGraphEmit` BOTH require `force === true`, so with no `--force` + * there is no sink at all and `scope-resolution/pipeline/run.ts` writes the PDG + * layers into the ordinary graph (`input.pdgEmitSink ?? graph`). Measured + * directly: one `runScopeResolution({ pdg: true })` with no sink leaves + * `relationshipCount = 1`, all of it `CFG`. A first-time `analyze --pdg` on a + * fresh repo is a FULL write (so the collapse check runs) and a non-streaming + * one, so `relationshipCount` there compares structural-plus-PDG against a + * structural-only measurement — the same false collapse the streamed path + * already fixed, on the default configuration rather than the `--force` one. + * + * `forEachRelationshipFields` is the zero-allocation columnar scan (~90 ms per + * million edges) and `pipelineResult.graph` is always the RAW graph, never the + * sink, so this never has to recall an offloaded edge. + * + * `NaN` when the graph cannot be scanned at all, which is the SAME fact the + * previous `graph.relationshipCount` read produced for such a graph (`undefined + * + streamedRows`), and which `detectGraphWriteCollapse` maps to an explicit + * `'unmeasurable'`. Its docstring already names "a graph implementation that + * reports no total, a lightweight pipeline result" as an expected input, so + * calling an absent method here would convert a documented no-verdict into a + * crashed analyze. + */ +export function countStructuralRelationships( + graph: Partial> | undefined, +): number { + if (typeof graph?.forEachRelationshipFields !== 'function') return Number.NaN; + let structural = 0; + graph.forEachRelationshipFields((_sourceId, _targetId, type) => { + if (!PDG_EDGE_TYPES.has(type)) structural++; + }); + return structural; +} + +/** + * The STRUCTURAL relationship count a healthy write is expected to persist. + * + * Exported and called by production rather than mirrored in a test. That is the + * point: the wiring test kept a LOCAL COPY of this expression "because the + * production expression is inline in a 3000-line function", and a copy cannot + * catch a term the original got wrong. It did not catch this one. + * + * BOTH terms are objects, not pre-selected numbers, and for the same reason: + * every defect this expression has had was a wrong FIELD chosen at a call site + * no unit test can reach — first `totalRows` over `structuralRows`, then + * `relationshipCount` over the structural subtotal. Taking the graph and the + * manifest puts both choices inside the tested function. + */ +export function computeExpectedStructuralRelationships( + /** + * The in-memory graph, NOT its `relationshipCount`. That count includes the + * PDG layers whenever they did not stream — which is every run without + * `--force`, i.e. the default configuration. A graph that cannot be scanned + * yields `NaN`, i.e. an explicit no-verdict, exactly as an absent + * `relationshipCount` did. + */ + graph: Partial> | undefined, + /** + * The MANIFEST, not a pre-selected number. Taking the whole object puts the + * `structuralRows` / `totalRows` choice INSIDE the tested function — the + * choice that was wrong before, and that a numeric parameter leaves at an + * untestable call site. + */ + graphEmitManifest: Pick | undefined, +): number { + return countStructuralRelationships(graph) + (graphEmitManifest?.structuralRows ?? 0); +} + +/** + * Which `graphWriteCollapsed` stamp a finished run should PERSIST. + * + * Split on the VERDICT, never on the write mode. `saveMeta` overwrites + * meta.json atomically rather than merging, so returning `undefined` DELETES + * the stamp — and the stamp is what marks the index incomplete and forces the + * rebuild that repairs it. Only a positive `'healthy'` measurement earns that + * deletion; `'unmeasurable'` means this run compared nothing, and a run that + * measured nothing has repaired nothing. + * + * Exported and called by production for the same reason + * {@link computeExpectedStructuralRelationships} is: the previous version of + * this decision lived inline in a 3000-line function, where no unit test could + * reach it, and it shipped implementing a documented three-way taxonomy as a + * two-way branch on `wroteChangedSubgraphOnly`. + */ +export function selectPersistedCollapseStamp( + verdict: GraphWriteCollapseVerdict, + /** The stamp already on disk. Survives every non-`'healthy'` verdict. */ + previousStamp: RepoMeta['graphWriteCollapsed'], +): RepoMeta['graphWriteCollapsed'] { + switch (verdict.verdict) { + case 'collapsed': + return { expected: verdict.expected, persisted: verdict.persisted }; + case 'healthy': + return undefined; + case 'unmeasurable': + return previousStamp; + } +} export const PHASE_LABELS: Record = { extracting: 'Scanning files', @@ -1349,6 +1457,31 @@ async function runFullAnalysisInner( options = { ...options, force: true }; } + // ── a recorded graph-write collapse forces a full rebuild ──────── + // + // Every other meta-driven trigger above and below gets a block here; + // `graphWriteCollapsed` was recorded and then never read by anything + // (`grep -rn graphWriteCollapsed src/` showed writes only). The consequence is + // the worst available: a collapsed index whose commit has not changed takes + // the `alreadyUpToDate` fast path, prints "Already up to date", exits 0, and + // keeps doing so forever. The one state that means "most of your edges are + // gone" was the one state that repaired itself only if the user happened to + // pass `--force`. + // + // Forcing is the correct remedy rather than merely re-running: the collapse + // means the persisted graph disagrees with what the pipeline produced, and an + // incremental pass over unchanged files would write nothing and re-stamp the + // same broken index as fresh. + if (existingMeta?.graphWriteCollapsed) { + const { expected, persisted } = existingMeta.graphWriteCollapsed; + log( + `previous run persisted ${persisted} of ${expected} expected relationships ` + + `(recorded as a graph-write collapse); forcing a full re-analyze rather than ` + + `reporting an index this build already knows is incomplete.`, + ); + options = { ...options, force: true }; + } + // ── independently-versioned analysis capabilities ──────────────── // `schemaFingerprint` is reserved for graph-wide incremental invariants. Some // persisted semantics apply only to repositories containing relevant source @@ -2785,15 +2918,88 @@ async function runFullAnalysisInner( // recovery paths AND the `analyze --force` retry this check's own warning // tells the operator to run. Same correction, and for the same reason, as // the buffer-pool hint earlier in this file. - const expectedRelationships = - pipelineResult.graph.relationshipCount + (pipelineResult.graphEmitManifest?.totalRows ?? 0); + // + // `structuralRows`, NOT `totalRows`. The manifest's `totalRows` is a + // buffer-pool size hint and counts EVERY streamed row; PDG edges stream + // through this same sink (measured: `pdgEmitManifest` absent, zero PDG + // resident in the graph, 179,676 streamed rows of which ~110k were PDG), so + // using it compared a structural-plus-PDG expectation against the + // structural-only measurement below and declared a healthy `--pdg` index + // INCOMPLETE — 200,501 against 64,764 on a real repo, with every row + // present. The stamp then forced a rebuild on the next run, which repeated + // it: a permanent loop on an undamaged index. + // + // A pair key cannot separate them — it is `From|To` NODE LABELS, and a PDG + // edge shares `Function|Function` with `CALLS` — so the sink counts the + // split at the point it writes, where `relationship.type` is in hand. + // + // The GRAPH, not `graph.relationshipCount`. That count is PDG-inclusive on + // every run that does NOT stream, and streaming needs `force === true` + // (`resolveStreamGraphEmit` opens with `if (options.force !== true) return + // false`, `resolveStreamPdgEmit` the same), so plain `analyze --pdg` has no + // sink and `run.ts` writes the PDG layers into the ordinary graph. A first + // run on a fresh repo has no `existingMeta`, so it is not incremental and + // this check RUNS — comparing structural-plus-PDG against structural-only + // and failing a healthy index. `computeExpectedStructuralRelationships` + // therefore counts the heap side type-aware too, so both sides measure the + // same population in every configuration rather than only under `--force`. + const expectedRelationships = computeExpectedStructuralRelationships( + pipelineResult.graph, + pipelineResult.graphEmitManifest, + ); // `getLbugStats` returns `edges: undefined` when the count could not be // taken, which is a different fact from zero — an edge query that throws // must not read as a measured collapse. `nodes > 0` is independent evidence // the DB was readable at all, but it says nothing about whether the EDGE // query threw, so both conditions are required. + // + // STRUCTURAL ONLY, and that is the whole correction. `expected` above counts + // the in-memory graph plus the streamed STRUCTURAL manifest; the streamed + // PDG layers never enter `graph.relationshipCount`. But `stats.edges` counts + // EVERY `CodeRelation` row, and PDG writes into that same table — so on a + // `--pdg` run the two sides measured different populations and the surplus + // masked real loss. With 1,000 structural edges expected and 4,000 PDG rows + // persisted, losing EVERY structural edge still read `persisted = 4000` and + // cleared the ratio: a total wipeout, reported healthy, on exactly the large + // repos `--pdg` is used for. + // + // Padding `expected` with the PDG rows instead does NOT fix it — it makes + // the universes match but leaves the ratio judging a minority population: + // 4,000 of 5,000 still clears 0.5. Only comparing structural against + // structural asks the question the check exists to ask. + // + // FALLBACK when the structural query alone failed. `structuralEdges` is the + // newer, filtered, `IN`-predicate query; before it existed only `edges` had + // to succeed, and routing the whole check through the newer one made a + // single throw disable the guard AND — since the stamp now triggers the + // automatic rebuild — the repair it drives. When this run had no PDG layer + // the two counts are equal by construction (nothing writes a PDG row), so + // `edges` answers the same question and the guard keeps working. With + // `--pdg` on there is no substitute and the absence stands: it becomes an + // explicit `'unmeasurable'` verdict below, which preserves rather than + // erases the previous stamp. + const structuralCountMissed = stats.nodes > 0 && stats.structuralEdges === undefined; const persistedRelationships = - stats.nodes > 0 && stats.edges !== undefined ? stats.edges : undefined; + stats.nodes > 0 + ? (stats.structuralEdges ?? (options.pdg === true ? undefined : stats.edges)) + : undefined; + // Never swallowed. The count is taken inside a `catch {}` in `getLbugStats`, + // so without this line a failed measurement is indistinguishable from a + // healthy one in the logs — and "measured nothing" reading as "measured + // fine" is the whole class of defect this area keeps producing. + if (structuralCountMissed) { + log( + `Warning: the structural relationship count could not be read` + + `${stats.structuralEdgesError ? ` (${stats.structuralEdgesError})` : ''}` + + `${ + persistedRelationships === undefined + ? '; the graph-write-collapse check produced no verdict this run and any ' + + 'previously recorded collapse is kept rather than cleared.' + : `; falling back to the unfiltered edge count (${stats.edges}), which is ` + + 'equal to it on this run because no PDG layer was written.' + }`, + ); + } // NOT COMPARABLE ON AN INCREMENTAL WRITE. That path persists only // `extractChangedSubgraph(...)` while both counts here are whole-scope: the // full in-memory graph against the entire DB. A 10,000-edge index whose @@ -2803,9 +3009,40 @@ async function runFullAnalysisInner( // a collapse that did not happen. Producing no verdict is the honest answer // until the check is given the write-set delta to compare against; that is // the same fail-safe the `expected === 0` case already takes. - const graphWriteCollapsed = wroteChangedSubgraphOnly - ? undefined + const collapseVerdict: GraphWriteCollapseVerdict = wroteChangedSubgraphOnly + ? { verdict: 'unmeasurable', reason: 'incremental-write' } : detectGraphWriteCollapse(expectedRelationships, persistedRelationships); + const graphWriteCollapsed = + collapseVerdict.verdict === 'collapsed' + ? { expected: collapseVerdict.expected, persisted: collapseVerdict.persisted } + : undefined; + + // SPLIT ON THE VERDICT, NOT THE WRITE MODE. `saveMeta` is a full atomic + // overwrite, not a merge, so whichever branch omits the field DELETES the + // stamp from meta.json — and the stamp is what marks the index incomplete + // and forces the repairing rebuild. + // + // Three-way, explicitly: + // collapse detected -> stamp it + // healthy -> CLEAR it (the index really is healthy now) + // no verdict -> carry the previous stamp forward + // + // Keying on `wroteChangedSubgraphOnly` implemented that as a TWO-way and got + // the third case wrong wherever it arose on a FULL run: a run whose + // structural count could not be READ (the `catch {}` in `getLbugStats`, + // reachable through the `withConnLock` contention the comment on that call + // warns about) reaches no verdict, but took the "full run ⇒ clear it" + // branch and erased a stamp recording real, unrepaired loss. The next run + // then found nothing forcing a rebuild, took `alreadyUpToDate`, printed + // "Already up to date" and exited 0 — permanently, which is exactly the + // failure the stamp exists to prevent. + // + // Mirrors `branch: branchLabel ?? existingMeta?.branch` a few lines down in + // the meta write, which had the preserve-on-absence shape all along. + const persistedCollapseStamp = selectPersistedCollapseStamp( + collapseVerdict, + existingMeta?.graphWriteCollapsed, + ); if (graphWriteCollapsed) { log( `Warning: graph write incomplete — the pipeline produced ${expectedRelationships} ` + @@ -3223,9 +3460,10 @@ async function runFullAnalysisInner( // origin remote, which is fine: paths-only repos behave as // before. remoteUrl: hasGitDir(repoPath) ? getRemoteUrl(repoPath) : undefined, - // Absent on a healthy run; present it and the index reports as - // incomplete rather than fresh (`graph-write-collapsed`). - ...(graphWriteCollapsed ? { graphWriteCollapsed } : {}), + // Absent on a healthy FULL run; present it and the index reports as + // incomplete rather than fresh (`graph-write-collapsed`). Carried forward + // when this run had no verdict — see `persistedCollapseStamp`. + ...(persistedCollapseStamp ? { graphWriteCollapsed: persistedCollapseStamp } : {}), // R3-1. Not a health signal — the index is complete and correct. This // records which fields the per-language inference declined to link so a // later query can say WHY it is returning nothing, instead of leaving an diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 80058c4cf..f9783c75a 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -6029,12 +6029,33 @@ export class LocalBackend { // here; the flag is what lets a reader tell a broken fan-out from a // genuinely caller-less one. const anyKnownRisk = candidateSummaries.some((c) => RISK_ORDER.includes(c.risk)); - const maxRisk = anyKnownRisk + // The highest risk among candidates that actually RESOLVED. Kept as its + // own value rather than being folded into `maxRisk`, so narrowing the + // aggregate below does not throw away what was measured. + const knownMaxRisk = anyKnownRisk ? candidateSummaries.reduce( (worst, c) => (RISK_ORDER.indexOf(c.risk) > RISK_ORDER.indexOf(worst) ? c.risk : worst), 'LOW', ) : 'UNKNOWN'; + // UNKNOWN DOMINATES A MIXED SET, and that is the correction. + // + // The reasoning above covers the ALL-UNKNOWN case and stops there. The + // MIXED case fell through it: `RISK_ORDER` has no `UNKNOWN` entry, so + // `indexOf` returns -1 and an UNKNOWN candidate can never win the reduce. + // One caller-less candidate (UNKNOWN) beside one single-caller candidate + // (LOW) therefore reported `maxRisk: 'LOW'` — a confident floor over a + // set containing an interpretation nobody measured, which is the exact + // false-safe the all-UNKNOWN branch was written to prevent, one case over. + // + // `maxRisk` answers "how bad could this be?", and an unresolved candidate + // could be CRITICAL. So any UNKNOWN in the set makes the aggregate + // UNKNOWN, and `knownMaxRisk` carries the measured part alongside — the + // reader gets "at least LOW among what resolved, and one interpretation + // could not be walked at all", which is strictly more than either value + // alone. + const anyUnknownRisk = candidateSummaries.some((c) => !RISK_ORDER.includes(c.risk)); + const maxRisk = anyUnknownRisk ? 'UNKNOWN' : knownMaxRisk; // `candidateSummaries` is `Promise.all` over `probed`, so the two lengths // are the same; `probed` is the one the message and the flag agree on. const { atLeast, showing, fields } = ambiguityReport(outcome, probed.length, true); @@ -6044,7 +6065,11 @@ export class LocalBackend { message: `Found ${atLeast}${outcome.total} symbols matching '${target}'` + showing + - `. Blast radius differs per candidate (max ${maxImpactedCount} impacted at risk ${maxRisk}). ` + + `. Blast radius differs per candidate (max ${maxImpactedCount} impacted at risk ${maxRisk}` + + (anyUnknownRisk && anyKnownRisk + ? `; ${knownMaxRisk} among the candidates that resolved, and at least one could not be walked` + : '') + + `). ` + `Disambiguate with target_uid (or file_path/kind) for a single authoritative result.`, target: { name: target }, direction, @@ -6067,6 +6092,11 @@ export class LocalBackend { risk: 'UNKNOWN', maxImpactedCount, maxRisk, + // Present only when the two differ, i.e. when something resolved AND + // something did not. Absent on a fully-resolved set (where it would + // duplicate `maxRisk`) and on a fully-unknown one (where there is no + // measured part to report). + ...(anyUnknownRisk && anyKnownRisk ? { knownMaxRisk } : {}), ...(probeFailed ? { partialProbe: true } : {}), candidates: candidateSummaries, }; diff --git a/gitnexus/src/server/analyze-launch.ts b/gitnexus/src/server/analyze-launch.ts index 2bc685431..28199bac2 100644 --- a/gitnexus/src/server/analyze-launch.ts +++ b/gitnexus/src/server/analyze-launch.ts @@ -194,16 +194,78 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) { // Before marking complete: (1) wait for the worker's on-disk // finalization to settle (see waitForSettledIndex), (2) evict the // cached DB handle — same invalidation DELETE /api/repo performs, a - // handle opened before the rewrite reads pre-rewrite state — and - // only then (3) reinitialize the backend. This makes the ordering - // comment below true in practice: the repo is actually queryable - // when the client receives the SSE complete event. + // handle opened before the rewrite reads pre-rewrite state — (3) + // decide the outcome, and only then (4) reinitialize the backend, + // which is what PUBLISHES the index. This makes the ordering comment + // below true in practice: the repo is actually queryable when the + // client receives the SSE complete event, and an index this run knows + // to be incomplete is never published at all. waitForSettledIndex(targetPath, jobStartMs) .then(() => closeDbHandle()) .catch(() => {}) // best-effort: eviction failure must not fail the job - .then(() => backend.init()) .then(() => { - jobManager.updateJob(job.id, { status: 'complete', repoName: msg.result.repoName }); + // PARITY WITH THE CLI, which is what the IPC projection was added + // for. `analyze-worker-ipc.ts` carries `graphWriteCollapsed` + // "so a server-side caller sees the same degraded outcome the CLI + // does" — but nothing here read it, so the comment described an + // intention rather than the shipped behaviour and every collapsed + // run reported `complete` to the UI and to every API consumer. + // + // `failed` rather than `complete`, because that is the CLI's + // choice: it prints `Repository indexed INCOMPLETELY` and exits + // non-zero. The index exists but most of its edges do not, and a + // consumer that reads "complete" will query it and get confident + // wrong answers — the precise failure this whole guard exists to + // stop. The message names the remedy, and a re-run now forces a + // full rebuild on its own (see the `graphWriteCollapsed` trigger + // in run-analyze.ts). + // + // ── THE CHECK RUNS BEFORE `backend.init()`, AND THAT ORDER IS + // THE GUARD ── `backend.init()` is the PUBLISH step: it is + // `refreshRepos()`, which re-reads the registry and swaps the + // freshly-registered repo into the in-memory map every MCP tool + // and HTTP route resolves through. Running it first (as this + // chain used to) made the collapsed database live and queryable + // before the job was ever marked `failed`, so `status` was a + // label on an already-published index rather than a gate — and + // `backend-client.ts` routes the `failed` SSE event to + // `onError()` without ever calling `onComplete`, so the UI showed + // an error toast while every query answered from the incomplete + // graph. Publication cannot be undone from here (nothing on the + // backend un-registers a repo), so the only correct order is to + // decide first and publish second. + // + // `closeDbHandle()` above still runs on both paths, and must: the + // worker rewrote the DB files on disk, so a handle opened before + // the rewrite reads pre-rewrite state whatever the outcome was. + // Evicting it is not publication — it drops a cached connection, + // it does not add anything to the repo map. + const collapse = msg.result.graphWriteCollapsed; + if (collapse) { + // NOT published. `repoName` is reported even so: the success + // path sets it (`api.ts`'s repo-resolution wait matches jobs on + // `repoName` first and falls back to `repoUrl`/`repoPath` + // basenames), and a failure that drops it silently costs one of + // those three match keys for no reason. + jobManager.updateJob(job.id, { + status: 'failed', + repoName: msg.result.repoName, + error: + `Repository indexed INCOMPLETELY: only ${collapse.persisted} of ` + + `${collapse.expected} expected relationships are readable. The index was not ` + + `marked fresh and was NOT published to this server — a first-time analyze ` + + `stays unreachable until a run succeeds (a previously published index for ` + + `this repo keeps being served). Re-run the analysis — it will rebuild from ` + + `scratch.`, + }); + return; + } + // Healthy run only: publish, then report complete. This keeps the + // ordering comment above the chain true — the repo really is + // queryable when the client receives the SSE complete event. + return backend.init().then(() => { + jobManager.updateJob(job.id, { status: 'complete', repoName: msg.result.repoName }); + }); }) .catch((err) => { logger.error({ err }, 'backend.init() failed after analyze:'); diff --git a/gitnexus/src/storage/parse-cache.ts b/gitnexus/src/storage/parse-cache.ts index fca174dac..2e4a196de 100644 --- a/gitnexus/src/storage/parse-cache.ts +++ b/gitnexus/src/storage/parse-cache.ts @@ -388,7 +388,61 @@ import type { ParseWorkerResult } from '../core/ingestion/workers/parse-worker.j // published (46, 47) is superseded by 48, so a warm cache stamped with either is // correctly invalidated. // RE-CHECK AGAINST origin/main IMMEDIATELY BEFORE MERGING. -const SCHEMA_BUMP = 53; +// 53 -> 54 for W2-8: `@declaration.type-parameters` is now captured on generic +// FUNCTIONS, generator functions and type ALIASES in TYPESCRIPT_SCOPE_QUERY, not +// only on class/interface declarations. Parse-time emission, so a warm cache +// replays ParsedFiles whose defs carry no parameter list and the shadowing guard +// that consumes it silently does nothing — the feature would look implemented +// and be inert, which is the failure this constant exists to prevent. +// RE-CHECK AGAINST origin/main IMMEDIATELY BEFORE MERGING. +// 54 -> 55 for W2-9: the dispatch-guard verb walk now tracks boolean POLARITY, +// so `(req.method === 'GET' ? false : true) && pathname === '/x'` no longer +// reports GET — the one method that branch guarantees the request does not have +// — and `!!(req.method === 'GET')` no longer loses its verb. Routes are emitted +// at parse time and replayed verbatim from a warm cache, so without this bump an +// already-indexed repo keeps serving the inverted verb and the fix looks inert. +// Same reason 51 and 52 were taken for R3-7. +// RE-CHECK AGAINST origin/main IMMEDIATELY BEFORE MERGING. +// 55 -> 56 for R3-8 (part 1): a dispatch guard's verb walk now returns ALL the +// methods a guard serves, so `(req.method === 'GET' || req.method === 'POST') && +// pathname === '/x'` emits two routes instead of reporting GET alone, and a +// disjunction with a non-verb operand emits none instead of the first verb it +// saw. Routes are parse-time output replayed verbatim from a warm cache. +// RE-CHECK AGAINST origin/main IMMEDIATELY BEFORE MERGING. +// 56 -> 57 for R3-8 (part 2): `pathname.match(RE)` is read as a route test +// alongside `RE.test(pathname)`, a bound match takes its verb from where the +// binding is TESTED rather than where it is bound, a regex named by a same-file +// const resolves, and `regexToRoutePath` accepts a CAPTURING segment wildcard +// (`([^/]+)`) — the form every real dispatcher writes and the one it refused. +// All parse-time route output, replayed verbatim from a warm cache. +// RE-CHECK AGAINST origin/main IMMEDIATELY BEFORE MERGING. +// 57 -> 58 for #2897: the fetch capture no longer requires a LITERAL url, so a +// call passing a variable is recorded as an outward-action site. Measured, 44 of +// 47 fetch calls in this repo pass a variable, so the R3-6 sink signal was +// absent from 94% of them. Parse-time capture output replayed verbatim from a +// warm cache, so without the bump an indexed repo keeps its empty sink set and +// the fix looks inert. +// RE-CHECK AGAINST origin/main IMMEDIATELY BEFORE MERGING. +// 58 -> 59 for the #2899 REVIEW FOLLOW-UP to the dispatch-guard route walk. Two +// route-output changes, both parse-time and both replayed verbatim from a warm +// cache, so without this bump an already-indexed repo keeps serving the wrong +// routes and both fixes look implemented while being inert: +// (a) `matchBindings` / `tested` are keyed on (enclosing function, name) +// instead of the bare identifier. A same-named non-match binding in +// ANOTHER function used to mint a fabricated verbed route under the wrong +// handler — reproduced: `DELETE /api/live/positions/{param1}/replay +// handler=handleSettings` — which then EVICTED the true verb-less route +// through `reconcileDispatchGuardRoutes`. `buildRegexConstantMap` refuses +// a name rebound to a non-regex for the same reason. +// (b) `verbsFromTernary` INTERSECTS the operands of a conjunction instead of +// taking the first non-empty set. `(GET||POST) ? (POST||PUT) : false` +// emitted GET and POST where only POST is reachable, and +// `GET ? POST : false` emitted GET for an unsatisfiable guard. +// Both changes strictly REMOVE routes, so a stale cache serves strictly more +// wrong answers than a cold one — which is exactly the state this constant +// exists to make unreachable. Same reason 55, 56 and 57 were taken. +// RE-CHECK AGAINST origin/main IMMEDIATELY BEFORE MERGING. +const SCHEMA_BUMP = 59; const GITNEXUS_PKG_VERSION = (() => { try { // package.json sits at gitnexus/package.json — two levels up from diff --git a/gitnexus/test/fixtures/lang-resolution/member-call-producer/src/consumer.js b/gitnexus/test/fixtures/lang-resolution/member-call-producer/src/consumer.js new file mode 100644 index 000000000..d01896683 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/member-call-producer/src/consumer.js @@ -0,0 +1,25 @@ +import { SignalService, makeSignal } from './producer.js' + +export function readFree() { + const r = makeSignal() + return r.secretFlag +} + +export function readMake() { + const svc = new SignalService() + const r = svc.make() + return r.secretFlag +} + +export function readOther() { + const svc = new SignalService() + const r = svc.other() + return r.secretFlag +} + +// The member is on NEITHER method's shape — must stay unresolved. +export function readAbsent() { + const svc = new SignalService() + const r = svc.make() + return r.notOnAnyShape +} diff --git a/gitnexus/test/fixtures/lang-resolution/member-call-producer/src/producer.js b/gitnexus/test/fixtures/lang-resolution/member-call-producer/src/producer.js new file mode 100644 index 000000000..dc7b9da80 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/member-call-producer/src/producer.js @@ -0,0 +1,16 @@ +// THREE producers owning a `secretFlag`, so resolving to the wrong one is a +// detectable failure rather than a coin flip that happens to look right. +export class SignalService { + make() { + return { secretFlag: 'from-make', wickRatio: 0.5 } + } + + other() { + return { secretFlag: 'from-other' } + } +} + +// The free-function control. This already resolves (R3-5) and must keep doing so. +export function makeSignal() { + return { secretFlag: 'from-free', wickRatio: 0.9 } +} diff --git a/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/consumer.js b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/consumer.js new file mode 100644 index 000000000..7d1f22e8c --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/consumer.js @@ -0,0 +1,42 @@ +import { makeSpike, makeCandle } from './producer.js' + +// The W2-2 shape: a bare parameter receiver, typed only by what callers pass. +export function readSpike(spike) { + return spike.wickRatio +} + +export function callReadSpike() { + const s = makeSpike() + return readSpike(s) +} + +// AMBIGUOUS: two callers passing different producers. Must resolve to NEITHER. +export function readEither(thing) { + return thing.wickRatio +} + +export function callEitherA() { + const a = makeSpike() + return readEither(a) +} + +export function callEitherB() { + const b = makeCandle() + return readEither(b) +} + +// A parameter nobody calls with a typed argument — stays unresolved. +export function readUncalled(mystery) { + return mystery.wickRatio +} + +// TWO parameters: only the SECOND is a typed producer, so a rule that ignored +// the parameter index would type `first` from the wrong argument. +export function readSecond(first, second) { + return second.wickRatio +} + +export function callReadSecond() { + const c = makeCandle() + return readSecond(1, c) +} diff --git a/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/nested-block.js b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/nested-block.js new file mode 100644 index 000000000..57383d467 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/nested-block.js @@ -0,0 +1,19 @@ +import { makeSpike } from './producer.js' + +// CONTROL for the shadowing guard. +// +// The block declares a name, so it IS a scope the walk has to climb through — +// but not the RECEIVER's name, so the read still reaches the enclosing formal +// and must keep its precise edge. A guard that stops at any binding scope +// rather than at one that binds THIS name would silently delete the feature. +export function readThroughBlock(spike) { + { + const label = 1 + return label > 0 ? spike.wickRatio : 0 + } +} + +export function callReadThroughBlock() { + const s = makeSpike() + return readThroughBlock(s) +} diff --git a/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/other.js b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/other.js new file mode 100644 index 000000000..56709a7e2 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/other.js @@ -0,0 +1,13 @@ +import { makeCandle } from './producer.js' + +// A DIFFERENT function that happens to share the name `readSpike`. Its +// parameter must not be typed from the other file's callers, nor answer for +// them — the formal key carries the declaring file for exactly this. +export function readSpike(spike) { + return spike.source +} + +export function callLocalReadSpike() { + const c = makeCandle() + return readSpike(c) +} diff --git a/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/producer.js b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/producer.js new file mode 100644 index 000000000..db9f4b2b1 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/producer.js @@ -0,0 +1,9 @@ +// Two producers sharing a field name — the case name inference must refuse and +// the one this pass exists to answer with evidence. +export function makeSpike() { + return { wickRatio: 0.5, source: 'spike' } +} + +export function makeCandle() { + return { wickRatio: 0.9, source: 'candle' } +} diff --git a/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/same-file-method.js b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/same-file-method.js new file mode 100644 index 000000000..89294675e --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/same-file-method.js @@ -0,0 +1,22 @@ +import { makeSpike } from './producer.js' + +// SAME-FILE COLLISION, free function vs class method. +// +// The second shape of the same defect: `Runner.apply`'s formal owner is the +// bare identifier `apply`, so it collides with the free `apply` on +// (filePath, ownerName, parameterIndex) exactly as a nested function does. +export function apply(input) { + return input.source +} + +export class Runner { + // Same NAME as the free `apply`. No caller ever passes it a producer. + apply(input) { + return input.source + } +} + +export function callApply() { + const s = makeSpike() + return apply(s) +} diff --git a/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/same-file-nested.js b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/same-file-nested.js new file mode 100644 index 000000000..ea46078d3 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/same-file-nested.js @@ -0,0 +1,30 @@ +import { makeSpike } from './producer.js' + +// SAME-FILE COLLISION, free function vs nested function. +// +// A `formal` site names its owner with a BARE identifier, and one is emitted +// for every parameter of every callable in the file — nested functions +// included. So the free `parse` below and the `parse` nested inside `outer` +// key the formal index identically at parameter 0. +// +// Only the free one is ever called with a typed producer. A last-write-wins +// formal index therefore hands `makeSpike` to the parameter of the callable +// that never received it, and the fabricated edge lands at the 0.9 PRECISE +// tier where no `minConfidence` floor can filter it — while the genuine +// consumer is left untyped. Neither may be typed. +export function parse(row) { + return row.wickRatio +} + +export function callParse() { + const s = makeSpike() + return parse(s) +} + +export function outer() { + // Same NAME, different callable. No caller ever passes it a producer. + function parse(row) { + return row.wickRatio + } + return parse +} diff --git a/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/shadow-arrow.js b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/shadow-arrow.js new file mode 100644 index 000000000..fa4ca3aab --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/shadow-arrow.js @@ -0,0 +1,27 @@ +import { makeSpike } from './producer.js' + +// SHADOWING ARROW PARAMETER. +// +// `readShadowedArrow`'s own `spike` is typed by its caller, but the arrow +// declares its OWN `spike`. An anonymous arrow is dropped by the callable-flow +// collector (it cannot be named), so it emits no `formal` site and nothing +// marks the arrow's scope as binding the name — a walk that stops at the first +// scope carrying a PRODUCER climbs straight past it and types the arrow's +// parameter from the enclosing formal's callers. +// +// The arrow is handed to a LOCAL function rather than to `items.map(...)` on +// purpose: an unresolved call on a built-in would add a `call` drop to the +// receiver-resolution bench, whose gate counts calls only, for a reason that +// has nothing to do with what this fixture is testing. +function pick(fn) { + return fn +} + +export function readShadowedArrow(spike) { + return pick((spike) => spike.wickRatio) +} + +export function callReadShadowedArrow() { + const s = makeSpike() + return readShadowedArrow(s) +} diff --git a/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/shadow-const.js b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/shadow-const.js new file mode 100644 index 000000000..67f7b5c7c --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/parameter-producer/src/shadow-const.js @@ -0,0 +1,24 @@ +import { makeSpike } from './producer.js' + +// SHADOWING BLOCK-SCOPED CONST. +// +// The block rebinds `item` to an element of `rows`. The parameter `item` IS +// typed by its caller, so a walk that climbs to the first scope carrying a +// PRODUCER rather than the first scope carrying the NAME reads the block's +// `item` as the caller's producer. +// +// The initializer is a subscript on purpose: `const item = rows` would bind +// `item` to the alias `rows` through the type-binding channel, and the pass +// would decline before the scope walk ever ran — masking the defect instead of +// exercising it. +export function readShadowedConst(item, rows) { + { + const item = rows[0] + return item.wickRatio + } +} + +export function callReadShadowedConst() { + const s = makeSpike() + return readShadowedConst(s, []) +} diff --git a/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/aliased.ts b/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/aliased.ts new file mode 100644 index 000000000..6f991ae04 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/aliased.ts @@ -0,0 +1,9 @@ +import { Item as RowItem } from './values'; + +// The name WRITTEN here is `RowItem`; the def it resolves to is named `Item`. +// Only the written name can be shadowed, and no spelling of the parameter +// `` reaches this reference — so substituting the resolved def's name for +// the written one deletes an edge that was never shadowed at all. +export function useAliased(seed: Item): unknown { + return { render: RowItem, seed }; +} diff --git a/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/namespaced.ts b/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/namespaced.ts new file mode 100644 index 000000000..2a18d5a6e --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/namespaced.ts @@ -0,0 +1,19 @@ +export namespace Host { + export interface Inner { + ok: boolean; + } + + // The CONTROL for this file. `Host.Inner` has to be reachable at all before + // its absence from the generic below can mean anything. + export function readInner(v: Inner): boolean { + return v.ok; + } + + // The shadowed reference resolves to a def whose qualified name is + // `Host.Inner`. A rule that recovers the name by slicing the resolved graph id + // compares `Host.Inner` against the parameter `Inner`, misses, and keeps + // exactly the false edge it exists to remove. + export function hold(value: Inner): Inner { + return value; + } +} diff --git a/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/shapes.ts b/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/shapes.ts new file mode 100644 index 000000000..b65dea0d4 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/shapes.ts @@ -0,0 +1,37 @@ +// A declared contract, and a generic whose PARAMETER collides with its name. +// tsc reads both annotations in `unwrap` as the type parameter, not the +// interface — so a `USES` edge from `unwrap` reports a consumer of a contract it +// has no relationship with, at the same confidence as a real one. +export interface Result { + ok: boolean; +} + +export function unwrap(value: Result): Result { + return value; +} + +// The CONTROL. A genuine consumer of the interface, which must survive: the +// point is to stop shadowed references, not to disable the rule. +export function readResult(r: Result): boolean { + return r.ok; +} + +// Same collision on a generic type alias whose value is an OBJECT TYPE — the +// one alias form that opens a scope of its own. +export type Box = { held: Result }; + +// The same collision on the alias forms that open NO scope. A union, a +// conditional, a mapped type, an array, a tuple, a function type and a +// `Record` are all `type_alias_declaration`s whose value is not an +// `object_type`, so nothing anchors their parameters to a region of the file. +// A parameter list that binds nothing is harmless; one that binds the WHOLE +// MODULE deletes every `USES` edge in the file whose target is spelled +// `Result` — including `readResult` above, and including an imported type. +export type Maybe = Result | null; +export type Ids = Result[]; + +// A generic whose parameter does NOT collide — the interface reference inside +// it is real and must still link. +export function wrap(value: T, meta: Result): T { + return meta.ok ? value : value; +} diff --git a/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/values.ts b/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/values.ts new file mode 100644 index 000000000..76bc088b4 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/typescript-type-parameters/src/values.ts @@ -0,0 +1,12 @@ +// A VALUE and a TYPE PARAMETER may share a name — TypeScript keeps the type and +// value namespaces apart, so the function `Item` and the parameter `` are +// different symbols and neither can shadow the other. +export function Item(): void {} + +// `render: Item` is a value-ref (#2437), which maps to the same `USES` edge type +// as a type annotation. A shadowing rule keyed on the EDGE TYPE therefore has +// this registration in reach; keyed on the reference KIND it does not. The kind +// is what carries the meaning — the edge type is shared by three of them. +export function useRow(seed: Item): unknown { + return { render: Item, seed }; +} diff --git a/gitnexus/test/integration/impact-zero-caller-risk.test.ts b/gitnexus/test/integration/impact-zero-caller-risk.test.ts index 305fb51ba..3438ffc46 100644 --- a/gitnexus/test/integration/impact-zero-caller-risk.test.ts +++ b/gitnexus/test/integration/impact-zero-caller-risk.test.ts @@ -36,6 +36,13 @@ const SEED = [ // single-symbol one. `CREATE (t1:Function {id: 'Function:src/a.ts:orphanTwin', name: 'orphanTwin', filePath: 'src/a.ts', startLine: 1, endLine: 3, isExported: true, content: '', description: ''})`, `CREATE (t2:Function {id: 'Function:src/b.ts:orphanTwin', name: 'orphanTwin', filePath: 'src/b.ts', startLine: 1, endLine: 3, isExported: true, content: '', description: ''})`, + // MIXED ambiguity (W2-4): two symbols sharing a name where ONE has a caller + // (resolves to LOW) and the other has none (UNKNOWN). The all-UNKNOWN pair + // above cannot reach this case, which is exactly why it went unnoticed. + `CREATE (m1:Function {id: 'Function:src/m1.ts:mixedTwin', name: 'mixedTwin', filePath: 'src/m1.ts', startLine: 1, endLine: 3, isExported: true, content: '', description: ''})`, + `CREATE (m2:Function {id: 'Function:src/m2.ts:mixedTwin', name: 'mixedTwin', filePath: 'src/m2.ts', startLine: 1, endLine: 3, isExported: true, content: '', description: ''})`, + `CREATE (mc:Function {id: 'Function:src/mcaller.ts:mixedCaller', name: 'mixedCaller', filePath: 'src/mcaller.ts', startLine: 1, endLine: 8, isExported: true, content: '', description: ''})`, + `MATCH (a:Function {id:'Function:src/mcaller.ts:mixedCaller'}), (b:Function {id:'Function:src/m1.ts:mixedTwin'}) CREATE (a)-[:CodeRelation {type:'CALLS', confidence:0.9, reason:'direct', step:0}]->(b)`, ]; type BackendHandle = IndexedDBHandle & { _backend?: LocalBackend }; @@ -131,6 +138,49 @@ withTestLbugDB( expect(result.risk).toBe('LOW'); expect(result.riskNote).toBeUndefined(); }); + + // ── W2-4: a MIXED candidate set must not report the known floor ── + // + // The all-UNKNOWN branch above is reasoned about carefully and is right. + // The mixed case fell straight through it: `RISK_ORDER` has no `UNKNOWN` + // entry, so `indexOf` returns -1 and an UNKNOWN candidate can never win the + // reduce. One caller-less candidate beside one single-caller candidate + // therefore reported `maxRisk: 'LOW'` — a confident floor over a set that + // contains an interpretation nobody measured. + describe('a mixed UNKNOWN/LOW candidate set (W2-4)', () => { + it('reports UNKNOWN, not the known floor', async () => { + const result = await backend.callTool('impact', { + target: 'mixedTwin', + direction: 'upstream', + }); + // Asserted first: if this stopped being ambiguous, the rest is vacuous. + expect(result.status).toBe('ambiguous'); + expect(result.maxRisk).toBe('UNKNOWN'); + }); + + it('still reports what DID resolve, so narrowing costs no information', async () => { + const result = await backend.callTool('impact', { + target: 'mixedTwin', + direction: 'upstream', + }); + // The measured part travels alongside rather than being discarded: a + // reader gets "at least LOW among what resolved, and one interpretation + // could not be walked", which is strictly more than either alone. + expect(result.knownMaxRisk).toBe('LOW'); + }); + + it('omits knownMaxRisk when nothing resolved', async () => { + // The all-UNKNOWN pair: there is no measured part, so the field must be + // absent rather than echoing UNKNOWN twice. + const result = await backend.callTool('impact', { + target: 'orphanTwin', + direction: 'upstream', + }); + expect(result.status).toBe('ambiguous'); + expect(result.maxRisk).toBe('UNKNOWN'); + expect(result.knownMaxRisk).toBeUndefined(); + }); + }); }, { seed: SEED, diff --git a/gitnexus/test/integration/lbug-core-adapter.test.ts b/gitnexus/test/integration/lbug-core-adapter.test.ts index 7af37a108..f66d25e3b 100644 --- a/gitnexus/test/integration/lbug-core-adapter.test.ts +++ b/gitnexus/test/integration/lbug-core-adapter.test.ts @@ -106,6 +106,67 @@ withTestLbugDB( // 4 relationships (2 CALLS, 2 CONTAINS) expect(stats.edges).toBe(4); + + // STRUCTURAL count must be a real number, not `undefined`. + // + // This assertion exists because the failure mode is silent: the query is + // wrapped in a try/catch that yields `undefined` on error, and + // `undefined` makes the graph-write-collapse check decline to compare. + // A typo in the Cypher would therefore not throw, not fail any test, and + // simply switch the collapse guard off — the exact shape of + // confidently-doing-nothing this whole area exists to prevent. + // + // The seeded graph has no PDG layers, so structural == total here; the + // point is that the count was TAKEN. + expect(stats.structuralEdges).toBe(4); + + // ...and that it was taken WITHOUT an error, which is the fact the + // collapse guard reads to tell "measured" from "could not measure". + expect(stats.structuralEdgesError).toBeUndefined(); + }); + + it('getLbugStats: the structural count EXCLUDES PDG rows that `edges` counts', async () => { + // The assertion above cannot see the `WHERE NOT r.type IN [...]` filter + // at all — its own comment says "structural == total here" — so a broken + // or dropped predicate would pass it unchanged while silently switching + // the graph-write-collapse guard from a structural comparison back to + // the total one that let PDG volume mask structural loss. + // + // Seeds a real PDG-typed row (same CREATE pattern as the + // deleteAllInterprocTaintPaths test below) and asserts the two counts + // DIVERGE by exactly it. Removed again at the end: the count-based + // assertions in this file share one singleton DB and run in declaration + // order. + const { getLbugStats, executeQuery: coreExecuteQuery } = + await import('../../src/core/lbug/lbug-adapter.js'); + + const before = await getLbugStats(); + expect(before.edges).toBe(before.structuralEdges); + + const fns = (await coreExecuteQuery('MATCH (n:Function) RETURN n.id AS id')) as { + id: string; + }[]; + expect(fns.length).toBe(2); + await coreExecuteQuery( + `MATCH (a:Function {id: '${fns[0].id}'}), (b:Function {id: '${fns[1].id}'}) ` + + `CREATE (a)-[:CodeRelation {type: 'CFG', confidence: 1.0, reason: 'seq', step: 0}]->(b)`, + ); + + try { + const after = await getLbugStats(); + // The total sees the new row... + expect(after.edges).toBe((before.edges ?? 0) + 1); + // ...and the structural count does NOT. + expect(after.structuralEdges).toBe(before.structuralEdges); + expect(after.structuralEdgesError).toBeUndefined(); + } finally { + await coreExecuteQuery(`MATCH ()-[r:CodeRelation]->() WHERE r.type = 'CFG' DELETE r`); + } + + // Restored, so the later count-based assertions still see the seeded graph. + const restored = await getLbugStats(); + expect(restored.edges).toBe(before.edges); + expect(restored.structuralEdges).toBe(before.structuralEdges); }); it('deleteAllInterprocTaintPaths: removes TAINT_PATH edges and is benign when none exist (#2084 review P2-5)', async () => { diff --git a/gitnexus/test/integration/resolvers/member-call-producer.test.ts b/gitnexus/test/integration/resolvers/member-call-producer.test.ts new file mode 100644 index 000000000..a023d96b0 --- /dev/null +++ b/gitnexus/test/integration/resolvers/member-call-producer.test.ts @@ -0,0 +1,78 @@ +/** + * MEMBER-CALL PRODUCERS (W2-1). + * + * `const svc = new SignalService(); const r = svc.make(); r.secretFlag` produced + * no edge. `return-shape-members` types the receiver `r` to the producer that + * made it, but a member call binds the spelling `svc.make`, and slicing that to + * its last segment leaves `make` — which is a METHOD, not a callable binding in + * scope, so the producer lookup failed and the pass declined. + * + * The note this item shipped with said answering it needed inter-procedural + * receiver typing. Measured, the pipeline had already done the hard part: + * - `readMake -> Method:…SignalService.make#0` resolves as a CALLS edge, and + * - `Property:…SignalService.make.secretFlag@N:C` already exists, because R3-4 + * anchors a returned literal's keys to the METHOD that returns them too. + * Only the ACCESSES edge between the two was missing. + * + * The fixture gives THREE producers a `secretFlag` — `SignalService.make`, + * `SignalService.other` and the free function `makeSignal` — so resolving to the + * wrong owner is a detectable failure rather than a coin flip that looks right. + */ +import { describe, it, expect, beforeAll } from 'vitest'; +import path from 'path'; +import { FIXTURES, getRelationships, runPipelineFromRepo, type PipelineResult } from './helpers.js'; + +describe('member-call producers (W2-1)', () => { + let result: PipelineResult; + + beforeAll(async () => { + result = await runPipelineFromRepo(path.join(FIXTURES, 'member-call-producer'), () => {}); + }, 60000); + + /** Return-shape ACCESSES targets for one reader, by target node id. */ + const shapeTargetsOf = (reader: string): string[] => + getRelationships(result, 'ACCESSES') + .filter((e) => e.source === reader && e.targetLabel === 'Property') + .map((e) => e.rel.targetId); + + it('still resolves a FREE-function producer at the precise tier', () => { + // Asserted first: every assertion below is vacuous if the pass stopped + // emitting altogether, which is the obvious wrong way to "fix" this. + const edge = getRelationships(result, 'ACCESSES').find( + (e) => e.source === 'readFree' && e.targetLabel === 'Property', + ); + expect(edge).toBeDefined(); + expect(edge!.rel.targetId).toContain('makeSignal.secretFlag'); + expect(edge!.rel.confidence).toBe(0.9); + }); + + it('resolves a member-call producer to the METHOD that returned the shape', () => { + const targets = shapeTargetsOf('readMake'); + expect(targets).toHaveLength(1); + expect(targets[0]).toContain('SignalService.make.secretFlag'); + }); + + it('separates two methods on the SAME class that own the same member name', () => { + // The discriminating case. Matching on the last segment (`make` / `other`) + // alone cannot tell these apart from each other or from the free function, + // because the owner qualifier in the node id is `.`. + const targets = shapeTargetsOf('readOther'); + expect(targets).toHaveLength(1); + expect(targets[0]).toContain('SignalService.other.secretFlag'); + }); + + it('does not let a member-call reader reach the FREE function of the same shape', () => { + // `makeSignal` owns a `secretFlag` too. A whole-graph textual join on + // `.secretFlag` would happily return it. + for (const target of [...shapeTargetsOf('readMake'), ...shapeTargetsOf('readOther')]) { + expect(target).not.toContain('makeSignal.secretFlag'); + } + }); + + it('claims nothing when the member is on NEITHER shape', () => { + // The receiver is typed and the producer's shape is known, so this is a + // disproof, not an absence of evidence — it must not fall through to the + // 0.5 name tier and get answered by an unrelated same-named key. + expect(shapeTargetsOf('readAbsent')).toEqual([]); + }); +}); diff --git a/gitnexus/test/integration/resolvers/parameter-producer.test.ts b/gitnexus/test/integration/resolvers/parameter-producer.test.ts new file mode 100644 index 000000000..ccac6e9ff --- /dev/null +++ b/gitnexus/test/integration/resolvers/parameter-producer.test.ts @@ -0,0 +1,145 @@ +/** + * CALLER-DERIVED PARAMETER TYPES (W2-2). + * + * `function f(spike) { return spike.wickRatio }` had nothing to type `spike` + * from, so the read fell through to the 0.5 name tier. That is the standing + * limit of R3-5 and, measured on the reporting repo, by far the largest one: + * 11,012 of 13,672 property edges (81%) rest on that name guess. + * + * The two facts needed were already extracted for the callable-value-flow + * solver — a `formal` site naming a function's parameter by index, and an + * `argument` site naming what reaches that index at a call. Joining them types + * the parameter from its callers with no new capture and no parse-time change. + * + * Two producers here share `wickRatio` ON PURPOSE. That is precisely the shape + * name inference must refuse, so an edge to the RIGHT one is only meaningful + * while the wrong one is also a candidate. + */ +import { describe, it, expect, beforeAll } from 'vitest'; +import path from 'path'; +import { FIXTURES, getRelationships, runPipelineFromRepo, type PipelineResult } from './helpers.js'; + +describe('caller-derived parameter types (W2-2)', () => { + let result: PipelineResult; + + beforeAll(async () => { + result = await runPipelineFromRepo(path.join(FIXTURES, 'parameter-producer'), () => {}); + }, 60000); + + /** + * Precise (0.9) return-shape targets for one reader. + * + * `inFile` is not decoration: the fixture deliberately declares TWO functions + * named `readSpike`, so filtering on the name alone would merge two different + * symbols' edges and report a passing count for the wrong reason. + */ + const preciseTargetsOf = (reader: string, inFile?: string): string[] => + getRelationships(result, 'ACCESSES') + .filter( + (e) => + e.source === reader && + e.targetLabel === 'Property' && + e.rel.confidence === 0.9 && + (inFile === undefined || e.sourceFilePath.endsWith(inFile)), + ) + .map((e) => e.rel.targetId); + + it('types a bare parameter from its single caller', () => { + const targets = preciseTargetsOf('readSpike', 'consumer.js'); + expect(targets).toHaveLength(1); + expect(targets[0]).toContain('makeSpike.wickRatio'); + }); + + it('does not reach the OTHER producer of the same field name', () => { + // `makeCandle.wickRatio` exists and is a candidate for any name-based join. + expect(preciseTargetsOf('readSpike', 'consumer.js')[0]).not.toContain('makeCandle'); + }); + + it('claims nothing when two callers pass DIFFERENT producers', () => { + // Which shape `thing` holds depends on the call. Picking one would fabricate + // at the 0.9 precise tier, which no `minConfidence` floor can filter out. + expect(preciseTargetsOf('readEither')).toEqual([]); + }); + + it('claims nothing for a parameter no caller types', () => { + expect(preciseTargetsOf('readUncalled')).toEqual([]); + }); + it('matches the formal by PARAMETER INDEX, not merely by callee', () => { + // `readSecond(first, second)` is called as `readSecond(1, c)`. Only index 1 + // carries a producer; a rule that ignored the index would type `first`. + const targets = preciseTargetsOf('readSecond'); + expect(targets).toHaveLength(1); + expect(targets[0]).toContain('makeCandle.wickRatio'); + }); + + it('keeps same-named functions in different files apart', () => { + // `other.js` declares its own `readSpike`, called with a DIFFERENT producer. + // Keyed without the declaring file, the two formals collide and both + // parameters go ambiguous — so both readers would silently lose their edge. + const here = preciseTargetsOf('readSpike', 'consumer.js'); + const there = preciseTargetsOf('readSpike', 'other.js'); + expect(here).toHaveLength(1); + expect(here[0]).toContain('makeSpike.wickRatio'); + // The other file's twin is typed from ITS caller, not from this one's. + expect(there).toHaveLength(1); + expect(there[0]).toContain('makeCandle.source'); + }); + + /** + * Every precise (0.9) return-shape target emitted from ONE fixture file. + * + * Scoped by FILE rather than by reader name because the fixtures below turn + * on two callables sharing a name: filtering by the name would report which + * of the twins was typed, and the property under test is that NEITHER is. + */ + const preciseTargetsInFile = (file: string): string[] => + getRelationships(result, 'ACCESSES') + .filter( + (e) => + e.targetLabel === 'Property' && + e.rel.confidence === 0.9 && + e.sourceFilePath.endsWith(file), + ) + .map((e) => e.rel.targetId); + + it('keeps same-named callables in ONE file apart — free vs nested function', () => { + // The declaring FILE separates `readSpike` from `other.js`'s twin, but not + // a free `parse` from a `parse` nested inside `outer`: a `formal`'s owner is + // a bare identifier, so both key parameter 0 identically. Last-write-wins + // then gives `callParse`'s `makeSpike` to whichever formal was visited last + // — an edge at the 0.9 PRECISE tier for a call that never happened. + expect(preciseTargetsInFile('same-file-nested.js')).toEqual([]); + }); + + it('keeps same-named callables in ONE file apart — free function vs method', () => { + // `Runner.apply` owns its formal under the bare name `apply`, so it + // collides with the free `apply` exactly as the nested function does. + expect(preciseTargetsInFile('same-file-method.js')).toEqual([]); + }); + + it('does not type a shadowing ARROW parameter from the enclosing formal', () => { + // `items.map((spike) => spike.wickRatio)` inside `readShadowedArrow(spike, …)`. + // The arrow rebinds `spike`; an anonymous arrow emits no `formal` site, so + // its scope looks empty to a producer-only walk and the ARRAY ELEMENT gets + // typed from the outer parameter's callers. + expect(preciseTargetsInFile('shadow-arrow.js')).toEqual([]); + }); + + it('does not type a shadowing block-scoped CONST from the enclosing formal', () => { + // `{ const item = rows[0]; return item.wickRatio }` inside + // `readShadowedConst(item, rows)`. The block binds the name nearer than the + // formal whose callers were measured. The initializer is a subscript + // deliberately: `const item = rows` would bind `item` through the + // type-binding alias channel and the pass would decline before the scope + // walk ever ran, passing this test for the wrong reason. + expect(preciseTargetsInFile('shadow-const.js')).toEqual([]); + }); + + it('still reaches the formal through a block that shadows nothing', () => { + // The guard must stop at a scope binding THIS name, not at any binding + // scope: `{ const label = 1; … spike.wickRatio }` still reads the formal. + const targets = preciseTargetsInFile('nested-block.js'); + expect(targets).toHaveLength(1); + expect(targets[0]).toContain('makeSpike.wickRatio'); + }); +}); diff --git a/gitnexus/test/integration/resolvers/typescript-type-parameters.test.ts b/gitnexus/test/integration/resolvers/typescript-type-parameters.test.ts new file mode 100644 index 000000000..08db848fc --- /dev/null +++ b/gitnexus/test/integration/resolvers/typescript-type-parameters.test.ts @@ -0,0 +1,147 @@ +/** + * A TYPE PARAMETER SHADOWS A DECLARED TYPE OF THE SAME NAME (W2-8). + * + * `export function unwrap(value: Result): Result` names the parameter, + * not the `interface Result` beside it. The type-reference capture that makes a + * contract answerable ("what breaks if I remove this field?") had no notion of a + * parameter binding, so every annotation mentioning `Result` inside `unwrap` + * minted a `USES` edge into the interface — at the same confidence as a real + * consumer and indistinguishable from one. + * + * Measured on the first four declarations of `shapes.ts` before any of this: + * `unwrap` produced TWO false edges while `readResult` produced the one correct + * edge. Measured on the file as it stands now, with the first cut of the rule in + * place and the two scope-less generic aliases added: ZERO edges, correct ones + * included, across all four fixture files. See below. + * + * The blast radius is every generic whose parameter name collides with a + * declared type — `Result`, `Key`, `Value`, `Item`, `Node`, `Options`, `Config`, + * `Props`, `State`, `Response` are all ordinary choices for both. + * + * #2833 introduced `bindsTypeParameter` for the CALL-receiver path, where a + * workspace `class T` was answering for ``. It could not fix this one, + * because `@declaration.type-parameters` was captured for class/interface + * declarations only — a generic FUNCTION recorded no parameter list at all, so + * the predicate correctly returned false ("absence is not evidence"). The fix is + * therefore in two halves: capture the parameters on generic functions and + * aliases, then consult them where a type reference is RESOLVED. + * + * ── WHY EVERY ARM BELOW EXISTS ──────────────────────────────────────────────── + * + * OVER-SUPPRESSION IS THE EXPENSIVE DIRECTION and the reason the fixture grew. + * A deleted edge answers "nothing uses this" for code that does, and nothing + * anywhere reports that an edge was removed — so the only way to know is to + * assert the edges that must SURVIVE, beside the ones that must not. + * + * · `readResult` / `wrap` (shapes.ts) FAIL without the fix. `Maybe` and `Ids` + * are generic aliases whose value is not an object type, so they open no + * scope of their own and their parameter list is owned by the MODULE. Read + * there, `Result` is bound as a type parameter in EVERY scope in the file + * and the file loses ALL of its genuine `USES` edges — including the control + * declared above the aliases, and measured at zero remaining edges across + * the four files. + * + * The remaining arms PASS both with and without the fix and are labelled as such + * on purpose. Each is a boundary the rule sits next to and must not creep + * across, and each is only reachable today by an accident of routing that a + * future change could remove: + * + * · `useRow` (values.ts) — a `USES` edge is not always a type annotation: + * `type-reference`, `value-ref` (#2437) and `macro` (#1934) all map to it, + * and TypeScript keeps types and values in separate namespaces, so `` + * cannot shadow the FUNCTION `Item`. Value refs happen to be emitted by + * `emitPropertyDispatchCalls` rather than through the resolver, so a rule + * keyed on the emitted EDGE TYPE never reached them — but only by routing. + * Keyed on the reference KIND, as it now is, it cannot reach them at all. + * · `useAliased` (aliased.ts) — written `RowItem`, resolves to a def named + * `Item`. Only the written name can shadow, and it does not. + * · `hold` / `readInner` (namespaced.ts) — the shadowed reference and the + * genuine one, in the same namespace, so the absence means "suppressed" + * rather than "nothing resolved". A rule that recovers the name from the + * resolved graph id gets `hold` right only while TypeScript happens to key + * that node on the bare `Inner`; write the qualified `Host.Inner` there — + * as other languages do — and the false edge comes back. + * + * NOT PINNED HERE, deliberately: the alias-vs-resolved-name split on an imported + * TYPE. TypeScript emits no cross-file `USES` edge for a type annotation at all + * today — verified on this fixture both with and without a colliding parameter — + * so an assertion on one would pass for the wrong reason in both directions. + * `aliased.ts` therefore makes the point through an imported VALUE, which does + * resolve across files. + */ +import { describe, it, expect, beforeAll } from 'vitest'; +import path from 'path'; +import { FIXTURES, getRelationships, runPipelineFromRepo, type PipelineResult } from './helpers.js'; + +describe('TypeScript type-parameter shadowing (W2-8)', () => { + let result: PipelineResult; + + beforeAll(async () => { + result = await runPipelineFromRepo(path.join(FIXTURES, 'typescript-type-parameters'), () => {}); + }, 60000); + + const usersOf = (typeName: string): string[] => + getRelationships(result, 'USES') + .filter((e) => e.target === typeName) + .map((e) => e.source); + + const reasonsFor = (targetName: string, sourceName: string): string[] => + getRelationships(result, 'USES') + .filter((e) => e.target === targetName && e.source === sourceName) + .map((e) => e.rel.reason); + + it('links a genuine consumer of the interface', () => { + // Asserted FIRST: every absence below is vacuous if the rule stopped + // emitting entirely, which is the obvious wrong way to "fix" this. + expect(usersOf('Result')).toContain('readResult'); + }); + + it('does not link a generic whose parameter shadows the type name', () => { + expect(usersOf('Result')).not.toContain('unwrap'); + }); + + it('does not link a generic type alias whose parameter shadows it', () => { + expect(usersOf('Result')).not.toContain('Box'); + }); + + it('still links a real reference inside a generic that does NOT collide', () => { + // `wrap` annotates `meta: Result`, which is the interface — the shadowing + // rule must be keyed on the actual parameter names, not on "is generic". + expect(usersOf('Result')).toContain('wrap'); + }); + + it('keeps the whole file answerable when a generic alias opens no scope', () => { + // `Maybe` / `Ids` are the alias forms that own no scope, so + // their parameters are owned by the module. Both consumers above sit in that + // same module and are the measurement: one un-anchored parameter list takes + // every one of them out at once. + expect(usersOf('Result').sort()).toEqual(['readResult', 'wrap']); + }); + + it('keeps a value reference whose name collides with an enclosing parameter', () => { + // The type and value namespaces are separate — `` cannot shadow the + // FUNCTION `Item`. The reason is asserted because it is the discriminator: + // widen the rule to the emitted edge type and this same registration is the + // first thing it deletes. + expect(usersOf('Item')).toContain('useRow'); + expect(reasonsFor('Item', 'useRow')).toEqual(['scope-resolution: value-ref']); + }); + + it('keeps a reference written under an import alias inside a colliding generic', () => { + // Written `RowItem`, resolves to a def named `Item`. Only the written name + // can shadow, and it does not — so this survives whether the rule reads the + // written name (it does) or is widened to reach value references (it must + // not, and then this is what says so). + expect(usersOf('Item')).toContain('useAliased'); + }); + + it('links a genuine consumer declared inside a namespace', () => { + // The control for the arm below — `Host.Inner` has to be reachable at all + // before its absence from a generic can mean anything. + expect(usersOf('Inner')).toContain('readInner'); + }); + + it('drops the shadowed reference to a namespace-qualified type', () => { + expect(usersOf('Inner')).not.toContain('hold'); + }); +}); diff --git a/gitnexus/test/unit/ai-context-unknown-risk-policy.test.ts b/gitnexus/test/unit/ai-context-unknown-risk-policy.test.ts new file mode 100644 index 000000000..b906a1975 --- /dev/null +++ b/gitnexus/test/unit/ai-context-unknown-risk-policy.test.ts @@ -0,0 +1,52 @@ +import { describe, it, expect } from 'vitest'; +import { generateGitNexusContent } from '../../src/cli/ai-context.js'; + +// Regression guard for #2899. The `risk: UNKNOWN` Always-Do bullet and its +// Never-Do clause describe `impact`'s risk semantics, which are not +// PDG-dependent — unlike the `pdg_query` bullet (gated on `hasPdg`, see +// ai-context.test.ts's "gates the pdg_query line on hasPdg" test), these two +// must render in the generated block regardless of +// hasPdg. +// +// They were previously hand-added INSIDE the machine-managed block of the +// committed AGENTS.md/CLAUDE.md instead of living in this template, so every +// real `gitnexus analyze` run silently deleted them on regeneration — twice +// (#2856's 8f8261021, then #2899's own 9e602aef0, which piggybacked an +// unrelated fetch-parsing fix and also regressed the checked-in index stats +// 248612/565510/918 -> 42853/135955/758, itself evidence the docs had been +// regenerated from a stale local index rather than hand-edited). Moving the +// two lines into generateGitNexusContent (src/cli/ai-context.ts) is the +// actual fix; this test is what keeps them there. A second, independent +// guard reads the committed AGENTS.md/CLAUDE.md docs directly — see +// "root AGENTS.md / CLAUDE.md managed block keeps the risk: UNKNOWN policy +// (#2899)" in shipped-skills-sync.test.ts — so a hand-revert or a stale +// generator binary is caught even if this template-level test somehow isn't. +describe('generateGitNexusContent keeps the risk: UNKNOWN policy unconditional (#2899)', () => { + const stats = { nodes: 50, edges: 100, processes: 5 }; + + it.each([true, false])( + 'renders both the Always-Do bullet and Never-Do clause when hasPdg=%s', + (hasPdg) => { + const content = generateGitNexusContent('UnknownRiskProject', stats, { hasPdg }); + + expect(content).toContain('MUST treat `risk: UNKNOWN` as unresolved, not as low.'); + expect(content).toContain( + 'callers are not resolvable by the index (plain-object property access, dynamic dispatch, cross-language calls)', + ); + expect(content).toContain('`impact` pairs `UNKNOWN` with a `riskNote` saying so'); + + expect(content).toContain('never read `UNKNOWN` as an all-clear'); + expect(content).toContain( + 'it means the walk could not answer, which is the one verdict that requires confirming by other means', + ); + }, + ); + + it('keeps the pdg_query bullet correctly gated on hasPdg while the UNKNOWN policy stays unconditional', () => { + // Guards against a fix that accidentally moves the UNKNOWN policy inside + // the hasPdg branch instead of leaving it unconditional. + const withoutPdg = generateGitNexusContent('PlainProject', stats); + expect(withoutPdg).toContain('MUST treat `risk: UNKNOWN` as unresolved, not as low.'); + expect(withoutPdg).not.toContain('pdg_query'); + }); +}); diff --git a/gitnexus/test/unit/ai-context.test.ts b/gitnexus/test/unit/ai-context.test.ts index 30cf9cff2..1fbd558b8 100644 --- a/gitnexus/test/unit/ai-context.test.ts +++ b/gitnexus/test/unit/ai-context.test.ts @@ -308,9 +308,14 @@ describe('generateAIContextFiles', () => { // legitimate future additions but will fail loudly if the trim is // reverted or someone pads the block back out toward the original size. // - // Raised 2700 → 2900 for #243, then 2900 → 2950 for the bunx bootstrap note - // — each time with the same argument, that the added line is load-bearing and - // the block is still about half its old size. That is a ratchet with no + // Raised 2700 → 2900 for #243, then 2900 → 2950 for the bunx bootstrap note, + // then 0.55 → 0.65 for the #2899 `risk: UNKNOWN` Always-Do bullet + Never-Do + // clause (previously hand-added inside the committed docs instead of this + // template, so a real `gitnexus analyze` silently deleted them on every + // regeneration — moving them into the template is the fix, and they are + // unconditional text load-bearing enough to warrant the budget) — each time + // with the same argument, that the added line is load-bearing and the block + // is still meaningfully smaller than the original. That is a ratchet with no // ratchet: an absolute cap can only ever fail on the PR that adds the // character, and the fix is always to nudge the number. Assert the invariant // the justifications actually appeal to — the RATIO to the pre-trim size — @@ -326,7 +331,7 @@ describe('generateAIContextFiles', () => { content.indexOf(''), content.indexOf(''), ); - expect(block.length).toBeLessThan(PRE_TRIM_BLOCK_CHARS * 0.55); + expect(block.length).toBeLessThan(PRE_TRIM_BLOCK_CHARS * 0.65); }); it('handles empty stats', async () => { diff --git a/gitnexus/test/unit/analyze-launch-collapse.test.ts b/gitnexus/test/unit/analyze-launch-collapse.test.ts new file mode 100644 index 000000000..5eee43bdf --- /dev/null +++ b/gitnexus/test/unit/analyze-launch-collapse.test.ts @@ -0,0 +1,216 @@ +/** + * `createLaunchAnalysisWorker`'s collapsed-index guard — ORDERING, not just status. + * + * `backend.init()` is the PUBLISH step (it is `LocalBackend.refreshRepos()`, + * which swaps the freshly-registered repo into the in-memory map every MCP tool + * and HTTP route resolves through). The guard added in #2899 read + * `graphWriteCollapsed` only AFTER that call had already resolved, so a + * known-incomplete database was live and queryable before the job was ever + * marked `failed` — the job status was a label on a published index rather than + * a gate. These tests pin the order, because the order is the defect. + * + * `analyze-launch.ts` had ZERO test coverage before this file, which is why a + * field-name drift against `analyze-worker-ipc.ts`'s wire shape would have made + * the branch permanently dead and silently restored the pre-guard behaviour. + * The worker messages below are therefore built by calling the PRODUCTION + * projection `projectAnalyzeResultForIpc` rather than hand-rolling a literal, so + * a rename of `graphWriteCollapsed` breaks these tests instead of disabling the + * branch they cover. + */ +import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest'; +import { EventEmitter } from 'node:events'; + +// `vi.mock` factories are hoisted above every top-level `const`, and this file +// imports the module under test statically — so anything a factory closes over +// must be hoisted with it. +const H = vi.hoisted(() => ({ + forkMock: vi.fn(), + STORAGE_PATH: '/tmp/gitnexus-test-storage', + REPO_PATH: '/tmp/gitnexus-test-repo', + METADATA_FILE: 'gitnexus.json', +})); +const { forkMock, REPO_PATH } = H; + +vi.mock('child_process', async () => { + const actual = await vi.importActual('child_process'); + return { ...actual, fork: H.forkMock }; +}); + +// The launcher's finalization gate (`waitForSettledIndex`) probes the registry +// and the filesystem. Pin both so the gate settles on its FIRST poll — the gate +// itself is not under test here and its 200ms poll would otherwise put a real +// timer between the worker message and the assertions. +vi.mock('../../src/storage/repo-manager.js', () => ({ + canonicalizePath: (p: string) => p, + getStoragePath: () => H.STORAGE_PATH, + INDEX_METADATA_FILE: H.METADATA_FILE, + listRegisteredRepos: async () => [{ path: H.REPO_PATH, storagePath: H.STORAGE_PATH }], + registryPathEquals: (a: string, b: string) => a === b, +})); + +vi.mock('node:fs', async () => { + const actual = await vi.importActual('node:fs'); + return { + ...actual, + // Both index files were (re)written far in the future relative to jobStartMs… + statSync: () => ({ mtimeMs: Number.MAX_SAFE_INTEGER }), + // …and no WAL/shadow/checkpoint sidecar remains. + existsSync: () => false, + }; +}); + +import { createLaunchAnalysisWorker } from '../../src/server/analyze-launch.js'; +import { JobManager } from '../../src/server/analyze-job.js'; +import { projectAnalyzeResultForIpc } from '../../src/server/analyze-worker-ipc.js'; +import type { AnalyzeResult } from '../../src/core/run-analyze.js'; +import type { CompleteMessage } from '../../src/server/analyze-worker.js'; + +const REPO_NAME = 'collapse-fixture'; + +/** + * Build the exact `complete` message the worker puts on the wire, by running the + * production projection. The `graphWriteCollapsed` key is therefore whatever + * `analyze-worker-ipc.ts` actually sends — not a literal this test invented. + */ +const completeMessage = (graphWriteCollapsed?: { expected: number; persisted: number }) => { + const result = { + repoName: REPO_NAME, + repoPath: REPO_PATH, + stats: { files: 10, nodes: 100, edges: 500 }, + ...(graphWriteCollapsed ? { graphWriteCollapsed } : {}), + } satisfies Partial as AnalyzeResult; + return { type: 'complete', result: projectAnalyzeResultForIpc(result) } satisfies CompleteMessage; +}; + +interface FakeChild extends EventEmitter { + stderr: EventEmitter; + send: Mock<(msg: unknown) => boolean>; + kill: Mock<(signal?: NodeJS.Signals) => boolean>; +} + +const makeChild = (): FakeChild => { + const child = new EventEmitter() as FakeChild; + child.stderr = new EventEmitter(); + child.send = vi.fn(); + child.kill = vi.fn(); + return child; +}; + +describe('createLaunchAnalysisWorker — collapsed index is never published', () => { + let jobManager: JobManager; + let child: FakeChild; + let calls: string[]; + let backendInit: Mock<() => Promise>; + let closeDbHandle: Mock<() => Promise>; + + /** Drive one analyze to its terminal state and return the observed call order. */ + const runWorker = async (msg: CompleteMessage) => { + const launch = createLaunchAnalysisWorker({ + jobManager, + backend: { init: backendInit }, + acquireRepoLock: () => null, + releaseRepoLock: () => {}, + closeDbHandle, + }); + + const job = jobManager.createJob({ repoPath: REPO_PATH }); + launch(job, REPO_PATH, {}); + child.emit('message', msg); + + await vi.waitFor(() => expect(calls).toContain('updateJob:terminal')); + return jobManager.getJob(job.id); + }; + + beforeEach(() => { + calls = []; + jobManager = new JobManager(); + child = makeChild(); + forkMock.mockImplementation(() => child); + + backendInit = vi.fn(async () => { + calls.push('backend.init'); + return true; + }); + closeDbHandle = vi.fn(async () => { + calls.push('closeDbHandle'); + }); + + const realUpdate = jobManager.updateJob.bind(jobManager); + vi.spyOn(jobManager, 'updateJob').mockImplementation((id, update) => { + calls.push(`updateJob:${update.status ?? 'progress'}`); + realUpdate(id, update); + // Recorded after the real call so the marker only lands once the status is + // committed — `updateJob` drops any update to an already-terminal job. + calls.push( + ...['complete', 'failed'] + .filter((s) => s === update.status) + .map(() => 'updateJob:terminal'), + ); + }); + }); + + afterEach(() => { + jobManager.dispose(); + vi.restoreAllMocks(); + forkMock.mockReset(); + }); + + it('does not publish the index — backend.init() is never called for a collapsed run', async () => { + await runWorker(completeMessage({ expected: 500, persisted: 3 })); + + // The defect: init() resolved FIRST, so the incomplete graph was live and + // queryable by every MCP/API consumer before the job was marked failed. + expect(backendInit).not.toHaveBeenCalled(); + expect(calls).not.toContain('backend.init'); + // The cached handle is still evicted — the worker rewrote the DB files on + // disk, so a pre-rewrite handle is stale whatever the outcome was. Eviction + // is not publication. + expect(closeDbHandle).toHaveBeenCalledTimes(1); + expect(calls.indexOf('closeDbHandle')).toBeLessThan(calls.indexOf('updateJob:failed')); + }); + + it('marks the collapsed run failed and still reports repoName', async () => { + const job = await runWorker(completeMessage({ expected: 500, persisted: 3 })); + + expect(job?.status).toBe('failed'); + // The success path sets repoName; api.ts's repo-resolution wait matches jobs + // on it first. Dropping it here cost one of three match keys for no reason. + expect(job?.repoName).toBe(REPO_NAME); + expect(job?.error).toContain('INCOMPLETELY'); + expect(job?.error).toContain('3 of 500'); + // The failure is explicit about the index being unreachable, not merely stale. + expect(job?.error).toContain('NOT published'); + }); + + it('publishes and completes a healthy run, in that order', async () => { + const job = await runWorker(completeMessage()); + + expect(job?.status).toBe('complete'); + expect(job?.repoName).toBe(REPO_NAME); + expect(backendInit).toHaveBeenCalledTimes(1); + // Publish strictly BEFORE the terminal complete, so the repo really is + // queryable when the client receives the SSE complete event. + expect(calls).toEqual([ + 'updateJob:analyzing', + 'closeDbHandle', + 'backend.init', + 'updateJob:complete', + 'updateJob:terminal', + ]); + }); + + it('reads the collapse flag under the name analyze-worker-ipc.ts actually sends', async () => { + const wire = completeMessage({ expected: 500, persisted: 3 }); + + // Guards against a silent rename: the branch under test keys off this exact + // field, and the message was produced by the production projection. + expect(Object.keys(wire.result)).toContain('graphWriteCollapsed'); + expect(wire.result.graphWriteCollapsed).toEqual({ expected: 500, persisted: 3 }); + + // A projection that stopped carrying the field must not read as healthy. + const healthy = completeMessage(); + expect(healthy.result.graphWriteCollapsed).toBeUndefined(); + const job = await runWorker(healthy); + expect(job?.status).toBe('complete'); + }); +}); diff --git a/gitnexus/test/unit/dispatch-guard-routes.test.ts b/gitnexus/test/unit/dispatch-guard-routes.test.ts index 2195fd148..66bc6a0e8 100644 --- a/gitnexus/test/unit/dispatch-guard-routes.test.ts +++ b/gitnexus/test/unit/dispatch-guard-routes.test.ts @@ -238,6 +238,194 @@ describe('dispatch-guard route extraction', () => { paths(`function h(req) { if (!/^\\/api\\/runs\\/[^/]+$/.test(pathname)) { return 1 } }`), ).toEqual([]); }); + + it('reads a doubly-negated VERB, not just a doubly-negated path', () => { + // The parity rule was stated for `!` but the verb walk refused on the mere + // PRESENCE of one, so this lost a verb the source states outright. The + // path-position case above passed throughout and hid it. + expect( + extract( + `function h(req) { if (!!(req.method === 'GET') && pathname === '/api/dn') { return 1 } }`, + ), + ).toMatchObject([{ routePath: '/api/dn', httpMethod: 'GET' }]); + }); + }); + + // A ternary SELECTS between its arms, so a verb inside one is not reached + // merely because the whole condition is truthy. Reproduced against the + // extractor before fixing: the first case emitted `GET /api/i`, the one method + // that branch guarantees the request does NOT have — the same inversion `!` + // produced before it was handled, one level up. + // + // The last three cases were ALREADY correct and are here to pin them: refusing + // every ternary would fix the bug and silently drop three real verbs. + describe('ternary polarity', () => { + const guard = (cond: string, path = '/api/i') => + extract(`function h(req) { if (${cond} && pathname === '${path}') { return 1 } }`); + + it('drops the verb when a ternary INVERTS it', () => { + // `c ? false : true` is `!c`: the branch runs for every method except GET. + expect(guard(`(req.method === 'GET' ? false : true)`)).toMatchObject([ + { routePath: '/api/i', httpMethod: '' }, + ]); + }); + + it('keeps the verb when a ternary is the identity', () => { + // `c ? true : false` is `c`. Verb-less would be safe but wrong to settle for. + expect(guard(`(req.method === 'GET' ? true : false)`)).toMatchObject([ + { routePath: '/api/i', httpMethod: 'GET' }, + ]); + }); + + it('keeps a verb in the consequence when the alternative is `false`', () => { + // `c ? A : false` is `c && A` — reaching the body requires BOTH. + expect(guard(`(isAdmin ? req.method === 'GET' : false)`)).toMatchObject([ + { routePath: '/api/i', httpMethod: 'GET' }, + ]); + }); + + it('keeps a verb in the alternative when the consequence is `false`', () => { + // `c ? false : B` is `!c && B`. + expect(guard(`(isAdmin ? false : req.method === 'GET')`)).toMatchObject([ + { routePath: '/api/i', httpMethod: 'GET' }, + ]); + }); + + it('claims no verb when both arms are live comparisons', () => { + // Which verb this serves depends on `isAdmin`, so naming either is a guess. + expect(guard(`(isAdmin ? req.method === 'GET' : req.method === 'POST')`)).toMatchObject([ + { routePath: '/api/i', httpMethod: '' }, + ]); + }); + + it('claims no verb when a `true` arm makes the ternary a disjunction', () => { + // `c ? true : B` is `c || B`: the body is also reached for any method when + // `isAdmin` holds, so `GET` would present a broader route as a narrow one. + expect(guard(`(req.method === 'GET' ? true : isAdmin)`)).toMatchObject([ + { routePath: '/api/i', httpMethod: '' }, + ]); + }); + + it('claims no verb when the whole ternary is negated', () => { + // `!(c ? A : false)` is `!(c && A)`, i.e. `!c || !A` — a disjunction, which + // guarantees nothing. Without the parity guard the conjunction rule would + // read `GET` straight out of the consequence and invert it exactly as the + // un-negated form did. + expect(guard(`!(isAdmin ? req.method === 'GET' : false)`, '/api/n')).toMatchObject([ + { routePath: '/api/n', httpMethod: '' }, + ]); + }); + + // `c ? A : false` is `c && A`, and the methods a CONJUNCTION guarantees are + // the ones both sides admit — their intersection. Taking the first side that + // named a verb reported one operand's set unintersected, which is how a verb + // the guard excludes got minted as a route of its own. + it('INTERSECTS the two sides of the conjunction instead of taking the first', () => { + // {GET,POST} ∩ {POST,PUT} is POST alone. GET reaches the ternary but not + // its consequence, so `GET /api/t1` was a route no request can take. + expect( + guard( + `((req.method === 'GET' || req.method === 'POST') ? (req.method === 'POST' || req.method === 'PUT') : false)`, + '/api/t1', + ), + ).toMatchObject([{ routePath: '/api/t1', httpMethod: 'POST' }]); + }); + + it('claims no verb when the two sides cannot both hold', () => { + // `GET && POST` is unsatisfiable — no method satisfies this guard, so + // naming either side invents a route. Verb-less is the honest answer, and + // the path itself is still proven. + expect( + guard(`(req.method === 'GET' ? req.method === 'POST' : false)`, '/api/t2'), + ).toMatchObject([{ routePath: '/api/t2', httpMethod: '' }]); + }); + + it('intersects the other conjunction too, at flipped parity', () => { + // `c ? false : B` is `!c && B`. With `c` = `!(method === 'GET')` the guard + // reads `GET && POST` — the same contradiction, reached through the arm + // that searches the condition negated. + expect( + guard(`(!(req.method === 'GET') ? false : req.method === 'POST')`, '/api/t4'), + ).toMatchObject([{ routePath: '/api/t4', httpMethod: '' }]); + }); + + it('still reads a conjunction where only ONE side names a verb', () => { + // The fallthrough the intersection replaces stays right when a side is + // simply silent about the method: `isReady && POST` serves POST. An empty + // side means "names no method", not "admits none". + expect(guard(`(isReady ? req.method === 'POST' : false)`, '/api/t3')).toMatchObject([ + { routePath: '/api/t3', httpMethod: 'POST' }, + ]); + }); + }); + + // One guard, several methods. Verbatim from the reporting repo: + // if ((req.method === 'GET' || req.method === 'POST') && bundlesMatch) { … } + // Taking the FIRST verb reported this as GET-only, so `route_map` presented a + // route open to two methods as restricted to one, and `impact` on the POST + // path found nothing. + describe('multi-method guards', () => { + const guard = (cond: string) => + extract(`function h(req) { if (${cond} && pathname === '/api/i') { return 1 } }`).map( + (r) => r.httpMethod, + ); + + it('emits one route per method in a verb disjunction', () => { + expect(guard(`(req.method === 'GET' || req.method === 'POST')`)).toEqual(['GET', 'POST']); + }); + + it('handles more than two', () => { + expect( + guard(`(req.method === 'GET' || req.method === 'POST' || req.method === 'PUT')`), + ).toEqual(['GET', 'POST', 'PUT']); + }); + + it('claims NO verb when a disjunct is not a verb test', () => { + // `GET || isAdmin` is reached for ANY method when `isAdmin` holds. Naming + // GET would describe a route open to everything as single-method — the + // direction this module treats as more expensive than saying nothing. + expect(guard(`(req.method === 'GET' || isAdmin)`)).toEqual(['']); + }); + + it('claims no verb when the disjunction is negated', () => { + // `!(GET || POST)` excludes both rather than offering either. + expect(guard(`!(req.method === 'GET' || req.method === 'POST')`)).toEqual(['']); + }); + + it('still distributes ONE verb across an OR of paths', () => { + // The pre-existing rule, pinned against the disjunction change: here the + // `||` joins PATHS, not verbs, and must not start multiplying methods. + expect( + extract(` + function h(req) { + if (req.method === 'GET' && (pathname === '/api/a' || pathname === '/api/b')) { return 1 } + } + `), + ).toMatchObject([ + { routePath: '/api/a', httpMethod: 'GET' }, + { routePath: '/api/b', httpMethod: 'GET' }, + ]); + }); + + it('gives every switch arm the full method set', () => { + expect( + extract(` + function h(req) { + if (req.method === 'GET' || req.method === 'POST') { + switch (pathname) { + case '/api/a': return 1 + case '/api/b': return 2 + } + } + } + `), + ).toMatchObject([ + { routePath: '/api/a', httpMethod: 'GET' }, + { routePath: '/api/a', httpMethod: 'POST' }, + { routePath: '/api/b', httpMethod: 'GET' }, + { routePath: '/api/b', httpMethod: 'POST' }, + ]); + }); }); // Not in any report — the same dispatch written with different syntax. A @@ -436,6 +624,315 @@ describe('dispatch-guard route extraction', () => { it('ignores a regex tested against something that is not a request path', () => { expect(paths(`function f() { if (/^\\/api\\/x$/.test(filename)) { return 1 } }`)).toEqual([]); }); + + // The form every real dispatcher writes, and the one this converter refused. + // `(` fell through to the metacharacter bail, so the capturing pattern + // translated to nothing while its non-capturing twin translated fine — which + // is exactly why every test above passed. The reporting repo does not contain + // a single non-capturing path wildcard: a dispatcher captures the segment + // because it needs the id. + it('converts a CAPTURING single-segment wildcard', () => { + expect(regexToRoutePath('^\\/api\\/research-runs\\/([^/]+)$')).toBe( + '/api/research-runs/{param1}', + ); + }); + + it('converts a capturing wildcard followed by more literal path', () => { + expect(regexToRoutePath('^\\/api\\/live\\/positions\\/([^/]+)\\/replay$')).toBe( + '/api/live/positions/{param1}/replay', + ); + }); + + it('still refuses a capture group around anything that is not one segment', () => { + // `.+` spans slashes, so it is not a single segment and cannot be one + // `{param}`. Accepting `(` must not mean accepting every group. + expect(regexToRoutePath('^\\/api\\/x\\/(.+)$')).toBeNull(); + expect(regexToRoutePath('^\\/api\\/x\\/(a|b)$')).toBeNull(); + }); + + it('refuses an unbalanced capture group', () => { + // A stray `)` would otherwise be read as a literal path character. + expect(regexToRoutePath('^\\/api\\/x\\/([^/]+$')).toBeNull(); + }); + }); + + // `RE.test(pathname)` and `pathname.match(RE)` are the same test with the + // operands swapped. Only `.test` was read, which is why 28 of the reporting + // repo's 75 routes still named the shared route table as their handler: their + // modules dispatch with `.match`. + // + // `.match` differs in one way that matters — its result is USED, so it is + // almost always BOUND, and the verb then lives in a later `if` rather than + // around the call. + describe('bound .match() dispatch', () => { + const RUNS = `/^\\/api\\/research-runs\\/([^/]+)$/`; + + it('reads the verb from where the binding is TESTED, not where it is bound', () => { + expect( + extract(` + function handle(req) { + const runMatch = pathname.match(${RUNS}) + if (req.method === 'GET' && runMatch) { return runMatch[1] } + } + `), + ).toMatchObject([ + { routePath: '/api/research-runs/{param1}', httpMethod: 'GET', handlerName: 'handle' }, + ]); + }); + + it('emits a route per method for a multi-method bound match', () => { + // Verbatim shape from researchRunRoutes.js. + expect( + extract(` + function handle(req) { + const bundlesMatch = pathname.match(/^\\/api\\/runs\\/([^/]+)\\/bundles$/) + if ((req.method === 'GET' || req.method === 'POST') && bundlesMatch) { return 1 } + } + `).map((r) => r.httpMethod), + ).toEqual(['GET', 'POST']); + }); + + it('emits once per TEST SITE, not once per capture read', () => { + // `m[1]` is a read of the captured segment. It says nothing about + // dispatch, and counting it would mint a duplicate route per use of the id. + expect( + extract(` + function handle(req) { + const m = pathname.match(/^\\/api\\/w\\/([^/]+)$/) + if (req.method === 'GET' && m) { return [m[1], m[2], m[1]] } + } + `), + ).toMatchObject([{ routePath: '/api/w/{param1}', httpMethod: 'GET' }]); + }); + + it('emits a route per test site when one binding is tested for two methods', () => { + expect( + extract(` + function handle(req) { + const m = pathname.match(/^\\/api\\/t\\/([^/]+)$/) + if (req.method === 'GET' && m) { return 1 } + if (req.method === 'PUT' && m) { return 2 } + } + `).map((r) => r.httpMethod), + ).toEqual(['GET', 'PUT']); + }); + + it('does not inherit a verb across a NEGATED guard clause', () => { + // `if (!m) return` is the early-out. The `if (method === 'GET')` after it + // governs the rest of the function, not this binding's test. + expect( + extract(` + function handle(req) { + const m = pathname.match(/^\\/api\\/z\\/([^/]+)$/) + if (!m) { return false } + if (req.method === 'GET') { return 1 } + } + `), + ).toMatchObject([{ routePath: '/api/z/{param1}', httpMethod: '' }]); + }); + + it('keeps the path when a binding is never tested', () => { + // The code still computed an anchored match against the request path — + // the same evidence an unbound `.test` carries. + expect( + extract(` + function handle(req) { + const m = pathname.match(/^\\/api\\/y\\/([^/]+)$/) + return m[1] + } + `), + ).toMatchObject([{ routePath: '/api/y/{param1}', httpMethod: '' }]); + }); + + it('reads an UNBOUND .match like a .test', () => { + expect( + extract( + `function handle(req) { if (req.method === 'GET' && pathname.match(/^\\/api\\/x$/)) { return 1 } }`, + ), + ).toMatchObject([{ routePath: '/api/x', httpMethod: 'GET' }]); + }); + + it('resolves a regex named by a same-file const, both ways round', () => { + // positionReplayRoutes.js declares the pattern once and uses it both ways. + const re = `const RE = /^\\/api\\/positions\\/([^/]+)\\/replay$/`; + expect( + extract(` + ${re} + function handle(req) { + const routeMatch = pathname.match(RE) + if (req.method === 'DELETE' && routeMatch) { return 1 } + } + `), + ).toMatchObject([{ routePath: '/api/positions/{param1}/replay', httpMethod: 'DELETE' }]); + expect( + extract(` + ${re} + function handle(req) { if (req.method === 'GET' && RE.test(pathname)) { return 1 } } + `), + ).toMatchObject([{ routePath: '/api/positions/{param1}/replay', httpMethod: 'GET' }]); + }); + + it('refuses .match on a receiver that is not a request path', () => { + // The genuine route alongside it is load-bearing, NOT decoration: without + // a path token somewhere in the file, PATH_TOKEN_HINT skips the walk + // entirely and this assertion is satisfied by a file that was never + // examined. It proves the file WAS processed and `userAgent` was refused + // on its merits. + expect( + paths(` + function handle(req) { + const m = userAgent.match(/^\\/api\\/nope$/) + if (req.method === 'GET' && m) { return 1 } + if (req.method === 'GET' && pathname === '/api/real') { return 2 } + } + `), + ).toEqual(['/api/real']); + }); + + it('claims no verb when the test site itself sits under a negation', () => { + // `if (!(method === 'GET' && m))` runs precisely when the path did NOT + // match, so attributing GET is backwards. `!m` alone never reaches this + // check — a `unary_expression` parent is not a truthiness position to + // begin with — so the wrapped conjunction is the shape that exercises it. + expect( + extract(` + function handle(req) { + const m = pathname.match(/^\\/api\\/n\\/([^/]+)$/) + if (!(req.method === 'GET' && m)) { return false } + } + `), + ).toMatchObject([{ routePath: '/api/n/{param1}', httpMethod: '' }]); + }); + + it('refuses a regex const bound twice to different patterns', () => { + // Same ambiguity refusal the string-constant map applies: a half-right + // regex is a wrong route. + expect( + paths(` + const RE = /^\\/api\\/a\\/([^/]+)$/ + const RE = /^\\/api\\/b\\/([^/]+)$/ + function handle(req) { if (req.method === 'GET' && RE.test(pathname)) { return 1 } } + `), + ).toEqual([]); + }); + + // That refusal only ever compared regex LITERALS, so the two rebindings that + // actually occur walked straight past it and the literal's route was minted + // as though the name still held it. + it('refuses a regex const REASSIGNED to something dynamic', () => { + expect( + paths(` + let RE = /^\\/api\\/re\\/([^/]+)$/ + RE = buildDynamic(req) + function handle(req) { if (req.method === 'GET' && RE.test(pathname)) { return 1 } } + `), + ).toEqual([]); + }); + + it('refuses a regex const with a non-literal twin in another function', () => { + // The map is flat, so a same-named binding anywhere in the file is the + // ambiguity it claims to refuse — `new RegExp(userPrefix + '/x')` is not a + // `regex` node, which is the only reason it used to survive. + expect( + paths(` + const RE = /^\\/api\\/twin\\/([^/]+)$/ + function other(userPrefix) { const RE = new RegExp(userPrefix + '/x'); return RE } + function handle(req) { if (req.method === 'GET' && RE.test(pathname)) { return 1 } } + `), + ).toEqual([]); + }); + + // A match binding is keyed by the FUNCTION it is bound in, not by its bare + // name. `m`, `match` and `result` are the commonest locals in dispatcher + // code, so a file with two handlers routinely binds one of them twice to + // unrelated things — and the poison rule never fired, because it only ran + // when a second REGEX MATCH bound the name. + it('does not resolve a same-named local in ANOTHER function to this binding', () => { + // Measured before fixing: this emitted a second route, + // `DELETE /api/live/positions/{param1}/replay @9 handler=handleSettings` + // — wrong verb, wrong handler, wrong line, for a path that handler never + // serves. Being VERBED, it also outranked the real route in + // `reconcileDispatchGuardRoutes`, which drops a verb-less URL claimed with + // a verb anywhere in the repo. + expect( + extract(` + function handleReplay(req, res) { + const pathname = new URL(req.url, 'http://x').pathname + const m = pathname.match(/^\\/api\\/live\\/positions\\/([^/]+)\\/replay$/) + if (req.method === 'GET' && m) { return replay(m[1]) } + } + function handleSettings(req, res) { + const m = req.headers['x-mode'] + if (req.method === 'DELETE' && m) { return wipeEverything() } + } + `), + ).toEqual([ + { + routePath: '/api/live/positions/{param1}/replay', + httpMethod: 'GET', + handlerName: 'handleReplay', + source: DISPATCH_GUARD_SOURCE, + }, + ]); + }); + + it('keeps an untested binding verb-less when another function reuses the name', () => { + // The second loss channel of the same defect: `tested` was keyed by bare + // name too, so the unrelated `if (m)` below marked `m` tested and + // SUPPRESSED this binding's own verb-less emit. The honest route was not + // merely joined by a fabricated one — it was replaced by it, reporting + // `handleSettings` as the handler for a path only `handleReplay` serves. + expect( + extract(` + function handleReplay(req, res) { + const m = pathname.match(/^\\/api\\/live\\/positions\\/([^/]+)\\/replay$/) + return m[1] + } + function handleSettings(req, res) { + const m = req.headers['x-mode'] + if (m) { return wipeEverything() } + } + `), + ).toEqual([ + { + routePath: '/api/live/positions/{param1}/replay', + httpMethod: '', + handlerName: 'handleReplay', + source: DISPATCH_GUARD_SOURCE, + }, + ]); + }); + + it('refuses a name shadowed by a second declarator in the SAME function', () => { + // The key is the function, not the block, so a shadow inside one handler + // is ambiguity this walk cannot order — refused whole, the way + // `buildConstantMap` refuses a constant declared twice. It costs the real + // GET alongside the DELETE the shadow would have fabricated, which is the + // cheaper of the two failures. + expect( + extract(` + function handle(req) { + const m = pathname.match(/^\\/api\\/bs\\/([^/]+)$/) + if (req.method === 'GET' && m) { return 1 } + { const m = req.headers['x']; if (req.method === 'DELETE' && m) { return wipe() } } + } + `), + ).toEqual([]); + }); + + it('refuses a match binding REASSIGNED later in the same function', () => { + // `m` no longer holds the match by the time it is tested, so the + // declaration is not evidence of what the `if` asks about. + expect( + paths(` + function handle(req) { + let m = pathname.match(/^\\/api\\/ra\\/([^/]+)$/) + m = req.headers['x-mode'] + if (req.method === 'GET' && m) { return 1 } + } + `), + ).toEqual([]); + }); }); describe('handler attribution', () => { diff --git a/gitnexus/test/unit/fetch-site-capture.test.ts b/gitnexus/test/unit/fetch-site-capture.test.ts new file mode 100644 index 000000000..ee9187990 --- /dev/null +++ b/gitnexus/test/unit/fetch-site-capture.test.ts @@ -0,0 +1,101 @@ +/** + * `fetch()` call-SITE capture, independent of whether the URL is a literal + * (#2897). + * + * The rule required the argument to be a string or template literal, so + * `fetch(url)` — a variable — matched nothing. That made the R3-6 sink signal + * absent from almost every real call: measured across this repository's own + * TypeScript sources, 44 of 47 `fetch(` calls pass a variable, so 94% produced + * no site and sink-terminated flows could effectively never fire. + * + * The URL alternation is now optional. The R3-6 sink set needs only WHERE the + * program reaches outward, not where to; route linking still needs the URL and + * already skips an entry whose URL normalizes to nothing, so widening the + * capture adds sink sites without inventing a FETCHES edge. + */ +import { describe, expect, it } from 'vitest'; +import Parser from 'tree-sitter'; +import JavaScript from 'tree-sitter-javascript'; +import TypeScript from 'tree-sitter-typescript'; +import { + JAVASCRIPT_QUERIES, + TYPESCRIPT_QUERIES, +} from '../../src/core/ingestion/tree-sitter-queries.js'; + +interface Site { + readonly line: number; + readonly url: string | undefined; +} + +/** Every `route.fetch` site the query reports, with its URL when it has one. */ +function fetchSites(source: string, lang: 'js' | 'ts'): Site[] { + const parser = new Parser(); + const language = lang === 'js' ? JavaScript : TypeScript.typescript; + parser.setLanguage(language); + const query = new Parser.Query(language, lang === 'js' ? JAVASCRIPT_QUERIES : TYPESCRIPT_QUERIES); + + const byLine = new Map(); + for (const match of query.matches(parser.parse(source).rootNode)) { + const caps = Object.fromEntries(match.captures.map((c) => [c.name, c.node])); + const anchor = caps['route.fetch']; + if (anchor === undefined) continue; + const url = caps['route.url'] ?? caps['route.template_url']; + byLine.set(anchor.startPosition.row + 1, { + line: anchor.startPosition.row + 1, + url: url?.text, + }); + } + return [...byLine.values()].sort((a, b) => a.line - b.line); +} + +const SOURCE = [ + "async function literal() { return fetch('/api/literal') }", // 1 + 'async function variable(url) { return fetch(url) }', // 2 + 'async function template(id) { return fetch(`/api/${id}`) }', // 3 + "async function computed() { return fetch(buildUrl(), { method: 'POST' }) }", // 4 + "function notFetch() { return prefetch('/api/nope') }", // 5 +].join('\n'); + +describe.each([ + ['JavaScript', 'js' as const], + ['TypeScript', 'ts' as const], +])('fetch site capture — %s (#2897)', (_label, lang) => { + it('captures a call whose URL is a VARIABLE', () => { + // The regression case: this produced no site at all, so the function was + // never a sink and no flow through it could terminate there. + const variable = fetchSites(SOURCE, lang).find((s) => s.line === 2); + expect(variable).toBeDefined(); + expect(variable!.url).toBeUndefined(); + }); + + it('captures a call whose argument is a computed expression', () => { + const computed = fetchSites(SOURCE, lang).find((s) => s.line === 4); + expect(computed).toBeDefined(); + expect(computed!.url).toBeUndefined(); + }); + + it('still captures the literal URL, unchanged', () => { + // Asserted because route linking depends on it: widening the capture must + // not cost the URL where one exists. + const literal = fetchSites(SOURCE, lang).find((s) => s.line === 1); + expect(literal?.url).toBe('/api/literal'); + }); + + it('still captures a template URL, unchanged', () => { + const template = fetchSites(SOURCE, lang).find((s) => s.line === 3); + expect(template?.url).toContain('/api/'); + }); + + it('emits exactly ONE site per call', () => { + // An optional alternation must not make a literal call match twice — a + // duplicate would double-count the site and, for a literal, could mint two + // FETCHES edges. + expect(fetchSites(SOURCE, lang).map((s) => s.line)).toEqual([1, 2, 3, 4]); + }); + + it('does not capture a different function whose name merely ends in fetch', () => { + // `prefetch(...)` on line 5. The identifier equality is what keeps the + // widened rule from matching anything that is not a fetch. + expect(fetchSites(SOURCE, lang).map((s) => s.line)).not.toContain(5); + }); +}); diff --git a/gitnexus/test/unit/graph-collapse-wiring.test.ts b/gitnexus/test/unit/graph-collapse-wiring.test.ts index 0cc38986f..13c09605a 100644 --- a/gitnexus/test/unit/graph-collapse-wiring.test.ts +++ b/gitnexus/test/unit/graph-collapse-wiring.test.ts @@ -9,10 +9,35 @@ * pure helper had tests; nothing exercised the wiring at all. */ import { describe, it, expect } from 'vitest'; +import type { RelationshipType } from 'gitnexus-shared'; import { detectGraphWriteCollapse, GRAPH_WRITE_COLLAPSE_MIN_EDGES, } from '../../src/core/index-freshness.js'; +import { + computeExpectedStructuralRelationships, + countStructuralRelationships, + selectPersistedCollapseStamp, +} from '../../src/core/run-analyze.js'; +import type { KnowledgeGraph } from '../../src/core/graph/types.js'; + +/** + * An in-memory graph holding exactly this many relationships of each type. + * + * `computeExpectedStructuralRelationships` takes the GRAPH rather than a + * pre-selected number, so the heap-side term can only be exercised through + * something that iterates like one. Only `forEachRelationshipFields` is used — + * the same zero-allocation columnar scan production walks. + */ +const graphWith = ( + byType: Partial>, +): Pick => ({ + forEachRelationshipFields(fn) { + for (const [type, count] of Object.entries(byType)) { + for (let i = 0; i < (count ?? 0); i++) fn('src', 'dst', type as RelationshipType, 1); + } + }, +}); /** * The `expected` count as `run-analyze` computes it. Kept as a tiny local @@ -31,8 +56,9 @@ describe('graph-collapse wiring: the expected count (3a)', () => { // SURPLUS and the ratio passes trivially. const bare = 200; const streamed = 9800; - expect(detectGraphWriteCollapse(bare, 4000)).toBeUndefined(); + expect(detectGraphWriteCollapse(bare, 4000)).toEqual({ verdict: 'healthy' }); expect(detectGraphWriteCollapse(expectedRelationships(bare, streamed), 4000)).toEqual({ + verdict: 'collapsed', expected: 10000, persisted: 4000, }); @@ -49,23 +75,34 @@ describe('graph-collapse wiring: an unreadable count is not zero (3b)', () => { // exact call — produced a measured-looking 0 and certified a HEALTHY index as // a total collapse. it('says nothing when the edge count could not be taken', () => { - expect(detectGraphWriteCollapse(10000, undefined)).toBeUndefined(); + expect(detectGraphWriteCollapse(10000, undefined)).toEqual({ + verdict: 'unmeasurable', + reason: 'persisted-unreadable', + }); }); it('still reports a genuine zero that WAS measured', () => { - expect(detectGraphWriteCollapse(10000, 0)).toEqual({ expected: 10000, persisted: 0 }); + expect(detectGraphWriteCollapse(10000, 0)).toEqual({ + verdict: 'collapsed', + expected: 10000, + persisted: 0, + }); }); }); describe('graph-collapse wiring: total loss is never exempt (3c)', () => { it('reports a small repo that lost every edge', () => { const small = GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1; - expect(detectGraphWriteCollapse(small, 0)).toEqual({ expected: small, persisted: 0 }); + expect(detectGraphWriteCollapse(small, 0)).toEqual({ + verdict: 'collapsed', + expected: small, + persisted: 0, + }); }); it('keeps exempting a small repo that lost only some', () => { const small = GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1; - expect(detectGraphWriteCollapse(small, small - 1)).toBeUndefined(); + expect(detectGraphWriteCollapse(small, small - 1)).toEqual({ verdict: 'healthy' }); }); }); @@ -76,6 +113,266 @@ describe('graph-collapse wiring: incremental writes are not comparable (3a)', () // would be certified complete. `run-analyze` therefore skips the check // entirely on that path; this pins the arithmetic that makes skipping right. it('cannot see a real incremental loss through whole-scope counts', () => { - expect(detectGraphWriteCollapse(10000, 9800)).toBeUndefined(); + expect(detectGraphWriteCollapse(10000, 9800)).toEqual({ verdict: 'healthy' }); + }); +}); + +/** + * PDG ROWS MUST NOT INFLATE `expected` (#2899 regression). + * + * These import the REAL `computeExpectedStructuralRelationships` rather than the + * local mirror above — and that is the whole point of them. The mirror exists + * "because the production expression is inline in a 3000-line function", and a + * mirror cannot catch a term the original got wrong. It did not catch this. + * + * Measured on a real repo: in-memory 20,825 + streamed 179,676 (of which ~110k + * were PDG) gave `expected` 200,501 against a structural `persisted` of 64,764, + * so a complete index reported INCOMPLETE and exited non-zero — then the stamp + * forced a rebuild that did it again. + */ +describe('graph-collapse wiring: PDG rows are excluded from `expected`', () => { + /** The measured `--force` shape: 20,825 in the heap, all of it structural + * because the PDG layers went to the sink. */ + const forcedRunHeap = graphWith({ CALLS: 20_825 }); + + it('uses the sink STRUCTURAL subtotal, not its total-row size hint', () => { + // 179,676 streamed of which 69,771 were structural. + expect( + computeExpectedStructuralRelationships(forcedRunHeap, { + structuralRows: 69_771, + totalRows: 179_676, + }), + ).toBe(90_596); + }); + + it('does not report a collapse on a healthy --pdg run', () => { + const expected = computeExpectedStructuralRelationships(forcedRunHeap, { + structuralRows: 69_771, + totalRows: 179_676, + }); + // The structural rows actually readable back. Well above the ratio. + expect(detectGraphWriteCollapse(expected, 64_764)).toEqual({ verdict: 'healthy' }); + }); + + it('would have reported one against the unfiltered total — the bug', () => { + // Pinning the defect itself: feeding the total-row hint reproduces the + // false INCOMPLETE exactly, so the distinction cannot be quietly undone. + expect(detectGraphWriteCollapse(20_825 + 179_676, 64_764)).toEqual({ + verdict: 'collapsed', + expected: 200_501, + persisted: 64_764, + }); + }); + + it('still detects a REAL structural collapse', () => { + // The subtraction must not blind the check. + const expected = computeExpectedStructuralRelationships(forcedRunHeap, { + structuralRows: 69_771, + totalRows: 179_676, + }); + expect(detectGraphWriteCollapse(expected, 1_000)).toEqual({ + verdict: 'collapsed', + expected: 90_596, + persisted: 1_000, + }); + }); + + it('picks structuralRows over totalRows when they differ', () => { + // The field choice itself, which a numeric parameter left at an untestable + // call site — and choosing wrong there is the whole defect. + expect( + computeExpectedStructuralRelationships(graphWith({}), { structuralRows: 7, totalRows: 999 }), + ).toBe(7); + }); +}); + +/** + * THE NON-`--force` HALF OF THE SAME DEFECT. + * + * Excluding PDG from the STREAMED term fixed the `--force` configuration only. + * Streaming needs `force === true` on both sides — `resolveStreamGraphEmit` + * opens with `if (options.force !== true) return false`, `resolveStreamPdgEmit` + * requires it too — so a plain `gitnexus analyze --pdg` has NO sink, there is no + * manifest to subtract from, and `scope-resolution/pipeline/run.ts` writes the + * PDG layers straight into the ordinary graph (`input.pdgEmitSink ?? graph`). + * Verified by running `runScopeResolution({ pdg: true })` with no sink: the + * resulting `relationshipCount` is 1 and every row of it is `CFG`. + * + * A FIRST analyze has no `existingMeta`, so it is not incremental, so the + * collapse check runs on it — against structural-plus-PDG expected and + * structural-only persisted. The heap term is therefore counted the same + * type-aware way the sink counts its own subtotal, which makes both sides + * measure one population in EVERY configuration rather than only under + * `--force`. + */ +describe('graph-collapse wiring: PDG resident in the heap is excluded too (no --force)', () => { + it('counts only structural rows out of a PDG-inclusive in-memory graph', () => { + // The non-streaming shape: everything is in the heap, PDG included. + expect(countStructuralRelationships(graphWith({ CALLS: 60_000, CFG: 110_000 }))).toBe(60_000); + }); + + it('excludes every PDG edge type, not just CFG', () => { + expect( + countStructuralRelationships( + graphWith({ + CFG: 1, + REACHING_DEF: 2, + CDG: 3, + POST_DOMINATE: 4, + TAINTED: 5, + SANITIZES: 6, + CALLS: 7, + }), + ), + ).toBe(7); + }); + + it('keeps counting TAINT_PATH, which is structural despite being a --pdg product', () => { + // Deliberately NOT in PDG_EDGE_TYPES: a whole-program Function→Function edge + // that lives in the in-memory graph and is persisted by the normal emit, so + // it is counted on BOTH sides. Dropping it here would understate `expected`. + expect(countStructuralRelationships(graphWith({ TAINT_PATH: 3, CFG: 9 }))).toBe(3); + }); + + it('does not treat a PDG-inclusive heap count as a structural expectation', () => { + // The regression itself. 60,000 structural + 110,000 PDG resident, with no + // manifest because nothing streamed; 58,000 structural rows read back is a + // healthy write. Taking `relationshipCount` (170,000) makes 58,000 look like + // a 66% loss and fails a complete index on its very first `--pdg` run. + const expected = computeExpectedStructuralRelationships( + graphWith({ CALLS: 60_000, CFG: 110_000 }), + undefined, + ); + expect(expected).toBe(60_000); + expect(detectGraphWriteCollapse(expected, 58_000)).toEqual({ verdict: 'healthy' }); + // What the PDG-inclusive count would have produced, pinned so the term + // cannot be quietly restored. + expect(detectGraphWriteCollapse(170_000, 58_000)).toEqual({ + verdict: 'collapsed', + expected: 170_000, + persisted: 58_000, + }); + }); + + it('still detects a real collapse on a non-streaming --pdg run', () => { + // Excluding resident PDG must not blind the check: the PDG rows persisted + // fine and every structural edge is gone. + const expected = computeExpectedStructuralRelationships( + graphWith({ CALLS: 60_000, CFG: 110_000 }), + undefined, + ); + expect(detectGraphWriteCollapse(expected, 100)).toEqual({ + verdict: 'collapsed', + expected: 60_000, + persisted: 100, + }); + }); + + it('is unchanged on a run with no streaming and no PDG at all', () => { + // The plain incremental/default run: no manifest, no PDG rows, so the + // structural count is simply the whole graph — as it always was. + expect(computeExpectedStructuralRelationships(graphWith({ CALLS: 10_000 }), undefined)).toBe( + 10_000, + ); + }); + + it('reaches a NO-VERDICT, not a crash, on a graph it cannot scan', () => { + // Reading `relationshipCount` off a lightweight pipeline result yielded + // `undefined` and therefore a non-finite `expected`, which + // `detectGraphWriteCollapse` already documents as an expected input ("a + // graph implementation that reports no total, a lightweight pipeline + // result"). Scanning must degrade to the same no-verdict rather than + // throwing an analyze that was otherwise about to succeed. + const unscannable = {} as Partial>; + expect(countStructuralRelationships(unscannable)).toBeNaN(); + expect(countStructuralRelationships(undefined)).toBeNaN(); + const expected = computeExpectedStructuralRelationships(unscannable, undefined); + expect(expected).toBeNaN(); + expect(detectGraphWriteCollapse(expected, 5_000)).toEqual({ + verdict: 'unmeasurable', + reason: 'expected-unavailable', + }); + }); +}); + +/** + * THE STAMP TAXONOMY, which was documented three-way and implemented two-way. + * + * `selectPersistedCollapseStamp` decides what `saveMeta` writes, and `saveMeta` + * OVERWRITES rather than merges — so returning `undefined` deletes the stamp, + * and the stamp is what marks the index incomplete and forces the repairing + * rebuild. The shipped code split on `wroteChangedSubgraphOnly` (the write MODE) + * instead of on whether a verdict was reached, so a FULL run that could not + * measure took the "no collapse ⇒ clear it" branch. + */ +describe('graph-collapse wiring: the persisted stamp splits on the VERDICT', () => { + const previous = { expected: 23_009, persisted: 2_170 }; + + it('stamps a detected collapse', () => { + expect( + selectPersistedCollapseStamp( + { verdict: 'collapsed', expected: 10_000, persisted: 100 }, + undefined, + ), + ).toEqual({ expected: 10_000, persisted: 100 }); + }); + + it('overwrites an older stamp with the collapse this run measured', () => { + expect( + selectPersistedCollapseStamp( + { verdict: 'collapsed', expected: 10_000, persisted: 100 }, + previous, + ), + ).toEqual({ expected: 10_000, persisted: 100 }); + }); + + it('CLEARS the stamp on a measured healthy full run', () => { + expect(selectPersistedCollapseStamp({ verdict: 'healthy' }, previous)).toBeUndefined(); + }); + + it('CARRIES the stamp forward when the structural count could not be read', () => { + // The reported trigger: run 1 genuinely collapses and is stamped; run 2 is + // forced full by that stamp, but its `WHERE NOT r.type IN [...]` query + // throws on `withConnLock` contention with the WAL-checkpoint driver. The + // old code read that as "full run, no collapse" and deleted the stamp, so + // run 3 took `alreadyUpToDate`, printed "Already up to date" and exited 0 — + // forever, on an index still missing 91% of its edges. + expect( + selectPersistedCollapseStamp( + { verdict: 'unmeasurable', reason: 'persisted-unreadable' }, + previous, + ), + ).toEqual(previous); + }); + + it('carries it forward on an incremental write and on an unavailable expectation', () => { + expect( + selectPersistedCollapseStamp( + { verdict: 'unmeasurable', reason: 'incremental-write' }, + previous, + ), + ).toEqual(previous); + expect( + selectPersistedCollapseStamp( + { verdict: 'unmeasurable', reason: 'expected-unavailable' }, + previous, + ), + ).toEqual(previous); + }); + + it('invents nothing when there was no previous stamp', () => { + expect( + selectPersistedCollapseStamp( + { verdict: 'unmeasurable', reason: 'persisted-unreadable' }, + undefined, + ), + ).toBeUndefined(); + }); + + it('routes an unreadable persisted count to unmeasurable, not to a clear', () => { + // End to end through the predicate: this is the pairing that erased stamps. + const verdict = detectGraphWriteCollapse(10_000, undefined); + expect(verdict).toEqual({ verdict: 'unmeasurable', reason: 'persisted-unreadable' }); + expect(selectPersistedCollapseStamp(verdict, previous)).toEqual(previous); }); }); diff --git a/gitnexus/test/unit/incremental-parse-cache.test.ts b/gitnexus/test/unit/incremental-parse-cache.test.ts index 91b2f29d2..aaf7d4d1c 100644 --- a/gitnexus/test/unit/incremental-parse-cache.test.ts +++ b/gitnexus/test/unit/incremental-parse-cache.test.ts @@ -176,8 +176,27 @@ describe('PARSE_CACHE_VERSION', () => { // does do is fail loudly the moment the constant and this expectation drift // apart, which is what forces the merge-time diff against origin/main to // happen at all. - it('pins SCHEMA_BUMP to 53 so concurrent bumps cannot silently collide (#2766)', () => { - expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(53); + // Moved 53 -> 54 for W2-8: type parameters are captured on generic functions + // and aliases, not just class-likes, so the shadowing guard has data to read. + // Moved 54 -> 55 for W2-9: the dispatch-guard verb walk tracks boolean polarity, + // so a ternary can no longer report the verb it excludes. Routes are emitted at + // parse time, so a warm cache would replay the inverted verb indefinitely. + // Moved 55 -> 56 for R3-8 part 1: the verb walk returns every method a guard + // serves, so a multi-method guard emits several routes where it emitted one. + // Moved 56 -> 57 for R3-8 part 2: `.match()` dispatch, bound-match test sites, + // named regex consts, and capturing segment wildcards in `regexToRoutePath`. + // Moved 57 -> 58 for #2897: fetch sites are captured without a literal URL. + // Moved 58 -> 59 for the #2899 review follow-up: the dispatch-guard walk keys + // match bindings on (enclosing function, name) instead of the bare identifier, + // and a ternary conjunction INTERSECTS its operands instead of taking the first + // non-empty set. Both strictly remove routes, so a warm cache would keep + // serving a fabricated verbed route that evicts the true one. + it('pins SCHEMA_BUMP to 59 so concurrent bumps cannot silently collide (#2766)', () => { + expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(59); + // The PREVIOUS version must fail the reuse gate, not merely differ from the + // current one — a hardcoded number outside the conflict hunk rebases cleanly + // while being wrong, which is exactly how the 37/38 exact clashes landed. + expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).not.toBe(58); }); it('embeds the gitnexus package version (so upgrades invalidate the cache)', () => { diff --git a/gitnexus/test/unit/index-freshness-graph-collapse.test.ts b/gitnexus/test/unit/index-freshness-graph-collapse.test.ts index bc610c3c3..f92a8e375 100644 --- a/gitnexus/test/unit/index-freshness-graph-collapse.test.ts +++ b/gitnexus/test/unit/index-freshness-graph-collapse.test.ts @@ -24,20 +24,58 @@ import { describe('detectGraphWriteCollapse (B2 detection)', () => { it('flags the reported field failure (23009 built, 2170 persisted)', () => { - expect(detectGraphWriteCollapse(23009, 2170)).toEqual({ expected: 23009, persisted: 2170 }); + expect(detectGraphWriteCollapse(23009, 2170)).toEqual({ + verdict: 'collapsed', + expected: 23009, + persisted: 2170, + }); }); it('flags a missing relation table, which reads back as zero persisted', () => { - expect(detectGraphWriteCollapse(23009, 0)).toEqual({ expected: 23009, persisted: 0 }); + expect(detectGraphWriteCollapse(23009, 0)).toEqual({ + verdict: 'collapsed', + expected: 23009, + persisted: 0, + }); }); it('stays silent on a healthy write', () => { - expect(detectGraphWriteCollapse(23009, 23009)).toBeUndefined(); + expect(detectGraphWriteCollapse(23009, 23009)).toEqual({ verdict: 'healthy' }); }); - it('stays silent when MORE rows persist than the call graph built (--pdg)', () => { - // PDG layers write into the same table, so persisted > expected is normal. - expect(detectGraphWriteCollapse(1000, 4000)).toBeUndefined(); + // A surplus is still tolerated — the detector only ever fires on a SHORTFALL, + // and small overcounts are legitimate (a row written by a path the manifest + // does not enumerate). What changed is the caller, not this rule. + it('stays silent when more rows persist than expected', () => { + expect(detectGraphWriteCollapse(1000, 4000)).toEqual({ verdict: 'healthy' }); + }); + + // THE CASE THIS FILE USED TO PIN THE WRONG WAY, and why the fix is at the + // CALLER rather than here. + // + // The old assertion read `detectGraphWriteCollapse(1000, 4000)` with the + // comment "PDG layers write into the same table, so persisted > expected is + // normal". True about the table — and it quietly licensed the masking. The + // caller passed `stats.edges`, a count of EVERY CodeRelation row, against an + // expectation covering only the structural halves, so losing all 1,000 + // structural edges while 4,000 PDG rows persisted was indistinguishable from + // health. + // + // Padding `expected` with the PDG rows does NOT fix that, which is worth + // recording because it is the obvious move: 4,000 persisted against 5,000 + // expected still clears the 0.5 ratio. The ratio would be judging a minority + // population. `run-analyze.ts` therefore compares STRUCTURAL against + // STRUCTURAL, using the new `getLbugStats().structuralEdges`. + // + // At this level that is simply the ordinary shortfall case: once both sides + // count structural edges only, a total structural wipeout on a --pdg run is + // `(1000, 0)` and fires like any other. + it('fires on a total structural loss even when PDG rows are plentiful', () => { + expect(detectGraphWriteCollapse(1000, 0)).toEqual({ + verdict: 'collapsed', + expected: 1000, + persisted: 0, + }); }); // REGRESSION. A non-numeric `expected` does not merely skip the guards, it @@ -46,31 +84,43 @@ describe('detectGraphWriteCollapse (B2 detection)', () => { // check "passes" too. Shipped briefly and reported healthy runs as total // collapses — the exact false certainty this check exists to prevent. it('never fires when the expected count is not a number', () => { - expect(detectGraphWriteCollapse(undefined as unknown as number, 0)).toBeUndefined(); - expect(detectGraphWriteCollapse(NaN, 0)).toBeUndefined(); - expect(detectGraphWriteCollapse(Infinity, 0)).toBeUndefined(); + const unmeasurable = { verdict: 'unmeasurable', reason: 'expected-unavailable' }; + expect(detectGraphWriteCollapse(undefined as unknown as number, 0)).toEqual(unmeasurable); + expect(detectGraphWriteCollapse(NaN, 0)).toEqual(unmeasurable); + expect(detectGraphWriteCollapse(Infinity, 0)).toEqual(unmeasurable); }); it('never fires when the persisted count is not a number', () => { // `getLbugStats` returns `{}` under some mocks/degraded paths, so // `stats.edges` arrives as undefined rather than a measured zero. - expect(detectGraphWriteCollapse(23009, undefined)).toBeUndefined(); - expect(detectGraphWriteCollapse(23009, NaN)).toBeUndefined(); + const unmeasurable = { verdict: 'unmeasurable', reason: 'persisted-unreadable' }; + expect(detectGraphWriteCollapse(23009, undefined)).toEqual(unmeasurable); + expect(detectGraphWriteCollapse(23009, NaN)).toEqual(unmeasurable); }); it('is fail-safe when the expected count is unavailable', () => { // An implementation that offloads relationships out of memory may report 0; // a false "your index is broken" is worse than a missed one. - expect(detectGraphWriteCollapse(0, 0)).toBeUndefined(); - expect(detectGraphWriteCollapse(0, 5000)).toBeUndefined(); + // + // `'unmeasurable'`, deliberately NOT `'healthy'`: a run that compared + // nothing has repaired nothing, so it must not be allowed to clear a stamp + // recording an earlier, real collapse. + const unmeasurable = { verdict: 'unmeasurable', reason: 'expected-unavailable' }; + expect(detectGraphWriteCollapse(0, 0)).toEqual(unmeasurable); + expect(detectGraphWriteCollapse(0, 5000)).toEqual(unmeasurable); }); it('exempts small repos where the ratio is meaningless', () => { // A PARTIAL shortfall under the threshold — the case the exemption was // written for ("a handful of edges lost to legitimate filtering"). + // + // `'healthy'` rather than `'unmeasurable'`: both counts WERE taken and the + // comparison did run, so a stamp may be cleared here. Calling the exemption + // a non-verdict would make the stamp unclearable on any repo that shrank + // below the threshold — a permanent forced-rebuild wedge. const justUnder = GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1; - expect(detectGraphWriteCollapse(justUnder, justUnder - 1)).toBeUndefined(); - expect(detectGraphWriteCollapse(justUnder, 1)).toBeUndefined(); + expect(detectGraphWriteCollapse(justUnder, justUnder - 1)).toEqual({ verdict: 'healthy' }); + expect(detectGraphWriteCollapse(justUnder, 1)).toEqual({ verdict: 'healthy' }); }); // This assertion previously read `detectGraphWriteCollapse(99, 0) === undefined`, @@ -80,28 +130,38 @@ describe('detectGraphWriteCollapse (B2 detection)', () => { // reported success. Losing all of a small graph is still losing all of it. it('never exempts a TOTAL loss, however small the repo', () => { expect(detectGraphWriteCollapse(GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1, 0)).toEqual({ + verdict: 'collapsed', expected: GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1, persisted: 0, }); - expect(detectGraphWriteCollapse(1, 0)).toEqual({ expected: 1, persisted: 0 }); + expect(detectGraphWriteCollapse(1, 0)).toEqual({ + verdict: 'collapsed', + expected: 1, + persisted: 0, + }); }); // The boundary the total-loss rule must NOT cross: zero expected is the // fail-safe "cannot measure" case, not a collapse. it('still says nothing when nothing was expected', () => { - expect(detectGraphWriteCollapse(0, 0)).toBeUndefined(); + expect(detectGraphWriteCollapse(0, 0)).toEqual({ + verdict: 'unmeasurable', + reason: 'expected-unavailable', + }); }); // An unreadable edge count is not a measured zero. `getLbugStats` now returns // `undefined` when the query threw, and the total-loss rule must not treat // that as a total loss. it('does not call an unreadable count a total loss', () => { - expect(detectGraphWriteCollapse(50, undefined)).toBeUndefined(); - expect(detectGraphWriteCollapse(5000, undefined)).toBeUndefined(); + const unmeasurable = { verdict: 'unmeasurable', reason: 'persisted-unreadable' }; + expect(detectGraphWriteCollapse(50, undefined)).toEqual(unmeasurable); + expect(detectGraphWriteCollapse(5000, undefined)).toEqual(unmeasurable); }); it('applies exactly at the minimum-edge boundary', () => { expect(detectGraphWriteCollapse(GRAPH_WRITE_COLLAPSE_MIN_EDGES, 0)).toEqual({ + verdict: 'collapsed', expected: GRAPH_WRITE_COLLAPSE_MIN_EDGES, persisted: 0, }); @@ -110,8 +170,34 @@ describe('detectGraphWriteCollapse (B2 detection)', () => { it('treats the ratio as inclusive — exactly at threshold is not a collapse', () => { const expected = 1000; const atThreshold = expected * GRAPH_WRITE_COLLAPSE_RATIO; - expect(detectGraphWriteCollapse(expected, atThreshold)).toBeUndefined(); - expect(detectGraphWriteCollapse(expected, atThreshold - 1)).toBeDefined(); + expect(detectGraphWriteCollapse(expected, atThreshold)).toEqual({ verdict: 'healthy' }); + expect(detectGraphWriteCollapse(expected, atThreshold - 1)).toEqual({ + verdict: 'collapsed', + expected, + persisted: atThreshold - 1, + }); + }); + + // Every verdict must be one of the three tags — an outcome that is neither a + // measured collapse, a measured all-clear, nor an explicit non-verdict is how + // "could not measure" got to look like "measured fine" in the first place. + it('never returns an untagged or absent verdict', () => { + const inputs: [number, number | undefined][] = [ + [23009, 2170], + [23009, 23009], + [1000, 4000], + [0, 0], + [0, 5000], + [99, 0], + [99, 98], + [100, 0], + [10000, undefined], + [NaN, 0], + ]; + for (const [expected, persisted] of inputs) { + const verdict = detectGraphWriteCollapse(expected, persisted); + expect(['collapsed', 'healthy', 'unmeasurable']).toContain(verdict.verdict); + } }); }); diff --git a/gitnexus/test/unit/lbug/graph-emit-sink.test.ts b/gitnexus/test/unit/lbug/graph-emit-sink.test.ts index a2daf2cff..bf3054016 100644 --- a/gitnexus/test/unit/lbug/graph-emit-sink.test.ts +++ b/gitnexus/test/unit/lbug/graph-emit-sink.test.ts @@ -424,3 +424,54 @@ describe('field scan matches the object scan', () => { sink.finalize(); }); }); + +/** + * STRUCTURAL SUBTOTAL (#2899 regression). + * + * `totalRows` is a buffer-pool size hint and counts every streamed row. The + * graph-write-collapse check reused it as its expectation while measuring + * STRUCTURAL rows on the other side — and PDG edges stream through this very + * sink, so on a `--pdg` run it compared ~200k against ~65k and declared a + * complete index INCOMPLETE. The stamp then forced a rebuild next run, which + * repeated it. + * + * A pair key cannot separate the two: it is `From|To` NODE LABELS, and a `CFG` + * edge shares `Function|Function` with `CALLS`. Only this write path sees + * `relationship.type`, so the split has to be counted here. + */ +describe('GraphEmitSink structural subtotal', () => { + it('counts a structural row in BOTH totals', () => { + const sink = new GraphEmitSink(createKnowledgeGraph(), csvDir); + sink.beginStreaming(); + sink.addRelationship(rel('CALLS', 'a', 'b')); + expect(sink.finalize()).toMatchObject({ totalRows: 1, structuralRows: 1 }); + }); + + it('excludes a PDG row from structuralRows but not from totalRows', () => { + // `totalRows` must keep counting it — it still sizes the buffer pool. + const sink = new GraphEmitSink(createKnowledgeGraph(), csvDir); + sink.beginStreaming(); + sink.addRelationship(rel('CFG', 'a', 'b')); + expect(sink.finalize()).toMatchObject({ totalRows: 1, structuralRows: 0 }); + }); + + it('splits a MIXED stream, which is the shape a --pdg run produces', () => { + const sink = new GraphEmitSink(createKnowledgeGraph(), csvDir); + sink.beginStreaming(); + sink.addRelationship(rel('CALLS', 'a', 'b')); + sink.addRelationship(rel('CFG', 'a', 'b')); + sink.addRelationship(rel('REACHING_DEF', 'a', 'b')); + sink.addRelationship(rel('CALLS', 'b', 'c')); + expect(sink.finalize()).toMatchObject({ totalRows: 4, structuralRows: 2 }); + }); + + it('counts TAINT_PATH as structural', () => { + // Deliberately NOT in PDG_EDGE_TYPES: a whole-program Function->Function + // edge persisted by the normal emit, so it is structural and must stay + // counted on both sides of the collapse check. + const sink = new GraphEmitSink(createKnowledgeGraph(), csvDir); + sink.beginStreaming(); + sink.addRelationship(rel('TAINT_PATH', 'a', 'b')); + expect(sink.finalize()).toMatchObject({ totalRows: 1, structuralRows: 1 }); + }); +}); diff --git a/gitnexus/test/unit/process-processor.test.ts b/gitnexus/test/unit/process-processor.test.ts index 6265a99db..69785034c 100644 --- a/gitnexus/test/unit/process-processor.test.ts +++ b/gitnexus/test/unit/process-processor.test.ts @@ -3,6 +3,7 @@ import { processProcesses, traceFromEntryPoint, buildSinkFunctionSet, + deduplicateTraces, type ProcessDetectionConfig, } from '../../src/core/ingestion/process-processor.js'; import { computeDynamicMaxProcesses } from '../../src/core/ingestion/pipeline-phases/processes.js'; @@ -824,3 +825,589 @@ describe('process selection diversity (R2-3)', () => { expect(result.processes.map((p) => p.terminalId)).toContain('func:ownTerminal'); }); }); + +// ============================================================================ +// DETERMINISM (W2-5) +// ============================================================================ +// +// The persisted graph must not depend on the order nodes and edges happened to +// be inserted. Four sorts in this file ranked by score or length alone and +// returned 0 on a tie; `Array.prototype.sort` is stable, so a 0 preserves INPUT +// order, which traces back to `graph.iterNodes()` — i.e. to the order the +// filesystem enumerated files. Under `maxProcesses` capping that decided which +// `Process` and `STEP_IN_PROCESS` nodes were persisted at all. +// +// Reproduced before the fix: two equal three-step flows with `maxProcesses: 1` +// selected `handleAlpha`; inserting the identical nodes and CALLS edges in +// reverse selected `handleBeta`. Same repository, same commit, different graph. +// +// This asserts the INVARIANT rather than any one sort, so it covers all four +// sites — and any future one — without needing to know where they are. +describe('process detection is insertion-order invariant (W2-5)', () => { + const buildGraph = (reverse: boolean) => { + const graph = createKnowledgeGraph(); + const memberships: CommunityMembership[] = []; + const chains = [ + ['handleAlpha', 'midAlpha', 'endAlpha'], + ['handleBeta', 'midBeta', 'endBeta'], + ['handleGamma', 'midGamma', 'endGamma'], + ]; + const ordered = reverse ? [...chains].reverse() : chains; + for (const chain of ordered) { + for (const name of chain) { + graph.addNode({ + id: `func:${name}`, + label: 'Function', + properties: { + name, + filePath: `src/${name}.ts`, + startLine: 1, + endLine: 10, + isExported: true, + }, + }); + memberships.push({ nodeId: `func:${name}`, communityId: 'community:0' }); + } + } + for (const chain of ordered) { + for (let i = 0; i < chain.length - 1; i++) { + graph.addRelationship({ + id: `call:${chain[i]}`, + sourceId: `func:${chain[i]}`, + targetId: `func:${chain[i + 1]}`, + type: 'CALLS', + confidence: 0.9, + reason: 'import-resolved', + }); + } + } + return { graph, memberships }; + }; + + it('selects the same process under a cap regardless of insertion order', async () => { + // The capped case is the one that mattered: with room for everything the + // set is equal either way and only the ORDER differs, so a cap is what turns + // an ordering difference into a persistence difference. + const forward = buildGraph(false); + const reversed = buildGraph(true); + const a = await processProcesses(forward.graph, forward.memberships, undefined, { + maxProcesses: 1, + }); + const b = await processProcesses(reversed.graph, reversed.memberships, undefined, { + maxProcesses: 1, + }); + expect(a.processes.length).toBe(1); + expect(a.processes[0]?.entryPointId).toBe(b.processes[0]?.entryPointId); + }); + + it('produces an identical process set uncapped', async () => { + const forward = buildGraph(false); + const reversed = buildGraph(true); + const a = await processProcesses(forward.graph, forward.memberships); + const b = await processProcesses(reversed.graph, reversed.memberships); + const shape = (r: Awaited>): string[] => + r.processes.map((p) => `${p.entryPointId}->${p.terminalId}`).sort(); + expect(shape(a).length).toBeGreaterThan(0); + expect(shape(a)).toEqual(shape(b)); + }); + + // The TRACE-RANK tie specifically. The chains above differ by entry point, so + // they are separated by the entry-point sort before trace ranking is reached — + // which means they do NOT exercise `rankedByInterest`'s tiebreak, verified by + // mutation. This fixture gives ONE entry point two equal-length branches to + // different terminals, so the only thing that can order them is the trace + // comparator itself. + const buildBranchedGraph = (reverse: boolean) => { + const graph = createKnowledgeGraph(); + const memberships: CommunityMembership[] = []; + const branches = [ + ['midAlpha', 'endAlpha'], + ['midBeta', 'endBeta'], + ]; + const ordered = reverse ? [...branches].reverse() : branches; + const add = (name: string, isExported: boolean) => { + graph.addNode({ + id: `func:${name}`, + label: 'Function', + properties: { name, filePath: `src/${name}.ts`, startLine: 1, endLine: 10, isExported }, + }); + memberships.push({ nodeId: `func:${name}`, communityId: 'community:0' }); + }; + add('handleShared', true); + for (const branch of ordered) for (const name of branch) add(name, true); + for (const branch of ordered) { + graph.addRelationship({ + id: `call:root:${branch[0]}`, + sourceId: 'func:handleShared', + targetId: `func:${branch[0]}`, + type: 'CALLS', + confidence: 0.9, + reason: 'import-resolved', + }); + graph.addRelationship({ + id: `call:${branch[0]}`, + sourceId: `func:${branch[0]}`, + targetId: `func:${branch[1]}`, + type: 'CALLS', + confidence: 0.9, + reason: 'import-resolved', + }); + } + return { graph, memberships }; + }; + + it('orders two equal-length traces from ONE entry point deterministically', async () => { + const forward = buildBranchedGraph(false); + const reversed = buildBranchedGraph(true); + const a = await processProcesses(forward.graph, forward.memberships, undefined, { + maxProcesses: 1, + }); + const b = await processProcesses(reversed.graph, reversed.memberships, undefined, { + maxProcesses: 1, + }); + expect(a.processes.length).toBe(1); + expect(a.processes[0]?.terminalId).toBe(b.processes[0]?.terminalId); + }); + + it('emits the traces in the same ORDER, not merely the same set', async () => { + // Order is what the cap consumes, so a set-only assertion would pass while + // the defect persisted. + const forward = buildGraph(false); + const reversed = buildGraph(true); + const a = await processProcesses(forward.graph, forward.memberships); + const b = await processProcesses(reversed.graph, reversed.memberships); + expect(a.processes.map((p) => p.entryPointId)).toEqual(b.processes.map((p) => p.entryPointId)); + }); +}); + +// W2-3. Every ceiling in this file used to fire silently: the result came back +// looking whole and no consumer could tell it was partial. The code's own +// comment said as much ("a silently truncating cap reads as 'this is +// everything'") and then only logged at debug — a log nobody has enabled is not +// a disclosure. Each counter below is asserted against a graph built to trip +// exactly one ceiling. +describe('truncation is reported, not swallowed (W2-3)', () => { + const addFn = (graph: ReturnType, id: string): void => { + graph.addNode({ + id, + label: 'Function', + properties: { name: id.split(':')[1], filePath: 'src/a.ts', startLine: 1, endLine: 2 }, + }); + }; + const addCall = ( + graph: ReturnType, + from: string, + to: string, + ): void => { + graph.addRelationship({ + id: `rel:${from}->${to}`, + sourceId: from, + targetId: to, + type: 'CALLS', + confidence: 1, + reason: 'test', + }); + }; + + /** A chain of `len` functions, prefixed so several can coexist in one graph. */ + const addChain = ( + graph: ReturnType, + prefix: string, + len: number, + ): void => { + for (let i = 0; i < len; i++) addFn(graph, `func:${prefix}${i}`); + for (let i = 0; i < len - 1; i++) + addCall(graph, `func:${prefix}${i}`, `func:${prefix}${i + 1}`); + }; + + it('reports nothing truncated when every flow fits', async () => { + // Asserted FIRST: every positive assertion below is meaningless if the flag + // is simply always true. + const graph = createKnowledgeGraph(); + addChain(graph, 'a', 3); + const result = await processProcesses(graph, [], undefined, { + maxTraceDepth: 10, + maxBranching: 4, + maxProcesses: 50, + }); + expect(result.processes.length).toBeGreaterThan(0); + expect(result.stats.truncation.truncated).toBe(false); + expect(result.stats.truncation).toMatchObject({ + entryPointsUnexplored: 0, + walksCutByBudget: 0, + tracesDepthCapped: 0, + calleesDropped: 0, + processesDropped: 0, + }); + }); + + it('counts entry points that were never traced at all', async () => { + // The trace loop stops on the TRACE quota (maxProcesses * 2), so the + // remaining entry points are not "no flows found" — nothing looked at them. + const graph = createKnowledgeGraph(); + for (let e = 0; e < 8; e++) addChain(graph, `e${e}_`, 3); + const result = await processProcesses(graph, [], undefined, { maxProcesses: 1 }); + const { entryPointsFound } = result.stats; + const { entryPointsUnexplored } = result.stats.truncation; + expect(entryPointsFound).toBeGreaterThan(0); + // Strictly between: some WERE traced, so this is a real early exit rather + // than "the loop never ran", and strictly less than the total, so the + // counter is not just echoing `entryPointsFound` back. + expect(entryPointsUnexplored).toBeGreaterThan(0); + expect(entryPointsUnexplored).toBeLessThan(entryPointsFound); + expect(result.stats.truncation.truncated).toBe(true); + }); + + it('counts traces that stop at maxTraceDepth rather than at a terminal', async () => { + // The trace is KEPT, but it is a prefix of a longer flow, and only this + // counter tells the two apart downstream. + const graph = createKnowledgeGraph(); + addChain(graph, 'deep', 12); + const result = await processProcesses(graph, [], undefined, { maxTraceDepth: 4 }); + expect(result.stats.truncation.tracesDepthCapped).toBeGreaterThan(0); + expect(result.stats.truncation.truncated).toBe(true); + }); + + it('counts callees never followed because of maxBranching', async () => { + const graph = createKnowledgeGraph(); + addFn(graph, 'func:fanout'); + for (let c = 0; c < 9; c++) { + addChain(graph, `leaf${c}_`, 2); + addCall(graph, 'func:fanout', `func:leaf${c}_0`); + } + const result = await processProcesses(graph, [], undefined, { maxBranching: 2 }); + expect(result.stats.truncation.calleesDropped).toBeGreaterThan(0); + expect(result.stats.truncation.truncated).toBe(true); + }); + + it('counts entry-point walks abandoned with branches still on the stack', async () => { + // Per-entry-point trace budget is `maxBranching * 3`, so a tree that is + // wide enough exhausts it with unexplored branches left. Every node here + // has EXACTLY `maxBranching` callees, which keeps `calleesDropped` at zero + // so this asserts its own counter and not a neighbour's. + const graph = createKnowledgeGraph(); + addFn(graph, 'func:root'); + for (let a = 0; a < 4; a++) { + addFn(graph, `func:mid${a}`); + addCall(graph, 'func:root', `func:mid${a}`); + for (let b = 0; b < 4; b++) { + addFn(graph, `func:leaf${a}_${b}`); + addCall(graph, `func:mid${a}`, `func:leaf${a}_${b}`); + } + } + const result = await processProcesses(graph, [], undefined, { maxBranching: 4 }); + expect(result.stats.truncation.walksCutByBudget).toBeGreaterThan(0); + expect(result.stats.truncation.calleesDropped).toBe(0); + expect(result.stats.truncation.truncated).toBe(true); + }); + + it('counts deduplicated traces dropped by the maxProcesses cap', async () => { + // Counted against the DEDUPED population: the gap between raw traces and + // deduped ones is deduplication working, which is not truncation. + const graph = createKnowledgeGraph(); + for (let e = 0; e < 6; e++) addChain(graph, `p${e}_`, 3); + const result = await processProcesses(graph, [], undefined, { maxProcesses: 2 }); + expect(result.processes.length).toBeLessThanOrEqual(2); + expect(result.stats.truncation.processesDropped).toBeGreaterThan(0); + expect(result.stats.truncation.truncated).toBe(true); + }); + + it('leaves the four pre-existing stats untouched', async () => { + // The field is ADDITIVE. A consumer reading totalProcesses must not have to + // learn about truncation to keep working. + const graph = createKnowledgeGraph(); + addChain(graph, 'x', 3); + const result = await processProcesses(graph, []); + expect(result.stats).toMatchObject({ + totalProcesses: expect.any(Number), + crossCommunityCount: expect.any(Number), + avgStepCount: expect.any(Number), + entryPointsFound: expect.any(Number), + }); + }); +}); + +// The ceiling the first pass of W2-3 MISSED. `findEntryPoints` ranks every +// scoring candidate and then keeps the top 200, so `entryPointsUnexplored` — +// computed over the list it RETURNS — can only ever see the survivors, and the +// cap that decides how much of a repository is looked at at all reported +// nothing. On anything above 200 candidates it is the DOMINANT ceiling. +describe('the entry-point candidate cap is disclosed too', () => { + const addFn = (graph: ReturnType, id: string): void => { + graph.addNode({ + id, + label: 'Function', + properties: { name: id.split(':')[1], filePath: 'src/a.ts', startLine: 1, endLine: 2 }, + }); + }; + const addCall = ( + graph: ReturnType, + from: string, + to: string, + ): void => { + graph.addRelationship({ + id: `rel:${from}->${to}`, + sourceId: from, + targetId: to, + type: 'CALLS', + confidence: 1, + reason: 'test', + }); + }; + + /** + * 205 three-node chains. Every node with at least one callee scores above + * zero, so this is 410 candidates for 200 slots — and NOTHING else is + * truncated: the chains are three long (under `maxTraceDepth`), single-callee + * (under `maxBranching`), one trace each (under the per-entry budget), and + * `maxProcesses` is set high enough that none are dropped. + */ + const manyCandidates = (): ReturnType => { + const graph = createKnowledgeGraph(); + for (let c = 0; c < 205; c++) { + for (let i = 0; i < 3; i++) addFn(graph, `func:c${c}_${i}`); + for (let i = 0; i < 2; i++) addCall(graph, `func:c${c}_${i}`, `func:c${c}_${i + 1}`); + } + return graph; + }; + + it('counts the candidates that never made the ranked list', async () => { + const result = await processProcesses(manyCandidates(), [], undefined, { + maxProcesses: 1000, + }); + + // 410 candidates, 200 kept: the counter reports what `entryPointsFound` + // structurally cannot. + expect(result.stats.entryPointsFound).toBe(200); + expect(result.stats.truncation.entryPointCandidatesDropped).toBe(210); + }); + + it('folds the new ceiling into `truncated`, and fires ALONE', async () => { + // Asserted exhaustively rather than as `truncated === true`: if any other + // counter were also non-zero the first assertion would prove nothing about + // which ceiling was detected. + const result = await processProcesses(manyCandidates(), [], undefined, { + maxProcesses: 1000, + }); + + expect(result.stats.truncation).toEqual({ + truncated: true, + entryPointCandidatesDropped: 210, + entryPointsUnexplored: 0, + walksCutByBudget: 0, + tracesDepthCapped: 0, + calleesDropped: 0, + processesDropped: 0, + }); + }); + + it('reports nothing dropped when every candidate fits', async () => { + // The control for the two above — the counter must not simply always fire. + const graph = createKnowledgeGraph(); + for (let c = 0; c < 5; c++) { + for (let i = 0; i < 3; i++) addFn(graph, `func:s${c}_${i}`); + for (let i = 0; i < 2; i++) addCall(graph, `func:s${c}_${i}`, `func:s${c}_${i + 1}`); + } + + const result = await processProcesses(graph, [], undefined, { maxProcesses: 1000 }); + + expect(result.stats.truncation.entryPointCandidatesDropped).toBe(0); + expect(result.stats.truncation.truncated).toBe(false); + }); +}); + +// The three trace sorts in this file each joined the path inside the COMPARATOR +// — up to four joins per comparison — and two of the three joined on a SPACE. +// Both are now one shared helper keyed on NUL. +// +// The separator is not cosmetic. Node ids embed file paths and a path may +// contain a space, so `['A B', 'C']` and `['A', 'B C']` produce the same +// space-joined key, the comparator returns 0, and a stable sort falls back to +// the input order the tiebreak exists to remove — the exact defect W2-5 fixed, +// reintroduced by the key. `traceKey` two functions away already pads with `->` +// because an unanchored join is ambiguous (#2894); this is the same lesson. +describe('trace ordering is total and allocation-free (#2899 follow-up)', () => { + const noSink = (): boolean => false; + + /** + * A deterministic 200-trace corpus with NO space in any id — i.e. the corpus + * on which the old space-joined key and the new NUL-joined key must agree. + * + * Lehmer LCG rather than `Math.random`: the assertion below is an ORDER + * IDENTITY claim, and evidence for it has to be reproducible. Every trace ends + * in an id unique to it, which is what keeps subsumption out of the way so the + * function returns exactly its sorted input. + */ + const seededCorpus = (): string[][] => { + let seed = 20260809; + const next = (): number => (seed = (seed * 48271) % 2147483647); + const traces: string[][] = []; + for (let i = 0; i < 200; i++) { + const depth = 3 + (next() % 3); + const trace: string[] = []; + for (let j = 0; j < depth - 1; j++) trace.push(`n${next() % 6}`); + trace.push(`term${i}`); + traces.push(trace); + } + return traces; + }; + + it('produces exactly the order the space-joined comparator produced', () => { + // ORDER IDENTITY. The refactor is only allowed to change WHEN keys are + // built, never the resulting order, because the order is what the + // `maxProcesses` cap consumes. Both separators sort below every character a + // node id can contain, so joining on either is order-equivalent to comparing + // the arrays element by element — this pins that equivalence instead of + // asserting it in a comment. + const corpus = seededCorpus(); + const legacy = [...corpus].sort( + (a, b) => + b.length - a.length || (a.join(' ') < b.join(' ') ? -1 : a.join(' ') > b.join(' ') ? 1 : 0), + ); + + expect(deduplicateTraces(corpus, noSink)).toEqual(legacy); + }); + + it('orders a pair that COLLIDES under a space separator', () => { + // `['r', 'a b', 'c']` and `['r', 'a', 'b c']` both join to "r a b c", so the + // space comparator returns 0 and `Array.prototype.sort`, being stable, hands + // the decision back to input order. Under NUL they differ at the third + // character and the order is fixed. + const first: string[][] = [ + ['r', 'a b', 'c'], + ['r', 'a', 'b c'], + ]; + const second: string[][] = [ + ['r', 'a', 'b c'], + ['r', 'a b', 'c'], + ]; + + expect(deduplicateTraces(first, noSink)).toEqual(deduplicateTraces(second, noSink)); + }); +}); + +// The same collision, reached through the WHOLE processor rather than one +// helper — because a space in a node id is not hypothetical (ids embed file +// paths, and directories with spaces are ordinary), and because W2-5 states its +// guarantee over `processProcesses`, not over its internals. +// +// The observable defect was narrower than the collision itself: `rankedByInterest` +// already keyed on NUL, so the FINAL rank was safe. It was `deduplicateByEndpoints` +// — which keeps ONE representative per entry->terminal pair — that still joined on +// a space, so when two equal-length paths between the SAME two endpoints collided, +// which one survived was decided by insertion order. The surviving path is what +// the `Process` node records, so the persisted graph differed. +describe('insertion-order invariance survives ids containing spaces', () => { + /** + * Two four-step paths from `func:r` to `func:z`, via `func:a b -> func:c` and + * via `func:a -> b func:c`. Both join to "func:r func:a b func:c func:z" under + * a space, so the endpoint-dedup comparator returned 0 and kept whichever the + * DFS happened to reach first. Under NUL they differ at the separator after + * `func:a` and the representative is fixed. + */ + const collidingGraph = (reverse: boolean): ReturnType => { + const graph = createKnowledgeGraph(); + const add = (id: string, name: string): void => { + graph.addNode({ + id, + label: 'Function', + properties: { name, filePath: 'src/a.ts', startLine: 1, endLine: 2 }, + }); + }; + const call = (from: string, to: string): void => { + graph.addRelationship({ + id: `rel:${from}=>${to}`, + sourceId: from, + targetId: to, + type: 'CALLS', + confidence: 1, + reason: 'test', + }); + }; + const branches: [string, string][] = [ + ['func:a b', 'func:c'], + ['func:a', 'b func:c'], + ]; + const ordered = reverse ? [...branches].reverse() : branches; + add('func:r', 'r'); + add('func:z', 'z'); + for (const [mid, next] of ordered) { + add(mid, 'mid'); + add(next, 'next'); + } + for (const [mid, next] of ordered) { + call('func:r', mid); + call(mid, next); + call(next, 'func:z'); + } + return graph; + }; + + it('keeps the same representative path whichever branch is inserted first', async () => { + const a = await processProcesses(collidingGraph(false), []); + const b = await processProcesses(collidingGraph(true), []); + + // One entry->terminal pair, so endpoint dedup keeps exactly one path — and + // that path is what the Process node records. + expect(a.processes.length).toBe(1); + expect(a.processes[0]?.trace).toEqual(b.processes[0]?.trace); + }); + + it('selects the same flow under a cap whichever branch is inserted first', async () => { + const a = await processProcesses(collidingGraph(false), [], undefined, { maxProcesses: 1 }); + const b = await processProcesses(collidingGraph(true), [], undefined, { maxProcesses: 1 }); + + expect(a.processes.length).toBe(1); + expect(a.processes[0]?.trace).toEqual(b.processes[0]?.trace); + }); +}); + +// #2894. `deduplicateTraces` decided subsumption with an UNANCHORED +// `String.includes`, so a match could begin in the middle of a node id and a +// trace was discarded against a chain it does not appear in. +// +// Reported as measured-inert — the collision needs one node id to be a strict +// suffix of another at a `->` boundary, and real ids (`Function::`) +// do not produce that. These use bare ids to exercise the predicate directly, +// which is the only way to reach it: the shape cannot be built from realistic +// ids, and that is precisely why nothing caught it. +describe('trace subsumption matches whole steps only (#2894)', () => { + const noSink = (): boolean => false; + + it('keeps a trace whose key appears mid-identifier in a longer trace', () => { + // 'X->AA->B'.includes('A->B') is true, but `A` is not a step of that chain. + const kept = deduplicateTraces( + [ + ['X', 'AA', 'B'], + ['A', 'B'], + ], + noSink, + ); + expect(kept.map((t) => t.join('->'))).toContain('A->B'); + }); + + it('still discards a GENUINE sub-path', () => { + // The behaviour the predicate exists for, pinned so the fix cannot be + // "stop subsuming anything", which would pass the test above trivially. + const kept = deduplicateTraces( + [ + ['A', 'B', 'C'], + ['A', 'B'], + ], + noSink, + ); + expect(kept.map((t) => t.join('->'))).toEqual(['A->B->C']); + }); + + it('discards a sub-path that is a SUFFIX of a longer trace', () => { + // Padding both ends must not break suffix or prefix subsumption. + const kept = deduplicateTraces( + [ + ['A', 'B', 'C'], + ['B', 'C'], + ], + noSink, + ); + expect(kept.map((t) => t.join('->'))).toEqual(['A->B->C']); + }); +}); diff --git a/gitnexus/test/unit/processes-phase-sink-wiring.test.ts b/gitnexus/test/unit/processes-phase-sink-wiring.test.ts new file mode 100644 index 000000000..8f91e79a4 --- /dev/null +++ b/gitnexus/test/unit/processes-phase-sink-wiring.test.ts @@ -0,0 +1,301 @@ +/** + * The processes phase's SINK WIRING, exercised on its success path (#2896). + * + * `processesPhase` reads `allFetchCalls` / `allORMQueries` off the parse output + * to build the R3-6 sink set, wrapped in a `try/catch` that falls open to "no + * sinks". Every other phase-level test omits `parse` from its deps map, so all + * of them take the CATCH branch — the success path had no coverage at all. + * + * That matters because `getPhaseOutput` is a raw `as T` cast. If the field names + * on `ParseOutput` ever drift, the phase reads nothing, detects zero sinks, and + * every existing test still passes, because zero sinks is exactly what they + * already assert. The wiring could break silently and in complete silence. + * + * So this asserts the thing only the success path can produce: a flow that ENDS + * at the sink, while a longer chain continues past it. Without the sink set that + * prefix is subsumed and only the long chain survives. + */ +import { describe, expect, it } from 'vitest'; +import { createKnowledgeGraph } from '../../src/core/graph/graph.js'; +import { _captureLogger, type LoggerCapture } from '../../src/core/logger.js'; +import { processesPhase } from '../../src/core/ingestion/pipeline-phases/processes.js'; +import type { ProcessesOutput } from '../../src/core/ingestion/pipeline-phases/processes.js'; +import type { + PhaseResult, + PipelineContext, +} from '../../src/core/ingestion/pipeline-phases/types.js'; +import type { KnowledgeGraph } from '../../src/core/graph/types.js'; +import type { GraphNode, NodeLabel } from 'gitnexus-shared'; + +function makeCtx(graph: KnowledgeGraph): PipelineContext { + return { repoPath: '/tmp/repo', graph, onProgress: () => {}, pipelineStart: 0 }; +} + +function phaseResult(phaseName: string, output: T): PhaseResult { + return { phaseName, output, durationMs: 0 }; +} + +const FILE = 'src/orders.ts'; + +function addNode(graph: KnowledgeGraph, id: string, label: NodeLabel, name: string, line: number) { + graph.addNode({ + id, + label, + properties: { + name, + filePath: FILE, + startLine: line, + endLine: line + 4, + isExported: true, + content: '', + }, + } satisfies GraphNode); +} + +function addCall(graph: KnowledgeGraph, from: string, to: string): void { + graph.addRelationship({ + id: `rel:${from}->${to}`, + sourceId: from, + targetId: to, + type: 'CALLS', + confidence: 1, + reason: 'test', + }); +} + +/** + * `scan -> score -> placeOrder -> formatDate`, where `placeOrder` performs the + * outward call. The business flow ends at `placeOrder`; the chain runs on into a + * helper. Both must exist as processes — that is the whole point of R3-6. + */ +function buildGraph(): KnowledgeGraph { + const graph = createKnowledgeGraph(); + addNode(graph, 'File:' + FILE, 'File', 'orders.ts', 1); + addNode(graph, 'Function:scan', 'Function', 'scan', 1); + addNode(graph, 'Function:score', 'Function', 'score', 10); + addNode(graph, 'Function:placeOrder', 'Function', 'placeOrder', 20); + addNode(graph, 'Function:formatDate', 'Function', 'formatDate', 30); + addCall(graph, 'Function:scan', 'Function:score'); + addCall(graph, 'Function:score', 'Function:placeOrder'); + addCall(graph, 'Function:placeOrder', 'Function:formatDate'); + return graph; +} + +const baseDeps = (): Map> => + new Map>([ + ['structure', phaseResult('structure', { totalFiles: 1 })], + ['communities', phaseResult('communities', { communityResult: { memberships: [] } })], + ['routes', phaseResult('routes', { routeRegistry: new Map() })], + ['tools', phaseResult('tools', { toolDefs: [] })], + ]); + +/** A fetch site INSIDE `placeOrder`, which is what makes it a sink. */ +const parseWithSinks = (): PhaseResult => + phaseResult('parse', { + allFetchCalls: [{ filePath: FILE, lineNumber: 22 }], + allORMQueries: [], + }); + +const terminalsOf = (graph: KnowledgeGraph): string[] => { + const out: string[] = []; + for (const node of graph.iterNodes()) { + if (node.label === 'Process') out.push(String(node.properties.terminalId)); + } + return out; +}; + +describe('processes phase — parse-output sink wiring (#2896)', () => { + it('declares `parse` as a dependency', () => { + // The read and the declaration must not diverge: dropping the dep would + // make `getPhaseOutput` throw into the fail-open catch on every run, and + // every other test would still pass. + expect(processesPhase.deps).toContain('parse'); + }); + + it('reads the parse output and produces a SINK-TERMINATED flow', async () => { + const graph = buildGraph(); + const deps = baseDeps(); + deps.set('parse', parseWithSinks()); + + await processesPhase.execute(makeCtx(graph), deps); + + // `placeOrder` is a terminal even though the chain continues into + // `formatDate` — only the sink set can produce that. + expect(terminalsOf(graph)).toContain('Function:placeOrder'); + }); + + it('the same graph WITHOUT parse yields no sink-terminated flow', async () => { + // The control. Without it the assertion above could pass for an unrelated + // reason — this is the fail-open branch every other phase test takes, and it + // is what makes the difference attributable to the wiring. + const graph = buildGraph(); + + await processesPhase.execute(makeCtx(graph), baseDeps()); + + expect(terminalsOf(graph)).not.toContain('Function:placeOrder'); + }); + + it('survives a parse output whose sink fields are absent', async () => { + // Fail-open is deliberate: a pipeline composed without those outputs should + // detect no sinks rather than lose every process. + const graph = buildGraph(); + const deps = baseDeps(); + deps.set('parse', phaseResult('parse', {})); + + await processesPhase.execute(makeCtx(graph), deps); + + expect(terminalsOf(graph).length).toBeGreaterThan(0); + }); +}); + +/** + * WHICH ceilings are loud (#2899 follow-up). + * + * The truncation disclosure landed as an ungated `logger.warn` on + * `truncation.truncated`, and this phase overrides only `maxProcesses` — so at + * the shipped defaults (`maxBranching: 4`, `maxTraceDepth: 10`, per-entry trace + * budget 12) it fired for any function with five callees and any chain deeper + * than ten, i.e. on every non-trivial repository, every run. A warning that + * always fires is a warning nobody reads. + * + * The split asserted here is the one `ProcessTruncationStats` already writes + * down: a ceiling that removes WHOLE FLOWS from the report warns; a ceiling that + * only makes a reported flow shorter than the code path it describes goes to + * debug. Both fixtures are truncated — what differs is which kind. + */ +describe('processes phase — truncation is disclosed proportionately (#2899)', () => { + const addFn = (graph: KnowledgeGraph, id: string): void => { + graph.addNode({ + id, + label: 'Function', + properties: { name: id.split(':')[1], filePath: 'src/a.ts', startLine: 1, endLine: 2 }, + }); + }; + const addCallEdge = (graph: KnowledgeGraph, from: string, to: string): void => { + graph.addRelationship({ + id: `rel:${from}->${to}`, + sourceId: from, + targetId: to, + type: 'CALLS', + confidence: 1, + reason: 'test', + }); + }; + + /** + * Trips ONLY the shape ceilings: a 13-long chain (traces cut at + * `maxTraceDepth`) and a five-way fan-out (a callee skipped at + * `maxBranching`). Every flow found is still in the report — none of the four + * surviving traces was dropped, and the phase's own `maxProcesses` floor of 20 + * is well clear of them. + */ + const shortenedOnly = (): KnowledgeGraph => { + const graph = createKnowledgeGraph(); + for (let i = 0; i < 13; i++) addFn(graph, `func:c${i}`); + for (let i = 0; i < 12; i++) addCallEdge(graph, `func:c${i}`, `func:c${i + 1}`); + addFn(graph, 'func:fanout'); + for (let i = 0; i < 5; i++) { + addFn(graph, `func:leaf${i}`); + addCallEdge(graph, 'func:fanout', `func:leaf${i}`); + } + return graph; + }; + + /** + * Trips ONLY the whole-flow ceilings: 40 independent three-step chains against + * a `maxProcesses` of 20 (the floor `computeDynamicMaxProcesses` gives 120 + * symbols), so half the deduplicated flows are dropped outright and the trace + * quota stops the loop with entry points still unvisited. Nothing here is + * deep enough or wide enough to hit `maxTraceDepth` or `maxBranching`. + */ + const flowsMissing = (): KnowledgeGraph => { + const graph = createKnowledgeGraph(); + for (let c = 0; c < 40; c++) { + for (let i = 0; i < 3; i++) addFn(graph, `func:p${c}_${i}`); + for (let i = 0; i < 2; i++) addCallEdge(graph, `func:p${c}_${i}`, `func:p${c}_${i + 1}`); + } + return graph; + }; + + const runCaptured = async ( + graph: KnowledgeGraph, + ): Promise<{ output: ProcessesOutput; records: ReturnType }> => { + // Captured at `debug` so an ABSENT warn can be distinguished from a silent + // phase: the debug line has to be there instead. + const capture = _captureLogger('debug'); + try { + const output = (await processesPhase.execute(makeCtx(graph), baseDeps())) as ProcessesOutput; + return { output, records: capture.records() }; + } finally { + capture.restore(); + } + }; + + const PROCESS_LINES = /^\[processes\] /; + + it('does NOT warn when the caps only made flows shorter', async () => { + const { output, records } = await runCaptured(shortenedOnly()); + const { truncation } = output.processResult.stats; + + // The fixture is genuinely truncated — this is not a "nothing happened" pass. + expect(truncation.truncated).toBe(true); + expect(truncation.tracesDepthCapped).toBeGreaterThan(0); + expect(truncation.calleesDropped).toBeGreaterThan(0); + // ...and truncated in NO other way, so the assertions below are attributable. + expect(truncation.entryPointCandidatesDropped).toBe(0); + expect(truncation.entryPointsUnexplored).toBe(0); + expect(truncation.processesDropped).toBe(0); + + const lines = records.filter((r) => PROCESS_LINES.test(String(r.msg))); + expect(lines.map((r) => r.level)).toEqual([20]); // debug, not warn + expect(String(lines[0]?.msg)).toContain('shorter than the code path'); + }); + + it('DOES warn when whole flows are missing from the report', async () => { + const { output, records } = await runCaptured(flowsMissing()); + const { truncation } = output.processResult.stats; + + expect(truncation.processesDropped).toBeGreaterThan(0); + expect(truncation.entryPointsUnexplored).toBeGreaterThan(0); + // Neither shape ceiling fired here, so the warn is attributable to the + // whole-flow counters and not to a chain that merely ran long. + expect(truncation.tracesDepthCapped).toBe(0); + expect(truncation.calleesDropped).toBe(0); + + const lines = records.filter((r) => PROCESS_LINES.test(String(r.msg))); + expect(lines.map((r) => r.level)).toEqual([40]); // warn + expect(String(lines[0]?.msg)).toContain('whole flows are MISSING'); + }); + + it('says nothing at all when no ceiling fired', async () => { + // The control for both: the phase must not narrate an untruncated run. + const graph = createKnowledgeGraph(); + for (let i = 0; i < 3; i++) addFn(graph, `func:q${i}`); + for (let i = 0; i < 2; i++) addCallEdge(graph, `func:q${i}`, `func:q${i + 1}`); + + const { output, records } = await runCaptured(graph); + + expect(output.processResult.processes.length).toBeGreaterThan(0); + expect(output.processResult.stats.truncation.truncated).toBe(false); + expect(records.filter((r) => PROCESS_LINES.test(String(r.msg)))).toEqual([]); + }); + + it('reports the entry-point candidate cap in the warn payload', async () => { + // The dominant ceiling on any real repository, and the one that stays in the + // loud set precisely because it is the only counter that grows with repo + // size — `entryPointsUnexplored` and `processesDropped` can only fire while + // `maxProcesses` is small enough to bind. + const graph = createKnowledgeGraph(); + for (let c = 0; c < 205; c++) { + for (let i = 0; i < 3; i++) addFn(graph, `func:m${c}_${i}`); + for (let i = 0; i < 2; i++) addCallEdge(graph, `func:m${c}_${i}`, `func:m${c}_${i + 1}`); + } + + const { output, records } = await runCaptured(graph); + + expect(output.processResult.stats.truncation.entryPointCandidatesDropped).toBe(210); + const lines = records.filter((r) => PROCESS_LINES.test(String(r.msg))); + expect(lines.map((r) => r.level)).toEqual([40]); + expect(String(lines[0]?.msg)).toContain('210 of 410 candidate entry point(s) never ranked in'); + }); +}); diff --git a/gitnexus/test/unit/shipped-skills-sync.test.ts b/gitnexus/test/unit/shipped-skills-sync.test.ts index 6a1077275..dce17a15c 100644 --- a/gitnexus/test/unit/shipped-skills-sync.test.ts +++ b/gitnexus/test/unit/shipped-skills-sync.test.ts @@ -188,6 +188,25 @@ describe('intended standard-skill improvements stay in every applicable copy', ( } }); + // #2899: the "Inline staleness signal" section was deleted from the + // canonical `.claude/` copy by an unrelated commit while the plugin mirror + // kept it — the same silent-deletion shape as the UNKNOWN-risk guard above, + // just for a hand-authored section instead of the machine-managed block. + // Scoped to canonical + plugin only: at the time of writing the npm mirror + // (gitnexus/skills/gitnexus-guide.md) already lacks this section as + // pre-existing, unrelated drift, so folding it into the loop above would + // fail on that unrelated copy instead of guarding this regression. + it('keeps the inline-staleness-signal section in the canonical and plugin guide copies', () => { + for (const file of [ + path.join(REPO_ROOT, '.claude', 'skills', 'gitnexus-guide', 'SKILL.md'), + path.join(REPO_ROOT, 'gitnexus-claude-plugin', 'skills', 'gitnexus-guide', 'SKILL.md'), + ]) { + const content = fs.readFileSync(file, 'utf-8'); + expect(content).toContain('### Inline staleness signal'); + expect(content).toContain('commitsBehind'); + } + }); + it("uses the rename API's text_search vocabulary in every refactoring copy", () => { for (const file of standardSkillCopies('gitnexus-refactoring')) { const content = fs.readFileSync(file, 'utf-8'); @@ -197,6 +216,64 @@ describe('intended standard-skill improvements stay in every applicable copy', ( }); }); +// The root AGENTS.md / CLAUDE.md machine-managed block ( +// ... ) is regenerated by generateGitNexusContent +// (src/cli/ai-context.ts) on every `gitnexus analyze`. The `risk: UNKNOWN` +// Always-Do bullet and its Never-Do clause were hand-added INSIDE that region +// instead of living in the template, so a real analyze run silently deleted +// them on regeneration — twice (#2856's 8f8261021, then #2899's 9e602aef0, +// which piggybacked an unrelated fetch-parsing fix and also regressed the +// index stats 248612/565510/918 -> 42853/135955/758, itself evidence the +// block had been rebuilt from a stale local index). ai-context.ts now +// generates both lines directly regardless of `hasPdg` (see +// ai-context.test.ts's hasPdg-independent UNKNOWN test), so a real analyze +// cannot drop them again. This guard is the second line of defense: it reads +// the committed docs themselves, so a hand-revert or a stale generator binary +// landing the same regression fails here even if the template is fine. +describe('root AGENTS.md / CLAUDE.md managed block keeps the risk: UNKNOWN policy (#2899)', () => { + const REQUIRED_FRAGMENTS = [ + 'MUST treat `risk: UNKNOWN` as unresolved, not as low.', + 'never read `UNKNOWN` as an all-clear', + ]; + + function extractManagedBlock(file: string): string { + const content = fs.readFileSync(path.join(REPO_ROOT, file), 'utf-8'); + // Markers must occupy their own line — CLAUDE.md's "GitNexus rules" + // section links to AGENTS.md with an inline prose mention of both + // marker strings ("See the ` ... `" etc.) that a + // bare indexOf would mistake for the real block (mirrors + // findSectionMarkerIndex in ai-context.ts, #1041). + const match = + /(?:^|\n)\r?\n([\s\S]*?)\n(?:\r?\n|$)/.exec( + content, + ); + expect(match, `${file} must contain an own-line gitnexus:start/end block`).not.toBeNull(); + return match![1]; + } + + it.each(['AGENTS.md', 'CLAUDE.md'])('%s managed block documents the policy', (file) => { + const block = extractManagedBlock(file); + for (const fragment of REQUIRED_FRAGMENTS) expect(block).toContain(fragment); + }); + + it.each(['AGENTS.md', 'CLAUDE.md'])( + "%s managed block's Always Do / Never Do bullet counts do not drop below the known floor", + (file) => { + const block = extractManagedBlock(file); + const alwaysDoSection = block.slice( + block.indexOf('## Always Do'), + block.indexOf('## Never Do'), + ); + const neverDoSection = block.slice(block.indexOf('## Never Do')); + // 7 Always-Do bullets are unconditional; an 8th (pdg_query) only + // appears when the index was built with --pdg, so the floor is 7, not 8. + expect((alwaysDoSection.match(/^- /gm) || []).length).toBeGreaterThanOrEqual(7); + // Never Do never varies with hasPdg — exactly 4 today, so 4 is the floor. + expect((neverDoSection.match(/^- NEVER /gm) || []).length).toBeGreaterThanOrEqual(4); + }, + ); +}); + describe.each(FAMILY)('shipped copies of %s stay in sync', (name) => { const canonical = snapshotDir(path.join(REPO_ROOT, '.claude', 'skills', name));