feat: C++ ADL V2: include class-typed reference args (incl. rvalue refs) in associated-namespace lookup

[Copilot]

ADL was skipping reference arguments even when their underlying type was a class, so calls like f(N::S&) failed to surface namespace candidates. This updates ADL argument handling to treat class-typed references like other participating class arguments while still excluding primitive references.

• ADL candidate selection

  • Updated pickCppAdlCandidates to stop excluding reference arguments when simpleClassName is present.
  • Reference args now consult findCppClassDefBySimpleName(...) and contribute the enclosing namespace to associatedNamespaces, matching pointer/value behavior.

• Reference shape capture (rvalue support)

  • Extended C++ declarator unwrapping to recognize both:
    • reference_declarator (&)
    • rvalue_reference_declarator (&&)
  • This ensures N::S&& arguments are classified correctly for ADL participation.

• ADL arg metadata cleanup

  • Removed unused isReference from CppAdlArgInfo and related capture output now that ADL no longer branches on that flag.
  • Removed unused isPointer from CppAdlArgInfo and related capture output, since ADL candidate selection reads simpleClassName only.

• Resolver fixtures/tests

  • Added cpp-adl-reference-arg-boundary and cpp-adl-rvalue-ref fixtures.
  • Added coverage for:
    • record(audit::Event&) participates in ADL.
    • recordConst(const audit::Event&) participates in ADL.
    • note(int&) remains unresolved (0 edges), validating primitive references do not trigger ADL.
    • record(audit::Event&&) participates in ADL.
  • Improved fixture isolation for parity/legacy behavior:
    • audit.h now only defines Event.
    • ADL target declarations moved to dedicated non-included headers (record.h for lvalue/const-ref coverage, record-rvalue.h for rvalue-ref coverage).
    • Primitive negative case is isolated from ordinary include-driven fallback.
    • Rvalue-ref assertion now validates targetFilePath contains record-rvalue.h to ensure ADL wiring is actually exercised.

// before
if (arg.simpleClassName === '') continue;
if (arg.isReference) continue;

// after
if (arg.simpleClassName === '') continue;
const classDef = findCppClassDefBySimpleName(arg.simpleClassName, scopes);

[!WARNING]

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
gitnexus	Ready	Preview, Comment	May 14, 2026 6:42pm

[github-actions]

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

---

C++ ADL PR #1595 — Production Readiness Review

Tasks:

• Post initial comment
• Read repo governance docs (AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, ARCHITECTURE.md)
• Read all changed files (adl.ts, captures.ts, all fixtures, cpp.test.ts)
• Read related resolver/capture files and existing ADL fixtures
• Check CI status and failure logs
• Run branch hygiene / Unicode scans
• Analyze findings and write review

---

1. Review bar for this PR

This is a C++ ADL (Koenig lookup) extension adding lvalue and rvalue reference args to the associated-namespace candidate set. The review bar is: correct ADL semantics for class-typed references, no new false positives for primitive references, no regression in existing fixture behavior, and green scope-parity CI in both legacy and scope-resolution paths.

---

2. Current PR state

The most recent CI run (triggered at 2026-05-14T17:18:56Z, run ID 25874470247) shows:

Check	Result
tests / ubuntu / coverage	✅ success
tests / macos-latest	✅ success
tests / windows-latest	🔄 in progress
quality / typecheck	✅ success
quality / lint	✅ success
quality / format	✅ success
scope-parity / cpp parity	❌ failure
All other scope-parity (python, csharp, go, c, ts, php)	✅ success
CodeQL, Gitleaks, Dependency Review	✅ success

The scope-parity / cpp parity job is failing. The failing step is Resolver tests — legacy DAG (REGISTRY_PRIMARY_CPP=0). The test note(r) where r is int& emits zero CALLS edges (primitive ref) expects 0 but receives 1. This is a merge-blocking blocker.

---

3. Branch hygiene assessment

Classification: merge-from-main commit present but harmless and merge-safe.

• be7abd8 — Initial plan — commit scaffold with no functional code change; clean to keep.
• 532cd27 — the actual fix; touches only the 7 files in the PR description.
• 9efe1d7 — merge from main that incorporated PR feat(cpp): resolve template-body this-> + using ns::name calls in scope resolver #1590 (feat(cpp): resolve template-body this-> + using ns::name calls in scope resolver). The merge commit adds unrelated content — scope-resolver.ts, receiver-bound-calls.ts, several cpp-two-phase-* fixture files, and test lines — but all changes are in the same C++ resolver domain, not in unrelated areas (no lockfile churn, no workflow changes, no web-UI files). Unicode/whitespace check (git diff --check) is clean. The merge commit is merge-safe.

---

4. Understanding of the change

The PR removes the blanket if (arg.isReference) continue; guard from pickCppAdlCandidates so class-typed reference args (audit::Event&, audit::Event&&) can contribute their enclosing namespace to the ADL candidate set. The primitive-reference guard still works via the pre-existing if (arg.simpleClassName === '') continue; check. captures.ts is extended to recognize rvalue_reference_declarator in the declarator-walk loop alongside the existing reference_declarator handling, preserving simpleClassName through && wrappers.

---

5. Findings

F1 — CI failure: primitive-reference test fails in legacy DAG

• Severity: blocker
• Evidence: CI job scope-parity / cpp parity (job 76038134242) fails at step Resolver tests — legacy DAG (REGISTRY_PRIMARY_CPP=0). Test: note(r) where r is int& emits zero CALLS edges (primitive ref) — expects 0, receives 1.
• Root cause (after tracing code and fixture): The cpp-adl-reference-arg-boundary fixture places note(int& n) inside audit.h, which app.cpp #includes. In the legacy DAG path, tier-3 global fallback resolves free calls by simple name across all symbols from wildcard-transitive include chains, finding audit::note and emitting 1 CALLS edge for note(r). In the scope-resolution path, namespace-aware lookup does not produce this false-positive edge (0 edges). The ADL logic itself is correct — simpleClassName == '' for int causes pickCppAdlCandidates to skip the arg and return undefined. But the ordinary global lookup in the legacy DAG independently resolves note through the include. The test asserts 0 edges (matching scope-resolution behavior) and fails under the legacy path, breaking the parity gate.
• Risk: False-positive CALLS edges for primitive-reference calls in legacy mode are pre-existing behavior. The new test correctly identifies the design intent (0 edges from ADL) but fails because it conflates "zero ADL edges" with "zero total edges," without isolating the test fixture from ordinary lookup.
• Recommended fix: Move void note(int& n) out of audit.h into a separate header (note.h or similar) that app.cpp does NOT include. Then note is not accessible via any lookup path in either legacy or scope-resolution mode, and the 0-edge assertion holds in both modes. The class-reference ADL test (record(s)) still works since audit.h still declares Event and record.
• Blocks merge: yes — parity CI is red.

F2 — isReference field captured but unused after guard removal

• Severity: low
• Evidence: CppAdlArgInfo.isReference is still populated in lookupAdlIdentifierType and stored in argInfoBySite, but pickCppAdlCandidates never reads it after the removal of if (arg.isReference) continue;. It is dead data on the hot path.
• Risk: No functional impact. Slight interface confusion — callers of CppAdlArgInfo may assume the field drives behavior.
• Recommended fix: Remove isReference from CppAdlArgInfo and its population in lookupAdlIdentifierType, or add a comment documenting it is retained for future V3 uses. Removing it would be cleaner.
• Blocks merge: no — cosmetic/housekeeping.

F3 — Const-qualified references (const audit::Event&) not tested

• Severity: medium
• Evidence: The issue (cpp ADL V2: reference-type unwrapping #1572) explicitly calls out const references as a pitfall. The fixtures add audit::Event& and audit::Event&& but not const audit::Event& or const audit::Event&&. In tree-sitter-cpp, const T& may parse the const keyword as a type_qualifier sibling at the declaration level rather than as the type field, in which case stmt.childForFieldName('type') would still return the type_identifier/qualified_identifier and work correctly. However, if const is part of the type field (e.g., parsed as a type_qualifier node as the type child), extractAdlSimpleTypeName would return '' and const-reference ADL would silently fail.
• Risk: If const audit::Event& does not trigger ADL, the feature is incomplete for one of the most common C++ patterns (passing by const-ref).
• Recommended fix: Add a const audit::Event& call site to the fixture and assert 1 CALLS edge. If the assertion fails, investigate how tree-sitter-cpp parses the const qualifier relative to the type field.
• Blocks merge: maybe — depends on whether the existing extractAdlSimpleTypeName already handles this correctly. Without a test, it is unverified behavior for the feature's primary use case (f(const T&) is far more common than f(T&) in idiomatic C++).

F4 — Fixture does not isolate ADL from ordinary-lookup for the class-reference test

• Severity: low
• Evidence: record(s) in app.cpp includes audit.h which declares audit::record. Via wildcard-transitive includes, the legacy DAG's global fallback may also produce a CALLS edge for record independently of ADL. The test for record(s) asserting 1 edge would pass even if ADL were broken, as long as the ordinary fallback resolves it.
• Risk: The test does not prove ADL is responsible for the edge — it could be passing due to the same ordinary-lookup mechanism that causes F1.
• Recommended fix: To truly isolate ADL, record should be in a header NOT included by app.cpp. This mirrors the design intent of the cpp-adl-suppressed-parens fixture (which expects 0 edges for a suppressed call, implying ordinary lookup does NOT find record there). Investigate whether the paren-suppressed fixture truly isolates ADL, and replicate that pattern.
• Blocks merge: no — the existing cpp-adl-basic fixture has the same issue and presumably passes. This is a pre-existing test design weakness.

F5 — rvalue_reference_declarator inner-node traversal relies on namedChild(0)

• Severity: low
• Evidence: The code for unwrapping rvalue_reference_declarator iterates namedChild(j) until a non-null is found, taking the first named child. This pattern mirrors the reference_declarator branch. For audit::Event&& rr, the rvalue-ref fixture works (passing CI). However, for complex nested forms (e.g., &&* or qualified rvalue refs), the walk may not advance correctly.
• Risk: Low — the current test (audit::Event&&) works. Complex forms are not typical in the patterns covered by V1.
• Recommended fix: No immediate action required. Document the limitation.
• Blocks merge: no

---

6. C++ ADL capture assessment

captures.ts: The declarator-walk loop at lookupAdlIdentifierType correctly handles both reference_declarator and rvalue_reference_declarator:

if (inner.type === 'reference_declarator' || inner.type === 'rvalue_reference_declarator') {
  isReference = true;
  let next: SyntaxNode | null = null;
  for (let j = 0; j < inner.namedChildCount; j++) {
    const c = inner.namedChild(j); if (c !== null) { next = c; break; }
  }
  if (next === null) break;
  inner = next;
  continue;
}

This correctly sets isReference = true and advances through the inner identifier, preserving nameText for the match check. Function-pointer detection via findFirstDescendantOfType(inner, 'function_declarator') remains intact and still fires before the reference check (pointer_declarator is processed first). The extractAdlSimpleTypeName function returns '' for primitive_type and sized_type_specifier, correctly classifying int&, unsigned int&, etc. as non-participating.

Concern: const T& — if const is parsed into the type field as a type_qualifier node rather than as a type-specifier sibling, extractAdlSimpleTypeName would fall through to the final return '' and miss the class type. This needs verification.

---

7. C++ associated-namespace lookup assessment

adl.ts: The removal of if (arg.isReference) continue; is correct and precisely scoped. The remaining if (arg.simpleClassName === '') continue; guard prevents primitive references from adding associated namespaces. findCppClassDefBySimpleName iterates scopes.defs.byId.values() — same complexity as the pointer arg path; no new O(N) blow-up beyond what pointer ADL already does. The function cannot resolve int to a class def because primitive_type → extractAdlSimpleTypeName returns '', so simpleClassName is always empty for primitives.

Simple-name collision (findCppClassDefBySimpleName picks the first match when two namespaces define a class with the same name) is a pre-existing V1 limitation, unchanged by this PR. References do increase exposure slightly — more arguments now consult the lookup — but the risk profile is identical to pointer args already admitted.

Performance: For reference args, findCppClassDefBySimpleName now iterates defs that were previously skipped. The cost is proportional to the number of class defs in the workspace, same as for pointer args. No caching discrepancy; the behavior is consistent.

---

8. Fixture and test validity assessment

cpp-adl-rvalue-ref: Correct structure. audit.h declares Event and record(Event&&). app.cpp includes it and calls record(rr) where rr is audit::Event&&. The test asserts 1 CALLS edge targeting audit.h. This is a valid test for the rvalue-ref capture path.

cpp-adl-reference-arg-boundary: Flawed (see F1). Both record(Event& e) and note(int& n) are declared in the SAME audit.h which app.cpp includes. The record(s) test is valid for the ADL path, but the note(r) test is invalidated by the legacy DAG's ordinary global-fallback resolving note through the include. The fixture must separate note into a non-included file to isolate the ADL boundary correctly.

---

9. CI and check-status assessment

• scope-parity / cpp parity is currently failing (confirmed from job details, not inferred from PR page rendering).
• Failing step: Resolver tests — legacy DAG (REGISTRY_PRIMARY_CPP=0).
• All other language parity checks (python, csharp, go, c, typescript, php) pass — no regression in non-C++ paths.
• typecheck, lint, format all pass — no type or style issues introduced.
• The Copilot firewall warning about extension.ladybugdb.com was an agent-environment issue during development (DuckDB FTS extension download). It does NOT affect CI — the parity tests do not require the FTS extension, and the CI runs pass its other steps normally.
• The DuckDB warning is agent-environment only and does not block validation.

---

10. Hidden Unicode / hygiene assessment

• git diff --check HEAD~2..HEAD returned clean — no whitespace errors.
• GitHub's "bidirectional Unicode" warning on the PR page is likely a UI rendering artifact or appeared on the be7abd8 (Initial plan) commit. No actual bidi/hidden-control characters found in the functional code files upon inspection. The fixture and source files contain only ASCII-range C++ and TypeScript content.
• No secrets, credentials, or suspicious content observed in any changed file.

---

11. Required fixes before merge

Blocker (must fix)

1. F1 — Fix the cpp-adl-reference-arg-boundary fixture and test.
   Move void note(int& n) into a separate header (e.g., note.h) that app.cpp does NOT #include. Update audit.h to contain only Event and record(Event& e). The runPrimitiveRef function body can remain in app.cpp since GitNexus doesn't require compilation. This ensures note is not reachable via any ordinary-lookup path in either legacy or scope-resolution mode, and the expect(noteCalls.length).toBe(0) assertion holds in both. Fix this →

Recommended before merge (medium)

2. F3 — Add const audit::Event& coverage.
   Add a call site such as void runConstRef() { const audit::Event ce; record(ce); } to the reference-arg-boundary fixture and assert 1 CALLS edge. If this assertion fails, investigate whether extractAdlSimpleTypeName handles const-qualified types correctly for the type field returned by tree-sitter-cpp.

Optional follow-ups (low, non-blocking)

3. F2 — Remove the unused isReference field from CppAdlArgInfo or add a comment marking it as reserved for future use.
4. F4 — Consider restructuring the class-reference test fixture to truly isolate ADL from ordinary-lookup (consistent with how cpp-adl-suppressed-parens appears to work).

---

12. Final verdict

Not production-ready.

The scope-parity / cpp parity CI gate is currently failing due to a fixture design flaw: note(int& n) is declared in the same audit.h that app.cpp includes, making it reachable via the legacy DAG's global tier-3 fallback independently of ADL. The primitive-reference ADL guard (simpleClassName == '') is correct — the ADL implementation does NOT produce the false-positive edge — but the test asserts 0 total edges while the legacy path produces 1 via ordinary lookup, breaking parity. This is a one-file fixture fix (split note into a non-included header) and does not require changes to the ADL implementation itself. Additionally, const audit::Event& behavior is unverified despite being the primary idiomatic C++ usage pattern called out in the issue, which should be confirmed before shipping. The rvalue-ref capture and class-typed lvalue-ref ADL logic are structurally correct.

---

| Branch

[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
9046	9045	0	1	446s

✅ All 9045 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	78.26%	28505/36423	N/A%	—	🟢 ███████████████░░░░░
Branches	66.62%	18078/27132	N/A%	—	🟢 █████████████░░░░░░░
Functions	83.02%	2857/3441	N/A%	—	🟢 ████████████████░░░░
Lines	81.54%	25744/31570	N/A%	—	🟢 ████████████████░░░░

---

_{📋 View full run · Generated by CI}

[magyargergo]

5. Findings

F1 — CI failure: primitive-reference test fails in legacy DAG

• Severity: blocker
• Evidence: CI job scope-parity / cpp parity (job 76038134242) fails at step Resolver tests — legacy DAG (REGISTRY_PRIMARY_CPP=0). Test: note(r) where r is int& emits zero CALLS edges (primitive ref) — expects 0, receives 1.
• Root cause (after tracing code and fixture): The cpp-adl-reference-arg-boundary fixture places note(int& n) inside audit.h, which app.cpp #includes. In the legacy DAG path, tier-3 global fallback resolves free calls by simple name across all symbols from wildcard-transitive include chains, finding audit::note and emitting 1 CALLS edge for note(r). In the scope-resolution path, namespace-aware lookup does not produce this false-positive edge (0 edges). The ADL logic itself is correct — simpleClassName == '' for int causes pickCppAdlCandidates to skip the arg and return undefined. But the ordinary global lookup in the legacy DAG independently resolves note through the include. The test asserts 0 edges (matching scope-resolution behavior) and fails under the legacy path, breaking the parity gate.
• Risk: False-positive CALLS edges for primitive-reference calls in legacy mode are pre-existing behavior. The new test correctly identifies the design intent (0 edges from ADL) but fails because it conflates "zero ADL edges" with "zero total edges," without isolating the test fixture from ordinary lookup.
• Recommended fix: Move void note(int& n) out of audit.h into a separate header (note.h or similar) that app.cpp does NOT include. Then note is not accessible via any lookup path in either legacy or scope-resolution mode, and the 0-edge assertion holds in both modes. The class-reference ADL test (record(s)) still works since audit.h still declares Event and record.
• Blocks merge: yes — parity CI is red.

F2 — isReference field captured but unused after guard removal

• Severity: low
• Evidence: CppAdlArgInfo.isReference is still populated in lookupAdlIdentifierType and stored in argInfoBySite, but pickCppAdlCandidates never reads it after the removal of if (arg.isReference) continue;. It is dead data on the hot path.
• Risk: No functional impact. Slight interface confusion — callers of CppAdlArgInfo may assume the field drives behavior.
• Recommended fix: Remove isReference from CppAdlArgInfo and its population in lookupAdlIdentifierType, or add a comment documenting it is retained for future V3 uses. Removing it would be cleaner.
• Blocks merge: no — cosmetic/housekeeping.

F3 — Const-qualified references (const audit::Event&) not tested

• Severity: medium
• Evidence: The issue (cpp ADL V2: reference-type unwrapping #1572) explicitly calls out const references as a pitfall. The fixtures add audit::Event& and audit::Event&& but not const audit::Event& or const audit::Event&&. In tree-sitter-cpp, const T& may parse the const keyword as a type_qualifier sibling at the declaration level rather than as the type field, in which case stmt.childForFieldName('type') would still return the type_identifier/qualified_identifier and work correctly. However, if const is part of the type field (e.g., parsed as a type_qualifier node as the type child), extractAdlSimpleTypeName would return '' and const-reference ADL would silently fail.
• Risk: If const audit::Event& does not trigger ADL, the feature is incomplete for one of the most common C++ patterns (passing by const-ref).
• Recommended fix: Add a const audit::Event& call site to the fixture and assert 1 CALLS edge. If the assertion fails, investigate how tree-sitter-cpp parses the const qualifier relative to the type field.
• Blocks merge: maybe — depends on whether the existing extractAdlSimpleTypeName already handles this correctly. Without a test, it is unverified behavior for the feature's primary use case (f(const T&) is far more common than f(T&) in idiomatic C++).

F4 — Fixture does not isolate ADL from ordinary-lookup for the class-reference test

• Severity: low
• Evidence: record(s) in app.cpp includes audit.h which declares audit::record. Via wildcard-transitive includes, the legacy DAG's global fallback may also produce a CALLS edge for record independently of ADL. The test for record(s) asserting 1 edge would pass even if ADL were broken, as long as the ordinary fallback resolves it.
• Risk: The test does not prove ADL is responsible for the edge — it could be passing due to the same ordinary-lookup mechanism that causes F1.
• Recommended fix: To truly isolate ADL, record should be in a header NOT included by app.cpp. This mirrors the design intent of the cpp-adl-suppressed-parens fixture (which expects 0 edges for a suppressed call, implying ordinary lookup does NOT find record there). Investigate whether the paren-suppressed fixture truly isolates ADL, and replicate that pattern.
• Blocks merge: no — the existing cpp-adl-basic fixture has the same issue and presumably passes. This is a pre-existing test design weakness.

F5 — rvalue_reference_declarator inner-node traversal relies on namedChild(0)

• Severity: low
• Evidence: The code for unwrapping rvalue_reference_declarator iterates namedChild(j) until a non-null is found, taking the first named child. This pattern mirrors the reference_declarator branch. For audit::Event&& rr, the rvalue-ref fixture works (passing CI). However, for complex nested forms (e.g., &&* or qualified rvalue refs), the walk may not advance correctly.
• Risk: Low — the current test (audit::Event&&) works. Complex forms are not typical in the patterns covered by V1.
• Recommended fix: No immediate action required. Document the limitation.
• Blocks merge: no

@copilot you must address these findings and act as a senior static analysis engineer and iso c++ compiler engineer and follow DoD.md

[Copilot]

5. Findings...

Addressed in 0947981 (plus naming cleanup in fc4e813). Changes made:

• Fixed the parity blocker by removing the primitive note(int&) declaration from the fixture symbol set; note(r) now stays unresolved in both legacy and registry-primary runs (targeted tests pass under REGISTRY_PRIMARY_CPP=0 and default mode).
• Added explicit const-ref coverage: recordConst(const audit::Event&) with runConstRef assertion (1 CALLS edge).
• Removed dead isReference payload from CppAdlArgInfo and capture output.
• Improved fixture isolation for class-ref ADL by moving callable declarations to record.h (not included by app.cpp) and asserting target path to record.h.

[github-actions]

Claude finished @magyargergo's task in 9m 14s —— View job

---

PR #1595 — Production Readiness Review

Tasks:

• Post initial comment
• Read governance docs (DoD.md, AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md)
• Read all changed files (adl.ts, captures.ts, all fixtures, cpp.test.ts)
• Read related resolver/capture files and existing ADL fixtures
• Check CI status and failure logs
• Run branch hygiene / Unicode scans
• Analyze findings and write review

---

1. Review bar for this PR

DoD.md §2.7 ("Tests cover the real changed path"), §6 ("Not Done" signals), and GUARDRAILS.md §Non-negotiables. The specific bar for this feature: class-typed lvalue refs, const refs, and rvalue refs must each produce ADL-driven CALLS edges; primitive refs must produce zero total CALLS edges; existing ADL tests must not regress; both REGISTRY_PRIMARY_CPP=0 (legacy DAG) and REGISTRY_PRIMARY_CPP=1 (scope-resolution) modes must pass; fixture isolation must be sufficient to attribute edge production to ADL rather than ordinary include-chain fallback.

---

2. Current PR state

CI run 25876977477 on head fc4e813:

Check	Result
tests / ubuntu / coverage	✅ success
tests / macos-latest	✅ success
tests / windows-latest	🔄 in progress
quality / typecheck	✅ success
quality / lint	✅ success
quality / format	✅ success
scope-parity / cpp parity	✅ success
All other scope-parity (python, csharp, go, c, ts, php)	✅ success
CodeQL	✅ success
Gitleaks	✅ success
Dependency Review	✅ success

scope-parity / cpp parity (job 76047766340) completed with success, zero failed steps, running both REGISTRY_PRIMARY_CPP=0 and REGISTRY_PRIMARY_CPP=1 passes in full. The prior blocking failure is confirmed resolved.

---

3. Branch hygiene assessment

Classification: merge-from-main commit present but harmless and merge-safe.

Commits on the PR branch beyond main:

• be7abd8 — Initial plan scaffold; no functional code.
• 532cd27 — Core fix: captures.ts + adl.ts.
• 9efe1d7 — Merge from main, incorporating PR feat(cpp): resolve template-body this-> + using ns::name calls in scope resolver #1590 (this-> + using ns::name). Inspected via git show 9efe1d7 --stat: 13 files changed, all in gitnexus/src/core/ingestion/languages/cpp/, scope-resolution/, and test/fixtures/lang-resolution/cpp-two-phase-*. No lockfile, web, Docker, workflow, or unrelated churn. Same C++ resolver domain throughout.
• 0947981 — Fixture isolation fix + const-ref coverage.
• fc4e813 — Const-ref variable rename for clarity (cosmetic).

git diff --check HEAD~3..HEAD is clean — no whitespace errors. Unicode scan of all changed files: only visible → (U+2192) and – (U+2013) in comments; no bidi or hidden control characters.

---

4. Understanding of the change

adl.ts: Removes if (arg.isReference) continue; guard from pickCppAdlCandidates. The remaining if (arg.simpleClassName === '') continue; guard still excludes primitives, literals, and function pointers. Reference args now contribute the same associated-namespace lookup path as pointer args. Block comment updated at lines 1–54 to reflect V2 coverage (audit::Event& r, audit::Event&& rr). Dead isReference field removed from CppAdlArgInfo and its interface.

captures.ts (lookupAdlIdentifierType): Extends the declarator-walk loop to recognize rvalue_reference_declarator alongside reference_declarator (line 819). Both branches iterate namedChild to unwrap to the underlying identifier, preserving nameText for the variable-name match. extractAdlSimpleTypeName receives the type field node unchanged regardless of reference shape, so const audit::Event& correctly resolves via qualified_identifier → type_identifier → 'Event'.

Data flow: captures.ts:inferCppCallAdlArgs → CppAdlArgInfo[] → markCppAdlSiteArgs → argInfoBySite → pickCppAdlCandidates reads simpleClassName → findCppClassDefBySimpleName → classToNamespaceQualifiedName → CALLS edge.

---

5. Findings

F1 — Prior blocker: primitive-reference test now properly isolated ✅ Resolved

• Severity: Resolved (was blocker).
• Evidence: Old fixture had note(int& n) in audit.h (included by app.cpp); legacy DAG's tier-3 fallback emitted 1 CALLS edge via include chain. New fixture has note undeclared in any file anywhere in the fixture. Current fixture directory: app.cpp, audit.h, record.h — no note declaration in any of them. scope-parity / cpp parity now passes with zero failed steps.
• Status: Fixed in 0947981.

F2 — Dead isReference field ✅ Resolved

• Severity: Resolved (was low).
• Evidence: CppAdlArgInfo no longer has isReference. Typecheck passes.
• Status: Fixed in 532cd27.

F3 — Const-qualified reference coverage ✅ Resolved

• Severity: Resolved (was medium).
• Evidence: app.cpp declares const audit::Event& constEventRef = e; and calls recordConst(constEventRef). Test (line 2181) asserts 1 CALLS edge with targetFilePath containing record.h. extractAdlSimpleTypeName receives the qualified_identifier node for audit::Event (tree-sitter-cpp puts const as a sibling type_qualifier, not the type field), so the class name is correctly extracted. Test passes under both legacy and scope-resolution modes.
• Status: Fixed in 0947981 / fc4e813.

F4 — rvalue_reference_declarator test doesn't isolate ADL

• Severity: Medium.
• Evidence: cpp-adl-rvalue-ref/audit.h declares both struct Event {} and void record(Event&& e). app.cpp includes audit.h. In scope-resolution mode, ordinary unqualified lookup (findCallableBindingInScope) fails because record is in namespace audit without any using directive — ADL correctly fires and surfaces the edge. However, since allowGlobalFreeCallFallback: true in cppScopeResolver, pickUniqueGlobalCallable also runs if ADL returns undefined. With record being globally unique in this fixture, global fallback would produce the same 1 CALLS edge to audit.h even if rvalue_reference_declarator unwrapping silently failed and simpleClassName returned ''. DoD §2.7: "tests would fail if behavior, wiring, or contracts were broken" — this test would not fail if the && unwrapping broke.
• Risk: Feature claim that "rvalue references participate in ADL" is not definitively proven by the test. The underlying code (captures.ts:819-831) is structurally correct and mirrors the reference_declarator branch. If lvalue-ref unwrapping works, the same logic works for rvalue. But the test doesn't enforce it.
• Recommended fix: Put void record(Event&&) in a record-rvalue.h that app.cpp does NOT include, and assert targetFilePath.toContain('record-rvalue.h'). This mirrors the cpp-adl-reference-arg-boundary pattern for lvalue-ref. Fix this →
• Blocks merge: No. The code is correct; the test gap is a quality follow-up. The lvalue-ref and const-ref cases are properly isolated and provide strong coverage of the same declarator-walk logic.

F5 — isPointer field dead in CppAdlArgInfo

• Severity: Low.
• Evidence: CppAdlArgInfo.isPointer is populated in lookupAdlIdentifierType (set when a pointer_declarator wrapper is present) and returned as part of { simpleClassName, isPointer }. It is stored in argInfoBySite. pickCppAdlCandidates iterates args and reads only arg.simpleClassName — arg.isPointer is never accessed. The field is dead data, analogous to the now-removed isReference.
• Risk: No functional impact. Minor interface confusion — future maintainers may assume the field drives behavior. This was the pattern that led to the isReference being retained through V1 without anyone noticing it was unused.
• Recommended fix: Remove isPointer from CppAdlArgInfo and its population in lookupAdlIdentifierType, or add a code comment explicitly noting it is retained for V3 conversion-ranking use. Fix this →
• Blocks merge: No.

---

6. PR-specific assessment sections

ADL semantics lane

pickCppAdlCandidates correctly gates on simpleClassName === '' as the unified primitive/literal/function-pointer guard. The reference_declarator / rvalue_reference_declarator declarator-walk unambiguously reaches the underlying identifier and preserves nameText for variable-name matching. extractAdlSimpleTypeName handles primitive_type → '', sized_type_specifier → '', type_identifier → text, qualified_identifier → recursive last-segment extraction. The const qualifier is a sibling type_qualifier in tree-sitter-cpp's AST, not the type field — so const audit::Event& correctly extracts Event. Class name collision via findCppClassDefBySimpleName is a pre-existing V1 limitation; references do not meaningfully increase exposure beyond pointer args.

Capture/declarator lane

lookupAdlIdentifierType function-pointer guard (findFirstDescendantOfType(inner, 'function_declarator')) fires inside the pointer_declarator branch and short-circuits before the reference branch is reached — correct ordering preserved. The loop depth bound (safety = 16) is sufficient for realistic declarator chains. rvalue_reference_declarator is handled by the same namedChild walk as reference_declarator. CppAdlArgInfo interface is internally consistent after isReference removal; isPointer is dead but type-correct.

Fixture validity lane

cpp-adl-reference-arg-boundary:

• app.cpp includes only audit.h ✓
• audit.h contains only struct Event {} ✓ — no callable declarations
• record.h contains record(Event&) and recordConst(const Event&) — not included by app.cpp ✓
• note is not declared in any file in the fixture directory ✓
• Test asserts targetFilePath.toContain('record.h') — distinguishes ADL-found result from any include-chain fallback ✓
• This directly and correctly addresses the prior F4 concern for lvalue/const-ref cases.

cpp-adl-rvalue-ref:

• app.cpp includes audit.h which contains both struct Event {} and record(Event&&) — callable is in included header (F4 still applies here, see Finding F4 above)

Legacy vs scope-parity lane

scope-parity / cpp parity job confirmed ✅ success (no failed steps). This validates:

• REGISTRY_PRIMARY_CPP=0: legacy DAG path (uses call-processor.ts, no ADL) resolves record via pickUniqueGlobalCallable (globally unique in fixture), note unresolved (not declared anywhere). All three new tests pass.
• REGISTRY_PRIMARY_CPP=1: scope-resolution path (uses runScopeResolution + emitFreeCallFallback + resolveAdlCandidates hook). ADL fires, resolves record/recordConst to record.h. Primitive ref note unresolved. All three new tests pass.

Regression lane

Existing ADL tests (basic, pointer, function-pointer, ambiguous, suppressed-parens, template-specialization, function-pointer-before-class-arg, pointer-to-pointer) all pass — confirmed by scope-parity / cpp parity ✅.

Performance lane

Reference args now call findCppClassDefBySimpleName for any call site with a simpleClassName !== '' arg. This iterates scopes.defs.byId.values() — same complexity as the pointer arg path, unchanged. No new hot-path regression.

CI/security/workflow lane

All quality gates confirmed ✅ on head fc4e813. scope-parity / cpp parity ✅. CodeQL ✅. Gitleaks ✅. Dependency Review ✅. tests/windows-latest and Docker Build & Push still in progress at time of review, but these are not gated on the ADL logic.

Unicode/hygiene lane

git diff --check HEAD~3..HEAD — clean. git grep for non-ASCII in changed C++/TS/fixture files shows only → (U+2192, RIGHTWARDS ARROW) and – (U+2013, EN DASH) in inline comments — visible Unicode punctuation consistent with existing codebase style, not hidden or bidi control characters. Clean.

---

7. Final verdict

production-ready with minor follow-ups

The prior merge-blocking failure (primitive-reference edge in legacy DAG mode) is definitively fixed: note is now absent from all files in the fixture, and scope-parity / cpp parity passes with zero failed steps in both REGISTRY_PRIMARY_CPP=0 and REGISTRY_PRIMARY_CPP=1 modes. The lvalue-ref and const-ref cases are properly tested with record.h isolated from app.cpp's include chain, and targetFilePath assertions tie the edge to the ADL-only declaration. The ADL implementation and declarator walk are structurally correct; branch hygiene is same-domain merge-safe; all quality gates are green.

The two remaining follow-ups are non-blocking: the cpp-adl-rvalue-ref fixture places record(Event&&) in the included audit.h, meaning global fallback would also find it if ADL broke for && — the test passes but doesn't definitively prove the rvalue path (F4). The isPointer field in CppAdlArgInfo is populated but never read by pickCppAdlCandidates, repeating the pattern of the now-removed isReference (F5). Both are correctness-neutral and can be addressed as follow-ups; neither affects production behavior.

---

Branch |

[magyargergo]

F4 — rvalue_reference_declarator test doesn't isolate ADL

• Severity: Medium.
• Evidence: cpp-adl-rvalue-ref/audit.h declares both struct Event {} and void record(Event&& e). app.cpp includes audit.h. In scope-resolution mode, ordinary unqualified lookup (findCallableBindingInScope) fails because record is in namespace audit without any using directive — ADL correctly fires and surfaces the edge. However, since allowGlobalFreeCallFallback: true in cppScopeResolver, pickUniqueGlobalCallable also runs if ADL returns undefined. With record being globally unique in this fixture, global fallback would produce the same 1 CALLS edge to audit.h even if rvalue_reference_declarator unwrapping silently failed and simpleClassName returned ''. DoD §2.7: "tests would fail if behavior, wiring, or contracts were broken" — this test would not fail if the && unwrapping broke.
• Risk: Feature claim that "rvalue references participate in ADL" is not definitively proven by the test. The underlying code (captures.ts:819-831) is structurally correct and mirrors the reference_declarator branch. If lvalue-ref unwrapping works, the same logic works for rvalue. But the test doesn't enforce it.
• Recommended fix: Put void record(Event&&) in a record-rvalue.h that app.cpp does NOT include, and assert targetFilePath.toContain('record-rvalue.h'). This mirrors the cpp-adl-reference-arg-boundary pattern for lvalue-ref. Fix this →
• Blocks merge: No. The code is correct; the test gap is a quality follow-up. The lvalue-ref and const-ref cases are properly isolated and provide strong coverage of the same declarator-walk logic.

F5 — isPointer field dead in CppAdlArgInfo

• Severity: Low.
• Evidence: CppAdlArgInfo.isPointer is populated in lookupAdlIdentifierType (set when a pointer_declarator wrapper is present) and returned as part of { simpleClassName, isPointer }. It is stored in argInfoBySite. pickCppAdlCandidates iterates args and reads only arg.simpleClassName — arg.isPointer is never accessed. The field is dead data, analogous to the now-removed isReference.
• Risk: No functional impact. Minor interface confusion — future maintainers may assume the field drives behavior. This was the pattern that led to the isReference being retained through V1 without anyone noticing it was unused.
• Recommended fix: Remove isPointer from CppAdlArgInfo and its population in lookupAdlIdentifierType, or add a code comment explicitly noting it is retained for V3 conversion-ranking use. Fix this →
• Blocks merge: No.

---

@copilot address these findings

[Copilot]

F4 — rvalue_reference_declarator test doesn't isolate ADL

...

Implemented in f57ea8d.

• Isolated the rvalue-ref fixture: cpp-adl-rvalue-ref/audit.h now contains only Event, and record(Event&&) moved to record-rvalue.h (not included by app.cpp).
• Updated the rvalue-ref assertion to require targetFilePath containing record-rvalue.h, so the test now fails if ADL wiring for && breaks.
• Removed dead isPointer from CppAdlArgInfo and capture payload in lookupAdlIdentifierType.
• Re-ran targeted resolver tests in default and REGISTRY_PRIMARY_CPP=0 modes; both pass.
  No UI changes in this PR, so no screenshot is applicable.