[PR #359] [MERGED] fix(cursor-review): address approve_scope review findings from #357 (BE-19455) #360

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19455-approve-scope-review-followups

📝 Commits (3)

a30afc3 fix(cursor-review): address approve_scope review findings from #357 (BE-19455)
d33d3b9 fix(cursor-review): fail closed on quoted rename paths and blank approve_scope (BE-19455)
37f1710 Merge remote-tracking branch 'origin/main' into matt/be-19455-approve-scope-review-followups

📊 Changes

6 files changed (+211 additions, -48 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -1)
📝 .github/cursor-review/auto-approve.py (+78 -24)
📝 .github/cursor-review/post-review.py (+18 -1)
📝 .github/cursor-review/tests/test_auto_approve.py (+110 -19)
📝 .github/workflows/cursor-review.yml (+3 -2)
📝 docs/callers/cursor-review.md (+1 -1)

📄 Description

ELI-5

#357 taught auto-approve to ignore some findings that sit outside the newest changes on a PR. The PR merged before its own review panel finished, and the panel then found a few ways that relaxation could let something through by accident. This PR closes the small gaps: whenever the bot can't be sure, it goes back to counting every finding.

What changed

High, model text could re-form the non-gating marker (post-review.py L2522). A nested copy such as  survived the single replace(). A new strip_non_gating() strips the marker and the visible "not blocking" note until the text stops changing.
High, unparseable diff path failed open (auto-approve.py L267). hunk_ranges now returns None when any section's path can't be read, for example a git-C-quoted name. resolve_scope then fails closed to full.
Medium, empty delta laundered outdated threads (auto-approve.py L310). An incremental block with no new-side hunk, such as a pure rebase, now fails closed to full. Before, every non-severe finding became non-gating. The caller guide, README and input description are updated to match.
Medium, plan_thread_resolution honoured the marker under full (auto-approve.py L757). A new honour_non_gating flag is True only when cmd_decide gated under delta. A full round and approve-external now treat a marked thread above the threshold as blocking.
Low, len(reasons) > 1 used as a proxy for the scope note (auto-approve.py L944 / L886). The new scope_note_of() matches the `approve_scope `` prefix, so untrusted trust reasons are never shown as the scope line.
Low, empty --approve-scope meant delta (auto-approve.py L255). An empty value now resolves to full. Only the workflow input's own default: chooses delta.
Low, finding with no usable file failed open (auto-approve.py L334). It now returns no usable file anchor, so it gates.
Low, a marked thread survived only through the threshold comparison (auto-approve.py L756). classify_thread now returns an explicit skipped-non-gating verdict for marked threads, so a looser threshold later on cannot auto-resolve them.

Not changed

Medium, approve_scope defaults to delta (cursor-review.yml L358). Explained on the thread: this is the deliberate purpose of BE-19455 and is documented in the input description, the README and the caller guide. Auto-approve itself stays opt-in through approve_max_severity, and callers who want the old behaviour set approve_scope: full.
Medium, re-raises without repeat_of don't gate (auto-approve.py L325). This is valid but needs a change to the judge contract or a structural thread match, so it's ticketed as BE-19480.
gpt-5.6-sol-max:edge-case errored on #357 and contributed no findings.

Testing

.github/cursor-review: python3 -m unittest discover -s tests ran 1002 tests, OK. There are 7 new regression tests in ApproveScopeTest, and all of them fail against origin/main without this change.
.github/cursor-approve: 149 tests, OK (1 skipped).
.github/workflow-pins: 551 tests, OK (43 skipped).
bash .github/bump-callers/tests/*.sh: 3 of 3 pass.
actionlint .github/workflows/cursor-review.yml: only the known job.workflow_sha and SC2018/SC2019 noise from the old local actionlint.

🤖 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/359 **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:** `matt/be-19455-approve-scope-review-followups` --- ### 📝 Commits (3) - [`a30afc3`](https://github.com/Comfy-Org/github-workflows/commit/a30afc3634b98484d15c4f493bf655336d5aa977) fix(cursor-review): address approve_scope review findings from #357 (BE-19455) - [`d33d3b9`](https://github.com/Comfy-Org/github-workflows/commit/d33d3b9dfff70136b1751aa207ea6e790c819ee4) fix(cursor-review): fail closed on quoted rename paths and blank approve_scope (BE-19455) - [`37f1710`](https://github.com/Comfy-Org/github-workflows/commit/37f171010d198c5ed65f960345d85321b75ae482) Merge remote-tracking branch 'origin/main' into matt/be-19455-approve-scope-review-followups ### 📊 Changes **6 files changed** (+211 additions, -48 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -1) 📝 `.github/cursor-review/auto-approve.py` (+78 -24) 📝 `.github/cursor-review/post-review.py` (+18 -1) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+110 -19) 📝 `.github/workflows/cursor-review.yml` (+3 -2) 📝 `docs/callers/cursor-review.md` (+1 -1) </details> ### 📄 Description ## ELI-5 #357 taught auto-approve to ignore some findings that sit outside the newest changes on a PR. The PR merged before its own review panel finished, and the panel then found a few ways that relaxation could let something through by accident. This PR closes the small gaps: whenever the bot can't be sure, it goes back to counting every finding. ## What changed - **High, model text could re-form the non-gating marker** (`post-review.py` L2522). A nested copy such as `` survived the single `replace()`. A new `strip_non_gating()` strips the marker and the visible "not blocking" note until the text stops changing. - **High, unparseable diff path failed open** (`auto-approve.py` L267). `hunk_ranges` now returns `None` when any section's path can't be read, for example a git-C-quoted name. `resolve_scope` then fails closed to `full`. - **Medium, empty delta laundered outdated threads** (`auto-approve.py` L310). An incremental block with no new-side hunk, such as a pure rebase, now fails closed to `full`. Before, every non-severe finding became non-gating. The caller guide, README and input description are updated to match. - **Medium, `plan_thread_resolution` honoured the marker under `full`** (`auto-approve.py` L757). A new `honour_non_gating` flag is True only when `cmd_decide` gated under `delta`. A `full` round and `approve-external` now treat a marked thread above the threshold as blocking. - **Low, `len(reasons) > 1` used as a proxy for the scope note** (`auto-approve.py` L944 / L886). The new `scope_note_of()` matches the `approve_scope \`` prefix, so untrusted trust reasons are never shown as the scope line. - **Low, empty `--approve-scope` meant `delta`** (`auto-approve.py` L255). An empty value now resolves to `full`. Only the workflow input's own `default:` chooses `delta`. - **Low, finding with no usable `file` failed open** (`auto-approve.py` L334). It now returns `no usable file anchor`, so it gates. - **Low, a marked thread survived only through the threshold comparison** (`auto-approve.py` L756). `classify_thread` now returns an explicit `skipped-non-gating` verdict for marked threads, so a looser threshold later on cannot auto-resolve them. ## Not changed - **Medium, `approve_scope` defaults to `delta`** (`cursor-review.yml` L358). Explained on the thread: this is the deliberate purpose of BE-19455 and is documented in the input description, the README and the caller guide. Auto-approve itself stays opt-in through `approve_max_severity`, and callers who want the old behaviour set `approve_scope: full`. - **Medium, re-raises without `repeat_of` don't gate** (`auto-approve.py` L325). This is valid but needs a change to the judge contract or a structural thread match, so it's ticketed as **BE-19480**. - gpt-5.6-sol-max:edge-case errored on #357 and contributed no findings. ## Testing - `.github/cursor-review`: `python3 -m unittest discover -s tests` ran 1002 tests, OK. There are 7 new regression tests in `ApproveScopeTest`, and all of them fail against `origin/main` without this change. - `.github/cursor-approve`: 149 tests, OK (1 skipped). - `.github/workflow-pins`: 551 tests, OK (43 skipped). - `bash .github/bump-callers/tests/*.sh`: 3 of 3 pass. - `actionlint .github/workflows/cursor-review.yml`: only the known `job.workflow_sha` and SC2018/SC2019 noise from the old local actionlint. 🤖 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

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