RFC 014 - Command-line option mappings

[Evangelink]

Adds RFC 014 proposing command-line option mappings: a new extensibility point that lets an extension declaratively accept a user-facing option (e.g. --logger trx, --collect ""XPlat Code Coverage"") and rewrite it, at parse time, into one or more first-class MTP options.

Why

The primary scenario is easing migration from VSTest to MTP. Today, dotnet test --logger trx --collect ""XPlat Code Coverage"" produces Unknown option against MTP — correct but unhelpful for the very large body of CI pipelines and tribal knowledge built around VSTest syntax.

The existing ICommandLineOptionsProvider contract intentionally forbids two extensions from registering the same option name. --logger and --collect are however intrinsically polyvalent: the value selects which extension owns the option. A new concept is therefore needed.

What's in the RFC

• Naming rationale (Mapping over Alias/Transformation/Shim/Legacy…), explicitly addressing the alias-vs-rewrite discussion from [Proposal]: MTP Command-line aliases #7249.
• Public API surface: ICommandLineOptionMappingProvider, CommandLineOptionMapping, CommandLineOptionMapper delegate (TryHandle style), CommandLineOptionMappingResult.
• Resolution algorithm: mappings run after response-file expansion, before validation. Zero claimants → error. Multiple claimants → error. Exactly one → rewrite.
• Worked examples for TRX (including trx;LogFileName=…) and Code Coverage.
• Cross-cutting validation: mappings cannot collide with canonical option names, cannot rewrite to unknown options, cannot rewrite to other mappings (no loops).
• Compatibility analysis, 5 rejected alternatives, 5-phase rollout, 5 unresolved questions.

Discussion seeds

Specific points where reviewer input would be most valuable:

1. Naming — keep Mapping, or switch back to Alias (familiar) or Transformation (most accurate)?
2. Help layout — separate ""VSTest-style options"" section, or interleave with canonical options?
3. Warning channel — should TryMap get an out List<string> warnings parameter in v1, or defer?
4. Telemetry — log only used_compat_mapping: bool, or include allowlisted values?

Refs #7249 — this PR provides the design artefact requested by the issue; implementation will follow in a separate PR once the RFC is approved.

[Copilot]

Pull request overview

Adds a new design RFC to the docs/RFCs area proposing command-line option mappings for Microsoft.Testing.Platform (MTP): an extensibility point allowing VSTest-style options like --logger trx / --collect ... to be rewritten into canonical MTP options during parsing to ease migration.

Changes:

• Introduces an RFC describing the motivation, naming, proposed public API, and resolution/validation algorithm for command-line option mappings.
• Provides worked examples (TRX and code coverage) and outlines help/info/telemetry implications plus a phased rollout plan.

Show a summary per file

File	Description
docs/RFCs/014-Command-Line-Option-Mappings.md	New RFC document defining the proposed “command-line option mappings” concept, API surface, and behavior.

Copilot's findings

• Files reviewed: 1/1 changed files
• Comments generated: 5

[Evangelink]

RFC 014 Review Summary

I've reviewed RFC 014 - Command-line option mappings against 21 expert review dimensions. The RFC proposes a well-motivated solution to a real migration pain point (VSTest → MTP compatibility), with strong design principles and thorough alternatives analysis. However, critical issues in the proposed API design require changes before approval.

Verdict Table

#	Dimension	Verdict
1	Public API & Binary Compatibility	🔴 1 BLOCKING, 2 MODERATE
2	Defensive Coding at Boundaries	🔴 2 BLOCKING, 2 MAJOR
3	Algorithmic Correctness	🟡 2 MAJOR
4	Naming & Conventions	⚪ 2 NIT
5	Documentation Accuracy	⚪ 1 NIT
6	Performance & Allocations	🟡 1 MODERATE (covered above)

✅ 15/21 dimensions clean.

---

Critical Issues (Must Fix Before Approval)

🔴 BLOCKING #1: Delegate signature prevents future evolution and lacks safety

Location: Lines 117-119 (CommandLineOptionMapper delegate)

Problems:

1. Cannot add warnings later: RFC acknowledges (line 350) wanting to add warnings, but adding an out parameter to a delegate is a breaking change
2. No exception handling: Extension throws → platform crash during startup
3. No result validation: Extension returns true with result = null → NullReferenceException
4. No timeout: Extension infinite loop → platform hangs indefinitely

Fix: Replace bool return + out parameter with a result struct that can carry warnings in future versions:

public delegate CommandLineOptionMapperResult CommandLineOptionMapper(
    ReadOnlySpan<string> arguments);

public readonly struct CommandLineOptionMapperResult
{
    public bool Success { get; init; }
    public IReadOnlyList<CommandLineOptionMappingResult> Results { get; init; }
    // Future: public IReadOnlyList<string> Warnings { get; init; }
}

Add defensive wrapping (try/catch, validation, timeout) to the RFC's algorithm section.

🔴 BLOCKING #2: Naming inconsistency in manager pattern

Location: Lines 141, 149

Problem: ICommandLineMappingsManager uses plural "Mappings", breaking the universal MTP pattern: ICommandLineManager, ITestHostManager, IConfigurationManager, ILoggingManager (all singular).

Fix: Rename to ICommandLineMappingManager (singular). Update property name to CommandLineMapping to match the TestHost / ITestHostManager pattern.

---

Major Issues (Should Fix)

🟡 Algorithmic correctness gaps (2 issues)

1. Empty result list undefined (line 246): What if TryMap returns true with empty results? Does occurrence get deleted?
2. Runtime validation missing (line 258): Validation rules claim "enforced at startup" but can't detect invalid OptionName until runtime

Fix: Specify behavior for empty results (error recommended). Add runtime validation step in algorithm.

🟡 Defensive coding gaps (2 more issues beyond BLOCKING)

1. Unbounded growth (line 192): Example code splits on ; with no limits → 100K segments from malicious input
2. Malformed sub-options (line 194): No guidance on handling trx;LogFileName= (empty value)

Fix: Add defensive limits to examples (max segments, length checks). Document error handling approach for malformed sub-options.

---

Moderate Issues (Recommended Fixes)

Constructor design (line 126)

1. params blocks future evolution: Can't add optional parameters after params array
2. Unnecessary allocation: params creates array wrapper even for zero-argument cases

Fix: Add non-params overload for zero-argument case and explicit IReadOnlyList<string> overload.

---

Summary

Strong points:

• Clear problem statement and motivation
• Thorough alternatives analysis (5 rejected options)
• Well-defined design principles
• Comprehensive phasing plan
• Threading/concurrency design is sound (synchronous, pre-DI, single-threaded startup)

Must address before approval:

• Delegate signature evolution strategy (warnings, safety)
• Naming consistency with existing patterns
• Algorithm specification completeness (empty results, runtime validation)
• Defensive coding guidance (limits, error handling)

The core concept is sound and valuable. The issues are fixable refinements to the API design, not fundamental flaws. Recommend addressing BLOCKING issues and reconsidering the MAJOR issues before moving to implementation.

---

Review dimensions not applicable to RFC: Test Isolation, Assertion Quality, Flakiness Patterns, Data-Driven Test Coverage, Analyzer Quality, IPC Wire Compatibility, Resource Management, Localization, Build Infrastructure, Cross-TFM Compatibility (these apply to implementation, not design docs).

Generated by Expert Code Review (on open) for issue #8501 · ● 22.6M