Repository navigation
Skip unsafe Rush subspace locks in the lock inventory - #2003
Open
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
Open
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
Mikola Lysenko (mikolalysenko) wants to merge 3 commits into
Conversation
Lockfile discovery lists a Rush monorepo's subspace pnpm locks with `rush_lock_rels`, which keeps only real directories whose names pass `is_safe_single_segment` (no `\`, `:`, traversal) and are UTF-8, and reads them through the `ProjectView`. The lock inventory listed the same locks separately: on disk and Snapshot views it ran its own `read_dir` with no name check and read each lock by absolute path, so it inventoried packages from subspace locks discovery deliberately skips (e.g. a subspace named `a:b`), and under a Snapshot view it bypassed the snapshot cache, overlay and read tracking, so the two passes could see different bytes in one run. The Memory branch listed children without the name check too. `inventory_rush_pnpm_locks_in` now takes its paths from `rush_lock_rels(view)` and reads each through the view with `inventory_pnpm_lock_rel_in`, for every view kind. The `rush.json` gate is unchanged (disk/Snapshot: `metadata` on disk; Memory: `contains`). The disk-only `inventory_rush_pnpm_locks` and the duplicated Memory listing are gone, and `inventory_pnpm_lock_at`, now used only by the FIFO test, is `#[cfg(test)]`. Tested: new `rush_skips_unsafe_subspace_names`, the inventory twin of discovery's `pnpm_rush_without_subspaces_and_unsafe_subspace_names` (Unix only, as `:` can't be created on Windows); it fails on the old code and passes now. The existing Rush pins (`rush_monorepo_inventories_common_and_subspace_locks`, `rush_json_without_any_lock_yields_none`, `root_pnpm_lock_wins_over_rush_fallback`), discovery's Rush tests and the vex-discover golden corpus (incl. `nested-rush-lock`) still pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Mikola Lysenko (mikolalysenko)
added a commit
that referenced
this pull request
Nov 4, 2026
Two npm-family inventory rules now follow lockfile discovery (and so what hosted mode pins): - Rush's common and subspace pnpm locks are separate install trees, read whenever rush.json is present, beside whatever lock the flavor router chose. Before, the inventory read them only as a fallback when the root had no lock (lock-model divergence N12). The listing itself is unchanged (#2003 aligns it separately). root_pnpm_lock_wins_over_ rush_fallback pinned the old shadowing; it becomes rush_locks_are_read_beside_the_root_lock. - pnpm's `lockfile` setting off (lockfile_disabled, not under Rush, as in discovery): pnpm neither reads nor writes the root or member locks. Discovery keeps a hosted ref from such a lock but withholds its lock pin; the inventory now keeps the entry (its coordinates and hosted url, which ledger liveness reads) and drops its integrity, so vendor's fetch and repair never verify against a pin the install does not use. Registry entries keep their integrity: a registry release's hash verifies its content whether or not pnpm reads the lock (N14). Pinned by pnpm_lockfile_off_withholds_hosted_pins. "Hosted" here is the default Socket host (origin::hosted_patch_uuid with no configured origins): the inventory has no per-run origin list yet; the install-tree PR moves it onto the single classifier. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Mikola Lysenko (mikolalysenko)
marked this pull request as ready for review
November 5, 2026 16:24
Collaborator
Author
|
bugbot run Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
added
the
Ready for review
Collaborator
Author
|
Labeled Ready for review (burn-down agent).
Generated by Claude Code |
Collaborator
Author
Final review briefWhat it does. The Rush fallback in the lock inventory used to build its own list of subspace locks: a raw Risk: low. Two files change (pnpm.rs, about +25/−59, plus one test). The read is no looser: the Disk view uses the same FIFO-safe reader as before, and symlinked subspace directories are still skipped. The only change in behaviour is the narrowing the PR intends: names with Look here
Verified
Nits (non-blocking)
Auto-merge is not armed: ci-ok has to go green first. Once it's green it gets armed, and approving then sends it straight to the merge queue. Generated by Claude Code |
Review follow-ups on the Rush subspace listing fix.
The inventory's rush.json gate went around the view on Disk and Snapshot
views (`disk_root()` + `tokio::fs::metadata`). That marked a tracked
snapshot's recording unusable, kept rush.json out of the ReadSet and
ignored overlays. It also followed links, where discovery uses lstat
(`exists_no_follow`), so a dangling rush.json symlink or an implied
in-memory `rush.json/` directory got different answers from the two.
The member-lock reader used a third check (`is_file`). Now one
`is_rush(view)` (`exists_no_follow("rush.json")`) gates discovery, the
Rush lock reader and the member-lock reader.
`rush_lock_rels` only checked that the subspace leaf was a real
directory. A symlinked `common`, `common/config`, `common/config/rush` or
`common/config/subspaces` was followed, so another repository's locks
could be read. Any symlinked component now leaves its locks out.
The `#[cfg(test)]` `inventory_pnpm_lock_at` helper is gone. The FIFO test
reads `shrinkwrap.yaml` through the production
`inventory_pnpm_lock_rel_in(&ProjectView::Disk(..), ..)`. The Rush
fallback's docs now give the condition it actually runs under: any
non-PnP flavor-probe refusal that no earlier fallback answered.
Tests, each failing before this change:
- rush_gate_sees_the_snapshot_overlay
- rush_gate_keeps_the_recording_reusable
- rush_gate_counts_a_dangling_symlink (Unix)
- rush_gate_counts_a_memory_implied_dir
- rush_member_locks_gate_matches_discovery
- rush_refuses_symlinked_lock_dirs (Unix)
rush_skips_unsafe_subspace_names now also runs through Snapshot and
Memory views. The Memory case runs on every platform and covers both `a:b`
and `a\b`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
bugbot run Generated by Claude Code |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f6f32f2. Configure here.
Collaborator
Author
|
Ready for review re-confirmed at the current head (burn-down agent).
Generated by Claude Code |
Collaborator
Author
Final review brief (updated for f6f32f2)What it does. Lockfile discovery and the lock inventory now read the same Rush pnpm locks: the common lock plus subspace locks whose names are safe single path segments, all read through the project view. The new commit makes one Risk: low. It narrowly tightens a fallback path, which now refuses more than it did before. Its only other effect is that the inventory no longer reads Look here
Verified
Nits (non-blocking)
Auto-merge is not armed yet: ci-ok must go green first, after #2028 lands and this branch picks it up. Generated by Claude Code |
Second review round. Hosted mode had its own Rush gate (`rush.json` is a
regular file) and its own subspace listing (no name check, symlinked
`common/...` dirs followed), so it pinned, and wrote back, locks that
discovery and the inventory refuse, and the readers' gate
(`exists_no_follow`) disagreed with it on a dangling `rush.json` link or
a directory of that name.
Modes are ground truth, so the shared `is_rush` gate is now hosted's:
`view.is_file("rush.json")` (on disk a link to a regular file counts; a
dangling link, a directory, or an implied in-memory directory does not,
since Rush cannot load any of them). Hosted mode, the vendor flavor
probe, discovery and both inventory readers use it, and hosted pins
exactly the locks `rush_lock_rels` lists.
Behavior changes:
- discovery / inventory: a dangling `rush.json` link or a `rush.json`
directory is no longer a Rush monorepo (no Rush lock read; a plain
pnpm workspace's member locks are read again);
- hosted: a subspace whose name is not a safe path segment, and every
lock under a symlinked `common`, `common/config`,
`common/config/rush` or `common/config/subspaces`, is no longer pinned;
- vendor probe: a `rush.json` directory no longer refuses with
`vendor_rush_unsupported`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
What was wrong
Lockfile discovery lists a Rush monorepo's subspace pnpm locks with
rush_lock_rels(vendor/lock_inventory/pnpm.rs). That function keeps only real directories with UTF-8 names that passis_safe_single_segment, and it reads them through theProjectView.The lock inventory (
inventory_rush_pnpm_locks_in) built its own list of the same locks:read_dirwith no name check and read each lock by absolute path. As a result it inventoried packages from subspace locks that discovery skips on purpose, such as a subspace nameda:b. Under a Snapshot view it also bypassed the snapshot cache, overlay and read tracking, so discovery and the inventory could see different bytes in one run.Discovery and the inventory also decided "is this a Rush monorepo?" in three different ways:
exists_no_follow(lstat)tokio::fs::metadata(follows links, bypasses the view) on disk,containsin memoryis_fileFinally,
rush_lock_relsfollowed symlinkedcommon/…directories, so it could read another repository's locks.Fix
inventory_rush_pnpm_locks_ingets its paths fromrush_lock_rels(view)and reads each one through the view, for every view kind. The disk-onlyinventory_rush_pnpm_locksand the duplicated Memory listing are gone.is_rush(view)(view.is_file("rush.json"), hosted mode's gate; round 1 usedexists_no_follow, see round 2) is used by hosted mode, the vendor flavor probe, discovery (extract_pnpm), the inventory's Rush lock reader and its member-lock reader. Hosted also pins exactly therush_lock_relslist.rush_lock_relsdrops the common lock ifcommon,common/configorcommon/config/rushis a symlink. It drops all subspace locks ifcommon,common/configorcommon/config/subspacesis a symlink. The subspace directory itself must still be a real directory with a safe UTF-8 name.#[cfg(test)]inventory_pnpm_lock_athelper and its re-export are deleted. The FIFO test now calls the productioninventory_pnpm_lock_rel_in(&ProjectView::Disk(&root), "shrinkwrap.yaml").Tests
New tests. Each behavior test failed on the code before the review fixes:
rush_skips_unsafe_subspace_names: the inventory twin of discovery'spnpm_rush_without_subspaces_and_unsafe_subspace_names. It checks the Disk, Snapshot and Memory views. Thea:bdirectory is created on disk on Unix only. The Memory case covers botha:banda\band runs on every platform.rush_gate_sees_the_snapshot_overlayrush_gate_keeps_the_recording_reusablerush_gate_counts_a_dangling_symlink(Unix only)rush_gate_counts_a_memory_implied_dirrush_member_locks_gate_matches_discoveryrush_refuses_symlinked_lock_dirs(Unix only): covers a symlinkedsubspaces,rushandcommon, checking bothrush_lock_relsand the inventory.Runs:
cargo test -p socket-patch-core --lib -- vendor::lock_inventory:: vex::discover:: crawlers::npm_crawler::: 712 passed, 0 failed. This includes the Rush inventory pins, discovery's Rush tests, the FIFO test and the vex-discover golden corpus testcommitted_fixture_corpus_matches_golden, which coversnested-rush-lock.cargo clippy --locked --workspace --all-features -- -D warningsandcargo fmt --all -- --checkboth pass.Review findings
All seven
/code-review highfindings were confirmed and fixed. For each behavior finding I first wrote a test and saw it fail.Noneand the overlay was ignored. Fixed byis_rush.rush.jsonand an implied in-memoryrush.json/directory gave different answers. Fixed byis_rush.is_file). Confirmed. All three readers now shareis_rush.common/…directories were followed. Confirmed. Symlinked components are now refused, and the doc comment says so.Not changed, because they are outside this PR's readers:
vendor/npm_flavor.rs) checksrush.jsonwithview.exists, which follows links. That check only chooses the refusal code and does not change which locks the inventory reads.hosted/engine.rs) has its ownrush.jsoncheck. (Fixed in round 2 below.)A lock file that is itself a symlink is still followed, the same way every other lock is read.
Review findings, round 2 (
/code-review highon the review-fix commit, cloud routine)Nine findings. Fixed in
c0fe5b5; every behavior test below failed onf6f32f2.hosted/engine.rshad no name check and followed symlinkedcommon/...dirs, so it pinned and wrote back locks that discovery and the inventory refuse (a subspace nameda:b; every lock under a symlinkedcommon/config/subspaces). Hosted now pins exactly the locksrush_lock_relslists. Test:hosted::engine::tests::rush_locks_are_the_ones_discovery_reads.rush.jsonis a regular file) differed from the readers'exists_no_follow: confirmed and fixed the other way round. Modes are ground truth (owner decision 3), so the sharedis_rushis now hosted's gate,view.is_file("rush.json"), still probed through the view. Hosted, the vendor flavor probe, discovery and both inventory readers use it. This reverses round 1's choice ofexists_no_follow.rush.jsonlink, arush.jsondirectory, or an implied in-memoryrush.json/directory is no longer a Rush monorepo. Rush can't load any of them, and hosted never pinned Rush locks there. Tests:rush_gate_ignores_a_dangling_symlink,rush_gate_ignores_a_memory_implied_dir,rush_gate_counts_a_link_to_a_file,vex::discover::npm::tests::pnpm_rush_gate_ignores_a_dangling_rush_json. The round-1 testsrush_gate_counts_a_dangling_symlinkandrush_gate_counts_a_memory_implied_dirare replaced by these.rush.jsonsuppressed a plain pnpm workspace's member locks: confirmed and fixed by the same gate. Test:rush_member_locks_gate_matches_discovery(extended).view.exists: confirmed and fixed. It usesis_rushnow, so arush.jsondirectory no longer refuses withvendor_rush_unsupported. Test:vendor::npm_flavor::tests::a_rush_json_directory_is_not_rush.rush_lock_rels, which is pinned byrush_refuses_symlinked_lock_dirs.Not changed:
symlinked_reads). The read boundary for every project-file read is Never read or rewrite project files outside the git repository root #2025's job (ReadBoundary, real-path checked), so this PR doesn't add a Rush-only version of it.Tests (cloud run):
cargo test -p socket-patch-core --lib -- vendor::lock_inventory:: vex::discover:: crawlers::npm_crawler:: hosted::engine:: vendor::npm_flavorgives 828 passed, including the golden corpus. CLI--test in_process_redirect|hosted_memory_parity|in_process_alternate_installers rushpass.cargo clippy --locked --workspace --all-features -- -D warningsandcargo fmt --all -- --checkpass.Generated by Claude Code