mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
7 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
DuduPhudu
|
223ac7010d
|
feat: close reported graph blind spots in reference resolution, analyze and storage (#2856)
* fix(mcp): report UNKNOWN risk when an upstream impact walk finds no callers `risk: LOW` asserts "safe to change" — a claim ABOUT callers. An upstream walk that resolved none has nothing to base it on: the symbol may be genuinely unused, or reached only through a reference class the index does not record (a property access on a plain object, a bare-identifier read of a module-scope const). Seeding LOW from an empty result is the false-safe signal `anyKnownRisk` already refuses to emit on the ambiguous-candidate path, and that #2687 removed by making an undetermined impactedCount `null` rather than `0`. Zero-caller upstream results now report risk UNKNOWN with a riskNote saying absence of edges is not evidence of disuse. Downstream is untouched: an empty downstream walk reports resolved callees, not safety. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(javascript): emit ACCESSES for bare-identifier reads of module-scope consts A constant read only as a bare identifier — `Math.max(LIMIT, n)`, a default parameter value, `return LIMIT` — minted no reference site at all, because JS captured only `@reference.read.member`, which requires a receiver a bare identifier does not have. So "who uses this constant?", the question behind every dead-code trim and constants refactor, answered with a confident zero in both directions. The rest of the machinery was already in place: `FIELD_KINDS` accepts `Const`, the scope query already declares it via `@declaration.const`, and `read` maps to ACCESSES for any resolved target. This adds the missing capture in VALUE POSITIONS ONLY (call arguments, default-parameter values, return statements) — a blanket `(identifier)` rule would mint a site for every token in the file, which is unaffordable at repo scale and would keep alive the block-local symbols `pruneLocalSymbols` exists to drop. Cross-file readers are NOT yet covered: the site exists and a call through the same import statement resolves, but a value-kind def does not link across the import edge. Recorded as a todo with the investigation. PARSE_CACHE_VERSION bumped 44 -> 45: this is parse-time capture emission, so a warm cache replays the pre-change capture set and the new edges never appear — observed directly, a full `analyze --force` produced a byte-identical graph until the cache was cleared by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(javascript): pin A1/A5 plain-object property acceptance criteria Fixture plus todo specs for the four shapes plain-object property access has to answer: object-literal keys indexed as Property nodes, a read through the holding variable, a property WRITE, and a read through an untyped param. Records the investigation so the work is resumable: the parse-query pattern scoped to literals bound to a variable matches correctly (verified against the raw JAVASCRIPT_QUERIES), but no Property node reaches the graph and local-symbol-pruner is not the cause — it drops only Const/Variable/Static. The remaining gate is in the parse worker's node-creation path. No production code — specs only, so the suite stays green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(javascript): index object-literal keys of a named object as Property nodes Idiomatic JS models configuration as an object literal, not a class, but Property definition nodes existed only for DECLARED CLASS FIELDS. A config field therefore had no symbol at all: `context({name: 'exitMinAtrMult'})` answered "not found" for a field read and written throughout a live code path, and ACCESSES had no target to point at. Both halves are added for keys of a literal BOUND TO A VARIABLE — the parse query mints the graph node, the scope query mints the def the resolver can aim at. Unbound literals are deliberately excluded: an inline call argument or a JSX prop bag is call-site data, not a named surface other code references, so a node per key there would add volume without adding an answerable question. This lands the definition-node half only. The ACCESSES edges still require receiver resolution — typing the const that holds the literal to the literal's scope for the precise case, and name-based matching at reduced confidence for the untyped-param (option bag) case. Both are recorded as todos with the mechanism each needs. Also records a trap that cost a wrong conclusion: under vitest the parse worker runs the BUILT dist code (parse-impl resolves parse-worker.js, absent under src/, and falls back to dist), so parse-query changes are invisible to tests until `npm run build`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(cache): move the SCHEMA_BUMP pin to 45 The pin is the guard that makes two branches claiming one cache-schema number fail loudly instead of silently serving each other's entries, so a bump is only half-done until the pin moves with it. The bump itself landed with the JavaScript bare-identifier captures; this is the other half. Caught by the guard working exactly as designed — the suite failed with "expected 45 to be 44" rather than letting a mismatched pair through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): resolve plain-object property access by unique name Idiomatic JS reads configuration off an object whose receiver cannot be typed — an options bag passed as a parameter, a destructured handle, an imported literal. No precise pass resolves those, so a field read and written across a live code path produced no ACCESSES edge at all and "who reads this setting?" answered a confident zero. A last-resort pass runs after every precise pass and sees only what they left behind. For each still-unresolved read/write site it asks whether exactly ONE Property in the workspace carries that name. If so the read almost certainly means it. If two or more do, nothing is emitted and the site is COUNTED as ambiguous — a guess between them would be a coin flip, and a wrong edge in the pre-edit safety gate is worse than a missing one. Uniqueness is the right gate because it recovers exactly the names worth recovering: distinctive domain fields (exitMinAtrMult, bookNotionalUsdt) are unique in a repo and resolve, while generic keys (id, name, data) are not and are skipped — which is where name matching would over-connect. Bounded four ways: - Confidence 0.5, the global tier, with the inference named in the reason, so a consumer can filter inferences without losing scope-resolved edges. - Never second-guesses a precise result: sites already resolved are excluded, because first-write-wins stops a duplicate but NOT a second edge to a different target. - Honors `fieldFallbackOnMethodLookup`. A statically-typed language opts out of name matching precisely because it over-connects; inferring an ACCESSES edge by name is the same claim and must obey the same opt-out. - Requires an explicit receiver — a bare identifier is not a property access, and matching one by name would link a local to an unrelated key. Indexes graph nodes rather than scope defs because an object-literal key mints a Property NODE but no scope-resolution DEF: `localDefs` and `scope.bindings` are both empty for exactly the population this serves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(analyze): record a collapsed graph write instead of reporting fresh The dangerous half of a broken refresh: metadata IS written, so the index reads as fresh, hooks re-arm, and every tool answers from a graph missing most of its edges — indistinguishable from a codebase that genuinely has no such relationships. Reported in the field as edges collapsing 23009 -> 2170 and as a CodeRelation table that never materialized. `analyze` now compares the relationship count the pipeline PRODUCED against what the DB hands back after the write. Both numbers are already in scope at the same point, so the shortfall is provable rather than inferred — no comparison against the previous index, which cannot distinguish a failed write from a repo that legitimately shrank. A missing relation table needs no special case: it reads back as a persisted count of zero. On a collapse the run records `graphWriteCollapsed` in metadata, which `getIndexIncompleteReasons` turns into `graph-write-collapsed` so status and the MCP resources report the index INCOMPLETE rather than fresh. A ratio, not equality: some relationship types do not round-trip one-for-one and `--pdg` writes MORE rows into the same table, so demanding equality would fire on healthy runs. Only a collapse is a defect. Fail-safe when the expected count is unavailable — an implementation that offloads relationships out of memory may not be able to report a total, and a false "your index is broken" is worse than a missed one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(ingestion): qualify object-literal Property ids by their owning object Two config objects in one file that share a key name generated the same `Property:<file>:<key>` id and COLLAPSED INTO ONE node, so two distinct settings became a single symbol. Worse, the merged name then looked workspace-unique to name inference, which happily resolved reads of it to a node representing both — a wrong edge in the pre-edit safety gate, which is precisely what the unique-name pass is bounded to avoid. `objectLiteralOwnerInfo` already existed for exactly this ("so two constructors in one file that both define `bar` stay distinct nodes") but was gated to `Method`. `Property` now opts in. `findObjectLiteralBindingInfo` returns `ownerName` only when asked. Its `Method` ids must stay byte-identical — qualifying them would rewrite every object-literal method id in every indexed repo — while object-literal KEYS, indexed only since A1/A5, have no such history to preserve. Found by a test written for the ambiguity path rather than by review: the suite reported one node where two were expected, and an edge where none should exist. Both are now pinned, along with the detection boundaries of the B2 collapse check, which was previously an untestable inline expression and is now a pure function. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(typescript): index type aliases and shape members as symbols A TS frontend models its API contracts as `type X = { … }` and `interface`, so a field on one is exactly what "who breaks if I remove this?" is asked about. Three gaps made that unanswerable, all in the TypeScript queries: 1. No `type_alias_declaration` -> `@definition.type`, so an alias minted NO NODE AT ALL and a context() lookup on an exported contract type answered "Symbol not found". TypeScript was the ONLY language missing this — Rust (type_item), Kotlin (type_alias), Swift (typealias_declaration) and Dart all emit it. The alias was declared for scope resolution but never became a graph symbol. 2. No `property_signature` in the parse query, so INTERFACE members minted no Property nodes either — the upstream report's "class/interface index fine" holds only for the type, not its fields. 3. No `property_signature` in the scope query, so even with nodes present the resolver had no member declaration to aim at. Its sibling `method_signature` -> `@declaration.method` already existed; only properties were missing. Interface bodies and object-type aliases both spell members as property_signature, so one pattern per query covers both shapes. Lands the SYMBOLS, not yet the ACCESSES edges: the shape is already a class-like scope and now has member declarations, but no edge forms — the remaining link is owner/type-binding, recorded as todos with the diagnosis. Note TypeScript sets fieldFallbackOnMethodLookup:false, so unlike JavaScript there is deliberately no name-based fallback here; the precise path is the only route by design. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(golden): accept interface members in the mini-repo snapshot Drift is entirely the new TypeScript shape-member indexing: the fixture's three interfaces (ValidationResult 2, DbRecord 3, LogEntry 3) contribute exactly 8 Property nodes, each with exactly one HAS_PROPERTY owner edge. Verified before regenerating rather than after: every pre-existing count is untouched (CALLS 9, IMPORTS 12, DEFINES 16, HAS_METHOD 1, MEMBER_OF 12, STEP_IN_PROCESS 12), so nothing was rewired — the digest moved only because 8 edges were added. The fixture's inline `return { valid: false, … }` literals correctly produced nothing, confirming the object-literal rule stays scoped to variable-bound literals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(analyze): never report a collapse from a non-numeric count The B2 check reported healthy runs as total graph-write collapses. A non-numeric `expected` (a graph implementation reporting no total, a lightweight pipeline result) does not skip the guards — it INVERTS them: `undefined < 100` is false, so the small-repo exemption never fires, and `0 >= undefined * 0.5` is `0 >= NaN`, also false, so the ratio check "passes" as well. Both bounds silently evaporate and every such run is flagged. That is precisely the failure this check was written to catch, reproduced inside the check itself: an unmeasurable quantity treated as a measured zero. Both sides are now validated as finite numbers before any comparison. `persisted` is also passed as UNKNOWN rather than zero when the DB was not demonstrably readable: `getLbugStats` flattens "no connection", "query threw" and "empty table" all into `edges: 0`, so `stats.nodes > 0` is used as independent evidence the read happened at all. Caught by the existing run-analyze suites, not by the new unit tests — those exercised the pure function with well-formed numbers and were blind to the integration's actual inputs. Both cases are now pinned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(typescript): make object-type aliases own their members A TS object-type alias declares the same `property_signature` members as the interface beside it and answers the same question, but was not a member owner: its fields were minted with bare ids and no owner edge, so two aliases in one file sharing a field name collapsed onto one node, while the identical interface resolved normally. `type_alias_declaration` joins CLASS_CONTAINER_TYPES (and CONTAINER_TYPE_TO_LABEL, as that set's invariant requires — a container missing there gets orphaned member edges or a wrong owner label). Aliases with no object type (`type Id = string`) declare no members, so they own nothing and are unaffected. This also lands the INTERFACE field -> consumer edges, verified on the mini-repo fixture rather than only on a purpose-built one: `saveToDb` now links to `ValidationResult.value`, and `formatLogEntry` to `LogEntry.level` and `LogEntry.message` — three real contract-field reads that previously had no graph path at all. Golden updated: +3 ACCESSES, no node changes. The ALIAS field -> consumer edge is still not linked and is recorded as a todo with the exact blocker: resolving a receiver typed as the alias needs the NAME to resolve to a class-like def, and `isClassLike` is Class|Interface|Struct|Record|Enum|Trait. That predicate is read from ~12 sites including MRO and heritage, and every language mints TypeAlias, so widening it would enrol aliases in linearizations where they do not belong. Widening only the scope index was tried and reverted — the type-name walkers gate on it independently, so it fixed nothing and left dead code. That needs a deliberate "shape-like" concept, not more call-site widening. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test): record the traced diagnosis for the unlinked alias field edge Traced to the end rather than left as "needs investigation", so the next attempt starts from facts: 1. Graph side is COMPLETE and symmetric with the interface — Property:...:LiveModeConfig.bookSlots is owner-qualified and carries HAS_PROPERTY. 2. Resolution DOES reach resolveClassBindingForName('LiveModeConfig') (instrumented) and misses. 3. It misses because the module scope binds LiveModeIface:Interface, renderAlias, renderIface — and not LiveModeConfig. The alias has no binding on the receiver's scope chain at all. 4. The TS scope query tags aliases @declaration.type, but normalizeNodeLabel accepts only typealias / type_alias and has no "type" case, so it returns undefined. Kotlin and Dart use @declaration.type_alias; TypeScript is alone on the dead tag. 5. Retagging is NECESSARY BUT NOT SUFFICIENT — tried, and the binding still does not appear, so a second gate exists in how a declaration anchored on a node that is ALSO a @scope.class anchor is attached: the alias appears to bind inside its own scope rather than hoisting to Module, where interface_declaration evidently does hoist. An isShapeLike predicate (the nominal-vs-structural split: shapes declare members, nominal types participate in MRO) plus a mirrored findShapeBindingInScope were built and REVERTED along with the retag. With no binding on the chain they never fire, and shipping inert widening is worse than shipping none — the same standard applied to the earlier scope-index attempt. The design is recorded here; it is worth doing once step 5 is fixed, and it also unblocks Rust's parked union_item, which the MEMBER_OWNER_NODE_TYPES comment documents as the same gap in another language. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): resolve cross-file value references, skip block-locals Two halves of the same question, "who uses this constant?". CROSS-FILE. `resolveReferenceSites` runs against the registries and, as its own comment says, "imports live in finalized bindings the registries can't see" — which is why free CALLS need `emitFreeCallFallback`. Reads had no counterpart, so `import { LIMIT }` followed by a bare use resolved to nothing while a CALL through the very same import statement resolved fine. This adds the read/write counterpart, reusing `findValueBindingInScope` (which walks the FINALIZED chain) rather than inventing a lookup. Confidence 0.9: the import names the def, so this is precise resolution, not inference. BLOCK-LOCALS. Bare-identifier capture also matches a read of a block-local `const`, and an edge to one keeps alive exactly the inert locals `pruneLocalSymbols` exists to drop — a pruned node becomes a retained node plus an edge, in every function of every indexed repo. Emission now takes the set of value defs bound at MODULE scope and drops ACCESSES to Const/Variable/Static outside it. The cross-file pass carries the same guarantee structurally: a def in another file cannot be a block-local of this one, so it skips same-file hits entirely. The block-local leak was already shipped in the intra-file A2 commit and was found only because a test was written for the guard rather than the feature — the same way the object-literal id collision surfaced. Verified on the full resolver matrix: 3172 tests, golden unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(lbug): diagnose a vanished staging CSV instead of surfacing a Binder error A forced rebuild could fail with "COPY failed for File: Binder exception: No file found that matches the pattern .gitnexus/csv/file.csv" and then an ENOENT on .gitnexus/csv/rel_Folder_File.csv — two engine-level messages that name neither a cause nor a remedy, which is where several field reports end. Only tables with rows > 0 enter the COPY manifest (csv-generator.ts), so an absent file was WRITTEN during this run and removed since. Both COPY loops now preflight and say exactly that, with the row count, both causes the reports point at (a second `gitnexus analyze` on the same repo — they share .gitnexus/csv — or an external cleanup of .gitnexus/), and the action to take. Scope note, deliberately narrow: this does not attempt to fix WAL corruption or checkpoint rotation. Those already have detection and recovery hints (isWalCorruptionError, WAL_RECOVERY_SUGGESTION, the configurable wal-checkpoint-threshold), and the ~6000 lines added to lbug/ + storage/ since v1.6.9 — index-lock.ts most of all, which serializes writers and plausibly closes the concurrent-run class outright — postdate every report in the window. Guessing at unreproducible durability faults would be speculation; making the one failure with NO handling legible is not. An existing overlap test induced this exact scenario (a manifest entry pointing at a missing csv) and asserted on the engine's wording. Its intent — that a node-COPY failure is rethrown at the FK barrier rather than swallowed — is unchanged and still asserted; only the message it matches moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): split shape-like from class-like, linking alias fields Completes A4: a field on a TypeScript object-type alias now links to the code that reads it, the last unanswerable half of "who breaks if I remove this?" for a TS frontend that models contracts as `type X = { … }`. `isClassLike` answered two questions that only coincide for classes: 1. does this declare MEMBERS I can look up? — a SHAPE (structural) 2. does this participate in inheritance / MRO? — a NOMINAL TYPE An object-type alias is (1) and emphatically not (2) — it has no supertypes and no place in a linearization. Widening `isClassLike` to buy (1) would have enrolled every language's aliases (Rust type_item, Kotlin/Swift/Dart typealias, C typedef) into MRO and heritage, so the two questions now get two predicates. Call sites split by which they ask, and their names already said which: `resolveInheritanceBaseInScope` and `resolveQualifiedInheritanceBase` keep `isClassLike`; receiver typing and member OWNERSHIP take `isShapeLike`. Three parts, each necessary and none sufficient alone: - `findShapeBindingInScope`, mirroring `findValueBindingInScope`'s established relationship to `findClassBindingInScope` (same walker, different accepted def-type), consulted only AFTER the class lookup misses so a class of the same name always wins. - `populateClassOwnedMembers` uses it, so alias members get an `ownerId` and are registered under the alias. Without this the receiver resolved to the alias and then found no members under it. - The TS scope query tags aliases `@declaration.type_alias`, not `@declaration.type`: `normalizeNodeLabel` accepts typealias / type_alias and has no "type" case, so the old tag mapped to NO label and TypeScript aliases produced no scope-resolution def at all. Kotlin and Dart already spelled it this way; TypeScript alone was on the dead tag. An earlier attempt concluded a further "scope-attachment gate" existed. That was wrong and is worth recording: scope extraction runs in the parse WORKER, which loads built `dist`, so the retag was never executed. Rebuilt, the alias hoists to Module scope exactly as the interface does. Same trap as the parse query — `src` edits to anything the worker runs are invisible until `npm run build`. Typedef and Union stay out of `isShapeLike` deliberately: they belong conceptually (the union_item note on MEMBER_OWNER_NODE_TYPES records the same gap) but neither is wired as a member container, so including them would widen a predicate nothing exercises. Verified on the full resolver matrix: 3173 tests, golden unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(typescript): pin the type-alias capture to a tag that maps to a label The capture test asserted `@declaration.type`, the tag that `normalizeNodeLabel` does not recognize (it accepts typealias / type_alias and has no "type" case). So the test passed for as long as the tag was broken: it checked only that the capture FIRED, never that it resolved to anything, while TypeScript aliases produced no scope-resolution def at all. Updated to the working tag and given a second assertion that the derived kind string is one the label mapper accepts — the property that actually matters, and the one whose absence let a dead tag sit pinned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(lbug): declare TypeAlias member pairs so analyze does not abort Making object-type aliases member owners emits HAS_PROPERTY from a `TypeAlias`, and the relation schema declared no such pair. The emit therefore threw `UndeclaredRelationPairError` and the ENTIRE analyze died on any repo containing `type X = { ... }` — a hard stop, not a dropped edge. Found by running the analyzer over a real 16k-node TypeScript repo, not by a test. `Method` is declared alongside `Property`: a member written `type Handler = { onClick(): void }` is a method_signature and would fail in exactly the same way. Why every existing test missed it: the resolver suites build an in-memory graph via `runPipelineFromRepo` and never write to LadybugDB, so the schema constraint was never exercised. `structural-pair-coverage.test.ts` is the one suite that does run the emitters against the declared pairs — and its own docstring names the gap: coverage is bounded by NON_BRIDGE_CORPUS, "a new structural emitter should land with an entry here". This adds that entry, pinning TypeAlias|Property and Interface|Property as sentinels. Verified the guard is not vacuous: removing the pair again makes the suite fail with undeclaredPairs: ["TypeAlias|Property"]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(processes): trace depth-first so multi-hop flows are detected D1 ("query ranks frontend components above the backend module that owns the concept") and D2 ("processes is dominated by trivial mechanical chains") are the same defect, and neither is about ranking or selection. The walk stops after a fixed NUMBER of traces, so traversal order decides which traces those are. Breadth-first reaches every shallow terminal before any deep one, so the quota filled with the shortest paths in the graph and the walk stopped — `maxTraceDepth: 10` was never approached. Measured on a real repo before the fix: of 300 processes NONE exceeded 7 steps and 90% were 3-4. A multi-hop business flow (signal → order → exit) therefore had no process that could represent it, and `query` could only rank the mechanical pairs that did exist. Step 4 of the caller already sorts by length and dedupes by endpoint — it was always asking for the deepest traces this walk could give it. Depth-first descends to a terminal first, so the same quota is spent on paths worth keeping. Cost is unchanged: same budget, same cycle guard, same depth ceiling — only the order differs. Measured on the same 16k-node repo, same build and flags, BFS vs DFS (an earlier comparison was discarded as confounded — it crossed builds and --pdg): steps 6-8: 50 → 168 (3.4x) totals: 844 → 806 and the reported query moved from `LiveSetupView → Cn` (a React component) to `ReconcilePositions → IsTpInProfit / WithHeld / ShouldNotify` — server-side exit management, which is what was asked for. `traceFromEntryPoint` is exported for the test. Traversal order is unobservable through `processProcesses`: `findEntryPoints` supplies several starting points, so a deep chain is traced from inside it whatever the order does. A test at that level passes under BOTH traversals — the first version of this test did exactly that and guarded nothing. Driving the walk directly, it fails under breadth-first with "expected 3 to be greater than 3". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(test): correct a stale status note left behind by a later fix The A1/A5 header still said "edge resolution REMAINING ... neither is implemented". Both shapes resolve — the typeable receiver precisely, the untyped one by workspace-unique name — and the tests below assert exactly that, so the note contradicted the file it sat on. It was accurate when written and went stale when the work continued past it. Left as-is it would tell a reviewer that a landed feature is missing. The TRAP note is kept: the parse worker still runs built dist under vitest, and that is still the trap it describes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): index literals behind identity-preserving wrappers `export const INERT_EXIT_CONTRACT = Object.freeze({ ... })` minted no `Property` node for any of its keys. The object-literal rule matches `variable_declarator > value: (object)` as a DIRECT child, and freezing puts a call expression in between — so the shape whose fields are most worth querying was the one shape the rule could not see. Freezing a config object is how JS publishes an immutable contract, which is why this reads as a confident zero on exactly the fields a reader cares about. The allowlist is three functions, not "any call". `Object.freeze`, `seal` and `preventExtensions` RETURN THE ARGUMENT THEY WERE GIVEN, which is what makes the literal's keys members of the bound name. For `const x = compute({ a: 1 })` the literal is an argument and `x` holds compute's return value, so attributing `a` to `x` would be a fabrication. Two negative controls, because the obvious one is vacuous: a bare-identifier callee is rejected structurally and would pass with no allowlist at all, so the assertion that actually pins the predicate uses `Object.entries` — identical shape, differing only by name. Verified load-bearing by adding `entries` to the allowlist and watching that test alone fail. SCHEMA_BUMP 46 -> 47: parse-time emission, so a warm cache replays the pre-fix capture set. Observed as a false negative first — `analyze --force` returned the old node set until the on-disk cache was removed by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(scope-resolution): narrow multi-candidate property names by scope Workspace uniqueness was the wrong denominator. Measured on the reporting repo: `exitMinAtrMult` has 26 `Property` definitions — 16 in one-off `scripts/`, 7 in the frontend, one in a test, and exactly ONE in the backend that reads it. Every backend read was refused because of competitors the reader cannot see. The gate was not too permissive or too strict, it was scope-blind. A name with several definitions is now narrowed before being abandoned: same-file first, then files the reading file directly imports, using the finalized import graph rather than a path-shape heuristic. Exactly one survivor at the first non-empty tier resolves; anything else stays refused. A tier holding several candidates stops the walk instead of falling through — local evidence that is itself ambiguous still contradicts reaching further out. Confidence stays 0.5 at every tier. Narrowing changes which candidate is chosen, not the kind of claim: it is still a name match, and the round-1 contract is that filtering on confidence drops all name inference at once. The reason string now names the tier that fired. Ambiguity reporting goes from a count to the actual names (capped), because a count says a gap exists while the names say which fields are unanswerable. Measured on that repo, backend readers of `exitMinAtrMult` go 0 -> 24 and total readers 9 -> 45, including the two call sites in `oppositeSignalExitManager.js` the report singled out. Both narrowing tests were mutation-checked by dropping the import evidence and confirming they, and only they, fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): capture destructured parameter keys as property reads `function exit({ exitMinAtrMult = 0 })` reads that property off whatever the caller passes, exactly as `cfg.exitMinAtrMult` would. It never appears in a member_expression, so it had no reference site at all — and this is the shape the function that IMPLEMENTS a behaviour uses, so the most relevant reader was the one systematically missing from "who reads this setting?". Uses a distinct `@reference.read.destructured` anchor rather than `@reference.read.member`. The latter is filtered emit-side to matches with a member_expression ancestor, because calls and writes share its shape, and a destructuring pattern has none — reusing the tag would have been silently dropped by that filter. The `read.` head already maps to a read kind, so no mapping change is needed. Scoped to formal_parameters. A destructuring binding elsewhere (`const { x } = require('m')`) is frequently an import rather than a field read, and minting a property read there would attribute module bindings to unrelated same-named keys. All three cases (default value, bare shorthand, renamed key) mutation-checked by removing the patterns and confirming those three tests, and only those, fail. The renamed case also asserts the edge points at the KEY and that the local alias mints nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(scope-resolution): link type consumers to the type they name An exported contract type owned its members after round 1 and still answered `incoming: {}`, so "what breaks if I remove this field?" — the question a contract type exists to answer — had no edge to walk. Measured on the reporting repo: all 324 TypeAlias nodes AND every Interface node had DEFINES as their only incoming edge. Two independent causes, and the second is why the first was not enough. TypeScript captured no type references at all — only cpp and csharp did — so an annotation naming a declared type minted no reference site. Added for annotations, generic arguments and `as` assertions, anchored to those contexts rather than a bare `(type_identifier)`, which would also match the name in `type X = …` and make every declaration a consumer of itself. That alone fixed interfaces and left aliases still empty. `TypeAlias` was missing from `LINKABLE_LABELS`, so alias graph nodes were never indexed in `nodeLookup` and `resolveDefGraphId` could not bridge a def to its node — the edge was dropped AFTER a successful lookup. `CLASS_KINDS` has always listed TypeAlias and the ClassRegistry returned the def correctly, which is what made this read as a resolution failure; instrumenting the lookup showed it returning the right def all along and moved the search one table over. Exactly the bug already documented two entries above it for Trait. Fixes every language that spells an alias this way — TypeScript, Kotlin, Dart and Rust all emit `@declaration.type_alias`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): capture record construction as property writes The read side answered well after the narrowing work while "who SETS this field?" still missed the code that stamps the value. A record built inline — `return { exitContract: { exitMinAtrMult: settings.x } }` — is bound to no variable, so it minted no definition and its keys referenced nothing. Modelled as WRITE REFERENCES, deliberately not definitions. The round-1 rule already mints Property nodes for literals bound to a variable; minting more for anonymous records would add same-named competitors to the very name-narrowing that makes these fields resolvable — measured at 26 competing definitions for one field on the reporting repo, which is what made every backend read unanswerable in the first place. A construction site is a USE of a field, not another declaration of it. Two positions only: nested under a key, and returned. Both are records with a name attached (the key, or the function). An inline call argument (`doThing({ id: 1 })`) stays excluded for the same reason round 1 excluded it from definitions — it is call-site data, not a named surface — and is asserted as such. The enclosing literal is the receiver and it is anonymous, so these route through the same narrowing and the same refusal-to-guess as every other untyped receiver. Verified on the reporting repo: `entryPlan.js` went from no rows to `selectExitEnvelope` as a writer of `exitMinAtrMult`. Both captures mutation-checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(processes): select round-robin by terminal so the list is not one flow repeated Ranking was `sort by length` alone, so the top of the list was one behaviour described many ways: eleven of the top fourteen processes on the reporting repo were four entry points crossed with three terminals of the SAME date-window utility cluster. Genuine call chains, but a reader learns one thing from fourteen entries, and the repo's own domain flows sat below them. Selection now round-robins across TERMINALS, deepest first. Depth still orders within a terminal and still leads the list; what changes is that no terminal takes a second slot until every other has had a first. Keying on the entry point was tried first and made it worse — many files declare a `main`, so each was a distinct entry that round-robin then awarded its own slot, and `Main -> AlignWindowEnd` went from one row to eight. The repetition was never in where a flow starts. Measured on that repo: distinct terminals in the top 20 went 3 -> 20, and its domain flows (`ReconcilePositions -> ...`) moved into the top 4%. Two things this deliberately does not claim. The reported cause — ranking rewarding fan-in, promoting chains ending in widely-called helpers — measured FALSE: those terminals have one caller each (`alignWindowStart` 1, `validateSymbol` 1). A fan-in discount was implemented against that hypothesis, measured, and reverted for moving nothing. And a business flow still cannot be a process in its own right: the walk only emits at a leaf, at max depth, or on a cycle, so a flow whose meaningful endpoint calls onward survives only as whatever leaf it bottoms out in. Both are recorded in the code so neither reads as settled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(structural-pairs): pin the type-annotation USES pair R2-2 emits USES INTO a `TypeAlias`, so the pair is `Function|TypeAlias` — a different table from the `TypeAlias|Property` entry added in round 1, and one that entry stays green without. `TypeAlias` is on the eleven-table list this suite exists for, and an undeclared pair does not degrade: it throws `UndeclaredRelationPairError` and kills the entire analyze on any repo containing an annotated type. Every resolver suite still passes, because they build an in-memory graph and never write to the DB. That exact failure shipped once in this PR already. Two emitters into the same label, each with its own way to reach a released build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(scope-resolution): build the module-level set before the out-of-core seal Review blocker. Under `GITNEXUS_DISK_SCOPE_INDEX=1` the seal replaces every ParsedFile with a scope-STRIPPED copy, and the block-local filter's set was built after it — so it walked `scopes: []` for every file, came out empty, and the filter read that as "no def is module-level" and dropped EVERY `Const`/`Variable`/`Static` ACCESSES edge in the repo. All languages, all files, including the module-scope-const edges this PR exists to add. Nothing threw and nothing logged, on the path the largest repos take: the exact confident-empty answer the PR is about. Built above the seal now, from `parsedFiles`, and passed as `undefined` rather than an empty set when no scope was inspectable — an empty set is a legitimate answer ("this repo has no module-level value defs") and must not be indistinguishable from "could not look". Fails open; the block-local exclusion is still asserted under the seal, since that is correctness rather than optimization. Also widens module level past `kind === 'Module'`. A `Namespace` scope (TS `namespace`, Rust `mod`, C++/C# `namespace`) holds importable values too, and treating its consts as function-locals dropped their reads. Included only when the whole chain to the root is Module/Namespace, so a namespace declared inside a function body stays local — asserted both ways. That fixture then failed for a third reason: `@reference.read.identifier` existed ONLY in the JavaScript query, so A2 did not work for TypeScript at all. Added there, and both languages widened to `variable_declarator value:` and `binary_expression` operands — the gaps review named between what A2 claimed and what it matched. Nothing covered `GITNEXUS_DISK_SCOPE_INDEX`. The new parity test asserts the seal changes no edge, and was verified against an emulation of the original bug: same-file readers vanish and only the cross-file reader survives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(typescript): anchor property_signature to declared shapes Review blocker, and it reproduces end to end. `property_signature` occurs in EVERY object_type in the TS grammar, not only in an interface body or an alias's object type, so inline parameter types, inline return types and nested object types all matched — and the enclosing-container walk hung each one off the nearest class, interface or alias. Measured against the unanchored rule, all four appeared as members of shapes that do not have them: Property:contracts.ts:Svc.inlineParamOnlyKey Property:contracts.ts:Repo.inlineQueryOnlyKey Property:contracts.ts:NestedConfig.nestedOnlyKey Property:contracts.ts:buildInline.inlineReturnOnlyKey@46:33 When the inline member shares a name with a real one — `run(opts: { retries: number })` inside a class that declares `retries` — `addNode` is first-write-wins and the two distinct symbols merge onto one node, so every context()/impact()/rename() answer about that field describes the merge. The sibling JS object-literal rule in this same PR is anchored for exactly this reason; this is the TypeScript half of the same fix. `(A (B))` matches DIRECT children, so nested object types are excluded by the same anchor rather than by a second rule. The first version of these tests was VACUOUS and is recorded here because the reason generalizes: a collision and a correct exclusion both leave exactly one node behind, so counting ids cannot distinguish them. Every inline member in the fixture is now uniquely named, which is the only thing that discriminates — verified by restoring the unanchored rule and watching exactly those four assertions fail. A fifth test asserts anchoring costs no real member. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(analyze): correct the numbers feeding the graph-write-collapse guard Review blocker. The predicate itself held under adversarial probing; every defect was in what it was handed and what happened after it fired. (a) `expected` was wrong twice. Under `GraphEmitSink` streaming the bulk types leave the heap at parse time and never enter `relationshipCount`, so the count understated the real volume by most of it and the ratio passed trivially — on `force === true` runs, which include crash recovery AND the `analyze --force` retry this check's own warning tells the operator to run. Adds the manifest totals, the same correction the buffer-pool hint in this file already makes for the same reason. Separately, an incremental run persists only the changed subgraph while both counts are whole-scope: a 10,000-edge index that lost 200 replacements reads 9,800 and is certified complete. The check is skipped on that path rather than answered wrongly. (b) A throwing edge count became a measured zero. `getLbugStats` initialised its total to 0 and ran the query in a swallowing catch, so WAL/lock contention during finalize — documented on this exact call — reported a healthy index as a total collapse. It now returns `number | undefined`, and the caller requires both a readable node count and a defined edge count. (c) A total loss was exempted for being small. The min-edges rule tested `expected` before looking at `persisted` at all, so `expected = 99, persisted = 0` — every edge gone — stayed fresh and reported success. Total loss is now decided first. The existing test asserted the defect; it now asserts a PARTIAL shortfall, which is the case the exemption was written for. (d) A detected collapse reported success and exited 0. It is different in kind from the other incomplete reasons: those describe a run that did what it said and left work for later, this one means most of your edges are gone and every query answers a confident empty. The CLI now prints INCOMPLETE with the counts and sets a non-zero exit code, and the flag crosses IPC so the worker cannot send a clean `complete` either. Nothing exercised this wiring — only the pure helper. Adds tests for all four, each written so the pre-fix arithmetic fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(scope-resolution): keep unique-name property inference inside one language The pass indexed `Property` nodes from the whole shared graph. Per-language gating decides whether it RUNS for a language; it never restricted which nodes could be TARGETS. So the only carrier of a name could be in another language entirely, and a read here resolved to it on name uniqueness alone — no owner, no file, no call path. Reproduced: a Java class declaring `private int loyaltyPointsBalance` and a JS `cfg.loyaltyPointsBalance` on an untyped parameter produced an ACCESSES edge from the JS function to the Java private field. Confidence does not mitigate it, because `minConfidence` defaults to 0 — the tier is only a filter for consumers who ask for one. Candidates are now restricted to files in the language's own `parsedFiles`, which is a precise restriction rather than a heuristic and needs no new node property. Every other fixture in the suite is single-language, so this could not be caught anywhere by construction. The new fixture is deliberately polyglot and asserts both halves: no cross-language edge, and a same-language unique name still resolves. Known and not addressed here: the index is still O(total graph nodes) and is rebuilt once per qualifying language, the per-language whole-graph-scan pattern `phase.ts` hoisted out for `sharedNodeLookup`. Hoisting it belongs with that machinery rather than in this fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(processes): explore siblings in source order, log the exhausted budget `slice(0, maxBranching)` selected the FIRST N callees while `pop()` explored them LAST-first, so the trace budget went to the last-declared branch. For `main() { init(); loadConfig(); run(); shutdown(); }` the walk spends itself on `shutdown` and can drop `init` — the earliest steps of a flow, which is the opposite of what a process describes. Selecting first-N and exploring last-first was simply inconsistent; pushing in reverse makes the stack pop in source order. Measured on the reporting repo, this costs depth: 6-8 step processes go 168 -> 146 of 816. Still roughly three times the pre-PR baseline of 50, and the right trade — a deep branch is no longer reached by accident of being declared last. The remaining limit is the BUDGET, not the traversal: with a fixed quota a deep branch declared after enough shallow ones is not reached at all. That is now asserted in both directions rather than left implicit, and the walk logs when it stops with branches unexplored — a silently truncating cap reads as "this is everything", the same confident-empty answer this work is about, and the repo already sets that precedent for `dispatchFanoutSkipped`. Removes the second depth test, which was vacuous: the note twelve lines above it already said a `processProcesses`-level depth assertion passes under BOTH traversals, and measured it does — breadth-first yields the same deepest stepCount of 8, so it passed with the production change reverted. Traversal order is asserted against `traceFromEntryPoint` directly; what is observable at the pipeline level is which traces survive selection, which the diversity tests cover. Also renames `queue` to `stack` and corrects the BFS references in the module docstring and the function's own JSDoc, which is what an IDE hover shows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(impact): carry riskNote onto ambiguous candidates and separate UNKNOWN's two meanings Two problems on the ambiguous fan-out, which builds its own candidate object rather than returning the single-symbol shape. The narrowed type had no `riskNote` field and never read one, so a candidate that resolved and found no callers reported `risk: UNKNOWN` with nothing attached — losing the entire point of the change on the path where the reader has the least context, since the name is ambiguous there by definition. And `UNKNOWN` used to mean exactly one thing on this path: the probe threw. The zero-caller branch gives it a second meaning, so an all-UNKNOWN fan-out could no longer be told apart from a broken one. Candidates now carry `probeFailed`, and the comment asserting the old reading is corrected. Also aligns `gitnexus-web`, which review flagged as giving a different verdict for the same symbol. That surface answers in prose rather than an enum, and its message said the symbol "appears to be unused (not called by anything)" — the identical false certainty in words. It now carries the same MEANING rather than the same field. Downstream wording is unchanged: no outgoing dependencies really is a fact about the symbol itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: replace assertions that cannot fail Four from review, each satisfied by the defect it was meant to catch. `new Set(props).size === 2` over two different literal strings can only ever be 2, so it could not detect the node merge its title promises — that is a difference in COUNT, now asserted on the raw array. The ambiguity test asserted only an empty edge set, which is satisfied equally by "the gate fired" and "the name was never looked up". It now also requires the ambiguity counter to have moved. `Interface|Property` was listed as a structural-pair sentinel beside `TypeAlias|Property`, but both its labels are in the SCOPE_BRIDGE cross-product so the pair is generated by construction and the sentinel cannot fail. Dropped rather than left reading as coverage; `TypeAlias|Property` is the load-bearing one. `TypeAlias|Method` was declared in the schema with no fixture emitting it — a declared pair no emitter exercises is indistinguishable from a missing one until an analyze aborts on a real repo. Adds a method-shaped alias member, and the suite requires sentinels to actually appear, so it is not vacuous. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: document the new incomplete reason, the UNKNOWN verdict and the id churn Review found the code changes landed without the guidance around them, and an agent following this repo's own rules would have been told the wrong thing. `graph-write-collapsed` joined `INDEX_INCOMPLETE_REASONS` with no Sign block and no recovery section, while the precedent it cites (`embedding-checkpoint-pending`) has both — so `gitnexus status` would surface a new string naming silent wrong answers with nothing explaining trigger or remedy. Added to RUNBOOK and GUARDRAILS, including why this reason alone also fails the exit code. `AGENTS.md` said MUST warn on HIGH or CRITICAL and never mentioned UNKNOWN, and the shipped impact skill's risk table had no UNKNOWN row and still implied few-callers ⇒ LOW. An agent obeying those rules literally sees `risk: UNKNOWN` and proceeds, which negates the change the verdict exists to make. Both copies of both skills updated. `MIGRATION.md` now records that process ids do not survive this release — positional ids plus depth-first tracing, source-order siblings and round-robin selection mean `proc_7_handle` is a different flow afterwards. Bounded honestly: nothing in-repo joins on a raw process id, so it is index churn, not a broken consumer. `ARCHITECTURE.md`'s scope-resolution stage list gains the two new stages. The guide skill's node list gains `Property` and `TypeAlias` — the two node types this work most prominently creates. Also, on the pair-CSV preflight review asked to confirm: the hard abort IS deliberate, because a fallback recovering zero rows is the confident-empty failure this work targets. But the transient the message itself names — a second concurrent analyze sharing `.gitnexus/csv` — is a race, so the check now re-looks three times over ~150ms before declaring the file gone. Long enough to ride out a rename, far too short to mask a file that is genuinely missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: drop redundant TypeAlias pairs and keep bare identifiers off class members Two regressions the full suite caught after the review fixes, both real. `schema-pair-coverage` failed with eleven hand-declared pairs that a rule now generates. Adding `TypeAlias` to `LINKABLE_LABELS` — needed so `resolveDefGraphId` can bridge an alias def to its node — also makes it a SCOPE_BRIDGE source and target, so the cross-product produces `File|TypeAlias`, `TypeAlias|Property` and nine others that round 1 had declared by hand. Removed; the invariant is that no pair is both generated and hand-declared. This also changes what the structural-pair sentinel means, and the comment is corrected rather than left overstating it: `TypeAlias|Property` is no longer load-bearing because the label is off the generated grid — it is load-bearing because it now depends on `TypeAlias` being IN `LINKABLE_LABELS`. Remove it and the pair stops being generated while the hand declaration is gone, which is the same state that silently breaks alias consumer edges. `block-scope-shadowing` failed because a bare identifier resolved to a class `Property`. `class Box { baseUrl = '...'; pick() { const baseUrl = ...; return baseUrl; } }` linked the block-local read to `Box.baseUrl`, duplicating the legitimate `this.baseUrl` edge. A bare identifier is not a member access: with no receiver there is no object whose property it could be, and in JS/TS a field read needs `this.`. Receiver-less read/write sites no longer accept `Property` hits; callables stay reachable, so `cb = save` naming a top-level function is unaffected. That defect PREDATES this branch's TypeScript captures — JavaScript has emitted bare-identifier reads since A2 and no class fixture exercised the shadow. The TS parity added here is what surfaced it. Golden snapshot regenerated after verifying the drift line by line: exactly +5 USES from type annotations in the mini-repo, every pre-existing count unchanged, so nothing was rewired. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(scope-resolution): share the property-name index across language passes Review follow-up. `indexPropertyNodesByName` scanned every node in the graph and was rebuilt inside each qualifying language pass, reintroducing exactly the pattern `phase.ts` hoisted out for `sharedNodeLookup` — whose comment records why it matters: "the previous per-language rebuild burned that CPU+heap N times and, on a huge repo, a tiny language's full-graph copy overlapped the next language's — a real contributor to the scope-resolution memory peak." Built once in `phase.ts` beside `sharedNodeLookup` and `sharedFnNodeIndex`, and threaded through the same `prebuilt*` seam, so tests and isolated calls still build their own. Sharing is only safe because the per-language restriction MOVED rather than disappeared: the shared index is whole-graph, and candidates are filtered to the language's own files at lookup time. That also fixes a subtlety the per-language build had backwards — the cap now applies to the FILTERED set, so a name carried by forty properties across a polyglot monorepo but only two in the language being resolved is still answerable, where a global cap would have refused it. The tri-state at the lookup boundary is deliberate and the three outcomes are not interchangeable: no property of this name in this language (nothing to say, and NOT an ambiguity), too many to choose between (reportable), or a list to narrow. Caught mid-change by the polyglot fixture: an intermediate state shared the index without moving the filter, and the cross-language edge came straight back. That test earning its keep twice is the reason it exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): report when a field's only anchor is another language Round 3, found OUT-OF-SAMPLE — six field names appearing in no prior report, so nothing here was tuned against them. All six answered 0 backend ACCESSES while their definitions sat in `apps/research-dashboard/**`: TypeScript only. The in-sample set scored 5/5 and the out-of-sample set 0/6, and the gap is entirely this. Per-language inference (`3c5eadc7`) is right and stays. What was wrong is that declining is INVISIBLE: an empty result for a field anchored only in TypeScript is byte-identical to an empty result for a field nobody reads. One says "look in the other language or grep"; the other says "delete it". That is the same confident-empty failure this series exists to remove, one surface over — and this time the missing fact is about the ANALYZER's reach rather than the code. Declines are now counted and named, with the languages the anchors actually live in, kept SEPARATE from ambiguity because the remedies differ: ambiguity wants better receiver typing, this wants an anchor in the reading language. Collapsing them would tell a reader the wrong thing to do. A non-zero count warns at analyze time regardless of dev mode. The facts are published as `PipelineResult.propertyInference`, which they had to be for any of this to be testable — and that exposed a second defect. The round-2 ambiguity assertion, which I told the reviewer of #2856 I had strengthened, read its stat off a `scopeResolution` field that does not exist on PipelineResult: the `if (undefined) return` guard swallowed it and the test passed with the production code deleted. Both that assertion and the new ones now read the published field, and the guard is an assertion rather than an escape. Verified by deleting the counter and watching them fail. Reported by the same round-3 method note that caught it: verifying a fix against the cases it was written for only proves those cases pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(context): explain an empty property result caused by a cross-language anchor The other half of R3-1. The analyze pass now knows which fields it declined to link because every definition of the name lives in another language; this puts that fact where it is actually read. `context()` on such a field previously returned an incoming list byte-identical to a genuinely unread field. The two demand opposite actions — "look in the other language, or grep" versus "delete it" — so the difference has to travel with the answer: unresolved: property reads of this name were NOT linked: every definition of it is typescript, and name inference does not cross languages. An empty or short incoming list here is not evidence the field is unused — confirm with a text search, or give it an anchor in the reading language. anchorLanguages: ['typescript'] Carried through repo meta because the graph cannot answer it: the unlinked reads mint no edge and no node, so the only record is the pass that declined them. Keyed on the NAME, not on the resolved label. Gating on `=== 'Property'` was tried first and is wrong — the label reads `''` on this path for a plain Property node, so the gate silently suppressed the entire feature while every test still passed. Caught by asserting the field is DEFINED rather than guarding on it, which is the same anti-pattern that made two earlier assertions vacuous. The meta list only ever contains property names, so matching the name is itself the type check. Cached per (index, indexedAt): `ensureInitialized` deliberately avoids a per-call `loadMeta` because every tool routes through it, so this re-reads exactly when a re-analyze could have changed the answer and never otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): report declined property reads for opt-out languages too Generalizing R3-1 rather than waiting for it to be re-reported in the other direction. The reported case was a JavaScript read whose only anchor was TypeScript; the mirror — a TypeScript read anchored only in JavaScript — was still silent, because a language that sets `fieldFallbackOnMethodLookup: false` had the whole pass skipped, and skipping emission also skipped REPORTING. Detection is not inference. Counting what could not be linked asserts nothing about what it means, so `reportOnly` runs the pass for its facts while emitting no edge, and the opt-out keeps protecting exactly what it protected before. Two things this turned up that a single-instance fix would have missed: The cross-language fixture could NOT prove `reportOnly` is load-bearing — the per-language candidate filter already blocks those edges, so the assertion passed with the flag forced off. The case that discriminates is a SAME-language TypeScript read that name inference could legitimately link and the opt-out forbids; forcing the flag off there emits `readsTsOnly -> tsOnlyBudget`, which is the violation. Getting to that case surfaced a sibling gap, recorded but NOT fixed here: the object-literal `Property` rule is JavaScript-only, so `const CONFIG = { ... }` in a `.ts` file mints no node and its keys are invisible. The first draft of this fixture used exactly that shape and could not discriminate for that reason. It is the TypeScript half of R2-1a and wants its own change, not a rider on this one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(typescript): index object-literal keys, as JavaScript already did The sibling recorded in `0c5a4f64` and deliberately left out of it. Both the named object-literal rule (A1/A5) and the identity-wrapper rule (R2-1a) lived only in JAVASCRIPT_QUERIES, so the single most common config idiom in TypeScript — export const tsRuntimeConfig = { tsConfigRetries: 3 }; — minted no node for any key. `context()` answered "Symbol not found" and a precise read through the holding variable had nothing to resolve to. TypeScript sets `fieldFallbackOnMethodLookup: false`, so these gain no name-based inference. What they gain is the PRECISE path, which is the route TypeScript is meant to use: `tsRuntimeConfig.tsConfigRetries` has a typeable receiver and now resolves. A read through an untyped receiver stays unresolved and, since `0c5a4f64`, is reported as such rather than answering an empty set. Scoped exactly as the JavaScript rules are — bound to a variable, and for the wrapper only the three functions that return the argument they were given — with the same `Object.entries` negative control pinning the allowlist. Found by fixture, not by report: the first draft of the `reportOnly` test used a TS `const CONFIG = { ... }` as its discriminator and could not discriminate, because the shape mints nothing. That is the whole argument for sweeping a class instead of waiting for each instance to be filed. SCHEMA_BUMP 47 -> 48: parse-time, so a warm cache replays ParsedFiles carrying none of these matches and the keys stay invisible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): anchor anonymous returned object literals to their function The last gap round 3 named, and the dominant shape in idiomatic JS: 437 `return {` sites in a single backend directory of the reporting repo, including the ~25-field payload of its entire signal pipeline. The literal binds to nothing, so its keys could not even be named — "who reads wickRatio?" had no symbol to ask about. The enclosing FUNCTION is the owner: the literal is that function's return shape, a contract its callers consume. Keys qualify as `<function>.<key>`, so two functions returning the same name stay two shapes rather than one merged symbol, and multiple returns in one function stay distinct by position. RECONCILING THIS WITH R2-1b, which deliberately modelled returned keys as WRITES to avoid adding same-named competitors to narrowing. These are definitions, but narrowing now ranks DECLARED anchors — named literals, class fields, interface and alias members — strictly above return shapes. A name that already resolved keeps resolving to what it resolved to before, so the competitor problem R2-1b was avoiding cannot come back. Mutation-checked: dropping that ranking breaks five pre-existing R2 resolutions. That also required an R2-1b assertion to change, and the change is a strengthening rather than a concession. It asserted `toHaveLength(1)` — no new definition — as a proxy for "adding definitions must not move an existing answer". The proxy is now false while the property still holds, so the property itself is asserted directly. No `HAS_PROPERTY` edge from the function: that would be a `Function|Property` relation pair the schema does not declare, and an undeclared pair does not degrade — it throws and kills the whole analyze. That already shipped once in this PR. Two things found by dumping rather than assuming, both fixed here: SHORTHAND keys were not matched at all. `return { symbol, interval, score }` is the commonest spelling and the reporting repo's own payload is mostly this form, but tree-sitter models it as `shorthand_property_identifier`, which `(pair)` does not match. Caught by dumping the golden fixture and seeing a literal returning `{ level, message, timestamp: Date.now() }` had indexed only `timestamp`. Now covered in return position AND in the variable-bound rule, which had the same gap. Provenance was flagged by owner-presence, which mislabelled the anonymous case: a callback's return shape yields no name to qualify by, so it looked like a DECLARED anchor and would have outranked real declarations. Flagged by position now — a different question from whether a name could be derived. SCHEMA_BUMP 48 -> 49. Within one PR the version only has to differ from main's, but a build stamped 48 was installed and used to analyze before these captures existed, so caches stamped 48 carry none of them — the intermediate-build hazard this ledger already records for 33/34. Golden regenerated after verifying the drift: exactly +10 Property and +10 DEFINES, every pre-existing count unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(scope-resolution): rank production anchors above test fixtures Found by testing R3-4 on the reporting repo instead of on its fixtures. Anchoring returned literals took `wickRatio` from 6 definitions to 13 — and backend reads still resolved to nothing, because SEVEN of the new JavaScript anchors compete and four of them are in `tests/`. A test constructs throwaway shapes carrying production field names; a read in shipped code cannot mean one of them. Applied before the declared/return-shape split, because "is this the shipped program" is the stronger signal — a declaration inside a test fixture is still a test fixture. Skipped when the READER is itself a test, since a read there legitimately means the test's own shape. The first version of this test was vacuous and the mutation check caught it: the reader sat in the same file as the production anchor, so the same-file tier resolved it whether or not this tier existed. The reader now lives in a file that imports neither anchor, which leaves production-vs-test as the only thing that can decide. Honest about what this does NOT do: it narrows `wickRatio` from seven candidates to three, and three functions in different files each returning that field is GENUINELY ambiguous — refusing is correct, and the ambiguity is now counted and named rather than silent. The reported question ("who reads wickRatio?") is answerable only where one producer exists; where several do, the honest answer is the list of producers, which R3-4 made nameable for the first time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(scope-resolution): resolve members through a call result's return shape The question three rounds of reports could not answer, and the one narrowing must refuse by design: a field produced by SEVERAL functions. A read of `spike.wickRatio` could mean any producer, so name inference correctly declines and no amount of tier-tuning changes that. It needs evidence, not inference. The evidence existed in two halves that had never been joined. The call-result type binding (`const alert = formatSpikeAlert(row)` binds `alert` to a TypeRef whose rawName is the callee) predates all of this work; it simply had nothing to resolve to when the callee returned an anonymous literal, because an anonymous literal named nothing. R3-4 gave it a name. Joining them: const alert = formatSpikeAlert(row); alert.wickRatio -> Property:...:formatSpikeAlert.wickRatio Precise, at ordinary emission confidence, and it works EXACTLY where narrowing cannot: several producers sharing a field name stop being competitors because the receiver says which one. Runs before the name fallback and claims its sites, so a precise answer is never second-guessed by a name match. Measured on the reporting repo: 1,410 precise edges, and all six fields round 3 verified OUT-OF-SAMPLE go from 0 backend readers to 7, 11, 10, 7, 6 and 14. Round 3 scored 0/6 on that set; this is 6/6. The bound is asserted, not just documented: a read off a BARE PARAMETER has no binding here, because typing it needs the caller's type to flow in — that is inter-procedural and genuinely larger. Those reads still fall through to name inference and are still reported when it declines. The fixture has two producers sharing a field name precisely so the test cannot pass by name matching, and mutation-checking the owner lookup fails it. No SCHEMA_BUMP: this is scope resolution, not parse-time capture, so a warm cache already carries everything it reads. Noted in the ledger because the reflex on this branch has been to bump, and an unnecessary bump costs every user a full re-parse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Revert "return-shape anchoring" (R3-4/R3-5): it degrades query Reverts |
||
|
Gergő Magyar
|
7468cc915b
|
fix(analyze): replace the hand-incremented schema version with a derived DDL fingerprint (#2798) (#2808)
* feat(schema): derive a fingerprint from the DDL this build creates `SCHEMA_FINGERPRINT` is a sha256 digest of the node and relation DDL that `runSchemaCreationQueries` actually executes, in the same shape as the existing `taintModelVersion` stamp (hex, sliced to 12). It exists because `INCREMENTAL_SCHEMA_VERSION` is hand-picked and has to *predict* whether an on-disk database matches this build's DDL. That number has collided with `main` eight times, twice exactly — and an exact clash is the quiet one, because the reuse gate is a strict `===`. `EMBEDDING_SCHEMA` is deliberately excluded: its `FLOAT[N]` width comes from `GITNEXUS_EMBEDDING_DIMS` at module load, so folding it in would make the digest a function of the environment rather than of code, and two runs of the same build under different env would thrash full rebuilds. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(storage): record the DDL fingerprint in RepoMeta `RepoMeta.schemaFingerprint` stores the digest of the DDL an index's tables were actually created from. It is the derived companion to `schemaVersion`, not its replacement: both are compared, and both must match. Absent means mismatch, deliberately. Grandfathering a missing fingerprint would let an incremental top-up stamp a fresh one onto a database whose DDL was never verified, permanently certifying exactly the wrong-shaped index the field exists to catch. The cost is one full rebuild per pre-existing index. The version ladder gains a note that its "re-check against origin/main before merge" ritual now only guards *semantic* bumps. v25, v26, v30, v31 and v34 all changed emitted ids, edges or wire formats while leaving the DDL byte-identical, and the fingerprint cannot see any of them — but DDL collisions no longer need renumbering. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(analyze): gate index reuse on the DDL fingerprint, not just the version (#2798) `INCREMENTAL_SCHEMA_VERSION` is a hand-incremented integer that has to predict a derived fact: whether the on-disk DDL matches the code's DDL. It has collided with `main` eight times, and twice the collision was *exact*. An exact clash is the silent one. Two builds stamp the same number over different DDL, the `===` reuse gate reads the index as current, every `CREATE ... TABLE` is then skipped as "already exists" (suppressed in `runSchemaCreationQueries`), and the edges whose endpoint pair the live database cannot hold are dropped by `fallbackRelationshipInserts`' bare `catch`. The result is a wrong graph, with no error anywhere. Reuse now requires the version AND the DDL fingerprint to match, in both the pre-pipeline force-rebuild guard and the `isIncremental` predicate, and the fingerprint is stamped alongside the version at the end of a run. Both conditions are necessary. The fingerprint does not replace the integer: most entries in the version ladder change emitted ids, edges or wire formats while the DDL stays byte-identical, and a fingerprint-only gate would stop forcing rebuilds for all of them. What it does buy is that two branches picking the same number no longer need renumbering. The new branch sits above the `alreadyUpToDate` fast path for the same reason the version guard does — a clean tree at an unchanged commit would otherwise early-return before either check ran. Closes #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(analyze): pin the DDL fingerprint gate and its two failure cases `schema-fingerprint.test.ts` pins the properties the gate rests on: the digest covers exactly the node and relation DDL that gets executed (recomputed from the exported lists, so adding a table or a FROM/TO pair without the fingerprint moving is impossible), it excludes the environment-derived embedding DDL, and it moves when any covered string moves. The two `incremental-orchestration` cases exercise the production path rather than modelling it: an index carrying the *current* version with a foreign fingerprint, and one with no fingerprint at all. Both were run against the pre-fix tree first and both failed there with `alreadyUpToDate === true` — the fast path swallowing the mismatch, which is the #2798 symptom exactly. `call-summary-schema-version.test.ts` widens its gate model to two equalities. The second argument defaults to the current fingerprint so all 33 existing version cases read unchanged, and a new case covers the collision, the legacy absence, and the semantic bump the fingerprint cannot see. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(review-skill): point the schema-constant check at the fingerprint, not the deleted integer All four `gitnexus-review` SKILL.md mirrors told reviewers to verify `INCREMENTAL_SCHEMA_VERSION` "was bumped or regenerated". That constant no longer exists, so the instruction sent every future reviewer looking for something they could not find — and, worse, past its replacement. The check for graph DDL is now derived: `SCHEMA_FINGERPRINT` moves on its own, so the question is whether the diff changed a string in `NODE_SCHEMA_QUERIES` / `REL_SCHEMA_QUERIES`, and whether a newly added DDL array was folded into the fingerprint at all — the one way the derived gate can still be bypassed. What did NOT change is called out explicitly: the parse-store `SCHEMA_BUMP` and the bench fingerprint sets are still hand-maintained and still need the re-check-against-base ritual, and semantic changes that leave the DDL untouched fall outside the fingerprint entirely — those rely on the analyzer runner-identity receipt. Found by the review swarm's docs lane. The original plan for #2798 claimed no documentation mentioned the constant; that sweep covered five root docs and never looked at `.claude/skills/**` or the three mirrors. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(migration): record the one-time rebuild the fingerprint switch costs Replacing `schemaVersion` with `schemaFingerprint` means every index written by an earlier GitNexus carries no fingerprint, reads as a mismatch, and is rebuilt once. That is deliberate — grandfathering absence would stamp a fresh fingerprint onto a database whose DDL was never verified — but until now it was undocumented, so a user's first post-upgrade analyze would announce a full re-analyze with nothing to explain it. MIGRATION.md already sets the precedent: PR #2363's meta.json → gitnexus.json rename was equally automatic and equally in need of an entry. This follows that shape, and is explicit about the parts that are easy to undersell: - the cost is per INDEX, and branch-scoped slots (#2106) each pay separately; on a large repository a full re-analyze is substantial, not a blip; - rollback is safe — an older binary sees no `schemaVersion` and forces its own rebuild, which is a cost, never a stale graph; - alternating between an old and a new binary rebuilds on every switch, because the end-of-run meta is written as a fresh literal so neither field survives the other's run. The retired ladder's per-version rationale is pointed at in git history rather than reproduced: `git show 561f913a3:.../repo-manager.ts`. That commit is an ancestor of origin/main, so the pointer survives this branch being squash-merged. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(identity): cover workspace-linked packages in the analyzer dependency digest `dependencyNames` enumerated `dependencies`, `optionalDependencies` and `peerDependencies` only. `gitnexus-shared` is declared as a devDependency (`file:../gitnexus-shared`), and in a source-mode run the build root is the gitnexus package tree, which does not contain that sibling. So a change to gitnexus-shared moved neither `build.digest` nor `dependencyRuntime.digest`. That gap matters more since #2798 deleted `INCREMENTAL_SCHEMA_VERSION`. A DDL-affecting edit there is still caught by `SCHEMA_FINGERPRINT`, but a SEMANTIC-only edit — a new `REL_TYPES` member, say, where the relation table carries a bare `type STRING` column so no CREATE statement moves — was covered by nothing at all. Roughly thirty of the retired ladder's entries were exactly that change class, and the runner-identity receipt is what now carries them. Only checkout-local specifiers are added: `file:`, `link:`, `workspace:`, `portal:` and npm's bare local-path shorthands. Pulling in every devDependency was rejected — vitest, eslint and typescript would enter the digest and force a full re-analyze on unrelated tool bumps, which is worse than the hole. Scanning the linked sibling for the first time exposed a latent throw: `collectArtifacts` honoured `PRUNED_RUNTIME_DIRECTORIES` only for a real directory, so a SYMLINKED `node_modules` fell through to the payload branch and died with "Analyzer identity input is not a file". Worktree-style dev layouts and pnpm shared stores hit this immediately — verified in this worktree, where `gitnexus-shared/node_modules` is such a symlink. Pruning it loses nothing: packages beneath are still reached through `resolveDependencyPackageRoot`. Verified: a real `analyze` in this worktree succeeds with `packageCount` 259; editing the linked package's source moves the digest, bumping an installed registry devDependency does not, and removing the link moves it. `DEPENDENCY_RUNTIME_CANONICALIZATION` is deliberately not bumped — freshness compares digests, not the label, and the input-set change already moves them. Follow-up worth having: no fixture in the suite declares `devDependencies`, so this has no regression test yet. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(analyze)!: delete INCREMENTAL_SCHEMA_VERSION, gate reuse on the DDL fingerprint alone The integer and its ~180-line version ladder are gone, along with `RepoMeta.schemaVersion`. Index reuse is now decided solely by `SCHEMA_FINGERPRINT`; a mismatch — including the absent stamp every pre-existing index carries — warns and forces a full re-analyze, which wipes and recreates the database so the tables are built from the current DDL. Deleting the integer is safe because it was already redundant: the runner-identity guard deep-compares the whole schema-v4 receipt, including a digest over the build tree, and forces a rebuild on ANY analyzer delta. Verified empirically — a comment-only edit to logger.ts, with the fingerprint byte identical, produced "runner identity changed ... forcing a full rebuild". The fingerprint is not thereby redundant. It fires where that guard cannot: a DDL-affecting change in `gitnexus-shared`, which is a workspace-linked devDependency and so sat outside both digests until the companion commit closed that gap. Review findings folded in, each correcting a line this rewrite itself introduced and never published: - B1: two assertions matched a log string the rewrite had renamed; both tests failed. They now assert what production emits. - B2: the pre-existing downgrade test perturbed `schemaVersion: 7`, a field this change deletes, so the spread carried a valid fingerprint, every guard passed, and the run legitimately took the fast path. It perturbs the fingerprint now, restoring the only integration coverage of the gate-above-the-fast-path ordering invariant. - N5: duplicate `schemaFingerprint` keys silently collapsed two assertions into one (TS1117). - N6: the absent-stamp message told non-git repositories their index was "built by an older GitNexus version" — on every run, about an index this exact build had just written. Non-git repos never record a fingerprint, and now the message says so. - N9: the on-disk stamp is shape-checked before being echoed, so a crafted gitnexus.json cannot push ANSI escapes through the CLI log. - N7: a test case that re-computed the same digest expression with its operands swapped, mislabelled as a randomness check on a module-level const. - N10: comments claiming the digest "cannot collide" (it is 48 bits), pointing at a vector-column gate that does not exist, and asserting storage/ is free of a core/ dependency two lines below a core/ value import. None of these were caught by `tsc -p tsconfig.json`, which covers src only, nor by eslint, where no-dupe-keys is off. `tsconfig.test.json` reports all three test defects and is not currently wired into CI. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(schema): pin that the fingerprint covers every DDL statement init executes `SCHEMA_QUERIES` is the list `runSchemaCreationQueries` iterates — the DDL that actually runs. The fingerprint hashes only two of its three members, and until now no test imported `SCHEMA_QUERIES` at all, so nothing tied the two together. A fourth member appended to that array — the one literally named for what init executes — would have been invisible to the gate. Every existing test would still pass, because they all recompute the digest from the same two arrays the fingerprint already uses. An index whose gate passed would then run `initLbug` over the old database, where `runSchemaCreationQueries` suppresses "already exists", so the new table would never be created and its edges would be dropped by `fallbackRelationshipInserts`' bare catch. A wrong graph, no error — exactly the failure #2798 exists to end. The check is a pure predicate over (executed, fingerprinted, documented exclusions) rather than a positional `toEqual`, so `EMBEDDING_SCHEMA` is named as an exclusion with its reason — its FLOAT[N] width is environment-derived — rather than sitting in a list where a future reader might "fix" it by folding it in. It asserts both directions and is order-insensitive, leaving ordering to the digest assertion that already pins it. The negative case is pinned in CI rather than checked by hand once: the same predicate over a synthetic fourth member must report it. If a refactor ever makes the predicate vacuous, that case fails even though the positive one would not. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(analyze): name the invariant the version deletion now rests on Deleting `INCREMENTAL_SCHEMA_VERSION` moved a load-bearing guarantee into an implicit one. Roughly thirty of the retired ladder's entries changed no DDL at all — node ids, wire formats, resolution tiers — and the fingerprint is structurally incapable of firing on any of them. Their only remaining cover is the analyzer runner-identity receipt, and nothing in the suite said so. This adds a table over the real `analyzerRunnerIdentitiesEqual` with a well-formed schema-v4 receipt: byte-identical reuses; an entrypoint-only difference reuses (CLI vs analyze worker); a moved build digest with unchanged DDL forces — that case IS the invariant, commented as such; and a dependency change, an ABI change, undefined, null, a schema-v3 legacy receipt, a missing build section and a non-sha256 digest all fail closed. The deleted `expect(INCREMENTAL_SCHEMA_VERSION).toBe(35)` pin is also worth naming: it failed CI on every bump by design, which is what made an author stop and think. Nothing replaced it. This does not restore that — a digest has no literal to pin — but it does make the mechanism that took over the job visible to the next person who reads the file. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(spring): pin CLASS_SCHEMA's membership in the fingerprinted DDL set When `INCREMENTAL_SCHEMA_VERSION` went away, its sibling in basicblock-callee-ids-schema.test.ts got a replacement assertion tying BASICBLOCK_SCHEMA to the fingerprint's input set. This file's `>= 23` floor was deleted with nothing put in its place. The file still asserts CLASS_SCHEMA's CONTENT — that the `frameworkAnnotations` column exists — but not that CLASS_SCHEMA is part of what the digest covers, and the second is what makes an index built before that column carry a different fingerprint and get rebuilt. Mirrors the sibling so the two read the same way. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(identity): stop a symlinked directory from aborting the whole analyze `collectArtifacts` fused two orthogonal facts into one condition: that four directory names never carry runtime payload, and that a symlink where a real directory was assumed falls through to the payload branch, where `snapshotReadableFile` stats the target, sees a directory, and throws "Analyzer identity input is not a file". The second was only fixed for those four names. Every other symlinked directory in a scanned package root still aborted the run — `dist -> build`, a vendored grammar link, anything inside a linked sibling checkout. Newly reachable, because making workspace-linked packages scannable pointed the scanner at a live checkout instead of an immutable registry tarball for the first time. Split along the actual seam: prune on the NAME alone, and give symlinks their own branch in the type dispatch, ahead of the payload branch. Link text is recorded rather than followed. Following was rejected on three grounds, each checked in source: the traversal is a stack with no visited set, so a self-referential link would recurse to `runtimeDepth` — which throws, trading one hard abort for another; `snapshotDirectory` rejects a symlink outright, so the directory guard could not accept one without a realpath rewrite of its canonical-path identity; and a link into an already-scanned tree double-counts against `runtimeEntries`/`runtimeBytes`, which also throw. The cost is stated in code: a link out of the package contributes its text, not its target's content. Links resolving to a regular file keep the existing content digest. The new `'unfollowed-symlink'` kind is threaded through every consumer, including the cache validator — which re-probes with `mode: 'link'`, since the readable-file probe resolves the target and would return null for exactly this kind, silently failing every warm validation. No canonicalization or cache-schema bump. Digest content changes only for trees that previously crashed: a delta scan over all 258 scanned roots of this install found no regular file bearing a pruned name and no symlink failing to resolve to a file, so `dependencyRuntime.digest` is byte-identical here. Six of the eight new tests fail against the unfixed tree with the exact production error; all eight pass after. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(analyze): give the reuse gate a real seam and sanitize logs at the funnel Cleanup pass over the #2798 branch. Net -183 lines. The gate had no extracted predicate, so its own test asserted it by regex-matching run-analyze.ts SOURCE TEXT. That pinned production formatting: one pattern froze three back-to-back single-name imports from './lbug/schema.js', so merging them — the obvious tidy-up — failed a test named "still imports the DDL digest itself". `schemaFingerprintMismatch` and `isSchemaFingerprintShaped` now live in core/lbug/schema.ts beside the constant. Not in run-analyze.ts next to `pdgModeMismatch`, because storage/ must stay off the analyze pipeline and mcp/resources.ts is a plausible second consumer — the same reasoning that puts `cjkSegmentationModeMismatch` in core/search/. The regex block is gone; the test calls the predicate. The three imports are merged. ANSI sanitation moved from one field to the funnel. The per-field guard's own comment stated the general hazard — gitnexus.json is parsed with no runtime shape validation and the notice reaches console.log — while two sibling guards twelve lines away echoed `runnerIdentity.schemaVersion` and `cjkSegmentation` from that same file raw into the same log. `log()` now strips C0/C1 controls, covering all seven guard messages and any written later. Also: - Deleted a duplicate integration test. After the downgrade test was repointed at `schemaFingerprint` it became the same scenario as the new one, differing only by an extra log assertion — which is now folded into the survivor. Saves a fixture and two full pipeline runs per CI pass. - Replaced a 3-parameter set-difference helper with one set equality. Its doc was false at one call site (arguments semantically swapped) and it needed a fourth test purely to prove itself non-vacuous; set equality cannot go vacuous. - Removed ~115 lines of runner-identity table that duplicated analyzer-identity.test.ts. The three genuinely uncovered cases moved there, and the #2798 invariant — build digest moved while the DDL did not — now asserts against a REAL analyzer-build-tree edit rather than a hand-built literal, which is strictly stronger than what it replaces. - MIGRATION.md quoted a log line the code cannot emit; it was written before the placeholder changed. - Restored the rationale on the `capabilities` docstring, which a previous pass replaced with its consequence — leaving a maintainer reading "duplicated by hand" as a wart to fix by importing, which is what the original forbade. - Marked the `isIncremental` conjunct as belt-and-braces: `!options.force` short-circuits before it, so it cannot decide anything. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(analyze): force a rebuild when the vector column width changes `CodeEmbedding.embedding` is declared `FLOAT[EMBEDDING_DIMS]`, resolved from `GITNEXUS_EMBEDDING_DIMS` at module load. Nothing gated it. Flip the variable on a same-commit clean tree and no guard fired at all: `alreadyUpToDate` returned over a `FLOAT[384]` table while the process embedded at 768. The only reaction anywhere discards the embedding CACHE and re-embeds — into a column whose type it never revisits. This predates #2798; `INCREMENTAL_SCHEMA_VERSION` never covered dims either. It surfaced because the fingerprint work had to reason about why `EMBEDDING_SCHEMA` must stay OUT of the digest: its width is environment-derived, so folding it in would make the same build disagree with itself and thrash rebuilds. That exclusion is correct, and it leaves the width needing its own guard. Modelled on `cjkSegmentation`, the closest sibling: an env-resolved scalar stamped at write time and compared by a small exported predicate that forces on mismatch. `embeddingDimsMismatch` sits in core/lbug/schema.ts beside `EMBEDDING_DIMS`, so the query side can adopt it without importing the analyze pipeline — mcp/local/local-backend.ts already warns on a cjkSegmentation disagreement and has the identical claim here, since the query path embeds at the live width against a table of unknown width with no validation at all today. ABSENCE IS NOT A MISMATCH, deliberately. Forcing on it would be dead code: `embeddingDims` and `schemaFingerprint` ship together, and a missing fingerprint already forces exactly one rebuild — which is where this stamp lands. Absence also carries no signal here, unlike the fingerprint: a missing fingerprint means "DDL this build cannot vouch for" and ships WITH a DDL change, whereas a missing dims stamp means only "written before the field existed", and that run's table agreed with that run's width. Drift requires the env to change, which absence says nothing about. The `cjkSegmentation` trick of folding absence into the default was unavailable — there is no width that is safe to assume for an existing table — so the stamp is instead written unconditionally, giving absence exactly one meaning. Malformed values are not grandfathered: null, '384', NaN and objects all read as a mismatch and err toward a rebuild. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(mcp): warn when the served index's vector width differs from the query embedder's The analyze side now forces a rebuild when the vector column width changes. The query side had no equivalent: a serving process embeds a query at its own width and searches a table whose width was fixed when the index was built. Disagree and the user gets wrong or missing semantic results with nothing explaining why. Mirrors the cjkSegmentation drift warning immediately above it — same warnings[] array, same per-query recomputation, agent-visible in the tool response, and it warns rather than refuses. A width mismatch degrades the semantic lane only; keyword results are unaffected, so `partial` is deliberately not set. Compares against `getEmbeddingDims()` — the width the query embedder actually produces — NOT schema.ts's `EMBEDDING_DIMS`. The two diverge exactly when GITNEXUS_EMBEDDING_DIMS is set on a server that embeds LOCALLY: the query path ignores that variable and embeds at 384, so comparing against the env-derived constant would report drift on a lane that works fine. The recorded width is what the vector CAST actually binds. `embeddingDimsMismatch` is imported from core/lbug/schema.js rather than restated, so "absent is not a mismatch" cannot drift between the analyze and query sides. That predicate was placed in schema.ts precisely so this consumer could reach it without importing the analyze pipeline. Two gates keep it quiet when it would be noise: it fires only for a repo where this process actually produced a query vector, so an index analyzed without --embeddings (or a server whose embedder is unavailable) never carries it. An untrusted recorded value — meta.json is schema-less JSON — is reported as "an unrecognized width" rather than echoed. `loadMeta` is hoisted out of the neighbouring try so both diagnostics share one read and an invalid GITNEXUS_FTS_CJK_SEGMENTATION cannot take this one down with it. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(identity): detect an npm-linked dev dependency the specifier cannot see `isLocallyLinkedSpecifier` admits a devDependency whose SPECIFIER is checkout-local. `npm link <pkg>` leaves the specifier a registry range while the node_modules entry symlinks to a checkout — locally linked, invisible to a specifier check, so a semantic-only edit there still moves neither digest. The obvious placement is unaffordable, measured rather than assumed: probing every dev-only name inside collectRuntimePackages costs 1998 resolutions, not the ~8 it looks like, because dependencyNames runs for every package in the BFS and published tarballs retain their devDependencies. Persisted path guards go 2221 -> 11050 (+398%), and every guard is re-probed on each warm validation — the path `status` takes. Scoped to the root package instead. The declared-intent half is untouched and still enumerated everywhere: it alone can emit the `<missing>` edge for a declared link whose checkout is absent, where resolution returns null and cannot distinguish that from an uninstalled dev tool. The new resolved-location half runs only when `parent.root === packageRoot`, resolves through the existing resolver so its path guards are recorded, and admits a name iff the realpath'd root carries no node_modules segment. Bounded against mis-fire by EXPANSION. "Not under node_modules" is a proxy for "checkout-local"; under a relocated pnpm virtual store every dev dep passes it and the whole dev tree folds into the receipt — against limits that THROW, so a legitimate install would abort. Measured here: uncapped, that shape takes 259 -> 347 packages and 2250 -> 3786 guards. The cap admits at most four and DROPS THE WHOLE CHANNEL on overflow rather than an arbitrary prefix, because the abort comes from the transitive payload of whichever trees get folded in — four of a mis-fired thirteen is still unbounded, and a sorted-prefix receipt would be arbitrary. Overflow falls back to the specifier-only receipt that ships today. Cost on this install: 259 packages unchanged, 13 dev names resolved, guards 2221 -> 2250 (+29, +1.3%). Verified against the real implementation, not just a replay: validation guards 16295 -> 16324, packageCount and artifactCount unchanged, and `dependencyRuntime.digest` byte-identical — so this forces no re-analysis for anyone. Each test fails on the defect it targets: disabling the channel kills the npm-link and cap cases; dropping the root-only scope makes the differential guard-count case fail at 2.8x guards; removing the specifier half kills the `<missing>` case. Refs #2798 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Gergő Magyar
|
990d79ba8c
|
fix(mcp): make impact/context reproducible — deterministic ordering on every capped query (#2787) (#2796) | ||
|
Gergő Magyar
|
89bbdcf566
|
fix(ingestion): stop double-indexing const X = () => {} as Function + edgeless Const twin (#2687) (#2691)
Some checks failed
CodeQL / Analyze (javascript-typescript) (push) Waiting to run
CodeQL / Analyze (python) (push) Waiting to run
Gitleaks / gitleaks (push) Waiting to run
Publish / Classify release event (push) Waiting to run
Publish / RC guard (marker + release-PR skip) (push) Blocked by required conditions
Publish / ci (push) Blocked by required conditions
Publish / Publish to npm (push) Blocked by required conditions
Publish / Build & Push RC Docker images (push) Blocked by required conditions
Scorecard / Scorecard analysis (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-cli) (push) Waiting to run
Trivy Image Scan / Trivy (gitnexus-web) (push) Waiting to run
Devcontainer Smoke / Config-transform unit tests (push) Has been cancelled
Devcontainer Smoke / Build devcontainer image (push) Has been cancelled
Skill copy sync / shipped skills drift guard (push) Has been cancelled
|
||
|
Livio Gamassia
|
d546fa3cce
|
fix(storage): rename index metadata to gitnexus.json with dual-write compatibility (#2363) | ||
|
azizur100389
|
131d411ae4
|
feat(mcp): rank context/impact disambiguation candidates and expose kind/file_path hints (#888)
* feat(mcp): rank context/impact disambiguation candidates and expose kind/file_path hints
The `context` MCP tool already returned `{ status: 'ambiguous', candidates }`
when a name hit multiple symbols, but the candidates were returned in
arbitrary DB order and the only hint it accepted was file_path. The
`impact` tool was worse: when its name resolver found multiple viable
matches it silently picked the first one from a priority UNION, with no
signal back to the caller that a different symbol might have been
intended.
Both failure modes were flagged in issue #470 and reconfirmed in the
comments by a second user who described impact as returning "incorrect
parsing results and meaningless tool calls" in the multi-match case.
Changes:
* Add `resolveSymbolCandidates(repo, query, hints)` private helper on
LocalBackend. Single place that:
- Short-circuits on direct uid (zero-ambiguity)
- Runs the same name-or-qualified-id match as before, with LIMIT 20
(was 10) so the ranker has headroom instead of arbitrary truncation
- Preserves the #480 Class/Constructor preference -- when the only
ambiguity is a Class and its own Constructor, the Class wins
silently
- Scores each candidate (pure TS, no extra DB round-trip): base 0.50,
+0.40 for file_path match, +0.20 for kind match, plus a small
kind-priority tiebreaker (Class > Interface > Function > Method >
Constructor) when no explicit kind hint is given
- Sorts desc by score with stable tiebreakers (shorter filePath,
then lex uid)
- Promotes to a single confident resolve when the top score is
>= 0.95 AND beats the runner-up by >= 0.10 -- lets a strong hint
cut through without forcing the caller through a disambiguation
round-trip
* Rewire `context()` to use the shared helper. Response shape is a
strict superset of today's: candidates gain a `score` field, the
existing `{ uid, name, kind, filePath, line }` keys are preserved so
every downstream consumer (rename, eval-server formatter, etc.) keeps
working. New `kind` input hint accepted.
* Rewire `impact()` to use the shared helper. Now emits the same
`{ status: 'ambiguous', candidates, impactedCount: 0, risk: 'UNKNOWN' }`
shape instead of silent first-pick. New inputs accepted:
`target_uid`, `file_path`, `kind`.
* Update tool schemas in mcp/tools.ts to advertise the new inputs and
describe ranked disambiguation.
Backward compatibility:
The #480 Class/Constructor collapse is preserved and covered by the
existing java-class-impact integration test (still green). The
ambiguous response shape is a strict superset -- `eval-formatters`
unit test that parses the old shape is unchanged and still passes.
`impact` going from silent-first-pick to structured ambiguous is a
semantic improvement that is the entire point of the issue; callers
relying on silent first-pick now get an actionable response.
Scope declined for v1:
module/community hint -- the issue lists it as one of several hints,
but kind + file_path cover the vast majority of disambiguation needs
in practice, and a community-label filter requires an extra graph
query per candidate. Natural v2 follow-up.
Tests: calltool-dispatch.test.ts gains 5 new cases covering file_path
boost, kind hint boost, impact ambiguous shape, impact target_uid
short-circuit, and score field presence on the existing ambiguous
test. Plus the extended assertions on the existing
`context tool returns disambiguation for multiple matches`.
Verification:
npx vitest run test/unit/calltool-dispatch.test.ts -> 64 pass
npx vitest run test/integration/java-class-impact.test.ts -> pass
npm run test:unit -> 3642 pass
(4 pre-existing env failures unchanged: skip-git-cli needs built
dist/, git-utils tmpdir on Windows worktree -- same on main)
npx tsc --noEmit -> clean
Closes #470
* fix(mcp): enrich labels from UNION when labels(n)[0] is empty; address review findings
CI on PR #888 caught 13 integration-test failures I did not cover locally:
my resolver refactor collected candidates via `labels(n)[0] AS type`, but
LadybugDB returns an empty string for that projection on certain node
types (most importantly Class). With an empty `type`, impact's downstream
`_runImpactBFS` no longer recognised `symType === 'Class' | 'Interface'`
and stopped seeding Constructor + File nodes into the frontier, so the
"impact(upstream) surfaces the file importer" assertion broke across 11
language fixtures plus 2 OVERRIDES filter tests.
The original impact resolver worked around this by running a prioritised
UNION across Class/Interface/Function/Method/Constructor and picking the
first hit. My refactor dropped that. Fix: keep the simple candidate MATCH
but enrich types afterward via a single scoped UNION query, so every
candidate carries an accurate label for both scoring and downstream
BFS seeding. The UID direct-lookup path is patched the same way.
Also addresses the findings from the senior reviewer on PR #888:
* MIGRATION.md: document the `impact` behavioural change (silent first-
pick → structured `{ status: 'ambiguous', candidates }`) so downstream
callers know to branch on `result.status` before reading byDepth/
summary. `context` is unchanged shape-wise (strict superset).
* New test: `context tool promotes top candidate via scoring when
multiple rows survive DB pre-filter`. The review flagged that the
existing file_path test works only because the mock ignores WHERE
parameters -- the scored-promotion path (top ≥ 0.95 AND gap > 0.09)
wasn't directly exercised. The new test uses two candidates both in
App.tsx-containing paths plus a kind hint so promotion is decided by
scoring, not DB pre-filtering. Also tightened the comment on the
earlier file_path test to describe the mock vs production divergence
honestly.
* NIT: added a paragraph explaining why `scored.length >= 2` is kept as
a defensive guard even though the `normalized.length === 1` early
return already covers the single-candidate path.
* Integration: two tests in `local-backend-calltool.test.ts` targeted
`'authenticate'`, which now correctly resolves as ambiguous (two
Method nodes: AuthService.authenticate and BaseService.authenticate).
Updated both to pass `file_path: 'src/auth.ts'` so they exercise the
new disambiguation API and still assert the METHOD_OVERRIDES filtering
they were originally about.
Edge case fix in the promotion gap check: IEEE754 makes 0.50 + 0.40 +
0.20 - 0.90 = 0.09999999999999998 instead of exactly 0.10, which would
otherwise break the "winner clearly dominates" intent for legitimate
1.00 vs 0.90 cases. Changed `>= 0.10` to `> 0.09`; same user-facing
intent, no floating-point sensitivity.
Verification (all from gitnexus/):
npx vitest run test/integration/class-impact-all-languages.test.ts
-> 52 pass (was 11 FAIL on CI before this fix)
npx vitest run test/integration/local-backend-calltool.test.ts
-> 18 pass (was 2 FAIL on CI before this fix)
npx vitest run test/integration/java-class-impact.test.ts
-> 10 pass (regression guard for #480 preserved)
npx vitest run test/unit/calltool-dispatch.test.ts
-> 65 pass (1 new test + 4 from original #470 PR)
npm run test:unit
-> 3626 pass, 4 pre-existing env failures unchanged
npx tsc --noEmit
-> clean
|
||
|
Gergő Magyar
|
0561d24efd
|
feat: METHOD_IMPLEMENTS edges, overload disambiguation, MethodExtractor unification (#574) (#642) |