From a76c4c70fe88b68259d37a1fdbbf216cd5e4c386 Mon Sep 17 00:00:00 2001 From: ReidenXerx Date: Fri, 7 Aug 2026 19:18:38 +0300 Subject: [PATCH] fix(scope-resolution): resolve the import map by point lookup so the seal cannot empty it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding 4, reproduced end-to-end by two lanes: the same commit and the same repo produced a DIFFERENT graph depending on `GITNEXUS_DISK_SCOPE_INDEX`. `buildDirectImportMap` built `scopeToFile` by walking `parsed.scopes`. The out-of-core seal replaces `emitParsedFiles` with a scope-STRIPPED copy — that is its documented contract, scopes are reachable only via `scopeTree.getScope` afterwards — so under the seal the map came out empty, every `directImports` lookup returned undefined, and tier-2 narrowing died repo-wide. The reporting is the worse half. The loss surfaced as `ambiguous`, which means "several candidates and the pass refused to choose". The truth was "the evidence was discarded one function earlier". A reader acting on that would go looking for better receiver typing to fix a problem that was not there. This is the SECOND consumer of `parsed.scopes` on this branch to hit the seal. The first was hoisted above it. This one is converted to the point lookup instead, which is the stronger fix: a point lookup survives the seal by contract, so there is no ordering left for a future edit to get wrong. The parity assertion that would have caught it now exists. The sealed harness in `javascript-const-references` already ran the fixture both ways, but every assertion in it pinned ONE field's readers — which is exactly how a second instance slipped in, since no assertion happened to cover a narrowed name. It now also compares the WHOLE ACCESSES edge set between the two runs, as a sorted diff so a failure names the edges that moved, with a non-empty guard so two empty sets cannot compare equal and assert nothing. Mutation-verified: forcing the map empty fails it. Co-Authored-By: Claude Opus 5 (1M context) --- .../passes/unique-name-properties.ts | 29 ++++++++++++------- .../javascript-const-references.test.ts | 24 +++++++++++++++ 2 files changed, 43 insertions(+), 10 deletions(-) diff --git a/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts b/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts index 2deac2a96..f8957aa95 100644 --- a/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts +++ b/gitnexus/src/core/ingestion/scope-resolution/passes/unique-name-properties.ts @@ -263,19 +263,28 @@ function candidatesForLanguage( * narrow every read to nothing. */ function buildDirectImportMap( - parsedFiles: readonly ParsedFile[], + indexes: ScopeResolutionIndexes, finalized: FinalizedImportView, ): ReadonlyMap> { - const scopeToFile = new Map(); - for (const parsed of parsedFiles) { - for (const scope of parsed.scopes) { - scopeToFile.set(scope.id, parsed.filePath); - } - } - + // Resolve each importing scope to its file by POINT LOOKUP on the scope tree, + // not by walking `parsed.scopes`. + // + // The out-of-core seal replaces `emitParsedFiles` with a scope-STRIPPED copy — + // `scopes: []` for every file — and that is the documented contract: after the + // seal, scopes are reachable only via `scopeTree.getScope`. Building the map + // from `parsed.scopes` therefore produced an EMPTY map under + // `GITNEXUS_DISK_SCOPE_INDEX=1`, every `directImports` lookup returned + // undefined, tier-2 narrowing died, and — worst of it — the loss was + // misreported as `ambiguous`: "several candidates, refused to choose" when the + // truth was "the evidence was discarded one function earlier". Same commit, + // same repo, two different graphs depending on an env var. + // + // This is the SECOND consumer of `parsed.scopes` on this branch to hit the + // seal. The first was hoisted above it; this one is converted to the point + // lookup instead, which is stronger — there is no ordering left to get wrong. const byFile = new Map>(); for (const [scopeId, edges] of finalized.imports) { - const fromFile = scopeToFile.get(scopeId); + const fromFile = indexes.scopeTree.getScope(scopeId)?.filePath; if (fromFile === undefined) continue; for (const edge of edges) { if (edge.targetFile === null || edge.targetFile === fromFile) continue; @@ -392,7 +401,7 @@ export function emitUniqueNamePropertyAccesses( const directImports = finalized === undefined ? new Map>() - : buildDirectImportMap(parsedFiles, finalized); + : buildDirectImportMap(indexes, finalized); let emitted = 0; let ambiguous = 0; diff --git a/gitnexus/test/integration/resolvers/javascript-const-references.test.ts b/gitnexus/test/integration/resolvers/javascript-const-references.test.ts index a2551dece..2548a0ef2 100644 --- a/gitnexus/test/integration/resolvers/javascript-const-references.test.ts +++ b/gitnexus/test/integration/resolvers/javascript-const-references.test.ts @@ -91,6 +91,30 @@ describe('JavaScript module-scope const references (A2)', () => { expect([...sealedReaders()].sort()).toEqual([...readersOfConst()].sort()); }); + // WHOLESALE parity, not one field's readers. + // + // The two assertions around this one each pin a single name, and that is how + // a second instance of the same defect got in: a different consumer of + // `parsed.scopes` (`buildDirectImportMap`) was also reading scope-stripped + // files under the seal, tier-2 import narrowing died repo-wide, and every + // targeted assertion here still passed because none of them covered a + // narrowed name. Comparing the whole ACCESSES set is the only shape that + // notices a loss nobody thought to name. + // + // Reported as a sorted diff rather than a bare count so a failure says WHICH + // edges moved. + it('produces an identical ACCESSES edge set to the unsealed run', () => { + const edgeSet = (r: PipelineResult): string[] => + getRelationships(r, 'ACCESSES') + .map((e) => `${e.source} -> ${e.target} (${e.rel.reason})`) + .sort(); + const unsealed = edgeSet(result); + // Guard the guard: an empty set on both sides would compare equal and + // assert nothing. + expect(unsealed.length).toBeGreaterThan(0); + expect(edgeSet(sealed)).toEqual(unsealed); + }); + it('still withholds the block-local edge', () => { // The filter must fail OPEN when scopes are unavailable, not be disabled: // the block-local exclusion is a correctness property, not an optimization.