fix(supervisor): triage PR #392 review feedback - stderr, sort, deploy errors (t147.2)#451
fix(supervisor): triage PR #392 review feedback - stderr, sort, deploy errors (t147.2)#451marcusquinn merged 3 commits intomainfrom
Conversation
…y errors (t147.2) - Add post-PR states to ORDER BY CASE in cmd_status and cmd_list - Add 300s timeout to deploy setup.sh with portable fallback - Deploy failures now transition to 'failed' instead of silently marking 'deployed' - Redirect cmd_pr_lifecycle output to post-pr.log instead of /dev/null - Fix missing $SUPERVISOR_DB arg in db() calls for no_pr retry counter - Remove unused no_pr_key variable Dismissed 3 of 6 review threads as already fixed or invalid: - gh pr merge stderr: already uses 2>&1 - Fallback PR lookup: already uses git -C and gh pr list --repo - Missing pr_review:deployed transition: not needed, complete:deployed handles it
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ 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 |
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 and resolves critical feedback from a previous pull request (#392) concerning the Supervisor's post-PR lifecycle. The changes ensure that task states are correctly sorted, deployment failures are properly handled and reported, and the lifecycle process provides better logging and error tracking. It also includes general code improvements and updates to task tracking. Highlights
Changelog
Activity
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
|
🔍 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: Sat Feb 7 18:11:59 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
![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 effectively addresses several pieces of feedback from a previous review, fixing some important bugs. The changes to handle deployment failures correctly, fix database calls, and improve logging are all excellent. I've suggested one refactoring in run_deploy_for_task to improve memory efficiency and code clarity by removing an unnecessary variable and redirecting command output directly to the log file.
| log_info "Running setup.sh for $task_id (timeout: 300s)..." | ||
| local deploy_output deploy_log | ||
| deploy_log="$SUPERVISOR_DIR/logs/${task_id}-deploy-$(date +%Y%m%d%H%M%S).log" | ||
| mkdir -p "$SUPERVISOR_DIR/logs" | ||
|
|
||
| # Portable timeout: prefer timeout/gtimeout, fall back to background+kill | ||
| local timeout_cmd="" | ||
| if command -v timeout &>/dev/null; then | ||
| timeout_cmd="timeout 300" | ||
| elif command -v gtimeout &>/dev/null; then | ||
| timeout_cmd="gtimeout 300" | ||
| fi | ||
|
|
||
| if [[ -n "$timeout_cmd" ]]; then | ||
| if ! deploy_output=$(cd "$repo" && $timeout_cmd ./setup.sh 2>&1); then | ||
| log_warn "Deploy (setup.sh) returned non-zero for $task_id (see $deploy_log)" | ||
| echo "$deploy_output" > "$deploy_log" 2>/dev/null || true | ||
| return 1 | ||
| fi | ||
| else | ||
| # Fallback: background process with manual timeout | ||
| (cd "$repo" && ./setup.sh > "$deploy_log" 2>&1) & | ||
| local deploy_pid=$! | ||
| local waited=0 | ||
| while kill -0 "$deploy_pid" 2>/dev/null && [[ "$waited" -lt 300 ]]; do | ||
| sleep 5 | ||
| waited=$((waited + 5)) | ||
| done | ||
| if kill -0 "$deploy_pid" 2>/dev/null; then | ||
| kill "$deploy_pid" 2>/dev/null || true | ||
| log_warn "Deploy (setup.sh) timed out after 300s for $task_id (see $deploy_log)" | ||
| return 1 | ||
| fi | ||
| if ! wait "$deploy_pid"; then | ||
| deploy_output=$(cat "$deploy_log" 2>/dev/null || echo "") | ||
| log_warn "Deploy (setup.sh) returned non-zero for $task_id (see $deploy_log)" | ||
| return 1 | ||
| fi | ||
| deploy_output=$(cat "$deploy_log" 2>/dev/null || echo "") | ||
| fi | ||
| log_success "Deploy complete for $task_id" | ||
| return 0 |
There was a problem hiding this comment.
The deploy_output variable can be removed to improve memory efficiency and simplify the code. Instead of capturing the command's output in a variable and then writing it to a file, we can redirect the output directly to the log file. This avoids holding potentially large amounts of data in memory, which is especially important for a long-running deployment script.
The variable is also assigned in the fallback path but never used, so it can be safely removed from there as well.
log_info "Running setup.sh for $task_id (timeout: 300s)..."
local deploy_log
deploy_log="$SUPERVISOR_DIR/logs/${task_id}-deploy-$(date +%Y%m%d%H%M%S).log"
mkdir -p "$SUPERVISOR_DIR/logs"
# Portable timeout: prefer timeout/gtimeout, fall back to background+kill
local timeout_cmd=""
if command -v timeout &>/dev/null; then
timeout_cmd="timeout 300"
elif command -v gtimeout &>/dev/null; then
timeout_cmd="gtimeout 300"
fi
if [[ -n "$timeout_cmd" ]]; then
if ! (cd "$repo" && $timeout_cmd ./setup.sh > "$deploy_log" 2>&1); then
log_warn "Deploy (setup.sh) returned non-zero or timed out for $task_id (see $deploy_log)"
return 1
fi
else
# Fallback: background process with manual timeout
(cd "$repo" && ./setup.sh > "$deploy_log" 2>&1) &
local deploy_pid=$!
local waited=0
while kill -0 "$deploy_pid" 2>/dev/null && [[ "$waited" -lt 300 ]]; do
sleep 5
waited=$((waited + 5))
done
if kill -0 "$deploy_pid" 2>/dev/null; then
kill "$deploy_pid" 2>/dev/null || true
log_warn "Deploy (setup.sh) timed out after 300s for $task_id (see $deploy_log)"
return 1
fi
if ! wait "$deploy_pid"; then
log_warn "Deploy (setup.sh) returned non-zero for $task_id (see $deploy_log)"
return 1
fi
fi
log_success "Deploy complete for $task_id"
return 01 participant
Summary
Triages 6 review threads from PR #392 (Supervisor post-PR lifecycle). Verified each claim against current code, fixed 3 real bugs, dismissed 3 as already fixed or invalid.
Changes
Fixed (3 threads)
cmd_statusandcmd_listSQL queries now includepr_review,merging,deploying,merged,deployedin sort orderrun_deploy_for_taskfailure now transitions task tofailedstate instead of unconditionally markingdeployedcmd_pr_lifecycleoutput redirected to$SUPERVISOR_DIR/post-pr.loginstead of/dev/nullAdditional fixes found during triage
$SUPERVISOR_DBindb()calls (lines 3168, 3186): no_pr retry counter never persisted becausedb()received SQL as first arg instead of DB path (overlaps with t146)no_pr_keyvariable removedsetup.shexecution withtimeout/gtimeoutsupport and background-process fallbackDismissed (3 threads)
gh pr mergestderr suppressed (Gemini, high): Already fixed - line 2873 uses2>&1git -C "$task_repo_check"andgh pr list --repopr_review:deployedtransition (CodeRabbit, critical): Invalid - code handles no-PR tasks viacomplete:deployedtransition at line 3102; tasks only reachpr_reviewwhen a PR existsTask
Closes t147.2