fix(pull-request skill): reply and resolve all threads including non-actionable#3167
Open
PierreBrisorgueil wants to merge 4 commits intomasterfrom
Open
fix(pull-request skill): reply and resolve all threads including non-actionable#3167PierreBrisorgueil wants to merge 4 commits intomasterfrom
PierreBrisorgueil wants to merge 4 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 monitoring loop to require explicit replies and resolution for all open threads (including non-actionable informational/false-positive threads); introduced a Changes
Sequence Diagram(s)sequenceDiagram
actor Monitor as Monitor Loop
participant CI as CI System
participant PR as Pull Request (Threads)
participant GH as Branch Protection Check
Monitor->>CI: wait for CI results
alt CI failed
CI-->>Monitor: failure
Monitor->>Monitor: clear consecutive_zero, retry (goto CI)
else CI passed
CI-->>Monitor: success
Monitor->>PR: list open threads
alt any open threads
Monitor->>PR: handle actionable (fix/commit/resolve)
Monitor->>PR: handle non-actionable (reply explaining, resolve)
Monitor->>Monitor: reset consecutive_zero, loop back to CI/polling
else zero open threads
Monitor->>Monitor: increment consecutive_zero
alt consecutive_zero threshold reached
Monitor->>GH: check branch protection
GH-->>Monitor: status
Monitor->>PR: STOP ✓
else
Monitor->>Monitor: wait grace, continue loop
end
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3167 +/- ##
=======================================
Coverage 89.86% 89.86%
=======================================
Files 52 52
Lines 1164 1164
Branches 234 234
=======================================
Hits 1046 1046
Misses 107 107
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Updates the pull-request skill documentation so the monitor loop explicitly handles (replies to and resolves) non-actionable review threads, preventing PRs from being left with unresolved conversations that require manual cleanup.
Changes:
- Adds an explicit loop step to reply to + resolve non-actionable open threads before stopping.
- Updates the “Informational” feedback definition to require replying and resolving (instead of skipping).
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/pull-request/SKILL.md:
- Around line 130-131: Update the document to consistently use the "zero open
threads" stop-condition: change the top description (Line 3), section 6f (Line
241), and any other occurrences that currently read "zero actionable threads" or
"zero unresolved actionable threads" to read "zero open threads" so the loop
termination is unambiguous; ensure Step 7's wording and cross-references are
identical to the new phrase.
Section 6f and the frontmatter description still referenced 'zero unresolved actionable threads' — now updated to 'zero open threads' to match the loop procedure step 7.
…PR-level comments Approvals and bot reports (codecov, etc.) are PR-level comments, not resolvable review threads. Clarify that 'resolve' only applies to unresolved review threads, and that PR-level comments cannot be resolved via the GitHub thread API.
…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.
🧹 Nitpick comments (1)
.claude/skills/pull-request/SKILL.md (1)
130-130: Use “pending status checks” instead of “pending review checks.”
gh pr checksrefers to CI/status checks; the current label is slightly ambiguous in an operator runbook.✏️ Suggested wording tweak
- 4. Re-check pending review checks → gh pr checks <number> — if any still pending, GOTO 3 + 4. Re-check pending status checks → gh pr checks <number> — if any still pending, GOTO 3🤖 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 130, Update the step text that currently reads "Re-check pending review checks → gh pr checks <number> — if any still pending, GOTO 3" to use "pending status checks" instead of "pending review checks" so it accurately refers to CI/status checks (i.e., change the phrase "pending review checks" to "pending status checks" in the step string).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/skills/pull-request/SKILL.md:
- Line 130: Update the step text that currently reads "Re-check pending review
checks → gh pr checks <number> — if any still pending, GOTO 3" to use "pending
status checks" instead of "pending review checks" so it accurately refers to
CI/status checks (i.e., change the phrase "pending review checks" to "pending
status checks" in the step string).
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
Scope
.claude/skills/pull-request/SKILL.mdnonelowValidation
npm run lintnpm testGuardrails check
.env*,secrets/**, keys, tokens)Notes for reviewers
Summary by CodeRabbit