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 #421] [MERGED] fix(cursor-approve): approve-external accepts an n/a axis like aggregate.py does #421

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/approve-external-accepts-na

📝 Commits (2)

6476d15 fix(cursor-approve): approve-external accepts an n/a axis like aggregate.py does
116a99c test(cursor-approve): parity test covers every yellow cap; document the cap is not re-checked

📊 Changes

2 files changed (+48 additions, -4 deletions)

View changed files

📝 .github/cursor-review/auto-approve.py (+10 -3)
📝 .github/cursor-review/tests/test_approve_external.py (+38 -1)

📄 Description

ELI-5

The approver bot makes its decision in two places. The first place learned that "n/a" (this axis has nothing to check here) shouldn't block a PR. The second place, which double-checks the decision just before posting the approval, never learned that. So every comment-only or docs-only PR got "❌ Not approved" with nothing to fix. This change teaches the second place the same rule.

Description

aggregate.py decide() approves when no axis is red, yellows are within the limit, and not every axis is n/a (#395). auto-approve.py approve-external then re-checks the decision in external_decision_approves(), but that check (written in #350, before n/a existed) still required every axis to be green/yellow. So an APPROVE decision with one n/a axis became not_approved, "the axes did not approve", and a standing REQUEST_CHANGES.

Seen on Comfy-Org/cloud#12992 (round 3, 3150b93). The diff was comments only: correctness n/a, conformance green. The decide job's decision.json had "event": "APPROVE" with reason "no red axis, 0 yellow of at most 0; n/a on correctness (1 axes judged)", but the card said ❌ and told the author to relabel, which can't help because every round fails the same way.

Fix: external_decision_approves() now applies aggregate's rule. n/a counts as reporting and doesn't block, and a round where every axis is n/a still withholds. It is still a re-check against hand-edited or truncated files: a non-APPROVE event, a missing axis or an unknown verdict all still refuse.

Tests

test_agrees_with_aggregate_on_every_verdict_combination runs every 2-axis combination of green/yellow/red/n/a through the real aggregate.decide(), under every yellow cap the CLI accepts for two axes (0 and 1), and checks that external_decision_approves() reaches the same answer. This is the test that would have caught the drift. The cap itself is deliberately not re-checked by approve-external (it is never passed the cap); its docstring now says so.
test_one_na_axis_beside_a_green_one_approves: the #12992 shape goes end-to-end through cmd_approve_external and posts an APPROVE.
test_all_na_withholds_even_if_the_file_says_approve: all-n/a is still refused, even with a forged APPROVE event.
The run_cmd helper's derived event repeated the old rule, so it was updated to match.

Mutation check: with the old external_decision_approves(), 5 subtests fail (all of them the n/a-beside-green/yellow cases). With the fix, test_approve_external passes 51/51, and every other cursor-review/tests and cursor-approve/tests suite passes locally.

Follow-up

Callers pick this up through the usual pin bump (cloud is on 1c7823e, and #417 bumps it to 8d35cca).

Provenance

Authored by: agent-work loop
Verified:
- cursor-review/tests suite passes locally at the current head (1589 tests OK).
- Parity test covers all 16 verdict combinations under yellow caps 0 and 1.
- Doc-drift sweep run against origin/main: no doc describes approve-external re-checking the cap; nothing to update.
Deviations:
- Review round: the cap gap was resolved by documenting it, not by passing the cap through to approve-external.

🤖 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/421 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/approve-external-accepts-na` --- ### 📝 Commits (2) - [`6476d15`](https://github.com/Comfy-Org/github-workflows/commit/6476d154f7d30a4e320da99aadda07e4c28d9326) fix(cursor-approve): approve-external accepts an n/a axis like aggregate.py does - [`116a99c`](https://github.com/Comfy-Org/github-workflows/commit/116a99cda4d8b9806e34dad15445d6facdfbbf53) test(cursor-approve): parity test covers every yellow cap; document the cap is not re-checked ### 📊 Changes **2 files changed** (+48 additions, -4 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/auto-approve.py` (+10 -3) 📝 `.github/cursor-review/tests/test_approve_external.py` (+38 -1) </details> ### 📄 Description ## ELI-5 The approver bot makes its decision in two places. The first place learned that "n/a" (this axis has nothing to check here) shouldn't block a PR. The second place, which double-checks the decision just before posting the approval, never learned that. So every comment-only or docs-only PR got "❌ Not approved" with nothing to fix. This change teaches the second place the same rule. ## Description `aggregate.py decide()` approves when no axis is red, yellows are within the limit, and not every axis is `n/a` (#395). `auto-approve.py approve-external` then re-checks the decision in `external_decision_approves()`, but that check (written in #350, before `n/a` existed) still required **every** axis to be `green`/`yellow`. So an APPROVE decision with one `n/a` axis became `not_approved`, "the axes did not approve", and a standing REQUEST_CHANGES. **Seen on Comfy-Org/cloud#12992** (round 3, `3150b93`). The diff was comments only: correctness `n/a`, conformance `green`. The decide job's `decision.json` had `"event": "APPROVE"` with reason "no red axis, 0 yellow of at most 0; n/a on correctness (1 axes judged)", but the card said ❌ and told the author to relabel, which can't help because every round fails the same way. **Fix:** `external_decision_approves()` now applies aggregate's rule. `n/a` counts as reporting and doesn't block, and a round where every axis is `n/a` still withholds. It is still a re-check against hand-edited or truncated files: a non-APPROVE event, a missing axis or an unknown verdict all still refuse. ## Tests - `test_agrees_with_aggregate_on_every_verdict_combination` runs every 2-axis combination of green/yellow/red/n/a through the real `aggregate.decide()`, under every yellow cap the CLI accepts for two axes (0 and 1), and checks that `external_decision_approves()` reaches the same answer. This is the test that would have caught the drift. The cap itself is deliberately not re-checked by approve-external (it is never passed the cap); its docstring now says so. - `test_one_na_axis_beside_a_green_one_approves`: the #12992 shape goes end-to-end through `cmd_approve_external` and posts an APPROVE. - `test_all_na_withholds_even_if_the_file_says_approve`: all-n/a is still refused, even with a forged APPROVE event. - The `run_cmd` helper's derived event repeated the old rule, so it was updated to match. Mutation check: with the old `external_decision_approves()`, 5 subtests fail (all of them the n/a-beside-green/yellow cases). With the fix, `test_approve_external` passes 51/51, and every other `cursor-review/tests` and `cursor-approve/tests` suite passes locally. ## Follow-up Callers pick this up through the usual pin bump (cloud is on `1c7823e`, and #417 bumps it to `8d35cca`). ## Provenance - **Authored by:** agent-work loop - **Verified:** - `cursor-review/tests` suite passes locally at the current head (1589 tests OK). - Parity test covers all 16 verdict combinations under yellow caps 0 and 1. - Doc-drift sweep run against `origin/main`: no doc describes approve-external re-checking the cap; nothing to update. - **Deviations:** - Review round: the cap gap was resolved by documenting it, not by passing the cap through to approve-external. 🤖 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:38 +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#421

No description provided.

Rows
Columns