Repository navigation
Fix pnpm brace and class workspace globs (#1846) - #1863
Mikola Lysenko (mikolalysenko) wants to merge 11 commits into
Conversation
Assisted-by: Claude Code:claude-opus-5-5
The check for "does this pnpm lock list workspace members" counted a `#`-commented importer line as a member, and a column-0 comment ended the importers section early. It now skips comments, accepts tab-led lines, and gains a whole-text entry point so the second, hand-split copy in pnpm_workspace can go (#1846). Assisted-by: Claude Code:claude-opus-5-5
A pnpm-workspace.yaml listing members as `packages/{a,b}` or
`packages/[ab]*` was refused by every non-hosted membership check,
or (when deciding which workspace file governs a project) treated as
listing every directory, even though pnpm installs exactly those
members and the shared glob matcher already reads that syntax. Hosted
mode kept its own second reader to work around the refusal.
Now one reader answers for all callers: brace sets and plain classes
of letters, digits, `_` and `-` match as pnpm 10.28 reads them, and
the member walk expands braces and widens classes before narrowing.
Syntax pnpm reads its own way stays refused: `[!a]` (pnpm reads it
as `[a]`), `[^a]` and `[.]` (they match a leading dot), POSIX classes
and extglob groups. The hosted fallback reader and the duplicate
lock-importer detector are gone (#1846).
Assisted-by: Claude Code:claude-opus-5-5
The hosted/in-memory parity test used `packages/{a,b}` as its example
of a pnpm glob socket-patch cannot read. Brace sets are now read
(#1846), so the case uses an extglob group, which is still refused.
Assisted-by: Claude Code:claude-opus-5-5
Review on #1846: beside a class or brace set, pnpm reads `+` as a quantifier (`packages/[ab]+` lists `packages/ab`) and `\` as an escape (`packages/\[a]` lists only a directory named `[a]`), while the shared matcher reads `+` literally and `\` as a separator. Those globs are refused again, in each brace alternative too, instead of answering "not a member". Globs without classes or braces read as before. The member walk now applies one directory bound across every brace alternative, so a brace glob can no longer collect many times more candidates than the limit. Each candidate also skips the repeated syntax check. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
1 similar comment
|
BugBot review Generated by Claude Code |
A class range from `-` (as in `packages/[--9]h`) spans `.` and `/`, so pnpm can match a hidden directory that the shared matcher never does. Such classes are refused now. A literal `-` (`[-a]`) and ranges between letters, digits and `_` stay readable. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
|
Burn-down agent: labeled Generated by Claude Code |
|
Burn-down agent: labeled Ready for review at
Generated by Claude Code |
pnpm 10.28.0 reads braces as picomatch does, not as brace-expansion
does. So modeled() accepted three shapes whose members differ from
what pnpm lists:
- a stepped or multi-character sequence: `p{1..10}` lists p0 and p1
under pnpm, not p10;
- a `*` or `?` inside braces: `{a,*}` lists dot directories under pnpm;
- an empty alternative: `packages/{a,}` listed `packages` itself.
Refuse these shapes, as the rest of modeled() already does for syntax
pnpm reads its own way, instead of guessing a member set that would
reach member_dirs and the hosted governing root.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Final review of I compared against a real pnpm 10.28.0 (
The fix adds Ran: Non-blocking, for the PR description:
I removed bugbot run Generated by Claude Code |
|
BugBot review Generated by Claude Code |
|
Burn-down agent: labeled Ready for review at
Generated by Claude Code |
member_dirs walks the tree once for each brace alternative, and the
directory bound only counts matches, so an alternative that matches
nothing walks the whole tree without ever reaching it. A scanned repo's
pnpm-workspace.yaml could list up to 4095 alternatives (for example
**/{a..z}{a..z}{a..e}) and stall the scan for minutes on a modest tree.
Allow at most 64 alternatives over all globs and answer Err beyond
that, which callers already treat as no answer.
Refs #1846
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011f9aeRR24V93EPLyuXR8tp
|
Final review of The problem: an untrusted The fix: Verified:
Non-blocking, for the description: two changes go beyond the brace/class fix, and the description doesn't mention them. Both are tested and look right.
I removed bugbot run Generated by Claude Code |
|
BugBot review Generated by Claude Code |
The cap on how many tree walks a pnpm-workspace.yaml can ask for also counted globs without braces. A workspace listing more than 64 plain `packages:` entries, which large monorepos do, failed member-lock discovery, though main walks those without a cap. Only the extra walks brace sets add now count; a plain glob is one walk, as before. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
|
Burn-down agent: labeled Ready for review at
Generated by Claude Code |
|
Final review brief (head What it does. pnpm Risk: low–medium. The refusals keep it conservative and the walk is bounded. But it changes which directories count as workspace members, in both the hosted and the non-hosted paths. Look here
Verified
Changes I made: none. Open questions (non-blocking, contrived inputs)
Auto-merge (squash) is armed, so approving sends this straight to the merge queue. Generated by Claude Code |
A brace sequence with single-character ends that are not the same
kind stepped through punctuation: `packages/{-..0}` expanded through
`.` and `/`, listing `packages` itself as a member, and `{0..Z}`
through `?`, which the walk then read as a wildcard. A sequence now
needs both ends to be digits, both lowercase or both uppercase.
Found in the final review of #1863.
Assisted-by: Claude Code:claude-opus-5-5
|
[agent] Fixed the final review's open question in 5ca5e0e instead of leaving it as a follow-up. This PR introduced it, and Generated by Claude Code |
|
BugBot review Generated by Claude Code |
pnpm 10.28 expands a `..` inside a comma brace set as a sequence
(`packages/{a,b..c}` lists `,` `0` `A` `[` ...), and reads braces inside a
class as class members (`packages/[a{b,c}]` lists `,` `{` `}`). modeled()
accepted both and listed a different member set, where the rule is to
refuse syntax we don't read the way pnpm does. Refuse any alternative
holding `..` beside a comma, and check classes on the raw glob too.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011f9aeRocMg7S8GHB43bJsX
|
Final review found two globs where
All four globs are added to the refusal table. Local results: I removed bugbot run Generated by Claude Code |
|
BugBot review Generated by Claude Code |
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 032b960. Configure here.
LLM Description written by Claude Code:claude-opus-5-5
Fixes #1846
Summary
A
pnpm-workspace.yamllisting members aspackages/{a,b}orpackages/[ab]*was refused by every non-hosted membership check (member_dirs,root_accounts_for_member_lock). When working out which workspace file governs a project, it was instead treated as listing every directory. pnpm installs exactly those members, and the shared glob matcher already reads that syntax. Hosted mode had its own second reader to work around the refusal, so hosted and non-hosted answered differently.User-visible: brace sets and plain character classes in
packages:are no longer refused outside hosted mode. They now resolve to the members pnpm itself lists.Root cause
utils::pnpm_workspace::modeledrefused any glob containing{ } [ ] ( )as "syntax the shared matcher does not model". Butworkspace_globs::pattern_matchesalready expands braces and matches classes. In addition,member_dirswalked candidates with cargo'sexpand_glob_bounded, which reads neither braces nor classes.Changes
modelednow refuses only syntax that pnpm reads differently from the shared matcher. I probed this on pnpm 10.28.0 withpnpm list -r, overpackages/{a,b,c,ab,x,.h}:packages/{a,b}packages/[ab]*packages/{a..c}packages/[a-b]packages/*,!packages/[ab]packages/[!a]*[!a]as[a])packages/[^a]*packages/[.]hpackages/[[:alpha:]],packages/(aA class is read only when it is closed, not negated, and holds just letters, digits,
_and-ranges. Such a class can never match.or/. A glob with more brace alternatives than the matcher expands is also refused, so the answer is never partial.member_dirswalks each brace alternative, with its classes widened to?(a superset), and thenlists_membernarrows the result exactly.hosted::governing_root::pnpm_lists_unmodeledand itsunwrap_or_elsefallback are deleted. Hosted now reads throughlists_member. Syntax that is still unmodeled lists nothing there, which matches that function's documented "errs toward not a member" rule.There is now one detector for "the lock lists member importers":
formats::pnpm::workspace, with a newlock_text_has_member_importers(&str)entry point. It replacespnpm_workspace::root_lock_lists_members. It now skips#comment lines: an indented commented importer was counted as a member, and a column-0 comment ended the section early. It also treats tab-led lines as continuation, as the deleted copy did.Brace shapes that pnpm reads as picomatch does, not as brace expansion, are refused (a7ee393, from the final review). These are stepped or multi-character sequences (
p{1..10}listsp0 p1under pnpm),*or?inside braces (these match a leading.), and an empty alternative (packages/{a,}must not listpackages).member_dirswalks at most 64 brace alternatives across all globs and returnsErrbeyond that (2161606, from the final review). Globs without braces are not counted against that cap (d52f3b6, from Bugbot). Each alternative is one walk of the tree, and an alternative that matches nothing never reachesMAX_MANIFESTS. In the reviewer's measurement,**/{a..z}{a..z}{a..e}took 519 s on about 4,200 directories.A
{x..y}sequence must have both ends digits, both lowercase or both uppercase (5ca5e0e, from the final review's open question):{-..0}stepped through.and/, and{0..Z}through?.A
..beside a comma in a brace set ({a,b..c}) and braces inside a class ([a{b,c}]) are refused (032b960, from the final review): pnpm reads the first as a sequence and the second as class members.Behaviour notes: the merged lock-importer reader skips
#comments, no longer endsimporters:at a column-0 comment, and treats tab-indented lines as indented. The member walk'sMAX_MANIFESTSbound now counts the union of every glob's candidates.I did not add
WorkspacesDialect::PNPM, which the issue suggested. With the second reader gone,lists_member(viaglob_matches_no_dot) is the only pnpm reader, so the constant would have no caller.Test evidence
Red before the fix (
cargo test -p socket-patch-core --lib, new tests on 78ccde1 plus the test commit): 4 failed:member_dirs_read_braces_and_classes_as_pnpm_does,a_project_outside_the_packages_globs_is_not_governed,member_importers_mark_a_pre_10_5_workspace(commented importer),lock_text_member_importers_read_every_lock_shape.Green after:
cargo fmt --all -- --check: okcargo clippy --workspace --all-features -- -D warnings: okcargo test -p socket-patch-core --all-features: 6992 passed. 4 failed only because this container runs as root, so chmod-based "write fails" tests cannot fail:copy_tree::relax_loop_must_not_traverse_symlinked_root,vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry,pypi_poetry::wire_write_failure_…,pypi_requirements::wire_failure_rolls_back_…. This PR doesn't touch those files.hosted_memory_parity(42),in_process_redirect_pnpm(41),in_process_vendor_pnpm_parent_child(8),in_process_vendor_pnpm_takeover(15),scan_pnpm_relocated_store_cwd_e2e(23): all pass.e2e_redirect_pnpm_build -- --ignored --skip pnpm_pinned_matrix(CI's leg, local pnpm 10.28): 11 passed.cargo test --workspacerun ran out of container disk while linking CLI test binaries, so CI is the full run.Per-issue checklist (#1846 acceptance criteria):
member_dirs:governing_root::tests::pnpm_brace_and_class_globs_list_what_member_dirs_lists,pnpm_workspace::tests::member_dirs_read_braces_and_classes_as_pnpm_does(disk and in-memory trees).#-commented importer line is not a member:member_importers_mark_a_pre_10_5_workspace,lock_text_member_importers_read_every_lock_shape.governing_roottests pass.hosted_memory_parity::an_unconfirmed_pnpm_member_keeps_its_locknow uses an extglob as its unmodeled glob, because braces are read now./code-review high
Fixed in acf9d23:
+and\beside a class or brace set: pnpm reads them as a quantifier and an escape. I confirmed this on pnpm 10.28:packages/[ab]+lists a, ab, b;packages/\[a]lists only[a]. These globs are refused again, checked on the raw glob and on each brace alternative (so{Z..a}is refused too).member_dirs_refuse_a_glob_the_bounded_walk_cannot_finish).member_dirsno longer re-runsmodeledfor each candidate (lists_modeled_member).Errfrommember_dirsinstead of relying onunwrap_or_default.scan --mode vendoredstill exits 1 forever and drops its in-run VEX when a nested npm project has no lockfile (package-lock=false); the #1534 fix only covers nested projects with their own lock #1554 doc comment is back on its own test.Bugbot (acf9d23): a class range starting at
-([--9]) spans.and/. Fixed in f9b15ba: a class body containing--is refused.Not fixed, by choice:
cargo_workspace::expand_glob_boundedbraces and classes for every caller. That is Cargo workspace member globs ignore [...] character classes, so members = ["crates/[ab]*"] expands to no members #1833 (Cargo classes, p2), a separate root cause. Here the widen-then-narrow walk stays local to pnpm and stays exact, becauselists_memberdecides every candidate.modeled's class check onworkspace_globs::char_class. The check here deliberately accepts only a strict subset: closed, non-negated classes of[A-Za-z0-9_-]with no range starting at-. Anything else,[]a]included, is refused. So when it disagrees with the matcher's grammar, the result is a refusal, never a wrong answer.https://claude.ai/code/session_011f9aeQPdgsuof1CE3TcTC5
Generated by Claude Code
Note
Medium Risk
Changes core pnpm workspace membership, lock shared/per-member classification, and hosted governing-root behavior; wrong glob modeling could mis-pin or drop member locks, though unmodeled syntax still fails closed with errors or conservative defaults.
Overview
pnpm
packages:globs with brace sets and plain character classes (e.g.packages/{a,b},packages/[ab]*) are now resolved the same way in hosted and non-hosted paths, matching what pnpm lists instead of refusing them or using a separate hosted-only reader.Membership logic narrows
modeled()so only patterns pnpm interprets differently from the shared matcher stay refused (extglobs, negated/POSIX classes, quantifiers/escapes beside braces, odd brace sequences). Supported patterns are discovered by walking each brace alternative with classes widened to?, then filtering withlists_member; brace-driven walks are capped at 64 alternatives across all globs. Hostedlisting_workspacedrops the oldpnpm_lists_unmodeledfallback and useslists_memberonly (unmodeled syntax lists nothing).Shared lock detection consolidates on
lock_text_has_member_importers/member_importers:#comments no longer count as importers or end theimporters:section early, and tab-indented lines are handled like the previous ad-hoc parser.Reviewed by Cursor Bugbot for commit 032b960. Configure here.