scriptoriumadmin/GitNexus
1
0
Fork 0
mirror of https://github.com/abhigyanpatwari/GitNexus.git synced 2026-08-28 05:25:25 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 9

fix(scope-resolution): a closure binding is a call SOURCE in every language, and function-local values carry their own identity (closes #2699) (#2718)

Browse source 
* test(scope-resolution): audit the consumers of file-scoped node ids (#2699 part A)

#2699 item 4 — "audit consumers that assume file-scoped ids" — after #2695/#2714
gave function-local CALLABLES position-bearing ids. Tests and findings only; no
production change. That split is deliberate: `impact` reports
`resolveDefGraphId` at CRITICAL with 23 DIRECT dependents across 7 modules
(every language MRO builder, both Spring attachers, C++ member lookup,
tryEmitEdge, emitReferencesViaLookup, buildGraphTargetIndex, emitFreeCallFallback,
emitReceiverBoundCalls, preEmitInheritanceEdges, emitDetectedInterfaceImplementations,
phpEmitUnresolvedReceiverEdges, emitRubyMixinEdges, emitRustTraitImplEdges,
emitDartHeritageEdges), so changing that key chain is its own change, not a
rider on an audit.

A2 — detect_changes: CONCERN RESOLVED, now pinned. The worry was that an id
containing `@row:col` re-keys whenever a declaration MOVES, making every edit
look like symbol churn. It cannot: `local-backend.ts` maps diff hunks to
symbols by LINE-RANGE OVERLAP (`n.startLine`/`n.endLine`) and merely REPORTS
`n.id`. Node identity never participates in the match. New structural test
asserts the WHERE clause never gains `n.id =` or `n.id IN`, keeps the one
legitimate id-shaped predicate (the `BasicBlock:` prefix exclusion, #2082 U7),
and confirms the id is returned rather than matched. Structural in the same
idiom as `detect-changes-worktree.test.ts`, and labelled as not proving runtime
behaviour.

A1 — ANSWERED, and the answer is that #2699 is NOT fully closed by items 1-3.
The fail-closed guard is gated on `isOverloadableCallable`
(Function | Method | Constructor), so a function-local VALUE never reaches it.
Measured on a fixture: a top-level `const handler` and a function-local
`const handler` still produce ONE node, `Const:v.ts:handler`. That is the
residual half of the issue's original complaint. Pinned as a KNOWN LIMIT with
its reason (widening identity to values re-keys ~14,700 build-time nodes to
change ~800 persisted ones — the decision recorded in `parse-worker.ts`), and
deliberately NOT fixed here.

A3 — id-persisting consumers, classified:
  - detect_changes ................ SAFE (position-keyed; pinned by A2)
  - MCP impact/context/trace ...... SAFE (resolve by name/uid at query time)
  - bench fingerprints ............ SAFE (digest capture shape, not node ids)
  - rust-captures golden .......... SAFE (digests captures, not ids)
  - cfg pipeline-pdg snapshot ..... AT RISK by design — pins exact edge ids, so
    it trips whenever attribution changes. That is the gate working; #2714
    already exercised it.
  - wiki / group-contract links ... NOT id-keyed on locals (locals are never
    cross-file addressable, per the document-scoped contract of item 2).

Verified: tsc clean; 14/14 across the two touched files; `detect_changes`
reports 0 changed symbols (tests only).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR

* fix(php): a closure binding is a call SOURCE, not only a TARGET (#2699 part B, S1)

A call made inside a closure binding was attributed to the ENCLOSING scope, so
the closure was a call TARGET but never a call SOURCE: impact(handler,
direction:"downstream") reported nothing even though the closure calls out.

Root cause, probe-measured rather than inferred. Instrumenting
pickCallerCallableDef (graph-bridge/ids.ts) to log every rejection reason shows
the closure's own scope EXISTS and its range DOES contain the call site, but its
ownedDefs is EMPTY, so the ":94" owned-callable filter drops it and attribution
falls through to the ":97" enclosing-scope fallback.

The reason is one missing query rule. javascript/query.ts pairs the binding name
with the closure via @declaration.function anchored on the INNER arrow node, so
anchor.range equals the @scope.function range and pass2AttachDeclarations
attaches the declaration to the CLOSURE's scope. No other language had that
rule — PHP, Rust, Kotlin, Ruby and Dart all captured named function
declarations only. That single omission is the entire empty-ownedDefs cause.

This ports the rule to PHP with the same anchor discipline (@declaration.function
on the inner anonymous_function / arrow_function, NOT on the
assignment_expression wrapper). PHP needs nothing else: it already declares
(anonymous_function) and (arrow_function) as @scope.function, so the rule alone
completes it.

Measured on a fixture: `$handler = function ($x) { return target($x); }` inside
outer() now emits

  Function:src/a.php:outer.$handler@3:2 -> Function:src/a.php:target

where it previously emitted `outer -> target`.

The pinned test in closure-binding-labels.test.ts asserted the OLD, wrong
behaviour by design ("to catch that asymmetry changing in EITHER direction"), so
it is INVERTED here rather than deleted, per its own instruction. Its block
comment is corrected to record the measured root cause, including that Kotlin
and Ruby will need BOTH this rule AND a relaxed kind gate (their lambda_literal
/ do_block is @scope.block deliberately, #1757), and that Dart has no closure
scope at all.

Verification: closure-binding-labels 50/50; PHP resolver suites 221/221
(php, php-coverage, php-response-shapes). detect_changes {staged}: 1 changed
symbol (PHP_SCOPE_QUERY), 0 affected processes, risk LOW.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR

* fix(rust): emit a node for a closure binding and make it a call SOURCE (#2699 part B, S3)

Rust was the one exception to #2687's "a closure bound to a name is a Function
node in every language": `let handler = || target(1);` produced NO graph node at
all, so the closure could be neither a call target nor a call source.

Needed BOTH query channels, which is the finding worth recording. Porting only
the scope-resolution rule (as S1 did for PHP) changed nothing measurable here,
because there was no node to attribute anything to:

  - languages/rust/query.ts — closure-binding declaration, @declaration.function
    on the INNER closure_expression so anchor.range aligns with the existing
    (closure_expression) @scope.function. This is what gives the closure's own
    scope a callable in ownedDefs, which is what stops pickCallerCallableDef
    falling through to the enclosing fn.
  - tree-sitter-queries.ts — @definition.function on the OUTER let_declaration.
    This emits the Function NODE that Rust never had.

Note the deliberate anchor asymmetry between the two channels: the graph-node
channel anchors the WRAPPER (matching the existing
(lexical_declaration (variable_declarator ... (arrow_function))) rule), while
the scope-resolution channel anchors the INNER closure (to align with
@scope.function). Getting these backwards silently produces either no node or
an unattributable one, so both sites carry a comment saying so.

Measured on a fixture — `let handler = || target(1);` inside outer():

  Function:src/a.rs:outer                CALLS  Function:src/a.rs:outer.handler@2:4
  Function:src/a.rs:outer.handler@2:4    CALLS  Function:src/a.rs:target

Previously the whole binding was absent and the call read as `outer -> target`.
The rule also covers `move` closures: the closure_expression node spans the
`move` keyword.

Verification: closure-binding-labels 50/50; rust.test.ts 192/192;
rust-coverage, rust-f70, rust-scope all pass; rust-captures-golden passes
UNCHANGED, so no golden regeneration was required. detect_changes {staged}:
2 changed symbols (RUST_SCOPE_QUERY, RUST_QUERIES), 0 affected processes,
risk LOW.

One caveat on the suite runs: this host times out `beforeAll` hooks at the
default 60s under load — rust.test.ts needed --hookTimeout=600000 to complete,
and a concurrent second vitest run starves worker startup entirely (every test
fails at ~5001ms). Both are host artifacts, not signal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR

* fix(kotlin,ruby): a closure binding is a call SOURCE, via a Block-scope callable boundary (#2699 part B, S2)

Kotlin and Ruby anchor a closure on a Block-kind scope — Kotlin lambda_literal
and Ruby do_block/block are @scope.block DELIBERATELY (#1757 smart casts), so
they must not be re-kinded. pickCallerCallableDef gated its child-scope walk on
kind === 'Function', so a closure there could never become a call SOURCE.

Both halves are required; neither alone changes anything:

1. kotlin/query.ts and ruby/query.ts gain the closure-binding declaration rule,
   with @declaration.function on the INNER lambda_literal / block so its range
   aligns with the @scope.block range (the anchor discipline documented in
   javascript/query.ts). Without this the closure scope owns no callable def.

2. pickCallerCallableDef accepts a Block-kind child as a callable boundary when
   the scope IS that callable's body. Without this the kind gate still rejects.

The alignment test in (2) is the part worth scrutiny. Relaxing the kind gate to
accept ANY Block owning a callable would be a real regression: a nested
`fun foo()` declared inside a block is owned by that block, so a call made at
BLOCK level — outside foo — would be misattributed to foo. Comparing the def's
declaration position against the scope's start position discriminates them: for
a closure the declaration and the scope sit on the SAME node, so the positions
match; for a nested function the block starts at `{` while the def starts at the
declaration, so they do not. Existing Function-kind behaviour is untouched, so
every already-working language is unaffected by construction.

The comparison is base-safe: scope-extractor.ts builds a def id as
`def:<filePath>#<startLine>:<startCol>:<type>:<name>` from the same Range a
scope carries, so both sides share one coordinate base. This is called out in
the helper's docblock because `defStartLine` nearby documents its own output as
1-based, which invites a wrong "fix" (#2377 is exactly this class of hazard).

Ruby's call forms are restricted to lambda/proc by name: an unrestricted
(call block: (block)) would match ANY method call taking a block, so
`mapped = items.map { |i| ... }` would wrongly declare `mapped` a callable.
Verified against the parser: 3 matches (->, lambda, proc), map excluded.
Separate #eq? patterns rather than one #match? alternation, which is a known
hazard on this tree-sitter line.

Measured on fixtures:

  Kotlin  Function:src/A.kt:outer.handler@2:4  CALLS  Function:src/A.kt:target
  Ruby    Function:src/a.rb:outer.handler@4:2  CALLS  Method:src/a.rb:target#1

previously `outer -> target` and `outer#0 -> target#1`.

The pinned Kotlin test asserted the old behaviour by design and is INVERTED, not
deleted. Ruby had NO pinned case, so a new one is added rather than inverted.
The describe title no longer claimed something false ("not yet a call SOURCE"
now holds only for Dart) and was retitled.

Verification: closure-binding-labels 51/51; kotlin.test.ts, kotlin-coverage,
ruby.test.ts, ruby-scope, ruby-namespaced all pass (478 passed / 1 expected
inversion before the test was flipped). impact on pickCallerCallableDef:
CRITICAL, 191 impacted, ONE d=1 (resolveCallerGraphId) — the return contract is
unchanged, so that dependent is unaffected. detect_changes {staged}: 5 changed
symbols, 2 affected processes (both EmitReferencesViaLookup, one of them the new
ScopeIsCallableBody step), risk medium.

Dart remains the last failing language: dart/query.ts declares no
@scope.function at all, and dart/captures.ts synthesizes one only from a
declaration WITH a body node, which an expression-bodied closure lacks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR

* fix(dart): give a closure binding a scope and a distinct identity (#2699 part B, S4)

Dart was the last language where a closure binding could not be a call SOURCE,
and fixing only that would have made the graph WORSE, not better. This lands
both halves together for that reason.

## The attribution half

A Dart closure had no scope at all. `dart/query.ts` declares no
@scope.function anywhere — Dart's function scopes are SYNTHESIZED in
`dart/captures.ts` from `declNode` + `findFunctionBody(declNode)`, and
`findFunctionBody` looked only at the next named SIBLING for a `function_body`.
A closure literal carries its body as a CHILD (`function_expression_body`), so
it matched nothing and no scope was produced.

`query.ts` gains the closure-binding declaration rule and `findFunctionBody`
understands the child form. Deliberately NO @scope.function is added to the
query: it would collide at identical range with the synthesized one, and
duplicate scope ids make `buildScopeTree` throw, which DROPS THE WHOLE FILE.

## The identity half, and why it is not optional

With attribution alone, two same-named closures in one file both keyed to the
bare `Function:a.dart:handler`. One node then appeared to call BOTH targets —
a CALLS edge present nowhere in the source. That is worse than the missing edge
it replaced, so S4 could not ship without this.

Root cause is not Dart-specific. `enclosingCallablePrefix` derives a SEMANTIC
relation — what encloses this callable — by SYNTACTIC ancestor walk. Dart parses
`int outer() { … }` as `function_signature` followed by `function_body` as
SIBLINGS, so the enclosing callable is never an ancestor of code inside it and
no membership set can fix that; the walk looks in the wrong direction.

This is what SCIP and real compilers avoid by construction. SCIP keeps a local
symbol opaque (`local <id>` — no name, no position, no chain) and models
containment as a SEPARATE `enclosing_symbol` field; its spec says the local/global
choice should follow ACCESSIBILITY, not the ability to name an enclosure. Dart's
own analyzer answers this from `Element.enclosingElement` in the element model,
never from AST ancestry. clang uses `name@offset` for a function-local; Kythe
uses a document-scoped VName plus a `childof` edge. Identity is positional and
opaque; enclosure is a relation.

`findSplitBodyCallableAncestor` is the narrow fix at that seam: a fallback used
ONLY when the ancestor walk finds nothing, recovering the callable from the
body's preceding sibling.

The sibling must be a BARE SIGNATURE, and that restriction is load-bearing —
"any preceding callable sibling" is WRONG and was caught regressing PHP during
this work. In `<?php function target($x) {…} $handler = function ($x) {…};` the
closure is at FILE level, so the ancestor walk correctly finds nothing, the
fallback runs, and an unrestricted version mis-qualified the file-level
`$handler` as `target.$handler`. A preceding sibling is only an ENCLOSING
callable when it cannot hold its own body.

`SPLIT_SIGNATURE_NODE_TYPES` is exactly that set and is DERIVED, not listed:
`LOCAL_SCOPE_BODY_NODE_TYPES` is already `FUNCTION_NODE_TYPES` minus the bare
signature types, so the difference between them IS the split-signature set
(`function_signature`, `method_signature` — verified at runtime). PHP's
`function_definition` carries a body and is in both, so it is excluded. No
language is named in shared code, and any future split-grammar language is
covered for free.

## Verification

Full resolver sweep — the gate that caught #2714's Rust regression — 2926
passed / 1 skipped / 0 failed across 51 files. closure-binding-labels 52/52;
dart.test.ts, dart-coverage, callable-id-lockstep, function-local-identity,
caller-identity-regression all pass (156/156 across 6 files).
impact on `enclosingCallablePrefix`: LOW, 5 impacted, 3 d=1 all inside
parse-worker. detect_changes {staged}: 5 changed symbols, 0 affected processes,
risk LOW.

Three existing Dart expectations FLIPPED rather than being deleted: Dart locals
now carry the same enclosing-callable + position identity every other language
got in #2695, so `local.dart:handler` became `local.dart:caller.handler@1:2`.
A new test pins the actual defect — two same-named closures staying DISTINCT
nodes — because the qualification assertions alone would not fail if the
fabricated edge returned.

One note for future work: an id-shape assertion here carries a call-site suffix
on indirect invocations (`…handler@3:2:5:9`) but not on direct calls. That is
the callable-value-flow pass keying its edge by invocation position, not part of
the node id.

Part B is now complete: PHP (S1), Rust (S3), Kotlin + Ruby (S2), Dart (S4).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR

* fix(scope-resolution): close every deferred item on #2699 (A1 values, twin-list guard, schema bumps)

Clears the limitations this PR had been carrying rather than leaving them as
follow-ups.

## A1 — function-local VALUES now carry their own identity

This was #2699's ORIGINAL complaint and the one a callable-only gate could never
reach: a top-level `const handler` and a function-local `const handler`
collapsed onto ONE `Const:v.ts:handler`. #2695 restricted position-qualified
identity to Function|Method|Constructor because the collision that produced
wrong CALLS edges was between callables, and widening churned ids for symbols
the pruner mostly deletes. The churn is real and is accepted here deliberately.

Widening needed THREE gates aligned, not one:
  - id-building     — `parse-worker.ts` nestedCallablePrefix
  - resolution      — `ids.ts` position key
  - registration    — `node-lookup.ts` position-key registration

Missing the third would register no position key for values, so every lookup
misses and falls through silently. That is the #2714 failure mode: the caller
attaches to a node that does not exist and the edge is DROPPED, which looks like
"zero dangling edges" from outside. All three now route through ONE predicate,
`isPositionQualifiedLocalLabel`, rather than repeating the label set a third
time.

Only LOCALS move. The prefix comes from `enclosingCallablePrefix`, which returns
undefined when nothing encloses the declaration, so top-level and class-member
ids are untouched — verified by the full resolver sweep, where a leak onto class
members would have broken assertions in every language. `Property` is included
on purpose: a class field stays unqualified because the prefix walk boundaries
on class-likes, while an object-literal property inside a function is genuinely
local and would otherwise keep the old collision.

Measured: `Const:v.ts:handler` + `Const:v.ts:run.handler@3:2`, two distinct
nodes. The KNOWN LIMIT test is FLIPPED per its own former instruction ("this
test should be updated as part of it rather than deleted").

## Schema bumps — required by Part B, not just by A1

INCREMENTAL_SCHEMA_VERSION 20 -> 21, parse-cache SCHEMA_BUMP 27 -> 29.

SCHEMA_BUMP is 29, not 28, and that is the point of re-checking it against
origin/main at MERGE time rather than branch time. This branch cut at 27 and
bumped to 28; #2415 also bumped 27 -> 28 and merged first. The automated
main-merge onto this branch surfaced the collision — leaving it at 28 would have
shipped this whole change with NO parse-cache invalidation, so every warm cache
keeps replaying the pre-fix captures and ids. This is the third instance of that
collision recorded in parse-cache.ts (#2632/#2653 hit it at v21, and
#2653/#2654 hit INCREMENTAL_SCHEMA_VERSION the same way).

Part B already changed emitted node ids AND edges on files that did not
themselves change (Dart locals re-keyed, Rust gained a node it never emitted,
five languages gained closure-source attribution). A v20 index topped up
incrementally keeps serving the old attribution, and a warm parse cache replays
the old captures and ids verbatim. Shipping S1-S4 without these would have let
every existing index silently keep the pre-fix graph.

## Twin-list drift guard — the sixth instance in this family

`IMPLICIT_RECEIVERS` (gitnexus-shared lookup-core.ts) and `THIS_RECEIVERS`
(type-env.ts) spell the same concept in two packages, and nothing enforced
agreement — `$this` was added to the shared list in #2714 only because it was
already in the other. New structural test asserts set equality plus the ONE
deliberate asymmetry (`Me`, Visual Basic spelling, absent from the shared list
because no SupportedLanguages entry uses it) in BOTH directions, so re-adding it
there or dropping it here each fail loudly.

Structural rather than value-imported: both constants are module-private, and
exporting them purely to be testable would widen two public surfaces to satisfy
a test.

## Two false comments corrected

  - `lookup-core.ts` said "see the drift guard noted in #2714", implying a guard
    existed when it was only a deferred follow-up. It exists now, and the
    comment points at it.
  - `callable-id-lockstep.test.ts` claimed its regex "fails if any site
    reconstructs the id". It matches ONE template spelling; a hand-rolled
    concatenation still slips past. Now stated as a tripwire for the known
    shape, not a proof.

## Skill learnings

Four entries appended to eval/workflow_bench/learnings.jsonl from this run: the
v9fs safe-writer failure, backticks silently terminating a query template
literal (hit three times), a module-level TDZ const that passes tsc and then
presents as N file failures with ZERO failing assertions, and concurrent vitest
runs starving worker startup so a whole suite fails at ~5001ms.

## Verification

Full resolver sweep 2926 passed / 1 skipped / 0 failed (51 files) — identical to
pre-A1, which is the evidence that only locals moved. All EIGHT bench gates PASS
with fingerprints UNCHANGED, so no regeneration was needed. function-local-identity,
callable-id-lockstep, receiver-twin-list-drift and closure-binding-labels 71/71.
tsc --noEmit clean.

detect_changes {staged}: 9 changed symbols, 14 affected processes, risk HIGH —
expected, and the reason the sweep above is the gate rather than a targeted list.
Every affected process routes through `resolveDefGraphId`, the key chain Part A
measured at CRITICAL with 23 direct dependents.

Deliberately NOT done: the SCIP end state (opaque `local <id>` plus an explicit
enclosure EDGE instead of containment encoded in the id string). It is a design
direction, not a limitation of this work, and it is INCOMPATIBLE with A1 — A1
widens chain-encoded identity, that removes chain encoding entirely. Bundling
both would re-key every local twice. Written up in the research notes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR

* test: update two assertions the #2699 changes correctly invalidated

Both failed on CI at da3d8397 and are fixed here. Neither is a behaviour
regression; both pinned values that this PR deliberately changed.

1. this-boundary.test.ts — "a Kotlin lambda still sees the receiver"

The `this.m()` edge still exists and `this` still resolves to the enclosing
receiver, which is the ONLY property this test exists to guard (its own comment
said so: "what matters here is only that the `this.m()` edge still exists at
all"). Only the SOURCE moved, from `run` to the lambda:

  Method:K.kt:K.run#0       -> Method:K.kt:K.run.f@2:16
  Method:K.kt:K.run.f@2:16  -> Method:K.kt:K.m#0

The comment justifying the old expectation is now false and is corrected rather
than left: it said the lambda "is not its own caller anchor" because Kotlin
scopes `lambda_literal` as a BLOCK. Kotlin still scopes it as a block (#1757 is
unchanged) — what changed in S2 is that a Block-kind scope is accepted as a
caller anchor when the scope IS the callable's body.

2. call-summary-schema-version.test.ts — INCREMENTAL_SCHEMA_VERSION pin

Moves 20 -> 21 with the bump, which is the point of pinning it: a change that
alters emitted ids or edges without bumping would otherwise ship silently.

Also adds the missing reuse-gate case. `passesReuseGate(20)` now asserts FALSE —
a v20 index predates closure bindings becoming call SOURCES, the Rust node for
`let f = || …`, the Dart closure scope + enclosing-callable identity, and
position-qualified function-local values. Topping such an index up incrementally
keeps serving the old attribution, including the Dart case where two same-named
closures collapsed onto one node and asserted a CALLS edge present nowhere in
the source.

Why CI found these and local verification did not: the verification set was
`test/integration/resolvers/` plus a hand-picked list, and both failures sat
outside it — one integration test about `this` (which a caller-attribution
change obviously touches) and one unit test pinning the exact constant that was
bumped. Grepping for the changed constant, and for tests asserting closure
attribution, would have found both. All 8 suites that reference the schema
constants were then run: 177/177, no third pin.

Verification: this-boundary + call-summary-schema-version 17/17;
the 8 schema-referencing suites 177/177. detect_changes {staged}: 0 changed
symbols, 0 affected processes, risk LOW (assertion-only edits).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR

* fix(scope-resolution): resolve every finding from the multi-engine review of #2699 part B

The first cut of Part B shipped four P1 defects. A two-engine review (Claude
swarm + ce personas; Codex gpt-5.6-sol swarm + ce + adversarial) found all four,
three of them because an independent engine disagreed with the authoring one.
Each is fixed here and pinned in test/integration/closure-review-findings.test.ts.

## P1-1 — a multi-line closure binding fabricated a CALLS edge

The worst of the four, because it reintroduced the exact defect class #2699
exists to remove. The two query channels anchor on DIFFERENT nodes by design
(graph-node on the outer wrapper, scope-resolution on the inner closure) and the
bridge joins them on line only. Same line, the join matches. Split across lines:

    $multi =
        function ($x) { return target($x); };

the join missed, `resolveDefGraphId` failed closed, and `resolveCallerGraphId`
then CLIMBED to the parent scope — emitting `outer -> target` although `outer`
calls nothing, while the real `outer.$multi` node sat with zero outgoing edges.

`resolveCallerGraphId` now fails closed at the owning callable instead of
climbing. If we have identified the callable that owns a call site and cannot
name its graph node, crediting an ancestor is not graceful degradation — it
invents a relationship. A missing edge is the correct failure direction for a
graph whose consumers include `impact`.

Getting there took two attempts, worth recording: the first guard keyed on the
def's qualifiedName carrying the `@line:col` local marker, but that suffix is
added by parse-worker for GRAPH NODE ids and scope-resolution defs do not have
it, so the guard never fired. `pickCallerCallableDef` now reports whether the
callable came from a child scope, and the fail-closed applies to the owning
callable either way.

## P1-2 — TS constructor parameter properties were re-keyed as locals

A REGRESSION against the base, not merely an incomplete fix. Admitting
`Property` to the position-qualified set made the enclosing-callable walk reach
the constructor's `method_definition` THROUGH the parameter list — a
LOCAL_SCOPE_BODY hit that lands before any class boundary — so
`constructor(private readonly port: Port)` produced
`Property:svc.ts:Service.constructor.port@2:14` instead of `Service.port`. That
silently empties the slot `impact`, `rename` and FTS address while the class
still asserts HAS_PROPERTY against it, and it is the Angular/NestJS DI idiom.
A real instance exists in this repo at src/core/group/service.ts:304.

parse-worker.ts already computed the correct exemption (`isFunctionLocalProperty`,
lines 2245-2257) two lines above; the new ternary discarded it. Now reused, so
the owner-edge decision and the id decision cannot disagree.

## P1-3 — Dart top-level and `final` closures were never call sources

The rule matched only `initialized_variable_definition`, Dart's FUNCTION-LOCAL
shape. A top-level `var` is `initialized_identifier` and a top-level
`final`/`const` is `static_final_declaration`; the second declarator of
`var f = ..., g = ...` is also `initialized_identifier`. None got a declaration
capture, so `findFunctionBody` never synthesized their scope.
dart/captures.ts ALREADY listed all three in bindingNodeTypes for callable-flow
— the declaration rule simply did not mirror it. It does now.

## P1-4 — Ruby `do ... end` and `Proc.new` closures were uncovered

`do ... end` is the dominant MULTI-LINE Ruby style and produces `(do_block)`;
all three patterns matched `(block)` only. The scope channel already covered
both, so these closures got a Block scope owning nothing and their calls fell
through to the enclosing method. The PR's own Ruby test used the brace form, so
it passed.

Fixing it needed BOTH channels — tree-sitter-queries.ts had no graph-node rule
for the `(call)` forms either, exactly as Rust did. Verified: brace, do/end and
Proc.new are now all sources.

## Also from the review

- The split-signature fallback could fire on VALID TypeScript: a
  `declare namespace` containing a bodyless overload made the next declaration's
  `export_statement` a sibling of a `function_signature`, so `send` became
  `internalHelper.send@2:9`. The fallback now requires the matched node to be
  the signature's BODY (a body holds statements; a declaration wrapper holds
  another signature), which separates the two without naming a grammar.
- `isCallableDef` re-spelled `Function | Method | Constructor` in the same file
  that imports `isOverloadableCallable` and calls it three times — a NEW twin
  list, in the PR whose headline is a twin-list drift guard. It now delegates.
- A partial edit had left a self-contradictory comment in parse-worker.ts
  ("Restricted to CALLABLE labels: the / Applies to VALUES as well as callables").
- eval/workflow_bench/learnings.jsonl carried a "skill": "gitnexus-plan" entry,
  but that skill's SKILL.md:347 states feedback is chat-only and forbids
  appending learnings during a planning task. Dropped; the three gitnexus-work
  entries are sanctioned and stay.
- Ruby's lambda/proc patterns tested the method NAME only, so `MyMod.lambda { }`
  was captured as a closure binding. `!receiver` now constrains them.
- Rust's closure work (S3) had ZERO test coverage anywhere — verified once by a
  throwaway fixture and never pinned. Now covered.

## Verification

Full resolver sweep plus the identity/closure suites: 3017 passed / 1 skipped /
0 failed across 58 files (up from 2997 — the new tests). This is the gate that
mattered for P1-1: failing closed instead of climbing could have silently
deleted real edges in any language, and ~2900 resolver assertions say it did
not. All EIGHT bench fingerprint gates PASS with fingerprints UNCHANGED.
tsc --noEmit clean.

detect_changes {staged}: 14 changed symbols, 18 affected processes, risk
CRITICAL — expected, since P1-1 changes the fallthrough of `resolveCallerGraphId`,
the key chain Part A measured at CRITICAL with 23 direct dependents.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184RmD24KFJidYqpM7v3XjR

---------

Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Gergő Magyar 2026-07-28 18:25:19 +01:00 • committed by GitHub
parent b0cacd05ee
commit 0ce7880290
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
24 changed files with 1053 additions and 50 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
eval/workflow_bench/learnings.jsonl
View file

@ -1,2 +1,5 @@
{"skill": "gitnexus-work", "date": "2026-07-25", "task": "#2687 const-arrow Const/Function twin fix in parse-worker + MCP impact envelope", "friction": "Phase 2's Build-current/index-current procedure indexes the repo-under-test, which makes CLI-spawning suites (skip-git-cli, cli/tool-no-index-stderr) time out because repo resolution then opens the 237k-node index from that cwd; they pass at the same commit in an unindexed worktree, so the procedure manufactures false regressions in its own final verification.", "suggestion": "Phase 4 should note that CLI-spawn suites can fail solely because the worktree became an indexed repo, and prescribe the A/B check (same commit, unindexed worktree) instead of leaving the executor to conclude a regression."}
{"skill": "gitnexus-work", "date": "2026-07-25", "task": "#2687 same run", "friction": "Phase 2 requires top-level `status: up-to-date` before graph queries, but any uncommitted staged edit makes status report `stale` by design, so the gate is unsatisfiable in the stage -> detect_changes -> commit sequence Phase 3 mandates.", "suggestion": "Scope the up-to-date requirement to index.commit == HEAD + empty incompleteReasons + runnerIdentityStatus current, and state that a `stale` top-level status caused solely by uncommitted working-tree edits is expected at the detect_changes gate."}
{"skill": "gitnexus-work", "date": "2026-07-28", "task": "#2699 part B same run", "friction": "Every language query lives in a TypeScript template literal, so a backtick inside a `;;` comment silently terminates it and produces confusing TS1005/TS1128 parse errors far from the real edit. Hit this three separate times in one session.", "suggestion": "Phase 3 should warn that *.query.ts bodies are template literals and backticks in comments are a syntax error, or the repo should add a lint rule; the build catches it but the error location does not point at the comment."}
{"skill": "gitnexus-work", "date": "2026-07-28", "task": "#2699 part B same run", "friction": "A module-level `const` derived from another const declared LOWER in the same file passes tsc and builds a clean dist, then throws ReferenceError (temporal dead zone) at import. It presents as N test FILES failing with ZERO failing assertions, which reads like host/infra flake rather than a code defect.", "suggestion": "Phase 3's verification note should call out that file-level failures with zero test failures usually mean a module-load error, and to grep the run output for ReferenceError before blaming the host."}
{"skill": "gitnexus-work", "date": "2026-07-28", "task": "#2699 part B same run", "friction": "Two concurrent `vitest run` invocations on this host starve worker-pool startup: every test in both runs fails at ~5001ms against the default GITNEXUS_WORKER_READY_TIMEOUT_MS, which looks exactly like a real regression across the whole suite.", "suggestion": "Phase 3 should state that verification runs must be serial, and that a whole-suite failure at ~5001ms is worker-startup starvation, not signal."}

4
gitnexus-shared/src/scope-resolution/registries/lookup-core.ts
View file

@ -343,7 +343,9 @@ function resolveReceiverOwner(
* That twin also lists `Me`, deliberately NOT mirrored here: no entry in
* `SupportedLanguages` uses it, so it can only ever exempt a variable that
* happens to be called `Me`. The two lists are otherwise the same set, and
* nothing enforces that — see the drift guard noted in #2714.
* that equality — plus the `Me` exemption in both directions — is now ENFORCED
* by `gitnexus/test/unit/receiver-twin-list-drift.test.ts`. Editing either list
* without the other fails there.
*/
const IMPLICIT_RECEIVERS: readonly string[] = Object.freeze(['self', 'this', '$this']);

9
gitnexus/src/core/ingestion/languages/dart/captures.ts
View file

@ -249,6 +249,15 @@ function dartCallableCallee(selector: SyntaxNode): SyntaxNode | null {
* nodes are unaffected.
*/
function findFunctionBody(declNode: SyntaxNode): SyntaxNode | null {
// A closure literal carries its body as a CHILD (function_expression_body),
// unlike a Dart declaration whose body is the next named SIBLING. Without
// this branch the caller synthesizes no @scope.function for a closure at all,
// so a closure binding has no scope to own its callable def and can never be
// a call SOURCE (#2699 S4 — this is why Dart alone showed zero child scopes).
if (declNode.type === 'function_expression') {
const body = declNode.namedChildren.find((c) => c.type === 'function_expression_body');
return body ?? null;
}
const node =
declNode.parent !== null && declNode.parent.type === 'method_signature'
? declNode.parent

37
gitnexus/src/core/ingestion/languages/dart/query.ts
View file

@ -92,6 +92,43 @@ const DART_SCOPE_QUERY = `
(function_signature
name: (identifier) @declaration.name) @declaration.function)
; ── Declarations — closure bound to a local ──────────────────────────────────
;
; var handler = (int x) => target(x); / var blk = (int y) { ... };
;
; Anchor discipline (same contract as javascript/query.ts): @declaration.function
; sits on the INNER function_expression, NOT on the local_variable_declaration
; wrapper. Dart is the one language that declares NO @scope.function in this
; file — its function scopes are SYNTHESIZED in captures.ts from
; declNode + findFunctionBody(declNode). So this rule deliberately does not add
; a @scope.function of its own: doing that would collide with the synthesized
; one at identical range, and duplicate scope ids make buildScopeTree throw,
; which drops the whole file. Instead findFunctionBody now understands a
; closure's child function_expression_body, so the existing synthesis produces
; exactly one scope, anchored on the same node as the declaration (#2699 S4).
;; THREE binding shapes, not one. Dart wraps only the FIRST local declarator in
;; initialized_variable_definition; a top-level var and every later declarator
;; are initialized_identifier, and a top-level final/const is
;; static_final_declaration. Matching only the first shape left idiomatic
;; top-level closures and the g of "var f = ..., g = ..." with no declaration
;; capture, so findFunctionBody never synthesized their scope and they could
;; never be call SOURCES.
;;
;; This mirrors DART_CALLABLE_CAPTURE_OPTIONS.bindingNodeTypes in captures.ts,
;; which already listed all three for callable-flow. The two lists must stay in
;; step; dart-closure-binding-shapes.test.ts pins that.
(initialized_variable_definition
(identifier) @declaration.name
(function_expression) @declaration.function)
(initialized_identifier
(identifier) @declaration.name
(function_expression) @declaration.function)
(static_final_declaration
(identifier) @declaration.name
(function_expression) @declaration.function)
; ── Declarations — methods (inside class/mixin/extension bodies) ─────────────
(method_signature
(function_signature

11
gitnexus/src/core/ingestion/languages/kotlin/query.ts
View file

@ -115,6 +115,17 @@ const KOTLIN_SCOPE_QUERY = `
(function_declaration
(simple_identifier) @declaration.name) @declaration.function
;; Lambda bound to a val/var: val handler = { x: Int -> target(x) }
;; Anchor discipline (same contract as javascript/query.ts): @declaration.function
;; sits on the INNER lambda_literal, NOT on the property_declaration wrapper, so
;; anchor.range aligns with the (lambda_literal) @scope.block range. That
;; alignment is what lets pickCallerCallableDef accept a Block-kind scope as a
;; callable boundary: the scope IS the callable's body. The lambda stays
;; @scope.block deliberately (#1757 smart casts) — do NOT re-kind it.
(property_declaration
(variable_declaration (simple_identifier) @declaration.name)
(lambda_literal) @declaration.function)
(property_declaration
(variable_declaration
(simple_identifier) @declaration.name)) @declaration.property

17
gitnexus/src/core/ingestion/languages/php/query.ts
View file

@ -87,6 +87,23 @@ const PHP_SCOPE_QUERY = `
(function_definition
name: (name) @declaration.name) @declaration.function
;; Closure assigned to a variable: $handler = function () {...}; or fn() => ...;
;; Anchor discipline (same contract as javascript/query.ts): @declaration.function
;; sits on the INNER anonymous_function / arrow_function, NOT on the
;; assignment_expression wrapper. That aligns anchor.range with the
;; @scope.function range above, so pass2AttachDeclarations attaches the
;; declaration to the CLOSURE's own scope rather than the enclosing function's.
;; Without this the closure scope owns no callable def and
;; pickCallerCallableDef falls through to the enclosing callable, making the
;; closure a call TARGET but never a call SOURCE (#2699).
(assignment_expression
left: (variable_name) @declaration.name
right: (anonymous_function) @declaration.function)
(assignment_expression
left: (variable_name) @declaration.name
right: (arrow_function) @declaration.function)
;; ── Declarations — properties ─────────────────────────────────────────────
;; PHP 7.4+ typed property: private UserRepo $repo;

58
gitnexus/src/core/ingestion/languages/ruby/query.ts
View file

@ -79,6 +79,64 @@ const RUBY_SCOPE_QUERY = `
(singleton_method
name: (identifier) @declaration.name) @declaration.function
;; ── Declarations — closure bound to a local ──────────────────────────────
;;
;; handler = ->(x) { target(x) } / lambda { |x| ... } / proc { |x| ... }
;;
;; Anchor discipline (same contract as javascript/query.ts): @declaration.function
;; sits on the INNER (block), NOT on the assignment wrapper and NOT on the
;; (lambda) node — the block is what carries @scope.block above, so anchoring
;; there aligns anchor.range with the scope range. That alignment is what lets
;; pickCallerCallableDef accept a Block-kind scope as a callable boundary.
;; do_block/block stay @scope.block deliberately — do NOT re-kind them.
;;
;; The call forms are restricted to lambda/proc by name. An unrestricted
;; (call block: (block)) would match ANY method call with a block, so
;; mapped = items.map { |i| ... } would wrongly declare mapped a callable.
;; Separate #eq? patterns rather than one #match? alternation: alternation
;; predicates are a known hazard on this tree-sitter line.
;; BOTH block forms in every pattern. do...end produces (do_block), braces
;; produce (block), and do/end is the dominant MULTI-LINE style — covering only
;; braces left lambda-do-end with a Block scope owning nothing, so its
;; calls fell through to the enclosing method. @scope.block above already covers
;; both, so the two channels now agree.
;;
;; The !receiver constraint is load-bearing: without it #eq? tests the method
;; NAME only, so MyMod.lambda {...} / obj.proc {...} would be captured as
;; closure bindings. Verified against the grammar: with !receiver those are
;; rejected while bare lambda/proc still match.
(assignment
left: (identifier) @declaration.name
right: (lambda body: [(block) (do_block)] @declaration.function))
(assignment
left: (identifier) @declaration.name
right: (call
!receiver
method: (identifier) @_lambda-kw
block: [(block) (do_block)] @declaration.function)
(#eq? @_lambda-kw "lambda"))
(assignment
left: (identifier) @declaration.name
right: (call
!receiver
method: (identifier) @_proc-kw
block: [(block) (do_block)] @declaration.function)
(#eq? @_proc-kw "proc"))
;; Proc.new { ... } / Proc.new do ... end — the explicit constructor form.
;; Receiver IS required here and must be the Proc constant, so this cannot
;; collide with the bare-call patterns above.
(assignment
left: (identifier) @declaration.name
right: (call
receiver: (constant) @_proc-const
method: (identifier) @_new-kw
block: [(block) (do_block)] @declaration.function)
(#eq? @_proc-const "Proc")
(#eq? @_new-kw "new"))
;; ── Declarations — variable assignment ───────────────────────────────────
(assignment

13
gitnexus/src/core/ingestion/languages/rust/query.ts
View file

@ -64,6 +64,19 @@ const RUST_SCOPE_QUERY = `
(function_signature_item
name: (identifier) @declaration.name) @declaration.function
;; Declarations — closure bound to a let: let handler = || target(1);
;; Anchor discipline (same contract as javascript/query.ts): @declaration.function
;; sits on the INNER closure_expression, NOT on the let_declaration wrapper, so
;; anchor.range aligns with the (closure_expression) @scope.function range above.
;; pass2AttachDeclarations then attaches the declaration to the CLOSURE's own
;; scope instead of the enclosing block, which is what lets pickCallerCallableDef
;; treat the closure as a call SOURCE rather than falling through to the
;; enclosing fn (#2699). Also covers move closures — the closure_expression
;; node spans the move keyword.
(let_declaration
pattern: (identifier) @declaration.name
value: (closure_expression) @declaration.function)
;; Declarations — struct fields
(field_declaration
name: (field_identifier) @declaration.name

104
gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts
View file

@ -28,7 +28,10 @@ import {
simpleKey,
type GraphNodeLookup,
} from '../graph-bridge/node-lookup.js';
import { isOverloadableCallable } from '../../utils/callable-labels.js';
import {
isOverloadableCallable,
isPositionQualifiedLocalLabel,
} from '../../utils/callable-labels.js';
import { templateConstraintsIdTag } from '../../utils/template-arguments.js';
import { parameterShapeIdTag } from '../../utils/method-props.js';
/**
@ -73,6 +76,34 @@ function rangeContainsPoint(
return true;
}
/**
* Adapter over the canonical {@link isOverloadableCallable} — deliberately NOT a
* second spelling of the label set. An earlier revision of this file inlined
* `Function | Method | Constructor` here, which recreated the exact twin-list
* drift this PR adds a guard test for elsewhere.
*/
const isCallableDef = (d: SymbolDefinition): boolean => isOverloadableCallable(d.type);
/**
* True when `range` is the body of `def` itself — the scope's start position
* equals the def's declaration position.
*
* Safe to compare directly: `scope-extractor.ts` builds a def id as
* `def:<filePath>#<startLine>:<startCol>:<type>:<name>` from the same `Range`
* a scope carries, so both sides share one coordinate base and need no
* conversion. (Do not "fix" this against the 1-based reading in
* `defStartLine`'s docblock — what matters here is that the two sides agree
* with each other, not which base they use.)
*/
function scopeIsCallableBody(
range: { startLine: number; startCol: number },
def: SymbolDefinition,
): boolean {
const m = def.nodeId.match(/#(\d+):(\d+):/);
if (m === null) return false;
return Number(m[1]) === range.startLine && Number(m[2]) === range.startCol;
}
/** Pick the callable that owns `atRange` when multiple overloads share a class scope. */
function pickCallerCallableDef(
scope: {
@ -82,21 +113,35 @@ function pickCallerCallableDef(
},
scopes: ScopeResolutionIndexes,
atRange?: { startLine: number; startCol: number },
): SymbolDefinition | undefined {
): { def: SymbolDefinition; fromChildScope: boolean } | undefined {
if (atRange !== undefined) {
for (const childId of scopes.scopeTree.getChildren(scope.id)) {
const child = scopes.scopeTree.getScope(childId);
if (child === undefined || child.kind !== 'Function') continue;
if (child === undefined) continue;
if (!rangeContainsPoint(child.range, atRange)) continue;
const childCallable = child.ownedDefs.find(
(d) => d.type === 'Function' || d.type === 'Method' || d.type === 'Constructor',
);
if (childCallable !== undefined) return childCallable;
const childCallable = child.ownedDefs.find(isCallableDef);
if (childCallable === undefined) continue;
if (child.kind === 'Function') return { def: childCallable, fromChildScope: true };
// A Block-kind scope is a callable boundary ONLY when the scope IS that
// callable's own body. Kotlin `lambda_literal` and Ruby `do_block`/`block`
// are @scope.block deliberately (#1757 smart casts), so the kind gate
// alone would never let a closure there become a call SOURCE (#2699).
//
// But relaxing the gate to accept ANY Block owning a callable is wrong:
// a nested `fun foo()` declared inside a block is owned by that block, so
// a call made at BLOCK level — outside foo — would be misattributed to
// foo. The alignment test discriminates them. For a closure the
// declaration and the scope sit on the SAME node (the anchor discipline
// documented in javascript/query.ts), so their start positions match; for
// a nested function the block starts at `{` and the def starts at the
// declaration, so they do not.
if (child.kind === 'Block' && scopeIsCallableBody(child.range, childCallable)) {
return { def: childCallable, fromChildScope: true };
}
}
}
return scope.ownedDefs.find(
(d) => d.type === 'Function' || d.type === 'Method' || d.type === 'Constructor',
);
const own = scope.ownedDefs.find(isCallableDef);
return own === undefined ? undefined : { def: own, fromChildScope: false };
}
/**
@ -140,9 +185,16 @@ function defStartLine(nodeId: string | undefined): number | undefined {
function simpleNameOf(qualifiedName: string): string {
const dot = qualifiedName.lastIndexOf('.');
const tail = dot === -1 ? qualifiedName : qualifiedName.slice(dot + 1);
return tail.replace(/@\d+:\d+$/, '');
return tail.replace(LOCAL_IDENTITY_SUFFIX, '');
}
/**
* The function-local identity marker appended by `localIdentity` (`name@row:col`).
* Shared so the stripper above and the fail-closed guard in
* `resolveCallerGraphId` cannot disagree about what counts as a local.
*/
const LOCAL_IDENTITY_SUFFIX = /@\d+:\d+$/;
export function resolveDefGraphId(
filePath: string,
def: {
@ -170,7 +222,7 @@ export function resolveDefGraphId(
// 0-based, def ids 1-based. An `AMBIGUOUS_POSITION` tombstone (two
// callables on one line) falls through to the name-based keys below.
const line = defStartLine(def.nodeId);
if (line !== undefined && isOverloadableCallable(def.type)) {
if (line !== undefined && isPositionQualifiedLocalLabel(def.type)) {
const simple = simpleNameOf(qn);
const posHit = nodeLookup.get(positionKey(filePath, def.type, line - 1, simple));
if (posHit !== undefined && posHit !== AMBIGUOUS_POSITION) return posHit;
@ -312,10 +364,34 @@ export function resolveCallerGraphId(
// Prefer Function/Method/Constructor anchors; fall back to
// Class/Interface/Struct/Enum. Variable/Property are NOT valid
// caller anchors — see `isCallerAnchorLabel` for why.
const fnDef = pickCallerCallableDef(scope, scopes, atRange);
if (fnDef !== undefined) {
const picked = pickCallerCallableDef(scope, scopes, atRange);
if (picked !== undefined) {
const fnDef = picked.def;
const id = resolveDefGraphId(fnDef.filePath, fnDef, nodeLookup);
if (id !== undefined) return id;
// FAIL CLOSED at a FUNCTION-LOCAL boundary (#2699 review P1-1).
//
// `fnDef` is the callable that genuinely owns this call site. When its
// graph node cannot be named, climbing to the parent scope does not
// degrade gracefully — it ATTRIBUTES THE CALL TO A FUNCTION THAT DOES NOT
// MAKE IT. Measured before this guard, a closure whose literal starts on
// the line after its binding
// $multi =
// function ($x) { return target($x); };
// emitted `outer -> target` while the real `outer.$multi` node sat with no
// outgoing edges: a CALLS edge present nowhere in the source, which is the
// exact defect class #2699 exists to remove.
//
// Restricted to defs carrying the function-local `@line:col` identity,
// because that is precisely the set whose position join can miss (the two
// query channels anchor on different nodes by design). For every other
// callable the historical climb is preserved, so this cannot silently
// delete edges outside the local-identity path.
// The scope we are standing in OWNS this call site and we have identified
// its callable — we simply cannot name that callable's graph node. Climbing
// to an ancestor here does not degrade gracefully: it credits the call to a
// function that does not make it. Fail closed instead.
return undefined;
}
const classDef = scope.ownedDefs.find((d) => isCallerAnchorLabel(d.type));
if (classDef !== undefined) {

7
gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts
View file

@ -20,7 +20,10 @@
import type { NodeLabel, ParameterTypeClass } from 'gitnexus-shared';
import type { KnowledgeGraph } from '../../../graph/types.js';
import { isOverloadableCallable } from '../../utils/callable-labels.js';
import {
isOverloadableCallable,
isPositionQualifiedLocalLabel,
} from '../../utils/callable-labels.js';
import { templateConstraintsIdTag } from '../../utils/template-arguments.js';
import { parameterShapeIdTag } from '../../utils/method-props.js';
@ -135,7 +138,7 @@ export function buildGraphNodeLookup(graph: KnowledgeGraph): GraphNodeLookup {
// Position key (#2699) — see `positionKey`. Second write on a key marks it
// ambiguous rather than letting source order decide.
const startLine = (props as { startLine?: number }).startLine;
if (startLine !== undefined && isOverloadableCallable(node.label)) {
if (startLine !== undefined && isPositionQualifiedLocalLabel(node.label)) {
const posK = positionKey(props.filePath, node.label, startLine, props.name);
lookup.set(posK, lookup.has(posK) ? AMBIGUOUS_POSITION : node.id);
// A local-identity node carries `@<row>:<col>` on its last name segment. Record

46
gitnexus/src/core/ingestion/tree-sitter-queries.ts
View file

@ -1332,6 +1332,20 @@ export const RUST_QUERIES = `
; Functions & Items
(function_item name: (identifier) @name) @definition.function
(function_signature_item name: (identifier) @name) @definition.function
; Closure bound to a let: let handler = || target(1);
; Emits the Function NODE. Without it a Rust closure binding had no graph node
; at all, so it could be neither a call target nor a call source (#2699), which
; made Rust the one exception to "a closure bound to a name is a Function node
; in every language" (#2687).
; Anchor note: this channel puts @definition.function on the OUTER
; let_declaration, which is the OPPOSITE of the scope-resolution channel in
; languages/rust/query.ts (inner closure_expression, to align with
; @scope.function). Both match their own channel's convention -- compare the
; (lexical_declaration (variable_declarator ... (arrow_function))) rule above.
(let_declaration
pattern: (identifier) @name
value: (closure_expression)) @definition.function
(struct_item name: (type_identifier) @name) @definition.struct
; A union is materialized as a Struct node (same rationale as the
; scope-resolution @declaration.struct in languages/rust/query.ts: every
@ -1580,6 +1594,38 @@ export const RUBY_QUERIES = `
(assignment
left: (identifier) @name
right: (lambda)) @definition.function
; The (lambda) rule above covers the stabby forms only. lambda do...end,
; proc do...end and Proc.new are (call) nodes, so without these the scope
; channel declared a closure the graph channel never gave a node to, and the
; call fell through to the enclosing method. Same two-channel lockstep Rust
; needed. Receiver constraints mirror ruby/query.ts exactly: bare for
; lambda/proc, Proc-constant for new — otherwise every block-taking call
; (items.map { }) would mint a Function node.
(assignment
left: (identifier) @name
right: (call
!receiver
method: (identifier) @_lam
block: [(block) (do_block)])
(#eq? @_lam "lambda")) @definition.function
(assignment
left: (identifier) @name
right: (call
!receiver
method: (identifier) @_prc
block: [(block) (do_block)])
(#eq? @_prc "proc")) @definition.function
(assignment
left: (identifier) @name
right: (call
receiver: (constant) @_pc
method: (identifier) @_nw
block: [(block) (do_block)])
(#eq? @_pc "Proc")
(#eq? @_nw "new")) @definition.function
`;
// Kotlin queries - works with tree-sitter-kotlin (fwcd/tree-sitter-kotlin)

87
gitnexus/src/core/ingestion/utils/ast-helpers.ts
View file

@ -409,6 +409,77 @@ export function findAncestorBeforeBoundary(
return null;
}
/**
* Enclosing callable for grammars that split a callable into a SIGNATURE node
* and a SIBLING body, where the callable is therefore never an ancestor of the
* code inside it.
*
* Dart is the case that forced this: `int outer() { … }` parses as
* `function_signature` followed by `function_body` as SIBLINGS, so an ancestor
* walk from a closure inside the body can never reach `outer`. No membership
* set fixes that — the walk is looking in the wrong direction (#2699).
*
* Deliberately a FALLBACK, used only when the ancestor walk found nothing.
*
* The sibling must be a BARE SIGNATURE, and that restriction is load-bearing —
* "any preceding callable sibling" is WRONG and was caught regressing PHP. In
* `<?php function target($x) {…} $handler = function ($x) {…};` the closure is
* at FILE level, so the primary ancestor walk correctly finds nothing and this
* fallback runs; an unrestricted version then grabs the preceding
* `function_definition` and mis-qualifies the file-level `$handler` as
* `target.$handler`. A preceding sibling is only an ENCLOSING callable when it
* cannot hold its own body — i.e. when the grammar split the body off.
*
* `SPLIT_SIGNATURE_NODE_TYPES` is exactly that set, and it is derived rather
* than listed: `LOCAL_SCOPE_BODY_NODE_TYPES` already filters the bare-signature
* types out of `FUNCTION_NODE_TYPES`, so the difference between them IS the
* split-signature set. PHP's `function_definition` carries a body and is in
* both, so it is excluded; Dart's `function_signature` is in only the former,
* so it qualifies.
*
* Language-neutral by construction — it names no grammar, and any future
* signature/body-split language is covered for free.
*/
export function findSplitBodyCallableAncestor(
node: SyntaxNode,
signatureOnlyTypes: ReadonlySet<string>,
boundaryTypes: ReadonlySet<string>,
): SyntaxNode | null {
let current = node.parent;
while (current !== null) {
if (boundaryTypes.has(current.type)) return null;
const prev = current.previousNamedSibling;
if (
prev !== null &&
signatureOnlyTypes.has(prev.type) &&
// `current` must be the signature's BODY, not merely the next thing after
// it. Without this, valid TypeScript trips the fallback: in
// declare namespace Api {
// function internalHelper(x): number;
// export function send(x): number;
// }
// `send`'s `export_statement` is the next sibling of `internalHelper`'s
// `function_signature`, so `send` was mis-qualified as
// `internalHelper.send@r:c`. TypeScript emits bodyless
// function_signature/method_signature for overloads and ambient
// declarations, so the split-signature set is NOT Dart-only.
//
// A body contains statements; a declaration wrapper contains another
// signature. Rejecting any `current` that directly holds a signature of
// its own separates the two without naming a grammar.
!current.namedChildren.some((child) => signatureOnlyTypes.has(child.type))
) {
return prev;
}
current = current.parent;
}
return null;
}
// SPLIT_SIGNATURE_NODE_TYPES is defined next to LOCAL_SCOPE_BODY_NODE_TYPES,
// which it derives from — declaring it here would read it in its temporal dead
// zone and throw at module load (tsc does NOT catch that; only running does).
/**
* Determine the graph node label from a tree-sitter capture map.
* Handles language-specific reclassification via the provider's labelOverride hook
@ -1218,6 +1289,22 @@ export const LOCAL_SCOPE_BODY_NODE_TYPES: ReadonlySet<string> = new Set(
]),
);
/**
* Callable node types whose grammar splits the body off into a SIBLING node, so
* the callable is never an ancestor of the code inside it (Dart
* `function_signature` / `method_signature`).
*
* Derived, not listed, so it cannot drift from the two sets that define it:
* `LOCAL_SCOPE_BODY_NODE_TYPES` is `FUNCTION_NODE_TYPES` minus exactly the bare
* signature types, so the difference IS the split-signature set.
*
* Must stay BELOW `LOCAL_SCOPE_BODY_NODE_TYPES` — reading it earlier hits the
* temporal dead zone and throws at module load.
*/
export const SPLIT_SIGNATURE_NODE_TYPES: ReadonlySet<string> = new Set(
[...FUNCTION_NODE_TYPES].filter((t) => !LOCAL_SCOPE_BODY_NODE_TYPES.has(t)),
);
// ============================================================================
// Generic AST traversal helpers (shared by parse-worker + php-helpers)
// ============================================================================

34
gitnexus/src/core/ingestion/utils/callable-labels.ts
View file

@ -14,3 +14,37 @@ import type { NodeLabel } from 'gitnexus-shared';
export function isOverloadableCallable(label: NodeLabel | undefined): boolean {
return label === 'Function' || label === 'Method' || label === 'Constructor';
}
/**
* Labels whose FUNCTION-LOCAL declarations carry the enclosing-callable +
* position identity of #2699 (`Function:x.ts:run.save@3:2`).
*
* Wider than {@link isOverloadableCallable} on purpose. #2695 restricted the
* rule to callables because the collision that produced wrong CALLS edges was
* between callables, and widening churned ids for symbols the local-symbol
* pruner mostly deletes. But the issue's ORIGINAL complaint was about values:
* a top-level `const handler` and a function-local `const handler` collapsed
* onto one `Const:v.ts:handler`, and no callable gate ever reaches that. The
* limitation is closed here rather than carried.
*
* Only LOCALS are affected either way: the prefix comes from
* `enclosingCallablePrefix`, which returns `undefined` when nothing encloses
* the declaration, so top-level and class-member ids are untouched — that is
* what keeps this off the symbols other files and stored references address.
* A class field stays unqualified even inside a function, because the prefix
* walk boundaries on class-likes.
*
* ONE definition, deliberately: the id-building phase and the resolution phase
* must agree on this set or the caller attaches to a node that does not exist
* and the edge is silently dropped — the failure mode #2714 fixed, invisible
* from outside because "zero dangling edges" is what it looks like.
*/
export function isPositionQualifiedLocalLabel(label: NodeLabel | undefined): boolean {
return (
isOverloadableCallable(label) ||
label === 'Variable' ||
label === 'Const' ||
label === 'Property' ||
label === 'Static'
);
}

60
gitnexus/src/core/ingestion/workers/parse-worker.ts
View file

@ -82,6 +82,8 @@ import {
buildDefinitionPreScan,
FUNCTION_NODE_TYPES,
findAncestorBeforeBoundary,
findSplitBodyCallableAncestor,
SPLIT_SIGNATURE_NODE_TYPES,
getDefinitionNodeFromCaptures,
findEnclosingClassInfo,
findObjectLiteralBindingInfo,
@ -98,6 +100,7 @@ import {
LOCAL_SCOPE_BODY_NODE_TYPES,
type SyntaxNode,
} from '../utils/ast-helpers.js';
import { isPositionQualifiedLocalLabel } from '../utils/callable-labels.js';
import { extractCallArgTypes, type MixedChainStep } from '../utils/call-analysis.js';
import { buildTypeEnv } from '../type-env.js';
import type { ConstructorBinding } from '../type-env.js';
@ -821,11 +824,20 @@ const enclosingCallablePrefix = (
//
// Over-inclusion here is the SAFE direction: an extra boundary only suppresses
// the nesting prefix, which falls back to the pre-#2699 class qualification.
const fnNode = findAncestorBeforeBoundary(
node,
LOCAL_SCOPE_BODY_NODE_TYPES,
CALLABLE_PREFIX_BOUNDARY_TYPES,
);
const fnNode =
findAncestorBeforeBoundary(node, LOCAL_SCOPE_BODY_NODE_TYPES, CALLABLE_PREFIX_BOUNDARY_TYPES) ??
// Signature/body-split grammars: the enclosing callable is a SIBLING of the
// body, not an ancestor, so the walk above returns null for every local
// inside it. Dart is the case in hand (`function_signature` +
// `function_body` as siblings) — without this a Dart closure gets no
// prefix, so two same-named closures in one file collapse onto ONE node and
// the graph asserts a CALLS edge that does not exist in the source (#2699).
//
// SPLIT_SIGNATURE_NODE_TYPES, NOT FUNCTION_NODE_TYPES: only a callable that
// cannot hold its own body can be an enclosing callable of a SIBLING. Using
// the wider set mis-qualified a file-level PHP `$handler = function …` as
// `target.$handler` by grabbing the preceding `function target() {…}`.
findSplitBodyCallableAncestor(node, SPLIT_SIGNATURE_NODE_TYPES, CALLABLE_PREFIX_BOUNDARY_TYPES);
if (fnNode === null) return undefined;
return callableOwnQualifiedName(fnNode, filePath, provider);
};
@ -2285,14 +2297,40 @@ const processFileGroup = (
// worker-path Impl node id matches the sequential path and the owner walk.
// #2699: a callable nested inside another callable is qualified by the
// enclosing callable, so a function-local closure stops colliding with a
// same-named file-level function. Restricted to CALLABLE labels: the
// collision that produced wrong CALLS edges is between callables, and
// widening it to every function-local Variable/Property would churn ids
// for symbols the local-symbol pruner mostly deletes anyway.
// same-named file-level function.
// Applies to VALUES as well as callables since #2699 closed A1: a
// top-level `const handler` and a function-local `const handler`
// otherwise collapse onto one `Const:v.ts:handler`, which was the
// issue's original complaint and is unreachable from a callable-only
// gate. `isPositionQualifiedLocalLabel` is the single definition of that
// set, shared with resolution in `ids.ts` — the two phases disagreeing
// silently drops edges rather than failing (#2714).
// Same helper as the caller-attribution phase — see `enclosingCallablePrefix`.
//
// A CLASS MEMBER must never gain a local prefix, and the plain walk is not
// enough to guarantee that once `Property`/`Static` are in the set. A
// TypeScript constructor PARAMETER PROPERTY —
// `constructor(private readonly port: Port)` — reaches the constructor's
// `method_definition` THROUGH the parameter list, and `method_definition`
// is in LOCAL_SCOPE_BODY_NODE_TYPES, so the walk hits it BEFORE any class
// boundary and re-keys a genuine field as `C.constructor.port@r:c`. That
// silently empties the `C.port` slot every `impact` / `rename` / FTS
// consumer addresses, and the class still asserts HAS_PROPERTY against it.
//
// `isFunctionLocalProperty` above already encodes the correct test (a
// parameter-list ancestor reached before any local-scope body means NOT
// function-local); reuse that exclusion here rather than spelling a second
// rule, so the owner-edge decision and the id decision cannot disagree.
const isParameterScopedMember =
(nodeLabel === 'Property' || nodeLabel === 'Static') &&
definitionNode !== undefined &&
findAncestorBeforeBoundary(
definitionNode,
PARAMETER_LIST_NODE_TYPES,
LOCAL_SCOPE_BODY_NODE_TYPES,
) !== null;
const nestedCallablePrefix =
(nodeLabel === 'Function' || nodeLabel === 'Method' || nodeLabel === 'Constructor') &&
definitionNode
isPositionQualifiedLocalLabel(nodeLabel) && definitionNode && !isParameterScopedMember
? enclosingCallablePrefix(definitionNode, file.path, provider)
: undefined;

14
gitnexus/src/storage/parse-cache.ts
View file

@ -55,6 +55,18 @@ import type { ParseWorkerResult } from '../core/ingestion/workers/parse-worker.j
// the main thread (the #1983 OOM). Because the two stores share this version,
// any future change to the `ParsedFile` serialization shape MUST bump
// SCHEMA_BUMP so both invalidate in lockstep.
// v29: closure-binding declaration rules for PHP/Rust/Kotlin/Ruby/Dart, a Rust
// graph node for `let f = || …`, a Dart closure scope, and function-local VALUES
// (Variable/Const/Property/Static) qualified by their enclosing callable plus
// position (#2699 parts A1 + B). All parse-time, so a warm cache would replay
// the old captures and the pre-qualification ids verbatim.
//
// This is 29 and not 28 because of the exact collision the v21 note below warns
// about: this branch cut at 27 and bumped to 28, while #2415 bumped 27 -> 28 and
// merged FIRST. Re-checking against origin/main at merge time — not at branch
// time — is what caught it; leaving it at 28 would have shipped this change with
// NO parse-cache invalidation, so every warm cache keeps serving the pre-fix
// captures and ids.
// v28: Java/Kotlin capture side-channels persist Spring condition facts and
// annotation-source line numbers (#2415).
// v26: the enclosing-callable walk stops at class bodies and anonymous-class
@ -103,7 +115,7 @@ import type { ParseWorkerResult } from '../core/ingestion/workers/parse-worker.j
// JLS 13.1 immediate-host chains (#2555).
// v18: Worker$N anonymous bodies. v17: callable-value-flow operand identity.
// v16: direct callee identity.
const SCHEMA_BUMP = 28;
const SCHEMA_BUMP = 29;
const GITNEXUS_PKG_VERSION = (() => {
try {
// package.json sits at gitnexus/package.json — two levels up from

10
gitnexus/src/storage/repo-manager.ts
View file

@ -525,8 +525,16 @@ export interface RepoMeta {
* reads it also covered are kept. A v19 index holds those false CALLS/ACCESSES on
* every unchanged file and would keep serving them through the reuse gate; force a
* full re-analyze instead.
* v21: a closure bound to a name is a call SOURCE in every language, not only a
* TARGET (#2699 part B). PHP/Rust/Kotlin/Ruby/Dart closure bindings gained the
* declaration rule, Rust gained the graph NODE it never emitted, and Dart locals
* gained the enclosing-callable + position identity that made two same-named
* closures collapse onto one node — which had them asserting a CALLS edge
* present nowhere in the source. All of that changes emitted node ids AND edges
* on files that did not themselves change, so a v20 index topped up
* incrementally keeps serving the old attribution; force a full re-analyze.
*/
export const INCREMENTAL_SCHEMA_VERSION = 20;
export const INCREMENTAL_SCHEMA_VERSION = 21;
export interface IndexedRepo {
repoPath: string;

113
gitnexus/test/integration/closure-binding-labels.test.ts
View file

@ -247,7 +247,13 @@ describeIfWorkerBuilt('calls to a closure binding resolve to its Function node',
'int caller() {\n var handler = (int x) => x;\n return handler(1);\n}\n',
);
expect(targets).toEqual(['Function:local.dart:handler']);
// Qualified by #2699: Dart's enclosing callable is a SIBLING of the body
// (function_signature + function_body), so the ancestor walk that builds
// this prefix found nothing and every Dart local stayed bare. Two
// same-named closures in one file therefore collapsed onto ONE node. Now
// carries the same enclosing-callable + position identity as every other
// language.
expect(targets).toEqual(['Function:local.dart:caller.handler@1:2']);
});
it('Dart: a top-level `final` closure binding resolves', async () => {
@ -272,7 +278,13 @@ describeIfWorkerBuilt('calls to a closure binding resolve to its Function node',
'int caller() {\n var f = (int x) => x, g = (int y) => y;\n return f(1) + g(2);\n}\n',
);
expect(targets).toEqual(['Function:multi.dart:f', 'Function:multi.dart:g']);
// Both declarators are function-local, so both carry the enclosing-callable
// + position identity (#2699). The distinct columns are the point: `g` is a
// nested initialized_identifier on the SAME line as `f`.
expect(targets).toEqual([
'Function:multi.dart:caller.f@1:2',
'Function:multi.dart:caller.g@1:24',
]);
});
it('Kotlin: a class-body closure property resolves to its Method node', async () => {
@ -517,7 +529,7 @@ describeIfWorkerBuilt('closure bindings resolve in the remaining languages (#269
});
});
describeIfWorkerBuilt('a closure binding is a call TARGET, not yet a call SOURCE', () => {
describeIfWorkerBuilt('a closure binding as a call SOURCE (#2699 part B)', () => {
// Known limit, pinned deliberately so it is visible rather than surprising.
//
// A call made INSIDE a closure binding is attributed to the ENCLOSING scope,
@ -541,31 +553,106 @@ describeIfWorkerBuilt('a closure binding is a call TARGET, not yet a call SOURCE
// scope for the walk to consider.
//
// So a fix needs per-language work, not one switch: a callable-boundary
// signal independent of scope `kind` (Kotlin/Ruby), an association from a
// closure scope to its binding's def (PHP), and a scope that does not exist
// yet (Dart). See #2699.
// signal independent of scope `kind` (Kotlin/Ruby — DONE, S2), an
// association from a closure scope to its binding's def (PHP — DONE, S1;
// Rust — DONE, S3), and a scope that did not exist at all (Dart — DONE, S4).
// See #2699.
//
// All five languages now attribute closure calls to the binding. The review
// of that work found the FIRST cut incomplete in four ways — multi-line
// bindings, Dart top-level/`final` shapes, Ruby `do ... end`/`Proc.new`, and
// TS constructor parameter properties — each now pinned in
// `closure-review-findings.test.ts`.
//
// Probe-measured root cause (#2699): EVERY still-failing language has an
// EMPTY ownedDefs on the closure's own scope, because the closure-binding
// declaration rule (binding name + @declaration.function on the INNER
// closure node) existed only in javascript/query.ts. Kotlin and Ruby need
// BOTH that rule AND a relaxed kind gate — their lambda_literal / do_block
// is @scope.block deliberately (#1757), so the rule alone leaves them
// rejected. Dart has no closure scope at all: dart/query.ts declares no
// @scope.function, and dart/captures.ts synthesizes one only from a
// declaration WITH a body node, which an expression-bodied closure lacks.
//
// TS/JS free bindings are the exception: their arrow has a `@scope.function`
// with a matching range, so the closure IS the anchor there. These tests exist
// to catch that asymmetry changing in EITHER direction.
it('Kotlin: a call inside the closure is attributed to the file, not the binding', async () => {
it('Kotlin: a call inside the closure IS attributed to the binding (#2699 S2)', async () => {
// FLIPPED by #2699 S2, which took BOTH halves:
// 1. kotlin/query.ts gained the closure-binding declaration rule, with
// @declaration.function on the INNER lambda_literal so its range
// aligns with the (lambda_literal) @scope.block range;
// 2. pickCallerCallableDef now accepts a Block-kind scope as a callable
// boundary when the scope IS the callable's body (def start position
// == scope start position).
// Half 1 alone changes nothing here — the lambda stays @scope.block
// deliberately (#1757 smart casts), so the kind gate would still reject it.
const targets = await callEdgeIdsFor(
'A.kt',
'fun target(x: Int): Int = x\n\nval handler = { x: Int -> target(x) }\n',
);
expect(targets).toEqual(['rel:CALLS:File:A.kt->Function:A.kt:target']);
expect(targets).toEqual(['rel:CALLS:Function:A.kt:handler->Function:A.kt:target']);
});
it('PHP: a call inside the closure is attributed to the file, not the binding', async () => {
it('PHP: a call inside the closure IS attributed to the binding (#2699 S1)', async () => {
// FLIPPED by #2699 S1. php/query.ts now carries the closure-binding
// declaration rule with javascript/query.ts's anchor discipline
// (@declaration.function on the INNER anonymous_function, so its range
// aligns with the (anonymous_function) @scope.function above). The closure
// scope therefore owns the callable def and pickCallerCallableDef stops
// falling through to the enclosing scope — the closure is now a call
// SOURCE, not only a TARGET.
const targets = await callEdgeIdsFor(
'a.php',
'<?php\nfunction target($x) { return $x; }\n' +
'$handler = function ($x) { return target($x); };\n',
);
expect(targets).toEqual(['rel:CALLS:File:a.php->Function:a.php:target']);
expect(targets).toEqual(['rel:CALLS:Function:a.php:$handler->Function:a.php:target']);
});
it('Dart: two same-named closures in one file stay DISTINCT nodes (#2699 S4)', async () => {
// The defect this pins is worse than a missing edge. Before #2699 S4 gave
// Dart locals an enclosing-callable prefix, both closures keyed to the bare
// `Function:collide.dart:handler`, so ONE node appeared to call BOTH
// `target` and `other` — a CALLS edge that exists nowhere in the source.
//
// Dart is the only grammar here that splits a callable into a signature and
// a SIBLING body, so its enclosing callable was unreachable by ancestor
// walk and every Dart local stayed unqualified. Distinct positions in the
// two ids are the whole property.
const targets = await callEdgeIdsFor(
'collide.dart',
'int target(int x) => x;\nint other(int x) => x;\n' +
'int outer() {\n var handler = (int x) => target(x);\n return handler(1);\n}\n' +
'int second() {\n var handler = (int x) => other(x);\n return handler(2);\n}\n',
);
// The trailing `:5:9` / `:9:9` on the first and third edges is the CALL
// SITE, not part of the node id: invoking a closure binding is an indirect
// call emitted by the callable-value-flow pass, which keys its edge by the
// invocation position. The direct `handler -> target` calls carry no such
// suffix. Do not "normalize" these away — they are different edge kinds.
expect(targets).toEqual([
'rel:CALLS:Function:collide.dart:outer->Function:collide.dart:outer.handler@3:2:5:9',
'rel:CALLS:Function:collide.dart:outer.handler@3:2->Function:collide.dart:target',
'rel:CALLS:Function:collide.dart:second->Function:collide.dart:second.handler@7:2:9:9',
'rel:CALLS:Function:collide.dart:second.handler@7:2->Function:collide.dart:other',
]);
});
it('Ruby: a call inside a lambda binding IS attributed to the binding (#2699 S2)', async () => {
// Ruby had no pinned case before #2699 S2, so this is new coverage rather
// than an inverted assertion. do_block/block stay @scope.block (matching
// Kotlin), so this exercises the same Block-scope alignment path.
const targets = await callEdgeIdsFor(
'a.rb',
'def target(x)\n x\nend\n\nhandler = ->(x) { target(x) }\n',
);
expect(targets).toEqual(['rel:CALLS:Function:a.rb:handler->Method:a.rb:target#1']);
});
it('JavaScript: a free arrow binding IS the caller anchor', async () => {
@ -653,7 +740,11 @@ describeIfWorkerBuilt('a value binding is never aliased onto a same-named callab
'int run() {\n var save = (int x) => x * 2;\n return save(1);\n}\n',
);
expect(targets).toEqual(['Function:svc.dart:save']);
// The target is the LOCAL closure, never `Svc.save`. Since #2699 the local
// also carries its enclosing callable and position, so the two are now
// distinct by id and not merely by which node the edge happened to reach —
// `run.save@5:2` cannot collide with the method however the lookup is keyed.
expect(targets).toEqual(['Function:svc.dart:run.save@5:2']);
});
it('Kotlin: a genuine constant mints no CALLS', async () => {

191
gitnexus/test/integration/closure-review-findings.test.ts Normal file
View file

@ -0,0 +1,191 @@
/**
* Regression tests for the findings of the multi-engine review of #2699 part B.
*
* Each test here pins a defect that SHIPPED in the first cut of that work and
* was caught only by review — three of the four by an independent engine
* disagreeing with the authoring one. They are grouped in one file because they
* share a root theme: the two query channels (graph-node vs scope-resolution)
* must agree about which binding shapes exist, and when they cannot agree the
* bridge must fail closed rather than guess.
*/
import { describe, expect, it, vi } from 'vitest';
import fs from 'node:fs';
import os from 'node:os';
import path from 'node:path';
import { runPipelineFromRepo } from '../../src/core/ingestion/pipeline.js';
import { DIST_WORKER_URL, distWorkerExists } from '../helpers/worker-parse.js';
vi.setConfig({ testTimeout: 90_000 });
const describeIfWorkerBuilt = distWorkerExists() ? describe : describe.skip;
/** Every CALLS edge in a one-file repo, as `src -> dst`, sorted. */
const callEdges = async (filename: string, source: string): Promise<string[]> => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-review-'));
try {
fs.writeFileSync(path.join(dir, filename), source, 'utf-8');
const result = await runPipelineFromRepo(dir, () => {}, {
workerPoolSize: 1,
workerUrlForTest: DIST_WORKER_URL,
});
return result.graph.relationships
.filter((rel) => rel.type === 'CALLS')
.map((rel) => `${rel.sourceId} -> ${rel.targetId}`)
.sort();
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
};
/** Node ids in a one-file repo whose id contains `needle`. */
const nodeIdsContaining = async (
filename: string,
source: string,
needle: string,
): Promise<string[]> => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-review-ids-'));
try {
fs.writeFileSync(path.join(dir, filename), source, 'utf-8');
const result = await runPipelineFromRepo(dir, () => {}, {
workerPoolSize: 1,
workerUrlForTest: DIST_WORKER_URL,
});
return result.graph.nodes
.map((n) => n.id)
.filter((id) => id.includes(needle))
.sort();
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
};
describeIfWorkerBuilt(
'#2699 review P1-1 — a closure that cannot be named never credits its parent',
() => {
it('a MULTI-LINE closure binding does not fabricate a call from the enclosing function', async () => {
// The two channels anchor on DIFFERENT nodes by design — graph-node on the
// outer wrapper, scope-resolution on the inner closure. On one line they
// share a row and the position join matches. Split across lines it misses,
// and before the fix `resolveCallerGraphId` CLIMBED to the enclosing scope,
// emitting `outer -> target` although `outer` calls nothing. That is a CALLS
// edge present nowhere in the source — the exact defect class #2699 exists
// to remove — so the bridge now fails closed at the owning callable.
//
// The single-line binding in the same fixture proves the fail-closed path
// did not simply delete the feature.
const edges = await callEdges(
'ml.php',
'<?php\nfunction target($x) { return $x; }\nfunction outer() {\n' +
' $single = function ($x) { return target($x); };\n' +
' $multi =\n function ($x) { return target($x); };\n return 1;\n}\n',
);
expect(edges).toEqual(['Function:ml.php:outer.$single@3:2 -> Function:ml.php:target']);
});
},
);
describeIfWorkerBuilt(
'#2699 review P1-2 — widening identity to values must not touch class members',
() => {
it('a TypeScript constructor PARAMETER PROPERTY keeps its class-qualified id', async () => {
// Admitting `Property` to the position-qualified set made this walk reach
// the constructor's `method_definition` THROUGH the parameter list — a
// LOCAL_SCOPE_BODY hit before any class boundary — so a genuine field was
// re-keyed `Service.constructor.port@r:c`. That silently empties the
// `Service.port` slot `impact`, `rename` and FTS address, while the class
// still asserts HAS_PROPERTY against it. This is the DI idiom every
// Angular/NestJS codebase uses, so the blast radius is large.
const ids = await nodeIdsContaining(
'svc.ts',
'export class Port { send(): void {} }\n' +
'export class Service {\n constructor(private readonly port: Port) {}\n}\n',
'port',
);
expect(ids).toEqual(['Property:svc.ts:Service.port']);
});
},
);
describeIfWorkerBuilt('#2699 review P1-3 — every Dart binding shape can be a call SOURCE', () => {
it('top-level `var` and `final` closures are call sources, not just function-local ones', async () => {
// The first cut matched only `initialized_variable_definition`, which is
// Dart's FUNCTION-LOCAL shape. A top-level `var` is `initialized_identifier`
// and a top-level `final` is `static_final_declaration`, so idiomatic
// top-level closures got no declaration capture, no synthesized scope, and
// could never be sources. captures.ts already listed all three shapes for
// callable-flow — the declaration rule just did not mirror it.
const edges = await callEdges(
'top.dart',
'int target(int x) => x;\n' +
'var topVar = (int x) => target(x);\n' +
'final topFinal = (int y) => target(y);\n',
);
expect(edges).toEqual([
'Function:top.dart:topFinal -> Function:top.dart:target',
'Function:top.dart:topVar -> Function:top.dart:target',
]);
});
});
describeIfWorkerBuilt(
'#2699 review P1-4 — Ruby do...end and Proc.new closures are call SOURCES',
() => {
it('covers brace, do...end and Proc.new forms alike', async () => {
// `do ... end` is the dominant MULTI-LINE Ruby style and produces
// (do_block), while the first cut matched only (block). The scope channel
// already covered both, so these closures got a Block scope owning nothing
// and their calls fell through to the enclosing method. Fixing it needed
// BOTH channels — the graph-node channel had no rule for the (call) forms
// either, exactly as Rust did.
const edges = await callEdges(
'rb.rb',
'def target(x)\n x\nend\ndef outer\n' +
' a = ->(x) { target(x) }\n' +
' b = lambda do |y| target(y) end\n' +
' c = Proc.new { |z| target(z) }\n' +
'end\n',
);
expect(edges).toEqual([
'Function:rb.rb:outer.a@4:2 -> Method:rb.rb:target#1',
'Function:rb.rb:outer.b@5:2 -> Method:rb.rb:target#1',
'Function:rb.rb:outer.c@6:2 -> Method:rb.rb:target#1',
]);
});
it('a receiver-qualified `.lambda` / `.proc` is NOT a closure binding', async () => {
// The `#eq?` predicate tests the method NAME only, so without `!receiver`
// `MyMod.lambda { }` was captured as a closure binding. `items.map { }` was
// already rejected; this is the narrower sibling case.
const edges = await callEdges(
'recv.rb',
'def target(x)\n x\nend\ndef outer\n' + ' d = MyMod.lambda { |q| target(q) }\n' + 'end\n',
);
expect(edges).toEqual(['Method:recv.rb:outer#0 -> Method:recv.rb:target#1']);
});
},
);
describeIfWorkerBuilt(
'#2699 review S3 — Rust closure bindings (previously untested entirely)',
() => {
it('a Rust closure binding is a call SOURCE', async () => {
// The Rust half of this work shipped with ZERO test coverage — it was
// verified once by a throwaway fixture and never pinned. Rust needed BOTH
// channels because it emitted no graph node for `let f = || ...` at all.
const edges = await callEdges(
'a.rs',
'fn target(x: i32) -> i32 { x }\nfn outer() -> i32 {\n let handler = || target(1);\n handler()\n}\n',
);
expect(edges).toEqual([
'Function:a.rs:outer -> Function:a.rs:outer.handler@2:4',
'Function:a.rs:outer.handler@2:4 -> Function:a.rs:target',
]);
});
},
);

71
gitnexus/test/integration/function-local-identity.test.ts
View file

@ -281,3 +281,74 @@ describeIfWorkerBuilt('a function-local callable does not collide with a file-le
]);
});
});
/** Node ids for `name`, with local value symbols kept so the pruner can't hide them. */
const valueNodeIdsFor = async (
filename: string,
source: string,
name: string,
): Promise<string[]> => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-local-value-identity-'));
try {
fs.writeFileSync(path.join(dir, filename), source, 'utf-8');
const result = await runPipelineFromRepo(dir, () => {}, {
workerPoolSize: 1,
workerUrlForTest: DIST_WORKER_URL,
// `pruneLocalSymbols` deletes ~94% of inert function-local value symbols,
// which would make the collapse below invisible rather than absent.
keepLocalValueSymbols: true,
});
return result.graph.nodes
.filter((node) => node.properties.name === name)
.map((node) => node.id)
.sort();
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
};
describeIfWorkerBuilt('function-local VALUES carry their own identity (#2699 A1)', () => {
it('a function-local VALUE does not collapse onto the file-level node', async () => {
// FLIPPED, per this test's own former instruction. It previously pinned the
// collapse as a KNOWN LIMIT: #2695 gave function-local CALLABLES a
// position-bearing id and deliberately excluded VALUES, so a top-level
// `const handler` and a function-local `const handler` shared ONE node.
// That was the residual half of #2699's ORIGINAL complaint — the issue is
// about values first, and no callable-only gate could ever reach it.
//
// Widened here via `isPositionQualifiedLocalLabel`, the single definition
// shared by all THREE phases that must agree: id-building
// (`parse-worker.ts`), resolution (`ids.ts` position key) and registration
// (`node-lookup.ts`). Two of them disagreeing does not fail loudly — the
// caller attaches to a node that does not exist and the edge is silently
// dropped, which is the #2714 failure mode.
//
// The churn this was deferred for is real and was accepted deliberately:
// it re-keys ~14,700 build-time nodes to change ~800 persisted ones,
// because `pruneLocalSymbols` deletes most locals. Hence the paired
// INCREMENTAL_SCHEMA_VERSION / parse-cache SCHEMA_BUMP bumps — without them
// a warm cache or an incremental top-up replays the old un-suffixed ids.
//
// Only LOCALS move. The prefix comes from `enclosingCallablePrefix`, which
// returns undefined when nothing encloses the declaration, so the
// file-level `handler` below keeps its bare id — that is what keeps this
// off the symbols other files and stored references address.
const ids = await valueNodeIdsFor(
'v.ts',
[
"export const handler = 'top-level value';",
'',
'export function run(): string {',
" const handler = 'function-local value';",
' return handler;',
'}',
'',
].join('\n'),
'handler',
);
// Two distinct nodes: the file-level one keeps its bare id, the local
// carries its enclosing callable AND declaration position.
expect(ids).toEqual(['Const:v.ts:handler', 'Const:v.ts:run.handler@3:2']);
});
});

12
gitnexus/test/integration/this-boundary.test.ts
View file

@ -188,10 +188,14 @@ describeIfWorkerBuilt('an arrow inherits `this`; every other function form binds
'\n',
),
),
// Attributed to `run`, not to `f`: Kotlin scopes `lambda_literal` as a
// BLOCK (#1757), so the lambda is not its own caller anchor. What matters
// here is only that the `this.m()` edge still exists at all.
).toContain('Method:K.kt:K.run#0 -> Method:K.kt:K.m#0');
// Attributed to `f` since #2699 S2. Kotlin still scopes `lambda_literal`
// as a BLOCK (#1757 — that has NOT changed), but a Block-kind scope is now
// accepted as a caller anchor when the scope IS the callable's body, so
// the lambda is its own anchor. The property this test exists for is
// unchanged and is what the assertion still checks: `this` inside a Kotlin
// lambda resolves to the enclosing receiver, so the `this.m()` edge exists.
// Only its SOURCE moved, from `run` to `run.f`.
).toContain('Method:K.kt:K.run.f@2:16 -> Method:K.kt:K.m#0');
});
});

19
gitnexus/test/unit/call-summary-schema-version.test.ts
View file

@ -73,8 +73,12 @@ describe('CALL_SUMMARY relation-type exclusion (U-C1)', () => {
});
describe('CALL_SUMMARY incremental reuse gate (U-C5)', () => {
it('INCREMENTAL_SCHEMA_VERSION is bumped to 20 (named-receiver lexical fallback, #2699)', () => {
expect(INCREMENTAL_SCHEMA_VERSION).toBe(20);
it('INCREMENTAL_SCHEMA_VERSION is bumped to 21 (closure bindings are call SOURCES, #2699 part B)', () => {
// Moves with every bump BY DESIGN — that is the point of pinning it. A
// change that alters emitted ids or edges without bumping would otherwise
// ship silently, and an existing index would keep serving the old graph
// through the reuse gate below.
expect(INCREMENTAL_SCHEMA_VERSION).toBe(21);
});
it('a pre-current stamp fails the `=== INCREMENTAL_SCHEMA_VERSION` reuse gate → forces full re-analyze', () => {
@ -155,7 +159,16 @@ describe('CALL_SUMMARY incremental reuse gate (U-C5)', () => {
// `const baseUrl`) — 709 of them on a 762-file corpus. Reusing it would keep
// every one on unchanged files.
expect(passesReuseGate(19)).toBe(false);
// A pre-v21 (v20) index predates closure bindings becoming call SOURCES in
// PHP/Rust/Kotlin/Ruby/Dart, the Rust graph node for `let f = || …`, the Dart
// closure scope + enclosing-callable identity, and position-qualified
// function-local VALUES. All of those change emitted ids and edges on files
// that did not themselves change, so reusing a v20 index keeps serving the
// old attribution — including the Dart case where two same-named closures
// collapsed onto one node and asserted a CALLS edge present nowhere in the
// source.
expect(passesReuseGate(20)).toBe(false);
// A current-version stamp passes the gate (incremental top-up eligible).
expect(passesReuseGate(20)).toBe(true);
expect(passesReuseGate(21)).toBe(true);
});
});

9
gitnexus/test/unit/callable-id-lockstep.test.ts
View file

@ -64,8 +64,13 @@ describe('no call site re-inlines the rule', () => {
it('parse-worker.ts contains no inlined `<prefix>.${localIdentity(...)}` template', () => {
// The structural half. The unit assertions above would still pass if a
// fourth phase appeared and spelled the rule out by hand — which is
// exactly how the divergence #2714 fixed came to exist. This fails if any
// site reconstructs the id instead of calling the shared function.
// exactly how the divergence #2714 fixed came to exist.
//
// Scope, stated honestly: this matches ONE template spelling — the
// `${prefix}.${localIdentity(...)}` form the divergence actually took. A
// hand-rolled id built by string concatenation, or with the interpolation
// spelled differently, still slips past. It is a tripwire for the known
// shape, not a proof that no site reconstructs the id.
const source = readFileSync(
fileURLToPath(new URL('../../src/core/ingestion/workers/parse-worker.ts', import.meta.url)),
'utf8',

75
gitnexus/test/unit/detect-changes-local-id-stability.test.ts Normal file
View file

@ -0,0 +1,75 @@
/**
* #2699 consumer audit — `detect_changes` must not key on node ids.
*
* #2695/#2714 gave function-local CALLABLES position-bearing ids
* (`Function:x.ts:run.save@3:2`). That raised a specific worry for this
* consumer: an id containing `@row:col` changes whenever the declaration
* MOVES, even when the code is byte-identical, so an id-keyed
* `detect_changes` would report churn for every edit above a local.
*
* The worry is unfounded, and this file pins why. `detect_changes` maps diff
* hunks to symbols by LINE-RANGE OVERLAP — it matches `n.startLine`/`n.endLine`
* against the hunk bounds and merely REPORTS `n.id`. Node identity never
* participates in the match, so a position-bearing id cannot inflate
* `changed_count`.
*
* These are structural (source-grep) assertions, in the same idiom as
* `detect-changes-worktree.test.ts`: they prove the query still has the shape
* the audit verified, and would fail loudly if someone switched the mapping to
* id equality. They do NOT execute the query — the behavioural coverage for
* detect_changes lives in the MCP integration suites.
*/
import { describe, expect, it } from 'vitest';
import { readFileSync } from 'fs';
import path from 'path';
import { fileURLToPath } from 'url';
const __dirname = path.dirname(fileURLToPath(import.meta.url));
const backendSrc = readFileSync(
path.join(__dirname, '../../src/mcp/local/local-backend.ts'),
'utf-8',
);
/** The hunk→symbol query, isolated so the assertions below can't match text elsewhere. */
const symbolQuery = (): string => {
const start = backendSrc.indexOf('const symbolQuery = `');
expect(start, 'symbolQuery template not found — update this test').toBeGreaterThan(-1);
const from = backendSrc.indexOf('`', start) + 1;
const to = backendSrc.indexOf('`', from);
return backendSrc.slice(from, to);
};
describe('#2699 audit — detect_changes maps hunks to symbols by position, not id', () => {
it('matches on startLine/endLine, so a moved local cannot register as churn', () => {
const q = symbolQuery();
expect(q).toContain('n.startLine IS NOT NULL');
expect(q).toContain('n.endLine IS NOT NULL');
});
it('never matches a symbol by node id', () => {
// The guard that matters. `n.id` may be SELECTED (it is reported back to
// the caller) but must not appear in a WHERE-side equality against a
// parameter — that would reintroduce the id-churn failure mode.
const q = symbolQuery();
const whereClause = q.slice(q.indexOf('WHERE'), q.indexOf('RETURN'));
expect(whereClause).not.toMatch(/n\.id\s*=/);
expect(whereClause).not.toMatch(/n\.id\s+IN\b/);
});
it('still excludes BasicBlock rows by id prefix (#2082 U7)', () => {
// The one legitimate id-shaped predicate: a PREFIX filter that drops
// nameless PDG substrate. Pinned so the assertion above cannot be
// satisfied by deleting this exclusion.
const q = symbolQuery();
expect(q).toContain("NOT n.id STARTS WITH 'BasicBlock:'");
});
it('reports the id rather than matching on it', () => {
const q = symbolQuery();
expect(q.slice(q.indexOf('RETURN'))).toContain('n.id AS id');
});
});

99
gitnexus/test/unit/receiver-twin-list-drift.test.ts Normal file
View file

@ -0,0 +1,99 @@
/**
* The drift guard for the implicit-receiver twin lists (#2699 follow-up).
*
* TWO lists spell "this is an implicit receiver", in two packages:
*
* - `IMPLICIT_RECEIVERS` — gitnexus-shared `lookup-core.ts`. Two consumers:
* the Step-1 lexical skip (a NAMED receiver must not resolve its member
* through the lexical chain) and `resolveReceiverOwner`.
* - `THIS_RECEIVERS` — gitnexus `type-env.ts`. Decides whether a receiver
* rewrites to the enclosing type.
*
* They are the SIXTH twin-list instance found in this family of work, and the
* previous five each shipped a bug when one side moved. `$this` was added to
* the shared list in #2714 precisely because it was already in the other one;
* nothing but this test stops the next divergence.
*
* `Me` is the one deliberate asymmetry: `THIS_RECEIVERS` carries it (Visual
* Basic spelling) and the shared list does not, because no entry in
* `SupportedLanguages` uses it — mirroring it there could only ever exempt a
* variable that happens to be named `Me`. That exemption is asserted
* explicitly rather than tolerated, so RE-adding `Me` to the shared list, or
* dropping it from the local one, both fail loudly.
*
* Structural (source-parsed) rather than value-imported: both constants are
* module-private, and exporting them purely to be testable would widen two
* public surfaces to satisfy a test. Same idiom as
* `detect-changes-local-id-stability.test.ts`.
*/
import { describe, expect, it } from 'vitest';
import { readFileSync } from 'fs';
import path from 'path';
import { fileURLToPath } from 'url';
const __dirname = path.dirname(fileURLToPath(import.meta.url));
/**
* String literals inside the first `[...]` following `marker` that actually
* CONTAINS a string literal.
*
* "First `[`" is not good enough: `IMPLICIT_RECEIVERS` is declared
* `: readonly string[] = Object.freeze([...])`, so the first bracket belongs to
* the TYPE annotation and yields an empty list — which would make every
* assertion below vacuously pass. That is exactly what the non-empty check in
* the first test exists to catch, and it did.
*/
const literalsAfter = (source: string, marker: string): string[] => {
const at = source.indexOf(marker);
expect(at, `${marker} not found — update this test`).toBeGreaterThan(-1);
for (let open = source.indexOf('[', at); open !== -1; open = source.indexOf('[', open + 1)) {
const close = source.indexOf(']', open);
if (close === -1) break;
const names = [...source.slice(open + 1, close).matchAll(/'([^']*)'|"([^"]*)"/g)]
.map((m) => m[1] ?? m[2] ?? '')
.filter((s) => s.length > 0);
if (names.length > 0) return names.sort();
}
return [];
};
const sharedList = (): string[] =>
literalsAfter(
readFileSync(
path.join(
__dirname,
'../../../gitnexus-shared/src/scope-resolution/registries/lookup-core.ts',
),
'utf-8',
),
'const IMPLICIT_RECEIVERS',
);
const typeEnvList = (): string[] =>
literalsAfter(
readFileSync(path.join(__dirname, '../../src/core/ingestion/type-env.ts'), 'utf-8'),
'const THIS_RECEIVERS',
);
describe('#2699 — implicit-receiver twin lists do not drift', () => {
it('both lists are non-empty and were actually parsed', () => {
// Guards the guard: a regex that silently matched nothing would make every
// assertion below vacuously true.
expect(sharedList().length).toBeGreaterThan(0);
expect(typeEnvList().length).toBeGreaterThan(0);
});
it('the shared list is exactly the type-env list minus the deliberate `Me`', () => {
expect(sharedList()).toEqual(typeEnvList().filter((name) => name !== 'Me'));
});
it('`Me` stays OUT of the shared list', () => {
// Stated separately so the intent survives even if the set comparison above
// is ever relaxed: this asymmetry is a decision, not an oversight.
expect(sharedList()).not.toContain('Me');
});
it('`Me` stays IN the type-env list', () => {
expect(typeEnvList()).toContain('Me');
});
});