RoslynTools: handle snap commits by jjonescz · Pull Request #85843 · dotnet/roslyn

jjonescz · 2026-09-30T12:35:55Z

For example, the PR list in insertion https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/787957 contains commits from release/insiders branch even though main branch was snapped into it. This PR should fix that and display only the following:

View Complete Diff of Changes

[release/insiders] Update Gladstone and related SDK assemblies

Snap main into release/insiders (18.12)

azure-pipelines · 2026-09-30T12:36:18Z

Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run.

Copilot

Copilot review overview

🟡 Changes recommended

Azure DevOps commit details are fetched with unbounded concurrency, risking throttling for large ranges.

Review effort: Balanced
Findings: 1

Open (1)

Bound concurrent commit detail requests to prevent throttling · New

What changed in this PR

Updates RoslynTools changelog generation to exclude branch history replaced by snap merges.

Changes:

Adds tree-aware commit-history filtering.
Supports paginated GitHub and Azure DevOps comparisons.
Adds regression tests across local Git and both providers.

File	Description
`Tool/PRFinder/PRFinder.cs`	Applies filtering and cancellation.
`Tool/PRFinder/GitCommit.cs`	Records parent commits.
`Tool/PRFinder/CommitHistoryFilter.cs`	Implements tree-based filtering.
`Tool/Insertion/RoslynInsertionTool.VisualStudioTeamServices.cs`	Adds provider pagination and filtering.
`Tool/Insertion/RoslynInsertionTool.cs`	Propagates cancellation.
`Tool/Commands/PRFinderCommand.cs`	Passes command cancellation.
`Test/PRFinder/PRFinderTests.cs`	Tests local repository behavior.
`Test/PRFinder/CommitHistoryFilterTests.cs`	Tests graph-filtering scenarios.
`Test/Insertion/InsertionChangelogTests.cs`	Tests GitHub changelogs.
`Test/Insertion/AzureInsertionChangelogTests.cs`	Tests Azure DevOps changelogs.

Copilot

Copilot review overview

🔵 Needs a closer look

Azure filtering should reuse fetched tree IDs to avoid potentially hundreds of sequential REST requests.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Bound concurrent commit detail requests to prevent throttling

Previously missed (1)

In code that hasn't changed since last review

Avoid redundant commit fetches causing PR creation delays

src/Tools/dotnet-roslyn-tools/Tool/Insertion/RoslynInsertionTool.VisualStudioTeamServices.cs:614

The filter now performs a sequential GetItemAsync REST call for every unique merge commit and parent, even though the full GitCommit objects fetched above already include TreeId. A large insertion range with many merged PRs can therefore add hundreds of serialized Azure DevOps requests and significantly delay or time out PR creation. Cache each detail's c.TreeId while constructing commits, and use that cache here; only fetch a tree for a parent outside the returned range.

Copilot

Copilot review overview

🔵 Needs a closer look

The provider-sensitive graph changes look sound, but the latest CI run is still pending.

Review effort: Balanced
Findings: None

jasonmalinowski

See if Copilot can do a simplification pass on this -- if it cleans up a lot of unnecessary tests that make the rest easier to follow.

jasonmalinowski · 2026-09-30T23:15:45Z

+    [Fact]
+    public async Task DetailFailurePreventsLaterBatches()
+    {
+        var commits = Enumerable.Range(1, 33).Reverse()
+            .Select(i => Commit($"commit-{i}", $"Update (#{i})", i == 1 ? "base" : $"commit-{i - 1}"))
+            .ToArray();
+        var releaseDetails = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        var failure = new IOException("Commit details unavailable.");
+        using var client = new Client(commits, new Dictionary<string, string>())
+        {
+            BeforeCommitDetailsAsync = (id, token) => id == "commit-33"
+                ? Task.FromException(failure)
+                : releaseDetails.Task.WaitAsync(token),
+        };
+
+        var getChanges = RoslynInsertionTool.GetChangesBetweenBuildsFromAzDOAsync(client, "project", "repo", RepoUrl, "base", "commit-33");
+        Assert.Equal(16, client.CommitDetailRequests.Count);
+        Assert.False(getChanges.IsCompleted);
+        releaseDetails.SetResult();
+        var actualFailure = await Assert.ThrowsAsync<IOException>(() => getChanges);
+
+        Assert.Same(failure, actualFailure);
+        Assert.Equal(commits.Take(16).Select(commit => commit.CommitId), client.CommitDetailRequests);
+        Assert.Empty(client.TreeLookups);
+    }
+
+    [Fact]
+    public async Task CancellationDuringDetailsPreventsLaterBatches()
+    {
+        var commits = Enumerable.Range(1, 33).Reverse()
+            .Select(i => Commit($"commit-{i}", $"Update (#{i})", i == 1 ? "base" : $"commit-{i - 1}"))
+            .ToArray();
+        using var cancellation = new CancellationTokenSource();
+        var releaseDetails = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        using var client = new Client(commits, new Dictionary<string, string>())
+        {
+            BeforeCommitDetailsAsync = (_, token) =>
+            {
+                Assert.Equal(cancellation.Token, token);
+                return releaseDetails.Task.WaitAsync(token);
+            },
+        };
+
+        var getChanges = RoslynInsertionTool.GetChangesBetweenBuildsFromAzDOAsync(client, "project", "repo", RepoUrl, "base", "commit-33", cancellation.Token);
+        Assert.Equal(16, client.CommitDetailRequests.Count);
+        cancellation.Cancel();
+        await Assert.ThrowsAnyAsync<OperationCanceledException>(() => getChanges);
+
+        Assert.Equal(commits.Take(16).Select(commit => commit.CommitId), client.CommitDetailRequests);
+        Assert.Empty(client.TreeLookups);
+    }
+
+    [Fact]
+    public async Task CancellationBetweenBatchesPreventsFurtherRequests()
+    {
+        var commits = Enumerable.Range(1, 33).Reverse()
+            .Select(i => Commit($"commit-{i}", $"Update (#{i})", i == 1 ? "base" : $"commit-{i - 1}"))
+            .ToArray();
+        using var cancellation = new CancellationTokenSource();
+        using var client = new Client(commits, new Dictionary<string, string>())
+        {
+            BeforeCommitDetailsAsync = (id, _) =>
+            {
+                if (id == "commit-18")
+                {
+                    cancellation.Cancel();
+                }
+
+                return Task.CompletedTask;
+            },
+        };
+
+        await Assert.ThrowsAnyAsync<OperationCanceledException>(() =>
+            RoslynInsertionTool.GetChangesBetweenBuildsFromAzDOAsync(client, "project", "repo", RepoUrl, "base", "commit-33", cancellation.Token));
+
+        Assert.Equal(commits.Take(16).Select(commit => commit.CommitId), client.CommitDetailRequests);
+        Assert.Empty(client.TreeLookups);
+    }


Can we reduce the tests in this file to the core behavior changes? I've noticed this a lot with Copilot recently that it likes to write a lot of (really verbose) tests to assert that cancellation is respected and exceptions flow. But these tests aren't really useful, and also mandates all sorts of funky test hooks.

jasonmalinowski · 2026-10-01T01:27:06Z

+    [Fact]
+    public async Task FetchesAllCommitPagesBeforeFiltering()
+    {
+        var commits = Enumerable.Range(1, 1001).Reverse()
+            .Select(i => Commit($"commit-{i}", $"Update (#{i})", i == 1 ? "base" : $"commit-{i - 1}"))
+            .ToArray();
+        var releaseDetails = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        var pendingDetails = 0;
+        using var client = new Client(commits, new Dictionary<string, string>())
+        {
+            BeforeCommitDetailsAsync = async (_, token) =>
+            {
+                var pending = Interlocked.Increment(ref pendingDetails);
+                try
+                {
+                    Assert.InRange(pending, 1, 16);
+                    await releaseDetails.Task.WaitAsync(token);
+                    await Task.Yield();
+                }
+                finally
+                {
+                    Interlocked.Decrement(ref pendingDetails);
+                }
+            },
+        };
+
+        var getChanges = RoslynInsertionTool.GetChangesBetweenBuildsFromAzDOAsync(client, "project", "repo", RepoUrl, "base", "commit-1001");
+        try
+        {
+            Assert.Equal(16, client.CommitDetailRequests.Count);
+            Assert.Equal(16, Volatile.Read(ref pendingDetails));
+        }
+        finally
+        {
+            releaseDetails.SetResult();
+        }
+
+        var (changes, _) = await getChanges;
+
+        Assert.Equal(commits.Select(commit => commit.CommitId), changes.Select(commit => commit.CommitId));
+        Assert.Equal(commits.Select(commit => commit.CommitId), client.CommitDetailRequests);
+        Assert.Equal(0, pendingDetails);
+        Assert.Equal([0, 1000], client.PageOffsets);
+        Assert.Empty(client.TreeLookups);
+    }


This test seems to be baking in unecessary implemetation details.

Copilot

Copilot review overview

🟢 Approval recommended

The implementation matches the snap topology and has comprehensive provider and graph regression coverage.

Review effort: Balanced
Findings: None

RoslynTools: handle snap commits

b6336b8

Copilot AI balanced review requested due to automatic review settings September 30, 2026 12:35

jjonescz requested a review from a team as a code owner September 30, 2026 12:35

Copilot started reviewing on behalf of jjonescz September 30, 2026 12:36 View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

Comment thread src/Tools/dotnet-roslyn-tools/Tool/Insertion/RoslynInsertionTool.VisualStudioTeamServices.cs Outdated

jjonescz added 2 commits September 30, 2026 15:32

Batch requests

9568d41

Remove unused member

91347b2

Copilot AI balanced review requested due to automatic review settings September 30, 2026 13:51

Copilot started reviewing on behalf of jjonescz September 30, 2026 13:52 View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

Cache tree lookups

4dc0e10

Copilot AI balanced review requested due to automatic review settings September 30, 2026 14:29

Copilot started reviewing on behalf of jjonescz September 30, 2026 14:29 View session

Copilot AI reviewed Sep 30, 2026

View reviewed changes

akhera99 approved these changes Sep 30, 2026

View reviewed changes

jasonmalinowski reviewed Sep 30, 2026

View reviewed changes

jasonmalinowski reviewed Oct 1, 2026

View reviewed changes

Simplify tests

f0cf27e

Copilot AI balanced review requested due to automatic review settings October 1, 2026 11:42

Copilot started reviewing on behalf of jjonescz October 1, 2026 11:43 View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

jjonescz requested a review from jasonmalinowski October 2, 2026 08:56

Navigation Menu

RoslynTools: handle snap commits - #85843

RoslynTools: handle snap commits#85843
jjonescz wants to merge 5 commits into
dotnet:mainfrom
jjonescz:pr-list

jjonescz commented Sep 30, 2026 •

edited

Loading

Uh oh!

azure-pipelines Bot commented Sep 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

jasonmalinowski left a comment

Uh oh!

jasonmalinowski Sep 30, 2026

Uh oh!

jasonmalinowski Oct 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

jjonescz commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azure-pipelines Bot commented Sep 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

jasonmalinowski left a comment

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jjonescz commented Sep 30, 2026 •

edited

Loading