[PR #106] [MERGED] feat(agents-md-integrity): add exclude_paths for payload subtrees (BE-6009) #117

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-6009-agents-md-exclude-paths

📝 Commits (2)

f465dc0 feat(agents-md-integrity): add exclude_paths for payload subtrees (BE-6009)
825c487 fix(agents-md-integrity): harden --exclude glob semantics from review (BE-6009)

📊 Changes

6 files changed (+724 additions, -19 deletions)

View changed files

📝 .github/agents-md-integrity/README.md (+49 -1)
📝 .github/agents-md-integrity/check_agents_md.py (+270 -16)
📝 .github/agents-md-integrity/tests/test_check_agents_md.py (+311 -0)
📝 .github/workflows/agents-md-integrity.yml (+40 -1)
📝 .github/workflows/test-agents-md-integrity.yml (+53 -0)
📝 README.md (+1 -1)

📄 Description

ELI-5

The AGENTS.md checker has a rule: "every AGENTS.md in a subfolder needs a one-line CLAUDE.md next to it that just says @AGENTS.md." That's right for a monorepo. It's wrong for a repo whose product is agent instructions — a plugin marketplace publishes plugins/<name>/AGENTS.md next to a real, multi-line CLAUDE.md as the thing it ships to consumers, and rewriting that sibling into a shim would corrupt the published plugin. Until now the only escape was check_nested: false, which turns the rule off for the entire repo. This PR adds a scalpel: exclude_paths: plugins/** carves out just that subtree, prints exactly what it skipped, and refuses to let anyone exclude the root files.

What changed

.github/workflows/agents-md-integrity.yml gains an optional exclude_paths input (newline- or comma-separated globs, default empty), plumbed to check_agents_md.py as a repeatable/CSV --exclude flag. Per the ticket's four constraints:

Walk-time, not post-filter. Exclusions prune directories inside os.walk and skip directly-matched files before they're read, so an excluded subtree is never opened or line-counted (test_excluded_subtree_is_never_line_counted puts a 500-line AGENTS.md in an excluded dir and asserts the ceiling never fires).
Additive, never a replacement. The hardcoded SKIP_DIRS baseline still applies alongside --exclude.
Always visible. Each exclusion prints EXCLUDED: <path> (matched <glob>) plus a ::notice:: annotation, followed by a count line; the configured globs are echoed too, so a typo'd glob that matches nothing still leaves a trace. An exclusion nobody can see in the log is the failure mode this ticket exists to prevent.
Root is not excludable. A glob matching the root agents file or CLAUDE.md (**, *, *.md, AGENTS.md, **/CLAUDE.md, /AGENTS.md, …) raises ExcludeConfigError and exits 2 (0 = pass, 1 = a check failed). Validated unconditionally — even with check_nested: false, where the globs would never be consulted.

Glob semantics are deliberately narrower than the CODEOWNERS translation already in the file: always repo-root-relative (no match-the-basename-at-any-depth magic, which is how a glob silently deletes coverage nobody asked to drop), */? stay inside a path segment, ** crosses segments, a leading **/ means "at any depth", and a glob matching a directory excludes the subtree beneath it.

Also documented in the reusable's header, .github/agents-md-integrity/README.md, and the root README catalog row.

Verification

python3 -m unittest discover -s .github/agents-md-integrity/tests — 40 passed (19 pre-existing, unchanged, plus 21 new).
The other suites, untouched and still green: cursor-review (40), groom (170), bump-callers shellcheck + 123 shell assertions.
Two new CI smoke steps in test-agents-md-integrity.yml — a payload repo that fails without --exclude, passes with plugins/**, and must show the EXCLUDED: line; and a root-excluding glob that must exit 2. Both were run locally verbatim under bash -e.
This repo's own AGENTS.md still passes the checker (exit 0).

Covering the ticket's four required cases: excluded nested pair passes (test_excluded_nested_pair_passes), non-excluded nested pair still fails in the same run (test_non_excluded_nested_pair_still_fails), root exclusion errors out (test_exclusion_targeting_root_errors_out, six globs), and no --exclude reproduces today's behavior exactly (test_no_exclude_reproduces_todays_behavior, plus test_cli_without_exclude_fails_the_same_payload).

Backwards compatibility

exclude_paths defaults to '', and the run step adds --exclude only when the value is non-blank, so every currently-registered caller invokes the identical command line. With an empty glob list the walk takes the same code path it always did.

Two internal changes worth a reviewer's eye: run_checks() now returns a 3-tuple (failures, warnings, exclusions), and _iter_nested_agents() became _scan_nested_agents() returning lists. Both are module-internal — the only callers are main() and the test suite, and the CLI contract is unchanged, which is what external consumers (including the comfy-infra sweep, which shells out with --root) actually depend on.

Judgment calls

One addition beyond the spec: exclusions configured while check_nested is false now emit a warning that they exclude nothing. Both knobs set together is precisely the invisible-coverage-loss shape this ticket exists to replace, and it would otherwise read as "scoped" when nested checking is off repo-wide.
Report granularity: the EXCLUDED: line names the pruned directory (e.g. plugins/comfy-conventions), not each buried file — a file inside a pruned directory is never discovered, which is the point of constraint 3. A repo with 30 plugins therefore logs 30 lines; that visible cost is intentional.
Declined: failing (or warning) on a glob that matches nothing. It would catch typos, but a glob can legitimately match nothing on some branches. Echoing the configured globs gives the same visibility without a new failure mode.
Declined: an EXCLUDE_PATHS env fallback for --exclude, to keep exactly one way to pass exclusions. Every other knob is env-driven; this one is CLI-only, as the ticket specified.
Negative-claim check (self-review clause (e)): this diff adds a capability rather than denying one. The single deny path is the root-exclusion rejection, which is the ticket's own requirement 5 — and it is not a dead-end: the error names the working alternative (narrow the glob), and check_nested / require_shim remain available for the cases they're actually for.

Out of scope here — the ticket's other two repos

The ticket's remaining items land in different repos, so they are not in this PR (this loop is single-repo per PR):

Comfy-Org/comfy-conventions — set exclude_paths: | / plugins/** on its caller once this merges and a SHA is available (unblocks BE-5923).
Comfy-Org/comfy-infra → agents-md-hygiene.yml — the daily org-wide sweep passes only --root, so it will still report comfy-conventions as non-conformant regardless of that repo's own caller. It needs a per-repo path-exclusion map (or to read each caller's exclude_paths) so the gate and the radar agree. This PR provides the mechanism the sweep needs — --exclude is the same flag, on the same pinned checker — but does not wire it up, so acceptance criterion 3 ("the daily sweep reaches the same verdict") is not met by this PR alone. Per BE-3532's amendment, the sweep must reuse this exclusion implementation rather than growing a second one.

_{🔄 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/106 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/2/2026 **Status:** ✅ Merged **Merged:** 8/4/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-6009-agents-md-exclude-paths` --- ### 📝 Commits (2) - [`f465dc0`](https://github.com/Comfy-Org/github-workflows/commit/f465dc0728acc4db8f8ffded6e612b2eeba98cf3) feat(agents-md-integrity): add exclude_paths for payload subtrees (BE-6009) - [`825c487`](https://github.com/Comfy-Org/github-workflows/commit/825c48767a4bff352d626332c50d3f3c5f4795cc) fix(agents-md-integrity): harden --exclude glob semantics from review (BE-6009) ### 📊 Changes **6 files changed** (+724 additions, -19 deletions) <details> <summary>View changed files</summary> 📝 `.github/agents-md-integrity/README.md` (+49 -1) 📝 `.github/agents-md-integrity/check_agents_md.py` (+270 -16) 📝 `.github/agents-md-integrity/tests/test_check_agents_md.py` (+311 -0) 📝 `.github/workflows/agents-md-integrity.yml` (+40 -1) 📝 `.github/workflows/test-agents-md-integrity.yml` (+53 -0) 📝 `README.md` (+1 -1) </details> ### 📄 Description ## ELI-5 The AGENTS.md checker has a rule: "every `AGENTS.md` in a subfolder needs a one-line `CLAUDE.md` next to it that just says `@AGENTS.md`." That's right for a monorepo. It's wrong for a repo whose *product* is agent instructions — a plugin marketplace publishes `plugins/<name>/AGENTS.md` next to a real, multi-line `CLAUDE.md` as the thing it ships to consumers, and rewriting that sibling into a shim would corrupt the published plugin. Until now the only escape was `check_nested: false`, which turns the rule off for the **entire** repo. This PR adds a scalpel: `exclude_paths: plugins/**` carves out just that subtree, prints exactly what it skipped, and refuses to let anyone exclude the root files. ## What changed `.github/workflows/agents-md-integrity.yml` gains an optional `exclude_paths` input (newline- or comma-separated globs, default empty), plumbed to `check_agents_md.py` as a repeatable/CSV `--exclude` flag. Per the ticket's four constraints: 1. **Walk-time, not post-filter.** Exclusions prune directories inside `os.walk` and skip directly-matched files before they're read, so an excluded subtree is never opened or line-counted (`test_excluded_subtree_is_never_line_counted` puts a 500-line `AGENTS.md` in an excluded dir and asserts the ceiling never fires). 2. **Additive, never a replacement.** The hardcoded `SKIP_DIRS` baseline still applies alongside `--exclude`. 3. **Always visible.** Each exclusion prints `EXCLUDED: <path> (matched <glob>)` plus a `::notice::` annotation, followed by a count line; the *configured* globs are echoed too, so a typo'd glob that matches nothing still leaves a trace. An exclusion nobody can see in the log is the failure mode this ticket exists to prevent. 4. **Root is not excludable.** A glob matching the root agents file or `CLAUDE.md` (`**`, `*`, `*.md`, `AGENTS.md`, `**/CLAUDE.md`, `/AGENTS.md`, …) raises `ExcludeConfigError` and exits **2** (0 = pass, 1 = a check failed). Validated unconditionally — even with `check_nested: false`, where the globs would never be consulted. Glob semantics are deliberately narrower than the CODEOWNERS translation already in the file: always repo-root-relative (no match-the-basename-at-any-depth magic, which is how a glob silently deletes coverage nobody asked to drop), `*`/`?` stay inside a path segment, `**` crosses segments, a leading `**/` means "at any depth", and a glob matching a directory excludes the subtree beneath it. Also documented in the reusable's header, `.github/agents-md-integrity/README.md`, and the root README catalog row. ## Verification - `python3 -m unittest discover -s .github/agents-md-integrity/tests` — **40 passed** (19 pre-existing, unchanged, plus 21 new). - The other suites, untouched and still green: cursor-review (40), groom (170), bump-callers shellcheck + 123 shell assertions. - Two new CI smoke steps in `test-agents-md-integrity.yml` — a payload repo that fails without `--exclude`, passes with `plugins/**`, and must show the `EXCLUDED:` line; and a root-excluding glob that must exit 2. Both were run locally verbatim under `bash -e`. - This repo's own `AGENTS.md` still passes the checker (exit 0). Covering the ticket's four required cases: excluded nested pair passes (`test_excluded_nested_pair_passes`), non-excluded nested pair still fails in the *same* run (`test_non_excluded_nested_pair_still_fails`), root exclusion errors out (`test_exclusion_targeting_root_errors_out`, six globs), and no `--exclude` reproduces today's behavior exactly (`test_no_exclude_reproduces_todays_behavior`, plus `test_cli_without_exclude_fails_the_same_payload`). ## Backwards compatibility `exclude_paths` defaults to `''`, and the run step adds `--exclude` **only** when the value is non-blank, so every currently-registered caller invokes the identical command line. With an empty glob list the walk takes the same code path it always did. Two internal changes worth a reviewer's eye: `run_checks()` now returns a **3-tuple** `(failures, warnings, exclusions)`, and `_iter_nested_agents()` became `_scan_nested_agents()` returning lists. Both are module-internal — the only callers are `main()` and the test suite, and the **CLI contract is unchanged**, which is what external consumers (including the comfy-infra sweep, which shells out with `--root`) actually depend on. ## Judgment calls - **One addition beyond the spec:** exclusions configured while `check_nested` is `false` now emit a warning that they exclude nothing. Both knobs set together is precisely the invisible-coverage-loss shape this ticket exists to replace, and it would otherwise read as "scoped" when nested checking is off repo-wide. - **Report granularity:** the `EXCLUDED:` line names the *pruned directory* (e.g. `plugins/comfy-conventions`), not each buried file — a file inside a pruned directory is never discovered, which is the point of constraint 3. A repo with 30 plugins therefore logs 30 lines; that visible cost is intentional. - **Declined:** failing (or warning) on a glob that matches nothing. It would catch typos, but a glob can legitimately match nothing on some branches. Echoing the configured globs gives the same visibility without a new failure mode. - **Declined:** an `EXCLUDE_PATHS` env fallback for `--exclude`, to keep exactly one way to pass exclusions. Every other knob is env-driven; this one is CLI-only, as the ticket specified. - **Negative-claim check (self-review clause (e)):** this diff adds a capability rather than denying one. The single deny path is the root-exclusion rejection, which is the ticket's own requirement 5 — and it is not a dead-end: the error names the working alternative (narrow the glob), and `check_nested` / `require_shim` remain available for the cases they're actually for. ## Out of scope here — the ticket's other two repos The ticket's remaining items land in **different repos**, so they are not in this PR (this loop is single-repo per PR): 1. **`Comfy-Org/comfy-conventions`** — set `exclude_paths: |` / `plugins/**` on its caller once this merges and a SHA is available (unblocks BE-5923). 2. **`Comfy-Org/comfy-infra` → `agents-md-hygiene.yml`** — the daily org-wide sweep passes only `--root`, so it will still report comfy-conventions as non-conformant regardless of that repo's own caller. It needs a per-repo path-exclusion map (or to read each caller's `exclude_paths`) so the gate and the radar agree. **This PR provides the mechanism the sweep needs — `--exclude` is the same flag, on the same pinned checker — but does not wire it up**, so acceptance criterion 3 ("the daily sweep reaches the same verdict") is **not met by this PR alone**. Per BE-3532's amendment, the sweep must reuse this exclusion implementation rather than growing a second one. --- <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:26 +00:00