Repository navigation
Conversation
A Ruby `Const.new` call is rewritten to name the constant rather than the method, so the call links the class the way every other language's `new T()` does — the constructor body lives in `initialize`, so naming `new` links nothing. That rewrite only fired for bare `constant` receivers. A scope-resolved receiver fell through it and kept the whole call expression as its callee: `Admin::User.new`. That resolves to nothing, and it cannot join the Ruby LSP's constructor rows, whose callee is the class QN. Treat `scope_resolution` receivers the same way. The new extraction row pins both spellings and fails without this change, printing the 'Admin::User.new' callee that is the bug's signature. Refs DeusData#1701 Assisted-by: OpenCode:anthropic/claude-opus-5 Signed-off-by: Ben Fairless <ben@oaf.org.au>
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
The previous run hit two transient infrastructure failures: - security/codeql-gate: gate poller timed out by ~4 seconds; CodeQL analysis itself completed successfully - test/test-unix (macos-15-intel): tests passed but the post-run artifact upload hit ENOTFOUND (network unavailability on the runner) No code changes. Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Ben Fairless <ben@oaf.org.au>
db9656a to
944b229
Compare
daemon_runtime_connection_cap_covers_slow_hello_and_stopping_is_terminal failed on macos CI. The test's capacity_rejected condition sampled active_connections immediately after the overflow connect returned. On a loaded runner the server's HELLO timeout (5 s) can fire for the slow_hello raw connection during the overflow client_connect call (up to 2 s), dropping the count from 2 to 1 and falsifying the == 2 assertion. The capacity rejection message already proves the reason. Drop the point-in-time active_connections sample; keep active_clients == 1, which is stable because we still hold the accepted client open. Same class of race fixed in db91b88 for the forged-identity test. Assisted-by: Claude Code:claude-sonnet-4-6 Signed-off-by: Ben Fairless <ben@oaf.org.au>
|
The Ruby fix is right. There is one thing in the diff that does not belong to it. The fix itself: two lines, gated on And the correction deserves saying out loud. You went back and contradicted your own #1706 description — the pre-fix callee is the whole The thing that does not belong
- cbm_daemon_runtime_service_active_connections(fixture.service) == 2 &&That is not mentioned anywhere in the PR description, and it has nothing to do with Ruby constructor callees. I also checked the other four slices — none of them touches that file — so it is specific to this one rather than something shared across the series. I am not disputing the change on its merits. I checked the precedent you cite: But it needs to be its own PR. A change that removes test coverage gets read on its own terms, by someone thinking about that test rather than about Ruby extraction. Bundled into a language fix it is very easy to wave through, which is precisely the risk. Please lift it out — the commit message you would write for it is already most of the way there in that comment. I suspect this is a stray from local iteration rather than deliberate bundling, given how carefully the rest of this series is scoped. Worth saying, though, because your own note when closing #1706 was that bundled changes are "exactly the kind of thing an umbrella PR hides" — and this one hid in a slice. Status
Cleared and approved on the Ruby change. Split the daemon hunk out and this merges. |
Restore tests/test_daemon_runtime.c to the PR's base, as asked in review: the dropped active_connections assertion is unrelated to the Ruby constructor callee and belongs in its own change. main has since reworked this test on its own (flaky-ledger item 12), so the merge of main that follows takes main's version. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Hi @benrfairless, thank you for this fix, and sorry it sat so long after the review. The change itself is exactly right: two lines gated on Ruby, putting Since the branch allows maintainer edits, we finished the last step ourselves instead of asking you for another round:
We checked it end to end: without the fix the new test fails and prints Your commits are untouched. We will take it from here once CI is green. Thank you again! |
|
Thanks @DeusData! It looks like the CI is all green now. Fingers cross we are good to merge soon |
Refresh the contributor branch onto current main while preserving the reviewed constructor extraction change and regression coverage. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for fixing namespaced Ruby constructor extraction! I merged current main into this branch in The integrated diff passed review; fresh required GitHub CI will validate the updated branch before merge. Thanks again for the contribution! |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thanks again, @benrfairless, for the Ruby constructor fix! I merged the latest main into this branch in b40ed9c because another merged change touched the shared extraction tests. The Ruby fix is unchanged (the patch ID matches); this refresh gives it a fresh required CI run on the current base. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you again for the Ruby constructor fix. I merged the shared CI baseline repair into this branch in |
|
Thanks for the Ruby constructor fix and your CI follow-up. The subsequent branch refreshes are on our side; no further contributor changes are requested. Sorry for the delay after those updates—I’ll follow up soon. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you again, @benrfairless, for the Ruby constructor fix, and for your patience. Here is the follow-up we promised: tonight's merges on main touched tests/test_extraction.c, which this pull request also changes, so we merged current main into your branch once more (one merge commit, 73833de; your changes are untouched). Once the required checks are green on this head, we will merge it. Please pull before pushing again. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @benrfairless. The last run had one red check, a subprocess cancellation test on the macOS leg that is unrelated to this change and has failed the same way on loaded runners before. Main also changed tests/test_extraction.c in the meantime, so we merged current main into the branch once more (one merge commit, 5b3fe06; your changes are untouched) to get a single fresh run on today's tree. We will merge once it is green. Please pull before pushing again. |
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you for your patience, @benrfairless. CI went green on this branch, but this evening's merges on main touched files this pull request also changes, so that green no longer covered today's main. We merged current main into the branch once more (one merge commit, 849e74f; your changes are untouched) and will merge once the fresh checks are green. Please pull before pushing again. |
|
Merged as 006101a - thank you, @benrfairless! A call like A::B.new now reduces its callee to the constant A::B, so the constructor call reaches the right class instead of a guessed .new. Thank you for the small, precise fix and for your patience through the branch refreshes. Your Ruby resolver in #1711 is waiting on one decision on our side, and we will be back on it soon. |
What does this PR do?
Slice 2 of 5 splitting #1706. Stands alone; depends on nothing else in the series.
A Ruby
Const.newcall is rewritten to name the constant rather than the method, so the call links the class the way every other language'snew T()does — the constructor body lives ininitialize, so namingnewlinks nothing.That rewrite only fired for bare
constantreceivers. A scope-resolved receiver fell through it and kept the whole call expression as its callee:Admin::User.new. That resolves to nothing, and it cannot join the Ruby LSP's constructor rows, whose callee is the class QN.Correction to the original description. While writing the reproduce-first test I found the claim I made in #1706 — that the callee stayed
"new"— is wrong. The actual pre-fix callee is the fullAdmin::User.newexpression. The commit message and test here state the real behaviour; the fix itself is unchanged and still needed.Reproduce-first. Without the change, the new row prints exactly that:
The test pins both spellings — scope-resolved reduces to
Admin::User, bare staysWidgetas a regression guard — and rejects any callee that isnewor retains a.newsuffix. There was previously no test anywhere for this rewrite in either form.Where this sits in the series
feat/ruby-lsp-per-filefeat/ruby-lsp-cross-filedocs/ruby-hybrid-lspCI baseline
mainat 4d7d9f1 is red for reasons unrelated to this change: #1181 gavelist_projectspagination parameters, retiring the last empty-properties schema that #1359's guards used as their zero-argument example. #1704 already fixes it. Until that merges,ci-okwill be red here for that reason.Refs #1701
Checklist
git commit -s)scripts/test.sh) — see fix(extraction): keep Ruby superclass names bare #1709 for the full-leg numbers; all failures are pre-existing on unmodifiedorigin/main.make -f Makefile.cbm lint-ci)Assisted by OpenCode using anthropic/claude-opus-5.