[PR #65] [CLOSED] ci: add the team-gated Cursor review caller #67

New issue

Closed

opened 2026-10-31 22:04:16 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:04:16 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/65
Author: @mattmillerai
Created: 8/22/2026
Status: ❌ Closed

Base: main ← Head: chore/cursor-review-caller

📝 Commits (5)

8c2a448 ci: add the team-gated Cursor review caller
83dc03c ci: allowlist the public reusable-workflows repo in the hygiene check
6ea9eda ci: gate via the reusable workflow and size-exempt the generated tree
9b78e2c merge: resolve add/add conflict with main's already-merged Cursor review caller
ed6c5a0 fixup: use main's exact hygiene-allowlist entry, not the PR's duplicate

📄 Description

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

The repo's generated and heavy paths go to extra_generated_globs, not diff_excludes. At the pinned SHA those inputs are not interchangeable: diff_excludes strips paths from the reviewed patch only, while extra_generated_globs feeds the shared check-pr-size classifier and so drops them from the diff_size_cap count as well.

Supplying it replaces the reusable default wholesale, so the shared defaults (node_modules, dist, vendor, *.generated.*, minified output) are restated verbatim before this repo's own paths:

src/low/generated/** — the generated client tree
assets/**

The generated tree is the one that matters. Under diff_excludes a routine regeneration would still be charged its full added+removed churn against the 5000-line cap and could skip the entire panel — including the hand-written changes riding along with it, which is exactly the case the exclusion exists to prevent.

Lockfiles need no entry: the classifier already covers pnpm-lock.yaml among its eight built-in lockfiles.

Gating

There is deliberately no job-level if: — the reusable workflow's gate job owns all gating (fork skip, skip-cursor-review, trigger label, same-commit dedupe), and every downstream job is guarded on needs.gate.outputs.should_run. A caller-side if: on labeled would make the unlabeled trigger dead code, so removing skip-cursor-review would never re-fire the review.

Access control

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

Applying a label in a public repo requires triage permission or higher, so the public cannot trigger this.
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 and the one open question

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.

What I could not verify: CURSOR_API_KEY is an org-level secret — no repo carries it locally, yet the workflow runs green in comfy-toolbox — but reading org secret visibility needs admin:org, which I do not have. So I cannot confirm whether its visibility is all repositories or a selected list that excludes this one.

If it is a selected list, the first labelled run here fails on a missing secret, visibly and with an obvious cause. That is cheap and self-diagnosing, so I would rather land this and let the first run answer it than block on an org-admin check. To test: apply the cursor-review label to this PR.

Provenance

Verified: pnpm test (169 tests, 16 files), pnpm lint, pnpm format:check, pnpm typecheck, and node scripts/check-public-repo-hygiene.mjs all pass locally on the merged tree. The workflow YAML parses and resolves to no if:, no diff_excludes, and the eight-glob extra_generated_globs value. The pathspec defect was confirmed empirically — git ls-files -- ':!**/pnpm-lock.yaml' still lists the root pnpm-lock.yaml, while :!pnpm-lock.yaml excludes it. Input semantics, the classifier's globRegexp, and every downstream job's needs.gate.outputs.should_run guard were read from Comfy-Org/github-workflows at the pinned SHA 6a374b7.

Deviations: None from the review findings, all four of which were confirmed against upstream source and applied. The panel additionally suggested keeping the restated diff_excludes defaults alongside the move; they were dropped instead, since extra_generated_globs restates them and the lockfile entries are classifier built-ins.

_{🔄 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/comfy-typescript-sdk/pull/65 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/22/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `chore/cursor-review-caller` --- ### 📝 Commits (5) - [`8c2a448`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/8c2a44860b8e68ae4e79e171a526bc1dfc016ed9) ci: add the team-gated Cursor review caller - [`83dc03c`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/83dc03c75076e5a8124d317c38a87e5febfe715c) ci: allowlist the public reusable-workflows repo in the hygiene check - [`6ea9eda`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/6ea9eda91f83611614954a30ed799ca4b3f644f0) ci: gate via the reusable workflow and size-exempt the generated tree - [`9b78e2c`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/9b78e2cd4e7fbfe6ec1a7aa4a871d911aee370c6) merge: resolve add/add conflict with main's already-merged Cursor review caller - [`ed6c5a0`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/ed6c5a002d63072459f3e9d1beac1bd155c6306c) fixup: use main's exact hygiene-allowlist entry, not the PR's duplicate ### 📄 Description ## 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 The repo's generated and heavy paths go to **`extra_generated_globs`**, not `diff_excludes`. At the pinned SHA those inputs are not interchangeable: `diff_excludes` strips paths from the reviewed patch *only*, while `extra_generated_globs` feeds the shared `check-pr-size` classifier and so drops them from the **`diff_size_cap` count as well**. Supplying it replaces the reusable default wholesale, so the shared defaults (`node_modules`, `dist`, `vendor`, `*.generated.*`, minified output) are restated verbatim before this repo's own paths: - `src/low/generated/**` — the generated client tree - `assets/**` The generated tree is the one that matters. Under `diff_excludes` a routine regeneration would still be charged its full added+removed churn against the 5000-line cap and could skip the **entire panel** — including the hand-written changes riding along with it, which is exactly the case the exclusion exists to prevent. Lockfiles need no entry: the classifier already covers `pnpm-lock.yaml` among its eight built-in lockfiles. ## Gating There is deliberately **no job-level `if:`** — the reusable workflow's `gate` job owns all gating (fork skip, `skip-cursor-review`, trigger label, same-commit dedupe), and every downstream job is guarded on `needs.gate.outputs.should_run`. A caller-side `if:` on `labeled` would make the `unlabeled` trigger dead code, so removing `skip-cursor-review` would never re-fire the review. ## 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 and the one open question 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. **What I could not verify:** `CURSOR_API_KEY` is an **org-level** secret — no repo carries it locally, yet the workflow runs green in `comfy-toolbox` — but reading org secret visibility needs `admin:org`, which I do not have. So I cannot confirm whether its visibility is *all repositories* or a *selected* list that excludes this one. If it is a selected list, the first labelled run here fails on a missing secret, visibly and with an obvious cause. That is cheap and self-diagnosing, so I would rather land this and let the first run answer it than block on an org-admin check. **To test: apply the `cursor-review` label to this PR.** ## Provenance **Verified:** `pnpm test` (169 tests, 16 files), `pnpm lint`, `pnpm format:check`, `pnpm typecheck`, and `node scripts/check-public-repo-hygiene.mjs` all pass locally on the merged tree. The workflow YAML parses and resolves to no `if:`, no `diff_excludes`, and the eight-glob `extra_generated_globs` value. The pathspec defect was confirmed empirically — `git ls-files -- ':!**/pnpm-lock.yaml'` still lists the root `pnpm-lock.yaml`, while `:!pnpm-lock.yaml` excludes it. Input semantics, the classifier's `globRegexp`, and every downstream job's `needs.gate.outputs.should_run` guard were read from `Comfy-Org/github-workflows` at the pinned SHA `6a374b7`. **Deviations:** None from the review findings, all four of which were confirmed against upstream source and applied. The panel additionally suggested keeping the restated `diff_excludes` defaults alongside the move; they were dropped instead, since `extra_generated_globs` restates them and the lockfile entries are classifier built-ins. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:04:16 +00:00