From 6fe7ac3cf33a90b20c3355577b1250e72f8c496b Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:32:07 -0400 Subject: [PATCH 1/5] fix(pr-merge): recompute an unverified-stale post-merge answer in lane_room and annotate room holds lane_room read the run's post-merge answer as unusable once it was older than two minutes with a lane merge unverified, which the post-merge step alone nearly reaches, so every pending lane merge left the room unreadable and enable-auto-merge disarmed every armed lane pull request. On unverified-stale only, the room now reads the answer again as the gate does (head, config commit, demotion, floor, then the check), writes it back to the --postmerge-result file and uses it. A failed or unreadable check keeps the room unreadable. enable-auto-merge raises one warning annotation and one step-summary line when the room is held. Closes #9577 Co-Authored-By: Claude Opus 5.5 --- .../fix-lane-room-recompute-9577a1c3.md | 7 + .../fix-lane-room-recompute-9577a1c3.md | 7 + .../scripts/pr-merge/enable-auto-merge.mjs | 37 ++- .../pr-merge/enable-auto-merge.test.mjs | 34 ++- docs/conventions/pr-pipeline/README.md | 7 +- .../babysit-prs/scripts/babysit_merge.py | 13 +- .../scripts/babysit_merge_policy.py | 61 ++++- .../babysit-prs/scripts/babysit_postmerge.py | 20 ++ .../scripts/babysit_verify_evidence.py | 13 +- .../scripts/tests/test_babysit_lane_room.py | 240 +++++++++++++++++- 10 files changed, 409 insertions(+), 30 deletions(-) create mode 100644 .changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md create mode 100644 .changes/source-control/fix-lane-room-recompute-9577a1c3.md diff --git a/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md b/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md new file mode 100644 index 0000000000..c440411d28 --- /dev/null +++ b/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- A pending lane merge no longer makes `enable-auto-merge` disarm every armed lanes App pull request. The post-merge step takes about as long as the gate's two-minute reuse age, so the lane's room read the report as too old. It now checks again, as the gate does. When the room is held, by a refusal or because it cannot be read, the run raises one warning annotation and one step-summary line naming the reason and each lanes App pull request it left unarmed. It posts no comment on the pull requests. diff --git a/.changes/source-control/fix-lane-room-recompute-9577a1c3.md b/.changes/source-control/fix-lane-room-recompute-9577a1c3.md new file mode 100644 index 0000000000..8650edfce3 --- /dev/null +++ b/.changes/source-control/fix-lane-room-recompute-9577a1c3.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- `babysit_verify_evidence.py lane-room` no longer reports the lane's room unreadable when the run's post-merge answer is past the gate's two-minute reuse age only because a lane merge is still unverified (`unverified-stale`). It checks again the way the gate does (`babysit_merge_policy.lane_room` with the gate's `post_merge` and `share`), writes the fresh answer back to the `--postmerge-result` file, and uses it, so a failure found since still refuses. A check that fails or cannot be read leaves the room unreadable, and every other refusal reads as before. `babysit_postmerge.gate_reader` is the one post-merge reader the gate and the room share. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index d1244997ed..dc2777ffc5 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -53,7 +53,11 @@ // 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. +// armed and every armed lanes App pull request is disarmed. A report past +// its reuse age only for a lane merge still unverified is checked again +// first, as the gate does. A held room raises one warning annotation and +// one step-summary line naming why and each lane pull request it left +// unarmed. // - pr-merge's gate would not hold it on its risk score: `evidence // --lane-reviews --rung ` runs the gate's scorer // (babysit_risk.score, holding) over the inputs the gate builds, the @@ -108,7 +112,7 @@ // run cache, as the gate's // GITHUB_API_URL import { spawnSync } from "node:child_process"; -import { readFileSync } from "node:fs"; +import { appendFileSync, readFileSync } from "node:fs"; import process from "node:process"; import { pathToFileURL } from "node:url"; @@ -619,22 +623,23 @@ export async function main({ env = process.env, github, verify, room, sleep = (m let inFlight = lane === null ? 0 : lane.count + pulls.filter((pull) => lanes(pull) && pull.autoMergeRequest != null && !lane.queued.includes(pull.number)).length; // Why the room keeps `pull` from arming, with `disarm` when it also turns // an armed one off; null when it has room. + // An unknown room holds as a refusal does: unknown holds. + const laneHeld = + lane?.refusal != null ? `pr-merge's gate holds its lane off (${lane.refusal})` : lane?.unreadable != null ? `the lane's room could not be read (${lane.unreadable})` : null; const roomHold = (pull) => { if (lane === null || !lanes(pull)) { return null; } - if (lane.refusal !== null) { - return { disarm: true, reason: `pr-merge's gate holds its lane off (${lane.refusal})` }; - } - // An unknown room holds as a refusal does: unknown holds. - if (lane.unreadable !== null) { - return { disarm: true, reason: `the lane's room could not be read (${lane.unreadable})` }; + if (laneHeld !== null) { + return { disarm: true, reason: laneHeld }; } if (pull.autoMergeRequest != null) { return null; } return inFlight >= lane.cap ? { disarm: false, reason: `merge.max-in-flight is ${lane.cap} and ${inFlight} merges are in flight` } : null; }; + // The lane pull requests the room's refusal or unread room left unarmed. + const roomHeld = []; let failed = 0; const minutes = riskEscalationMinutes(env.RESOLVED_CONFIG ?? ""); // A reader run's per pull request refusal (a hold path or cost hold, or @@ -684,6 +689,9 @@ export async function main({ env = process.env, github, verify, room, sleep = (m let detail = null; // Checked before the reads, so a held lane spends none. const held = ["read", "verify", "arm"].includes(action) ? roomHold(pull) : null; + if (held?.disarm) { + roomHeld.push(pull.number); + } if (held !== null) { ({ action, reason } = { action: held.disarm && pull.autoMergeRequest != null ? "disarm" : "none", reason: held.reason }); } @@ -769,6 +777,19 @@ export async function main({ env = process.env, github, verify, room, sleep = (m await note(pull, detail, { clears, reason }); } } + // One annotation and one summary line per run, not a comment per pull + // request: the room is read again every scheduled run. + if (roomHeld.length > 0) { + const text = escapeCommand(`${laneHeld}; left unarmed: ${roomHeld.map((number) => `#${number}`).join(", ")}`); + console.log(`::warning title=enable-auto-merge::${text}`); + if (env.GITHUB_STEP_SUMMARY) { + try { + appendFileSync(env.GITHUB_STEP_SUMMARY, `enable-auto-merge: ${text}\n`); + } catch (error) { + console.log(`enable-auto-merge: the step summary was not written: ${describe(error)}`); + } + } + } return failed === 0 ? 0 : 1; } catch (error) { console.log(`::error title=enable-auto-merge::${describe(error)}`); diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index 27b4a21463..cc2fc1cd41 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -8,7 +8,7 @@ // the parity tests read their verdicts from Python. import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; -import { mkdtempSync, writeFileSync } from "node:fs"; +import { mkdtempSync, readFileSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import process from "node:process"; @@ -1114,6 +1114,38 @@ test("a room that cannot be read arms nothing and disarms every armed lane pull assert.deepEqual(writes(), [{ mutation: "disarm", id: "PR_2" }]); }); +// Issue #9577's scope addition: a held room logged only to the console, so +// nothing on the run said why nothing armed. One annotation and one summary +// line per run, naming the reason and every lane pull request left unarmed. +test("a held room raises one warning and one step-summary line naming why and each pull request left unarmed", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const pulls = [pull(1), pull(2, { autoMergeRequest: armed }), pull(3, { author: { __typename: "User", login: "kyle-sexton" } }), pull(4, { isDraft: true })]; + const cases = [ + [{ ...ROOM, unreadable: "post-merge answer unverified-stale" }, "the lane's room could not be read (post-merge answer unverified-stale); left unarmed: #1, #2"], + [{ ...ROOM, refusal: "rung-off:demotion" }, "pr-merge's gate holds its lane off (rung-off:demotion); left unarmed: #1, #2"], + ]; + for (const [lane, expected] of cases) { + const summary = join(mkdtempSync(join(tmpdir(), "enable-auto-merge-summary-")), "summary.md"); + writeFileSync(summary, ""); + const { lines } = await run(pulls, { GITHUB_STEP_SUMMARY: summary }, {}, lane); + assert.deepEqual( + lines.filter((line) => line.startsWith("::warning")), + [`::warning title=enable-auto-merge::${expected}`], + ); + assert.equal(readFileSync(summary, "utf8"), `enable-auto-merge: ${expected}\n`); + } + // A lane with room raises neither. + const summary = join(mkdtempSync(join(tmpdir(), "enable-auto-merge-summary-")), "summary.md"); + writeFileSync(summary, ""); + const { lines, writes } = await run(pulls, { GITHUB_STEP_SUMMARY: summary }); + assert.deepEqual(writes, [{ mutation: "arm", id: "PR_1", head: HEAD }]); + assert.deepEqual( + lines.filter((line) => line.startsWith("::warning")), + [], + ); + assert.equal(readFileSync(summary, "utf8"), ""); +}); + // An event run reads no room, so it never disarms on one. test("an event run leaves an armed lane pull request armed whatever the room", async () => { const armed = { enabledAt: "2026-10-23T10:00:00Z" }; diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 4bb336e831..158cde3ee9 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1113,12 +1113,15 @@ review, the trust gate and the cost review all hold. 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): while the gate holds its lane off (a demotion record, +merge landed since that it does not count), it holds as a refusal does; one past the gate's +two-minute reuse age only because a lane merge is still unverified is checked again and written +back, as the gate does, and holds only when that check cannot be read): while the gate holds its lane off (a demotion record, `merge.rung`, the loop setting or the rung cap off, a lane merge that failed its post-merge check) nothing is armed and every armed lanes App pull request is disarmed; at `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests the queue has not taken, nothing more is armed; a room that cannot be read (no report, a failed read) holds -as a refusal does. +as a refusal does. A held room raises one warning annotation and one step-summary line naming why +and each lanes App pull request it left unarmed, and no comment on the pull requests. Only the scheduled run and a hand-started one read them and arm, last, for a pull request every other rule would arm or keep armed, with the gate's saved listing, head-ref map and run cache; an event run reads no link or verdict, never arms, and 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 d298786dbf..c324d64c1c 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_merge.py @@ -3284,16 +3284,9 @@ def assess_unattended( the review evidence and the score. Raises `PolicyReadError` when the pull request or its pipeline config cannot be read.""" - def post_merge(floor: datetime, identity: str) -> merge_policy.PostMergeGate: - verdict = postmerge.check( - repo, - agent_identity=identity, - is_human=postmerge._default_is_human(repo, identity, self_logins), - merged_since=floor, - pinned_sha=postmerge.pin_reader(repo, identity), - lane_merges_only=authority == merge_policy.PR_MERGE, - ) - return postmerge.gate_from_report(verdict.report()) + post_merge = postmerge.gate_reader( + repo, self_logins, lane_merges_only=authority == merge_policy.PR_MERGE + ) def promotion(window_start: datetime | None) -> merge_policy.Promotion: return merge_policy.read_promotion( 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 4205feed3b..1545586296 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 @@ -4490,18 +4490,62 @@ def report(self) -> dict[str, Any]: } +def _recompute_shared( + repo: str, + default: str, + identity: str, + self_logins: frozenset[str], + post_merge: Callable[[datetime, str], PostMergeGate], + now: datetime | None, +) -> SharedPostMerge | None: + """The run's shared answer read afresh at the default branch head, as + `assess` reads it for a pr-merge candidate: the head, its config commit, + the demotion, the floor, then the post-merge check, bound at a + `computed_at` taken before them. None when the check is not readable. + Raises `PolicyReadError`.""" + computed_at = now or datetime.now(UTC) + _, head = _default_branch_head(repo, default) + config_sha = newest_config_commit(repo, head) + demotion = read_demotion( + repo, + agent_identity=identity, + self_logins=self_logins, + permission=permission_reader(repo), + ) + floor = merged_since_floor(repo, head, demotion, PR_MERGE) + gate = post_merge(floor, identity) + if gate.report.get("readable") is not True: + return None + return SharedPostMerge( + main_sha=head, + config_sha=config_sha, + identity=identity, + self_logins=self_logins, + lane_merges_only=True, + computed_at=computed_at, + demotion=demotion, + floor=floor, + gate=gate, + ) + + def lane_room( repo: str, shared: SharedPostMerge | None, *, lane_rung_cap: str | None, now: datetime | None = None, + post_merge: Callable[[datetime, str], PostMergeGate] | None = None, + share: Callable[[SharedPostMerge], object] | None = None, ) -> LaneRoom: """Whether the pr-merge lane may merge one more of its own pull requests now, for native auto-merge, which skips `assess`: the run-level refusals `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` and `share` are `assess`'s: an answer refused only as + `unverified-stale` is read again as the gate reads it (`_recompute_shared`), + and a readable one goes to `share` and stands in for `shared`. - A config naming another authority stops (`enabled_authority`). - A post-merge refusal stops (`post-merge:`). @@ -4517,7 +4561,10 @@ def lane_room( 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.""" + since, or a lane merge landed since that it does not count. An + `unverified-stale` one is 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`.""" if shared is None: return LaneRoom(unreadable="post-merge answer missing or unreadable") try: @@ -4545,6 +4592,18 @@ def lane_room( authority=PR_MERGE, now=now, ) + if unusable == "unverified-stale" and post_merge is not None: + try: + fresh = _recompute_shared( + repo, default, identity, shared.self_logins, post_merge, now + ) + except (RuntimeError, ValueError, OSError) as exc: + return LaneRoom(unreadable=f"post-merge answer {unusable}: {exc}") + if fresh is None: + return LaneRoom(unreadable=f"post-merge answer {unusable}: unreadable") + if share is not None: + share(fresh) + shared, unusable = fresh, None if unusable: return LaneRoom(unreadable=f"post-merge answer {unusable}") if shared.gate.refusal: diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py index 293f8dc5a4..7e6028f91f 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py @@ -3097,6 +3097,26 @@ def gate_from_report(report: dict[str, Any]) -> policy.PostMergeGate: ) +def gate_reader( + repo: str, self_logins: Collection[str], *, lane_merges_only: bool +) -> Callable[[datetime, str], policy.PostMergeGate]: + """The merge gate's `post_merge(floor, identity)`: this check from + `floor`, as `gate_from_report` reads it.""" + + def post_merge(floor: datetime, identity: str) -> policy.PostMergeGate: + verdict = check( + repo, + agent_identity=identity, + is_human=_default_is_human(repo, identity, self_logins), + merged_since=floor, + pinned_sha=pin_reader(repo, identity), + lane_merges_only=lane_merges_only, + ) + return gate_from_report(verdict.report()) + + return post_merge + + def read_shared(path: str) -> policy.SharedPostMerge | None: """The run's shared answer from this script's own report at `path`, or None when it is missing, malformed, unreadable as a verdict, or has no 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 0c4725cc5e..7954116c8a 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 @@ -108,6 +108,7 @@ import argparse import contextlib +import functools import hashlib import io import json @@ -1548,7 +1549,8 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: pr-merge lane may merge one more of its own pull requests now (`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).""" + the workflow read (empty is no cap; missing holds the rung off). A report + refused only as `unverified-stale` is checked again and written back.""" parser = argparse.ArgumentParser( prog="babysit_verify_evidence.py", allow_abbrev=False ) @@ -1570,7 +1572,14 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: args = parser.parse_args(argv) if args.command == "lane-room": shared = postmerge.read_shared(args.postmerge_result) - answer = merge_policy.lane_room(args.repo, shared, lane_rung_cap=args.rung_cap) + answer = merge_policy.lane_room( + args.repo, + shared, + lane_rung_cap=args.rung_cap, + # The gate's recompute and write-back, bound as the room binds. + post_merge=postmerge.gate_reader(args.repo, (), lane_merges_only=True), + share=functools.partial(postmerge.write_shared, args.postmerge_result), + ) print(json.dumps(answer.report())) return 0 if ( 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 581849973b..bccf9844ba 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 @@ -15,6 +15,7 @@ import json import pathlib import sys +import tempfile import unittest from datetime import UTC, datetime, timedelta from unittest import mock @@ -30,6 +31,17 @@ FLOOR = datetime(2026, 10, 1, tzinfo=UTC) # Five minutes after the answer was computed: inside the gate's reuse age. NOW = FLOOR + timedelta(minutes=5) +# The default branch head and floor a recompute reads. +NEW_MAIN = "e" * 40 +NEW_FLOOR = FLOOR + timedelta(minutes=1) +# A passing answer with lane merge #5 still unverified, read three minutes on: +# past the two minutes the gate reuses it (issue #9577's 2:01 read). +PENDING = { + "readable": True, + "ok": True, + "autoMerges": [{"number": 5, "verified": False}], +} +STALE_NOW = FLOOR + timedelta(minutes=3) def config(**overrides: object) -> policy.PipelineConfig: @@ -47,7 +59,11 @@ def config(**overrides: object) -> policy.PipelineConfig: def shared( - *, demoted: bool = False, refusal: str | None = None, identity: str = IDENTITY + *, + demoted: bool = False, + refusal: str | None = None, + identity: str = IDENTITY, + report: dict[str, object] | None = None, ) -> policy.SharedPostMerge: return policy.SharedPostMerge( main_sha=MAIN, @@ -60,10 +76,32 @@ def shared( demoted, "record-open" if demoted else None, (7,) if demoted else () ), floor=FLOOR, - gate=policy.PostMergeGate(refusal, {}, lambda merge: True), + gate=policy.PostMergeGate(refusal, report or {}, lambda merge: True), ) +class Recheck: + """A fake post-merge check: `gate` or `error`, with each floor and + identity it was asked for, and each answer shared.""" + + def __init__( + self, gate: policy.PostMergeGate | None = None, error: Exception | None = None + ) -> None: + self.gate, self.error = gate, error + self.asked: list[tuple[datetime, str]] = [] + self.shared: list[policy.SharedPostMerge] = [] + + def post_merge(self, floor: datetime, identity: str) -> policy.PostMergeGate: + self.asked.append((floor, identity)) + if self.error is not None: + raise self.error + assert self.gate is not None + return self.gate + + def share(self, answer: policy.SharedPostMerge) -> None: + self.shared.append(answer) + + class LaneRoom(unittest.TestCase): def room( self, @@ -75,6 +113,10 @@ def room( config_error: Exception | None = None, now: datetime = NOW, demotion_changed: bool = False, + post_merge: object = None, + share: object = None, + head_error: Exception | None = None, + fresh_demotion: policy.Demotion | None = None, ) -> policy.LaneRoom: seen: dict[str, object] = {} @@ -101,12 +143,31 @@ def in_flight(repo: str, **kwargs: object) -> policy.InFlight: # head, so only its age and demotion decide. mock.patch.object(policy, "newest_config_commit", return_value=MAIN), mock.patch.object(policy, "_get", return_value=(200, {"sha": MAIN})), + # The recompute's reads at the default branch head: a newer head, + # no demotion record, and a later floor than the old answer's. + mock.patch.object( + policy, + "_default_branch_head", + side_effect=head_error, + return_value=("main", NEW_MAIN), + ), + mock.patch.object(policy, "permission_reader"), + mock.patch.object( + policy, + "read_demotion", + return_value=fresh_demotion or policy.Demotion(False, None, ()), + ), + mock.patch.object(policy, "merged_since_floor", return_value=NEW_FLOOR), ): + kwargs = {"post_merge": post_merge} if post_merge is not None else {} + if share is not None: + kwargs["share"] = share result = policy.lane_room( REPO, answer if answer is not None else shared(), lane_rung_cap=cap, now=now, + **kwargs, # type: ignore[arg-type] ) self.seen = seen return result @@ -168,14 +229,26 @@ def test_an_answer_the_gate_would_not_reuse_is_unreadable(self) -> None: cases = { "stale": dict(now=FLOOR + policy.SHARED_MAX_AGE + timedelta(seconds=1)), "future": dict(now=FLOOR - timedelta(seconds=1)), - "demotion changed": dict(demotion_changed=True), + # 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(): with self.subTest(name): - room = self.room(**kwargs) # type: ignore[arg-type] + room = self.room( + shared(report=PENDING), + post_merge=recheck.post_merge, + share=recheck.share, + **kwargs, # 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, recheck.shared), ([], [])) def test_a_lane_merge_the_answer_does_not_count_is_unreadable(self) -> None: # The gate's reuse rule (shared_refusal) reads afresh when a lane @@ -187,13 +260,15 @@ def refusal(repo: str, answer: object, **kwargs: object) -> str: seen.append(kwargs) return "new-merge:#7" + recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) with mock.patch.object(policy, "shared_refusal", side_effect=refusal): - room = self.room() + room = self.room(post_merge=recheck.post_merge, share=recheck.share) 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, recheck.shared), ([], [])) # Bound as the gate binds a candidate at the default branch head. self.assertEqual( { @@ -210,6 +285,126 @@ def refusal(repo: str, answer: object, **kwargs: object) -> str: ) self.assertEqual(seen[0]["self_logins"], frozenset()) + def test_a_pending_merge_past_two_minutes_is_read_again_and_counted( + self, + ) -> None: + # Issue #9577: the post-merge step takes about 1m41s and the room is + # read at about 2:01, so every pending lane merge left it unreadable. + verified = { + "readable": True, + "ok": True, + "autoMerges": [{"number": 5, "verified": True}], + } + fresh = policy.PostMergeGate(None, verified, lambda merge: True) + recheck = Recheck(fresh) + room = self.room( + shared(report=PENDING), + now=STALE_NOW, + post_merge=recheck.post_merge, + share=recheck.share, + ) + self.assertEqual( + (room.refusal, room.unreadable, room.count, room.cap, room.rung), + (None, None, 2, 5, "C2"), + ) + # The check ran from the floor read again, as the identity. + self.assertEqual(recheck.asked, [(NEW_FLOOR, IDENTITY)]) + # The in-flight count judges merges by the fresh answer, from its floor. + self.assertIs(self.seen["verified"], fresh.verified) + self.assertEqual(self.seen["merged_since"], NEW_FLOOR) + # Written back once, bound to the head and time it was read at. + self.assertEqual(len(recheck.shared), 1) + written = recheck.shared[0] + self.assertEqual( + ( + written.main_sha, + written.computed_at, + written.floor, + written.identity, + written.lane_merges_only, + written.self_logins, + written.gate, + ), + (NEW_MAIN, STALE_NOW, NEW_FLOOR, IDENTITY, True, frozenset(), fresh), + ) + + def test_a_recompute_that_finds_a_failure_refuses(self) -> None: + # A verification failure after computedAt still holds the lane off. + failed = { + "readable": True, + "ok": False, + "failures": [{"signal": "red-main", "number": 5}], + } + recheck = Recheck(policy.PostMergeGate("failure:red-main#5", failed, None)) + room = self.room( + shared(report=PENDING), + now=STALE_NOW, + post_merge=recheck.post_merge, + share=recheck.share, + ) + self.assertEqual( + (room.refusal, room.unreadable), ("post-merge:failure:red-main#5", None) + ) + self.assertFalse(self.seen, "no in-flight read after a refusal") + # A readable failure goes back to the file, as the gate's does. + self.assertEqual(len(recheck.shared), 1) + + def test_a_demotion_record_filed_since_holds_a_recomputed_room_off( + self, + ) -> None: + # Past two minutes the age refuses before the demotion is re-checked, + # so the recompute reads the demotion again. + recheck = Recheck(policy.PostMergeGate(None, PENDING, lambda merge: False)) + room = self.room( + shared(report=PENDING), + now=STALE_NOW, + post_merge=recheck.post_merge, + share=recheck.share, + fresh_demotion=policy.Demotion(True, "record-open", (8,)), + ) + self.assertEqual(room.refusal, "rung-off:demotion") + + def test_a_failed_recompute_leaves_the_room_unreadable(self) -> None: + unreadable = {"readable": False, "ok": False, "reason": "runs-unreadable"} + cases = { + "check raises": Recheck(error=RuntimeError("runs 502")), + "check unreadable": Recheck( + policy.PostMergeGate("runs-unreadable", unreadable, None) + ), + } + for name, recheck in cases.items(): + with self.subTest(name): + room = self.room( + shared(report=PENDING), + now=STALE_NOW, + post_merge=recheck.post_merge, + share=recheck.share, + ) + self.assertIsNone(room.refusal) + self.assertTrue( + str(room.unreadable).startswith( + "post-merge answer unverified-stale" + ), + room.unreadable, + ) + self.assertEqual(recheck.shared, [], "nothing unreadable is written") + # A head that cannot be read is a failed recompute too. + recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) + room = self.room( + shared(report=PENDING), + now=STALE_NOW, + post_merge=recheck.post_merge, + share=recheck.share, + head_error=policy.PolicyReadError("head 502"), + ) + self.assertIsNone(room.refusal) + self.assertTrue(room.unreadable) + self.assertEqual((recheck.asked, recheck.shared), ([], [])) + # With no check to run, it stays unreadable as before. + room = self.room(shared(report=PENDING), now=STALE_NOW) + self.assertEqual(room.unreadable, "post-merge answer unverified-stale") + self.assertFalse(self.seen, "no in-flight read after an unusable answer") + def test_an_active_demotion_is_not_re_read(self) -> None: # Already refusing: a changed record cannot make it allow. room = self.room(shared(demoted=True), demotion_changed=True) @@ -244,10 +439,18 @@ class LaneRoomCommand(unittest.TestCase): def test_it_prints_the_room_from_the_post_merge_report(self) -> None: seen: list[object] = [] + hooks: dict[str, object] = {} + def room( - repo: str, answer: object, *, lane_rung_cap: str | None + repo: str, + answer: object, + *, + lane_rung_cap: str | None, + post_merge: object, + share: object, ) -> policy.LaneRoom: seen.append((repo, answer, lane_rung_cap)) + hooks.update(post_merge=post_merge, share=share) return policy.LaneRoom(count=1, cap=5, queued=frozenset({3}), rung="C2") out = io.StringIO() @@ -283,6 +486,31 @@ def room( "rung": "C2", }, ) + self.assertTrue(callable(hooks["post_merge"])) + + def test_a_recompute_is_written_back_to_the_report_it_read(self) -> None: + # The gate's write-back: the next reader of the same file reuses it. + hooks: dict[str, object] = {} + + def room(repo: str, answer: object, **kwargs: object) -> policy.LaneRoom: + hooks.update(kwargs) + return policy.LaneRoom() + + with tempfile.TemporaryDirectory() as tmp: + path = str(pathlib.Path(tmp) / "post-merge.json") + with ( + mock.patch.object(policy, "lane_room", side_effect=room), + contextlib.redirect_stdout(io.StringIO()), + ): + ve.main(["lane-room", "--repo", REPO, "--postmerge-result", path]) + answer = shared(report={**PENDING, "ok": True}) + hooks["share"](answer) # type: ignore[operator] + back = ve.postmerge.read_shared(path) + assert back is not None + self.assertEqual( + (back.main_sha, back.identity, back.floor, back.computed_at), + (MAIN, IDENTITY, FLOOR, FLOOR), + ) def test_a_missing_report_is_unreadable(self) -> None: out = io.StringIO() From 916e4ba9388d9a6266322edce02c341ffafa0294 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 01:45:47 -0400 Subject: [PATCH 2/5] fix(pr-merge): keep an unverified-stale answer nothing has moved since; drop the room's write-back The room's write-back went to a file nothing in the job reads again, so it is removed; the gate keeps its own. On unverified-stale the room now keeps the answer when main's head, its config commit, the demotion records and the unverified merges' full and canary runs are unchanged (one run listing: no conclusion since, no merge past the dispatch deadline), and runs the full post-merge check only when one has moved. Refs: #9577 Co-Authored-By: Claude Opus 5.5 --- .../fix-lane-room-recompute-9577a1c3.md | 2 +- .../fix-lane-room-recompute-9577a1c3.md | 2 +- .../scripts/pr-merge/enable-auto-merge.mjs | 4 +- docs/conventions/pr-pipeline/README.md | 6 +- .../scripts/babysit_merge_policy.py | 64 +++-- .../babysit-prs/scripts/babysit_postmerge.py | 58 ++++ .../scripts/babysit_verify_evidence.py | 11 +- .../scripts/tests/test_babysit_lane_room.py | 247 +++++++++++++----- 8 files changed, 302 insertions(+), 92 deletions(-) diff --git a/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md b/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md index c440411d28..288c31c9d4 100644 --- a/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md +++ b/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- A pending lane merge no longer makes `enable-auto-merge` disarm every armed lanes App pull request. The post-merge step takes about as long as the gate's two-minute reuse age, so the lane's room read the report as too old. It now checks again, as the gate does. When the room is held, by a refusal or because it cannot be read, the run raises one warning annotation and one step-summary line naming the reason and each lanes App pull request it left unarmed. It posts no comment on the pull requests. +- A pending lane merge no longer makes `enable-auto-merge` disarm every armed lanes App pull request. The post-merge step takes about as long as the gate's two-minute reuse age, so the lane's room read the report as too old. It now keeps the report while nothing it was read from has moved, and otherwise checks again as the gate does. When the room is held, by a refusal or because it cannot be read, the run raises one warning annotation and one step-summary line naming the reason and each lanes App pull request it left unarmed. It posts no comment on the pull requests. diff --git a/.changes/source-control/fix-lane-room-recompute-9577a1c3.md b/.changes/source-control/fix-lane-room-recompute-9577a1c3.md index 8650edfce3..7c115012e8 100644 --- a/.changes/source-control/fix-lane-room-recompute-9577a1c3.md +++ b/.changes/source-control/fix-lane-room-recompute-9577a1c3.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `babysit_verify_evidence.py lane-room` no longer reports the lane's room unreadable when the run's post-merge answer is past the gate's two-minute reuse age only because a lane merge is still unverified (`unverified-stale`). It checks again the way the gate does (`babysit_merge_policy.lane_room` with the gate's `post_merge` and `share`), writes the fresh answer back to the `--postmerge-result` file, and uses it, so a failure found since still refuses. A check that fails or cannot be read leaves the room unreadable, and every other refusal reads as before. `babysit_postmerge.gate_reader` is the one post-merge reader the gate and the room share. +- `babysit_verify_evidence.py lane-room` no longer reports the lane's room unreadable when the run's post-merge answer is past the gate's two-minute reuse age only because a lane merge is still unverified (`unverified-stale`). It keeps the answer when nothing it was read from has moved: main's head, its config commit, the demotion records, and the unverified merges' full and canary runs (`babysit_postmerge.pending_runs_unchanged`, one run listing: none reached a conclusion since and none is past the dispatch deadline). Otherwise it runs the post-merge check again as the gate does (`babysit_merge_policy.lane_room` with the gate's `post_merge`), so a failure found since still refuses. A check that fails or cannot be read leaves the room unreadable, and every other refusal reads as before. `babysit_postmerge.gate_reader` is the one post-merge reader the gate and the room share. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index dc2777ffc5..679dd2b978 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -54,8 +54,8 @@ // 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 checked again -// first, as the gate does. A held room raises one warning annotation and +// 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 // one step-summary line naming why and each lane pull request it left // unarmed. // - pr-merge's gate would not hold it on its risk score: `evidence diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 158cde3ee9..ccd2565073 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1114,8 +1114,10 @@ waits for the same run's `post-merge-dispatch` job and reads the lane's room fro 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 -two-minute reuse age only because a lane merge is still unverified is checked again and written -back, as the gate does, and holds only when that check cannot be read): while the gate holds its lane off (a demotion record, +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, and no merge is past the six-hour dispatch deadline), and +otherwise checked again as the gate does, holding only when that check cannot be read): while the gate holds its lane off (a demotion record, `merge.rung`, the loop setting or the rung cap off, a lane merge that failed its post-merge check) nothing is armed and every armed lanes App pull request is disarmed; at `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests the queue 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 1545586296..bd1a1bfdd7 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 @@ -4490,6 +4490,29 @@ def report(self) -> dict[str, Any]: } +def _still_current( + repo: str, + default: str, + identity: str, + shared: SharedPostMerge, + runs_unchanged: Callable[[SharedPostMerge, str], bool], +) -> bool: + """Whether the post-merge check would answer `shared` again, in order: + the default branch head is its `main_sha`, the head's config commit is its + `config_sha` (with the demotion unchanged, the same floor), no demotion + record was filed or changed since (`_demotion_changed`), and + `runs_unchanged`: none of its unverified merges' verifying runs reached a + conclusion since. Raises `PolicyReadError`.""" + owner, name = _split_repo(repo) + _, head = _default_branch_head(repo, default) + return ( + head == shared.main_sha + and newest_config_commit(repo, head) == shared.config_sha + and not _demotion_changed(owner, name, shared, identity) + and runs_unchanged(shared, default) + ) + + def _recompute_shared( repo: str, default: str, @@ -4536,16 +4559,18 @@ def lane_room( lane_rung_cap: str | None, now: datetime | None = None, post_merge: Callable[[datetime, str], PostMergeGate] | None = None, - share: Callable[[SharedPostMerge], object] | None = None, + runs_unchanged: Callable[[SharedPostMerge, str], bool] | None = None, ) -> LaneRoom: """Whether the pr-merge lane may merge one more of its own pull requests now, for native auto-merge, which skips `assess`: the run-level refusals `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` and `share` are `assess`'s: an answer refused only as - `unverified-stale` is read again as the gate reads it (`_recompute_shared`), - and a readable one goes to `share` and stands in for `shared`. + `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`. - A config naming another authority stops (`enabled_authority`). - A post-merge refusal stops (`post-merge:`). @@ -4562,9 +4587,9 @@ def lane_room( 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 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`.""" + `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`.""" if shared is None: return LaneRoom(unreadable="post-merge answer missing or unreadable") try: @@ -4593,17 +4618,24 @@ def lane_room( now=now, ) if unusable == "unverified-stale" and post_merge is not None: + # Kept when nothing it was read from moved, else read again. try: - fresh = _recompute_shared( - repo, default, identity, shared.self_logins, post_merge, now + current = runs_unchanged is not None and _still_current( + repo, default, identity, shared, runs_unchanged ) - except (RuntimeError, ValueError, OSError) as exc: - return LaneRoom(unreadable=f"post-merge answer {unusable}: {exc}") - if fresh is None: - return LaneRoom(unreadable=f"post-merge answer {unusable}: unreadable") - if share is not None: - share(fresh) - shared, unusable = fresh, None + except (RuntimeError, ValueError, OSError): + current = False + if not current: + try: + fresh = _recompute_shared( + repo, default, identity, shared.self_logins, post_merge, now + ) + except (RuntimeError, ValueError, OSError) as exc: + return LaneRoom(unreadable=f"post-merge answer {unusable}: {exc}") + if fresh is None: + return LaneRoom(unreadable=f"post-merge answer {unusable}: unreadable") + shared = fresh + unusable = None if unusable: return LaneRoom(unreadable=f"post-merge answer {unusable}") if shared.gate.refusal: diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py index 7e6028f91f..6c1e4ac5d6 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py @@ -3117,6 +3117,64 @@ def post_merge(floor: datetime, identity: str) -> policy.PostMergeGate: return post_merge +VERIFYING_WORKFLOWS = frozenset( + f".github/workflows/{w}" for w in (FULL_RUN_WORKFLOW, CANARY_WORKFLOW) +) + + +def pending_runs_unchanged( + repo: str, + shared: policy.SharedPostMerge, + branch: str, + *, + now: datetime | None = None, +) -> bool: + """Whether `check` would judge `shared`'s unverified merges as it did, + from one run listing: no full or canary run on `branch` created since the + earliest of them completed or was re-run since `computed_at` (less + `policy.SINCE_CLOCK_MARGIN`), and none has passed `DISPATCH_DEADLINE`, + after which a missing dispatch or pin is a failure. A run started since + and still running only stays pending. A listing that cannot be read or + is not whole, or an entry without a merge time, is False.""" + pending = [ + m + for m in map(json_object, json_array(shared.gate.report.get("autoMerges"))) + if m.get("verified") is not True + ] + if not pending: + return False + try: + earliest = min(_time(m.get("mergedAt")) for m in pending) + if (now or datetime.now(UTC)) > earliest + DISPATCH_DEADLINE: + return False + owner, name = _split_repo(repo) + created = quote( + f">={earliest.astimezone(UTC).strftime('%Y-%m-%dT%H:%M:%SZ')}", safe="" + ) + payload = json_object( + _read( + [ + "api", + f"repos/{owner}/{name}/actions/runs?branch={quote(branch, safe='')}" + f"&created={created}&per_page=100", + ], + "verifying runs", + ) + ) + runs = json_array(payload.get("workflow_runs")) + if payload.get("total_count") != len(runs): + return False + since = shared.computed_at - policy.SINCE_CLOCK_MARGIN + return not any( + run.get("path") in VERIFYING_WORKFLOWS + and run.get("status") == "completed" + and _time(run.get("updated_at")) >= since + for run in map(json_object, runs) + ) + except PostMergeReadError: + return False + + def read_shared(path: str) -> policy.SharedPostMerge | None: """The run's shared answer from this script's own report at `path`, or None when it is missing, malformed, unreadable as a verdict, or has no 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 7954116c8a..3555b90074 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 @@ -108,7 +108,6 @@ import argparse import contextlib -import functools import hashlib import io import json @@ -1550,7 +1549,8 @@ 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 checked again and written back.""" + refused only as `unverified-stale` is kept while nothing it was read from + has moved, else checked again.""" parser = argparse.ArgumentParser( prog="babysit_verify_evidence.py", allow_abbrev=False ) @@ -1576,9 +1576,12 @@ def main(argv: list[str] | None = None, *, now: datetime | None = None) -> int: args.repo, shared, lane_rung_cap=args.rung_cap, - # The gate's recompute and write-back, bound as the room binds. + # The gate's recompute, bound as the room binds, and the one run + # listing that spares it while the pending merges' runs hold. post_merge=postmerge.gate_reader(args.repo, (), lane_merges_only=True), - share=functools.partial(postmerge.write_shared, args.postmerge_result), + runs_unchanged=lambda answer, branch: postmerge.pending_runs_unchanged( + args.repo, answer, branch + ), ) print(json.dumps(answer.report())) return 0 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 bccf9844ba..5fcc194382 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 @@ -15,7 +15,6 @@ import json import pathlib import sys -import tempfile import unittest from datetime import UTC, datetime, timedelta from unittest import mock @@ -23,6 +22,7 @@ sys.path.insert(0, str(pathlib.Path(__file__).resolve().parent.parent)) import babysit_merge_policy as policy # noqa: E402 +import babysit_postmerge as pm # noqa: E402 import babysit_verify_evidence as ve # noqa: E402 REPO = "example-org/example-repo" @@ -39,7 +39,9 @@ PENDING = { "readable": True, "ok": True, - "autoMerges": [{"number": 5, "verified": False}], + "autoMerges": [ + {"number": 5, "verified": False, "mergedAt": "2026-10-01T00:00:00Z"} + ], } STALE_NOW = FLOOR + timedelta(minutes=3) @@ -82,14 +84,13 @@ def shared( class Recheck: """A fake post-merge check: `gate` or `error`, with each floor and - identity it was asked for, and each answer shared.""" + identity it was asked for.""" def __init__( self, gate: policy.PostMergeGate | None = None, error: Exception | None = None ) -> None: self.gate, self.error = gate, error self.asked: list[tuple[datetime, str]] = [] - self.shared: list[policy.SharedPostMerge] = [] def post_merge(self, floor: datetime, identity: str) -> policy.PostMergeGate: self.asked.append((floor, identity)) @@ -98,8 +99,17 @@ def post_merge(self, floor: datetime, identity: str) -> policy.PostMergeGate: assert self.gate is not None return self.gate - def share(self, answer: policy.SharedPostMerge) -> None: - self.shared.append(answer) + +def run( + workflow: str, *, status: str, updated: datetime, created: datetime = FLOOR +) -> dict[str, object]: + """One row of the actions runs listing.""" + return { + "path": f".github/workflows/{workflow}", + "status": status, + "created_at": created.isoformat(), + "updated_at": updated.isoformat(), + } class LaneRoom(unittest.TestCase): @@ -114,7 +124,10 @@ def room( now: datetime = NOW, demotion_changed: bool = False, post_merge: object = None, - share: object = None, + runs_unchanged: object = None, + head: str = NEW_MAIN, + config_commit: str = MAIN, + fresh_floor: datetime = NEW_FLOOR, head_error: Exception | None = None, fresh_demotion: policy.Demotion | None = None, ) -> policy.LaneRoom: @@ -141,7 +154,9 @@ def in_flight(repo: str, **kwargs: object) -> policy.InFlight: ), # shared_refusal's reads: the answer's own config commit and main # head, so only its age and demotion decide. - mock.patch.object(policy, "newest_config_commit", return_value=MAIN), + mock.patch.object( + policy, "newest_config_commit", return_value=config_commit + ), mock.patch.object(policy, "_get", return_value=(200, {"sha": MAIN})), # The recompute's reads at the default branch head: a newer head, # no demotion record, and a later floor than the old answer's. @@ -149,7 +164,7 @@ def in_flight(repo: str, **kwargs: object) -> policy.InFlight: policy, "_default_branch_head", side_effect=head_error, - return_value=("main", NEW_MAIN), + return_value=("main", head), ), mock.patch.object(policy, "permission_reader"), mock.patch.object( @@ -157,11 +172,11 @@ def in_flight(repo: str, **kwargs: object) -> policy.InFlight: "read_demotion", return_value=fresh_demotion or policy.Demotion(False, None, ()), ), - mock.patch.object(policy, "merged_since_floor", return_value=NEW_FLOOR), + mock.patch.object(policy, "merged_since_floor", return_value=fresh_floor), ): kwargs = {"post_merge": post_merge} if post_merge is not None else {} - if share is not None: - kwargs["share"] = share + if runs_unchanged is not None: + kwargs["runs_unchanged"] = runs_unchanged result = policy.lane_room( REPO, answer if answer is not None else shared(), @@ -241,14 +256,13 @@ def test_an_answer_the_gate_would_not_reuse_is_unreadable(self) -> None: room = self.room( shared(report=PENDING), post_merge=recheck.post_merge, - share=recheck.share, **kwargs, # 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, recheck.shared), ([], [])) + self.assertEqual(recheck.asked, []) def test_a_lane_merge_the_answer_does_not_count_is_unreadable(self) -> None: # The gate's reuse rule (shared_refusal) reads afresh when a lane @@ -262,13 +276,13 @@ 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, share=recheck.share) + room = self.room(post_merge=recheck.post_merge) 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, recheck.shared), ([], [])) + self.assertEqual(recheck.asked, []) # Bound as the gate binds a candidate at the default branch head. self.assertEqual( { @@ -301,7 +315,6 @@ def test_a_pending_merge_past_two_minutes_is_read_again_and_counted( shared(report=PENDING), now=STALE_NOW, post_merge=recheck.post_merge, - share=recheck.share, ) self.assertEqual( (room.refusal, room.unreadable, room.count, room.cap, room.rung), @@ -312,21 +325,6 @@ def test_a_pending_merge_past_two_minutes_is_read_again_and_counted( # The in-flight count judges merges by the fresh answer, from its floor. self.assertIs(self.seen["verified"], fresh.verified) self.assertEqual(self.seen["merged_since"], NEW_FLOOR) - # Written back once, bound to the head and time it was read at. - self.assertEqual(len(recheck.shared), 1) - written = recheck.shared[0] - self.assertEqual( - ( - written.main_sha, - written.computed_at, - written.floor, - written.identity, - written.lane_merges_only, - written.self_logins, - written.gate, - ), - (NEW_MAIN, STALE_NOW, NEW_FLOOR, IDENTITY, True, frozenset(), fresh), - ) def test_a_recompute_that_finds_a_failure_refuses(self) -> None: # A verification failure after computedAt still holds the lane off. @@ -340,14 +338,11 @@ def test_a_recompute_that_finds_a_failure_refuses(self) -> None: shared(report=PENDING), now=STALE_NOW, post_merge=recheck.post_merge, - share=recheck.share, ) self.assertEqual( (room.refusal, room.unreadable), ("post-merge:failure:red-main#5", None) ) self.assertFalse(self.seen, "no in-flight read after a refusal") - # A readable failure goes back to the file, as the gate's does. - self.assertEqual(len(recheck.shared), 1) def test_a_demotion_record_filed_since_holds_a_recomputed_room_off( self, @@ -359,7 +354,6 @@ def test_a_demotion_record_filed_since_holds_a_recomputed_room_off( shared(report=PENDING), now=STALE_NOW, post_merge=recheck.post_merge, - share=recheck.share, fresh_demotion=policy.Demotion(True, "record-open", (8,)), ) self.assertEqual(room.refusal, "rung-off:demotion") @@ -378,7 +372,6 @@ def test_a_failed_recompute_leaves_the_room_unreadable(self) -> None: shared(report=PENDING), now=STALE_NOW, post_merge=recheck.post_merge, - share=recheck.share, ) self.assertIsNone(room.refusal) self.assertTrue( @@ -387,24 +380,103 @@ def test_a_failed_recompute_leaves_the_room_unreadable(self) -> None: ), room.unreadable, ) - self.assertEqual(recheck.shared, [], "nothing unreadable is written") # A head that cannot be read is a failed recompute too. recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None)) room = self.room( shared(report=PENDING), now=STALE_NOW, post_merge=recheck.post_merge, - share=recheck.share, head_error=policy.PolicyReadError("head 502"), ) self.assertIsNone(room.refusal) self.assertTrue(room.unreadable) - self.assertEqual((recheck.asked, recheck.shared), ([], [])) + self.assertEqual(recheck.asked, []) # With no check to run, it stays unreadable as before. room = self.room(shared(report=PENDING), now=STALE_NOW) self.assertEqual(room.unreadable, "post-merge answer unverified-stale") self.assertFalse(self.seen, "no in-flight read after an unusable answer") + def current(self, **kwargs: object) -> tuple[policy.LaneRoom, Recheck, list]: + """The room for a pending answer read three minutes on, with the + default branch head, config commit and demotion all as it was read + unless `kwargs` moves one; `runs` is what the run check answers.""" + runs = kwargs.pop("runs", True) + asked: list[tuple[object, str]] = [] + + def runs_unchanged(answer: object, branch: str) -> bool: + asked.append((answer, branch)) + return runs(answer, branch) if callable(runs) else bool(runs) + + recheck = Recheck( + kwargs.pop("gate", policy.PostMergeGate(None, PENDING, lambda merge: False)) + ) + room = self.room( + kwargs.pop("answer", shared(report=PENDING)), + now=STALE_NOW, + post_merge=recheck.post_merge, + runs_unchanged=runs_unchanged, + **{"head": MAIN, **kwargs}, # type: ignore[arg-type] + ) + return room, recheck, asked + + def test_an_answer_nothing_has_moved_since_is_kept_without_a_check( + self, + ) -> None: + answer = shared(report=PENDING) + room, recheck, asked = self.current(answer=answer) + self.assertEqual( + (room.refusal, room.unreadable, room.count, room.cap), (None, None, 2, 5) + ) + self.assertEqual(recheck.asked, [], "no full post-merge check") + self.assertEqual(asked, [(answer, "main")]) + # Counted from the kept answer: its floor and its verified merges. + self.assertEqual(self.seen["merged_since"], FLOOR) + self.assertIs(self.seen["verified"], answer.gate.verified) + + def test_anything_moved_since_reads_the_answer_again(self) -> None: + cases = { + "(a) main's head moved": dict(head=NEW_MAIN), + # The same floor, so the gate's reuse rule does not refuse first. + "(b) the config commit moved": dict( + config_commit="f" * 40, fresh_floor=FLOOR + ), + "(c) a demotion record changed": dict(demotion_changed=True), + "(d) a verifying run reached a conclusion": dict(runs=False), + } + for name, kwargs in cases.items(): + with self.subTest(name): + room, recheck, _ = self.current(**kwargs) + self.assertEqual(len(recheck.asked), 1, "one full post-merge check") + self.assertEqual((room.refusal, room.unreadable), (None, None)) + + def test_a_pending_dispatch_run_that_completed_red_refuses(self) -> None: + # The case the two-minute rule exists for: main's head has not moved, + # but merge #5's dispatch run finished red after the answer was read. + listing = { + "total_count": 1, + "workflow_runs": [ + run( + "pr-require-checks.yml", + status="completed", + updated=STALE_NOW - timedelta(seconds=30), + ) + ], + } + failed = { + "readable": True, + "ok": False, + "failures": [{"signal": "dispatch", "number": 5}], + } + with mock.patch.object(pm, "_read", return_value=listing): + room, recheck, _ = self.current( + runs=lambda answer, branch: pm.pending_runs_unchanged( + REPO, answer, branch, now=STALE_NOW + ), + gate=policy.PostMergeGate("failure:dispatch#5", failed, None), + ) + self.assertEqual(len(recheck.asked), 1) + self.assertEqual(room.refusal, "post-merge:failure:dispatch#5") + def test_an_active_demotion_is_not_re_read(self) -> None: # Already refusing: a changed record cannot make it allow. room = self.room(shared(demoted=True), demotion_changed=True) @@ -435,6 +507,72 @@ def test_a_failed_read_refuses_nothing_and_allows_nothing(self) -> None: self.assertTrue(room.unreadable) +class PendingRunsUnchanged(unittest.TestCase): + """(d): the verifying runs of the answer's unverified merges, from one + listing. Expected from the check's own rule (`_judge`): a full or canary + run that reaches a conclusion changes the verdict, one still running + leaves it pending, and past `DISPATCH_DEADLINE` a merge with no dispatch + run fails.""" + + def unchanged( + self, rows: list[dict[str, object]], *, total: int | None = None, now=STALE_NOW + ) -> tuple[bool, list[list[str]]]: + calls: list[list[str]] = [] + + def read(args: list[str], label: str) -> dict[str, object]: + calls.append(args) + return { + "total_count": len(rows) if total is None else total, + "workflow_runs": rows, + } + + with mock.patch.object(pm, "_read", side_effect=read): + answer = pm.pending_runs_unchanged( + REPO, shared(report=PENDING), "main", now=now + ) + return answer, calls + + def test_runs_still_running_or_finished_before_the_answer_hold(self) -> None: + rows = [ + run("pr-require-checks.yml", status="in_progress", updated=STALE_NOW), + run( + "post-merge-detect-silent-revert.yml", + status="queued", + updated=STALE_NOW, + ), + # Finished before the answer was read (less the clock margin). + run( + "post-merge-detect-silent-revert.yml", + status="completed", + updated=FLOOR - policy.SINCE_CLOCK_MARGIN - timedelta(seconds=1), + ), + # Another workflow's run judges nothing. + run("ci-status.yml", status="completed", updated=STALE_NOW), + ] + answer, calls = self.unchanged(rows) + self.assertTrue(answer) + # One listing call, of runs on main created since merge #5. + self.assertEqual(len(calls), 1) + self.assertIn("branch=main", calls[0][1]) + self.assertIn("created=%3E%3D2026-10-01T00%3A00%3A00Z", calls[0][1]) + + def test_a_conclusion_since_a_partial_listing_or_the_deadline_moves_it( + self, + ) -> None: + done = run("pr-require-checks.yml", status="completed", updated=STALE_NOW) + canary = run( + "post-merge-detect-silent-revert.yml", status="completed", updated=FLOOR + ) + self.assertFalse(self.unchanged([done])[0]) + self.assertFalse(self.unchanged([canary])[0]) + self.assertFalse(self.unchanged([], total=101)[0]) + self.assertFalse( + self.unchanged([], now=FLOOR + pm.DISPATCH_DEADLINE + timedelta(seconds=1))[ + 0 + ] + ) + + class LaneRoomCommand(unittest.TestCase): def test_it_prints_the_room_from_the_post_merge_report(self) -> None: seen: list[object] = [] @@ -447,10 +585,10 @@ def room( *, lane_rung_cap: str | None, post_merge: object, - share: object, + runs_unchanged: object, ) -> policy.LaneRoom: seen.append((repo, answer, lane_rung_cap)) - hooks.update(post_merge=post_merge, share=share) + hooks.update(post_merge=post_merge, runs_unchanged=runs_unchanged) return policy.LaneRoom(count=1, cap=5, queued=frozenset({3}), rung="C2") out = io.StringIO() @@ -487,30 +625,7 @@ def room( }, ) self.assertTrue(callable(hooks["post_merge"])) - - def test_a_recompute_is_written_back_to_the_report_it_read(self) -> None: - # The gate's write-back: the next reader of the same file reuses it. - hooks: dict[str, object] = {} - - def room(repo: str, answer: object, **kwargs: object) -> policy.LaneRoom: - hooks.update(kwargs) - return policy.LaneRoom() - - with tempfile.TemporaryDirectory() as tmp: - path = str(pathlib.Path(tmp) / "post-merge.json") - with ( - mock.patch.object(policy, "lane_room", side_effect=room), - contextlib.redirect_stdout(io.StringIO()), - ): - ve.main(["lane-room", "--repo", REPO, "--postmerge-result", path]) - answer = shared(report={**PENDING, "ok": True}) - hooks["share"](answer) # type: ignore[operator] - back = ve.postmerge.read_shared(path) - assert back is not None - self.assertEqual( - (back.main_sha, back.identity, back.floor, back.computed_at), - (MAIN, IDENTITY, FLOOR, FLOOR), - ) + self.assertTrue(callable(hooks["runs_unchanged"])) def test_a_missing_report_is_unreadable(self) -> None: out = io.StringIO() From 80f859f34c3d500c7814235e8e52ce3c71a2edaf Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 02:02:58 -0400 Subject: [PATCH 3/5] fix(pr-merge): carry lane records into the room and end a kept answer at its quarantine expiry The enable-auto-merge job now reads the lane records the last run kept, as post-merge-dispatch does, and hands them to the room's post-merge check, so a full check answers those records without an artifact and a run read each. The post-merge report records when the earliest quarantine that excused a failing suite expires (excusesExpireAt), and the room's cheap freshness check runs the full check once that time passes, or when an answer does not carry the field. Refs: #9577 Co-Authored-By: Claude Opus 5.5 --- .../fix-lane-room-recompute-9577a1c3.md | 2 +- .../fix-lane-room-recompute-9577a1c3.md | 2 +- .github/scripts/pr-merge/workflow.test.mjs | 19 ++++++ .github/workflows/pr-merge.yml | 15 +++++ docs/conventions/pr-pipeline/README.md | 5 +- .../babysit-prs/scripts/babysit_postmerge.py | 67 +++++++++++++------ .../scripts/tests/test_babysit_lane_room.py | 44 +++++++++++- .../test_babysit_postmerge_quarantine.py | 23 +++++++ 8 files changed, 152 insertions(+), 25 deletions(-) diff --git a/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md b/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md index 288c31c9d4..3540b02f11 100644 --- a/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md +++ b/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- A pending lane merge no longer makes `enable-auto-merge` disarm every armed lanes App pull request. The post-merge step takes about as long as the gate's two-minute reuse age, so the lane's room read the report as too old. It now keeps the report while nothing it was read from has moved, and otherwise checks again as the gate does. When the room is held, by a refusal or because it cannot be read, the run raises one warning annotation and one step-summary line naming the reason and each lanes App pull request it left unarmed. It posts no comment on the pull requests. +- A pending lane merge no longer makes `enable-auto-merge` disarm every armed lanes App pull request. The post-merge step takes about as long as the gate's two-minute reuse age, so the lane's room read the report as too old. It now keeps the report while nothing it was read from has moved, and otherwise checks again as the gate does, with the lane records the last run kept, so that check reads them without an artifact or run read each. When the room is held, by a refusal or because it cannot be read, the run raises one warning annotation and one step-summary line naming the reason and each lanes App pull request it left unarmed. It posts no comment on the pull requests. diff --git a/.changes/source-control/fix-lane-room-recompute-9577a1c3.md b/.changes/source-control/fix-lane-room-recompute-9577a1c3.md index 7c115012e8..f777cb0998 100644 --- a/.changes/source-control/fix-lane-room-recompute-9577a1c3.md +++ b/.changes/source-control/fix-lane-room-recompute-9577a1c3.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `babysit_verify_evidence.py lane-room` no longer reports the lane's room unreadable when the run's post-merge answer is past the gate's two-minute reuse age only because a lane merge is still unverified (`unverified-stale`). It keeps the answer when nothing it was read from has moved: main's head, its config commit, the demotion records, and the unverified merges' full and canary runs (`babysit_postmerge.pending_runs_unchanged`, one run listing: none reached a conclusion since and none is past the dispatch deadline). Otherwise it runs the post-merge check again as the gate does (`babysit_merge_policy.lane_room` with the gate's `post_merge`), so a failure found since still refuses. A check that fails or cannot be read leaves the room unreadable, and every other refusal reads as before. `babysit_postmerge.gate_reader` is the one post-merge reader the gate and the room share. +- `babysit_verify_evidence.py lane-room` no longer reports the lane's room unreadable when the run's post-merge answer is past the gate's two-minute reuse age only because a lane merge is still unverified (`unverified-stale`). It keeps the answer when nothing it was read from has moved: main's head, its config commit, the demotion records, and the unverified merges' full and canary runs (`babysit_postmerge.pending_runs_unchanged`, one run listing: none reached a conclusion since, none is past the dispatch deadline, and no quarantine that excused a failing suite has expired). Otherwise it runs the post-merge check again as the gate does (`babysit_merge_policy.lane_room` with the gate's `post_merge`), so a failure found since still refuses. A check that fails or cannot be read leaves the room unreadable, and every other refusal reads as before. `babysit_postmerge.gate_reader` is the one post-merge reader the gate and the room share. The post-merge report now carries `excusesExpireAt`, when the earliest quarantine that excused a failing suite expires. diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index fce9696f03..485effe3e3 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -218,6 +218,25 @@ test("enable-auto-merge reads the lane's room from this run's post-merge-dispatc assert.ok(names.indexOf("Read this run's post-merge report") < names.indexOf("Arm and disarm auto-merge")); }); +// A room that checks the post-merge answer again reads the lane records the +// last run kept, as post-merge-dispatch's check does, so the check answers +// them with no artifact or run read each (465 calls and 4m20s without them, +// past the room's VERIFY_TIMEOUT_MS). +test("enable-auto-merge hands the room's post-merge check the lane records the last run kept", () => { + const arm = job("enable-auto-merge"); + const records = /- name: Read the lane records the last run kept\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)?.[1]; + assert.ok(records, "a lane-records read"); + assert.match(records, /\n {8}continue-on-error: true\n/); + assert.match(records, /\n {10}ACTIONS_TOKEN: \$\{\{ github\.token \}\}\n {10}LANE_RECORDS_PATH: \$\{\{ runner\.temp \}\}\/lane-records\.json\n/); + assert.match(records, /\n {8}run: node \.base\/\.github\/scripts\/pr-merge\/lane-records\.mjs read\n/); + const step = /- name: Arm and disarm auto-merge\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)[1]; + assert.match(step, /\n {10}BABYSIT_POSTMERGE_LANE_RECORDS: \$\{\{ runner\.temp \}\}\/lane-records\.json\n/); + const names = [...arm.matchAll(/\n {6}- name: (.*)/g)].map((m) => m[1]); + assert.ok(names.indexOf("Read the lane records the last run kept") < names.indexOf("Arm and disarm auto-merge")); + // Within the job's own permissions: the read needs actions: read alone. + assert.match(arm, /\n {6}actions: read #/); +}); + test("enable-auto-merge's step timeouts fit inside its job's", () => { const arm = job("enable-auto-merge"); const total = Number(/\n {4}timeout-minutes: ([0-9]+)\n/.exec(arm)[1]); diff --git a/.github/workflows/pr-merge.yml b/.github/workflows/pr-merge.yml index 415e65bf31..10879ebb7b 100644 --- a/.github/workflows/pr-merge.yml +++ b/.github/workflows/pr-merge.yml @@ -1496,6 +1496,19 @@ jobs: name: pr-merge-post-merge-report path: ${{ runner.temp }}/post-merge + # The lane records earlier checks proved, as post-merge-dispatch reads + # them, for a room whose post-merge answer must be checked again: the + # check answers those records with no read. Any error leaves no file, + # and the check reads each record. Rules: lane-records.mjs. + - name: Read the lane records the last run kept + if: steps.lane-token.outputs.token != '' && (github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') + continue-on-error: true + timeout-minutes: 2 + env: + ACTIONS_TOKEN: ${{ github.token }} + LANE_RECORDS_PATH: ${{ runner.temp }}/lane-records.json + run: node .base/.github/scripts/pr-merge/lane-records.mjs read + # While merge.report-only is anything but false it reports each # decision and writes nothing. - name: Arm and disarm auto-merge @@ -1514,6 +1527,8 @@ jobs: # rung cap as the merge job passes it, empty meaning no cap. POSTMERGE_RESULT: ${{ runner.temp }}/post-merge/post-merge.json RUNG_CAP: ${{ vars.CLAUDE_LANES_MERGE_RUNG_CAP }} + # The lane records read above, for the room's post-merge check. + BABYSIT_POSTMERGE_LANE_RECORDS: ${{ runner.temp }}/lane-records.json # Name the two tokens' rate-limit readings for the reader's gh calls # (api-budget.bash): the lane token by default, the job token for # the Actions reads that carry it. diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 1f34f522c6..99f8857aa9 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1116,8 +1116,9 @@ one the gate would not reuse (`shared_refusal`: too old, a demotion record chang merge landed since that it does not count), it holds as a refusal 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, and no merge is past the six-hour dispatch deadline), and -otherwise checked again as the gate does, holding only when that check cannot be read): while the gate holds its lane off (a demotion record, +were (no run reached a conclusion since, no merge is past the six-hour dispatch deadline, and no +quarantine that excused a failing suite has expired), and otherwise checked again as the gate does, +with the lane records the last run kept, holding only when that check cannot be read): while the gate holds its lane off (a demotion record, `merge.rung`, the loop setting or the rung cap off, a lane merge that failed its post-merge check) nothing is armed and every armed lanes App pull request is disarmed; at `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests the queue diff --git a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py index 6c1e4ac5d6..50284dfd69 100755 --- a/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py +++ b/plugins/source-control/skills/babysit-prs/scripts/babysit_postmerge.py @@ -303,6 +303,8 @@ class PostMergeVerdict: # for a fresh run. hold: str | None = None recover_dispatch: bool = False + # When the earliest quarantine that excused a failing suite expires. + excuses_expire_at: datetime | None = None @property def ok(self) -> bool: @@ -370,6 +372,9 @@ def report(self) -> dict[str, Any]: "needsDispatch": self.needs_dispatch, "warnings": list(self.warnings), "branch": self.branch, + "excusesExpireAt": ( + self.excuses_expire_at.isoformat() if self.excuses_expire_at else None + ), } @@ -1070,6 +1075,9 @@ class _Context: quarantine_issues: dict[str, dict[str, Any]] | None = None # Excused failing suites, by (suite, sighting). quarantines: dict[tuple[str, str], Quarantine] = field(default_factory=dict) + # The earliest time a quarantine that excused a failing suite expires, + # after which the same runs judge differently (`excuse_until`). + excuses_expire_at: datetime | None = None # The branch's dispatch runs at each commit, read by SHA. dispatch_at_cache: dict[str, dict[int, Run]] = field(default_factory=dict) # The lane records file (`LANE_RECORDS_ENV`), its records once loaded, @@ -1164,16 +1172,18 @@ def expired(self, run: Run, attempt: int, checks: frozenset[str]) -> frozenset[s return frozenset() issues = self._quarantine_issues() days = self._quarantine_days(run.head_sha) - return frozenset( - check - for check, suites in named.items() - if any( - not is_gate_suite(suite) - and (issue := issues.get(suite)) is not None - and self.now >= _time(issue.get("created_at")) + timedelta(days=days) - for suite in suites - ) - ) + expired: set[str] = set() + for check, suites in named.items(): + for suite in suites: + issue = None if is_gate_suite(suite) else issues.get(suite) + if issue is None: + continue + ends = _time(issue.get("created_at")) + timedelta(days=days) + if self.now >= ends: + expired.add(check) + else: + self.excuse_until(ends) + return frozenset(expired) def demoting( self, run: Run, attempt: int, checks: frozenset[str] @@ -1205,17 +1215,27 @@ def demoting( demote.add(check) continue issue = issues.get(suite) - if issue is not None and self.now >= _time( - issue.get("created_at") - ) + timedelta(days=days): + ends = ( + None + if issue is None + else _time(issue.get("created_at")) + timedelta(days=days) + ) + if ends is not None and self.now >= ends: demote.add(check) else: + if ends is not None: + self.excuse_until(ends) excused.append(Quarantine(suite, sighting, run.head_sha)) for q in excused: self.quarantines[(q.suite, q.run_url)] = q self.demoting_cache[key] = frozenset(demote) return self.demoting_cache[key] + def excuse_until(self, ends: datetime) -> None: + """Note a quarantine excuse that holds until `ends`.""" + if self.excuses_expire_at is None or ends < self.excuses_expire_at: + self.excuses_expire_at = ends + def failing_suites( self, run_id: int, attempt: int, check: str ) -> frozenset[str] | None: @@ -2182,6 +2202,7 @@ def lane_prs_at(sha: str) -> str | None: warnings=tuple(warnings), branch=branch, quarantines=tuple(ctx.quarantines.values()), + excuses_expire_at=ctx.excuses_expire_at, ) @@ -3132,20 +3153,28 @@ def pending_runs_unchanged( """Whether `check` would judge `shared`'s unverified merges as it did, from one run listing: no full or canary run on `branch` created since the earliest of them completed or was re-run since `computed_at` (less - `policy.SINCE_CLOCK_MARGIN`), and none has passed `DISPATCH_DEADLINE`, - after which a missing dispatch or pin is a failure. A run started since - and still running only stays pending. A listing that cannot be read or - is not whole, or an entry without a merge time, is False.""" + `policy.SINCE_CLOCK_MARGIN`), none has passed `DISPATCH_DEADLINE`, after + which a missing dispatch or pin is a failure, and no quarantine that + excused a failing suite has expired (`excusesExpireAt`; an answer + without the field is False). Those are `_judge`'s time-based excuses. A + run started since and still running only stays pending. A listing that + cannot be read or is not whole, or an entry without a merge time, is + False.""" pending = [ m for m in map(json_object, json_array(shared.gate.report.get("autoMerges"))) if m.get("verified") is not True ] - if not pending: + report = shared.gate.report + if not pending or "excusesExpireAt" not in report: return False + now = now or datetime.now(UTC) try: earliest = min(_time(m.get("mergedAt")) for m in pending) - if (now or datetime.now(UTC)) > earliest + DISPATCH_DEADLINE: + excuse = report["excusesExpireAt"] + if now > earliest + DISPATCH_DEADLINE or ( + excuse is not None and now >= _time(excuse) + ): return False owner, name = _split_repo(repo) created = quote( 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 5fcc194382..b6fcd9386d 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 @@ -42,6 +42,7 @@ "autoMerges": [ {"number": 5, "verified": False, "mergedAt": "2026-10-01T00:00:00Z"} ], + "excusesExpireAt": None, } STALE_NOW = FLOOR + timedelta(minutes=3) @@ -449,6 +450,24 @@ def test_anything_moved_since_reads_the_answer_again(self) -> None: self.assertEqual(len(recheck.asked), 1, "one full post-merge check") self.assertEqual((room.refusal, room.unreadable), (None, None)) + def test_a_quarantine_that_expired_since_reads_the_answer_again(self) -> None: + # (e): the answer excused a failing suite under a quarantine that + # expired a minute after it was read, so the same red run now demotes. + report = { + **PENDING, + "excusesExpireAt": (FLOOR + timedelta(minutes=1)).isoformat(), + } + listing = {"total_count": 0, "workflow_runs": []} + with mock.patch.object(pm, "_read", return_value=listing): + room, recheck, _ = self.current( + answer=shared(report=report), + runs=lambda answer, branch: pm.pending_runs_unchanged( + REPO, answer, branch, now=STALE_NOW + ), + ) + self.assertEqual(len(recheck.asked), 1, "one full post-merge check") + self.assertEqual((room.refusal, room.unreadable), (None, None)) + def test_a_pending_dispatch_run_that_completed_red_refuses(self) -> None: # The case the two-minute rule exists for: main's head has not moved, # but merge #5's dispatch run finished red after the answer was read. @@ -515,7 +534,12 @@ class PendingRunsUnchanged(unittest.TestCase): run fails.""" def unchanged( - self, rows: list[dict[str, object]], *, total: int | None = None, now=STALE_NOW + self, + rows: list[dict[str, object]], + *, + total: int | None = None, + now=STALE_NOW, + report: dict[str, object] = PENDING, ) -> tuple[bool, list[list[str]]]: calls: list[list[str]] = [] @@ -528,7 +552,7 @@ def read(args: list[str], label: str) -> dict[str, object]: with mock.patch.object(pm, "_read", side_effect=read): answer = pm.pending_runs_unchanged( - REPO, shared(report=PENDING), "main", now=now + REPO, shared(report=report), "main", now=now ) return answer, calls @@ -566,6 +590,22 @@ def test_a_conclusion_since_a_partial_listing_or_the_deadline_moves_it( self.assertFalse(self.unchanged([done])[0]) self.assertFalse(self.unchanged([canary])[0]) self.assertFalse(self.unchanged([], total=101)[0]) + # A quarantine excuse that ended since, and an answer that does not + # say when its excuses end. + self.assertFalse( + self.unchanged( + [], report={**PENDING, "excusesExpireAt": FLOOR.isoformat()} + )[0] + ) + self.assertFalse( + self.unchanged( + [], report={k: v for k, v in PENDING.items() if k != "excusesExpireAt"} + )[0] + ) + later = (STALE_NOW + timedelta(days=1)).isoformat() + self.assertTrue( + self.unchanged([], report={**PENDING, "excusesExpireAt": later})[0] + ) self.assertFalse( self.unchanged([], now=FLOOR + pm.DISPATCH_DEADLINE + timedelta(seconds=1))[ 0 diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_quarantine.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_quarantine.py index 04efd65b96..743e934c7c 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_quarantine.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_quarantine.py @@ -275,6 +275,29 @@ def test_an_open_quarantine_takes_an_out_of_set_sighting(self) -> None: (post,) = self.posts() self.assertIn(f"body={pm.QUARANTINE_NOTE}{SIGHTING}", post) + def test_the_answer_says_when_its_quarantine_excuse_ends(self) -> None: + # The lane room keeps an answer only until then (issue #9577): the + # issue's creation plus the default 7 quarantine days. + self.quarantine_issue(FLAKY, "2026-10-10T09:00:00Z") + self.red_dispatch(bash_log((FLAKY, False))) + report = self.check().report() + self.assertEqual(report["failures"], []) + self.assertEqual(report["excusesExpireAt"], "2026-10-17T09:00:00+00:00") + # A baseline failure excused by a live quarantine ends the same way. + self.gh.issues.clear() + self.quarantine_issue(FLAKY, "2026-10-10T10:00:00Z") + self.red_baseline() + report = self.check().report() + self.assertEqual(report["failures"], []) + self.assertEqual(report["excusesExpireAt"], "2026-10-17T10:00:00+00:00") + + def test_an_answer_nothing_excuses_has_no_excuse_end(self) -> None: + # A suite with no quarantine issue yet is excused with no end. + self.red_dispatch(bash_log((FLAKY, False))) + report = self.check().report() + self.assertEqual(report["failures"], []) + self.assertIsNone(report["excusesExpireAt"]) + def test_the_selector_scripts_are_gate_suites(self) -> None: for suite in ( "scripts/plan-test-lanes.test.sh", From 19379316873252cb2b2729ca96e4d58cc940f042 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 02:10:27 -0400 Subject: [PATCH 4/5] fix(pr-merge): report a room hold's pull requests by outcome and narrow the clock margin to skew A pull request whose disarm failed, or that report-only left armed, was listed as left unarmed. The room hold now records each outcome: left unarmed (never armed or disarmed) and report-only in the warning, and still armed after a failed disarm in an error-level line. The SINCE_CLOCK_MARGIN drops from 5 minutes to 60 seconds: computedAt is stamped before the reads it covers, so the margin covers clock skew only. Refs: #9577 Co-Authored-By: Claude Opus 5.5 --- .../fix-lane-room-recompute-9577a1c3.md | 2 +- .../fix-lane-room-recompute-9577a1c3.md | 2 +- .../scripts/pr-merge/enable-auto-merge.mjs | 57 +++++++++++++------ .../pr-merge/enable-auto-merge.test.mjs | 33 +++++++++++ docs/conventions/pr-pipeline/README.md | 6 +- .../scripts/babysit_merge_policy.py | 7 ++- .../scripts/tests/test_babysit_lane_room.py | 11 ++++ 7 files changed, 95 insertions(+), 23 deletions(-) diff --git a/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md b/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md index 3540b02f11..f271ab64ae 100644 --- a/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md +++ b/.changes/pr-pipeline/fix-lane-room-recompute-9577a1c3.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- A pending lane merge no longer makes `enable-auto-merge` disarm every armed lanes App pull request. The post-merge step takes about as long as the gate's two-minute reuse age, so the lane's room read the report as too old. It now keeps the report while nothing it was read from has moved, and otherwise checks again as the gate does, with the lane records the last run kept, so that check reads them without an artifact or run read each. When the room is held, by a refusal or because it cannot be read, the run raises one warning annotation and one step-summary line naming the reason and each lanes App pull request it left unarmed. It posts no comment on the pull requests. +- A pending lane merge no longer makes `enable-auto-merge` disarm every armed lanes App pull request. The post-merge step takes about as long as the gate's two-minute reuse age, so the lane's room read the report as too old. It now keeps the report while nothing it was read from has moved, and otherwise checks again as the gate does, with the lane records the last run kept, so that check reads them without an artifact or run read each. When the room is held, by a refusal or because it cannot be read, the run raises one warning annotation and step-summary line naming the reason and each lanes App pull request it left unarmed or, in report-only mode, that a disarm would have turned off. A pull request still armed because its disarm failed gets an error annotation and summary line instead. It posts no comment on the pull requests. diff --git a/.changes/source-control/fix-lane-room-recompute-9577a1c3.md b/.changes/source-control/fix-lane-room-recompute-9577a1c3.md index f777cb0998..c4f7f5ad94 100644 --- a/.changes/source-control/fix-lane-room-recompute-9577a1c3.md +++ b/.changes/source-control/fix-lane-room-recompute-9577a1c3.md @@ -4,4 +4,4 @@ bump: patch ### Fixed -- `babysit_verify_evidence.py lane-room` no longer reports the lane's room unreadable when the run's post-merge answer is past the gate's two-minute reuse age only because a lane merge is still unverified (`unverified-stale`). It keeps the answer when nothing it was read from has moved: main's head, its config commit, the demotion records, and the unverified merges' full and canary runs (`babysit_postmerge.pending_runs_unchanged`, one run listing: none reached a conclusion since, none is past the dispatch deadline, and no quarantine that excused a failing suite has expired). Otherwise it runs the post-merge check again as the gate does (`babysit_merge_policy.lane_room` with the gate's `post_merge`), so a failure found since still refuses. A check that fails or cannot be read leaves the room unreadable, and every other refusal reads as before. `babysit_postmerge.gate_reader` is the one post-merge reader the gate and the room share. The post-merge report now carries `excusesExpireAt`, when the earliest quarantine that excused a failing suite expires. +- `babysit_verify_evidence.py lane-room` no longer reports the lane's room unreadable when the run's post-merge answer is past the gate's two-minute reuse age only because a lane merge is still unverified (`unverified-stale`). It keeps the answer when nothing it was read from has moved: main's head, its config commit, the demotion records, and the unverified merges' full and canary runs (`babysit_postmerge.pending_runs_unchanged`, one run listing: none reached a conclusion since, none is past the dispatch deadline, and no quarantine that excused a failing suite has expired). Otherwise it runs the post-merge check again as the gate does (`babysit_merge_policy.lane_room` with the gate's `post_merge`), so a failure found since still refuses. A check that fails or cannot be read leaves the room unreadable, and every other refusal reads as before. `babysit_postmerge.gate_reader` is the one post-merge reader the gate and the room share. The post-merge report now carries `excusesExpireAt`, when the earliest quarantine that excused a failing suite expires. `SINCE_CLOCK_MARGIN` is now 60 seconds, which covers clock skew only, because `computedAt` is stamped before the reads it covers. It applies to the demotion re-check and to the run check. diff --git a/.github/scripts/pr-merge/enable-auto-merge.mjs b/.github/scripts/pr-merge/enable-auto-merge.mjs index 679dd2b978..49b873fae2 100755 --- a/.github/scripts/pr-merge/enable-auto-merge.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.mjs @@ -55,9 +55,11 @@ // 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 -// one step-summary line naming why and each lane pull request it left -// unarmed. +// 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 +// 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. // - pr-merge's gate would not hold it on its risk score: `evidence // --lane-reviews --rung ` runs the gate's scorer // (babysit_risk.score, holding) over the inputs the gate builds, the @@ -638,8 +640,11 @@ export async function main({ env = process.env, github, verify, room, sleep = (m } return inFlight >= lane.cap ? { disarm: false, reason: `merge.max-in-flight is ${lane.cap} and ${inFlight} merges are in flight` } : null; }; - // The lane pull requests the room's refusal or unread room left unarmed. - const roomHeld = []; + // Each lane pull request the room's refusal or unread room holds, by + // what became of it: unarmed (never armed, or disarmed), armed (its + // disarm failed, so the hold is not enforced) or report-only (a disarm + // would have run). + const roomHeld = new Map(); let failed = 0; const minutes = riskEscalationMinutes(env.RESOLVED_CONFIG ?? ""); // A reader run's per pull request refusal (a hold path or cost hold, or @@ -690,7 +695,7 @@ export async function main({ env = process.env, github, verify, room, sleep = (m // Checked before the reads, so a held lane spends none. const held = ["read", "verify", "arm"].includes(action) ? roomHold(pull) : null; if (held?.disarm) { - roomHeld.push(pull.number); + roomHeld.set(pull.number, pull.autoMergeRequest != null ? "armed" : "unarmed"); } if (held !== null) { ({ action, reason } = { action: held.disarm && pull.autoMergeRequest != null ? "disarm" : "none", reason: held.reason }); @@ -744,6 +749,9 @@ export async function main({ env = process.env, github, verify, room, sleep = (m continue; } if (!write) { + if (roomHeld.get(pull.number) === "armed") { + roomHeld.set(pull.number, "report-only"); + } console.log(`enable-auto-merge: #${pull.number} would ${action} (report-only): ${escapeCommand(reason)}`); await note(pull, detail, { clears, reason }); continue; @@ -765,6 +773,9 @@ export async function main({ env = process.env, github, verify, room, sleep = (m console.log(`enable-auto-merge: #${pull.number} armed at ${pull.headRefOid}`); } else { await github("POST", "/graphql", { query: DISARM_MUTATION, variables: { id: pull.id } }); + if (roomHeld.has(pull.number)) { + roomHeld.set(pull.number, "unarmed"); + } console.log(`enable-auto-merge: #${pull.number} disarmed: ${escapeCommand(reason)}`); } } catch (error) { @@ -777,17 +788,29 @@ export async function main({ env = process.env, github, verify, room, sleep = (m await note(pull, detail, { clears, reason }); } } - // One annotation and one summary line per run, not a comment per pull - // request: the room is read again every scheduled run. - if (roomHeld.length > 0) { - const text = escapeCommand(`${laneHeld}; left unarmed: ${roomHeld.map((number) => `#${number}`).join(", ")}`); - console.log(`::warning title=enable-auto-merge::${text}`); - if (env.GITHUB_STEP_SUMMARY) { - try { - appendFileSync(env.GITHUB_STEP_SUMMARY, `enable-auto-merge: ${text}\n`); - } catch (error) { - console.log(`enable-auto-merge: the step summary was not written: ${describe(error)}`); - } + // One warning (and an error while a disarm failed) and their summary + // lines per run, not a comment per pull request: the room is read again + // every scheduled run. + const held = (state) => [...roomHeld].filter(([, outcome]) => outcome === state).map(([number]) => `#${number}`).join(", "); + const lines = []; + const warned = [ + ["left unarmed", held("unarmed")], + ["report-only, a disarm would have run", held("report-only")], + ].filter(([, numbers]) => numbers); + if (warned.length > 0) { + lines.push(["warning", escapeCommand(`${laneHeld}; ${warned.map(([what, numbers]) => `${what}: ${numbers}`).join("; ")}`)]); + } + if (held("armed")) { + lines.push(["error", escapeCommand(`${laneHeld}; still armed, its disarm failed, so the hold is not enforced: ${held("armed")}`)]); + } + for (const [level, text] of lines) { + console.log(`::${level} title=enable-auto-merge::${text}`); + } + if (lines.length > 0 && env.GITHUB_STEP_SUMMARY) { + try { + appendFileSync(env.GITHUB_STEP_SUMMARY, lines.map(([, text]) => `enable-auto-merge: ${text}\n`).join("")); + } catch (error) { + console.log(`enable-auto-merge: the step summary was not written: ${describe(error)}`); } } return failed === 0 ? 0 : 1; diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index cc2fc1cd41..e02a724eca 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -1146,6 +1146,39 @@ test("a held room raises one warning and one step-summary line naming why and ea assert.equal(readFileSync(summary, "utf8"), ""); }); +// A pull request whose disarm failed, or that report-only left armed, is +// still armed: naming it "left unarmed" would say the hold was enforced when +// it was not (review thread on #9602). +test("a held room names a pull request whose disarm failed as still armed, at error level", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const summary = join(mkdtempSync(join(tmpdir(), "enable-auto-merge-summary-")), "summary.md"); + writeFileSync(summary, ""); + const lane = { ...ROOM, refusal: "rung-off:demotion" }; + const { lines, code } = await run([pull(1), pull(2, { autoMergeRequest: armed })], { GITHUB_STEP_SUMMARY: summary }, { disarmError: new Error("POST /graphql failed with status 502") }, lane); + assert.equal(code, 1); + const why = "pr-merge's gate holds its lane off (rung-off:demotion)"; + assert.deepEqual( + lines.filter((line) => /^::(warning|error) title=enable-auto-merge::pr-merge/.test(line)), + [`::warning title=enable-auto-merge::${why}; left unarmed: #1`, `::error title=enable-auto-merge::${why}; still armed, its disarm failed, so the hold is not enforced: #2`], + ); + assert.equal(readFileSync(summary, "utf8"), `enable-auto-merge: ${why}; left unarmed: #1\nenable-auto-merge: ${why}; still armed, its disarm failed, so the hold is not enforced: #2\n`); +}); + +test("a held room in report-only names the armed pull request a disarm would have turned off, not as unarmed", async () => { + const armed = { enabledAt: "2026-10-23T10:00:00Z" }; + const summary = join(mkdtempSync(join(tmpdir(), "enable-auto-merge-summary-")), "summary.md"); + writeFileSync(summary, ""); + const lane = { ...ROOM, unreadable: "post-merge answer missing or unreadable" }; + const { lines, writes } = await run([pull(1), pull(2, { autoMergeRequest: armed })], { GITHUB_STEP_SUMMARY: summary, REPORT_ONLY: "true" }, {}, lane); + assert.deepEqual(writes, []); + const expected = "the lane's room could not be read (post-merge answer missing or unreadable); left unarmed: #1; report-only, a disarm would have run: #2"; + assert.deepEqual( + lines.filter((line) => line.startsWith("::warning") || line.startsWith("::error")), + [`::warning title=enable-auto-merge::${expected}`], + ); + assert.equal(readFileSync(summary, "utf8"), `enable-auto-merge: ${expected}\n`); +}); + // An event run reads no room, so it never disarms on one. test("an event run leaves an armed lane pull request armed whatever the room", async () => { const armed = { enabledAt: "2026-10-23T10:00:00Z" }; diff --git a/docs/conventions/pr-pipeline/README.md b/docs/conventions/pr-pipeline/README.md index 99f8857aa9..183d9ee952 100644 --- a/docs/conventions/pr-pipeline/README.md +++ b/docs/conventions/pr-pipeline/README.md @@ -1123,8 +1123,10 @@ with the lane records the last run kept, holding only when that check cannot be nothing is armed and every armed lanes App pull request is disarmed; at `merge.max-in-flight`, counting the queue's lane entries, the lane's unverified merges and armed pull requests the queue has not taken, nothing more is armed; a room that cannot be read (no report, a failed read) holds -as a refusal does. A held room raises one warning annotation and one step-summary line naming why -and each lanes App pull request it left unarmed, and no comment on the pull requests. +as a refusal does. A held room raises one warning annotation and step-summary line naming why and +each lanes App pull request it left unarmed or, report-only, that a disarm would have turned off, +and one error annotation and summary line naming each one still armed because its disarm failed; +it posts no comment on the pull requests. Only the scheduled run and a hand-started one read them and arm, last, for a pull request every other rule would arm or keep armed, with the gate's saved listing, head-ref map and run cache; an event run reads no link or verdict, never arms, and 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 bd1a1bfdd7..a0a3c9036f 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 @@ -3868,8 +3868,11 @@ class PostMergeGate: "config-unreadable", } ) -# Runner clocks may run ahead of GitHub's; `since` starts this much earlier. -SINCE_CLOCK_MARGIN = timedelta(minutes=5) +# Clock skew alone: a runner clock may run ahead of GitHub's, so `since` +# starts this much earlier. `computed_at` is stamped before the reads it +# covers (pr-merge run 40060427179: its step started 03:58:09.10Z and +# stamped 03:58:09.27Z), so no read time needs covering. +SINCE_CLOCK_MARGIN = timedelta(seconds=60) @dataclass(frozen=True) 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 b6fcd9386d..de35e5689c 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 @@ -580,6 +580,17 @@ def test_runs_still_running_or_finished_before_the_answer_hold(self) -> None: self.assertIn("branch=main", calls[0][1]) self.assertIn("created=%3E%3D2026-10-01T00%3A00%3A00Z", calls[0][1]) + def test_the_clock_margin_is_sixty_seconds_of_skew(self) -> None: + # computedAt is stamped before the runs are read, so the margin covers + # only clock skew: 60 s (owner decision on #9602). + def finished(before: timedelta) -> dict[str, object]: + return run( + "pr-require-checks.yml", status="completed", updated=FLOOR - before + ) + + self.assertFalse(self.unchanged([finished(timedelta(seconds=59))])[0]) + self.assertTrue(self.unchanged([finished(timedelta(seconds=61))])[0]) + def test_a_conclusion_since_a_partial_listing_or_the_deadline_moves_it( self, ) -> None: From fe4338c89908d4a902cb692cdde7ea024f6eb748 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sat, 24 Oct 2026 03:13:56 -0400 Subject: [PATCH 5/5] fix(pr-merge): drop tracker references and run citations from the room's comments The comment-hygiene check rejects issue references in code comments. The comments keep their reasoning; the issue numbers, the review thread and the run timings stay in the pull request. Refs: #9577 Co-Authored-By: Claude Opus 5.5 --- .github/scripts/pr-merge/enable-auto-merge.test.mjs | 6 +++--- .github/scripts/pr-merge/workflow.test.mjs | 4 ++-- .../skills/babysit-prs/scripts/babysit_merge_policy.py | 3 +-- .../babysit-prs/scripts/tests/test_babysit_lane_room.py | 8 ++++---- .../scripts/tests/test_babysit_postmerge_quarantine.py | 2 +- 5 files changed, 11 insertions(+), 12 deletions(-) diff --git a/.github/scripts/pr-merge/enable-auto-merge.test.mjs b/.github/scripts/pr-merge/enable-auto-merge.test.mjs index e02a724eca..2bf6d42e46 100644 --- a/.github/scripts/pr-merge/enable-auto-merge.test.mjs +++ b/.github/scripts/pr-merge/enable-auto-merge.test.mjs @@ -1114,8 +1114,8 @@ test("a room that cannot be read arms nothing and disarms every armed lane pull assert.deepEqual(writes(), [{ mutation: "disarm", id: "PR_2" }]); }); -// Issue #9577's scope addition: a held room logged only to the console, so -// nothing on the run said why nothing armed. One annotation and one summary +// A held room that logs only to the console leaves nothing on the run +// saying why nothing armed. One annotation and one summary // line per run, naming the reason and every lane pull request left unarmed. test("a held room raises one warning and one step-summary line naming why and each pull request left unarmed", async () => { const armed = { enabledAt: "2026-10-23T10:00:00Z" }; @@ -1148,7 +1148,7 @@ test("a held room raises one warning and one step-summary line naming why and ea // A pull request whose disarm failed, or that report-only left armed, is // still armed: naming it "left unarmed" would say the hold was enforced when -// it was not (review thread on #9602). +// it was not. test("a held room names a pull request whose disarm failed as still armed, at error level", async () => { const armed = { enabledAt: "2026-10-23T10:00:00Z" }; const summary = join(mkdtempSync(join(tmpdir(), "enable-auto-merge-summary-")), "summary.md"); diff --git a/.github/scripts/pr-merge/workflow.test.mjs b/.github/scripts/pr-merge/workflow.test.mjs index 485effe3e3..191a58a8aa 100644 --- a/.github/scripts/pr-merge/workflow.test.mjs +++ b/.github/scripts/pr-merge/workflow.test.mjs @@ -220,8 +220,8 @@ test("enable-auto-merge reads the lane's room from this run's post-merge-dispatc // A room that checks the post-merge answer again reads the lane records the // last run kept, as post-merge-dispatch's check does, so the check answers -// them with no artifact or run read each (465 calls and 4m20s without them, -// past the room's VERIFY_TIMEOUT_MS). +// them with no artifact or run read each; without them the check can run +// past the room's VERIFY_TIMEOUT_MS. test("enable-auto-merge hands the room's post-merge check the lane records the last run kept", () => { const arm = job("enable-auto-merge"); const records = /- name: Read the lane records the last run kept\n([\s\S]*?)(?=\n {6}- name: )/.exec(arm)?.[1]; 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 a0a3c9036f..afb1a48fcc 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 @@ -3870,8 +3870,7 @@ class PostMergeGate: ) # Clock skew alone: a runner clock may run ahead of GitHub's, so `since` # starts this much earlier. `computed_at` is stamped before the reads it -# covers (pr-merge run 40060427179: its step started 03:58:09.10Z and -# stamped 03:58:09.27Z), so no read time needs covering. +# covers, so no read time needs covering. SINCE_CLOCK_MARGIN = timedelta(seconds=60) 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 de35e5689c..1e33fff1fc 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 @@ -35,7 +35,7 @@ NEW_MAIN = "e" * 40 NEW_FLOOR = FLOOR + timedelta(minutes=1) # A passing answer with lane merge #5 still unverified, read three minutes on: -# past the two minutes the gate reuses it (issue #9577's 2:01 read). +# past the two minutes the gate reuses it. PENDING = { "readable": True, "ok": True, @@ -303,8 +303,8 @@ def refusal(repo: str, answer: object, **kwargs: object) -> str: def test_a_pending_merge_past_two_minutes_is_read_again_and_counted( self, ) -> None: - # Issue #9577: the post-merge step takes about 1m41s and the room is - # read at about 2:01, so every pending lane merge left it unreadable. + # The post-merge step takes nearly the two minutes the gate reuses an + # answer for, so a pending lane merge must not leave the room unreadable. verified = { "readable": True, "ok": True, @@ -582,7 +582,7 @@ def test_runs_still_running_or_finished_before_the_answer_hold(self) -> None: def test_the_clock_margin_is_sixty_seconds_of_skew(self) -> None: # computedAt is stamped before the runs are read, so the margin covers - # only clock skew: 60 s (owner decision on #9602). + # only clock skew: 60 s. def finished(before: timedelta) -> dict[str, object]: return run( "pr-require-checks.yml", status="completed", updated=FLOOR - before diff --git a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_quarantine.py b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_quarantine.py index 743e934c7c..3f8e2ca8cb 100644 --- a/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_quarantine.py +++ b/plugins/source-control/skills/babysit-prs/scripts/tests/test_babysit_postmerge_quarantine.py @@ -276,7 +276,7 @@ def test_an_open_quarantine_takes_an_out_of_set_sighting(self) -> None: self.assertIn(f"body={pm.QUARANTINE_NOTE}{SIGHTING}", post) def test_the_answer_says_when_its_quarantine_excuse_ends(self) -> None: - # The lane room keeps an answer only until then (issue #9577): the + # The lane room keeps an answer only until then: the quarantine # issue's creation plus the default 7 quarantine days. self.quarantine_issue(FLAKY, "2026-10-10T09:00:00Z") self.red_dispatch(bash_log((FLAKY, False)))