fix: never auto-close issues without merged PR evidence#2115
fix: never auto-close issues without merged PR evidence#2115marcusquinn merged 4 commits intomainfrom
Conversation
Three bugs fixed in sync_issue_status_label(): 1. Failed tasks were auto-closed — now they post a failure comment and add needs-review label, keeping the issue OPEN for human triage. 2. Verified/deployed tasks were auto-closed even without a merged PR — now they verify the PR state is MERGED before closing. If no merged PR exists, the issue is flagged for human review instead. 3. needs-review label was not in ensure_status_labels() or ALL_STATUS_LABELS — now it is created on-demand and cleaned up on subsequent state transitions. Reopened 5 incorrectly auto-closed issues: #2106, #1998, #1764, #1763, #1762. Supersedes PR #2112 (which only fixed the failed-state bug).
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 refines the automated issue management system by addressing critical bugs that led to premature or incorrect issue closures. The primary goal is to enhance the reliability of issue status transitions, ensuring that issues are only closed when appropriate conditions are met, particularly the presence of a merged pull request for verified/deployed tasks. It also introduces a "needs-review" label to clearly mark issues requiring human attention, preventing silent failures and improving the overall workflow for issue resolution. 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
|
|
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. WalkthroughIntroduced a merged-PR gating and a new "needs-review" label in Changes
Sequence Diagram(s)sequenceDiagram
participant Supervisor as Supervisor Script
participant GH as GitHub API
participant Issue as Issue
participant PR as Pull Request
Supervisor->>GH: fetch issue labels & linked PR(s)
GH-->>Supervisor: labels, PR refs
Supervisor->>PR: check PR state (merged/open/closed)
PR-->>Supervisor: PR.state
alt PR.state == "MERGED"
Supervisor->>GH: post close comment & close Issue
GH-->>Issue: issue closed, comment added
else PR.state != "MERGED"
Supervisor->>GH: post verification/failure comment
Supervisor->>GH: add `needs-review` label to Issue
GH-->>Issue: comment added, label applied (issue remains open)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: Sat Feb 21 17:59:10 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 correctly fixes a critical bug where issues were being auto-closed without proper evidence. The new logic, which requires a merged pull request to close 'verified' or 'deployed' tasks and flags 'failed' tasks for human review, is a significant improvement to the issue lifecycle management. The changes are well-structured and directly address the root causes outlined in the description. I have two suggestions for improvement. Firstly, the widespread use of 2>/dev/null suppresses important diagnostic errors from gh and db commands, which violates the repository's style guide and makes debugging difficult. I've recommended removing this suppression, aligning with repository rules on error visibility. Secondly, the control flow in the new failed case is confusing and has a misleading comment; it should return explicitly like other terminal states, which also aligns with repository rules for explicit function returns. Overall, this is a valuable fix that enhances the reliability of the automated supervisor.
| pr_state=$(gh pr view "$pr_number" --repo "$repo_slug" --json state,mergedAt,changedFiles \ | ||
| --jq '"state:\(.state) merged:\(.mergedAt // "n/a") files:\(.changedFiles)"' 2>/dev/null || echo "") |
There was a problem hiding this comment.
While using || echo "" is a good way to prevent the script from exiting on an expected error (like a PR not being found), suppressing stderr with 2>/dev/null hides critical unexpected errors such as authentication failures, API rate limits, or network issues. This makes debugging failures much harder and violates the repository's style guide (line 50) and general rules.
By removing 2>/dev/null, expected errors (e.g., gh exiting with status 1 for a non-existent PR) will still be handled by || echo "", but unexpected system or auth errors will become visible in the logs. This pattern should be applied to the other gh and db calls in this file that use 2>/dev/null.
| pr_state=$(gh pr view "$pr_number" --repo "$repo_slug" --json state,mergedAt,changedFiles \ | |
| --jq '"state:\(.state) merged:\(.mergedAt // "n/a") files:\(.changedFiles)"' 2>/dev/null || echo "") | |
| pr_state=$(gh pr view "$pr_number" --repo "$repo_slug" --json state,mergedAt,changedFiles \ | |
| --jq '"state:\(.state) merged:\(.mergedAt // "n/a") files:\(.changedFiles)"' || echo "") |
References
2>/dev/nullis acceptable ONLY when redirecting to log files, not blanket suppression. This usage is a blanket suppression of potentialghCLI errors. (link)- 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.
| # Fall through to non-terminal state logic for label handling | ||
| ;; |
There was a problem hiding this comment.
The comment on line 481 is misleading. The ;; does not cause a fall-through in the case statement in the way the comment implies. Instead, execution continues after the case block, which is confusing.
The failed case should be treated as a terminal action within this case statement, similar to the verified | deployed and cancelled cases. It performs all necessary actions (commenting, labeling) and should then exit the function. Using return 0 makes the control flow clearer and consistent with the other terminal states.
| # Fall through to non-terminal state logic for label handling | |
| ;; | |
| # This is a terminal action. Return to prevent fall-through to the generic label handler. | |
| return 0 |
References
- All functions in shell scripts must have explicit
returnstatements to ensure predictable behavior and exit code propagation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.agents/scripts/supervisor/issue-sync.sh (1)
346-346: Naming inconsistency:needs-reviewbreaks thestatus:prefix convention.Every other label in
ALL_STATUS_LABELSuses thestatus:prefix. Consider renaming tostatus:needs-reviewfor consistency, or updating the comment on line 351 (which says "Removes all status:* labels") to reflect that this set now includes a non-status:label. Either way, the current code is functionally correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/issue-sync.sh at line 346, ALL_STATUS_LABELS currently contains a label "needs-review" that breaks the "status:" prefix convention; update the ALL_STATUS_LABELS string to replace "needs-review" with "status:needs-review" to maintain consistency (and scan for any usages of ALL_STATUS_LABELS or downstream label parsing logic to ensure they still match), or alternatively update the nearby comment that states "Removes all status:* labels" to mention the non-prefixed label—preferably rename to status:needs-review for consistency across label handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/supervisor/issue-sync.sh:
- Around line 424-427: The current check using grep -qi 'MERGED' against
pr_state always matches because the label "merged:" is present in the jq output;
update the logic that sets has_merged_pr to reliably detect a merged PR by
either 1) matching the state field explicitly (e.g., search for "state:MERGED"
or use a regex that anchors the field name) instead of plain "MERGED", or
preferably 2) replace the string-parse with a dedicated gh JSON query: call gh
pr view --repo "$repo_slug" "$pr_number" --json state and inspect the returned
.state value, then set has_merged_pr="true" only when that value equals
"MERGED"; update references to pr_state, the grep -qi 'MERGED' check, and the
has_merged_pr assignment accordingly.
- Around line 474-481: The "failed" case currently falls through because
state_to_status_label("failed") returns empty, so new_label is "", and gated
blocks never reopen or relabel a previously-closed issue; update the sync logic
to detect the "failed" state (e.g., check the status/state variable used before
calling state_to_status_label) and explicitly run gh to reopen the issue (gh
issue edit "$issue_number" --repo "$repo_slug" --state open) before adding the
"needs-review" label and posting fail_comment; ensure this happens regardless of
new_label being empty so closed issues are reopened for human review, and keep
using the existing fail_comment and needs-review label variables.
- Around line 421-422: The current check that tests whether a PR is merged
incorrectly matches the substring "merged" in the jq output (variable pr_state),
causing CLOSED/OPEN PRs to be treated as merged; update the conditional that
uses grep so it explicitly matches the state field value (e.g., change the grep
in the if that inspects pr_state to search for 'state:MERGED' instead of just
'MERGED' or 'merged') — locate the conditional around the use of pr_state (the
if echo "$pr_state" | grep ...) and replace the pattern to grep -q
'state:MERGED' to only detect actually merged PRs.
---
Nitpick comments:
In @.agents/scripts/supervisor/issue-sync.sh:
- Line 346: ALL_STATUS_LABELS currently contains a label "needs-review" that
breaks the "status:" prefix convention; update the ALL_STATUS_LABELS string to
replace "needs-review" with "status:needs-review" to maintain consistency (and
scan for any usages of ALL_STATUS_LABELS or downstream label parsing logic to
ensure they still match), or alternatively update the nearby comment that states
"Removes all status:* labels" to mention the non-prefixed label—preferably
rename to status:needs-review for consistency across label handling.
…eopen
1. Replace grep -qi 'MERGED' with dedicated gh pr view --json state
query. The old pattern matched the field label 'merged:' in the jq
output, causing ALL PRs to be treated as merged (false positive).
2. Add explicit reopen logic for failed state. The fall-through was a
no-op because state_to_status_label('failed') returns empty, so
the reopen block gated on new_label never fired. Now explicitly
checks if issue is closed and reopens it for human review.
🔍 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 21 20:50:10 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
Addresses Gemini review feedback: blanket stderr suppression hides authentication failures, API rate limits, and network issues. The || echo "" fallback still handles expected errors (non-existent PR).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.agents/scripts/supervisor/issue-sync.sh (2)
420-423:pr_state_rawcan be derived frompr_state, eliminating a redundant API call.Both lines 420–421 and 422–423 independently call
gh pr viewfor the same PR number. The first call fetches only.state; the second already retrieves the same field (plusmergedAtandchangedFiles). This is one extra GitHub API round-trip per state transition and introduces a small TOCTOU window where the PR could change state between the two requests.♻️ Merge into a single call
- local pr_state="" pr_state_raw="" - pr_state_raw=$(gh pr view "$pr_number" --repo "$repo_slug" --json state --jq '.state' 2>/dev/null || echo "") - pr_state=$(gh pr view "$pr_number" --repo "$repo_slug" --json state,mergedAt,changedFiles \ - --jq '"state:\(.state) merged:\(.mergedAt // "n/a") files:\(.changedFiles)"' 2>/dev/null || echo "") + local pr_state="" pr_state_raw="" + pr_state=$(gh pr view "$pr_number" --repo "$repo_slug" --json state,mergedAt,changedFiles \ + --jq '"state:\(.state) merged:\(.mergedAt // "n/a") files:\(.changedFiles)"' 2>/dev/null || echo "") + pr_state_raw=$(echo "$pr_state" | sed -n 's/^state:\([A-Z]*\).*/\1/p')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/issue-sync.sh around lines 420 - 423, The two gh pr view calls are redundant; remove the first call and derive pr_state_raw from the output of the second call (the pr_state variable) to avoid an extra API request and a TOCTOU window. Update the assignment that sets pr_state to run a single gh pr view --json state,mergedAt,changedFiles ... --jq '..."' and then extract the raw state into pr_state_raw by parsing pr_state (e.g., pulling the .state value from the jq output string) so both pr_state and pr_state_raw are produced from the same API response.
29-29:needs-reviewcolorD93F0Bduplicatesstatus:claimed.Both labels render identically in GitHub's label list. Consider a distinct hex (e.g.
E99695) to make triage easier.🎨 Suggested color tweak
- gh label create "needs-review" --repo "$repo_slug" --color "D93F0B" --description "Flagged for human review by AI supervisor" --force 2>/dev/null || true + gh label create "needs-review" --repo "$repo_slug" --color "E99695" --description "Flagged for human review by AI supervisor" --force 2>/dev/null || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/issue-sync.sh at line 29, The label creation for "needs-review" currently uses color "D93F0B", which duplicates the existing "status:claimed" color; update the gh label create invocation that sets "needs-review" (the line calling gh label create "needs-review" --repo "$repo_slug" --color "D93F0B" ...) to use a distinct hex (e.g. "E99695") so the label is visually unique from "status:claimed".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/supervisor/issue-sync.sh:
- Around line 444-445: The script currently builds remove_args (from
ALL_STATUS_LABELS) and then calls gh issue edit with both --remove-label ... and
--add-label "needs-review" in the same command, causing "needs-review" to be
removed when new_label is empty; fix by splitting the operations: first call gh
issue edit to remove status labels using the constructed "${remove_args[@]}",
then call a separate gh issue edit to add "needs-review" (only when desired).
Update both call sites that use gh issue edit (the no-merged-PR path and the
failed path) so they perform the removal and the needs-review addition in two
sequential gh calls rather than one combined call, ensuring remove_args does not
include or conflict with the separate add operation; keep existing error
handling (2>/dev/null || true) behavior.
---
Nitpick comments:
In @.agents/scripts/supervisor/issue-sync.sh:
- Around line 420-423: The two gh pr view calls are redundant; remove the first
call and derive pr_state_raw from the output of the second call (the pr_state
variable) to avoid an extra API request and a TOCTOU window. Update the
assignment that sets pr_state to run a single gh pr view --json
state,mergedAt,changedFiles ... --jq '..."' and then extract the raw state into
pr_state_raw by parsing pr_state (e.g., pulling the .state value from the jq
output string) so both pr_state and pr_state_raw are produced from the same API
response.
- Line 29: The label creation for "needs-review" currently uses color "D93F0B",
which duplicates the existing "status:claimed" color; update the gh label create
invocation that sets "needs-review" (the line calling gh label create
"needs-review" --repo "$repo_slug" --color "D93F0B" ...) to use a distinct hex
(e.g. "E99695") so the label is visually unique from "status:claimed".
marcusquinn
dismissed
coderabbitai[bot]’s stale review
Both issues raised in this review have been fixed in subsequent commits and marked resolved. Dismissing stale CHANGES_REQUESTED.
🔍 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 21 20:56:49 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
CodeRabbit found that needs-review in ALL_STATUS_LABELS caused the remove_args loop to include --remove-label needs-review, negating the --add-label needs-review in the same gh issue edit call. Fix: remove needs-review from ALL_STATUS_LABELS (it's a triage flag, not a status label). Also merge redundant dual gh pr view calls into one API request, and use distinct label color (E99695) to avoid duplicating status:claimed color.
marcusquinn
dismissed
coderabbitai[bot]’s stale review
All 3 items addressed in commit 02fcf8f: (1) needs-review removed from ALL_STATUS_LABELS so it's no longer simultaneously added and removed, (2) dual gh pr view merged into single API call, (3) label color changed to E99695.
🔍 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 21 20:59:27 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
1 participant
Summary
needs-reviewlabel, keeps issue OPEN for human triageneeds-reviewlabel added toensure_status_labels()andALL_STATUS_LABELSfor proper lifecycle managementRoot Cause
sync_issue_status_label()inissue-sync.shhad three bugs:failedstate calledgh issue close— should never close, only flagverified|deployedstate closed issues even whenpr_urlwas empty,no_pr,task_only, ortask_obsolete— should require actual merged PRneeds-reviewlabel was used but never created byensure_status_labels(), causing silent failuresImpact
Reopened 5 incorrectly auto-closed issues: #2106, #1998, #1764, #1763, #1762
Supersedes
Supersedes PR #2112 (which only fixed the failed-state bug, not the verified/deployed path or label management).
Files Changed
.agents/scripts/supervisor/issue-sync.shFixes #1763, #1762, #2106
Summary by CodeRabbit
New Features
Bug Fixes