diff --git a/.changes/source-control/fix-rerun-binds-infra-failure-3c71e0a2.md b/.changes/source-control/fix-rerun-binds-infra-failure-3c71e0a2.md new file mode 100644 index 0000000000..a2f87aea4f --- /dev/null +++ b/.changes/source-control/fix-rerun-binds-infra-failure-3c71e0a2.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `/source-control:babysit-prs` binds a review lane's second attempt after the first failed on a rejected Claude credential (the lane's own `failure-class=auth` failure line), as it already did after a usage limit or a spent key, which the lane reports as `rate-limit`. Such a re-run was refused as `rerun-not-allowed`, so a pull request whose review was re-run after a credential outage stayed held. The merge gate still re-runs a review only after a rate limit, so a re-run before the secret is fixed cannot spend the one allowed attempt. diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index 8d7a844a3d..c02da8e00b 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -856,7 +856,11 @@ and the run is on its first attempt. Five hours is the plan's session-limit wind has reset whenever it started. A weekly limit outlasts it, and the rerun then fails again and stays failed, because an attempt above one is never rerun: at most one gate rerun per check per head. Any other failure class is never rerun, and a check-only run never reruns. The rerun only restarts the -review; the PR still merges only once the check reports SUCCESS. +review; the PR still merges only once the check reports SUCCESS. The review evidence also binds a +second attempt after the lane's own `: failure-class=auth:` failure line (a rejected +credential, which left no verdict), but the gate never starts that rerun: a person or session reruns +it once the secret is fixed, since a rerun before then would spend the one attempt. A spent key +already reports as `rate-limit`. - **Pointer**: the `class=` contract in ci-workflows [`.github/actions/report-lane-outcome/action.yml`](https://github.com/melodic-software/ci-workflows/blob/main/.github/actions/report-lane-outcome/action.yml), diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py index dc3319fe20..358146eec6 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_review_evidence.py @@ -98,6 +98,13 @@ # The reconciler re-runs any failed lane run once; attempt 2 binds only when # attempt 1 failed on a usage limit. RATE_LIMIT_CLASS_RE = re.compile(r"\bclass=rate-limit\b") +# Or on a rejected Claude credential: this lane's publish-review failure line +# (ci-workflows publish.cjs `fail`) with class `auth`. Like a usage limit, +# attempt 1 then left no verdict, so a re-run cannot trade one review for +# another. It binds a re-run but does not start one (rate_limited), since a +# re-run before the secret is fixed would spend the one allowed attempt. A +# spent key needs no entry: publish.cjs reports it as `rate-limit`. +AUTH_FAILURE_RE = re.compile(r"(?P[\w-]+): failure-class=auth: ") # Or when the review's own post hit GitHub's API rate limit: the message opens # with this lane's publish-review failure line (ci-workflows publish.cjs `fail`) # whose detail, the request error's message, opens with GitHub's primary or @@ -376,7 +383,7 @@ def _bound_record(lane: str, sha: str, lane_runs: list[Mapping[str, Any]]) -> _R raise Unbound("rerun-not-allowed") if first is None: raise Unbound("evidence-missing") - if first.get("conclusion") != "failure" or not rate_limited( + if first.get("conclusion") != "failure" or not rerun_binds( lane, _lane_job(lane, newest["id"], first) ): raise Unbound("rerun-not-allowed") @@ -432,18 +439,33 @@ def _lane_job(lane: str, run_id: int, attempt: Mapping[str, Any]) -> Mapping[str return job -def rate_limited(lane: str, job: Mapping[str, Any]) -> bool: - def limited(message: str) -> bool: - publish = PUBLISH_RATE_LIMIT_RE.match(message) - return bool(RATE_LIMIT_CLASS_RE.search(message)) or ( - publish is not None and publish["lane"] == lane - ) +def _failures(job: Mapping[str, Any]) -> list[str]: + """The messages of the job's failure-level annotations.""" + return [ + str(a.get("message") or "") + for a in job["annotations"] + if isinstance(a, Mapping) and a.get("annotation_level") == "failure" + ] + + +def _lane_line(pattern: re.Pattern[str], lane: str, message: str) -> bool: + """Whether `message` opens with `pattern` for this lane.""" + found = pattern.match(message) + return found is not None and found["lane"] == lane + +def rate_limited(lane: str, job: Mapping[str, Any]) -> bool: return any( - isinstance(a, Mapping) - and a.get("annotation_level") == "failure" - and limited(str(a.get("message") or "")) - for a in job["annotations"] + RATE_LIMIT_CLASS_RE.search(m) or _lane_line(PUBLISH_RATE_LIMIT_RE, lane, m) + for m in _failures(job) + ) + + +def rerun_binds(lane: str, job: Mapping[str, Any]) -> bool: + """Whether attempt 1's lane `job` lets attempt 2 bind: a rate limit, or a + rejected credential (AUTH_FAILURE_RE).""" + return rate_limited(lane, job) or any( + _lane_line(AUTH_FAILURE_RE, lane, m) for m in _failures(job) ) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py index 7e6d6bb14b..b6df035408 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_review_evidence.py @@ -67,6 +67,13 @@ def record(lane: str, sha: str, tail: str) -> dict[str, Any]: "title": "claude-review failed", "message": "failure-class=incomplete: no completed review was found on this head", } +# publish-review's failure line (ci-workflows publish.cjs `fail`) for a +# rejected Claude credential. +AUTH_FAILURE = dict( + OTHER_FAILURE, + message="claude-review: failure-class=auth:" + " the Claude credential was rejected; the owner replaces the secret", +) # publish-review's failure line (ci-workflows publish.cjs `fail`) as a run # recorded it when the review's own post hit GitHub's API rate limit. @@ -564,6 +571,56 @@ def test_second_attempt_after_a_rate_limit_failure(self) -> None: result = self.bind([self.lane_run(first, self.ok(2))]) self.assertTrue(result["claude-review"].bound) + def test_second_attempt_after_a_credential_failure(self) -> None: + first = attempt("claude-review", S, 1, [AUTH_FAILURE], "failure") + result = self.bind([self.lane_run(first, self.ok(2))]) + self.assertTrue(result["claude-review"].bound) + + def test_a_warning_level_auth_does_not_bind(self) -> None: + # publish.cjs warns ` key: class=auth` when the key leg's + # credential is refused, whatever the run then failed on; only a + # failure-level line names why the run failed. + timeout = dict( + OTHER_FAILURE, + message="claude-review: failure-class=timeout:" + " the review leg reached its time limit", + ) + for message in ( + "claude-review key: class=auth: the API refused the key;" + " the owner must replace the secret.", + AUTH_FAILURE["message"], + ): + with self.subTest(message=message): + warning = dict( + AUTH_FAILURE, annotation_level="warning", message=message + ) + first = attempt("claude-review", S, 1, [warning, timeout], "failure") + result = self.bind([self.lane_run(first, self.ok(2))]) + self.assertUnbound(result["claude-review"], "rerun-not-allowed") + + def test_an_auth_lookalike_does_not_bind(self) -> None: + for message in ( + "claude-review: failure-class=auth-x: something else", + "claude-review: failure-class=other: an unclassified failure" + " (detail quoting failure-class=auth: rejected)", + "security-review: failure-class=auth: another lane's credential", + ): + with self.subTest(message=message): + first = attempt( + "claude-review", + S, + 1, + [dict(AUTH_FAILURE, message=message)], + "failure", + ) + result = self.bind([self.lane_run(first, self.ok(2))]) + self.assertUnbound(result["claude-review"], "rerun-not-allowed") + + def test_a_credential_failure_is_not_a_rate_limit(self) -> None: + self.assertFalse( + ev.rate_limited("claude-review", {"annotations": [AUTH_FAILURE]}) + ) + def test_second_attempt_after_a_publish_rate_limit_failure(self) -> None: first = attempt("claude-review", S, 1, [PUBLISH_RATE_LIMIT], "failure") result = self.bind([self.lane_run(first, self.ok(2))])