Note: posting as a formal review since the previous two issue comments with the same content were deleted shortly after posting. HEAD is unchanged at 82679867 since 10:09:19Z (no new commits, identical blob SHAs 9f8a1fb… / 698b0d3…), so the findings below are unchanged from my prior comments — included again here for completeness so the PR has a durable record of the analysis.

The token-scoping refactor is the right shape: per-task env: blocks, two distinct trusted phases (Setup/Post), an untrusted Gate with no env, and a CopilotReview phase that gets only COPILOT_GITHUB_TOKEN. persistCredentials: false, removal of the gh auth login step, and --secret-env-vars=GH_TOKEN,COPILOT_GITHUB_TOKEN,GITHUB_TOKEN are all good belt-and-braces moves. The four issues below are the ones I'd want fixed before this lands.

🔴 1. trusted-scripts/ is writable by PR code

eng/pipelines/ci-copilot.yml 567-572 (Setup, Task 1):

TRUSTED="$(Agent.TempDirectory)/trusted-scripts"
mkdir -p "$TRUSTED"
cp -r .github/scripts "$TRUSTED/scripts"
cp -r .github/skills  "$TRUSTED/skills"
cp -r eng/scripts     "$TRUSTED/eng-scripts"

Agent.TempDirectory is owned by the agent user, and nothing makes the copies read-only. Task 2 (Gate) runs dotnet build / dotnet test on PR-controlled code and can chmod +w and overwrite anything under $TRUSTED/ — for example scripts/post-ai-summary-comment.ps1 or eng-scripts/detect-ui-test-categories.ps1. Task 4 (Post) then re-invokes Review-PR.ps1 -Phase Post from that directory with GH_TOKEN (line 704) and runs the modified script.

So an attacker PR with a dotnet test payload that rewrites $AGENT_TEMPDIRECTORY/trusted-scripts/... re-acquires GH_COMMENT_TOKEN in Task 4 — defeating the whole partition.

Fix: lock the directory after copy.

cp -r .github/scripts "$TRUSTED/scripts"
cp -r .github/skills  "$TRUSTED/skills"
cp -r eng/scripts     "$TRUSTED/eng-scripts"
chmod -R a-w "$TRUSTED"     # at minimum
# or better: own as a different user (e.g. root via sudo) and chmod 0555

Even better, copy into a path outside the agent user's writable space (e.g. /opt/maui-ci-trusted/$(Build.BuildId), root-owned via the image bootstrap).

🔴 2. $gateOutputDir referenced in Setup-only path → Join-Path $null crash

.github/scripts/Review-PR.ps1 306-314:

if ($Phase -eq 'Setup') {
    $sentinelDir = if ($TrustedScriptsDir) { Split-Path $TrustedScriptsDir -Parent } else { $gateOutputDir }
    "OK" | Set-Content (Join-Path $sentinelDir "setup-complete") -Encoding UTF8
    ...
    exit 0
}

$gateOutputDir is only defined at line 575, inside if ($runGate) { ... }. In CI this happens to work because -TrustedScriptsDir is always passed, so the first branch is taken. But the comment block at lines 91-93 explicitly advertises a "no -Phase for backward-compat for local development" mode, and pwsh Review-PR.ps1 -Phase Setup -PRNumber 1 (no trusted dir) hits the else branch and resolves $gateOutputDir to $null, then Join-Path $null "setup-complete" throws.

Fix: make the fallback well-defined, e.g.

$sentinelDir = if ($TrustedScriptsDir) {
    Split-Path $TrustedScriptsDir -Parent
} else {
    Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent"
}
New-Item -ItemType Directory -Force -Path $sentinelDir | Out-Null

Same pattern would help line 666 / 786 where $gateVerdictDir has the same fallback.

🟠 3. Tier 3 category refresh deleted, never reinstated

Review-PR.ps1 829-831:

# NOTE: Tier 3 category refresh (re-running detect-ui-test-categories.ps1 with AI
# categories from pre-flight) has been moved to the Post phase so that PR-controlled
# scripts don't run in a task that has COPILOT_GITHUB_TOKEN in scope.

The note is correct about why it was removed from CopilotReview, but the refresh code was never actually added to the if ($runPost) { ... } block (lines 840-897) — only the AI-Summary post and label application live there. Net effect: when the agent writes ai-categories.md, nothing reads it back, and **Detected UI test categories** in the AI Summary comment will reflect only Tier-1/2 (filename heuristics), missing the AI tier on every PR.

Either (a) re-add the refresh block at the top of if ($runPost), reading $aiCategoriesFile and re-running detect-ui-test-categories.ps1 -AiCategories ..., or (b) drop the misleading NOTE and document that Tier 3 is intentionally gone.

🟠 4. ##vso[] filter is bypassable

ci-copilot.yml 615 / 645:

... | sed 's/^##vso\[/##_vso[/'

The Azure DevOps agent matches logging commands with surprisingly forgiving rules — leading whitespace, leading \r (common in Windows-encoded output and ANSI-colored streams), and embedded ANSI escapes can all let a ##vso[...] slip past ^##vso\[. Examples that bypass this regex but are still parsed by the agent on at least some configurations:

\r##vso[task.setvariable variable=GH_TOKEN;isSecret=false]leak
   ##vso[task.prependpath]/tmp/evil
\e[0m##vso[task.logissue type=error]anything

Tighter version:

... | tr -d '\r' | sed -E 's/^[[:space:]]*##vso\[/##_vso[/'

(Strip \r first, then anchor after optional whitespace.) Worth adding to Setup and Post too — both echo PR-derived content (Setup runs the merge, Post echoes gate/content.md and friends, both written by Gate/CopilotReview which were untrusted).

🟡 Smaller things

• Setup failure → no PR comment, just a red X. If Task 1 throws (merge conflict, gh transient failure), the sentinel isn't written, Tasks 3 and 4 exit 0 silently (lines 628-637, 675-678), and the PR author sees "maui-pr failed" with no AI Summary comment to explain. The job result is at least red because of Task 1, so this is UX, not security — but consider posting a "Setup failed: $reason" comment from a small failure-only step that still has GH_COMMENT_TOKEN.

• Cross-phase state via working tree. gate-result.txt is persisted to Split-Path $TrustedScriptsDir -Parent (good — outside the working tree), but ai-categories.md and gate/content.md live under CustomAgentLogsTmp/PRState/... in the working tree, and Gate runs PR code that can rewrite either. Post should treat anything it reads from there as untrusted (it's posted verbatim into a PR comment, so XSS-in-Markdown / spoofed gate verdict are both reachable). Worth a sanitization pass when reading.

• cp -r CustomAgentLogsTmp $(Build.ArtifactStagingDirectory)/copilot-logs/ in Task 4. The comment "exclude .copilot session state — may contain auth metadata" is spot on for ~/.copilot, but the same caveat applies to CustomAgentLogsTmp/ — Copilot's intermediate output files there can include tool transcripts. If they're meant to be public artifacts, fine; if not, strip them before publish.

• Setup also has GH_TOKEN and runs git merge on PR code. Out of scope for this PR, but worth noting: a malicious .gitattributes with a merge= driver pointing at a script in the PR can run that script during merge with GH_TOKEN in env. Mitigations: git -c core.attributesFile=/dev/null merge ... or git config merge.<driver>.driver false before the merge.

🟢 What I really like

• Per-task env: partition with # env: deliberately empty on Gate. Crystal clear, easy to audit, easy to break the wrong way and notice in review.
• persistCredentials: false + removal of gh auth login (no ~/.config/gh/hosts.yml left on disk for dotnet subprocesses to read).
• --secret-env-vars=... on the copilot invocation as defense-in-depth even after the env: scoping.
• Sentinel-based gating between tasks is the right pattern for "did Setup succeed?" (just needs the not-silent-on-failure tweak above).
• Phase routing in PowerShell with backward-compat for local runs — the $run<Phase> flags read very nicely.

Summary

Happy to elaborate on any of these. The overall direction is great — the env-scoping refactor was overdue.

🔍 Adversarial PR Review

Methodology: 3 independent reviewers with adversarial consensus
CI: ✅ 30/30 checks passing

Summary

Verdict: 5 blocking issues, 2 warnings. The security architecture (per-task env scoping) is the right direction, but the implementation has gaps that undermine the security boundary — particularly #1 and #2 which allow Gate-phase PR code to tamper with files that Post executes with GH_TOKEN.

⚠️ Author's Claimed Fixes Not Present

The author comment states all 4 issues from kubaflo's review are "addressed in current HEAD (8267986)." All 3 reviewers independently confirmed none of the 4 fixes are in the code on disk. The commit hash is unchanged.

Discarded Findings (1/3, no consensus)

• setup-complete sentinel writable by Gate (single reviewer)
• $testDetectScript uses $PSScriptRoot (single reviewer)
• Post applies gate-failed label when gate never ran (single reviewer)
• $reviewBranch checkout warns but continues (single reviewer)
• post-ai-summary-comment.ps1 called twice (single reviewer — intentional idempotent design)

Pull request overview

This PR refactors the ci-copilot Azure DevOps pipeline to reduce secret exposure by splitting the previous single “Run PR Reviewer Agent” step into distinct phases with tightly scoped env: blocks, and updates Review-PR.ps1 to support per-phase invocation via a new -Phase parameter (plus -TrustedScriptsDir) so trusted scripts can be copied before merging PR code.

Changes:

• Split the pipeline into 4 tasks (Setup / Gate / CopilotReview / Post) with scoped environment variables and a “trusted-scripts” copy staged in $(Agent.TempDirectory).
• Add phase routing to .github/scripts/Review-PR.ps1, including log file suffixing per phase and cross-task state handoff via sentinel/result files.
• Harden Copilot invocation by passing --secret-env-vars and move gate verdict persistence to $(Agent.TempDirectory) to avoid PR-writeable state.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

          - bash: |
              echo "Validating PR Number parameter..."
              if [ -z "${{ parameters.PRNumber }}" ]; then
                echo "##vso[task.logissue type=error]PRNumber parameter is required"
                exit 1
              fi

LGTM!