diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bc3940..a04cb7d 100644 --- a/CHANGELOG.md +++ b/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.** diff --git a/docs/trust-and-preflight.md b/docs/trust-and-preflight.md index c316524..ec9c626 100644 --- a/docs/trust-and-preflight.md +++ b/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 diff --git a/skills/pr-batch/bin/agent-coord-bounded b/skills/pr-batch/bin/agent-coord-bounded index e6c1748..b95da06 100755 --- a/skills/pr-batch/bin/agent-coord-bounded +++ b/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 diff --git a/skills/pr-batch/bin/agent-coord-bounded-test.rb b/skills/pr-batch/bin/agent-coord-bounded-test.rb index a6464bb..b59564a 100755 --- a/skills/pr-batch/bin/agent-coord-bounded-test.rb +++ b/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