fix(pull-request skill): reply and resolve all threads including non-actionable#3573
Open
PierreBrisorgueil wants to merge 3 commits intomasterfrom
Open
fix(pull-request skill): reply and resolve all threads including non-actionable#3573PierreBrisorgueil wants to merge 3 commits intomasterfrom
PierreBrisorgueil wants to merge 3 commits intomasterfrom
Conversation
…actionable Informational/false-positive threads must be explicitly replied to and resolved before the stop condition is met — not silently skipped. - Loop procedure: add step 6 for non-actionable open threads (reply, resolve) - Stop condition: zero open threads (not just zero actionable)
📝 WalkthroughWalkthroughUpdated the pull-request skill doc to replace the single-pass stop condition with a multi-pass loop using a consecutive_zero counter, explicitly require replying to and resolving non-actionable/informational review threads, clarify CI waiting/retry semantics, and require two consecutive grace-pass confirmations before stopping. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Monitor as PR Monitor
participant GH as GitHub API
participant CI as Branch Protection / CI
Monitor->>GH: Fetch review threads & check statuses
GH-->>Monitor: Return threads (actionable + non-actionable) + CI checks
alt non-actionable/unresolved exist
Monitor->>GH: Post brief reply to each non-actionable thread
Monitor->>GH: Resolve thread via GraphQL
Monitor-->>Monitor: Increment loop, reset consecutive_zero on fixes
Monitor->>GH: Re-fetch threads (loop)
else zero unresolved threads (after grace)
Monitor->>CI: Re-check branch protection / CI status
CI-->>Monitor: Return status
alt passes confirmed for two consecutive polls
Monitor-->>Monitor: Stop monitor
else not confirmed
Monitor-->>Monitor: Continue loop (apply grace/retry)
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 (2)
.claude/skills/pull-request/SKILL.md (2)
239-242:⚠️ Potential issue | 🟡 MinorStop condition text contradicts the loop update.
Section 6f still says “zero unresolved actionable threads,” but the loop now stops on “zero open threads.” Please align this stop condition with the new policy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/pull-request/SKILL.md around lines 239 - 242, Update the stop-condition wording in the "6f. Stop condition" section: replace the phrase “zero unresolved actionable threads” with “zero open threads” (or otherwise mirror the loop's stop condition) so the text matches the new policy; ensure the paragraph under the "6f. Stop condition" heading and any nearby references use the exact same term (“zero open threads”) as the loop update.
3-3:⚠️ Potential issue | 🟡 MinorAlign description with new “zero open threads” stop condition.
Line 3 still says “zero actionable threads,” which conflicts with the updated loop logic. Update the description to “zero open threads” to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/pull-request/SKILL.md at line 3, Replace the phrase "zero actionable threads" with "zero open threads" in the PR lifecycle description so it matches the updated loop logic; edit the description line in SKILL.md (the "Full PR lifecycle — branch, commit, issue, draft PR, CI, ready, autonomous monitor loop (fix comments, resolve threads, iterate until CI green and zero actionable threads)." string) to read "...iterate until CI green and zero open threads."
🤖 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 @.claude/skills/pull-request/SKILL.md:
- Around line 239-242: Update the stop-condition wording in the "6f. Stop
condition" section: replace the phrase “zero unresolved actionable threads” with
“zero open threads” (or otherwise mirror the loop's stop condition) so the text
matches the new policy; ensure the paragraph under the "6f. Stop condition"
heading and any nearby references use the exact same term (“zero open threads”)
as the loop update.
- Line 3: Replace the phrase "zero actionable threads" with "zero open threads"
in the PR lifecycle description so it matches the updated loop logic; edit the
description line in SKILL.md (the "Full PR lifecycle — branch, commit, issue,
draft PR, CI, ready, autonomous monitor loop (fix comments, resolve threads,
iterate until CI green and zero actionable threads)." string) to read
"...iterate until CI green and zero open threads."
There was a problem hiding this comment.
Pull request overview
This PR fixes a workflow issue in the pull-request skill where non-actionable review threads (e.g., informational comments, approvals, false positives) were being skipped instead of replied to and resolved, leaving open threads on merged PRs. The solution updates the autonomous monitor loop to require handling ALL thread types and changes the stop condition from "zero actionable comments" to "zero open threads."
Changes:
- Added new step 6 in the monitor loop to handle non-actionable threads by replying and resolving them
- Renumbered the stop condition from step 6 to step 7 and updated it to check for "zero open threads" instead of "zero actionable comments"
- Updated the "Informational" category definition to clarify that these threads must be replied to and resolved (not skipped)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3573 +/- ##
=======================================
Coverage 95.93% 95.93%
=======================================
Files 20 20
Lines 516 516
Branches 140 140
=======================================
Hits 495 495
Misses 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…tency - Terminology: 'open threads' → 'unresolved threads' throughout loop procedure - Step 6: add 'all' to match step 5 explicitness (reply all, resolve all) - Informational section: reformat as bullet list, clarify review threads vs PR-level comments
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 (2)
.claude/skills/pull-request/SKILL.md (2)
3-3:⚠️ Potential issue | 🟠 MajorAlign summary description with new stop condition (“zero unresolved threads”).
The description still says “zero actionable threads,” which conflicts with the updated loop. Update to “zero unresolved threads” to reflect the new behavior.
Proposed fix
-description: Full PR lifecycle — branch, commit, issue, draft PR, CI, ready, autonomous monitor loop (fix comments, resolve threads, iterate until CI green and zero actionable threads). +description: Full PR lifecycle — branch, commit, issue, draft PR, CI, ready, autonomous monitor loop (fix comments, resolve threads, iterate until CI green and zero unresolved threads).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/pull-request/SKILL.md at line 3, Update the description line in SKILL.md to match the new stop condition by replacing the phrase "zero actionable threads" with "zero unresolved threads" (edit the value for the description key shown in the diff); ensure the single-line summary now reads "...CI, ready, autonomous monitor loop (fix comments, resolve threads, iterate until CI green and zero unresolved threads)."
244-245:⚠️ Potential issue | 🟠 MajorStop condition still references “unresolved actionable” instead of “unresolved.”
Section 6f contradicts the new loop logic by limiting to “unresolved actionable threads.” This can cause the loop to stop while non-actionable unresolved threads remain. Update to “zero unresolved threads.”
Proposed fix
-All CI checks pass **and** a complete polling pass (after the grace period) produces **zero unresolved actionable threads**. +All CI checks pass **and** a complete polling pass (after the grace period) produces **zero unresolved threads**.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/pull-request/SKILL.md around lines 244 - 245, Update the stop condition text in Section 6f to match the loop logic by replacing the phrase "zero unresolved actionable threads" with "zero unresolved threads" so the stop condition no longer excludes non-actionable unresolved threads; locate the Section 6f wording (search for "unresolved actionable threads" or the stop condition sentence) and change it to explicitly state "zero unresolved threads."
🤖 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 @.claude/skills/pull-request/SKILL.md:
- Line 3: Update the description line in SKILL.md to match the new stop
condition by replacing the phrase "zero actionable threads" with "zero
unresolved threads" (edit the value for the description key shown in the diff);
ensure the single-line summary now reads "...CI, ready, autonomous monitor loop
(fix comments, resolve threads, iterate until CI green and zero unresolved
threads)."
- Around line 244-245: Update the stop condition text in Section 6f to match the
loop logic by replacing the phrase "zero unresolved actionable threads" with
"zero unresolved threads" so the stop condition no longer excludes
non-actionable unresolved threads; locate the Section 6f wording (search for
"unresolved actionable threads" or the stop condition sentence) and change it to
explicitly state "zero unresolved threads."
…ing review checks - Loop procedure: add consecutive_zero counter (X=2) — stop only after 2 consecutive passes with zero unresolved threads, catching slow bots - Step 4: re-check gh pr checks before reading threads — if any check still pending, loop back to grace period (generic, bot-agnostic) - Stop condition (6f): updated to reflect double-pass requirement
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 (2)
.claude/skills/pull-request/SKILL.md (2)
205-205:⚠️ Potential issue | 🟠 MajorActionable detection can miss carry-over unresolved threads.
The definition on Line 205 scopes “New actionable” to threads not yet replied to in the current pass. An unresolved actionable thread from a prior pass (already replied) can be skipped by both step 6 and step 7. Recommend classifying actionable by unresolved state/content, not reply recency.
Suggested doc fix
-**New actionable** = unresolved review threads not yet replied to in this pass. +**Actionable in current pass** = all unresolved review threads that require code or test changes, regardless of whether they were replied to in a previous pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/pull-request/SKILL.md at line 205, Update the "New actionable" definition so it detects any unresolved review threads based on their unresolved state/content rather than whether they were replied to in the current pass; change the logic referenced by "New actionable" and the checks in step 6 and step 7 to evaluate thread.unresolved (or equivalent unresolved flag/state) and thread content instead of reply recency, ensuring carry-over unresolved threads from prior passes are classified as actionable.
3-3:⚠️ Potential issue | 🟡 MinorTop-level description is now stale vs. loop behavior.
Line 3 still says “zero actionable threads,” but the updated logic requires zero unresolved threads (with two consecutive passes). Please align this line to avoid operator confusion.
Suggested doc fix
-description: Full PR lifecycle — branch, commit, issue, draft PR, CI, ready, autonomous monitor loop (fix comments, resolve threads, iterate until CI green and zero actionable threads). +description: Full PR lifecycle — branch, commit, issue, draft PR, CI, ready, autonomous monitor loop (fix comments, resolve threads, iterate until CI green and zero unresolved threads).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/pull-request/SKILL.md at line 3, Update the top-level description in SKILL.md that currently reads “zero actionable threads” to reflect the new loop requirement: change the phrase to “zero unresolved threads (after two consecutive passes)” so it matches the implementation that requires two consecutive verification passes; locate the exact sentence containing “zero actionable threads” and replace it with the new wording, keeping punctuation and tense consistent with the surrounding description.
🤖 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 @.claude/skills/pull-request/SKILL.md:
- Line 205: Update the "New actionable" definition so it detects any unresolved
review threads based on their unresolved state/content rather than whether they
were replied to in the current pass; change the logic referenced by "New
actionable" and the checks in step 6 and step 7 to evaluate thread.unresolved
(or equivalent unresolved flag/state) and thread content instead of reply
recency, ensuring carry-over unresolved threads from prior passes are classified
as actionable.
- Line 3: Update the top-level description in SKILL.md that currently reads
“zero actionable threads” to reflect the new loop requirement: change the phrase
to “zero unresolved threads (after two consecutive passes)” so it matches the
implementation that requires two consecutive verification passes; locate the
exact sentence containing “zero actionable threads” and replace it with the new
wording, keeping punctuation and tense consistent with the surrounding
description.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.claude/skills/pull-request/SKILL.md— loop step 6 now requires replying to and resolving all open threads (including non-actionable/informational ones); stop condition updated to zero open threads.Scope
.claude/skills/pull-request/SKILL.md(skill definition only)nonelowValidation
npm run lintnpm run test:unitnpm run buildGuardrails check
.env*,secrets/**, keys, tokens)Notes for reviewers
Summary by CodeRabbit