Fix /review tests trigger and output

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

Description of Change

Refines the /review tests gh-aw workflow so it only activates for the exact /review tests command, not other /review subcommands such as /review rerun.

Also removes the noisy gh-aw status/reaction comments and changes the final test-failure report to post as a non-blocking PR review with a single collapsed <details> block.

Validation

• gh aw compile copilot-review-tests --validate
• PowerShell parse check for .github/scripts/Review-Tests.ps1
• git diff --check

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

Or

• Run remotely in PowerShell:

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

[github-actions]

🔍 Skill Validation Results

✅ Static Checks Passed

Skills checked: 19 | Agents checked: 4

Full validator output

Found 1 skill(s)
[review-test-failures] 📊 review-test-failures: 1,803 BPE tokens [chars/4: 1,964] (detailed ✓), 14 sections, 1 code blocks
✅ All checks passed (1 skill(s))
Found 4 agent(s)
Validated 4 agent(s)

✅ All checks passed (4 agent(s))

⏭️ LLM Evaluation: Skipped

No changed skills with eval tests found.

🔍 Full results and investigation steps

[PureWeen]

Adversarial Multi-Model PR Review

Methodology: 3 independent reviewers analyzed this PR in parallel with adversarial consensus. The full gh-aw workflow, the compiled lock.yml, the local PowerShell helper, and the skill instruction file were reviewed against the gh-aw safe-outputs reference.

Verdict: Mergeable with one ⚠️ worth addressing

The core change (tightening the trigger to exact /review tests and switching from add-comment to submit-pull-request-review with allowed-events: [COMMENT]) is coherent, security-preserving, and correctly compiled. Permission reductions are correct least-privilege. The new exact-match regex ^[[:space:]]*/review[[:space:]]+tests[[:space:]]*$ properly rejects /review rerun, /review tests --foo, and trailing-text variants. The double-write to $GITHUB_OUTPUT (should_run=false then conditional should_run=true) is fine — Actions takes the last value.

Findings

1. ⚠️ Config Impact / Performance — pre_activation.if: gate dropped (2/3 reviewers)
   Adding jobs.pre-activation: with custom outputs caused the compiler to drop the previous author-association gate in the lock.yml. Downstream security is intact (activation.if still requires activated == 'true'), but every PR/issue comment now spins up a pre_activation runner — and because cancel-in-progress: false shares the review-tests-<pr#> concurrency group, those comment-triggered runners queue serially behind any active /review tests run. See inline comment with the suggested fix.

2. 💡 Regression / UX — Reviews accumulate on repeated /review tests (2/3 reviewers)
   The old add-comment config used hide-older-comments: true and the local script edited a single comment in place. submit-pull-request-review has no equivalent for COMMENT-event reviews (supersede-older-reviews: only dismisses REQUEST_CHANGES). Each new /review tests invocation now appends a fresh entry to the PR's Reviews tab. By-design tradeoff for the new non-blocking model — worth a doc note so users know iterative re-runs will stack.

3. 💡 Error Handling — gh api ... --jq .html_url 2>&1 URL parsing (2/3 reviewers)
   2>&1 merges any gh warnings (auth refresh, deprecation notices) into $postOutput. Select-Object -Last 1 could then return a warning line instead of the URL. The review is already posted by that point, so this is cosmetic (wrong URL in the Write-Host line). See inline comment.

Disputed findings (discarded after validation)

• One reviewer flagged pull_request_number as missing from the safe-output validation schema and predicted workflow_dispatch runs would fail. Refuted: the upstream gh-aw safe-outputs-pull-requests reference confirms that with target: "*" the agent must supply pull_request_number in each submit_pull_request_review call — exactly what the .md and SKILL.md already instruct. The validation JSON in the lock.yml only enumerates fields needing extra constraints (body for sanitization, event for enum); pull_request_number is implicit. The skill instruction is correct as written.

Test coverage

This is a workflow/script change; the validation listed by the author (gh aw compile --validate, PowerShell parse check, git diff --check) is appropriate. No additional test scaffolding required.

[PureWeen]

Adversarial Multi-Model PR Review — Round 2

Methodology: 3 independent reviewers re-analyzed the PR after round-1 changes were applied, with adversarial consensus and multi-round self-correction (findings against round-2-introduced changes warrant closer scrutiny).

Round-1 follow-up

Round 1 finding	Status
⚠️ Author-association gate dropped from pre_activation	✅ Fully resolved. Restored via gh-aw built-in skip-author-associations: — cleaner than the manual if: I suggested. Lock.yml lines 1431-1432 compile the correct allowlist.
💡 Reviews accumulate in PR Reviews tab on re-runs	✅ Resolved differently than expected — the PR reversed course and switched back to add-comment with hide-older-comments: true. Cleaner end-state than submit-pull-request-review for this use case.
💡 gh api ... 2>&1 mixed stderr into URL	✅ Fully resolved. New Invoke-GhApiWithJsonPayload helper writes stderr to a separate temp file and filters stdout to ^https?://.

Round-2 findings

1. ⚠️ Regression / Config Impact — on.steps: exit 1 pattern fails the pre_activation job for every non-matching collaborator comment, producing red workflow runs across the repo. This is a regression from b6a264655d's should_run=true/false output pattern. The gh-aw guide explicitly warns against this. 3/3 reviewers (mixed severity).

2. ⚠️ Logic / UX — Local PS1 script and the gh-aw bot share the same <!-- Test Failure Review --> marker but use incompatible dedup strategies (PATCH vs. POST+hide), leading to invisible edits or orphaned comments. 2/3 reviewers.

3. 💡 Defense-in-depth — $verdict is interpolated unescaped into the comment HTML <summary> while $prTitle is correctly escaped — inconsistency, defense-in-depth gap. 3/3 reviewers.

Discarded / out-of-scope

• ❌ target: "*" allows the agent to post on any PR/issue — 1/3, pre-existing in origin/main. Not introduced by this PR; raise separately if you want to harden it.
• 💡 Strict regex rejects /review tests <trailing text> — 1/3, intentional (the step is literally called "Confirm exact /review tests command").
• 💡 No failure feedback path (reaction: none + status-comment: false) — 1/3, deliberate quiet-mode design choice.
• 💡 Tempfile cleanup not in try/finally — 1/3, minor cleanup gap.
• One reviewer initially claimed HEAD still uses submit-pull-request-review — discarded as a stale-context misread (grep confirmed add-comment in current HEAD).

Verdict

Mergeable. The round-2 regression (⚠️ #1) is a noise problem rather than a functional break — the gate still correctly prevents activation on non-matching comments. Worth addressing because every collaborator comment that isn't /review tests will produce a failed workflow run, drowning out genuine failures in the Actions UI.

[PureWeen]

⚠️ Regression / Config Impact — exit 1 on the non-match path fails the pre_activation job for every OWNER/MEMBER/COLLABORATOR comment that isn't exactly /review tests — including comments on plain issues (no ISSUE_PULL_REQUEST_URL → line 24 exit 1) and any other slash command (line 29 exit 1).

The compiled lock.yml (pre_activation, lines 1429–1495) has no continue-on-error: true, so the failed step fails the job, which marks the whole run red in the Actions tab. The activation job is then skipped via the needs: failure path rather than the intended if: exact_command_result == 'success' gate — which works for skipping, but produces noisy failed runs on essentially every collaborator comment repo-wide.

Flagged by: 3/3 reviewers (one called this ❌, two called it ⚠️ — included at ⚠️ since the gate functionally works; impact is UX/noise, not correctness).

Why this matters specifically here: the gh-aw guide explicitly warns against exit 1 in on.steps: blocks: "Use step outputs rather than exit 1 — failing the job normalizes failures and masks real errors." The previous revision (b6a264655d) used the correct pattern (echo "should_run=…"; exit 0), so this is a round-2 regression.

Suggested fix — revert to the explicit-output variant:

steps:
  - name: Confirm exact /review tests command
    id: exact_command
    env:
      EVENT_NAME: ${{ github.event_name }}
      COMMENT_BODY: ${{ github.event.comment.body }}
      ISSUE_PULL_REQUEST_URL: ${{ github.event.issue.pull_request.url }}
    run: |
      if [ "$EVENT_NAME" = "workflow_dispatch" ]; then
        echo "should_run=true" >> "$GITHUB_OUTPUT"; exit 0
      fi
      if [ "$EVENT_NAME" != "issue_comment" ] || [ -z "${ISSUE_PULL_REQUEST_URL:-}" ]; then
        echo "should_run=false" >> "$GITHUB_OUTPUT"; exit 0
      fi
      if [[ "$COMMENT_BODY" =~ ^[[:space:]]*/review[[:space:]]+tests[[:space:]]*$ ]]; then
        echo "should_run=true" >> "$GITHUB_OUTPUT"
      else
        echo "should_run=false" >> "$GITHUB_OUTPUT"
      fi

Then surface should_run as an output and gate on … == 'true'. All paths exit 0 → clean skipped runs.

[PureWeen]

⚠️ Logic / UX — The local script and the gh-aw workflow both publish comment bodies that start with <!-- Test Failure Review -->, but they use different deduplication strategies that don't compose:

• The workflow uses add-comment + hide-older-comments: true → POSTs a new comment and hides the older one via GraphQL minimizeComment.
• The local script (here) scans for body.Contains("<!-- Test Failure Review -->"), picks the last match, and PATCHes it in place (lines 335 → 339-343).

Concrete scenario:

1. Workflow runs → bot posts Comment A (marker, by github-actions[bot]).
2. Workflow runs again → posts Comment B, hides Comment A (still has marker).
3. A developer runs Review-Tests.ps1 -PostComment.
4. The script's REST scan returns both A and B (hidden comments are still returned by the REST API). Select-Object -Last 1 picks B and PATCHes it cross-author. Or worse: in a race, it edits Comment A which is already hidden → developer's update is invisible on the PR.
5. Next workflow run won't recognize the developer-edited comment as its own lineage → posts a fresh comment and hides the edited one → orphaning the local update.

Flagged by: 2/3 reviewers

Suggested fix — give the local path a distinct marker so it never collides with bot-authored comments:

$marker = "<!-- Test Failure Review (local) -->"

Apply the same marker change in New-TestFailureReviewBody (line 221). Alternatively, change the local path to always POST-new + minimize older so both paths use the same strategy.

[PureWeen]

💡 Defense-in-depth / Consistency — $verdict is interpolated unescaped into the HTML <summary> tag here and into the author-ping line above (line 283 in the heredoc). $prTitle is correctly run through Escape-Html on line 236, so this is an inconsistency.

$verdict comes from Get-ReportVerdict (line 154-160), which regex-extracts the text after **Overall verdict:** from agent-generated content. The agent is constrained to four taxonomy values, but the report content originates from untrusted PR diffs, test logs, and CI evidence — so injection isn't impossible, just unlikely in practice.

Flagged by: 3/3 reviewers

Suggested fix:

$verdict = Escape-Html (Get-ReportVerdict -Content $ReportContent)

or at the interpolation site. The badge URL path already URL-encodes the value via New-Badge; only the <summary> and author-ping interpolations are raw.

[kubaflo]

Applied the review suggestions from #35820 (review) and merged the latest main into the PR branch.

Summary:

• /review tests pre-activation now exits cleanly on non-matches and gates on an explicit should_run output.
• Local Review-Tests.ps1 -PostComment now uses a distinct local marker so it does not update workflow-authored comments.
• The rendered verdict is HTML-escaped before being inserted into the <summary>.
• Regenerated copilot-review-tests.lock.yml with gh aw compile.