feat(pipeline): wire Ruby cross-file LSP resolution - #1712

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

Reviewed slice 4. The wiring is right and the Zeitwerk argument is correct. There is one consequence of it I want measured before this lands, and you have already named half of it yourself.

The filter exemption is justified. Ruby constants live in one global namespace and Rails/Zeitwerk autoloads them with no require, so an own+imports filter really would starve cross-file resolution. Putting Ruby beside Rust in that branch, with a comment saying why the mechanics differ, is the right way to record it.

The consequence: Ruby has the Rust exemption without the Rust mitigation

Read the branch you edited alongside its own comment:

Rust therefore always resolves against the FULL def universe: the lazily built shared registry when available, else a full per-file build.

Rust survives the exemption because the registry is its normal path and the full per-file build is its fallback. Your PR description says the quiet part plainly: "there is no prebuilt tier-2 registry yet." So for Ruby the fallback is the only path.

Following it through:

• cbm_pxc_dispatch_file runs per file; with lang != CBM_LANG_RUBY removed from the filter guard, file_defs = all_defs.
• cbm_pxc_run_one hands that whole array to cbm_run_ruby_lsp_cross.
• Which opens with for (int i = 0; i < def_count; i++) to build its class table.

So every Ruby file rebuilds a class table over the entire project's defs. That is F × D, and D grows with F — the corpus-proportional shape, and precisely what "a language without a Tier-2 shared registry is automatically quadratic" describes. A Rails app is exactly the kind of repo where F is large.

I want to be honest about the limits of this: I am reasoning from the call structure, not from a measurement. It may be perfectly acceptable at real Ruby project sizes — the constant factor is small and the class table is cheap per def. But that is a claim to check rather than assume, and right now nothing checks it.

What I would like

Either a measurement, or — better, because it is permanent — a Ruby fixture under tests/fixtures/complexity/ruby/.

The complexity suite auto-discovers fixture directories at that path (tests/test_complexity.c:167-175) and gates on counter ratios at k versus 2k replicated copies, so a files×corpus coupling lands at ~4 and goes red. It needs no test edit — just fixture files. That is the mechanism this repo built for exactly this case, and a new language taking the filter exemption is the strongest reason to use it.

If the ratio comes back at ~2, this merges and the fixture stops the next person reintroducing the coupling. If it comes back at ~4, better to know now than after a Rails user reports a slow index.

Smaller notes

The split is clean and I checked the part that is easy to get wrong — ruby_lsp.{c,h} appearing in both slices is the reviewer-requested move of cbm_run_ruby_lsp_cross out of the resolver slice, and you verified the halves reassemble byte-identically.

Registering methods on both instance and singleton receiver keys because CBMLSPDef carries no def self.m split is a reasonable over-registration, and worth the comment it has.

Clearance: the two flagged files here (Makefile.cbm, scripts/check-lsp-originality.sh) are #1711's commit, already reviewed and cleared on that PR — nothing new in this slice.

Baseline note: the #1181 × #1359 cli failures you documented are fixed on main now; I confirmed both pass in a full-suite run today.

Thanks for the cross-file Ruby work. The later agreement in #1711 records the shared read-only registry and complexity coverage for this slice once the base lands. Please keep it behind #1711 for now. Sorry for the delay on our review of that base—I’ll follow up soon.

Hi @benrfairless, thank you again for #1711, which landed on main on November 2 as fabc2b0. Ruby has a real per-file resolver now, and that is your work. Thank you as well for not waiting on us here: your October 20 commits already give this slice most of what we discussed in #1711's review. There is a project-wide registry built once and sealed, a hashed class table keyed by QN and by constant path, grouped reopenings, a cross-file cost row and a complexity fixture. That made this review much easier.

Here is the whole review in one go, against current main (f031c30). Everything below comes from reading the code; we have not built this head yet.

1. Rebase onto current main (or merge main in, whichever you prefer).
Now that #1711 is merged, the real remaining diff of this PR is 10 files (+829/-32), not the 16 that GitHub still shows. Three files conflict:

• internal/cbm/lsp/ruby_lsp.h: both sides add fields to RubyClassInfo (main adds seen_instance/seen_singleton, you add group_head/shard_next). Please read 2a before keeping both.
• tests/test_ruby_lsp.c and tests/test_lsp_resolution_probe.c: both sides add tests and RUN_TEST lines at the same spot. Keep both. Please rebuild each file as main's version plus your own diff instead of joining the two conflict sides: git moves the lines both insertions share (closing braces) out of the hunk, and the joined result can leave a function unclosed.
• internal/cbm/lsp/ruby_lsp.c merges without a textual conflict, but not semantically (see 2a).

For orientation: the same functions you touch now contain our three small follow-ups from #1711 (0210ea9 single-walk visit stamps, 214a9ac iterative constant path, 435baf6 seed NULL checks). Main also changed the signature of cbm_pxc_run_one_with_php_bindings (a new rel_path parameter, #1910). Your CBM_LANG_RUBY case sits inside that function's body, so it needs no change. Please just check that no new test calls it with the old signature.

2. The shared read-only registry has the right shape. Four points against our checklist (finalize after registration, no lookup in the registration loop, no registry tail scan, shared registries read-only).
The model is the same as before: cbm_go_build_cross_registry (internal/cbm/lsp/go_lsp.c:3547), cbm_py_build_cross_registry (py_lsp.c:6859) and cbm_ts_build_cross_registry (ts_lsp.c:6229). They are used through the prebuilt switch in cbm_pxc_dispatch_file_with_php_bindings (src/pipeline/pass_lsp_cross.c:2257; the switch follows line 2291). They are built in the three places your diff already covers: run_parallel_pipeline (src/pipeline/pipeline.c, the builders block from line 3098), run_extract_resolve (src/pipeline/pipeline_incremental.c:1994) and cbm_pipeline_pass_lsp_cross (pass_lsp_cross.c:2475, the sequential path). Finalize-after-registration holds: cbm_registry_finalize runs after the last add and the registry is then sealed. The dedupe in the registration loop is now a hash probe instead of the old scan. Thank you for both.

a. Keeping the table read-only after the merge. 0210ea9 stores the per-walk visit marks inside RubyClassInfo, and ruby_walk_seen (ruby_lsp.c:560) writes them through whatever ruby_class_by_qn returns. With your change, that can be an entry of the shared table, which every resolve worker reads at the same time. Under -Werror the merged file should stop compiling at that point, because the const is dropped. Please don't fix that by casting the const away or by making the lookup non-const. Each worker counts its own stamps from 1, so a mark one worker leaves on a shared entry can make another worker skip that class, and whether an edge is missed would then depend on thread timing. Please keep the marks for shared entries in per-file storage sized by what the file touches, and leave per-file entries as they are. A small hash keyed by the shared index would work. An array as long as the project's class count, allocated per file, would bring the files x classes term back. It would also help to extend rubylsp_cross_registry_built_once_serves_many_files to compare the shared class array byte for byte before and after the resolves; the count check it has now cannot see a written field.
b. No tail scan. In cbm_ruby_build_cross_registry, every reopening walks to the end of its group to join it (while (sh->classes[t].shard_next)), so a constant reopened R times costs about R^2/2 steps. Namespace modules are exactly this case: module Admin or module Api is reopened in every file under it. Storing the tail index on the group head makes the append O(1).
c. Per-lookup cost must not grow with the number of reopenings. Whenever the head's key misses, ruby_own_method walks the whole group, with a cbm_arena_sprintf per member for the singleton key. A miss is the normal case for an inherited method. So for a class or namespace reopened in R files, every such call site pays R, in every file, which is files x reopenings again. Item 4 fixes this.
d. Small: two comments still say "fallback cbm_pxc_run_one path" (the one on cbm_pxc_has_cross_lsp and the one on the ruby_lsp.h include in pass_lsp_cross.h), but Ruby is on the prebuilt path now. Separately, in the incremental closure route a closure of CLOSURE_BUDGET_FLOOR_FILES (8) files or fewer gets no Tier-2 registries. Because Ruby is exempt from the def filter, each changed Ruby file there rebuilds the full table through cbm_run_ruby_lsp_cross. That is bounded (at most 8 builds), so it is not a blocker. Building the table once, lazily, as Rust does (pp_rust_shared_registry, src/pipeline/pass_parallel.c:4197) would close it, and so would a one-line comment stating the bound.

3. The complexity fixture: thank you, and one change so it can fail.
tests/fixtures/complexity/Ruby/ is the right path. The suite looks the directory up by cbm_language_name (tests/test_complexity.c:185), so the name is Ruby, not the lowercase name we wrote in #1711. Our mistake. Two things stop it from catching 2b and 2c today:

• The flagship gate (complexity_replicated_modules_scale_linearly, test_complexity.c:882) compares node and edge ratios, and the group walks change neither. Please count the work: CBM_LSP_WORK(1) per group member visited, plus a row in the style of main's "17. Bounded resolver work" section in tests/test_ruby_lsp.c (rubylsp_counted_work, line 1331). The row would resolve one fixed file against a shared registry with R and then 2R reopenings of one class, and assert that the ratio stays under the gate of 3. Please check that it fails against the current tail walk and per-lookup group walk.
• The fixture provider copies the directory into k module dirs, and Ruby constants are global, so every class in the fixture is already reopened k times. If you add a call that misses on the head's key, for example CxWidget.new.label (inherited from CxBase), the fixture will exercise the group path end to end.

4. Merge reopened classes at build time. Today a reopening is found by walking its group at lookup time. Please build one merged record per constant path instead, holding its methods (each still pointing at the def in its own file), its superclass and later its mixins, so that a lookup is one hash probe however many files reopen the class. Two details to cover while you are there:

• The group head is the first declaration in def order. A full build orders defs by file index, but the incremental closure route builds all_defs as the persisted base defs followed by the fresh ones (run_extract_resolve, pipeline_incremental.c). So after an edit to the file that holds the head, constructor edges and the inherited superclass can land differently than they would after a full rebuild. Please pick the head by a stable key (the smallest QN, or the file path) so both routes agree.
• As far as we can see, a file that reopens a class without < Base resolves through its own per-file entry, and that entry has no superclass. Inside that file, inherited methods and the ActiveRecord model check are lost. The merged record's superclass should back that entry up.

5. Instance + singleton double registration: please justify it with numbers or drop it. CBMLSPDef does not carry the split today, but the extractor knows it: singleton_method and method are distinct node kinds (internal/cbm/lang_specs.c:467). A flag on the def would let the builder register each method on exactly one key. The cost of over-registering is that an instance call can hit a def self.x on the receiver's class and stop there. It never reaches the inherited instance method of the same name, which is the one Ruby would actually call, and class-side calls have the mirror problem. If you'd rather keep the double registration, a count of how many cross-file edges change on a real app when you register on one key only would settle it either way.

6. One more fail-closed case, across files: mixins of classes declared elsewhere. CBMLSPDef carries the superclass (embedded_types) but no include, prepend or extend. For a class from another file, the shared table therefore knows none of its mixins, ruby_lookup_in_mixins (ruby_lsp.c:587) only sees this file's records, and the walk goes on to the superclass. Ruby dispatches to the included or prepended module first, so a method that a module overrides resolves to the superclass's version instead. This is the case your #1711 item 5 made fail closed within one file. There are two ways to handle it: carry the mixin refs into the shared table and resolve them once at build time, or treat a class whose mixins the table cannot see as blocked below its own methods. A test row for it: a class in file A includes a module that overrides a superclass method, and a call from file B must not resolve to the superclass method.

Smaller: the new cross-file rows match the caller with strstr (out_has). Matching the caller exactly as well as the callee, the way find_resolved_exact (tests/test_ruby_lsp.c:103) does for the single-file rows, would keep #1711 item 7 true here too.

On our side, once the rebased head is up: we will re-run the per-language benchmark from #1711 on rails, redmine, mastodon and loomio: CALLS before and after, a hand-checked precision sample, a determinism check, wall time and memory. We will also re-judge the ActiveRecord-finder stratum. On #1711 it came out 17/17, but only for models declared in the same file. With the shared table, the rule now also fires for models declared in other files, which is the usual Rails layout, so it gets a fresh sample. The bar is at least 95% correct. If it lands below that, we would ask you to keep the finder rule same-file for now and move its cross-file part into a follow-up, so the rest of this slice isn't held up. CI on the October 20 head was red because of the language-count contract on the old base, which main has fixed since; the rebase clears it.

#1713 (the docs slice) is waiting on this one, and we will pick it up as soon as this lands.

Thank you again. This is careful work, and the shared registry is already most of the way there.

Thanks for the thorough review. Everything below is against current main (e0c7a1d at the time of the merge). I merged main in rather than rebasing, because the branch already carried two merge commits and the per-file commits that landed on main under different SHAs. The diff is now 15 files, +1608/-41.

1. Merge. The three conflicts were the three you named. ruby_lsp.h keeps both field sets. The two test files are main's version plus this branch's diff, not the two conflict sides joined. The merged ruby_lsp.c did fail under -Werror at the dropped const, exactly as you predicted (see 2a). The only test that calls cbm_pxc_run_one_with_php_bindings is main's test_parallel.c, which this branch does not touch and which builds against the new signature.

2a. Shared table stays read-only. Visit marks for shared entries now live in a small per-file hash keyed by shared index, sized by what the file touches. Per-file entries keep their inline marks. No cast, no non-const lookup. rubylsp_cross_registry_built_once_serves_many_files now compares the shared class array and both hash indexes byte for byte before and after three files resolve.

2b, 2c and 4. One merged record per constant path. I replaced group_head/shard_next rather than adding a tail index. All reopenings' methods register under the head's QN, so a lookup is one probe however many files reopen the class.

• The head is the smallest QN, so the full build and the incremental closure route agree. The superclass comes from the smallest-QN declaration that has one, and a file that reopens a class without < Base now falls back to the merged record for inherited methods and the ActiveRecord check.
• A method defined by two reopenings resolves to the smaller QN. That is my choice and Ruby has no real answer (the last file loaded wins at runtime), so tell me if you would rather something else.
• rubylsp_cross_registry_is_def_order_independent builds the same defs forwards and reversed and compares the edges.

3. Complexity gate. rubylsp_cross_reopened_class_work_is_bounded counts work with CBM_LSP_WORK. Doubling the reopenings of one class from 64 to 128:

The build gate is 3. I made the resolve gate 1.5, not 3, because a per-lookup group walk only doubles when R does, so a gate of 3 would have let it through. The old code is over both gates (the row stops at the first failed assertion, which was the build one). I also added CxWidget.new.label (inherited from CxBase) to the Ruby fixture, and the cross-file rows now match the caller exactly.

2d. The two stale comments are fixed. I left the lazy build out of this PR and put a comment at the incremental closure site stating the bound (at most CLOSURE_BUDGET_FLOOR_FILES builds).

5. Singleton and instance. I added the def flag. The extractor sets is_singleton on def self.m and carries it through CBMDefinition, CBMLSPDef and the persisted lsp surface (sg). Each method registers on exactly one key. I did not measure edges changed on a real app, because the flag removes the wrong-edge case at its source. One limit: the extractor does not emit a definition for a method inside class << self, so those have no entry in the shared table and give no edge (never a wrong one). I have not widened the extractor for it here.

6. Mixins across files. The extractor now records a class body's include/prepend/extend of constants (extend self is skipped). They go through the same three structures (mx in the lsp surface), are resolved once at build time from the declaring scope into the merged record, and are ordered by declaring QN then source order. A module the table cannot see leaves the lookup blocked, as within one file. Rows: an include in the head's file, an include in a reopening in another file, and an unseen module, each called from a third file, plus one that runs real extraction through the pipeline.

A bug this turned up. The new mixin_refs string was not in result_compact.c's field list, so it pointed into a scratch arena that is freed before the cross-file pass reads it. AddressSanitizer reported a use-after-free in lsp_resolution_probe and edge_types_probe. It is fixed, and the memory census has the field too.

Finder rule. The cross-file half of the ActiveRecord finder rule is behind one constant, ruby_ar_finder_cross_file, with a row for each mode. If your sample comes in under 95%, setting it to false restores same-file models only and nothing else changes.

Test status. The ruby_lsp, lsp_resolution_probe, complexity and edge_types_probe suites pass. The full parallel run is 10304 passed, 31 failed, and all 31 failures are in the cli suite (agent-config install tests). I have not run cli against an unmodified main to confirm they are pre-existing, so please read that as unconfirmed. For lint, lint-format (Apple clang-format 21.0.0), lint-no-suppress and lint-memory-core pass locally. lint-cppcheck reports 11 findings, all in code this branch does not change (for example src/mcp/mcp.c:1017 and pipeline_incremental.c:4064), so lint-ci as a whole does not pass on my machine. My cppcheck may not be the version CI uses.

Assisted-by: Claude Code:claude-sonnet-5-5

Hi @benrfairless, thank you so much for this round. You answered every point of our November 3 review, and in the way we hoped: one merged record per constant path with a smallest-QN head, so a full build and the incremental route agree; visit marks for shared entries kept per file, so the shared table really is read-only; an extractor flag so each method registers on exactly one key; mixins carried across files, with an unseen module failing closed; and a gate that counts work and fails on the old tail walk. The ruby_ar_finder_cross_file switch you added turned out to be exactly the right call, as you will see below.

Here is the benchmark we promised, on the same commits as the #1711 run: rails/rails (3bbf10e6), redmine (c08408c1), mastodon (147bfc1f) and loomio (6a29a527), main vs main with this branch (including our commit below), fresh indexes.

• Precision: a fixed-seed sample of 24 edges this branch adds (6 per repo) was 24/24 correct, for example dm = DeleteManager.new; dm.order now reaching TreeManager::StatementMethods#order, and get in controller tests reaching ActionController::TestCase::Behavior#get instead of a routing helper with the same name.
• rails loses 1,460 TESTS edges, and 98.5% of the ones that go away were backed only by a name guess. Those tests now point at the method they really call.
• Determinism: two runs give byte-identical edge lists on all four repos, and the Ruby resolver's edges are the same with one worker as with 18.
• Cost: the project table builds in 3 to 24 ms, index time is unchanged within run-to-run spread, and peak memory rises about 1% on rails and loomio and about 6% on redmine and mastodon.
• Complexity: the complexity suite passes with your Ruby fixture, and your new gate reports 212 -> 386 for the build with a flat resolve. Through the real pipeline, the fixture copied 64 vs 128 times costs 1,184 -> 2,142 steps to build the table and 3,195 -> 6,267 to resolve, so both grow linearly. A single class with 10,000 vs 20,000 methods stays linear too.

The ActiveRecord re-judge: we drew 48 edges that only the cross-file half of the finder rule produces (fixed seed, 14 per repo and all 6 on mastodon) and checked each against the source. 38/48 were correct (79%), below the 95% bar. Nine of the ten misses are one pattern: a relation chain such as Topic.where(id: x).update(...) or loomio's User.where(...).order(:id).with_write_lock(:id) is typed as a model instance, so the call lands on an instance method. In Ruby a relation answers those itself (Relation#update) or hands them to the model's class methods (def self.with_write_lock), never to an instance method. The tenth is the same-named class issue in item 1 below.

So, as we said on November 3, the finder rule stays same-file for now. Since you allowed maintainer edits, we pushed two commits on top of yours (your own commits are unchanged):

1. 0c25a96 fix(ruby-lsp): keep the ActiveRecord finder rule same-file for now. ruby_ar_finder_cross_file is now false by default, with the reason next to it. The test seam now returns the previous setting, so each test restores what it found. With the fixed reset, the default-mode row could not fail: we flipped only the default back to true and it stayed green. After this change it goes red. The two rows that need the cross-file half switch it on explicitly, and the UsersController row now matches its caller exactly.
2. c1ed41b merges current main. One conflict, in src/pipeline/pipeline.c (log_result_census): main added client_route_path to the same sum your mixin_refs term extends, so it is main's lines plus your term.

One thing we would like fixed before we merge, because it moves edges that are right on main to wrong targets:

1. Same-named classes in independent parts of one repository. rails/rails is several gems, and their test suites each declare their own Topic, Post, Comment or Customer. The merged record pools them under the smallest QN, so Topic.new in an activemodel test now points at actionview/test/fixtures/topic.rb. Of the 7,481 Ruby edges this branch adds on rails, 875 land in another gem's test tree. All 8 we checked are wrong, and at 492 of those call sites main pointed into the caller's own gem. redmine, mastodon and loomio showed none of this. One way to handle it: build the merged record per package root (a directory holding a .gemspec), and leave a constant unresolved when it is declared under several roots and none of them is the caller's. Anything else that keeps those 492 sites right works for us too, and we will rerun the same count on your next head.

Once that is in, we expect to merge this, and #1713 right after it. The cross-file finder can come back in a small follow-up once a relation is typed apart from a record: find, find_by, first, last, take, create and new return the model, while where, order, includes and friends return a relation whose calls go to the relation API or the model's class methods. With your switch, turning it back on is then a one-line change.

Since the branch now has our two commits on top of yours, please pull before pushing again. Thank you again. This is careful, well-measured work.