diff --git a/.github/groom/README.md b/.github/groom/README.md index 3a77e11d..d1ebe61b 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -685,8 +685,9 @@ just untrusted data the agent analyzes, never a path to the runner's credentials How `groom.yml` composes them per agent job: a **broker step** (the only step holding `secrets.ANTHROPIC_API_KEY`) starts `broker.mjs` on the host socket -`$BROKER_SOCK` and waits for its `/healthz`; the **agent step** — carrying NO real -key — runs `agent-sandbox.sh --uds "$BROKER_SOCK"` with the brief (and, for the +`$BROKER_SOCK` and waits for its `/healthz`; a **"Preflight the sandbox" step** +does the whole no-spend setup half (`--preflight-only` then `--validate-only`, see +below); the **agent step** — carrying NO real key — runs `agent-sandbox.sh --uds "$BROKER_SOCK"` with the brief (and, for the builder, the finding JSON) passed `--ro-file`, every output under the one rw `--out-dir` (`$GROOM_OUT_DIR`), and a `bash -c` wrapper that brings up the in-jail `jail-shim.mjs` before `exec`ing the pinned `claude` CLI with a DUMMY key and @@ -704,8 +705,15 @@ read-only. agent-sandbox.sh --clone --clone-mode ro|rw-git-ro --out-dir \ [--ro-file ...] [--env KEY=VALUE ...] [--uds ] \ -- + + agent-sandbox.sh --preflight-only # bring-up only, no command + agent-sandbox.sh --validate-only ``` + The two extra modes are the *pre-agent-step split* described under + [the no-spend pre-agent split](#the-no-spend-pre-agent-split) below; + both take no `-- ` and neither ever starts the agent. + - **[`broker.mjs`](broker.mjs)** — a ~50-line node-stdlib reverse proxy (`node broker.mjs `) that holds the real key on the host and forwards the jail's requests to it. In socket mode it listens on a unix-domain @@ -784,6 +792,63 @@ it drops the userns restriction and retests; if it *still* fails it emits `::error::bwrap sandbox unavailable …` and exits non-zero. A broken sandbox stops the run — it never silently degrades to no sandbox. +### The no-spend pre-agent split + +Everything `agent-sandbox.sh` does *before* `exec bwrap` is no-spend: the sandbox +bring-up above, and then a wall of fail-loud guards (required/absolute-path +argument validation, the `--uds` `-S` check plus a live-broker `/healthz` probe, +clone and out-dir existence, the out-dir↔clone overlap check, and the `--env +KEY=VALUE` / `rw-git-ro` `.git` / `--ro-file` absolute-path-and-existence checks +inside the mount assembly). Every one of them is answerable host-side, with no +jail — `--ro-file` included: `--ro-bind` (unlike `--ro-bind-try`) aborts on a +missing source, so an absent brief or jail-shim would otherwise sail through +validation and kill the billed step, which is the whole miscount in miniature. +Run from inside the billed `Run ` step, any of them failing leaves that +step `failure` having billed nothing — and +[`interval.py`](interval.py)'s exact-name match then reads the agent as *started*, +so `run_audited` counts a spent audit and advances the `GROOM_INTERVAL_DAYS` +cadence clock for a run that spent nothing (BE-4814). The most plausible live +trigger: the broker dies between its step and the agent step, leaving a stale +socket that passes `-S` and fails `/healthz`. That probe runs over `curl`, falling +back to `python3` — under `--validate-only` a host with neither is a hard failure +rather than a skipped probe, because a validation that silently cannot validate +is the green no-op this mode exists to prevent. (A real run keeps the older +best-effort skip: it is about to run the agent regardless, and a spurious failure +*there* is the expensive one.) + +So both halves run in their own `Preflight the sandbox` step, whose name is +deliberately DISTINCT from the billed step: + +| Mode | Runs | Takes | +|---|---|---| +| `--preflight-only` (BE-14756) | ONLY the mutating bring-up | no clone/out-dir/uds/ro-file/env, no `-- command` | +| `--validate-only` (BE-14771) | the SAME guard path a real run walks, stopping at the single `exec` point | the same arguments as the agent step; no `-- command` | + +`--validate-only` deliberately routes through the real code rather than +re-implementing the checks — a parallel copy would drift, and a guard it missed +would still kill the billed step no-spend. Both modes reject nonsensical +combinations loudly (each other, or a `-- command`), so a stray flag on a real +agent step dies instead of becoming a green no-op that runs no agent. + +`--validate-only` walks the *whole* pre-exec path, `preflight()` included — so +**it is only side-effect-free once the bring-up has already succeeded.** In the +`Preflight the sandbox` step that is guaranteed (`--preflight-only` ran first, so +`preflight()` takes its idempotent fast path), and the sole remaining side effect +is the `mkdir -p` on the out-dir that the real run performs anyway; the agent +step's own copies of both are then no-ops. Run standalone on a host where the +sandbox is *not* yet usable, the same call will `sudo apt-get install bubblewrap`, +write `/etc/apparmor.d/bwrap`, and as a last resort `sudo sysctl -w +kernel.apparmor_restrict_unprivileged_userns=0` — the bring-up's host-wide +mutations, from a mode named for validation. Pair it with `--preflight-only`, as +groom.yml does, or expect the bring-up. + +**What this does NOT close:** the window between that step and the agent step. A +broker that dies *after* the `/healthz` probe — or an input deleted after it is +checked — still fails the billed step with no spend, and that failure is still +counted as an audit. Nor can validation reach a mount that `bwrap` itself rejects +at exec for a source that *does* exist. Proving the agent actually BILLED is +tracked separately (BE-4850). + ### Tests — deterministic, no API spend [`tests/sandbox-tests.sh`](tests/sandbox-tests.sh) (run by the `sandbox-tests` job @@ -810,7 +875,13 @@ netns is empty), then its contents (`/proc/net/dev` must list `lo` and nothing else, and neither `/proc/net/route` nor `/proc/net/ipv6_route` may carry a default route). It deliberately does *not* key on a connect exit code, which could not tell an isolated netns from a shared one behind a firewall REJECT or on an offline -host. No `claude`, no API key, no spend. +host. Sections 8 and 9 cover the no-spend split: `--preflight-only` exits 0 on a +usable host and fails loud on a broken `bwrap`, and `--validate-only` exits 0 on +a real run's arguments *without exec'ing the jail* (a stubbed `bwrap` records +every invocation, so "did it exec?" is asserted, not assumed) while failing loud +on a bad argument — including a `--ro-file` that does not exist — and on a +`-S`-passing socket with no live broker, over curl and over the python3 fallback +alike. No `claude`, no API key, no spend. ```bash shellcheck -x .github/groom/agent-sandbox.sh .github/groom/tests/sandbox-tests.sh diff --git a/.github/groom/agent-sandbox.sh b/.github/groom/agent-sandbox.sh index 0ee96b06..047decfb 100755 --- a/.github/groom/agent-sandbox.sh +++ b/.github/groom/agent-sandbox.sh @@ -23,6 +23,10 @@ # # agent-sandbox.sh --preflight-only # +# agent-sandbox.sh --validate-only --clone --clone-mode ro|rw-git-ro \ +# --out-dir [--ro-file ...] [--env KEY=VALUE ...] \ +# [--uds ] +# # --uds bind-mounts a host-side listening unix socket (the broker) to the fixed # in-jail path /run/broker.sock (read-only: connect(2) to a socket works under a # read-only bind, but the jail can't chmod/replace the shared inode). @@ -39,6 +43,24 @@ # sandbox is already usable), so the real `Run ` step's own preflight is # then a no-op. # +# --validate-only is the second half of that split (BE-14771). It takes the SAME +# arguments a real run does and walks the SAME code path — argument validation, +# the absolute-path checks, the --uds `-S` + live-broker healthz probe, the +# clone/out-dir existence + overlap check, preflight(), and the whole bwrap_args +# assembly with its embedded `--env KEY=VALUE`, `rw-git-ro` `.git`-pointer and +# `--ro-file` absolute-path + existence guards — then stops at the single exec +# point instead of exec'ing bwrap, printing `validate-only: all pre-exec guards passed` and +# exiting 0. Every one of those guards is no-spend and fail-loud, but on a real +# run they die INSIDE the billed `Run ` step, which interval.py then reads +# as a started (spent) audit and advances the cadence clock for a run that billed +# nothing (BE-4814). Hoisting them into the same separate step as the bring-up +# moves that failure off the billed step's name. It takes NO `-- `: +# nothing is ever executed, and rejecting one keeps a stray `--validate-only` on +# a real agent step from becoming a green no-op that runs no agent. Walking the +# REAL path (rather than a re-implementation of the checks) is the point — a +# parallel copy would drift, and a guard it missed would still kill `Run ` +# no-spend. +# # The preflight FAILS LOUD: if a working bwrap sandbox cannot be established on # this runner image, the script exits non-zero and the command is NEVER run. It # never falls back to running the command unsandboxed. @@ -66,6 +88,42 @@ selftest() { true 2>/dev/null } +# Probe the broker's /healthz over the host-side unix socket at $1, proving a +# process is actually LISTENING (a socket left behind by a crashed broker passes +# `-S` but gets ECONNREFUSED here). Returns 0 = live, 1 = probe failed, 2 = no +# probe tool on this host. curl is preferred; python3 is the fallback so a +# curl-less host still gets the real check instead of a silent skip (both are +# present on the runner images, and the test suite already requires python3). +# Writes nothing to stdout — this script's stdout is the agent's exec JSON. +broker_healthz() { + local sock="$1" + if command -v curl >/dev/null 2>&1; then + if curl -fsS --max-time 5 --unix-socket "$sock" http://broker/healthz >/dev/null 2>&1; then + return 0 + fi + return 1 + fi + if command -v python3 >/dev/null 2>&1; then + # Same question curl -f answers: does a listener accept the connection and + # answer /healthz with a non-error status (< 400)? + if python3 - "$sock" >/dev/null 2>&1 <<-'PY' + import socket, sys + + s = socket.socket(socket.AF_UNIX) + s.settimeout(5) + s.connect(sys.argv[1]) + s.sendall(b"GET /healthz HTTP/1.0\r\nHost: broker\r\nConnection: close\r\n\r\n") + parts = s.recv(256).split(b"\r\n", 1)[0].split() + sys.exit(0 if len(parts) >= 2 and parts[0].startswith(b"HTTP/1.") and parts[1].isdigit() and int(parts[1]) < 400 else 1) + PY + then + return 0 + fi + return 1 + fi + return 2 +} + # Establish a working unprivileged-userns bwrap sandbox or exit non-zero. Mirrors # the runner image's own podman AppArmor workaround # (actions/runner-images: images/ubuntu/scripts/build/install-container-tools.sh): @@ -116,7 +174,7 @@ PROFILE } main() { - local clone="" clone_mode="" out_dir="" uds="" preflight_only="" + local clone="" clone_mode="" out_dir="" uds="" preflight_only="" validate_only="" local ro_files=() envs=() cmd=() while [[ $# -gt 0 ]]; do @@ -128,11 +186,21 @@ main() { --env) [[ $# -ge 2 ]] || die "--env needs a value"; envs+=("$2"); shift 2 ;; --uds) [[ $# -ge 2 ]] || die "--uds needs a value"; [[ -n "$2" ]] || die "--uds needs a non-empty value"; [[ -z "$uds" ]] || die "--uds may be given at most once"; uds="$2"; shift 2 ;; --preflight-only) preflight_only=1; shift ;; + --validate-only) validate_only=1; shift ;; --) shift; cmd=("$@"); break ;; *) die "unknown argument: $1" ;; esac done + # The two pre-agent-step modes are mutually exclusive: --preflight-only takes NO + # execution-mode arguments and --validate-only requires the full set, so the + # combination cannot mean anything. Reject it instead of letting the + # --preflight-only branch below win and silently skip the validation the caller + # asked for — a green no-op where a caller expected a check is exactly the + # failure mode both of these modes exist to prevent. + [[ -z "$preflight_only" || -z "$validate_only" ]] \ + || die "--preflight-only and --validate-only are mutually exclusive" + # --preflight-only: run ONLY the (mutating) sandbox bring-up and report whether # a working jail is now available (BE-14756). It takes NO clone/clone-mode/ # out-dir/uds/ro-file/env and NO `-- `; combining it with any of those @@ -153,7 +221,17 @@ main() { [[ -n "$clone" ]] || die "--clone is required" [[ -n "$out_dir" ]] || die "--out-dir is required" - [[ ${#cmd[@]} -gt 0 ]] || die "a -- is required" + if [[ -n "$validate_only" ]]; then + # Nothing is ever executed under --validate-only, so a `-- ` here + # is meaningless. Rejecting it (rather than accepting and ignoring it) is + # what keeps a stray --validate-only on a real agent step LOUD: it dies + # instead of exiting 0 having silently discarded the agent invocation. + # Same misuse-guard posture as --preflight-only above. + [[ ${#cmd[@]} -eq 0 ]] \ + || die "--validate-only takes no -- : nothing is executed, so drop the command" + else + [[ ${#cmd[@]} -gt 0 ]] || die "a -- is required" + fi # bwrap binds each of these at its REAL path; a relative value would resolve # against an unexpected CWD instead of failing loud, so require absolute paths. [[ "$clone" = /* ]] || die "--clone must be an absolute path (got '$clone')" @@ -173,11 +251,25 @@ main() { # listening — a stale socket from a crashed broker would pass -S yet the # in-jail connect() then fails at runtime, breaking the fail-loud-before- # running guarantee. Probe /healthz over the socket to confirm a live - # listener (best-effort: only when curl is present, matching the tests). - if command -v curl >/dev/null 2>&1; then - curl -fsS --max-time 5 --unix-socket "$uds" http://broker/healthz >/dev/null 2>&1 \ - || die "--uds socket has no live broker listening (healthz probe failed): $uds" - fi + # listener. + local probe_rc=0 + broker_healthz "$uds" || probe_rc=$? + case "$probe_rc" in + 0) ;; + 1) die "--uds socket has no live broker listening (healthz probe failed): $uds" ;; + # No probe tool on this host. Under --validate-only that silently + # downgrades the mode's whole promise — the crashed-broker case is the + # most plausible live trigger for hoisting these guards off the billed + # step, and skipping the probe hands that failure straight back to it. + # A validation that cannot validate must say so rather than exit 0. On a + # real run, keep the historical best-effort skip: the agent step is about + # to run anyway and a spurious die there is the expensive failure. + *) + [[ -z "$validate_only" ]] \ + || die "--validate-only cannot probe the broker: neither curl nor python3 is on PATH (install one, or drop --uds)" + echo "agent-sandbox: warning: neither curl nor python3 on PATH; skipping the --uds liveness probe" >&2 + ;; + esac fi # out-dir must exist on the host before it can be bound rw into the jail; create @@ -251,6 +343,14 @@ main() { if [[ ${#ro_files[@]} -gt 0 ]]; then for f in "${ro_files[@]}"; do [[ "$f" = /* ]] || die "--ro-file must be an absolute path (got '$f')" + # bwrap's --ro-bind (unlike --ro-bind-try) aborts when the SOURCE does + # not exist, so a missing brief or jail-shim kills the run either way. + # Check it HOST-side — no jail needed, exactly like the `-d` on --clone + # and the `-S` on --uds — so --validate-only catches it too. Otherwise + # validation passes and the failure lands on the billed `Run ` + # step having spent nothing, which is the BE-4814 miscount this whole + # split exists to move off that step name. + [[ -e "$f" ]] || die "--ro-file does not exist on the host: $f" bwrap_args+=(--ro-bind "$f" "$f") done fi @@ -270,6 +370,16 @@ main() { bwrap_args+=(--bind "$out_dir" "$out_dir" --chdir "$clone") + # THE single exec point, and therefore the single place --validate-only can + # branch (BE-14771) and still be sure every pre-exec guard above ran — including + # the ones embedded in the bwrap_args assembly just above (`--env KEY=VALUE`, + # the rw-git-ro `.git`-pointer check, `--ro-file` absolute paths + existence), + # which a validation re-implemented elsewhere would silently skip. + if [[ -n "$validate_only" ]]; then + echo "validate-only: all pre-exec guards passed" + exit 0 + fi + # stdout/stderr pass through to the host shell; the caller redirects stdout # on the HOST side to capture any exec JSON out of the agent's reach. exec bwrap "${bwrap_args[@]}" -- "${cmd[@]}" diff --git a/.github/groom/tests/sandbox-tests.sh b/.github/groom/tests/sandbox-tests.sh index e5157df9..54a21898 100755 --- a/.github/groom/tests/sandbox-tests.sh +++ b/.github/groom/tests/sandbox-tests.sh @@ -483,6 +483,207 @@ if PATH="$failbin:$PATH" "$SANDBOX" --preflight-only >/dev/null 2>&1; then fi pass "--preflight-only fails loud when the sandbox self-test cannot pass" +# --- 9. --validate-only: the pre-exec guards, off the billed step (BE-14771) -- +# The groom jobs run this in the SAME separate "Preflight the sandbox" step as +# --preflight-only. Everything the wrapper checks before `exec bwrap` is no-spend +# and fail-loud, but run from inside the billed "Run " step a failure there +# stamps that step failed having billed nothing — which interval.py reads as a +# STARTED (spent) audit and counts against the cadence clock. --validate-only runs +# the SAME arguments through the SAME code path and stops at the single exec +# point, so those failures land on the preflight step's name instead. +# +# `bwrap` is stubbed for this whole section so "did it exec the jail?" is +# observable: the stub logs its argv and always succeeds, so preflight() takes its +# idempotent fast path (bwrap present + selftest green) and mutates nothing, and +# the only remaining invocation would be the real exec — identifiable by +# --clearenv, which preflight()'s selftest probe never passes. + +stubbin="$work/stubbin" +mkdir -p "$stubbin" +cat > "$stubbin/bwrap" <<'STUB' +#!/bin/sh +echo "$@" >> "$BWRAP_LOG" +exit 0 +STUB +chmod +x "$stubbin/bwrap" +export BWRAP_LOG="$work/bwrap-argv.log" + +# 9d exercises the probe's PREFERRED implementation (curl). The python3 fallback +# and the no-probe-tool case get their own coverage in 9f, so this assertion is +# about knowing WHICH path 9d took, not about the probe existing at all. +command -v curl >/dev/null 2>&1 || fail "curl missing on the host — 9d would exercise the python3 fallback instead of the curl path (9f covers that separately)" + +validate_only() { + PATH="$stubbin:$PATH" "$SANDBOX" --validate-only "$@" +} + +assert_no_jail() { + if grep -q -- '--clearenv' "$BWRAP_LOG"; then + fail "$1: bwrap was exec'd — a real run would have started the agent (and spent budget) here" + fi +} + +# 9a. The exact shape a groom "Preflight the sandbox" step uses — the same +# clone/clone-mode/out-dir/uds/ro-file arguments as the agent step, against the +# LIVE broker socket from section 5 — exits 0, says so, and never execs the jail. +: > "$BWRAP_LOG" +vo_out="$(validate_only --clone "$clone" --clone-mode ro --out-dir "$outdir" \ + --uds "$work/broker.sock" --ro-file "$SHIM" --env FOO=bar 2>/dev/null)" \ + || fail "--validate-only exited non-zero on the arguments a real run accepts" +echo "$vo_out" | grep -q "all pre-exec guards passed" \ + || fail "--validate-only did not report success on stdout (got: $vo_out)" +assert_no_jail "good args" +pass "--validate-only exits 0 on a real run's arguments without exec'ing the jail" + +# 9b. Control for 9a: the SAME arguments WITHOUT --validate-only do reach the exec. +# Without this, a broken stub or an un-stubbed PATH would make every "no jail" +# assertion in this section pass for the wrong reason. +: > "$BWRAP_LOG" +PATH="$stubbin:$PATH" "$SANDBOX" --clone "$clone" --clone-mode ro --out-dir "$outdir" \ + --uds "$work/broker.sock" --ro-file "$SHIM" --env FOO=bar -- true >/dev/null 2>&1 \ + || fail "control run (no --validate-only) failed under the bwrap stub" +grep -q -- '--clearenv' "$BWRAP_LOG" \ + || fail "the bwrap stub never recorded a real exec — section 9's no-jail assertions would false-pass" +pass "bwrap stub observes the real exec (so the 9a/9c/9d/9e no-jail assertions mean something)" + +# 9c. A bad argument must fail validation, with no jail and no spend. Each case +# below targets a DIFFERENT guard, and the last three live inside the bwrap_args +# assembly — the ones a re-implemented validator would silently skip, letting +# validate-only pass while "Run " still dies no-spend on them. +ptr_clone="$work/ptr-clone" +mkdir -p "$ptr_clone" +echo "gitdir: /nowhere/else" > "$ptr_clone/.git" +: > "$BWRAP_LOG" +if validate_only --clone-mode ro --out-dir "$outdir" >/dev/null 2>&1; then + fail "--validate-only accepted a missing --clone" +fi +if validate_only --clone "$clone" --clone-mode ro --out-dir relative/out >/dev/null 2>&1; then + fail "--validate-only accepted a relative --out-dir" +fi +if validate_only --clone "$clone" --clone-mode banana --out-dir "$outdir" >/dev/null 2>&1; then + fail "--validate-only accepted an unknown --clone-mode" +fi +if validate_only --clone "$clone" --clone-mode ro --out-dir "$clone/nested/out" >/dev/null 2>&1; then + fail "--validate-only accepted an out-dir nested in the clone (section 3b's overlap guard)" +fi +if validate_only --clone "$clone" --clone-mode ro --out-dir "$outdir" --env NOEQUALSIGN >/dev/null 2>&1; then + fail "--validate-only accepted --env without '=' (that guard lives in the bwrap_args loop — validate-only must route through it)" +fi +if validate_only --clone "$clone" --clone-mode ro --out-dir "$outdir" --ro-file relative.txt >/dev/null 2>&1; then + fail "--validate-only accepted a relative --ro-file (bwrap_args-loop guard)" +fi +# Absolute but ABSENT. `--ro-bind` (not `--ro-bind-try`) aborts on a missing +# source, so before the host-side `-e` this passed validation and then killed the +# billed agent step no-spend — the exact miscount the hoist exists to prevent. +if validate_only --clone "$clone" --clone-mode ro --out-dir "$outdir" --ro-file "$work/no-such-brief.md" >/dev/null 2>&1; then + fail "--validate-only accepted a --ro-file that does not exist (bwrap would abort at exec, killing the billed step no-spend)" +fi +if validate_only --clone "$ptr_clone" --clone-mode rw-git-ro --out-dir "$outdir" >/dev/null 2>&1; then + fail "--validate-only accepted rw-git-ro over a gitdir-pointer .git (bwrap_args-loop guard)" +fi +assert_no_jail "bad args" +pass "--validate-only fails loud on every bad argument, including the bwrap_args-loop guards, without exec'ing the jail" + +# 9d. The failure this hoisting was written for: a broker that died leaving its +# socket behind. `-S` still passes (the inode is a socket), so only the healthz +# probe catches it — and before BE-14771 that `die` landed inside the billed agent +# step, where it was counted as a spent audit having spent nothing. +dead_sock="$work/dead-broker.sock" +# bind + listen, then exit WITHOUT close(): the filesystem node survives (an +# AF_UNIX bind is not unlinked on exit), so -S passes while connect() gets +# ECONNREFUSED — exactly the shape a crashed broker leaves behind. +python3 -c 'import socket,sys; s=socket.socket(socket.AF_UNIX); s.bind(sys.argv[1]); s.listen(1)' "$dead_sock" +[[ -S "$dead_sock" ]] || fail "fixture: $dead_sock is not a socket — the -S half of the probe would not be exercised" +: > "$BWRAP_LOG" +if validate_only --clone "$clone" --clone-mode ro --out-dir "$outdir" \ + --uds "$dead_sock" >/dev/null 2>&1; then + fail "--validate-only accepted a stale socket with no live broker (-S alone passes it; only the healthz probe catches it)" +fi +assert_no_jail "dead broker socket" +pass "--validate-only fails loud on a -S-passing socket with no live broker, without exec'ing the jail" + +# 9e. Misuse guards, mirroring --preflight-only's (section 8a'): the two pre-agent +# modes are mutually exclusive, and --validate-only refuses a `-- command` so a +# stray --validate-only on a real agent step DIES instead of exiting 0 having +# silently run no agent. +: > "$BWRAP_LOG" +if validate_only --preflight-only >/dev/null 2>&1; then + fail "--validate-only --preflight-only exited 0 (mutually exclusive modes must die)" +fi +if PATH="$stubbin:$PATH" "$SANDBOX" --preflight-only --validate-only >/dev/null 2>&1; then + fail "--preflight-only --validate-only exited 0 (mutually exclusive modes must die, in either order)" +fi +if validate_only --clone "$clone" --clone-mode ro --out-dir "$outdir" -- true >/dev/null 2>&1; then + fail "--validate-only with a -- command exited 0 (must die, not silently discard the command)" +fi +assert_no_jail "misuse combinations" +pass "--validate-only dies loud when combined with --preflight-only or a -- command" + +# 9f. The liveness probe must be STRUCTURAL under --validate-only, not conditional +# on curl. A curl-less host used to skip the probe entirely and exit 0 on a stale +# socket — handing the crashed-broker failure (9d, the case this whole split was +# written for) straight back to the billed step. Build minimal PATHs that omit +# curl (and then python3 too) and pin both halves of the contract. +minpath() { + # $1 = dir to build, rest = basenames to expose. `bash` is needed for the + # script's `#!/usr/bin/env bash` lookup; the rest are what the pre-exec path + # actually shells out to (preflight() takes its fast path against the stub). + local dir="$1"; shift + mkdir -p "$dir" + local b src + for b in "$@"; do + src="$(command -v "$b")" || fail "9f fixture: $b not found on the host" + ln -sf "$src" "$dir/$b" + done + ln -sf "$stubbin/bwrap" "$dir/bwrap" +} + +nocurl="$work/pathnocurl" +minpath "$nocurl" bash mkdir realpath python3 +noprobe="$work/pathnoprobe" +minpath "$noprobe" bash mkdir realpath + +command -v python3 >/dev/null 2>&1 || fail "9f fixture: python3 missing — the fallback half cannot be exercised" +[[ ! -x "$nocurl/curl" ]] || fail "9f fixture: curl leaked into the no-curl PATH" + +# The fallback still PASSES a live broker (section 5's socket) — the point is to +# keep the check working without curl, not to fail closed on every curl-less host. +: > "$BWRAP_LOG" +PATH="$nocurl" "$SANDBOX" --validate-only --clone "$clone" --clone-mode ro \ + --out-dir "$outdir" --uds "$work/broker.sock" --ro-file "$SHIM" >/dev/null 2>&1 \ + || fail "--validate-only failed against a LIVE broker with only the python3 probe available" +assert_no_jail "python3 probe, live broker" + +# ...and still CATCHES the stale socket from 9d, which is the whole point. +: > "$BWRAP_LOG" +if PATH="$nocurl" "$SANDBOX" --validate-only --clone "$clone" --clone-mode ro \ + --out-dir "$outdir" --uds "$dead_sock" >/dev/null 2>&1; then + fail "--validate-only accepted a stale socket on a curl-less host (the python3 fallback did not run)" +fi +assert_no_jail "python3 probe, dead broker" +pass "--validate-only probes broker liveness via python3 when curl is absent (live passes, stale socket still dies)" + +# With NEITHER tool the probe cannot run at all. --validate-only must say so +# rather than exit 0 on an unverified socket: a validation that cannot validate +# is the green no-op this mode exists to prevent. +: > "$BWRAP_LOG" +if PATH="$noprobe" "$SANDBOX" --validate-only --clone "$clone" --clone-mode ro \ + --out-dir "$outdir" --uds "$work/broker.sock" >/dev/null 2>&1; then + fail "--validate-only exited 0 with no probe tool available (the liveness guarantee was silently skipped)" +fi +assert_no_jail "no probe tool" + +# But the REAL run keeps the historical best-effort skip: the agent step is about +# to run regardless, and a spurious die there is the expensive failure. This is +# the control proving 9f denies nothing that worked before. +: > "$BWRAP_LOG" +PATH="$noprobe" "$SANDBOX" --clone "$clone" --clone-mode ro --out-dir "$outdir" \ + --uds "$work/broker.sock" -- true >/dev/null 2>&1 \ + || fail "a REAL run was refused on a host with no probe tool — the probe must stay best-effort off --validate-only" +grep -q -- '--clearenv' "$BWRAP_LOG" \ + || fail "the real run with no probe tool never reached the exec" +pass "no probe tool: --validate-only fails loud, a real run still proceeds (best-effort, as before)" + if [[ "$skips" -gt 0 ]]; then echo "ALL SANDBOX TESTS PASSED ($skips skipped)" else diff --git a/.github/groom/tests/test_interval.py b/.github/groom/tests/test_interval.py index fee23547..d5407103 100644 --- a/.github/groom/tests/test_interval.py +++ b/.github/groom/tests/test_interval.py @@ -488,21 +488,14 @@ def test_groom_yml_names_exactly_the_agent_step_this_module_matches(self): step = step.split("\n - name:", 1)[0] self.assertNotRegex(step, r"(?m)^\s+if:\s", "the pinned agent step must not be conditional") - # BE-14756: the sandbox bring-up is a SEPARATE step that PRECEDES the billed - # agent step, so a no-spend setup failure fails that step and never reaches - # "Run finder" (the runs-jobs API then reports it queued/skipped and - # `agent_step_started` reads it as unstarted). Pin the structure: exactly one - # such step exists in audit_find, it comes BEFORE "Run finder", and its name - # is DISTINCT from the billed step so the exact-name matcher can't confuse - # the two. - preflight_name = "Preflight the sandbox" - self.assertNotEqual(preflight_name, interval.agent_step_name()) - self.assertEqual(finder_block[0].count(f"- name: {preflight_name}\n"), 1) - self.assertLess( - finder_block[0].index(f"- name: {preflight_name}\n"), - finder_block[0].index(f"- name: {interval.agent_step_name()}\n"), - "the sandbox preflight step must come BEFORE the billed agent step", - ) + # BE-14756 + BE-14771: the sandbox bring-up (`--preflight-only`) and the + # pre-exec guard wall (`--validate-only`) are both no-spend, and both run in + # a SEPARATE, distinctly-named step that PRECEDES this one — so a failure in + # either never stamps "Run finder" failed, and `agent_step_started` reads it + # as unstarted rather than as a spent audit. Nothing in THIS module changes + # (the exact-name match is what keeps that step uncounted), and the structure + # is not audit_find-specific, so it is pinned once for all three agent jobs + # in SandboxPreflightHoistTest below rather than a second time here. def test_the_gate_job_is_time_bounded(self): # The gate walks run history (and, for re-run entries, per-attempt job @@ -901,5 +894,126 @@ def test_bad_workflow_file_rejected(self): interval.fetch_workflow_runs("o/r", "ci-groom", run=make_gh_stub([], {})) + +# Every groom job that runs an agent inside the jail: (job key, billed step name). +# `audit_find`'s is the one `interval.py` matches by name; the other two are +# structurally identical and hoist the same guards for the same reason. +_AGENT_JOBS = ( + ("audit_find", "Run finder"), + ("audit_verify", "Run verifier"), + ("build", "Run builder"), +) + +_PREFLIGHT_STEP = "Preflight the sandbox" + +# The mount-shaping arguments of an `agent-sandbox.sh` invocation. `--env` and the +# `-- ` are deliberately excluded — validate-only refuses a command, and +# every --env key in groom.yml is a literal, so the KEY=VALUE guard cannot fire +# from this caller. +_MOUNT_ARGS = r"--(?:clone|clone-mode|out-dir|uds|ro-file)\s+\S+" + + +def _job_block(text, job): + """The `job:` block of groom.yml, as text. + + Matched as text rather than parsed — PyYAML is not stdlib and this repo is + stdlib-only, so a parse would add a CI dependency for a structural pin. + """ + blocks = re.split(r"(?m)^ (?=[A-Za-z_][A-Za-z0-9_-]*:\s*$)", text) + blocks = [b for b in blocks if b.startswith(f"{job}:")] + assert len(blocks) == 1, f"could not isolate the {job} job in groom.yml" + return blocks[0] + + +def _step_body(block, name): + """The body of the `- name: ` step inside a job block.""" + body = block.split(f"- name: {name}\n", 1)[1] + return body.split("\n - name:", 1)[0] + + +def _invocation(block, start): + """The mount arguments of ONE `bash ... agent-sandbox.sh ...` call at `start`. + + Continuation lines until the first that does not end in a backslash. + """ + lines = [] + for line in block[start:].split("\n"): + lines.append(line) + if not line.rstrip().endswith("\\"): + break + return re.findall(_MOUNT_ARGS, "\n".join(lines)) + + +class SandboxPreflightHoistTest(unittest.TestCase): + """BE-14756 + BE-14771, pinned for EVERY agent job, not just the billed one. + + `agent-sandbox.sh`'s pre-exec work — the mutating bring-up (`--preflight-only`) + and the wall of fail-loud guards (`--validate-only`) — is no-spend. Run from + inside a billed `Run ` step, a failure there stamps that step failed + having billed nothing, which `interval.py` reads as a STARTED (spent) audit. + Both phases therefore live in a separate, distinctly-named step that precedes + it. `interval.py` only matches `audit_find`'s step by name, but the verifier + and builder hoist the same guards for the same reason and are equally able to + drift — and they carry the longer `--ro-file` lists and the only + `--clone-mode rw-git-ro`, whose `.git`-pointer guard is the one most likely + to kill those jobs no-spend. + """ + + def setUp(self): + wf = os.path.join(os.path.dirname(__file__), "..", "..", "workflows", "groom.yml") + with open(wf, encoding="utf-8") as f: + self.text = f.read() + + def test_the_preflight_step_precedes_every_billed_agent_step(self): + self.assertNotEqual(_PREFLIGHT_STEP, interval.agent_step_name()) + for job, agent_step in _AGENT_JOBS: + with self.subTest(job=job): + block = _job_block(self.text, job) + self.assertEqual(block.count(f"- name: {agent_step}\n"), 1) + self.assertEqual(block.count(f"- name: {_PREFLIGHT_STEP}\n"), 1) + self.assertLess( + block.index(f"- name: {_PREFLIGHT_STEP}\n"), + block.index(f"- name: {agent_step}\n"), + f"{job}: the sandbox preflight step must come BEFORE the billed agent step", + ) + + def test_both_no_spend_phases_run_off_the_billed_step(self): + for job, agent_step in _AGENT_JOBS: + with self.subTest(job=job): + block = _job_block(self.text, job) + preflight = _step_body(block, _PREFLIGHT_STEP) + billed = _step_body(block, agent_step) + # Match the INVOCATION, not the flag name: both flags are discussed + # in the steps' own comments, so a bare substring check would pass + # on the prose alone and keep passing after the command was deleted. + self.assertIn('agent-sandbox.sh" --preflight-only', preflight, job) + self.assertIn('agent-sandbox.sh" --validate-only', preflight, job) + # And neither phase may run inside the billed step: the whole point + # is that they fail somewhere interval.py does not count. + self.assertNotIn('agent-sandbox.sh" --validate-only', billed, job) + self.assertNotIn('agent-sandbox.sh" --preflight-only', billed, job) + + def test_validate_only_mirrors_the_invocation_its_job_actually_runs(self): + # A `--ro-file` (or a `--clone-mode`) added to `Run ` but not to the + # preflight call leaves that path unvalidated until the BILLED step dies on + # it — precisely the miscount the hoist exists to prevent. The step comments + # say "KEEP THE TWO LISTS IN SYNC"; a comment is not a guard. + for job, agent_step in _AGENT_JOBS: + with self.subTest(job=job): + block = _job_block(self.text, job) + preflight = _step_body(block, _PREFLIGHT_STEP) + billed = _step_body(block, agent_step) + validated = _invocation( + preflight, preflight.index('agent-sandbox.sh" --validate-only') + ) + executed = _invocation(billed, billed.index('agent-sandbox.sh"')) + self.assertTrue(validated, f"{job}: no mount arguments found on the --validate-only call") + self.assertEqual( + validated, + executed, + f"{job}: the --validate-only arguments must mirror the billed agent step's", + ) + + if __name__ == "__main__": unittest.main() diff --git a/.github/workflows/groom.yml b/.github/workflows/groom.yml index 5d690c02..ccaf712c 100644 --- a/.github/workflows/groom.yml +++ b/.github/workflows/groom.yml @@ -1518,29 +1518,60 @@ jobs: echo "key broker ready on $BROKER_SOCK" - name: Preflight the sandbox - # BE-14756: bring the bubblewrap sandbox up in its OWN step, BEFORE the - # billed "Run finder" step below. `--preflight-only` runs ONLY the mutating - # bring-up (install bubblewrap, the AppArmor profile, the sysctl fallback) - # and exits non-zero if a working jail cannot be established — burning NO - # agent budget. Splitting it out is why a no-spend sandbox BRING-UP failure - # never lands on the "Run finder" name: when this step fails the job fails - # but "Run finder" is never reached, so the runs-jobs API reports it - # queued/skipped and interval.py's `agent_step_started` reads it as - # unstarted -> the cadence clock is NOT advanced (mirrors the "Start the - # key broker" split, done for the same reason). NOTE this covers the - # bring-up ONLY: agent-sandbox.sh still runs no-spend, fail-loud guards - # (argument validation, the --uds live-broker healthz probe, the clone/ - # out-dir path + overlap checks) INSIDE "Run finder" before it execs the - # agent, and any of those dying still stamps "Run finder" failed with no - # spend -> counted as a spent audit. Hoisting that validation ahead of the - # billed step (a validate-only mode) is tracked separately. The name is deliberately - # DISTINCT from "Run finder" so interval.py's EXACT-name match never - # mistakes this bring-up for the billed agent step. preflight() is - # idempotent, so "Run finder"'s own preflight then hits its fast path - # (already usable -> instant return, no side effects). + # BE-14756 + BE-14771: run BOTH no-spend pre-agent phases here, under THIS + # step's name, BEFORE the billed "Run finder" step below. + # 1. `--preflight-only` (BE-14756) runs ONLY the mutating sandbox bring-up + # (install bubblewrap, the AppArmor profile, the sysctl fallback) and + # exits non-zero if a working jail cannot be established. + # 2. `--validate-only` (BE-14771) then walks the SAME pre-exec guard path a + # real invocation does — required/absolute-path argument validation, the + # `--uds` `-S` + live-broker `/healthz` probe, clone and out-dir + # existence, the out-dir<->clone overlap check, and the guards embedded + # in the bwrap_args assembly — and stops at the exec point instead of + # exec'ing bwrap. It is handed the SAME arguments as "Run finder" below, + # so KEEP THE TWO LISTS IN SYNC or this validates an invocation the + # agent step does not make. Two deliberate omissions: the `-- ` + # (refused: nothing is executed) and the `--env`s (their keys are + # literals in this file, so the `KEY=VALUE` guard cannot fire from this + # caller; the `--ro-file`/`--clone-mode` guards in that same loop ARE + # exercised). + # Neither phase burns agent budget, and hoisting both is why a no-spend + # failure in either never lands on the "Run finder" name: when this step + # fails the job fails but "Run finder" is never reached, so the runs-jobs API + # reports it queued/skipped and interval.py's `agent_step_started` reads it + # as unstarted -> the cadence clock is NOT advanced for a run that billed + # nothing (BE-4814; mirrors the "Start the key broker" split, done for the + # same reason). Before BE-14771 phase 2's guards ran inside "Run finder", so + # e.g. a broker that died leaving a stale socket (passes `-S`, fails healthz) + # failed the billed step having spent nothing and was counted as an audit. + # RESIDUAL, deliberately NOT closed here: the window between this step and + # "Run finder" stays open. A broker that dies AFTER the healthz probe below + # still kills "Run finder" no-spend, and that failure is still counted as a + # spent audit — the same residual the bring-up split carries. Nor can this + # reach a failure `bwrap` itself raises at exec — a mount that fails on a + # source that exists — since validating without running the jail is the + # whole point. (A `--ro-file` source that is simply MISSING is NOT in that + # class: it needs no jail to detect, so `--validate-only` checks it + # host-side, `-e`, alongside the `-d` on --clone and the `-S` on --uds.) + # Proving the finder actually BILLED is tracked separately (BE-4850). + # The name is deliberately DISTINCT from "Run finder" so interval.py's + # EXACT-name match never mistakes either phase for the billed agent step. + # preflight() is idempotent, so "Run finder"'s own preflight then hits its + # fast path (already usable -> instant return, no side effects) and re-runs + # the same validation as a cheap no-op. + # `-e` is spelled out (the sibling steps write `set -uo pipefail` and inherit + # it from the runner's default `bash -e`) because this step now runs TWO + # commands: a bring-up failure must fail the step, not be masked by the + # validation's exit status. run: | - set -uo pipefail + set -euo pipefail bash "$GROOM_ASSETS/agent-sandbox.sh" --preflight-only + bash "$GROOM_ASSETS/agent-sandbox.sh" --validate-only \ + --clone "$GROOM_CLONE" --clone-mode ro \ + --out-dir "$GROOM_OUT_DIR" \ + --uds "$BROKER_SOCK" \ + --ro-file "$GROOM_ASSETS/jail-shim.mjs" \ + --ro-file /tmp/groom-finder-prompt.md # NAME IS LOAD-BEARING — `interval.py` matches this step name EXACTLY # (`_AGENT_STEP_NAME`) against the runs-jobs API's `steps[]` to decide @@ -1551,9 +1582,10 @@ jobs: # `test_interval.py` pins both halves. Same for adding an `if:` to this # step: a conditionally-skipped agent inside a SUCCEEDING job would still # count, because a success is trusted on the job conclusion alone. The - # sandbox bring-up is deliberately in the separate "Preflight the sandbox" - # step ABOVE (BE-14756), NOT here, so a no-spend bring-up failure fails that - # distinctly-named step instead of this billed one. + # sandbox bring-up AND its pre-exec argument/path validation are deliberately + # in the separate "Preflight the sandbox" step ABOVE (BE-14756, BE-14771), NOT + # here, so a no-spend setup or validation failure fails that distinctly-named + # step instead of this billed one. - name: Run finder # BE-4303: the agent runs ONLY inside `agent-sandbox.sh` (the bubblewrap # jail). What the old `chmod`/`env -u` dance did by hand the jail now does @@ -1986,17 +2018,29 @@ jobs: echo "key broker ready on $BROKER_SOCK" - name: Preflight the sandbox - # BE-14756: bring the bubblewrap sandbox up in its OWN step before the - # "Run verifier" step (see the finder job's "Preflight the sandbox" step - # for the full rationale). `--preflight-only` runs ONLY the mutating - # bring-up and burns no agent budget; "Run verifier"'s own preflight then - # hits its idempotent fast path. Only the finder job is cadence-gated, so - # this split does not affect interval.py's clock here — it mirrors the + # BE-14756 + BE-14771: do the bubblewrap bring-up AND the pre-exec argument/ + # path validation in this OWN step before the "Run verifier" step (see the + # finder job's "Preflight the sandbox" step for the full rationale, the + # keep-the-arg-lists-in-sync warning, and the residual this does NOT close). + # `--preflight-only` runs ONLY the mutating bring-up; `--validate-only` then + # walks the same guard path a real invocation does (including the `--uds` + # live-broker healthz probe) and stops at the exec point. Neither burns agent + # budget; "Run verifier"'s own preflight then hits its idempotent fast path + # and its validation re-runs as a no-op. Only the finder job is cadence-gated, + # so this split does not affect interval.py's clock here — it mirrors the # finder for consistency (and the "Start the key broker" precedent), and - # keeps a no-spend bring-up failure off the agent step in either case. + # keeps a no-spend bring-up or validation failure off the agent step in + # either case. run: | - set -uo pipefail + set -euo pipefail bash "$GROOM_ASSETS/agent-sandbox.sh" --preflight-only + bash "$GROOM_ASSETS/agent-sandbox.sh" --validate-only \ + --clone "$GROOM_CLONE" --clone-mode ro \ + --out-dir "$GROOM_OUT_DIR" \ + --uds "$BROKER_SOCK" \ + --ro-file "$GROOM_ASSETS/jail-shim.mjs" \ + --ro-file /tmp/groom-verifier-prompt.md \ + --ro-file "$FINDER_IN" - name: Run verifier # A FRESH agent session on a FRESH checkout — it sees only the finder's JSON @@ -2914,17 +2958,32 @@ jobs: echo "key broker ready on $BROKER_SOCK" - name: Preflight the sandbox - # BE-14756: bring the bubblewrap sandbox up in its OWN step before the - # "Run builder" step (see the finder job's "Preflight the sandbox" step - # for the full rationale). `--preflight-only` runs ONLY the mutating - # bring-up and burns no agent budget; "Run builder"'s own preflight then - # hits its idempotent fast path. Only the finder job is cadence-gated, so - # this split does not affect interval.py's clock here — it mirrors the - # finder for consistency (and the "Start the key broker" precedent), and - # keeps a no-spend bring-up failure off the agent step in either case. + # BE-14756 + BE-14771: do the bubblewrap bring-up AND the pre-exec argument/ + # path validation in this OWN step before the "Run builder" step (see the + # finder job's "Preflight the sandbox" step for the full rationale, the + # keep-the-arg-lists-in-sync warning, and the residual this does NOT close). + # `--preflight-only` runs ONLY the mutating bring-up; `--validate-only` then + # walks the same guard path a real invocation does and stops at the exec + # point. `--clone-mode rw-git-ro` is passed because that is what "Run builder" + # uses, and it is load-bearing here: rw-git-ro is the one mode with an extra + # pre-exec guard (the clone's `.git` must be a real directory, not a git- + # worktree `gitdir:` pointer file), so validating in `ro` would silently skip + # the check most likely to kill this job no-spend. Neither phase burns agent + # budget; "Run builder"'s own preflight then hits its idempotent fast path and + # its validation re-runs as a no-op. Only the finder job is cadence-gated, so + # this split does not affect interval.py's clock here — it mirrors the finder + # for consistency (and the "Start the key broker" precedent), and keeps a + # no-spend bring-up or validation failure off the agent step in either case. run: | - set -uo pipefail + set -euo pipefail bash "$GROOM_ASSETS/agent-sandbox.sh" --preflight-only + bash "$GROOM_ASSETS/agent-sandbox.sh" --validate-only \ + --clone "$GROOM_CLONE" --clone-mode rw-git-ro \ + --out-dir "$GROOM_OUT_DIR" \ + --uds "$BROKER_SOCK" \ + --ro-file "$GROOM_ASSETS/jail-shim.mjs" \ + --ro-file /tmp/groom-builder-prompt.md \ + --ro-file "$FINDING_IN" - name: Run builder # Runs ONLY inside `agent-sandbox.sh` (BE-4303), in `rw-git-ro` mode: the