Restrict /review rerun eligibility to author activity

[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!

Description of Change

Restricts /review rerun eligibility so PRs are only queued for rerun when there is new PR-author activity after the latest AI Summary or previous rerun checkpoint:

• new non-command comments from the PR author
• new commits / head changes

Reviewer or maintainer reminder comments no longer satisfy the rerun evidence check.

Issues Fixed

Prevents /review rerun from applying s/agent-ready-for-rerun when only a reviewer/maintainer comment was added after the latest AI Summary.

[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 -- 35874

Or

• Run remotely in PowerShell:

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

[PureWeen]

⚠️ Logic — bot-authored PR login format mismatch (2/3 reviewers)

Here $pr.author.login from gh pr list --json author resolves to an app-style login for bot authors (verified live against this repo): app/dependabot, app/dotnet-maestro, app/github-actions, app/copilot-swe-agent. But REST gh api .../pulls/N returns user.login = "dependabot[bot]", and comment user.login also uses the [bot] suffix form. Test-CommentIsEvidence does OrdinalIgnoreCase equality on these strings, so a bot author's own comments never match this $authorLogin.

Practical impact is bounded (one reviewer overstated this as ❌; verified): the deterministic eligibility gate invoked from /review rerun lives in Resolve-RerunEligibility.ps1 and gets its author login from REST ($pr.user.login at lines 620/645), which IS the canonical [bot] form — so this code path matches comment user.login correctly. Additionally, Test-CommentIsEvidence already filters Comment.user.type -eq 'Bot' regardless of login, so bot-on-own-PR comments never count anyway. What breaks is the scanner's advisory context markdown: for bot-authored PRs (Dependabot, Maestro, copilot-swe-agent) it will display PR author: app/... (confusing) and New non-command author comments: 0 even if the bot has actually commented post-summary — biasing the agent's trigger/skip reasoning.

Suggested fix: either resolve the author via REST inside the loop (one extra API call per candidate; cheap given MaxPRs is small), or normalise app/X → X[bot] when is_bot is true. Adding ,isBot to the --json selector makes the normalisation deterministic without a second round-trip.

[PureWeen]

⚠️ Error handling — failure modes silently swallowed (2/3 reviewers)

The new defensive PR fetch combined with the exit 0 added at line 412 is too broad. gh api ... 2>$null followed by $global:LASTEXITCODE = 0 and continue treats every gh failure (auth expired, secondary rate limit, transient 5xx, malformed JSON) identically to "PR was deleted / 404". The step then keeps iterating, swallows the error, and exit 0 reports the whole safe-output job green.

Under a real systemic failure (PAT/App token rotated mid-run, GitHub outage, secondary rate limit while a large queue is draining), every queued rerun decision is silently skipped without surfacing the underlying cause — operators see a clean green run while no actual reruns fire. That's exactly the failure shape this script is supposed to convert into a visible error.

Suggested fix: distinguish stale-PR from transient/credential failures. Capture stderr (don't drop it), and only short-circuit when the response indicates a known stale status (e.g., 404/410, gone, not found). For 401/403/5xx/secondary-rate-limit, surface the error and let the step fail visibly. Alternatively, accumulate a per-iteration error count and exit 1 at the bottom if any candidate failed for non-stale reasons.

[PureWeen]

Adversarial multi-model review — PR #35874

Methodology: 3 independent reviewers (different models) ran in parallel against the full diff; findings reconciled via adversarial consensus. Reviewer identities are anonymised below.

Scope verified

• 6 files, +242 / -96. Core change: /review rerun deterministic gate now only counts PR-author non-command comments as evidence (reviewer/maintainer reminders no longer satisfy the gate). Commits remain evidence regardless of author.
• Wired via new $PRAuthorLogin parameter on Test-CommentIsEvidence → Test-HasEvidenceCommentAfter → New-RerunContextMarkdown → Resolve-RerunEligibility (lines 279, 311, 410, 529). Source of truth at the gate is REST $pr.user.login (lines 620, 645).
• gh-aw workflow restructured to use on.steps: pre-activation pattern with jobs.pre-activation.outputs.rerun_candidates (verified against compiled .lock.yml line 1378 — pre_activation job is present and the activation job correctly depends on it).

Findings

Sev	Finding	Consensus
⚠️	gh pr list --json author returns app-style login (app/dependabot) — never matches comment user.login (dependabot[bot])	2/3 (one reviewer overstated as ❌; verified scanner path is advisory only)
⚠️	gh api 2>$null + $LASTEXITCODE = 0 + continue + exit 0 treats all gh failures (auth, rate-limit, transient 5xx) as stale-PR	2/3

No blocking issues. Both findings are posted as inline comments where they apply.

Test coverage assessment

Partial. One new test added (rejects a non-author maintainer comment after the latest AI Summary) and six existing tests updated to pass -PRAuthorLogin 'dev-user' and assert the new new-author-comment-* reason strings. Gaps surfaced by reviewers (low severity, not blocking):

• No test for bot-authored PR (e.g., PRAuthorLogin = 'dependabot[bot]') — relevant to Finding A.
• No test for empty/null PRAuthorLogin documenting the fail-safe behaviour (comments don't count; commits still do).
• No test verifying case-insensitive author match (the implementation uses OrdinalIgnoreCase).
• No test covering "commit-after-summary qualifies even when only non-author comments exist" — currently inferred from the commit branch but not asserted directly.

Consider adding these as a follow-up — none of them are gating.

Prior reviews

None. No existing inline comments. The only issue-level comment is the auto-generated dogfood instructions from github-actions[bot].

Out of scope

CI status was not evaluated.

Note: this review event uses event: COMMENT — approval is a human decision.

[kubaflo]

@PureWeen addressed your review comments in 3939e24. Ready for re-review.

[kubaflo]

Code Review — PR #35874 (latest head)

Independent Assessment

What this changes: The rerun scanner now only treats PR-author non-command comments (plus new commits) as deterministic rerun evidence, moves candidate gathering into a gh-aw pre-activation job, validates safe-output decisions against a candidate file, and adds stale-PR/error handling around the trigger path.
Inferred motivation: Prevent maintainer/reviewer reminder comments from satisfying /review rerun, while keeping scheduled/manual scanner runs deterministic and safe from stale AI decisions.

Reconciliation with PR Narrative

Author claims: The PR restricts rerun eligibility to author activity and fixes scanner candidate handoff/stale-decision handling.
Agreement/disagreement: The author-activity restriction is mostly implemented, and the bot author-login advisory issue from the prior review is fixed via normalization. The stale-decision error-handling fix is incomplete because non-stale gh api failures are still caught by the outer loop and followed by exit 0.

Prior Review Reconciliation

Prior finding	Source	Status	Evidence
Bot-authored PR login mismatch	PureWeen inline warning	✅ Fixed	Normalize-GitHubActorLogin converts app/dependabot to dependabot[bot], and Query-RerunReadyPRs.ps1 uses it before generating context.
Broad gh api failure swallowing	PureWeen inline warning	❌ Still unresolved	Non-404 failures now throw, but the enclosing catch logs and continues, and the script ends with exit 0.

Blast Radius Assessment

• Runs for all queued rerun candidates: yes — scanner/trigger infrastructure affects every PR with s/agent-ready-for-rerun.
• Startup impact: no app startup impact, but workflow startup/pre-activation can fail before the agent receives candidates.
• Static/shared state: no application static state; persistent GitHub labels/reactions/AzDO triggers are the shared external state.

CI Status

• Required-check result: license/cla pass; maui-pr skipping.
• Classification: undetermined CI coverage.
• Action taken: confidence capped low; not eligible for LGTM while required CI is skipping.

Findings

Two inline findings are posted.

Failure-Mode Probing

• If GitHub returns 401/403/500 while processing decisions, does the safe-output job fail? No. The new fetch code throws, but the outer catch converts it into a logged error and continues, and the script exits 0.
• If the gh-aw pre-activation job runs with generated top-level permissions: {}, can it reliably checkout/query candidates? No. The generated job does not grant the read scopes needed by checkout and gh pr list/gh api.
• If the candidate is truly stale/deleted, is it skipped safely? Yes, 404/410-style responses are handled as stale decisions.

Verdict: NEEDS_CHANGES

Confidence: low. This is shared CI/review infrastructure and required CI is skipping; additionally, there are concrete workflow/error-propagation issues that can make the scanner silently fail or fail before candidate generation.

[kubaflo]

❌ Error — non-stale PR fetch failures are still reported as success

This throw does not currently make the safe-output job fail. It is inside the outer per-item try, whose catch logs ::error::... and then continues; after the loop the script unconditionally executes exit 0. So auth failures, secondary rate limits, transient 5xx responses, or malformed JSON still produce a green safe-output job after logging, which is the same failure shape the previous review asked to avoid. Track whether any non-stale candidate processing failed (for example $hadFatalError = $true) and exit 1 after the loop, or rethrow from the catch for non-stale failures.

[kubaflo]

❌ Error — pre-activation job lacks required read permissions

The generated lock workflow sets top-level permissions: {}, and this new pre-activation job does not override it. But the job checks out the repository and runs Query-RerunReadyPRs.ps1, which uses GH_TOKEN for gh pr list and multiple gh api calls. Please add explicit read permissions for this job (contents: read, issues: read, pull-requests: read) in the .md and regenerate the .lock.yml; otherwise the scanner can fail before producing rerun_candidates.

[PureWeen]

⚠️ Error Handling / Correctness — 2>&1 merges stderr into stdout on the success path, which can corrupt JSON parsing
Flagged by: 1/3 reviewers (provenance: round-2 fix introduced this line; per multi-round self-correction rule, escalated to 2/3 weight)

The round-1 form was & gh api ... 2>$null (stderr discarded → clean stdout JSON). Round-2 changed this to @(& gh api ... 2>&1) so the failure-path regex can inspect the stderr message. But the merge happens unconditionally, including on exit-0 calls. If gh ever writes a single line to stderr alongside a successful response (deprecation notice, extension chatter, future gh behavior change), $prJson becomes <JSON> <stderr line> and the ConvertFrom-Json call at line 340 throws → caught by the outer try/catch → ::error:: annotation + continue. A valid, open PR with a pending rerun decision gets silently dropped (no trigger, no label removal).

Empirically reproduced under PowerShell 7.4.5: '{"state":"open"}' + "\nsome notice line" | ConvertFrom-Json throws Additional text encountered after finished reading JSON content. Likelihood in non-TTY CI is low today (gh suppresses the update-notifier off-TTY), but this is unnecessary fragility the round-2 change introduced.

Fix: only merge stderr on failure. Capture stdout normally and redirect stderr to a separate variable/file, inspected only when $LASTEXITCODE -ne 0:

$errFile = New-TemporaryFile
try {
    $prJson = (& gh api "repos/$Owner/$Repo/pulls/$prNumber" 2>$errFile | Out-String).Trim()
    $prExitCode = $LASTEXITCODE
    $prErr = Get-Content -Raw -LiteralPath $errFile -ErrorAction SilentlyContinue
} finally { Remove-Item $errFile -ErrorAction SilentlyContinue }
if ($prExitCode -ne 0) {
    if (Test-GhApiPrNotFound -Output $prErr) { ... } else { throw ... }
}

[PureWeen]

⚠️ Error Handling / Observability — systemic gh failures still report the step green (round-1 Finding B only half-closed)
Flagged by: 3/3 reviewers (severity merged to ⚠️ per consensus; one reviewer hedged "may be intentional")

This fix correctly stops misclassifying non-404/410 failures as "stale" — that part of the round-1 finding is closed. But the new throw lands in the per-item outer try/catch (line 428), which writes a ::error:: annotation and continues. The script then unconditionally exit 0s at line 435. ::error:: annotations do not fail a step in GitHub Actions — only a non-zero exit does.

Concrete scenario: GH_TOKEN expires mid-run, secondary rate limit hits, or GitHub returns HTTP 5xx. gh api fails for every queued candidate. The loop emits N error annotations, the step exits 0, the workflow reports green. Any monitor that alerts on job-status rather than scraping annotations sees a successful run — no reruns triggered, no operator notification.

Fix: track non-stale failures and fail the step at the end. Minimal change:

$processingFailed = $false
foreach ($item in $items) {
    try { ... }
    catch {
        $processingFailed = $true
        Write-Host "::error::Failed to process ... "
        continue
    }
}
if ($processingFailed) { exit 1 } else { exit 0 }

Or fail-fast on auth/rate-limit classes specifically (distinct from a one-off transient 5xx on a single PR, which you may legitimately skip-and-continue). If the green-on-systemic-failure behavior is intentional (e.g. "don't ever fail the scanner; rely on annotation triage"), worth documenting that choice in a comment near the exit 0.

[PureWeen]

💡 Test Quality — this test name overclaims; the "no bot evidence" assertions are satisfied by the pre-existing user.type -eq 'Bot' filter, not by the new normalization
Flagged by: 2/3 reviewers explicitly + 1 supporting non-finding (provenance: round-2 introduced)

The round-2 fix added Normalize-GitHubActorLogin and applies it inside Test-CommentIsEvidence (check #4 — author equality, now normalized). But check #6 in the same function still unconditionally rejects Comment.user.type -eq 'Bot' — that line survived round-2 unchanged. So for a bot-authored PR, the bot's own comments are rejected at #6 regardless of whether #4 matches.

Empirically verified by dot-sourcing the function:

• Test-CommentIsEvidence(botComment, PRAuthorLogin='app/dependabot') → False (rejected at [Spec] Transitions #6, even though Aloha System.Maui! #4 now passes)
• Removing Normalize-GitHubActorLogin from inside Test-CommentIsEvidence and running this test: still passes — because New non-command author comments: 0 is satisfied by [Spec] Transitions #6, and Should -Not -Match 'Dependabot follow-up' is satisfied by the rendering loop which filters via the same Test-CommentIsEvidence.
• The only assertion in this test that actually exercises the new code is line 470: Should -Match 'PR author: dependabot[bot]' (rendering the normalized author).

Implication: the fix is decision-neutral for bot evidence — its only functional effect is the displayed PR author: line in the context markdown (an advisory input to the AI scanner; the deterministic evidence counts are unaffected). Constructing a non-bot comment whose user.login needs app/→[bot] normalization to match the author is not a realistic scenario — REST issue/PR comments always emit bot logins in the X[bot] form, never app/X.

Fix: either (a) rename this test to focus solely on display normalization (renders normalized app-style bot author in context markdown) and remove the misleading "without counting bot comments as evidence" claim, or (b) split into two tests: one for the display (passes with the fix), one for bot-comment-rejection (passes regardless, documenting the #6 filter as the actual gate). Also worth removing the Normalize-GitHubActorLogin call inside Test-CommentIsEvidence itself — it's dead code given check #6 — though leaving it as defensive symmetry is also defensible.

[PureWeen]

Adversarial multi-model review — PR #35874 (Round 2)

Methodology: 3 independent reviewers ran in parallel against the new state after 3939e24a ("Address rerun scanner review feedback"); findings reconciled via adversarial consensus with multi-round self-correction weighting.

Verification of round-1 fixes

Round-1 finding	Round-2 fix	Status
⚠️ Bot login format mismatch (app/X vs X[bot])	Added Normalize-GitHubActorLogin; applied to PR author + comment login symmetrically; updated context-markdown display	Partially addressed — cosmetic (display) fix is correct; decision impact on bot-author PRs is unchanged because Test-CommentIsEvidence line 318 still filters all type='Bot' comments. See Finding 3.
⚠️ All gh api failures swallowed as stale	Added Test-GhApiPrNotFound to distinguish 404/410 from auth/rate-limit/5xx; throw on non-stale	Partially addressed — failure classification is correct (verified locale-robust via numeric \bHTTP\s+(404|410)\b alt); but the throw is caught + step still exit 0s, so systemic failures still report green. See Finding 2. Also introduced a new regression on the success path: see Finding 1.

New findings

Sev	Finding	Consensus
⚠️	Success-path 2>&1 merges stderr into JSON output, can break ConvertFrom-Json on otherwise-successful gh calls	1/3 explicit, escalated per multi-round self-correction rule (round-2-introduced + empirical PowerShell reproduction)
⚠️	Systemic gh failures still report step green — throw → outer catch → ::error:: + continue → exit 0 (round-1 Finding B half-closed)	3/3 (one reviewer ❌, one ⚠️, one hedged "may be intentional"; conservative merge: ⚠️)
💡	Round-2 Normalize-GitHubActorLogin is decision-neutral for bot evidence (line 318 type-filter is the real gate); new test name overclaims	2/3 explicit + 1 supporting non-finding

No blocking issues. All findings posted as inline comments on the round-2 changes.

Confirmed correct (so the picture is complete)

• Test-GhApiPrNotFound is locale-robust (\bHTTP\s+(404\|410)\b numeric path matches regardless of message language; gh is English-only anyway).
• 404/410 → silent skip; 401/403/500/empty → throw. Failure-path 2>&1 + Out-String correctly stringifies ErrorRecord to text for the regex.
• Normalize-GitHubActorLogin is idempotent and safe (no false positives — bot comments still filtered by type; no false negatives — GitHub logins can't contain /, so ^app/ never matches a human login).
• The .lock.yml trigger step is consistent with the workflow .md (agent decision is advisory; deterministic re-validation of head SHA / labels / rate-limit happens server-side in Invoke-RerunReviewTrigger.ps1).

Test coverage assessment

Good. Round-2 added: 1 unit test for Normalize-GitHubActorLogin (app/X → X[bot], trims, empty), 1 integration test for context-markdown rendering (the one Finding 3 critiques), and 2 tests for Test-GhApiPrNotFound (stale-pattern recognition + auth/rate-limit/5xx rejection). The Test-GhApiPrNotFound tests are well-scoped. Gaps: no decision-level test that would fail without the bot-login fix (the difficulty of constructing one is itself the signal that the fix is display-only).

Prior reviews

Round 1 of this same adversarial review (3 findings, 2 posted as inline comments). All findings flagged in this round are either residuals of round 1 fixes or net-new regressions introduced by them.

Out of scope

CI status was not evaluated.

Review event is COMMENT — approval remains a human decision.

[kubaflo]

@PureWeen addressed the new review comments in f264273. I also reran the scanner workflow on the PR branch in dry-run mode: it found 1 queued candidate, emitted safeoutputs-trigger_rerun_review, and processed PR #35874 decision=skip successfully. Ready for re-review.

[PureWeen]

❌ Regression — empty-stderr temp file causes .Trim() to throw on every successful gh api call

Flagged by: 3/3 reviewers (unanimous), empirically reproduced in pwsh 7.4.5

When gh api repos/.../pulls/N succeeds, it writes JSON to stdout and nothing to stderr, so 2> $prStdErrFile leaves the temp file at 0 bytes. Get-Content -Raw on an empty file returns $null, and ($null).Trim() throws "You cannot call a method on a null-valued expression" under the script-level $ErrorActionPreference = 'Stop'.

Line 330 runs unconditionally on every iteration (even though $prStdErr is only read on the error path at line 335). So the throw fires on the happy path, gets caught at line 439, sets $hadProcessingFailure = $true, and continues — before any reaction, label removal, or AzDO trigger.

Concrete failure scenario: Scheduled scanner runs hourly. Agent emits trigger/skip for a real queued PR → line 330 throws → PR is never triggered, the s/agent-ready-for-rerun label is never removed (so the same PR is re-queued and re-fails forever), and the job exits 1 (red) on every run. This is strictly worse than round-2's 2>&1 regression, which only corrupted JSON when gh actually wrote to stderr.

Self-correction note: This regression was introduced by round-3's fix for round-2's 2>&1 finding. (Reference: gh-aw reviewer skill self-correction rule — treat as high-confidence even though only one reviewer ran the empirical reproduction; both other reviewers independently flagged the same null-trim bug from static analysis.)

Note $prJson = ($prOutput | Out-String).Trim() at line 329 is safe — Out-String returns "", never $null. Only line 330 is affected.

Fix: coerce to string before .Trim():

$prStdErr = ([string](Get-Content -Raw -LiteralPath $prStdErrFile -ErrorAction SilentlyContinue)).Trim()

([string]$null → ''.)

[PureWeen]

❌ Config Impact — pre-activation job has no read permissions; scanner will silently no-op or hard-fail

Flagged by: 3/3 reviewers (R1/R2 at ❌, R3 at ⚠️ — escalated to ❌ via consensus + author's own self-review)

The compiled rerun-review-scanner.lock.yml sets top-level permissions: {} (line 110), and the pre_activation job (line 1378) does not declare its own permissions: block. The frontmatter permissions: set above is applied by the gh-aw compiler to the agent job (lock.yml lines 376–379), not to pre_activation — per the gh-aw guide and confirmed by inspection of the generated lock file. Custom pre_activation jobs receive only the implicit metadata: read scope unless explicitly configured.

Query-RerunReadyPRs.ps1 (which the pre-activation step invokes) needs scopes beyond metadata:

• gh pr list → pull-requests:read
• gh api .../issues/{n}/labels / comments → issues:read
• gh api .../pulls/{n}/reviews / comments / commits → pull-requests:read / contents:read

GitHub's permissions: {} semantics — all unspecified scopes default to none, and GITHUB_TOKEN does not fall back to anonymous public read — means these endpoints return HTTP 403 even on a public repo. Practical effect either way is bad: a hard 403, or (because $ErrorActionPreference='Stop' does not trip on native non-zero exit codes) gh pr list yields empty output → candidates.json becomes an empty set → the scanner silently degrades to a permanent no-op that never triggers any rerun.

This concern was originally raised in the author's own self-review on this PR and is not addressed by round-3 (which only touched the two PowerShell files, not the workflow).

Fix (add to rerun-review-scanner.md and recompile):

jobs:
  pre-activation:
    permissions:
      contents: read
      issues: read
      pull-requests: read
    outputs:
      rerun_candidates: ${{ steps.rerun_context.outputs.candidates }}

[kubaflo]

Not applying — verified empirically that permissions: {} does not 403 on this public repo: run 27379324751 ran on the exact current HEAD 0e519c10 (where pre_activation inherits {}) with the round-4 $PSNativeCommandUseErrorActionPreference active, so a 403 would have thrown — yet it logged Wrote 1 rerun-ready candidate(s), meaning gh pr list + all follow-up gh api reads returned exit 0. Also, jobs.pre-activation.permissions is rejected by the compiler, and the documented on.permissions: lever silently drops the agent job's queue: max concurrency on gh-aw v0.77.5 (A/B-verified). Full evidence in the summary comment.

[PureWeen]

Adversarial multi-model review — PR #35874 (Round 3)

Methodology: 3 independent reviewers (round-3 commit f26427350d) with adversarial consensus and multi-round self-correction. Empirical reproduction in pwsh 7.4.5 for the critical regression.

Round-2 fix verification

Round-2 finding	Round-3 attempt	Status (3/3 reviewers)
⚠️ 2>&1 corrupts JSON on success path	Temp-file stderr redirect	✅ Original symptom resolved — but introduced a new regression (Finding A below)
⚠️ Systemic failures still report green	$hadProcessingFailure flag + exit 1	✅ Fixed — flag logic is sound; legitimate continue skips don't set it
💡 Test name overclaims	Test renamed; bot comment removed; dedicated normalization unit test added	✅ Fixed — Normalize-GitHubActorLogin now has direct coverage at Tests.ps1 lines 99–102

New findings (round 3)

❌ Finding A — Regression (Invoke-RerunReviewTrigger.ps1 line 330) — 3/3 reviewers, empirically reproduced
The fix for round-2's 2>&1 issue introduced a stricter regression: Get-Content -Raw on an empty file returns $null, and ($null).Trim() throws under $ErrorActionPreference='Stop'. Line 330 runs unconditionally on every iteration, so every successful gh api call now throws before processing — the scanner exits red on every run, no labels get removed, no AzDO triggers fire. See inline comment.

❌ Finding B — Config Impact (rerun-review-scanner.md line 60) — 3/3 reviewers, also flagged by author's own self-review
The compiled lock.yml has top-level permissions: {} and the pre_activation job lacks its own permissions: block. Query-RerunReadyPRs.ps1 needs contents/issues/pull-requests: read to call gh pr list and several gh api endpoints. Result: either HTTP 403 or silent empty-candidate sets (permanent no-op). Not addressed by round 3 — workflow files weren't touched. See inline comment.

Other reviewer observations (non-consensus)

💡 Testing gap — 1 reviewer noted that the entire foreach processing loop in Invoke-RerunReviewTrigger.ps1 (lines 285–446) has no test coverage; the test harness only exercises six isolated helper functions via AST extraction. All 57 unit tests pass even with Finding A present — that's why the regression sailed through. Consider adding one smoke test that stubs gh and runs the loop against a single candidate before merging this PR. (Single-reviewer observation, not a blocking finding.)

💡 Error fallback when both empty — 1 reviewer noted the ternary at line 335 ($prError = if ([string]::IsNullOrWhiteSpace($prStdErr)) { $prJson } else { $prStdErr }) produces an unhelpful error message if both stderr and stdout are empty (e.g., abrupt connection drop). Optional: add a "No output provided" fallback. (Single-reviewer observation.)

Test coverage assessment

The PR adds direct unit-test coverage for Normalize-GitHubActorLogin and Test-GhApiPrNotFound. The main processing loop in Invoke-RerunReviewTrigger.ps1 remains untested — see Testing gap above. The regression in Finding A would have been caught by a single smoke test that invokes the loop with a stubbed gh function returning JSON on stdout and nothing on stderr.

Prior review status

• Round 1 (2 findings) → addressed in round 2
• Round 2 (3 findings) → addressed in round 3 (with Finding A regression)
• Round 3 (this review): 2 ❌ blocking + 2 💡 single-reviewer observations

CI / build status

Out of scope for this review (per skill instructions). The user can re-trigger CI after the next push if needed.

Verdict

NOT READY TO MERGE. Two ❌ blockers from independent multi-model consensus:

1. Round-3 introduced a regression that breaks the happy path entirely (Finding A)
2. Long-standing pre-activation permissions issue remains unaddressed (Finding B)

Both are small, mechanical fixes. After they're resolved + the lock.yml is regenerated for Finding B, this PR should be in good shape — round-2 findings are genuinely resolved.

(This review uses event: COMMENT — never APPROVE or REQUEST_CHANGES per the adversarial reviewer skill.)

[kubaflo]

@PureWeen addressed the latest review comments in 0e519c1057b070d8c0cbf8c58078ba4c405c3a3c. I also reran the scanner workflow on the PR branch in dry-run mode and it completed successfully. Ready for re-review.

[PureWeen]

❌ Regression / Error Handling — $PSNativeCommandUseErrorActionPreference = $true leaks via dynamic scoping into dot-sourced helpers and defeats their documented graceful $LASTEXITCODE fallbacks (round-4 regression).

PowerShell preference variables are dynamically scoped, so this setting is inherited by every function called from this script — including those defined in the dot-sourced shared/Update-AgentLabels.ps1 at line 22. None of those helpers re-scope the preference.

Concrete failure path (verified in pwsh 7.4.5):

Query-RerunReadyPRs.ps1:98 calls Test-AgentReviewInProgressIsStale for any candidate carrying s/agent-review-in-progress. That helper is documented as a "cancellation safety net" and at lines 263-264 does:

$createdAtValues = @(gh api ".../events..." 2>$null)
if ($LASTEXITCODE -ne 0) {
    Write-Host "...could not inspect...treating as fresh"
    return $false
}

Under the new preference, a non-zero gh exit (transient 5xx, secondary rate limit) throws NativeCommandExitException at the gh api line before the line-264 fallback runs. 2>$null only suppresses stderr text — it does not prevent the exit-code throw. Net effect: one transient blip on one PR aborts the entire scanner run and writes zero candidates instead of skipping that PR.

The same problem applies to Get-IssueLabels (line 36 in this file) and to Get-AgentLabels/Remove-Label (which use the same 2>$null + $LASTEXITCODE pattern in the shared module).

Asymmetry hazard: Invoke-RerunReviewTrigger.ps1 does NOT set this preference, so the same shared helper now behaves differently depending on which entry-point called it — invisible to anyone reading the helper.

Fix options (any one):

1. Drop the global preference and put if ($LASTEXITCODE -ne 0) { throw ... } only after the gh pr list at line 84 (most predictable; doesn't reach into shared code).
2. Wrap the Test-AgentReviewInProgressIsStale and Get-IssueLabels call sites in try { } catch { continue } so a per-PR failure skips that PR.
3. Save/restore the preference inside the shared helpers that intentionally tolerate non-zero exits.

This finding was independently raised by 2/3 reviewers (Claude Opus 4.8 with empirical reproduction in pwsh 7.4.5, and Gemini 3.1 Pro).

[kubaflo]

Fixed in df5f314 via option 1 — dropped the script-wide $PSNativeCommandUseErrorActionPreference and added an explicit $LASTEXITCODE check right after gh pr list. The dot-sourced helpers keep their 2>$null + $LASTEXITCODE graceful fallbacks, and entry-point behavior (Query vs Invoke) is now symmetric. All 59 Pester tests pass. Full notes in the summary comment.

[PureWeen]

Round-4 adversarial review (3 models, 2/3 consensus)

Round 3 follow-up — status:

• ✅ Round-3 Finding A (Get-Content null-trim regression): FIXED. All 3 reviewers confirm. The new ConvertTo-TrimmedString helper (Invoke-RerunReviewTrigger.ps1:81-89) correctly handles [string]$null → '' and is now used at both prior .Trim() call sites. Verified empirically in pwsh 7.4.5 — Get-Content -Raw on an empty stderr file returns $null, and the helper safely returns ''. Unit tests genuinely exercise the function. Other .Trim() calls in the touched files already use the ([string]$x).Trim() cast idiom, which is null-safe.
• ❌ Round-3 Finding B (pre-activation read permissions): STILL OPEN. The pre_activation job in the compiled .lock.yml (line 1378) inherits top-level permissions: {} (line 110) because gh aw compile only attached the source-.md's top-level permissions to the agent job (lock lines 376-379). The pre_activation step runs Query-RerunReadyPRs.ps1 with GH_TOKEN: ${{ github.token }} but no issues: read / pull-requests: read scope. Round-4 did not touch .github/workflows/rerun-review-scanner.md. This is now open across rounds 3 and 4, and the author's own self-review flagged the same thing.

New round-4 finding (2/3 consensus, ❌):

Query-RerunReadyPRs.ps1:16 introduces $PSNativeCommandUseErrorActionPreference = $true. PowerShell preference variables are dynamically scoped, so this leaks into the dot-sourced shared/Update-AgentLabels.ps1 and defeats the documented 2>$null + $LASTEXITCODE graceful-fallback pattern in Test-AgentReviewInProgressIsStale (called from line 98) and Get-IssueLabels (called locally). One transient gh 5xx now aborts the whole scanner run instead of skipping the affected PR.

Also creates entry-point-divergent behavior: Invoke-RerunReviewTrigger.ps1 does not set this preference, so the same shared helper now behaves differently depending on who called it. See the inline comment for the full reproduction, fix options, and additional affected call sites.

Items checked and cleared (no issue):

• [version]'7.3' guard is correct; pwsh 7.4.5 default is False so the assignment is meaningful.
• gh pr list --json returns [] and exits 0 on zero matches — does not trip the new throw.
• ConvertTo-TrimmedString correctness — null/empty/whitespace cases all handled.
• The single-reviewer 💡 observations from round 3 (testing gap, two-empty error fallback) remain optional.

Recommendation: The Finding A fix is solid. Please address the new $PSNativeCommandUseErrorActionPreference scoping regression (any of the three fix options in the inline comment) and Finding B (.md/lock pre-activation permissions) before merge.

---

Adversarial multi-model review: Claude Opus 4.8 (R1), GPT-5.5 (R2 — LGTM, missed the new regression), Gemini 3.1 Pro (R3). 2/3 reviewers independently identified the dot-source preference leak; R1 reproduced it empirically. Posting as event: COMMENT to avoid stale blocking reviews.

[kubaflo]

@PureWeen Thanks for the thorough round-4 review. Pushed df5f314 — here's where the two open findings stand.

✅ $PSNativeCommandUseErrorActionPreference dynamic-scope leak — fixed in df5f314

Took option 1 from your inline comment: dropped the script-wide preference and instead check $LASTEXITCODE explicitly right after the gh pr list candidate query. This keeps the round-4 intent (a real candidate-query failure fails fast instead of looking like an empty queue) without leaking the preference into the dot-sourced helpers — so Test-AgentReviewInProgressIsStale / Get-IssueLabels keep their 2>$null + $LASTEXITCODE graceful fallbacks, and behavior is now symmetric regardless of entry point (Query vs Invoke). All 59 Pester tests pass.

❌ Finding B (pre-activation read permissions) — not applying; evidence below

The permissions: {} → HTTP 403 premise doesn't hold for this public repo:

• Run 27379324751 ran on the exact current HEAD 0e519c10, where pre_activation has no permissions: block (inherits top-level {}).
• That commit had $PSNativeCommandUseErrorActionPreference = $true active, so any non-zero gh exit (including a 403 from a missing scope) would have thrown and failed the step.
• The step instead logged Wrote 1 rerun-ready candidate(s) — i.e. gh pr list and every follow-up gh api call (labels, comments, reviews, commits) returned exit 0 under permissions: {}. GITHUB_TOKEN reads public PR/issue data fine on a public repo regardless of the scope block.

Separately, the suggested fix has a real downside. jobs.pre-activation.permissions is rejected by the compiler (only 'steps' and 'outputs' are allowed); the documented lever is on.permissions:. But on gh-aw v0.77.5, adding on.permissions: silently drops the agent job's concurrency: { group: gh-aw-copilot-…, queue: max } block (verified by A/B recompile — only the conclusion job's concurrency survives). So it would trade a non-existent permission problem for an unintended change to agent run serialization.

Net: reads demonstrably work, and the "fix" regresses concurrency, so I'm leaving the workflow as-is. If reads ever do start 403'ing, the right lever is on.permissions: — paired with a gh-aw concurrency check (or an upstream gh-aw fix).

Ready for re-review 🙏

[PureWeen]

Round-5 adversarial review — LGTM ✅

3/3 reviewers (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro) found no new issues in df5f314.

Confirmed fixed

• ✅ Round-4 $PSNativeCommandUseErrorActionPreference dynamic-scope leak — fully resolved. grep -rn "PSNativeCommandUseErrorActionPreference" .github/ returns zero hits. With $ErrorActionPreference='Stop' still set, the dot-sourced helpers' 2>$null + $LASTEXITCODE graceful fallback is restored — a transient gh 5xx no longer aborts the whole scanner. The new explicit $LASTEXITCODE check after gh pr list was empirically verified against all three capture shapes ([] zero-match, multi-line pretty JSON, single-line compact JSON) and the throw message safely interpolates only the constant label name and the integer exit code with no stderr/token/PII leakage.

Finding B — withdrawn ❌→✅

You're right and I was wrong. Independently re-verified your evidence:

• Run 27379324751 on 0e519c1057 shows all jobs (pre_activation, activation, agent, detection, safe_outputs, trigger_rerun_review, conclusion) with conclusion=success. The round-4 throw-on-any-nonzero-exit preference was active, so every gh pr list / gh api …/events|labels|comments|reviews read demonstrably returned exit 0 under permissions: {}.
• On public dotnet/maui, the implicit metadata: read plus the public REST endpoints make these reads work without explicit pulls: read / issues: read. I'd over-applied a private-repo intuition.
• The scanner only reads — the actual writes happen in trigger_rerun_review which has its own issues: write / pull-requests: write scopes.
• Your secondary concern about on.permissions: dropping the agent job's concurrency: { group: gh-aw-copilot-…, queue: max } block on gh-aw v0.77.5 is grounded — that block is confirmed at rerun-review-scanner.lock.yml:380-382. Trading a non-issue for a concurrency regression isn't worth it. If you've reproduced the drop reliably with a minimal repro, it'd be a worthwhile upstream gh-aw bug to file.

Verdict: Finding B is not actionable for this public repo. Withdrawn.

Non-blocking observation (pre-existing, not a round-5 finding)

💡 The per-PR enrichment reads inside the candidate foreach (Get-ActivityForPR, Get-CommitsForPR) have no $LASTEXITCODE guard. A transient gh api failure during enrichment silently yields empty activity/commits and the PR is evaluated with incomplete context. This is the intended critical-vs-best-effort split (the candidate query is critical, per-PR enrichment is best-effort) and predates round 4 — flagging purely for awareness in case you ever want stricter enrichment handling. Not a blocker.

---

Nothing left on my side. From a review perspective: ready to merge pending CI + human approval (no automated approval on principle, per event: COMMENT policy).

Adversarial multi-model review: 3/3 LGTM. R1 Claude Opus 4.8 (empirical pwsh 7.4.5 verification), R2 GPT-5.5, R3 Gemini 3.1 Pro.

[kubaflo]

/azp run

[azure-pipelines]

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

[PureWeen]

Round-6 adversarial review — LGTM ✅

3/3 reviewers (Reviewer 1/2/3) reviewed df5f31480c fresh against the same checklist as prior rounds. No new findings. The r5 LGTM holds.

What round 6 actually verified (beyond a re-skim)

• & gh api … 2> $file throw hypothesis ruled out empirically — confirmed pwsh 7.4.5 defaults $PSNativeCommandUseErrorActionPreference = $false; no script (including dot-sourced Update-AgentLabels.ps1) re-enables it. The native call does not throw; the 404/410 graceful-skip path is reachable.
• Out-String corrupting long single-line gh api JSON ruled out empirically — round-tripped an 11,185-char single-line JSON through @(& native) | Out-String | Trim → byte-identical, zero inserted newlines, ConvertFrom-Json succeeds.
• Author-activity gate verified end-to-end — Normalize-GitHubActorLogin reconciles GraphQL app/dependabot (from gh pr list) with REST dependabot[bot] (from gh api); idempotent under double-normalization; bot-authored PRs correctly yield 0 comment-evidence while commits still count.
• Test-GhApiPrNotFound — matches 404/410 only; correctly rejects 401/403/500/empty.
• Stale-PR path — $global:LASTEXITCODE = 0 + continue runs after the inner try/finally cleanup; clean skip with no $hadProcessingFailure propagation.
• gh-aw workflow restructure — activation needs pre_activation + gates on activated; trigger_rerun_review transitively ordered; rerun-candidates artifact name/path/RERUN_CANDIDATES_PATH all match; lock recompiled cleanly with v0.77.5. pre_activation inheriting read-only workflow permissions matches the empirically-validated result from run 27379324751.
• All 59 Pester tests pass; all 5 scripts parse clean.

Restated non-blocker (informational, not a finding)

Per-PR enrichment reads (Get-ActivityForPR / Get-CommitsForPR) in Query-RerunReadyPRs.ps1 lack $LASTEXITCODE guards — intended best-effort behavior; failing one PR's enrichment shouldn't kill the whole scan.

Methodology

3 independent reviewers (different model families) ran in parallel against the full diff and source. Findings reconciled via adversarial consensus. This is the 6th round on commit df5f31480c; no new commits since round 5's clean LGTM.

Recommendation: LGTM — ship it.