From 0854aca577f73691ed94bb91ae16ad6940e7d5c3 Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Tue, 15 Sep 2026 10:03:47 +0100 Subject: [PATCH] Run Homebrew through isolated zsh - Read user configuration from `brew.env`, independent of shell setup. - Filter system startup banners while preserving startup diagnostics. - Drain terminal output before closing its descriptors to prevent loss. - Use the same restricted environment for app self-upgrades. - Document the policy and configure live CI through `brew.env`. --- .ai/memory.md | 18 ++ .github/workflows/e2e.yml | 7 + ARCHITECTURE.md | 4 + BrewUITests/E2E/Brew.swift | 3 +- BrewUITests/E2E/BrewE2EApp.swift | 5 +- BrewUITests/E2E/README.md | 4 + Homebrew/BrewApp.swift | 3 +- .../HelperSelfUpgradeHandoff.swift | 2 - HomebrewUpgradeHelper/UpgradeHelper.swift | 1 - README.md | 36 +++ .../BrewCommandExecutionContext+Live.swift | 6 +- ...rewCommandExecutionContext+UITesting.swift | 3 +- Sources/BrewCLI/BrewCommandService.swift | 15 +- .../Config/BrewConfigEnvironmentReader.swift | 2 +- .../BrewCLI/LoginShellBrewCommandRunner.swift | 133 -------- Sources/BrewCLI/LoginShellResolver.swift | 68 ---- Sources/BrewCLI/PseudoTerminal.swift | 6 +- Sources/BrewCLI/ZshBrewCommandRunner.swift | 101 ++++++ .../HomebrewEnvironmentReading.swift | 3 +- .../BrewFeatureConfig/Views/ConfigView.swift | 4 + .../BrewConfigRepository.swift | 5 +- .../BrewInstalledPackagesRepository.swift | 6 +- .../SelfUpgradeHandoffSpec.swift | 5 - .../SelfUpgradeRunner.swift | 13 +- ...rewCommandServicePseudoTerminalTests.swift | 33 ++ .../LoginShellBrewCommandRunnerTests.swift | 303 ------------------ .../LoginShellResolverTests.swift | 40 --- .../ZshBrewCommandRunnerTests.swift | 188 +++++++++++ .../SelfUpgradeHandoffSpecTests.swift | 2 - .../SelfUpgradeRunnerTests.swift | 34 +- 30 files changed, 432 insertions(+), 621 deletions(-) delete mode 100644 Sources/BrewCLI/LoginShellBrewCommandRunner.swift delete mode 100644 Sources/BrewCLI/LoginShellResolver.swift create mode 100644 Sources/BrewCLI/ZshBrewCommandRunner.swift delete mode 100644 Tests/BrewCLITests/LoginShellBrewCommandRunnerTests.swift delete mode 100644 Tests/BrewCLITests/LoginShellResolverTests.swift create mode 100644 Tests/BrewCLITests/ZshBrewCommandRunnerTests.swift diff --git a/.ai/memory.md b/.ai/memory.md index a8b0094..e7a9eb5 100644 --- a/.ai/memory.md +++ b/.ai/memory.md @@ -675,3 +675,21 @@ ## 2026-09-15 — Before and after PR screenshots - The PR template requires before and after screenshots for visible changes, with a comparison table. Changes with no visual impact must explain why screenshots do not apply. + +## 2026-09-15 — Homebrew uses isolated system zsh + +- Supersedes the login-shell policy from 2026-06-23 and 2026-09-09. `ZshBrewCommandRunner` always launches `/bin/zsh --no-rcs --no-global-rcs` with an explicit environment. Login-shell discovery and startup-output filtering are removed. +- `PATH` contains the located brew executable's directory followed by `/usr/bin:/bin`. Identity, home and temporary directories come from Foundation; shell exports, including `HOMEBREW_*` and `XDG_CONFIG_HOME`, are not inherited. Homebrew loads user settings from `brew.env` itself. The README and Configuration tab explain migration and relaunching after edits because the API-mode probe is cached. +- Stock zsh always executes `/etc/zshenv`. A second `env -i` after startup clears its exports before brew. Both environment assignments and brew arguments travel as literal argv, never interpolated shell code. App-owned output controls and explicit fixture variables are retained. +- Self-upgrades use the same runner by default; the handoff no longer carries a shell-selection flag. Deterministic UI tests still invoke the fake executable directly to inherit their fixture environment. +- Live E2E launch variables no longer configure the app. CI writes the deterministic settings into `~/.homebrew/brew.env` on its ephemeral runner; manual-run guidance is in `BrewUITests/E2E/README.md`. Never run the live suite unasked. + +## 2026-09-15 — Filter unavoidable zsh startup output + +- Restores startup-output filtering for `/etc/zshenv`: clearing its exports cannot prevent banners from corrupting JSON or appearing in the console. A per-run marker gates each live stream and trims buffered output, with a leading newline to separate unterminated banners. +- Detect the actual terminal in zsh when emitting markers so allocation fallback marks both pipes. Retain startup diagnostics if the shell exits before emitting a marker. Tests simulate startup at the runner boundary without editing system files. + +## 2026-09-15 — Drain terminal output before closing the replica + +- Darwin discards unread terminal output when the last replica descriptor closes. `BrewCommandService` must keep its replica open until draining finishes, including cancellation and launch failure. Close both descriptors in one `defer`; finish the drain only after a quiet poll that began after child exit. +- The regression test holds the output observer until the child is reaped, then checks buffered and streamed stdout/stderr. This reproduces the output loss without depending on CI scheduling or adding a production test hook. diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 331927a..557273c 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -43,6 +43,13 @@ jobs: TEST_FILTER: ${{ inputs.test_filter }} run: | set -o pipefail + mkdir -p "$HOME/.homebrew" + cat > "$HOME/.homebrew/brew.env" <<'EOF' + HOMEBREW_NO_AUTO_UPDATE=1 + HOMEBREW_NO_ANALYTICS=1 + HOMEBREW_NO_INSTALL_CLEANUP=1 + HOMEBREW_NO_ENV_HINTS=1 + EOF if [[ -n "${TEST_FILTER}" ]]; then scripts/test-e2e -only-testing:"${TEST_FILTER}" | tee xcodebuild-e2e.log else diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 169fff2..40017fa 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -54,6 +54,10 @@ Guiding patterns: Run Homebrew commands **asynchronously** via subprocess; support **cancellation**; **stream or preserve** stdout/stderr for transparency and logs. Always make the **exact command** visible to the user; treat **CLI text output as unstable** (tolerant parsing, fallbacks). +Production commands, including self-upgrades, use system zsh with optional startup files disabled +and an explicit environment. `PATH` is the located brew directory followed by `/usr/bin:/bin`. +Homebrew loads user configuration from `brew.env`; see [configuration and the `/etc/zshenv` exception](README.md#homebrew-configuration). + ## JSON API Use the [Homebrew JSON API](https://formulae.brew.sh/docs/api/) where it helps. Prefer **optional / resilient decoding** — schema can change; **never crash** on unknown fields. Combine with CLI only as needed when the app grows. diff --git a/BrewUITests/E2E/Brew.swift b/BrewUITests/E2E/Brew.swift index 08b0e18..f88410f 100644 --- a/BrewUITests/E2E/Brew.swift +++ b/BrewUITests/E2E/Brew.swift @@ -8,8 +8,7 @@ import Foundation /// Arranges and cleans up live-suite state through the machine's real `brew`, never as the code under /// test: the tests themselves act through the app's own path to brew. nonisolated enum Brew { - /// Applied to every brew this suite runs, the app's included. Without `HOMEBREW_NO_AUTO_UPDATE` an - /// install can spend minutes updating the tap first, which reads as a hung test. + /// Applied to fixture setup and cleanup. CI puts the same settings in `brew.env` for the app. static let determinismEnvironment = [ "HOMEBREW_NO_AUTO_UPDATE": "1", "HOMEBREW_NO_ANALYTICS": "1", diff --git a/BrewUITests/E2E/BrewE2EApp.swift b/BrewUITests/E2E/BrewE2EApp.swift index 1cb58a5..dd216cb 100644 --- a/BrewUITests/E2E/BrewE2EApp.swift +++ b/BrewUITests/E2E/BrewE2EApp.swift @@ -5,15 +5,12 @@ import XCTest -/// Launches with **no** `-uiTesting` argument, so `BrewApp.init()` takes `.live()`: real login shell, +/// Launches with **no** `-uiTesting` argument, so `BrewApp.init()` takes `.live()`: isolated system zsh, /// real `brew`, real network. @MainActor enum BrewE2EApp { static func launch() -> XCUIApplication { let app = XCUIApplication() - for (key, value) in Brew.determinismEnvironment { - app.launchEnvironment[key] = value - } app.launch() BrewApp.activate(app) return app diff --git a/BrewUITests/E2E/README.md b/BrewUITests/E2E/README.md index 58afd8a..d9eb0a0 100644 --- a/BrewUITests/E2E/README.md +++ b/BrewUITests/E2E/README.md @@ -56,6 +56,10 @@ Responsibilities are split: **arrange and clean up by shelling out to real brew* **act through the app's UI** using the same page objects the stubbed suite uses. A broken arrange then reads as a fixture failure rather than as a red assertion inside the flow under test. +The app ignores inherited Homebrew variables. [CI](../../.github/workflows/e2e.yml) writes the +fixture settings to `~/.homebrew/brew.env`. For equivalent manual runs, merge those settings into +your configuration and restore it afterwards. The test harness does not edit your configuration. + ## Requirements - Homebrew installed (`/opt/homebrew/bin/brew` or `/usr/local/bin/brew`) — the suite fails by name in diff --git a/Homebrew/BrewApp.swift b/Homebrew/BrewApp.swift index 5f842df..7a785d9 100644 --- a/Homebrew/BrewApp.swift +++ b/Homebrew/BrewApp.swift @@ -342,12 +342,11 @@ extension BrewApp { } #endif - /// The same locator and login-shell decision every other brew invocation goes through. + /// The same brew locator every other invocation uses. private static func makeHelperHandoff(_ context: SelfUpgradeLaunchContext) -> HelperSelfUpgradeHandoff { HelperSelfUpgradeHandoff( brewExecutableURL: { try context.executionContext.brewExecutableURL() }, commandCenter: context.commandCenter, - usesLoginShell: context.uiTesting == nil, defaultsKeyPrefix: context.selfUpgradeKeyPrefix, relaunchArguments: relaunchArguments(uiTesting: context.uiTesting), relaunchEnvironment: relaunchEnvironment(uiTesting: context.uiTesting), diff --git a/Homebrew/SelfUpgrade/HelperSelfUpgradeHandoff.swift b/Homebrew/SelfUpgrade/HelperSelfUpgradeHandoff.swift index 4c95dfd..c28cc7e 100644 --- a/Homebrew/SelfUpgrade/HelperSelfUpgradeHandoff.swift +++ b/Homebrew/SelfUpgrade/HelperSelfUpgradeHandoff.swift @@ -15,7 +15,6 @@ struct HelperSelfUpgradeHandoff: SelfUpgradeHandoff { let brewExecutableURL: @MainActor () throws -> URL /// Asked before quitting: terminating would kill the `brew` an install is streaming through. let commandCenter: any BrewCommandCenter - let usesLoginShell: Bool let defaultsKeyPrefix: String /// Carried across the relaunch so a UI-test run comes back still pointed at its fixtures. let relaunchArguments: [String] @@ -64,7 +63,6 @@ struct HelperSelfUpgradeHandoff: SelfUpgradeHandoff { relaunchEnvironment: relaunchEnvironment, brewExecutablePath: brewExecutableURL.path, upgradeArguments: BrewCommands.selfUpgrade().arguments, - usesLoginShell: usesLoginShell, upgradeEnvironment: upgradeEnvironment, logFilePath: logFileURL.path, defaultsSuiteName: Bundle.main.bundleIdentifier ?? SelfUpgradeIdentity.bundleIdentifier, diff --git a/HomebrewUpgradeHelper/UpgradeHelper.swift b/HomebrewUpgradeHelper/UpgradeHelper.swift index 3a1346a..a398ae6 100644 --- a/HomebrewUpgradeHelper/UpgradeHelper.swift +++ b/HomebrewUpgradeHelper/UpgradeHelper.swift @@ -45,7 +45,6 @@ struct UpgradeHelper { private func performUpgrade() async -> Bool { log.write("running \(spec.brewExecutablePath) \(spec.upgradeArguments.joined(separator: " "))") let runner = SelfUpgradeRunner( - usesLoginShell: spec.usesLoginShell, transcriptSink: { line in log.write(line) }, ) let outcome = await runner.run( diff --git a/README.md b/README.md index 8649279..ca2aebf 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,42 @@ Enable CLI-averse users to safely discover, install, update, and manage Homebrew brew install --cask homebrew-app ``` +## Homebrew configuration + +BrewUI always launches Homebrew through `/bin/zsh`, including app self-upgrades. It disables +optional user and system shell startup files with `--no-rcs --no-global-rcs` and supplies a clean environment. +`PATH` contains only the directory of the located `brew` executable followed by `/usr/bin:/bin`. +Your login shell, shell aliases, exported variables and custom `PATH` do not configure Homebrew in BrewUI. + +**Put your Homebrew configuration variables in `brew.env` files.** Homebrew reads these itself: + +| Scope | File | +| --- | --- | +| User | `~/.homebrew/brew.env` | +| Installation | `/etc/homebrew/brew.env` | +| System | `/etc/homebrew/brew.env` | + +For example, add this line to `~/.homebrew/brew.env`: + +```text +HOMEBREW_NO_ENV_HINTS=1 +``` + +Use literal `NAME=value` lines without `export`, shell expansion or command substitution. +User settings normally override installation settings, which override system settings. +`HOMEBREW_SYSTEM_ENV_TAKES_PRIORITY=1` in the system file makes that file take precedence. +See [Homebrew's environment documentation](https://docs.brew.sh/Manpage#environment). +An `XDG_CONFIG_HOME` exported by your shell is also ignored; use the user file above. + +Relaunch BrewUI after changing configuration, then check the Configuration tab. Its report and +Doctor describe Homebrew's environment in the app and may differ from Terminal. BrewUI still +sets output controls for its console and self-upgrade log. + +System zsh always reads `/etc/zshenv`, if present; its execution cannot be disabled. +BrewUI clears the environment again afterwards and discards startup output so banners do not +reach Homebrew's reports or the console. If startup fails before Homebrew runs, its diagnostics are retained. +See [zsh's startup-file documentation](https://zsh.sourceforge.io/Doc/Release/Files.html). + ## 🛠️ Development After cloning: diff --git a/Sources/BrewCLI/BrewCommandExecutionContext+Live.swift b/Sources/BrewCLI/BrewCommandExecutionContext+Live.swift index eb3b859..7a1ff1c 100644 --- a/Sources/BrewCLI/BrewCommandExecutionContext+Live.swift +++ b/Sources/BrewCLI/BrewCommandExecutionContext+Live.swift @@ -7,12 +7,10 @@ import BrewCore import Foundation public extension BrewCommandExecutionContext { - /// 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``). + /// Production wiring: system zsh, a restricted PATH and settings loaded by Homebrew from `brew.env`. static func live() -> BrewCommandExecutionContext { BrewCommandExecutionContext( - commandRunner: LoginShellBrewCommandRunner(), + commandRunner: ZshBrewCommandRunner(), locator: BrewExecutableLocator(), ) } diff --git a/Sources/BrewCLI/BrewCommandExecutionContext+UITesting.swift b/Sources/BrewCLI/BrewCommandExecutionContext+UITesting.swift index e4ec9bb..0552bf1 100644 --- a/Sources/BrewCLI/BrewCommandExecutionContext+UITesting.swift +++ b/Sources/BrewCLI/BrewCommandExecutionContext+UITesting.swift @@ -8,8 +8,7 @@ import Foundation public extension BrewCommandExecutionContext { /// The real ``BrewCommandService`` pointed at a fake `brew`, so spawning, pipes and streaming stay - /// under test. Deliberately *not* ``LoginShellBrewCommandRunner``: wrapping the fake in the - /// developer's login shell would source their dotfiles and make runs machine-dependent. + /// under test. The fake inherits the fixture environment published by the UI-test installer. /// /// `nil` resolves nothing, which drives the brew-not-found surfaces and stops a launch that named /// no fake from falling through to a real Homebrew install. diff --git a/Sources/BrewCLI/BrewCommandService.swift b/Sources/BrewCLI/BrewCommandService.swift index bec7b37..6a36518 100644 --- a/Sources/BrewCLI/BrewCommandService.swift +++ b/Sources/BrewCLI/BrewCommandService.swift @@ -79,6 +79,10 @@ private extension BrewCommandService { arguments: [String], options: BrewRunOptions, ) async throws -> CommandOutput { + defer { + terminal.closeReplica() + terminal.closePrimary() + } let childHasExited = TerminalDrainGate() let sink = options.lineObserver @@ -95,15 +99,10 @@ private extension BrewCommandService { input: .none, output: .fileDescriptor(terminal.replicaDescriptor, closeAfterSpawningProcess: false), error: .fileDescriptor(terminal.replicaDescriptor, closeAfterSpawningProcess: false), - body: { _ in - // Runs once the child is spawned, the only safe moment to drop our replica copy. - terminal.closeReplica() - }, ) childHasExited.open() let transcript = await drained - terminal.closePrimary() try Task.checkCancellation() return CommandOutput( @@ -113,10 +112,8 @@ private extension BrewCommandService { ) } catch { // Reap the drain before rethrowing, so no thread is left parked on the primary. - terminal.closeReplica() childHasExited.open() _ = await drained - terminal.closePrimary() if error is CancellationError { throw error @@ -144,6 +141,8 @@ private extension BrewCommandService { var readFailure: Int32? loop: while true { + // Only a quiet interval after exit can prove the final output has been drained. + let childHadExited = gate.isOpen switch terminal.read() { case let .data(chunk): undecoded.append(chunk) @@ -152,7 +151,7 @@ private extension BrewCommandService { emit(event, sink: sink) } case .timedOut: - if gate.isOpen { + if childHadExited { break loop } case .endOfInput: diff --git a/Sources/BrewCLI/Config/BrewConfigEnvironmentReader.swift b/Sources/BrewCLI/Config/BrewConfigEnvironmentReader.swift index 27f7c82..cfeff2f 100644 --- a/Sources/BrewCLI/Config/BrewConfigEnvironmentReader.swift +++ b/Sources/BrewCLI/Config/BrewConfigEnvironmentReader.swift @@ -7,7 +7,7 @@ import BrewCore import Foundation /// Asks `brew config`, which prints `HOMEBREW_NO_INSTALL_FROM_API: set` only when it is set. -/// Probed once: changing it means editing a shell profile, which needs a relaunch anyway. +/// Probed once per app launch. Relaunch after changing this setting in `brew.env`. public actor BrewConfigEnvironmentReader: HomebrewEnvironmentReading { private static let noInstallFromAPIKey = "HOMEBREW_NO_INSTALL_FROM_API" diff --git a/Sources/BrewCLI/LoginShellBrewCommandRunner.swift b/Sources/BrewCLI/LoginShellBrewCommandRunner.swift deleted file mode 100644 index 27a4c12..0000000 --- a/Sources/BrewCLI/LoginShellBrewCommandRunner.swift +++ /dev/null @@ -1,133 +0,0 @@ -// -// 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 - private let makeMarker: @Sendable () -> String - - public init( - underlying: any BrewCommandRunning = BrewCommandService(), - shellResolver: LoginShellResolver = LoginShellResolver(), - makeMarker: @escaping @Sendable () -> String = { UUID().uuidString }, - ) { - self.underlying = underlying - self.shellResolver = shellResolver - self.makeMarker = makeMarker - } - - public func run( - executableURL: URL, - arguments: [String], - options: BrewRunOptions, - ) async throws -> CommandOutput { - var options = options - let shell = shellResolver.resolve() - let marker = makeMarker() - let shellCommand = Self.shellCommand(for: shell, marker: marker, output: options.output) - let shellArguments = Self.shellArguments(executableURL: executableURL, arguments: arguments) - - if let lineObserver = options.lineObserver { - let gate = LoginShellStartupGate(marker: marker) - options.lineObserver = { line in - if let admitted = gate.admit(line) { - lineObserver(admitted) - } - } - } - - // Forward `options` so the wrapped subprocess still streams + colours — the default protocol - // implementation would drop it, silently disabling colour on the production login-shell path. - let output = try await underlying.run( - executableURL: shell, - arguments: ["-l", "-i", "-c", shellCommand] + shellArguments, - options: options, - ) - return CommandOutput( - standardOutput: Self.removingStartupNoise(from: output.standardOutput, upTo: marker), - standardError: Self.removingStartupNoise(from: output.standardError, upTo: marker), - terminationStatus: output.terminationStatus, - ) - } - - /// Printed to the streams the child will actually use, right after `-l -i` startup and before - /// `exec`, so ``removingStartupNoise(from:upTo:)`` has an exact line to cut rc-file noise at. - static func shellCommand(for shell: URL, marker: String, output: BrewRunOptions.OutputChannel) -> String { - let announce = switch output { - case .pipes: - "printf '%s\\n' '\(marker)' 1>&2; printf '%s\\n' '\(marker)'; " - case .pseudoTerminal: - "printf '%s\\n' '\(marker)'; " - } - let exec = shell.lastPathComponent == "fish" ? "exec $argv" : "exec \"$0\" \"$@\"" - return announce + exec - } - - static func shellArguments(executableURL: URL, arguments: [String]) -> [String] { - [executableURL.path] + arguments - } - - static func removingStartupNoise(from text: String, upTo marker: String) -> String { - guard let markerRange = text.range(of: marker) else { - return text - } - let remainder = text[markerRange.upperBound...] - guard let newline = remainder.firstIndex(of: "\n") else { - return String(remainder) - } - return String(remainder[remainder.index(after: newline)...]) - } -} - -/// Drops every live console line up to and including the login-shell startup marker, so streamed -/// output skips the same rc-file noise ``LoginShellBrewCommandRunner`` strips from the buffered result. -// swiftlint:disable:next unchecked_sendable -private final class LoginShellStartupGate: @unchecked Sendable { - private let lock = NSLock() - private let marker: String - private var admittingStdout = false - private var admittingStderr = false - - init(marker: String) { - self.marker = marker - } - - func admit(_ line: BrewCommandOutputLine) -> BrewCommandOutputLine? { - lock.lock() - defer { lock.unlock() } - let admitting = switch line.stream { - case .stdout: - admittingStdout - case .stderr: - admittingStderr - } - if admitting { - return line - } - if line.text == marker { - switch line.stream { - case .stdout: - admittingStdout = true - case .stderr: - admittingStderr = true - } - } - return nil - } -} diff --git a/Sources/BrewCLI/LoginShellResolver.swift b/Sources/BrewCLI/LoginShellResolver.swift deleted file mode 100644 index f233e3d..0000000 --- a/Sources/BrewCLI/LoginShellResolver.swift +++ /dev/null @@ -1,68 +0,0 @@ -// -// LoginShellResolver.swift -// BrewCLI -// - -import Darwin -import Foundation - -/// Resolves the current user's login shell from Directory Services (`getpwuid_r` 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`, using the thread-safe - /// `getpwuid_r`. Returns nil if the lookup fails or the path isn't a trustworthy absolute path. - public static let directoryServicesLookup: @Sendable () -> URL? = { - guard let path = resolveLoginShellPath() else { return nil } - return URL(fileURLWithPath: path) - } - - /// Does the actual `getpwuid_r` lookup. - private static func resolveLoginShellPath() -> String? { - var pwd = passwd() - var result: UnsafeMutablePointer? - - let suggestedSize = sysconf(_SC_GETPW_R_SIZE_MAX) - let bufferSize = suggestedSize > 0 ? Int(suggestedSize) : 16384 - - var buffer = [CChar](repeating: 0, count: bufferSize) - - let rc: Int32 = buffer.withUnsafeMutableBufferPointer { buf in - guard let baseAddress = buf.baseAddress else { return EINVAL } - return getpwuid_r(getuid(), &pwd, baseAddress, buf.count, &result) - } - - guard rc == 0, let entry = result else { return nil } - guard let cString = entry.pointee.pw_shell else { return nil } - - let path = String(cString: cString) - - guard path.hasPrefix("/") else { return nil } - - return path - } -} diff --git a/Sources/BrewCLI/PseudoTerminal.swift b/Sources/BrewCLI/PseudoTerminal.swift index 990a098..cfd9a72 100644 --- a/Sources/BrewCLI/PseudoTerminal.swift +++ b/Sources/BrewCLI/PseudoTerminal.swift @@ -11,8 +11,8 @@ import System /// A pty pair: the primary (POSIX: master) stays here, the replica (POSIX: slave) becomes the child's /// stdout/stderr and is a real terminal device, so `isatty` holds in the child. /// -/// The replica must be closed in this process once the child is spawned. While any replica descriptor -/// stays open here the kernel sees a potential writer, so reads on the primary never report EOF. +/// Keep the local replica open until output is drained: Darwin discards queued bytes when the last +/// replica closes. The runner uses child exit and a quiet poll interval to finish without waiting for EOF. /// /// The descriptors never change after `init`, so ``read(timeout:)`` touches `primaryFD` unlocked; the /// lock guards only the closed flags, and `read` could not hold it anyway while parked on `poll`. What @@ -60,7 +60,7 @@ final class PseudoTerminal: @unchecked Sendable { return FileDescriptor(rawValue: replicaFD) } - /// Call immediately after the child is spawned; see the ownership note on the type. Idempotent. + /// Call after draining output; see the ownership note on the type. Idempotent. func closeReplica() { lock.lock() defer { lock.unlock() } diff --git a/Sources/BrewCLI/ZshBrewCommandRunner.swift b/Sources/BrewCLI/ZshBrewCommandRunner.swift new file mode 100644 index 0000000..f86c18d --- /dev/null +++ b/Sources/BrewCLI/ZshBrewCommandRunner.swift @@ -0,0 +1,101 @@ +// +// ZshBrewCommandRunner.swift +// BrewCLI +// + +import BrewCore +import Foundation +import Synchronization + +/// Runs brew through system zsh with an explicit environment. Homebrew loads user settings from `brew.env`. +public struct ZshBrewCommandRunner: BrewCommandRunning { + private let underlying: any BrewCommandRunning + + public init(underlying: any BrewCommandRunning = BrewCommandService()) { + self.underlying = underlying + } + + public func run( + executableURL: URL, + arguments: [String], + options: BrewRunOptions, + ) async throws -> CommandOutput { + var environment = [ + "HOME": FileManager.default.homeDirectoryForCurrentUser.path, + "USER": NSUserName(), + "LOGNAME": NSUserName(), + "TMPDIR": FileManager.default.temporaryDirectory.path, + "LANG": "en_US.UTF-8", + ] + switch options.output { + case .pseudoTerminal: + environment["TERM"] = "xterm-256color" + fallthrough + case .pipes(forceColor: true): + // Also covers a pseudo-terminal falling back to pipes. + environment["HOMEBREW_COLOR"] = "1" + environment["CLICOLOR_FORCE"] = "1" + case .pipes(forceColor: false): + break + } + environment.merge(options.environment) { _, override in override } + environment["SHELL"] = "/bin/zsh" + environment["PATH"] = executableURL.deletingLastPathComponent().path + ":/usr/bin:/bin" + let assignments = environment.map { "\($0.key)=\($0.value)" } + let startup = ZshStartupOutput() + var wrappedOptions = options + if let observer = options.lineObserver { + wrappedOptions.lineObserver = { line in + if startup.admit(line) { observer(line) } + } + } + defer { startup.pendingLines.forEach { options.lineObserver?($0) } } + + // /etc/zshenv always runs. Mark its output and clear its exports, keeping argv literal. + // Check the actual terminal so a fallback to pipes gets a marker on each stream. + var output = try await underlying.run( + executableURL: URL(fileURLWithPath: "/usr/bin/env"), + arguments: ["-i"] + assignments + [ + "/bin/zsh", "--no-rcs", "--no-global-rcs", "-c", + "[[ -t 1 ]] || printf '\\n%s\\n' \"$0\" >&2; printf '\\n%s\\n' \"$0\"; exec /usr/bin/env -i \"$@\"", + startup.marker, + ] + assignments + [executableURL.path] + arguments, + options: wrappedOptions, + ) + output.standardOutput = startup.removingBanner(from: output.standardOutput) + output.standardError = startup.removingBanner(from: output.standardError) + return output + } +} + +private final class ZshStartupOutput: Sendable { + let marker = UUID().uuidString + private let state = Mutex<(started: [BrewCommandOutputLine.Stream], pending: [BrewCommandOutputLine])>(([], [])) + + /// Retain diagnostics if startup exits before the marker, including on cancellation. + var pendingLines: [BrewCommandOutputLine] { + state.withLock { $0.pending } + } + + func admit(_ line: BrewCommandOutputLine) -> Bool { + state.withLock { state in + if state.started.contains(line.stream) { + return true + } + if line.isComplete, line.text == marker { + state.started.append(line.stream) + state.pending.removeAll { $0.stream == line.stream } + } else { + state.pending.append(line) + } + return false + } + } + + func removingBanner(from text: String) -> String { + guard let range = text.range(of: "\n\(marker)\n") else { + return text + } + return String(text[range.upperBound...]) + } +} diff --git a/Sources/BrewCore/Operations/HomebrewEnvironmentReading.swift b/Sources/BrewCore/Operations/HomebrewEnvironmentReading.swift index e2ed762..34a45c3 100644 --- a/Sources/BrewCore/Operations/HomebrewEnvironmentReading.swift +++ b/Sources/BrewCore/Operations/HomebrewEnvironmentReading.swift @@ -5,8 +5,7 @@ import Foundation -/// The Homebrew environment as `brew` resolves it. Not `ProcessInfo`: the app is Finder-launched, so -/// a profile-exported `HOMEBREW_*` is invisible to it yet in effect for every brew invocation. +/// The environment Homebrew resolves from its configuration files, independent of the app's environment. public protocol HomebrewEnvironmentReading: Sendable { /// True when brew resolves packages from local tap clones rather than the JSON API. func isInstallFromAPIDisabled() async -> Bool diff --git a/Sources/BrewFeatureConfig/Views/ConfigView.swift b/Sources/BrewFeatureConfig/Views/ConfigView.swift index d7e4ba6..28f95c6 100644 --- a/Sources/BrewFeatureConfig/Views/ConfigView.swift +++ b/Sources/BrewFeatureConfig/Views/ConfigView.swift @@ -65,6 +65,10 @@ struct ConfigView: View { private func loadedCards(snapshot: BrewConfigSnapshot) -> some View { ScrollView { VStack(alignment: .leading, spacing: BrewSpacing.lg) { + NoteCallout(""" + Homebrew settings are read from brew.env files. Shell profiles and exported variables are ignored. \ + Put user settings in ~/.homebrew/brew.env, then relaunch BrewUI. + """, tone: .info) ForEach(viewModel.sections(for: snapshot)) { section in ConfigSectionCard(section: section) } diff --git a/Sources/BrewRepositories/BrewConfigRepository.swift b/Sources/BrewRepositories/BrewConfigRepository.swift index b1b38ac..3b0d255 100644 --- a/Sources/BrewRepositories/BrewConfigRepository.swift +++ b/Sources/BrewRepositories/BrewConfigRepository.swift @@ -41,7 +41,7 @@ public final class BrewConfigRepository: ConfigRepository { } /// Takes the context rather than building its own runner, so the composition root points every - /// brew invocation at one place: production's login shell, or a UI test's fake executable. + /// brew invocation at one place: production's isolated zsh runner, or a UI test's fake executable. public convenience init(executionContext: BrewCommandExecutionContext) { self.init( commandRunner: executionContext.commandRunner, @@ -49,8 +49,7 @@ public final class BrewConfigRepository: ConfigRepository { ) } - /// Production wiring: brew is spawned through the user's login + interactive shell - /// (``LoginShellBrewCommandRunner``) so `brew config` reflects the same environment as Terminal. + /// Production wiring: `brew config` reports the environment used by every app command. public static func live() -> BrewConfigRepository { BrewConfigRepository(executionContext: .live()) } diff --git a/Sources/BrewRepositories/BrewInstalledPackagesRepository.swift b/Sources/BrewRepositories/BrewInstalledPackagesRepository.swift index 2fb5558..7a838aa 100644 --- a/Sources/BrewRepositories/BrewInstalledPackagesRepository.swift +++ b/Sources/BrewRepositories/BrewInstalledPackagesRepository.swift @@ -67,7 +67,7 @@ public final class BrewInstalledPackagesRepository: InstalledPackagesRepository } /// Takes the context rather than building its own runner, so the composition root points every - /// brew invocation at one place: production's login shell, or a UI test's fake executable. + /// brew invocation at one place: production's isolated zsh runner, or a UI test's fake executable. public convenience init( executionContext: BrewCommandExecutionContext, cache: InstalledInventoryCache, @@ -86,9 +86,7 @@ public final class BrewInstalledPackagesRepository: InstalledPackagesRepository completionObserverTask?.cancel() } - /// 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`. + /// Production wiring: the shared zsh execution policy, reconciled off `commandCenter`. public static func live( cache: InstalledInventoryCache, commandCenter: any BrewCommandCenter, diff --git a/Sources/BrewSelfUpgradeContract/SelfUpgradeHandoffSpec.swift b/Sources/BrewSelfUpgradeContract/SelfUpgradeHandoffSpec.swift index 945d6b7..2f4a93d 100644 --- a/Sources/BrewSelfUpgradeContract/SelfUpgradeHandoffSpec.swift +++ b/Sources/BrewSelfUpgradeContract/SelfUpgradeHandoffSpec.swift @@ -17,9 +17,6 @@ public struct SelfUpgradeHandoffSpec: Codable, Sendable, Equatable { public let brewExecutablePath: String /// Built by `BrewCommands.selfUpgrade()`, so the argv the helper runs is what the app displays. public let upgradeArguments: [String] - /// The helper inherits the app's stripped environment, so without the login shell the one command that - /// replaces the app sees none of the `HOMEBREW_*` the user's profile exports. - public let usesLoginShell: Bool /// Empty in production. Under `-uiTesting` this points the fake `brew` at the fixture tree, which the /// helper cannot inherit: it is spawned by the app, but outlives it. public let upgradeEnvironment: [String: String] @@ -40,7 +37,6 @@ public struct SelfUpgradeHandoffSpec: Codable, Sendable, Equatable { relaunchEnvironment: [String: String], brewExecutablePath: String, upgradeArguments: [String], - usesLoginShell: Bool, upgradeEnvironment: [String: String], logFilePath: String, defaultsSuiteName: String, @@ -56,7 +52,6 @@ public struct SelfUpgradeHandoffSpec: Codable, Sendable, Equatable { self.relaunchEnvironment = relaunchEnvironment self.brewExecutablePath = brewExecutablePath self.upgradeArguments = upgradeArguments - self.usesLoginShell = usesLoginShell self.upgradeEnvironment = upgradeEnvironment self.logFilePath = logFilePath self.defaultsSuiteName = defaultsSuiteName diff --git a/Sources/BrewSelfUpgradeHelperCore/SelfUpgradeRunner.swift b/Sources/BrewSelfUpgradeHelperCore/SelfUpgradeRunner.swift index 1667764..885a59b 100644 --- a/Sources/BrewSelfUpgradeHelperCore/SelfUpgradeRunner.swift +++ b/Sources/BrewSelfUpgradeHelperCore/SelfUpgradeRunner.swift @@ -25,7 +25,7 @@ public struct SelfUpgradeRunner: Sendable { private let sleep: @Sendable (TimeInterval) async throws -> Void public init( - commandRunner: any BrewCommandRunning, + commandRunner: any BrewCommandRunning = ZshBrewCommandRunner(), transcriptSink: @escaping @Sendable (String) -> Void = { _ in }, sleep: @escaping @Sendable (TimeInterval) async throws -> Void = { try await Task.sleep(for: .seconds($0)) }, ) { @@ -34,17 +34,6 @@ public struct SelfUpgradeRunner: Sendable { self.sleep = sleep } - /// See ``SelfUpgradeHandoffSpec/usesLoginShell``. - public init( - usesLoginShell: Bool, - transcriptSink: @escaping @Sendable (String) -> Void = { _ in }, - ) { - self.init( - commandRunner: usesLoginShell ? LoginShellBrewCommandRunner() : BrewCommandService(), - transcriptSink: transcriptSink, - ) - } - public func run( executablePath: String, arguments: [String], diff --git a/Tests/BrewCLITests/BrewCommandServicePseudoTerminalTests.swift b/Tests/BrewCLITests/BrewCommandServicePseudoTerminalTests.swift index fb236ec..29fc235 100644 --- a/Tests/BrewCLITests/BrewCommandServicePseudoTerminalTests.swift +++ b/Tests/BrewCLITests/BrewCommandServicePseudoTerminalTests.swift @@ -6,6 +6,7 @@ @testable import BrewCLI import BrewCore import Foundation +import Synchronization import Testing @Suite(.serialized) @@ -41,6 +42,38 @@ struct BrewCommandServicePseudoTerminalTests { #expect(collector.allLines().filter(\.isComplete).map(\.text) == ["one", "two", "three"]) } + @Test func `terminal output survives the child exiting while the observer is busy`() async throws { + let release = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + defer { try? FileManager.default.removeItem(at: release) } + let collector = OutputCollector() + let exitedBeforeReading = Mutex(false) + + let output = try await BrewCommandService().run( + executableURL: URL(fileURLWithPath: "/bin/zsh"), + arguments: [ + "-c", + "printf '%s\\n' $$; while [[ ! -e \"$1\" ]]; do sleep 0.01; done; printf 'out\\n'; printf 'err\\n' >&2; exit 12", + "brewui", release.path, + ], + options: BrewRunOptions(lineObserver: { line in + collector.append(line) + guard line.isComplete, let pid = Int32(line.text) else { return } + _ = FileManager.default.createFile(atPath: release.path, contents: nil) + // Hold the reader until the child is reaped, leaving its final output queued. + let deadline = ContinuousClock.now.advanced(by: .seconds(5)) + while kill(pid, 0) == 0, ContinuousClock.now < deadline { + usleep(1000) + } + exitedBeforeReading.withLock { $0 = kill(pid, 0) == -1 && errno == ESRCH } + }, output: .pseudoTerminal), + ) + + #expect(exitedBeforeReading.withLock { $0 }) + #expect(output.standardOutput.hasSuffix("\nout\nerr\n")) + #expect(output.terminationStatus == 12) + #expect(collector.allLines().filter(\.isComplete).dropFirst().map(\.text) == ["out", "err"]) + } + @Test func `pseudo-terminal run merges stderr into the stdout stream`() async throws { // One device carries both streams, so stderr cannot be told apart. let output = try await run( diff --git a/Tests/BrewCLITests/LoginShellBrewCommandRunnerTests.swift b/Tests/BrewCLITests/LoginShellBrewCommandRunnerTests.swift deleted file mode 100644 index d56f1d3..0000000 --- a/Tests/BrewCLITests/LoginShellBrewCommandRunnerTests.swift +++ /dev/null @@ -1,303 +0,0 @@ -// -// LoginShellBrewCommandRunnerTests.swift -// BrewCLITests -// - -@testable import BrewCLI -import BrewCore -import Foundation -import Testing - -struct LoginShellBrewCommandRunnerTests { - // MARK: - Command construction - - @Test func `shellCommand for posix shells uses positional parameters`() { - let command = LoginShellBrewCommandRunner.shellCommand( - for: URL(fileURLWithPath: "/bin/zsh"), - marker: "MARK", - output: .pipes(forceColor: false), - ) - #expect(command.hasSuffix("exec \"$0\" \"$@\"")) - } - - @Test func `shellCommand for fish uses argv`() { - let command = LoginShellBrewCommandRunner.shellCommand( - for: URL(fileURLWithPath: "/opt/homebrew/bin/fish"), - marker: "MARK", - output: .pipes(forceColor: false), - ) - #expect(command.hasSuffix("exec $argv")) - } - - @Test func `shellCommand announces the marker on both streams for pipes`() { - let command = LoginShellBrewCommandRunner.shellCommand( - for: URL(fileURLWithPath: "/bin/zsh"), - marker: "MARK", - output: .pipes(forceColor: false), - ) - #expect(command == "printf '%s\\n' 'MARK' 1>&2; printf '%s\\n' 'MARK'; exec \"$0\" \"$@\"") - } - - @Test func `shellCommand announces the marker once for a pseudo-terminal`() { - let command = LoginShellBrewCommandRunner.shellCommand( - for: URL(fileURLWithPath: "/bin/zsh"), - marker: "MARK", - output: .pseudoTerminal, - ) - #expect(command == "printf '%s\\n' 'MARK'; exec \"$0\" \"$@\"") - } - - @Test func `removingStartupNoise cuts everything through the marker's line`() { - let text = "hello from zshrc\nMARK\n{\"foo\":1}\n" - #expect(LoginShellBrewCommandRunner.removingStartupNoise(from: text, upTo: "MARK") == "{\"foo\":1}\n") - } - - @Test func `removingStartupNoise leaves text unchanged when the marker is absent`() { - let text = "{\"foo\":1}\n" - #expect(LoginShellBrewCommandRunner.removingStartupNoise(from: text, upTo: "MARK") == text) - } - - @Test func `removingStartupNoise returns empty when the marker is the last line`() { - let text = "startup noise\nMARK\n" - #expect(LoginShellBrewCommandRunner.removingStartupNoise(from: text, upTo: "MARK") == "") - } - - @Test func `shellArguments prefixes the brew path and appends arguments`() { - let arguments = LoginShellBrewCommandRunner.shellArguments( - executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), - arguments: ["info", "--installed", "--json=v2"], - ) - #expect(arguments == ["/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") }, - ), - makeMarker: { "MARK" }, - ) - - _ = 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 == 6) - #expect(invocation.arguments[0] == "-l") - #expect(invocation.arguments[1] == "-i") - #expect(invocation.arguments[2] == "-c") - #expect(invocation.arguments[3] == "printf '%s\\n' 'MARK' 1>&2; printf '%s\\n' 'MARK'; exec \"$0\" \"$@\"") - #expect(invocation.arguments[4] == "/opt/homebrew/bin/brew") - #expect(invocation.arguments[5] == "doctor") - } - - @Test func `run uses fish-compatible exec script when login shell is fish`() async throws { - let recorder = InvocationRecorder() - let wrapped = LoginShellBrewCommandRunner( - underlying: recorder, - shellResolver: LoginShellResolver( - lookup: { URL(fileURLWithPath: "/opt/homebrew/bin/fish") }, - ), - makeMarker: { "MARK" }, - ) - - _ = try await wrapped.run( - executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), - arguments: ["config", "--foo=it's"], - ) - - let invocation = try #require(await recorder.first) - #expect(invocation.executableURL.path == "/opt/homebrew/bin/fish") - #expect(invocation.arguments[3] == "printf '%s\\n' 'MARK' 1>&2; printf '%s\\n' 'MARK'; exec $argv") - #expect(invocation.arguments[4] == "/opt/homebrew/bin/brew") - #expect(invocation.arguments[5] == "config") - #expect(invocation.arguments[6] == "--foo=it's") - } - - @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) - } - - /// The shell exports what it inherits, so a variable pinned on it reaches brew. - @Test func `run forwards the pinned environment to the shell`() async throws { - let recorder = InvocationRecorder() - let wrapped = LoginShellBrewCommandRunner( - underlying: recorder, - shellResolver: LoginShellResolver(lookup: { URL(fileURLWithPath: "/bin/zsh") }), - ) - - _ = try await wrapped.run( - executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), - arguments: ["upgrade"], - options: BrewRunOptions(environment: ["HOMEBREW_NO_COLOR": "1"]), - ) - - let invocation = try #require(await recorder.first) - #expect(invocation.options.environment["HOMEBREW_NO_COLOR"] == "1") - } - - /// Reproduces the iTerm2 shell-integration report: startup noise lands on both streams ahead of - /// the real command's output, and only the marker line tells us where it ends. - @Test func `run strips shell startup noise from both streams`() async throws { - let recorder = InvocationRecorder( - stubbedOutput: CommandOutput( - standardOutput: "hello from zshrc\nMARK\n{\"foo\":1}\n", - standardError: "OSC 1337 junk\nMARK\nreal warning\n", - terminationStatus: 0, - ), - ) - let wrapped = LoginShellBrewCommandRunner( - underlying: recorder, - shellResolver: LoginShellResolver(lookup: { URL(fileURLWithPath: "/bin/zsh") }), - makeMarker: { "MARK" }, - ) - - let output = try await wrapped.run( - executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), - arguments: ["info", "--installed", "--json=v2"], - ) - - #expect(output.standardOutput == "{\"foo\":1}\n") - #expect(output.standardError == "real warning\n") - } - - @Test func `run filters the startup marker out of streamed lines`() async throws { - let collector = LineCollector() - let recorder = InvocationRecorder( - linesToEmit: [ - BrewCommandOutputLine(stream: .stdout, text: "hello from zshrc"), - BrewCommandOutputLine(stream: .stdout, text: "MARK"), - BrewCommandOutputLine(stream: .stdout, text: "==> Installing wget"), - ], - ) - let wrapped = LoginShellBrewCommandRunner( - underlying: recorder, - shellResolver: LoginShellResolver(lookup: { URL(fileURLWithPath: "/bin/zsh") }), - makeMarker: { "MARK" }, - ) - - _ = try await wrapped.run( - executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), - arguments: ["install", "wget"], - options: BrewRunOptions(lineObserver: { collector.append($0.text) }, output: .pseudoTerminal), - ) - - #expect(collector.all == ["==> Installing wget"]) - } - - @Test func `run filters startup markers independently from stdout and stderr`() async throws { - let collector = LineCollector() - let recorder = InvocationRecorder( - linesToEmit: [ - BrewCommandOutputLine(stream: .stderr, text: "stderr startup"), - BrewCommandOutputLine(stream: .stdout, text: "stdout startup"), - BrewCommandOutputLine(stream: .stderr, text: "MARK"), - BrewCommandOutputLine(stream: .stdout, text: "MARK"), - BrewCommandOutputLine(stream: .stdout, text: "Your system is ready to brew."), - BrewCommandOutputLine(stream: .stderr, text: "real warning"), - ], - ) - let wrapped = LoginShellBrewCommandRunner( - underlying: recorder, - shellResolver: LoginShellResolver(lookup: { URL(fileURLWithPath: "/bin/zsh") }), - makeMarker: { "MARK" }, - ) - - _ = try await wrapped.run( - executableURL: URL(fileURLWithPath: "/opt/homebrew/bin/brew"), - arguments: ["doctor"], - options: BrewRunOptions(lineObserver: { collector.append($0.text) }, output: .pipes(forceColor: true)), - ) - - #expect(collector.all == ["Your system is ready to brew.", "real warning"]) - } - - @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] - let options: BrewRunOptions -} - -// swiftlint:disable:next unchecked_sendable -private final class LineCollector: @unchecked Sendable { - private let lock = NSLock() - private var lines: [String] = [] - - var all: [String] { - lock.lock() - defer { lock.unlock() } - return lines - } - - func append(_ line: String) { - lock.lock() - defer { lock.unlock() } - lines.append(line) - } -} - -private actor InvocationRecorder: BrewCommandRunning { - private(set) var invocations: [RecordedInvocation] = [] - private let stubbedOutput: CommandOutput - private let linesToEmit: [BrewCommandOutputLine] - - init( - stubbedOutput: CommandOutput = CommandOutput(standardOutput: "", standardError: "", terminationStatus: 0), - linesToEmit: [BrewCommandOutputLine] = [], - ) { - self.stubbedOutput = stubbedOutput - self.linesToEmit = linesToEmit - } - - var first: RecordedInvocation? { - invocations.first - } - - func run(executableURL: URL, arguments: [String], options: BrewRunOptions) async throws -> CommandOutput { - invocations.append(RecordedInvocation(executableURL: executableURL, arguments: arguments, options: options)) - linesToEmit.forEach { options.lineObserver?($0) } - return stubbedOutput - } -} diff --git a/Tests/BrewCLITests/LoginShellResolverTests.swift b/Tests/BrewCLITests/LoginShellResolverTests.swift deleted file mode 100644 index b6e91a8..0000000 --- a/Tests/BrewCLITests/LoginShellResolverTests.swift +++ /dev/null @@ -1,40 +0,0 @@ -// -// 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) - } -} diff --git a/Tests/BrewCLITests/ZshBrewCommandRunnerTests.swift b/Tests/BrewCLITests/ZshBrewCommandRunnerTests.swift new file mode 100644 index 0000000..35c65d4 --- /dev/null +++ b/Tests/BrewCLITests/ZshBrewCommandRunnerTests.swift @@ -0,0 +1,188 @@ +// +// ZshBrewCommandRunnerTests.swift +// BrewCLITests +// + +@testable import BrewCLI +import BrewCore +import Foundation +import Synchronization +import Testing + +struct ZshBrewCommandRunnerTests { + @Test func `brew receives only the explicit environment`() async throws { + let output = try await ZshBrewCommandRunner(underlying: PollutedEnvironmentRunner()).run( + executableURL: URL(fileURLWithPath: "/usr/bin/env"), + arguments: [], + ) + + let keys = output.standardOutput.split(separator: "\n").compactMap { $0.split(separator: "=", maxSplits: 1).first } + #expect(Set(keys) == ["HOME", "USER", "LOGNAME", "TMPDIR", "LANG", "SHELL", "PATH"]) + } + + @Test func `the executable path and arguments survive shell metacharacters`() async throws { + let directory = FileManager.default.temporaryDirectory.appendingPathComponent("brew ' $; \(UUID())") + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: directory) } + let executable = directory.appendingPathComponent("brew") + try FileManager.default.createSymbolicLink(at: executable, withDestinationURL: URL(fileURLWithPath: "/usr/bin/printf")) + let arguments = ["", "two words", "it's", "$(echo injected)", "a;b", "*", "a\nb"] + + let output = try await ZshBrewCommandRunner().run( + executableURL: executable, + arguments: ["<%s>"] + arguments, + ) + + #expect(output.standardOutput == arguments.map { "<\($0)>" }.joined()) + } + + @Test func `user startup files are skipped`() async throws { + let directory = FileManager.default.temporaryDirectory.appendingPathComponent(UUID().uuidString) + try FileManager.default.createDirectory(at: directory, withIntermediateDirectories: true) + defer { try? FileManager.default.removeItem(at: directory) } + for name in [".zshenv", ".zprofile", ".zshrc", ".zlogin"] { + try "print sourced > \"$ZDOTDIR/sourced\"\n".write(to: directory.appendingPathComponent(name), atomically: true, encoding: .utf8) + } + + _ = try await ZshBrewCommandRunner().run( + executableURL: URL(fileURLWithPath: "/usr/bin/printf"), + arguments: ["brew output"], + options: BrewRunOptions(environment: ["HOME": directory.path, "ZDOTDIR": directory.path]), + ) + + #expect(!FileManager.default.fileExists(atPath: directory.appendingPathComponent("sourced").path)) + } + + @Test func `exports from system startup are cleared before Homebrew`() async throws { + let output = try await ZshBrewCommandRunner( + underlying: PollutedEnvironmentRunner(startupScript: "export HOMEBREW_STARTUP_LEAK=yes PATH=/unexpected; "), + ).run(executableURL: URL(fileURLWithPath: "/usr/bin/env"), arguments: []) + + #expect(!output.standardOutput.contains("/unexpected") && !output.standardOutput.contains("HOMEBREW_STARTUP_LEAK")) + } + + @Test(arguments: [BrewRunOptions.OutputChannel.pipes(forceColor: true), .pseudoTerminal]) + func `colour controls reach Homebrew`(channel: BrewRunOptions.OutputChannel) async throws { + let output = try await ZshBrewCommandRunner().run( + executableURL: URL(fileURLWithPath: "/bin/sh"), + arguments: ["-c", "printf '%s|%s' \"$HOMEBREW_COLOR\" \"$CLICOLOR_FORCE\""], + options: BrewRunOptions(output: channel), + ) + + #expect(output.standardOutput == "1|1") + } + + @Test func `explicit output controls survive a terminal allocation failure`() async throws { + let lines = Mutex<[String]>([]) + let output = try await ZshBrewCommandRunner( + underlying: PollutedEnvironmentRunner( + startupScript: "printf 'startup banner'; printf 'startup warning' >&2; ", + service: BrewCommandService(makeTerminal: { throw CocoaError(.fileWriteUnknown) }), + ), + ).run( + executableURL: URL(fileURLWithPath: "/bin/sh"), + arguments: ["-c", "printf '%s|%s' \"$HOMEBREW_COLOR\" \"$HOMEBREW_NO_COLOR\""], + options: BrewRunOptions( + lineObserver: { line in lines.withLock { $0.append(line.text) } }, + output: .pseudoTerminal, + environment: ["HOMEBREW_NO_COLOR": "1"], + ), + ) + + #expect(output.standardOutput == "1|1") + #expect(output.standardError.isEmpty) + #expect(lines.withLock { $0 } == ["1|1"]) + } + + @Test func `PATH and SHELL cannot be overridden`() async throws { + let output = try await ZshBrewCommandRunner().run( + executableURL: URL(fileURLWithPath: "/bin/sh"), + arguments: ["-c", "printf '%s|%s' \"$PATH\" \"$SHELL\""], + options: BrewRunOptions(environment: ["PATH": "/unexpected", "SHELL": "/bin/bash"]), + ) + + #expect(output.standardOutput == "/bin:/usr/bin:/bin|/bin/zsh") + } + + @Test func `buffered output and failure status are preserved`() async throws { + let output = try await ZshBrewCommandRunner().run( + executableURL: URL(fileURLWithPath: "/bin/sh"), + arguments: ["-c", "printf 'output'; printf 'warning' >&2; exit 12"], + ) + + #expect(output == CommandOutput(standardOutput: "output", standardError: "warning", terminationStatus: 12)) + } + + @Test func `streaming preserves the first output on both streams`() async throws { + let lines = Mutex<[String]>([]) + _ = try await ZshBrewCommandRunner().run( + executableURL: URL(fileURLWithPath: "/bin/sh"), + arguments: ["-c", "printf 'output\\n'; printf 'warning\\n' >&2"], + options: BrewRunOptions(lineObserver: { line in lines.withLock { $0.append(line.text) } }), + ) + + #expect(lines.withLock { $0.sorted() } == ["output", "warning"]) + } + + @Test(arguments: [BrewRunOptions.OutputChannel.pipes(forceColor: false), .pseudoTerminal]) + func `system startup banners are removed from buffered and streamed output`(channel: BrewRunOptions.OutputChannel) async throws { + let lines = Mutex<[BrewCommandOutputLine]>([]) + let output = try await ZshBrewCommandRunner( + underlying: PollutedEnvironmentRunner( + startupScript: "printf 'startup\\nbanner'; printf 'startup\\nwarning' >&2; sleep 0.05; ", + ), + ).run( + executableURL: URL(fileURLWithPath: "/bin/sh"), + arguments: ["-c", "printf '{\"formulae\":[]}\\n'; printf 'brew warning\\n' >&2; exit 12"], + options: BrewRunOptions(lineObserver: { line in lines.withLock { $0.append(line) } }, output: channel), + ) + + #expect(output.standardOutput == (channel == .pseudoTerminal ? "{\"formulae\":[]}\nbrew warning\n" : "{\"formulae\":[]}\n")) + #expect(output.standardError == (channel == .pseudoTerminal ? "" : "brew warning\n")) + #expect(output.terminationStatus == 12) + #expect(lines.withLock { $0.filter(\.isComplete).map(\.text).sorted() } == ["brew warning", "{\"formulae\":[]}"]) + #expect(lines.withLock { $0.allSatisfy { "{\"formulae\":[]}".hasPrefix($0.text) || "brew warning".hasPrefix($0.text) } }) + } + + @Test(arguments: [BrewRunOptions.OutputChannel.pipes(forceColor: false), .pseudoTerminal]) + func `startup failures retain their diagnostics`(channel: BrewRunOptions.OutputChannel) async throws { + let lines = Mutex<[BrewCommandOutputLine]>([]) + let output = try await ZshBrewCommandRunner( + underlying: PollutedEnvironmentRunner(startupScript: "printf 'startup failed\\n' >&2; exit 17; "), + ).run( + executableURL: URL(fileURLWithPath: "/usr/bin/printf"), + arguments: ["unreachable"], + options: BrewRunOptions(lineObserver: { line in lines.withLock { $0.append(line) } }, output: channel), + ) + + #expect(output.standardOutput == (channel == .pseudoTerminal ? "startup failed\n" : "")) + #expect(output.standardError == (channel == .pseudoTerminal ? "" : "startup failed\n")) + #expect(output.terminationStatus == 17) + #expect(lines.withLock { $0.filter(\.isComplete).map(\.text) } == ["startup failed"]) + } +} + +private struct PollutedEnvironmentRunner: BrewCommandRunning { + var environment = [ + "HOMEBREW_NO_INSTALL_FROM_API": "1", + "HOMEBREW_NO_ENV_FILE": "1", + "HOMEBREW_CASK_OPTS": "--no-quarantine", + "PATH": "/bin:/usr/bin:/unexpected", + "SHELL": "/nonexistent/shell", + "XDG_CONFIG_HOME": "/unexpected/config", + "BASH_ENV": "/unexpected/bashenv", + ] + var startupScript = "" + var service = BrewCommandService() + + func run(executableURL: URL, arguments: [String], options: BrewRunOptions) async throws -> CommandOutput { + var arguments = arguments + // Simulate /etc/zshenv without modifying the machine's startup files. + if let index = arguments.firstIndex(of: "-c") { + arguments[index + 1] = startupScript + arguments[index + 1] + } + var options = options + options.environment.merge(environment) { _, polluted in polluted } + return try await service.run(executableURL: executableURL, arguments: arguments, options: options) + } +} diff --git a/Tests/BrewSelfUpgradeContractTests/SelfUpgradeHandoffSpecTests.swift b/Tests/BrewSelfUpgradeContractTests/SelfUpgradeHandoffSpecTests.swift index 703a1fa..ca5d043 100644 --- a/Tests/BrewSelfUpgradeContractTests/SelfUpgradeHandoffSpecTests.swift +++ b/Tests/BrewSelfUpgradeContractTests/SelfUpgradeHandoffSpecTests.swift @@ -17,7 +17,6 @@ struct SelfUpgradeHandoffSpecTests { relaunchEnvironment: ["BREW_UITEST_SCENARIO": "selfUpgradeAvailable"], brewExecutablePath: "/opt/homebrew/bin/brew", upgradeArguments: ["upgrade", "--cask", "homebrew-app"], - usesLoginShell: true, upgradeEnvironment: ["BREW_UITEST_FIXTURES": "/tmp/fixtures"], logFilePath: "/tmp/self-upgrade.log", defaultsSuiteName: "sh.brew.app", @@ -49,7 +48,6 @@ struct SelfUpgradeHandoffSpecTests { #expect(decoded.brewExecutablePath == "/opt/homebrew/bin/brew") #expect(decoded.upgradeArguments == ["upgrade", "--cask", "homebrew-app"]) #expect(decoded.upgradeEnvironment["BREW_UITEST_FIXTURES"] == "/tmp/fixtures") - #expect(decoded.usesLoginShell) #expect(decoded.logFilePath == "/tmp/self-upgrade.log") } diff --git a/Tests/BrewSelfUpgradeHelperCoreTests/SelfUpgradeRunnerTests.swift b/Tests/BrewSelfUpgradeHelperCoreTests/SelfUpgradeRunnerTests.swift index ab40ad7..8088ded 100644 --- a/Tests/BrewSelfUpgradeHelperCoreTests/SelfUpgradeRunnerTests.swift +++ b/Tests/BrewSelfUpgradeHelperCoreTests/SelfUpgradeRunnerTests.swift @@ -17,7 +17,7 @@ struct SelfUpgradeRunnerTests { let brew = try FakeExecutable(script: "exit 0") defer { brew.remove() } - let outcome = await FakeExecutable.runner().run( + let outcome = await SelfUpgradeRunner().run( executablePath: brew.path, arguments: ["upgrade", "--cask", "homebrew-app"], environment: [:], @@ -32,7 +32,7 @@ struct SelfUpgradeRunnerTests { let brew = try FakeExecutable(script: "exit 12") defer { brew.remove() } - let outcome = await FakeExecutable.runner().run( + let outcome = await SelfUpgradeRunner().run( executablePath: brew.path, arguments: ["upgrade"], environment: [:], @@ -47,7 +47,7 @@ struct SelfUpgradeRunnerTests { let brew = try FakeExecutable(script: "kill -TERM $$; sleep 5") defer { brew.remove() } - let outcome = await FakeExecutable.runner().run( + let outcome = await SelfUpgradeRunner().run( executablePath: brew.path, arguments: [], environment: [:], @@ -61,7 +61,7 @@ struct SelfUpgradeRunnerTests { /// The app resolves `brew` before it quits, so the path can be stale by the time the helper runs it. @Test func `a path with no executable fails without launching anything`() async { - let outcome = await FakeExecutable.runner().run( + let outcome = await SelfUpgradeRunner().run( executablePath: "/nonexistent/brew", arguments: ["upgrade"], environment: [:], @@ -73,7 +73,7 @@ struct SelfUpgradeRunnerTests { } @Test func `a directory is not an executable`() async { - let outcome = await FakeExecutable.runner().run( + let outcome = await SelfUpgradeRunner().run( executablePath: NSTemporaryDirectory(), arguments: ["upgrade"], environment: [:], @@ -91,7 +91,7 @@ struct SelfUpgradeRunnerTests { defer { brew.remove() } let started = Date() - let outcome = await FakeExecutable.runner().run( + let outcome = await SelfUpgradeRunner().run( executablePath: brew.path, arguments: [], environment: [:], @@ -202,7 +202,7 @@ struct SelfUpgradeRunnerTests { defer { brew.remove() } let transcript = TranscriptRecorder() - let outcome = await FakeExecutable.runner(transcriptSink: { transcript.append($0) }).run( + let outcome = await SelfUpgradeRunner(transcriptSink: { transcript.append($0) }).run( executablePath: brew.path, arguments: [], environment: [:], @@ -219,7 +219,7 @@ struct SelfUpgradeRunnerTests { defer { brew.remove() } let transcript = TranscriptRecorder() - _ = await FakeExecutable.runner(transcriptSink: { transcript.append($0) }).run( + _ = await SelfUpgradeRunner(transcriptSink: { transcript.append($0) }).run( executablePath: brew.path, arguments: [], environment: [:], @@ -235,7 +235,7 @@ struct SelfUpgradeRunnerTests { defer { brew.remove() } let transcript = TranscriptRecorder() - let outcome = await FakeExecutable.runner(transcriptSink: { transcript.append($0) }).run( + let outcome = await SelfUpgradeRunner(transcriptSink: { transcript.append($0) }).run( executablePath: brew.path, arguments: [], environment: [:], @@ -253,7 +253,7 @@ struct SelfUpgradeRunnerTests { defer { brew.remove() } let transcript = TranscriptRecorder() - _ = await FakeExecutable.runner(transcriptSink: { transcript.append($0) }).run( + _ = await SelfUpgradeRunner(transcriptSink: { transcript.append($0) }).run( executablePath: brew.path, arguments: ["upgrade", "--cask", "homebrew-app"], environment: [:], @@ -269,7 +269,7 @@ struct SelfUpgradeRunnerTests { defer { brew.remove() } let transcript = TranscriptRecorder() - _ = await FakeExecutable.runner(transcriptSink: { transcript.append($0) }).run( + _ = await SelfUpgradeRunner(transcriptSink: { transcript.append($0) }).run( executablePath: brew.path, arguments: [], environment: ["BREW_UITEST_SCENARIO": "selfUpgradeAvailable"], @@ -279,19 +279,19 @@ struct SelfUpgradeRunnerTests { #expect(transcript.lines == ["selfUpgradeAvailable"]) } - @Test func `the inherited environment survives alongside the pinned one`() async throws { + @Test func `self upgrade uses the same restricted PATH as the app`() async throws { let brew = try FakeExecutable(script: #"echo "${PATH}""#) defer { brew.remove() } let transcript = TranscriptRecorder() - _ = await FakeExecutable.runner(transcriptSink: { transcript.append($0) }).run( + _ = await SelfUpgradeRunner(transcriptSink: { transcript.append($0) }).run( executablePath: brew.path, arguments: [], environment: ["PINNED": "yes"], timeout: 30, ) - #expect(transcript.lines == [ProcessInfo.processInfo.environment["PATH"]]) + #expect(transcript.lines == [brew.url.deletingLastPathComponent().path + ":/usr/bin:/bin"]) } /// The transcript is a file, so escape codes in it are noise rather than colour. @@ -350,12 +350,6 @@ private struct FakeExecutable { func remove() { try? FileManager.default.removeItem(at: url) } - - /// Invoked directly, for the reason `BrewCommandExecutionContext.uiTesting(brewURL:)` does the same: - /// the login shell would source the developer's dotfiles. - static func runner(transcriptSink: @escaping @Sendable (String) -> Void = { _ in }) -> SelfUpgradeRunner { - SelfUpgradeRunner(usesLoginShell: false, transcriptSink: transcriptSink) - } } /// Fires the timeout once the fake `brew` has started, rather than on a wall-clock guess.