From 56e2560c08e4ac17ec4cf618bc9613bad7818fa3 Mon Sep 17 00:00:00 2001 From: Oleksiy Akimov <5861674+oakimov@users.noreply.github.com> Date: Tue, 10 Nov 2026 05:04:18 +0100 Subject: [PATCH 1/3] Stop the isolation boundary before cleanup on pid-targeted cancellation. A TERM, INT, HUP, or QUIT sent only to isolated-check.sh or isolate-run.sh ran the script's EXIT trap and removed the archive or stage while the sandboxed command kept running, reparented and with network access, and on the --live path with CURSOR_API_KEY. Neither script signalled its child, and the namespace init forwards only signals it receives itself. A process-group signal reached the boundary directly, which is why the existing termination test passed. isolate-run.sh now runs the boundary as its own session and waits for it, ported from oakimov/opencode-plugin-compat@c78395c. Cancellation signals are forwarded to that session (unshare ignores TERM while it waits; the namespace PID 1 shares its process group), and the session is killed after ISOLATE_TERM_GRACE seconds (default 10). Leftover session members are killed before staging files are removed. INT and QUIT, ignored in a non-interactive background job, are restored for the command. isolated-check.sh starts every boundary through check_supervise: the checks, and the live test that holds the key. It runs isolate-run.sh in the background, passes a cancellation on as TERM, waits for the boundary to stop, then exits with 128 plus the signal number so its EXIT trap removes the archive last. live-test.sh stopped its OpenCode process before removing its files on TERM, INT, and HUP, but QUIT left that process running: bash 3.2 dies without running the EXIT trap and bash 5 keeps waiting. QUIT is now trapped like INT and TERM. New tests signal only the script pid. On Linux (namespaces) and macOS (sandbox-exec) they fail on the previous scripts, leaving the command running, and pass with this change. SIGKILL still cannot be forwarded. --- CHANGELOG.md | 1 + docs/pr-review.md | 4 + scripts/isolate-run.sh | 105 ++++++++++++++++- scripts/isolated-check.sh | 57 ++++++++- scripts/live-test.sh | 9 +- tasks/lessons/opencode.md | 1 + test/review-scripts.test.ts | 227 +++++++++++++++++++++++++++++++++++- 7 files changed, 390 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd04d501..f81e9fe4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/pr-review.md b/docs/pr-review.md index e01772e8..a17b09db 100644 --- a/docs/pr-review.md +++ b/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//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 ` 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. diff --git a/scripts/isolate-run.sh b/scripts/isolate-run.sh index f1e087ca..0275bab1 100755 --- a/scripts/isolate-run.sh +++ b/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,89 @@ 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_sent="" + +# Record the first cancellation signal and pass it to the whole session. +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 +} + +# 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 grace ticks killed="" + shift + grace="${ISOLATE_TERM_GRACE:-10}" + [[ "$grace" =~ ^[0-9]+$ ]] || 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_sent="" + 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=$! + 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 + fi + 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 +399,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 +882,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 diff --git a/scripts/isolated-check.sh b/scripts/isolated-check.sh index 2aa6ddf9..69cd0fff 100755 --- a/scripts/isolated-check.sh +++ b/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 \ diff --git a/scripts/live-test.sh b/scripts/live-test.sh index 2d2ebdf5..3cc8bba8 100755 --- a/scripts/live-test.sh +++ b/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 diff --git a/tasks/lessons/opencode.md b/tasks/lessons/opencode.md index 5e145716..ed469e99 100644 --- a/tasks/lessons/opencode.md +++ b/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 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; diff --git a/test/review-scripts.test.ts b/test/review-scripts.test.ts index 566027c5..50047a61 100644 --- a/test/review-scripts.test.ts +++ b/test/review-scripts.test.ts @@ -1639,7 +1639,9 @@ describe("live test deadlines and cleanup", () => { } }) - it("removes plugin-list files when the live test is cancelled", async () => { + // The signal goes to the script's pid alone. QUIT once left OpenCode, which + // holds the key, running: bash does not run the EXIT trap for it. + for (const [signal, code] of [["SIGTERM", 143], ["SIGQUIT", 131]] as const) it(`removes plugin-list files and stops OpenCode when the live test receives ${signal}`, async () => { const directory = fs.mkdtempSync(path.join(os.tmpdir(), "live-cancel-")) const bin = path.join(directory, "bin") const reviewHome = path.join(directory, "review-home") @@ -1675,16 +1677,17 @@ describe("live test deadlines and cleanup", () => { if (Date.now() - started > 8000) throw new Error("plugin list did not start") await pause() } - child.kill("SIGTERM") + child.kill(signal) const killed = Date.now() while (child.exitCode === null && child.signalCode === null) { - if (Date.now() - killed > 8000) throw new Error("live test did not exit after SIGTERM") + if (Date.now() - killed > 8000) throw new Error(`live test did not exit after ${signal}`) await pause() } - const temps = fs.readdirSync(path.join(reviewHome, "tmp")) - expect(temps).toEqual([]) const sleeperAlive = spawnSync("kill", ["-0", fs.readFileSync(pidFile, "utf8").trim()]) expect(sleeperAlive.status).not.toBe(0) + expect(child.exitCode).toBe(code) + const temps = fs.readdirSync(path.join(reviewHome, "tmp")) + expect(temps).toEqual([]) } finally { if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL") if (fs.existsSync(pidFile)) spawnSync("kill", ["-KILL", fs.readFileSync(pidFile, "utf8").trim()]) @@ -1875,3 +1878,217 @@ wait } }) }) + +// Cancellation that reaches only the script's own pid, as a tool runner's +// kill(pid) or subprocess.terminate() does. A process-group signal also +// reaches the boundary directly and does not show whether the script passes +// the cancellation on before its cleanup runs. +describe("pid-targeted cancellation", () => { + const pause = () => new Promise((resolve) => setTimeout(resolve, 50)) + + // Visible to ps on the host for both the Linux namespace and macOS sandbox. + function marked(marker: string): number[] { + const listing = spawnSync("ps", ["-A", "-o", "pid=,args="], { encoding: "utf8" }) + if (listing.status !== 0) throw new Error(`ps failed: ${listing.stderr}`) + return listing.stdout.split("\n").filter((line) => line.includes(marker)).map((line) => Number.parseInt(line, 10)) + } + + function commandAlive(marker: string): boolean { + return marked(marker).length > 0 + } + + // A failed assertion must not leave the command it found running. + function killMarked(marker: string): void { + for (const pid of marked(marker)) { + try { process.kill(pid, "SIGKILL") } catch { /* already gone */ } + } + } + + type Exit = { code: number | null; signal: NodeJS.Signals | null; stdout: string; stderr: string; at: number } + + async function cancelAfterReady( + args: string[], + readyFile: string, + signal: NodeJS.Signals, + env: NodeJS.ProcessEnv, + ): Promise { + const child = spawn("bash", args, { cwd: root, env: { ...env, PYTHONDONTWRITEBYTECODE: "1" } }) + let stdout = "" + let stderr = "" + child.stdout.on("data", (chunk) => (stdout += chunk)) + child.stderr.on("data", (chunk) => (stderr += chunk)) + const exited = new Promise((resolve) => + child.on("exit", (code, exitSignal) => resolve({ code, signal: exitSignal, stdout, stderr, at: Date.now() })), + ) + const started = Date.now() + try { + while (!fs.existsSync(readyFile)) { + if (child.exitCode !== null || child.signalCode !== null) { + const early = await exited + return { ...early, unavailable: /ISOLATION=unavailable/.test(early.stderr), signalledAt: early.at } + } + if (Date.now() - started > 15000) throw new Error(`command did not start\n${stdout}\n${stderr}`) + await pause() + } + const signalledAt = Date.now() + child.kill(signal) + const timeout = new Promise((_, reject) => + setTimeout(() => reject(new Error(`no exit after ${signal}\n${stdout}\n${stderr}`)), 20000), + ) + const result = await Promise.race([exited, timeout]) + return { ...result, unavailable: false, signalledAt } + } finally { + if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL") + } + } + + // The temp root is left as the host reports it; on macOS that is a symlink. + function fixture(prefix: string) { + const base = fs.mkdtempSync(path.join(os.tmpdir(), prefix)) + const src = path.join(base, "src") + const tmp = path.join(base, "tmp") + fs.mkdirSync(src) + fs.mkdirSync(tmp) + const marker = `${prefix}${process.pid}-${Math.random().toString(36).slice(2)}` + return { base, src, tmp, marker } + } + + it("isolate-run.sh kills a command that ignores TERM before removing its stage", async () => { + const { base, src, tmp, marker } = fixture("isolate-ignore-term-") + try { + const result = await cancelAfterReady( + [ + "scripts/isolate-run.sh", "--src", src, "--", + "/bin/sh", "-c", 'trap "" TERM; echo ready > ready; while :; do sleep 0.1; done', marker, + ], + path.join(src, "ready"), + "SIGTERM", + { ...process.env, TMPDIR: tmp, ISOLATE_TERM_GRACE: "1" }, + ) + if (result.unavailable) return + // The command outlived TERM, so this exit must follow the grace-period kill. + expect(commandAlive(marker), `${result.stdout}\n${result.stderr}`).toBe(false) + expect(result.code, result.stderr).toBe(143) + expect(result.at - result.signalledAt).toBeGreaterThanOrEqual(900) + expect(fs.readdirSync(tmp)).toEqual([]) + } finally { + killMarked(marker) + fs.rmSync(base, { recursive: true, force: true }) + } + }) + + it("isolate-run.sh forwards TERM so the command can stop itself", async () => { + const { base, src, tmp, marker } = fixture("isolate-forward-term-") + try { + const result = await cancelAfterReady( + [ + "scripts/isolate-run.sh", "--src", src, "--", + "/bin/sh", "-c", 'trap "echo stopped > stopped; exit 0" TERM; echo ready > ready; while :; do sleep 0.1; done', marker, + ], + path.join(src, "ready"), + "SIGTERM", + { ...process.env, TMPDIR: tmp, ISOLATE_TERM_GRACE: "30" }, + ) + if (result.unavailable) return + expect(commandAlive(marker)).toBe(false) + expect(result.code, result.stderr).toBe(143) + expect(fs.readFileSync(path.join(src, "stopped"), "utf8")).toBe("stopped\n") + // Far below the grace period: the command stopped on the forwarded TERM. + expect(result.at - result.signalledAt).toBeLessThan(10000) + expect(fs.readdirSync(tmp)).toEqual([]) + } finally { + killMarked(marker) + fs.rmSync(base, { recursive: true, force: true }) + } + }) + + for (const [signal, code] of [["SIGTERM", 143], ["SIGINT", 130]] as const) { + it(`isolated-check.sh waits for the boundary before removing the archive on ${signal}`, async () => { + const { base, src, tmp, marker } = fixture(`isolated-check-${signal.toLowerCase()}-`) + try { + const result = await cancelAfterReady( + [ + "-c", + ` + set -euo pipefail + source scripts/isolated-check.sh + worktree="$WT" + trap check_cleanup EXIT + cd "$WT" + check_step UNIT_TESTS /bin/sh -c 'trap "" TERM INT; echo ready > ready; while :; do sleep 0.1; done' "$MARKER" || true + echo after-step + `, + ], + path.join(src, "ready"), + signal, + { ...process.env, TMPDIR: tmp, ISOLATE_TERM_GRACE: "1", WT: src, MARKER: marker }, + ) + if (result.unavailable) return + expect(commandAlive(marker), `${result.stdout}\n${result.stderr}`).toBe(false) + expect(result.code, result.stderr).toBe(code) + expect(result.stderr).toContain(`Check cancelled by ${signal}.`) + // The cancelled check neither reports the step nor runs the next one. + expect(result.stdout).not.toContain("UNIT_TESTS=") + expect(result.stdout).not.toContain("after-step") + expect(fs.existsSync(src)).toBe(false) + expect(fs.readdirSync(tmp)).toEqual([]) + } finally { + killMarked(marker) + fs.rmSync(base, { recursive: true, force: true }) + } + }) + } + + // check_run_live is the call that passes CURSOR_API_KEY into the boundary. + // The harness here is a stand-in checkout: the real isolate-run.sh, and a + // live-test.sh that reports whether it was given a key and ignores TERM. + it("isolated-check.sh waits for the live boundary, which holds the key, before removing the archive", async () => { + const { base, src, tmp, marker } = fixture("isolated-check-live-") + const harness = path.join(base, "harness") + fs.mkdirSync(path.join(harness, "scripts"), { recursive: true }) + fs.copyFileSync(path.join(root, "scripts/isolate-run.sh"), path.join(harness, "scripts/isolate-run.sh")) + fs.chmodSync(path.join(harness, "scripts/isolate-run.sh"), 0o755) + fs.writeFileSync( + path.join(harness, "scripts/live-test.sh"), + [ + 'echo "LIVE_STUB key=${CURSOR_API_KEY:+present}"', + `exec /bin/sh -c 'trap "" TERM INT; echo ready > ready; while :; do sleep 0.1; done' ${marker}`, + "", + ].join("\n"), + ) + try { + const result = await cancelAfterReady( + [ + "-c", + ` + set -euo pipefail + source scripts/isolated-check.sh + ROOT="$HARNESS" + worktree="$WT" + trap check_cleanup EXIT + cd "$WT" + set +e + check_run_live "$WT" + echo "live_exit=$?" + `, + ], + path.join(src, "ready"), + "SIGTERM", + { ...process.env, TMPDIR: tmp, ISOLATE_TERM_GRACE: "1", WT: src, HARNESS: harness, CURSOR_API_KEY: secret }, + ) + if (result.unavailable) return + expect(commandAlive(marker), `${result.stdout}\n${result.stderr}`).toBe(false) + expect(result.code, result.stderr).toBe(143) + expect(result.stderr).toContain("Check cancelled by SIGTERM.") + expect(result.stdout).toContain("LIVE_STUB key=present") + expect(result.stdout).not.toContain(secret) + // The cancelled live run does not return to the caller's summary. + expect(result.stdout).not.toContain("live_exit=") + expect(fs.existsSync(src)).toBe(false) + expect(fs.readdirSync(tmp)).toEqual([]) + } finally { + killMarked(marker) + fs.rmSync(base, { recursive: true, force: true }) + } + }) +}) From 3289c40570b85e0ce896701424fc9633f9ebc758 Mon Sep 17 00:00:00 2001 From: Oleksiy Akimov <5861674+oakimov@users.noreply.github.com> Date: Tue, 10 Nov 2026 10:28:57 +0100 Subject: [PATCH 2/3] Stop the boundary when a cancellation is trapped before the wait. A trap that bash takes between two commands does not end the `wait` that follows it. TERM, INT, HUP, or QUIT trapped before isolate_supervise had stored the leader's pid, or just before its `wait` started, was recorded, but the stop sequence ran only after `wait` returned. isolate-run.sh then waited for the command to end by itself, forever for a command that ignores TERM. Nothing was removed under the running command, but the cancellation never finished. The trap now runs the stop sequence itself (isolate_sup_stop): send the signal to the session once it exists, kill the session after ISOLATE_TERM_GRACE, and return when the leader has gone. isolate_supervise calls it once more for a signal trapped before the leader was known. Two tests take the trap at each boundary through a BASH_ENV hook whose DEBUG trap signals the script just before the named command. Both fail on the previous script, which never exits, and pass with this change. --- scripts/isolate-run.sh | 68 +++++++++++++++++++++++-------------- tasks/lessons/opencode.md | 2 +- test/review-scripts.test.ts | 64 ++++++++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 27 deletions(-) diff --git a/scripts/isolate-run.sh b/scripts/isolate-run.sh index 0275bab1..c54edf25 100755 --- a/scripts/isolate-run.sh +++ b/scripts/isolate-run.sh @@ -88,14 +88,40 @@ isolate_unavailable() { # scope, so these are globals. isolate_sup_leader="" isolate_sup_signal="" -isolate_sup_sent="" +isolate_sup_stopping="" +isolate_sup_grace=10 -# Record the first cancellation signal and pass it to the whole session. +# Record the first cancellation signal and stop the session. 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 +} + +# 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. @@ -113,13 +139,13 @@ 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 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 @@ -140,24 +166,14 @@ 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 + # 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 diff --git a/tasks/lessons/opencode.md b/tasks/lessons/opencode.md index ed469e99..8a91992d 100644 --- a/tasks/lessons/opencode.md +++ b/tasks/lessons/opencode.md @@ -790,7 +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 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; diff --git a/test/review-scripts.test.ts b/test/review-scripts.test.ts index 50047a61..992cd3aa 100644 --- a/test/review-scripts.test.ts +++ b/test/review-scripts.test.ts @@ -2002,6 +2002,70 @@ describe("pid-targeted cancellation", () => { } }) + // bash takes a trap between two commands, and a `wait` that starts after + // the trap does not end early for it. The hook, read through BASH_ENV, + // sends TERM to the script just before the named command, so the trap is + // taken at exactly that boundary rather than while the script waits. + for (const [boundary, when] of [ + ["isolate_sup_leader=$!", "before the session leader is known"], + ['wait "$isolate_sup_leader"', "just before the wait starts"], + ] as const) { + it(`isolate-run.sh stops a command when TERM is trapped ${when}`, async () => { + const { base, src, tmp, marker } = fixture("isolate-trap-boundary-") + const hook = path.join(base, "hook.sh") + const sent = path.join(base, "sent") + fs.writeFileSync( + hook, + [ + "unset BASH_ENV", + "set -o functrace", + `trap 'if [[ ! -e "$BOUNDARY_SENT" && "$BASH_COMMAND" == "$BOUNDARY_COMMAND" ]]; then echo sent > "$BOUNDARY_SENT"; kill -s TERM $$; fi' DEBUG`, + "", + ].join("\n"), + ) + const child = spawn( + "bash", + ["scripts/isolate-run.sh", "--src", src, "--", "/bin/sh", "-c", 'trap "" TERM; while :; do sleep 0.1; done', marker], + { + cwd: root, + env: { + ...process.env, + TMPDIR: tmp, + ISOLATE_TERM_GRACE: "1", + PYTHONDONTWRITEBYTECODE: "1", + BASH_ENV: hook, + BOUNDARY_COMMAND: boundary, + BOUNDARY_SENT: sent, + }, + }, + ) + let stderr = "" + child.stdout.resume() + child.stderr.on("data", (chunk) => (stderr += chunk)) + try { + const code = await new Promise((resolve, reject) => { + const timer = setTimeout( + () => reject(new Error(`no exit; TERM sent: ${fs.existsSync(sent)}\n${stderr}`)), + 15000, + ) + child.on("exit", (exitCode) => { + clearTimeout(timer) + resolve(exitCode) + }) + }) + if (/ISOLATION=unavailable/.test(stderr)) return + expect(fs.existsSync(sent), stderr).toBe(true) + expect(commandAlive(marker), stderr).toBe(false) + expect(code, stderr).toBe(143) + expect(fs.readdirSync(tmp)).toEqual([]) + } finally { + if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL") + killMarked(marker) + fs.rmSync(base, { recursive: true, force: true }) + } + }) + } + for (const [signal, code] of [["SIGTERM", 143], ["SIGINT", 130]] as const) { it(`isolated-check.sh waits for the boundary before removing the archive on ${signal}`, async () => { const { base, src, tmp, marker } = fixture(`isolated-check-${signal.toLowerCase()}-`) From c03f2acae8d953e56f09f3879ba905da6708c616 Mon Sep 17 00:00:00 2001 From: Oleksiy Akimov <5861674+oakimov@users.noreply.github.com> Date: Tue, 10 Nov 2026 10:28:57 +0100 Subject: [PATCH 3/3] Stop the review fixture's writer from bringing its file back. The ownership fixture in test/review-eval.test.ts starts a descendant that appends to `heartbeat` in the runner's workspace every 50 ms, opening the file with create. The defective runner variants remove that workspace while the descendant still runs. A write that landed between the runner's unlink and rmdir recreated the file, the removal failed with ENOTEMPTY, and the runner exited 1 with the workspace in place instead of the status the test expects. It failed one full macOS run in six. The descendant now creates the file once, before it reports ready, and appends without creating. With a writer that does not pause, the `term` runner failed 15 of 15 runs before and none of 15 after. --- tasks/lessons/opencode.md | 5 +++++ test/review-eval.test.ts | 10 ++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/tasks/lessons/opencode.md b/tasks/lessons/opencode.md index 8a91992d..1f05460b 100644 --- a/tasks/lessons/opencode.md +++ b/tasks/lessons/opencode.md @@ -806,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, diff --git a/test/review-eval.test.ts b/test/review-eval.test.ts index 4fe2f560..b27e438a 100644 --- a/test/review-eval.test.ts +++ b/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)