diff --git a/.changes/pr-pipeline/fix-lane-room-any-refusal-reads-afresh-40255059.md b/.changes/pr-pipeline/fix-lane-room-any-refusal-reads-afresh-40255059.md new file mode 100644 index 0000000000..1f1cd89313 --- /dev/null +++ b/.changes/pr-pipeline/fix-lane-room-any-refusal-reads-afresh-40255059.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `enable-auto-merge` no longer leaves every lanes App pull request unarmed, and disarms the armed ones, when the run's post-merge report is one the gate would not reuse (for example `demotion-changed`, which any issue the lanes App files and triage then labels sets). The lane's room now checks the report again as the gate does and arms from that answer. diff --git a/.changes/source-control/fix-lane-room-any-refusal-reads-afresh-40255059.md b/.changes/source-control/fix-lane-room-any-refusal-reads-afresh-40255059.md new file mode 100644 index 0000000000..a3f8cdb05c --- /dev/null +++ b/.changes/source-control/fix-lane-room-any-refusal-reads-afresh-40255059.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `babysit_verify_evidence.py lane-room` no longer reports the lane's room unreadable when the gate would not reuse the run's post-merge answer for a reason other than `unverified-stale`: too old, from the future, a demotion record filed or changed since (`demotion-changed`), or a lane merge it does not count (`new-merge:#`). As the gate does, it reads the answer again (`_recompute_shared`, demotion included), so a demotion filed since still holds the rung off and a failure found since still refuses. Only an `unverified-stale` answer may still be kept unchanged when nothing it was read from has moved. With no check to run, or a check that fails or cannot be read, the room stays unreadable. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 632e03d104..e6d2dd9772 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -57,10 +57,10 @@ // lane's unverified merges and the armed pull requests not yet queued, it // arms; at the cap it arms nothing more. A room that cannot be read (no // post-merge report, a failed read) holds as a refusal does: nothing is -// armed and every armed lanes App pull request is disarmed. A report past -// its reuse age only for a lane merge still unverified is kept while -// nothing it was read from has moved, else checked again as the gate -// does. A held room raises one warning annotation and step-summary line +// armed and every armed lanes App pull request is disarmed. A report the +// gate would not reuse is checked again as the gate does; one past its +// reuse age only for a lane merge still unverified is first kept while +// nothing it was read from has moved. A held room raises one warning annotation and step-summary line // naming why and each lane pull request left unarmed or, report-only, // that a disarm would have turned off, and one error and summary line // naming each one still armed because its disarm failed. diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 361668c9b9..db5816514f 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1134,9 +1134,10 @@ gate's `` comment and no label, since a pu count; an unread count leaves it unarmed with no comment, and the next run reads it again. An armed one is not disarmed on the count. Each such run also waits for the same run's `post-merge-dispatch` job and reads the lane's room from the post-merge -report that job keeps as an artifact (`babysit_verify_evidence.py lane-room`; with no report, or -one the gate would not reuse (`shared_refusal`: too old, a demotion record changed since, or a lane -merge landed since that it does not count), it holds as a refusal does; one past the gate's +report that job keeps as an artifact (`babysit_verify_evidence.py lane-room`; with no report it +holds as a refusal does; one the gate would not reuse (`shared_refusal`: too old, a demotion record +changed since, or a lane merge landed since that it does not count) is checked again as the gate +does; one past the gate's two-minute reuse age only because a lane merge is still unverified is kept while main's head, its config commit, the demotion records and the unverified merges' full and canary runs are as they were (no run reached a conclusion since, no merge is past the six-hour dispatch deadline, and no diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py index 2e6731ce8c..ef3a013567 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge_policy.py @@ -5094,11 +5094,12 @@ def lane_room( `assess` applies to every lane candidate, read once at the default branch head, in its order. `shared` is this run's `babysit_postmerge.py` answer (`babysit_postmerge.read_shared`), as the gate's `--postmerge-result`. - `post_merge` is `assess`'s: an answer refused only as `unverified-stale` - is kept when nothing it was read from has moved (`_still_current`, with - `runs_unchanged(shared, branch)` for its unverified merges' runs), else - read again as the gate reads it (`_recompute_shared`), and a readable - one stands in for `shared`. + `post_merge` is `assess`'s: an answer the gate would not reuse + (`shared_refusal`) is read again as the gate reads it + (`_recompute_shared`, with the room's self logins, none), and a readable + one stands in for `shared`; one refused only as `unverified-stale` is + first kept when nothing it was read from has moved (`_still_current`, + with `runs_unchanged(shared, branch)` for its unverified merges' runs). - A config naming another authority stops (`enabled_authority`). - A post-merge refusal stops (`post-merge:`). @@ -5110,12 +5111,9 @@ def lane_room( Any read failure, a missing `shared` and an in-flight count that could not be read included, is `unreadable`. So is a `shared` the gate would - not reuse (`shared_refusal`): too old (a re-run job downloads the run's - first report, up to a day old), a demotion record filed or changed - since, or a lane merge landed since that it does not count. An - `unverified-stale` one is kept or read again instead, since a lane - merge's verifying run may have passed or failed since; a read that fails - or is not readable leaves it `unreadable`.""" + not reuse (too old, a demotion record filed or changed since, a lane + merge landed since that it does not count) when there is no `post_merge` + to read it again, or that read fails or is not readable.""" if shared is None: return LaneRoom(unreadable="post-merge answer missing or unreadable") try: @@ -5143,18 +5141,21 @@ def lane_room( authority=PR_MERGE, now=now, ) - if unusable == "unverified-stale" and post_merge is not None: - # Kept when nothing it was read from moved, else read again. + if unusable and post_merge is not None: + # As the gate, any refusal reads afresh; an unverified-stale answer + # is first kept when nothing it was read from moved. try: - current = runs_unchanged is not None and _still_current( - repo, default, identity, shared, runs_unchanged + current = ( + unusable == "unverified-stale" + and runs_unchanged is not None + and _still_current(repo, default, identity, shared, runs_unchanged) ) except (RuntimeError, ValueError, OSError): current = False if not current: try: fresh = _recompute_shared( - repo, default, identity, shared.self_logins, post_merge, now + repo, default, identity, frozenset(), post_merge, now ) except (RuntimeError, ValueError, OSError) as exc: return LaneRoom(unreadable=f"post-merge answer {unusable}: {exc}") diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py index 7bd767781f..6a9e0ad78c 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_verify_evidence.py @@ -1992,8 +1992,9 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: (`merge_policy.lane_room`, `LaneRoom.report`), from this run's `babysit_postmerge.py` report and the `CLAUDE_LANES_MERGE_RUNG_CAP` value the workflow read (empty is no cap; missing holds the rung off). A report - refused only as `unverified-stale` is kept while nothing it was read from - has moved, else checked again.""" + the gate would not reuse is checked again; one refused only as + `unverified-stale` is first kept while nothing it was read from has + moved.""" parser = argparse.ArgumentParser( prog="babysit_verify_evidence.py", allow_abbrev=False ) diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py index 1e33fff1fc..a638f95577 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_lane_room.py @@ -239,36 +239,90 @@ def test_at_the_cap_it_reports_the_cap_and_refuses_nothing(self) -> None: (room.refusal, room.unreadable, room.count, room.cap), (None, None, 5, 5) ) - def test_an_answer_the_gate_would_not_reuse_is_unreadable(self) -> None: - # The gate reads afresh past SHARED_MAX_AGE or after a demotion record - # changed (shared_refusal); a re-run job reads the run's first report. - cases = { - "stale": dict(now=FLOOR + policy.SHARED_MAX_AGE + timedelta(seconds=1)), - "future": dict(now=FLOOR - timedelta(seconds=1)), - # Inside the two minutes: past them the answer is read again, - # demotion included. - "demotion changed": dict( - demotion_changed=True, now=FLOOR + timedelta(minutes=1) - ), - } - recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) - for name, kwargs in cases.items(): + # The gate's reuse refusals other than unverified-stale (shared_refusal): + # past SHARED_MAX_AGE (a re-run job reads the run's first report), from + # the future, and a demotion record filed or changed since, inside the + # two minutes the gate reuses a pending answer for. + UNUSABLE = { + "stale": dict(now=FLOOR + policy.SHARED_MAX_AGE + timedelta(seconds=1)), + "future": dict(now=FLOOR - timedelta(seconds=1)), + "demotion changed": dict( + demotion_changed=True, now=FLOOR + timedelta(minutes=1) + ), + } + + def test_an_answer_the_gate_would_not_reuse_is_read_again(self) -> None: + # The gate reads afresh on any refusal (assess); so does the room, + # and a readable answer counts from its own floor. Run 40255059710 + # left 12 lane pull requests unarmed on `demotion-changed`. + for name, kwargs in self.UNUSABLE.items(): with self.subTest(name): + fresh = policy.PostMergeGate(None, {"readable": True}, lambda m: True) + recheck = Recheck(fresh) room = self.room( shared(report=PENDING), post_merge=recheck.post_merge, **kwargs, # type: ignore[arg-type] ) + self.assertEqual( + (room.refusal, room.unreadable, room.count, room.cap), + (None, None, 2, 5), + ) + self.assertEqual(recheck.asked, [(NEW_FLOOR, IDENTITY)]) + self.assertIs(self.seen["verified"], fresh.verified) + self.assertEqual(self.seen["merged_since"], NEW_FLOOR) + + def test_an_unusable_answer_is_never_kept_on_the_cheap_path(self) -> None: + # Only an unverified-stale answer may be kept unchanged; a stale one + # is read again even when nothing it was read from moved. + recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) + runs = mock.Mock(return_value=True) + self.room( + shared(report=PENDING), + post_merge=recheck.post_merge, + runs_unchanged=runs, + head=MAIN, + **self.UNUSABLE["stale"], # type: ignore[arg-type] + ) + self.assertEqual(len(recheck.asked), 1) + runs.assert_not_called() + + def test_a_demotion_read_again_holds_the_room_off(self) -> None: + recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) + room = self.room( + post_merge=recheck.post_merge, + fresh_demotion=policy.Demotion(True, "record-open", (8,)), + **self.UNUSABLE["demotion changed"], # type: ignore[arg-type] + ) + self.assertEqual(room.refusal, "rung-off:demotion") + + def test_an_unusable_answer_with_no_fresh_read_is_unreadable(self) -> None: + cases = { + "no check": None, + "check raises": Recheck(error=RuntimeError("runs 502")).post_merge, + "check unreadable": Recheck( + policy.PostMergeGate("runs-unreadable", {"readable": False}, None) + ).post_merge, + } + for name, post_merge in cases.items(): + with self.subTest(name): + room = self.room( + post_merge=post_merge, + **self.UNUSABLE["demotion changed"], # type: ignore[arg-type] + ) self.assertIsNone(room.refusal) - self.assertTrue(room.unreadable) - self.assertFalse(self.seen, "no in-flight read after an unusable answer") - # Only an unverified-stale answer is read again. - self.assertEqual(recheck.asked, []) + self.assertTrue( + str(room.unreadable).startswith( + "post-merge answer demotion-changed" + ), + room.unreadable, + ) + self.assertFalse(self.seen, "no in-flight read") - def test_a_lane_merge_the_answer_does_not_count_is_unreadable(self) -> None: + def test_a_lane_merge_the_answer_does_not_count_is_read_again(self) -> None: # The gate's reuse rule (shared_refusal) reads afresh when a lane # merge landed after the report was computed (`new-merge:#`), so - # its failed full run is not missed; the room holds rather than arm. + # its failed full run is not missed; the room reads it again too. seen: list[dict[str, object]] = [] def refusal(repo: str, answer: object, **kwargs: object) -> str: @@ -278,12 +332,14 @@ def refusal(repo: str, answer: object, **kwargs: object) -> str: recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) with mock.patch.object(policy, "shared_refusal", side_effect=refusal): room = self.room(post_merge=recheck.post_merge) + self.assertEqual((room.refusal, room.unreadable), (None, None)) + self.assertEqual(recheck.asked, [(NEW_FLOOR, IDENTITY)]) + with mock.patch.object(policy, "shared_refusal", side_effect=refusal): + room = self.room() self.assertEqual( (room.refusal, room.unreadable), (None, "post-merge answer new-merge:#7"), ) - self.assertFalse(self.seen, "no in-flight read after an unusable answer") - self.assertEqual(recheck.asked, []) # Bound as the gate binds a candidate at the default branch head. self.assertEqual( {