fix(ruby): scope-resolution namespaced class/module definitions — F62 (#1933)#1972
Conversation
|
@prajapatisparsh is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 10897 tests passed 12 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
magyargergo
left a comment
magyargergo
left a comment
There was a problem hiding this comment.
PR #1972 Tri-Review — fix(ruby): namespaced class/module definitions (F62)
Methods & engines. GitNexus risk / test-CI lanes + Compound-Engineering personas (correctness, adversarial, testing, performance) — all Claude — plus Codex (gpt-5.5, xhigh) as the intended independent engine. Codex's sandbox blocked local execution, so it acted as a second static reader (not an independent executor): by inspection it confirmed the committed golden has 80 entries (not 81), that the golden test auto-globs ruby-* fixtures, and that the new tests are capture-only. The two findings below were reproduced end-to-end by the coordinator (full pipeline run + BASE comparison + a bare-vs-namespaced control) — the strongest evidence here. Two of the three "methods" are the same engine (Claude), so treat cross-persona agreement as consistent, not independently confirmed.
What's solid (validated)
- Not a regression. Correctness lane + the coordinator's BASE comparison confirm the change is additive: graph output for namespaced declarations is byte-identical to BASE.
- Heritage intact.
class Foo::Bar < Basestill emits its inheritance reference (adversarial-verified) — namespacing doesn't break heritage capture. - Bench rebaseline legitimate. The performance lane ran the scope-capture bench; the new ruby fingerprint matches the live measurement and scaling stays ~1.03–1.09 (budget 1.5). The
tests / benchmarksCI job passes. The two new patterns are O(1)-per-node matching — no O(n²) risk. - No collision regression. A suspected
class Bar/class Foo::Bartail-constant collision was probed and found pre-existing & non-regressive.
Findings (2 — see inline)
[P1 · CI blocker · reproduced] Captures golden not regenerated.
The new fixture ruby-namespaced/namespaced.rb is auto-globbed by test/unit/scope-resolution/ruby/ruby-captures-golden.test.ts, but test/fixtures/ruby-captures-golden/expected-captures.json was not regenerated (80 entries, needs 81). Reproduced locally (vitest … ruby-captures-golden → FAIL, expected {…(81)} to deeply equal {…(80)}, exit 1) and confirmed live in CI: tests / ubuntu / coverage is failing, with CI Gate failing downstream. Fix: UPDATE_GOLDEN=1 npx vitest run test/unit/scope-resolution/ruby/ruby-captures-golden.test.ts and commit the regenerated golden.
[P2 · incomplete fix / misleading tests · reproduced] Namespaced declarations still produce no graph node.
With the PR applied, class Foo::Bar / module Baz::Qux still produce no Class/Trait node; their methods are keyed to the full scope (Foo::Bar.bar_method) and the resulting HAS_METHOD edges dangle to a non-existent owner (Class:…:Foo::Bar → MISSING). Reproduced on both resolver legs, with a control where a bare class Plain in the same file does get a node. This is pre-existing (byte-identical at BASE), so it is not a merge regression — but it means the PR's headline F62 goal (modeling namespaced class/module definitions) is reached only at the capture layer, not the graph layer. The 5 new tests assert only emitRubyScopeCaptures output and never run the pipeline, so they pass while this gap stays invisible. Suggested: add a runPipelineFromRepo test asserting the node exists and HAS_METHOD resolves; if true node modeling is intended, reconcile the def name (tail Bar) with the method-owner scope key (full Foo::Bar).
Lower-priority / coverage gaps
- Orphan fixture.
ruby-namespaced.test.tsuses inline source strings and does not loadnamespaced.rb; the fixture is consumed only by the golden test'sruby-*glob (the test it currently breaks). Either wire it into a pipeline test or it adds no coverage. - Untested combo:
class Foo::Bar < Base(namespaced + heritage) has no test. - Worker-mode parity for namespaced declarations couldn't be verified in the worktree (parse-worker not built).
CI / merge state
Blocking: tests / ubuntu / coverage fail (the golden above) → CI Gate fail. Vercel fail is a separate deploy-auth gate (not code). All quality checks (lint / typecheck / format), tests / benchmarks, platform-sensitive, tree-sitter ABI, and packaged-install smoke jobs pass. Verdict: not mergeable until the captures golden is regenerated (P1). P2 is a worthwhile follow-up but not a merge blocker on its own.
Automated multi-tool digest (GitNexus swarm + Compound-Engineering personas + Codex). Both findings reproduced by the coordinator; verify before acting.
| "scaling_budget": 1.5, | ||
| "_rebaselined": "#1956 synth-widening: + ruby-qualified-base fixture; synth now reduces a scope_resolution superclass (class C < Mod::Super) to its trailing constant (matching the #1940 legacy leg), at parity. Linear (~1.03). (Earlier #1956: heritage-bearing scale source.)" | ||
| "_rebaselined": "#1956 synth-widening: + ruby-qualified-base fixture; synth now reduces a scope_resolution superclass (class C < Mod::Super) to its trailing constant (matching the #1940 legacy leg), at parity. Linear (~1.03). (Earlier #1956: heritage-bearing scale source.)", | ||
| "_note": "F62: + scope_resolution class/module declaration captures — fixture count 78→81, fingerprint drift expected." |
There was a problem hiding this comment.
[P1 · CI blocker] The captures golden wasn't regenerated — a separate artifact from the bench baseline. [reproduced]
This _note correctly flags bench-fingerprint drift, but the scope-captures golden — test/fixtures/ruby-captures-golden/expected-captures.json (not in this diff) — is a different file and was not regenerated. ruby-captures-golden.test.ts globs every ruby-* fixture, so it now collects ruby-namespaced/namespaced.rb and fails: expected {…(81)} to deeply equal {…(80)} (exit 1). Confirmed failing in CI (tests / ubuntu / coverage → fail; CI Gate fails downstream).
Fix: UPDATE_GOLDEN=1 npx vitest run test/unit/scope-resolution/ruby/ruby-captures-golden.test.ts and commit the updated expected-captures.json.
| const classDecls = matches.filter((m) => m['@declaration.class']); | ||
| expect(classDecls.length).toBe(1); | ||
| expect(classDecls[0]['@declaration.name'].text).toBe('Bar'); | ||
| }); |
There was a problem hiding this comment.
[P2 · incomplete fix] These assertions verify capture output only — the graph node is never created. [reproduced]
All 5 tests assert emitRubyScopeCaptures output (@declaration.name === 'Bar') but never run the pipeline. Running the full pipeline on this PR's own fixture (runPipelineFromRepo, both resolver legs) shows class Foo::Bar / module Baz::Qux produce no Class/Trait node, and their HAS_METHOD edges dangle to a missing owner (Class:namespaced.rb:Foo::Bar → MISSING); a bare class Plain in the same file does get a node. So the capture lands, but the namespaced declaration still isn't modeled as a graph node.
This is pre-existing (byte-identical at BASE), so not a regression — but the tests pass while the F62 goal isn't met end-to-end. Consider a runPipelineFromRepo assertion that the Class/Trait node exists and HAS_METHOD resolves; the root cause is the def keyed on tail Bar vs the method-owner scope keyed on full Foo::Bar.
✨ PR AutofixFound fixable formatting / unused-import issues across 17 changed lines. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@magyargergo P1 golden regenerated. P2 is a pre-existing downstream gap — extractScope doesn't model scope_resolution name nodes. Our query captures the tail constant correctly (Bar from Foo::Bar), but the graph builder doesn't create a Class node from it. This is byte-identical to BASE, not a regression from this PR. The fix is a scope-extractor change, not a query change. OK to defer to follow-up in another pr or right here ? |
|
Thanks @prajapatisparsh — and your read on both is right. P1: Confirmed fixed. I re-ran the captures golden on your latest head and it's green (10/10), and the regenerated entry matches the digest the test actually computes — so it's a clean regen, not a hand-edit. CI is re-running now and P2: Agreed on all counts. The query is doing the right thing (tail constant I'd lean towards deferring it to a follow-up rather than bolting it onto this PR, for two reasons:
This PR is small and green, and it's graph-neutral (main already carries the same dangling edges for namespaced Ruby), so I'm happy to merge it as-is. Just two small things first, so we don't leave a "looks done" trap for the next person:
Then it's good to go — nice clean change otherwise. 👍 |
|
I'll tackle with the follow-up; :) |
here #1975 |
3 participants
Closes F62 in #1933.
Adds scope_resolution patterns to ruby/query.ts for namespaced class/module definitions (class Foo::Bar, module Baz::Qux).
F63 done via #1940, F64/F65 done via #1937.
Changes:
129 existing Ruby tests pass, 5 new tests pass.