Skip to content

t2839: Fix cmd_pr_lifecycle fast-path merging PRs with zero reviews #2842

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.

Sign up for GitHub

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
alex-solovyev merged 4 commits into from
Mar 4, 2026
Merged

t2839: Fix cmd_pr_lifecycle fast-path merging PRs with zero reviews #2842

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e13ad5f
fix: require at least one review before fast-path merge (t2839)
alex-solovyev Mar 4, 2026
84838d2
fix: address CodeRabbit + Gemini review feedback on review gate logic
alex-solovyev Mar 4, 2026
907a92d
fix: SKIP from bot gate no longer bypasses zero-review check (t2839)
alex-solovyev Mar 4, 2026
2713e45
fix: clarify WAITING semantics and review count check flow (t2839)
alex-solovyev Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .agents/scripts/commands/pulse.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,8 @@ Then skip to the next PR. The next pulse cycle will retry the permission check

**For maintainer PRs (admin/maintain/write permission):**

- **Green CI + no blocking reviews** → merge: `gh pr merge <number> --repo <slug> --squash`. If the PR resolves an issue, the issue should be closed with a comment linking to the merged PR.
- **Green CI + at least one review posted + no blocking reviews** → merge: `gh pr merge <number> --repo <slug> --squash`. If the PR resolves an issue, the issue should be closed with a comment linking to the merged PR. **CRITICAL (t2839): Zero reviews means "not yet reviewed", NOT "clean to merge".** Before merging, verify at least one review exists (human or bot). Check with `review-bot-gate-helper.sh check <number> <slug>` — if it returns `WAITING`, skip the PR this cycle. If the script is unavailable, check `gh pr view <number> --repo <slug> --json reviews --jq '.reviews | length'` — if 0, skip. A PR with green CI but zero reviews should be left for the next pulse cycle, not merged.
- **Green CI + zero reviews** → skip this cycle. Review bots typically post within 2-5 minutes. The next pulse will pick it up once a review exists.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

@gemini-code-assist gemini-code-assist Bot Mar 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The updated instructions are a bit difficult to parse due to the first bullet point being very long and containing information that is repeated in the second bullet point. For better readability and to avoid redundancy, I suggest restructuring this section to clearly separate the merge rule from the implementation details of the check.

Suggested change
- **Green CI + at least one review posted + no blocking reviews** → merge: `gh pr merge <number> --repo <slug> --squash`. If the PR resolves an issue, the issue should be closed with a comment linking to the merged PR. **CRITICAL (t2839): Zero reviews means "not yet reviewed", NOT "clean to merge".** Before merging, verify at least one review exists (human or bot). Check with `review-bot-gate-helper.sh check <number> <slug>` — if it returns `WAITING`, skip the PR this cycle. If the script is unavailable, check `gh pr view <number> --repo <slug> --json reviews --jq '.reviews | length'` — if 0, skip. A PR with green CI but zero reviews should be left for the next pulse cycle, not merged.
- **Green CI + zero reviews** → skip this cycle. Review bots typically post within 2-5 minutes. The next pulse will pick it up once a review exists.
- **Green CI + at least one review posted + no blocking reviews** → merge: `gh pr merge <number> --repo <slug> --squash`. If the PR resolves an issue, the issue should be closed with a comment linking to the merged PR.
- **CRITICAL (t2839):** Before merging, verify at least one review exists. Use `review-bot-gate-helper.sh check <number> <slug>`. If it returns `WAITING`, the PR has zero reviews. As a fallback, check if `gh pr view <number> --repo <slug> --json reviews --jq '.reviews | length'` is `0`.
- **Green CI + zero reviews** → skip this cycle. Zero reviews means "not yet reviewed", NOT "clean to merge". Review bots typically post within 2-5 minutes. The next pulse will pick it up.

- **Failing CI or changes requested** → dispatch a worker to fix it (counts against worker slots)

**For all PRs (regardless of author):**
Expand Down
35 changes: 33 additions & 2 deletions .agents/scripts/supervisor-archived/deploy.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,15 +342,46 @@ cmd_pr_lifecycle() {
pr_number_fastpath="${parsed_fastpath##*|}"

if [[ -n "$pr_number_fastpath" && -n "$repo_slug_fastpath" ]]; then
# t2839: Check that at least one review exists before fast-path merge.
# Zero reviews means "not yet reviewed", not "clean to merge".
# Uses review-bot-gate-helper.sh (t1382) if available, falls back to
# gh pr view --json reviews count.
local review_gate_result="UNKNOWN"
local review_bot_gate_script
review_bot_gate_script="$(dirname "$(dirname "${BASH_SOURCE[0]}")")/review-bot-gate-helper.sh"
if [[ -x "$review_bot_gate_script" ]]; then
review_gate_result=$("$review_bot_gate_script" check "$pr_number_fastpath" "$repo_slug_fastpath" 2>/dev/null) || review_gate_result="WAITING"

@gemini-code-assist gemini-code-assist Bot Mar 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Suppressing stderr with 2>/dev/null prevents visibility into potential failures within the review-bot-gate-helper.sh script, such as authentication issues or unexpected errors. While the command correctly falls back to WAITING, logging the actual error would significantly aid in debugging. Please remove the stderr redirection to allow errors to be logged.

Suggested change
review_gate_result=$("$review_bot_gate_script" check "$pr_number_fastpath" "$repo_slug_fastpath" 2>/dev/null) || review_gate_result="WAITING"
review_gate_result=$("$review_bot_gate_script" check "$pr_number_fastpath" "$repo_slug_fastpath") || review_gate_result="WAITING"
References
  1. Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.

else
# Fallback: count reviews directly via gh API
local review_count_fastpath
review_count_fastpath=$(gh pr view "$pr_number_fastpath" --repo "$repo_slug_fastpath" \
--json reviews --jq '.reviews | length' 2>/dev/null || echo "0")

@gemini-code-assist gemini-code-assist Bot Mar 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of 2>/dev/null here hides potential errors from the gh command, such as authentication failures, network issues, or invalid repository slugs. Although the command falls back to 0, the underlying reason for the failure is lost, making debugging difficult. It's better to let stderr be printed to the logs.

Suggested change
review_count_fastpath=$(gh pr view "$pr_number_fastpath" --repo "$repo_slug_fastpath" \
--json reviews --jq '.reviews | length' 2>/dev/null || echo "0")
review_count_fastpath=$(gh pr view "$pr_number_fastpath" --repo "$repo_slug_fastpath" \
--json reviews --jq '.reviews | length' || echo "0")
References
  1. Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.

if [[ "$review_count_fastpath" -gt 0 ]]; then
review_gate_result="PASS"
else
review_gate_result="WAITING"
fi
fi

if [[ "$review_gate_result" == "WAITING" ]]; then
log_info "Fast-path blocked: no reviews posted yet for $task_id — waiting for review before merge (t2839)"
# Stay in pr_review state; next pulse will re-check
local stage_end
stage_end=$(date +%s)
stage_timings="${stage_timings}pr_review:$((stage_end - stage_start))s(no_reviews),"
record_lifecycle_timing "$task_id" "$stage_timings" 2>/dev/null || true

@gemini-code-assist gemini-code-assist Bot Mar 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Suppressing stderr for record_lifecycle_timing can hide errors related to the monitoring and metrics pipeline. While || true correctly prevents the script from exiting on failure, allowing errors to be logged is crucial for diagnosing issues with timing recordings. Please remove the 2>/dev/null.

Suggested change
record_lifecycle_timing "$task_id" "$stage_timings" 2>/dev/null || true
record_lifecycle_timing "$task_id" "$stage_timings" || true
References
  1. Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.

return 0
fi

local threads_json_fastpath
threads_json_fastpath=$(check_review_threads "$repo_slug_fastpath" "$pr_number_fastpath" 2>/dev/null || echo "[]")
local thread_count_fastpath
thread_count_fastpath=$(echo "$threads_json_fastpath" | jq 'length' 2>/dev/null || echo "0")

if [[ "$thread_count_fastpath" -eq 0 ]]; then
log_info "Fast-path: CI green + zero review threads - skipping review_triage, going directly to merge${merge_note}"
log_info "Fast-path: CI green + reviews posted + zero unresolved threads - skipping review_triage, going directly to merge${merge_note}"
if [[ "$dry_run" == "false" ]]; then
db "$SUPERVISOR_DB" "UPDATE tasks SET triage_result = '{\"action\":\"merge\",\"threads\":0,\"fast_path\":true,\"sonarcloud_unstable\":$(if [[ "$pr_status" == "unstable_sonarcloud" ]]; then echo "true"; else echo "false"; fi)}' WHERE id = '$escaped_id';"
db "$SUPERVISOR_DB" "UPDATE tasks SET triage_result = '{\"action\":\"merge\",\"threads\":0,\"fast_path\":true,\"review_gate\":\"$review_gate_result\",\"sonarcloud_unstable\":$(if [[ "$pr_status" == "unstable_sonarcloud" ]]; then echo "true"; else echo "false"; fi)}' WHERE id = '$escaped_id';"
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
cmd_transition "$task_id" "merging" 2>>"$SUPERVISOR_LOG" || true
fi
tstatus="merging"
Expand Down
Loading