Migrate FormatUrl to multithreaded execution model

[jankratochvilcz]

Migrates the FormatUrl task to MSBuild's multithreaded execution model, applying the "absolutize-at-boundary" pattern previously established for other tasks (see #13267).

Fixes #13567.

Non-obvious decisions

Explicit Windows whitespace guard in PathUtil.Resolve. Pre-migration, Path.GetFullPath(" ") threw ArgumentException on Windows. After absolutizing first, Path.Combine(projectDir, " ") produces "projectDir\ " and Path.GetFullPath silently trims the trailing whitespace and returns the project directory — silently losing the contract. To preserve historical behavior, Resolve now does an explicit IsNullOrWhiteSpace check on Windows and throws with a clearer message ("Path cannot be null or whitespace on Windows."). Whitespace remains valid on Unix, matching the original cross-platform behavior. Captured as "Sin 5a" in the multithreaded-task-migration skill so future migrations catch the same trap.

Validation

• Microsoft.Build.Tasks.UnitTests FormatUrl_Tests pass on net10.0 and net472 (2 Unix-only tests skip on Windows).
• Full Microsoft.Build.Tasks.UnitTests suite passes on net10.0 (1184/1184).

[github-actions]

🔍 Skill Validator Results

⚠️ Warnings or advisories found

Scope	Checked
Skills	13
Agents	1
Total	14
Severity	Count
---	---:
❌ Errors	0
⚠️ Warnings	6
ℹ️ Advisories	0

Summary

Level	Finding
ℹ️	Found 13 skill(s)
ℹ️	[assessing-breaking-changes] 📊 assessing-breaking-changes: 992 BPE tokens [chars/4: 1,154] (detailed ✓), 8 sections, 2 code blocks
ℹ️	[merge-dependency-updates] 📊 merge-dependency-updates: 2,030 BPE tokens [chars/4: 1,887] (detailed ✓), 16 sections, 7 code blocks
ℹ️	[reviewing-msbuild-code] 📊 reviewing-msbuild-code: 166 BPE tokens [chars/4: 216] (compact ✓), 1 sections, 0 code blocks
ℹ️	[reviewing-msbuild-code] ⚠ Skill is only 166 BPE tokens (chars/4 estimate: 216) — may be too sparse to provide actionable guidance.
ℹ️	[reviewing-msbuild-code] ⚠ No code blocks — agents perform better with concrete snippets and commands.
ℹ️	[reviewing-msbuild-code] ⚠ No numbered workflow steps — agents follow sequenced procedures more reliably.
ℹ️	[maintaining-binary-log-compatibility] 📊 maintaining-binary-log-compatibility: 1,271 BPE tokens [chars/4: 1,509] (detailed ✓), 12 sections, 2 code blocks
ℹ️	[release] 📊 release: 1,901 BPE tokens [chars/4: 1,889] (detailed ✓), 11 sections, 0 code blocks
ℹ️	[release] ⚠ No code blocks — agents perform better with concrete snippets and commands.

Full validator output

```text Found 13 skill(s) [assessing-breaking-changes] 📊 assessing-breaking-changes: 992 BPE tokens [chars/4: 1,154] (detailed ✓), 8 sections, 2 code blocks [merge-dependency-updates] 📊 merge-dependency-updates: 2,030 BPE tokens [chars/4: 1,887] (detailed ✓), 16 sections, 7 code blocks [reviewing-msbuild-code] 📊 reviewing-msbuild-code: 166 BPE tokens [chars/4: 216] (compact ✓), 1 sections, 0 code blocks [reviewing-msbuild-code] ⚠ Skill is only 166 BPE tokens (chars/4 estimate: 216) — may be too sparse to provide actionable guidance. [reviewing-msbuild-code] ⚠ No code blocks — agents perform better with concrete snippets and commands. [reviewing-msbuild-code] ⚠ No numbered workflow steps — agents follow sequenced procedures more reliably. [maintaining-binary-log-compatibility] 📊 maintaining-binary-log-compatibility: 1,271 BPE tokens [chars/4: 1,509] (detailed ✓), 12 sections, 2 code blocks [release] 📊 release: 1,901 BPE tokens [chars/4: 1,889] (detailed ✓), 11 sections, 0 code blocks [release] ⚠ No code blocks — agents perform better with concrete snippets and commands. [multithreaded-task-migration] 📊 multithreaded-task-migration: 3,719 BPE tokens [chars/4: 4,048] (standard ~), 30 sections, 9 code blocks [multithreaded-task-migration] ⚠ Skill is 3,719 BPE tokens (chars/4 estimate: 4,048) — approaching "comprehensive" range where gains diminish. [use-bootstrap-msbuild] 📊 use-bootstrap-msbuild: 709 BPE tokens [chars/4: 751] (detailed ✓), 14 sections, 5 code blocks [pipelines-health-check] 📊 pipelines-health-check: 5,025 BPE tokens [chars/4: 5,098] (comprehensive ✗), 40 sections, 10 code blocks [pipelines-health-check] ⚠ Skill is 5,025 BPE tokens (chars/4 estimate: 5,098) — "comprehensive" skills hurt performance by 2.9pp on average. Consider splitting into 2–3 focused skills. [authoring-errors-and-warnings] 📊 authoring-errors-and-warnings: 1,287 BPE tokens [chars/4: 1,393] (detailed ✓), 13 sections, 4 code blocks [changewaves] 📊 changewaves: 901 BPE tokens [chars/4: 1,007] (detailed ✓), 9 sections, 1 code blocks [optimizing-msbuild-performance] 📊 optimizing-msbuild-performance: 1,182 BPE tokens [chars/4: 1,342] (detailed ✓), 10 sections, 1 code blocks [integrating-sdk-and-msbuild] 📊 integrating-sdk-and-msbuild: 1,250 BPE tokens [chars/4: 1,440] (detailed ✓), 14 sections, 2 code blocks [deploy-msbuild-to-vs] 📊 deploy-msbuild-to-vs: 2,312 BPE tokens [chars/4: 2,363] (detailed ✓), 24 sections, 12 code blocks ✅ All checks passed (13 skill(s)) Found 1 agent(s) Validated 1 agent(s)

✅ All checks passed (1 agent(s))

</details>

[Copilot]

Pull request overview

This PR migrates the FormatUrl built-in task to MSBuild’s multithreaded task execution model by injecting TaskEnvironment and anchoring relative-path resolution to the project directory (instead of the process current directory), following the “absolutize-at-boundary” migration pattern.

Changes:

• Mark FormatUrl as [MSBuildMultiThreadableTask], implement IMultiThreadableTask, and route relative-path resolution through TaskEnvironment.ProjectDirectory.
• Update PathUtil.Format/PathUtil.Resolve to accept an AbsolutePath baseDirectory and resolve local relative paths via AbsolutePath(...).GetCanonicalForm().
• Refactor and extend FormatUrl unit tests to inject TaskEnvironment and add a project-directory-relative resolution regression test.

Reviewed changes

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

File	Description
src/Tasks/ManifestUtil/PathUtil.cs	Threads a base directory through path formatting/resolution and anchors local relative paths to the project directory.
src/Tasks/FormatUrl.cs	Migrates FormatUrl to multithreaded task model via attribute + IMultiThreadableTask and uses TaskEnvironment.ProjectDirectory.
src/Tasks.UnitTests/FormatUrl_Tests.cs	Updates tests to provide TaskEnvironment and adds coverage for project-directory-relative resolution.
.github/skills/multithreaded-task-migration/SKILL.md	Updates migration guidance around TaskEnvironment and test setup patterns.

[github-actions]

Expert Review: 24-Dimension Analysis of PR #13573 (Migrate FormatUrl to Multithreaded Execution)

🔴 BLOCKING Issues

Dimension 1: Backwards Compatibility — LGTM
PathUtil is internal static class, so the signature change from Format(string) to Format(string, AbsolutePath) has no public API impact. FormatUrl is the only caller. The ArgumentException on Windows whitespace-only input preserves the historical Path.GetFullPath(" ") behavior. No new warnings are emitted.

Dimension 2: ChangeWave Discipline — LGTM
The behavioral change (resolving against ProjectDirectory instead of Environment.CurrentDirectory) only manifests in multithreaded mode, which is itself an opt-in feature. No ChangeWave needed — this is part of the multithreaded execution model, not a behavior change in the default mode.

Dimension 13: Concurrency & Thread Safety — LGTM
FormatUrl has no shared mutable state. TaskEnvironment.ProjectDirectory is set once by the engine before Execute(). PathUtil methods are stateless. No concurrency concerns.

Dimension 21: Evaluation Model Integrity — N/A (task-only change)

Dimension 24: Security Awareness — LGTM
No security regression. Path resolution is now anchored to ProjectDirectory rather than process cwd, which is actually more secure in multi-tenant scenarios.

---

🟡 Issues Found (see inline comments)

Dimension 22: Correctness & Edge Cases — ISSUE (BLOCKING)
FormatUrl.TaskEnvironment lacks = TaskEnvironment.Fallback; default initializer. All 19 other migrated tasks have it. Direct instantiation without setting TaskEnvironment → NullReferenceException. See inline comment on FormatUrl.cs:21.

Dimension 5: Error Message Quality — ISSUE (MODERATE)
Hardcoded English string "Path cannot be null or whitespace on Windows." in PathUtil.Resolve. Should be in .resx per MSBuild conventions. Mitigated by the fact that old code also threw a BCL-provided English message. See inline comment on PathUtil.cs:251.

---

✅ Clean Dimensions

#	Dimension	Verdict
3	Performance & Allocation	LGTM — AbsolutePath is a readonly struct, no unnecessary allocations. PathUtil is not a hot path.
4	Test Coverage	LGTM — Null, empty, whitespace (per-platform), UNC, URL, localhost, absolute, relative, and the new project-directory-relative test. Excellent coverage.
6	Logging & Diagnostics	LGTM — No logging changes needed for this simple task.
7	String Comparison	LGTM — Existing StringComparison.OrdinalIgnoreCase for localhost comparison is correct (network hostnames).
8	API Surface	LGTM — FormatUrl.TaskEnvironment is public but it implements the IMultiThreadableTask interface which requires it. PathUtil is internal. No PublicAPI.txt tracking needed (checked — this project doesn't use them).
9	Target Authoring	N/A — no .targets/.props changes.
10	Design & Intent	LGTM — "Absolutize-at-boundary" pattern correctly applied; PathUtil.Format/Resolve gain the base directory parameter, which is pattern #2 from the SKILL.md.
11	Cross-Platform	LGTM — Windows whitespace guard with Unix pass-through preserves historical cross-platform semantics. #if NET / #if !NET guards preserved.
12	Code Simplification	LGTM — Clean, minimal change.
14	Naming	LGTM — baseDirectory parameter name is clear. GetTarget() test helper is descriptive.
15	SDK Integration	N/A
16	Idiomatic C#	LGTM — file has #nullable disable and the PR correctly does not add nullable annotations.
17	File I/O & Path Handling	LGTM — Correctly uses AbsolutePath + GetCanonicalForm() instead of raw Path.GetFullPath.
18	Documentation	LGTM — SKILL.md updates are valuable: Sin 7 (cwd buried in helpers), migration pattern ranking, pure-string utility callouts. Comments in PathUtil explain the whitespace guard rationale well.
19	Build Infrastructure	LGTM
20	Scope & PR Discipline	LGTM — Single concern (FormatUrl migration) + related SKILL.md documentation.
23	Dependency Management	LGTM — No new dependencies.

---

Summary

This is a well-structured migration following the established "absolutize-at-boundary" pattern. The SKILL.md documentation additions (Sin 7, migration patterns, utility callouts) are excellent contributions.

One blocking fix required: Add = TaskEnvironment.Fallback default to FormatUrl.TaskEnvironment to match the pattern used by all other migrated tasks and prevent NRE on direct instantiation.

Generated by Expert Code Review (on open) for issue #13573 · ● 13.5M

[ViktorHofer]

Dimension 22: Correctness & Edge Cases — ISSUE (BLOCKING)
FormatUrl.TaskEnvironment lacks = TaskEnvironment.Fallback; default initializer. All 19 other migrated tasks have it. Direct instantiation without setting TaskEnvironment → NullReferenceException. See inline comment on FormatUrl.cs:21.

Good find from our bot :)

[JanProvaznik]

Suggested change

	if (NativeMethods.IsWindows && path.Length > 0 && path.Trim().Length == 0)
	if (NativeMethods.IsWindows && string.IsNullOrWhiteSpace(path))

https://learn.microsoft.com/en-us/dotnet/api/system.string.isnullorwhitespace?view=net-10.0

[jankratochvilcz]

@JanProvaznik this is intentional tho' as I tried to explicitly target whitespace and exclude null from the condition :-)