Fix rerun review scanner: dispatch review-trigger.yml instead of reimplementing AzDO

[kubaflo]

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!

Problem

The /review rerun scanner (rerun-review-scanner) dispatched zero AzDO reviews after #35955, and the few it should dispatch would target the wrong branch. Two independent bugs:

Bug 1 — every dispatch aborted with a spurious 404

In the gh-aw safe-output job, the gh CLI returns HTTP 404 for repos/dotnet/maui/pulls/N — even with pull-requests: write on a public repo. #35955 misread these as transient and "hardened" the not-found guard, so it now faithfully confirms the bogus 404 and skips every PR. Evidence it is not a permission/token problem: the pre_activation job (only Metadata: read) reads the same PRs fine, and the built-in safe_outputs job (octokit) succeeds in the same run.

Bug 2 — custom review branches silently downgraded to main

The candidate builder decided whose /review -b <branch> -p <platform> to trust using the comment's author_association. Under the Actions GITHUB_TOKEN, a maintainer whose org membership is private reads as CONTRIBUTOR (not MEMBER), so their command was dropped and the rerun fell back to the main pipeline. A live scan confirmed it dispatched four net11 (feature/enhanced-reviewer) PRs on main.

Fix — do exactly what a maintainer /review does

Bug 1: instead of re-implementing PR validation + OIDC + the AzDO trigger inside the safe job, the scanner now dispatches the same review-trigger.yml workflow /review runs, via workflow_dispatch. That workflow owns PR validation, the s/agent-review-in-progress lock, platform inference, OIDC, and the AzDO trigger. (workflow_dispatch via GITHUB_TOKEN always creates a run — it is exempt from Actions recursion-prevention.)

• Invoke-RerunReviewTrigger.ps1 is now pure: validate batched decisions against candidates.json, emit actions.json. No gh/AzDO/OIDC/lock/rate-limit I/O.
• A github-script (octokit) step performs all GitHub writes — octokit works in the safe-job context where the gh CLI does not. trigger → createWorkflowDispatch(review-trigger.yml,{pr_number,platform,pipeline_ref}) + 👍; skip → 👎 + remove the queue label.
• Permissions: +actions:write, −id-token:write; dropped the AZDO_TRIGGER_* secrets from the job.

Bug 2: authorize /review options by a live collaborator-permission lookup (collaborators/<user>/permission → write/maintain/admin) — the exact call review-trigger.yml's auth step makes. It only needs metadata: read (every token has it) and reflects current access. No new secret or permission.

• Resolve-RerunEligibility.ps1: add Test-ReviewOptionLoginTrusted (cached per login); Get-LatestReviewCommandOptions computes trust through it. Removed the author_association gate.
• Query-RerunReadyPRs.ps1: drop the author_association helpers; pass -Owner/-Repo.

Validation

• 69 Pester tests pass (36 dispatch + 33 resolver), incl. a regression test that an author_association=NONE command is still honored when the login has write access.
• Live, real (non-dry) scan: the safe job validated all decisions with no 404s and octokit performed real createWorkflowDispatch, producing two review-trigger.yml runs that triggered real AzDO maui-copilot builds (HTTP 200, Run IDs 14459427 & 14459428).
• Real-data check for Bug 2: PR [Android][Windows] Fix GraphicsView passing fractional dirtyRect dimensions to IDrawable.Draw #34564's history now resolves to pipelineRef=feature/enhanced-reviewer (author kubaflo, author_association=CONTRIBUTOR) instead of main, using the live permission lookup.

Files

• .github/scripts/Invoke-RerunReviewTrigger.ps1, .github/scripts/Invoke-RerunReviewTrigger.Tests.ps1
• .github/scripts/Resolve-RerunEligibility.ps1, .github/scripts/Resolve-RerunEligibility.Tests.ps1
• .github/scripts/Query-RerunReadyPRs.ps1
• .github/workflows/rerun-review-scanner.md + recompiled .lock.yml
• .github/docs/agent-labels.md

---

🔍 This PR was created by an AI agent (GitHub Copilot CLI) on behalf of @kubaflo.

[github-actions]

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 36080

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36080"

[MauiBot]

Expert Review — 1 findings

See inline comments for details.

[PureWeen]

Multi-model adversarial review — PR #36080 (round 2, commit 6f8aa93)

Re-reviewed after the round-1 fixes. 3 independent reviewers (different model families) re-ran in parallel with adversarial consensus; disputed findings were re-verified against the source. Code-only review (CI out of scope). Comment review — no approval/blocking implied.

Round-1 findings — all resolved ✅

All three reviewers independently confirmed the round-2 commit correctly and completely addresses the prior round:

Round-1 finding	Status	Verification
❌ Batch poisoning (one bad decision drops the whole batch)	✅ Fixed	if: ${{ !cancelled() }} on the dispatch step runs it even when validate exit 1s; actions.json is written before the exit so valid actions persist; the JS no-ops when the file is absent and core.setFaileds on a torn/parse error; the job still goes red so a dropped decision stays visible. .lock.yml faithfully mirrors the .md.
⚠️ Per-PR rate limiter removed	✅ Resolved (documented, intentional)	The new "Rerun volume" subsection matches the code: the deterministic new-activity gate in Resolve-RerunEligibility.ps1 means identical PR state can't be re-queued, and re-entry needs a human /review rerun each cycle. Confirmed the trade-off is sound and matches manual /review.
💡 Transient permission blip → silent main downgrade	✅ Fixed	Test-ReviewOptionLoginTrusted now distinguishes a definitive HTTP 404/410 (cached untrusted) from a transient error (retried 3×, never cached), so a momentary blip can't downgrade a maintainer's custom branch.
💡 $Login path interpolation	✅ Hardened	^[A-Za-z0-9][A-Za-z0-9-]*$ guard rejects bot/invalid logins before the API call; confirmed it rejects no valid GitHub login.

Tests: 75/75 Pester pass (one reviewer ran the suite). The new permission tests are isolated (cache reset in BeforeEach, Get-CollaboratorPermissionResult mocked) and cover trust / no-trust / 404-cache / transient-no-cache / recovery.

---

New finding this round

❌ Skip-path TOCTOU — skip removes the queue label against stale state · .github/workflows/rerun-review-scanner.md:209

3/3 reviewers agree it's a real silent-drop race (corroborating MauiBot's inline ❌); 2 rate it ❌ must-fix, 1 rates it ⚠️ (recoverable).

The validate step's expected_head_sha == candidate.headSha is an anti-hallucination check against the scan-time candidates.json, not a live-head check. So the skip branch can strip s/agent-ready-for-rerun from a PR whose head has already advanced.

• Trigger: scan snapshots PR #X at head abc (label present) → minutes later the author pushes def (label still present, or a fresh /review rerun keeps it) → the agent, blind to def, returns skip → this step validates (abc == abc) and removes the label from the def head. The newly-queued rerun for def is dropped; the 👎 lands on the old rerun comment.
• Asymmetry: the trigger path is immune — it doesn't remove the label; it dispatches review-trigger.yml, which re-validates and reviews the live head. Only skip does a destructive, non-revalidated removal.
• Severity nuance: it's recoverable — one more human /review rerun re-applies the label and the next scan re-evaluates — and the window is narrow (needs skip + concurrent new author activity within the scan→agent→dispatch minutes). That's why one reviewer holds it at ⚠️. Still a genuine silent-drop of a user's request, which is why the majority (and MauiBot) call it ❌.
• Fix (verified feasible): the action object already carries headSha, and this step uses octokit (not the gh CLI the PR says 404s here), so a live read works. Before the skip reaction/removal, github.rest.pulls.get(...) and only removeReadyLabel when livePr.head.sha === a.headSha and the ready label is still present; otherwise log and leave the label. Scope the guard to skip only. (Inline comment added at :209.)

💡 Minor (single-reviewer, non-blocking)

• actions.json not written atomically (Invoke-RerunReviewTrigger.ps1:257). Set-Content writes directly rather than temp-file + rename. Flagged ⚠️ by one reviewer; another explicitly judged it acceptable because a torn/partial write fails JSON.parse → core.setFailed (loud failure, never silently consumed) and runner.temp is per-run so no stale file can be read. Net: not data loss; an atomic temp+rename would be belt-and-suspenders. (1/3)
• Treat 403/401 as definitive-untrusted in Test-ReviewOptionLoginTrusted. Today a persistent 403 falls into the transient branch → 3 pointless retries (~6s) before the (correct) untrusted result. Classifying 401/403 alongside 404/410 would skip the wasted retries. Not expected on this public repo. (1/3)

---

Prior reviews: MauiBot posted the skip-path ❌ (which all 3 reviewers independently corroborate here) plus an AI Summary; the round-1 adversarial review's findings are resolved by 6f8aa93. No unresolved prior ❌ other than the skip-path race addressed above.

Bottom line: the round-1 fixes are solid and the architecture is sound. The one outstanding item is the skip-path label-removal race — worth guarding with a live head-SHA check (cheap, feasible, skip-only) before merge.

Methodology: 3 independent reviewers with adversarial consensus; severity split and the proposed fix's feasibility (headSha present in actions.json) verified against source rather than asserted.

[kubaflo]

🔍 AI-generated — posted by GitHub Copilot CLI on behalf of @kubaflo. Please verify before relying on it.

Thanks for round 2 — all round-1 items confirmed resolved, and the new finding is addressed in e075ae1:

❌ Skip-path TOCTOU (label removed against stale head) — ✓ Fixed. The skip branch now does a live head read before touching the label. Using octokit github.rest.pulls.get (the gh CLI is what 404s in this job — octokit works), it:

• removes s/agent-ready-for-rerun + reacts -1 only when the live head still equals the scan-time headSha;
• if the head advanced since the scan (new push / fresh /review rerun), leaves the label for the next scan to re-evaluate and does not react on the superseded comment;
• if the live read fails, conservatively leaves the label.

Scoped to skip only — the trigger path was already immune (it never removes the label; review-trigger.yml re-validates the live head). MauiBot's inline thread at :209 is resolved. Verified all four branches via a node simulation (head-match → react+remove, head-advanced → leave, read-fail → leave, trigger → unchanged); .lock.yml recompiled.

💡 actions.json atomic write — Left as-is (one reviewer judged it acceptable): a torn/partial write fails JSON.parse → core.setFailed (loud, never silently consumed), and runner.temp is per-run so no stale file can be read. Not data loss.

💡 Classify 401/403 as definitive-untrusted — Intentionally not applied: GitHub returns 403 for secondary rate limits too, which should be retried — treating all 403 as definitive could re-introduce the silent main downgrade under load. The current retry-on-non-404/410 is the safer choice; the only cost is a brief retry on a genuine 403 forbidden, which isn't expected on a public repo.

Tests: 75/75 Pester pass (this round is a JS-only change in the dispatch step). Re-triggering the automated review next.

[MauiBot]

Expert Review — 2 findings

See inline comments for details.

[MauiBot]

[major] Security / Rerun Freshness — The scanner validates the agent decision against the scan-time headSha, but the dispatched review-trigger.yml receives only pr_number, platform, and pipeline_ref. If the PR gets a new push after this validation but before the downstream workflow starts, review-trigger.yml only checks that the PR is open and then triggers review for the new, unvalidated head. Please pass the expected head SHA (and have review-trigger.yml reject/no-op stale dispatches) at the final trusted boundary.

[MauiBot]

AI Review Summary

@kubaflo — new AI review results are available based on this last commit: e075ae1. To request a fresh review after new comments or commits, comment /review rerun.

---

🗂️ Review Sessions — click to expand

🚦 Gate — Test Before & After Fix

Gate Result: ⚠️ INCONCLUSIVE

Platform: ANDROID

⚠️ verify-tests-fail.ps1 exited before writing a verification report. Diagnostics below.

Exit code: 3

Likely cause:

• Test detection failed — no runnable tests were found in the PR diff.
• No fix files detected in the diff (PR may be test-only — should now run in failure-only mode).

Gate output log (last 60 lines)

📁 Output directory: CustomAgentLogsTmp/PRState/36080/PRAgent/gate/verify-tests-fail
🔍 Detecting base branch and merge point...
No PR detected, scanning remote branches for closest base...
✅ Base branch: main (via closest-merge-base)
✅ Merge base commit: 19abc43e
   (1 commits ahead of main)
╔═══════════════════════════════════════════════════════════╗
║         VERIFY FAILURE ONLY MODE                          ║
╠═══════════════════════════════════════════════════════════╣
║  No fix files detected - will only verify:                ║
║  1. Tests FAIL (proving they catch the bug)               ║
║                                                           ║
║  Use this mode when creating tests before writing a fix.  ║
╚═══════════════════════════════════════════════════════════╝
🔍 Auto-detecting test filter from changed test files...
⚠️ No tests detected in this PR.
   Searched for: UI tests, unit tests, XAML tests, device tests
   Consider adding tests via write-tests-agent.

---

📋 Pre-Flight — Context & Validation

Issue: Unknown - no linked issue available from local context
PR: #36080 - Rerun review scanner refactor (local squashed PR branch)
Platforms Affected: CI/review infrastructure; requested test platform: android
Files Changed: 6 implementation/workflow/docs, 2 test

Key Findings

• GitHub metadata could not be fetched because gh is unauthenticated; local squashed PR commit and working repository context were used.
• The PR changes rerun scanner automation under .github/scripts/** and .github/workflows/rerun-review-scanner.*, including batched decisions, deterministic candidate validation, and dispatching review-trigger.yml.
• Expert review found a trigger-path TOCTOU gap: trigger decisions are validated against scan-time head SHA before dispatch, but review-trigger.yml does not receive or validate that expected SHA before lock/OIDC/AzDO work.
• The pre-run gate was explicitly inconclusive and was not rerun.

Code Review Summary

Verdict: NEEDS_CHANGES
Confidence: medium
Errors: 1 | Warnings: 2 | Suggestions: 0

Key code review findings:

• ❌ Trigger path lacks end-to-end head-SHA freshness at the final trusted boundary (rerun-review-scanner.md dispatches only PR/platform/ref; review-trigger.yml validates only open state).
• ⚠️ Scanner reacts +1 after workflow dispatch, before the downstream trigger actually succeeds.
• ⚠️ review-trigger.yml removes s/agent-ready-for-rerun before OIDC/AzDO success and does not restore it on trigger failure.

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #36080	Validate batched scanner decisions against deterministic candidates, emit action file, dispatch review-trigger.yml for triggers, handle skips in safe-output job	⚠️ INCONCLUSIVE (Gate pre-run blocked; not rerun)	.github/scripts/Invoke-RerunReviewTrigger.ps1, tests, scanner workflow/docs	Original PR; directionally good but trigger freshness is incomplete

---

🔬 Code Review — Deep Analysis

Code Review — PR #36080

Independent Assessment

What this changes: PR #36080 refactors the rerun-review scanner path so the gh-aw safe-output job validates batched agent decisions against deterministic candidates, emits normalized actions, and dispatches review-trigger.yml rather than directly triggering AzDO from the scanner job.
Inferred motivation: Avoid gh CLI failures in the gh-aw safe-output context, process more than one rerun candidate per scan, and centralize lock/OIDC/AzDO triggering in the existing trusted review trigger workflow.

Reconciliation with PR Narrative

Author claims: GitHub PR metadata could not be fetched because gh is unauthenticated and public web fetch for the PR returned an action-denied page. Local commit/diff context was used.
Agreement/disagreement: The local diff supports the inferred motivation, but the trigger path leaves a stale-head race after scan-time validation.

Prior Review Reconciliation

No prior ❌ Error findings could be fetched; gh is unauthenticated in this environment.

Blast Radius Assessment

• Runs for all instances: Yes — affects scheduled/manual rerun scanner processing for all queued PRs labeled s/agent-ready-for-rerun.
• Startup impact: No application startup impact; CI/review infrastructure only.
• Static/shared state: No runtime static state, but workflow labels and dispatch side effects are persistent shared state.

CI Status

• Required-check result: undetermined
• Classification: undetermined; gate was pre-run and inconclusive by prompt, and gh pr checks cannot run without authentication.
• Action taken: confidence capped; no PR comments posted.

Findings

❌ Error — Trigger path lacks end-to-end head-SHA freshness at the final trusted boundary

The PR validates agent output against the scan-time candidate head SHA before writing dispatch actions, but the safe-output job dispatches review-trigger.yml with only pr_number, platform, and pipeline_ref. If a PR advances after scanner validation but before the dispatched workflow starts, review-trigger.yml validates only that the PR is open and then locks/triggers review for the newer head. This can run privileged/costly review infrastructure on content the scanner did not evaluate.

⚠️ Warning — Trigger acknowledgement is emitted before downstream trigger success

The scanner reacts +1 immediately after successfully dispatching review-trigger.yml. A downstream stale-head/no-op or OIDC/AzDO failure can leave a user-visible acceptance reaction even though no review rerun was actually triggered.

⚠️ Warning — Ready queue label can be removed before downstream trigger success

review-trigger.yml removes s/agent-ready-for-rerun during lock acquisition before OIDC/token/AzDO trigger completion. If downstream triggering fails, cleanup clears the in-progress lock but does not restore the ready label, so a queued rerun can be lost.

Failure-Mode Probing

• PR head changes after scanner validation but before review-trigger.yml starts: current PR can trigger review for the new head without scanner approval.
• review-trigger.yml dispatch succeeds but AzDO trigger fails: current PR may already have reacted +1 and removed the ready label even though no rerun happened.
• Skip decision sees a changed head: current PR already handles this conservatively by live-reading the head and leaving the queue label.

Verdict: NEEDS_CHANGES

Confidence: medium
Summary: The refactor is directionally sound, but trigger decisions need the same freshness and transactional semantics as skip decisions. The best alternative found moves trigger acceptance into review-trigger.yml so the final trusted boundary validates the scan-time head, reacts only after AzDO trigger success, and restores the queue label on post-lock trigger failure.

---

🛠️ Fix — Analysis & Comparison

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
1	try-fix	Add expected_head_sha to review-trigger.yml and fail Validate PR on stale head	⚠️ Partial static pass; Pester blocked	3 files	Closes main race but stale dispatch becomes workflow failure; early +1/label-loss remain
2	try-fix	Re-read live PR head in scanner immediately before dispatch	⚠️ Partial static pass; Pester blocked	2 files	Narrows but does not close race before downstream lock/OIDC/AzDO
3	try-fix	Add expected_head_sha and make stale dispatch a clean no-op via gated downstream steps	⚠️ Partial static pass; Pester blocked	3 files	Clean stale no-op, but early +1 and ready-label loss remain
4	try-fix	Move trigger acceptance to review-trigger.yml: expected SHA validation, deferred +1, ready-label restore on trigger failure	✅ Best available (static pass; Pester blocked)	3 files	Expert-reviewed PASS; demonstrably stronger than PR fix and prior candidates
PR	PR #36080	Validate scanner decisions and dispatch review-trigger.yml for trigger decisions	⚠️ INCONCLUSIVE (Gate pre-run blocked; not rerun)	8 files	Original PR lacks end-to-end trigger freshness and transactional acknowledgement

Cross-Pollination

Model	Round	New Ideas?	Details
maui-expert-reviewer	1	Yes	Recommended end-to-end expected_head_sha validation at review-trigger.yml, not only in scanner pre-dispatch code
maui-expert-reviewer	2	Yes	Refined candidate 1 to no-op stale dispatches instead of failing the workflow
maui-expert-reviewer	3	Yes	Flagged candidate 3 transaction gaps; recommended moving +1/ready-label ownership into review-trigger.yml
maui-expert-reviewer	4	No	Candidate 4 passed; only caveats were best-effort reaction, SHA validation, and optional exact-head recheck after lock. SHA validation was added.

Exhausted: Yes
Selected Fix: Candidate #4 — It is the only candidate that validates scan-time head SHA at the final trusted boundary, avoids false-positive +1 reactions, and preserves the rerun queue on downstream trigger failure. Full test execution remained blocked by missing Pester; the available static checks passed.

---

📝 Recommended PR Title & Description

Assessment: ✏️ Recommend updating — the current description accurately explains the raw PR refactor, but the winning fix adds downstream stale-head validation and transactional rerun acknowledgement/queue handling that the current metadata does not describe.

Recommended title

Review Scanner: Dispatch review-trigger.yml with stale-head and rerun outcome guards

Recommended description

## Problem

The `/review rerun` scanner (`rerun-review-scanner`) dispatched zero AzDO reviews after #35955, and the few it should dispatch would target the wrong branch. Two independent bugs:

### Bug 1 — every dispatch aborted with a spurious 404

In the gh-aw safe-output job, the `gh` CLI returns `HTTP 404` for `repos/dotnet/maui/pulls/N` — even with `pull-requests: write` on a public repo. #35955 misread these as transient and "hardened" the not-found guard, so it now faithfully confirms the bogus 404 and skips every PR. Evidence it is not a permission/token problem: the `pre_activation` job (only `Metadata: read`) reads the same PRs fine, and the built-in `safe_outputs` job (octokit) succeeds in the same run.

### Bug 2 — custom review branches silently downgraded to `main`

The candidate builder decided whose `/review -b <branch> -p <platform>` to trust using the comment's `author_association`. Under the Actions `GITHUB_TOKEN`, a maintainer whose org membership is private reads as `CONTRIBUTOR` (not `MEMBER`), so their command was dropped and the rerun fell back to the `main` pipeline. A live scan confirmed it dispatched four net11 (`feature/enhanced-reviewer`) PRs on `main`.

## Fix — do exactly what a maintainer `/review` does, with rerun-specific freshness guards

**Bug 1:** instead of re-implementing PR validation + OIDC + the AzDO trigger inside the safe job, the scanner now dispatches the same `review-trigger.yml` workflow `/review` runs, via `workflow_dispatch`. That workflow owns PR validation, the `s/agent-review-in-progress` lock, platform inference, OIDC, and the AzDO trigger. (`workflow_dispatch` via `GITHUB_TOKEN` always creates a run — it is exempt from Actions recursion-prevention.)

- `Invoke-RerunReviewTrigger.ps1` is now pure: validate batched `decisions` against `candidates.json`, emit `actions.json`. No `gh`/AzDO/OIDC/lock/rate-limit I/O.
- A `github-script` (octokit) step performs all GitHub writes — octokit works in the safe-job context where the `gh` CLI does not.
- For `trigger`, the scanner dispatches `review-trigger.yml` with `pr_number`, `platform`, `pipeline_ref`, the scan-time `expected_head_sha`, and the `rerun_comment_id`.
- `review-trigger.yml` validates the expected SHA at the final trusted boundary and cleanly no-ops stale rerun dispatches before lock/OIDC/AzDO work.
- The scanner no longer reacts `+1` immediately after workflow dispatch. `review-trigger.yml` reacts `+1` only after AzDO run creation succeeds.
- If a scanner-dispatched rerun fails after the in-progress lock is applied, cleanup clears the lock and restores `s/agent-ready-for-rerun` so the queued rerun is not lost.
- `skip` decisions remain scanner-owned: the safe-output job reacts `-1` and removes the queue label only after confirming the live head still matches the scan-time candidate.
- Permissions: `+actions:write`, `−id-token:write`; dropped the `AZDO_TRIGGER_*` secrets from the scanner job.

**Bug 2:** authorize `/review` options by a live collaborator-permission lookup (`collaborators/<user>/permission` → write/maintain/admin) — the exact call `review-trigger.yml`'s auth step makes. It only needs `metadata: read` (every token has it) and reflects current access. No new secret or permission.

- `Resolve-RerunEligibility.ps1`: add `Test-ReviewOptionLoginTrusted` (cached per login); `Get-LatestReviewCommandOptions` computes trust through it. Removed the `author_association` gate.
- `Query-RerunReadyPRs.ps1`: drop the `author_association` helpers; pass `-Owner/-Repo`.

## Validation

- 69 Pester tests pass (36 dispatch + 33 resolver), incl. a regression test that an `author_association=NONE` command is still honored when the login has write access.
- Live, real (non-dry) scan: the safe job validated all decisions with no 404s and octokit performed real `createWorkflowDispatch`, producing two `review-trigger.yml` runs that triggered real AzDO `maui-copilot` builds (HTTP 200, Run IDs 14459427 & 14459428).
- Real-data check for Bug 2: PR #34564's history now resolves to `pipelineRef=feature/enhanced-reviewer` (author `kubaflo`, `author_association=CONTRIBUTOR`) instead of `main`, using the live permission lookup.

## Files

- `.github/scripts/Invoke-RerunReviewTrigger.ps1`, `.github/scripts/Invoke-RerunReviewTrigger.Tests.ps1`
- `.github/scripts/Resolve-RerunEligibility.ps1`, `.github/scripts/Resolve-RerunEligibility.Tests.ps1`
- `.github/scripts/Query-RerunReadyPRs.ps1`
- `.github/workflows/rerun-review-scanner.md`, `.github/workflows/rerun-review-scanner.lock.yml`
- `.github/workflows/review-trigger.yml`
- `.github/docs/agent-labels.md`

---

🏁 Report — Final Recommendation

Comparative Fix Report — PR #36080

Candidates compared

Rank	Candidate	Regression result	Summary
1	pr-plus-reviewer	⚠️ Static checks only; Pester blocked	Applies the expert review feedback to the PR fix. It passes scan-time expected_head_sha and rerun_comment_id into review-trigger.yml, no-ops stale dispatches at the downstream trusted boundary, defers +1 until AzDO trigger success, and restores s/agent-ready-for-rerun on post-lock trigger failure.
2	try-fix-4	✅ Best available static result; Pester blocked	Functionally equivalent to pr-plus-reviewer and the strongest non-PR fallback. Ranked below pr-plus-reviewer only because the reviewer-applied PR-track candidate is the same solution integrated as direct feedback on the submitted fix.
3	try-fix-3	⚠️ Partial static pass; Pester blocked	Adds downstream expected-head validation and clean stale no-op behavior, but still leaves the scanner's early +1 and can lose the ready label on downstream trigger failure.
4	try-fix-1	⚠️ Partial static pass; Pester blocked	Moves freshness validation to review-trigger.yml, but stale dispatches fail the workflow instead of cleanly no-oping, and it still has early +1 / ready-label loss gaps.
5	try-fix-2	⚠️ Partial static pass; Pester blocked	Re-reads the live head immediately before workflow dispatch, which narrows but does not close the race before review-trigger.yml lock/OIDC/AzDO work.
6	pr	⚠️ Inconclusive	The submitted PR fixes the broad architecture problem by dispatching review-trigger.yml and replacing author_association trust with live permission lookup, but trigger freshness and acknowledgement remain incomplete.

No candidate had a recorded regression-test failure; Pester was unavailable for the try-fix candidates and the gate was explicitly inconclusive. Therefore, ranking is based on code correctness and failure-mode coverage, not on treating inconclusive tests as failures.

Winning candidate

Winner: pr-plus-reviewer

pr-plus-reviewer wins because it keeps the PR's strong architectural refactor and applies the expert reviewer's two required corrections: final-boundary head-SHA freshness and transactional rerun acknowledgement/queue handling. It is strictly stronger than the raw PR and equivalent in behavior to try-fix-4, while staying on the PR-fix path.

Why the raw PR does not win

The raw PR validates decisions against scan-time candidates before dispatch, but does not pass that expected head SHA to the workflow that actually locks and triggers AzDO. A PR can advance between scanner validation and downstream workflow execution, causing review infrastructure to run on content the scanner did not validate. It also reacts +1 after createWorkflowDispatch succeeds, even though the downstream workflow may later no-op or fail before AzDO run creation.

Why the other try-fix candidates do not win

try-fix-1 and try-fix-3 both identify the right trusted boundary, but candidate 1 makes stale dispatches fail and candidate 3 still leaves false acceptance and queue-loss behavior. try-fix-2 is weaker because it only performs a pre-dispatch live head read in the scanner; the race remains open until review-trigger.yml starts. try-fix-4 is the best alternative and should be used as the fallback implementation if the PR-track reviewer feedback cannot be applied directly.

---

🧭 Next Steps — review latest findings

No alternative fix was selected for this run. Review the session findings and CI results before merging.

[PureWeen]

Multi-model adversarial review — PR #36080 (round 3, commit e075ae12b7)

Re-reviewed after the round-2 fix. 3 independent reviewers (different model families) re-ran in parallel with adversarial consensus; every disputed claim was re-verified against source. Code-only review (CI out of scope). Comment review — no approval/blocking implied.

Round-2 finding resolved ✅

❌ Skip-path TOCTOU (rerun-review-scanner.md:209) — fixed in e075ae12b7, verified correct & complete by all 3 reviewers. The skip branch now does a live octokit pulls.get, and only reacts -1 + removes s/agent-ready-for-rerun when the live head still equals the scan-time headSha. The three-way branch is sound: read-failure → leave the label (conservative); head-advanced → leave the label for re-evaluation; head-matches → consume the genuinely-stale request. No new null/empty-SHA path (Invoke-RerunReviewTrigger.ps1:200 throws before emitting any action with a blank headSha, so the a.headSha && guard can't fall through). .lock.yml mirrors it faithfully (body_hash unchanged is expected — the safe-outputs script lives in frontmatter). Residual read→remove micro-window is inherent/best-effort and vastly narrower than round 2.

New MauiBot [major] findings — adjudicated NOT-A-DEFECT (3/3)

Both new bot findings target the trigger path, which this PR did not change in round 3. All 3 reviewers independently concluded neither is a defect this PR introduces; I'm corroborating the bot's concerns explicitly rather than dismissing them:

M1 · :196 "Rerun Freshness" (pass head SHA to review-trigger.yml) → not a defect. The dispatch passes only pr_number/platform/pipeline_ref — exactly what a maintainer /review rerun does; review-trigger.yml (unchanged, not in this PR's file set) reviews the live head by design. For a rerun, reviewing the newer head is the desired behavior, not a stale-head vulnerability — pinning the scan-time SHA would make the scanner diverge from the manual path it's meant to mirror. The headSha check is an anti-hallucination guard (the agent's decision must match the deterministic candidate set), not a contract to pin the eventual review. No untrusted head is smuggled — review-trigger.yml is the trusted boundary (open-PR check, lock, OIDC, auth) and acts on the PR's own head.

M2 · :204 "Rerun Transactionality" (+1 ≠ AzDO-confirmed) → not a defect; its core premise is factually wrong for this path. MauiBot claims the trigger path can consume s/agent-ready-for-rerun before a downstream failure, leaving a false acceptance. Verified: the scanner's trigger branch only react('+1') and never calls removeReadyLabel — the sole removeReadyLabel call is in the skip matched-head branch (:229). So the durable state (the ready label) is preserved on trigger; if review-trigger.yml no-ops, the next hourly scan re-evaluates — no lost request, no data loss. The +1 is posted only after createWorkflowDispatch returns success (a throw → hadFailure, no +1), so it accurately means "dispatch queued," the strongest claim this job can make. "+1 ≠ AzDO-confirmed" is the same trade-off the manual /review rerun path already carries via the unchanged review-trigger.yml; the proposed downstream 2-phase-commit fix would require editing that out-of-scope file.

💡 Minor (non-blocking, acknowledging M2's kernel): on the trigger path a downstream review-trigger.yml failure leaves a +1 that reads as "accepted" even though the AzDO run never started. It's cosmetic (the label persists → the request self-heals on the next scan) and not introduced by this PR, but a future tweak could move the reaction into review-trigger.yml after AzDO run creation if exact reaction fidelity is wanted.

Tests

75/75 Pester pass (Invoke-RerunReviewTrigger.Tests.ps1 + Resolve-RerunEligibility.Tests.ps1) — two reviewers ran the full suite; the skip-path guard and permission logic are covered.

Prior reviews

Round-2 ❌ skip-path TOCTOU → resolved by e075ae12b7 (confirmed above). MauiBot's two new [major] findings → corroborated and adjudicated NOT-A-DEFECT with verified reasoning, not silently dropped.

Bottom line: the round-3 guard cleanly closes the last ❌. No new ❌/⚠️ within this PR's changed files; the two outstanding bot [major]s root-cause to the unchanged review-trigger.yml (shared with the manual /review path) and are by-design for a rerun. Only a single 💡 cosmetic note remains. The approve/merge call is a maintainer's — this review stays comment-only.

Methodology: 3 independent reviewers with adversarial consensus. The decisive facts (validator guarantees non-empty headSha; trigger path never removes the ready label; pulls.get has pull-requests permission) were verified against source, not asserted.

[kubaflo]

🔍 This is an AI-generated comment posted on @kubaflo's behalf.

@PureWeen — addressed the one remaining 💡 cosmetic note from your Round-3 review in 7f75813b97.

The trigger path now reacts 🚀 (rocket) instead of 👍 (+1) on createWorkflowDispatch success, so the acknowledgement reads as "dispatch queued / launching" rather than "accepted/complete" — matching the actual guarantee (the AzDO run is owned by review-trigger.yml, which may no-op). As you noted, the durable state is unaffected: the trigger path never removes s/agent-ready-for-rerun, so a downstream no-op self-heals on the next scan. The skip path keeps 👎.

Kept it in-scope (no changes to review-trigger.yml); regenerated the .lock.yml via gh aw compile (frontmatter_hash refreshed, body_hash unchanged). Ready for re-review.

[PureWeen]

Multi-model adversarial review — PR #36080 (round 4, commit 7f75813b97)

3 independent reviewers (different model families) re-ran in parallel with adversarial consensus, scoped to the round-4 delta. Facts verified against source. Code-only review (CI out of scope). Comment review — no approval/blocking implied.

Round-3 💡 resolved ✅

The only outstanding item from round 3 — the trigger-path +1 reaction reading as "accepted/confirmed" rather than "queued" — is addressed in 7f75813b97. The trigger path now reacts 🚀 (rocket) instead of 👍 (+1), with a comment clarifying it acknowledges the dispatch was queued (not AzDO-confirmed) and that s/agent-ready-for-rerun persists so the next scan self-heals on a downstream no-op. Clean fit for the concern.

Round-4 change — clean (3/3)

All three reviewers independently confirmed the one-line reaction change is correct:

• rocket is a valid reactions-API value — GitHub's reactions.createForIssueComment content enum is exactly +1, -1, laugh, confused, heart, hooray, rocket, eyes; rocket is a member. The react() helper passes content straight through (and is try/catch-guarded regardless).
• Skip path untouched — :232 still reacts '-1'; the round-2/3 head-SHA TOCTOU guard is intact. Only the trigger branch changed.
• Lock/source parity — 'rocket' mirrored at lock:1748; diff is exactly +4/−1 (comment + reaction) in each file. frontmatter_hash regenerated, body_hash unchanged — expected (the github-script lives in the frontmatter).
• No regression — no Pester test or other consumer asserts the old '+1' string; the comment's persist-label/self-heal description is accurate (the trigger branch deliberately never calls removeReadyLabel).

One minor 💡 (1/3, verified in source)

💡 Documentation — Invoke-RerunReviewTrigger.ps1:16. The script's .DESCRIPTION synopsis still reads "…then react +1 to the rerun comment", which went stale with this commit's switch to rocket. Zero behavioral impact (the .ps1 performs no reactions itself — it only emits the validated actions array), and the skip-path -1 mention on line 26 is still accurate. Optional: update line 16 to react 🚀 (rocket) so the synopsis matches the workflow. Flagged by 1 reviewer; confirmed against source.

Prior reviews

Round-2 ❌ skip-path TOCTOU → resolved (e075ae12b7). Round-3 MauiBot [major] findings (head-SHA freshness, +1 transactionality) → adjudicated NOT-A-DEFECT (3/3) and the cosmetic kernel of the latter is now further softened by this 🚀 change. No unresolved ❌/⚠️ remain.

Bottom line: round 4 is a correct, zero-risk cosmetic refinement that closes the last 💡 from round 3. No ❌/⚠️ anywhere in the PR's changed files — only one optional doc-text 💡 (the stale +1 in the .ps1 synopsis). From a code-review standpoint this is clean; the approve/merge call is a maintainer's — this review stays comment-only.

Methodology: 3 independent reviewers with adversarial consensus. Load-bearing facts (rocket is a valid reaction value; skip path unchanged; lock parity; no test asserts the reaction; the stale +1 synopsis line) verified against source, not asserted.

[kubaflo]

🔍 This is an AI-generated comment posted on @kubaflo's behalf.

@PureWeen — thanks for the Round-4 pass confirming the 🚀 change is clean. Addressed the one minor 💡 in e52e1469d3: updated the Invoke-RerunReviewTrigger.ps1 .DESCRIPTION synopsis (line 16) from "react +1" to "react rocket (🚀)" so the doc-text matches the workflow. Zero behavioral impact — the script performs no reactions itself, it only emits the validated actions array; the skip-path -1 mention stays accurate. No outstanding findings remain.

[PureWeen]

Adversarial review — PR #36080 (round 5, commit e52e1469d3)

Code-only review. Comment review — no approval/blocking implied.

Round-4 💡 resolved ✅

The one remaining item from round 4 — the stale +1 in the Invoke-RerunReviewTrigger.ps1 .DESCRIPTION synopsis — is fixed in e52e1469d3. Line 16 now reads react rocket (🚀) … to acknowledge the dispatch was queued, matching the workflow's actual react(a.rerunCommentId, 'rocket') (rerun-review-scanner.md:208). The skip-path -1 description (line 21) is preserved and still accurate.

Scope of this round

The entire round-5 delta is +2/−2 lines of doc comment in one .ps1 synopsis — no code, logic, or behavior change (the .ps1 performs no GitHub I/O; it only emits the validated actions array). Verified directly:

• Synopsis now consistent with the implementation (rocket on trigger, -1 on skip).
• No other stale +1/👍 reaction references remain in the changed files. The lone remaining 👍 mention (md:204, "Use 🚀 (not 👍)") is the intentional rationale comment, not a stale reference.
• No new reviewer findings on this commit.

Given this is a pure documentation edit closing my own round-4 note, a fresh 3-model adversarial pass would add no signal beyond rounds 1–4 (which cleared every substantive code finding); I verified the delta directly instead.

Trajectory across 5 rounds

• ❌ Batch poisoning → resolved (if: ${{ !cancelled() }})
• ❌ Skip-path head-SHA TOCTOU → resolved (live pulls.get guard, e075ae12b7)
• ⚠️/💡 permission-blip, login validation, rate-limiter → resolved/documented
• MauiBot [major] head-freshness + +1 transactionality → adjudicated NOT-A-DEFECT (3/3), and the cosmetic kernel softened via 🚀 (7f75813b97)
• 💡 +1 reaction semantics → resolved (🚀)
• 💡 stale +1 synopsis → resolved (this commit)

Bottom line: no ❌/⚠️/💡 remain in the PR's changed files. Every finding raised across five rounds has been addressed or adjudicated. From a code-review standpoint this PR is clean and merge-ready; the approve/merge decision is a maintainer's — this review stays comment-only.