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.
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.
Authors routinely quote a reviewer who tagged them and then answer underneath, so a
naive match on the handle finds the reviewer's words rather than the author's.
- Tagging someone else is not a self-tag. An author asking a named reviewer a question is judged on content by the promotion rule below.
- Promote author commentary out of that category when it genuinely asks for something even without a self-tag: an open question put to reviewers, a flagged TODO, or a decision the author says they want challenged. Say why you promoted it, and classify it normally from then on.
The reason will be displayed to describe this comment to others. Learn more.
Move this promotion check before step 5. Otherwise, an author's TODO without a self-tag is excluded from the plan and only recognized as actionable after implementation.
You're right about the ordering. Step 5 excluded author commentary from the plan, while the rule that can pull an untagged TODO back out of that category lived in step 7 — which runs after step 6 has already implemented. The only untagged author request the skill could recognise was therefore one it had already finished working without.
Step 5 now applies step 7's promotion test before excluding anything, and promotes on the spot. Step 7 keeps the definition and states that an item promoted in step 5 arrives already actionable, so the two places cannot drift into being two different tests.
- Resolving is a gated action, separate from the reply gate. See Approvals.
- Work out which threads qualify. Judge authorship from the snapshot step 2 recorded, before this run posted anything. A thread qualifies only when every comment in that snapshot was authored by a bot or by this skill, a reply covering the current request and outcome exists on it from this run or an earlier one, and its classification is terminal — Fixed, Rejected, Already Addressed or Informational.
- Re-fetch each candidate thread immediately before resolving it, and compare against the snapshot. A run takes time, and a human can comment while it is in progress. If anyone other than you has commented since the snapshot, drop that thread from the list, say so, and leave it open: they have now engaged, and the reply they are owed is theirs to judge.
The reason will be displayed to describe this comment to others. Learn more.
Exempt only comment IDs posted by this run, not everyone posting as "you". A handwritten comment from the same account after the snapshot must also prevent resolution.
The re-fetch check said "if anyone other than you has commented since the snapshot", which exempts the whole account — and the bullet a few lines below already said the opposite for the bot-only test: a comment the user writes by hand comes from the same login and is genuine human participation. A handwritten comment landing mid-run would have been ignored and the thread resolved out from under it.
It now drops a candidate thread when any comment this run did not itself post has appeared since the snapshot, decided by the comment ids this run posted rather than by the account they came from.
From PR #4750 feedback:
- Apply the author-commentary promotion test in step 5, before the planning
exclusion, so an untagged author TODO is planned rather than recognised
only after step 6 has implemented without it.
- Exempt only the comment ids this run posted when re-checking a thread
before resolving; a handwritten comment from the same account now blocks
resolution.
- Defer base-repository selection until branch inference has searched every
remote and its upstream/parent, so a fork checkout finds its upstream PR,
and push to the remote pointing at the PR's head repository.
- Record and evaluate every comment in a review thread, not just the root,
so a request added in a reply is assessed and answered.
- Scope the summary comment to non-thread items that need a new reply, and
post none when that set is empty.
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 three arrived under Previously missed in review 5306551476, with no review comment attached, under a header reading "Findings: None". All fixed in 6a01087.
Valid. Step 1 resolved a single repository from the git remote before inferring the PR, so the ordinary contributor case — remotes naming the fork while the PR lives upstream — searched the wrong repository and found nothing.
Resolution is now deferred: branch inference searches every repository the remotes point at plus the upstream or parent of each, the base repository comes from the PR that search selects, and push goes to the remote pointing at that PR's head repository. Status: Fixed.
Only evaluate root comments, missing reply requests — SKILL.md:291
Valid. Step 2 extracted the root comment's body and paged the remaining comments only for author type, so a request added in a reply was never assessed, planned or answered while the run still reported the thread as fully covered.
Step 2 now records the url, author login and body of every comment in the thread, and treats the latest outstanding request as the one to act on with the earlier comments as its context. Status: Fixed.
Valid, and a direct conflict with step 8's rule against answering unchanged items twice.
The summary comment is now scoped to the non-thread items step 8 decided need a new reply, and is suppressed when that set is empty — nothing found, everything already answered with nothing changed, or only author commentary left. Status: Fixed.
Note on this run
Also recorded: no Copilot review on this PR carries a suppressed-comments block — all 12 were checked — so this run's suppressed-findings count is genuinely zero rather than unchecked.
The two human review threads on this PR were fixed and answered in their own threads, and left open for the reviewer to judge.
Resolve circular ordering between pre-flight and branch/authorship steps
.github/skills/review-pr-feedback/SKILL.md:253
Step 1 cannot be executed in the stated order. These lines say to finish the remaining branch/authorship bullets before pre-flight, but line 275 says the write principals used by those bullets come from pre-flight, while the pre-flight section says it runs after step 1 has settled. Define one non-circular sequence—for example: identify and confirm the PR, run capability/identity pre-flight, then select the run mode from those results.
Prevent duplicate collection of findings from suppressed feedback blocks
.github/skills/review-pr-feedback/SKILL.md:334
The ordinary review-body pass currently keeps every substantive assessment, which includes entries inside Copilot's suppressed-feedback block; step 3b then collects those same entries again. This can double-count and report one finding in both review-body sections despite the per-source totals contract. Make step 3b the exclusive collector for suppressed blocks.
…fects
From PR #4750 feedback:
- State the pre-approval exception in the skill description and the Approvals
headline, so "explicit approval, every time" no longer contradicts the
documented escape hatch, and say that carrying approval forward is the user
overriding the gate rather than the skill inferring that a yes still stands.
- Disclose at the commit gate when a file the user chose to keep editing also
carries their own uncommitted hunks: naming a path does not separate them,
so the guarantee is scoped to untouched files and the alternatives are
re-offered at the last point they can be taken.
- Give step 1 a non-circular order — confirm the PR, run pre-flight to gather
the workspace, branch, HEAD and per-path principal facts, then interpret
them to select the run mode.
- Make step 3b the sole collector of Copilot's suppressed block, so a
suppressed finding is not counted again as ordinary review-body text.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve circular ordering between pre-flight and branch/authorship steps — SKILL.md:253
Valid, and self-inflicted: this is the second visit to this ordering, after the fix in d2bdbe1c4. Step 1 said to finish its branch and authorship bullets before pre-flight, those bullets said pre-flight supplies the principals, and pre-flight said it runs once step 1 has settled.
There is now one sequence: confirm the PR, run pre-flight — which is where the workspace, branch, HEAD and per-path principal facts are gathered, since each of them needs a validated path to read it — then let the remaining bullets of step 1 interpret those results and select the run mode. Status: Fixed.
Prevent duplicate collection of findings from suppressed feedback blocks — SKILL.md:334
Valid, and a consequence of an earlier Copilot fix: "read every collapsed section" was added so nested findings would stop being missed, and it swept in the suppressed block that step 3b collects, so one finding could be reported under two sources against a report that promises per-source totals.
Step 3a now skips that block explicitly, and step 3b is named as its sole collector so each suppressed finding is counted exactly once. Status: Fixed.
Note on this run
Two human review threads on this PR were answered in a previous run and are unchanged, so they were not answered again; they remain open for the reviewer to judge.
This is the 13th Copilot review round on this PR. Findings per round have been 6, 5, 5, 4, 1, 4, 1, 4, 4, 3, 4, 3, 4 while the file grew from 123 to 761 lines — a flat finding rate against six times the surface. The character has shifted too: the early rounds found missing capability, while the recent ones are almost all internal contradictions between rules added by earlier fixes, two of four this round included. The findings are real and none were rejected, but the trend says this is steady-state churn rather than convergence.
Manual skill invocation is blocked in VS Code Agents
.github/skills/review-pr-feedback/SKILL.md:4
disable-model-invocation: true currently blocks manual /skill-name execution in VS Code's Agents window: the skill is listed but does not load (open microsoft/vscode#331477, reported on VS Code 1.133). Because this PR specifically claims to restore /review-pr-feedback for Agent Host sessions, the migration remains unusable on that surface. Either provide a temporary invocation workaround/remove this flag, or document and gate the migration on the upstream fix.
Bare PR numbers cannot resolve a repository
.github/skills/review-pr-feedback/SKILL.md:18
The advertised /review-pr-feedback 3412 form cannot resolve a repository. This row only supplies one from a URL, an explicit owner/name, or branch inference, while step 1 runs branch inference only when no PR was supplied. A bare PR number therefore reaches pre-flight with no repository. Define discovery for this case—for example, search each remote/upstream repository for that number and require one match, otherwise ask the user.
This issue also appears in the following locations of the same file:
line 308
line 310
Command requests are incorrectly classified as hostile
.github/skills/review-pr-feedback/SKILL.md:158
This treats every request to run a command as hostile, so ordinary review feedback such as “run the formatter” or “run this test” must be ignored and mislabeled Informational. Limit the suspicious case to commands that attempt to redirect scope or bypass controls; validation requests should remain untrusted data that the agent assesses and translates into its own safe command.
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.