Skip to content

fix: prevent auto-closing failed issues and preserve em-dash content #2112

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

Closed
marcusquinn wants to merge 1 commit into from
Closed

fix: prevent auto-closing failed issues and preserve em-dash content #2112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 16 additions & 3 deletions .agents/scripts/issue-sync-lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,23 @@ parse_task_line() {
local task_id
task_id=$(echo "$line" | grep -oE 't[0-9]+(\.[0-9]+)*' | head -1 || echo "")

# Extract description (between task ID and first metadata field)
# Extract FULL description (everything after task ID, including em-dash content)
# We'll clean out metadata fields separately, but preserve the full text
local full_text
full_text=$(echo "$line" | sed -E 's/^[[:space:]]*- \[.\] t[0-9]+(\.[0-9]+)* //' || echo "")

# Extract description by removing all metadata fields but keeping em-dash content
# Strategy: remove tags, estimate, and key:value pairs, but preserve text after em dash
local description
description=$(echo "$line" | sed -E 's/^[[:space:]]*- \[.\] t[0-9]+(\.[0-9]+)* //' |
sed -E 's/ (#[a-z]|~[0-9]|→ |logged:|started:|completed:|ref:|actual:|blocked-by:|blocks:|assignee:|verified:).*//' ||
description=$(echo "$full_text" |
sed -E 's/ #[a-z][a-z0-9-]*//g' | # Remove tags
sed -E 's/ ~[0-9]+[hmd]( \(ai:[^)]+\))?//g' | # Remove estimate
sed -E 's/ (logged|started|completed|actual|verified):[^ ]+//g' | # Remove date fields
sed -E 's/ (assignee|blocked-by|blocks|model|category):[^ ]+//g' | # Remove other metadata
sed -E 's/ ref:GH#[0-9]+//g' | # Remove GH ref
sed -E 's/ pr:#[0-9]+//g' | # Remove PR ref
sed -E 's/ → \[todo\/PLANS\.md#[^]]+\]//g' | # Remove plan link
sed -E 's/^[[:space:]]+//; s/[[:space:]]+$//' || # Trim whitespace
echo "")
Comment on lines +91 to 100
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Feb 21, 2026

Choose a reason for hiding this comment

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

medium

Chaining multiple sed commands via pipes can be inefficient as each one starts a new process. You can consolidate these into a single sed call using the -e option for each expression. This improves both performance and readability.

Suggested change
description=$(echo "$full_text" |
sed -E 's/ #[a-z][a-z0-9-]*//g' | # Remove tags
sed -E 's/ ~[0-9]+[hmd]( \(ai:[^)]+\))?//g' | # Remove estimate
sed -E 's/ (logged|started|completed|actual|verified):[^ ]+//g' | # Remove date fields
sed -E 's/ (assignee|blocked-by|blocks|model|category):[^ ]+//g' | # Remove other metadata
sed -E 's/ ref:GH#[0-9]+//g' | # Remove GH ref
sed -E 's/ pr:#[0-9]+//g' | # Remove PR ref
sed -E 's/ → \[todo\/PLANS\.md#[^]]+\]//g' | # Remove plan link
sed -E 's/^[[:space:]]+//; s/[[:space:]]+$//' || # Trim whitespace
echo "")
description=$(echo "$full_text" | sed -E \
-e 's/ #[a-z][a-z0-9-]*//g' \
-e 's/ ~[0-9]+[hmd]( \(ai:[^)]+\))?//g' \
-e 's/ (logged|started|completed|actual|verified):[^ ]+//g' \
-e 's/ (assignee|blocked-by|blocks|model|category):[^ ]+//g' \
-e 's/ ref:GH#[0-9]+//g' \
-e 's/ pr:#[0-9]+//g' \
-e 's/ → \[todo\/PLANS\.md#[^]]+\]//g' \
-e 's/^[[:space:]]+//; s/[[:space:]]+$//' ||
echo "")
References
  1. Optimize shell script pipelines by replacing 'grep | sed' combinations with a single, more efficient 'sed' command where possible to improve performance. This principle extends to consolidating multiple 'sed' commands for efficiency.

Comment on lines +83 to 100
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 21, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

ref:todo/tasks/ metadata not stripped from description.

The sanitization pipeline removes ref:GH#, pr:#, and plan links, but ref:todo/tasks/ references (used in find_related_files at line 546) are not stripped. Any task line containing ref:todo/tasks/prd.md will have that text leak into the generated issue description.

🛠️ Proposed fix — add `ref:todo/tasks/` pattern to the pipeline 
 		sed -E 's/ ref:GH#[0-9]+//g' |                                     # Remove GH ref
 		sed -E 's/ pr:#[0-9]+//g' |                                        # Remove PR ref
+		sed -E 's/ ref:todo\/tasks\/[^ ]+//g' |                            # Remove task file ref
 		sed -E 's/ → \[todo\/PLANS\.md#[^]]+\]//g' |                       # Remove plan link
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.agents/scripts/issue-sync-lib.sh around lines 83 - 100, The description
sanitization pipeline fails to strip "ref:todo/tasks/..." references; update the
pipeline that assigns description (the sed chain that currently removes
"ref:GH#...", "pr:#", and plan links) to also remove patterns like
"ref:todo/tasks/..." by adding a sed rule that strips "
ref:todo/tasks/<non-space>" (e.g., a sed -E replacement for " ref:todo/tasks/[^
]+") into the same chain that produces the description variable so
find_related_files and generated issue descriptions no longer leak those refs.


# Extract tags
Expand Down
13 changes: 7 additions & 6 deletions .agents/scripts/supervisor/issue-sync.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,13 +457,14 @@ sync_issue_status_label() {
if [[ -n "$fail_error" && "$fail_error" != "null" ]]; then
fail_comment="Task $task_id failed (was: $old_state). Error: $fail_error"
fi
# Close with failure comment but don't add status:done
gh issue close "$issue_number" --repo "$repo_slug" \
--comment "$fail_comment" 2>/dev/null || true
# DO NOT auto-close failed tasks - they need human review
# Add needs-review label and post failure comment, but keep issue OPEN
gh issue comment "$issue_number" --repo "$repo_slug" \
--body "$fail_comment" 2>/dev/null || true
Comment on lines +462 to +463
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Feb 21, 2026

Choose a reason for hiding this comment

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

medium

Using 2>/dev/null suppresses all error messages from the gh command, which can hide important issues like authentication failures, network problems, or invalid issue numbers. The || true already prevents the script from exiting on failure. According to the repository style guide (line 50) and repository rules, 2>/dev/null should not be used for blanket error suppression. Please remove it to allow errors to be visible for debugging.

Suggested change
gh issue comment "$issue_number" --repo "$repo_slug" \
--body "$fail_comment" 2>/dev/null || true
gh issue comment "$issue_number" --repo "$repo_slug" \
--body "$fail_comment" || true
References
  1. Line 50: 2>/dev/null is acceptable ONLY when redirecting to log files, not blanket suppression. (link)
  2. 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.
  3. In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.

gh issue edit "$issue_number" --repo "$repo_slug" \
"${remove_args[@]}" 2>/dev/null || true
log_verbose "sync_issue_status_label: closed #$issue_number as failed ($task_id)"
return 0
--add-label "needs-review" "${remove_args[@]}" 2>/dev/null || true
Comment on lines 464 to +465
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Feb 21, 2026

Choose a reason for hiding this comment

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

medium

Similar to the previous comment, using 2>/dev/null here suppresses potentially important error messages from the gh command. This violates the repository style guide (line 50) and repository rules which advise against blanket error suppression. Please remove 2>/dev/null to aid in debugging potential issues.

Suggested change
gh issue edit "$issue_number" --repo "$repo_slug" \
"${remove_args[@]}" 2>/dev/null || true
log_verbose "sync_issue_status_label: closed #$issue_number as failed ($task_id)"
return 0
--add-label "needs-review" "${remove_args[@]}" 2>/dev/null || true
gh issue edit "$issue_number" --repo "$repo_slug" \
--add-label "needs-review" "${remove_args[@]}" || true
References
  1. Line 50: 2>/dev/null is acceptable ONLY when redirecting to log files, not blanket suppression. (link)
  2. 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.
  3. In shell scripts with 'set -e' enabled, use '|| true' to prevent the script from exiting when a command like 'jq' fails on an optional lookup. Do not suppress stderr with '2>/dev/null' so that actual syntax or system errors remain visible for debugging.

log_verbose "sync_issue_status_label: flagged #$issue_number for review ($task_id failed)"
# Don't return here - let the non-terminal state logic handle label updates
;;
Comment on lines +460 to 468
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 21, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

needs-review label will silently fail to apply if it doesn't exist on the repo.

GitHub CLI's --add-label requires the label to already exist in the repository — it will error (silently swallowed by 2>/dev/null || true) if needs-review hasn't been created. Since this label is the core deliverable of the PR fix, a silent failure means failed tasks appear to stay open but carry no needs-review signal.

The established in-file pattern (add_model_label, lines 178–184) always calls gh label create --force before gh issue edit --add-label. needs-review also needs to be added to ALL_STATUS_LABELS (line 344) so it is cleaned up in subsequent state transitions — right now it will persist as a zombie label when a failed task is re-queued, retried, and eventually succeeds/closes.

🛠️ Proposed fix

In ensure_status_labels (line 21+), add:

 	gh label create "status:done" --repo "$repo_slug" --color "6F42C1" --description "Task is complete" --force 2>/dev/null || true
+	gh label create "needs-review" --repo "$repo_slug" --color "E4E669" --description "Failed task needs human review" --force 2>/dev/null || true
 	return 0

In ALL_STATUS_LABELS (line 344):

-ALL_STATUS_LABELS="status:available,status:queued,status:claimed,status:in-review,status:blocked,status:verify-failed,status:needs-testing,status:done"
+ALL_STATUS_LABELS="status:available,status:queued,status:claimed,status:in-review,status:blocked,status:verify-failed,status:needs-testing,status:done,needs-review"
🤖 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 460 - 468, The script
currently attempts to add the "needs-review" label with gh issue edit which will
silently fail if the label doesn't exist; update the label creation flow by
ensuring the "needs-review" label is created (use the existing pattern that
calls gh label create --force before gh issue edit, e.g. in ensure_status_labels
or the same place add_model_label is invoked) so the --add-label call cannot
fail, and add "needs-review" to the ALL_STATUS_LABELS array so it is cleaned up
during subsequent state transitions (reference ensure_status_labels and
ALL_STATUS_LABELS to locate where to add the create call and the new label
entry).

Comment on lines +467 to 468
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Feb 21, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Misleading fall-through comment — non-terminal label logic is a no-op for failed.

The comment says "let the non-terminal state logic handle label updates", but state_to_status_label("failed") returns "" (line 333), so new_label is empty and both guarded blocks at lines 484–488 and 492–500 (if [[ -n "$new_label" ]]) are skipped entirely. The fall-through is harmless but the comment misrepresents the intent; it matches the blocked case only because blocked gets a non-empty label. Update the comment to reflect the actual behaviour.

✏️ Proposed fix 
-		# Don't return here - let the non-terminal state logic handle label updates
+		# Return early — failed is a terminal state; label and comment already applied above.
+		return 0

Or, if intentional fall-through is desired for future flexibility, correct the comment:

-		# Don't return here - let the non-terminal state logic handle label updates
+		# Fall through intentionally (new_label is empty for failed; no further label is applied)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Don't return here - let the non-terminal state logic handle label updates
;;
# Return early — failed is a terminal state; label and comment already applied above.
return 0
;;
🤖 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 467 - 468, The comment
on the fall-through after the "failed" case is misleading because
state_to_status_label("failed") returns an empty string so the subsequent
non-terminal label logic is a no-op; update the comment near the fall-through to
state that the fall-through is intentional to let the "blocked" branch (which
yields a non-empty label) reuse the label-update logic and that "failed"
produces no label, or alternatively change the comment to say "fall-through to
shared label handling; no-op for failed (empty label)"; reference
state_to_status_label, the "failed" and "blocked" cases, and the guarded blocks
that check if [[ -n "$new_label" ]] to guide where to change the text.

blocked)
# Read the error/blocked reason from DB
Expand Down
Loading