Skip to content

ci: add the team-gated Cursor review caller - #67

Merged
wei-hai merged 3 commits into
mainfrom
chore/cursor-review-caller
Aug 24, 2026
Merged

ci: add the team-gated Cursor review caller#67
wei-hai merged 3 commits into
mainfrom
chore/cursor-review-caller

Conversation

@mattmillerai

@mattmillerai mattmillerai commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

ELI-5

Every actively-developed repo in the org gets an automated code review when you put a cursor-review label on a PR. This repo was missing the file that switches that on, so its PRs never got one. This adds it.

What this is

A thin caller for the reusable cursor-review workflow in Comfy-Org/github-workflows, which owns the review panel, the judge, the prompts and the scripts. The caller carries no logic of its own — it pins a SHA and passes repo-specific settings.

Copied from the callers in comfy-cli / comfy-toolbox / evals and adjusted for this repo. The cursor-review label already exists here, so nothing else needs creating.

Why now

This repo was just made routable for automated code work, and it is about to receive its first agent-authored PRs. Landing the review caller before that work arrives means those PRs get a review pass from the outset rather than retroactively — and this PR is itself a deliberately small first change to prove the path.

Repo-specific tuning

Generated and heavy paths are passed via extra_generated_globs, not diff_excludes. The distinction is load-bearing: diff_excludes filters only the patch handed to the review panel, while extra_generated_globs feeds the shared check-pr-size classifier that drives both the reviewed diff and the diff_size_cap count. Listing a generated path under diff_excludes would hide it from the reviewers and still count it toward the cap — so a routine regeneration would trip the cap and skip the entire review.

extra_generated_globs does replace its upstream default wholesale, so the shared defaults (node_modules, dist, vendor, *.generated.*, minified output) are restated verbatim, plus:

  • src/comfy_low/models/_generated.py — the pydantic models emitted from spec/openapi.yaml by scripts/gen_models.sh. Scoped to that one file, not the package: the sibling __init__.py is hand-written (re-export list and __all__) and must stay visible to the panel.
  • assets/**

Dependency lockfiles including uv.lock are not restated — the classifier covers eight of them as built-ins.

The generated file is the one that matters. Without it, a routine regeneration would bury a review in machine-written diff and the signal would be lost.

Label filtering is delegated entirely to the reusable workflow's Gate — the caller carries no job-level if. The Gate owns the review_label input and implements the unblock path (removing skip-cursor-review while cursor-review is still applied re-triggers the review), and every downstream job is already gated on needs.gate.outputs.should_run, so filtering in the caller would only break those behaviours.

Access control

Two layers, both inherited from the reusable workflow's design:

  1. Applying a label in a public repo requires triage permission or higher, so the public cannot trigger this.
  2. The secret-bearing jobs do not run on fork PRs (forks receive no secrets), so CURSOR_API_KEY is only reachable from internal branches.

Permissions are contents: read + pull-requests: write — no write access to code.

Verification

The workflow YAML parses, the pin is the current github-workflows main (6a374b7), and workflows_ref matches the uses: SHA so prompts and scripts load from the same commit as the definition.

The open question is answered. The original concern was that CURSOR_API_KEY is an org-level secret whose visibility (all repositories vs. a selected list) could not be read without admin:org. Applying the label to this PR settled it empirically: the full panel ran green here — 8 review cells across 4 labs, plus preflight, ledger, consolidate and the Slack notifications — so the secret is visible to this repo and no org-admin change is needed.

Provenance

Authored by: agent-work loop

Verified: ruff check ., ruff format --check ., mypy src and pytest all pass locally (140 passed, 4 skipped); python3 scripts/check_public_repo_hygiene.py reports no internal-only references; the workflow YAML parses under yaml.safe_load; gh repo view Comfy-Org/github-workflows --json visibility returns PUBLIC, which is the sole basis for the hygiene allowlist entry. The extra_generated_globs / diff_excludes semantics, the Gate's label and unblock branches, and the needs.gate.outputs.should_run guard on every downstream job were each read directly from the reusable workflow at the pinned SHA 6a374b7, not assumed. The full PR check rollup is green.

Deviations: One review finding was deferred rather than fixed. A PR opened with the cursor-review label already applied may never be reviewed, because the Gate only accepts action == labeled. There is no caller-side fix — adding opened to types reaches the Gate with an empty label.name and falls through — so closing it requires a change to the shared reusable workflow that affects every consumer repo. It is proposed as a follow-up for triage; the workaround is to remove and re-add the label. All other findings from the review panel were fixed in 5033278.

Thin caller for the reusable cursor-review workflow in
Comfy-Org/github-workflows, which owns the panel, judge, prompts and scripts.
Every other actively-developed repo in the org has one; this repo did not, so
its PRs got no automated review pass.

Triggered by the 'cursor-review' label, which already exists here. Gated two
ways: applying a label in a public repo needs triage permission or higher, and
the reusable workflow's secret-bearing jobs do not run on fork PRs.

diff_excludes restates the reusable default (overriding it replaces the default
wholesale rather than extending it) and adds this repo's lockfile, assets, and
GENERATED client code — the generated tree is the important one, since a
regeneration would otherwise swamp a review with machine-written diff.
@mattmillerai
mattmillerai requested review from a team as code owners August 22, 2026 04:17
@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown

Important

Approval pending

CodeRabbit has no unresolved comments, but it has not reviewed the latest commit.

Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands.

The hygiene check is default-deny on Comfy-Org repo references, so the new
cursor-review caller's two `Comfy-Org/github-workflows` references failed it.

That repo is public -- `gh repo view` reports visibility=PUBLIC -- and it has
to be, because a `uses:` reference to a private reusable workflow cannot
resolve from this repo at all. So this is the false positive the error message
invites resolving by extending the allowlist, not a leak to scrub.

Default-deny is the right posture and is left intact: this adds one confirmed
entry with the verification recorded, rather than loosening the rule.
@mattmillerai mattmillerai added the cursor-review Request an automated Cursor review label Aug 22, 2026
@mattmillerai
mattmillerai enabled auto-merge August 22, 2026 04:27
@mattmillerai
mattmillerai disabled auto-merge August 22, 2026 04:27
@mattmillerai
mattmillerai enabled auto-merge August 22, 2026 04:27

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 6 finding(s).

Severity Count
🟡 Medium 2
🟢 Low 4

Panel: 8/8 reviewers contributed findings.

Comment thread .github/workflows/ci-cursor-review.yml Outdated
Comment thread .github/workflows/ci-cursor-review.yml Outdated
Comment thread .github/workflows/ci-cursor-review.yml Outdated
Comment thread .github/workflows/ci-cursor-review.yml Outdated
Comment thread scripts/check_public_repo_hygiene.py Outdated
Comment thread .github/workflows/ci-cursor-review.yml
Addresses the cursor-review panel findings on #67, verified against the
reusable workflow at the pinned SHA (6a374b7).

- Replace `diff_excludes` with `extra_generated_globs`. `diff_excludes`
  filters only the patch handed to the panel, NOT the `diff_size_cap`
  count, so a `_generated.py` regeneration or a large `assets/` commit
  was hidden from reviewers yet still counted -- tripping the cap and
  skipping the whole review. `extra_generated_globs` feeds the shared
  check-pr-size classifier, which drives both.
- Drop the four lockfile entries: package-lock.json, yarn.lock,
  pnpm-lock.yaml and uv.lock are all classifier built-ins already.
- Narrow `src/comfy_low/models/**` to `src/comfy_low/models/_generated.py`.
  Only that file is emitted by scripts/gen_models.sh; the sibling
  __init__.py is hand-written and must stay visible to the panel.
- Drop the `.claude/**` entry. As a git pathspec `:!**/.claude/**` never
  matched a root-level `.claude/` anyway (no `:(glob)` magic, so the
  literal `/` before `.claude` is still required), and hand-authored
  agent instructions are prose worth reviewing.
- Fix the stale comment: `diff_excludes` defaults to empty upstream, so
  nothing was being "replaced wholesale". `extra_generated_globs` is the
  input that does replace its default, and its defaults are now restated.
- Remove the job-level `if`. It rejected every `unlabeled` event before
  the reusable's Gate saw it, killing the documented unblock path where
  removing `skip-cursor-review` while `cursor-review` is still applied
  re-triggers the review. It also hardcoded a label name the reusable
  owns via its `review_label` input. Every downstream job is already
  gated on `needs.gate.outputs.should_run`, so the only cost is a
  seconds-long Gate no-op on unrelated label events.
- check_public_repo_hygiene.py: drop the circular justification for the
  github-workflows allowlist entry. A private repo can share reusable
  workflows org-internally, so a resolving `uses:` proves nothing; the
  `gh repo view` visibility check is the only valid basis.
@mattmillerai

Copy link
Copy Markdown
Contributor Author

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

  • BE-8659 — cursor-review: run when the trigger label is already present at PR-open time — filed as agent-spike (premise unverified)

The following carry agent-spike instead of agent-ok because their reachability claim was not backed by evidence (BE-5378) — the claim is investigated before any code is written, and "the premise does not hold" is a valid, successful outcome:

  • cursor-review: run when the trigger label is already present at PR-open time — no reachability block in the proposal

@wei-hai
wei-hai disabled auto-merge August 24, 2026 18:16
@wei-hai
wei-hai merged commit 2cd1ece into main Aug 24, 2026
11 checks passed
@wei-hai
wei-hai deleted the chore/cursor-review-caller branch August 24, 2026 18:16
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cursor-review Request an automated Cursor review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants