Skip to content

Make MSBuild tasks safe for multi-threaded execution - #17381

Open
ViktorHofer wants to merge 9 commits into
dotnet:mainfrom
ViktorHofer:mt-task-migration
Open

Make MSBuild tasks safe for multi-threaded execution#17381
ViktorHofer wants to merge 9 commits into
dotnet:mainfrom
ViktorHofer:mt-task-migration

Conversation

@ViktorHofer

@ViktorHofer ViktorHofer commented Aug 21, 2026

Copy link
Copy Markdown
Member

Why

MSBuild 18.x can run tasks on multiple threads inside a single node (-mt), but only for tasks that opt in via [MSBuildMultiThreadableTask]. Everything else is routed to an out-of-proc sidecar TaskHost, which is much more expensive than the in-proc path.

Measured on a runtime inner-repo build in the VMR-mt -nodeReuse:true -ms against an otherwise identical baseline, with no Arcade task annotated:

baseline -mt + server + node reuse
Wall clock 3090.2 s 2978.2 s
TaskHost task invocations 849 14,125
Total task time (non-nesting) 4,026 s 4,124 s
Effective parallelism 4.41x 4.09x

Only a 3.6% wall-clock gain: task time went up by 98 s and effective parallelism went down, cancelling most of the benefit of threading. Arcade tasks run in essentially every project in every repo, so they are the single biggest lever here.

Those numbers are the unmigrated state. Across a full VMR build, where every repo pays this cost on every project, the expected saving from this change is in the 5-15 minute range.

What

Annotates 104 of the 121 tasks that Arcade registers via UsingTask and that resolve to source in this repo, and makes the supporting code multi-threading safe.

  • Update MSBuild dependencies 17.12.50 -> 18.8.2. First version exposing both [MSBuildMultiThreadableTask]/IMultiThreadableTask and ToolTask.TaskEnvironment. The routing check is guarded by BuildParameters.MultiThreaded, so the attribute is inert on older hosts.
  • Reference Microsoft.Build.TaskAuthoring.Analyzer from eng/BuildTask.targets, scoped by eng/MultiThreadableTaskAnalyzer.globalconfig to tasks that have already opted in (msbuild_task_analyzer.scope = multithreadable_only). That keeps it as a regression guard without drowning the build in diagnostics. The API-shape suggestions (MSBuildTask0006-0008, 0011) are off: several would be binary-breaking for task parameters set from targets across the ecosystem.
  • Replace ambient process state with the injected TaskEnvironment. Relative paths resolve against the project directory instead of the process-wide current directory, environment variables are read per-project, and child processes start from TaskEnvironment.GetProcessStartInfo().
  • Thread AbsolutePath through the shared path helpers in Packaging, GenFacades, Feed, NuGetRepack, SharedFramework.Sdk, PackageTesting and XliffTasks. Retyping a helper's path parameter is what actually resolves the transitive MSBuildTask0005 chains.
  • Delete the internal BuildTask/ILog base class. It only wrapped Microsoft.Build.Utilities.Task and TaskLoggingHelper, and standing between tasks and the MSBuild base class meant every derived task needed its own TaskEnvironment plumbing anyway. This is the bulk of the line count.
  • Delete the unused LaunchDebugger and UploadToAzure tasks. Neither has a UsingTask registration or a reference anywhere in the repo or the wider dotnet org, so both were unreachable, and both would otherwise have needed a permanent exclusion. Debugger.Launch() attaches to the whole process, so a shared multithreaded node would attach for every project building on it. UploadToAzure's CancellationTokenSource is static, so Cancel() on one instance cancelled the token handed to every other concurrent and future instance — a real bug that goes away with the file. Uploads already go through AzureStorageAssetPublisher and BlobFeedAction; the shared base AzureConnectionStringBuildTask and the AzureStorageUtils helpers stay in use.

Concurrency bugs found and fixed

The analyzer checks path/env/process API usage, not thread-safety of shared mutable state, so a clean analyzer run is not evidence a task is thread-safe. These needed a manual sweep, and all are latent bugs today under any concurrent access — not just under -mt:

Site Defect
TargetFrameworkResolver Shared resolver; NuGet's non-concurrent internal caches threw out of FindBestItemGroup under concurrent GetNearest. Hit ~15 runtime projects at once.
GetPackageDescription Unsynchronized static Dictionary; additionally memoized the null returned after a logged IOException, so every later invocation on the node silently got null without retrying.
GetCompatiblePackageTargetFrameworks static List/Dictionary mutated from Execute; also accumulated duplicates across invocations on a reused node.
PackageIndex Shared static Newtonsoft JsonSerializer, which is not thread-safe.
FindDotNetCliPackage static HttpClient overwritten by every execution; a concurrent instance could issue requests through another's disposed handler.
AkaMSLinksBase File.ReadAllText(ClientCertificate) on a raw input path, in the shared base of two annotated tasks.
AzureDevOpsTask static Random for retry jitter -> Random.Shared.

Two regressions the migration itself introduced, both fixed and worth flagging to anyone doing this work:

  • GetAbsolutePath(null) throws, while File.Exists(null)/Directory.Exists(null) return false. Mechanically wrapping those two calls turns an optional, unset task property into a hard failure — it took out every source-build leg. They are the only two BCL path APIs with that tolerance, so the regression surface is exactly those; all 292 added call sites were cross-referenced against [Required] and 6 fixed. Note this surfaced on legs running without -mt: TaskEnvironment is injected in normal builds too.
  • Five tasks carried the attribute without implementing IMultiThreadableTask (FinalizeInsertionVsixFile, ReadNuGetPackageInfos, ChooseBestTargetFrameworksTask, ChooseBestP2PTargetFrameworkTask, GetCompatiblePackageTargetFrameworks), so TaskEnvironment was never injected and every resolution silently used Fallback. The two signals are independent — the attribute controls routing, the interface controls injection — and nothing fails loudly when only one is present. The interface cannot be made the routing signal, since ToolTask implements it and that would opt in every ToolTask subclass in the ecosystem.

A late review pass found 11 tasks passing a raw path into a helper that opens it with plain File/Directory/NuGet APIs, below the analyzer's visibility. The properties involved ($(RuntimeIdentifierGraphPath), $(RuntimeIdGraphDefinitionFile), ApkPath, AppBundlePath, BaseDirectory, PackagePaths) are all user-settable — RuntimeIdGraphDefinitionFile is even declared with Condition="'$(RuntimeIdGraphDefinitionFile)' == ''" — so their absolute defaults are not something a task may rely on. All now resolve through TaskEnvironment. In ChooseBest* this also fixed the TargetFrameworkResolver cache, which was keyed on the raw path string and so could hand two projects with different working directories the same resolver built from a different file.

A follow-up sweep for the same shape also removed the attribute from WixToolTask and HarvesterToolTask. Neither is UsingTask-registered — MsiBase constructs them directly — so MSBuild never routes them and never injects a TaskEnvironment, leaving WixToolTaskBase on TaskEnvironment.Fallback. The annotation was inert but marked part of the CreateVisualStudioWorkload chain as migrated, contradicting the TODO comments on both entry points. Registered-task counts are unaffected.

Deliberately not migrated

18 tasks are left unannotated: the 17 registered ones plus CreateAzureContainerIfNotExists, which is not UsingTask-registered but is called through BlobFeedAction. Every one carries a comment at its declaration recording why, so the decision is not silently reverted.

All are P3 with no measured invocations in the hot-path profile — every task appearing in a runtime inner-repo binlog (7,143 invocations, ~82 s of TaskHost overhead in the measurement above) is already migrated, so annotating the rest cannot save build time. They are also the tasks where a mistake is most expensive and least testable: publishing to Maestro and blob storage, code signing, Wix packaging. No PR build exercises them, so a regression would surface in a release pipeline instead of CI.

Ambient credentials or pipeline environment (8). Build an AzureCliCredential when no PAT/AccountKey is supplied, or read pipeline variables straight from the process environment: CreateAzureDevOpsFeed, CreateNewAzureContainer, CreateAzureContainerIfNotExists, PublishArtifactsInManifest, PublishBuildToMaestro, PublishSignedAssets, SendHelixJob (JobDefinition reads BUILD_REPOSITORY_NAME, BUILD_SOURCEBRANCH, SYSTEM_TEAMPROJECT, BUILD_REASON), InstallDotNetTool (via ICommandFactory).

Process-wide static state (4). SignCheckTask (static _fileVerifiers in SignatureVerificationManager), GenPartialFacadeSource and NotSupportedAssemblyGenerator (RoslynBuildTask subscribes an instance handler to the process-wide AssemblyLoadContext.Resolving, so A's resolution can be serviced by B's handler — precisely the cross-location Roslyn load the method's own comment exists to prevent, and a latent bug today), SingleError (GetRegisteredTaskObject followed by RegisterTaskObject is not atomic).

Unresolved paths flowing into helper chains (6). GenAPITask (HostEnvironment expands variables and probes with Directory.Exists/File.Exists on raw input), PushToBuildStorage (six *LocalStorageDir inputs plus artifact items that would all have to migrate together), SignToolTask, CreateLightCommandPackageDrop, CreateVisualStudioWorkload, CreateVisualStudioWorkloadSet (helper chains spanning 29 and 62 files that still resolve against the process-wide current directory; MSBuildTask0005 is suppressed at those entry points).

Several of these keep their IMultiThreadableTask implementation and TaskEnvironment-based path handling. That is deliberate and is not a half-migration: routing is decided by the attribute alone, so the interface only causes TaskEnvironment to be injected, which makes path resolution correct in either mode. The interface also cannot ever become a routing signal — ToolTask itself implements it, so that would opt in every ToolTask-derived task in the ecosystem. Removing it from a not-yet-safe task would revert that task's paths to the process current directory while leaving it exactly as unsafe.

Path.GetTempPath() is suppressed at four sites where the temp root is only ever the parent of a freshly generated GUID/random name and is therefore never shared between concurrent tasks. TaskEnvironment has no temp-directory member today.

Validation

  • dotnet build Arcade.slnx -c Release --no-incremental with CI defaults (TreatWarningsAsErrors on): 0 errors, 0 warnings, both in this repo and in the VMR.
    • Reviewers: incremental builds skip up-to-date projects, so the analyzer does not re-run and reports a false "0 warnings". --no-incremental is required for an authoritative count. Analyzer liveness was separately confirmed by injecting a deliberate Environment.GetEnvironmentVariable call into an annotated task and checking it was reported as MSBuildTask0002.
  • Full test suite: SignTool (196), Feed (214), Packaging (48), XliffTasks (42), Helix.Sdk (236), Installers (13), NuGetRepack (5), PackageTesting (9), Templating (11) all pass.
    • Pre-existing/environmental failures unrelated to this change: CentralPackageManagementTests.ImplicitPackageReferences_ShouldNotConflictWithPackageVersionEntries (NU1009 on MicroBuild.Plugins.SwixBuild, untouched here), two Workloads.Tests needing a full VS MSBuild.exe, and Arcade.Validation.Tests.RepoTests which time out building test repos locally.

Contributes to #17378; the remaining tasks are tracked in #17388.

Filed against MSBuild while doing this work: #14779 and #14790 (TaskRouter documentation contradicts the attribute-only implementation), #14780 (MSBuildTask0003 code fix produces non-compiling code), and #14783/#14784/#14785 (analyzer gaps this migration surfaced — most importantly that it does not walk into base types).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR prepares Arcade MSBuild tasks for in-process multithreaded execution through task annotations, project-scoped environment handling, analyzer enforcement, and concurrency fixes.

Changes:

  • Updates MSBuild dependencies and adds analyzer guardrails.
  • Migrates tasks to TaskEnvironment and removes the obsolete shared task base.
  • Addresses shared caches, serializers, and other concurrency hazards while leaving unsafe task chains unannotated.
Show a summary per file
File Review result
src/Microsoft.DotNet.XliffTasks/Tasks/XlfTask.cs Reviewed; no final comment.
src/Microsoft.DotNet.XliffTasks/Tasks/UpdateXlf.cs Moderate: exception handling still compares against a potentially relative path.
src/Microsoft.DotNet.XliffTasks/Tasks/TranslateSource.cs Reviewed; no final comment.
src/Microsoft.DotNet.XliffTasks/Tasks/TransformTemplates.cs Reviewed; no final comment.
src/Microsoft.DotNet.XliffTasks/Tasks/SortXlf.cs Reviewed; no final comment.
src/Microsoft.DotNet.XliffTasks/Tasks/GatherXlf.cs Reviewed; no final comment.
src/Microsoft.DotNet.XliffTasks/Tasks/GatherTranslatedSource.cs Reviewed; no final comment.
src/Microsoft.DotNet.XliffTasks/Tasks/EnsureAllResourcesTranslated.cs Reviewed; no final comment.
src/Microsoft.DotNet.XliffTasks/Model/Document.cs Reviewed; no final comment.
src/Microsoft.DotNet.SwaggerGenerator/Microsoft.DotNet.SwaggerGenerator.MSBuild/GenerateSwaggerCode.cs Reviewed; no final comment.
src/Microsoft.DotNet.SourceBuild/tasks/src/WriteBuildOutputProps.cs Moderate: relative asset directories are checked against the process directory.
src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WriteUsageReports.cs Reviewed; no final comment.
src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WritePackageUsageData.cs Moderate: RootDir is not normalized consistently.
src/Microsoft.DotNet.SourceBuild/tasks/src/ReadNuGetPackageInfos.cs Reviewed; no final comment.
src/Microsoft.DotNet.SignTool/src/SignToolTask.cs Reviewed; no final comment.
src/Microsoft.DotNet.SignCheckTask/SignCheckTask.cs Critical: the shared verifier registry is not safe for concurrent or repeated construction.
src/Microsoft.DotNet.SharedFramework.Sdk/src/ValidateFileVersions.cs Reviewed; no final comment.
src/Microsoft.DotNet.SharedFramework.Sdk/src/GenerateSharedFrameworkDepsFile.cs Reviewed; no final comment.
src/Microsoft.DotNet.SharedFramework.Sdk/src/GeneratePlatformManifestEntriesFromTemplate.cs Reviewed; no final comment.
src/Microsoft.DotNet.SharedFramework.Sdk/src/GeneratePlatformManifestEntriesFromFileList.cs Reviewed; no final comment.
src/Microsoft.DotNet.SharedFramework.Sdk/src/FileUtilities.cs Reviewed; no final comment.
src/Microsoft.DotNet.SharedFramework.Sdk/src/CreateFrameworkListFile.cs Reviewed; no final comment.
src/Microsoft.DotNet.SharedFramework.Sdk/Microsoft.DotNet.SharedFramework.Sdk.csproj Reviewed; no final comment.
src/Microsoft.DotNet.PackageTesting/VerifyTypes.cs Reviewed; no final comment.
src/Microsoft.DotNet.PackageTesting/VerifyClosure.cs Reviewed; no final comment.
src/Microsoft.DotNet.PackageTesting/Microsoft.DotNet.PackageTesting.csproj Reviewed; no final comment.
src/Microsoft.DotNet.PackageTesting/GetCompatiblePackageTargetFrameworks.cs Critical: static mutable framework collections are shared across executions.
src/Microsoft.DotNet.NuGetRepack/tests/VersionUpdaterTests.cs Reviewed; no final comment.
src/Microsoft.DotNet.NuGetRepack/tasks/src/UpdatePackageVersionTask.cs Reviewed; no final comment.
src/Microsoft.DotNet.NuGetRepack/tasks/src/ReplacePackageParts.cs Reviewed; no final comment.
src/Microsoft.DotNet.NuGetRepack/tasks/src/NuGetVersionUpdater.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/WaitForHelixJobCompletion.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/StopAzurePipelinesTestRun.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/StartAzurePipelinesTestRun.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/InstallDotNetTool.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/GetHelixWorkItems.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/FindDotNetCliPackage.cs Critical: the process-wide client can be overwritten or disposed during concurrent execution.
src/Microsoft.DotNet.Helix/Sdk/DownloadFromResultsContainer.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/CreateXUnitWorkItems.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAppleWorkItems.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAndroidWorkItems.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/CreateMTPWorkItems.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/CreateFailedTestsForFailedWorkItems.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/CheckHelixJobStatus.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/CheckAzurePipelinesTestResults.cs Reviewed; no final comment.
src/Microsoft.DotNet.Helix/Sdk/CancelHelixJob.cs Reviewed; no final comment.
src/Microsoft.DotNet.GenFacades/TypeParser.cs Reviewed; no final comment.
src/Microsoft.DotNet.GenFacades/SourceGenerator.cs Reviewed; no final comment.
src/Microsoft.DotNet.GenFacades/RoslynBuildTask.cs Reviewed; no final comment.
src/Microsoft.DotNet.GenFacades/NotSupportedAssemblyGenerator.cs Critical: the shared process-wide assembly resolver is not thread-safe.
src/Microsoft.DotNet.GenFacades/Microsoft.DotNet.GenFacades.csproj Reviewed; no final comment.
src/Microsoft.DotNet.GenFacades/GenPartialFacadeSourceGenerator.cs Reviewed; no final comment.
src/Microsoft.DotNet.GenFacades/GenPartialFacadeSource.cs Critical: the shared process-wide assembly resolver is not thread-safe.
src/Microsoft.DotNet.GenFacades/ClearAssemblyReferenceVersions.cs Reviewed; no final comment.
src/Microsoft.DotNet.GenAPI/Microsoft.DotNet.GenAPI.csproj Reviewed; no final comment.
src/Microsoft.DotNet.GenAPI/GenAPITask.cs Reviewed; no final comment.
src/Microsoft.DotNet.Deployment.Tasks.Links/src/DeleteAkaMSLinks.cs Reviewed; no final comment.
src/Microsoft.DotNet.Deployment.Tasks.Links/src/CreateAkaMSLinks.cs Reviewed; no final comment.
src/Microsoft.DotNet.CMake.Sdk/src/CreateCMakeFileApiQuery.cs Reviewed; no final comment.
src/Microsoft.DotNet.CMake.Sdk/Microsoft.DotNet.CMake.Sdk.csproj Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Workloads/src/Wix/WixToolTaskBase.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Workloads/src/Wix/WixToolTask.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Workloads/src/Wix/HarvesterToolTask.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Workloads/src/Microsoft.DotNet.Build.Tasks.Workloads.csproj Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Workloads/src/CreateVisualStudioWorkloadSet.wix.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Workloads/src/CreateVisualStudioWorkload.wix.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.VisualStudio/Vsix/GetPkgDefAssemblyDependencyGuid.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.VisualStudio/Vsix/FinalizeInsertionVsixFile.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.VisualStudio/OptProf/GetRunSettingsSessionConfiguration.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.VisualStudio/OptProf/GenerateTrainingPropsFile.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.VisualStudio/OptProf/GenerateTrainingInputFiles.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.VisualStudio/OptProf/FindLatestDrop.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Templating/src/GenerateFileFromTemplate.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.TargetFramework/src/TargetFrameworkResolver.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.TargetFramework/src/Microsoft.DotNet.Build.Tasks.TargetFramework.csproj Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.TargetFramework/src/ChooseBestTargetFrameworksTask.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.TargetFramework/src/ChooseBestP2PTargetFrameworkTask.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/ValidateHarvestVersionIsLatestForReleaseTests.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/TestBuildEngine.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/RuntimeGraphTests.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/PackageIndexTests.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/Log.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/tests/CreateTrimDependencyGroupsTests.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/VersionUtility.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidationTask.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidatePackage.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidateHarvestVersionIsLatestForRelease.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidateFrameworkPackage.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/UpdatePackageIndex.cs Critical: mutable cached PackageIndex instances are shared across executions.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/SplitReferences.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/SplitDependenciesBySupport.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/PromoteDependencies.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/PackageReport.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/PackageMetadata.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/PackageItem.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/PackageIndex.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetUtility.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetPack.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetAssetResolver.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/Microsoft.DotNet.Build.Tasks.Packaging.csproj Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/HarvestPackage.cs Moderate: relative package-folder offsets can produce invalid asset paths.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetSupportedPackagesFromPackageReports.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetRuntimeTargets.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetRuntimeJsonValues.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetPackageVersion.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetPackageFromModule.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetPackageDestination.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetPackageDescription.cs Critical: the static description cache is not synchronized.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetMinimumNETStandard.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetLayoutFiles.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetLastStablePackage.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetInboxFrameworks.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetAssemblyReferences.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetApplicableAssetsFromPackages.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetApplicableAssetsFromPackageReports.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GenerateRuntimeDependencies.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GeneratePackageReport.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/GenerateNuSpec.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/FilterUnknownPackages.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/CreateTrimDependencyGroups.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ApplyPreReleaseSuffix.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ApplyMetaPackages.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Packaging/src/ApplyBaseLine.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/StabilizeWixFileId.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMsiVersion.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateMacOSDistributionFile.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateGuidFromName.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/GenerateCurrentVersion.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/ExecWithRetries.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateWixCommandPackageDropBase.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateRpmPackage.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateMD5SumsFile.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateLightCommandPackageDrop.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateDebPackage.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateControlFile.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateChangelogFile.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Installers/Microsoft.DotNet.Build.Tasks.Installers.csproj Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.FileCatalog/GenerateFileCatalog.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.FileCatalog/CatalogEntry.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.FileCatalog/CatalogBuilder.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.FileCatalog.Tests/CatalogTests.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/PushToBuildStorage.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishSignedAssets.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishBuildToMaestro.cs Critical: the environment proxy still reads process-global environment state.
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestV4.cs Critical: shared credential detection still reads process-global environment state.
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestV3.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifest.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/LaunchDebugger.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/CreateAzureDevOpsFeed.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/ConfigureInputFeed.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/common/UploadToAzure.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/common/CreateNewAzureContainer.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/common/CreateAzureContainerIfNotExists.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/common/AzureStorageUtils.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/BlobFeedAction.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/AzureStorageExtensions.cs Reviewed; no final comment.
src/Microsoft.DotNet.Build.Tasks.Feed/src/AzureStorageAssetPublisher.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/ValidateLicense.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/Unsign.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/SingleError.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/SetCorFlags.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/SaveItems.cs Moderate: the saved output still uses the original relative path.
src/Microsoft.DotNet.Arcade.Sdk/src/LocateDotNet.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/InstallDotNetCore.cs Moderate: project loading and process start still use original relative paths.
src/Microsoft.DotNet.Arcade.Sdk/src/GroupItemsBy.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/GetLicenseFilePath.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/GetAssemblyFullName.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/GenerateSourcePackageSourceLinkTargetsFile.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/GenerateResxSource.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/GenerateChecksums.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/ExtractNgenMethodList.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/DownloadFile.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/CompareVersions.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/CheckRequiredDotNetVersion.cs Reviewed; no final comment.
src/Microsoft.DotNet.Arcade.Sdk/src/CalculateAssemblyAndFileVersions.cs Reviewed; no final comment.
src/Common/Internal/BuildTask.cs Reviewed; no final comment.
eng/Version.Details.xml Reviewed; no final comment.
eng/Version.Details.props Reviewed; no final comment.
eng/MultiThreadableTaskAnalyzer.globalconfig Reviewed; no final comment.
eng/BuildTask.targets Reviewed; no final comment.
Directory.Packages.props Reviewed; no final comment.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Suppressed comments (16)

src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateLightCommandPackageDrop.cs:10

  • The derived task is annotated, but CreateWixCommandPackageDropBase still performs its core Directory/File operations with raw packageDropOutputFolder, OutputFolder, WixSrcFiles, and Loc paths. Relative inputs therefore continue to use the process current directory; the single resolved WixProjectFile copy here does not make the inherited execution safe. Thread TaskEnvironment through the base chain or leave this task unannotated.
    src/Microsoft.DotNet.Build.Tasks.Installers/src/ExecWithRetries.cs:17
  • The outer task is annotated, but each inner Microsoft.Build.Tasks.Exec is created without propagating the outer TaskEnvironment. Its working directory and child-process environment consequently fall back to process-global state, so relative commands/working directories can resolve incorrectly in a multi-threaded build. Add and assign the child task's TaskEnvironment, or do not opt this wrapper in.
    src/Microsoft.DotNet.Build.Tasks.Packaging/src/GeneratePackageReport.cs:24
  • The new environment plumbing does not reach RuntimeFile: both resolver constructors later pass the raw string to JsonRuntimeFormat.ReadRuntimeGraph. A relative runtime.json consequently remains process-current-directory dependent. Resolve RuntimeFile before constructing either resolver.
    src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetApplicableAssetsFromPackages.cs:24
  • RuntimeFile is still passed unmodified to AggregateNuGetAssetResolver, whose constructor reads the file directly. Relative values therefore remain dependent on the process current directory despite this task being annotated; pass TaskEnvironment.GetAbsolutePath(RuntimeFile) instead.
    src/Microsoft.DotNet.Build.Tasks.Packaging/src/HarvestPackage.cs:20
  • RuntimeFile is still passed raw to both NuGet resolver constructors, which read it directly. Relative runtime graph paths therefore bypass the newly injected project directory and can load/fail against the process current directory. Resolve this input before constructing the resolvers.
    src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetPack.cs:17
  • This task is now marked multithreadable, but BaseDirectory is passed unchanged to PackageBuilder.PopulateFiles. If a caller supplies a relative base directory, NuGet resolves the package files against the process current directory rather than TaskEnvironment.ProjectDirectory, so -mt can pack the wrong files. Resolve BaseDirectory through TaskEnvironment before passing it to the NuGet API.
    src/Microsoft.DotNet.Deployment.Tasks.Links/src/CreateAkaMSLinks.cs:12
  • The shared AkaMSLinksBase helper reads ClientCertificate with File.ReadAllText directly. For a relative certificate path, this annotated task still resolves against the process current directory rather than the project directory supplied by TaskEnvironment, so concurrent projects can read the wrong file or fail. Add TaskEnvironment plumbing and resolve the certificate path before enabling the annotation.
    src/Microsoft.DotNet.Deployment.Tasks.Links/src/DeleteAkaMSLinks.cs:11
  • The shared AkaMSLinksBase helper reads ClientCertificate with File.ReadAllText directly. For a relative certificate path, this annotated task still resolves against the process current directory rather than the project directory supplied by TaskEnvironment, so concurrent projects can read the wrong file or fail. Add TaskEnvironment plumbing and resolve the certificate path before enabling the annotation.
    src/Microsoft.DotNet.Helix/Sdk/CheckAzurePipelinesTestResults.cs:16
  • This task now runs concurrently with other AzureDevOpsTask instances, but the base class uses one static Random for GetRetryDelay. Random instances are not thread-safe, so concurrent retries race on the generator and can produce corrupted/non-random backoff values, defeating retry staggering. Use Random.Shared or an instance-local generator before opting these tasks into multithreaded execution.
    src/Microsoft.DotNet.Helix/Sdk/CheckAzurePipelinesTestResults.cs:16
  • This annotation also opts the AzureDevOpsTask base implementation into in-process multi-threading, but that base reads BUILD_*, SYSTEM_*, and the access token via Environment.GetEnvironmentVariable. Those values are process-global rather than the injected per-project task environment, so concurrent projects can observe the wrong build credentials/metadata. Thread TaskEnvironment through the base or leave these derived tasks TaskHost-routed.
    src/Microsoft.DotNet.Helix/Sdk/CreateFailedTestsForFailedWorkItems.cs:15
  • This task now runs concurrently with other AzureDevOpsTask instances, but the base class uses one static Random for GetRetryDelay. Random instances are not thread-safe, so concurrent retries race on the generator and can produce corrupted/non-random backoff values, defeating retry staggering. Use Random.Shared or an instance-local generator before opting these tasks into multithreaded execution.
    src/Microsoft.DotNet.Helix/Sdk/InstallDotNetTool.cs:18
  • This task is annotated while inheriting MSBuildTaskBase, which has no TaskEnvironment; DestinationPath, ToolPath, and WorkingDirectory are passed to FileSystem/Command unchanged. Relative paths and the command's inherited environment therefore still depend on the process current directory/environment. Resolve these inputs and propagate the task environment through the helper interfaces, or keep the task out of in-process multi-threading.
    src/Microsoft.DotNet.Helix/Sdk/StartAzurePipelinesTestRun.cs:13
  • This task now runs concurrently with other AzureDevOpsTask instances, but the base class uses one static Random for GetRetryDelay. Random instances are not thread-safe, so concurrent retries race on the generator and can produce corrupted/non-random backoff values, defeating retry staggering. Use Random.Shared or an instance-local generator before opting these tasks into multithreaded execution.
    src/Microsoft.DotNet.Helix/Sdk/StopAzurePipelinesTestRun.cs:13
  • This task now runs concurrently with other AzureDevOpsTask instances, but the base class uses one static Random for GetRetryDelay. Random instances are not thread-safe, so concurrent retries race on the generator and can produce corrupted/non-random backoff values, defeating retry staggering. Use Random.Shared or an instance-local generator before opting these tasks into multithreaded execution.
    src/Microsoft.DotNet.SourceBuild/tasks/src/ReadNuGetPackageInfos.cs:13
  • PackagePaths is a plain string array, and ReadIdentity passes each value directly to PackageArchiveReader. Relative package paths therefore still resolve against the process current directory despite this task being annotated. Inject TaskEnvironment and resolve the paths before opening them (including callers of the static helper).
    src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WritePackageUsageData.cs:24
  • The four package-file arrays are still passed directly to ReadNuGetPackageInfos.ReadIdentity, which opens each string as-is. Relative nupkg paths therefore use the process current directory even though this task now has a per-project TaskEnvironment; pass resolved paths into the helper.
  • Files reviewed: 184/184 changed files
  • Comments generated: 17
  • Review effort level: Lite

Comment thread src/Microsoft.DotNet.Arcade.Sdk/src/InstallDotNetCore.cs Outdated
Comment thread src/Microsoft.DotNet.Arcade.Sdk/src/InstallDotNetCore.cs Outdated
Comment thread src/Microsoft.DotNet.Arcade.Sdk/src/SaveItems.cs
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestV4.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishBuildToMaestro.cs
Comment thread src/Microsoft.DotNet.SignCheckTask/SignCheckTask.cs Outdated
Comment thread src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WritePackageUsageData.cs Outdated
Comment thread src/Microsoft.DotNet.SourceBuild/tasks/src/WriteBuildOutputProps.cs Outdated
Comment thread src/Microsoft.DotNet.XliffTasks/Tasks/UpdateXlf.cs Outdated
Copilot AI review requested due to automatic review settings August 21, 2026 20:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (17)

src/Microsoft.DotNet.Arcade.Sdk/src/SaveItems.cs:53

  • The directory is now resolved through TaskEnvironment, but project.Save(File) below still passes the raw path. With a relative File in an in-process multithreaded build, the directory can be created under the project directory while the generated project is written relative to the process current directory. Save using the same environment-resolved path.
                Directory.CreateDirectory(TaskEnvironment.GetAbsolutePath(path));

src/Microsoft.DotNet.Arcade.Sdk/src/SingleError.cs:9

  • The get-then-register sequence is not atomic. Two concurrently scheduled SingleError instances with the same text can both observe no sentinel and both log an error, defeating the task's single-error contract. Synchronize the check/registration or use an atomic build-scoped gate before opting this task into multithreaded execution.
    [MSBuildMultiThreadableTask]

src/Microsoft.DotNet.Build.Tasks.Feed/src/LaunchDebugger.cs:11

  • Debugger.Launch() depends on process-wide debugger state and can prompt or attach the debugger for the entire MSBuild process. That is not safe for a task opted into in-process multithreaded execution; remove the multithreadable annotation from this diagnostic-only task.
    [MSBuildMultiThreadableTask]
    public class LaunchDebugger : Microsoft.Build.Utilities.Task

src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifest.cs:28

  • The concrete task is opted into multithreading, but its inherited PublishArtifactsInManifestBase still calls Environment.GetEnvironmentVariable directly when deciding whether Entra credentials are available. The injected TaskEnvironment on this class is therefore not used for that decision, so concurrent project executions still depend on process-global environment state. Move the check into the environment-aware plumbing in the base class, or remove the opt-in until that migration is complete.
    src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestV4.cs:21
  • This annotation opts the task into in-process execution even though the inherited PublishArtifactsInManifestBase reads Azure credential variables with Environment.GetEnvironmentVariable and has no TaskEnvironment plumbing. That makes the task depend on process-global state, contrary to the multithreadable-task contract. Either thread the environment through the base class or leave this task unannotated.
    src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishBuildToMaestro.cs:35
  • Several execution paths still call _getEnvProxy.GetEnv, whose implementation uses Environment.GetEnvironmentVariable directly (for example, the Azure DevOps collection URI, project, and build identifiers). The task is now annotated and has a TaskEnvironment, but those reads bypass it and retain process-global behavior. Replace the proxy with the injected environment before opting this task into multithreading.
    src/Microsoft.DotNet.Build.Tasks.Feed/src/PushToBuildStorage.cs:48
  • This task is now routed as multithreadable, but it has no TaskEnvironment; the injected file system receives paths such as package.OriginalFile, the local storage directories, and AssetManifestPath without project-relative normalization. Any relative input still uses the process current directory. Normalize the task's path inputs before model/file-system operations or leave it TaskHost-routed.
    src/Microsoft.DotNet.Build.Tasks.Feed/src/common/UploadToAzure.cs:20
  • This task is now opted into concurrent in-process execution, but TokenSource is static and Cancel() cancels that shared source. Cancelling one task instance therefore cancels every concurrently running UploadToAzure invocation, causing unrelated uploads to stop. Make the cancellation source/token instance-owned before enabling multithreaded execution.
    src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateMD5SumsFile.cs:42
  • The file is opened via TaskEnvironment.GetAbsolutePath, but the emitted relative name still subtracts the raw RootDirectory length from the raw ItemSpec. When these inputs are project-relative, the checksum file contains a malformed path (or throws) even though hashing succeeded. Compute the relative name from the same normalized file and root paths.
    src/Microsoft.DotNet.Build.Tasks.Installers/src/ExecWithRetries.cs:18
  • This wrapper opts into in-process multithreading but delegates to Microsoft.Build.Tasks.Exec without injecting a per-project environment or resolving WorkingDirectory. A relative working directory therefore still uses the process current directory, and the child process inherits ambient process state. Keep this task TaskHost-routed (or replace the wrapper with a ToolTask implementation using TaskEnvironment).
    src/Microsoft.DotNet.Build.Tasks.Packaging/src/GeneratePackageReport.cs:15
  • This task now runs in-process and calls FrameworkUtilities, whose shared FrameworkReducer is initialized through an unsynchronized if (s_reducer == null) s_reducer = new FrameworkReducer() lazy path. Concurrent first use can race on that static state, so this opt-in does not meet the thread-safety requirement. Make the helper use Lazy<FrameworkReducer>/synchronized initialization (and audit the other newly annotated callers) before enabling this task.
    src/Microsoft.DotNet.Build.Tasks.Packaging/src/HarvestPackage.cs:619
  • This helper has the same mismatch: it enumerates from TaskEnvironment.GetAbsolutePath(packageFolder) but strips packageFolder.Length from the resulting absolute path. Relative package folders therefore yield incorrect item paths. Keep the normalized folder in the relative-path calculation.
    src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAndroidWorkItems.cs:18
  • The annotated task passes APK metadata through XHarnessTaskBase.GetNameAndPath and raw IFileSystem operations, but neither the task nor the base has a TaskEnvironment. A relative APK path (and the generated payload path) is consequently resolved against the process current directory rather than the project directory. Normalize these paths before the helper/file-system calls or remove the annotation.
    src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAppleWorkItems.cs:18
  • The annotated task passes app-bundle and temporary paths through XHarnessTaskBase/raw IFileSystem operations without a TaskEnvironment. Relative inputs are therefore still resolved against the process current directory in multithreaded builds. Normalize all paths before this helper chain or remove the annotation.
    src/Microsoft.DotNet.Helix/Sdk/FindDotNetCliPackage.cs:21
  • The task is now eligible to run concurrently, but its HTTP client is stored in the static mutable _client field and reassigned for every task instance. Concurrent invocations can overwrite each other's client while requests are in flight, so one task can use another task's handler/configuration. Make the client instance-owned (and dispose it appropriately), or do not opt this task into multithreaded execution.
    src/Microsoft.DotNet.Helix/Sdk/InstallDotNetTool.cs:18
  • This task is annotated, but its destination, dotnet executable, working directory, and mutex/file-system paths are passed through raw IFileSystem/command APIs with no TaskEnvironment. Relative inputs therefore still resolve against the process current directory in multithreaded builds. Inject and apply the task environment to every path, or do not opt this task in.
    src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WritePackageUsageData.cs:146
  • The files returned here are absolute because GetAbsolutePath(RootDir) is used, but GetPathRelativeToRoot still compares them with the unnormalized RootDir. If RootDir is relative, every discovered asset path fails the StartsWith check and the task throws instead of producing usage data. Normalize RootDir once with TaskEnvironment before both the scan and relative-path calculation.
  • Files reviewed: 185/185 changed files
  • Comments generated: 15
  • Review effort level: Lite

Comment thread src/Microsoft.DotNet.Arcade.Sdk/src/CheckRequiredDotNetVersion.cs Outdated
Comment thread src/Microsoft.DotNet.Arcade.Sdk/src/LocateDotNet.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishArtifactsInManifestV3.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishSignedAssets.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Installers/src/CreateLightCommandPackageDrop.cs Outdated
Comment thread src/Microsoft.DotNet.Helix/Sdk/StartAzurePipelinesTestRun.cs
Comment thread src/Microsoft.DotNet.Helix/Sdk/StopAzurePipelinesTestRun.cs
Comment thread src/Microsoft.DotNet.SourceBuild/tasks/src/ReadNuGetPackageInfos.cs Outdated
Comment thread src/Microsoft.DotNet.SourceBuild/tasks/src/WriteBuildOutputProps.cs Outdated
Comment thread src/Microsoft.DotNet.XliffTasks/Tasks/UpdateXlf.cs Outdated
ViktorHofer added a commit to dotnet/dotnet that referenced this pull request Aug 23, 2026
Copilot's reviewer found two bug classes on dotnet/arcade#17381 that my own
audit missed, plus the two races I had already fixed independently.

1. Paths resolved at one site but used raw at another. These do not throw;
   they silently produce wrong results, which is why the null-guard audit
   did not surface them:

   - HarvestPackage enumerated the resolved absolute folder but computed
     package-relative paths with Substring(rawPath.Length + 1), so a relative
     PackagesFolders entry misclassified every harvested asset. Fixed at the
     source: LocatePackageFolder now returns an absolute path, so every
     downstream use agrees.
   - WritePackageUsageData compared absolute enumeration results against a raw
     RootDir in both GetPathRelativeToRoot and the ProjectDirectories
     validation, throwing for every discovered file when RootDir is relative.
     Added AbsoluteRootDir, which preserves any trailing separator because
     callers depend on the stripped result staying relative.
   - UpdateXlf's exception filter compared FileNotFoundException.FileName
     against the unresolved path, turning a friendly BuildErrorException into
     an unhandled exception.
   - SaveItems created the resolved directory but saved to the raw path;
     InstallDotNetCore checked the resolved VersionsPropsPath but loaded the
     raw one and launched the raw DotNetInstallScript; WriteBuildOutputProps
     filtered AdditionalAssetDirs with an unresolved Directory.Exists.

2. Tasks annotated despite process-global state inherited from a base class.
   The analyzer does not walk into base types, so these all passed a clean
   analyzer run. De-annotated with TODOs pointing at arcade#17378:

   - The five Feed publishing tasks (PublishArtifactsInManifestBase reads
     AZURESUBSCRIPTION_*, SYSTEM_ACCESSTOKEN and workload-identity variables
     from the process environment; PublishBuildToMaestro goes through
     _getEnvProxy).
   - Four AzureDevOpsTask derivatives (same environment reads plus a shared
     static Random for retry jitter).
   - Two RoslynBuildTask derivatives (process-wide AssemblyLoadContext.Resolving).
   - SignCheckTask (static _fileVerifiers populated per manager construction),
     FindDotNetCliPackage (static HttpClient overwritten per execution),
     CreateLightCommandPackageDrop (base still uses raw paths).

   FinalizeInsertionVsixFile and ReadNuGetPackageInfos were annotated but never
   implemented IMultiThreadableTask, so TaskEnvironment was never injected and
   every resolution silently used Fallback. Both are now fully migrated instead.

Also keyed the LocateDotNet and CheckRequiredDotNetVersion build-scoped caches
by repository and SDK version; they are registered per build, not per project,
so a second repository could reuse the first one's result.

UpdatePackageIndex is deliberately left annotated: it passes a single
AbsolutePath to PackageIndex.Load, which binds to the uncached overload rather
than the cached IEnumerable one, so it never shares a mutable index.

Annotated tasks: 119 of 136. Arcade builds clean (0 warnings, 0 errors);
PackageTesting (9), Packaging (48) and Feed (214) tests pass. The one
Arcade.Sdk.Tests failure is the pre-existing NU1009 SwixBuild conflict, which
this change does not touch.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 23, 2026 07:18
Copilot stopped reviewing on behalf of ViktorHofer due to an error August 23, 2026 07:39
MSBuild 18.x can execute tasks on multiple threads inside a single node
(`-mt`), but only for tasks that opt in via [MSBuildMultiThreadableTask].
Everything else is routed to an out-of-proc sidecar TaskHost, which is far
more expensive than the in-proc path.

Measuring a runtime inner-repo build in the VMR with `-mt -nodeReuse:true`
against a baseline showed only a 3.6% wall-clock improvement, because none of
Arcade's tasks were annotated: TaskHost task invocations went from 849 to
14,125 and total task time went *up* by 98 seconds, cancelling out most of the
gain from threading.

This annotates 133 of Arcade's 136 concrete tasks and makes the supporting
code multi-threading safe:

- Update the MSBuild package dependencies from 17.12.50 to 18.8.2, which is the
  first version exposing both [MSBuildMultiThreadableTask]/IMultiThreadableTask
  and ToolTask.TaskEnvironment.
- Reference Microsoft.Build.TaskAuthoring.Analyzer from eng/BuildTask.targets so
  new violations are caught at build time. eng/MultiThreadableTaskAnalyzer.globalconfig
  scopes it to tasks that have already opted in, and disables the API-shape
  suggestions (MSBuildTask0006-0008, 0011) which would be binary-breaking for
  task parameters set from targets across the ecosystem.
- Replace ambient process state with the injected TaskEnvironment: relative
  paths now resolve against the project directory instead of the process-wide
  current directory, environment variables are read per-project, and child
  processes are started from TaskEnvironment.GetProcessStartInfo().
- Thread AbsolutePath through the shared path helpers in Packaging, GenFacades,
  Feed, NuGetRepack, SharedFramework.Sdk, PackageTesting and XliffTasks.
- Fix a real data race: TargetFrameworkResolver.CreateOrGet used an
  unsynchronized static Dictionary cache. It is now a ConcurrentDictionary.
- Stop sharing a static Newtonsoft JsonSerializer instance in PackageIndex.
- Delete the internal BuildTask/ILog base class. It only wrapped
  Microsoft.Build.Utilities.Task and TaskLoggingHelper, and standing between
  tasks and the MSBuild base class meant every derived task needed its own
  TaskEnvironment plumbing. Tasks now derive from Microsoft.Build.Utilities.Task
  directly and use Log/TaskLoggingHelper.

SignToolTask, CreateVisualStudioWorkload and CreateVisualStudioWorkloadSet are
deliberately left unannotated. Their helper chains (BatchSignUtil, ZipData,
Configuration, VerifySignatures, WorkloadPackageBase, MsiBase, SwixProject, ...)
still resolve paths against the process-wide current directory, and converting
them is a much larger change. Each carries a TODO pointing at the tracking
issue. Both run once per build, so keeping them TaskHost-routed costs
effectively nothing.

Contributes to dotnet#17378

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (1)

src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishBuildToMaestro.cs:39

  • IMultiThreadableTask is documented as a thread-safe routing signal, even though the current router bug ignores interface-only implementations. Once fixed, this task would run in-process while _getEnvProxy still reads ambient process environment. Remove the interface until environment access is migrated.
  • Files reviewed: 186/186 changed files
  • Comments generated: 14
  • Review effort level: Balanced

Comment thread src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetPackageDescription.cs Outdated
Comment thread src/Microsoft.DotNet.Arcade.Sdk/src/GetAssemblyFullName.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Installers/src/ExecWithRetries.cs Outdated
Comment thread src/Microsoft.DotNet.Helix/Sdk/InstallDotNetTool.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/LaunchDebugger.cs Outdated
Comment thread src/Microsoft.DotNet.GenFacades/GenPartialFacadeSource.cs
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/PublishSignedAssets.cs
- GetPackageDescription: stop memoizing failed description loads. GetOrAdd cached
  the null returned after an IOException, so every later invocation on the node
  silently got null without retrying or re-logging.
- GetAssemblyFullName, ExecWithRetries: implement IMultiThreadableTask and resolve
  paths through TaskEnvironment. ExecWithRetries forwards its TaskEnvironment into
  the nested Exec instance, which MSBuild does not inject into because the task
  constructs it directly.
- InstallDotNetTool, PushToBuildStorage, SingleError, LaunchDebugger: remove the
  MSBuildMultiThreadableTask attribute. These are not yet safe to run in-process,
  so they must keep routing to a TaskHost until they are migrated properly.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 23, 2026 18:45
Copilot AI review requested due to automatic review settings August 23, 2026 18:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/Microsoft.DotNet.Arcade.Sdk/src/LocateDotNet.cs:83

  • This still splits PATH with the Windows-only ; separator. On Unix, PATH is colon-delimited, so the entire value is treated as one directory and dotnet cannot be found. Use Path.PathSeparator for cross-platform lookup.
            var dotNetDir = paths.Split(new[] { ';' }, StringSplitOptions.RemoveEmptyEntries).FirstOrDefault(p => File.Exists(TaskEnvironment.GetAbsolutePath(Path.Combine(p, fileName))));

src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidateHarvestVersionIsLatestForRelease.cs:57

  • Correct the malformed success message.
    src/Microsoft.DotNet.Build.Tasks.Feed/src/LaunchDebugger.cs:13
  • The PR description still claims 133 of 136 tasks are annotated and lists only three deliberate exclusions, but the updated diff now leaves additional tasks unannotated (including this one, SingleError, SignCheckTask, InstallDotNetTool, GenFacades tasks, and several Feed tasks). Update the stated count and exclusion/validation scope so reviewers and performance consumers are not relying on the superseded inventory.
    // Not opted into multithreading, and deliberately never will be: Debugger.Launch attaches a
    // debugger to the whole process. In a shared multithreaded node that would affect every project
    // building on that node, and concurrent invocations would race to attach. Keeping this
    // diagnostic task in the out-of-proc TaskHost confines the attach to a single sidecar.
  • Files reviewed: 187/187 changed files
  • Comments generated: 6
  • Review effort level: Balanced

Comment thread src/Microsoft.DotNet.GenAPI/GenAPITask.cs Outdated
Comment thread src/Microsoft.DotNet.Helix/Sdk/SendHelixJob.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/CreateAzureDevOpsFeed.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/common/CreateNewAzureContainer.cs Outdated
Comment thread src/Microsoft.DotNet.Build.Tasks.Feed/src/common/UploadToAzure.cs Outdated
…DotNetVersion

Both tasks do a non-atomic GetRegisteredTaskObject/RegisterTaskObject pair, which
under multithreaded execution lets two threads miss and populate the cache. Both
computations are pure, so the result is unchanged; the only visible effect is that
CheckRequiredDotNetVersion can log a failing check's error twice.

Recording this so the pattern is not mistaken for the SingleError case, where the
same race defeated the task's entire purpose and the attribute had to be removed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 23, 2026 19:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.DotNet.SourceBuild/tasks/src/UsageReport/WritePackageUsageData.cs:21

  • This opt-in still leaves the four package-file inputs on ambient path resolution: ReadNuGetPackageInfos.ReadIdentity opens each supplied string directly, unlike the runtime JSON paths migrated below. If any item identity is relative, a shared-node build can read from the process working directory instead of this project. Resolve every package path with TaskEnvironment.GetAbsolutePath before calling ReadIdentity, or keep this task TaskHost-routed until that migration is complete.
  • Files reviewed: 187/187 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

…ncies

All six are P3 with no measured invocations in the hot-path profile, so
in-process routing buys no measured build-time saving while taking on real
correctness risk. Each keeps its `IMultiThreadableTask` implementation and
`TaskEnvironment`-based path handling -- correct either way, and the groundwork
for a later migration -- but loses the routing attribute, and carries a comment
recording why so it is not silently re-added.

- GenAPITask: `HostEnvironment` resolves the raw `LibPath`/`Assembly` values via
  `Environment.ExpandEnvironmentVariables` plus `Directory.Exists`/`File.Exists`
  (HostEnvironment.cs:719-740). The task resolves `OutputPath` and `HeaderFile`
  through `TaskEnvironment` but hands these two through raw -- resolved at one
  site, raw at another. Fixing it needs expand-then-resolve inside Cci, because
  resolving a path that still contains %VAR% would itself be wrong.
- SendHelixJob: `JobDefinition` reads BUILD_REPOSITORY_NAME, BUILD_SOURCEBRANCH,
  SYSTEM_TEAMPROJECT and BUILD_REASON straight from `Environment`
  (JobDefinition.cs:209-223,402-423).
- CreateAzureDevOpsFeed, CreateNewAzureContainer, CreateAzureContainerIfNotExists:
  build an `AzureCliCredential` when no PAT/AccountKey is supplied, which launches
  `az` from the ambient process environment. This is the same class already used
  to exclude PublishArtifactsInManifest*; the criterion was applied incompletely.
- UploadToAzure: cancellation state is `static`, so `Cancel()` on one instance
  cancels the token handed to every other concurrent and future instance, and the
  uploads use an unlinked timeout token rather than the task token.

Also avoided writing the literal attribute name in these comments, so an audit
grep for the attribute does not match a class that deliberately lacks it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
Copilot AI review requested due to automatic review settings August 24, 2026 06:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (5)

Previously missed (5) — in code that hasn't changed since the last review.

src/Microsoft.DotNet.Build.Tasks.Packaging/src/HarvestPackage.cs:17

  • This opt-in is not thread-safe yet: HarvestSupportedFrameworks passes RuntimeFile unchanged to AggregateNuGetAssetResolver and NuGetAssetResolver, whose constructors call JsonRuntimeFormat.ReadRuntimeGraph(runtimeFile). A relative value (the existing tests use "runtime.json") therefore still resolves against the shared process working directory and can load another project's graph or fail under -mt. Resolve the value through TaskEnvironment and preferably change both resolver constructors to accept AbsolutePath; until then, keep this task TaskHost-routed.
    src/Microsoft.DotNet.Build.Tasks.Packaging/src/GeneratePackageReport.cs:15
  • This task still sends the raw RuntimeFile to AggregateNuGetAssetResolver and NuGetAssetResolver; both constructors open it via JsonRuntimeFormat.ReadRuntimeGraph(string). Relative task inputs consequently resolve against the shared node's process directory rather than this project's directory, so concurrent projects can read the wrong runtime graph. Resolve this path through TaskEnvironment (ideally by retyping the shared resolver API to AbsolutePath) before opting in.
    src/Microsoft.DotNet.Build.Tasks.Packaging/src/GetApplicableAssetsFromPackages.cs:17
  • The new opt-in leaves RuntimeFile unresolved when constructing AggregateNuGetAssetResolver; that helper opens the string directly with JsonRuntimeFormat.ReadRuntimeGraph. With a relative runtime graph, this task therefore depends on the shared process current directory and can read another project's file under -mt. Thread an AbsolutePath into the resolver or leave this task unannotated.
    src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetPack.cs:18
  • BaseDirectory is still passed unchanged to NuGet's PackageBuilder.PopulateFiles, which performs filesystem enumeration relative to that base. When callers provide a relative BaseDirectory, the annotated task resolves package inputs against the shared process working directory instead of the project directory. Absolutize BaseDirectory before PopulateFiles, or keep this task TaskHost-routed until that path is migrated.
    src/Microsoft.DotNet.SignCheckTask/SignCheckTask.cs:17
  • The PR description still says 133 of 136 tasks are annotated and lists only three exclusions, but the current revision documents many additional concrete exclusions (including this task, SingleError, SendHelixJob, InstallDotNetTool, both GenFacades tasks, several Feed tasks, and others). This materially changes the claimed migration coverage and expected TaskHost reduction; update the inventory/count and the “Deliberately not migrated” section to match the code.
  • Files reviewed: 187/187 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread eng/Version.Details.props
ViktorHofer added a commit to dotnet/msbuild that referenced this pull request Aug 24, 2026
…ration (#14795)

Encodes into the migration skill and reviewer agent the defect classes
found while migrating ~150 tasks to `[MSBuildMultiThreadableTask]`
across dotnet/arcade
([arcade#17381](dotnet/arcade#17381)),
dotnet/source-build-assets
([#1776](dotnet/source-build-assets#1776)) and
the dotnet/dotnet VMR.

### Why

`Microsoft.Build.TaskAuthoring.Analyzer` was enabled in all three repos
and every build was **0 warnings / 0 errors**. A manual audit afterwards
still found **9 real defects**:

| Why it was missed | Count |
|---|---|
| Code outside the analysis scope (unannotated base class) | 2 |
| API not on the monitored list (`AssemblyName.GetAssemblyName`) | 3 |
| Failure class not modeled at all (task-object race, memoized failure,
nested task construction, path crossing a DI boundary) | 4 |

Every one of these was findable by reading the code with the right
checklist — so they belong in the skill. Analyzer gaps are filed
separately against #14772 (#14791, #14792, #14793) plus #14794 for a
runtime forcing function.

### Changes

**SKILL.md**
- **Sin 8 — swallowed exceptions hiding an unresolved path.** Where a
`catch` means a *semantic answer* ("not an assembly") rather than
"fail", an unresolved path produces a silently wrong result instead of
an error. In the VMR's `CheckForPoison` this skipped both poison checks,
turning a leaked binary into a false negative in source-build's
leak-detection gate — with a green build.
- **Analyzer-invisible path consumers.** `MSBuildTask0003` monitors 8
types; anything else taking a path string is equally unsafe and silent.
`AssemblyName.GetAssemblyName` on a raw input was the most-repeated
defect of the whole exercise. Notes the string-vs-stream overload
distinction.
- **Unsafe code in an unannotated base class.** The attribute is
`Inherited = false`, so bases are unannotated — and under `scope =
multithreadable_only` they are never analyzed, despite executing
multithreaded. Real case: `AkaMSLinksBase` held `File.ReadAllText` on a
raw task input while both derived tasks were annotated and clean.
- **Engine-owned shared state: `RegisterTaskObject`.** Non-atomic
read/write with no `static` field in sight, so it reads as thread-safe.
Includes the benign/broken decision table — `LocateDotNet` is fine,
`SingleError` was not.
- **Paths crossing interface / DI boundaries**, where no `System.IO`
type appears in the task at all.
- **New "Verification" section** — a clean analyzer run is not a
migration, with the miss breakdown above and why "it worked when I
tested it" is weak evidence for a failure mode that is load- and
schedule-dependent.
- **New "Migrating Is Not Always the Right Answer" section** — leaving a
task unannotated preserves today's TaskHost behavior exactly and is a
supported outcome. A slower task beats a wrong one.
- Sign-off checklist extended with the above; fixed a pre-existing
broken anchor to Sin 2.

**mt-migration-reviewer.agent.md**
- Operating rule 7: never accept a clean analyzer run as a basis for
approval.
- Step 1 now walks the base chain, `catch` blocks, caches and DI
boundaries.
- Step 2 hazard table gains analyzer-invisible APIs, DI boundaries,
semantic-answer catches and the `RegisterTaskObject` pair. Also
**corrects an existing row**: `AssemblyName.GetAssemblyName` was listed
only under Sin 2 message leakage, missing that the call is itself
CWD-dependent.
- Step 5: explicitly approve a justified decision *not* to migrate, so
the agent does not create an incentive to maximize annotation count.

Docs only — no product code, no tests affected.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
LaunchDebugger has no UsingTask registration and no reference anywhere in
the repo, so it is unreachable dead code. It was one of the tasks left
unannotated for the multithreading migration, since Debugger.Launch attaches
a debugger to the whole process and a shared multithreaded node would attach
for every project building on it. Deleting it removes the exclusion instead
of documenting it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
Copilot AI review requested due to automatic review settings August 24, 2026 10:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (1)

src/Microsoft.DotNet.Build.Tasks.Feed/src/LaunchDebugger.cs:1

  • The PR description says LaunchDebugger is one of the tasks deliberately left unannotated, but this change deletes the public task class instead. Any consumer that references Microsoft.DotNet.Build.Tasks.Feed.LaunchDebugger will now fail to load it. Restore the unannotated task (as described), or explicitly document and version this as a breaking removal.
  • Files reviewed: 187/187 changed files
  • Comments generated: 11
  • Review effort level: Balanced

Comment thread src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAndroidWorkItems.cs
Comment thread src/Microsoft.DotNet.PackageTesting/GetCompatiblePackageTargetFrameworks.cs Outdated
Comment thread src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAppleWorkItems.cs
Comment thread src/Microsoft.DotNet.Build.Tasks.Packaging/src/NuGetPack.cs
Comment thread src/Microsoft.DotNet.Build.Tasks.Packaging/src/HarvestPackage.cs
UploadToAzure has no UsingTask registration and no reference anywhere in the
repo, so it is unreachable dead code. Uploads go through
AzureStorageAssetPublisher and BlobFeedAction instead.

It was one of the tasks left unannotated for the multithreading migration:
its CancellationTokenSource is static, so Cancel() on one instance cancelled
the token handed to every other concurrent and future instance, and the
uploads used an unlinked timeout token rather than the task token. Deleting
it removes that bug along with the exclusion.

Its base AzureConnectionStringBuildTask and the AzureStorageUtils helpers stay
in use by CreateAzureContainer and the publishers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
Copilot AI review requested due to automatic review settings August 24, 2026 10:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Microsoft.DotNet.Build.Tasks.Packaging/src/ValidateHarvestVersionIsLatestForRelease.cs:57

  • Correct the misspelling in this success message: “erreleasea” should be “release.”
  • Files reviewed: 187/187 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAndroidWorkItems.cs
Comment thread src/Microsoft.DotNet.Helix/Sdk/CreateXHarnessAppleWorkItems.cs
Copilot review flagged eleven annotated tasks that pass a raw, user-settable
path into a helper that opens it with plain File/Directory/NuGet APIs. In a
shared -mt node those bind to the process current directory rather than the
invoking project.

The inputs are not absolute by construction. RuntimeGraph and
RuntimeIdentifierGraph come from $(RuntimeIdentifierGraphPath), which is a
public settable property, and RuntimeFile comes from
$(RuntimeIdGraphDefinitionFile), which is explicitly guarded with
Condition="'$(...)' == ''" so it is designed to be overridden. Only the
Bundled* SDK defaults are absolute by construction.

Resolved through the injected TaskEnvironment:

- ChooseBestTargetFrameworksTask, ChooseBestP2PTargetFrameworkTask: RuntimeGraph.
  These had the attribute but did not implement IMultiThreadableTask, so they had
  no way to resolve anything; both now do. TargetFrameworkResolver also caches by
  the path string, so a relative value additionally let two projects share one
  resolver built from different files.
- GeneratePackageReport, GetApplicableAssetsFromPackages, HarvestPackage:
  RuntimeFile, before constructing the NuGet asset resolvers.
- GenerateSharedFrameworkDepsFile: RuntimeIdentifierGraph. It is optional, so an
  unset value is passed through unchanged to keep the existing ReadRuntimeGraph
  error instead of throwing from GetAbsolutePath.
- NuGetPack: BaseDirectory before PopulateFiles, and the nuspec-relative fallback.
- GetCompatiblePackageTargetFrameworks: PackagePaths. Also had the attribute
  without the interface.
- WritePackageUsageData: the four package-file arrays feeding ReadIdentity.
- CreateXHarnessAndroidWorkItems, CreateXHarnessAppleWorkItems: ApkPath and
  AppBundlePath, plus the Apple TmpDir. Relative paths here are the documented
  usage in tools/xharness-runner/Readme.md, so this was the most likely of the
  set to bite. TaskEnvironment is added to the shared XHarnessTaskBase.

The XHarness tests already covered relative paths, which is why five of them
caught the change. They now resolve their MockFileSystem registrations through
the same TaskEnvironment the task uses, so the relative-path coverage is kept
and the resolution itself is exercised.

Full non-incremental Release build: 0 errors, 0 warnings.
Helix.Sdk 236, Packaging 48, PackageTesting 9, SourceBuild 1: all pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
Copilot AI review requested due to automatic review settings August 24, 2026 11:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/Microsoft.DotNet.Arcade.Sdk/src/CheckRequiredDotNetVersion.cs:74

  • Absolute paths are case-sensitive on Unix. This comparison can treat /repo/A/global.json and /repo/a/global.json as the same cache entry; when the timestamps and requested SDK match, the second repository can reuse the first repository's validation result even if its global.json has different requirements. Use an ordinal comparison (a harmless extra cache miss on Windows) or an OS-aware path comparer.
                string.Equals(globalJsonPath.Value, cachedResult.GlobalJsonPath, StringComparison.OrdinalIgnoreCase) &&

src/Microsoft.DotNet.Arcade.Sdk/src/LocateDotNet.cs:66

  • Absolute paths are case-sensitive on Unix. This comparison aliases repositories whose paths differ only by case, so matching timestamps and PATH values can cause one repository to reuse the other repository's resolved SDK path even when their global.json files request different SDKs. Use an ordinal comparison (a harmless extra cache miss on Windows) or an OS-aware path comparer.
                string.Equals(globalJsonPath.Value, cachedResult.GlobalJsonPath, StringComparison.OrdinalIgnoreCase) &&
  • Files reviewed: 190/190 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

…asks

WixToolTask and HarvesterToolTask are not registered via UsingTask; MsiBase
constructs them directly. MSBuild therefore never routes them and never injects
a TaskEnvironment, so WixToolTaskBase resolves against TaskEnvironment.Fallback
and the annotation had no effect other than marking part of the
CreateVisualStudioWorkload helper chain as migrated, which contradicts the
TODO comments on both entry-point tasks.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: be19d794-fe4f-43fe-9e6e-f6e6b13ec6e7
Copilot AI review requested due to automatic review settings August 24, 2026 11:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/Microsoft.DotNet.Arcade.Sdk/src/CheckRequiredDotNetVersion.cs:74

  • OrdinalIgnoreCase can conflate distinct repositories on case-sensitive filesystems. For example, /repo/A/global.json and /repo/a/global.json with the same timestamp and requested SDK version cause the second invocation to reuse the first file's result. Use an ordinal comparison; on Windows, a casing-only difference would then only cause a harmless cache miss.
                string.Equals(globalJsonPath.Value, cachedResult.GlobalJsonPath, StringComparison.OrdinalIgnoreCase) &&

src/Microsoft.DotNet.Arcade.Sdk/src/LocateDotNet.cs:66

  • OrdinalIgnoreCase can conflate different global.json files on case-sensitive filesystems. If paths differing only by case have matching timestamps and PATH values, this task can return the dotnet executable selected for the other repository. Use an ordinal comparison; a casing-only path on Windows would merely miss the cache.
                string.Equals(globalJsonPath.Value, cachedResult.GlobalJsonPath, StringComparison.OrdinalIgnoreCase) &&
  • Files reviewed: 190/190 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread src/Microsoft.DotNet.XliffTasks/Tasks/XlfTask.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants