Repository navigation
fix(copilot-review,review-pipeline): reply after the fix lands - #240
Merged
Merged
Conversation
…n a changed disposition The Copilot phase replied to each review thread before applying the fix the reply reported, and a thread could take only one reply, so a finding re-triaged afterwards was never reported again. review-pipeline now replies after the fix is pushed and before the re-review is requested, and once more after the loop for the round that ends it. copilot-review defines when a thread is due a reply: it has none, or its latest reply does not name the disposition its finding now holds. The listing no longer filters on resolved or replied state. Copilot resolves its own threads during a re-review, so a filter on resolved state would hide the threads a changed disposition sits on. An empty set of due threads posts nothing instead of stopping to ask. list-pr-threads.sh emits last_reply_body so the definition can be evaluated from the thread itself, and pr-with-copilot-review.sh prints each inline comment's id so a finding is tied to its thread by id. The combined-pipeline diagram in copilot-review is removed: it ordered the reply before the fix and restated a flow review-pipeline owns.
list-pr-threads.sh loses --unresolved and --unreplied: no skill step passes them now that the due-reply rule reads reply_count and last_reply_body from an unfiltered listing. The skill's comment on last_reply_body now carries the bound the script has: it is the latest reply among the comments the script fetched. test-scripts.sh runs both scripts against a stub gh and checks the fields the due-reply rule reads and the id-first line the poll prints.
The due-reply rule gains two conditions. A thread is due only when the run has triaged its finding, so a thread from an earlier review that the run never triaged is not answered with a disposition it does not have. And a thread whose finding is actionable is not due until the fix is pushed, so the order holds whenever the reply step is reached, not only when a caller sequences it after the push. A reply names the disposition its finding now holds, the same value the rule compares the latest reply against. The script test also checks that a removed filter flag is rejected.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The workflow changes are internally consistent and the script contract tests pass.
Review effort: Balanced
Findings: None
What changed in this PR
Updates the Copilot review workflow so replies reflect the current disposition and actionable fixes are pushed before being reported.
Changes:
- Defines when review threads require replies.
- Moves actionable replies after fix pushes and supports repeated replies.
- Adds thread metadata, comment IDs, and contract tests.
| File | Description |
|---|---|
skills/review-pipeline/SKILL.md |
Reorders fix-loop replies. |
skills/copilot-review/SKILL.md |
Defines reply eligibility. |
skills/copilot-review/scripts/test-scripts.sh |
Adds 14 passing contract checks. |
skills/copilot-review/scripts/pr-with-copilot-review.sh |
Includes comment IDs in poll output. |
skills/copilot-review/scripts/list-pr-threads.sh |
Exposes latest replies without state filters. |
.claude-plugin/marketplace.json |
Advances the plugin version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
list-pr-threads.sh asks for each thread's first comment, its last comment and its comment count, in place of its first 50 comments. last_reply_body is the thread's latest reply and reply_count is exact, whatever the thread's length. The script test covers a multi-line and a tab-carrying comment body in the poll's output and an empty reply body in the listing, and its header states only what a run needs.
…sposition A thread was due again whenever its latest comment did not name the finding's disposition, so any comment posted below the run's reply made the run post the same reply once more. The run posts under the user's own account, so the comment's author cannot tell the two apart. The rule now reads the latest reply that names a disposition, and a comment that names none is passed over. list-pr-threads.sh emits reply_bodies, every reply of the thread oldest first, read from all pages of the pull request's review comments. It needs jq, which the skill's prerequisites list.
list-pr-threads.sh handed every reply body to jq as one argument, which fails past the argument-length limit on a pull request with long threads. The replies go through a temporary file. The script test describes one pull request in all its fixtures, serves an unpaginated comment read only its first 8 entries, and checks that a reply past them is read and that each id the poll prints is a head_id of the listing.
pr-with-copilot-review.sh read the first page only, 30 items. Past that, the latest review's inline comments were not printed and a new review was never seen to arrive. The poll lists the Copilot review ids across all pages, takes their count and the last id from that list, fetches that review's summary by id, and filters the inline comments page by page. The script test puts the latest review and its comments past the first page, and covers a pull request with no reply, one with no Copilot review, and a failed reviews read.
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
In the Copilot phase of
review-pipeline, each reply on a review thread was sent before the fix it reported, and a thread could take only one reply. Two harms followed: a reply reported an edit that had not landed, and a finding whose triage disposition changed after its reply was never reported again.A reply to an
actionablefinding now waits until its fix is pushed, and a thread is answered again when the last reply that names a disposition no longer names the one its finding holds.The poll also read only the first page of a pull request's reviews and review comments, 30 items each, so on a long pull request the latest review's inline comments went unprinted. It now reads every page.
Closes #170 Closes #239
Changes
copilot-review§ Respond to review defines when a thread is due a reply. All three must hold: the run has triaged the thread's finding; no reply on the thread names a disposition, or the latest reply that names one does not name the disposition the finding now holds; and the finding is notactionable, or its fix is pushed. A reply names the disposition its finding holds at the time it is sent. When no thread is due, nothing is posted.list-pr-threads.shtakes no--unresolvedor--unrepliedflag, and emitsreply_bodies: the bodies of the thread's replies, oldest first. It reads the replies from every page of the pull request's review comments, and usesjq, whichcopilot-reviewlists as a prerequisite.pr-with-copilot-review.shreads every page of the pull request's reviews and review comments, and prints each inline comment asid<TAB>path:line<TAB>body. That id is the value the listing emits ashead_id, so a triaged finding is tied to its thread by id.review-pipelinePhase 2, the reply sits inside the fix loop, after the push and before the re-review request, and once more after the loop for the review that ends it.copilot-reviewhas no "Combined pipeline with codex-review" section. Its diagram ordered the reply before the fix and restated the review, reply, fix and re-review sequence thatreview-pipelinedefines.skills/copilot-review/scripts/test-scripts.shruns both scripts against a stubgh.metadata.versionin.claude-plugin/marketplace.jsonis2026.10.1.Resolved state is not part of the rule on purpose. Copilot resolves its own threads: GitHub's changelog entry of 2026-09-11, Auto-resolution and analysis updates in Copilot code review, says "When you push a commit that addresses a Copilot code review comment, Copilot now resolves that comment during its rereview." A thread whose fix was pushed is therefore resolved by the next re-review, and a filter on resolved state would hide exactly the threads a later change of disposition sits on.
Impact
review-pipelinePhase 2 is the only caller of § Respond to review.pr-with-copilot-review.sh; the agent that ran it reads it.--unresolvedor--unrepliedtolist-pr-threads.shnow gets a usage error and exit code 2. No skill passes either.Verification
bash skills/copilot-review/scripts/test-scripts.sh: 26 checks, all pass. With--paginatetaken off any one of the three reads that carry it, the test fails.list-pr-threads.sh ultimatile/development-skills 230returns that pull request's four threads withreply_count2, 1, 1, 1, each resolved and each with that manyreply_bodies; the two bodies of the first thread come in the order they were posted. On fix(review-telemetry): record the run's models and skills version #193 it returns one thread withreply_count0 and an emptyreply_bodies.pr-with-copilot-review.sh --pollon fix(code-review-gate): give the headless and user lanes the root's range #234 prints its one inline comment with the id4141092601first, the same value the listing returns as that thread'shead_id.Notes
copilot-review§ Respond to review still requires a reply to state "what the run will do about the finding", though the reply to anactionablefinding now follows the fix. copilot-review: a reply lands before its fix, with no second turn #170 leaves what a reply says out of scope.