Repository navigation
bundle: reject failed uv inventories - #24363
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes.
|
| Layer / File(s) | Summary |
|---|---|
Run and validate uv inventory Library/Homebrew/bundle/extensions/uv.rb, Library/Homebrew/test/bundle/uv_spec.rb |
The extension obtains tool-list output through SystemCommand.run! and returns cached inventory results. Specs cover parsing, successful and failed command results, cache resets and retries, and Brewfile behaviour when inventory retrieval fails. |
Priority:
Merge Risk: 
Minimal · previous review 5f70c
-
No concrete merge-blocking risk remains in the reviewed change.
Pre-merge checks: 
4
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the uv inventory failure-handling change, including its retry, caching, and Brewfile-safety concerns.
No open issues 🎉
What changed in this PR
This pull request makes brew bundle dump --uv reject failed uv tool list commands, preventing incomplete inventories from being parsed, cached, or written to Brewfiles.
Changes:
- Uses
SystemCommand.run!to preserve uv failure status and diagnostics. - Avoids caching failed inventories so later requests can retry.
- Adds regression coverage for partial and empty failures, retries, caching, and Brewfile preservation.
- Adds the required
system_commanddependency before merging.
The unresolved SystemCommand require issue must be addressed before approval.
| File | Summary |
|---|---|
Library/Homebrew/test/bundle/uv_spec.rb |
Updates inventory stubs and adds coverage for failed inventories, retries, caching behavior, and Brewfile safety. |
Library/Homebrew/bundle/extensions/uv.rb |
Uses SystemCommand.run! for uv inventory execution; should explicitly require system_command to support extension-only loading. |
This pull request requires changes.
Review effort: Lite
e9aa033 to
5f70c66
Compare
5f70c66 to
70c9348
Compare
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! Could you take a look and see if any other extension types are affected by the same issue?
Yes, I reproduced this during forced dumps for Cargo, Krew, mas, VS Code, Go, Flatpak, npm and WinGet. For the first seven, Bundle trusts whatever the failed command printed. For WinGet, it notices the failure but treats it as "no packages installed." Bundle then remembers that incorrect result and reuses it instead of trying again. Fixing Go and npm requires extra care because their commands sometimes return a nonzero exit code for an expected result, so rejecting every nonzero code would break valid behavior. My plan is to have a PR per extension to control PR size. I already have a draft of the fix for the Go extension in the works. Would you prefer I do a larger PR, or stick to my current plan? |
|
@edel67 larger PR for them all please! May suggest opportunities for how to DRY things up a bit. Thanks! |
When
uv tool listfails, Bundle treats its empty or partial stdout as a valid inventory, sobrew bundle dump --forcecan overwrite an existing Brewfile with an incomplete list of tools.Use
SystemCommand.run!to raise with uv's exit status and diagnostics before parsing or caching the output. This prevents the write and allows a later inventory request to retry.To reproduce from a Homebrew/brew checkout with a disposable uv executable:
Before the fix, dump succeeds and replaces the file with
uv "ruff". With the fix, dump fails with uv's diagnostic and leavesuv "existing-tool"intact../bin/brew lgtm --onlinepassed locally on macOS (68 uv examples), including tests that failed inventories neither overwrite nor create a Brewfile. The broader Bundle/dump suites also passed (483 examples). Linux and remote CI remain untested.brew benchmarkresults.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?OpenAI Codex (GPT-6) assisted with drafting the tests. I reviewed every draft myself and expanded coverage or revised the tests as I thought necessary.