mirror of
https://github.com/Comfy-Org/github-workflows.git
synced 2026-11-05 00:17:37 +00:00
[PR #301] [MERGED] feat(groom): hoist agent-sandbox pre-exec validation out of the billed agent step #301
Labels
No labels
cursor-review-catalog-drift
groom
groom
pull-request
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Comfy-Org/github-workflows#301
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
📋 Pull Request Information
Original PR: https://github.com/Comfy-Org/github-workflows/pull/301
Author: @mattmillerai
Created: 9/17/2026
Status: ✅ Merged
Merged: 9/18/2026
Merged by: @mattmillerai
Base:
main← Head:matt/be-14771-groom-sandbox-validate-only📝 Commits (10+)
ad1036dfeat(groom): run the finder/verifier/builder agents inside the sandbox + key brokereff84d9fix(groom): address cursor-review panel findings on the sandbox+broker wiring4e80b8eMerge origin/main into matt/be-4303-groom-sandbox-wirec4a8fd1feat(groom): split sandbox preflight into its own step before Run finder/verifier/builder3005fabfix(groom): make --preflight-only fail loud on misuse and drop the misleading --selftest alias2b2539fMerge remote-tracking branch 'origin/main' into matt/be-14756-groom-sandbox-preflight-splitbebca74feat(groom): hoist agent-sandbox pre-exec validation out of the billed agent step025b801fix(groom): close the --validate-only gaps the review panel found (BE-14771)7fd4200Merge remote-tracking branch 'origin/main' into matt/be-14771-groom-sandbox-validate-only3132f6eMerge remote-tracking branch 'origin/main' into matt/be-14771-groom-sandbox-validate-only📊 Changes
5 files changed (+621 additions, -66 deletions)
View changed files
📝
.github/groom/README.md(+74 -3)📝
.github/groom/agent-sandbox.sh(+117 -7)📝
.github/groom/tests/sandbox-tests.sh(+201 -0)📝
.github/groom/tests/test_interval.py(+129 -15)📝
.github/workflows/groom.yml(+100 -41)📄 Description
#297 merged into
main; GitHub retargeted this PR's base tomainautomatically, so it is no longer stacked. Its diff also picked up a semantic conflict with #300 (sandbox-tests.sh§7 renumbering + a shared final-echo line), resolved in the merge commit below — both PRs' test sections and README prose are preserved.ELI-5
The sandbox wrapper checks a pile of things before it starts the agent — are the paths absolute, does the clone exist, is the key broker actually alive, does the output directory overlap the repo. All of those checks are free: if one fails, the agent never starts and nothing is billed. The problem is where they run. They run inside the step named "Run finder", so when one fails, the cadence gate sees a failed "Run finder" step, concludes the audit was spent, and pushes the next scheduled groom a whole interval into the future — for a run that spent nothing. This moves those checks into the step that already does the free setup work, where a failure costs nothing and counts for nothing.
What changed
agent-sandbox.sh— new--validate-only. It parses exactly like a real run (--clone/--clone-mode/--out-dir/--uds/--ro-file/--env) and walks the identical code path, branching at the singleexec bwrapline instead of re-implementing the checks. That routing is the point: several guards live inside thebwrap_argsassembly loop (--env KEY=VALUE, therw-git-ro.git-pointer check,--ro-fileabsolute paths), and a separate validator would pass while the real step still died no-spend on one of them. It refuses a-- <command>and refuses to be combined with--preflight-only, mirroring that flag's misuse-guard style — so a stray--validate-onlyon a real agent step dies loudly rather than exiting 0 having run no agent.groom.yml— all three agent jobs. The existingPreflight the sandboxstep now runs--preflight-onlythen--validate-onlywith the same arguments the correspondingRun <agent>step uses (rw-git-rofor the builder). The step name is unchanged, sointerval.py's exact-name match still never counts it.set -eis spelled out there now: the step runs two commands, and a bring-up failure must not be masked by the second one's exit status.Tests.
sandbox-tests.sh§9 stubsbwrapso "did it exec the jail?" is asserted rather than assumed — the stub logs every invocation, and the real exec is identifiable by--clearenv(the preflight self-test probe never passes it). §9b is the control: the same arguments without--validate-onlydo reach the exec, so the no-jail assertions cannot pass for the wrong reason.test_interval.pygains a comment plus two structural pins (see judgment calls).Verification of the artifacts this touches
--validate-onlyon good args, against the live broker socket from §5 — exits 0, prints the success line, nobwrapexec.--clone, relative--out-dir, unknown--clone-mode, out-dir nested in the clone,--envwithout=, relative--ro-file, andrw-git-roover agitdir:pointer file. All non-zero, no exec.-Spasses,/healthzfails. Non-zero, no exec.--validate-only --preflight-only(both orders) and--validate-only … -- trueboth die.--validate-onlythat branches early (before the required-arg checks) is caught by §9c; one that branches afterpreflight()but before the mount assembly is caught by the--envcase — the exact regression the "route through the real path" requirement exists to prevent. On the workflow side, deleting the--validate-onlyinvocation and dropping a single--ro-filefrom it each failtest_interval.py. A first draft of that assertion matched the flag name and kept passing against the step's own prose after the command was deleted; it now matches the invocation.Run <agent>steps — all three match.Review round (cursor-review panel, 4 findings — all fixed in
025b801)Every one was a guard
--validate-onlyclaimed to cover but did not, so everyone left its failure on the billed step — the exact miscount the hoist exists to
prevent.
--ro-filesources were checked for absoluteness but not existence.--ro-bind(not--ro-bind-try) aborts on a missing source, so an absentbrief or jail-shim passed validation and then killed
Run <agent>no-spend.[[ -e "$f" ]]now sits beside the absolute-path check in the samebwrap_argsloop. Confirmed it cannot false-fail the real caller: every--ro-filegroom.yml passes is produced by an unconditional step thatprecedes
Preflight the sandboxin all three agent jobs.command -v curl-gated, so a curl-lesshost exited 0 on a stale socket — the crashed-broker case named as this
split's most plausible live trigger. Extracted as
broker_healthz(): curlpreferred, python3 fallback, and only a host with neither is fatal —
fatal under
--validate-onlyalone. A real run keeps the best-effort skip.audit_find,leaving the verifier and builder — the two with the longer
--ro-filelistsand the only
rw-git-ro— unpinned. NewSandboxPreflightHoistTestparameterizes ordering, both invocations, and the token-for-token argument
mirror over all three jobs; the
audit_find-only copy is replaced by apointer so there is no second copy to drift.
--validate-onlyas side-effect-free. Itbranches after
preflight(), so standalone it runs the bring-up'sapt-get/AppArmor/sysctlmutations. The claim is now scoped to "once thebring-up has already succeeded", with the mutations named. groom.yml's
residual comment is corrected the same way — a missing
--ro-fileis nolonger listed among the failures validation cannot reach.
Each fix is mutation-tested: deleting the
-echeck, the no-probe-tool fatalbranch, the python3 fallback, a builder
--ro-file, or the verifier's--validate-onlycall each fail their own assertion and nothing else.Negative-claim falsification
The diff adds two deny paths (
--preflight-onlyand--validate-onlyare mutually exclusive;--validate-onlytakes no-- <command>). Neither denies a capability that previously existed —--validate-onlyis introduced by this diff, and the pre-existinga -- <command...> is requiredcheck for real runs is preserved byte-for-byte in theelsebranch, not broadened. Empirically: §9b runs the full real path (same arguments, no--validate-only, a-- truecommand) through the stubbedbwrapand it still reaches the exec, and the pre-existing suite's sections 1–7 still drive real sandboxed commands. So nothing that worked before is refused now.The review round adds two more deny paths, both falsified empirically:
--ro-filenow dies host-side. This denies nothing:--ro-bindalready aborted on a missing source, so the run failed either way — only the
step name and the error message change. Sections 1–7 still drive real
--ro-fileinvocations through the jail, and §9b's control still reaches theexec.
--validate-onlywith no probe tool now dies. Rather than ship thedead-end the finding proposed (fatal on missing curl), the capability is
preserved by a python3 fallback: §9f proves
--validate-onlystill succeedsagainst a LIVE broker on a PATH containing python3 but no curl, and still
catches §9d's stale socket there. The die fires only where neither tool
exists, and §9f's last control proves a real run on that same PATH still
reaches the exec — so the refusal is scoped to the one case where the mode
genuinely cannot do its job.
Judgment calls
--validate-onlyrejects a-- <command>rather than tolerating one. The plan allowed either relaxing the "command required" check or having callers pass-- true. Rejecting is the stricter reading: it keeps the relaxation from becoming a silent green no-op if the flag is ever pasted onto a real agent step. Cost: you cannot copy a real invocation verbatim and append the flag — you must drop the command, and the error message says so.--envs. Every--envingroom.ymlhas a literal key, so theKEY=VALUEguard cannot fire from this caller, and duplicating the multi-linePROMPTinto a second place is a drift hazard for no coverage. The other guards in that same loop (--ro-file,--clone-mode) are exercised. Stated in the step comment.test_interval.pygot two assertions, not just the comment the plan asked for. One pins that the preflight step actually invokes both flags and that the billed step does not; one pins that the--validate-onlyargument list mirrors the agent step's. The step comment says "keep the two lists in sync", and a comment is not a guard. Both are scoped toaudit_find, matching that file's existing scope..github/groom/README.mddocuments both halves of the split (--preflight-onlywas merged in #297 without a README entry), since documenting the second half alone would read as if the first did not exist.Residual
Preflight the sandboxandRun <agent>is still open, by design. A broker that dies after the healthz probe, or a--ro-filethat is deleted in between, still kills the billed step with no spend — andinterval.pystill counts that as a spent audit. This is the same residual the bring-up split carries. Proving the agent actually BILLED (the alternative fix, at theinterval.pyend) is the tracked follow-up; it is a distinct approach, not a duplicate of this one.bwrapitself at exec — a non-existent--ro-file, a mount that fails. Validating without running the jail is the whole point, so these are structurally out of reach of--validate-onlyand land on the billed step, no-spend, counted. Stated in the step comment so the window is not implied to be closed.Preflight the sandboxahead ofStart the key broker. If that lands, the healthz half of--validate-onlycannot run in a pre-broker step — the two would need splitting into a broker-independent arg/path pass and a broker-dependent healthz pass. This PR deliberately does not pre-empt that; it keeps the whole validation in a step that runs after the broker is up.sandbox-tests.sh. On this host, sections 1–4 and 7–9 of that suite pass but section 5's in-jail assertion cannot:nodelives outside/usrhere and so is invisible inside the jail. Section 9 was therefore run through a local harness that sets up the same fixtures and a live broker; CI'ssandbox-testsjob runs the whole file onubuntu-latest, wherenodeis under/usr/local, and is the authority.Provenance
shellcheck -x .github/groom/agent-sandbox.sh .github/groom/tests/sandbox-tests.sh: clean;python3 -m unittest discover -s .github/groom/tests -p 'test_*.py': 405 tests, 0 failures (402 + #300's 3 newtest_scope.pycases picked up by the merge);sandbox-tests.sh§§8–9 via a local fixture harness with a live broker: 10 assertions, all PASS (§5 not runnable on this host — see Residual; §§1–4 pass unchanged); mutation-tested all five new guards/assertions individually;check_workflow_pins.py: OK, 11 workflows, 0 defaults;check_agents_md.py --root .: passed (2 pre-existing warnings). After mergingorigin/mainto pick up #298–#300: resolved thesandbox-tests.sh/README conflicts by keeping this PR's §9 content and #300's §7e renumbering plus its conditional finalALL SANDBOX TESTS PASSED (N skipped)echo side by side; CI (sandbox-tests,unittest,unittest + lint this repo,AGENTS.md integrity, CodeRabbit) all green on the merged tree; diff vs.origin/mainstill 5 files, +621/-66, matching the pre-merge delta.curlfatal under--validate-only; shipped a python3 fallback instead, so the probe stays structural without denying the mode on a curl-less host. The four judgment calls above are unchanged and still additive.🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.