Thread TaskEnvironment through StaticWebAsset path helpers (foundation for dotnet/msbuild#14036)

[jankratochvilcz]

Fixes the latent transitive call-stack hazard in StaticWebAsset.Normalize() and related helpers (StaticWebAsset.cs:1067 and friends) where Path.GetFullPath(...) and new FileInfo(...) resolve relative paths against Environment.CurrentDirectory instead of the task's project directory. Tracks dotnet/msbuild#14036.

This is the foundational refactor that unblocks per-task migrations to fully MT-safe behavior (see PRs #54700-#54707 for the per-task migrations; the gated test in #54707 documents the bug).

What this PR does

Adds TaskEnvironment-aware overloads for every helper on StaticWebAsset that resolves a relative path against a base directory:

Helper	New overload
Normalize()	Normalize(TaskEnvironment env)
NormalizeContentRootPath(string)	NormalizeContentRootPath(string, TaskEnvironment)
FromTaskItem(ITaskItem, bool)	FromTaskItem(ITaskItem, TaskEnvironment, bool)
FromV1TaskItem(ITaskItem)	FromV1TaskItem(ITaskItem, TaskEnvironment)
FromTaskItemGroup(ITaskItem[], bool)	FromTaskItemGroup(ITaskItem[], TaskEnvironment, bool)
ToAssetDictionary(ITaskItem[], bool)	ToAssetDictionary(ITaskItem[], TaskEnvironment, bool)
ApplyDefaults()	ApplyDefaults(TaskEnvironment)
ResolveFile(string, string) / ResolveFile()	ResolveFile(string, string, TaskEnvironment) / ResolveFile(TaskEnvironment)
ComputeIntegrity(string, string)	ComputeIntegrity(string, string, TaskEnvironment)
HasContentRoot(string)	HasContentRoot(string, TaskEnvironment)
MaterializeFrameworkAsset(...)	overload with TaskEnvironment env parameter

Internally, each Path.GetFullPath(x) is replaced with Path.GetFullPath((string)env.GetAbsolutePath(x)) and each new FileInfo(x) with new FileInfo((string)env.GetAbsolutePath(x)). env.GetAbsolutePath roots x against TaskEnvironment.ProjectDirectory (MT-safe via MSBuild's AsyncLocal FileUtilities.CurrentThreadWorkingDirectory); the outer Path.GetFullPath preserves the prior canonicalization (.. resolution).

Back-compat

Every existing call site keeps compiling and behaves identically. The parameterless overloads are retained and simply delegate to the new ones with TaskEnvironment.Fallback, which is exactly the previous (multi-process) behavior — relative paths still resolve against the process CWD when no TaskEnvironment is supplied. Test NormalizeContentRootPath_WithoutEnvOverload_StillUsesProcessCurrentDirectory_ForBackCompat pins this.

Compatibility sins audit

• Sin 1 (output contamination): no new [Output]-bearing types touched.
• Sin 2 (error-message inflation): the ResolveFile exception message still reports the original (un-absolutized) identity / originalItemSpec inputs as supplied by the caller.
• Sin 3 (?? swallowing exceptions): no new null-coalescing introduced. ResolveFile adds a small AbsolutizeForFileInfo local function that explicitly short-circuits null/empty inputs to preserve the pre-existing FileInfo("") → ArgumentException path so the second FileInfo can still be tried.
• Sin 4 (try-catch scope): no new try blocks.
• Sin 5 (canonicalization): wrapping env.GetAbsolutePath in Path.GetFullPath preserves .. resolution — covered by NormalizeContentRootPath_WithTaskEnvironment_PreservesCanonicalization_DotDot.
• Sin 6 (exception type): env.GetAbsolutePath("") throws ArgumentException, same as Path.GetFullPath("") / new FileInfo(""). ResolveFile short-circuits null/empty so the second FileInfo is still attempted, preserving pre-existing behavior.

Test coverage

10 new unit tests in test/Microsoft.NET.Sdk.StaticWebAssets.Tests/StaticWebAssets/StaticWebAssetTaskEnvironmentTests.cs:

Test	Asserts
NormalizeContentRootPath_WithTaskEnvironment_AbsolutizesAgainstProjectDirectory_NotProcessCurrentDirectory	Decoy CWD ≠ project dir → result rooted at project dir
NormalizeContentRootPath_WithoutEnvOverload_StillUsesProcessCurrentDirectory_ForBackCompat	Parameterless overload preserves prior process-CWD behavior
Normalize_WithTaskEnvironment_AbsolutizesContentRootAndRelatedAssetAgainstProjectDirectory	Both fields end up under project dir
FromTaskItem_WithTaskEnvironment_HydratesAssetWithProjectDirectoryAbsolutizedPaths	End-to-end via the task item entry point
FromV1TaskItem_WithTaskEnvironment_HydratesAssetWithProjectDirectoryAbsolutizedPaths	Same for V1 (also exercises ApplyDefaults → ResolveFile)
FromTaskItemGroup_WithTaskEnvironment_AbsolutizesAllAssets	Bulk path
ResolveFile_WithTaskEnvironment_ResolvesIdentityRelativeToProjectDirectory	FileInfo correctly rooted
HasContentRoot_WithTaskEnvironment_ComparesAgainstProjectDirectoryNormalizedForm	Equality check still works for relative inputs
NormalizeContentRootPath_WithTaskEnvironment_PreservesCanonicalization_DotDot	.. resolution preserved
Normalize_WithTaskEnvironment_AbsolutePathInputs_ArePreservedAndCanonicalized	No surprise re-rooting of already-absolute inputs

All 10 pass locally (./.dotnet/dotnet exec ...Microsoft.NET.Sdk.StaticWebAssets.Tests.dll -method "*StaticWebAssetTaskEnvironmentTests*").

Follow-ups (separate PRs)

This PR ships only the StaticWebAsset API. Existing call sites still invoke the parameterless overloads and therefore retain the pre-PR behavior. Per-task migrations to thread TaskEnvironment into FromTaskItem* (and un-skip the gated demo test in #54707) will follow per task — small, mechanical change once the foundation is in place.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

[jankratochvilcz]

Here is the entry point if you're looking at the PR

[jankratochvilcz]

This is the thing the PR is fundamentally trying to fix

[jankratochvilcz]

Also here

[Copilot]

Pull request overview

This PR refactors StaticWebAsset path-resolution helpers to accept a TaskEnvironment, so relative paths can be rooted against the MSBuild task’s project directory (rather than the process current working directory). This is foundational for making Static Web Assets tasks safe under MSBuild multithreaded execution (dotnet/msbuild#14036), while retaining back-compat via TaskEnvironment.Fallback.

Changes:

• Add TaskEnvironment-aware overloads for StaticWebAsset helpers that resolve/normalize filesystem paths, with existing overloads delegating to TaskEnvironment.Fallback.
• Update internal path normalization and file resolution (Normalize*, ResolveFile, MaterializeFrameworkAsset, etc.) to absolutize via env.GetAbsolutePath and then canonicalize via Path.GetFullPath.
• Add a new unit test suite validating MT-safe rooting behavior and confirming parameterless overloads still use process CWD.

Reviewed changes

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

File	Description
src/StaticWebAssetsSdk/Tasks/Data/StaticWebAsset.cs	Introduces TaskEnvironment overloads and routes path resolution through env.GetAbsolutePath to avoid Environment.CurrentDirectory hazards under MT builds.
test/Microsoft.NET.Sdk.StaticWebAssets.Tests/StaticWebAssets/StaticWebAssetTaskEnvironmentTests.cs	Adds focused tests covering env-based absolutization/canonicalization and back-compat behavior for existing overloads.

[OvesN]

Review of #54708 — TaskEnvironment migration of StaticWebAsset helpers

Drive-by review from a Copilot CLI session, scoped strictly to the APIs migrated for thread-safety in this PR and the helpers they call directly. No other code reviewed.

Scope: APIs changed for thread-safety + their direct callees only.

🛑 Blockers

None. The migration preserves the Sin 2/5/6 contracts:

• Every pre-PR Path.GetFullPath(x) is preserved as Path.GetFullPath(env.GetAbsolutePath(x)) — canonicalization (.. resolution, separator normalization) is intact (Sin 5 avoided). See diff lines 91, 103, 142.
• All new log statements correctly use sourceFile.OriginalValue so error/log messages still show the user-supplied path, not the absolutized one (Sin 2 avoided). See diff lines 154, 163, 168.
• For every env.GetAbsolutePath(x) callsite, the empty/null case still throws the same exception type as pre-PR (or is guarded explicitly), so Sin 6 doesn’t bite.

⚠️ Concerns

1. MaterializeFrameworkAsset — early-return placed after destPath absolutization (diff lines 142–147, StaticWebAsset.cs around line 2125).
   The new guard is ordered:

   destPath = Path.GetFullPath(env.GetAbsolutePath(destPath));   // can throw
   if (string.IsNullOrEmpty(asset.Identity)) { log.LogError(...); return (null, null, null); }
   var sourceFile = env.GetAbsolutePath(asset.Identity);

   The empty-Identity guard exists specifically because env.GetAbsolutePath(null|"") would throw ArgumentException whereas pre-PR File.Exists(null|"") silently returned false. But by leaving the guard after the destPath absolutization, an unrelated bad input (empty intermediateOutputPath → empty destPath → ArgumentException from env.GetAbsolutePath) now masks the friendly "Source file does not exist" error path. Move the guard to the top of the method, before any path absolutization, so the failure mode for empty asset.Identity is independent of destPath validity. Pre-PR semantics were "log and return"; the guard placement should match that ordering.

2. ResolveFile short-circuit is cosmetic; PR description claims a behavior it doesn’t deliver (diff lines 200, 206, StaticWebAsset.cs around line 3007).
   The PR body says the short-circuit "preserves the pre-existing FileInfo("") → ArgumentException path so the second FileInfo can still be tried." That’s not quite right: new FileInfo("") throws ArgumentException, exiting the method before the second FileInfo is constructed. The behavior is identical pre- and post-PR, and identical with or without the short-circuit, because:

   • env.GetAbsolutePath(null) → ArgumentNullException (same type as new FileInfo(null))
   • env.GetAbsolutePath("") → ArgumentException (same type as new FileInfo(""))

   Either fix the PR description to say "short-circuit avoids an extra Path.Combine call for the empty case" (its real effect) or drop the short-circuit entirely to simplify the line. The exception-type match is already guaranteed by GetAbsolutePath's own validation — the playbook's Sin 6 audit confirms behavioral parity either way.

3. NormalizeContentRootPath(path, env) internal short-circuit is dead (diff line 103, StaticWebAsset.cs around line 1932).

   => Path.GetFullPath(string.IsNullOrEmpty(path) ? path : env.GetAbsolutePath(path)) + …switch { … };

   If path is "", the outer Path.GetFullPath("") still throws ArgumentException. If path is null, the right-hand path.EndsWith(...) throws NullReferenceException (and Path.GetFullPath(null) throws ArgumentNullException). The internal short-circuit cannot make either case observably succeed. All current callers already guard:

   • Normalize (diff line 87): !string.IsNullOrEmpty(ContentRoot) guard at call site.
   • HasContentRoot(path, env): unguarded but matches the pre-PR contract (which also threw).
   • MaterializeFrameworkAsset: passes fxDir which is non-empty by construction.

   Remove the inner string.IsNullOrEmpty(path) ? path : … ternary — it just adds noise and a misleading "this method handles empty" cue.

4. Test coverage gaps (diff lines 248–505). The new test file covers NormalizeContentRootPath, Normalize, FromTaskItem, FromV1TaskItem, FromTaskItemGroup, ResolveFile, HasContentRoot against the project-directory scenario — but does not cover:

   • MaterializeFrameworkAsset(..., env) — the most complex migrated method, with the new early-return and 4 changed log statements. No test validates that sourceFile.OriginalValue is what's logged, or that the new early-return fires for empty Identity, or that destPath is absolutized against ProjectDirectory rather than process CWD.
   • ToAssetDictionary(items, env, validate) — public-API parity wrapper, no test.
   • ComputeIntegrity(identity, originalItemSpec, env) — no test.
   • NormalizeContentRootPath(path, env) with path == "" or null — no test that the exception contract is preserved (these are exactly the Sin 6 inputs the playbook flags).
   • ResolveFile(identity, originalItemSpec, env) where identity is non-empty-but-missing-on-disk and originalItemSpec is a relative path resolvable via env.ProjectDirectory — the fallback path through the second FileInfo is uncovered.

   At minimum, please add a MaterializeFrameworkAsset_WithTaskEnvironment_LogsOriginalIdentityWhenSourceMissing test (asserts the log captures asset.Identity, not the absolutized form) and a NormalizeContentRootPath_WithTaskEnvironment_NullOrEmpty_ThrowsArgumentException test (pins the Sin 6 contract).

5. XML doc only on FromTaskItem(ITaskItem, TaskEnvironment, bool) (diff lines 11–16). The other 10 new overloads (FromV1TaskItem, ApplyDefaults, ComputeIntegrity, Normalize, NormalizeContentRootPath, HasContentRoot, MaterializeFrameworkAsset, ResolveFile, ToAssetDictionary, FromTaskItemGroup) carry no doc explaining when to pass env. Sibling SDK targets migrating one at a time will see two indistinguishable overloads in IntelliSense with no guidance. Copy the doc paragraph from FromTaskItem (or factor a <remarks> block referenced via <inheritdoc cref=…> plus a one-line <summary> per method).

💡 Suggestions

6. 11 new public methods doubles the surface of StaticWebAsset (diff lines 17, 32, 51, 70, 84, 102, 115, 132, 197, 216, 232). The matching hook on TaskEnvironment itself is hidden with [EditorBrowsable(Never)] upstream (TaskEnvironment.CreateWithProjectDirectoryAndEnvironment). Consider doing the same on the new TaskEnvironment-bearing overloads here until the SDK has finished migrating all call sites — keeps IntelliSense clean and signals "consume only from migrated targets." Not blocking, but matches the upstream convention.

7. Trailing whitespace + accidental blank line at the destPath rewrite (diff line 141). Diff shows a + (trailing spaces, no code) line followed by a + blank line then the real destPath = … statement. Drop the spurious blank. Cosmetic.

8. Inconsistent ternary spacing in ResolveFile (diff line 200):

   new FileInfo(string.IsNullOrEmpty(identity) ? identity: env.GetAbsolutePath(identity))
   //                                                    ^ missing space before colon

   The line below (diff line 206) has correct spacing. Trivial.

✅ What's done well

• Sin 2 (path inflation) systematically handled: all four new/changed log statements in MaterializeFrameworkAsset use sourceFile.OriginalValue (diff lines 154, 163, 168), matching the playbook's mandate that user-facing messages show the original input.
• Sin 5 (canonicalization) preserved everywhere it mattered: Path.GetFullPath(env.GetAbsolutePath(x)) wrapping retained in Normalize (RelatedAsset), NormalizeContentRootPath, and MaterializeFrameworkAsset — confirmed by the explicit Normalize_WithTaskEnvironment_AbsolutePathInputs_ArePreservedAndCanonicalized and NormalizeContentRootPath_WithTaskEnvironment_PreservesCanonicalization_DotDot tests (diff lines 427–468).
• Back-compat overloads consistently delegate to TaskEnvironment.Fallback with one-liner expression bodies (diff lines 9, 30, 49, 68, 82, 100, 113, 124, 190, 195, 214, 230) — easy to grep, easy to delete when the SDK fully migrates.
• The decoy-CWD test pattern is correctly pinned: WithDecoyCwdAndProjectDirectory mutates Directory.SetCurrentDirectory with try/finally restore, and assembly-wide [assembly:CollectionBehavior(DisableTestParallelization = true)] is referenced in LegacyStaticWebAssetsV1IntegrationTest.cs. The dedicated NormalizeContentRootPath_WithoutEnvOverload_StillUsesProcessCurrentDirectory_ForBackCompat test (diff lines 280–291) is exactly the right Fallback regression pin.
• No Sin 1 (output contamination) regression: Normalize/MaterializeFrameworkAsset write absolutized paths into ContentRoot/RelatedAsset/Identity/OriginalItemSpec, but pre-PR did the same via Path.GetFullPath — properties were always absolutized; the only difference is which base directory is used.
• AbsolutePath→string implicit conversion is allocation-free (AbsolutePath.cs:167 returns the existing Value reference), so the new File.Exists(sourceFile) / File.Copy(sourceFile, …) callsites add no per-call heap pressure beyond what env.GetAbsolutePath already allocates.