best-practices: add guidance on AI-generated commits and AI-assisted PR workflows - #61
paddybyers wants to merge 2 commits into
Conversation
|
|
||
| Code generated by coding agents is now the default way that much of our code is produced. Coding agents can raise PRs, so long as they follow the guidance in this document and in [commits.md](commits.md). | ||
|
|
||
| Raising a PR in non-draft state is a signal that the author is asking for independent human review; this is just as true for AI-generated PRs as for any other. Therefore, if the author wishes to have an AI-assisted PR review — such as by having Copilot reviews configured in GitHub — this should be done with the PR in draft state. Once the author is satisfied that the PR is ready for independent human review, it should be updated to a full (non-draft) PR. |
There was a problem hiding this comment.
I think we should also be explicit in that ALL bot comments should be addressed before the PR is marked as ready for review.
There was a problem hiding this comment.
Agreed, even if it's just to dismiss them.
There was a problem hiding this comment.
There is an exception though.. if you've received reviews, made changes, and then Copilot re-reviews. I don't think it makes sense to put a PR back into draft while you make changes, unless the changes are significant.
|
|
||
| Be aware that coding agents can generate large volumes of changes quickly, which can easily result in oversized or unfocused commits. If a coding agent has produced a large body of work, it is still important to ensure that the work is committed in logical, independently-reviewable chunks rather than as a single monolithic commit. Use interactive rebase or other tools to reshape the history if needed before requesting review. | ||
|
|
||
| If a coding agent generated the content of a commit — whether or not it performed the commit itself — it is preferable that this is indicated in the commit message or the associated PR description. This helps reviewers understand the provenance of the changes and adjust their review approach accordingly. |
There was a problem hiding this comment.
This seems unnecessary... you outline "Code generated by coding agents is now the default way that much of our code is produced." in the section below.
I don't think how the PR gets made is important. Heavily simplified list, but we care about:
- Is the intent clear?
- Are the changes reviewable?
- Is the author confident in the changes?
The last point could apply because the entire PR was created with an LLM, or it could apply because someone is working with a new language or system for the first time. For the former, the guidance in this PR explicitly states how they still need to review it themselves. For the latter, the author should know to tag appropriate reviewers and be explicit about where they may need extra attention.
I don't think reviewers should change their strategy on how they review a PR as the author is taking full responsibility by marking it ready for review.
There was a problem hiding this comment.
I agree with @umair-ably here. However, I think indicating that an LLM assisted in the content is a nice to have and we'd recommend that. Whilst technically it's not needed, for the reasons stated, it does help others provide feedback in a way that may enable the reviewer to understand why things are in the state they are, and provide quicker feedback to course correct. So is this required, no, is it recommended, I would say yes.
There was a problem hiding this comment.
My general assumption now is that an LLM has always assisted or produced the code. Even if a commit doesn't specifically state so (or even if the PR doesn't mention LLM usage), I think we should all be reviewing with that assumption in mind now. The only case where I think this makes a big difference, is if a PR was created entirely by Claude via some automated workflow, akin to dependabot - but this means that no human was in the loop at all, so I would adjust my review approach.
|
|
||
| Be aware that coding agents can generate large volumes of changes quickly, which can easily result in oversized or unfocused commits. If a coding agent has produced a large body of work, it is still important to ensure that the work is committed in logical, independently-reviewable chunks rather than as a single monolithic commit. Use interactive rebase or other tools to reshape the history if needed before requesting review. | ||
|
|
||
| If a coding agent generated the content of a commit — whether or not it performed the commit itself — it is preferable that this is indicated in the commit message or the associated PR description. This helps reviewers understand the provenance of the changes and adjust their review approach accordingly. |
There was a problem hiding this comment.
I agree with @umair-ably here. However, I think indicating that an LLM assisted in the content is a nice to have and we'd recommend that. Whilst technically it's not needed, for the reasons stated, it does help others provide feedback in a way that may enable the reviewer to understand why things are in the state they are, and provide quicker feedback to course correct. So is this required, no, is it recommended, I would say yes.
|
|
||
| Raising a PR in non-draft state is a signal that the author is asking for independent human review; this is just as true for AI-generated PRs as for any other. Therefore, if the author wishes to have an AI-assisted PR review — such as by having Copilot reviews configured in GitHub — this should be done with the PR in draft state. Once the author is satisfied that the PR is ready for independent human review, it should be updated to a full (non-draft) PR. | ||
|
|
||
| By default, AI review comments and the resulting discussion and resolution should remain in the PR for future reference, unless the author believes that they constitute extraneous noise. In that case, consider re-raising a clean PR without the noise. It is the author's responsibility to ensure that the PR is in a fit state to review, and that extends to the PR discussion as well as to the code and history. |
There was a problem hiding this comment.
I don't agree with this. Surely this should be the exception as opposed to the norm. Why would humans need to know (by default) what iterations a human has gone through with an AI reviewer? My expectation is that largely this is noise, and if relevant, I'd prefer the user links to the draft PR. Promoting draft PRs by default will bring a lot of noise. I would advocate we discourage this.
There was a problem hiding this comment.
Personally, I think the comments are useful - though for the abatement of noise I would suggest that Claude posts a single "summary" comment with findings, rather than lots of inline comments - for example how we do it on the website, whereby the bot review is one comment with collapsible sections and a machine summary so as not to overwhelm the feed.
They're useful as a reviewer because I (and also other bots) can see what's been raised before and the justifications for potentially not fixing it, it's useful context.
Further - if we're moving towards a model by which auto-approval of PRs by bots becomes common, then these comments form evidence as to why (or not) a PR was auto approved.
I think it's fine for us to say that we should strive to keep bot noise to a minimum wherever possible, and that mixing bot noise with human noise is also undesirable. But I think then PR is the perfect place to see "what changed since the author thought the work was ready to land".
There was a problem hiding this comment.
I've left this as-is; I will do a further update with the outcome of https://ably.atlassian.net/wiki/spaces/ENG/pages/5307367443/ERFC-001+Separating+humans+from+bots+in+our+pull+requests, which will address the question of comment noise, and ensuring that AI feedback uses the appropriate feature/surface.
|
|
||
| Code generated by coding agents is now the default way that much of our code is produced. Coding agents can raise PRs, so long as they follow the guidance in this document and in [commits.md](commits.md). | ||
|
|
||
| Raising a PR in non-draft state is a signal that the author is asking for independent human review; this is just as true for AI-generated PRs as for any other. Therefore, if the author wishes to have an AI-assisted PR review — such as by having Copilot reviews configured in GitHub — this should be done with the PR in draft state. Once the author is satisfied that the PR is ready for independent human review, it should be updated to a full (non-draft) PR. |
There was a problem hiding this comment.
I would argue that this doesn't hold as well when you intend to have bots actually approving a PR - you want those bots to act on actual "ready for review" PRs.
FWIW the reason we put the code review bot in the "ready for review" stage on ably/website is because over there people put up draft PRs all the time for Heroku review apps and the bot would just become even more spammy (which is a legitimate use-case, and equally I don't think its great to have review bots as opt-in).
There was a problem hiding this comment.
I don't think its great to have review bots as opt-in
Why not? If you're publishing as draft in order to get a review app, but don't want to trigger bot review, what's the alternative?
I think an author can choose to get bot review either locally or in the GH workflow, so I don't see why the GH workflow should be unconditional.
There was a problem hiding this comment.
so I don't see why the GH workflow should be unconditional.
I think the case where it would be is if you've got an automated approval mechanism going on, if that's happening, then having the review step on CI as an artefact that informs the approval is necessary
There was a problem hiding this comment.
I've clarified that the independent review state may have AI as well as human review: see 46e13b2
|
This has been stagnant for 2-3 months now @paddybyers |
…PR workflows Coding agents are now a significant part of how we produce code. These additions establish clear expectations for their use within our existing commit and PR standards. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
72ed073 to
46e13b2
Compare
|
I have updated; PTAL. |
|
Thanks, looks good. How is this going to be used @paddybyers? In addition to communicating with the team, Is this not also a shared context/skill/plugin that runs automatically to help people when creating PRs, effectively acting as your assistant to help people implement this? What is your thinking on how we use this guidance vs write it it down? |
| @@ -145,6 +145,14 @@ In body: | |||
| Fixes #123, #124 | |||
There was a problem hiding this comment.
merging a PR with a commit message containing either
Fixes #123, #124
automatically closes the corresponding GitHub issues or pull requests after merging right?
This has recently resulted in related PRs being unintentionally closed across multiple repositories e.g ably/ably-pubsub-js#2288, ably/ably-pubsub-js#2284. So, I had to explicitly re-open and merge the same.
Maybe explicit guidance regarding the same would be useful.
|
|
||
| Repositories should contain appropriate configuration (for example, in `CLAUDE.md` or agent skills) to help ensure that coding agents comply with these standards. In practice, coding agents generally follow commit policy guidance well when it is provided. | ||
|
|
||
| Be aware that coding agents can generate large volumes of changes quickly, which can easily result in oversized or unfocused commits. If a coding agent has produced a large body of work, it is still important to ensure that the work is committed in logical, independently-reviewable chunks rather than as a single monolithic commit. Use interactive rebase or other tools to reshape the history if needed before requesting review. |
There was a problem hiding this comment.
We can probably mention stacked PRs as another method of splitting where appropriate, but up to you
|
|
||
| Raising a PR in non-draft state is an indication by the author that it is ready for independent review; this is just as true for AI-generated PRs as for any other. Therefore, if the author wishes to trigger an AI-assisted PR review as part of their own pre-submission process, this should be done with the PR in draft state. Once the author is satisfied that the PR is ready for independent review - which requires any prior review feedback to be resolved - it should be updated to a full (non-draft) PR. | ||
|
|
||
| By default, AI review comments and the resulting discussion and resolution should remain in the PR for future reference, unless the author believes that they constitute extraneous noise. In that case, consider re-raising a clean PR without the noise. It is the author's responsibility to ensure that the PR is in a fit state to review, and that extends to the PR discussion as well as to the code and history. |
There was a problem hiding this comment.
Non-blocking, could also mention that you can "hide" comments on GitHub which leaves them in the PR stream but collapsed, so you can see where the discussion happened but not have to scroll through reems of comments
This branch has not been deployed
6 participants
Summary
commits.md, establishing that AI-produced commits must meet the same standards for coherence, atomicity, and message quality, and that AI provenance should be indicatedpull-requests.md, clarifying expectations for AI-raised PRs, AI-assisted reviews (draft state), and author responsibility to personally review AI-generated code before requesting peer reviewTest plan
🤖 Generated with Claude Code