Add bun pm sbom command - #29512
Conversation
|
Updated 10:47 PM PT - Aug 14th, 2026
❌ @robobun, your commit 36ae2ab has 3 failures in
🧪 To try this PR locally: bunx bun-pr 29512That installs a local version of the PR into your bun-29512 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
ef8b96f to
75a7827
Compare
StatusReady for maintainer review at 36ae2ab, a single commit rebased onto current main (most recent rebase resolved keep-both conflicts with the new Verified with CI on 36ae2ab: Rebase details are in the PR description. Closes #8483. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/pm_sbom_command.zig`:
- Around line 365-388: The function collectDeps declares unused parameters
deps_buf and pkg_dependencies (they are immediately discarded with `_ =`) —
either remove these parameters from collectDeps' signature and update every call
site to stop passing those arguments, or, if intended, implement the missing
logic to use pkg_dependencies and deps_buf (e.g., filter resolved_id by
consulting pkg_dependencies and map via deps_buf) inside collectDeps; update the
symbol names mentioned (collectDeps, deps_buf, pkg_dependencies,
comp.deps.append) accordingly so callers and internal logic stay consistent.
🪄 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: d795fc1d-65eb-4fda-887b-426caa6f4a0d
📒 Files selected for processing (5)
src/cli/package_manager_command.zigsrc/cli/pm_sbom_command.zigsrc/install/PackageManager/CommandLineArguments.zigsrc/install/PackageManager/PackageManagerOptions.zigtest/cli/install/bun-pm-sbom.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/pm_sbom_command.zig`:
- Around line 76-78: The writeAll call currently swallows all errors via
`Output.writer().writeAll(output) catch {}`, so change it to explicitly handle
errors: capture the result, if the error is the broken-pipe EPIPE (e.g., compare
against `error.EPIPE` or the platform-specific EPIPE constant) then ignore it,
otherwise log the error or propagate/return it (for example use `|err| { if (err
== error.EPIPE) {} else std.debug.print("stdout write failed: {}\n", .{err});
}`). Update the block around the `Output.writer().writeAll(output)` invocation
to implement this explicit branching so non-EPIPE I/O failures are not silently
discarded while preserving the common broken-pipe behavior.
- Around line 332-340: The scope selection for packages (using pkg_flags[idx],
flags, and Scope) misclassifies packages with optional=true and dev=true as
.required; update the conditional order so that after checking for required and
the dev-only excluded case you check if flags.optional and !flags.required and
return .optional (i.e., add a branch evaluating flags.optional and
!flags.required before the final else), leaving the existing .dev-only
(.excluded) and default .required branches intact so packages with only optional
non-dev edges are marked .optional.
🪄 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: 8131f36e-636e-4aae-8b29-bfb08554f5ed
📥 Commits
Reviewing files that changed from the base of the PR and between 75a7827 and aa6b852c0115f0935430f92c0fefa163e4ef9efa.
📒 Files selected for processing (1)
src/cli/pm_sbom_command.zig
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/cli/pm_sbom_command.zig (2)
76-78:⚠️ Potential issue | 🟡 MinorDo not silently swallow stdout write failures.
catch {}masks all output errors, which can hide real write failures in redirected/CI environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/pm_sbom_command.zig` around lines 76 - 78, The write to stdout is silently ignored by the `Output.writer().writeAll(output) catch {}` which can hide real failures; replace the empty catch with proper error handling—either propagate the error with `try Output.writer().writeAll(output);` or handle and report it (e.g., log to stderr or return the error) inside the `pm_sbom_command` flow so write failures are not swallowed. Ensure you update any surrounding function signature to propagate the error if you choose `try`.
337-344:⚠️ Potential issue | 🟠 MajorFix scope classification when a package has both dev and optional incoming edges.
The current branch order marks
optional=true+dev=true+required=falseas.required, but your own rule says it should be.optionalwhen every non-dev edge is optional.♻️ Proposed fix
- const scope: Scope = if (flags.required or (res.tag == .root or res.tag == .workspace)) + const scope: Scope = if (flags.required or (res.tag == .root or res.tag == .workspace)) .required - else if (flags.optional and !flags.dev) - .optional else if (flags.dev and !flags.optional and !flags.required) .excluded + else if (flags.optional and !flags.required) + .optional else .required;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/pm_sbom_command.zig` around lines 337 - 344, The scope classification logic in the const declaration for scope incorrectly falls through to .required when flags.optional and flags.dev are true but flags.required is false; to fix, update the branch order/conditions in the expression that sets scope (the const scope: Scope = ... using flags and res.tag) so that after the initial required check (flags.required or res.tag == .root or .workspace) you check flags.optional (e.g., else if flags.optional => .optional) before the dev-only check, then keep the dev-exclusive branch (flags.dev and !flags.optional and !flags.required => .excluded) and the fallback; adjust only the condition ordering in that const expression to ensure optional wins when every non-dev edge is optional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cli/pm_sbom_command.zig`:
- Around line 76-78: The write to stdout is silently ignored by the
`Output.writer().writeAll(output) catch {}` which can hide real failures;
replace the empty catch with proper error handling—either propagate the error
with `try Output.writer().writeAll(output);` or handle and report it (e.g., log
to stderr or return the error) inside the `pm_sbom_command` flow so write
failures are not swallowed. Ensure you update any surrounding function signature
to propagate the error if you choose `try`.
- Around line 337-344: The scope classification logic in the const declaration
for scope incorrectly falls through to .required when flags.optional and
flags.dev are true but flags.required is false; to fix, update the branch
order/conditions in the expression that sets scope (the const scope: Scope = ...
using flags and res.tag) so that after the initial required check
(flags.required or res.tag == .root or .workspace) you check flags.optional
(e.g., else if flags.optional => .optional) before the dev-only check, then keep
the dev-exclusive branch (flags.dev and !flags.optional and !flags.required =>
.excluded) and the fallback; adjust only the condition ordering in that const
expression to ensure optional wins when every non-dev edge is optional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f4efe230-88fd-4e71-a809-095c3179c381
📥 Commits
Reviewing files that changed from the base of the PR and between aa6b852c0115f0935430f92c0fefa163e4ef9efa and 832ef7f443f5da2979d83a726c1a236863c601e3.
📒 Files selected for processing (4)
src/cli/package_manager_command.zigsrc/cli/pm_sbom_command.zigsrc/install/PackageManager/CommandLineArguments.zigsrc/install/PackageManager/PackageManagerOptions.zig
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/cli/pm_sbom_command.zig (1)
76-78:⚠️ Potential issue | 🟠 MajorDo not treat every stdout write failure as success.
catch {}hides real I/O failures likeENOSPCorEBADF, so callers can get exit code 0 even though no SBOM was written. Ignoring broken pipes is fine, but other write errors should still fail the command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/pm_sbom_command.zig` around lines 76 - 78, The current Output.writer().writeAll(output) call swallows all write errors via catch {}; change it to explicitly handle broken-pipe errors only and propagate any other I/O failures so the command can fail. Replace the silent catch with a catcher that matches the write error (capture the error from Output.writer().writeAll(output) catch |err| { ... }), ignore only the broken pipe/EPIPE case (compare against std.os.EPIPE or the framework's broken-pipe error) and otherwise return or propagate the error (so the process exits non-zero); keep the write call and the output variable as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/pm_sbom_command.zig`:
- Around line 238-255: The cloned name returned by
json.getStringCloned(allocator, "name") is being duplicated again with
allocator.dupe for this.root.name, leaking the original clone; modify the branch
that sets root_name so that when json.getStringCloned(...) returns a value you
either (A) assign that owned buffer directly to this.root.name and avoid the
later allocator.dupe, or (B) if you must keep the pattern of using root_name,
free the original clone after creating the duplicate (use defer or explicit
free) so ownership is balanced; update construction of root_ref/this.root to use
the chosen owned buffer and ensure deinit() can free it unconditionally.
- Around line 329-334: The current uniqueness check uses the raw ref but
SpdxIdFormatter later sanitizes refs, so generate a sanitized SPDX id (using the
same sanitation logic as SpdxIdFormatter) for each ref (including the root
package) before inserting into seen_refs and before serialization; if the
sanitized id already exists, append a disambiguator like "~{idx}" to the
sanitized id (allocating with allocator as done for unique), free any replaced
allocations, then insert the sanitized id into seen_refs and use that sanitized
identifier for output. Apply the same fix in both places referenced (the shown
block around the ref handling and the similar block at 665-677) so collisions
are prevented after sanitization.
---
Duplicate comments:
In `@src/cli/pm_sbom_command.zig`:
- Around line 76-78: The current Output.writer().writeAll(output) call swallows
all write errors via catch {}; change it to explicitly handle broken-pipe errors
only and propagate any other I/O failures so the command can fail. Replace the
silent catch with a catcher that matches the write error (capture the error from
Output.writer().writeAll(output) catch |err| { ... }), ignore only the broken
pipe/EPIPE case (compare against std.os.EPIPE or the framework's broken-pipe
error) and otherwise return or propagate the error (so the process exits
non-zero); keep the write call and the output variable as-is.
🪄 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: cdf93f2d-f318-4f8b-b549-c3406a602a38
📥 Commits
Reviewing files that changed from the base of the PR and between 832ef7f443f5da2979d83a726c1a236863c601e3 and 8cf31778d29d083666c891929fab30eae6fd2979.
📒 Files selected for processing (1)
src/cli/pm_sbom_command.zig
8cf3177 to
26a5d1c
Compare
|
✅ No merge conflicts detected when merging into Your branch is good to go! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/pm_sbom_command.zig`:
- Around line 666-682: The current writeSPDXRelationships function (inside
writeSPDXRelationships and the relationshipType block) stops at the first
matching raw dependency entry and returns that edge's relationship, making
output input-order dependent; instead, scan all deps/resolved pairs for the
given dep_id (using pkg_dep_resolutions, pkg_deps and the for (deps, resolved)
loop) and accumulate flags for whether any matching entry is required, optional,
or dev, then pick the relationship by precedence (required -> DEPENDS_ON,
optional -> OPTIONAL_DEPENDENCY_OF, dev -> DEV_DEPENDENCY_OF); replace the early
breaks with logic that checks all matches and sets rel_type accordingly so
duplicate resolved_ids yield the correct aggregated relationship (consistent
with collectDeps/component scope).
- Around line 345-362: The current single contains() check for seen_refs and
seen_spdx_ids can still produce collisions because the first disambiguated
candidate may already exist; change both branches (the ref disambiguation and
the spdx_id disambiguation around sanitizeSpdxId) to loop-generating new
candidates until a unique value is produced (e.g., try "{s}~{d}" or "{s}.{d}"
with an incrementing suffix) and only call try seen_refs.put(ref) / try
seen_spdx_ids.put(spdx_id) once the final unique string is chosen; ensure each
intermediate allocated string (ref or spdx_id and any generated unique
candidate) is freed via allocator.free() when it is replaced to avoid leaks, and
keep using allocator, idx, seen_refs, seen_spdx_ids, sanitizeSpdxId, ref, and
spdx_id to locate the changes.
🪄 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: e48ece5c-015c-493a-a511-945a869e9037
📥 Commits
Reviewing files that changed from the base of the PR and between 8cf31778d29d083666c891929fab30eae6fd2979 and 26a5d1c.
📒 Files selected for processing (1)
src/cli/pm_sbom_command.zig
c7cfa9f to
2a72ba0
Compare
There was a problem hiding this comment.
All prior review feedback is addressed and I didn't find new issues, but this is a ~800-line new user-facing feature implementing two external SBOM specs (CycloneDX 1.7 / SPDX 2.3) with several design choices made along the way (whole-lockfile vs workspace-scoped output, BFS scope semantics, global --format/-o flags on bun pm), plus a shared test/harness.ts change — worth a human pass before merge.
Extended reasoning...
Overview
This PR adds bun pm sbom, a new CLI subcommand that generates a Software Bill of Materials from the lockfile in CycloneDX 1.7 or SPDX 2.3 JSON. The bulk of the change is the new src/cli/pm_sbom_command.zig (~790 lines) containing a Generator that walks the lockfile, runs a BFS from the root to classify each package's scope (required/optional/excluded), deduplicates bom-refs and SPDXIDs, and serializes to either format. Wiring changes in package_manager_command.zig, CommandLineArguments.zig, and PackageManagerOptions.zig are small and mechanical. A ~400-line integration test exercises both formats against a Verdaccio registry. test/harness.ts is modified to bind Verdaccio on 127.0.0.1 explicitly (affects all tests using VerdaccioRegistry, not just this one).
Security risks
No injection/auth/permission surface — the command is read-only over the lockfile and writes to a user-specified path. However, SBOMs are a supply-chain security artifact: downstream scanners (Grype, Trivy, Dependency-Track) will rely on the scope/relationship fields to decide which vulnerabilities to surface, so semantic correctness here has security implications even though the code itself isn't security-sensitive.
Level of scrutiny
This warrants human review. It is a brand-new public CLI command implementing two external specifications, and it went through ~15 rounds of bot-driven fixes during review — including two rewrites of the scope-classification logic (per-edge → BFS → BFS-from-lockfile-root), an arena-allocator refactor, and a late fix for -o cwd resolution. Several design decisions were made without human input: always describing the whole lockfile regardless of cwd (vs. a workspace-scoped SBOM), adding --format/-o to the shared bun pm clap param set, and the precise scope/relationship precedence rules. Someone familiar with how Bun wants to position this against npm sbom/cyclonedx-npm should sanity-check those choices and ideally spot-check the output against a real scanner.
Other factors
Test coverage is solid (12 tests covering both formats, scope propagation, workspaces, subdir invocation, -o resolution, error cases). All inline review threads are resolved. The test/harness.ts change is reasonable but touches shared test infrastructure used across the install test suite, which is another reason for a human glance.
023fc1c to
5826fa1
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
f01be16 to
de7358d
Compare
There was a problem hiding this comment.
I reviewed this PR and the automated pass found no bugs — all earlier nits (in-tree hex/date helpers, optional-dependency coverage, over-long -o handling, SPDX §7.7 VCS locators, the orphaned doc comment, and describe.concurrent) are addressed through 8bfa0d8. Because this adds a new user-facing subcommand with two spec-defined output formats and re-widens several bun_install items to pub, a maintainer look at the API surface would still be worthwhile.
What was reviewed: scope-propagation BFS and the Required > Optional > Excluded lattice; bom-ref/SPDXID dedup and the sanitizer charset; purl percent-encoding and vcs_locator against SPDX 2.3 §7.7; -o path resolution via join_abs_string_buf_checked; and test hermeticity (Verdaccio + local GITHUB_API_URL server, per-test temp dirs, concurrent describe).
Extended reasoning...
Overview
This PR adds bun pm sbom, a new subcommand that emits a Software Bill of Materials from the lockfile in CycloneDX 1.7 or SPDX 2.3 JSON. The core is a new ~1000-line src/runtime/cli/pm_sbom_command.rs that walks the lockfile, computes per-package scope via a strongest-path BFS, builds Component records (name/version/purl/download URL/integrity/deps), and hand-serializes both formats. It is wired into the pm dispatcher, CLI argument parser, options struct, help text, docs, and shell completions. To read lockfile internals cross-crate it widens Resolution::repository(), the Resolution::Tag constants, Integrity::slice() + its SHA tags, and Meta.integrity from pub(crate) to pub. shell/builtin/ls.rs::civil_from_days becomes pub(crate) so the timestamp helper can be reused rather than duplicated. A 486-line test file exercises both formats via Verdaccio, workspaces, git/github deps served locally, error paths, and CLI edge cases.
Security risks
Low. The command reads a trusted local lockfile and emits JSON; there is no network I/O in the command itself. The one user-controlled write path (-o <path>) now uses join_abs_string_buf_checked and exits cleanly on overflow rather than panicking. JSON string values are escaped via bun_core::fmt::format_json_string_utf8, so lockfile-derived names/URLs cannot break out of string context. purl encoding percent-escapes outside the RFC 3986 unreserved set. No auth, crypto, or permission logic is touched.
Level of scrutiny
High — this is a new user-facing API surface. Per the repo's landing-PR guidance, API additions call for maintainer agreement on shape: whether CycloneDX 1.7 / SPDX 2.3 are the right targets, whether the flag spellings (--format, -o/--outfile) match sibling commands, whether the pub(crate)→pub widenings in bun_install are acceptable long-term, and whether hand-rolled JSON serialization is preferred over a schema-checked emitter. The implementation itself has been through several review iterations and looks sound, but these are design calls a bot should not sign off on unilaterally.
Other factors
Test coverage is solid after the review-driven additions: required/optional/excluded scopes, transitive-dev vs mixed prod+dev paths, scoped-package purl encoding, workspaces (including running from a subdir with relative -o), git+file:// and github: deps via a local tarball server, unknown --format, missing lockfile, over-long -o, and the help-text entry. Tests are hermetic (no public network) and now run under describe.concurrent. The finder-flagged "ported from" trailer comment referencing a deleted Zig file is cosmetic and was ruled out. All prior inline threads on this PR are resolved.
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues; all earlier review threads have been addressed. Since this adds a new user-facing CLI command (bun pm sbom) with ~1000 lines of new Rust plus visibility widening in bun_install, a maintainer sign-off on the API surface and SBOM format choices would still be worthwhile.
Checked: scope-propagation BFS and SPDX *_OF edge direction against the test assertions; -o path handling now uses join_abs_string_buf_checked (no panic on long argv); git/github downloadLocation now emits SPDX §7.7 VCS locators; hex/timestamp/percent-encoding reuse in-tree bun_core::fmt and civil_from_days; visibility changes in integrity.rs/resolution.rs/Meta.rs are read-only accessors matching what the command consumes.
Extended reasoning...
Overview
This PR adds bun pm sbom, a new CLI subcommand that reads the lockfile and emits a Software Bill of Materials in CycloneDX 1.7 or SPDX 2.3 JSON. The core is a ~1000-line new file src/runtime/cli/pm_sbom_command.rs implementing a Generator that walks the lockfile's package list, computes per-package scope via BFS from the root, builds Component records (bom-ref, SPDXID, purl, download URL, integrity hash, dep edges), and hand-serializes both output formats. Supporting changes wire the subcommand into package_manager_command.rs, add --format/--outfile options to CommandLineArguments/PackageManagerOptions, widen pub(crate) → pub on the Resolution tags, Resolution::repository(), Integrity::slice()/tag constants, and Meta.integrity so the runtime crate can read them, expose civil_from_days from shell/builtin/ls.rs as pub(crate), and add docs/completions/help text. A 486-line Verdaccio-backed test file covers both formats, scope propagation (required/optional/excluded, mixed-path), workspaces, git/github locators, -o handling, and error cases.
Security risks
Low. The command is read-only against the lockfile and writes only to the user-specified -o path (or stdout). Input surface is CLI argv and lockfile contents. The -o path now goes through join_abs_string_buf_checked and exits cleanly on overflow rather than panicking. JSON output escapes all lockfile-derived strings via bun_core::fmt::format_json_string_utf8, and SPDXID generation sanitizes to [A-Za-z0-9.-] with collision-dedup. No network, no auth, no crypto beyond reading the stored integrity digest. The vcs_locator helper reads repo.resolved/repo.committish from the lockfile string buffer with bounds derived from stored String handles. The visibility widening exposes only read-only data (union tag constants, a slice getter, a Copy field) — no new mutation surface.
Level of scrutiny
This warrants human review. It is a net-new user-facing feature with API-design decisions a maintainer should ratify: the CLI flag names (--format, -o/--outfile), the default format (CycloneDX 1.7), the SBOM field mappings (e.g. dev deps → scope: "excluded" / DEV_DEPENDENCY_OF, whole-lockfile scope even from a workspace subdir), and the choice to hand-serialize JSON rather than use a schema library. REVIEW.md's referenced landing-prs.md specifically calls out API design as requiring a targeted read before adding user-facing surface. The visibility widening in bun_install is a small architectural change that a maintainer should acknowledge. None of this is risky per se — it's additive, well-tested, and follows the sibling-command patterns (pm_why_command.rs, audit_command.rs) — but ~1500 net-new lines of feature code is beyond the "simple/mechanical/obvious" bar for auto-approval.
Other factors
The PR has been through many rounds of automated review over its lifetime (the timeline shows dozens of resolved threads from CodeRabbit, comment-cop, and prior bug-hunter passes), and every finding from earlier runs of this reviewer — optional-dep coverage, in-tree helper reuse, -o overflow, SPDX VCS-locator syntax, orphaned doc comment, describe.concurrent — was addressed in follow-up commits and resolved. Test coverage is solid: 14 concurrent Verdaccio-backed tests exercising both formats, all three scope states, transitive-dev vs mixed-path, workspaces from root and subdir, git+file:// and github: (via local GITHUB_API_URL), -o relative/absolute/overlong, unknown --format, and missing lockfile. No human reviewer has commented on the PR.
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues; all earlier inline findings have been addressed. Because this adds a new user-facing bun pm subcommand (~1000 lines of new Rust plus ~500 lines of tests) and makes API-surface choices — CycloneDX 1.7 vs SPDX 2.3, purl/VCS-locator formatting, scope semantics, and re-widening several bun_install items to pub — a maintainer sign-off on the design would still be worthwhile.
What was reviewed:
- Scope-propagation BFS and SPDX relationship direction (
*_OFsubject/object ordering) — covered by tests for required/optional/excluded and mixed prod+dev paths. - JSON serialization: all user-derived strings go through
json_str/format_json_string_utf8; SPDXIDs are sanitized and independently deduplicated from bom-refs. -opath handling usesjoin_abs_string_buf_checked(no panic on over-long argv); write errors exit non-zero.- Visibility widenings in
integrity.rs/resolution.rs/Meta.rsare read-only accessors and tag constants; no new mutation surface.
Extended reasoning...
Overview
This PR adds bun pm sbom, a new CLI subcommand that reads the lockfile and emits a Software Bill of Materials in CycloneDX 1.7 or SPDX 2.3 JSON. The bulk of the change is a new file src/runtime/cli/pm_sbom_command.rs (~1000 lines) plus wiring in package_manager_command.rs, CLI argument plumbing in CommandLineArguments.rs/PackageManagerOptions.rs, help text, docs, shell completions, and a 500-line Verdaccio-backed test file. It also widens a handful of bun_install items from pub(crate) to pub (the Resolution::Tag constants, Resolution::repository(), Integrity::slice() and its tag constants, Meta.integrity) so the new command can read them cross-crate, and makes civil_from_days in shell/builtin/ls.rs pub(crate) for reuse.
Security risks
Low. The command is read-only against the lockfile and writes either to stdout or to a user-specified file. All strings interpolated into the JSON output (package names, versions, URLs, refs) are routed through bun_core::fmt::format_json_string_utf8, so lockfile content cannot break out of a JSON string. SPDXIDs are sanitized to [A-Za-z0-9.-] and independently deduplicated. The -o path is joined against the original cwd via join_abs_string_buf_checked and errors cleanly on overflow rather than panicking. No network, no auth, no untrusted-input parsing beyond what PackageManager::init already does.
Level of scrutiny
High — this is a new user-facing CLI subcommand with externally-specified output formats. Per the repo guidance, adding user-facing API surface calls for the API-design section of .claude/docs/landing-prs.md and maintainer agreement on the shape (flag names, default format, spec versions, whether --format/-o on bun pm collide with future subcommands). The implementation itself is self-contained and well-tested, but the design choices — CycloneDX 1.7 as default, SPDX 2.3 field selection, NOASSERTION for licenses, scope: excluded for dev deps, VCS-locator formatting — are the kind of thing a maintainer should ratify before they become a compatibility surface for downstream SBOM consumers.
Other factors
The PR has been through many review iterations (all inline threads resolved): optional-dependency coverage, in-tree helper reuse (hex_lower, hex_byte_upper, civil_from_days), -o path-length hardening, SPDX §7.7-conformant VCS locators, describe.concurrent with per-test cache/tmp isolation, and dead-comment cleanup. Test coverage is thorough (14 tests: both formats, purl encoding, scope propagation including transitive-dev and mixed paths, workspaces from root and subdir, git+github locators via a local server, error paths). This run's bug-hunting pass found nothing. The remaining reason to defer is scope, not correctness concerns.
f04bc62 to
7f4d005
Compare
Generates a Software Bill of Materials from the lockfile in either CycloneDX 1.7 (default) or SPDX 2.3 JSON format. bun pm sbom # CycloneDX to stdout bun pm sbom --format spdx -o sbom.json # SPDX to file For each package in the lockfile the SBOM includes the package name, resolved version, purl identifier, download location (registry tarball URL, or a git+<url>@<revision> VCS locator for git and github packages), and integrity hash. Dependency relationships are emitted as the CycloneDX `dependencies` graph or SPDX `relationships`. Scope classification is computed via BFS from the lockfile root: a package's scope is the strongest (required > optional > excluded) over all paths that reach it, where a path's scope is the weakest edge along it, so transitive deps of a root devDependency are `excluded`, but a package reachable via both a dev path and a prod path is `required`. This matches what `bun install --production` would actually install. The SBOM always describes the whole lockfile, regardless of which workspace subdirectory the command is run from. Relative `-o` paths resolve against the invocation directory. Re-widens to `pub` the bun_install items this command reads cross-crate (the remaining Resolution tags, Resolution::repository, Integrity::slice and its SHA1/384/512 tags, Meta.integrity), which #36184 had narrowed because nothing outside the crate used them at the time, and exposes the shell's civil_from_days within the runtime crate for the timestamp. Closes #8483.
7f4d005 to
36ae2ab
Compare
There was a problem hiding this comment.
The rebase regression I flagged on the licenses dispatch arm is fixed — both licenses and sbom now terminate with their own Global::exit(0). This pass found no further issues. Given the size (~1000 lines of new Rust implementing a user-facing bun pm sbom subcommand against two external specs, plus visibility widening across bun_install), a human sign-off on the API surface and SBOM spec conformance is still worthwhile.
What was reviewed: the package_manager_command.rs dispatch chain (verified licenses/sbom/pkg each exit correctly); scope-propagation BFS and ref/SPDXID dedup in Generator::init; vcs_locator against SPDX §7.7; the -o path handling (checked variant, pooled buffer). Ruled out: test temp-dir leakage via newProject() (verifiers confirmed harmless).
Extended reasoning...
Overview
This PR adds bun pm sbom, a new subcommand that generates CycloneDX 1.7 or SPDX 2.3 SBOM documents from the lockfile. The bulk of the change is a new ~1000-line file src/runtime/cli/pm_sbom_command.rs plus ~500 lines of Verdaccio-backed integration tests. Supporting changes wire the subcommand into CLI parsing, help text, docs, and completions, and re-widen a handful of bun_install items (Resolution::repository(), Integrity::slice(), several Tag consts, Meta.integrity) from pub(crate) to pub so the runtime crate can read them. One incidental change makes civil_from_days in shell/builtin/ls.rs pub(crate) for reuse.
Prior finding resolved
My previous review (2026-08-14) flagged a 🔴 rebase mistake where the new sbom arm was inserted between PmLicensesCommand::exec(...)? and its Global::exit(0), causing bun pm licenses to fall through to the unknown-command handler. The current diff shows this is fixed: the licenses arm at package_manager_command.rs:754-756 now ends with its own Global::exit(0) before the sbom arm begins, and sbom has its own terminator. All earlier nits (SPDX VCS locator format, join_abs_string_buf_checked, orphaned doc comment, describe.concurrent, in-tree helper reuse) were also addressed in prior commits and remain in place.
Security risks
Low. The command reads the lockfile and package.json from the workspace root and writes JSON to stdout or a user-specified file. Output-path handling uses the checked path-join variant with a clean error on overflow. The SBOM output includes package names, versions, tarball URLs, and integrity hashes — all already present in bun.lock. No network I/O, no untrusted input parsing beyond the lockfile Bun already trusts.
Level of scrutiny
Moderate-to-high. This is a new user-facing CLI subcommand implementing two external specifications (CycloneDX 1.7 JSON schema, SPDX 2.3). The JSON is hand-serialized line-by-line rather than via a structured serializer, so field ordering, escaping, and comma placement are all manual. The test suite validates the required schema fields for both formats, but spec conformance for edge cases (unusual package names, git URL shapes, purl encoding subtleties) is the kind of thing a maintainer familiar with SBOM tooling should eyeball. The visibility widening on bun_install types is minimal and read-only, but reverses part of #36184's narrowing and should be acknowledged.
Other factors
The PR has been through ~10 rounds of automated review with every finding addressed. Test coverage is solid: both formats, scoped packages, workspaces, git+github deps via a local server, dev/optional/transitive scope propagation, error paths, and the -o path-too-long guard. The bug-hunting pass this run raised only two candidate findings about test temp-dir cleanup, both refuted by verifiers. I'm deferring rather than approving because a ~1500-line new feature that defines public CLI surface and claims conformance to two external standards is exactly the case where a human should confirm the API shape and output format before it ships.
2 participants
Adds
bun pm sbom, which generates a Software Bill of Materials from the lockfile.Usage
What's in the SBOM
For every package in the lockfile:
name,versionname,versionInfobom-ref(e.g.lodash@4.17.21)SPDXID(SPDXRef-Package-lodash-4.17.21)purl(pkg:npm/lodash@4.17.21, scopes %-encoded)externalRefs[].referenceLocatorexternalReferences[].urldownloadLocationhashes[](SHA-512hex)checksums[](SHA512hex)dependencies[].dependsOnrelationships[]Registry packages use the tarball URL as the download location; git and github packages use an SPDX 2.3 VCS locator (
git+<url>@<revision>). Dev dependencies are marked withscope: "excluded"(CycloneDX) /DEV_DEPENDENCY_OF(SPDX); optional dependencies usescope: "optional"/OPTIONAL_DEPENDENCY_OF. Workspaces, folders, git/github, and tarball resolutions are all represented.The root component is emitted under
metadata.component(CycloneDX) and as thedocumentDescribestarget (SPDX), with Bun listed as the generating tool.Example (CycloneDX, abridged)
{ "bomFormat": "CycloneDX", "specVersion": "1.7", "serialNumber": "urn:uuid:...", "metadata": { "timestamp": "2026-04-20T12:34:56Z", "tools": { "components": [{ "type": "application", "name": "bun", "version": "1.x.x" }] }, "component": { "type": "application", "bom-ref": "my-app@1.0.0", "name": "my-app", "version": "1.0.0" } }, "components": [ { "type": "library", "bom-ref": "lodash@4.17.21", "name": "lodash", "version": "4.17.21", "scope": "required", "purl": "pkg:npm/lodash@4.17.21", "externalReferences": [{ "type": "distribution", "url": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz" }], "hashes": [{ "alg": "SHA-512", "content": "bf690311..." }] } ], "dependencies": [ { "ref": "my-app@1.0.0", "dependsOn": ["lodash@4.17.21"] }, { "ref": "lodash@4.17.21", "dependsOn": [] } ] }Tests
test/cli/install/bun-pm-sbom.test.tsuses the Verdaccio test registry to install real packages, then validates both output formats against the required fields from the CycloneDX 1.7 and SPDX 2.3 JSON schemas (serial number pattern, timestamp format, purl encoding, hash hex length, SPDXID charset, dependency graph integrity, relationship types).Fixes #8483
Rebase notes
src/was restructured and Zig left the build (src/runtime/cli/pm_sbom_command.rs, followingaudit_command.rs/pm_why_command.rs).bun_installvisibility, this PR re-widens topubexactly the items it reads cross-crate: the remainingResolutiontags,Resolution::repository(),Integrity::sliceand its SHA1/384/512 tags, andMeta.integrity.test/harness.tsVerdaccio127.0.0.1bind fix landed on main independently and is no longer part of this diff.bun pm licenseslanded alongside; the conflicts were keep-both in thepmargument struct, dispatch chain and docs page. The missing-lockfile test now matches the reworded shared error (missing lockfile). The first push of that rebase dropped theGlobal::exit(0)at the end of thelicensesdispatch arm (CI caught it viabun-pm-licenses.test.ts); it is restored, and the dispatch chain now differs from main only by the addedsbomarm.no test proof · iteration 13 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-pm-sbom.test.ts