Stop the isolation boundary when cancellation is trapped before wait

CHANGELOG.md

@@ -12,6 +12,8 @@ The monorepo ships **one shared train version** across all eleven packages, so u
 
 ### Fixed
 
+- macOS review checks stop same-session children that change process groups before removing their staging files.
+- Cancelling `scripts/isolated-check.sh` or `scripts/isolate-run.sh` with TERM, INT, HUP, or QUIT sent only to the script's process stops the sandboxed command before the archive or staging files are removed, including when the signal arrives before session creation, before the session leader is known or just before the wait. A boundary still running `ISOLATE_TERM_GRACE` seconds later (default 10) is killed. SIGKILL still cannot be forwarded.
 - `post`, `threads`, and `complete-resolutions.py` send one GitHub credential on every `gh` call: the `--token-env` personal access token when that option is set, otherwise the App installation token. The named token wins over an ambient `GH_TOKEN` or `GITHUB_TOKEN`.
 - Cloud Agent setup copies `bun` and `bunx` onto `/usr/bin`, so non-interactive shells and the isolated-check sandbox can run them without `~/.bashrc`.
 - The isolated-check sandbox names the program it could not start.

docs/pr-review.md

@@ -348,7 +348,7 @@ Ubuntu's AppArmor restriction on unprivileged user namespaces makes `unshare` fa
 
 Inside the boundary the command can see read-only `/usr` and `/etc`, the named tool binaries, and the archive at `/src` (read-write). A fresh tmpfs is `/tmp` and `/home/reviewer`. Host home, host temp, the parent checkout, agent sockets, and git metadata are not mounted. The environment starts empty except `PATH`, `HOME`, `TMPDIR`, `USER`, `LOGNAME`, `LANG`, and `CI`. Network stays available because install needs it. Review-posting credentials, including `REVIEW_APP_PRIVATE_KEY` and `REVIEW_APP_PRIVATE_KEY_FILE`, are not in the environment and their files are not mounted. Exit 0 means those checks passed. Exit 1 is a check failure. Exit 2 is a bad invocation or an unavailable isolation mechanism. `Check root:` and `Check commit:` are private and stay out of the public review.
 
-Cancelling a check is safe when the signal reaches only the script's own process, as a tool runner's `kill <pid>` does. `isolated-check.sh` passes TERM, INT, HUP, or QUIT on to `isolate-run.sh`, which forwards it to the boundary's session and kills that session if it is still running `ISOLATE_TERM_GRACE` seconds later (default 10). Only after the boundary has stopped does the check exit with 128 plus the signal number and remove the archive. SIGKILL cannot be forwarded; killing either script that way can still leave the boundary running.
+Cancelling a check is safe when the signal reaches only the script's own process, as a tool runner's `kill <pid>` does. `isolated-check.sh` passes TERM, INT, HUP, or QUIT on to `isolate-run.sh`, which forwards it to the boundary's session and kills that session if it is still running `ISOLATE_TERM_GRACE` seconds later (default 10). The stop runs in the trap itself, so a signal that arrives before the session leader is known, or just before `wait`, still stops the session. If the starter has not created its session when the grace expires, the owned starter is killed directly. Only after the boundary has stopped does the check exit with 128 plus the signal number and remove the archive. SIGKILL cannot be forwarded; killing either script that way can still leave the boundary running.
 
 There is no `--live` path in this repository. Interactive stock-host acceptance remains a manual evidence requirement outside the isolated check.
 
@@ -366,4 +366,6 @@ The reviewer also follows [docs/review-rules.md](review-rules.md): logic and syn
 
 - The script does not decide that a diff is correct. The reviewer still reads it.
 - There is no `--live` credential gate in this repository. Interactive stock-host acceptance stays outside the isolated check.
+- Cancellation is forwarded for TERM, INT, HUP, and QUIT. SIGKILL of `isolated-check.sh` or `isolate-run.sh` cannot be forwarded and can leave the boundary running.
+- macOS settles all process groups in the boundary's session before cleanup. It has no pid namespace: a process that starts its own session inside `sandbox-exec` is not stopped with the boundary. On Linux the namespace ends with its PID 1.
 - The boundary does not hide the network. It hides the reviewer's files and credentials.

scripts/isolate-run.sh

@@ -24,7 +24,9 @@
 # QUIT sent to this script's pid alone is forwarded to that session, and the
 # script removes its staging files only after the session has stopped,
 # killing it after ISOLATE_TERM_GRACE seconds (default 10). A cancelled run
-# exits 128 plus the signal number.
+# exits 128 plus the signal number. SIGKILL of this script cannot be
+# forwarded. On macOS there is no pid namespace, so a process that starts
+# its own session inside sandbox-exec is not stopped with the boundary.
 #
 #   scripts/isolate-run.sh --src DIR [--tool NAME] [--ro-bind HOST:DEST] \
 #     [--env NAME] [--set NAME=VALUE] -- COMMAND...
@@ -87,14 +89,85 @@ isolate_unavailable() {
 # scope, so these are globals.
 isolate_sup_leader=""
 isolate_sup_signal=""
-isolate_sup_sent=""
+isolate_sup_stopping=""
+isolate_sup_grace=10
+isolate_sup_darwin=""
+
+# A macOS session can contain several process groups. Signal its members,
+# and on final KILL wait until none can still use the staging files. Linux
+# instead owns a pid namespace, whose death stops even new-session children.
+isolate_sup_darwin_session() {
+  [[ "$isolate_sup_darwin" == 1 ]] || return 0
+  python3 - "$isolate_sup_leader" "$1" "$2" <<'PY'
+import os, signal, subprocess, sys, time
+session = int(sys.argv[1])
+signum = getattr(signal, "SIG" + sys.argv[2])
+settle = sys.argv[3] == "1"
+# Repeated catchable cancellation must not interrupt the final ownership
+# check and let the caller remove files under a surviving writer.
+for name in ("SIGTERM", "SIGINT", "SIGHUP", "SIGQUIT"):
+    signal.signal(getattr(signal, name), signal.SIG_IGN)
+while True:
+    try:
+        listing = subprocess.run(["ps", "-A", "-o", "pid=,stat="], check=True, capture_output=True, text=True)
+    except (OSError, subprocess.SubprocessError):
+        if not settle:
+            break
+        time.sleep(0.01)
+        continue
+    members = []
+    for row in listing.stdout.splitlines():
+        fields = row.split()
+        if len(fields) < 2 or fields[1].startswith("Z"):
+            continue
+        pid = int(fields[0])
+        try:
+            if os.getsid(pid) == session:
+                members.append(pid)
+                os.kill(pid, signum)
+        except ProcessLookupError:
+            pass
+    if not settle or not members:
+        break
+    time.sleep(0.01)
+PY
+}
 
-# Record the first cancellation signal and pass it to the whole session.
+# Record the first cancellation signal and run the stop sequence. A bash
+# trap taken between two commands does not interrupt the wait that follows,
+# so recording the signal and waiting for that wait to return leaves the
+# script blocked when the session ignores TERM.
 isolate_sup_note() {
   [[ -n "$isolate_sup_signal" ]] || isolate_sup_signal="$1"
-  if [[ -n "$isolate_sup_leader" ]] && kill -s "$isolate_sup_signal" -- "-$isolate_sup_leader" 2>/dev/null; then
-    isolate_sup_sent=1
-  fi
+  isolate_sup_stop
+}
+
+# Signal the session, kill it after the grace period, and return only once
+# the session leader is gone. A second call, including one after a trap that
+# already stopped the session, returns immediately. Count 0.1 s sleeps rather
+# than whole-second SECONDS ticks, which can end a one-second grace almost
+# at once.
+isolate_sup_stop() {
+  local ticks sent="" killed=""
+  [[ -n "$isolate_sup_leader" && -z "$isolate_sup_stopping" ]] || return 0
+  isolate_sup_stopping=1
+  ticks=$((isolate_sup_grace * 10))
+  while kill -0 "$isolate_sup_leader" 2>/dev/null; do
+    if [[ -z "$sent" ]] && kill -s "$isolate_sup_signal" -- "-$isolate_sup_leader" 2>/dev/null; then
+      sent=1
+      isolate_sup_darwin_session "$isolate_sup_signal" 0
+    fi
+    if [[ -z "$killed" && "$ticks" -le 0 ]]; then
+      # Cancellation can beat the starter's setsid(), including while the
+      # starter is stopped. Its process group does not exist yet; kill the
+      # owned child directly in that case. Never latch an unsuccessful kill.
+      if kill -s KILL -- "-$isolate_sup_leader" 2>/dev/null || kill -s KILL "$isolate_sup_leader" 2>/dev/null; then
+        killed=1
+      fi
+    fi
+    sleep 0.1 || true
+    ticks=$((ticks - 1))
+  done
 }
 
 # Run COMMAND in DIR as the leader of a new session and wait for it.
@@ -112,13 +185,15 @@ isolate_sup_note() {
 # A cancelled run returns 128 plus the signal number. SIGKILL to this shell
 # cannot be forwarded.
 isolate_supervise() {
-  local dir="$1" status=0 grace ticks killed=""
+  local dir="$1" status=0
   shift
-  grace="${ISOLATE_TERM_GRACE:-10}"
-  [[ "$grace" =~ ^[0-9]+$ ]] || grace=10
+  isolate_sup_grace="${ISOLATE_TERM_GRACE:-10}"
+  [[ "$isolate_sup_grace" =~ ^[0-9]+$ ]] || isolate_sup_grace=10
+  isolate_sup_darwin=""
+  [[ "$(uname -s)" != Darwin ]] || isolate_sup_darwin=1
   command -v python3 >/dev/null 2>&1 || isolate_unavailable "python3 is required to supervise the command." || return 2
   isolate_sup_signal=""
-  isolate_sup_sent=""
+  isolate_sup_stopping=""
   isolate_sup_leader=""
   trap 'isolate_sup_note TERM' TERM
   trap 'isolate_sup_note INT' INT
@@ -139,25 +214,19 @@ os.execvp(sys.argv[1], sys.argv[1:])
 ' "$@"
   ) <&0 &
   isolate_sup_leader=$!
-  wait "$isolate_sup_leader"
-  status=$?
-  if [[ -n "$isolate_sup_signal" ]]; then
-    # Count 0.1 s sleeps rather than whole-second SECONDS ticks, which can
-    # end a one-second grace almost at once.
-    ticks=$((grace * 10))
-    while kill -0 "$isolate_sup_leader" 2>/dev/null; do
-      # The signal can arrive before the session exists; send it once it does.
-      [[ -n "$isolate_sup_sent" ]] || isolate_sup_note "$isolate_sup_signal"
-      if [[ -z "$killed" && "$ticks" -le 0 ]]; then
-        kill -s KILL -- "-$isolate_sup_leader" 2>/dev/null || true
-        killed=1
-      fi
-      sleep 0.1 || true
-      ticks=$((ticks - 1))
-    done
-    wait "$isolate_sup_leader" 2>/dev/null || true
+  if [[ -z "$isolate_sup_signal" ]]; then
+    wait "$isolate_sup_leader"
+    status=$?
+  else
+    # The trap ran before this pid was known, so it could not signal the
+    # session. Stop it now instead of waiting on a command that ignores TERM.
+    isolate_sup_stop
   fi
+  [[ -z "$isolate_sup_signal" ]] || wait "$isolate_sup_leader" 2>/dev/null || true
+  # The Linux boundary's leader is unshare, which ignores TERM. Killing its
+  # process group ends the pid namespace and every process inside it.
   kill -s KILL -- "-$isolate_sup_leader" 2>/dev/null || true
+  isolate_sup_darwin_session KILL 1
   trap - TERM INT HUP QUIT
   if [[ -n "$isolate_sup_signal" ]]; then
     status=$((128 + $(kill -l "$isolate_sup_signal")))

scripts/isolated-check.sh

@@ -17,7 +17,8 @@
 # Exit 0 when the checks pass, 1 when a check fails, 2 when the invocation
 # or the isolation mechanism is unusable, and 128 plus the signal number when
 # TERM, INT, HUP, or QUIT cancels a check. A cancelled check waits for the
-# boundary to stop before it removes the archive.
+# boundary to stop before it removes the archive. SIGKILL of this script
+# cannot be forwarded and can leave the boundary running.
 set -euo pipefail
 export PYTHONDONTWRITEBYTECODE=1
 ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"

tasks/lessons.md

@@ -1,5 +1,12 @@
 # Lessons learned
 
+## 2026-11-10 — A trap before wait does not stop an isolation boundary
+
+- TERM sent only to the pid of `isolate-run.sh`, before the session leader is stored or just before `wait`, leaves the script blocked on a command that ignores TERM. The trap must run the whole stop sequence: signal the session, kill it after `ISOLATE_TERM_GRACE`, and return only when the leader is gone. Prove it by signalling the script pid at those command boundaries with a `BASH_ENV` DEBUG hook, and check with `ps` that the command is gone after the script exits.
+- A failed process-group kill does not establish cancellation: the owned starter may not have called `setsid` yet. Fall back to its child pid at the grace deadline, and latch only a successful kill. Pause the starter before session creation to prove bounded cancellation independently of scheduler timing.
+- A session can contain multiple process groups. On macOS, check and settle the full owned session after the leader exits; killing only the original group leaves children that called `setpgrp` alive. Prove success, nonzero exit, and cancellation with an acknowledged child in a different group.
+- SIGKILL of `isolated-check.sh` or `isolate-run.sh` cannot be forwarded. macOS has no pid namespace, so a process that starts its own session inside `sandbox-exec` is not stopped with the boundary. This repository has no live OpenCode path, so there is no key-holding live boundary to supervise.
+
 ## 2026-11-09 — DSH ocp-dev host tsc needs Typert before package `./remote` imports
 
 - Stock `build:lib:host` / `host-libraries` runs `scripts/generate-typert.ts` before host compilation because package `./remote` exports resolve to generated `lib/typert.remote-client.*`. ocp-dev must not jump straight to `tsc -b tsconfig.host.json`.

test/isolate-session.test.ts

@@ -0,0 +1,85 @@
+import { describe, expect, it } from "bun:test"
+import { spawn } from "node:child_process"
+import fs from "node:fs"
+import os from "node:os"
+import path from "node:path"
+
+// Linux owns the pid namespace; macOS must settle every process group in
+// the session. The child acknowledges its new group before the leader exits.
+describe.skipIf(process.platform !== "darwin")("isolation session ownership", () => {
+  for (const mode of ["success", "failure", "cancel", "inspection-retry"] as const) {
+    it(`settles a same-session child in another process group on ${mode}`, async () => {
+      const dir = fs.mkdtempSync(path.join(os.tmpdir(), "isolate-session-"))
+      const childFile = path.join(dir, "child.pid")
+      const ready = path.join(dir, "ready")
+      const bin = path.join(dir, "bin")
+      const inspectionFailed = path.join(dir, "inspection-failed")
+      fs.mkdirSync(bin)
+      if (mode === "inspection-retry") {
+        fs.writeFileSync(path.join(bin, "ps"), [
+          "#!/bin/sh",
+          'if [ ! -e "$PROBE_INSPECTION_FAILED" ]; then : > "$PROBE_INSPECTION_FAILED"; exit 1; fi',
+          'exec /bin/ps "$@"',
+          "",
+        ].join("\n"), { mode: 0o755 })
+      }
+      fs.writeFileSync(path.join(dir, "child.py"), [
+        "import os, pathlib, signal, sys, time",
+        "root = pathlib.Path(sys.argv[1])",
+        "signal.signal(signal.SIGTERM, signal.SIG_IGN)",
+        "(root / 'child.pid').write_text(str(os.getpid()))",
+        "(root / 'ready').write_text('1')",
+        "while True: time.sleep(0.1)",
+        "",
+      ].join("\n"))
+      fs.writeFileSync(path.join(dir, "leader.py"), [
+        "import os, pathlib, subprocess, sys, time",
+        "root, mode = pathlib.Path(sys.argv[1]), sys.argv[2]",
+        "subprocess.Popen([sys.executable, str(root / 'child.py'), str(root)], preexec_fn=os.setpgrp, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)",
+        "while not (root / 'ready').exists(): time.sleep(0.01)",
+        "if mode == 'cancel': time.sleep(20)",
+        "raise SystemExit(7 if mode == 'failure' else 0)",
+        "",
+      ].join("\n"))
+      const owner = spawn("bash", ["-c", `
+        source scripts/isolate-run.sh
+        set +e
+        isolate_supervise "$1" python3 "$1/leader.py" "$1" "$2"
+      `, "probe", dir, mode], {
+        cwd: path.resolve(import.meta.dir, ".."),
+        env: { ...process.env, PATH: `${bin}:${process.env.PATH}`, PROBE_INSPECTION_FAILED: inspectionFailed, ISOLATE_TERM_GRACE: "0", PYTHONDONTWRITEBYTECODE: "1" },
+      })
+      owner.stdout.resume()
+      let stderr = ""
+      owner.stderr.on("data", chunk => { stderr += chunk })
+      const exited = new Promise<number | null>(resolve => owner.on("exit", resolve))
+      let timer: ReturnType<typeof setTimeout> | undefined
+      try {
+        const deadline = Date.now() + 5000
+        while (!fs.existsSync(ready)) {
+          if (owner.exitCode !== null || owner.signalCode !== null || Date.now() > deadline) throw new Error(`child did not start: ${stderr}`)
+          await Bun.sleep(10)
+        }
+        if (mode === "cancel") owner.kill("SIGTERM")
+        const status = await Promise.race([
+          exited,
+          new Promise<never>((_, reject) => { timer = setTimeout(() => reject(new Error(`owner did not settle: ${stderr}`)), 5000) }),
+        ])
+        expect(status, stderr).toBe(mode === "cancel" ? 143 : mode === "failure" ? 7 : 0)
+        const pid = Number(fs.readFileSync(childFile, "utf8"))
+        let alive = true
+        try { process.kill(pid, 0) } catch { alive = false }
+        expect(alive, stderr).toBe(false)
+        if (mode === "inspection-retry") expect(fs.existsSync(inspectionFailed)).toBe(true)
+      } finally {
+        clearTimeout(timer)
+        if (fs.existsSync(childFile)) {
+          try { process.kill(Number(fs.readFileSync(childFile, "utf8")), "SIGKILL") } catch { /* already gone */ }
+        }
+        if (owner.exitCode === null && owner.signalCode === null) owner.kill("SIGKILL")
+        await exited
+        fs.rmSync(dir, { recursive: true, force: true })
+      }
+    })
+  }
+})

test/review-eval.test.ts

@@ -154,12 +154,20 @@ if ignore:
     signal.signal(signal.SIGTERM, signal.SIG_IGN)
 (state / "descendant.pid").write_text(str(os.getpid()))
 (state / "workspace").write_text(os.getcwd())
+# Create the heartbeat once, before ready. Appending with O_CREAT can
+# recreate the file in the window between the runner's unlink and rmdir,
+# which makes rmdir fail with ENOTEMPTY and the probe exit 1 instead of the
+# cancelled status.
+open("heartbeat", "a").close()
 (state / "descendant.ready").write_text("1")
 deadline = time.monotonic() + 20
 while time.monotonic() < deadline and not (state / "stop").exists():
     try:
-        with open("heartbeat", "a") as handle:
-            handle.write("x")
+        handle = os.open("heartbeat", os.O_WRONLY | os.O_APPEND)
+        try:
+            os.write(handle, b"x")
+        finally:
+            os.close(handle)
     except OSError:
         pass
     time.sleep(0.05)