feat: implement bun update --recursive
#23555
Conversation
WalkthroughWorkspace discovery, filtering, and package.json path construction were extracted to a new Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (2)**/*.zig📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
src/**/*.zig📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-10-07T03:19:53.031ZApplied to files:
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}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/cli/outdated_command.zig(1 hunks)src/cli/update_interactive_command.zig(1 hunks)src/install/PackageManager/WorkspaceHelpers.zig(1 hunks)src/install/PackageManager/updatePackageJSONAndInstall.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/cli/update_interactive_command.zigsrc/install/PackageManager/WorkspaceHelpers.zigsrc/install/PackageManager/updatePackageJSONAndInstall.zigsrc/cli/outdated_command.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/cli/update_interactive_command.zigsrc/install/PackageManager/WorkspaceHelpers.zigsrc/install/PackageManager/updatePackageJSONAndInstall.zigsrc/cli/outdated_command.zig
🔇 Additional comments (4)
src/cli/outdated_command.zig (1)
616-619: Aliasing helpers here is good; check filter semantics parityDelegating to WorkspaceHelpers reduces duplication. Please verify that findMatchingWorkspaces preserves prior matching semantics (any-of vs all-of across multiple filters), and that glob/path matching behaves on Windows as before.
src/cli/update_interactive_command.zig (1)
534-536: LGTM: centralizing workspace helpersAliases keep interactive flow consistent with the new shared logic.
src/install/PackageManager/WorkspaceHelpers.zig (2)
14-35: getAllWorkspaces: OKCollecting root + workspace PackageIDs and returning an owned slice looks correct.
37-122: Remove outdated ownership suggestion; confirm filter semantics
- Ownership:
.toOwnedSlice(allocator)is already used—no change needed.- Semantics: filters are currently ANDed; confirm if they should be ORed instead.
ShlomoCode
force-pushed
the
feat/update-recursive
branch
from
fd32a2c to
195e23c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/install/PackageManager/updatePackageJSONAndInstall.zig (1)
272-283: Single matched workspace still skippedThe regression called out earlier remains: when filters match exactly one workspace (and
--recursiveis not set) we still fall through to the root-only path, so that workspace never gets updated. Please execute the previously suggested fix to always delegate to the workspace-aware path whenever any match exists.- if (workspace_pkg_ids.len > 1 or (workspace_pkg_ids.len == 1 and manager.options.do.recursive)) { - defer bun.default_allocator.free(workspace_pkg_ids); - return try updatePackageJSONForWorkspaces( - manager, - ctx, - original_cwd, - workspace_pkg_ids, - ); - } else if (workspace_pkg_ids.len > 0) { - bun.default_allocator.free(workspace_pkg_ids); - } + if (workspace_pkg_ids.len > 0) { + defer bun.default_allocator.free(workspace_pkg_ids); + return try updatePackageJSONForWorkspaces( + manager, + ctx, + original_cwd, + workspace_pkg_ids, + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/cli/outdated_command.zig(1 hunks)src/cli/update_interactive_command.zig(1 hunks)src/install/PackageManager/WorkspaceHelpers.zig(1 hunks)src/install/PackageManager/updatePackageJSONAndInstall.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/PackageManager/updatePackageJSONAndInstall.zigsrc/cli/outdated_command.zigsrc/cli/update_interactive_command.zigsrc/install/PackageManager/WorkspaceHelpers.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/PackageManager/updatePackageJSONAndInstall.zigsrc/cli/outdated_command.zigsrc/cli/update_interactive_command.zigsrc/install/PackageManager/WorkspaceHelpers.zig
🧠 Learnings (1)
📚 Learning: 2025-10-07T03:19:53.031Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.031Z
Learning: Applies to src/**/*.zig : Place import directives at the bottom of Zig files
Applied to files:
src/install/PackageManager/WorkspaceHelpers.zig
ShlomoCode
force-pushed
the
feat/update-recursive
branch
from
195e23c to
05aac39
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/install/PackageManager/updatePackageJSONAndInstall.zig (1)
238-249: Bug: single matched workspace with filters is not updated.If exactly one workspace matches filters (and
--recursiveis not set), the condition on line 238 evaluates to false:if (workspace_pkg_ids.len > 1 or (workspace_pkg_ids.len == 1 and manager.options.do.recursive))This causes the code to free the IDs and fall through to the non-workspace path, skipping the intended workspace update.
Apply this diff to fix the issue:
- if (workspace_pkg_ids.len > 1 or (workspace_pkg_ids.len == 1 and manager.options.do.recursive)) { - defer bun.default_allocator.free(workspace_pkg_ids); - return try updatePackageJSONForWorkspaces( - manager, - ctx, - original_cwd, - workspace_pkg_ids, - ); - } else if (workspace_pkg_ids.len > 0) { - bun.default_allocator.free(workspace_pkg_ids); - } + if (workspace_pkg_ids.len > 0) { + defer bun.default_allocator.free(workspace_pkg_ids); + return try updatePackageJSONForWorkspaces( + manager, + ctx, + original_cwd, + workspace_pkg_ids, + ); + }src/install/PackageManager/WorkspaceHelpers.zig (2)
7-10: Critical: Return allocator-owned empty slices to prevent invalid free.
getAllWorkspacesreturns a static zero-length slice when no packages exist. Callers (e.g., inupdatePackageJSONAndInstall.zigat line 233) immediately free the result withbun.default_allocator.free(workspace_pkg_ids), which will trigger an invalid-free panic because the pointer was never allocated by the allocator.Apply this diff to fix the issue:
if (lockfile.packages.len == 0) { - const empty: []const PackageID = &[_]PackageID{}; - return empty; + return allocator.alloc(PackageID, 0); }
63-109: Critical: Transfer ownership properly before returning.
findMatchingWorkspacesreturnsworkspace_pkg_ids.itemsat line 108, leaving the backing buffer owned by the localArrayListUnmanaged. When callers free the slice using its post-filter length (which may differ from the allocated capacity), this yields undefined behavior and potential leaks when no matches remain.Apply this diff to transfer ownership properly:
while (i < workspace_pkg_ids.items.len) { ... } - return workspace_pkg_ids.items; + const result = try workspace_pkg_ids.toOwnedSlice(allocator); + if (result.len == 0) { + return allocator.alloc(PackageID, 0); + } + return result;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/cli/outdated_command.zig(1 hunks)src/cli/update_interactive_command.zig(5 hunks)src/install/PackageManager/WorkspaceHelpers.zig(1 hunks)src/install/PackageManager/updatePackageJSONAndInstall.zig(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/PackageManager/updatePackageJSONAndInstall.zigsrc/cli/update_interactive_command.zigsrc/cli/outdated_command.zigsrc/install/PackageManager/WorkspaceHelpers.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/PackageManager/updatePackageJSONAndInstall.zigsrc/cli/update_interactive_command.zigsrc/cli/outdated_command.zigsrc/install/PackageManager/WorkspaceHelpers.zig
🧠 Learnings (1)
📚 Learning: 2025-10-07T03:19:53.031Z
Learnt from: CR
PR: oven-sh/bun#0
File: src/CLAUDE.md:0-0
Timestamp: 2025-10-07T03:19:53.031Z
Learning: Applies to src/**/*.zig : Place import directives at the bottom of Zig files
Applied to files:
src/install/PackageManager/WorkspaceHelpers.zig
🔇 Additional comments (7)
src/cli/outdated_command.zig (1)
616-619: LGTM! Clean delegation to WorkspaceHelpers.The aliases properly delegate workspace discovery to the centralized WorkspaceHelpers module, and imports are correctly placed at the bottom per Zig coding guidelines.
src/cli/update_interactive_command.zig (2)
158-288: LGTM! Consistent use of centralized path helper.The migration to
buildWorkspacePackageJsonPathis implemented consistently across all call sites, properly accessing the.pathand.path_zfields from the returned struct.
1904-1908: LGTM! Clean WorkspaceHelpers integration.The imports and aliases follow Zig coding guidelines with imports at the bottom, and the aliasing pattern is consistent with other command files.
src/install/PackageManager/updatePackageJSONAndInstall.zig (2)
1-161: Verify correctness of two-pass workspace update logic.The function processes workspace package.jsons twice with different
exact_versionssettings:
- First pass (lines 13-90):
exact_versions = true,before_install = true- Install step (line 104)
- Second pass (lines 106-160):
exact_versions = manager.options.enable.exact_versionsPlease confirm this two-pass approach is intentional and that the second pass is necessary after the install completes. If the second pass is only meant to revert the exact_versions behavior when the flag is off, consider documenting this workflow with inline comments explaining the rationale.
983-989: LGTM! Proper WorkspaceHelpers integration.The imports and aliases are correctly structured, enabling workspace-aware update logic.
src/install/PackageManager/WorkspaceHelpers.zig (2)
111-138: LGTM! Path helper function is correct.The
buildWorkspacePackageJsonPathfunction properly constructs absolute paths and returns both regular and zero-terminated slices, facilitating clean integration with filesystem APIs.
140-151: LGTM! Imports correctly placed at the bottom.The imports follow the Zig coding guideline of placing
@importdirectives at the bottom of the file.As per coding guidelines
ShlomoCode
force-pushed
the
feat/update-recursive
branch
2 times, most recently
from
43dd937 to
e38d4e6
Compare
ShlomoCode
force-pushed
the
feat/update-recursive
branch
from
e38d4e6 to
6e4790c
Compare
ShlomoCode
changed the title
update --recursivebun update --recursive
1 participant
What does this PR do?
Closes #23507
How did you verify your code works?
I tried with the example monorepo provided in the issue (https://github.com/therealsamyak/bun-monorepo)