Fix TRX logger mangling astral-plane (non-BMP) characters

[Azat Mukhametshin (azat-msft)]

Symptom

Every astral-plane (non-BMP) character written to a TRX file is corrupted into literal escape text. A test named party 🎉 done is recorded in the TRX as the literal text party \ud83c\udf89 done. The same happens to anything the emoji appears in: Console.WriteLine output (Output/StdOut, StdErr, DebugTrace), error messages and stack traces, TestMethod/@name and @className, computer name, attachment descriptions, owners and categories.

This hits emoji, rare CJK extensions, mathematical alphanumerics and several historic scripts. It affects the classic vstest path and the Microsoft Testing Platform path equally, because the bug is in the shared TRX writer, not in any adapter.

Root cause

XmlPersistence.RemoveInvalidXmlChar operates on UTF-16 code units:

// we are handling only #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD]
// because C# support unicode character in range \^@ to \uFFFF
string invalidChar = @"[^\x09\x0A\x0D\x20-\uD7FF\uE000-\uFFFD]";
return Regex.Replace(str, invalidChar, evaluator);

The allowed set stops at \uD7FF and resumes at \uE000, so it skips U+D800–U+DFFF entirely — the surrogate range. Each half of a valid, well-formed surrogate pair is therefore independently classified as an invalid XML character and escaped. The comment is self-refuting: it quotes the XML spec including [#x10000-#x10FFFF] and then excludes the only mechanism by which UTF-16 can represent that range.

After — a surrogate-aware scan:

if (char.IsSurrogatePair(str, i)) { /* copy both code units through untouched */ i++; continue; }
if (IsValidXmlChar(str[i])) { /* copy through */ continue; }
/* otherwise escape as \uXXXX */

This follows the same control flow as the Jsonite JSON writer in testfx (JsonWriter.cs), which solves the identical problem: look ahead on a high surrogate, emit both code units if a low surrogate follows, treat it as invalid otherwise. char.IsSurrogatePair(str, i) does the lookahead-and-validate in one call and is safe at end-of-string, so a trailing high surrogate correctly falls through to the invalid branch without any out-of-range indexing. As a side benefit the StringBuilder is allocated lazily, so the overwhelmingly common all-valid string is now returned as-is with no allocation, replacing an unconditional Regex.Replace.

Lone surrogates are still escaped — deliberately

The fix is emphatically not "allow U+D800–U+DFFF". That would trade a string-mangling bug for a "vstest emits unparseable XML" bug, which is strictly worse — it fails at read time for every consumer of the TRX instead of degrading one string. A lone/unpaired surrogate is not a valid Unicode scalar value and is not valid XML — XmlWriter throws on it — so it must continue to be escaped. Only properly paired surrogates pass through. The comment in the source now spells this out, so it doesn't get "simplified" away later.

Covered cases: valid pair anywhere in the string survives byte-for-byte; lone high surrogate escaped; lone low surrogate escaped; high surrogate as the final character escaped without out-of-range indexing; low surrogate as the first character escaped. Behaviour for all other invalid characters (\0, \v, \x05, \uFFFE, …) and for all valid BMP characters is unchanged.

An existing test encoded the old, wrong behaviour

XmlPersistenceTests.SaveObjectShouldReplaceInvalidCharacter put (char)0xd800 and (char)0xdc00 adjacent in its "invalid characters" array and asserted the output contained \ud800\udc00. Those two chars form a valid surrogate pair for U+10000, which is legal XML, so the assertion was wrong. Rather than deleting the test, it has been split so both halves of the behaviour are pinned:

• SaveObjectShouldReplaceInvalidCharacter keeps the genuinely invalid 0x5, 0xb, 0xf, 0xfffe, 0x0 with unchanged expectations, guarding against over-correcting.
• SaveObjectShouldNotReplaceAdjacentHighAndLowSurrogate asserts that the exact (char)0xd800, (char)0xdc00 sequence now round-trips unescaped.
• SaveObjectShouldReplaceLoneHighSurrogate asserts a lone (char)0xd800 still comes out as \ud800.

Further new tests: valid pair preserved through an attribute value and through element inner text; lone low surrogate escaped; high surrogate at end-of-string escaped; a string mixing a valid pair, a lone surrogate and other invalid characters handled correctly in one pass; BMP non-ASCII (Grüße 日本語 Čau) untouched; and a round-trip test asserting the produced XML still parses with XmlDocument.LoadXml with a real emoji present in both an attribute and element text.

Validation

Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests — 164/164 passing in Release on both target frameworks (net481 and net11.0).

Precedent: the HTML logger in this repo already gets this right

Microsoft.TestPlatform.Extensions.HtmlLogger has its own copy of this sanitization and already handles surrogate pairs correctly (HtmlLogger.cs:44-46):

private static readonly Regex InvalidXmlCharsRegex = new(
    @"[^\x09\x0A\x0D\x20-\uD7FF\uE000-\uFFFD\uD800-\uDFFF]|[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(?<![\uD800-\uDBFF])[\uDC00-\uDFFF]",
    RegexOptions.Compiled);

That pattern excludes the surrogate range from the "invalid" class and then matches only lone surrogates via lookahead/lookbehind — semantically identical to what this change does for the TRX writer, just expressed as a regex rather than a scan.

So the correct behaviour was already established in this codebase; the TRX writer simply never received it. That makes the current TRX behaviour an inconsistency rather than a deliberate design choice, and it means the two loggers now agree on how a test name containing an emoji is recorded.

[Copilot]

Pull request overview

This PR fixes TRX XML serialization so astral-plane (non-BMP) Unicode characters (e.g., emoji) are preserved correctly instead of being escaped as literal \ud83c\udf89 surrogate text. The change is implemented in the shared TRX XML writer (XmlPersistence) so it benefits both the classic vstest and Microsoft Testing Platform flows.

Changes:

• Update invalid-XML-character filtering to preserve well-formed UTF-16 surrogate pairs while still escaping lone/unpaired surrogates.
• Adjust the existing “invalid character” unit test to remove an accidentally valid surrogate pair case.
• Add targeted unit tests covering surrogate pairs in element text/attributes, lone surrogates, mixed invalid content, and XML round-tripping.

Reviewed changes

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

File	Description
src/Microsoft.TestPlatform.Extensions.TrxLogger/XML/XmlPersistence.cs	Updates RemoveInvalidXmlChar to treat well-formed surrogate pairs as valid XML while continuing to escape lone surrogates and other invalid code units.
test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/XmlPersistenceTests.cs	Expands test coverage to validate correct handling of astral-plane characters and lone surrogates, and fixes an existing test that encoded incorrect behavior.

[Copilot]

Pull request overview

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

[Amaury Levé (Evangelink)]

Approving. I applied both files onto current main, built -c Release (0 errors, no new warnings) and ran the TrxLogger unit tests: 82/82 green on net481 (= 164 across both TFMs, matching your claim; net11.0 runtime isn't installed on my box). I also verified the fix end-to-end and audited the surrounding code for bypass paths. Details below, then a few non-blocking nits and one correction to the description.

Correctness — checked, not just skimmed

I traced the loop by hand across every branch:

Input	Result
"a\U0001F389"	builder never allocated, original str returned	correct
"\v\U0001F389"	\u000b + pair — backfill at i=0 is empty	correct
"\U0001F389\v"	pair + \u000b — Append(str, 0, i) with i == 2 backfills the pair	correct
"ab" + (char)0xd800	ab\ud800, no out-of-range	correct

The lazy-backfill index is right: builder ??= new StringBuilder(...).Append(str, 0, i) runs before the current char is appended, and i is always the index of that char and never mid-pair, because the pair branch continues after i++.

Your claim about char.IsSurrogatePair(string, int) being safe at end-of-string holds — the BCL implementation is literally if (index + 1 < s.Length) { return IsSurrogatePair(s[index], s[index + 1]); } return false;, so a trailing high surrogate falls through to the invalid branch rather than throwing.

Parity with the old regex is exact for everything other than paired surrogates: IsValidXmlChar reproduces [\x09\x0A\x0D\x20-\uD7FF\uE000-\uFFFD] character for character, so \uFFFE/\uFFFF stay escaped. And x4 formatting is culture-insensitive for hex, so replacing $@"\u{(ushort)x:x4}" with @"\u" + ToString("x4", InvariantCulture) is byte-identical output while also being CA1305-clean.

The fix has no bypass path

I grepped the whole TrxLogger project for writes into the XML DOM. The only string-valued ones are XmlPersistence.cs:656 (InnerText) and :659 (attribute .Value), both downstream of RemoveInvalidXmlChar in SaveSimpleData; the single direct SetAttribute at line 473 writes int counters. So the fix really does cover test names, class names, StdOut/StdErr, error messages, owners and categories as you claim — there is no second sanitization path to miss.

End-to-end verification

I reproduced TrxLogger.PopulateTrxFile's exact writer configuration (XmlWriter.Create(fs, new XmlWriterSettings { NewLineHandling = Entitize, Indent = true })) against a document containing U+1F389 in both an attribute and element text. Confirmed the file contains the correct 4-byte UTF-8 sequence F0 9F 8E 89 and round-trips through XmlDocument.Load.

Worth noting for the record: XmlWriterSettings.CheckCharacters defaults to true, so your decision to keep escaping lone surrogates isn't merely defensible — it's load-bearing. Letting \uD800-\uDFFF through wholesale would have thrown at write time. The long comment justifying that is the most valuable part of the diff; please keep it.

One correction to the PR description

It affects the classic vstest path and the Microsoft Testing Platform path equally, because the bug is in the shared TRX writer, not in any adapter.

This isn't right. There is no MTP TRX writer in this repo — MTP's TRX report lives in microsoft/testfx (Microsoft.Testing.Extensions.TrxReport/TrxReportEngine.Utilities.cs) with its own private RemoveInvalidXmlChar, and it was already fixed there by microsoft/testfx#8451 (merged 2026-05-21, tracked by microsoft/testfx#8416). Their implementation is functionally identical to yours, down to the lazily-allocated StringBuilder and the extracted IsValidXmlChar helper.

That's a point in this PR's favour rather than against it — it's now the third independent implementation in the org converging on the same shape — but the description should say MTP has a separate writer that was fixed separately, and cite testfx#8451 as the direct precedent. The HtmlLogger precedent you cite is accurate, by the way; I verified HtmlLogger.cs:44-46 does exclude the surrogate range from the invalid class and match only lone surrogates via lookaround, and it also keeps \uFFFE invalid. The two loggers in this repo now genuinely agree.

Risk

Low. The behaviour change is confined to well-formed surrogate pairs, which previously could only ever produce corrupted output. The single thing to be aware of: TRX files are no longer implicitly ASCII-safe on the wire, so any downstream consumer that assumed that will now see multi-byte UTF-8. That's spec-correct XML and matches what MTP has emitted since May, so I'm comfortable — but it's the only place a surprise could surface.

Nits inline; none of them block.

[Amaury Levé (Evangelink)]

Nit: builder?.Append(str, i, 2) says the same thing in one call and makes it obvious the two code units are copied as a unit. The chained form is also slightly subtle in that the null-conditional short-circuits the whole chain — correct here, but it's the kind of thing that looks like a bug on a fast read.

[Amaury Levé (Evangelink)]

Nit: this capacity hint is guaranteed to be too small. We only ever get here because a char needs escaping, and each escaped code unit expands 1 -> 6 chars, so the builder will always have to grow at least once.

testfx solved the identical problem in TrxReportEngine.Utilities.cs with length + length / 2 plus an overflow guard:

private static int GetInvalidXmlCharBuilderCapacity(int length)
{
    int extraCapacity = length / 2;
    return length <= int.MaxValue - extraCapacity ? length + extraCapacity : length;
}

They also use the new StringBuilder(str, 0, i, capacity) constructor, which does the prefix copy as part of construction instead of a separate Append. Worth matching, both for the allocation and because it keeps the two implementations diffable.

[Amaury Levé (Evangelink)]

This mixed-input test is the best one in the set — it's the one that would actually catch a regression in the backfill index.

Three edges are still unpinned, and they're precisely the ones that break if someone later "simplifies" the surrogate branch:

• two consecutive high surrogates, (char)0xd800 + (char)0xd801 — both must be escaped
• the inverted order, (char)0xdc00 + (char)0xd800 — low then high, neither forms a pair
• the empty string, which must return without allocating

All three behave correctly today (I checked), so this is purely about locking the behaviour down. A single [DataRow]-driven test would cover them and let you collapse some of the near-duplicate cases above.

[Amaury Levé (Evangelink)]

Broader point about the test strategy rather than this specific test: every new test targets XmlPersistence in isolation, but the premise of the PR is about the bytes that end up in the .trx file. Nothing here would catch a regression outside the sanitizer — e.g. someone setting CheckCharacters = false on the XmlWriterSettings in TrxLogger.PopulateTrxFile, or a future change that writes a string into the DOM without going through SaveSimpleData.

TrxLoggerTests already has a PopulateTrxFile override harness that writes a real file. One test through that path — emoji in a test name, write, re-parse, assert the name survived — would pin the thing users actually reported, and it's cheap. Not a blocker for this PR, but I'd like it before this pattern gets copied again.