Repository navigation
Conversation
Archive creation hardcoded 0o644 for every entry, so there was no way to
mark a file executable inside a created tarball. Entry values may now be a
{ data, mode } descriptor in addition to the existing string / Blob /
TypedArray / ArrayBuffer forms:
new Bun.Archive({
"bin/mytool": { data: bytes, mode: 0o755 },
"README.md": "hello",
});
The mode is masked to 0o777 (matching what the extraction path preserves)
and defaults to 0o644 when the descriptor omits it. Blob/ArrayBuffer/
TypedArray are matched before the descriptor branch since they are objects
too. Both new Bun.Archive({...}) and Bun.Archive.write(path, {...}) share
the same build path, so both gain the feature.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR adds per-entry Unix file mode support to Bun.Archive when creating archives. Object entries can now use a ChangesPer-entry Archive Mode Support
Related Issues: Suggested labels: Suggested reviewers: none 🐰 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/runtime/archive.mdx (1)
79-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting invalid-input rejection.
The Rust implementation throws
TypeErrorfor a non-numeric/negative/non-integermodeor a descriptor missingdata, and this is covered by tests. The docs only cover the happy path (default, override, masking) — a brief note on validation would round out the section.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/runtime/archive.mdx` around lines 79 - 89, The Bun.Archive docs section only shows valid `{ data, mode }` usage; add a brief validation note to document that entry descriptors must include data and that mode must be a non-negative integer. Mention the behavior in the Bun.Archive constructor example area so readers know invalid descriptors are rejected with TypeError, including non-numeric, negative, or non-integer mode values. Keep the note close to the existing mode masking explanation to cover both accepted and rejected inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/runtime/archive.mdx`:
- Around line 79-89: The Bun.Archive docs section only shows valid `{ data, mode
}` usage; add a brief validation note to document that entry descriptors must
include data and that mode must be a non-negative integer. Mention the behavior
in the Bun.Archive constructor example area so readers know invalid descriptors
are rejected with TypeError, including non-numeric, negative, or non-integer
mode values. Keep the note close to the existing mode masking explanation to
cover both accepted and rejected inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3f68efeb-25e3-40a3-af6e-fa22d168ce11
📒 Files selected for processing (1)
docs/runtime/archive.mdx
|
Addressed the docs suggestion in 8e6ab1c: the "Setting file modes" section now notes that a descriptor must include |
|
Status for a maintainer: the diff is green and ready. The change now uses Every red CI lane so far has been an unrelated flaky test, different ones each run and none touching the changed files:
No archive-related failures appear in any build, and the changed files are only |
There was a problem hiding this comment.
Implementation and tests look solid and my earlier docs note was addressed — deferring only because this adds new public Bun.Archive API surface ({ data, mode } descriptor shape, 0o777 masking, strict integer validation), which is a design call a maintainer should sign off on.
Extended reasoning...
Overview
Adds per-entry Unix file mode support to Bun.Archive creation. Entry values may now be a { data, mode? } descriptor in addition to string/Blob/TypedArray/ArrayBuffer. Touches four files: src/runtime/api/Archive.rs (~50 new lines: DEFAULT_ENTRY_PERM, get_entry_data_and_perm, resolve_entry_perm, and one changed line in build_tarball_from_object), packages/bun-types/bun.d.ts (new ArchiveEntry type + updated ArchiveInput), docs/runtime/archive.mdx (bullet, subsection, reference type), and test/js/bun/archive.test.ts (12 new tests + a parseTarModes helper).
The Rust change is careful: Blob/ArrayBuffer are matched before the descriptor branch (they're objects too), mode is validated as a finite non-negative integer via is_number() → as_number() → range/fract checks before the as u32 cast, and the perm is fully resolved before get_entry_data returns a borrowed byte view so no slice is held across the .get(global, "mode") property read. The one behavioral change to previously-untyped input — a plain-object entry value now requires data instead of stringifying to "[object Object]" — is a strict improvement.
Security risks
None identified. The mode is masked to 0o777 (matching what the extraction path already preserves), so setuid/setgid/sticky bits and file-type bits cannot be injected into created archives. Input validation rejects NaN/Infinity/negative/non-integer before the integer cast. No new filesystem or path handling.
Level of scrutiny
Moderate. The native-code change is small and localized to one function, and test coverage is thorough (mode set/default/mask, all four data types round-trip, Archive.write path, extracted exec bit on POSIX, and rejection of five invalid mode shapes plus missing data). But this is new public API surface on Bun.Archive: the descriptor shape, the choice to mask to 0o777 rather than accept setuid/sticky bits, and throwing (vs. coercing) on non-integer modes are design decisions that lock in once shipped. That's the kind of call the repo's review guidance flags for maintainer sign-off rather than bot approval.
Other factors
My earlier inline comment (missing archive.mdx docs) was fully addressed in 617883a/8e6ab1cd and the thread is resolved. No other reviewer comments are outstanding. CI failures (zlib.test.js, test-net-connect-memleak.js, bake/dev/production.test.ts) are in unrelated files with no plausible connection to this diff. The bug-hunting pass found nothing.
|
Thanks for the review. Rationale on the three API decisions a maintainer would want to weigh, each kept consistent with existing
Happy to adjust any of these if different semantics are preferred. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
use validateIntegerRange or even better the existing node:fs mode validator
Replace the hand-rolled mode checks in resolve_entry_perm with node::types::mode_from_js, the same validator node:fs uses for chmod/mkdir. This gains node-parity error codes (ERR_INVALID_ARG_TYPE / ERR_OUT_OF_RANGE / ERR_INVALID_ARG_VALUE) and octal-string support (e.g. mode: "755"), and keeps the 0o777 masking. The ArchiveEntry mode type widens to number | string to match.
|
Done in 783a296: |
|
Stale PR review: keep open, rework. The feature is still wanted. Main still hard-codes A plain rebase is not safe:
After that, the new public API needs a maintainer's decision. |
This branch was successfully deployed
2 participants
What
Bun.Archivehardcoded0o644for every entry when creating a tarball (entry_ref.set_perm(0o644)inbuild_tarball_from_object), so there was no way to mark a file executable. That made it unusable for building npm tarballs that ship precompiled binaries (esbuild-style platform packages), where the binary must carry0o755inside the tarball.Closes #33212.
API
Entry values may now be a
{ data, mode }descriptor in addition to the existing string / Blob / TypedArray / ArrayBuffer forms:modeis optional; missing/undefined falls back to0o644.modeis masked to0o777, matching the bits the extraction path already preserves (so a fullfs.stat()mode like0o100755resolves to0o755).NaN,Infinity, and non-number modes throw a clearTypeError; a descriptor missingdatathrows.new Bun.Archive({...})andBun.Archive.write(path, {...})go through the same build path, so both gain the feature.Blob / ArrayBuffer / TypedArray are matched before the descriptor branch because they are objects too. Plain-object entry values were never part of the typed input (
Record<string, BlobPart>), so treating them as descriptors is not a change to any typed usage; extraction already preserved modes, so this only closes the creation-side gap.Verification
New tests in
test/js/bun/archive.test.ts(aper-entry file modeblock) verify the stored mode by parsing the tar header bytes (umask-independent, cross-platform) and by extracting on POSIX and checking the owner-exec bit:{ data, mode: 0o755 }round-trips to0o755; plain values stay0o644.dataaccepts string / Blob / Uint8Array / ArrayBuffer and the content round-trips.0o644when omitted; high bits mask to0o777.Bun.Archive.write(...)preserves modes on disk.dataare rejected.11 of the 12 new tests fail on the released binary (the 12th asserts the
0o644default, which matches the old hardcoded value); all pass with the change. Thebun-typestype test passes with the updatedArchiveInput/ newArchiveEntrytypes.