Port consumer-repo preflight and coord hardening

CHANGELOG.md

@@ -16,6 +16,8 @@ All notable changes to this portable workflow pack are documented here.
 
 #### Changed
 
+- **Port consumer-repo preflight hardening: fail closed when `AGENT_WORKFLOWS_TRUST_CONFIG` points to a missing file, treat explicit `--trust-config` paths outside the consuming repo's git root as user-global (warning on ignored unqualified team slugs), scan trusted metadata-bot comments for suspicious-text warnings, keep blocking-pattern warnings visible on resolved trusted-bot review threads, and require full source-actor timeline coverage before treating a PR source as trusted for diff-warning downgrades.**
+- **Port consumer-repo `agent-coord-bounded` hardening: preserve captured stdout/stderr on interrupt and timeout exits, and wait for the whole process group to exit during termination.**
 - **Default post-merge audits to the obvious just-run batch before asking for batch confirmation.**
 - **Start pasteable batch prompts with a short title that includes a repository-derived project abbreviation, optional A/B/C split marker, and `MM-DD HH:MM` from the local shell `date` command.**
 

docs/trust-and-preflight.md

@@ -32,7 +32,14 @@ The preflight resolves trust in this order:
 5. packaged `skills/pr-batch/trusted-github-actors.yml`
 
 A present empty file is an intentional policy and does not fall through. An
-absent file falls through to the next layer.
+absent file falls through to the next layer, with one fail-closed exception: a
+set `$AGENT_WORKFLOWS_TRUST_CONFIG` that points to a missing file aborts the
+run instead of silently falling through to weaker layers.
+
+An explicit `--trust-config PATH` inside the consuming repo's git root is
+treated as repo-local; a path outside the git root is treated as user-global,
+so its team entries must use `OWNER/team-slug` form (unqualified slugs are
+ignored with a warning).
 
 By default, non-allowlisted comments/reviews and hidden participants are printed
 as audit findings but do not block exact-target preflight. Use `--strict-trust`

skills/pr-batch/bin/agent-coord-bounded

@@ -37,13 +37,36 @@ rescue Errno::ESRCH
   nil
 end
 
+def process_group_alive?(pid)
+  Process.kill(0, -pid)
+  true
+rescue Errno::ESRCH
+  false
+rescue Errno::EPERM
+  true
+end
+
+def wait_for_process_group_exit(pid, deadline)
+  while Time.now < deadline
+    return true unless process_group_alive?(pid)
+
+    sleep 0.05
+  end
+
+  !process_group_alive?(pid)
+end
+
 def terminate_process_group(pid, signal: "TERM", grace_seconds: 1, kill_grace_seconds: 1)
+  term_deadline = Time.now + grace_seconds
   signal_process_group(pid, signal)
-  status = wait_for_child(pid, Time.now + grace_seconds)
+  status = wait_for_child(pid, term_deadline)
+  return status if wait_for_process_group_exit(pid, term_deadline)
 
   # The direct child may exit before helper processes in its process group.
   signal_process_group(pid, "KILL")
-  status ||= wait_for_child(pid, Time.now + kill_grace_seconds)
+  kill_deadline = Time.now + kill_grace_seconds
+  status ||= wait_for_child(pid, kill_deadline)
+  wait_for_process_group_exit(pid, kill_deadline)
 
   status
 end
@@ -55,6 +78,13 @@ def exit_code_for_status(status)
   status.exitstatus || 1
 end
 
+def flush_captured_output(stdout_file, stderr_file)
+  stdout_file.rewind
+  stderr_file.rewind
+  print stdout_file.read
+  $stderr.print(stderr_file.read)
+end
+
 options = {}
 
 parser = OptionParser.new do |opts|
@@ -114,6 +144,7 @@ begin
   loop do
     if interrupted_signal
       terminate_process_group(pid, signal: interrupted_signal)
+      flush_captured_output(stdout_file, stderr_file)
       exit(128 + Signal.list.fetch(interrupted_signal))
     end
 
@@ -133,20 +164,19 @@ begin
 
   if interrupted_signal
     terminate_process_group(pid, signal: interrupted_signal)
+    flush_captured_output(stdout_file, stderr_file)
     exit(128 + Signal.list.fetch(interrupted_signal))
   end
 
   terminate_process_group(pid) if timed_out
 
   if timed_out
+    flush_captured_output(stdout_file, stderr_file)
     warn "agent-coord-bounded: timed out after #{timeout}s: #{Shellwords.join(command)}"
     exit 124
   end
 
-  stdout_file.rewind
-  stderr_file.rewind
-  print stdout_file.read
-  $stderr.print(stderr_file.read)
+  flush_captured_output(stdout_file, stderr_file)
 
   exit(exit_code_for_status(status))
 ensure

skills/pr-batch/bin/agent-coord-bounded-test.rb

@@ -27,14 +27,17 @@ def test_forwards_agent_coord_exit_status_stdout_and_stderr
   def test_times_out_and_reports_unknown_friendly_status
     with_fake_agent_coord(<<~RUBY) do |env|
       puts "partial json output"
+      $stderr.puts "partial stderr output"
       $stdout.flush
+      $stderr.flush
       sleep 5
     RUBY
-      stdout, stderr, status = run_script(env, "--timeout", "0.2", "doctor", "--json")
+      stdout, stderr, status = run_script(env, "--timeout", "1", "doctor", "--json")
 
       assert_equal 124, status.exitstatus
-      assert_empty stdout
-      assert_includes stderr, "agent-coord-bounded: timed out after 0.2s"
+      assert_equal "partial json output\n", stdout
+      assert_includes stderr, "partial stderr output"
+      assert_includes stderr, "agent-coord-bounded: timed out after 1.0s"
       assert_includes stderr, "agent-coord doctor --json"
     end
   end
@@ -116,15 +119,21 @@ def test_reports_missing_agent_coord_without_backtrace
   def test_terminates_agent_coord_process_group_when_interrupted
     Dir.mktmpdir("agent-coord-bounded-test") do |dir|
       child_pid_file = File.join(dir, "child.pid")
+      wrapper_stdout_file = File.join(dir, "wrapper.stdout")
+      wrapper_stderr_file = File.join(dir, "wrapper.stderr")
       wrapper_pid = nil
       child_pid = nil
 
       with_fake_agent_coord(<<~RUBY, "AGENT_COORD_CHILD_PID" => child_pid_file) do |env|
+        puts "interrupted stdout output"
+        $stderr.puts "interrupted stderr output"
+        $stdout.flush
+        $stderr.flush
         File.write(ENV.fetch("AGENT_COORD_CHILD_PID"), Process.pid.to_s)
         sleep 10
       RUBY
         wrapper_pid = Process.spawn(env, RbConfig.ruby, SCRIPT, "--timeout", "20", "status",
-                                    out: File::NULL, err: File::NULL)
+                                    out: wrapper_stdout_file, err: wrapper_stderr_file)
 
         assert wait_until(timeout: 5) { File.size?(child_pid_file) }, "fake agent-coord did not start"
 
@@ -133,6 +142,8 @@ def test_terminates_agent_coord_process_group_when_interrupted
         _, status = Process.waitpid2(wrapper_pid)
 
         assert_equal 143, status.exitstatus
+        assert_equal "interrupted stdout output\n", File.read(wrapper_stdout_file)
+        assert_includes File.read(wrapper_stderr_file), "interrupted stderr output"
         assert wait_until(timeout: 5) { !process_alive?(child_pid) }, "fake agent-coord survived wrapper termination"
       ensure
         Process.kill("KILL", wrapper_pid) if wrapper_pid && process_alive?(wrapper_pid)
@@ -170,8 +181,51 @@ def test_timeout_kills_remaining_process_group_helpers
     end
   end
 
+  def test_timeout_replays_helper_output_before_killing_process_group
+    Dir.mktmpdir("agent-coord-bounded-test") do |dir|
+      helper_pid_file = File.join(dir, "helper.pid")
+      helper_pid = nil
+
+      with_fake_agent_coord(<<~RUBY, "AGENT_COORD_HELPER_PID" => helper_pid_file) do |env|
+        helper_code = <<~'HELPER'
+          trap("TERM") do
+            sleep 0.2
+            puts "helper stdout after TERM"
+            $stderr.puts "helper stderr after TERM"
+            $stdout.flush
+            $stderr.flush
+            exit! 0
+          end
+          File.write(ENV.fetch("AGENT_COORD_HELPER_PID"), Process.pid.to_s)
+          sleep 10
+        HELPER
+        helper_pid = Process.spawn({ "AGENT_COORD_HELPER_PID" => ENV.fetch("AGENT_COORD_HELPER_PID") },
+                                   #{RbConfig.ruby.dump}, "-e", helper_code)
+        Process.detach(helper_pid)
+        deadline = Time.now + 5
+        sleep 0.05 until File.size?(ENV.fetch("AGENT_COORD_HELPER_PID")) || Time.now >= deadline
+        sleep 10
+      RUBY
+        stdout, stderr, status = run_script(env, "--timeout", "1", "status")
+
+        assert_equal 124, status.exitstatus
+        assert_includes stdout, "helper stdout after TERM"
+        assert_includes stderr, "helper stderr after TERM"
+        assert_includes stderr, "agent-coord-bounded: timed out after 1.0s"
+        assert wait_until(timeout: 5) { File.size?(helper_pid_file) }, "fake helper did not start"
+
+        helper_pid = File.read(helper_pid_file).to_i
+        assert wait_until(timeout: 5) { !process_alive?(helper_pid) }, "helper survived process-group cleanup"
+      ensure
+        Process.kill("KILL", helper_pid) if helper_pid && process_alive?(helper_pid)
+      end
+    end
+  end
+
   def test_process_alive_treats_eperm_as_alive
     original_kill = Process.method(:kill)
+    # This patches Process globally for the duration of the assertion; the
+    # ensure block restores it so later tests do not inherit the EPERM stub.
     Process.define_singleton_method(:kill) { |*| raise Errno::EPERM }
 
     assert process_alive?(1234)

skills/pr-batch/bin/pr-security-preflight

@@ -194,14 +194,19 @@ def resolved_trust_config(explicit_path)
     expanded = File.expand_path(explicit_path)
     abort "Trust config not found: #{expanded}" unless File.exist?(expanded)
 
-    return { path: expanded, global: false }
+    return { path: expanded, global: !path_inside_git_root?(expanded) }
   end
 
   repo_path = repo_trust_config_path
   return { path: repo_path, global: false } if File.exist?(repo_path)
 
   env_path = ENV[USER_TRUST_CONFIG_ENV].to_s
-  return { path: env_path, global: true } if !env_path.empty? && File.exist?(File.expand_path(env_path))
+  unless env_path.empty?
+    expanded_env_path = File.expand_path(env_path)
+    abort "#{USER_TRUST_CONFIG_ENV} points to a missing trust config: #{expanded_env_path}" unless File.exist?(expanded_env_path)
+
+    return { path: expanded_env_path, global: true }
+  end
 
   home_path = File.expand_path(HOME_TRUST_CONFIG)
   return { path: home_path, global: true } if File.exist?(home_path)
@@ -222,6 +227,22 @@ rescue StandardError
   nil
 end
 
+def path_inside_git_root?(path)
+  root = git_toplevel
+  return false unless root
+
+  expanded_root = canonical_path(root)
+  expanded_path = canonical_path(path)
+  expanded_path == expanded_root ||
+    expanded_path.start_with?("#{expanded_root}#{File::SEPARATOR}")
+end
+
+def canonical_path(path)
+  File.realpath(path)
+rescue StandardError
+  File.expand_path(path)
+end
+
 def normalized_team_entry(value, require_owner:)
   owner_or_slug, slug = value.to_s.split("/", 2)
   return if owner_or_slug.empty?
@@ -231,6 +252,8 @@ def normalized_team_entry(value, require_owner:)
 
     { owner: normalized_login(owner_or_slug), slug: }
   elsif require_owner
+    warn "WARN: global trust config ignores unqualified team slug #{owner_or_slug.inspect}: " \
+         "use OWNER/team-slug format"
     nil
   else
     { owner: nil, slug: owner_or_slug }
@@ -495,14 +518,19 @@ def target_source_actor_logins(target)
 end
 
 def trusted_pr_source?(repo, target, trust_config:, team_cache:)
-  return false unless commit_author_coverage_findings(target).empty?
+  return false unless source_actor_coverage_findings(target).empty?
 
   logins = pr_source_actor_logins(target)
   return false if logins.empty?
 
   logins.all? { |login| trusted_actor?(repo, login, trust_config, team_cache) }
 end
 
+def actor_trusted_for_suspicious_warning?(repo, login, trust_config, team_cache)
+  trusted_actor?(repo, login, trust_config, team_cache) ||
+    trusted_metadata_bot?(login, trust_config)
+end
+
 def suspicious_issue_text(rest_context, repo, trust_config:, team_cache:, pattern:)
   findings = []
   issue = rest_context.fetch(:issue)
@@ -514,7 +542,7 @@ def suspicious_issue_text(rest_context, repo, trust_config:, team_cache:, patter
 
   rest_context.fetch(:issue_comments).each do |comment|
     author = comment.dig("user", "login")
-    next unless trusted_actor?(repo, author, trust_config, team_cache)
+    next unless actor_trusted_for_suspicious_warning?(repo, author, trust_config, team_cache)
     next unless (comment["body"] || "").match?(pattern)
 
     findings << {
@@ -533,11 +561,12 @@ def suspicious_pr_review_comments(rest_context, repo, trust_config:, team_cache:
 
   rest_context.fetch(:review_comments).each do |comment|
     author = comment.dig("user", "login")
-    next unless trusted_actor?(repo, author, trust_config, team_cache)
+    next unless actor_trusted_for_suspicious_warning?(repo, author, trust_config, team_cache)
     # Resolved trusted-bot review comments are historical review metadata. Keep
-    # unresolved/current bot findings blocking, and fail closed if resolution
-    # state could not be fetched.
-    next if trusted_bot?(author, trust_config) && resolved_review_comment_ids.include?(comment["id"])
+    # unresolved/current bot findings visible, preserve blocking-pattern warnings,
+    # and fail closed if resolution state could not be fetched.
+    next if trusted_bot?(author, trust_config) && resolved_review_comment_ids.include?(comment["id"]) &&
+            !pattern.equal?(BLOCKING_SUSPICIOUS_PATTERN)
     next unless (comment["body"] || "").match?(pattern)
 
     findings << {
@@ -958,6 +987,13 @@ def graph_coverage_findings(target)
     commit_author_coverage_findings(target)
 end
 
+def source_actor_coverage_findings(target)
+  [
+    connection_coverage_finding(target, "timelineItems"),
+    *commit_author_coverage_findings(target)
+  ].compact
+end
+
 def commit_author_coverage_findings(target)
   return [] unless target["headRefOid"]