Add WSLC_SESSION env for the global --session option - #41468
Add WSLC_SESSION env for the global --session option#41468Feng Wang (chemwolf6922) wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for specifying the global --session option via an environment variable (WSLC_SESSION) so callers (e.g., Dev Containers) don’t need to repeat --session on every wslc command invocation.
Changes:
- Added an environment-variable binding from
WSLC_SESSIONtoArgType::Sessionand ensured env-derived values populate the global argument map. - Updated
RootCommandto advertiseSessionas an env-eligible argument alongsideNO_COLOR. - Added/updated unit tests to verify env application, absence behavior, and CLI
--sessionprecedence overWSLC_SESSION.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/windows/wslc/WSLCCLIEnvironmentOptionsUnitTests.cpp | Adds unit tests covering WSLC_SESSION application and precedence vs CLI --session. |
| test/windows/wslc/WSLCCLICommandUnitTests.cpp | Updates RootCommand env-argument shape test to include Session alongside NoColor. |
| src/windows/wslc/core/EnvironmentOptions.h | Adds WSLC_SESSION -> ArgType::Session env binding. |
| src/windows/wslc/commands/RootCommand.cpp | Includes Session in RootCommand::GetEnvArguments() so it’s eligible for env-based defaults. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
David Bennett (dkbennett)
left a comment
David Bennett (dkbennett)
left a comment
There was a problem hiding this comment.
I had already proposed adding this environment variable support for session, but this was explicitly rejected by the team. The primary reason is that it is easy to get into an unexpected state without more support in the CLI for handling this and making it visible to the user whenever session is being set by the environment. Also for no-color support even an empty environment variable is valid, which it is not for Session, leading to obscure errors if the environment variable is not completely removed.
At a minimum, we'd want to have a way to easily reset the environment variable (delete it) from the CLI, and possibly issue diagnostic output whenever session is being set by the environment so it is obvious to the user which session is being used. We just need more careful environment variable handling, validation, and information whenever environment variable is being used for session targeting for this to be a good user experience.
So I'm marking this as "Request Changes" but realistically this PR must unfortunately be rejected, because not having it is a design decision at this time. We may revisit this in the future as we get customer feedback, but for now session is command line specified only by-design.
|
Hi David Bennett (@dkbennett) . I made the suggested changes and converted this into a draft for now. This was discussed with Craig Loewen (@craigloewen-msft) and Pooja Trivedi (@ptrivedi) to support the Dev Containers attach scenario. But has not been discussed with the broader team yet. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/windows/wslc/core/EnvironmentOptions.cpp:50
ApplyEnvironmentOptions()currently copies the environment variable value intoArgMap(target.Add(arg.Type(), *value)) only so it can still be logged afterwards. This adds an avoidable allocation/copy for value-kind args (notablyWSLC_SESSION). You can log first and then move the string intoArgMapto keep the log intact while preserving the original move-only behavior.
target.Add(arg.Type(), *value);
David Bennett (dkbennett)
left a comment
David Bennett (dkbennett)
left a comment
There was a problem hiding this comment.
Changes look good, the empty value boolean is a good addition. If we get a third behavior type would want to make that an enum but easy change to make later.
Good test with making sure CLI overrides the environment variable.
One thing I would question is the error case of a non-existent session being specified in an environment variable and it may not be visible to the user.
If session is set by environment variable and that session does not exist, what do we do?
In the --session does-not-exist case, it's a clear argument error on the command line that should be obvious to the user. They specified a session that does not exist.
In the environment variable case, it may not be obvious to the user that the session specified in the environment is invalid. The user may have set it and forgotten about it and be confused why the session doesn't exist. Or a script set it and they are not aware of it at all. Such environment variables will also span updates to the WSL install, so it seems plausible that this could be encountered by any user who sets the environment variable for a specific session or has a script or tool or app that does it.
One option here is Argument could have an additional member variable added - the value's Source. I'm expecting this could be an enum with values such as "Default" (if the value's default setting was used) or "CommandLine" or "Environment" or "Policy". Any call to Add a value to an argument could pass along the Source, then we could differentiate behavior based on the source. For example, when resolving the Session, we could check on error if the source is Environment, and if so display a warning or different error message to the user indicating that the environment variable for session is pointing to an invalid session. Its possible with policy support later that some settings might be policy-controlled so we could have special errors or warnings for that as well (such as not allowing a value set by Policy to be overridden by anything).
This is a bigger change to support a feature like this, but it is one I would want to make if session environment variable is supported so we have clear user messaging when the environment variable is invalid.
A "system session reset/clear/default" command may also be useful that removes any environment variables set as an easy way to get back into a working state if a user encounters this situation.
changes have been addressed
There was a problem hiding this comment.
🟡 Changes recommended
There is at least one concrete, fixable issue (an avoidable string copy in env option application) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
| else if (arg.Kind() == Kind::Value) | ||
| { | ||
| target.Add(arg.Type(), std::move(*value)); | ||
| target.Add(arg.Type(), *value, argument::ArgumentValueSource::Environment); | ||
| } |
Summary of the Pull Request
Currently to work with an application session, the users need to prepend the --session option to every command they call. This is especially complex for adding the "attach to application session containers" feature to Dev Containers.
This PR adds a WSLC_SESSION environment for the --session option. When it's present and the --session option is not, the command uses its value for the --session option.
Please check internal debug experience docs for details.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
Update / add tests:
WSLCCLICommandUnitTests::WSLCCLICommandUnitTests::RootCommand_EnvArguments_NoColorAndSession
WSLCCLIEnvironmentOptionsUnitTests::WSLCCLIEnvironmentOptionsUnitTests::ApplyEnvironmentOptions_Session_SetsValue
WSLCCLIEnvironmentOptionsUnitTests::WSLCCLIEnvironmentOptionsUnitTests::ApplyEnvironmentOptions_SessionAbsent_DoesNotSetValue
WSLCCLIEnvironmentOptionsUnitTests::WSLCCLIEnvironmentOptionsUnitTests::ApplyEnvironmentOptions_CliSessionOverridesEnvironmentSession