chore(shared): apply Ring 2 SHARED review follow-ups in one diff#964
Merged
magyargergo merged 2 commits intoApr 18, 2026
Merged
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 6700 tests passed 97 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
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)
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:
Your task has 2 phases, in this exact order: PHASE 1 — DEFINE THE BAR PHASE 2 — REVIEW THE PR AGAINST THAT BAR CONTEXT TO LOAD FIRST
Additional context:
PRIMARY OBJECTIVE Do not optimize for completeness at the expense of signal. REVIEW RULES
For each finding, assign one severity:
REPO-SPECIFIC REVIEW CHECKLIST 1. Correctness & functional completenessCheck:
2. Code clarity & clean codeCheck:
3. Test coverage & change safetyEvaluate against TESTING.md:
4. PerformanceInspect for:
5. Operational riskCheck:
6. MaintainabilityCheck:
NON-NEGOTIABLES TO VERIFY
OUTPUT FORMAT Production-ready bar for this PRGive a concise repo-specific definition in 5–10 bullets. FindingsFor each finding, use this structure exactly: [BLOCKING|NON-BLOCKING|NIT] Short title
Only include findings that are real and supported.
What I could not fully verifyList any behavior you could not prove from the diff, tests, or available context. Final verdictChoose exactly one:
If verdict is not ready, list the blocking items in priority order. IMPORTANT REVIEW BEHAVIOR
You must also ensure we are on track with the RFC plan! Check the issue tickets involved! |
Contributor
|
Claude finished @magyargergo's task in 4m 13s —— View job PR Review: chore(shared): apply Ring 2 SHARED review follow-ups in one diffTasks:
Production-ready bar for this PRBased on AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md, and the affected area (
Findings1. Correctness & functional completeness
The first commit missed
Evidence: if (firstHit && state.signals.origin === undefined) {
state.tieBreakKey.origin = 'import';
}The guard
The removed
Old comment at the
2. Code clarity & clean codeBFS vs C3 test differentiation — correct and meaningful The old BFS test used [NIT] Overlong JSDoc line in
|
| Test | File:block | Covers |
|---|---|---|
| Touching-boundary right-sibling-wins | position-index.test.ts:436-452 |
New JSDoc invariant; exercises the backward scan tie-break |
implementsOfCalls counter |
method-dispatch-index.test.ts:138-425 |
Pins the implementsOf-per-occurrence contract |
| Hit-path freeze assertion | qualified-name-index.test.ts:464-466 |
Proves frozen bucket on hit path, not just miss path |
| Multi-candidate local>wildcard ordering | registries.test.ts:496-515 |
First genuine confidence-DESC ordering test in Step 7 |
| Step-2 demotion + evidence kind invariant | registries.test.ts:524-575 |
Pins tieBreakKey.origin='import' demotion AND asserts no signals.origin in evidence output |
All assertions are non-tautological. The Step-2 demotion test in particular is well-constructed: it asserts the stronger observable invariant (no origin signal in evidence) rather than the weaker internal state, making the test robust to implementation changes.
CI confirms: 260/260 scope-resolution tests pass (6698/6698 total). TypeCheck clean.
[NON-BLOCKING] PR description opening line vs. actual change
- Evidence: PR body: "No behavior changes" vs. the
recordTypeBindingHitdemotion fix described in the#917table row - Why it matters: The opening sentence says "no behavior changes" but the Step-2 origin demotion IS a ranking behavior change (though it's a correctness fix for a pre-existing bug, not a new feature). The detailed table in the description is accurate. Reviewers scanning only the lede could be misled about the scope.
- Recommendation: Change "No behavior changes" to "No new features; one latent ranking bug fixed (
tieBreakKey.origindemotion for Step-2-only candidates) — see#917row." - Confidence: High
4. Performance
No hot-path changes. The demotion fix adds two field assignments and one boolean check per recordTypeBindingHit invocation — negligible. The collectOwnedMembers O(D) scan (lookup-core.ts:341) is pre-existing and acknowledged in the comment ("tracked as a follow-up optimization before Ring 3"). No new allocations, no new Cypher paths.
- No issues found in this area based on the diff and reviewed context.
5. Operational risk
-
No changes to ingestion phases, LadybugDB schema, MCP tool contracts, embeddings, or staleness logic.
-
The
buildModuleScopeIndexJSDoc caller contract (path normalization warning) is documentation-only; no runtime behavior altered. -
Rollback is safe: all changes are in
gitnexus-shared/types and tests; no runtime state migration required. -
No cross-platform path handling changes introduced (despite the warning about
C:\vsC:/in the new JSDoc — that warning is additive documentation for callers, not a behavior change). -
No issues found in this area based on the diff and reviewed context.
6. Maintainability
-
ScopeLookupis now correctly anchored in the data-model layer (types.ts), resolving the layer inversion flagged in Ring 2 review. Future §4 modules can depend on it without coupling to each other. -
aggregate.tsmodule-level comment points consumers to the canonical barrel, so the next contributor won't re-introduce the partial re-export. -
wrapIndexnaming is consistent across all 5 index builders — no stalefreezeIndexreference remains. -
Pipeline Phase DAG boundaries: not touched — PR is entirely in
gitnexus-shared/, which sits above the DAG layer. -
No issues found in this area based on the diff and reviewed context.
Non-negotiables (GUARDRAILS.md / CONTRIBUTING.md)
| Check | Status |
|---|---|
| No secrets or machine-specific paths | ✅ |
No blind find-and-replace — wrapIndex rename was deliberate, applied file-by-file |
✅ |
Upstream impact considered for shared symbol move (ScopeLookup) |
✅ — barrel route unchanged for consumers |
| No HIGH/CRITICAL impact left unaddressed | ✅ |
PR title: chore(shared): ... — valid conventional commit |
✅ |
| PolyForm license untouched | ✅ |
RFC Plan Tracking (RFC #909)
Based on recent commit history and PR references:
- Ring 2 SHARED (feat(shared): add scope-resolution types + constants (#910, RFC #909 Ring 1) #949–feat(shared): ClassRegistry / MethodRegistry / FieldRegistry + 7-step lookup (#917, RFC #909 Ring 2 SHARED) #963): merged (commits
8cf9ae0through1bf9fb4on branch) - Ring 2 SHARED follow-up (chore(shared): apply Ring 2 SHARED review follow-ups in one diff #964, this PR): addresses all actionable review findings across the 9 SHARED PRs; PR description provides explicit audit trail per PR number
- Ring 2 PKG (RING2-PKG-1: ScopeExtractor driver (tree-sitter + provider hooks → shared ScopeTree) #919–RING2-PKG-7: ReferenceIndex emission to LadybugDB #925): not yet started — gated on this PR per the stated sequencing
- Two cross-cutting items explicitly deferred to Ring 3 (
LanguageProvider.resolveImportTargetdivergence;findDefByIdO(F×D) scan) — deferred items are documented in PR body and acknowledged as out-of-scope for Ring 2
The sequencing is on track. No Ring 2 SHARED item appears to have been dropped silently.
What I could not fully verify
- Whether any external consumer uses a direct subpath import from
gitnexus-shared/dist/scope-resolution/aggregate.jsforShadowAgreement/ShadowDiff— would break silently. Internal usage withingitnexus/would be caught bytsc --noEmit(confirmed clean by CI), but a downstream consumer in a separate package would not. Low risk given the AGENTS.md convention of barrel-only imports. - Whether
gitnexus/src/core/ingestion/currently importsScopeLookupfrom the old path — thetscclean result confirms no broken imports exist, but I did not independently trace all import sites. - Ring 2 PKG issues RING2-PKG-1: ScopeExtractor driver (tree-sitter + provider hooks → shared ScopeTree) #919–RING2-PKG-7: ReferenceIndex emission to LadybugDB #925 contents — cannot access GitHub issue bodies directly to confirm no SHARED prerequisite was missed.
Final verdict
✅ Ready to merge — no blocking issues.
The one NON-BLOCKING finding (PR description lede vs. actual change) is a documentation precision issue only and doesn't affect correctness or safety. The NIT is trivial. All code logic is verified correct, all new tests are substantive, CI is fully green (6698/6698, tsc clean), and the RFC Ring 2 SHARED loop is properly closed. Ready to proceed to Ring 2 PKG.
magyargergo
deleted the
rfc/scope-resolution/ring2-shared-review-followups
branch
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#949–abhigyanpatwari#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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Aggregates 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 (
ScopeLookupmigration totypes.ts).What's in this diff
#913 — DefIndex / ModuleScopeIndex / QualifiedNameIndex
freezeIndex→wrapIndexin all three buildersObject.freezeon the wrapper, which we never applied.wrapIndexdescribes the lightweight readonly-interface wrap accurately.buildModuleScopeIndexcallers must pre-normalizefilePathC:\vsC:/) or trailing-slash misses.EMPTYarray.#914 — MethodDispatchIndex
implementsOfCallscounter + document call-count contractimplementsOffires per occurrence ininput.owners, not per unique owner.computeMrois memoized. Callers with expensiveimplementsOfneed this.#916 — resolveTypeRef
'Type'NodeLabel exclusion fromTYPE_KINDSgitnexus/src/core/ingestion/emits it for annotation-relevant symbols.'resolves ...'→'returns null for a namespace-origin binding whose def is not a type kind'matches intent.#918 — shadow diff + aggregate
export type { ShadowAgreement, ShadowDiff }fromaggregate.tsShadowCallsite, diverged from the top-level barrel.'wildcard'evidence kind indiff.test.tsfixtureResolutionEvidence.kind; test uses'global-name'now (a real kind the test treats identically).#912 — ScopeTree / PositionIndex
atPositionScopeLookupinterface fromresolve-type-ref.tstotypes.tsscope-tree.tsshouldn't import from a §4 module. Home is the data-model layer (types.ts). No convenience re-export — perfeedback_no_reexportpreference, the one call site (scope-tree.ts) updated directly, and the top-level barrel moves alongside.#917 — Class / Method / Field registries
lookupReceiverTyperesolveTypeRefon ambiguous qnames; no phantom fallback.tieBreakKey.origindefault for pure Step-2 candidates'local'from the neutral default. Now demoted to'import'on first type-binding hit; a later Step-1 lexical hit can still upgrade to'local'. Keeps the Appendix B cascade faithful to the true origin.'global-name'origin as Ring 3 reservedcomposeEvidenceremains exhaustive over the origin union.'confidence DESC is the primary key'test actually exercised hard-shadow baseline. Renamed + added a genuine multi-candidate local-vs-wildcard ordering test.Verification
tsc --noEmitclean (both packages)gitnexus-sharedbuild cleanNot addressed (non-actionable, documented in commit)
cbac32ba(ParsedImport discriminated union +ScopeId | nullon hook signatures).54515a7e(dead code, unused params, multi-hop re-export docs, cap-hit test, stats granularity). The two cross-cutting follow-ups (LanguageProvider.resolveImportTargetsignature divergence;findDefByIdO(F×D) scan) are tracked separately for the Ring 3 migration window.Closes the Ring 2 SHARED review loop
With this diff merged, all actionable inline review feedback across the 9 Ring 2 SHARED PRs is addressed. Ready to proceed to Ring 2 PKG (#919–#925).