5 commits

Author	SHA1	Message	Date
DuduPhudu	fa31a7d824	fix: close the nine follow-up review findings from #2856 (routes, receiver typing, truncation honesty) (#2899) ... * fix(typescript): a type parameter shadows a declared type of the same name (W2-8) First item of wave 2, promised to the reviewer on #2856. `export function unwrap<Result>(value: Result): Result` names the PARAMETER, not the `interface Result` beside it — tsc resolves both annotations to the parameter. 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 new fixture: `unwrap` produced TWO false edges while the genuine consumer produced one. Blast radius is every generic whose parameter name collides with a declared type, and the colliding names are ordinary choices for both: `Result`, `Key`, `Value`, `Item`, `Node`, `Options`, `Config`, `Props`, `State`, `Response`. TWO HALVES, and the first is why upstream's fix could not reach this. #2833 introduced `bindsTypeParameter` for the CALL-receiver path, where a workspace `class T` was answering for `<T>`. Reusing it here changed nothing at first, and the reason is its own documented contract: `@declaration.type-parameters` was captured for class/interface declarations ONLY, so a generic FUNCTION recorded no parameter list and the predicate correctly returned false — absence is not evidence. The data was missing, not the logic. So: - TYPESCRIPT_SCOPE_QUERY now captures type parameters on `function_declaration`, `generator_function_declaration` and `type_alias_declaration`; - the graph bridge consults `bindsTypeParameter` before emitting `USES`. Both are load-bearing — removing either one fails the fixture. The fixture carries two controls, because the obvious wrong fix is to stop emitting: a genuine consumer of the interface must still link, and a generic whose parameter does NOT collide must still link its real reference. Both are asserted, and the "genuine consumer" case is asserted FIRST so the absences below it cannot pass vacuously. SCHEMA_BUMP 53 -> 54: parse-time capture change. A warm cache replays defs with no parameter list, so the guard reads nothing and the feature is inert while looking implemented. Capture fingerprint re-baselined with justification. NO NEW CAPTURE NAME — diffing the capture-name sets against the wave-1 branch returns empty; the tag existed and now fires on more declarations. capture_groups_fp 2338 -> 2371, fixture_count 151 -> 152, scaling 1.06 < 1.5, and JavaScript's fingerprint does not move at all, which is the check that this is the TS declaration rules rather than something broader. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(analyze): close the four false-success paths in the graph-write-collapse guard (W2-6) Second wave-2 item, promised on #2856. All four were reported; all four reproduced by reading the code they name. (a) A SAME-COMMIT RE-RUN REPORTED SUCCESS FOREVER. Every other meta-driven trigger — schema fingerprint, PDG mode, runner identity, CJK segmentation, embedding dims — has a block that forces a rebuild before the `alreadyUpToDate` fast path. `graphWriteCollapsed` had none; `grep -rn` found writes and no reads. So the one state meaning "most of your edges are gone" was the one state that repaired itself only if the user happened to pass `--force`. Now forces a full rebuild, and forcing is right rather than merely re-running: the persisted graph disagrees with what the pipeline produced, so an incremental pass over unchanged files would write nothing and re-stamp the same broken index as fresh. (b) AN INCREMENTAL RE-RUN ERASED THE STAMP. `saveMeta` is a full atomic overwrite, and the field was spread in only when the CURRENT run had a verdict. `undefined` meant two different things at that site — "full run, no collapse" (a positive all-clear) and "incremental write, not comparable" (no opinion) — so the second case silently dropped `graph-write-collapsed` from meta.json while the edges were still missing. Now three-way: stamp on detection, CLEAR on a healthy full run, CARRY FORWARD when there is no verdict. That is the shape `branch: branchLabel ?? existingMeta?.branch` two lines away had all along. (c) THE SERVER PATH NEVER CONSUMED IT. `analyze-worker-ipc.ts` projects the field "so a server-side caller sees the same degraded outcome the CLI does" — but nothing read it, so the comment described an intention and every collapsed run reported `complete` to the UI and to every API consumer. Now reports `failed` with the counts and the remedy, matching the CLI, which prints `Repository indexed INCOMPLETELY` and exits non-zero. A consumer that reads "complete" will query the index and get confident wrong answers. (d) --pdg ROWS MASKED TOTAL STRUCTURAL LOSS. `expected` counts the in-memory graph plus the streamed STRUCTURAL manifest; the streamed PDG layers never enter `graph.relationshipCount`. But `persisted` was `stats.edges`, a count of EVERY `CodeRelation` row, and PDG writes into that same table. With 1,000 structural edges expected and 4,000 PDG rows persisted, losing every structural edge still read `persisted = 4000`, cleared the ratio, and stayed silent — on exactly the large repos `--pdg` is used for. Worth recording that the OBVIOUS fix does not work. Padding `expected` with the PDG rows makes the two universes match but leaves the ratio judging a minority population: 4,000 of 5,000 still clears 0.5. I wrote that first, and the test I wrote to prove it failed. Only comparing structural against structural asks the question the check exists to ask, so `getLbugStats` gains a `structuralEdges` count excluding `PDG_EDGE_TYPES`. `TAINT_PATH` is deliberately NOT in that set — it is a whole-program Function→Function edge persisted by the normal emit, so it is structural and stays counted on both sides. `index-freshness-graph-collapse.test.ts` had pinned the masking as correct (`detectGraphWriteCollapse(1000, 4000)` → undefined, "PDG layers write into the same table, so persisted > expected is normal"). True about the table, and it licensed the hole. Replaced with the case that matters and a note on why the fix is at the caller. The new `structuralEdges` assertion in `lbug-core-adapter` is there because the failure mode is silent: the query sits in a try/catch that yields `undefined`, and `undefined` makes the collapse check decline to compare — so a typo in the Cypher would throw nothing, fail nothing, and switch the guard off. Verified against a real LadybugDB and mutation-checked by breaking the query. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(processes): make process selection insertion-order invariant (W2-5) Third wave-2 item. Reproduced before fixing: 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, a different persisted graph — so a filesystem that enumerates differently, or an incremental run that reorders assembly, silently changes what the tool reports. Four sorts 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()`. Under `maxProcesses` capping that decided which `Process` and `STEP_IN_PROCESS` nodes were persisted at all. Each now falls through to a totally-ordered, content-derived key — node id for entry points, the joined path for traces. WHAT IS ACTUALLY VERIFIED, stated precisely because "four fixes" would overclaim: - the ENTRY-POINT sort is individually mutation-verified; - the two DEDUP sorts are collectively mutation-verified; - the TRACE-RANK tiebreak is NOT individually observable, and the source says so. The dedup sorts already impose a total order on the list that reaches it, so removing it alone fails nothing. Kept as defence in depth: it cannot misbehave — it only makes an already-deterministic order explicit — and it is what stops a change to dedup ordering from silently re-opening this. Finding that out took two fixtures. The first (three chains, three entry points) is separated by the entry-point sort before trace ranking is reached, so it never exercises the trace comparator at all; the second gives ONE entry point two equal-length branches to different terminals, which is the only shape where the trace comparator decides. Both are kept — they gate different sites. The invariance tests assert the INVARIANT rather than any single sort, so they cover all four sites and any future one without needing to know where they are. Three assertions: same selection under a cap, identical set uncapped, and identical ORDER — the last because order is what the cap consumes, so a set-only assertion would pass while the defect persisted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(impact): UNKNOWN dominates a mixed candidate set, instead of reporting the known floor (W2-4) Fourth wave-2 item. The all-UNKNOWN branch here was reasoned about carefully and is correct — its comment even names the two ways a set can be all-UNKNOWN. The MIXED case fell straight through it. `RISK_ORDER` is `['LOW','MEDIUM','HIGH','CRITICAL']` and has no `UNKNOWN` entry, so `indexOf('UNKNOWN')` is -1 and an UNKNOWN candidate can never win the reduce. An ambiguous name with one caller-less candidate (UNKNOWN, per the round-1 fix) beside one single-caller candidate (LOW) reported `maxRisk: 'LOW'` — a confident floor over a set containing an interpretation nobody measured. That is the same false-safe the all-UNKNOWN branch exists to prevent, one case over, and it surfaced in the UI as "Max blast radius N (LOW risk)". `maxRisk` answers "how bad could this be?", and an unresolved candidate could be CRITICAL — so any UNKNOWN in the set makes the aggregate UNKNOWN. Narrowing it that way would normally cost information, so the measured part travels alongside as `knownMaxRisk`, present only when the two differ: absent on a fully-resolved set, where it would duplicate `maxRisk`, and absent on a fully-unknown one, where there is no measured part. A 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. The human-readable message says the same thing. The seed gained a mixed pair because the existing one could not reach this: both its twins are caller-less, so it only ever exercises the all-UNKNOWN branch — which is precisely why the gap survived a round of review. Three assertions, both halves mutation-verified. `eval-server.ts` needs no change: it renders `result.maxRisk ?? 'UNKNOWN'`, so it now shows UNKNOWN where it previously showed the floor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(routes): track ternary polarity in dispatch guards, so a selected verb cannot be inverted (W2-9) `if ((req.method === 'GET' ? false : true) && pathname === '/api/i')` emitted `GET /api/i` — the one method that branch guarantees the request does NOT have. A ternary SELECTS between its arms, so a verb inside one is not reached merely because the whole condition is truthy, but `findVerbInSubtree` descended into both arms and returned the first verb it saw. Same inversion `!` produced before d4dcba8c, one level up. Handled by folding the ternary where an arm is a boolean literal, which is what collapses the selection into a conjunction: c ? A : false == c && A both hold, so search 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 Two non-literal arms leave the verb chosen by an unknown condition, so the ternary guarantees nothing. Refusing every ternary would also have fixed the reported bug, but three of the four shapes measured were ALREADY correct and would have silently lost their verb; they are pinned now. A second defect in the same walk, found while reproducing: the `!` rule was keyed on PRESENCE, returning null at the first negation it saw, while `isNegatedContext` two functions above states the rule is PARITY and says so outright — `!!x` is `x`. So `!!(req.method === 'GET')` dropped a verb the source states plainly. The existing double-negation test covered the PATH position, where the parity walk already ran, and so never saw it. The verb walk now tracks parity too, and the two agree. Verb-less, not route-less: the path comparison is untouched evidence that the branch serves that path, so an inverted verb becomes a missing verb rather than a missing route. SCHEMA_BUMP 54 -> 55. Routes are emitted at parse time and replayed verbatim from a warm cache, so without the bump an already-indexed repo keeps serving the inverted verb and the fix looks inert. Free against origin/main (48). Every rule mutation-checked: removing the ternary dispatch, either literal-arm rule, the negated-ternary guard, or the parity walk each fails exactly the tests that claim it. One assertion I wrote survived all five mutations and was removed rather than kept. Not a recall win on crypto-trading-bot, which contains neither shape — this is precision insurance for dispatchers that do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(routes): report every method a dispatch guard serves, not just the first (R3-8 part 1) `if ((req.method === 'GET' || req.method === 'POST') && bundlesMatch)` is two routes. The verb walk returned the FIRST verb it found, so `route_map` presented a two-method route as GET-only and `impact` on the POST path found nothing. Taken verbatim from the reporting repo's researchRunRoutes.js. `governingVerb` -> `governingVerbs`, returning a list; `findVerbInSubtree` and `verbFromTernary` likewise. A guard with several verbs emits one route per verb via the new `pushPerVerb` — 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. A disjunction yields ALL its verbs or NONE, which also fixes an over-attribution the first-match rule had: req.method === 'GET' || req.method === 'POST' -> GET, POST req.method === 'GET' || isAdmin -> no verb The second is reached for ANY method when `isAdmin` holds. Reporting `GET` — as first-match did — describes a route open to everything as single-method, which is the direction this module treats as more expensive than saying nothing. Negated, `!(A || B)` is `!A && !B`, so it excludes verbs rather than offering them and yields none. Generic descent deliberately stays 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 unrelated than alternatives. `||` is the one construct that genuinely means "either of these". Pinned against regression: the pre-existing rule that distributes ONE verb across an OR of PATHS must not start multiplying methods, and switch arms inherit the full method set. SCHEMA_BUMP 55 -> 56. Routes are parse-time output replayed verbatim from a warm cache. Free against origin/main (48). Four mutations, each failing exactly the tests that claim it: removing the disjunction dispatch, dropping the all-operands rule, allowing a disjunction at odd parity, and emitting only the first verb. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(routes): read `.match()` dispatch, and the capturing wildcard it needs (R3-8 part 2) `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 rather than the module that serves them: those modules dispatch with `.match`. THE CAPTURING WILDCARD, which is the part that made the rest inert. `regexToRoutePath` accepted `[^/]+` and refused `([^/]+)` — `(` fell through to the metacharacter bail. So the non-capturing form translated and the capturing form produced nothing, and every existing test passed because every existing test used the non-capturing form. The tests were written against the implementation rather than against the corpus, and the reporting repo contains no non-capturing path wildcard at all: a dispatcher captures the segment because it needs the id. This alone also repairs the already-shipped `.test` rule. A capture around anything that is NOT one segment still bails — `(.+)` spans slashes — and the alternation is balanced, so `([^/]+` unclosed is not a match. `.match` differs from `.test` in one way that matters: its result is USED, so it is almost always BOUND, and the verb then lives in a later `if`: 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 records `name -> path` and the route is emitted where the binding is TESTED, once per test site — one binding tested for GET and for PUT is two routes. A reference counts only in a truthiness position (`&&`/`||` operand, or a whole `if` condition), which is what separates `if (m && …)` from `m[1]`: a read of the captured segment says nothing about dispatch and would otherwise mint a duplicate route per use of the id. A binding never tested still emits one verb-less route — the code did compute an anchored match against the path. Regexes named by a same-file const resolve too (`pathname.match(POSITION_REPLAY_RE)`), with the same ambiguity refusal the string-constant map uses: bound twice to different patterns means dropped, because a half-right regex is a wrong route. SCHEMA_BUMP 56 -> 57. Free against origin/main (48). Nine mutations, each failing exactly the tests that claim it. TWO of my own tests initially survived their mutation and were rewritten, not kept: - the non-path-receiver case had no path token anywhere in the fixture, so PATH_TOKEN_HINT skipped the file and the assertion was satisfied by a file that was never examined; - the negation case used `!m`, which never reaches the negation check at all — a `unary_expression` parent is not a truthiness position to begin with. The shape that exercises it is `!(req.method === 'GET' && m)`. A declaration-site skip written alongside them proved unreachable for the same reason and was removed rather than left to imply a hazard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(processes): report what the detection ceilings dropped, instead of logging it at debug (W2-3) `processProcesses` has five ceilings - the entry-point trace quota, the per-entry trace budget, `maxTraceDepth`, `maxBranching` and `maxProcesses` - and every one of them fired silently. The result came back looking whole and no consumer could tell it was a sample. The code's own comment already said so: // A silently truncating cap reads as "this is everything", which is the // same class of confident-empty answer this work is about. and then only called `logger.debug`. A log nobody has enabled is not a disclosure. `stats.truncation` is additive, so every existing consumer of `totalProcesses` / `crossCommunityCount` / `avgStepCount` / `entryPointsFound` is unchanged. It carries one boolean to branch on plus a counter per ceiling, kept SEPARATE rather than summed because they mean different things: unexplored entry points mean whole flows are missing, while a depth-capped trace means a flow is present but shorter than it really is. `processesDropped` counts against the DEDUPED population, not the raw trace list - the gap between those two is deduplication doing its job, and counting it as truncation would report a permanent non-zero on every healthy repo. `truncated` is DERIVED from the counters rather than set at each site, so a ceiling added later only has to increment its own counter to be reported. Surfaced at `warn` and NOT gated on `isDev`: "823 flows" printed without it reads as the complete set, which is the confident-empty failure wearing its other face - a confident-COMPLETE one. The debug line stays for the per-entry detail it carries. Seven mutations, each failing exactly the tests that claim it, including BOTH directions of the flag: hardcoding `truncated` false fails the four positive cases, and hardcoding it true fails the nothing-was-truncated case, which is asserted first precisely so the positives cannot pass vacuously. The `walksCutByBudget` fixture gives every node exactly `maxBranching` callees so it asserts its own counter and not a neighbour's. Also fixes a defect this work exposed: 10b0c7a1 (W2-5) embedded a RAW NUL BYTE in `trace.join(...)` instead of the backslash-u escape the rest of the repo uses. It behaves identically at runtime, but `file` reports the source as `data`, and grep, git diff and code search treat it as binary - several greps against this file silently returned nothing while I was reading it. main was clean here; two other files carry the same raw byte from before this branch and are left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): resolve members through a MEMBER-CALL producer's return shape (W2-1) const svc = new SignalService() const r = svc.make() return r.secretFlag // <- no edge `return-shape-members` types `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` — a METHOD, never a callable binding in scope. The producer lookup failed and the pass declined. The limit shipped documented as needing inter-procedural receiver typing. It does not. Measured on a fixture, the pipeline had already done the hard part: - `readMake -> Method:...SignalService.make#0` already resolves as an ordinary CALLS edge, so the receiver is already typed; and - `Property:...SignalService.make.secretFlag@N:C` already exists, because R3-4 anchors a returned literal's keys to the METHOD that returns them, not only to free functions. Both halves were present and unjoined — the same shape as R3-5 itself. ADDITIVE, not a reroute. The new branch sits inside `if (producerFile === undefined)`, so it can only fire where the callable lookup already declined; every reference that resolved before resolves identically, by construction rather than by test. Nothing new is inferred. The receiver is typed by the SAME predicate that typed `r`, and it must itself resolve to a class — a receiver that cannot be typed still declines, so `make.<member>` is never matched by name across the graph. That fabrication is what the existing guards exist to stop and they all carry over unchanged: the owner must resolve, its file must match the candidate's, and `ownFilePaths` keeps the polyglot class registry from walking a JS read into a Java field. The owner segment is TWO parts for a method (`SignalService.make`) and one for a free function (`makeSignal`), which is exactly how R3-4 qualifies each. That is what separates two methods of one class returning the same key name from each other AND from a free function of that name — the fixture gives `secretFlag` three owners so a wrong resolution is detectable rather than a coin flip that happens to look right. Four mutations, each failing exactly the two tests that claim it: removing the fallback, using the method alone as the owner segment, taking the producer file from the reading file instead of the owner class, and dropping the receiver-type requirement. 3,408 resolver tests pass, including `polyglot-property-isolation`, which is the one this could plausibly break. No SCHEMA_BUMP: this is a resolution pass over ParsedFiles, not parse-time output, so a warm cache replays the same input and produces the new edges. Measured on crypto-trading-bot: ZERO new edges, byte-identical at 62,158. Its 170 `const x = new Y()` bindings are overwhelmingly built-ins (Map, Set, Promise, S3Client) rather than workspace classes whose methods return object literals — it is a module-style JS codebase. Correctness fix for class-shaped code, not a recall win on this corpus, and it should not be presented as one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): type a bare parameter from what its callers pass (W2-2) function readSpike(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, by far the largest: 11,012 of 13,672 property edges on the reporting repo (81%) rest on that name guess. The two facts needed were already extracted, for a different consumer. For JS and TS among others, `callable-flow-captures` synthesizes: formal owner=readSpike binding=spike parameter-index=0 argument source=s parameter-index=0 direct-callee-name=readSpike Joining them on (callee, parameterIndex) says which cell reaches which parameter, and the argument's own binding is typed by the same `findReceiverTypeBinding` a directly-bound receiver already uses. So the parameter inherits the producer and `spike.wickRatio` resolves as evidence rather than inference. No new capture, no parse-time change, NO SCHEMA_BUMP. And deliberately not a change to the callable-value-flow solver that owns these sites: that pass is guarded by a fingerprint CORRECTNESS gate plus a timing budget, so this reads the same facts and computes its own map. AMBIGUITY DECLINES. A parameter whose callers pass 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. Keyed by the formal's (scope, name), not by a definition id. The first attempt used a def and measured `paramDef=NONE`: a parameter is not reachable through `findValueBindingInScope` (its predicate is `isOwnableValueLabel`, which lists Const/Variable/Property/Static because it exists for OWNERSHIP registration, and a parameter is owned by nothing) and it is not a `local` binding either. The formal site already states the scope its parameter binds in, which is enough. Formals carry their DECLARING FILE in the key, so two same-named functions in different files cannot answer for each other — dropping it makes both go ambiguous and both readers silently lose their edge. COVERAGE, counted rather than assumed. The synthesis skips an argument that is itself a call result (an explicit `continue` in `callable-flow-captures`), so `f(makeSignal())` emits no argument site and only the bound spelling `const s = makeSignal(); f(s)` is served. That looked fatal until measured: in the reporting repo, bare-identifier arguments outnumber call-result arguments 2,563 to 50 — 51:1. Extending the shared, benched capture synthesis for the 2% case is not worth its risk. Four mutations, each failing exactly the tests that claim it: keeping the first producer instead of declining on conflict, dropping the read-site lookup, matching a formal at index 0 regardless of the argument's index, and dropping the declaring file from the formal key. Two of those could not be caught by the first fixture at all — it had a single parameter and a single consumer file — so the fixture gained a two-parameter callee and a same-named twin in a second file before they were meaningful. The test helper also had to start filtering by source FILE, or two different `readSpike` symbols merged into one count. Measured on crypto-trading-bot: 36 reads left the 0.5 name-guess tier. 26 became precise 0.9 edges (return-shape reads 1,130 -> 1,156, which is the whole delta), and 10 became honest absences — the receiver was typed, the producer's shape was known, and the member is NOT on it, so the site is claimed as disproved rather than left for the name fallback to invent an answer for. That is ~0.3% of the 11,012, and it should be reported as such. The 81% figure is the size of the PROBLEM, not of this fix: the shape requires a bound argument, a producer that returns an object literal, and a parameter read as a receiver, and that intersection is narrow. The remaining name-tier reads are mostly receivers no workspace producer types at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(processes,ci): anchor trace subsumption, cover the sink wiring, stop one bench guard hiding the rest (#2894, #2896, #2895) Three follow-ups reported against #2856 after it merged. Each was reproduced before it was fixed. #2894 — trace subsumption matched mid-identifier. `deduplicateTraces` decided whether one trace is a sub-path of another with an UNANCHORED `String.includes`, so a match could begin in the middle of a node id: 'X->AA->B'.includes('A->B') -> true and `A -> B` was discarded as redundant against a chain `A` is not a step of at all. Reproduced directly against the function before fixing. Padding both keys with the separator makes `includes` match whole steps only. Reported as measured-inert and that holds — the collision needs one node id to be a strict suffix of another at a `->` boundary, which real ids (`Function:<path>:<name>`) do not produce. Fixed anyway because the predicate did not mean what the surrounding code says it means, in a function whose entire job is deciding what to delete, and nothing pinned it. `deduplicateTraces` is exported for the test, matching how `traceFromEntryPoint` and `buildSinkFunctionSet` are already reached. The tests use bare ids because the shape cannot be built from realistic ones — which is exactly why nothing caught it. Alongside the regression case, two tests pin that GENUINE subsumption still happens, prefix and suffix, so the fix cannot degenerate into "subsume nothing" and pass the first test trivially. Mutation-checked: reverting the padding fails the mid-identifier test and only that one. The encoding assumes `->` never appears IN a node id; a C++ `operator->` would defeat the join regardless of padding. Out of scope, but the assumption is now written down where the join happens. #2896 — the sink wiring was only ever exercised through its fail-open catch. `processesPhase` reads `allFetchCalls` / `allORMQueries` off the parse output inside a try/catch that falls open to "no sinks", and every phase-level test omitted `parse` — so all of them took the CATCH branch and the success path had no coverage. `getPhaseOutput` is a raw `as T` cast, so a field rename would make the phase detect zero sinks while every test still passed, because zero sinks is what they already assert. The new test asserts the one thing only the success path can produce: a flow ENDING at the sink while a longer chain continues past it. Its control is the same graph with no `parse` dep, which must NOT produce that terminal — without the control the assertion could pass for an unrelated reason. Also asserts `processesPhase.deps` contains `parse`, so the read and the declaration cannot diverge, and that a parse output missing those fields still fails open rather than losing every process. Mutation-checked, including the exact drift scenario reported: renaming `allFetchCalls` at the read site, dropping `parse` from `deps`, and passing no sinks to `processProcesses` each fail exactly the test that claims them. #2895 — a failing bench guard aborted the job and masked every later guard. Every step in the benchmarks job was fail-fast, so the first failing `--check` aborted it and the rest reported `skipped`, which reads identically to "nothing to do". Audited over 13 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. Both guards did in fact pass — that was luck, not verification. `if: ${{ !cancelled() }}` on all ten steps after the first, so one stale baseline reports one red step instead of hiding nine. `!cancelled()` rather than `always()` so an explicit cancel still stops the job instead of running seven minutes of benchmarks nobody is waiting for. The two steps easiest to miss are covered: `Receiver-resolution drop guards`, whose `run:` sits twenty lines below its `name:` behind a long comment, and the final `Cross-language pipeline benchmarks` step, which is not a `--check` and so falls outside any grep for one — and is one of the two that never ran. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(parse): capture a fetch call site even when its URL is not a literal (#2897) The `fetch` rule required the argument to be a string or template literal: arguments: (arguments [(string (string_fragment) @route.url) (template_string) @route.template_url]) so `fetch(url)` with a variable matched nothing at all. Measured across this repository's own TypeScript sources: **44 of 47 fetch calls pass a variable**, so 94% produced no site. That is what makes R3-6 look inert. The sink set is built entirely from `allFetchCalls` / `allORMQueries`, so a function performing an outward call through a computed URL was never a sink, no flow could terminate there, and the sink-first ranking rule never changed an ordering. The feature was fine; the signal underneath it was almost always empty. The URL alternation is now OPTIONAL, so one match covers both shapes. The R3-6 sink set needs only WHERE the program reaches outward, not where to. Route linking is untouched, by construction rather than by hope: `processNextjsFetchRoutes` normalizes the URL first and skips anything that yields nothing, so a URL-less entry cannot mint a FETCHES edge. Verified on this repo — FETCHES went 8 -> 9 across the change, i.e. the widening added sink sites without inventing route edges, which was the one real risk here. Tested in BOTH JavaScript and TypeScript, since the rule is duplicated in each query block and fixing one would have left the other blind: - a variable argument is captured, with no URL <- the regression case - a computed argument (`fetch(buildUrl(), {...})`) likewise - a literal URL is still captured WITH its URL <- route linking depends on it - a template URL likewise - exactly ONE site per call — an optional alternation must not make a literal match twice, which would double-count the site and could mint two edges - `prefetch('/x')` is still not a fetch Mutation-checked: restoring the mandatory alternation fails six of the twelve, three in each language. SCHEMA_BUMP 57 -> 58. Parse-time capture output is replayed verbatim from a warm cache, so without the bump an already-indexed repo keeps its empty sink set and the fix looks inert — which is the failure this constant exists to prevent, and would have reproduced the very symptom being fixed. Not addressed here, and worth stating: this widens `fetch` only. The reporter's broader point stands — anything keyed on FETCHES / QUERIES is only as good as the extraction underneath it, and the ORM side has not been measured. A guard that fails when a corpus known to contain outward calls yields zero sites is the right follow-up; this change makes such a guard meaningful rather than tautological. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(bench): re-baseline receiver-resolution for the two fixtures this PR adds `receiver-resolution --check` failed on: countArm.totalDropsAllKinds: 140 -> 148 countArm.bySiteKind.write: 11 -> 19 Investigated before touching the baseline, because a guard that exists to catch unexplained movement should not be silenced by an unverified story. WHAT IT IS: the count arm runs the real pipeline over a corpus that includes `test/fixtures/lang-resolution/`, and this PR adds two fixtures there — `member-call-producer` (W2-1) and `parameter-producer` (W2-2). Each returns an object literal with two keys, and a producer writing its own returned key is a write site the receiver recorder logs. Four each, eight total. Attributed by dumping the individual drops rather than reading the aggregate: member-call-producer/src/producer.js secretFlag, wickRatio (2 lines) = 4 parameter-producer/src/producer.js source, wickRatio (2 lines) = 4 The eleven drops already in the baseline are all `javascript-object-properties` fixtures of exactly the same shape, so the new ones are not a new KIND of drop — they are more of one the baseline already records. This is the first case the guard's own failure message names: "a fixture was added". WHAT IT IS NOT: `callDrops` — THE gate number, and `call`-only by deliberate design because reads and writes "would inflate it" — is unchanged at 102. `read` drops unchanged at 27. The SHAPE ARM shows no drift at all: no receiver spelling moved between RESOLVES / VISIBLE-GAP / INVISIBLE-GAP, so no resolution regressed. HOW IT WAS ISOLATED, since the first attempt was misleading and the record is worth having: reverting `return-shape-members.ts` alone did NOT reproduce it and pointed away from W2-1/W2-2. Only a commit-level bisect was trustworthy — `origin/main` OK, W2-8 OK, W2-3 OK, then W2-1 +4 and W2-2 +4, which matches the fixture count exactly. A file-level revert leaves the fixtures in the tree, and the fixtures are the cause. The update is two numbers. Nothing else in the baseline moves. Worth noting where this failure became visible at all: under the fail-fast benchmarks job it would have aborted the run and shown the five guards after it as `skipped`. It is legible here because #2895 — fixed in this same PR — now lets every later guard run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(analyze): stop `--pdg` runs reporting a healthy index as INCOMPLETE Every `gitnexus analyze --pdg` reported a graph-write collapse and exited 1 on an index where every row had persisted. Reported by a user hitting it on a real repo; introduced by this PR's own W2-6(d). Repository indexed INCOMPLETELY the pipeline produced 200,501 relationships but only 64,764 are readable The index was complete: 200,190 rows present, 109,905 PDG and the rest structural, all queryable. WHAT WENT WRONG. W2-6(d) made the persisted side count STRUCTURAL rows only — correct, and the reason is in its own comment: PDG writes into the same table, so counting everything let PDG surplus mask real structural loss. But the expected side kept using `graphEmitManifest.totalRows`, and that is a BUFFER-POOL SIZE HINT which counts every streamed row. PDG streams through that same sink, so the check compared a structural-plus-PDG expectation against a structural measurement. On any repo with a PDG layer that is a guaranteed false collapse. It compounds rather than merely misreporting: the run stamps `graph-write-collapsed`, and W2-6(a)'s rebuild trigger — added alongside it — forces a full re-analyze next run, which collapses again. A permanent rebuild loop, on an index that was never damaged, at ~100s a cycle. MEASURED RATHER THAN ASSUMED, because the first attempt was wrong. I first subtracted PDG edges RESIDENT in `graph.relationshipCount`, rebuilt, re-ran the failing command and got byte-identical numbers. Instrumenting the three terms showed why: relationshipCount=20,825 graphManifestTotalRows=179,676 pdgEmitManifest=absent residentPdgInGraph=0 PDG is not resident in the graph AND has no separate manifest — it streams through the ordinary `GraphEmitSink`. The reverted attempt is not in this diff. THE FIX. A pair key cannot separate them: it is `From|To` NODE LABELS, and a CFG edge shares `Function|Function` with CALLS. Only the write path sees `relationship.type`, so the sink now counts a `structuralRows` subtotal there and publishes it on the manifest. `totalRows` is unchanged — it still sizes the buffer pool, which is what it was for. WHY THIS SHIPPED UNCAUGHT, and what changed about that. The wiring test kept a LOCAL MIRROR of the expected-count expression "because the production expression is inline in a 3000-line function". A mirror cannot catch a term the original got wrong. That expression is now an exported `computeExpectedStructuralRelationships` which production calls and the test imports. It also takes the MANIFEST rather than a pre-selected number, deliberately: the defect was choosing the wrong FIELD, and a numeric parameter leaves that choice at a call site no unit test can reach. Verified — with the helper taking a number, reverting to `totalRows` failed nothing; taking the manifest, the same revert fails four tests. Verified end to end on the reported command: `analyze --force --embeddings 0 --pdg` now exits 0 with "indexed successfully", 86,963 nodes / 200,217 edges, and the run clears the stale collapse stamp. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(routes): scope match bindings, and intersect ternary conjunctions Two ways the dispatch-guard walk minted a route that does not exist — the one thing this module's header says is worse than missing one. MATCH BINDINGS WERE KEYED BY BARE NAME, FILE-WIDE. `collectFromMatchBindings` walked from `tree.rootNode` and resolved `matchBindings.get(node.text)` at every identifier in a truthiness position, so a same-named binding in ANOTHER function answered for it. The poison check only fired on a second REGEX match with a different URL; a non-match binding never entered `collectFromRegexDispatch`, so nothing refused it. Reproduced: function handleReplay(req, res) { const m = pathname.match(/^\/api\/live\/positions\/([^/]+)\/replay$/); if (req.method === 'GET' && m) { … } } function handleSettings(req, res) { const m = req.headers['x-mode']; // unrelated value, same name if (req.method === 'DELETE' && m) { … } } GET /api/live/positions/{param1}/replay handler=handleReplay correct DELETE /api/live/positions/{param1}/replay handler=handleSettings FABRICATED Wrong in method, handler and line. `m`, `match`, `result` are the ordinary names here. Two ways the truth was then lost: the fabricated route is VERBED, so `reconcileDispatchGuardRoutes` kept it and dropped the true verb-less one — the #2856 `/api/report` shape, through the channel this series added — and `tested` was name-keyed too, so the tail loop suppressed the real binding's own honest verb-less emit before reconciliation ever ran. `matchBindings` and `tested` are now keyed on (enclosing function, name). `enclosingFunction` is extracted from the walk `enclosingHandlerName` already did, so there is one function-boundary mechanism, not two. A second declarator for a key refuses it, and an assignment refuses the name in its own scope and every enclosing one. `buildRegexConstantMap` refuses a name rebound to anything that is not a regex literal, closing `let RE = /…/; RE = buildDynamic(req)` and the `new RegExp(prefix + '/x')` twin. A use resolves only within its own function. Resolving outward would need a complete declaration model — params, imports, catch bindings — and a miss there fabricates exactly the route this fixes. Declining costs the verb, not the path. THE TERNARY TOOK FIRST-MATCH WHERE THE ALGEBRA IS INTERSECTION. The docblock proves `c ? A : false ≡ c && A` and says "both hold, so search both", but `firstNonEmpty` returned one operand's set unintersected: (req.method === 'GET' || req.method === 'POST') ? (req.method === 'POST' || req.method === 'PUT') : false emitted GET and POST only POST is reachable req.method === 'GET' ? req.method === 'POST' : false emitted GET, unsatisfiable `intersectVerbs` replaces it for both conjunction shapes. An empty side still yields to the other — "names no method" is not "admits none", which is what the `isAdmin && POST` fallthrough is for — but two non-empty sides intersect, and an empty intersection is an unsatisfiable guard that yields no verb. Both changes strictly REMOVE routes, so SCHEMA_BUMP 58 -> 59: routes are parse-time output replayed verbatim from a warm cache, and without the bump an indexed repo keeps serving the fabricated verbed route while the fix looks implemented. 10 tests added, 9 of which fail without the change. All 86 existing assertions pass unchanged; none was weakened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LZu3G5USo5Rs9myaaxVWjK * fix(scope-resolution): bind a type parameter only inside the scope it opened W2-8 captured `@declaration.type-parameters` on EVERY `type_alias_declaration`, but an alias becomes a SCOPE only when its value is an `object_type` (`typescript/query.ts:149`). For a union, array, conditional, mapped, tuple or function alias there is no scope, so the def — now carrying `typeParameters` — attached to the innermost enclosing scope, which is the MODULE. And `typeParameterNamesInScope` folds each scope's set from its PARENT'S, so the name landed in every scope in the file. The `USES` guard then deleted every edge whose target had that simple name: export interface Result { ok: boolean } export type Maybe<Result> = Result | null // one ordinary line export function readResult(r: Result) { … } // its USES edge is DELETED Silent data loss, in the edge class whose whole purpose is answering "what breaks if I remove this field?". Measured: adding two scope-less generic aliases emptied the fixture of USES edges entirely. The existing fixture could not see it — it wrote `type Box<Result> = { held: Result }`, the ONE alias form that opens a scope. `typeParameterNamesInScope` now reads a def's `typeParameters` only when that declaration OPENED the scope owning it: `scope.kind !== 'Module'` and the def-id position equals the scope range start, via the canonical `definitionIdPosition` rather than slicing the id. That is the same alignment test `pickCallerCallableDef` uses to tell a closure from a nested function, and it is language-neutral — it also covers `function f() { type W<Result> = Result[] }`, which a module-scope-only stopgap would miss. Every language populating the capture was audited (ts, java, csharp, kotlin, rust, cpp): all anchor it on a declaration that IS a scope node, including C++ where the capture rides `template_declaration` but the anchor is the inner `class_specifier`. Go uses a separate sidecar. The TypeScript non-object alias was the only mismatch in the codebase. `query.ts` is untouched. THE GUARD ALSO SAT AT THE WRONG LAYER, which forced three defects at once. It keyed on `edgeType === 'USES'` — and `mapReferenceKindToEdgeType` maps THREE kinds there, `type-reference`, `value-ref` (#2437) and `macro` (#1934) — and, because `Reference` carries no spelled name, substituted the resolved def's name via `simpleNameOfDefId`. So `import { Result as ApiResult }` inside `function unwrap<Result>()` deleted a REAL edge, while a namespace-qualified target (`Host.Result`) kept a FALSE one, and a positional `@row:col` suffix broke the last-colon parse outright. Moved to `lookupForSite`'s `case 'type-reference'` in `resolve-references.ts`, which has the spelled `site.name` and the reference kind in hand. One line closes all three, deletes `simpleNameOfDefId` — a byte-identical duplicate of `simpleNameOfGraphId` — and removes the only `graph-bridge/` -> `scope/` import in that directory. Honest scope: all three sub-defects are real in the code but none is observable end-to-end today (`value-ref` never reaches this path; TypeScript emits no cross-file USES for a type annotation at all — a separate pre-existing gap). Those arms are labelled forward guards in the test rather than claimed as repros. Fixture grows 1 file -> 4; 3 of 9 assertions fail without the change. The scope-capture TypeScript fingerprint moves for FIXTURE-CORPUS GROWTH ONLY, with per-file accounting that sums to the delta and JavaScript unchanged as the control — see the `_rebaselined_` key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LZu3G5USo5Rs9myaaxVWjK * fix(scope-resolution): refuse an ambiguous formal, stop the walk at the nearest binding Two ways W2-2 typed a parameter from the wrong caller, both at the PRECISE 0.9 tier — above every `minConfidence` floor, so nothing downstream can filter them. `formals` WAS LAST-WRITE-WINS. The key is (filePath, ownerName, parameterIndex), `ownerName` is a bare identifier, and `emitFormalFacts` emits one site per parameter of EVERY function collected, nested functions and class methods included. A plain `.set` let two same-named callables in one file collide — a free `parse` and a nested `parse`, a free `apply` and `Runner.apply` — so the last one visited won, fabricating an edge on the loser and leaving the genuine consumer untyped. The file's own comment covers only the cross-FILE axis. The correct shape was thirty lines below, in the `producers` map, which does `producers.delete(cell); conflicted.add(cell)`. `formals` now refuses the same way: a key claimed by two DIFFERENT parameters is deleted and recorded, so a third same-named formal cannot re-claim it. Re-stating the same cell is not a disagreement, so a benign duplicate capture cannot poison a real key. THE SCOPE WALK CLIMBED PAST A NEARER BINDING. The docblock claimed it stops at the first scope carrying the name, but it consulted only `parameterProducers` — a shadowing `const`, a catch binding or an arrow parameter is not in that map, so the walk went straight past it to the enclosing formal: function readSpike(spike) { … items.map((spike) => spike.wickRatio) … } typed the ARRAY ELEMENT from the outer parameter. `parameterProducerFor` now stops at the first scope that binds the name AT ALL — reading the scope's own tables, the same channels and the same reasoning as the sibling `isNamespaceNameShadowed` — and then stops at a Function boundary. That boundary is what covers the anonymous arrow: `collectFunctions` drops a callable it cannot name, so an anonymous arrow emits no formal site and its scope looks empty while in fact rebinding the name. The cost — a closure genuinely reading an enclosing parameter now declines — is documented as the deliberate trade. No cycle guard, deliberately and with the reason stated: both constructions of `indexes.scopeTree` validate through `buildScopeTree`, which enforces strict parent-contains-child ranges, so a cycle needs a scope strictly containing itself. A per-site Set on every read/write site in the repo would defend against a state the builder rejects. 5 fixtures, 5 assertions; 4 fail without the change and the control passes both ways. Still uncovered and not faked: a `for (const x of …)` binder shadow — the binder lives in the loop header, so JS emits no scope to stop at and no Function boundary intervenes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LZu3G5USo5Rs9myaaxVWjK * fix(analyze): measure one population in every config, split the stamp on the verdict `1b41c9df6` fixed the collapse check for the STREAMED configuration by giving the sink a `structuralRows` subtotal. It does not cover the other one. `resolveStreamGraphEmit` and `resolveStreamPdgEmit` both open with a `force === true` gate, so a run without `--force` streams nothing: there is no manifest, `structuralRows ?? 0` contributes 0, and `scope-resolution/pipeline/run.ts:1222` (`input.pdgEmitSink ?? graph`) writes PDG into the ordinary in-memory graph, where `relationshipCount` counts it. And `isIncremental` requires an existing meta, so a FIRST run is a full write and the check runs. A first-time `gitnexus analyze --pdg` on a fresh repo therefore compared structural+PDG against structural and exited non-zero with "Repository indexed INCOMPLETELY" on a healthy index. MEASURED, not assumed — `runScopeResolution({ pdg: true })` with no sink: pdgEmitSink = absent (non-force shape) relationshipCount = 1 residentPdgInGraph = 1 byType = [["CFG",1]] The prior `residentPdgInGraph=0` was taken on a `--force` run, where `input.graph` IS the sink; it never spoke to this case. `graph-collapse-wiring.test.ts` had pinned the gap, asserting a PDG-inclusive in-memory count was a valid structural expectation. `countStructuralRelationships(graph)` filters `PDG_EDGE_TYPES` over `forEachRelationshipFields` — the same predicate the sink uses for `structuralRows` and the adapter for `structuralEdges` — so all three terms measure one population in every configuration. Declining whenever `pdg && !streaming` was rejected: that is the DEFAULT PDG shape, so the guard would be off for every non-force run including the only full write most users ever do. An unscannable graph (mocked pipelines) yields NaN, the same fact the old `undefined + rows` produced and one `detectGraphWriteCollapse` already documents as expected input. THE THREE-WAY STAMP WAS A TWO-WAY. The comment enumerated collapse -> stamp, healthy -> clear, no verdict -> carry forward, but the code split on the WRITE MODE. `graphWriteCollapsed` is undefined for two different reasons, and one of them is "the structural query threw" — so on a full run where the count could not be READ, the code took "healthy, clear it" and erased a stamp recording real edge loss. Run 3 then printed "Already up to date" forever: the exact failure the comment says it fixed, reachable through the new code's own `catch {}`. `detectGraphWriteCollapse` now returns `'collapsed' | 'healthy' | 'unmeasurable'` with a reason, and `selectPersistedCollapseStamp` is a pure exported function production calls. Two boundaries worth naming: `expected === 0` is unmeasurable (its own docstring calls it "could not report a total"), but the small-repo exemption and a cleared ratio are HEALTHY — both counts were taken. Making the exemption a non-verdict would leave a stamp unclearable on any repo that shrank below 100 edges, relocating the wedge rather than fixing it. `getLbugStats` now reports `structuralEdgesError` and warns, and `run-analyze` falls back to `stats.edges` only when the run had no PDG layer, where the two are equal by construction. With `--pdg` on there is no substitute, so the absence becomes an explicit unmeasurable verdict — which preserves the stamp. 13 tests added; 8 fail without the change. The integration suite now seeds a CFG row and asserts `edges` moves while `structuralEdges` does not — the exclusion filter was previously unexercised, its own comment conceding "structural == total here". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LZu3G5USo5Rs9myaaxVWjK * fix(server): check the collapse before publishing the index W2-6 marked a collapsed run's job `failed`, but the check ran INSIDE `.then(() => backend.init())` — after the publish. `LocalBackend.init()` is the publish step: it refreshes the registry and atomically swaps the in-memory repo map every MCP tool and HTTP route resolves through, and its `validate` pass prunes only entries whose metadata is provably gone, so it can publish but never quarantine. The known-incomplete database was therefore live and queryable before the job was ever marked failed — the job status was a label on a published index, not a gate. The pre-existing comment two lines above says so outright: "the repo is actually queryable when the client receives the SSE complete event." `backend-client.ts` routes `failed` to `onError` and never calls `onComplete`, so the UI showed an error toast while every query against that repo answered from the incomplete graph — precisely the confident-wrong-answers failure this guard exists to prevent. The collapse branch now returns before publishing; the healthy path publishes via a nested `backend.init()` so the trailing `.catch` still converts init failures into the same message. `closeDbHandle()` runs on both paths — it is eviction, not publication, and the worker rewrote the DB files regardless of outcome, so skipping it would leave a stale pre-rewrite handle. Honest limit, stated in the error string rather than overclaimed: this keeps a FIRST-TIME analyze unpublished, which is the UI's main flow. On re-analysis of an already-published repo the existing map entry survives and points at the same storagePath. A real quarantine needs an un-register hook on `LocalBackend`, which does not exist today — follow-up. `'partial'` was considered and rejected on evidence: it is not a status. It is an embedding-specific detail object in the `updateJob` allowlist; the status union excludes it. Adding it would make `isTerminalJobStatus` false, so `sse-progress` never writes a terminal frame and never calls `res.end()` — the stream hangs open — while `backend-client` falls through to `onMessage` and `api.ts` spins the full hold-queue timeout. `failed` at least terminates. The failure branch also now sets `repoName`, which only the success path did. First tests this file has ever had: 4, of which 2 fail without the change. They assert the ORDERING, not just the final status, and build the worker message by calling the production `projectAnalyzeResultForIpc` so a field rename breaks the test instead of silently disabling the branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LZu3G5USo5Rs9myaaxVWjK * fix(processes): count the entry-point cap, and make the disclosure proportionate W2-3 added a truncation disclosure and then missed the largest ceiling it was written to report. `findEntryPoints` ends `.slice(0, 200)` and `entryPointsUnexplored` counted against the POST-slice list, so candidates 201..N were invisible — while the derivation docblock claimed "a new ceiling added later cannot be forgotten here". An existing one was. On this repo's own corpus the new counter reads 780 of 980 candidates never ranked in. `entryPointCandidatesDropped` reports the pre-slice count, folded into `truncated`, with `ENTRY_POINT_CANDIDATE_LIMIT` extracted and `findEntryPoints` taking the same optional out-parameter `traceFromEntryPoint` already uses. Its return contract is unchanged. THE WARN FIRED ON EVERY RUN. At the shipped defaults — only `maxProcesses` is overridden — `calleesDropped` fires for any function with 5+ callees and `tracesDepthCapped` for any chain deeper than 10, so an ungated `logger.warn` was constant background noise, and a warning that always fires is one nobody reads. The split is the module's own, from the `ProcessTruncationStats` docblock: "unexplored entry points mean whole flows are missing, while a depth-capped trace means a flow is present but shorter than it really is." So `warn` iff whole flows are absent — candidates dropped, entry points never traced, or flows dropped at `maxProcesses` — and `debug` for a run truncated only in depth or breadth. `stats.truncation` still carries all six counters; the machine-readable channel is unchanged, only the log level moves. `entryPointCandidatesDropped` stays in the loud set deliberately: it is the only ceiling that GROWS with repo size, while the other two can only fire while `maxProcesses` is small enough to bind, so gating on those alone would go silent on exactly the large repos where 200-of-several-thousand is the thinnest sample. The message leads with the ratio so the line carries a fact, not an alarm. THREE COMPARATORS ALLOCATED PER COMPARISON, in the function whose own comment explains the hoist that removed this shape (`deep_chain` 1233 -> 102 ms). Measured here: +99 ms once per analyze at 80k functions — small, because `n` is capped at 200 entry points x a 12-trace budget = 2,400 traces regardless of repo size. Worth fixing anyway: 23,851 comparisons cost 70,524 joins. One shared `sortByDepthThenPath` (Schwartzian, key built once per trace) now serves all three sites, and `rankedByInterest` additionally hoists the `isSink` test that ran twice per comparison. It also settles a separator inconsistency: `deduplicateByEndpoints` joined on a SPACE while `traceOrderKey` used NUL, and node ids embed file paths, so two different traces could produce the same key and the tiebreak fell back to the insertion order it exists to remove — the same hazard this series' own `->`-padding fix addresses. Order identity is pinned by a seeded 200-trace corpus asserting the new sort equals the old one exactly. 11 tests added, 9 failing without the change, including two end-to-end insertion-order arms. The W2-5 determinism block is unregressed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LZu3G5USo5Rs9myaaxVWjK * fix(docs): restore the agent guidance, and put it in the generator that deleted it Commit `9e602aef0` — whose message is entirely about the fetch capture — also regenerated the machine-managed `<!-- gitnexus:start -->` block from a local non-`--pdg` index, deleting from both AGENTS.md and CLAUDE.md: - the whole `MUST treat risk: UNKNOWN as unresolved, not as low` bullet - the `pdg_query({mode:"controls"/"flows"})` bullet - the `mode: "pdg"` text on the impact bullet - `…never read UNKNOWN as an all-clear…` from Never Do and regressing the stats 248612/565510/918 -> 42853/135955/758. All four document SHIPPED features: `pdg_query` at `mcp/tools.ts:675`, dispatched at `local-backend.ts:2233`; `mode: "pdg"` at `tools.ts:448`; `riskNote` at eight sites. It matters more than a docs nit because the SAME series makes `UNKNOWN` dominate a mixed candidate set (`local-backend.ts:6058`) — correct, and it makes UNKNOWN far more common. The surviving rule only warns on HIGH/CRITICAL, so a set measuring CRITICAL now reports UNKNOWN and that rule no longer fires, while the rule that covered the gap was deleted in the same commit range, from all three files agents actually read. ROOT CAUSE, which is why restoring the files alone would not have held. `cli/ai-context.ts` is the template. The `pdg_query` and `mode: "pdg"` text IS in it, correctly `hasPdg`-gated — a non-PDG analyze SHOULD drop those. The `risk: UNKNOWN` rules were never in the template at all: they had been hand-added INSIDE the machine-managed region, so every `gitnexus analyze` on any repo silently deleted them. This was the second occurrence; #2856's `8f8261021` was the first. Both lines are now generated unconditionally — they describe impact's risk semantics, which are not PDG-dependent — so regeneration restores them instead of removing them. AGENTS.md and CLAUDE.md are byte-identical to origin/main again, and the fixed template reproduces that block exactly for `hasPdg: true` plus the real stats. `.claude/skills/gitnexus-guide/SKILL.md` regains the "Inline staleness signal" section for a live feature (`local-backend.ts:921`, `:1017-1024`, `:1995`); the npm mirror's lack of it is pre-existing drift and is left alone, so the new sync guard is scoped to the canonical and plugin copies. Guards added, both demonstrated failing against the unrestored files: the managed block must contain the UNKNOWN policy and its Always-Do/Never-Do bullet counts must not fall below a floor, and `generateGitNexusContent` must render both lines for `hasPdg` true AND false while keeping `pdg_query` gated. The existing fragment lists could never have caught this — they assert presence, and this was a deletion. One deliberate loosening, called out rather than buried: the restored text pushes `ai-context.test.ts`'s block-size ratio past 0.55, so it moves to 0.65. That test argues against exactly this nudge-the-number pattern. The defence is that the wording is origin/main's own and the 0.55 budget was calibrated against a block already missing it; trimming shipped guidance to fit a budget would be the wrong direction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LZu3G5USo5Rs9myaaxVWjK --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>	2026-08-09 11:44:52 +01:00
Gergő Magyar	010a7d806a	fix(schema): declare the full scope-resolution relation cross product (#2792) (#2793) ... * fix(schema): declare the full scope-resolution relation cross product (#2792) `RELATION_SCHEMA` was hand-listed, and every prior fix added only the FROM/TO pair named in a crash report — `Const→Method` in #2769, the Swift/Rust member pairs before it. So `analyze` kept aborting at `assertDeclaredPair` on the next codebase whose edges happened to land on a different pair; #2792 reports `Class→Variable` on Java. Audit the surface instead of the symptom. `buildGraphNodeLookup` skips any node whose label is not in `isLinkableLabel`, so the lookup holds only linkable-labelled nodes — and both endpoints of every graph-bridge edge resolve through that lookup. The emittable surface is therefore exactly: FROM LINKABLE_LABELS + File (the module-level caller fallback) TO LINKABLE_LABELS + CALL_TARGET_TYPES `isCallerAnchorLabel` is a strict subset of linkable and contributes nothing on top. `CALL_TARGET_TYPES` contributes `Delegate`, which `tryEmitEdgeWithExplicitTargetId` can emit without going through the lookup at all. Generate that 14x14 block into the DDL rather than listing it: 223 -> 322 declared pairs, and no future pair from these sets can be missing by construction. The containment/inheritance/DI/route/cluster/PDG pairs stay hand-declared — no single predicate describes them. Both label sets live in the ingestion layer, which `core/lbug` must not import, so schema.ts carries twin lists. test/unit/schema-pair-coverage.ts derives the requirement from the originals and fails CI when either set grows without the pairs landing here — the piecemeal loop this fix ends. Measured before widening: at 322 pairs the cost is inside noise (1.09s vs 1.12s per 300 anchored queries on a 32-table DB), but the full 32x32 cross product is ~1.8x on untyped-endpoint anchored queries. The audited subset is the right scope, not "declare everything". INCREMENTAL_SCHEMA_VERSION 34 -> 35: LadybugDB fixes endpoint pairs when the rel table is created, so a pre-v35 database physically cannot store these edges. Closes #2792 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(schema): declare the non-bridge structural pairs COBOL and Vue emit The generated scope-resolution block closed the half of RELATION_SCHEMA a label predicate can describe. The hand-declared half was still stale: with #2791's Function->Variable fix applied, `analyze` continued to abort on this repo's own test/fixtures/lang-resolution with Relationship label pair Module→Property is not declared A full sweep (assertDeclaredPair patched to log-and-skip, run over the whole fixture corpus) found 13 undeclared pairs over 106 edges. This branch already covered 3 of them via the cross product; the remaining 10 come from emitters outside the graph bridge: - cobol-processor.ts mints Module / Namespace / Record / Property / CodeElement and wires them with CONTAINS, CALLS and ACCESSES (9 pairs) - vue-sfc-extractor.ts emits BINDS_EVENT_HANDLER from a handler Function to the child component's File, the only edge whose target is a File (1 pair) CodeElement, Namespace, Record and File are in neither scope-bridge label set, so neither the generated block nor schema-pair-coverage.test.ts can reach them. Adds test/integration/structural-pair-coverage.test.ts, which derives the requirement from a corpus instead of a predicate: it runs the real pipeline over the non-bridge fixtures and requires every FROM/TO pair they produce to be declared. Mutation-checked — dropping `FROM Function TO File` fails it with exactly Function|File. Verified: cobol-app, vue-basic and php-transitive-traits now index instead of aborting; the full lang-resolution corpus completes at 10,876 nodes / 18,517 edges; scrypster/muninndb at 0b7a4272 (the #2789 repro) completes at 20,069 nodes / 71,580 edges, matching #2791 exactly, so this supersedes that PR. * refactor(test): simplify the structural pair coverage guard Cleanup pass over the previous commit. No behaviour change to the schema. - reuse `FIXTURES` and `runPipelineFromRepo` from resolvers/helpers.ts instead of re-deriving the fixture root and importing pipeline.js directly - gate on `distWorkerExists()` like every other integration test that passes `workerUrlForTest`, so a missing dist skips rather than fails - run the three fixtures with `it.concurrent.each`; they share nothing and the cost is almost all worker spawn plus grammar load, which overlaps well (tests phase 21-24s -> 5.6s measured) - replace the sentinel-in-a-Set filter with a plain `.filter()` chain, matching the sibling unit test, and move the declared/table lookups off the per-edge path onto the deduped set - move the pure string pin out of the integration tier into schema-pair-coverage.test.ts, where the identical construct already lives, so it needs no build and survives fixture deletion - trim the schema and test prose that restated the code, and correct the BINDS_EVENT_HANDLER attribution: it is emitted by languages/vue/scope-resolver.ts, not vue-sfc-extractor.ts - amend the v35 comment to mention the 10 structural pairs it now also stamps Still mutation-checked: dropping `FROM Function TO File` now fails both the integration sweep and the unit pin with exactly Function|File. 89 tests green. * fix(schema): generate the attachment pair surface and close four analyze aborts Review of the generated scope-bridge cross product found four `analyze` hard-aborts still live at head, each reproduced end-to-end on the default user path (`analyze --index-only --skip-git`): Method→Annotation Spring `@Bean` + `@ConditionalOnMissingBean` (Java + Kotlin) Method→File Vue Options-API `methods:` handler bound to a child event Namespace→Record COBOL `DECLARATIVES` / `USE AFTER STANDARD ERROR ON <file>` Class→Tool `@mcp.tool()` applied to a class All four are pre-existing on main, and both existing guards were structurally blind to them: the unit guard derives from LINKABLE_LABELS ∪ CALL_TARGET_TYPES (none of Annotation/Tool/Record/File-as-target is a member) and the corpus guard ran three fixtures that exercise none of these emitters. All 16 tests passed while all four crashes were live. The PR's model — "bridge endpoint × structural endpoint" — does not fit: Namespace→Record is structural on both sides. The property that does hold is that the ANCHOR is a lookup result, not a literal at the emit site, so the emitter cannot constrain its label. That gives a second closed-form rule: DEFINITION_ANCHOR_LABELS × ATTACHMENT_TARGET_LABELS DEFINITION_ANCHOR_LABELS is derived from NODE_TABLES by subtraction, so a new node table joins automatically. 332 → 450 declared pairs. Sized against a committed harness (gitnexus/bench/schema-pairs), real @ladybugdb/core, identical data: 450 costs 0.93–1.05× of 332 on untyped-endpoint anchored queries — inside noise — versus 1.22–1.43× at 641 and 2.03–2.34× at 1024. The harness reproduces the known #2792 cliff, which is what makes the 450 figure trustworthy. Also in this change: - Delete the 161 hand-declared pairs the rules already generate (233 → 72). The declared set is byte-identical at 450; those lines were load-bearing shadow, because the generator suppresses anything already declared structurally, so narrowing a rule later would silently keep pairs alive. A new guard fails CI if a hand-declared pair is ever re-added inside a rule. - Import LINKABLE_LABELS / CALL_TARGET_TYPES instead of hand-copying them. The twins' stated justification ("the ingestion layer must not be imported here") is false: csv-generator.ts and lbug-adapter.ts, siblings in the same directory, already do, and no rule in AGENTS.md / ARCHITECTURE.md / CONTRIBUTING.md / GUARDRAILS.md states otherwise. - Resolve `resolveStreamGraphEmit` after the guards that rebind `options.force`, not at function entry. It gates on `force`, and every freshness guard runs ~360 lines later, so the v34→v35 bump would have pushed every existing index down the non-streamed emit path — losing the #2680 memory streaming added for the #2649 kernel-scale OOM, for exactly the population most likely to be memory-constrained. - `UndeclaredRelationPairError` now carries the relationship type, both node ids and the source file, with a matching CLI branch. The old message named only the abstract label pair, which a user could not act on. Found through the cause chain, since pipeline-phases/runner.ts rewraps every phase failure. - Share one classifier (`relPairKeyFor`) across the router, both emit sinks and the corpus guard, which previously hand-mirrored the router's skip rule; one cause-chain walker in lib/utils.ts; one exported pair-matching regex. - Corpus guard: four new fixtures reproducing the aborts, per-fixture sentinel pairs so a fixture that stops emitting fails loudly instead of passing vacuously on an empty graph. The per-edge path stays allocation-free: the failure context is passed positionally and the message is built only inside the throw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0182jkjQqzACkJKYw4MLDnhX * test(bench): re-baseline the COBOL capture fingerprint for the new fixture `bench/scope-capture` globs `lang-resolution/cobol-*`, so the `cobol-declaratives` fixture added in 81daf370e (to reproduce the `Namespace→Record` analyze abort) joined that corpus and shifted the fingerprint — 14 → 15 files. Verified corpus-only, not a capture change: with that one fixture moved aside the fingerprint is byte-identical to the prior baseline (d45bb091…), and 81daf370e touches no COBOL capture code. The new value reproduces CI's reported hash exactly. Scaling 0.677 < 1.5 budget. `bench/scope-capture/measure.mjs --check` → PASS (15 languages). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0182jkjQqzACkJKYw4MLDnhX --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-02 16:26:25 +01:00
Karl Lehenbauer	bebb1d2367	fix(schema): declare Swift member-containment pairs in CONTAINS DDL (#2769) ... * fix(schema): declare Swift member-containment pairs in CONTAINS DDL * fix(schema): declare remaining Rust impl/trait and JS/TS object-literal HAS_METHOD pairs; guard streamed emit sinks against undeclared pairs (PR #2769 review) * refactor(schema): share one declared-pairs constant across router and sinks DECLARED_REL_PAIRS was being computed independently in three places (csv-generator.ts, graph-emit-sink.ts, pdg-emit-sink.ts) from the same static RELATION_SCHEMA parse. Export the existing constant from csv-generator.ts (already imported by both sinks) instead. assertDeclaredPair now takes the pre-built pairKey rather than the two labels, since every caller (RelPairRouter.route, both sinks' addRelationship) needs that same key immediately after for its own Map/stream lookup on the per-streamed-edge hot path — avoids rebuilding the template string twice per edge. Also drops two schema.test.ts assertions that duplicated coverage already in the more narrowly-named regression tests below them, and trims the v32 ladder comment to point at assertDeclaredPair's docstring instead of re-explaining the same failure mechanism. * fix(schema): use replaceAll for the pair-arrow error message (CodeQL) .replace(str, ...) only touches the first match; CodeQL flags that as incomplete string escaping regardless of the caller's invariant that pairKey contains exactly one '|'. replaceAll is equivalent here and silences the alert. --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>	2026-08-01 12:35:37 +01:00
Gergő Magyar	7316503ebc	perf(analyze): hold structural relationships out of the JS heap, on by default (#2680) (#2685) ... * refactor(lbug): extract SyncCsvWriter into a shared module `PdgEmitSink` (#2202) declared `SyncCsvWriter` as a private, non-exported class. The structural streaming sink for #2680 needs the same buffered sync-write + poison/openFailure IO discipline, and importing it is not possible while it is module-private — so the alternative was copying ~90 lines of it. Extract the class (and the chunk-rows default it uses) into `sync-csv-writer.ts` and have `PdgEmitSink` import it. `DEFAULT_PDG_EMIT_CHUNK_ROWS` stays exported as an alias so no existing caller changes. Pure refactor: no behaviour change. pdg-emit-sink.ts 396 -> 302 lines; tsc clean; the 23 existing #2202 tests pass unchanged. Refs #2680 * feat(lbug): add GraphEmitSink for streaming structural relationship emit Structural sibling of PdgEmitSink (#2202): a KnowledgeGraph façade that routes relationships no mid-pipeline phase reads back to bounded CSV-on-disk and never stores them. Nothing constructs it yet. Measurement drove the design. On a kernel-shaped synthetic graph (400k nodes, 2.7 edges/node): nodes only ...... 367 B/node nodes + edges ... 2075 B/node <- reproduces the #2649 ~2.1 KB/node => the relationship layer is 83% of graph heap, ~646 B/edge so streaming *relationships* is where the memory is; nodes stay resident (they are 17%, and two scope-resolution index builders scan them). Dropping just the redundant relationshipsByType/edgeIdsByNode indexes was also measured — 174 of 648 B/edge, ~1.3x — and is not a substitute. RETAINED_REL_TYPES is derived from an exhaustive audit of every relationship read site under src/, and each entry names its reader. An earlier draft carried 14 types, 5 of which no reachable phase reads. Two deliberate departures from PdgEmitSink, both because its invariants do not hold here: - dedup by relationship id, since no upstream per-file uniqueness guarantee exists for structural edges and COPY would violate the PK; - removeRelationship on an already-streamed id throws instead of no-oping, so a mutating consumer cannot corrupt the graph undetected. Also exposes hasStreamedSemanticEdge for the local-symbol pruner: without it a block-local symbol referenced only by a streamed edge looks unreferenced and gets pruned, leaving a CSV row pointing at a node with no row. Refs #2680 * feat(analyze): stream structural relationships to CSV under GITNEXUS_STREAM_GRAPH_EMIT Wires GraphEmitSink into the pipeline behind a full-rebuild-only flag, so relationships that no mid-pipeline phase reads back never enter the JS heap. Measured ~2.9x reduction of graph heap: 0.17 (nodes) + 0.83 * 0.21 (retained edges) = 0.344 retained. This is a constant factor, NOT O(chunk) — node identity and the resolution registries stay O(repo). The sink is armed at the PARSE boundary, not at graph construction. An exhaustive audit of every relationship read site under src/ found four mid-pipeline CALLS consumers, not the two an earlier draft assumed: - local-symbol-pruner (full iterRelationships scan, then removeNode) - communities / processes (whole-graph forEachRelationship) - mapCobolToGraph, which scans CALLS and REMOVES the unresolved ones — and runs BEFORE parse, so streaming from construction would have silently stopped COBOL cross-program call resolution - taintSummaries, gated on `pdg` and NOT on `skipGraphPhases`, so it needs its own gate or --pdg + this flag yields an empty taint layer Accordingly communities, processes, taintSummaries and callSummaries are all disabled under the flag, and the run logs what it is giving up. Two fixes that are correct independently of the flag: - runPipelineFromRepo keyed its community/process extraction off `!skipGraphPhases` while getPhaseOutput THROWS on a phase filtered out by any enabledWhen predicate — now a presence check, so filtered combinations return undefined instead of crashing. - loadGraphToLbug COPYs one job per CSV FILE rather than per label pair. #2202's throw-on-collision merge is only sound because BasicBlock pairs are disjoint; a streamed CALLS edge is Function|Function and always collides with the whole-graph CSV for that pair, so the structural manifest appends instead. The buffer-pool hint adds the streamed row count back in: the hint only ever shrinks the pool, so sizing it from the post-streaming relationshipCount would starve the COPY at exactly the scale this targets. detect_changes: 18 symbols / 10 files / 9 processes, all within the planned scope. Full suite green with the flag off. Refs #2680 * fix(mcp): stop impact() under-reporting risk on a streamed index An index built with streamed structural emit has no Process or Community rows, and impact()'s risk scorer uses processCount >= 5 and moduleCount >= 5 as two of its four CRITICAL escalation criteria. The missing-table errors are swallowed as benign without raising `partial`, so nothing distinguished 'this repo has no processes' from 'this index was built without them' — the same change would report LOW off a streamed index and CRITICAL off a complete one, with no signal either way. That is the false-clean shape #2283 ruled out for detect_changes, and it matters more here because the repo's own workflow mandates impact() before every symbol edit. Stamp `graphPhases: 'complete' | 'skipped'` into RepoMeta and have impact() attach riskUnderstated + an explanatory riskNote when the index is stamped skipped, so the reported level is explicitly a lower bound. Unlike the rest of RepoMeta.capabilities this stamp has a real programmatic reader. Also documents GITNEXUS_STREAM_GRAPH_EMIT in the README env table, including everything the flag disables. Refs #2680 * test(lbug): differential set-identity gate for streamed structural emit The acceptance property for #2680: for the same node/edge set, the rows reaching the bulk COPY must be identical whether streaming is on or off. With streaming on they arrive from two places — the residual in-memory graph via streamAllCSVsToDisk, plus the sink's per-pair CSVs — so the test asserts their UNION equals the single whole-graph emit. Also asserts the split is real (retained + streamed == total, streamed > 0), so a sink that silently streamed nothing cannot pass the equality vacuously. Verified discriminating: with sink.arm() commented out the test fails ('expected 0 to be greater than 0'); restored, it passes. Fixture spans both sides of RETAINED_REL_TYPES and includes a self-edge and a duplicate relationship id — the cases where a naive sink diverges from the whole-graph emit. Drives the sink directly rather than running analyze, matching pdg-emit-streaming-roundtrip.test.ts: the guarantee is about emitted rows, and the worker pool would add unrelated machinery without strengthening the assertion. Refs #2680 * fix(test): remove literal NUL byte and cover streamGraphEmit phase gating Two review findings, both verified before accepting. 1. The round-trip test contained a literal NUL byte as a key separator, which made Git treat the whole .ts file as BINARY — `git show --numstat` reported `-\t-` for it, so the file would not diff or blame and CI text tooling would skip it. Replaced with the escaped \\u0000 sequence; behaviour is identical, the file is text again. (Found by the Codex swarm lane.) 2. buildPhaseList's four new streamGraphEmit gating predicates and the flag-off default path had no test that would fail on revert — two review lanes flagged this independently. Reversing any enabledWhen condition would have passed the suite silently, which matters because an ungated taintSummaries yields an empty taint layer rather than an error. Added four cases: the streamed run drops communities/processes/ taintSummaries/callSummaries; it keeps mro/di (their reads are all in RETAINED_REL_TYPES); the flag-off list is untouched; and skipGraphPhases still works independently. Refs #2680 * fix(analyze): don't leak a temp dir when streaming is off; correct two overclaims Three review findings, all verified before accepting. 1. `graphEmitCsvDir: resolveNativeSafeStorageDir(...)` was evaluated unconditionally inside the pipeline-options literal. On a Windows non-ASCII storage path that helper mkdtempSyncs a REAL directory, so every analyze leaked one temp dir even with the flag off. Now resolved only when streaming is active, matching how the PDG sibling resolves inside its own guard. This was the only finding affecting flag-off users. 2. The retain-set comment claimed 'the differential round-trip test is what catches drift'. It cannot. addRelationship PARTITIONS edges between the graph and the CSVs, and the union of a partition is invariant under where the partition line falls — so that test stays green no matter how RETAINED_REL_TYPES is drawn. Only the read-site audit protects the invariant, and the comment now says so and names the grep to re-run. 3. The ~2.9x figure assigned streamed edges a retained cost of zero, ignoring the sink's own streamedIds/streamedEndpoints Sets — and relationship ids are plain concatenations of both endpoint ids, not hashes. Review measured those Sets at ~35% of full per-edge retention, not the '~a tenth' assumed, putting the real figure nearer ~1.7-2.2x; a member-dense Java/C# repo lands lower still, since the retained structural spine is a larger share there than in the TypeScript census the 0.21 came from. Code comment and README now give a range and say plainly that no end-to-end measurement on a real repository exists yet. Refs #2680 * fix(mcp): disclose degraded risk in detect_changes; stop pinning the sink Two more review findings, both cross-lane corroborated. 1. detect_changes derives risk_level SOLELY from affected-process count, and a graphPhases:'skipped' index has zero Process rows by construction. The STEP_IN_PROCESS query then succeeds with zero rows, so queryDegraded stays false and the tool returns risk_level 'low', affected_count 0, with no partial marker — for every change, forever. That is a false-clean on the gate this repo mandates before every commit, and it is the same #2283 shape the previous commit fixed in impact() while leaving its sibling untouched. Now carries the same riskUnderstated + riskNote disclosure. 2. PipelineResult.graphEmitSink had zero readers — the pruner predicate and the manifest are both threaded elsewhere — but returning it kept the sink, and therefore its O(streamed-edges) id and endpoint Sets, reachable through the entire COPY/FTS/embedding phase. That is precisely the phase this feature exists to fit inside RAM, so the field actively worked against the change's purpose. Dropped. Refs #2680 * refactor(2680): one named capability, one risk helper, a shorter header Pure cleanup pass — no behaviour change, 66 tests across the six affected suites still green, and the round-trip test still fails when the sink is left un-started. Three things were untidy: 1. The phase layer reached the sink through TWO loose callbacks bolted onto PipelineContext (`armStreaming`, `hasStreamedSemanticEdge`) — two fields, two wiring lines, no name for the thing they belonged to. Replaced by one `graphEmit?: GraphEmitControl`, a two-method interface declared beside the sink. Phases now say what they mean: `ctx.graphEmit?.beginStreaming()`. Also renames `arm()` to `beginStreaming()`, which needs no comment to explain. 2. The degraded-index risk disclosure was copy-pasted into impact() and detect_changes() — two meta probes, two near-identical prose blocks, and two long comments restating the same reasoning. Now one `streamedIndexRiskDisclosure()` helper carrying the explanation once; each caller passes only the clause naming which count is structurally zero for it. Same file, 45 lines in / 45 out, with the duplication gone. 3. The sink's file header had grown into a changelog of my own review corrections ('this once assumed', 'review measured'). A reader does not care what an earlier draft believed. Rewritten to state the design argument once — relationships are ~83% of graph heap, so they are what streams; nodes are the other 17% and are scanned, so they stay — under headings, with the honest 'this is an estimate, ~1.7-2.2x, no real-repo measurement yet' caveat kept in full. Refs #2680 * feat(analyze): make streamed graph emit the default, with nothing traded away Streaming was opt-in because it disabled the four phases that consume the whole CALLS graph — communities, processes, taintSummaries, callSummaries. That made it unshippable as a default: query() is process-grouped and clusters/skill-gen are community-backed, so every index would have silently lost them. The sink now answers a COMPLETE relationship read. It keeps streamed edges as four parallel columns over an interned node table — sourceId, targetId, type, confidence — and iterRelationships/iterRelationshipsByType/ forEachRelationship/relationshipCount return the retained edges concatenated with those. Every consumer therefore sees the whole graph and no phase knows streaming happened. Four fields, not six, because an audit showed community-processor, process-processor, taint-summaries and the pruner read only those — none keys on rel.id. That matters: relationship ids are unique long strings, and retaining them is precisely what made a fully-columnar attempt LOSE to the object graph (measured 838 MB vs 822 MB). Ids stay out of the columns; a read synthesizes one, which is safe because buildRelRow never persists it. Consequently deleted, not merely disabled: - the four enabledWhen gates and the 'what you give up' warning; - the pruner's hasStreamedSemanticEdge predicate and its plumbing — a complete scan sees streamed edges, so the dangling-edge hazard is gone by construction rather than by compensation; - the whole degraded-index apparatus: the graphPhases RepoMeta stamp, streamedIndexRiskDisclosure, and the riskUnderstated markers on impact() and detect_changes(). Nothing degrades, so nothing needs disclosing. Default is ON for full rebuilds; GITNEXUS_STREAM_GRAPH_EMIT=0 (or an explicit option) is the escape hatch, for bisecting a suspected streaming fault rather than routine use. Incremental runs still refuse it — the writeback reads relationships back out of the in-memory graph. Measured A/B, 400k nodes / 1.08M edges, all edges streamable (worst case for this design): 823 MB -> 626 MB, ~1.3x, all 1.08M edges still visible. That is deliberately less than the ~2.9x the retained-share formula implies — losslessness costs the dedup Set and the columns. The earlier, bigger number was bought by disabling phases. README and the file header both state 1.3x measured; neither claims O(chunk). New coverage: reads are complete (proven discriminating — 3 tests fail when the streamed leg is removed), endpoints/confidence survive the round trip, per-type lookup finds streamed types, and every CALLS-consuming phase stays registered under the flag. Refs #2680 * docs(2680): pin the invariants the default-on change relies on Review follow-ups. No behaviour change except the id-uniqueness fix. - pipeline.ts returns the RAW graph, not the sink, and that is load-bearing: phases read the sink so their scans are complete, but loadGraphToLbug feeds this value to streamAllCSVsToDisk, whose iterator would then emit every streamed edge a SECOND time on top of the per-pair CSVs the sink already wrote. Returning the sink there silently doubles every streamed relationship in the persisted graph, so the reason is now written down at the return site. - Synthesized ids now carry the column index, making them unique even when two streamed edges share (type, source, target) and differ only in reason/step. Harmless today because no consumer keys on relationship id, but real ids are unique and the synthesized ones should match, so a future id-keyed consumer cannot silently collapse two edges. - Recorded WHY dropping reason/step is safe, which is not the same argument as for id: the persisted row keeps their true values because buildRelRow receives the original relationship on the way through, so only in-memory reads see the 'streamed' placeholder. The ACCESSES reason:'read'|'write' distinction that MCP queries depend on therefore survives in the database. A future in-pipeline consumer needing either field must add a column rather than trust the placeholder. Also verified while chasing a review lead: removeNodesByFile has no production callers and removeNode has exactly one (the pruner), which reads through the sink and so sees streamed edges. The dangling-edge hazard the deleted hasStreamedSemanticEdge predicate used to compensate for is closed by construction, not by luck. Refs #2680 * fix(2680): fail loudly on a missing CSV dir, and guard the retain set Resolves both findings from the review of this branch. MEDIUM — pipeline.ts silently skipped streaming when `streamGraphEmit` was true but `graphEmitCsvDir` was absent. The CLI always supplies the dir, but streaming is on by DEFAULT now, and the callers that build PipelineOptions themselves (eval-server, MCP daemon, tests) are exactly the ones that would omit it — so they would ask for streaming, not get it, and still see a successful run. That is the silent-degraded-outcome shape the rest of this work exists to prevent, so it now throws with the resolution hint. Covered by a test asserting the rejection. LOW — RETAINED_REL_TYPES had no automated guard, and the round-trip test structurally cannot be one: addRelationship PARTITIONS edges between the graph and the CSVs, and a partition's union is invariant under where the line falls, so that test stays green for any partitioning including a wrong one. Drift there yields a silently incomplete mid-pipeline edge set, not a crash. Added a test that derives the required set by grepping every literal iterRelationshipsByType('X') under src/ and asserts the constant covers it, with CALLS as the documented exemption (taintSummaries reads it, which is why the sink answers a complete read rather than retaining it). Proven discriminating: removing EXTENDS from the constant fails with "expected [ 'EXTENDS' ] to deeply equal []". 128 tests green across the eight affected suites, including the index-lock suite that arrived with the #2677 merge. Refs #2680 * docs(2680): record the measured CPU cost, not just the memory win I measured memory before shipping and never measured time, which was a gap: reads now allocate, rebuilding objects instead of returning stored ones, and a real analyze does SIX full relationship scans (pruner, communities x2, processes x2, the taint fixpoint's CALLS pass). Same 400k-node / 1.08M-edge graph: heap 820 MB -> 623 MB (1.32x better) scans 96 ms -> 651 ms (6.8x WORSE) 6.8x on iteration is worth knowing, but the absolute number decides it: ~0.5 s here, ~2 s extrapolated to kernel scale, against an analyze measured in minutes — under 1% of wall-clock. The ~26M short-lived objects at kernel scale are young-generation churn (the cheap case), and being ~800 MB further from the heap ceiling matters more than the churn costs: #2649's cascade came from GC thrash NEAR the limit, not from allocation volume as such. Also names the first lever if these scans ever go hot — a per-type index over the columns, so iterRelationshipsByType stops scanning all streamed edges — and notes that it trades memory back, so it needs a measurement first. Refs #2680 * perf(2680): cut the iteration regression from 6.8x to 1.8x The memory win came with an unmeasured CPU cost. Iteration went from returning stored objects to rebuilding them, across the SIX full relationship scans an analyze performs (pruner, communities x2, processes x2, taint's CALLS pass). First measurement: 90 ms -> 651 ms, 6.8x worse. Fixed properly rather than documented away. Two causes, each measured before and after: 1. The ~150-character synthesized `id` was built eagerly on every read — 6.5M concatenations per analyze, for a field NO in-pipeline consumer reads. Isolating it (constant id) showed 436 ms of the 555 ms regression. Now a lazy prototype getter on a fixed-shape `StreamedRelationship` class: the string is built only if someone asks, and V8 keeps one hidden class across millions of instances. 2. Generator and iterator-protocol overhead on million-edge walks. `forEachRelationship` (community detection's form, called twice) now loops the columns directly, skipping both. `iterRelationships` keeps an iterator but reuses one result record — a hand-rolled version allocating a fresh {value, done} per edge measured WORSE than the generator (252 ms), which is why the obvious rewrite is not the one that shipped. heap 821 MB -> 623 MB (1.32x better) scans 90 ms -> 180 ms (was 651 ms) The residual ~90 ms is object allocation, 6.5M instances across six scans, and it is irreducible while the read API returns objects at all. The remaining fix for true parity is a field-wise callback passing sourceId/targetId/type/ confidence as primitives — all four hot consumers read only those — but that changes the KnowledgeGraph interface and its consumers, so it belongs in its own measured change rather than bolted on here. Refs #2680 * perf(2680): zero-allocation field scan brings iteration back to parity Third and final step on the iteration cost. The memory win had come with a 6.8x iteration regression; the previous commit cut that to 1.8x by making the synthesized id lazy and removing generator overhead. The residual was object allocation itself — 6.5M instances across the six full relationship scans an analyze performs — which no amount of tuning removes while the read API hands back objects. So the hot consumers stop asking for objects. Adds `KnowledgeGraph.forEachRelationshipFields`, which passes (sourceId, targetId, type, confidence) as primitives — exactly and only what every whole-graph scan reads. On the sink those come straight out of the columns, allocating nothing; on the object-based graph they are read off the stored relationship, so the flag-off path is unaffected. Converted the five whole-graph scans: community detection (x2), process extraction (x2), and the local-symbol pruner. `isFileDefinesEdge` now takes (type, sourceId) rather than a relationship. The taint fixpoint's by-type pass is left alone — one scan of six, and converting it would turn an indexed bucket lookup into a full scan on the object-based graph. heap 820 MB -> 623 MB (1.32x better) scans ~82 ms -> ~90 ms (was 651 ms; now parity within noise) Also deletes the pruner's `hasStreamedSemanticEdge` option, which has had no caller since the sink's reads became complete — a dead knob is worse than no knob. Verified: 104 tests across the eight affected suites, including the pruner's pipeline integration test (which needs the raised worker-ready timeout on this host; it passes cleanly with it and its failures are the known 5s handshake). Refs #2680 * perf(2680): compact dedup keys — 1.32x -> 1.59x, speed unchanged An audit of where duplicate relationship ids actually come from, then the saving it unlocked. The audit (instrumented analyze of this repo): 25 duplicate-id hits across 63,412 streamed edges — 0.04%, all CALLS, every one the SAME call site re-emitted when a file is resolved in more than one language pass. Three things follow, and they rule out the cheap options: - dedup cannot be dropped (25 != 0, and a duplicate reaching COPY is a wrong graph); - it cannot move to row contents, because emit-references builds ids as `...->target:line:col`, so two calls between the same pair at different sites have byte-identical CSV rows that the whole-graph emit keeps; - it cannot move to a per-file source guard like `pdgEmittedFiles`, because a later language pass can resolve genuinely NEW edges for the same file. What was left was the key itself. An id embeds both node ids in full (~200 chars here) while the endpoints are ALREADY interned for the columns, so the Set was storing them twice. Keys are now built from the interner indices plus the id's trailing disambiguator parsed into NUMBERS. Numbers, not substrings, and that is load-bearing: a key built by slicing inside a long string is a V8 sliced/cons string that keeps its parent alive, so the id would never be freed and the saving would silently fail to appear. An earlier attempt at this measured no improvement for exactly that reason. Unrecognized id shapes (`rel:contains:` has no tail) fall back to storing the id verbatim — correctness first, saving second. heap 821 MB -> 518 MB (1.59x, was 1.32x) scans ~83 ms -> ~88 ms (parity, unchanged) Speed is untouched by construction: dedup is on the WRITE path, and none of the six full scans reads it. Also fixes removeRelationship, which the test suite caught: it looked up the raw id in a Set that now holds compact keys, so it silently stopped throwing on an already-streamed edge. It cannot recompute a key from a bare id, so it is now conservative — anything the real graph does not hold is treated as possibly-streamed once streaming has begun and fails loudly. A genuinely-absent id throws where main returns false; acceptable because the only production caller (the COBOL resolver) runs before the sink is armed. 89 tests green across the six affected suites. Refs #2680 * fix(2680): dedup key dropped edges when tail segment counts differed Both findings from the review of this branch, and the coverage gap named alongside them. HIGH — the compact dedup key packed the id's trailing numeric segments as `|${a}|${b}`, with `b` defaulting to 0 when only one segment was present and the segment COUNT absent from the key. So `:7` and `:7:0` produced the same key and the second edge was silently discarded as a duplicate: a lost relationship, no error, no warning. Found by probe, not by reading — two distinct ids for one (source, target, type) went in and one edge came out. The key now carries `seen`. Nothing existing caught it. The round-trip test compares the UNION of graph and CSV rows, and a dropped edge is missing from both, so it stayed green; the duplicate test only feeds a genuinely identical id, which is the case that SHOULD collapse. Four new cases pin the boundary instead: differing segment counts stay distinct, two call sites between one pair stay distinct (the `:line:col` shape from emit-references), a truly repeated id still collapses, and a non-numeric tail falls back to the full id. Proven discriminating — reverting the fix fails with "expected 1 to be 2". This costs ~66 MB at 400k nodes / 1.08M edges (584 MB, was 518 MB), so the heap win is 1.40x rather than 1.59x. Not a trade worth making the other way: a silently missing relationship is the exact failure class the rest of this work exists to prevent. I am not asserting a mechanism for why two extra characters per key cost that much — it is stable and reproducible across runs, and inventing a cause is how I got the earlier cons-string diagnosis wrong. LOW — removeRelationship throws for an absent id once streaming has begun, where KnowledgeGraph.removeRelationship returns false. The behaviour is deliberate (a bare id cannot be turned back into a compact key, and answering "false" for an edge already on disk is the worse failure) but it was undocumented and untested. Now stated on the interface itself and pinned by two cases: absent-id-while-streaming throws, absent-id-before-streaming returns false. Coverage gap — added a test asserting forEachRelationshipFields yields the same (source, target, type, confidence) tuples as iterRelationships. That guards the five whole-graph scans converted in 9fa18384, where a divergence would silently skew community detection, process extraction and the pruner. Also records the verified scaling in the file header: linear at 100k/200k/ 400k/800k nodes, per-edge scan cost flat at ~13 ns in both arms, heap ratio drifting only 1.7x -> 1.5x as interner indices gain digits. No super-linear term. 135 tests green across the eight affected suites. Refs #2680 --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>	2026-07-25 07:43:51 +01:00
Gergő Magyar	3c82361b66	perf(cfg): streaming/chunked PDG graph emit for full-kernel-scale repos (#2202) (#2216)	2026-06-16 05:04:10 +01:00