From 123bd4d1d63d838fe3f2a177cf474cf04c928ebf Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:23:50 +0000 Subject: [PATCH] feat(scripts): finish open loop PRs before starting new issues Workers skipped every PR whose checks were running and claimed new issues instead, so open PRs piled up (15 in flight) and each one raised the odds of conflicts. - Stop starting new issues once -MaxOpenPrs (default 8) unparked loop PRs are open. Existing PRs, stranded branches and stale claims still run. - Treat a PR with unresolved review threads, or a changes-requested review newer than the last push and the loop's last reply, as actionable even while its checks run, and give it full effort instead of the light merge-gate effort. --- scripts/Start-IssuePrLoop.ps1 | 99 ++++++++++++++++++++++++++++++----- scripts/Test-IssuePrLoop.ps1 | 48 ++++++++++++++++- 2 files changed, 134 insertions(+), 13 deletions(-) diff --git a/scripts/Start-IssuePrLoop.ps1 b/scripts/Start-IssuePrLoop.ps1 index dc2c8cfb93..73ae2c8d0a 100755 --- a/scripts/Start-IssuePrLoop.ps1 +++ b/scripts/Start-IssuePrLoop.ps1 @@ -10,12 +10,15 @@ # 2. Runs the merged-worktree sweep and surveys open PRs and issues with `gh`. When # the latest push CI run of any workflow on main failed, fixing main comes first. # 3. Includes draft PRs for review processing. Skips `needs-human` and `on-hold` -# items, PRs whose checks are still running, and items whose GitHub state is +# items, PRs whose checks are still running (unless they conflict, have unresolved +# review threads, or have a changes-requested review newer than the last push), and items whose GitHub state is # unchanged since an agent last deferred them. Also skips issues that cannot be worked yet: issues with open native # sub-issues or open `blocked_by` dependencies, issues that already have an open # closing PR, and bot-owned tracking issues. Stranded branches follow the same # rules. Ranks local `issue-*` branches with unpublished commits and no open PR -# above new issues. Takes issues by `priority:*` tier (critical, high, medium or +# above new issues. Starts no new issue while -MaxOpenPrs or more of the loop's +# PRs are open and not parked, because every PR in flight is another chance of a +# conflict; existing PRs, stranded branches, and stale claims still run. Takes issues by `priority:*` tier (critical, high, medium or # unlabelled, low), `ci` first within a tier, then parents whose sub-issues are all # closed, then issues that block the most other issues. An item that a deferral # named as its blocker goes first, so the next unit works the blocker instead of @@ -61,7 +64,7 @@ # # Stop: create the stop file printed at startup (checked between units), or Ctrl+C. # Usage: pwsh scripts/Start-IssuePrLoop.ps1 [-Agent auto|claude|codex] [-IdleSeconds n] [-MaxUnits n] -# [-DeferSeconds n] [-MaxDeferSeconds n] [-LightEffort level] [-HeavyEffort level] +# [-DeferSeconds n] [-MaxDeferSeconds n] [-MaxOpenPrs n] [-LightEffort level] [-HeavyEffort level] # [-TriageEvery n] [-TriageBatch n] [-AuditDays n] [-ParkedAuditHours n] [-DiscoveryHours n] [-DryRun] # pwsh scripts/Start-IssuePrLoop.ps1 -Report [-ReportHours n] @@ -80,6 +83,9 @@ param( # Upper bound for a repeatedly deferred item's backoff. [ValidateRange(60, 604800)] [int]$MaxDeferSeconds = 259200, + # Open, unparked loop PRs at which no new issue is started. 0 means no limit. + [ValidateRange(0, 1000)] + [int]$MaxOpenPrs = 8, # Reasoning effort for PRs that look ready to merge, and for all other units. # An empty value keeps the agent's configured default. [string]$LightEffort = 'medium', @@ -261,6 +267,62 @@ function Get-PrFingerprint($Pr) { Get-Hash ((@($Pr.headRefOid, $Pr.isDraft, $Pr.mergeable, ($labels -join ','), $review, $comment) + $checks + $codexReactions) -join '|') } +function ConvertTo-Instant($Value) { + if ($Value -is [datetime]) { return [DateTimeOffset]$Value } + if ($Value) { return [DateTimeOffset]::Parse([string]$Value, [Globalization.CultureInfo]::InvariantCulture) } + return $null +} + +# True when a PR has review feedback that no unit has dispositioned: an unresolved review +# thread (units resolve each thread they disposition), or a changes-requested review newer +# than both the last push and the loop's last reply. Such feedback can be handled while +# checks still run. Conversation comments do not count: a finished review with no +# findings is also a comment, and would send units to PRs with nothing to do. +function Test-HasNewFeedback($Pr, $Activity) { + if (-not $Activity) { return $false } + if ($Activity.UnresolvedThreads -gt 0) { return $true } + $since = ConvertTo-Instant $Activity.HeadCommittedAt + if (-not $since) { return $false } + $requested = [Collections.Generic.List[object]]::new() + foreach ($review in @($Pr.reviews)) { + if (-not $review -or -not $review.submittedAt) { continue } + $at = ConvertTo-Instant $review.submittedAt + # The loop replies to review threads as the PR author, which GitHub records as reviews. + if ($review.author.login -eq $Pr.author.login) { if ($at -gt $since) { $since = $at } } + elseif ($review.state -eq 'CHANGES_REQUESTED') { $requested.Add($at) } + } + foreach ($comment in @($Pr.comments)) { + if ($comment -and $comment.createdAt -and ([string]$comment.body).Contains($loopMarker)) { + $at = ConvertTo-Instant $comment.createdAt + if ($at -gt $since) { $since = $at } + } + } + @($requested | Where-Object { $_ -gt $since }).Count -gt 0 +} + +# Returns PR number -> head commit time and unresolved review thread count for open PRs. +# `gh pr list --json commits` asks for every commit's authors and exceeds GitHub's +# GraphQL node limit, so this asks only for the head commit. +function Get-PrActivity { + $query = 'query($owner: String!, $name: String!, $cursor: String) { repository(owner: $owner, name: $name) { ' + + 'pullRequests(states: OPEN, first: 100, after: $cursor) { pageInfo { hasNextPage endCursor } ' + + 'nodes { number commits(last: 1) { nodes { commit { committedDate } } } reviewThreads(first: 100) { nodes { isResolved } } } } } }' + $activity = @{} + $cursor = $null + do { + $variables = if ($cursor) { @('-f', "cursor=$cursor") } else { @() } + $page = (Invoke-GraphQl $query $variables).pullRequests + foreach ($node in @($page.nodes)) { + $activity[[int]$node.number] = [pscustomobject]@{ + HeadCommittedAt = @($node.commits.nodes)[0].commit.committedDate + UnresolvedThreads = @($node.reviewThreads.nodes | Where-Object { -not $_.isResolved }).Count + } + } + $cursor = $page.pageInfo.endCursor + } while ($page.pageInfo.hasNextPage) + return $activity +} + function Get-IssueFingerprint($Issue) { $labels = @($Issue.labels.nodes | ForEach-Object { $_.name } | Sort-Object) $assignees = @($Issue.assignees.nodes | ForEach-Object { $_.login } | Sort-Object) @@ -327,7 +389,7 @@ $deferCategories = @('human', 'upstream', 'credentials', 'locked') $surveyStats = [ordered]@{} function Reset-SurveyStats { - foreach ($key in 'issues', 'issue-deferred', 'issue-locked', 'issue-excluded', 'issue-parent', 'issue-blocked', 'issue-has-pr', 'issue-claimed', + foreach ($key in 'issues', 'issue-wip', 'issue-deferred', 'issue-locked', 'issue-excluded', 'issue-parent', 'issue-blocked', 'issue-has-pr', 'issue-claimed', 'prs', 'pr-deferred', 'pr-locked', 'pr-pending', 'pr-excluded', 'pr-draft') { $surveyStats[$key] = 0 } @@ -335,10 +397,10 @@ function Reset-SurveyStats { function Format-SurveyStats { $s = $surveyStats - ('issues: {0} open, {1} deferred, {2} locked, {3} excluded by label/assignee/bot, {4} with open sub-issues, {5} blocked, {6} with open PR, {7} claimed; ' + - 'PRs: {8} open, {9} deferred, {10} locked, {11} pending checks, {12} needs-human/on-hold, {13} draft') -f ` + ('issues: {0} open, {1} deferred, {2} locked, {3} excluded by label/assignee/bot, {4} with open sub-issues, {5} blocked, {6} with open PR, {7} claimed, ' + + '{14} held by the open-PR limit; PRs: {8} open, {9} deferred, {10} locked, {11} pending checks, {12} needs-human/on-hold, {13} draft') -f ` $s['issues'], $s['issue-deferred'], $s['issue-locked'], $s['issue-excluded'], $s['issue-parent'], $s['issue-blocked'], $s['issue-has-pr'], $s['issue-claimed'], - $s['prs'], $s['pr-deferred'], $s['pr-locked'], $s['pr-pending'], $s['pr-excluded'], $s['pr-draft'] + $s['prs'], $s['pr-deferred'], $s['pr-locked'], $s['pr-pending'], $s['pr-excluded'], $s['pr-draft'], $s['issue-wip'] } function Invoke-GraphQl([string]$Query, [string[]]$Variables) { @@ -487,21 +549,30 @@ function Get-Candidates { $prs = gh pr list --repo $repoSlug --author '@me' --state open --limit 500 --json $prFields | ConvertFrom-Json if ($LASTEXITCODE -ne 0) { throw 'gh pr list failed.' } $surveyStats['prs'] = @($prs).Count + $inFlight = 0 + $prActivity = if (@($prs).Count -gt 0) { Get-PrActivity } else { @{} } $ranked = foreach ($pr in $prs) { if ($pr.isDraft) { $surveyStats['pr-draft']++ } if (@($pr.labels | Where-Object { $_.name -in $excludedPrLabels }).Count -gt 0) { $surveyStats['pr-excluded']++; continue } + $inFlight++ $checks = @($pr.statusCheckRollup) $pending = @($checks | Where-Object { (Get-CheckState $_) -in 'PENDING', 'EXPECTED', 'QUEUED', 'IN_PROGRESS', 'WAITING', 'REQUESTED', '' }) $failed = @($checks | Where-Object { (Get-CheckState $_) -in 'FAILURE', 'ERROR', 'TIMED_OUT', 'CANCELLED', 'ACTION_REQUIRED', 'STARTUP_FAILURE' }) $conflicting = $pr.mergeable -eq 'CONFLICTING' - # Running checks leave nothing to do yet; the next survey picks the PR up. - if ($pending.Count -gt 0 -and -not $conflicting) { $surveyStats['pr-pending']++; continue } - $rank = if (-not $pr.isDraft -and $pr.mergeStateStatus -eq 'CLEAN' -and $failed.Count -eq 0) { 0 } + $feedback = Test-HasNewFeedback $pr $prActivity[[int]$pr.number] + # Running checks leave nothing to do yet, unless the PR conflicts or has new review + # feedback: finishing open PRs beats starting new ones. The next survey picks it up. + if ($pending.Count -gt 0 -and -not $conflicting -and -not $feedback) { $surveyStats['pr-pending']++; continue } + $rank = if (-not $pr.isDraft -and $pr.mergeStateStatus -eq 'CLEAN' -and $failed.Count -eq 0 -and -not $feedback) { 0 } elseif ($conflicting) { 1 } - elseif ($failed.Count -gt 0) { 2 } + elseif ($failed.Count -gt 0 -or $feedback) { 2 } else { 3 } + $hint = if ($feedback -and $pending.Count -gt 0) { + "PR #$($pr.number) has review feedback without a disposition while checks are still running. Handle the feedback now instead of waiting for the checks." + } + else { '' } # A green, clean PR usually needs only the merge gate or a disposition reply. - New-Candidate 'pr' ([int]$pr.number) $rank (Get-PrFingerprint $pr) ($rank -eq 0) + New-Candidate 'pr' ([int]$pr.number) $rank (Get-PrFingerprint $pr) ($rank -eq 0) $hint } foreach ($c in @($ranked | Sort-Object Rank, Number)) { $candidates.Add($c) } @@ -510,6 +581,9 @@ function Get-Candidates { $openIssues = @{} foreach ($issue in $issues) { $openIssues[[int]$issue.number] = $issue } $script:lastIssues = $issues + # Every open PR is another chance of a conflict, so finish existing work before + # starting more once enough PRs are in flight. + $atPrLimit = $MaxOpenPrs -gt 0 -and $inFlight -ge $MaxOpenPrs $stranded = Get-StrandedBranches $openIssues foreach ($number in @($stranded.Keys | Sort-Object)) { $s = $stranded[$number] @@ -561,6 +635,7 @@ function Get-Candidates { New-Candidate 'issue' $number 5 $fingerprint $true $hint continue } + if ($atPrLimit) { $surveyStats['issue-wip']++; continue } $candidate = New-Candidate 'issue' $number 6 $fingerprint $false $candidate.Blocking = [int]$issue.issueDependenciesSummary.blocking $candidate.Comments = [int]$issue.comments.totalCount diff --git a/scripts/Test-IssuePrLoop.ps1 b/scripts/Test-IssuePrLoop.ps1 index 8bb5f2b8b0..4cc54b45d8 100755 --- a/scripts/Test-IssuePrLoop.ps1 +++ b/scripts/Test-IssuePrLoop.ps1 @@ -7,7 +7,8 @@ $ast = [Management.Automation.Language.Parser]::ParseFile( (Join-Path $PSScriptRoot 'Start-IssuePrLoop.ps1'), [ref]$null, [ref]$parseErrors) if ($parseErrors.Count) { throw ($parseErrors -join "`n") } foreach ($name in 'Get-Hash', 'Get-CheckState', 'Get-LatestOutsideComment', 'Get-PrFingerprint', - 'Reset-SurveyStats', 'New-Candidate', 'Get-Candidates', 'Get-IssueFingerprint', 'Test-HasLabel', 'Test-HasOpenPr', 'Get-IssuePriority') { + 'Reset-SurveyStats', 'New-Candidate', 'Get-Candidates', 'Get-IssueFingerprint', 'Test-HasLabel', 'Test-HasOpenPr', 'Get-IssuePriority', + 'ConvertTo-Instant', 'Test-HasNewFeedback') { $definition = $ast.Find({ param($node) $node -is [Management.Automation.Language.FunctionDefinitionAst] -and $node.Name -eq $name }, $false) @@ -24,6 +25,8 @@ function Assert-Equal($Actual, $Expected, [string]$Message) { function Get-MainCandidate { } function Get-OpenIssues { } function Get-StrandedBranches { @{} } +$script:activity = @{} +function Get-PrActivity { $script:activity } function gh { $global:LASTEXITCODE = 0; ConvertTo-Json -InputObject $script:prs -Depth 20 } $repoSlug = 'test/repo' @@ -66,6 +69,35 @@ foreach ($label in $excludedPrLabels) { } $draft.labels = @() +# Review feedback without a disposition is actionable while checks still run. +$draft.statusCheckRollup = @(@{ name = 'claude-review'; status = 'IN_PROGRESS' }) +$script:activity = @{ 1 = [pscustomobject]@{ HeadCommittedAt = '2026-10-24T10:00:00Z'; UnresolvedThreads = 1 } } +Reset-SurveyStats +$feedbackCandidate = @(Get-Candidates) | Where-Object Number -eq 1 +Assert-Equal $feedbackCandidate.Rank 2 'An unresolved review thread must make a pending PR actionable' +Assert-Equal ($feedbackCandidate.Hint -like '*review feedback*') $true 'A pending PR with feedback must say why it was chosen' +$script:activity[1].UnresolvedThreads = 0 +$draft.reviews = @(@{ author = @{ login = 'reviewer' }; state = 'CHANGES_REQUESTED'; submittedAt = '2026-10-24T11:00:00Z' }) +Reset-SurveyStats +Assert-Equal @((Get-Candidates).Number | Sort-Object) @(1, 2) 'A changes-requested review after the last push is feedback' +$draft.reviews += @{ author = @{ login = 'author' }; state = 'COMMENTED'; submittedAt = '2026-10-24T11:30:00Z' } +Reset-SurveyStats +Assert-Equal @((Get-Candidates).Number) @(2) 'Feedback the loop already answered must not wake a pending PR' +$draft.reviews = @(@{ author = @{ login = 'reviewer' }; state = 'CHANGES_REQUESTED'; submittedAt = '2026-10-24T09:00:00Z' }) +Reset-SurveyStats +Assert-Equal @((Get-Candidates).Number) @(2) 'Feedback older than the last push must not wake a pending PR' +$draft.reviews = @(@{ author = @{ login = 'reviewer' }; state = 'COMMENTED'; submittedAt = '2026-10-24T11:00:00Z' }) +$draft.comments = @(@{ body = 'Review: no blocking issues found.'; createdAt = '2026-10-24T11:00:00Z' }) +Reset-SurveyStats +Assert-Equal @((Get-Candidates).Number) @(2) 'Comments and comment-only reviews without open threads must not wake a pending PR' +$script:activity[2] = [pscustomobject]@{ HeadCommittedAt = '2026-10-24T10:00:00Z'; UnresolvedThreads = 2 } +Reset-SurveyStats +Assert-Equal (@(Get-Candidates) | Where-Object Number -eq 2).Light $false 'A green PR with open threads needs full effort, not the merge gate' +$script:activity = @{} +$draft.comments = @() +$draft.reviews = @() +$draft.statusCheckRollup = @(@{ name = 'claude-review'; conclusion = 'FAILURE' }) + $before = Get-PrFingerprint $draft $draft.isDraft = $false Assert-Equal ((Get-PrFingerprint $draft) -ne $before) $true 'Marking ready must invalidate a deferral' @@ -111,4 +143,18 @@ function Get-OpenIssues { $script:issues } Reset-SurveyStats Assert-Equal @((Get-Candidates).Number) @(15, 14, 13, 11, 12, 10, 16) 'Issues must be taken by priority tier, ci first within a tier, unlabelled as medium' +# At the open-PR limit, existing PRs and claims still run but no new issue starts. +$MaxOpenPrs = 2 +$parked = $draft.PSObject.Copy(); $parked.number = 3; $parked.labels = @(@{ name = 'on-hold' }) +$script:prs = @($draft, $parked) +Reset-SurveyStats +Assert-Equal @((Get-Candidates).Number) @(1, 15, 14, 13, 11, 12, 10, 16) 'Parked PRs must not count toward the open-PR limit' +$script:prs = @($draft, $ready) +Reset-SurveyStats +Assert-Equal @((Get-Candidates).Number) @(2, 1, 16) 'At the open-PR limit only existing work may be queued' +Assert-Equal $surveyStats['issue-wip'] 6 'Issues held by the open-PR limit must be counted' +$MaxOpenPrs = 0 +Reset-SurveyStats +Assert-Equal @((Get-Candidates).Count) 9 'A zero open-PR limit must not hold issues' + Write-Host 'Issue/PR loop survey and fingerprint tests passed.'