Skip to content

pack: publish workspace: directory dependencies as the workspace's version - #38835

Open
robobun wants to merge 2 commits into
farm/672d542e/pack-workspace-catalog-from-manifestsfrom
farm/0750128c/pack-workspace-path
Open

pack: publish workspace: directory dependencies as the workspace's version#38835
robobun wants to merge 2 commits into
farm/672d542e/pack-workspace-catalog-from-manifestsfrom
farm/0750128c/pack-workspace-path

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #38813 (this PR's base branch): the change is the last two commits, and the PR retargets to main once that one lands.

Problem

  • bun install reads the text after workspace: (other than *, ^, ~) as a version range when the dependency is itself one of the workspaces, and otherwise as the directory of the workspace to link, relative to the package.json declaring it: "c7": "workspace:../core", "self": "workspace:.", "core": "workspace:packages/core" from the root (Package::parse_dependency, src/install/lockfile/Package.rs).
  • bun pm pack and bun publish copy every such spec as written (Substitution::WorkspaceRange in src/runtime/cli/pack_command.rs; the same verbatim copy before pack/publish: resolve workspace: and catalog: specs from the package.json files, not bun.lock #38813), so the tarball and the manifest sent to the registry carry "c7": "../core". Installing the published package fails: error: Could not find package.json for "file:../core" dependency "c7" (bun; npm and pnpm read it as a local directory too). Exit code 0, no warning.
  • Same in 1.3.14, so not a regression.

Fix

  • Every workspace: spec other than */^/~ is now resolved against the workspace manifests (Substitution::WorkspaceRangeOrDirectory), applying the installer's rule rather than a syntactic guess: if a workspace is in the directory the spec names, that workspace is published; otherwise, if the dependency is itself a workspace the spec is a range and is copied as written (as before, and as is workspace:<name>@<range> when <name> is a workspace); otherwise the pack fails, because bun install would have failed on the same spec and there is nothing installable to publish.
  • The directory is tried first because "pkg1": "workspace:../pkg1" (a form in bun-install.test.ts) fits both readings and only the directory reading has a version to publish.
  • A directory is published as that workspace's current version when the dependency is declared under the workspace's own name ("@acme/core": "workspace:../core" -> "1.2.3"), otherwise as the npm:<name>@<version> alias ("c7": "workspace:../core" -> "npm:@acme/core@1.2.3"), which is the registry spec that installs the same package under the same name and what pnpm publishes for this input.
  • WorkspaceManifests (src/install/PackageManager/workspace_manifests.rs) gains the two lookups this needs: has_workspace (the workspace_paths membership the installer uses) and workspace_name_at_path, which resolves the spec against the packing package's directory, makes it relative to the workspace root (/ separators on Windows) and matches it against the workspace entries of the parsed root, the same computation parse_dependency makes when installing, so pack publishes what install linked. Versions come from the same parse as workspace:*, so a version bumped after the last install is what gets published, and no lockfile is needed.
  • Because every workspace: spec now loads the manifests, a package with only workspace:<range> specs now also fails when the root manifests do not parse; such a package is in a workspace that bun install reads the same way.
  • workspace:<name>@<range> is only classified, not rewritten; pack: publish workspace: aliases as npm: aliases #38788 rewrites it to an npm: alias and is the remaining form.
  • Verified with test/cli/install/bun-pack.test.ts (10 new cases in the workspaces block: sibling directories in all four dependency groups, own-name and aliased, ./../ and trailing-slash normalization, a directory containing @, workspace:., directories from the root including a bare directory name and a range of a workspace next to it, a version bumped after the last install, no lockfile at all, aliases copied as written, and failures for a directory that exists but is not a workspace, a missing directory, a range on a non-workspace, a <name>@<range> whose name is not a workspace, and a workspace without a version) and test/cli/install/bun-publish.test.ts (publishes two workspaces to the test registry without a prior install and installs the dependent one from a fresh project). The 9 behavior-changing pack cases and the publish case fail on the base branch; with this change bun-pack.test.ts (96), bun-publish.test.ts (42), catalogs.test.ts, bun-add-filter.test.ts and bun-add-catalog.test.ts (the other ScratchManifests users) pass with the debug build, and bun_install / bun_runtime type-check for x86_64-pc-windows-msvc. The previous revision's CI run was green on every lane that ran (177 jobs, both Windows test lanes included).
  • Docs: added the directory form to the publishing section of docs/pm/workspaces.mdx.

Background

  • Workspace protocol: inside a monorepo, "dep": "workspace:*" tells the package manager to link the workspace package named dep instead of downloading it. The registry knows nothing about the workspace, so pack/publish must rewrite these specs into registry ones. Bun's installer also accepts a directory after workspace:; in that form the dependency name can be anything, which is why the rewrite sometimes needs an alias.
  • npm alias: "c7": "npm:@acme/core@1.2.3" installs the registry package @acme/core under node_modules/c7; it is the only registry spec that installs a package under a different name.
  • WorkspaceManifests (pack/publish: resolve workspace: and catalog: specs from the package.json files, not bun.lock #38813): the root package.json and the workspaces it lists, parsed by the code bun install uses into a throw-away lockfile. That parse records each workspace's name in workspace_paths and adds it to the root's dependencies as a Behavior::WORKSPACE entry whose value is the workspace's root-relative directory; those two are what the new lookups read, and the versions come from the same parse.
Repro
mkdir -p m/packages/core m/packages/ui && cd m
echo '{ "name": "mono", "private": true, "workspaces": ["packages/*"] }' > package.json
echo '{ "name": "@acme/core", "version": "1.2.3" }' > packages/core/package.json
echo '{ "name": "@acme/ui", "version": "2.0.0", "dependencies": { "c7": "workspace:../core", "@acme/core": "workspace:../core" } }' > packages/ui/package.json
bun install
cd packages/ui && bun pm pack && tar -xzOf acme-ui-2.0.0.tgz package/package.json

Before:

"c7": "../core",
"@acme/core": "../core"

After (same as pnpm pack):

"c7": "npm:@acme/core@1.2.3",
"@acme/core": "1.2.3"

Error paths with this change:

$ bun pm pack   # "x": "workspace:../nope"
error: Failed to resolve workspace version for "x" in `dependencies` ("/tmp/m/package.json" has no workspace named "x" and no workspace in the directory "../nope").
$ bun pm pack   # "nv": "workspace:../noversion", packages/noversion/package.json has no version
error: Failed to resolve workspace version for "nv" in `dependencies` (the package.json of workspace "noversion" in the directory "../noversion" has no version).
First revision (superseded)

The first revision classified specs syntactically (is_workspace_path: starts with . or contains a separator) and only resolved those. That missed directory forms the installer accepts, such as a bare directory name from the root ("bare": "workspace:core"), and duplicated part of the installer's grammar in pack; the current revision applies the installer's own rule through the manifests instead, and the heuristic is gone.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 7:05 AM PT - Aug 15th, 2026

@robobun, your commit 15e94df has some failures in Build #97783 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38835

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

bun-38835 --bun

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Reproduced on the current release and on the base branch (#38813) with the repro in the description: bun pm pack from packages/ui wrote "c7": "../core" and "@acme/core": "../core"; installing the published package fails with Could not find package.json for "file:../core" dependency "c7". With this branch the tarball carries "c7": "npm:@acme/core@1.2.3" and "@acme/core": "1.2.3", with or without a bun.lock, and using a version bumped after the last install.

Current revision (a1c6943): the 9 behavior-changing cases in bun-pack.test.ts and the round trip in bun-publish.test.ts fail on the base branch and pass with the debug build; the full bun-pack, bun-publish, catalogs, bun-add-filter and bun-add-catalog files pass with the debug build. The previous revision's CI was green on every lane that ran (177 jobs, both Windows test lanes included); the only jobs that did not run were the two darwin 14 aarch64 shards, which expired in the queue without an agent, as they did for the base branch's build.

@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. Because it changes what bun pm pack/bun publish write into published tarballs and registry manifests (including a new hard-fail path where it previously exited 0), a human look at the classification heuristic and the npm: alias behavior would still be worthwhile.

What was reviewed:

  • is_workspace_path classification — checked that ranges never start with . or contain //\, and that @scope/name@range is excluded via the leading-@ guard.
  • workspace_name_at_path — path resolution against top_level_dir, Windows separator normalization, and the behavior.is_workspace() filter over root dependencies.
  • Test coverage — all four dependency groups, own-name vs aliased, ./../ and trailing-slash normalization, workspace:., root-relative paths, no-lockfile, and both error paths (unlisted directory, no version).
Extended reasoning...

Overview

This PR makes bun pm pack and bun publish correctly rewrite workspace:<path> dependency specs (e.g. "c7": "workspace:../core") into registry-installable specs. Previously the protocol was stripped and the raw path ("../core") was written into the published package.json, which is uninstallable from a registry. The fix classifies path-like specs in Substitution::for_spec, resolves them through the new WorkspaceManifests::workspace_name_at_path, and emits either the workspace's version (when the dependency name matches) or an npm:<name>@<version> alias (when it differs), matching pnpm's behavior. Unresolvable paths now fail the pack instead of silently publishing a broken spec. Touches src/runtime/cli/pack_command.rs, src/install/PackageManager/workspace_manifests.rs, docs, and adds 6 tests across bun-pack.test.ts and bun-publish.test.ts.

Security risks

None identified. The path in workspace:<path> is resolved via join_abs_string_buf_checked and compared against the root's declared workspace directories; it is never opened or traversed on disk in this code path, so .. escapes only produce a "lists no workspace at" error. No untrusted network input is involved.

Level of scrutiny

Medium-high. This is a user-facing behavior change in the publish path — it determines what gets written into tarballs uploaded to registries, and it converts a previous silent-success (with broken output) into a hard failure for unresolvable paths. The is_workspace_path heuristic is new classification logic; misclassifying a valid range as a path (or vice versa) would break existing publishes. The Windows #[cfg] branch was type-checked but not exercised by the test suite. The npm: alias emission for name mismatches is a design choice worth a maintainer's eye.

Other factors

  • Stacked on #38813, which introduced WorkspaceManifests; this PR's diff is against that branch.
  • Test coverage is thorough: sibling and root-relative paths, all four dependency groups, aliased vs own-name, path normalization variants, workspace:., post-install version bump, no lockfile, and both new error paths. The end-to-end publish test round-trips through Verdaccio.
  • The PR description states all 6 new tests fail on the base branch and current release, and the full bun-pack/bun-publish suites pass with the change.
  • No prior human review comments on the PR.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

On the two points left for a human:

  • Classification: every spec is_workspace_path now classifies as a directory (./x, ../x, a/b, .) was previously published verbatim, and npm, pnpm and bun all read such a spec as a local directory, so none of those publishes produced an installable package (the new publish test shows the Could not find package.json for "file:../core" failure on the base branch). The new hard failure therefore only replaces an exit-0 publish of an uninstallable manifest. Specs that do not match keep the verbatim behavior, which the existing table tests (workspace:1.x, 1.1.x, -, and workspace:1.1.1 without a lockfile) still cover, and workspace:<name>@<range> is excluded so pack: publish workspace: aliases as npm: aliases #38788 can handle it.
  • npm: alias: bun install links the directory under whatever the dependency key is, so the registry spec that installs the same thing under the same key is npm:<name>@<version>; it is only used when the key differs from the workspace's name, otherwise the bare version is published. pnpm's publish rewrite makes the same choice for this input. The bun-publish.test.ts case round-trips it: both workspaces are published to the test registry and a fresh project installs the dependent one, ending up with the core package under both node_modules/core-alias and node_modules/publish-pkg-path-core.

…rsion

`bun install` reads the text after `workspace:` (other than `*`, `^`, `~`)
as a version range when the dependency is itself one of the workspaces, and
otherwise as the directory of the workspace to link, relative to the
package.json declaring it: `"c7": "workspace:../core"`. `bun pm pack` and
`bun publish` copied every such spec as written, so the published
package.json carried `"c7": "../core"`, which no registry client can install.

Pack now resolves these specs against the workspace manifests the way the
installer does. A spec naming the directory of a workspace is published as
that workspace's current version, behind an `npm:<name>@` alias when the
dependency is declared under another name (what pnpm publishes for it); a
range of a dependency that is a workspace, and the `<name>@<range>` alias
form, are still copied as written; anything else fails the pack instead of
publishing an uninstallable manifest.
@robobun
robobun force-pushed the farm/0750128c/pack-workspace-path branch from fb0c876 to b4b50b7 Compare August 15, 2026 09:53
Comment on lines +97 to +99
/// The root package's dependencies. The root parse turns each entry of its `workspaces` into a
/// `Behavior::WORKSPACE` dependency named after the workspace whose `workspace` value is its
/// directory relative to the root.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment on lines +122 to +124
/// Whether one of the workspaces (the root package is not one) is named `name`. This is what
/// decides, in `Package::parse_dependency`, whether the text after `workspace:` in a dependency
/// of that name is a version range or a directory.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment on lines +130 to +134
/// The name of the workspace whose directory is `path` taken relative to `package_dir`, the
/// directory of the package.json declaring `workspace:<path>`. `Package::parse_dependency`
/// links such a dependency by computing the same root-relative directory and matching it
/// against the workspaces, so this resolves exactly what `bun install` linked. `None` when no
/// workspace is in that directory.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment on lines +136 to +137
// `workspace:` with nothing after it is an empty range, not the declaring package's own
// directory.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment thread src/runtime/cli/pack_command.rs Outdated
Comment on lines +3201 to +3204
/// Anything else after `workspace:`. `bun install` reads it as a version range when the
/// dependency is itself one of the workspaces (`"pkg1": "workspace:1.x"`) and otherwise as the
/// directory of the workspace to link (`"c7": "workspace:../core"`), so which one it is takes
/// the manifests; see `publish_spec_for_workspace_range_or_directory`.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment thread src/runtime/cli/pack_command.rs Outdated
Comment on lines +3265 to +3271
/// What the tarball gets for `Substitution::WorkspaceRangeOrDirectory`, or why there is nothing to
/// publish for it. The directory reading is tried first because `"pkg1": "workspace:../pkg1"`
/// satisfies both readings and only the directory has a version to publish: that workspace's own,
/// as an `npm:` alias when the dependency is declared under another name (the registry's way of
/// installing a package under a different name, and what pnpm publishes for this spec). A range is
/// copied as written; so is `workspace:<name>@<range>`, which installs the workspace `<name>` under
/// the dependency's name (the installer, too, only reads it that way when `<name>` is a workspace).

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment thread src/runtime/cli/pack_command.rs Outdated
Comment on lines +3306 to +3307
/// `workspace_manifests` is `Some` whenever `needs_workspace_manifests(json.root)` is. `package_dir`
/// is the directory of `json`, which `workspace:<directory>` specs are relative to.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment on lines +126 to +127
/// The workspace in the directory `path`, resolved from `package_dir` (the directory of the
/// package.json declaring `workspace:<path>`) the way `Package::parse_dependency` links it.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment thread src/runtime/cli/pack_command.rs Outdated
Comment on lines +3201 to +3202
/// `"pkg1": "workspace:1.x"` (a range, the dependency being a workspace) or
/// `"c7": "workspace:../core"` (the directory of the workspace to link), as in `bun install`.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

Comment thread src/runtime/cli/pack_command.rs Outdated
Comment on lines +3263 to +3265
/// The published form of a `WorkspaceRangeOrDirectory` spec, or why it has none. A directory turns
/// into its workspace's version (as in pnpm) and is tried first: `"pkg1": "workspace:../pkg1"`
/// reads both ways, and only the directory reading has a version.

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

@robobun
robobun force-pushed the farm/0750128c/pack-workspace-path branch from a20d13f to a1c6943 Compare August 15, 2026 10:11
@robobun robobun changed the title pack: publish workspace:<path> dependencies as the workspace's version pack: publish workspace: directory dependencies as the workspace's version Aug 15, 2026
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Reworked after self-review (b4b50b7, a1c6943; description updated):

  • The syntactic is_workspace_path classifier is gone. Every workspace: spec other than */^/~ now goes through the manifests and gets the installer's own rule: a workspace in the named directory is published; otherwise a dependency that is itself a workspace keeps its range as written (as does <name>@<range> when <name> is a workspace); otherwise the pack fails, since bun install fails on the same spec. This also covers forms the first revision missed, such as a bare directory name from the root ("bare": "workspace:core") and a directory containing @ (workspace:../@scoped/thing), both now tested, and it makes the docs sentence accurate.
  • WorkspaceManifests gains has_workspace next to workspace_name_at_path; the rest of the lookup is unchanged.
  • Rebased onto the current head of pack/publish: resolve workspace: and catalog: specs from the package.json files, not bun.lock #38813.
  • The automated comment-length findings: the comments they pointed at were doc comments describing the two lookups and the ordering of the two readings, not workaround justifications, but they were longer than needed; every comment this PR adds is now a single line, with the one non-obvious decision (directory reading first) kept.

Verified the same way as before: the 9 behavior-changing pack cases plus the publish round trip fail on the base branch and pass here; bun-pack.test.ts (96), bun-publish.test.ts (42), catalogs.test.ts, bun-add-filter.test.ts and bun-add-catalog.test.ts pass with the debug build; bun_install/bun_runtime type-check for Windows.

test(`fails when a workspace: spec is neither a workspace's directory nor a workspace's range: ${spec}`, async () => {
await createDirectoryWorkspace({ dependencies: { "plain": "workspace:../plain" }, [group]: { [name]: spec } });
await write(join(packageDir, "unlisted", "package.json"), JSON.stringify({ name: "unlisted", version: "1.0.0" }));

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.

🟡 For the { group: "dependencies", name: "missing", ... } case, this object literal has two dependencies keys — the computed [group] overwrites the literal one, so "plain": "workspace:../plain" is silently dropped and the 'valid spec alongside invalid spec' coexistence is only exercised for the other three groups. Merge the two objects when group === "dependencies" (e.g. build dependencies first and spread the failing entry into it), or drop the plain entry so all four variants are symmetric.

Extended reasoning...

What the bug is

In the notAWorkspaceSpecs loop, each test builds the ui package's dependencies with:

await createDirectoryWorkspace({ dependencies: { "plain": "workspace:../plain" }, [group]: { [name]: spec } });

For the second entry in notAWorkspaceSpecs{ group: "dependencies", name: "missing", spec: "workspace:../missing" } — this expands to an object literal with two dependencies keys: the literal dependencies: { plain: ... } and the computed ["dependencies"]: { missing: ... }. In JavaScript, when an object literal has duplicate keys, the last one wins silently (no error, no merge). So the argument passed to createDirectoryWorkspace is { dependencies: { missing: "workspace:../missing" } } and the plain entry never reaches the ui package.json.

Step-by-step proof

  1. Loop iteration: group = "dependencies", name = "missing", spec = "workspace:../missing".
  2. Object literal evaluated left-to-right: first assigns .dependencies = { plain: "workspace:../plain" }, then the computed key ["dependencies"] reassigns .dependencies = { missing: "workspace:../missing" }.
  3. Result: { dependencies: { missing: "workspace:../missing" } }.
  4. createDirectoryWorkspace writes { name: "@acme/ui", version: "2.0.0", dependencies: { missing: "workspace:../missing" } } — no plain entry.
  5. packExpectError runs, sees the invalid missing spec, and fails with the expected message. The assertions on err and the tarball's non-existence still pass because they only reference name/group/spec, never plain.

Why existing code doesn't catch it

Nothing asserts on the plain entry in the error-path tests, so the drop is invisible. The test's core contract — that the invalid spec fails pack with the right error in the right group — is still validated for all four groups, which is why CI passes. What's lost is asymmetric: the intent of including plain is to prove that a valid workspace: directory spec sitting alongside the invalid one doesn't mask the failure (i.e., pack doesn't exit 0 just because one spec in the section resolves). That coexistence property is exercised for devDependencies, peerDependencies, and optionalDependencies (where dependencies: { plain } and [group]: { ... } are distinct keys), but silently skipped for dependencies.

Impact

Test-quality only — no false pass and no production behavior at stake. Per REVIEW.md's "every line you add must be demonstrably live", the plain entry is dead for 1 of 4 iterations, making the four cases quietly asymmetric.

How to fix

Merge instead of overwrite for the dependencies case, e.g.:

const deps = { plain: "workspace:../plain", ...(group === "dependencies" ? { [name]: spec } : {}) };
await createDirectoryWorkspace({ dependencies: deps, ...(group !== "dependencies" ? { [group]: { [name]: spec } } : {}) });

Or, if the coexistence property isn't intentional, drop the plain entry entirely so all four variants are symmetric and every line is live.

Comment on lines +121 to +124
pub fn has_workspace(&self, name: &[u8]) -> bool {
let name_hash: PackageNameHash = bun_semver::string::Builder::string_hash(name);
self.lockfile.workspace_paths.contains(&name_hash)
}

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.

🟡 has_workspace has a false-positive hole: workspace_paths is not just the glob-matched workspace names — Package::parse_dependency's features.is_main branch (Package.rs:2017) also inserts the root's own workspace:-tagged dependency keys into it. So when packing the root with "nope": "workspace:1.2.3" where nope is not a workspace, has_workspace("nope") returns true (self-inserted while parsing this very dependency) and pack copies 1.2.3 verbatim instead of failing. Not a regression (pre-PR copied verbatim too) and only bites when packing the root itself, but the doc comment is inaccurate and the new error path has a gap; iterating root_dependencies for a Behavior::WORKSPACE entry with the given name (mirroring workspace_name_at_path) would be robust.

Extended reasoning...

What the bug is

has_workspace(name) at workspace_manifests.rs:121-124 is documented as "Whether name is one of the workspaces" and is implemented as self.lockfile.workspace_paths.contains(&name_hash). The assumption is that workspace_paths contains exactly the names matched by the root's workspaces glob. That assumption is wrong: workspace_paths also contains the dependency keys of every workspace:-tagged entry in the root's own dependencies / devDependencies / peerDependencies / optionalDependencies, whether or not those keys name a real workspace.

The code path that triggers it

ScratchManifests::parse_root calls Package::parse_with_json with Features::main() (resolver_hooks.rs:1252-1261: is_main=true, dependencies=true, workspaces=true). parse_with_json_impl (Package.rs:2271-2286) processes the WORKSPACES group first, then DEPENDENCIES etc. When it reaches a root entry like "nope": "workspace:1.2.3":

  1. At Package.rs:1834, workspace_paths.get(&hash("nope")) is None (the workspaces glob just ran and nope is not one of them), so workspace_path is None.
  2. Control falls into the else if features.is_main || features.is_workspace branch (Package.rs:1959).
  3. Line 2017 executes workspace_paths.get_or_put(name_hash), which inserts hash("nope") into the map. workspace_version is None and found_existing is false, so no error is logged; line 2069 sets the entry's value.
  4. fail_on_logged_errors() passes, and WorkspaceManifests is built with the polluted workspace_paths.

Now publish_spec_for_workspace_range_or_directory (pack_command.rs:3268-3280) processes the same "nope": "workspace:1.2.3" while packing the root:

  • workspace_name_at_path(root_dir, "1.2.3") returns None — the nope entry in root_dependencies has the DEPENDENCIES-group behavior, not Behavior::WORKSPACE, so the .is_workspace() filter excludes it, and no real workspace lives at <root>/1.2.3.
  • is_alias is false (no @ after index 0).
  • manifests.has_workspace("nope") returns true (the self-inserted entry), so the function returns Ok("1.2.3") verbatim instead of the intended Err("…has no workspace named \"nope\"…").

Why existing code doesn't prevent it

The installer's own check at Package.rs:1834 runs before the self-insertion at line 2017, so bun install correctly sees nope as not-a-workspace and later fails resolution. But pack's has_workspace runs after the whole root parse, when the self-insertion has already happened — a temporal mismatch. workspace_name_at_path is unaffected because it filters on Behavior::WORKSPACE, which the self-inserted entry lacks; only the workspace_paths.contains check is fooled. The notAWorkspaceSpecs tests don't catch this because they pack pkgs/ui (a member), and parse_root never processes a member's dependencies with is_main=true.

Step-by-step repro

root/package.json:
  { "name": "root", "version": "1.0.0", "workspaces": ["pkgs/*"],
    "dependencies": { "nope": "workspace:1.2.3" } }
pkgs/pkg1/package.json:
  { "name": "pkg1", "version": "1.0.0" }

$ bun pm pack   # from root
  • parse_root: WORKSPACES pass inserts hash("pkg1"); DEPENDENCIES pass hits Package.rs:2017 and inserts hash("nope"). No error logged.
  • edit_root_package_jsonWorkspaceRangeOrDirectory("1.2.3")publish_spec_for_workspace_range_or_directory:
    • workspace_name_at_path(root, "1.2.3")None
    • is_aliasfalse
    • has_workspace("nope")true (false positive)
    • returns Ok(b"1.2.3")
  • Tarball ships "nope": "1.2.3" and pack exits 0. bun install on this same root would have failed, so the PR's stated invariant ("pack fails when bun install would fail on the same spec") is violated for this case.

Impact

Low, hence nit: (a) not a regression — pre-PR WorkspaceRange also copied 1.2.3 verbatim, so the published output is byte-identical to before; (b) only reachable when packing the root package (roots are usually private and rarely packed); (c) the input configuration already fails bun install, so nobody with a green install hits it. The concrete defects are that the new error detection has a hole for exactly the case the notAWorkspaceSpecs tests target (just at the root instead of a member), and the doc comment on has_workspace is inaccurate.

How to fix

Mirror workspace_name_at_path: iterate self.root_dependencies for an entry whose behavior.is_workspace() and whose name matches, instead of consulting workspace_paths:

pub fn has_workspace(&self, name: &[u8]) -> bool {
    let string_buf = self.lockfile.buffers.string_bytes.as_slice();
    self.root_dependencies
        .get(self.lockfile.buffers.dependencies.as_slice())
        .iter()
        .any(|d| d.behavior.is_workspace() && d.name.slice(string_buf) == name)
}

That reads the same Behavior::WORKSPACE entries workspace_name_at_path already trusts, so both lookups agree on what counts as a workspace. Alternatively, workspace_versions (populated only from the glob walk) would work as the membership set.

Comment on lines +3262 to +3282
/// Directory first: `"pkg1": "workspace:../pkg1"` reads both ways; only a directory has a version.
fn publish_spec_for_workspace_range_or_directory(
manifests: &WorkspaceManifests,
package_dir: &[u8],
dependency_name: &[u8],
spec: &[u8],
) -> Result<Vec<u8>, String> {
let Some(workspace_name) = manifests.workspace_name_at_path(package_dir, spec) else {
// `workspace:<name>@<range>` installs workspace `<name>` under `dependency_name`.
let is_alias = strings::last_index_of_char(spec, b'@')
.is_some_and(|at| at > 0 && manifests.has_workspace(&spec[..at]));
if manifests.has_workspace(dependency_name) || is_alias {
return Ok(spec.to_vec());
}
return Err(format!(
"\"{}\" has no workspace named \"{}\" and no workspace in the directory \"{}\"",
bstr::BStr::new(manifests.root_package_json_path()),
bstr::BStr::new(dependency_name),
bstr::BStr::new(spec),
));
};

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.

🟡 The directory-first ordering here diverges from the installer's precedence: Package::parse_dependency checks workspace_paths for the dependency key first and only falls through to the directory reading when the key is not a workspace name. So for { "pkg-a": "workspace:b" } where both pkg-a and pkg-b are workspaces (at directories a and b), install links pkg-a but pack now publishes npm:pkg-b@… — a different package. Contrived, and pre-PR was also wrong (published b verbatim), so not a regression; but consider only preferring the directory reading when !has_workspace(dependency_name) or when both readings resolve to the same workspace.

Extended reasoning...

What the bug is

publish_spec_for_workspace_range_or_directory tries the directory reading first (per the comment: "Directory first: 'pkg1': 'workspace:../pkg1' reads both ways; only a directory has a version"), then falls back to the range reading only when no workspace lives in that directory. The installer does the opposite: Package::parse_dependency (src/install/lockfile/Package.rs:1799-1958) computes name_hash from the dependency key, looks that up in workspace_paths first, and only treats the spec as a directory when the dependency key is not a workspace name. When both readings apply and point at different workspaces, install and pack disagree on which package the dependency is.

Concrete trigger

Root package.json:

{ "workspaces": ["a", "b"], "dependencies": { "pkg-a": "workspace:b" } }

with a/package.json = { "name": "pkg-a", "version": "1.0.0" } and b/package.json = { "name": "pkg-b", "version": "2.0.0" }.

Installer path (Package.rs:1822 → 1834 → 1938-1941): name_hash = hash("pkg-a") is in workspace_paths, so workspace_path = "a" and the spec b is parsed as a range. range.satisfies fails, but SemverVersion::is_tagged_version_only("b") is true (single alphabetic byte, Version.rs:392-407), so dependency_version.value.workspace = "a" — install links pkg-a.

Pack path (this function): workspace_name_at_path(root, "b") finds the workspace at directory b, whose name is pkg-b. Since workspace_name ("pkg-b") != dependency_name ("pkg-a"), it returns npm:pkg-b@2.0.0. The published tarball would install pkg-b under node_modules/pkg-a — the opposite package from what bun install linked locally.

Why the existing code doesn't prevent it

The directory-first ordering was chosen so that "pkg1": "workspace:../pkg1" (which reads both ways and resolves to the same workspace) publishes a version instead of the verbatim path. That's correct when both readings agree, but nothing checks that they agree. When the dependency key names one workspace and the bare spec happens to be another workspace's root-relative directory, the two readings pick different workspaces, and the directory reading silently wins over the reading the installer would use.

Impact

Low. The trigger requires (1) the dependency key to be a workspace's package name, and (2) the spec after workspace: to be a bare alphanumeric string (so is_tagged_version_only accepts it as a range) that also happens to be a different workspace's root-relative directory. Before this PR the same input published "b" verbatim — an uninstallable dist-tag on pkg-a — so this is not a regression from working behavior; it swaps one wrong answer for a differently-wrong one. It does, however, contradict the PR's stated invariant that "pack publishes what install linked."

How to fix

The naive fix — check has_workspace(dependency_name) before workspace_name_at_path — would break this PR's own "@acme/core": "workspace:core" test (both readings resolve to @acme/core, but returning early would publish the range core verbatim). A safer change is to keep the directory lookup first, but when it finds a workspace whose name differs from dependency_name and has_workspace(dependency_name) is also true, defer to the installer's precedence — i.e. treat it as a range on the workspace named by the dependency key (or publish that workspace's version). Equivalently: only let the directory reading override the range reading when both readings resolve to the same workspace or when the range reading doesn't apply.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant