publish: resolve a relative tarball path against the invoking directory - #38704
publish: resolve a relative tarball path against the invoking directory#38704robobun wants to merge 3 commits into
Conversation
`bun publish ./pkg.tgz` joined the argument onto the package root that PackageManager::init had already chdir'd to. Run from a subdirectory of a project, or from a workspace member, that is not the directory the user is in: the tarball is either not found, or a same-named tarball sitting in the root is published instead. Resolve it against the directory the command was run from, which init already returns.
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review. Reproduced on the released bun (1.4.0-canary.1) from a CI: every Linux and Windows lane is green on both a02fd43 (build 96933) and ceea652 (build 97133); the publish test file passes on Windows x64 and aarch64. The only jobs not finished are the two |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, targeted fix that swaps the base directory for the tarball path from top_level_dir to the original_cwd that PackageManager::init already returns.
What was reviewed:
- Confirmed the pattern matches
add_remove_with_filter.rs:405(join_abs_string_bufonoriginal_cwdfor user-supplied paths). - Checked
join_abs_string_buf_zignores the base for absolute args, so the existing absolute-path test in "can publish from a tarball" still holds; when cwd == package root,original_cwd == top_level_dirso nothing changes. - Verified the removed
FileSystemimport has no remaining uses in the file. - Tests use
--dry-runwith auth-in-URL (same trick as the existing "registry summary line" test),tempDir, andtest.concurrent; the first test asserts the stale root tarball does not win, not just that the right one is found.
Extended reasoning...
Overview
Two files touched. In src/runtime/cli/publish_command.rs, Context::from_tarball_path gains an original_cwd: &[u8] parameter and joins the CLI-supplied tarball path onto it instead of FileSystem::instance().top_level_dir. PublishCommand::exec stops dropping the second tuple element from PackageManager::init and passes it through. The now-unused bun_resolver::fs::FileSystem import is removed. In test/cli/install/bun-publish.test.ts, a three-test describe("relative tarball path") block is added.
Security risks
None. This changes which base directory a user-supplied CLI argument is resolved against, from the discovered package root to the directory the user was standing in — which is the less surprising and less dangerous of the two (it stops silently publishing a stale tarball that happens to sit at the root). No new path-traversal surface: the resulting absolute path is fed straight to File::read_from, same as before.
Level of scrutiny
Low-to-medium. The Rust change is ~10 lines and mechanically follows an existing pattern (add_remove_with_filter.rs already resolves bun add ./dir against original_cwd the same way). join_abs_string_buf_z treats absolute parts as absolute, so the existing absolute-path test path is unaffected. The original_cwd is a Box<[u8]> that now lives to the end of exec instead of being dropped one line after creation — no lifetime or ownership concern.
Other factors
The three new tests are well-shaped: tempDir + using, test.concurrent, --dry-run with a bogus npm_config_registry carrying userinfo so the auth check passes without network I/O (same technique already used by the "registry summary line" test in this file). The first test plants a same-named stale tarball at the root and asserts it is not the one selected — covering the silent-wrong-publish variant, not just ENOENT. The PR description confirms all three fail under USE_SYSTEM_BUN=1 and the whole file passes on the branch.
|
Nothing to change from the automated review. Lints, format and clippy are green; Buildkite is still running on dd059c8. |
|
Updated 11:05 PM PT - Aug 14th, 2026
❌ @robobun, your commit ceea652 has some failures in 🧪 To try this PR locally: bunx bun-pr 38704That installs a local version of the PR into your bun-38704 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, correct fix that swaps the tarball path's join base from top_level_dir to the original_cwd that PackageManager::init already returns for exactly this purpose.
What was reviewed:
- Confirmed
PackageManager::initreturns(manager, Box<[u8]>)and the sameoriginal_cwdjoin pattern is already used inadd_remove_with_filter.rs:405andprune.rs. join_abs_string_buf_zignores the base for absolute args, so absolute tarball paths are unaffected; the removedFileSystemimport has no other uses in this file.- Three new
--dry-runtests cover the variant matrix (stale root tarball, project subdir, workspace member), usetempDir/test.concurrent, and were verified to fail on the released binary. - The comment-cop flag on line 139 is a false positive: the two-line doc comment documents a parameter (why
original_cwddiffers from the process cwd), not a workaround.
Extended reasoning...
Overview
This PR fixes bun publish ./path/to/tarball.tgz resolving relative tarball paths against the wrong directory. PackageManager::init walks up to the package/workspace root and chdirs there before returning; it hands the pre-walk cwd back as the second tuple element precisely so callers can resolve user-typed relative paths against it. PublishCommand::exec was explicitly drop()ing that value and from_tarball_path was joining onto FileSystem::instance().top_level_dir (the post-walk root) instead. The fix threads original_cwd through as a &[u8] parameter and joins onto it — a ~7-line Rust change plus a now-unused import removal.
Security risks
None. This only changes which base directory a user-supplied CLI path is joined onto; join_abs_string_buf_z already handles absolute paths by ignoring the base, and the resulting path is opened read-only for a file the user explicitly named. No new untrusted input, no auth/crypto/permissions surface touched.
Level of scrutiny
Low-to-medium. The change is mechanical and follows an established repo pattern: add_remove_with_filter.rs:405 does the identical join_abs_string_buf::<platform::Auto>(original_cwd, ...) for bun add ./dir, and prune.rs / workspace_selection thread the same value the same way. The only caller of from_tarball_path is updated in the same diff. The removed drop(original_cwd) was purely cosmetic (an owned Box<[u8]> that now lives to the end of scope instead).
Other factors
- Tests are solid per REVIEW.md: they cover the three failure modes described (stale same-named tarball at the root wins silently, ENOENT from a subdir, ENOENT from a workspace member), assert on the specific package name/version in stdout rather than just exit code, use
--dry-runwith a bogus auth-in-URL registry so nothing hits the network, and usetempDir/test.concurrentper harness conventions. The PR description confirms all three fail withUSE_SYSTEM_BUN=1and pass withbun bd test, and that the full 42-test file passes. - The github-actions comment-cop inline comment ("if you need a paragraph-long comment to justify why the workaround is OK, the code is wrong") fired on the new two-line doc comment for the
original_cwdparameter. That comment documents genuinely non-obvious state (the process has alreadychdir'd away from the invoking directory) rather than justifying a workaround — the fix is the correct fix, not a workaround. I'm treating it as a false positive of the heuristic and not a blocker.
There was a problem hiding this comment.
LGTM — small, well-scoped fix that follows the existing original_cwd pattern.
What was reviewed:
- Confirmed
PackageManager::initreturns the pre-walk cwd as its second tuple element and thatadd_remove_with_filter.rs/pm_version_command.rsalready resolve user-supplied relative paths against it the same way;from_tarball_pathhas no other callers. - Absolute tarball paths are unaffected (
join_abs_string_buf_zignores the base); the removedFileSystemimport anddrop(original_cwd)are the only collateral changes. - Tests are hermetic (
--dry-run+ bogus in-URL auth, no registry contact), usetempDir/test.concurrent, and the first one asserts the stale root tarball is not picked — the silent-wrong-publish case. - The comment-cop note about a paragraph-long doc comment was addressed in ceea652.
Extended reasoning...
Overview
The PR changes Context::from_tarball_path in src/runtime/cli/publish_command.rs to accept an original_cwd: &[u8] parameter and join the user-supplied tarball path onto that instead of FileSystem::instance().top_level_dir. PublishCommand::exec was already receiving original_cwd from PackageManager::init and immediately dropping it; it now passes it through. The unused FileSystem import is removed. Three new tests in test/cli/install/bun-publish.test.ts cover: (1) a same-named stale tarball in the package root must not win over the one in the cwd, (2) publishing from a subdirectory of the package root, (3) publishing from a workspace member directory.
Security risks
None. This is CLI path resolution for a local-file argument the user typed; the fix makes it resolve against the directory they typed it in, which is the less-surprising and less-dangerous behavior (the bug could silently publish a stale tarball from a parent directory). No network, auth, or untrusted-input parsing is touched.
Level of scrutiny
Low. The diff is ~10 production lines and follows an established repo pattern verbatim: add_remove_with_filter.rs:405 already does join_abs_string_buf::<platform::Auto>(original_cwd, …) for bun add ./dir, and pm_version_command.rs, dedupe_command.rs, outdated_command.rs all thread original_cwd from PackageManager::init for the same reason. from_tarball_path has exactly one call site (grep-verified), so no sibling to miss. Absolute paths are unaffected because join_abs_string_buf_z ignores the base when the joined segment is absolute — the existing "can publish from a tarball" test already exercises that.
Other factors
The tests are well-constructed per the repo's review guidelines: tempDir + using, test.concurrent, --dry-run with a bogus npm_config_registry carrying in-URL credentials so the auth gate passes without any network I/O, exit code asserted last, and a negative assertion (not.toContain("publish-tarball-cwd-stale")) that catches the silent-wrong-tarball failure mode rather than just ENOENT. The PR description states all three fail under USE_SYSTEM_BUN=1 and pass with bun bd, and the whole file passes. The github-actions comment-cop flag on a paragraph-long parameter doc comment was addressed in the follow-up commit ceea652, so there are no outstanding review comments.
|
Heads up: #38739 is the pack side of this same problem ( Today, from a workspace package, |
2 participants
Problem
bun publish ./pkg.tgzresolves the tarball path against the wrong directory whenever the package root is not the directory the command was run from:cd packages/foo && bun pm pack && bun publish ./foo-1.2.3.tgzfails withENOENT: No such file or directory: failed to read tarball: './foo-1.2.3.tgz' (open)even though the file is right there.<repo>/dist, the usual build-then-publish layout): same ENOENT, or, if a same-named tarball happens to sit in<repo>/, that stale tarball is published instead, silently. An absolute path works.Context::from_tarball_path(src/runtime/cli/publish_command.rs:144) joins the argument ontoFileSystem::instance().top_level_dir. By thenPackageManager::inithas walked up to the nearest package.json (or the workspace root), settop_level_dirto it andchdir'd there.initreturns the directory the command was actually run from as its second tuple element, andPublishCommand::execwas dropping it on the next line.Fix
from_tarball_pathtakesoriginal_cwdand joins the argument onto that instead oftop_level_dir;execpasses the valuePackageManager::initreturns. Absolute arguments are unaffected (join_abs_string_buf_zignores the base), and when the cwd is the package root nothing changes.add_remove_with_filter.rsjoinsbun add ./dirontooriginal_cwd).bunfig.toml/.npmrc(registry, token), which the existing tarball publish tests depend on.test/cli/install/bun-publish.test.ts, newrelative tarball pathdescribe block (three tests: stale root tarball must not win, subdirectory of a project, workspace member). All three fail on the released bun (USE_SYSTEM_BUN=1: first one publishespublish-tarball-cwd-stale@0.0.1, the other two hit ENOENT) and pass withbun bd test; the whole file passes (42 tests).Background
PackageManager::initis shared by every install subcommand. It starts from the process cwd, walks up to the nearestpackage.json, and if that package is a member of a workspace continues up to the workspace root; it thenchdirs into that directory and makes ittop_level_dir, so the rest of the package manager can work with root-relative paths. It hands the pre-walk cwd back to the caller (original_cwd) precisely for command-line arguments that the user typed relative to where they were standing.bun publish <tarball>is the two-step flow documented indocs/pm/cli/publish.mdx(bun pm pack, thenbun publish ./package.tgz): it reads an existing tarball, takes name/version from thepackage.jsoninside it, and uploads it; nothing about it needs the project root except configuration.