Resolve source-host anchor assemblies by name (Phase 6e-4c1)

[Evangelink]

Phase 6e-4c1 — Resolve the source-host anchor assemblies by name

Part of the initiative to make MSTestAdapter.PlatformServices platform-agnostic by removing its dependency on the VSTest object model. Strict byte-for-byte refactor.

What this does

TestSourceHost used two typeof(...).Assembly anchors into the VSTest object model (both in netfx / net-non-uwp source-host setup):

1. typeof(EqtTrace).Assembly — force-loads Microsoft.TestPlatform.CoreUtilities into the child app domain to avoid a recursive assembly-resolution cycle (per the existing comment).
2. typeof(AssemblyHelper).Assembly — locates the test-platform directory for the resolution paths.

Both reflect parent-side loaded assemblies before the child domain resolves anything, so they are now resolved by simple name via a small GetLoadedAssembly(simpleName) helper (the same pattern proven in 6e-4b), returning the same loaded assembly identity the type references did.

Key detail (empirically verified against the built assemblies): the two anchors target different assemblies — EqtTrace is type-forwarded from the object model, so its defining assembly is CoreUtilities (matching the "Force loading … CoreUtilities" comment); AssemblyHelper lives in the object model. The helper is guarded to exactly the TFMs where the typeofs compiled (NETFRAMEWORK for the EqtTrace anchor; NETFRAMEWORK || (NET && !WINDOWS_UWP) for the AssemblyHelper anchor / GetResolutionPaths).

The only remaining object-model mention in the file is the assembly simple name as a string literal (not an assembly reference). This fully decouples TestSourceHost.

Fidelity proof

• PlatformServices.Desktop.IntegrationTests (net462 child-app-domain force-load + assembly-resolution + deployment paths): 15/15 green — the exact scenario these anchors protect.
• Timing: both run after the adapter/host loaded CoreUtilities + the object model, so the by-name lookups find them (no null / no fallback probing on the real path).

Verification

• Full build.cmd -c Debug green all TFMs (net462/net8.0/net9.0/UWP/WinUI), IDE0005 clean.
• PlatformServices.Desktop.IntegrationTests: 15, 0 failed.
• MSTestAdapter.PlatformServices.UnitTests: 897 (net8.0) / 935 (net462), 0 failed.

Remaining after this

2 files with real deps: TestSourceHandler (AssemblyHelper.DoesReferencesAssembly) and TestDeployment (SuspendCodeCoverage, netfx). Then Phase 7 (drop the PackageReference + guard test).

Stacking

… 6e-3b #9626 → 6e-4a #9627 → 6e-4b #9628 → 6e-4c1 (this), onto dev/amauryleve/vstest-decoupling-base. Base = 6e-4b branch (dev/amauryleve/vstest-decoupling-appdomain). Do not rebase/reset the base.

[github-actions]

Note

🤖 Automated review by GitHub Copilot. Posted via a maintainer's GitHub token, so it appears under their account — the account owner did not write or approve this content personally. Generated by the Expert Code Review workflow. To request a follow-up action, reply by tagging @copilot directly.

Expert TestFx Review — PR #9630 (Post-Merge)

Phase 6e-4c1: Resolve source-host anchor assemblies by name

Overall: a clean, well-scoped refactoring. Two minor/major findings on the new DoesSourceReferenceAssembly implementation; everything else is in good shape.

Note on diff fidelity: The diff used to trigger this review contained a rendering artefact in SuspendCodeCoverage.cs — the constructor header appeared duplicated inside the constructor body. The actual committed file (c561276) is correct. The DoesSourceReferenceAssembly implementation in the actual commit also differs from the diff (uses ArePublicKeyTokensEqual helper + broad catch instead of SequenceEqual + typed exception filter) — presumably a follow-up fixup commit was squashed before merge.

---

Verdict Table

#	Dimension	Status	Severity	Notes
1	Algorithmic Correctness	⚠️ Notes	MAJOR	ArePublicKeyTokensEqual(byte[], byte[]) does not guard against null arguments; both call sites pass GetPublicKeyToken() which returns byte[]?. Functionally saved today by the broad catch, but fragile (see inline comment on line 142).
2	Threading & Concurrency	✅ Clean	—	No shared mutable state. GetAssemblies() snapshot is inherently safe for a read.
3	Security & IPC Contract Safety	✅ Clean	—	Assembly-name strings are const; no user-controlled input reaches Assembly.Load. Env-var mutation in SuspendCodeCoverage is process-scoped and properly restored.
4	Public API & Binary Compatibility	✅ Clean	—	All new types (InvalidRunSettingsException, XmlReaderUtilities, SuspendCodeCoverage) are internal/internal sealed. No PublicAPI.Unshipped.txt update needed. No init accessors introduced.
5	Performance & Allocations	⚠️ Notes	MINOR	GetObjectModelAssembly() is called twice in SetConfigurationFile (lines 172 & 175), each time scanning GetAssemblies(). Inexpensive in context but easily avoided with a local (see inline comment on line 175).
6	Cross-TFM Compatibility	✅ Clean	—	#if NETFRAMEWORK / #if NETFRAMEWORK || (NET && !WINDOWS_UWP) guards are correctly scoped. ReflectionOnlyLoadFrom (netfx-only) stays behind #if NETFRAMEWORK. AppDomain APIs are correctly guarded.
7	Resource & IDisposable Management	✅ Clean	—	SuspendCodeCoverage.Dispose correctly restores the previous env-var value with a _isDisposed guard. No unmanaged resources; GC.SuppressFinalize not required.
8	Defensive Coding at Boundaries	⚠️ Notes	MAJOR	Same as #1 — ArePublicKeyTokensEqual receives potentially-null byte arrays without a null-guard. The broad catch { return null; } is the only runtime shield.
9	Localization & Resources	⚠️ Notes	MINOR	XmlReaderUtilities.ReadToRootNode throws new InvalidRunSettingsException($"Could not find '{RunSettingsRootNodeName}'...") with a hardcoded interpolated string. All similar messages elsewhere go through Resource.*. Low impact (internal diagnostic path), but inconsistent with project conventions.
10	Test Isolation	✅ Clean	—	SuspendCodeCoverage restores the process environment variable on dispose; no lasting side-effects between tests.
11	Assertion Quality	✅ Clean	—	Updated test assertions correctly target InvalidRunSettingsException instead of SettingsException. No tautological or trivial assertions introduced.
12	Naming & Conventions	✅ Clean	—	Constants follow PascalCase, private fields _camelCase. nameof not required (no member references to replace). ArePublicKeyTokensEqual is a clear, accurate name.
13	Error Handling Completeness	⚠️ Notes	MINOR	The actual committed DoesSourceReferenceAssembly uses catch { return null; } (catch-all). The diff version used a typed filter (BadImageFormatException | FileNotFoundException | FileLoadException). The typed filter is better practice as it preserves unexpected exceptions (OutOfMemoryException, etc.) from being silently swallowed.
14	Code Duplication & Reuse	⚠️ Notes	MINOR	TestSourceHost.GetLoadedAssembly(string simpleName) and AppDomainUtilities.GetObjectModelAssembly() are near-identical (same LINQ scan + Assembly.Load fallback). A shared TestPlatformAssemblyResolver utility (or promoting GetLoadedAssembly to AppDomainUtilities) would eliminate the duplication. Reasonable to defer to a follow-up decoupling phase.
15	MSTest/MTP-Specific Patterns	✅ N/A	—	No test attributes, TestContext usage, or retry logic changed.
16	Documentation & Comments	✅ Clean	—	All new types and helpers have accurate XML doc comments with <exception> documentation where relevant. The GetLoadedAssembly doc clearly states the "always loaded before this runs" invariant.
17	Dependency & Coupling	✅ Clean	—	The explicit goal of the PR: removes ObjectModel and ObjectModel.Utilities compile-time references from TestSourceHost, AppDomainUtilities, TestSourceHandler, MSTestAdapterSettings, RunSettingsUtilities. Net improvement.
18	Configuration & Flags	✅ Clean	—	Assembly simple-names ("Microsoft.VisualStudio.TestPlatform.ObjectModel", "Microsoft.TestPlatform.CoreUtilities") are stable VSTest contracts; const string is appropriate. __VANGUARD_SUSPEND_INSTRUMENT__ / "TRUE" match the well-known collector contract.
19	Diff Hygiene	⚠️ Notes	MINOR	Presented diff showed a corrupted SuspendCodeCoverage constructor (actual committed code is clean — diff artefact). Copyright-header-only changes on TestSourceHost.cs and AppDomainUtilities.cs (trailing-whitespace normalisation).
20	Test Coverage for Changed Code	⚠️ Notes	MINOR	DoesSourceReferenceAssembly / ArePublicKeyTokensEqual are not covered by the visible unit-test changes. The PR reports 897/935 PlatformServices unit tests pass and 15 integration tests pass, which provides reasonable integration-level coverage. A unit test for ArePublicKeyTokensEqual(null, ...) / (..., null) would lock in the expected behaviour and would guard against narrowing the catch.
21	MTP Extension Contracts	✅ N/A	—	No Microsoft.Testing.Platform extension code touched.
22	PowerShell / Script Quality	✅ N/A	—	No .ps1 or script files changed.

---

Summary

No blocking issues. The two actionable items are:

1. ArePublicKeyTokensEqual null safety (MAJOR, inline comment on TestSourceHandler.cs:142): update the signature to byte[]? and add a null/empty guard to make the "name-only match when token absent" behaviour explicit, as described in the PR. This also makes the broad catch safe to tighten back to a typed filter in a follow-up.

2. byte[] referenceAssemblyPublicKeyToken annotation (MAJOR, inline comment on TestSourceHandler.cs:113): the type should be byte[]? to match GetPublicKeyToken()'s return type.

Everything else is informational and can be addressed in follow-up phases.

[github-actions]

Nullable type annotation mismatch — latent NRE risk

AssemblyName.GetPublicKeyToken() returns byte[]?, but the local is declared as byte[] (non-nullable):

byte[] referenceAssemblyPublicKeyToken = referenceAssembly.GetPublicKeyToken();
//     ^^^^^^ should be byte[]?

This propagates into ArePublicKeyTokensEqual(byte[] left, byte[] right) which dereferences .Length unconditionally. In practice, MSTest's strongly-named assemblies always produce a non-null token, so this doesn't fire today — but the broad catch { return null; } is the only runtime guard. If the catch is ever narrowed to typed exceptions (as the original diff version showed) the NRE would surface as null instead of false.

Suggested fix:

byte[]? referenceAssemblyPublicKeyToken = referenceAssembly.GetPublicKeyToken();

And update ArePublicKeyTokensEqual to accept byte[]? (or add an upfront guard against null/empty in DoesSourceReferenceAssembly before passing to the helper).

[github-actions]

ArePublicKeyTokensEqual does not guard against null arguments

Both GetPublicKeyToken() calls at the call site can return null. The method signature accepts byte[] (non-nullable) and immediately accesses .Length, so a null token causes a NullReferenceException swallowed by the outer catch.

If the catch is ever tightened to specific exceptions (the original typed-filter version was more defensive), this will silently flip null→false instead of returning null from DoesSourceReferenceAssembly.

// Current signature
private static bool ArePublicKeyTokensEqual(byte[] left, byte[] right)

// Safer signature (handles unsigned / absent tokens explicitly)
private static bool ArePublicKeyTokensEqual(byte[]? left, byte[]? right)
{
    // If either side has no token, treat as a name-only match (unsigned assemblies)
    if (left is null || left.Length == 0 || right is null || right.Length == 0)
    {
        return true;
    }
    if (left.Length != right.Length) return false;
    for (int i = 0; i < left.Length; i++) { if (left[i] != right[i]) return false; }
    return true;
}

This matches the behaviour described in the PR description ("If either key token is absent, treat as match on name alone").

[github-actions]

GetObjectModelAssembly() called twice in the same if-block

Both line 172 and this line call GetObjectModelAssembly(), which each time scans all loaded assemblies via AppDomain.CurrentDomain.GetAssemblies(). This is initialisation code so the cost is low, but the pattern is inconsistent — TestSourceHost.GetLoadedAssembly is invoked the same way. Caching in a local avoids redundant allocation and makes the intent clearer:

var objectModelAssembly = GetObjectModelAssembly();
string currentVersionOfObjectModel = objectModelAssembly.GetName().Version.ToString();
if (!string.Equals(currentVersionOfObjectModel, ObjectModelVersionBuiltAgainst, StringComparison.Ordinal))
{
    AssemblyName assemblyName = objectModelAssembly.GetName();
    ...
}

(Minor; fine to leave or fix as a follow-up.)