Repository navigation
Conversation
…ot a terminal `bun pm pack --destination` and `--filename` print the path of the tarball. The printer formatted that path through `bstr::BStr`, so each byte that is not UTF-8 became U+FFFD. The line then named a file that does not exist. The printer now takes the path as bytes. A path that is valid UTF-8 goes through the same statement as before. So does every path when stdout is a terminal. For any other path, each name that is valid UTF-8 goes through `EscapeControlChars`. In each other name, the ASCII bytes go through `EscapeControlChars` and the other bytes go to stdout unchanged.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Tarball path formatting src/runtime/cli/pack_command.rs |
print_tarball_path now accepts TarballNameFormatter. The new print_tarball_abs_path formats absolute paths based on UTF-8 validity and whether output is a terminal. |
Pack output integration src/runtime/cli/pack_command.rs, test/cli/install/bun-pack.test.ts |
Dry-run and completed-pack output use print_tarball_abs_path. Tests cover byte-containing paths, control characters, logging modes, and terminal versus piped output. |
Suggested reviewers: jarred-sumner
Priority:
Merge Risk: 
66ccf
-
The pack output change appears ready to merge after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and specifically describes the main change: preserving non-UTF-8 tarball path bytes when stdout is not a terminal. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description follows the repository template. It has both required sections, "What does this PR do?" (as "Problem", "Fix", and "Background") and "How did you verify your code works?" (as "Verified"… Full details: Description check
-
Explanation
The description follows the repository template. It has both required sections, "What does this PR do?" (as "Problem", "Fix", and "Background") and "How did you verify your code works?" (as "Verified" and "Tests"). It gives a concrete reproduction, a measured verification plan, and the downsides. Its verification covers Linux only. The macOS runs were not done locally, and CI on macOS aarch64 is cited. The reviewer should check the macOS evidence, which the bot flagged as platform-specific test evidence deferred to CI. The description is detailed and specific, so it does not fall into the vague category.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Updated 9:38 AM PT - Oct 26th, 2026
✅ @robobun, your commit 66ccf4da44eed15dc72a322cf29e9bbef4c03cc7 passed in 🧪 To try this PR locally: bunx bun-pr 46987That installs a local version of the PR into your bun-46987 --bun |
|
Status: ready for review. In CI build 133128 every lane that ran is green: Linux x64 and aarch64 (Debian, Ubuntu, Alpine), the ASAN lane, Windows x64 and aarch64, and macOS aarch64. The two
How it was reproduced (Linux x64, on 1.4.3-canary 367d939 and on a debug build of main 449877f): mkdir /tmp/pk && cd /tmp/pk && echo '{"name":"p","version":"1.0.0"}' > package.json
t="$(bun pm pack --quiet --destination "$(printf 'dest-\377')")"; test -f "$t" || echo MISSINGBoth builds print |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked that the mixed pretty! / Output::print_bytes writes in the non-TTY arm cannot reorder — both go through the same Destination::Stdout writer — and that slice::chunk_by is not caught by clippy.toml's disallowed-methods list or test/internal/source-lints/byte-search.test.ts, so the new arm passes the byte-search lint.
Extended reasoning...
The change touches only src/runtime/cli/pack_command.rs (the tarball-path printer and its two callers) and the matching test file; it writes raw non-UTF-8 bytes to stdout when stdout is not a terminal, which is output-escaping territory but not auth, crypto or injection. Two inline findings remain (missing real-pack --filename non-UTF-8 coverage and the unchanged behaviour for valid-UTF-8 names holding control characters), so a human look is warranted.
The `--filename` route had such a name only with `--dry-run`. This test packs for real and runs `test -f` on the captured line.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I re-reviewed after the latest pushes and found no bugs; the new real-pack --filename test covers the second call site I asked about earlier. Because the PR settles an output-policy question (a non-UTF-8 path prints raw bytes in a pipe, while a valid-UTF-8 path with control characters is still escaped there, and a terminal keeps U+FFFD), a human look at that rule is still worthwhile before merging.
What was reviewed:
print_tarball_abs_path: the split onSEP_STRis lossless (empty leading/duplicate fields are re-joined with the separator),chunk_byruns andescape_control_charson ASCII runs leave every non-ASCII byte unchanged, andpretty!/print_bytesshare the same stdout writer so ordering holds.- Both callers (dry-run at :1884, real pack at :2162) pass the same
abs_tarball_dest;--filenameis resolved againstoriginal_cwdfirst so the printed line is absolute. - Tests:
test -f "$tarball"assertions on the captured line fail on main for every matrix entry; the pty test mirrors the existingbun pm diffterminal test and drains stdout/stderr concurrently.
Extended reasoning...
The change touches one function family in src/runtime/cli/pack_command.rs (about 40 lines) and adds 13 subprocess tests in test/cli/install/bun-pack.test.ts; no CODEOWNERS entry covers either file. The only security-relevant surface is terminal escape handling, and the code keeps the existing escaping whenever stdout is a TTY or the path is valid UTF-8, so no new control-sequence injection route reaches a terminal. The code is small and I traced it as correct, but the PR introduces a behavioral rule (raw bytes only for non-UTF-8 names in a pipe) that the author themselves marks as an open question, which is a product decision rather than a correctness check. My prior inline comment about the untested real --filename route was addressed by commit 6c2c438; the prior pre-existing note was explicitly kept as-is by design.
Problem
bun pm pack --destinationand--filenameprint U+FFFD for each byte of the tarball path that is not UTF-8.bun pm pack --quiet --destination dest-\xFFwritesdest-\xFF/p-1.0.0.tgzand printsdest-\xEF\xBF\xBD/p-1.0.0.tgz, which does not exist.bstr::BStrtoprint_tarball_path(impl fmt::Display)(src/runtime/cli/pack_command.rs:109,:1851,:2132). 1.3.14 printed the bytes. No user reported it (fuzzing).Fix
print_tarball_abs_path(&[u8]). Valid UTF-8 paths, and all paths on a terminal, run the same statement as before.EscapeControlChars.test/cli/install/bun-pack.test.ts(14 new tests fail on main) andescape-control-characters.test.ts. Self-reviewed: 23 concerns raised, 23 addressed (Notes).Background
bstr::BStrlets{}take&[u8]. ItsDisplaywrites U+FFFD for invalid bytes: right for a message, wrong for a path.EscapeControlChars(install: fix regressions and older bugs found after the package manager consolidation #45307) spells out control and bidirectional characters (ESC prints as\x1b), so a name cannot alter terminal output. That PR madebun pm diffwrite raw bytes only off a terminal. This PR uses that rule.C2 82reads as U+0082.Downsides
.textgrows 1,280 bytes.Notes
Provenance and reach. Automated fuzzing of paths that are not UTF-8 found this. No issue exists, and no user reported it in 1.4.0 to 1.4.4. It needs
--destinationor--filename, and a byte that is not UTF-8 in the working directory or in the flag value (a Latin-1, Shift-JIS or GBK name). Linux accepts such names. The documentedTARBALL=$(bun pm pack --quiet)prints the bare name and was never affected.History. 1.3.14 printed the bytes of the path, but its quiet line began with a newline (#32751 removed that in 1.4.0). So
test -f "$(bun pm pack --quiet --destination dir)"passed on no release for such a path. What 1.4.0 lost is the bytes of the line: in 1.3.14| tail -n 1gave the path.Repro (Linux x64).
The code.
print_tarball_pathnow takes only aTarballNameFormatter, so aBStrno longer compiles there. The two path callers (--dry-runand real) passabs_tarball_dest.as_bytes()toprint_tarball_abs_path. Both entries share the leading newline of the default log level throughbegin_tarball_path.The rule.
Name by name: a name that is valid UTF-8 goes through
EscapeControlCharsas on main. In any other name, the ASCII bytes go throughEscapeControlChars(so ESC, CR and LF are still spelled out and the line stays one line) and every other byte is printed unchanged. The name of the tarball comes frompackage.jsonand is always UTF-8, so it is always text.Why name by name. The first version of this branch split the path with
utf8_chunks()and sent each valid chunk through the escaper. That is wrong for a multi-byte legacy encoding. In Shift-JIS つくる is82 C2 82 AD 82 E9. The second byte of つ and the first byte of く areC2 82, which is U+0082 in UTF-8, so the line was82 5C 75 30 30 38 32 AD 82 E9and named no file. GBK 聯合國 (C2 93 BA CF 87 F8) failed in the same way. A name is what the kernel stores, and one program wrote it in one encoding. So a name is text or it is bytes.Why a terminal keeps U+FFFD.
vtparse/src/transitions.rs,anywhere_or, at e2a98e1b). This is read from the source, not run in a WezTerm window.bun pm diffwrites raw bytes only when stdout is not a terminal (src/runtime/cli/pm_diff_command.rs:1722-1730, from install: fix regressions and older bugs found after the package manager consolidation #45307).sanitize_for_terminalblanks every byte above 0x7F of text that is not UTF-8 (src/runtime/bake/dev_server/error_report_request.rs:525-538).None if Output::is_stdout_tty()arm. Nothing else changes.Open question, not blocking. When stdout is not a terminal, is the
--quietline a message or the result? Since #45307 a control character in a path that is valid UTF-8 is spelled out in a pipe too, so that line does not name the file. This PR keeps that. The valid UTF-8 half of the new test "control characters are still spelled out" records the behaviour of main, not a new rule. If the answer is "the result", the last arm becomesOutput::print_bytes(path).Still failing, as before.
bun add "$tarball"with such a path. Forcaf\xE9it exits 1 (invalid dependency name). Fordest-\xFFit exits 0 and writes U+FFFD intobun.lock.prepackorpreparescript that runs JavaScript in a working directory that is not UTF-8:error: Module not found, before the line is printed.Not changed.
TarballNameFormatterbuilds the file name and the line through the sameDisplay, so they agree.packed,bundledandignorelines and the error messages. They are text for a person, and U+FFFD stays.bun publishprints no tarball line.bun pm binandBun.cron(pm bin, Bun.cron: append path bytes instead of formatting them #46504),run --filter,build --outfileand the shellwhich(Append non-UTF-8 bytes instead of formatting them: run --filter output, build names, CSS module hash, shell which #46157), completions (completions: build the bunx link and .zshrc paths from raw bytes #46423).Measurements (Linux x64, merge base 449877f against this PR).
od,test -f).--destination,--filename} x {real,--dry-run} x {--quiet, default} x names with {ASCII, ESC[2K, CR, BEL, U+009B, U+202E}).E0 80 9B, ESC, default level, ASCII). Lone 0x80 to 0x9F bytes on the terminal: 0 -> 0. In a pipe: 0 -> 1.--quiet: allocator calls 0 -> 0, entries into the stdout writer 1 -> 1,write_allcalls 2 -> 2, UTF-8 passes 2 -> 2 (one moved fromUtf8Chunksto simdutf), syscalls 0 -> 0. Default level: 2 -> 2 entries and 3 -> 3write_allcalls. Bare name: all counts equal.write_allcalls 4 -> 12, syscalls 0 -> 0.write(1)calls per process (stdout as a datagram socket): 1 -> 1 with--quiet, 3 -> 3 at the default level..text: 82,164,617 -> 82,165,897 bytes (llvm-size -A)..rodataand.dataare equal. The printer symbols: 274 -> 1,510 bytes (nm -S).strace,perf,valgrindandbloatyare not installed here.Tests (14 new, in
test/cli/install/bun-pack.test.ts).test -fon the captured line. One more packs from a working directory that holds the byte, and one packs to a--filenamethat holds it.--dry-run:--filename,--silentand the default level, control characters, and a terminal against a pipe.test/cli/install/escape-control-characters.test.ts: 66 pass. The pty test ofbun pm diffpasses.Self-review. 23 concerns raised, 23 addressed.
test -fon each row, the pty test and the macOS runs.Related.
bun pm diff.bunxpaths.elseto theifthat is now inbegin_tarball_path. Those four lines are unchanged here.pack_command.rs, not these lines.no test proof · iteration 0 · 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