fix(pr-review): retry the merge-queue state read and surface a failed read

[don-petry]

Closes #2186

Problem

mq_dequeue_if_unapproved runs right after an approval is dismissed. It read the PR's live queue and review state once; if that read failed (a transient GraphQL error, a rate limit) it logged one warning and returned, and a PR sitting in the merge queue could then merge with no approval standing. Raised by CodeRabbit on the pull request for #2174 and accepted there as a follow-up.

What changes

• The state read is retried a bounded number of times (MQ_READ_ATTEMPTS, default 3; MQ_READ_RETRY_DELAY_SEC between attempts, default 2).
• If every attempt fails, the unknown state is also written to the job step summary, naming the PR. Still no dequeue call and no PR comment on that path (most PRs that reach it are not queued), and the run never fails.
• AGENTS.md documents the retry and the two settings.

Risk

Low. One library function gains a retry loop; a malformed setting falls back to the default. Worst case is up to two extra reads and four seconds on a path that only runs after a dismissal.

Test plan

• bats tests/test_merge_queue_dequeue.bats: 27 of 27 pass locally (three new cases: one failed read then success still dequeues; every read failing makes exactly MQ_READ_ATTEMPTS reads, no dequeue, no comment, a summary entry, exit 0; a malformed setting uses the default). The 24 existing tests are unchanged.
• Mutation check: with the loop cut to a single attempt, all three new tests fail.
• scripts/check-duplicate-decls.sh passes. shellcheck -x reports only the two existing SC2016 info notes on the GraphQL query strings.

Rollback

Revert this commit. The dequeue behaviour returns to a single read.

Monitoring

A failed read shows as a ::warning:: in the pr-review run and as a "Merge queue: state unknown" entry in its step summary.

This PR is authored directly in the session and labelled dev-lead:hands-off.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 17 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 46707b7e-e2fd-4dec-9419-725437c209bd

---

📥 Commits

Reviewing files that changed from the base of the PR and between 12cbe82 and ef15fb1.

---

📒 Files selected for processing (3)

• AGENTS.md
• scripts/lib/merge-queue-dequeue.sh
• tests/test_merge_queue_dequeue.bats

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[gemini-code-assist]

Code Review

This pull request introduces a retry mechanism with bounded attempts and delays for reading the live merge-queue and review state, addressing transient GraphQL errors. It also logs failures to the GitHub step summary when all retries fail. The review feedback highlights a potential issue in the retry logic where a standalone conditional expression (&&) could cause unintended script termination under set -e if the delay is zero, and suggests using an explicit if block instead.

[codeant-ai]

CodeAnt Nitpicks

2 code suggestions

1. Any large valid attempt count or delay can keep this synchronous call retrying for hours before it surfaces the unknown state.

Performance · scripts/lib/merge-queue-dequeue.sh:67-70

---

2. This summary also runs after a new approval is posted and verified; if the state read fails, the heading falsely says it was withdrawn and can prompt an unnecessary queue removal.

Logic error · scripts/lib/merge-queue-dequeue.sh:132-134

[codeant-ai]

CodeAnt PR Risk: Low Risk

• The PR appears safe to merge; failed live-state reads are retried, and exhausted retries avoid dequeueing or commenting while surfacing a warning.
• Tests cover recovery after a transient failure and bounded failure handling, including the step summary.

Assessed commit: 9d62c850f7d5

[don-petry]

dev-lead is withholding action on this item.

It is labeled dev-lead:hands-off (deliberately kept off-limits to dev-lead (e.g. changes to the dev-lead automation itself)), so dev-lead will not pick it up while that label is present. This notice is posted once so the withhold is visible rather than looking like a stalled run.

To re-enable automated pickup: remove the dev-lead:hands-off label.

[don-petry]

QA Lead — test-risk advisory

Risk tier: LOW. The change only adds to a guard on the approval path. The new tests are deterministic (MQ_READ_RETRY_DELAY_SEC=0, a counter-file mock, no real sleeps), and the mutation check shows they catch a single-attempt loop. The "exactly N reads" assertion also catches an off-by-one in the loop bound.

What I'd shore up (highest leverage first):

• The malformed-delay case is never tested. The third test sets MQ_READ_RETRY_DELAY_SEC=-1 and then sets it to 0 on the next line, so the -1 value is never read. Only MQ_READ_ATTEMPTS=abc is exercised. Put a stub sleep on PATH in setup() that logs its argument and returns at once (sleep is an external binary, so the stub will be used). Then assert that -1 falls back to 2 and 0 means no sleep, with no wall-clock cost. Also add MQ_READ_ATTEMPTS=0, which the regex sends to the default.
• Add the negative check on the recovery path. In the "fails once then succeeds" test, set GITHUB_STEP_SUMMARY and assert that it stays empty and that no ::warning:: is printed. A recovered read should leave no trace in the summary.
• Cover the summary write failing safely. Point GITHUB_STEP_SUMMARY at an unwritable path, and also leave it unset, with every read failing under set -euo pipefail. Assert exit 0 and that the caller keeps running. The existing set -e test only covers a failed dequeue.
• Not a test gap, but worth knowing: the tests prove the loop works, not that 3 tries 2 s apart outlast a real secondary rate limit. How long to wait is a design call for the dev-lead persona.

Escalate? no

Advisory only — I comment, I do not change code. Ground: BMAD Test Architecture
(bmad-tea v1.19.0). Opt out on this item with the qa-lead:hands-off label.

[donpetry-bot]

Automated review — APPROVED ✓

Risk: LOW
Reviewed commit: 11fe0e9361d9cf737c38b5527256196236f6d997
Review mode: triage-approved (single reviewer)

Summary

Adds a bounded retry (MQ_READ_ATTEMPTS, default 3; MQ_READ_RETRY_DELAY_SEC, default 2) around the live merge-queue/review state read in mq_dequeue_if_unapproved. When every attempt fails, it also writes a step-summary entry naming the PR. It still makes no dequeue call, posts no PR comment, and returns 0. AGENTS.md documents the change, and 3 bats cases are added. The triage-approved LOW assessment holds: the change is confined to one library function on a best-effort path, and the existing tests are unchanged.

Linked issue analysis

Closes #2186. All three scope items are met: a bounded retry with backoff, a step-summary entry on total failure, and no PR comment on that path. All three acceptance criteria are covered by tests:

1. Fail once then succeed → dequeued, with one marker comment.
2. Every read fails → no dequeue, no comment, a warning, a summary entry, exit 0.
3. The tunables can be overridden in tests (MQ_READ_RETRY_DELAY_SEC=0 in setup), and the 24 existing tests are untouched.

Findings

No blocking findings.

• Resolved thread (Gemini, set -e with [ "$delay" -gt 0 ] && sleep): I agree with the author's rebuttal. A failing left operand of && is exempt from errexit, and the call site is st=$(...) || status_st=$?, which disables errexit inside anyway. The existing "under set -e the helper never aborts its caller" test also covers this.
• Nit (non-blocking): The malformed-setting test sets MQ_READ_RETRY_DELAY_SEC=-1 and then immediately overrides it to 0, so only the malformed MQ_READ_ATTEMPTS fallback is tested. The delay-regex fallback is untested. This is acceptable because exercising it would mean a real 2s sleep.
• Nit (non-blocking, CodeAnt #2): post-pr-review.sh:532 also calls the helper right after a new approval is posted. The cleanup did dismiss a prior approval first, so the summary heading is accurate, but on that path the wording "may merge without an approval" is overly alarming. It only matters when every read fails.
• Nit (non-blocking, CodeAnt #1): The tunables have no upper bound. They are set only by the operator or tests, so this is low concern.
• Secret-scan MCP tool not available in this run; gitleaks CI check passed.

CI status

All required checks are green: bats, unit-tests, shellcheck/ShellCheck, Lint, CodeQL, SonarCloud (Quality Gate passed), gitleaks, actionlint, duplicate-decl-gate, AgentShield, and all stub/standards guards. The cancelled dev-lead / * runs are expected under the dev-lead:hands-off label. The queued pr-auto-review run is this review.

---

Reviewed automatically by the PR-review agent (single-reviewer mode: opus 5.5 [opus 4.8, opus 4.7]). Reply if you need a human review.

[donpetry-bot]

pr-review approved on PARTIAL advisory evidence: 3/5 required advisory bots reported before the gate's head-age-timeout fallback proceeded. Recorded for the miss-rate metric (#1596).

Dismissing approval due to a PR issue comment lacking a verified disposition (#1813)

[donpetry-bot]

Removed from the merge queue: approval withdrawn

The automated review withdrew its approval while this PR was in the merge queue, and no other approval stands for the current head commit 11fe0e9361d9cf737c38b5527256196236f6d997. GitHub checks reviews only when a PR joins the queue, not when the queue merges it, so the PR was removed from the queue to stop it merging without an approval.

What happens next: the PR needs an approval for its head commit again. Address the review findings, or a maintainer approves it. Once an approval stands, dev-lead re-enables auto-merge on its next pass and the PR rejoins the queue; a maintainer can also re-add it by hand.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud