fix(ujust): install fzf before choosing recipes - #862
Conversation
Install the Homebrew-managed chooser on demand when --choose runs before brew-preinstall completes. Assisted-by: GPT-5 via pi
📝 WalkthroughWalkthroughThe Changesujust fzf bootstrap
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ujust
participant Homebrew
participant just
User->>ujust: invoke --choose
ujust->>Homebrew: install fzf
Homebrew-->>ujust: provide shell environment
ujust->>just: execute with fixed justfile and arguments
Possibly related issues
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
tests/test_ujust.bats (2)
12-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the mock prove that installed
fzfis usable.The fake installer only records the command, and fake
justnever checks for fzf. These tests therefore pass even if the installation orshellenvsetup fails and the real chooser would still exit 127. Create a fake fzf in a separate Homebrew prefix duringbrew install fzf, add that prefix throughshellenv, and make mockjustfail whencommand -v fzfis unavailable. Also assert the logged order: install → shellenv → just.Also applies to: 32-40
🤖 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 `@tests/test_ujust.bats` around lines 12 - 23, Update the mock brew and just scripts in the test setup so brew install fzf creates an executable fzf under a separate Homebrew prefix and brew shellenv exposes that prefix in PATH; make mock just verify command -v fzf succeeds and fail otherwise. Extend the test assertions to require the call order install → shellenv → just.
32-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the negative bootstrap branches.
Add cases for fzf already being present, Homebrew being absent, and
brew install fzffailing. These branches have explicit production behavior but are currently untested.🤖 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 `@tests/test_ujust.bats` around lines 32 - 50, Add tests in tests/test_ujust.bats covering the --choose bootstrap branches: verify an existing fzf skips installation, missing Homebrew follows the production fallback/error behavior, and a failing brew install fzf is handled as expected. Reuse the existing PATH, MOCK_BIN, CALLS, and assertions from the current ujust tests while confirming no unintended command execution occurs.
🤖 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 `@system_files/shared/usr/bin/ujust`:
- Line 3: Update the argument check in the ujust bootstrap condition to iterate
over "$@" and match an argument exactly equal to --choose, rather than searching
the joined $* string. Preserve the existing fzf availability check and bootstrap
behavior when --choose is passed as its own argument.
- Around line 17-20: Update the brew shell initialization around eval
"${BREW_BIN}" shellenv to capture its output and propagate any shellenv failure
before dispatching just. After applying the environment, validate that command
-v fzf succeeds, and abort with a clear error if it is unavailable; only then
execute just with the existing arguments.
In `@tests/test_ujust.bats`:
- Around line 33-35: Update the test invocations using --choose in
tests/test_ujust.bats to set PATH exclusively to the mock directory, removing
/usr/bin:/bin from both affected run commands. Preserve the existing WORKDIR,
CALLS, MOCK_BIN, and absolute bash execution setup so the tests consistently
exercise bootstrap behavior.
---
Nitpick comments:
In `@tests/test_ujust.bats`:
- Around line 12-23: Update the mock brew and just scripts in the test setup so
brew install fzf creates an executable fzf under a separate Homebrew prefix and
brew shellenv exposes that prefix in PATH; make mock just verify command -v fzf
succeeds and fail otherwise. Extend the test assertions to require the call
order install → shellenv → just.
- Around line 32-50: Add tests in tests/test_ujust.bats covering the --choose
bootstrap branches: verify an existing fzf skips installation, missing Homebrew
follows the production fallback/error behavior, and a failing brew install fzf
is handled as expected. Reuse the existing PATH, MOCK_BIN, CALLS, and assertions
from the current ujust tests while confirming no unintended command execution
occurs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81cc79c3-f551-4da4-a786-266f94fdd0e4
📒 Files selected for processing (2)
system_files/shared/usr/bin/ujusttests/test_ujust.bats
| #!/usr/bin/bash | ||
|
|
||
| just --justfile /usr/share/ublue-os/just/00-entry.just "${@}" | ||
| if [[ " ${*} " == *" --choose "* ]] && ! command -v fzf >/dev/null 2>&1; then |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match --choose as an actual argument.
Joining arguments through $* means an argument such as "recipe --choose" also triggers the bootstrap, even though --choose was not passed as its own option. Iterate over "$@" and compare each argument exactly.
🤖 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 `@system_files/shared/usr/bin/ujust` at line 3, Update the argument check in
the ujust bootstrap condition to iterate over "$@" and match an argument exactly
equal to --choose, rather than searching the joined $* string. Preserve the
existing fzf availability check and bootstrap behavior when --choose is passed
as its own argument.
| eval "$("${BREW_BIN}" shellenv)" | ||
| fi | ||
|
|
||
| exec just --justfile /usr/share/ublue-os/just/00-entry.just "${@}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate brew shellenv failure before dispatch.
If shellenv fails, eval can receive an empty or partial command and still return successfully; just --choose then runs without usable fzf and can reproduce the original chooser failure. Capture and validate the environment initialization, then verify command -v fzf before executing just.
Proposed fix
- eval "$("${BREW_BIN}" shellenv)"
+ if ! BREW_ENV="$("${BREW_BIN}" shellenv)"; then
+ echo "ujust: failed to initialize Homebrew environment" >&2
+ exit 1
+ fi
+ if ! eval "$BREW_ENV" || ! command -v fzf >/dev/null 2>&1; then
+ echo "ujust: fzf is still unavailable after Homebrew setup" >&2
+ exit 1
+ fi📝 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.
| eval "$("${BREW_BIN}" shellenv)" | |
| fi | |
| exec just --justfile /usr/share/ublue-os/just/00-entry.just "${@}" | |
| if ! BREW_ENV="$("${BREW_BIN}" shellenv)"; then | |
| echo "ujust: failed to initialize Homebrew environment" >&2 | |
| exit 1 | |
| fi | |
| if ! eval "$BREW_ENV" || ! command -v fzf >/dev/null 2>&1; then | |
| echo "ujust: fzf is still unavailable after Homebrew setup" >&2 | |
| exit 1 | |
| fi | |
| fi | |
| exec just --justfile /usr/share/ublue-os/just/00-entry.just "${@}" |
🤖 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 `@system_files/shared/usr/bin/ujust` around lines 17 - 20, Update the brew
shell initialization around eval "${BREW_BIN}" shellenv to capture its output
and propagate any shellenv failure before dispatching just. After applying the
environment, validate that command -v fzf succeeds, and abort with a clear error
if it is unavailable; only then execute just with the existing arguments.
| run env PATH="${WORKDIR}/bin:/usr/bin:/bin" \ | ||
| CALLS="${WORKDIR}/calls.log" MOCK_BIN="${WORKDIR}/bin" \ | ||
| bash "${UJUST}" --choose |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use an isolated PATH in the tests.
Appending /usr/bin:/bin makes the tests depend on the runner image. If either directory already contains fzf, the bootstrap is skipped and the first test exercises a different branch. Keep only the mock directories in PATH, with absolute /usr/bin/bash shebangs already making the scripts executable.
Also applies to: 44-46
🤖 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 `@tests/test_ujust.bats` around lines 33 - 35, Update the test invocations
using --choose in tests/test_ujust.bats to set PATH exclusively to the mock
directory, removing /usr/bin:/bin from both affected run commands. Preserve the
existing WORKDIR, CALLS, MOCK_BIN, and absolute bash execution setup so the
tests consistently exercise bootstrap behavior.
hanthor
left a comment
hanthor
left a comment
There was a problem hiding this comment.
The on-demand path needs to fail closed before dispatching just. The current joined-argument search treats text containing --choose as the option, and a failed or ineffective brew shellenv can still fall through to just with fzf unavailable. Please match --choose as an exact argument, propagate shellenv failure, verify fzf is actually resolvable after applying the environment, and cover those failure paths with an isolated PATH in Bats.
- Match --choose as an exact argument instead of joined-string search - Propagate brew shellenv failure before dispatching just - Verify fzf is resolvable after shellenv and fail closed - Strengthen test_ujust.bats with isolated PATH, fake fzf via shellenv, install→shellenv→just ordering, and negative paths Assisted-by: Claude via GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
castrojo
added
4-review
hanthor
left a comment
hanthor
left a comment
There was a problem hiding this comment.
Approving. The ordering is correct and well-tested — install, then shellenv, then exec just, with each failure mode exiting distinctly (127 for missing brew / still-missing fzf, 1 for install or shellenv failure) and never falling through to just. Switching to exec is a nice incidental cleanup. The bats suite covers all five paths including the negative ones.
One behavioral note, not a blocker: ujust --choose now silently installs software into the user's Homebrew prefix as a side effect of a command that reads like "show me a menu." The installing missing fzf message goes to stderr, so it is at least visible. If you'd rather not have an implicit install, a read -p confirmation would be the alternative — but given --choose is unusable without fzf, auto-installing is defensible. Your call.
Generated by Claude Code
2 participants
Summary
fzfon demand whenujust --chooseruns before brew-preinstallFixes #861
Validation
bats tests/test_ujust.batsjust checkjust testshellcheck -S warning system_files/shared/usr/bin/ujustactionlintworkflow validationpre-commit run --all-filesis currently blocked by pre-existing untracked docs artifacts (docs/skills/INDEX.mdfront matter/index-link checks).Summary by CodeRabbit
New Features
fzfutility when usingujust --chooseand it isn’t available.ujustcommands without installing unnecessary tools.Tests
fzfinstallation and correct command execution.