Fix /review trigger lock label permissions

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

Summary

• Gives the normal /review trigger job pull-requests: write, matching the rerun job, so it can apply the s/agent-review-in-progress lock label before dispatching the AzDO pipeline.
• Logs the actual gh api response when adding a label fails, instead of suppressing stderr and returning only a generic failure.

Why this broke

/review -b feature/enhanced-reviewer was still parsed correctly; the failure was introduced by a default-branch workflow change. The last successful trigger on PR #33365 ran on workflow SHA c389325e, before the Set review in-progress lock step existed. After main moved to dd5b6d2, the normal /review path started applying s/agent-review-in-progress before dispatching AzDO, but that job still only requested pull-requests: read.

The rerun path already requested pull-requests: write and could apply labels successfully. This PR aligns the normal /review path with that permission so the lock label can be applied and the AzDO pipeline dispatch can proceed.

Validation

• git diff --check
• Parsed .github/workflows/review-trigger.yml with Ruby YAML
• Fake gh success/failure checks for Add-Label
• Invoke-Pester .github/scripts -CI
• Invoke-Pester .github/skills -CI

Fixes

• Fixes /review -b feature/enhanced-reviewer failing before AzDO dispatch at Set review in-progress lock.

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

Or

• Run remotely in PowerShell:

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

[PureWeen]

Adversarial code review — 3 independent reviewers in parallel, with consensus and empirical fact-checking of disputed claims.

Verdict

The PR does what it says: brings trigger-review's pull-requests: scope into parity with mark-rerun-ready so the Set review in-progress lock step can apply s/agent-review-in-progress, and surfaces the actual gh api failure body instead of returning a generic boolean. Permission change is correct (label endpoints on PRs require pull-requests: write; issues: write alone is not enough for PRs) and remains gated by the existing actor permission check (write/maintain/admin only on issue_comment).

One ⚠️ finding posted inline against the new error-logging path — see Update-AgentLabels.ps1:143. It's a small bug in the new code that partially defeats the stated goal of logging the real gh api response; fixable in one line by switching to Out-String.

Findings

• ⚠️ Error Handling — Update-AgentLabels.ps1:143 — [string] cast on stderr ErrorRecord injects System.Management.Automation.RemoteException for blank stderr lines. Empirically verified locally with a 3-line stderr repro. 1/3 reviewers; included because the claim was confirmed by direct test (per skill rule 3d).

No ❌ blocking issues found.

Discarded (1/3, not confirmed)

• 💡 Update-AgentLabels.ps1:147 — Substring(0, 1000) could split a Unicode surrogate pair. Theoretical; gh api error bodies are essentially always ASCII JSON. Discarded.
• 💡 Update-AgentLabels.ps1:171 (Remove-Label) — silent error swallowing remains. Out of scope for this PR (line not in diff). Discarded.
• 💡 Update-AgentLabels.ps1:143 (forward-looking) — gh --verbose / GH_DEBUG could put auth headers into the error log. Not a defect today; gh api does not echo GH_TOKEN in default failure output. Discarded.

Permission widening (security-sensitive)

Reviewed under the 2/3 consensus rule. All three reviewers concluded the widening is safe: the Check actor permission step (lines 156–167) rejects anyone without write/maintain/admin before any label operation runs, and the sibling mark-rerun-ready job already has pull-requests: write. No privilege escalation introduced. ✅

Test coverage

No Pester test directly exercises Add-Label, so the ErrorRecord issue above would not be caught by Invoke-Pester .github/scripts -CI. The functional behavior (return value contract) is unchanged for all ~10 callers — none of them parse stdout — so the silent-success path is fine.

Prior reviews / threads

No prior human reviews on this PR. One auto-generated bot comment (dogfood instructions). No unresolved threads.

Methodology: 3 independent reviewers with adversarial consensus. Disputed 1/3 finding above was promoted after empirical verification rather than follow-up vote, since the underlying PowerShell stream-merging behavior is deterministic and locally testable.

[PureWeen]

Adversarial code review — round 2 (3 independent reviewers in parallel).

Verdict

No new findings. The round-2 commit (8736cbd7) correctly addresses the prior ⚠️: ($output | Out-String).Trim() preserves native gh api stderr text — including blank lines — without injecting System.Management.Automation.RemoteException literals for empty ErrorRecords.

Empirically re-verified locally:

• Real 403 stderr (JSON body + blank line + HTTP summary) round-trips cleanly through Out-String.
• Out-String on native command output via 2>&1 does not wrap at 80 columns (250-char line stays 250 chars).
• Empty stderr → IsNullOrWhiteSpace correctly triggers the "gh api exited with code N." fallback.
• Boolean return contract preserved; $LASTEXITCODE captured before Out-String runs.

Permission widening (pull-requests: read → write) remains correct and gated by the existing actor permission check — re-confirmed against GitHub's published Actions permission model (PR-targeted operations via the /issues/{n}/labels endpoint require the pull-requests scope; issues: write alone is not enough for PRs).

Clean to merge from a code-review standpoint, pending maintainer sign-off and CI.

Methodology: 3/3 reviewer consensus, multi-round self-correction rule applied — no reviewer found grounds to revert the round-2 change.

[kubaflo]

@PureWeen The prior review feedback is addressed in 8736cbd and the thread is resolved. Ready for re-review.