Comment on FF PRs that look good to merge by dkurepa · Pull Request #6435 · dotnet/arcade-services

dkurepa · 2026-06-23T14:55:59Z

The check has two layers.
First we do a --name-only diff, where we check if the PR changed any files the source repo didn't. If it did, we don't give a go ahead.
If the there are no PR only changed files, we do the second phase where we diffs the source repo diff and the PR diff with -U0 and then compare them line by line if they match.
Last, if there's any files the source repo changed that don't appear in the VMR, we check if the VMR is already in the desired target state. If it is, we say it's good

…val1

Copilot

Pull request overview

This PR adds an automated “looks good to merge” signal for forward-flow codeflow PRs by verifying that the VMR PR branch faithfully matches the source repo diff, then posting a PR comment when it does.

Changes:

Introduces a ICodeflowSourceDiffVerifier implementation in DarcLib to compare source vs. VMR PR diffs for forward-flows.
Adds a new CodeflowApprovalCheck work item + processor in PCS DependencyFlow, scheduled after FF PR creation/update.
Extends/updates test coverage across PCS DependencyFlow tests and DarcLib codeflow tests to validate the new verification behavior.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
test/ProductConstructionService/ProductConstructionService.ScenarioTests.Tests/TestHelpersTest.cs	Adds missing helper namespace import for scenario test helper usage.
test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/UpdateAssetsForCodeFlowTests.cs	Updates expectations to include the new codeflow approval-check reminder.
test/ProductConstructionService/ProductConstructionService.DependencyFlow.Tests/PullRequestUpdaterTests.cs	Wires a mock `ICodeflowSourceDiffVerifier` into the DI container and adds a helper for asserting the new reminder.
test/Darc/Microsoft.DotNet.DarcLib.Tests/VirtualMonoRepo/CodeflowChangeAnalyzerTests.cs	Adds an import needed by updated VMR/codeflow-related test types.
test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/ForwardFlowTests.cs	Adds source-SHA capture + verification calls for forward-flow diffs.
test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowUpdatingPRsTests.cs	Validates source-diff verification behavior across PR-branch modifications and subsequent flows.
test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeFlowTestsBase.cs	Adds a helper to run the same source-diff verification logic used by PCS.
test/Darc/Microsoft.DotNet.DarcLib.Codeflow.Tests/CodeflowSourceDiffVerifierTests.cs	New unit tests for the source-diff verifier (mocked git/VMR interactions).
src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItems/CodeflowApprovalCheck.cs	New work item carrying the SHA range to verify for “looks good” commenting.
src/ProductConstructionService/ProductConstructionService.DependencyFlow/WorkItemProcessors/CodeflowApprovalCheckProcessor.cs	New processor that locates the subscription/updater and runs the approval check.
src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestUpdaters/CodeFlowPullRequestUpdater.cs	Schedules the new approval check reminder and implements the approval-check execution/commenting.
src/ProductConstructionService/ProductConstructionService.DependencyFlow/PullRequestStateManager.cs	Adds a Redis reminder manager to enqueue `CodeflowApprovalCheck` items.
src/ProductConstructionService/ProductConstructionService.DependencyFlow/IPullRequestStateManager.cs	Exposes a new API for scheduling the codeflow approval check.
src/ProductConstructionService/ProductConstructionService.DependencyFlow/DependencyFlowConfiguration.cs	Registers the new work item processor with DI.
src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowSourceDiffVerifier.cs	New verifier implementation that compares file sets and zero-context diffs.
src/Microsoft.DotNet.Darc/DarcLib/VirtualMonoRepo/CodeflowExtensions.cs	Registers `ICodeflowSourceDiffVerifier` in the codeflow DI setup.
arcade-services.slnx	Adds the ScenarioTests.Tests project to the solution.

+        if (matches)
+        {
+            await remote.CommentPullRequestAsync(pr.Url, "Looks good");
+        }    


dkurepa · 2026-06-25T13:07:57Z

            inProgressPr.LastUpdate = DateTime.UtcNow;
            await _stateManager.SetCheckReminderAsync(inProgressPr, pr, isCodeFlow: true);
            await _stateManager.UnsetUpdateReminderAsync(isCodeFlow: true);
+            if (!skipCodeflowApprovalCheck && !string.IsNullOrEmpty(previousSourceSha))


we should also check if this was a FF, will add

premun

Did you run this over some existing PRs to see the results? I am curious what would be the final behaviour

premun · 2026-06-25T11:50:45Z

+
+        var intersection = sourceRepoChangedFiles.Where(vmrPrChangedFiles.Contains).ToList();
+        var sourceRepoOnlyChanges = sourceRepoChangedFiles.Where(f => !vmrPrChangedFiles.Contains(f)).ToList();
+        var unexpectedFiles = vmrPrChangedFiles.Where(f => !sourceRepoChangedFiles.Contains(f)).ToList();


I'd name this filesChangedInPrOnly then you might not need the comment below (btw this is always a good pointer to increase readability - to see if we can reflect purpose in variable names that can render a comment useless)

premun · 2026-06-25T11:52:55Z

+        HashSet<string> vmrPrChangedFiles = await GetChangedMappingFilesAsync(
+            vmr, mappingName, vmrTargetBranch, vmrHeadBranch, relativePath: srcMappingPrefix, cancellationToken: cancellationToken);
+
+        var intersection = sourceRepoChangedFiles.Where(vmrPrChangedFiles.Contains).ToList();


I'd name intersection something better. Like filesChangedInBoth or something

premun · 2026-06-25T12:43:32Z

+        ILocalGitRepo sourceRepo,
+        ILocalGitRepo vmr,
+        string file,
+        string srcMappingPath,


srcMappingPath can be passed around as LocalPath and then you could have srcMappingPath / file instead of $"{srcMappingPath}/{file}".

Applies here and in other places

premun · 2026-06-25T12:45:27Z

    }
+
+    public async Task SetCodeflowApprovalCheck(CodeflowApprovalCheck check) =>
+        await _codeflowApprovalCheckReminders.SetReminderAsync(check, PullRequestUpdater.DefaultReminderDelay, true);


Are we sending it to the right queue? The codeflow workitem one

yes, but it's missing the isCodeflow, will add so it's just not a naked true, my bad

dkurepa · 2026-06-25T14:37:56Z

Did you run this over some existing PRs to see the results? I am curious what would be the final behaviour

I did yes. I ran it on some basic FFs that were supposed to pass the check maestro-auth-test/dotnet#259
but then I also ran it on the FF that was duplicating a method, and it was failing there as expected maestro-auth-test/dotnet#260

adamzip · 2026-06-25T15:03:51Z

+        if (pr == null)
+        {
+            _logger.LogError("No in-progress PR found for codeflow approval check");
+            return;


Would be nice to also store the PR url in the workitem and make sure it matches the tracked PR, to avoid any weird timing issues where the diff verifier runs on a PR that was just closed, and ends up commenting on a PR that was just opened

adamzip · 2026-06-25T15:08:27Z

+                "Skipping codeflow approval check for PR {prUrl} because it is no longer open (status: {status})",
+                pr.Url,
+                prInfo.Status);
+            return;


I don't think this gives any guarantee. If we don't want to trust the cached PR because its state could be out of date, maybe we should check only at the end before posting. or, never.
It's possible that a PR is closed or merged before the verifier can even run. IMO it can still be useful in that case for the verifier to post a comment on the closed PR - it can still alert you to a bad codeflow, or help in any investigations. I would just not care about the PR status

this is getting info from GitHub/Azdo, not the cache

adamzip · 2026-06-25T15:13:26Z

+            return;
+        }
+
+        if (await _codeflowSourceDiffVerifier.ForwardFlowMatchesSourceDiffAsync(


nit: can this method call be taken out of the if and have its own var?

adamzip · 2026-06-25T15:24:37Z

I did yes. I ran it on some basic FFs that were supposed to pass the check maestro-auth-test/dotnet#259 but then I also ran it on the FF that was duplicating a method, and it was failing there as expected maestro-auth-test/dotnet#260

What's the expected action when the check fails? Should it post a comment about the failure, or should it just skip posting the approval and do nothing?

dkurepa · 2026-06-25T15:26:38Z

I did yes. I ran it on some basic FFs that were supposed to pass the check maestro-auth-test/dotnet#259 but then I also ran it on the FF that was duplicating a method, and it was failing there as expected maestro-auth-test/dotnet#260

What's the expected action when the check fails? Should it post a comment about the failure, or should it just skip posting the approval and do nothing?

nothing, this will eventually be an approval on the pr itself, the comment is there just temporarily

adamzip · 2026-06-25T15:40:40Z

+            _logger.LogInformation(
+                "Codeflow approval check for PR {prUrl} passed; posting approval comment",
+                pr.Url);
+            await remote.CommentPullRequestAsync(pr.Url, "Looks good");


Should this include build id, SHA, or some other information that can associate the comment with a specific codeflow, for PRs that flow multiple times and get multiple automatic checks?

right now we don't have this information in this WorkItem, we'd have to provide it from the trigger.
But it might not be that useful too, you can get this info in a different way, so I guess it'd be a matter of convenience

dkurepa added 6 commits June 8, 2026 16:54

bootstrap arcade-services for agentic coding

e16614d

First draft, still no review

6b3f3cc

Merge remote-tracking branch 'origin/main' into dkurepa/CodeflowApproval

618e2f3

small improvements

a85a621

Run the codeflow code check as it's own job

f9111da

Add tests

aa86859

dkurepa changed the title ~~bootstrap arcade-services for agentic coding~~ [DRAFT] Comment on FF PRs that look good to merge Jun 23, 2026

dkurepa added 8 commits June 24, 2026 10:38

Merge remote-tracking branch 'origin/main' into dkurepa/CodeflowAppro…

2a3ade4

…val1

Merge remote-tracking branch 'origin/main' into dkurepa/CodeflowAppro…

5576916

…val1

cleanup

026ef42

test

10fb18c

Rename to Codeflow Approval Check

ff31e68

Revert test changes

a47dee7

Add verifier tests

8e9d7e8

cleanup

87e83fc

dkurepa marked this pull request as ready for review June 24, 2026 12:27

Copilot AI review requested due to automatic review settings June 24, 2026 12:27

dkurepa changed the title ~~[DRAFT] Comment on FF PRs that look good to merge~~ Comment on FF PRs that look good to merge Jun 24, 2026

Copilot started reviewing on behalf of dkurepa June 24, 2026 12:27 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

copilot cr changes

e9a8e99

dkurepa commented Jun 25, 2026

View reviewed changes

Check commit authors, rename things, add logging

54cce80

premun reviewed Jun 25, 2026

View reviewed changes

adamzip reviewed Jun 25, 2026

View reviewed changes

cr changes

c7b447d

dkurepa added 2 commits June 26, 2026 09:26

Add distributed redis lock around codeflow approval task

1643888

Improve message

1aa40d9

Uh oh!

Conversation

dkurepa commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

premun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkurepa commented Jun 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamzip commented Jun 25, 2026

Uh oh!

dkurepa commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dkurepa commented Jun 23, 2026 •

edited

Loading

dkurepa commented Jun 25, 2026 •

edited

Loading