.y*ml in the builder patch policy (BE-9612) #247

New issue

Closed

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

bsakaguchi commented

2026-10-26 22:37:44 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/239
Author: @mattmillerai
Created: 8/27/2026
Status: ✅ Merged
Merged: 8/28/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-9612-groom-deny-dataset-of-record-paths

📝 Commits (4)

916d87c feat(groom): deny suites//cases/.y*ml dataset-of-record paths in the builder patch policy (BE-9612)
01d9e71 fix(groom): close dataset-of-record deny bypasses from review (BE-9612)
32eb35d fix(groom): deny suites-tree symlinks by mode; widen to zero mid segments; --no-renames on numstat (BE-9612)
31ced91 test(groom): pin the rename-record fail-closed invariant in parse_raw_z (BE-9612)

📊 Changes

4 files changed (+373 additions, -47 deletions)

View changed files

📝 .github/groom/README.md (+35 -5)
📝 .github/groom/patch_policy.py (+149 -22)
📝 .github/groom/tests/test_patch_policy.py (+159 -10)
📝 .github/workflows/groom.yml (+30 -10)

📄 Description

ELI-5

Groom's opt-in auto-builder can write a patch against any tracked file, and the only thing standing between that patch and an auto-opened PR is a deny-list of paths a human must author. Until now that list only covered CI-privileged paths (workflows, lockfiles, build config). One consumer repo just turned the builder on, and its crown jewels are graded eval case files under a suites/**/cases/ tree — files whose merge publishes immutable versions and whose meaning is reserved for the dataset owner. This teaches the deny-list about that family: a builder patch touching those paths is downgraded to a filed issue, so a human authors the change. Nothing is dropped — the finding still gets filed.

What is here

.github/groom/patch_policy.py — a separately named _DATASET_OF_RECORD_REGEXES tuple folded into _PATTERN with the same segment-anchored, $-terminated, IGNORECASE wrapping as _PATH_SUFFIXES:
- suites/(?:[^/]+/)+cases/(?s:.*)\.ya?ml — case files, any depth above and below cases/, empty stems and newline-bearing names included.
- suites/(?:[^/]+/)+cases — a change whose final segment is cases under a suite. git tracks no directories, so that path shape is a file or a symlink: the indirection that would otherwise point a globbing importer at an undenied tree.
- _is_denied now tests the unsplit path as well as each newline-split line, so a match may straddle a raw newline (the line-split alone cannot satisfy any $-anchored branch across one). Strictly additive.
- Module and denied_paths docstrings no longer promise "CI-privileged" for a list that now returns two classes.
.github/workflows/groom.yml — --no-renames on the policy producer. With git's default rename detection, diff --cached --name-only reports only a rename's DESTINATION, so a patch that moved a case out of cases/ — the retirement this class exists to gate — showed the policy an empty list and would have auto-opened a PR. Plus the bail message, the ::warning line, and the header comment naming both families.
.github/groom/tests/test_patch_policy.py — DatasetOfRecordTest: the shape matrix, a no-bypass test (grouped/versioned layouts, empty stem, embedded newline), and the symlink-shape test. Fixtures are deliberately generic (s1, s2) — public repo. Review round added test_parse_raw_z_rejects_rename_records, pinning invariant 2's documented fail-closed handling of a two-path R/C record — the tripwire for a future editor who drops --no-renames.
.github/groom/README.md — the family, the --no-renames requirement, and an explicit statement of this entry's asymmetry: it is the one entry with no CI-execution justification, it is hardcoded rather than caller-gated, and an unrelated suites/*/cases/ fixture tree inherits it with no opt-out.

Verification

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v — 374 tests, all green (1 pre-existing skip: the Linux-only sandbox suite). The other two suites are green too: cursor-review 369, agents-md-integrity 46.
--no-renames reproduced end-to-end on a scratch repo (git 2.54): git mv suites/s1/cases/c1.yaml suites/s1/retired-c1.yaml → before the fix the policy sees nothing (PR would be auto-opened); after, it reports suites/s1/cases/c1.yaml and the patch is denied.
End-to-end against the live target: fed the motivating consumer's full tracked-file tree through denied_paths — every case file denied, zero non-case suites/ files denied (drivers, fixtures, READMEs all pass through), and every other denied path a pre-existing CI-privileged hit. No behaviour change for callers without a suites/**/cases/ tree; all pre-existing patch-policy tests pass unchanged.
ReDoS probe on the new nested quantifier: each inner [^/]+ cannot cross /, so the segment partition is unique and there is nothing to backtrack over — verified sub-millisecond on 9k-char pathological near-miss paths.
check_workflow_pins.py OK; check_agents_md.py --root . passed (1 pre-existing CODEOWNERS warning); groom.yml parses.

Judgment calls

The ::warning echo next to the bail still said "CI-privileged file(s)", misleading on a dataset-of-record bail; updated to name both families. One adjacent line beyond the ticket's enumerated edits, flagged rather than shipped silently.
The ticket's suggested comment text named the specific private consumer repo, its importer workflow, and its conventions file. This repo's top guardrail keeps private caller detail out of workflow files, commits and PR text, so the comments describe the mechanism generically. Review caught a residue of this in the test fixtures (real suite names, a real-looking driver filename) — now generic.
Kept the class segment-anchored rather than root-anchored. Root-anchoring would remove the tests/suites/*/cases/ collateral, but it under-blocks any caller whose dataset tree is nested, and invariant 1 is explicit that under-blocking is the hole while over-blocking only downgrades a PR to an issue.
denied_paths still returns the two classes undifferentiated. With one consumer that tests only non-emptiness, a richer return type is unexercised API surface; the docstring now states the constraint instead.

Provenance

Authored by: agent-work loop
Verified: groom suite 374 passed / 0 failed / 1 pre-existing skip; cursor-review 369 and agents-md-integrity 46 both green; --no-renames regression reproduced and fixed on a scratch repo (denied path invisible before, reported after); live-tree sweep of the motivating consumer repo — all case files denied, zero false positives; ReDoS probe to 9k-char paths sub-millisecond; check_workflow_pins.py OK; check_agents_md.py passed (1 pre-existing warning); groom.yml parses. Review round: the new rename-record test verified by mutation — relaxing either raise in parse_raw_z fails it.
Deviations: code comments and test fixtures genericized instead of naming the private consumer repo the ticket's verbatim text named (public-repo guardrail); one adjacent ::warning line updated beyond the ticket's enumerated edits, for message consistency; the review pass widened the regex beyond the ticket's literal suites/*/cases/*.y*ml (any depth above and below cases/, empty stems, embedded newlines, the cases-as-symlink shape) and added --no-renames to the producer — all in the over-block direction invariant 1 mandates.

Residual

The driving ticket's evidence base (spike BE-9609) lives in a Linear findings comment this worker could not read (no Linear access); the implementation was instead re-verified independently against the live consumer dataset (see Verification), so nothing here depends on the unread comment — but if that spike's findings name additional dataset-of-record families beyond the suites/**/cases/ shape, they are not covered by this PR.
Making the deny-list caller-extensible (e.g. a builder_deny_paths input, with an opt-out for this class) is the known open item — raised by 2 of 8 review models and recorded as a follow-up. It is a new public input on a shared reusable workflow with a live caller fleet, so it needs a human decision on naming and default semantics before it can be built.
The gate remains path-shaped: a symlink placed one level above cases/ is not covered. Closing that class means denying on git mode 120000 rather than on path shape — a different mechanism, no reachable path today.
CODEOWNERS / branch-protection hardening in the consumer repo that owns the dataset — human calls, tracked in the BE-9609 findings.

_{🔄 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/239 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/27/2026 **Status:** ✅ Merged **Merged:** 8/28/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9612-groom-deny-dataset-of-record-paths` --- ### 📝 Commits (4) - [`916d87c`](https://github.com/Comfy-Org/github-workflows/commit/916d87cdb0f4f34e499f846204f3c86e187080f9) feat(groom): deny suites/*/cases/*.y*ml dataset-of-record paths in the builder patch policy (BE-9612) - [`01d9e71`](https://github.com/Comfy-Org/github-workflows/commit/01d9e71c4df01854b7a2b0cf947448f71d65241d) fix(groom): close dataset-of-record deny bypasses from review (BE-9612) - [`32eb35d`](https://github.com/Comfy-Org/github-workflows/commit/32eb35d85ff7548c171c6e5c4a8ccf2c47becb6c) fix(groom): deny suites-tree symlinks by mode; widen to zero mid segments; --no-renames on numstat (BE-9612) - [`31ced91`](https://github.com/Comfy-Org/github-workflows/commit/31ced912546d3be5e9334a4e7032b4cd6e3b98a6) test(groom): pin the rename-record fail-closed invariant in parse_raw_z (BE-9612) ### 📊 Changes **4 files changed** (+373 additions, -47 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+35 -5) 📝 `.github/groom/patch_policy.py` (+149 -22) 📝 `.github/groom/tests/test_patch_policy.py` (+159 -10) 📝 `.github/workflows/groom.yml` (+30 -10) </details> ### 📄 Description ## ELI-5 Groom's opt-in auto-builder can write a patch against any tracked file, and the only thing standing between that patch and an auto-opened PR is a deny-list of paths a human must author. Until now that list only covered CI-privileged paths (workflows, lockfiles, build config). One consumer repo just turned the builder on, and its crown jewels are graded eval case files under a `suites/**/cases/` tree — files whose merge publishes immutable versions and whose meaning is reserved for the dataset owner. This teaches the deny-list about that family: a builder patch touching those paths is downgraded to a filed issue, so a human authors the change. Nothing is dropped — the finding still gets filed. ## What is here - `.github/groom/patch_policy.py` — a separately named `_DATASET_OF_RECORD_REGEXES` tuple folded into `_PATTERN` with the same segment-anchored, `$`-terminated, IGNORECASE wrapping as `_PATH_SUFFIXES`: - `suites/(?:[^/]+/)+cases/(?s:.*)\.ya?ml` — case files, any depth above **and** below `cases/`, empty stems and newline-bearing names included. - `suites/(?:[^/]+/)+cases` — a change whose final segment is `cases` under a suite. git tracks no directories, so that path shape is a file or a **symlink**: the indirection that would otherwise point a globbing importer at an undenied tree. - `_is_denied` now tests the **unsplit** path as well as each newline-split line, so a match may straddle a raw newline (the line-split alone cannot satisfy any `$`-anchored branch across one). Strictly additive. - Module and `denied_paths` docstrings no longer promise "CI-privileged" for a list that now returns two classes. - `.github/workflows/groom.yml` — **`--no-renames` on the policy producer.** With git's default rename detection, `diff --cached --name-only` reports only a rename's DESTINATION, so a patch that *moved* a case out of `cases/` — the retirement this class exists to gate — showed the policy an empty list and would have auto-opened a PR. Plus the bail message, the `::warning` line, and the header comment naming both families. - `.github/groom/tests/test_patch_policy.py` — `DatasetOfRecordTest`: the shape matrix, a no-bypass test (grouped/versioned layouts, empty stem, embedded newline), and the symlink-shape test. Fixtures are deliberately generic (`s1`, `s2`) — public repo. Review round added `test_parse_raw_z_rejects_rename_records`, pinning invariant 2's documented fail-closed handling of a two-path R/C record — the tripwire for a future editor who drops `--no-renames`. - `.github/groom/README.md` — the family, the `--no-renames` requirement, and an explicit statement of this entry's asymmetry: it is the one entry with no CI-execution justification, it is hardcoded rather than caller-gated, and an unrelated `suites/*/cases/` fixture tree inherits it with no opt-out. ## Verification - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v` — **374 tests, all green** (1 pre-existing skip: the Linux-only sandbox suite). The other two suites are green too: cursor-review 369, agents-md-integrity 46. - **`--no-renames` reproduced end-to-end** on a scratch repo (git 2.54): `git mv suites/s1/cases/c1.yaml suites/s1/retired-c1.yaml` → before the fix the policy sees nothing (PR would be auto-opened); after, it reports `suites/s1/cases/c1.yaml` and the patch is denied. - **End-to-end against the live target:** fed the motivating consumer's full tracked-file tree through `denied_paths` — every case file denied, **zero** non-case `suites/` files denied (drivers, fixtures, READMEs all pass through), and every other denied path a pre-existing CI-privileged hit. No behaviour change for callers without a `suites/**/cases/` tree; all pre-existing patch-policy tests pass unchanged. - **ReDoS probe** on the new nested quantifier: each inner `[^/]+` cannot cross `/`, so the segment partition is unique and there is nothing to backtrack over — verified sub-millisecond on 9k-char pathological near-miss paths. - `check_workflow_pins.py` OK; `check_agents_md.py --root .` passed (1 pre-existing CODEOWNERS warning); `groom.yml` parses. ## Judgment calls - The `::warning` echo next to the bail still said "CI-privileged file(s)", misleading on a dataset-of-record bail; updated to name both families. One adjacent line beyond the ticket's enumerated edits, flagged rather than shipped silently. - The ticket's suggested comment text named the specific private consumer repo, its importer workflow, and its conventions file. This repo's top guardrail keeps private caller detail out of workflow files, commits and PR text, so the comments describe the mechanism generically. Review caught a residue of this in the test fixtures (real suite names, a real-looking driver filename) — now generic. - Kept the class **segment-anchored** rather than root-anchored. Root-anchoring would remove the `tests/suites/*/cases/` collateral, but it under-blocks any caller whose dataset tree is nested, and invariant 1 is explicit that under-blocking is the hole while over-blocking only downgrades a PR to an issue. - `denied_paths` still returns the two classes undifferentiated. With one consumer that tests only non-emptiness, a richer return type is unexercised API surface; the docstring now states the constraint instead. ## Provenance - **Authored by:** agent-work loop - **Verified:** groom suite 374 passed / 0 failed / 1 pre-existing skip; cursor-review 369 and agents-md-integrity 46 both green; `--no-renames` regression reproduced and fixed on a scratch repo (denied path invisible before, reported after); live-tree sweep of the motivating consumer repo — all case files denied, zero false positives; ReDoS probe to 9k-char paths sub-millisecond; `check_workflow_pins.py` OK; `check_agents_md.py` passed (1 pre-existing warning); `groom.yml` parses. Review round: the new rename-record test verified by mutation — relaxing either `raise` in `parse_raw_z` fails it. - **Deviations:** code comments and test fixtures genericized instead of naming the private consumer repo the ticket's verbatim text named (public-repo guardrail); one adjacent `::warning` line updated beyond the ticket's enumerated edits, for message consistency; the review pass widened the regex beyond the ticket's literal `suites/*/cases/*.y*ml` (any depth above and below `cases/`, empty stems, embedded newlines, the `cases`-as-symlink shape) and added `--no-renames` to the producer — all in the over-block direction invariant 1 mandates. ## Residual - The driving ticket's evidence base (spike BE-9609) lives in a Linear findings comment this worker could not read (no Linear access); the implementation was instead re-verified independently against the live consumer dataset (see Verification), so nothing here depends on the unread comment — but if that spike's findings name additional dataset-of-record families beyond the `suites/**/cases/` shape, they are not covered by this PR. - **Making the deny-list caller-extensible** (e.g. a `builder_deny_paths` input, with an opt-out for this class) is the known open item — raised by 2 of 8 review models and recorded as a follow-up. It is a new public input on a shared reusable workflow with a live caller fleet, so it needs a human decision on naming and default semantics before it can be built. - The gate remains path-shaped: a symlink placed one level *above* `cases/` is not covered. Closing that class means denying on git mode 120000 rather than on path shape — a different mechanism, no reachable path today. - CODEOWNERS / branch-protection hardening in the consumer repo that owns the dataset — human calls, tracked in the BE-9609 findings. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:44 +00:00