Auto-apply s/agent-ready-for-rerun from the PR Review Queue workflow - #36358
Auto-apply s/agent-ready-for-rerun from the PR Review Queue workflow#36358kubaflo wants to merge 21 commits into
Conversation
The daily PR Review Queue workflow now autonomously detects PRs that make sense to re-review and applies the s/agent-ready-for-rerun label, which the hourly rerun-review-scanner already consumes. This gives the queue path the same deterministic signal as a maintainer's /review rerun command, without requiring a human comment. - Add Resolve-AutonomousRerunEligibility to Resolve-RerunEligibility.ps1: the AI-free, comment-independent counterpart of Resolve-RerunEligibility. A PR qualifies only when it already has a MauiBot AI Summary AND has a new commit, a new non-command author comment, or a head SHA differing from the last reviewed SHA since that summary. Never-reviewed PRs do not qualify. - Add Query-AutoRerunCandidates.ps1: enumerates open non-draft PRs, classifies each, and applies the label (Ensure-LabelExists + Add-Label). Honours a stale s/agent-review-in-progress lock and supports -DryRun. - Wire into pr-review-queue.yml: a real-apply step in generate-report and a -DryRun step in the PR validate job; extend PR trigger paths. - Add Pester coverage for the new autonomous eligibility function. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36358Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36358" |
There was a problem hiding this comment.
Pull request overview
This PR closes the loop between the daily PR Review Queue workflow and the hourly rerun-review-scanner by autonomously applying s/agent-ready-for-rerun to PRs that have deterministic new PR-author activity since their last MauiBot AI Summary (without requiring a maintainer to post /review rerun).
Changes:
- Adds
Resolve-AutonomousRerunEligibilityto reuse the existing deterministic rerun signal without requiring a/review reruncomment. - Introduces
Query-AutoRerunCandidates.ps1to scan open, non-draft PRs and applys/agent-ready-for-rerun(with-DryRunsupport). - Extends
pr-review-queue.ymlto run the auto-labeler in the scheduled workflow, plus a PR-triggered dry-run validation, and adds 8 new Pester tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| .github/workflows/pr-review-queue.yml | Runs the new auto-rerun labeler (real apply on schedule, dry-run on PR validation) and updates PR trigger paths. |
| .github/scripts/Resolve-RerunEligibility.Tests.ps1 | Adds Pester coverage for the new autonomous eligibility function. |
| .github/scripts/Resolve-RerunEligibility.ps1 | Adds Resolve-AutonomousRerunEligibility to decide rerun readiness without a /review rerun comment. |
| .github/scripts/Query-AutoRerunCandidates.ps1 | New driver script that enumerates open PRs, evaluates eligibility, and applies s/agent-ready-for-rerun. |
The live workflow run proved that adding s/agent-ready-for-rerun to a PR returns HTTP 403 (Resource not accessible by integration) with only pull-requests: read — a label add is a write to the PR resource. Bump the generate-report job to pull-requests: write, matching review-trigger.yml's mark-rerun-ready job that applies the same label. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The PR Review Queue can now apply s/agent-ready-for-rerun autonomously (no /review rerun comment). Such labels carry rerunCommentId=0, which the scanner previously skipped: - Invoke-RerunReviewTrigger.ps1 threw for trigger decisions with rerunCommentId<=0; relaxed to allow comment-less triggers. - rerun-review-scanner.md instructed the agent to skip missing rerun comment ids; updated so a missing id is not a skip reason and both label sources (manual and autonomous) are treated identically. The downstream review-trigger.yml dispatch needs no comment, and the rocket reaction no-ops for id<=0, so comment-less triggers are safe. Deterministic eligibility still gates re-entry (new activity since the latest AI Summary), so autonomous reruns cannot loop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial multi-model code review
Three reviewers evaluated the full 8-file change independently; the findings below are cross-validated by adversarial consensus and each factual claim was re-verified against the code. This is a round-2 re-review — the round-1 headline (the trigger path rejecting an autonomously-labeled PR that carries no /review rerun comment) is now fixed by a2fc3e91d8. CI status is out of scope here.
What it does: a new deterministic driver (Query-AutoRerunCandidates.ps1) applies s/agent-ready-for-rerun to open PRs that have a MauiBot AI Summary plus new author activity since it; the consumer is relaxed so a comment-less (autonomous) label no longer rejects the trigger. The trigger path now closes correctly.
Consensus findings
Resolve-RerunEligibility.ps1. The PR's "no cap needed" safety argument only holds on the trigger branch. When the scanner agent decides skip on activity the deterministic gate considered eligible, it strips the label without advancing the checkpoint, so the daily queue re-labels the same unchanged state indefinitely — perpetual churn, review never runs. (Reviewer 3 initially reported "no flap", but on inspection it had traced only the trigger path, not the scanner's skip branch.)
created_at throws and aborts the scan (2/3). See inline comment on Query-AutoRerunCandidates.ps1.
$Owner/$Repo to dotnet/maui (3/3). Already flagged by the Copilot reviewer inline at Query-AutoRerunCandidates.ps1:56 and still open — confirming it is real: one reviewer reproduced it (acme/widget → dotnet/maui), and the sibling Query-RerunReadyPRs.ps1:18 shows the correct pattern (capture the values before dot-sourcing). Benign today (the workflow passes dotnet/maui and is owner-gated), but the -Owner/-Repo params are effectively dead.
Secondary observations
-Limit 100vs 191 open PRs (1 reviewer this round; count verified today): the daily scan can only evaluate 100 PRs, so ~91 open PRs are never considered per run — a long-lived PR with fresh activity that sits beyond the first 100 bygh's default ordering would be missed.- The Copilot reviewer's
validate-jobissues: readcomments (pr-review-queue.yml:167/169) are corroborated —rerun-review-scanner's own workflow explicitly grantsissues: read, so the label/comment/event reads likely need it. Worth confirming before merge.
Prior review reconciliation (Copilot PR reviewer, 2 rounds)
| Prior finding | Status |
|---|---|
Dot-source $Owner/$Repo clobber (:56) |
Unresolved — confirmed 3/3 above |
Get-IssueLabels stderr-suppressed, no exit check (:62) |
Open — subsumed by the error-isolation finding |
validate job missing issues: read (:167/:169) |
Open — corroborated |
Label description mismatch (:52/:53) |
Pre-existing pattern — minor |
Test name says "skips" but asserts eligible (Tests:589) |
Minor naming clarity |
Cleared — checked, not issues (3/3 agree)
- Permission bump
pull-requests: read → writeis not an escalation: the write job is gatedif: github.repository_owner == 'dotnet' && github.event_name != 'pull_request', and the workflow triggers onpull_request(notpull_request_target), so a fork PR can only reach the read-only-DryRunvalidatejob. - gh-aw lock recompile is consistent:
frontmatter_hashunchanged, onlybody_hashbumped (a prompt-body-only edit); the react-guard JS is unchanged. - Comment-less trigger relaxation is correct end-to-end:
rerunCommentIdnull→0, thereact()guard no-ops on id ≤ 0,review-trigger.ymlneeds no comment; the inverted Pester test matches.
Verdict: NEEDS_CHANGES · confidence: medium
No blocking security issue, and the round-1 headline is fixed. But the skip-path flap will cause perpetual daily label churn (with no review) for a plausible class of PRs — a trivial comment or commit the agent declines — and the missing per-PR isolation lets one malformed PR silently truncate the whole daily scan. Both are fixable without redesign.
3 independent reviewers, adversarial consensus. Informational — not a substitute for human review, and not an approval.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/scripts/Query-AutoRerunCandidates.ps1:256
- When Add-Label fails and the follow-up label verification also can’t confirm the label, the script currently only logs a warning and records the decision as non-error. That can produce a successful exit code and a decision summary that looks healthy even though labels were not applied (e.g., permissions regressions), making scheduled runs harder to diagnose.
Consider throwing here so the per-PR try/catch records an error: decision and the scan can fail when failures become systemic (while still continuing to evaluate other PRs).
} else {
Write-Host " ⚠️ Failed to apply $ReadyForRerunLabel to #$number" -ForegroundColor Yellow
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/scripts/Query-AutoRerunCandidates.Tests.ps1:228
- This ordering test can pass even if one of the expected substrings is missing:
IndexOf(...)returns-1, and-1is still “less than” any valid index. That means a regression inrerun-review-scanner.mdcould slip through while this test remains green. Capture the indices, assert they’re all found (>-1), then compare the ordering.
$scanner.IndexOf('await markDeclined(prNumber);') |
Should -BeLessThan $scanner.IndexOf("await react(a.rerunCommentId, '-1');")
$scanner.IndexOf("await react(a.rerunCommentId, '-1');") |
Should -BeLessThan $scanner.IndexOf('await removeReadyLabel(prNumber);')
$scanner.IndexOf('await clearDeclined(prNumber);') |
.github/scripts/Resolve-RerunEligibility.ps1:698
- The decline-gated
new-head-commitpath usesTest-HasCommitAfter(commit author/committer timestamps) as a proxy for “fresh push after decline”. This misses force-push/rewind scenarios where the PR head SHA changes after a decline but all commits have timestamps before the decline checkpoint. In that case, the head really is new activity, but this logic will returndeclined-state-unchangeduntil the author comments.
Consider basing the post-decline head-change detection on a push/timeline event timestamp (or another server-side activity signal) rather than commit dates, so a force-push after a decline can re-qualify without requiring an extra comment.
# A head SHA that differs from the last-reviewed SHA only re-qualifies when it is
# backed by a commit that landed after the checkpoint. Absent a decline this is
# always true (the differing head IS that post-summary push), so behaviour is
# unchanged; once a decline advances the checkpoint, a head that merely still
# differs from the summary's SHA (the exact state the scanner declined) no longer
# counts — only a fresh push after the decline does.
if ($headDiffers -and (-not $isDeclineGated -or $hasNewCommit)) {
return [pscustomobject]@{ Eligible = $true; Reason = 'new-head-commit'; Label = $ReadyForRerunLabel }
|
@MauiBot Final head |
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial re-review of the current head found two additional lifecycle gaps in the new explicit decline-marker flow. Existing unresolved feedback is not repeated here.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
@PureWeen Ready for re-review at |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/scripts/Query-AutoRerunCandidates.ps1:149
Invoke-AutoRerunCandidateScanassigns$Owner/$Repo/$Limit/...without an explicit scope. In PowerShell this creates local variables, so helper functions likeGet-ActivityForPR/Get-CommitsForPR(which read the script-scope$Owner/$Repo) will still query the original repo if-ScanOwner/-ScanRepoare ever passed. This makes the override parameters ineffective and can mix PR listing from one repo with activity reads from another.
$Owner = $ScanOwner
$Repo = $ScanRepo
$Limit = $ScanLimit
$DryRun = $ScanDryRun
$OutputPath = $ScanOutputPath
|
Triaged Copilot review |
PureWeen
left a comment
There was a problem hiding this comment.
Adversarial review
Test coverage: The helper return values and cleanup ordering are covered, but the workflow call site does not test the failed-cleanup behavior described inline.
Prior review status: Both findings from review 4898600507 are addressed. Earlier unresolved feedback outside this delta is not duplicated here.
Methodology: 3 independent reviewers with adversarial consensus; finding confirmed by 2/3 reviewers.
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 4 findings
See inline comments for details.
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@kubaflo — new AI review results are available based on commit
3a3c1ab.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ⚠️ SKIPPED
No tests were detected in this PR.
Recommendation: Add tests to verify the fix using the write-tests-agent.
📋 Pre-Flight — Context & Validation
PR #36358 Pre-Flight
Context
- Title: Auto-apply
s/agent-ready-for-rerunfrom the PR Review Queue workflow - PR head:
3a3c1abc1a5c8cdd866902d335e45e74b87a737c - Review commit:
063a68ddca93a68f5f6c5543898be475774e269d - Target:
main - Platform requested for STEP 5a: Android. The patch is infrastructure-only and has no platform-specific runtime path.
- Gate: Skipped because the generic detector found no product tests. The PR does add or update four focused Pester files.
Problem
The daily PR Review Queue should autonomously re-queue an already AI-reviewed PR only after genuine PR-author activity, without requiring a maintainer /review rerun comment. The hourly scanner must accept these comment-less queue entries, avoid repeatedly re-queuing a state that its semantic pass already declined, and retain existing concurrency/lock safety.
Existing PR approach
The committed PR diff was inspected directly (git diff HEAD^ HEAD): 12 files, 1,255 additions and 21 deletions.
- Adds
Query-AutoRerunCandidates.ps1, which lists a bounded set of open PRs, fetches comments/reviews/review comments/commits, delegates the deterministic decision toResolve-AutonomousRerunEligibility, and appliess/agent-ready-for-rerun. - Adds a second eligibility entrypoint in
Resolve-RerunEligibility.ps1. It requires a prior MauiBot AI Summary and recognizes a new author comment, a new commit, or a head SHA different from the reviewed SHA. - Changes
Invoke-RerunReviewTrigger.ps1to permitrerunCommentId = 0. - Persists semantic scanner skips with
s/agent-rerun-declinedplus a minimized bot comment containing the declined head SHA. Later scans use that label/comment as an anti-flap checkpoint; review entrypoints clear the label before dispatch/locking. - Gives the queue workflow pull-request write permission, runs the labeler after queue issue creation, and adds a bounded dry-run validation job.
The main alternative seam is architectural: the PR distributes queue state across a ready label, a decline label, a marker comment, the queue labeler, and the scanner safe-output workflow. Any candidate must use a different root-cause/coordination strategy rather than cosmetically moving the same checks.
Candidate scope
Primary files:
.github/scripts/Query-AutoRerunCandidates.ps1.github/scripts/Resolve-RerunEligibility.ps1.github/scripts/Invoke-RerunReviewTrigger.ps1.github/scripts/shared/Update-AgentLabels.ps1.github/workflows/pr-review-queue.yml.github/workflows/rerun-review-scanner.md.github/workflows/rerun-review-scanner.lock.yml.github/workflows/review-trigger.yml
Focused validation only:
pwsh -NoProfile -Command "Invoke-Pester -Path @('.github/scripts/Query-AutoRerunCandidates.Tests.ps1', '.github/scripts/Resolve-RerunEligibility.Tests.ps1', '.github/scripts/Invoke-RerunReviewTrigger.Tests.ps1', '.github/scripts/shared/Update-AgentLabels.Tests.ps1') -CI"No full test suite and no gate re-run are permitted.
Environment constraint
The checkout contains pre-existing tracked trusted-script overlays and untracked trusted helper files. They are not candidate changes and must not be discarded. EstablishBrokenBaseline.ps1 rejects a dirty tracked worktree before baseline creation and also classifies all .github/* paths as excluded test/infrastructure paths. A try-fix invocation must follow the skill exactly; if its mandatory baseline step blocks, it must record Blocked, restore if applicable, preserve all existing worktree changes, and return without using manual git checkout, git restore, or git reset.
🔬 Code Review — Deep Analysis
Code Review — PR #36358
Independent Assessment
What this changes: The PR adds a daily, deterministic scan that labels previously AI-reviewed PRs for another review when the author has added qualifying activity. It also allows the hourly scanner to dispatch comment-less reruns and adds a decline label plus bot-authored head-SHA marker to prevent an unchanged state from being queued repeatedly.
Inferred motivation: Close the loop between queue discovery and the existing rerun scanner without requiring a maintainer to post /review rerun, while retaining stale-lock, head-race, and anti-loop protections.
Approach assessment: The core state machine is coherent: only trusted AI summaries establish review checkpoints, bot-authored markers record semantic declines, a live-head comparison prevents stale scanner decisions from consuming newer work, and a new AI summary advances the checkpoint after a successful rerun. The submitted implementation nevertheless has one concrete availability regression and three operational/security concerns that should be addressed before merge.
External Output Contract
| Consumer token/pattern | Producer location | Producer emission condition | Consumer assumption | Ordinary negative case | Downstream effect |
|---|---|---|---|---|---|
<!-- agent-rerun-declined:<40-hex-sha> --> |
rerun-review-scanner.md, markDeclined |
The scanner semantically skips a candidate whose live head still equals the scanned head | A trusted marker is the latest declined checkpoint | A partial failure leaves the ready label present after the marker was already posted | The next hourly scan can post the same marker again before another removal attempt |
s/agent-rerun-declined |
Scanner safe-output job and Update-AgentLabels.ps1 |
A semantic skip is persisted for the current state | Clearing the advisory label is required before every review lock can be acquired | The label disappears between the earlier label snapshot and the DELETE request | GitHub returns 404 and the submitted throw aborts an otherwise valid review |
AI Summary plus SESSION:<sha> START |
MauiBot AI-summary issue comment, consumed by Resolve-RerunEligibility.ps1 |
A completed AI review posts a trusted summary, optionally with a reviewed SHA | Only a trusted prior review can make a PR autonomously eligible | A summary has no session SHA | The resolver fails closed for SHA-only evidence and does not create a rerun loop |
Prior Review Reconciliation
Prior top-level reviews, inline comments, and issue comments could not be queried because the GitHub CLI has no authentication in this environment. No prior finding is treated as resolved on that basis.
Blast Radius Assessment
- Runs for all instances: The scheduled scan considers up to 300 open PRs and the shared review-trigger change affects every review entrypoint.
- Startup impact: No product startup impact; this is repository-wide review infrastructure.
- Static/shared state: GitHub labels and minimized comments are durable shared workflow state across scheduled and manual runs.
- Trust boundary: The scheduled job receives repository write permissions and processes repository-wide PR activity, so least privilege and bounded API usage are required.
CI Status
- Required-check result: Undetermined;
gh pr checks --requiredcould not authenticate. - Gate result: Skipped because the generic detector reported no tests, although the submitted diff contains focused Pester tests.
- Classification: Validation coverage is undetermined.
- Action taken: Confidence is capped at low; the gate was not rerun.
Findings
❌ Error — Advisory-label cleanup can abort every review entrypoint
.github/workflows/review-trigger.yml:267 throws when Clear-AgentRerunDeclined returns false. The helper ultimately treats every nonzero DELETE result as failure, including GitHub's normal 404 when the label was concurrently removed. This makes a benign race or transient label-cleanup failure abort /review before the in-progress lock is acquired. Decline cleanup is anti-flap bookkeeping and should warn rather than block review dispatch, or make absence explicitly idempotent.
⚠️ Warning — Scheduled job requests unnecessary pull-request write access
.github/workflows/pr-review-queue.yml:36 grants pull-requests: write, while the implementation mutates labels through the Issues API and already grants issues: write; its Pulls API usage is read-only. The rationale in the workflow does not match the submitted call path and broadens the authority of a repository-wide scheduled job.
⚠️ Warning — Decline marker creation is not idempotent under partial failure
.github/workflows/rerun-review-scanner.md:312 posts the marker before consuming the ready label. If label mutation or ready-label removal fails, the ready label remains and the next hourly scan can post another marker for the same head. Marker persistence should detect an existing trusted marker for that head before creating another comment.
⚠️ Warning — Daily scan has high per-PR API fan-out
.github/scripts/Query-AutoRerunCandidates.ps1:89 performs four paginated history reads per non-draft PR, plus event history for in-progress locks, across a default ceiling of 300. The scan can consume a large portion of the repository token budget; once requests fail, decisions become errors and the workflow intentionally reduces the failed scan to a warning. Cheaply reject never-reviewed PRs before fetching reviews, review comments, and commits, or otherwise narrow the expensive candidate set.
Failure-Mode Probing
- Concurrent decline-label removal: The stale label snapshot still enters cleanup; DELETE returns 404; the submitted workflow throws and no review lock is acquired.
- Marker succeeds but ready-label removal fails: The ready label remains; the next scan reprocesses the PR and unconditionally creates another marker for the same SHA.
- Never-reviewed PR: The current scan still downloads all four paginated histories before the resolver rejects it, multiplying cost for the common negative case.
- Head advances while a skip is being persisted: The live-head guard leaves the ready label in place, so the new state is reconsidered; this path is sound.
- Comment-less autonomous candidate:
rerunCommentId = 0reaches dispatch, while reaction handling no-ops for nonpositive IDs; this path is sound.
Verdict: NEEDS_CHANGES
Confidence: low
Summary: The anti-loop and comment-less rerun design is fundamentally sound, but the new hard failure on advisory decline-label cleanup creates a concrete review-availability regression. The permission, marker-idempotency, and API-fan-out findings also merit a consolidated correction; infrastructure-wide blast radius and unavailable required-check evidence cap confidence.
🛠️ Try-Fix — Analysis & Comparison
STEP 5a Try-Fix Aggregate
Candidate 1 — Stateless AI-summary checkpoint
- Model:
claude-opus-5 - Result:
Blocked - Findings: 0
- Candidate record:
../try-fix-1/content.md
The candidate proposed reducing state rather than extending the PR's label/comment protocol:
- Reuse
Resolve-RerunEligibilitywith a command-independent mode instead of addingResolve-AutonomousRerunEligibility. - Store the last semantic scanner outcome and head SHA as a machine-readable footer in the existing MauiBot AI Summary, replacing both
s/agent-rerun-declinedand the extra minimized marker comment. - Reconcile against that checkpoint and model a missing rerun comment as nullable rather than using
0. - Avoid the new standalone candidate-query script and queue-job permission expansion.
No implementation or test ran. The mandatory baseline script rejected the 41 pre-existing tracked trusted-script overlay changes as a dirty worktree. Those changes could not safely be discarded. Even in a clean checkout, the baseline detector currently excludes every .github/* path, while this PR changes only .github/*; the skill requires that condition to be recorded as Blocked. The attempt preserved the existing worktree and produced an empty diff.
Candidate 2 — Event-sourced ready-label transition checkpoint
- Model:
gpt-5.6-sol - Result:
Blocked - Findings: 0
- Candidate record:
../try-fix-2/content.md
This candidate used a distinct root-cause hypothesis: autonomous eligibility forgets the last queue attempt because it compares activity only with the preceding AI review. It proposed treating the existing s/agent-ready-for-rerun label's immutable timeline as the attempt ledger. Eligibility would require genuine PR-author activity after both the AI review and the latest removal of the ready label. A scanner semantic skip already removes that label, so its removal becomes the durable high-water mark; later author activity permits re-entry.
Unlike the PR, this adds neither a decline label nor a marker comment. Unlike Candidate 1, it does not mutate the AI Summary. Comment-less candidates remain dispatchable and existing lock/concurrency behavior remains unchanged.
No implementation or test ran. The mandatory baseline script again rejected the pre-existing tracked trusted-script overlay before creating baseline state. Manual cleanup was prohibited, and the independent .github/* fix-file exclusion would also block this infrastructure-only PR. The mandatory restore command found no baseline state. The attempt preserved the worktree and produced an empty diff.
Aggregate status
Two candidates were considered, satisfying the two-candidate bound. Both are design-level alternatives only and are Blocked; neither has empirical validation or a shippable diff.
🏁 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Winner: pr-plus-reviewer
pr-plus-reviewer is the strongest candidate because it preserves the submitted PR's sound autonomous-rerun state machine while removing the expert review's concrete availability regression: a race or transient failure while clearing an advisory decline label no longer aborts every review request. It also narrows the scheduled job's permission and avoids three paginated feeds plus commit history for the common never-reviewed case.
Comparative ranking
| Rank | Candidate | Implementation | Regression evidence | Assessment |
|---|---|---|---|---|
| 1 | pr-plus-reviewer |
Consolidated patch addressing the blocking cleanup race, least privilege, and API fan-out; adds focused tests | Blocked before test discovery because Pester is unavailable; diff check passed | Best code-level result, but must be submitted and validated in CI |
| 2 | pr |
Complete submitted implementation with coherent checkpoint, TOCTOU, stale-lock, and comment-less dispatch handling | Gate skipped; no trusted regression result from this run | Expert verdict NEEDS_CHANGES because advisory label cleanup can abort valid reviews; also retains permission, fan-out, and marker-idempotency concerns |
| 3 | try-fix-1 |
Design only: consolidate eligibility and store a checkpoint in the AI Summary | Blocked; no implementation and no test run | Potentially lower-state design, but it is not a shippable candidate and mutates the review artifact |
| 4 | try-fix-2 |
Design only: use ready-label removal events as the attempt ledger | Blocked; no implementation and no test run | Interesting event-sourced alternative, but no diff or empirical evidence establishes correctness |
No candidate passed regression tests in this run. The pr-plus-reviewer command was environment-blocked rather than producing failing assertions; both try-fix candidates were blocked before implementation, and the raw PR Gate was skipped. Therefore no failed-regression candidate is ranked above a passing candidate.
Expert findings and disposition
| Finding | Raw pr |
pr-plus-reviewer |
|---|---|---|
| Review aborts when advisory decline-label cleanup returns false | ❌ Unresolved | ✅ Converted to a non-blocking warning |
Scheduled job grants pull-requests: write for Issues-API label calls |
✅ Reduced to read | |
| Never-reviewed PRs incur four paginated history feeds | ✅ Issue comments now provide a cheap deterministic preflight | |
| Repeated skip can duplicate decline-marker comments after partial failure |
Remaining uncertainty
The winner still needs the focused Pester command in an environment with Pester installed, and the permission reduction needs normal workflow execution to confirm the repository token behaves as documented. Marker creation remains non-idempotent under partial mutation failure and should be addressed in a follow-up that regenerates rerun-review-scanner.lock.yml with the pinned gh-aw compiler.
Because the raw submitted PR is not the winner, approval is not recommended until the consolidated winner changes are incorporated.
🧭 Next Steps — reviewer patch required (pr-plus-reviewer)
The reviewer-enhanced candidate won, so the submitted PR still needs those changes.
Why: pr-plus-reviewer preserves the PR's sound rerun state machine while removing the concrete review-aborting cleanup race, reducing scheduled-job privilege, and short-circuiting expensive history reads for never-reviewed PRs. Its focused validation was blocked because Pester is unavailable, but the other alternatives have no implementation and the raw PR retains the blocking error.
Apply PRAgent/pr-plus-reviewer/reviewer.patch from the CopilotLogs
artifact (or follow the report's Required submitted-PR change), push the update, and run
the review again.
There was a problem hiding this comment.
Adversarial review
Test coverage: No test exercises a non-404 clearDeclined failure before dispatch.
Prior review status: Review 4901341869 remains valid at the current head.
Methodology: 3 independent reviewers; the finding was validated by a two-model dispute.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
@PureWeen addressed the latest feedback in 1967d88: cleanup is non-fatal/idempotent, queue permissions are least-privilege, decline markers are idempotent, and candidate scans avoid unnecessary history fan-out. Focused Pester tests (125 passed) and strict gh-aw compilation passed. Ready for re-review — thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/rerun-review-scanner.md:219
ensureDeclinedLabel()creates thes/agent-rerun-declinedlabel on 404, but it doesn’t handle the common race where another workflow/run creates the label between thegetLabelandcreateLabelcalls. In that case GitHub returns HTTP 422 and this safe-output job will fail even though the desired end-state (label exists) is already true.
Handle 422 from createLabel as a benign “already exists” outcome (or re-fetch) so the scanner remains robust under concurrent label creation (e.g., first day after merge, or when other entrypoints also ensure the label).
} catch (e) {
if (e.status !== 404) { throw e; }
await github.rest.issues.createLabel({ owner, repo, ...declinedLabel });
}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
Addressed the suppressed race finding from review #4921411015 in 9657be3. @copilot-pull-request-reviewer please re-review. Ready for re-review. |
Surface unverified label application failures after writing the decision artifact, and prevent scanner reactions from targeting historical rerun comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d00747b7-96f3-4e7a-8dfb-e3a48db04b2d
|
@PureWeen all four previously valid unresolved findings are now implemented: the decline lifecycle fixes are in |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description
The daily PR Review Queue workflow (
pr-review-queue.yml, the one that creates issues like #36323) now autonomously detects PRs that make sense to re-review and applies thes/agent-ready-for-rerunlabel. The existing hourlyrerun-review-scanneralready consumes that label and re-runs the review — so this closes the loop without a maintainer having to type/review rerun.The eligibility decision reuses the exact same deterministic signal as
/review rerun(no AI, no semantic inspection of untrusted text).What changed
Resolve-RerunEligibility.ps1— newResolve-AutonomousRerunEligibilityfunction: the comment-independent counterpart ofResolve-RerunEligibility. A PR qualifies only when it already carries a MauiBot AI Summary and has, since that summary:PRs that were never AI-reviewed do not qualify.
Query-AutoRerunCandidates.ps1(new) — enumerates open, non-draft PRs, classifies each, and appliess/agent-ready-for-rerun(Ensure-LabelExists+Add-Label). Honours a stales/agent-review-in-progresslock (same staleness rule the scanner uses) and supports-DryRun.pr-review-queue.yml— real-apply step in thegenerate-reportjob (failures are surfaced as a warning and never block the already-created queue issue) and a-DryRunstep in the PRvalidatejob; PR trigger paths extended to the touched scripts. Thegenerate-reportjob is bumped topull-requests: write(a label add is a write to the PR resource —pull-requests: readyields HTTP 403), matchingreview-trigger.yml'smark-rerun-readyjob.Tests — 8 new Pester cases in
Resolve-RerunEligibility.Tests.ps1.Loop safety
After a rerun, the scanner triggers a review that posts a new AI Summary; its timestamp advances the checkpoint, so the PR is not re-labelled on the next daily run. No artificial cap needed.
Validation
-DryRunover 60 open PRs: 24 drafts skipped, 25 never-reviewed skipped, 10 no-new-activity skipped, 1 genuine candidate ([net11.0][Android] Fix StackNavigationManager binary break and ShellItemWrapperFragment OnDestroyView gap (follow-up #34758) #36133, reasonnew-head-commit). No labels applied in dry-run.Scanner-side change (closes the full loop)
Live testing revealed the loop was only half-connected: autonomously-applied labels carry no
/review reruncomment (rerunCommentId=0), and the scanner previously skipped those:Invoke-RerunReviewTrigger.ps1threw fortriggerdecisions withrerunCommentId<=0; relaxed to allow comment-less triggers.rerun-review-scanner.md(+ recompiled.lock.yml) instructed the agent to treat a missing rerun comment id as a skip; updated so a missing id is not a skip reason and both label sources (manual/review rerunand autonomous queue) are treated identically.The downstream
review-trigger.ymldispatch needs no comment, and the rocket reaction no-ops forid<=0, so comment-less triggers are safe. Deterministic eligibility still gates re-entry, so autonomous reruns cannot loop.Note: the scheduled scanner runs from
main, so this scanner-side behavior takes effect for scheduled runs only once merged.Update — merged
mainto adopt the PAT pool (#36204)mainmoved agentic workflows to a PAT pool (#36204): the scanner now authenticates the Copilot agent viaimports: shared/pat_pool.md+environment: copilot-pat-pool+engine.env.COPILOT_GITHUB_TOKENfromsecrets.COPILOT_PAT_0..9. This branch predated that, causingNo authentication information foundon dispatch. Mergedorigin/mainin; thererun-review-scanner.mdmerge kept both main's PAT-pool frontmatter and this PR's comment-less-trigger prompt edits, and.lock.ymlwas regenerated withgh aw compile(v0.80.9). Pester still 83/83.Testing note: the
copilot-pat-poolenvironment is restricted to protected branches (protected_branches: true), so the scanner can no longer be exercised viaworkflow_dispatchfrom this feature branch (pre_activation is blocked at job-init). Final end-to-end validation of the scanner-side change must therefore happen after merge, on the scheduled hourly run frommain. The queue labeler half (non-agentic) continues to validate live from the branch.🤖 This PR was authored by an AI agent (GitHub Copilot CLI) on behalf of @kubaflo.