Skip to content

Sanitize upstream titles in release-readiness tracker tables (embedded newlines + unescaped pipes) - #36031

Merged
PureWeen merged 6 commits into
mainfrom
pureween-harden-ciscan-title-newlines
Jun 22, 2026
Merged

Sanitize upstream titles in release-readiness tracker tables (embedded newlines + unescaped pipes)#36031
PureWeen merged 6 commits into
mainfrom
pureween-harden-ciscan-title-newlines

Conversation

@PureWeen

@PureWeen PureWeen commented Jun 19, 2026

Copy link
Copy Markdown
Member

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!

Problem

The [Release Readiness] tracker issues render markdown tables built from upstream-controlled content — issue/PR titles dropped into pipe-delimited rows (| #link | <title> | <…> |). Several kinds of malformed (or hostile) title corrupt those rows:

  1. Embedded newlines. Some ci-scan issues have a title containing a literal newline — observed live on issue [ci-scan] Recurring: MemoryLeakB42329 and Issue1939Test fail with Appium timeout in Controls ListView on macOS MacCatalyst (maui-pr-uitest [Content truncated due to length] #35957, whose real GitHub title spans two physical lines and ends with (maui-pr-uitest\n[Content truncated due to length]. An embedded \r\n splits the row across two physical lines, so the title tail + trailing cells land on a line that no longer contains the issue link. This was visible on the live trackers: net10-sr9 [Release Readiness] .NET 10 SR9 — shipped (release/10.0.1xx-sr9) #35867 (2 broken rows) and net11-preview6 [Release Readiness] .NET 11.0 preview6 — release/11.0.1xx-preview6 #35866 (3 broken rows).
  2. Unescaped pipes. A literal | in a title (common — e.g. Fix A | B) injects an extra column. The preview engine already escaped pipes everywhere via its shared Format-MarkdownCell, but on the SR side cells were escaped ad-hoc — several embedded titles with no newline collapse at all.
  3. Escape-the-escaper pipe breakout. A title may legally contain a literal \| (backslash immediately followed by a pipe). Escaping only the pipe turns that into \\|, which GitHub-flavored Markdown renders as a literal \ followed by an active column delimiter — so the row still breaks out. Both engines had this latent bug.
  4. Raw </> (HTML injection / display loss) on the SR side. SR dropped titles in raw, so a title like Crash <!-- could inject an HTML-comment opener, and a legitimate List<T> (or engine-authored placeholder text like Bump <PatchVersion> …) was silently swallowed by GitHub as an unknown HTML tag and never displayed.

The ultimate root cause of (1) is upstream (the CI Failure Scanner producing a multi-line title), but a readiness engine should defensively sanitize any external content it embeds into its own tables.

Fix

  • Get-ReleaseReadiness.ps1 — introduce a single null-safe Format-MarkdownTableCell helper and route every SR site that embeds upstream-controlled text through it. This covers the ci-scan rows, the Open PRs Targeting <srBranch>, regression classification, 🔴 Blocking summary, 🧹 Cleanup, 📥 Open Fix PRs Inbound, and Ship-readiness checks tables, plus the candidate-PR bulleted list. The helper:
    • collapses [\r\n]+ → space (hazard 1),
    • escapes each |\| and doubles only the backslash run immediately preceding that pipe (via a single (\\*)\| regex pass), so a pre-existing \| becomes \\\| (renders a literal \|, no breakout — hazards 2 & 3). The doubling is scoped to pipe-adjacent runs rather than every backslash, so a title's other backslash escapes (\[link\](url), \*not emphasis\*) are preserved verbatim and not de-escaped into active Markdown. No-pipe-adjacent-backslash titles are unchanged (a | ba \| b).
    • escapes </>&lt;/&gt; (hazard 4), matching the preview engine for SR↔Preview parity.
  • Get-PreviewReadiness.ps1 (Format-MarkdownCell) — the newline collapse plus the same pipe-adjacent backslash handling so the preview engine is immune to the \| breakout too.

On </> escaping (now consistent across both engines): escaping angle brackets to entities has zero visual cost&lt;T&gt; renders as <T> — so List<T> fidelity is preserved while raw HTML injection is neutralized (<!--&lt;!--). It also fixes a latent display bug: engine-authored NextAction text such as Bump <PatchVersion> in eng/Versions.props was previously rendered raw and swallowed by GitHub as an unknown HTML tag, so the Release Captain saw Bump in eng/Versions.props; it now displays correctly. SR is additionally hash-freeze-immune (it emits its own hash at the top of the body, extracted with head -n1) and its human-notes markers are matched full-line-anchored, so escaping <> is defense-in-depth layered on top of those backend invariants rather than the sole protection. (.Trim() only touches leading/trailing whitespace.)

Tests

Deterministic, offline assertions:

  • Format-MarkdownTableCell / Format-MarkdownCell unit tests — pipe escaping, LF/CRLF-run collapse, newline+pipe together, null/empty → empty string, whitespace trim; angle brackets escaped to &lt;/&gt; (both engines, parity); literal \| does NOT break out (→ A \\\| B); non-pipe backslash preserved (C:\dir unchanged) and author-escaped non-pipe Markdown not de-escaped (\[link\](url) unchanged) for both engines; <!-- opener neutralized to &lt;!--.
  • SR ci-scan row — an embedded-newline title renders as a single physical row with its tail + age intact.
  • SR tables (end-to-end Format-MarkdownReport) — a piped+newline title in the Open PRs Targeting, regression classification, 🔴 Blocking summary, 📥 Open Fix PRs Inbound, and Ship-readiness checks tables each stays on one physical row, pipe escaped, trailing column intact; the BLOCKED ship-check next-action with <PatchVersion> renders entity-escaped (so GitHub actually displays it).
  • Human-notes marker-forgery regressions (security) — a title embedding …\n<!-- …:human-notes:begin -->\n… in a table cell and in the candidate-PR list must leave exactly one anchored begin-marker in the rendered body (the legitimate one), proving a hostile title cannot forge a second notes region.
  • Preview engineFormat-MarkdownCell collapses LF/CRLF runs and preserves the existing pipe / angle-bracket escaping contract.

The discriminating assertions were verified red on the pre-fix scripts and green after (and the surgical-scoping assertions were verified red against the earlier global-doubling commit). Offline suite: 566 passed / 0 failed; full E2E: 632 / 0.

Scope / follow-ups (intentionally out of this PR)

  • Backtick (inline-code) is intentionally not escaped: doing so would degrade the very common legitimate case of code-quoted titles like `CollectionView`, and an unescaped backtick is a cosmetic-only, non-structural concern (it cannot create a new column, inject HTML, or forge a human-notes marker, all of which require |/<, which are escaped).
  • Null-safety of .title.Length under Set-StrictMode -Version Latest is pre-existing (titles are non-null by GitHub API contract) and intentionally deferred to a focused follow-up rather than mixed into this rendering PR.
  • The SR "Reverts" table's "Reverts commit" column shows ? for every row (the This reverts commit <sha> body-regex never resolves). Pre-existing and unrelated; noted for a future pass.
  • Filing an upstream issue against the CI Failure Scanner for the malformed (multi-line) titles is worth doing separately so the trackers receive clean input at the source.

…lines

Some CI Failure Scanner (`ci-scan`) issues have titles that contain a literal
newline (observed live: #35957, whose title ends `(maui-pr-uitest\n[Content
truncated due to length]`). Both readiness engines copy those titles into the
"Recent CI Failure Scanner signals" markdown table; the embedded LF splits the
table row across two physical lines and breaks the rendered table in the posted
`[Release Readiness]` tracker issues (seen in net10-sr9 #35867 and
net11-preview6 #35866).

Root cause is upstream (the scanner producing a multi-line title), but the
readiness engines should defensively sanitize external content they embed into
their tables. Both cell formatters escaped `|` (and the preview one `<`/`>`) but
neither stripped newlines.

Fix:
- Get-ReleaseReadiness.ps1 (`Format-CiScanIssueRows`): collapse `[\r\n]+` -> ' '
  before escaping the title cell.
- Get-PreviewReadiness.ps1 (`Format-MarkdownCell`): same collapse, applied in the
  shared cell formatter so every preview table cell is protected.

Tests (offline, deterministic): a ci-scan title with an embedded newline now
renders as a single table row in the SR engine, and `Format-MarkdownCell`
collapses LF/CRLF runs while preserving the existing pipe/angle-bracket escaping
contract. Verified these fail on the pre-fix code (4 reds) and pass after
(offline suite 537 passed / 0 failed).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

🚀 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 -- 36031

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 36031"

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Skill Validation Results

@PureWeen — new skill validation results are available based on this last commit: 4f7d879.
To request a fresh validation after new comments or commits, comment /evaluate-skills.

Overall Passed Static Passed LLM Skipped Skills 20 Agents 6

Skill Validation Results4f7d879 · Sanitize upstream titles in release-readiness tracker tables (embedded newlines + unescaped pipes) · 2026-06-20T20:43:44Z

✅ Static Checks Passed

Skills: 20 | Eval specs linted: 7

Full lint output
── .github/skills/agentic-labeler/tests/eval.vally.yaml
npm warn deprecated prebuild-install@7.1.3: No longer maintained. Please contact the author of the relevant native addon; alternatives are available.
✔ .github/skills/agentic-labeler/tests/eval.vally.yaml is valid
── .github/skills/code-review/tests/eval.capability.vally.yaml
✔ .github/skills/code-review/tests/eval.capability.vally.yaml is valid
── .github/skills/code-review/tests/eval.vally.yaml
✔ .github/skills/code-review/tests/eval.vally.yaml is valid
── .github/skills/code-review/tests/hermeticity.vally.yaml
✔ .github/skills/code-review/tests/hermeticity.vally.yaml is valid
── .github/skills/evaluate-pr-tests/tests/eval.vally.yaml
✔ .github/skills/evaluate-pr-tests/tests/eval.vally.yaml is valid
── .github/skills/try-fix/tests/eval.vally.yaml
✔ .github/skills/try-fix/tests/eval.vally.yaml is valid
── .github/skills/verify-tests-fail-without-fix/tests/eval.vally.yaml
✔ .github/skills/verify-tests-fail-without-fix/tests/eval.vally.yaml is valid

⏭️ LLM Evaluation: Skipped

No changed skills with eval specs found.

🔍 Full results and investigation steps

@github-actions github-actions Bot added the area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions label Jun 19, 2026
@MauiBot MauiBot added s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review) labels Jun 20, 2026
MauiBot

This comment was marked as outdated.

@MauiBot MauiBot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review Summary

@PureWeen — new AI review results are available based on this last commit: eb765c0. To request a fresh review after new comments or commits, comment /review rerun.

Gate Skipped Code Review In Review Confidence High Platform Windows

Review Sessions — click to expand
Gate — Test Before & After Fix

Gate Result: ⚠️ SKIPPED

No tests were detected in this PR.

Recommendation: Add tests to verify the fix using the write-tests-agent.


Pre-Flight — Context & Validation

Issue: #36031 - PR-local infrastructure bug report (no linked issue detected)
PR: #36031 - Harden release-readiness tracker tables against embedded newlines in upstream ci-scan titles
Platforms Affected: windows; infrastructure/release-readiness markdown generation
Files Changed: 2 implementation, 1 test

Key Findings

  • The PR changes .github/skills/release-readiness/scripts/Get-ReleaseReadiness.ps1, .github/skills/release-readiness/scripts/Get-PreviewReadiness.ps1, and .github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1.
  • Root cause: upstream ci-scan issue titles can contain literal CR/LF, which split generated markdown table rows across physical lines.
  • Gate was already skipped by the caller because no tests were detected by the gate phase; local candidate testing used the release-readiness offline test script.
  • Local baseline offline suite exits nonzero before alternatives because two notes-marker assertions fail on Windows line endings; candidate comparisons treat those as baseline unless a candidate explicitly changes that area.

Code Review Summary

Verdict: LGTM
Confidence: high
Errors: 0 | Warnings: 0 | Suggestions: 2

Key code review findings:

  • 💡 Add a symmetric SR CRLF assertion for Format-CiScanIssueRows if desired.
  • 💡 SR table-cell centralization is a reasonable future refactor, but not required for the observed ci-scan title bug.

Fix Candidates

# Source Approach Test Result Files Changed Notes
PR PR #36031 Collapse [\r\n]+ in Preview Format-MarkdownCell and SR ci-scan title before table rendering ⚠️ Gate skipped by caller; local relevant regression assertions pass, full suite has unrelated baseline failures .github/skills/release-readiness/scripts/Get-PreviewReadiness.ps1, .github/skills/release-readiness/scripts/Get-ReleaseReadiness.ps1, .github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1 Original PR

Code Review — Deep Analysis

Code Review — PR #36031

Independent Assessment

What this changes: Adds CR/LF collapse before markdown table output can consume upstream ci-scan issue titles, preventing embedded newlines from splitting tracker table rows. Preview applies this in the shared Format-MarkdownCell; SR applies it to the observed ci-scan title cell.
Inferred motivation: A malformed upstream ci-scan title containing a literal newline broke release-readiness tracker markdown tables.

Reconciliation with PR Narrative

Author claims: The PR hardens SR and Preview readiness tracker tables against embedded newlines and adds deterministic offline tests.
Agreement/disagreement: The code matches the narrative. The SR fix is intentionally targeted; Preview uses the broader shared formatter.

Prior Review Reconciliation

No prior ❌ Error findings found. GitHub CLI auth was unavailable; public PR metadata/comments and local branch diff were used.

Blast Radius Assessment

  • Runs for all instances: No — only release-readiness report-generation scripts.
  • Startup impact: No.
  • Static/shared state: No.

CI Status

  • Required-check result: undetermined locally because gh is unauthenticated.
  • Classification: public PR metadata shows skill/static validation success; local release-readiness offline suite has two baseline notes-marker failures unrelated to this PR before candidate changes.
  • Action taken: confidence capped for CI auth limitation in local review context.

Findings

💡 Suggestion — Add symmetric SR CRLF assertion

The SR ci-scan regression test uses LF; the implementation handles CRLF via [ ]+. A CRLF-specific SR assertion would improve symmetry with Preview tests.

💡 Suggestion — SR table-cell centralization remains future work

SR still has scattered table-cell escaping in other sections. The PR documents centralization as future work; this is not a blocker for the observed ci-scan title bug.

Failure-Mode Probing

  • Malformed ci-scan title with LF: remains one physical markdown row.
  • CRLF run in Preview Format-MarkdownCell: collapses to one space.
  • Null Preview cell: existing guard returns empty string.
  • Title with newline and pipe: newline collapse precedes pipe escaping.

Verdict: LGTM

Confidence: high for code correctness; local CI status collection limited by missing gh authentication.
Summary: The PR fix is minimal, correctly ordered, and covered by deterministic tests. No blocker was found in the PR fix itself.


Fix — Analysis & Comparison

Fix Candidates

# Source Approach Test Result Files Changed Notes
1 try-fix Broad SR Format-MarkdownCell helper used across SR table cells; Preview unchanged from PR ⚠️ 601 passed / 2 failed (same unrelated baseline notes-marker failures) 1 file More robust SR coverage, but larger blast radius and helper-name collision risk in dot-sourced tests
2 try-fix Row-boundary CR/LF collapse in ci-scan row appenders; Preview helper reverted and test moved to Add-CiScanTable ⚠️ 600 passed / 2 failed after fixing a PowerShell parsing error (same unrelated baseline failures) 3 files Narrowest diff, but less robust than PR because other Preview table cells lose newline protection
3 try-fix Broad SR helper plus LF-only release-captain notes block normalization; Preview unchanged from PR 603 passed / 0 failed 1 file Full suite green, but only because it also fixes an unrelated notes-marker Windows line-ending baseline bug
PR PR #36031 Targeted SR ci-scan title collapse plus shared Preview Format-MarkdownCell collapse ⚠️ Gate skipped; local relevant tests pass, full suite has unrelated baseline failures 3 files Best scoped fix for the PR's stated issue

Cross-Pollination

Model Round New Ideas? Details
maui-expert-reviewer 1 Yes Candidate 1: centralize SR table-cell sanitation with an SR helper
maui-expert-reviewer 2 Yes Candidate 2: sanitize at ci-scan row emission boundary to avoid broad behavior changes
maui-expert-reviewer 3 Yes Candidate 3: combine broad SR helper with unrelated notes-marker line-ending baseline fix

Exhausted: Yes
Selected Fix: PR's fix — it is the best scoped fix for the embedded ci-scan newline problem. Candidate 3 is the only all-green local run, but it wins by adding an unrelated baseline repair; Candidate 1 is broader than necessary; Candidate 2 is narrower but loses the PR's broader Preview formatter hardening.


Recommended PR Title & Description

Assessment: ✏️ Recommend updating — the current title is good, but the description overstates the Windows test outcome available in the PR-agent artifacts.

Recommended title

Release-readiness: Harden tracker tables against embedded ci-scan title newlines

Recommended description

### Problem

The `[Release Readiness]` tracker issues render a **"Recent CI Failure Scanner signals"** markdown table built from upstream `ci-scan` issues. Some of those issues have a title that contains a **literal newline** — observed live on issue #35957, whose real GitHub title spans two physical lines and ends with `(maui-pr-uitest\n[Content truncated due to length]`.

Both readiness engines copy the raw title straight into a table cell (`| #link | <title> | <age> |`). They escaped the pipe character (and the preview engine also escapes `<`/`>`) but **neither stripped newlines**. An embedded `\r\n` splits the table row across two physical lines, so the row's title tail + age cell land on a line that no longer contains the issue link — breaking the rendered markdown table.

This was visible on the live trackers after the most recent run: **net10-sr9 #35867** (2 broken rows) and **net11-preview6 #35866** (3 broken rows). net10-sr8 #35876 happened not to contain such a title and rendered fine.

The ultimate root cause is upstream (the CI Failure Scanner producing a multi-line title), but a readiness engine should defensively sanitize external content it embeds into its own tables — exactly as it already does for `|` and `<`/`>`.

### Fix

- **`Get-ReleaseReadiness.ps1` (`Format-CiScanIssueRows`)** — collapse `[\r\n]+` → single space before escaping the ci-scan title cell.
- **`Get-PreviewReadiness.ps1` (`Format-MarkdownCell`)** — the same collapse, applied in the shared cell formatter so **every** preview table cell is protected (not just ci-scan).

`.Trim()` only removes leading/trailing whitespace, so it never handled the internal newline. Normal GitHub issue titles cannot contain newlines, so this only affects the malformed ci-scan titles.

### Tests

Adds 7 deterministic, offline assertions:

- **SR engine:** a ci-scan issue whose title contains an embedded newline now renders as a **single** physical table row, with the title tail + age still on that row.
- **Preview engine:** `Format-MarkdownCell` collapses LF and CRLF-runs to a single space, leaves no CR/LF in the cell, and **preserves the existing pipe / angle-bracket escaping contract**.

The discriminating newline assertions were verified to fail on the pre-fix code and pass after the fix. On the available Windows PR-agent run, the relevant regression assertions pass; the full offline suite still has unrelated pre-existing notes-marker baseline failures.

### Scope / follow-ups (intentionally out of this PR)

- The SR engine escapes table cells inline at multiple sites; only the ci-scan title site manifests the newline bug today (it's the only cell fed unsanitized upstream titles), so only that site is hardened here. Centralizing SR cell escaping (to match the preview engine's `Format-MarkdownCell`, including `<`/`>` parity) is a larger refactor left as future work.
- The SR "Reverts" table's "Reverts commit" column shows `?` for every row (the `This reverts commit <sha>` body-regex never resolves). Pre-existing and unrelated to this change; noted for a future pass.
- Filing an upstream issue against the CI Failure Scanner for the malformed (multi-line) titles is worth doing separately so the trackers receive clean input at the source.

Report — Final Recommendation

Comparative Fix Report - PR #36031

Candidates compared

Rank Candidate Result Assessment
1 pr Relevant regression assertions pass; gate skipped; full local Windows suite has unrelated notes-marker baseline failures Best scoped fix. It hardens the known SR ci-scan title path and the shared Preview markdown cell formatter without pulling unrelated baseline repairs into the PR.
1 pr-plus-reviewer Same as pr Expert reviewer produced no actionable findings, so this candidate is identical to pr.
3 try-fix-3 Full local suite passed (603 passed / 0 failed) Technically green, but wins by also fixing an unrelated Windows notes-marker baseline issue. That broader unrelated change is not the right fix for this PR.
4 try-fix-1 Relevant newline assertions passed; full suite still had the same unrelated baseline failures More comprehensive SR cell centralization, but larger blast radius and introduces helper-name collision risk when scripts are dot-sourced together.
5 try-fix-2 Relevant SR/Preview ci-scan row assertions passed; full suite still had the same unrelated baseline failures Narrow and avoids helper collision risk, but weakens the PR by removing Preview's shared formatter hardening for non-ci-scan table cells.

Key comparison

try-fix-3 is the only candidate with a fully green local suite, but its extra green result comes from an unrelated notes-marker line-ending repair. The requested bug is table breakage caused by embedded newlines in upstream ci-scan titles; the PR fix addresses that directly and includes targeted tests without changing unrelated tracker behavior.

try-fix-1 is a plausible future refactor for SR table sanitation, but it changes more SR table emitters than needed for the observed bug. try-fix-2 is narrower, but less robust than the PR because it protects only Preview ci-scan row emission instead of preserving the PR's shared Format-MarkdownCell newline defense.

Winning candidate

Winner: pr

The raw PR fix is the best candidate because it is narrowly scoped to the real bug, preserves existing escaping contracts, adds deterministic regression coverage, and avoids unrelated baseline repairs or broader SR refactors. pr-plus-reviewer is equivalent because the expert reviewer had no actionable feedback.


Future Action — review latest findings

No alternative fix was selected for this run. Review the session findings and CI results before merging.

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 — Round 1

Verdict: LGTM

After independent review and cross-pollination, all three models converged on LGTM:

  • Gemini: LGTM
  • GPT: NEEDS_DISCUSSION → LGTM (retracted CI concern after cross-pollination)
  • Opus: LGTM

Summary

This PR adds robust newline handling to Format-MarkdownCell in the release-readiness tracker suite, preventing embedded newlines in upstream CI scan titles from corrupting Markdown table rows. The fix is correct, safe, and well-tested.

Key validation (Opus empirical verification):

  • ✅ Regex [\r\n]+ is safe (no ReDoS), correct PowerShell semantics
  • ✅ All 5 PR assertions pass post-fix (leading/trailing trim, CRLF collapse, $null safety)
  • ✅ Tests are discriminating — fail pre-fix, pass post-fix
  • ✅ No bugs in the changed code

Non-Blocking Suggestions

💡 SR cell escaping consolidation

Location: Test-ReleaseReadiness.ps1:2848

The SR ci-scan cell escapes | and newlines but not </>. Preview's Format-MarkdownCell escapes all four. Consider consolidating to a shared escaping helper (already noted in PR description as deferred follow-up: "centralize SR escaping").

Sibling sites (L3196/L3386/L3439) also embed PR/issue titles with only pipe escaping — same pre-existing pattern.

💡 Test clarity

Location: Test-ReleaseReadiness.ps1:2276

The first SR assertion is non-discriminating (would pass even without the fix). The second assertion is the real regression guard. Consider adding a clarifying comment.


What All Models Agree On

Fix is correct: [\r\n]+ → single space handles all edge cases
No injection risk: Regex is safe, orthogonal to existing pipe/angle escaping
Tests validate the fix: Regression tests discriminate pre/post-fix behavior
CI concern resolved: maui-pr: skipping is expected path-filtering for .github/skills/** changes


Confidence Assessment

High — All three models converged to LGTM with no blocking findings. Opus performed empirical PowerShell 7.5.4 validation. The fix is narrow, safe, and well-tested.


createdAt = $nowUtc.AddDays(-3).ToString('o') })
$nlRows = Format-CiScanIssueRows -Issues $nlIssue -RepoUrl 'https://github.com/dotnet/maui'
$nlRowLines = @($nlRows -split "`r?`n" | Where-Object { $_ -match '#35957' })
Assert-Eq -Label "Newline ci-scan title: issue row is a single physical line" -Expected 1 `

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 💡 Minor (test clarity, non-blocking). This first assertion (issue row is a single physical line → Expected 1) is non-discriminating: I verified it passes on the pre-fix code too. With the embedded newline, the pre-fix row splits as | [#35957](...) | ...tail / [Content truncated...] | age |, and only the first physical line contains #35957, so the count is still 1. The real guard against the regression is the second assertion (title tail + age stay on that row), which I confirmed fails ❌ on pre-fix and passes ✅ post-fix. Consider a brief comment noting the second assertion is the discriminating one, so a future maintainer doesn't weaken it assuming the first already covers row integrity. (Gemini and GPT both concurred with this in cross-review.)

createdAt = $nowUtc.AddDays(-3).ToString('o') })
$nlRows = Format-CiScanIssueRows -Issues $nlIssue -RepoUrl 'https://github.com/dotnet/maui'
$nlRowLines = @($nlRows -split "`r?`n" | Where-Object { $_ -match '#35957' })
Assert-Eq -Label "Newline ci-scan title: issue row is a single physical line" -Expected 1 `

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 💡 Minor (test clarity, non-blocking): I agree with Opus that the first new SR assertion (issue row is a single physical line) is not the discriminating regression check by itself: pre-fix output can still produce exactly one physical line containing #35957 while the title tail and age are pushed to the next line. The following assertion (title tail + age stay on that row) is the one that actually catches the bug. Consider a brief comment so future maintainers do not weaken the second assertion thinking the first one fully covers row integrity.

createdAt = $nowUtc.AddDays(-3).ToString('o') })
$nlRows = Format-CiScanIssueRows -Issues $nlIssue -RepoUrl 'https://github.com/dotnet/maui'
$nlRowLines = @($nlRows -split "`r?`n" | Where-Object { $_ -match '#35957' })
Assert-Eq -Label "Newline ci-scan title: issue row is a single physical line" -Expected 1 `

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 💡 Suggestion (Test clarity): As noted by Opus, the first assertion (issue row is a single physical line -> Expected 1) is non-discriminating and passes on the pre-fix code as well, because the embedded newline splits the row such that only the first physical line contains the #35957 issue number. The real guard against the bug is the second assertion (title tail + age stay on that row). Consider a brief comment noting that the second assertion is the discriminating one.

$title = ($iss.title -replace '\|', '\|').Trim()
# Collapse embedded newlines first: a malformed upstream ci-scan title can
# contain a literal CR/LF (observed: #35957), which would otherwise split this
# markdown table row across physical lines and break the rendered table.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 💡 Suggestion (non-blocking): Consider also escaping < and > to &lt; and &gt; here to match the escaping done in Get-PreviewReadiness.ps1's Format-MarkdownCell. This would prevent HTML injection if a ci-scan issue title contains angle brackets (e.g. List<T>). As noted in the PR description, centralizing cell escaping is a planned follow-up, which would be the perfect time to address this and other pre-existing unescaped sites (like L3196, L3386, and L3439) that Opus identified.

# Collapse embedded newlines first: a malformed upstream ci-scan title can
# contain a literal CR/LF (observed: #35957), which would otherwise split this
# markdown table row across physical lines and break the rendered table.
$title = ($iss.title -replace '[\r\n]+', ' ' -replace '\|', '\|').Trim()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 💡 Suggestion (non-blocking): The newline collapse here looks correct, but the SR tracker still handles markdown-cell escaping ad hoc. Gemini's angle-bracket point is valid as a consistency/content-fidelity concern (Format-MarkdownCell escapes </> in the Preview path), and Opus also found sibling SR rows that embed upstream titles without pipe escaping or newline collapse. This PR is fine as-is for the observed ci-scan newline bug, but the planned follow-up to centralize SR cell formatting should cover this ci-scan title plus the other upstream-title sites, and should consider matching Preview's pipe/newline/angle-bracket escaping contract.

# Collapse embedded newlines first: a malformed upstream ci-scan title can
# contain a literal CR/LF (observed: #35957), which would otherwise split this
# markdown table row across physical lines and break the rendered table.
$title = ($iss.title -replace '[\r\n]+', ' ' -replace '\|', '\|').Trim()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 💡 Suggestion (non-blocking, pre-existing scope). The newline collapse on this line is correct and well-targeted for the observed ci-scan bug. Two consistency gaps remain between the SR title-cell escaping here and the Preview engine's shared Format-MarkdownCell — both worth folding into the already-deferred “centralize SR cell escaping” follow-up:

  1. Angle brackets (credit: Gemini). This cell escapes | and now collapses \r\n, but does not escape </>. Format-MarkdownCell does, and the author's own comment there states GitHub “would otherwise swallow List<T> as an HTML tag.” So a ci-scan title containing angle brackets renders with reduced fidelity here. (Framing it as content-fidelity/consistency rather than injection — GitHub sanitizes rendered HTML, so this isn't a security hole, and the PR is correctly scoped to the live newline bug.)

  2. Sibling SR sites with no escaping at all. Three other sites embed upstream titles into pipe-delimited rows with neither pipe-escape nor newline collapse: L3196 $issCell = "$issLink — $titleShort" ($r.title), L3386 AppendLine("| $prLink | $title | … |") ($pr.title), L3439 AppendLine("| $issueLink | $title | … |") ($it.title). An unescaped | in a PR/issue title is legal and common and would already break those rows today.

Net: the PR is fine as-is for the observed bug. The “only cell fed unsanitized upstream titles” wording in the description slightly understates the existing surface, and a future Format-MarkdownCell-style SR helper should cover all of these sites and ideally match Preview's full pipe/newline/angle-bracket contract.

PureWeen and others added 2 commits June 20, 2026 11:34
Addresses non-blocking review feedback on PR #36031 (kubaflo multi-model
review, round 1): note in the test comment + assertion labels that the
first ci-scan-newline assertion is a coarse sanity check that also passes
pre-fix, while the second (title tail + age stay on the row) is the real
regression guard. Comment-only / label-only change; no behavior change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tizer

Extends the ci-scan newline fix to the other two SR tables that embed
upstream-controlled titles into pipe-delimited rows (review feedback on #36031,
kubaflo multi-model review): the 'Open PRs Targeting <srBranch>' table ($pr.title)
and the regression classification table ($it.title). Both previously had NO pipe
escaping or newline collapse, so a literal '|' (common in titles) or an embedded
newline would corrupt the row today.

Introduces Format-MarkdownTableCell — a single null-safe helper that collapses
CR/LF runs and escapes pipes — and routes the ci-scan cell plus both sibling sites
through it, replacing the ad-hoc inline escaping the reviewers flagged. The SR
helper deliberately omits '<'/'>' escaping: unlike the hash-less Preview engine
(whose Format-MarkdownCell must escape angle brackets to block an HTML-comment
hash-freeze), the SR engine emits its own semantic hash at the top of the body and
is structurally immune, so escaping '<>' there would only reduce title fidelity.
(The third site the review cited, the 'Open Fix PRs Inbound' table, already escapes
pipes at its render site — no change needed.)

Tests: direct unit coverage of Format-MarkdownTableCell (pipe/newline/null/empty/trim
+ the deliberate no-angle-bracket parity), plus end-to-end Format-MarkdownReport
assertions for both sibling tables. The end-to-end pipe/trailing-column assertions
are discriminating (verified red on pre-fix sibling sites, green after). Offline
suite 550/0.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PureWeen PureWeen changed the title Harden release-readiness tracker tables against embedded newlines in upstream ci-scan titles Sanitize upstream titles in release-readiness tracker tables (embedded newlines + unescaped pipes) Jun 20, 2026
Adversarial review of the ci-scan newline hardening found the centralization
was incomplete: the Blocking summary, Cleanup, Open-Fix-PRs, and Ship-readiness
checks tables, plus the candidate-PR bulleted list, still escaped pipes inline
without collapsing newlines — leaving the row-split vector (live #35957) open
on those tables and the candidate list.

Route every remaining upstream-controlled cell through the shared
Format-MarkdownTableCell sanitizer so the newline-collapse + pipe-escape contract
applies uniformly. Only the helper itself now performs the inline escape.

Security: an upstream title embedding `...\n<!-- release-readiness:human-notes:begin -->\n...`
could, pre-fix, isolate that marker on its own physical line and forge a second
notes region — corrupting the workflow's full-line-anchored notes-preservation
splice and wiping Release Captain Notes. Comprehensive newline collapse defeats
this WITHOUT escaping `<>`, preserving deliberate `List<T>` title fidelity (the
SR engine is hash-freeze immune via its top-of-body hash, and the anchored marker
regex can only fire when a marker lands alone on a line — which requires a newline).

Tests: +9 discriminating assertions (Blocking/Open-Fix/Ship-checks-table row
integrity + escaped pipes, and two marker-forgery regressions on a table cell and
the candidate list asserting exactly one anchored begin-marker survives). 8 of the
9 go red against the pre-fix script; offline suite 559/0. Null-safety of
`.title.Length` under StrictMode is pre-existing and deferred (titles are non-null
by GitHub API contract).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 reconcile differences. This is Round 2 following significant scope expansion (+34/-2 → +261/-16) after Round 1 LGTM.

Multi-Model Review Summary — PR #36031 Round 2

Title: Sanitize upstream titles in release-readiness tracker tables (embedded newlines + unescaped pipes)
HEAD: 0b7f7555 · Stats: +261/-16 lines, 3 files

Verdict: 🟡 NEEDS_DISCUSSION

Confidence: Medium
Recommendation: Code is ship-quality (unanimous), but required CI check is skipping due to by-design path filter for .github/**-only changes.


Consensus Assessment (3/3 Models)

✅ Code Quality: Unanimous LGTM on Merits

All three models independently concluded the code has no defects:

  • Sanitizer correctness verifiedFormat-MarkdownTableCell (CR/LF collapse + pipe escape) is correct and null-safe; replacement order prevents dangling \ on truncated strings
  • Completeness confirmed — Every upstream-title cell across both tracker engines routes through sanitization; cells that bypass it carry only engine-generated data
  • Security rationale validated — Deliberate <> omission in SR (preserving List<T> fidelity) is defended by (a) hash-freeze immunity via head -n1 extraction, (b) marker-forgery immunity via full-line-anchored regex + newline collapse
  • Tests are discriminating — Opus empirically verified: neutering Format-MarkdownTableCell caused exactly 21 sanitization assertions to fail; "coarse sanity" checks correctly stayed green
  • Round 1 suggestions addressed — Consolidation into shared helper complete; test clarity comments added

CI Status Disagreement

The one divergence: How to handle by-design CI skip for .github/**-only PRs.

Check Status Why
license/cla ✅ pass
maui-pr (required) ⚠️ skipping eng/pipelines/ci.yml path filter excludes .github/** (line 30) — all 3 changed files are under .github/skills/

Opus investigated:

  • Path-filter skip is deterministic and correct for tooling-only PRs
  • No workflow runs Test-ReleaseReadiness.ps1skill-validation reports "passed (static only)"
  • Functional validation is local-only (Opus ran 568 tests + confirmed discrimination)
  • Position: LGTM / Medium confidence — code is sound, confidence capped because no automated functional gate

Gemini & GPT:

  • CI skip means undetermined/no coverage
  • Policy: Never LGTM with high confidence when CI is skipped/pending
  • Position: NEEDS_DISCUSSION / Low-Medium confidence — wait for CI to run before LGTM

Synthesis: Per multi-model verdict resolution rules, use majority verdict when split. Majority (2/3) is NEEDS_DISCUSSION, so final verdict reflects CI policy conservatism. Note for merger: This is routine for .github/**-only PRs — maui-pr build has nothing to compile. The 29 new Pester tests pass locally (Opus-verified) but have no CI coverage.


Findings

💡 Suggestion (Non-Blocking) — Defense-in-Depth

File: .github/skills/release-readiness/Get-ReleaseReadiness.ps1
Lines: 3484–3488 (regression table), 3350 (CI-pipeline), 3380 (Reverts table)

$title cells correctly route through Format-MarkdownTableCell, but recommendedAction and some other cells are interpolated raw:

"| … | $($it.recommendedAction) |"  # L3488

Safe todayrecommendedAction values (L1914–2309) are engine-generated constants/issue numbers only.

Future-risk — If a future change embeds an upstream title in recommendedAction or other currently-raw cells, it would silently bypass sanitization.

Optional mitigation: Route all table cells through Format-MarkdownTableCell, or add comments marking cells as "intentionally engine-controlled."


Round 2 Scope Expansion Assessment

Author expanded from targeted ci-scan fix (+34/-2) to comprehensive, render-layer sanitization (+261/-16) addressing both Round 1 suggestions:

  1. SR escaping consolidation ✅ — All scattered -replace '\|' sites centralized into Format-MarkdownTableCell helper. Deliberate <> omission for SR (vs. Preview's escaping) is documented and defended.

  2. Test clarity ✅ — Added explicit "Discrimination note" and relabeled assertions (coarse sanity; non-discriminating) vs. (discriminating regression guard).

Result: 29 new tests (539 → 568), single choke point for all upstream-title cells, comprehensive newline-collapse + pipe-escape contract.


Bottom Line

Code verdict: ✅ Ready to ship (3/3 unanimous)
CI caveat: Required check is skipping by design (.github/** path filter)
Action: Maintainer can merge with standard .github/**-only-PR workflow, noting that Pester tests have no CI coverage but were verified locally by review automation.

Confidence: Medium — full marks on code quality, CI skip is expected but lacks automated functional gate.

# Stable sort: by issue number ascending
foreach ($it in ($items | Sort-Object issue)) {
$title = if ($it.title.Length -gt 50) { $it.title.Substring(0, 50) + '...' } else { $it.title }
$title = Format-MarkdownTableCell $title

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 💡 [Suggestion] Defense-in-depth: Engine-controlled cells bypass sanitizer

$title correctly routes through Format-MarkdownTableCell, but recommendedAction (L3488), CI-pipeline cells (L3350), and Reverts table cells (L3380) are interpolated raw.

Safe today — All contain only engine-generated constants/issue numbers (L1914–2309), never upstream titles.

Future risk — If someone later embeds an upstream title in these cells, it would silently bypass sanitization and re-open row-split/pipe-injection vectors.

Optional mitigation: Route all table cells through Format-MarkdownTableCell, or add comments marking them as "engine-controlled, intentionally unsanitized."

(Opus finding, validated by Opus cross-poll, not contradicted by peers)

PureWeen and others added 2 commits June 20, 2026 15:28
…y escaping

Two correctness/robustness fixes surfaced by adversarial review of the cell
sanitizers this PR centralizes:

1. Escape-the-escaper table breakout (both engines). A GitHub issue/PR title may
   legally contain a literal \| (backslash + pipe). Escaping only the pipe turned
   that into \\| which GFM renders as a literal backslash followed by an ACTIVE
   column delimiter, breaking the row out into extra columns. Double pre-existing
   backslashes BEFORE escaping pipes so \| -> \\\| (renders literal \|).
   No-backslash titles are unchanged (a | b -> a \| b), so existing assertions hold.

2. SR <>/HTML escaping (Get-ReleaseReadiness.ps1). Format-MarkdownTableCell now
   escapes < and > to &lt;/&gt;, matching Get-PreviewReadiness.ps1's Format-MarkdownCell.
   Zero visual cost (List&lt;T&gt; renders as List<T>) and defense-in-depth: an injected
   <!-- comment opener is rendered inert. This also fixes a latent display bug for
   engine-authored NextAction text such as 'Bump <PatchVersion> ...', which GitHub
   previously swallowed as an unknown HTML tag (Release Captain saw 'Bump  in ...').

Tests: +7 discriminating assertions (all proven red on pre-fix scripts); offline
564/0, full E2E 630/0. Doc-comments and the marker-forgery rationale updated.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…-pipe Markdown)

Adversarial review (GPT-5.5) flagged that the round-2 fix doubled EVERY backslash
in a cell before escaping pipes. That closed the pipe breakout but had an unintended
side effect: a title that legitimately escaped its own Markdown — e.g. \[link\](url)
or \*not emphasis\* — was de-escaped (\[ -> \\[), re-activating the link/emphasis on
GitHub's renderer. That is a fidelity regression vs main for those inputs.

Fix: scope the doubling to backslash runs IMMEDIATELY preceding a pipe via a single
regex pass, [regex]::Replace(v, '(\*)\|', { doubled-run + '\|' }), in both engines'
cell sanitizers. This is byte-identical to the previous commit for every pipe/breakout
case (a | b -> a \| b; A \| B -> A \\\| B; \\| -> \\\\\|), so the breakout stays closed
and all pipe assertions hold — while non-pipe backslash escapes are now preserved
verbatim. Net more faithful, same structural safety.

Tests: the two 'pre-existing backslash doubled' assertions become 'NON-pipe backslash
preserved', plus a new 'author-escaped non-pipe Markdown NOT de-escaped' assertion per
engine (all 4 proven red against the prior global-doubling commit, green now). Offline
566/0, full E2E 632/0. Backtick (inline code) is intentionally left unescaped — escaping
it would degrade the very common legitimate case of code-quoted titles like `CollectionView`,
and it is a cosmetic-only, non-structural concern (no column breakout / HTML / marker forgery).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@kubaflo kubaflo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 reconcile differences. This is Round 3 reviewing incremental changes after Round 2 NEEDS_DISCUSSION verdict.

Multi-Model Review Summary — PR #36031 Round 3

Title: Sanitize upstream titles in release-readiness tracker tables (embedded newlines + unescaped pipes)
HEAD: 4f7d8796 (Round 3) · Stats: +299/-18 lines, 3 files

Verdict: ✅ LGTM

Confidence: High (Unanimous 3/3)
Recommendation: Ready to merge. All Round 2 findings addressed, CI validated.


Unanimous Consensus (3/3 Models After Cross-Pollination)

Round 3 represents excellent responsive development:

All three models independently reviewed the Round 3 delta (0b7f75554f7d8796, +61/-25 net across 2 commits) and reached unanimous LGTM with high confidence after cross-pollination.

What Changed in Round 3

1. Round 2 Defense-in-Depth Finding: ✅ ADDRESSED

Round 2 flagged that recommendedAction and other engine-controlled cells bypassed the sanitizer. Author responded by:

  • Added <> escaping to Format-MarkdownTableCell — Now provides full SR↔Preview parity
  • Side benefit: Fixes latent display bug for Bump <PatchVersion> milestone titles
  • All upstream-controlled cells now route through sanitizer with complete escape coverage

2. New Proactive Security Fix: Escape-the-Escaper Protection

Author discovered and fixed a "pipe-breakout" vulnerability where a title like foo\|bar would:

  • Attempt escape: foo\|barfoo\\|bar (double backslash + escaped pipe)
  • But \\ renders as literal \, leaving unescaped | → table column injection

Solution: Backslash-doubling regex (\\*)\|$1$1\|

  • Doubles all backslashes immediately preceding pipes
  • Example: foo\|barfoo\\\|bar (3 backslashes: \\ renders as \, then \| renders as |)
  • Mathematically sound: N backslashes → 2N+1 (odd count) → pipe always inert
  • Perfect fidelity: Non-pipe-adjacent backslashes unchanged

Refined across 2 commits after GPT-5.5 flagged initial over-aggressive fidelity impact.

Empirical Verification (Opus)

  • Dot-sourced both sanitizers with 12 adversarial edge cases (nested escapes, Unicode, multi-space runs)
  • Byte-identical output with pre-expansion behavior on non-adversarial inputs
  • Full test suite: 566/0 pass under TZ=UTC (CI environment)
  • T8 local failure: Pre-existing timezone-fragile test (not in Round 3 delta, green in CI)

CI Validation — Critical Clarification

Round 2 incorrectly stated: "No workflow runs Test-ReleaseReadiness.ps1"

Round 3 correction (Opus discovery):

  • release-readiness.yml Validate (PR) job DOES run the full test suite
  • Triggers on pull_request for .github/skills/release-readiness/** (exact match for this PR)
  • Status: ✅ Passing (1m47s) on HEAD 4f7d8796
  • Tests: 566/0 pass in CI's UTC environment

Why maui-pr skips:

  • eng/pipelines/ci.yml path filter excludes .github/** (line 30)
  • By design: maui-pr builds MAUI product code (Core/Controls/Essentials)
  • These are PowerShell tooling scripts with nothing to compile
  • Correct skip, proper validation via Validate job

GPT's verdict evolution:

  • Independent: NEEDS_DISCUSSION (Low) — Cited gh pr checks --required showing maui-pr skip
  • Cross-poll: Upgraded to LGTM (High) — After learning about Validate job passing

Findings

Zero blocking or warning-level findings from all three reviewers.

All three models returned empty inline findings arrays after cross-pollination. No regressions, no new issues, no concerns.


Round-by-Round Evolution

Round Verdict Key Outcome
1 Unanimous LGTM (High) Targeted ci-scan fix, 2 non-blocking suggestions
2 Split → NEEDS_DISCUSSION (Majority) Scope expansion addressed suggestions, unanimous on code quality, split on CI interpretation
3 Unanimous LGTM (High) Defense-in-depth addressed, escape-the-escaper fix, CI validation confirmed

Total scope: Initial +34/-2 → Final +299/-18 (9× growth across 3 rounds)


Bottom Line

Code quality: Excellent responsive development. Author:

  • Addressed all Round 2 feedback within ~1 hour
  • Proactively discovered and fixed escape-the-escaper vulnerability
  • Refined solution across 2 commits after review feedback
  • Maintained perfect test coverage (29 new tests, all discriminating)

CI validation: Confirmed passing via release-readiness.yml Validate (PR) job

Security: Complete sanitization coverage (newline collapse + pipe escape + angle-bracket escape + backslash-doubling)

Verdict: Ship it. 🚀

Confidence: High — Unanimous agreement, empirically verified, CI validated, zero findings.

@PureWeen
PureWeen merged commit 5a7d8cb into main Jun 22, 2026
17 of 18 checks passed
@PureWeen
PureWeen deleted the pureween-harden-ciscan-title-newlines branch June 22, 2026 14:24
@github-actions github-actions Bot added this to the .NET 10 SR9 milestone Jun 22, 2026
PureWeen added a commit that referenced this pull request Jun 22, 2026
…ts (#36061)

<!-- Please let the below note in for people that find this PR -->
> [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from
this PR and let us know in a comment if this change resolves your issue.
Thank you!

### Summary

The SR release-readiness tracker overcounts the regressions header when
**exactly one** regression candidate exists. The live `.NET 10 SR9`
tracker ([#35867](#35867)) renders:

```
## Regression Candidates — 13 issues scanned
```

…even though only **one** issue
([#35615](#35615)) was actually
scanned. The summary table, tiers, and verdict all correctly show 1 —
only the header is wrong.

### Root cause

The header is built from `$regs.Count`:

```powershell
$regs = $Data['regressions']
... "## Regression Candidates — $($regs.Count) issues scanned"
```

Regression results are **hashtables**. `Get-RegressionCandidates`
returns its `$results` accumulator, and when exactly one candidate
matches, PowerShell **unwraps the single-element array on return**, so
`$Data['regressions']` arrives as a lone hashtable rather than a
1-element array. `.Count` on a hashtable returns its **key count** (13 —
`createdAt, confidence, milestone, state, closedAt, evidence,
candidateFixPrs, labels, stateReason, classification, recommendedAction,
issue, title`), not 1.

- **N = 0** → `@()` → `.Count` = 0 ✅ (already correct)
- **N = 1** → scalar hashtable → `.Count` = 13 ❌ (this bug)
- **N ≥ 2** → real array → `.Count` = element count ✅ (already correct)

### Fix

Force array context so `.Count` always reflects the candidate count:

```powershell
$regs = @($Data['regressions'])
```

One line. The sibling SR headers (`$blockingItems`, `$cleanupItems`,
`$openFixRows` are all `List[hashtable]`) and the preview engine
(`Get-PreviewReadiness.ps1`, which uses `List`/`@()`-wrapped
collections) are **not** affected — this is the only header fed the raw
`regressions` value.

### Tests

Added a **discriminating** regression test in
`Test-ReleaseReadiness.ps1` that reproduces the production unwrap by
assigning the regression result as a **scalar hashtable** (not `@(...)`,
which would mask the bug) and asserts the header reports `1 issues
scanned`, plus an N=2 guard for the already-correct path. A precondition
assertion locks in that the value is a scalar hashtable so a future edit
can't silently neuter the test.

- ✅ Verified the new test **fails pre-fix** (renders the key count) and
**passes post-fix**.
- ✅ Offline suite: **569 passed / 0 failed**.
- ℹ️ Full E2E: 627 passed / 3 failed — the 3 failures are
**pre-existing** (live-`gh` E2E tests: `sr-source-prs.txt`, candidate
JSON, `-InheritFromPriorSr` validation), reproduced identically on
pristine `main` (624/3) and unrelated to this change. They pass in CI's
`release-readiness.yml` Validate job, which has a proper `gh` token.

### Scope

Separate, focused follow-up off `main` — unrelated to the table-escaping
fix in #36031 (already merged). No behavior change beyond the header
count.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-infrastructure CI, Maestro / Coherency, upstream dependencies/versions s/agent-fix-pr-picked AI could not beat the PR fix - PR is the best among all candidates s/agent-reviewed PR was reviewed by AI agent workflow (full 4-phase review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants