From 76068f845fd366d0a9a6e40d206c37ad8b19a6d8 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Wed, 22 Apr 2026 21:23:19 +0200 Subject: [PATCH 1/2] [devops] Fix PR latest-commit detection Treat PR merge refs as PR builds regardless of the reported build reason, and consider synthetic merge commits current when one of their parents matches the PR head. Also pass the checkout template's isPR flag correctly so the fixed commit hash is used for PR comment handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../automation/scripts/GitHub.Tests.ps1 | 91 ++++++++++++++ tools/devops/automation/scripts/GitHub.psm1 | 119 ++++++++++++++++-- .../automation/templates/common/checkout.yml | 2 +- 3 files changed, 201 insertions(+), 11 deletions(-) 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..3d866a38cfeb 100644 --- a/tools/devops/automation/scripts/GitHub.psm1 +++ b/tools/devops/automation/scripts/GitHub.psm1 @@ -41,6 +41,41 @@ 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 + return $LASTEXITCODE -eq 0 +} + class GitHubStatus { [ValidateNotNullOrEmpty ()] [string] $Status [ValidateNotNullOrEmpty ()] [string] $Description @@ -228,6 +263,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 +277,8 @@ class GitHubComments { $this.Token = $githubToken $this.Hash = $null $this.PRIds = [string[]]@() + $this.CurrentCommitIsLatestInPR = $false + $this.CurrentCommitIsLatestInPRCalculated = $false } GitHubComments ( @@ -253,6 +292,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 +309,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 +765,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 +795,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 From 3cc6e0e212b9408d8215a13fb94af1cdad964005 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Thu, 23 Apr 2026 13:53:51 +0200 Subject: [PATCH 2/2] Test-GitIsAncestor: handle git error exit codes explicitly Distinguish exit code 1 (not an ancestor) from 128+ (git error such as missing refs in a shallow checkout). Throw on unexpected exit codes so the outer try/catch falls back to the safe path instead of silently returning false. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tools/devops/automation/scripts/GitHub.psm1 | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tools/devops/automation/scripts/GitHub.psm1 b/tools/devops/automation/scripts/GitHub.psm1 index 3d866a38cfeb..82436b73540d 100644 --- a/tools/devops/automation/scripts/GitHub.psm1 +++ b/tools/devops/automation/scripts/GitHub.psm1 @@ -72,8 +72,12 @@ function Test-GitIsAncestor { $Branch ) - & git merge-base --is-ancestor $Commit $Branch 2>$null - return $LASTEXITCODE -eq 0 + & 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 {