Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
bump: patch
---

### Changed

- `enable-auto-merge` no longer spends most of its lane-room time limit on a full post-merge check when the run's report was refused only because an issue the lanes App opened was edited since (`demotion-changed`). The room reads the demotion records alone and keeps the report when they are unchanged, so a slow API window no longer risks running past the time limit and disarming every armed lanes App pull request.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
bump: patch
---

### Changed

- `babysit_verify_evidence.py lane-room` no longer runs the full post-merge check when the only reason the gate would not reuse the run's answer is `demotion-changed`, which any issue the lanes App opens and triage then edits sets. It reads the demotion records alone (`read_demotion`) and keeps the answer when they read as the answer recorded them; a demotion that turned active or moved still runs the full check, so a real demotion still holds the rung off. `shared_refusal` now checks the demotion last, so `demotion-changed` is reported only when nothing else refuses: a lane merge the answer does not count reports `new-merge:#<n>` first.
8 changes: 5 additions & 3 deletions .github/scripts/pr-merge/enable-auto-merge.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,11 @@
// 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 the
// gate would not reuse is checked again as the gate does; one past its
// reuse age only for a lane merge still unverified is first kept while
// nothing it was read from has moved. A held room raises one warning annotation and step-summary line
// gate would not reuse, for any reason, is checked again as the gate does;
// one past its reuse age only for a lane merge still unverified is first
// kept while nothing it was read from has moved, and one refused only for
// a demotion record filed or changed since is first kept when the
// demotion records alone read as they were. 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.
Expand Down
6 changes: 3 additions & 3 deletions docs/conventions/pr-pipeline/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1135,9 +1135,9 @@ count; an unread count leaves it unarmed with no comment, and the next run reads
armed one is not disarmed on the count. Each such run also
waits for the same run's `post-merge-dispatch` job and reads the lane's room from the post-merge
report that job keeps as an artifact (`babysit_verify_evidence.py lane-room`; with no report it
holds as a refusal does; one the gate would not reuse (`shared_refusal`: too old, a demotion record
changed since, or a lane merge landed since that it does not count) is checked again as the gate
does; one past the gate's
holds as a refusal does; one the gate would not reuse, for any `shared_refusal`, is checked again
as the gate does; one refused only because a demotion record was filed or changed since is first
kept when the demotion records alone read as they were; one past the gate's
two-minute reuse age only because a lane merge is still unverified is kept while main's head, its
config commit, the demotion records and the unverified merges' full and canary runs are as they
were (no run reached a conclusion since, no merge is past the six-hour dispatch deadline, and no
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4494,11 +4494,13 @@ def shared_refusal(
demotion), be no older than `SHARED_MAX_AGE` (`SHARED_UNVERIFIED_MAX_AGE`
while it passes with a merge unverified), have no demotion record filed
or changed since (`_demotion_changed`), and judge every merge the
post-merge check would judge now. The binding comes before the age, so a
candidate bound elsewhere reports `SHARED_BINDING_REFUSALS` and does not
replace the run's answer. The floor is compared under the shared answer's
demotion, which may be out of date; that changes only the reason and the
write-back, since any refusal reads afresh.
post-merge check would judge now. The demotion comes last, so
`demotion-changed` means nothing else refused (`lane_room` keeps such an
answer when the demotion reads the same). The binding comes before the
age, so a candidate bound elsewhere reports `SHARED_BINDING_REFUSALS` and
does not replace the run's answer. The floor is compared under the
shared answer's demotion, which may be out of date; that changes only the
reason and the write-back, since any refusal reads afresh.

The check judges `branch` from the floor, never the base, so the answer
holds for any base with that floor. A newer default-branch head changes
Expand Down Expand Up @@ -4535,33 +4537,32 @@ def shared_refusal(
refusal = _shared_age_refusal(shared, age)
if refusal:
return refusal
if not shared.demotion.active and _demotion_changed(owner, name, shared, identity):
return "demotion-changed"
status, payload = _get(f"repos/{owner}/{name}/commits/{quote(branch, safe='')}")
head = payload.get("sha") if status == 200 and is_json_object(payload) else None
if head == shared.main_sha:
return None
counted = {
json_object(m).get("number")
for m in json_array(shared.gate.report.get("autoMerges"))
}
try:
merges = _merged_since(owner, name, branch, shared.floor)
if merges is None:
return "merged-history-truncated"
for pr in merges:
# Treating no actor as human counts a superset of the check's
# merges, so no merge it would judge is missed.
merge = derive_auto_merge(
pr,
lambda actor: False,
identity,
lane_merges_only=shared.lane_merges_only,
)
if isinstance(merge, AutoMerge) and merge.number not in counted:
return f"new-merge:#{merge.number}"
except PolicyReadError:
return "merged-unreadable"
if head != shared.main_sha:
counted = {
json_object(m).get("number")
for m in json_array(shared.gate.report.get("autoMerges"))
}
try:
merges = _merged_since(owner, name, branch, shared.floor)
if merges is None:
return "merged-history-truncated"
for pr in merges:
# Treating no actor as human counts a superset of the check's
# merges, so no merge it would judge is missed.
merge = derive_auto_merge(
pr,
lambda actor: False,
identity,
lane_merges_only=shared.lane_merges_only,
)
if isinstance(merge, AutoMerge) and merge.number not in counted:
return f"new-merge:#{merge.number}"
except PolicyReadError:
return "merged-unreadable"
if not shared.demotion.active and _demotion_changed(owner, name, shared, identity):
return "demotion-changed"
return None


Expand Down Expand Up @@ -5094,12 +5095,14 @@ def lane_room(
`assess` applies to every lane candidate, read once at the default branch
head, in its order. `shared` is this run's `babysit_postmerge.py` answer
(`babysit_postmerge.read_shared`), as the gate's `--postmerge-result`.
`post_merge` is `assess`'s: an answer the gate would not reuse
(`shared_refusal`) is read again as the gate reads it
`post_merge` is `assess`'s: an answer the gate would not reuse, for any
`shared_refusal`, is read again as the gate reads it
(`_recompute_shared`, with the room's self logins, none), and a readable
one stands in for `shared`; one refused only as `unverified-stale` is
first kept when nothing it was read from has moved (`_still_current`,
with `runs_unchanged(shared, branch)` for its unverified merges' runs).
one stands in for `shared`. Two refusals are first kept: one refused only
as `unverified-stale` when nothing it was read from has moved
(`_still_current`, with `runs_unchanged(shared, branch)` for its
unverified merges' runs), and one refused only as `demotion-changed`
when `read_demotion` alone reads the shared demotion again.

- A config naming another authority stops (`enabled_authority`).
- A post-merge refusal stops (`post-merge:<refusal>`).
Expand All @@ -5111,9 +5114,8 @@ def lane_room(

Any read failure, a missing `shared` and an in-flight count that could
not be read included, is `unreadable`. So is a `shared` the gate would
not reuse (too old, a demotion record filed or changed since, a lane
merge landed since that it does not count) when there is no `post_merge`
to read it again, or that read fails or is not readable."""
not reuse (any `shared_refusal`) when there is no `post_merge` to read it
again, or that read fails or is not readable."""
if shared is None:
return LaneRoom(unreadable="post-merge answer missing or unreadable")
try:
Expand Down Expand Up @@ -5143,13 +5145,23 @@ def lane_room(
)
if unusable and post_merge is not None:
# As the gate, any refusal reads afresh; an unverified-stale answer
# is first kept when nothing it was read from moved.
# is first kept when nothing it was read from moved, a
# demotion-changed one when the demotion reads the same (an issue
# the lane opened that triage edited, most runs).
try:
current = (
unusable == "unverified-stale"
and runs_unchanged is not None
and _still_current(repo, default, identity, shared, runs_unchanged)
)
if unusable == "demotion-changed":
current = shared.demotion == read_demotion(
repo,
agent_identity=identity,
self_logins=frozenset(),
permission=permission_reader(repo),
)
else:
current = (
unusable == "unverified-stale"
and runs_unchanged is not None
and _still_current(repo, default, identity, shared, runs_unchanged)
)
except (RuntimeError, ValueError, OSError):
current = False
if not current:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1992,9 +1992,10 @@ 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
the gate would not reuse is checked again; one refused only as
`unverified-stale` is first kept while nothing it was read from has
moved."""
the gate would not reuse, for any `shared_refusal`, is checked again; one
refused only as `unverified-stale` is first kept while nothing it was
read from has moved, and one refused only as `demotion-changed` when the
demotion reads the same."""
parser = argparse.ArgumentParser(
prog="babysit_verify_evidence.py", allow_abbrev=False
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import pathlib
import sys
import unittest
from dataclasses import replace
from datetime import UTC, datetime, timedelta
from unittest import mock

Expand Down Expand Up @@ -133,6 +134,9 @@ def room(
fresh_demotion: policy.Demotion | None = None,
) -> policy.LaneRoom:
seen: dict[str, object] = {}
reads = mock.Mock(
return_value=fresh_demotion or policy.Demotion(False, None, ())
)

def in_flight(repo: str, **kwargs: object) -> policy.InFlight:
seen.update(kwargs)
Expand Down Expand Up @@ -168,11 +172,7 @@ def in_flight(repo: str, **kwargs: object) -> policy.InFlight:
return_value=("main", head),
),
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, "read_demotion", reads),
mock.patch.object(policy, "merged_since_floor", return_value=fresh_floor),
):
kwargs = {"post_merge": post_merge} if post_merge is not None else {}
Expand All @@ -185,7 +185,7 @@ def in_flight(repo: str, **kwargs: object) -> policy.InFlight:
now=now,
**kwargs, # type: ignore[arg-type]
)
self.seen = seen
self.seen, self.reads = seen, reads
return result

def test_a_clear_lane_counts_its_merges_in_flight_against_the_cap(self) -> None:
Expand Down Expand Up @@ -239,17 +239,16 @@ def test_at_the_cap_it_reports_the_cap_and_refuses_nothing(self) -> None:
(room.refusal, room.unreadable, room.count, room.cap), (None, None, 5, 5)
)

# The gate's reuse refusals other than unverified-stale (shared_refusal):
# past SHARED_MAX_AGE (a re-run job reads the run's first report), from
# the future, and a demotion record filed or changed since, inside the
# two minutes the gate reuses a pending answer for.
# Reuse refusals the room reads afresh (shared_refusal): past
# SHARED_MAX_AGE (a re-run job reads the run's first report) and from the
# future.
UNUSABLE = {
"stale": dict(now=FLOOR + policy.SHARED_MAX_AGE + timedelta(seconds=1)),
"future": dict(now=FLOOR - timedelta(seconds=1)),
"demotion changed": dict(
demotion_changed=True, now=FLOOR + timedelta(minutes=1)
),
}
# A demotion record filed or changed since, inside the two minutes the
# gate reuses a pending answer for.
DEMOTION_CHANGED = dict(demotion_changed=True, now=FLOOR + timedelta(minutes=1))

def test_an_answer_the_gate_would_not_reuse_is_read_again(self) -> None:
# The gate reads afresh on any refusal (assess); so does the room,
Expand Down Expand Up @@ -287,34 +286,89 @@ def test_an_unusable_answer_is_never_kept_on_the_cheap_path(self) -> None:
self.assertEqual(len(recheck.asked), 1)
runs.assert_not_called()

def test_a_demotion_read_again_holds_the_room_off(self) -> None:
def test_a_no_op_demotion_edit_keeps_the_answer(self) -> None:
# Triage editing an issue the lane opened refuses the answer as
# `demotion-changed`; a demotion read alone that finds it unchanged
# keeps it, so the room does not pay the full post-merge check.
recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None))
answer = shared()
room = self.room(
answer,
post_merge=recheck.post_merge,
fresh_demotion=policy.Demotion(True, "record-open", (8,)),
**self.UNUSABLE["demotion changed"], # type: ignore[arg-type]
**self.DEMOTION_CHANGED, # type: ignore[arg-type]
)
self.assertEqual(room.refusal, "rung-off:demotion")
self.assertEqual(
(room.refusal, room.unreadable, room.count, room.cap), (None, None, 2, 5)
)
self.assertEqual(recheck.asked, [], "no full post-merge check")
self.reads.assert_called_once()
self.assertEqual(self.reads.call_args.kwargs["self_logins"], frozenset())
# 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_a_demotion_that_moved_reads_the_answer_again(self) -> None:
# Active, the rung is off; inactive with other records or another
# clear, the floor may differ. Either way the full read runs.
cases = {
"active": (
policy.Demotion(True, "record-open", (8,)),
"rung-off:demotion",
),
"another record": (policy.Demotion(False, None, (9,)), None),
}
for name, (demotion, refusal) in cases.items():
with self.subTest(name):
recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None))
room = self.room(
post_merge=recheck.post_merge,
fresh_demotion=demotion,
**self.DEMOTION_CHANGED, # type: ignore[arg-type]
)
self.assertEqual((room.refusal, room.unreadable), (refusal, None))
self.assertEqual(recheck.asked, [(NEW_FLOOR, IDENTITY)])

def test_an_answer_bound_to_other_self_logins_is_read_with_none(self) -> None:
# The room binds no self logins, so a fresh read uses none and the
# in-flight count excludes none: not the answer's own.
answer = replace(shared(), self_logins=frozenset({"lane-self[bot]"}))
recheck = Recheck(policy.PostMergeGate(None, {"readable": True}, None))
room = self.room(answer, post_merge=recheck.post_merge)
self.assertEqual((room.refusal, room.unreadable), (None, None))
self.assertEqual(recheck.asked, [(NEW_FLOOR, IDENTITY)])
self.assertEqual(self.reads.call_args.kwargs["self_logins"], frozenset())
self.assertEqual(self.seen["self_logins"], frozenset())

def test_an_unusable_answer_with_no_fresh_read_is_unreadable(self) -> None:
cases = {
"no check": None,
"check raises": Recheck(error=RuntimeError("runs 502")).post_merge,
"check unreadable": Recheck(
policy.PostMergeGate("runs-unreadable", {"readable": False}, None)
).post_merge,
"no check": (None, self.UNUSABLE["stale"], "stale"),
"no check, demotion changed": (
None,
self.DEMOTION_CHANGED,
"demotion-changed",
),
"check raises": (
Recheck(error=RuntimeError("runs 502")).post_merge,
self.UNUSABLE["stale"],
"stale",
),
"check unreadable": (
Recheck(
policy.PostMergeGate("runs-unreadable", {"readable": False}, None)
).post_merge,
self.UNUSABLE["stale"],
"stale",
),
}
for name, post_merge in cases.items():
for name, (post_merge, kwargs, reason) in cases.items():
with self.subTest(name):
room = self.room(
post_merge=post_merge,
**self.UNUSABLE["demotion changed"], # type: ignore[arg-type]
**kwargs, # type: ignore[arg-type]
)
self.assertIsNone(room.refusal)
self.assertTrue(
str(room.unreadable).startswith(
"post-merge answer demotion-changed"
),
str(room.unreadable).startswith(f"post-merge answer {reason}"),
room.unreadable,
)
self.assertFalse(self.seen, "no in-flight read")
Expand All @@ -340,6 +394,7 @@ def refusal(repo: str, answer: object, **kwargs: object) -> str:
(room.refusal, room.unreadable),
(None, "post-merge answer new-merge:#7"),
)
self.assertFalse(self.seen, "no in-flight read after an unusable answer")
# Bound as the gate binds a candidate at the default branch head.
self.assertEqual(
{
Expand Down
Loading
Loading