Guard Against a Review Reply That Defers Its Thread Resolve

[ptr727]

What happened

On #756, a Copilot review thread was replied to and left unresolved for about seven minutes, across a push and a re-request, and was resolved only on a later pass. The maintainer read the pull request inside that window and saw an answered finding with an open conversation, which is indistinguishable from a finding nobody triaged.

Timeline, all on 2026-08-16 UTC:

Time	Event
03:20:42	Round 1 review 4945287818 lands on acf70a4, one inline thread on host-setup/bootstrap.sh
03:22:14	Reply posted to that thread, fix pushed as 4a9abe4, round 2 requested
03:25:47	Round 2 review lands on the new head
~03:29	Thread PRRT_kwDOQ5caqM6ZkWcG finally resolved

The gate noticed even though a human did not have to: the pull request's ruleset requires conversation resolution, so mergeStateStatus read BLOCKED throughout, for a reason no field names.

Root cause

Not a defect in scripts/pr_review.py, because the script was never run. The reply was hand-rolled as a POST to the review comment replies endpoint, and the resolve as a separate resolveReviewThread mutation issued on a later pass. Splitting the two into separate acts is what creates the window.

.github/copilot-instructions.md already says to reach for the helper first, and the helper already closes this by construction:

scripts/pr_review.py reply <N> --repo <owner>/<name> --match "<words from the finding>" --body "<answer>" --resolve queries the thread id itself and passes it straight to the mutation.

The same paragraph also records why another sentence will not fix it:

That rule is known and read by the agents that break it anyway, three times so far, so the shape is what changes rather than the wording.

This is the fourth. The wording is not the problem. The hand-run GraphQL form documented below that paragraph is the path an agent falls into, since it presents reply and resolve as two snippets, which an agent then runs as two decisions.

Suggested fix

Extend host-setup/agent-safety/gh-write-guard.py with a third denial class, alongside the discarded-output and literal-node-id rules it already carries. Deny, from the Bash PreToolUse matcher:

• a resolveReviewThread mutation issued directly through gh api graphql
• a POST to the pull request review comment replies endpoint

The refusal names scripts/pr_review.py reply --match ... --body ... --resolve as the path to take instead, the way the existing rules name what to do rather than only what is refused.

This mirrors the precedent exactly. The discarded-output rule exists because a mutation whose result is thrown away is a class of mistake no amount of documentation stopped, and it fired correctly during this same session on a requestReviews call carrying a > /dev/null tail. The reply-without-resolve split is the same shape of mistake with the same history.

Two properties worth keeping in the implementation:

• Deny the mechanism, not the intent. Reading threads, requesting reviews, and posting an issue-level comment stay untouched. Only the two calls the helper supersedes are refused.
• Leave an escape. A cross-owner pull request, which pr_review.py refuses outright, still needs the hand-run form, so the guard permits it on the same GH_WRITE_GUARD_ALLOW grant the cross-owner write rule already reads.

Alternative considered and rejected

Stating "do not defer the resolve to a later pass" in pr-review-conduct and in the runbook. It is the fifth restatement of a rule already stated in three places, and the file itself records that restating it has not worked.

A second, smaller finding in the same file

Filing this issue was itself denied by gh-write-guard.py, as a cross-owner write to an owner named .... The command was an gh issue create --repo ptr727/ProjectTemplate whose --body prose quoted an API path in the abbreviated form a reader expects, and the guard's target extraction read that path out of the body text as the write's target.

The deny was correct in shape and wrong in fact, and the body had to move to --body-file to get the issue filed. Worth handling while the file is open, since a guard that fires on prose describing a path teaches an agent to route around it, and routing around a guard is the behavior least wanted here. Target extraction should read only an argument in target position, never text carried by --body, --title, --field body, or a heredoc feeding one.

[ptr727]

A third false positive, and a sharper statement of the second

Three more denials while finishing #756, all on writes that were correct. Each was reproduced against classify() directly rather than by firing a write, so nothing below was learned by probing the server.

1. A redirect is attributed across a compound command

A redirect on an unrelated local command is read as belonging to a git push later in the same compound.

A. './host-setup/linux/install-tools.sh --sudo-timestamp git > /dev/null 2>&1; git push <ssh-url> feature/x:feature/x'
   -> DENY  "This is a GitHub write with its output discarded or forced to success ..."

B. './host-setup/linux/install-tools.sh --sudo-timestamp git; git push <ssh-url> feature/x:feature/x'
   -> ALLOW

The two differ only in a redirect on the first command, which is a local script whose exit code the run was checking. The push carries no redirect in either case. This is the discarded-output rule reaching a token that is not the write's.

2. The body-text case is not heredoc-specific

The report above frames this as text carried through a heredoc. Measured both ways, it is neither the heredoc nor the fallback line-splitting:

A. gh issue create --repo <this-repo> --title T --body "$(cat <<'EOF' ... prose quoting a gh api repos/... call ... EOF)"
   -> DENY  "This write targets .../pulls, under a different owner ..."

B. gh issue create --repo <this-repo> --title T --body "<the same prose on one line>"
   -> DENY  "This write targets .../pulls, under a different owner ..."

A single-line --body denies identically, so target extraction is scanning the whole command text for a repos/<a>/<b> token wherever it sits, rather than reading the argument in target position. The --repo flag naming this repository is present and correct in both.

3. The rule reaches a command that is not a write at all

The clearest case. A read-only Python script, passed to python3 on standard input, was denied because the script's own data quoted a gh api repos/... string. The script imported gh-write-guard.py and called classify() on some strings. It made no network call, ran no gh, and wrote nothing.

That is the same root cause as case 2, seen from further away: the scan reads the command line as a bag of tokens, so any payload that merely mentions a call is judged as making one. The workaround, assembling the token from string parts so the script no longer contains it literally, is the shape worth worrying about, because it is indistinguishable from what someone routing around the guard would write.

What this adds to the suggested fix

The original suggestion stands. These sharpen where the parsing has to change:

• Attribute a redirect to the command it belongs to, not to any write in the same compound. Case 1's A and B differ only in a redirect on a different command.
• Read a target only from an argument in target position. Not from --body, --title, --field, standard input, or any other payload, and not from a bare repos/<a>/<b> token found anywhere in the text.
• Judge only a command in command position. Case 3 was not a gh invocation at all.

Case 3 also suggests a test worth having: a command whose payload quotes a write, asserted to be allowed. A guard that cannot tell a mention from an invocation will keep teaching its users to phrase things so it does not notice, which is the failure mode that costs the most here.