diff --git a/gitnexus/src/core/ingestion/languages/python/import-target.ts b/gitnexus/src/core/ingestion/languages/python/import-target.ts index 3905f3301..677372966 100644 --- a/gitnexus/src/core/ingestion/languages/python/import-target.ts +++ b/gitnexus/src/core/ingestion/languages/python/import-target.ts @@ -61,37 +61,81 @@ export function resolvePythonImportTarget( const pathLike = parsedImport.targetRaw.replace(/\./g, '/'); if (pathLike.includes('/')) { const [leadingSegment] = pathLike.split('/').filter(Boolean); - if (!leadingSegment || !hasRepoCandidate(leadingSegment, ctx.allFilePaths)) { + if (!leadingSegment || !hasRepoCandidate(leadingSegment, ctx.allFilePaths, ctx.fromFile)) { return null; } } - // Multi-segment absolute resolve: try exact paths first, then suffix - // match in nested repos. Using direct `Set.has` + `endsWith` instead of - // `suffixResolve`'s shared helper because that helper requires a - // pre-built `SuffixIndex` to disambiguate ties — without one it falls - // back to an O(files) scan that silently picks the wrong file when - // the last segment collides across directories (e.g. `accounts.models` - // matching `billing/models.py` when both files exist). - return resolveAbsoluteFromFiles(pathLike, ctx.allFilePaths); + // Multi-segment absolute resolve: try exact paths first, then ancestor + // walk (mirrors the single-segment ancestor walk in + // `resolvePythonImportInternal`), then a suffix match in nested repos. + // Using direct `Set.has` + `endsWith` instead of `suffixResolve`'s shared + // helper because that helper requires a pre-built `SuffixIndex` to + // disambiguate ties — without one it falls back to an O(files) scan that + // silently picks the wrong file when the last segment collides across + // directories (e.g. `accounts.models` matching `billing/models.py` when + // both files exist). + return resolveAbsoluteFromFiles(pathLike, ctx.allFilePaths, ctx.fromFile); } /** * Resolve `package/sub/module` style paths (already dot-flattened) to a - * concrete file in `allFilePaths`. Tries the exact path first, then the - * `__init__.py` variant, then a suffix match for nested layouts. + * concrete file in `allFilePaths`. Tries the exact path first, then walks + * ancestors of `fromFile` looking for `/.py` (or + * `__init__.py`), then falls back to a suffix match for nested layouts. * Returns the original (un-normalized) path from the set. + * + * Precedence order: + * 1. Workspace-root direct hit (`.py`, `/__init__.py`). + * 2. Closest-ancestor match walking up from the importer's directory. + * 3. Suffix fallback (first match). + * + * Root wins over ancestor by construction — if both `services/sync.py` and + * `backend/services/sync.py` exist, `backend/routers/cron.py`'s + * `from services.sync import X` resolves to the root file. This mirrors + * Python's `sys.path` semantics where the project root is searched first. + * + * The ancestor walk mirrors the single-segment behavior in + * `resolvePythonImportInternal`. For `from services.sync import X` in + * `backend/routers/cron.py`, walk up: `backend/routers/services/sync.py` → + * `backend/services/sync.py` ✓. */ -function resolveAbsoluteFromFiles(pathLike: string, allFilePaths: Set): string | null { +function resolveAbsoluteFromFiles( + pathLike: string, + allFilePaths: Set, + fromFile: string, +): string | null { const directFile = `${pathLike}.py`; const directPkg = `${pathLike}/__init__.py`; + + // Direct hit at workspace root. + if (allFilePaths.has(directFile)) return directFile; + if (allFilePaths.has(directPkg)) return directPkg; + + // Ancestor walk — match the single-segment resolver's behavior at + // multi-segment granularity. Closest match wins. Stop at `i > 0` because + // `i === 0` would re-check the workspace-root candidates already covered + // by the direct check above. + const importerDir = fromFile.replace(/\\/g, '/').split('/').slice(0, -1).join('/'); + if (importerDir) { + const dirParts = importerDir.split('/').filter(Boolean); + for (let i = dirParts.length; i > 0; i--) { + const ancestor = dirParts.slice(0, i).join('/'); + const prefix = `${ancestor}/`; + const candidateFile = `${prefix}${directFile}`; + const candidatePkg = `${prefix}${directPkg}`; + if (allFilePaths.has(candidateFile)) return candidateFile; + if (allFilePaths.has(candidatePkg)) return candidatePkg; + } + } + + // Existing suffix-match fallback (preserved for monorepo/nested-repo + // layouts that don't share a directory ancestor with the importer). const suffixFile = `/${directFile}`; const suffixPkg = `/${directPkg}`; - let suffixMatch: string | null = null; for (const raw of allFilePaths) { const f = raw.replace(/\\/g, '/'); - if (f === directFile || f === directPkg) return raw; if (suffixMatch === null && (f.endsWith(suffixFile) || f.endsWith(suffixPkg))) { suffixMatch = raw; } @@ -100,21 +144,54 @@ function resolveAbsoluteFromFiles(pathLike: string, allFilePaths: Set): } /** - * Does the repo contain a module/package named `leadingSegment` at the top - * level? Used to guard against false-positive suffix matches on external - * dotted imports (e.g. `django.apps` matching a local `accounts/apps.py`). + * Does the repo contain a module/package named `leadingSegment` somewhere + * the importer can plausibly reach? * - * Checks, in order: `.py` root file, `/__init__.py` - * regular package, or any `/**.py` file (namespace package). + * Used to guard against false-positive suffix matches on external dotted + * imports (e.g. `django.apps` matching a local `accounts/apps.py`). + * + * Checks, in order: + * 1. `SEGMENT.py` root file or `SEGMENT/__init__.py` regular package. + * 2. Any `SEGMENT/...py` file at the workspace root (namespace package). + * 3. Any `/SEGMENT/...py` file (nested namespace + * package the importer could reach via an ancestor walk, e.g. + * `backend/services/sync.py` from `backend/routers/cron.py`). + * + * The nested case is bounded to the importer's own ancestors so a + * vendored copy of an external package (e.g. `vendor/django/urls.py`) + * does not gate-pass external imports like `from django.urls import path` + * issued from `app/main.py`. Files inside the vendored tree itself + * (importer under `vendor/django/...`) still resolve correctly because + * the ancestor walk includes their own parents. */ -function hasRepoCandidate(leadingSegment: string, allFilePaths: Set): boolean { +function hasRepoCandidate( + leadingSegment: string, + allFilePaths: Set, + fromFile: string, +): boolean { const prefix = `${leadingSegment}/`; const rootFile = `${leadingSegment}.py`; const initFile = `${leadingSegment}/__init__.py`; + + // Build importer-ancestor prefixes: for `backend/routers/cron.py`, + // produces `["backend/routers/services/", "backend/services/"]` for + // segment `services` (closest first, root excluded — covered above). + const importerDir = fromFile.replace(/\\/g, '/').split('/').slice(0, -1).join('/'); + const dirParts = importerDir ? importerDir.split('/').filter(Boolean) : []; + const ancestorPrefixes: string[] = []; + for (let i = dirParts.length; i > 0; i--) { + ancestorPrefixes.push(`${dirParts.slice(0, i).join('/')}/${leadingSegment}/`); + } + for (const raw of allFilePaths) { const f = raw.replace(/\\/g, '/'); if (f === rootFile || f === initFile) return true; if (f.startsWith(prefix) && f.endsWith('.py')) return true; + if (f.endsWith('.py')) { + for (const ap of ancestorPrefixes) { + if (f.startsWith(ap)) return true; + } + } } return false; } diff --git a/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/auth_utils.py b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/auth_utils.py new file mode 100644 index 000000000..c384c28c6 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/auth_utils.py @@ -0,0 +1,6 @@ +def verify_cron_secret(token): + return token == "expected" + + +def get_org_id_from_header(headers): + return headers.get("x-org-id") if headers else None diff --git a/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/routers/__init__.py b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/routers/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/routers/alerts.py b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/routers/alerts.py new file mode 100644 index 000000000..997a75a83 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/routers/alerts.py @@ -0,0 +1,2 @@ +def send_daily_alerts(): + return "sent" diff --git a/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/routers/cron.py b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/routers/cron.py new file mode 100644 index 000000000..f57dcf73c --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/routers/cron.py @@ -0,0 +1,24 @@ +from auth_utils import verify_cron_secret, get_org_id_from_header +from services.sync import _start_cron_run, _complete_cron_run +from services.alerts import _create_ops_alert +from routers.alerts import send_daily_alerts + + +def handler_a(): + if verify_cron_secret("x"): + org = get_org_id_from_header(None) + _start_cron_run("a") + _create_ops_alert("a") + send_daily_alerts() + _complete_cron_run("a") + return org + + +def handler_b(): + _start_cron_run("b") + _create_ops_alert("b") + send_daily_alerts() + + +def handler_c(): + _start_cron_run("c") diff --git a/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/services/__init__.py b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/services/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/services/alerts.py b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/services/alerts.py new file mode 100644 index 000000000..92229fc68 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/services/alerts.py @@ -0,0 +1,2 @@ +def _create_ops_alert(name): + return f"alert:{name}" diff --git a/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/services/sync.py b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/services/sync.py new file mode 100644 index 000000000..ea3f1a77f --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/python-multi-segment-ancestor-import/backend/services/sync.py @@ -0,0 +1,6 @@ +def _start_cron_run(name): + return f"started:{name}" + + +def _complete_cron_run(name): + return f"completed:{name}" diff --git a/gitnexus/test/integration/resolvers/python.test.ts b/gitnexus/test/integration/resolvers/python.test.ts index 817483849..dd0104a5c 100644 --- a/gitnexus/test/integration/resolvers/python.test.ts +++ b/gitnexus/test/integration/resolvers/python.test.ts @@ -416,6 +416,213 @@ describe('Python ancestor directory import resolution (Issue #417)', () => { }); }); +// --------------------------------------------------------------------------- +// Multi-segment ancestor walk: `from services.sync import X` style imports +// from a sibling sub-package nested under a shared root directory. +// +// Before this fix, single-segment ancestor walks worked (`from middleware +// import X` from `backend/services/auth.py` → `backend/middleware.py`) but +// multi-segment dotted imports were only resolved against the workspace +// root. In a `backend/`-prefixed repo, `from services.sync import X` from +// `backend/routers/cron.py` would silently drop because `services/sync.py` +// does not exist at the workspace root — only `backend/services/sync.py` +// does. The fix mirrors the single-segment ancestor walk for multi-segment +// paths. +// --------------------------------------------------------------------------- + +describe('Python multi-segment ancestor directory import resolution', () => { + let result: PipelineResult; + + beforeAll(async () => { + result = await runPipelineFromRepo( + path.join(FIXTURES, 'python-multi-segment-ancestor-import'), + () => {}, + ); + }, 60000); + + it('resolves from services.sync import to backend/services/sync.py via ancestor walk', () => { + const imports = getRelationships(result, 'IMPORTS'); + const syncImport = imports.find( + (i) => + i.sourceFilePath === 'backend/routers/cron.py' && + i.targetFilePath === 'backend/services/sync.py', + ); + expect(syncImport).toBeDefined(); + }); + + it('resolves from services.alerts import to backend/services/alerts.py via ancestor walk', () => { + const imports = getRelationships(result, 'IMPORTS'); + const alertsImport = imports.find( + (i) => + i.sourceFilePath === 'backend/routers/cron.py' && + i.targetFilePath === 'backend/services/alerts.py', + ); + expect(alertsImport).toBeDefined(); + }); + + it('resolves from routers.alerts import to backend/routers/alerts.py (sibling sub-package)', () => { + const imports = getRelationships(result, 'IMPORTS'); + const routerImport = imports.find( + (i) => + i.sourceFilePath === 'backend/routers/cron.py' && + i.targetFilePath === 'backend/routers/alerts.py', + ); + expect(routerImport).toBeDefined(); + }); + + it('emits CALLS edges for every multi-segment-imported callee', () => { + const calls = getRelationships(result, 'CALLS').filter( + (c) => c.sourceFilePath === 'backend/routers/cron.py', + ); + + const startCronRunCalls = calls.filter((c) => c.target === '_start_cron_run'); + expect(startCronRunCalls.length).toBe(3); + expect(startCronRunCalls.every((c) => c.targetFilePath === 'backend/services/sync.py')).toBe( + true, + ); + + const completeCronRunCalls = calls.filter((c) => c.target === '_complete_cron_run'); + expect(completeCronRunCalls.length).toBe(1); + expect(completeCronRunCalls[0].targetFilePath).toBe('backend/services/sync.py'); + + const opsAlertCalls = calls.filter((c) => c.target === '_create_ops_alert'); + expect(opsAlertCalls.length).toBe(2); + expect(opsAlertCalls.every((c) => c.targetFilePath === 'backend/services/alerts.py')).toBe( + true, + ); + + const sendDailyCalls = calls.filter((c) => c.target === 'send_daily_alerts'); + expect(sendDailyCalls.length).toBe(2); + expect(sendDailyCalls.every((c) => c.targetFilePath === 'backend/routers/alerts.py')).toBe( + true, + ); + }); + + it('preserves single-segment ancestor walk (regression check for from auth_utils import X)', () => { + const calls = getRelationships(result, 'CALLS').filter( + (c) => c.sourceFilePath === 'backend/routers/cron.py', + ); + + const verifyCalls = calls.filter((c) => c.target === 'verify_cron_secret'); + expect(verifyCalls.length).toBe(1); + expect(verifyCalls[0].targetFilePath).toBe('backend/auth_utils.py'); + + const orgCalls = calls.filter((c) => c.target === 'get_org_id_from_header'); + expect(orgCalls.length).toBe(1); + expect(orgCalls[0].targetFilePath).toBe('backend/auth_utils.py'); + }); +}); + +// --------------------------------------------------------------------------- +// Negative case for `hasRepoCandidate` widening: a vendored copy of an +// external package (e.g. `vendor/django/urls.py`) must not cause an external +// import like `from django.urls import path` issued from an unrelated file +// (`app/main.py`) to be treated as a local candidate. The ancestor-bounded +// nested check rejects vendored matches that don't sit on the importer's +// own ancestor path. +// --------------------------------------------------------------------------- + +describe('Python multi-segment widening: vendored external package false-positive guard', () => { + let repoDir: string; + let result: PipelineResult; + + beforeAll(async () => { + repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-python-vendored-django-')); + writeFixtureRepo(repoDir, { + 'app/main.py': `from django.urls import path + +def boot(): + path("/") +`, + 'vendor/django/__init__.py': '', + 'vendor/django/urls.py': `def path(p): + return p +`, + }); + result = await runPipelineFromRepo(repoDir, () => {}); + }, 60000); + + afterAll(() => { + if (repoDir !== undefined) fs.rmSync(repoDir, { recursive: true, force: true }); + }); + + it('does not resolve from django.urls to vendor/django/urls.py from an unrelated importer', () => { + const imports = getRelationships(result, 'IMPORTS'); + const stray = imports.find( + (i) => i.sourceFilePath === 'app/main.py' && i.targetFilePath === 'vendor/django/urls.py', + ); + expect(stray).toBeUndefined(); + }); + + it('does not emit a CALLS edge from app/main.py:boot to vendor/django/urls.py:path', () => { + const calls = getRelationships(result, 'CALLS').filter( + (c) => c.sourceFilePath === 'app/main.py', + ); + const stray = calls.find( + (c) => c.target === 'path' && c.targetFilePath === 'vendor/django/urls.py', + ); + expect(stray).toBeUndefined(); + }); +}); + +// --------------------------------------------------------------------------- +// Workspace-root precedence: when both `services/sync.py` (root) and +// `backend/services/sync.py` (ancestor) exist, an importer at +// `backend/routers/cron.py` doing `from services.sync import X` resolves to +// the root file. Mirrors Python's `sys.path` semantics where the project +// root is searched before package-local namespaces. +// --------------------------------------------------------------------------- + +describe('Python multi-segment resolution: workspace root wins over ancestor', () => { + let repoDir: string; + let result: PipelineResult; + + beforeAll(async () => { + repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-python-root-precedence-')); + writeFixtureRepo(repoDir, { + 'services/__init__.py': '', + 'services/sync.py': `def root_marker(): + return "root" +`, + 'backend/__init__.py': '', + 'backend/services/__init__.py': '', + 'backend/services/sync.py': `def ancestor_marker(): + return "ancestor" +`, + 'backend/routers/__init__.py': '', + 'backend/routers/cron.py': `from services.sync import root_marker + +def handler(): + return root_marker() +`, + }); + result = await runPipelineFromRepo(repoDir, () => {}); + }, 60000); + + afterAll(() => { + if (repoDir !== undefined) fs.rmSync(repoDir, { recursive: true, force: true }); + }); + + it('resolves the import edge to the root services/sync.py, not backend/services/sync.py', () => { + const imports = getRelationships(result, 'IMPORTS').filter( + (i) => i.sourceFilePath === 'backend/routers/cron.py', + ); + const rootEdge = imports.find((i) => i.targetFilePath === 'services/sync.py'); + expect(rootEdge).toBeDefined(); + + const ancestorEdge = imports.find((i) => i.targetFilePath === 'backend/services/sync.py'); + expect(ancestorEdge).toBeUndefined(); + }); + + it('binds the imported name to the root file, not the ancestor copy', () => { + const calls = getRelationships(result, 'CALLS').filter( + (c) => c.sourceFilePath === 'backend/routers/cron.py' && c.target === 'root_marker', + ); + expect(calls.length).toBe(1); + expect(calls[0].targetFilePath).toBe('services/sync.py'); + }); +}); + // --------------------------------------------------------------------------- // Re-export chain: from .base import X barrel pattern via __init__.py // ---------------------------------------------------------------------------