Adds ability to set Contour as default terminal on Windows OS

[christianparpart]

closes #605

[Copilot]

Pull request overview

This pull request adds support for setting Contour as the default terminal on Windows 11 by implementing the Windows Terminal Handoff protocol. This allows Contour to be registered as a COM server that Windows can invoke when launching console applications.

Key changes:

• Implements the ITerminalHandoff3 COM interface for terminal handoff protocol
• Creates HandoffPty class to manage PTY communication via named pipes
• Registers Contour as a COM LocalServer in the Windows Registry via WiX installer

Reviewed changes

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

Show a summary per file

File	Description
src/vtpty/HandoffPty.h	Defines HandoffPty class for managing handed-off PTY connections using Windows named pipes
src/vtpty/HandoffPty.cpp	Implements HandoffPty with overlapped I/O for reading/writing to named pipes
src/vtpty/CMakeLists.txt	Adds HandoffPty source files to Windows build
src/contour/windows/ITerminalHandoff.h	Defines ITerminalHandoff3 COM interface and TERMINAL_STARTUP_INFO structure
src/contour/windows/TerminalHandoff.h	Declares COM classes for terminal handoff (TerminalHandoff and TerminalHandoffFactory)
src/contour/windows/TerminalHandoff.cpp	Implements COM server for terminal handoff, creates named pipes and dispatches to app
src/contour/TerminalSessionManager.h	Adds createSessionWithPty method to create sessions with custom PTY instances
src/contour/TerminalSessionManager.cpp	Implements createSessionWithPty to support handoff scenarios
src/contour/ContourGuiApp.h	Declares newWindowWithHandoff method for handling COM handoff requests
src/contour/ContourGuiApp.cpp	Implements COM server registration, -Embedding flag handling, and handoff window creation
src/contour/CMakeLists.txt	Adds Windows-specific terminal handoff source files to build
src/contour/wix_patch.xml	Registers Contour's CLSID in Windows Registry for COM LocalServer
metainfo.xml	Documents the new Windows 11 default terminal feature

Comments suppressed due to low confidence (1)

src/vtpty/CMakeLists.txt:65

• By adding HandoffPty.cpp to the Windows build here, you are enabling a PTY implementation whose HandoffPty::read method can overflow the crispy::buffer_object<char> passed in because it reads up to _ptyReadBufferSize bytes from the pipe and then calls storage.advance(bytesRead) without checking storage.bytesAvailable(). Once _currentPtyBuffer->bytesAvailable() drops below the configured read buffer size (which is typical as the buffer fills), a local COM client that writes data into the handoff pipe can cause storage.advance(bytesRead) and the subsequent std::copy in HandoffPty::read to write past the end of the buffer, leading to memory corruption in the Contour process. Before shipping this, the HandoffPty::read implementation should be hardened to cap the read length by the destination buffer’s remaining capacity (similar to the existing ConPty, UnixPty and SshSession implementations) so that enabling it here does not introduce a memory-safety vulnerability.

    list(APPEND vtpty_SOURCES ConPty.cpp HandoffPty.cpp)
    list(APPEND vtpty_HEADERS ConPty.h HandoffPty.h)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The write function returns -1 on error but should check if m_hInputWrite is INVALID_HANDLE_VALUE before attempting to write. Add a check at the beginning similar to the one in the close() method to prevent writing to an invalid handle.

Suggested change

	if (m_hInputWrite == INVALID_HANDLE_VALUE)
	return -1;

[Copilot]

The constructor does not check if CreateEvent calls succeed. If CreateEvent returns NULL, subsequent operations on m_hWakeup, m_readOverlapped.hEvent, or m_writeOverlapped.hEvent will fail. Add error checking after each CreateEvent call and handle the failure appropriately, possibly by throwing an exception or logging an error.

[Copilot]

Resource leak potential: If CreateFileW for hOutWrite fails after successfully creating both pipes and hInRead, the previously allocated handles (hInRead, hInWrite, hOutRead) are closed, but the cleanup doesn't close hOutRead before returning. Actually, looking more carefully, hOutRead IS closed on line 106, so this is correct. However, there's still a resource ownership issue: after returning S_OK, the handles hInWrite and hOutRead are passed to ContourHandleHandoff but the function has already returned them to the caller via *in and *out. This creates a situation where both the caller and ContourHandleHandoff own the same handles, which could lead to double-closure. The handles should either be duplicated for ContourHandleHandoff or the ownership model needs to be clarified.

[Copilot]

The factory object is declared as a static local variable, which means it has static storage duration and will be destroyed after main() returns. However, the COM registration holds a pointer to this object. If COM calls are made after the static destructor runs (which is possible during program shutdown), this will cause undefined behavior. Consider using dynamic allocation with appropriate lifetime management or ensuring COM is unregistered before static destruction occurs. The current placement of UnregisterCOMServer before the return should help, but the static lifetime is still concerning.

Suggested change

	static TerminalHandoffFactory factory;
	HRESULT hr = CoRegisterClassObject(CLSID_ContourTerminalHandoff,
	&factory,
	// Allocate the factory dynamically so it is not destroyed during static shutdown.
	static TerminalHandoffFactory* factory = new TerminalHandoffFactory();
	HRESULT hr = CoRegisterClassObject(CLSID_ContourTerminalHandoff,
	factory,

[Copilot]

Incomplete comment: The comment indicates existing implementations and references to line numbers, but this appears to be leftover scaffolding or notes from development. These comments should be removed as they don't provide useful information and may confuse readers.

Suggested change

	// ... (Existing implementations)
	// ... (Around line 350, inside terminalGuiAction)

[Copilot]

The read function should check if m_hOutputRead is INVALID_HANDLE_VALUE before attempting to read. Add a validation check at the beginning similar to the one in the close() method to prevent reading from an invalid handle.

Suggested change

	if (m_hOutputRead == INVALID_HANDLE_VALUE)
	return std::nullopt;

[Copilot]

Missing CoInitialize/CoUninitialize: The COM server is registered with CoRegisterClassObject but there's no corresponding CoInitialize call before registering and CoUninitialize after unregistering. COM requires that each thread calling COM functions must first initialize COM via CoInitialize or CoInitializeEx. Add CoInitializeEx(NULL, COINIT_MULTITHREADED) before RegisterCOMServer() and CoUninitialize() after UnregisterCOMServer().

[Copilot]

The COM registration sets the LocalServer32 value to "[INSTALL_ROOT]bin\contour.exe", but this requires the application to handle the -Embedding command-line flag to register as a COM server. While this is implemented in ContourGuiApp.cpp (lines 527-535), the registry doesn't include the -Embedding flag in the command line. According to COM LocalServer conventions, the registry value should typically include command-line arguments like "-Embedding". Consider changing the Value to "[INSTALL_ROOT]bin\contour.exe -Embedding" to explicitly pass the embedding flag when COM launches the server.

Suggested change

	<RegistryValue Root="HKCR" Key="CLSID\{B178D323-E77D-4C67-AF21-AE2B81F269F0}\LocalServer32" Type="string" Value="[INSTALL_ROOT]bin\contour.exe" />
	<RegistryValue Root="HKCR" Key="CLSID\{B178D323-E77D-4C67-AF21-AE2B81F269F0}\LocalServer32" Type="string" Value="&quot;[INSTALL_ROOT]bin\contour.exe&quot; -Embedding" />

[Copilot]

The TERMINAL_STARTUP_INFO structure uses BSTR for pszTitle and pszIconPath, but these fields are never freed or validated in the EstablishPtyHandoff implementation. BSTRs must be freed using SysFreeString to avoid memory leaks. Consider checking if these fields are non-null in the startupInfo parameter and freeing them appropriately, or document that the caller is responsible for managing their lifetime.

[Copilot]

Ambiguous comment: The comment on line 123 says "Or empty result?" indicating uncertainty about what should be returned when wakeupReader is called. The current implementation returns std::nullopt, but the comment suggests this decision wasn't finalized. Either confirm this is correct or implement a more specific return value that distinguishes wakeup from other nullopt cases.

Suggested change

	return std::nullopt; // Or empty result?
	return std::nullopt; // Wakeup: no data read; caller should treat this as "no data available"