CLI: Cache converted CLI arguments with on-demand revalidation, and unify typed value accessors

[dkbennett]

Summary of the Pull Request

Argument validation in the WSLC CLI converts string inputs into typed values (memory sizes, durations, signals, timestamps, labels, and so on). Until now that conversion ran twice for each argument: once while parsing and validating the command line, and again during execution when a task re-parsed the raw string. Beyond the wasted work, the two conversions could drift apart, and some arguments validated their input late, in the middle of execution, so bad input failed after side effects had already started. This PR validates and converts each argument as needed and caches the typed result on ArgMap, so execution reuses the cached value instead of re-parsing. These are CLI-only changes: no WSLC API or runtime changes.

The cache is memoized rather than one-shot: the typed value is reused until an argument's raw values change, and it is re-derived on demand the next time the argument is read. This keeps validation in one place while supporting commands that legitimately add or change arguments during execution (for example, the Registry commands, which gather input from the user mid-run). A value added after the up-front pass is validated on first read, before it is used, so late input can no longer slip through unchecked. Validate-only arguments (those with no converted value) are re-checked on demand the same way, so an out-of-range or malformed value added late is still rejected.

The single-source-of-truth model is backed by compile-time type safety. Each argument declares its converted type in one place (the WSLC_ARGUMENTS X-macro), and the cache accessors derive their types from that declaration. A read that disagrees with the declared type, or an attempt to cache an argument that has no converted type, is now a compile error rather than a runtime surprise. This makes the "convert in the right place" rule something the compiler enforces instead of a convention contributors have to remember.

To keep call sites simple and consistent, value access is unified behind accessors that mirror the recently added GetFlag semantics, so callers no longer need to know whether a given argument is converted or raw. Finally, the change removes the ability to reach the old double-conversion pattern at all: the execution-path converters are no longer visible to task and command code, so anyone working from the existing code only has the correct pattern to copy, and the wrong one will not compile.

PR Checklist

• Closes: Link to issue #xxx
• Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
• Tests: Added/updated if needed and all pass
• Localization: All end user facing strings can be localized
• Dev docs: Added/updated if needed
• Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

• Validate-and-cache: Argument::Validate converts each raw string and stores the typed value on the ArgMap. Execution reads the cached value instead of re-parsing, and the cached value is reused until the argument's raw values change.
• On-demand revalidation for arguments added or changed during execution: adding or removing a raw value invalidates that argument's cached entry (via a map-action callback), so the next read re-validates it. This covers commands that add arguments mid-run (for example, the Registry commands), and it applies to validate-only arguments too, so a value added after the up-front validation pass is still range and format checked before use.
• ArgMap moved into its own header. It had grown well beyond the enums and type mappings it started next to, so it now lives in ArgMap.h; ArgumentTypes.h keeps only the argument enums and type mappings that ArgMap is built on. ArgMap.h includes ArgumentTypes.h, code that uses the container includes ArgMap.h (usually indirectly through CLIExecutionContext), and code that needs only the types keeps including ArgumentTypes.h.
• New ArgumentConvertedTypes.h types-only header: exposes the converted-type aliases and the ArgConvertedTypeMapping specializations that back the accessors, without pulling in the validation:: converter functions. Task and command code includes this header (instead of ArgumentValidation.h) to read converted values via GetValue/GetAllValues, keeping the converters confined to the validation layer.
• Strong type safety, enforced at compile time: each argument declares its converted type in the WSLC_ARGUMENTS X-macro (single source of truth). The read and write cache accessors derive their type from that declaration, so a wrong-type access is a compile error, not a runtime failure. Arguments without a converted type map to a NoConversion sentinel and cannot touch the cache (also a compile error).
• Unified value accessors consistent with GetFlag semantics. Three-way split:
  • GetFlag<E>() for Kind::Flag.
  • GetValue<E>() / GetAllValues<E>() for value, positional, and forward arguments. These dispatch at compile time: converted arguments return the cached typed value, non-converted arguments return the raw value. Callers do not need to know which. Using them on a flag is a compile error (use GetFlag); using GetFlag on a value arg is a compile error too.
• Folded three misplaced conversions into the cache. Label, Options, and Type previously converted at execution, bypassing the cache and validating late (mid-execution). They are now proper converted arguments (Label/Options -> key/value pair, Type -> InspectType), so bad input is rejected during validation.
• Regression guard via include hygiene. ArgumentValidation.h is no longer included by task/command files, so the old pattern (calling a validation:: converter directly at execution) no longer compiles. Files that need only the converted types include a types-only header instead.

Validation Steps Performed

• Added coverage that every converted argument type converts and caches during validation, and, driven by the same table, that reading a converted argument with no prior validation pass validates it on demand and returns the same typed value.
• Added on-demand scenarios: an invalid value read with no prior validation pass throws during the read and caches nothing; an argument added after the up-front pass is re-validated on the next read (both converted and validate-only shapes); repeated arguments cache every value in insertion order.
• Verified that validate-only arguments never populate the converted cache, and that invalid input throws during validation.
• The WSLCCLI unit suite passes, including the new on-demand and runtime-add coverage in WSLCCLIArgumentUnitTests.

[Copilot]

Pull request overview

This PR refactors the WSLC CLI argument pipeline so string-to-typed conversions happen exactly once during Argument::Validate, then execution reuses cached typed values via unified ArgMap accessors (GetValue / GetAllValues). This aims to eliminate double-conversion drift, fail fast on invalid input before side effects, and make type mismatches compile-time errors via an ArgType -> ConvertedType mapping.

Changes:

• Add a validated-value cache to ArgMap and route per-argument validation through a single conversion+cache path in Argument::Validate.
• Introduce ArgumentConvertedTypes.h (types/mapping only) and update tasks/commands to read arguments via GetValue / GetAllValues instead of re-parsing raw strings at execution.
• Expand unit test coverage to assert caching behavior for all converted arguments (including newly converted Type, Label, and Options) and update X-macro consumers for the new ConvertedType parameter.

Reviewed changes

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

Show a summary per file

File	Description
test/windows/wslc/WSLCCLICommandUnitTests.cpp	Updates WSLC_ARGUMENTS X-macro consumer to account for the new ConvertedType column.
test/windows/wslc/WSLCCLIArgumentUnitTests.cpp	Adds coverage for the validated cache and ensures converted args are cached during validation.
src/windows/wslc/tasks/VolumeTasks.cpp	Switches execution reads to GetValue/GetAllValues and consumes cached parsed key/value options/labels.
src/windows/wslc/tasks/SessionTasks.cpp	Switches execution reads to GetValue for raw/forward args without re-parsing.
src/windows/wslc/tasks/RegistryTasks.cpp	Switches execution reads to GetValue for credentials/server args.
src/windows/wslc/tasks/NetworkTasks.cpp	Switches execution reads to GetValue/GetAllValues and consumes cached key/value options/labels/filters.
src/windows/wslc/tasks/InspectTasks.cpp	Switches --type consumption to cached InspectType via GetValue.
src/windows/wslc/tasks/ImageTasks.cpp	Switches common reads to GetValue/GetAllValues and consumes cached filters; keeps labels raw for build API.
src/windows/wslc/tasks/ContainerTasks.cpp	Switches execution reads to GetValue/GetAllValues for converted types (signals, durations, sizes, ulimit, filters, labels).
src/windows/wslc/core/Command.h	Documents the post-Argument::Validate internal validation hook contract.
src/windows/wslc/commands/VersionCommand.cpp	Switches --format reads to cached FormatType via GetValue.
src/windows/wslc/commands/ContainerAttachCommand.cpp	Switches positional container id read to GetValue.
src/windows/wslc/arguments/ArgumentValidation.h	Includes converted-type mapping header so validation can cache typed results.
src/windows/wslc/arguments/ArgumentValidation.cpp	Centralizes conversion+cache logic (CacheConverted) and moves late conversions (Label/Options/Type) into validation.
src/windows/wslc/arguments/ArgumentTypes.h	Adds validated cache storage and unified typed accessors (AddValidated, GetValue, GetAllValues).
src/windows/wslc/arguments/ArgumentDefinitions.h	Extends WSLC_ARGUMENTS with ConvertedType as the single source of truth for typed conversion.
src/windows/wslc/arguments/ArgumentConvertedTypes.h	New header defining ArgType -> ConvertedType mapping and required type aliases.
src/windows/wslc/arguments/Argument.cpp	Updates X-macro construction helper to accept the new ConvertedType parameter.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/windows/wslc/tasks/ContainerTasks.cpp:777

• GetAllValues() returns a std::vector by value, so envArgs cannot be a reference. This is undefined behavior when iterating envArgs below.

    if (context.Args.Contains(ArgType::Env))
    {
        auto const& envArgs = context.Args.GetAllValues<ArgType::Env>();
        for (const auto& arg : envArgs)

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

localization/strings/en-US/Resources.resw:3202

• This PR removes the WSLCCLI_InvalidFormatError resource, but invalid --format values still need a localized, consistent error message. Consider restoring a localized invalid-format string (with the correct locking/placeholder comment format) and updating the validation path to use it, rather than emitting an English-only formatted string.

  <data name="WSLCCLI_SessionListVerboseArgDescription" xml:space="preserve">
    <value>Show detailed information about the listed sessions.</value>
  </data>
  <data name="WSLCCLI_InvalidInspectError" xml:space="preserve">
    <value>Invalid {} value: {} is not a recognized inspect type. Supported inspect types are: {}.</value>
    <comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
  </data>

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/windows/wslc/arguments/ArgumentValidation.cpp:70

• --format validation now relies on GetFormatTypeFromString, which throws a hard-coded (non-localized) std::format message on invalid input. Since command-specific localized validation was removed, invalid format values will surface this non-localized text to users. Wrap the converter here to throw a localized ArgumentException instead (and keep/restore the corresponding resw string).

    case ArgType::Format:
        CacheConverted<ArgType::Format>(execArgs, m_name, validation::GetFormatTypeFromString);
        break;

[Copilot]

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/windows/wslc/arguments/ArgumentTypes.h:264

• GetAllValidated() copies each cached converted value into a new std::vector. For large converted types (notably services::BuildSecret, which owns a potentially large Value byte buffer), this can double peak memory at execution time (cache + returned vector) and add avoidable memcpy cost.

Consider adding a non-copying accessor for converted values (e.g., an iterator range/view over the cache) or a move-out API (e.g., TakeAllValuesArgType::Secret() that moves from the cache and erases entries) so call sites like ImageTasks can avoid duplicating secret payloads.

    // Branch helper for GetAllValues's converted path. Private so callers go through GetAllValues.
    template <ArgType E>
    std::vector<typename details::ArgConvertedTypeMapping<E>::value_t> GetAllValidated() const
    {
        using value_t = typename details::ArgConvertedTypeMapping<E>::value_t;
        static_assert(
            !std::is_same_v<value_t, details::NoConversion>,
            "This argument has no converted type (NoConversion); it cannot be read from the cache. "
            "Declare its ConvertedType in ArgumentDefinitions.h to enable caching.");

        // Debug canary: see GetValidated. The cached count must still match the raw argument count.
        WI_ASSERT_MSG(Count(E) == CountValidated(E), "validated cache is stale: argument count does not match validated count");

        std::vector<value_t> results;
        auto range = m_validated.equal_range(E);
        for (auto it = range.first; it != range.second; ++it)
        {
            // See GetValidated: any_cast cannot fail for a correctly populated cache.
            const value_t* value = std::any_cast<value_t>(&it->second);
            WI_ASSERT_MSG(value != nullptr, "validated cache holds the wrong type for this argument");
            results.push_back(*value);
        }

        return results;
    }

[Copilot]

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/windows/wslc/tasks/ImageTasks.cpp:104

• BuildImage still reads labels via GetAllArgType::Label() (raw) without forcing validation. Since ArgType::Label is now a converted/validated argument and the ArgMap cache can be invalidated by post-validation mutations, this bypasses the on-demand validation/caching path and can allow newly-added/overwritten labels to slip through unvalidated. Trigger validation via the unified accessor before consuming the raw strings (or switch the service API to accept the converted KeyValuePair type).

    auto tags = context.Args.GetAllValues<ArgType::Tag>();
    auto buildArgs = context.Args.GetAllValues<ArgType::BuildArg>();
    // Labels are validated during argument validation; the build API consumes the raw strings.
    auto labels = context.Args.GetAll<ArgType::Label>();

src/windows/wslc/arguments/ArgumentValidation.cpp:54

• CacheConverted appends converted values to the validated cache without clearing any existing entries for that ArgType and without ensuring the cache is empty on failure. If an argument is validated more than once (or if one of multiple values throws mid-conversion), the cache can contain duplicate/partial results, violating the “throws during validation and caches nothing” invariant and potentially returning stale data.

        for (const auto& value : execArgs.GetAll<A>())
        {
            execArgs.AddValidated<A>(convert(value, argName));
        }

        // Sanity check: each raw value for this argument must produce exactly one cached value.
        WI_ASSERT(execArgs.CountValidated(A) == execArgs.Count(A));
    }

[Copilot]

Pull request overview

Copilot reviewed 34 out of 34 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/windows/wslc/arguments/ArgMap.h:209

• GetAllValidated() appends to results without reserving capacity. For large/variable-size converted types (notably services::BuildSecret which can carry large Value buffers), this can cause repeated reallocations and copies. Reserve using the known count up front to keep this path O(n) with minimal allocations.

        std::vector<value_t> results;
        auto range = m_validated.equal_range(E);
        for (auto it = range.first; it != range.second; ++it)
        {
            // See GetValidated: any_cast cannot fail for a correctly populated cache.
            const value_t* value = std::any_cast<value_t>(&it->second);
            WI_ASSERT_MSG(value != nullptr, "validated cache holds the wrong type for this argument");
            results.push_back(*value);