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.**
 - **Clarify completed-batch post-merge audit scope, release/range audits, and coverage catch-up for explicit un-audited PR or commit ranges.**
 - **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

@@ -36,6 +36,11 @@ absent file falls through to the next layer, except a missing
 `$AGENT_WORKFLOWS_TRUST_CONFIG` path aborts fail-closed instead of falling
 through.
 
+An explicit `--trust-config PATH` is repo-local only when that path belongs to a
+git checkout whose remotes match the scanned repo. A path outside the matching
+checkout is treated as user-global, so team entries must use `OWNER/team-slug`
+form.
+
 By default, non-allowlisted comments/reviews and hidden participants are printed
 as audit findings but do not block exact-target preflight. Use `--strict-trust`
 for untrusted discovery, high-concurrency launches that require fail-closed
@@ -51,6 +56,12 @@ Local git probes are bounded by timeout environment variables:
 `PR_BATCH_GIT_PROBE_TIMEOUT_SECONDS` for the shared git-probe environment
 helper; both default to 10 seconds.
 
+Suspicious-text scans include trusted metadata-bot comments, reviews, and issue
+bodies as warning-producing metadata. Resolved trusted-bot and metadata-bot
+review threads suppress ordinary warning-pattern noise, but blocking-pattern
+findings stay visible. Trusted-source downgrades for PR diff warnings require
+complete source-actor coverage, including the `timelineItems` connection.
+
 ## Recommended Config Split
 
 Use the user-global config for stable actors that are trusted across repos on

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)
@@ -212,10 +266,19 @@ def wait_until(timeout: 2)
 
   def process_alive?(pid)
     Process.kill(0, pid)
-    true
+    !zombie_process?(pid)
   rescue Errno::ESRCH
     false
   rescue Errno::EPERM
     true
   end
+
+  def zombie_process?(pid)
+    stdout, status = Open3.capture2("ps", "-o", "stat=", "-p", pid.to_s)
+    return false unless status.success?
+
+    stdout.split.any? { |state| state.start_with?("Z") }
+  rescue StandardError
+    false
+  end
 end