fix(signals): Ctrl-C and SIGTERM do not stop the loop

ralph_loop.sh

@@ -2099,6 +2099,9 @@ execute_claude_code() {
 
         # Get PID and monitor progress
         local claude_pid=$!
+        # Expose the child to on_signal(): claude_pid is local, and a SIGTERM
+        # sent to this script does not reach a backgrounded child.
+        CLAUDE_CHILD_PID=$claude_pid
         local progress_counter=0
 
         # Early failure detection: if the command doesn't exist or fails immediately,
@@ -2174,6 +2177,7 @@ EOF
         # Wait for the process to finish and get exit code
         wait $claude_pid
         exit_code=$?
+        CLAUDE_CHILD_PID=""
     fi
 
     # Issue #75: pull changed files back from the cloud sandbox BEFORE the
@@ -2452,16 +2456,177 @@ cleanup() {
 
     # Only record "interrupted" status for abnormal exits (non-zero exit code)
     # Normal exit (code 0) preserves the status already written by the main loop
-    if [[ $loop_count -gt 0 && $trap_exit_code -ne 0 ]]; then
+    # _INTERRUPTED is authoritative for signal paths: in a trap handler $? is the
+    # status of the last completed command, NOT 128+n, so trap_exit_code alone
+    # records the interrupt only when the preceding command happened to fail.
+    if [[ $loop_count -gt 0 && ( $trap_exit_code -ne 0 || "${_INTERRUPTED:-false}" == "true" ) ]]; then
         log_status "INFO" "Ralph loop interrupted. Cleaning up..."
         reset_session "manual_interrupt"
         update_status "$loop_count" "$(cat "$CALL_COUNT_FILE" 2>/dev/null || echo "0")" "interrupted" "stopped"
     fi
-    # No exit here — EXIT trap handles natural termination
+    # No exit here — cleanup() is teardown only, and is called from both the
+    # normal loop-exit path and on_signal() below. The signal handler owns
+    # termination; see the warning there.
+}
+
+# ⚠️ WHY STOPPING NEEDS A TREE WALK AND NOT A kill.
+#
+# `$!` does NOT reliably name the agent. portable_timeout is a shell function,
+# so backgrounding it forks a subshell; bash sometimes replaces that subshell
+# with an exec of gtimeout and sometimes does not, and which one you get varies
+# with the execution context. Measured, mid-work, under a real pty:
+#
+#     loop      pid 94095   pgid 94095   <- foreground group; gets the Ctrl-C
+#     bash      pid 94100   pgid 94095   <- $! captured THIS, not the agent
+#     gtimeout  pid 94107   pgid 94107   <- gtimeout setpgid()s ITSELF
+#     claude    pid 94112   pgid 94107   <- the agent, in that other group
+#
+# Two consequences, both load-bearing:
+#   - Ctrl-C signals only the foreground group, so the agent never sees it.
+#   - TERMing $! kills the subshell; gtimeout and the agent are orphaned and
+#     keep working. Measured: files written 6s, 11s and 17s after the stop,
+#     and the API call billed in full.
+# A `kill -$PGID` is NOT the answer either: the agent is not in our group, so
+# that signals the loop itself and spares the agent. Membership must be
+# established by parentage before anything is signalled, never assumed.
+#
+# Targets bash 3.2 (`#!/bin/bash` on macOS): no mapfile, no readarray, no
+# associative arrays, no array expansions that break under `set -u`.
+
+# Print every descendant of $1 (excluding $1), one pid per line, shallowest
+# first, proven against the live ppid table.
+list_descendants() {
+    _ld_root="$1"
+    [ -n "$_ld_root" ] || return 0
+    _ld_snapshot=`ps -eo pid=,ppid= 2>/dev/null`
+    [ -n "$_ld_snapshot" ] || return 0
+
+    _ld_frontier="$_ld_root"
+    _ld_found=""
+    _ld_guard=0
+    while [ -n "$_ld_frontier" ]; do
+        _ld_guard=`expr $_ld_guard + 1`
+        [ "$_ld_guard" -gt 64 ] && break
+        _ld_next=""
+        while read -r _ld_p _ld_pp; do
+            [ -n "$_ld_p" ] || continue
+            for _ld_f in $_ld_frontier; do
+                if [ "$_ld_pp" = "$_ld_f" ]; then
+                    _ld_next="$_ld_next $_ld_p"
+                    break
+                fi
+            done
+        done <<EOF
+$_ld_snapshot
+EOF
+        [ -n "$_ld_next" ] || break
+        _ld_found="$_ld_found $_ld_next"
+        _ld_frontier="$_ld_next"
+    done
+
+    for _ld_p in $_ld_found; do echo "$_ld_p"; done
+}
+
+# kill_tree ROOT_PID [GRACE_SECONDS]
+# TERM every verified member deepest-first, then SIGKILL whatever outlives the
+# grace period. Returns 0 if the tree is gone, 1 if anything survived SIGKILL.
+kill_tree() {
+    _kt_root="$1"
+    _kt_grace="${2:-5}"
+    [ -n "$_kt_root" ] || return 0
+
+    # Never signal ourselves or our own ancestors, whatever the ppid table
+    # says: a stop control that kills the supervisor and leaves the agent
+    # running is the failure this whole function exists to prevent.
+    _kt_forbidden=""
+    _kt_a=$$
+    _kt_guard=0
+    while [ -n "$_kt_a" ] && [ "$_kt_a" != "0" ] && [ "$_kt_a" != "1" ]; do
+        _kt_forbidden="$_kt_forbidden $_kt_a"
+        _kt_guard=`expr $_kt_guard + 1`
+        [ "$_kt_guard" -gt 64 ] && break
+        _kt_a=`ps -o ppid= -p "$_kt_a" 2>/dev/null | tr -d ' '`
+    done
+    _kt_forbids() {
+        for _kt_x in $_kt_forbidden; do
+            [ "$1" = "$_kt_x" ] && return 0
+        done
+        return 1
+    }
+
+    # ⛔ ENUMERATE BEFORE SIGNALLING. Once the root dies its descendants are
+    # reparented to launchd and no later walk can ever find them.
+    _kt_members=""
+    for _kt_p in `list_descendants "$_kt_root"`; do
+        _kt_forbids "$_kt_p" || _kt_members="$_kt_p $_kt_members"
+    done
+    _kt_forbids "$_kt_root" || _kt_members="$_kt_members $_kt_root"
+    [ -n "$_kt_members" ] || return 0
+
+    for _kt_p in $_kt_members; do kill -TERM "$_kt_p" 2>/dev/null; done
+
+    _kt_waited=0
+    _kt_tenths=`expr $_kt_grace \* 10`
+    while [ "$_kt_waited" -lt "$_kt_tenths" ]; do
+        _kt_any=no
+        for _kt_p in $_kt_members; do
+            if kill -0 "$_kt_p" 2>/dev/null; then _kt_any=yes; break; fi
+        done
+        [ "$_kt_any" = "no" ] && return 0
+        sleep 0.1
+        _kt_waited=`expr $_kt_waited + 1`
+    done
+
+    # Re-enumerate from SURVIVING members (not the root, which may be gone) to
+    # catch anything spawned during the grace window.
+    _kt_late=""
+    for _kt_p in $_kt_members; do
+        if kill -0 "$_kt_p" 2>/dev/null; then
+            for _kt_q in `list_descendants "$_kt_p"`; do
+                _kt_forbids "$_kt_q" || _kt_late="$_kt_late $_kt_q"
+            done
+        fi
+    done
+    for _kt_p in $_kt_members $_kt_late; do
+        kill -0 "$_kt_p" 2>/dev/null && kill -KILL "$_kt_p" 2>/dev/null
+    done
+
+    sleep 0.3
+    for _kt_p in $_kt_members $_kt_late; do
+        if kill -0 "$_kt_p" 2>/dev/null; then return 1; fi
+    done
+    return 0
 }
 
 # Set up signal handlers
-trap cleanup SIGINT SIGTERM
+# ⚠️ cleanup() deliberately does NOT exit — it is teardown only. A signal trap
+# that merely calls it RETURNS, and bash resumes the main loop: the operator
+# sees "interrupted", and the loop keeps running. The handler must terminate
+# the process itself, and must also stop the backgrounded Claude child, which
+# a SIGTERM sent to this script alone never reaches.
+_INTERRUPTED=false
+on_signal() {
+    local sig="$1"
+    # Restore defaults FIRST so a second ^C always kills, even if cleanup hangs.
+    trap - SIGINT SIGTERM
+    _INTERRUPTED=true
+    log_status "WARN" "Received SIG${sig} — stopping Ralph…"
+    if [[ -n "${CLAUDE_CHILD_PID:-}" ]] && kill -0 "$CLAUDE_CHILD_PID" 2>/dev/null; then
+        # NOT `kill -TERM "$CLAUDE_CHILD_PID"`: that pid is usually a wrapper,
+        # and killing it orphans the agent, which keeps working and keeps
+        # billing. Walk the tree and terminate every verified member.
+        if kill_tree "$CLAUDE_CHILD_PID" 5; then
+            log_status "INFO" "Claude process tree stopped"
+        else
+            log_status "ERROR" "⛔ Claude process tree SURVIVED SIGKILL — check for strays before relaunching"
+        fi
+    fi
+    cleanup
+    # Re-raise so we die of the signal with the correct exit status (128+n).
+    kill -s "$sig" $$
+}
+trap 'on_signal INT'  SIGINT
+trap 'on_signal TERM' SIGTERM
 
 # Global variable for loop count (needed by cleanup function)
 loop_count=0

tests/integration/test_process_tree_kill.bats

@@ -0,0 +1,173 @@
+#!/usr/bin/env bats
+#
+# Regression tests for kill_tree() / list_descendants() — the stop control's
+# child-kill leg.
+#
+# The bug these cover: the loop stored `$!` in CLAUDE_CHILD_PID and sent it a
+# single SIGTERM. That pid is not reliably the agent. Measured mid-work under a
+# real pty:
+#
+#     loop      pid 94095  pgid 94095   <- foreground group; receives the Ctrl-C
+#     bash      pid 94100  pgid 94095   <- $! captured THIS, not the agent
+#     gtimeout  pid 94107  pgid 94107   <- gtimeout setpgid()s ITSELF
+#     claude    pid 94112  pgid 94107   <- the agent, in that other group
+#
+# `portable_timeout` is a shell function, so backgrounding it forks a subshell;
+# bash sometimes replaces that subshell with an exec of the timeout binary and
+# sometimes does not, varying with execution context. Killing the captured pid
+# therefore killed a wrapper and orphaned the agent, which kept working and
+# kept billing. A process-group kill is not the answer either — the agent is
+# not in the loop's group, so `-$PGID` signals the loop and spares the agent.
+#
+# These tests use `sleep` trees. They make no API calls and cost nothing.
+#
+# Each behavioural test that proves kill_tree works is paired with a control
+# that must genuinely FAIL first — see "a single-pid TERM leaves the tree
+# alive". If that control ever passes, the others prove nothing and it fails
+# loudly rather than going green for the wrong reason.
+#
+# Written for bash 3.2 (`#!/bin/bash` on macOS): no mapfile, no readarray, and
+# no array expansion that aborts under `set -u`.
+
+load ../helpers/test_helper
+
+RALPH_LOOP="${BATS_TEST_DIRNAME}/../../ralph_loop.sh"
+
+# Run a snippet with the real ralph_loop.sh sourced, in a subshell so the
+# loop's own SIGINT/SIGTERM traps are never installed in the bats shell.
+# The functions under test are the ones in the shipped file, not a copy.
+in_loop_ctx() {
+    bash -c "source '$RALPH_LOOP' >/dev/null 2>&1; $1"
+}
+
+@test "list_descendants finds a child and a grandchild through a wrapper" {
+    run in_loop_ctx '
+        bash -c "bash -c \"sleep 30\" & sleep 30" &
+        root=$!
+        sleep 1
+        n=0
+        for p in $(list_descendants "$root"); do n=$((n + 1)); done
+        kill -KILL $(list_descendants "$root") "$root" 2>/dev/null || true
+        echo "descendants=$n"
+    '
+    [ "$status" -eq 0 ]
+    [[ "$output" == *"descendants=2"* ]]
+}
+
+@test "a single-pid TERM leaves the tree alive — the defect this fix exists to stop" {
+    # KNOWN-FALSE CONTROL. This asserts the OLD behaviour still fails. If this
+    # ever passes, every kill_tree test below is proving nothing.
+    run in_loop_ctx '
+        bash -c "bash -c \"sleep 30\" & sleep 30" &
+        root=$!
+        sleep 1
+        kids=$(list_descendants "$root")
+        kill -TERM "$root" 2>/dev/null      # exactly the old child-kill leg
+        sleep 1
+        alive=0
+        for p in $kids; do kill -0 "$p" 2>/dev/null && alive=$((alive + 1)); done
+        kill -KILL $kids 2>/dev/null || true
+        echo "survivors=$alive"
+    '
+    [ "$status" -eq 0 ]
+    [[ "$output" == *"survivors=2"* ]]
+}
+
+@test "kill_tree clears the whole tree from a live root" {
+    run in_loop_ctx '
+        bash -c "bash -c \"sleep 30\" & sleep 30" &
+        root=$!
+        sleep 1
+        kids=$(list_descendants "$root")
+        kill_tree "$root" 3
+        sleep 1
+        alive=0
+        for p in $root $kids; do kill -0 "$p" 2>/dev/null && alive=$((alive + 1)); done
+        kill -KILL $root $kids 2>/dev/null || true
+        echo "alive=$alive"
+    '
+    [ "$status" -eq 0 ]
+    [[ "$output" == *"alive=0"* ]]
+}
+
+@test "kill_tree reaches a descendant in a DIFFERENT process group" {
+    # The production shape: the timeout binary puts itself and the agent into
+    # their own group, so group membership cannot be assumed from the pid the
+    # loop captured. Parentage is the only test that survives this.
+    command -v perl >/dev/null || skip "perl required to create a new process group"
+    run in_loop_ctx '
+        bash -c "perl -e \"setpgrp(0,0); exec q(sleep), q(30)\" & sleep 30" &
+        root=$!
+        sleep 1
+        kids=$(list_descendants "$root")
+        rootpg=$(ps -o pgid= -p "$root" | tr -d " ")
+        crossed=no
+        for p in $kids; do
+            pg=$(ps -o pgid= -p "$p" 2>/dev/null | tr -d " ")
+            [ -n "$pg" ] && [ "$pg" != "$rootpg" ] && crossed=yes
+        done
+        kill_tree "$root" 3
+        sleep 1
+        alive=0
+        for p in $root $kids; do kill -0 "$p" 2>/dev/null && alive=$((alive + 1)); done
+        kill -KILL $root $kids 2>/dev/null || true
+        echo "crossed=$crossed alive=$alive"
+    '
+    [ "$status" -eq 0 ]
+    # The fixture must actually straddle a group boundary, or it tests nothing.
+    [[ "$output" == *"crossed=yes"* ]]
+    [[ "$output" == *"alive=0"* ]]
+}
+
+@test "kill_tree does not kill its own caller" {
+    # A candidate fix that signalled -$PGID killed the supervisor and left the
+    # target running. A stop control that kills the supervisor is worse than
+    # one that does nothing.
+    run in_loop_ctx '
+        bash -c "sleep 30" &
+        root=$!
+        sleep 1
+        kill_tree "$root" 3
+        # The target MUST be dead as well, or this test passes vacuously
+        # wherever kill_tree does not exist — a test that goes green against
+        # the unpatched loop is worthless.
+        if kill -0 "$root" 2>/dev/null; then target=alive; else target=dead; fi
+        kill -KILL "$root" 2>/dev/null || true
+        # Cleanup must not be the last command: killing an already-dead pid
+        # exits non-zero and would fail the test for the wrong reason.
+        kill -0 $$ 2>/dev/null && echo "caller=alive target=$target" \
+                               || echo "caller=dead target=$target"
+    '
+    [ "$status" -eq 0 ]
+    [[ "$output" == *"caller=alive"* ]]
+    [[ "$output" == *"target=dead"* ]]
+}
+
+@test "kill_tree escalates to SIGKILL past a process that ignores SIGTERM" {
+    run in_loop_ctx '
+        bash -c "trap \"\" TERM; sleep 30" &
+        stubborn=$!
+        sleep 1
+        kill_tree "$stubborn" 2
+        sleep 1
+        if kill -0 "$stubborn" 2>/dev/null; then
+            kill -KILL "$stubborn" 2>/dev/null
+            echo "stubborn=survived"
+        else
+            echo "stubborn=killed"
+        fi
+    '
+    [ "$status" -eq 0 ]
+    [[ "$output" == *"stubborn=killed"* ]]
+}
+
+@test "the signal handler stops the tree, not a single pid" {
+    # Comment lines are excluded deliberately: the handler carries a comment
+    # naming the old form to explain why it is wrong, and a naive grep counts
+    # that as the bug still being present.
+    run bash -c "grep -v '^[[:space:]]*#' '$RALPH_LOOP' | grep -cF 'kill -TERM \"\$CLAUDE_CHILD_PID\"'"
+    [ "$output" -eq 0 ]
+
+    run bash -c "grep -cF 'kill_tree \"\$CLAUDE_CHILD_PID\"' '$RALPH_LOOP'"
+    [ "$output" -ge 1 ]
+}