Comfy-Org
diff --git a/‎.github/groom/README.md‎
Lines changed: 74 additions & 3 deletions b/‎.github/groom/README.md‎
Lines changed: 74 additions & 3 deletions
diff --git a/‎.github/groom/agent-sandbox.sh‎
Lines changed: 117 additions & 7 deletions b/‎.github/groom/agent-sandbox.sh‎
Lines changed: 117 additions & 7 deletions
@@ -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 <path> --clone-mode ro|rw-git-ro --out-dir <path> \
       [--ro-file <path> ...] [--env KEY=VALUE ...] [--uds <host-socket-path>] \
       -- <command...>
+
+  agent-sandbox.sh --preflight-only          # bring-up only, no command
+  agent-sandbox.sh --validate-only <same args as a real run, minus the command>
   ```
 
+  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 `-- <command>` and neither ever starts the agent.
+
 - **[`broker.mjs`](broker.mjs)** — a ~50-line node-stdlib reverse proxy
   (`node broker.mjs <port|socket-path>`) 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 <agent>` 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
 
@@ -23,6 +23,10 @@
 #
 #   agent-sandbox.sh --preflight-only
 #
+#   agent-sandbox.sh --validate-only --clone <path> --clone-mode ro|rw-git-ro \
+#       --out-dir <path> [--ro-file <path> ...] [--env KEY=VALUE ...] \
+#       [--uds <host-socket-path>]
+#
 #   --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 <agent>` 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 <agent>` 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 `-- <command>`:
+#   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 <agent>`
+#   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 `-- <command>`; 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 -- <command...> is required"
+	if [[ -n "$validate_only" ]]; then
+		# Nothing is ever executed under --validate-only, so a `-- <command>` 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 -- <command...>: nothing is executed, so drop the command"
+	else
+		[[ ${#cmd[@]} -gt 0 ]] || die "a -- <command...> 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 <agent>`
+			# 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[@]}"