diff --git a/.changes/source-control/fix-rerun-binds-kill-switch-skip-7d41b9e2.md b/.changes/source-control/fix-rerun-binds-kill-switch-skip-7d41b9e2.md new file mode 100644 index 0000000000..d321792bb9 --- /dev/null +++ b/.changes/source-control/fix-rerun-binds-kill-switch-skip-7d41b9e2.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `/source-control:babysit-prs` binds a review lane's second attempt after a kill switch stopped the first: the caller's `kill-switch` job succeeded and wrote its lane's `a kill switch stops this review (` or `security review (` notice, and the first attempt has no lane job, so the review never started. ci-status tells a person to re-run such a run once the switch is off, and that re-run was refused as `rerun-not-allowed`. A kill-switched run on an earlier head also no longer unbinds the lane as `no-lane-job`. The lane-run collector reads the kill-switch job's annotations only for an attempt with no lane job. The merge gate does not start this re-run, because it cannot read the switch. diff --git a/plugins/source-control/skills/babysit-prs/reference/safety.md b/plugins/source-control/skills/babysit-prs/reference/safety.md index c02da8e00b..8e16cc1646 100644 --- a/plugins/source-control/skills/babysit-prs/reference/safety.md +++ b/plugins/source-control/skills/babysit-prs/reference/safety.md @@ -860,7 +860,11 @@ review; the PR still merges only once the check reports SUCCESS. The review evid 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`. +already reports as `rate-limit`. It also binds a second attempt after a kill switch stopped the +first: the caller's `kill-switch` job succeeded with its lane's `a kill switch stops this review (` +(or `security review (`) notice, and the first attempt has no lane job, so the review never +started. The gate never starts that rerun either, since it cannot read the switch: ci-status asks a +person to rerun the run once the switch is off. - **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_gh.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py index 96fc99f012..35149a64d1 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_gh.py @@ -1613,7 +1613,8 @@ def collect_lane_runs( from a fresh `GET actions/runs/{id}` taken after the listing, and that response stands in for its latest attempt, so only earlier attempts are read from `attempts/{n}`; each attempt - carries its jobs, and each lane job its check run's annotations. Any read + carries its jobs, and each lane job its check run's annotations, as does + the succeeded `kill-switch` job of an attempt with no lane job. Any read failure, a deleted or expired run included, raises `RuntimeError`, which the caller turns into unbound evidence; so does a listing that reached RUN_SEARCH_CAP, which may be missing runs. `env_overrides` applies to @@ -1675,8 +1676,19 @@ def pages(endpoint: str, label: str, items_key: str | None = None) -> list[Any]: # differ only in `jobs_url` and `logs_url`, which nothing reads. attempt = latest if n == count else json_object(read(base)) jobs = pages(f"{base}/jobs?per_page=100", f"run {run_id} jobs", "jobs") + lane_jobs = [ + job + for job in jobs + if str(job.get("name") or "").rsplit(" / ", 1)[-1] == job_suffix + ] for job in jobs: - if str(job.get("name") or "").rsplit(" / ", 1)[-1] == job_suffix: + # With no lane job, the kill-switch notice shows the review + # never started (`babysit_review_evidence.rerun_binds`). + if job in lane_jobs or ( + not lane_jobs + and job.get("name") == "kill-switch" + and job.get("conclusion") == "success" + ): job["annotations"] = pages( f"repos/{repo}/check-runs/{job.get('id')}/annotations" "?per_page=100", diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py index 2dad81ff05..d688b4db94 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -301,7 +301,9 @@ def is_ai_review_check(check_name: str, names: tuple[str, ...]) -> bool: # API rate limit carries the lane's publish failure line. Under `--merge` the # gate re-runs that check's workflow run once the limit has reset, instead of # holding on it until a person re-runs it, on exactly the annotations -# `review_evidence.rate_limited` accepts, so the re-run's attempt binds. +# `review_evidence.rate_limited` accepts, so the re-run's attempt binds +# (`review_evidence.rerun_binds` accepts more: a rejected credential or a +# kill-switch stop, which the gate leaves to a person). # "Reset" is conservative: the shortest plan limit is a five-hour session # window (https://support.claude.com/en/articles/11145838, checked # 2026-10-09), so five hours after the failure that window has reset whenever 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 358146eec6..7550c696b5 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 @@ -115,6 +115,20 @@ r"(?P[\w-]+): failure-class=publish-failed: [^(\n]*" r"\((?:API rate limit exceeded|You have exceeded a secondary rate limit)\b" ) +# Or when a kill switch stopped attempt 1 before the review started: the +# caller's `kill-switch` job succeeded and its `Report the kill switch` step +# wrote this lane's notice, whose `if` is the negation of the `proceed` output +# the lane job needs, and attempt 1 has no lane job. Attempt 1 then left no +# verdict. The step's text comes from the switch values only, and a head that +# edits a caller to forge it unbinds through `lane_source`. Like `auth`, it +# binds a re-run but does not start one: the gate cannot read the switch. +KILL_SWITCH_JOB = "kill-switch" +KILL_SWITCH_NOTICE_RE = { + "claude-review": re.compile(r"a kill switch stops this review \("), + "claude-security-review": re.compile( + r"a kill switch stops this security review \(" + ), +} MARKER_AUTHOR = "github-actions[bot]" MARKER_RE = re.compile( rf"^$" @@ -383,9 +397,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 rerun_binds( - lane, _lane_job(lane, newest["id"], first) - ): + if not rerun_binds(lane, newest["id"], first): raise Unbound("rerun-not-allowed") job = _lane_job(lane, newest["id"], latest) if job.get("conclusion") != "success" or job.get("head_sha") != sha: @@ -461,14 +473,48 @@ def rate_limited(lane: str, job: Mapping[str, Any]) -> bool: ) -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).""" +def rerun_binds(lane: str, run_id: int, first: Mapping[str, Any]) -> bool: + """Whether attempt 1 (`first`) left no verdict, so attempt 2 binds: it + failed on a rate limit or a rejected credential (AUTH_FAILURE_RE), or a + kill switch stopped it (`_kill_switch_stopped`).""" + if first.get("conclusion") == "success": + return _kill_switch_stopped(lane, run_id, first) + if first.get("conclusion") != "failure": + return False + job = _lane_job(lane, run_id, first) return rate_limited(lane, job) or any( _lane_line(AUTH_FAILURE_RE, lane, m) for m in _failures(job) ) +def _kill_switch_stopped(lane: str, run_id: int, attempt: Mapping[str, Any]) -> bool: + """Whether a kill switch stopped this attempt before its lane job existed.""" + jobs = attempt.get("jobs") + if not isinstance(jobs, list | tuple) or not all( + isinstance(job, Mapping) for job in jobs + ): + raise Unbound("evidence-missing") + suffix = LANES[lane][1] + if any(str(job.get("name") or "").rsplit(" / ", 1)[-1] == suffix for job in jobs): + return False + kills = [job for job in jobs if job.get("name") == KILL_SWITCH_JOB] + if len(kills) != 1 or kills[0].get("conclusion") != "success": + return False + kill = kills[0] + if kill.get("run_id") != run_id or kill.get("run_attempt") != attempt.get( + "run_attempt" + ): + raise Unbound("evidence-error") + if not isinstance(kill.get("annotations"), list | tuple): + raise Unbound("evidence-missing") + return any( + isinstance(a, Mapping) + and a.get("annotation_level") == "notice" + and KILL_SWITCH_NOTICE_RE[lane].match(str(a.get("message") or "")) + for a in kill["annotations"] + ) + + def _record(lane: str, sha: str, job: Mapping[str, Any]) -> _Record | None: """The job's one completion record for `sha`, None when it left none.""" found = [ @@ -642,11 +688,11 @@ def _open_findings( head_spent = False for item in lane_runs: for attempt in _attempts(item).values(): - # A draft-skipped run has only the caller job and reviewed nothing. + # A draft-skipped or kill-switched run reviewed nothing. if ( attempt.get("status") != "completed" or attempt.get("conclusion") == "skipped" - ): + ) or _kill_switch_stopped(lane, item["id"], attempt): continue job = _lane_job(lane, item["id"], attempt) rec = _record(lane, item["head_sha"], job) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh_lane_attempt_reuse.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh_lane_attempt_reuse.py index 5768667ec8..9db912f13b 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh_lane_attempt_reuse.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_gh_lane_attempt_reuse.py @@ -46,7 +46,13 @@ def test_the_recorded_pair_differs_only_in_unread_urls(self) -> None: } self.assertEqual(differing, {"jobs_url", "logs_url"}) - def collect(self, run: dict[str, Any], earlier: dict[int, Any]) -> Any: + def collect( + self, + run: dict[str, Any], + earlier: dict[int, Any], + jobs: dict[int, list[dict[str, Any]]] | None = None, + annotations: dict[int, list[dict[str, Any]]] | None = None, + ) -> Any: run_id = run["id"] base = f"repos/{self.REPO}/actions/runs/{run_id}" listing = ( @@ -57,9 +63,15 @@ def collect(self, run: dict[str, Any], earlier: dict[int, Any]) -> Any: number = run["pull_requests"][0]["number"] answers: dict[str, Any] = {listing: [{"workflow_runs": [run]}], base: run} for n in range(1, run["run_attempt"] + 1): - answers[f"{base}/attempts/{n}/jobs?per_page=100"] = [{"jobs": []}] + answers[f"{base}/attempts/{n}/jobs?per_page=100"] = [ + {"jobs": (jobs or {}).get(n, [])} + ] for n, attempt in earlier.items(): answers[f"{base}/attempts/{n}"] = attempt + for job_id, notes in (annotations or {}).items(): + answers[ + f"repos/{self.REPO}/check-runs/{job_id}/annotations?per_page=100" + ] = [notes] self.reads: list[str] = [] def fake(args: list[str]) -> Any: @@ -98,6 +110,35 @@ def test_earlier_attempts_are_still_read(self) -> None: self.assertIn(f"{base}/attempts/1", self.reads) self.assertNotIn(f"{base}/attempts/2", self.reads) + def test_an_earlier_attempts_kill_switch_notice_is_read(self) -> None: + # #10092's attempt 1: the kill-switch job wrote its notice and the + # reusable's caller job was skipped. The kill-switch job of an attempt + # that has a lane job, and one that did not succeed, cost no read. + rerun = {**self.run_record, "run_attempt": 2} + notice = { + "annotation_level": "notice", + "message": "a kill switch stops this review (lanes: kill-switch)", + } + jobs = { + 1: [ + {"id": 11, "name": "kill-switch", "conclusion": "success"}, + {"id": 12, "name": "pr-review", "conclusion": "skipped"}, + ], + 2: [ + {"id": 21, "name": "kill-switch", "conclusion": "success"}, + {"id": 22, "name": "pr-review / claude-review-status"}, + ], + } + runs = self.collect(rerun, {1: self.attempt}, jobs, {11: [notice], 22: []}) + first, second = (a["jobs"] for a in runs[0]["attempts"]) + self.assertEqual(first[0]["annotations"], [notice]) + self.assertNotIn("annotations", first[1]) + self.assertNotIn("annotations", second[0]) + self.assertEqual(second[1]["annotations"], []) + failed = {1: [{"id": 13, "name": "kill-switch", "conclusion": "failure"}]} + self.collect(rerun, {1: self.attempt}, failed) + self.assertFalse(any("check-runs" in path for path in self.reads)) + if __name__ == "__main__": unittest.main() 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 b6df035408..06cc173c50 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 @@ -724,6 +724,217 @@ def test_stale_listing_on_an_earlier_link_is_an_evidence_error(self) -> None: result = self.bind([base, head]) self.assertUnbound(result["claude-review"], "evidence-error") + def test_every_rerun_the_gate_starts_binds(self) -> None: + # The merge gate re-runs a review only on what `rate_limited` accepts + # (babysit_merge.rerun_rate_limited_review); each such re-run must + # bind, or the gate spends the one allowed attempt for nothing. + for note in (RATE_LIMIT, PUBLISH_RATE_LIMIT): + with self.subTest(message=note["message"][:40]): + first = attempt("claude-review", S, 1, [note], "failure") + self.assertTrue(ev.rate_limited("claude-review", first["jobs"][0])) + result = self.bind([self.lane_run(first, self.ok(2))]) + self.assertTrue(result["claude-review"].bound) + + +# The caller workflows' `Report the kill switch` notice +# (pr-review-hosted.yml, pr-review-security-hosted.yml). +KILL_NOTICE = { + "claude-review": "a kill switch stops this review" + " (lanes: kill-switch, CLAUDE_REVIEW_DISABLED: false)", + "claude-security-review": "a kill switch stops this security review" + " (lanes: kill-switch, CLAUDE_SECURITY_REVIEW_DISABLED: false)", +} + + +def kill_switched( + lane: str, + sha: str = S, + *, + message: str | None = None, + level: str = "notice", + kill_conclusion: str = "success", + kill_name: str = "kill-switch", +) -> dict[str, Any]: + """Attempt 1 as #10092's review runs recorded it: the kill-switch job + succeeded and wrote its notice, and the reusable's caller job was skipped, + so no lane job exists.""" + caller = JOBS[lane].split(" / ", 1)[0] + + def job(name: str, conclusion: str, annotations: Any = None) -> dict[str, Any]: + item = { + "id": next(_ids), + "run_attempt": 1, + "name": name, + "status": "completed", + "conclusion": conclusion, + "head_sha": sha, + } + if annotations is not None: + item["annotations"] = annotations + return item + + notice = { + "path": ".github", + "annotation_level": level, + "title": "", + "message": KILL_NOTICE[lane] if message is None else message, + } + return { + "run_attempt": 1, + "head_sha": sha, + "status": "completed", + "conclusion": "success", + "jobs": [ + job(kill_name, kill_conclusion, [notice]), + job("resolve-model", "success"), + job(caller, "skipped"), + ], + } + + +class KillSwitchRerunTests(Base): + """A re-run after a kill switch stopped attempt 1 (#10092, #10057): the + review never started, so attempt 2 is the first review of the head.""" + + def lane_run(self, lane: str, *attempts: dict[str, Any]) -> dict[str, Any]: + return run(lane, S, attempts=list(attempts)) + + def ok(self, lane: str, n: int = 2) -> dict[str, Any]: + return attempt(lane, S, n, [record(lane, S, "findings=0 inline=0 scope=whole")]) + + def test_second_attempt_after_a_kill_switch_stop_binds(self) -> None: + for lane in LANES: + with self.subTest(lane=lane): + result = self.bind( + [self.lane_run(lane, kill_switched(lane), self.ok(lane))] + ) + self.assertTrue(result[lane].bound) + self.assertEqual(result[lane].open_findings, 0) + + def test_the_other_lanes_notice_does_not_bind(self) -> None: + for lane, other in zip(LANES, reversed(LANES), strict=True): + with self.subTest(lane=lane): + first = kill_switched(lane, message=KILL_NOTICE[other]) + result = self.bind([self.lane_run(lane, first, self.ok(lane))]) + self.assertUnbound(result[lane], "rerun-not-allowed") + + def test_a_lookalike_notice_does_not_bind(self) -> None: + for message in ( + "the log says: a kill switch stops this review (lanes: x)", + "a kill switch stops this review", + "A kill switch stops this review (lanes: x)", + ): + with self.subTest(message=message): + first = kill_switched("claude-review", message=message) + result = self.bind( + [self.lane_run("claude-review", first, self.ok("claude-review"))] + ) + self.assertUnbound(result["claude-review"], "rerun-not-allowed") + + def test_a_notice_at_another_level_does_not_bind(self) -> None: + for level in ("warning", "failure"): + with self.subTest(level=level): + first = kill_switched("claude-review", level=level) + result = self.bind( + [self.lane_run("claude-review", first, self.ok("claude-review"))] + ) + self.assertUnbound(result["claude-review"], "rerun-not-allowed") + + def test_the_notice_on_another_job_does_not_bind(self) -> None: + first = kill_switched("claude-review", kill_name="resolve-model") + result = self.bind( + [self.lane_run("claude-review", first, self.ok("claude-review"))] + ) + self.assertUnbound(result["claude-review"], "rerun-not-allowed") + + def test_a_kill_switch_job_that_did_not_succeed_does_not_bind(self) -> None: + for conclusion in ("failure", "cancelled", "skipped"): + with self.subTest(conclusion=conclusion): + first = kill_switched("claude-review", kill_conclusion=conclusion) + result = self.bind( + [self.lane_run("claude-review", first, self.ok("claude-review"))] + ) + self.assertUnbound(result["claude-review"], "rerun-not-allowed") + + def test_two_kill_switch_jobs_do_not_bind(self) -> None: + first = kill_switched("claude-review") + first["jobs"].append(copy.deepcopy(first["jobs"][0])) + first["jobs"][-1]["id"] = next(_ids) + result = self.bind( + [self.lane_run("claude-review", first, self.ok("claude-review"))] + ) + self.assertUnbound(result["claude-review"], "rerun-not-allowed") + + def test_a_first_attempt_that_reviewed_never_rebinds(self) -> None: + # The notice beside a lane job that ran: attempt 1 may hold a verdict. + reviewed = self.ok("claude-review", 1) + first = kill_switched("claude-review") + first["jobs"].append(reviewed["jobs"][0]) + for conclusion in ("success", "failure"): + with self.subTest(conclusion=conclusion): + trial = copy.deepcopy(first) + trial["conclusion"] = conclusion + result = self.bind( + [self.lane_run("claude-review", trial, self.ok("claude-review"))] + ) + self.assertUnbound(result["claude-review"], "rerun-not-allowed") + + def test_a_failed_first_attempt_with_the_notice_does_not_bind(self) -> None: + first = kill_switched("claude-review") + first["conclusion"] = "failure" + result = self.bind( + [self.lane_run("claude-review", first, self.ok("claude-review"))] + ) + self.assertUnbound(result["claude-review"], "no-lane-job") + + def test_third_attempt_after_a_kill_switch_stop_is_never_allowed(self) -> None: + stopped_again = kill_switched("claude-review") + stopped_again["run_attempt"] = 2 + for item in stopped_again["jobs"]: + item["run_attempt"] = 2 + result = self.bind( + [ + self.lane_run( + "claude-review", + kill_switched("claude-review"), + stopped_again, + self.ok("claude-review", 3), + ) + ] + ) + self.assertUnbound(result["claude-review"], "rerun-not-allowed") + + def test_a_kill_switched_run_on_an_earlier_head_does_not_unbind(self) -> None: + stopped = run( + "claude-review", S0, attempts=[kill_switched("claude-review", S0)] + ) + result = self.bind([stopped, whole("claude-review", S)]) + self.assertTrue(result["claude-review"].bound) + + def test_a_kill_switched_head_alone_does_not_bind(self) -> None: + result = self.bind( + [self.lane_run("claude-review", kill_switched("claude-review"))] + ) + self.assertUnbound(result["claude-review"], "no-lane-job") + + def test_unread_kill_switch_annotations_are_missing_evidence(self) -> None: + first = kill_switched("claude-review") + del first["jobs"][0]["annotations"] + result = self.bind( + [self.lane_run("claude-review", first, self.ok("claude-review"))] + ) + self.assertUnbound(result["claude-review"], "evidence-missing") + + def test_a_changed_caller_workflow_still_binds_no_lane(self) -> None: + # The head's copy of a caller runs on pull_request, so a head that + # edits one could write the notice itself; any such edit unbinds. + runs = [ + self.lane_run(lane, kill_switched(lane), self.ok(lane)) for lane in LANES + ] + result = self.bind(runs, changed_paths=[FILES["claude-review"]]) + for lane in LANES: + self.assertUnbound(result[lane], "lane-source-changed") + class RecordTests(Base): def test_no_completion_record(self) -> None: