Address post-merge review findings on the PR finalize apply step

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

Description of Change

Follow-up to #36849. That PR merged with two review findings still open, so both are live on main today. This closes them.

1. The failure path in STEP 5.5 was never sanitized — flagged by the Copilot reviewer in round 2 on Review-PR.ps1:2416, after the round-1 fixes had already landed. This one is a real unaddressed finding, not a nitpick.

The apply step's catch block wrote the raw exception to the console:

Write-Host "  ⚠️ Failed to apply PR title/description (non-fatal): $_"

That exception can carry agent-authored text from content.md, which in turn derives from the PR title. So an AzDO logging command could reach stdout through the error path — reintroducing the exact injection class the rest of #36849 closed. Every other PR-derived console value in Review-PR.ps1 already routes through ConvertTo-AzdoSafeConsole; this one was simply missed.

Verified the fix defangs a payload rather than assuming it:

UNSANITIZED: boom ##vso[task.setvariable variable=GateFailed]false
SANITIZED  : boom ## vso[task.setvariable variable=GateFailed]false

## vso[ is no longer parsed as a command, so the payload cannot mask a failing gate at ci-copilot.yml:966.

2. The body file used a predictable temp path — @kubaflo's optional hardening note on the round-2 approval.

The body was written to pr-finalize-body-<PR>.md in the system temp dir. I confirmed Set-Content -LiteralPath follows a pre-existing symlink and writes through to its target, which makes a deterministic name a write-through primitive.

New-ExclusiveTempFile now creates a randomly-named file via New-Item without -Force, so it fails closed if anything already occupies the path. I verified New-Item raises IOException on a pre-planted symlink and leaves the target file untouched — the mitigation depends on that behaviour, so it is pinned by a test rather than assumed. The helper also prefers AGENT_TEMPDIRECTORY when the pipeline sets it, keeping the file on agent-scoped storage per rule 5 of ci-copilot-pipeline-security.instructions.md.

Worth stating plainly, since @kubaflo raised it himself: this second one is defence in depth, not a live exploit. Its precondition — arbitrary filesystem write as the agent user before Task 4 — already confers strictly greater capability than the vector it enables. It was correctly filed as non-blocking; it is cheap, so it is worth doing.

Issues Fixed

Follow-up to #36849 — no separate issue.

Testing

Apply-PRFinalize.Tests.ps1 goes from 30 to 35 tests. The new cases cover placement inside AGENT_TEMPDIRECTORY, fallback when it is unset, tolerance of a stale/missing value, path uniqueness across calls, and a symlink-write-through regression.

• 97/97 pass across Apply-PRFinalize.Tests.ps1, Review-PR.Tests.ps1, and Post-AISummaryComment.Tests.ps1 (baseline was 92/92).
• All three modified scripts parse-check clean.
• Exercised end to end against real PR data with a stubbed gh, confirming the randomized file reaches --body-file with the correct content and is cleaned up afterward:

--title -> [inflight regression][iOS] CarouselView2: Stop internal recenter scrolls...
--body-file -> /var/folders/.../pr-finalize-body-36753-gcjfaxkg.mgk.md (exists=yes)
body first line: <!-- Please let the below note in for people that find this PR -->

Note that maui-pr reports skipping on script-only PRs because of path filters, so the local Pester suites are the meaningful gate here.

Thanks to @kubaflo for the adversarial review on #36849 — the round-2 approval note is what surfaced the second item.

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

Or

• Run remotely in PowerShell:

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

[azure-pipelines]

Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Pull request overview

This PR hardens the /review pipeline’s PR-finalize “apply” step by ensuring failure-path console output is AzDO-safe and by generating a non-predictable temp file for gh pr edit --body-file, with accompanying Pester coverage.

Changes:

• Sanitize the STEP 5.5 exception message in Review-PR.ps1 to prevent AzDO logging-command injection via error text.
• Introduce New-ExclusiveTempFile and use it for the PR body temp file in apply-pr-finalize.ps1.
• Extend Apply-PRFinalize.Tests.ps1 with new tests for temp-file behavior and AGENT_TEMPDIRECTORY handling.

Show a summary per file

File	Description
.github/scripts/Review-PR.ps1	Sanitizes the STEP 5.5 catch-path console output via ConvertTo-AzdoSafeConsole.
.github/scripts/apply-pr-finalize.ps1	Adds New-ExclusiveTempFile and uses it to create a randomized --body-file path; cleans up conditionally.
.github/scripts/Apply-PRFinalize.Tests.ps1	Adds Pester tests for New-ExclusiveTempFile behavior and temp directory selection.

Copilot's findings

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

[kubaflo]

Round 1 — both security fixes are complete ✅ (one test-quality gap to tighten)

Independent adversarial pass (Claude Opus 4.8 + GPT‑5.6‑sol), every claim reproduced with pwsh on the worktree. Both follow‑up fixes from #36849 are complete and verified:

• Fix 1 — STEP 5.5 failure‑path sanitization (Review-PR.ps1:2418): ConvertTo-AzdoSafeConsole is defined at the same top‑level scope (:649) and in scope at the catch. Dot‑sourcing the real function and throwing an exception containing CR/LF, ##vso[…], and ##[…] emitted only ## vso[…] / ## […] — the payload is defanged. Audited the whole apply path: every PR‑derived Write‑Host sink is sanitized (apply-pr-finalize.ps1:310,334,370,377,400,403), no gaps.
• Fix 2 — New-ExclusiveTempFile (apply-pr-finalize.ps1:257): empirically confirmed on macOS that New-Item -ItemType File (no -Force) refuses both an existing and a dangling pre‑planted symlink (IOException, target untouched), uses a ~55‑bit random name, retries finitely then throws with no predictable fallback, guards AGENT_TEMPDIRECTORY with Test-Path, and sanitizes an injected temp‑dir error. The passive pre‑plant primitive from the round‑2 note is genuinely closed.

🟡 One thing to fix before merge (LOW, but it undercuts this PR's own goal)

The symlink regression test is vacuous (Apply-PRFinalize.Tests.ps1:356, "writes to the new file rather than through a pre-planted symlink"). It creates an unrelated secret.txt, asks the helper for an already‑created random file, and writes to it — it never plants a symlink at the candidate path, and "two different paths" proves uniqueness, not unpredictability. Proven vacuous by mutation: replacing New-ExclusiveTempFile with a predictable‑name, Set-Content write‑through implementation still passes the suite 35/35; only when the mutant's known first path is pre‑planted does the target flip from ORIGINAL to the injected content. So the test that's supposed to guard finding #2 would not catch a regression of finding #2.

Concrete fix: inject a seam/mock that forces known candidate names, then in the test plant both an existing and a dangling symlink at that name and assert (a) the helper refuses / the target is unchanged, and (b) exactly 5 attempts then throw with no fallback. That turns the mutant red.

Verdict

The security fixes themselves are done and independently verified — nice, focused follow‑up. Leaving this as a comment, not a block: please tighten the vacuous symlink regression test so the fix stays protected against future regressions, and I'm happy to re‑review for approval. (Also minor, non‑blocking: the STEP 5.5 sanitization is a correct backstop, though the child script already sanitizes its own output, so the in‑code comment slightly overstates current reachability.)

_{Multi‑model adversarial review (Claude Opus 4.8 + GPT‑5.6‑sol); sanitizer + symlink‑refusal behavior and the vacuous‑test mutation all reproduced with pwsh before posting.}

[PureWeen]

Thanks @kubaflo — the vacuous-test finding is correct and it's the most useful thing anyone could have caught here. The Copilot reviewer flagged the same test independently, so two reviewers converged on it.

I reproduced your mutation before fixing anything. Swapping New-ExclusiveTempFile for a predictable-name Set-Content write-through implementation — the exact vulnerability the helper exists to close — passed 35/35. You're right about why: the test created an unrelated secret.txt and never planted a symlink at a path the helper would actually try, so "two different paths" proved uniqueness, not unpredictability. A test guarding finding #2 that can't detect a regression of finding #2 is worse than no test, because it looks like coverage.

Fixed with the seam you suggested — an optional -NameGenerator so a test can force known candidates and plant a symlink at the precise path the helper will attempt. Six new tests:

Test	Pins
refuses a pre-planted symlink	target unchanged, link still a link
refuses a dangling symlink	target never created
every candidate occupied	throws, no predictable fallback
attempt count	exactly MaxAttempts, then stop
non-collision failure	surfaces immediately, not retried away
AGENT_TEMPDIRECTORY is a file	falls back instead of failing

The same mutant now fails all six (was 35/35 green):

FAILED: refuses a pre-planted symlink and leaves its target untouched
FAILED: refuses a dangling pre-planted symlink rather than creating its target
FAILED: throws instead of falling back to a predictable path when every candidate is occupied
FAILED: tries exactly MaxAttempts candidates before giving up
FAILED: surfaces a non-collision failure immediately instead of retrying it away
FAILED: ignores AGENT_TEMPDIRECTORY when it points at a file rather than a directory

Worth noting the first draft of these tests failed for a real reason — $i++ inside the generator scriptblock mutates a local copy, so every attempt re-requested the planted name and the helper correctly exhausted. Annoying, but a good sign: the tests are now sensitive to actual behaviour rather than passing regardless. Fixed with $script:-scoped counters and a comment explaining why.

On your minor note — you're right and I've corrected it. The child does sanitize its own console output, and its single throw carries only $baseDir and an [int] PR number, so no PR-derived text reaches that path today. My comment implied a live path. It now says plainly that this is a backstop against a future throw that quotes the recommendation, not a reachable vector. Overstated security comments are their own hazard.

Also took the three remaining Copilot inline points:

• Test-Path -PathType Container, so a stale AGENT_TEMPDIRECTORY pointing at a file falls back instead of failing later inside New-Item.
• Retry narrowed to IOException (occupied path). DirectoryNotFoundException derives from IOException, so it's caught first and rethrown — a missing base dir never resolves by picking another name. Access-denied and friends propagate unwrapped rather than being masked by the generic "after N attempts" message. Types confirmed empirically:

existing symlink  -> IOException                  | already exists
dangling symlink  -> IOException                  | already exists
read-only dir     -> UnauthorizedAccessException  | access denied
missing dir       -> DirectoryNotFoundException   | could not find path

• .SYNOPSIS no longer claims it "fails if the path already exists" — it skips an occupied path and only throws once attempts are exhausted.

Validation: 35 → 40 tests here, 102/102 across the three affected suites, all scripts parse clean, and the end-to-end stubbed-gh run still reaches --body-file with correct content and cleans up. Ready for re-review whenever you have a minute.

[Copilot]

Copilot's findings

Comments suppressed due to low confidence (1)

.github/scripts/apply-pr-finalize.ps1:299

• New-ExclusiveTempFile constructs a concrete filesystem path and should use -LiteralPath (not -Path) when creating the file. Using -Path allows wildcard interpretation if $Prefix ever contains wildcard characters, and it’s inconsistent with the surrounding use of -LiteralPath for file operations in this script.

            $file = New-Item -ItemType File -Path $candidate -ErrorAction Stop

• Files reviewed: 3/3 changed files
• Comments generated: 0 new

[kubaflo]

Adversarial re-review (round 2) — Opus 4.8 + GPT-5.5 + Gemini 3.1 Pro + GPT-5.6 Sol, verified against 8dd0f7f. The r1 residual is fixed — LGTM. ✅

✅ r1 residual (vacuous symlink test) — GENUINELY FIXED

My r1 COMMENT flagged one issue: the New-ExclusiveTempFile symlink-regression test was vacuous (a predictable-name write-through impl still passed 35/35). That's now closed.

I re-verified empirically (pwsh 7.5.4 / Pester 5.7.1, head files co-located, mutation testing):

• The r1-equivalent write-through mutant (keeps the $Prefix-$(& $NameGenerator).md candidate template, swaps exclusive-create → Set-Content) is now caught directly by the two new symlink tests — the old "35/35 pass" vacuousness is gone.
• No write-through / predictable-name mutant I built survives the suite: passing the exhaustion-throw + exactly-5-attempts tests forces a mutant to honor the -NameGenerator seam, and passing the symlink tests while honoring the seam requires skipping pre-existing symlinks = the secure behavior. The suite is collectively non-vacuous.
• Head suite: 40/40 pass; scripts parse clean.

✅ Delta claims verified

• DirectoryNotFoundException : IOException → the typed-catch ordering (DNFE before IOException) is correct and necessary; UnauthorizedAccessException is not an IOException, so it surfaces immediately. Strict improvement over r1's blanket catch.
• New-Item (no -Force) refuses a pre-existing regular file, a valid symlink (target intact), and a dangling symlink (target not created) via IOException → retry. -Force does write through, so "no -Force" is load-bearing and now regression-tested.
• -PathType Container only adds a graceful fallback when $env:AGENT_TEMPDIRECTORY points at a file; an unwritable-but-existing dir still surfaces access-denied (no silent world-writable fallback).
• No production caller passes -NameGenerator — sole call site (apply-pr-finalize.ps1:405) uses the default random generator; $Prefix is [int]-derived. PR content influences file content, never the path.
• Review-PR.ps1 change is comment-only; ConvertTo-AzdoSafeConsole unchanged.

Confirmed on both macOS and Linux (exception types are .NET runtime types, message strings are portable .NET resource strings).

💡 Optional, non-blocking follow-ups (test quality only — not required for merge)

• The two symlink tests lean on the exhaustion/uniqueness tests as backstops rather than standing fully alone; asserting the -NameGenerator advanced past the planted index would make them self-sufficient.
• One test asserts on the localizable *Could not find a part of the path* message — asserting the exception type instead would be robust on a non-en-US agent (AzDO agents are en-US, so low practical risk).
• New-Item -Path (wildcard-interpreting) vs -LiteralPath used elsewhere — cosmetic; inputs are wildcard-free.

None of these is a security defect or a behavior bug.

Disposition

Unanimous READY across all four models; the r1 residual is proven fixed via mutant-kill; no blocking issues. Pester (.github/scripts) is green; maui-pr correctly skips (scripts-only). Approving. 👍

_{🤖 Adversarial ensemble — Opus 4.8 + GPT-5.5 + Gemini 3.1 Pro + GPT-5.6 Sol; residual-fix re-verified by the orchestrator running the suite against hand-built mutants at 8dd0f7f.}