refactor(ingestion): give the nested-callable id rule one definition (#2699)

Review finding (LOW): the lockstep change in this PR shipped without a test.
The plan called for a unit test asserting the two id-derivation phases agree.
Two things changed that plan during execution, both recorded here.

FIRST — there are THREE phases, not two. Re-verifying the plan's assumption
(`grep -n localIdentity`) found a third call site: the worker-path node-id
derivation in `processFileGroup` (parse-worker.ts:2316), whose own comment
already acknowledged the coupling. `impact` on `localIdentity` corroborates:
three direct dependents, all in the Workers module. So the invariant three
phases must agree on is now ONE function, `nestedCallableQualifiedName`, and
divergence requires deleting a call rather than editing a duplicated
expression.

SECOND — the planned `_forTest` alias seam does not work for this module.
`parse-worker.ts` posts a `ready` message to `parentPort` at module scope, so
value-importing it from a unit test throws before any test runs; the existing
unit tests that reference it use `import type` only, which erases. The rules
therefore move to a new pure module, `workers/callable-id.ts`. That is what
makes them testable at all, rather than merely commented.

Pure refactor — no id changes. Verified by the suites that assert exact node
ids (`Function:svc.ts:run.save@7:2`, `Function:c.php:run.$save@3:2`): 74/74
green, and `detect_changes` reports only the three expected symbols and the
two `processFileGroup` flows `impact` predicted.

The test pins both halves: the rule's contract, and a structural assertion
that no site has re-inlined `${prefix}.${localIdentity(...)}` — the unit
assertions alone would still pass if a fourth phase spelled the rule out by
hand, which is exactly how the divergence arose.

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

gitnexus/src/core/ingestion/workers/callable-id.ts

@@ -0,0 +1,62 @@
+/**
+ * The id rules for a callable nested inside another callable (#2699).
+ *
+ * Extracted from `parse-worker.ts` for one reason: **three** phases there
+ * build these ids independently — the definition phase
+ * (`callableOwnQualifiedName`), the caller-attribution phase
+ * (`findEnclosingFunctionId`), and the worker-path node-id derivation in
+ * `processFileGroup`. An id they compute differently is not a test failure;
+ * the caller attaches to a node that does not exist, so the edge is dropped
+ * rather than reported. "Zero dangling edges" is what that looks like from
+ * outside, which is why the divergence #2714 fixed went unnoticed.
+ *
+ * These functions are pure and free of module-scope side effects, unlike
+ * `parse-worker.ts`, which posts a `ready` message to `parentPort` at import
+ * and therefore cannot be value-imported by a unit test at all. That is what
+ * makes the rule testable rather than merely commented.
+ *
+ * See `parse-worker.ts`'s `enclosingCallablePrefix` for how the prefix passed
+ * in here is derived, and why only genuinely nested callables get one.
+ */
+
+import type { SyntaxNode } from '../utils/ast-helpers.js';
+
+/**
+ * A function-local callable's own name segment: its name plus its declaration
+ * position.
+ *
+ * The name chain alone is not enough, and the gap is the language's, not the
+ * grammar's: ECMAScript creates an environment record per function AND per
+ * block, so sibling blocks in one function hold genuinely different bindings —
+ *
+ *     function outer(a) {
+ *       if (a) { const pick = …; return pick(1); }   // one binding
+ *       else   { const pick = …; return pick(2); }   // a DIFFERENT binding
+ *     }
+ *
+ * — and both are `outer.pick` by name. Putting a block token in the qualifier
+ * would tag every local inside any `if`, the common case, and buy nothing over
+ * putting the position on the declaration itself: a declaration's own position
+ * is unique across every environment record it could belong to, without the
+ * qualifier having to enumerate them. One rule, no conditionals, O(1).
+ *
+ * Applied ONLY to locals. Top-level functions and class methods keep their
+ * bare/class-qualified ids, which is what keeps this off the symbols other
+ * files, saved queries and stored references actually address.
+ */
+export const localIdentity = (node: SyntaxNode, name: string): string =>
+  `${name}@${node.startPosition.row}:${node.startPosition.column}`;
+
+/**
+ * The qualified name of a callable nested inside another callable — THE single
+ * definition of that rule, shared by all three id-building phases.
+ *
+ * A comment asking three call sites to stay in step is exactly the invariant
+ * that rots; routing them through one function makes divergence require
+ * deleting a call rather than editing a duplicated expression.
+ */
+export const nestedCallableQualifiedName = (
+  prefix: string,
+  node: SyntaxNode,
+  name: string,
+): string => `${prefix}.${localIdentity(node, name)}`;

gitnexus/src/core/ingestion/workers/parse-worker.ts

@@ -1,4 +1,5 @@
 import { parentPort, threadId, workerData } from 'node:worker_threads';
+import { localIdentity, nestedCallableQualifiedName } from './callable-id.js';
 import Parser from 'tree-sitter';
 import JavaScript from 'tree-sitter-javascript';
 import TypeScript from 'tree-sitter-typescript';
@@ -767,28 +768,12 @@ function getMethodInfo(
  * and keep their existing ids byte-for-byte, which is what bounds the id churn
  * this change forces.
  *
- * `localIdentity` below completes it. The name chain alone is not enough, and
- * the gap is the language's, not the grammar's: ECMAScript creates an
- * environment record per function AND per block, so sibling blocks in one
- * function hold genuinely different bindings —
- *
- *     function outer(a) {
- *       if (a) { const pick = …; return pick(1); }   // one binding
- *       else   { const pick = …; return pick(2); }   // a DIFFERENT binding
- *     }
- *
- * — and both are `outer.pick` by name. Putting a block token in the qualifier
- * would tag every local inside any `if`, the common case, and buy nothing over
- * putting the position on the declaration itself: a declaration's own position
- * is unique across every environment record it could belong to, without the
- * qualifier having to enumerate them. One rule, no conditionals, O(1).
- *
- * Applied ONLY to locals. Top-level functions and class methods keep their
- * bare/class-qualified ids, which is what keeps this off the symbols other
- * files, saved queries and stored references actually address.
+ * `localIdentity` completes it, and both it and the shared
+ * `nestedCallableQualifiedName` rule now live in `./callable-id.ts`: this
+ * module posts a `ready` message to `parentPort` at import, so a unit test
+ * cannot value-import it, and a rule three phases must agree on has to be
+ * testable rather than merely commented (#2714).
  */
-const localIdentity = (node: SyntaxNode, name: string): string =>
-  `${name}@${node.startPosition.row}:${node.startPosition.column}`;
 
 /**
  * Boundary for the enclosing-callable walk (#2699).
@@ -873,9 +858,9 @@ const callableOwnQualifiedName = (
   if (cached !== undefined) return cached;
 
   const efnResult = provider.methodExtractor?.extractFunctionName?.(fnNode, filePath);
-  // An anonymous callable has no name of its own, so it IS its position —
-  // `localIdentity` supplies the same suffix the local branch below appends,
-  // and the two must not stack.
+  // An anonymous callable has no name of its own, so it IS its position: the
+  // `ownName === null` branch below carries the position INSTEAD of a name,
+  // never in addition to one, so the two spellings cannot stack.
   const ownName = efnResult?.funcName ?? genericFuncName(fnNode) ?? null;
 
   const prefix = enclosingCallablePrefix(fnNode, filePath, provider);
@@ -884,12 +869,11 @@ const callableOwnQualifiedName = (
       ? cachedFindEnclosingClassInfo(fnNode, filePath, provider.resolveEnclosingOwner)
       : null;
   const owner = prefix ?? classInfo?.className;
-  const localName = localIdentity(fnNode, ownName ?? 'fn');
   const result =
     prefix !== undefined
-      ? `${prefix}.${localName}`
+      ? nestedCallableQualifiedName(prefix, fnNode, ownName ?? 'fn')
       : ownName === null
-        ? localName
+        ? localIdentity(fnNode, 'fn')
         : owner
           ? `${owner}.${ownName}`
           : ownName;
@@ -947,15 +931,16 @@ const findEnclosingFunctionId = (
         const nestedPrefix = enclosingCallablePrefix(current, filePath, provider);
         const ownerName =
           nestedPrefix ?? classInfo?.className ?? standaloneMethodInfo?.receiverType ?? undefined;
-        // Lockstep with the definition phase. `callableOwnQualifiedName` appends
-        // `localIdentity` to a nested callable's OWN name segment, under exactly
-        // this condition (`prefix !== undefined`). Omitting it here made the two
-        // phases derive different ids for the same callable — and the failure is
-        // silent: the caller id names a node that does not exist, so the edge is
-        // dropped rather than reported. The condition is deliberately identical
-        // to the definition phase's so the two cannot diverge again.
-        const ownSegment = nestedPrefix !== undefined ? localIdentity(current, funcName) : funcName;
-        const qualifiedName = ownerName ? `${ownerName}.${ownSegment}` : ownSegment;
+        // Lockstep with the other two id-building phases — see
+        // `nestedCallableQualifiedName`, which is the shared rule. When a
+        // nested prefix exists it IS `ownerName`, so this branch and the
+        // owner branch below cannot disagree about which prefix applies.
+        const qualifiedName =
+          nestedPrefix !== undefined
+            ? nestedCallableQualifiedName(nestedPrefix, current, funcName)
+            : ownerName
+              ? `${ownerName}.${funcName}`
+              : funcName;
         // Include #<arity> suffix to match definition-phase Method/Constructor IDs.
         // Use the same MethodExtractor (getMethodInfo) as the definition phase.
         // When same-arity collisions exist, also append ~type1,type2.
@@ -2328,7 +2313,7 @@ const processFileGroup = (
               qualifiedTypeName !== undefined
             ? qualifiedTypeName
             : nestedCallablePrefix !== undefined && definitionNode
-              ? `${nestedCallablePrefix}.${localIdentity(definitionNode, nodeName)}`
+              ? nestedCallableQualifiedName(nestedCallablePrefix, definitionNode, nodeName)
               : enclosingClassInfo
                 ? `${enclosingClassInfo.className}.${nodeName}`
                 : nodeName;

gitnexus/test/unit/callable-id-lockstep.test.ts

@@ -0,0 +1,77 @@
+/**
+ * #2699 / #2714 — the nested-callable id rule has ONE definition.
+ *
+ * Three phases in `parse-worker.ts` build the id of a callable nested inside
+ * another callable, independently: the definition phase
+ * (`callableOwnQualifiedName`), the caller-attribution phase
+ * (`findEnclosingFunctionId`), and the worker-path node-id derivation in
+ * `processFileGroup`. They must agree byte-for-byte, and when they do not the
+ * failure is SILENT — the caller id names a node that does not exist, so the
+ * edge is dropped rather than reported. "Zero dangling edges" is what that
+ * looks like from the outside, which is why it went unnoticed.
+ *
+ * Caller attribution really did omit the position suffix until #2714. The fix
+ * routed all three through `nestedCallableQualifiedName`; this file pins both
+ * halves of that — the rule's contract, and the fact that no call site has
+ * re-inlined it.
+ *
+ * The rule reads only `startPosition` off the node, so a positional stub is a
+ * complete input here; parsing real source would add a tree-sitter dependency
+ * without testing anything more of this function.
+ *
+ * The rules live in `callable-id.ts` rather than `parse-worker.ts` precisely so
+ * this file can exist: parse-worker posts a `ready` message to `parentPort` at
+ * import, so value-importing it from a unit test throws before any test runs.
+ */
+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 type { SyntaxNode } from '../../src/core/ingestion/utils/ast-helpers.js';
+
+const nodeAt = (row: number, column: number): SyntaxNode =>
+  ({ startPosition: { row, column } }) as unknown as SyntaxNode;
+
+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');
+  });
+
+  it('takes the position from the node, never from the name', () => {
+    // Guards against a "fix" that formats the suffix from anything but the
+    // declaration site — the position is what makes the id unique.
+    expect(nestedCallableQualifiedName('outer', nodeAt(12, 9), 'fn')).toBe('outer.fn@12:9');
+  });
+
+  it('separates same-named siblings in different blocks', () => {
+    // The case names alone cannot express (#2699): two `pick` bindings in the
+    // if/else arms of one function are genuinely different bindings, and both
+    // are `outer.pick` by name.
+    const first = nestedCallableQualifiedName('outer', nodeAt(2, 4), 'pick');
+    const second = nestedCallableQualifiedName('outer', nodeAt(5, 4), 'pick');
+
+    expect(first).not.toBe(second);
+  });
+
+  it('carries a multi-level chain verbatim in the prefix', () => {
+    expect(nestedCallableQualifiedName('A.outer.mid', nodeAt(7, 0), 'inner')).toBe(
+      'A.outer.mid.inner@7:0',
+    );
+  });
+});
+
+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.
+    const source = readFileSync(
+      fileURLToPath(new URL('../../src/core/ingestion/workers/parse-worker.ts', import.meta.url)),
+      'utf8',
+    );
+    const inlined = source.match(/\}\.\$\{localIdentity\(/g) ?? [];
+
+    expect(inlined).toEqual([]);
+  });
+});