fix(brew): replace invalid brew tap --trust with brew tap + brew trust - #941
Conversation
clubanderson
left a comment
There was a problem hiding this comment.
One concern: || true suppresses brew trust failures as well as idempotent tap errors. If trust fails, the recipe continues to install from an untrusted tap and hides the actionable error. Prefer guarding only the expected already-tapped case, or verify trust before continuing.
hanthor
left a comment
There was a problem hiding this comment.
Approving — the core fix is correct and I verified it against real Homebrew rather than from the docs alone. Two notes, one of which answers your reviewer question directly.
Premise confirmed. On Homebrew 6.0.15 locally:
brew tap --trust→Usage: brew tap [options] [user/repo] [URL], i.e. a hard usage error before doing anything. The bug is real and exactly as described.brew trustexists, and barebrew trust <owner>/<tap>parses fine without needing--tap(matches docs.brew.sh/Tap-Trust). The two-command replacement is right.
Nice catch on the fifth call site in bazaar-hook — that one wasn't in #814.
Your judgment call on && → ; in bazaar-hook: I'd keep &&. The rationale in the PR body doesn't hold empirically. Re-tapping an already-tapped repo exits 0, not non-zero:
$ brew tap charmbracelet/tap # already tapped
$ echo $?
0
brew trust is idempotent the same way — Already trusted tap: charmbracelet/tap, exit 0. So the "user who already has the tap gets a non-zero exit and the cask install silently never runs" failure mode doesn't occur, and && was never unsafe here.
Switching to ; costs something real: this runs detached in xdg-terminal-exec, so if the tap genuinely fails (network, renamed tap), ; marches on and the user gets a cascade of confusing errors in a terminal that may close before they can read it. && stops at the actual cause. Suggest reverting that hunk to:
f'{brew} tap ublue-os/tap && {brew} trust ublue-os/tap && {brew} install --cask {app}'Same reasoning for 2>/dev/null || true in apps.just / system.just. Since both commands already exit 0 on the repeat-invocation path, the suppression isn't buying idempotency — it's only hiding genuine failures. In install-asus in particular, the recipe runs under set -euo pipefail specifically so failures surface, and || true opts these two lines out of that; the user then hits brew install --cask against an untrusted tap and gets an error that points at the wrong thing. I'd drop at least the 2>/dev/null so the real message survives:
brew tap ublue-os/tap
brew trust ublue-os/tapNeither point is blocking — the PR is a strict improvement over shipped-and-broken either way, and the bats coverage (7/7 pass, 6/7 fail pre-fix) is genuinely load-bearing rather than fitted. Note the mock always exits 0 for a well-formed brew tap, so it happens to model the real semantics correctly and doesn't bake in the non-zero assumption.
One housekeeping item: this is currently DIRTY — it conflicts with main (and with #942/#943, which also touch Justfile and docs/TESTING.md). Needs a rebase before it can merge regardless of the above.
… trust` `--trust` is not a valid flag on `brew tap`. Homebrew 6.0+ rejects the unknown flag and exits non-zero before tapping anything, breaking `ujust devmode`, `install-jetbrains-toolbox`, `install-asus`, and the Bazaar JetBrains hook. Trust is a separate command: https://docs.brew.sh/Tap-Trust Every call site now runs `brew tap <tap>` followed by `brew trust <tap>`, guarded with `2>/dev/null || true` so an already-tapped repo does not abort the `set -euo pipefail` recipes. Adds tests/test_brew_tap_trust.bats, which mocks `brew` to reject unknown flags on `tap` the way Homebrew 6.0 does, and asserts tap and trust are issued as separate invocations. Closes #814 Assisted-by: Claude Opus 5 via GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
3de8c76 to
bb04aaa
Compare
…fin#943) ## Root cause — confirmed against bash-preexec 0.6.0 source Verified by reading the tagged 0.6.0 source directly, not from memory: - L74: the installer is deferred by appending a string to `PROMPT_COMMAND`: `__bp_trap_string="$(trap -p DEBUG)"; trap - DEBUG; __bp_install` - L289-293: `__bp_install()` **returns early** if `"${PROMPT_COMMAND[*]}"` already contains `__bp_precmd_invoke_cmd` - The self-removal code reads `existing_prompt_command="${PROMPT_COMMAND:-}"` and later assigns `PROMPT_COMMAND='__bp_precmd_invoke_cmd'` On Fedora, `PROMPT_COMMAND` is an **array** (bash ≥ 5.1). `${PROMPT_COMMAND}` and bare assignment address **element `[0]` only**. So once any other hook (mise, direnv, starship, vte, systemd, zoxide) pushes the installer out of element `[0]`, it is **never removed** — and runs on every prompt. Its first act is `trap - DEBUG`; `__bp_install` then returns early. From prompt 2 onward the DEBUG trap is permanently empty. That is exactly the reporter's `declare -p` output: `__bp_precmd_invoke_cmd` in `[0]`, installer stranded in `[2]`, atuin's hook absent. Reproduced empirically in a real prompt loop: `cycle1 trap: [trap -- '__bp_preexec_invoke_exec "$_"' DEBUG]`, `cycle2: []`, `cycle3: []`. With this fix, all three cycles retain the trap. Matches upstream rcaloras/bash-preexec#188 (fixed upstream but insufficient alone) and #186 (still open). ## Non-obvious discovery Bash does not inherit the DEBUG trap into functions without `functrace`: `trap -p DEBUG` **inside a function always returns empty**, and `trap - DEBUG` inside a function is a no-op — but `trap ... DEBUG` inside a function *does* set it globally. Two consequences: a detector-guard is impossible (hence the unconditional re-arm), and it invalidated the first simulation, which passed **vacuously**. Written up in `docs/skills/shell-scripts.md`. ## Changes - New `system_files/shared/.../bling/bash-preexec-rearm.sh` — re-arms the exact trap bash-preexec installs, once per prompt - `bling.sh` sources it **last**, under the bash guard, so the hook is the final `PROMPT_COMMAND` entry `bling.sh` stays POSIX (it is under an sh-dialect shellcheck gate); the bash-only logic lives in the sibling file. `BLING_DIR` allows bats to point at the repo tree. No vendored copy of bash-preexec. ## Tests — 14 cases, all pass Tests **1 and 4 are negative controls** that pin the bash-preexec stand-in to the real defect, so the positive cases cannot pass vacuously. Covered: array `PROMPT_COMMAND` trap survival, atuin-style hook still firing, idempotency on double-source, scalar `PROMPT_COMMAND` (no regression), bash-preexec absent, readonly `PROMPT_COMMAND`, missing helper, zsh source-safety. Also green: `bash -n` / `sh -n`, both CI shellcheck invocations, `just check`, `pre-commit run --all-files`. ## Confidence and limits High that the DEBUG trap now survives — the failure and its repair are both directly reproduced. Two things only real hardware can prove: 1. atuin actually writing records end-to-end (tests use a preexec stub, not atuin 18.18.1) 2. that no downstream hook on a real session appends its own `trap - DEBUG` **after** our entry Ordering is safe for everything bling itself initializes. ## Reviewer note The re-install is **unconditional** per prompt, forced by the functrace scoping above; it matches the reporter's validated workaround. A third party's own raw DEBUG trap set after bling would be overwritten each prompt — but such a trap already breaks bash-preexec today. ## Blast radius — highest in this batch `bling.sh` is sourced by **every interactive bash shell** on bluefin, bluefin-lts, and dakota. Recommend lab validation before merge. Closes projectbluefin#869 --- ## Branch state Updated onto `main` after projectbluefin#926 landed (via `gh pr update-branch`). No conflict — this PR's `docs/TESTING.md` row and `Justfile` entry merged cleanly. ## Overlap with the sibling PRs in this batch **projectbluefin#942** also edits `docs/skills/shell-scripts.md`, but adds different sections (this PR: `Bash DEBUG traps are invisible inside functions`, `POSIX-sh files cannot hold bash array code`; projectbluefin#942: `image-info.json is build-time state`). Verified non-conflicting with a real sequential merge. No file overlap with projectbluefin#941. ## Pre-merge checklist for the reviewer - [ ] Lab-validate on real hardware — the tests use a preexec stub, not atuin 18.18.1, so end-to-end recording is unproven - [ ] Confirm the unconditional per-prompt trap re-install is acceptable (see reviewer note above; a detector-guard is impossible due to bash's functrace scoping) Co-authored-by: Jorge Castro <jorge@projectbluefin.io> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Problem
brew tap --trust <owner>/<tap>is invalid syntax.--trusthas never been a valid flag onbrew tap. On Homebrew 6.0+,brew taprejects the unknown flag and exits non-zero before doing anything, soujust devmodeand several app installers are broken as shipped.The correct interface (docs.brew.sh/Tap-Trust) is two commands:
brew tapthenbrew trust.Changes
Five call sites fixed — one more than #814 listed:
system_files/shared/.../just/apps.just:12install-jetbrains-toolboxsystem_files/shared/.../just/apps.just:63install-asus(underset -euo pipefail)system_files/bluefin/.../just/system.just:149,152toggle-devmodesystem_files/bluefin/usr/libexec/bazaar-hook:34spawn_brew— not in the original reportbbrew/cncfwere checked: nobbrewrecipe exists insystem_files/anymore, andcncfusesbrew bundlewithtrusted: trueBrewfiles, which is already correct.Docs:
docs/skills/brew-lifecycle.mdactively taught the bug in a diff block (+ brew tap --trust); reversed and corrected, along with a stale note inoem-hardware-hooks.md.Tests
tests/test_brew_tap_trust.bats— 7 tests. Thebrewmock rejects unknown flags ontapexactly as Homebrew 6.0 does, and the real recipes are driven viajust --justfile.Reviewer note — one judgment call
bazaar-hookchanged&&to;between tap and install. With&&, a user who already has the tap gets a non-zero exit frombrew tapand the cask install silently never runs. This is a behavior change beyond the literal flag fix — say the word if you'd rather keep&&.Blast radius
system_files/shared/→ bluefin + bluefin-lts + dakota.Closes #814
Branch state
Merged
mainat3de8c769after #926 landed. The only collision was aone-line conflict in the
docs/TESTING.mdtest table — both PRs append a row.Resolved by keeping both rows;
Justfileauto-merged.Re-verified after that merge:
test_brew_tap_trust.bats7/7 pass andtest_theming_hook.bats(from #926) 5/5 pass.The 6/7-fail-on-pre-fix-code claim above was re-confirmed independently by
checking out
origin/main'ssystem_files/under this branch's tests.