From 8eebac29575f114587631d3aae3e944abf06a049 Mon Sep 17 00:00:00 2001 From: jelsco <58397194+jelsco@users.noreply.github.com> Date: Sat, 2 May 2026 04:36:28 -0600 Subject: [PATCH] fix(python): make multi-segment suffix fallback deterministic (#1253) --- .../languages/python/import-target.ts | 38 ++- .../test/integration/resolvers/helpers.ts | 10 + .../test/integration/resolvers/python.test.ts | 227 +++++++++++++++++- 3 files changed, 266 insertions(+), 9 deletions(-) diff --git a/gitnexus/src/core/ingestion/languages/python/import-target.ts b/gitnexus/src/core/ingestion/languages/python/import-target.ts index 677372966..3a75868d9 100644 --- a/gitnexus/src/core/ingestion/languages/python/import-target.ts +++ b/gitnexus/src/core/ingestion/languages/python/import-target.ts @@ -88,7 +88,8 @@ export function resolvePythonImportTarget( * 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). + * 3. Suffix fallback (deterministic: fewest path segments, then + * lexicographic on the normalized path). * * Root wins over ancestor by construction — if both `services/sync.py` and * `backend/services/sync.py` exist, `backend/routers/cron.py`'s @@ -129,18 +130,39 @@ function resolveAbsoluteFromFiles( } } - // Existing suffix-match fallback (preserved for monorepo/nested-repo - // layouts that don't share a directory ancestor with the importer). + // Suffix-match fallback (preserved for monorepo/nested-repo layouts + // that don't share a directory ancestor with the importer). + // + // Tie-break order when multiple files match the same suffix: + // 1. Fewest path segments (shorter, more canonical paths win — `lib/x.py` + // beats `tooling/extras/x.py`). + // 2. Lexicographic order over the normalized path (final stable + // tiebreak independent of file-set insertion order). + // + // Without an explicit tie-break the previous implementation returned + // the first match in `Set` iteration order, which depended on file + // ingestion order and produced non-deterministic edges across runs in + // multi-directory collision repos. const suffixFile = `/${directFile}`; const suffixPkg = `/${directPkg}`; - let suffixMatch: string | null = null; + const matches: { raw: string; norm: string }[] = []; for (const raw of allFilePaths) { - const f = raw.replace(/\\/g, '/'); - if (suffixMatch === null && (f.endsWith(suffixFile) || f.endsWith(suffixPkg))) { - suffixMatch = raw; + const norm = raw.replace(/\\/g, '/'); + if (norm.endsWith(suffixFile) || norm.endsWith(suffixPkg)) { + matches.push({ raw, norm }); } } - return suffixMatch; + if (matches.length === 0) return null; + if (matches.length === 1) return matches[0].raw; + matches.sort((a, b) => { + const aDepth = a.norm.split('/').length; + const bDepth = b.norm.split('/').length; + if (aDepth !== bDepth) return aDepth - bDepth; + if (a.norm < b.norm) return -1; + if (a.norm > b.norm) return 1; + return 0; + }); + return matches[0].raw; } /** diff --git a/gitnexus/test/integration/resolvers/helpers.ts b/gitnexus/test/integration/resolvers/helpers.ts index 710fcddeb..427fd7e5f 100644 --- a/gitnexus/test/integration/resolvers/helpers.ts +++ b/gitnexus/test/integration/resolvers/helpers.ts @@ -12,6 +12,16 @@ const LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES: Readonly Models/User.cs through the scope-resolution path', ]), + python: new Set([ + // Suffix-fallback lex tiebreak depends on the registry-primary + // resolver's deterministic sort. The legacy resolver returns the + // first match in `Set` iteration order, which is insertion-order + // dependent and not aligned with this guarantee. Backporting the + // sort to legacy is out of scope. + 'picks the lexicographically smaller path on equal-depth ties', + 'binds the call to alpha/services/sync.py, not omega', + 'lex tiebreak still picks alpha/services/sync.py with reversed file-write order', + ]), }; type ResolverParityEnv = Readonly>; diff --git a/gitnexus/test/integration/resolvers/python.test.ts b/gitnexus/test/integration/resolvers/python.test.ts index dd0104a5c..450ba46fc 100644 --- a/gitnexus/test/integration/resolvers/python.test.ts +++ b/gitnexus/test/integration/resolvers/python.test.ts @@ -1,13 +1,14 @@ /** * Python: relative imports + class inheritance + ambiguous module disambiguation */ -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { describe, expect, beforeAll, afterAll } from 'vitest'; import path from 'path'; import fs from 'node:fs'; import os from 'node:os'; import { FIXTURES, CROSS_FILE_FIXTURES, + createResolverParityIt, getRelationships, getNodesByLabel, getNodesByLabelFull, @@ -16,6 +17,11 @@ import { type PipelineResult, } from './helpers.js'; +// Mirrors `csharp.test.ts`: skips tests in `LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.python` +// when the legacy-resolver parity sweep runs (`REGISTRY_PRIMARY_PYTHON=0`). For the +// default registry-primary CI run this is a transparent passthrough to vitest's `it`. +const it = createResolverParityIt('python'); + function writeFixtureRepo(root: string, files: Record): void { for (const [relPath, content] of Object.entries(files)) { const fullPath = path.join(root, relPath); @@ -623,6 +629,225 @@ def handler(): }); }); +// --------------------------------------------------------------------------- +// Suffix-fallback determinism: when both root + ancestor walk miss but the +// suffix scan finds multiple candidates in unrelated trees, the resolver +// must pick the same file regardless of file-set insertion order. The +// previous implementation returned the first match in `Set` iteration +// order, which depended on file ingestion order and produced flapping +// edges across runs in multi-directory collision repos. +// +// Tie-break order: fewest path segments, then lexicographic. +// --------------------------------------------------------------------------- + +describe('Python multi-segment resolution: suffix fallback determinism', () => { + let repoDir: string; + let result: PipelineResult; + + beforeAll(async () => { + repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-python-suffix-determinism-')); + writeFixtureRepo(repoDir, { + // Importer's package. The `app/services/marker.py` file makes the + // `services` segment gate-pass under the ancestor-bounded + // `hasRepoCandidate` check, but `app/services/sync.py` is + // intentionally absent so the ancestor walk misses and the suffix + // fallback fires. + 'app/services/marker.py': `def _marker(): return True +`, + 'app/main.py': `from services.sync import handler + +def boot(): + return handler() +`, + // Two suffix candidates outside the importer's ancestor tree. + // `lib/services/sync.py` has 3 path segments, the alternative has + // 4 — the deterministic pick is `lib/services/sync.py`. + 'lib/services/sync.py': `def handler(): + return "lib" +`, + 'tooling/extras/services/sync.py': `def handler(): + return "tooling" +`, + }); + result = await runPipelineFromRepo(repoDir, () => {}); + }, 60000); + + afterAll(() => { + if (repoDir !== undefined) fs.rmSync(repoDir, { recursive: true, force: true }); + }); + + it('picks the shortest-path candidate (lib/services/sync.py) and only that one', () => { + const imports = getRelationships(result, 'IMPORTS').filter( + (i) => i.sourceFilePath === 'app/main.py', + ); + + const libEdge = imports.find((i) => i.targetFilePath === 'lib/services/sync.py'); + expect(libEdge).toBeDefined(); + + const toolingEdge = imports.find((i) => i.targetFilePath === 'tooling/extras/services/sync.py'); + expect(toolingEdge).toBeUndefined(); + }); + + it('binds the call to the deterministic pick, not the alternate copy', () => { + const calls = getRelationships(result, 'CALLS').filter( + (c) => c.sourceFilePath === 'app/main.py' && c.target === 'handler', + ); + expect(calls.length).toBe(1); + expect(calls[0].targetFilePath).toBe('lib/services/sync.py'); + }); +}); + +// --------------------------------------------------------------------------- +// Lexicographic tiebreak: when two suffix candidates have the same +// directory depth, the lexicographically smaller path wins. Without this, +// equal-depth collisions would still depend on file-set insertion order. +// --------------------------------------------------------------------------- + +describe('Python multi-segment resolution: suffix fallback lexicographic tiebreak', () => { + let repoDir: string; + let result: PipelineResult; + + beforeAll(async () => { + repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-python-suffix-lex-tiebreak-')); + writeFixtureRepo(repoDir, { + // Same gate-passing pattern as the determinism test — non-init + // marker file makes the `services` segment satisfy + // `hasRepoCandidate` for an importer at `app/main.py`. + 'app/services/marker.py': `def _marker(): return True +`, + 'app/main.py': `from services.sync import handler + +def boot(): + return handler() +`, + // Both candidates have depth 3, so directory-depth alone cannot + // disambiguate. Lexicographic order picks `alpha/...` over + // `omega/...` regardless of which file was ingested first. + 'alpha/services/sync.py': `def handler(): + return "alpha" +`, + 'omega/services/sync.py': `def handler(): + return "omega" +`, + }); + result = await runPipelineFromRepo(repoDir, () => {}); + }, 60000); + + afterAll(() => { + if (repoDir !== undefined) fs.rmSync(repoDir, { recursive: true, force: true }); + }); + + it('picks the lexicographically smaller path on equal-depth ties', () => { + const imports = getRelationships(result, 'IMPORTS').filter( + (i) => i.sourceFilePath === 'app/main.py', + ); + + const alphaEdge = imports.find((i) => i.targetFilePath === 'alpha/services/sync.py'); + expect(alphaEdge).toBeDefined(); + + const omegaEdge = imports.find((i) => i.targetFilePath === 'omega/services/sync.py'); + expect(omegaEdge).toBeUndefined(); + }); + + it('binds the call to alpha/services/sync.py, not omega', () => { + const calls = getRelationships(result, 'CALLS').filter( + (c) => c.sourceFilePath === 'app/main.py' && c.target === 'handler', + ); + expect(calls.length).toBe(1); + expect(calls[0].targetFilePath).toBe('alpha/services/sync.py'); + }); +}); + +// --------------------------------------------------------------------------- +// Insertion-order independence: re-runs the depth and lexicographic +// scenarios with the candidate files written in reverse order. The +// deterministic sort in `resolveAbsoluteFromFiles` should pick the same +// winner regardless. If a future refactor accidentally drops the sort +// and falls back to `Set` insertion order, these tests pin the +// regression directly. +// --------------------------------------------------------------------------- + +describe('Python multi-segment resolution: suffix fallback insertion-order independence', () => { + let depthRepoDir: string; + let lexRepoDir: string; + let depthResult: PipelineResult; + let lexResult: PipelineResult; + + beforeAll(async () => { + // Depth scenario, files written in reverse order: tooling first, lib + // second. `writeFixtureRepo` iterates in object-property insertion + // order, and the pipeline scanner's directory traversal is also + // affected by mtime/inode order on most filesystems. The expected + // winner is still `lib/services/sync.py` (depth 3 < depth 4). + depthRepoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-python-suffix-determinism-rev-')); + writeFixtureRepo(depthRepoDir, { + 'tooling/extras/services/sync.py': `def handler(): + return "tooling" +`, + 'lib/services/sync.py': `def handler(): + return "lib" +`, + 'app/services/marker.py': `def _marker(): return True +`, + 'app/main.py': `from services.sync import handler + +def boot(): + return handler() +`, + }); + depthResult = await runPipelineFromRepo(depthRepoDir, () => {}); + + // Lexicographic scenario, files written in reverse order: omega first. + // Expected winner is still `alpha/services/sync.py`. + lexRepoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-python-suffix-lex-rev-')); + writeFixtureRepo(lexRepoDir, { + 'omega/services/sync.py': `def handler(): + return "omega" +`, + 'alpha/services/sync.py': `def handler(): + return "alpha" +`, + 'app/services/marker.py': `def _marker(): return True +`, + 'app/main.py': `from services.sync import handler + +def boot(): + return handler() +`, + }); + lexResult = await runPipelineFromRepo(lexRepoDir, () => {}); + }, 120000); + + afterAll(() => { + if (depthRepoDir !== undefined) fs.rmSync(depthRepoDir, { recursive: true, force: true }); + if (lexRepoDir !== undefined) fs.rmSync(lexRepoDir, { recursive: true, force: true }); + }); + + it('depth tiebreak still picks lib/services/sync.py with reversed file-write order', () => { + const imports = getRelationships(depthResult, 'IMPORTS').filter( + (i) => i.sourceFilePath === 'app/main.py', + ); + + const libEdge = imports.find((i) => i.targetFilePath === 'lib/services/sync.py'); + expect(libEdge).toBeDefined(); + + const toolingEdge = imports.find((i) => i.targetFilePath === 'tooling/extras/services/sync.py'); + expect(toolingEdge).toBeUndefined(); + }); + + it('lex tiebreak still picks alpha/services/sync.py with reversed file-write order', () => { + const imports = getRelationships(lexResult, 'IMPORTS').filter( + (i) => i.sourceFilePath === 'app/main.py', + ); + + const alphaEdge = imports.find((i) => i.targetFilePath === 'alpha/services/sync.py'); + expect(alphaEdge).toBeDefined(); + + const omegaEdge = imports.find((i) => i.targetFilePath === 'omega/services/sync.py'); + expect(omegaEdge).toBeUndefined(); + }); +}); + // --------------------------------------------------------------------------- // Re-export chain: from .base import X barrel pattern via __init__.py // ---------------------------------------------------------------------------