t1075: Fix verification infinite loop, stuck verifying recovery, and bot review dismissal#1566
Conversation
… review dismissal (t1075) Three systemic fixes for supervisor stuck states: 1. Verification infinite loop: Phase 3b2 endlessly resets verify_failed tasks back to deployed. Now caps at 3 retries via state_log counting, then marks failed with actionable error message. 2. Stuck verifying recovery: If pulse crashes mid-verification, tasks stay in 'verifying' forever. process_verify_queue now detects tasks stuck in verifying for >5 minutes and resets them to deployed. 3. Bot review dismissal: dismiss_bot_reviews() checked for 'coderabbitai' but actual login is 'coderabbitai[bot]'. Changed to regex test() that matches any login starting with known bot prefixes. Also fixes: - tech-stack-helper.sh: remove unused USAGE_COMMAND_OPTIONS (SC2034) - subagent-index.toon: add missing openexplorer and wappalyzer entries
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses several critical issues within the supervisor system that caused tasks to enter infinite loops or become permanently stuck. The changes enhance the system's robustness by introducing retry limits for verification failures, implementing a recovery mechanism for tasks stuck during verification, and improving the accuracy of bot review detection. These fixes aim to reduce manual intervention and improve the overall reliability of task processing. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughThe changes address three core issues: bot review dismissal logic is widened with regex pattern matching to catch more bot types, infinite verify_failed→deployed retry loops are capped at 3 attempts with permanent failure escalation, and tasks stuck in 'verifying' state are recovered after 5 minutes. Additionally, the subagent index is incremented and two new tool entries (openexplorer and wappalyzer) are registered with supporting scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as GitHub PR
participant Pulse as pulse.sh<br/>(Phase 3b2)
participant StateLog as state_log<br/>(Database)
participant PR_Status as PR Status<br/>(Issue)
PR->>Pulse: MERGED, in verify_failed
Pulse->>StateLog: Query verify_failed→deployed<br/>transition count
StateLog-->>Pulse: Reset count (transitions found)
alt Reset count >= max_verify_retries (3)
Pulse->>PR_Status: Mark task FAILED<br/>(Exhausted retries)
Pulse->>StateLog: Log exhaustion reason
else Reset count < max_verify_retries
Pulse->>PR_Status: Reset to deployed
Pulse->>PR_Status: Clear errors
Pulse->>StateLog: Log retry attempt `#N`
Pulse->>PR: Queue for re-verification
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Feb 17 14:16:54 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.agents/scripts/supervisor/pulse.sh (1)
610-662:⚠️ Potential issue | 🟡 MinorRetry exhaustion currently logs as deployed — misreports reconciliation.
When the cap is hit, you mark
failedbut the generic proof-log still recordsverify_failed->deployedand bumpsreconciled_merged. Add a failure-specific proof-log and skip the generic path.🧾 Suggested fix
if [[ "$verify_reset_count" -ge "$max_verify_retries" ]]; then # Exhausted verification retries — mark permanently failed log_error " Phase 3b2: $stale_id exhausted $max_verify_retries verification retries — marking failed" db "$SUPERVISOR_DB" "UPDATE tasks SET status = 'failed', error = 'Verification failed after $max_verify_retries retries — manual fix needed', updated_at = strftime('%Y-%m-%dT%H:%M:%SZ','now') WHERE id = '$escaped_stale_id';" 2>/dev/null || true db "$SUPERVISOR_DB" "INSERT INTO state_log (task_id, from_state, to_state, timestamp, reason) VALUES ('$escaped_stale_id', 'verify_failed', 'failed', strftime('%Y-%m-%dT%H:%M:%SZ','now'), 'Phase 3b2: verification exhausted ($verify_reset_count/$max_verify_retries retries)');" 2>/dev/null || true sync_issue_status_label "$stale_id" "failed" "phase_3b2" 2>>"$SUPERVISOR_LOG" || true + write_proof_log --task "$stale_id" --event "reconcile_failed" --stage "phase_3b2" \ + --decision "verify_failed->failed (retries exhausted)" \ + --evidence "retries=$verify_reset_count/$max_verify_retries pr=#$pr_number" \ + --maker "pulse:phase_3b2" 2>/dev/null || true + continue else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/pulse.sh around lines 610 - 662, The proof-log and reconciled_merged increment are always executed after the phase_3b2 block, causing misreports when verify_reset_count >= max_verify_retries; modify the branch that marks the task as 'failed' (the if branch guarded by verify_reset_count and max_verify_retries) to emit a failure-specific write_proof_log call (e.g., decision "verify_failed->failed", evidence including verify_reset_count/max_verify_retries and pr_number) and then skip the common generic write_proof_log + reconciled_merged increment (use a continue/else/return or move the generic proof-log into the non-failure branch) so only successful reconciliations increment reconciled_merged..agents/subagent-index.toon (1)
24-76:⚠️ Potential issue | 🟠 MajorTOON index count looks out of sync (subagents[50] vs ~52 entries).
To keep the index machine-parseable, update the header count or consolidate duplicate
tools/research/rows.🧭 If keeping all entries, update the header
-<!--TOON:subagents[50]{folder,purpose,key_files}: +<!--TOON:subagents[52]{folder,purpose,key_files}:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/subagent-index.toon around lines 24 - 76, The TOON header count "TOON:subagents[50]" is out of sync with the actual entries (about 52) and there are duplicate "tools/research/" rows; either update the header count to the correct total (adjust "subagents[50]" to the actual number) or remove/consolidate the duplicate "tools/research/" entries so the index remains machine-parseable, ensuring you edit the TOON header token and the duplicate lines accordingly.
🧹 Nitpick comments (1)
.agents/scripts/supervisor/deploy.sh (1)
1442-1444: Harden bot-review detection to avoid dismissing human reviews.For A-grade automation safety, require GitHub’s
user.type == "Bot"in addition to the prefix match.🔒 Suggested filter hardening
-bot_reviews=$(echo "$reviews_json" | jq -r '.[] | select(.state == "CHANGES_REQUESTED" and (.user.login | test("^(coderabbitai|gemini-code-assist|copilot)"))) | .id' 2>/dev/null || echo "") +bot_reviews=$(echo "$reviews_json" | jq -r '.[] | select(.state == "CHANGES_REQUESTED" and (.user.type == "Bot" or (.user.login | test("^(coderabbitai|gemini-code-assist|copilot)"; "i")))) | .id' 2>/dev/null || echo "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/deploy.sh around lines 1442 - 1444, The bot-review detection currently selects reviews by matching reviewer login prefixes in the jq filter for variable bot_reviews; update the jq expression to also require that .user.type == "Bot" so only GitHub-identified bots are matched (i.e., change the select to include both (.user.login | test("^(coderabbitai|gemini-code-assist|copilot)")) and (.user.type == "Bot")), keeping the existing fallback (2>/dev/null || echo "") behavior and referencing the same reviews_json and bot_reviews variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.agents/scripts/supervisor/pulse.sh:
- Around line 610-662: The proof-log and reconciled_merged increment are always
executed after the phase_3b2 block, causing misreports when verify_reset_count
>= max_verify_retries; modify the branch that marks the task as 'failed' (the if
branch guarded by verify_reset_count and max_verify_retries) to emit a
failure-specific write_proof_log call (e.g., decision "verify_failed->failed",
evidence including verify_reset_count/max_verify_retries and pr_number) and then
skip the common generic write_proof_log + reconciled_merged increment (use a
continue/else/return or move the generic proof-log into the non-failure branch)
so only successful reconciliations increment reconciled_merged.
In @.agents/subagent-index.toon:
- Around line 24-76: The TOON header count "TOON:subagents[50]" is out of sync
with the actual entries (about 52) and there are duplicate "tools/research/"
rows; either update the header count to the correct total (adjust
"subagents[50]" to the actual number) or remove/consolidate the duplicate
"tools/research/" entries so the index remains machine-parseable, ensuring you
edit the TOON header token and the duplicate lines accordingly.
---
Nitpick comments:
In @.agents/scripts/supervisor/deploy.sh:
- Around line 1442-1444: The bot-review detection currently selects reviews by
matching reviewer login prefixes in the jq filter for variable bot_reviews;
update the jq expression to also require that .user.type == "Bot" so only
GitHub-identified bots are matched (i.e., change the select to include both
(.user.login | test("^(coderabbitai|gemini-code-assist|copilot)")) and
(.user.type == "Bot")), keeping the existing fallback (2>/dev/null || echo "")
behavior and referencing the same reviews_json and bot_reviews variables.
![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 several important fixes for the supervisor's stability and robustness. It addresses an infinite loop in verification by adding a retry limit, recovers tasks stuck in the 'verifying' state, and improves the logic for dismissing bot reviews. The changes are well-structured and address critical operational issues.
My review focuses on improving maintainability and adhering to the repository's style guide. I've suggested making magic numbers for retry counts and timeouts into constants for easier configuration. I've also pointed out several instances where error output is suppressed with 2>/dev/null, which violates the style guide, and recommended redirecting to the supervisor log instead for better debuggability.
Overall, these are excellent and necessary changes.
| WHERE task_id = '$escaped_stale_id' | ||
| AND from_state = 'verify_failed' | ||
| AND to_state = 'deployed';" 2>/dev/null || echo "0") | ||
| local max_verify_retries=3 |
There was a problem hiding this comment.
For better maintainability and clarity, it's best to define magic numbers like 3 as a script-level constant. Since this requires a change outside the diff, I can't provide a direct suggestion, but please consider defining readonly MAX_VERIFY_RETRIES=3 at the top of the script and using $MAX_VERIFY_RETRIES here. This also aligns with the style guide's naming convention for constants (line 39).
References
- The style guide at line 39 specifies that constants should use UPPER_SNAKE_CASE. (link)
| db "$SUPERVISOR_DB" "UPDATE tasks SET | ||
| status = 'failed', | ||
| error = 'Verification failed after $max_verify_retries retries — manual fix needed', | ||
| updated_at = strftime('%Y-%m-%dT%H:%M:%SZ','now') | ||
| WHERE id = '$escaped_stale_id';" 2>/dev/null || true | ||
| db "$SUPERVISOR_DB" "INSERT INTO state_log (task_id, from_state, to_state, timestamp, reason) | ||
| VALUES ('$escaped_stale_id', 'verify_failed', 'failed', | ||
| strftime('%Y-%m-%dT%H:%M:%SZ','now'), | ||
| 'Phase 3b2: verification exhausted ($verify_reset_count/$max_verify_retries retries)');" 2>/dev/null || true | ||
| sync_issue_status_label "$stale_id" "failed" "phase_3b2" 2>>"$SUPERVISOR_LOG" || true | ||
| else | ||
| log_info " Phase 3b2: $stale_id (verify_failed) — PR #$pr_number merged, resetting to deployed for re-verification (attempt $((verify_reset_count + 1))/$max_verify_retries)" | ||
| db "$SUPERVISOR_DB" "UPDATE tasks SET | ||
| status = 'deployed', | ||
| error = NULL, | ||
| updated_at = strftime('%Y-%m-%dT%H:%M:%SZ','now') | ||
| WHERE id = '$escaped_stale_id';" 2>/dev/null || true | ||
| db "$SUPERVISOR_DB" "INSERT INTO state_log (task_id, from_state, to_state, timestamp, reason) | ||
| VALUES ('$escaped_stale_id', 'verify_failed', 'deployed', | ||
| strftime('%Y-%m-%dT%H:%M:%SZ','now'), | ||
| 'Phase 3b2: reset for re-verification attempt $((verify_reset_count + 1))/$max_verify_retries (PR #$pr_number merged)');" 2>/dev/null || true | ||
| sync_issue_status_label "$stale_id" "deployed" "phase_3b2" 2>>"$SUPERVISOR_LOG" || true | ||
| fi |
There was a problem hiding this comment.
The style guide (line 50) states that 2>/dev/null should only be used when redirecting to log files, not for blanket suppression. The db calls in this block use this pattern. To improve debuggability and adhere to the style guide, please redirect stderr to $SUPERVISOR_LOG instead (e.g., ... 2>>"$SUPERVISOR_LOG" || true).
References
- The style guide at line 50 states that
2>/dev/nullis only acceptable when redirecting to log files, not for blanket error suppression. (link)
| stuck_verifying=$(db -separator '|' "$SUPERVISOR_DB" " | ||
| SELECT id FROM tasks | ||
| WHERE status = 'verifying' | ||
| AND updated_at < strftime('%Y-%m-%dT%H:%M:%SZ', 'now', '-5 minutes') | ||
| ORDER BY id; | ||
| " 2>/dev/null || echo "") | ||
|
|
||
| if [[ -n "$stuck_verifying" ]]; then | ||
| while IFS='|' read -r stuck_id; do | ||
| [[ -z "$stuck_id" ]] && continue | ||
| log_warn " $stuck_id: stuck in 'verifying' for >5min — resetting to 'deployed'" | ||
| local escaped_stuck_id | ||
| escaped_stuck_id=$(sql_escape "$stuck_id") | ||
| db "$SUPERVISOR_DB" "UPDATE tasks SET | ||
| status = 'deployed', | ||
| error = NULL, | ||
| updated_at = strftime('%Y-%m-%dT%H:%M:%SZ','now') | ||
| WHERE id = '$escaped_stuck_id';" 2>/dev/null || true | ||
| db "$SUPERVISOR_DB" "INSERT INTO state_log (task_id, from_state, to_state, timestamp, reason) | ||
| VALUES ('$escaped_stuck_id', 'verifying', 'deployed', | ||
| strftime('%Y-%m-%dT%H:%M:%SZ','now'), | ||
| 'process_verify_queue: recovered from stuck verifying state (>5min timeout)');" 2>/dev/null || true |
There was a problem hiding this comment.
The style guide (line 50) states that 2>/dev/null should only be used when redirecting to log files, not for blanket suppression. The db calls in this block (lines 320, 333, and 338) use this pattern. To improve debuggability and adhere to the style guide, please redirect stderr to $SUPERVISOR_LOG instead (e.g., ... 2>>"$SUPERVISOR_LOG" || true).
References
- The style guide at line 50 states that
2>/dev/nullis only acceptable when redirecting to log files, not for blanket error suppression. (link)
| stuck_verifying=$(db -separator '|' "$SUPERVISOR_DB" " | ||
| SELECT id FROM tasks | ||
| WHERE status = 'verifying' | ||
| AND updated_at < strftime('%Y-%m-%dT%H:%M:%SZ', 'now', '-5 minutes') |
There was a problem hiding this comment.
The timeout value '5 minutes' is a magic number. For better maintainability, please define it as a script-level constant, e.g., readonly STUCK_VERIFYING_TIMEOUT_MINUTES=5, and use the variable in the query. This makes the timeout policy easier to find and adjust.
1 participant
Summary
Three systemic fixes for supervisor stuck states that caused tasks to loop indefinitely or get permanently stuck:
verify_failed→deployed, creating an infinite loop when checks genuinely fail. Now counts retries viastate_logand marksfailedafter 3 attemptsverifyingforever.process_verify_queuenow detects tasks stuck >5 minutes and resets todeployeddismiss_bot_reviews()checked for"coderabbitai"but actual GitHub login is"coderabbitai[bot]". Changed to regextest()matching any login starting with known bot prefixes (coderabbitai, gemini-code-assist, copilot)Also fixes:
tech-stack-helper.sh: remove unusedUSAGE_COMMAND_OPTIONSconstant (SC2034)subagent-index.toon: add missingopenexplorerandwappalyzerprovider entries (unblocks t1066, t1067 verification)Files Changed
.agents/scripts/supervisor/pulse.sh.agents/scripts/supervisor/todo-sync.sh.agents/scripts/supervisor/deploy.sh.agents/scripts/tech-stack-helper.sh.agents/subagent-index.toonCloses #1565
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Chores