t1055: Issue-sync includes PR proof-log when closing issues#1504
t1055: Issue-sync includes PR proof-log when closing issues#1504marcusquinn merged 1 commit intomainfrom
Conversation
…issues (t1055) When closing issues for verified/deployed/cancelled/failed tasks, the closing comment now includes: - PR number, merge state, and changed file count (for verified/deployed) - 'No PR on record' warning when no PR exists (flags manual review) - Cancellation reason from DB error field (for cancelled) - Failure error from DB (for failed) Prevents knowledge loss when issues are the only record of work. Closes #1503
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 significantly enhances the issue closing process within the 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 PR enhances GitHub issue closure with data-driven context. When issues reach terminal states (verified, deployed, cancelled, failed), the script now constructs rich close comments by querying task lineage, extracting stored PR URLs, and fetching PR details via Changes
Sequence DiagramsequenceDiagram
participant Script as issue-sync.sh
participant DB as Database
participant GH as GitHub API (gh)
participant Issue as GitHub Issue
Script->>Script: Detect terminal state
alt Verified/Deployed
Script->>DB: Query task lineage & PR URL
DB-->>Script: PR URL (or none)
alt PR URL exists
Script->>Script: Extract PR number
Script->>GH: Query PR details (state, mergedAt, files)
GH-->>Script: PR metadata
Script->>Script: Build composite message with proof
else No PR URL
Script->>Script: Build manual delivery notice
end
else Cancelled
Script->>DB: Fetch cancellation reason
DB-->>Script: Reason text
Script->>Script: Append reason to close comment
else Failed
Script->>DB: Fetch error message
DB-->>Script: Error context
Script->>Script: Include error in close comment
end
Script->>Issue: Close with data-driven comment
Issue-->>Script: Closure confirmed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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: Sun Feb 15 22:36:04 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 enhances the issue-sync process by adding more detailed, context-rich comments when closing GitHub issues. It now includes pull request details for 'verified' or 'deployed' tasks, and database-sourced reasons for 'cancelled' or 'failed' tasks, which significantly improves traceability. My review focuses on improving code structure and ensuring adherence to the repository's shell scripting style guide. I've identified opportunities to simplify conditional logic and correct a style guide violation related to error suppression.
| local close_comment | ||
| close_comment="Task $task_id reached state: $new_state (from $old_state)" | ||
| local pr_url="" | ||
| pr_url=$(db "$SUPERVISOR_DB" "SELECT pr_url FROM tasks WHERE id='$(sql_escape "$task_id")';" 2>/dev/null || echo "") |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null violates the repository's style guide (line 50), which states that 2>/dev/null is only acceptable when redirecting to log files. This blanket suppression can hide important error messages from the db command, making debugging more difficult. Please remove the suppression to allow errors to be visible or logged appropriately. This issue is present in multiple places in this function (e.g., lines 418, 427, 430, 438, 444, 447, 455, 461, 463).
| pr_url=$(db "$SUPERVISOR_DB" "SELECT pr_url FROM tasks WHERE id='$(sql_escape "$task_id")';" 2>/dev/null || echo "") | |
| pr_url=$(db "$SUPERVISOR_DB" "SELECT pr_url FROM tasks WHERE id='$(sql_escape "$task_id")';" || echo "") |
References
- The style guide prohibits blanket error suppression with
2>/dev/null. Errors should be logged or visible, and suppression is only allowed when redirecting to a log file. (link)
| if [[ -n "$pr_url" && "$pr_url" != "null" ]]; then | ||
| local pr_number="" | ||
| pr_number=$(echo "$pr_url" | grep -oE '[0-9]+$' || echo "") | ||
| if [[ -n "$pr_number" ]]; then | ||
| local pr_state="" | ||
| 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 "") | ||
| close_comment="Verified: PR #$pr_number ($pr_state). Task $task_id: $old_state -> $new_state" | ||
| fi | ||
| fi | ||
| if [[ -z "$pr_url" || "$pr_url" == "null" ]]; then | ||
| close_comment="Task $task_id reached state: $new_state (from $old_state). No PR on record — verify deliverables manually." | ||
| fi |
There was a problem hiding this comment.
The two if statements checking for pr_url are redundant. The condition in the second if is the logical negation of the first. This can be simplified into a more readable and maintainable if/else block.
| if [[ -n "$pr_url" && "$pr_url" != "null" ]]; then | |
| local pr_number="" | |
| pr_number=$(echo "$pr_url" | grep -oE '[0-9]+$' || echo "") | |
| if [[ -n "$pr_number" ]]; then | |
| local pr_state="" | |
| 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 "") | |
| close_comment="Verified: PR #$pr_number ($pr_state). Task $task_id: $old_state -> $new_state" | |
| fi | |
| fi | |
| if [[ -z "$pr_url" || "$pr_url" == "null" ]]; then | |
| close_comment="Task $task_id reached state: $new_state (from $old_state). No PR on record — verify deliverables manually." | |
| fi | |
| if [[ -n "$pr_url" && "$pr_url" != "null" ]]; then | |
| local pr_number="" | |
| pr_number=$(echo "$pr_url" | grep -oE '[0-9]+$' || echo "") | |
| if [[ -n "$pr_number" ]]; then | |
| local pr_state="" | |
| 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 "") | |
| close_comment="Verified: PR #$pr_number ($pr_state). Task $task_id: $old_state -> $new_state" | |
| fi | |
| else | |
| close_comment="Task $task_id reached state: $new_state (from $old_state). No PR on record — verify deliverables manually." | |
| fi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.agents/scripts/supervisor/issue-sync.sh:
- Around line 407-431: The proof-log logic should treat an unusable PR URL or a
failed gh query as "no usable PR" and avoid empty parentheticals: after
extracting pr_url, if pr_number is empty set close_comment to the "No PR on
record" message (or append a note that the PR URL is unusable) instead of
leaving the default; when running gh pr view, check if pr_state is non-empty
before building close_comment (only include "PR #$pr_number ($pr_state)" when
pr_state is non-empty), otherwise fall back to the "No PR on record — verify
deliverables manually" message or a clear "PR info unavailable" message. Update
the logic around variables pr_url, pr_number, pr_state and the construction of
close_comment (and the gh pr view call) so both edge cases (malformed pr_url and
gh failures) produce a clear fallback comment.
🧹 Nitpick comments (1)
.agents/scripts/supervisor/issue-sync.sh (1)
435-461: Optional: Extract the DB error fetch into a small helper to DRY up cancelled/failed paths.Both blocks perform the same
SELECT error FROM tasks WHERE id=...query and the same empty/null guard. A tiny helper likeget_task_error()would reduce duplication and make future changes (e.g., truncating long errors) single-point.Example helper
# At the top of the function or as a standalone helper _get_task_db_field() { local field="$1" task_id="$2" local val="" val=$(db "$SUPERVISOR_DB" "SELECT $field FROM tasks WHERE id='$(sql_escape "$task_id")';" 2>/dev/null || echo "") if [[ -n "$val" && "$val" != "null" ]]; then echo "$val" fi }
| # Build proof-log comment with PR reference and changed files | ||
| local close_comment | ||
| close_comment="Task $task_id reached state: $new_state (from $old_state)" | ||
| local pr_url="" | ||
| pr_url=$(db "$SUPERVISOR_DB" "SELECT pr_url FROM tasks WHERE id='$(sql_escape "$task_id")';" 2>/dev/null || echo "") | ||
| if [[ -n "$pr_url" && "$pr_url" != "null" ]]; then | ||
| local pr_number="" | ||
| pr_number=$(echo "$pr_url" | grep -oE '[0-9]+$' || echo "") | ||
| if [[ -n "$pr_number" ]]; then | ||
| local pr_state="" | ||
| 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 "") | ||
| close_comment="Verified: PR #$pr_number ($pr_state). Task $task_id: $old_state -> $new_state" | ||
| fi | ||
| fi | ||
| if [[ -z "$pr_url" || "$pr_url" == "null" ]]; then | ||
| close_comment="Task $task_id reached state: $new_state (from $old_state). No PR on record — verify deliverables manually." | ||
| fi | ||
| # Close the issue with proof-log comment | ||
| gh issue close "$issue_number" --repo "$repo_slug" \ | ||
| --comment "Task $task_id reached state: $new_state (from $old_state)" 2>/dev/null || true | ||
| --comment "$close_comment" 2>/dev/null || true | ||
| # Add status:done and remove all other status labels | ||
| gh issue edit "$issue_number" --repo "$repo_slug" \ | ||
| --add-label "status:done" "${remove_args[@]}" 2>/dev/null || true | ||
| log_verbose "sync_issue_status_label: closed #$issue_number ($task_id -> $new_state)" | ||
| log_verbose "sync_issue_status_label: closed #$issue_number ($task_id -> $new_state) proof: ${pr_url:-none}" |
There was a problem hiding this comment.
Proof-log fallback has a gap when PR URL exists but details can't be fetched.
Two edge cases in the verified/deployed path:
-
pr_urlis non-empty butpr_numberextraction fails (e.g., malformed URL without trailing digits): the code skips lines 415–420 and falls to line 422 wherepr_urlis not empty, so the "No PR on record" warning is not set. You end up with the generic default from line 409 — no proof-log, no warning. Consider restructuring so the "No PR on record" fallback also covers "PR URL present but unusable." -
gh pr viewfails (API error, network issue):pr_stateis empty, and line 419 produces"Verified: PRfeat(plan+): add granular bash permissions for file discovery #123(). Task ..."— an empty parenthetical. A guard onpr_statebeing non-empty would keep the comment clean.
Suggested restructure
local close_comment
- close_comment="Task $task_id reached state: $new_state (from $old_state)"
local pr_url=""
pr_url=$(db "$SUPERVISOR_DB" "SELECT pr_url FROM tasks WHERE id='$(sql_escape "$task_id")';" 2>/dev/null || echo "")
+ close_comment=""
if [[ -n "$pr_url" && "$pr_url" != "null" ]]; then
local pr_number=""
pr_number=$(echo "$pr_url" | grep -oE '[0-9]+$' || echo "")
if [[ -n "$pr_number" ]]; then
local pr_state=""
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 "")
- close_comment="Verified: PR #$pr_number ($pr_state). Task $task_id: $old_state -> $new_state"
+ if [[ -n "$pr_state" ]]; then
+ close_comment="Verified: PR #$pr_number ($pr_state). Task $task_id: $old_state -> $new_state"
+ fi
fi
fi
- if [[ -z "$pr_url" || "$pr_url" == "null" ]]; then
+ if [[ -z "$close_comment" && ( -z "$pr_url" || "$pr_url" == "null" ) ]]; then
close_comment="Task $task_id reached state: $new_state (from $old_state). No PR on record — verify deliverables manually."
fi
+ if [[ -z "$close_comment" ]]; then
+ close_comment="Task $task_id reached state: $new_state (from $old_state)"
+ fi🤖 Prompt for AI Agents
In @.agents/scripts/supervisor/issue-sync.sh around lines 407 - 431, The
proof-log logic should treat an unusable PR URL or a failed gh query as "no
usable PR" and avoid empty parentheticals: after extracting pr_url, if pr_number
is empty set close_comment to the "No PR on record" message (or append a note
that the PR URL is unusable) instead of leaving the default; when running gh pr
view, check if pr_state is non-empty before building close_comment (only include
"PR #$pr_number ($pr_state)" when pr_state is non-empty), otherwise fall back to
the "No PR on record — verify deliverables manually" message or a clear "PR info
unavailable" message. Update the logic around variables pr_url, pr_number,
pr_state and the construction of close_comment (and the gh pr view call) so both
edge cases (malformed pr_url and gh failures) produce a clear fallback comment.
1 participant
Summary
Problem
sync_issue_status_label()closed issues with generic comments like "Task t1049.11 reached state: verified" — no PR reference, no proof the work was done. When subtasks don't have TODO.md entries, the issue is the sole knowledge trail. Closing without proof loses traceability.Changes
issue-sync.sh: verified/deployed path looks uppr_urlfrom DB, callsgh pr viewfor state/files, builds proof commenterrorfield from DB as cancellation reasonerrorfield from DB as failure contextCloses #1503
Summary by CodeRabbit