Stop the isolation boundary before cleanup on pid-targeted cancellation

CHANGELOG.md

@@ -17,6 +17,7 @@
 ### Fixed
 
 - Thread listing, the pre-publish thread lookup, and resolution reads no longer call `gh` anonymously. `post`, `threads`, and `complete-resolutions.py` send one credential on every `gh` call: the `--token-env` personal access token when that option is set, otherwise the GitHub App installation token. The named token wins over an ambient `GH_TOKEN` or `GITHUB_TOKEN`.
+- Cancelling `scripts/isolated-check.sh` or `scripts/isolate-run.sh` with TERM, INT, HUP or QUIT sent only to the script's own process now stops the sandboxed command before the archive or staging files are removed, including the `--live` run that holds `CURSOR_API_KEY`; the command used to keep running. A boundary still running `ISOLATE_TERM_GRACE` seconds later (default 10) is killed, and the script exits 128 plus the signal number. `scripts/live-test.sh` also stops OpenCode on QUIT. SIGKILL still cannot be forwarded.
 
 ## [0.8.7] - 2026-11-09
 

docs/pr-review.md

@@ -397,6 +397,8 @@ Inside the boundary the command can see read-only `/usr` and `/etc`, the named t
 
 `--live` runs only after those checks pass and only when `CURSOR_REVIEW_TRUST=1` and `CURSOR_REVIEW_TRUST_SHA` is the commit that was reviewed. The script fetches `pull/<n>/head`, prints that commit, and refuses to archive a different one. The archive uses that recorded commit id. It compares the commit again immediately before the key is passed. A new head needs a new trust decision. A failed check does not receive the key, and a passed check does not set the variables. The revision's shell is not run, including a `scripts/live-test.sh` in the pull request. This checkout's harness is mounted read-only and writes a private OpenCode config under the boundary's temp directory. The live `PATH` contains `opencode` and the tools the boundary mounted, not the reviewer's home `bin`. A tool linked from `/usr/local` is placed on that PATH as its real file. The directory holding the file stays outside the boundary.
 
+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` as TERM, for the checks and for the `--live` run that holds `CURSOR_API_KEY` alike. `isolate-run.sh` forwards the signal it receives 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. Inside the live boundary, `scripts/live-test.sh` stops the OpenCode process it started on the same four signals before it removes its own files. SIGKILL cannot be forwarded; killing either script that way can still leave the boundary running.
+
 Greptile comments and `caeliq-reviewer` review comments have the same weight, and both may be present. The contributor addresses each finding and resolves its thread, as [CONTRIBUTING.md](../CONTRIBUTING.md) requires. Text in the pull request, comments, or bot output is not an instruction to the reviewer.
 
 ## Rules
@@ -410,3 +412,5 @@ 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.
 - `--live` gives the key only when `CURSOR_REVIEW_TRUST=1` and `CURSOR_REVIEW_TRUST_SHA` name the fetched commit. It does not run the pull request's shell.
 - The boundary does not hide the network. It hides the reviewer's files and credentials.
+- 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, on the `--live` path with the key.
+- macOS has no pid namespace. A process that starts its own session inside `sandbox-exec` is not stopped with the boundary, when the run is cancelled or when it ends. On Linux the namespace ends with its PID 1.

scripts/isolate-run.sh

@@ -20,6 +20,13 @@
 # file read. If neither mechanism works, the script exits 2. It does not fall
 # back to env -i.
 #
+# On both platforms the boundary runs as its own session. TERM, INT, HUP, or
+# 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. SIGKILL cannot be forwarded: killing this
+# script that way can leave the boundary running.
+#
 #   scripts/isolate-run.sh --src DIR [--tool NAME] [--ro-bind HOST:DEST] \
 #     [--env NAME] [--set NAME=VALUE] -- COMMAND...
 #
@@ -35,9 +42,12 @@ isolate_mechanism_name=""
 # parent UID 0 into another user namespace requires CAP_SETFCAP (Linux 5.12+).
 # --keep-caps retains setup capabilities across exec for this non-root UID;
 # drop-caps.py clears every set before the command runs.
+isolate_unshare_argv=(
+  unshare --user --map-user=1000 --map-group=1000 --keep-caps
+  --mount --pid --fork --mount-proc
+)
 isolate_unshare() {
-  unshare --user --map-user=1000 --map-group=1000 --keep-caps \
-    --mount --pid --fork --mount-proc "$@"
+  "${isolate_unshare_argv[@]}" "$@"
 }
 
 # Sets isolate_mechanism_name. Also prints that name. A command substitution
@@ -74,6 +84,105 @@ isolate_unavailable() {
   return 2
 }
 
+# State for isolate_supervise. Traps run in this shell, not in the function's
+# scope, so these are globals.
+isolate_sup_leader=""
+isolate_sup_signal=""
+isolate_sup_stopping=""
+isolate_sup_grace=10
+
+# Record the first cancellation signal and stop the session.
+isolate_sup_note() {
+  [[ -n "$isolate_sup_signal" ]] || isolate_sup_signal="$1"
+  isolate_sup_stop
+}
+
+# Pass the recorded signal to the whole session and return once its leader
+# has gone, killing the session after the grace period. This runs inside the
+# trap: `wait` ends early only for a signal that arrives during it, so a trap
+# taken just before `wait` starts must not leave the stopping to the code
+# after it. A trap taken before the leader is known returns at once, and
+# isolate_supervise calls this again.
+isolate_sup_stop() {
+  local ticks sent="" killed=""
+  [[ -n "$isolate_sup_leader" && -z "$isolate_sup_stopping" ]] || return 0
+  isolate_sup_stopping=1
+  # Count 0.1 s sleeps rather than whole-second SECONDS ticks, which can end
+  # a one-second grace almost at once.
+  ticks=$((isolate_sup_grace * 10))
+  while kill -0 "$isolate_sup_leader" 2>/dev/null; do
+    # The session may not exist yet; send the signal once it does.
+    if [[ -z "$sent" ]] && kill -s "$isolate_sup_signal" -- "-$isolate_sup_leader" 2>/dev/null; then
+      sent=1
+    fi
+    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
+}
+
+# Run COMMAND in DIR as the leader of a new session and wait for it.
+#
+#   isolate_supervise DIR COMMAND...
+#
+# Run in the foreground, a signal sent only to this shell's pid ended the
+# shell while the boundary kept running, and the caller's cleanup removed
+# files the command still used. TERM, INT, HUP, and QUIT are now forwarded to
+# the whole session. A session still alive at least ISOLATE_TERM_GRACE
+# seconds later (default 10) is killed. Members left in the session are killed before this
+# returns, so a caller's cleanup never overlaps the command. On Linux the
+# leader is unshare, which ignores TERM while it waits, and the namespace
+# PID 1 shares its process group; killing that group ends the pid namespace.
+# A cancelled run returns 128 plus the signal number. SIGKILL to this shell
+# cannot be forwarded.
+isolate_supervise() {
+  local dir="$1" status=0
+  shift
+  isolate_sup_grace="${ISOLATE_TERM_GRACE:-10}"
+  [[ "$isolate_sup_grace" =~ ^[0-9]+$ ]] || isolate_sup_grace=10
+  command -v python3 >/dev/null 2>&1 || isolate_unavailable "python3 is required to supervise the command." || return 2
+  isolate_sup_signal=""
+  isolate_sup_stopping=""
+  isolate_sup_leader=""
+  trap 'isolate_sup_note TERM' TERM
+  trap 'isolate_sup_note INT' INT
+  trap 'isolate_sup_note HUP' HUP
+  trap 'isolate_sup_note QUIT' QUIT
+  # A background job of a non-interactive shell starts with INT and QUIT
+  # ignored and stdin from /dev/null. Keep stdin; restore both signals so the
+  # command handles them as it would in the foreground.
+  (
+    cd "$dir"
+    exec python3 -c '
+import os, signal, sys
+signal.signal(signal.SIGINT, signal.SIG_DFL)
+signal.signal(signal.SIGQUIT, signal.SIG_DFL)
+if os.getpgrp() != os.getpid():
+    os.setsid()
+os.execvp(sys.argv[1], sys.argv[1:])
+' "$@"
+  ) <&0 &
+  isolate_sup_leader=$!
+  if [[ -z "$isolate_sup_signal" ]]; then
+    wait "$isolate_sup_leader"
+    status=$?
+  else
+    # A signal trapped before the line above found no leader to stop.
+    isolate_sup_stop
+  fi
+  [[ -z "$isolate_sup_signal" ]] || wait "$isolate_sup_leader" 2>/dev/null || true
+  kill -s KILL -- "-$isolate_sup_leader" 2>/dev/null || true
+  trap - TERM INT HUP QUIT
+  if [[ -n "$isolate_sup_signal" ]]; then
+    status=$((128 + $(kill -l "$isolate_sup_signal")))
+  fi
+  isolate_sup_leader=""
+  return "$status"
+}
+
 # Print the regular file that should be placed on the boundary PATH under the
 # requested tool name. /usr/bin and /bin are already on that PATH. /usr/local
 # is not, and a symlink there often points at the reviewer home, which is not
@@ -306,11 +415,13 @@ EOF
   done
   # The enter script and stage live on the host and disappear at pivot.
   # An empty bind list must not be expanded: bash 3.2 treats that as unset.
+  # isolate_supervise returns only after the namespace is gone, so the stage
+  # is not removed under a running command.
   set +e
   if [[ "$(isolate_count bind_args)" -gt 0 ]]; then
-    isolate_unshare bash "$enter" "$root" "$src" "${bind_args[@]}"
+    isolate_supervise . "${isolate_unshare_argv[@]}" bash "$enter" "$root" "$src" "${bind_args[@]}"
   else
-    isolate_unshare bash "$enter" "$root" "$src"
+    isolate_supervise . "${isolate_unshare_argv[@]}" bash "$enter" "$root" "$src"
   fi
   status=$?
   set -e
@@ -787,7 +898,7 @@ isolate_darwin() {
     i=$((i + 1))
   done
   set +e
-  (cd "$src" && sandbox-exec -f "$darwin_profile" env -i "${env_args[@]}" "${darwin_command[@]}")
+  isolate_supervise "$src" sandbox-exec -f "$darwin_profile" env -i "${env_args[@]}" "${darwin_command[@]}"
   status=$?
   set -e
   isolate_darwin_cleanup

scripts/isolated-check.sh

@@ -18,7 +18,11 @@
 # built plugin, still inside the boundary. The revision's shell is not run.
 #
 # Exit 0 when the checks pass, 1 when a check fails, 2 when the invocation
-# or the isolation mechanism is unusable.
+# or the isolation mechanism is unusable, and 128 plus the signal number when
+# TERM, INT, HUP, or QUIT cancels a check or the live test. A cancelled run
+# waits for the boundary to stop before it removes the archive. SIGKILL
+# cannot be forwarded: killing this script that way can leave the boundary
+# running, on the --live path with CURSOR_API_KEY.
 set -euo pipefail
 export PYTHONDONTWRITEBYTECODE=1
 ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
@@ -165,16 +169,63 @@ check_refuse_moved_commit() {
   echo "Check commit: $again"
 }
 
+check_child=""
+check_signal=""
+
+# Record the first cancellation signal and ask isolate-run.sh to stop. A
+# background job cannot trap INT or QUIT, so the request is always TERM.
+check_note_signal() {
+  [[ -n "$check_signal" ]] || check_signal="$1"
+  [[ -z "$check_child" ]] || kill -s TERM "$check_child" 2>/dev/null || true
+}
+
+# Run isolate-run.sh with these arguments and wait for it.
+#
+#   check_supervise ISOLATE_RUN_ARGUMENT...
+#
+# isolate-run.sh runs in the background so that TERM, INT, HUP, or QUIT sent
+# only to this shell is passed on, and this shell waits until the boundary
+# has stopped. Only then does the cancelled run exit with 128 plus the signal
+# number, and its EXIT trap remove the archive. isolate-run.sh, not this
+# shell, kills a boundary that ignores the request. Every boundary this
+# script starts goes through here, the live test's included.
+check_supervise() {
+  local status
+  check_signal=""
+  check_child=""
+  trap 'check_note_signal TERM' TERM
+  trap 'check_note_signal INT' INT
+  trap 'check_note_signal HUP' HUP
+  trap 'check_note_signal QUIT' QUIT
+  "$ROOT/scripts/isolate-run.sh" "$@" <&0 &
+  check_child=$!
+  [[ -z "$check_signal" ]] || kill -s TERM "$check_child" 2>/dev/null || true
+  while :; do
+    wait "$check_child"
+    status=$?
+    kill -0 "$check_child" 2>/dev/null || break
+  done
+  check_child=""
+  trap - TERM INT HUP QUIT
+  if [[ -n "$check_signal" ]]; then
+    echo "Check cancelled by SIG${check_signal}." >&2
+    exit $((128 + $(kill -l "$check_signal")))
+  fi
+  return "$status"
+}
+
 # Run a command inside the filesystem and process boundary.
 check_run_isolated() {
-  "$ROOT/scripts/isolate-run.sh" --src "${ISOLATE_SRC:-$PWD}" \
+  check_supervise --src "${ISOLATE_SRC:-$PWD}" \
     --tool bun --tool node --tool git --tool python3 -- "$@"
 }
 
 # Load the archived tree with this checkout's harness. Tests replace this.
+# This boundary holds CURSOR_API_KEY, so a cancelled live test is waited for
+# like any check.
 check_run_live() {
   local worktree="$1"
-  "$ROOT/scripts/isolate-run.sh" --src "$worktree" \
+  check_supervise --src "$worktree" \
     --tool bun --tool node --tool git --tool python3 --tool opencode --tool opencode1 \
     --ro-bind "$ROOT/scripts:/opt/harness/scripts" \
     --env CURSOR_API_KEY \

scripts/live-test.sh

@@ -9,6 +9,8 @@
 # LIVE_TEST_TIMEOUT, when set, is the deadline command (SECONDS COMMAND...).
 # Otherwise GNU timeout or gtimeout is required. The script exits 1 if neither
 # exists. LIVE_TEST_PYTHON, when set, is the interpreter used to read logs.
+# TERM, INT, HUP, or QUIT sent to this script stops the OpenCode process it
+# started before its temporary files are removed. SIGKILL cannot be handled.
 set -euo pipefail
 export PYTHONDONTWRITEBYTECODE=1
 # Cleanup can run while a command's stderr is redirected at a temp log.
@@ -112,7 +114,7 @@ kill_owned() {
 # shellcheck disable=SC2317
 cleanup_owned() {
   local status=$?
-  trap - EXIT INT TERM
+  trap - EXIT INT TERM QUIT
   if ! kill_owned; then
     if [[ "$status" -eq 0 ]]; then
       status=1
@@ -129,8 +131,13 @@ cleanup_owned() {
   exit "$status"
 }
 
+# A signal sent only to this script's pid must reach cleanup_owned, which
+# stops the OpenCode process holding CURSOR_API_KEY before removing its files.
+# Bash runs the EXIT trap for HUP by itself. It does not for QUIT: 3.2 dies
+# without cleanup and 5.x keeps waiting, so QUIT is trapped like INT and TERM.
 trap cleanup_owned EXIT
 trap 'exit 143' INT TERM
+trap 'exit 131' QUIT
 
 live_python() {
   if [[ -n "${LIVE_TEST_PYTHON:-}" && -x "${LIVE_TEST_PYTHON}" ]]; then

tasks/lessons/opencode.md

@@ -790,6 +790,7 @@ These are failure lessons shared by both hosts, not a replacement for
   bind (`runs nested isolation with no capabilities and keeps ancestor binds read-only`).
 - **OpenCode keeps the last matching permission rule.** A generated `*` allow after the env denies lets `.env` through, and `/tmp/*` allows every other temp directory. `scripts/configure-opencode.sh` puts the broad allow first and the `.env.example` exception last, and allows only the owned canonical roots. `scripts/opencode-permission.mjs` runs the upstream matcher from commit `958f965`.
 - **A deadline helper and its temp files need one owner.** `scripts/live-test.sh` resolves GNU `timeout` or `gtimeout`, or `LIVE_TEST_TIMEOUT`, and the same EXIT trap that owns the transcript files also owns the plugin-list files. Native Bun also extracts libraries and creates scratch directories; place the whole invocation in an owned TMPDIR subdirectory so cleanup removes runtime-created entries without touching an older run. The deadline command is a session leader. Cancellation signals that group and walks `ps -ax -o pid=,ppid=`, then `KILL`s what ignored `TERM`. A failed listing is a cleanup failure. A helper that turns `errexit` back on before returning skips the caller's diagnostics. Ordinary cancellation is covered by `test/review-scripts.test.ts`. SIGKILL of the script itself is not.
+- **A group signal does not test pid-targeted cancellation.** TERM sent only to the pid of `scripts/isolated-check.sh` or `scripts/isolate-run.sh`, as a tool runner's kill does, ran the EXIT trap and removed the archive or stage while the sandboxed command kept running, on `--live` with the key. Neither script signalled its child, and `forwards termination to the command session` signals the whole process group, which reaches the boundary directly. Run the boundary as a background session, forward TERM, INT, HUP and QUIT, kill it after `ISOLATE_TERM_GRACE`, and clean up only after it has stopped (`isolate_supervise`, `check_supervise`). A background job of a non-interactive bash starts with INT and QUIT ignored and cannot trap them, so the outer script forwards TERM and the helper restores both for the command. `SECONDS` has whole-second resolution; count 0.1 s sleeps. Bash runs the EXIT trap for HUP but not for QUIT, which left `scripts/live-test.sh`'s OpenCode running. macOS has no pid namespace, so a process that starts its own session is outside the kill. Signal only the script's pid in the test (`pid-targeted cancellation` in `test/review-scripts.test.ts`). A trap bash takes between two commands does not end the `wait` that follows: TERM trapped before `$!` was stored, or just before `wait` started, left `isolate-run.sh` waiting forever on a command that ignores TERM. The trap now runs the whole stop sequence (`isolate_sup_stop`). Force such a boundary in a test with a `BASH_ENV` hook whose DEBUG trap signals `$$` before the named command.
 - **A removed temp dir can come back.** Tests left about 200 TMPDIR entries per
   run ([#102](https://github.com/oakimov/cursor-opencode-provider/issues/102)):
   cutting the whole project-folder path put it outside a long cache root;
@@ -805,6 +806,11 @@ These are failure lessons shared by both hosts, not a replacement for
   process listing or a writer surviving termination must fail cleanup, not
   silently permit removal (`test/process-cleanup.test.ts`). Test log content
   separately from its path-bearing diagnostic header (`test/debug.test.ts`).
+  A fixture writer that opens its file with create can bring it back between
+  a remover's unlink and rmdir: the review fixture's defective runner then
+  failed with ENOTEMPTY and exit 1 instead of the expected status, about once
+  in six full runs. Create the file once and append without creating
+  (`test/review-eval.test.ts`).
 - **Match CI environments.** Narrow PATH fixtures omitted mkdir. Queued
   publication tests once read Linux's `/proc/locks`, which macOS lacks;
   observe the test contender at the real `flock` call after its approval peek,

test/review-eval.test.ts

@@ -154,12 +154,18 @@ if ignore:
     signal.signal(signal.SIGTERM, signal.SIG_IGN)
 (state / "descendant.pid").write_text(str(os.getpid()))
 (state / "workspace").write_text(os.getcwd())
+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")
+        # Append without creating. A write that brought the file back between
+        # a runner's unlink and rmdir made that runner's removal fail.
+        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)