Go-live improvements for PR review agent#2
Merged
Conversation
- Upgrade actions/checkout v4→v5 (Node.js 20 deprecation) - Increase job timeout 30→60 min - Add MAX_PRS cap (default 10) so cron doesn't timeout on large backlogs - Filter drafts at enumeration time (--draft=false) instead of only at review time — saves unnecessary council invocations - Remove unused ME variable from list-prs.sh - Fix idempotency jq query: use // empty fallback for PRs with zero reviews or comments (previously errored on empty array iteration) - Update AGENT.md tuning docs to reflect shared.md, council prompts, MAX_PRS, models, and Max plan billing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Improves the reliability and operability of the scheduled PR review agent by reducing unnecessary work (skipping drafts), keeping cron runs within timeout via a per-run cap, and making idempotency detection more robust.
Changes:
- Make idempotency marker detection tolerant of PRs with zero reviews/comments.
- Exclude draft PRs during enumeration and cap per-run PR processing via
MAX_PRS. - Update workflow runtime settings and refresh docs to match the council architecture.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
scripts/review-one-pr.sh |
Hardens the jq query so marker scanning won’t fail when reviews/comments are absent. |
scripts/list-prs.sh |
Filters out draft PRs at query time to avoid wasted review work. |
AGENT.md |
Updates tuning/cost documentation to reflect council prompts and the new MAX_PRS knob. |
.github/workflows/pr-review.yml |
Increases job timeout, bumps checkout action version, and caps PRs processed per run using MAX_PRS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| echo "${PR_URL_OVERRIDE}" > prs.txt | ||
| else | ||
| bash scripts/list-prs.sh > prs.txt | ||
| bash scripts/list-prs.sh | head -n "$MAX_PRS" > prs.txt |
There was a problem hiding this comment.
MAX_PRS comes from a repo variable and is passed directly to head -n. If it’s set to a non-integer (e.g., empty/whitespace/"ten"), head will exit non‑zero and (with set -euo pipefail) the workflow will fail. Consider sanitizing/validating MAX_PRS (e.g., ensure it matches ^[0-9]+$ and default to 10 otherwise) before using it, or clamp it to a safe range.
Major: - Fix jq operator-precedence bug in idempotency check. The comma operator has higher precedence than pipe in jq, so the previous expression silently failed (Cannot index string with body), making EXISTING_MARKER_SHA always empty. Fixed by concatenating arrays first: ((.reviews // []) + (.comments // [])) | .[].body Minor/Info: - Add log notice when MAX_PRS truncates the backlog - Add inline comment explaining the jq null guards - Remove specific model IDs from AGENT.md (point to script instead) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
don-petry
pushed a commit
that referenced
this pull request
Apr 21, 2026
Major: - Fix jq operator-precedence bug in idempotency check. The comma operator has higher precedence than pipe in jq, so the previous expression silently failed (Cannot index string with body), making EXISTING_MARKER_SHA always empty. Fixed by concatenating arrays first: ((.reviews // []) + (.comments // [])) | .[].body Minor/Info: - Add log notice when MAX_PRS truncates the backlog - Add inline comment explaining the jq null guards - Remove specific model IDs from AGENT.md (point to script instead) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced May 7, 2026
3 tasks
This was referenced May 16, 2026
don-petry
added a commit
that referenced
this pull request
May 17, 2026
- Upgrade actions/checkout v4→v5 (Node.js 20 deprecation) - Add MAX_PRS cap (default 10) to stay within 60-min job timeout - Filter drafts at enumeration (--draft=false) - Fix jq operator-precedence bug in idempotency check - Add truncation notice log, inline jq comments, docs updates Council-reviewed: 2 rounds (escalated round 1 on jq bug, approved round 2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
May 17, 2026
- Upgrade actions/checkout v4→v5 (Node.js 20 deprecation) - Add MAX_PRS cap (default 10) to stay within 60-min job timeout - Filter drafts at enumeration (--draft=false) - Fix jq operator-precedence bug in idempotency check - Add truncation notice log, inline jq comments, docs updates Council-reviewed: 2 rounds (escalated round 1 on jq bug, approved round 2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced May 17, 2026
don-petry
added a commit
that referenced
this pull request
Jun 7, 2026
Address all findings from pr-review agent automated review (cycle 1/3): **CRITICAL FIX #1: Early-exit logic in Tier 1 & 2 loops** Problem: Loops only broke on timeout, always blocking full 60 minutes Solution: Added all_bots_submitted() check with early return(0) when all participating bots submit their reviews Impact: PR reviews now complete in seconds/minutes instead of 60 min **FIX #2: Improved BATS test coverage** Problem: Tests accepted any result (0 or 2), missing early-exit validation Solution: Updated tests to validate early-exit behavior and new logic Coverage: Now validates all helper functions and exit conditions **FIX #3: Subshell isolation in review-one-pr.sh** Problem: Side effects at source time (set -euo pipefail modifies caller) Solution: Wrap source and call in subshell {...} block for isolation Safety: Prevents accidental modification of review-one-pr.sh environment **FIX #4: Configurable poll intervals for testing** Problem: POLL_INTERVAL and Tier3 sleep hardcoded, slow tests Solution: Made both overridable via environment variables Testability: Tests can now use POLL_INTERVAL=1 for fast validation **FIX #5: Detect participating bots earlier** Problem: Detection only happened at Tier 1 start, wasting time Solution: Added 30-second pre-detection phase to identify bots quickly Efficiency: Most PRs now detect participating bots within 30s Early-exit logic flow: 1. Detect participating bots (30s max) 2. Tier 1: Poll every 10s, return 0 when all participating bots submit 3. Tier 2: If still waiting, continue with 10s polling 4. Tier 3: Hard timeout at 60m with warning, return 2 Addresses: pr-review agent findings (cycle 1/3) Fixes: Issue #457 correctness gap Tests: 25+ BATS test cases covering all code paths
don-petry
added a commit
that referenced
this pull request
Jun 7, 2026
- Upgrade actions/checkout v4→v5 (Node.js 20 deprecation) - Add MAX_PRS cap (default 10) to stay within 60-min job timeout - Filter drafts at enumeration (--draft=false) - Fix jq operator-precedence bug in idempotency check - Add truncation notice log, inline jq comments, docs updates Council-reviewed: 2 rounds (escalated round 1 on jq bug, approved round 2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Jun 7, 2026
- Upgrade actions/checkout v4→v5 (Node.js 20 deprecation) - Add MAX_PRS cap (default 10) to stay within 60-min job timeout - Filter drafts at enumeration (--draft=false) - Fix jq operator-precedence bug in idempotency check - Add truncation notice log, inline jq comments, docs updates Council-reviewed: 2 rounds (escalated round 1 on jq bug, approved round 2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
don-petry
added a commit
that referenced
this pull request
Jun 7, 2026
- Upgrade actions/checkout v4→v5 (Node.js 20 deprecation) - Add MAX_PRS cap (default 10) to stay within 60-min job timeout - Filter drafts at enumeration (--draft=false) - Fix jq operator-precedence bug in idempotency check - Add truncation notice log, inline jq comments, docs updates Council-reviewed: 2 rounds (escalated round 1 on jq bug, approved round 2) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Jul 4, 2026
[Phase 2] Assess simplifying the sonnet->opus in-engine fallback given rate-limit equalization
#1099
Closed
Closed
Closed
Open
This was referenced Jul 10, 2026
This was referenced Jul 14, 2026
feat: implement issue #1147 — [Phase 2] Multi-repo initiative coordination — design-only spike
#1222
Merged
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
actions/checkoutv4→v5 (Node.js 20 deprecation fix)MAX_PRScap (default 10) so hourly cron stays within timeout on large backlogs--draft=false) to avoid wasting council invocationsTest plan
dry_run=trueagainst this PR (dogfood)Closes: go-live readiness for the PR review agent
🤖 Generated with Claude Code