[PR #83] [MERGED] feat(groom): `path` input scopes an audit to one directory, without resetting the cadence clock (BE-4757) #95

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-4757-groom-path-scoping

📝 Commits (9)

5e46c7d feat(groom): path input scopes an audit to one directory, without resetting the cadence clock (BE-4757)
f297fcc fix(groom): per-scope cadence clock, site traversal, and scope-brief accuracy (BE-4757)
fbcf3d0 fix(groom): enforce path scope after the verifier + close four scoping holes (BE-4757)
99f189e fix(groom): submodule + line-separator + byte-cap + verdict-enum scope hardening (BE-4757)
a8cddb7 Merge remote-tracking branch 'origin/main' into matt/be-4757-groom-path-scoping
5f7234b fix: resolve review feedback
e488b6a Merge origin/main into matt/be-4757-groom-path-scoping
9fb8594 Merge origin/main into matt/be-4757-groom-path-scoping
bff4947 fix(groom): update scoped-cadence test for evidence-based audit detection

📊 Changes

10 files changed (+2193 additions, -88 deletions)

View changed files

📝 .github/groom/README.md (+20 -12)
📝 .github/groom/interval.py (+116 -29)
➕ .github/groom/scope.py (+770 -0)
📝 .github/groom/tests/test_interval.py (+138 -3)
➕ .github/groom/tests/test_scope.py (+819 -0)
📝 .github/groom/verifier.md (+7 -3)
📝 .github/workflows/groom.yml (+292 -21)
📝 .github/workflows/test-groom-scripts.yml (+21 -14)
📝 AGENTS.md (+9 -5)
📝 README.md (+1 -1)

📄 Description

ELI-5

Groom currently audits your whole repo or nothing. This adds a path input so you can say "just look at services/api this time" — and, importantly, makes that actually true rather than a polite request in a prompt. It also makes sure a small scoped run doesn't trick groom into thinking it already did the big whole-repo sweep this week.

What changed

New input path (type: string, required: false, default: ''). Empty is today's whole-repo behavior; all 11 live callers pass nothing and are unaffected. Exposed as a normal input, not gated on github.event_name, so a monorepo caller can pin a permanently scoped run the way the studio fleet already grooms Comfy-Org/cloud's two subtrees as independent units.

It constrains, it does not instruct. scope_label/scope_desc are prompt substitutions — setting scope_desc: "only services/api" today asks and enforces nothing. Layered enforcement in new .github/groom/scope.py:

Syntactic validation in the cheap gate job, before any billed agent: absolute paths, .. components, empty components, backslashes, control chars and anything outside a conservative [A-Za-z0-9._-] per-component charset are rejected. A dotted directory name (services/my..svc) is legitimate and is accepted — only a .. component is dangerous.
Filesystem containment in the finder job (the first job with the target checked out): both sides normalized through os.path.realpath (the Python cd && pwd -P) before the prefix compare, so a symlinked component can't escape and a trailing separator can't report a false escape. Also proves the directory exists, so a typo'd dispatch fails loudly instead of auditing an empty file list and reporting "clean".
A concrete in-scope file list appended to the finder brief (git ls-files), truncated at 1500 with the truncation announced.
Post-filter of findings whose evidence sites fall outside the scope, with the dropped count and each dropped title/sites logged as ::warning::. Runs on the finder output because that is where sites live — which also avoids paying the verifier to re-check findings that would be dropped anyway.

The checkout stays full on purpose: a refactor in services/api legitimately references common/, and a sparse checkout would blind the finder to the context that makes a finding correct.

The cadence clock — the headline requirement. workflow_dispatch bypasses the interval gate by design, so a scoped run does reach the finder. Without a fix, a manual path: services/api run would become the repo's "last real groom" and suppress the next scheduled whole-repo tick for a full GROOM_INTERVAL_DAYS — a partial audit stamping "done" over the full one.

I picked option (a), distinguishing the job name, over (b) run-name. Reason: run-name is a property of the caller workflow file, not the called reusable — a run created by ci-groom.yml takes ci-groom.yml's run-name, and the reusable's is ignored. Option (b) would therefore require editing all 11 callers to teach each one about path, which directly contradicts "existing callers unaffected". Option (a) is implementable entirely inside the reusable. The coupling the ticket flags is real, so I made it explicit and pinned on both sides: groom.yml renames the finder job Audit — finder (scoped) when path is set, interval.py grew _SCOPED_JOB_MARKER with a comment on each side saying "keep in sync", and ScopedRunDoesNotResetCadence in test_interval.py asserts it end-to-end through the real evaluate(), with a control case proving an unscoped run in the same fixture does suppress the tick (so the assertion can't pass because the gate is broken).

Dedup stays content-derived. verifier.md's signature template moved from {{REPO_BASENAME}}:{{SCOPE_LABEL}}:<slug> to {{REPO_BASENAME}}:{{SIG_SCOPE}}:<slug>, and SIG_SCOPE is wired to the caller's raw scope_label, never the path-derived one. So a scoped run and a whole-repo run produce the same signature for the same defect and suppress each other — and, just as important, already-filed issues (whose markers carry whole-repo) keep deduping, which a straight "drop the scope component" rewrite would have broken by re-filing every open finding once.

Filed issues name their scope. When path is set and scope_label/scope_desc are still at their documented defaults, they're derived (services/api, "the services/api directory"). An explicit caller value always wins.

Tests

python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' — 125 tests, green (97 before). New test_scope.py mirrors scan-path-scoping-test.sh's structure and names the same four invariants in the order they'd hurt. actionlint clean on both touched workflows. .github/workflows/groom.yml added to test-groom-scripts.yml's path filter, since test_scope.py now asserts against the workflow's own SIG_SCOPE wiring.

Judgment calls

Keep-if-ANY-site-in-scope, not keep-only-if-ALL. A duplication finding spanning services/api/a.go and common/x.go is kept. Requiring every site in scope would suppress exactly the cross-cutting findings the full checkout was deliberately kept to enable. What gets dropped is a finding whose evidence lies entirely outside the scope (including one with no locatable sites, which can't be attributed). Called out because the acceptance criterion is compatible with either reading.
"Explicit override" is detected as "not equal to the documented default." An Actions reusable cannot distinguish "input omitted" from "input supplied with the default value"; this is the only available proxy and it is stated in the input descriptions and in scope.py.
scope_label/scope_desc now have whitespace runs collapsed before reaching $GITHUB_OUTPUT, where an embedded newline would truncate the value and inject a bogus output key. YAML >- inputs already fold to one line, so this is byte-identical for every real caller.
Builder scoping is advisory, not enforced. A scoped finding's fix legitimately needs a call-site update outside the directory to compile, so hard-rejecting an out-of-scope hunk would ship a broken patch. pr_size_limit and human review remain the backstops on patch breadth.
Collision direction on (scoped) is deliberate. A caller whose own job name contained (scoped) would make a real whole-repo run stop counting — i.e. groom runs more often. Same fail-open bias as every other branch in interval.py; never the silent under-run.

Unmet criterion

AC 7's docs/callers/groom.md row is NOT in this PR — that guide does not exist on main; it lands in #80 (docs/caller-setup-guides-and-public-hygiene, still open). The ticket sanctions either "rebase on it or add the row when merging", and I chose not to stack: stacking would block this behind a docs PR for one table row. Documentation here instead goes to the three places that do exist on main — the path input's own description (which covers the cadence interaction that AC 7 wants in the footguns section), the caller-pattern block in groom.yml's header, the groom.yml row in README.md, and AGENTS.md's layout section. Whichever of #80 and this PR merges second should add the path row + the cadence footgun to docs/callers/groom.md.

Not applicable: negative-claim falsification

This change adds a capability rather than denying one. The only deny paths it introduces (UnsafePathError on /etc, ../../etc, a non-existent directory) reject malformed input, not a product capability a user could reach another way — and each is paired with a positive test proving the legitimate neighbouring form (services/agent/internal/api, services/my..svc) is accepted, which is the anti-over-rejection half.

_{🔄 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/83 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 7/27/2026 **Status:** ✅ Merged **Merged:** 8/11/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-4757-groom-path-scoping` --- ### 📝 Commits (9) - [`5e46c7d`](https://github.com/Comfy-Org/github-workflows/commit/5e46c7dc77b402010aa6bbf7500c12216c83c335) feat(groom): `path` input scopes an audit to one directory, without resetting the cadence clock (BE-4757) - [`f297fcc`](https://github.com/Comfy-Org/github-workflows/commit/f297fcc9f53e5f7c6527d6094a385a815f7c7516) fix(groom): per-scope cadence clock, site traversal, and scope-brief accuracy (BE-4757) - [`fbcf3d0`](https://github.com/Comfy-Org/github-workflows/commit/fbcf3d014f01ce4ecc046168885b0dcf63a4477b) fix(groom): enforce path scope after the verifier + close four scoping holes (BE-4757) - [`99f189e`](https://github.com/Comfy-Org/github-workflows/commit/99f189e5111ce36942fb026104d96b9d17cd689b) fix(groom): submodule + line-separator + byte-cap + verdict-enum scope hardening (BE-4757) - [`a8cddb7`](https://github.com/Comfy-Org/github-workflows/commit/a8cddb742cf3c3784027c38d7a3b571b344de455) Merge remote-tracking branch 'origin/main' into matt/be-4757-groom-path-scoping - [`5f7234b`](https://github.com/Comfy-Org/github-workflows/commit/5f7234b70cc8a67e26b75f8aaed210736837c421) fix: resolve review feedback - [`e488b6a`](https://github.com/Comfy-Org/github-workflows/commit/e488b6a0267b03fa91da364dae941a358608a729) Merge origin/main into matt/be-4757-groom-path-scoping - [`9fb8594`](https://github.com/Comfy-Org/github-workflows/commit/9fb8594b5917832f9509a85c535e01fd306b46bd) Merge origin/main into matt/be-4757-groom-path-scoping - [`bff4947`](https://github.com/Comfy-Org/github-workflows/commit/bff4947aa177f0d7f571b535ea2c1ffecaf493c8) fix(groom): update scoped-cadence test for evidence-based audit detection ### 📊 Changes **10 files changed** (+2193 additions, -88 deletions) <details> <summary>View changed files</summary> 📝 `.github/groom/README.md` (+20 -12) 📝 `.github/groom/interval.py` (+116 -29) ➕ `.github/groom/scope.py` (+770 -0) 📝 `.github/groom/tests/test_interval.py` (+138 -3) ➕ `.github/groom/tests/test_scope.py` (+819 -0) 📝 `.github/groom/verifier.md` (+7 -3) 📝 `.github/workflows/groom.yml` (+292 -21) 📝 `.github/workflows/test-groom-scripts.yml` (+21 -14) 📝 `AGENTS.md` (+9 -5) 📝 `README.md` (+1 -1) </details> ### 📄 Description ## ELI-5 Groom currently audits your **whole repo** or nothing. This adds a `path` input so you can say "just look at `services/api` this time" — and, importantly, makes that actually *true* rather than a polite request in a prompt. It also makes sure a small scoped run doesn't trick groom into thinking it already did the big whole-repo sweep this week. ## What changed **New input `path`** (`type: string`, `required: false`, `default: ''`). Empty is today's whole-repo behavior; all 11 live callers pass nothing and are unaffected. Exposed as a normal input, not gated on `github.event_name`, so a monorepo caller can pin a permanently scoped run the way the studio fleet already grooms `Comfy-Org/cloud`'s two subtrees as independent units. **It constrains, it does not instruct.** `scope_label`/`scope_desc` are prompt substitutions — setting `scope_desc: "only services/api"` today *asks* and enforces nothing. Layered enforcement in new `.github/groom/scope.py`: - **Syntactic validation** in the cheap `gate` job, before any billed agent: absolute paths, `..` **components**, empty components, backslashes, control chars and anything outside a conservative `[A-Za-z0-9._-]` per-component charset are rejected. A dotted directory name (`services/my..svc`) is legitimate and is **accepted** — only a `..` *component* is dangerous. - **Filesystem containment** in the finder job (the first job with the target checked out): both sides normalized through `os.path.realpath` (the Python `cd && pwd -P`) before the prefix compare, so a symlinked component can't escape and a trailing separator can't report a *false* escape. Also proves the directory exists, so a typo'd dispatch fails loudly instead of auditing an empty file list and reporting "clean". - **A concrete in-scope file list** appended to the finder brief (`git ls-files`), truncated at 1500 with the truncation announced. - **Post-filter** of findings whose evidence sites fall outside the scope, with the dropped count and each dropped title/sites logged as `::warning::`. Runs on the *finder* output because that is where `sites` live — which also avoids paying the verifier to re-check findings that would be dropped anyway. The **checkout stays full** on purpose: a refactor in `services/api` legitimately references `common/`, and a sparse checkout would blind the finder to the context that makes a finding correct. **The cadence clock — the headline requirement.** `workflow_dispatch` bypasses the interval gate by design, so a scoped run *does* reach the finder. Without a fix, a manual `path: services/api` run would become the repo's "last real groom" and suppress the next scheduled whole-repo tick for a full `GROOM_INTERVAL_DAYS` — a partial audit stamping "done" over the full one. I picked **option (a), distinguishing the job name**, over (b) `run-name`. Reason: `run-name` is a property of the **caller** workflow file, not the called reusable — a run created by `ci-groom.yml` takes `ci-groom.yml`'s `run-name`, and the reusable's is ignored. Option (b) would therefore require editing all 11 callers to teach each one about `path`, which directly contradicts "existing callers unaffected". Option (a) is implementable entirely inside the reusable. The coupling the ticket flags is real, so I made it explicit and pinned on both sides: `groom.yml` renames the finder job `Audit — finder (scoped)` when `path` is set, `interval.py` grew `_SCOPED_JOB_MARKER` with a comment on each side saying "keep in sync", and `ScopedRunDoesNotResetCadence` in `test_interval.py` asserts it end-to-end through the real `evaluate()`, with a control case proving an *unscoped* run in the same fixture *does* suppress the tick (so the assertion can't pass because the gate is broken). **Dedup stays content-derived.** `verifier.md`'s signature template moved from `{{REPO_BASENAME}}:{{SCOPE_LABEL}}:<slug>` to `{{REPO_BASENAME}}:{{SIG_SCOPE}}:<slug>`, and `SIG_SCOPE` is wired to the caller's **raw** `scope_label`, never the path-derived one. So a scoped run and a whole-repo run produce the *same* signature for the same defect and suppress each other — and, just as important, already-filed issues (whose markers carry `whole-repo`) keep deduping, which a straight "drop the scope component" rewrite would have broken by re-filing every open finding once. **Filed issues name their scope.** When `path` is set and `scope_label`/`scope_desc` are still at their documented defaults, they're derived (`services/api`, "the `services/api` directory"). An explicit caller value always wins. ## Tests `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'` — **125 tests, green** (97 before). New `test_scope.py` mirrors `scan-path-scoping-test.sh`'s structure and names the same four invariants in the order they'd hurt. `actionlint` clean on both touched workflows. `.github/workflows/groom.yml` added to `test-groom-scripts.yml`'s path filter, since `test_scope.py` now asserts against the workflow's own `SIG_SCOPE` wiring. ## Judgment calls - **Keep-if-ANY-site-in-scope, not keep-only-if-ALL.** A duplication finding spanning `services/api/a.go` and `common/x.go` is kept. Requiring every site in scope would suppress exactly the cross-cutting findings the full checkout was deliberately kept to enable. What gets dropped is a finding whose evidence lies *entirely* outside the scope (including one with no locatable sites, which can't be attributed). Called out because the acceptance criterion is compatible with either reading. - **"Explicit override" is detected as "not equal to the documented default."** An Actions reusable cannot distinguish "input omitted" from "input supplied with the default value"; this is the only available proxy and it is stated in the input descriptions and in `scope.py`. - **`scope_label`/`scope_desc` now have whitespace runs collapsed** before reaching `$GITHUB_OUTPUT`, where an embedded newline would truncate the value and inject a bogus output key. YAML `>-` inputs already fold to one line, so this is byte-identical for every real caller. - **Builder scoping is advisory, not enforced.** A scoped finding's fix legitimately needs a call-site update outside the directory to compile, so hard-rejecting an out-of-scope hunk would ship a broken patch. `pr_size_limit` and human review remain the backstops on patch breadth. - **Collision direction on `(scoped)` is deliberate.** A caller whose own job name contained `(scoped)` would make a real whole-repo run stop counting — i.e. groom runs *more* often. Same fail-open bias as every other branch in `interval.py`; never the silent under-run. ## Unmet criterion **AC 7's `docs/callers/groom.md` row is NOT in this PR** — that guide does not exist on `main`; it lands in #80 (`docs/caller-setup-guides-and-public-hygiene`, still open). The ticket sanctions either "rebase on it **or** add the row when merging", and I chose not to stack: stacking would block this behind a docs PR for one table row. Documentation here instead goes to the three places that *do* exist on `main` — the `path` input's own description (which covers the cadence interaction that AC 7 wants in the footguns section), the caller-pattern block in `groom.yml`'s header, the `groom.yml` row in `README.md`, and `AGENTS.md`'s layout section. **Whichever of #80 and this PR merges second should add the `path` row + the cadence footgun to `docs/callers/groom.md`.** ## Not applicable: negative-claim falsification This change *adds* a capability rather than denying one. The only deny paths it introduces (`UnsafePathError` on `/etc`, `../../etc`, a non-existent directory) reject **malformed input**, not a product capability a user could reach another way — and each is paired with a positive test proving the legitimate neighbouring form (`services/agent/internal/api`, `services/my..svc`) is accepted, which is the anti-over-rejection half. --- <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:10 +00:00