Add artifact naming service with template support for consistent naming across extensions

[Evangelink]

Summary

Add ArtifactNamingHelper — a reusable static helper that allows extensions to generate consistent artifact file names using template placeholders.

Users can specify templates like:

• {pname}_{pid}_{time}_hang.dmp → MyTests_12345_2025-09-22_13-49-34.0000000_hang.dmp
• {asm}_{tfm}_{time}.log → MyTests_net9.0_2025-09-22_13-49-34.0000000.log

Available Placeholders

Placeholder	Description	Example
{pname}	Process name	MyTests
{pid}	Process ID	12345
{asm}	Assembly name (entry assembly, or unknown if unavailable)	MyTests
{tfm}	Target framework moniker (best-effort, detected at runtime)	net9.0
{time}	Timestamp (high precision, 100ns)	2025-09-22_13-49-34.0000000

Key Design Decisions

• Static helper — ArtifactNamingHelper is a shared static class, compiled into each extension via file linking (no service registration or IVT required)
• Centralized value resolution — GetStandardReplacements() provides all standard placeholder values so consumers don't duplicate resolution logic
• Case-sensitive placeholders — use lowercase names (e.g., {pname}, not {PName})
• Source-generated regex on .NET (falls back to RegexOptions.Compiled on netstandard2.0)
• Custom replacements allow callers to override defaults (e.g., when dumping a child process with a different PID)
• Unknown placeholders are preserved as-is
• Backward compatibility — legacy %p pattern handled in the HangDump caller via string.Replace
• Filesystem-safe time format — uses hyphens and underscores instead of colons
• Path traversal protection — resolved paths are validated to stay within the results directory

Changes

• New ArtifactNamingHelper static class (file-linked into extensions)
• HangDumpProcessLifetimeHandler updated to use the helper
• Unit tests and integration test
• Documentation in docs/microsoft.testing.platform/ArtifactNamingHelper.md

Note

This PR revives and fixes the content from #6587 (closed copilot PR), with extensive code review fixes:

• Fixed build for netstandard2.0 (removed OperatingSystem.IsXxx(), KeyValuePair deconstruction, [..8] range syntax)
• Fixed non-existent Guard.NotNullOrEmpty API
• Fixed broken legacy %p handling
• Removed fragile {project} placeholder (directory tree traversal)
• Fixed banned Environment API usage (RS0030)
• Fixed all analyzer warnings

Fixes #6586

Related to #5364, #7345, #6648, #4130, #7126, #6778

[Copilot]

Pull request overview

This PR introduces an internal IArtifactNamingService to centralize artifact filename/path templating across Microsoft.Testing.Platform extensions, and migrates HangDump to use it (while keeping legacy %p support).

Changes:

• Added IArtifactNamingService + ArtifactNamingService implementation with placeholder-based template resolution.
• Registered the service in TestHostBuilder and exposed it via IServiceProvider extension method.
• Updated HangDump to use the service and added/updated unit + integration tests plus documentation.

Show a summary per file

File	Description
test/UnitTests/Microsoft.Testing.Platform.UnitTests/Services/ArtifactNamingServiceTests.cs	Adds unit tests covering placeholder replacement, casing, unknown placeholders, and error cases.
test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/HangDumpTests.cs	Adds integration coverage for HangDump template-based filenames.
src/Platform/Microsoft.Testing.Platform/Services/ServiceProviderExtensions.cs	Adds GetArtifactNamingService() accessor.
src/Platform/Microsoft.Testing.Platform/Services/IArtifactNamingService.cs	Introduces the internal artifact naming service contract.
src/Platform/Microsoft.Testing.Platform/Services/ArtifactNamingService.cs	Implements regex-based placeholder resolution and default replacement sources.
src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.cs	Registers ArtifactNamingService into the platform service provider.
src/Platform/Microsoft.Testing.Platform/Helpers/System/SystemEnvironment.cs	Implements newly added Version property on the environment wrapper.
src/Platform/Microsoft.Testing.Platform/Helpers/System/IEnvironment.cs	Adds Version to the environment abstraction.
src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpProcessLifetimeHandler.cs	Uses IArtifactNamingService for dump filename resolution (then applies legacy %p).
src/Platform/Microsoft.Testing.Extensions.HangDump/HangDumpExtensions.cs	Wires IArtifactNamingService into HangDumpProcessLifetimeHandler construction.
docs/ArtifactNamingService.md	Documents placeholders, custom replacements, and HangDump usage.

Copilot's findings

• Files reviewed: 11/11 changed files
• Comments generated: 2

[Copilot]

Copilot's findings

• Files reviewed: 7/8 changed files
• Comments generated: 2

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-12
Repository: microsoft/testfx

Key Findings

The test suite is in good shape — previous reviews have successfully addressed the major findings (broken TFM regex, weak ContainsSingle assertion). Two minor coverage gaps remain in the unit tests:

1. [Coverage] No test for repeated placeholder substitution (ArtifactNamingHelperTests.cs line 88) — Regex.Replace with a MatchEvaluator replaces all matches globally, but this global-replace behaviour is never explicitly asserted. A template like <pname>_<pname>.dmp would be the minimal test.

2. [Coverage] No test for a no-placeholder template with non-empty replacements (ArtifactNamingHelperTests.cs line 126) — The NullReplacements/EmptyReplacements tests short-circuit before the regex runs. A plain template like "simple.dmp" with a non-empty dictionary exercises the regex path with zero matches, which is currently untested.

What looks good

• Isolation: No shared mutable state; every test uses local variables only.
• Error paths: All three invalid-template inputs (null, empty, whitespace) are covered with Assert.ThrowsExactly.
• Case-sensitivity: Explicitly tested and correctly asserted.
• Integration tests: Guid.NewGuid() per-run directories avoid file-system conflicts; Assert.ContainsSingle used for deterministic dump-file detection; TFM regex correctly uses net[\w.]+ to cover net9.0, net10.0, net462, etc.
• Platform assertion: GetOperatingSystemName_ReturnsKnownValue uses Assert.Contains against a closed set and explicitly anchors the three known platforms.

Recommendations

Add the two suggested test cases noted above — they are small and will make the coverage complete.

---

Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-12
Repository: microsoft/testfx

Key Findings

[Correctness] <asm>/<tfm> placeholder omission creates invalid Windows paths when resolution fails

The latest commit (5f8bb212) correctly replaced the "" (empty-string) fallback for asm/tfm to fix a path-traversal risk (<asm>/ + empty string → root-relative path on Linux). However, the chosen fix — omitting the key from replacements when the value is null — introduces a symmetric correctness problem on Windows: if the entry assembly is null or TargetFrameworkParser can't parse the framework description, the placeholder remains as literal <asm> or <tfm> in the resolved filename. Path.GetFullPath then throws ArgumentException on Windows because < and > are illegal path characters.

Impact: ArgumentException crash on Windows for any user whose --hangdump-filename template uses <asm> or <tfm> and whose entry assembly or TFM resolution returns null. The <os> placeholder is immune because GetOperatingSystemName() always returns a safe string (falling back to "unknown").

Suggestion: Replace the conditional-add pattern with an unconditional fallback of "unknown", which is already the established pattern for <os>:

replacements["asm"] = asmName ?? "unknown";
replacements["tfm"] = tfm ?? "unknown";

Positive Observations

• The overall design of ArtifactNamingHelper as a file-linked, [Embedded] static class is clean and avoids IVT coupling.
• ResolveTemplate correctly preserves unknown placeholders as-is and is case-sensitive by design.
• The static Regex with RegexOptions.Compiled is appropriately placed.
• The integration tests are thorough, covering both the flat template case and the subdirectory case (with <asm>/), and the unit tests cover all documented edge cases including case-sensitivity and null/empty inputs.
• The legacy %p backward compatibility via string.Replace after ResolveTemplate is a clean approach.

Overall Assessment

The PR is in very good shape — all previously flagged issues have been addressed. One narrow correctness issue remains in the new commit's null-handling strategy for <asm> and <tfm>.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-12
Repository: microsoft/testfx

Key Findings

[Security] Path traversal guard bypass (HangDumpProcessLifetimeHandler.cs line 336)

The new path-confinement check introduced in commit 50f8d99f uses:

if (!finalDumpFileName.StartsWith(resultsDirectory, StringComparison.Ordinal))

resultsDirectory has no trailing path separator (e.g. /tmp/testresults). A template that resolves via .. to a sibling directory with a name that shares the same prefix (e.g. /tmp/testresults_evil/file.dmp) passes the check despite being outside the results directory. The fix is to append Path.DirectorySeparatorChar to the guard string.

Positive Observations

• The ArtifactNamingHelper.ResolveTemplate logic is clean and correct: compiled regex avoids repeated JIT cost, unknown placeholders are preserved as-is, ordinal comparison is enforced regardless of the caller's dictionary comparer.
• The GetOperatingSystemName fallback to RuntimeInformation.IsOSPlatform is compatible with all TFMs including net462 (no OperatingSystem.IsXxx() guards needed).
• Template values (pname, pid, etc.) are always populated with safe fallbacks ("unknown") preventing <placeholder> literal characters appearing in Windows file names.
• The integration tests HangDump_TemplateFileName_CreateDump and HangDump_TemplateFileNameWithSubdirectory_CreateDump provide good end-to-end coverage of template expansion and subdirectory creation.

Recommendations

1. Fix the StartsWith guard to use resultsDirectory + Path.DirectorySeparatorChar (or a TrimEnd-safe variant) to close the sibling-directory bypass.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-12
Repository: microsoft/testfx

Key Findings

1. [Coverage] Path-traversal rejection untested — guard has a bypass (HangDumpTests.cs line 168)
   The new StartsWith-based security check in TakeDumpAsync is neither unit-tested nor integration-tested. Worse, a sibling-directory attack (/tmp/results-evil/hang.dmp passes the /tmp/results prefix check) slips through because no trailing separator is appended before the comparison. A test would both catch the missing code path and expose the bypass.

2. [Coverage] Ordinal comparer normalization never exercised (ArtifactNamingHelperTests.cs line 92)
   The copy-to-ordinal-dict branch added in this commit is not reached by any test. A caller providing a StringComparer.OrdinalIgnoreCase dict with uppercase keys (e.g. ["PNAME"]) would see their key NOT matched against <pname>, but this behavior is currently invisible to the test suite.

3. [Structure] Misleading test name (ArtifactNamingHelperTests.cs line 30)
   WithCustomValues_OverridesCorrectly implies override-of-defaults semantics, but the template has no overridable defaults — _custom is just a literal string suffix.

Recommendations

• Fix the StartsWith path-traversal bypass by appending Path.DirectorySeparatorChar (or using Path.GetRelativePath + leading .. check) and add a test that exercises the rejection path.
• Add a test for the ordinal-copy path (non-ordinal dict + uppercase key).
• Rename ResolveTemplate_WithCustomValues_OverridesCorrectly to reflect actual intent.

---

Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Copilot]

Copilot's findings

• Files reviewed: 7/8 changed files
• Comments generated: 2

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-12
Repository: microsoft/testfx

Key Findings

The test additions in this revision are well-structured and comprehensive. All previously flagged issues have been resolved.

Unit tests (ArtifactNamingHelperTests.cs):

• Full coverage of ResolveTemplate: happy path, empty/null/whitespace template (throws), unknown placeholders preserved, case-sensitivity, repeated placeholders, null/empty replacements, IDictionary with non-ordinal comparer.
• GetOperatingSystemName is platform-anchored with explicit per-OS assertions.
• No isolation issues — fully stateless static method under test, no shared mutable state.
• Assert.ThrowsExactly<ArgumentException> is used correctly for input validation paths.

Integration tests (HangDumpTests.cs):

• HangDump_TemplateFileName_CreateDump: Verifies template expansion end-to-end with <pname>_<pid>_<tfm>_<time>_hang.dmp, using a well-formed regex and a targeted TFM presence check.
• HangDump_TemplateFileNameWithSubdirectory_CreateDump: Verifies <asm>/... expansion creates the expected subdirectory, with explicit Path.GetFileName assertions.
• Both new integration tests use DynamicData(AllForDynamicData) for cross-TFM coverage, Guid.NewGuid() result directories for isolation, and TestContext.CancellationToken for CI-safe execution — all consistent with repo conventions.
• SLEEPTIMEMS2 = "20000" leaves an ~8-second window before the second sleep completes; the hang dump fires at ~12 s (4 s + 8 s timeout), which is well within that window and also makes failures faster to diagnose (rather than waiting 10 minutes on a broken hang dump mechanism).

Recommendations

No action required. The test suite is reliable, isolated, and covers all meaningful branches.

---

Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-12
Repository: microsoft/testfx

Key Findings

✅ Previous Security Finding Fixed (Path Traversal): The trailing-separator bypass reported in the previous run (/tmp/results vs /tmp/results-evil) has been correctly addressed. Path.GetFullPath is now applied to both paths and resultsDirectoryGuard uses a trailing separator, closing the sibling-directory escape vector.

⚠️ [Correctness / Cross-TFM] Unknown placeholder tokens throw on Windows (HangDumpProcessLifetimeHandler.cs:333)
ResolveTemplate documents that unknown <placeholder> tokens are "preserved as-is", but on Windows < and > are illegal path characters. Path.GetFullPath throws ArgumentException: Illegal characters in path before reaching the user-friendly InvalidOperationException guard. On Linux the same template silently embeds <foo> literally in the filename. A user with a typo gets a cryptic internal error on Windows, a silent oddity on Linux — asymmetric, undocumented, and actionable-error-breaking behavior.

⚠️ [Correctness] Companion .log file inconsistency with subdirectory templates (HangDumpProcessLifetimeHandler.cs:353)
When a template like <asm>/<pname>_<pid>_hang.dmp places the dump in a subdirectory, the companion .log file (in-progress tests) is always written to the root results directory via _configuration.GetTestResultDirectory() + Path.GetFileName(finalDumpFileName). This means the dump and its log land in different places. The second GetTestResultDirectory() call also doesn't reuse the already-normalized resultsDirectory local variable.

Positive Observations

• The compiled Regex static field avoids per-call allocation. ✅
• Ordinal dictionary normalization pattern in ResolveTemplate correctly prevents case-insensitive dict bypass. ✅
• Path.GetFullPath + trailing-separator guard correctly neutralizes path traversal from both rooted paths and .. segments. ✅
• Directory.CreateDirectory before dump creation correctly handles subdirectory templates without an extra existence check. ✅
• RuntimeInformation.IsOSPlatform is available on all target frameworks (netstandard2.0, net462, net8.0+) — no cross-TFM issue. ✅
• Regex replacement is non-recursive (substituted values are not re-expanded), preventing injection through process name or assembly name values. ✅

Recommendations

1. Add a pre-Path.GetFullPath validation that the resolved pattern contains no remaining <...> tokens, with a clear error message listing supported placeholders.
2. Co-locate the .log file with the dump (Path.GetDirectoryName(finalDumpFileName)!) instead of always writing to the root results directory, and use the pre-computed resultsDirectory variable.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-12
Repository: microsoft/testfx

Key Findings

The new ArtifactNamingHelperTests.cs is thorough and covers the main scenarios well (null/empty/whitespace templates, case sensitivity, repeated placeholders, unknown placeholders, dictionary-type variants). The integration tests in HangDumpTests.cs follow established patterns (unique result directories via Guid.NewGuid(), TestContext.CancellationToken, Assert.ContainsSingle).

The following gaps were identified:

1. Untestable null-guard branch (value is not null in ArtifactNamingHelper.cs line 37) — never exercised because IDictionary<string, string> has non-nullable values. The guard should either be removed (trust the type annotation) or covered with a test using a custom IDictionary implementation that can return null.

2. Missing edge case: empty placeholder <> — the regex <([^>]+)> requires ≥1 character, so <> is preserved as-is, but no test documents or protects this behavior.

3. CaseInsensitiveDictionary test covers only the non-matching case — the complementary scenario (lowercase key in an OrdinalIgnoreCase dict correctly resolves <pname> after ordinal copy) is untested.

4. <os> placeholder not exercised in integration tests — all other five documented placeholders (<pname>, <pid>, <asm>, <tfm>, <time>) have integration-level coverage; <os> is tested only in unit isolation.

Recommendations

• Decide on the value is not null guard: either remove it (it's dead code with the current API) or add a targeted test with a custom IDictionary.
• Add a test for <> to pin the "empty placeholder is preserved" behavior.
• Add the complementary CaseInsensitiveDictionary test to cover both sides of the ordinal-copy path.
• Add <os> to one of the two new integration test templates to close the end-to-end coverage gap.

No isolation bugs, flakiness patterns, false-positive assertions, or test-order dependencies were found. The use of Guid.NewGuid() for unique result directories, TestContext.CancellationToken, and the [DynamicData] approach are all correct.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-12
Repository: microsoft/testfx

Key Findings

1. [Correctness — Medium] .log co-location breaks with subdirectory templates.
   When <asm>/<pname>_hang.dmp is used, the dump lands in results/TestAssembly/proc.dmp but the hanging-test list log still goes to results/proc.log (via Path.GetFileName). The companion files are now in different directories and the subdirectory integration test doesn't assert on the log placement.

2. [Defensive — Low] The InvalidOperationException error message for path-escape rejection embeds the fully-resolved dump path, which is derived from process.Name (OS-supplied) and the user-provided --hangdump-filename flag. In most environments this only surfaces in developer output, but an unbounded string from an adversarially-named process could produce very long messages.

Positive Observations

• Security guard is solid: Path.GetFullPath normalization + StartsWith(resultsDirectoryGuard, pathComparison) correctly catches rooted paths, .. traversal, and Windows vs. Unix case sensitivity. The sibling-directory bypass (/results vs. /results-evil) is explicitly prevented with the trailing-separator trick.
• Cross-TFM correctness: RuntimeInformation.IsOSPlatform is used throughout instead of OperatingSystem.IsXxx() — builds correctly on net462 / netstandard2.0.
• Exception handling: TakeDumpAsync is called inside a try/catch (Exception) at the call site, so the new InvalidOperationException from the path guard is caught and reported gracefully rather than crashing the handler.
• ResolveTemplate correctness: Single-pass Regex.Replace prevents re-injection through replacement values; the ordinal-comparer enforcement correctly uses the singleton reference check; unknown placeholders are preserved as documented.
• Backward compatibility: Legacy %p is handled after template resolution, maintaining existing behaviour for users who haven't migrated to the new <pid> placeholder.

Recommendations

1. Fix .log co-location: change Path.GetFileName(finalDumpFileName) to use finalDumpFileName directly so log and dump stay in the same directory regardless of template shape. (Inline comment left at the relevant line.)

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.