diff --git a/tools/devops/automation/scripts/GitHub.Tests.ps1 b/tools/devops/automation/scripts/GitHub.Tests.ps1 index ea6b26f8c2cf..ac74796090d1 100644 --- a/tools/devops/automation/scripts/GitHub.Tests.ps1 +++ b/tools/devops/automation/scripts/GitHub.Tests.ps1 @@ -257,8 +257,17 @@ Describe 'IsCurrentCommitLatestInPR' { "head" = @{ "sha" = "different123hash" } + "base" = @{ + "ref" = "main" + } } } -ModuleName 'GitHub' + Mock Test-GitIsAncestor { + return $false + } -ModuleName 'GitHub' + Mock Get-GitCommitParents { + return @("basebranch123") + } -ModuleName 'GitHub' $result = Get-IsCurrentCommitLatestInPR -Org "testorg" -Repo "testrepo" -Token "test-token" -Hash "abc123def456" -PrIDs @("123") $result | Should -Be $false @@ -276,5 +285,87 @@ Describe 'IsCurrentCommitLatestInPR' { $result = Get-IsCurrentCommitLatestInPR -Org "testorg" -Repo "testrepo" -Token "test-token" -Hash "abc123def456" -PrIDs @() # Empty array means not in PR $result | Should -Be $true } + + It 'returns true when the current commit is a synthetic merge commit for the latest PR head' { + Mock Invoke-Request { + return @{ + "head" = @{ + "sha" = "abc123def456" + } + "base" = @{ + "ref" = "main" + } + } + } -ModuleName 'GitHub' + Mock Test-GitIsAncestor { + return $false + } -ModuleName 'GitHub' + Mock Get-GitCommitParents { + return @("basebranch123", "abc123def456") + } -ModuleName 'GitHub' + + $result = Get-IsCurrentCommitLatestInPR -Org "testorg" -Repo "testrepo" -Token "test-token" -Hash "merge123456" -PrIDs @("123") + $result | Should -Be $true + } + + It 'returns false when commit is in the base branch despite having the PR head as a parent' { + Mock Invoke-Request { + return @{ + "head" = @{ + "sha" = "prhead123" + } + "base" = @{ + "ref" = "main" + } + } + } -ModuleName 'GitHub' + Mock Test-GitIsAncestor { + param ($Commit, $Branch) + # The commit is in the base branch + return $Branch -eq "origin/main" + } -ModuleName 'GitHub' + Mock Get-GitCommitParents { + return @("prhead123", "someothercommit") + } -ModuleName 'GitHub' + + $result = Get-IsCurrentCommitLatestInPR -Org "testorg" -Repo "testrepo" -Token "test-token" -Hash "mergecommit456" -PrIDs @("123") + $result | Should -Be $false + } + + It 'returns false when commit is in the PR branch despite having the PR head as a parent' { + Mock Invoke-Request { + return @{ + "head" = @{ + "sha" = "prhead123" + } + "base" = @{ + "ref" = "main" + } + } + } -ModuleName 'GitHub' + Mock Test-GitIsAncestor { + param ($Commit, $Branch) + # The commit is in the PR branch (ancestor of PR head) + return $Branch -eq "prhead123" + } -ModuleName 'GitHub' + Mock Get-GitCommitParents { + return @("prhead123", "someothercommit") + } -ModuleName 'GitHub' + + $result = Get-IsCurrentCommitLatestInPR -Org "testorg" -Repo "testrepo" -Token "test-token" -Hash "mergecommit456" -PrIDs @("123") + $result | Should -Be $false + } + } +} + +Describe 'IsPR' { + It 'returns true for manual builds that use a PR merge ref' { + Set-Item -Path "Env:BUILD_REASON" -Value "Manual" + Set-Item -Path "Env:BUILD_SOURCEBRANCH" -Value "refs/pull/123/merge" + + $githubComments = New-GitHubCommentsObject -Org "testorg" -Repo "testrepo" -Token "test-token" + + $githubComments.IsPR() | Should -Be $true + $githubComments.PRIds | Should -Contain "123" } } diff --git a/tools/devops/automation/scripts/GitHub.psm1 b/tools/devops/automation/scripts/GitHub.psm1 index 5f8ad2bc9190..82436b73540d 100644 --- a/tools/devops/automation/scripts/GitHub.psm1 +++ b/tools/devops/automation/scripts/GitHub.psm1 @@ -41,6 +41,45 @@ function Invoke-Request { } while ($true) } +function Get-GitCommitParents { + param ( + [ValidateNotNullOrEmpty ()] + [string] + $Commit + ) + + $output = & git rev-list --parents -n 1 -- $Commit 2>$null + if ($LASTEXITCODE -ne 0 -or [string]::IsNullOrWhiteSpace($output)) { + throw [System.InvalidOperationException]::new("Failed to get parent commits for '$Commit'.") + } + + $commits = $output.Trim() -split '\s+' + if ($commits.Length -le 1) { + return @() + } + + return @($commits[1..($commits.Length - 1)]) +} + +function Test-GitIsAncestor { + param ( + [ValidateNotNullOrEmpty ()] + [string] + $Commit, + + [ValidateNotNullOrEmpty ()] + [string] + $Branch + ) + + & git merge-base --is-ancestor -- $Commit $Branch 2>$null + switch ($LASTEXITCODE) { + 0 { return $true } + 1 { return $false } + default { throw [System.InvalidOperationException]::new("Failed to determine whether '$Commit' is an ancestor of '$Branch'.") } + } +} + class GitHubStatus { [ValidateNotNullOrEmpty ()] [string] $Status [ValidateNotNullOrEmpty ()] [string] $Description @@ -228,6 +267,8 @@ class GitHubComments { [ValidateNotNullOrEmpty ()][string] $Token [string] $Hash [string[]] $PRIds + [bool] $CurrentCommitIsLatestInPR + [bool] $CurrentCommitIsLatestInPRCalculated hidden static [string] $GitHubGraphQLEndpoint = "https://api.github.com/graphql" GitHubComments ( @@ -240,6 +281,8 @@ class GitHubComments { $this.Token = $githubToken $this.Hash = $null $this.PRIds = [string[]]@() + $this.CurrentCommitIsLatestInPR = $false + $this.CurrentCommitIsLatestInPRCalculated = $false } GitHubComments ( @@ -253,6 +296,8 @@ class GitHubComments { $this.Token = $githubToken $this.Hash = $hash $this.PRIds = Get-GitHubPRsForHash -Org $githubOrg -Repo $githubRepo -Token $githubToken -Hash $hash + $this.CurrentCommitIsLatestInPR = $false + $this.CurrentCommitIsLatestInPRCalculated = $false } [bool] IsPR() { @@ -268,13 +313,12 @@ class GitHubComments { return $true; } - if (($Env:BUILD_REASON -eq "ResourceTrigger")) { - $sourceBranch = $Env:BUILD_SOURCEBRANCH - if ($sourceBranch.StartsWith("refs/pull/") -and $sourceBranch.EndsWith("/merge")) { - # Set the PRs parsing the source branch - $this.PRIds = @($sourceBranch.Replace("refs/pull/", "").Replace("/merge", "")) - return $true - } + $sourceBranch = $Env:BUILD_SOURCEBRANCH + if ($sourceBranch -and $sourceBranch.StartsWith("refs/pull/") -and $sourceBranch.EndsWith("/merge")) { + # Some builds (such as pipeline-completion/manual follow-up jobs) still use PR merge refs + # even when BUILD_REASON is not "PullRequest". + $this.PRIds = @($sourceBranch.Replace("refs/pull/", "").Replace("/merge", "")) + return $true } return $false @@ -725,13 +769,21 @@ mutation { Also returns true if not in a PR context or if hash comparison cannot be performed. #> [bool] IsCurrentCommitLatestInPR() { + if ($this.CurrentCommitIsLatestInPRCalculated) { + return $this.CurrentCommitIsLatestInPR + } + # If we're not in a PR context, we can't determine this if (-not $this.IsPR()) { + $this.CurrentCommitIsLatestInPR = $true + $this.CurrentCommitIsLatestInPRCalculated = $true return $true } # If we don't have a hash to compare, assume it's latest if (-not $this.Hash) { + $this.CurrentCommitIsLatestInPR = $true + $this.CurrentCommitIsLatestInPRCalculated = $true return $true } @@ -747,14 +799,65 @@ mutation { $prInfo = Invoke-Request -Request { Invoke-RestMethod -Uri $url -Headers $headers -Method "GET" -ContentType 'application/json' } $latestCommit = $prInfo.head.sha - + Write-Host "Current commit: $($this.Hash)" Write-Host "Latest commit in PR #${prId}: $latestCommit" - - return $this.Hash -eq $latestCommit + + $hashesToCompare = [System.Collections.Generic.List[string]]::new() + $hashesToCompare.Add($this.Hash) + if ($Env:SYSTEM_PULLREQUEST_SOURCECOMMITID) { + $hashesToCompare.Add($Env:SYSTEM_PULLREQUEST_SOURCECOMMITID) + } + if ($Env:BUILD_SOURCEVERSION) { + $hashesToCompare.Add($Env:BUILD_SOURCEVERSION) + } + + foreach ($hash in ($hashesToCompare | Select-Object -Unique)) { + if ($hash -eq $latestCommit) { + Write-Host "Detected latest PR commit via hash comparison: $hash" + $this.CurrentCommitIsLatestInPR = $true + $this.CurrentCommitIsLatestInPRCalculated = $true + return $true + } + } + + # PR validation builds typically use a synthetic merge commit. If that's the hash we were + # given, accept it when one of its local git parents is the current PR head commit. + # However, skip this check if the commit is already in the base or PR branch - a commit + # that's in either branch is not a synthetic merge commit, and checking its parents could + # produce a false positive (e.g. a merge from the base branch into the PR branch). + $baseBranch = $prInfo.base.ref + $isInKnownBranch = $false + + if ($baseBranch -and (Test-GitIsAncestor -Commit $this.Hash -Branch "origin/$baseBranch")) { + Write-Host "Commit $($this.Hash) is in the base branch ($baseBranch), skipping synthetic merge check." + $isInKnownBranch = $true + } + + if (-not $isInKnownBranch -and (Test-GitIsAncestor -Commit $this.Hash -Branch $latestCommit)) { + Write-Host "Commit $($this.Hash) is in the PR branch, skipping synthetic merge check." + $isInKnownBranch = $true + } + + if (-not $isInKnownBranch) { + foreach ($parent in (Get-GitCommitParents -Commit $this.Hash)) { + if ($parent -eq $latestCommit) { + Write-Host "Detected latest PR commit via merge parent: $parent" + $this.CurrentCommitIsLatestInPR = $true + $this.CurrentCommitIsLatestInPRCalculated = $true + return $true + } + } + } + + $this.CurrentCommitIsLatestInPR = $false + $this.CurrentCommitIsLatestInPRCalculated = $true + return $false } catch { Write-Host "Error checking if current commit is latest in PR: $_" # On error, assume it's the latest to avoid hiding valid comments + $this.CurrentCommitIsLatestInPR = $true + $this.CurrentCommitIsLatestInPRCalculated = $true return $true } } diff --git a/tools/devops/automation/templates/common/checkout.yml b/tools/devops/automation/templates/common/checkout.yml index e19d8fddd6a3..ec054c523e92 100644 --- a/tools/devops/automation/templates/common/checkout.yml +++ b/tools/devops/automation/templates/common/checkout.yml @@ -55,7 +55,7 @@ steps: workingDirectory: $(System.DefaultWorkingDirectory)/$(BUILD_REPOSITORY_TITLE)/tools/devops/automation/scripts timeoutInMinutes: 15 env: - IS_PR: and(eq(parameters.isPR, 'true'), not(startsWith(variables['Build.SourceBranch'], 'refs/pull'))) + IS_PR: ${{ parameters.isPR }} - checkout: yaml-templates clean: true