Migrate FindAssembliesWithReferencesTo to IMultiThreadableTask

[OvesN]

Fixes dotnet/msbuild#14051

Migrate FindAssembliesWithReferencesTo to IMultiThreadableTask

[Copilot]

Pull request overview

This PR migrates the Razor SDK task FindAssembliesWithReferencesTo to MSBuild’s IMultiThreadableTask model (per dotnet/msbuild#14051), flowing TaskEnvironment through to reference scanning so file I/O resolves relative paths correctly under multi-threaded builds.

Changes:

• Mark FindAssembliesWithReferencesTo as [MSBuildMultiThreadableTask], implement IMultiThreadableTask, and pass TaskEnvironment into the resolver.
• Update ReferenceResolver to resolve assembly paths via TaskEnvironment.GetAbsolutePath(...) before performing File.Exists / File.OpenRead.
• Add a new agency.toml configuration file at repo root.

Reviewed changes

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

File	Description
src/RazorSdk/Tasks/ReferenceResolver.cs	Accepts TaskEnvironment and uses it to absolutize paths; refactors lookup dictionary initialization.
src/RazorSdk/Tasks/FindAssembliesWithReferencesTo.cs	Migrates the task to IMultiThreadableTask and flows TaskEnvironment into ReferenceResolver.
agency.toml	Introduces an MCP server configuration entry (appears unrelated to the PR’s stated purpose).

[OvesN]

Expert review — FindAssembliesWithReferencesTo → IMultiThreadableTask

Verdict: approve-with-comments. No bugs; production change is mechanical and conforms to the established [MSBuildMultiThreadableTask] + TaskEnvironment.GetAbsolutePath(...) pattern used by ~20 other migrated tasks. All substantive findings are about the new test file; none should block merge.

Substantive findings

1. Dead helper in test file  ·  cleanup
test/Microsoft.NET.Sdk.Razor.Tests/FindAssembliesWithReferencesToMultiThreadingTest.cs lines 73–74 — private static TaskEnvironment CreateMultiThreadedEnvironment() is never referenced. Looks like a leftover from an earlier iteration. Remove it, or wire it into a real parity test (see #2).

2. Missing multi-process / multi-threaded parity coverage  ·  test gap
Both new tests only exercise the multi-threaded path (TaskEnvironment.CreateWithProjectDirectoryAndEnvironment(...) with CWD elsewhere). Neither runs against TaskEnvironment.Fallback. The whole purpose of this migration is "behaves identically in both modes," and the canonical sibling test — test/Microsoft.NET.Build.Tasks.Tests/GivenAGenerateClsidMapMultiThreading.cs — asserts result/error equivalence across both modes precisely so a future regression in GetAbsolutePath semantics is caught. A drop-in of that ItProducesSameErrorsInMultiProcessAndMultiThreadedEnvironments / ItProducesSame…InMultiProcessAndMultiThreadedEnvironments pattern (run twice: once with Fallback + Directory.SetCurrentDirectory(temp.Path), once with the multi-threaded environment + CWD elsewhere, restore CWD in finally, assert outputs identical) would meaningfully harden this PR.

3. Tests never exercise the success path of GetReferences  ·  test gap (low)
Both tests write a 1-byte 0x00 stub, which makes PEReader throw BadImageFormatException; the catch returns an empty list, so the code that reads metadata, mutates Lookup, and classifies assemblies is not covered. Path-resolution wiring is proven (good), but the metadata-read step isn't. The Clsid test handles this by File.Copying typeof(GenerateClsidMap).Assembly.Location into the project dir — same technique works here (copy a real managed assembly, FusionName-tag it, assert it classifies against a chosen TargetAssemblyNames set).

4. Test 2's positive assertions are weak  ·  test gap (low)
Execute_PassesTaskEnvironmentToResolver_AndResolvesRelativeAssemblyItemSpecs asserts Execute() == true, no errors, and no RAZORSDK1007 warning, but never inspects task.ResolvedAssemblies. Combined with #3, the test would still pass if ResolveAssemblies silently returned something wrong as long as it didn't throw. Subsumed by fixing #3 — once a real assembly is in play, assert task.ResolvedAssemblies content.

Verified non-findings (earlier bot comments)

• "ToDictionary throws on duplicates" — incorrect; the diff keeps foreach { Lookup[item.AssemblyName] = item; } (indexer assignment, last-write-wins) at ReferenceResolver.cs:43-46. No ToDictionary call exists.
• "New agency.toml at repo root" — incorrect; no such file in the diff or repo. The PR changes exactly the 3 files listed.
• "No tests for FindAssembliesWithReferencesTo" — was valid for the first two commits; addressed by 80951c48 add tests.
• Thread safety of ReferenceResolver.Lookup under IMultiThreadableTask — safe. Each FindAssembliesWithReferencesTo.Execute() instantiates its own ReferenceResolver, so the per-GetReferences mutation of Lookup is per-task-instance; concurrent task instances do not share state.

Optional / cosmetic

• ReferenceResolver.cs:33-36 — if (taskEnvironment != null) { _taskEnvironment = taskEnvironment; } could be _taskEnvironment = taskEnvironment ?? TaskEnvironment.Fallback; to make the default explicit rather than relying on the field initializer. Equally, the dual constructors (2-arg delegating to 3-arg with null) are justified for binary-compat of a public class, but the "null means Fallback" semantic in the 3-arg overload is undocumented — a one-line XML doc would help.
• FindAssembliesWithReferencesToMultiThreadingTest.cs:6 — using System.Reflection; is unused.

[jankratochvilcz]

Thank you! The main bit for me is the architectural comment

[jankratochvilcz]

Hm, it worries me here that we have the pattern of passing around environment, which makes things quite implicit - we discussed previously that we want to use tasks as a boundary and do the transforms there without passing the environment deeper into the call chain. Looking here, it seems like here it would mean that we change the AssemblyItem.Path to an AbsolutePath - I briefly checked and the AssemblyItem seems not that widely used, so it might be viable. What do you think @OvesN?

[jankratochvilcz]

Please let's not reference "the migration" as it's unclear what that means outside of the context of the PR

[jankratochvilcz]

Should we also add a negative test?