feat(cpp): thread base-specifier qualifier through dependent-base lookup (#1815)

[prajapatisparsh]

Summary

Infrastructure for #1815. Provide the syntactic namespace qualifier (e.g. detail in detail::Inner<T>) from AST capture through storage to resolution. Part of #1564.

Changes

captures.ts:

• extractBaseLookupQualifier — reads the scope field from qualified_identifier AST nodes (e.g. detail from detail::Inner<T>)
• isBaseDependent fix — now recognizes qualified_identifier bases (previously rejected unconditionally); they descend into children to check for template parameter references

two-phase-lookup.ts:

• dependentBasesByFile storage changed from Set<baseName> to Map<baseName, qualifier>
• markCppDependentBase accepts optional qualifier parameter
• classesBySimpleName index deduplicated by nodeId (last-wins Map); removes duplicate entries caused by scope-extractor and class-extractor overlap
• V3 qualifier-based exact targeting in multi-candidate path (dormant — see note below)
• V2 prefix-heuristic fallback unchanged

What's proven

Component	Status
Qualifier extraction	✓ Extracts "detail" from detail::Inner<T>
isBaseDependent fix	✓ qualified_identifier bases now processed
Qualifier storage	✓ Capture → storage → resolver
Index dedup	✓ Duplicate nodeId entries eliminated

What's dormant

The V3 qualifier targeting and V2 prefix-heuristic both rely on nsPrefix derived from def.qualifiedName on parsed.localDefs entries. The class extractor's buildQualifiedName correctly produces mylib.detail.Inner but this qualifiedName doesn't reach localDefs — the scope-extractor creates separate defs with simple-name qualifiedNames like "Inner" and "Inner.Inner". Fixing this requires changes in parsing-processor.ts to thread the class extractor's qualifiedName into @declaration.qualified_name captures (tracked separately).

Testing

Default mode: 250/250 passed
Legacy mode:  196 passed, 54 skipped, 0 failed
Pre-commit:   eslint, prettier, typechecking passed

[vercel]

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

A member of the Team first needs to authorize it.

[prajapatisparsh]

hey @magyargergo can u give me the claude's insight by reviewing i am not sure about this implementation no need to run the CI till then

[github-actions]

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
9857	9854	0	3	612s

✅ All 9854 tests passed

3 test(s) skipped — expand for details

• PHP pipeline benchmark > scales with file count (workers enabled)
• Rust pipeline benchmark > scales with file count (workers enabled)
• 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.88%	34032/42601	N/A%	—	🟢 ███████████████░░░░░
Branches	68.47%	21750/31764	N/A%	—	🟢 █████████████░░░░░░░
Functions	84.81%	3524/4155	N/A%	—	🟢 ████████████████░░░░
Lines	83.36%	30651/36768	N/A%	—	🟢 ████████████████░░░░

---

_{📋 View full run · Generated by CI}

[github-actions]

Claude finished @magyargergo's task in 5m 17s —— View job

---

PR #1819 Review — feat(cpp): thread base-specifier qualifier through dependent-base lookup

Todo

• Read repo docs (DoD.md, AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md)
• Verify merge state and CI status
• Inspect branch hygiene (merge commit, changed files)
• Read changed files: captures.ts, two-phase-lookup.ts (full diff vs origin/main)
• Search for all referenced symbols and call sites
• Inspect fixtures and test coverage
• Run Unicode/bidi hygiene checks
• Assess each finding against DoD
• Write final verdict

---

Review bar for this PR

DoD.md §2.1 (real runtime path), §2.7 (tests cover changed path), DoD §6 (not-done signals: runtime path not exercised by tests). Issue #1564 policy: suppress rather than guess. Issue #1815 DoD: cpp-two-phase-dependent-base-cross-ns-sibling-suppress should flip from 0 → 1 edge.

---

Problem being solved

GitNexus C++ two-phase dependent-base lookup cannot currently distinguish detail::Inner<T> from public_api::Inner<T> when both share the simple name Inner. The previous V2 heuristic suppresses both. The intended fix is to thread the syntactic namespace qualifier (detail) from the base-specifier AST node through storage and into resolution, so the resolver can exact-match the correct sibling namespace.

---

Current PR state

Two TypeScript files changed. No test files changed. CI reports all 9847 tests passing (prior tests unchanged). The PR body explicitly states V3 qualifier targeting is dormant because qualifiedName does not reliably reach parsed.localDefs for local class definitions.

---

Merge status and mergeability

• Branch: prajapatisparsh:feat/1815-base-specifier-qualifier → abhigyanpatwari:main
• Commits: c207b29 (feature, touches only 2 C++ files) + 2e366c2 (merge-from-main, brings in unrelated Rust/setup/CLI changes from main — not PR content)
• CI: ✅ All checks passed per bot comment (9847 tests, typecheck, E2E)
• Vercel: Authorization pending — preview deployment only, not a merge gate
• Hidden Unicode: git grep scan confirmed no bidi/hidden characters in either changed file
• mergeStateStatus / reviewDecision: Not directly verified via gh (no gh tool access), but CI is green

---

Repository history considered

• LANG-cpp: Remaining work after PR #1520 scope-resolution parity #1564: Under-approximation policy — suppress over guess
• cpp two-phase: thread base-specifier qualifier through dependent-base lookup for ISO-correct sibling disambiguation #1815: Direct issue; DoD requires sibling-suppress fixture to flip from 0 → 1 edge
• fix(cpp): dependent-base resolution across nested/inline namespaces (#1634) #1814 / cpp two-phase lookup: dependent-base resolution across namespaces / inline namespaces #1634: Previous prefix-contains / one-level-cap implementation (V2), all prior fixtures still pass
• feat(cpp): resolve template-body this-> + using ns::name calls in scope resolver #1590 / cpp: positive resolution inside template bodies (this->, namespace::) #1570: this-> dependent lookup fixtures — not modified by this PR

---

Branch hygiene assessment

The feature commit (c207b29) touches only the two intended C++ files. The merge-from-main commit (2e366c2) brings in ~30 unrelated files from main (Rust migration, setup hooks, CLI changes) — this is expected from a merge sync, not churn introduced by the author. The PR diff seen by reviewers (+104/-39, 2 files) accurately reflects the feature work. Branch hygiene: acceptable.

---

Understanding of the change

captures.ts:

• extractBaseLookupQualifier: new function that reads childForFieldName('scope') on qualified_identifier nodes. For detail::Inner<T> returns 'detail'; for ns::detail::Inner<T> (nested) returns the full scope text; for unqualified bases returns ''.
• isBaseDependent fix: qualified_identifier nodes now fall through to the stack walk instead of returning false immediately. This allows detail::Inner<T> (where Inner<T> is the template_type child) to be recognized as dependent.
• detectCppDependentBases: extracts qualifier and passes it to markCppDependentBase.

two-phase-lookup.ts:

• dependentBasesByFile changed from Map<className, Set<baseName>> to Map<className, Map<baseName, qualifier>>.
• markCppDependentBase gains optional qualifier parameter; stores bases.set(baseName, qualifier).
• classesBySimpleName building now deduplicates by nodeId (last-wins) before populating the index.
• V3 qualifier path: when baseQualifier is non-empty, computes expectedNs = classEntry.nsPrefix + '.' + normalizedQualifier and attempts exact (===) match against candidates. Falls through to V2 on miss.
• V2 prefix-heuristic fallback unchanged.

---

Findings

---

Finding 1 — Dormant V3 does not satisfy #1815 DoD

Risk: The PR title claims to implement #1815 (feat(cpp): thread base-specifier qualifier through dependent-base lookup). The issue #1815 DoD explicitly requires the cpp-two-phase-dependent-base-cross-ns-sibling-suppress fixture to flip from 0 edges → 1 edge, targeting the qualifier-correct detail::Inner<T>. This PR does not do that.

Evidence found:

• gitnexus/test/integration/resolvers/cpp.test.ts:2371–2375:

  it('Derived<T>::g() -> this->f_a() emits zero CALLS when detail::Inner and public_api::Inner are sibling namespaces (ambiguity suppressed)', () => {
    const suppressed = calls.filter((c) => c.source === 'g' && c.target === 'f_a');
    expect(suppressed.length).toBe(0);  // ← UNCHANGED: still expects 0
  });

• PR body: "Fixing this requires changes in parsing-processor.ts to thread the class extractor's qualifiedName into @declaration.qualified_name captures (tracked separately)."
• The V3 path in two-phase-lookup.ts:206–214 is guarded by if (baseQualifier) and requires candidates[i].nsPrefix === expectedNs — but the nsPrefix for local class definitions is derived from def.qualifiedName ?? '' which the PR body says is unreliable (scope-extractor produces simple-name qualifiedName first, class extractor produces fully-qualified but may not be what ends up in parsed.localDefs).

Recommended fix: Either (a) complete the parsing-processor.ts plumbing in this PR so qualifiedName reaches parsed.localDefs and V3 actually fires, add a fixture asserting 1 edge and a negative assertion for public_api::Inner, or (b) retitle/re-scope this PR as infrastructure-only, update the description to not claim #1815 completion, and track the feature gap in a follow-up issue.

Blocks merge: yes if claiming #1815 feature completion. Maybe if maintainers explicitly accept an infrastructure-only PR and create a clear follow-up — but the title and linked issue currently imply completion.

Fix this →

---

Finding 2 — Map<baseName, qualifier> collapses two bases with the same simple name

Risk: markCppDependentBase stores: bases.set(baseName, qualifier). If a class inherits from two bases sharing the same simple name but different qualifiers — e.g., Derived<T> : ns1::Inner<T>, ns2::Inner<T> — the second call silently overwrites the first. One base is lost; the edge to the first base's members is never produced.

Evidence found: two-phase-lookup.ts:83:

bases.set(baseName, qualifier);  // overwrites if baseName already present

No fixture tests this case. The storage model is a Map<baseName, qualifier>, not a Map<baseName, qualifier[]> or Map<{baseName, qualifier}, ...>.

Recommended fix: Change the storage to Map<baseName, qualifier[]> (or use a stable composite key like ${baseName}@${qualifier}) and update the resolution loop accordingly. While C++ disallows inheriting the same fully-qualified type twice, inheriting two distinct types that share a simple name via different namespace paths is legal and the fixture in sibling-suppress demonstrates exactly this scenario exists.

Blocks merge: yes — this is a structural data loss bug in the storage layer.

Fix this →

---

Finding 3 — Unique-candidate path bypasses qualifier check — false-positive graph edge

Risk: When only one class is indexed with a given simple name, the code accepts it unconditionally, even when an explicit qualifier is present that does not match:

// two-phase-lookup.ts:191-199
if (candidates.length === 1) {
  // Even if the namespace doesn't match the qualifier, recording
  // the candidate as a dependent base is better than skipping it
  bases.add(candidates[0].nodeId);
  continue;
}

If the correct class is in another translation unit (not indexed) and only a wrong-namespace class with the same simple name is indexed, a false-positive edge is added. This directly violates #1564's suppress over guess policy and the PR-specific DoD item 4 ("use qualifier information conservatively — if the qualifier cannot be matched reliably, suppress").

The comment cites "#1815 discussion on B2 edge cases" as justification, but no fixture or issue link demonstrates that the false-positive is acceptable here.

Recommended fix: When baseQualifier is non-empty and the sole candidate's nsPrefix does not match either expectedNs or normalizedQualifier, suppress rather than add. Only apply the single-candidate fast-accept for unqualified bases (baseQualifier === ''), which is the original pre-PR behavior.

Blocks merge: yes — violates the repo's stated conservative policy and can produce wrong graph edges.

Fix this →

---

Finding 4 — V3 qualifier mismatch falls through to V2, potentially producing wrong edges

Risk: When V3 qualifier matching fails (exact nsPrefix match not found), the code falls through to V2's prefix-contains heuristic. The V2 heuristic was designed for unqualified bases where namespace context is the only signal. Applying it when an explicit qualifier is present and mis-matched can produce false-positive edges: V2 may accept a candidate from the wrong namespace when the qualifier clearly excludes it.

Evidence found: two-phase-lookup.ts:214:

// Exact qualifier match failed — fall through to V2 prefix-heuristic.

Then V2 runs for all candidates regardless of qualifier. For example: Derived in ns::outer with base detail::Inner<T> — V3 fails (dormant), V2 then checks if any Inner candidate has nsPrefix.startsWith('ns.outer.'), which would match ns::outer::inner::Inner but also potentially match wrong candidates.

In the cross-ns-sibling-suppress fixture this is safe because mylib::detail::Inner and mylib::public_api::Inner both satisfy startsWith('mylib.') but the V2 one-level cap excludes them (too deep from mylib). However, for other fixture shapes this fallthrough may not be safe.

Recommended fix: When baseQualifier is non-empty and V3 match fails, suppress immediately rather than falling through to V2. The qualifier is explicit information; falling back to a heuristic that ignores it is contradictory.

Blocks merge: maybe — unverified whether existing fixtures expose this, but the fallthrough is architecturally unsafe for the stated policy.

---

Finding 5 — No new tests for any changed code path

Risk: The feature commit changes two TypeScript files and zero test files. There is no new fixture, no new test assertion, and no modified assertion that proves the new extractBaseLookupQualifier function runs, that the qualifier is stored, that V3 is attempted, or that isBaseDependent's new qualified_identifier branch fires.

Evidence found:

• git diff HEAD~2..HEAD~1 -- gitnexus/test/ produced no output — no test files changed
• The sibling-suppress test at cpp.test.ts:2371 is unchanged and still expects 0 edges
• DoD §2.7: "Tests cover the real changed path — they would fail if behavior, wiring, or contracts were broken"
• DoD §6 not-done signal: "The runtime path is not actually exercised by the tests"

Recommended fix: Add at minimum:

1. A fixture asserting extractBaseLookupQualifier correctly returns 'detail' for detail::Inner<T> (unit test or integration fixture).
2. A fixture asserting isBaseDependent returns true for a qualified_identifier base with a template argument.
3. When V3 is made functional: a positive assertion that the sibling-suppress fixture produces exactly 1 edge to detail::Inner::f_a, and a negative assertion that public_api::Inner::f_b produces 0 edges.

Blocks merge: yes per DoD §2.7 and §6.

---

ISO C++ qualifier semantics

• detail::Inner<T> (relative qualifier): The extractBaseLookupQualifier correctly returns 'detail'. For a Derived class inside mylib, the expectedNs would be computed as 'mylib.detail'. This is ISO-correct for relative name lookup within the enclosing namespace.
• ns::detail::Inner<T> (nested relative): scope text in tree-sitter for a nested qualified_identifier would be the left-hand scope node. For a::b::Inner, the scope field yields a::b (the entire prefix), so normalizedQualifier = 'a.b' and expectedNs = classEntry.nsPrefix + '.a.b'. This is likely correct.
• ::ns::detail::Inner<T> (absolute): The scope text would include the leading ::, so normalizedQualifier = '.ns.detail' (leading dot after replace). This would fail both checks and fall through to V2 — documented in the PR as an edge case. Acceptable given the PR explicitly notes this limitation.
• Inline namespaces: V2 behavior unchanged; inline namespace behavior from fix(cpp): dependent-base resolution across nested/inline namespaces (#1634) #1814 not regressed.
• Sibling namespace collisions: V3 is dormant, so sibling suppression still works (0 edges) — not worse than before.

---

AST capture fidelity

extractBaseLookupQualifier correctly handles:

• qualified_identifier → childForFieldName('scope') for detail::Inner (non-template)
• template_type with name child being a qualified_identifier for detail::Inner<T>

Gap: For deeply nested qualifiers like a::b::Inner<T>, the scope field of the outer qualified_identifier is another qualified_identifier (not a type_identifier), so scopeNode.text would return 'a::b' (the full nested text). After replace(/::/g, '.'), this becomes 'a.b'. This appears handled correctly.

Gap: The isBaseDependent stack walk for qualified_identifier nodes now pushes all children recursively, including the scope (namespace path) identifiers. This means if a namespace name happens to match a template parameter name (e.g., template<class T> struct S : T::Inner<T> — truly dependent), it correctly returns true. But for detail::ConcreteBase where detail is just a namespace name coincidentally sharing a template param name — that is genuinely dependent behavior in C++ (dependent type if detail is a template arg). This specific edge case is unlikely to cause false positives in practice because namespace names are not typically template parameter names.

---

Dependent-base storage and collision behavior

Confirmed bug (Finding 2 above): Map<baseName, qualifier> collapses same-name bases.

Additional concern: localClassByName (line 159) for the deriving class uses simple → {nodeId, nsPrefix}, which also uses last-wins if two classes with the same simple name exist in the same file. This was a pre-existing limitation, but it interacts with the new qualifier logic: if two Derived<T> classes exist in different namespaces in the same file, only the last one is resolved. Pre-existing issue, not introduced by this PR.

---

Candidate resolution and false-positive suppression

• Finding 3 above (unique-candidate bypass) is the main concern.
• V3 qualifierMatch uses candidates.find(c => c.nsPrefix === expectedNs || c.nsPrefix === normalizedQualifier). The || allows matching either the fully-resolved prefix OR the bare qualifier. This means detail::Inner inside mylib::Derived could match a global-scope detail::Inner (nsPrefix = 'detail'). Since the intent is relative qualification, this may be too permissive.
• When V3 fires correctly and finds a match, it stops (no fallthrough). This is correct behavior.

---

Regression against #1814/#1564 history

• The V2 one-level-cap logic is unchanged in code and behavior.
• All prior fixtures (cross-ns-pos, cross-ns-neg, cross-ns-inline, cross-ns-deep, sibling-suppress) still pass at their current expected edge counts.
• this-> dependent lookup (feat(cpp): resolve template-body this-> + using ns::name calls in scope resolver #1590) is not affected: isCppDependentBaseMember logic is unchanged.
• No regression confirmed against fix(cpp): dependent-base resolution across nested/inline namespaces (#1634) #1814.

---

Tests and CI

Fixture	Before PR	After PR	Expected by #1815
cpp-two-phase-dependent-base-cross-ns-sibling-suppress	0 edges	0 edges (unchanged)	1 edge
cpp-two-phase-dependent-base-cross-ns-pos	1 edge	1 edge	1 edge
cpp-two-phase-dependent-base-cross-ns-neg	0 edges	0 edges	0 edges
cpp-two-phase-dependent-base-cross-ns-inline	1 edge	1 edge	1 edge
cpp-two-phase-dependent-base-cross-ns-deep	0 edges	0 edges	0 edges

CI is green because the feature is dormant — the test expectation was never updated. No test proves the new runtime path is exercised.

---

Hidden Unicode and hygiene

• git grep scan for bidi/hidden Unicode in both changed files: No hidden/bidi characters found.
• The GitHub UI warning banner may be triggered by non-ASCII in adjacent unrelated files pulled in by the merge commit; the changed files themselves are clean.
• The diff removes one blank line between populateCppDependentBases and isCppDependentBaseMember (cosmetic, harmless).

---

Back-and-forth avoided by verifying

• Confirmed via diff that isBaseDependent stack walk is not over-classifying concrete qualified bases: the walk checks for type_identifier text in templateParams set, not for arbitrary identifiers. A detail::ConcreteBase with no template arguments will have no type_identifier child matching a template parameter in the argument list.
• Confirmed the cross-ns-pos fixture still expects 1 edge (V2 behavior preserved for nested namespace case with single candidate).
• Confirmed the merge commit's unrelated files are from main branch sync, not introduced by the PR author.

---

Open questions (unavoidable)

1. Does the class extractor's buildQualifiedName reliably produce the fully-qualified name in practice? The PR body says it does, but the qualifiedName doesn't reach parsed.localDefs. Is this a parsing-processor contract gap or a per-file extractor ordering issue? The answer determines whether fixing V3 is a one-file change or a multi-phase refactor.

2. Is the || in c.nsPrefix === expectedNs || c.nsPrefix === normalizedQualifier intentional? Matching bare normalizedQualifier allows a global-scope detail::Inner to match when the base specifier says detail::Inner<T> inside mylib::Derived. Is this the intended fallback or a latent false-positive?

---

Final verdict

not production-ready

This PR introduces useful infrastructure — qualifier extraction, storage threading, and isBaseDependent fix for qualified-identifier bases — but it does not satisfy #1815's stated DoD. The V3 qualifier-targeting path is dormant because qualifiedName does not reach parsed.localDefs, so the key fixture (cpp-two-phase-dependent-base-cross-ns-sibling-suppress) still expects 0 edges instead of the required 1. Beyond the dormant feature, there are two structural bugs that block merge independently: (1) Map<baseName, qualifier> silently collapses multiple same-name bases from different qualifiers, and (2) the single-candidate path accepts a mis-qualified candidate in violation of #1564's suppress-over-guess policy. No new tests prove any of the changed code paths are exercised. The smallest path to production-ready is to either complete the parsing-processor.ts plumbing and add qualifying fixtures with exact positive/negative edge assertions, or to explicitly re-scope as infrastructure-only (retitle, remove the #1815 close claim) while fixing the Map-collision bug and the false-positive single-candidate bypass.

[Copilot]

Pull request overview

This PR adds plumbing to carry a dependent base-specifier’s syntactic namespace qualifier (e.g., detail in detail::Inner<T>) from C++ AST capture through storage into the dependent-base resolver, enabling more ISO-correct disambiguation among same-name candidates during two-phase lookup.

Changes:

• Extract and record base-specifier qualifiers during capture (qualified_identifier scope handling + dependent-base detection fix).
• Change dependent-base storage from Set<baseName> to Map<baseName, qualifier> and thread the qualifier through markCppDependentBase.
• Adjust workspace class indexing to deduplicate entries by nodeId, and add a qualifier-based exact-match path (with V2 heuristic fallback).

Reviewed changes

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

File	Description
gitnexus/src/core/ingestion/languages/cpp/two-phase-lookup.ts	Stores dependent base qualifiers and uses them (when available) to disambiguate dependent base resolution; deduplicates class index by nodeId.
gitnexus/src/core/ingestion/languages/cpp/captures.ts	Extracts base-specifier qualifiers and fixes dependent-base detection to handle qualified_identifier bases.

---

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

[magyargergo]

@prajapatisparsh could you please look into claude's findings? 🙏

[prajapatisparsh]

@prajapatisparsh could you please look into claude's findings? 🙏

yes yes .. i was just reading them only