Optimize PR review pipeline workflow

[PureWeen]

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!

Optimizes the PR review pipeline workflow.

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

Or

• Run remotely in PowerShell:

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

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR restructures the ci-copilot pipeline so the “Post” work (posting gate comments, labels, and coordinating AI summary output variables) runs in a separate job, rather than inside the main Copilot review job, and updates the supporting scripts/docs accordingly.

Changes:

• Moves the Post phase out of the CopilotReview job into a new PostReview job that downloads CopilotLogs and prepares the expected CustomAgentLogsTmp layout.
• Updates UpdateAISummaryComment stage wiring to read aiSummaryReviewId from the new PostReview job output.
• Refactors Review-PR.ps1 sentinel enforcement to apply only to phases that require the prepared review worktree, adds a Pester test for this behavior, and updates pipeline security instructions to describe the new Post-job model.

Show a summary per file

File	Description
eng/pipelines/ci-copilot.yml	Moves Post into a dedicated job and rewires stage output dependencies for aiSummaryReviewId.
.github/scripts/Review-PR.Tests.ps1	Adds coverage for which phases require the prepared review worktree.
.github/scripts/Review-PR.ps1	Introduces Test-PhaseRequiresReviewWorktree and narrows sentinel checks to Gate/CopilotReview phases.
.github/instructions/ci-copilot-pipeline-security.instructions.md	Updates documented security rules/checklist for the new Post-job execution model.

Copilot's findings

• Files reviewed: 4/4 changed files
• Comments generated: 1

[kubaflo]

🔍 AI-generated review — 4-model adversarial security ensemble (Claude Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro · GPT-5.6 Sol), cross-pollinated and independently verified. Round 1.

🟡 NEEDS_DISCUSSION — a genuine security improvement, with one pre-existing gap now at the privileged boundary

The core refactor is sound and strengthens the trust boundary. Moving Post out of the PR-merge job into a separate, clean Microsoft-hosted PostReview job correctly isolates GH_COMMENT_TOKEN from PR-controlled code. I verified this myself and all four models agree.

Verified (SHIP-quality)

• Trust anchor holds. Pipeline is trigger: none / pr: none (dispatched with a PRNumber param); the PR is fetched only into a separate review worktree (git fetch origin pull/N/head, ~:1340). So PostReview's checkout: self at $(Build.SourceVersion) is a trusted main revision, and -TrustedScriptsDir is not passed → $ScriptsDir resolves to the clean checkout. No PR-controlled code runs in the token-bearing job.
• Artifact drives no verdict. Inline findings post as event=COMMENT only; the APPROVE veto keys off the trusted CopilotReview.RunGate.gateResult (fails closed if blank), and Prepare review results overwrites gate-result.txt with the normalized trustedGateResult before Post re-reads it. Summary posting is DEFERred to Stage 3.
• Defenses: reparse-point rejection before Copy-Item (symlinks/junctions rejected; no practical TOCTOU on the immutable artifact); git diff --exit-code fails closed via $LASTEXITCODE; gateResult validated against a fixed allow-list.
• Cross-job plumbing correctly renamed CopilotReview.RunPost.* → PostReview.RunPost.* (stage condition + job var); RunGate.gateResult intentionally stays on CopilotReview; no dangling refs.
• Instructions match the code (Rule 4 clean-Post-checkout, Rule 6 cross-job output-vars/artifacts); no rule weakened. Tests 24/24 incl. the new Test-PhaseRequiresReviewWorktree.

⚠️ The remaining gap (3 of 4 models: Gemini, Opus, Sol) — ##vso injection via untrusted artifact at RunPost

RunPost now holds GH_COMMENT_TOKEN and runs Review-PR.ps1 -Phase Post, which echoes attacker-influenced artifact content unsanitized to a stdout that the Azure agent scans for logging commands:

• post-inline-review.ps1:163 (and the raw-dump at ~:113 / dropped-line print ~:232) — prints finding-derived path/raw-JSON.
• Review-PR.ps1:2470 — echoes the unvalidated winner field of winner.json.

PR-controlled code (an MSBuild target during the agent's build/test in the CopilotReview phase) can plant, e.g.:

{"winner":"x\n##vso[task.setvariable variable=aiSummaryReviewId;isOutput=true]123","isPRFix":true}

When echoed, the agent interprets the newline-prefixed directive and spoofs PostReview.RunPost.aiSummaryReviewId, which Stage 3 then consumes on privileged GitHub API paths. This violates the repo's own Rule 7 ("strip ##vso[...] from PR-controlled stdout").

Why this is NEEDS_DISCUSSION, not a block: it's pre-existing (both sites are unchanged by this diff; git blame puts :2470 at b71adea6, and the identical exposure existed in the old in-job Post step) — this PR neither introduces nor worsens it. But it's the one remaining hole in the exact trust boundary this PR builds: the "trusted" PostReview job still ingests untrusted data into a ##vso-interpreting stdout. Since you're already in this code and the repo has a ready helper, sealing it here (or in an immediate follow-up) would complete the isolation.

Suggested fix: wrap the RunPost echo sites in the existing ConvertTo-AzdoSafeConsole (already applied to agent stream output at Review-PR.ps1:1220-1277), and regex-validate review IDs (^\d+$) before use in GitHub API calls.

CI

license/cla pass; maui-pr pending (this PR touches eng/pipelines/.github/scripts, so it runs). Not APPROVE-eligible while CI is pending, and the open discussion item above is the reason for 🟡 regardless.

---

Verdict: NEEDS_DISCUSSION. Confidence: high. The refactor is a real, verified isolation improvement; the pre-existing ##vso-via-artifact path at the new privileged RunPost boundary is the remaining gap — your call whether to close it in this PR or a fast follow-up. Not an approval; posting as comments per policy.

[kubaflo]

⚠️ ⚠️ Pre-existing ##vso injection reachable at this new privileged boundary (3 of 4 models: Gemini/Opus/Sol).

This RunPost step holds GH_COMMENT_TOKEN and runs Review-PR.ps1 -Phase Post, which echoes attacker-influenced artifact content unsanitized to a stdout the Azure agent scans for logging commands:

• post-inline-review.ps1:163 (+ raw-dump ~:113, dropped-line ~:232) — prints finding-derived path/raw JSON.
• Review-PR.ps1:2470 — echoes the unvalidated winner field of winner.json.

PR-controlled code (an MSBuild target during the CopilotReview build/test) can plant e.g. {"winner":"x\n##vso[task.setvariable variable=aiSummaryReviewId;isOutput=true]123",...}. When echoed, the agent interprets the newline-prefixed directive and spoofs PostReview.RunPost.aiSummaryReviewId, which Stage 3 consumes on privileged GitHub API paths. Violates the repo's own Rule 7.

Not a block: pre-existing (both sites unchanged by this diff; git blame puts :2470 at b71adea6; identical exposure in the old in-job Post) — this PR neither introduces nor worsens it. But it's the one remaining hole in the trust boundary this PR builds: the isolated "trusted" PostReview job still pipes untrusted data into a ##vso-interpreting stdout.

Fix: wrap these echo sites in the existing ConvertTo-AzdoSafeConsole (already used on agent stream output at Review-PR.ps1:1220-1277) and regex-validate review IDs (^\d+$) before GitHub API use. Sealing it here (or a fast follow-up) completes the isolation.