From 8da3035d6f925dfbecdb27de17a415fe6d59d0af Mon Sep 17 00:00:00 2001 From: Jean-Philippe Date: Mon, 14 Sep 2026 21:24:18 -0400 Subject: [PATCH 1/2] brew: escalate writes to the prefix owner so the agent converges deps itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Homebrew assumes exactly one non-root owner of exactly one prefix, so on cooperative shared machines (plans#26) the sandboxed agent user cannot install formulae — the machine-level root cause of the plans#36 leg-2 googletest incident (brew wwise-cli EPERM). Rejected alternatives: per-user prefix (no bottles off the default prefix on Apple Silicon → source builds), host-level provisioning (a human in the loop for every new formula), group-writable prefix (fragments brew's single-owner ownership metadata). Precedent: snappy's build image already routes brew through a dedicated linuxbrew user — same pattern, designated owner. - BrewIntegration prepends sudo -n -u to brew writes (tap, install) when the prefix is not writable by the current user. The owner is stat'd at runtime — never hardcoded — and a writable prefix short-circuits to plain brew: single-user machines unchanged. A sudo -n refusal names the missing edge and its remediation. - AgentBootstrap's sudoers drop-in gains the brew edge: the agent may run exactly brew as the prefix owner, NOPASSWD. Stat'd at bootstrap, re-converged by re-running register, omitted when the host has no brew or the agent owns the prefix itself. Co-authored-by: Cursor --- lib/dev/agent_bootstrap.rb | 54 ++++++++-- lib/dev/deps/brew_integration.rb | 96 +++++++++++++++-- test/dev/agent_bootstrap_test.rb | 48 +++++++++ test/dev/deps/brew_integration_test.rb | 140 +++++++++++++++++++++++-- 4 files changed, 314 insertions(+), 24 deletions(-) diff --git a/lib/dev/agent_bootstrap.rb b/lib/dev/agent_bootstrap.rb index 0b1d237..b28c3da 100644 --- a/lib/dev/agent_bootstrap.rb +++ b/lib/dev/agent_bootstrap.rb @@ -46,6 +46,17 @@ class StepFailedError < RuntimeError; end # The sudoers drop-in carrying the one-way spawn edge. SUDOERS_PATH = "/etc/sudoers.d/ai-flow-agent" + # Where brew lives on supported hosts, in discovery order (Apple + # Silicon, Intel mac, Linuxbrew). + BREW_LOCATIONS = T.let( + [ + "/opt/homebrew/bin/brew", + "/usr/local/bin/brew", + "/home/linuxbrew/.linuxbrew/bin/brew", + ].freeze, + T::Array[String], + ) + # The shared DDC directory under the shared root. UE disables a shared # cache store whose path is missing rather than creating it (probed on # ue5-mac 5.8), so register provisions the leaf; cellbound-3d's committed @@ -132,6 +143,8 @@ def quiet?(*cmd) # @param shared_root [String] where the shared data root is provisioned # @param home_dev [String] the per-user data dir migrated out of # @param launch_agents_dir [String] where svc.sh installs runner plists + # @param brew_executable [String, nil] the host's brew binary (discovered + # from BREW_LOCATIONS; injectable for tests; nil = no brew edge) sig do params( agent_user: String, @@ -142,12 +155,14 @@ def quiet?(*cmd) shared_root: String, home_dev: String, launch_agents_dir: String, + brew_executable: T.nilable(String), ).void end def initialize(agent_user: DEFAULT_AGENT_USER, runner_user: T.must(Etc.getpwuid(Process.uid)).name, executor: Executor.new, out: $stdout, darwin: RUBY_PLATFORM.include?("darwin"), shared_root: DataRoot::SHARED_ROOT, home_dev: File.expand_path(DataRoot::HOME_ROOT), - launch_agents_dir: File.join(Dir.home, "Library", "LaunchAgents")) + launch_agents_dir: File.join(Dir.home, "Library", "LaunchAgents"), + brew_executable: BREW_LOCATIONS.find { |path| File.exist?(path) }) @agent_user = agent_user @runner_user = runner_user @executor = executor @@ -156,6 +171,7 @@ def initialize(agent_user: DEFAULT_AGENT_USER, runner_user: T.must(Etc.getpwuid( @shared_root = shared_root @home_dev = home_dev @launch_agents_dir = launch_agents_dir + @brew_executable = brew_executable end # Converge the host-singular facts: agent user, group, sudoers edge, @@ -237,19 +253,45 @@ def agent_config_content(existing) # The sudoers drop-in content: the one-way NOPASSWD SETENV edge (env is # allowlisted by the caller's --preserve-env, which is why SETENV is # safe here), plus the agent-side umask defaults that keep agent-created - # dirs group-accessible for the dispatcher's cleanup and `git add`. + # dirs group-accessible for the dispatcher's cleanup and `git add`, + # plus the brew escalation edge (when the host has brew). # # @return [String] sig { returns(String) } def sudoers_content - <<~SUDOERS - #{@runner_user} ALL=(#{@agent_user}) NOPASSWD:SETENV: ALL - Defaults>#{@agent_user} env_reset, umask=0002, umask_override - SUDOERS + lines = [ + "#{@runner_user} ALL=(#{@agent_user}) NOPASSWD:SETENV: ALL", + "Defaults>#{@agent_user} env_reset, umask=0002, umask_override", + ] + edge = brew_edge + lines << edge if edge + "#{lines.join("\n")}\n" end private + # The brew escalation edge (the Homebrew single-user gap on cooperative + # machines): the agent may run exactly brew as the prefix owner, + # NOPASSWD, so `dev install-deps` converges formulae without a human + # even though the prefix belongs to the enrolling user. The owner is + # stat'd here at bootstrap time — never hardcoded — and re-running + # register re-converges the edge. Omitted when the host has no brew or + # the agent owns the prefix itself (nothing to escalate). Consumed by + # Dev::Deps::BrewIntegration's `sudo -n` escalation. + # + # @return [String, nil] + sig { returns(T.nilable(String)) } + def brew_edge + brew = @brew_executable + return nil unless brew && File.exist?(brew) + + prefix = File.dirname(File.dirname(brew)) + owner = T.must(Etc.getpwuid(File.stat(prefix).uid)).name + return nil if owner == @agent_user + + "#{@agent_user} ALL=(#{owner}) NOPASSWD: #{brew}" + end + # @raise [UnsupportedPlatformError] off macOS sig { void } def assert_darwin! diff --git a/lib/dev/deps/brew_integration.rb b/lib/dev/deps/brew_integration.rb index 094b68a..0f69b76 100644 --- a/lib/dev/deps/brew_integration.rb +++ b/lib/dev/deps/brew_integration.rb @@ -1,6 +1,7 @@ # typed: strict # frozen_string_literal: true +require "etc" require "open3" require "pathname" require "uri" @@ -15,6 +16,15 @@ module Deps # install_all installs each formula/cask via brew. Registers taps # (if configured) before the first install. # + # Homebrew assumes exactly one non-root owner of exactly one prefix, so + # on cooperative shared machines (plans#26) a sandboxed agent user + # cannot write /opt/homebrew. Brew *writes* (tap, install) therefore + # escalate to the prefix owner via `sudo -n` when the prefix is not + # writable by the current user — the owner is stat'd at runtime, never + # hardcoded, and the NOPASSWD edge is converged by the agent host + # bootstrap (Dev::AgentBootstrap). On a single-user machine the prefix + # is writable and nothing changes. + # # Env filtering (install vs skip based on ci/dev) is the caller's # responsibility — only pass deps that should be installed. class BrewIntegration < Integration @@ -27,19 +37,24 @@ class TapRegistrationError < StandardError; end # @param cache [Cache, nil] shared download cache # @param taps [Array] Homebrew taps to register before installing # @param project_dir [String, Pathname, nil] project root for resolving file:// tap URLs + # @param brew_prefix [String, Pathname, nil] the Homebrew prefix + # (discovered via `brew --prefix` when nil; injectable for tests) sig do params( repository: T.nilable(Repository), cache: T.nilable(Cache), taps: T::Array[Tap], project_dir: T.nilable(T.any(String, Pathname)), + brew_prefix: T.nilable(T.any(String, Pathname)), ).void end - def initialize(repository:, cache:, taps: [], project_dir: nil) + def initialize(repository:, cache:, taps: [], project_dir: nil, brew_prefix: nil) super(repository:, cache:) @taps = taps @project_dir = T.let(project_dir ? Pathname(project_dir) : nil, T.nilable(Pathname)) @taps_registered = T.let(false, T::Boolean) + @brew_prefix = T.let(brew_prefix&.to_s, T.nilable(String)) + @brew_prefix_resolved = T.let(!brew_prefix.nil?, T::Boolean) end # Install all brew dependencies. Registers taps on first call. @@ -76,7 +91,8 @@ def ensure_taps_registered @taps_registered = true end - # Register a single Homebrew tap. + # Register a single Homebrew tap (escalated to the prefix owner when + # the prefix is not ours — see the class doc). # # @param tap [Tap] tap to register # @raise [TapRegistrationError] if `brew tap` fails @@ -86,15 +102,15 @@ def register_tap(tap) url = tap.url if tap.local? && project_dir && url path = resolve_file_url(url, project_dir) - success = system("brew", "tap", tap.name, path) - raise TapRegistrationError, "brew tap #{tap.name} #{path} failed" unless success + success = system(*escalation, "brew", "tap", tap.name, path) + raise TapRegistrationError, "brew tap #{tap.name} #{path} failed#{escalation_hint}" unless success elsif url url_str = url.to_s - success = system("brew", "tap", tap.name, url_str) - raise TapRegistrationError, "brew tap #{tap.name} #{url_str} failed" unless success + success = system(*escalation, "brew", "tap", tap.name, url_str) + raise TapRegistrationError, "brew tap #{tap.name} #{url_str} failed#{escalation_hint}" unless success else - success = system("brew", "tap", tap.name) - raise TapRegistrationError, "brew tap #{tap.name} failed" unless success + success = system(*escalation, "brew", "tap", tap.name) + raise TapRegistrationError, "brew tap #{tap.name} failed#{escalation_hint}" unless success end end @@ -163,15 +179,73 @@ def brew_installed?(name) system("brew list #{name} >/dev/null 2>&1") end - # Run `brew install` with the given spec. + # Run `brew install` with the given spec (escalated to the prefix + # owner when the prefix is not ours — see the class doc). # # @param name [String] dependency name (for error messages) # @param spec [String] full install spec (e.g. "cmake@3.31.4") # @raise [InstallError] if brew exits non-zero sig { params(name: String, spec: String).void } def run_brew_install(name, spec) - _out, err, status = T.unsafe(Open3).capture3("brew", "install", *spec.split) - raise InstallError, "brew install #{spec} failed: #{err}" unless status.success? + _out, err, status = T.unsafe(Open3).capture3(*escalation, "brew", "install", *spec.split) + return if status.success? + + if sudo_refused?(err) + raise InstallError, + "brew install #{spec} needs the prefix owner and sudo -n was refused — " \ + "the brew sudoers edge is missing on this host; run `dev runner register` to re-converge it" + end + + raise InstallError, "brew install #{spec} failed: #{err}" + end + + # argv prefix for brew write commands: empty when the prefix is + # writable by the current user, `sudo -n` to the stat'd prefix owner + # otherwise. -n so a missing sudoers edge fails fast instead of + # hanging on a password prompt no one is watching. + # + # @return [Array] + sig { returns(T::Array[String]) } + def escalation + prefix = brew_prefix + return [] if prefix.nil? || File.writable?(prefix) + + ["sudo", "-n", "-u", T.must(Etc.getpwuid(File.stat(prefix).uid)).name] + end + + # Remediation appended to escalated-write failures: the two host facts + # that break them (missing sudoers edge, a path the owner cannot read). + # + # @return [String] "" when not escalated + sig { returns(String) } + def escalation_hint + return "" if escalation.empty? + + " (escalated to the brew prefix owner: ensure any local tap path is readable " \ + "by them and the sudoers brew edge exists — run `dev runner register` to re-converge it)" + end + + # The Homebrew prefix, resolved once: injected (tests), else asked of + # brew itself. nil when brew is absent — writes then run unescalated + # and surface brew's own error. + # + # @return [String, nil] + sig { returns(T.nilable(String)) } + def brew_prefix + return @brew_prefix if @brew_prefix_resolved + + @brew_prefix_resolved = true + out, _err, status = Open3.capture3("brew", "--prefix") + @brew_prefix = (out.strip if status.success? && !out.strip.empty?) + rescue Errno::ENOENT + @brew_prefix = nil + end + + # @param err [String] a brew invocation's stderr + # @return [Boolean] whether sudo -n refused for lack of a NOPASSWD rule + sig { params(err: String).returns(T::Boolean) } + def sudo_refused?(err) + err.include?("a password is required") end end end diff --git a/test/dev/agent_bootstrap_test.rb b/test/dev/agent_bootstrap_test.rb index 6db89e3..ba20c8a 100644 --- a/test/dev/agent_bootstrap_test.rb +++ b/test/dev/agent_bootstrap_test.rb @@ -3,6 +3,7 @@ require "test_helper" require "dev/agent_bootstrap" +require "etc" require "stringio" require "tmpdir" @@ -493,6 +494,53 @@ def converged_identity_executor content.end_with?("\n") end + test "sudoers content grants the agent a brew edge to the prefix owner" do + Given "a host with brew at a known prefix owned by the human" + dir = Dir.mktmpdir + brew = File.join(dir, "bin", "brew") + FileUtils.mkdir_p(File.dirname(brew)) + File.write(brew, "") + owner = Etc.getpwuid(File.stat(dir).uid).name + content = Dev::AgentBootstrap.new( + runner_user: "human", darwin: true, brew_executable: brew, + ).sudoers_content + + Expect "the agent may run exactly brew as the prefix owner, NOPASSWD" + content.include?("ai-agent ALL=(#{owner}) NOPASSWD: #{brew}") + + Cleanup + FileUtils.rm_rf(dir) + end + + test "sudoers content omits the brew edge when brew is absent" do + Given "a host with no brew executable" + content = Dev::AgentBootstrap.new( + runner_user: "human", darwin: true, + brew_executable: File.join(Dir.mktmpdir, "no-brew"), + ).sudoers_content + + Expect "only the spawn edge and the umask defaults remain" + content.lines.size == 2 + end + + test "sudoers content omits the brew edge when the agent already owns the prefix" do + Given "a brew prefix owned by the agent user itself" + dir = Dir.mktmpdir + brew = File.join(dir, "bin", "brew") + FileUtils.mkdir_p(File.dirname(brew)) + File.write(brew, "") + owner = Etc.getpwuid(File.stat(dir).uid).name + content = Dev::AgentBootstrap.new( + runner_user: "human", agent_user: owner, darwin: true, brew_executable: brew, + ).sudoers_content + + Expect "no self-edge is emitted" + !content.include?("NOPASSWD: #{brew}") + + Cleanup + FileUtils.rm_rf(dir) + end + test "the agent user is a register-time parameter" do Given "an overridden run-as user" executor = RecordedBootstrapExecutor.new diff --git a/test/dev/deps/brew_integration_test.rb b/test/dev/deps/brew_integration_test.rb index 5c08c8c..0571e52 100644 --- a/test/dev/deps/brew_integration_test.rb +++ b/test/dev/deps/brew_integration_test.rb @@ -7,6 +7,7 @@ require "dev/deps/cache" require "dev/deps/dependency" require "dev/deps/tap" +require "etc" require "pathname" require "tmpdir" require "uri" @@ -18,7 +19,7 @@ class Dev::Deps::BrewIntegrationTest < Minitest::Test dir = Dir.mktmpdir("dev-brew-int-test-") cache = Dev::Deps::Cache.new(cache_dir: dir) repository = Dev::Deps::BrewRepository.new - integration = Dev::Deps::BrewIntegration.new(repository: repository, cache: cache) + integration = Dev::Deps::BrewIntegration.new(repository: repository, cache: cache, brew_prefix: dir) deps = [ Dev::Deps::Dependency.new(name: "cmake", integration: :brew, group: :build, version: "3.31.4", hash: "SHA256=abc", metadata: {}), @@ -41,7 +42,7 @@ class Dev::Deps::BrewIntegrationTest < Minitest::Test Given "an unversioned brew dependency (resolved version recorded, no suffix)" dir = Dir.mktmpdir("dev-brew-int-test-") cache = Dev::Deps::Cache.new(cache_dir: dir) - integration = Dev::Deps::BrewIntegration.new(repository: Dev::Deps::BrewRepository.new, cache: cache) + integration = Dev::Deps::BrewIntegration.new(repository: Dev::Deps::BrewRepository.new, cache: cache, brew_prefix: dir) deps = [ Dev::Deps::Dependency.new(name: "cmake", integration: :brew, group: :build, version: "4.3.4", hash: "SHA256=abc", metadata: {}), @@ -63,7 +64,7 @@ class Dev::Deps::BrewIntegrationTest < Minitest::Test Given "a versioned brew dependency (resolved 18.1.8, suffix 18)" dir = Dir.mktmpdir("dev-brew-int-test-") cache = Dev::Deps::Cache.new(cache_dir: dir) - integration = Dev::Deps::BrewIntegration.new(repository: Dev::Deps::BrewRepository.new, cache: cache) + integration = Dev::Deps::BrewIntegration.new(repository: Dev::Deps::BrewRepository.new, cache: cache, brew_prefix: dir) deps = [ Dev::Deps::Dependency.new(name: "llvm", integration: :brew, group: :build, version: "18.1.8", hash: "SHA256=abc", @@ -87,7 +88,7 @@ class Dev::Deps::BrewIntegrationTest < Minitest::Test dir = Dir.mktmpdir("dev-brew-int-test-") cache = Dev::Deps::Cache.new(cache_dir: dir) repository = Dev::Deps::BrewRepository.new - integration = Dev::Deps::BrewIntegration.new(repository: repository, cache: cache) + integration = Dev::Deps::BrewIntegration.new(repository: repository, cache: cache, brew_prefix: dir) deps = [ Dev::Deps::Dependency.new(name: "bad_formula", integration: :brew, group: :build, version: "1.0.0", hash: nil, metadata: { "version_suffix" => "1" }), @@ -115,7 +116,7 @@ class Dev::Deps::BrewIntegrationTest < Minitest::Test Given "two brew dependencies, the first of which fails to install" dir = Dir.mktmpdir("dev-brew-int-test-") cache = Dev::Deps::Cache.new(cache_dir: dir) - integration = Dev::Deps::BrewIntegration.new(repository: Dev::Deps::BrewRepository.new, cache: cache) + integration = Dev::Deps::BrewIntegration.new(repository: Dev::Deps::BrewRepository.new, cache: cache, brew_prefix: dir) deps = [ Dev::Deps::Dependency.new(name: "bad_formula", integration: :brew, group: :build, version: "1.0.0", hash: nil, metadata: {}), @@ -153,7 +154,7 @@ class Dev::Deps::BrewIntegrationTest < Minitest::Test cache = Dev::Deps::Cache.new(cache_dir: dir) tap = Dev::Deps::Tap.new(name: "local/tap", url: "file://#{dir}/brew-tap") integration = Dev::Deps::BrewIntegration.new( - repository: Dev::Deps::BrewRepository.new, cache: cache, taps: [tap], project_dir: dir, + repository: Dev::Deps::BrewRepository.new, cache: cache, taps: [tap], project_dir: dir, brew_prefix: dir, ) integration.expects(:system).with("brew", "tap", "local/tap", "#{dir}/brew-tap").returns(true) @@ -176,7 +177,7 @@ class Dev::Deps::BrewIntegrationTest < Minitest::Test cache = Dev::Deps::Cache.new(cache_dir: dir) tap = Dev::Deps::Tap.new(name: "org/tap", url: "https://github.com/org/homebrew-tap") integration = Dev::Deps::BrewIntegration.new( - repository: Dev::Deps::BrewRepository.new, cache: cache, taps: [tap], project_dir: dir, + repository: Dev::Deps::BrewRepository.new, cache: cache, taps: [tap], project_dir: dir, brew_prefix: dir, ) integration.expects(:system).with("brew", "tap", "org/tap", "https://github.com/org/homebrew-tap").returns(true) @@ -190,6 +191,131 @@ class Dev::Deps::BrewIntegrationTest < Minitest::Test FileUtils.rm_rf(dir) end + test "brew writes run unescalated when the prefix is writable by the current user" do + Given "an integration whose brew prefix is writable" + dir = Dir.mktmpdir("dev-brew-int-test-") + prefix = File.join(dir, "homebrew") + FileUtils.mkdir_p(prefix) + integration = Dev::Deps::BrewIntegration.new( + repository: Dev::Deps::BrewRepository.new, + cache: Dev::Deps::Cache.new(cache_dir: dir), + brew_prefix: prefix, + ) + deps = [ + Dev::Deps::Dependency.new(name: "cmake", integration: :brew, group: :build, + version: "4.3.4", hash: nil, metadata: {}), + ] + integration.stubs(:brew_installed?).returns(false) + Open3.expects(:capture3).with("brew", "install", "cmake").returns(["", "", stub(success?: true)]) + + When "installing all" + integration.install_all(deps) + + Then "brew ran directly, no sudo (Mocha-verified)" + true + + Cleanup + FileUtils.rm_rf(dir) + end + + test "brew writes escalate to the prefix owner when the prefix is not writable" do + Given "an integration whose brew prefix is owned read-only" + dir = Dir.mktmpdir("dev-brew-int-test-") + prefix = File.join(dir, "homebrew") + FileUtils.mkdir_p(prefix) + FileUtils.chmod(0o555, prefix) + owner = Etc.getpwuid(File.stat(prefix).uid).name + integration = Dev::Deps::BrewIntegration.new( + repository: Dev::Deps::BrewRepository.new, + cache: Dev::Deps::Cache.new(cache_dir: dir), + brew_prefix: prefix, + ) + deps = [ + Dev::Deps::Dependency.new(name: "cmake", integration: :brew, group: :build, + version: "4.3.4", hash: nil, metadata: {}), + ] + integration.stubs(:brew_installed?).returns(false) + Open3.expects(:capture3) + .with("sudo", "-n", "-u", owner, "brew", "install", "cmake") + .returns(["", "", stub(success?: true)]) + + When "installing all" + integration.install_all(deps) + + Then "brew ran through sudo -n as the prefix owner (Mocha-verified)" + true + + Cleanup + FileUtils.chmod(0o755, prefix) + FileUtils.rm_rf(dir) + end + + test "a sudo refusal surfaces the agent bootstrap remediation" do + Given "an unwritable prefix and a sudo -n that refuses (no sudoers edge)" + dir = Dir.mktmpdir("dev-brew-int-test-") + prefix = File.join(dir, "homebrew") + FileUtils.mkdir_p(prefix) + FileUtils.chmod(0o555, prefix) + owner = Etc.getpwuid(File.stat(prefix).uid).name + integration = Dev::Deps::BrewIntegration.new( + repository: Dev::Deps::BrewRepository.new, + cache: Dev::Deps::Cache.new(cache_dir: dir), + brew_prefix: prefix, + ) + deps = [ + Dev::Deps::Dependency.new(name: "cmake", integration: :brew, group: :build, + version: "4.3.4", hash: nil, metadata: {}), + ] + integration.stubs(:brew_installed?).returns(false) + Open3.stubs(:capture3) + .with("sudo", "-n", "-u", owner, "brew", "install", "cmake") + .returns(["", "sudo: a password is required\n", stub(success?: false)]) + + When "installing all and capturing the aggregate error" + error = nil + begin + integration.install_all(deps) + rescue StandardError => e + error = e + end + + Then "the failure names the missing sudoers edge and its remediation" + error.is_a?(Dev::Deps::Integration::PartialInstallError) + error.failures[0][1].message.include?("dev runner register") + + Cleanup + FileUtils.chmod(0o755, prefix) + FileUtils.rm_rf(dir) + end + + test "tap registration escalates with the same prefix-owner rule" do + Given "a remote tap and an unwritable brew prefix" + dir = Dir.mktmpdir("dev-brew-int-test-") + prefix = File.join(dir, "homebrew") + FileUtils.mkdir_p(prefix) + FileUtils.chmod(0o555, prefix) + owner = Etc.getpwuid(File.stat(prefix).uid).name + tap = Dev::Deps::Tap.new(name: "org/tap", url: "https://github.com/org/homebrew-tap") + integration = Dev::Deps::BrewIntegration.new( + repository: Dev::Deps::BrewRepository.new, + cache: Dev::Deps::Cache.new(cache_dir: dir), + taps: [tap], project_dir: dir, brew_prefix: prefix, + ) + integration.expects(:system) + .with("sudo", "-n", "-u", owner, "brew", "tap", "org/tap", "https://github.com/org/homebrew-tap") + .returns(true) + + When "installing all (no deps, taps only)" + integration.install_all([]) + + Then "brew tap ran through sudo -n as the prefix owner (Mocha-verified)" + true + + Cleanup + FileUtils.chmod(0o755, prefix) + FileUtils.rm_rf(dir) + end + test "resolve_file_url resolves a ./ path against the project dir" do Given "an integration with a project dir and a project-relative file URI" dir = Dir.mktmpdir("dev-brew-int-test-") From fd047395dcdc946c6b42548a397daa173d2f073a Mon Sep 17 00:00:00 2001 From: Jean-Philippe Date: Tue, 15 Sep 2026 10:27:05 -0400 Subject: [PATCH 2/2] Fix the tap splat for sorbet and cover the escalation seams srb tc rejects splatting a dynamically-sized array into system() (error 7019); route the escalation argv through T.unsafe like run_brew_install already does. Cover the changed lines codecov flagged: URL-less tap registration (with and without escalation, asserting the remediation hint only appears when escalated) and brew --prefix discovery (memoized success and ENOENT-absent brew). Co-authored-by: Cursor --- lib/dev/deps/brew_integration.rb | 6 +- test/dev/deps/brew_integration_test.rb | 91 ++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 3 deletions(-) diff --git a/lib/dev/deps/brew_integration.rb b/lib/dev/deps/brew_integration.rb index 0f69b76..0788f94 100644 --- a/lib/dev/deps/brew_integration.rb +++ b/lib/dev/deps/brew_integration.rb @@ -102,14 +102,14 @@ def register_tap(tap) url = tap.url if tap.local? && project_dir && url path = resolve_file_url(url, project_dir) - success = system(*escalation, "brew", "tap", tap.name, path) + success = system(*T.unsafe(escalation), "brew", "tap", tap.name, path) raise TapRegistrationError, "brew tap #{tap.name} #{path} failed#{escalation_hint}" unless success elsif url url_str = url.to_s - success = system(*escalation, "brew", "tap", tap.name, url_str) + success = system(*T.unsafe(escalation), "brew", "tap", tap.name, url_str) raise TapRegistrationError, "brew tap #{tap.name} #{url_str} failed#{escalation_hint}" unless success else - success = system(*escalation, "brew", "tap", tap.name) + success = system(*T.unsafe(escalation), "brew", "tap", tap.name) raise TapRegistrationError, "brew tap #{tap.name} failed#{escalation_hint}" unless success end end diff --git a/test/dev/deps/brew_integration_test.rb b/test/dev/deps/brew_integration_test.rb index 0571e52..d51e60a 100644 --- a/test/dev/deps/brew_integration_test.rb +++ b/test/dev/deps/brew_integration_test.rb @@ -316,6 +316,97 @@ class Dev::Deps::BrewIntegrationTest < Minitest::Test FileUtils.rm_rf(dir) end + test "a URL-less tap registers by name alone and a failure raises without the escalation hint" do + Given "a tap with no URL and a writable brew prefix, where brew tap fails" + dir = Dir.mktmpdir("dev-brew-int-test-") + tap = Dev::Deps::Tap.new(name: "org/tap") + integration = Dev::Deps::BrewIntegration.new( + repository: Dev::Deps::BrewRepository.new, + cache: Dev::Deps::Cache.new(cache_dir: dir), + taps: [tap], project_dir: dir, brew_prefix: dir, + ) + integration.stubs(:system).with("brew", "tap", "org/tap").returns(false) + + When "installing all (no deps, taps only)" + error = assert_raises(Dev::Deps::BrewIntegration::TapRegistrationError) do + integration.install_all([]) + end + + Then "the error names the tap and carries no escalation hint (we were not escalated)" + error.message.include?("brew tap org/tap") + !error.message.include?("dev runner register") + + Cleanup + FileUtils.rm_rf(dir) + end + + test "an escalated tap failure carries the sudoers-edge remediation hint" do + Given "a URL-less tap and an unwritable brew prefix, where the escalated brew tap fails" + dir = Dir.mktmpdir("dev-brew-int-test-") + prefix = File.join(dir, "homebrew") + FileUtils.mkdir_p(prefix) + FileUtils.chmod(0o555, prefix) + owner = Etc.getpwuid(File.stat(prefix).uid).name + tap = Dev::Deps::Tap.new(name: "org/tap") + integration = Dev::Deps::BrewIntegration.new( + repository: Dev::Deps::BrewRepository.new, + cache: Dev::Deps::Cache.new(cache_dir: dir), + taps: [tap], project_dir: dir, brew_prefix: prefix, + ) + integration.stubs(:system).with("sudo", "-n", "-u", owner, "brew", "tap", "org/tap").returns(false) + + When "installing all (no deps, taps only)" + error = assert_raises(Dev::Deps::BrewIntegration::TapRegistrationError) do + integration.install_all([]) + end + + Then "the error points at the sudoers brew edge remediation" + error.message.include?("dev runner register") + + Cleanup + FileUtils.chmod(0o755, prefix) + FileUtils.rm_rf(dir) + end + + test "brew_prefix is discovered from brew --prefix once and memoized" do + Given "an integration with no injected prefix and a brew that answers --prefix" + dir = Dir.mktmpdir("dev-brew-int-test-") + integration = Dev::Deps::BrewIntegration.new( + repository: Dev::Deps::BrewRepository.new, cache: Dev::Deps::Cache.new(cache_dir: dir), + ) + Open3.expects(:capture3).with("brew", "--prefix").once.returns(["#{dir}\n", "", stub(success?: true)]) + + When "resolving the prefix twice" + first = integration.send(:brew_prefix) + second = integration.send(:brew_prefix) + + Then "both resolutions return the discovered prefix from a single brew call (Mocha-verified once)" + first == dir + second == dir + + Cleanup + FileUtils.rm_rf(dir) + end + + test "brew_prefix is nil when brew is absent, so writes run unescalated" do + Given "an integration with no injected prefix and no brew on PATH" + dir = Dir.mktmpdir("dev-brew-int-test-") + integration = Dev::Deps::BrewIntegration.new( + repository: Dev::Deps::BrewRepository.new, cache: Dev::Deps::Cache.new(cache_dir: dir), + ) + Open3.stubs(:capture3).with("brew", "--prefix").raises(Errno::ENOENT) + + When "resolving the prefix" + prefix = integration.send(:brew_prefix) + + Then "the prefix is nil and escalation is empty" + prefix.nil? + integration.send(:escalation) == [] + + Cleanup + FileUtils.rm_rf(dir) + end + test "resolve_file_url resolves a ./ path against the project dir" do Given "an integration with a project dir and a project-relative file URI" dir = Dir.mktmpdir("dev-brew-int-test-")