From a0bce3a8cfab8e49be5bea8d5dabb2cf0c7f850d Mon Sep 17 00:00:00 2001
From: Gergo Magyar <gergomagyar@icloud.com>
Date: Tue, 21 Apr 2026 08:38:52 +0100
Subject: [PATCH] fix(scope-resolution): filter export index to module-level
 defs + label-prefixed qualified key
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Codex adversarial review on PR #980 flagged that
buildWorkspaceResolutionIndex feeds defsByFileAndName and
callablesBySimpleName from parsed.localDefs — the flat set of every
def in the file including methods, fields, and nested functions.
findExportedDef / findExportedDefByName treat those maps as
file-level exports, so `mod.save()` could silently bind to User.save
whenever a method's simple name appeared first in parse order.

Plan: docs/plans/2026-04-21-001-fix-workspace-index-module-scope-only-plan.md

Fix layers:

1. workspace-index.ts: split the single parsed.localDefs loop into
   two passes:
   - Module-export pass: iterate moduleScope.ownedDefs PLUS ownedDefs
     of every child scope whose parent is the module scope. Top-level
     class and function declarations each live in their own scope
     with parent=module, not in moduleScope.ownedDefs directly, so
     the "parent === moduleScope.id" walk is required to reach them.
     Methods (scope.parent === Class scope) and nested functions
     (scope.parent === another Function scope) are excluded.
   - Member-by-owner pass: keeps iterating parsed.localDefs since
     that map is keyed on ownerId and correctly saw class-owned defs
     before this change.

2. graph-bridge/node-lookup.ts: qualified keys now live in a separate
   keyspace (`<q>:filePath::<label>::<qualifiedName>`) and include
   the node label. Without the label prefix, a top-level `def save`
   (Function, qualifier `save`) would collide with a class method
   `User.save` (Method, simple name `save`) in the same simple-key
   slot because the Function's qualifier happens to equal the
   Method's simple name. The label differentiates them.

3. graph-bridge/ids.ts: resolveDefGraphId uses the new
   type-prefixed qualified key when def.type is set. Simple-name
   fallback retained for languages that don't yet synthesize
   qualifiers on their defs.

Test fixture: python-module-export-vs-method-collision places
`class User: def save` BEFORE top-level `def save` — parse order
that exposes the bug (class method enters the index first). Three
new integration assertions:
  - `mod.save(x)` resolves to the module-level Function, not User.save
  - `u.save()` resolves to User.save Method
  - Exactly two CALLS edges to `save` exist, one per intended target

Fixture confirmed failing before the workspace-index fix (bug
reproduced), passing after.

Verification: 197/197 test/integration/resolvers/python.test.ts pass
both REGISTRY_PRIMARY_PYTHON=0 and =1. 523/523 related unit tests.
tsc --noEmit clean.
---
 .../scope-resolution/graph-bridge/ids.ts      | 34 +++++--
 .../graph-bridge/node-lookup.ts               | 38 ++++++--
 .../scope-resolution/workspace-index.ts       | 94 +++++++++++++------
 .../app.py                                    | 11 +++
 .../mod.py                                    | 21 +++++
 .../test/integration/resolvers/python.test.ts | 54 +++++++++++
 6 files changed, 206 insertions(+), 46 deletions(-)
 create mode 100644 gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/app.py
 create mode 100644 gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/mod.py

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 0843ca59a..5f12a3e6f 100644
--- a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts
+++ b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/ids.ts
@@ -17,32 +17,46 @@
  * migrate.
  */
 
-import type { ScopeId, SymbolDefinition } from 'gitnexus-shared';
+import type { NodeLabel, ScopeId, SymbolDefinition } from 'gitnexus-shared';
 import type { ScopeResolutionIndexes } from '../../model/scope-resolution-indexes.js';
 import { generateId } from '../../../../lib/utils.js';
-import { isLinkableLabel, type GraphNodeLookup } from '../graph-bridge/node-lookup.js';
+import {
+  isLinkableLabel,
+  qualifiedKey,
+  simpleKey,
+  type GraphNodeLookup,
+} from '../graph-bridge/node-lookup.js';
 
 /**
  * Look up a `SymbolDefinition` in the graph node lookup.
  *
- * Tries the fully-qualified name FIRST — that's the only correct key
- * when two classes in the same file define a method with the same
- * simple name (`class User: def save` + `class Document: def save`).
+ * Tries the type-prefixed fully-qualified key FIRST. That's the only
+ * correct key when:
+ *   - Two classes in the same file define a method with the same
+ *     simple name (`class User: def save` + `class Document: def save`).
+ *   - A top-level function and a class method share a simple name
+ *     (`def save` + `class User: def save` — the Function's qualifier
+ *     is just `save`, which would alias the Method's simple-key slot
+ *     without the type prefix).
+ *
  * Falls back to the simple name for definitions whose qualifier the
  * lookup didn't capture (rare, but keeps cross-file simple-name
- * resolution working).
+ * resolution working for languages that don't yet synthesize
+ * qualifiers).
  */
 export function resolveDefGraphId(
   filePath: string,
-  def: { qualifiedName?: string },
+  def: { qualifiedName?: string; type?: NodeLabel },
   nodeLookup: GraphNodeLookup,
 ): string | undefined {
   const qn = def.qualifiedName;
   if (qn === undefined || qn.length === 0) return undefined;
-  const qualifiedHit = nodeLookup.get(`${filePath}::${qn}`);
-  if (qualifiedHit !== undefined) return qualifiedHit;
+  if (def.type !== undefined) {
+    const qualifiedHit = nodeLookup.get(qualifiedKey(filePath, def.type, qn));
+    if (qualifiedHit !== undefined) return qualifiedHit;
+  }
   const simpleName = qn.lastIndexOf('.') === -1 ? qn : qn.slice(qn.lastIndexOf('.') + 1);
-  return nodeLookup.get(`${filePath}::${simpleName}`);
+  return nodeLookup.get(simpleKey(filePath, simpleName));
 }
 
 /** Derive the simple (unqualified) name of a def from its `qualifiedName`. */
diff --git a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts
index bb69670a7..db4c6024c 100644
--- a/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts
+++ b/gitnexus/src/core/ingestion/scope-resolution/graph-bridge/node-lookup.ts
@@ -41,6 +41,25 @@ function parseQualifiedFromId(id: string, label: NodeLabel, filePath: string): s
   return hash === -1 ? suffix : suffix.slice(0, hash);
 }
 
+/**
+ * Build a qualified-key string in a separate keyspace from simple-key
+ * strings. Prefix `<q>` can't appear in a valid filePath on any OS, so
+ * no collision between the two keyspaces is possible.
+ *
+ * Includes the node label so a top-level `def save` (Function,
+ * qualifier = `save`) doesn't alias a class method `User.save` (Method,
+ * simple name = `save`) whose Function-typed qualifier would collapse
+ * to the same simple-key slot in a single map.
+ */
+export function qualifiedKey(filePath: string, label: NodeLabel, qualifiedName: string): string {
+  return `<q>:${filePath}::${label}::${qualifiedName}`;
+}
+
+/** Simple-name key (legacy fallback keyspace — no `<q>` prefix). */
+export function simpleKey(filePath: string, name: string): string {
+  return `${filePath}::${name}`;
+}
+
 export function buildGraphNodeLookup(graph: KnowledgeGraph): GraphNodeLookup {
   const lookup = new Map<string, string>();
   for (const node of graph.iterNodes()) {
@@ -52,15 +71,18 @@ export function buildGraphNodeLookup(graph: KnowledgeGraph): GraphNodeLookup {
     if (props.filePath === undefined || props.name === undefined) continue;
     if (!isLinkableLabel(node.label)) continue;
 
-    // Primary key: fully-qualified name when available. Class nodes
-    // carry `qualifiedName` in their properties (set by the parsing
-    // processor). Method/Function nodes do not, so derive the
-    // qualifier from the node id — that's where the parse-phase
-    // encoded it.
+    // Primary key: fully-qualified name + label, in a separate
+    // keyspace from simple names. Class nodes carry `qualifiedName`
+    // in their properties (set by the parsing processor).
+    // Method/Function nodes do not, so derive the qualifier from the
+    // node id — that's where the parse-phase encoded it. Including
+    // the label avoids a collision when a free Function's qualifier
+    // happens to equal a Method's simple name (e.g. top-level
+    // `def save` vs `class User: def save`).
     const qualified =
       props.qualifiedName ?? parseQualifiedFromId(node.id, node.label, props.filePath);
     if (qualified !== undefined && qualified.length > 0) {
-      const qKey = `${props.filePath}::${qualified}`;
+      const qKey = qualifiedKey(props.filePath, node.label, qualified);
       if (!lookup.has(qKey)) lookup.set(qKey, node.id);
     }
 
@@ -68,8 +90,8 @@ export function buildGraphNodeLookup(graph: KnowledgeGraph): GraphNodeLookup {
     // the caller doesn't know the qualifier (unqualified free-call
     // fallback, cross-file resolution where MethodRegistry already
     // disambiguated the owner).
-    const simpleKey = `${props.filePath}::${props.name}`;
-    if (!lookup.has(simpleKey)) lookup.set(simpleKey, node.id);
+    const sKey = simpleKey(props.filePath, props.name);
+    if (!lookup.has(sKey)) lookup.set(sKey, node.id);
   }
   return lookup;
 }
diff --git a/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts b/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts
index b8ce38d54..72428b2e3 100644
--- a/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts
+++ b/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts
@@ -26,16 +26,24 @@ export interface WorkspaceResolutionIndex {
   readonly classScopeByDefId: ReadonlyMap<string, Scope>;
 
   /** Owner def `nodeId` → (simple-member-name → owned `SymbolDefinition`).
-   *  Replaces `findOwnedMember`'s O(N × D) walk with O(1) lookup. */
+   *  Replaces `findOwnedMember`'s O(N × D) walk with O(1) lookup.
+   *  Built from `parsed.localDefs` so class-owned members land in the
+   *  right bucket via their `ownerId`. */
   readonly memberByOwner: ReadonlyMap<string, ReadonlyMap<string, SymbolDefinition>>;
 
-  /** File path → (simple-name → first matching `SymbolDefinition`).
-   *  Replaces `findExportedDef`'s O(N × D) walk. */
+  /** File path → (simple-name → first matching module-scope-owned
+   *  `SymbolDefinition`). Backs `findExportedDef` — the lookup for
+   *  `from mod import X` / `mod.X()` targets. Only defs directly
+   *  owned by the file's `Module` scope are indexed here; methods,
+   *  fields, and nested-function defs are NOT visible as file-level
+   *  exports. First-seen-within-module wins. */
   readonly defsByFileAndName: ReadonlyMap<string, ReadonlyMap<string, SymbolDefinition>>;
 
   /** Workspace-wide simple-name fallback: simple-name → all matching
-   *  Function/Method/Constructor defs. Backs the
-   *  `findExportedDefByName` fallback scan. */
+   *  module-scope-owned Function/Method/Constructor defs. Backs the
+   *  `findExportedDefByName` fallback scan. Class methods and nested
+   *  functions are NOT eligible here — they are not import-visible
+   *  callables. */
   readonly callablesBySimpleName: ReadonlyMap<string, readonly SymbolDefinition[]>;
 
   /** Module scope by file path — used by cross-file return-type
@@ -64,38 +72,68 @@ export function buildWorkspaceResolutionIndex(
       if (cd !== undefined) classScopeByDefId.set(cd.nodeId, scope);
     }
 
-    // per-file by-simple-name index + callable fallback
+    // Module-export pass — populates the file-level export lookup
+    // and the workspace callable fallback with ONLY module-level
+    // defs. "Module-level" here means: defs owned by the module scope
+    // OR by any scope whose parent is the module scope (top-level
+    // class and function declarations live in their own Class /
+    // Function scopes whose parent is the module scope — not in
+    // moduleScope.ownedDefs).
+    //
+    // Methods (Function/Method defs whose owning scope's parent is a
+    // Class scope) and nested-function defs (parent is another
+    // Function scope) are intentionally excluded — they are not
+    // import-visible as `from mod import X` / `mod.X()` targets.
+    // Without this filter, a class method can win the file-level
+    // export lookup by parse order and produce silently wrong CALLS
+    // edges.
     let fileBucket = defsByFileAndName.get(parsed.filePath);
     if (fileBucket === undefined) {
       fileBucket = new Map();
       defsByFileAndName.set(parsed.filePath, fileBucket);
     }
+    if (moduleScope !== undefined) {
+      const addExport = (def: SymbolDefinition): void => {
+        const simple = simpleQualifiedName(def);
+        if (simple === undefined) return;
+        // First-seen wins to match `findExportedDef` semantics.
+        if (!fileBucket!.has(simple)) fileBucket!.set(simple, def);
+        if (def.type === 'Function' || def.type === 'Method' || def.type === 'Constructor') {
+          let bucket = callablesBySimpleName.get(simple);
+          if (bucket === undefined) {
+            bucket = [];
+            callablesBySimpleName.set(simple, bucket);
+          }
+          bucket.push(def);
+        }
+      };
+      // Defs directly owned by the module scope (rare — usually
+      // module-level variable assignments and re-exports).
+      for (const def of moduleScope.ownedDefs) addExport(def);
+      // Defs whose containing scope is a direct child of the module
+      // scope — top-level class declarations and top-level function
+      // declarations each get their own scope with parent = module.
+      for (const scope of parsed.scopes) {
+        if (scope.parent !== moduleScope.id) continue;
+        for (const def of scope.ownedDefs) addExport(def);
+      }
+    }
+
+    // Member-by-owner pass — keyed on `ownerId`, so it must iterate
+    // `parsed.localDefs` (class-owned defs live in nested class scopes,
+    // not the module scope). Requires populateOwners to have run first.
     for (const def of parsed.localDefs) {
+      const ownerId = (def as { ownerId?: string }).ownerId;
+      if (ownerId === undefined) continue;
       const simple = simpleQualifiedName(def);
       if (simple === undefined) continue;
-      // First-seen wins to match `findExportedDef` semantics.
-      if (!fileBucket.has(simple)) fileBucket.set(simple, def);
-
-      if (def.type === 'Function' || def.type === 'Method' || def.type === 'Constructor') {
-        let bucket = callablesBySimpleName.get(simple);
-        if (bucket === undefined) {
-          bucket = [];
-          callablesBySimpleName.set(simple, bucket);
-        }
-        bucket.push(def);
-      }
-
-      // member-by-owner: requires populateOwners to have run first.
-      const ownerId = (def as { ownerId?: string }).ownerId;
-      if (ownerId !== undefined) {
-        let memberBucket = memberByOwner.get(ownerId);
-        if (memberBucket === undefined) {
-          memberBucket = new Map();
-          memberByOwner.set(ownerId, memberBucket);
-        }
-        // First-seen wins to match `findOwnedMember` semantics.
-        if (!memberBucket.has(simple)) memberBucket.set(simple, def);
+      let memberBucket = memberByOwner.get(ownerId);
+      if (memberBucket === undefined) {
+        memberBucket = new Map();
+        memberByOwner.set(ownerId, memberBucket);
       }
+      // First-seen wins to match `findOwnedMember` semantics.
+      if (!memberBucket.has(simple)) memberBucket.set(simple, def);
     }
   }
 
diff --git a/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/app.py b/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/app.py
new file mode 100644
index 000000000..0d43adc64
--- /dev/null
+++ b/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/app.py
@@ -0,0 +1,11 @@
+import mod
+from mod import User
+
+
+def use_module_export() -> None:
+    mod.save(1)
+
+
+def use_method() -> None:
+    u = User()
+    u.save()
diff --git a/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/mod.py b/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/mod.py
new file mode 100644
index 000000000..68a296ba1
--- /dev/null
+++ b/gitnexus/test/fixtures/lang-resolution/python-module-export-vs-method-collision/mod.py
@@ -0,0 +1,21 @@
+"""
+Class method declared BEFORE a top-level function with the same
+simple name. Order matters for the workspace-resolution-index bug:
+without the module-scope filter, `User.save` enters
+`defsByFileAndName[mod.py]['save']` first and wins first-seen. Then
+`mod.save(x)` silently binds to `User.save` instead of the free
+function — the exact wrong-edge symptom Codex flagged.
+
+Assertions in the paired test pin the intended behavior: `mod.save`
+resolves to the top-level Function, `u.save()` resolves to User.save
+Method.
+"""
+
+
+class User:
+    def save(self) -> bool:
+        return True
+
+
+def save(x: int) -> bool:
+    return x > 0
diff --git a/gitnexus/test/integration/resolvers/python.test.ts b/gitnexus/test/integration/resolvers/python.test.ts
index ce8fddc91..c95793fc8 100644
--- a/gitnexus/test/integration/resolvers/python.test.ts
+++ b/gitnexus/test/integration/resolvers/python.test.ts
@@ -2277,3 +2277,57 @@ describe('Python same-file method-name collision across classes', () => {
     expect(targets[1]).toContain('User.save');
   });
 });
+
+// ---------------------------------------------------------------------------
+// Module export vs class method collision within the same file
+// Codex review on PR #980 flagged: buildWorkspaceResolutionIndex feeds
+// defsByFileAndName and callablesBySimpleName from parsed.localDefs (every
+// def in the file, flat). A class method declared before a top-level
+// function with the same simple name wins the file-level export lookup,
+// so `mod.save(x)` silently binds to `User.save`.
+// ---------------------------------------------------------------------------
+
+describe('Python module export vs method-name collision in same file', () => {
+  let result: PipelineResult;
+
+  beforeAll(async () => {
+    result = await runPipelineFromRepo(
+      path.join(FIXTURES, 'python-module-export-vs-method-collision'),
+      () => {},
+    );
+  }, 60000);
+
+  it('mod.save(x) resolves to the module-level Function, not User.save', () => {
+    const calls = getRelationships(result, 'CALLS');
+    const saveCalls = calls.filter((c) => c.target === 'save');
+    const fromModuleExport = saveCalls.find((c) => c.source === 'use_module_export');
+    expect(fromModuleExport).toBeDefined();
+    // Target must be the top-level Function save, not the User.save Method.
+    // Node id format: `Function:mod.py:save` vs `Method:mod.py:User.save#0`.
+    expect(fromModuleExport!.rel.targetId).toContain('Function:');
+    expect(fromModuleExport!.rel.targetId).toContain('mod.py:save');
+    expect(fromModuleExport!.rel.targetId).not.toContain('User.save');
+  });
+
+  it('u.save() resolves to User.save Method via typed receiver', () => {
+    const calls = getRelationships(result, 'CALLS');
+    const saveCalls = calls.filter((c) => c.target === 'save');
+    const fromMethod = saveCalls.find((c) => c.source === 'use_method');
+    expect(fromMethod).toBeDefined();
+    expect(fromMethod!.rel.targetId).toContain('User.save');
+  });
+
+  it('exactly two CALLS edges to save — one to the free function, one to the method', () => {
+    const calls = getRelationships(result, 'CALLS');
+    const saveCalls = calls.filter((c) => c.target === 'save');
+    expect(saveCalls).toHaveLength(2);
+    const targetIds = saveCalls.map((c) => c.rel.targetId).sort();
+    // One Function target, one Method target. Exact shape pins the fix.
+    const hasFunctionTarget = targetIds.some(
+      (id) => id.startsWith('Function:') && !id.includes('User.save'),
+    );
+    const hasMethodTarget = targetIds.some((id) => id.includes('User.save'));
+    expect(hasFunctionTarget).toBe(true);
+    expect(hasMethodTarget).toBe(true);
+  });
+});