` from the extra_generated_globs default (BE-7633) #177

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/167
Author: @mattmillerai
Created: 8/14/2026
Status: ✅ Merged
Merged: 8/15/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-7633-drop-claude-generated-glob

📝 Commits (4)

d212d87 fix(cursor-review): drop /.claude/ from the extra_generated_globs default (BE-7633)
144d368 docs(cursor-review): list the literal extra_generated_globs defaults (BE-7633)
2cf1319 docs(cursor-review): close the round-2 review gaps on the glob/pathspec split (BE-7633)
cbad703 Merge remote-tracking branch 'origin/main' into matt/be-7633-drop-claude-generated-glob

📊 Changes

5 files changed (+79 additions, -17 deletions)

View changed files

📝 .github/cursor-review/README.md (+27 -7)
📝 .github/workflows/ci-cursor-review.yml (+15 -3)
📝 .github/workflows/cursor-review.yml (+29 -3)
📝 README.md (+1 -1)
📝 docs/callers/cursor-review.md (+7 -3)

📄 Description

ELI-5

cursor-review had a list of "this is generated code, don't bother reviewing it" patterns, and .claude/** was on it. But a .claude/ folder is usually hand-written instructions for AI agents — prose a human wrote to steer a machine, which is exactly the sort of change you want a review panel to read. So it comes off the default list. The one repo where the folder really is machine-generated (a tool installed it) can put it back with one line in its own caller.

What changed

.github/workflows/cursor-review.yml — removed **/.claude/** from the extra_generated_globs input default (it was the second of seven entries). The remaining six are unchanged: node_modules, dist, vendor, *.generated.*, *.min.js, *.min.css.
Same input's description now records the decision and the escape hatch: supplying the input replaces the default wholesale, so a repo whose .claude/ tree is vendored/tool-installed output should pass the defaults plus its own **/.claude/** glob.
docs/callers/cursor-review.md — the only other place in the repo that restated the list. Its diff_excludes row still described the pre-classifier default (lockfiles, node_modules, .claude, …, "kept out of both the size count and the reviewed diff"); that default has been '' and diff-only since the shared check-pr-size classifier took over. Fixed, and the extra_generated_globs row it moved to now carries the opt-back-in note.
.github/workflows/pr-size.yml deliberately untouched — its extra_generated_globs default is empty and correct.

Why `.claude` was on the list at all

It was inherited verbatim when the old diff_excludes default was split into the classifier + extra_generated_globs, not chosen. Classifying an agent-instruction tree as codegen has two effects, both wrong for hand-authored content: it is exempt from the diff-size gate, and it is stripped from the diff handed to the panel and judge. A shared default cannot serve both the hand-authored case and the vendored case, so the default takes the safe side (review it) and the vendored case opts out per repo.

Blast radius

Zero at merge. Every consumer pins both uses: and workflows_ref to a full commit SHA, so the changed default reaches a repo only when that repo merges its own SHA-bump PR — which is a reviewed change in that repo.

For whoever authors the bump PRs

Comfy-Org/ComfyUI_frontend (public, verified below): hand-authored .claude/commands + .claude/skills markdown — it should adopt this. One nuance: its caller also restates :!**/.claude/** in its own diff_excludes, so bumping the pin alone gives it the size-gate half only; the panel starts seeing those files only if that line is also dropped. That is the frontend repo's call, in its own repo — untouched here.
The other adopting consumer is the one whose .claude/ is a vendored tool install; it should pass its own extra_generated_globs (defaults + **/.claude/**) when it bumps. Its name is withheld here on purpose — it is a private repo and this repo is public, and AGENTS.md's first "Conventions & gotchas" bullet forbids naming private callers in workflow files, commit messages or PR text. The source ticket asked for both names in this body; I am naming the public one and describing the private one, because the guardrail outranks the request. Both are named on the ticket, where the bump author can read them.
The third repo with a .claude/ tree is handling its caller under its own ticket.

Verification

actionlint .github/workflows/cursor-review.yml — one finding (github.job_workflow_sha is not in actionlint's context schema), byte-identical on origin/main at the same line. Zero new findings.
python3 .github/workflow-pins/check_workflow_pins.py → OK (7 workflows, none defaulting workflows_ref); its suite: 78 tests OK. This is the one CI job path-filtered onto .github/workflows/**, so it is what this PR actually runs.
.github/cursor-review/tests — 187 tests OK (not path-filtered onto this change; run as a sanity check).
Parsed the workflow and asserted the resolved default is exactly **/node_modules/** **/dist/** **/vendor/** **/*.generated.* **/*.min.js **/*.min.css.
git grep -n 'claude/\*\*' and git grep -n '\.claude' — no remaining glob-default or doc restatement contradicts the change. .github/cursor-review/README.md's extra_generated_globs row never listed .claude, so it was already consistent; README.md and CONTRIBUTING.md do not enumerate the list.
Exercised the one consumer artifact readable from here: fetched Comfy-Org/ComfyUI_frontend's .claude/ tree (commands/, skills/ — e.g. adr-compliance-audit.md, comprehensive-pr-review.md: hand-authored prose, not tool output) and its cursor-review caller (pinned at an older SHA of this repo, restating :!**/.claude/** in diff_excludes). Both match what the ticket asserts.

Judgment calls

Table scope. The ticket sent me to docs/callers/cursor-review.md for restatements of the default list. Rewriting that row exposed three more wrong values in the same six-row table — judge_model (claude-opus-4-8-… vs the actual claude-opus-5-thinking-max), workflows_ref (documented as defaulting to main; it is required: with deliberately no default), and diff_excludes itself — plus two undocumented inputs. I fixed the values and added the rows rather than leave a table I had just edited half-wrong; the sibling docs/callers/pr-size.md documents all nine of its inputs, so a complete table is the convention here. Doc-only, no behavior.
Withholding a private consumer's name — see above.

Unexercised artifacts / residual

README.md's cursor-review row documents three inputs that do not exist — blocking:, runs_on: and models: are all absent from on.workflow_call.inputs (the eleven that exist are judge_model, diff_size_cap, ignore_comments, review_label, extra_generated_globs, extra_lockfiles, diff_excludes, workflows_ref, bot_app_id, ledger_prior_review, run_without_label). A caller copying any of them fails at startup, which is the exact footgun AGENTS.md records under "Document only inputs that exist" — blocking is even its worked example. It is a different defect from this one and I did not want to guess whether the README is describing an intended future state, so it is not fixed here and needs a maintainer's call. This is why the ticket reference below is Refs, not Closes: the ticket names README.md among the files to check, and this leaves an unfixed defect in it.
BE-7625's findings comment and the consumer enumeration behind it are unexercised. I have this ticket's summary of the spike, not the spike; and the caller roster is a write-only repo secret, so I cannot independently confirm "only three of ~15 consumers have a .claude/ tree". The one consumer I could read (above) matches. The two private ones I could not, and would not name here regardless.

Refs BE-7633

_{🔄 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/167 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/14/2026 **Status:** ✅ Merged **Merged:** 8/15/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-7633-drop-claude-generated-glob` --- ### 📝 Commits (4) - [`d212d87`](https://github.com/Comfy-Org/github-workflows/commit/d212d87dee01d945ee076ccf03739ce71dfbe9ac) fix(cursor-review): drop **/.claude/** from the extra_generated_globs default (BE-7633) - [`144d368`](https://github.com/Comfy-Org/github-workflows/commit/144d3681e3f6cdfaa8fa3b3935529fce56a31892) docs(cursor-review): list the literal extra_generated_globs defaults (BE-7633) - [`2cf1319`](https://github.com/Comfy-Org/github-workflows/commit/2cf13197e9df90577e1787cb4a8a5c85c54f892a) docs(cursor-review): close the round-2 review gaps on the glob/pathspec split (BE-7633) - [`cbad703`](https://github.com/Comfy-Org/github-workflows/commit/cbad703c741be5fe358fb399e6940cf816cbbfdf) Merge remote-tracking branch 'origin/main' into matt/be-7633-drop-claude-generated-glob ### 📊 Changes **5 files changed** (+79 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+27 -7) 📝 `.github/workflows/ci-cursor-review.yml` (+15 -3) 📝 `.github/workflows/cursor-review.yml` (+29 -3) 📝 `README.md` (+1 -1) 📝 `docs/callers/cursor-review.md` (+7 -3) </details> ### 📄 Description ## ELI-5 `cursor-review` had a list of "this is generated code, don't bother reviewing it" patterns, and `.claude/**` was on it. But a `.claude/` folder is usually hand-written instructions for AI agents — prose a human wrote to steer a machine, which is exactly the sort of change you *want* a review panel to read. So it comes off the default list. The one repo where the folder really is machine-generated (a tool installed it) can put it back with one line in its own caller. ## What changed - `.github/workflows/cursor-review.yml` — removed `**/.claude/**` from the `extra_generated_globs` input default (it was the second of seven entries). The remaining six are unchanged: `node_modules`, `dist`, `vendor`, `*.generated.*`, `*.min.js`, `*.min.css`. - Same input's `description` now records the decision and the escape hatch: supplying the input **replaces** the default wholesale, so a repo whose `.claude/` tree is vendored/tool-installed output should pass the defaults *plus* its own `**/.claude/**` glob. - `docs/callers/cursor-review.md` — the only other place in the repo that restated the list. Its `diff_excludes` row still described the pre-classifier default (lockfiles, `node_modules`, `.claude`, …, "kept out of both the size count and the reviewed diff"); that default has been `''` and diff-only since the shared `check-pr-size` classifier took over. Fixed, and the `extra_generated_globs` row it moved to now carries the opt-back-in note. - `.github/workflows/pr-size.yml` deliberately untouched — its `extra_generated_globs` default is empty and correct. ## Why `.claude` was on the list at all It was inherited verbatim when the old `diff_excludes` default was split into the classifier + `extra_generated_globs`, not chosen. Classifying an agent-instruction tree as codegen has two effects, both wrong for hand-authored content: it is exempt from the diff-size gate, and it is stripped from the diff handed to the panel and judge. A shared default cannot serve both the hand-authored case and the vendored case, so the default takes the safe side (review it) and the vendored case opts out per repo. ## Blast radius Zero at merge. Every consumer pins both `uses:` and `workflows_ref` to a full commit SHA, so the changed default reaches a repo only when that repo merges its own SHA-bump PR — which is a reviewed change in that repo. ## For whoever authors the bump PRs - **`Comfy-Org/ComfyUI_frontend`** (public, verified below): hand-authored `.claude/commands` + `.claude/skills` markdown — it *should* adopt this. One nuance: its caller also restates `:!**/.claude/**` in its own `diff_excludes`, so bumping the pin alone gives it the size-gate half only; the panel starts seeing those files only if that line is also dropped. That is the frontend repo's call, in its own repo — untouched here. - The other adopting consumer is the one whose `.claude/` is a vendored tool install; it should pass its own `extra_generated_globs` (defaults + `**/.claude/**`) when it bumps. **Its name is withheld here on purpose** — it is a private repo and this repo is public, and `AGENTS.md`'s first "Conventions & gotchas" bullet forbids naming private callers in workflow files, commit messages or PR text. The source ticket asked for both names in this body; I am naming the public one and describing the private one, because the guardrail outranks the request. Both are named on the ticket, where the bump author can read them. - The third repo with a `.claude/` tree is handling its caller under its own ticket. ## Verification - `actionlint .github/workflows/cursor-review.yml` — one finding (`github.job_workflow_sha` is not in actionlint's context schema), byte-identical on `origin/main` at the same line. Zero new findings. - `python3 .github/workflow-pins/check_workflow_pins.py` → OK (7 workflows, none defaulting `workflows_ref`); its suite: 78 tests OK. This is the one CI job path-filtered onto `.github/workflows/**`, so it is what this PR actually runs. - `.github/cursor-review/tests` — 187 tests OK (not path-filtered onto this change; run as a sanity check). - Parsed the workflow and asserted the resolved default is exactly `**/node_modules/** **/dist/** **/vendor/** **/*.generated.* **/*.min.js **/*.min.css`. - `git grep -n 'claude/\*\*'` and `git grep -n '\.claude'` — no remaining glob-default or doc restatement contradicts the change. `.github/cursor-review/README.md`'s `extra_generated_globs` row never listed `.claude`, so it was already consistent; `README.md` and `CONTRIBUTING.md` do not enumerate the list. - Exercised the one consumer artifact readable from here: fetched `Comfy-Org/ComfyUI_frontend`'s `.claude/` tree (`commands/`, `skills/` — e.g. `adr-compliance-audit.md`, `comprehensive-pr-review.md`: hand-authored prose, not tool output) and its cursor-review caller (pinned at an older SHA of this repo, restating `:!**/.claude/**` in `diff_excludes`). Both match what the ticket asserts. ## Judgment calls - **Table scope.** The ticket sent me to `docs/callers/cursor-review.md` for restatements of the default list. Rewriting that row exposed three more wrong values in the same six-row table — `judge_model` (`claude-opus-4-8-…` vs the actual `claude-opus-5-thinking-max`), `workflows_ref` (documented as defaulting to `main`; it is `required:` with deliberately no default), and `diff_excludes` itself — plus two undocumented inputs. I fixed the values and added the rows rather than leave a table I had just edited half-wrong; the sibling `docs/callers/pr-size.md` documents all nine of its inputs, so a complete table is the convention here. Doc-only, no behavior. - **Withholding a private consumer's name** — see above. ## Unexercised artifacts / residual - **`README.md`'s cursor-review row documents three inputs that do not exist** — `blocking:`, `runs_on:` and `models:` are all absent from `on.workflow_call.inputs` (the eleven that exist are `judge_model`, `diff_size_cap`, `ignore_comments`, `review_label`, `extra_generated_globs`, `extra_lockfiles`, `diff_excludes`, `workflows_ref`, `bot_app_id`, `ledger_prior_review`, `run_without_label`). A caller copying any of them fails at startup, which is the exact footgun `AGENTS.md` records under "Document only inputs that exist" — `blocking` is even its worked example. It is a different defect from this one and I did not want to guess whether the README is describing an intended future state, so it is **not** fixed here and needs a maintainer's call. This is why the ticket reference below is `Refs`, not `Closes`: the ticket names `README.md` among the files to check, and this leaves an unfixed defect in it. - **BE-7625's findings comment and the consumer enumeration behind it are unexercised.** I have this ticket's summary of the spike, not the spike; and the caller roster is a write-only repo secret, so I cannot independently confirm "only three of ~15 consumers have a `.claude/` tree". The one consumer I could read (above) matches. The two private ones I could not, and would not name here regardless. Refs BE-7633 --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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