Skip to content

feat(shared): add scope-resolution types + constants (#910, RFC #909 Ring 1)#949

Merged
magyargergo merged 1 commit into
mainfrom
rfc/scope-resolution/910-types
Apr 18, 2026
Merged

feat(shared): add scope-resolution types + constants (#910, RFC #909 Ring 1)#949
magyargergo merged 1 commit into
mainfrom
rfc/scope-resolution/910-types

Conversation

@magyargergo

Copy link
Copy Markdown
Collaborator

Closes #910. Part of RFC #909 Ring 1.

Summary

Lands the authoritative data model + constants for the pure scope-based resolution RFC as the first foundational PR. Types and constants only — no runtime behavior changes.

New in gitnexus-shared/src/scope-resolution/:

  • types.tsScope, ScopeKind, ScopeId, DefId, Range, Capture, BindingRef, ImportEdge, TypeRef, Resolution, ResolutionEvidence, Reference, ReferenceIndex, LookupParams, RegistryContributor
  • evidence-weights.tsEvidenceWeights constant map + typeBindingWeightAtDepth helper (RFC Appendix A)
  • origin-priority.tsORIGIN_PRIORITY constant map for deterministic tie-breaks (RFC Appendix B)
  • language-classification.tsLanguageClassification type + LanguageClassifications map. Initial: production × 14, experimental × 2 (Vue for SFC complexity, COBOL for the regex-provider path)
  • symbol-definition.tsSymbolDefinition moved from gitnexus/src/core/ingestion/model/symbol-table.ts so scope-resolution types in gitnexus-shared can reference it without a reverse package dependency

Consumer migration

Per the project convention on direct imports from gitnexus-shared:

  • symbol-table.ts drops its local SymbolDefinition declaration and imports from gitnexus-shared
  • model/index.ts drops the SymbolDefinition barrel re-export
  • 9 source files + 5 test files updated to import SymbolDefinition directly from 'gitnexus-shared'

Verification

  • gitnexus-shared builds clean (tsc)
  • gitnexus builds clean (node scripts/build.js)
  • 131/132 unit test files pass; 3767 tests green; zero regressions
  • SymbolDefinition shape unchanged — interface body identical to prior definition

Unblocks

Design references

Test plan

  • gitnexus-shared build (cd gitnexus-shared && npm run build)
  • gitnexus full build (cd gitnexus && node scripts/build.js)
  • Model unit tests (npx vitest run test/unit/model) — 84 pass
  • Full unit suite — 3767 pass, 0 fail

…Ring 1)

Lands the authoritative data model and constants for the pure scope-based
resolution RFC (#909) as Ring 1, part 1. No runtime behavior changes —
types + constants only.

New in gitnexus-shared/src/scope-resolution/:
  - types.ts — Scope, ScopeKind, ScopeId, DefId, Range, Capture,
    BindingRef, ImportEdge, TypeRef, Resolution, ResolutionEvidence,
    Reference, ReferenceIndex, LookupParams, RegistryContributor
  - evidence-weights.ts — EvidenceWeights constant map + typeBindingWeightAtDepth
    (RFC Appendix A)
  - origin-priority.ts — ORIGIN_PRIORITY constant map for deterministic
    tie-breaks (RFC Appendix B)
  - language-classification.ts — LanguageClassification type +
    LanguageClassifications map (production × 14, experimental × 2
    for vue/cobol; governs Ring 4 DAG-retirement gate)
  - symbol-definition.ts — SymbolDefinition moved from
    gitnexus/src/core/ingestion/model/symbol-table.ts so scope-resolution
    types can reference it from the shared package

Consumer updates:
  - symbol-table.ts: removes local SymbolDefinition declaration; imports
    from gitnexus-shared
  - model/index.ts: drops SymbolDefinition from barrel re-export per
    "direct imports from gitnexus-shared" convention (see
    gitnexus-shared feedback in project memory)
  - 9 source files + 5 test files: import SymbolDefinition directly
    from 'gitnexus-shared'

Verification:
  - gitnexus-shared builds clean (tsc)
  - gitnexus builds clean (scripts/build.js)
  - 131/132 unit test files pass; 3767 tests green
  - Zero behavior changes; SymbolDefinition shape unchanged

Blocks: #911 (LanguageProvider hook interface extensions) and all of
Ring 2 (#912-#925). Closes part of #909.
@vercel

vercel Bot commented Apr 18, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
gitnexus Ready Ready Preview, Comment Apr 18, 2026 10:36am

Request Review

@github-actions

github-actions Bot commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

CI Report

All checks passed

Pipeline Status

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

Test Results

Tests Passed Failed Skipped Duration
6606 6509 0 97 260s

✅ All 6509 tests passed

97 test(s) skipped — expand for details
  • Swift MethodExtractor > isTypeDeclaration > recognizes class_declaration
  • Swift MethodExtractor > isTypeDeclaration > recognizes protocol_declaration
  • Swift MethodExtractor > isTypeDeclaration > rejects import_declaration
  • Swift MethodExtractor > visibility > extracts public method
  • Swift MethodExtractor > visibility > extracts private method
  • Swift MethodExtractor > visibility > defaults to internal when no modifier
  • Swift MethodExtractor > protocol methods > marks protocol method as abstract
  • Swift MethodExtractor > static and class methods > detects static func as isStatic
  • Swift MethodExtractor > static and class methods > detects class func as isStatic
  • Swift MethodExtractor > parameters > extracts parameters with types and default values
  • Swift MethodExtractor > return type > extracts return type from -> annotation
  • Swift MethodExtractor > annotations > extracts @objc attribute
  • Swift MethodExtractor > isFinal > detects final func
  • Swift MethodExtractor > isFinal > is false when not final
  • Swift MethodExtractor > isAsync > detects async func
  • Swift MethodExtractor > isOverride > detects override method
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift cross-file constructor inference uses lookupClassByName
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift explicit init inference uses lookupClassByName
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift cross-file constructor inference does not bind plain functions
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature
  • Swift constructor-inferred type resolution > detects User and Repo classes, both with save methods
  • Swift constructor-inferred type resolution > resolves user.save() to Models/User.swift via constructor-inferred type
  • Swift constructor-inferred type resolution > resolves repo.save() to Models/Repo.swift via constructor-inferred type
  • Swift constructor-inferred type resolution > emits exactly 2 save() CALLS edges (one per receiver type)
  • Swift self resolution > detects User and Repo classes, each with a save function
  • Swift self resolution > resolves self.save() inside User.process to User.save, not Repo.save
  • Swift parent resolution > detects BaseModel and User classes plus Serializable protocol
  • Swift parent resolution > emits EXTENDS edge: User → BaseModel
  • Swift parent resolution > emits IMPLEMENTS edge: User → Serializable (protocol conformance)
  • Swift cross-file User.init() inference > resolves user.save() via User.init(name:) inference
  • Swift cross-file User.init() inference > resolves user.greet() via User.init(name:) inference
  • Swift return type inference > detects User class and getUser function
  • Swift return type inference > detects save function on User (Swift class methods are Function nodes)
  • Swift return type inference > resolves user.save() to User#save via return type of getUser() -> User
  • Swift return-type inference via function return type > resolves user.save() to User#save via return type of getUser()
  • Swift return-type inference via function return type > user.save() does NOT resolve to Repo#save
  • Swift return-type inference via function return type > resolves repo.save() to Repo#save via return type of getRepo()
  • Swift implicit imports (cross-file visibility) > detects UserService class in Models.swift
  • Swift implicit imports (cross-file visibility) > resolves UserService() constructor call across files (no explicit import)
  • Swift implicit imports (cross-file visibility) > resolves service.fetchUser() member call across files
  • Swift implicit imports (cross-file visibility) > creates IMPORTS edges between files in the same module
  • Swift extension deduplication > detects Product class
  • Swift extension deduplication > resolves Product() constructor despite extension creating duplicate class node
  • Swift extension deduplication > resolves product.save() to Product.swift (primary definition)
  • Swift constructor call fallback (no new keyword) > resolves OCRService() as constructor call across files
  • Swift constructor call fallback (no new keyword) > resolves ocr.recognize() member call via constructor-inferred type
  • Swift export visibility (internal vs private) > resolves PublicService() constructor across files
  • Swift export visibility (internal vs private) > resolves internalHelper() across files (internal = module-scoped)
  • Swift if let / guard let binding resolution > detects User and Repo classes
  • Swift if let / guard let binding resolution > resolves user.save() inside if-let to User#save
  • Swift if let / guard let binding resolution > resolves repo.save() inside guard-let to Repo#save
  • Swift if let / guard let binding resolution > user.save() in if-let does NOT resolve to Repo#save
  • Swift await / try expression unwrapping > resolves user.save() via await fetchUser() return type
  • Swift await / try expression unwrapping > resolves repo.save() via try parseRepo() return type
  • Swift await / try expression unwrapping > detects fetchUser and parseRepo as functions
  • Swift for-in loop element type inference > detects User and Repo classes
  • Swift for-in loop element type inference > creates implicit import edges between files
  • Swift field-type resolution > detects classes and their properties
  • Swift field-type resolution > emits HAS_PROPERTY edges from class to field
  • Swift field-type resolution > resolves field-chain call user.address.save() → Address#save
  • Swift field-type resolution > emits ACCESSES edges for field reads in chains
  • Swift field-type resolution > populates field metadata (visibility, declaredType) on Property nodes
  • Swift call-result binding > resolves call-result-bound method call user.save() → User#save
  • Swift call-result binding > getUser() is present as a defined function
  • Swift call-result binding > emits processUser -> getUser CALLS edge for let-assigned free function call
  • Swift method enrichment > detects Animal protocol and Dog class
  • Swift method enrichment > emits IMPLEMENTS edge Dog -> Animal
  • Swift method enrichment > emits HAS_METHOD edges for Dog methods
  • Swift method enrichment > marks protocol Animal.speak as isAbstract
  • Swift method enrichment > marks Dog.speak as NOT isAbstract
  • Swift method enrichment > marks breathe as isFinal
  • Swift method enrichment > marks classify as isStatic
  • Swift method enrichment > captures @objc annotation on breathe
  • Swift method enrichment > populates parameterTypes for classify(_ name: String)
  • Swift method enrichment > records parameterCount for classify
  • Swift method enrichment > records returnType for speak
  • Swift method enrichment > resolves dog.speak() CALLS edge
  • Swift method enrichment > resolves Dog.classify("dog") CALLS edge
  • Swift abstract dispatch > detects Repository protocol and SqlRepository class
  • Swift abstract dispatch > emits IMPLEMENTS edge SqlRepository -> Repository
  • Swift abstract dispatch > emits HAS_METHOD edges for Repository.find and Repository.save
  • Swift abstract dispatch > emits HAS_METHOD edges for SqlRepository.find and SqlRepository.save
  • Swift abstract dispatch > marks base Repository.find as isAbstract
  • Swift abstract dispatch > marks base Repository.save as isAbstract
  • Swift abstract dispatch > marks concrete SqlRepository.find as NOT isAbstract
  • Swift abstract dispatch > resolves repo.find(id: 42) CALLS edge
  • Swift abstract dispatch > resolves repo.save(entity: user) CALLS edge
  • Swift abstract dispatch > populates parameterTypes for Repository.find
  • Swift abstract dispatch > populates parameterTypes for Repository.save
  • Swift abstract dispatch > records returnType for SqlRepository.find
  • Swift abstract dispatch > emits METHOD_IMPLEMENTS edges from SqlRepository methods → Repository protocol methods
  • Swift overloaded method disambiguation > detects 2 distinct find Method nodes on SqlRepository
  • Swift overloaded method disambiguation > emits METHOD_IMPLEMENTS edges for both find overloads
  • Swift overloaded method disambiguation > emits METHOD_IMPLEMENTS edge for save
  • Swift overloaded method disambiguation > emits exactly 3 METHOD_IMPLEMENTS edges total
  • Swift Child extends Parent — inherited method resolution (SM-9) > detects Parent and Child classes
  • Swift Child extends Parent — inherited method resolution (SM-9) > resolves c.parentMethod() to Parent.parentMethod via first-wins MRO walk

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 73.22% 17852/24380 73.22% = 0.0 🟢 ██████████████░░░░░░
Branches 62.24% 11339/18218 62.24% = 0.0 🟢 ████████████░░░░░░░░
Functions 77.82% 1688/2169 77.82% = 0.0 🟢 ███████████████░░░░░
Lines 75.78% 16185/21356 75.78% = 0.0 🟢 ███████████████░░░░░

📋 View full run · Generated by CI

@magyargergo

Copy link
Copy Markdown
Collaborator Author

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


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.

You must also ensure we are on track with the RFC plan! Check the issue tickets involved!

@github-actions

github-actions Bot commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

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


Senior Code Review — PR #949


Production-ready bar for this PR

Based on AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, and ARCHITECTURE.md, the bar for a types+constants-only shared-package PR is:

  1. Shape fidelity — any moved interface (SymbolDefinition) must be byte-identical to the original; no silent field drops or type widening.
  2. Consumer migration completeness — every file that imported the moved symbol from its old location must be updated to the new canonical import path (gitnexus-shared).
  3. Barrel hygiene — the old barrel (model/index.ts) must not re-export the moved symbol; per-repo convention, consumers import directly from gitnexus-shared.
  4. Exhaustiveness enforcement — any Record<SupportedLanguages, ...> or similar constant map must be type-checked exhaustively at compile time, not just at runtime.
  5. Build greentsc --noEmit clean in both gitnexus/ and gitnexus-shared/; all unit tests pass with no regressions.
  6. No runtime behavior changes — types and constants only; no phase DAG modifications, no KnowledgeGraph mutations, no MCP contract changes.
  7. No secrets, machine paths, or unauthorized renames (GUARDRAILS.md non-negotiables).
  8. PR title follows conventional commit style (CONTRIBUTING.md).
  9. RFC alignment — the types, weights, and constants must match the RFC sections they claim to implement (§2, Appendix A, Appendix B, §6.1).
  10. Rollback safety — the commit must be safely revertable without requiring a re-analyze.

Findings

1. Correctness & functional completeness

SymbolDefinition shape identity — verified ✓

  • The removed block in gitnexus/src/core/ingestion/model/symbol-table.ts and the new gitnexus-shared/src/scope-resolution/symbol-definition.ts:14-35 are field-for-field identical: nodeId, filePath, type, qualifiedName?, parameterCount?, requiredParameterCount?, parameterTypes?, returnType?, declaredType?, ownerId?. No widening, no narrowing. ✓

All 14 consumer files updated — verified ✓

  • All files that previously imported SymbolDefinition now do so from 'gitnexus-shared' directly. A grep over gitnexus/src confirms zero imports via the old model/index barrel path. ✓

LanguageClassifications exhaustiveness — verified ✓

  • language-classification.ts:26 types the constant as Readonly<Record<SupportedLanguages, LanguageClassification>>. TypeScript enforces all 16 keys at compile time. All 16 SupportedLanguages members are present. ✓

Barrel correctly narrowed — verified ✓

  • model/index.ts removes type SymbolDefinition from its re-export and adds a directing comment. No stale re-export remains. ✓

No graph schema changes — verified ✓

  • No modifications to lbug/schema.ts, KnowledgeGraph, node labels, edge types, or generateId. ✓

RFC section coverage:

  • types.ts covers RFC §2.1–2.8 (all named types from the PR description are present). ✓
  • evidence-weights.ts matches RFC Appendix A structure. ✓
  • origin-priority.ts matches RFC Appendix B tie-break ordering. ✓
  • language-classification.ts matches RFC §6.1 Ring 3/4 governance spec. ✓

2. Code clarity & clean code

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

  • Naming is idiomatic and consistent with the RFC.
  • No dead code, no unused abstractions.
  • No // removed comments; the symbol-table.ts migration note (lines 116–119) is a forward-facing guide comment, not a removed-code marker.
  • No drive-by refactors outside stated scope.
  • Direct imports from gitnexus-shared convention is correctly followed in all 14 updated files.

3. Test coverage & change safety

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

This is a types-only PR. The correct test signal is:

  • tsc --noEmit clean — confirmed by CI Typecheck ✅
  • Unit test suite passes with zero regressions — confirmed by CI (6509 passed, 0 failed)
  • SymbolDefinition shape identity verified structurally — no new unit tests are needed for a moved interface with no behavior changes

The 5 updated test files (field-registry.test.ts, helpers.ts, registration-table.test.ts, type-registry.test.ts, type-env.test.ts) are import-path updates only; test assertions are unchanged. ✓


4. Performance

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

Pure types and as const constants have zero runtime overhead. No hot paths, allocations, or data structures are modified.


5. Operational risk

[NON-BLOCKING] CI stage reports "failure" with zero failing tests

  • Evidence: CI report — Stage: ❌ Tests: failure | unit tests, 3 platforms; but results table: 6509 passed / 0 failed / 97 skipped. These are contradictory.
  • Why it matters: A failure label on the Tests stage means this PR would block auto-merge rules if any are configured. The 97 skipped tests are all Swift tests (Swift MethodExtractor, Swift resolution suites), none of which are touched by this PR. They appear pre-existing. If this PR introduced the skips, that would be a regression; if pre-existing, the CI stage label is a false positive.
  • Recommendation: Confirm the Swift test skips predate this PR (check main-branch CI run). If they are pre-existing, the CI stage failure is noise and can be ignored for merge purposes. If they are new, investigate before merging.
  • Confidence: High (the evidence is in front of us, but the pre-existing vs. new skip question cannot be answered from diff alone).

No other operational risk found:

  • No Windows/cross-platform path handling (types only). ✓
  • LadybugDB single-writer invariant unaffected. ✓
  • Embeddings unaffected. ✓
  • All MCP tool contracts unchanged. ✓
  • Staleness logic unchanged. ✓
  • Rollback safety: fully revertable — reverting this commit restores SymbolDefinition to symbol-table.ts and removes the shared types. No schema migration required. ✓

6. Maintainability

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

  • Pipeline Phase DAG: untouched. No phase coupling introduced. ✓
  • gitnexus-shared/src/scope-resolution/ is a clean sub-directory; each file has a single responsibility traceable to an RFC section. ✓
  • RegistryContributor = unknown (types.ts:246) is an intentional opaque placeholder for Ring 2 (RING2-SHARED-6: ClassRegistry / MethodRegistry / FieldRegistry + evidence composition #917), documented with a JSDoc comment explaining the deferral. A future Ring 2 PR has a clear target to narrow this type. ✓
  • typeBindingWeightAtDepth defensive guard for mroDepth < 0 (line 87) is reasonable but the negative-depth case is unspecified in RFC Appendix A. Not a blocker — it is a conservative fallback to table[0] and cannot silently corrupt results.
  • The next contributor working on Ring 2 will find the RFC-section-labelled files directly navigable.

NON-NEGOTIABLES CHECK

Item Status
No secrets, tokens, or machine paths
No blind find-and-replace renames ✓ (SymbolDefinition moved, not renamed; all dependents updated)
Impact of SymbolDefinition move considered ✓ (14 consumers updated, no missed dependents in gitnexus/src or gitnexus-web/src)
No HIGH/CRITICAL impact left unaddressed ✓ (build and tests pass)
PR title follows conventional commit format ✓ (feat(shared): ...)
PolyForm Noncommercial license metadata untouched

RFC PLAN ALIGNMENT

RFC #909 Ring 1 (#910) — On track ✓

Ring 1 is defined as the foundational data model + constants. This PR delivers exactly that with no more and no less scope. The stated unblocks are correct:

The RegistryContributor = unknown placeholder in types.ts:246 is explicitly flagged as a Ring 2 concern (#917), meaning the RFC sequencing is intentional and correct.


What I could not fully verify

  1. Whether the 97 skipped Swift tests are pre-existing on main — cannot determine from the diff alone. A CI run on main would confirm. This is the only open question for the NON-BLOCKING finding above.
  2. Whether the RFC Notion document values for EvidenceWeights and ORIGIN_PRIORITY exactly match the constants in this PR — the Notion link is external and inaccessible. I verified internal consistency (JSDoc references match code structure) but not the authoritative RFC source values.
  3. Whether gitnexus-web has any implicit dependency on SymbolDefinition via the old barrel path — a grep over gitnexus-web/src found zero references to SymbolDefinition. ✓ Verified.

Final verdict

Ready to merge — no blocking issues.

This is a clean, well-scoped, well-documented foundational PR. Shape fidelity verified, all consumers migrated, barrel hygiene correct, exhaustiveness type-checked, and RFC alignment confirmed. The single NON-BLOCKING item (CI stage failure label vs. zero actual test failures) should be clarified but does not block merge if the Swift skips predate this PR.

@magyargergo magyargergo merged commit afc0a8b into main Apr 18, 2026
29 of 31 checks passed
@magyargergo magyargergo deleted the rfc/scope-resolution/910-types branch April 18, 2026 11:55
magyargergo added a commit that referenced this pull request Apr 18, 2026
Aggregates all actionable follow-ups from the 9 Ring 2 SHARED PRs
(#949#963) before proceeding to Ring 2 PKG. No behavior changes;
docstring edits, test refinements, and one structural cleanup.

## #913 (DefIndex / ModuleScopeIndex / QualifiedNameIndex)
  - Rename `freezeIndex` → `wrapIndex` across all three index builders.
    The old name implied `Object.freeze` on the wrapper, which we never
    applied; `wrapIndex` more accurately describes the lightweight
    readonly-interface wrap. Safety surface (frozen bucket arrays,
    frozen miss-empty array, readonly Maps) is unchanged.
  - Document in `buildModuleScopeIndex` JSDoc that callers must
    pre-normalize `filePath` keys (no path-separator canonicalization
    happens here). Prevents silent cross-platform misses.
  - Add an explicit hit-path freeze assertion in
    `qualified-name-index.test.ts` (the existing test covered only the
    miss-path `EMPTY` array).

## #914 (MethodDispatchIndex)
  - Differentiate the C3 and BFS test cases: both tests now use
    distinct MRO orderings so they prove the materializer stores
    whatever order the `computeMro` callback produces (not that C3 and
    BFS yield identical output).
  - Add `implementsOfCalls` counter in the first-write-wins test, and
    document the call-count contract in `MethodDispatchInput.implementsOf`
    JSDoc: `implementsOf` fires **per occurrence** in `input.owners`
    (not per unique owner); `computeMro` fires at most once per unique
    owner. Callers with expensive `implementsOf` implementations should
    pre-dedupe `owners`.

## #916 (resolveTypeRef)
  - Document the deliberate exclusion of `'Type'` from `TYPE_KINDS`
    (verified no extractor in `gitnexus/src/core/ingestion/` emits
    `type: 'Type'` for annotation-relevant symbols).
  - Rename the namespace-origin test from `'resolves ...'` to
    `'returns null for a namespace-origin binding whose def is not a
    type kind'`, matching the failure-case intent.

## #918 (shadow diff + aggregate)
  - Remove the partial re-export `export type { ShadowAgreement, ShadowDiff };`
    from `aggregate.ts` — it omitted `ShadowCallsite` and diverged
    from the top-level barrel. Consumers import all three from the
    `gitnexus-shared` entry point.
  - Fix the invalid `'wildcard'` evidence kind in `diff.test.ts` fixture
    (that kind is not a valid `ResolutionEvidence.kind`). Replaced with
    `'global-name'`, a real kind the test treats identically.

## #912 (ScopeTree / PositionIndex / makeScopeId)
  - Document the touching-boundary semantics on `PositionIndex.atPosition`:
    when siblings share a boundary point, the right (later-start) sibling
    wins per the existing innermost-wins sort contract.
  - Resolve the layer-inversion flagged by review: move `ScopeLookup`
    from `resolve-type-ref.ts` to `types.ts` (its natural home in the
    data-model layer). `scope-tree.ts` now imports `ScopeLookup` from
    `types.js` directly; the old re-export from `resolve-type-ref.ts`
    is removed per repo convention (`feedback_no_reexport`). Barrel
    export moved alongside.

## #917 (ClassRegistry / MethodRegistry / FieldRegistry)
  - Replace the dangling "try a name-match among class-like defs"
    comment in `lookupReceiverType` with explicit prose that callers
    must pre-resolve via `resolveTypeRef` if they want richer semantics.
    No behavior change — the function already returned `undefined` on
    ambiguous/missing qnames.
  - Fix `tieBreakKey.origin` default for pure Step-2 candidates.
    Type-binding-only hits no longer falsely inherit `'local'` from
    `ensureCandidate`'s neutral default; they now demote to `'import'`
    on their first type-binding hit, and only a later Step-1 lexical
    hit can upgrade them back to `'local'`. Keeps the Appendix B
    cascade faithful to the true origin.
  - Document `'global-name'` in `evidence.ts`: currently reserved for
    Ring 3's byName global index; `lookupCore` never emits it today.
    The weight stays live so `composeEvidence` remains exhaustive over
    the origin union.
  - Rename the mislabeled Step-7 test from `'confidence DESC is the
    primary key'` (which actually tested hard-shadow baseline) to
    `'inner scope shadows outer, yielding single result'`, and add a
    separate test that actually exercises multi-candidate confidence
    ordering (local vs wildcard at the same scope).

## Verification
  - `tsc --noEmit` clean (both `gitnexus-shared` and `gitnexus`)
  - `gitnexus-shared` build clean
  - Combined scope-resolution / model / shadow suite: **260/260 pass**
    (+1 from the new multi-candidate ordering test in #917)

## Not addressed (non-actionable)
  - #949 CI "failure with zero failing tests": pre-existing Swift Node 22
    grammar flake unrelated to #910 scope.
  - #950: the two non-blocking findings were already addressed in
    follow-up commit `cbac32ba` (ParsedImport discriminated union +
    `ScopeId | null` on the two hooks).
  - #915: the five in-scope findings were already addressed in
    follow-up commit `54515a7e` (dead code, unused params, multi-hop
    docs, cap-hit test, stats granularity).
  - #915 LanguageProvider.resolveImportTarget signature divergence +
    `findDefById` O(F×D) perf: tracked separately as follow-up issues
    for the Ring 3 migration window.
magyargergo added a commit that referenced this pull request Apr 18, 2026
* chore(shared): apply Ring 2 SHARED review follow-ups in one diff

Aggregates all actionable follow-ups from the 9 Ring 2 SHARED PRs
(#949#963) before proceeding to Ring 2 PKG. No behavior changes;
docstring edits, test refinements, and one structural cleanup.

## #913 (DefIndex / ModuleScopeIndex / QualifiedNameIndex)
  - Rename `freezeIndex` → `wrapIndex` across all three index builders.
    The old name implied `Object.freeze` on the wrapper, which we never
    applied; `wrapIndex` more accurately describes the lightweight
    readonly-interface wrap. Safety surface (frozen bucket arrays,
    frozen miss-empty array, readonly Maps) is unchanged.
  - Document in `buildModuleScopeIndex` JSDoc that callers must
    pre-normalize `filePath` keys (no path-separator canonicalization
    happens here). Prevents silent cross-platform misses.
  - Add an explicit hit-path freeze assertion in
    `qualified-name-index.test.ts` (the existing test covered only the
    miss-path `EMPTY` array).

## #914 (MethodDispatchIndex)
  - Differentiate the C3 and BFS test cases: both tests now use
    distinct MRO orderings so they prove the materializer stores
    whatever order the `computeMro` callback produces (not that C3 and
    BFS yield identical output).
  - Add `implementsOfCalls` counter in the first-write-wins test, and
    document the call-count contract in `MethodDispatchInput.implementsOf`
    JSDoc: `implementsOf` fires **per occurrence** in `input.owners`
    (not per unique owner); `computeMro` fires at most once per unique
    owner. Callers with expensive `implementsOf` implementations should
    pre-dedupe `owners`.

## #916 (resolveTypeRef)
  - Document the deliberate exclusion of `'Type'` from `TYPE_KINDS`
    (verified no extractor in `gitnexus/src/core/ingestion/` emits
    `type: 'Type'` for annotation-relevant symbols).
  - Rename the namespace-origin test from `'resolves ...'` to
    `'returns null for a namespace-origin binding whose def is not a
    type kind'`, matching the failure-case intent.

## #918 (shadow diff + aggregate)
  - Remove the partial re-export `export type { ShadowAgreement, ShadowDiff };`
    from `aggregate.ts` — it omitted `ShadowCallsite` and diverged
    from the top-level barrel. Consumers import all three from the
    `gitnexus-shared` entry point.
  - Fix the invalid `'wildcard'` evidence kind in `diff.test.ts` fixture
    (that kind is not a valid `ResolutionEvidence.kind`). Replaced with
    `'global-name'`, a real kind the test treats identically.

## #912 (ScopeTree / PositionIndex / makeScopeId)
  - Document the touching-boundary semantics on `PositionIndex.atPosition`:
    when siblings share a boundary point, the right (later-start) sibling
    wins per the existing innermost-wins sort contract.
  - Resolve the layer-inversion flagged by review: move `ScopeLookup`
    from `resolve-type-ref.ts` to `types.ts` (its natural home in the
    data-model layer). `scope-tree.ts` now imports `ScopeLookup` from
    `types.js` directly; the old re-export from `resolve-type-ref.ts`
    is removed per repo convention (`feedback_no_reexport`). Barrel
    export moved alongside.

## #917 (ClassRegistry / MethodRegistry / FieldRegistry)
  - Replace the dangling "try a name-match among class-like defs"
    comment in `lookupReceiverType` with explicit prose that callers
    must pre-resolve via `resolveTypeRef` if they want richer semantics.
    No behavior change — the function already returned `undefined` on
    ambiguous/missing qnames.
  - Fix `tieBreakKey.origin` default for pure Step-2 candidates.
    Type-binding-only hits no longer falsely inherit `'local'` from
    `ensureCandidate`'s neutral default; they now demote to `'import'`
    on their first type-binding hit, and only a later Step-1 lexical
    hit can upgrade them back to `'local'`. Keeps the Appendix B
    cascade faithful to the true origin.
  - Document `'global-name'` in `evidence.ts`: currently reserved for
    Ring 3's byName global index; `lookupCore` never emits it today.
    The weight stays live so `composeEvidence` remains exhaustive over
    the origin union.
  - Rename the mislabeled Step-7 test from `'confidence DESC is the
    primary key'` (which actually tested hard-shadow baseline) to
    `'inner scope shadows outer, yielding single result'`, and add a
    separate test that actually exercises multi-candidate confidence
    ordering (local vs wildcard at the same scope).

## Verification
  - `tsc --noEmit` clean (both `gitnexus-shared` and `gitnexus`)
  - `gitnexus-shared` build clean
  - Combined scope-resolution / model / shadow suite: **260/260 pass**
    (+1 from the new multi-candidate ordering test in #917)

## Not addressed (non-actionable)
  - #949 CI "failure with zero failing tests": pre-existing Swift Node 22
    grammar flake unrelated to #910 scope.
  - #950: the two non-blocking findings were already addressed in
    follow-up commit `cbac32ba` (ParsedImport discriminated union +
    `ScopeId | null` on the two hooks).
  - #915: the five in-scope findings were already addressed in
    follow-up commit `54515a7e` (dead code, unused params, multi-hop
    docs, cap-hit test, stats granularity).
  - #915 LanguageProvider.resolveImportTarget signature divergence +
    `findDefById` O(F×D) perf: tracked separately as follow-up issues
    for the Ring 3 migration window.

* chore(shared): address ce:review findings on the follow-up diff

ce:review (interactive) on PR #964 surfaced two P2s and several P3s. This
commit applies all `safe_auto` fixes + both manual tests in-line so the
PR ships with a cleaner review trail.

## P2 fixes

- **Complete `freezeIndex` → `wrapIndex` rename.** The prior commit renamed
  3 of 5 sibling index files; `method-dispatch-index.ts` and
  `position-index.ts` still carried the old name. Now all 5 helpers use
  the consistent `wrapIndex` naming.
  (maintainability + project-standards reviewers both flagged this.)

- **Add regression tests for the `recordTypeBindingHit` origin demotion.**
  The prior commit introduced the `tieBreakKey.origin = 'import'`
  demotion for Step-2-only candidates without a direct test. Added:
    - `registries.test.ts`: two Step-2-only siblings under the same
      interface, asserting deterministic DefId.localeCompare tie-break
      AND the stronger invariant that composeEvidence never emits a
      where-found signal for Step-2-only candidates (no `signals.origin`).
    - `position-index.test.ts`: touching-boundary test proving the
      right-sibling-wins rule documented in the new JSDoc.
  (testing + kieran-typescript + api-contract reviewers all flagged these gaps.)

## P3 fixes

- Fix wrong comment in `recordTypeBindingHit` that claimed Step 1 could
  later upgrade a demoted origin. Step 1 runs BEFORE Step 2 — the actual
  upgrade path is Step 3 (`seedFromOwnerScopedContributor`). Comment now
  describes execution order correctly.

- Fix inaccurate "re-exported there" comment in `index.ts`. `types.ts`
  *defines* ScopeLookup natively; it's not a re-export. Phrasing now
  says "defined in types.ts and exported from the type-export block
  above — not from this module."

- Update stale `scope-tree.ts` file-header prose that still referenced
  `ScopeLookup` as living in #916/resolve-type-ref.ts. Now points to
  `./types.js` with a cross-ref to both #916 and #917 consumers.

- Expand `atPosition` touching-boundary JSDoc to name the mechanism
  (backward scan through start-sorted array) so readers can trace the
  binary-search code to the claim.

- Add breadcrumb to `aggregate.ts` module header pointing future readers
  to `./diff.ts` / the top-level barrel for `ShadowAgreement`,
  `ShadowCallsite`, and `ShadowDiff`.

- Remove unnecessary non-null assertion in `recordTypeBindingHit`. Local
  `const existingMroDepth = ...` lets TS narrow to `number` in the
  else-branch, eliminating the `!` without behavior change.

## Verification

- `tsc --noEmit` clean (both `gitnexus-shared` and `gitnexus`)
- `gitnexus-shared` build clean
- Combined scope-resolution / model / shadow suite: **262/262 pass** (+2
  from the new origin-demotion + touching-boundary regression tests)
github714801013 pushed a commit to github714801013/GitNexus that referenced this pull request Apr 28, 2026
…#910, RFC abhigyanpatwari#909 Ring 1) (abhigyanpatwari#949)

Lands the authoritative data model and constants for the pure scope-based
resolution RFC (abhigyanpatwari#909) as Ring 1, part 1. No runtime behavior changes —
types + constants only.

New in gitnexus-shared/src/scope-resolution/:
  - types.ts — Scope, ScopeKind, ScopeId, DefId, Range, Capture,
    BindingRef, ImportEdge, TypeRef, Resolution, ResolutionEvidence,
    Reference, ReferenceIndex, LookupParams, RegistryContributor
  - evidence-weights.ts — EvidenceWeights constant map + typeBindingWeightAtDepth
    (RFC Appendix A)
  - origin-priority.ts — ORIGIN_PRIORITY constant map for deterministic
    tie-breaks (RFC Appendix B)
  - language-classification.ts — LanguageClassification type +
    LanguageClassifications map (production × 14, experimental × 2
    for vue/cobol; governs Ring 4 DAG-retirement gate)
  - symbol-definition.ts — SymbolDefinition moved from
    gitnexus/src/core/ingestion/model/symbol-table.ts so scope-resolution
    types can reference it from the shared package

Consumer updates:
  - symbol-table.ts: removes local SymbolDefinition declaration; imports
    from gitnexus-shared
  - model/index.ts: drops SymbolDefinition from barrel re-export per
    "direct imports from gitnexus-shared" convention (see
    gitnexus-shared feedback in project memory)
  - 9 source files + 5 test files: import SymbolDefinition directly
    from 'gitnexus-shared'

Verification:
  - gitnexus-shared builds clean (tsc)
  - gitnexus builds clean (scripts/build.js)
  - 131/132 unit test files pass; 3767 tests green
  - Zero behavior changes; SymbolDefinition shape unchanged

Blocks: abhigyanpatwari#911 (LanguageProvider hook interface extensions) and all of
Ring 2 (abhigyanpatwari#912-abhigyanpatwari#925). Closes part of abhigyanpatwari#909.
github714801013 pushed a commit to github714801013/GitNexus that referenced this pull request Apr 28, 2026
…igyanpatwari#964)

* chore(shared): apply Ring 2 SHARED review follow-ups in one diff

Aggregates all actionable follow-ups from the 9 Ring 2 SHARED PRs
(abhigyanpatwari#949abhigyanpatwari#963) before proceeding to Ring 2 PKG. No behavior changes;
docstring edits, test refinements, and one structural cleanup.

## abhigyanpatwari#913 (DefIndex / ModuleScopeIndex / QualifiedNameIndex)
  - Rename `freezeIndex` → `wrapIndex` across all three index builders.
    The old name implied `Object.freeze` on the wrapper, which we never
    applied; `wrapIndex` more accurately describes the lightweight
    readonly-interface wrap. Safety surface (frozen bucket arrays,
    frozen miss-empty array, readonly Maps) is unchanged.
  - Document in `buildModuleScopeIndex` JSDoc that callers must
    pre-normalize `filePath` keys (no path-separator canonicalization
    happens here). Prevents silent cross-platform misses.
  - Add an explicit hit-path freeze assertion in
    `qualified-name-index.test.ts` (the existing test covered only the
    miss-path `EMPTY` array).

## abhigyanpatwari#914 (MethodDispatchIndex)
  - Differentiate the C3 and BFS test cases: both tests now use
    distinct MRO orderings so they prove the materializer stores
    whatever order the `computeMro` callback produces (not that C3 and
    BFS yield identical output).
  - Add `implementsOfCalls` counter in the first-write-wins test, and
    document the call-count contract in `MethodDispatchInput.implementsOf`
    JSDoc: `implementsOf` fires **per occurrence** in `input.owners`
    (not per unique owner); `computeMro` fires at most once per unique
    owner. Callers with expensive `implementsOf` implementations should
    pre-dedupe `owners`.

## abhigyanpatwari#916 (resolveTypeRef)
  - Document the deliberate exclusion of `'Type'` from `TYPE_KINDS`
    (verified no extractor in `gitnexus/src/core/ingestion/` emits
    `type: 'Type'` for annotation-relevant symbols).
  - Rename the namespace-origin test from `'resolves ...'` to
    `'returns null for a namespace-origin binding whose def is not a
    type kind'`, matching the failure-case intent.

## abhigyanpatwari#918 (shadow diff + aggregate)
  - Remove the partial re-export `export type { ShadowAgreement, ShadowDiff };`
    from `aggregate.ts` — it omitted `ShadowCallsite` and diverged
    from the top-level barrel. Consumers import all three from the
    `gitnexus-shared` entry point.
  - Fix the invalid `'wildcard'` evidence kind in `diff.test.ts` fixture
    (that kind is not a valid `ResolutionEvidence.kind`). Replaced with
    `'global-name'`, a real kind the test treats identically.

## abhigyanpatwari#912 (ScopeTree / PositionIndex / makeScopeId)
  - Document the touching-boundary semantics on `PositionIndex.atPosition`:
    when siblings share a boundary point, the right (later-start) sibling
    wins per the existing innermost-wins sort contract.
  - Resolve the layer-inversion flagged by review: move `ScopeLookup`
    from `resolve-type-ref.ts` to `types.ts` (its natural home in the
    data-model layer). `scope-tree.ts` now imports `ScopeLookup` from
    `types.js` directly; the old re-export from `resolve-type-ref.ts`
    is removed per repo convention (`feedback_no_reexport`). Barrel
    export moved alongside.

## abhigyanpatwari#917 (ClassRegistry / MethodRegistry / FieldRegistry)
  - Replace the dangling "try a name-match among class-like defs"
    comment in `lookupReceiverType` with explicit prose that callers
    must pre-resolve via `resolveTypeRef` if they want richer semantics.
    No behavior change — the function already returned `undefined` on
    ambiguous/missing qnames.
  - Fix `tieBreakKey.origin` default for pure Step-2 candidates.
    Type-binding-only hits no longer falsely inherit `'local'` from
    `ensureCandidate`'s neutral default; they now demote to `'import'`
    on their first type-binding hit, and only a later Step-1 lexical
    hit can upgrade them back to `'local'`. Keeps the Appendix B
    cascade faithful to the true origin.
  - Document `'global-name'` in `evidence.ts`: currently reserved for
    Ring 3's byName global index; `lookupCore` never emits it today.
    The weight stays live so `composeEvidence` remains exhaustive over
    the origin union.
  - Rename the mislabeled Step-7 test from `'confidence DESC is the
    primary key'` (which actually tested hard-shadow baseline) to
    `'inner scope shadows outer, yielding single result'`, and add a
    separate test that actually exercises multi-candidate confidence
    ordering (local vs wildcard at the same scope).

## Verification
  - `tsc --noEmit` clean (both `gitnexus-shared` and `gitnexus`)
  - `gitnexus-shared` build clean
  - Combined scope-resolution / model / shadow suite: **260/260 pass**
    (+1 from the new multi-candidate ordering test in abhigyanpatwari#917)

## Not addressed (non-actionable)
  - abhigyanpatwari#949 CI "failure with zero failing tests": pre-existing Swift Node 22
    grammar flake unrelated to abhigyanpatwari#910 scope.
  - abhigyanpatwari#950: the two non-blocking findings were already addressed in
    follow-up commit `cbac32ba` (ParsedImport discriminated union +
    `ScopeId | null` on the two hooks).
  - abhigyanpatwari#915: the five in-scope findings were already addressed in
    follow-up commit `54515a7e` (dead code, unused params, multi-hop
    docs, cap-hit test, stats granularity).
  - abhigyanpatwari#915 LanguageProvider.resolveImportTarget signature divergence +
    `findDefById` O(F×D) perf: tracked separately as follow-up issues
    for the Ring 3 migration window.

* chore(shared): address ce:review findings on the follow-up diff

ce:review (interactive) on PR abhigyanpatwari#964 surfaced two P2s and several P3s. This
commit applies all `safe_auto` fixes + both manual tests in-line so the
PR ships with a cleaner review trail.

## P2 fixes

- **Complete `freezeIndex` → `wrapIndex` rename.** The prior commit renamed
  3 of 5 sibling index files; `method-dispatch-index.ts` and
  `position-index.ts` still carried the old name. Now all 5 helpers use
  the consistent `wrapIndex` naming.
  (maintainability + project-standards reviewers both flagged this.)

- **Add regression tests for the `recordTypeBindingHit` origin demotion.**
  The prior commit introduced the `tieBreakKey.origin = 'import'`
  demotion for Step-2-only candidates without a direct test. Added:
    - `registries.test.ts`: two Step-2-only siblings under the same
      interface, asserting deterministic DefId.localeCompare tie-break
      AND the stronger invariant that composeEvidence never emits a
      where-found signal for Step-2-only candidates (no `signals.origin`).
    - `position-index.test.ts`: touching-boundary test proving the
      right-sibling-wins rule documented in the new JSDoc.
  (testing + kieran-typescript + api-contract reviewers all flagged these gaps.)

## P3 fixes

- Fix wrong comment in `recordTypeBindingHit` that claimed Step 1 could
  later upgrade a demoted origin. Step 1 runs BEFORE Step 2 — the actual
  upgrade path is Step 3 (`seedFromOwnerScopedContributor`). Comment now
  describes execution order correctly.

- Fix inaccurate "re-exported there" comment in `index.ts`. `types.ts`
  *defines* ScopeLookup natively; it's not a re-export. Phrasing now
  says "defined in types.ts and exported from the type-export block
  above — not from this module."

- Update stale `scope-tree.ts` file-header prose that still referenced
  `ScopeLookup` as living in abhigyanpatwari#916/resolve-type-ref.ts. Now points to
  `./types.js` with a cross-ref to both abhigyanpatwari#916 and abhigyanpatwari#917 consumers.

- Expand `atPosition` touching-boundary JSDoc to name the mechanism
  (backward scan through start-sorted array) so readers can trace the
  binary-search code to the claim.

- Add breadcrumb to `aggregate.ts` module header pointing future readers
  to `./diff.ts` / the top-level barrel for `ShadowAgreement`,
  `ShadowCallsite`, and `ShadowDiff`.

- Remove unnecessary non-null assertion in `recordTypeBindingHit`. Local
  `const existingMroDepth = ...` lets TS narrow to `number` in the
  else-branch, eliminating the `!` without behavior change.

## Verification

- `tsc --noEmit` clean (both `gitnexus-shared` and `gitnexus`)
- `gitnexus-shared` build clean
- Combined scope-resolution / model / shadow suite: **262/262 pass** (+2
  from the new origin-demotion + touching-boundary regression tests)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RING1-1: Add scope-resolution types + constants to gitnexus-shared

1 participant