Reimplement source assembly-reference check without ObjectModel (Phase 6e-4c2)

[Evangelink]

Phase 6e-4c2 — reimplement the source assembly-reference check without ObjectModel

Part of the initiative to make Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices platform-agnostic by removing its dependency on Microsoft.TestPlatform.ObjectModel. VSTest coupling moves up into MSTest.TestAdapter; the platform-services engine becomes neutral. Strict byte-for-byte, no behavior change.

What this changes

TestSourceHandler.IsAssemblyReferenced (netfx-only) decided whether a source assembly references the test framework — used to skip discovery on sources that don't reference MSTest — by calling AssemblyHelper.DoesReferencesAssembly from Microsoft.VisualStudio.TestPlatform.ObjectModel.Utilities.

This replaces that call with a local neutral DoesSourceReferenceAssembly helper that reproduces the exact observable behavior of the VSTest implementation:

• Assembly.ReflectionOnlyLoadFrom(source) → GetReferencedAssemblies().
• Match a referenced assembly by simple name (OrdinalIgnoreCase) plus public-key-token bytes; version is ignored — identical to AssemblyLoadWorker.CheckAssemblyReference.
• Name match + public-key-token length/byte mismatch keeps scanning further references (mirrors the original continue), rather than short-circuiting.
• Null/empty source or null reference assembly → null (undeterminable).
• Any exception → null, so discovery proceeds conservatively.

The IsAssemblyReferenced decision line (return !utfReference.HasValue || utfReference.Value; — null-or-true ⇒ proceed, false ⇒ skip) is unchanged.

Fidelity note (dead child-AppDomain)

VSTest's DoesReferencesAssembly created a child AppDomain and an AssemblyLoadWorker instance, but then called the static AssemblyLoadWorker.CheckAssemblyReference(...) — the worker instance is assigned and never used, and the ReflectionOnlyLoadFrom actually runs in the current domain. The child domain is therefore dead code for the result, so omitting it is behavior-preserving for every input where AppDomain.CreateDomain would have succeeded (the only theoretical divergence — a machine where domain creation throws but reflection-only load succeeds — is unreachable in practice, and the conservative null-return there would only proceed with discovery either way).

Removes the last real ObjectModel dependency from TestSourceHandler (it now carries only string-literal well-known-assembly names, which don't reference the package).

Verification

• All real TFMs build 0-warning (UWP builds via full msbuild in CI, unaffected — change is #if NETFRAMEWORK-guarded).
• MSTestAdapter.PlatformServices.UnitTests: 935/935 (net462), 897/897 (net8.0).
• DesktopTestSourceTests — the direct IsAssemblyReferenced net — 7/7, covering: assembly referenced (true), not referenced (false), null name (true), null source (true).
• PlatformServices.Desktop.IntegrationTests: 15/15 (net462).
• Expert-reviewer pass.

Stacking

Stacks on #9630 (Phase 6e-4c1); base branch dev/amauryleve/vstest-decoupling-sourcehost. Review/merge after the earlier PRs in the chain reach the base. Do not squash-rebase 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.

Review Summary — Phase 6e-4c2 (TestSourceHandler ObjectModel removal)

The architectural goal is correct and well-executed. The decoupling is clean, the #if NETFRAMEWORK guard is properly applied, and the fidelity note in the PR description is accurate about the dead child-AppDomain. One MAJOR null-safety bug in the helper method needs a follow-up fix; two MINOR polish items are noted.

---

Verdict: NEEDS WORK

#	Dimension	Status	Severity	Note
1	Algorithmic Correctness	⚠️ Minor	MAJOR	Logic is correct for signed assemblies (the production case). For unsigned assemblies (null PKT), ArePublicKeyTokensEqual throws NRE → caught → null → proceed. Accidentally correct outcome, wrong code path.
2	Threading & Concurrency	✅ Clean	—	Static method, local variables only, no shared state.
3	Security & IPC Contract Safety	✅ Clean	—	ReflectionOnlyLoadFrom is read-only; no code execution; source path supplied by caller.
4	Public API & Binary Compatibility	✅ Clean	—	No public API changes; removed using for ObjectModel is a positive dependency reduction.
5	Memory Management & Resource Leaks	✅ Clean	—	Reflection-only load context limitations are pre-existing; behaviour unchanged from original call.
6	Error Handling	⚠️ Minor	MINOR	Bare catch absorbs all exceptions silently; no diagnostic trace makes debugging difficult (see inline comment on line 135).
7	Naming & Code Style	✅ Clean	—	DoesSourceReferenceAssembly, ArePublicKeyTokensEqual are clear verb-led names; file-scoped namespace, no abbreviations.
8	Documentation & Comments	⚠️ Minor	MINOR	XML doc on DoesSourceReferenceAssembly is accurate. Stale comment on line 78 ("different app domain") is now inaccurate (see inline comment).
9	Test Coverage	✅ Clean	—	7/7 DesktopTestSourceTests reported; edge cases (null name, null source, referenced/not-referenced) are covered per PR description. Null-PKT path not explicitly tested but the outer catch provides the safety net.
10	Performance	✅ Clean	—	StringComparison.OrdinalIgnoreCase used; no unnecessary allocations in the loop.
11	Cross-TFM Correctness	✅ Clean	—	All new code is inside #if NETFRAMEWORK; Assembly.ReflectionOnlyLoadFrom is .NET Framework-only.
12	Localization	N/A	—	No user-facing strings added.
13	Dependency Management	✅ Clean	—	Positive change: removes the ObjectModel using directive from PlatformServices.
14	Telemetry & Observability	N/A	—	No new user-visible behaviour; existing discovery flow preserved.
15	CI/CD & Build System	✅ Clean	—	No build script changes; guarded by #if NETFRAMEWORK.
16	Backward Compatibility	✅ Clean	—	IsAssemblyReferenced return logic is unchanged; callers see identical results for all signed-assembly inputs.
17	Null Safety	❌ Issue	MAJOR	GetPublicKeyToken() returns null on .NET Framework for unsigned assemblies. ArePublicKeyTokensEqual(byte[] left, byte[] right) has no null guards; left.Length throws NullReferenceException. referenceAssemblyPublicKeyToken declared byte[] rather than byte[]?, hiding the nullable concern. See inline comment on line 144.
18	Exception Safety	✅ Clean	—	No partial-state mutations; catch is for observation, not rollback.
19	Immutability & Value Semantics	✅ Clean	—	Pure static methods operating on immutable inputs.
20	Design Patterns & Architecture	✅ Clean	—	Aligns perfectly with the stated goal; dependency removed at the correct layer.
21	Logging Correctness	⚠️ Minor	MINOR	No diagnostic output when the catch fires; a Debug.WriteLine would cost nothing and aid debugging.
22	Scope & PR Hygiene	✅ Clean	—	Single concern; no unrelated changes; no dead code introduced.

---

Required follow-up

ArePublicKeyTokensEqual — add null guards (line 144, inline comment posted):

private static bool ArePublicKeyTokensEqual(byte[]? left, byte[]? right)
{
    if (left is null && right is null) return true;
    if (left is null || right is null) return false;
    if (left.Length != right.Length) return false;
    for (int i = 0; i < left.Length; ++i)
    {
        if (left[i] != right[i]) return false;
    }
    return true;
}

Also fix the assignment on line 113 to byte[]? referenceAssemblyPublicKeyToken = referenceAssembly.GetPublicKeyToken();.

In practice MSTest is always shipped signed so null PKTs will not occur in the field, but the code is objectively wrong and should be fixed before being built upon in the next phase of this decoupling series.

[github-actions]

[MAJOR — Null Safety] GetPublicKeyToken() returns null on .NET Framework for unsigned assemblies (those with no public key token). Both left (from referencedAssembly.GetPublicKeyToken()) and right (from referenceAssembly.GetPublicKeyToken()) can therefore be null, so left.Length on this line throws NullReferenceException. The outer catch absorbs that and returns null (→ proceed with discovery), so the process doesn't crash; but there are two problems:

1. For two assemblies that both carry no public-key token, the semantically correct result is true (name match + neither is signed = same unsigned identity). Instead the caller gets a swallowed exception and falls through to "proceed" via null, which is accidentally correct today but for the wrong reason.
2. referenceAssemblyPublicKeyToken on line 113 is declared byte[] (non-nullable), masking the actual nullable return of GetPublicKeyToken(). It should be byte[]?.

Suggested fix:

private static bool ArePublicKeyTokensEqual(byte[]? left, byte[]? right)
{
    if (left is null && right is null) return true;
    if (left is null || right is null) return false;
    if (left.Length != right.Length) return false;
    for (int i = 0; i < left.Length; ++i)
    {
        if (left[i] != right[i]) return false;
    }
    return true;
}

Also update the assignment on line 113:

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

[github-actions]

[MINOR — Bare Catch / No Diagnostics] The catch block silently swallows every exception — file-not-found, BadImageFormatException, security exceptions, and the null-deref discussed on ArePublicKeyTokensEqual all land here invisibly. The conservative null return is intentional and correct, but without any trace output it is very hard to tell the difference between "file legitimately couldn't be opened" and "code bug".

Suggestion — at minimum emit a Debug.WriteLine (no user-visible surface, zero overhead in production):

catch (Exception ex)
{
    Debug.WriteLine($"[MSTest] DoesSourceReferenceAssembly could not inspect '{source}': {ex.Message}");
    return null;
}

[github-actions]

[MINOR — Stale Comment] The comment says "This loads the dll in a different app domain", but the new implementation (DoesSourceReferenceAssembly) uses Assembly.ReflectionOnlyLoadFrom in the current AppDomain — no child domain is created. Both sentences are now inaccurate (the second was an optimisation note that is now moot since the domain split was intentionally dropped).

Suggested replacement:

// Reflection-only loads the assembly to inspect its references without executing any code from it.