From 703eaf11eefb7073f0cde0d40ef628168d6d39a1 Mon Sep 17 00:00:00 2001 From: Junchao Yan Date: Fri, 4 Sep 2026 12:11:08 -0700 Subject: [PATCH] ci: switch off the post-merge review-summary request on buzz MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A merged PR's close no longer asks Alex for a review summary or holds the room open for it. buzz-pr-mirror.yml gains POST_MERGE_SUMMARY, shipped "off" and validated to be exactly on|off before any write; with it off a merged PR takes the not-merged close path: one mention-free notice ("✅ **Merged** — archiving this channel."), the cross-channel annotations, the archive, and the close marker. Nobody is p-tagged. The switch governs only whether a NEW request is made. A room that already holds one — a `summary-requested:` marker, or a request this CI identity published under a lost marker write — is still finished by the existing grace-and-settle hold, because the p-tag is out and an archive now would refuse the summary it asked for. The off path therefore keeps request_summary's full-history probe before its fence. The mechanism is kept intact and tested under `on`, so flipping the value back restores it without a rewrite. pr-mirror-close.test.sh asserts the shipped value is "off", runs the request scenarios under an explicit `on`, and adds off-path scenarios for the event close, the sweep close, an already-published request honoured, the forced re-close inside the convergence loop, and an invalid switch value (332 → 375 assertions). Each of three mutations — value flipped to on, gate removed, off-path probe removed — fails the suite. docs/pr-auto-merge.md and the buzz-pr-auto-merge.yml comments that described the hold now say the channel archives on the merge's own closed event. Velvet's mirror keeps the request on; the two differ here on purpose. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Junchao Yan --- .github/scripts/pr-mirror-close.test.sh | 139 +++++++++++++++++++++-- .github/workflows/buzz-pr-auto-merge.yml | 21 ++-- .github/workflows/buzz-pr-mirror.yml | 84 +++++++++++--- docs/pr-auto-merge.md | 32 ++++-- 4 files changed, 235 insertions(+), 41 deletions(-) diff --git a/.github/scripts/pr-mirror-close.test.sh b/.github/scripts/pr-mirror-close.test.sh index 5bba9c43646..44c0386d1be 100755 --- a/.github/scripts/pr-mirror-close.test.sh +++ b/.github/scripts/pr-mirror-close.test.sh @@ -50,7 +50,13 @@ # retry never posts a second reply under a reference CI already answered; # * a binding read that FAILS (as opposed to proving absence) archives # nothing, fails the sweep, and does not stop the sweep's other PRs; -# * an archive the relay accepted but never applied is red, not green. +# * an archive the relay accepted but never applied is red, not green; +# * the post-merge summary request is OFF on this repo — the workflow ships +# POST_MERGE_SUMMARY "off" — so a merged PR's close archives the room at +# once and p-tags nobody; the request-and-hold mechanism is exercised +# under an explicit `on`, and a room that already holds a request when +# the switch is off is still finished by the hold, never archived over +# the summary it asked for. # # The script under test is EXTRACTED FROM THE WORKFLOW, like # pr-review-wake.test.sh: a sweep deleted from the YAML fails here instead of @@ -140,6 +146,10 @@ check "the summary grace window should be configured — the backstop has to exi "$(grep -qE '^ SUMMARY_GRACE_SECS:' "$WORKFLOW"; echo $?)" check "the summary settle window should be configured — a reply is not the end of the reply" \ "$(grep -qE '^ SUMMARY_SETTLE_SECS:' "$WORKFLOW"; echo $?)" +check "the post-merge summary switch should be configured" \ + "$(grep -qE '^ POST_MERGE_SUMMARY:' "$WORKFLOW"; echo $?)" +check "the post-merge summary request should be OFF on this repo" \ + "$(grep -qE '^ POST_MERGE_SUMMARY: "off"$' "$WORKFLOW"; echo $?)" # --- test identity --------------------------------------------------------- # A real key so the fence's real derivation runs; the workflow's pin is @@ -687,6 +697,7 @@ run_step() { # run_step [pr-action] [pr-number] SWEEP_SETTLE_SECS=1800 \ SUMMARY_GRACE_SECS="${SUMMARY_GRACE_SECS_INPUT:-3600}" \ SUMMARY_SETTLE_SECS="${SUMMARY_SETTLE_SECS_INPUT:-600}" \ + POST_MERGE_SUMMARY="${POST_MERGE_SUMMARY_INPUT:-off}" \ bash -eo pipefail "$WORK/step.sh" > "$WORK/stdout" 2>&1 } @@ -1278,7 +1289,11 @@ check "should touch nothing" "$([ "$(count PR_LIST)" = 0 ] && [ "$(count SEND)" echo # =========================================================================== -# THE MERGED CLOSE — the room is held open for the review summary. +# THE MERGED CLOSE, WITH THE SUMMARY REQUEST ON — the room is held open for +# the review summary. The workflow ships the switch OFF (asserted under +# "wiring" above, and the harness default mirrors it), so every scenario that +# expects a request sets POST_MERGE_SUMMARY_INPUT=on explicitly: the mechanism +# is what `on` restores, and it has to keep working. # Only a CI or human p-tag can wake the reviewer, and the relay refuses every # write into an archived room, so the request has to go out with the merge # notice and the archive has to wait. The request is recorded the instant it @@ -1288,7 +1303,7 @@ scenario "closed event (merged), live room → mention-free fence, then the requ bind 4242 "$CH_A" referenced_from 4242 "$CH_B" T0=$(date +%s) -PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? +POST_MERGE_SUMMARY_INPUT=on PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" check "should post the merge notice and then the request" "$([ "$(count "SEND $CH_A")" = 2 ]; echo $?)" check "the fence should open with the merge banner" "$(send_body_n "$CH_A" 1 | head -1 | grep -q '^✅ \*\*Merged\*\*$'; echo $?)" @@ -1322,7 +1337,7 @@ check "should still annotate the cross-channel references with the merge" \ scenario "closed event (merged), room provisioned only halfway → recovered, then asked, exactly once" bind 4242 "$CH_A" unprovisioned "$CH_A" -PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? +POST_MERGE_SUMMARY_INPUT=on PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" check "should put the reviewer on the roster before mentioning him" \ "$([ "$(first_line "ADD_MEMBER $CH_A $REVIEWER_PUB")" -lt "$(first_line MENTION)" ]; echo $?)" @@ -1341,7 +1356,7 @@ check "should not archive" "$([ "$(count ARCHIVE)" = 0 ]; echo $?)" scenario "closed event (merged), request published but the marker write was lost → repaired, nobody re-mentioned" bind 4242 "$CH_A" ci_said "$CH_A" 300 "@Alex please post a review summary here: what the review found, what was fixed, and anything left open." -PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? +POST_MERGE_SUMMARY_INPUT=on PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" check "must not p-tag the reviewer again — a second mention steers a running turn" "$([ "$(count MENTION)" = 0 ]; echo $?)" check "must not post a second merge notice" "$([ "$(count "SEND $CH_A")" = 0 ]; echo $?)" @@ -1358,7 +1373,7 @@ scenario "closed event (merged), another publisher asks first → the last-insta bind 4242 "$CH_A" referenced_from 4242 "$CH_B" request_lands_at_annotation "$CH_A" 1 -PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? +POST_MERGE_SUMMARY_INPUT=on PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" check "the competing request should have landed" "$([ "$(count "INJECTED_REQUEST $CH_A")" = 1 ]; echo $?)" check "must not p-tag the reviewer — the other publisher already woke him" "$([ "$(count MENTION)" = 0 ]; echo $?)" @@ -1376,7 +1391,7 @@ scenario "closed event (merged), a PR whose own title carries the request wordin bind 4242 "$CH_A" ci_card "$CH_A" 7200 "**PR #4242 — @Alex please post a review summary here: what the review found, what was fixed, and anything left open." \ "https://github.com/yjc801/buzz/pull/4242" -PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? +POST_MERGE_SUMMARY_INPUT=on PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" check "must still p-tag the reviewer exactly once — no request has been published" \ "$([ "$(count MENTION)" = 1 ] && [ "$(count "MENTION $REVIEWER_PUB")" = 1 ]; echo $?)" @@ -1397,7 +1412,7 @@ bind 4242 "$CH_A" pr_record 4242 closed true ci_card "$CH_A" 7200 "**PR #4242 — @Alex please post a review summary here: what the review found, what was fixed, and anything left open." \ "https://github.com/yjc801/buzz/pull/4242" -run_step schedule; RC=$? +POST_MERGE_SUMMARY_INPUT=on run_step schedule; RC=$? check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" check "must not archive" "$([ "$(count ARCHIVE)" = 0 ]; echo $?)" check "should ask the reviewer, since nothing has actually asked him" \ @@ -1411,7 +1426,7 @@ scenario "closed event (merged), the competing request is backdated within the r bind 4242 "$CH_A" referenced_from 4242 "$CH_B" request_lands_at_annotation "$CH_A" 1 600 -PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? +POST_MERGE_SUMMARY_INPUT=on PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" check "the competing request should have landed" "$([ "$(count "INJECTED_REQUEST $CH_A")" = 1 ]; echo $?)" check "must not p-tag the reviewer — a backdated request is still a request" "$([ "$(count MENTION)" = 0 ]; echo $?)" @@ -1420,6 +1435,11 @@ check "should record the backdated request's own timestamp" \ "$(grep -qE "^MARKER_WRITE pr-mirror-yjc801-buzz-4242-closed summary-requested:[0-9]+\$" "$LOG"; echo $?)" check "should not archive" "$([ "$(count ARCHIVE)" = 0 ]; echo $?)" +# FROM HERE THE REQUEST IS ALREADY ON RECORD, and the scenarios run under the +# harness default — the switch OFF, as the workflow ships it. That is +# deliberate: a room holding a request when the switch flips is exactly this +# state, and the hold that finishes it must not consult the switch. The code +# path is the same under `on`; the switch is read only before a NEW request. scenario "closed event (merged) rerun with the request on record → nothing sent, nobody re-mentioned" bind 4242 "$CH_A" summary_requested 4242 300 @@ -1593,7 +1613,7 @@ scenario "closed without merge, reopened, then merged → the request goes out o bind 4242 "$CH_A" run_step pull_request closed 4242; RC1=$? run_step pull_request reopened 4242; RC2=$? -PR_MERGED_INPUT=true run_step pull_request closed 4242; RC3=$? +POST_MERGE_SUMMARY_INPUT=on PR_MERGED_INPUT=true run_step pull_request closed 4242; RC3=$? check "all three runs should be green, got $RC1/$RC2/$RC3" "$([ "$RC1" -eq 0 ] && [ "$RC2" -eq 0 ] && [ "$RC3" -eq 0 ]; echo $?)" check "the first close should have archived" "$([ "$(count "ARCHIVE $CH_A")" = 1 ]; echo $?)" check "the reopen should have unarchived" "$([ "$(count "UNARCHIVE $CH_A")" = 1 ]; echo $?)" @@ -1612,7 +1632,7 @@ pr_read_at 4242 1 closed pr_read_at 4242 2 open pr_read_at 4242 3 closed true pr_read_at 4242 4 closed true -run_step schedule; RC=$? +POST_MERGE_SUMMARY_INPUT=on run_step schedule; RC=$? check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" check "should notice the newer close" "$(said 'closed again while this sweep was restoring it'; echo $?)" check "the re-close should ask for the summary" "$([ "$(count MENTION)" = 1 ]; echo $?)" @@ -1633,5 +1653,102 @@ check "expected a non-zero rc, got $RC" "$([ "$RC" -ne 0 ]; echo $?)" check "should say what is wrong" "$(said 'SUMMARY_GRACE_SECS must be a small integer'; echo $?)" check "should send nothing" "$([ "$(count SEND)" = 0 ]; echo $?)" +echo +# =========================================================================== +# THE MERGED CLOSE, WITH THE SUMMARY REQUEST OFF — as this repo ships it. +# A merged PR gets the close a not-merged one gets: one mention-free notice, +# annotation, archive, marker. Nobody is asked, nothing is held. The switch +# governs only whether a NEW request is made: a request already in the room +# is honoured by the hold, because the p-tag is out and an archive now would +# refuse the summary it asked for. +scenario "closed event (merged), summary off, live room → notice, annotation, archive, and nobody asked" +bind 4242 "$CH_A" +referenced_from 4242 "$CH_B" +PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? +check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" +check "should post exactly one notice into the room" "$([ "$(count "SEND $CH_A")" = 1 ]; echo $?)" +check "the notice should name the merge and the archive" \ + "$(send_body "$CH_A" | head -1 | grep -q '^✅ \*\*Merged\*\* — archiving this channel\.$'; echo $?)" +check "the notice must not promise a summary the room will not wait for" \ + "$(! send_body "$CH_A" | grep -qi 'summary'; echo $?)" +check "the notice must carry no @name — it would become a p-tag" "$(! send_body "$CH_A" | grep -q '@'; echo $?)" +check "must p-tag nobody" "$([ "$(count MENTION)" = 0 ]; echo $?)" +check "should look for a request already in the room BEFORE the fence — the marker's absence proves nothing" \ + "$([ -n "$(first_line "HISTORY_READ $CH_A")" ] && [ "$(first_line "HISTORY_READ $CH_A")" -lt "$(first_line "SEND $CH_A")" ]; echo $?)" +check "should annotate the cross-channel references with the merge" \ + "$([ "$(grep '^EDIT' "$LOG" | tail -1)" = 'EDIT ✅ **Merged**' ]; echo $?)" +check "should archive once" "$([ "$(count "ARCHIVE $CH_A")" = 1 ]; echo $?)" +check "should prove the archive by the relay's refusal" "$(said 'proved by the relay refusing'; echo $?)" +check "the notice must precede the archive — it is the fence" \ + "$([ "$(first_line "SEND $CH_A")" -lt "$(first_line "ARCHIVE $CH_A")" ]; echo $?)" +check "should record the completed close, after the archive" \ + "$(grep -q '^MARKER_WRITE pr-mirror-yjc801-buzz-4242-closed closed$' "$LOG" && [ "$(first_line "ARCHIVE $CH_A")" -lt "$(first_line MARKER_WRITE)" ]; echo $?)" +check "must not record a request" "$(! grep -q 'summary-requested' "$LOG"; echo $?)" +check "should say the switch decided it" "$(said 'summary request is off'; echo $?)" +check "an event run must not touch GitHub" "$([ "$(count PR_)" = 0 ]; echo $?)" + +scenario "sweep: merged, live room, nothing on record, summary off → archived now, nobody asked" +listing "$(listed 4242 7200 true)" +bind 4242 "$CH_A" +pr_record 4242 closed true +run_step schedule; RC=$? +check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" +check "should post one notice" "$([ "$(count "SEND $CH_A")" = 1 ]; echo $?)" +check "the notice should name the merge" "$(send_body "$CH_A" | head -1 | grep -q '^✅ \*\*Merged\*\* — archiving'; echo $?)" +check "must p-tag nobody" "$([ "$(count MENTION)" = 0 ]; echo $?)" +check "should archive once" "$([ "$(count "ARCHIVE $CH_A")" = 1 ]; echo $?)" +check "should record the completed close" "$(grep -q '^MARKER_WRITE pr-mirror-yjc801-buzz-4242-closed closed$' "$LOG"; echo $?)" +check "should re-read GitHub after writing" "$([ "$(count "PR_READ 4242")" = 2 ]; echo $?)" +check "should account for it as a close, not a request" \ + "$(said '1 closed examined, 1 reconciled' && said '0 summaries requested, 0 awaiting a reply'; echo $?)" + +# The marker and the request are two writes, whatever the switch says. A +# request already in the room is a p-tag already out; archiving over it would +# refuse the summary it asked for. +scenario "closed event (merged), summary off, but a request is already in the room → honoured, not archived over" +bind 4242 "$CH_A" +ci_said "$CH_A" 300 "@Alex please post a review summary here: what the review found, what was fixed, and anything left open." +PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? +check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" +check "must not p-tag the reviewer — he has been asked" "$([ "$(count MENTION)" = 0 ]; echo $?)" +check "must write nothing into the room" "$([ "$(count "SEND $CH_A")" = 0 ]; echo $?)" +check "must not archive — the summary can only land while the room is live" "$([ "$(count ARCHIVE)" = 0 ]; echo $?)" +check "should repair the marker from the request's own timestamp" \ + "$(grep -qE "^MARKER_WRITE pr-mirror-yjc801-buzz-4242-closed summary-requested:$((NOW - 300))\$" "$LOG"; echo $?)" +check "should say what it found" "$(said 'already requested here'; echo $?)" +check "should say it is inside the grace window" "$(said 'of the grace window left'; echo $?)" + +# The forced re-close inside the convergence loop, on a merge, with the +# switch off: archived like any other close — not held for a summary nobody +# will be asked for. +scenario "sweep: a PR merged while the sweep was restoring its room, summary off → re-closed and archived" +listing "$(listed 4242 7200 false)" +bind 4242 "$CH_A" +referenced_from 4242 "$CH_B" +pr_read_at 4242 1 closed +pr_read_at 4242 2 open +pr_read_at 4242 3 closed true +pr_read_at 4242 4 closed true +run_step schedule; RC=$? +check "expected rc 0, got $RC" "$([ "$RC" -eq 0 ]; echo $?)" +check "should notice the newer close" "$(said 'closed again while this sweep was restoring it'; echo $?)" +check "must p-tag nobody" "$([ "$(count MENTION)" = 0 ]; echo $?)" +check "the room must end archived: closed, restored, closed again" \ + "$([ "$(count "ARCHIVE $CH_A")" = 2 ] && [ "$(count "UNARCHIVE $CH_A")" = 1 ] && [ "$(first_line "UNARCHIVE $CH_A")" -lt "$(nth_line "ARCHIVE $CH_A" 2)" ]; echo $?)" +check "the cross-channel banner must end on the merge" \ + "$([ "$(grep '^EDIT' "$LOG" | tail -1)" = 'EDIT ✅ **Merged**' ]; echo $?)" +check "the record must end closed" \ + "$([ "$(grep '^MARKER_WRITE' "$LOG" | tail -1)" = 'MARKER_WRITE pr-mirror-yjc801-buzz-4242-closed closed' ]; echo $?)" +check "the last read must follow the last write and agree with it" "$([ "$(count "PR_READ 4242")" = 4 ]; echo $?)" +check "should account for both compensations" \ + "$(said '1 restored after a concurrent reopen, 1 re-closed after a concurrent close, 0 failed'; echo $?)" + +scenario "a post-merge summary switch that is neither on nor off is refused before any write" +bind 4242 "$CH_A" +POST_MERGE_SUMMARY_INPUT=yes PR_MERGED_INPUT=true run_step pull_request closed 4242; RC=$? +check "expected a non-zero rc, got $RC" "$([ "$RC" -ne 0 ]; echo $?)" +check "should say what is wrong" "$(said "POST_MERGE_SUMMARY must be 'on' or 'off'"; echo $?)" +check "should send nothing" "$([ "$(count SEND)" = 0 ]; echo $?)" + echo "$PASS assertions passed, $FAILED failed" [ "$FAILED" -eq 0 ] diff --git a/.github/workflows/buzz-pr-auto-merge.yml b/.github/workflows/buzz-pr-auto-merge.yml index 5572c1ee7bc..22d52057ecf 100644 --- a/.github/workflows/buzz-pr-auto-merge.yml +++ b/.github/workflows/buzz-pr-auto-merge.yml @@ -889,10 +889,11 @@ jobs: # Announce, then act: the intent message must land BEFORE the # merge. The merge is the point of no return, and nothing here - # waits on the mirror — which, since the room is held open for - # the reviewer's post-merge summary, archives this channel - # minutes to an hour after the merge event rather than seconds - # (buzz-pr-mirror.yml, "A MERGED PR'S ROOM IS HELD OPEN"). An + # waits on the mirror — which archives this channel on the + # merge's own `closed` event, as soon as that run gets to it + # (buzz-pr-mirror.yml: POST_MERGE_SUMMARY is off on this repo; + # switched on, the room is instead held open for the reviewer's + # post-merge summary — "A MERGED PR'S ROOM IS HELD OPEN"). An # announcement that could only land after the archive would be # lost; one that lands before the merge cannot be. The wording # promises only what this job knows — the merge job re-checks @@ -1488,11 +1489,13 @@ jobs: if ! GH_TOKEN="$MERGE_TOKEN" gh api "repos/$R/issues/$PR/comments" -f body="$ALERT" >/dev/null; then echo "::error::PR #$PR: the post-merge alert could not be posted to the PR — this run log is the only record" fi - # Best-effort: the mirror archives the PR channel once the - # reviewer's post-merge summary has settled, or after its grace - # window, so this send usually lands — but it is still racing an - # archive it does not control and cannot always win. The PR - # comment above is the durable record. + # Best-effort: the mirror archives the PR channel on the merge's + # own `closed` event (buzz-pr-mirror.yml: POST_MERGE_SUMMARY is + # off on this repo), and that run checks out and installs its + # CLI before it writes, so this send — seconds after the merge — + # generally gets there first. It is still racing an archive it + # does not control and cannot always win. The PR comment above + # is the durable record. if [ -n "${CHANNEL:-}" ]; then printf '%s\n' "$ALERT" | relay send --channel "$CHANNEL" >/dev/null \ || echo "::warning::PR #$PR: post-merge alert not delivered to the PR channel — the PR comment is the durable record" diff --git a/.github/workflows/buzz-pr-mirror.yml b/.github/workflows/buzz-pr-mirror.yml index ac54bf7c683..e3dee60f298 100644 --- a/.github/workflows/buzz-pr-mirror.yml +++ b/.github/workflows/buzz-pr-mirror.yml @@ -11,10 +11,12 @@ name: buzz-pr-mirror # ever wakes anything), so a coder's "@Alex pushed the fix" # alone leaves him asleep # closed → 🚫 Closed without merge: post the notice, then archive. -# ✅ Merged: post the notice with a review-summary request -# to Alex and leave the room LIVE — the sweep archives it -# once his last reply has settled, or after a grace window -# under a notice that states only what CI saw (see below) +# ✅ Merged: the same, on this repo — the post-merge +# review-summary request is switched OFF (POST_MERGE_SUMMARY +# below). Switched on, the notice carries a review-summary +# request to Alex and the room is left LIVE — the sweep +# archives it once his last reply has settled, or after a +# grace window under a notice that states only what CI saw # reopened → unarchive, and reverse the close annotations everywhere # schedule → archive the room of any PR whose close GitHub never # announced (sweep_missed_closes — see below), finish the @@ -106,6 +108,21 @@ name: buzz-pr-mirror # CONVERGE_MAX_PASSES is red: the sweep says so and leaves the PR to the next # one, rather than reporting a repair it cannot show. # +# THE POST-MERGE SUMMARY REQUEST IS SWITCHED OFF ON THIS REPO. With +# POST_MERGE_SUMMARY=off (the env below) a merged PR's close is the close a +# not-merged PR gets: one mention-free notice, the annotation sweep, the +# archive, the marker — and nobody is asked for anything. Two things survive +# the switch, because it governs only whether a NEW request is made. A room +# that already holds a request — the close marker says `summary-requested:`, +# or the room holds a request this CI identity published and the marker +# write was lost — is finished by the hold below exactly as before: the +# reviewer was p-tagged, a summary may be mid-turn, and an archive now would +# refuse it; the hold is bounded, so such rooms drain within one grace +# window of the switch flipping. And the mechanism itself is kept and tested +# under `on` (.github/scripts/pr-mirror-close.test.sh), so flipping the +# value back restores it without a rewrite. The paragraphs that follow +# describe that mechanism. +# # A MERGED PR'S ROOM IS HELD OPEN FOR THE REVIEW SUMMARY. The room is meant # to archive into the record of why the code exists (VISION.md, "Branches # are channels"), and the reviewer's own account of what the rounds found and @@ -375,6 +392,14 @@ jobs: # the protection. See close_after_summary. Seconds, both. SUMMARY_GRACE_SECS: "3600" SUMMARY_SETTLE_SECS: "600" + # Whether a merged PR's close ASKS the reviewer for a post-merge + # review summary and holds the room open for it (`on`), or archives + # the room at once like a close without a merge (`off`). Off on + # this repo — see the header. A room already holding a request is + # finished by the hold regardless, so the windows above still + # apply to it. Exactly `on` or `off`; anything else is refused + # before any write. + POST_MERGE_SUMMARY: "off" PR_ACTION: ${{ github.event.action }} PR_MERGED: ${{ github.event.pull_request.merged }} PR_TITLE: ${{ github.event.pull_request.title }} @@ -481,6 +506,11 @@ jobs: done SUMMARY_GRACE_SECS=$((10#$SUMMARY_GRACE_SECS)) SUMMARY_SETTLE_SECS=$((10#$SUMMARY_SETTLE_SECS)) + case "$POST_MERGE_SUMMARY" in + on|off) ;; + *) echo "POST_MERGE_SUMMARY must be 'on' or 'off', got '${POST_MERGE_SUMMARY}'" >&2 + exit 1 ;; + esac # CI's own pubkey — authenticates the seed marker (only a # CI-authored card proves provisioning; the channel is open, so @@ -1549,9 +1579,12 @@ jobs: # Shared by the `closed` event and by sweep_missed_closes, so a # close GitHub never announced gets exactly the treatment an # announced one does. Sets CLOSE_OUTCOME: - # "reconciled" this call closed a not-merged PR's room - # "summary-requested" merged: the reviewer was asked for the - # summary and the room stays live + # "reconciled" this call closed the room at once: a + # not-merged PR, or a merged one with the + # summary request off + # "summary-requested" merged, summary on: the reviewer was + # asked for the summary and the room + # stays live # "awaiting-reply" merged: asked earlier, nothing decided # yet, nothing written # "closed-after-reply" merged: his last reply settled; archived @@ -1589,7 +1622,16 @@ jobs: # is not proof the reviewer was never asked, though — the marker # and the request are two writes — so request_summary reconciles # against the published request before it asks anything. - local CH="$1" FORCE="${2:-}" MARKER REQUESTED_AT + # + # WITH THE SUMMARY REQUEST OFF (POST_MERGE_SUMMARY, see the + # header) a merged PR takes the not-merged path below — one + # notice, then finish_close — unless the reviewer HAS been asked: + # a `summary-requested:` marker, or a request published under a + # lost marker write, is honoured by the hold regardless, because + # the p-tag is out and an archive now would refuse the summary it + # asked for. The switch decides whether to ask, never whether to + # keep a promise already made. + local CH="$1" FORCE="${2:-}" MARKER REQUESTED_AT PUBLISHED CLOSE_OUTCOME="" # Whether this pass put anything durable into the world. The # OUTCOME names what was decided; this names whether there is @@ -1611,8 +1653,19 @@ jobs: close_after_summary "$CH" "$FORCE" "$REQUESTED_AT" || return 1 return 0 ;; esac - request_summary "$CH" "$FORCE" || return 1 - return 0 + if [ "$POST_MERGE_SUMMARY" = on ]; then + request_summary "$CH" "$FORCE" || return 1 + return 0 + fi + # Off. The probe is the same full walk request_summary opens + # with, for the same reason: the marker's absence does not + # prove nobody asked. + PUBLISHED=$(summary_request_at "$CH") || return 1 + if [ -n "$PUBLISHED" ]; then + adopt_published_request "$CH" "$PUBLISHED" "$FORCE" || return 1 + return 0 + fi + echo "#${NAME}: merged, and the summary request is off — archiving at once" fi if ! printf '%s — archiving this channel.\n%s\n' "$STATUS" "$PR_URL" \ | buzz messages send --channel "$CH" --content - 2>"$ERRFILE"; then @@ -2193,10 +2246,13 @@ jobs: # it under an open PR. # # A merged PR comes through here on every sweep inside the window - # until its close is complete: first to request the review summary - # if no run has yet, then to archive the room once the reviewer's - # reply has settled or the grace window has passed (see the - # header). Passes that decide nothing write nothing. + # until its close is complete. With the summary request on: first + # to request the review summary if no run has yet, then to archive + # the room once the reviewer's reply has settled or the grace + # window has passed (see the header). With it off, as on this + # repo: once, to archive the room like any other close — unless + # the room already holds a request, which the hold finishes. + # Passes that decide nothing write nothing. # # One PR's failure fails the sweep but not its siblings: each is # independent, the cron retries, and a red run is the honest diff --git a/docs/pr-auto-merge.md b/docs/pr-auto-merge.md index 4016f26ec2e..9347c7da5ff 100644 --- a/docs/pr-auto-merge.md +++ b/docs/pr-auto-merge.md @@ -267,9 +267,10 @@ or a republished approval a changed authorization would be a false report: All three go **red**, post the reason to the PR, and best-effort tell the PR channel (best-effort because the mirror archives that channel after the merge -— once the reviewer's post-merge summary has settled, or after its grace -window, see [Post-merge review summary](#post-merge-review-summary) — and this -send does not wait on it; the PR comment is the durable record). All three +— on this repo on the merge's own `closed` event, the post-merge summary +request being switched off, see [Post-merge review +summary](#post-merge-review-summary) — and this send does not wait on it; the +PR comment is the durable record). All three are "after state changed", which is the one category this workflow's failure philosophy reserves red for rather than degrading to a warning. Prevention was impossible. Silence was not. @@ -439,8 +440,9 @@ together (`just auto-merge-check`, wired into CI's contract steps). - Before merging, CI posts an intent message into the PR channel (`⏩ auto-merge authorized for — verdict event … Merging now if the final gates still hold.`) — *before*: the merge is the point of no - return, and nothing here waits on the mirror, which archives the channel - once the reviewer's post-merge summary has settled (see [Post-merge review + return, and nothing here waits on the mirror, which archives the channel on + the merge's `closed` event (the post-merge summary request is switched off + on this repo — see [Post-merge review summary](#post-merge-review-summary)). If the announcement cannot be published, the merge does not happen this tick. The wording is hedged on purpose: the message is written by `evaluate`, and the isolated `merge` job @@ -457,8 +459,24 @@ together (`just auto-merge-check`, wired into CI's contract steps). ## Post-merge review summary -A merged PR's room is not archived by its `closed` event. The mirror -(`.github/workflows/buzz-pr-mirror.yml`) posts the merge notice and then, as +**Switched off on this repo.** The mirror's `POST_MERGE_SUMMARY` env value is +`"off"` (`.github/workflows/buzz-pr-mirror.yml`), so a merged PR's `closed` +event archives its room exactly as a close without a merge does: one +mention-free notice (`✅ **Merged** — archiving this channel.`), the +cross-channel annotations, the archive, the close marker — and the reviewer is +not asked for anything. The switch governs only whether a *new* request is +made. A room that already holds one when it flips — a `summary-requested:` +marker, or a request this CI identity published under a lost marker write — is +still finished by the hold described below, because the p-tag is out and an +archive now would refuse the summary it asked for; such rooms drain within one +grace window. The mechanism is kept and tested under `on` +(`.github/scripts/pr-mirror-close.test.sh`, which also asserts the shipped +value is `"off"`), so setting it back to `"on"` restores it. What follows +describes that mechanism. + +With the request on, a merged PR's room is not archived by its `closed` event. +The mirror (`.github/workflows/buzz-pr-mirror.yml`) posts the merge notice and +then, as the last thing it writes into the room, a request to the reviewer — a CI-authored p-tag, the only kind that wakes an agent (`crates/buzz-waker/src/decide.rs`: no agent-authored event ever wakes