fix(decorators): evaluate member decorator expressions before class decorators - #33407
Closed
springmin wants to merge 1 commit into
Closed
fix(decorators): evaluate member decorator expressions before class decorators#33407springmin wants to merge 1 commit into
springmin wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
WalkthroughThe evaluation order of TC39 decorators in lower_decorators.rs was corrected so member decorator arrays are evaluated before class decorator arrays, matching the spec. Both expression-mode and statement-mode output assembly logic were reordered accordingly. ChangesDecorator emission order fix
Sequence Diagram(s)sequenceDiagram
participant lower_impl
participant MemberDecorators
participant ClassDecorators
lower_impl->>lower_impl: emit base_decl_stmt / base_assign_expr
lower_impl->>MemberDecorators: evaluate member decorator arrays
lower_impl->>ClassDecorators: evaluate class decorator arrays
Related Issues: Related PRs: None found Suggested labels: bug, js-parser, decorators Suggested reviewers: None specified 🐰 A hop, a skip, decorators realign, 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 36
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/install/TarballStream.rs (1)
1466-1486: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge duplicate ENOENT/EPERM/EACCES retry arms.
Both arms do exactly the same thing (derive parent dir,
make_path, retrysymlinkat). Combine them to avoid the two copies drifting apart on a future edit.♻️ Proposed refactor
match bun_sys::symlinkat(target, dest_fd, path) { Ok(()) => true, - Err(e) if e.get_errno() == bun_sys::E::ENOENT => { - let Some(dir) = bun_paths::dirname(path_slice) else { - return false; - }; - let _ = dest_fd.make_path(dir); - bun_sys::symlinkat(target, dest_fd, path).is_ok() - } - Err(e) if e.get_errno() == bun_sys::E::EPERM || e.get_errno() == bun_sys::E::EACCES => { - // OHOS SELinux blocks symlinkat. Ensure parent dir exists and retry. - // A copy fallback is not safe here: the symlink target points at - // another tarball entry that may not be on disk yet. + // ENOENT: missing parent dir. EPERM/EACCES: OHOS SELinux blocks + // symlinkat; ensure parent dir exists and retry (a copy fallback is + // not safe here — the symlink target may not be on disk yet). + Err(e) if matches!(e.get_errno(), bun_sys::E::ENOENT | bun_sys::E::EPERM | bun_sys::E::EACCES) => { let Some(dir) = bun_paths::dirname(path_slice) else { return false; }; let _ = dest_fd.make_path(dir); bun_sys::symlinkat(target, dest_fd, path).is_ok() } Err(_) => false, }🤖 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 `@src/install/TarballStream.rs` around lines 1466 - 1486, The symlink retry logic in TarballStream’s symlinkat match has duplicate recovery branches for ENOENT and EPERM/EACCES that perform the same parent-directory creation and retry. Merge these into a single branch in the symlink handling code so the dirname/path_slice lookup, dest_fd.make_path call, and symlinkat retry live in one place and cannot drift apart.Source: Coding guidelines
test/js/bun/test/dots.test.ts (1)
18-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the OHOS-specific
/storage/Users/line from both snapshots — this makes the test output environment-dependent and brittle on non-OHOS runs. Gate the expectation or normalize the path instead.🤖 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 `@test/js/bun/test/dots.test.ts` around lines 18 - 42, The snapshot in dots.test.ts includes an OHOS-specific "/storage/Users/" error line that makes the expectation environment-dependent. Update the inline snapshot in the relevant test to remove that platform-specific line, or normalize/gate it in the test harness so the output from bun test is stable across non-OHOS environments. Use the dots.test.ts snapshot block and the bun test output assertions to locate the affected expectation.src/jsc/bindings/highway_sourcemap.cpp (1)
312-329: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnaligned
reinterpret_casttype-punning in the new SVEToBitsbranch — usememcpylike the existingmaskToBitshelper.
bitsis a plainuint8_t[8](default 1-byte alignment) and*reinterpret_cast<uint64_t*>(bits)both violates C++'s strict-aliasing rule and risks an unaligneduint64_tread. This exact operation is already done correctly elsewhere in this same file (maskToBitsat Lines 997-1004), usingalignas(8)on the buffer andstd::memcpyto safely type-pun the bytes. The new branch should follow the same, already-established pattern instead of introducing a second, UB-prone implementation of the same conversion.🛠️ Proposed fix mirroring the existing safe pattern
`#elif` HWY_TARGET == HWY_SVE || HWY_TARGET == HWY_SVE2 // Scalable SVE/SVE2: BitsFromMask is only available for fixed-size // variants (SVE_256/SVE2_128). Use StoreMaskBits instead. - uint8_t bits[8] = {}; + alignas(8) uint8_t bits[8] = {}; hn::StoreMaskBits(d, m, bits); - return *reinterpret_cast<uint64_t*>(bits); + uint64_t result; + std::memcpy(&result, bits, sizeof(result)); + return result;🤖 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 `@src/jsc/bindings/highway_sourcemap.cpp` around lines 312 - 329, The new SVE branch in ToBits is using unsafe type-punning by reading a uint64_t through a reinterpret_cast from a byte buffer; fix it by following the existing maskToBits pattern instead. Keep the temporary buffer properly aligned, convert the bytes to uint64_t with std::memcpy, and avoid any direct pointer cast in ToBits so the SVE/SVE2 path matches the safe implementation already used elsewhere in this file.src/cares_sys/c_ares.rs (1)
1925-1948: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove the redundant OHOS-only special case in
src/cares_sys/c_ares.rs
EAI::FAIL => Some(Error::ENOTFOUND)already covers the shared non-Windows path, so the#[cfg(target_env = "ohos")]early return no longer changes behavior and its comment about glibc/macOS is misleading. Drop the branch unless there’s a separate OHOS-only distinction to preserve.🤖 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 `@src/cares_sys/c_ares.rs` around lines 1925 - 1948, The OHOS-only early return in the c-ares error mapping is redundant because `EAI::FAIL => Some(Error::ENOTFOUND)` already handles the shared non-Windows path. Remove the `#[cfg(target_env = "ohos")]` special case from the error conversion logic in `c_ares.rs`, keeping the existing `match eai` mapping intact, and update the surrounding comment or delete it so it no longer suggests a separate glibc/macOS distinction that is not used.scripts/build/source.ts (1)
1403-1421: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the OHOS dep on the pinned toolchain
cfg.ohosoverwritesRUSTUP_TOOLCHAINtostable, but the workspace Rust build still pinscfg.rustToolchainfromrust-toolchain.toml(nightly). That mixes libstds on OHOS cross builds and can trigger the duplicaterust_eh_personalitylink failure this comment already warns about.🤖 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 `@scripts/build/source.ts` around lines 1403 - 1421, The OHOS cross-build path in source.ts is overriding RUSTUP_TOOLCHAIN to stable inside the cross-target block, which conflicts with the workspace’s pinned Rust toolchain and can mix libstds. Update the OHOS branch in the build logic around cfg.ohos, cfg.rustToolchain, and the cross-compilation env setup so it keeps using the pinned toolchain instead of forcing stable, while preserving the existing linker and CARGO_ENCODED_RUSTFLAGS handling.scripts/build/config.ts (1)
185-196: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDeduplicate
hostCcinConfigandresolveConfig()The secondhostCcassignment overwrites the OHOS-specific branch, so OHOS cross-builds fall back totoolchain.hostCc ?? toolchain.cc; the interface also declareshostCctwice.🤖 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 `@scripts/build/config.ts` around lines 185 - 196, Deduplicate the `hostCc` field in `Config` and the corresponding assignment in `resolveConfig()`, since `hostCc` is declared twice and the later entry is overwriting the OHOS-specific behavior. Update the `Config` interface to keep only one `hostCc` definition, and ensure `resolveConfig()` preserves the intended branch for cross-build host tools rather than falling back to `toolchain.hostCc ?? toolchain.cc` for OHOS. Verify the `hostCc` logic still distinguishes native builds, OHOS cross-compiles, and the Windows-from-unix case using the existing `toolchain` handling.Source: Linters/SAST tools
src/spawn/process.rs (1)
3275-3279: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRestore the fallback pipe-drain loop before
reap_child().The comment still says
Noneshould fall through to the plainpoll()loop so buffered stdio drains. The loop was removed, so a child that fills stdout/stderr can deadlock while the parent blocks inwait4().🤖 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 `@src/spawn/process.rs` around lines 3275 - 3279, The fallback pipe-drain path is missing before `reap_child()`, so the parent can block in `wait4()` while buffered stdout/stderr remains undrained. Restore the plain poll-based drain loop in the process wait path around `reap_child(process.pid)`, using the existing kqueue/kevent fallback flow in `src/spawn/process.rs`, so `None` still falls through and stdio is drained before reaping the child.src/standalone_graph/StandaloneModuleGraph.rs (1)
1281-1304: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the second tmpdir-prefix retry path.
Line 1281 makes
znameabsolute in the tmpdir from the start, but the later retry still prependsRealFS::tmpdir_path()tozname. After a transient first-open failure, that builds paths like<tmpdir>/<absolute tmp path>and prevents recovery.🤖 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 `@src/standalone_graph/StandaloneModuleGraph.rs` around lines 1281 - 1304, The temp-file path handling in StandaloneModuleGraph should not prepend the tmpdir twice during the retry path. Since the initial `tmpname`/`zname_z` construction already makes the name absolute under `RealFS::tmpdir_path()`, update the later fallback logic in the same flow to reuse the existing absolute path instead of concatenating the tmpdir again. Fix the retry branch around the temporary open/create handling so it preserves the original `zname`/`zname_owned` path and only retries with the same resolved temp file location.
🤖 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.
Inline comments:
In @.github/workflows/ohos-build-incremental.yml:
- Around line 48-55: The workflow uses mutable action tags and a persisted
checkout token in a reusable workspace. Update the two actions/checkout usages
and both actions/upload-artifact steps to pin them to specific commit SHAs, and
add persist-credentials: false to the checkout step that uses clean: false. Keep
the guidance focused on the checkout/upload steps so the workspace does not
retain credentials between runs.
In @.github/workflows/ohos-build-rust.yml:
- Around line 275-304: The packaging step in the OHOS workflow uploads the built
bun binary without the required signing step, so update the “Package binary” /
“Upload binary (raw)” flow to sign the executable before creating the tarball or
uploading the raw artifact. Add the signing operation in the workflow around the
existing build/release-ohos/bun handling, then package and upload the signed
output using the same artifact variables so the artifact consumers receive the
signed binary.
- Line 218: The workflow uses mutable third-party action tags instead of
immutable commit pins. Update the action references in the build workflow for
actions/cache and actions/upload-artifact (and any other listed occurrences) to
specific commit SHAs so they satisfy the repository policy; locate the relevant
entries by their uses: actions/cache@v4 and actions/upload-artifact@v4 and
replace each with the pinned SHA form.
- Around line 78-94: The checkout step is always building the fixed ohos-aarch64
branch instead of the commit being tested, so the workflow can miss PR/push
changes. Update the “Checkout Bun source (incremental)” logic to use the event’s
commit/PR head SHA from github context when cloning and checking out, rather
than hard-coding the branch in the git clone and git checkout flow. Keep the
incremental local-reference behavior, but make the final checkout in the bun
clone resolve to the submitted revision.
- Around line 126-127: The shell script in the workflow currently interpolates
inputs.webkit_ref directly inside the run block, so move that value into the
step env and read it from a shell variable instead. Update the relevant workflow
step around WK_REPO/WK_REF so the script uses the environment-provided variable
rather than inline GitHub expression syntax, and keep the existing identifiers
like WK_REF and inputs.webkit_ref easy to trace.
In @.github/workflows/ohos-build.yml:
- Line 27: The workflow reads inputs.webkit_ref in the ohos-build workflow
without defining that input, so add a workflow_dispatch input for webkit_ref
with a default value or switch the later expressions to use env.WEBKIT_REF
instead. Update the workflow_dispatch block and any related references in the
workflow steps so the variable is always defined before use.
- Around line 119-124: The ohos build workflow in the commit verification block
is only warning and continuing, so it never enforces the pinned WebKit revision.
Update the logic around the WK_HASH check so the job either checks out/builds
from the verified commit using the existing git step flow, or fails fast with a
nonzero exit when that commit cannot be verified; make sure the behavior is tied
to the WK_HASH gating and the sync/push step so the workflow always uses one
source of truth.
- Line 170: The workflow still uses tag-based action refs instead of immutable
pins. Update the actions in the build workflow that currently reference
actions/cache and actions/upload-artifact to use the repository’s commit SHA
pins instead of v4 tags, keeping the same steps but replacing the ref values in
the corresponding cache and artifact upload entries.
In `@scripts/build/deps/cares.ts`:
- Around line 289-302: The OHOS-specific POSIX macro list in cares.ts is being
retyped manually instead of deriving from the shared POSIX source, which can
drift over time. Update the ohosPosix setup near the platform string assembly to
reuse the existing POSIX macro collection and exclude only the OHOS-incompatible
entry (HAVE_MEMMEM), so the list stays in sync with POSIX changes. Use the
existing POSIX and def1 symbols in this block to locate the refactor.
In `@scripts/build/flags.ts`:
- Around line 1227-1231: The OHOS linker flag entry in the flags list is too
broad because `-Wl,--noinhibit-exec` suppresses all fatal linker errors and can
let broken binaries pass CI. Remove this flag from the OHOS-specific branch in
the flags config and instead address the specific alignment warning at the
source or narrow the suppression to only that warning. Keep the change localized
to the `flags` array entry with `when: c => c.ohos` so other linker behavior
remains unchanged.
In `@scripts/ohos/build-bun-ohos.sh`:
- Around line 82-87: The file-diff scratch files use predictable /tmp paths,
which should be replaced to avoid temp-file collisions and TOCTOU issues. Update
the cleanup block in build-bun-ohos.sh that creates /tmp/dev_files.txt and
/tmp/ci_files.txt to use mktemp-generated unique paths, and keep the existing
find/sort/comm flow using those variables before removing stale files.
- Around line 107-165: sync_webkit currently only reads the current WebKit
commit inside $wk_dir, but all later git operations run in the script’s ambient
directory and may target the wrong repository. Update sync_webkit so every git
fetch, remote add, push, stash, and checkout is executed with the working
directory set to $wk_dir (or via a cd into vendor/WebKit around the entire
function). Keep the existing logic and symbols like sync_webkit, wk_dir,
current, and fetch_ok, but ensure the full sync flow operates on the WebKit
checkout only.
In `@scripts/ohos/build.sh`:
- Around line 121-153: The packaging flow in build.sh uses a predictable
/tmp/bun-release/${PKG_NAME} directory, which can be hijacked by pre-created
files or symlinks on shared hosts. Update the packaging setup around
PKG_NAME/PKG_DIR and the subsequent mkdir -p, cp, README generation, and tar
step to use a unique private parent directory created with mktemp -d, then build
the package entirely under that path before archiving it.
In `@scripts/runner.node.mjs`:
- Around line 75-78: Confirm that the timeout increases in
getNodeParallelTestTimeout and the related runner timeout constants are
intentional and specifically justified by slower cross-compiled environments
like OHOS; if not, reduce the blanket increase and scope the longer timeout only
to the affected cases in scripts/runner.node.mjs so it does not hide hangs or
regressions in normal CI runs.
- Around line 577-579: The parallelism calculation in runner.node.mjs should
reject invalid `BUN_TEST_PARALLELISM` values, because `parseInt(..., 10) ||
availableParallelism()` still allows negative numbers through to `pLimit`.
Update the `parallelism` logic near the `options["parallel"]` check to validate
the env var from `BUN_TEST_PARALLELISM` is a positive integer before using it,
and fall back to `availableParallelism()` otherwise.
In `@src/brotli/lib.rs`:
- Around line 31-40: Update the OHOS allocator in src/brotli/lib.rs so alloc
returns memory aligned to max_align_t, not just HEADER-sized alignment: adjust
the offset logic in alloc/mmap usage so the pointer returned from the mapping is
properly aligned on 64-bit targets. Also harden page_align and the map size
calculation against overflow by using checked arithmetic for len + HEADER and
size + page - 1, and fail safely if the requested region cannot be represented;
apply the same fix to the related alloc/free paths referenced by the Brotli
allocator helpers.
In `@src/install/lockfile/Package/Scripts.rs`:
- Around line 382-385: The add_node_gyp_rebuild_script condition in Scripts.rs
is conflating forced source builds with the “no install/preinstall hooks” case,
which can cause lifecycle scripts to be skipped. Split the logic so the
get_script_entries()/copying behavior only uses the empty install/preinstall
path when those hooks are actually absent, and handle
BUN_FEATURE_FLAG_FORCE_BUILD_FROM_SOURCE separately in the
add_node_gyp_rebuild_script decision without suppressing existing scripts.
In `@src/install/npm.rs`:
- Around line 1180-1191: The cache copy path in npm install is reading from the
same O_TMPFILE fd after it has already been written to, so `file.read` in the
`src/install/npm.rs` fallback loop can hit EOF immediately and produce an empty
cache file. Update the copy logic around `bun_sys::File::create` to rewind or
use offset-based reads before copying, such as resetting the file position to
the start or switching the loop to read via `pread` from offset 0. Keep the
existing write loop to `cache_file.write_all`, but ensure the source fd is
positioned correctly before the copy begins.
In `@src/install/PackageInstall.rs`:
- Around line 1871-1907: The symlink fallback in PackageInstall should use the
resolved cache target, not the destination path or basename. In the EEXIST retry
path around symlinkat, retry with the computed target value instead of
entry.basename, and update copy_file_fallback calls so the source is the cache
entry path/target and the destination remains entry.path. Also handle top-level
files in the EPERM/EACCES branch of the install flow so dirname() returning None
does not cause an early error before the copy fallback runs.
In `@src/install/PackageInstaller.rs`:
- Around line 1813-1823: In the PackageInstaller::install success path for the
ohos-specific signing block, avoid returning early when AbsPath::from or
pkg_path.append fails, because that skips the rest of the InstallResult::Success
bookkeeping. Make the signing-path derivation and ohos_sign_native_binaries call
best-effort only, and then continue through the normal success flow so success
accounting, bin linking, lifecycle-script enqueueing, and
increment_tree_install_count still run.
- Around line 2401-2445: The OHOS native-binary signing logic in
ohos_sign_native_binaries is building the target path from entry.basename and
using from_utf8_unchecked, which can miss nested addons and assumes invalid
UTF-8. Update the path construction to use the walker entry’s full path
(entry.path) when forming the file passed to binary-sign-tool, and replace the
unchecked UTF-8 conversion with a safe path handling approach so signing works
for subdirectories and non-UTF-8 paths.
In `@src/js/node/os.ts`:
- Around line 27-29: The lazyCpus wrapper no longer adds any behavior because it
only forwards binding.cpus, so inline it by removing the separate lazyCpus
function and using the binding value directly wherever it is referenced in
os.ts. Update the surrounding os exports/setup so the cpu accessor still
resolves correctly without the extra passthrough indirection.
In `@src/jsc/bindings/bun-spawn.cpp`:
- Around line 154-183: The fork fallback path in bun-spawn.cpp is still relying
on child_errno after fork(), which does not propagate back to the parent on OHOS
and can incorrectly report success for exec failures. Update the spawn flow
around the vfork()/fork() selection to use an O_CLOEXEC error pipe whenever
use_fork_fallback is set, write exec failures from the child into that pipe, and
have the parent read and handle it before returning success. Keep the fix
aligned with the existing Darwin/FreeBSD error-pipe handling in the spawn exec
path so the logic stays consistent across platforms.
In `@src/jsc/bindings/c-bindings.cpp`:
- Around line 93-97: The fallback executable check in the Linux path currently
only inspects execute bits and can incorrectly accept directories; update the
non-O_EXEC branch in the executable-check helper to also require the path to be
a regular file, preserving the existing S_ISREG guard alongside the stat and
permission check in c-bindings.cpp.
- Around line 1067-1084: The OHOS SIGSYS handler in ohos_sigsys_handler
currently uses stdio calls that are not async-signal-safe. Replace the
fprintf(stderr, ...) and fflush(stderr) logging with an async-signal-safe
approach such as write(2), or capture the syscall number in ohos_sigsys_handler
and defer formatting/logging to code outside the signal context.
In `@src/runtime/api/js_bundle_completion_task.rs`:
- Around line 438-455: The OHOS post-processing in js_bundle_completion_task.rs
is swallowing failures from both ohos_sign_binary and the chmod command. Update
the OHOS block in the compiled output handling to check and surface errors
instead of ignoring them, either by having the signing helper return a result or
by logging/propagating the command status from the existing bundle completion
flow. Keep the fix anchored around the ohos_sign_binary call and the chmod
invocation so failures are not reported as a successful compile.
In `@src/runtime/cli/build_command.rs`:
- Around line 987-1016: The OHOS post-build permission step in the build command
is spawning an external chmod process and ignoring its result; replace that with
the in-tree bun_sys::chmod call in the same OHOS signing block. Use the computed
outfile_path from build_command’s OHOS branch, convert it appropriately for
bun_sys::chmod, and handle/report any returned error instead of discarding the
status.
In `@src/runtime/cli/filter_run.rs`:
- Around line 108-111: The OHOS branch in filter_run is mutating the shared
DotEnv::Loader via ohos_set_pwd() without restoring PWD afterward, so the
current scoped save/restore only covers PATH. Update the same execution path to
snapshot and restore PWD around the ohos_set_pwd(env, &handle.options.cwd) call,
using the existing restore pattern already used for PATH. Keep the change
localized to the OHOS-specific flow in filter_run so each run gets its own cwd
state.
In `@src/runtime/cli/run_command.rs`:
- Around line 688-697: The fallback root lookup in run_command should only run
when the primary directory lookup fails, instead of unconditionally calling
read_dir_info_ignore_error on HOME and "/". Update the logic around
root_dir_info_fallback and the surrounding root_dir_info handling so a valid
primary DirInfo is returned without requiring fallback reads. Also replace
std::env::var("HOME") with bun_core::env_var::HOME::get() and keep the
byte-oriented path handling in the existing resolver calls.
In `@src/runtime/cli/test_command.rs`:
- Around line 2060-2072: The safety-net in test_command::run_tests currently
uses libc::SIGALRM and libc::alarm() unconditionally and arms only once from
ctx.test_options.default_timeout_ms, so update this block to be Unix-only and
ensure it is re-armed per test if the intent is per-test enforcement. Use the
existing sig_alrm_handler and the timeout setup around default_timeout_ms to
gate the alarm logic behind a Unix-specific path, and move or repeat the arming
so each test gets its own watchdog instead of one global timer for the whole
run.
In `@src/runtime/cli/test/parallel/Channel.rs`:
- Around line 486-490: Treat unknown frame kinds as protocol errors in
Channel::ingest instead of skipping them. In the frame::Kind::try_from failure
branch, follow the same error-handling pattern used for the oversized-frame
path: mark the channel as done, stop processing further input, and record an
error status/message. Keep the behavior consistent with existing protocol
violation handling so corrupt or version-skewed data cannot keep the channel
alive.
In `@src/spawn/process.rs`:
- Around line 3934-3937: The parent-death fallback in `spawn/process.rs` should
not trigger when `ppid_to_watch()` returns None and `ppid` is 0. Update the
watchdog condition in the parent-check block so the `getppid() != ppid` fallback
is only evaluated when `ppid > 1`, alongside the existing
`ppid_fd.fd()`/`buf[ppid_idx].revents` logic. Use the `ppid`, `ppid_fd`, and
`ParentDeathWatchdog::EXIT_CODE` path to locate the guard and ensure the OHOS
fallback does not exit immediately on the first poll.
In `@src/standalone_graph/StandaloneModuleGraph.rs`:
- Around line 1777-1783: The file:// handling in StandaloneModuleGraph::load
tarball logic is stripping the prefix and using std::fs/std::path directly,
which breaks proper URL parsing and violates the runtime API guidelines. Update
this branch to parse the URL through the existing URL-to-file-path conversion
used elsewhere in StandaloneModuleGraph, then read the tarball via
bun_sys/bun_core file APIs instead of std::fs::read. Keep the error handling in
this block but route it through the same bun_core error path after converting
the file URL correctly.
- Around line 1635-1649: Handle the OHOS-specific `Syscall::ftruncate` and
`libc::fsync` failures in `StandaloneModuleGraph::...` where the ELF payload is
written before `move_file_z_with_handle`. Check both return values, and if
either call fails, log the error, run `cleanup(zname, cloned_executable_fd)`,
and return `Fd::INVALID` so a corrupt executable is not published.
In `@src/sys/lib.rs`:
- Around line 6180-6196: The OHOS signing helper silently drops both path
conversion and signing errors, so update ohos_sign_binary to return a
Result/Maybe instead of void and propagate failures back to the caller. In the
ZStr-to-CStr conversion branch, return an error rather than exiting quietly, and
in the binary-sign-tool sign path, check the Command::output outcome and fail
when the tool is missing or returns a non-success status. Then update the bun
build --compile flow that calls ohos_sign_binary to stop the compile on signing
failure, since OHOS requires successful signing and the helper must not signal
success when it did not sign.
In `@test/harness.ts`:
- Around line 1518-1520: The expiredTls fixture setup in harness.ts is
swallowing initialization failures with an empty catch, which can leave the
fixture partially initialized and hide the real error. Update the expiredTls
creation path to either handle the thrown error explicitly by
logging/propagating it, or guard the certificate/PFX generation with an
appropriate platform capability check before entering the try/catch. Use the
expiredTls setup block and its surrounding try/catch to locate the fix, and
remove the unconditional empty catch so failures are visible immediately.
---
Outside diff comments:
In `@scripts/build/config.ts`:
- Around line 185-196: Deduplicate the `hostCc` field in `Config` and the
corresponding assignment in `resolveConfig()`, since `hostCc` is declared twice
and the later entry is overwriting the OHOS-specific behavior. Update the
`Config` interface to keep only one `hostCc` definition, and ensure
`resolveConfig()` preserves the intended branch for cross-build host tools
rather than falling back to `toolchain.hostCc ?? toolchain.cc` for OHOS. Verify
the `hostCc` logic still distinguishes native builds, OHOS cross-compiles, and
the Windows-from-unix case using the existing `toolchain` handling.
In `@scripts/build/source.ts`:
- Around line 1403-1421: The OHOS cross-build path in source.ts is overriding
RUSTUP_TOOLCHAIN to stable inside the cross-target block, which conflicts with
the workspace’s pinned Rust toolchain and can mix libstds. Update the OHOS
branch in the build logic around cfg.ohos, cfg.rustToolchain, and the
cross-compilation env setup so it keeps using the pinned toolchain instead of
forcing stable, while preserving the existing linker and CARGO_ENCODED_RUSTFLAGS
handling.
In `@src/cares_sys/c_ares.rs`:
- Around line 1925-1948: The OHOS-only early return in the c-ares error mapping
is redundant because `EAI::FAIL => Some(Error::ENOTFOUND)` already handles the
shared non-Windows path. Remove the `#[cfg(target_env = "ohos")]` special case
from the error conversion logic in `c_ares.rs`, keeping the existing `match eai`
mapping intact, and update the surrounding comment or delete it so it no longer
suggests a separate glibc/macOS distinction that is not used.
In `@src/install/TarballStream.rs`:
- Around line 1466-1486: The symlink retry logic in TarballStream’s symlinkat
match has duplicate recovery branches for ENOENT and EPERM/EACCES that perform
the same parent-directory creation and retry. Merge these into a single branch
in the symlink handling code so the dirname/path_slice lookup, dest_fd.make_path
call, and symlinkat retry live in one place and cannot drift apart.
In `@src/jsc/bindings/highway_sourcemap.cpp`:
- Around line 312-329: The new SVE branch in ToBits is using unsafe type-punning
by reading a uint64_t through a reinterpret_cast from a byte buffer; fix it by
following the existing maskToBits pattern instead. Keep the temporary buffer
properly aligned, convert the bytes to uint64_t with std::memcpy, and avoid any
direct pointer cast in ToBits so the SVE/SVE2 path matches the safe
implementation already used elsewhere in this file.
In `@src/spawn/process.rs`:
- Around line 3275-3279: The fallback pipe-drain path is missing before
`reap_child()`, so the parent can block in `wait4()` while buffered
stdout/stderr remains undrained. Restore the plain poll-based drain loop in the
process wait path around `reap_child(process.pid)`, using the existing
kqueue/kevent fallback flow in `src/spawn/process.rs`, so `None` still falls
through and stdio is drained before reaping the child.
In `@src/standalone_graph/StandaloneModuleGraph.rs`:
- Around line 1281-1304: The temp-file path handling in StandaloneModuleGraph
should not prepend the tmpdir twice during the retry path. Since the initial
`tmpname`/`zname_z` construction already makes the name absolute under
`RealFS::tmpdir_path()`, update the later fallback logic in the same flow to
reuse the existing absolute path instead of concatenating the tmpdir again. Fix
the retry branch around the temporary open/create handling so it preserves the
original `zname`/`zname_owned` path and only retries with the same resolved temp
file location.
In `@test/js/bun/test/dots.test.ts`:
- Around line 18-42: The snapshot in dots.test.ts includes an OHOS-specific
"/storage/Users/" error line that makes the expectation environment-dependent.
Update the inline snapshot in the relevant test to remove that platform-specific
line, or normalize/gate it in the test harness so the output from bun test is
stable across non-OHOS environments. Use the dots.test.ts snapshot block and the
bun test output assertions to locate the affected expectation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: abf636e3-604f-4443-aeb4-2ccfc6c0990f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (139)
.github/workflows/ohos-build-incremental.yml.github/workflows/ohos-build-rust.yml.github/workflows/ohos-build.yml.gitignorebuild-bun-ohos.shc.ohospackage.jsonpackages/bun-usockets/src/crypto/root_certs.cpppackages/bun-usockets/src/crypto/root_certs_linux.cpppatches/lolhtml/crate-type.patchpatches/zstd/ohos-qsort-r.patchrust-toolchain.tomlscripts/build.tsscripts/build/bun.tsscripts/build/config.tsscripts/build/deps/cares.tsscripts/build/deps/mimalloc.tsscripts/build/deps/webkit.tsscripts/build/deps/zstd.tsscripts/build/flags.tsscripts/build/rust.tsscripts/build/source.tsscripts/build/tools.tsscripts/ohos/build-bun-ohos.shscripts/ohos/build.shscripts/ohos/git-fetch-upstream.shscripts/ohos/prepare-cross-libs.shscripts/ohos/run-all-official.shscripts/runner.node.mjsscripts/utils.mjssrc/CLAUDE.mdsrc/brotli/lib.rssrc/bun_alloc/BufferFallbackAllocator.rssrc/bun_alloc/NullableAllocator.rssrc/bun_alloc/fallback.rssrc/bun_alloc/fallback/z.rssrc/bun_alloc/lib.rssrc/bun_bin/lib.rssrc/bun_core/Global.rssrc/bun_core/env.rssrc/bun_core/env_var.rssrc/cares_sys/c_ares.rssrc/collections/multi_array_list.rssrc/crash_handler/lib.rssrc/http/lib.rssrc/http_jsc/headers_jsc.rssrc/install/PackageInstall.rssrc/install/PackageInstaller.rssrc/install/PackageManager.rssrc/install/PackageManager/CommandLineArguments.rssrc/install/PackageManager/PackageManagerLifecycle.rssrc/install/PackageManager/install_with_manager.rssrc/install/TarballStream.rssrc/install/isolated_install/Hardlinker.rssrc/install/lib.rssrc/install/lifecycle_script_runner.rssrc/install/lockfile/Package/Scripts.rssrc/install/npm.rssrc/io/ParentDeathWatchdog.rssrc/js/builtins/ProcessObjectInternals.tssrc/js/node/os.tssrc/js_parser/lower/lower_decorators.rssrc/jsc/FetchHeaders.rssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/ZigGlobalObject.hsrc/jsc/bindings/bun-spawn.cppsrc/jsc/bindings/c-bindings.cppsrc/jsc/bindings/highway_json.cppsrc/jsc/bindings/highway_sourcemap.cppsrc/jsc/bindings/highway_strings.cppsrc/jsc/bindings/v8/V8FunctionCallbackInfo.cppsrc/jsc/bindings/v8/V8FunctionCallbackInfo.hsrc/jsc/bindings/v8/V8Isolate.hsrc/jsc/bindings/v8/v8_handle_scope_data.hsrc/jsc/bindings/xxhash3.cppsrc/jsc/webcore_types.rssrc/libarchive/lib.rssrc/options_types/compile_target.rssrc/resolver/fs.rssrc/resolver/lib.rssrc/resolver/package_json.rssrc/resolver/resolver.rssrc/runtime/api/Archive.rssrc/runtime/api/bun/Terminal.rssrc/runtime/api/bun/spawn/stdio.rssrc/runtime/api/js_bundle_completion_task.rssrc/runtime/bake/DevServer.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/build_command.rssrc/runtime/cli/bunx_command.rssrc/runtime/cli/filter_run.rssrc/runtime/cli/pack_command.rssrc/runtime/cli/run_command.rssrc/runtime/cli/test/parallel/Channel.rssrc/runtime/cli/test_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/napi/napi_body.rssrc/runtime/server/HTMLBundle.rssrc/runtime/server/server_body.rssrc/runtime/shell/builtin/echo.rssrc/runtime/shell/builtin/which.rssrc/runtime/shell/subproc.rssrc/runtime/socket/Listener.rssrc/runtime/socket/socket_body.rssrc/runtime/test_runner/harness/recover.rssrc/runtime/webcore/BakeResponse.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/Body.rssrc/runtime/webcore/ReadableStream.rssrc/runtime/webcore/Request.rssrc/runtime/webcore/Response.rssrc/runtime/webcore/S3Client.rssrc/runtime/webcore/S3File.rssrc/runtime/webcore/blob/Store.rssrc/runtime/webcore/fetch.rssrc/runtime/webcore/fetch/FetchTasklet.rssrc/runtime/webcore/s3/client.rssrc/runtime/webview/WebKitBackend.cppsrc/spawn/process.rssrc/spawn_sys/spawn_process.rssrc/standalone_graph/StandaloneModuleGraph.rssrc/symbols.dynsrc/sys/lib.rssrc/sys/linux_syscall.rstest/bake/bake-harness.tstest/bake/dev/bundle.test.tstest/bundler/bun-build-compile.test.tstest/bundler/transpiler/jsx-production.tsxtest/harness.tstest/internal/bun-alloc-unsafe-free.test.tstest/internal/libarchive-free-safety.test.tstest/js/bun/terminal/terminal-platform-gaps.test.tstest/js/bun/test/dots.test.tstest/js/bun/util/filesink.test.tstest/js/node/fs/fs-birthtime-linux.test.tstest/js/node/net/double-connect.test.tstest/js/node/process/process.test.jstest/js/web/websocket/websocket-close-fragmented.test.tstest/package.json
Comment on lines
+48
to
+55
| - name: Checkout Bun source (incremental — preserve workspace) | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: springmin/bun | ||
| ref: ohos-aarch64 | ||
| fetch-depth: 1 | ||
| # Do NOT clean — keep build artifacts for incremental rebuild | ||
| clean: false |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
bunx actions-up .github/workflows/ohos-build-incremental.ymlRepository: oven-sh/bun
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## workflow excerpt"
cat -n .github/workflows/ohos-build-incremental.yml | sed -n '1,260p'
echo
echo "## action references"
rg -n "uses:\s+" .github/workflows/ohos-build-incremental.yml
echo
echo "## checkout defaults relevant to persisted credentials"
python3 - <<'PY'
from pathlib import Path
text = Path('.github/workflows/ohos-build-incremental.yml').read_text()
for key in ['persist-credentials', 'clean:', 'actions/checkout@', 'actions/upload-artifact@']:
print(f"{key}: {text.count(key)}")
PYRepository: oven-sh/bun
Length of output: 11999
Pin the actions and disable persisted checkout credentials.
This self-hosted workflow keeps the workspace between runs (clean: false), so actions/checkout should set persist-credentials: false to avoid leaving the token in .git/config. Also replace the mutable @v4 references on actions/checkout and both actions/upload-artifact steps with commit SHA pins.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 48-55: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 49-49: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ohos-build-incremental.yml around lines 48 - 55, The
workflow uses mutable action tags and a persisted checkout token in a reusable
workspace. Update the two actions/checkout usages and both
actions/upload-artifact steps to pin them to specific commit SHAs, and add
persist-credentials: false to the checkout step that uses clean: false. Keep the
guidance focused on the checkout/upload steps so the workspace does not retain
credentials between runs.
Source: Linters/SAST tools
Comment on lines
+78
to
+94
| - name: Checkout Bun source (incremental) | ||
| run: | | ||
| LOCAL=/home/user/sources/bun | ||
| rm -rf bun | ||
| if [ -d "$LOCAL/.git" ]; then | ||
| git clone --reference "$LOCAL/.git" \ | ||
| https://github.com/springmin/bun.git bun 2>/dev/null || \ | ||
| git clone --depth 1 --branch ohos-aarch64 \ | ||
| https://github.com/springmin/bun.git bun | ||
| cd bun | ||
| git fetch origin ohos-aarch64 --depth=1 | ||
| git checkout -f FETCH_HEAD | ||
| echo "✅ Cloned from local reference" | ||
| else | ||
| git clone --depth 1 --branch ohos-aarch64 \ | ||
| https://github.com/springmin/bun.git bun | ||
| echo "⚠️ Full clone (no local reference)" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Build the event SHA, not the fixed ohos-aarch64 branch.
This workflow currently ignores github.sha/PR head and always checks out springmin/bun’s ohos-aarch64, so pushes to claude/ohos-* and pull requests can pass without building the submitted changes.
Proposed checkout shape
- git clone --reference "$LOCAL/.git" \
- https://github.com/springmin/bun.git bun 2>/dev/null || \
- git clone --depth 1 --branch ohos-aarch64 \
- https://github.com/springmin/bun.git bun
+ git clone --reference-if-able "$LOCAL/.git" --no-checkout \
+ "$GITHUB_SERVER_URL/$GITHUB_REPOSITORY.git" bun
cd bun
- git fetch origin ohos-aarch64 --depth=1
- git checkout -f FETCH_HEAD
+ git fetch --depth=1 origin "$GITHUB_SHA"
+ git checkout --detach "$GITHUB_SHA"🤖 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 @.github/workflows/ohos-build-rust.yml around lines 78 - 94, The checkout
step is always building the fixed ohos-aarch64 branch instead of the commit
being tested, so the workflow can miss PR/push changes. Update the “Checkout Bun
source (incremental)” logic to use the event’s commit/PR head SHA from github
context when cloning and checking out, rather than hard-coding the branch in the
git clone and git checkout flow. Keep the incremental local-reference behavior,
but make the final checkout in the bun clone resolve to the submitted revision.
Comment on lines
+126
to
+127
| WK_REPO=${{ env.WEBKIT_FORK }} | ||
| WK_REF=${{ inputs.webkit_ref || env.WEBKIT_REF }} |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Move workflow inputs out of shell source text.
inputs.webkit_ref is interpolated directly into a run: script on a self-hosted runner. Pass it via step env and reference the shell variable instead.
Proposed hardening
- name: Checkout WebKit fork (incremental)
+ env:
+ WK_REPO: ${{ env.WEBKIT_FORK }}
+ WK_REF: ${{ inputs.webkit_ref || env.WEBKIT_REF }}
run: |
@@
- WK_REPO=${{ env.WEBKIT_FORK }}
- WK_REF=${{ inputs.webkit_ref || env.WEBKIT_REF }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| WK_REPO=${{ env.WEBKIT_FORK }} | |
| WK_REF=${{ inputs.webkit_ref || env.WEBKIT_REF }} | |
| - name: Checkout WebKit fork (incremental) | |
| env: | |
| WK_REPO: ${{ env.WEBKIT_FORK }} | |
| WK_REF: ${{ inputs.webkit_ref || env.WEBKIT_REF }} | |
| run: | |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 126-126: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 127-127: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 127-127: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 @.github/workflows/ohos-build-rust.yml around lines 126 - 127, The shell
script in the workflow currently interpolates inputs.webkit_ref directly inside
the run block, so move that value into the step env and read it from a shell
variable instead. Update the relevant workflow step around WK_REPO/WK_REF so the
script uses the environment-provided variable rather than inline GitHub
expression syntax, and keep the existing identifiers like WK_REF and
inputs.webkit_ref easy to trace.
Source: Linters/SAST tools
| echo "Cache key: webkit-ohos-rust-${{ inputs.webkit_ref || env.WEBKIT_REF }}-${WK_VER}" | ||
|
|
||
| - name: Cache WebKit build | ||
| uses: actions/cache@v4 |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin third-party actions to commit SHAs.
actions/cache@v4 and actions/upload-artifact@v4 are tag refs; the workflow policy requires immutable SHA pins.
Also applies to: 294-294, 301-301
🧰 Tools
🪛 zizmor (1.26.1)
[error] 218-218: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ohos-build-rust.yml at line 218, The workflow uses mutable
third-party action tags instead of immutable commit pins. Update the action
references in the build workflow for actions/cache and actions/upload-artifact
(and any other listed occurrences) to specific commit SHAs so they satisfy the
repository policy; locate the relevant entries by their uses: actions/cache@v4
and actions/upload-artifact@v4 and replace each with the pinned SHA form.
Source: Linters/SAST tools
Comment on lines
+275
to
+304
| - name: Package binary | ||
| working-directory: bun | ||
| run: | | ||
| VERSION=$(build/release-ohos/bun --version 2>/dev/null || python3 -c "import json; print(json.load(open('package.json'))['version'])") | ||
| COMMIT=$(git rev-parse --short HEAD) | ||
| DATE=$(date +%Y%m%d_%H%M%S) | ||
| NAME="bun-ohos-aarch64-${VERSION}-${COMMIT}-${DATE}" | ||
| mkdir -p /tmp/bun-release/${NAME} | ||
| cp build/release-ohos/bun /tmp/bun-release/${NAME}/bun | ||
| cat > /tmp/bun-release/${NAME}/README.md << 'EOF' | ||
| # Bun for HarmonyOS (OHOS) | ||
| Rust-based build. See springmin/bun on GitHub for details. | ||
| EOF | ||
| cd /tmp/bun-release | ||
| tar czf ${NAME}.tar.gz ${NAME}/ | ||
| echo "artifact_name=${NAME}" >> $GITHUB_ENV | ||
| echo "artifact_path=/tmp/bun-release/${NAME}.tar.gz" >> $GITHUB_ENV | ||
|
|
||
| - name: Upload artifact | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: ${{ env.artifact_name }} | ||
| path: ${{ env.artifact_path }} | ||
| retention-days: 30 | ||
|
|
||
| - name: Upload binary (raw) | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: ${{ env.artifact_name }}.bun | ||
| path: bun/build/release-ohos/bun |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Sign the OHOS binary before packaging/uploading it.
The workflow uploads the raw build/release-ohos/bun artifact, but OHOS device execution requires the signing step. Add signing before the tarball/raw artifact upload. Based on learnings, for OHOS, account for “the required binary-signing step.” <retrieved_learnings>
🧰 Tools
🪛 zizmor (1.26.1)
[error] 294-294: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 301-301: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ohos-build-rust.yml around lines 275 - 304, The packaging
step in the OHOS workflow uploads the built bun binary without the required
signing step, so update the “Package binary” / “Upload binary (raw)” flow to
sign the executable before creating the tarball or uploading the raw artifact.
Add the signing operation in the workflow around the existing
build/release-ohos/bun handling, then package and upload the signed output using
the same artifact variables so the artifact consumers receive the signed binary.
Source: Learnings
Comment on lines
+3934
to
+3937
| if (ppid_fd.fd() != Fd::INVALID && buf[ppid_idx].revents != 0) | ||
| || getppid() != ppid | ||
| { | ||
| Global::exit(ParentDeathWatchdog::EXIT_CODE as u32); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Guard the OHOS parent-death fallback with ppid > 1.
When ppid_to_watch() returns None, ppid is 0; this condition then calls Global::exit(...) after the first poll because getppid() != 0 is always true.
Proposed fix
if (ppid_fd.fd() != Fd::INVALID && buf[ppid_idx].revents != 0)
- || getppid() != ppid
+ || (ppid > 1 && getppid() != ppid)
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (ppid_fd.fd() != Fd::INVALID && buf[ppid_idx].revents != 0) | |
| || getppid() != ppid | |
| { | |
| Global::exit(ParentDeathWatchdog::EXIT_CODE as u32); | |
| if (ppid_fd.fd() != Fd::INVALID && buf[ppid_idx].revents != 0) | |
| || (ppid > 1 && getppid() != ppid) | |
| { | |
| Global::exit(ParentDeathWatchdog::EXIT_CODE as u32); |
🤖 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 `@src/spawn/process.rs` around lines 3934 - 3937, The parent-death fallback in
`spawn/process.rs` should not trigger when `ppid_to_watch()` returns None and
`ppid` is 0. Update the watchdog condition in the parent-check block so the
`getppid() != ppid` fallback is only evaluated when `ppid > 1`, alongside the
existing `ppid_fd.fd()`/`buf[ppid_idx].revents` logic. Use the `ppid`,
`ppid_fd`, and `ParentDeathWatchdog::EXIT_CODE` path to locate the guard and
ensure the OHOS fallback does not exit immediately on the first poll.
Comment on lines
+1635
to
+1649
| #[cfg(target_env = "ohos")] | ||
| let _ = Syscall::ftruncate(cloned_executable_fd, 0); | ||
|
|
||
| let write_file = bun_sys::File::borrow(&cloned_executable_fd); | ||
| if let Err(err) = write_file.write_all(&elf_file.data) { | ||
| bun_core::pretty_errorln!("Error writing ELF file: {}", err); | ||
| cleanup(zname, cloned_executable_fd); | ||
| return Fd::INVALID; | ||
| } | ||
|
|
||
| // OHOS: fsync before move, because move_file_z_with_handle may use | ||
| // copy_file_range (EXDEV fallback) which reads from disk, not the | ||
| // page cache. Without fsync, the on-disk data may be stale. | ||
| #[cfg(target_env = "ohos")] | ||
| unsafe { libc::fsync(cloned_executable_fd.native()); } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle OHOS ftruncate and fsync failures.
These calls are required by the comments to avoid stale on-disk ELF data, but both return values are ignored. If either fails, this can still publish a corrupt standalone executable.
🤖 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 `@src/standalone_graph/StandaloneModuleGraph.rs` around lines 1635 - 1649,
Handle the OHOS-specific `Syscall::ftruncate` and `libc::fsync` failures in
`StandaloneModuleGraph::...` where the ELF payload is written before
`move_file_z_with_handle`. Check both return values, and if either call fails,
log the error, run `cleanup(zname, cloned_executable_fd)`, and return
`Fd::INVALID` so a corrupt executable is not published.
Comment on lines
+1777
to
+1783
| // Support file:// protocol for local tarballs (e.g., CI cross-compile) | ||
| if strings::has_prefix(&url_str_copy, b"file://") { | ||
| let path = &url_str_copy[b"file://".len()..]; | ||
| let path_str = String::from_utf8_lossy(path); | ||
| let raw_data = std::fs::read(std::path::Path::new(path_str.as_ref())) | ||
| .map_err(|e| bun_core::Error::from(std::io::Error::new(e.kind(), format!("reading local tarball {}: {}", path_str, e))))?; | ||
| compressed_archive_bytes.list = raw_data.into(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse file:// URLs instead of stripping the prefix.
This mishandles valid file URLs with percent-encoding or a host component, and it also routes runtime file I/O through std::fs/std::path. Use the existing URL/file-path conversion and bun_sys file APIs. As per coding guidelines, runtime Rust code should prefer bun_core / bun_sys over std equivalents for file and path logic.
🤖 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 `@src/standalone_graph/StandaloneModuleGraph.rs` around lines 1777 - 1783, The
file:// handling in StandaloneModuleGraph::load tarball logic is stripping the
prefix and using std::fs/std::path directly, which breaks proper URL parsing and
violates the runtime API guidelines. Update this branch to parse the URL through
the existing URL-to-file-path conversion used elsewhere in
StandaloneModuleGraph, then read the tarball via bun_sys/bun_core file APIs
instead of std::fs::read. Keep the error handling in this block but route it
through the same bun_core error path after converting the file URL correctly.
Source: Coding guidelines
Comment on lines
+6180
to
+6196
| pub fn ohos_sign_binary(path: &ZStr) { | ||
| use std::process::Command; | ||
| let path_str = match path.as_cstr().to_str() { | ||
| Ok(s) => s, | ||
| Err(_) => return, | ||
| }; | ||
| // Skip if already signed. | ||
| if Command::new("binary-sign-tool") | ||
| .args(["display-sign", "-inFile", path_str]) | ||
| .output() | ||
| .is_ok_and(|o| o.status.success()) | ||
| { | ||
| return; | ||
| } | ||
| let _ = Command::new("binary-sign-tool") | ||
| .args(["sign", "-selfSign", "1", "-inFile", path_str, "-outFile", path_str]) | ||
| .output(); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate OHOS signing failures.
bun build --compile can return an unsigned executable if binary-sign-tool is missing/fails, because both UTF-8 conversion failure and the sign command result are silently ignored. Return a Result/Maybe from this helper and fail the compile when signing fails. Based on learnings, OHOS requires the binary-signing step. As per coding guidelines, “Never swallow a failure or signal success on one.”
🤖 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 `@src/sys/lib.rs` around lines 6180 - 6196, The OHOS signing helper silently
drops both path conversion and signing errors, so update ohos_sign_binary to
return a Result/Maybe instead of void and propagate failures back to the caller.
In the ZStr-to-CStr conversion branch, return an error rather than exiting
quietly, and in the binary-sign-tool sign path, check the Command::output
outcome and fail when the tool is missing or returns a non-success status. Then
update the bun build --compile flow that calls ohos_sign_binary to stop the
compile on signing failure, since OHOS requires successful signing and the
helper must not signal success when it did not sign.
Sources: Coding guidelines, Learnings
Comment on lines
+1518
to
+1520
| passphrase: "1234", | ||
| }); | ||
|
|
||
| } catch (_) {} |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Empty catch silently swallows expiredTls fixture setup failures.
If certificate/PFX generation throws (e.g. an unsupported crypto primitive on a given platform), this now fails silently and expiredTls may end up partially initialized or undefined, deferring the failure to whichever test consumes it later with a much less clear error. As per coding guidelines, "Never swallow a failure or signal success on one" — consider logging the error or gating this fixture behind a platform check instead of an unconditional empty catch.
🤖 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 `@test/harness.ts` around lines 1518 - 1520, The expiredTls fixture setup in
harness.ts is swallowing initialization failures with an empty catch, which can
leave the fixture partially initialized and hide the real error. Update the
expiredTls creation path to either handle the thrown error explicitly by
logging/propagating it, or guard the certificate/PFX generation with an
appropriate platform capability check before entering the try/catch. Use the
expiredTls setup block and its surrounding try/catch to locate the fix, and
remove the unconditional empty catch so failures are visible immediately.
Source: Coding guidelines
…ecorator expressions Per the ES decorator proposal (tc39/proposal-decorators), member decorator expressions must be evaluated before class decorator expressions. Bun was evaluating class decorator arrays first, causing to call d1()/d2() factories before d3(). Fix: reorder Phase 8 output assembly to emit member decorator array declarations (pre_eval_stmts) before the class decorator array (class_dec_stmt/class_dec_assign_expr).
springmin
force-pushed
the
claude/fix-decorator-eval-order
branch
from
July 6, 2026 02:35
d33c9a3 to
0f7d7cb
Compare
Author
|
Closing — analysis was incorrect. ES decorator expression evaluation order is source-text order (class first, members second), matching TypeScript. The reported test failures on OHOS have a different root cause. |
Contributor
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)
src/js_parser/lower/lower_decorators.rs (1)
2440-2487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReorder is spec-correct, but the existing test can't prove it.
The expression-mode reorder (base heritage first, member decorator arrays via the
pre_eval_stmts/prefix_stmtsloop, thenclass_dec_assign_expr) correctly evaluates member decorator expressions before the class decorator expression, matchingClassDefinitionEvaluation.However, the change this PR makes is only observable with decorator factories (
@dec()), where the array-construction call has a side effect. Thedecorators.test.tscase referenced for this behavior uses plain-function decorators, so its counter only tracks application order (emitted insuffix_exprs, unchanged here) — it passes both before and after this reorder and therefore does not exercise the fix. Please add a factory-based case asserting the member factory runs before the class factory.test("member decorator factories run before class decorator factory", () => { const calls: string[] = []; const cls = (t: string) => (calls.push(t), () => {}); `@cls`("class") class C { `@cls`("member") m() {} } expect(calls).toEqual(["member", "class"]); });🤖 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 `@src/js_parser/lower/lower_decorators.rs` around lines 2440 - 2487, The current decorators test does not verify the observable behavior of the reorder in the expression lowering path, since plain function decorators only check application order and not factory evaluation. Add a new test in the decorators suite that uses decorator factories with side effects and asserts the member factory runs before the class factory, so the behavior introduced in lower_decorators::lower_decorators is actually covered; use the existing decorators.test.ts patterns and keep the assertion focused on the evaluation order of the factory calls.
🤖 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 `@src/js_parser/lower/lower_decorators.rs`:
- Around line 2440-2487: The current decorators test does not verify the
observable behavior of the reorder in the expression lowering path, since plain
function decorators only check application order and not factory evaluation. Add
a new test in the decorators suite that uses decorator factories with side
effects and asserts the member factory runs before the class factory, so the
behavior introduced in lower_decorators::lower_decorators is actually covered;
use the existing decorators.test.ts patterns and keep the assertion focused on
the evaluation order of the factory calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1803fe0c-766e-4442-8c97-aa5db0fad04e
📒 Files selected for processing (1)
src/js_parser/lower/lower_decorators.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #33406
Problem
Per the ES decorator proposal, member decorator expressions must be evaluated before class decorator expressions. Bun was emitting the class decorator array declaration before member decorator array declarations, causing class decorator factories to be called first.
For
@d1 @d2 class C { @d3 x; }:Fix
In
src/js_parser/lower/lower_decorators.rsPhase 8 (Assemble output), reorder the emission of class decorator array to come after member decorator arrays:class_dec_stmtafterpre_eval_stmtsclass_dec_assign_exprafter thepre_eval_stmtsloopNotes
This does not fix other decorator bugs in the same test file:
targetbinding (expect target === prototype, receives false)propertyKeyserialization ([object Object]instead of string)export default classdecorated methods not applied