[PR #297] [MERGED] feat(groom): split sandbox preflight into its own step before the agent step #297

New issue

Closed

opened 2026-10-26 22:38:11 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:11 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/297
Author: @mattmillerai
Created: 9/16/2026
Status: ✅ Merged
Merged: 9/17/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-14756-groom-sandbox-preflight-split

📝 Commits (6)

ad1036d feat(groom): run the finder/verifier/builder agents inside the sandbox + key broker
eff84d9 fix(groom): address cursor-review panel findings on the sandbox+broker wiring
4e80b8e Merge origin/main into matt/be-4303-groom-sandbox-wire
c4a8fd1 feat(groom): split sandbox preflight into its own step before Run finder/verifier/builder
3005fab fix(groom): make --preflight-only fail loud on misuse and drop the misleading --selftest alias
2b2539f Merge remote-tracking branch 'origin/main' into matt/be-14756-groom-sandbox-preflight-split

📊 Changes

4 files changed (+163 additions, -2 deletions)

View changed files

📝 .github/groom/agent-sandbox.sh (+33 -1)
📝 .github/groom/tests/sandbox-tests.sh (+43 -0)
📝 .github/groom/tests/test_interval.py (+32 -0)
📝 .github/workflows/groom.yml (+55 -1)

📄 Description

STACKED — merging lands on matt/be-4303-groom-sandbox-wire (owned by @mattmillerai, PR #293), NOT main. This is NOT safe to merge to main on its own: its diff builds on the sandbox invocation that only exists on #293's branch. Do not merge until #293 lands; GitHub auto-retargets this to main when #293 merges.

ELI-5

The groomer runs an AI agent inside a locked-down sandbox. Setting that sandbox up used to happen inside the same step that runs the agent. If the setup failed (no sandbox tool, a blocked kernel feature), the whole step failed — and a bookkeeping check read that failure as "we already spent this run's audit," so it stopped grooming for a whole interval even though no agent actually ran. This PR moves the sandbox setup into its own step before the agent step. Now a setup failure fails the setup step, the agent step never starts, and the bookkeeping correctly sees "the agent never ran" — so grooming isn't suppressed.

What changed

agent-sandbox.sh — new --preflight-only flag (alias --selftest): runs only the mutating sandbox bring-up (preflight() — install bubblewrap, the AppArmor profile, the sysctl fallback) and exits 0 if a working jail is usable, non-zero otherwise. Takes no --clone/--out-dir/-- <command>; it short-circuits before those requirement checks. preflight() itself is unchanged and already idempotent + fail-loud.
groom.yml — a new, distinctly-named Preflight the sandbox step added before the Run <agent> step in audit_find, audit_verify and build, running agent-sandbox.sh --preflight-only. Because the bring-up now lives in its own step, a no-agent-budget setup failure fails that step; Run <agent> 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. The real Run <agent> step's own preflight() then hits its fast path (already usable → instant return, no side effects). The load-bearing comment above Run finder was updated to note the split.
interval.py — unchanged; _AGENT_STEP_NAME = "Run finder" stays.
Tests — sandbox-tests.sh gains section 8 (--preflight-only/--selftest exit 0 with no command/clone/out-dir on a working host; fail-loud with a stubbed failing bwrap + no-op sudo). test_interval.py extends the audit_find structural pin (a preflight step exists, precedes Run finder, and its name ≠ agent_step_name()) and adds a run_audited case (a queued/skipped Run finder step alongside a completed/failure preflight step → not a spent audit).

Only audit_find gates the clock

interval.py is consulted only for the finder job. The verifier/builder splits do not affect cadence — they mirror the finder for consistency and the Start the key broker precedent, and keep a no-spend bring-up failure off the agent step in either job.

Testing

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' → 399 tests OK.
shellcheck -x .github/groom/agent-sandbox.sh .github/groom/tests/sandbox-tests.sh → clean.
check_workflow_pins.py → OK; check_agents_md.py --root . → passed (pre-existing warnings on untouched AGENTS.md).
groom.yml parses as YAML; all three Preflight the sandbox steps verified to precede their agent step.
New sandbox-tests.sh section 8 verified directly: --preflight-only/--selftest → exit 0; stubbed-broken-bwrap → non-zero; a normal invocation missing --out-dir still dies (exit 2).

Residual

Stacked on an unmerged PR. This builds on #293 (open), which introduces the sandbox invocation this PR splits. Do not merge to main before #293; the base is #293's branch and GitHub retargets to main on #293's merge.
The confinement suite was not run green end-to-end on this host. Sections 1–4 pass; section 5 (the broker credential proxy, which execs node inside the jail) fails here because this host's node lives outside /usr and the jail binds only /usr read-only — a host-env limitation, not a regression from this diff (section 5 is unchanged by it). CI's ubuntu runner installs node under /usr, so section 5 passes there. The new section 8 was verified independently (see Testing).
Complementary prior work, not closed here: a broader, separately-tracked prior-art item proposes proving the finder actually billed (verifying spend from the exec JSON), which also covers the residual case of a claude that starts but bills ~nothing. This PR is the cheaper structural split and deliberately does not close that broader case — the two are complementary.
Duplicate-check was flagged INCOMPLETE at ticket creation (the Linear half could not be fully searched). I confirmed via gh pr list that no open PR in this repo implements this preflight split (only #293 touches the sandbox; #295 is unrelated gate-path validation). A human should still glance for a sibling ticket before merge, as the ticket noted.
Unexercised artifacts: the ticket cross-references three related issues (a broader billing-proof item, a gate-path-validation PR, and the originating spike). I have no issue-tracker access, so I could not read their bodies or set the cross-link relations the ticket suggested; those remain for a human. No attachments were named in the brief.

Provenance

Authored by: agent-work loop
Verified: interval unittest suite (399 OK), shellcheck clean, workflow-pins + agents-md lints pass, groom.yml YAML parse + step-ordering check, and direct verification of the new --preflight-only/--selftest/fail-loud behavior. The full confinement suite could not complete on this host (section 5 needs node under /usr); noted under Residual.
Deviations: built as a STACKED PR on #293's branch rather than waiting for #293 to merge, per the standing "always stack on an unmerged PR with buildable code" directive. The ticket text suggested holding until #293 merged; stacking yields a clean net diff now and GitHub auto-retargets on merge.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/297 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/16/2026 **Status:** ✅ Merged **Merged:** 9/17/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-14756-groom-sandbox-preflight-split` --- ### 📝 Commits (6) - [`ad1036d`](https://github.com/Comfy-Org/github-workflows/commit/ad1036de6dc49263692337167dd2fe5a47842de5) feat(groom): run the finder/verifier/builder agents inside the sandbox + key broker - [`eff84d9`](https://github.com/Comfy-Org/github-workflows/commit/eff84d94cabda078d6cd5ba70dfaea2267d3dcc5) fix(groom): address cursor-review panel findings on the sandbox+broker wiring - [`4e80b8e`](https://github.com/Comfy-Org/github-workflows/commit/4e80b8ee0ebab388eaf608c9d197d9e21e1e9aab) Merge origin/main into matt/be-4303-groom-sandbox-wire - [`c4a8fd1`](https://github.com/Comfy-Org/github-workflows/commit/c4a8fd150ee73e1fc295f1ecd790c079515ffdde) feat(groom): split sandbox preflight into its own step before Run finder/verifier/builder - [`3005fab`](https://github.com/Comfy-Org/github-workflows/commit/3005fabedd1f10d523fa3dcfad331612677d0577) fix(groom): make --preflight-only fail loud on misuse and drop the misleading --selftest alias - [`2b2539f`](https://github.com/Comfy-Org/github-workflows/commit/2b2539f40d01c464ddcf258d18104c2bda831ea6) Merge remote-tracking branch 'origin/main' into matt/be-14756-groom-sandbox-preflight-split ### 📊 Changes **4 files changed** (+163 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/agent-sandbox.sh` (+33 -1) 📝 `.github/groom/tests/sandbox-tests.sh` (+43 -0) 📝 `.github/groom/tests/test_interval.py` (+32 -0) 📝 `.github/workflows/groom.yml` (+55 -1) </details> ### 📄 Description **STACKED — merging lands on `matt/be-4303-groom-sandbox-wire` (owned by @mattmillerai, PR #293), NOT `main`.** This is NOT safe to merge to `main` on its own: its diff builds on the sandbox invocation that only exists on #293's branch. Do not merge until #293 lands; GitHub auto-retargets this to `main` when #293 merges. ## ELI-5 The groomer runs an AI agent inside a locked-down sandbox. Setting that sandbox up used to happen *inside* the same step that runs the agent. If the setup failed (no sandbox tool, a blocked kernel feature), the whole step failed — and a bookkeeping check read that failure as "we already spent this run's audit," so it stopped grooming for a whole interval even though no agent actually ran. This PR moves the sandbox setup into its own step *before* the agent step. Now a setup failure fails the setup step, the agent step never starts, and the bookkeeping correctly sees "the agent never ran" — so grooming isn't suppressed. ## What changed - **`agent-sandbox.sh`** — new `--preflight-only` flag (alias `--selftest`): runs only the mutating sandbox bring-up (`preflight()` — install bubblewrap, the AppArmor profile, the sysctl fallback) and exits `0` if a working jail is usable, non-zero otherwise. Takes no `--clone`/`--out-dir`/`-- <command>`; it short-circuits before those requirement checks. `preflight()` itself is unchanged and already idempotent + fail-loud. - **`groom.yml`** — a new, distinctly-named `Preflight the sandbox` step added *before* the `Run <agent>` step in `audit_find`, `audit_verify` and `build`, running `agent-sandbox.sh --preflight-only`. Because the bring-up now lives in its own step, a no-agent-budget setup failure fails *that* step; `Run <agent>` 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. The real `Run <agent>` step's own `preflight()` then hits its fast path (already usable → instant return, no side effects). The load-bearing comment above `Run finder` was updated to note the split. - **`interval.py`** — unchanged; `_AGENT_STEP_NAME = "Run finder"` stays. - **Tests** — `sandbox-tests.sh` gains section 8 (`--preflight-only`/`--selftest` exit `0` with no command/clone/out-dir on a working host; fail-loud with a stubbed failing `bwrap` + no-op `sudo`). `test_interval.py` extends the `audit_find` structural pin (a preflight step exists, precedes `Run finder`, and its name ≠ `agent_step_name()`) and adds a `run_audited` case (a `queued`/`skipped` `Run finder` step alongside a `completed`/`failure` preflight step → not a spent audit). ## Only audit_find gates the clock `interval.py` is consulted only for the finder job. The verifier/builder splits do not affect cadence — they mirror the finder for consistency and the `Start the key broker` precedent, and keep a no-spend bring-up failure off the agent step in either job. ## Testing - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'` → **399 tests OK**. - `shellcheck -x .github/groom/agent-sandbox.sh .github/groom/tests/sandbox-tests.sh` → clean. - `check_workflow_pins.py` → OK; `check_agents_md.py --root .` → passed (pre-existing warnings on untouched `AGENTS.md`). - `groom.yml` parses as YAML; all three `Preflight the sandbox` steps verified to precede their agent step. - New `sandbox-tests.sh` section 8 verified directly: `--preflight-only`/`--selftest` → exit `0`; stubbed-broken-`bwrap` → non-zero; a normal invocation missing `--out-dir` still dies (exit `2`). ## Residual - **Stacked on an unmerged PR.** This builds on #293 (open), which introduces the sandbox invocation this PR splits. Do not merge to `main` before #293; the base is #293's branch and GitHub retargets to `main` on #293's merge. - **The confinement suite was not run green end-to-end on this host.** Sections 1–4 pass; section 5 (the broker credential proxy, which execs `node` *inside* the jail) fails here because this host's `node` lives outside `/usr` and the jail binds only `/usr` read-only — a host-env limitation, not a regression from this diff (section 5 is unchanged by it). CI's ubuntu runner installs `node` under `/usr`, so section 5 passes there. The new section 8 was verified independently (see Testing). - **Complementary prior work, not closed here:** a broader, separately-tracked prior-art item proposes proving the finder actually *billed* (verifying spend from the exec JSON), which also covers the residual case of a `claude` that starts but bills ~nothing. This PR is the cheaper structural split and deliberately does not close that broader case — the two are complementary. - **Duplicate-check was flagged INCOMPLETE at ticket creation** (the Linear half could not be fully searched). I confirmed via `gh pr list` that no open PR in this repo implements this preflight split (only #293 touches the sandbox; #295 is unrelated gate-path validation). A human should still glance for a sibling ticket before merge, as the ticket noted. - **Unexercised artifacts:** the ticket cross-references three related issues (a broader billing-proof item, a gate-path-validation PR, and the originating spike). I have no issue-tracker access, so I could not read their bodies or set the cross-link relations the ticket suggested; those remain for a human. No attachments were named in the brief. ## Provenance - **Authored by:** agent-work loop - **Verified:** interval unittest suite (399 OK), shellcheck clean, workflow-pins + agents-md lints pass, `groom.yml` YAML parse + step-ordering check, and direct verification of the new `--preflight-only`/`--selftest`/fail-loud behavior. The full confinement suite could not complete on this host (section 5 needs `node` under `/usr`); noted under Residual. - **Deviations:** built as a STACKED PR on #293's branch rather than waiting for #293 to merge, per the standing "always stack on an unmerged PR with buildable code" directive. The ticket text suggested holding until #293 merged; stacking yields a clean net diff now and GitHub auto-retargets on merge. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:11 +00:00