Refactor wsladiag CLI parsing and fix shell PTY handling #13965

beena352 · 2025-12-23T01:48:12Z

Summary of the Pull Request:
This PR refactors wsladiag command-line argument handling to use per-verb parsing and fixes multiple correctness and robustness issues in the interactive shell (PTY) path. It improves live terminal resize behavior, simplifies process lifecycle handling, and makes console cleanup more diagnosable, while preserving existing behavior by default.

Detailed Description:
Argument parsing

Refactors argument handling to be per-verb (list, shell), allowing each command to define and validate its own arguments.

Adds an opt-in relaxed mode to ArgumentParser to allow top-level verb dispatch without throwing on subcommand arguments. The default parsing behavior remains unchanged.

This makes the CLI easier to extend as additional wsladiag commands are added.

Shell / PTY handling

Fixes TTY size initialization by passing rows and columns in the correct order.

Restores and correctly handles live terminal resize using the TerminalControl FD.

Validation Steps Performed

Ran wsladiag list to verify session enumeration.

Ran wsladiag shell to verify interactive shell launch.

Verified interactive input/output behavior inside the shell.

Verified live terminal resize by resizing the console window while running commands such as top.

Verified clean process exit without hangs and correct console state restoration.

OneBlue · 2025-12-23T01:53:26Z

src/windows/wsladiag/wsladiag.cpp

+    }
+    catch (...)
+    {
+        const auto hr = wil::ResultFromCaughtException();


I'd recommend letting the exception go up and handle the error at the main() level, so we can have one error handler for all arguments

OneBlue · 2025-12-23T01:54:10Z

src/windows/wsladiag/wsladiag.cpp

+        throw;
+    }
+
+    if (sessionName.empty())


We should throw an error here since that sessionName being empty would mean that the command line was invalid

OneBlue · 2025-12-23T01:54:42Z

src/windows/wsladiag/wsladiag.cpp

-    wsl::shared::SocketChannel controlChannel{wil::unique_socket{(SOCKET)ttyControl.release()}, "TerminalControl", exitEvent.get()};
+
+    std::optional<wsl::shared::SocketChannel> controlChannel;
+    try


What's the reason for adding a try/catch here ?

there isn’t a strong reason to special-case this. I’ve removed the try/catch and the optional and now treat TerminalControl as required; any failure will propagate and be handled by the existing top-level error handling.

OneBlue · 2025-12-23T01:55:30Z

src/windows/wsladiag/wsladiag.cpp

    wsl::windows::common::relay::InterruptableRelay(ttyOut.get(), consoleOut, exitEvent.get());

+    // Output relay finished - signal input thread to stop and wait for process exit
+    exitEvent.SetEvent();


what's the reason for adding this ?

Yeah, the input relay is already stopped and joined via the existing scope_exit cleanup, so this extra signal was redundant. I’ve removed it and kept a single shutdown path.

OneBlue · 2025-12-23T01:57:04Z

src/shared/inc/CommandLine.h

 #else

-    ArgumentParser(int argc, const char* const* argv) : m_argc(argc), m_argv(argv), m_startIndex(1)
+    ArgumentParser(int argc, const char* const* argv, bool IgnoreUnknownArgs = false) :


nit: I'd recommend naming this argument something like stopOnUnknownArgs

Let's also rename the constructor argument to match the same name

Also stopUnknownArgs -> stopOnUnknownArgs

OneBlue · 2025-12-24T02:34:29Z

src/windows/wsladiag/wsladiag.cpp

        WSLA_TERMINAL_CHANGED message{};
-        message.Columns = infoEx.srWindow.Right - infoEx.srWindow.Left + 1;
-        message.Rows = infoEx.srWindow.Bottom - infoEx.srWindow.Top + 1;
+        message.Columns = static_cast<unsigned short>(infoEx.srWindow.Right - infoEx.srWindow.Left + 1);


What's the reason for this change ?

OneBlue · 2025-12-31T23:30:21Z

src/windows/wsladiag/wsladiag.cpp

    catch (...)
    {
        const auto hr = wil::ResultFromCaughtException();
+        if (hr == E_INVALIDARG)


E_INVALIDARG can be thrown for other reasons for I would not recommend having a specific catch block here.

Once we have ExecutionContext based errors, we can use those to show command line parsing specific errors

Copilot

Pull request overview

This PR refactors wsladiag's command-line argument parsing to use per-verb parsing, enabling each subcommand (list, shell) to define and validate its own arguments independently. It also fixes several PTY-related issues in the interactive shell path, including TTY size initialization, live terminal resize handling, and console cleanup diagnostics.

Key changes:

Introduces a relaxed parsing mode in ArgumentParser via the stopUnknownArgs parameter to allow top-level verb dispatch without throwing on subcommand arguments
Fixes TTY size initialization by passing rows and columns in the correct order and restores live terminal resize functionality through the TerminalControl FD
Improves console cleanup by using scope_exit_log and logging failures for better diagnostics

Reviewed changes

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

File	Description
src/windows/wsladiag/wsladiag.cpp	Refactors RunShellCommand to parse its own arguments, moves ttyControl initialization closer to use, improves console cleanup logging, updates Abstract comment to reflect verb-based parsing
src/shared/inc/CommandLine.h	Adds `stopUnknownArgs` parameter to ArgumentParser constructors to enable relaxed parsing mode, fixes typo in comment ("arvg" → "argv")

Copilot · 2026-01-02T23:13:34Z

src/windows/wsladiag/wsladiag.cpp

+    ArgumentParser parser(std::wstring{commandLine}, L"wsladiag", 2); // Skip "wsladiag.exe shell" to parse shell-specific args
+    parser.AddPositionalArgument(sessionName, 0);
+    parser.AddArgument(verbose, L"--verbose", L'v');
+
+    parser.Parse();
+
+    if (sessionName.empty())
+    {
+        THROW_HR(E_INVALIDARG);
+    }


The ArgumentParser.Parse() call here will throw THROW_HR_WITH_USER_ERROR with E_INVALIDARG if parsing fails, which sets a user-friendly error message in the ExecutionContext. However, wsladiag doesn't use ExecutionContext (unlike wsl.exe which does), so when the exception propagates to wmain and ReportError is called, only the HRESULT will be shown using ErrorCodeToString, and the user-friendly error message will be lost. This means users won't see helpful messages like "Missing argument" or "Invalid command line" - they'll just see "0x80070057" (E_INVALIDARG).

Consider either: 1) wrapping the Parse() call in a try-catch to call PrintUsage() on E_INVALIDARG, or 2) setting up an ExecutionContext in wsladiag_main so error messages are captured, or 3) using a simpler error handling approach that doesn't rely on THROW_HR_WITH_USER_ERROR.

Copilot · 2026-01-02T23:13:34Z

src/shared/inc/CommandLine.h

+    ArgumentParser(const std::wstring& CommandLine, LPCWSTR Name, int StartIndex = 1, bool stopUnknownArgs = false) :
+        m_startIndex(StartIndex), m_name(Name), m_stopUnknownArgs(stopUnknownArgs)


The parameter name "stopUnknownArgs" is unclear and potentially confusing. The behavior is to stop parsing (break out of the loop) when an unknown argument is encountered, rather than throwing an error. A clearer name would be "ignoreUnknownArgs" or "allowUnknownArgs" to better convey that unknown arguments are tolerated rather than causing an error.

OneBlue reviewed Dec 23, 2025

View reviewed changes

OneBlue reviewed Dec 24, 2025

View reviewed changes

beena352 force-pushed the user/beenachauhan/wsladiag-verb-parsing branch 2 times, most recently from 7a264c4 to a7841e2 Compare December 30, 2025 20:05

OneBlue reviewed Dec 31, 2025

View reviewed changes

beena352 added 3 commits January 2, 2026 12:00

Refactor wsladiag CLI parsing and fix shell PTY handling

8cf28f6

Rename ArgumentParser flag to stopOnUnknownArgs

5ec6d87

Rename ArgumentParser constructor flag to stopOnUnknownArgs

dbc865f

beena352 force-pushed the user/beenachauhan/wsladiag-verb-parsing branch from a7841e2 to dbc865f Compare January 2, 2026 20:05

Remove E_INVALIDARG special-casing in wmain error handling

2700c39

beena352 marked this pull request as ready for review January 2, 2026 23:08

beena352 requested a review from a team as a code owner January 2, 2026 23:08

Copilot AI review requested due to automatic review settings January 2, 2026 23:08

Copilot started reviewing on behalf of beena352 January 2, 2026 23:08 View session

Copilot AI reviewed Jan 2, 2026

View reviewed changes

		ArgumentParser(const std::wstring& CommandLine, LPCWSTR Name, int StartIndex = 1, bool stopUnknownArgs = false) :
		m_startIndex(StartIndex), m_name(Name), m_stopUnknownArgs(stopUnknownArgs)

Refactor wsladiag CLI parsing and fix shell PTY handling #13965

Are you sure you want to change the base?

Refactor wsladiag CLI parsing and fix shell PTY handling #13965

Conversation

beena352 commented Dec 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants