feat(cpp): add template partial ordering

[azizur100389]

Fixes #1635

Summary

• add conservative C++ template partial-ordering tie-breaks for overload narrowing
• preserve C++ parameter shape sidecars through method extraction and graph-node lookup so template overload nodes remain distinct
• add pointer-vs-value and incomparable-shape fixtures plus unit guards
• harden partial ordering to suppress when a surviving candidate cannot participate, and keep ref/value shapes ambiguous

Reindex note

• C++ graphs containing template pointer/ref overloads should be reindexed after this change so shape-aware callable node IDs are rebuilt.

Tests

• npm run build
• TEMP=C:\tmp TMP=C:\tmp npm test
• REGISTRY_PRIMARY_CPP=1 npx vitest run test/integration/resolvers/cpp.test.ts
• REGISTRY_PRIMARY_CPP=0 npx vitest run test/integration/resolvers/cpp.test.ts
• npx vitest run test/unit/scope-resolution/cpp/cpp-overload-ranking.test.ts
• TEMP=C:\tmp TMP=C:\tmp npx vitest run test/unit/group/http-route-extractor.test.ts -t " FastAPI.*include_router\

[vercel]

@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]

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
10181	10174	0	7	637s

✅ All 10174 tests passed

7 test(s) skipped — expand for details

• COBOL pipeline benchmark > scales with file count
• C# pipeline benchmark > scales with file count — namespaces spread across the solution
• C# pipeline benchmark > scales with file count — all types in one (global) namespace bucket
• PHP pipeline benchmark > scales with file count (workers enabled)
• Ruby 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.96%	35787/44755	79.48%	📈 +0.5	🟢 ███████████████░░░░░
Branches	68.61%	22859/33316	68%	📈 +0.6	🟢 █████████████░░░░░░░
Functions	84.94%	3680/4332	84.56%	📈 +0.4	🟢 ████████████████░░░░
Lines	83.51%	32229/38591	82.97%	📈 +0.5	🟢 ████████████████░░░░

---

_{📋 View full run · Generated by CI}

[github-actions]

Claude finished @magyargergo's task in 10m 49s —— View job

---

Adversarial Production-Readiness Review: PR #1885 feat(cpp): add template partial ordering

Tasks

• Initialize review and post task list
• Branch hygiene & Unicode hygiene checks
• Read repo docs (DoD.md, AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md)
• Read all 13 changed files
• Read related files (overload-narrowing pipeline, conversion-rank, SFINAE, graph lookup, worker)
• Analyze: ISO C++ partial-ordering correctness
• Analyze: Conservative suppression / false-positive risk
• Analyze: Graph identity and lookup compatibility
• Analyze: Sidecar propagation through runtime path
• Analyze: Resolver integration order
• Analyze: Test adequacy and parity
• Compile findings

---

Review bar for this PR

This PR modifies C++ template overload resolution inside the call-resolution DAG. A wrong CALLS edge emitted by the partial-ordering step is worse than a suppressed edge; the project's explicit posture in #1564 is "suppress rather than guess." The bar is therefore: every narrowing action must be proven correct against all co-surviving candidates, the sidecar propagation path must be end-to-end tested, graph IDs must not silently change for existing nodes beyond the minimum scope needed, and all ISO C++ semantics claimed must be accurate within the stated approximation.

---

Problem being solved

Issue #1635: when both template<class T> void f(T) and template<class T> void f(T*) are viable overloads for f(p) (where p is a pointer), GitNexus v1 left both alive and either picked arbitrarily or suppressed. The issue DoD asks for a syntactic closed-table comparator (T* > T, const T& > T) rather than full [temp.func.order] deduction, a positive fixture resolving exactly one edge to f(T*), and a negative fixture producing zero edges for genuinely incomparable pairs.

---

Current PR state

Open. Head commit: 57a0d79 (merge-from-main on top of feature commit a7e6e9b). CI: all checks pass (10055 tests, 0 failures). No reviewers assigned. Vercel authorization pending — informational only.

---

Merge status and mergeability

review blocked — three confirmed logic issues block merge (see Findings 1, 2, 3 below). CI green does not reflect these because they are not tested. No branch conflicts observed.

---

Repository history considered

Directly inspected: overload-narrowing.ts, method-props.ts, node-lookup.ts, ids.ts, arity-metadata.ts, conversion-rank.ts, c-cpp.ts, method-types.ts, parsing-processor.ts, parse-worker.ts, fixture files, cpp.test.ts, helpers.ts, cpp-overload-ranking.test.ts, DoD.md, GUARDRAILS.md. Referenced: #1635, #1564, #1606, #1623, #1630, #1637.

---

Branch hygiene assessment

merge-from-main commit present but harmless and merge-safe

git diff HEAD~2..HEAD reveals 30+ files — the 18 additional files (FastAPI router bindings, Java HTTP patterns, test utilities) all originate from the merge parent 57a0d79 bringing in unrelated main-branch commits. The feature commit a7e6e9b is correctly scoped to 13 files. No conflict artifacts are visible. The merge commit is a routine sync and does not pollute the C++ feature code, but it muddies the PR review surface.

---

Understanding of the change

Sidecar flow: classifyCppParameterType in c-cpp.ts populates ParameterInfo.typeClass for each C++ parameter during tree-sitter extraction. buildMethodProps accumulates these into parameterTypeClasses[] on the graph node props, but only when all parameters have a typeClass (length equality guard at method-props.ts:203). parameterShapeIdTag in parsing-processor.ts and parse-worker.ts computes a shape suffix (e.g., ~shape:T:none:pointer:1) for Function/Method node IDs, but only when at least one parameter is an uppercase-starting type token (template placeholder heuristic) AND at least one has a non-value shape. Non-C++ functions and concrete C++ functions both produce an empty tag and are unaffected. The shape tag is registered as an additional lookup key in buildGraphNodeLookup alongside the existing param-types key and constraint key. resolveDefGraphId tries the shape key before the param-types key. In overload narrowing, step 4d (rankByTemplatePartialOrdering) runs after exact-type matching, conversion ranking, and constraint filtering, and only when argTypeClasses is provided and result.length > 1.

---

Findings

---

Finding 1 — rankByTemplatePartialOrdering silently drops unrankable candidates and can emit false-positive CALLS edges

• Severity: HIGH
• Risk: The function builds a viable list from candidates by excluding any candidate where parameterTypeClasses is undefined, where any slot's templatePartialOrderSlotRank returns undefined (i.e., the slot has a non-placeholder paramType), or where no template placeholder was seen (sawTemplateSlot=false). If viable.length <= 1, it returns viable.map(v => v.def). The problem: excluded candidates were not proven dominated — they were silently skipped. If exactly one candidate enters viable while a legitimate competitor was excluded (e.g., because it has a mixed-template+concrete slot), a false-positive CALLS edge is emitted.
• Evidence: overload-narrowing.ts:366–406. Concrete scenario: f(T, int) vs f(T, T*) both survive arity filtering and conversion ranking (both have slot 0 = 'T' which gets Infinity from cppConversionRank, so both survive in candidates). In step 4d: f(T, int) has slot 1 paramType = 'int', isTemplatePlaceholder('int') = false → templatePartialOrderSlotRank returns undefined → ok=false → excluded. f(T, T*) has all-placeholder slots → viable. viable = [f(T,T*)], viable.length = 1 → returns [f(T,T*)]. But f(T, int) was not dominated; it could be the better match for a concrete int argument.
• Evidence to check: There is no test with a mixed-template+concrete-slot overload pair. The unit tests in cpp-overload-ranking.test.ts:154–192 only cover pairs where ALL parameter slots are template placeholders.
• Recommended fix: Change the viable.length <= 1 shortcut: if viable.length < candidates.length (some candidates were excluded), do not return viable[0] — return [] so the caller suppresses. Only apply partial ordering when every candidate in candidates participated in the ranking. Fix this →
• Blocks merge: yes

---

Finding 2 — const T& > T claim is incorrect per ISO C++ [temp.func.order]

• Severity: MEDIUM
• Risk: The docstring at overload-narrowing.ts:354–358 says "const T& / T& are more specialized than T for non-pointer args." This is wrong. In ISO C++ [temp.func.order], f(T) and f(const T&) have equal specialization — neither dominates the other under the partial ordering algorithm (both directions of deduction succeed). Real compilers resolve between them via ICS ranking, not partial ordering. A call site that is genuinely ambiguous between f(T) and f(const T&) will be given a CALLS edge to f(const T&) by this implementation, which is a false positive.
• Evidence: overload-narrowing.ts:421–424: if (isReferenceShape(paramClass)) return isPointerShape(argClass) ? undefined : 2; — returns rank 2 for any reference-shaped parameter with a non-pointer argument. The unit test at cpp-overload-ranking.test.ts:166–175 ("selects const T& over T for value arguments") asserts this behavior and would pass, but it asserts behavior that contradicts ISO C++ semantics.
• Evidence to check: ISO C++ [temp.func.order]/4-6. For f(T) vs f(const T&): synthesized-A matching against B's parameter pattern const T& deduces T = const T& (succeeds). Synthesized-B matching against A's pattern T deduces T = T (succeeds). Both deductions succeed → equal specialization → no partial ordering winner. ISO C++ would give an ambiguous overload error here.
• Recommended fix: Either remove the isReferenceShape branch (making ref/value cases suppress, which is conservative and correct under the project's bar), or document explicitly that this departs from ISO semantics and only applies when the issue DoD explicitly requests it. Add a test proving that f(T) vs f(const T&) with a non-pointer arg does NOT pick a winner.
• Blocks merge: yes — the current implementation emits false-positive edges for a class of calls that are genuinely ambiguous in ISO C++.

---

Finding 3 — Unconditional lookup.set(shapeKey, ...) vs conditional pKey is an inconsistency that risks overwrite

• Severity: MEDIUM
• Risk: In node-lookup.ts:120, lookup.set(shapeKey, node.id) is unconditional. The nearby pKey registration at line 113 uses if (!lookup.has(pKey)). If two function nodes in the same file produce the same shape key (e.g., two template overloads with identical shape tags from duplicate declarations or a hash collision in the tag), the second node silently overwrites the first in the lookup. A CALLS edge then routes to the wrong overload.
• Evidence: node-lookup.ts:115–121. parameterShapeIdTag at method-props.ts:177–182 produces the tag deterministically from parameterTypes+parameterTypeClasses, so identical overloads in the same file (e.g., from a split declaration+definition pair) would produce identical tags.
• Recommended fix: Change lookup.set(shapeKey, node.id) to if (!lookup.has(shapeKey)) lookup.set(shapeKey, node.id) for consistency with pKey. This preserves first-wins behavior and is safe since shape keys are already more specific than qualified keys.
• Blocks merge: maybe — depends on whether duplicate shape keys can occur in practice. Recommend fixing for consistency and safety before merge.

---

Finding 4 — No test exercises the mixed-template+concrete slot scenario

• Severity: MEDIUM
• Risk: The only unit tests for partial ordering in cpp-overload-ranking.test.ts:154–192 cover: T* vs T (pure pointer vs pure value), const T& vs T (pure ref vs pure value), and crossed T*/T vs T/T* (both all-placeholder). No test covers a candidate pair where one overload has mixed placeholder+concrete slots. The false-positive scenario in Finding 1 is not caught by any existing test.
• Recommended fix: Add a unit test: f(T, int) vs f(T, T*) with a pointer argument for both slots — partial ordering should return [] (suppress) rather than picking f(T, T*). Also add: a test where one candidate has parameterTypeClasses = undefined while the other is rankable — should suppress.
• Blocks merge: yes, as a companion to Finding 1.

---

Finding 5 — isTemplatePlaceholder uppercase heuristic is imprecise but safe in end-to-end flow

• Severity: LOW (after analysis)
• Risk: isTemplatePlaceholder uses /^[A-Z]\w*$/ which matches any uppercase-starting identifier, not just known template parameters. Concrete class names like Widget would be misidentified. However, analysis of the full step-4 path shows this is safe in practice: for concrete non-template overloads, exactTypeSlotMatches at step 4 separates value vs pointer overloads via shape sidecars before step 4d is reached. The heuristic does cause missed detections for lowercase template parameters (e.g., template<class value_type>), which are false negatives — acceptable per project policy.
• Recommended fix: Document the uppercase-only limitation explicitly in the function comment and add a test confirming lowercase template parameters safely suppress (false negative) rather than misroute.
• Blocks merge: no, but document the limitation.

---

Finding 6 — Graph ID change for C++ template-with-shape overloads; no reindex note

• Severity: LOW
• Risk: Adding parameterShapeIdTag to Function/Method node IDs changes persisted graph IDs for C++ template overloads with pointer/ref shapes. Existing cached indexes from before this PR will have different IDs for these nodes. DoD.md §2.4 requires persisted data changes to be "backward-compatible or accompanied by a documented migration / reindex path." The PR description mentions no reindex requirement.
• Recommended fix: Add a note to the PR description stating that existing C++ graphs with template pointer/ref overloads require reindex after this change. No migration tooling needed — a plain npx gitnexus analyze rebuilds IDs.
• Blocks merge: maybe, depending on whether any deployed indexes are affected.

---

Finding 7 — Unicode hygiene: inconclusive

• Severity: INFO
• Risk: The GitHub UI reportedly shows hidden/bidirectional Unicode warnings. Tool restrictions prevented running the full scan. No visible hidden Unicode found in the key files I read directly. The warning may originate from the merge commit's unrelated file changes (FastAPI/Java) rather than the C++ feature code.
• Recommended fix: Run git show a7e6e9b | LC_ALL=C grep -P '[\x{200B}-\x{200D}\x{FEFF}\x{202A}-\x{202E}\x{2066}-\x{2069}]' locally and confirm clean.
• Blocks merge: no if scan is clean.

---

PR-specific assessment sections

ISO C++ partial-ordering correctness

T* > T for pointer arguments (ranks 3 vs 1): correct — ISO C++ does consider f(T*) more specialized than f(T). T& > T for lvalue-ref: incorrect — ISO C++ does not order f(T&) above f(T) via partial ordering (both deductions succeed). const T& > T: incorrect for the same reason. The pointer case (the primary claimed feature) is correct; the reference cases overreach.

Conservative suppression / false-positive risk

The existing conversion-rank step (4b) with cppConversionRank provides strong protection: template placeholders ('T') get Infinity rank for any concrete argType, while concrete overloads get 0–4, naturally separating them before step 4d is reached. This means the most dangerous false-positive scenario (template overload wins over a concrete one) is largely protected by step 4b. The remaining confirmed false-positive scenario is mixed-template+concrete-slot overloads (Finding 1), which escape both step 4 and 4b but are silently excluded from viable in step 4d.

Graph identity and lookup compatibility

parameterShapeIdTag is correctly scoped: returns non-empty only for functions/methods with at least one uppercase-starting paramType and at least one non-value shape. Non-C++ functions and concrete C++ functions get the empty string → no ID change. The stripCallableDisambiguatorTags regex (/~shape:.*$/) is greedy and will strip everything after the first ~shape:, including any subsequent tags — this is likely safe since shape tags are always appended last, but should be verified if tag ordering ever changes. The resolveDefGraphId correctly tries shape key → param-types key → qualified key → simple key in order. However, Finding 3's unconditional overwrite of shape keys in buildGraphNodeLookup is a correctness concern.

Sidecar propagation through runtime path

parameterTypeClasses flows correctly: classifyCppParameterType → ParameterInfo.typeClass → buildMethodProps (conditional on all params having typeClass) → parse-worker.ts SymbolData type (line 138) → parse-worker.ts:1974 graph node props → SymbolDefinition.parameterTypeClasses → narrowOverloadCandidates. The condition in buildMethodProps (typeClasses.length === info.parameters.length) correctly excludes partial typeClass coverage. The integration tests use runPipelineFromRepo which exercises the real worker path. Sidecar propagation is correctly wired end-to-end.

Resolver integration order

Step 4d runs after exact-type match (4), conversion rank (4b), and constraint filter (4c). This is the correct phase for a tie-breaker. The condition result.length > 1 && argTypes !== undefined && argTypes.length > 0 correctly gates the step. The early return if (argTypeClasses === undefined) return [] correctly suppresses when no call-site shape info is available. Integration order is sound.

Test adequacy and parity

Positive integration test: asserts calls.length === 1 and target.properties.startLine === 5 (exact line). This is a good assertion. Correctly listed in LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.cpp. Negative (tied) integration test: asserts calls.length === 0. NOT in expected failures → expected to pass in legacy mode. Analysis confirms this is correct: legacy mode uses isOverloadAmbiguousAfterNormalization which detects both pick(T*,T) and pick(T,T*) sharing parameterTypes=['T','T'] and suppresses. However: no unit test covers mixed-template+concrete-slot candidates and no unit test covers const-ref vs value partial ordering being intentionally suppressed.

CI/checks and branch hygiene

CI passes. Branch is clean from the feature perspective. Merge commit is unrelated and harmless. DoD §2.9 requires a reindex note for persisted ID changes — not present in PR description.

---

Back-and-forth avoided by verifying

1. cppConversionRank for template placeholder 'T': returns Infinity for any non-matching argType (no special-case for 'T'). This means template candidates get excluded from conversion-ranked viable, naturally falling back to candidates for step 4d — confirming the integration path reaches step 4d with both template overloads surviving.
2. exactTypeSlotMatches uses isPointerShape comparison — this correctly prevents concrete value-vs-pointer collisions from reaching step 4d, making the uppercase heuristic safe for non-template overloads.
3. parameterShapeIdTag returns '' for all-value template functions (e.g., f(T) only) — does NOT change graph IDs for unaffected nodes.
4. Both integration test fixtures use describe with plain it — the LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES check works because helpers.ts:createResolverParityIt wraps the it imported by cpp.test.ts. The tied-fixture test correctly passes in both modes via normalization-collision suppression.
5. buildMethodProps:203 — typeClasses.length === info.parameters.length — variadic functions (... bare param) don't get typeClass → parameterTypeClasses is not set for variadic functions, which is safe since they don't participate in template partial ordering.

---

Open questions

1. Is the const T& > T ranking intentional as a pragmatic approximation (not ISO-conforming), or is it claimed to be ISO-conforming? If intentional approximation, the docstring at line 354–358 needs updating to say "pragmatic approximation — ISO [temp.func.order] does not actually order these" and the unit test needs to note the deviation.

---

Final verdict

not production-ready

Three issues block merge. Finding 1 (unrankable-candidate false-positive) is the most serious: the viable.length <= 1 shortcut in rankByTemplatePartialOrdering can return a single template-only candidate as the winner while silently discarding legitimate mixed-slot competitors, emitting false-positive CALLS edges in a class of code patterns that is not currently tested. Finding 2 (const T& > T) is a semantic error: ISO C++ [temp.func.order] does not give f(const T&) precedence over f(T) via partial ordering; the implementation emits false-positive edges for calls that are genuinely ambiguous. Finding 3 (unconditional shape-key overwrite) is a correctness risk under duplicate-shape conditions. All three violate DoD §2.1 (real runtime correctness) and §2.7 (tests that would fail on broken behavior). CI green does not catch these because the test suite does not exercise the failing scenarios.
· Branch

[magyargergo]

@azizur100389 could you please look into these findings? 🙏

[azizur100389]

Thanks, I looked through the review findings and pushed fixes in 579eee4.

What changed:

• Finding 1 / 4: partial ordering now only returns a winner when every surviving candidate participates in the syntactic ranking. If any co-surviving candidate is unrankable, the result suppresses instead of picking. Added unit coverage for mixed template/concrete slots and missing sidecars.
• Finding 2: removed the const T& / T& over T partial-ordering shortcut. Ref/value template shapes now remain ambiguous; the unit test now asserts suppression rather than a winner.
• Finding 3: shape-key registration in buildGraphNodeLookup now uses first-wins, matching the parameter-type key behavior.
• Finding 5: documented the uppercase-placeholder limitation and added a lowercase-placeholder regression guard showing it stays ambiguous rather than guessing.
• Finding 6: added the C++ reindex note to the PR body.

Also, after syncing latest main, the full local suite exposed a Windows-only path separator issue in the newly merged FastAPI include_router route-prefix tests. I fixed that by normalizing backslashes to forward slashes in the FastAPI file key helpers; the focused FastAPI tests now pass.

Validation run locally:

• npx prettier --check ...
• npx tsc --noEmit
• npx vitest run test/unit/scope-resolution/cpp/cpp-overload-ranking.test.ts (16 passed)
• REGISTRY_PRIMARY_CPP=1 npx vitest run test/integration/resolvers/cpp.test.ts (259 passed)
• REGISTRY_PRIMARY_CPP=0 npx vitest run test/integration/resolvers/cpp.test.ts (197 passed, 62 skipped)
• TEMP=C:\tmp TMP=C:\tmp npx vitest run test/unit/group/http-route-extractor.test.ts -t FastAPI.*include_router (3 passed)
• npm run build
• TEMP=C:\tmp TMP=C:\tmp npm test (391 files passed, 9744 tests passed, 47 skipped)
• node gitnexus\dist\cli\index.js detect-changes (low risk on this follow-up diff)