Skip to content

feat(cpp): Add structured resolver suppression outcomes#1785

Merged
magyargergo merged 6 commits into
abhigyanpatwari:mainfrom
azizur100389:codex/cpp-resolution-outcome
May 24, 2026
Merged

feat(cpp): Add structured resolver suppression outcomes#1785
magyargergo merged 6 commits into
abhigyanpatwari:mainfrom
azizur100389:codex/cpp-resolution-outcome

Conversation

@azizur100389

Copy link
Copy Markdown
Contributor

Fixes #1638

Summary

  • add a ResolutionOutcome diagnostic type for registry-primary resolver decisions
  • thread an additive outcome stream through scope-resolution and pipeline results without changing graph edge emission
  • record C++ suppression reasons for inline namespace ambiguity and overload/conversion ambiguity in receiver/free-call paths
  • add C++ integration assertions that diagnostics are observable while existing zero-edge suppression behavior is preserved

Tests

  • npx tsc --noEmit
  • REGISTRY_PRIMARY_CPP=1 focused structured suppression tests: passed
  • REGISTRY_PRIMARY_CPP=1 focused C++ ambiguity and inline namespace tests: passed
  • REGISTRY_PRIMARY_CPP=0 focused structured suppression tests: skipped as expected
  • REGISTRY_PRIMARY_CPP=1 full cpp resolver suite: 242 passed
  • REGISTRY_PRIMARY_CPP=0 full cpp resolver suite: 193 passed, 49 skipped
  • git diff --check

Local full-suite note

  • npm test was run locally and did not pass: 9213 passed, 44 skipped, 24 failed. The failures were outside this change area, including worker readiness, non-git temp directory assumptions, route/tool extraction fixtures, Ruby worker-path assertion, and the OOM guidance e2e.
  • npx gitnexus impact and npx gitnexus detect-changes were attempted; both exited 1 locally without usable impact output.

@vercel

vercel Bot commented May 22, 2026

Copy link
Copy Markdown

@azizur100389 is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions

github-actions Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

CI Report

All checks passed

Pipeline Status

Stage Status Details
✅ Typecheck success tsc --noEmit
✅ Tests success unit tests, 3 platforms
✅ E2E success gitnexus-web changes only

Test Results

Tests Passed Failed Skipped Duration
9668 9667 0 1 541s

✅ All 9667 tests passed

1 test(s) skipped — expand for details
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 79.34% 32458/40905 N/A% 🟢 ███████████████░░░░░
Branches 68% 20792/30574 N/A% 🟢 █████████████░░░░░░░
Functions 84.12% 3385/4024 N/A% 🟢 ████████████████░░░░
Lines 82.71% 29222/35329 N/A% 🟢 ████████████████░░░░

📋 View full run · Generated by CI

@github-actions

github-actions Bot commented May 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 8m 32s —— View job


PR #1785 — Production-Readiness Review

Reviewer: Combined production-readiness / ISO C++ static analysis / C++ compiler-engineering / GitNexus maintainer persona
Review basis: Direct source inspection of all 9 changed files + 9 related files + CI status


Review bar for this PR

The highest priority is graph correctness (false negatives preferred over false positives). Diagnostic observability is valuable only if additive, stable, and truthful. A diagnostic that says "suppressed" for a call site that later gets an edge from a downstream pass is worse than no diagnostic at all — it would actively mislead consumers.


Problem being solved

Issue #1638 requests that the C++ resolver's intentional suppression decisions (undefined / 'ambiguous' returns) become externally observable as a structured ResolutionOutcome type, enabling wiki/debug/future graph diagnostics to distinguish "no candidate found" from "deliberately suppressed because X". The stated bar is additive — no graph-edge changes, at least two distinct reasons observable, resolver migration opt-in.


Current PR state

  • Typecheck: ✅ (prior CI run confirmed; no new tsc errors)
  • CI: in progress (run #26294413573 — Gitleaks ✅, Dependency Review ✅, PR Autofix ✅, CodeQL ✅, CI and Docker still running at review time)
  • Prior CI run (#26292171535): reported "Tests | failure" on Windows-latest with CLI e2e READY-signal timeouts — an infrastructure/environmental failure pattern, not caused by changed files. Test summary for that run: 9556 passed, 0 failed, 1 skipped. Failures are consistent with pre-existing Windows worker-readiness flakiness.
  • Branch: clean one-commit feature PR; 9 files changed, all scoped to resolver diagnostics, pipeline plumbing, and C++ tests. No unrelated churn.

Merge status and mergeability

Current CI: in progress. Cannot confirm pass/fail for this run at review time. Prior run Windows CI gate failure appears pre-existing/environmental. Final merge verdict depends on this run completing cleanly.


Repository history considered


Branch hygiene assessment

Clean. Commit d0d782a touches exactly the 9 files claimed. The merge commit 2c5a781 brings in WAL checkpoint changes (#1772) from main — those are in-tree but unrelated to this review. git diff --check on the PR commit: clean (no trailing whitespace). Unicode/BIDI scan: clean (no hidden controls in any changed file).


Understanding of the change

The PR introduces:

  1. resolution-outcome.ts: ResolutionOutcome discriminated union, ResolutionSuppressionReason union (6 reasons), ResolutionOutcomeRecorder callback type.
  2. run.ts: Internal resolutionOutcomes[] collected per run, forwarded to an optional parent callback.
  3. phase.ts: ScopeResolutionOutput.resolutionOutcomes, phase-level aggregation, NOOP_OUTPUT with empty array.
  4. pipeline.ts: runPipelineFromRepo pulls resolutionOutcomes from scopeResolution output unconditionally (safe, scopeResolution always runs).
  5. types/pipeline.ts: PipelineResult.resolutionOutcomes?: readonly ResolutionOutcome[].
  6. free-call-fallback.ts: recordSuppressedOutcome helper; adl-non-callable-block and normalization/rank-tie reasons recorded at overload suppression branches.
  7. receiver-bound-calls.ts: inline-ns-ambiguous recorded at 'ambiguous' sentinel; recordReceiverOverloadSuppression records normalization/rank-tie reasons at OVERLOAD_AMBIGUOUS return.
  8. Tests: two new it blocks asserting outcome observability; both added to LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp.

Findings


Finding 1 — adl-non-callable-block diagnostic placed on a path that does NOT mark the site as handled

Risk: The diagnostic records "suppressed" but emitReferencesViaLookup can still emit an edge for the same call site, making the diagnostic a lie at the graph level.

Evidence to check: free-call-fallback.ts, lines 200–211:

if (adlSuppressed && ordinary.length === 0) {
  recordSuppressedOutcome(options.recordResolutionOutcome, {
    reason: 'adl-non-callable-block',
    candidates: ordinary,   // always []
  });
  // ← NO handledSites.add(siteKey), NO continue
}
if (adl === undefined || adl.length === 0) { // true since adl===undefined
  // ordinary.length===0 → fnDef = ordinary[0] = undefined

Confirmed evidence: When adlSuppressed && ordinary.length === 0, the diagnostic is recorded, but:

  • No handledSites.add(siteKey) is called.
  • No continue is issued.
  • Execution falls into if (adl === undefined || adl.length === 0), sets fnDef = ordinary[0] = undefined.
  • Falls through to if (fnDef === undefined) continue; at line 327 — so no edge from this pass.
  • But the site is not in handledSites, so emitReferencesViaLookup will process it. If the registry resolved a target for this name (e.g., a namespace member visible globally), a CALLS edge is emitted after the diagnostic already said "adl-non-callable-block suppressed."

Confirmed: The graph behavior was identical before this PR (the site was already not in handledSites), so this is not a new graph regression. However, the diagnostic is newly incorrect: it reports suppression for a call site that may still receive an edge. This is a diagnostic truthfulness failure.

Recommended fix: After recording the adl-non-callable-block outcome, add handledSites.add(siteKey); continue;. If the intent is to record "ADL was blocked" but still allow fallback, the reason name is wrong — rename to adl-non-callable-observed and document that fallback may still run. Fix this →

Blocks merge: yes — diagnostic correctness failure; a consumer trusting adl-non-callable-block cannot rely on the edge not being emitted.


Finding 2 — conversion-rank-tied reason is over-broad

Risk: The reason conversion-rank-tied is used as a fallback for any multi-candidate case that does not trigger isOverloadAmbiguousAfterNormalization, even when no conversion-rank comparison was performed (no argTypes, no conversionRankFn, arity-filter alone left multiple survivors).

Evidence to check: free-call-fallback.ts, suppressionReasonForOverload (lines 362–369):

function suppressionReasonForOverload(
  candidates: readonly SymbolDefinition[],
  arity: number | undefined,
): ResolutionSuppressionReason {
  return isOverloadAmbiguousAfterNormalization(candidates, arity)
    ? 'overload-ambiguous-normalization'
    : 'conversion-rank-tied';  // ← fallback for ALL other multi-candidate cases
}

Confirmed evidence: narrowOverloadCandidates reaches the multi-candidate exit in several distinct situations:

  1. Arity filter left >1 candidates, no argTypes provided → no conversion ranking was attempted.
  2. Exact-type filter left >1 candidates and conversionRankFn is undefined → no conversion ranking.
  3. Conversion ranker ran and returned tied results → true conversion-rank-tied.

Cases 1 and 2 will be labelled conversion-rank-tied even though the ranker was never invoked. In the ADL merged-candidate path (lines 280–297), a suppression diagnostic is recorded and then isOverloadAmbiguousAfterNormalization is checked to decide whether to continue. The reason is already recorded before this check, so the reason can be conversion-rank-tied even when the normalization check would have returned true on a slightly different call.

Recommended fix: Pass a richer narrowing evidence struct from narrowOverloadCandidates (e.g., a suppressionCause: 'normalization' | 'rank-tie' | 'arity-ambiguous' field), or split the fallback reason into overload-ambiguous (arity/type ambiguity without ranking) and conversion-rank-tied (only when the ranker ran). At minimum, only emit conversion-rank-tied when options.conversionRankFn !== undefined was active in the narrowing call. Fix this →

Blocks merge: yes — the diagnostic reason is not reliably truthful. Downstream consumers (wiki, debug tools) will see conversion-rank-tied on sites where no ranking was performed.


Finding 3 — recordReceiverOverloadSuppression omits argumentTypeClasses

Risk: The candidate set recomputed in recordReceiverOverloadSuppression may differ from the candidate set that triggered OVERLOAD_AMBIGUOUS in pickOverload, causing the wrong reason and/or wrong candidateIds to be recorded.

Evidence to check: receiver-bound-calls.ts, lines 860–889 vs pickOverload lines 830–835:

// pickOverload (primary path):
const candidates = narrowOverloadCandidates(overloads, site.arity, site.argumentTypes, {
    argumentTypeClasses: site.argumentTypeClasses,  // ← present
    conversionRankFn: provider.conversionRankFn,
    constraintCompatibility: provider.constraintCompatibility,
});

// recordReceiverOverloadSuppression (diagnostic path):
const candidates = narrowOverloadCandidates(overloads, site.arity, site.argumentTypes, {
    // argumentTypeClasses: MISSING
    conversionRankFn: provider.conversionRankFn,
    constraintCompatibility: provider.constraintCompatibility,
});

Confirmed evidence: site.argumentTypeClasses is available in recordReceiverOverloadSuppression (it takes site: ParsedFile['referenceSites'][number]) but is not passed. The constraint filter (step 4c) in narrowOverloadCandidates uses argumentTypeClasses when evaluating constraintCompatibility. Without it, the constraint hook receives a ConstraintContext without argument type classes, potentially retaining candidates that would have been dropped in the primary path — leading to (a) wrong candidateIds (extra candidates) and (b) a potentially wrong reason assignment when isOverloadAmbiguousAfterNormalization is called on the recomputed set.

Recommended fix: Pass argumentTypeClasses: site.argumentTypeClasses in recordReceiverOverloadSuppression's call to narrowOverloadCandidates. Fix this →

Blocks merge: maybe — diagnostic inaccuracy (wrong candidateIds/reason) for receiver-bound overload suppression. Not a graph regression, but misleads consumers.


Finding 4 — inline-ns-ambiguous always emits candidateIds: [] without documentation

Risk: Downstream consumers expecting candidate IDs for inline namespace ambiguity will always see an empty array, making the candidateIds field useless for this reason. The PR type contract does not indicate this field is nullable/empty for specific reasons.

Evidence to check: receiver-bound-calls.ts, lines 477–485:

options.recordResolutionOutcome?.({
    kind: 'suppressed',
    phase: 'receiver-bound-calls',
    ...
    reason: 'inline-ns-ambiguous',
    candidateIds: [],   // ← always empty
});

Confirmed evidence: The 'ambiguous' sentinel from resolveQualifiedReceiverMember (PR #1600) carries no candidate definitions — they are lost at the sentinel boundary. No fix is possible without restructuring inline-namespaces.ts to return a richer result. The limitation is real and known.

Recommended fix: Either (a) add a JSDoc or inline comment in resolution-outcome.ts noting that inline-ns-ambiguous will always have candidateIds: [] until resolveQualifiedReceiverMember is refactored to return candidate definitions, and file a follow-up issue; or (b) add a separate test assertion confirming candidateIds.length === 0 for this reason so it's explicit rather than accidental. Fix this →

Blocks merge: maybe — not a graph regression, but undocumented limitation in a new public API field.


Finding 5 — Test assertions are too weak to catch mutations

Risk: The two new tests do not fail for several plausible mutations: wrong reason string, empty candidateIds when candidates exist, removed plumbing, wrong phase name.

Evidence to check: cpp.test.ts, lines 1824–1834 (normalization ambiguity) and 3001–3007 (inline namespace ambiguity):

// Normalization test:
expect(outcomes.length).toBeGreaterThan(0);
expect(outcomes[0]?.candidateIds.length).toBe(2);  // good

// Inline-ns-ambiguous test:
expect(outcomes.length).toBeGreaterThan(0);  // only this

Confirmed evidence:

  • Both tests check outcomes.length > 0 — would survive if reason strings were swapped.
  • Inline-ns-ambiguous test: no assertion on phase === 'receiver-bound-calls', no filePath, no range, no candidateIds.length === 0 (explicit for the known limitation).
  • Normalization test: candidateIds.length === 2 is good, but no phase, filePath, or range assertions.
  • Neither test verifies the outcome stream reaches PipelineResult.resolutionOutcomes with a guard that the runPipelineFromRepo plumbing is the source (a unit test calling runScopeResolution directly would also pass).
  • No test for conversion-rank-tied as an observable reason.
  • No test for adl-non-callable-block as an observable reason (the suppression test exists for zero edges, but not for the structured diagnostic).

Recommended fix: Add assertions on phase, filePath, and range.startLine for both tests. For the inline-ns-ambiguous test, add expect(outcomes[0]?.candidateIds.length).toBe(0) to document the known limitation explicitly. For conversion-rank-tied, add a test using an existing fixture (e.g., h(42, 2.5) in cpp-overload-conversion-rank) that asserts the reason. Fix this →

Blocks merge: yes — tests cannot catch mutations to reason strings, plumbing removal, or phase name changes.


Finding 6 — ResolutionSuppressionReason includes constraint-unknown and dependent-base-skipped that are never recorded

Risk: The public type union advertises reasons that no code path currently emits. Downstream consumers treating the union as exhaustive will write dead branches and be confused when these reasons never appear.

Evidence to check: resolution-outcome.ts, lines 3–9:

export type ResolutionSuppressionReason =
  | 'adl-non-callable-block'
  | 'constraint-unknown'       // ← never recorded
  | 'conversion-rank-tied'
  | 'dependent-base-skipped'   // ← never recorded
  | 'inline-ns-ambiguous'
  | 'overload-ambiguous-normalization';

Confirmed evidence: Searching free-call-fallback.ts, receiver-bound-calls.ts, and run.ts: no use of 'constraint-unknown' or 'dependent-base-skipped' as record arguments. These were in the issue #1638 taxonomy but are not wired in this PR. If left in the type, documentation should state they are reserved for future use.

Recommended fix: Either remove the un-implemented reasons from the union (and add them in the follow-up PR that wires them), or add a TSDoc comment on each un-wired reason noting it is reserved for a named follow-up and currently not emitted. Fix this →

Blocks merge: maybe — type truthfulness gap, not graph regression. Future consumers will be confused.


Finding 7 — Inconsistent siteKey format in emitFreeCallFallback

Risk: Minor, style only. Three different formats for the same key in the same function body.

Confirmed evidence:

  • Non-ADL path, line 168: handledSites.add(siteKey(parsed.filePath, site)) — calls module-level helper
  • ADL path, line 200: const siteKey = \${parsed.filePath}:${...}`` — local variable that shadows the function name
  • Normal edge emission, line 335: handledSites.add(\${parsed.filePath}:${site.atRange.startLine}:${site.atRange.startCol}`)` — inline template

All three produce identical strings, so no functional issue. The local siteKey variable in the ADL block shadows the module-level siteKey function, which is confusing.

Recommended fix: Use the module-level siteKey(parsed.filePath, site) helper consistently in all three locations.

Blocks merge: no — style only.


PR-specific assessment sections

A. API and contract lane

  • ResolutionOutcome type: well-structured discriminated union. phase and reason are string and ResolutionSuppressionReason respectively — appropriate width.
  • resolutionOutcomes?: readonly ResolutionOutcome[] on PipelineResult: additive, optional, does not break existing callers.
  • NOOP_OUTPUT.resolutionOutcomes: [] — the array is not deep-frozen, but typed as readonly ResolutionOutcome[] so compile-time protection is correct. Not a concern.
  • scopeResolutionPhase is always in the phase list regardless of skipGraphPhases, so getPhaseOutput(results, 'scopeResolution') in runPipelineFromRepo is always safe — no throw risk. ✅

B. Resolver recording lane: free-call fallback

  • recordSuppressedOutcome helper correctly guards with record?.() — no call when no recorder. ✅
  • Non-ADL same-file suppression branches correctly add to handledSites and continue. ✅
  • ADL merged-candidate paths (lines 280–297) correctly add to handledSites and continue. ✅
  • adl-non-callable-block branch (Finding 1): no handledSites.add/continue after recording.
  • suppressionReasonForOverload: see Finding 2. ❌
  • candidateIds: candidates.map(d => d.nodeId) — uses nodeId not graph ID. Acceptable for incremental scope, but note that graph consumers who try to look up candidateIds in the graph may not find them directly (graph IDs may differ for overloaded/template functions). This is a known limitation acceptable at this PR's incremental scope.

C. Resolver recording lane: receiver-bound calls

  • inline-ns-ambiguous correctly placed at the 'ambiguous' sentinel check. ✅
  • recordReceiverOverloadSuppression called with consistent ownerId as primary pickOverload. ✅
  • Missing argumentTypeClasses in recordReceiverOverloadSuppression (Finding 3).
  • Receiver-bound candidateIds are node IDs (not graph IDs) — same caveat as free-call above.

D. Pipeline propagation lane

  • runScopeResolution internal resolutionOutcomes[] + forwarding to parent callback: two separate arrays, no duplication in either. ✅
  • scopeResolutionPhase aggregates via callback across all languages. Multi-language outcomes share the same array — phase field distinguishes free-call-fallback vs receiver-bound-calls but not language. File extension provides implicit language context, which is acceptable for this incremental scope. ✅
  • runPipelineFromRepo unconditional getPhaseOutput('scopeResolution'): safe (phase always runs). ✅

E. Test lane

  • Both new tests in LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp — correct, since they depend on registry-primary C++ behavior. ✅
  • getResolutionOutcomes helper in helpers.ts: reads result.resolutionOutcomes ?? [] — additive, safe for legacy path (returns empty). ✅
  • Test mutation strength is insufficient (Finding 5). ❌
  • No test for conversion-rank-tied as observable reason. ❌
  • No test for adl-non-callable-block as observable reason. ❌

F. ISO C++ / compiler-engineering lane

  • inline-ns-ambiguous: Qualified lookup ambiguity correctly suppressed. Losing candidate IDs is a known limitation of the 'ambiguous' sentinel design. Acceptable for this scope with documentation (Finding 4).
  • overload-ambiguous-normalization / conversion-rank-tied: The distinction between normalization collision and conversion-rank tie is correct in principle, but the assignment is not reliably truthful (Finding 2).
  • adl-non-callable-block: ISO C++ [basic.lookup.unqual] §7 ADL blocker — the semantics are correct (ADL suppressed by non-callable/block-scope), but the diagnostic placement is incorrect (Finding 1).
  • constraint-unknown / dependent-base-skipped: Not wired — correctly out of scope for this PR if removed from the type or documented as reserved (Finding 6).

G. Static-analysis production lane

  • Ranges use ParsedFile['referenceSites'][number].atRange — consistent with existing range conventions. ✅
  • Diagnostics do not serialize into cache/output unexpectedly: PipelineResult.resolutionOutcomes is optional and only materialized in-memory per run. ✅
  • No absolute paths leak: filePath values come from parsed.filePath which is already in the existing graph and workspace-relative per GitNexus conventions. ✅
  • Determinism: outcome ordering is deterministic (same file iteration order as edge emission). ✅

Back-and-forth avoided by verifying

  • getPhaseOutput crash when scopeResolution absent: Confirmed safe — scopeResolutionPhase is always in the phase list, NOOP_OUTPUT.resolutionOutcomes = [] handles the no-language-ran case. No crash risk.
  • Duplicate outcomes from nested recorders: Confirmed not a duplication issue — RunScopeResolutionStats.resolutionOutcomes and ScopeResolutionOutput.resolutionOutcomes are separate arrays, not the same array double-pushed.
  • ownerGraphId vs nodeId namespace mismatch: Confirmed that recordReceiverOverloadSuppression uses the same ownerId as the primary pickOverload call — consistent. The argumentTypeClasses omission is the actual diagnostic inaccuracy (Finding 3).
  • Windows CI flakiness: Confirmed via CI bot comment and current CI run status — READY-signal timeouts are environment/infrastructure, not caused by this PR's changes.
  • Hidden Unicode: Confirmed clean (direct file scan of all 9 changed files).

Open questions

None that are unavoidable given the review scope. The findings above are sufficient for an actionable verdict.


Final verdict

❌ not production-ready

Rationale (3–6 sentences):

Three diagnostic correctness defects block this PR from the production DoD bar. Finding 1 (adl-non-callable-block recorded without handledSites.add) means the diagnostic can report "suppressed" for a site that emitReferencesViaLookup then processes, making the diagnostic actively misleading to downstream consumers. Finding 2 (conversion-rank-tied reason is over-broad) means a consumer cannot distinguish "conversion ranker ran and tied" from "multiple candidates remained because there was no argument type information" — the diagnostic lies about its own cause. Finding 3 (missing argumentTypeClasses in recordReceiverOverloadSuppression) means receiver-bound overload suppression diagnostics may carry wrong candidateIds and a wrong reason. Finding 5 (test weakness) means that fixing Finding 1 and 2 would not be caught by regression if the tests remain as written. Findings 4 and 6 are minor documentation/type honesty gaps. The CI gate from the prior run showed Windows infrastructure flakiness (pre-existing), and the current re-run is in progress — if that run passes cleanly, CI is not a blocker; the four code-level findings are.


@magyargergo

Copy link
Copy Markdown
Collaborator

@azizur100389 can you please resolve the merge conflicts? 🙏

@azizur100389

Copy link
Copy Markdown
Contributor Author

Addressed the review findings and resolved the main-branch conflicts.

Changes:

  • ADL blocker outcomes now mark the site handled before returning, so a suppression diagnostic cannot be followed by a downstream edge.
  • Overload diagnostics now distinguish normalization ambiguity, conversion-rank ties, and generic overload ambiguity.
  • Receiver-bound diagnostic recomputation now passes argumentTypeClasses, matching the primary narrowing path.
  • Inline namespace ambiguity now documents and tests the empty candidateIds limitation.
  • Strengthened C++ diagnostic assertions and added ADL blocker / conversion-rank tie coverage.

Validation:

  • npx tsc --noEmit
  • REGISTRY_PRIMARY_CPP=1 full C++ resolver suite: 244 passed
  • REGISTRY_PRIMARY_CPP=0 full C++ resolver suite: 192 passed, 52 skipped
  • git diff --check
  • npm test was run locally before the merge update; unrelated/local environment failures remain: 9313 passed, 24 failed, 44 skipped.
  • npx gitnexus detect-changes was attempted and exited 1 locally without output.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an additive “resolution outcome” diagnostics stream to the registry-primary scope-resolution pipeline (focused on C++), so callers can observe why certain CALLS edges were deliberately suppressed without changing existing edge-emission semantics.

Changes:

  • Introduces ResolutionOutcome / ResolutionSuppressionReason types and threads an outcomes stream through scope-resolution → pipeline output.
  • Records structured suppression outcomes for key C++ suppression cases (inline-namespace ambiguity, overload/conversion ambiguity, ADL suppression).
  • Adds C++ integration assertions that outcomes are observable while preserving existing “zero edges on suppression” behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
gitnexus/test/integration/resolvers/helpers.ts Adds a helper to read resolutionOutcomes from PipelineResult.
gitnexus/test/integration/resolvers/cpp.test.ts Adds assertions for structured suppression reasons in C++ fixtures.
gitnexus/src/types/pipeline.ts Extends PipelineResult with optional resolutionOutcomes.
gitnexus/src/core/ingestion/scope-resolution/resolution-outcome.ts Defines ResolutionOutcome and related types.
gitnexus/src/core/ingestion/scope-resolution/pipeline/run.ts Collects and threads outcome recording through scope-resolution passes.
gitnexus/src/core/ingestion/scope-resolution/pipeline/phase.ts Aggregates outcomes into the scope-resolution phase output.
gitnexus/src/core/ingestion/scope-resolution/passes/receiver-bound-calls.ts Records suppression outcomes for receiver-bound ambiguities (incl. inline-ns and overload ambiguity).
gitnexus/src/core/ingestion/scope-resolution/passes/free-call-fallback.ts Records suppression outcomes for free-call fallback ambiguities and ADL suppression.
gitnexus/src/core/ingestion/pipeline.ts Exposes scope-resolution outcomes on the final PipelineResult.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 183 to +212
const {
callables: ordinary,
nonCallableFound,
blockScopeDeclFound,
} = findCallableBindingsAndAdlBlocker(site.inScope, site.name, scopes);
const adlSuppressed = nonCallableFound || blockScopeDeclFound;
const adl = adlSuppressed
? undefined
: options.resolveAdlCandidates(
{
name: site.name,
arity: site.arity,
argumentTypes: site.argumentTypes,
atRange: { startLine: site.atRange.startLine, startCol: site.atRange.startCol },
},
parsed,
scopes,
parsedFiles,
);

const siteKey = `${parsed.filePath}:${site.atRange.startLine}:${site.atRange.startCol}`;
const key = siteKey(parsed.filePath, site);
if (adlSuppressed && ordinary.length === 0) {
recordSuppressedOutcome(options.recordResolutionOutcome, {
phase: 'free-call-fallback',
filePath: parsed.filePath,
name: site.name,
range: site.atRange,
reason: 'adl-non-callable-block',
candidates: ordinary,
});
if (isOverloadAmbiguousAfterNormalization(candidates, arity)) {
return 'overload-ambiguous-normalization';
}
if (ctx.conversionRankFn !== undefined && ctx.argumentTypes !== undefined) {
Comment thread gitnexus/src/types/pipeline.ts Outdated
Comment on lines +17 to +20
* deliberately suppress edge emission. Empty/undefined means no diagnostic
* stream was requested or produced; graph edge semantics are unchanged.
*/
resolutionOutcomes?: readonly ResolutionOutcome[];
@azizur100389

Copy link
Copy Markdown
Contributor Author

Addressed the latest review comments.

Changes:

  • Renamed the ADL suppression reason to cover both ordinary-lookup blocker cases instead of implying only non-callable bindings.
  • Matched conversion-rank diagnostics to the narrowing guard by requiring non-empty argument types before reporting a rank tie.
  • Made resolutionOutcomes always present on PipelineResult and updated the field documentation accordingly.

Validation:

  • npx tsc --noEmit
  • REGISTRY_PRIMARY_CPP=1 focused diagnostics: 4 passed
  • REGISTRY_PRIMARY_CPP=1 full C++ resolver suite: 244 passed
  • REGISTRY_PRIMARY_CPP=0 full C++ resolver suite: 192 passed, 52 skipped
  • git diff --check
  • GitNexus impact: emitFreeCallFallback LOW, PipelineResult LOW
  • GitNexus detect-changes: 4 files, 3 symbols, medium affected-flow risk limited to free-call fallback flows
  • npm test was run locally; unrelated environment-sensitive failures remain (worker readiness, temp-dir git-root detection, route/tool extraction fixtures, eval-server local registry).

@magyargergo magyargergo merged commit eb69f66 into abhigyanpatwari:main May 24, 2026
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cpp resolver: structured suppression diagnostics (ResolutionOutcome)

3 participants