diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index fec56fb23..6ecb49d33 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -3006,9 +3006,9 @@ } }, "node_modules/express-rate-limit": { - "version": "8.6.0", - "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.6.0.tgz", - "integrity": "sha512-XKJXDsASUOo0LLtFwW5hCcQGH0N4WQc/Rn8/Pvoia+TJFOkkFPvrtW9lZOeeNcxQJspvOIERMwiRLsVFlhHEkA==", + "version": "8.6.1", + "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.6.1.tgz", + "integrity": "sha512-0D493aP61w0TJ2A0wy27riRsO7FMQ7FK+KUHOKCSfPvYo0R55aiC6emCVgFUeShH0fq0ICPVzNcgoS+BsbXQCA==", "license": "MIT", "dependencies": { "debug": "^4.4.3", diff --git a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts index c4e1383c5..abe8b48e4 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts @@ -218,6 +218,11 @@ export function resolveDefGraphId( // without either side having to model the scope chain. Node ids are // 0-based, def ids 1-based. An `AMBIGUOUS_POSITION` tombstone (two // callables on one line) falls through to the name-based keys below. + // + // For a closure binding the two query channels still anchor on different + // AST nodes (outer wrapper vs inner callable), but the graph node's + // `startLine` follows the initializer (#2735) so this join matches even + // when the binding is split across lines. const line = defStartLine(def.nodeId, filePath); if (line !== undefined && isPositionQualifiedLocalLabel(def.type)) { const simple = simpleNameOf(qn); @@ -226,13 +231,15 @@ export function resolveDefGraphId( // FAIL CLOSED when a function-local of this name exists in the file (#2699 // follow-up). Falling through to the name keys would end at the label-agnostic, // first-write-wins `simpleKey` below and alias this def onto whichever same-named - // callable was registered first — reproducibly minting a FALSE edge for a - // multiline `const pick =` (the declaration and its initializer land on different - // lines, so the position join misses). A missing edge is the correct failure - // direction for a graph whose consumers include `impact`; a fabricated caller is - // not. Gated on `localNameKey` so this ONLY fires where the collision is real — - // a file with no such local keeps its previous fallback behaviour, which is what - // preserves legitimate anchor differences such as a Vue SFC's `lineOffset`. + // callable was registered first — reproducibly minting a FALSE edge. A missing + // edge is the correct failure direction for a graph whose consumers include + // `impact`; a fabricated caller is not. Gated on `localNameKey` so this ONLY + // fires where the collision is real — a file with no such local keeps its + // previous fallback behaviour, which is what preserves legitimate anchor + // differences such as a Vue SFC's `lineOffset`. + // + // Multi-line closure bindings are NOT this case anymore (#2735): their graph + // `startLine` follows the initializer, so the position key above hits. if (nodeLookup.get(localNameKey(filePath, def.type, simple)) !== undefined) { return undefined; } diff --git a/gitnexus/src/core/ingestion/workers/callable-id.ts b/gitnexus/src/core/ingestion/workers/callable-id.ts index 75d6bfcdd..cb8cea942 100644 --- a/gitnexus/src/core/ingestion/workers/callable-id.ts +++ b/gitnexus/src/core/ingestion/workers/callable-id.ts @@ -19,8 +19,78 @@ * in here is derived, and why only genuinely nested callables get one. */ +import type { NodeLabel, SymbolDefinition } from 'gitnexus-shared'; import type { SyntaxNode } from '../utils/ast-helpers.js'; +import { definitionIdPosition } from '../scope-resolution/utils/definition-id.js'; +const LOCAL_IDENTITY_SUFFIX = /@\d+:\d+$/; + +function simpleDefinitionName(def: SymbolDefinition): string | undefined { + const qualifiedName = def.qualifiedName; + if (qualifiedName === undefined) return undefined; + const dot = qualifiedName.lastIndexOf('.'); + const tail = dot === -1 ? qualifiedName : qualifiedName.slice(dot + 1); + return tail.replace(LOCAL_IDENTITY_SUFFIX, ''); +} + +function containsPosition(node: SyntaxNode, row: number, column: number): boolean { + const start = node.startPosition; + const end = node.endPosition; + if (row < start.row || row > end.row) return false; + if (row === start.row && column < start.column) return false; + if (row === end.row && column > end.column) return false; + return true; +} + +/** + * Zero-based start row that keys the graph-to-scope position join for a bound + * callable (#2735). + * + * Graph-node queries may anchor on an outer binding wrapper while the scope + * channel anchors on the inner callable. The join is line-only, so a multi-line + * binding needs the graph node's `startLine` to follow the semantic definition. + * + * `ParsedFile.localDefs` is the language-agnostic source of that position. + * Matching uses only the canonical label, name, and source range; shared worker + * code does not need to know grammar node types or initializer field names. + * + * Node ids stay on the binding wrapper via `localIdentity(definitionNode)`. + * Missing or ambiguous semantic matches retain the wrapper row, preserving the + * existing fail-closed behavior. + */ +export function boundCallableStartRow( + definitionNode: SyntaxNode, + nodeName: string, + nodeLabel: NodeLabel, + localDefs: readonly SymbolDefinition[] | undefined, + nameNode?: SyntaxNode | null, +): number { + if (localDefs === undefined) return definitionNode.startPosition.row; + + const origin = nameNode?.startPosition ?? definitionNode.startPosition; + let best: { row: number; distance: number } | undefined; + let tied = false; + + for (const def of localDefs) { + if (def.type !== nodeLabel || simpleDefinitionName(def) !== nodeName) continue; + const position = definitionIdPosition(def.nodeId, def.filePath); + if (position === undefined) continue; + + const row = position.line - 1; + if (!containsPosition(definitionNode, row, position.column)) continue; + + const distance = + Math.abs(row - origin.row) * 1_000_000 + Math.abs(position.column - origin.column); + if (best === undefined || distance < best.distance) { + best = { row, distance }; + tied = false; + } else if (distance === best.distance && row !== best.row) { + tied = true; + } + } + + return best !== undefined && !tied ? best.row : definitionNode.startPosition.row; +} /** * A function-local callable's own name segment: its name plus its declaration * position. diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index f467ef852..1914b0101 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -1,5 +1,9 @@ import { parentPort, threadId, workerData } from 'node:worker_threads'; -import { localIdentity, nestedCallableQualifiedName } from './callable-id.js'; +import { + boundCallableStartRow, + localIdentity, + nestedCallableQualifiedName, +} from './callable-id.js'; import Parser from 'tree-sitter'; import JavaScript from 'tree-sitter-javascript'; import TypeScript from 'tree-sitter-typescript'; @@ -2240,11 +2244,27 @@ const processFileGroup = ( } } - const startLine = definitionNode - ? definitionNode.startPosition.row + lineOffset - : nameNode - ? nameNode.startPosition.row + lineOffset - : lineOffset; + // #2735: for a bound callable the graph-node capture sits on the OUTER + // wrapper while scope-resolution anchors on the INNER expression. The + // position join is line-only, so `startLine` must follow the initializer + // (ids still use `definitionNode` via `localIdentity`). + const startRow = + definitionNode && + (nodeLabel === 'Function' || nodeLabel === 'Method' || nodeLabel === 'Constructor') + ? boundCallableStartRow( + definitionNode, + nodeName, + nodeLabel, + parsedFile?.localDefs, + nameNode, + ) + : definitionNode?.startPosition.row; + const startLine = + startRow !== undefined + ? startRow + lineOffset + : nameNode + ? nameNode.startPosition.row + lineOffset + : lineOffset; // Compute enclosing class BEFORE node ID — needed to qualify method IDs const needsOwner = @@ -2750,7 +2770,7 @@ const processFileGroup = ( properties: { name: nodeName, filePath: file.path, - startLine: definitionNode ? definitionNode.startPosition.row + lineOffset : startLine, + startLine, endLine: definitionNode ? definitionNode.endPosition.row + lineOffset : startLine, language: language, isExported: diff --git a/gitnexus/src/storage/parse-cache.ts b/gitnexus/src/storage/parse-cache.ts index 1f18d2910..40a89f0f7 100644 --- a/gitnexus/src/storage/parse-cache.ts +++ b/gitnexus/src/storage/parse-cache.ts @@ -143,7 +143,10 @@ 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 = 35; +// v36: bound-callable graph `startLine` follows the initializer so multi-line +// closure bindings join the scope channel (#2735). Warm cache would otherwise +// keep serving wrapper-line startLines and drop the CALLS edge. +const SCHEMA_BUMP = 36; const GITNEXUS_PKG_VERSION = (() => { try { // package.json sits at gitnexus/package.json — two levels up from diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index f3d4df922..1f1808f66 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -631,8 +631,13 @@ export interface RepoMeta { * `#[cfg(test)] mod tests` makes that close to every Rust repo — so a pre-v25 * index holds ids an incremental top-up cannot reconcile and would simply * strand. Force a full re-analyze. + * + * v30: bound-callable graph `startLine` follows the initializer (#2735), so a + * multi-line closure binding joins the scope channel and emits its CALLS edge. + * Pre-v30 indexes keep the wrapper line on unchanged files and would keep + * failing closed (no edge) through the reuse gate. Force a full re-analyze. */ -export const INCREMENTAL_SCHEMA_VERSION = 29; +export const INCREMENTAL_SCHEMA_VERSION = 30; export interface IndexedRepo { repoPath: string; diff --git a/gitnexus/test/integration/closure-review-findings.test.ts b/gitnexus/test/integration/closure-review-findings.test.ts index 8879f7e57..0ed979afe 100644 --- a/gitnexus/test/integration/closure-review-findings.test.ts +++ b/gitnexus/test/integration/closure-review-findings.test.ts @@ -64,19 +64,14 @@ const nodeIdsContaining = async ( }; describeIfWorkerBuilt( - '#2699 review P1-1 — a closure that cannot be named never credits its parent', + '#2735 — a multi-line closure binding is a call SOURCE (not merely fail-closed)', () => { - 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. + it('PHP: both single-line and multi-line bindings emit CALLS to target', async () => { + // Graph-node queries anchor `@definition.function` on the OUTER assignment; + // scope-resolution anchors `@declaration.function` on the INNER closure. + // #2699 made a miss fail closed (no fabricated `outer -> target`). #2735 + // makes the join hit by putting the graph node's `startLine` on the + // initializer, so the real `outer.$multi -> target` edge appears. const edges = await callEdges( 'ml.php', ' Function:ml.php:target']); + expect(edges).toEqual([ + 'Function:ml.php:outer.$multi@4:2 -> Function:ml.php:target', + 'Function:ml.php:outer.$single@3:2 -> Function:ml.php:target', + ]); + }); + + it('Rust: a wrapped closure binding emits CALLS to target', async () => { + const edges = await callEdges( + 'ml.rs', + 'fn target(x: i32) -> i32 { x }\nfn outer() -> i32 {\n' + + ' let handler =\n || target(1);\n handler()\n}\n', + ); + + expect(edges).toEqual([ + 'Function:ml.rs:outer -> Function:ml.rs:outer.handler@2:4', + 'Function:ml.rs:outer.handler@2:4 -> Function:ml.rs:target', + ]); + }); + + it('TypeScript: a multi-line const arrow binding emits CALLS to target', async () => { + const edges = await callEdges( + 'ml.ts', + 'function target(x: number): number { return x; }\nfunction outer(): number {\n' + + ' const single = (x: number) => target(x);\n' + + ' const multi =\n (x: number) => target(x);\n return single(1) + multi(2);\n}\n', + ); + + expect(edges).toEqual([ + 'Function:ml.ts:outer -> Function:ml.ts:outer.multi@3:2', + 'Function:ml.ts:outer -> Function:ml.ts:outer.single@2:2', + 'Function:ml.ts:outer.multi@3:2 -> Function:ml.ts:target', + 'Function:ml.ts:outer.single@2:2 -> Function:ml.ts:target', + ]); + }); + + it('Kotlin: a multi-line val lambda binding emits CALLS to target', async () => { + const edges = await callEdges( + 'ml.kt', + 'fun target(x: Int): Int = x\nfun outer(): Int {\n' + + ' val single = { x: Int -> target(x) }\n' + + ' val multi =\n { x: Int -> target(x) }\n return 1\n}\n', + ); + + expect(edges.some((e) => e.includes('multi') && e.endsWith('-> Function:ml.kt:target'))).toBe( + true, + ); + expect( + edges.some((e) => e.startsWith('Function:ml.kt:outer ->') && e.endsWith('target')), + ).toBe(false); + }); + + it('Ruby: a multi-line lambda do-end binding emits CALLS to target', async () => { + const edges = await callEdges( + 'ml.rb', + 'def target(x)\n x\nend\ndef outer\n' + + ' a = ->(x) { target(x) }\n' + + ' b =\n lambda do |y|\n target(y)\n end\nend\n', + ); + + expect(edges.some((e) => e.includes('.b@') && e.endsWith('-> Method:ml.rb:target#1'))).toBe( + true, + ); + expect(edges.some((e) => e.startsWith('Method:ml.rb:outer#0 ->'))).toBe(false); + }); + + it('Dart: a multi-line var closure binding emits CALLS to target', async () => { + const edges = await callEdges( + 'ml.dart', + 'int target(int x) => x;\nint outer() {\n' + + ' var single = (int x) => target(x);\n' + + ' var multi =\n (int x) => target(x);\n return 1;\n}\n', + ); + + expect( + edges.some((e) => e.includes('multi') && e.endsWith('-> Function:ml.dart:target')), + ).toBe(true); + expect( + edges.some((e) => e.startsWith('Function:ml.dart:outer ->') && e.endsWith('target')), + ).toBe(false); + }); + }, +); + +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 () => { + // Retained as the fail-closed half of #2735: even when the join works, + // `outer` itself must not grow a CALLS edge to `target` — only the + // binding nodes do. + const edges = await callEdges( + 'ml.php', + ' e.startsWith('Function:ml.php:outer ->'))).toBe(false); + expect(edges).toContain('Function:ml.php:outer.$multi@4:2 -> Function:ml.php:target'); }); }, ); diff --git a/gitnexus/test/unit/call-summary-schema-version.test.ts b/gitnexus/test/unit/call-summary-schema-version.test.ts index d0944f8cc..146239be3 100644 --- a/gitnexus/test/unit/call-summary-schema-version.test.ts +++ b/gitnexus/test/unit/call-summary-schema-version.test.ts @@ -73,12 +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 29 (Spring Bean relation schema, #2413)', () => { + it('INCREMENTAL_SCHEMA_VERSION is bumped to 30 (multi-line closure startLine join, #2735)', () => { // 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(29); + expect(INCREMENTAL_SCHEMA_VERSION).toBe(30); }); it('a pre-current stamp fails the `=== INCREMENTAL_SCHEMA_VERSION` reuse gate → forces full re-analyze', () => { @@ -202,7 +202,10 @@ describe('CALL_SUMMARY incremental reuse gate (U-C5)', () => { // so Spring @Bean injection edges (#2413) would be dropped during // persistence → must NOT reuse. expect(passesReuseGate(28)).toBe(false); + // A pre-v30 (v29) index keeps wrapper-line startLines for multi-line closure + // bindings (#2735), so the graph-to-scope join still drops the CALLS edge. + expect(passesReuseGate(29)).toBe(false); // The current stamp passes the gate (incremental top-up eligible). - expect(passesReuseGate(29)).toBe(true); + expect(passesReuseGate(30)).toBe(true); }); }); diff --git a/gitnexus/test/unit/callable-id-lockstep.test.ts b/gitnexus/test/unit/callable-id-lockstep.test.ts index cf7726f6b..5555286e6 100644 --- a/gitnexus/test/unit/callable-id-lockstep.test.ts +++ b/gitnexus/test/unit/callable-id-lockstep.test.ts @@ -26,12 +26,109 @@ import { readFileSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; -import { nestedCallableQualifiedName } from '../../src/core/ingestion/workers/callable-id.js'; +import { + boundCallableStartRow, + nestedCallableQualifiedName, +} from '../../src/core/ingestion/workers/callable-id.js'; +import type { NodeLabel, SymbolDefinition } from 'gitnexus-shared'; import type { SyntaxNode } from '../../src/core/ingestion/utils/ast-helpers.js'; const nodeAt = (row: number, column: number): SyntaxNode => ({ startPosition: { row, column } }) as unknown as SyntaxNode; +function stubNode(opts: { + type: string; + id: number; + row?: number; + column?: number; + endRow?: number; + endColumn?: number; +}): SyntaxNode { + return { + type: opts.type, + id: opts.id, + startPosition: { row: opts.row ?? 0, column: opts.column ?? 0 }, + endPosition: { + row: opts.endRow ?? opts.row ?? 0, + column: opts.endColumn ?? opts.column ?? 0, + }, + } as unknown as SyntaxNode; +} + +const semanticDef = ( + filePath: string, + type: NodeLabel, + name: string, + line: number, + column: number, +): SymbolDefinition => ({ + nodeId: `def:${filePath}#${line}:${column}:${type}:${name}`, + filePath, + type, + qualifiedName: name, +}); + +describe('boundCallableStartRow - #2735 semantic position join', () => { + it('keeps the wrapper row when no semantic definition is available', () => { + const wrapper = stubNode({ + type: 'binding_wrapper', + id: 1, + row: 2, + column: 0, + endRow: 4, + endColumn: 1, + }); + + expect(boundCallableStartRow(wrapper, 'handler', 'Function', undefined)).toBe(2); + }); + + it('uses the matching semantic callable position inside the wrapper', () => { + const wrapper = stubNode({ + type: 'binding_wrapper', + id: 1, + row: 1, + column: 0, + endRow: 5, + endColumn: 10, + }); + const name = stubNode({ type: 'binding_name', id: 2, row: 1, column: 4 }); + const defs = [semanticDef('src/file.ext', 'Function', 'handler', 4, 8)]; + + expect(boundCallableStartRow(wrapper, 'handler', 'Function', defs, name)).toBe(3); + }); + + it('selects by canonical name and label rather than grammar shape', () => { + const wrapper = stubNode({ + type: 'binding_wrapper', + id: 1, + row: 1, + column: 0, + endRow: 8, + endColumn: 10, + }); + const defs = [ + semanticDef('src/file.ext', 'Function', 'sibling', 3, 4), + semanticDef('src/file.ext', 'Method', 'handler', 4, 4), + semanticDef('src/file.ext', 'Function', 'handler', 6, 4), + ]; + + expect(boundCallableStartRow(wrapper, 'handler', 'Function', defs)).toBe(5); + }); + + it('ignores a same-named semantic definition outside the wrapper', () => { + const wrapper = stubNode({ + type: 'binding_wrapper', + id: 1, + row: 4, + column: 0, + endRow: 6, + endColumn: 10, + }); + const defs = [semanticDef('src/file.ext', 'Function', 'handler', 2, 0)]; + + expect(boundCallableStartRow(wrapper, 'handler', 'Function', defs)).toBe(4); + }); +}); describe('nestedCallableQualifiedName — the shared nested-callable id rule', () => { it('qualifies by the enclosing callable AND the declaration position', () => { expect(nestedCallableQualifiedName('run', nodeAt(3, 2), 'save')).toBe('run.save@3:2'); diff --git a/gitnexus/test/unit/incremental-parse-cache.test.ts b/gitnexus/test/unit/incremental-parse-cache.test.ts index 3b2d1dbf0..c9560f43d 100644 --- a/gitnexus/test/unit/incremental-parse-cache.test.ts +++ b/gitnexus/test/unit/incremental-parse-cache.test.ts @@ -101,12 +101,11 @@ describe('fileContentHash', () => { }); describe('PARSE_CACHE_VERSION', () => { - // 34 -> 35 for the Spring @Bean/@Resource side-channel captures (#2413). - // Updated deliberately — this branch cut at 32 while `main` independently took - // 32 (#2742) and 33/34 (#2747), so the pin caught the collision again at merge - // time. Re-check against origin/main before merging, not at branch time. - it('pins SCHEMA_BUMP to 35 so concurrent bumps cannot silently collide (#2736)', () => { - expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(35); + // 35 -> 36 for the bound-callable start-line join (#2735). Updated + // deliberately after main independently took 35 for Spring side-channel + // captures (#2413), so the pin continues to catch concurrent bump collisions. + it('pins SCHEMA_BUMP to 36 so concurrent bumps cannot silently collide (#2736)', () => { + expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(36); }); it('embeds the gitnexus package version (so upgrades invalidate the cache)', () => {