GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix package.json workspaces read five ways (#1900) by mikolalysenko · Pull Request #1971 · SocketDev/socket-patch

Conversation

Mikola Lysenko (mikolalysenko) commented Nov 2, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Start fix for #1900

d803c39

Assisted-by: Claude Code:claude-opus-5-5

Mikola Lysenko (mikolalysenko) mentioned this pull request

Nov 2, 2026

Read package.json workspaces through one helper: five readers disagree on non-string items, null and empty lists #1900

Closed

Claude (claude) added 2 commits

November 2, 2026 04:31


          Read package.json workspaces one way (#1900)

6d9f5e3

Five places parsed a package.json `workspaces` field by hand and
disagreed on what a malformed field means: a list with a non-string
item, `null`, or the wrong type. Depending on the caller, the same
manifest made the Bun member walk read no members, made patch
discovery skip the bad item silently, or made the yarn workspace
warning and the upgrade hint think workspaces were declared.

npm 10.9, yarn 1.22.22 and Bun 1.4.4 all refuse to install a list with
a non-string item, and npm and Bun also refuse `null`, a string, and a
non-array `packages`. A new formats::package_json::Workspaces reader
classifies the field once (absent, patterns, or malformed), and every
caller reads it:

- governing-root refusals and the npm crawler's extra roots keep
  reading a malformed field's string items, so they still refuse or
  crawl on the safe side;
- the Bun member walk treats a malformed field as unreadable and falls
  back to bun.lock;
- patch discovery records the malformed manifest as unknown, so a
  project patch cannot be ruled out silently;
- the yarn classic warning and the upgrade hint need at least one
  pattern string.

Assisted-by: Claude Code:claude-opus-5-5


          Keep patch search quiet on bad workspaces

f5135db

Review follow-up for #1900. Patch discovery no longer flags every npm
package as possibly user-patched when a package.json carries a
malformed `workspaces` field. pnpm ignores the field and yarn 1
installs some malformed shapes, and the string patterns are still
walked, so the flag added no safety.

Callers now read the shared reader directly instead of going through
a wrapper in the hosted module, and the module doc says which readers
widen and which treat a malformed field as unreadable.

Assisted-by: Claude Code:claude-opus-5-5

Mikola Lysenko (mikolalysenko) marked this pull request as ready for review

November 2, 2026 05:10

Mikola Lysenko (mikolalysenko) commented Nov 2, 2026

Copy link

Copy Markdown

Collaborator Author

BugBot review

Generated by Claude Code

cursor[bot] reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cursor Bot reviewed

Nov 2, 2026

View reviewed changes

cursor Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label

Nov 2, 2026

Mikola Lysenko (mikolalysenko) commented Nov 2, 2026

Copy link

Copy Markdown

Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) mentioned this pull request

Nov 2, 2026

Move npm-family workspace resolution out of hosted::governing_root: vendor, vex, scan and the crawler all import it #1974

Open

Mikola Lysenko (mikolalysenko) commented Nov 2, 2026

Copy link

Copy Markdown

Collaborator Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) enabled auto-merge

November 2, 2026 07:07

Wenxin Jiang (Wenxin-Jiang) approved these changes

Nov 2, 2026

View reviewed changes

Wenxin Jiang (Wenxin-Jiang) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mikola Lysenko (mikolalysenko) added this pull request to the merge queue

Nov 2, 2026

Merged via the queue into main with commit 1efd051

Nov 2, 2026

82 checks passed

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) deleted the agent/fix-package-json-workspaces-reader branch

November 2, 2026 16:44

Mikola Lysenko (mikolalysenko) mentioned this pull request

Nov 2, 2026

Fix vendored member refusal not naming root (#1904) #1913

Merged

Reviewers

cursor[bot]

cursor[bot] left review comments

Wenxin Jiang (Wenxin-Jiang)

Wenxin-Jiang approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Ready for review

Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Read package.json workspaces through one helper: five readers disagree on non-string items, null and empty lists

Uh oh!

There was an error while loading. Please reload this page.

3 participants

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.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Mikola Lysenko (mikolalysenko) commented Nov 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Summary

Root cause

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

Review notes (/code-review high)

Test evidence

Uh oh!

Mikola Lysenko (mikolalysenko) commented Nov 2, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Mikola Lysenko (mikolalysenko) commented Nov 2, 2026

Uh oh!

Mikola Lysenko (mikolalysenko) commented Nov 2, 2026

Final review brief

Uh oh!

Wenxin Jiang (Wenxin-Jiang) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mikola Lysenko (mikolalysenko) commented Nov 2, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Review notes (`/code-review high`)