Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -851,7 +851,10 @@ function Format-MarkdownCell {
# `List<T>` that GitHub markdown would otherwise swallow as an HTML tag. The
# engine's own markers are emitted via AppendLine, not through this formatter,
# so escaping cells never disturbs them.
return (($Value -replace "\|", "\|") -replace "<", "&lt;" -replace ">", "&gt;").Trim()
# Collapse embedded newlines first: a malformed upstream title can contain a
# literal CR/LF (observed: ci-scan issue #35957), which would otherwise split
# the markdown table row across physical lines and break the rendered table.
return ((($Value -replace "[\r\n]+", " ") -replace "\|", "\|") -replace "<", "&lt;" -replace ">", "&gt;").Trim()
}

function Format-GitHubHandle {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2842,7 +2842,10 @@ function Format-CiScanIssueRows {
}
}
$issLink = "[#$($iss.number)]($RepoUrl/issues/$($iss.number))"
$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.

$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.

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.

[void]$sb.AppendLine("| $marker$issLink | $title | $ageDisplay |")
}
if ($Issues.Count -gt $MaxRows) {
Expand Down
26 changes: 26 additions & 0 deletions .github/skills/release-readiness/tests/Test-ReleaseReadiness.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -2263,6 +2263,21 @@ Assert-Eq -Label "Age column shows 'Nd ago' for older issues" -Expected $true `
Assert-Eq -Label "Format-CiScanIssueRows returns null for empty input" -Expected $true `
-Actual ($null -eq (Format-CiScanIssueRows -Issues @() -RepoUrl 'https://github.com/dotnet/maui'))

# Regression: a malformed upstream ci-scan title containing a literal newline
# (observed live: #35957) must NOT split the markdown table row across physical
# lines. The title cell is collapsed to a single line so the rendered table stays
# intact. On the pre-fix code the embedded LF pushed the title tail + age cell onto
# a second line that no longer contained the issue link.
$nlIssue = @([PSCustomObject]@{ number = 35957; url = 'https://github.com/dotnet/maui/issues/35957';
title = "Recurring long title (maui-pr-uitest`n[Content truncated due to length]";
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.)

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.

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.

-Actual $nlRowLines.Count
Assert-Eq -Label "Newline ci-scan title: title tail + age stay on that row" -Expected $true `
-Actual ($nlRowLines.Count -eq 1 -and $nlRowLines[0] -match 'truncated due to length.*ago \|')

# Truncation behavior: > MaxRows
$manyIssues = 1..20 | ForEach-Object {
[PSCustomObject]@{ number = 40000 + $_; url = "https://github.com/dotnet/maui/issues/$(40000+$_)";
Expand Down Expand Up @@ -3178,6 +3193,17 @@ try { $null = Get-CategorizedPullRequests -TargetPRs $null -InflightPRs @($null,
catch { $explicitNullThrew = $true }
Assert-Eq -Label "explicit null target + @(null, maestro) inflight → no throw" -Expected $false -Actual $explicitNullThrew

Write-Host "`n[Unit] Format-MarkdownCell collapses embedded newlines (table-row safety)" -ForegroundColor Cyan
# A malformed upstream title with a literal CR/LF (observed live: ci-scan issue
# #35957) must be collapsed to a single line so it cannot split the markdown table
# row in the rendered Preview tracker body. The existing pipe / angle-bracket
# escaping contract must remain intact.
Assert-Eq -Label "Format-MarkdownCell: LF collapsed to single space" -Expected 'a b' -Actual (Format-MarkdownCell "a`nb")
Assert-Eq -Label "Format-MarkdownCell: CRLF run collapsed to single space" -Expected 'a b' -Actual (Format-MarkdownCell "a`r`n`r`nb")
Assert-Eq -Label "Format-MarkdownCell: no CR/LF survives in the cell" -Expected $false -Actual ((Format-MarkdownCell "x`ny") -match "`r|`n")
Assert-Eq -Label "Format-MarkdownCell: pipe still escaped" -Expected 'a \| b' -Actual (Format-MarkdownCell 'a | b')
Assert-Eq -Label "Format-MarkdownCell: angle brackets still escaped" -Expected 'List&lt;T&gt;' -Actual (Format-MarkdownCell 'List<T>')

Write-Host "`n────────────────────────────────────────" -ForegroundColor Cyan
Write-Host "Passed: $script:passed Failed: $script:failed" -ForegroundColor $(if ($script:failed -eq 0) { 'Green' } else { 'Red' })
exit $(if ($script:failed -eq 0) { 0 } else { 1 })
Loading