Sitelet https://github.com/dotnet/SqlClient/pull/4481
Skip to content

Feature | Implement SSRP packet parsing - #4481

Open
edwardneal wants to merge 11 commits into
dotnet:mainfrom
edwardneal:feat/ssrp/02-parsing
Open

edwardneal wants to merge 11 commits into
dotnet:mainfrom
edwardneal:feat/ssrp/02-parsing

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Description

This builds on the earlier test cases laid in #3741, and implements parsing of SSRP responses (for both SVR_RESP & SVR_RESP (DAC) packets.)

These responses will be parsed from untrusted/malicious network packets, so I've split the parsers into their own PR to make sure they can get a dedicated review. Each of the parsers stand alone, and SVR_RESP (DAC) parsing is the simplest by some margin!

SVR_RESP parsing is the more complex part, since it has a top-level header followed by RESP_DATA parsing. I've broken this down as seems reasonable:

  • SqlDataSourceResponse parses the header
  • During parsing, we parse RESP_DATA into a chain of RespDataComponents (key + value pairs)
  • Once RespDataComponents have been extracted and verified as structurally valid, verify that they're valid for the keys we care about
  • Gather the RespDataComponents for TCP and Named Pipes in an ExposedProtocols and verify that the SQL Server instance has at least one usable endpoint
  • When we know that the above are all true, populate the properties on SqlDataSourceResponse

The end goal will be to gather packet buffers asynchronously from a UDP client or socket, then pass them to SqlDataSourceResponseReader and DacResponseReader and populate the DataTable or TCP port.

Issues

Follows up #3741
Contributes to #3700

Link to any relevant issues, bugs, or discussions (e.g., Closes #123, Fixes issue #456).

Testing

Automated tests have been added, and I've added several test cases as I wrote the parsers.

These data are issued as unicast responses, and we should recognise the first response, not the last.
These relate to invalid data and empty packet buffers
Add assertion on bytesRead in DacResponseReader
* Locate one reference to Encoding.UTF8 rather than to s_mbcsEncoding.
* Indentation for conditional compilation.
* Permit underscores in machine names.
* Complete rename from DacResponseProcessorTest to DacResponseReaderTest.
* XML documentation updates.
Also provide an early break if the BV_INFO protocol component is too short
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@edwardneal edwardneal left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments to aid review.

return GetByteCount(encoding, slicedString);
}

public static int GetByteCount(this Encoding encoding, ReadOnlySpan<char> chars)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This refactor provides the ability to restrict the number of bytes in a particular component of RESP_DATA after having been converted to a string in order to parse it.


private const byte ResponseHeaderValue = 0x05;

private static readonly Encoding s_mbcsEncoding = new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question of which encoding to use is odd. The specification just refers to a multi-byte character set, but the existing (partial) implementation just uses ASCII. I've chosen UTF8 because it meets the specification requirements while also maintaining the property of normal characters being single bytes.

ReadOnlySpan<char> valueCandidate = respDataSpan.Slice(terminatorPos + 1);

// There could potentially be a number of terminators within the RESP_DATA token's value. The Banyan VINES parameters contain five.
// BV_PARAMETERS = "bv;<ITEM NAME>;<GROUP NAME>;<ITEM NAME>;<GROUP NAME>;<ORG NAME>;"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the repetition here. This comes from the below statements in the SVR_RESP page:

BV_INFO=SEMICOLON "bv" SEMICOLON ITEMNAME SEMICOLON GROUPNAME SEMICOLON BV_PARAMETERS
BV_PARAMETERS=ITEMNAME SEMICOLON GROUPNAME SEMICOLON ORGNAME

This might well be a document error, but I've not got a real-world implementation to validate against so I've taken the specification as written.

We care about Banyan VINES but will never use it, because this is the only protocol component where the key can contain semicolons; as such, trying to parse it as part of the RESP_DATA string when it appears first in the list of protocols could lead to the succeeding protocol entries being misinterpreted.


// If a maximum value length is specified, this is in bytes. Calculate the number of
// bytes in the string, and compare.
if (maxValueLength != -1)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used for ServerName, InstanceName and VersionString. MC-SQLR references the bytes underlying RESP_DATA as a string, then requires that the string's components are converted back to bytes in order to have their length checked.

Comment on lines +280 to +282
/// A successfully parsed instance does not guarantee secure data. Callers must ensure that
/// <see cref="NamedPipe"/> points to the same server identified by the SSRP response's server
/// name before they connect to it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement isn't currently important because we don't actually resolve SSRP responses to anything other than a TCP port number, and we don't expose the named pipe in the SqlDataSource return value. If either of these assumptions change, it'll be a problem.

Section 3.1.5.2 notes:

The SQL Server Resolution Protocol SHOULD NOT verify the length or content of the PIPENAME field, which is provided by the higher layer. It is the upper layer's responsibility to ensure that PIPENAME conforms to the specification of a valid pipe name.

This is the only field which the specification explicitly bars a compliant client from validating, and it's the reason why the constructor just reads RespDataComponent.Value directly rather than using a TryGetX method.


public ReadOnlySpan<char> NamedPipe { get; }

public bool Valid => TcpEnabled || NamedPipeEnabled;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type rolls data up into a piece of coherent connectivity metadata and takes responsibility for determining where SqlClient needs to care about it. Its notion of Valid is thus more involved than RespDataComponent, which considers a component valid if that component has been successfully parsed from the RESP_DATA string.

// selected based upon the key - a protocol name. A failure to match a known key will
// allow the default value (a managed reference to a boolean true) value to stand, forcing
// this method to return false.
if (key.Equals(NamedPipesInfoKey.AsSpan(), StringComparison.Ordinal))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are deliberately case sensitive - the specification is silent on this, so I've chosen to take a more conservative approach.

}

// Parse and validate the header components
if (!RespDataComponent.TryParse(decodedRespData, ref currRespDataOffset, RespDataComponent.MaxServerNameLength, out RespDataComponent serverNameToken)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a long if statement (same style as in the DacResponseReader) but it's well-commented, all of the clauses come together to form the fixed-position and mandatory components, and splitting it into one statement per token also looked slightly clumsy. I've got no objection to changing the style if anyone thinks there's a clearer approach.

/// <summary>
/// Utilities used to extract zero or more parsed SSRP DAC responses from a set of packet buffers.
/// </summary>
internal static class DacResponseReader

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name change here and in SqlDataSourceResponseReader is purely a layering point:

  • DacResponse handles the parsing of one SVR_RESPONSE (DAC) from the start of a ReadOnlySequence;
  • DacResponseReader builds on that to read the first/last DacResponse from the ReadOnlySequence;
  • In future, the layer above will handle the networking and interpretation of the results to output the DAC port. This will be the processing layer.

/// This method parses SVR_RESP messages sent as the result of issuing a CLNT_BCAST_EX or a CLNT_UCAST_EX
/// message.
/// </remarks>
public static bool TryReadLast(ReadOnlySequence<byte> sourceSequence, out SqlDataSourceResponse lastResponse)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in addition to the mechanism of DacResponseReader, and it supports the extra functionality in the subnet-wide SSRP discovery mechanism. The processing layer for those SSRP responses will broadcast a request into the network, then listen for 30s. At the end of this, we'll have one ReadOnlySequence per source IP address and we'll want to know the most recent announcement for each of them.

In theory, this also means that if a malicious client were to announce another server's name, this would be detected. We can't break the assumption that SERVERNAME\INSTANCENAME will be a unique key for the resultant DataTable, but we can at least log it.

@paulmedynski paulmedynski moved this from To triage to Backlog in SqlClient Board Jul 29, 2026
@github-actions

Copy link
Copy Markdown

This pull request has been marked as stale due to inactivity for more than 30 days.

If you would like to keep this pull request open, please provide an update or respond to any comments. Otherwise, it will be closed automatically in 7 days.

@github-actions github-actions Bot added the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Aug 28, 2026
@edwardneal

Copy link
Copy Markdown
Contributor Author

This PR is not stale.

@github-actions github-actions Bot removed the Stale The Issue or PR has become stale and will be automatically closed shortly if no activity occurs. label Aug 29, 2026
paulmedynski added a commit that referenced this pull request Oct 5, 2026
* Migrate review-pr-feedback prompt to an agent skill

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>

* Rewrite review-pr-feedback skill body for the skill format

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>

* Make review-pr-feedback tool-agnostic with capability pre-flight

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>

* Always gather Copilot suppressed feedback in review-pr-feedback

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>

* Reply to all PR feedback, resolve only bot-authored threads

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>

* Infer PR from branch, always inspect all feedback sources

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>

* Gate commit, push, reply and resolve behind explicit approval

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>

* Fix five defects found by running the skill against a real PR

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>

* Add an Author Commentary category, not actionable by default

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>

* Treat an author's self-tag as actionable, not commentary

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>

* Refine self-tag detection against observed usage

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>

* Remove the review-pr-feedback prompt superseded by the skill

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>

* Fix six defects found by Copilot review of the skill

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>

* Stop the reply step from disqualifying its own threads

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>

* Add a trust boundary, constrain credentials, re-check authorship

Three findings from Copilot's second review of #4750, each verified
against the file before being accepted.

Treat fetched content as data. Review comments, review bodies and PR
comments are written by other people and on a public repository by
anyone, yet nothing told the agent they are input rather than
instructions. That matters more here than in most skills: this one lost
its tools allowlist in the migration, so it inherits whatever the agent
has, edits files, runs commands and prepares writes to a public
repository. A Trust boundary section now states that instructions come
only from this file and the user, that fetched text attempting to
redirect the run is itself a finding to report rather than obey, and
that claimed attribution confers no authority.

Constrain credentials. Tool selection offered "a REST/GraphQL call with
a token" without saying where the token comes from, which invites
exactly what the repository's secret-handling policy forbids: routing a
secret through tooling or the model. Paths must now be already
authenticated or read from the environment or a secret store, tokens are
never requested or echoed, and failure reports must describe an
authentication problem without reproducing secret material.

Re-check authorship before resolving. The previous commit fixed
resolution by judging authorship from the step 2 snapshot, and in doing
so introduced a race: a human commenting while the run is in progress is
invisible to that snapshot, so their thread could be resolved from stale
data. Candidates are now re-fetched immediately before resolution and
dropped if anyone else has commented since.

Copilot also reported that replying disqualifies bot threads, which was
already fixed in f6a96bf; that review predates the fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Protect the user's working tree and verify the summary marker

Two findings from Copilot's fourth review of #4750. Both are flaws in
earlier commits on this branch rather than in the original migration.

Respect pre-existing working-tree state. Pre-flight confirmed only that
the workspace was writable, so step 6 could edit over uncommitted work
and step 9 could sweep unrelated staged or untracked files into a commit
presented as this run's. The workspace this was found in demonstrates
the risk: it has carried an untracked notes file through every run of
this skill, and a single wildcard stage would have published it to a
public PR, which deleting the file afterwards does not undo. Pre-flight
now records what was already staged, modified or untracked; step 6 stops
and asks when a planned file overlaps that set; step 9 stages only the
files this run changed, named explicitly, and reports what it left
alone.

Stop trusting the summary marker on its own. The marker added to break
the summary-comment feedback loop is public text, so any commenter can
copy it into their own comment and be skipped by step 4 — letting an
untrusted author decide what this skill is allowed to read, which is
precisely what the trust boundary added in the following commit forbids.
A comment is now excluded only when it carries the marker and was
authored by the account this skill posts as. Any other comment carrying
it is inspected as ordinary feedback and the collision is reported,
since copying the marker is itself a signal.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Order scope before pre-flight, and read collapsed review sections

Two issues from Copilot's fifth review of #4750, both reported only in
the review body with no review comment attached.

Fix the circular ordering. Step 1 opened by requiring pre-flight to be
complete, but pre-flight's first check confirms the PR exists and is
readable, and the repository, PR, branch and authorship it needs are all
resolved by step 1's later bullets. It also meant fetching from an
inferred PR before the confirmation that the same step and the Rules
both require. Scope is now settled and confirmed first, pre-flight runs
against known scope, and gathering follows; reads before confirmation
are limited to what identifying the PR requires.

The document had been wrong while practice was right, which is why four
runs of this skill never hit it: the correct order was followed anyway
and the broken instruction was never exercised.

Read collapsed sections of a review body. This run's finding sat two
`<details>` deep under "Previously missed", in a review whose header
read "Findings: None", with no thread anywhere. Step 3a now says to open
nested sections such as "Previously missed" and "Resolved since last
review", and not to trust a bot's own count of what it found.

Found on a run where every thread was already resolved, so a
thread-only reading of this PR would have reported no outstanding
feedback at all.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Tie resolution to the PR head, and close three status gaps

Four findings from Copilot's sixth review of #4750. One arrived as a
review comment; the other three came from the "Previously missed"
section of the review body, which the previous commit taught this skill
to read.

Require a fix to exist on the PR head before resolving. Push can be
declined or unavailable, yet an item fixed only in the local workspace
was still classified Fixed, and Fixed is terminal, so its bot thread
qualified for resolution while the PR contained no fix at all. Threads
classified Fixed are now resolved only once the change is confirmed
present on the PR's head; otherwise they stay open and the run reports
that the fix is local.

Compare commits, not just branch names. The workspace check matched the
branch name and head repository but never the commit, so a stale or
diverged local branch passed while step 5 judged "Already Addressed"
against a tree the PR does not have and step 6 edited code that was not
under review. Local HEAD is now compared with the PR head and a
divergence is handled like any other workspace mismatch.

Resolve the Already Addressed deadlock. Step 8 excluded that status from
replies while step 11 required a posted reply and listed the status as
eligible, so no such thread could ever resolve. Bot-authored threads in
this status now get a short no-action reply naming what already
satisfied the request; elsewhere they are reported without a reply,
since a human's thread is waiting on that human.

Add Informational to the two status contracts that omitted it, where it
was already valid, terminal and resolution-eligible but unrepresentable.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Select analysis-only mode on other people's PRs

Raised by Copilot at 12:22 and not acted on until now. It appeared once
in a review body's "Previously missed" section, was never repeated, and
three runs of this skill passed over it because they did not read that
section.

Step 1 said to default to analysis and advice on someone else's PR, but
only withheld replies; it never selected the analysis-only mode that
steps 6 and 9 check. The run could therefore still edit files and commit
on a contributor's branch while politely declining to comment, which is
the more invasive half of the behaviour. External PRs now select
analysis-only explicitly, and leave it only when the user asks for this
PR to be changed.

Also harden the gathering that let this sit unnoticed. A "Previously
missed" entry is a re-report of feedback already raised and still
unaddressed, so it is acted on now and reported with how many reviews
have carried it. One item here was repeated across four reviews before
being fixed. And an item that stops appearing is not thereby resolved,
since bots stop repeating themselves, so a body is compared against
earlier ones on the same PR before anything is allowed to drop.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Fix repository resolution, report ordering, and identity assumptions

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>

* Resolve four self-inflicted conflicts and regroup the Rules

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>

* Stop reruns re-answering, and tighten two identity rules

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>

* Let prior-run replies satisfy resolution, and unblock pushing

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>

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants