fix(automerge): anchor the quiet period on the review TRIGGER, not only on elapsed silence - #161
Conversation
…ly on elapsed silence wxa_vpn#1392, incident PR wxa_vpn#1512 (2026-08-14). The quiet gate held the arm until the conversation had been silent for findings_quiet_minutes, measured against three anchors — newest authored commit, newest inline comment, newest non-self [bot] comment. All three measure silence that has ALREADY ELAPSED, which is the right question only when the event that woke the workflow did not itself start a review. `ready_for_review` does exactly that. pr-review.yml, pr-codex-review.yml and this workflow share the same `types:` list, so readying a draft fires the review lanes and arms the gate in the same instant, while every anchor the gate can read still dates from before the PR was drafted. The longer a PR sits in draft, the more thoroughly the gate is satisfied that nobody is talking — at the exact moment three review lanes start talking. Measured on #1512, which was drafted at 03:21 and readied at 05:38:04Z: Quiet: newest commit/review activity is 7757s old (>= 1200s). whois-api-llc/wxa_vpn#1512 — no unaddressed findings (last commit 03:20:37Z) Enabling auto-merge (squash) [05:38:26Z] It never slept. The detector's "clean" was accurate and useless: it read a conversation the reviews had not joined yet. Codex posted VERDICT: REGRESSION at 05:39:01Z — 35s after the arm — and the PR merged at 05:42:51Z carrying it. The regression shipped and needed follow-up PR #1518 to fix. #1518 then cleared the same way at 3074s; its review came back clean, so that one was a near miss rather than a hit. A longer window is not the fix: no window helps when the clock starts pre-expired, and 7757s already exceeded the 20-minute default by 6.5x. The anchor has to include the moment the CURRENT round of review was triggered — so c4 is the newest `ready_for_review`/`reopened` event. This is not new policy. The push path already waits: wxa_vpn#1513, the same day, slept 1162s then 441s and merged clean. This only stops the ready path from skipping a wait the push path has always taken. Three properties the tests pin, because over-gating is how these controls get switched off: * the anchor is the event's TIMESTAMP, not a boolean "was readied", so a later `labeled` re-run sees it age out normally and the gate cannot be held shut by its own re-runs; * only the two review triggers anchor — `labeled` above all must not, since pr-classify labels every PR within seconds of opening and this workflow re-runs on `labeled`; * a PR with no such event keeps exactly the pre-fix anchor set. The read fails CLOSED like its three siblings: "could not read the timeline" and "the PR was never readied" are the same empty value downstream, and only the second justifies arming. Tests: selftest/test_automerge_quiet_anchor.sh (14 checks). Proven to bite by mutation — removing the anchor fails 5 checks with A1 reporting `sleeps=[] clear=1 reason=quiet+clean`, the #1512 log line exactly; making the read fail-open fails A5 alone; dropping the event filter fails S2 and all six A4 cases. test_automerge_findings_gate.sh's S4 loop grew the fourth endpoint (its claim is "every anchor read paginates", and a list enumerating three of four silently stops being that) and its gh stub grew a timeline arm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Coverage Floor — mode:
|
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Risk class: This PR touches one of the blocked path categories from Auto-merge is refused by (This is a policy notice, not a code-quality failure. The classify job itself does not fail — required CI checks remain authoritative for "is the code green.") |
…pper list's completeness test_workflow_guards.py runs the .sh selftests under pytest via an explicit parametrize list — "what makes them CI-enforced rather than run-manually-only documentation", per its own docstring. This PR's test_automerge_quiet_anchor.sh was not in that list: it passed 14/14 on the workstation and ran in CI never, which is the exact silently-unenforced shape the gate it tests exists to close. Listing it fixes the instance. The new test_every_shell_selftest_is_listed fixes the class: every selftest/test_*.sh on disk must be in the list or in _UNLISTED_OK (named exemptions with reasons; currently only test_bb_automerge_risk_patterns.sh, which needs a workstation checkout). Stale exemptions fail too. The guard bit on its first execution: test_claude_review_max_turns_type.sh (#152) and test_ruff_format_gate.sh (#160) both shipped unlisted and have never run in CI. Both pass today; both are now listed. Mutation-proven the other way with a stray selftest/test_stray_zz.sh (fails naming it), then removed. 45 tests pass (was 42 — the three newly-wired selftests now run). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| printf '%s\n%s\n%s\n' "$c1" "$c2" "$c3" | grep -v '^$' | sort | tail -1 | ||
| timeline=$(retry gh api "repos/${GITHUB_REPOSITORY}/issues/${PR}/timeline?per_page=100" --paginate) || return 1 | ||
| c4=$(printf '%s' "$timeline" | jq -rs '[(add // [])[] | ||
| | select(.event == "ready_for_review" or .event == "reopened") |
There was a problem hiding this comment.
opened event is a review trigger not anchored by c4
pr-review.yml and pr-codex-review.yml trigger on types: [opened, synchronize, reopened, ready_for_review]. The automerge gate fires on the same set of events. When a PR is opened (not as a draft), the review lanes start in the same instant — but opened is not in this filter, so c4 is empty and the gate falls back to c1 (commit date).
If a developer pushes commits to a branch at T0, then opens the PR at T1 > T0 + 20m, the gate sees c1 as old, skips the quiet wait, and arms before any review lands — the exact incident shape of wxa_vpn#1512 replayed on the opened path.
The PR comment says "A PR opened non-draft and never reopened has no such event, max yields empty, and the anchor set is exactly what it is today", which acknowledges the gap. But opened fires the same three review lanes as ready_for_review, so the justification for anchoring on ready_for_review applies equally here. Consider also selecting .event == "opened", or documenting why the commit-date anchor is considered sufficient for the opened path.
There was a problem hiding this comment.
Fixed in 440d61e — c5 (PR creation time, from the event payload) anchors the opened trigger with A7/A8/S3 pinning it (mutation: dropping c5 fails A7 with sleeps=[] clear=1, the incident shape); the immutable timeline read is hoisted out of the wait loop with its own fail-closed error; the in-loop anchor error names exactly the reads it covers; A3 asserts clear=1; the date stub's unmapped-timestamp error now says where to add the mapping.
| new_case | ||
| timeline_with reopened 2026-01-01T02:00:00Z | ||
| exec_gate 20 | ||
| if [ "$RC" -eq 0 ] && grep -q '^1000$' "$CASE/sleep.log"; then |
There was a problem hiding this comment.
A3 checks the sleep but not the arm — a regression in reopened would pass undetected
A1 (the analogous case for ready_for_review) asserts all three: RC -eq 0, clear=1 in GITHUB_OUTPUT, and sleep.log contains 1000. A3 only asserts RC -eq 0 and sleep.log. If a bug causes the gate to sleep the correct 1000 s but then write clear=0 (e.g., the findings-check path misbehaves for the reopened case), A3 passes while the gate is broken.
| if [ "$RC" -eq 0 ] && grep -q '^1000$' "$CASE/sleep.log"; then | |
| if [ "$RC" -eq 0 ] && grep -q '^clear=1$' "$CASE/output" && grep -q '^1000$' "$CASE/sleep.log"; then |
There was a problem hiding this comment.
Fixed in 440d61e — c5 (PR creation time, from the event payload) anchors the opened trigger with A7/A8/S3 pinning it (mutation: dropping c5 fails A7 with sleeps=[] clear=1, the incident shape); the immutable timeline read is hoisted out of the wait loop with its own fail-closed error; the in-loop anchor error names exactly the reads it covers; A3 asserts clear=1; the date stub's unmapped-timestamp error now says where to add the mapping.
| c4=$(printf '%s' "$timeline" | jq -rs '[(add // [])[] | ||
| | select(.event == "ready_for_review" or .event == "reopened") | ||
| | .created_at] | max // empty') || return 1 | ||
| printf '%s\n%s\n%s\n%s\n' "$c1" "$c2" "$c3" "$c4" | grep -v '^$' | sort | tail -1 |
There was a problem hiding this comment.
Stale error message 7 lines below: "commits/comments" now omits timeline
read_anchor now makes four API calls (commits, inline comments, issue comments, timeline). When the timeline read fails, the error at line 1168 still says (commits/comments) — steering an on-call engineer toward the wrong endpoints. The message needs to include timeline:
::error::could not read the quiet-period anchors (commits/comments/timeline) after retries — refusing to arm auto-merge (fail closed).
There was a problem hiding this comment.
Fixed in 440d61e — c5 (PR creation time, from the event payload) anchors the opened trigger with A7/A8/S3 pinning it (mutation: dropping c5 fails A7 with sleeps=[] clear=1, the incident shape); the immutable timeline read is hoisted out of the wait loop with its own fail-closed error; the in-loop anchor error names exactly the reads it covers; A3 asserts clear=1; the date stub's unmapped-timestamp error now says where to add the mapping.
| | select(((.body // "") | test("<!-- (claude-author-automerge|pr-classify)")) | not) | ||
| | .created_at] | max // empty') || return 1 | ||
| printf '%s\n%s\n%s\n' "$c1" "$c2" "$c3" | grep -v '^$' | sort | tail -1 | ||
| timeline=$(retry gh api "repos/${GITHUB_REPOSITORY}/issues/${PR}/timeline?per_page=100" --paginate) || return 1 |
There was a problem hiding this comment.
Timeline API re-fetched on every loop iteration, but ready_for_review is immutable
c1 (commits), c2 (inline review comments), and c3 (bot comments) can all change between iterations — a new push, a new review — so re-reading them makes sense. c4 (ready_for_review/reopened events) cannot change: historical PR timeline events are immutable.
For a PR with a long timeline (many labels, reviews, assignments over months), --paginate on the timeline endpoint can require many HTTP requests. This cost is paid on every sleep/wake cycle. Consider reading the timeline once before the loop and reusing the result:
local c4_cached
c4_cached=$(read_c4) || return 1 # single read before loop startsThe quiet-cap design (cap = 3 * QUIET_S) means the loop can run multiple times for very active PRs, so the extra round-trips are not purely theoretical.
There was a problem hiding this comment.
Fixed in 440d61e — c5 (PR creation time, from the event payload) anchors the opened trigger with A7/A8/S3 pinning it (mutation: dropping c5 fails A7 with sleeps=[] clear=1, the incident shape); the immutable timeline read is hoisted out of the wait loop with its own fail-closed error; the in-loop anchor error names exactly the reads it covers; A3 asserts clear=1; the date stub's unmapped-timestamp error now says where to add the mapping.
| printf '%s\n%s\n%s\n' "$c1" "$c2" "$c3" | grep -v '^$' | sort | tail -1 | ||
| timeline=$(retry gh api "repos/${GITHUB_REPOSITORY}/issues/${PR}/timeline?per_page=100" --paginate) || return 1 | ||
| c4=$(printf '%s' "$timeline" | jq -rs '[(add // [])[] | ||
| | select(.event == "ready_for_review" or .event == "reopened") |
There was a problem hiding this comment.
Bug (PLAUSIBLE): opened trigger has the same pre-fix vulnerability — the fix is incomplete
The c4 filter covers ready_for_review and reopened but not opened. The caller's types: list is [opened, synchronize, reopened, ready_for_review], so opened fires both the review lanes AND this gate simultaneously — exactly the triggering condition that caused wxa_vpn#1512.
Concrete failure scenario: A developer pushes commits to claude/fix-something at 14:00, then opens the PR at 16:30 (2.5 hours later, a common pattern when commits are staged before PR creation). At 16:30 the gate fires on opened, reads c1 = 14:00 (9000s ago, >> 1200s), c4 is empty (GitHub's timeline API has no opened event type — opened is a PR-creation event, not a timeline event), and the gate arms immediately while pr-review.yml and pr-codex-review.yml are just starting their runs.
The PR comment says "exactly what it is today" for this path, but gives no reason why opened is safer than ready_for_review. It isn't: the mechanism is identical, and the incident (old commits + simultaneous arm + reviews not yet posted) can recur.
Possible fix: Before the while loop, separately read the PR's created_at field via gh api repos/.../pulls/${PR} --jq .created_at and include it in the anchor set only when GITHUB_EVENT_NAME == pull_request && GITHUB_EVENT_ACTION == opened. This requires a GITHUB_EVENT_ACTION env var in the step's env: block, but the approach is the same point-in-time anchor applied here for the other two triggers.
There was a problem hiding this comment.
Fixed in 440d61e — c5 (PR creation time, from the event payload) anchors the opened trigger with A7/A8/S3 pinning it (mutation: dropping c5 fails A7 with sleeps=[] clear=1, the incident shape); the immutable timeline read is hoisted out of the wait loop with its own fail-closed error; the in-loop anchor error names exactly the reads it covers; A3 asserts clear=1; the date stub's unmapped-timestamp error now says where to add the mapping.
| case "$3" in | ||
| 2026-01-01T00:00:00Z) echo 1000000 ;; | ||
| 2026-01-01T02:00:00Z) echo 1006800 ;; | ||
| *) echo "date-stub: unmapped timestamp '$3'" >&2; exit 1 ;; |
There was a problem hiding this comment.
date stub exits 1 for any timestamp not in the two-entry map — cryptic failure for future test cases
The stub maps only 2026-01-01T00:00:00Z (OLD) and 2026-01-01T02:00:00Z (YOUNG). All six current cases happen to produce an anchor that is one of these two values, so this is not triggered today.
If a future case introduces a third fixture timestamp (e.g., a CHATTY scenario analogous to the one in test_automerge_findings_gate.sh), the stub exits 1, and the arithmetic expansion $(date -u -d "$anchor" +%s) fails inside age=$(( ... )). With set -euo pipefail, the gate script aborts and RC is non-zero — but the failure manifests as date-stub: unmapped timestamp '...' in stdout rather than a recognisable assertion failure.
test_automerge_findings_gate.sh's date stub avoids this by adding a third dynamic entry:
2026-01-01T09:09:09Z) echo $(( $(cat "$T_NOW") - 100 )) ;;Consider mirroring that pattern, or documenting the two-entry constraint as intentional so the next author knows to extend the stub before adding a new fixture timestamp.
There was a problem hiding this comment.
Fixed in 440d61e — c5 (PR creation time, from the event payload) anchors the opened trigger with A7/A8/S3 pinning it (mutation: dropping c5 fails A7 with sleeps=[] clear=1, the incident shape); the immutable timeline read is hoisted out of the wait loop with its own fail-closed error; the in-loop anchor error names exactly the reads it covers; A3 asserts clear=1; the date stub's unmapped-timestamp error now says where to add the mapping.
| c4=$(printf '%s' "$timeline" | jq -rs '[(add // [])[] | ||
| | select(.event == "ready_for_review" or .event == "reopened") | ||
| | .created_at] | max // empty') || return 1 | ||
| printf '%s\n%s\n%s\n%s\n' "$c1" "$c2" "$c3" "$c4" | grep -v '^$' | sort | tail -1 |
There was a problem hiding this comment.
Bug (CONFIRMED): Error message 7 lines below still says "commits/comments" — misleading when the timeline read fails
At line 1168 (unchanged), the error message printed when read_anchor fails still reads:
could not read the quiet-period anchors (commits/comments) after retries
read_anchor now makes four API calls. When the timeline call at line 1157 fails (e.g., transient API error or rate-limit on /issues/{n}/timeline), this message tells operators to inspect the commits/comments endpoints — which may be perfectly healthy. An incident responder following this error would look in the wrong place.
Fix: update line 1168 to read (commits/comments/timeline) or just (all four anchor reads).
There was a problem hiding this comment.
Fixed in 440d61e — c5 (PR creation time, from the event payload) anchors the opened trigger with A7/A8/S3 pinning it (mutation: dropping c5 fails A7 with sleeps=[] clear=1, the incident shape); the immutable timeline read is hoisted out of the wait loop with its own fail-closed error; the in-loop anchor error names exactly the reads it covers; A3 asserts clear=1; the date stub's unmapped-timestamp error now says where to add the mapping.
| new_case | ||
| timeline_with reopened 2026-01-01T02:00:00Z | ||
| exec_gate 20 | ||
| if [ "$RC" -eq 0 ] && grep -q '^1000$' "$CASE/sleep.log"; then |
There was a problem hiding this comment.
Bug (PLAUSIBLE): A3 missing clear=1 check — test would pass even if reopened sleeps but gate fails to arm
A1 (the ready_for_review case) checks three conditions: RC=0, clear=1 in GITHUB_OUTPUT, and sleep.log contains 1000. A3 only checks two: RC=0 and sleep.log contains 1000.
Concrete failure scenario: A regression changes the gate so that when the anchor is a reopened event, the findings-check stub path writes clear=0 instead of clear=1 (e.g., wrong branch taken after the quiet period). The gate sleeps 1000s correctly (so sleep.log has 1000) and exits 0 (so RC=0). A3 passes. The actual arming behavior is broken but CI is green.
| if [ "$RC" -eq 0 ] && grep -q '^1000$' "$CASE/sleep.log"; then | |
| if [ "$RC" -eq 0 ] && grep -q '^clear=1$' "$CASE/output" && grep -q '^1000$' "$CASE/sleep.log"; then |
There was a problem hiding this comment.
Fixed in 440d61e — c5 (PR creation time, from the event payload) anchors the opened trigger with A7/A8/S3 pinning it (mutation: dropping c5 fails A7 with sleeps=[] clear=1, the incident shape); the immutable timeline read is hoisted out of the wait loop with its own fail-closed error; the in-loop anchor error names exactly the reads it covers; A3 asserts clear=1; the date stub's unmapped-timestamp error now says where to add the mapping.
| | select(((.body // "") | test("<!-- (claude-author-automerge|pr-classify)")) | not) | ||
| | .created_at] | max // empty') || return 1 | ||
| printf '%s\n%s\n%s\n' "$c1" "$c2" "$c3" | grep -v '^$' | sort | tail -1 | ||
| timeline=$(retry gh api "repos/${GITHUB_REPOSITORY}/issues/${PR}/timeline?per_page=100" --paginate) || return 1 |
There was a problem hiding this comment.
Efficiency (PLAUSIBLE): c4 is immutable but re-fetched on every loop iteration
c1, c2, and c3 must be re-read on each iteration because new commits or review comments can arrive while the gate is sleeping. c4 cannot: ready_for_review and reopened are point-in-time historical events; once a PR is readied it cannot un-ready itself to change c4's value.
For a PR with a long event history (hundreds of labels, reviews, assignments), --paginate on /issues/{n}/timeline makes multiple HTTP calls. At the 3× quiet cap, the loop can iterate many times with short remaining periods, multiplying those calls proportionally.
Fix: compute c4 once before the loop and include it as a fixed offset in read_anchor's final printf:
c4_fixed=$(…timeline jq…) || exit 1 # before the while :; do
read_anchor() {
…
printf '%s\n%s\n%s\n%s\n' "$c1" "$c2" "$c3" "$c4_fixed" | grep -v '^$' | sort | tail -1
}This eliminates all redundant timeline fetches without changing behavior, since the value can never differ between iterations.
There was a problem hiding this comment.
Fixed in 440d61e — c5 (PR creation time, from the event payload) anchors the opened trigger with A7/A8/S3 pinning it (mutation: dropping c5 fails A7 with sleeps=[] clear=1, the incident shape); the immutable timeline read is hoisted out of the wait loop with its own fail-closed error; the in-loop anchor error names exactly the reads it covers; A3 asserts clear=1; the date stub's unmapped-timestamp error now says where to add the mapping.
| case "$3" in | ||
| 2026-01-01T00:00:00Z) echo 1000000 ;; | ||
| 2026-01-01T02:00:00Z) echo 1006800 ;; | ||
| *) echo "date-stub: unmapped timestamp '$3'" >&2; exit 1 ;; |
There was a problem hiding this comment.
Test fragility (PLAUSIBLE): date stub only maps 2 timestamps — cryptic failure if new fixtures are added
The sibling file test_automerge_findings_gate.sh maps three timestamps in its date -u -d stub (it also handles 2026-01-01T09:09:09Z for its livelock/CHATTY case). This file maps only two, with a hard exit 1 for anything else.
Concrete failure scenario: A future contributor adds a CHATTY-style test case here (e.g. to pin the quiet-cap behaviour), uses a new timestamp like 2026-01-01T06:00:00Z, and the stub exits 1 inside age=$(( $(date -u +%s) - $(date -u -d "$anchor" +%s) )). With set -euo pipefail, the gate script aborts with a bash arithmetic expansion error — no ✗ line, no failed=1, just a confusing non-zero RC and an error message pointing at date-stub: unmapped timestamp. The failing assertion appears to be gate logic; the real cause is a missing stub mapping.
The companion test already has the fix: map timestamps dynamically via echo $(( $(cat "$T_NOW") - N )) for relative values, or add a *) date -u -d "$3" +%s ;; fallback to the real date binary for unmapped values.
There was a problem hiding this comment.
Fixed in 440d61e — c5 (PR creation time, from the event payload) anchors the opened trigger with A7/A8/S3 pinning it (mutation: dropping c5 fails A7 with sleeps=[] clear=1, the incident shape); the immutable timeline read is hoisted out of the wait loop with its own fail-closed error; the in-loop anchor error names exactly the reads it covers; A3 asserts clear=1; the date stub's unmapped-timestamp error now says where to add the mapping.
|
Flagged 5 issues inline — see comments. Most significant: the |
…5); hoist immutable reads; test tightening Five findings from #161's Claude Review, all addressed: 1. THE REAL ONE: `opened` is in the same review-lane `types:` list, so a branch whose commits idled before the PR was opened NON-draft arrives with c1 pre-aged and armed at the instant the reviews started — wxa_vpn#1512's hole through a different door, which c4 (ready/reopen events) cannot see because a never-drafted PR has neither event. c5 is the PR's own creation time, taken from the event payload (zero API reads; immutable, so a later labeled re-run sees it aged out). A7 pins the door (mutation: dropping c5 from the anchor set fails A7 with `sleeps=[] clear=1` — the incident shape); A8 pins that an aged-out creation cannot hold the gate shut; S3 pins the env binding, because the run block is ${{ }}-free and an unbound var is an empty c5 forever. 2. Immutable anchors hoisted out of the wait loop: c4/c5 cannot change while the run sleeps (a fresh ready_for_review event's own run replaces this one via the callers' cancel-in-progress groups), so the timeline is read once instead of per wake — on a long timeline --paginate cost multiplied per iteration. Each pre-loop read fails closed with its own named error. 3. The in-loop anchor error no longer says "commits/comments" while the timeline read lives elsewhere — each failure names exactly the reads it covers. 4. A3 now asserts clear=1 like A1 — it previously passed a gate that slept correctly and then failed to arm. 5. The date stub's unmapped-timestamp error now tells a future contributor where to add the mapping instead of failing cryptically inside an arithmetic expansion. All 17 checks pass; actionlint + shellcheck clean; sibling selftests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reviewed. No inline comments posted (no 80 %+ confirmed bugs), but two latent issues worth noting:
|
…name the undatable-event boundary — round-2 advisories The sibling harness inherits ambient env: a developer shell exporting PR_CREATED_AT would activate c5 in tests that assert zero sleeps. Pinned empty (proven by running the suite WITH a hostile ambient value exported). And the fail-closed comment now says what it covers — readability (fetch+parse) — and that a readable event with created_at:null is UNDATABLE and contributes no anchor: c4 carries a timestamp, not an existence claim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both round-2 advisories addressed in c232c68: findings_gate's exec_gate now pins PR_CREATED_AT="" (proven by running that suite with a hostile ambient export — green), and the fail-closed comment now states its boundary precisely: it covers READABILITY (fetch + parse); a readable event carrying |
|
No issues found. Logic, null handling, jq expressions, Bash scoping, and test coverage all checked out clean. |
wxa_vpn#1392's false-positive half, measured on wxa_vpn#1523: the report's top
three hits were the PR's own "Fixed in 38774043" inline replies, posted 7-12s
after the fix commit they cited. A reply that RESOLVES a finding necessarily
post-dates the commit it cites — the author pushes, then replies citing the
push — so every resolution round tripped the newer-than-last-commit signal and
the report re-flagged resolved threads forever. Across that PR: 5/5 false
positives carried in_reply_to_id; 0/14 genuine bot findings did.
The narrowing drops inline comments that are BOTH a reply AND human:
select(.in_reply_to_id == null or ((.user.login // "") | endswith("[bot]")))
Untouched on purpose: human TOP-LEVEL inline comments still count (a human's
own comment on the diff is a deliberate finding — author-agnosticism survives),
and BOT replies still count (a reviewer bot answering a thread with "still
broken after the fix" is a live finding; keeping them cost zero false positives
in the measured corpus). Accepted residual: a human typing a NEW finding into
an existing thread is skipped — the operator wrote it, so the operator knows it.
This matters beyond the operator CLI because the automerge quiet gate runs this
detector and quotes its report verbatim into auto-merge-withheld comments — a
false flag there declines arms on clean PRs, and a gate that blocks passing PRs
gets switched off.
Vendored-twin discipline: the same edit ships to dotclaude's
templates/ci-workflows/scripts/bb-unaddressed-findings.sh in a paired PR;
byte-identity below the header block verified both ways.
Tests: selftest/test_findings_reply_narrowing.sh (6 checks), CI-wired via
_SHELL_SELFTESTS. Proven to bite by mutation: removing the narrowing fails
R1+R4 (the #1523 shape, rc=1 where 0 expected); dropping the bot escape fails
R2; over-correcting to bots-only inline fails R3+R5. Restored, all green;
shellcheck clean on both files.
Stacked on fix/automerge-quiet-anchor-review-trigger (#161) because both PRs
touch _SHELL_SELFTESTS — review only the top commit until #161 merges.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
DRAFT — needs manual review and a manual merge. PRs to
topcoder1/ci-workflowsare always manual (global
CLAUDE.md), and this one changes the gate that decideswhether every fleet PR auto-arms. Do not ready it until the reasoning below has
been checked, particularly the two over-correction guards.
HUMAN_READABLE_SUMMARY:The auto-merge findings gate waits for the reviewconversation to go quiet before arming, but it only ever measured silence that had
already elapsed. Readying a draft starts the review lanes and arms the gate in the
same instant — so a PR that sat in draft for two hours looked maximally quiet at the
exact moment three reviewers began speaking, and armed before any of them posted.
This adds the
ready_for_review/reopenedtimestamp as a fourth anchor, so theready path waits exactly as long as a push already does.
Why the existing gate did not fire — measured, not assumed
ci-workflows#149added this gate and it works: it withheld the arm on wxa_vpn#1503.The question was why it let wxa_vpn#1512 through on 2026-08-14. It is not a blind spot
in the detector, and it is not that the gate skips a path — the gate ran, cleared, and
armed.
From run 31773626968,
step Quiet period + unaddressed findings, job wall time 05:38:10Z → 05:38:29Z (19s)
against a 20-minute window:
The PR was drafted at 03:21 and readied at 05:38:04Z. All three anchors — last authored
commit, last inline comment, last non-self bot comment — dated from before it went into
draft, so the window was already satisfied 6.5× over at the instant of readying. The
gate never slept. The detector's "clean" was accurate and useless: it read a conversation
the reviews had not joined yet. Codex posted
VERDICT: REGRESSIONat 05:39:01Z, 35seconds after the arm, and the PR merged at 05:42:51Z carrying it. It shipped and needed
follow-up PR wxa_vpn#1518 to fix.
The generator: the gate asks "has anyone spoken recently?" when the question it needs
answered is "have the reviewers this event just triggered finished speaking?" Those differ
by exactly one case — the one where the arming event is the review trigger.
pr-review.yml,pr-codex-review.ymlandclaude-author-automerge.ymlshare the sametypes:list, so that case isready_for_review(andreopened), and it fires on everydraft that is readied.
A longer window cannot fix it. No window helps when the clock starts pre-expired.
What the fix is
One more anchor in
read_anchor(): the newestready_for_review/reopenedtimelineevent,
max'd in with the other three.This is not new policy — the push path already waits. wxa_vpn#1513, the same day,
slept 1162s then 441s and merged clean. This only stops the ready path from skipping a
wait the push path has always taken.
Scope of the behaviour change
synchronizelabeledre-run, long after readysafe-paths-automerge.yml, no gateThe last row is deliberate: docs-only auto-merge is policy and
safe-paths-automerge.ymlhas no findings gate by design. Nothing here touches it.
Tests — proven to bite, not merely added
New
selftest/test_automerge_quiet_anchor.sh(14 checks), executing the run blockextracted from the shipped workflow under
gh/date/sleepstubs, same harness astest_automerge_findings_gate.sh. Three mutations, each caught by the cases meant tocatch it:
sleeps=[] output=[clear=1 reason=quiet+clean], the #1512 log line exactly|| return 1→|| timeline='[]'(fail open).event ==filter (anchor on everything)Reverted after each; full suite green. The over-correction guards (A2, A4, A6) pass
under mutation 1 by design — they guard against the fix going too far, not against its
absence, so they must not move when it is removed.
A2andA4are the ones worth reviewing hardest. Over-gating is how these controls getset to
findings_quiet_minutes=0, so the anchor is pinned to be the event's timestamp(not a boolean "was readied", which would hold every later run for a full window) and
pinned to ignore
labeled— pr-classify labels every PR within seconds of opening, andthis workflow re-runs on
labeled.Also:
test_automerge_findings_gate.sh's S4 loop grew the fourth endpoint. Its claim is"every anchor read paginates", and a list enumerating three of four silently stops being
that. Its
ghstub grew a timeline arm returning[](the pre-fix anchor set exactly).Verification run locally: new test 14/14;
test_automerge_findings_gate.sh130 checks,test_automerge_hold_gate.sh24,test_automerge_base_gate.sh30,test_automerge_riskfile_gate.sh22 — all pass.
actionlintandshellcheck -s bashclean.Two corrections to the incident report, from the run data
Of the five wxa_vpn PRs said to have merged past open findings on 08-14, one is an
auto-arm failure:
A near miss, not a hit.
risk:blocked; the gate refused to arm and said so at 20:01:09Z. The21:10:27Z finding was live, and it was merged manually at 21:20:14Z, four minutes
after the operator's own comment noting
bb-unaddressed-findings.shexits 1. That isthe manual lane, which this gate never blocks by design.
an adversarial review posted at 00:38:16Z, after the 00:31:58Z merge. On #1513 the gate
waited the full 20 minutes (slept 1162s + 441s) and merged clean.
Known defect this PR does NOT fix
unaddressed-findings.shflags non-bot inline replies that resolve a finding. Onwxa_vpn#1523 the author's five "Fixed in
38774043" replies landed 7–12s after the fixcommit and every one was flagged. Measured on that PR: 5/5 false positives carry
in_reply_to_id; 0/14 genuine bot findings do. The one-line narrowing isselect(.in_reply_to_id == null or (.user.login | endswith("[bot]"))).Left out of this PR on purpose. It is pre-existing and not worsened here — inline
comments already anchor the clock, so the gate already waits on such a reply and then
flags it, with or without this change. And the detector is a vendored twin required to
stay byte-identical with
topcoder1/dotclaude'stemplates/ci-workflows/scripts/bb-unaddressed-findings.sh(verified identical today), soit needs a paired PR in both repos rather than a rider on this one. Happy to open that pair
next.
🤖 Generated with Claude Code