Pull request overview

This PR updates the Static Web Assets MSBuild task ComputeEndpointsForReferenceStaticWebAssets to participate in MSBuild’s multi-threaded task execution model, addressing dotnet/msbuild#11835 by making the task explicitly multi-threadable and environment-aware for path resolution.

Changes:

• Marked ComputeEndpointsForReferenceStaticWebAssets as [MSBuildMultiThreadableTask] and implemented IMultiThreadableTask.
• Added TaskEnvironment support and forwarded it into StaticWebAsset.ToAssetDictionary(...) so asset parsing/path handling uses the MSBuild-provided environment when available.

Review

Headline

Production change is correct and MT-safe, but the new test does not exercise the migration — it passes identically whether Execute() calls ToAssetDictionary(Assets, TaskEnvironment) or the pre-PR ToAssetDictionary(Assets). The test gives false regression coverage.

Major: new test does not pin the migration behavior

test/Microsoft.NET.Sdk.StaticWebAssets.Tests/StaticWebAssets/ComputeEndpointsForReferenceStaticWebAssetsMultiThreadingTest.cs

The only production diff is at src/StaticWebAssetsSdk/Tasks/ComputeEndpointsForReferenceStaticWebAssets.cs:27 — passing TaskEnvironment into ToAssetDictionary. The env is plumbed solely into StaticWebAsset.Normalize(env) (StaticWebAsset.cs:1086-1092), which mutates four fields:

ContentRoot  = !string.IsNullOrEmpty(ContentRoot) ? NormalizeContentRootPath(ContentRoot, env) : ContentRoot;
BasePath     = Normalize(BasePath);                       // env-independent: pure string Replace+Trim
RelativePath = Normalize(RelativePath, allowEmpyPath:true); // env-independent
RelatedAsset = !string.IsNullOrEmpty(RelatedAsset) ? Path.GetFullPath(env.GetAbsolutePath(RelatedAsset)) : RelatedAsset;

In this test, RelatedAsset = "", so only ContentRoot is env-sensitive. But in Execute() (lines 34–76) the dictionary value is consumed for asset.BasePath only (and asset.Identity for a log message). ContentRoot is never observed. The dictionary key is Identity, and the test pre-absolutizes it (projectAbsoluteContentRoot/candidate.js) and uses the same string for candidateEndpoint.AssetFile — so the lookup succeeds regardless of env.

Concrete demonstration. Revert line 27 to var assets = StaticWebAsset.ToAssetDictionary(Assets); and trace:

1. FromTaskItem(item, TaskEnvironment.Fallback) is called.
2. Normalize(Fallback) sets ContentRoot to <spawnDir>/wwwroot/ (wrong) instead of <projectDir>/wwwroot/.
3. dictionary.Add(asset.Identity, asset) adds the same key the test fixed (absolute).
4. assets.TryGetValue(assetIdentity, out asset) → hit.
5. RouteHasPathPrefix("candidate.js", "base", …) → false (pure string).
6. CombineNormalizedPaths("", "base", "candidate.js", '/') → "base/candidate.js".
7. Endpoints[0].ItemSpec == "base/candidate.js" ✅ — test still passes.

The wrong ContentRoot is silently materialized in the dictionary value and discarded.

Contrast with MergeConfigurationPropertiesMultiThreadingTest: it feeds a relative MSBuildSourceProjectFile and asserts on task.ProjectConfigurations[0].GetMetadata("Source") == "myRcl", which only succeeds if the env-aware Path.GetFullPath(env.GetAbsolutePath(...)) rooted the path under projectDir. Reverting the env wiring there does flip the test. The new test has no analogous env-dependent assertion.

Suggested test improvement

ComputeEndpointsForReferenceStaticWebAssets.Execute() does not consume any env-sensitive property of the matched asset, so there is no input shape that makes the task's Endpoints[] output depend on env. Any task-level test must therefore assert on intermediate state or be honest about being a smoke test.

Two reasonable options:

1. Move the regression to StaticWebAsset.ToAssetDictionary(items, env) — that's the actual migration surface and a clean unit. One assertion covers every task that gets migrated this way:

   var dict = StaticWebAsset.ToAssetDictionary(items, env);
   dict[absId].ContentRoot.Should().StartWith(projectDir); // not spawnDir

   This fails immediately if any caller drops the env argument, and it would also fail if the production change here were reverted.

2. Keep this test as a smoke test and rename the method + comment to reflect that ("task runs to completion with a non-Fallback env and produces the expected route shape"), so future readers don't mistake it for a regression test for the env plumbing.

I'd lean (1).

Minor

• MergeConfigurationPropertiesMultiThreadingTest.cs:13-16 documents that CWD mutation is safe because [assembly:CollectionBehavior(DisableTestParallelization = true)] is set in LegacyStaticWebAssetsV1IntegrationTest.cs. The new file relies on the same invariant but omits the comment. Not a bug (the attribute is still in place), just makes the safety contract less discoverable.
• The trailing "st" in ComputeEndpointsForReferenceStaticWebAsset**st** (PR title, branch, test class) is a typo carried through. Cosmetic.

Verified

• Normalize(env) (StaticWebAsset.cs:1086) touches only ContentRoot/BasePath/RelativePath/RelatedAsset. Identity is untouched.
• Identity getter (StaticWebAsset.cs:86-102) is env-independent for in-memory StaticWebAsset items.
• Execute() body reads only asset.BasePath and asset.Identity from the dictionary value.
• CombineNormalizedPaths, RouteHasPathPrefix, StaticWebAssetEndpoint.FromTaskItem are pure — no CWD reads.
• MT-safety of Execute(): locals only, no static cache writes, no Environment.CurrentDirectory or unscoped Path.GetFullPath. Looks correct.
• [assembly:CollectionBehavior(DisableTestParallelization = true)] is still present.