Fix /review tests comment formatting

[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

Updates the /review tests failure-analysis comment shape so the marker, title, author/commit attribution, and badges are visible before the collapsed analysis details.

This removes the invalid/noisy Data badge, changes the top-level marker to <!-- Tests Failure -->, and keeps only the detailed analysis inside the collapsed <details> block with a click to expand summary.

The gh-aw prompt now uses Markdown commit links instead of raw <a> tags because the gh-aw safe-output handler sanitizes raw anchors into visible (a href=...) text before posting. The local Review-Tests.ps1 formatter is updated to the same visible shape.

Validation

• gh aw compile copilot-review-tests --validate
• PowerShell parser check for .github/scripts/Review-Tests.ps1
• Local formatter assertion for the requested marker, heading, badges, summary, and no Data badge
• 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 -- 35828

Or

• Run remotely in PowerShell:

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

[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,802 BPE tokens [chars/4: 1,984] (detailed ✓), 15 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]

Multi-Model Adversarial Review

Methodology: 3 independent reviewers analyzed the PR in parallel with adversarial consensus (unanimous findings included at highest severity, 2/3 at lower severity, 1/3 sent for cross-validation or discarded).

Inline findings posted on .github/scripts/Review-Tests.ps1

• ❌ Regression (line 221, 3/3) — Local PS1 marker now collides with the gh-aw bot marker; local script may PATCH (overwrite) the bot's analysis comment, or 403 trying.
• ⚠️ Logic (line 223, 2/3) — Marker rename makes the Contains($marker) early-return fire in the normal local path, bypassing the new PS1 wrapper (heading + $authorPing + badges) the PR adds.

Additional findings (not anchored to a diffed line)

• 💡 Documentation (3/3) — .github/scripts/Review-Tests.ps1:263-268 $authorPing still emits raw <a href> HTML for the commit link, while SKILL.md and copilot-review-tests.md were both updated to Markdown links and the new spec rule says "Use Markdown links, not raw <a> tags." No functional break locally (gh-aw's sanitizer isn't in the local path), but the PR description claims the local formatter was updated "to the same visible shape," and it wasn't. Suggested fix: "> @$prAuthor — test-failure review results are available based on commit [`$commitSha7`]($commitUrl)."
• 💡 Cleanup (2/3) — .github/scripts/Review-Tests.ps1:227 still requests --json title,author,headRefOid,url but title and url are no longer used. Benign; remove for clarity.
• 💡 Migration (2/3) — Renaming the marker means existing open PRs with comments that carry the old <!-- Test Failure Review --> or <!-- Test Failure Review (local) --> will not be matched by the new code; next /review tests will leave them visible and post a new one. One-time UX papercut, not a correctness issue. Optionally search/update both legacy and new markers for one release.

Discarded (1/3, low impact)

• The Write-Warning swap in the catch block silently shows "Failures 0" with no in-comment signal when context.json fails to parse (1/3, edge case + matches PR intent of removing the Data badge).
• Marker exact-casing dependence — pre-existing.

Verified clean

• .lock.yml change is exactly a body_hash regeneration; frontmatter_hash, pinned action SHAs, container digests, and safe-output handler config are unchanged. Consistent with the .md body change.
• safe-outputs (max: 1, hide-older-comments: true) semantics intact. gh-aw's hide-older-comments tracks its own internal GH_AW_WORKFLOW_ID-based marker — it is not affected by the human-readable marker rename, so the gh-aw side will continue to hide its prior comments correctly.
• No other repo files reference the old markers (grep across .github/ returned only the three files changed by this PR).
• No null-ref / data-loss regression from the $limitations / $prTitle variable removals — $failureCount defaults to 0, $platforms defaults to @(), both safe in the fallback path.

Top priority before merge

The marker collision on PS1 line 221/323 is the single ❌ in this review and was the explicit reason the prior code carried the (local) suffix. Recommend restoring a distinct local marker (which incidentally also resolves the early-return bypass).

[kubaflo]

@PureWeen thanks, addressed the high-signal items from #35828 (comment) in 71ec6dd.

Applied:

• restored a distinct local-only marker (<!-- Tests Failure (local) -->) so Review-Tests.ps1 will not patch gh-aw bot comments and will not early-return when the agent emits the gh-aw marker;
• kept lookup compatibility for the legacy local marker (<!-- Test Failure Review (local) -->) for migration;
• switched the local formatter commit link to Markdown syntax to match the gh-aw prompt contract;
• removed unused title/url fields from the local gh pr view call.

I intentionally did not match legacy non-local gh-aw markers from the local script, because that would reintroduce the risk of the local runner updating bot-owned comments. gh-aw hide-older-comments still handles workflow comments independently via its own workflow metadata.

[PureWeen]

Multi-Model Adversarial Review — Round 2

Re-reviewed commit 71ec6dd (the response to my previous review). Same methodology: 3 independent reviewers (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro) ran in parallel against the cumulative diff and were specifically asked to (a) validate the round-1 fixes work, (b) look for new issues introduced by the fixes, and (c) not re-flag resolved findings.

✅ Round-1 fixes verified (3/3)

Finding	Status
Marker collision (<!-- Tests Failure --> → <!-- Tests Failure (local) -->)	✅ Verified — local script can no longer PATCH the gh-aw bot's comment
Early-return bypass (now keyed on the local-specific marker)	✅ Verified — wrapper is correctly applied to legitimate LLM output
Raw <a>/<code> → Markdown link	✅ Replaced, but introduced a regression — see ❌ below
Unused title/url in gh pr view --json	✅ Verified — now requests author,headRefOid only, both consumed

Publish-TestFailureReviewComment's migration handling for the legacy <!-- Test Failure Review (local) --> marker is correct (2-element array, substring match, .Count -gt 0).

❌ NEW: Backtick escape regression (3/3) — Review-Tests.ps1 L264 & L267

Posted inline at L264 with a suggestion block. TL;DR: in a PowerShell double-quoted string, ` is the escape character, so `$commitSha7 emits literal $commitSha7 and the surrounding backticks disappear. Every local /review tests run produces a visibly broken attribution line. Fix is $commitSha7 on both branches.

⚠️ Likely-new: Duplicate chrome / nested <details> (2/3, freshly exposed)

Now that the early-return correctly matches only the local marker, the wrapper unconditionally runs on the LLM's full report — and per SKILL.md lines 100–138 the LLM already emits the full structure (gh-aw marker + ## Tests Failure Analysis heading + author ping + badges + <details> block). So the rendered local comment ends up with:

• 2 markers (<!-- Tests Failure (local) --> wrapping <!-- Tests Failure -->) — harmless, gh-aw tracks by internal workflow-ID, not by visible HTML
• 2 ## Tests Failure Analysis headings
• 2 author-ping blockquotes
• 2 badge rows
• Nested <details> (outer "click to expand" / inner LLM block)

This was masked in round 1 by the early-return that fired on the gh-aw marker; the round-1 fix is correct but exposes that the local wrapper and the SKILL.md output template overlap.

Suggested directions (either works):

1. Have the local prompt instruct the LLM to emit only the inner analysis (skip the marker, heading, ping, badges, outer <details>), and let New-TestFailureReviewBody own the chrome. Then $ReportContent is just the body.
2. Keep SKILL.md as the single source of truth for the comment shape and reduce New-TestFailureReviewBody to a minimal swap: $body -replace '<!-- Tests Failure -->', '<!-- Tests Failure (local) -->'.

Option (2) matches the design intent that local and bot output should look identical, and is the smaller patch.

💡 Housekeeping (1/3)

The legacy marker array in Publish-TestFailureReviewComment (<!-- Test Failure Review (local) -->) can be dropped after the migration window — once all open PRs that received a local review under the pre-PR-35828 format are merged/closed/refreshed. ~2–4 weeks is the typical window. A # TODO: remove after <date> comment would make this self-cleaning.

Investigated and dismissed (all 3 reviewers agreed)

• Double-marker side effect — gh-aw uses GH_AW_WORKFLOW_ID-based metadata for hide-older-comments, not the visible HTML marker. The bot will not hide the local comment just because its $ReportContent embeds the bot marker text.
• Where-Object nested array semantics — Verified for 0/1/2 element bodies. .Contains is substring, multi-line safe.
• Legacy-marker false match — A maintainer would have to literally type <!-- Test Failure Review (local) --> in PR conversation for it to misfire. Acceptable for the migration window.
• Idempotency — Re-passing an already-formatted local body hits the early-return and is returned unmodified. Correct.
• Legacy-marker query scope — gh api repos/.../issues/N/comments correctly returns only issue-level comments (where the local script posts), not inline review comments. No false positives from review threads.

---

Recommendation: Fix the backtick escape (~30s edit) before merging; the duplicate-chrome issue can ship as a follow-up since it's an aesthetic regression that doesn't break functionality, but ideally lands together. Once those are green, no further blockers from the multi-model panel.

[kubaflo]

@PureWeen I applied the review feedback in b4da1c8 and resolved the addressed review threads. Ready for re-review.

[PureWeen]

Multi-Model Adversarial Review — Round 3

Re-reviewed commit b4da1c8c33 ("Fix review tests commit link escaping"). Same methodology: 3 independent reviewers (Claude Opus 4.8, GPT-5.5, Gemini 3.1 Pro) ran in parallel. The round-3 commit applies exactly the fix suggested in round 2 (double backticks: $commitSha7 ).

✅ Round-2 backtick fix — VERIFIED (3/3 reviewers + empirical pwsh test)

Round-3 change at Review-Tests.ps1 L264 & L267 renders correctly:

> @kubaflo — test-failure review results are available based on commit [`abc1234`](https://github.com/dotnet/maui/commit/...).

This is the intended Markdown — [ ` abc1234 ` ](url). Round-2 regression is genuinely resolved, and no prior-round fix has regressed.

⚠️ New finding (3/3 after 1/3-dispute, pre-existing) — incomplete escaping fix

One reviewer found, and the other two confirmed after a dispute round + empirical pwsh verification, that the same backtick-escape bug class the round-3 commit just fixed exists at three other sites in the same file:

.github/scripts/Review-Tests.ps1 — the LLM prompt here-string at L403–423:

- Read and follow `.github/skills/review-test-failures/SKILL.md`.   # L407
- Write the final report to `$ReportPath`.                            # L410
- JSON: `$ContextJsonPath`                                            # L414
- Markdown: `$ContextMarkdownPath`                                    # L415

Plus L278 in the wrapper here-string:

> To request a fresh review after new comments, commits, or CI runs, comment `/review tests`.

In a PowerShell double-quoted string or here-string, ` is the escape character. The backticks vanish and any `$Var form suppresses variable expansion. Empirical pwsh test confirms — the LLM (Copilot CLI) actually receives:

- Read and follow .github/skills/review-test-failures/SKILL.md.
- Write the final report to $ReportPath.
- JSON: $ContextJsonPath
- Markdown: $ContextMarkdownPath
...
> ...comment /review tests.

Impact (verified by inspecting the rest of the script):

• L414/L415 — ⚠️ degraded analysis, no self-heal. The LLM is told context files exist but isn't given paths (no OutputDirectory is passed elsewhere in the prompt). It must guess or fall back to filesystem search — the analysis runs on potentially evidence-light input.
• L410 — recoverable. The runner has a fallback at ~L462–468 that writes the final assistant message to the actual $ReportPath if the file doesn't exist. The report still gets produced.
• L278 / L407 — 💡 cosmetic only. The /review tests mention and the SKILL.md path render as plain text instead of Markdown code spans.

Scope: git blame confirms these lines all originated in dfa37431ac ("Add /review tests failure review workflow" — prior merged PR #35701), not in this PR. Strict diff-scope says "separate issue." But this PR's round-3 commit is literally titled "Fix review tests commit link escaping" and applies the exact `` (double-backtick) pattern that fixes this class of bug. Leaving sibling instances unfixed in the same file makes the escaping fix incomplete — this is the textbook bundling case.

Suggested fix — same one-line pattern as the round-3 commit:

-- Read and follow `.github/skills/review-test-failures/SKILL.md`.
+- Read and follow ``.github/skills/review-test-failures/SKILL.md``.
...
-- Write the final report to `$ReportPath`.
+- Write the final report to ``$ReportPath``.
-- JSON: `$ContextJsonPath`
+- JSON: ``$ContextJsonPath``
-- Markdown: `$ContextMarkdownPath`
+- Markdown: ``$ContextMarkdownPath``
...
-> To request a fresh review after new comments, commits, or CI runs, comment `/review tests`.
+> To request a fresh review after new comments, commits, or CI runs, comment ``/review tests``.

(All 5 lines use the same $Var / literal pattern the round-3 commit used at L264/L267. Verified renders correctly via pwsh.)

Verdict — non-blocking. This is an incomplete-fix observation, not a regression introduced by this PR. The PR can ship as-is; opening a follow-up is also fine. But since 4 of the 5 lines have a real impact (degraded local-run analysis quality) and the fix is a 1-line sed-style change in the same file you're already editing, folding it in feels right.

Round-2 duplicate-chrome — acknowledged unchanged

The SKILL.md output template instructs the LLM to emit the full comment shape (marker + heading + ping + badges + <details>), and the local wrapper New-TestFailureReviewBody still wraps that whole thing again. Per your message above you marked the thread resolved without a code change. All 3 reviewers noted this remains unaddressed; per round-2 recommendation that this can ship as a follow-up, not re-flagged as a blocker here.

Investigated and clean

• Fallback behavior when $commitSha7 == "unknown" or $commitUrl == "#" → renders as [`unknown`](#); cosmetically inert, no breakage. (Same as pre-PR <a href="#"> behavior.)
• No other instances of the [`$var`](url) Markdown-link pattern in changed files.
• All round-1 fixes (marker rename, early-return key, <a>→Markdown, JSON-field cleanup) verified intact in round-3 HEAD.

---

Round-3 recommendation: the round-3 commit itself is correct and complete for what it set out to fix. The 5-line follow-up above is the only outstanding item from the multi-model panel; it's a natural extension of the round-3 commit. No blockers.

[kubaflo]

/review tests

[kubaflo]

/review tests

[kubaflo]

@PureWeen I fixed the /review tests no-failure path in 05f1d9b so future runs post a visible result comment instead of silently no-oping. Ready for re-review.

[kubaflo]

@PureWeen I removed emoji icons from the /review tests output contract and local formatter in 24ecc5f, regenerated the gh-aw lock, and verified the touched source/lock files contain no emoji-like characters. Ready for re-review.

[PureWeen]

Round-4 review (post-24ecc5fa + 05f1d9b8)

3 independent reviewers, adversarial consensus.

---

✅ Round-3 follow-up — backtick escaping fully resolved

Commit 05f1d9b8 folded in the suggestion from round 3 and extended the double-backtick escaping across all five remaining sites in the prompt block and wrapper here-string ($ReportPath, $ContextJsonPath, $ContextMarkdownPath, .github/skills/..., and /review tests). A repo-wide scan for single-backtick $Var patterns returns zero hits. 3/3 reviewers + empirical pwsh test.

✅ Emoji removal — clean

• No emoji characters remain in any of the 4 changed files (Unicode-aware scan; only non-ASCII chars are pre-existing em-dashes in prose).
• Get-VerdictIcon is fully removed with no orphan callers.
• [icon] placeholder is gone from both SKILL.md and copilot-review-tests.md summary templates.
• The new "Do not use emojis anywhere" rule is mirrored in both spec files.

3/3 reviewers.

✅ noop / dry-run / hide-older-comments semantics — internally consistent

• All four cases covered without contradiction: not-dry-run+failures → add_comment; not-dry-run+no-failures → add_comment "No failures found"; dry-run+failures → log-only; dry-run+no-failures → log + noop.
• safe-outputs.noop.report-as-issue: false keeps dry-run noop side-effect-free.
• add-comment: max: 1, hide-older-comments: true means the new no-failures comments correctly age out prior failure comments (strictly better than the old noop path — no concern about no-failure comments piling up).

3/3 reviewers.

✅ lock.yml change — consistent with source

The .md body is pulled in via {{#runtime-import …copilot-review-tests.md}} rather than embedded inline, so a body-only edit correctly produces only a body_hash metadata change — exactly what the diff shows. Would be regenerated identically by gh aw compile. 3/3.

---

⚠️ NEW — Get-VerdictColor missing case for "No failures found" (local wrapper only)

File: .github/scripts/Review-Tests.ps1 — Get-VerdictColor (lines 163–172)
Flagged by: 3/3 reviewers (severity 2/3 Warning, 1/3 Blocking → lower wins)

Bug. Commit 05f1d9b8 introduced the new verdict "No failures found" and explicitly mandates green badge color 1a7f37 (per SKILL.md L152 and workflow.md L202). But the local wrapper's Get-VerdictColor switch has no case for it — it falls through to default { return 'bf8700' } (orange/warning).

Empirically verified in pwsh:

Likely PR-caused               -> d1242f
Likely unrelated               -> 1a7f37
Insufficient data              -> 6e7781
Needs human investigation      -> bf8700
No failures found              -> bf8700   ← should be 1a7f37

Scope: local manual wrapper (pwsh Review-Tests.ps1 -PostComment …) only. The gh-aw CI path is unaffected because the LLM builds the badge URL itself with the colors from workflow.md L202. Visual-only on the locally-posted comment — no data integrity impact.

Fix (2 lines, code at L163–172 isn't in a diff hunk so cannot use an inline suggestion):

function Get-VerdictColor {
    param([string]$Verdict)

    switch -Regex ($Verdict) {
        'Likely PR-caused'  { return 'd1242f' }
        'Likely unrelated'  { return '1a7f37' }
        'No failures found' { return '1a7f37' }
        'Insufficient data' { return '6e7781' }
        default             { return 'bf8700' }
    }
}

💡 NEW — SKILL.md canonical color list doesn't include the new green case

Inline suggestion posted at SKILL.md line 147. 2/3 reviewers, Info.

---

🔁 Unchanged

• Round-2 duplicate-chrome / nested <details> (the SKILL.md template emits the full comment shape AND Review-Tests.ps1 wraps it again) — author elected to defer this. The new "No failures found" path uses the same comment shape, so it doesn't make the duplication worse. Not re-flagged.

---

Verdict — no blockers

Round-3 backtick fix is fully delivered and extended. Emoji removal is complete and clean. The new "No failures found" pathway is internally consistent across all 3 files. One small follow-up: add a 'No failures found' case to Get-VerdictColor so the local wrapper renders the correct green badge color the spec mandates (Finding above). Can ship without this — it only affects the local manual-testing path — but a 1-line addition closes the loop.

This review evaluates code only; CI status is intentionally out of scope. Posted comments only — never APPROVE / REQUEST_CHANGES (human decision).

[kubaflo]

@PureWeen I applied the round-4 documentation suggestion in f8c37b4 and resolved the addressed thread. Ready for re-review.

[PureWeen]

Round-4 follow-up — f8c37b4b applied

Verified f8c37b4b09 is the verbatim application of the round-4 SKILL.md L147 suggestion — 1-line documentation diff, no other code touched. Thread auto-resolved. ✅

Status of all findings to date

Item	Status
Round-1 marker collision, early-return bypass, raw <a> tags, unused JSON fields	✅ Resolved (71ec6dde)
Round-2 backtick-escape regression (commitSha7)	✅ Resolved (b4da1c8c)
Round-3 backtick escaping for $ReportPath / $ContextJsonPath / $ContextMarkdownPath / static strings	✅ Resolved (05f1d9b8)
Round-4 emoji removal completeness	✅ Clean (24ecc5fa)
Round-4 SKILL.md L147 color-list gap	✅ Resolved (f8c37b4b)
Round-2 duplicate-chrome / nested <details>	🔁 Deferred by author
Round-4 ⚠️ Get-VerdictColor missing 'No failures found' case in Review-Tests.ps1 (L163–172)	⚠️ Open — local wrapper only, cosmetic. CI/gh-aw path unaffected (LLM picks color from workflow.md L202). Can ship and follow up, or fold in as a 2-line tweak.

Verdict — no blockers from code review

The only open finding is a local-wrapper-only badge-color polish that does not affect the CI / production posting path. PR is ready from a review standpoint pending a CODEOWNER approving review.

This skill never APPROVEs / REQUEST_CHANGES — comments only.

[kubaflo]

@PureWeen I folded in the local-wrapper polish in $(git rev-parse --short HEAD): No failures found now maps to the same green badge color locally too. Ready for re-review.

[PureWeen]

Round-5 confirmation — 129f6533 applied & verified

Verified 129f6533bc is the 2-line Get-VerdictColor fix from the round-4 ⚠️ finding. Empirical pwsh test confirms:

Likely PR-caused               -> d1242f
Likely unrelated               -> 1a7f37
Insufficient data              -> 6e7781
Needs human investigation      -> bf8700
No failures found              -> 1a7f37   ✅

Local wrapper now matches the spec colors at SKILL.md L147 / L152 and workflow.md L202.

Final status — all consensus findings closed

#	Round	Finding	Status
1	R1	Local PS1 marker collision with gh-aw bot marker	✅ 71ec6dde
2	R1	Early-return bypass in normal local path	✅ 71ec6dde
3	R1	Raw <a> tag vs Markdown links	✅ 71ec6dde
4	R1	Unused JSON fields	✅ 71ec6dde
5	R2	Backtick escape regression on commitSha7 link	✅ b4da1c8c
6	R3	Backtick escaping for $ReportPath / $ContextJsonPath / $ContextMarkdownPath / static strings	✅ 05f1d9b8
7	R4	Emoji removal completeness	✅ 24ecc5fa
8	R4	SKILL.md color-list missing No failures found	✅ f8c37b4b
9	R4	Get-VerdictColor missing 'No failures found' case (local wrapper)	✅ 129f6533
—	R2	Duplicate-chrome / nested <details>	🔁 Deferred by author (acceptable)

Verdict — ✅ ready to merge from code review

No open code findings. Methodology: 3 independent reviewers per round (Opus + GPT-5.5 + Gemini) with adversarial consensus; 1/3 disputed findings went through follow-up rounds.

Merge button gating is REVIEW_REQUIRED — needs a CODEOWNER approval (this skill posts comments only — never APPROVE / REQUEST_CHANGES).