Refactor bootstrap, optimize package installation, and fix macOS compatibility issues#320
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -0,0 +1,630 @@ | |||
| { | |||
There was a problem hiding this comment.
[Important] This renames preferences/iTerm Profile.json to preferences/iTerm.json, but brew-hooks/iterm2.sh line 12 still points at the old path (profile_src ending in 'preferences/iTerm Profile.json'). With the old file deleted, the hook hits its missing-file guard and silently exits 0, so the iTerm2 DynamicProfile is no longer installed. Also, the new iTerm.json is a flat single-profile export with no top-level Profiles array wrapper, so it is not a valid iTerm2 DynamicProfile; symlinking it into DynamicProfiles/ as the hook does would not load it correctly even if the path were fixed. Either update the hook to the new filename AND convert the file back to DynamicProfile format, or remove brew-hooks/iterm2.sh if iTerm2 setup is now a manual import (matching the 'Manual color preset import required' note in CLAUDE.md).
|
|
||
| # Restore/cleanup temporary pip.conf | ||
| if [[ "$temp_pip_conf" == true ]]; then | ||
| rm -f "$HOME/.config/pip/pip.conf" |
There was a problem hiding this comment.
[Important] The script runs under set -euo pipefail. If brew bundle at line 1018 (or the AI bundle at 1026) exits non-zero, the script aborts before reaching this cleanup, leaving the temp ~/.config/pip/pip.conf in place. Since that temp file masks the corporate /Library/Application Support/pip/pip.conf (which this code only touches when no user config exists), a failed brew bundle permanently shadows the corporate mirror config on subsequent runs too: the existence guard at line 1008 then sees the leftover file and skips recreating/cleaning it. Guarantee cleanup with a trap set right after creating the temp file (trap to rm the file on RETURN/EXIT) instead of an inline rm at the end of the function.
| echo "Configuring NPM registry credentials via gcloud..." | ||
| local settings | ||
| if settings=$(gcloud artifacts print-settings npm --project=artifact-foundry-prod --repository=ah-3p-staging-npm --location=us 2>/dev/null); then | ||
| echo "$settings" | grep -v "always-auth" >> "$HOME/.npmrc" |
There was a problem hiding this comment.
[Suggestion] Under set -o pipefail plus set -e, if every line of settings contains always-auth then grep -v exits 1 and the non-zero pipe status aborts the whole bootstrap. In practice print-settings emits registry and _authToken lines too, so this is unlikely, but appending a trailing '|| true' makes it safe regardless. Minor: the appended _authToken from gcloud artifacts print-settings is a short-lived OAuth token; once written it is not refreshed (the guard at line 1182 skips re-running once ah-3p-staging-npm is present), so it can go stale. Worth a comment noting it may need periodic regeneration.
| brew "whisper-cpp" | ||
|
|
||
| # Tailscale Mesh VPN (event bus, openclaw) | ||
| cask "tailscale-app" |
There was a problem hiding this comment.
[Suggestion] tailscale-app is moved from the base Brewfile into Brewfile.ai, so it now only installs when the user opts into AI packages (and the non-interactive default for AI install was just flipped to false). Per CLAUDE.md, Tailscale underpins the infra services and OpenClaw gateway networking independent of AI tooling; gating it behind the AI bundle means a plain ./bootstrap.sh -p will not install it. If that is intentional, ignore; otherwise consider keeping tailscale-app in the base Brewfile.
| # Restore/cleanup temporary pip.conf | ||
| if [[ "$temp_pip_conf" == true ]]; then | ||
| rm -f "$HOME/.config/pip/pip.conf" | ||
| echo "Restored corporate Pip configuration." |
There was a problem hiding this comment.
[Suggestion] This message is misleading: nothing was restored. The corporate config at /Library/Application Support/pip/pip.conf was never modified; this branch only removes the temporary ~/.config/pip/pip.conf it created. Wording like 'Removed temporary PyPI mirror override.' reads more accurately.
There was a problem hiding this comment.
Code Review
Summary. Solid robustness pass on the bootstrap flow: non-interactive guards for chsh/AI install, brew shellenv wiring, a temporary PyPI mirror override for Homebrew Python deps, gcloud-driven npm registry auth, a Stats post-install hook, and a Brewfile reorg that moves AI/optional tooling into Brewfile.ai. Two issues should be fixed before merge.
Findings (posted inline):
- [Important] brew-hooks/iterm2.sh still references the deleted preferences/iTerm Profile.json; the renamed iTerm.json is also not a DynamicProfile format, so the hook now silently no-ops.
- [Important] The temporary ~/.config/pip/pip.conf is leaked if brew bundle fails under set -euo pipefail, permanently masking the corporate pip config on later runs. Use a trap.
- [Suggestion] grep -v always-auth under pipefail can abort the whole bootstrap if all lines match; the appended OAuth _authToken can also go stale.
- [Suggestion] tailscale-app moved into Brewfile.ai gates infra/gateway networking behind the AI install.
- [Suggestion] The 'Restored corporate Pip configuration' message is misleading (nothing is restored).
Verdict. REQUEST_CHANGES — Two important issues: a broken iTerm2 hook reference and a non-guaranteed pip.conf cleanup.
Automated review by Claude Code
- pip.conf: guarantee cleanup via EXIT trap so a failed brew bundle under set -euo pipefail can't leave the temp file masking the corporate pip.conf on later runs; clear the trap on the success path. Reword the cleanup message (nothing is "restored"). - npm registry: guard `grep -v always-auth` with `|| true` so a fully filtered output (grep exit 1) can't abort bootstrap under pipefail; note the embedded OAuth token can go stale. - Brewfile: move tailscale-app back out of Brewfile.ai into the base Brewfile — it underpins infra/gateway networking independent of AI tooling. - Remove obsolete brew-hooks/iterm2.sh: preferences/iTerm.json is now a flat color-preset export (manual import), not a DynamicProfile, and the hook still pointed at the deleted iTerm Profile.json.
| eval "$(/usr/local/bin/brew shellenv)" | ||
| fi | ||
| else | ||
| echo "Updating Homebrew..." |
There was a problem hiding this comment.
[Suggestion] This branch now runs only brew update (metadata); the old brew update / brew upgrade / brew cleanup block was removed from update_packages. Net effect: ./bootstrap.sh -p no longer upgrades installed Homebrew formulae or reclaims disk via cleanup (brew bundle installs missing packages but does not upgrade existing ones). That diverges from CLAUDE.md's Phase 1 'Updates existing packages.' If intentional (speed / avoid breakage), consider updating the CLAUDE.md and --help wording so the documented behavior matches; otherwise restore brew upgrade / cleanup.
| cask "gcloud-cli" # Requires: gcloud init | ||
|
|
||
| # Mac App Store Apps | ||
| mas "Amphetamine", id: 937984704 if OS.mac? |
There was a problem hiding this comment.
[Suggestion] mas install requires an active App Store session. On a machine not signed in, these entries cause brew bundle to exit non-zero, and since bootstrap.sh runs under set -euo pipefail with no || true on the brew bundle --file call, the whole bootstrap aborts during Phase 1 — before sync_dotfiles runs. Given the PR's robustness theme, consider tolerating bundle failure or noting the App Store sign-in prerequisite.
| # Note: print-settings embeds a short-lived OAuth _authToken that is not | ||
| # refreshed here (the guard above skips re-running once ah-3p-staging-npm | ||
| # is present in ~/.npmrc). Re-run this step if npm auth starts failing. | ||
| echo "$settings" | { grep -v "always-auth" || true; } >> "$HOME/.npmrc" |
There was a problem hiding this comment.
[Suggestion] Appending to ~/.npmrc with >> does not guarantee a preceding newline. If an existing ~/.npmrc lacks a trailing newline, the first gcloud-emitted line (the @scope:registry= line) concatenates onto the previous line and corrupts both. Consider emitting a leading newline before the appended settings.
| # Close Stats app if running to ensure it picks up the new config on restart | ||
| killall Stats 2>/dev/null || true | ||
|
|
||
| defaults import eu.exelban.Stats "$plist_src" |
There was a problem hiding this comment.
[Suggestion] This defaults import runs on every run_brew_hooks invocation (i.e. every -p bootstrap), unconditionally overwriting eu.exelban.Stats — so any settings the user changed in the Stats GUI are silently reverted to the committed plist on the next bootstrap. If that is the intended source-of-truth behavior it is fine; otherwise consider importing only when the prefs domain is absent.
There was a problem hiding this comment.
Code Review
Summary. Clean robustness pass on bootstrap: non-interactive guards for chsh/AI install, brew shellenv wiring, a trap-protected temporary PyPI mirror override, gcloud-driven npm registry auth, a Stats post-install hook, and a Brewfile reorg that moves AI/optional tooling into Brewfile.ai.
Previously Addressed (Filtered). The two [Important] issues from the prior review round are resolved in this diff and are not re-raised:
- iTerm2 hook referencing the deleted profile JSON — brew-hooks/iterm2.sh is now deleted entirely; the new preferences/iTerm.json is a manual-import color preset (consistent with CLAUDE.md 'Manual color preset import required').
- Leaked temporary ~/.config/pip/pip.conf — now guarded by a
trap ... EXITcleared on the success path; the misleading 'Restored corporate Pip configuration' message is also gone (now 'Removed temporary PyPI mirror override.').
Findings (posted inline). Four [Suggestion] items, none blocking: brew upgrade/cleanup dropped from -p; mas entries can abort brew bundle under set -e; ~/.npmrc append lacks a leading-newline guard; stats.sh re-imports prefs every bootstrap.
Verdict. APPROVE — No Critical or Important issues; suggestions are non-blocking and posted as inline comments.
Automated review by Claude Code
- brew bundle: tolerate a non-zero exit (e.g. a `mas` App Store entry on a machine that isn't signed in) with a visible warning, so Phase 1 no longer aborts before sync_dotfiles runs. brew bundle still reports which packages failed, so failures stay visible. - npm registry: prepend a newline before appending gcloud settings to ~/.npmrc so the first line can't concatenate onto an existing entry that lacks a trailing newline. - CLAUDE.md: document that `-p` refreshes metadata + installs missing packages but no longer runs `brew upgrade`/`cleanup` on macOS. Left stats.sh prefs import as-is: the committed plist is intentionally the source of truth, matching the repo's tracked-config pattern.
|
|
||
| # Update pip packages (no safe "update all" — list managed packages explicitly) | ||
| if command -v pip3 >/dev/null 2>&1; then | ||
| if [[ "$INSTALL_AI" == true ]] && command -v pip3 >/dev/null 2>&1; then |
There was a problem hiding this comment.
[Suggestion] Gating the pip update on INSTALL_AI == true means it never runs on Linux: prompt_ai_install (the only setter of INSTALL_AI) is invoked only inside the macOS install_brew_packages path. On Debian/Ubuntu/SteamOS, INSTALL_AI stays empty, so piper-tts is no longer installed or upgraded there, with no way to opt in. If that is intended (AI is macOS/gateway-only), a short comment here would keep the asymmetry from being read as a bug later.
| # is present in ~/.npmrc). Re-run this step if npm auth starts failing. | ||
| # Prepend a newline so the first appended line can't concatenate onto an | ||
| # existing ~/.npmrc entry that lacks a trailing newline. | ||
| { printf '\n'; echo "$settings" | { grep -v "always-auth" || true; }; } >> "$HOME/.npmrc" |
There was a problem hiding this comment.
[Suggestion] Two small notes on this append: (1) the unconditional leading newline writes a blank first line into a freshly-created ~/.npmrc; guarding on a non-empty file before prepending the newline avoids the stray blank line. (2) As the comment above notes, the embedded short-lived OAuth token is not refreshed on later runs because the ah-3p-staging-npm guard skips re-execution. That is documented, so just flagging it as a known re-run footgun for when npm auth starts failing.
| # Set up Homebrew PATH for macOS | ||
| if [[ "$(uname)" == "Darwin" ]]; then | ||
| if [[ -f "/opt/homebrew/bin/brew" ]]; then | ||
| eval "$(/opt/homebrew/bin/brew shellenv)" |
There was a problem hiding this comment.
[Suggestion] eval of brew shellenv forks a brew subprocess on every interactive shell startup; with many panes or tabs (zellij) that latency adds up. Since bootstrap.sh already runs the same block at its top for its own session, you could skip it here or guard on an unset HOMEBREW_PREFIX so it evaluates only once per shell tree.
| @@ -0,0 +1,630 @@ | |||
| { | |||
There was a problem hiding this comment.
[Suggestion] This is a full iTerm2 profile export (it carries Name, Guid, fonts, and a hardcoded Working Directory of /Users/evansenter), not just a color preset. CLAUDE.md still describes iTerm setup as "Manual color preset import required," which undersells what this file is and how to import it (Preferences -> Profiles -> Other Actions -> Import JSON Profiles). A one-line doc tweak would help; the embedded absolute home path is harmless for personal dotfiles but will not match a differently-named user.
There was a problem hiding this comment.
Code Review
Summary. Solid robustness pass on bootstrap: non-interactive guards for chsh/AI install, brew shellenv wiring, a trap-protected temporary PyPI mirror override, gcloud-driven npm registry auth, a Stats post-install hook, btop symlink-break-before-sed, and a Brewfile reorg that splits AI/optional tooling into Brewfile.ai.
Previously Addressed (Resolved in this diff). The two [Important] issues from the first review round are fixed and are not re-raised:
- iTerm2 hook referenced a deleted profile JSON — brew-hooks/iterm2.sh is now deleted entirely; iTerm setup is fully manual via preferences/iTerm.json, consistent with CLAUDE.md.
- Leaked temporary ~/.config/pip/pip.conf — now guarded by a trap on EXIT (the only EXIT trap in the script) and cleared on the success path; the misleading "Restored corporate Pip configuration" message is gone.
Findings (posted inline). Four [Suggestion] items, none blocking:
- bootstrap.sh:901 — pip/piper-tts update is gated on INSTALL_AI, which is never set on Linux.
- bootstrap.sh:1221 — leading-newline writes a blank first line into a fresh ~/.npmrc; OAuth token is not refreshed on re-runs.
- home/.exports:13 — brew shellenv forks on every shell startup; duplicated with bootstrap top-level.
- preferences/iTerm.json:1 — full profile export (with personal Working Directory), not just a color preset; doc wording is slightly off.
Verdict. APPROVE — No Critical or Important issues. Suggestions are posted inline for the author to address at their discretion.
Automated review by Claude Code
2 participants
This PR contains multiple robustness improvements and fixes to the bootstrap process.