From f337e059f51dda7faa2ce4d91cd48d08358cb027 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 2 Aug 2026 00:36:17 -0700 Subject: [PATCH 1/2] feat(agents-md-integrity): add exclude_paths for payload subtrees (BE-6009) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The nested-shim rule is right for a monorepo subtree and wrong for a repo whose PRODUCT is agent instructions: a plugin/skill marketplace ships AGENTS.md + a real multi-line CLAUDE.md as distributable payload, and the only escape today is `check_nested: false`, which silently drops nested coverage for the whole repo. Adds an `exclude_paths` workflow input (newline-/comma-separated globs, default empty) plumbed to the checker as a repeatable/CSV `--exclude` flag: - Applied during the discovery walk, not as a post-filter on findings, so an excluded subtree is never opened or line-counted. - Additive on top of the always-on SKIP_DIRS baseline. - Every exclusion is logged as `EXCLUDED: (matched )` plus a `::notice::` annotation, and the configured globs are echoed even when they match nothing — an exclusion with no trace is how coverage rots invisibly. - A glob matching the ROOT agents file or CLAUDE.md is rejected with exit 2. - Exclusions configured while `check_nested` is false now warn that they exclude nothing. With no `exclude_paths` the workflow runs the exact command it ran before. --- .github/agents-md-integrity/README.md | 42 +++- .../agents-md-integrity/check_agents_md.py | 223 ++++++++++++++-- .../tests/test_check_agents_md.py | 238 ++++++++++++++++++ .github/workflows/agents-md-integrity.yml | 35 ++- .../workflows/test-agents-md-integrity.yml | 40 +++ README.md | 2 +- 6 files changed, 561 insertions(+), 19 deletions(-) diff --git a/.github/agents-md-integrity/README.md b/.github/agents-md-integrity/README.md index fa7bd2a..b91b73b 100644 --- a/.github/agents-md-integrity/README.md +++ b/.github/agents-md-integrity/README.md @@ -14,7 +14,8 @@ rewrite the check). a one-line `@AGENTS.md` `CLAUDE.md` shim, no divergent `.cursorrules`, per-subtree shims in monorepos, and a CODEOWNERS DRI. Inputs come from env vars (`MAX_LINES`, `WARN_LINES`, `FORBID_CURSORRULES`, `CHECK_NESTED`, - `REQUIRE_CODEOWNERS`, `AGENTS_FILE`); see the workflow header for the mapping. + `REQUIRE_CODEOWNERS`, `AGENTS_FILE`) plus the `--exclude` flag; see the + workflow header for the mapping. - **`tests/`** — `unittest` suite, run by [`test-agents-md-integrity.yml`](../workflows/test-agents-md-integrity.yml). @@ -23,3 +24,42 @@ Run locally against any repo: ```bash python3 .github/agents-md-integrity/check_agents_md.py --root /path/to/repo ``` + +## Excluding payload subtrees (`--exclude` / `exclude_paths`) + +The nested-shim rule ("every nested `AGENTS.md` needs a sibling `@AGENTS.md` +`CLAUDE.md`") is right for a monorepo subtree and **wrong for a repo whose +product IS agent instructions** — a plugin/skill marketplace ships +`AGENTS.md` + a real multi-line `CLAUDE.md` as distributable payload, and +turning that sibling into a shim would corrupt what gets published. Such a repo +used to have only one escape, `check_nested: false`, which silently drops nested +coverage for the **whole** repo. + +`--exclude` (workflow input `exclude_paths`) carves out just those subtrees: + +```bash +python3 .github/agents-md-integrity/check_agents_md.py --root . --exclude 'plugins/**' +``` + +```yaml +with: + workflows_ref: + exclude_paths: | + plugins/** +``` + +- Repeatable, and one value may be comma- or newline-separated. Globs are + repo-root relative; `*`/`?` stay within a path segment, `**` crosses + segments, a leading `**/` means "at any depth", and a glob matching a + directory excludes everything beneath it. +- **Additive**, never a replacement: the hardcoded `SKIP_DIRS` baseline + (`node_modules`, `vendor`, `.git`, …) still applies. +- Applied during the **walk**, so an excluded subtree is never opened or + line-counted — not post-filtered out of the findings. +- Every exclusion is echoed to the log as + `EXCLUDED: (matched )` (plus a `::notice::` annotation), and the + configured globs are printed even when they match nothing. An exclusion that + leaves no trace is how coverage rots invisibly. +- A glob matching the **root** agents file or `CLAUDE.md` is rejected with exit + code **2** (`1` = a check failed, `0` = pass). Root compliance is the + non-negotiable part of the standard and is not excludable. diff --git a/.github/agents-md-integrity/check_agents_md.py b/.github/agents-md-integrity/check_agents_md.py index 4662393..621f8d6 100644 --- a/.github/agents-md-integrity/check_agents_md.py +++ b/.github/agents-md-integrity/check_agents_md.py @@ -14,8 +14,20 @@ status check. It operates on a checked-out repo tree (the CALLER's repo when run from the reusable workflow) and exits non-zero when any hard check fails. +A repo whose PRODUCT is agent instructions (a plugin/skill marketplace) ships +`AGENTS.md` + `CLAUDE.md` pairs as distributable payload, where the nested-shim +rule is simply wrong — that payload is not this repo's own agent instructions. +`--exclude` carves those subtrees out of the nested walk without disabling the +nested check everywhere else. Exclusions are always echoed to the log, and a +glob that would exclude the ROOT agents file or `CLAUDE.md` is rejected: root +compliance is the non-negotiable part of the standard. + +Exit codes: 0 pass, 1 one or more checks failed, 2 bad `--exclude` config. + Run locally: python3 .github/agents-md-integrity/check_agents_md.py --root . + python3 .github/agents-md-integrity/check_agents_md.py --root . \ + --exclude 'plugins/**' """ import argparse @@ -51,6 +63,90 @@ ) +class ExcludeConfigError(Exception): + """An `--exclude` glob is not usable (today: it would exclude the root).""" + + +def _split_patterns(values): + """Flatten repeated / comma- / newline-separated `--exclude` values. + + The workflow hands the whole `exclude_paths` input over as ONE argument, so + a value may itself be a multi-line or comma-separated list. Blank entries + are dropped, which is what makes an empty input a true no-op. + """ + patterns = [] + for value in values or (): + for chunk in re.split(r"[,\n\r]", value): + chunk = chunk.strip() + if chunk: + patterns.append(chunk) + return patterns + + +def _exclude_pattern_to_regex(pattern): + """Translate one exclusion glob into an anchored full-match regex. + + Deliberately narrower than the CODEOWNERS translation above: an exclusion + glob is ALWAYS repo-root-relative (no match-the-basename-at-any-depth + magic), because a glob that silently matched deeper than intended would + delete coverage nobody asked to drop. `*`/`?` match within one path + segment, `**` matches across segments, and a leading `**/` means "at any + depth". A glob that matches a directory excludes everything beneath it (the + trailing group) — that is what makes `plugins` and `plugins/**` both prune + the whole subtree. A leading `/` or `./` is tolerated and stripped. + """ + p = pattern.strip() + if p.startswith("./"): + p = p[2:] + p = p.lstrip("/").rstrip("/") + + prefix = r"" + if p.startswith("**/"): + prefix = r"(?:.*/)?" + p = p[3:] + + body = re.escape(p) + body = body.replace(r"\*\*", ".*").replace(r"\*", "[^/]*").replace(r"\?", "[^/]") + return re.compile(r"^" + prefix + body + r"(?:/.*)?$") + + +def _compile_excludes(patterns): + """Return [(glob, regex)] for each non-empty glob, preserving order. + + Blank entries are dropped here as well as in `_split_patterns`, so a config + dict assembled by hand can't smuggle in an empty glob. + """ + return [(p, _exclude_pattern_to_regex(p)) for p in patterns if p.strip()] + + +def _match_exclude(rel_path, excludes): + """Return the first glob matching `rel_path`, or None.""" + for pattern, regex in excludes: + if regex.match(rel_path): + return pattern + return None + + +def _validate_excludes(excludes, agents_file): + """Reject any glob that would exclude the ROOT agents file or CLAUDE.md. + + Root compliance is the non-negotiable part of the standard, so this is a + loud config error (exit 2), not one failure among many — a caller that + writes `**` must be told it asked for something the checker will not do, + rather than quietly getting a green run over an unchecked repo. + """ + protected = [os.path.normpath(agents_file), "CLAUDE.md"] + for pattern, regex in excludes: + for rel in protected: + if regex.match(rel): + raise ExcludeConfigError( + f"exclusion glob '{pattern}' would exclude the root " + f"'{rel}', which is not excludable — root AGENTS.md / " + f"CLAUDE.md compliance is the non-negotiable part of the " + f"standard. Narrow the glob (e.g. 'plugins/**')." + ) + + def _count_lines(path): """Line count of a text file (a trailing newline doesn't add a phantom line).""" with open(path, "r", encoding="utf-8", errors="replace") as f: @@ -127,28 +223,63 @@ def _codeowners_owns(root, rel_path): return False, False -def _iter_nested_agents(root, agents_basename, top_level_rel): - """Yield repo-relative paths of every nested AGENTS.md (not the top-level one). +def _rel(root, path): + """Repo-relative, normalized, forward-slash path — the form globs match.""" + return os.path.normpath(os.path.relpath(path, root)).replace(os.sep, "/") - `top_level_rel` is the configured agents_file path (normalized) so a pathful - value like `docs/AGENTS.md` isn't also re-checked here as a "nested" file. + +def _scan_nested_agents(root, agents_basename, top_level_rel, excludes): + """Find every nested AGENTS.md (not the top-level one), honoring exclusions. + + Returns (nested, excluded): repo-relative paths to check, and the + (path, glob) pairs the exclusion globs pruned. `top_level_rel` is the + configured agents_file path (normalized) so a pathful value like + `docs/AGENTS.md` isn't also re-checked here as a "nested" file. + + Exclusions are applied DURING the walk, not as a post-filter on findings: + an excluded directory is never descended into, so nothing inside it is ever + opened or line-counted. `SKIP_DIRS` remains the always-on baseline; + `excludes` is purely additive on top of it. """ + nested = [] + excluded = [] for dirpath, dirnames, filenames in os.walk(root): dirnames[:] = [d for d in dirnames if d not in SKIP_DIRS] + if excludes: + kept = [] + for d in dirnames: + rel_dir = _rel(root, os.path.join(dirpath, d)) + match = _match_exclude(rel_dir, excludes) + if match: + excluded.append((rel_dir, match)) + else: + kept.append(d) + dirnames[:] = kept if agents_basename in filenames: - rel = os.path.relpath(os.path.join(dirpath, agents_basename), root) - if os.path.normpath(rel) != top_level_rel: # skip the top-level file - yield rel + rel = _rel(root, os.path.join(dirpath, agents_basename)) + if rel == top_level_rel: # skip the top-level file + continue + match = _match_exclude(rel, excludes) + if match: + excluded.append((rel, match)) + else: + nested.append(rel) + return nested, excluded def run_checks(root, config): """Run every integrity check against `root`. - Returns (failures, warnings): two lists of human-readable strings. An empty + Returns (failures, warnings, exclusions): two lists of human-readable + strings plus the (path, glob) pairs the nested walk excluded. An empty `failures` list means the repo passes; warnings never fail the check. + + Raises ExcludeConfigError when `config["exclude"]` contains a glob that + would exclude the root agents file or CLAUDE.md. """ failures = [] warnings = [] + exclusions = [] agents_file = config["agents_file"] agents_basename = os.path.basename(agents_file) @@ -156,6 +287,20 @@ def run_checks(root, config): max_lines = config["max_lines"] warn_lines = config["warn_lines"] + # Validated unconditionally — a root-excluding glob is a config error even + # when `check_nested` is off and the globs would never have been consulted. + excludes = _compile_excludes(config.get("exclude") or []) + _validate_excludes(excludes, agents_file) + if excludes and not config["check_nested"]: + # Both knobs set means the caller narrowed an exclusion they think is + # scoping coverage while nested checking is off for the WHOLE repo — + # exactly the invisible coverage loss exclusions exist to replace. + warnings.append( + "exclusion globs are configured but `check_nested` is false, so " + "they exclude nothing — nested checking is already off for the " + "entire repo. Re-enable `check_nested` to use the exclusions." + ) + agents_path = os.path.join(root, agents_file) # 1. Exists. @@ -213,8 +358,12 @@ def run_checks(root, config): # 5. Nested AGENTS.md (monorepo). if config["check_nested"]: - top_level_rel = os.path.normpath(agents_file) - for rel in sorted(_iter_nested_agents(root, agents_basename, top_level_rel)): + top_level_rel = os.path.normpath(agents_file).replace(os.sep, "/") + nested, excluded = _scan_nested_agents( + root, agents_basename, top_level_rel, excludes + ) + exclusions = sorted(set(excluded)) + for rel in sorted(nested): nested_path = os.path.join(root, rel) sibling_claude = os.path.join(os.path.dirname(nested_path), "CLAUDE.md") if not ( @@ -251,7 +400,7 @@ def run_checks(root, config): else: warnings.append(msg) - return failures, warnings + return failures, warnings, exclusions def _env_bool(name, default): @@ -271,8 +420,17 @@ def _env_int(name, default): return default -def _emit(failures, warnings): - """Print human lines plus GitHub Actions annotations, and return exit code.""" +def _emit(failures, warnings, exclusions=()): + """Print human lines plus GitHub Actions annotations, and return exit code. + + Exclusions are printed FIRST and annotated as notices: an exclusion that + leaves no trace in the log is how coverage rots invisibly, so every subtree + the walk skipped is named alongside the glob that skipped it. + """ + for path, pattern in exclusions: + line = f"EXCLUDED: {path} (matched {pattern})" + print(line) + print(f"::notice::AGENTS.md integrity: {line}") for w in warnings: print(f"WARN: {w}") print(f"::warning::AGENTS.md integrity: {w}") @@ -280,6 +438,12 @@ def _emit(failures, warnings): print(f"FAIL: {f}") print(f"::error::AGENTS.md integrity: {f}") + if exclusions: + print( + f"\n{len(exclusions)} path(s) excluded from the nested scan " + f"by --exclude." + ) + if failures: print(f"\nResult: {len(failures)} check(s) failed.") return 1 @@ -297,6 +461,19 @@ def main(argv=None): default=os.environ.get("AGENTS_CHECK_ROOT", "."), help="Repo root to check (default: current directory).", ) + parser.add_argument( + "--exclude", + action="append", + default=[], + metavar="GLOB", + help=( + "Path glob to exclude from the NESTED AGENTS.md scan, relative to " + "the repo root (e.g. 'plugins/**'). Repeatable; a single value may " + "also be comma- or newline-separated. Additive on top of the " + "always-on SKIP_DIRS baseline. A glob matching the root agents " + "file or CLAUDE.md is rejected." + ), + ) args = parser.parse_args(argv) config = { @@ -307,11 +484,25 @@ def main(argv=None): "check_nested": _env_bool("CHECK_NESTED", True), "require_shim": _env_bool("REQUIRE_SHIM", True), "require_codeowners": _env_bool("REQUIRE_CODEOWNERS", False), + "exclude": _split_patterns(args.exclude), } - print(f"Checking AGENTS.md integrity in '{args.root}'...\n") - failures, warnings = run_checks(args.root, config) - return _emit(failures, warnings) + print(f"Checking AGENTS.md integrity in '{args.root}'...") + # Echo the CONFIGURED globs, not just the paths they hit: a typo'd glob + # that matches nothing must still be visible in the log. + if config["exclude"]: + print("Exclusion globs: " + ", ".join(config["exclude"])) + print() + + try: + failures, warnings, exclusions = run_checks(args.root, config) + except ExcludeConfigError as exc: + print(f"FAIL: {exc}") + print(f"::error::AGENTS.md integrity: {exc}") + print("\nResult: invalid --exclude configuration.") + return 2 + + return _emit(failures, warnings, exclusions) if __name__ == "__main__": diff --git a/.github/agents-md-integrity/tests/test_check_agents_md.py b/.github/agents-md-integrity/tests/test_check_agents_md.py index 7c6172b..34068c4 100644 --- a/.github/agents-md-integrity/tests/test_check_agents_md.py +++ b/.github/agents-md-integrity/tests/test_check_agents_md.py @@ -8,7 +8,9 @@ Run: python3 .github/agents-md-integrity/tests/test_check_agents_md.py """ +import contextlib import importlib.util +import io import os import tempfile import unittest @@ -27,6 +29,7 @@ "check_nested": True, "require_shim": True, "require_codeowners": False, + "exclude": [], } @@ -52,6 +55,12 @@ def tearDown(self): self._tmp.cleanup() def _run(self, **overrides): + """(failures, warnings) — the pair almost every case cares about.""" + failures, warnings, _ = self._run_full(**overrides) + return failures, warnings + + def _run_full(self, **overrides): + """(failures, warnings, exclusions) — for the exclusion cases.""" return cam.run_checks(self.root, _config(**overrides)) # --- passing case ----------------------------------------------------- @@ -215,5 +224,234 @@ def test_pathful_agents_file_not_double_checked_as_nested(self): self.assertEqual(failures, []) +class ExcludePathsTest(unittest.TestCase): + """`--exclude` / `exclude_paths`: carve payload subtrees out of the walk. + + The motivating shape is a repo whose PRODUCT is agent instructions — a + plugin marketplace ships `plugins//AGENTS.md` next to a real + multi-line Claude payload, not an `@AGENTS.md` shim — where `check_nested` + is correct everywhere except that subtree. + """ + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = self._tmp.name + # A compliant root, so only the nested/exclusion behavior is under test. + _write(self.root, "AGENTS.md", "thin\n") + _write(self.root, "CLAUDE.md", "@AGENTS.md\n") + _write(self.root, "CODEOWNERS", "* @o\n") + + def tearDown(self): + self._tmp.cleanup() + + def _run(self, **overrides): + return cam.run_checks(self.root, _config(**overrides)) + + def _write_plugin_payload(self): + """The comfy-conventions shape: payload AGENTS.md + a real CLAUDE.md.""" + _write(self.root, "plugins/comfy-conventions/AGENTS.md", "payload\n") + _write( + self.root, + "plugins/comfy-conventions/CLAUDE.md", + "\n".join(f"claude payload line {i}" for i in range(44)), + ) + + # --- the four acceptance cases --------------------------------------- + + def test_excluded_nested_pair_passes(self): + self._write_plugin_payload() + failures, warnings, exclusions = self._run(exclude=["plugins/**"]) + self.assertEqual(failures, []) + self.assertEqual(warnings, []) + self.assertIn(("plugins/comfy-conventions", "plugins/**"), exclusions) + + def test_non_excluded_nested_pair_still_fails(self): + # Same repo, a SECOND nested file outside the excluded subtree: the + # exclusion must not disable the rule everywhere else. + self._write_plugin_payload() + _write(self.root, "packages/api/AGENTS.md", "nested\n") + _write(self.root, "packages/api/CLAUDE.md", "divergent, no import\n") + + failures, _, exclusions = self._run(exclude=["plugins/**"]) + self.assertEqual(len(failures), 1) + self.assertIn("packages/api/AGENTS.md", failures[0]) + self.assertIn("no sibling 'CLAUDE.md'", failures[0]) + self.assertNotIn("plugins", "\n".join(failures)) + self.assertEqual(exclusions, [("plugins/comfy-conventions", "plugins/**")]) + + def test_exclusion_targeting_root_errors_out(self): + for glob in ("**", "AGENTS.md", "CLAUDE.md", "*", "/AGENTS.md", "./CLAUDE.md"): + with self.subTest(glob=glob): + with self.assertRaises(cam.ExcludeConfigError) as ctx: + self._run(exclude=[glob]) + self.assertIn("not excludable", str(ctx.exception)) + + def test_root_exclusion_rejected_even_when_nested_check_is_off(self): + # The glob is never consulted with check_nested off, but asking for + # something the checker will not do is still a loud config error. + with self.assertRaises(cam.ExcludeConfigError): + self._run(exclude=["**"], check_nested=False) + + def test_pathful_agents_file_is_protected_too(self): + _write(self.root, "docs/AGENTS.md", "thin\n") + with self.assertRaises(cam.ExcludeConfigError): + self._run(agents_file="docs/AGENTS.md", exclude=["docs/**"]) + # ...but excluding an unrelated subtree is still fine. + self._run(agents_file="docs/AGENTS.md", exclude=["plugins/**"]) + + def test_no_exclude_reproduces_todays_behavior(self): + self._write_plugin_payload() + failures, warnings, exclusions = self._run() + self.assertEqual(exclusions, []) + self.assertEqual(warnings, []) + self.assertEqual(len(failures), 1) + self.assertIn("plugins/comfy-conventions/AGENTS.md", failures[0]) + self.assertIn("no sibling 'CLAUDE.md'", failures[0]) + + # --- walk-time (not post-filter) semantics ---------------------------- + + def test_excluded_subtree_is_never_line_counted(self): + # A nested payload file way over the ceiling: a post-filter on findings + # would have opened and counted it first. Excluded means never read. + _write( + self.root, + "plugins/big/AGENTS.md", + "\n".join(f"l{i}" for i in range(500)), + ) + _write(self.root, "plugins/big/CLAUDE.md", "44 lines of payload\n") + failures, _, _ = self._run(exclude=["plugins/**"]) + self.assertEqual(failures, []) + + def test_exclusion_prunes_the_directory_before_descending(self): + _write(self.root, "plugins/a/b/c/AGENTS.md", "deep payload\n") + failures, _, exclusions = self._run(exclude=["plugins/**"]) + self.assertEqual(failures, []) + # Reported once, at the pruned directory — not once per buried file. + self.assertEqual(exclusions, [("plugins/a", "plugins/**")]) + + def test_directly_matched_nested_file_is_reported(self): + glob = "packages/api/AGENTS.md" + _write(self.root, glob, "nested, no shim\n") + failures, _, exclusions = self._run(exclude=[glob]) + self.assertEqual(failures, []) + self.assertEqual(exclusions, [(glob, glob)]) + + def test_exclusions_with_check_nested_off_warn_that_they_do_nothing(self): + # Both knobs set is the invisible-coverage-loss shape exclusions exist + # to replace, so say so rather than letting it read as scoped. + _write(self.root, "plugins/x/AGENTS.md", "payload\n") + failures, warnings, exclusions = self._run( + exclude=["plugins/**"], check_nested=False + ) + self.assertEqual(failures, []) + self.assertEqual(exclusions, []) + self.assertEqual(len(warnings), 1) + self.assertIn("`check_nested` is false", warnings[0]) + + def test_blank_glob_in_a_handmade_config_is_dropped(self): + _write(self.root, "packages/api/AGENTS.md", "nested, no shim\n") + failures, _, exclusions = self._run(exclude=["", " "]) + self.assertEqual(exclusions, []) + self.assertEqual(len(failures), 1) # a blank glob excludes NOTHING + + def test_skip_dirs_remain_the_always_on_baseline(self): + # SKIP_DIRS is not replaced by --exclude; it still applies alongside it. + _write(self.root, "node_modules/pkg/AGENTS.md", "vendored\n") + _write(self.root, "plugins/x/AGENTS.md", "payload\n") + failures, _, exclusions = self._run(exclude=["plugins/**"]) + self.assertEqual(failures, []) + # The vendored tree is skipped silently (baseline), not reported as an + # exclusion — only the caller's own globs get an EXCLUDED line. + self.assertEqual(exclusions, [("plugins/x", "plugins/**")]) + + # --- glob semantics --------------------------------------------------- + + def test_bare_directory_glob_excludes_the_whole_subtree(self): + _write(self.root, "plugins/x/AGENTS.md", "payload\n") + failures, _, exclusions = self._run(exclude=["plugins"]) + self.assertEqual(failures, []) + self.assertEqual(exclusions, [("plugins", "plugins")]) + + def test_single_star_does_not_cross_a_path_separator(self): + _write(self.root, "packages/api/AGENTS.md", "nested, no shim\n") + # `packages/*` matches `packages/api` (one segment) — excluded. + failures, _, _ = self._run(exclude=["packages/*"]) + self.assertEqual(failures, []) + # `pack*/AGENTS.md` must NOT match `packages/api/AGENTS.md`. + failures, _, exclusions = self._run(exclude=["pack*/AGENTS.md"]) + self.assertEqual(exclusions, []) + self.assertEqual(len(failures), 1) + + def test_leading_double_star_matches_at_any_depth(self): + _write(self.root, "a/payload/AGENTS.md", "payload\n") + _write(self.root, "payload/AGENTS.md", "payload\n") + failures, _, exclusions = self._run(exclude=["**/payload"]) + self.assertEqual(failures, []) + self.assertEqual( + exclusions, [("a/payload", "**/payload"), ("payload", "**/payload")] + ) + + def test_unmatched_glob_is_a_silent_no_op_not_an_error(self): + _write(self.root, "packages/api/AGENTS.md", "nested\n") + _write(self.root, "packages/api/CLAUDE.md", "@AGENTS.md\n") + failures, warnings, exclusions = self._run(exclude=["does/not/exist/**"]) + self.assertEqual(failures, []) + self.assertEqual(warnings, []) + self.assertEqual(exclusions, []) + + # --- CLI / value parsing ---------------------------------------------- + + def test_split_patterns_handles_repeatable_csv_and_newlines(self): + self.assertEqual( + cam._split_patterns(["a/**,b/**", " c/** \n\n d/** \n", ""]), + ["a/**", "b/**", "c/**", "d/**"], + ) + + def test_split_patterns_of_blank_input_is_empty(self): + # The workflow passes the raw input through; a blank one must be a + # true no-op rather than an empty glob that matches everything. + for value in ([], [""], [" "], ["\n"], [",,"]): + with self.subTest(value=value): + self.assertEqual(cam._split_patterns(value), []) + + def _main(self, *argv): + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + code = cam.main(["--root", self.root, *argv]) + return code, buf.getvalue() + + def test_cli_excluded_payload_passes_and_logs_the_exclusion(self): + self._write_plugin_payload() + code, out = self._main("--exclude", "plugins/**") + self.assertEqual(code, 0) + self.assertIn("Exclusion globs: plugins/**", out) + self.assertIn( + "EXCLUDED: plugins/comfy-conventions (matched plugins/**)", out + ) + self.assertIn("::notice::AGENTS.md integrity: EXCLUDED", out) + self.assertIn("Result: AGENTS.md integrity OK.", out) + + def test_cli_without_exclude_fails_the_same_payload(self): + self._write_plugin_payload() + code, out = self._main() + self.assertEqual(code, 1) + self.assertNotIn("EXCLUDED", out) + self.assertNotIn("Exclusion globs", out) + + def test_cli_root_exclusion_exits_two(self): + code, out = self._main("--exclude", "**") + self.assertEqual(code, 2) + self.assertIn("not excludable", out) + self.assertIn("::error::", out) + + def test_cli_accepts_repeated_and_csv_flags(self): + _write(self.root, "plugins/x/AGENTS.md", "payload\n") + _write(self.root, "vendored-skills/y/AGENTS.md", "payload\n") + code, out = self._main("--exclude", "plugins/**,vendored-skills/**") + self.assertEqual(code, 0) + self.assertIn("EXCLUDED: plugins/x", out) + self.assertIn("EXCLUDED: vendored-skills/y", out) + + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/.github/workflows/agents-md-integrity.yml b/.github/workflows/agents-md-integrity.yml index 7d36318..8f7a0f4 100644 --- a/.github/workflows/agents-md-integrity.yml +++ b/.github/workflows/agents-md-integrity.yml @@ -40,6 +40,21 @@ name: AGENTS.md Integrity (reusable) # check_nested When true, every nested `AGENTS.md` (monorepo subtree) # must have a sibling `CLAUDE.md` containing `@AGENTS.md` # and must itself be <= max_lines. Default true. +# exclude_paths Newline- or comma-separated path globs (repo-root +# relative) carved out of the NESTED scan — for a repo +# that ships agent-instruction files as distributable +# PAYLOAD (a plugin/skill marketplace), where the +# nested-shim rule does not apply to the payload but +# should still bind everywhere else. Prefer this over +# `check_nested: false`, which drops nested coverage for +# the whole repo. Excluded subtrees are never scanned, +# and each one is reported in the run log as +# `EXCLUDED: (matched )`. A glob that would +# exclude the ROOT agents file or `CLAUDE.md` is rejected +# (the run fails) — root compliance is not excludable. +# Default empty (nothing excluded). Example: +# exclude_paths: | +# plugins/** # require_shim When true, a missing root `CLAUDE.md` FAILS whenever the # agents file exists (Claude Code reads only CLAUDE.md and # does not fall back, so no shim = invisible instructions). @@ -82,6 +97,16 @@ on: type: boolean required: false default: true + exclude_paths: + description: >- + Newline- or comma-separated path globs carved out of the nested + AGENTS.md scan (e.g. `plugins/**` for a repo that ships agent + instructions as distributable payload). Excluded subtrees are never + scanned and are reported in the run log. A glob matching the root + agents file or `CLAUDE.md` is rejected. Default empty. + type: string + required: false + default: '' require_shim: description: >- Fail when the root `CLAUDE.md` shim is missing while the agents file @@ -149,6 +174,14 @@ jobs: REQUIRE_SHIM: ${{ inputs.require_shim }} REQUIRE_CODEOWNERS: ${{ inputs.require_codeowners }} AGENTS_FILE: ${{ inputs.agents_file }} + EXCLUDE_PATHS: ${{ inputs.exclude_paths }} run: | + # `--exclude` is added ONLY when the caller set a non-blank value, so + # a caller that never heard of exclude_paths runs the exact command + # it ran before. + args=(--root "$AGENTS_CHECK_ROOT") + if [ -n "$(printf '%s' "$EXCLUDE_PATHS" | tr -d '[:space:]')" ]; then + args+=(--exclude "$EXCLUDE_PATHS") + fi python3 "$GITHUB_WORKSPACE/_agents_md_integrity/.github/agents-md-integrity/check_agents_md.py" \ - --root "$AGENTS_CHECK_ROOT" + "${args[@]}" diff --git a/.github/workflows/test-agents-md-integrity.yml b/.github/workflows/test-agents-md-integrity.yml index 3482ce6..0f51733 100644 --- a/.github/workflows/test-agents-md-integrity.yml +++ b/.github/workflows/test-agents-md-integrity.yml @@ -59,3 +59,43 @@ jobs: exit 1 fi echo "checker correctly failed the drifted repo" + + - name: Smoke test — --exclude carves out a payload subtree, and is logged + run: | + # A repo that SHIPS agent instructions as plugin payload: the nested + # pair is real content, not a shim, so it can only pass via exclusion. + root="$(mktemp -d)" + printf 'thin agents file\n' > "$root/AGENTS.md" + printf '@AGENTS.md\n' > "$root/CLAUDE.md" + mkdir -p "$root/.github" "$root/plugins/demo" + printf '* @comfy-org/backend\n' > "$root/.github/CODEOWNERS" + printf 'plugin payload\n' > "$root/plugins/demo/AGENTS.md" + printf 'Claude-only instructions shipped to plugin consumers.\n' > "$root/plugins/demo/CLAUDE.md" + + if python3 .github/agents-md-integrity/check_agents_md.py --root "$root"; then + echo "::error::payload repo passed WITHOUT --exclude — check_nested is not firing" + exit 1 + fi + + out="$(python3 .github/agents-md-integrity/check_agents_md.py --root "$root" --exclude 'plugins/**')" + echo "$out" + grep -qF 'EXCLUDED: plugins/demo (matched plugins/**)' <<<"$out" || { + echo "::error::exclusion applied but left no trace in the log" + exit 1 + } + echo "checker correctly excluded the payload subtree and logged it" + + - name: Smoke test — a root-excluding glob is rejected + run: | + root="$(mktemp -d)" + printf 'thin agents file\n' > "$root/AGENTS.md" + printf '@AGENTS.md\n' > "$root/CLAUDE.md" + set +e + python3 .github/agents-md-integrity/check_agents_md.py --root "$root" --exclude '**' + code=$? + set -e + if [ "$code" -ne 2 ]; then + echo "::error::expected exit 2 for a root-excluding glob, got $code" + exit 1 + fi + echo "checker correctly rejected a root-excluding glob" diff --git a/README.md b/README.md index 17d83d0..f07c6fe 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ This repo is **public** so any repo — public or private, inside or outside the | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | | [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. | -| [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | +| [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). `exclude_paths` (newline-/comma-separated globs, default empty) carves payload subtrees — a repo whose product IS agent instructions, e.g. a plugin marketplace shipping `plugins/**/AGENTS.md` + a real `CLAUDE.md` — out of the nested scan without the all-or-nothing `check_nested: false`; exclusions are applied during the walk (never scanned or line-counted), reported in the log as `EXCLUDED: (matched )`, and a glob that would exclude the ROOT `AGENTS.md`/`CLAUDE.md` is rejected (exit 2). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | ## Usage From c7be0509f46c401988eec44f0b37cc8459ff2bc9 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 2 Aug 2026 01:30:00 -0700 Subject: [PATCH 2/2] fix(agents-md-integrity): harden --exclude glob semantics from review (BE-6009) Cursor-review panel findings on the `exclude_paths` feature: - Interior `**` now spans ZERO or more segments, so the documented globstar form `plugins/**/AGENTS.md` matches `plugins/AGENTS.md` instead of silently not applying. The translator is segment-aware rather than a flat re.escape + string replace. - A trailing `/**` is dropped: `plugins` and `plugins/**` are now genuinely identical, and a marketplace subtree prunes ONCE at `plugins` instead of emitting one EXCLUDED line per plugin directory. - Globs are matched with `fullmatch`, not `match` + `$` (which also accepts a trailing newline, so a directory named "plugins/demo\n" was pruned by an exact `plugins/demo` exclusion). - Reject the two disguised whole-repo exclusions the root guard missed: a glob normalizing to nothing (`/`, `.`, `//`) and a wildcard-only glob (`*/**`, `*/*`), which pruned every top-level directory while `check_nested` still read `true`. - Escape `%`/CR/LF out of every workflow-command line: paths come from the scanned tree, and a directory named "x\n::stop-commands::tok" could emit a second, attacker-chosen command into a public log. - `protected` normalizes os.sep to `/` like every other path the globs see, so the pathful-agents-file guard actually fires on Windows. - Pass the value as `--exclude=$EXCLUDE_PATHS` so a glob starting with a hyphen is not parsed by argparse as an unknown option. - The payload smoke test asserts exit 1 plus the expected FAIL text rather than any non-zero exit, which an exit-2 config error or traceback also satisfied. Co-Authored-By: Claude Opus 5 --- .github/agents-md-integrity/README.md | 18 ++- .../agents-md-integrity/check_agents_md.py | 103 ++++++++++++++---- .../tests/test_check_agents_md.py | 91 ++++++++++++++-- .github/workflows/agents-md-integrity.yml | 14 ++- .../workflows/test-agents-md-integrity.yml | 19 +++- README.md | 2 +- 6 files changed, 205 insertions(+), 42 deletions(-) diff --git a/.github/agents-md-integrity/README.md b/.github/agents-md-integrity/README.md index b91b73b..b18ce39 100644 --- a/.github/agents-md-integrity/README.md +++ b/.github/agents-md-integrity/README.md @@ -48,10 +48,14 @@ with: plugins/** ``` -- Repeatable, and one value may be comma- or newline-separated. Globs are - repo-root relative; `*`/`?` stay within a path segment, `**` crosses - segments, a leading `**/` means "at any depth", and a glob matching a - directory excludes everything beneath it. +- Repeatable, and one value may be comma- or newline-separated. Because `,` is + always a separator, a path containing a literal comma cannot be expressed. +- Globs are repo-root relative; `*`/`?` stay within a path segment, `**` + crosses **zero or more** segments (so `plugins/**/AGENTS.md` also matches + `plugins/AGENTS.md`), a leading `**/` means "at any depth", and a glob + matching a directory excludes everything beneath it — `plugins` and + `plugins/**` are identical, and both prune once at `plugins` rather than once + per child. - **Additive**, never a replacement: the hardcoded `SKIP_DIRS` baseline (`node_modules`, `vendor`, `.git`, …) still applies. - Applied during the **walk**, so an excluded subtree is never opened or @@ -62,4 +66,8 @@ with: leaves no trace is how coverage rots invisibly. - A glob matching the **root** agents file or `CLAUDE.md` is rejected with exit code **2** (`1` = a check failed, `0` = pass). Root compliance is the - non-negotiable part of the standard and is not excludable. + non-negotiable part of the standard and is not excludable. So are the two + ways of asking for the whole repo without saying so: a glob that normalizes + to nothing (`/`, `.`, `//`) and a glob made only of wildcard segments (`*`, + `**`, `*/**`, `*/*`) — the latter would otherwise prune every top-level + directory while `check_nested` still read `true`. Name the subtree. diff --git a/.github/agents-md-integrity/check_agents_md.py b/.github/agents-md-integrity/check_agents_md.py index 621f8d6..9acbb77 100644 --- a/.github/agents-md-integrity/check_agents_md.py +++ b/.github/agents-md-integrity/check_agents_md.py @@ -90,24 +90,64 @@ def _exclude_pattern_to_regex(pattern): glob is ALWAYS repo-root-relative (no match-the-basename-at-any-depth magic), because a glob that silently matched deeper than intended would delete coverage nobody asked to drop. `*`/`?` match within one path - segment, `**` matches across segments, and a leading `**/` means "at any - depth". A glob that matches a directory excludes everything beneath it (the - trailing group) — that is what makes `plugins` and `plugins/**` both prune - the whole subtree. A leading `/` or `./` is tolerated and stripped. + segment, `**` matches across ZERO or more segments, and a leading `**/` + means "at any depth". A glob that matches a directory excludes everything + beneath it (the trailing group) — that is what makes `plugins` and + `plugins/**` both prune the whole subtree. Redundant separators and a + leading `/` or `./` are tolerated and stripped. + + Raises ExcludeConfigError for a glob that normalizes to nothing (`/`, `.`, + `//`) or that is nothing but wildcard segments (`*`, `**`, `*/**`, `*/*`): + both read as "exclude the whole repo", which is the one thing an exclusion + must never do quietly. Without this, `*/**` would prune every top-level + directory while `check_nested` still read `true`. """ - p = pattern.strip() - if p.startswith("./"): - p = p[2:] - p = p.lstrip("/").rstrip("/") + segs = [s for s in pattern.strip().split("/") if s not in ("", ".")] + if not segs: + raise ExcludeConfigError( + f"exclusion glob '{pattern}' normalizes to the repo root, which is " + f"not excludable. Name the subtree instead (e.g. 'plugins/**')." + ) + if all(s in ("*", "**") for s in segs): + raise ExcludeConfigError( + f"exclusion glob '{pattern}' has no literal path segment, so it " + f"prunes the tree wholesale instead of scoping a subtree — the " + f"nested scan as a whole is not excludable this way. Name the " + f"subtree instead (e.g. 'plugins/**')." + ) + + # A trailing `**` is redundant with the trailing subtree group below, and + # keeping it costs real signal: `plugins/**` would then match only the + # CHILDREN of `plugins`, so the walk prunes each child separately and emits + # one EXCLUDED line per plugin instead of one for the subtree. Dropping it + # is what makes `plugins` and `plugins/**` behave identically, as documented. + while len(segs) > 1 and segs[-1] == "**": + segs.pop() prefix = r"" - if p.startswith("**/"): + if segs[0] == "**": + # Leading `**/` — "at any depth", zero leading segments included. prefix = r"(?:.*/)?" - p = p[3:] + while segs[0] == "**": + segs.pop(0) + + body = "" + for seg in segs: + if seg == "**": + # An INTERIOR `**` spans zero or more whole segments, so the + # mandatory separator belongs to the FOLLOWING literal rather than + # to this group: `plugins/**/AGENTS.md` has to match + # `plugins/AGENTS.md`, not only `plugins//AGENTS.md`. + body += r"(?:/.*)?" + continue + esc = re.escape(seg).replace(r"\*", "[^/]*").replace(r"\?", "[^/]") + body += esc if not body else "/" + esc - body = re.escape(p) - body = body.replace(r"\*\*", ".*").replace(r"\*", "[^/]*").replace(r"\?", "[^/]") - return re.compile(r"^" + prefix + body + r"(?:/.*)?$") + # Matched with `fullmatch` (never `match` + `$`): Python's `$` also accepts + # a trailing newline, and POSIX permits a newline inside a path component, + # so `plugins/demo` would otherwise prune a directory named "plugins/demo\n" + # and drop coverage outside the configured subtree. + return re.compile(prefix + body + r"(?:/.*)?", re.DOTALL) def _compile_excludes(patterns): @@ -122,7 +162,7 @@ def _compile_excludes(patterns): def _match_exclude(rel_path, excludes): """Return the first glob matching `rel_path`, or None.""" for pattern, regex in excludes: - if regex.match(rel_path): + if regex.fullmatch(rel_path): return pattern return None @@ -135,10 +175,14 @@ def _validate_excludes(excludes, agents_file): writes `**` must be told it asked for something the checker will not do, rather than quietly getting a green run over an unchecked repo. """ - protected = [os.path.normpath(agents_file), "CLAUDE.md"] + # Normalized to forward slashes like every other path the globs see (`_rel`, + # `top_level_rel`) — on Windows a bare normpath of `docs/AGENTS.md` yields + # `docs\AGENTS.md`, which a forward-slash-only glob can never match, so the + # guard would silently never fire. + protected = [os.path.normpath(agents_file).replace(os.sep, "/"), "CLAUDE.md"] for pattern, regex in excludes: for rel in protected: - if regex.match(rel): + if regex.fullmatch(rel): raise ExcludeConfigError( f"exclusion glob '{pattern}' would exclude the root " f"'{rel}', which is not excludable — root AGENTS.md / " @@ -420,6 +464,19 @@ def _env_int(name, default): return default +def _esc_cmd(text): + """Escape a value before it is interpolated into a workflow-command line. + + Every path here comes from the scanned repo tree, which a PR author + controls, and POSIX/git permit a newline inside a path component. Unescaped, + a directory named "x\\n::stop-commands::tok" would close this line and emit a + SECOND, attacker-chosen workflow command — suppressing the `::error::` + annotations printed just below, or forging notices in a public log. Applied + to the plain line too, since that line would equally start a `::` command. + """ + return str(text).replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A") + + def _emit(failures, warnings, exclusions=()): """Print human lines plus GitHub Actions annotations, and return exit code. @@ -428,13 +485,15 @@ def _emit(failures, warnings, exclusions=()): the walk skipped is named alongside the glob that skipped it. """ for path, pattern in exclusions: - line = f"EXCLUDED: {path} (matched {pattern})" + line = f"EXCLUDED: {_esc_cmd(path)} (matched {_esc_cmd(pattern)})" print(line) print(f"::notice::AGENTS.md integrity: {line}") for w in warnings: + w = _esc_cmd(w) print(f"WARN: {w}") print(f"::warning::AGENTS.md integrity: {w}") for f in failures: + f = _esc_cmd(f) print(f"FAIL: {f}") print(f"::error::AGENTS.md integrity: {f}") @@ -491,14 +550,18 @@ def main(argv=None): # Echo the CONFIGURED globs, not just the paths they hit: a typo'd glob # that matches nothing must still be visible in the log. if config["exclude"]: - print("Exclusion globs: " + ", ".join(config["exclude"])) + print("Exclusion globs: " + ", ".join(_esc_cmd(g) for g in config["exclude"])) print() try: failures, warnings, exclusions = run_checks(args.root, config) except ExcludeConfigError as exc: - print(f"FAIL: {exc}") - print(f"::error::AGENTS.md integrity: {exc}") + # The glob is echoed back in the message and comes from the caller's + # workflow file, which a `pull_request` run reads from the merge ref — + # so it gets the same workflow-command escaping as scanned paths. + msg = _esc_cmd(exc) + print(f"FAIL: {msg}") + print(f"::error::AGENTS.md integrity: {msg}") print("\nResult: invalid --exclude configuration.") return 2 diff --git a/.github/agents-md-integrity/tests/test_check_agents_md.py b/.github/agents-md-integrity/tests/test_check_agents_md.py index 34068c4..d3b0f9c 100644 --- a/.github/agents-md-integrity/tests/test_check_agents_md.py +++ b/.github/agents-md-integrity/tests/test_check_agents_md.py @@ -263,7 +263,7 @@ def test_excluded_nested_pair_passes(self): failures, warnings, exclusions = self._run(exclude=["plugins/**"]) self.assertEqual(failures, []) self.assertEqual(warnings, []) - self.assertIn(("plugins/comfy-conventions", "plugins/**"), exclusions) + self.assertIn(("plugins", "plugins/**"), exclusions) def test_non_excluded_nested_pair_still_fails(self): # Same repo, a SECOND nested file outside the excluded subtree: the @@ -277,7 +277,7 @@ def test_non_excluded_nested_pair_still_fails(self): self.assertIn("packages/api/AGENTS.md", failures[0]) self.assertIn("no sibling 'CLAUDE.md'", failures[0]) self.assertNotIn("plugins", "\n".join(failures)) - self.assertEqual(exclusions, [("plugins/comfy-conventions", "plugins/**")]) + self.assertEqual(exclusions, [("plugins", "plugins/**")]) def test_exclusion_targeting_root_errors_out(self): for glob in ("**", "AGENTS.md", "CLAUDE.md", "*", "/AGENTS.md", "./CLAUDE.md"): @@ -327,7 +327,7 @@ def test_exclusion_prunes_the_directory_before_descending(self): failures, _, exclusions = self._run(exclude=["plugins/**"]) self.assertEqual(failures, []) # Reported once, at the pruned directory — not once per buried file. - self.assertEqual(exclusions, [("plugins/a", "plugins/**")]) + self.assertEqual(exclusions, [("plugins", "plugins/**")]) def test_directly_matched_nested_file_is_reported(self): glob = "packages/api/AGENTS.md" @@ -362,7 +362,7 @@ def test_skip_dirs_remain_the_always_on_baseline(self): self.assertEqual(failures, []) # The vendored tree is skipped silently (baseline), not reported as an # exclusion — only the caller's own globs get an EXCLUDED line. - self.assertEqual(exclusions, [("plugins/x", "plugins/**")]) + self.assertEqual(exclusions, [("plugins", "plugins/**")]) # --- glob semantics --------------------------------------------------- @@ -391,6 +391,61 @@ def test_leading_double_star_matches_at_any_depth(self): exclusions, [("a/payload", "**/payload"), ("payload", "**/payload")] ) + def test_interior_double_star_spans_zero_segments(self): + # The documented contract is "`**` crosses segments", and the leading + # `**/` case already matches zero of them; an interior one that needed + # at least one segment would silently not apply the exclusion a caller + # wrote in the standard globstar form. + _write(self.root, "plugins/AGENTS.md", "payload\n") + _write(self.root, "plugins/deep/nest/AGENTS.md", "payload\n") + failures, _, exclusions = self._run(exclude=["plugins/**/AGENTS.md"]) + self.assertEqual(failures, []) + self.assertIn(("plugins/AGENTS.md", "plugins/**/AGENTS.md"), exclusions) + self.assertIn( + ("plugins/deep/nest/AGENTS.md", "plugins/**/AGENTS.md"), exclusions + ) + + def test_trailing_double_star_prunes_at_the_directory_itself(self): + # `plugins` and `plugins/**` are documented as identical. If `/**` + # matched only the CHILDREN, a marketplace with hundreds of plugins + # would emit hundreds of EXCLUDED lines instead of one. + for name in ("a", "b", "c"): + _write(self.root, f"plugins/{name}/AGENTS.md", "payload\n") + bare = self._run(exclude=["plugins"]) + globbed = self._run(exclude=["plugins/**"]) + self.assertEqual(bare[0], []) + self.assertEqual([p for p, _ in bare[2]], ["plugins"]) + self.assertEqual([p for p, _ in globbed[2]], ["plugins"]) + + def test_glob_is_a_strict_full_match_not_match_plus_dollar(self): + # Python's `$` also matches before a trailing newline, and a path + # component may contain one, so `re.match(...\n)` would let a crafted + # directory name be pruned by an exclusion that does not name it. + regex = cam._exclude_pattern_to_regex("plugins/demo") + self.assertTrue(regex.fullmatch("plugins/demo")) + self.assertTrue(regex.fullmatch("plugins/demo/nested")) + self.assertIsNone(regex.fullmatch("plugins/demo\n")) + self.assertIsNone(cam._match_exclude("plugins/demo\n", [("g", regex)])) + + def test_wildcard_only_glob_is_rejected(self): + # `*/**` matches every path containing a slash but neither protected + # root file, so the root guard alone would let it disable the whole + # nested scan while `check_nested` still read `true`. + for glob in ("*/**", "*/*", "**/*", "**/**"): + with self.subTest(glob=glob): + with self.assertRaises(cam.ExcludeConfigError) as ctx: + self._run(exclude=[glob]) + self.assertIn("not excludable", str(ctx.exception)) + + def test_glob_normalizing_to_the_root_is_rejected(self): + # `/` most plausibly reads as "exclude the repo root"; it must be the + # loud exit-2 rejection, not a regex that silently matches nothing. + for glob in ("/", "./", "//", "."): + with self.subTest(glob=glob): + with self.assertRaises(cam.ExcludeConfigError) as ctx: + self._run(exclude=[glob]) + self.assertIn("not excludable", str(ctx.exception)) + def test_unmatched_glob_is_a_silent_no_op_not_an_error(self): _write(self.root, "packages/api/AGENTS.md", "nested\n") _write(self.root, "packages/api/CLAUDE.md", "@AGENTS.md\n") @@ -425,9 +480,7 @@ def test_cli_excluded_payload_passes_and_logs_the_exclusion(self): code, out = self._main("--exclude", "plugins/**") self.assertEqual(code, 0) self.assertIn("Exclusion globs: plugins/**", out) - self.assertIn( - "EXCLUDED: plugins/comfy-conventions (matched plugins/**)", out - ) + self.assertIn("EXCLUDED: plugins (matched plugins/**)", out) self.assertIn("::notice::AGENTS.md integrity: EXCLUDED", out) self.assertIn("Result: AGENTS.md integrity OK.", out) @@ -444,13 +497,33 @@ def test_cli_root_exclusion_exits_two(self): self.assertIn("not excludable", out) self.assertIn("::error::", out) + def test_annotations_escape_newlines_out_of_repo_controlled_paths(self): + # A path component may contain a newline, and the scanned tree is + # PR-controlled: unescaped, the name below would close the `::notice::` + # and emit a second workflow command that suppresses the annotations + # printed after it. + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + cam._emit( + ["nested 'a\n::error::forged' is bad"], + [], + [("x\n::stop-commands::tok", "x*")], + ) + lines = buf.getvalue().splitlines() + # The injected commands survive only as inert %0A-escaped text, so no + # LINE begins with a workflow command other than the ones we emitted. + self.assertIn("EXCLUDED: x%0A::stop-commands::tok (matched x*)", lines) + for line in lines: + if line.startswith("::"): + self.assertRegex(line, r"^::(notice|warning|error)::AGENTS\.md ") + def test_cli_accepts_repeated_and_csv_flags(self): _write(self.root, "plugins/x/AGENTS.md", "payload\n") _write(self.root, "vendored-skills/y/AGENTS.md", "payload\n") code, out = self._main("--exclude", "plugins/**,vendored-skills/**") self.assertEqual(code, 0) - self.assertIn("EXCLUDED: plugins/x", out) - self.assertIn("EXCLUDED: vendored-skills/y", out) + self.assertIn("EXCLUDED: plugins (matched plugins/**)", out) + self.assertIn("EXCLUDED: vendored-skills (matched vendored-skills/**)", out) if __name__ == "__main__": diff --git a/.github/workflows/agents-md-integrity.yml b/.github/workflows/agents-md-integrity.yml index 8f7a0f4..402d09e 100644 --- a/.github/workflows/agents-md-integrity.yml +++ b/.github/workflows/agents-md-integrity.yml @@ -50,8 +50,10 @@ name: AGENTS.md Integrity (reusable) # the whole repo. Excluded subtrees are never scanned, # and each one is reported in the run log as # `EXCLUDED: (matched )`. A glob that would -# exclude the ROOT agents file or `CLAUDE.md` is rejected -# (the run fails) — root compliance is not excludable. +# exclude the ROOT agents file or `CLAUDE.md`, or that +# asks for the whole repo without saying so (`/`, `*`, +# `*/**`), is rejected and the run fails — neither root +# compliance nor the nested scan as a whole is excludable. # Default empty (nothing excluded). Example: # exclude_paths: | # plugins/** @@ -103,7 +105,8 @@ on: AGENTS.md scan (e.g. `plugins/**` for a repo that ships agent instructions as distributable payload). Excluded subtrees are never scanned and are reported in the run log. A glob matching the root - agents file or `CLAUDE.md` is rejected. Default empty. + agents file or `CLAUDE.md`, or made only of wildcards, is rejected. + Default empty. type: string required: false default: '' @@ -181,7 +184,10 @@ jobs: # it ran before. args=(--root "$AGENTS_CHECK_ROOT") if [ -n "$(printf '%s' "$EXCLUDE_PATHS" | tr -d '[:space:]')" ]; then - args+=(--exclude "$EXCLUDE_PATHS") + # Attached form: as a separate token, a legitimate glob starting + # with a hyphen (`-generated/**`) is parsed by argparse as an + # unknown option and the job dies with a usage error. + args+=("--exclude=$EXCLUDE_PATHS") fi python3 "$GITHUB_WORKSPACE/_agents_md_integrity/.github/agents-md-integrity/check_agents_md.py" \ "${args[@]}" diff --git a/.github/workflows/test-agents-md-integrity.yml b/.github/workflows/test-agents-md-integrity.yml index 0f51733..fd9ec50 100644 --- a/.github/workflows/test-agents-md-integrity.yml +++ b/.github/workflows/test-agents-md-integrity.yml @@ -72,14 +72,27 @@ jobs: printf 'plugin payload\n' > "$root/plugins/demo/AGENTS.md" printf 'Claude-only instructions shipped to plugin consumers.\n' > "$root/plugins/demo/CLAUDE.md" - if python3 .github/agents-md-integrity/check_agents_md.py --root "$root"; then - echo "::error::payload repo passed WITHOUT --exclude — check_nested is not firing" + # Assert the exact failure, not merely "non-zero": an exit-2 config + # error or an unhandled traceback would also be non-zero and would + # mask a checker that is broken for every repo. + set +e + out="$(python3 .github/agents-md-integrity/check_agents_md.py --root "$root" 2>&1)" + code=$? + set -e + echo "$out" + if [ "$code" -ne 1 ]; then + echo "::error::payload repo without --exclude: expected exit 1, got $code" exit 1 fi + grep -qF "FAIL: nested 'plugins/demo/AGENTS.md' has no sibling" <<<"$out" || { + echo "::error::payload repo passed WITHOUT --exclude — check_nested is not firing" + exit 1 + } out="$(python3 .github/agents-md-integrity/check_agents_md.py --root "$root" --exclude 'plugins/**')" echo "$out" - grep -qF 'EXCLUDED: plugins/demo (matched plugins/**)' <<<"$out" || { + # Pruned once at `plugins`, not once per plugin directory. + grep -qF 'EXCLUDED: plugins (matched plugins/**)' <<<"$out" || { echo "::error::exclusion applied but left no trace in the log" exit 1 } diff --git a/README.md b/README.md index f07c6fe..f71eb46 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ This repo is **public** so any repo — public or private, inside or outside the | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | | [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. | -| [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). `exclude_paths` (newline-/comma-separated globs, default empty) carves payload subtrees — a repo whose product IS agent instructions, e.g. a plugin marketplace shipping `plugins/**/AGENTS.md` + a real `CLAUDE.md` — out of the nested scan without the all-or-nothing `check_nested: false`; exclusions are applied during the walk (never scanned or line-counted), reported in the log as `EXCLUDED: (matched )`, and a glob that would exclude the ROOT `AGENTS.md`/`CLAUDE.md` is rejected (exit 2). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | +| [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). `exclude_paths` (newline-/comma-separated globs, default empty) carves payload subtrees — a repo whose product IS agent instructions, e.g. a plugin marketplace shipping `plugins/**/AGENTS.md` + a real `CLAUDE.md` — out of the nested scan without the all-or-nothing `check_nested: false`; exclusions are applied during the walk (never scanned or line-counted), reported in the log as `EXCLUDED: (matched )`, and a glob that would exclude the ROOT `AGENTS.md`/`CLAUDE.md` — or the whole tree without saying so (`/`, `*`, `*/**`) — is rejected (exit 2). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | ## Usage