Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #361] [MERGED] fix(cursor-approve): honour delta non-gating marks in approve-external thread resolution #361

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: be-19482-approve-external-delta-scope

📝 Commits (3)

5fb4d29 fix(cursor-approve): honour delta non-gating marks in approve-external thread resolution
455498d Merge origin/main into be-19482-approve-external-delta-scope
03e6456 fix(cursor-review): report approve_scope_effective full whenever no decision stands

📊 Changes

8 files changed (+196 additions, -14 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -1)
📝 .github/cursor-review/auto-approve.py (+41 -7)
📝 .github/cursor-review/tests/test_approve_external.py (+16 -3)
📝 .github/cursor-review/tests/test_auto_approve.py (+99 -2)
📝 .github/workflows/cursor-approve.yml (+12 -1)
📝 .github/workflows/cursor-review.yml (+13 -0)
📝 docs/callers/cursor-approve.md (+8 -0)
📝 docs/callers/cursor-review.md (+6 -0)

📄 Description

ELI-5

When cursor-review hands approval off to cursor-approve (defer_approval: true), cursor-approve is also meant to tidy up: it closes the bot's own low-severity review threads. A delta round can leave some above-threshold findings marked "outside this round's changes: not blocking". cursor-review's own approval already ignored those marked threads when deciding whether to close the others. cursor-approve did not, because it had no way to know whether that round really ran as delta. So one marked thread kept every thread open. This PR passes the round's real scope from cursor-review to cursor-approve, so both follow the same rule.

What changed

.github/cursor-review/auto-approve.py
- decide now writes a new step output, approve_scope_effective: the scope the round actually gated under, after resolve_scope and #360's open-thread snapshot. It is written as full first, and it is set to the resolved scope only once decide_gate returns a decision. A new withdraw_decision() sets it back to full whenever the gate is later downgraded to untrusted/capped (a failed POST, a head/base move, a late needs-human-review label, on both the posting path and the deferred path). So every fail-closed fallback (round 1, an unavailable, discarded or empty incremental block, an unparseable path, an unknown ledger, an unreadable list of earlier open threads) and every exit where no decision stands reports full.
- approve-external gains --approve-scope (default full). The value goes through validate_scope, and only delta reaches resolve_eligible_threads as honour_non_gating=True.
.github/workflows/cursor-review.yml: the post-review job forwards that step output, and a new workflow-level output approve_scope_effective exposes it. It falls back to full when no decision ran.
.github/workflows/cursor-approve.yml: new optional approve_scope input (default full, the fail-closed reading), passed as --approve-scope. The header comment documents it.
Docs: docs/callers/cursor-approve.md (inputs table, prose, and the caller example now passes approve_scope: ${{ needs.cursor-review.outputs.approve_scope_effective }}), docs/callers/cursor-review.md (new output row) and .github/cursor-review/README.md (defer_approval row).

Behaviour is unchanged for every existing caller: they do not pass approve_scope, so it is full, which is today's behaviour where a marked thread blocks like any other.

Judgment calls

An unknown --approve-scope value warns and resolves as full; it does not fail the step. At that point the APPROVE has already been posted and re-checked. The existing invalid---threshold path also only warns there, and resolution is never fatal. full is the stricter reading, so a bad value can never widen what gets resolved. When nothing above the threshold is open, threads are still resolved as before.
The workflow output says delta only while a decision stands on a delta scope. cursor-approve's decide phase already runs only after approve_gate == 'pass'. The output still reports full on every no-decision or withdrawn exit (review finding r4204307345), so it never contradicts its documented contract.

Self-review

Riskiest line: scope == SCOPE_DELTA in cmd_approve_external. It is safe because validate_scope lowercases and strips the value and turns empty into full, and the ValueError branch pins it to full. Only an exact delta turns the exemption on. Even then a marked thread is never resolved itself (SKIP_NON_GATING), and an unmarked above-threshold thread still blocks everything (tested).
Call sites: resolve_eligible_threads has exactly two callers, cmd_decide (already passed the flag) and cmd_approve_external (fixed here). The only workflow that runs approve-external is cursor-approve.yml.

Provenance

Authored by: agent-work loop
Verified:
- Tests at the current head (merged with main through #360): .github/cursor-review suite green (1029), .github/cursor-approve suite green, .github/workflow-pins suite green (551, 43 skips are its own "Default-column comparison is off" opt-outs, which includes the declared-vs-documented input drift harness for the new input)
- Mutation-tested: reverting the approve-external flag fails 10 subtests. Restoring the old write-on-resolve placement of the decide output fails 5 subtests (head move, late label, failed POST, judge error, deferred head move)
- Lints: check_workflow_pins.py, check_agents_md.py, the org-repo-literal lint all pass
- actionlint (local v1.7.7) on both changed workflows: no new findings versus main. The remaining findings already exist on main (job.workflow_sha unknown to that version, plus shellcheck info notes), and no CI job here runs actionlint
- Not run: a live cursor-review → cursor-approve run on a real PR
New tests
- ApproveExternalResolveScopeTest runs approve-external through the real resolver with a marked Medium thread plus Low/Nit threads. delta (in any case, with surrounding whitespace) resolves the Low and Nit threads. Omitted, empty or full resolves nothing. An unknown value warns and resolves as full. Under delta an unmarked Medium thread still blocks.
- test_approve_scope_reaches_the_resolver_as_honour_non_gating covers the argument mapping for every value.
- test_scope_effective_is_full_unless_the_round_gated_on_delta checks the decide output for a built delta, delta with REQUEST_CHANGES, a deferred delta pass, round 1, an unavailable block, a missing open-thread snapshot, explicit full, two early validation exits, and the exits after the scope resolves where no decision stands.
Deviations: the ticket's fix sketch implied "validate" might reject an invalid --approve-scope; it warns and falls back to full instead (see Judgment calls)

Residual

Consumer callers must wire the new output to get the fix. It is opt-in by design: approve_scope defaults to full, so a cursor-approve caller sees no change until its decide-phase job passes approve_scope: ${{ needs.cursor-review.outputs.approve_scope_effective }}. Its pin must also be bumped to a SHA that includes this change. The callers live in other repos, which this PR cannot edit. Each caller that runs cursor-review with approve_scope: delta (the default) and defer_approval: true needs a one-line with: addition after its next bump.
Not exercised end to end. Nothing ran a real cursor-review round with defer_approval: true into cursor-approve, so it is not confirmed that approve_scope_effective crosses jobs.post-review.outputs → the workflow output → cursor-approve's input in a live run. The unit tests cover the Python on both ends, and the YAML wiring copies the existing approve_gate output path.

_{🔄 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/361 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/7/2026 **Status:** ✅ Merged **Merged:** 10/7/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `be-19482-approve-external-delta-scope` --- ### 📝 Commits (3) - [`5fb4d29`](https://github.com/Comfy-Org/github-workflows/commit/5fb4d29edbc4c3b6e63fdf63302396c7fe57d057) fix(cursor-approve): honour delta non-gating marks in approve-external thread resolution - [`455498d`](https://github.com/Comfy-Org/github-workflows/commit/455498dfd8ad16aab9ab98d2fe6d72300ddc2087) Merge origin/main into be-19482-approve-external-delta-scope - [`03e6456`](https://github.com/Comfy-Org/github-workflows/commit/03e64569ec6b6f936ba3f8eaca65569e9d5797f2) fix(cursor-review): report approve_scope_effective `full` whenever no decision stands ### 📊 Changes **8 files changed** (+196 additions, -14 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -1) 📝 `.github/cursor-review/auto-approve.py` (+41 -7) 📝 `.github/cursor-review/tests/test_approve_external.py` (+16 -3) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+99 -2) 📝 `.github/workflows/cursor-approve.yml` (+12 -1) 📝 `.github/workflows/cursor-review.yml` (+13 -0) 📝 `docs/callers/cursor-approve.md` (+8 -0) 📝 `docs/callers/cursor-review.md` (+6 -0) </details> ### 📄 Description ## ELI-5 When cursor-review hands approval off to cursor-approve (`defer_approval: true`), cursor-approve is also meant to tidy up: it closes the bot's own low-severity review threads. A delta round can leave some above-threshold findings marked "outside this round's changes: not blocking". cursor-review's own approval already ignored those marked threads when deciding whether to close the others. cursor-approve did not, because it had no way to know whether that round really ran as `delta`. So one marked thread kept every thread open. This PR passes the round's real scope from cursor-review to cursor-approve, so both follow the same rule. ## What changed - `.github/cursor-review/auto-approve.py` - `decide` now writes a new step output, `approve_scope_effective`: the scope the round actually gated under, after `resolve_scope` and #360's open-thread snapshot. It is written as `full` first, and it is set to the resolved scope only once `decide_gate` returns a decision. A new `withdraw_decision()` sets it back to `full` whenever the gate is later downgraded to `untrusted`/`capped` (a failed POST, a head/base move, a late `needs-human-review` label, on both the posting path and the deferred path). So every fail-closed fallback (round 1, an unavailable, discarded or empty incremental block, an unparseable path, an unknown ledger, an unreadable list of earlier open threads) and every exit where no decision stands reports `full`. - `approve-external` gains `--approve-scope` (default `full`). The value goes through `validate_scope`, and only `delta` reaches `resolve_eligible_threads` as `honour_non_gating=True`. - `.github/workflows/cursor-review.yml`: the `post-review` job forwards that step output, and a new workflow-level output `approve_scope_effective` exposes it. It falls back to `full` when no decision ran. - `.github/workflows/cursor-approve.yml`: new optional `approve_scope` input (default `full`, the fail-closed reading), passed as `--approve-scope`. The header comment documents it. - Docs: `docs/callers/cursor-approve.md` (inputs table, prose, and the caller example now passes `approve_scope: ${{ needs.cursor-review.outputs.approve_scope_effective }}`), `docs/callers/cursor-review.md` (new output row) and `.github/cursor-review/README.md` (`defer_approval` row). Behaviour is unchanged for every existing caller: they do not pass `approve_scope`, so it is `full`, which is today's behaviour where a marked thread blocks like any other. ## Judgment calls - **An unknown `--approve-scope` value warns and resolves as `full`; it does not fail the step.** At that point the APPROVE has already been posted and re-checked. The existing invalid-`--threshold` path also only warns there, and resolution is never fatal. `full` is the stricter reading, so a bad value can never widen what gets resolved. When nothing above the threshold is open, threads are still resolved as before. - **The workflow output says `delta` only while a decision stands on a `delta` scope.** cursor-approve's decide phase already runs only after `approve_gate == 'pass'`. The output still reports `full` on every no-decision or withdrawn exit (review finding r4204307345), so it never contradicts its documented contract. ## Self-review - Riskiest line: `scope == SCOPE_DELTA` in `cmd_approve_external`. It is safe because `validate_scope` lowercases and strips the value and turns empty into `full`, and the `ValueError` branch pins it to `full`. Only an exact `delta` turns the exemption on. Even then a marked thread is never resolved itself (`SKIP_NON_GATING`), and an unmarked above-threshold thread still blocks everything (tested). - Call sites: `resolve_eligible_threads` has exactly two callers, `cmd_decide` (already passed the flag) and `cmd_approve_external` (fixed here). The only workflow that runs `approve-external` is `cursor-approve.yml`. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests at the current head (merged with main through #360): `.github/cursor-review` suite green (1029), `.github/cursor-approve` suite green, `.github/workflow-pins` suite green (551, 43 skips are its own "Default-column comparison is off" opt-outs, which includes the declared-vs-documented input drift harness for the new input) - Mutation-tested: reverting the `approve-external` flag fails 10 subtests. Restoring the old write-on-resolve placement of the `decide` output fails 5 subtests (head move, late label, failed POST, judge error, deferred head move) - Lints: `check_workflow_pins.py`, `check_agents_md.py`, the org-repo-literal lint all pass - actionlint (local v1.7.7) on both changed workflows: no new findings versus main. The remaining findings already exist on main (`job.workflow_sha` unknown to that version, plus shellcheck info notes), and no CI job here runs actionlint - Not run: a live cursor-review → cursor-approve run on a real PR <details><summary>New tests</summary> - `ApproveExternalResolveScopeTest` runs approve-external through the real resolver with a marked Medium thread plus Low/Nit threads. `delta` (in any case, with surrounding whitespace) resolves the Low and Nit threads. Omitted, empty or `full` resolves nothing. An unknown value warns and resolves as `full`. Under `delta` an unmarked Medium thread still blocks. - `test_approve_scope_reaches_the_resolver_as_honour_non_gating` covers the argument mapping for every value. - `test_scope_effective_is_full_unless_the_round_gated_on_delta` checks the decide output for a built delta, delta with REQUEST_CHANGES, a deferred delta pass, round 1, an unavailable block, a missing open-thread snapshot, explicit `full`, two early validation exits, and the exits after the scope resolves where no decision stands. </details> - **Deviations:** the ticket's fix sketch implied "validate" might reject an invalid `--approve-scope`; it warns and falls back to `full` instead (see Judgment calls) ## Residual - **Consumer callers must wire the new output to get the fix.** It is opt-in by design: `approve_scope` defaults to `full`, so a cursor-approve caller sees no change until its decide-phase job passes `approve_scope: ${{ needs.cursor-review.outputs.approve_scope_effective }}`. Its pin must also be bumped to a SHA that includes this change. The callers live in other repos, which this PR cannot edit. Each caller that runs cursor-review with `approve_scope: delta` (the default) and `defer_approval: true` needs a one-line `with:` addition after its next bump. - **Not exercised end to end.** Nothing ran a real cursor-review round with `defer_approval: true` into cursor-approve, so it is not confirmed that `approve_scope_effective` crosses `jobs.post-review.outputs` → the workflow output → cursor-approve's input in a live run. The unit tests cover the Python on both ends, and the YAML wiring copies the existing `approve_gate` output path. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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

closed this issue
added the
pull-request
label

No labels

cursor-review-catalog-drift

groom

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/github-workflows#361

No description provided.

Rows
Columns