diff --git a/MIGRATION.md b/MIGRATION.md index f6af6c6a7..ccc94fc35 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -17,7 +17,7 @@ and the caller supplied none of `target_uid` / `file_path` / `kind`, "message": "Found N symbols matching ''. Use target_uid, file_path, or kind to disambiguate.", "target": { "name": "" }, "direction": "upstream", - "impactedCount": 0, + "impactedCount": null, "risk": "UNKNOWN", "candidates": [ { "uid": "...", "name": "...", "kind": "Function", "filePath": "...", "line": 42, "score": 0.76 } @@ -25,6 +25,13 @@ and the caller supplied none of `target_uid` / `file_path` / `kind`, } ``` +> `impactedCount` is `null`, not `0`, on an ambiguous result (#2687): no single +> symbol was resolved, so the blast radius is *undetermined*. A numeric `0` was +> indistinguishable from a genuine "nothing depends on this", so a caller +> testing `impactedCount === 0` read a false all-clear. Read `maxImpactedCount` +> (callgraph ambiguity) or the per-candidate counts in `candidates[]` for the +> real figure. Callers written as `impactedCount || 0` are unaffected. + ### Do I need to migrate? **Probably not, but check for assumptions.** Callers that unconditionally diff --git a/eval/workflow_bench/learnings.jsonl b/eval/workflow_bench/learnings.jsonl new file mode 100644 index 000000000..7d25e3359 --- /dev/null +++ b/eval/workflow_bench/learnings.jsonl @@ -0,0 +1,2 @@ +{"skill": "gitnexus-work", "date": "2026-07-25", "task": "#2687 const-arrow Const/Function twin fix in parse-worker + MCP impact envelope", "friction": "Phase 2's Build-current/index-current procedure indexes the repo-under-test, which makes CLI-spawning suites (skip-git-cli, cli/tool-no-index-stderr) time out because repo resolution then opens the 237k-node index from that cwd; they pass at the same commit in an unindexed worktree, so the procedure manufactures false regressions in its own final verification.", "suggestion": "Phase 4 should note that CLI-spawn suites can fail solely because the worktree became an indexed repo, and prescribe the A/B check (same commit, unindexed worktree) instead of leaving the executor to conclude a regression."} +{"skill": "gitnexus-work", "date": "2026-07-25", "task": "#2687 same run", "friction": "Phase 2 requires top-level `status: up-to-date` before graph queries, but any uncommitted staged edit makes status report `stale` by design, so the gate is unsatisfiable in the stage -> detect_changes -> commit sequence Phase 3 mandates.", "suggestion": "Scope the up-to-date requirement to index.commit == HEAD + empty incompleteReasons + runnerIdentityStatus current, and state that a `stale` top-level status caused solely by uncommitted working-tree edits is expected at the detect_changes gate."} diff --git a/gitnexus/src/core/ingestion/languages/cpp/query.ts b/gitnexus/src/core/ingestion/languages/cpp/query.ts index b50463a7f..52fcc0785 100644 --- a/gitnexus/src/core/ingestion/languages/cpp/query.ts +++ b/gitnexus/src/core/ingestion/languages/cpp/query.ts @@ -98,6 +98,16 @@ const CPP_SCOPE_QUERY = ` declarator: (function_declarator declarator: (identifier) @declaration.name)) @declaration.function +;; Lambda bindings (\`auto f = [](int x){ … };\`). The \`@declaration.function\` +;; anchor sits on the INNER lambda_expression so its range aligns with +;; \`(lambda_expression) @scope.function\` above; otherwise the def is owned by +;; the enclosing scope and calls inside the lambda lose caller attribution. +;; Mirrors the TypeScript arrow patterns (#2687). +(declaration + declarator: (init_declarator + declarator: (identifier) @declaration.name + value: (lambda_expression) @declaration.function)) + ;; ─── Declarations — function definition with pointer return ───────── (function_definition declarator: (pointer_declarator diff --git a/gitnexus/src/core/ingestion/languages/go/query.ts b/gitnexus/src/core/ingestion/languages/go/query.ts index 4246ee3b4..885c19df7 100644 --- a/gitnexus/src/core/ingestion/languages/go/query.ts +++ b/gitnexus/src/core/ingestion/languages/go/query.ts @@ -35,6 +35,25 @@ const GO_SCOPE_QUERY = ` (function_declaration name: (identifier) @declaration.name) @declaration.function +;; Declarations — closure bindings (\`var f = func(){}\`, \`f := func(){}\`). +;; The \`@declaration.function\` anchor sits on the INNER func_literal so its +;; range aligns with the \`(func_literal) @scope.function\` scope above — +;; without that alignment pass2AttachDeclarations owns the def by the module +;; scope and calls inside the closure lose caller attribution. Mirrors the +;; TypeScript \`const f = () => {}\` patterns (#2687). +(var_declaration + (var_spec + name: (identifier) @declaration.name + value: (expression_list (func_literal) @declaration.function))) +(var_declaration + (var_spec_list + (var_spec + name: (identifier) @declaration.name + value: (expression_list (func_literal) @declaration.function)))) +(short_var_declaration + left: (expression_list (identifier) @declaration.name) + right: (expression_list (func_literal) @declaration.function)) + ;; Declarations — method (method_declaration name: (field_identifier) @declaration.name) @declaration.method diff --git a/gitnexus/src/core/ingestion/languages/python/query.ts b/gitnexus/src/core/ingestion/languages/python/query.ts index 530e78af3..b1892ec20 100644 --- a/gitnexus/src/core/ingestion/languages/python/query.ts +++ b/gitnexus/src/core/ingestion/languages/python/query.ts @@ -22,6 +22,15 @@ const PYTHON_SCOPE_QUERY = ` (function_definition name: (identifier) @declaration.name) @declaration.function +;; Lambda bindings (\`f = lambda x: x\`). The \`@declaration.function\` anchor +;; sits on the INNER lambda so its range aligns with \`(lambda) @scope.function\` +;; above; otherwise the def is owned by the module scope and calls inside the +;; lambda lose caller attribution. Mirrors the TypeScript arrow patterns (#2687). +(expression_statement + (assignment + left: (identifier) @declaration.name + right: (lambda) @declaration.function)) + (assignment left: (identifier) @declaration.name) @declaration.variable diff --git a/gitnexus/src/core/ingestion/tree-sitter-queries.ts b/gitnexus/src/core/ingestion/tree-sitter-queries.ts index 3261913ba..2d4334eef 100644 --- a/gitnexus/src/core/ingestion/tree-sitter-queries.ts +++ b/gitnexus/src/core/ingestion/tree-sitter-queries.ts @@ -699,6 +699,17 @@ export const PYTHON_QUERIES = ` (assignment left: (identifier) @name)) @definition.variable +; Lambda bindings: \`f = lambda x: x\` binds a CALLABLE, so it emits Function +; rather than Variable, matching what TS/JS already do for \`const f = () => {}\`. +; This aligns the LABEL only — call resolution runs off the scope-resolution +; query, which still models the binding as a value, so \`f()\` does not resolve +; here yet. Overlap with the assignment pattern above is collapsed by the +; parse-worker dedup (#2687). +(expression_statement + (assignment + left: (identifier) @name + right: (lambda))) @definition.function + ; Write access: obj.field = value (assignment left: (attribute @@ -868,6 +879,25 @@ export const GO_QUERIES = ` ; Short variable declaration: x := 5 (short_var_declaration left: (expression_list (identifier) @name)) @definition.variable +; Closure bindings: \`var f = func(){}\` / \`f := func(){}\` bind a CALLABLE, so +; they emit Function, not Variable — the same convention TS/JS already use for +; \`const f = () => {}\`. This aligns the LABEL only — call resolution runs off +; the scope-resolution query, which still models the binding as a value, so +; \`f()\` does not resolve here yet. Overlap with the value patterns above is +; collapsed by the parse-worker dedup (#2687). +(var_declaration + (var_spec + name: (identifier) @name + value: (expression_list (func_literal)))) @definition.function +(var_declaration + (var_spec_list + (var_spec + name: (identifier) @name + value: (expression_list (func_literal))))) @definition.function +(short_var_declaration + left: (expression_list (identifier) @name) + right: (expression_list (func_literal))) @definition.function + ; Struct literal construction: User{Name: "Alice"} (composite_literal type: (type_identifier) @call.name) @call @@ -1031,6 +1061,16 @@ export const CPP_QUERIES = ` declarator: (init_declarator declarator: (identifier) @name)) @definition.variable +; Lambda bindings: \`auto f = [](int x){ … };\` binds a CALLABLE, so it emits +; Function rather than Variable, matching TS/JS. This aligns the LABEL only — +; call resolution runs off the scope-resolution query, which still models the +; binding as a value, so \`f()\` does not resolve here yet. Overlap with the +; pattern above is collapsed by the parse-worker dedup (#2687). +(declaration + declarator: (init_declarator + declarator: (identifier) @name + value: (lambda_expression))) @definition.function + ; Structured bindings: auto [a, b] = makePair(); (one @name per bound identifier) (declaration declarator: (init_declarator @@ -1379,6 +1419,16 @@ export const KOTLIN_QUERIES = ` (variable_declaration (simple_identifier) @name)) @definition.property +; Lambda bindings: \`val f = { x -> x }\` binds a CALLABLE, so it emits Function +; rather than Property, matching TS/JS. This aligns the LABEL only — call +; resolution runs off the scope-resolution query, which still models the binding +; as a value, so \`f()\` does not resolve here yet. Overlap with the property +; pattern above is collapsed by the parse-worker dedup (#2687). +(property_declaration + (variable_declaration + (simple_identifier) @name) + (lambda_literal)) @definition.function + ; ── Destructuring declarations (F51, issue #1919) ──────────────────────── ; "val (a, b) = pair" binds several names through a multi_variable_declaration ; (NOT a variable_declaration), which the property rule above misses. Emit one @@ -1503,6 +1553,15 @@ export const SWIFT_QUERIES = ` ; Properties (stored and computed) (property_declaration (pattern (simple_identifier) @name)) @definition.property +; Closure bindings: \`let f = { ... }\` binds a CALLABLE, so it emits Function +; rather than Property, matching TS/JS. This aligns the LABEL only — call +; resolution runs off the scope-resolution query, which still models the binding +; as a value, so \`f()\` does not resolve here yet. Overlap with the property +; pattern above is collapsed by the parse-worker dedup (#2687). +(property_declaration + name: (pattern (simple_identifier) @name) + value: (lambda_literal)) @definition.function + ; Protocol property requirements (F75): "var title: String { get }" parses to a ; protocol_property_declaration (NOT property_declaration). Its name is a ; "name:" pattern field wrapping a value_binding_pattern + the bound @@ -1659,6 +1718,16 @@ export const DART_QUERIES = ` (initialized_identifier_list (initialized_identifier (identifier) @name)) @definition.variable) +; Closure bindings: \`var f = (x) => x;\` binds a CALLABLE, so it emits Function +; rather than Variable, matching TS/JS. This aligns the LABEL only — call +; resolution runs off the scope-resolution query, which still models the binding +; as a value, so \`f()\` does not resolve here yet. Overlap with the pattern +; above is collapsed by the parse-worker dedup (#2687). +(program + (initialized_identifier_list + (initialized_identifier + (identifier) @name + (function_expression))) @definition.function) (program (static_final_declaration_list (static_final_declaration diff --git a/gitnexus/src/core/ingestion/utils/ast-helpers.ts b/gitnexus/src/core/ingestion/utils/ast-helpers.ts index 508a0c466..0eafe74c5 100644 --- a/gitnexus/src/core/ingestion/utils/ast-helpers.ts +++ b/gitnexus/src/core/ingestion/utils/ast-helpers.ts @@ -8,6 +8,7 @@ import { templateArgumentsIdTag, } from './template-arguments.js'; import { splitQualifiedName } from './qualified-name.js'; +import { isOverloadableCallable } from './callable-labels.js'; /** Tree-sitter AST node. Re-exported for use across ingestion modules. */ export type SyntaxNode = Parser.SyntaxNode; @@ -110,24 +111,6 @@ const isConcreteTypedefCapture = (captureMap: Record): boole ); }; -export const buildConcreteTypedefDefinitionRanges = ( - matches: readonly QueryMatchLike[], -): Set => { - const ranges = new Set(); - for (const match of matches) { - const captureMap: Record = {}; - for (const capture of match.captures) { - captureMap[capture.name] = capture.node; - } - - const definitionNode = getDefinitionNodeFromCaptures(captureMap); - if (definitionNode && isConcreteTypedefCapture(captureMap)) { - ranges.add(nodeRangeKey(definitionNode)); - } - } - return ranges; -}; - export const isSuppressedConcreteTypedefDuplicate = ( captureMap: Record, concreteTypedefRanges: ReadonlySet, @@ -140,6 +123,129 @@ export const isSuppressedConcreteTypedefDuplicate = ( ); }; +/** + * Graph labels produced by a value capture (`@definition.const` / + * `@definition.static` / `@definition.variable`) — a binding that holds a value. + * + * `Property` is deliberately NOT here. It outranks these: Python matches both + * `@definition.property` (annotated) and `@definition.variable` (bare) on one + * assignment, and the property must win so a typed class attribute keeps its + * `Property` node and its owning `HAS_PROPERTY` edge. `Property` is instead + * suppressed only by a *callable* claim — see {@link buildDefinitionNameClaims}. + */ +const VALUE_DEFINITION_LABELS: ReadonlySet = new Set([ + 'Const', + 'Static', + 'Variable', +]); + +/** True when `label` is the kind of node a value capture emits. */ +export const isValueDefinitionLabel = (label: NodeLabel): boolean => + VALUE_DEFINITION_LABELS.has(label); + +/** + * One pass over a file's matches: definition-name claims by rank, plus the + * concrete-typedef ranges the loop's separate typedef guard consumes. + */ +export interface DefinitionPreScan { + /** + * Keys claimed by any non-value capture — consulted by `Const`/`Static`/ + * `Variable`. Includes `Property`, so an annotated Python attribute still + * beats the bare-assignment `Variable` capture on the same statement. + */ + readonly nonValue: ReadonlySet; + /** + * Keys claimed by a *callable* capture (`Function`/`Method`/`Constructor`) — + * consulted by `Property`. Narrower than `nonValue` on purpose: a `Property` + * must be collapsible by a callable (Kotlin `val f = { … }`, Swift + * `let f = { … }`) without being collapsible by its own claim. + */ + readonly callable: ReadonlySet; + /** Ranges of `type_definition` nodes that already emit a concrete struct/enum. */ + readonly concreteTypedefRanges: ReadonlySet; +} + +/** + * Pre-scan `matches` for the `${definitionNode.startIndex}:${name}` keys already + * claimed by a higher-ranked definition capture, so the parse-worker's duplicate + * suppression is order-independent. + * + * Rank, highest first: callable (`Function`/`Method`/`Constructor`) → `Property` + * → value (`Const`/`Static`/`Variable`). A capture is dropped only when a + * STRICTLY higher rank claimed the same declaration node and name, so no capture + * can suppress itself and no rank can suppress a peer. + * + * ## Why this exists (#2687) + * + * `const X = () => {}` matches BOTH `@definition.function` and + * `@definition.const` on the same `lexical_declaration`. Only one graph node + * should survive — the `Function`, because that is what `CALLS` edges target. + * The parse-worker's in-loop dedup intends exactly that, but only the value + * branch consults its `processedDefinitionNodes` set, so suppression worked only + * if the function match happened to be processed first. It is not: tree-sitter + * completes the const pattern at `@name`, while the function pattern must also + * match the trailing `(arrow_function)` / `(function_expression)` value, so the + * const match is yielded FIRST and the edgeless `Const:` twin escaped. + * + * Consulting this set makes the outcome independent of match order. + * + * ## Keying + * + * Keys are `startIndex:name`, never `startIndex` alone — a multi-name + * declaration (`const a = 1, b = () => {}`) shares ONE definition node, and a + * bare-index key would wrongly suppress `a`'s legitimate `Const` node. + * + * Labels come from {@link getLabelFromCaptures}, the same function the main loop + * uses, so the pre-scan and the loop can never disagree about what counts as a + * value capture — including when a provider's `labelOverride` reclassifies one. + * A match that resolves to a value label registers nothing, so a match can never + * suppress itself. + * + * Language-agnostic: keyed off capture names and labels only. + * + * Also collects the concrete-typedef ranges that suppress the analogous + * typedef/struct duplicate, so both suppression sets come from one traversal. + */ +export const buildDefinitionPreScan = ( + matches: readonly QueryMatchLike[], + provider: LanguageProvider, +): DefinitionPreScan => { + const nonValue = new Set(); + const callable = new Set(); + const concreteTypedefRanges = new Set(); + for (const match of matches) { + // ONE capture-map build per match feeds both suppression sets. These used + // to be two independent passes over `matches` (each rebuilding this object) + // on the hot per-file parse path. + const captureMap: Record = {}; + for (const capture of match.captures) { + captureMap[capture.name] = capture.node; + } + + const definitionNode = getDefinitionNodeFromCaptures(captureMap); + if (definitionNode === null) continue; + + if (isConcreteTypedefCapture(captureMap)) { + concreteTypedefRanges.add(nodeRangeKey(definitionNode)); + } + + // No `@name` capture means nothing a lower-ranked capture could collide + // with — a value or property pattern always binds a name. Checked before + // `getLabelFromCaptures` so a nameless match never pays for label + // resolution (which can reach a provider's `labelOverride`). + const nameNode = captureMap['name']; + if (nameNode === undefined) continue; + + const label = getLabelFromCaptures(captureMap, provider); + if (label === null || isValueDefinitionLabel(label)) continue; + + const key = `${definitionNode.startIndex}:${nameNode.text}`; + nonValue.add(key); + if (isOverloadableCallable(label)) callable.add(key); + } + return { nonValue, callable, concreteTypedefRanges }; +}; + /** * Node types that represent function/method definitions across languages. * Used by parent-walk in call-processor, parse-worker, and type-env to detect diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index bf0e19f07..4e35f72d2 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -78,7 +78,7 @@ try { } catch {} import { getLanguageFromFilename } from 'gitnexus-shared'; import { - buildConcreteTypedefDefinitionRanges, + buildDefinitionPreScan, FUNCTION_NODE_TYPES, findAncestorBeforeBoundary, getDefinitionNodeFromCaptures, @@ -89,6 +89,7 @@ import { genericFuncName, inferFunctionLabel, isSuppressedConcreteTypedefDuplicate, + isValueDefinitionLabel, isQualifiableScopeLabel, qualifyRustImplTargetByModScope, CLASS_CONTAINER_TYPES, @@ -1313,10 +1314,15 @@ const processFileGroup = ( ); continue; } - const concreteTypedefRanges = buildConcreteTypedefDefinitionRanges(matches); - const provider = getProvider(language); + // #2687: ONE pass over `matches` yields both suppression sets — the + // definition-name claims by rank (callable > Property > value), so the dedup + // below cannot depend on tree-sitter's match order, and the concrete-typedef + // ranges the typedef guard consumes. + const definitionPreScan = buildDefinitionPreScan(matches, provider); + const concreteTypedefRanges = definitionPreScan.concreteTypedefRanges; + // Produce the `ParsedFile` for the scope-resolution pipeline HERE, reusing // the tree we just parsed (no second tree-sitter parse). Scope-resolution // consumes these via the disk-backed parsedfile-store instead of @@ -1967,19 +1973,41 @@ const processFileGroup = ( // Dedup: variable captures (Const/Static/Variable) may overlap with higher-priority // captures (e.g. `const fn = () => {}` matches both @definition.function and @definition.const). // Multi-name declarations share the same definition node, so include the emitted name. + // + // `processedDefinitionNodes` alone only suppressed the value twin when the + // function-like match happened to be processed FIRST — and it is not. + // tree-sitter completes `@definition.const` at `@name`, while + // `@definition.function` must also match the trailing arrow / function + // expression, so the const match is yielded first and its edgeless twin + // escaped (#2687). `definitionPreScan` is the order-independent view of + // the same claim, pre-scanned over `matches` before this loop and ranked so + // a capture is dropped only by a STRICTLY higher-ranked claimant. + // + // It also replaces the old bare-`startIndex` claim, which was too coarse: + // a callable declared FIRST in a multi-name declaration + // (`const cb = () => 1, SIBLING = 2`) registered the shared definition + // node and silently dropped every later sibling on it. Both keys are now + // name-scoped, so siblings survive in either declarator order. + // + // The long-term collapse seam for this duplicate class is + // `selectNodeBearingDef` (#1876, still unwired); this pre-scan is the local + // form that keeps the hot loop single-pass. Keep them in sync if #1876 lands. if (definitionNode) { - const definitionBaseKey = `${definitionNode.startIndex}`; - if (nodeLabel === 'Const' || nodeLabel === 'Static' || nodeLabel === 'Variable') { - const definitionNameKey = `${definitionBaseKey}:${nodeName}`; + const definitionNameKey = `${definitionNode.startIndex}:${nodeName}`; + if (isValueDefinitionLabel(nodeLabel)) { if ( - processedDefinitionNodes.has(definitionBaseKey) || - processedDefinitionNodes.has(definitionNameKey) + processedDefinitionNodes.has(definitionNameKey) || + definitionPreScan.nonValue.has(definitionNameKey) ) { continue; } processedDefinitionNodes.add(definitionNameKey); - } else { - processedDefinitionNodes.add(definitionBaseKey); + } else if (nodeLabel === 'Property' && definitionPreScan.callable.has(definitionNameKey)) { + // Only a CALLABLE collapses a property. Consulting the wider + // `nonValue` set here would let a property suppress itself, and would + // let an annotated Python attribute lose to its own bare-assignment + // twin — the property must outrank `Variable`, not tie with it. + continue; } } diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index 2eb104e86..a727e5ee3 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -115,6 +115,13 @@ import { type PdgLayerStatus, } from './pdg-impact.js'; +/** + * Candidate `type`s that label enrichment newly populates (#2687). Before that, + * these surfaced as `''`, which several resolution gates read as "kind unknown". + * Anything keyed on the empty string must name these explicitly. + */ +const VALUE_CANDIDATE_TYPES: ReadonlySet = new Set(['Const', 'Variable', 'Static']); + /** Real source-file extensions (`.ts`, `.py`, …) from the resolver's list, * excluding the empty entry and the `/index.*` forms — used to decide whether * an `explain` target is a file path vs a (possibly dotted) symbol name. */ @@ -2864,10 +2871,15 @@ export class LocalBackend { * Patch the `type` field on candidates whose `labels(n)[0]` projection * came back empty — a known LadybugDB behaviour for several node types. * - * Uses one scoped UNION query across the five priority labels rather - * than per-candidate round-trips, so cost is a single DB call regardless - * of how many candidates need enrichment. No-op when every candidate - * already has a non-empty type. + * Uses one scoped UNION query across the priority labels rather than + * per-candidate round-trips, so cost is a single DB call regardless of how + * many candidates need enrichment. No-op when every candidate already has a + * non-empty type. + * + * The value labels (`Const` / `Variable` / `Static`) are included because a + * value candidate otherwise surfaces with `kind: ""` — which reads as + * "unknown kind" and, worse, makes the `kind` disambiguation hint unable to + * filter it out (#2687). * * Failures are swallowed: label enrichment is an optimisation for * downstream scoring and #480 Class/Interface BFS seeding; if it fails @@ -2892,6 +2904,12 @@ export class LocalBackend { MATCH (n:\`Method\`) WHERE n.id IN $ids RETURN n.id AS id, 'Method' AS label UNION ALL MATCH (n:\`Constructor\`) WHERE n.id IN $ids RETURN n.id AS id, 'Constructor' AS label + UNION ALL + MATCH (n:\`Const\`) WHERE n.id IN $ids RETURN n.id AS id, 'Const' AS label + UNION ALL + MATCH (n:\`Variable\`) WHERE n.id IN $ids RETURN n.id AS id, 'Variable' AS label + UNION ALL + MATCH (n:\`Static\`) WHERE n.id IN $ids RETURN n.id AS id, 'Static' AS label `, { ids }, ); @@ -3066,7 +3084,7 @@ export class LocalBackend { // types (notably Class), which left downstream consumers (impact's // Class/Interface BFS seed, the kind-priority scoring bonus) unable to // distinguish a Class target from "unknown kind". One scoped UNION - // across the five priority labels patches the type in-place without + // across the priority labels patches the type in-place without // per-candidate round-trips. await this.enrichCandidateLabels(repo, normalized); @@ -3078,7 +3096,15 @@ export class LocalBackend { // the `type === 'Constructor'` gate still correctly triggers when a // Class and its Constructor share the name. if (!hints.kind && normalized.length > 1) { - const ambiguousType = normalized.some((s) => s.type === '' || s.type === 'Constructor'); + // A value candidate (`Const`/`Variable`/`Static`) used to reach here with + // `type === ''`, which is what kept this gate true for a `class Foo` + + // `const Foo` pair and let the collapse resolve it to the Class. Label + // enrichment now fills those in (#2687), so they must be named explicitly + // or the collapse silently stops firing and confident resolutions become + // `ambiguous` across every resolver-backed tool. + const ambiguousType = normalized.some( + (s) => s.type === '' || s.type === 'Constructor' || VALUE_CANDIDATE_TYPES.has(s.type), + ); if (ambiguousType) { const candidateIds = normalized.map((s) => s.id).filter(Boolean); for (const label of ['Class', 'Interface']) { @@ -5159,10 +5185,12 @@ export class LocalBackend { target: { name: target }, direction, totalCandidates: outcome.candidates.length, - // No single resolved symbol → impactedCount stays 0 / risk UNKNOWN - // (UNKNOWN must never read as "safe to refactor"). No callgraph - // fan-out runs, so there is no per-candidate blast radius here yet. - impactedCount: 0, + // No single resolved symbol → the blast radius is UNDETERMINED, not + // zero. `null` (not 0) because no callgraph fan-out runs on this path, + // so there is not even a `maxImpactedCount` to correct a numeric zero + // against — it would be indistinguishable from a genuine "nothing + // depends on this" (#2687). + impactedCount: null, risk: 'UNKNOWN', ...(truncated && { candidatesTruncated: true }), candidates: shown.map((c) => ({ @@ -5278,12 +5306,15 @@ export class LocalBackend { // so consumers (CLI formatter) need this to report "N of M" honestly (#2129 // review F11; the CLI previously read the truncated array length). totalCandidates: outcome.candidates.length, - // `impactedCount` stays 0 and `risk` stays UNKNOWN — there is no single - // resolved symbol, and UNKNOWN must NOT read as "safe to refactor". The - // real blast radius is surfaced per-candidate plus `maxImpactedCount` / - // `maxRisk` so a real caller can never hide behind the ambiguous zero - // (#2129). - impactedCount: 0, + // `impactedCount` is `null` — UNDETERMINED, not zero — and `risk` stays + // UNKNOWN, because there is no single resolved symbol. #2129 hoisted + // `maxImpactedCount` / `maxRisk` here so a real caller could not hide + // behind the ambiguous zero, but the zero itself remained + // byte-identical to a genuine "nothing depends on this": a consumer + // testing `impactedCount === 0` still read a confident all-clear + // without ever looking at `candidates[]`. `null` cannot be mistaken for + // a measured zero, while `|| 0` consumers are unchanged (#2687). + impactedCount: null, risk: 'UNKNOWN', maxImpactedCount, maxRisk, diff --git a/gitnexus/src/storage/parse-cache.ts b/gitnexus/src/storage/parse-cache.ts index 9283060dd..8f8c7cc22 100644 --- a/gitnexus/src/storage/parse-cache.ts +++ b/gitnexus/src/storage/parse-cache.ts @@ -55,6 +55,10 @@ import type { ParseWorkerResult } from '../core/ingestion/workers/parse-worker.j // the main thread (the #1983 OOM). Because the two stores share this version, // any future change to the `ParsedFile` serialization shape MUST bump // SCHEMA_BUMP so both invalidate in lockstep. +// v22: `const X = ` emits one `Function` node +// instead of a `Function` plus an edgeless `Const` twin (#2687). Cached worker +// results are replayed verbatim — including across `--force` — so without this +// bump a warm cache keeps serving the old two-node set. // v21: Java/Kotlin Spring DI facts persist constructor, field/property, and // method injection sites plus bean-name and @Primary provider metadata. // v20: Java/Kotlin capture side-channels persist package and class-annotation @@ -66,7 +70,7 @@ import type { ParseWorkerResult } from '../core/ingestion/workers/parse-worker.j // JLS 13.1 immediate-host chains (#2555). // v18: Worker$N anonymous bodies. v17: callable-value-flow operand identity. // v16: direct callee identity. -const SCHEMA_BUMP = 21; +const SCHEMA_BUMP = 22; const GITNEXUS_PKG_VERSION = (() => { try { // package.json sits at gitnexus/package.json — two levels up from diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index e61baab49..faca22eff 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -467,8 +467,13 @@ export interface RepoMeta { * instance owner is outside the caller's enclosing class/MRO (#2563). The * incremental write set would otherwise retain those stale CALLS edges on * every unchanged C# and Kotlin file; force a full re-analyze instead. + * v15: `const X = ` no longer emits an edgeless + * `Const::X` twin beside its `Function` node (#2687). The incremental + * write set only covers changed files, so every unchanged TS/JS file would + * keep its twin and `impact`/`context` would stay ambiguous on those names; + * force a full re-analyze instead. */ -export const INCREMENTAL_SCHEMA_VERSION = 14; +export const INCREMENTAL_SCHEMA_VERSION = 15; export interface IndexedRepo { repoPath: string; diff --git a/gitnexus/test/integration/cfg/__snapshots__/pipeline-pdg.test.ts.snap b/gitnexus/test/integration/cfg/__snapshots__/pipeline-pdg.test.ts.snap index f1da88949..d8337e88c 100644 --- a/gitnexus/test/integration/cfg/__snapshots__/pipeline-pdg.test.ts.snap +++ b/gitnexus/test/integration/cfg/__snapshots__/pipeline-pdg.test.ts.snap @@ -27,15 +27,18 @@ exports[`U7 — C-family worker-mode --pdg pipeline > C#: --pdg off is byte-iden exports[`U7 — C-family worker-mode --pdg pipeline > C++: --pdg off is byte-identical (zero PDG nodes/edges, stable golden digest) 1`] = ` { "byRelType": { - "DEFINES": 6, + "CALLS": 1, + "DEFINES": 7, + "MEMBER_OF": 2, }, "byType": { + "Community": 1, "File": 1, - "Function": 6, + "Function": 7, }, - "edgeDigest": "4e8cfcfe7cbde0d0a858e2f5db8af82713fbb42527d23e6fabf704382d8088df", - "relationships": 6, - "symbols": 7, + "edgeDigest": "5b6f4ec33d30b5d95c06529dfe7bf7a279cfffa73638efc55432363365896328", + "relationships": 10, + "symbols": 9, } `; diff --git a/gitnexus/test/integration/closure-binding-labels.test.ts b/gitnexus/test/integration/closure-binding-labels.test.ts new file mode 100644 index 000000000..e37362f10 --- /dev/null +++ b/gitnexus/test/integration/closure-binding-labels.test.ts @@ -0,0 +1,189 @@ +/** + * #2687 follow-up — a closure bound to a name emits ONE `Function` node in + * every language, not `Variable` in some and `Property` in others. + * + * `const f = () => {}` already produced a `Function` in TS/JS (that is what the + * #2687 twin fix preserved), but the same construct produced a `Variable` in + * Go/Python/Dart/C++ and a `Property` in Kotlin/Swift. The graph schema states + * "Function: Functions and arrow functions", and every syntactic tagger the + * convention was checked against (tree-sitter tags, universal-ctags) labels the + * binding a function — so the callable label is the consistent one. + * + * Each language's value capture still matches the same declaration node, so + * these rely on the #2687 pre-scan collapsing the pair; a regression there + * would surface here as a twin rather than a wrong label. + * + * The label alone does not make `f()` resolve — free-call resolution runs off + * the per-language scope-resolution queries. Go, Python and C++ now also carry a + * `@declaration.function` capture anchored on the inner closure literal, so + * calls resolve there too (asserted in the second describe). Kotlin, Swift and + * Dart still lack a `@scope.function` whose range matches the closure literal — + * Kotlin deliberately scopes `lambda_literal` as a BLOCK (#1757) — and an + * unaligned declaration anchor mis-attributes callers, so those three keep the + * label fix only. + */ +import { describe, expect, it } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { runPipelineFromRepo } from '../../src/core/ingestion/pipeline.js'; +import { DIST_WORKER_URL, distWorkerExists } from '../helpers/worker-parse.js'; +import { parseFilesWithWorkers } from '../helpers/worker-parse.js'; + +const labelsFor = async (path: string, content: string, name: string): Promise => { + const { graph } = await parseFilesWithWorkers([{ path, content }]); + return graph.nodes + .filter((node) => node.properties.name === name) + .map((node) => node.label) + .sort(); +}; + +describe('closure bindings emit a single Function node in every language', () => { + it('Go: var f = func(){}', async () => { + expect( + await labelsFor( + 'src/handler.go', + 'package main\n\nvar Handler = func(x int) int { return x }\n', + 'Handler', + ), + ).toEqual(['Function']); + }); + + it('Python: f = lambda x: x', async () => { + expect(await labelsFor('src/handler.py', 'handler = lambda x: x\n', 'handler')).toEqual([ + 'Function', + ]); + }); + + it('Kotlin: val f = { x -> x }', async () => { + expect(await labelsFor('src/Handler.kt', 'val handler = { x: Int -> x }\n', 'handler')).toEqual( + ['Function'], + ); + }); + + it('Swift: let f = { ... }', async () => { + expect( + await labelsFor( + 'src/Handler.swift', + 'let handler = { (x: Int) -> Int in return x }\n', + 'handler', + ), + ).toEqual(['Function']); + }); + + it('C++: auto f = [](int x){ ... }', async () => { + expect( + await labelsFor('src/handler.cpp', 'auto handler = [](int x) { return x; };\n', 'handler'), + ).toEqual(['Function']); + }); + + it('Dart: var f = (int x) => x', async () => { + expect(await labelsFor('src/handler.dart', 'var handler = (int x) => x;\n', 'handler')).toEqual( + ['Function'], + ); + }); + + // The suppression must key on an actual closure value, never on the + // declaration keyword — otherwise ordinary constants would vanish. One `it` + // per language: each spins its own worker pool and four in a single test + // exceeds the default timeout. + + it('Go: leaves a genuine const alone', async () => { + expect( + await labelsFor('src/consts.go', 'package main\n\nconst MaxSize = 10\n', 'MaxSize'), + ).toEqual(['Const']); + }); + + it('Python: leaves a genuine assignment alone', async () => { + expect(await labelsFor('src/consts.py', 'MAX_SIZE = 10\n', 'MAX_SIZE')).toEqual(['Variable']); + }); + + it('Kotlin: leaves a genuine property alone', async () => { + expect(await labelsFor('src/Consts.kt', 'val maxSize = 10\n', 'maxSize')).toEqual(['Property']); + }); + + it('C++: leaves a genuine variable alone', async () => { + expect(await labelsFor('src/consts.cpp', 'auto maxSize = 10;\n', 'maxSize')).toEqual([ + 'Variable', + ]); + }); + + it('Python: an annotated attribute stays a Property, not a Variable', async () => { + // Regression guard. Python matches BOTH `@definition.property` (annotated) + // and `@definition.variable` (bare assignment) on the same statement at the + // same byte offset. Ranking `Property` level with the value labels made the + // winner depend on match order, which silently turned every typed attribute + // — including dataclass fields — into a file-level `Variable`. + expect(await labelsFor('src/model.py', 'class C:\n name: str = "x"\n', 'name')).toEqual([ + 'Property', + ]); + }); + + it('Python: an annotated attribute keeps its owning HAS_PROPERTY edge', async () => { + // The label regression above also detached the attribute from its class: + // the node became `Variable::name` reached by `File -DEFINES->` + // instead of `Property::C.name` reached by `Class -HAS_PROPERTY->`. + const { graph } = await parseFilesWithWorkers([ + { path: 'src/owned.py', content: 'class C:\n name: str = "x"\n' }, + ]); + + expect( + graph.relationships + .filter((rel) => rel.type === 'HAS_PROPERTY') + .map((rel) => `${rel.sourceId} -> ${rel.targetId}`), + ).toEqual(['Class:src/owned.py:C -> Property:src/owned.py:C.name']); + }); +}); + +const describeIfWorkerBuilt = distWorkerExists() ? describe : describe.skip; + +/** Call targets resolved in a one-file repo, for the closure-call assertions. */ +const callTargetsFor = async (filename: string, source: string): Promise => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-closure-calls-')); + try { + fs.writeFileSync(path.join(dir, filename), source, 'utf-8'); + const result = await runPipelineFromRepo(dir, () => {}, { + workerPoolSize: 1, + workerUrlForTest: DIST_WORKER_URL, + }); + return result.graph.relationships + .filter((rel) => rel.type === 'CALLS') + .map((rel) => rel.targetId) + .sort(); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}; + +describeIfWorkerBuilt('calls to a closure binding resolve to its Function node', () => { + // The label change alone is not enough: each language also needs a + // `@declaration.function` anchored on the inner closure literal, so the def is + // owned by the closure's own scope and free-call resolution can find it. + + it('Go: Handler(1) resolves', async () => { + const targets = await callTargetsFor( + 'main.go', + 'package main\n\nvar Handler = func(x int) int { return x }\n\nfunc Caller() int { return Handler(1) }\n', + ); + + expect(targets).toContain('Function:main.go:Handler'); + }); + + it('Python: handler(1) resolves', async () => { + const targets = await callTargetsFor( + 'app.py', + 'handler = lambda x: x\n\ndef caller():\n return handler(1)\n', + ); + + expect(targets).toContain('Function:app.py:handler'); + }); + + it('C++: handler(1) resolves', async () => { + const targets = await callTargetsFor( + 'main.cpp', + 'auto handler = [](int x) { return x; };\n\nint caller() { return handler(1); }\n', + ); + + expect(targets).toContain('Function:main.cpp:handler'); + }); +}); diff --git a/gitnexus/test/integration/const-function-twin.test.ts b/gitnexus/test/integration/const-function-twin.test.ts new file mode 100644 index 000000000..506b07ef8 --- /dev/null +++ b/gitnexus/test/integration/const-function-twin.test.ts @@ -0,0 +1,148 @@ +/** + * #2687 — `const X = ` must emit exactly ONE graph + * node: the `Function` node that carries the CALLS edges. Before the fix it also + * emitted an edgeless `Const::X` twin at the same line, which made every + * `impact`/`context` call on that name come back `status: "ambiguous"` with a + * top-level `impactedCount: 0` — indistinguishable from a real "nothing depends + * on this". + * + * Root cause: the parse-worker duplicate suppression is order-dependent. Only + * the value branch (`Const`/`Static`/`Variable`) consults + * `processedDefinitionNodes`; function-like labels merely register into it. And + * tree-sitter yields the `@definition.const` match BEFORE `@definition.function` + * for the same `lexical_declaration` (the const pattern completes at `@name`, + * the function pattern needs the trailing arrow/function-expression value), so + * the twin was emitted first and never suppressed. + * + * The over-suppression guards below matter as much as the twin assertions: a + * genuine non-callable `const`, an object-literal service (#1718), a `var` + * binding, and the non-function initializers must all keep their value nodes. + * + * Mirrors the sibling suppression case in `c-cpp-typedef-legacy-parse.test.ts`. + */ +import { describe, expect, it } from 'vitest'; +import { parseFilesWithWorkers } from '../helpers/worker-parse.js'; + +const parseNodes = async (path: string, content: string) => { + const { graph } = await parseFilesWithWorkers([{ path, content }]); + return graph.nodes; +}; + +type ParsedNode = Awaited>[number]; + +/** Sorted labels of every node carrying `name` — length doubles as the node count. */ +const labelsOf = (nodes: readonly ParsedNode[], name: string): string[] => + nodes + .filter((node) => node.properties.name === name) + .map((node) => node.label) + .sort(); + +describe('#2687 export-const function twin', () => { + it('emits one Function node for a bare const arrow', async () => { + const nodes = await parseNodes('src/bare.ts', 'const Bare = () => 1;\n'); + + expect(labelsOf(nodes, 'Bare')).toEqual(['Function']); + // The twin was `Const::Bare` at the same line — assert the id is gone. + expect(nodes.filter((node) => node.id === 'Const:src/bare.ts:Bare')).toHaveLength(0); + }); + + it('emits one Function node for an exported const arrow', async () => { + const nodes = await parseNodes('src/exported.ts', 'export const Exported = () => 2;\n'); + + expect(labelsOf(nodes, 'Exported')).toEqual(['Function']); + }); + + it('emits one Function node for a bare const function-expression', async () => { + const nodes = await parseNodes( + 'src/bare-fn.ts', + 'const BareFnExpr = function () {\n return 3;\n};\n', + ); + + expect(labelsOf(nodes, 'BareFnExpr')).toEqual(['Function']); + }); + + it('emits one Function node for an exported const function-expression', async () => { + const nodes = await parseNodes( + 'src/exported-fn.ts', + 'export const ExportedFnExpr = function () {\n return 4;\n};\n', + ); + + expect(labelsOf(nodes, 'ExportedFnExpr')).toEqual(['Function']); + }); + + it('emits one Function node for an exported component in a .tsx file', async () => { + // The reporter's shape: `export const Button = (props) => …` in a React file. + const nodes = await parseNodes( + 'src/ui/button.tsx', + 'export const Button = (props: { label: string }) => {\n return props.label;\n};\n', + ); + + expect(labelsOf(nodes, 'Button')).toEqual(['Function']); + }); + + it('emits one Function node for a let-bound arrow', async () => { + // `let` shares the `lexical_declaration` pattern, so it twinned too. + const nodes = await parseNodes('src/let.ts', 'let mutable = () => 1;\n'); + + expect(labelsOf(nodes, 'mutable')).toEqual(['Function']); + }); + + it('keeps the Const node for a non-callable const', async () => { + const nodes = await parseNodes('src/config.ts', 'export const CONFIG = { a: 1 };\n'); + + expect(labelsOf(nodes, 'CONFIG')).toEqual(['Const']); + }); + + it('keeps the Const node for an object-literal service (#1718)', async () => { + // `receiver-bound-calls.ts` Case 5 bridges `fooService.getUser()` through + // this exact `Const::fooService` node id. + const nodes = await parseNodes( + 'src/service.ts', + 'export const fooService = {\n getUser(id: string) {\n return id;\n },\n};\n', + ); + + expect(labelsOf(nodes, 'fooService')).toEqual(['Const']); + }); + + it('keeps the Const node for a non-function initializer', async () => { + const nodes = await parseNodes( + 'src/ternary.ts', + 'function A() {\n return 1;\n}\nfunction B() {\n return 2;\n}\nconst ternary = A ?? B;\n', + ); + + expect(labelsOf(nodes, 'ternary')).toEqual(['Const']); + }); + + it('keeps the Variable node for a var-bound function-expression', async () => { + // `var` has no matching `@definition.function` pattern, so nothing claims + // the name and the value node must survive untouched. + const nodes = await parseNodes('src/var.ts', 'var legacy = function () {\n return 3;\n};\n'); + + expect(labelsOf(nodes, 'legacy')).toEqual(['Variable']); + }); + + it('suppresses only the callable name in a multi-name declaration', async () => { + // Both declarators share ONE `lexical_declaration`, so a suppression keyed + // by definition-node start index alone would wrongly delete `a`. + const nodes = await parseNodes('src/multi.ts', 'const a = 1,\n b = () => {};\n'); + + expect(labelsOf(nodes, 'a')).toEqual(['Const']); + expect(labelsOf(nodes, 'b')).toEqual(['Function']); + }); + + it('keeps multi-name siblings when the callable is declared FIRST', async () => { + // Mirror of the case above. The callable's claim on the shared definition + // node used to be recorded under a bare `startIndex`, which swallowed every + // LATER sibling on that declaration — so `SIB_A`/`SIB_B` vanished entirely + // (no node, no symbol). Both claims are name-scoped now, so declarator + // order cannot decide whether a sibling exists. + const nodes = await parseNodes( + 'src/multi-first.ts', + 'export const cb = () => 1,\n SIB_A = 2,\n SIB_B = 3;\n', + ); + + expect(labelsOf(nodes, 'cb')).toEqual(['Function']); + expect(labelsOf(nodes, 'SIB_A')).toEqual(['Const']); + expect(labelsOf(nodes, 'SIB_B')).toEqual(['Const']); + }); +}); diff --git a/gitnexus/test/integration/impact-ambiguous-blast-radius.test.ts b/gitnexus/test/integration/impact-ambiguous-blast-radius.test.ts index 0e5ee9667..0172f9606 100644 --- a/gitnexus/test/integration/impact-ambiguous-blast-radius.test.ts +++ b/gitnexus/test/integration/impact-ambiguous-blast-radius.test.ts @@ -41,6 +41,20 @@ const SEED = [ `MATCH (a:Function {id:'Function:src/actions.ts:syncContent'}), (b:Function {id:'${SYNC_LOGIC_ID}'}) CREATE (a)-[:CodeRelation {type:'CALLS', confidence:0.85, reason:'direct', step:0}]->(b)`, `MATCH (a:Function {id:'Function:src/actions.ts:scheduleSync'}), (b:Function {id:'${SYNC_LOGIC_ID}'}) CREATE (a)-[:CodeRelation {type:'CALLS', confidence:0.85, reason:'direct', step:0}]->(b)`, `MATCH (a:Function {id:'Function:src/ui-helpers.ts:renderCard'}), (b:Function {id:'${UI_HELPERS_ID}'}) CREATE (a)-[:CodeRelation {type:'CALLS', confidence:0.85, reason:'direct', step:0}]->(b)`, + + // Two same-named non-callable consts — an ambiguity that survives the #2687 + // twin fix, used to pin that a value candidate reports a real `kind`. + `CREATE (k1:Const {id: 'Const:src/config-a.ts:APP_CONFIG', name: 'APP_CONFIG', filePath: 'src/config-a.ts', startLine: 1, endLine: 1, content: '', description: ''})`, + `CREATE (k2:Const {id: 'Const:src/config-b.ts:APP_CONFIG', name: 'APP_CONFIG', filePath: 'src/config-b.ts', startLine: 1, endLine: 1, content: '', description: ''})`, + + // A class and a same-named value binding in another file — the #480 + // Class/Constructor collapse must still fold onto the Class. Before the + // enrichment widening these value candidates carried `type: ''`, which is + // what kept the collapse gate open. + `CREATE (rc:Class {id: 'Class:src/registry.ts:Registry', name: 'Registry', filePath: 'src/registry.ts', startLine: 1, endLine: 9, isExported: true, content: '', description: ''})`, + `CREATE (rv:Const {id: 'Const:test/registry.test.ts:Registry', name: 'Registry', filePath: 'test/registry.test.ts', startLine: 3, endLine: 3, content: '', description: ''})`, + `CREATE (ru:Function {id: 'Function:src/boot.ts:boot', name: 'boot', filePath: 'src/boot.ts', startLine: 1, endLine: 5, isExported: true, content: '', description: ''})`, + `MATCH (a:Function {id:'Function:src/boot.ts:boot'}), (b:Class {id:'Class:src/registry.ts:Registry'}) CREATE (a)-[:CodeRelation {type:'CALLS', confidence:0.85, reason:'direct', step:0}]->(b)`, ]; withTestLbugDB( @@ -85,6 +99,76 @@ withTestLbugDB( ); }); + it('reports an undetermined impactedCount, never a numeric zero (#2687)', async () => { + const result = await backend.callTool('impact', { + target: 'classifyCard', + direction: 'upstream', + }); + + // #2129 hoisted maxImpactedCount so a real caller could not hide behind + // the ambiguous zero — but the zero itself was still byte-identical to a + // genuine "nothing depends on this". A consumer testing + // `impactedCount === 0` got a confident all-clear without ever reading + // `candidates[]`. `null` is undetermined and cannot be misread that way. + expect(result).toMatchObject({ status: 'ambiguous', impactedCount: null, risk: 'UNKNOWN' }); + expect(typeof result.impactedCount).not.toBe('number'); + + // The truthful signal is still present and still non-zero. + expect(result.maxImpactedCount).toBeGreaterThanOrEqual(2); + }); + + it('reports a real kind for an ambiguous value candidate (#2687)', async () => { + // `labels(n)[0]` comes back empty for these node types, and the label + // enrichment UNION used to cover only Class/Interface/Function/Method/ + // Constructor — so a value candidate surfaced as `kind: ""`, which reads + // as "unknown kind" and leaves the `kind` disambiguation hint unable to + // filter it out. + const result = await backend.callTool('impact', { + target: 'APP_CONFIG', + direction: 'upstream', + }); + + expect(result.status).toBe('ambiguous'); + expect(result.candidates.map((c: { kind: string }) => c.kind)).toEqual(['Const', 'Const']); + }); + + it('still collapses a Class against a same-named value binding (#480)', async () => { + // Regression guard for the enrichment widening: the collapse gate keys on + // "some candidate has an indeterminate kind". Value candidates used to + // qualify by carrying `type: ''`; now that enrichment fills them in they + // must be named explicitly, or this resolves to `ambiguous` and every + // resolver-backed tool loses a previously confident answer. + const result = await backend.callTool('impact', { + target: 'Registry', + direction: 'upstream', + }); + + expect(result.status).not.toBe('ambiguous'); + expect(result.target).toMatchObject({ + id: 'Class:src/registry.ts:Registry', + type: 'Class', + }); + expect(result.impactedCount).toBeGreaterThanOrEqual(1); + }); + + it('reports an undetermined impactedCount for an ambiguous pdg target (#2687)', async () => { + // The pdg branch has no per-candidate fan-out, so it carries no + // maxImpactedCount at all — a numeric zero here is even less correctable. + const result = await backend.callTool('impact', { + target: 'classifyCard', + direction: 'upstream', + mode: 'pdg', + }); + + expect(result).toMatchObject({ + status: 'ambiguous', + mode: 'pdg', + impactedCount: null, + risk: 'UNKNOWN', + }); + expect(typeof result.impactedCount).not.toBe('number'); + }); + it('disambiguation by uid returns the exact dropped caller (BFS unchanged)', async () => { const result = await backend.callTool('impact', { target: 'classifyCard', diff --git a/gitnexus/test/integration/local-symbol-pruner-pipeline.test.ts b/gitnexus/test/integration/local-symbol-pruner-pipeline.test.ts index 454904660..129f44889 100644 --- a/gitnexus/test/integration/local-symbol-pruner-pipeline.test.ts +++ b/gitnexus/test/integration/local-symbol-pruner-pipeline.test.ts @@ -57,9 +57,16 @@ class Client { expect(findNode(result, 'Const', 'handler')).toBeUndefined(); expect(findNode(result, 'Const', 'MODULE_CONST')).toBeDefined(); - expect(findNode(result, 'Const', 'exportedHandler')).toBeDefined(); expect(findNode(result, 'Function', 'handler')).toBeDefined(); + // #2687: `export const exportedHandler = () => …` emits ONE node — the + // Function that carries the CALLS edges — not a Function plus an edgeless + // Const twin. This makes the module-scoped arrow consistent with the + // block-scoped `const handler = () => boring` asserted above, which has + // never had a surviving Const node. + expect(findNode(result, 'Const', 'exportedHandler')).toBeUndefined(); + expect(findNode(result, 'Function', 'exportedHandler')).toBeDefined(); + const keepsResolvedClientCall = result.graph.relationships.some((rel) => { if (rel.type !== 'CALLS') return false; const source = result.graph.getNode(rel.sourceId); diff --git a/gitnexus/test/unit/call-summary-schema-version.test.ts b/gitnexus/test/unit/call-summary-schema-version.test.ts index 87256a2b2..db16663a5 100644 --- a/gitnexus/test/unit/call-summary-schema-version.test.ts +++ b/gitnexus/test/unit/call-summary-schema-version.test.ts @@ -73,8 +73,8 @@ describe('CALL_SUMMARY relation-type exclusion (U-C1)', () => { }); describe('CALL_SUMMARY incremental reuse gate (U-C5)', () => { - it('INCREMENTAL_SCHEMA_VERSION is bumped to 14 (C#/Kotlin instance-ownership free-call gate, #2563)', () => { - expect(INCREMENTAL_SCHEMA_VERSION).toBe(14); + it('INCREMENTAL_SCHEMA_VERSION is bumped to 15 (const-arrow twin removal, #2687)', () => { + expect(INCREMENTAL_SCHEMA_VERSION).toBe(15); }); it('a pre-current stamp fails the `=== INCREMENTAL_SCHEMA_VERSION` reuse gate → forces full re-analyze', () => { @@ -128,7 +128,12 @@ describe('CALL_SUMMARY incremental reuse gate (U-C5)', () => { // A pre-v14 (v13) index predates the C#/Kotlin instance-ownership gate, // so unchanged files may retain spurious same-file CALLS edges. expect(passesReuseGate(13)).toBe(false); + // A pre-v15 (v14) index predates the #2687 const-arrow twin removal — an + // edgeless `Const::X` twin survives beside its `Function` node on + // every unchanged TS/JS file, and the incremental write set never touches + // those files → must NOT reuse. + expect(passesReuseGate(14)).toBe(false); // A current-version stamp passes the gate (incremental top-up eligible). - expect(passesReuseGate(14)).toBe(true); + expect(passesReuseGate(15)).toBe(true); }); }); diff --git a/gitnexus/test/unit/calltool-dispatch.test.ts b/gitnexus/test/unit/calltool-dispatch.test.ts index ec2b7795b..1c55e11cc 100644 --- a/gitnexus/test/unit/calltool-dispatch.test.ts +++ b/gitnexus/test/unit/calltool-dispatch.test.ts @@ -1092,7 +1092,9 @@ describe('LocalBackend.callTool', () => { expect(result.status).toBe('ambiguous'); expect(result.candidates).toHaveLength(2); - expect(result.impactedCount).toBe(0); + // #2687: undetermined, NOT a numeric zero — a measured 0 is indistinguishable + // from a genuine "nothing depends on this". + expect(result.impactedCount).toBeNull(); expect(result.risk).toBe('UNKNOWN'); expect(result.target.name).toBe('login'); for (const c of result.candidates) { @@ -2516,7 +2518,9 @@ describe('LocalBackend impact mode (KTD1/KTD5/KTD12)', () => { expect(result.status).toBe('ambiguous'); expect(result.mode).toBe('pdg'); expect(result.candidates).toHaveLength(2); - expect(result.impactedCount).toBe(0); + // #2687: undetermined, NOT a numeric zero. This branch runs no per-candidate + // fan-out, so it carries no maxImpactedCount to correct a zero against. + expect(result.impactedCount).toBeNull(); expect(result.risk).toBe('UNKNOWN'); // The callgraph per-candidate probe fan-out MUST NOT run under pdg. expect(bfsSpy).not.toHaveBeenCalled(); diff --git a/gitnexus/test/unit/ingestion/non-value-definition-keys.test.ts b/gitnexus/test/unit/ingestion/non-value-definition-keys.test.ts new file mode 100644 index 000000000..ca6e35bc0 --- /dev/null +++ b/gitnexus/test/unit/ingestion/non-value-definition-keys.test.ts @@ -0,0 +1,128 @@ +/** + * #2687 — unit coverage for `buildNonValueDefinitionNameKeys`, the pre-scan that + * makes the parse-worker's duplicate suppression order-independent. + * + * The parse-worker consults these keys from its value-label branch, so what this + * pre-scan registers decides which `Const`/`Static`/`Variable` nodes get dropped. + * The two guards that matter most: keys are name-qualified (a multi-name + * declaration shares one definition node), and a match resolving to a value label + * registers nothing (so a match can never suppress itself). + */ +import { describe, expect, it } from 'vitest'; +import type { LanguageProvider } from '../../../src/core/ingestion/language-provider.js'; +import { + buildDefinitionPreScan, + type SyntaxNode, +} from '../../../src/core/ingestion/utils/ast-helpers.js'; + +/** Minimal stub — the pre-scan only reads `startIndex` and `text`. */ +const node = (startIndex: number, text: string): SyntaxNode => + ({ startIndex, text }) as unknown as SyntaxNode; + +const match = (captures: Record) => ({ + captures: Object.entries(captures).map(([name, syntaxNode]) => ({ name, node: syntaxNode })), +}); + +/** `getLabelFromCaptures` only reaches for `labelOverride`; nothing else. */ +const PROVIDER = {} as unknown as LanguageProvider; + +/** The non-value claim set — what `Const`/`Static`/`Variable` consult. */ +const nonValueOf = ( + matches: Parameters[0], + provider: LanguageProvider, +): ReadonlySet => buildDefinitionPreScan(matches, provider).nonValue; + +describe('buildDefinitionPreScan', () => { + it('registers a function capture under its startIndex and name', () => { + const keys = nonValueOf( + [match({ 'definition.function': node(0, 'const Bare = () => 1;'), name: node(6, 'Bare') })], + PROVIDER, + ); + + expect([...keys]).toEqual(['0:Bare']); + }); + + it('registers nothing for a value capture', () => { + const keys = nonValueOf( + [match({ 'definition.const': node(0, 'const CONFIG = {};'), name: node(6, 'CONFIG') })], + PROVIDER, + ); + + expect([...keys]).toEqual([]); + }); + + it('registers nothing for a match with no name capture', () => { + const keys = nonValueOf([match({ 'definition.function': node(0, '() => 1') })], PROVIDER); + + expect([...keys]).toEqual([]); + }); + + it('registers nothing for a match with no definition capture', () => { + const keys = nonValueOf([match({ name: node(0, 'orphan') })], PROVIDER); + + expect([...keys]).toEqual([]); + }); + + it('keys by name so a shared definition node does not over-suppress', () => { + // `const a = 1, b = () => {}` — both declarators share ONE definition node, + // so only `b`'s name may be claimed. + const declaration = node(0, 'const a = 1, b = () => {}'); + const keys = nonValueOf( + [ + match({ 'definition.const': declaration, name: node(6, 'a') }), + match({ 'definition.function': declaration, name: node(13, 'b') }), + ], + PROVIDER, + ); + + expect([...keys]).toEqual(['0:b']); + }); + + it('registers nothing when a provider reclassifies a function capture to a value label', () => { + // Guards against self-suppression: if the pre-scan keyed off capture names + // rather than the resolved label, this match would register a key and then + // the main loop's value branch would drop its own node. + const provider = { + labelOverride: () => 'Const', + } as unknown as LanguageProvider; + + const keys = nonValueOf( + [match({ 'definition.function': node(0, 'val x = {}'), name: node(4, 'x') })], + provider, + ); + + expect([...keys]).toEqual([]); + }); + + it('returns an empty set for no matches', () => { + expect([...nonValueOf([], PROVIDER)]).toEqual([]); + }); + + it('ranks a property claim as non-value but NOT callable', () => { + // The rank split is what keeps an annotated Python attribute ahead of its + // bare-assignment `Variable` twin while still letting a callable collapse a + // Kotlin/Swift closure property. A property in `callable` would make a + // property suppress itself. + const claims = buildDefinitionPreScan( + [match({ 'definition.property': node(0, 'name: str = "x"'), name: node(0, 'name') })], + PROVIDER, + ); + + expect({ nonValue: [...claims.nonValue], callable: [...claims.callable] }).toEqual({ + nonValue: ['0:name'], + callable: [], + }); + }); + + it('ranks a callable claim into both sets', () => { + const claims = buildDefinitionPreScan( + [match({ 'definition.function': node(0, 'val f = { }'), name: node(4, 'f') })], + PROVIDER, + ); + + expect({ nonValue: [...claims.nonValue], callable: [...claims.callable] }).toEqual({ + nonValue: ['0:f'], + callable: ['0:f'], + }); + }); +});