[PR #291] [MERGED] feat(groom): add extra_denied_paths caller input to extend the builder patch deny-list #295

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-4405-groom-extra-denied-paths

📝 Commits (1)

567c9f3 feat(groom): add extra_denied_paths caller input to extend the builder patch deny-list

📊 Changes

5 files changed (+402 additions, -39 deletions)

View changed files

📝 .github/groom/README.md (+32 -10)
📝 .github/groom/patch_policy.py (+125 -15)
📝 .github/groom/tests/test_patch_policy.py (+150 -0)
📝 .github/workflows/groom.yml (+77 -13)
📝 docs/callers/groom.md (+18 -1)

📄 Description

ELI-5

The groom auto-builder refuses to open an auto-PR when a machine-written patch touches a file your CI runs before a human reviews it (workflow files, lockfiles, build config) — those go to a filed issue instead, so untrusted code never executes with your secrets pre-review. That deny-list lived only in the shared patch_policy.py, so a repo with its OWN privileged file (a checked-in scripts/ci/ entrypoint, a custom build runner) had to edit the shared reusable workflow to protect itself. This adds an extra_denied_paths input so a caller lists its own paths in its own caller file. The patterns can only ADD denials (widen), never remove one, and a typo that won't compile fails the run closed rather than silently letting the path it was meant to guard slip through.

What changed

groom.yml — new workflow_call input extra_denied_paths (type: string, default ''), threaded into the Capture patch step via env: EXTRA_DENIED_PATHS: ${{ inputs.extra_denied_paths }} (never inline ${{ }} into run: — matches the file's SINK/CADENCE/etc. convention). Header comment + commented caller example updated; the "must add it here first" phrasing replaced with "add repo-specific paths via extra_denied_paths; propose broadly-applicable ones upstream".
patch_policy.py — main() reads EXTRA_DENIED_PATHS, compile_extra_patterns() splits on newlines (blank/whitespace lines skipped, each surviving line .strip()-ed and compiled re.IGNORECASE to mirror the built-in _PATTERN), and denied_paths/denied_entries/_is_denied OR the compiled patterns onto the built-in test via a new optional extra_patterns arg (default (), so every existing call site is byte-identical).
Tests — test_patch_policy.py gains coverage for: an extra pattern matching; extra patterns not affecting the built-ins (and a path only the extra covers not matching without it); additive-only behaviour; flow through denied_entries + the symlink-mode deny; case-insensitive compile; strip/skip-blanks; empty/whitespace no-op; invalid regex raising InvalidExtraPattern naming the bad pattern; and main() end-to-end via the env var, including the fail-closed path and the unset/empty no-op. 51 tests total, all green.
Docs — .github/groom/README.md (patch_policy.py section + intro) and docs/callers/groom.md (input table row + a new "Extending the patch deny-list" prose section).

Design decision — fail-closed by aborting the step, not by "bail to an issue"

The ticket plan says an uncompilable pattern should "cause the capture step to bail that patch to an issue." I implemented fail-closed as a nonzero exit (::error:: naming the bad pattern → the set -euo pipefail, no-|| true pipeline aborts the Capture patch step → no PR opens for any finding until the pattern is fixed), rather than emitting a synthetic denied-path so the step files an issue. Two reasons, both about a global config typo (the env var is identical for every matrix leg):

No ledger pollution. build_pr's bail path (file_issue) writes the finding's signature_marker into the issue, so a bail permanently dedups that finding in the ledger. A config typo would therefore file every finding this run as a (misleadingly-reasoned "modifies a CI-privileged path") issue and mark them all done, so they'd never be re-attempted as PRs even after the typo is fixed. Aborting drops nothing to the ledger — findings are simply re-proposed next run.
Matches the module's existing fail-closed idiom. parse_raw_z already fails closed by raising → aborting the same pipeline ("fails CLOSED (no PR opens)"), and the step's own comment documents that a nonzero exit from patch_policy.py fails closed.

The ticket's essential requirement — "a typo in a security control must never silently widen the allow side," and the acceptance line "an invalid pattern bails the patch (fail closed), with a clear ::error:: naming the bad pattern" — is fully met: fail closed, loud ::error::, no PR. Flagging the abort-vs-file-an-issue choice explicitly as a judgment call.

Verification

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' — all groom script suites green (51 in test_patch_policy.py).
python3 .github/workflow-pins/check_workflow_pins.py — OK (no reusable defaults workflows_ref; every ref checkout guarded).
python3 .github/agents-md-integrity/check_agents_md.py --root . — passed (2 pre-existing warnings, unrelated).
actionlint 1.7.7 on groom.yml — the only findings are the 16 pre-existing, documented job.workflow_sha false-positives (actionlint's job-context schema predates runner v2.334.0; groom.yml's header calls this out). Zero new findings, none near the edited regions.
YAML parses; the new input resolves as type: string, required: false, default: ''.

Residual

Not exercised: a live groom builder run. The env→compile→deny logic and the fail-closed exit are verified by unit tests and by static YAML/actionlint checks, but I could not run an end-to-end credentialed builder: true groom workflow (needs the bot App token, a real repo, and a scheduled/dispatched run). The Capture patch step's shell wiring around $EXTRA_DENIED_PATHS is reviewed statically only.
No caller changes, per the acceptance criteria. comfy-cloud-mcp-server (or any other caller) opting in via extra_denied_paths is a separate caller-repo decision and is not part of this PR. This repo's own ci-groom.yml pin is intentionally left for the standard caller-bump machinery to move post-merge (this is a behavioural change, so it should bump — not a Skip-caller-bump case).
Dataset-of-record paths stay hardcoded. extra_denied_paths covers the CI-privileged extension case the README anticipated ("make the class a caller input"); the BE-9609 dataset-of-record tuple remains non-caller-configurable by design and is out of scope here.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest (.github/groom/tests): 51 passed, 0 failed; check_workflow_pins.py: OK; check_agents_md.py: passed (2 pre-existing warnings); actionlint 1.7.7 groom.yml: only the 16 documented job.workflow_sha false-positives
Deviations: fail-closed implemented as a nonzero-exit step-abort rather than the plan's "bail to an issue", to avoid dedup-ledger pollution from a global config typo (see "Design decision" above)

_{🔄 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/291 **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-4405-groom-extra-denied-paths` --- ### 📝 Commits (1) - [`567c9f3`](https://github.com/Comfy-Org/github-workflows/commit/567c9f34ce701923234794a5388516dd195e6e98) feat(groom): add extra_denied_paths caller input to extend the builder patch deny-list ### 📊 Changes **5 files changed** (+402 additions, -39 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+32 -10) 📝 `.github/groom/patch_policy.py` (+125 -15) 📝 `.github/groom/tests/test_patch_policy.py` (+150 -0) 📝 `.github/workflows/groom.yml` (+77 -13) 📝 `docs/callers/groom.md` (+18 -1) </details> ### 📄 Description ## ELI-5 The groom auto-builder refuses to open an auto-PR when a machine-written patch touches a file your CI *runs* before a human reviews it (workflow files, lockfiles, build config) — those go to a filed issue instead, so untrusted code never executes with your secrets pre-review. That deny-list lived only in the shared `patch_policy.py`, so a repo with its OWN privileged file (a checked-in `scripts/ci/` entrypoint, a custom build runner) had to edit the shared reusable workflow to protect itself. This adds an `extra_denied_paths` input so a caller lists its own paths in its own caller file. The patterns can only ADD denials (widen), never remove one, and a typo that won't compile fails the run closed rather than silently letting the path it was meant to guard slip through. ## What changed - **`groom.yml`** — new `workflow_call` input `extra_denied_paths` (`type: string`, default `''`), threaded into the `Capture patch` step via `env: EXTRA_DENIED_PATHS: ${{ inputs.extra_denied_paths }}` (never inline `${{ }}` into `run:` — matches the file's `SINK`/`CADENCE`/etc. convention). Header comment + commented caller example updated; the "must add it here first" phrasing replaced with "add repo-specific paths via `extra_denied_paths`; propose broadly-applicable ones upstream". - **`patch_policy.py`** — `main()` reads `EXTRA_DENIED_PATHS`, `compile_extra_patterns()` splits on newlines (blank/whitespace lines skipped, each surviving line `.strip()`-ed and compiled `re.IGNORECASE` to mirror the built-in `_PATTERN`), and `denied_paths`/`denied_entries`/`_is_denied` OR the compiled patterns onto the built-in test via a new optional `extra_patterns` arg (default `()`, so every existing call site is byte-identical). - **Tests** — `test_patch_policy.py` gains coverage for: an extra pattern matching; extra patterns not affecting the built-ins (and a path *only* the extra covers not matching without it); additive-only behaviour; flow through `denied_entries` + the symlink-mode deny; case-insensitive compile; strip/skip-blanks; empty/whitespace no-op; invalid regex raising `InvalidExtraPattern` naming the bad pattern; and `main()` end-to-end via the env var, including the fail-closed path and the unset/empty no-op. 51 tests total, all green. - **Docs** — `.github/groom/README.md` (`patch_policy.py` section + intro) and `docs/callers/groom.md` (input table row + a new "Extending the patch deny-list" prose section). ## Design decision — fail-closed by aborting the step, not by "bail to an issue" The ticket plan says an uncompilable pattern should "cause the capture step to bail that patch to an issue." I implemented fail-closed as a **nonzero exit** (`::error::` naming the bad pattern → the `set -euo pipefail`, no-`|| true` pipeline aborts the `Capture patch` step → **no PR opens for any finding** until the pattern is fixed), rather than emitting a synthetic denied-path so the step files an issue. Two reasons, both about a *global config typo* (the env var is identical for every matrix leg): 1. **No ledger pollution.** `build_pr`'s bail path (`file_issue`) writes the finding's `signature_marker` into the issue, so a bail permanently dedups that finding in the ledger. A config typo would therefore file *every* finding this run as a (misleadingly-reasoned "modifies a CI-privileged path") issue and mark them all done, so they'd never be re-attempted as PRs even after the typo is fixed. Aborting drops nothing to the ledger — findings are simply re-proposed next run. 2. **Matches the module's existing fail-closed idiom.** `parse_raw_z` already fails closed by raising → aborting the same pipeline ("fails CLOSED (no PR opens)"), and the step's own comment documents that a nonzero exit from `patch_policy.py` fails closed. The ticket's essential requirement — "a typo in a security control must never silently widen the allow side," and the acceptance line "an invalid pattern bails the patch (fail closed), with a clear `::error::` naming the bad pattern" — is fully met: fail closed, loud `::error::`, no PR. Flagging the abort-vs-file-an-issue choice explicitly as a judgment call. ## Verification - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'` — all groom script suites green (51 in `test_patch_policy.py`). - `python3 .github/workflow-pins/check_workflow_pins.py` — OK (no reusable defaults `workflows_ref`; every ref checkout guarded). - `python3 .github/agents-md-integrity/check_agents_md.py --root .` — passed (2 pre-existing warnings, unrelated). - `actionlint 1.7.7` on `groom.yml` — the only findings are the 16 pre-existing, **documented** `job.workflow_sha` false-positives (actionlint's `job`-context schema predates runner v2.334.0; groom.yml's header calls this out). Zero new findings, none near the edited regions. - YAML parses; the new input resolves as `type: string, required: false, default: ''`. ## Residual - **Not exercised: a live groom builder run.** The env→compile→deny logic and the fail-closed exit are verified by unit tests and by static YAML/actionlint checks, but I could not run an end-to-end credentialed `builder: true` groom workflow (needs the bot App token, a real repo, and a scheduled/dispatched run). The `Capture patch` step's shell wiring around `$EXTRA_DENIED_PATHS` is reviewed statically only. - **No caller changes**, per the acceptance criteria. `comfy-cloud-mcp-server` (or any other caller) opting in via `extra_denied_paths` is a separate caller-repo decision and is not part of this PR. This repo's own `ci-groom.yml` pin is intentionally left for the standard caller-bump machinery to move post-merge (this is a behavioural change, so it should bump — not a `Skip-caller-bump` case). - **Dataset-of-record paths stay hardcoded.** `extra_denied_paths` covers the CI-privileged extension case the README anticipated ("make the class a caller input"); the BE-9609 dataset-of-record tuple remains non-caller-configurable by design and is out of scope here. ## Provenance - **Authored by:** agent-work loop - **Verified:** python3 -m unittest (.github/groom/tests): 51 passed, 0 failed; check_workflow_pins.py: OK; check_agents_md.py: passed (2 pre-existing warnings); actionlint 1.7.7 groom.yml: only the 16 documented job.workflow_sha false-positives - **Deviations:** fail-closed implemented as a nonzero-exit step-abort rather than the plan's "bail to an issue", to avoid dedup-ledger pollution from a global config typo (see "Design decision" above) --- <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:10 +00:00