Skip to content

feat(ingestion): add Kotlin scope resolver#1727

Merged
magyargergo merged 4 commits into
abhigyanpatwari:mainfrom
ShiningXu:codex/kotlin-scope-resolution-933
May 21, 2026
Merged

feat(ingestion): add Kotlin scope resolver#1727
magyargergo merged 4 commits into
abhigyanpatwari:mainfrom
ShiningXu:codex/kotlin-scope-resolution-933

Conversation

@ShiningXu

Copy link
Copy Markdown
Contributor

Summary

Closes #933.

Adds a Kotlin scope-resolution adapter for the RFC #909 Ring 3 pipeline, following the Java migration pattern from #1482:

  • add Kotlin scope captures for scopes, declarations, imports, type bindings, receivers, arity, local assignment aliases, and for-loop element bindings
  • add Kotlin import interpretation / target resolution, binding merge, receiver binding, arity compatibility, and companion-owner population
  • register kotlinScopeResolver in SCOPE_RESOLVERS
  • wrap Kotlin resolver integration tests with createResolverParityIt('kotlin')

Kotlin is intentionally not added to MIGRATED_LANGUAGES yet. Forced registry mode currently passes 154/175 Kotlin resolver fixtures, below the RFC #909 flip threshold, so default behavior remains legacy while the adapter is available for forced/shadow validation.

Validation

  • cd gitnexus && npx --no-install tsc --noEmit
  • cd gitnexus && npx --no-install vitest run test/integration/resolvers/kotlin.test.ts --reporter dot (175/175)
  • cd gitnexus && REGISTRY_PRIMARY_KOTLIN=1 npx --no-install vitest run test/integration/resolvers/kotlin.test.ts --reporter dot (154/175 forced-mode baseline)
  • cd gitnexus && node dist/cli/index.js detect-changes --scope staged --repo GitNexus (low risk, 0 affected processes)

Notes

Full npm test -- --run was attempted locally before this commit. Kotlin-specific validation passed, but the full suite had unrelated local environment/fixture failures: a corrupted onnxruntime-node native binding, existing mini-repo .gitignore state in CLI e2e, and pipeline golden drift from local fixture state.

@vercel

vercel Bot commented May 20, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions

github-actions Bot commented May 20, 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
9361 9360 0 1 495s

✅ All 9360 tests passed

1 test(s) skipped — expand for details
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 78.08% 30678/39287 N/A% 🟢 ███████████████░░░░░
Branches 66.63% 19546/29332 N/A% 🟢 █████████████░░░░░░░
Functions 82.31% 3109/3777 N/A% 🟢 ████████████████░░░░
Lines 81.46% 27648/33940 N/A% 🟢 ████████████████░░░░

📋 View full run · Generated by CI

@github-actions

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 12m 7s —— View job


PR #1727 Deep Review — Kotlin Scope Resolver

Tasks:

  • Post initial comment
  • Read repository guidance files (AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, ARCHITECTURE.md)
  • Read all 17 changed files
  • Read related files (Java resolver, registry, pipeline contracts, CI parity workflow)
  • Run branch hygiene checks (git diff, Unicode scan)
  • Inspect CI logs
  • Analyze findings across all 5 lanes
  • Post final review

Review bar for this PR

  • Kotlin must stay off MIGRATED_LANGUAGES; default production path must not change.
  • kotlinScopeResolver registration must be safe to sit idle until the flip threshold is reached.
  • Forced-mode correctness must be provably validated (CI or documented risk at Java-pattern parity).
  • Scope captures, import handling, receiver binding, arity, companion owner, and merge logic must be semantically coherent even at 88% parity.
  • Hidden Unicode/bidi controls are a hard block anywhere in executable code or queries.

Current PR state

  • Status: Open. Single commit dda0e73. 17 changed .ts files — all Kotlin scope-resolution or the shared registry/test file. No reviews submitted.
  • CI (verified): github-actions bot comment reports 9266 passed, 0 failed, 1 skipped (Ruby block-param — documented skip). Typecheck, unit, E2E all green.
  • Scope parity CI: ci-scope-parity.yml auto-discovers MIGRATED_LANGUAGES. Kotlin is not in MIGRATED_LANGUAGES, so the parity workflow ran zero Kotlin-specific steps. The 154/175 forced-mode number comes from local execution only.
  • Hidden Unicode scan: git diff HEAD~1 piped through bidi/ZWS grep returned no matches. GitHub's bidirectional Unicode warning in the diff view does not correspond to any actual bidi or hidden control character in the changed files.

Branch hygiene assessment

Verdict: clean feature/fix PR

Single commit touching only gitnexus/src/core/ingestion/languages/kotlin/ (14 new files), kotlin.ts (hook wiring), scope-resolution/pipeline/registry.ts (+2 lines), and test/integration/resolvers/kotlin.test.ts (+4/-1). No unrelated workflow changes, no package-lock drift, no generated artifacts, no formatting churn. The commit graph is linear off the previous main-equivalent commit.


Understanding of the change

  • Adds 14 new Kotlin-only files implementing ScopeResolver: scope captures, import decomposition/target resolution, type binding interpretation, merge logic, arity metadata, receiver binding, companion/owner population, and a query.ts tree-sitter scope query.
  • Wires all RFC Meta: Pure Scope-Based Resolution in the Semantic Model #909 Ring 3 hooks (emitScopeCaptures, interpretImport, interpretTypeBinding, bindingScopeFor, importOwningScope, mergeBindings, receiverBinding, arityCompatibility, resolveImportTarget) into the existing kotlinProvider in kotlin.ts.
  • Registers kotlinScopeResolver in SCOPE_RESOLVERS without touching MIGRATED_LANGUAGES. Default Kotlin behavior remains the legacy DAG.
  • Wraps Kotlin integration tests with createResolverParityIt('kotlin') — the helper skips named tests in legacy mode when they appear in LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES['kotlin']. That key is absent from helpers.ts, so all 175 Kotlin tests run in both modes whenever the env var is set.
  • scope-resolver.ts explicitly documents forced-mode baseline (154/175, 88%), known gaps (smart-cast, cross-file iterable propagation, overload target-id, virtual dispatch, interface defaults), and migration-pattern rationale matching Java (feat: migrate Java to scope-based registry resolution (RFC #909 Ring 3) #1482).

Findings


Finding: CI forced-mode Kotlin validation is absent

Severity: high

File(s): gitnexus/src/core/ingestion/languages/kotlin/scope-resolver.ts, .github/workflows/ci-scope-parity.yml

Evidence: ci-scope-parity.yml is driven solely by MIGRATED_LANGUAGES. Kotlin is intentionally absent, so the parity workflow's discover step emits zero Kotlin matrix entries. The 154/175 forced-mode baseline exists only as local documentation in scope-resolver.ts. No CI step runs REGISTRY_PRIMARY_KOTLIN=1 vitest run kotlin.test.ts. Future commits can regress forced-mode without CI alerting.

Risk: Any regression in the forced/shadow adapter goes undetected until manual re-run before the flip. The 154 passing forced-mode fixtures carry no CI-enforced regression protection.

Recommended fix: Add a non-required CI step (e.g., a ci-scope-parity-preview.yml or an [allowed-fail] matrix row) that runs Kotlin in forced mode and posts the pass/fail count as a PR check annotation. The Java resolver PR (#1482) documents the same gap in its JSDoc and explicitly calls it out as "must fix before flip." Apply the same treatment here so the baseline is CI-tracked, not doc-only.

Blocks merge: maybe — this is the same pattern accepted for Java (#1482), so precedent allows it; but the DoD §12 requires CI scope-parity coverage. Whether the Java precedent overrides the DoD is a maintainer judgment call.


Finding: Companion member owner assignment makes companion methods graph-indistinguishable from instance methods

Severity: high

File(s): gitnexus/src/core/ingestion/languages/kotlin/owners.ts

Evidence: populateCompanionMembersOnEnclosingClass walks Function scopes inside Class scopes where the parent Class scope has no class-like ownedDefs. For an unnamed companion object { fun bar() {} }, the companion's Class scope has no class-like def (the @declaration.class lands in the enclosing class's scope, not the companion's own scope), so the guard passes. bar's ownerId is then set to the enclosing class Foo's nodeId via findEnclosingClassWithDef. Result: bar is owned by Foo, qualifiedName = "Foo.bar", identical to a regular instance method.

Risk: In forced mode (REGISTRY_PRIMARY_KOTLIN=1), receiver-bound call resolution for fooInstance.bar() (invalid Kotlin — companion members are not callable on instances) and Foo.bar() (valid) both produce the same call edge. Companion-dispatch filtering is absent. DoD §9: "must not make companion members globally visible incorrectly."

Recommended fix: Distinguish companion methods from instance methods by keeping companion defs under the companion object's nodeId as the primary owner. Add a secondary edge or qualifier (e.g., companionOf: enclosingClassNodeId) for Foo.bar() resolution without merging into instance method visibility. Alternatively, mark companion defs with a isCompanion: true flag that arityCompatibility/receiver-bound passes can gate on.

Blocks merge: maybe — production default is legacy; forced-mode only. Pre-flip blocker per DoD §9.


Finding: resolveKotlinImportTarget always returns null when called via the languageProvider.resolveImportTarget hook (dead code in legacy path)

Severity: medium

File(s): gitnexus/src/core/ingestion/languages/kotlin/import-target.ts, gitnexus/src/core/ingestion/languages/kotlin/kotlin.ts

Evidence: kotlin.ts registers resolveImportTarget: resolveKotlinImportTarget as a languageProvider hook. The hook receives a real WorkspaceIndex (from the framework). resolveKotlinImportTarget immediately casts it to KotlinResolveContext and checks typeof ctx.fromFile !== 'string' || !(ctx.allFilePaths instanceof Set). A real WorkspaceIndex has neither field, so the function returns null on every call via this path. The function only works correctly when called from scope-resolver.ts which constructs and passes a KotlinResolveContext manually.

Risk: No production regression (legacy path uses createImportResolver(kotlinImportConfig), not resolveImportTarget; scope-resolver path is correctly wired). But a future caller relying on languageProvider.resolveImportTarget for Kotlin would silently get null, breaking import target resolution without error.

Recommended fix: Either remove the languageProvider.resolveImportTarget assignment in kotlin.ts (since the function only works via the ScopeResolver wrapper) or refactor resolveKotlinImportTarget to accept both call shapes safely. Fix this →

Blocks merge: no — no production impact.


Finding: extensionFreeCallFallback generates a spurious free-call capture for every complex-receiver member call, not just extension function calls

Severity: medium

File(s): gitnexus/src/core/ingestion/languages/kotlin/captures.ts:444-468

Evidence: extensionFreeCallFallback fires whenever @reference.call.member has a receiver that is not a simple_identifier. The guard is receiverNode.type === 'simple_identifier' at line 456 — if receiver is more complex (e.g., a chained navigation expression, a call result, a this reference), a synthetic @reference.call.free is emitted. This is correct for listOf(...).filter { } where filter might be an extension function, but it also fires for someObject.someRegularMethod() when someObject is a chained expression. Every such call generates an additional free-call candidate lookup.

Risk: In forced mode, complex-receiver member calls generate both a member-call edge AND a spurious free-call edge. If a free-call candidate named identically to the member method exists in scope, a false CALLS edge is emitted. Contributes to the 21 forced-mode fixture failures.

Recommended fix: Narrow the guard to only synthesize the free-call fallback when the member's @reference.name matches a known extension function name in the current file's imports or declarations (requires a light lookup), or document this as a known over-emission and suppress the synthetic capture only when the member call is already definitively resolved.

Blocks merge: no — Kotlin not in MIGRATED_LANGUAGES; contributes to known forced-mode gap.


Finding: Trailing lambda arguments are not counted by callArguments(), causing false arity-incompatible verdicts for trailing-lambda call sites

Severity: medium

File(s): gitnexus/src/core/ingestion/languages/kotlin/captures.ts:412-417

Evidence: callArguments looks for call_suffix → value_arguments → value_argument children. Kotlin trailing lambdas are represented as call_suffix → annotated_lambda → lambda_literal, not as value_argument nodes. A call like items.forEach { println(it) } produces arity=0 from callArguments. If forEach's definition has requiredParameterCount=1 (the lambda param, which is not optional and not vararg), kotlinArityCompatibility returns 'incompatible', excluding this candidate. This would cause all no-explicit-arg trailing-lambda calls to fail arity gating.

Risk: Very common Kotlin pattern (let, run, apply, forEach, map, filter, etc.) would generate arity-incompatible verdicts in forced mode, suppressing call edges. Likely a significant contributor to the 21 forced-mode failures.

Recommended fix: Extend callArguments to count trailing annotated_lambda children in call_suffix as arguments. Alternatively, when value_arguments is absent but annotated_lambda is present, return [lambdaNode] (count of 1). Fix this →

Blocks merge: no — forced-mode only. Pre-flip blocker.


Finding: @type-binding.extension-receiver query pattern creates a spurious self-type binding keyed by the function name, not by this

Severity: low

File(s): gitnexus/src/core/ingestion/languages/kotlin/query.ts:45-52, gitnexus/src/core/ingestion/languages/kotlin/interpret.ts:45

Evidence: The query pattern captures (simple_identifier) @type-binding.name for extension functions, where simple_identifier is the function name (e.g., slug in fun String.slug()). interpretKotlinTypeBinding sets source = 'self' when @type-binding.extension-receiver is present, so it emits {boundName: 'slug', rawTypeName: 'String', source: 'self'}. Separately, synthesizeKotlinReceiverBinding correctly emits {name: 'this', type: 'String', source: 'self'}. Both enter the scope. The spurious 'slug' binding has source='self' and could confuse any framework pass that inspects source='self' bindings for anything other than the this/super name lookup in kotlinReceiverBinding (which safely ignores it by only looking up 'this').

Risk: Currently harmless because kotlinReceiverBinding only reads 'this' and 'super'. If a future framework pass uses the full typeBindings map to enumerate self-type entries, 'slug' would appear as a self-reference for type 'String'.

Recommended fix: Change @type-binding.name in the extension-receiver query pattern to a synthetic name literal (e.g., remove the extension-receiver query pattern entirely since synthesizeKotlinReceiverBinding covers it correctly).

Blocks merge: no — harmless under current framework semantics.


Finding: Global mutable cache stats in cache-stats.ts accumulate across all test runs in a process without auto-reset

Severity: low

File(s): gitnexus/src/core/ingestion/languages/kotlin/cache-stats.ts

Evidence: Module-level let hits = 0; let misses = 0. resetKotlinCaptureCacheStats() is exported but not called in any test beforeEach/afterEach hook visible in kotlin.test.ts. Stats accumulate across all 175 test cases. Does not affect resolution results.

Risk: Metric-only; no resolution correctness impact. Nondeterministic stats if order changes. Not a blocker.

Recommended fix: Call resetKotlinCaptureCacheStats() in a beforeAll or afterAll in kotlin.test.ts if cache-hit-rate assertions are ever added.

Blocks merge: no


Kotlin static-analysis semantics

The lexical scope model (file/class/object/companion/function) is architecturally sound. Key gaps for the flip but acceptable for staged adapter:

  • Lambda scopes (lambda_literal) are not opened — lambda parameters are invisible to the resolver. All 14 standard library higher-order functions (forEach, map, filter, let, run, apply, etc.) would fail receiver binding inside lambda bodies.
  • for-loop and local-assignment binding synthesis is correct and carefully scoped inside function_declaration only.
  • normalizeKotlinType correctly strips nullability, qualifiers, and collection wrappers; the Map/collection erasure to element type is intentional and consistent with Java's JVM type-erasure pattern.
  • The extension-receiver spurious binding (Finding above) is currently harmless.

Kotlin/JVM interpreter semantics

  • kotlinArityCompatibility correctly handles default params (via requiredParameterCount), varargs (unbounded max), unknown arity (returns 'unknown' not 'incompatible'), and too-few/too-many args.
  • Trailing lambda arity undercounting (Finding above) is the principal forced-mode defect here.
  • Companion member visibility is over-inclusive (instance-method confusion) per Finding above.
  • mergeBindings deduplicates by def.nodeId, not by short name — it cannot produce cross-package symbol collapse. The tier-priority logic (local → import/namespace → wildcard → other) matches Kotlin's import resolution rules.

GitNexus scope-resolution integration

  • Kotlin is correctly registered in SCOPE_RESOLVERS and absent from MIGRATED_LANGUAGES. The scopeResolutionPhase iterates SCOPE_RESOLVERS ∩ MIGRATED_LANGUAGES, so the Kotlin resolver is idle in default mode. Verified in ci-scope-parity.yml — Kotlin did not appear in the discovered matrix.
  • hoistTypeBindingsToModule: true and kotlinBindingScopeFor follow the identical Java pattern (confirmed in java/scope-resolver.ts:94 and java/simple-hooks.ts). The same name-collision risk for same-name methods in different classes exists in Java; it is a known architectural limitation of the module-scope hoisting approach.
  • fieldFallbackOnMethodLookup: false is correct for statically typed Kotlin.
  • propagatesReturnTypesAcrossImports: true is correct.
  • The mergeBindings wrapper in kotlin.ts drops the _scope argument. For Kotlin's LEGB-like lookup, scope-independent merge is sufficient at 88% parity; no regression risk.
  • resolveKotlinImportTarget WorkspaceIndex cast (Finding above) is dead code for the LanguageProvider path — no production impact.

Tests and CI coverage

  • All 175 Kotlin tests pass in default mode (CI verified).
  • createResolverParityIt('kotlin') correctly wraps tests; LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES['kotlin'] is absent, so no tests are pre-skipped in either mode — all 175 run in forced mode.
  • Forced-mode 154/175 is local-only — CI gap per Finding Add support for Ollama as a local inference backend #1.
  • The test file covers heritage, cross-file import, extension function dispatch, companion object member access, data/sealed class construction, vararg, and default param fixtures (visible from the fixture structure). Semantic negative assertions (e.g., "companion method does NOT resolve on instance receiver") are not visible in the first 100 lines; the full 175-test file may include them.
  • Known flip-blockers are explicitly enumerated in scope-resolver.ts JSDoc. This mirrors the Java PR pattern.

Security, hidden Unicode, and hygiene

  • No hidden Unicode, bidi controls, or ZWS found in the diff. The GitHub rendered-diff warning is a false positive from GitHub's display, not from actual content.
  • No dynamic query construction — the tree-sitter query string is a static literal in query.ts. No injection risk.
  • No generated artifacts, no .d.ts or dist/ changes.
  • descendants() is O(n²) in nesting depth due to array spread at each recursion level. For large files this degrades ingestion throughput; not a correctness issue.
  • The module-level parser/query singletons in query.ts are safe for single-threaded test runs. Worker threads each get their own module instance. No concurrency risk in practice.

Branch, diff, and review-state confidence

  • Branch hygiene is fully verifiable: single commit, 17 Kotlin-only files + 2-line registry change + 4-line test change. No ambiguous content.
  • CI report is authoritative for default-mode behavior. The 9266/9267 pass rate is trustworthy.
  • Forced-mode parity (154/175) is not CI-verified — this is explicitly the same limitation documented in the Java PR (feat: migrate Java to scope-based registry resolution (RFC #909 Ring 3) #1482). It is a known risk, not a hidden one.
  • No active reviews; no approval required to merge (per repo policy).

Production readiness

not production-ready


Final verdict

not production-ready

The PR correctly stages the Kotlin scope resolver as a forced/shadow adapter with no change to default production behavior (Kotlin absent from MIGRATED_LANGUAGES, confirmed in code and CI). However, per the PR-specific DoD: §9 is violated because companion members are assigned to the enclosing class owner with no instance/static distinction, making companion-only methods graph-identical to instance methods in forced mode; §11 requires coverage for the trailing-lambda arity false-incompatible path (a very common Kotlin pattern); and §12 requires CI-enforced forced-mode validation, which is absent. These are all pre-flip blockers — the same category tracked in the Java PR (#1482) and not yet resolved there either. If the maintainer accepts the Java precedent (merge adapter now, track flip-blockers in follow-up issues), the verdict upgrades to production-ready with minor follow-ups gated on the three pre-flip issues enumerated above. The decision on that precedent is a maintainer call, not a technical one.
· PR branch

@magyargergo

Copy link
Copy Markdown
Collaborator

@ShiningXu can you please look into claude's findings above?

@ShiningXu

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I pushed 9d2f7c76 addressing the local correctness findings that fit this staged Kotlin adapter PR:

  • Removed the dead languageProvider.resolveImportTarget wiring for Kotlin. resolveKotlinImportTarget now remains used through the ScopeResolver wrapper where the context shape is correct.
  • Removed the extension-receiver type-binding query that produced a spurious source='self' binding keyed by the extension function name. Extension receivers are still handled by synthesizeKotlinReceiverBinding as this.
  • Count trailing annotated_lambda call suffixes as call arguments, so calls like items.forEach { ... } get arity 1 instead of 0.
  • Narrowed extensionFreeCallFallback to literal receivers only, avoiding synthetic free-call candidates for chained regular member calls like service.current().save().
  • Added test/unit/kotlin-scope-captures.test.ts covering those capture-level regressions.

Validation after the fix:

  • cd gitnexus && npx --no-install tsc --noEmit
  • cd gitnexus && npx --no-install vitest run test/unit/kotlin-scope-captures.test.ts test/integration/resolvers/kotlin.test.ts --reporter dot (179/179)
  • cd gitnexus && REGISTRY_PRIMARY_KOTLIN=1 npx --no-install vitest run test/unit/kotlin-scope-captures.test.ts test/integration/resolvers/kotlin.test.ts --reporter dot (158/179; Kotlin integration forced-mode baseline remains 154/175)

On the two larger findings:

  • Companion member owner/static distinction: agreed as a pre-flip blocker. I did not expand this PR into generic receiver-bound dispatch changes because Kotlin is still absent from MIGRATED_LANGUAGES, matching the Java feat: migrate Java to scope-based registry resolution (RFC #909 Ring 3) #1482 staged-adapter pattern. The limitation remains documented in the forced-mode baseline/gap list.
  • CI forced-mode Kotlin preview: agreed as observability debt before flipping. Since forced-mode intentionally fails today, adding a normal required parity row would make the PR red. I left this as a follow-up unless maintainers want a non-required/continue-on-error preview workflow in this PR.

@magyargergo

Copy link
Copy Markdown
Collaborator

Please open a follow-up ticket with subtasks similarly to #1564

@ShiningXu

Copy link
Copy Markdown
Contributor Author

Opened the follow-up tracking ticket with subtasks here: #1746\n\nIt keeps Kotlin out of MIGRATED_LANGUAGES for this staged adapter PR, and tracks the forced-mode CI preview plus the remaining pre-flip resolver gaps separately.

@magyargergo magyargergo merged commit 1b5c6e5 into abhigyanpatwari:main May 21, 2026
30 of 31 checks passed
@magyargergo

Copy link
Copy Markdown
Collaborator

Thank you! Please create subtasks for each items so, we can work on it parallel if possible. Please also use GitHub API to setup the dependencies between each tasks 🙏

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.

LANG-kotlin: Migrate Kotlin to scope-based resolution

2 participants