From 8931f60e7853891f4af7331ea63f8f94ab1a2b33 Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Thu, 21 May 2026 09:20:40 +0100 Subject: [PATCH] refactor(ingestion): address code-review findings on object-literal owner resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Multi-agent code review on the prior commit surfaced 7 actionable findings, all walked through and applied here. None change observable behavior for issue #1358's fix; all harden correctness, predicate stability, and test signal. - #1 (P1 / 3-reviewer corroboration): Case 5 in receiver-bound-calls.ts no longer hand-builds graph.addRelationship + a dedup key. New tryEmitEdgeWithExplicitTargetId in edges.ts takes a pre-resolved target id (the canonical Method nodeId from the parser) and reuses every invariant of tryEmitEdge: dedup-key format, collapse-flag honoring, caller-id resolution, rel-id shape, mapReferenceKindToEdgeType for read/write ACCESSES. This also lands the adversarial reviewer's "F2" follow-up (hardcoded type: 'CALLS' for non-call sites) for free. - #2 (P2 cross-reviewer): findValueBindingInScope's predicate inverted from denylist ("not class-like and not callable") to explicit allowlist matching reconcileOwnership's registration set: Const | Variable | Property | Static. Extracted as isOwnableValueLabel so future NodeLabel additions require an explicit opt-in. - #6 (P2): walkScopeChain() extracted; both findClassBindingInScope and findValueBindingInScope now route through it. Local scope.bindings are exhausted BEFORE lookupBindingsAt (imported/augmented) at every scope level — preserves JavaScript lexical scoping where a local const shadows an imported binding of the same name. Behavior was already correct in findClassBindingInScope but was implicit; now it is the walker's explicit, documented contract. - #7 (P2): scope-walker duplication closed. findClassBindingInScope and findValueBindingInScope reduce to thin wrappers over walkScopeChain with their respective predicate. findClassBindingInScope keeps its qualifiedNames + dotted-name fallback tail. - #3 (P2): parse-worker.ts hoists `const ownerId = enclosingClassId ?? objectLiteralOwnerInfo?.ownerId` once before the symbol push, dropping the duplicated coalesce + `as string` cast. Matches the cast-free pattern at parsing-processor.ts:793. HAS_METHOD emit site reuses the same hoisted local. - #4 (P2): object-literal-owner-resolution.test.ts Test A's CALLS-edge assertion no longer matches by name alone. .toEqual now pins the canonical target id (Method:src/service.ts:getUser#1 via generateId), confidence (0.85), and reason ('import-resolved'). A regression that emits the edge at confidence=0, with the wrong reason, or against a phantom Method node now fails the test. - #5 (P2): worker-parity test adds a CI tripwire — when CI=1 and dist/parse-worker.js is missing, throw at module top with a clear message. Locally, skipIf(!hasDistWorker) keeps the fast-iteration experience; CI cannot pass with U3 (worker-path ownerId) unverified. Verification: tsc --noEmit clean. Targeted regression sweep on ast-helpers-object-literal-binding (13), object-literal-owner-resolution (9), has-method (60), cross-file-binding (40) — 122/122 pass. Full unit sweep: 6056/6056. Integration suite: 1 pre-existing Windows-flake in worker-pool.test.ts (passes 28/28 in isolation) unrelated to this diff. --- .../scope-resolution/graph-bridge/edges.ts | 51 ++++++++++++ .../passes/receiver-bound-calls.ts | 57 +++++++------ .../scope-resolution/scope/walkers.ts | 81 +++++++++++-------- .../core/ingestion/workers/parse-worker.ts | 12 ++- .../object-literal-owner-resolution.test.ts | 35 +++++++- 5 files changed, 163 insertions(+), 73 deletions(-) diff --git a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/edges.ts b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/edges.ts index 080972691..2562868e4 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/edges.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/edges.ts @@ -98,3 +98,54 @@ export function tryEmitEdge( }); return true; } + +/** + * Variant of `tryEmitEdge` that takes a pre-resolved target graph id + * instead of resolving it from a `SymbolDefinition`. Used by the + * value-receiver-owner bridge (`receiver-bound-calls.ts` Case 5) where + * the picked owner-indexed method def carries no `qualifiedName` (object + * literals have no class owner to seed it) and therefore cannot + * round-trip through `resolveDefGraphId`. The def's `nodeId` IS the + * canonical graph node id (written by the parse phase), so the caller + * passes it directly. + * + * All other invariants of `tryEmitEdge` apply: dedup key shape, collapse + * flag honoring, edge-type mapping, caller-id resolution. + */ +export function tryEmitEdgeWithExplicitTargetId( + graph: KnowledgeGraph, + scopes: ScopeResolutionIndexes, + nodeLookup: GraphNodeLookup, + site: { + readonly inScope: ScopeId; + readonly atRange: { startLine: number; startCol: number }; + readonly kind: string; + }, + targetGraphId: string, + reason: string, + seen: Set, + confidence = 0.85, + collapseByCallerTarget = false, +): boolean { + const callerGraphId = resolveCallerGraphId(site.inScope, scopes, nodeLookup); + const edgeType = mapReferenceKindToEdgeType(site.kind as Reference['kind']); + if (callerGraphId === undefined) return false; + if (edgeType === undefined) return false; + + const useCollapsed = collapseByCallerTarget && edgeType === 'CALLS'; + const dedupKey = useCollapsed + ? `${edgeType}:${callerGraphId}->${targetGraphId}` + : `${edgeType}:${callerGraphId}->${targetGraphId}:${site.atRange.startLine}:${site.atRange.startCol}`; + if (seen.has(dedupKey)) return false; + seen.add(dedupKey); + + graph.addRelationship({ + id: `rel:${dedupKey}`, + sourceId: callerGraphId, + targetId: targetGraphId, + type: edgeType, + confidence, + reason, + }); + return true; +} diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/receiver-bound-calls.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/receiver-bound-calls.ts index 39cb8ce23..6a9469693 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/receiver-bound-calls.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/passes/receiver-bound-calls.ts @@ -54,9 +54,9 @@ import { findValueBindingInScope, isClassLike, } from '../scope/walkers.js'; -import { tryEmitEdge } from '../graph-bridge/edges.js'; +import { tryEmitEdge, tryEmitEdgeWithExplicitTargetId } from '../graph-bridge/edges.js'; import { resolveCompoundReceiverClass } from '../passes/compound-receiver.js'; -import { resolveCallerGraphId, resolveDefGraphId } from '../graph-bridge/ids.js'; +import { resolveDefGraphId } from '../graph-bridge/ids.js'; import { narrowOverloadCandidates, isOverloadAmbiguousAfterNormalization, @@ -730,9 +730,10 @@ export function emitReceiverBoundCalls( // // Object-literal methods do not carry a `qualifiedName` (no class // owner to seed it), so the picked def cannot round-trip through - // `tryEmitEdge` → `resolveDefGraphId`. We emit directly using - // `picked.nodeId` (already the canonical graph node id, written by - // the legacy parse phase). + // `tryEmitEdge` → `resolveDefGraphId`. We use + // `tryEmitEdgeWithExplicitTargetId` instead, passing `picked.nodeId` + // directly — same dedup-key shape, collapse-flag honoring, and + // caller resolution as `tryEmitEdge`. const valueDef = findValueBindingInScope(site.inScope, receiverName, scopes); if (valueDef !== undefined) { const ownerGraphId = @@ -743,31 +744,27 @@ export function emitReceiverBoundCalls( continue; } if (picked !== undefined) { - const callerGraphId = resolveCallerGraphId(site.inScope, scopes, nodeLookup); - if (callerGraphId !== undefined) { - const reason = - site.kind === 'write' || site.kind === 'read' - ? site.kind - : picked.filePath !== parsed.filePath - ? 'import-resolved' - : 'global'; - const confidence = site.kind === 'write' || site.kind === 'read' ? 1.0 : 0.85; - const dedupKey = `CALLS:${callerGraphId}->${picked.nodeId}:${site.atRange.startLine}:${site.atRange.startCol}`; - if (!seen.has(dedupKey)) { - seen.add(dedupKey); - graph.addRelationship({ - id: `rel:${dedupKey}`, - sourceId: callerGraphId, - targetId: picked.nodeId, - type: 'CALLS', - confidence, - reason, - }); - emitted++; - } - handledSites.add(siteKey); - continue; - } + const reason = + site.kind === 'write' || site.kind === 'read' + ? site.kind + : picked.filePath !== parsed.filePath + ? 'import-resolved' + : 'global'; + const confidence = site.kind === 'write' || site.kind === 'read' ? 1.0 : 0.85; + const ok = tryEmitEdgeWithExplicitTargetId( + graph, + scopes, + nodeLookup, + site, + picked.nodeId, + reason, + seen, + confidence, + collapse, + ); + if (ok) emitted++; + handledSites.add(siteKey); + continue; } } } diff --git a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts index 1b5de4516..a9bb188d2 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/scope/walkers.ts @@ -165,28 +165,9 @@ export function findClassBindingInScope( receiverName: string, scopes: ScopeResolutionIndexes, ): SymbolDefinition | undefined { - let currentId: ScopeId | null = startScope; - const visited = new Set(); - while (currentId !== null) { - if (visited.has(currentId)) return undefined; - visited.add(currentId); - const scope = scopes.scopeTree.getScope(currentId); - if (scope === undefined) return undefined; + const local = walkScopeChain(startScope, receiverName, scopes, (def) => isClassLike(def.type)); + if (local !== undefined) return local; - const localBindings = scope.bindings.get(receiverName); - if (localBindings !== undefined) { - for (const b of localBindings) { - if (isClassLike(b.def.type)) return b.def; - } - } - - const importedBindings = lookupBindingsAt(currentId, receiverName, scopes); - for (const b of importedBindings) { - if (isClassLike(b.def.type)) return b.def; - } - - currentId = scope.parent; - } // Fallback for languages (Go) where namespace-style imports don't // create scope bindings: resolve via QualifiedNameIndex. Only fires // when the scope-chain walk found nothing; single-match wins. @@ -212,17 +193,31 @@ export function findClassBindingInScope( } /** - * Look up a value-binding (non-class-like, non-callable) by name in + * Predicate for value-receiver bridge: the labels for which + * `reconcileOwnership` registers methods/fields under the def's + * `nodeId` as the `ownerId`. Explicit allowlist so future NodeLabel + * additions (Module, Namespace, TypeAlias, EnumMember, etc.) do NOT + * silently widen the bridge — adding a new ownerable label requires + * touching both this predicate and `reconcileOwnership`. + * + * See: `scope-resolution/pipeline/reconcile-ownership.ts` Property / + * Variable / Const / Static registration block. + */ +export function isOwnableValueLabel(t: string): boolean { + return t === 'Const' || t === 'Variable' || t === 'Property' || t === 'Static'; +} + +/** + * Look up a value-binding (Const/Variable/Property/Static) by name in * the given scope's chain. Used by the value-receiver-owner bridge * for object-literal services such as: * * export const fooService = { getUser(id) {...} }; * * where `fooService` is a `Const`/`Variable` whose `nodeId` is the - * `ownerId` of the member method but where neither `findClassBindingInScope` - * (rejects non-class-like) nor `findReceiverTypeBinding` (no typeBinding for - * an unannotated literal) finds it. Returns the first non-class-like, - * non-callable binding match. + * `ownerId` of the member method. Neither `findClassBindingInScope` + * (rejects non-class-like) nor `findReceiverTypeBinding` (no typeBinding + * for an unannotated literal) finds it. * * Mirrors `findClassBindingInScope` exactly; only the accepted def-type * predicate differs. @@ -231,6 +226,27 @@ export function findValueBindingInScope( startScope: ScopeId, receiverName: string, scopes: ScopeResolutionIndexes, +): SymbolDefinition | undefined { + return walkScopeChain(startScope, receiverName, scopes, (def) => isOwnableValueLabel(def.type)); +} + +/** + * Generic scope-chain walker. Walks from `startScope` toward the root, + * consulting both the local `scope.bindings` channel and the dual-source + * `lookupBindingsAt` view (finalized + augmented). At each scope, local + * bindings are exhausted BEFORE imported/augmented bindings — preserves + * JavaScript-style lexical scoping where a local `const x` shadows an + * imported `x` of the same name. + * + * Returns the first binding `def` matching `predicate`. Cycles in the + * scope graph terminate the walk (defensive — should not occur in + * well-formed inputs). + */ +function walkScopeChain( + startScope: ScopeId, + name: string, + scopes: ScopeResolutionIndexes, + predicate: (def: SymbolDefinition) => boolean, ): SymbolDefinition | undefined { let currentId: ScopeId | null = startScope; const visited = new Set(); @@ -240,19 +256,18 @@ export function findValueBindingInScope( const scope = scopes.scopeTree.getScope(currentId); if (scope === undefined) return undefined; - const isValueLike = (t: string): boolean => - !isClassLike(t) && t !== 'Function' && t !== 'Method' && t !== 'Constructor'; - - const localBindings = scope.bindings.get(receiverName); + // Local first: a `const x` in this scope shadows any imported `x`. + const localBindings = scope.bindings.get(name); if (localBindings !== undefined) { for (const b of localBindings) { - if (isValueLike(b.def.type)) return b.def; + if (predicate(b.def)) return b.def; } } - const importedBindings = lookupBindingsAt(currentId, receiverName, scopes); + // Then imported/augmented bindings — only consulted when no local match. + const importedBindings = lookupBindingsAt(currentId, name, scopes); for (const b of importedBindings) { - if (isValueLike(b.def.type)) return b.def; + if (predicate(b.def)) return b.def; } currentId = scope.parent; diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index 56b50399b..ccb88e0de 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -2311,6 +2311,7 @@ const processFileGroup = ( }); // enclosingClassId already computed above (before nodeId generation) + const ownerId = enclosingClassId ?? objectLiteralOwnerInfo?.ownerId; result.symbols.push({ filePath: file.path, @@ -2327,9 +2328,7 @@ const processFileGroup = ( ...(classTemplateArguments !== undefined && classTemplateArguments.length > 0 ? { templateArguments: classTemplateArguments } : {}), - ...((enclosingClassId ?? objectLiteralOwnerInfo?.ownerId) - ? { ownerId: (enclosingClassId ?? objectLiteralOwnerInfo?.ownerId) as string } - : {}), + ...(ownerId !== undefined ? { ownerId } : {}), visibility: methodProps.visibility as string | undefined, isStatic: methodProps.isStatic as boolean | undefined, isReadonly: methodProps.isReadonly as boolean | undefined, @@ -2362,12 +2361,11 @@ const processFileGroup = ( }); // ── HAS_METHOD / HAS_PROPERTY: link member to enclosing class ── - const ownerIdForMemberEdge = enclosingClassId ?? objectLiteralOwnerInfo?.ownerId ?? null; - if (ownerIdForMemberEdge) { + if (ownerId !== undefined) { const memberEdgeType = nodeLabel === 'Property' ? 'HAS_PROPERTY' : 'HAS_METHOD'; result.relationships.push({ - id: generateId(memberEdgeType, `${ownerIdForMemberEdge}->${nodeId}`), - sourceId: ownerIdForMemberEdge, + id: generateId(memberEdgeType, `${ownerId}->${nodeId}`), + sourceId: ownerId, targetId: nodeId, type: memberEdgeType, confidence: 1.0, diff --git a/gitnexus/test/integration/object-literal-owner-resolution.test.ts b/gitnexus/test/integration/object-literal-owner-resolution.test.ts index 72a9afde1..b0641d7f0 100644 --- a/gitnexus/test/integration/object-literal-owner-resolution.test.ts +++ b/gitnexus/test/integration/object-literal-owner-resolution.test.ts @@ -50,6 +50,19 @@ const DIST_WORKER = path.resolve( ); const hasDistWorker = fs.existsSync(DIST_WORKER); +// CI tripwire: worker-parity test (Test B below) silently skips when +// `dist/parse-worker.js` is missing. That's fine locally — devs may not +// have run `npm run build` — but on CI a missing dist would mean U3 +// (worker-path ownerId emission) is unverified. Fail hard so a missing +// dist surfaces as a red build, not a green test with a silent skip. +// Locally, run `npm run build` before this suite to exercise worker mode. +if (!hasDistWorker && process.env.CI) { + throw new Error( + 'dist/parse-worker.js missing on CI — worker-parity test would silently skip. ' + + 'Ensure the build runs before this suite.', + ); +} + /** Materialise a tiny fixture repo on disk. Returns the absolute repo root. */ function writeFixture(files: Record): string { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gnx-objlit-')); @@ -124,10 +137,26 @@ describe('object-literal owner resolution — sequential pipeline (PR #1718)', ( expect(fooServiceNode!.id).toBe(expectedNodeId); }); - it('emits a CALLS edge from caller to getUser (issue #1358 fix)', () => { + it('emits a CALLS edge from caller to getUser with the expected target/confidence/reason (issue #1358 fix)', () => { const calls = getRelationships(result, 'CALLS'); - const callerToGetUser = calls.filter((e) => e.source === 'caller' && e.target === 'getUser'); - expect(callerToGetUser.length).toBe(1); + const callerToGetUser = calls + .filter((e) => e.source === 'caller' && e.target === 'getUser') + .map((e) => ({ + targetId: e.rel.targetId, + confidence: e.rel.confidence, + reason: e.rel.reason, + })); + + // The Method node id encodes arity disambiguation (#1 = one-arity overload). + // Pin the canonical id so a regression that targets a phantom node fails. + const expectedTargetId = generateId('Method', 'src/service.ts:getUser#1'); + expect(callerToGetUser).toEqual([ + { + targetId: expectedTargetId, + confidence: 0.85, + reason: 'import-resolved', + }, + ]); }); });