We need to implement an alert mechanism when this fails. Right now, Legacy coherency is probably filling some gaps where repos are failing the strict coherency update. Whether it gets it right is unknown, but they're getting updates rather than failing silently.

We should implement this part: #2687 in this PR:

• We allow coherency to fail and still do an update PR. This is not the case today.
• In the update PR, we add a new Maestro policy check that is "coherency valid" (sort of like the Do not automerge downgrades). This is always on, regardless of repo policies. If coherency is failing, then we make this check Red

This behavior would not be implemented in local darc, just the subscription actor service. The check, if it could not get info on which dependencies fail coherency, would tell the user to go and run local darc to resolve.

I am going to introduce a new merge policy class like:

public class ValidateCoherencyMergePolicy : MergePolicy
{
    public override string DisplayName => "TODO";

    /// overrides `IMergePolicy.EvaluateAsync`
    public override async Task<MergePolicyEvaluationResult> EvaluateAsync(IPullRequest pr, IRemote darc)
    {
        string prBranch = "";
        string repoUrl = "";

        List<DependencyDetail> existingDependencies = (await darc.GetDependenciesAsync(repoUrl, prBranch)).ToList();
        List<DependencyUpdate> coherencyUpdates = new List<DependencyUpdate>();
        try
        {
            // remoteFactory ???
            coherencyUpdates = await darc.GetRequiredCoherencyUpdatesAsync(existingDependencies, remoteFactory, CoherencyMode.Strict);
        }
        catch (DarcCoherencyException)
        {
            return Fail("Is not coherent");
        }

        if (coherencyUpdates.Any())
        {
            return Fail("Is not coherent");
        }

        return Succeed("Is coherent.");

    }
}

and utilize the IRemote.GetRequiredCoherencyUpdatesAsync(IEnumerable<DependencyDetail>, IRemoteFactory, CoherencyMode) to get the list of coherency update and if it empty - the repo + pr branch is coherent.

[!] But we don't have access to IRemoteFactory on the MergePolicy level. What about to modify the interface of the IMergePolicy interface?

public interface IMergePolicy : IMergePolicyInfo
{
-   Task<MergePolicyEvaluationResult> EvaluateAsync(IPullRequest pr, IRemote darc);
+   Task<MergePolicyEvaluationResult> EvaluateAsync(IPullRequest pr, IRemoteFactory remoteFactory);
}

/cc @riarenas , @mmitche

I don't think you need to redo the coherency calculation. Instead, GetRequiredUpdates would return whether the coherency update failed: https://github.com/dotnet/arcade-services/blob/main/src/Maestro/SubscriptionActorService/PullRequestActor.cs#L1083-L1088

Then, you modify the IPullRequest interface to include a bit that indicates whether the last coherency update failed or not. It currently just knows about the set of updates in the PR.

The new merge policy would check this bit and let the user know.

As one of the results of this work, could you please log an issue with proposal to improve our testing capabilities for these scenarios? This might involve anything that you found missing while working on this issue, such as having a local BAR database or resources for integration testing.

@riarenas , @mmitche could you please short feedback about the latest changes in the PR. @riarenas , we can have a call and discuss next steps.

1. Example of a test PR with a failing coherency check: https://github.com/maestro-auth-test/maestro-test3/pull/2059/checks?check_run_id=13442834926
2. I am going to add one more test scenario to ScenarioTests_GitHubFlow.cs
3. As we are going to remove fallback and introduce new check, this new Github check won't be available in existing subscription (as I understand). Do I need to update all existing subscriptions? Write a data change script?

1. Example of a test PR with a failing coherency check: https://github.com/maestro-auth-test/maestro-test3/pull/2059/checks?check_run_id=13442834926

Yay! a check! Is it possible to add instructions to the error message on how to fix the coherency issue? I believe the Exception can capture some more details that would be helpful to add. I'll see if I can track which ones I'm talking about.

2. I am going to add one more test scenario to ScenarioTests_GitHubFlow.cs

Sounds good

3. As we are going to remove fallback and introduce new check, this new Github check won't be available in existing subscription (as I understand). Do I need to update all existing subscriptions? Write a data change script?

We could add the check to the standard merge policy, which is already an aggregation of policies that we suggest everyone should have: https://github.com/dotnet/arcade-services/blob/main/src/Maestro/Maestro.MergePolicies/StandardMergePolicy.cs

For subscriptions not using the standard policy I see a few options:

• we just inform partners that this is a new check and to please add it to their subscriptions.
• We update every subscription (ideally through the API or through darc, not through the database) to include the new policy
• We inject the policy and check to every subscription, regardless of how they have them configured.

PR has been updated. Here are a few examples:

• Failing coherency update: https://github.com/maestro-auth-test/maestro-test3/pull/2080/checks?check_run_id=13719412670
• Successful coherency update: https://github.com/maestro-auth-test/maestro-test2/pull/18497/checks?check_run_id=13719232632

I did refactor/rewrite the ScenarioTests_GitHubFlow::Darc_GitHubFlow_NonBatched_WithCoherency test, since the Strick algorithm consistently failed and it always fall back to the Legacy algorithm.