refactor(SM-20): wire SemanticModel as first-class resolution input

[Copilot]

Completes Phase 6 of the fuzzy-lookup elimination roadmap. Prior phases extracted registries into model/ behind a SemanticModel interface and migrated call sites to ctx.model.* — this PR fixes remaining review findings and consolidates the module surface.

O(n²) BFS fix in gatherAncestors

resolve.ts still used queue.shift() (O(n) per dequeue) while the adjacent buildParentMapFromHeritage was already fixed. Replaced with head-pointer:

// Before: O(n²) for deep hierarchies
const id = queue.shift()!;

// After: O(n) — matching buildParentMapFromHeritage
let head = 0;
while (head < queue.length) {
  const id = queue[head++]!;

Complete model/index.ts barrel exports

Added missing types that external consumers were importing by reaching past the barrel into individual subfiles:

• SymbolDefinition, AddMetadata, HeritageMap
• CLASS_TYPES, CALL_TARGET_TYPES, FREE_CALLABLE_TYPES and their tuple/label-type companions

Consolidate external consumer imports

Migrated call-processor.ts, type-env.ts, parsing-processor.ts, field-types.ts from direct ./model/symbol-table.js / ./model/heritage-map.js / ./model/resolve.js imports to the barrel (./model/index.js).

Verified already-complete items (no changes needed)

• All owner-scoped lookups already use ctx.model.types/methods/fields.*
• lookupMethodByOwnerWithMRO already takes model: SemanticModel
• Constructor dispatch routes correctly to methodByOwner via registration-table
• No duplicate JSDoc in model files

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• extension.ladybugdb.com
  • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/GitNexus/GitNexus/gitnexus/node_modules/.bin/vitest run test/unit --reporter=dot 14.1/deps/uv/include git modules/@npmcli/run-script/lib/node-gyp-bin/python3 get 14.1/deps/v8/inc-c st sh -c ')); git /.bin/sh _binding/src/scash credential.helpe-c on3 make (dns block)
  • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/GitNexus/GitNexus/gitnexus/node_modules/.bin/vitest run test/unit --reporter=dot ch-tree-sitter-swift.cjs && nodenode --64 (dns block)
  • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/GitNexus/GitNexus/gitnexus/node_modules/.bin/vitest run test/unit/symbol-table.test.ts --reporter=verbose HEAD gs.cjs cal/bin/git ite(sys.executab/opt/hostedtoolcache/node/24.14.1/x64/bin/node build p/bin/as rks.js rev-�� HEAD 6c3960ccb62272a4--conditions k/_temp/ghcca-nodevelopment e_sitter_proto_bgit git xus/node_modules--is-inside-work-tree git (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
gitnexus	Ready	Preview, Comment	Apr 16, 2026 1:21pm

[magyargergo]

@claude Act as a senior reviewer for GitNexus. Your job is to determine whether this PR is production-ready for this repo, not to give a generic code review.

You are reviewing a PR in the GitNexus monorepo:

• gitnexus/ → CLI + MCP
• gitnexus-web/
• gitnexus-shared/

Your task has 2 phases, in this exact order:

PHASE 1 — DEFINE THE BAR
Before reviewing the diff, establish a concise repo-specific definition of “production-ready” for GitNexus, based only on the repo docs and the affected area.
Keep this definition practical and reviewable. Do not invent standards that are not grounded in the repo.

PHASE 2 — REVIEW THE PR AGAINST THAT BAR
Review the actual diff only after defining the bar.
Stay tightly scoped to the changed code and its direct consequences.

---

CONTEXT TO LOAD FIRST
Read these before reviewing:

• AGENTS.md
• GUARDRAILS.md
• CONTRIBUTING.md
• TESTING.md
• ARCHITECTURE.md

Additional context:

• If the diff touches ingestion, also read: ARCHITECTURE.md § Pipeline Phase DAG
• PR [group/sync] Fix ManifestExtractor never called — config.links always produced 0 cross-links #827 diff area is primarily: gitnexus/src/core/group/ (sync + manifest extractor) plus associated tests

---

PRIMARY OBJECTIVE
Decide whether this PR is safe, correct, maintainable, and operationally acceptable to merge into production for GitNexus.

Do not optimize for completeness at the expense of signal.
Do not pad the review.
Do not propose unrelated refactors.
Do not restate the PR description unless needed for verification.

---

REVIEW RULES

• Every finding must be grounded in specific evidence from the diff or directly relevant surrounding code.
• Every finding must include path:line.
• If you make a behavioral claim, cite the code that proves it.
• If you make a performance claim, explain the mechanism.
• If something cannot be verified from the diff alone, explicitly say so.
• Distinguish clearly between:
  • verified issue
  • plausible risk
  • unverified concern
• Avoid vague wording like “might be better” or “could be improved” unless you explain exactly why.
• Keep the review focused on this PR’s scope only.

For each finding, assign one severity:

• BLOCKING → must be fixed before merge
• NON-BLOCKING → valid issue, but merge may still be acceptable
• NIT → stylistic/minor, not merge-relevant

---

REPO-SPECIFIC REVIEW CHECKLIST
Use these exact headings.

1. Correctness & functional completeness

Check:

• Does the implementation actually satisfy the PR claim?
  • ManifestExtractor is truly invoked
  • config.links produces non-zero cross-links where expected
• Resolver contracts are preserved:
  • resolveSymbol remains exact-match
  • label-scoped Cypher remains correct per contract type
  • flag any regression toward fuzzy or unscoped matching
• Graph schema integrity is preserved:
  • no silent changes to node labels
  • no silent changes to edge types
  • no silent changes to ID generation (generateId)
• Call out any missing wiring, partial integration, dead branch, or mismatch between tests and runtime behavior

2. Code clarity & clean code

Check:

• naming quality
• local cohesion
• dead code
• unnecessary abstraction
• hidden control flow
• confusing indirection
• adherence to repo conventions:
  • direct imports from gitnexus-shared
  • no barrel re-export regressions
  • no // removed comments
  • no unused re-exports
• no drive-by refactors outside stated scope per CONTRIBUTING.md and GUARDRAILS.md § Scope

3. Test coverage & change safety

Evaluate against TESTING.md:

• Are there unit tests under gitnexus/test/unit/ covering the newly wired path?
• Is there a regression guard for 0-link → N-link behavior?
• Are assertions meaningful rather than tautological?
• Are fixtures realistic for manifest inputs?
• If memoization/cache was introduced, is there a test proving hit/miss behavior and correctness?
• Is there evidence the expected validation path would pass for staged gitnexus/ files?
  • tsc --noEmit
  • vitest run --project default
    If not verifiable, say exactly what is missing.

4. Performance

Inspect for:

• hot-path overhead in ingestion/group sync
• excess allocations per manifest entry
• redundant Cypher round-trips
• missed batching or missed parallelism (Promise.all) where it materially matters
• O(n²) or repeated lookup patterns on large repos
• memoization tradeoffs:
  • correctness
  • invalidation
  • bounded vs unbounded memory growth
    Do not speculate casually; explain the mechanism and likely impact.

5. Operational risk

Check:

• Windows/cross-platform safety:
  • stream lifecycle
  • FD/file handle lifecycle
  • path separator assumptions
  • anything resembling prior ENOTEMPTY-style lifecycle regressions
• LadybugDB single-writer invariant is preserved
• Embeddings preservation:
  • no silent breakage of --embeddings
  • .gitnexus/meta.json.stats.embeddings not silently zeroed by changed paths
• MCP contracts remain compatible:
  • group_*
  • query
  • context
  • impact
  • detect_changes
  • rename
  • cypher
    Flag any schema or contract break without migration note
• staleness behavior still triggers correctly (gitnexus/src/mcp/staleness.ts)
• rollback safety:
  • can this PR be reverted safely without re-analyze?
  • if not, explain why

6. Maintainability

Check:

• Does the change respect Pipeline Phase DAG boundaries?
• Is cross-phase coupling being introduced?
• Are manifest-extractor entry points discoverable from group/sync.ts?
• Would the next contributor know where to extend this?
• Is shared logic placed appropriately:
  • in gitnexus-shared/
  • or gitnexus/src/core/**/utils.ts
  • and not duplicated between CLI and web bridge (server/api.ts)?

---

NON-NEGOTIABLES TO VERIFY
From GUARDRAILS.md / CONTRIBUTING.md:

• no secrets, tokens, or machine-specific paths committed
• no blind find-and-replace renames
• if a symbol rename occurred, rename MCP tool or equivalent careful review was used
• upstream impact was considered for modified shared symbols
• no HIGH/CRITICAL impact left unaddressed
• PR title follows conventional commit style + [area] Short description
• PolyForm Noncommercial license headers/metadata untouched

---

OUTPUT FORMAT

Production-ready bar for this PR

Give a concise repo-specific definition in 5–10 bullets.

Findings

For each finding, use this structure exactly:

[BLOCKING|NON-BLOCKING|NIT] Short title

• Evidence: path:line, path:line
• Why it matters: ...
• Recommendation: ...
• Confidence: High / Medium / Low

Only include findings that are real and supported.
If there are no findings in a section, say:

• No issues found in this area based on the diff and reviewed context.

What I could not fully verify

List any behavior you could not prove from the diff, tests, or available context.

Final verdict

Choose exactly one:

• ✅ Ready to merge — no blocking issues.
• ⚠️ Merge after addressing blocking items.
• ❌ Not ready — fundamental concerns.

If verdict is not ready, list the blocking items in priority order.

---

IMPORTANT REVIEW BEHAVIOR

• Be strict on correctness and contracts.
• Be conservative about operational risk.
• Be explicit about uncertainty.
• Do not suggest unrelated refactors.
• Do not turn nits into blockers.
• Do not reward intent; review actual implementation.

[github-actions]

Claude finished @magyargergo's task in 3m 16s —— View job

---

PR Review: refactor(SM-20): wire SemanticModel as first-class resolution input

Tasks:

• Load context docs (AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md)
• Read changed files diff
• Phase 1: Define production-ready bar
• Phase 2: Review PR against that bar
• Post final review

---

Production-ready bar for this PR

Based on GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, and ARCHITECTURE.md for an ingestion-layer refactoring PR:

1. No behavioral regression — import consolidation and BFS fix must produce identical runtime outputs; every re-exported symbol must resolve to the same source identity.
2. BFS correctness — head-pointer replacement must preserve breadth-first traversal order (same ancestor sequence, same visited semantics).
3. Barrel integrity — new exports must not leak factory internals that the barrel comment explicitly suppresses (e.g. createRegistrationTable, buildHeritageMap, resolveExtendsType).
4. Type-check clean — tsc --noEmit must pass; removed duplicate imports must be fully covered by the barrel.
5. No cross-phase DAG coupling — import changes must not introduce upward dependencies from model/ leaf into higher layers.
6. No drive-by refactors — only files/symbols stated in the PR description may change.
7. No secrets, machine-specific paths, or // removed comments committed.
8. PR title follows conventional-commit format — enforced by pr-labeler.yml.
9. Graph schema, node IDs, edge types, and MCP contracts untouched.
10. Rollback safe — can revert without re-analyze.

---

Findings

1. Correctness & functional completeness

• No issues found in this area based on the diff and reviewed context.

Verified details:

• BFS fix (resolve.ts:35-36): queue.shift() → queue[head++]. This is a standard head-pointer BFS pattern. Traversal order is identical — same breadth-first expansion, same visited guard logic, same order output. gatherAncestors is exported (resolve.ts:202) and used by mro-processor.ts; the function contract (BFS-ordered ancestor list, class excluded) is preserved.
• Barrel additions (model/index.ts:29-37, 79-80): SymbolDefinition, AddMetadata are type-only; CLASS_TYPES, CLASS_TYPES_TUPLE, ClassLikeLabel, FREE_CALLABLE_TYPES, FREE_CALLABLE_TUPLE, FreeCallableLabel, CALL_TARGET_TYPES are verified to exist in symbol-table.ts:57-114; HeritageMap is a type in heritage-map.ts. None of the factory internals the barrel intentionally suppresses (createRegistrationTable, buildHeritageMap, resolveExtendsType) were added.
• Import consolidation: All four files (call-processor.ts, field-types.ts, parsing-processor.ts, type-env.ts) correctly migrate to ./model/index.js. The barrel already exported all needed symbols before this PR (except the new additions) — verified.
• Resolver contracts (resolution-context.ts): not touched. Graph schema, node IDs, edge types: not touched. MCP tools/resources: not touched.

Note: The review checklist mentioned ManifestExtractor and config.links — those are from a different PR template (PR #827, gitnexus/src/core/group/). They do not apply to this diff.

---

2. Code clarity & clean code

• No issues found in this area based on the diff and reviewed context.

Verified details:

• The updated barrel comment (model/index.ts:75-77) correctly reflects that buildHeritageMap and resolveExtendsType remain unexported; HeritageMap (a type, not a builder) is now correctly included. Comment is accurate.
• Two separate import statements from ./model/index.js in call-processor.ts:7-9 (import type {...} + import {...}) — idiomatic TypeScript; ESLint import/consistent-type-specifier-style pattern.
• No dead code, no // removed comments, no unused re-exports, no drive-by refactors detected.

---

3. Test coverage & change safety

[NON-BLOCKING] No dedicated unit test for gatherAncestors head-pointer BFS

• Evidence: gitnexus/test/unit/model/ contains no test file for resolve.ts. gitnexus/test/unit/mro-processor.test.ts references gatherAncestors only indirectly. No test exercises the before/after behavior of head-pointer vs shift().
• Why it matters: The BFS fix is the only algorithmic change in this PR. While the fix is correct by inspection and the traversal order is provably equivalent, there is no regression guard. A future contributor changing gatherAncestors has no unit-level safety net.
• Recommendation: Add a unit test in gitnexus/test/unit/model/ that exercises gatherAncestors via lookupMethodByOwnerWithMRO with a multi-level hierarchy (e.g. A → B → C) and asserts the ancestor order. This is a low-effort guard for a hot ingestion path.
• Confidence: High (absence is directly observable from the test directory listing)

Cannot verify without running: tsc --noEmit (barrel type-correctness for removed duplicate imports) and vitest run test/unit (CI was blocked by network firewall on this PR — infrastructure issue, not a code issue).

---

4. Performance

• No issues found in this area based on the diff and reviewed context.

Verified: Array.shift() is O(n) due to element reindexing on every dequeue. The head-pointer replacement (queue[head++]) is O(1) per dequeue, keeping BFS linear in ancestor count. For deep hierarchies this is O(n²) → O(n). The fix matches buildParentMapFromHeritage which was already corrected. No new allocations introduced.

---

5. Operational risk

• No issues found in this area based on the diff and reviewed context.

Verified: No stream/file-handle lifecycle changes. No path separator assumptions. LadybugDB, embeddings, meta.json, and staleness.ts are untouched. All MCP tool signatures and resource schemas are unchanged. Rollback is safe — changes are purely in TypeScript source, not in graph data; reverting does not require re-analyze.

---

6. Maintainability

• No issues found in this area based on the diff and reviewed context.

Verified: Pipeline Phase DAG boundaries are respected — model/ remains a pure leaf (no upward imports added). Import consolidation actually improves discoverability: external consumers now have one canonical import point. No shared logic was moved between packages or duplicated across CLI/web bridge.

---

Non-negotiables check

Check	Status
No secrets/tokens committed	✅
No blind find-and-replace renames	✅ (import paths, not symbol renames)
Symbol renames via gitnexus_rename	N/A — no symbols renamed
Upstream impact considered	✅ — only import paths changed
No HIGH/CRITICAL impact unaddressed	✅ — all changes are internal to ingestion
PR title: conventional-commit format	✅ refactor(SM-20): ...
PolyForm license untouched	✅
No // removed comments	✅

---

What I could not fully verify

1. tsc --noEmit result — cannot run the compiler in this context. The barrel re-exports are textually correct, but type-check is the authoritative gate. CI ci-quality.yml is the gate to trust here.
2. vitest run test/unit result — CI was blocked by a network firewall error (extension.ladybugdb.com dns block) on this PR. This appears to be an infrastructure constraint on the agent runner, not a code issue. The test results are not available to confirm.
3. MRO integration coverage for gatherAncestors — the function is exercised via integration tests (Python MRO, C3 linearization paths) but I did not trace which specific test fixtures exercise multi-level BFS to confirm the head-pointer produces identical results.

---

Final verdict

✅ Ready to merge — no blocking issues.

This is a tight, correct, low-risk refactoring:

• The BFS fix is provably correct and a genuine O(n²) → O(n) improvement for deep hierarchies.
• Barrel export additions are purely additive and non-breaking.
• Import consolidation is mechanical with no behavioral change.
• Graph schema, node IDs, edge types, and all MCP contracts are untouched.

The one NON-BLOCKING finding (missing unit test for gatherAncestors) is worth tracking as follow-up but does not block merge.