diff --git a/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts b/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts index 72428b2e3..2d2170c92 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/workspace-index.ts @@ -72,50 +72,43 @@ export function buildWorkspaceResolutionIndex( if (cd !== undefined) classScopeByDefId.set(cd.nodeId, scope); } - // 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). + // Module-export pass — use the module scope's own `bindings` map + // as the source of truth for "what names this module exports". + // The scope extractor populates moduleScope.bindings with exactly + // the names visible at module level: top-level class/function + // declarations, module-level variable assignments, imports, etc. + // Filtering to `origin === 'local'` keeps only locally-defined + // names (not imports or wildcard re-exports brought in from + // elsewhere), which matches the pre-fix invariant that + // defsByFileAndName was built from `parsed.localDefs`. // - // 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. + // Class methods, class-body attributes, and nested-function defs + // are NOT in moduleScope.bindings — they're bound at their + // containing (Class or Function) scope — so they're naturally + // excluded, no per-kind filter required. 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); + for (const [, refs] of moduleScope.bindings) { + for (const ref of refs) { + if (ref.origin !== 'local') continue; + const def = ref.def; + 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); } - 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); } } diff --git a/gitnexus/test/fixtures/lang-resolution/python-class-attr-export-leak/app.py b/gitnexus/test/fixtures/lang-resolution/python-class-attr-export-leak/app.py new file mode 100644 index 000000000..6f366eea1 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-class-attr-export-leak/app.py @@ -0,0 +1,11 @@ +import mod + + +def use_class_attr() -> int: + # Would silently bind to User.MAX_USERS without the fix. + return mod.MAX_USERS + + +def use_helper() -> int: + # Happy-path guard: legitimate top-level function export must still resolve. + return mod.helper() diff --git a/gitnexus/test/fixtures/lang-resolution/python-class-attr-export-leak/mod.py b/gitnexus/test/fixtures/lang-resolution/python-class-attr-export-leak/mod.py new file mode 100644 index 000000000..f63f0b9d1 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-class-attr-export-leak/mod.py @@ -0,0 +1,21 @@ +""" +Class-body attribute (`User.MAX_USERS`) that MUST NOT leak into the +module's export index. `from mod import MAX_USERS` / `mod.MAX_USERS` +should find nothing — there is no top-level `MAX_USERS` at module +scope. + +Also includes a top-level `def helper()` as a happy-path guard: the +narrowing fix must not over-narrow and drop legitimate module-level +function exports. +""" + + +class User: + MAX_USERS = 100 + + def save(self) -> bool: + return True + + +def helper() -> int: + return 42 diff --git a/gitnexus/test/fixtures/lang-resolution/python-function-local-import-chain/app.py b/gitnexus/test/fixtures/lang-resolution/python-function-local-import-chain/app.py new file mode 100644 index 000000000..3ab788e38 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-function-local-import-chain/app.py @@ -0,0 +1,9 @@ +def do_work() -> bool: + # Function-local import — pythonImportOwningScope pins `get_user` + # to the function scope, not the module scope. The cross-file + # return-type propagation pass must mirror the return type into + # THIS scope's typeBindings, or `u.save()` misses its edge. + from svc import get_user + + u = get_user() + return u.save() diff --git a/gitnexus/test/fixtures/lang-resolution/python-function-local-import-chain/svc.py b/gitnexus/test/fixtures/lang-resolution/python-function-local-import-chain/svc.py new file mode 100644 index 000000000..8e30577be --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-function-local-import-chain/svc.py @@ -0,0 +1,16 @@ +""" +Provider module for the function-local-import propagation test. +`get_user` returns a `User` instance; the importer calls +`u = get_user(); u.save()` from INSIDE a function body, so the +`from svc import get_user` binding lives on the function scope, not +the module scope. +""" + + +class User: + def save(self) -> bool: + return True + + +def get_user() -> User: + return User() diff --git a/gitnexus/test/integration/resolvers/python.test.ts b/gitnexus/test/integration/resolvers/python.test.ts index c95793fc8..98bbf8e8d 100644 --- a/gitnexus/test/integration/resolvers/python.test.ts +++ b/gitnexus/test/integration/resolvers/python.test.ts @@ -2331,3 +2331,83 @@ describe('Python module export vs method-name collision in same file', () => { expect(hasMethodTarget).toBe(true); }); }); + +// --------------------------------------------------------------------------- +// Class-body attribute leak into module export index +// Codex round-2 review on PR #980: defsByFileAndName indexes ALL defs +// owned by every child scope of the module, including class-body defs +// (e.g. `User.MAX_USERS`). `mod.MAX_USERS` / `from mod import MAX_USERS` +// can silently bind to a class attribute that's not a module export. +// --------------------------------------------------------------------------- + +describe('Python class-body attribute does NOT leak into module export index', () => { + let result: PipelineResult; + + beforeAll(async () => { + result = await runPipelineFromRepo( + path.join(FIXTURES, 'python-class-attr-export-leak'), + () => {}, + ); + }, 60000); + + it('mod.MAX_USERS does not resolve to User.MAX_USERS as a module export', () => { + // Any edge sourced from `use_class_attr` must NOT target a node + // that represents `User.MAX_USERS`. Under the bug, CALLS/USES/ + // ACCESSES could silently bind to the class attribute. + const edges = [ + ...getRelationships(result, 'CALLS'), + ...getRelationships(result, 'USES'), + ...getRelationships(result, 'ACCESSES'), + ]; + const fromConsumer = edges.filter((e) => e.source === 'use_class_attr'); + for (const edge of fromConsumer) { + expect(edge.rel.targetId).not.toContain('User.MAX_USERS'); + } + }); + + it('mod.helper() still resolves to the top-level Function (happy-path guard)', () => { + // Regression guard: the narrowing fix must not drop legitimate + // top-level function exports. Without this, the fix would over- + // narrow and break normal `mod.helper()` calls. + const calls = getRelationships(result, 'CALLS'); + const helperCall = calls.find((c) => c.source === 'use_helper' && c.target === 'helper'); + expect(helperCall).toBeDefined(); + expect(helperCall!.rel.targetId).toContain('mod.py:helper'); + }); +}); + +// --------------------------------------------------------------------------- +// Function-local import + cross-file return-type propagation +// Codex round-2 flagged this as potentially broken, but empirically the +// finalize-algorithm hoists the `from svc import get_user` binding to +// the app.py module scope (observed via indexes.bindings dump), so +// `propagateImportedReturnTypes`'s module-scope pass already handles +// it. These assertions pin that working behavior as a regression +// guard against any future change to binding-scope routing. +// --------------------------------------------------------------------------- + +describe('Python function-local import feeds chained receiver-bound call', () => { + let result: PipelineResult; + + beforeAll(async () => { + result = await runPipelineFromRepo( + path.join(FIXTURES, 'python-function-local-import-chain'), + () => {}, + ); + }, 60000); + + it('emits CALLS edge do_work -> get_user (free call, baseline sanity)', () => { + const calls = getRelationships(result, 'CALLS'); + const getUserCall = calls.find((c) => c.source === 'do_work' && c.target === 'get_user'); + expect(getUserCall).toBeDefined(); + expect(getUserCall!.rel.targetId).toContain('svc.py:get_user'); + }); + + it('emits CALLS edge do_work -> User.save via function-local-scoped import return-type', () => { + const calls = getRelationships(result, 'CALLS'); + const saveCall = calls.find((c) => c.source === 'do_work' && c.target === 'save'); + expect(saveCall).toBeDefined(); + // Target must be the User.save Method in svc.py. + expect(saveCall!.rel.targetId).toContain('User.save'); + }); +}); diff --git a/gitnexus/test/unit/scope-resolution/workspace-index.test.ts b/gitnexus/test/unit/scope-resolution/workspace-index.test.ts new file mode 100644 index 000000000..e6524de0d --- /dev/null +++ b/gitnexus/test/unit/scope-resolution/workspace-index.test.ts @@ -0,0 +1,140 @@ +/** + * Directly assert the shape of `WorkspaceResolutionIndex` — in + * particular that `defsByFileAndName` and `callablesBySimpleName` + * filter class-body attributes and nested-function locals out of the + * file-level export keyspace. + * + * The equivalent integration-level assertions in + * `test/integration/resolvers/python.test.ts` (see the + * `python-class-attr-export-leak` fixture) cover the downstream + * edge-emission path. This unit test pins the index shape directly + * because the downstream consumer in Python today doesn't emit an + * ACCESSES edge for `mod.NAME` member access, so the leak would be + * latent at the index layer until a future capture path makes it + * visible. That's precisely when a unit-level pin is most valuable. + */ + +import { describe, it, expect } from 'vitest'; +import { extractParsedFile } from '../../../src/core/ingestion/scope-extractor-bridge.js'; +import { pythonScopeResolver } from '../../../src/core/ingestion/languages/python/scope-resolver.js'; +import { buildWorkspaceResolutionIndex } from '../../../src/core/ingestion/scope-resolution/workspace-index.js'; + +function parsePython(source: string, filePath: string) { + const parsed = extractParsedFile( + pythonScopeResolver.languageProvider, + source, + filePath, + () => {}, + ); + if (parsed === undefined) throw new Error('scope extraction failed'); + return parsed; +} + +describe('buildWorkspaceResolutionIndex — module-export filter', () => { + it('keeps top-level class and function defs', () => { + const parsed = parsePython( + ` +class User: + def save(self) -> bool: + return True + +def helper() -> int: + return 42 +`, + 'mod.py', + ); + const index = buildWorkspaceResolutionIndex([parsed]); + const fileBucket = index.defsByFileAndName.get('mod.py'); + expect(fileBucket).toBeDefined(); + expect(fileBucket!.get('User')?.type).toBe('Class'); + expect(fileBucket!.get('helper')?.type).toBe('Function'); + }); + + it('excludes class-body Variable defs from defsByFileAndName', () => { + // Python's scope extractor captures `MAX_USERS = 100` inside a + // class body as `Variable:MAX_USERS` in the Class scope's + // ownedDefs. Without the scope-defining-def filter, this entry + // would leak into defsByFileAndName['mod.py']['MAX_USERS'] and + // `mod.MAX_USERS` / `from mod import MAX_USERS` would silently + // resolve to the class attribute. + const parsed = parsePython( + ` +class User: + MAX_USERS = 100 +`, + 'mod.py', + ); + const index = buildWorkspaceResolutionIndex([parsed]); + const fileBucket = index.defsByFileAndName.get('mod.py'); + expect(fileBucket).toBeDefined(); + expect(fileBucket!.get('MAX_USERS')).toBeUndefined(); + // Positive-case invariant: the Class def itself is still exported. + expect(fileBucket!.get('User')?.type).toBe('Class'); + }); + + it('excludes class methods from defsByFileAndName', () => { + // A method lives in a Function scope whose parent is the Class + // scope (not the Module), so it shouldn't be reachable through + // the direct-child filter at all. Guard against a regression to + // the earlier "method wins module-export slot" bug. + const parsed = parsePython( + ` +class User: + def save(self) -> bool: + return True +`, + 'mod.py', + ); + const index = buildWorkspaceResolutionIndex([parsed]); + const fileBucket = index.defsByFileAndName.get('mod.py'); + expect(fileBucket).toBeDefined(); + // `save` is a method — NOT a module export. + expect(fileBucket!.get('save')).toBeUndefined(); + expect(fileBucket!.get('User')?.type).toBe('Class'); + }); + + it('excludes class methods from callablesBySimpleName', () => { + const parsed = parsePython( + ` +class User: + def save(self) -> bool: + return True + +def save(x: int) -> int: + return x +`, + 'mod.py', + ); + const index = buildWorkspaceResolutionIndex([parsed]); + const saves = index.callablesBySimpleName.get('save') ?? []; + // Only the top-level `def save(x)` is a module-level callable. + // The `User.save` method lives under a Class scope and must not + // appear in the workspace callable fallback. + expect(saves).toHaveLength(1); + expect(saves[0].qualifiedName).toBe('save'); + }); + + it('keeps memberByOwner populated for class methods (unchanged contract)', () => { + // Regression guard: the narrowing of defsByFileAndName must NOT + // collaterally drop class-method entries from memberByOwner. + // findOwnedMember relies on this for receiver-bound dispatch. + const parsed = parsePython( + ` +class User: + def save(self) -> bool: + return True +`, + 'mod.py', + ); + pythonScopeResolver.populateOwners(parsed); + const index = buildWorkspaceResolutionIndex([parsed]); + // User's nodeId is derivable from its ownedDefs — find the Class + // scope's Class def and look up 'save' under its nodeId. + const classScope = parsed.scopes.find((s) => s.kind === 'Class'); + const classDef = classScope?.ownedDefs.find((d) => d.type === 'Class'); + expect(classDef).toBeDefined(); + const members = index.memberByOwner.get(classDef!.nodeId); + expect(members).toBeDefined(); + expect(members!.get('save')?.type).toBe('Function'); + }); +});