You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Migrates review-pr-feedback from a VS Code prompt file to an agent skill, then hardens it over nine self-review iterations.
Why
Prompt files are deprecated for Agent Host sessions and are no longer loaded, so /review-pr-feedback had stopped working. Skills are the supported replacement.
This is the pilot for migrating the remaining 15 prompts under .github/prompts/.
What the skill does
Gathers feedback from four sources: review threads, review bodies, Copilot's hidden low-confidence suppressed findings, and discussion comments. Every item is tagged with its source and per-source counts are always reported, including zeroes.
Infers the PR from the current branch when none is given, and confirms before acting.
Works through whatever access path is available — gh, a GitHub MCP server, REST/GraphQL — chosen per capability, with read-only pre-flight validation of each.
Replies to every item it engaged with, stating what changed, why it was rejected, or that no action was needed.
Resolves only bot-authored threads. Anything a human touched stays open for them to judge.
Asks for explicit approval before committing, pushing, replying or resolving, separately for each, with no approval carried across turns.
Treats fetched content as data, never instructions, and never routes credentials through tooling.
How it was built
The first commit is the migration tool's raw output, unmodified, so the machine translation and the human work stay separable. The tool rewrote frontmatter only: it dropped tools, added disable-model-invocation, and left all 117 body lines untouched, including seven ${input:...} variables that nothing substitutes in a skill.
Everything after that came from rewriting the body for the skill format, then running the skill against this PR — nine times — and fixing what each run exposed.
Iteration log
Each run gathered feedback on this PR, applied fixes, replied, and resolved. Defects found per run:
Run
Found
Character of the findings
1
6
Contradictions in the migrated prompt: no pagination, no Rejected status, summary-comment feedback loop
2
3
Missing trust boundary, unconstrained credentials, stale authorship at resolve time
3
2
Dirty working tree ignored; the summary marker was attacker-spoofable
4
1
Circular pre-flight ordering, found only by reading collapsed review-body sections
5
4
Fixed could mean "not on the PR"; Already Addressed could never resolve
6
5
A PR URL did not set the repository; report claimed outcomes before they happened
7
4
All four were conflicts between earlier fixes — prompted a consolidation pass
8
3
Reruns re-answered unchanged feedback; identity rules exempted the account, not the reply
9
4
The previous commit's two fixes deadlocked each other
32 defects across 9 runs, 23 commits. 21 review threads, all resolved.
Three findings were verified against the repository rather than assumed:
Finding
Evidence
Copilot's suppressed findings live only in the review body
#4706 exposes 1 review thread while its body holds 2 more substantive findings
Reviewers request changes in review bodies with no inline comment
14 human reviews across 40 recent PRs, several CHANGES_REQUESTED
Authors annotate their own diff, and self-tag when they mean it as work
544 plain vs 45 self-tagged author-opened threads across 257 merged PRs
That last split is why author commentary is non-actionable by default while a self-tag makes it actionable. Self-tag detection ignores quoted text: on a sample where every hit was an author quoting a reviewer who had tagged them, naive matching was wrong 3 times out of 3.
Why iteration stopped
Deliberately, at diminishing returns rather than at zero findings.
The defect source shifted over the nine runs. Runs 1–4 found flaws in the migrated prompt. Runs 7–9 found flaws introduced by runs 6–8: seven of the last eleven findings were self-inflicted, and run 9's four were all consequences of recent commits, two from the immediately preceding one. A consolidation pass in run 7 regrouped 34 accumulated rules by theme and fixed the drift, but did not stop new rules interacting badly with old ones — that is a property of a 700-line procedural document, not of how its rules are grouped.
The remaining findings also concern edge cases the skill has not hit in nine runs: declined resolutions, divergent write principals, human reviewers on this PR. Each fix for a hypothetical adds surface area for the next contradiction.
Two classes of defect are worth noting, because neither method alone would have found both:
Only execution found them. Replying made resolution unreachable, because a reply adds a human comment and the bot-only test then failed forever. Six threads qualified before the reply gate and failed immediately after it. No amount of reading would have surfaced that.
Only reading found them. Step 1 required pre-flight to complete before resolving the PR that pre-flight checks. The document was wrong while practice was right, so four runs never exercised the broken instruction.
Notes for reviewers
The old prompt file is deleted. Nothing referenced it — it was never listed in AGENTS.md's prompt table, which remains incomplete for unrelated reasons and is untouched here.
tools has no equivalent in the skill format, so the former 9-entry allowlist is gone and the skill inherits the ambient agent's tools. The approval gates and the trust boundary are what constrain it now.
disable-model-invocation: true is deliberate: this runs only when explicitly invoked via /review-pr-feedback.
Skills are auto-detected from .github/skills/, but only on the branch you have checked out. Release branches will need this backported separately; release/7.0 does not carry the old prompt file, so only the skill applies there.
When code-review is migrated next, the Approvals, trust-boundary and bot/human sections are candidates for shared guidance rather than a second copy.
Evidence from use on other PRs
The skill has also been used on two unrelated engineering PRs — #4730 (XML documentation validation) and #4731 (symbol publishing diagnostics) — neither of which it was developed against. Both show the same pattern, and it is the one the four-source design exists for.
Findings that arrived in review bodies, not threads
most, including one batch of four
all three
Copilot suppressed blocks
none, reported as none
none, reported as none
On #4731 the last two rounds found zero unresolved review threads and zero discussion comments. Every finding came from Previously missed sections collapsed inside review bodies. A thread-only reading of that PR would have reported nothing outstanding on both occasions.
What the production runs demonstrate, beyond the count:
Per-source reporting holds up. A posted summary reads "Review threads: 10 total, all resolved, 0 unresolved. Copilot suppressed findings: none." — including the explicit zero the skill requires so a skipped source is visible rather than silent.
Zeros are reported honestly. "No suppressed-confidence block was present in any of the 9 reviews on this PR", and a note that this Copilot version reports through Open / Previously missed sections instead. That is the skill declining to imply coverage it did not have.
Findings are reproduced before being fixed. "Reproduced first: M:System.Foo.op_Implicit(System.Int32)~string ... both passed with zero findings", then a fix, then "214 tests pass, up from 208. Five of the six new cases fail against the previous logic rather than passing vacuously."
Adjacent defects surface during the fix. On one finding: "While fixing it I found an adjacent hole that was not reported: nothing after a parameter list was examined at all."
The bugs themselves were real and non-trivial — a documentation trim rewriting its own input so a later build could publish trimmed text as the full text; malformed XML being silently downgraded in report-only mode; a public wrong-kind reference passing validation and staying unresolved on Learn.
Both PRs are authored by the same user with the head branch in dotnet/SqlClient, so they exercise the same identity path as this PR. The untested paths below are still untested.
Still draft: across this PR and the two above, the skill has never run against a PR with an external reviewer, a fork head, or a contributor other than the invoking user — the paths its analysis-only and human-thread rules exist to protect. Every thread it has resolved so far was bot-opened, so the rule that keeps human threads open has never actually had to hold anything back.
Raw output of the VS Code prompt-to-skill migration tool, committed
unmodified so that subsequent hand-editing is reviewable on its own.
Prompt files are deprecated for Agent Host sessions and are no longer
loaded, so .github/prompts/review-pr-feedback.prompt.md had stopped
resolving as a slash command. This is the pilot migration.
The tool only rewrote frontmatter; the body is byte-identical:
- dropped `tools` (no skill equivalent; the skill now inherits the
ambient agent's tools rather than the former 9-entry allowlist)
- added `disable-model-invocation: true` to preserve the prompt's
manual-invocation-only behaviour
- kept `name`, `description` and `argument-hint` as-is
The original prompt file is retained for now.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The migration tool translated frontmatter only and left the body
untouched, so it still contained VS Code prompt-file syntax that nothing
substitutes in a skill. The model would have read it as literal text.
- Replace the seven ${input:...}/${workspaceFolder}/${selection} context
variables, and their six further references in the task steps, with an
Inputs table describing how to parse the freeform text a user supplies
after the slash command.
- Replace the `#skill:generate-mstest-filter` prompt-file directive with
a plain-language instruction to use that skill.
- Extend `description` and rewrite `argument-hint` in freeform terms
matching how skills actually receive arguments.
`disable-model-invocation: true` is kept deliberately: this skill should
run only when explicitly requested via /review-pr-feedback.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The skill hardcoded the gh CLI, including `gh api` GraphQL for the
central review-thread query, so it would fail wherever gh was absent
even when a GitHub MCP server or another path could do the work.
Add a Tool selection section that lets any path serve any capability and
sets a preference order: explicit user instruction, then whatever is
already working this session, then what previous runs recorded, then
whatever pre-flight proves capable. Selection is per capability rather
than per run, since reading and writing are frequently served by
different paths.
Add a Pre-flight validation section that probes each capability
separately with read-only calls before any work starts. Capabilities
differ in required permissions, so they are validated independently and
a failure is fatal only to the step it gates: missing write access now
degrades to read-only with exact instructions for the user instead of
aborting. Reading review threads with their resolved state is the one
hard requirement, as the rest of the skill depends on it. Resolving a
thread needs a GraphQL mutation that not every path exposes, so that is
called out explicitly.
Runs now report the paths used per capability, which is what carries the
preference into later runs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot code review withholds findings it judges low-confidence rather
than posting them as review comments. They appear only in the body of
the Copilot review, so the existing review-thread query could never see
them. Verified against #4706, where reviewThreads
returns one comment while the review body carries two further findings,
both substantive: the skill was acting on a third of Copilot's output.
Add a mandatory gathering step covering how to locate and parse the
block. Details that matter in practice:
- The heading varies by Copilot version ("Suppressed comments (n)" and
"Comments suppressed due to low confidence (n)" both occur in this
repo's history), and may be nested inside a "Review details" section,
so matching has to be tolerant.
- Entries carry a path:line marker, the finding text, and often a code
snippet, but no thread, URL or resolved state. They therefore cannot
be resolution-filtered, replied to, or resolved, and are tracked and
reported separately throughout.
- Re-reviews repeat earlier findings, so entries are collected across
all Copilot reviews and deduplicated.
- An author filter naming other reviewers must not discard them.
Low confidence is treated as Copilot's estimate rather than a verdict:
each finding is judged against the code, and rejections must be
justified. Reporting zero suppressed findings is a valid outcome; not
looking is not.
Add a pre-flight capability for reading full review bodies, since a path
that lists review comments but cannot return bodies would miss this
silently. Task steps renumbered to 9 and cross-references updated.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the old "prompt the user to reply/resolve" hand-off with defined
behaviour, split into two steps after the commit step.
Every item of feedback now gets a reply stating either what changed to
address it or why it was rejected; there are no silent dismissals. Thread
feedback is answered in its thread. Feedback with no thread to reply in —
Copilot suppressed findings and non-review discussion comments — is
covered by exactly one summary PR comment rather than one comment per
item. This reverses the previous instruction not to reply to suppressed
findings.
Resolution is now deliberately asymmetric. Bot-authored threads are
resolved once their reply lands. Threads any human participated in are
always left open so the human can accept or reject the reply themselves,
even when the fix is complete. A bot-opened thread that a human joined
counts as human.
Authorship is decided from the author type field, not the login, because
logins are path-dependent: the same Copilot reviewer is reported as
`copilot-pull-request-reviewer` by GraphQL and `Copilot` by REST, while
`__typename`/`user.type` cleanly separate Bot from User across every
automation and human account in this repo's recent history. Unknown or
ambiguous authorship falls back to human, so the failure mode is leaving
a thread open rather than auto-resolving someone's unanswered review.
Pre-flight gains capability checks for authorship detection, thread
replies, PR comment creation, and thread resolution, which are separate
permissions and can come from different paths. Push now precedes replying
so replies can cite the pushed commit.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Three related changes.
Infer the PR from the workspace branch when the request names none, so
the skill can be invoked bare. Inference matches the current branch to a
PR head ref, prefers an open PR, and matches on head repository too so a
same-named branch in another fork cannot be picked up. It stops and asks
when there is no single confident answer: detached HEAD, the default
branch, no match, or several open matches. The inferred PR is always
confirmed with the user before acting, since this skill now posts public
comments and resolves threads.
Discussion comments are now always inspected rather than opt-in, and are
mined for actionable feedback instead of being filed as informational by
default. Maintainers regularly request changes in a plain PR comment
rather than a formal review, and that is as binding as any other
feedback. The corresponding input is gone from the Inputs table and the
argument hint.
Step 3 is widened from Copilot's suppressed block to review bodies in
general, split into 3a (body text, any author) and 3b (Copilot
suppressed). This closes a second silent gap of the same shape as the
suppressed one: in the last 40 PRs of dotnet/SqlClient, 14 human reviews
carry body text with no inline comments, several of them
CHANGES_REQUESTED and plainly actionable, and none reachable from a
review-thread query.
A Feedback sources table now defines the four sources with, for each,
where a reply can go and whether it can ever be resolved. Every item is
tagged with its source and carries it through planning, reporting,
replying and resolving, and per-source counts are reported even when
zero so a skipped source is visible.
Also quote `argument-hint`, whose new value begins with `[` and would
otherwise parse as a YAML flow sequence and stop the skill loading.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The skill could commit, push, reply and resolve, but its consent rules
were inconsistent: commit and reply each had a loose "confirm with the
user" line, push was a suggestion, and resolve had no gate at all.
Add an Approvals section defining all four as separately gated actions,
each needing explicit approval every time, and state what must be shown
before asking: the commit message and files, the remote and branch, the
full reply text and its destination, the thread list with authors and
why each qualifies.
The rules that matter:
- Approval of one action never implies another. Agreeing to a commit
does not authorise a push; approving reply text does not authorise
resolving those threads.
- Only an unambiguous yes counts. Silence, a question, a partial answer
or approval of something else is a no.
- Approval covers exactly what was shown, so reworded replies or an
added commit void it.
- A gate may be skipped only when the user explicitly asked for that
action to proceed unattended, and a blanket instruction covers only
the actions it names.
- Approvals do not accumulate across turns or runs; they are reused only
when the user clearly made them standing, and ambiguity means ask.
Declining a gate is a normal outcome rather than a failure: the action
is skipped, the work is left in place, the remaining steps continue, and
the report records every gate as approved, pre-approved, declined or not
reached.
Approving the resolve gate still cannot resolve a human-authored thread;
the bot-only rule is independent of consent, so neither check can be
used to bypass the other.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Test run against #4256, chosen for having all four
feedback sources. The gathering mechanics held up — every source was
found, bot/human classification was right on all 17 threads, and the
suppressed block parsed — but the run exposed five real defects.
Workspace branch was never checked. Pre-flight only confirmed the right
repository, so with the workspace on an unrelated branch and the PR's
head on a fork, step 6 would have edited whatever was checked out and
step 9 committed it. Pre-flight now compares branch and head repository,
and a mismatch stops the run and offers either switching branches with
the user's agreement or an analysis-only mode that skips steps 6 and 9.
No way to say a fix was already made. Six of the eight unresolved
threads had been fixed by the PR author with commit SHAs, and the one
suppressed finding was fixed too, but the only statuses available forced
them into Fixed, claiming someone else's work. Adds an Already Addressed
status that cites its evidence, and a planning step that checks the
current head before planning any edit.
Third-party PRs were treated as your own. The skill assumed you author
the PR it acts on. It now determines that up front and, when you do not,
drafts everything but withholds replies unless explicitly asked, since
they post under your name on someone else's work.
Operational noise counted as feedback. Eight of the seventeen discussion
comments were `/azp run`, pipeline status and coverage reports, and
"reply to every item" would have answered them. These are now set aside
and counted, though a human's reply to one is still judged on content.
Duplicates were counted twice. The same request for benchmark code
arrived as both a review body and a discussion comment. Planning now
merges duplicates across sources into one item listing every source.
Replies are scoped to items the run actually engaged with, so Already
Addressed items, noise and merged duplicates no longer generate comments
that say nothing.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR authors routinely annotate their own diff to walk reviewers through a
change, and the skill had no way to represent that. Such items look like
feedback structurally but ask for nothing, so they were being planned,
fixed and replied to as though a reviewer had raised them.
The pattern is common rather than marginal: across 80 recent PRs in
dotnet/SqlClient there are 49 threads opened by the PR's own author,
carrying notes like "This field did not exist in the Config class" and
"Reduces 'using' noise". Review bodies on one's own PR are rarer but
real, #4481 being the canonical shape — a body reading
"Comments to aid review" attached to eleven explanatory inline comments.
Detection is by comparing the item's author with the PR's author, at the
point of gathering in steps 2 and 3a. Only a thread the author opened
counts; their reply inside a reviewer's thread is a response to feedback
and is unaffected.
Such items are excluded from planned work, from drafted replies and from
resolution, and are reported under their own status with per-source
counts. They are still read during planning, because an author's
explanation of why the code looks the way it does often changes how the
surrounding feedback should be addressed.
The default is overridable: commentary that genuinely asks something —
an open question to reviewers, a flagged TODO, a decision the author
invites challenge on — is promoted out of the category with a stated
reason and classified normally.
Step 2 is renamed from "Gather actionable review feedback", which is no
longer accurate now that it also collects non-actionable commentary.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Authors distinguish work they intend to do from context for reviewers by
tagging their own handle. The Author Commentary category added in the
previous commit had no such carve-out, so a self-assigned TODO would
have been filed as explanation and dropped from the plan.
A self-tag now overrides the commentary default wherever author content
is gathered, in both review threads and review bodies, and is classified
like any other actionable feedback. Tagging a different person is not a
self-tag; an author questioning a named reviewer is still judged on
content by the existing promotion rule.
Detection deliberately ignores mentions in quoted lines, fenced code
blocks and inline code. Testing a naive handle match over 100 PRs in
dotnet/SqlClient returned three hits and all three were false positives:
every one was a reviewer's message that the author had quoted with `>`
before answering underneath. Without that guard the rule would misfire
almost every time it fired at all.
No genuine instance of the convention appears in that sample, so this is
implemented from the stated convention rather than from observed usage,
and it is worth confirming against a real example when one exists.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous commit noted the convention could not be found in the
sample searched. It was there; the sample was wrong. Ordering 100 PRs by
recent activity missed this author's history almost entirely, and
searching their 257 merged PRs directly finds 45 self-tagged
author-opened threads: "@handle - Undo these leftover changes",
"@handle Stale comment", "@handle Why does this parameter have a
default?".
The same PRs hold 544 plain author-opened threads, explanatory notes
like "This took many hours to figure out" and "Prefer to expand static
variables at pipeline expansion time". One author using both forms
heavily, roughly one tagged for every twelve plain, confirms the split
is deliberate and that the commentary default is right.
Three details from that data tighten the rule:
- The separator after the tag varies between " - ", " - " and nothing
at all, so requiring one would drop real matches.
- The tag opens the comment in 44 of 45 cases but not always, so
position cannot be required either.
- The exception is the interesting one: it opens with commentary and
adds the self-tagged request in a later paragraph. A single comment can
therefore carry both, and the tagged part is the request.
The quoted-text guard holds up: across the 45 matches it produced no
false positives, against three out of three on the earlier sample where
every hit was a quoted reviewer message.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Prompt files are deprecated for Agent Host sessions and are no longer
loaded, which is what took this prompt out of service in the first
place. Keeping it alongside the skill would leave two copies of the same
workflow to drift apart, with only the skill actually running.
No tracked file references it: it was never listed in the prompt table
in AGENTS.md, so nothing is left pointing at a file that no longer
exists.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All six come from Copilot's review of #4750 and were each verified
against the file before being accepted.
Paginate every collection. Pre-flight only ever fetched "one page", and
no step said otherwise, so any PR with more feedback than one page would
silently drop the remainder while the report claimed every source was
inspected. A Gathering completely section now requires paging each
connection to exhaustion, including the comments within a thread, and
requires incomplete reads to be reported as incomplete.
Record both thread identifiers. Step 2 kept only the GraphQL thread id,
but a REST reply needs the root review comment's numeric id. Since the
skill deliberately allows reading and writing through different paths,
capturing one identifier could strand step 10 or 11 with no way to act.
Add a Rejected status. The instructions called for rejecting feedback
with a reason in three places while no status could express it, so a
rejected item had to be mislabelled and the totals corrupted.
Allow replies that report no action. Step 4 required informational items
to be replied to, while step 8 demanded every reply state a change or a
rejection. An informational item is neither, so the two rules could not
both be satisfied. Replies may now state that no action was required.
Mark and exclude generated summaries. The summary comment posted in step
10 is a PR comment, which step 4 collects on the next run; with
informational items being replied to, successive runs could answer their
own previous output indefinitely. Summaries now carry a marker that step
4 excludes.
Require a terminal outcome before resolving. Qualification considered
authorship and whether a reply was posted, but not the result, so a
bot-only thread classified Blocked or Needs Clarification could be
resolved with its request still open. Outcomes are now explicitly
terminal or not, and only terminal ones qualify.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Found by running the skill end-to-end against #4750, which is the first
run to get past the reply gate.
Step 11 required that every comment in a thread be authored by a bot,
evaluated at resolution time. Step 10 always adds a reply, and that
reply is written by a human, so after replying no thread could ever
satisfy the test. Six bot-only threads that qualified before step 10
were disqualified by step 10 itself, making resolution unreachable in
every run that replies.
Step 11 also contradicted itself: one bullet said to judge from the
author type recorded in step 2, a snapshot taken before anything was
posted, which would have given the right answer.
Resolution now judges authorship from that step 2 snapshot and
explicitly disregards this run's own replies, with the same
clarification applied to the three other bullets that phrase the rule as
"any human". The protection is unchanged for everyone else: a thread any
other human participated in still cannot be resolved.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Explicitly enable analysis-only mode for external PRs
.github/skills/review-pr-feedback/SKILL.md:206
This says to default to analysis and advice for someone else's PR, but it never puts the run into the analysis-only mode checked by steps 6 and 9. As written, the agent can still edit and commit on the contributor's branch while merely withholding replies. Explicitly select analysis-only mode here unless the user separately asks to modify that PR.
Add Informational to the output classification contract
.github/skills/review-pr-feedback/SKILL.md:420
Step 2 retains every unresolved thread and step 7 can classify one as Informational, but this output contract has no Informational value. An unresolved thread containing a non-request (for example, praise or context) therefore cannot be reported with its actual classification even though steps 8 and 10 require handling informational items.
Check for dirty working tree before switching branches
.github/skills/review-pr-feedback/SKILL.md:198
User approval alone does not make a branch switch safe when the working tree is dirty: compatible tracked or untracked changes can follow onto the PR branch and later be included in the proposed commit. Check for local changes before switching and stop for an explicit preservation decision when any exist.
Support Informational status in final review reports
.github/skills/review-pr-feedback/SKILL.md:421
Informational is a valid terminal classification for review threads (lines 328-329 and 387), and such threads are replied to, but this output contract cannot represent that status. An informational unresolved thread will therefore be omitted or mislabeled in the final report.
Validate review author type and login in capability checks
.github/skills/review-pr-feedback/SKILL.md:103
This pre-flight can pass without data that step 3b requires. The configured GitHub MCP get_reviews response for PR #4706 includes body, state, and login but no Bot/User author type; because step 3b requires type plus login, that path would be accepted here and then silently fail to identify the mandatory Copilot suppressed findings. Validate author login and type as part of this capability so the run selects a fallback or reports the source incomplete.
Identify the principal for each authenticated write path
.github/skills/review-pr-feedback/SKILL.md:108
The workflow explicitly permits different access paths per capability, but those paths may be authenticated as different accounts. Treating “the authenticated user” as a single identity can select full mode because the read client is the PR author while replies or pushes use another account, causing actions to occur under the wrong identity. Identify the principal for each authenticated write path and base edit/push/reply decisions on the actor that will perform that action.
Reply in every distinct review thread
.github/skills/review-pr-feedback/SKILL.md:376
Suppressing the second answer conflicts with step 10's requirement to reply in each review thread. When the same request appears in two distinct review threads, merging the implementation is correct, but answering only one leaves the other reviewer without the promised thread reply. Retain every original destination and reply once per distinct thread while consolidating duplicate non-thread feedback in the summary comment.
Five findings from Copilot's seventh and eighth reviews of #4750: one
review comment and four from "Previously missed" sections.
Take the repository from the PR URL. The Inputs table accepted a PR as a
number or a URL but resolved the repository from an explicit owner/name
or the local git remote, so a URL naming another repository was paired
with this workspace's remote. Passing a full URL could therefore act on
whatever PR happens to carry that number here. A URL now sets its own
repository, and an explicit repository that contradicts it stops the run.
Report outcomes only after they occur. Step 8 was called the final
report, yet commit, push, reply and resolve run afterwards and the output
contract requires their results — approvals, replies posted, threads
resolved. The step could only invent them or never be revisited. It is
now step 8 drafting, with a new step 12 producing the report once the
gated actions have run or been declined, and an action that did not
happen must be reported as such.
Require an author type when validating review-body access. Step 3b needs
type plus login to recognise the Copilot reviewer, but the capability
check asked only for body and state, so a path exposing logins without
types passed and then reported zero suppressed findings — identical in
the output to a PR genuinely having none.
Identify the principal per write path. Paths are selected per capability
and may authenticate as different accounts, so "the authenticated user"
was ambiguous: full mode could be chosen because the reading client was
the PR author while replies and pushes went out as someone else.
Authorship is now judged against the account that will actually write.
Merge the work, not the answers. Deduplication told the run not to write
two answers to the same question, while step 10 promises a reply in each
thread, so a point raised by two reviewers left one of them unanswered.
One fix and one explanation still, but posted to every destination the
item arrived through.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Responses to feedback that has no review thread to reply in. Posted by the review-pr-feedback skill.
All four arrived under Previously missed, across review 7 and review 8, with no review comment attached. All fixed in 1b61981.
Final report is generated before workflow outcomes
Valid. Step 8 was titled "Produce a final report", but commit, push, reply and resolve are steps 9 to 11, and the output contract requires exactly their results: which gates were approved, which replies were posted, which threads were resolved. The step could only invent those or never be revisited.
Step 8 is now "Draft findings and replies" and explicitly does not publish, and a new step 12 produces the report after the gated actions have run or been declined. An action that did not happen must be reported as not having happened, with the reason. Status: Fixed.
Validate review author type and login in capability checks
Valid, and a good catch about silent failure. The review-body capability check asked only for body and state, while step 3b needs an author type plus login to recognise the Copilot reviewer. A path exposing logins without types passed pre-flight and then reported zero suppressed findings — output indistinguishable from a PR that genuinely has none.
The check now requires the author to carry a Bot/User type, and says plainly why: a silent zero here looks exactly like a real zero. Status: Fixed.
Identify the principal for each authenticated write path
Valid. The skill deliberately allows a different access path per capability, and those paths can authenticate as different accounts, so "the authenticated user" was ambiguous. Full mode could be selected because the reading client was the PR author while replies or pushes went out under a different account.
Pre-flight now establishes the principal for every path that will write, authorship is judged against the account that will actually act rather than whichever client read the PR, and each principal is reported. Status: Fixed.
Reply in every distinct review thread
Valid, and a conflict between two rules added in different runs. Deduplication said not to write two answers to the same question; step 10 promises a reply in each review thread. When two reviewers raise the same point in separate threads, one of them was left unanswered.
Merging now affects the work, never the answers: one fix and one explanation, posted to every destination the item arrived through. Status: Fixed.
No Copilot suppressed findings this run. Two discussion comments were present — both previous summaries from this skill — and both were correctly excluded by marker and authorship.
Reply consistently to already-addressed feedback items
.github/skills/review-pr-feedback/SKILL.md:445
This exception deliberately leaves Already Addressed human-thread and non-thread feedback without a reply, contradicting the PR's stated behavior (“Replies to every item it engaged with”) and the rule at line 612. Either send a no-action reply with the evidence for these items or consistently narrow the advertised reply-all contract.
Four findings from Copilot's ninth review of #4750. All four are
contradictions between rules added by earlier commits on this branch
rather than defects in the migrated prompt, which is why this commit
also restructures the section where that keeps happening.
Describe suspicious content, do not quote it. The trust boundary asked
for the relevant part of an attacking comment to be quoted, while the
credential rule added in the same commit forbids echoing secret-shaped
values. Untrusted text can carry a token, so reporting an attack could
leak what it carried. Such content is now paraphrased and located, with
redaction if an excerpt is truly needed.
Compare every write principal, not "the writing account". The previous
commit judged authorship against the account that writes, but pre-flight
permits push, reply and resolve to authenticate as different accounts,
leaving the singular phrasing undecidable when only some match. Full
mode now requires every acting principal to be the PR's author, with no
averaging, and each principal is reported.
State the reply contract once. Two bullets disagreed about duplicates —
one suppressing a reply already given through another source, the other
requiring one per destination — and Already Addressed items were denied
replies in step 8 while two other places promised replies to everything
engaged with. Step 8 now carries a single statement of who gets a reply;
step 10 and the Rules defer to it instead of restating it.
Regroup the Rules. Thirty-four unordered bullets had accumulated one per
fix, several restating steps in wording that had since drifted. They are
now grouped by theme, with the rule that where a section or step is
named, that place is authoritative and the rule is only its short form.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Valid, and fixed in 2dd4c88. Step 8 excluded Already Addressed items from replies except for bot threads that needed one to be resolvable, while both the Rules and this PR's own description promise a reply to every item engaged with. Two places said different things about the same items.
Resolved in favour of the advertised contract rather than narrowing it: Already Addressed items now get a short no-action reply naming what already satisfied the request. That is honest about the outcome, keeps the "no silent dismissals" guarantee true, and still gives step 11 the posted reply it requires before a bot thread can be resolved. Status: Fixed.
Note on this run
All four findings this run were contradictions between rules added by earlier commits on this branch, not defects in the migrated prompt. Across the last three runs that is seven of fourteen.
The cause is structural: the file had grown to 623 lines with a flat, unordered list of 34 rules accumulated one per fix, several restating steps in wording that had since drifted apart. Patching each conflict individually was generating the next one.
So alongside the four fixes, this commit consolidates: the reply contract is now stated once in step 8 with step 10 and the Rules deferring to it, identity is stated once as "every acting principal", and the Rules are grouped by theme under an explicit convention that where a rule names a section or step, that place is authoritative and the rule is only its short form.
No Copilot suppressed findings this run. Three discussion comments were present — all previous summaries from this skill — and all were correctly excluded by marker and authorship.
Three findings from Copilot's tenth review of #4750.
Do not answer the same item twice across runs. Most feedback this skill
answers never becomes resolved: human threads are deliberately left
open, and review bodies and discussion comments have no resolved state,
so every run gathers them again. Excluding this skill's own summary
comments stops a loop but records nothing about what was already
answered, so a human reviewer would have collected a duplicate reply on
every subsequent run. Eight runs did not expose this only because every
thread so far was bot-authored and therefore resolved out of the
unresolved set. Step 8 now checks each destination for a reply already
posted there and repeats one only when the request or the outcome
changed, reporting the rest as answered previously.
Exempt the replies, not the account. The resolution rules said to
disregard "your own step 10 reply" but were phrased as exempting anyone
writing from the acting account, so a comment the user typed by hand
during a run would have been ignored and the thread resolved despite
genuine human participation. The exemption is now keyed to the comment
ids this run posted.
Key posting to the run mode. Step 10 still asked whether "the
authenticated user" is the PR's author, which the previous commit had
already replaced in step 1 with a decision over every acting principal.
A mismatched push principal alongside a matching reply principal
selected analysis-only there while still permitting posting here. Step
10 now defers to the mode instead of re-deriving identity.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Missing review author type must fail capability validation
.github/skills/review-pr-feedback/SKILL.md:103
This row first requires the review author type, but then says a reader missing that field “passes this check.” Taken literally, pre-flight can accept an incapable path and step 3b will silently report zero suppressed findings. Make the missing type fail this capability explicitly.
Report each write principal and author match separately
.github/skills/review-pr-feedback/SKILL.md:554
The workflow deliberately supports different principals for commit, push, reply, and resolve, so there may be no single “authenticated user” answer. This output field can conceal the mismatch that selected analysis-only mode. Report each write principal and its author match, consistent with lines 261–268.
Four findings from Copilot's eleventh review of #4750, all of them
consequences of recent commits on this branch.
Recognise this skill's earlier replies. The two fixes in the previous
commit deadlocked each other: once resolution is declined or
unavailable, the reply left on a bot thread becomes an ordinary User
comment in the next run's snapshot, while the new no-repeat rule
prevents posting a fresh one. Both resolution conditions then fail for
good, and the thread can never be resolved. Replies now carry a marker,
this skill's own replies are exempt from the bot-only test whichever run
posted them, and a prior reply satisfies the reply requirement as long
as it still covers the current request and outcome.
Permit the push that updates the PR. Consolidating the rules two commits
ago tightened the workspace invariant to forbid acting at a commit other
than the PR's head, but step 9 creates a commit, so local HEAD
necessarily differs from the remote head until it is pushed — the rule
forbade the push that would reconcile them. The equality now describes
the state before this run edits anything, and pushing commits this run
created is stated as expected.
Fail the capability when a review author has no type. The row required
the type and then described a path lacking it as passing, which was the
old behaviour written in the present tense and read as permission.
Report each write principal separately. The output still asked whether
"the authenticated user" is the PR's author, a singular question that an
earlier commit had already replaced with a decision over every acting
principal, and which can hide the mismatch that selected analysis-only.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Responses to feedback that has no review thread to reply in. Posted by the review-pr-feedback skill.
Both arrived under Previously missed. Both fixed in 3f0c8b6.
Missing review author type must fail capability validation — SKILL.md:103
Valid. The row required the author type and then described a path lacking it as one that "passes this check and then finds no suppressed findings" — the old broken behaviour written in the present tense, which read as permission rather than as the rationale it was meant to be.
Reworded so a path that cannot return the type explicitly fails the capability, with the silent-zero consequence given as the reason rather than as the outcome. Status: Fixed.
Report each write principal and author match separately — SKILL.md:554
Valid. The output field still asked whether "the authenticated user" is the PR's author — the singular question that an earlier commit had already replaced in step 1 with a decision across every acting principal. As written it can hide the very mismatch that selected analysis-only mode.
The field now reports each write principal (commit, push, reply, resolve), the account it runs as, and whether that account is the PR's author, named separately so a mismatch is visible rather than averaged away. Status: Fixed.
Note on this run
All four findings this run were consequences of recent commits on this branch, and two were introduced by the previous commit alone — its two fixes deadlocked each other, making a declined resolution permanently unresolvable.
That is worth stating plainly: the last three runs have found 4, 3 and 4 issues, and the majority in each were self-inflicted. The rules consolidation improved the structure but has not stopped new fixes from interacting badly with existing ones.
No Copilot suppressed findings this run. Four discussion comments were present — all previous summaries from this skill — and all were correctly excluded by marker and authorship.
Resolving a single repository before branch inference breaks the no-argument flow for a standard fork checkout: the workspace remote commonly names the contributor's fork, while the PR is hosted in the upstream repository, so searching that resolved repository finds no PR. Defer base-repository selection until all remotes and their upstream/parent repositories have been searched by head repository plus branch; then use the selected PR's base repository and the remote pointing to its head for pushes.
Only evaluate root comments, missing reply requests
.github/skills/review-pr-feedback/SKILL.md:291
Only the root comment's body is collected; later comments are paged only for authorship. If a reviewer adds a new request in a reply, the workflow never assesses, plans, or replies to it while still reporting complete thread coverage. Record and evaluate every comment's body and URL, not only its author type.
This issue also appears on line 534 of the same file.
Unconditional summary repeats unchanged items
.github/skills/review-pr-feedback/SKILL.md:528
This unconditional “new” summary conflicts with step 8's requirement not to answer unchanged items again. On a rerun where all non-thread items were already covered—or where the only items are author commentary—this still directs the agent to publish another summary. Scope the comment to items that actually need a new reply, and suppress it when that set is empty.
The reason will be displayed to describe this comment to others. Learn more.
This is a large bot file, and it is largely targeted at output from other bots. Its size is the result of many iterations uncovering nuances between that bot-bot conversation. In the end, I find it helpful for my workflow, especially since GitHub Copilot isn't very good at identifying all issues in a single review - it often surfaces issues present in the original commit many commits later.
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
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.
paulmedynski
added
the
Area\Engineering
paulmedynski
left a comment
Migrates
review-pr-feedbackfrom a VS Code prompt file to an agent skill, then hardens it over nine self-review iterations.Why
Prompt files are deprecated for Agent Host sessions and are no longer loaded, so
/review-pr-feedbackhad stopped working. Skills are the supported replacement.This is the pilot for migrating the remaining 15 prompts under
.github/prompts/.What the skill does
gh, a GitHub MCP server, REST/GraphQL — chosen per capability, with read-only pre-flight validation of each.How it was built
The first commit is the migration tool's raw output, unmodified, so the machine translation and the human work stay separable. The tool rewrote frontmatter only: it dropped
tools, addeddisable-model-invocation, and left all 117 body lines untouched, including seven${input:...}variables that nothing substitutes in a skill.Everything after that came from rewriting the body for the skill format, then running the skill against this PR — nine times — and fixing what each run exposed.
Iteration log
Each run gathered feedback on this PR, applied fixes, replied, and resolved. Defects found per run:
Rejectedstatus, summary-comment feedback loopFixedcould mean "not on the PR";Already Addressedcould never resolve32 defects across 9 runs, 23 commits. 21 review threads, all resolved.
Three findings were verified against the repository rather than assumed:
CHANGES_REQUESTEDThat last split is why author commentary is non-actionable by default while a self-tag makes it actionable. Self-tag detection ignores quoted text: on a sample where every hit was an author quoting a reviewer who had tagged them, naive matching was wrong 3 times out of 3.
Why iteration stopped
Deliberately, at diminishing returns rather than at zero findings.
The defect source shifted over the nine runs. Runs 1–4 found flaws in the migrated prompt. Runs 7–9 found flaws introduced by runs 6–8: seven of the last eleven findings were self-inflicted, and run 9's four were all consequences of recent commits, two from the immediately preceding one. A consolidation pass in run 7 regrouped 34 accumulated rules by theme and fixed the drift, but did not stop new rules interacting badly with old ones — that is a property of a 700-line procedural document, not of how its rules are grouped.
The remaining findings also concern edge cases the skill has not hit in nine runs: declined resolutions, divergent write principals, human reviewers on this PR. Each fix for a hypothetical adds surface area for the next contradiction.
Two classes of defect are worth noting, because neither method alone would have found both:
Notes for reviewers
AGENTS.md's prompt table, which remains incomplete for unrelated reasons and is untouched here.toolshas no equivalent in the skill format, so the former 9-entry allowlist is gone and the skill inherits the ambient agent's tools. The approval gates and the trust boundary are what constrain it now.disable-model-invocation: trueis deliberate: this runs only when explicitly invoked via/review-pr-feedback..github/skills/, but only on the branch you have checked out. Release branches will need this backported separately;release/7.0does not carry the old prompt file, so only the skill applies there.code-reviewis migrated next, the Approvals, trust-boundary and bot/human sections are candidates for shared guidance rather than a second copy.Evidence from use on other PRs
The skill has also been used on two unrelated engineering PRs — #4730 (XML documentation validation) and #4731 (symbol publishing diagnostics) — neither of which it was developed against. Both show the same pattern, and it is the one the four-source design exists for.
On #4731 the last two rounds found zero unresolved review threads and zero discussion comments. Every finding came from
Previously missedsections collapsed inside review bodies. A thread-only reading of that PR would have reported nothing outstanding on both occasions.What the production runs demonstrate, beyond the count:
Open/Previously missedsections instead. That is the skill declining to imply coverage it did not have.M:System.Foo.op_Implicit(System.Int32)~string... both passed with zero findings", then a fix, then "214 tests pass, up from 208. Five of the six new cases fail against the previous logic rather than passing vacuously."The bugs themselves were real and non-trivial — a documentation trim rewriting its own input so a later build could publish trimmed text as the full text; malformed XML being silently downgraded in report-only mode; a public wrong-kind reference passing validation and staying unresolved on Learn.
Both PRs are authored by the same user with the head branch in
dotnet/SqlClient, so they exercise the same identity path as this PR. The untested paths below are still untested.Still draft: across this PR and the two above, the skill has never run against a PR with an external reviewer, a fork head, or a contributor other than the invoking user — the paths its analysis-only and human-thread rules exist to protect. Every thread it has resolved so far was bot-opened, so the rule that keeps human threads open has never actually had to hold anything back.