[PR #295] [MERGED] fix(groom): validate extra_denied_paths in the gate before agent spend #300

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-14745-groom-gate-validate-extra-denied-paths

📝 Commits (2)

3c327e6 fix(groom): validate extra_denied_paths in the gate before agent spend
321afdf fix(groom): gate extra_denied_paths validation on should_run; honest fail-closed scope

📊 Changes

3 files changed (+84 additions, -6 deletions)

View changed files

📝 .github/groom/tests/test_patch_policy.py (+32 -0)
📝 .github/workflows/groom.yml (+50 -4)
📝 docs/callers/groom.md (+2 -2)

📄 Description

ELI-5

If a caller mistypes one of its extra_denied_paths patterns, groom used to
only notice the typo at the very end — after the finder, verifier, and builder
agents had already been paid for. This moves that one check to the front door,
so a broken pattern fails the run instantly and costs $0 in agent spend. Nothing
about what fails changes: same error message, same fail-closed behaviour, just
~10 minutes and a full agent budget earlier.

What changed

extra_denied_paths (a caller's additive builder patch deny-list) was first
compiled in the build job's Capture patch step — the pipe into
patch_policy.py under set -euo pipefail — which runs after the finder,
verifier, and per-finding builder agents have billed. An uncompilable pattern
therefore burned the full agent budget for every selected finding, every
scheduled run, before aborting.

This adds one step to the cheap, pre-agent gate job (after Resolve operational knobs, so $GROOM_ASSETS is populated) that validates the patterns
before any model work:

if: ${{ inputs.builder }}
env:
  EXTRA_DENIED_PATHS: ${{ inputs.extra_denied_paths }}
run: |
  set -euo pipefail
  printf '' | python3 "$GROOM_ASSETS/patch_policy.py" >/dev/null

main() compiles EXTRA_DENIED_PATHS before reading stdin, so feeding it an
empty diff exercises only the pattern compile — exit 0 on a good/empty value,
exit 2 with the identical ::error:: on a bad one. The gate already checks out
the assets (Load groom assets), so no second checkout is added, and the
compile logic stays solely in patch_policy.py — no python3 -c loop, no
duplication (per AGENTS.md single-source-of-truth). A gate failure fails every
downstream job (they all needs: gate), so no agent job runs.

Builder-gated: extra_denied_paths is ignored unless builder is true, so a
non-builder caller's typo stays harmless exactly as it is today.

The operator-facing message and fail-closed shape are byte-identical to the
build-job failure seen today — just earlier and free.

Testing

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v — 396 pass.
Added GatePreAgentValidationTest pinning the gate's exact empty-stdin
invocation: rc 2 on a bad pattern (with ::error:: naming it), rc 0 on a
good pattern and on an empty/unset value. (The pre-existing main() tests all
fed a non-empty diff; these cover the printf '' path directly.)
Ran the exact gate command locally against the real patch_policy.py:
bad pattern → rc 2 + ::error:: on stderr; good / empty / unset → rc 0 with
no output. Confirms both that a typo fails closed and that a valid caller is
not spuriously blocked.
check_workflow_pins.py (OK — gate still declares workflows_ref, guarded)
and check_agents_md.py (passed; the two warnings are pre-existing and
unrelated to this diff). groom.yml parses as YAML.

Residual

No live Actions run of groom.yml. The workflow itself cannot be executed
from this environment; the substance — the exact run: command against the
real patch_policy.py — was verified locally as above, but the step's behaviour
inside a real gate job (step ordering, $GROOM_ASSETS population by the
checkout) was reasoned from the file, not observed in CI. First real invocation
is the definitive check.
Related-ticket overlap unverified. The source flagged a possible overlap
with a sibling "residual" ticket from the same feature PR (#291) and asked that
it be checked for an already-stated gate-validation task before implementing.
That ticket's body was not readable from this environment (no issue-tracker
access here), so the overlap could not be confirmed either way. Open-PR search
found no in-flight PR touching extra_denied_paths gate validation (checked
the extra_denied_paths and groom gate PR queues), so this does not duplicate
any open PR; a duplicate ticket remains possible and should be de-duped by a
human with tracker access.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v (396 pass); local run of the gate's run: command against patch_policy.py (rc 2 bad / rc 0 good/empty); check_workflow_pins.py, check_agents_md.py, YAML parse — all green.
Deviations: none.

_{🔄 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/295 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/16/2026 **Status:** ✅ Merged **Merged:** 9/16/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-14745-groom-gate-validate-extra-denied-paths` --- ### 📝 Commits (2) - [`3c327e6`](https://github.com/Comfy-Org/github-workflows/commit/3c327e68420bad8844ed45767b2c243773639093) fix(groom): validate extra_denied_paths in the gate before agent spend - [`321afdf`](https://github.com/Comfy-Org/github-workflows/commit/321afdf1f2c5cd3ab6bf3bec4d54dc2ffb1dbbd9) fix(groom): gate extra_denied_paths validation on should_run; honest fail-closed scope ### 📊 Changes **3 files changed** (+84 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/tests/test_patch_policy.py` (+32 -0) 📝 `.github/workflows/groom.yml` (+50 -4) 📝 `docs/callers/groom.md` (+2 -2) </details> ### 📄 Description ## ELI-5 If a caller mistypes one of its `extra_denied_paths` patterns, groom used to only notice the typo at the very end — after the finder, verifier, and builder agents had already been paid for. This moves that one check to the front door, so a broken pattern fails the run instantly and costs $0 in agent spend. Nothing about *what* fails changes: same error message, same fail-closed behaviour, just ~10 minutes and a full agent budget earlier. ## What changed `extra_denied_paths` (a caller's additive builder patch deny-list) was first compiled in the `build` job's `Capture patch` step — the pipe into `patch_policy.py` under `set -euo pipefail` — which runs *after* the finder, verifier, and per-finding builder agents have billed. An uncompilable pattern therefore burned the full agent budget for every selected finding, every scheduled run, before aborting. This adds **one** step to the cheap, pre-agent `gate` job (after `Resolve operational knobs`, so `$GROOM_ASSETS` is populated) that validates the patterns before any model work: ```yaml if: ${{ inputs.builder }} env: EXTRA_DENIED_PATHS: ${{ inputs.extra_denied_paths }} run: | set -euo pipefail printf '' | python3 "$GROOM_ASSETS/patch_policy.py" >/dev/null ``` `main()` compiles `EXTRA_DENIED_PATHS` **before** reading stdin, so feeding it an empty diff exercises only the pattern compile — exit 0 on a good/empty value, exit 2 with the identical `::error::` on a bad one. The gate already checks out the assets (`Load groom assets`), so **no second checkout** is added, and the compile logic stays solely in `patch_policy.py` — **no `python3 -c` loop, no duplication** (per AGENTS.md single-source-of-truth). A gate failure fails every downstream job (they all `needs: gate`), so no agent job runs. Builder-gated: `extra_denied_paths` is ignored unless `builder` is true, so a non-builder caller's typo stays harmless exactly as it is today. The operator-facing message and fail-closed shape are byte-identical to the build-job failure seen today — just earlier and free. ## Testing - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v` — 396 pass. - Added `GatePreAgentValidationTest` pinning the gate's exact empty-stdin invocation: rc 2 on a bad pattern (with `::error::` naming it), rc 0 on a good pattern and on an empty/unset value. (The pre-existing `main()` tests all fed a non-empty diff; these cover the `printf ''` path directly.) - Ran the exact gate command locally against the real `patch_policy.py`: bad pattern → rc 2 + `::error::` on stderr; good / empty / unset → rc 0 with no output. Confirms both that a typo fails closed and that a valid caller is **not** spuriously blocked. - `check_workflow_pins.py` (OK — gate still declares `workflows_ref`, guarded) and `check_agents_md.py` (passed; the two warnings are pre-existing and unrelated to this diff). `groom.yml` parses as YAML. ## Residual - **No live Actions run of `groom.yml`.** The workflow itself cannot be executed from this environment; the substance — the exact `run:` command against the real `patch_policy.py` — was verified locally as above, but the step's behaviour inside a real `gate` job (step ordering, `$GROOM_ASSETS` population by the checkout) was reasoned from the file, not observed in CI. First real invocation is the definitive check. - **Related-ticket overlap unverified.** The source flagged a possible overlap with a sibling "residual" ticket from the same feature PR (#291) and asked that it be checked for an already-stated gate-validation task before implementing. That ticket's body was not readable from this environment (no issue-tracker access here), so the overlap could not be confirmed either way. Open-PR search found no in-flight PR touching `extra_denied_paths` gate validation (checked the `extra_denied_paths` and `groom gate` PR queues), so this does not duplicate any open PR; a duplicate *ticket* remains possible and should be de-duped by a human with tracker access. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v` (396 pass); local run of the gate's `run:` command against `patch_policy.py` (rc 2 bad / rc 0 good/empty); `check_workflow_pins.py`, `check_agents_md.py`, YAML parse — all green. - **Deviations:** none. --- <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:12 +00:00