[PR #410] [MERGED] feat(cursor-review): accept pull_request_target, pin PR checkout refs, add approver_environment #412

New issue

Closed

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

bsakaguchi commented

2026-10-26 22:39:31 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/410
Author: @mattmillerai
Created: 10/11/2026
Status: ✅ Merged
Merged: 10/11/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-20107-cursor-review-pull-request-target

📝 Commits (3)

78024b9 feat(cursor-review): accept pull_request_target, pin PR checkout refs, add approver_environment
a653bf0 docs(cursor-review): align approve callers with the pull_request_target token rule
03a5220 fix(cursor-review): approve only from pull_request_target, pin PR checkouts to head.sha

📊 Changes

6 files changed (+339 additions, -37 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -0)
📝 .github/cursor-review/tests/test_workflow_job_isolation.py (+72 -0)
📝 .github/workflows/cursor-approve.yml (+37 -3)
📝 .github/workflows/cursor-review.yml (+127 -6)
📝 docs/callers/cursor-approve.md (+21 -1)
📝 docs/callers/cursor-review.md (+81 -27)

📄 Description

ELI-5

Today, a PR author who can push a branch can also edit the workflow that holds the approver token, because pull_request runs the caller workflow from the PR head. This change lets cursor-review and cursor-approve run from pull_request_target, where the caller comes from the base branch. It also adds an approver_environment input so the token can live in a base-branch-only environment. The change is additive: existing pull_request callers still run.

What changed

Events: pull_request_target is accepted everywhere pull_request was tested: the workflow-level concurrency expression (run_without_label branch), the dismiss-stale-approval if:, and the blocking-gate error text. EVENT_NAME is used only in that message. cursor-approve reads only github.event.pull_request.* (number, head sha, base ref), and that payload is present under both events.
Checkout refs: the three ref-less Checkout PR repo steps (review, judge-direct, consolidate) now pin github.event.pull_request.head.sha, as review-anthropic-direct and diff-size already did. The merge ref was mutable between jobs and missing on conflicted PRs, which pull_request_target still runs on.
approver_environment (string, default ''): declared on exactly post-review, dismiss-stale-approval, auto-retry (cursor-review) and approve (cursor-approve), and only under pull_request_target (environment: ${{ github.event_name == 'pull_request_target' && inputs.approver_environment || '' }}). Any other event runs on refs/pull/<n>/merge, which a base-branch-only deployment policy would reject before a step runs.
Guard: only pull_request_target may approve as APPROVER_TOKEN. On any other event with the token set (pull_request, pull_request_review_thread, …), the token steps print an ::error:: and approve nothing. The decision keeps the token identity under --defer-approval, so it still withdraws that identity's own earlier approvals. cursor-approve outputs withdraw_only and runs only Withdraw approvals. auto-retry drops to the fallback identity. With approver_environment set, a non-target run has no token and posts no approval under any identity. Under pull_request_target with no approver_environment, the steps warn. No posting job turns red.
Gate: the fork skip fails closed (an empty head repo on a PR event, i.e. a deleted fork, skips too). Under pull_request_target a Dependabot-authored PR is skipped, because it would otherwise get the repo's real secrets.
Docs: ### Trust model is now a real heading, so #trust-model resolves, and it is rewritten to the pull_request_target + environment shape, with the fork-skip and Dependabot notes. Also updated: the "drop the trigger" residual, the cursor-approve caller example, both inputs tables and the cursor-review.yml header example. Review round: the cursor-approve caller example adds edited (so a base retarget reaches dismiss-stale-approval) with its own concurrency group; the auto-approve example moves to pull_request_target + approver_environment; and the trust model tells public repositories to allow pull_request_target through an Actions event policy before switching (GitHub's default blocks it, enforced from November 2, 2026). Panel round: the trust model says thread resolution does not approve in this shape, and documents the base-branch Actions cache scope. The cursor-approve section explains why it has no pull_request_review_thread trigger.
Tests: two additions to test_workflow_job_isolation.py. One checks that the conditional environment key appears only on the APPROVER_TOKEN jobs (in both workflows) and never on a PR-checkout job. The other checks that every PR checkout pins ref: head.sha.

Probe: is an empty `environment:` accepted?

Yes. I ran this on a scratch branch (probe/…-empty-environment, deleted afterwards) with a push-triggered workflow:

inline job: environment: ${{ github.event_name == 'never' && 'x' || '' }} → success.
called / token-job: a local reusable whose job sets environment: ${{ inputs.env_name }}, called with env_name: '' → success.
Afterwards the repo's environments list was unchanged (["copilot"], so no '' environment was auto-created), and deployments?ref=<probe branch> returned 0. So the default: '' design holds, and neither fallback from the spec (a default: 'cursor-approver' or a split job) was needed.

Judgment calls

Untrusted events keep the token identity to withdraw, never to approve. The spec said "do not act as the token". The first cut switched the decision to the fallback identity, which stranded approvals the token had already posted (panel finding). Now post-review's decision, dismiss-stale-approval and cursor-approve keep the token login only to withdraw, and to request changes in the decision. They never approve. Only auto-retry drops to the fallback, because a relabel approves nothing.
Behavior change for existing pull_request callers that pass APPROVER_TOKEN. Their runs still go green, but no auto-approval is posted until the caller moves to pull_request_target (on any event, including thread resolution). The decision card says "approval is left to cursor-approve's axes" in that case, because it reuses the defer_approval path. The ::error:: annotation gives the real reason.
Thread-resolution approval is gone under the trust model. A pull_request_review_thread caller comes from the PR, so it may not approve as the token. The blocking caller's approval now waits for the next pull_request_target event. Routing thread resolution through a trusted trigger would be a separate design.
Dependabot skip is enforced in gate, not left to the caller, under pull_request_target only.
Warning wording. The pull_request_target-without-environment warning is worded for that case rather than copying the pull_request error verbatim, because "under a pull_request trigger" would be false there.

Residual

Not exercised: an environment secret reaching the reusable. This change assumes that when the reusable's job declares environment:, it receives the caller's environment secret APPROVER_TOKEN. GitHub's reusable-workflow docs say "the environment secret will be used". I have not run that end to end (I created no environment or secret). The first caller migration should confirm the approval posts under the token identity.
Not migrated: existing callers. No caller is changed here (callers, rulesets and the bot App key residual are out of scope).
Not run end to end: any pull_request_target or review_thread run. The guard and environment behavior is checked by static tests and actionlint only.

Provenance

Authored by: agent-work loop
Verified:
- Tests: .github/cursor-review/tests (1460) OK, .github/cursor-approve/tests OK, .github/workflow-pins/tests OK (43 skipped)
- Lints: check_workflow_pins.py OK, check-org-repo-literals.sh OK, check_agents_md.py passed (2 warnings, pre-existing)
- actionlint (with shellcheck) on both workflows: no findings beyond main's existing set
- Empty-environment: probe: run 38098449898, both jobs green, no environment or deployment created
- pull_request_target default-policy dates checked against GitHub's "Securely using pull_request_target" docs
- Not run: an end-to-end pull_request_target caller with a real environment secret
Deviations: untrusted events keep the token identity to withdraw but never approve, rather than dropping it entirely (see Judgment calls).

🤖 Generated with Claude Code

_{🔄 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/410 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/11/2026 **Status:** ✅ Merged **Merged:** 10/11/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-20107-cursor-review-pull-request-target` --- ### 📝 Commits (3) - [`78024b9`](https://github.com/Comfy-Org/github-workflows/commit/78024b97702c1e2b4adfdffbc6d98f4dc273f246) feat(cursor-review): accept pull_request_target, pin PR checkout refs, add approver_environment - [`a653bf0`](https://github.com/Comfy-Org/github-workflows/commit/a653bf0c676dbabcf432f826715e4a4f5ce8a45c) docs(cursor-review): align approve callers with the pull_request_target token rule - [`03a5220`](https://github.com/Comfy-Org/github-workflows/commit/03a52206ed2167187ff9010f16df08be413db8e1) fix(cursor-review): approve only from pull_request_target, pin PR checkouts to head.sha ### 📊 Changes **6 files changed** (+339 additions, -37 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -0) 📝 `.github/cursor-review/tests/test_workflow_job_isolation.py` (+72 -0) 📝 `.github/workflows/cursor-approve.yml` (+37 -3) 📝 `.github/workflows/cursor-review.yml` (+127 -6) 📝 `docs/callers/cursor-approve.md` (+21 -1) 📝 `docs/callers/cursor-review.md` (+81 -27) </details> ### 📄 Description ## ELI-5 Today, a PR author who can push a branch can also edit the workflow that holds the approver token, because `pull_request` runs the caller workflow from the PR head. This change lets cursor-review and cursor-approve run from `pull_request_target`, where the caller comes from the base branch. It also adds an `approver_environment` input so the token can live in a base-branch-only environment. The change is additive: existing `pull_request` callers still run. ## What changed - **Events:** `pull_request_target` is accepted everywhere `pull_request` was tested: the workflow-level concurrency expression (`run_without_label` branch), the `dismiss-stale-approval` `if:`, and the blocking-gate error text. `EVENT_NAME` is used only in that message. cursor-approve reads only `github.event.pull_request.*` (number, head sha, base ref), and that payload is present under both events. - **Checkout refs:** the three ref-less `Checkout PR repo` steps (`review`, `judge-direct`, `consolidate`) now pin `github.event.pull_request.head.sha`, as `review-anthropic-direct` and `diff-size` already did. The merge ref was mutable between jobs and missing on conflicted PRs, which `pull_request_target` still runs on. - **`approver_environment`** (string, default `''`): declared on exactly `post-review`, `dismiss-stale-approval`, `auto-retry` (cursor-review) and `approve` (cursor-approve), and only under `pull_request_target` (`environment: ${{ github.event_name == 'pull_request_target' && inputs.approver_environment || '' }}`). Any other event runs on `refs/pull/<n>/merge`, which a base-branch-only deployment policy would reject before a step runs. - **Guard:** only `pull_request_target` may approve as `APPROVER_TOKEN`. On any other event with the token set (`pull_request`, `pull_request_review_thread`, …), the token steps print an `::error::` and approve nothing. The decision keeps the token identity under `--defer-approval`, so it still withdraws that identity's own earlier approvals. cursor-approve outputs `withdraw_only` and runs only `Withdraw approvals`. `auto-retry` drops to the fallback identity. With `approver_environment` set, a non-target run has no token and posts no approval under any identity. Under `pull_request_target` with no `approver_environment`, the steps warn. No posting job turns red. - **Gate:** the fork skip fails closed (an empty head repo on a PR event, i.e. a deleted fork, skips too). Under `pull_request_target` a Dependabot-authored PR is skipped, because it would otherwise get the repo's real secrets. - **Docs:** `### Trust model` is now a real heading, so `#trust-model` resolves, and it is rewritten to the `pull_request_target` + environment shape, with the fork-skip and Dependabot notes. Also updated: the "drop the trigger" residual, the cursor-approve caller example, both inputs tables and the cursor-review.yml header example. Review round: the cursor-approve caller example adds `edited` (so a base retarget reaches `dismiss-stale-approval`) with its own concurrency group; the auto-approve example moves to `pull_request_target` + `approver_environment`; and the trust model tells public repositories to allow `pull_request_target` through an Actions event policy before switching (GitHub's default blocks it, enforced from November 2, 2026). Panel round: the trust model says thread resolution does not approve in this shape, and documents the base-branch Actions cache scope. The cursor-approve section explains why it has no `pull_request_review_thread` trigger. - **Tests:** two additions to `test_workflow_job_isolation.py`. One checks that the conditional environment key appears only on the APPROVER_TOKEN jobs (in both workflows) and never on a PR-checkout job. The other checks that every PR checkout pins `ref: head.sha`. ## Probe: is an empty `environment:` accepted? **Yes.** I ran this on a scratch branch (`probe/…-empty-environment`, deleted afterwards) with a `push`-triggered workflow: - `inline` job: `environment: ${{ github.event_name == 'never' && 'x' || '' }}` → **success**. - `called / token-job`: a local reusable whose job sets `environment: ${{ inputs.env_name }}`, called with `env_name: ''` → **success**. - Afterwards the repo's environments list was unchanged (`["copilot"]`, so no `''` environment was auto-created), and `deployments?ref=<probe branch>` returned 0. So the `default: ''` design holds, and neither fallback from the spec (a `default: 'cursor-approver'` or a split job) was needed. ## Judgment calls - **Untrusted events keep the token identity to withdraw, never to approve.** The spec said "do not act as the token". The first cut switched the decision to the fallback identity, which stranded approvals the token had already posted (panel finding). Now `post-review`'s decision, `dismiss-stale-approval` and cursor-approve keep the token login only to withdraw, and to request changes in the decision. They never approve. Only `auto-retry` drops to the fallback, because a relabel approves nothing. - **Behavior change for existing `pull_request` callers that pass `APPROVER_TOKEN`.** Their runs still go green, but no auto-approval is posted until the caller moves to `pull_request_target` (on any event, including thread resolution). The decision card says "approval is left to cursor-approve's axes" in that case, because it reuses the `defer_approval` path. The `::error::` annotation gives the real reason. - **Thread-resolution approval is gone under the trust model.** A `pull_request_review_thread` caller comes from the PR, so it may not approve as the token. The blocking caller's approval now waits for the next `pull_request_target` event. Routing thread resolution through a trusted trigger would be a separate design. - **Dependabot skip is enforced in `gate`**, not left to the caller, under `pull_request_target` only. - **Warning wording.** The `pull_request_target`-without-environment warning is worded for that case rather than copying the `pull_request` error verbatim, because "under a pull_request trigger" would be false there. ## Residual - **Not exercised: an environment secret reaching the reusable.** This change assumes that when the reusable's job declares `environment:`, it receives the caller's *environment* secret `APPROVER_TOKEN`. GitHub's reusable-workflow docs say "the environment secret will be used". I have not run that end to end (I created no environment or secret). The first caller migration should confirm the approval posts under the token identity. - **Not migrated: existing callers.** No caller is changed here (callers, rulesets and the bot App key residual are out of scope). - **Not run end to end: any `pull_request_target` or review_thread run.** The guard and environment behavior is checked by static tests and actionlint only. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: `.github/cursor-review/tests` (1460) OK, `.github/cursor-approve/tests` OK, `.github/workflow-pins/tests` OK (43 skipped) - Lints: `check_workflow_pins.py` OK, `check-org-repo-literals.sh` OK, `check_agents_md.py` passed (2 warnings, pre-existing) - actionlint (with shellcheck) on both workflows: no findings beyond `main`'s existing set - Empty-`environment:` probe: run 38098449898, both jobs green, no environment or deployment created - `pull_request_target` default-policy dates checked against GitHub's "Securely using pull_request_target" docs - Not run: an end-to-end `pull_request_target` caller with a real environment secret - **Deviations:** untrusted events keep the token identity to withdraw but never approve, rather than dropping it entirely (see Judgment calls). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-26 22:39:31 +00:00

bsakaguchi referenced this issue

2026-10-26 22:39:31 +00:00

[PR #413] [MERGED] feat(cursor-approve): show each axis's full first sentence and its full reasoning on the card (BE-20249) #413

bsakaguchi closed this issue

2026-10-26 22:39:31 +00:00

bsakaguchi referenced this issue

2026-10-26 22:39:32 +00:00

[PR #412] [MERGED] feat(cursor-review): link every auto-approve review to the cursor-approve card (BE-20246) #415