[Mikola Lysenko (mikolalysenko)]

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1900

Summary

A package.json workspaces field is now read by one helper, formats::package_json::Workspaces. It classifies the field as absent, a pattern list, or malformed, and every caller that parsed the field by hand now reads it:

Call site	Before	After
hosted::governing_root (member refusals; also the npm crawler's extra roots and the yarnrc redirect)	own parser, skipped non-string items	Workspaces::listed_patterns: same widening behaviour, wrapper removed
npm_crawler::bun_workspace_pattern_members_sync	null read as "no members"	malformed (including null) is unreadable, so it falls back to bun.lock or keeps every entry
update::channel::declares_workspaces	any non-empty array, [1] included	at least one pattern string
vendor::yarn_classic_workspace_member_risk	any non-empty array	at least one pattern string; ["a",1] still warns
patch::user_patches	own parser	shared reader; still walks a malformed field's string patterns

Root cause

Five readers parsed the same field by hand, and each decided on its own what a malformed field (a non-string item, null, the wrong type) means.

The issue's open question: fail closed or skip?

I probed it locally:

workspaces	npm 10.9	yarn 1.22.22	Bun 1.4.4
["a", 1]	fails	fails	fails
null	fails	no workspaces	fails
"a"	fails	no workspaces	fails
{"packages":"a"}	fails	no workspaces	fails
{"nohoist":[]}	fails	no workspaces	no workspaces

A non-string item breaks every install, so the shared reader classifies it as Malformed. What a caller does with that depends on which way is safe for it:

• Readers that can only widen (refusals, extra crawl roots, warnings, the patch search) keep the field's string items.
• The Bun member walk has to name an exact member set, so it treats the field as unreadable.

Review notes (/code-review high)

Fixed:

• An earlier revision flagged every npm package as possibly user-patched when the field was malformed. That flag is gone: pnpm ignores the field, and the string patterns are still walked.
• The hosted::governing_root::workspace_patterns wrapper is removed, and import grouping is fixed.

Not changed, with reasons:

• [1] (no string item) no longer counts as declaring workspaces for the yarn upgrade hint or the yarn classic warning. Every package manager refuses to install that manifest, so there is no member install for the warning to protect, and no working yarn command for the hint to suggest.
• The Bun walk does not union a malformed field's strings with bun.lock. Bun cannot install from that manifest, so the lock it last wrote is the member set on disk. With no lock, None keeps every entry, which is the safe side.
• governing_root::package_json_declares_workspaces stays as it is. It is a presence check for vlt's precedence ("a workspaces key with any value"), not a parse of the field's shape.
• Returning borrowed &str items instead of owned strings would save allocations, but the gain is negligible on these one-shot probes.

Test evidence

New tests, red on main's call-site code and green after the switch (except the guard test noted below):

• formats::package_json::tests::{classifies_every_shape, parse_ignores_a_bom_and_rejects_non_json, widening_readers_keep_a_malformed_fields_strings}: the table test the issue asks for (absent, null, empty, object form, non-string item, ! pattern).
• crawlers::npm_crawler::tests::yarnrc_and_bun_workspaces_read_past_one_bom_only: null → None (was Some([])).
• update::channel::tests::declares_workspaces_reads_the_shared_field_rule: [1] → false (was true).
• vendor::berry_migration_risk_tests::issue_691_…: [1] → no warning (was a warning); ["a",1] still warns.
• patch::user_patches::tests::malformed_workspaces_still_walk_their_string_patterns: member patches are found under ["packages/*", 1], and unrelated packages are not flagged.

Guard test (passes on main too; it pins that the shared reader keeps user_patches' existing behaviour):

• patch::user_patches::tests::malformed_workspaces_still_walk_their_string_patterns (moved out of the red-on-main list above after the final review).

Commands run locally:

• cargo fmt --all -- --check: passes.
• cargo clippy --workspace --all-features -- -D warnings: passes.
• cargo test --workspace --all-features --no-fail-fast: 15,338 passed, 11 failed. None of the failures are caused by this PR:
  • 9 are permission-denial tests that cannot fail when run as root (the container runs as uid 0).
  • 2 are in e2e_bun_lockb. One rollback reaches the live blob API (fixed in Fix bun.lockb nightly e2e reaching the live blob API #1905). In the other, the local Bun 1.4.4's own lock migration prints different output.
• The CLI suites that touch workspaces pass after the review fixes: in_process_redirect, in_process_vendor, in_process_vendor_bun_takeover, in_process_alternate_installers, in_process_redirect_pnpm, scan_vendor_e2e. The only exceptions are the same root-only write-failure tests.
• npm, PyPI and gem wrappers: no workspaces handling, so nothing to change.

Per-issue checklist:

• Read package.json workspaces through one helper: five readers disagree on non-string items, null and empty lists #1900: one reader, used by all five sites; the malformed rule is decided by the probe above; each changed site has a regression test.

https://claude.ai/code/session_011f9aeT3W3otMfyaPAWqHzf

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ 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 f5135db. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Ready for review at f5135db9 (burn-down agent).

• CI: 82/82 settled green (31 success, 50 skipped, 1 neutral).
• Bugbot: reviewed f5135db9, no findings.
• Mergeable, no conflicts; no CHANGELOG change.
• Reviewer focus: fixes Read package.json workspaces through one helper: five readers disagree on non-string items, null and empty lists #1900. package.json workspaces parsing now goes through one shared reader in formats/package_json.rs used by the npm crawler, governing root, yarnrc redirect, user patches, update channel and vendor. Check the malformed-field handling.
• Slack announcement: not sent this run (Slack send unavailable); the next run retries.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does: Adds one formats::package_json::Workspaces classifier (None / Patterns / Malformed(strings it holds)) and routes the five hand-written package_json.workspaces readers through it: governing_root, the Bun member walk, update::channel::declares_workspaces, the yarn classic risk warning in vendor, and user_patches. Readers that only widen (refusals, crawl roots, warnings, patch search) keep a malformed field's string items. The Bun walk needs an exact member set, so it treats a malformed field as unreadable and falls back to bun.lock.

Risk: low. Every behaviour change only affects manifests that npm and Bun refuse to install (workspaces: null, [1], {"packages":[1]}). I traced each old reader against the new one. governing_root now returns Some([]) where it used to return None, and both its call sites still take the same continue path.

Look here:

• package_json.rs:46-68:`` Workspaces::of, which classifies the field
• package_json.rs:82-103:``` into_listed / `listed_patterns` / `lists_any`
• npm_crawler.rs:918-926:```` the Bun walk returns None on Malformed (fallback at L890-895)
• governing_root.rs:734, channel.rs:391, vendor/mod.rs:235: the other call sites

Verified: I read the full diff against #1900. Targeted socket-patch-core tests (package_json, bun_workspace, yarnrc, declares_workspaces, user_patches, governing_root, skipped_members) gave 91 passed, 0 failed. cargo fmt --check is clean. CI is 82/82 green (ci-ok, clippy). Bugbot is neutral. CHANGELOG.md is untouched, and the diff adds no debug output and no CRLF. The new channel, vendor and Bun-null tests would fail on main's code.

Changes I made: none.

Open questions / nits (non-blocking):

• The description lists user_patches::tests::malformed_workspaces_still_walk_their_string_patterns as red on main. It isn't: I restored main's parser and the test still passes, so it is a guard test, not a regression test.
• With [1], the yarn upgrade hint (channel) and the yarn classic warning (vendor) no longer fire. No package manager installs that manifest, so this looks fine.

Auto-merge is armed, so approving sends this straight to the merge queue.

---

Generated by Claude Code

[Wenxin Jiang (Wenxin-Jiang)]

[agent] Reviewed f5135db9a56fb1e6c420063986a21f9cda9fc355. Checked shared package.json workspace shapes, malformed-field handling and widening versus exact readers. No major correctness issues found. Validation: source and regression-test review plus the available exact-head CI checks; no local test pass is claimed.