diff --git a/.agents/skills/pr-review-conduct/SKILL.md b/.agents/skills/pr-review-conduct/SKILL.md index 2880da22..43c74ee1 100644 --- a/.agents/skills/pr-review-conduct/SKILL.md +++ b/.agents/skills/pr-review-conduct/SKILL.md @@ -124,7 +124,8 @@ reviewer's own words to identify it), give one bold verdict per finding (`Fixed `Disproven`, or `No change needed`), state the `(N)` count the block gave so answers can be checked against findings, and link the review round. **Read every round, not only the head.** A suppressed finding does not retire when a later push supersedes it, it just stops showing up in a -head-scoped query while still unanswered. +head-scoped query while still unanswered. Post the answer with `scripts/pr_review.py comment` +from a hub checkout. Do not use a provider connector or reconstruct the GitHub mutation. ## Escalate to the maintainer when @@ -138,7 +139,8 @@ head-scoped query while still unanswered. This skill is the provider-agnostic contract. Use `scripts/pr_review.py` from a hub checkout for the GitHub-specific API operations. `status` reports coverage, threads, body-only findings, and -shapes in one call. `wait` requests and polls in-process. `reply` resolves a thread by matching -the finding's own words instead of a line number a fix push can move. The repository's +shapes in one call. `wait` requests and polls in-process. `comment` posts a PR-conversation +answer after it reads the PR node ID. `reply` resolves a thread by matching the finding's own +words instead of a line number a fix push can move. The repository's `.github/copilot-instructions.md` bootstraps Copilot into the `code-review` skill and its stable coverage marker. Do not reconstruct the API operations by hand. diff --git a/.claude-plugin/fleet-skills/.source-digest b/.claude-plugin/fleet-skills/.source-digest index 4dd2da13..c554a561 100644 --- a/.claude-plugin/fleet-skills/.source-digest +++ b/.claude-plugin/fleet-skills/.source-digest @@ -1 +1 @@ -ac73ed965d3dfe48 +a4b2a2c434e2ab63 diff --git a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md index 2880da22..43c74ee1 100644 --- a/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/pr-review-conduct/SKILL.md @@ -124,7 +124,8 @@ reviewer's own words to identify it), give one bold verdict per finding (`Fixed `Disproven`, or `No change needed`), state the `(N)` count the block gave so answers can be checked against findings, and link the review round. **Read every round, not only the head.** A suppressed finding does not retire when a later push supersedes it, it just stops showing up in a -head-scoped query while still unanswered. +head-scoped query while still unanswered. Post the answer with `scripts/pr_review.py comment` +from a hub checkout. Do not use a provider connector or reconstruct the GitHub mutation. ## Escalate to the maintainer when @@ -138,7 +139,8 @@ head-scoped query while still unanswered. This skill is the provider-agnostic contract. Use `scripts/pr_review.py` from a hub checkout for the GitHub-specific API operations. `status` reports coverage, threads, body-only findings, and -shapes in one call. `wait` requests and polls in-process. `reply` resolves a thread by matching -the finding's own words instead of a line number a fix push can move. The repository's +shapes in one call. `wait` requests and polls in-process. `comment` posts a PR-conversation +answer after it reads the PR node ID. `reply` resolves a thread by matching the finding's own +words instead of a line number a fix push can move. The repository's `.github/copilot-instructions.md` bootstraps Copilot into the `code-review` skill and its stable coverage marker. Do not reconstruct the API operations by hand. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index e3e4ae82..c2194371 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -38,9 +38,10 @@ For every review: 5. End the review body with the exact machine-readable marker required by the `code-review` skill. The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, -and `reply --resolve` commands instead of reconstructing GraphQL queries or copying review -identifiers by hand. Its status gate verifies the current head, diff coverage, output shape, -inline threads, body-only findings, and required checks. +`comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying +review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request +conversation. Its status gate verifies the current head, diff coverage, output shape, inline +threads, body-only findings, and required checks. A formal review with no findings is complete only when it covers the current head and states full diff coverage. A refusal, partial or absent coverage statement, unrecognized output shape, diff --git a/.github/skills/pr-review-conduct/SKILL.md b/.github/skills/pr-review-conduct/SKILL.md index 2880da22..43c74ee1 100644 --- a/.github/skills/pr-review-conduct/SKILL.md +++ b/.github/skills/pr-review-conduct/SKILL.md @@ -124,7 +124,8 @@ reviewer's own words to identify it), give one bold verdict per finding (`Fixed `Disproven`, or `No change needed`), state the `(N)` count the block gave so answers can be checked against findings, and link the review round. **Read every round, not only the head.** A suppressed finding does not retire when a later push supersedes it, it just stops showing up in a -head-scoped query while still unanswered. +head-scoped query while still unanswered. Post the answer with `scripts/pr_review.py comment` +from a hub checkout. Do not use a provider connector or reconstruct the GitHub mutation. ## Escalate to the maintainer when @@ -138,7 +139,8 @@ head-scoped query while still unanswered. This skill is the provider-agnostic contract. Use `scripts/pr_review.py` from a hub checkout for the GitHub-specific API operations. `status` reports coverage, threads, body-only findings, and -shapes in one call. `wait` requests and polls in-process. `reply` resolves a thread by matching -the finding's own words instead of a line number a fix push can move. The repository's +shapes in one call. `wait` requests and polls in-process. `comment` posts a PR-conversation +answer after it reads the PR node ID. `reply` resolves a thread by matching the finding's own +words instead of a line number a fix push can move. The repository's `.github/copilot-instructions.md` bootstraps Copilot into the `code-review` skill and its stable coverage marker. Do not reconstruct the API operations by hand. diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 79b1ab14..e7328ac3 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -25,7 +25,8 @@ This section keeps the full rules and is surfaced at its decision moment by the A state-changing GitHub call is the highest-blast-radius thing an agent does here: it runs under the maintainer's identity, so one wrong target writes to another owner's repository as the maintainer, an outward-facing and hard-to-reverse act. These rules bound every write (a git push, an API mutation, a comment, a label, a merge) on any platform, and they bound a write to a checkout on disk as well, since a blanket add or a hard reset in a working tree another task is using destroys work without ever reaching GitHub. Reads are unrestricted, and how far a local read can be trusted is governed under "Verification Discipline" rather than here. The bounds below are on writes. - **Write only within the owner of the current project's repository.** Every state-changing call targets this project's `origin` or another repository under the same owner, which is the fleet the maintainer already administers. A broad or logged-in identity is capability, not permission: a token that *can* reach another owner's repository does not authorize writing to it. Writing under a **different owner** needs explicit human permission naming that repository, granted deliberately rather than assumed from a token's reach, and a "harmless test" write is still a write, so there is no probe exception. That boundary is where the harm sits, since the incident this rule exists for was a stray comment on a stranger's repository, not work across the maintainer's own projects. Reads from anywhere are fine. -- **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a state-changing call consumes (a node id, a numeric id, a thread or comment id) is captured from a live query in the **same** session into a variable and passed from there. Do not hand-type an id, guess it, recall it from memory or an earlier session, or copy it from documentation or an example. Ids commonly resolve **globally**, so a wrong-but-valid id does not fail. It writes to the wrong target, in someone else's repository. If a query returns no id, stop rather than invent one to proceed. +- **Provider connectors are read-only for fleet work.** Use a provider's GitHub connector for reads where it helps. Perform each GitHub mutation through the documented hub tool, or through authenticated `gh` where no tool owns the operation. This gives Codex, Claude, opencode, and a terminal session one write path with the same checks. It also avoids a connector mutation that predictably lacks repository authorization while the verified `gh` session already has it. A provider-specific instruction may explain how to reach the common path. It never replaces that path with its own mutation surface. +- **Never fabricate, guess, or reuse an identifier passed to a write.** Capture every identifier a state-changing call consumes from a live query in the **same** session. This includes node, numeric, thread, and comment ids. Pass the captured value directly. Do not hand-type an id, recall it from another session, or copy it from documentation or an example. Ids commonly resolve **globally**, so a wrong-but-valid id does not fail. It writes to the wrong target, in someone else's repository. Apply the same rule to an identifier embedded in outward-facing text. Read the complete URL from the live object. Never construct a plausible link from an unverified id. If a query returns no id or URL, stop rather than invent one to proceed. - **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works: decide it should happen, make it happen, and read the result. Never append output-discarding redirection or a force-success tail to a mutation (for example `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, `|| echo`), because the write's output is exactly what must be read. A write that appears to fail is **verified, not assumed harmless**, because the operation may have succeeded on the server while the client reported an error, so confirm the actual state before retrying or moving on. The ban targets hiding a *failure*. An ad-hoc call's response is the only signal you get, so `>/dev/null 2>&1`, `|| true`, and `|| echo`, which swallow the error stream or force success, are never acceptable on one. A committed script under `set -e` is a narrow exception: it may send a write's *stdout* to `/dev/null` to drop the success-response noise, because stderr stays visible and a failed write still aborts loudly (the hub's own `repo-config/configure.sh` does exactly this, and a repository reaches it there rather than carrying a copy). The exception is stdout-only suppression inside a reviewed, fail-loud script, never `2>&1` or a force-success tail, and never an ad-hoc command. - **A refused write is reported, never re-shaped, and the maintainer's say-so does not lift a refusal by the harness.** These are two different permissions and only one of them is the maintainer's to give. When the agent harness refuses a write, the maintainer authorizing it in conversation does not change the outcome, and the identical call is refused again, so a second attempt is not worth making and reading the second refusal as a flake is how an agent starts hunting for another shape of the same request. **That hunt is the failure this rule exists to stop.** Re-expressing a refused `gh` command as a raw `gh api -X POST` reaches the same endpoint with the same identity and the same blast radius, having defeated the one control that stopped it, and it is the more dangerous version because the agent believes it has permission. So a refused write is never re-attempted through a different API surface, a different tool, or a rephrasing, and it is never routed around by the agent writing itself a permission rule, which is self-authorization whatever the maintainer said. Two routes remain, both of them the maintainer's: they add the permission rule themselves, or they run the command themselves. Raise it as a blocked decision naming those two (see "Communicating with the User"), and where the work needs the result rather than the call, say what the agent will verify once the maintainer has run it. **A refusal is also a fact about the contract, not just about the session**: where a required verification can only be performed by a write the agent is refused, the document requiring it says so and names who runs it, since a check that is mandatory and unperformable is quietly dropped and then reported as done. - **Each task runs in its own checkout, in its own directory, on its own feature branch.** The unit is the task rather than the agent, since one agent moving between two repositories meets the same hazard as two agents sharing one tree, and a rule written per agent permits exactly the case that goes wrong. The commands that cross the boundary are the ordinary ones rather than the reckless ones, and each is correct in isolation: a blanket `git add -A` sweeps another task's uncommitted work into the commit, a `git reset --hard` deletes it, and a branch switch carries it into an unrelated change. The mechanical habit that holds the rule up is that a mutating command takes an absolute path, or a `cd` to one in the same invocation, rather than the working directory it inherited, because a read in the wrong directory is a wasted call and a write there is damage. diff --git a/scripts/README.md b/scripts/README.md index 3f89ccb3..c04e645b 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -131,12 +131,14 @@ A repository adds its own `host-tools.json` at its root and the gate layers it o ## `pr_review.py` -One compact digest of a pull request's Copilot review state, replacing a sequence of one-`gh`-call-per-turn polls. `status` prints the digest, `wait` runs the backoff in-process so a long review wait costs one agent turn instead of one per poll, `reply` answers one thread and resolves it, and `claims` reads the description against the branch it describes. Re-requesting a review stays out and its runbook is in [`.github/copilot-instructions.md`][copilot-instructions]. +One compact interface to a pull request's Copilot review loop. `status` prints the digest, and `wait` runs the backoff in-process. `comment` posts a PR-conversation answer, while `reply` answers one thread and optionally resolves it. `claims` reads the description against the branch it describes. The runbook is in [`.github/copilot-instructions.md`][copilot-instructions]. ```sh python3 scripts/pr_review.py status 452 --repo ptr727/ProjectTemplate python3 scripts/pr_review.py claims 452 --repo ptr727/ProjectTemplate python3 scripts/pr_review.py wait 452 --repo ptr727/ProjectTemplate --timeout 2700 +python3 scripts/pr_review.py comment 452 --repo ptr727/ProjectTemplate \ + --body "Suppressed findings (1): **Disproven** - the target is checked before the write." python3 scripts/pr_review.py reply 452 --repo ptr727/ProjectTemplate \ --match "retry count is off by one" --body "Fixed in abc1234: the loop now stops at n." --resolve ``` @@ -178,11 +180,13 @@ The timeout path prints the full digest for the same reason, as a bare `PENDING` The digest also reports the **suppressed findings** a review body collapses into a `
` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation. +`comment` posts one answer in the pull request conversation. A suppressed finding has no thread, so this is the path that records its disposition. The command reads the pull request node ID in the same run and exposes no ID argument. It confirms the returned comment URL and exact body before it reports success. A missing target exits `65`, and an unconfirmed response exits `66`. An unconfirmed response may still have landed, so its output tells the reader to inspect the conversation before retrying. + `reply` posts one answer and resolves one thread, and it exists because the hand-run form keeps failing the same way rather than because a wrapper is tidier. Three instances are on record, each an agent that had read the rule against hand-typing a node id and reached for the literal regardless, the last of them refused by the `gh-write-guard` hook an hour after quoting that same rule in a pull request body. A shape that fails while the reader knows the rule is a shape to remove rather than a rule to restate, so the selector is the finding's own words and there is deliberately no argument a `PRRT_...` id fits in: the id is read from the query in the same run and passed straight to the mutation, and a case asserts the id the mutation carries is the one that run just read. The words are matched against the thread's opening comment rather than against a line number, because a fix push moves the line and every lookup keyed to one then misses, which is how three replies posted against nothing while the resolves still succeeded and closed the threads carrying no answer. Matching is case-insensitive, since the text is quoted back out of a digest by a reader. Every failure is a stop rather than a fallback, because each alternative closes a finding while leaving it unanswered, which is the state a reviewer reads as addressed. No match exits `60` and prints the open threads, since a no-match and an already-answered thread read identically from here. Two matches exit `61` and print both candidates rather than taking the first, `head -n 1` being how a reply lands on the wrong finding. A reply whose response carries no `url`, or a body that came back empty, exits `62` **without** resolving. A resolve that does not report `isResolved` exits `63`, with the reply already posted, so the thread is open behind an answer rather than silently assumed closed. `--resolve` is opt-in rather than the default, because a decline is resolved only once its evidence is in the thread. A target under an owner other than this checkout's exits `64` before anything is read at all, and that refusal takes no environment-variable escape: a grant this process can be handed is one the caller sets on the command that runs it, and a grant the caller writes for itself is not a grant, so the cross-owner case goes through the runbook's explicit `gh` path where the hook reads the maintainer's grant from the session instead. -What this trades away is stated rather than glossed. A mutation spelled as a `gh` command in a shell is read by the `gh-write-guard` hook and one this script performs is not, since the hook sees `python3 pr_review.py reply` and no `gh` write. That is a real loss of a second pair of eyes, and it is taken because what the hook guards against there is a fabricated id, which this removes at the source instead of catching after the fact. The guard's other rule is re-implemented here rather than assumed: the owner check above is the same scope rule, enforced in-process, and it is honest that it stops a mistake rather than a determined caller. The whole-source guard against every other state-changing call stays and was narrowed to these two documents rather than dropped when the first of them arrived. +What this trades away is stated rather than glossed. A mutation spelled as a `gh` command in a shell is read by the `gh-write-guard` hook and one this script performs is not. The script removes that failure at its source instead: every node ID comes from a live query in the same run. The owner check is also enforced in-process. The whole-source test guards against every other state-changing call and counts each reviewed mutation document. `claims` checks that a description does not contradict its own branch, and it exists for the same reason `sha-pin` now resolves: a reference that points at nothing is a silent failure caught by a reviewer or not at all. Three stale descriptions in one session generated six review findings between them, each a reviewer noticing that the body named a commit or a behavior the branch no longer carried. It reads the commits a body **claims** the branch carries and the `uses:` refs it quotes, confirms the head tree still carries each, and exits `70` where one does not. Prose claims stay **out of scope**, since judging those needs a similarity heuristic, which [`spec/section-model.md`][section-model] rejects for exactly the reason it would fail here. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 9028b28d..d747bc0f 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -8,6 +8,10 @@ Discipline" for the rule this implements. Subcommands + comment Post one PR-conversation answer, including a suppressed-finding disposition. The PR + node id is read in the same run, and the returned comment URL and body confirm the + write. Exit 0 = done, 64 = write scope could not be established or excludes the + target, 65 = the PR could not be read, 66 = the response did not confirm the comment. claims Check the description against the branch it describes. A body claiming a commit or quoting a `uses:` ref the head tree no longer carries is a silent failure caught by a reviewer or not at all, and three stale descriptions in one session generated six @@ -61,14 +65,12 @@ 50 = the request is pending and nothing picked it up, which no amount of waiting changes. Recovery is two mutations, and they stay in the runbook. -Reading is the bulk of this and `reply` is the one exception, which is a trade rather -than a free win. A mutation spelled as a `gh` command in a shell is read by the -gh-write-guard PreToolUse hook, and one this script performs is not, since the hook sees -`python3 pr_review.py reply` and no gh write. What the hook guards against there is a -fabricated id, and that is the failure this removes at the source instead: the id is -never in the caller's hands to fabricate. Re-requesting a review stays out, having no -such failure and no id to hide. See .github/copilot-instructions.md for the runbook, and -GOVERNANCE.md "Repository Boundaries and Write Safety" for the rules `reply` enforces. +Reading is the bulk of this and the writing commands are a trade rather than a free win. A +mutation spelled as a `gh` command in a shell is read by the gh-write-guard PreToolUse hook, and +one this script performs is not. The script removes the guarded failure at its source: every node +id comes from a live query in the same run, and the owner boundary is enforced in-process. See +.github/copilot-instructions.md for the runbook, and GOVERNANCE.md "Repository Boundaries and +Write Safety" for the rules these commands enforce. """ from __future__ import annotations @@ -371,9 +373,20 @@ }}} """ -# The two mutations the runbook publishes, in the order it publishes them. -# `url` is fetched because it is the one field that says the reply carried a body. +# The PR id for a conversation comment is read in the same run that writes it. +Q_COMMENT_TARGET = """ +query($o:String!,$r:String!,$n:Int!){ + repository(owner:$o,name:$r){ pullRequest(number:$n){ id url } }} +""" + +# The conversation-comment and thread mutations the runbook publishes. +# `url` is fetched because it is the one field that confirms a comment or reply carried a body. # A reply that posted empty still returns a comment, and three did, each then resolved. +M_COMMENT = """ +mutation($subjectId:ID!,$body:String!){ + addComment(input:{subjectId:$subjectId, body:$body}){ + commentEdge{ node{ id url body } } }} +""" M_REPLY = """ mutation($threadId:ID!,$body:String!){ addPullRequestReviewThreadReply(input:{pullRequestReviewThreadId:$threadId, body:$body}){ @@ -1572,6 +1585,38 @@ def matching_threads(threads: list[dict], match: str, path: str | None) -> list[ ] +def comment_on_pr(owner: str, repo: str, num: int, body: str) -> int: + """Post one PR conversation comment and confirm the returned comment. Returns an exit code.""" + ok, why = in_scope(owner) + if not ok: + print(f"status=OUT_OF_SCOPE nothing was written: {why}") + return 64 + + target = gql(Q_COMMENT_TARGET, owner, repo, num) + if not target or not target.get("id") or not target.get("url"): + print( + f"status=TARGET_NOT_READ nothing was written: {owner}/{repo} #{num} did not " + "return a pull request id and URL, so the mutation has no verified target" + ) + return 65 + + body = body.replace("\r\n", "\n").replace("\r", "\n") + edge = (gh_graphql(M_COMMENT, subjectId=target["id"], body=body).get("addComment") or {}).get( + "commentEdge" + ) or {} + comment = edge.get("node") or {} + if not comment.get("url") or (comment.get("body") or "") != body: + print( + "status=COMMENT_NOT_CONFIRMED the response returned no URL or a different body. " + "Inspect the PR conversation before retrying, since the write may have landed." + ) + print(f" response: {json.dumps(comment)[:400]}") + return 66 + print(f"commented: {comment['url']}") + print("status=COMMENTED") + return 0 + + def reply_to_thread( owner: str, repo: str, num: int, match: str, body: str, path: str | None, resolve: bool ) -> int: @@ -1816,7 +1861,7 @@ def check_claims(owner: str, repo: str, num: int) -> int: def main(argv: list[str] | None = None) -> int: ap = argparse.ArgumentParser() - ap.add_argument("cmd", choices=["claims", "status", "reply", "wait"]) + ap.add_argument("cmd", choices=["claims", "comment", "status", "reply", "wait"]) ap.add_argument("number", type=int) # No default, because the wrong repository is the failure this argument has actually had. # A default names one repository, and every run from elsewhere silently reads that one. @@ -1866,8 +1911,8 @@ def main(argv: list[str] | None = None) -> int: ap.add_argument( "--body", metavar="TEXT", - help="reply: the answer to post, carrying the fixing commit SHA or the " - "evidence that disproves the finding", + help="comment or reply: the answer to post, carrying the fixing commit SHA or " + "the evidence that disproves the finding", ) ap.add_argument( "--resolve", @@ -1878,22 +1923,24 @@ def main(argv: list[str] | None = None) -> int: # Named for the command they belong to, since one silently ignored reads as one that took effect. # A `status` given --body reports a clean digest and writes nothing. # Nothing in that output says the reply never happened. - writing = { + reply_only = { "--match": a.match, "--path": a.path, - "--body": a.body, "--resolve": a.resolve or None, } if a.cmd != "reply": - for flag, value in writing.items(): + for flag, value in reply_only.items(): if value is not None: ap.error(f"{flag} belongs to `reply`, not `{a.cmd}`") - else: - for flag in ("--match", "--body"): - if not (writing[flag] or "").strip(): + if a.cmd not in ("comment", "reply") and a.body is not None: + ap.error(f"--body belongs to `comment` or `reply`, not `{a.cmd}`") + required = ["--body"] + (["--match"] if a.cmd == "reply" else []) + if a.cmd in ("comment", "reply"): + values = {"--body": a.body, "--match": a.match} + for flag in required: + if not (values[flag] or "").strip(): ap.error( - f"reply requires a non-empty {flag}, since a thread resolved on an " - "empty answer reads as addressed while carrying nothing" + f"{a.cmd} requires a non-empty {flag}, since an empty answer records nothing" ) # A negative grace leaves the next reading permanently behind the clock. # That is the per-poll REST pattern the interval exists to prevent. @@ -1921,6 +1968,9 @@ def main(argv: list[str] | None = None) -> int: if a.cmd == "claims": return check_claims(owner, repo, a.number) + if a.cmd == "comment": + return comment_on_pr(owner, repo, a.number, a.body) + if a.cmd == "status": # One payload renders the digest and decides the code, for the reason `wait` reads one. # Fetched twice, a round landing between them prints one pull request and grades another. diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index ce703f28..2dd75c5c 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -28,6 +28,7 @@ REPO = Path(__file__).resolve().parent.parent.parent RUNBOOK = REPO / ".github" / "copilot-instructions.md" +GOVERNANCE = REPO / "GOVERNANCE.md" HEAD = "a" * 40 OLD = "b" * 40 @@ -2347,6 +2348,91 @@ def page(threads: list[dict], more: bool = False, cursor: str | None = None) -> return {"nodes": threads, "pageInfo": {"hasNextPage": more, "endCursor": cursor}} +COMMENT_BODY = "Disproven: the bound is checked before the write." +COMMENT = { + "id": "c1", + "url": "https://github.com/o/r/pull/7#issuecomment-1", + "body": COMMENT_BODY, +} + + +class CommentCase(unittest.TestCase): + """Drive `comment` against a live PR target and a crafted mutation response.""" + + def setUp(self) -> None: + self.out = self.enterContext(contextlib.redirect_stdout(io.StringIO())) + self.enterContext(mock.patch.object(pr_review, "origin_owner", return_value="o")) + self.calls: list[tuple[str, dict[str, object]]] = [] + + def wire(self, target: dict | None = None, comment: dict | None = COMMENT) -> None: + def fake(query: str, **variables: object) -> dict: + self.calls.append((query, variables)) + if "pullRequest(number" in query: + return {"repository": {"pullRequest": target}} + if "addComment" in query: + return {"addComment": {"commentEdge": {"node": comment}}} + raise AssertionError(f"unexpected document: {query[:60]}") + + self.enterContext(mock.patch.object(pr_review, "gh_graphql", side_effect=fake)) + + def run_comment(self, repo: str = "o/r", body: str = COMMENT_BODY) -> int: + return pr_review.main(["comment", "7", "--repo", repo, "--body", body]) + + +class TestCommentConfirmsItsTargetAndResult(CommentCase): + def test_the_pr_id_is_read_live_and_the_returned_comment_is_confirmed(self) -> None: + self.wire({"id": "PR_from_query", "url": "https://github.com/o/r/pull/7"}) + self.assertEqual(0, self.run_comment()) + mutation = next((v for q, v in self.calls if "addComment" in q)) + self.assertEqual("PR_from_query", mutation["subjectId"]) + self.assertEqual(COMMENT["body"], mutation["body"]) + self.assertIn(COMMENT["url"], self.out.getvalue()) + self.assertIn("status=COMMENTED", self.out.getvalue()) + + def test_an_unreadable_pr_stops_before_the_mutation(self) -> None: + self.wire(None) + self.assertEqual(65, self.run_comment()) + self.assertFalse(any("addComment" in query for query, _ in self.calls)) + self.assertIn("TARGET_NOT_READ", self.out.getvalue()) + + def test_a_response_without_a_url_is_not_reported_as_posted(self) -> None: + self.wire( + {"id": "PR_from_query", "url": "https://github.com/o/r/pull/7"}, + {"id": "c1", "url": None, "body": COMMENT["body"]}, + ) + self.assertEqual(66, self.run_comment()) + self.assertIn("COMMENT_NOT_CONFIRMED", self.out.getvalue()) + + def test_a_response_with_a_different_body_is_not_reported_as_posted(self) -> None: + self.wire( + {"id": "PR_from_query", "url": "https://github.com/o/r/pull/7"}, + {"id": "c1", "url": COMMENT["url"], "body": ""}, + ) + self.assertEqual(66, self.run_comment()) + + def test_newlines_are_normalized_before_the_comment_is_sent_and_confirmed(self) -> None: + body = "Suppressed finding:\r\n\r\nDisproven.\rOne boundary applies." + normalized = "Suppressed finding:\n\nDisproven.\nOne boundary applies." + response = {"id": "c1", "url": COMMENT["url"], "body": normalized} + self.wire({"id": "PR_from_query", "url": "https://github.com/o/r/pull/7"}, response) + self.assertEqual(0, self.run_comment(body=body)) + mutation = next((v for q, v in self.calls if "addComment" in q)) + self.assertEqual(normalized, mutation["body"]) + + def test_a_target_under_another_owner_is_refused_before_the_pr_read(self) -> None: + self.wire({"id": "PR_wrong_owner", "url": "https://github.com/x/r/pull/7"}) + self.assertEqual(64, self.run_comment(repo="x/r")) + self.assertEqual([], self.calls) + self.assertIn("OUT_OF_SCOPE", self.out.getvalue()) + + def test_an_unreadable_origin_refuses_before_the_pr_read(self) -> None: + self.wire({"id": "PR_from_query", "url": "https://github.com/o/r/pull/7"}) + with mock.patch.object(pr_review, "origin_owner", return_value=None): + self.assertEqual(64, self.run_comment()) + self.assertEqual([], self.calls) + self.assertIn("OUT_OF_SCOPE", self.out.getvalue()) + + LANDED = {"id": "c1", "url": "https://github.com/o/r/pull/7#discussion_r1", "body": "Fixed in abc."} @@ -2561,6 +2647,19 @@ def test_an_empty_body_is_rejected_rather_than_posted(self) -> None: def test_a_missing_match_is_rejected_rather_than_matching_everything(self) -> None: self.assertIn("--match", self.err(["reply", "7", "--repo", "o/r", "--body", "Fixed."])) + def test_a_comment_requires_a_non_empty_body(self) -> None: + for body in ("", " "): + with self.subTest(body=body): + self.assertIn("--body", self.err(["comment", "7", "--repo", "o/r", "--body", body])) + + def test_reply_only_options_are_rejected_on_comment(self) -> None: + for flag in (["--match", "x"], ["--resolve"], ["--path", "a.py"]): + with self.subTest(flag=flag[0]): + self.assertIn( + flag[0], + self.err(["comment", "7", "--repo", "o/r", "--body", "Fixed.", *flag]), + ) + def test_a_writing_option_on_a_reading_command_is_an_error(self) -> None: """Silently ignored, it reads as an option that took effect on a run that wrote nothing.""" for flag in (["--body", "Fixed."], ["--match", "x"], ["--resolve"], ["--path", "a.py"]): @@ -2854,17 +2953,16 @@ def test_the_runbook_names_partial_coverage_as_a_state_that_blocks_a_merge(self) text = RUNBOOK.read_text(encoding="utf-8") self.assertIn("partial or absent coverage statement", text) - def test_the_only_writes_are_the_three_named_here(self) -> None: - """Every write this script makes is one of three, and each arrived as a reviewed change. + def test_the_only_writes_are_the_four_named_here(self) -> None: + """Every write this script makes is one of four, and each arrived as a reviewed change. The read subcommands are the bulk of it and a mutation reaching them is a digest that writes, so the whole-source guard stays and is narrowed to the documents the script actually needs rather than dropped when the first of them arrived. `requestReviews` - joined `addPullRequestReviewThreadReply`/`resolveReviewThread` deliberately, closing a - gap where `wait` only ever polled and never asked, which twice left it waiting the full - timeout for a review nothing had requested. `union:true` only adds to the request set - (see `request_copilot_review`), so it cannot drop a human reviewer requested alongside - Copilot, unlike the `union:false` clear-and-recover form the runbook documents by hand. + closes the gap where `wait` only polled and never asked. `addComment` owns body-only + finding responses. The reply and resolve pair owns inline threads. `union:true` only + adds to the request set, so it cannot drop a requested human reviewer. The runbook keeps + the `union:false` clear-and-recover form as a manual operation. """ source = (REPO / "scripts" / "pr_review.py").read_text(encoding="utf-8") for verb in ( @@ -2891,8 +2989,9 @@ def test_the_only_writes_are_the_three_named_here(self) -> None: "an automatic one", ) # `mutation(` opens a document, so the count is the number of documents. - # A fourth arriving is a write nobody reviewed as one rather than a style drift. - self.assertEqual(3, source.count("mutation(")) + # A fifth arriving is a write nobody reviewed as one rather than a style drift. + self.assertEqual(4, source.count("mutation(")) + self.assertIn("addComment", source) self.assertIn("addPullRequestReviewThreadReply", source) self.assertIn("resolveReviewThread", source) self.assertIn("requestReviews", source) @@ -2901,8 +3000,22 @@ def test_the_runbook_routes_mutations_to_the_script(self) -> None: """Provider mechanics have one executable owner instead of copied query snippets.""" text = RUNBOOK.read_text(encoding="utf-8") self.assertIn("scripts/pr_review.py", text) + self.assertIn("`comment`", text) self.assertNotIn("mutation(", text) + def test_the_fleet_routes_provider_writes_through_portable_tooling(self) -> None: + """A connector-first write recreates the provider-specific 403 this change removes.""" + text = GOVERNANCE.read_text(encoding="utf-8") + self.assertIn("Provider connectors are read-only for fleet work", text) + self.assertIn("documented hub tool", text) + self.assertIn("authenticated `gh`", text) + + def test_outward_facing_links_require_a_live_url(self) -> None: + """A plausible review ID produced a valid-looking link to no review during this change.""" + text = GOVERNANCE.read_text(encoding="utf-8") + self.assertIn("identifier embedded in outward-facing text", text) + self.assertIn("complete URL from the live object", text) + def test_no_argument_accepts_a_thread_id(self) -> None: """The failure is an id typed into a mutation, so the fix is having nowhere to type one.