Skip to content

bundle: reject failed uv inventories - #24363

Merged
MikeMcQuaid merged 1 commit into
Homebrew:mainfrom
edel67:uv-inventory-failures
Oct 27, 2026
Merged

MikeMcQuaid merged 1 commit into
Homebrew:mainfrom
edel67:uv-inventory-failures

Conversation

@edel67

@edel67 edel67 commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

When uv tool list fails, Bundle treats its empty or partial stdout as a valid inventory, so brew bundle dump --force can 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:

uv_probe_dir="$(mktemp -d)"
cat > "$uv_probe_dir/uv" <<'SH'
#!/bin/sh
printf '%s\n' 'ruff v0.14.14'
printf '%s\n' 'inventory unavailable' >&2
exit 2
SH
chmod +x "$uv_probe_dir/uv"
printf '%s\n' 'uv "existing-tool"' > "$uv_probe_dir/Brewfile"
PATH="$uv_probe_dir:$PATH" HOMEBREW_NO_AUTO_UPDATE=1 \
  ./bin/brew bundle dump --uv --force --file="$uv_probe_dir/Brewfile"
cat "$uv_probe_dir/Brewfile"
rm -r "$uv_probe_dir"

Before the fix, dump succeeds and replaces the file with uv "ruff". With the fix, dump fails with uv's diagnostic and leaves uv "existing-tool" intact.

./bin/brew lgtm --online passed 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.


  • Have you followed our Contributing guidelines?
  • Have you checked for other open Pull Requests for the same change?
  • Have you explained what your changes do? Performance claims (e.g. "this is faster") must include brew benchmark results.
  • Have you explained why you'd like these changes included, not just what they do?
  • For bug fixes, have you given step-by-step brew commands to reproduce the bug?
  • Have you written new tests (excluding integration tests)? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) locally?

  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

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.


Copilot AI lite review requested due to automatic review settings October 26, 2026 20:34
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 8726bc9c-956e-47cd-be4c-2ec573b51220
Commits

Reviewing files that changed from the base of the PR and between 5f70c66 and 70c9348.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3e85e00d-a6bc-4965-8182-ea70522fa242
Commits

Reviewing files that changed from the base of the PR and between e9aa033 and 5f70c66.

Files selected for processing: 1
  • Library/Homebrew/bundle/extensions/uv.rb

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Package inventories now return an empty result when the package-manager executable is unavailable.
    • Inventory command failures are reported as errors, including when a command prints output before failing.
    • Brewfile dumps no longer create or overwrite files when inventory retrieval fails.
    • Failed inventory checks can be retried, while successful results are cached for reuse.
    • Inventory parsing uses standard output, supports UTF-8 output and ignores error output when commands succeed.

The uv bundle extension now runs uv tool list through SystemCommand.run! and caches inventory results. The specs cover output parsing, cache behaviour, command failures, and Brewfile dump outcomes.

Changes: uv inventory handling

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: Normal

Merge Risk: Minimal · previous review 5f70c

No concrete merge-blocking risk remains in the reviewed change.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: failed uv inventories are rejected.
Description check Passed The description directly explains the bug, the fix, the expected behaviour, reproduction steps, and test results.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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_command dependency 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

@edel67
edel67 force-pushed the uv-inventory-failures branch 2 times, most recently from e9aa033 to 5f70c66 Compare October 26, 2026 21:26
@edel67
edel67 force-pushed the uv-inventory-failures branch from 5f70c66 to 70c9348 Compare October 27, 2026 04:42

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Could you take a look and see if any other extension types are affected by the same issue?

@MikeMcQuaid
MikeMcQuaid added this pull request to the merge queue Oct 27, 2026
Merged via the queue into Homebrew:main with commit 4a2b2f2 Oct 27, 2026
53 checks passed
@edel67
edel67 deleted the uv-inventory-failures branch October 27, 2026 09:22
@edel67

edel67 commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

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?

@MikeMcQuaid

Copy link
Copy Markdown
Member

@edel67 larger PR for them all please! May suggest opportunities for how to DRY things up a bit. Thanks!

@edel67 edel67 mentioned this pull request Nov 1, 2026
8 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

⌂ 换网址