diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index cf9c3715..740b8bfa 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -427,7 +427,7 @@ All optional except `workflows_ref` (required, no default) — pass them under | `defer_approval` | `false` | For a caller that runs cursor-approve after this workflow: an APPROVE outcome still reports `approve_gate` = `pass` but posts no approval and resolves no thread, and withdraws the approver's own earlier auto-approvals and requests for changes, so cursor-approve's decide is the only approver. Pass the workflow's `approve_scope_effective` output (the scope `decide` actually gated under, `full` on any fallback) to cursor-approve's `approve_scope`, so its resolution honours non-gating marks only after a `delta` round. | | `approve_max_failed_reviewers` | `0` | Errored panel reviewers (status `error`) auto-approve tolerates before withholding its decision. `0` → any reviewer that did not complete means no decision. Never tolerates another status, more than N, or a review type with no completed reviewer; tolerated cells are named in the decision. | | `rereview_command` | `''` (off) | A comment command, e.g. `/review`, asking for another round on the **same commit**. Empty leaves the same-commit dedupe absolute. Needs the SAME value on [`cursor-review-auto-label.yml`](../../docs/callers/cursor-review-auto-label.md) plus `issue_comment` in that caller's triggers: that half re-applies the label (removing it first, since adding a label already present raises no event), this half stops the dedupe from skipping the round it starts. Setting one alone is a silent no-op. The command must be the first token of the first line, and the commenter's association must be `OWNER`, `MEMBER` or `COLLABORATOR` — not the opted-in roster. The request must postdate the review it overrides, so one request buys one round; `max_rounds` still applies. Exists because the documented alternative cannot work: the consolidated review is `COMMENTED`, which GitHub will not dismiss. See [the setup guide](../../docs/callers/cursor-review.md#asking-for-another-round-on-the-same-commit). | -| `auto_retry_errored_rounds` | `false` | Opt-in: re-run a round the PANEL broke (reviewer cells `error`) rather than leaving a standing block for a human to relabel. Bounded by the same one-per-PR budget as the moved-head retry (`AUTO_RETRY_MARKER` on decide's own block), by `max_rounds`, and by `can_relabel`. Never fires for a structural cause or for findings above the threshold. Its force signal at the gate is that marked block — decide's own shape, not dismissed, newer (by review id) than the newest consolidated review for this commit, and authored by a Bot or by the identity that applied the label (the APPROVER_TOKEN case) — NOT the `rereview_command` comment path, which stays authorized by comment association alone. No effect without `approve_max_severity`. | +| `auto_retry_errored_rounds` | `false` | Opt-in: re-run a round the PANEL broke (reviewer cells `error`) rather than leaving a standing block for a human to relabel. Bounded by the same one-per-PR budget as the moved-head retry (`AUTO_RETRY_MARKER` on decide's own block), by `max_rounds`, and by `can_relabel`. Never fires for a structural cause, findings above the threshold, or a zero-completed-reviewer panel: all cells failing together is treated as a systemic service, credential or account outage until a human verifies recovery. Its force signal at the gate is that marked block — decide's own shape, not dismissed, newer (by review id) than the newest consolidated review for this commit, and authored by a Bot or by the identity that applied the label (the APPROVER_TOKEN case) — NOT the `rereview_command` comment path, which stays authorized by comment association alone. No effect without `approve_max_severity`. | | `auto_retry_delay_minutes` | `0` | Minutes an automatic retry waits before relabelling (`0` = immediately, the prior behaviour). Clamped to 30 in the workflow and again in `card.py`'s sentence, so the card cannot promise a longer wait than the one taken; unparsable → no wait. Costs runner-minutes while it sleeps. No effect without `approve_max_severity` and `auto_retry_errored_rounds` (or a moved-head retry). | | `auto_rereview_after_dismissal` | `false` | When `dismiss-stale-approval` finds the card approving a commit that is no longer the head, start the next round itself: the job sets `auto_rereview`, and the `auto-rereview` job waits, re-reads the PR, the card and its timeline (`auto-approve.py auto-rereview`), and relabels once per head. The card is marked stale whether or not this is on (`mark_card_stale`). | | `auto_rereview_delay_minutes` | `10` | The `auto-rereview` job's debounce, clamped to 30 in the workflow and in `card.py`'s sentence; unparsable → no wait. | diff --git a/.github/cursor-review/auto-approve.py b/.github/cursor-review/auto-approve.py index a3ea3c76..f97ed07d 100644 --- a/.github/cursor-review/auto-approve.py +++ b/.github/cursor-review/auto-approve.py @@ -629,6 +629,26 @@ def is_panel_incomplete_reason(reason) -> bool: return isinstance(reason, str) and PANEL_INCOMPLETE_SUFFIX in reason +def all_panel_reviewers_failed(reason) -> bool: + """True only for the produced ``N/N panel reviewers did not complete`` shape. + + One or a few failed cells are plausibly transient provider capacity. Every + counted cell failing together is a systemic service, credential or account + failure until proven otherwise. An automatic retry would spend the PR's + retry budget and trigger label while the shared dependency is still down. + """ + if not isinstance(reason, str): + return False + match = re.fullmatch( + rf"([0-9]+)/([0-9]+) {re.escape(PANEL_INCOMPLETE_SUFFIX)}(?:,.*)?", + reason, + ) + if not match: + return False + failed, total = (int(value) for value in match.groups()) + return total > 0 and failed == total + + def panel_gate(panel: list, max_failed: int = 0): """(withhold_reason or None, tolerated_cells). Pure. @@ -1117,6 +1137,12 @@ def no_decision_next(reasons: list, label: str = ""): as COMMENT, which GitHub will not dismiss, so that clause is unreachable. """ card = _load_card() + if any(all_panel_reviewers_failed(reason) for reason in reasons or []): + return card.NEXT_HUMAN, ( + "A human is needed: zero reviewers completed, which indicates a systemic service, " + "credential or account failure. Verify that the shared dependency recovered before " + "spending another review round." + ) causes = [cause for needle, cause in STRUCTURAL_CAUSES if any(isinstance(r, str) and needle in r for r in reasons or [])] if causes: @@ -1191,6 +1217,12 @@ def auto_retry_eligible(reasons: list, retry_errored: bool = False, blocked_anyw reasons = [r for r in (reasons or []) if isinstance(r, str) and r.strip()] if not reasons: return False + if any(all_panel_reviewers_failed(reason) for reason in reasons): + # A partial panel can be a flaky cell. Zero completed cells across the + # matrix is shared-dependency evidence, and immediately relabelling is + # the one recovery that predictably burns another round during an + # account-wide outage. + return False if REASON_HEAD_MOVED in reasons and all(r in RETRYABLE_CAUSES for r in reasons): return True if not retry_errored: diff --git a/.github/cursor-review/tests/test_auto_approve.py b/.github/cursor-review/tests/test_auto_approve.py index 20278bb2..43639fd1 100644 --- a/.github/cursor-review/tests/test_auto_approve.py +++ b/.github/cursor-review/tests/test_auto_approve.py @@ -3820,7 +3820,16 @@ def test_only_an_explicit_can_relabel_true_retries(self): # moved. Retrying it was impossible until the gate learned to re-run a # commit it had already reviewed, so this whole section is off by default. - PANEL_ERR = [{"model": "m", "review_type": "adversarial", "status": "error"}] + # One flaky cell in an otherwise usable panel. Keep this distinct from an + # all-zero panel, which is a systemic-outage signal and must not retry. + PANEL_ERR = [ + {"model": "m1", "review_type": "adversarial", "status": "error"}, + {"model": "m2", "review_type": "edge-case", "status": "ok"}, + ] + PANEL_ZERO = [ + {"model": "m1", "review_type": "adversarial", "status": "error"}, + {"model": "m2", "review_type": "edge-case", "status": "error"}, + ] def test_an_errored_panel_is_not_retried_by_default(self): self.run_decide(head=SHA, panel=self.PANEL_ERR) @@ -3838,6 +3847,14 @@ def test_an_errored_panel_is_retried_when_the_caller_opts_in(self): self.assertIn(AA.AUTO_RETRY_MARKER, body) self.assertIn("re-run automatically", body) + def test_a_zero_completed_panel_routes_to_a_human_even_with_the_opt_in(self): + self.run_decide(head=SHA, panel=self.PANEL_ZERO, retry_errored="true") + self.assertFalse(self.retried()) + body = self.reviews_posted[0]["body"] + self.assertNotIn(AA.AUTO_RETRY_MARKER, body) + self.assertIn("zero reviewers completed", body) + self.assertNotIn("remove and re-add", body) + def test_the_opt_in_does_not_widen_the_other_causes(self): # Still structural, still a judge failure the panel cannot fix. self.run_decide(head=SHA, delivered="false", retry_errored="true") diff --git a/.github/cursor-review/tests/test_auto_retry_errored.py b/.github/cursor-review/tests/test_auto_retry_errored.py index 6c611b8f..dd60c664 100644 --- a/.github/cursor-review/tests/test_auto_retry_errored.py +++ b/.github/cursor-review/tests/test_auto_retry_errored.py @@ -126,6 +126,7 @@ class EligibilityTest(unittest.TestCase): """The whole truth table, because the dangerous answers are the `True`s.""" ERRORED = "2/6 " + auto_approve.PANEL_INCOMPLETE_SUFFIX + ZERO_COMPLETED = "6/6 " + auto_approve.PANEL_INCOMPLETE_SUFFIX def eligible(self, reasons, on): return auto_approve.auto_retry_eligible(reasons, retry_errored=on) @@ -148,6 +149,33 @@ def test_an_errored_panel_needs_the_flag(self): self.assertFalse(self.eligible([self.ERRORED], False)) self.assertTrue(self.eligible([self.ERRORED], True)) + def test_a_zero_completed_panel_is_never_auto_retried(self): + # Every cell failing together is a systemic outage until proven + # otherwise. Relabelling here spends the retry and another panel on + # the same unavailable account. + self.assertFalse(self.eligible([self.ZERO_COMPLETED], True)) + self.assertFalse(self.eligible([self.ZERO_COMPLETED, HEAD_MOVED], True)) + + def test_only_a_real_all_failed_reason_trips_the_circuit_breaker(self): + self.assertTrue(auto_approve.all_panel_reviewers_failed(self.ZERO_COMPLETED)) + for reason in ( + self.ERRORED, + "6/7 " + auto_approve.PANEL_INCOMPLETE_SUFFIX, + "0/6 " + auto_approve.PANEL_INCOMPLETE_SUFFIX, + "6/6 unrelated failure", + "no panel metadata", + "", + None, + ): + with self.subTest(reason=reason): + self.assertFalse(auto_approve.all_panel_reviewers_failed(reason)) + + def test_a_zero_completed_panel_does_not_tell_a_human_to_relabel(self): + marker, message = auto_approve.no_decision_next([self.ZERO_COMPLETED]) + self.assertEqual(card.NEXT_HUMAN, marker) + self.assertIn("zero reviewers completed", message) + self.assertNotIn("relabel", message.lower()) + def test_errored_alongside_a_race_is_eligible(self): self.assertTrue(self.eligible([self.ERRORED, HEAD_MOVED], True)) self.assertTrue(self.eligible([self.ERRORED, BASE_CHANGED], True)) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index a5661710..b43e83c3 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -305,7 +305,10 @@ on: once per PR (the existing auto-retry ceiling), the status card says so before it happens, and `max_rounds` still applies; a round that failed for a structural reason, or that found something above the threshold, - is never re-run because the next round would hit the same wall. + is never re-run because the next round would hit the same wall. A + zero-completed-reviewer round is also never re-run automatically: all + cells failing together is treated as a systemic service, credential + or account outage until a human verifies recovery. Independent of `rereview_command`, which is the HUMAN path: that one keys on an authorized comment, and the retry posts no comment. The retry's own force signal is the marked block decide already writes diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 317d30a3..8b3ea62a 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -140,7 +140,7 @@ pull-requests: write # posting the consolidated review (and, at the round cap, | `defer_approval` | `false` | Only for a caller that runs [cursor-approve](cursor-approve.md) after this workflow: a round that would approve still reports `approve_gate` = `pass`, but posts **no** approval and resolves no thread, and withdraws the approver's own earlier auto-approvals and requests for changes instead — so cursor-approve's decide is the only approver. Requests changes as usual. No effect without `approve_max_severity`. | | `approve_max_failed_reviewers` | `0` | How many panel reviewers may **error** (the cell ran but failed, timed out, or never uploaded) before auto-approve withholds its decision. `0` keeps the strict rule: any reviewer that did not complete means no decision. `N` tolerates up to N errored reviewers and names them in the decision (e.g. "approved with 1/6 reviewers errored: `:edge-case`"). See [auto-approve](#auto-approve). No effect without `approve_max_severity`. | | `rereview_command` | `''` (off) | A comment command, e.g. `/review`, that asks for ANOTHER round on the **same commit**. Empty leaves the same-commit dedupe absolute. Set the same value on [`cursor-review-auto-label.yml`](cursor-review-auto-label.md) — that half turns the comment into the label event, this half stops the dedupe from skipping the round it starts; setting only one is a silent no-op. Authorized = an `OWNER` / `MEMBER` / `COLLABORATOR` association on the comment. One request buys one round, and `max_rounds` still applies. See [below](#asking-for-another-round-on-the-same-commit). | -| `auto_retry_errored_rounds` | `false` | Re-run a round AUTOMATICALLY when it could not decide because **reviewers errored** — provider capacity, a cell timeout — rather than because of anything in the diff. The status card and the standing block say so before it happens, naming the wait. Once per PR (the existing auto-retry ceiling), and `max_rounds` still applies. A round that failed for a structural reason, or that found something above the threshold, is never re-run: the next round would hit the same wall. Needs a token that can relabel — the `bot_app_id` App token or `APPROVER_TOKEN`, not `GITHUB_TOKEN`, whose label edits fire no run. Independent of `rereview_command`: the retry posts no comment and is not authorized by one. No effect without `approve_max_severity`. See [below](#retrying-a-round-the-panel-broke). | +| `auto_retry_errored_rounds` | `false` | Re-run a round AUTOMATICALLY when it could not decide because **reviewers errored** — provider capacity, a cell timeout — rather than because of anything in the diff. The status card and the standing block say so before it happens, naming the wait. Once per PR (the existing auto-retry ceiling), and `max_rounds` still applies. A round that failed for a structural reason, found something above the threshold, or completed zero reviewer cells is never re-run: all cells failing together is treated as a systemic service, credential or account outage until a human verifies recovery. Needs a token that can relabel — the `bot_app_id` App token or `APPROVER_TOKEN`, not `GITHUB_TOKEN`, whose label edits fire no run. Independent of `rereview_command`: the retry posts no comment and is not authorized by one. No effect without `approve_max_severity`. See [below](#retrying-a-round-the-panel-broke). | | `auto_retry_delay_minutes` | `0` | How long an automatic retry waits before relabelling, in minutes — provider capacity is the usual cause, and a few minutes is what makes the second attempt different from the first. `0` relabels immediately, which is what every caller had before this input. The wait happens on a runner, so it costs runner-minutes on a private repo; that is the whole cost. Clamped to `30`; anything unparsable waits not at all. No effect without `approve_max_severity`. | | `auto_rereview_after_dismissal` | `false` | When a push dismisses the approver's approval, start the next round **automatically** after `auto_rereview_delay_minutes` — but only if the head is still the commit whose push dismissed it (so a burst of pushes costs one round, not one per push), the head has no consolidated review yet, the review label has not been applied since the approval the card reports (no round in flight), the PR is open and not a draft, it carries neither `skip-cursor-review` nor `needs-human-review`, and it is under `max_rounds`. Once per head commit, recorded on the status card, so a head that flips back is never re-run twice. With it off, the dismissal still marks the card stale (`no_decision`/`relabel`, or `capped`/`human` at the round cap) and nothing relabels. Needs `bot_app_id` or `APPROVER_TOKEN`. No effect without `approve_max_severity`, for an author `approve_authors` does not list, or under `run_without_label`, where the push already starts the round. See [cursor-approve's card contract](cursor-approve.md#a-push-that-dismisses-the-approval). | | `auto_rereview_delay_minutes` | `10` | The debounce before `auto_rereview_after_dismissal` relabels, in minutes. A newer push during the wait means no relabel; that push's own dismissal schedules its own wait. The wait happens on a runner, inside the push's run, so a later event in the same concurrency slot cancels it: a `pull_request`/`pull_request_target` event schedules a new one, but a `pull_request_review_thread` run (resolving a thread during the wait) does not, and the card's fallback (re-run by hand) applies. Clamped to `30`; anything unparsable waits not at all. | @@ -358,7 +358,10 @@ What happens, in order: land as threads, the reviewed diff was empty) or findings above the threshold are never retried: the next round would hit the same wall. That includes findings above the threshold from the cells that DID complete, and an open - thread above it from an earlier round. Mixed causes follow the strictest one. + thread above it from an earlier round. A round where zero reviewer cells + completed is also never retried automatically: simultaneous total failure is + a systemic service, credential or account signal, not one flaky cell. Mixed + causes follow the strictest one. - **A token that can actually relabel.** With the `GITHUB_TOKEN` fallback the retry is skipped, because a label applied by `GITHUB_TOKEN` fires no workflow run — it would stop the round rather than restart it. Set `bot_app_id` /