Review run: review another author's pull request and address its findings #706

Problem Statement

A colleague who is a Trusted account on a repository, but not an engineer by role, opens pull requests and asks the Day shift to review them. Before reading each one, the Day shift does the same thing by hand: it makes a worktree, starts a session, and runs the two-axis code review on the PR with "address the findings". It accepts nearly every finding, and always wants every Standards finding addressed. thirdshift can't do this today. Its review runs only inside its own Runs, on its own branches, against an Issue URL. It has no Command for a pull request someone else wrote, and no way to watch one author's pull requests.

Solution

A Review run: a Built-in process that starts on another author's open pull request. It reviews the change on the Standards and Spec axes, addresses the findings on the pull request's own branch, and comments with what it fixed and every Unaddressed finding. It is started by hand with thirdshift review <PR URL>, or as a Pass with thirdshift review --author <login>, which cron can run to review one waiting pull request by that author per invocation. With --recommend-only, it posts the review and pushes nothing. The Day shift hears about it in a Run notification, and the Inbox keeps it under Ready for review until the pull request is merged or closed. The first thing the Day shift reads is then a pull request that has already been through the review it would have run by hand.

User Stories

1. As the Day shift, I want to run thirdshift review <PR URL>, so that a colleague's pull request gets the review and fixes I'd otherwise make by hand.
2. As the Day shift, I want thirdshift review --author <login> to review one waiting pull request by that author per invocation, so that cron can review a colleague's pull requests before I read them.
3. As the Day shift, I want the Pass form to require --author, so that a cron line can never review every pull request in a repository.
4. As the Day shift, I want --recommend-only to post the review as a comment and push nothing, so that I can build trust in it before it changes anyone's branch.
5. As the Day shift, I want a Review run to refuse a pull request from a fork, a draft, or a closed or merged pull request, saying why, so that it only touches work that is in the repository and ready for review.
6. As the Day shift, I want a Review run to refuse a pull request whose author is not a Trusted account, so that it never runs on code and text from an account the repository doesn't trust.
7. As the Day shift, I want a Review run to read only the pull request's body, its linked issues, and comments from Trusted accounts, so that a comment from anyone else can't steer the session.
8. As the Day shift, I want the Spec axis to review against the issues the pull request links as closing, so that the issue is the spec, as it is for a Ticket run.
9. As the Day shift, I want the Spec axis to review against the pull request's body when it links no issue, so that a pull request without an issue still gets a Spec review.
10. As the Day shift, I want the comment to say when the Spec axis used the body alone, so that the author is reminded that a linked issue makes a better spec.
11. As the Day shift, I want a Review run to fix every Standards and Spec finding it can't refute, as a Ticket run does, so that the fixes I'd ask for are already made.
12. As a pull request's author, I want a departure from the linked issue that my body explains to be listed for the Day shift to decide, not reverted, so that a deliberate change of mine isn't undone.
13. As the Day shift, I want those departures listed as Unaddressed findings, citing the body's reason, with no needs-triage issue filed, so that I decide them in the review I'm about to do.
14. As the Day shift, I want the fixes pushed to the pull request's own branch, never force-pushed, so that the author's pull request stays the one thing I review and their commits are never rewritten.
15. As the Day shift, I want one comment listing what was fixed and every Unaddressed finding with what refutes it, so that I can see at a glance what the review did.
16. As the Day shift, I want a Review run that finds the branch has moved during its work to push nothing, so that its fixes are never applied on top of commits it didn't review.
17. As the Day shift, I want that Review run to post its findings, with the fixes it made described, and say the branch moved, so that the work isn't lost.
18. As the Day shift, I want each head the author pushes reviewed once, so that a colleague who pushes changes after feedback gets a fresh review without anyone touching labels.
19. As the Day shift, I want the Pass to skip a pull request whose head is the one a Review run reviewed, or the one it pushed, so that thirdshift never reviews its own fixes.
20. As the Day shift, I want that record kept in the Review run's comment, so that every machine running the Pass sees it.
21. As the Day shift, I want a Review run to reserve its pull request with the in-progress label while it works, so that two machines never review the same pull request at once.
22. As the Day shift, I want the Sweep to clear in-progress from closed pull requests, as it does from closed issues, so that a pull request merged during a review isn't left labelled.
23. As the Day shift, I want each pull request in a stack reviewed against its own base, bottom first, so that each review sees only that pull request's change.
24. As the Day shift, I want the Pass to skip a pull request whose base is another open pull request until that one has been reviewed at its current head, so that a stack is reviewed in order.
25. As a pull request's author, I want a Review run never to merge or rebase another pull request in my stack, so that the shape of my work is left to me.
26. As the Day shift, I want an upper pull request's comment to say its base got fixes and it needs updating before merge, so that the stack isn't merged out of date.
27. As the Day shift, I want a per-repository list of extra test commands in the User config that every Review run runs and reports, so that the suites an author couldn't run from a worktree are run for them.
28. As the Day shift, I want the comment to say what ran, what passed, and what couldn't run, so that I know what the review checked.
29. As the Day shift, I want a suite the repository's instructions say isn't valid from a worktree reported "not run against the branch", never green, so that a pass that tested the main checkout is never mistaken for one that tested the pull request.
30. As the Day shift, I want the Security review flag and User config setting to ask for a Security review on a Review run, after it addresses its findings, so that a colleague's change gets the same security check as an agent's.
31. As the Day shift, I want a Run notification when a Review run ends, whatever its outcome, so that I hear about it where I hear about every other Command.
32. As the Day shift, I want a Review run under Ready for review in the Inbox until the pull request is merged or closed, so that a reviewed but unmerged pull request stays in front of me.
33. As the Day shift, I want a Review run to count under the Command limit like any other Command, and its Pass skipped for it like any other Pass, so that cron can't pile reviews onto a busy machine.
34. As the Day shift, I want a Custom process to be able to start on a pull request as well, so that pull-request work other than review can be built from the same Steps.
35. As the Day shift, I want the Review run's Process file readable and copyable like any Built-in process's, so that I can start a Custom process from it.

Implementation Decisions

• Settled in the grilling in docs/grill-with-docs/done/706-1323-review-and-triage.md (Q1.3 to Q1.10, Q2.2 to Q2.7, Q3.3 to Q3.5); question numbers are cited below.
• Processes start on a pull request. A Process's input grows from "an optional issue" to "an optional issue or pull request", with a pull-request worktree kind checked out on the pull request's head branch. The Built-in process checks and the Custom-process checks learn which Steps need which input (Q2.3).
• A review-and-address catalogue Step. It reviews the worktree's change against the pull request's base with the embedded code-review skill, then addresses findings with the same rules and Unaddressed-finding format a Ticket run's implement session uses, sharing that prompt text. The Ticket run keeps reviewing inside its single implement session; it is not split into two sessions.
• A delivery Step for another author's branch. It pushes without force to the pull request's head branch and posts one comment. A rejected push means the branch moved: nothing is pushed, and the comment carries the findings and a description of the fixes (Q3.4). Under --recommend-only, the Step posts the comment and never pushes (Q2.2).
• The comment carries a hidden marker naming the head the Review run reviewed and, when it pushed, the head after its push. The Pass reads it from the pull request's comments by the account thirdshift acts as (Q2.4).
• Trust and eligibility reuse the Trusted-account rule from issues, applied to the pull request's author, plus checks that the pull request is open, not a draft, and not cross-repository (Q1.7). Only Trusted accounts' comments reach the session.
• Spec input: linked closing issues, read as for a Ticket run, plus the pull request body. With an issue linked, the issue is the spec (Q1.5, Q2.6).
• The Pass lists the author's open pull requests and takes one, lowest-numbered first. It skips reviewed heads, in-progress, and pull requests whose base is another open, unreviewed pull request. It reserves with the in-progress label the way a Claim does (Q1.6, Q2.5, Q3.3). The Sweep covers closed pull requests.
• The User config gains a per-repository list of extra review commands (Q1.9). The Security review setting and flag apply as for a Ticket run (Q2.7).
• A new Command kind, "Review run", in the Command record, Activity log, Run notification, status and the Inbox's Ready for review group (Q1.10).
• The glossary already defines Review run.

Testing Decisions

• Test external behaviour at the highest seam: the real binary run in a test Scenario against the fake gh and a fake Harness session whose script does the agent's work. That is how the Pickup run and Architect run tests work today. Assert on what GitHub and git see: commits on the origin branch, comments, labels, refusals and their messages. Also assert on the Run notification and the Command record. Don't assert on internal calls.
• The fake gh gains pull-request authors and author associations, drafts, pr list --author, pr comment, and labels on pull requests.
• Assert the compiled Built-in process as the existing compiled-process tests do for the Architect run.
• Prior art: the Pickup run tests (a Pass, its selection, Claim and Sweep), the Architect run and Architecture review process tests (a Built-in process), and the Security run tests.

Out of Scope

• Pull requests from forks, and from authors who aren't Trusted accounts.
• Merging, rebasing or force-pushing anyone's branch.
• Reviewing every author's pull requests in one Pass.
• A Retro over repeated findings (thirdshift retro: propose coding standards from repeated findings #1698).
• Making worktree-invalid suites valid from a worktree; that is the target repository's own work.

Further Notes

Part of #1691. A sibling of #1323 (Triage run), which shares the --recommend-only flag's name and meaning.