feat(onboarding): guided Homebrew install step (ENG-490) - #602
feat(onboarding): guided Homebrew install step (ENG-490)#602Andrew MacBride (amacbride) wants to merge 10 commits into
Conversation
Retarget the guided Homebrew onboarding step from the abandoned `develop` branch onto `main`, whose onboarding flow has since evolved (added machine-choice, customizations, inference, and build gates). The gate-based `computeOnboardingStep`/`STEPS` pattern is unchanged, so the optional "homebrew-setup" step slots in after `nix-setup` and before `config-dir`. Skip is session-only (mirrors `inferenceDeferred`); no Rust preferences or durable state added. Backend keeps the existing `invoke` command style that `main` still uses (`homebrew_check`, `homebrew_install_stream` alongside `main`'s adoption/diff commands) — no oRPC migration required. Password is fed over stdin, never argv; `sudo -k` invalidates the credential after install. cargo check, tsc, and the unit suite (333 tests) all pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LnWiVkakAZP6HRFzYETKq
…le (ENG-490) Three bugs found by running the onboarding flow on a clean macOS VM. None reproduce on a dev machine, which is why they survived until now. sudo credential scoping. install.sh runs `sudo -n` for its pre-flight check and could not see the credential we cached: macOS scopes the sudo timestamp per-TTY, falling back to per-parent-process when there is no TTY, so a credential cached by the app process is invisible to the installer's bash subtree. It aborted with "needs to be an Administrator" against a user who is one. Priming or keepalive could never have fixed this. Now sets SUDO_ASKPASS so install.sh uses `sudo -A` and obtains the password from a helper on every call, independent of TTY or timestamp. The password crosses a unix socket in a 0700 directory and never touches disk or argv. Command Line Tools hang. install.sh installs CLT via a headless `softwareupdate -i`, observed transferring zero bytes over 12 minutes with no output, error, or exit, while curl on the same machine reached Apple at full speed. The UI had no timeout and would spin forever. CLT is now installed first via `xcode-select --install`, which hands off to the normal macOS flow, with progress heartbeats and a 45 minute timeout that fails with actionable text. With CLT present, install.sh skips its own CLT path entirely. Bundle portability. libz-sys probed with pkg-config and, with a package-manager prefix ahead of the system toolchain on PATH, resolved /opt/local/lib/libz. That dragged -I/opt/local/include into the link, silently binding GNU libiconv and producing a bundle that crashed on any clean Mac with a dyld error. Declaring libz-sys with the static feature unifies it across the graph and removes the pkg-config probe; `bun run check:portability` fails the build on any dylib reference outside /usr/lib, /System, or the bundle. Also adds scripts/install-swift-toolchain.sh. The tauri-plugin-macos-passkey dependency builds a Swift package requiring swift-tools 6.1.0, but Xcode 16.2 and earlier ship Swift 6.0.3, so cargo build fails with a confusing swift-lib error. Xcode 16.3+ would fix it but requires macOS 15, forcing an OS upgrade on macOS 14 hosts. The script installs the official swift.org 6.1.2 toolchain alongside Xcode instead, leaving system and package-manager Swift untouched. Verified end to end on a Homebrew-less macOS 14.6.1 VM: Homebrew 6.0.12 installed in ~40s, the step went green, and onboarding advanced. Portability verified by rebuilding with a package-manager prefix first on PATH and no env overrides. Claude-Session: https://claude.ai/code/session_01B6hUFgC8sD6P5nt3e1KiJh
🎨 Storybook previewUpdated for 8260bef
|
📋 PR Overview
🔬 Coverage
|
|
I would like to take a closer look when I have more time over the week (sadly I'm with a migraine today) but one thing I'm unsure about is that whether we should ask users to install upfront, as suggested by this PR, or on demand, at a later stage, when the configuration requires Homebrew and the build requests it... |
|
Also: I find the link to the claude code session interesting for reproducibility, but the link doesn't work for me (it shows an empty session, perhaps because it would require to be logged in with your user?). |
There was a problem hiding this comment.
Hi!
Finally I had some time to test this properly.
First, I could not get a proper build from CI because it was failing. I've addressed the issues and pushed on your branch, I hope it's ok.
Main issues:
-
While testing it, I did find one bug: if you click on a previous onboarding step while the installation is happening (in my case the CLI tools was taking place, which takes a while) and then go back to the Homebrew page, it no longer shows that it's installing Homebrew, even though the installation is actually happening in the background. It should either remember the current "installing" state (this should be stored in the backend and flow to the UI using the store mirror logic we normally use) or cancel the installation (with a warning/confirmation) when moving away of this step.
-
Another issue I have with this is that this PR reimplements sudo. This is unnecessary if we used the
privileged-helperthat we have Built For these purposes.Note that
install.shrefuses to run as root, but the only thing it needs sudo for (once Command Line Tools are handled, which yourxcode-select --installpath already covers without privileges) is creating/chowning the prefix. Itsexecute_sudocalls only fire when its computed mkdir/chown/chmod lists are non-empty. So you can add an argument-freeHelperRequest::PrepareHomebrewPrefixthat derives the uid from the socket peer credentials (asauthorize_requestalready does) and creates the/opt/homebrewlayout owned by that user. Then runinstall.shas the user exactly as this PR does — with nothing left for it to sudo. -
Also: Homebrew now ships an official signed
.pkgthe helper could run viainstaller -pkg, maybe it simplifies some of this.
| app: &AppHandle, | ||
| askpass: Option<&Path>, | ||
| ) -> Result<(), (i32, String)> { | ||
| let script = format!(r#"/bin/bash -c "$(curl -fsSL {})""#, HOMEBREW_INSTALL_URL); |
There was a problem hiding this comment.
Must-fix: a curl failure reports install success. If the download fails (network blip, GitHub down), the command substitution is empty, bash -c "" exits 0, and we emit ok: true. Verified locally:
$ bash -c '/bin/bash -c "$(curl -fsSL https://nonexistent.invalid/install.sh)"'; echo $?
0
Combined with the phase/store mismatch you flagged in the description, this makes the "exit 0 but brew undetectable" dead-end reachable via a transient network failure — right after the user typed their password. Suggest fetching first and failing loudly:
script="$(curl -fsSL "$url")" || exit 1
[ -n "$script" ] || exit 1
exec /bin/bash -c "$script"| // Re-detect so a successful install flips the store to installed and the | ||
| // onboarding step advances; a failed install leaves it false. | ||
| await checkHomebrew(); | ||
| options.onDone?.(event.payload.ok, event.payload.error); |
There was a problem hiding this comment.
Since checkHomebrew() has already run by this point, the real outcome is known here — pass it through instead of trusting the exit code alone, so "exit 0 but brew undetectable" can never render success:
const installed = await checkHomebrew(); // have it return result.installed
options.onDone?.(event.payload.ok && installed, event.payload.error);| setPhase("installing"); | ||
| void installHomebrew({ | ||
| onLine: (line) => setLog((prev) => [...prev, line]), | ||
| onDone: (ok, error) => { |
There was a problem hiding this comment.
This is the other half of the dead-end you called out in the description: once phase hits "success" the footer disappears, and the store-sync effect above only downgrades from "checking" — so if the gate's re-detection says brew is absent, the user is stuck on "Homebrew is installed" with no buttons and no Skip. Let the effect downgrade "success" when homebrewInstalled === false (and/or set phase from the corrected ok per my comment in use-homebrew-install.ts).
| const handleSkip = () => onboardingActions.setHomebrewSkipped(true); | ||
|
|
||
| const footer = | ||
| phase === "missing" || phase === "failed" ? ( |
There was a problem hiding this comment.
There's no way out during "installing": the footer disappears, and the CLT wait can legitimately sit for up to 45 minutes on an Apple dialog the user may have dismissed. The only escape is force-quitting the app (which also orphans the installer and leaks the askpass dir). At minimum keep "Skip for now" visible while installing; a real cancel can be a follow-up.
| "desktop:build": "bun run build:sidecars && tauri build --config '{\"bundle\":{\"externalBin\":[\"binaries/nixmac-helper\",\"binaries/nixmac-sync-agent\"]}}'", | ||
| "desktop:build:local": "bun run build:sidecars && tauri build --bundles app --config '{\"bundle\":{\"createUpdaterArtifacts\":false,\"externalBin\":[\"binaries/nixmac-helper\",\"binaries/nixmac-sync-agent\"]}}' && bun run sign:local-app", | ||
| "desktop:test": "cargo test --manifest-path src-tauri/Cargo.toml && bun run test:unit", | ||
| "check:portability": "node scripts/check-bundle-portability.mjs", |
There was a problem hiding this comment.
Nothing runs this yet. If this is important we shall consider adding it to CI.
| /// The installer is run with `NONINTERACTIVE=1` so it does not pause to prompt | ||
| /// the user to press RETURN. It may still require `sudo`; password handling is | ||
| /// surfaced through the streamed log for now. | ||
| pub fn install_stream(app: &AppHandle) -> Result<(), anyhow::Error> { |
There was a problem hiding this comment.
Nit: no guard against concurrent run. A double invoke spawns two installers and two password dialogs. The frontend already kinda prevents it but a cheap AtomicBool here would make the command itself safe.
| info!("[homebrew] install completed successfully"); | ||
| // fire-and-forget: emit only errors when no listeners are | ||
| // registered (window hidden/destroyed); a missing event is non-fatal. | ||
| let _ = app_handle.emit( |
There was a problem hiding this comment.
Nit: HomebrewInstallDataEvent/HomebrewInstallEndEvent exist and generate the TS types, but the emit sites all use ad-hoc serde_json::json!, we should emit the scripts to avoid drift.
| } else { | ||
| let stderr = String::from_utf8_lossy(&output.stderr); | ||
| if stderr.contains("-128") || stderr.to_lowercase().contains("user canceled") { | ||
| Err((-128, "Installation cancelled.".to_string())) |
There was a problem hiding this comment.
Nit: dismissing the password dialog lands the UI in the red "failed" state with "Installation cancelled.", returning to "missing" would read better for a deliberate cancel.
| // only gate on the Nix package manager itself. `darwinRebuildAvailable` is | ||
| // still surfaced in the step as an optional check. | ||
| const nixReady = nixInstalled === true || settings.nixInstalledOverride === true; | ||
| const homebrewReady = homebrewInstalled === true; |
There was a problem hiding this comment.
Nit: detection only runs when the gated step itself mounts, so every user, including ones who already have brew, briefly lands on this step in "checking". Kicking checkHomebrew from the flow level (or app init) would let brew-havers never see the step at all.
| const walk = (dir) => { | ||
| for (const entry of readdirSync(dir)) { | ||
| const full = join(dir, entry); | ||
| const stat = statSync(full); |
There was a problem hiding this comment.
Nit: statSync throws on broken symlinks (common in Frameworks/ layouts), which would kill the whole check — use lstatSync or wrap it.
CI failed on two mechanical issues in the new homebrew code: - clippy manual_is_multiple_of on the CLT heartbeat tick check - rustfmt line-wrapping and import order in homebrew.rs / commands/homebrew.rs No behavior change.
|
| Option | What it gives users | What it costs | Effort to change later |
|---|---|---|---|
| Current (upfront gate) | One install decision before the first build; no mid-session interruption | Every brew-less user hits the step, including those who never need brew | Moderate — affects step order, tests, stories |
| On-demand trigger | Only users who actually need brew see the prompt | Brew install interrupts a build session; requires evolve-loop detection | High |
| Non-blocking offer | Shows installer as an option, not a gate | Users who need brew may skip and hit an unclear failure later | Low — remove the gate condition, keep the step |
2. Homebrew detection runs at app startup alongside Nix and permissions checks, so users who already have Homebrew skip the step without seeing any Homebrew UI.
This is the right pattern. The probe is a sub-second CLI call. Running it with the other startup checks means the hydration gate cannot flip before the result is known, preventing the flash of the Homebrew step for users who already have brew. Users with brew skip the step entirely.
3. A user's choice to skip the Homebrew step resets when the app closes, so a user who quits before finishing onboarding must decide again on next launch if brew is still absent.
Session-only skip mirrors inferenceDeferred, which is a deliberate consistency choice. For users who complete onboarding in one sitting, this works. The concern is the CLT wait: the download can take up to 45 minutes. A user who triggers the CLT install, loses power, and returns the next day will be deposited back at the Homebrew step with no memory of their earlier choice. A small durable flag — written once the user explicitly skips — would prevent that without committing to any permanent preference.
4. When an install exits with success but Homebrew is still not detectable, the UI shows a failure state with "Try again" and "Skip" options rather than a success screen with no way to continue.
This directly closes the dead end described in prior concern 4. The backend always probes brew --version after the install, regardless of the installer's exit code. The frontend derives its phase from that probe result, not from the end event's ok field. A unit test explicitly covers the scenario. This is the correct fix, and the test locks the behavior.
5. Homebrew installs as root through the privileged helper daemon, with no password dialog, verified against Homebrew's pinned signing team ID before the installer runs.
The prior approach required a SUDO_ASKPASS relay because the GUI app has no TTY and the credential cache does not cross process trees. The new approach removes the sudo indirection entirely: the helper already runs as root. The package signature check pins the team ID and requires Apple notarization before root executes anything. This is simpler and more secure than the SUDO_ASKPASS path. The only user-visible difference is the absence of any custom password dialog during the Homebrew phase.
6. Command Line Tools installation requires the user to complete a macOS system dialog, and the app polls for that completion for up to 45 minutes while showing a waiting state.
Using xcode-select --install is demonstrably better than headless softwareupdate (the PR measured a 12-minute stall vs. a 3-minute successful download). The system dialog is familiar and trustworthy. The concern is the dismissal case: a user who misses or dismisses the dialog will see "Waiting for Command Line Tools" for several minutes with no actionable guidance. The heartbeat messages tell the user how long they have waited, but they do not tell the user to re-run xcode-select --install or check for a system notification. One actionable line after the first heartbeat — for example, "If you dismissed the dialog, run xcode-select --install in Terminal to restart it" — would significantly reduce that failure mode.
7. Skip is accessible at any time during installation, including while the app waits for the Command Line Tools system dialog.
This is clearly correct. Without it, force-quitting the app would be the only exit from a stuck or unwanted CLT install. A unit test covers this case. The CLT wait can last 45 minutes, so Skip must remain reachable.
8. The step description tells users that "features that need Homebrew will be marked accordingly" when no such labeling system exists in the product.
This promise is what makes "skip" feel like a real choice. It tells the user that skipping is safe because they will know which features they cannot access. Without the labels, a user who skips Homebrew has no way to discover which parts of the product need brew. They will encounter failures with no context. The step description should either omit this promise until the labeling system ships, or the labeling system must land alongside this PR. Shipping copy that describes a capability that does not exist creates a broken expectation. Prior question 10 raised this concern and it remains unaddressed.
9. Installer output arrives as a batch after the install finishes, not as a live stream; the UI communicates progress through named phases rather than a scrolling log during the install.
This is a consequence of the privileged helper's request-response model: the helper runs the installer, captures all output, and returns one response. The PR documents this clearly. Named phases ("Waiting for Command Line Tools", "Installing Homebrew") give users meaningful status during the wait. The concern is failure debugging: when the install fails, the full log appears at once. If the failure message is buried near the end of a long run, users must scroll to find it. This is an acceptable tradeoff for the security gain, but edge-case failures will be harder to diagnose than they would be with a streaming approach.
10. Homebrew's signing team ID is hardcoded in the codebase, so accepting a future change to Homebrew's signing identity requires a deliberate code change.
The code comment explains this directly: the signer of a root-executed binary should be pinned, not merely required to be some valid Developer ID. A change to Homebrew's signing identity would be a public and notable event. The maintenance cost of one code update is low compared to the security benefit of not accepting an arbitrarily signed package.
Since the last review
- Still open: Every new user hits the Homebrew step upfront, regardless of whether their workflow needs brew (Gate condition in onboarding.ts still blocks all users without brew; Juanpe Bolívar (@arximboldi) raised the on-demand alternative directly in the PR conversation with no response from the author)
- Still open: Skip is session-only and resets on next launch (homebrewSkipped: false remains in initialOnboardingState (store.ts); no durable persistence call was added)
- Addressed in code: Detection runs only when the Homebrew step mounts, causing brew-installed users to briefly see the step (checkHomebrew() added to widget.tsx startup probe alongside checkNix and checkPermissions, before markViewModelHydrated() is called)
- Addressed in code: Stuck state: install exits 0, brew undetectable, success screen renders with no way forward (record_install_end in homebrew.rs always probes is_installed() post-install; homebrew-setup-step.tsx failed phase renders Try again and Skip buttons; unit test 'stays actionable when an install reports success but brew is undetectable' locks the behavior)
- Addressed in code: Custom osascript password dialog instead of system-native privilege escalation (install_homebrew in helper_runtime.rs runs /usr/sbin/installer as root through the privileged helper; no SUDO_ASKPASS relay or custom dialog exists in the new Homebrew install path)
- No longer applicable: Temp SudoAskpass credential helper directories (nixmac-ap-<pid>) left on force-quit (The SUDO_ASKPASS relay is not used in the new install path; the helper daemon uses WorkDir and HomebrewPkgUser structs with Drop-based cleanup in its own process space)
- Still open: What fraction of new users arrive without Homebrew installed (No usage data or survey cited in the PR description or conversation)
- Still open: What fraction of nixmac tasks need Homebrew vs. pure Nix packages (No usage data cited; the on-demand vs. upfront question that depends on this remains open in the PR conversation)
- Still open: Whether an on-demand trigger in the evolve loop is practical (Juanpe Bolívar (@arximboldi) raised this directly in the PR conversation; no response from the author; no on-demand mechanism is added in the diff)
- Still open: 'Homebrew features marked accordingly' labeling system needed to make Skip a meaningful choice (homebrew-setup-step.tsx description contains "features that need Homebrew will be marked accordingly" but no labeling UI or capability exists anywhere in the diff)
- No longer applicable: Realistic trigger frequency for the exit-0-but-brew-undetectable scenario (record_install_end probes is_installed() regardless of exit code; the fix is defensive and correct without needing to know the trigger frequency)
Open questions
-
What fraction of new nixmac users arrive without Homebrew installed? If the fraction is below roughly 30%, the upfront gate imposes friction on the majority for the benefit of the minority, and an on-demand model is likely preferable.
-
What percentage of nixmac's core user tasks require Homebrew-backed packages or casks vs. pure Nix packages and shell configuration? This determines whether upfront or on-demand placement is the right default.
-
If a user dismisses the macOS "Install Command Line Developer Tools" dialog mid-flow, does the step give them actionable instructions to restart it? The current heartbeat only reports elapsed time.
-
Is there a committed timeline for the Homebrew feature-labeling system the step description promises? The copy should not ship without either the feature or a rewrite that removes the promise.
-
Has the team evaluated whether the evolve loop could detect a brew-requiring configuration and surface the Homebrew install offer at that point, making the step on-demand rather than upfront?
Recommendation
Discuss first
Two issues require resolution before this ships. First, a reviewer asked directly in the PR conversation whether on-demand placement is preferable to the current upfront gate; that question has no response and must be answered with user data or a product commitment before the step order is locked. Second, the step description promises a Homebrew feature-labeling system that does not yet exist; that copy should not ship without either the feature or a rewrite that removes the promise.
![prelint[bot]](https://avatars.githubusercontent.com/in/2822344?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
homebrewInstalled is a system probe result — prerequisite health — which the onboarding state ADR's state taxonomy explicitly classifies as "Derived from probes — existing ViewModel slices" (see tab...
packages/state/src/onboarding/store.ts:34
1 finding(s) posted as inline comments.
| homebrewInstalled: null, | ||
| homebrewSkipped: false, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Homebrew presence is prerequisite health, which the onboarding state ADR places in probed ViewModel slices, not the session-only Zustand store. It was in the store, and the UI kept a second copy as component state derived from the installer's exit code. Two sources for one fact produced the review's headline bugs. Adds a `HomebrewInstallState` observable cell (`state/homebrew_state.rs`) mirrored through `viewmodel/homebrew.ts`, following the `nix_install_state` pattern. The installer output stream folds into `viewModel.homebrewLog` the way `rebuildLog` already does. Closes four review comments with one change: - Leaving the step mid-install no longer loses the run. `installing` and the current phase live on the backend, so navigating away and back finds both the run and its log intact. - The "installed with no way forward" dead end is gone. The backend probes `brew` when a run ends and records *that*, never the exit code, so the UI cannot render success over a gate that still blocks. - Skip stays reachable while installing. The Command Line Tools wait can sit on an Apple dialog for up to 45 minutes; force-quitting was the only exit. - Detection runs at app mount beside the Nix and permissions probes, so users who already have brew never render the step. Also from review: - Fetch the install script before running it. `bash -c "$(curl ...)"` exits 0 when the download fails, reporting a successful install that left no brew behind — the transient-network path into the dead end above. - Guard against concurrent runs with an `AtomicBool`; a double invoke spawned two installers and two password dialogs. - Emit the typed `HomebrewInstall*Event` structs instead of ad-hoc `json!`. - A dismissed password dialog returns to the offer state rather than the red failed state — a deliberate cancel is not an error. - Fold the e2e mock gating into `system::homebrew::is_installed()` so both callers share one definition. `STEP_ORDER` in onboarding-flow.stories.tsx never learned about the Homebrew gate, so three unrelated stories were parked on it — the storybook diff on the PR. Fixed, with the new slice seeded in both story files. Tests: 350 pass (was 344). New cases cover the dead end, the cancel path, Skip during install, and the CLT phase label. Claude-Session: https://claude.ai/code/session_01B6hUFgC8sD6P5nt3e1KiJh
… (ENG-490) Replaces driving `install.sh` with a relayed password. Review asked for the privileged-helper instead of a bespoke sudo path; the suggested route -- pre-create the prefix so the script has nothing left to sudo -- turns out not to work, but the .pkg it also suggested does, and better. Why the script cannot be made sudo-free: on macOS `install.sh` calls `have_sudo_access` unconditionally near the top (line ~530), before it looks at the prefix at all, and that function `abort`s when sudo is unusable. Its `chown -R` on HOMEBREW_REPOSITORY is unconditional too, not gated on the computed lists. And `check_run_command_as_root` refuses to run as root. There is no arrangement of a pre-created /opt/homebrew that gets past all three. Homebrew's official package sidesteps the question: - `auth="root"`, which the helper daemon already is -- no password prompt, no sudo, no credential relay. - Carries its whole payload (3797 files), so there is no fetch-a-script step that can fail into a "successful" install. - Its postinstall relocates to /usr/local/Homebrew on Intel, so one package is correct on both architectures. - Signed and notarized by Homebrew's own Developer ID. New argument-free `HelperRequest::InstallHomebrew`. Argument-free is the point: the helper picks the URL, and derives the owning account from the socket peer's credentials, so nothing the caller says can redirect what root installs. It downloads into root's own temp directory, verifies the signature *there*, and installs from there -- the requesting user never has a writable path in the sequence, so the verified bytes cannot be swapped before use. Verification requires a Developer ID Installer certificate for Homebrew's team on the *leaf* of the chain, plus Apple notarization. Matching the team ID anywhere in the report would let it appear on a deeper certificate or in an unrelated field; six tests cover the accept and reject paths, against verbatim `pkgutil --check-signature` output. Ownership: the package scripts otherwise hand the install to whoever owns /dev/console. A root-owned 0600 plist pins it to the peer account instead -- written only when absent, since that path is also how MDM pins an install user and clobbering an administrator's choice would be worse. Removes SudoAskpass, prime_sudo, the keepalive thread, the osascript password dialog, and the streamed script runner (~270 lines, approved). The Command Line Tools step stays: the package's installation_check requires /Library/Developer/CommandLineTools/usr/bin/git and fails fatally without it. Tests: 810 Rust (up 6), 350 TS. Not yet run on a VM -- the install path itself is unexercised. Claude-Session: https://claude.ai/code/session_01B6hUFgC8sD6P5nt3e1KiJh
`bundleBinaries` walked the bundle with `statSync`, which follows symlinks and throws ENOENT on a dangling one. Framework layouts are built out of symlinks (`A.framework/A` -> `Versions/Current/A`), so a single broken link aborted the entire check with an unhandled exception instead of reporting a result -- the check was one bad symlink away from never running at all. Walk with `lstatSync` and skip symlinks outright. Anything they point at inside the bundle is reached by walking the real file, so coverage is unchanged: both versions find the same 3 Mach-O binaries in the release bundle (nixmac, nixmac-helper, nixmac-sync-agent). Verified against a bundle carrying two dangling framework symlinks: the previous version died with ENOENT, this one completes. Raised in review on check-bundle-portability.mjs:48. Claude-Session: https://claude.ai/code/session_01B6hUFgC8sD6P5nt3e1KiJh
`check:portability` was added with nothing invoking it, so a build-host dylib leak could reach main unnoticed -- flagged in review as "nothing runs this yet". Run it in the macOS build job immediately after `desktop:build`, and in `desktop:build:local` before `sign:local-app`. This does not replace ops/scripts/release/check-portable-macos-app.sh, which stays where it is and remains the stricter gate: it also inspects rpaths and symlinks, and covers the DMG and the updater archive rather than just the .app. It cannot run any earlier, though, because it needs those artifacts to exist, which puts it after certificate import, signing and notarization. The node check needs only the freshly built bundle, so it fails the job in seconds rather than ~20 minutes in. The local hook is the case CI cannot cover at all: `desktop:build:local` is what a developer runs on their own machine, which is exactly where a package manager prefix winning pkg-config (the /opt/local libiconv breakage) comes from in the first place. Claude-Session: https://claude.ai/code/session_01B6hUFgC8sD6P5nt3e1KiJh
|
I see a lot of the issues I brought up fixed, which I very much appreciate. But CI is red, can you fix this? And I would like to give it a run in a VM. For this I'd like to download the CI-produced .dmg, but the red CI blocks this. |
3 participants
Adds an optional guided Homebrew installer to first-run onboarding (ENG-490), for
users who arrive on a Mac without
brew.The gate is optional: onboarding advances once brew is detected OR the user
skips for the session (
homebrewReady || homebrewSkipped). Step order:This is the only code path that installs Homebrew. The existing homebrew
subsystem (
homebrew_apply_diff/homebrew_get_state_diff/homebrew_add_items)only adopts and diffs packages and assumes brew is already present. No overlap.
How the install works now
Homebrew ships an official signed
.pkg.The app asks the existing
privileged_helperto install it, so there is nosecond privilege-escalation mechanism in the codebase:
HelperRequest::InstallHomebrewover the existinghelper socket. The helper derives the target user from the socket peer
credentials, exactly as
authorize_requestalready does — the app cannot askit to install as somebody else.
.pkginto a root-owned0700work dir, then verifies it before running it.installer -pkg … -target /, with/var/tmp/.homebrew_pkg_user.plistpinningthe owner to the invoking user.
WorkDirandHomebrewPkgUserare RAIIguards, so both are cleaned up on every exit path.
Signature verification is the security-critical part, so it is a pure function
(
check_signature_report) with 6 unit tests over verbatimpkgutiloutput. Itrequires both:
1. Developer ID Installer:ending in Homebrew'steam ID
(927JGANW46)— anchoring on the leaf line specifically, so a matchingteam ID appearing deeper in the chain is rejected
trusted by the Apple notary serviceTests cover: valid package, unsigned, another developer's package, un-notarized,
team ID on a deeper certificate, and team ID away from the leaf.
Review feedback
SudoAskpass,prime_sudo,spawn_sudo_keepalive,prompt_password,run_installer_streamedare gone (−325 lines); install goes throughprivileged_helperHelperRequest::PrepareHomebrewPrefix.pkg"HomebrewInstallStatecell mirrored through thehomebrewViewModel modulehomebrewInstalledviolates the onboarding state ADR"installing"curl | bashpath is deletedcheckHomebrew()runs in the app mount probe sequence, so brew-havers never render the stepAtomicBoolinsystem::homebrewjson!emits instead of typed eventscommands/homebrew.rssystem::homebrewcheck:portabilityisn't run by anythingdesktop:buildand indesktop:build:localstatSyncthrows on broken symlinkslstatSync+ skip; verified against a bundle with dangling framework symlinks, which crashed the old version with ENOENTWhy pre-creating the prefix doesn't work
The suggestion was that pre-creating
/opt/homebrewwould leaveinstall.shnothing to
sudo. Reading the script, that isn't the case — there are threeindependent blockers, not one:
have_sudo_accessis called unconditionally near line 530, before anyprefix inspection, and
aborts on macOS regardless of what already existschown -Rnear line 822 is unconditional, not guarded by a computed listcheck_run_command_as_rootrefuses to run as rootSo the prefix trick removes none of them. The
.pkgsuggestion, on the otherhand, sidesteps all three.
Findings from clean-VM testing
Two findings from a genuinely Homebrew-less VM, neither of which reproduces on a
dev machine. Both fixes are still in this PR.
Command Line Tools install hangs forever
install.shinstalls CLT via a headlesssoftwareupdate -i. Measured fromoutside the VM, that transferred zero bytes over 12 minutes with no output,
error, or exit — while
curlon the same machine reached Apple at 411 KB/s.softwareupdate -ixcode-select --installFixed by installing CLT via
xcode-select --install, with progress heartbeatsand a 45-minute timeout (
CLT_WAIT_TIMEOUT) that fails with actionable text. Thetimeout is deliberately generous: the working download sat flat for 2¼ minutes
before reaching full speed, so a tighter check would abort installs that were
about to succeed.
Bundles linked build-host libraries and crashed on clean Macs
libz-sysprobes withpkg-config. With a package-manager prefix ahead of thesystem toolchain on PATH it resolved
/opt/local/lib/libz, dragging-L/opt/local/libinto the link and silently binding GNU libiconv, whose_libiconv_opendoes not exist in Apple's/usr/lib/libiconv.2.dylib. Thebundle ran fine on the build machine and died instantly elsewhere:
Fixed by declaring
libz-syswith thestaticfeature so it unifies across thegraph (
git2 → libgit2-sys/libssh2-sys → libz-sys) and the pkg-config probedisappears. Verified by rebuilding with the package-manager prefix first on
PATH and no env overrides, so the fix doesn't depend on anyone remembering build
flags.
Test Plan
Automated, all green at
8260bef2:bun run test:unit— 355 passed / 355, 52 filescargo test— Homebrew suites pass, including all 6check_signature_reportcases;
privileged_helpersuite greentsc --noEmit— cleancargo check --all-targets— cleanoxlint— 0 errorsbun run check:portabilityon the release bundle — clean acrossnixmac,nixmac-helperandnixmac-sync-agent, confirmed independently withotool -LManual, on a Homebrew-less macOS 14.6.1 arm64 VM (non-root, no CLT, no brew) —
note this exercised the previous sudo-based implementation, and is retained
here because it is what produced the CLT finding above:
To review the new path manually you need a signed build, because the helper only
registers via SMAppService for a signed app in
/Applications:bun run desktop:build:local # bashbun run desktop:build:local # csh/tcsh — same commandthen launch from
/Applicationson a brew-less machine and walk onboarding tothe Homebrew step.
Not verified
The
.pkginstall path has not been exercised end to end on a VM. Localsigning needs
sopsplusops/secrets/secrets.sops.json, neither of which isavailable on my machine, and an unsigned bundle cannot register the privileged
helper the new path depends on. The signature-verification logic is unit-tested
against verbatim
pkgutiloutput, but the download → verify →installerchainhas only been reviewed, not run. Worth a signed CI build before merge.
Also still untested: skip, install failure, and re-detect-after-install.
Open questions
inferenceDeferred. prelint argues the CLT download can push onboardingacross sessions, so a user who skips may be asked again. A persisted
homebrew_skip_decidedonOnboardingStatewould fix it — happy to add ifyou agree it's worth the durable state.
on this: Nix-first users get asked a technical question before they have seen
the product work. Alternatives are on-demand install or a non-blocking hint.
Deliberately not decided unilaterally — this is a product call.