diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 4bc5e698..b36cfdf1 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -213,6 +213,10 @@ If a review did not run on the current head, retry: Every id below is captured from a live query into a variable and passed from there, never hand-typed, guessed, or pasted as a `PRRT_...` literal. A node id resolves globally, so a fabricated or stale id does not fail, it writes to a real thread on an unrelated repository. This runbook implements [GOVERNANCE.md "Repository Boundaries and Write Safety"](../GOVERNANCE.md#repository-boundaries-and-write-safety): write only to this repo, capture every id from a live query, and never suppress a mutation's output. +**Use the hub's helper, which has nowhere to type an id.** `scripts/pr_review.py reply --repo / --match "" --body "" --resolve` queries the thread id itself and passes it straight to the mutation. 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. It selects on the finding's own words rather than a line number, since a fix push moves the line; it refuses on no match and on more than one rather than picking; and it does not resolve a thread whose reply came back without a `url`. Cross-owner targets it refuses outright, which is where the hand-run form below applies, and there the `gh-write-guard` hook is what reads the maintainer's grant. It is hub-hosted per [GOVERNANCE.md "Hub-Hosted Tooling"](../GOVERNANCE.md#hub-hosted-tooling), so it is invoked from a hub checkout and never rebuilt locally. + +The hand-run form is below, for a cross-owner target and for the case where the hub cannot be reached and the work cannot wait. + List unresolved threads. Use `first: 100` with cursor-based pagination, and where `hasNextPage` is true, re-run with `after: ""` to retrieve the next page: ```sh diff --git a/OPERATIONS.md b/OPERATIONS.md index 575bc46d..456f4d06 100644 --- a/OPERATIONS.md +++ b/OPERATIONS.md @@ -98,7 +98,7 @@ The `editorconfig-checker` action is setup-only. Using it alone silently skips t Two `gh` limitations on the current host, both worked around rather than fixed: - `gh pr checks` carries no `--json` flag on the installed `gh` 2.46.0, so a watcher built on it prints nothing and a quiet result reads as a passing one. Read the checks from `gh pr view --json statusCheckRollup` instead. -- `gh pr edit --base` fails with a Projects-classic deprecation error. Use `gh api --method PATCH repos/[owner/repo]/pulls/[number] -f base=[branch]` instead. +- `gh pr edit` fails with a Projects-classic deprecation error whichever field it is given, `--base`, `--title` and `--body-file` alike, since the failure is in the mutation the command builds rather than in the field asked for. It exits non-zero without applying the change, so a stale pull request description survives review rounds. Use `gh api --method PATCH repos/[owner/repo]/pulls/[number]` with the field instead, `-f base=[branch]` or `-F body=@[file]`, and verify it took. [.github/copilot-instructions.md](./.github/copilot-instructions.md) carries the same limitation against the title and body under "PR Edits and Merge-State Gotchas". ## Configuration Layout diff --git a/TODO.md b/TODO.md index a581f793..c4d10b2f 100644 --- a/TODO.md +++ b/TODO.md @@ -29,7 +29,7 @@ One pull request clearing prose findings, leading with [`catalog/snippets/`][sni **State** `ready`. **Touches** [`catalog/snippets/`][snippets] first, then the hub's docs and spec. **Cost** one hub edit per batch, hub-only, and a snippet fix reaches the fleet only as repos re-adopt. - **Clear the [#519][issue-519] prose backlog, snippets first.** The whole-tree figure moves as readily with a fix to the gate as with a fix to the prose, so it is re-measured rather than quoted. - - **Blocked by** - Nothing, though a run after "The Prose Gate Scope Floor" ships is the one worth trusting. + - **Blocked by** - Nothing. - **Issue** - [#519][issue-519], whose headline numbers are stale and whose four planned changes are two-thirds landed. - **Checked** - `develop` at `3d1a0b1` on 2026-08-06, where `python3 scripts/prose_lint.py --summary` reports 557 violations across 45 files, and `catalog/snippets` alone reports 184 across 19. - **Open** - Whether the gate becomes a carried file rather than a hub-only one, which "Reducing the Carried Surface Further" asks from the other direction. @@ -75,21 +75,6 @@ One pull request fixing the argument-list split in the installed hook, plus the - **Settled** - The `&&` form resolves correctly, which is what isolates the defect to the newline case, and the existing suite covers only that form. - **Settled** - The error direction is over-blocking rather than under-blocking, so it is a usability defect rather than a safety hole, and that is why it is worth fixing: the denial claims a direct push to a protected branch when the push targets an ordinary feature branch, and teaching a safety hook to cry wolf is how it stops being read. -### The Reply-and-Resolve Helper - -One pull request moving a command that keeps failing the same way into a helper, rather than restating the rule it keeps failing against. - -**State** `ready`. **Touches** [`scripts/pr_review.py`][pr-review] and its test file. **Cost** one hub edit, hub-only, and it is runnable from every repository the day it lands. - -- **Add a reply-and-resolve helper that queries thread ids itself.** It takes a pull request number and a finding, and never puts an id anywhere a hand can type one. - - **Blocked by** - Nothing. - - **Issue** - [#580][issue-580], which carries the decision this implements. - - **Checked** - `develop` at `1ed0cc8` on 2026-08-03. - - **Open** - Nothing. - - **Settled** - Two failures are on record, a command run from the wrong working directory that returns a confident answer about a different tree, and a GitHub node id typed by hand into a mutation that the `gh-write-guard` hook correctly refused. - - **Settled** - The second is the instructive one, since the rule it broke is stated in [`GOVERNANCE.md`][governance] and in the host-level guidance, and an agent that had read both still reached for the literal, which says the shape of the operation fails rather than the agent's knowledge of the rule. - - **Settled** - The dependency is met, since [`GOVERNANCE.md`][governance] "Hub-Hosted Tooling" states how a repository reaches a hub script. - ### Three Rules That Leave the Recurring Case Unstated One pull request widening three carried [`GOVERNANCE.md`][governance] rules that each state their common case and go quiet on the case that recurs, filed together because they share that shape and land in one re-vendor. @@ -398,6 +383,7 @@ Regenerate [reports/divergences.md][divergences-report] before using it as the w - **Detail** - Three further [`GOVERNANCE.md`][governance] sections differ by a single word each, "Documentation Style Conventions", "Communicating with the User" and "Repository Details", where a format name took the capitalization [`CODESTYLE.md`][codestyle] "Markdown and Spelling" states, so they are byte-mismatched for a reason a reader of the diff would otherwise call cosmetic. - **Detail** - Two comment lines in [`.markdownlint-cli2.jsonc`][markdownlint] took the same capitalization, and that file is `verbatim` and `whole`, so every downstream copy is byte-mismatched on a config nothing else changed about. - **Detail** - [`CODESTYLE.md`][codestyle] is the fifth file, at `intent` rather than `verbatim`, so it reaches the fleet as a rule each repo adopts in its own copy, and the same mixed spelling waits in every downstream tree. + - **Detail** - [`.github/copilot-instructions.md`][copilot-instructions] is the sixth, also at `intent`, where "Reply and Thread Resolution Workflow" now leads with the hub's reply helper and keeps the hand-run mutations as the cross-owner and unreachable-hub path. A repo taking the old copy is not broken by it, since the mutations it documents still work, so this rides the visit rather than gating it. - **Carry the `Local Verification` heading into every repository's `OPERATIONS.md`.** The heading leads the file and states what verifying a change there requires, naming the part of the repo's contract CI structurally cannot exercise, and a repo whose gates are entirely in CI says that under it rather than omitting it. - **Hub state** - Done, verified `develop` at `8e10a2c` on 2026-08-06, where [`spec/section-model.md`][section-model] and [`STANDUP.md`][standup] declare six headings and this repo's own [`OPERATIONS.md`][operations] leads with the section. diff --git a/scripts/README.md b/scripts/README.md index 868dfca9..b8bb89b0 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -86,20 +86,22 @@ A stale-backticked-path check was built and **rejected**: a template repo legiti ## `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, and `wait` runs the backoff in-process so a long review wait costs one agent turn instead of one per poll. Read-only by design: the mutations (re-request, reply, resolve) stay as explicit `gh` calls so they remain visible to the `gh-write-guard` hook and to review, and their runbook is in [`.github/copilot-instructions.md`][copilot-instructions]. +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, and `reply` answers one thread and resolves it. Re-requesting a review stays out and its 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 wait 452 --repo ptr727/ProjectTemplate --timeout 2700 +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 ``` `--repo` is required and carries no default. A default names one repository, and a run from anywhere else resolves its number there instead: the digest renders, every field is well-formed, and nothing in the output disagrees. Two runs read this repository's pull requests while their own was the subject, each caught by the maintainer rather than by the run. The digest leads with `repo=OWNER/NAME` for the same reason, since a number alone reads as correct in any repository. A value that is not `OWNER/NAME` is rejected by name rather than raised as an unpacking traceback, that being the near-miss a required argument still admits. -`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another asserts no mutation has crept into a read-only script. +`wait` exits `30` when the review is still pending at the timeout, which is pending rather than failed. Its failure mode is a wrong answer rather than a crash, so the cases feed crafted GraphQL payloads: a review attributed to the wrong login, a review counted against a stale head, a maintainer's own thread read as a finding, and a wait that returns success while nothing landed. One case reads the reviewer login out of the runbook rather than restating it, since GraphQL drops the `[bot]` suffix REST carries, and another holds the script to exactly the two mutations `reply` owns, so a third arriving is a write nobody reviewed as one. `wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, since GraphQL offers no author filter, so ordinary traffic is what pushes theirs out of reach. `window_blind` is the one guard over both sides, and each side fails differently. Blind on **comments** means an answer could be back there unseen, which reads as `answered_outside_review=unknown` rather than `no`. Blind on **reviews** is worse, because the newest review in view is then not the newest there is, and an empty baseline dates every comment as newer so each one reads as an answer: a false `40` that stops the loop on a pull request whose review actually landed. That case reports nothing and lets the wait keep polling, since a wait that runs on is visible where a wrong terminal is not. -Everything else is decidable and says so. One of the reviewer's own nodes in view, even a **spent** one, settles the question, because nodes arrive in creation order, so anything behind the window is older than everything inside it. A window holding every node the pull request has is settled too, which is why the guard reads `pageInfo.hasPreviousPage` rather than the node count: a full window and a complete one are the same length, so length alone would report a gap where none exists. Cases hold `WINDOW` equal across all four windows and hold all four to asking for `hasPreviousPage`, since a connection that stops asking reports `no` instead of `unknown`, the silent narrowing one level up. `wait` exits `50` when the reviewer sits in the pending request set and no `copilot_work_started` follows the newest request, meaning nothing is acting on it and waiting on will not start it. That state is invisible from the reviews alone and indistinguishable from patience: one request sat thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot separate it from a slow round either, so the pickup event decides. It is the one thing here read over REST, since no GraphQL timeline item carries it, and it runs on its own interval rather than per poll: the first read comes after `--pickup-grace` (default five minutes), because inside that window a pending request is simply a review being worked on, and each later read waits another interval. One reading settles the request in front of it, and the next covers a request a push raises mid-wait, so a long wait costs a handful of REST calls instead of one per poll. The pickup is checked **before** the timeout, so the stall reports as itself instead of as `PENDING` once the clock runs out. Recovery stays out of this script, which holds its no-mutation contract: the digest names the state and the runbook carries the two mutations that clear and re-raise the request. The pending set is read through GraphQL rather than `gh pr view --json reviewRequests`, which omits a Bot reviewer outright and reports an empty set while Copilot sits in it. +Everything else is decidable and says so. One of the reviewer's own nodes in view, even a **spent** one, settles the question, because nodes arrive in creation order, so anything behind the window is older than everything inside it. A window holding every node the pull request has is settled too, which is why the guard reads `pageInfo.hasPreviousPage` rather than the node count: a full window and a complete one are the same length, so length alone would report a gap where none exists. Cases hold `WINDOW` equal across all four windows and hold all four to asking for `hasPreviousPage`, since a connection that stops asking reports `no` instead of `unknown`, the silent narrowing one level up. `wait` exits `50` when the reviewer sits in the pending request set and no `copilot_work_started` follows the newest request, meaning nothing is acting on it and waiting on will not start it. That state is invisible from the reviews alone and indistinguishable from patience: one request sat thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot separate it from a slow round either, so the pickup event decides. It is the one thing here read over REST, since no GraphQL timeline item carries it, and it runs on its own interval rather than per poll: the first read comes after `--pickup-grace` (default five minutes), because inside that window a pending request is simply a review being worked on, and each later read waits another interval. One reading settles the request in front of it, and the next covers a request a push raises mid-wait, so a long wait costs a handful of REST calls instead of one per poll. The pickup is checked **before** the timeout, so the stall reports as itself instead of as `PENDING` once the clock runs out. Recovery stays out of this script: the digest names the state and the runbook carries the two mutations that clear and re-raise the request, neither of which has an id to hide and so neither of which is worth wrapping. The pending set is read through GraphQL rather than `gh pr view --json reviewRequests`, which omits a Bot reviewer outright and reports an empty set while Copilot sits in it. `wait` exits `41` when the review carrying the head is a **refusal**, meaning its body opens by saying it did not review. That answer arrives as a formal review, `state: COMMENTED`, with the correct commit and zero inline threads, so it satisfies every coverage check a clean pass does and renders a digest byte for byte identical to one. The `40` reasoning does not reach it, because that reasoning rests on a comment carrying no commit, and this carries the right one. A pull request of 301 changed files, one over the reviewer's limit of 300, read as `rounds=1 review_on_head=yes threads=0 unresolved=0 merge=CLEAN` and was one command from merging on a review that never ran. A refusal is therefore not coverage: `review_on_head` reports `NO`, the summary line carries a `refusal=YES` of its own, since `rounds=1 review_on_head=NO` is equally what a stale round looks like and the two want opposite responses, and the body prints whole because its wording is the only thing separating a file-count refusal, cleared by splitting the pull request, from a quota one, cleared by waiting. The script reads neither cause, only that the round declined. The match is on the body's **opening line**, since a refusal is the whole body where a review that merely quotes the wording carries it below its own overview, and this script and this file are exactly that quotation. One line rather than two, because a review's first line is its heading and its second is the overview prose: reading two passed every case except the review describing this check, which reported itself as a refusal of itself. The cost is the other direction, that a refusal introduced by a heading would sit below the opening and be missed, and answering that shape means telling a refusal from an overview rather than reading one line further. It is an alternation over the runbook's phrasings for the same reason the suppressed heading is, and a case asserts the script's pattern is the one the runbook publishes. The reading is **head-scoped**, unlike a suppressed finding, because a refusal is a statement about one commit that a push retires, and a genuine review of that same head outranks it, coverage that landed being coverage. The field is spent by that coverage as well as the exit code is, or the summary line reads `review_on_head=yes refusal=YES` and tells a reader to split a pull request the reviewer has just reviewed. The liveness query carries no bodies, so a refusal reads there as ordinary coverage. That is deliberate: it ends the wait, which is what a terminal outcome should do, and the full read every wait finishes with is what tells the two apart, so no exit code comes from the cheaper reading. @@ -107,6 +109,12 @@ 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. +`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. + The match is on the block's heading rather than anywhere in the body, and on the runbook's alternation rather than on one phrasing, since the wording has already appeared two ways. A case asserts the script's pattern is the one the runbook publishes rather than a copy of it that can drift. Reading the whole body was the first implementation and its own review caught it: a review whose overview prose discusses suppressed findings carries none, and reporting that as a finding trains the reader to skim the field. A heading outside any `
` wrapper is still read, because reporting zero when the markup moves is the same false clean one level up, and that fallback takes a count so ordinary prose does not become one. diff --git a/scripts/pr_review.py b/scripts/pr_review.py index 95161235..72b36d1c 100644 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -9,6 +9,15 @@ Subcommands status One digest line, any unresolved threads, and any suppressed findings. Read-only. + reply Answer one thread selected by its text, and resolve it on request. The only + writing path here, and it exists because the hand-run form keeps failing the + same way: a node id typed into a mutation, which resolves globally and so + writes to a real thread somewhere rather than failing. This takes a pull + request number and words from the finding, queries the id itself, and offers + no argument an id fits in. Exit 0 = done, 60 = no thread matched, 61 = more + than one did, 62 = the reply returned no comment url so nothing was resolved, + 63 = the resolve did not report the thread resolved, 64 = the target is under + another owner. wait Poll until Copilot's review lands on the current head, then print the digest. The loop runs in-process, so a 45-minute wait costs one agent turn, not 90. Exit 0 = review present, 30 = still pending at timeout (pending is not failure), @@ -19,13 +28,18 @@ 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. -Read-only by design. Mutations (re-request review, reply, resolve thread) are -deliberately NOT implemented here - they are state-changing calls that must stay -visible to the gh-write-guard PreToolUse hook and to review. See -.github/copilot-instructions.md for the mutation 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. """ from __future__ import annotations import argparse, json, re, subprocess, sys, time +from pathlib import Path REVIEWER = 'copilot-pull-request-reviewer' @@ -96,15 +110,59 @@ """ -def gql(query: str, owner: str, repo: str, num: int) -> dict: - r = subprocess.run( - ['gh', 'api', 'graphql', '-f', f'query={query}', - '-F', f'o={owner}', '-F', f'r={repo}', '-F', f'n={num}'], - capture_output=True, text=True) +# Threads for the reply path, paginated. +# A first page read as the whole set reports no match on a thread that is simply further along. +# `line` is read for the confirmation line rather than for matching, since a push moves it. +# That is how a reply keyed on a line number went to a thread that had shifted underneath it. +Q_THREADS = """ +query($o:String!,$r:String!,$n:Int!,$after:String){ + repository(owner:$o,name:$r){ pullRequest(number:$n){ + reviewThreads(first:100, after:$after){ + nodes{ id isResolved path line comments(first:1){ nodes{ author{login} body } } } + pageInfo{ hasNextPage endCursor } } + }}} +""" + +# 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. +# A reply that posted empty still returns a comment, and three did, each then resolved. +M_REPLY = """ +mutation($threadId:ID!,$body:String!){ + addPullRequestReviewThreadReply(input:{pullRequestReviewThreadId:$threadId, body:$body}){ + comment{ id url body } }} +""" +M_RESOLVE = """ +mutation($threadId:ID!){ + resolveReviewThread(input:{threadId:$threadId}){ thread{ id isResolved } }} +""" + + +def gh_graphql(query: str, **variables) -> dict: + """Run one GraphQL document and return its `data`, raising rather than reporting a blank. + + A string goes through `-f` and an int through `-F`, because `-F` infers a type from the text: + a reply body of `123` or `true` arrives as an Int or a Boolean and the mutation fails on a + type nobody passed it, and a body opening with `@` is read as a filename. + + `errors` is checked rather than trusted to the exit code, since a GraphQL document can fail + per-field while the request itself succeeds, and the caller would read the null that leaves. + """ + argv = ['gh', 'api', 'graphql', '-f', f'query={query}'] + for name, value in variables.items(): + argv += ['-F' if isinstance(value, int) else '-f', f'{name}={value}'] + r = subprocess.run(argv, capture_output=True, text=True) if r.returncode != 0: sys.stderr.write(r.stderr[:800]) raise SystemExit(f'gh graphql failed rc={r.returncode}') - return json.loads(r.stdout)['data']['repository']['pullRequest'] + payload = json.loads(r.stdout) + if payload.get('errors'): + sys.stderr.write(json.dumps(payload['errors'])[:800]) + raise SystemExit('gh graphql reported errors') + return payload['data'] + + +def gql(query: str, owner: str, repo: str, num: int) -> dict: + return gh_graphql(query, o=owner, r=repo, n=num)['repository']['pullRequest'] def timeline(owner: str, repo: str, num: int) -> list[tuple[str, str]]: @@ -432,9 +490,158 @@ def digest(owner: str, repo: str, num: int, seen: set[str] | None = None, return '\n'.join(lines), len(unresolved) +def origin_owner() -> str | None: + """The owner of the checkout this script sits in, or None where that cannot be read. + + Anchored on the script's own directory rather than the working directory, because this is + reached from a hub checkout while the repository being answered is named on the command line, + so the working directory says nothing about who owns either. + """ + try: + url = subprocess.run(['git', '-C', str(Path(__file__).resolve().parent), + 'remote', 'get-url', 'origin'], + capture_output=True, text=True, timeout=5).stdout.strip() + except Exception: + return None + m = re.search(r'[:/]([A-Za-z0-9_.\-]+)/([A-Za-z0-9_.\-]+?)(?:\.git)?/?$', url) + return m.group(1).lower() if m else None + + +def in_scope(target_owner: str) -> tuple[bool, str]: + """Whether writing to `target_owner` is in scope here, with the reason where it is not. + + Same owner covers the origin and every sibling, which is the whole fleet. A different owner is + the shape that once put a comment on a stranger's pull request, and it is refused outright + rather than granted by an environment variable, because a variable this process can be handed + is one the caller can set on the command that runs it, and a grant the caller writes for + itself is not a grant. The `gh-write-guard` hook reads that grant from the session it was + launched with, which is why the cross-owner case belongs on the runbook's explicit `gh` path + where the hook is the one adjudicating it. + """ + origin = origin_owner() + if origin is None: + return False, ('this checkout has no readable `origin`, so the owner a write would stay ' + 'within cannot be established, and an unverified scope is not a scope') + if target_owner.lower() != origin: + return False, (f'the target is under {target_owner}, and this checkout is under {origin}. ' + 'A different owner is the shape this refuses outright: take it through the ' + 'runbook mutations, where the write-guard hook reads the maintainer grant') + return True, '' + + +def first_comment(thread: dict) -> dict: + """The thread's opening comment, which is the finding itself.""" + return ((thread.get('comments') or {}).get('nodes') or [{}])[0] + + +def unresolved_threads(owner: str, repo: str, num: int) -> list[dict]: + """Every unresolved review thread, following the cursor to the end. + + Stopping at the first page reports no match on a thread that is merely further along, and a + no-match is indistinguishable from a thread that was already answered. + """ + out: list[dict] = [] + after = None + while True: + extra = {'after': after} if after else {} + conn = gh_graphql(Q_THREADS, o=owner, r=repo, n=num, + **extra)['repository']['pullRequest']['reviewThreads'] + out += [t for t in conn['nodes'] if not t['isResolved']] + page = conn.get('pageInfo') or {} + if not page.get('hasNextPage'): + return out + after = page['endCursor'] + + +def describe(thread: dict) -> str: + """One line naming a thread by what a reader recognizes it as, never by its id.""" + c = first_comment(thread) + body = ' '.join((c.get('body') or '').split()) + return (f'{thread.get("path")}:{thread.get("line")} ' + f'by {(c.get("author") or {}).get("login")}: {body[:120]}') + + +def matching_threads(threads: list[dict], match: str, path: str | None) -> list[dict]: + """Threads whose finding text contains `match`, narrowed by `path` where one is given. + + Matched on the finding's own words rather than on a line number, because a fix push moves the + line and every lookup keyed to one then misses: replies posted against nothing while the + resolves still succeeded, so the threads closed carrying no answer. Case-insensitive, since + the text is quoted back out of a digest by a reader rather than compared by a machine. + """ + needle = match.lower() + return [t for t in threads + if needle in (first_comment(t).get('body') or '').lower() + and (path is None or t.get('path') == path)] + + +def reply_to_thread(owner: str, repo: str, num: int, match: str, body: str, + path: str | None, resolve: bool) -> int: + """Answer the one thread `match` selects, and resolve it where asked. Returns an exit code. + + Every refusal below is a stop rather than a fallback. There is no id to guess at, no + second-best thread to settle for, and no resolve on a reply that did not land, because each + of those closes a finding while leaving it unanswered, which is the state a reviewer reads as + addressed. + """ + ok, why = in_scope(owner) + if not ok: + print(f'status=OUT_OF_SCOPE nothing was written: {why}') + return 64 + + threads = unresolved_threads(owner, repo, num) + hits = matching_threads(threads, match, path) + if not hits: + print(f'status=NO_MATCH nothing was written: no unresolved thread on {owner}/{repo} ' + f'#{num} carries {match!r}' + + (f' at {path}' if path else '') + + '. Widen the words or drop --path rather than reaching for an id, since the ' + 'thread may also be resolved already, which reads the same from here.') + for t in threads: + print(f' unresolved: {describe(t)}') + return 60 + if len(hits) > 1: + print(f'status=AMBIGUOUS nothing was written: {len(hits)} unresolved threads carry ' + f'{match!r}, and picking one of them is the failure this avoids rather than a ' + 'default it can take. Quote more of the finding, or add --path.') + for t in hits: + print(f' candidate: {describe(t)}') + return 61 + + target = hits[0] + print(f'answering: {describe(target)}') + reply = (gh_graphql(M_REPLY, threadId=target['id'], body=body) + .get('addPullRequestReviewThreadReply') or {}) + comment = reply.get('comment') or {} + # The url is what says a reply carried a body, and an empty one still returns a comment. + # Resolving past that closes the thread with nothing in it, which is what happened three times. + if not comment.get('url') or not (comment.get('body') or '').strip(): + print('status=REPLY_NOT_CONFIRMED the reply returned no url or an empty body, so the ' + 'thread is NOT resolved and the answer is not recorded. Read the response above ' + 'before retrying, since a write that appears to fail may have taken on the server.') + print(f' response: {json.dumps(reply)[:400]}') + return 62 + print(f'replied: {comment["url"]}') + + if not resolve: + print('status=REPLIED the thread is answered and left open, since --resolve was not ' + 'given. A decline is resolved only once its evidence is in the thread.') + return 0 + + thread = ((gh_graphql(M_RESOLVE, threadId=target['id']).get('resolveReviewThread') or {}) + .get('thread') or {}) + if not thread.get('isResolved'): + print('status=RESOLVE_NOT_CONFIRMED the reply landed and the resolve did not report the ' + 'thread resolved, so it is still open and the answer is already posted.') + print(f' response: {json.dumps(thread)[:400]}') + return 63 + print('status=REPLIED_AND_RESOLVED') + return 0 + + def main(argv: list[str] | None = None) -> int: ap = argparse.ArgumentParser() - ap.add_argument('cmd', choices=['status', 'wait']) + ap.add_argument('cmd', choices=['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. @@ -446,7 +653,33 @@ def main(argv: list[str] | None = None) -> int: ap.add_argument('--timeout', type=int, default=2700, help='seconds (default 45m)') ap.add_argument('--pickup-grace', type=int, default=300, help='seconds before the first pickup read, and between reads (default 5m)') + # `reply` takes the finding's words rather than its id. + # There is deliberately no argument an id fits in, so the caller never holds one to mistype. + ap.add_argument('--match', metavar='TEXT', + help='reply: words from the finding, matched against the thread\'s opening ' + 'comment, and required to select exactly one unresolved thread') + ap.add_argument('--path', metavar='FILE', + help='reply: narrow --match to one file, for a file with several findings') + ap.add_argument('--body', metavar='TEXT', + help='reply: the answer to post, carrying the fixing commit SHA or the ' + 'evidence that disproves the finding') + ap.add_argument('--resolve', action='store_true', + help='reply: resolve the thread once the reply is confirmed') a = ap.parse_args(argv) + # 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 = {'--match': a.match, '--path': a.path, '--body': a.body, + '--resolve': a.resolve or None} + if a.cmd != 'reply': + for flag, value in writing.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(): + ap.error(f'reply requires a non-empty {flag}, since a thread resolved on an ' + 'empty answer reads as addressed while carrying nothing') # A negative grace leaves the next reading permanently behind the clock. # That is the per-poll REST pattern the interval exists to prevent. if a.pickup_grace < 0: @@ -462,6 +695,9 @@ def main(argv: list[str] | None = None) -> int: print(out) return 0 + if a.cmd == 'reply': + return reply_to_thread(owner, repo, a.number, a.match, a.body, a.path, a.resolve) + # In-process backoff, so the whole wait costs one agent turn. delays = [15, 20, 30, 45, 60, 120] start = time.monotonic() diff --git a/scripts/test_pr_review.py b/scripts/test_pr_review.py index e9758651..a33b0a8e 100644 --- a/scripts/test_pr_review.py +++ b/scripts/test_pr_review.py @@ -796,6 +796,200 @@ def test_the_digest_names_the_repository_it_read(self) -> None: self.assertIn('repo=o/r pr=7', self.out.getvalue()) +def rthread(tid: str, body: str = 'The retry count is off by one.', path: str = 'a.py', + line: int = 12, resolved: bool = False, + login: str = pr_review.REVIEWER) -> dict: + """A thread as the reply query reads it, with `path` and `line` on the thread itself.""" + return {'id': tid, 'isResolved': resolved, 'path': path, 'line': line, + 'comments': {'nodes': [{'author': {'login': login}, 'body': body}]}} + + +def page(threads: list[dict], more: bool = False, cursor: str | None = None) -> dict: + return {'nodes': threads, 'pageInfo': {'hasNextPage': more, 'endCursor': cursor}} + + +LANDED = {'id': 'c1', 'url': 'https://github.com/o/r/pull/7#discussion_r1', 'body': 'Fixed in abc.'} + + +class ReplyCase(unittest.TestCase): + """Base driving `reply` against crafted responses, so no case reaches the network.""" + + 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.docs: list[str] = [] + + def wire(self, *pages: dict, reply: dict | None = LANDED, resolved: bool = True) -> None: + """Answer the thread reads from `pages`, and each mutation from the given shape.""" + queue = list(pages) or [page([])] + + def fake(query: str, **variables: object) -> dict: + self.docs.append(query) + if 'reviewThreads' in query: + return {'repository': {'pullRequest': {'reviewThreads': queue.pop(0)}}} + if 'addPullRequestReviewThreadReply' in query: + return {'addPullRequestReviewThreadReply': {'comment': reply}} + if 'resolveReviewThread' in query: + return {'resolveReviewThread': {'thread': {'isResolved': resolved}}} + raise AssertionError(f'unexpected document: {query[:60]}') + + self.enterContext(mock.patch.object(pr_review, 'gh_graphql', side_effect=fake)) + + def run_reply(self, *extra: str) -> int: + return pr_review.main(['reply', '7', '--repo', 'o/r', '--match', 'retry count', + '--body', 'Fixed in abc.', *extra]) + + def wrote(self) -> bool: + return any('mutation' in d for d in self.docs) + + def resolved_a_thread(self) -> bool: + return any('resolveReviewThread' in d for d in self.docs) + + +class TestReplySelectsWithoutAnId(ReplyCase): + def test_the_matching_thread_is_answered_and_resolved(self) -> None: + self.wire(page([rthread('t1')])) + self.assertEqual(0, self.run_reply('--resolve')) + self.assertIn('REPLIED_AND_RESOLVED', self.out.getvalue()) + self.assertIn(LANDED['url'], self.out.getvalue()) + + def test_the_id_comes_from_the_query_rather_than_the_caller(self) -> None: + """The whole point: the id each mutation carries is one this same run just read.""" + ids = [] + + def capture(query: str, **variables: object) -> dict: + if 'reviewThreads' in query: + return {'repository': {'pullRequest': + {'reviewThreads': page([rthread('t-from-the-query')])}}} + ids.append(variables.get('threadId')) + if 'addPullRequestReviewThreadReply' in query: + return {'addPullRequestReviewThreadReply': {'comment': LANDED}} + return {'resolveReviewThread': {'thread': {'isResolved': True}}} + + with mock.patch.object(pr_review, 'gh_graphql', side_effect=capture): + self.assertEqual(0, self.run_reply('--resolve')) + self.assertEqual(['t-from-the-query', 't-from-the-query'], ids) + + def test_no_match_writes_nothing_and_lists_what_is_open(self) -> None: + """A no-match reads the same as an already-answered thread, so it stops rather than guesses.""" + self.wire(page([rthread('t1', body='An unrelated finding about naming.')])) + self.assertEqual(60, self.run_reply('--resolve')) + self.assertFalse(self.wrote()) + self.assertIn('NO_MATCH', self.out.getvalue()) + # The open threads print, or the reader's next move is to go hunting for an id. + self.assertIn('unrelated finding', self.out.getvalue()) + + def test_two_matches_refuse_rather_than_take_the_first(self) -> None: + """`head -n 1` on an ambiguous match is how a reply lands on the wrong finding.""" + self.wire(page([rthread('t1', path='a.py'), rthread('t2', path='b.py')])) + self.assertEqual(61, self.run_reply('--resolve')) + self.assertFalse(self.wrote()) + self.assertIn('AMBIGUOUS', self.out.getvalue()) + for path in ('a.py', 'b.py'): + self.assertIn(path, self.out.getvalue()) + + def test_path_narrows_an_otherwise_ambiguous_match(self) -> None: + self.wire(page([rthread('t1', path='a.py'), rthread('t2', path='b.py')])) + self.assertEqual(0, self.run_reply('--resolve', '--path', 'b.py')) + self.assertIn('b.py:12', self.out.getvalue()) + + def test_a_resolved_thread_is_not_a_candidate(self) -> None: + """It is answered, and replying again reopens a conversation nobody is reading.""" + self.wire(page([rthread('t1', resolved=True)])) + self.assertEqual(60, self.run_reply('--resolve')) + self.assertFalse(self.wrote()) + + def test_the_match_follows_the_cursor_to_the_last_page(self) -> None: + """A first page read as the whole set reports no match on a thread further along.""" + self.wire(page([rthread('t1', body='Something else.')], more=True, cursor='c1'), + page([rthread('t2')])) + self.assertEqual(0, self.run_reply('--resolve')) + self.assertIn('REPLIED_AND_RESOLVED', self.out.getvalue()) + + def test_the_match_reads_the_finding_text_rather_than_a_line_number(self) -> None: + """A fix push moves the line, and every lookup keyed to one then misses.""" + self.wire(page([rthread('t1', line=999, body='The RETRY COUNT is off by one.')])) + self.assertEqual(0, self.run_reply('--resolve')) + self.assertIn('REPLIED_AND_RESOLVED', self.out.getvalue()) + + +class TestReplyConfirmsBeforeResolving(ReplyCase): + def test_a_reply_returning_no_url_leaves_the_thread_open(self) -> None: + """Three replies posted empty and the resolves still succeeded, closing them unanswered.""" + self.wire(page([rthread('t1')]), reply={'id': 'c1', 'url': None, 'body': ''}) + self.assertEqual(62, self.run_reply('--resolve')) + self.assertFalse(self.resolved_a_thread()) + self.assertIn('REPLY_NOT_CONFIRMED', self.out.getvalue()) + + def test_a_reply_whose_body_came_back_empty_leaves_the_thread_open(self) -> None: + """A url alone says a comment exists, not that it carries the answer.""" + self.wire(page([rthread('t1')]), reply={'id': 'c1', 'url': LANDED['url'], 'body': ' '}) + self.assertEqual(62, self.run_reply('--resolve')) + self.assertFalse(self.resolved_a_thread()) + + def test_a_resolve_that_does_not_confirm_is_reported_rather_than_assumed(self) -> None: + """The reply is already posted, so silence here leaves a thread open behind an answer.""" + self.wire(page([rthread('t1')]), resolved=False) + self.assertEqual(63, self.run_reply('--resolve')) + self.assertIn('RESOLVE_NOT_CONFIRMED', self.out.getvalue()) + + def test_without_resolve_the_thread_is_answered_and_left_open(self) -> None: + """A decline is resolved once its evidence is in the thread, which is the reader's call.""" + self.wire(page([rthread('t1')])) + self.assertEqual(0, self.run_reply()) + self.assertFalse(self.resolved_a_thread()) + self.assertIn('status=REPLIED', self.out.getvalue()) + + +class TestReplyStaysInScope(ReplyCase): + def test_a_target_under_another_owner_is_refused_before_anything_is_read(self) -> None: + """The incident shape: a write that lands on a stranger's repository.""" + self.wire(page([rthread('t1')])) + code = pr_review.main(['reply', '7', '--repo', 'someone-else/r', '--match', 'retry', + '--body', 'Fixed.', '--resolve']) + self.assertEqual(64, code) + self.assertEqual([], self.docs) + self.assertIn('OUT_OF_SCOPE', self.out.getvalue()) + + def test_an_unreadable_origin_refuses_rather_than_assuming_scope(self) -> None: + """An unverified scope is not a scope, and a check that cannot run reports itself.""" + self.wire(page([rthread('t1')])) + with mock.patch.object(pr_review, 'origin_owner', return_value=None): + self.assertEqual(64, self.run_reply('--resolve')) + self.assertEqual([], self.docs) + + def test_a_sibling_repository_under_the_same_owner_is_in_scope(self) -> None: + """The fleet is one owner, and that is the case the maintainer works in daily.""" + self.wire(page([rthread('t1')])) + code = pr_review.main(['reply', '7', '--repo', 'o/some-other-repo', '--match', + 'retry count', '--body', 'Fixed in abc.', '--resolve']) + self.assertEqual(0, code) + + +class TestReplyArguments(unittest.TestCase): + def err(self, argv: list[str]) -> str: + with contextlib.redirect_stderr(io.StringIO()) as err: + with self.assertRaises(SystemExit): + pr_review.main(argv) + return err.getvalue() + + def test_an_empty_body_is_rejected_rather_than_posted(self) -> None: + """A thread resolved on an empty answer reads as addressed while carrying nothing.""" + for body in ('', ' '): + with self.subTest(body=body): + self.assertIn('--body', self.err( + ['reply', '7', '--repo', 'o/r', '--match', 'x', '--body', body])) + + 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_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']): + with self.subTest(flag=flag[0]): + self.assertIn(flag[0], self.err(['status', '7', '--repo', 'o/r', *flag])) + + class TestContract(unittest.TestCase): def test_the_reviewer_login_matches_the_runbook_graphql_form(self) -> None: """GraphQL drops the `[bot]` suffix REST carries, and this script is GraphQL-only. @@ -818,14 +1012,54 @@ def test_the_refusal_pattern_is_the_runbook_alternation(self) -> None: # The published filter is single-quoted, which no spelling of the apostrophe survives. self.assertNotIn("'", pr_review.REFUSAL.pattern) - def test_no_mutation_reaches_this_script(self) -> None: - """Mutations stay as explicit `gh` calls so the write-guard hook and review still see them.""" + def test_the_only_writes_are_the_two_the_reply_path_owns(self) -> None: + """One writing path, and everything else that changes state stays out of this script. + + 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 two documents `reply` + needs rather than dropped when the first of them arrived. + """ source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8') - # The GraphQL keyword is matched with its opening token, so naming the runbook is not a hit. - for verb in ('mutation(', 'mutation{', 'mutation {', '-X POST', '-X PATCH', '-X PUT', - '-X DELETE', 'gh pr merge', 'gh pr review'): + for verb in ('-X POST', '-X PATCH', '-X PUT', '-X DELETE', '--method', + 'gh pr merge', 'gh pr review', 'gh pr edit', 'requestReviews'): with self.subTest(verb=verb): - self.assertFalse(verb in source, f'{verb!r} is a state-changing call in a read-only script') + self.assertNotIn(verb, source, f'{verb!r} is a state-changing call this script ' + 'has no reason to make') + # `mutation(` opens a document, so the count is the number of documents. + # A third arriving is a write nobody reviewed as one rather than a style drift. + self.assertEqual(2, source.count('mutation(')) + self.assertIn('addPullRequestReviewThreadReply', source) + self.assertIn('resolveReviewThread', source) + + def test_the_mutations_are_the_ones_the_runbook_publishes(self) -> None: + """A helper performing a different write than the documented one is undocumented.""" + text = RUNBOOK.read_text(encoding='utf-8') + for name in ('addPullRequestReviewThreadReply', 'resolveReviewThread'): + with self.subTest(mutation=name): + self.assertIn(name, 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. + + A parser that takes an id restores the failing shape however plainly the docs discourage + it, which is the lesson the two prior instances taught: the rule was known and read. + """ + source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8') + # A node id literal anywhere in the source is an example a hand copies out of it. + self.assertNotIn('PRRT_', source) + parser_options = re.findall(r"add_argument\('(--[a-z-]+)'", source) + for opt in parser_options: + with self.subTest(option=opt): + self.assertNotIn('id', opt.replace('--', '').split('-')) + # The selector is the finding's text, and it is required rather than defaulted. + self.assertIn("'--match'", source) + + def test_no_write_suppresses_or_forces_its_own_result(self) -> None: + """A mutation whose output is discarded is a write nobody can say landed.""" + source = (REPO / 'scripts' / 'pr_review.py').read_text(encoding='utf-8') + for tail in ('>/dev/null', '2>/dev/null', '&>/dev/null', '|| true', '|| :', 'shell=True'): + with self.subTest(tail=tail): + self.assertNotIn(tail, source) def test_the_guard_tests_the_window_the_queries_actually_read(self) -> None: """A guard measuring one number while the query fetches another reads clean on drift."""