Repository navigation
Conversation
Reads `initialize`'s workspaceFolders (and the rootUri/rootPath fallback an older client sends instead) plus workspace/didChangeWorkspaceFolders, and diagnoses each document against the nearest rector.php/composer.json ancestor of its own folder instead of the server's single boot-time working directory -- the #94 bug (Sublime Text's ${folder} resolving only the first multi-root folder, every other folder refused as out-of-root). WorkspaceRootResolver finds that nearest ancestor; RootedDiagnosticsSourcePool lazily boots one warm RectorDiagnosticsSource per resolved root, capped with least-recently-used eviction (eviction needs no explicit shutdown: dropping the pool's last reference lets RectorRunner::__destruct() kill that root's forked worker); MultiRootDiagnosticsSource is the drop-in LspServer talks to, so every existing call site is unchanged. --working-dir stays an unconditional override, exactly the single-root behaviour from before this. workspace/executeCommand's rector-warm.fixWorkspace is not yet root-aware -- still scoped to the server's single boot-time working directory, called out in both docs pages as a known follow-up rather than silently left broken. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Explore review finding: executeCommand()'s rector-warm.fixWorkspace read getcwd() directly, which RootedDiagnosticsSourcePool now chdir()s on EVERY diagnose call -- so in multi-root mode fixWorkspace silently fixed whichever folder was last diagnosed, not any stable root. Capture the server's own boot-time cwd once, in LspServer's constructor, before any diagnose call can move it, and use that for fixWorkspace -- exactly its pre-#107 meaning. Red/green pinned in a new LspServerTest case that simulates the chdir side effect directly on a fake source. oss:auditor finding: WorkspaceRootResolver's walk-up relied on dirname() treating a bare Windows drive letter ('C:') as its own fixed point, which it is not (dirname('C:') is '.', the relative cwd) -- reasoned from POSIX dirname() semantics, not observed on a real Windows runner, and every reachable trace through the loop's own owner-equality check already intercepts a drive root first. Checked explicitly before dirname() is ever called on it anyway, so termination no longer depends on that behaviour at all, and pinned with a dedicated test. Also added the fixWorkspace root-awareness caveat to docs/lsp.md's multi-root section (Explore review finding #2): it previously only appeared in docs/lsp-for-rector-maintainers.md, and that sentence is also now accurate rather than aspirational, since fixWorkspace's root really is stable again. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…acuous test Both the second-pass Explore review and the second-pass oss:auditor pass, spawned over the prior self-review fix commit (needs-second-pass per fix_commit_scope.py, 5 files touched), independently reached the same conclusion by hand-tracing the loop's own invariants and (Explore) by empirically reverting the production code and re-running the test: the isFilesystemRoot() guard added in the previous commit can never execute, because the loop's pre-existing $dir === $owner check always intercepts a bare filesystem-root value first -- $dir can only equal a root-length string while still being isDescendant($dir, $owner) if $owner is that same value, which is exactly what that earlier check already catches. The pinning test for it therefore passed identically whether the guard was present or not: a vacuous red/green pin, false coverage rather than real coverage. Removed the guard and the method; left a comment on the real $dir === $owner check documenting the construction argument for why no extra guard is needed, and rewrote the test to assert what the code actually does (resolve cleanly at a drive root) without claiming to pin a branch it never reaches. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…ding Non-blocking finding from #107 self-review, routed per findings.md. Co-Authored-By: Claude Sonnet 5 <[email protected]>
cs leg: bin/rector-warm-lsp had a lone `fn (string $root)` (space after fn) left over from an earlier edit round -- php-cs-fixer's own style here is `fn(string $root)`, no space. Confirmed the only instance in the diff. Windows legs (tests windows-latest/8.3, coverage-windows): both failing tests' setUp() built $this->base from sys_get_temp_dir(), which is backslash-separated on Windows, then built every fixture path from it with '/' concatenation -- a mixed-separator string WorkspaceRootResolver::resolve() never produces, since its own normalize() always returns pure forward slashes. The code was right; the tests' expected values were not normalized the same way. Fixed by normalizing $this->base to forward slashes once, in setUp(), in both WorkspaceRootResolverTest and MultiRootDiagnosticsSourceTest -- every value built from it downstream (including the testWorkingDirOverride test, which returns the override path unchanged rather than through resolve()) stays consistent either way. RootedDiagnosticsSourcePoolTest was not touched: it never routes through WorkspaceRootResolver, and its own chdir assertions already go through realpath() on both sides. Confirmed via git stash + a plain rector dry-run against ce26891 that the one remaining rector finding in bin/rector-warm-lsp (an unrelated closure-style suggestion at a shifted line number) predates this branch. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Contributor
Author
|
Independent warm==cold E2E verification (CLAUDE.md gated-file requirement for code-action/fixWorkspace routing): PASS, 4/4 byte-identical checks + the bootRoot regression confirmed directly. Run by a separate agent against head 4167fcd (one commit behind current head 99861c5):
Not exercised: The two commits since 4167fcd (cs whitespace fix in Gate status for this PR: CI green, review clean, E2E check satisfied. Still blocked on the maintainer's explicit OK -- this is a new feature (LSP multi-root workspace support) under CLAUDE.md's feature gate and must not merge on green alone. 🤖 Generated with Claude Code |
This was referenced Oct 5, 2026
Closed
fdaviddpt
added a commit
that referenced
this pull request
Oct 5, 2026
* Fix: workspace root marker must respect containment (#296) Since #293, WorkspaceRootResolver::resolve() checked hasRootMarker($dir) before isDescendant($dir, $owner), so a document opened outside every declared workspaceFolders resolved to its own containing directory whenever that directory held a rector.php or composer.json -- RootedDiagnosticsSourcePool then chdir()s there, and the pre-existing containment guards in RectorTool::processWith() and RectorDiagnosticsSource::diagnoseBufferImpl() both compare against getcwd(), which by then is already the out-of-workspace directory, so both pass by construction. resolve() now only accepts a root marker when the directory is inside the owning workspace folder (or the fallback), restoring the pre-#293 refusal for a document outside the workspace entirely. Adds a regression test pinning the bypass case (document outside every folder, own directory carries a marker) alongside the existing sibling test for the no-marker case. Co-Authored-By: Claude Sonnet 5 <[email protected]> * Fix: update stale containment comment flagged in self-review (#296) The comment on the isDescendant() early-return branch claimed "RectorTool's own containment check is what actually refuses the call" -- the exact premise this issue's fix just disproved for the sibling hasRootMarker() branch. Reword it to say what is actually true: this check (and its twin now guarding hasRootMarker()) is the one that refuses the call, because RectorTool's cwd-based guard compares against a cwd that RootedDiagnosticsSourcePool has already chdir()'d into by the time it runs. Found by the Explore self-review spawn. Co-Authored-By: Claude Sonnet 5 <[email protected]> --------- Co-authored-by: Florian DAVID <[email protected]> Co-authored-by: Claude Sonnet 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #51.
Reads
workspaceFoldersfrominitialize(withrootUri/rootPathfallback for older clients), handlesworkspace/didChangeWorkspaceFolders, and resolves each document to its nearestrector.php/composer.jsonroot. Each resolved root gets its own warmRectorDiagnosticsSource, started lazily and held in a capped (default 4) LRU pool --RootedDiagnosticsSourcePool-- so two folders with differentrector.phpconfigs each get diagnostics from their own worker.--working-dirstays an unconditional override, checked first.Scope: real-time diagnostics (
textDocument/didOpen,didSave, buffer diagnostics, code-action edits) get full multi-root support.workspace/executeCommand'srector-warm.fixWorkspacedeliberately stays scoped to the server's boot-time working directory -- widening it safely needs the command's own plumbing to carry a target root, which it does not today, so it is documented as a known follow-up in bothdocs/lsp.mdanddocs/lsp-for-rector-maintainers.mdrather than silently left broken.Review
Two rounds (Explore + oss:auditor spawned independently each round). Round 1 found and fixed a real regression this diff introduced:
fixWorkspaceread ambientgetcwd(), which used to be stable (chdir only happened once, at boot) but the new pool chdir()s on every diagnose call, sofixWorkspacewould have silently targeted whichever root was most recently diagnosed. Fixed by capturinggetcwd()once in the constructor ($bootRoot), with a dedicated regression test confirmed red before the fix and green after. Round 1 also added the fixWorkspace caveat todocs/lsp.md, and flagged (not fixed, logged totrap.d/107.workspacefolders-malformed-vs-absent.md) thatworkspaceFolderListPaths()can't distinguish malformedworkspaceFoldersentries from none at all -- ranked non-blocking.Round 1 also raised a reasoned-not-observed Windows drive-root concern in the root-walking resolver; the first fix for it (
isFilesystemRoot()guard) turned out to be dead code once both round-2 reviewers independently traced the loop's pre-existing$dir === $ownercheck, which already intercepts a bare drive-root value by construction. Removed the dead guard and its vacuous test; replaced with a comment documenting the construction argument and a test that asserts what the code actually does.Tests
tests/Integration/MultiRootDiagnosticsSourceIntegrationTest.phpis a real integration test (no fakes) against two new fixture projects with differentrector.phpconfigs, through the realRectorTool/RectorRunner, proving root isolation; deliberately broken (hardcodedresolveRoot()) and confirmed it fails as expected, then reverted.tests/Unit/LspServerTest.php+tests/Unit/Lsp/+ the integration test): 221 tests, 1317 assertions, 4 pre-existing skips, green at every one of the three commits. Did not run the full./vendor/bin/phpunitsuite (narrowed-run convention).vendor/bin/phpstan analyse(level max, 89 files): clean at every commit.vendor/bin/rector process --dry-runon changed files: one pre-existing finding (bin/rector-warm-lsp:87, confirmed viagit stashagainst the base commit), not introduced by this diff.Platform claims
The Windows drive-root discussion is reasoned (from
dirname()semantics on POSIX used as a proxy), not observed on a real Windows runner.Closes #107
🤖 Generated with Claude Code