Skip to content

pack/publish: resolve workspace: and catalog: specs from the package.json files, not bun.lock - #38813

Open
robobun wants to merge 8 commits into
mainfrom
farm/672d542e/pack-workspace-catalog-from-manifests
Open

pack/publish: resolve workspace: and catalog: specs from the package.json files, not bun.lock#38813
robobun wants to merge 8 commits into
mainfrom
farm/672d542e/pack-workspace-catalog-from-manifests

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • bun pm pack and bun publish replace workspace:* / workspace:^ / workspace:~ and catalog: specs with the versions recorded in bun.lock, which only holds what the package.json files said at the last bun install (edit_root_package_json in src/runtime/cli/pack_command.rs: lockfile.workspace_versions and lockfile.catalogs).
  • A release bumps versions (bun pm version, changesets) and edits catalogs after that install, so the tarball and the manifest sent to the registry carry the pre-bump dependency versions next to the package's own new version. With this repo state, @acme/utils@0.4.1 gets published with "@acme/core": "1.2.3", "^1.2.3" and "react": "^18.3.1" while @acme/core@1.3.0 is published in the same run; consumers of a first release get No matching version, later releases silently pin the previous core. Exit code 0, no warning. Same in 1.3.14; pnpm publishes the on-disk values.
  • Root cause is the data source, not the substitution: bun.lock is a cache of the manifests, and it is stale in exactly the situation pack runs in.

Fix

  • Pack and publish no longer read the lockfile at all. When the manifest being packed contains a spec that needs resolving, pack() parses the root package.json and the workspaces it lists from disk the way bun install does (WorkspaceManifests::load in src/install/PackageManager/workspace_manifests.rs, built on the existing ScratchManifests) and edit_root_package_json resolves workspace: versions and catalog: ranges from that.
  • This is the parse (Package::parse_with_json) that produces the workspace_versions and catalogs bun.lock is written from, run with only the parts that produce them enabled: the workspaces walk and the catalogs (Features { is_main, workspaces } in WorkspaceManifests::load). So every input that resolved before resolves to the same thing, just from the current files, and the root's own dependency sections are not parsed: bun install's checks on them (a workspace:<range> in the root that no workspace satisfies) do not decide whether the root or a member packs, and such a range is still packed as written. parse_root takes the features as a parameter; the --filter relation graph and the add/remove write-back keep Features::main(), which they need for the dependency rows. No lockfile is needed anymore, so these specs also resolve in a checkout that has not run bun install.
  • The manifests are only read when a workspace:^|~|* or catalog: spec is present (needs_workspace_manifests), so a package without them packs regardless of the state of the workspace around it, as before. When one is present and the root manifests do not parse (a workspaces entry pointing at a missing directory, duplicate workspace names), pack now prints the same error bun install prints instead of falling back to stale data. That includes the errors the parse only logs before continuing (an invalid catalog range, a duplicate catalog entry): ScratchManifests::parse_root / parse_member now return InstallFailed when the parse logged errors. ScratchManifests is shared with the --filter relation graph (bun add --filter '...x'), which therefore also fails on such a root instead of building the graph without the affected entries; bun install fails on the same input either way. The add/remove package.json write-back (sync_lockfile) re-parses through ScratchManifests too; it runs after resolution has already printed those same errors (install_with_manager prints and resets the log before it), so on such a root the command now fails at that point instead of at the has_errors check a few steps later, with the same output.
  • The two error messages the lockfile produced are replaced: workspace: now reports "<root>/package.json" has no workspace named "x", or its package.json has no version; the catalog: "no matching catalog dependency" message is unchanged and "catalogs require a lockfile" is gone.
  • After lifecycle scripts ran, the whole package.json cache is cleared instead of one entry, so versions a prepack/prepublishOnly script writes to other workspaces are seen as well (this also removes the Windows-only cache key conversion). The package.json being packed is re-read after loading the manifests because loading can add entries to the hashbrown-backed cache the entry lives in.
  • edit_root_package_json is restructured around a Substitution classifier shared with needs_workspace_manifests, so the two cannot disagree about which specs need the manifests.
  • Verified with test/cli/install/bun-pack.test.ts (workspace versions, catalogs, no lockfile, packing the root, prepack editing a sibling, error messages), test/cli/install/catalogs.test.ts (catalog edited after install, invalid catalog range reported at its definition), test/cli/install/bun-publish.test.ts (manifest received by a mock registry). The new tests fail on the current release and pass with this change; the full bun-pack, catalogs, bun-add-catalog and bun-add-filter files pass with the debug build. bun-pack.test.ts also pins the root case (workspace:9.9.9 in the root next to a workspace:*: packing the root and packing a member both succeed; both failed with No matching version for workspace dependency while the root was parsed with Features::main()) and a workspaces entry that does not exist (Workspace not found for a pack that has a spec to resolve, success for a sibling that has none).
  • bun-pack.test.ts and bun-publish.test.ts also cover an empty bun.lock, a bun.lock with git conflict markers and a corrupt bun.lockb: packing a package without workspace specs, packing one with workspace:*, and publishing one with workspace:^ all succeed with nothing on stderr. The current release exits 1 with failed to parse lockfile in all three cases because it loaded the lockfile unconditionally.
  • Supersedes pack: resolve workspace:* to the dependency's package.json version, not bun.lock's #36279 and pack/publish: warn and continue when the lockfile is unreadable #36260, both closed in favor of this one. pack: resolve workspace:* to the dependency's package.json version, not bun.lock's #36279 followed the lockfile's workspace paths to each package.json and fell back to the lockfile's version when that package.json had no version; this change does not keep that fallback, since the lockfile's version is the stale value the issue is about (a workspace with no version now fails with the error above, covered by fails when no workspace with a version matches). pack/publish: warn and continue when the lockfile is unreadable #36260 made the lockfile load errors a warning; the load no longer exists.

Fixes #20477
Fixes #20829 (closed as a duplicate of #20477; same scenario with a peer dependency)

The pack half of #28935 is the same bug; the other half, bun pm version / bun install not refreshing the workspace versions in bun.lock, is #18906 (#36285, #28936) and is unaffected by this change, which just stops pack from depending on it.

Background

  • workspace: and catalog: are protocols that only mean something inside the monorepo: workspace:^ means "the sibling workspace package, and publish it as ^<its current version>", catalog: / catalog:<name> means "the range the root package.json defines for this dependency in that catalog". Pack has to rewrite both into plain ranges because registry consumers have neither the workspace nor the catalog.
  • bun.lock stores the workspace versions and the catalogs it saw at the last install (its workspaces and catalog sections); bun install computes them by parsing the root package.json, whose workspaces globs lead it to read every member's package.json. ScratchManifests runs that same root parse into a throw-away Lockfile and already existed for --filter relation graphs; WorkspaceManifests wraps it and exposes the two lookups pack needs.
  • PackageManager::init has already located the workspace root (and chdir'd there) by the time pack runs, which is why root_package_json_path() is the root's package.json when packing a member and the package's own otherwise.
  • WorkspacePackageJSONCache is a hash map of parsed package.json files keyed by path; pack() holds a reference into it, and inserting into a hash map can move its entries, hence the re-read after the manifests are loaded.
Repro (before / after)
mkdir -p m/packages/core m/packages/utils && cd m
cat > package.json <<'EOF'
{ "name": "mono", "private": true,
  "workspaces": { "packages": ["packages/*"], "catalog": { "react": "^18.3.1" } } }
EOF
echo '{ "name": "@acme/core", "version": "1.2.3" }' > packages/core/package.json
cat > packages/utils/package.json <<'EOF'
{ "name": "@acme/utils", "version": "0.4.0",
  "dependencies": { "@acme/core": "workspace:*" },
  "peerDependencies": { "@acme/core": "workspace:^", "react": "catalog:" } }
EOF
bun install
( cd packages/core  && bun pm version minor --no-git-tag-version )   # core  -> 1.3.0
( cd packages/utils && bun pm version patch --no-git-tag-version )   # utils -> 0.4.1
sed -i 's/\^18.3.1/^19.1.0/' package.json
( cd packages/utils && bun pm pack && tar -xzOf acme-utils-0.4.1.tgz package/package.json )

Before: "@acme/core": "1.2.3", peer "^1.2.3", "react": "^18.3.1".
After: "@acme/core": "1.3.0", peer "^1.3.0", "react": "^19.1.0", and the same with bun.lock deleted.

Error paths with this change:

$ bun pm pack   # dependency "nope": "workspace:*", no such workspace
error: Failed to resolve workspace version for "nope" in `dependencies` ("/tmp/m/package.json" has no workspace named "nope", or its package.json has no version).
$ bun pm pack   # peer "vue": "catalog:", not in any catalog
error: Failed to resolve catalog version for "vue" in `peerDependencies` (no matching catalog dependency).
$ bun pm pack   # root "workspaces" lists a missing directory, package uses workspace:*
error: Workspace not found "missing-dir"
    at /tmp/m/package.json:1:62

no test proof · iteration 7 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-pack.test.ts test/cli/install/bun-publish.test.ts

… files, not bun.lock

`bun pm pack` and `bun publish` substituted `workspace:^|~|*` and `catalog:`
dependency specs with the versions recorded in bun.lock, which only holds what
the package.json files said at the last install. A release bumps versions
(`bun pm version`, changesets) and edits catalogs after that install, so the
tarball and the registry manifest carried the pre-bump versions while the
package's own version was the new one.

Pack no longer reads the lockfile at all. When the manifest being packed has a
spec that needs substituting, it parses the root package.json and the
workspaces it lists the same way `bun install` does (WorkspaceManifests, built
on the existing ScratchManifests) and resolves from that. Packages without such
specs never touch the other manifests. The package.json cache is cleared after
lifecycle scripts so versions they write to other workspaces are seen too.

Fixes #20477
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 2:38 PM PT - Aug 15th, 2026

@robobun, your commit de34ed949c0e07c761e5ba1f013f1fd3c3a2e7ae passed in Build #98630! 🎉


🧪   To try this PR locally:

bunx bun-pr 38813

That installs a local version of the PR into your bun-38813 executable, so you can run:

bun-38813 --bun

@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 22bb05ba-abf5-4c10-8693-97da63bfc584

📥 Commits

Reviewing files that changed from the base of the PR and between 2acfb09 and de34ed9.

📒 Files selected for processing (7)
  • src/install/PackageManager/package_json_write_back.rs
  • src/install/PackageManager/workspace_manifests.rs
  • src/runtime/cli/pack_command.rs
  • src/runtime/cli/publish_command.rs
  • test/cli/install/bun-pack.test.ts
  • test/cli/install/bun-publish.test.ts
  • test/cli/install/catalogs.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Reproduced with the repro in the description on the current release: after bun pm version in both workspaces and a catalog edit, the packed @acme/utils@0.4.1 still had "@acme/core": "1.2.3", "^1.2.3" and "react": "^18.3.1" from bun.lock. With this branch the tarball (and the manifest bun publish sends, checked against a mock registry in the new publish test) carries 1.3.0, ^1.3.0 and ^19.1.0, with or without a bun.lock present.

Tests: bun-pack.test.ts (stale lockfile, no lockfile, packing the root, a prepack script editing a sibling, the error message, the unreadable-lockfile cases taken over from #36260, a root whose own workspace:<range> no workspace satisfies, a missing workspaces entry), catalogs.test.ts (catalog edited after the install, invalid catalog range reported at its definition) and bun-publish.test.ts (manifest received by a mock registry, with and without a parseable bun.lock). They fail on the release build and pass with the debug build; on the current head, bun-pack (workspaces block), the catalogs pack tests, bun-add-catalog and bun-add-filter (the other ScratchManifests users) pass with the debug build, and bun add on a root with an invalid catalog range prints the located error and exits 1.

Commits: 0031b3e the fix; 1f5fdca the #36260 tests; 56dd781 shorter comments; 662c8ee errors the manifest parse only logs fail pack like they fail bun install (review thread on load()); 15e94df drains stdout in the new test (review nit); 490b252 load() parses only the root's workspaces and catalogs, not its dependency sections, so a workspace:<range> in the root that no workspace satisfies no longer blocks packing (it never did before this PR either). All review threads are replied to and resolved.

CI: builds 97108 (first commit) and 97783 (through 15e94df, full matrix including macOS) passed. Builds 98253 and 98576 (490b252 and an empty rerun) were red only on test/regression/issue/09041.test.ts on the x64 ASAN lane: the FileReader EOF use-after-free fixed on main by #38886 after this branch was cut, unrelated to pack (the branch's base predated the fix; main passes the test). de34ed9 merges current main to pick it up; build 98630 on the merged head has 177 of 179 jobs passed with the ASAN lane green, the two macOS 14 arm64 lanes still waiting for agents.

Related: #36279 and #36260 are closed in favor of this PR. #38835 (workspace:<path>) is based on this branch and #38788 (workspace: aliases) on #38835, so they rebase once this lands. The bun.lock refresh side of the same release flow (#18906) is #36285 / #28936 and is independent of this change.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this PR and the automated bug-hunting pass found no issues. Because it changes the data source bun pm pack/bun publish use to resolve workspace: and catalog: specs (lockfile → on-disk manifests), reworks edit_root_package_json, and changes user-facing error messages, a human sign-off on the behavior change would still be worthwhile.

Checked: Substitution::for_spec matches the old prefix/trim logic for both protocols; needs_workspace_manifests and edit_root_package_json share the same classifier so the .expect() on workspace_manifests cannot fire; the re-read of json after WorkspaceManifests::load correctly handles the hashbrown cache growing; map.clear() after lifecycle scripts is a strict superset of the old single-entry removal (and drops the Windows-only path-separator conversion). The removed lockfile field on pack::Context has no remaining consumers.

Extended reasoning...

Overview

This PR changes bun pm pack and bun publish to resolve workspace:^|~|* and catalog: specifiers from the workspace's package.json files on disk instead of bun.lock. It adds WorkspaceManifests (wrapping the existing ScratchManifests) in src/install/PackageManager/workspace_manifests.rs, removes lockfile loading from both pack_command.rs and publish_command.rs, restructures edit_root_package_json around a shared Substitution classifier, and broadens post-lifecycle-script cache invalidation from one entry to the whole WorkspacePackageJSONCache. ~250 lines of Rust changed plus ~200 lines of new test coverage across three test files.

Security risks

None identified. This is CLI-side manifest reading using the same Package::parse_with_json path bun install already uses; no new untrusted-input parsing surface. bun publish sends data to a registry, but the substituted values come from local package.json files the user controls, same as before (just fresher).

Level of scrutiny

High. bun publish is irreversible (packages hit a public registry), and this PR changes what version ranges get written into published manifests. It also swaps a data source (lockfile → live manifests), which is a design decision — it now fails hard if the workspace root's manifests don't parse where before it would fall back to whatever the lockfile had. The PR description argues this is correct (matches pnpm, avoids stale versions), but that's a maintainer call. User-visible error messages change too.

Other factors

  • Test coverage is thorough: stale-lockfile case (the actual bug), no-lockfile case, packing the root, prepack scripts editing sibling workspaces, both error-message paths, and a mock-registry check on the actual PUT body bun publish sends. An existing test asserting the old "Run `bun install`" error message was correctly updated.
  • The crash() refactor in workspace_manifests.rs preserves the existing relation_graph behavior (log-first, generic message as fallback).
  • The pointer-safety note about re-reading json after WorkspaceManifests::load grows the cache is correct — ScratchManifests::parse_root calls fetch_entry, which inserts into the same workspace_package_json_cache map.
  • The PR mentions it subsumes #36279 and #36260; a maintainer should confirm those can be closed.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Nothing to change from the automated review. The two points it leaves to a maintainer are the ones called out in the description: (1) when a package uses workspace:/catalog: specs and the root manifests do not parse, pack now fails with bun install's error instead of packing from whatever bun.lock had (packages without such specs are unaffected), and (2) #36279 and #36260 become redundant if this lands. CI is still running.

Comment thread src/install/PackageManager/workspace_manifests.rs Outdated
Comment thread src/install/PackageManager/workspace_manifests.rs Outdated
Comment thread src/install/PackageManager/workspace_manifests.rs
Comment thread src/install/PackageManager/workspace_manifests.rs
Comment thread src/install/PackageManager/workspace_manifests.rs
Comment thread src/runtime/cli/pack_command.rs Outdated
Comment thread src/runtime/cli/pack_command.rs
Comment thread src/runtime/cli/pack_command.rs Outdated
Comment thread src/runtime/cli/pack_command.rs Outdated
Comment thread src/runtime/cli/pack_command.rs Outdated
Comment thread src/runtime/cli/pack_command.rs Outdated
Comment thread src/runtime/cli/pack_command.rs Outdated
@robobun
robobun force-pushed the farm/672d542e/pack-workspace-catalog-from-manifests branch from b21925d to 1f5fdca Compare August 15, 2026 07:49
Comment thread src/install/PackageManager/workspace_manifests.rs
Comment thread src/runtime/cli/pack_command.rs
Comment thread src/install/PackageManager/workspace_manifests.rs
CatalogMap::parse_append_group logs "Invalid dependency version" and
"Duplicate catalog" and returns Ok, so ScratchManifests::parse_root returned
Ok with errors in its log. bun install fails on those; pack silently dropped
them and went on to report the catalog entry as missing. parse_root and
parse_member now fail when the parse logged errors, so pack (and the --filter
relation graph) print them the way bun install does.
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Two follow-up commits from the reviews: 56dd781 shortens the comments the comment lint flagged (the ones kept are one- or two-line contracts, replies on each thread say which), and 662c8ee addresses the finding on WorkspaceManifests::load: errors the manifest parse only logs, such as an invalid catalog range or a duplicate catalog entry, now fail bun pm pack / bun publish with the located error bun install prints, instead of being dropped and surfacing later as a missing catalog entry. The check lives in ScratchManifests, so --filter relation graphs get the same behavior. New test in catalogs.test.ts; the CodeRabbit comments are rate-limit notices with nothing to act on.

Comment thread test/cli/install/catalogs.test.ts Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Both earlier findings are addressed (662c8ee surfaces logged catalog-parse errors; 15e94df drains stdout in the new test), and this run found no further issues. This is a well-scoped fix with thorough coverage, but it changes what pack/publish read from and adds fail_on_logged_errors to the shared ScratchManifests (so --filter relation graphs now also fail on log-only errors) — a maintainer should confirm those trade-offs.

Reviewed: the Substitution/needs_workspace_manifests split matches edit_root_package_json's use of manifests(); the cache re-read after WorkspaceManifests::load covers the hashbrown-move hazard the PR describes; map.clear() after lifecycle scripts is safe (only pack's own entry was live); the removed lockfile-load in from_workspace leaves no other consumer of ctx.lockfile.

Extended reasoning...

Overview

The PR changes bun pm pack and bun publish to resolve workspace:^|~|* and catalog: specifiers from the on-disk package.json files (via a new WorkspaceManifests wrapper around the existing ScratchManifests) instead of from bun.lock. It removes the lockfile load from both exec_pack and Context::from_workspace, restructures edit_root_package_json around a shared Substitution classifier, clears the whole WorkspacePackageJSONCache after lifecycle scripts instead of one entry, and adds fail_on_logged_errors to ScratchManifests::parse_root/parse_member. ~200 lines of Rust across three files plus ~300 lines of new tests in three test files.

Security risks

None identified. The change reads package.json files that bun install already reads with the same parser (Package::parse_with_json). No new external input surface, no path handling changes, no network/registry changes beyond removing a lockfile read.

Level of scrutiny

Medium-high. This is package-manager correctness code on the publish path — a wrong substitution ships to the registry. The refactor is disciplined (the Substitution enum keeps needs_workspace_manifests and edit_root_package_json in lockstep, so the .expect() on workspace_manifests is sound), and the test matrix is broad (stale lockfile, no lockfile, unreadable lockfile × 3, packing the root, prepack editing a sibling, both error messages, publish manifest capture, invalid catalog range). But there are two intentional behavioral changes a maintainer should confirm: (1) when a package has workspace:/catalog: specs and the root manifests don't parse, pack now fails with bun install's error instead of packing from stale lockfile data; (2) fail_on_logged_errors now also applies to relation_graph (the --filter path), so a workspace with a log-only manifest error (invalid catalog range, duplicate catalog entry) will now fail bun add --filter '...' where it previously didn't.

Other factors

Both of my earlier findings were addressed in follow-up commits (662c8ee, 15e94df). The comment-cop bot flagged several multi-line comments; the author shortened most in 56dd781 and replied on each thread explaining which were kept and why. The PR supersedes two closed PRs (#36279, #36260) and two more (#38835, #38788) are stacked on it, so a maintainer should be aware of the sequencing. CI on the head commit was still running per the last robobun update. No human reviewer has looked at it yet.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

No further changes from the latest automated pass. The one trade-off it asks a maintainer to confirm, ScratchManifests failing on log-only manifest errors for --filter relation graphs as well as for pack, is now spelled out in the description (Fix section, third bullet). All review threads are resolved; CI build 97783 is running on 15e94df.

WorkspaceManifests::load parsed the root with Features::main(), which also
runs bun install's checks on the root's own dependency sections. A
workspace:<range> there that no workspace satisfies made bun pm pack fail,
for the root and for members alike, but only when some other spec made
pack load the manifests. Only the workspaces walk and the catalogs are
what pack substitutes from, so load() parses just those.
Comment thread src/install/PackageManager/workspace_manifests.rs
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

490b252 narrows what WorkspaceManifests::load parses. With Features::main() the root's own dependency sections went through bun install's checks too, so a workspace:<range> in the root that no workspace satisfies ("pkg1": "workspace:9.9.9" next to a "pkg2": "workspace:*") failed bun pm pack with No matching version for workspace dependency "pkg1", both when packing the root and when packing a member whose own specs had nothing to do with it, and only if some spec triggered the load at all (a root with just the workspace:9.9.9 dependency packed it as written). load now enables only the workspaces walk and the catalogs, which are the two things pack reads; parse_root takes the features as a parameter and the --filter relation graph and the add/remove write-back keep Features::main().

Tests added to the workspaces block of bun-pack.test.ts: the root case above (root and member; both fail on 15e94df, pass now) and a workspaces entry that does not exist on disk (Workspace not found with the location for a pack that has a spec to resolve, success for a sibling that has none). bun-pack (90), bun-publish (41), the pack tests in catalogs, bun-add-filter (123) and bun-add-catalog (149) pass with the debug build. The description's Fix section is updated to match.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this and found no bugs. My two earlier findings (the dropped scratch.log on the Ok path, and the undrained stdout in the new catalogs test) are both addressed. Deferring to a maintainer for the behavior trade-offs the description already calls out — pack now failing on manifest parse errors when a workspace:/catalog: spec is present, and fail_on_logged_errors reaching the --filter relation graph — rather than for anything unresolved in the diff.

Also checked in this pass:

  • Substitution::for_spec vs. the old inline logic — workspace:^|~|*, workspace:<range>, catalog: and catalog:<name> (with whitespace) all classify the same as before; needs_workspace_manifests and edit_root_package_json share it so they cannot disagree.
  • 490b252's Features { dependencies: false, peer_dependencies: false, ... } — confirmed workspace_versions and catalogs are populated via is_main/workspaces, independent of the root's own dependency sections; the new "workspace: range in the root that no workspace satisfies" tests cover this.
  • fail_on_logged_errors reaching sync_lockfile via parse_root — that path only runs after a successful resolve, which already fails on the same inputs, so no new failure surface there.
Extended reasoning...

Overview

Changes the data source bun pm pack / bun publish use to substitute workspace:^|~|* and catalog: specs: instead of loading bun.lock (stale after a version bump), pack() parses the root and workspace package.json files via a new WorkspaceManifests wrapper around the existing ScratchManifests. The lockfile load is removed from both exec_pack and Context::from_workspace; the Context.lockfile field is deleted. edit_root_package_json is refactored around a Substitution enum shared with a new needs_workspace_manifests gate so packages without such specs never load the manifests. ScratchManifests::parse_root/parse_member now return InstallFailed when the parse logged errors (662c8ee), and parse_root gained a Features parameter (490b252) so pack can skip the root's own dependency sections. ~270 lines of new tests across bun-pack.test.ts, bun-publish.test.ts, and catalogs.test.ts.

Security risks

None identified. No new untrusted-input parsing (reuses Package::parse_with_json), no network/credential handling changes, no path construction from user data. The removed lockfile load path had no security relevance either.

Level of scrutiny

High. This is a behavioral change to a production package-manager path (bun pm pack / bun publish) that decides what dependency ranges get published to registries. It also widens ScratchManifests' error surface, which is shared with --filter relation graphs and package_json_write_back::sync_lockfile. The change is well-scoped and thoroughly tested, but the two trade-offs the author flagged (pack failing on manifest parse errors; the --filter side effect) are product decisions a maintainer should confirm.

Other factors

  • Both of my earlier findings were fixed (662c8ee, 15e94df) and covered by new tests.
  • Test coverage is strong: stale lockfile, no lockfile, unreadable lockfile (empty / conflict markers / corrupt .lockb), packing the root, prepack editing a sibling, unversioned/missing workspace, missing workspaces entry, invalid catalog range, and the publish manifest received by a mock registry. All stated to fail on the release build and pass on the debug build.
  • The one open thread is a comment-cop lint on the 3-line comment above the Features block in load(); it explains why dependencies: false is set and reads as design rationale rather than a workaround.
  • Supersedes #36279 and #36260; #38835 and #38788 are stacked on this branch.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Reviewed 490b252 (see the comment above it for the motivating case): agreed, and it is the right narrowing. load() now parses only what pack substitutes from, the workspaces walk and the catalogs, so bun install's checks on the root's own dependency rows no longer decide whether anything packs; that matches what the command did before this PR, which never looked at those rows. The description covers it (Fix, second and third bullets), and the remaining lint thread on its comment is resolved. The only red test on build 98253 is test/regression/issue/09041.test.ts on the ASAN lane, a use-after-free in bun run with a piped stdin that does not touch this code; it has been reported on its own.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this PR and the automated bug-hunting pass found no issues; the two earlier findings (dropped scratch.log on the Ok path, undrained stdout in the catalogs test) were addressed in 662c8ee and 15e94df. Given the scope — swapping pack/publish's data source from bun.lock to on-disk manifests, and the fail_on_logged_errors change to shared ScratchManifests that also affects --filter relation graphs and sync_lockfile — a maintainer should confirm the design trade-offs called out in the description before this lands.

What was reviewed:

  • Substitution::for_spec / needs_workspace_manifests agree on which specs need the manifests, and WorkspaceRange is correctly excluded so packages without ^/~/*/catalog: specs still pack regardless of workspace state.
  • The re-read of json after WorkspaceManifests::load (hashbrown insert can move entries) and after map.clear() post-lifecycle-scripts.
  • fail_on_logged_errors in parse_root/parse_member and its effect on the two other ScratchManifests callers; sync_lockfile runs after resolution has already printed those errors, so no user-visible regression there.
  • The new tests cover stale lockfile, no lockfile, unreadable lockfile, packing the root, prepack editing a sibling, unsatisfied workspace:<range> in the root, and a missing workspaces entry.
Extended reasoning...

Overview

This PR changes bun pm pack and bun publish to resolve workspace:^|~|* and catalog: dependency specs from the package.json files on disk instead of bun.lock, fixing #20477 (stale versions published after a version bump). It touches src/runtime/cli/pack_command.rs (removes lockfile loading, adds Substitution classifier and needs_workspace_manifests, restructures edit_root_package_json), src/runtime/cli/publish_command.rs (removes lockfile loading), src/install/PackageManager/workspace_manifests.rs (new WorkspaceManifests type, fail_on_logged_errors on ScratchManifests, parse_root now takes Features), and src/install/PackageManager/package_json_write_back.rs (passes Features::main() to the changed signature). ~270 net new test lines across three test files.

Security risks

None identified. The change reads local package.json files the same way bun install already does (via the existing ScratchManifests / parse_with_json path). No new untrusted-input parsing, no path handling changes, no network or auth surface.

Level of scrutiny

High. bun pm pack and bun publish are production-critical — a bug here ships wrong dependency ranges to the registry, which is exactly the class of bug this PR fixes. The change also alters shared infrastructure: ScratchManifests::parse_root / parse_member now return InstallFailed when the parse logged errors, which changes behavior for the --filter relation graph and sync_lockfile in package_json_write_back.rs. The PR description and follow-up comments explicitly call out two design decisions for maintainer confirmation: (1) pack now fails with bun install's error when manifests don't parse rather than falling back to stale bun.lock data, and (2) the fail_on_logged_errors behavior propagating to --filter.

Other factors

The PR is thoroughly tested (workspace versions, catalogs, no lockfile, unreadable lockfile, packing the root, prepack editing a sibling, error messages, unsatisfied workspace:<range> in root, missing workspaces entry), and CI has passed on earlier commits with only an unrelated ASAN failure on the latest. My two earlier inline findings were addressed. Two follow-up PRs (#38835, #38788) are based on this branch, and it supersedes #36279 and #36260. The comment-cop lint threads are all resolved with stated rationales for the comments kept. Given the scope, the cross-cutting ScratchManifests behavior change, and the author's own note that maintainer confirmation is expected on the trade-offs, this warrants human review rather than auto-approval.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

The ASAN red on builds 98253 and 98576 (test/regression/issue/09041.test.ts, heap-use-after-free while bun run pipes stdin to stdout) is a bug in this branch's base, not in this change: it is the FileReader EOF use-after-free that #38886 (88a6398) fixed on main this morning, and this branch was still based on yesterday's 26ec349. Main's builds pass the test; the earlier builds of this branch (97108, 97783) passed it by timing. de34ed9 merges current main into the branch (a merge rather than a rebase so #38835, which is based on this branch, keeps its diff); no conflicts, and main's #38743 placeholder assertion is exercised by the error-message tests here, which pass on the merged build along with the rest of the pack, catalog and publish tests and the 09041 fixture. Build 98630 is running on it. The automated review passes since then report nothing new.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed this PR and didn't find any bugs (my two earlier findings — the dropped scratch.log on the Ok path and the undrained stdout pipe — were addressed in 662c8ee and 15e94df). Because it changes the data source for workspace:/catalog: substitution in pack/publish, widens ScratchManifests error behavior to --filter and the add/remove write-back, and supersedes two other open PRs, a maintainer sign-off on the design trade-offs the author flagged would still be worthwhile.

What was reviewed:

  • Substitution::for_spec / needs_workspace_manifests / edit_root_package_json — checked that the classifier and the substitution agree on which specs need manifests, and that WorkspaceRange still packs the range as written.
  • WorkspaceManifests::load with the narrowed Features — verified the root's own dependency rows are not validated, so an unsatisfied workspace:<range> in the root no longer blocks packing (pinned by the two new tests).
  • The fail_on_logged_errors propagation into sync_lockfile and relation_graph — the description's claim that bun install already fails on the same inputs before those points holds for the catalog-parse errors I traced.
  • The cache re-read after WorkspaceManifests::load and after lifecycle scripts — the hashbrown-insert invalidation is real and both re-reads are placed correctly.
Extended reasoning...

Overview

This PR changes bun pm pack and bun publish to resolve workspace:^|~|* and catalog: dependency specs from the on-disk package.json files instead of bun.lock, fixing #20477/#20829 where a version bump between bun install and bun publish produced tarballs with stale dependency versions. It adds WorkspaceManifests (wrapping the existing ScratchManifests), removes the lockfile load from both commands entirely, restructures edit_root_package_json around a Substitution classifier, and clears the whole package.json cache after lifecycle scripts. It also makes ScratchManifests::parse_root/parse_member return InstallFailed when the parse logged errors, which propagates to --filter relation graphs and the add/remove write-back (sync_lockfile). ~400 lines of new tests across three files.

Security risks

None identified. The change reads package.json files the process already had to read, using the same parser bun install uses. No new external input surface, no path handling changes, no network or auth code touched (the publish diff only removes the lockfile load).

Level of scrutiny

Medium-high. This is user-facing package-manager behavior with a real correctness bug being fixed, but the fix is a data-source swap plus a restructuring, not new algorithm work. The blast radius extends beyond pack/publish because fail_on_logged_errors affects every ScratchManifests caller. The author has explicitly flagged two decisions for maintainer confirmation: (1) pack now fails with bun install's error when a workspace:/catalog: spec is present and the root manifests do not parse, instead of silently using stale lockfile data; (2) this supersedes #36279 and #36260, which took different approaches.

Other factors

  • My two earlier findings on this PR were both fixed (662c8ee for the dropped log, 15e94df for the undrained pipe); all review threads including the comment-cop lint threads are resolved.
  • Test coverage is thorough: stale lockfile, no lockfile, unreadable lockfile (three variants), packing the root vs. a member, prepack editing a sibling, error messages for missing workspace / missing catalog entry / invalid catalog range / missing workspaces directory, and the Features::main() regression case. The publish side is covered with a mock registry asserting the manifest body.
  • CI: build 98253 had one unrelated ASAN failure (issue 09041, bun run stdin UAF) and two expired macOS jobs; 98576 is a clean rerun in progress.
  • Downstream PRs #38835 and #38788 are based on this branch, and #36279/#36260 would be closed if this lands — a maintainer should confirm that direction.
  • The Features narrowing in 490b252 is subtle enough (deliberately not validating the root's own dependency sections) that the retained multi-line comment explaining it is justified, and the two tests pin it.

Given the explicit maintainer-confirmation points the author raised and the cross-cutting ScratchManifests change, this should get a human look rather than auto-approval.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Build 98630 on the merged head confirms the diagnosis above: 177 of 179 jobs passed, including every x64 ASAN shard (the 09041 test passes now that the branch contains #38886); the remaining two jobs are the macOS 14 arm64 lanes waiting for agents. The latest automated pass reports nothing new and every review thread has a reply and is resolved, so this is ready for a maintainer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

2 participants