Use correct shell#74
Open
graeme wants to merge 4 commits into
Open
Conversation
Resolves the current user's login shell from Directory Services via getpwuid(getuid())->pw_shell, with /bin/zsh as fallback. $SHELL is intentionally not consulted because it can be stale or absent in a GUI launch context. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Decorates a BrewCommandRunning so every brew invocation is executed inside the user's login + interactive shell: <login-shell> -l -i -c "<brew> <quoted args>". The -l flag sources profile files (.zprofile / .bash_profile) where Homebrew installs brew shellenv; -i additionally sources interactive rc files (.zshrc / .bashrc) so users who configure brew there also get parity with Terminal. Quoting is POSIX single-quote with the '\'' escape so package names and flags survive intact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The three production live() factories that construct a brew runner (BrewCommandExecutionContext, BrewConfigRepository, BrewInstalledPackagesRepository) now use LoginShellBrewCommandRunner, giving the app a single login-shell spawning path. Tests still use BrewCommandService directly for raw-process plumbing. This is the visible diagnostics fix: brew config and brew doctor reported through BrewUI now match what the user sees in Terminal, and every other brew invocation inherits the same environment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Records why brew is now spawned through the user's login + interactive shell, why -l AND -i (the .zshrc tradeoff), why $SHELL is not consulted, and the single-spawning-route invariant. Per acceptance criterion 5 of the shell-parity plan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates BrewUI’s production Homebrew command execution to run brew through the user’s login + interactive shell so GUI-launched sessions see the same environment as Terminal (improving parity for brew config / brew doctor and other brew calls).
Changes:
- Add
LoginShellResolver(pw_shell via Directory Services) andLoginShellBrewCommandRunner(wraps brew invocations with<shell> -l -i -c ...and token quoting). - Wire the new runner into the three production
live()factories so all brew subprocesses use the shell-wrapped path. - Add unit tests for resolver behavior and runner command construction/quoting; document the
-l -idecision in.ai/memory.md.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/BrewCLITests/LoginShellResolverTests.swift | Adds resolver unit tests (including a live probe test). |
| Tests/BrewCLITests/LoginShellBrewCommandRunnerTests.swift | Adds tests for quoting and shell-wrapped argv construction. |
| Sources/BrewRepositories/BrewInstalledPackagesRepository.swift | Switches production wiring to use LoginShellBrewCommandRunner. |
| Sources/BrewRepositories/BrewConfigRepository.swift | Switches production wiring to use LoginShellBrewCommandRunner. |
| Sources/BrewCLI/LoginShellResolver.swift | Introduces resolver for the current user’s login shell. |
| Sources/BrewCLI/LoginShellBrewCommandRunner.swift | Introduces the command-runner decorator that executes brew via the login shell. |
| Sources/BrewCLI/BrewCommandExecutionContext+Live.swift | Updates live execution context wiring to use the new runner. |
| .ai/memory.md | Documents rationale/tradeoffs for -l -i and the new spawning route. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+39
to
+51
| public static let directoryServicesLookup: @Sendable () -> URL? = { | ||
| guard let entry = getpwuid(getuid()) else { | ||
| return nil | ||
| } | ||
| guard let cString = entry.pointee.pw_shell else { | ||
| return nil | ||
| } | ||
| let path = String(cString: cString) | ||
| guard !path.isEmpty else { | ||
| return nil | ||
| } | ||
| return URL(fileURLWithPath: path) | ||
| } |
Comment on lines
+41
to
+52
| /// Joins the brew executable path and its arguments into a single shell-safe command string, | ||
| /// suitable for passing as the argument to `sh -c` / `zsh -c`. Each token is wrapped in single | ||
| /// quotes; any embedded single-quote is escaped as `'\''` (POSIX-portable). This keeps package | ||
| /// names and flags that contain spaces, `$`, backticks, or other metacharacters intact. | ||
| static func shellCommand(executableURL: URL, arguments: [String]) -> String { | ||
| let tokens = [executableURL.path] + arguments | ||
| return tokens.map(singleQuoted).joined(separator: " ") | ||
| } | ||
|
|
||
| static func singleQuoted(_ string: String) -> String { | ||
| "'" + string.replacingOccurrences(of: "'", with: "'\\''") + "'" | ||
| } |
Comment on lines
+33
to
+39
| @Test func `directoryServicesLookup returns the running user's login shell when present`() { | ||
| // Live probe — on macOS CI and dev machines the running user always has a pw_shell entry. | ||
| // The value itself varies (zsh, bash, fish, …), so we only assert non-nil and absolute path. | ||
| let resolved = LoginShellResolver.directoryServicesLookup() | ||
| try? #require(resolved != nil) | ||
| #expect(resolved?.path.hasPrefix("/") == true) | ||
| } |
Comment on lines
+47
to
53
| /// Production wiring: brew is spawned through the user's login + interactive shell | ||
| /// (``LoginShellBrewCommandRunner``) so `brew config` reflects the same environment as Terminal. | ||
| public static func live() -> BrewConfigRepository { | ||
| BrewConfigRepository( | ||
| commandRunner: BrewCommandService(), | ||
| commandRunner: LoginShellBrewCommandRunner(), | ||
| locator: BrewExecutableLocator(), | ||
| ) |
MikeMcQuaid
approved these changes
Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR: Run brew through the user's login + interactive shell
Summary
brew configandbrew doctorreported through BrewUI didn't match what users see in Terminal, because a GUI process launched from Finder/Dock/launchd inherits a stripped environment — missing thePATHentries andHOMEBREW_*vars that only exist after a login shell sources the user's profile files. This PR routes every brew invocation through<login-shell> -l -i -c "<brew> <quoted args>", fixing the diagnostics outright and giving every other brew call (install, upgrade, info) the same environment for free.Changes
New types in
BrewCLI:LoginShellResolver— resolves the current user's login shell from Directory Services viagetpwuid(getuid())->pw_shell, with/bin/zshas fallback.$SHELLis intentionally not consulted because it can be stale or absent in a GUI launch context.LoginShellBrewCommandRunner—BrewCommandRunningdecorator that rewritesrun(executableURL: brew, arguments: [...])into<login-shell> -l -i -c "<brew> <quoted args>". Quoting uses POSIX single-quote ('…'with'\''for embedded apostrophes) so package names and flags with spaces,$, backticks, etc. survive the shell parse intact.Live wiring (single spawning route):
BrewCommandExecutionContext.live(),BrewConfigRepository.live(), andBrewInstalledPackagesRepository.live()now constructLoginShellBrewCommandRunner()instead ofBrewCommandService()directly. These are the only three production sites that construct a brew runner.BrewCommandService()directly for raw-process plumbing — that's intentional; the shell wrap is a live-wiring concern.Decision documented:
.ai/memory.mdrecords why we ship both-land-i(Terminal-exact parity, including users who putbrew shellenvin.zshrc), and the tradeoff we accept (rc files may print banners or emit TTY warnings on stderr).Why this split (optional)
Stacked as four small commits, one per slice of the requirements plan, so each is reviewable on its own:
LoginShellResolver(pure, independently testable, no UI surface)LoginShellBrewCommandRunner(decorator + quoting + argv construction)live()factories (the demoable diagnostics fix)-l -idecision in.ai/memory.mdTesting
scripts/test— full BrewKit + BrewUILint test suite, 617 + 19 tests passmint run swiftformat --lint .cleanmint run swiftlint lint --strict— 0 violationsBrewUILintoverBrew+Sources— 0 violations-l -i -cargv, fallback path, output passthrough)brew configoutput matchesbrew configin Terminal (acceptance criterion 1)brew doctorin Terminal — no phantom warnings from a stripped environment (acceptance criterion 2)PR checklist
Claude Code (Opus 4.7) implemented the four-slice plan end-to-end: read the existing
BrewCommandService/BrewExecutableLocator/live()wiring, wroteLoginShellResolver+LoginShellBrewCommandRunner+ their unit tests, swept the three productionlive()factories, and drafted the.ai/memory.mdentry. Manual verification performed: ranscripts/test,mint run swiftformat --lint .,mint run swiftlint lint --strict, andBrewUILintover the production tree — all green. The-l -i(vs-lonly) decision was an explicit human call before implementation. The Finder-launched diagnostics checks above are still to be done manually before merge.Follow-ups (optional)
-iflag turns out to surface noisy banners or TTY warnings from real users' rc files, the dial is to drop back to-lonly (one-line change inLoginShellBrewCommandRunner). (cc @MikeMcQuaid : Maybe I should just do this off the bat?)