Skip to content

fix(ingestion): qualify Ruby same-tail nested mixin modules + route IMPLEMENTS by scope (#1991)#2006

Merged
magyargergo merged 3 commits into
mainfrom
fix/1991-ruby-mixin-module-trait
Jun 4, 2026
Merged

fix(ingestion): qualify Ruby same-tail nested mixin modules + route IMPLEMENTS by scope (#1991)#2006
magyargergo merged 3 commits into
mainfrom
fix/1991-ruby-mixin-module-trait

Conversation

@magyargergo

Copy link
Copy Markdown
Collaborator

Stacked PR 4/5 — base fix/1993-cpp-ns-qualified-inheritance (#2005). Auto-retargets when #2005 merges.

Summary

Follow-up to #1981 / #1982. A Ruby module maps to the Trait label but is not a typeDeclaration, so the structure phase never qualified its node id. Two same-tail nested mixin modules (App::Loggable + Web::Loggable) collapsed onto one Trait:f.rb:Loggable node, and the bare-name include Loggable reference cross-wired IMPLEMENTS via the first-wins tail fallback (dangle-free but wrong).

Fix

Structure phase (node identity). Exposed buildQualifiedName as a qualifyScopeName ClassExtractor hook and threaded it for Trait nodes in parsing-processor.ts + parse-worker.ts (lockstep), so a module node keys by its qualified scope path (App.Loggable). This is not Option A — Trait is not in CLASS_LIKE_LABELS and the qualified-id selection gates it out, and extractQualifiedName bails on non-typeDeclarations; qualifyScopeName bypasses that gate. getQualifiedOwnerName also falls back to qualifyScopeName, so methods inside a nested module own through the same qualified Trait id (no dangling HAS_METHOD).

Resolution (cross-wire). emitRubyMixinEdges now resolves a bare mixin reference lexically by the including class's enclosing scope (App::S + LoggableApp::Loggable), and the simple-tail fallback is delete-on-collision (refuse to guess on a same-tail tie) instead of first-wins.

Module → Trait is preserved; Trait is not added to CLASS_LIKE_LABELS. No language-specific names in shared code — qualifyScopeName is a neutral hook gated on qualifiedNodeId + the Trait label.

Tests

New single-file ruby-nested-mixin-tail-collision fixture + tests:

  • Two distinct Trait nodes (App.Loggable / Web.Loggable).
  • S IMPLEMENTS App.Loggable only, T IMPLEMENTS Web.Loggable only; one edge each; no dangling IMPLEMENTS / HAS_METHOD.
  • Worker-path parity block.

Verification

Closes #1991.

🤖 Generated with Claude Code

@vercel

vercel Bot commented Jun 3, 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 Jun 4, 2026 10:19am

Request Review

@magyargergo

Copy link
Copy Markdown
Collaborator Author

Tri-review — approve-with-nits

Method: per-PR multi-lens review → independent adversarial re-verification → static-analysis alignment (tsc/eslint/prettier, all clean) → cross-stack synthesis + critic gate.

Review: PR #2006 — Ruby same-tail nested mixin-module Trait qualification (#1991)

Verdict: Approve with nits. This is a clean, well-scoped fix that lands on the real runtime path, holds the language-neutrality line, and ships the right tests (positive node+edge identity, not dangle-only, with worker-parity and exact-match legacy-parity skips). tsc --noEmit is clean and the golden is additive; CHANGELOG is untouched.

What I verified

  • Correctness (structure phase). A Ruby module is labeled Trait and is not a typeDeclaration (ruby.ts typeDeclarationNodes: ['class']), so extractQualifiedName bails. The new qualifyScopeName hook reuses buildQualifiedName, which walks ancestorScopeNodeTypes ['module','class'] from node.parent upward → App.Loggable / Web.Loggable, anchored on the @declaration.trait module node. Both qualifiedTypeName and qualifiedName gate the new branch on qualifiedNodeId === true.
  • Correctness (resolution). emitRubyMixinEdges now resolves the bare mixin via full-qn → qualifyMixinByOwnerScope (innermost-enclosing-scope-first, matching Ruby constant lookup) → tail map, with the tail map carrying a null collision sentinel so the trailing ?? undefined correctly refuses to guess on a same-tail tie (replacing the old cross-wiring first-wins). The __heritage__ owner is the full qualified name (App.S), and isClassLike includes Trait so module defs populate graphIdByName.
  • Cross-language safety. The new getQualifiedOwnerName ?? qualifyScopeName fallback is reachable only when extractQualifiedName returns null. For C++ (the only other qualifiedNodeId language) every owner-container type is a typeDeclaration and there is no bare module AST node, so the fallback is Ruby-active-only and C++ owner ids are unchanged.
  • Neutrality. No language names in executable shared code — only in comments. Gating is on the generic Trait registry label + the qualifiedNodeId flag via the optional qualifyScopeName? hook. Trait is correctly kept out of CLASS_LIKE_LABELS and the inheritance machinery (explicitly not Option A).
  • Tests. Distinct-Trait-node assertion runs on both legs (structure-phase fix is leg-independent); routing tests resolve rel.targetId and assert the target's qualifiedName + exactly-one-edge-per-source; worker-parity block re-runs through the pool with a usedWorkerPool guard; the three registry-primary-only test names match the LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES['ruby'] registrations byte-for-byte.
  • Sequential ↔ worker paths are a faithful lockstep.

Strengths

Findings (all minor/nit)

  1. [minor] Persisted node-id keyspace splitparsing-processor.ts:682-694. Ruby Trait node ids change to the qualified form; CachedEmbedding is keyed by nodeId, so a default (non-clean) re-analyze skips embedding regen for the split nodes until a full reindex. Same property as the rest of the stack, but Trait/module is a new node class crossing the split — worth a one-line reindex note in the PR/issue.
  2. [nit] nodeLabel === 'Trait' hardcoded in 4 spots across two filesparsing-processor.ts:657,688 + parse-worker.ts:1865,1890. Ruby-only today (only Ruby/C++ set qualifiedNodeId, C++ emits no Trait), but if PHP/Dart/Rust adopt the flag their Traits auto-qualify. Consider a shared isQualifiableScopeLabel() predicate + a one-line comment. Readability only.
  3. [nit] qualifyMixinByOwnerScope docscope-resolver.ts:22-36. Resolves against the full-qn map only; a note that graphIdByName is per-scope-path unique (so the lexical walk can't itself produce a same-tail tie — that's the graphIdByTail null sentinel's job) would aid the next reader.

Note on verification limits

The worker tests in this worktree would run against a stale compiled dist/.../parse-worker.js (src is newer), and rebuilding dist is out of scope for a read-only review, so I verified the worker path by static lockstep comparison against the sequential path rather than executing it. The PR's claimed 149/149 registry-primary + worker-green is consistent with the code I read.

@magyargergo

Copy link
Copy Markdown
Collaborator Author

Operational note: embedding reindex after this stack lands

CachedEmbedding is keyed by nodeId (embeddings/types.ts), and this lane changes structure-phase node identity — #1991 gives Ruby Trait/module ids a scope qualifier, so same-tail nested mixin modules now carry distinct ids. A default incremental re-analyze regenerates embeddings for the renamed nodes, but a clean re-analyze (or stale-embedding sweep) after the stack merges is the safe call, consistent with the #1978/#1981/#1982 precedent.
For contrast in this stack: #1993 (#2005) is resolution-side only and does not re-key; #1994 (#2007) is a byte-identical refactor.

@magyargergo magyargergo force-pushed the fix/1993-cpp-ns-qualified-inheritance branch from 355f08d to 017cf27 Compare June 4, 2026 09:08
@magyargergo magyargergo changed the base branch from fix/1993-cpp-ns-qualified-inheritance to main June 4, 2026 09:35
magyargergo and others added 2 commits June 4, 2026 09:35
…MPLEMENTS by scope (#1991)

A Ruby `module` maps to the Trait label but is not a typeDeclaration, so the structure phase never qualified its node id: two same-tail nested mixin modules (App::Loggable / Web::Loggable) collapsed onto one Trait:f.rb:Loggable node and the bare-name `include Loggable` cross-wired IMPLEMENTS (first-wins tail).

Structure phase: expose buildQualifiedName as a `qualifyScopeName` ClassExtractor hook and thread it for Trait nodes in parsing-processor + parse-worker (lockstep), so a module node keys by its qualified scope path (App.Loggable). Not Option A — `Trait` is not in CLASS_LIKE_LABELS and the qualified-id selection gates it out; qualifyScopeName bypasses the typeDeclaration gate that makes extractQualifiedName bail on modules. getQualifiedOwnerName also falls back to qualifyScopeName so methods inside a nested module own through the same qualified Trait id (no dangling HAS_METHOD).

Resolution: emitRubyMixinEdges resolves a bare mixin reference lexically by the including class's enclosing scope (`App::S` + `Loggable` -> `App::Loggable`), and the simple-tail fallback is now delete-on-collision (refuse to guess on a same-tail tie) instead of first-wins.

New single-file fixture + tests: two distinct Trait nodes, S IMPLEMENTS App.Loggable only, T IMPLEMENTS Web.Loggable only, no dangling HAS_METHOD; both resolver legs + worker path. Module->Trait preserved; Trait NOT added to CLASS_LIKE_LABELS. ruby-captures-golden regenerated additively.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…te; regen ruby bench baseline (#1991)

F5 follow-up to #1991: replace the four hardcoded `nodeLabel === 'Trait'` checks
(two each in the sequential parsing-processor.ts and worker parse-worker.ts
definition paths) with a single isQualifiableScopeLabel() in ast-helpers.ts so the
lockstep paths can't drift. Value-identical predicate — no behavior change.

Also regenerate the ruby scope-capture bench baseline: #1991 added the
ruby-nested-mixin-tail-collision fixture (and updated the ruby captures-golden),
but the bench baseline was never regenerated, so the order-independent fingerprint
drifts (bf6b13a -> f0d9b4c6, fixture_count 85 -> 86). Pure fixture-corpus drift.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jun 4, 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
10589 10573 0 16 599s

✅ All 10573 tests passed

16 test(s) skipped — expand for details
  • COBOL pipeline benchmark > scales with file count
  • C++ ADL emit benchmark > emit phase scales sub-quadratically with co-scaled files and sites
  • C++ pipeline benchmark > scales with file count
  • C# pipeline benchmark > scales with file count — namespaces spread across the solution
  • C# pipeline benchmark > scales with file count — all types in one (global) namespace bucket
  • C# pipeline benchmark > scales with file count — all types in one (named) namespace bucket
  • Go pipeline benchmark > scales with file count (workers enabled)
  • Go pipeline benchmark — worker pool (issue Worker idle timeout kills long Go scope extraction and surfaces as Napi::Error during analyze #1848) > does not quarantine the large generated Go file on sub-batch idle timeout
  • Go structural interface detection benchmark > scales linearly with interface × struct count
  • Go structural interface detection split-phase benchmark > separates index-build and detection time
  • PHP pipeline benchmark > scales with file count (workers enabled)
  • Ruby pipeline benchmark > scales with file count (workers enabled)
  • Rust pipeline benchmark > scales with file count (workers enabled)
  • Vue pipeline benchmark > scales with component count
  • run.cjs direct-exec entrypoint (fix(cli): steer docs, skills, and hooks through a CLI-neutral project-local runner (#1939) #1945) > resolves a .cmd shim via the Windows shell branch, passing args and exit code
  • 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 80.36% 37343/46464 N/A% 🟢 ████████████████░░░░
Branches 69.03% 23740/34389 N/A% 🟢 █████████████░░░░░░░
Functions 85.59% 3915/4574 N/A% 🟢 █████████████████░░░
Lines 84.03% 33611/39998 N/A% 🟢 ████████████████░░░░

📋 View full run · Generated by CI

… legacy-DAG removal)

Resolved conflicts from #2023 (delete legacy call-resolution DAG + heritage
processor, #942):

- test/integration/resolvers/helpers.ts: took #942's removal of
  LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES + the createResolverParityIt /
  isLegacyResolverParity* helpers (no legacy leg remains; only a kotlin doc
  comment referenced the list).
- test/integration/resolvers/ruby.test.ts: converted the three #1991 `pit(...)`
  (parity-gated `it`) calls to plain `it(...)`, matching #942's single-leg harness.
- bench/scope-capture/baselines.json (ruby): both sides changed it — #942 reworded
  fixture comments (shifting capture byte-positions; capture LOGIC unchanged) and
  #1991 added the ruby-nested-mixin-tail-collision fixture. Recomputed the ruby
  fingerprint on the merged tree: bf6b13a -> b5ea93bb (86 fixtures).

Verified on the merged tree: bench --check PASS (14 languages), tsc 0 errors,
ruby resolver + ruby-captures-golden 159/159, prettier clean. F5 predicate +
swaps intact through the auto-merge.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@magyargergo magyargergo merged commit 560291a into main Jun 4, 2026
29 checks passed
@magyargergo magyargergo deleted the fix/1991-ruby-mixin-module-trait branch June 4, 2026 10:38
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.

fix(ingestion): Ruby same-tail nested mixin-MODULE (Trait) node collision — structure phase never qualifies module ids (#1982 / #1978 follow-up)

1 participant