Auto approve Forward Flow PRs after a check#6469
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for automatically approving forward-flow codeflow PRs after a “codeflow approval check” validates that the PR only contains expected source updates. It does so by introducing a dedicated GitHub App-based approver service and wiring it into the codeflow approval-check workflow and subscription creation options.
Changes:
- Add
IPullRequestApprover+GitHubPullRequestApproverto approve PRs via a dedicated GitHub App, and invoke it when the codeflow diff check passes. - Gate scheduling/execution of
CodeflowApprovalCheckonsubscription.AutoApprove. - Extend scenario coverage to create auto-approve subscriptions and wait for PR approval; update tests/DI for the new dependency.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ProductConstructionService/ProductConstructionService.ScenarioTests/ScenarioTests/ScenarioTests_CodeFlow.cs | Enables autoApprove for the forward-flow scenario and waits for the PR to become approved. |
| test/ProductConstructionService/ProductConstructionService.ScenarioTests/ScenarioTestBase.cs | Makes WAIT_DELAY accessible to derived scenario base classes. |
| test/ProductConstructionService/ProductConstructionService.ScenarioTests/CodeFlowScenarioTestBase.cs | Adds --auto-approve subscription option plumbing and polling helper for PR approval; adds optional PR cleanup behavior. |
| test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs | Registers a test DI mock for the new IPullRequestApprover dependency. |
| src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs | Prevents approval check processing for non-auto-approve subscriptions (currently via exception). |
| src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs | Schedules approval-check work only when AutoApprove is enabled and approves PRs when the diff check passes. |
| src/ProductConstructionService/ProductConstructionService.DependencyFlow/GitHubPullRequestApprover.cs | New service that approves PRs using a dedicated GitHub App installation token. |
| src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs | Adds DI registration/configuration hook for the codeflow approval GitHub App credentials + approver service. |
| src/ProductConstructionService/ProductConstructionService.Api/PcsStartup.cs | Wires approval GitHub App credentials from configuration and registers the approver in PCS startup. |
| src/Maestro/Maestro.Common/GitRepoUrlUtils.cs | Treats api.github.com URLs as GitHub for repo type parsing (needed for API-form PR URLs). |
|
|
||
| if (!subscription.AutoApprove) | ||
| { | ||
| throw new InvalidOperationException($"Subscription {subscription.Id} is not auto-approve enabled, cannot run codeflow approval check"); |
There was a problem hiding this comment.
This and the above exception can be NonRetriableException
There was a problem hiding this comment.
right, forgot about those
| /// Approves GitHub codeflow pull requests using a dedicated GitHub App that has permission | ||
| /// to approve pull requests. | ||
| /// </summary> | ||
| internal class GitHubPullRequestApprover : IPullRequestApprover |
There was a problem hiding this comment.
I have doubts about whether this requires its own class. Semantically it's just "give me a PR url and i'll do an action on it", which is what Remote and GithubClient are. It's only 1 method and it doesn't look like it has much potential to be expanded upon as a class, or even to be used in places where Remote / GithubClient are not already wired in. My vote would be to just add this 1 method into GithubClient, and add the approver identity as a separate client creation method to the one that GithubClient already has.
There was a problem hiding this comment.
I intentionally made them separate. The approver requires "approver" options to be configured, with it's own separate GH app.
having them on the same class is confusing IMO, as then you'd have to have two GH clients, one to create the PR, and one to approve it.
There was a problem hiding this comment.
I like the idea of preventing mixups between the two clients, but I still think it could be fine having them in the same class. Luckily the client is not wired by DI, but is gotten through GetClient() in every method - I think it would be totally fine to have something like GetCommonClient(), and GetPrApproverClient() which only the new method would use.
I'll leave the final call to you
| var repoType = GitRepoUrlUtils.ParseTypeFromUri(pullRequestUrl); | ||
| if (repoType != GitRepoType.GitHub) | ||
| { | ||
| _logger.LogInformation( |
There was a problem hiding this comment.
What about throwing a NotImplementedException here rather than failing silently?
There was a problem hiding this comment.
throwing this would retry the workitem, so I guess we either throw the non continuable exception, or just fail silently
There was a problem hiding this comment.
Agreed, NonRetriableException would fit here
Also, maybe we can extend NonRetriableException with something like GeneralNonRetriableException, or NonRetriableInvalidOperationException, just to make it more clear what we are throwing exactly, and keep NonRetriableException just as a base class (maybe we can make it an abstract class)
| var (owner, repo, prNumber) = DarcGitHubClient.ParsePullRequestUri(pullRequestUrl); | ||
| if (owner == null || repo == null || prNumber == 0) | ||
| { | ||
| _logger.LogInformation( |
There was a problem hiding this comment.
Same - shouldn't we throw in this case?
No description provided.