Enlighten GetAssemblyIdentity task for multithreaded mode

[jankratochvilcz]

Enlighten GetAssemblyIdentity task for multithreaded mode

Parent: #11834

Context

GetAssemblyIdentity is one of the remaining tasks listed in the migration epic (#11834) under "Other (either simple or with unknown issues)". The task is short — one Execute() method that loops over AssemblyFiles and emits one [Output] item per input — but it has exactly one cwd-dependent line:

GetAssemblyIdentity.Execute
  → foreach (ITaskItem item in AssemblyFiles)
       → AssemblyName.GetAssemblyName(item.ItemSpec)
            → File.OpenRead(path)   // consumes process cwd when ItemSpec is relative

AssemblyName.GetAssemblyName opens the file by path on both .NET Framework and .NET (Core+) to read the manifest — a relative ItemSpec is resolved against Environment.CurrentDirectory. Under multithreaded execution that's wrong: concurrent GetAssemblyIdentity instances may belong to different projects, and there is only one cwd per process.

AssemblyName.GetAssemblyName is a BCL API with no baseDirectory overload, so the only correct fix is to absolutize item.ItemSpec at the task boundary and pass the resulting absolute string to the BCL.

The [Output] Assemblies items carry no path leak risk: each is constructed with ItemSpec = an.FullName (an assembly identity such as "Foo, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null"), and item.CopyMetadataTo(newItem) copies the input item's metadata — including its original (unmodified) ItemSpec-derived metadata such as OriginalItemSpec, FullPath, etc. As long as we never assign an AbsolutePath (or its implicit string conversion) into the new item's metadata, Sin 1 is structurally avoided.

Approach

Apply the PR #13267 (MSBuild/CallTarget) pattern: absolutize at the task boundary, hand a pure string to the helper. Pattern #1 from the SKILL — smallest blast radius.

1. Mark GetAssemblyIdentity with [MSBuildMultiThreadableTask] and implement IMultiThreadableTask with:

   public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback;

2. Inside the loop, hoist the absolutization above the try:

   AbsolutePath assemblyPath;
   try
   {
       assemblyPath = TaskEnvironment.GetAbsolutePath(item.ItemSpec);
   }
   catch (ArgumentException e)
   {
       Log.LogErrorWithCodeFromResources("GetAssemblyIdentity.CouldNotGetAssemblyName", item.ItemSpec, e.Message);
       continue;
   }

   (See Sin 6 below — GetAbsolutePath throws ArgumentException for null/empty/invalid characters that the old code would have surfaced as an ArgumentException from AssemblyName.GetAssemblyName and caught via ExceptionHandling.IsIoRelatedException. Catching ArgumentException explicitly preserves the same per-item error semantics: log, continue to the next item.)
3. Replace AssemblyName.GetAssemblyName(item.ItemSpec) with AssemblyName.GetAssemblyName(assemblyPath) (implicit AbsolutePath → string conversion).
4. Leave the existing BadImageFormatException and ExceptionHandling.IsIoRelatedException catches unchanged — both still fire from AssemblyName.GetAssemblyName. The error message argument stays item.ItemSpec (Sin 2 prevention — user sees the path they wrote, not the absolutized form).
5. Do not touch the output construction or CopyMetadataTo — an.FullName is an assembly identity, not a path, and the input item's metadata is preserved as-is.

No canonicalization is needed (Sin 5 N/A): the absolutized path is consumed by File.OpenRead deep in the BCL, not used as a dictionary key, displayed to the user, or written to an output property.

Why no ChangeWave

For absolute AssemblyFiles inputs, behavior is byte-identical. For relative inputs the base directory changes from Environment.CurrentDirectory to TaskEnvironment.ProjectDirectory — but in legacy (non-MT) execution the two are equal, so observable behavior is unchanged. The semantic change only manifests under MT, which is the entire point of the migration.

The one edge case is null/empty/invalid-character ItemSpec: old code threw an ArgumentException from AssemblyName.GetAssemblyName, which IsIoRelatedException catches → log error and continue. New code throws ArgumentException from GetAbsolutePath first, which the new explicit catch (ArgumentException) handles identically (log error and continue). The MSBxxxx code, message format, and per-item continuation behavior are identical, so no ChangeWave is required.

If unforeseen behavioral diffs are found during review, gate the diff behind a new ChangeWave following the precedent set in PR #13069.

Sin 6 considerations

AssemblyName.GetAssemblyName already throws ArgumentException for null/empty paths and for paths containing invalid characters; that exception is caught today by ExceptionHandling.IsIoRelatedException (which includes ArgumentException). Adding an explicit catch (ArgumentException) around GetAbsolutePath preserves the exact same per-item outcome (error logged with the same MSBuild code, loop continues). No exception type observed by the user changes; Execute() still returns false if any item failed.

Test coverage assessment

Existing direct unit tests

None. A repo-wide search for GetAssemblyIdentity in src/Tasks.UnitTests/ finds no test file — the task ships with zero direct unit-test coverage today.

Integration tests in this repo

GetAssemblyIdentity is referenced from Microsoft.Common.CurrentVersion.targets (ClickOnce / publish flow, references resolution) and Microsoft.Common.tasks. End-to-end coverage of those flows lives in dotnet/sdk and the VS repo, not here. We will not add E2E publish/reference tests in this repo as part of this issue.

Gaps to fill in this PR

Because there is no existing test file, this PR creates src/Tasks.UnitTests/GetAssemblyIdentity_Tests.cs from scratch. Required tests:

• G0 — Baseline coverage (new file). At minimum:
  • Absolute path to an existing assembly → Assemblies[0].ItemSpec == an.FullName, Name/Version/PublicKeyToken/Culture metadata populated correctly, input metadata copied via CopyMetadataTo.
  • Non-existent file → Execute() returns false, error logged with MSBuild code from GetAssemblyIdentity.CouldNotGetAssemblyName, item is skipped, loop continues to subsequent items.
  • Non-assembly file (e.g., a text file) → BadImageFormatException path, error logged, loop continues.
  • Mixed batch (one good, one bad) → good item appears in output, bad item's error is logged, Execute() returns false.
  • Null/empty ItemSpec → error logged with the MSBuild code, loop continues, Execute() returns false (covers both old IsIoRelatedException path and new ArgumentException-from-GetAbsolutePath path; assertions identical).
• G1 — Concurrency test. Two GetAssemblyIdentity instances with different ProjectDirectory values, each with the same relative AssemblyFiles ItemSpec pointing at a file present in the respective project directory only, run concurrently. Assert each instance resolves to its own project's file and produces the correct identity. (SKILL red-team Phase 4.)
• G2 — Project-relative test. ProjectDirectory != Environment.CurrentDirectory, relative ItemSpec resolves against ProjectDirectory. Documents the intentional semantic change. Place a test assembly under ProjectDirectory, set process cwd elsewhere, assert success.
• G3 — Output integrity test (Sin 1 guard). Assert that for an input item with metadata OriginalItemSpec=foo.dll;Foo=Bar, the output item's ItemSpec is the assembly identity string (not a path), and OriginalItemSpec/Foo metadata are copied verbatim from the input — i.e., no absolutized path leaks into output metadata.
• G4 — Error message integrity test (Sin 2 guard). Pass a relative non-existent ItemSpec ("does-not-exist.dll") with ProjectDirectory set to a tmp dir. Assert the logged error message contains "does-not-exist.dll" (the original) and does not contain the absolutized ProjectDirectory + "/does-not-exist.dll" form.

Tests must inject t.TaskEnvironment = TaskEnvironmentHelper.CreateForTest() (G0, default) or TaskEnvironment.CreateWithProjectDirectoryAndEnvironment(...) (G1, G2, G4) per the SKILL "Updating Unit Tests" guidance. Run on both net472 and net10.0 — AssemblyName.GetAssemblyName exists on both but the underlying loader differs.

Acceptance criteria

• GetAssemblyIdentity decorated [MSBuildMultiThreadableTask] and implements IMultiThreadableTask with public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback;.
• item.ItemSpec is absolutized via TaskEnvironment.GetAbsolutePath(item.ItemSpec) before being passed to AssemblyName.GetAssemblyName.
• The absolutization call site catches ArgumentException and logs GetAssemblyIdentity.CouldNotGetAssemblyName with the original item.ItemSpec (Sin 2), then continues to the next item — preserving the legacy per-item error/continue semantics.
• Existing BadImageFormatException and ExceptionHandling.IsIoRelatedException catches are preserved unchanged; both still log the original item.ItemSpec.
• Output Assemblies[i].ItemSpec remains an.FullName (assembly identity); no AbsolutePath value is assigned to any output item's ItemSpec or metadata (Sin 1).
• item.CopyMetadataTo(newItem) continues to copy the input item's metadata as-is — OriginalItemSpec and other input metadata are not absolutized.
• All new tests in src/Tasks.UnitTests/GetAssemblyIdentity_Tests.cs (G0, G1, G2, G3, G4) pass on net472 and net10.0.
• Behavior for absolute AssemblyFiles inputs is byte-identical to today (output ItemSpec, metadata, error messages, exit code).
• No ChangeWave required; if any new behavioral diff is discovered, document it in the PR and gate behind the next wave.
• .\build.cmd -v quiet succeeds; Tasks.UnitTests and Build.UnitTests pass.
• No new compiler warnings (warnings-as-errors in official build).
• multithreaded-task-migration SKILL sign-off checklist walked and passes.

Implementation order

1. Create src/Tasks.UnitTests/GetAssemblyIdentity_Tests.cs with G0 (baseline coverage) against the current unmodified task. Capture exact MSBuild error code, error-message format, and output item shape from a green main-branch run — these are the compatibility baseline.
2. Apply [MSBuildMultiThreadableTask] + IMultiThreadableTask to GetAssemblyIdentity; hoist GetAbsolutePath above the existing try with its own catch (ArgumentException); pass the absolute string to AssemblyName.GetAssemblyName.
3. Run G0 — must still pass with no edits (proves no Sin 1/Sin 2 regression).
4. Add G1 (concurrency), G2 (project-relative), G3 (output integrity), G4 (error message integrity).
5. Run full Tasks.UnitTests and Build.UnitTests on both net472 and net10.0; verify clean.
6. Run repo build with -v quiet to ensure no new warnings.

Risks / open questions

• Cross-framework loader differences. On .NET Framework, AssemblyName.GetAssemblyName opens the file via the Fusion loader; on .NET (Core+) it uses the managed MetadataReader. Both consume cwd for relative paths, but error messages and the precise BadImageFormatException/FileLoadException discrimination differ. Existing catch arms already cover both shapes via IsIoRelatedException; no change expected, but G0 must run on both TFMs.
• ClickOnce / reference-resolution flow under MT is not exercised by this repo's tests. Coordination with dotnet/sdk recommended before MT mode ships generally, but not blocking for this PR — the task itself is correct in isolation once migrated.
• No existing direct unit tests means this PR effectively bootstraps the test surface for GetAssemblyIdentity. The G0 baseline tests should be reviewed independently of the migration changes — they document long-standing behavior for the first time.

References

• Parent epic: Migrate Tasks in the MSbuild repo to the new Task type #11834
• Sibling issue (same pattern): Enlighten FormatUrl task for multithreaded mode #13567 (FormatUrl)
• Pattern PRs:
  • Enlighten public versions of intrinsic tasks. #13267 — Enlighten public versions of intrinsic tasks (absolutize-at-boundary pattern — the closest analog)
  • Enlighten FindUnderPath and AssignTargetPath tasks #13069 — Enlighten FindUnderPath and AssignTargetPath (ChangeWave usage)
  • Enlighten more tasks that require no change #13045 — Enlighten more tasks that require no change (attribute-only baseline)
  • Enlighten GetFrameworkPath and GetFrameworkSdkPath. #13282 — Enlighten GetFrameworkPath and GetFrameworkSdkPath (locking shared static state)
  • Enlighten RAR task #13319 — Enlighten RAR (heavyweight thread-TaskEnvironment-through-helpers pattern; for reference only)
• SKILL: .github/skills/multithreaded-task-migration/SKILL.md