diff --git a/.ai/memory.md b/.ai/memory.md index 2c721a5..b99e10a 100644 --- a/.ai/memory.md +++ b/.ai/memory.md @@ -512,3 +512,11 @@ - **`DoctorSeverity.info` does not exist.** The enum is `caution / danger / unsupported` only. Tier 1 is **unreachable from any `Warning:` block in real `brew doctor` output**: Homebrew's `support_tier_message` (Ruby) starts with `return if tier.to_s == "1"`, so a Tier 1 configuration produces no callout text at all. "Tier 1" means "fully supported, no editorial needed" — brew doesn't print anything about it. The parser's regex is intentionally narrowed to `/This is a Tier ([23]) configuration:/` to make this guarantee visible at the source. - **Why this is a source-level guarantee, not a fixture-coverage gap:** when the `.ai/plans/brew-doctor-console.txt` fixture (which is the "all possible warnings" sample) produces no Tier 1 callouts, the first instinct is "fixture is incomplete." It isn't — there is no such callout anywhere in brew's source to capture. Don't re-add `.info` thinking the next fixture expansion will exercise it. If anyone wants a Tier 1-style "info" affordance in the UI, it has to come from a different source (e.g. an `add_info`/`ohai` capture from `brew config`), not the doctor warning stream. - **`.unsupported` is still real.** `support_tier_message` does emit "This is an Unsupported configuration:" for the `unsupported` slug (prerelease / outdated-macOS paths in `check_for_unsupported_macos` resolve there). The fixture happens not to include such a case; the unit test `unsupported configuration trumps any tier` exercises the parser branch. + +## 2026-06-23 — Brew is spawned through the user's login + interactive shell + +- **The problem.** A GUI process launched from Finder/Dock/launchd inherits a stripped environment: missing `PATH` entries and `HOMEBREW_*` vars that `brew shellenv` installs into the user's profile files (`~/.zprofile`). Running `brew config` / `brew doctor` against that stripped env produced output that didn't match what the user sees in Terminal, which destroys trust in the diagnostics. +- **The fix.** All brew invocations now route through `LoginShellBrewCommandRunner` (`Sources/BrewCLI/`), which rewrites `run(executableURL: brew, arguments: [...])` into ` -l -i -c " "`. The login shell is resolved by `LoginShellResolver` from `getpwuid(getuid())->pw_shell` (Directory Services) with a `/bin/zsh` fallback. **`$SHELL` is intentionally not consulted** — it can be stale or absent in a GUI launch. +- **Why `-l` AND `-i`.** `-l` sources `.zprofile` / `.bash_profile` (Homebrew's documented install writes `brew shellenv` to `.zprofile`). `-i` additionally sources interactive rc files (`.zshrc` / `.bashrc`) so users who put their brew setup in those files also get parity. The tradeoff is real and accepted: interactive rc files may print banners or expect a TTY (we run with `/dev/null` stdin, so any TTY-dependent code in rc files will surface as warnings on stderr). If we ever see that as a meaningful noise source for users, the dial is `-l` only. +- **Single spawning route.** Quoting is handled by `LoginShellBrewCommandRunner.singleQuoted` (POSIX `'…'` with `'\''` for embedded apostrophes), so package names and flags survive the shell parse intact. The three production `live()` factories (`BrewCommandExecutionContext.live()`, `BrewConfigRepository.live()`, `BrewInstalledPackagesRepository.live()`) all now construct `LoginShellBrewCommandRunner()`. Tests still use `BrewCommandService()` directly for raw-process plumbing tests — that's correct; the shell wrap is a live-wiring concern. +- **Out of scope (tracked separately).** `brew.env` file parsing and `BrewEnvFileLocator` assume brew sees the same world the user does, which they now do — but parser/locator changes are their own work. diff --git a/Sources/BrewCLI/BrewCommandExecutionContext+Live.swift b/Sources/BrewCLI/BrewCommandExecutionContext+Live.swift index 75795cd..eb3b859 100644 --- a/Sources/BrewCLI/BrewCommandExecutionContext+Live.swift +++ b/Sources/BrewCLI/BrewCommandExecutionContext+Live.swift @@ -7,10 +7,12 @@ import BrewCore import Foundation public extension BrewCommandExecutionContext { - /// Production wiring: real subprocess runner + default `brew` lookup. + /// Production wiring: brew spawned through the user's login + interactive shell so the + /// subprocess inherits the same environment the user sees in Terminal (see + /// ``LoginShellBrewCommandRunner``). static func live() -> BrewCommandExecutionContext { BrewCommandExecutionContext( - commandRunner: BrewCommandService(), + commandRunner: LoginShellBrewCommandRunner(), locator: BrewExecutableLocator(), ) } diff --git a/Sources/BrewCLI/LoginShellBrewCommandRunner.swift b/Sources/BrewCLI/LoginShellBrewCommandRunner.swift new file mode 100644 index 0000000..88b2fda --- /dev/null +++ b/Sources/BrewCLI/LoginShellBrewCommandRunner.swift @@ -0,0 +1,53 @@ +// +// LoginShellBrewCommandRunner.swift +// BrewCLI +// + +import BrewCore +import Foundation + +/// Decorates a ``BrewCommandRunning`` so every brew invocation is executed inside the user's +/// login + interactive shell. Produces parity with what the user sees in Terminal — `brew config` +/// and `brew doctor` are the strict acceptance bar; every other invocation inherits the same +/// environment for free. +/// +/// The wrapper rewrites `run(executableURL: brew, arguments: [...])` into +/// ` -l -i -c " "`. The `-l` flag forces the shell's profile files +/// (`.zprofile`, `.bash_profile`) to load — that is where Homebrew installs `brew shellenv`. The +/// `-i` flag additionally sources interactive rc files (`.zshrc`, `.bashrc`) so users who put +/// their brew setup in those files also get parity; the tradeoff is that interactive rc files may +/// print banners or expect a TTY, which we accept as the cost of exact-Terminal parity. +public struct LoginShellBrewCommandRunner: BrewCommandRunning { + private let underlying: any BrewCommandRunning + private let shellResolver: LoginShellResolver + + public init( + underlying: any BrewCommandRunning = BrewCommandService(), + shellResolver: LoginShellResolver = LoginShellResolver(), + ) { + self.underlying = underlying + self.shellResolver = shellResolver + } + + public func run(executableURL: URL, arguments: [String]) async throws -> CommandOutput { + let shell = shellResolver.resolve() + let command = Self.shellCommand(executableURL: executableURL, arguments: arguments) + return try await underlying.run( + executableURL: shell, + arguments: ["-l", "-i", "-c", command], + ) + } + + /// 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: "'\\''") + "'" + } +} diff --git a/Sources/BrewCLI/LoginShellResolver.swift b/Sources/BrewCLI/LoginShellResolver.swift new file mode 100644 index 0000000..cbad05c --- /dev/null +++ b/Sources/BrewCLI/LoginShellResolver.swift @@ -0,0 +1,52 @@ +// +// LoginShellResolver.swift +// BrewCLI +// + +import Darwin +import Foundation + +/// Resolves the current user's login shell from Directory Services (`getpwuid` reads the same +/// backing store as `dscl . -read /Users/ UserShell`, without a subprocess). +/// +/// Why this exists: a GUI process launched from Finder/Dock/launchd inherits a stripped environment +/// — `$SHELL` may be stale or absent and must not be consulted. The login shell is the input to +/// ``LoginShellBrewCommandRunner``, which then spawns brew under ` -l -i -c` so the user's +/// profile files load and the subprocess sees the same world as Terminal. +public struct LoginShellResolver: Sendable { + /// Default fallback when Directory Services is unreadable — modern macOS ships with zsh as the + /// default user shell. + public static let defaultFallback = URL(fileURLWithPath: "/bin/zsh") + + private let lookup: @Sendable () -> URL? + private let fallback: URL + + public init( + lookup: @escaping @Sendable () -> URL? = LoginShellResolver.directoryServicesLookup, + fallback: URL = LoginShellResolver.defaultFallback, + ) { + self.lookup = lookup + self.fallback = fallback + } + + /// Returns the resolved login shell, falling back to ``defaultFallback`` if the lookup yields nil. + public func resolve() -> URL { + lookup() ?? fallback + } + + /// Native equivalent of `dscl . -read /Users/ UserShell`: queries `getpwuid` for the + /// effective uid and reads `pw_shell`. Returns nil if the call fails or the shell field is empty. + 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) + } +} diff --git a/Sources/BrewRepositories/BrewConfigRepository.swift b/Sources/BrewRepositories/BrewConfigRepository.swift index 0a399e6..af29b23 100644 --- a/Sources/BrewRepositories/BrewConfigRepository.swift +++ b/Sources/BrewRepositories/BrewConfigRepository.swift @@ -44,10 +44,11 @@ public final class BrewConfigRepository: ConfigRepository { self.environment = environment } - /// Production wiring: real subprocess + default `brew` lookup + the live process environment. + /// 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(), ) } diff --git a/Sources/BrewRepositories/BrewInstalledPackagesRepository.swift b/Sources/BrewRepositories/BrewInstalledPackagesRepository.swift index 8b12811..f9732ee 100644 --- a/Sources/BrewRepositories/BrewInstalledPackagesRepository.swift +++ b/Sources/BrewRepositories/BrewInstalledPackagesRepository.swift @@ -56,13 +56,15 @@ public final class BrewInstalledPackagesRepository: InstalledPackagesRepository completionObserverTask?.cancel() } - /// Production wiring: real subprocess + default `brew` lookup, reconciling off `commandCenter`. + /// Production wiring: brew spawned through the user's login + interactive shell + /// (``LoginShellBrewCommandRunner``) so `brew info --installed --json=v2` reads the same world + /// as Terminal. Reconciles off `commandCenter`. public static func live( cache: InstalledInventoryCache, commandCenter: any BrewCommandCenter, ) -> BrewInstalledPackagesRepository { BrewInstalledPackagesRepository( - commandRunner: BrewCommandService(), + commandRunner: LoginShellBrewCommandRunner(), locator: BrewExecutableLocator(), cache: cache, commandCenter: commandCenter, diff --git a/Tests/BrewCLITests/LoginShellBrewCommandRunnerTests.swift b/Tests/BrewCLITests/LoginShellBrewCommandRunnerTests.swift new file mode 100644 index 0000000..9695b07 --- /dev/null +++ b/Tests/BrewCLITests/LoginShellBrewCommandRunnerTests.swift @@ -0,0 +1,132 @@ +// +// LoginShellBrewCommandRunnerTests.swift +// BrewCLITests +// + +@testable import BrewCLI +import BrewCore +import Foundation +import Testing + +struct LoginShellBrewCommandRunnerTests { + // MARK: - Quoting + + @Test func `singleQuoted wraps simple tokens`() { + #expect(LoginShellBrewCommandRunner.singleQuoted("brew") == "'brew'") + } + + @Test func `singleQuoted preserves spaces and metacharacters inside quotes`() { + #expect(LoginShellBrewCommandRunner.singleQuoted("package name") == "'package name'") + #expect(LoginShellBrewCommandRunner.singleQuoted("--flag=$VAR") == "'--flag=$VAR'") + #expect(LoginShellBrewCommandRunner.singleQuoted("a;b|c`d`") == "'a;b|c`d`'") + } + + @Test func `singleQuoted escapes embedded single quotes posix-style`() { + // POSIX-portable: close the quote, emit an escaped quote, reopen. + #expect(LoginShellBrewCommandRunner.singleQuoted("it's") == "'it'\\''s'") + } + + // MARK: - Command construction + + @Test func `shellCommand prefixes the brew path and joins arguments`() { + let command = LoginShellBrewCommandRunner.shellCommand( + executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), + arguments: ["config"], + ) + #expect(command == "'/opt/homebrew/bin/brew' 'config'") + } + + @Test func `shellCommand quotes each argument independently`() { + let command = LoginShellBrewCommandRunner.shellCommand( + executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), + arguments: ["info", "--installed", "--json=v2"], + ) + #expect(command == "'/opt/homebrew/bin/brew' 'info' '--installed' '--json=v2'") + } + + // MARK: - Wrapping behavior + + @Test func `run invokes the resolved login shell with -l -i -c`() async throws { + let recorder = InvocationRecorder() + let wrapped = LoginShellBrewCommandRunner( + underlying: recorder, + shellResolver: LoginShellResolver( + lookup: { URL(fileURLWithPath: "/bin/bash") }, + ), + ) + + _ = try await wrapped.run( + executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), + arguments: ["doctor"], + ) + + let invocation = try #require(await recorder.first) + #expect(invocation.executableURL.path == "/bin/bash") + #expect(invocation.arguments.count == 4) + #expect(invocation.arguments[0] == "-l") + #expect(invocation.arguments[1] == "-i") + #expect(invocation.arguments[2] == "-c") + #expect(invocation.arguments[3] == "'/opt/homebrew/bin/brew' 'doctor'") + } + + @Test func `run falls back to default shell when Directory Services lookup yields nil`() async throws { + let recorder = InvocationRecorder() + let wrapped = LoginShellBrewCommandRunner( + underlying: recorder, + shellResolver: LoginShellResolver(lookup: { nil }), + ) + + _ = try await wrapped.run( + executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), + arguments: ["config"], + ) + + let invocation = try #require(await recorder.first) + #expect(invocation.executableURL.path == LoginShellResolver.defaultFallback.path) + } + + @Test func `run returns the underlying CommandOutput verbatim`() async throws { + let expected = CommandOutput( + standardOutput: "ok", + standardError: "warn", + terminationStatus: 0, + ) + let recorder = InvocationRecorder(stubbedOutput: expected) + let wrapped = LoginShellBrewCommandRunner( + underlying: recorder, + shellResolver: LoginShellResolver(lookup: { URL(fileURLWithPath: "/bin/zsh") }), + ) + + let output = try await wrapped.run( + executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), + arguments: ["config"], + ) + + #expect(output.standardOutput == "ok") + #expect(output.standardError == "warn") + #expect(output.terminationStatus == 0) + } +} + +private struct RecordedInvocation { + let executableURL: URL + let arguments: [String] +} + +private actor InvocationRecorder: BrewCommandRunning { + private(set) var invocations: [RecordedInvocation] = [] + private let stubbedOutput: CommandOutput + + init(stubbedOutput: CommandOutput = CommandOutput(standardOutput: "", standardError: "", terminationStatus: 0)) { + self.stubbedOutput = stubbedOutput + } + + var first: RecordedInvocation? { + invocations.first + } + + func run(executableURL: URL, arguments: [String]) async throws -> CommandOutput { + invocations.append(RecordedInvocation(executableURL: executableURL, arguments: arguments)) + return stubbedOutput + } +} diff --git a/Tests/BrewCLITests/LoginShellResolverTests.swift b/Tests/BrewCLITests/LoginShellResolverTests.swift new file mode 100644 index 0000000..b6e91a8 --- /dev/null +++ b/Tests/BrewCLITests/LoginShellResolverTests.swift @@ -0,0 +1,40 @@ +// +// LoginShellResolverTests.swift +// BrewCLITests +// + +@testable import BrewCLI +import Foundation +import Testing + +struct LoginShellResolverTests { + @Test func `resolve returns lookup value when Directory Services yields a shell`() { + let resolver = LoginShellResolver( + lookup: { URL(fileURLWithPath: "/bin/bash") }, + fallback: URL(fileURLWithPath: "/bin/zsh"), + ) + + #expect(resolver.resolve().path == "/bin/bash") + } + + @Test func `resolve falls back when lookup yields nil`() { + let resolver = LoginShellResolver( + lookup: { nil }, + fallback: URL(fileURLWithPath: "/bin/zsh"), + ) + + #expect(resolver.resolve().path == "/bin/zsh") + } + + @Test func `default fallback is bin zsh`() { + #expect(LoginShellResolver.defaultFallback.path == "/bin/zsh") + } + + @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) + } +}