[fix] Fix tilde/exclamation characters corrupted in TerminalLogger test output

[nohwnd]

Caution

Security scanning requires review for Issue Repro Triage & Auto-Fix 🔌

Details

Potential security threats were detected in the agent output. The workflow output should be reviewed before merging.

Review the workflow run logs for details.

Summary

🛠 This is an automated fix for issue #15268.

Fixes #15268 — ~, !, and | characters in test output (assertion messages, test display names, stack traces) were being corrupted when using the TerminalLogger (dotnet test with live logger enabled).

Root Cause

MSBuildLogger.Escape() serializes test messages to a single line for transport over stdout between vstest.console and VSTestTask2. The old encoding used ~~~~ for \r and !!!! for \n, with a pre-sanitization step that replaced those sequences in user data with ____ — lossy, and also ate |||| (4 pipes) in the same way.

Fix

Switch to %-based escaping, same idea as URL encoding:

Character	Escaped
%	%%
|	%p
\r	%r
\n	%n

This is lossless — no user data is ever replaced with underscores. It also stays readable in MSBuild binary logs (unlike the intermediate \x02/\x03 control char approach, which would show up as invisible garbage in binlog viewers).

The decoder is a single-pass scanner, not chained Replace calls. Chained Replace would incorrectly decode %%n as % + newline instead of literal %n.

Tests

• MSBuildLoggerEncoderTests — round-trip tests through FormatMessage/Unescape for tildes, bangs, pipes, %, %n in user data, and multi-field messages.
• VSTestTask2RegressionTests — decoder-side tests verifying %r%n restores newlines, %p restores pipes, %% restores percent, and tilde/exclamation chars survive unchanged.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes corruption of ~ and ! characters in TerminalLogger-transported test output by changing the newline escaping protocol between vstest.console and VSTestTask2.

Changes:

• Switched CR/LF encoding from printable sequences (~~~~/!!!!) to ASCII control chars (\x02/\x03) in MSBuildLogger.
• Updated VSTestTask2 decoding logic to restore CR/LF from \x02/\x03.
• Added regression tests covering tilde/exclamation preservation and newline round-tripping.

Reviewed changes

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

File	Description
test/Microsoft.TestPlatform.Build.UnitTests/VSTestTask2RegressionTests.cs	Adds regression tests to validate decoding and prevent output corruption regressions.
src/vstest.console/Internal/MSBuildLogger.cs	Updates message formatting/escaping to use control characters for newline transport.
src/Microsoft.TestPlatform.Build/Tasks/VSTestTask2.cs	Updates message decoding to match the new control-character encoding.

[nohwnd]

Review Summary

The fix is correct and well-scoped. Using ASCII control characters \x02/\x03 (STX/ETX) as newline placeholders instead of ~~~~/!!!! eliminates the data-corruption bug: the old approach would mangle any user input containing those exact 4-character sequences.

IPC Protocol Stability note: This is a wire-protocol change between MSBuildLogger (vstest.console) and VSTestTask2. There is no version negotiation — a mismatched deployment (old VSTestTask2 + new vstest.console) would see literal \x02/\x03 characters in output instead of newlines, with no error. Since both components co-ship in the .NET SDK this is low practical risk, but it's worth being aware of for any scenario where these are installed independently.

One inline comment on test coverage (not blocking).

---

🧠 Reviewed by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[nohwnd]

Commit pushed: 667f4e7

🔧 Iterated by PR Iteration Agent 🔧

[nohwnd]

Review Summary (updated commit)

The new commit addresses the round-trip test gap from the prior review by adding MSBuildLoggerEncoderTests.cs and exposing FormatMessage as internal. The core fix remains correct and well-scoped.

One non-blocking observation on the new test file: the Decode helper duplicates VSTestTask2.TryGetMessage decoder logic locally rather than invoking the real decoder, which means encoder/decoder drift could go undetected. See inline comment.

Everything else looks good.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[nohwnd]

Commit pushed: 4ad352f

🔧 Iterated by PR Iteration Agent 🔧

[Copilot]

Pull request overview

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

[nohwnd]

Review Summary (updated commit)

Both observations from the prior reviews have been addressed:

1. Round-trip test gap — MSBuildLoggerEncoderTests.cs now calls MSBuildLogger.FormatMessage() directly for encoding, covering the full encoder→decoder path.
2. Decoder drift risk — The Decode helper in the test file now carries a clear warning comment that it must stay in sync with VSTestTask2.TryGetMessage.

The core fix remains correct and well-scoped. No new issues found.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[nohwnd]

Review Summary (updated commit — %-escaping)

The latest commit (772650a) replaces the \x02/\x03 control-character approach with %-based escaping. The implementation is correct and well-reasoned:

• Encoder order is correct: % → %% is applied before any %x escape sequences are introduced, so chained Replace on the encoder side is safe.
• Decoder single-pass is correct: %%n → %n (not % + newline) because the scanner consumes both chars atomically when it sees %.
• Splitter safety holds: | in user data is now %p, so no |||| can appear in any data field — the split is unambiguous.
• Prior review observations (test coverage gap, decoder drift warning) were addressed in earlier commits and remain intact.

No new issues found. The fix is lossless, readable in MSBuild binary logs, and the test coverage is thorough.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Copilot]

Pull request overview

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

[nohwnd]

Review Summary (updated commit — e2e test coverage)

The latest commit (b0c0741) adds end-to-end test coverage for the special-character encoding fix:

• FailingTestWithSpecialChars in TerminalLoggerTestProject/UnitTest1.cs exercises all four special chars (~~~~~, !!!!, ||||, %n) in a single assertion, covering the full encoder→stdout→decoder→TerminalLogger round-trip.
• DotnetTestMSBuildOutputTests verifies each character group appears in terminal output after dotnet test -tl:on.
• Test count updates (Failed: 1→2, Total: 3→4) are correct in both disabled-logger tests.

All prior review observations have been addressed. The fix is correct and well-tested end-to-end.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[nohwnd]

Review Summary (updated commit — boxing fix + doc correction)

Commit 269271bb addresses two minor issues observed in the previous round:

1. Boxing in Unescape — The switch expression was replaced with a switch statement to ensure all sb.Append(...) calls receive char literals directly, avoiding the StringBuilder.Append(object) overload that would box each char. The fix is correct: all four case branches and the default branch now call Append(char).

2. XML doc example — The message names in the SendMessage XML doc (run-start1, test-failed6) carried a parameter-count suffix that was never emitted by the code. These are now corrected to run-start and test-failed.

Both changes are accurate and no new issues were introduced. The fix, its tests (unit + e2e), and the decoder in VSTestTask2 are all correct and consistent.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠