From 841a62ae363116246374bf83e8bdc7425072dca1 Mon Sep 17 00:00:00 2001 From: ReidenXerx Date: Thu, 6 Aug 2026 01:37:06 +0300 Subject: [PATCH] fix(ingestion): qualify object-literal Property ids by their owning object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two config objects in one file that share a key name generated the same `Property::` id and COLLAPSED INTO ONE node, so two distinct settings became a single symbol. Worse, the merged name then looked workspace-unique to name inference, which happily resolved reads of it to a node representing both — a wrong edge in the pre-edit safety gate, which is precisely what the unique-name pass is bounded to avoid. `objectLiteralOwnerInfo` already existed for exactly this ("so two constructors in one file that both define `bar` stay distinct nodes") but was gated to `Method`. `Property` now opts in. `findObjectLiteralBindingInfo` returns `ownerName` only when asked. Its `Method` ids must stay byte-identical — qualifying them would rewrite every object-literal method id in every indexed repo — while object-literal KEYS, indexed only since A1/A5, have no such history to preserve. Found by a test written for the ambiguity path rather than by review: the suite reported one node where two were expected, and an edge where none should exist. Both are now pinned, along with the detection boundaries of the B2 collapse check, which was previously an untestable inline expression and is now a pure function. Co-Authored-By: Claude Opus 5 (1M context) --- gitnexus/src/core/index-freshness.ts | 35 ++++++++++++++ .../src/core/ingestion/utils/ast-helpers.ts | 19 +++++++- .../core/ingestion/workers/parse-worker.ts | 17 ++++++- gitnexus/src/core/run-analyze.ts | 21 +------- .../javascript-object-properties/ambiguous.js | 15 ++++++ .../javascript-object-properties.test.ts | 15 ++++++ .../index-freshness-graph-collapse.test.ts | 48 +++++++++++++++++++ 7 files changed, 147 insertions(+), 23 deletions(-) create mode 100644 gitnexus/test/fixtures/lang-resolution/javascript-object-properties/ambiguous.js diff --git a/gitnexus/src/core/index-freshness.ts b/gitnexus/src/core/index-freshness.ts index 8af3ad73a..f063e446c 100644 --- a/gitnexus/src/core/index-freshness.ts +++ b/gitnexus/src/core/index-freshness.ts @@ -10,6 +10,41 @@ export const INDEX_INCOMPLETE_REASONS = [ export type IndexIncompleteReason = (typeof INDEX_INCOMPLETE_REASONS)[number]; +/** + * Fraction of the pipeline's relationship count that must survive into the DB + * before the write counts as collapsed. Deliberately generous: this detects + * "most of the graph did not persist" (the reported case lost ~91%), not a + * per-edge reconciliation. + */ +export const GRAPH_WRITE_COLLAPSE_RATIO = 0.5; + +/** + * Below this many relationships the ratio is meaningless — a handful of edges + * lost to legitimate filtering would trip it — so small repos are exempt. + */ +export const GRAPH_WRITE_COLLAPSE_MIN_EDGES = 100; + +/** + * Decide whether a finished write collapsed, comparing what the pipeline + * produced against what the DB hands back. + * + * A RATIO, not equality: some relationship types do not round-trip one-for-one + * and `--pdg` writes MORE rows into the same table, so demanding equality would + * fire on healthy runs. Only a collapse is a defect. + * + * FAIL-SAFE at `expected === 0`: an implementation that offloads relationships + * out of memory may not be able to report a total, and a false "your index is + * broken" is worse than a missed one. + */ +export function detectGraphWriteCollapse( + expected: number, + persisted: number, +): { expected: number; persisted: number } | undefined { + if (expected < GRAPH_WRITE_COLLAPSE_MIN_EDGES) return undefined; + if (persisted >= expected * GRAPH_WRITE_COLLAPSE_RATIO) return undefined; + return { expected, persisted }; +} + /** Stable machine-readable reasons an index cannot be certified complete. */ export function getIndexIncompleteReasons( meta: diff --git a/gitnexus/src/core/ingestion/utils/ast-helpers.ts b/gitnexus/src/core/ingestion/utils/ast-helpers.ts index 64eab074f..9b243d044 100644 --- a/gitnexus/src/core/ingestion/utils/ast-helpers.ts +++ b/gitnexus/src/core/ingestion/utils/ast-helpers.ts @@ -1103,8 +1103,15 @@ export interface ObjectLiteralBindingInfo { * * Set by {@link findMemberAssignmentOwnerInfo} so a prototype method keys as * `Foo.bar` — without it two constructors in one file that each define - * `bar` collapse onto a single `Method::bar` id. Left undefined by - * {@link findObjectLiteralBindingInfo}, whose ids stay exactly as they were. + * `bar` collapse onto a single `Method::bar` id. + * + * {@link findObjectLiteralBindingInfo} sets it ONLY when the caller opts in + * via `includeOwnerName`. Its `Method` ids must stay exactly as they were — + * qualifying them would rewrite every object-literal method id in every + * indexed repo — but object-literal KEYS (indexed since A1/A5) genuinely + * need it: two config objects in one file sharing a key name otherwise + * collapse onto a single `Property::` id, merging two distinct + * settings into one symbol. */ ownerName?: string; } @@ -1161,6 +1168,13 @@ const BLOCK_SCOPE_BOUNDARY_TYPES = new Set([ export const findObjectLiteralBindingInfo = ( node: SyntaxNode, filePath: string, + options?: { + /** + * Also return `ownerName` so the member qualifies as `.`. + * Opt-in because turning it on for `Method` would rewrite existing ids. + */ + readonly includeOwnerName?: boolean; + }, ): ObjectLiteralBindingInfo | null => { // ── Phase A: walk up from node, count `object` ancestors, find declarator let current: SyntaxNode | null = node; @@ -1218,6 +1232,7 @@ export const findObjectLiteralBindingInfo = ( const ownerLabel = declaration?.type === 'variable_declaration' ? 'Variable' : 'Const'; return { ownerId: generateId(ownerLabel, `${filePath}:${nameNode.text}`), + ...(options?.includeOwnerName === true ? { ownerName: nameNode.text } : {}), }; }; diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index 3a80eaa71..029ffab83 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -2345,10 +2345,23 @@ const processFileGroup = ( // syntax rather than from an ancestor walk, and both are language-shaped // helpers behind the provider's own label decision — shared code here // only asks "does this Method name an owner". + // `Property` joins `Method` here because object-literal KEYS are now + // indexed (A1/A5), and a key is owned by the object that holds it exactly + // as a literal's function-valued member is. Without it, two config + // objects in one file sharing a key name (`httpConfig.timeoutMs` and + // `dbConfig.timeoutMs`) generate the same `Property::timeoutMs` id + // and COLLAPSE INTO ONE node — two distinct settings become one symbol, + // and the merged name then looks workspace-unique to name inference, + // which resolves reads of it to a node representing both. const objectLiteralOwnerInfo = - !enclosingClassId && nodeLabel === 'Method' && definitionNode + !enclosingClassId && (nodeLabel === 'Method' || nodeLabel === 'Property') && definitionNode ? (findMemberAssignmentOwnerInfo(definitionNode, file.path) ?? - findObjectLiteralBindingInfo(definitionNode, file.path)) + findObjectLiteralBindingInfo(definitionNode, file.path, { + // Only `Property` opts into the qualifier; `Method` ids must stay + // byte-identical or every object-literal method in every indexed + // repo changes id. + includeOwnerName: nodeLabel === 'Property', + })) : null; // #1978: hoisted ABOVE qualifiedName/node-id (load-bearing order) so a diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 83837201c..96c939304 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -9,6 +9,7 @@ * wrapper or server worker) is responsible for process lifecycle. */ +import { detectGraphWriteCollapse } from './index-freshness.js'; import path from 'path'; import fs from 'fs/promises'; import { randomUUID } from 'node:crypto'; @@ -502,20 +503,6 @@ export interface AnalyzeResult { // Class-neutral lead, reused for the missing-dependency degrade path (#2383 F2): // its remedy already explains that reinstalling will NOT help, so appending the // generic "install with network access" tail below would contradict it. -/** - * Fraction of the pipeline's relationship count that must survive into the DB - * before the write is treated as a collapse. Deliberately generous: this is a - * catastrophe detector for "most of the graph did not persist" (the reported - * case lost ~91%), not a reconciliation of every edge. - */ -const GRAPH_WRITE_COLLAPSE_RATIO = 0.5; - -/** - * Below this many relationships the ratio is meaningless — a handful of edges - * lost to legitimate filtering would trip it — so small repos are exempt. - */ -const GRAPH_WRITE_COLLAPSE_MIN_EDGES = 100; - const FTS_UNAVAILABLE_LEAD = 'FTS extension unavailable; skipping search-index creation.'; const FTS_UNAVAILABLE_MESSAGE = `${FTS_UNAVAILABLE_LEAD} ` + @@ -2552,11 +2539,7 @@ async function runFullAnalysisInner( // relationships out of memory may no longer be able to report a total, and // a false "your index is broken" is worse than a missed one. const expectedRelationships = pipelineResult.graph.relationshipCount; - const graphWriteCollapsed = - expectedRelationships >= GRAPH_WRITE_COLLAPSE_MIN_EDGES && - stats.edges < expectedRelationships * GRAPH_WRITE_COLLAPSE_RATIO - ? { expected: expectedRelationships, persisted: stats.edges } - : undefined; + const graphWriteCollapsed = detectGraphWriteCollapse(expectedRelationships, stats.edges); if (graphWriteCollapsed) { log( `Warning: graph write incomplete — the pipeline produced ${expectedRelationships} ` + diff --git a/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/ambiguous.js b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/ambiguous.js new file mode 100644 index 000000000..c63a0f414 --- /dev/null +++ b/gitnexus/test/fixtures/lang-resolution/javascript-object-properties/ambiguous.js @@ -0,0 +1,15 @@ +// Two DIFFERENT config objects that share a key name. A read of that name +// through an untyped receiver could mean either one, so the unique-name pass +// must emit nothing rather than pick — the safety property that keeps name +// inference from over-connecting on generic keys (id, name, data). +export const httpConfig = { + sharedTimeoutMs: 1000, +}; + +export const dbConfig = { + sharedTimeoutMs: 2000, +}; + +export function readsAmbiguous(cfg) { + return cfg.sharedTimeoutMs; +} diff --git a/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts b/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts index 23be3652a..e2feec99d 100644 --- a/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts +++ b/gitnexus/test/integration/resolvers/javascript-object-properties.test.ts @@ -93,6 +93,21 @@ describe('JavaScript plain-object property access (A1/A5)', () => { expect(readersOf('exitMinAtrMult')).toContain('applyRules'); }); + // The safety property. Name inference is only defensible because it refuses + // to choose between candidates: two objects sharing a key name means a read + // through an untyped receiver could mean either, and a wrong edge in the + // pre-edit safety gate is worse than a missing one. Without this, the pass + // would silently link generic keys (id, name, data) across unrelated objects. + it('emits NOTHING when two objects share the key name', () => { + expect(readersOf('sharedTimeoutMs')).toEqual([]); + }); + + it('still indexes both ambiguous keys as nodes — only the EDGE is withheld', () => { + // The symbols must remain findable; it is the inference that is unsafe, + // not the definitions. + expect(propertyNames().filter((n) => n === 'sharedTimeoutMs')).toHaveLength(2); + }); + it('marks a name-inferred edge at reduced confidence, not as precise', () => { const inferred = getRelationships(result, 'ACCESSES').filter( (e) => e.target === 'exitMinAtrMult' && (e.rel.reason ?? '').includes('unique-name'), diff --git a/gitnexus/test/unit/index-freshness-graph-collapse.test.ts b/gitnexus/test/unit/index-freshness-graph-collapse.test.ts index cdaaecb9f..bcff4d732 100644 --- a/gitnexus/test/unit/index-freshness-graph-collapse.test.ts +++ b/gitnexus/test/unit/index-freshness-graph-collapse.test.ts @@ -15,10 +15,58 @@ */ import { describe, it, expect } from 'vitest'; import { + detectGraphWriteCollapse, getIndexIncompleteReasons, + GRAPH_WRITE_COLLAPSE_MIN_EDGES, + GRAPH_WRITE_COLLAPSE_RATIO, INDEX_INCOMPLETE_REASONS, } from '../../src/core/index-freshness.js'; +describe('detectGraphWriteCollapse (B2 detection)', () => { + it('flags the reported field failure (23009 built, 2170 persisted)', () => { + expect(detectGraphWriteCollapse(23009, 2170)).toEqual({ expected: 23009, persisted: 2170 }); + }); + + it('flags a missing relation table, which reads back as zero persisted', () => { + expect(detectGraphWriteCollapse(23009, 0)).toEqual({ expected: 23009, persisted: 0 }); + }); + + it('stays silent on a healthy write', () => { + expect(detectGraphWriteCollapse(23009, 23009)).toBeUndefined(); + }); + + it('stays silent when MORE rows persist than the call graph built (--pdg)', () => { + // PDG layers write into the same table, so persisted > expected is normal. + expect(detectGraphWriteCollapse(1000, 4000)).toBeUndefined(); + }); + + it('is fail-safe when the expected count is unavailable', () => { + // An implementation that offloads relationships out of memory may report 0; + // a false "your index is broken" is worse than a missed one. + expect(detectGraphWriteCollapse(0, 0)).toBeUndefined(); + expect(detectGraphWriteCollapse(0, 5000)).toBeUndefined(); + }); + + it('exempts small repos where the ratio is meaningless', () => { + const justUnder = GRAPH_WRITE_COLLAPSE_MIN_EDGES - 1; + expect(detectGraphWriteCollapse(justUnder, 0)).toBeUndefined(); + }); + + it('applies exactly at the minimum-edge boundary', () => { + expect(detectGraphWriteCollapse(GRAPH_WRITE_COLLAPSE_MIN_EDGES, 0)).toEqual({ + expected: GRAPH_WRITE_COLLAPSE_MIN_EDGES, + persisted: 0, + }); + }); + + it('treats the ratio as inclusive — exactly at threshold is not a collapse', () => { + const expected = 1000; + const atThreshold = expected * GRAPH_WRITE_COLLAPSE_RATIO; + expect(detectGraphWriteCollapse(expected, atThreshold)).toBeUndefined(); + expect(detectGraphWriteCollapse(expected, atThreshold - 1)).toBeDefined(); + }); +}); + describe('graph-write-collapsed incomplete reason (B2)', () => { it('is part of the stable reason vocabulary', () => { expect(INDEX_INCOMPLETE_REASONS).toContain('graph-write-collapsed');