fix(ci): PR test selector could silently mis-attribute changed paths containing tabs or newlines

[Ankit Jain (radical)]

What broke

The selective PR CI test-selector (tools/SelectTests) has two layers that independently parse git diff output: Layer 1 (GraphAffectedProjects.cs, MSBuild project graph) and Layer 2 (Program.cs, curated map). Layer 1 used git's default text-mode --name-status -M output; Layer 2 used a NUL-delimited -z parser. Git's text mode quote-escapes tabs/newlines/quotes/backslashes in a path, so a changed file containing one of those bytes never matched Layer 1's attribution index — the change silently failed to select its own project, the opposite of the selector's fail-safe design.

While tracing this (from PR #19486, CI run 33197019835), a few adjacent gaps surfaced in the same code:

• Layer 1 also unconditionally rewrote \ → / in every changed path, even though git never uses \ as a separator — a literal backslash in a filename could be corrupted into a fake directory boundary and mis-attributed to an unrelated project.
• Raw changed-file paths (which can contain literal \r/\n/backticks) were interpolated into the job summary and PR comment Markdown unescaped.
• A truncated -z stream (missing the final NUL) parsed as if it were valid instead of failing loudly.

Root cause

Layer 1 had its own hand-rolled parser, independent from and less safe than Layer 2's. The two layers had quietly drifted.

The fix

• Layer 1 now issues the same git diff --name-status -M -z as Layer 2, and both parse it through one shared method, ParseNameStatusOutput (tools/SelectTests/Program.cs).
• Removed Layer 1's \→/ path rewrite; both layers now trust git's raw /-separated paths, matching Layer 2's existing behavior.
• Added an EscapeForDisplay helper and applied it everywhere a raw path is rendered into Markdown.
• ParseNameStatusOutput now throws if a -z stream doesn't end with a NUL terminator.

Why this approach

A separate fix was also attempted and rejected: exempting a rename's old path from the run-all fallback when its new path was already present in the changed-file set. An audit found that unsafe — a directory-scoped glob can have consumers beyond the file that moved, and a rename landing on an ignored glob or dropped by the prefilter satisfies the exemption's presence check without anything evaluating the real destination. Both could silently under-select tests. The exemption was removed; renames remain un-special-cased, and an unmatched rename old path still forces ALL, same as a plain deletion. docs/ci/test-trigger-map.md documents this under "Rename handling."

Call-out

Replaying PR #19486's exact diff still selects ALL — that PR's original symptom isn't "fixed" here, and that's intentional: an unnecessary full run is safer than a silently skipped test. The actual bug fixed is the narrower NUL-safety/escaping issue above, found while tracing the rename investigation.

Testing

New coverage under tests/Infrastructure.Tests/TestTriggerMap/ (169 tests passing) covers: tab/newline-containing renamed paths attributed correctly by both layers, truncated -z streams failing loudly, Markdown escaping of raw paths, the backslash-corruption false-attribution case, and the rename-exemption-removed behavior — including a golden replay of PR #19486's real diff (base fd9bbf76b4 / head 1e46e48122335ba5e970a5f78777a44fe3e963d2) confirming it still forces ALL.

Fixes # (no tracked issue — found during investigation of #19486)

[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/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19790

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19790"

[Copilot]

Pull request overview

Updates selective CI to distinguish Git-detected rename sources and avoid unnecessary full-matrix fallbacks.

Changes:

• Parses Layer 2 changes using git diff --name-status -M.
• Exempts unmatched rename-old paths from fallback selection.
• Adds regression tests and documents rename-threshold behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tools/SelectTests/TestSelector.cs	Adds rename-old-path exemption logic.
tools/SelectTests/Program.cs	Parses Git rename metadata.
tests/Infrastructure.Tests/TestTriggerMap/TestTriggerMapTests.cs	Adds production-map regression coverage.
tests/Infrastructure.Tests/TestTriggerMap/SelectTestsCliTests.cs	Adds Git integration scenarios.
tests/Infrastructure.Tests/TestTriggerMap/SelectTestsAcceptanceTests.cs	Tests selector exemption behavior.
docs/ci/test-trigger-selector-design.md	Documents selector design changes.
docs/ci/test-trigger-map.md	Documents rename fallback semantics.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

tools/SelectTests/TestSelector.cs:267

• This exemption applies to every detected rename, not only the stated case where the map rule moves with the file. For example, if an otherwise-unmapped loose CI input is renamed to a path covered by ignore (or filtered out before Select), the new path contributes no target and this branch suppresses the old path's fail-safe, so the selector can return no tests where the previous delete+add handling returned ALL. Preserve the fallback unless the corresponding new path is demonstrably accounted for by a moved/equivalent rule (which requires retaining the old/new pairing and validating the mapping), rather than exempting old paths solely because Git labeled them as renames.

            if (renameOldPathSet.Contains(file))
            {
                continue;
            }

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

tools/SelectTests/Program.cs:497

• This malformed-record branch also silently returns a partial diff, allowing changed files to disappear from selection. Abort so CI fails visibly rather than potentially under-selecting tests.

                if (tokenIndex >= tokens.Length)
                {
                    break; // truncated/malformed output; nothing left to parse

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

tests/Infrastructure.Tests/TestTriggerMap/SelectTestsAcceptanceTests.cs:360

• This comment records within-branch review history rather than the enduring regression condition. Remove the current PR/review reference so the test remains understandable after merge.

        // Regression test for PR #19790 review feedback: the exemption above must require the rename's

tests/Infrastructure.Tests/TestTriggerMap/SelectTestsCliTests.cs:1068

• This comment records within-branch review history rather than the enduring regression condition. Remove the current PR/review reference so the test remains understandable after merge.

    // Regression test for PR #19790 review feedback: the rename-old-path exemption above must not fire

docs/ci/test-trigger-selector-design.md:308

• This design contract omits the implemented requirement that the paired destination survive the prefilter. It therefore describes a broader exemption than TestSelector actually applies and incorrectly states it cannot under-select despite the false-rename limitation documented in test-trigger-map.md. Document the destination-membership guard and avoid the absolute safety claim.

   the prefilter in step 1, so they never reach this fallback. **Exception:** the
   old path of a git-detected rename (see [Changed paths](#changed-paths)) that
   matches nothing is not treated as an unmapped leftover — a same-commit rename
   that also updates the map's own rule to the new name (as
   [microsoft/aspire#19486](https://github.com/microsoft/aspire/pull/19486) did)

tests/Infrastructure.Tests/TestTriggerMap/TestTriggerMapTests.cs:515

• “this PR fixes” narrates the current branch rather than the selector behavior that the committed test documents. Reword it as a durable statement of scope.

    // this PR fixes the in-place-rename-with-moved-map-entry mechanism, not git's own rename-similarity
    // detection, and does not claim to change the fallback outcome for content-heavy rewrite+renames.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

tests/Infrastructure.Tests/TestTriggerMap/TestTriggerMapTests.cs:466

• This regression test depends on #19486 not having merged: that PR's head already contains only the plural filenames, so once it lands these assertions for the old names fail before the scenario runs. Make the fixture durable by applying the replacements first and asserting that the resulting map contains the new names (or use a self-contained map), so it works whether the checked-out production map is pre- or post-#19486; update the accompanying “checked out on disk today” wording as well.

        var realMapText = File.ReadAllText(Path.Combine(RepoRoot.Path, "eng", "github-ci", "test-trigger-map.yml"));
        Assert.Contains(oldCommonPath, realMapText);
        Assert.Contains(oldUpdatePath, realMapText);
        Assert.Contains(oldVerifyPath, realMapText);

tools/SelectTests/Program.cs:1058

• The new display escaping still misses the fail-safe reason: TestSelector embeds the unmatched raw filename in EscalationReason, and WriteSelectionComment/WriteSummary emit that value verbatim (lines 835 and 1290). An unmatched path containing a newline or Markdown/HTML markup can therefore still spoof both audit surfaces. Escape that reason at both Markdown sinks (including HTML-sensitive characters because it is prose, not a code span) and cover an unmatched newline-containing path.

    // Escapes bytes a `-z`-parsed path (see ParseNameStatusOutput) can legally contain but that would
    // otherwise corrupt the Markdown this method renders into: `-z` intentionally preserves a path's raw
    // bytes -- including a literal newline, carriage return, or backtick -- that git's older text-mode
    // format would have quoted away. Left raw, such a path could inject extra lines/headings into the
    // step summary or PR comment, or break out of its enclosing `code span`. Escaping is display-only;
    // callers still match/attribute the unescaped path everywhere else.
    private static string EscapeForDisplay(string value)