t1158: Fix audit script PR linkage detection for auto-reaped tasks#2197
t1158: Fix audit script PR linkage detection for auto-reaped tasks#2197alex-solovyev merged 1 commit intomainfrom
Conversation
…inkage for auto-reaped tasks (t1158)
Root cause: audit_closed_issues() called find_closing_pr("", task_id, repo_slug)
with an empty first arg. find_closing_pr uses arg1 to grep for pr:#NNN from
TODO.md task lines. Empty input meant Check 1/2 always failed, and Check 3
(platform search) was unavailable in the audit context (_DETECTED_PLATFORM unset).
For tasks auto-reaped from supervisor DB ("stuck in evaluating >10m"), the DB
cross-check (t1156) also returned not_found since the task record was removed.
This left no working path to detect PR linkage, causing false positives for
t1141, t1129, t1126, t1128 — all verified with PRs in TODO.md.
Fix: (1) Pass ${task_line:-} instead of "" to find_closing_pr so it can extract
pr:#NNN from TODO.md. (2) Add fallback: if has_evidence is true but merged_pr
is still empty, extract PR number directly from task_line as last resort.
Summary of ChangesHello @alex-solovyev, 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 a bug in the 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
|
WalkthroughEnhanced the PR linkage logic in the issue-audit script by passing task_line to find_closing_pr and adding a fallback mechanism. When standard linkage fails but completion evidence exists in TODO.md, the script now extracts the PR number directly from task_line to populate merged_pr. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: Mon Feb 23 21:06:29 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
The pull request correctly addresses the issue where PR linkage detection failed for auto-reaped tasks. By passing the task_line to find_closing_pr and adding a fallback extraction mechanism, it significantly reduces false positives in the audit report. The changes are minimal and follow the existing patterns in the script.
| if [[ -n "$task_id" ]]; then | ||
| local pr_info="" | ||
| pr_info=$(find_closing_pr "" "$task_id" "$repo_slug" 2>/dev/null || echo "") | ||
| pr_info=$(find_closing_pr "${task_line:-}" "$task_id" "$repo_slug" 2>/dev/null || echo "") |
There was a problem hiding this comment.
Avoid using 2>/dev/null for blanket suppression of command errors. This can hide important diagnostic information such as authentication failures or system issues in find_closing_pr or the tools it invokes (e.g., gh or tea).
| pr_info=$(find_closing_pr "${task_line:-}" "$task_id" "$repo_slug" 2>/dev/null || echo "") | |
| pr_info=$(find_closing_pr "${task_line:-}" "$task_id" "$repo_slug" || echo "") |
References
- 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/scripts/supervisor/issue-audit.sh (1)
247-263:⚠️ Potential issue | 🟡 Minor
task_lineis declared inside the conditional but referenced outside it — move the initialisation up
local task_line=""lives on line 247, insideif [[ -f "$todo_file" && -n "$task_id" ]]; then. This PR now referencestask_lineoutside that block at line 263 (find_closing_pr "${task_line:-}") and lines 285–292.While practically safe today —
task_idis always non-empty here (earlycontinueat line 214) so the condition collapses to[[ -f "$todo_file" ]], which is invariant across iterations — it creates a subtle invariant that isn't obvious. Any future edit that makes the condition non-constant (e.g., adding a per-issue guard) would silently stale-leaktask_linefrom a previous iteration intofind_closing_prand the fallback, producing incorrect PR linkage.🛠️ Proposed fix — hoist the initialisation before the `if` block
- # Check 2: Does TODO.md have completion evidence for this task? - local has_evidence=false - if [[ -f "$todo_file" && -n "$task_id" ]]; then - local task_line="" - task_line=$(grep -E "^\s*- \[.\] ${task_id} " "$todo_file" | head -1 || echo "") + # Check 2: Does TODO.md have completion evidence for this task? + local has_evidence=false + local task_line="" + if [[ -f "$todo_file" && -n "$task_id" ]]; then + task_line=$(grep -E "^\s*- \[.\] ${task_id} " "$todo_file" | head -1 || echo "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/issue-audit.sh around lines 247 - 263, task_line is declared inside the if-block but used later by find_closing_pr and other code, which risks leaking a stale value; hoist the declaration/initialization of local task_line (e.g., local task_line="") to immediately before the if [[ -f "$todo_file" && -n "$task_id" ]]; then block so task_line is always defined per-iteration, keep all existing uses of task_line (find_closing_pr "${task_line:-}" "$task_id" "$repo_slug" and later checks) unchanged.
🧹 Nitpick comments (1)
.agents/scripts/supervisor/issue-audit.sh (1)
282-292: Fallback duplicatesfind_closing_prCheck 1 and is effectively dead code in the normal flowThe grep at line 287 is identical to
find_closing_prCheck 1 inissue-sync-lib.sh(lines 1095–1099):todo_pr=$(echo "$task_line" | grep -oE 'pr:#[0-9]+' | head -1 | grep -oE '[0-9]+' ...)Because the primary fix at line 263 now passes
task_linetofind_closing_pr, Check 1 will extractpr:#NNN`` and populatemerged_prbefore this fallback is ever reached. The only scenario where the fallback activates is a catastrophic subshell failure of `find_closing_pr` itself (which `|| echo ""` already suppresses to an empty result).If the intent is purely defensive, a short comment to that effect would be clearer than duplicating the library's extraction logic inline:
- # t1158: Fallback — if TODO.md has completion evidence (pr:`#NNN`) but merged_pr - # is still empty (e.g., find_closing_pr failed, platform search unavailable), - # extract PR number directly from task_line as last resort. - if [[ "$has_evidence" == "true" && -z "$merged_pr" && -n "${task_line:-}" ]]; then - local fallback_pr="" - fallback_pr=$(printf '%s' "$task_line" | grep -oE 'pr:#[0-9]+' | head -1 | grep -oE '[0-9]+' || echo "") - if [[ -n "$fallback_pr" ]]; then - merged_pr="$fallback_pr" - log_verbose "audit_closed_issues: task $task_id — extracted PR #$fallback_pr from TODO.md as fallback" - fi - fi + # Defensive: find_closing_pr with task_line (line 263) already handles pr:`#NNN` extraction + # via Check 1 in issue-sync-lib.sh. This block is only reached on subshell failure.Alternatively, the block can be kept as-is with the understanding that it is dead under normal conditions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/supervisor/issue-audit.sh around lines 282 - 292, This fallback block duplicates the Check 1 extraction performed by find_closing_pr (called earlier) and is effectively dead in normal flow; either remove the block or keep it but add a short defensive comment above it clarifying that this duplicates find_closing_pr's 'pr:#[0-9]+' extraction and exists only to guard against catastrophic subshell/library failure—referencing the symbols merged_pr, task_line, task_id and the find_closing_pr behavior so future readers know it’s intentional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.agents/scripts/supervisor/issue-audit.sh:
- Around line 247-263: task_line is declared inside the if-block but used later
by find_closing_pr and other code, which risks leaking a stale value; hoist the
declaration/initialization of local task_line (e.g., local task_line="") to
immediately before the if [[ -f "$todo_file" && -n "$task_id" ]]; then block so
task_line is always defined per-iteration, keep all existing uses of task_line
(find_closing_pr "${task_line:-}" "$task_id" "$repo_slug" and later checks)
unchanged.
---
Nitpick comments:
In @.agents/scripts/supervisor/issue-audit.sh:
- Around line 282-292: This fallback block duplicates the Check 1 extraction
performed by find_closing_pr (called earlier) and is effectively dead in normal
flow; either remove the block or keep it but add a short defensive comment above
it clarifying that this duplicates find_closing_pr's 'pr:#[0-9]+' extraction and
exists only to guard against catastrophic subshell/library failure—referencing
the symbols merged_pr, task_line, task_id and the find_closing_pr behavior so
future readers know it’s intentional.
1 participant
Summary
audit_closed_issues()calledfind_closing_pr("", ...)with empty task_line, so PR number extraction from TODO.md always failedno_pr_linkagefindings for auto-reaped tasks (t1141, t1129, t1126, t1128) that had validpr:#NNNin TODO.md but were removed from supervisor DB${task_line:-}tofind_closing_pr+ add fallback PR extraction from task_line whenhas_evidenceis trueChanges
issue-audit.sh:260— Pass task_line to find_closing_pr instead of empty stringissue-audit.sh:282-292— Add fallback: extract PR number from task_line when has_evidence=true but merged_pr emptyVerification
Ref #1744
Summary by CodeRabbit