Draft PR if copilot has findings#369
Conversation
There was a problem hiding this comment.
Pull request overview
Extends the GitHub webhook ReviewDrafter so Copilot review findings can influence PR draft/ready-for-review flow, alongside the existing human-review behavior.
Changes:
- Add Copilot-specific review detection and unanswered-finding tracking in
ReviewDrafter. - Add Copilot tracker/outdated issue comment handling when reviews are submitted or PRs are marked ready.
- Add a new test suite covering Copilot review submission and ready-for-review scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
services/bots/src/github-webhook/handlers/review_drafter.ts |
Adds Copilot finding detection, tracker comment management, and draft/ready-for-review behavior. |
tests/services/bots/github-webhook/handlers/review_drafter.spec.ts |
Adds unit tests for Copilot review handling and tracker comment flows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I think drafting a PR whenever Copilot has findings is different from forcing everyone to respond to every comment. As long as Copilot leaves the same comments over and over again and authors are required to respond to every one of them, I think this will only lead to more frustration rather than a better first PR. 
|
|
@joostlek So what would you instead propose? Many authors ignore comments totally because it's AI but most of the time they are valuable and AI is improving |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
justanotherariel
left a comment
justanotherariel
left a comment
There was a problem hiding this comment.
So I think we need unanimous agreement tbh - I would discuss this on tuesday. Also, this webhook will fire for all home assistant repositories, as well as esphome. We might want to first try this out in home-assistant/core only for now - unless we know that other repos want this too.
| if (this.isCopilotReview(context)) { | ||
| await this.handleCopilotReviewSubmitted(context); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think this should be in here, but rather be branched of in handle(). Otherwise this gets a bit confusing.
To expand on this: This webhook does now four things:
- 'Changes requested' by member -> mark PR as draft
- PR is marked as ready -> request review from everyone who has requested changes
- Copilot reviews and has in-line comments ->write a comment listing how many inline comments are present
- Copilot in-line comment has been addressed by PR author -> update the comment (adjust the unaddressed reviews number or mark the comment as outdated)
There was a problem hiding this comment.
Looking at this more, I think this shouldn't be in this handler at all. Instead it should be a seperate handler IMO
| private async markCopilotCommentOutdated( | ||
| context: WebhookContext<PullRequestReviewSubmittedEvent | PullRequestReadyForReviewEvent>, | ||
| issueComments: Array<{ id: number; body?: string | null }>, | ||
| ): Promise<void> { | ||
| const activeComment = issueComments.find((comment) => | ||
| comment.body?.startsWith(COPILOT_MESSAGE_ID), | ||
| ); | ||
| if (!activeComment) { | ||
| return; | ||
| } | ||
|
|
||
| // Drop the active marker so subsequent lookups don't rediscover this comment, | ||
| // and prepend an outdated banner. | ||
| const remainingBody = (activeComment.body ?? '').replace(COPILOT_MESSAGE_ID, '').trimStart(); | ||
| const outdatedBody = `${COPILOT_OUTDATED_NOTICE}${remainingBody}`; | ||
|
|
||
| await context.github.issues.updateComment( | ||
| context.repo({ comment_id: activeComment.id, body: outdatedBody }), |
There was a problem hiding this comment.
I think we should just remove the comment after everything is done tbh
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
I think we should just make the PR draft and a comment stating "Make sure to resolve all copilot comments and reply to those you don't agree with". |
No description provided.