Pull Request (PR) review protocol - feedback requested

As a general rule, I don't specifically request reviews of my PRs unless I think they are particularly suited to a developer's interests or skills, but this practice depends on developers periodically checking the list of open PRs and helping to whittle down the backlog. This allows everyone manage their own time as they see fit, but also means that there's never an explicit prompt to view the backlog.

Would folks prefer that I explicitly ping them when a review is needed?

Another possibility for managing the PR backlog would be to periodically message folks when the backlog exceeds a certain threshold in number or staleness, like Martin has recently done (but with some automation).

For the immediate term, I'll devote time to reviewing/approving/closing PRs which aren't authored by me, but if someone could help out reviewing mine, that would be great.

For Copilot (or other AI generated) PRs, I'd suggest that the developer who did the prompting have the first responsibility for review and that others not be asked to review until they say it's ready. Note that, as we currently have things configured, an approval by the developer who prompted the agent does NOT count towards the approvals needed for merger. We could change this, but I think it's a reasonable policy for now.

Related threads:

1 Like

@tfmorris for confirmation, I understand that

  1. The contributor opens the PR
  2. A committer tags Copilot for a PR review. This is the equivalent of prompting
  3. The committer reviews Copilot's comment. At this point, the initial contributor does not need to take action
  4. Once the committer has reviewed the comment, the initial contributor can update the PR if needed.

Taking #7853 as an example, this means that arimu1 should wait for you to review the Copilot comment (since your prompt requests the review).

Actually, for #7853, I think the committer should address the Copilot review comments before human review. "Addressing" may be saying "this review comment is an AI hallucination" or invalid for some other reason. Eventually, I think we'll try to have the assigning of Copilot for reviews be done automatically, so the "prompting" will really just be our standard PR review guidelines. The goal here is to have all the initial automated feedback be addressed before involving a human reviewer.

What I was talking about in my original note was when a developer requests Copilot to generate a PR where the workflows goes something like:

  • Dev -- Copilot please fix issue #1234 paying special attention to backward compatibility
  • Copilot - generate work plan, possibly waiting for it to be approve
  • Copilot - generate code, perhaps in multiple rounds
  • Copilot - request Dev to review draft PR
  • Dev - optionally request Copilot modify the PR
  • Copilot - modify PR to comply with instructions & re-request review
  • Dev - mark PR as non-draft and approve (note, this approval does not qualify the PR to be ready to merge under Github's/our current rules)

All of the above is visible on Github (if Copilot is used rather than another coding agent), but can be ignored until the entire process is complete and the original dev has approved it.

Hopefully that clarifies the proposal rather than making it even more confusing!

1 Like

Thanks for the clarification.