Repository navigation
feat: implement advisory bot review gate in pr-review agent #458
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 3 commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
d620f1c
feat: implement advisory bot review gate in pr-review agent
5d6b94d
chore: apply manual instructions [skip ci-relay]
donpetry-bot 05ae5d9
fix: resolve critical issues in advisory-review-gate implementation
don-petry a46a7c4
fix: implement proper early-exit logic in advisory gate (critical)
don-petry 0655052
fix: resolve ShellCheck warnings (SC2034 - unused variables)
don-petry 9f5f112
Merge branch 'main' into advisory-bot-review-gate
don-petry 6b146e6
refactor: convert advisory-review-gate to non-blocking instant check
7cfb6c4
docs: enhance re-trigger pattern documentation and observability
022cecf
Merge branch 'main' into advisory-bot-review-gate
don-petry c818b59
fix(reviews): address review comments [skip ci-relay]
donpetry-bot c54d76a
Merge branch 'main' into advisory-bot-review-gate
don-petry 02b1a19
Merge branch 'main' into advisory-bot-review-gate
don-petry 83a00e9
chore: apply manual instructions [skip ci-relay]
donpetry-bot 3a4ffba
chore: apply manual instructions [skip ci-relay]
donpetry-bot 8718a53
fix(reviews): address review comments [skip ci-relay]
donpetry-bot 2f24990
chore: apply manual instructions [skip ci-relay]
donpetry-bot def1bde
fix(reviews): address review comments [skip ci-relay]
donpetry-bot 6766ad6
Merge branch 'main' into advisory-bot-review-gate
don-petry 6832ced
chore: apply manual instructions [skip ci-relay]
donpetry-bot 5db826d
Merge branch 'main' into advisory-bot-review-gate
don-petry f21afb2
chore: apply manual instructions [skip ci-relay]
donpetry-bot 9e7b4b2
fix: resolve Codex review findings - timestamp parsing and variable i…
3306976
fix(reviews): address review comments [skip ci-relay]
donpetry-bot 3b75f29
Merge branch 'main' into advisory-bot-review-gate
don-petry 2e949a9
Merge branch 'main' into advisory-bot-review-gate
don-petry 7602ebf
Merge branch 'main' into advisory-bot-review-gate
don-petry 24e4d1b
Merge branch 'main' into advisory-bot-review-gate
don-petry 8f35865
fix(pr-review): repair quiescence timestamp jq; feed advisory finding…
cb9d399
fix(pr-review): match [bot]-suffixed REST logins; slurp paginated com…
0d56d14
Merge branch 'main' into advisory-bot-review-gate
don-petry 7ff649b
Merge branch 'main' into advisory-bot-review-gate
don-petry 3843aa3
fix(pr-review): head-bound, supersede-aware advisory feedback incl. P…
1c9fa4d
Merge branch 'main' into advisory-bot-review-gate
don-petry 6cc9ff3
Merge branch 'main' into advisory-bot-review-gate
don-petry 061d288
Merge branch 'main' into advisory-bot-review-gate
don-petry 8cdab05
Merge branch 'main' into advisory-bot-review-gate
don-petry 544c3a5
Merge branch 'main' into advisory-bot-review-gate
don-petry 79df085
Merge branch 'main' into advisory-bot-review-gate
don-petry 1ade270
Merge branch 'main' into advisory-bot-review-gate
don-petry c04759a
Merge branch 'main' into advisory-bot-review-gate
don-petry 6fe4a39
Merge branch 'main' into advisory-bot-review-gate
don-petry ff5545d
Merge branch 'main' into advisory-bot-review-gate
don-petry d95ace2
chore: apply manual instructions [skip ci-relay]
donpetry-bot c61600d
chore: apply manual instructions [skip ci-relay]
donpetry-bot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,300 @@ | ||
| # Advisory Bot Review Gate | ||
|
|
||
| ## Overview | ||
|
|
||
| The advisory bot review gate ensures that the pr-review agent waits for code review feedback from advisory bots before posting approval. This prevents the situation where approval is posted before valid review feedback arrives. | ||
|
|
||
| **Related**: Issue #457, PR #453 incident | ||
|
|
||
| ## Problem Context | ||
|
|
||
| ### The Incident (PR #453) | ||
|
|
||
| - **02:51:35** — pr-review agent posted APPROVED | ||
| - **02:52:18** — Copilot bot review arrived (43 seconds too late) with valid feedback | ||
| - **Result**: Valid review feedback was never incorporated because approval already posted | ||
|
|
||
| ### Root Cause | ||
|
|
||
| The pr-review workflow triggers on `check_suite` (completed) event and approves immediately after CI checks pass, without checking if advisory bots have submitted their reviews yet. | ||
|
|
||
| ## Participating Bots | ||
|
|
||
| The gate waits for reviews from these bots: | ||
|
|
||
| 1. **Gemini Code Assist** (`gemini-code-assist`) | ||
| - Latency: Median 50 seconds | ||
| - Participation: 80% of PRs | ||
| - Reliability: Excellent (100% success rate) | ||
| - Limitation: Cannot process `.yml`, `.yaml`, `.toml` files (returns UNSUPPORTED) | ||
|
|
||
| 2. **Copilot PR Reviewer** (`copilot-pull-request-reviewer`) | ||
| - Latency: Median 186 seconds (3.1 minutes) | ||
| - Participation: 80% of PRs | ||
| - Reliability: Excellent (100% success rate) | ||
| - No rate-limiting | ||
|
|
||
| 3. **SonarCloud** (`sonarqubecloud`) | ||
| - Latency: Median 794 seconds (13.2 minutes) | ||
| - Participation: 80% of PRs | ||
| - Reliability: Excellent (100% success rate) | ||
| - May have slow outliers (26+ hours) from high-volume periods | ||
|
|
||
| 4. **Codex** (`chatgpt-codex-connector`) | ||
| - Latency: Median 1,059 seconds (17.7 minutes) | ||
| - Participation: 44% of PRs (newer bot, not on all PRs yet) | ||
| - Reliability: Good (100% when triggered) | ||
| - No rate-limiting | ||
|
|
||
| ### CodeRabbit Status | ||
|
|
||
| ⚠️ **CodeRabbit does NOT approve** (recent fix). It's treated as optional since it no longer provides the approval decision. The gate does not wait for CodeRabbit submissions. | ||
|
|
||
| ## Wait Strategy | ||
|
|
||
| The gate uses a 3-tier timeout strategy: | ||
|
|
||
| ### Tier 1: 900 seconds (15 minutes) | ||
|
|
||
| **Target**: Catch 95% of advisory bot submissions | ||
|
|
||
| **Latency coverage**: | ||
| - ✓ Gemini (0.84 min median) | ||
| - ✓ Copilot (3.1 min median) | ||
| - ✓ SonarCloud (13.2 min median) | ||
| - ~ Codex (17.7 min median) — misses ~30% of Codex submissions | ||
|
|
||
| **Action**: If all participating bots have submitted by this time, proceed to approval. | ||
|
|
||
| ### Tier 2: 1,200 seconds (20 minutes) | ||
|
|
||
| **Target**: Catch remaining stragglers (e.g., slow Codex) | ||
|
|
||
| **Latency coverage**: | ||
| - ✓ All bots when they participate | ||
| - ✓ P95 percentile for all bots (including outliers) | ||
|
|
||
|
don-petry marked this conversation as resolved.
Outdated
|
||
| **Action**: If all participating bots have now submitted, proceed. | ||
|
|
||
| ### Tier 3: 3,600 seconds (60 minutes) | ||
|
|
||
| **Target**: Hard timeout before escalation | ||
|
|
||
| **Behavior**: | ||
| - Logs a warning | ||
| - Suggests checking CodeRabbit plan (rate-limit), PR complexity, or bot status | ||
| - Proceeds with approval anyway (we don't block indefinitely) | ||
| - Exit code: 2 (soft failure with warning) | ||
|
|
||
| ## Smart Detection | ||
|
|
||
| The gate intelligently handles bots that don't participate in every PR: | ||
|
|
||
| ### How It Works | ||
|
|
||
| 1. On startup, query the PR to see which bots have already submitted | ||
| 2. **Determine participation set**: Which bots have submitted so far? | ||
| 3. **Poll only for missing bots**: Don't wait forever for bots that never trigger | ||
| 4. **Timeout**: If a bot participated (submitted once), we wait for all subsequent ones | ||
|
|
||
| ### Examples | ||
|
|
||
| **Scenario A: Codex not triggered (44% of PRs)** | ||
| ``` | ||
| PR #450 created | ||
| ├─ Gemini submits @ 50s ✓ | ||
| ├─ Copilot submits @ 180s ✓ | ||
| ├─ SonarCloud submits @ 800s ✓ | ||
| └─ Codex never triggered (not on this PR) | ||
|
|
||
| Result: By T+15min, 3/3 participating bots submitted → APPROVE | ||
| ``` | ||
|
|
||
| **Scenario B: All 4 bots triggered** | ||
| ``` | ||
| PR #451 created | ||
| ├─ Gemini submits @ 50s ✓ | ||
| ├─ Copilot submits @ 180s ✓ | ||
| ├─ SonarCloud submits @ 800s ✓ | ||
| ├─ Codex submits @ 1000s (slow) ✓ | ||
|
|
||
| Result: By T+20min, 4/4 participating bots submitted → APPROVE | ||
| ``` | ||
|
|
||
| **Scenario C: High-volume period (no bots submitted yet at T+15min)** | ||
| ``` | ||
| PR #452 created (during compliance blitz) | ||
| ├─ T+900s: No bots submitted yet (unusual) | ||
| ├─ T+1200s: Still none | ||
| ├─ T+3600s: Hard timeout | ||
|
|
||
| Result: Escalate (check for CodeRabbit rate-limit, bot downtime) | ||
| Proceed anyway with warning | ||
| ``` | ||
|
|
||
| ## Implementation Details | ||
|
|
||
| ### File: `scripts/lib/advisory-review-gate.sh` | ||
|
|
||
| Main entry point: `wait_for_advisory_reviews <pr_url>` | ||
|
|
||
| **Functions**: | ||
| - `wait_for_advisory_reviews()` — Main wait loop | ||
| - `get_advisory_bot_states()` — Query PR reviews/comments for advisory bots | ||
| - `format_bot_status()` — Pretty-print bot review state | ||
| - `has_submitted()` — Check if bot has submitted | ||
|
|
||
| **Return codes**: | ||
| - `0` — Bots reviewed, proceeding to approval | ||
| - `2` — Hard timeout reached, proceeding anyway with warning | ||
|
|
||
| ### Integration: `scripts/review-one-pr.sh` | ||
|
|
||
| The gate is called after the CI gate passes but before approval logic: | ||
|
|
||
| ```bash | ||
| # CI gate passes... | ||
| # ↓ | ||
| # Advisory bot review gate (new) | ||
| source "$SCRIPT_DIR/lib/advisory-review-gate.sh" | ||
| wait_for_advisory_reviews "$PR_URL" | ||
| # ↓ | ||
| # Idempotency check... | ||
| # ↓ | ||
| # Approval (if no blockers) | ||
| ``` | ||
|
|
||
| ## Observability | ||
|
|
||
| ### Logging | ||
|
|
||
| The gate logs detailed information for debugging: | ||
|
|
||
| ``` | ||
| [advisory-gate] Starting advisory bot review gate for PR #450 | ||
| [advisory-gate] Detecting participating bots... | ||
| [advisory-gate] ✓ gemini-code-assist → COMMENTED (advisory) | ||
| [advisory-gate] ✓ copilot-pull-request-reviewer → COMMENTED (advisory) | ||
| [advisory-gate] ✓ sonarqubecloud → COMMENTED (advisory) | ||
| [advisory-gate] Tier 1 wait: 900s (15 min) - waiting for advisory bots... | ||
| [advisory-gate] Tier 1 timeout reached (900 seconds) | ||
| [advisory-gate] Final advisory bot status: | ||
| [advisory-gate] ✓ gemini-code-assist → COMMENTED (advisory) | ||
| [advisory-gate] ✓ copilot-pull-request-reviewer → COMMENTED (advisory) | ||
| [advisory-gate] ✓ sonarqubecloud → COMMENTED (advisory) | ||
| [advisory-gate] Advisory bot review gate passed ✓ | ||
| ``` | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| ### Metrics | ||
|
|
||
| - **Wait time per PR**: Logged and can be extracted for analytics | ||
| - **Bot participation**: Which bots triggered for which PRs | ||
| - **Timeout incidents**: PRs that hit Tier 3 hard timeout (should be <5%) | ||
|
|
||
| ## Historical Data | ||
|
|
||
| Analysis of 50 recent merged PRs (May 23 - June 7, 2026): | ||
|
|
||
| ### Latency Statistics (Clean Data) | ||
|
|
||
| Excluding: CodeRabbit rate-limits, PRs blocked by CodeRabbit | ||
|
|
||
| | Bot | Min | Median | P95 | Max | Samples | | ||
| |-----|-----|--------|-----|-----|---------| | ||
| | **Gemini** | 3s | 50s | 348s | 408s | 36 | | ||
| | **Copilot** | 52s | 186s | 444s | 558s | 36 | | ||
| | **SonarCloud** | 38s | 794s | 93,996s | 417,729s | 36 | | ||
| | **Codex** | 170s | 1,059s | 47,360s | 93,463s | 36 | | ||
|
|
||
| **Notes**: | ||
| - SonarCloud/Codex P95+ outliers from compliance blitz period (not typical) | ||
| - Real-world typical waits: 15-20 minutes to catch all bots | ||
| - CodeRabbit excluded: doesn't approve anymore | ||
|
|
||
| ## Configuration | ||
|
|
||
| ### Timeout Adjustment | ||
|
|
||
| If you need to adjust wait times, edit `scripts/lib/advisory-review-gate.sh`: | ||
|
|
||
| ```bash | ||
| TIER1_WAIT=900 # 15 min → Change here | ||
| TIER2_WAIT=1200 # 20 min → Change here | ||
| TIER3_WAIT=3600 # 60 min → Change here | ||
| POLL_INTERVAL=10 # Check every 10s → Change here | ||
| ``` | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| ### Bot Customization | ||
|
|
||
| To add/remove bots, edit the `ADVISORY_BOTS` map: | ||
|
|
||
| ```bash | ||
| declare -A ADVISORY_BOTS=( | ||
| [gemini-code-assist]="Gemini Code Assist (advisory)" | ||
| [copilot-pull-request-reviewer]="Copilot PR Reviewer (advisory)" | ||
| [sonarqubecloud]="SonarCloud (advisory)" | ||
| [chatgpt-codex-connector]="Codex (advisory, newer bot)" | ||
| # Add new bots here | ||
| ) | ||
| ``` | ||
|
|
||
| ## Testing | ||
|
|
||
| Run the advisory gate tests: | ||
|
|
||
| ```bash | ||
| bats tests/dev-lead/unit/test_advisory_review_gate.bats | ||
| ``` | ||
|
|
||
| Test scenarios: | ||
| - All 4 bots present → success | ||
| - 3 bots present (no Codex) → success | ||
| - Slow Codex (Tier 2 wait) → success | ||
| - No bots submitted → hard timeout warning | ||
| - CHANGES_REQUESTED state → noted but doesn't block | ||
|
|
||
| ## Troubleshooting | ||
|
|
||
| ### "Hard timeout reached at 3600 seconds" | ||
|
|
||
| This happens when advisory bots don't submit within 60 minutes. Common causes: | ||
|
|
||
| 1. **CodeRabbit rate-limited** (most common) | ||
| - Check CodeRabbit quota at https://coderabbit.ai/settings/subscription | ||
| - Upgrade plan if exhausted | ||
| - Solution: Upgrade or throttle PR creation | ||
|
|
||
| 2. **PR had unusual complexity** | ||
| - Very large PRs sometimes trigger slow reviews | ||
| - Check PR file count, line changes | ||
| - Solution: Normal, monitor for pattern | ||
|
|
||
| 3. **Bot service issues** | ||
| - Gemini, Copilot, SonarCloud, Codex may be experiencing downtime | ||
| - Check official status pages | ||
| - Solution: Wait for service recovery or contact vendor | ||
|
|
||
| ### "No advisory bots have submitted by Tier 2 timeout" | ||
|
|
||
| This is unusual and suggests: | ||
| 1. PR isn't triggering bot webhooks (check workflow configuration) | ||
| 2. Bots are completely down (check status pages) | ||
| 3. PR has unusual properties (deleted files, binary content, etc.) | ||
|
|
||
| **Solution**: Check bot integrations in GitHub, restart workflows if needed. | ||
|
|
||
| ## Future Improvements | ||
|
|
||
| 1. **Per-bot timeout customization**: Different waits for slower/faster bots | ||
| 2. **Fallback reviewers**: If bots don't submit, require human review instead | ||
| 3. **Bot absence detection**: Fail loudly if expected bots never trigger | ||
| 4. **Metrics dashboard**: Track wait times, timeout rates per bot | ||
| 5. **Codex graduation**: Once Codex is on 95%+ of PRs, may reduce Tier 2 timeout | ||
|
|
||
| ## Related Issues & PRs | ||
|
|
||
| - **Issue #457**: Implement advisory bot review gate (this feature) | ||
| - **PR #453**: Auto-merge hold mechanism (incident that revealed the problem) | ||
| - **Issue #452**: PR branch deleted mid-run (CodeRabbit rate-limit consequence) | ||
| - **Fix**: CodeRabbit approval removal (context for why gate was needed) | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.