[PR #71] [MERGED] feat(groom): extract CI-privileged patch path policy into tested patch_policy.py (BE-4404) #82

New issue

Closed

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

bsakaguchi commented

2026-10-26 22:36:02 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/71
Author: @mattmillerai
Created: 7/24/2026
Status: ✅ Merged
Merged: 8/4/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-4404-groom-patch-policy

📝 Commits (3)

5278fbc feat(groom): extract CI-privileged patch path policy into tested patch_policy.py + close lockfile/.husky/action.yml gaps (BE-4404)
ef119a8 fix(groom): close patch_policy under-blocks + fail-open flagged by review panel (BE-4404)
c52567a Merge remote-tracking branch 'origin/main' into matt/be-4404-groom-patch-policy

📊 Changes

4 files changed (+616 additions, -36 deletions)

View changed files

📝 .github/groom/README.md (+40 -0)
➕ .github/groom/patch_policy.py (+226 -0)
➕ .github/groom/tests/test_patch_policy.py (+307 -0)
📝 .github/workflows/groom.yml (+43 -36)

📄 Description

ELI-5

The groom auto-builder lets a (prompt-injectable, credential-free) AI agent write a code patch, and a later job opens a review-gated PR from it. But review only gates the merge — pushing the branch already runs the repo's CI, with secrets, before any human looks. So if that patch edits a file CI executes, it's arbitrary code execution in credentialed CI. There was already a deny-list that downgrades such a patch from an auto-PR to a plain filed issue — but it was a one-line grep nobody could test, and it missed the scariest case: dependency lockfiles. Rewrite a lockfile's resolved URL + integrity hash and the next npm ci downloads and runs an attacker's install script. This PR moves the deny-list into a small, unit-tested Python file and plugs that gap (plus .husky/, action.yml, and build files for many ecosystems).

What changed

.github/groom/patch_policy.py (new) — denied_paths(paths) returns the CI-privileged subset of changed paths; main() reads NUL-delimited paths from stdin (matching git diff --cached --name-only -z) and prints matches, exit 0 always (the caller tests non-emptiness). Ports the pre-existing patterns verbatim, then adds: lockfiles (package-lock.json, npm-shrinkwrap.json, yarn.lock, pnpm-lock.yaml, bun.lock/bun.lockb, poetry.lock, uv.lock, Pipfile.lock, Cargo.lock, Gemfile.lock), .husky/ (any segment), action.yml/action.yaml, .gitmodules, requirements*.txt, Pipfile, Cargo.toml, build.rs, Package.swift, *.pbxproj, *.gradle/*.gradle.kts/settings.gradle, gradle/wrapper/, Gemfile, Rakefile, *.gemspec, CMakeLists.txt/*.cmake, Bazel WORKSPACE(.bazel)/BUILD(.bazel)/*.bzl, Jenkinsfile, Taskfile.yml, just(J)file.
.github/groom/tests/test_patch_policy.py (new) — 19 tests: every ported + added pattern family at root and nested depth, anchored negatives (src/foo.py, README.md, packages.json, Dockerfile.md, … must NOT match), and the git raw-byte regressions (embedded " and embedded newline still caught from -z bytes; both newline halves tested).
groom.yml Capture patch step — the inline grep -E becomes git -C repo diff --cached --name-only -z | python3 "$GROOM_ASSETS/patch_policy.py". GROOM_ASSETS is the existing workflow-env var pointing into the _groom_assets checkout the build job already does (Load groom assets (builder brief) runs before this step). The stale comment block is trimmed to a pointer keeping the two load-bearing notes (conservative default; NUL-delimited comparison mandatory), and the bail reason string now names the policy file.
README.md + groom.yml header — mention the policy file and its structural limit: the deny-list guards privileged-config surfaces, but any patch's source still executes when caller CI runs its tests — callers should avoid exposing secrets to test steps and consider npm ci --ignore-scripts where viable.

Verification

actionlint .github/workflows/groom.yml — clean (includes shellcheck on the run: block).
python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' — 78 tests OK (ledger 59 + patch_policy 19); test_patch_policy and test_ledger both auto-discovered by the existing test-groom-scripts.yml pattern, no workflow change needed.
End-to-end pipeline simulated: printf '.github/workflows/ci.yml\0package-lock.json\0src/app.py\0.husky/pre-commit\0tools/action.yml\0README.md\0' | python3 patch_policy.py prints exactly the four privileged paths; clean-only input prints nothing; empty stdin → exit 0.

Acceptance criteria

✅ actionlint clean; ledger + new policy tests green locally.
✅ Behavior identical for previously-covered patterns (PortedPatternsTest pins the full ported set at root + nested).
✅ New coverage: package-lock.json, .husky/pre-commit, tools/action.yml each bail.
✅ Bail semantics unchanged — a denied patch still bails → lands in the ledger and files a groom issue; nothing dropped.
✅ No change to the builder: false default; no caller touched.

Judgment calls

|| true kept on the assignment. The original wrapped the grep in || true (grep exits 1 on no-match, the common case). patch_policy.py exits 0 always, so || true now only masks a genuine git/python3 failure — preserving the original posture that this line never aborts the step under set -euo pipefail. Dropping it would instead abort the matrix leg on any error, which (per the step's own secret-scan comment) drops the finding entirely rather than filing it. The script is unit-tested and SHA-pinned via workflows_ref, so a broken script is caught by test-groom-scripts.yml before merge. Flagged rather than silently changed.
.github/(workflows|actions)/ stays root-anchored (verbatim port), while newly-added .husky/ and gradle/wrapper/ match at any depth per the ticket — documented inline. Over-blocking is the safe direction, but the ported pattern is kept byte-for-byte to guarantee no regression on the previously-covered set.

Negative-claim falsification

Not applicable in the #581 sense: this diff does not deny a product capability or add a "not supported"/STOP dead-end. "Denied" here means a builder patch is downgraded from an auto-PR to a filed issue — the finding is still delivered and the human authors the privileged change. Nothing is dropped. The deny behavior is empirically validated by the end-to-end pipeline simulation above, not merely asserted by self-authored tests.

BE-4404

_{🔄 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/71 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/24/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4404-groom-patch-policy` --- ### 📝 Commits (3) - [`5278fbc`](https://github.com/Comfy-Org/github-workflows/commit/5278fbc9da6fc5d3cf28fffff9b627e49bb2b5a6) feat(groom): extract CI-privileged patch path policy into tested patch_policy.py + close lockfile/.husky/action.yml gaps (BE-4404) - [`ef119a8`](https://github.com/Comfy-Org/github-workflows/commit/ef119a87b0a74f535f66e12bac2c68effd893775) fix(groom): close patch_policy under-blocks + fail-open flagged by review panel (BE-4404) - [`c52567a`](https://github.com/Comfy-Org/github-workflows/commit/c52567aa9606db037757ff543c66e4a64f34a276) Merge remote-tracking branch 'origin/main' into matt/be-4404-groom-patch-policy ### 📊 Changes **4 files changed** (+616 additions, -36 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+40 -0) ➕ `.github/groom/patch_policy.py` (+226 -0) ➕ `.github/groom/tests/test_patch_policy.py` (+307 -0) 📝 `.github/workflows/groom.yml` (+43 -36) </details> ### 📄 Description ## ELI-5 The groom auto-builder lets a (prompt-injectable, credential-free) AI agent write a code patch, and a later job opens a **review-gated** PR from it. But review only gates the *merge* — pushing the branch already runs the repo's CI, with secrets, before any human looks. So if that patch edits a file CI *executes*, it's arbitrary code execution in credentialed CI. There was already a deny-list that downgrades such a patch from an auto-PR to a plain filed issue — but it was a one-line `grep` nobody could test, and it missed the scariest case: **dependency lockfiles**. Rewrite a lockfile's `resolved` URL + `integrity` hash and the next `npm ci` downloads and runs an attacker's install script. This PR moves the deny-list into a small, unit-tested Python file and plugs that gap (plus `.husky/`, `action.yml`, and build files for many ecosystems). ## What changed - **`.github/groom/patch_policy.py`** (new) — `denied_paths(paths)` returns the CI-privileged subset of changed paths; `main()` reads NUL-delimited paths from stdin (matching `git diff --cached --name-only -z`) and prints matches, **exit 0 always** (the caller tests non-emptiness). Ports the pre-existing patterns verbatim, then adds: lockfiles (`package-lock.json`, `npm-shrinkwrap.json`, `yarn.lock`, `pnpm-lock.yaml`, `bun.lock`/`bun.lockb`, `poetry.lock`, `uv.lock`, `Pipfile.lock`, `Cargo.lock`, `Gemfile.lock`), `.husky/` (any segment), `action.yml`/`action.yaml`, `.gitmodules`, `requirements*.txt`, `Pipfile`, `Cargo.toml`, `build.rs`, `Package.swift`, `*.pbxproj`, `*.gradle`/`*.gradle.kts`/`settings.gradle`, `gradle/wrapper/`, `Gemfile`, `Rakefile`, `*.gemspec`, `CMakeLists.txt`/`*.cmake`, Bazel `WORKSPACE(.bazel)`/`BUILD(.bazel)`/`*.bzl`, `Jenkinsfile`, `Taskfile.yml`, `just(J)file`. - **`.github/groom/tests/test_patch_policy.py`** (new) — 19 tests: every ported + added pattern family at root and nested depth, anchored negatives (`src/foo.py`, `README.md`, `packages.json`, `Dockerfile.md`, … must NOT match), and the git raw-byte regressions (embedded `"` and embedded newline still caught from `-z` bytes; both newline halves tested). - **`groom.yml` `Capture patch` step** — the inline `grep -E` becomes `git -C repo diff --cached --name-only -z | python3 "$GROOM_ASSETS/patch_policy.py"`. `GROOM_ASSETS` is the existing workflow-env var pointing into the `_groom_assets` checkout the `build` job already does (`Load groom assets (builder brief)` runs before this step). The stale comment block is trimmed to a pointer keeping the two load-bearing notes (conservative default; NUL-delimited comparison mandatory), and the bail reason string now names the policy file. - **`README.md` + `groom.yml` header** — mention the policy file and its **structural limit**: the deny-list guards privileged-*config* surfaces, but any patch's source still executes when caller CI runs its tests — callers should avoid exposing secrets to test steps and consider `npm ci --ignore-scripts` where viable. ## Verification - `actionlint .github/workflows/groom.yml` — clean (includes shellcheck on the `run:` block). - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'` — **78 tests OK** (ledger 59 + patch_policy 19); `test_patch_policy` and `test_ledger` both auto-discovered by the existing `test-groom-scripts.yml` pattern, no workflow change needed. - End-to-end pipeline simulated: `printf '.github/workflows/ci.yml\0package-lock.json\0src/app.py\0.husky/pre-commit\0tools/action.yml\0README.md\0' | python3 patch_policy.py` prints exactly the four privileged paths; clean-only input prints nothing; empty stdin → exit 0. ## Acceptance criteria - ✅ actionlint clean; ledger + new policy tests green locally. - ✅ Behavior identical for previously-covered patterns (`PortedPatternsTest` pins the full ported set at root + nested). - ✅ New coverage: `package-lock.json`, `.husky/pre-commit`, `tools/action.yml` each bail. - ✅ Bail semantics unchanged — a denied patch still `bail`s → lands in the ledger and files a `groom` issue; nothing dropped. - ✅ No change to the `builder: false` default; no caller touched. ## Judgment calls - **`|| true` kept on the assignment.** The original wrapped the `grep` in `|| true` (grep exits 1 on no-match, the common case). `patch_policy.py` exits 0 always, so `|| true` now only masks a genuine `git`/`python3` failure — preserving the original posture that this line never aborts the step under `set -euo pipefail`. Dropping it would instead abort the matrix leg on any error, which (per the step's own secret-scan comment) drops the finding entirely rather than filing it. The script is unit-tested and SHA-pinned via `workflows_ref`, so a broken script is caught by `test-groom-scripts.yml` before merge. Flagged rather than silently changed. - **`.github/(workflows|actions)/` stays root-anchored** (verbatim port), while newly-added `.husky/` and `gradle/wrapper/` match at any depth per the ticket — documented inline. Over-blocking is the safe direction, but the ported pattern is kept byte-for-byte to guarantee no regression on the previously-covered set. ## Negative-claim falsification Not applicable in the #581 sense: this diff does not deny a product capability or add a "not supported"/`STOP` dead-end. "Denied" here means a builder patch is downgraded from an auto-PR to a **filed issue** — the finding is still delivered and the human authors the privileged change. Nothing is dropped. The deny behavior is empirically validated by the end-to-end pipeline simulation above, not merely asserted by self-authored tests. BE-4404 --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:36:02 +00:00