fix(pr-review): count only non-converging review cycles; make escalation recoverable - #468
Merged
Merged
Conversation
…overable (#467) The MAX_REVIEW_CYCLES circuit breaker had three defects (issue #467, observed on PR #458): 1. It counted every review marker ever posted — approvals included — so a PR that converged (approved) and then legitimately evolved burned its budget and escalated with a false "without converging" message. 2. Mention-triggered runs (FORCE_REVIEW=true) acked "starting a fresh review" and then silently no-op'd at the escalation marker / cap. 3. The escalation comment promised "remove the needs-human-review label to re-engage", but the no-op check only grepped for the escalation comment marker, which nothing ever removes — a capped PR was permanently un-reviewable. Fixes: - New scripts/lib/review-cycle.sh: compute_review_cycle counts only non-approval markers newer than the latest reset event (latest approval marker or latest escalation comment). Approval ⇒ fresh budget; re-engagement after escalation ⇒ fresh budget. - Escalation pause now requires the marker AND the needs-human-review label, so removing the label re-engages the cascade as documented. - FORCE_REVIEW bypasses both the escalation pause and the cycle cap, and drops the label so the cascade stays engaged on later scheduled runs. - Escalation comment text updated to describe the real re-engagement paths. - 17 bats tests covering reset semantics, #458 regression shape, superseded wrappers, null timestamps, and malformed input; wired into lint.yml. Closes #467 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 8 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Collaborator
Author
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
don-petry
enabled auto-merge (squash)
June 7, 2026 22:47
Collaborator
Author
|
@donpetry-bot review |
Contributor
|
@don-petry I'm on it — starting a fresh review now. Results will appear in a few minutes. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a robust cycle-cap mechanism to prevent infinite review loops on non-converging pull requests. It extracts helper functions into a new scripts/lib/review-cycle.sh script, integrates them into scripts/review-one-pr.sh, updates the documentation, and adds comprehensive unit tests. The review feedback focuses on optimizing GitHub API performance by fetching comments in the initial PR snapshot to avoid redundant network requests, and improving the robustness of the jq label check by using the more idiomatic any function.
don-petry
disabled auto-merge
June 7, 2026 22:49
…label check Review feedback (Gemini, PR #468): - fetch comments in the initial gh pr view snapshot and build PR_ITEMS from it, saving a second API round-trip per PR - replace the index()-based label check (relies on 0 being truthy in jq) with any(. == "needs-human-review"), which returns a real boolean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
donpetry-bot
approved these changes
Jun 7, 2026
donpetry-bot
left a comment
donpetry-bot
left a comment
Contributor
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: LOW
Reviewed commit: 73db8807a57e323126e50a668a6f340472e3f48d
Review mode: triage-approved (single reviewer)
Summary
Fixes the three cycle-cap defects documented in issue #467. compute_review_cycle (new scripts/lib/review-cycle.sh) counts only non-approval <!-- pr-review-agent v1 sha=... --> markers newer than the latest reset event (latest approval marker or latest <!-- pr-review-agent escalation --> comment) — so an approval or a human re-engagement grants a fresh budget instead of permanently disengaging the cascade. FORCE_REVIEW=true now bypasses both the escalation pause and the cap and drops the needs-human-review label. The escalation pause now requires the marker AND the label, making the documented "remove the label to re-engage" path actually work.
Linked issue analysis
Issue #467 enumerates three defects on PR #458; the diff addresses each one directly:
| Defect (from #467) | Fix in this PR |
|---|---|
| Lifetime counter; approvals counted against the cap | compute_review_cycle skips approval markers and resets at the latest approval/escalation |
@bot review acked then silently no-op'd at the cap |
FORCE_REVIEW=true bypasses the escalation pause and the cycle cap; also drops the label |
| "Remove the label to re-engage" was false | Escalation pause now requires the marker AND the label; pre-escalation markers don't count toward the next cap |
The genuine reviewer↔dev-lead ping-pong loop the breaker exists for still escalates after MAX_REVIEW_CYCLES consecutive non-converging cycles, which is the intended behavior.
Findings
None blocking.
Minor (non-blocking, optional follow-up):
- Gemini suggested folding the second
gh pr view(forreviews,comments) into the initialPR_SNAPSHOTto save a network round-trip and usingjq any(...)for theneeds-human-reviewlabel check. These are micro-optimizations / style preferences, not correctness issues — feel free to address in a follow-up or ignore.
Things done well:
compute_review_cycledegrades safely to0on malformed input (the same defensiveness the prior commit9ede0cahad to retrofit on the oldgrep -c | echo 0path), preventing"integer expression expected"regressions.DRY_RUNis honored before thegh pr edit --remove-labelcall.- Test suite includes the literal #458 marker sequence (fix, fix, approve → expected count 0), the superseded
<details>-wrapped approval case, nullsubmittedAt(PENDING reviews), and garbage-input safety — high confidence the regression is locked down.
CI status
All required checks green: Lint, ShellCheck, bats (including the new tests/test_review_cycle.bats wired into lint.yml), unit-tests, CodeQL, AgentShield, Agent Security Scan, gitleaks, SonarCloud, CodeRabbit, Dev-Lead dispatch. CodeRabbit hit its rate limit and posted a no-content comment but the check itself reports SUCCESS.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Collaborator
Author
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the PR-review cascade’s cycle-cap logic so it escalates only after consecutive non-converging (non-approval) cycles, and makes post-escalation re-engagement possible (label removal or mention-triggered FORCE_REVIEW).
Changes:
- Add
scripts/lib/review-cycle.shto compute the non-converging cycle count and detect escalation markers. - Update
scripts/review-one-pr.shto use the new cycle computation and make escalation pause recoverable (FORCE_REVIEW bypass + label-gated pause). - Add a dedicated Bats test suite for the new cycle semantics and wire it into
lint.yml; update docs accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
scripts/lib/review-cycle.sh |
New helper functions for computing consecutive non-approval cycles and detecting escalation markers. |
scripts/review-one-pr.sh |
Switch cycle counting to non-converging cycles; make escalation pause recoverable and FORCE_REVIEW-capable. |
tests/test_review_cycle.bats |
New unit tests covering approval/escalation reset semantics and regressions (incl. #458 shape). |
docs/pr-review-agent/pr-review-agent.md |
Document the updated cycle guard + re-engagement behavior. |
.github/workflows/lint.yml |
Run the new Bats test file in CI. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73db8807a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Collaborator
Author
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
don-petry
added a commit
that referenced
this pull request
Jun 12, 2026
…ion recoverable (#468) * fix(pr-review): count only non-converging cycles; make escalation recoverable (#467) The MAX_REVIEW_CYCLES circuit breaker had three defects (issue #467, observed on PR #458): 1. It counted every review marker ever posted — approvals included — so a PR that converged (approved) and then legitimately evolved burned its budget and escalated with a false "without converging" message. 2. Mention-triggered runs (FORCE_REVIEW=true) acked "starting a fresh review" and then silently no-op'd at the escalation marker / cap. 3. The escalation comment promised "remove the needs-human-review label to re-engage", but the no-op check only grepped for the escalation comment marker, which nothing ever removes — a capped PR was permanently un-reviewable. Fixes: - New scripts/lib/review-cycle.sh: compute_review_cycle counts only non-approval markers newer than the latest reset event (latest approval marker or latest escalation comment). Approval ⇒ fresh budget; re-engagement after escalation ⇒ fresh budget. - Escalation pause now requires the marker AND the needs-human-review label, so removing the label re-engages the cascade as documented. - FORCE_REVIEW bypasses both the escalation pause and the cycle cap, and drops the label so the cascade stays engaged on later scheduled runs. - Escalation comment text updated to describe the real re-engagement paths. - 17 bats tests covering reset semantics, #458 regression shape, superseded wrappers, null timestamps, and malformed input; wired into lint.yml. Closes #467 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(pr-review): reuse PR snapshot for cycle count; idiomatic jq label check Review feedback (Gemini, PR #468): - fetch comments in the initial gh pr view snapshot and build PR_ITEMS from it, saving a second API round-trip per PR - replace the index()-based label check (relies on 0 being truthy in jq) with any(. == "needs-human-review"), which returns a real boolean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Jun 14, 2026
…ion recoverable (#468) * fix(pr-review): count only non-converging cycles; make escalation recoverable (#467) The MAX_REVIEW_CYCLES circuit breaker had three defects (issue #467, observed on PR #458): 1. It counted every review marker ever posted — approvals included — so a PR that converged (approved) and then legitimately evolved burned its budget and escalated with a false "without converging" message. 2. Mention-triggered runs (FORCE_REVIEW=true) acked "starting a fresh review" and then silently no-op'd at the escalation marker / cap. 3. The escalation comment promised "remove the needs-human-review label to re-engage", but the no-op check only grepped for the escalation comment marker, which nothing ever removes — a capped PR was permanently un-reviewable. Fixes: - New scripts/lib/review-cycle.sh: compute_review_cycle counts only non-approval markers newer than the latest reset event (latest approval marker or latest escalation comment). Approval ⇒ fresh budget; re-engagement after escalation ⇒ fresh budget. - Escalation pause now requires the marker AND the needs-human-review label, so removing the label re-engages the cascade as documented. - FORCE_REVIEW bypasses both the escalation pause and the cycle cap, and drops the label so the cascade stays engaged on later scheduled runs. - Escalation comment text updated to describe the real re-engagement paths. - 17 bats tests covering reset semantics, #458 regression shape, superseded wrappers, null timestamps, and malformed input; wired into lint.yml. Closes #467 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(pr-review): reuse PR snapshot for cycle count; idiomatic jq label check Review feedback (Gemini, PR #468): - fetch comments in the initial gh pr view snapshot and build PR_ITEMS from it, saving a second API round-trip per PR - replace the index()-based label check (relies on 0 being truthy in jq) with any(. == "needs-human-review"), which returns a real boolean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Jun 18, 2026
…ion recoverable (#468) * fix(pr-review): count only non-converging cycles; make escalation recoverable (#467) The MAX_REVIEW_CYCLES circuit breaker had three defects (issue #467, observed on PR #458): 1. It counted every review marker ever posted — approvals included — so a PR that converged (approved) and then legitimately evolved burned its budget and escalated with a false "without converging" message. 2. Mention-triggered runs (FORCE_REVIEW=true) acked "starting a fresh review" and then silently no-op'd at the escalation marker / cap. 3. The escalation comment promised "remove the needs-human-review label to re-engage", but the no-op check only grepped for the escalation comment marker, which nothing ever removes — a capped PR was permanently un-reviewable. Fixes: - New scripts/lib/review-cycle.sh: compute_review_cycle counts only non-approval markers newer than the latest reset event (latest approval marker or latest escalation comment). Approval ⇒ fresh budget; re-engagement after escalation ⇒ fresh budget. - Escalation pause now requires the marker AND the needs-human-review label, so removing the label re-engages the cascade as documented. - FORCE_REVIEW bypasses both the escalation pause and the cycle cap, and drops the label so the cascade stays engaged on later scheduled runs. - Escalation comment text updated to describe the real re-engagement paths. - 17 bats tests covering reset semantics, #458 regression shape, superseded wrappers, null timestamps, and malformed input; wired into lint.yml. Closes #467 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(pr-review): reuse PR snapshot for cycle count; idiomatic jq label check Review feedback (Gemini, PR #468): - fetch comments in the initial gh pr view snapshot and build PR_ITEMS from it, saving a second API round-trip per PR - replace the index()-based label check (relies on 0 being truthy in jq) with any(. == "needs-human-review"), which returns a real boolean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 21, 2026
Closed
don-petry
added a commit
that referenced
this pull request
Jun 25, 2026
…ozen @v2) (#919) * chore(auto-rebase): pin caller to @auto-rebase/stable channel (was @v2) This repo's auto-rebase caller pinned the frozen `@v2` tag (376a4fcb, 2026-05-19), which predates the review-ready eligibility gate (#465/#468) and violates the org standard (callers MUST pin the centrally-advanced `auto-rebase/stable` channel, never a frozen `@vX` — see standards/ci-standards.md → Reusable workflow versioning). Re-syncs the caller stub from standards/workflows/auto-rebase.yml: - uses: ...auto-rebase-reusable.yml@v2 → @auto-rebase/stable - corrected "MUST NOT change the uses ref" header guidance - documents the eligibility / ready_label inputs Effect: once the `auto-rebase/stable` channel is promoted to a commit containing #528 (the tooling_ref fix) + #468 (the gate), this repo automatically adopts the review-ready restriction. Until then it runs the current channel head (unchanged unrestricted behavior) — no broken state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LUUSUQHqwLWZ583SAs41K * chore: apply manual instructions [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
don-petry
added a commit
that referenced
this pull request
Jun 25, 2026
…ion recoverable (#468) * fix(pr-review): count only non-converging cycles; make escalation recoverable (#467) The MAX_REVIEW_CYCLES circuit breaker had three defects (issue #467, observed on PR #458): 1. It counted every review marker ever posted — approvals included — so a PR that converged (approved) and then legitimately evolved burned its budget and escalated with a false "without converging" message. 2. Mention-triggered runs (FORCE_REVIEW=true) acked "starting a fresh review" and then silently no-op'd at the escalation marker / cap. 3. The escalation comment promised "remove the needs-human-review label to re-engage", but the no-op check only grepped for the escalation comment marker, which nothing ever removes — a capped PR was permanently un-reviewable. Fixes: - New scripts/lib/review-cycle.sh: compute_review_cycle counts only non-approval markers newer than the latest reset event (latest approval marker or latest escalation comment). Approval ⇒ fresh budget; re-engagement after escalation ⇒ fresh budget. - Escalation pause now requires the marker AND the needs-human-review label, so removing the label re-engages the cascade as documented. - FORCE_REVIEW bypasses both the escalation pause and the cycle cap, and drops the label so the cascade stays engaged on later scheduled runs. - Escalation comment text updated to describe the real re-engagement paths. - 17 bats tests covering reset semantics, #458 regression shape, superseded wrappers, null timestamps, and malformed input; wired into lint.yml. Closes #467 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(pr-review): reuse PR snapshot for cycle count; idiomatic jq label check Review feedback (Gemini, PR #468): - fetch comments in the initial gh pr view snapshot and build PR_ITEMS from it, saving a second API round-trip per PR - replace the index()-based label check (relies on 0 being truthy in jq) with any(. == "needs-human-review"), which returns a real boolean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Jun 25, 2026
…ion recoverable (#468) * fix(pr-review): count only non-converging cycles; make escalation recoverable (#467) The MAX_REVIEW_CYCLES circuit breaker had three defects (issue #467, observed on PR #458): 1. It counted every review marker ever posted — approvals included — so a PR that converged (approved) and then legitimately evolved burned its budget and escalated with a false "without converging" message. 2. Mention-triggered runs (FORCE_REVIEW=true) acked "starting a fresh review" and then silently no-op'd at the escalation marker / cap. 3. The escalation comment promised "remove the needs-human-review label to re-engage", but the no-op check only grepped for the escalation comment marker, which nothing ever removes — a capped PR was permanently un-reviewable. Fixes: - New scripts/lib/review-cycle.sh: compute_review_cycle counts only non-approval markers newer than the latest reset event (latest approval marker or latest escalation comment). Approval ⇒ fresh budget; re-engagement after escalation ⇒ fresh budget. - Escalation pause now requires the marker AND the needs-human-review label, so removing the label re-engages the cascade as documented. - FORCE_REVIEW bypasses both the escalation pause and the cycle cap, and drops the label so the cascade stays engaged on later scheduled runs. - Escalation comment text updated to describe the real re-engagement paths. - 17 bats tests covering reset semantics, #458 regression shape, superseded wrappers, null timestamps, and malformed input; wired into lint.yml. Closes #467 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(pr-review): reuse PR snapshot for cycle count; idiomatic jq label check Review feedback (Gemini, PR #468): - fetch comments in the initial gh pr view snapshot and build PR_ITEMS from it, saving a second API round-trip per PR - replace the index()-based label check (relies on 0 being truthy in jq) with any(. == "needs-human-review"), which returns a real boolean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Jun 27, 2026
…ive (#736) (#953) * docs(initiatives): record review-ready gate now deployed & verified live The §2 ≥50% verdict was computed from the eligible-PR multiplier (a predicate snapshot). At authoring time the restriction was not actually filtering in production: the central reusable defaulted tooling_ref to v1 (predates eligibility.sh from #468), so auto-rebase failed to source the predicate and ran the original unrestricted fan-out everywhere. Fixed in petry-projects/.github#528 (job_workflow_sha) on 2026-06-24, then the auto-rebase/stable channel was promoted org-wide. Gate now verified live (.github-private PR#744 went from auto-updated to skipped). Adds a dated §2 update note + Status-line tag. The free mitigation behind the "defer Merge Queue" decision is now genuinely in effect, so the §4 deferral holds on stronger footing. Docs-only; no decision change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LUUSUQHqwLWZ583SAs41K * fix(reviews): address review comments [skip ci-relay] --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: donpetry-bot <281750570+donpetry-bot@users.noreply.github.com>
This was referenced Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #467 — the PR-review cascade's cycle cap escalated converging PRs (e.g. #458) and, once escalated, permanently disengaged.
Defects fixed
compute_review_cycle(newscripts/lib/review-cycle.sh) counts only non-approval markers newer than the latest reset event (latest approval marker or latest escalation comment)@bot reviewmentions acked "starting fresh review" then silently no-op'd at the capFORCE_REVIEW=truebypasses the escalation pause and the cycle cap, and drops theneeds-human-reviewlabel so the cascade stays engagedWhat still quits (by design)
The cap still escalates after
MAX_REVIEW_CYCLES(default 3) consecutive non-converging fix-request/escalated cycles — the genuine reviewer↔dev-lead ping-pong loop the breaker exists for.Tests
tests/test_review_cycle.bats(17 cases, wired into the lint.yml bats job):<details>wrapper still resetshas_escalation_markerpositive/negative/garbageLocal verification: all 87 bats tests pass (full lint.yml list),
shellcheck --severity=warning -xclean acrossscripts/**/*.sh.🤖 Generated with Claude Code