Fix Copilot PR-reviewer prompts: unauthenticated gh reality + mandatory inline-findings.json write

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

What & why

Fixes to the Copilot PR-reviewer pipeline, driven by analyzing 64 recent maui-copilot PR-reviewer CI sessions (most-recent run per PR, last 7 days). Three independent analysis agents mined disjoint batches and converged on the same top two systemic issues — strong signal these are structural, not anecdotal. Fixes #1 and #2 are prompt/instruction-only; the PR additionally ships regression-guard evals for both, plus a token-auth migration of the eval harness workflow (see Scope & safety).

1. gh CLI is unauthenticated by design, but the prompt doesn't say so — ~61/64 sessions

The CopilotReview task intentionally strips all GitHub tokens (copilot ... --secret-env-vars=GH_TOKEN,COPILOT_GITHUB_TOKEN,GITHUB_TOKEN), so gh pr view / gh issue view / gh api always fail inside the agent run. But pr-preflight.md still leads with those commands, so nearly every session burns 1-3 turns watching them fail and pivoting — and several then wrongly log an "environment blocker" or lower review confidence.

Fix: pr-preflight.md now opens with an explicit "Environment & Authentication" note stating that unauthenticated gh is expected, not a blocker (don't stop, don't reduce confidence), and provides local-git + unauthenticated public-REST (curl) recipes that work in CI. The original gh recipes are retained for local pr-review runs where a token is present.

2. Agent refuses to write the REQUIRED inline-findings.json — ~34/64 sessions

The expert-review step over-generalizes a host "do not write output files" guardrail and refuses to write inline-findings.json, dumping the JSON into chat instead. There is no fallback — post-inline-review.ps1 finds no file (Test-Path fails) and inline review comments are silently dropped in roughly half of sessions. This is the most damaging correctness issue found.

Fix: the STEP 5b prompt in Review-PR.ps1 and the maui-expert-reviewer agent now explicitly authorize and mandate writing the file, state that the general "don't write review output" guidance does not apply to this required artifact, and forbid substituting a chat-text dump (with an orchestrator-writes-it-itself fallback instruction).

Files

• .github/pr-review/pr-preflight.md — environment note + local-first / curl recipes ([Draft] Readme WIP #1)
• .github/scripts/Review-PR.ps1 — strengthened STEP 5b inline-findings mandate (Update README.md #2, prompt string only)
• .github/agents/maui-expert-reviewer.md — reinforce required file write, no chat-dump (Update README.md #2)
• .github/skills/pr-review/tests/eval.gh-auth.vally.yaml — new regression guard for [Draft] Readme WIP #1
• .github/skills/code-review/tests/eval.inline-findings.vally.yaml — new regression guard for Update README.md #2
• .github/workflows/skill-validation.yml — token-auth migration for the eval harness (see Scope & safety)

Scope & safety

• Fixes [Draft] Readme WIP #1 and Update README.md #2 are prompt/instruction text only. No pipeline control-flow or security-boundary changes — the [Draft] Readme WIP #1 note merely documents the existing token-stripping (reinforces it, never weakens it). Review-PR.ps1's change is confined to a here-string prompt; pwsh parse check passes and $PRNumber interpolation is preserved.
• skill-validation.yml is a token-provisioning migration (not prompt text). The prior COPILOT_GITHUB_TOKEN* secrets were rotated out with the removal of gh-aw-agents, so the eval harness now sources Copilot auth from the copilot-pat-pool environment (COPILOT_PAT_0..9, index/run-id modulo selection, masked before GITHUB_OUTPUT). This is a like-for-like auth-source swap — same pull_request_target exposure model as before, no new secret exposure and no security loosening. ⚠️ Merge prerequisite: the copilot-pat-pool environment must be populated with COPILOT_PAT_0..9 (and must NOT carry required-reviewer/wait-timer protection rules, which would hang the pull_request_target job) so eval goes green immediately after merge.

Evals (regression guards for these fixes)

Now that we run vally eval suites, each fix ships with a guard that reproduces the exact failure mode it fixes. Both pass vally lint --strict and run on this PR (each lives under its skill's tests/ dir, which flags that skill as changed).

• .github/skills/pr-review/tests/eval.gh-auth.vally.yaml ([Draft] Readme WIP #1) — the vally eval step is already tokenless, so it natively reproduces the CI condition where gh is unauthenticated. The agent must classify that as expected, not a blocker, pivot to the local-first git/anonymous-REST path, and not lower its review confidence. Structural floor: the agent must end with GH_AUTH_BLOCKER: no — a necessary, not sufficient signal — with an LLM judge scoring the reasoning.
• .github/skills/code-review/tests/eval.inline-findings.vally.yaml (Update README.md #2) — pins a worktree to a real regression commit so the agent has a genuine diff, then must write inline-findings.json (canonical path/line/body schema) to the path the pipeline reads from disk and prove it landed (read back, echo FILE_OK:[…). An agent that refuses ("prohibited") and chat-dumps the JSON has no file to echo and fails. An LLM judge scores finding quality, non-refusal, and requires transcript evidence of an actual write tool call (not just a fabricated FILE_OK: line).

Each suite keeps two graders — one structural floor + one LLM judge. Both floors are satisfiable by a partial regression (the prompt hands the agent the giveaway token), so scoring.threshold is set to 0.7 (above the house 0.6): with the unweighted mean(floor, judge_norm) aggregate, a floor-1.0-but-judge-failing run scores (1.0 + 0.25)/2 = 0.625, which 0.6 would pass but 0.7 fails. This makes the LLM judge load-bearing (must reach ≥3/5) rather than letting the spoofable floor decide. Live-validated: good path 1.00, partial-regression 0.625.

Follow-ups (identified, not in this PR)

Same analysis surfaced: PowerShell-vs-bash redirection footguns in try-fix recipes; test-result classification keyed off exit code instead of TRX failed= count; winners crowned with no regression evidence; gate=FAILED conflating real regressions with wrong-platform/ineffective tests; and the agent re-deriving the true PR diff base each run. These can be addressed in separate PRs.

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

Or

• Run remotely in PowerShell:

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

[kubaflo]

Note

🤖 This review was automatically generated by a multi-model AI review system (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro). Three models independently reviewed the code, then cross-pollinated their findings to produce this consolidated review.

Multi-Model Review — PR #36002 Round 1

Verdict: ✅ LGTM (2 of 3 reviewers: Opus, Gemini)
Confidence: High

Summary

This PR correctly addresses real, documented failure modes in the PR review pipeline. All three reviewers agree the production prompt changes are sound, verified, and low-risk. The only discussion points concern eval design refinements — test-infrastructure improvements that don't gate merge.

What This PR Fixes

Problem: The pr-review skill was failing when gh CLI was unauthenticated (by design — pipeline strips tokens) and when agents tried to write inline findings to disk (deemed a violation).

Solution:

• pr-preflight.md: Documents the local-first, auth-free workflow using gh api with public endpoints
• maui-expert-reviewer.md + Review-PR.ps1: Explicitly authorizes writing inline-findings.json to disk as a required deliverable
• New regression tests: Two Vally evals with structural floors to guard against future regressions

Verification (All 3 Reviewers Agreed)

✅ Token-stripping behavior is correct (uses only public anonymous API)
✅ No security concerns (reinforces existing token-stripping)
✅ Inline findings read from disk via Test-Path (L1787-1789 of Review-PR.ps1)
✅ Both new evals parse, follow house style, and use sound structural checks
✅ Fixer uses jq -r + grep -F -f pattern (safe-by-construction)

Eval Design Considerations (Non-Blocking)

All three reviewers noted that the new evals embed some of their expected behavior in the test stimulus:

💡 gh-auth eval — The stimulus says "This is by design — the pipeline strips GitHub tokens" which is what pr-preflight.md should teach. Two independent reviewers (Opus, GPT) converged on this: reverting the production guidance wouldn't fail this eval. Suggest removing the answer-bearing sentence so it becomes a true regression guard.

💡 inline-findings eval — The stimulus restates "You ARE authorized..." which duplicates the fix. However, this is a faithful reproduction of the fixed runtime prompt (STEP 5b), so it works as a premise-validator ("does the model comply GIVEN authorization?") with a sound FILE_OK:\s*\[ structural floor. Opus and GPT both note this trades off regression-guard strength for runtime-fidelity.

💡 Two-dot vs three-dot diff (Opus) — git diff <base>..HEAD can show spurious reverts if <base> advances. Three-dot <base>...HEAD is more robust, though the inline hint mitigates this.

Verdict Resolution

• Gemini: LGTM (clean approval, praised evals as well-constructed)
• Opus: LGTM with 3 non-blocking suggestions (eval coupling + diff recipe)
• GPT: Initially NEEDS_DISCUSSION (eval concerns), downgraded to LGTM after cross-pollination when peers verified structural floors, security posture, and real-world value

Final verdict: ✅ LGTM (majority: 2 of 3, with GPT converging after cross-pollination)

The production prompt fixes are correct, complete, and address documented pain points. The eval coupling observations are valid test-infrastructure refinements best addressed in a follow-up. No bugs, no security issues, no merge blockers.

[kubaflo]

💡 💡 Eval self-fulfillment (Opus + GPT convergence → upgraded to suggestion)

The stimulus states "This is by design — the pipeline strips GitHub tokens from this phase," which is exactly what pr-preflight.md is supposed to teach. Two independent reviewers (Opus, GPT) converged on this coupling: reverting the production guidance wouldn't fail this eval because the answer is in the test prompt.

Non-blocking — the production prompt fix is correct and verified. Suggest removing the answer-bearing sentence (keep the neutral "discover it yourself" cue plus the directive to follow pr-preflight.md) so a revert would actually fail.

[kubaflo]

💡 💡 Eval authorization embedding (noted by all 3 reviewers)

The stimulus restates "You ARE authorized … not a violation of any no-output-files guardrail," duplicating the fix. This is a faithful reproduction of the fixed runtime STEP 5b prompt, so it works as a "does the model comply GIVEN the authorization?" premise-validator with a sound FILE_OK:\s*\[ structural floor.

However, deleting the authorization from maui-expert-reviewer.md / Review-PR.ps1 wouldn't fail this eval. Non-blocking — the structural check adds real value. Consider describing this as a premise-validator in the header, or source the authorization from the skill files under test.

[kubaflo]

💡 💡 Two-dot vs three-dot diff (Opus observation)

git diff --name-status <base>..HEAD is correct only when <base> is the true branch point. If an agent passes the advanced tip of main, two-dot shows main-only commits as spurious reverts. Three-dot <base>...HEAD is the more robust PR-review idiom.

Minor — the inline hint ("use the PR diff base") mitigates it. Neither peer reviewer mentioned this; Opus's finding stands unrefuted after cross-pollination.

[kubaflo]

Note

🤖 AI-generated approval (multi-model review automation, approving on @kubaflo's behalf).

✅ Approving — LGTM (multi-model review). Fixes the Copilot PR-reviewer prompts (unauthenticated gh reality + mandatory inline-findings.json write). Tooling/agent-infrastructure change, at the reviewed head, non-draft, mergeable.

[Copilot]

Copilot's findings

• Files reviewed: 6/6 changed files
• Comments generated: 2

[kubaflo]

Note

🔍 AI-generated review (automated multi-model orchestrator, on behalf of @kubaflo) — Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro.

✅ Round 3 — APPROVED (new commit 9105c38f reviewed)

New commit 9105c38f ("Harden eval discrimination: raise scoring threshold to 0.7") raises the pass threshold on the two Vally eval configs (eval.inline-findings.vally.yaml, eval.gh-auth.vally.yaml) from 0.6 → 0.7, with matched comment updates.

Code review — a test-strictness improvement

The rationale is sound and self-consistent: the structural floor is spoofable / a giveaway (the prompt hands the agent the exact no / FILE_OK token). With the unweighted mean(floor, judge_norm) aggregate and a scale_1_5 judge ((raw-1)/4), a floor-1.0-but-judge-fails run scores (1.0 + 0.25)/2 = 0.625 — which 0.6 would PASS but 0.7 FAILS. Raising to 0.7 forces the judge to reach ≥3/5, making the write-verifying / local-first-plan judge load-bearing instead of the spoofable floor. Live-validated (good path scores 1.00). This strictly tightens discrimination against partial regressions — no correctness risk.

CI red is the same confirmed base-workflow artifact — NOT this commit

Both evaluate (*) and the newly-selected Harness hermeticity gate fail, but I read the hermeticity log: TOKEN_1/2/3 empty → ##[error]No COPILOT_GITHUB_TOKEN secrets are configured — i.e. main's old for i in 1 2 3 workflow running via pull_request_target, not this PR's version. Identical root cause to R2: the old COPILOT_GITHUB_TOKEN* secrets are already rotated out, so main's workflow fails on every PR until this PR switches it to copilot-pat-pool. The threshold-only commit touches just two .vally.yaml files and cannot influence token provisioning. (Harness hermeticity gate is skipping on release-readiness PRs and only selected here because this PR touches eval-bearing skills.)

Verdict: Approve. The diff is a clean eval-strictness hardening; the red checks are confirmed not-PR-caused (base-workflow secret rotation, which this very PR fixes). Ensure copilot-pat-pool is populated with COPILOT_PAT_0..9 at merge so eval goes green immediately.

[kubaflo]

Note

🔍 AI-generated review (automated multi-model orchestrator, on behalf of @kubaflo) — Opus 4.8 · GPT-5.5 · Gemini 3.1 Pro.

✅ Round 4 — APPROVED

Commit 1091f299 broadens this PR into three coherent, evidence-driven fixes (mined from 64 maui-copilot sessions, 3 agents converging on the same top-2 systemic issues):

1. gh-unauth is EXPECTED, not a blocker (pr-preflight.md). The new "Environment & Authentication — READ FIRST" block correctly documents that Review-PR.ps1 strips GH_TOKEN/COPILOT_GITHUB_TOKEN/GITHUB_TOKEN in the CopilotReview task, so gh auth commands fail by design — and supplies the unauthenticated public-REST curl + local-git equivalents (dotnet/maui is public). This stops agents recording a benign auth failure as an environment blocker and lowering confidence.

2. Findings file MUST be written, never pasted to chat (maui-expert-reviewer.md, Review-PR.ps1). Explicitly overrides any "don't write output files" instinct for this one required artifact and warns that JSON returned as chat text is silently discarded by post-inline-review.ps1. Directly addresses dropped inline comments.

3. Two regression evals guarding exactly those behaviors — both with a robust anti-gaming design:

• eval.inline-findings.vally.yaml: structural floor (FILE_OK:\s*\[) + LLM judge, and the rubric explicitly scores a fabricated FILE_OK: with no real write-tool call in the transcript as 1/5 — closing the spoof the structural floor alone can't catch.
• eval.gh-auth.vally.yaml: GH_AUTH_BLOCKER:\s*no\b floor + judge asserting confidence is not lowered because of missing auth.

Plus the copilot-pat-pool token migration reviewed in prior rounds.

CI — red is the expected self-fixing-workflow artifact ✅

All evaluate (*) legs (incl. agentic-labeler, which this PR doesn't touch) fail identically with ##[error]No COPILOT_GITHUB_TOKEN secrets are configured. Because skill-validation.yml is pull_request_target, CI runs main's old token step, which can't see the rotated-out secrets — and #36002 is the migration that fixes it. The uniform failure across an untouched skill confirms this is the token infra, not any prompt regression. It resolves once merged to main.

Verdict: Approve. Well-scoped, low-risk instruction fixes + genuinely spoof-resistant regression evals; the red CI is the known base-workflow token artifact this PR remedies.