Add support for pseudo-terminal (PTY) mode in CLI commands

[thomhurst]

Closes #225

I got AI to help me as I'm not the most familiar with native code etc 😄

[Copilot]

Pull request overview

This PR adds support for pseudo-terminal (PTY) mode in CLI command execution, enabling applications to behave as if running in an interactive terminal with features like colored output and progress indicators. The implementation is cross-platform, supporting Windows ConPTY (Windows 10 1809+), Linux, and macOS through native PTY APIs.

Key changes:

• Platform-specific PTY implementations using Windows ConPTY and Unix openpty
• New public API for configuring PTY options (PseudoTerminalOptions, PseudoTerminalOptionsBuilder)
• Custom process wrapper (PtyProcessEx) for PTY-based execution with stdin/stdout/stderr redirection
• Comprehensive test suite covering configuration, execution, and edge cases

Reviewed changes

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

Show a summary per file

File	Description
CliWrap/PseudoTerminalOptions.cs	Public API for PTY configuration with dimension validation
CliWrap/Builders/PseudoTerminalOptionsBuilder.cs	Fluent builder for configuring PTY options
CliWrap/ICommandConfiguration.cs	Extends interface to include PTY configuration
CliWrap/Command.cs	Adds WithPseudoTerminal() methods for enabling PTY mode
CliWrap/Command.Execution.cs	Implements PTY-specific execution pipeline with stream piping
CliWrap/Utils/PseudoTerminal.cs	Abstract base class for platform-specific PTY implementations
CliWrap/Utils/WindowsPseudoTerminal.cs	Windows ConPTY implementation with pipe-based I/O
CliWrap/Utils/UnixPseudoTerminal.cs	Unix PTY implementation using openpty()
CliWrap/Utils/UnixFdStream.cs	Stream wrapper for Unix file descriptors with read/write operations
CliWrap/Utils/PtyProcessEx.cs	PTY process wrapper handling process creation and lifecycle
CliWrap/Utils/NativeMethods.cs	P/Invoke declarations for Windows and Unix PTY APIs
CliWrap.Tests/PseudoTerminalSpecs.cs	Comprehensive test suite with 1099 lines covering configuration, execution, and edge cases

Comments suppressed due to low confidence (2)

CliWrap/Utils/NativeMethods.cs:106

• Minimise the use of unmanaged code.

        public static extern int WaitPid(int pid, out int status, int options);

CliWrap/Utils/NativeMethods.cs:124

• Minimise the use of unmanaged code.

        public static extern int CreatePseudoConsole(

---

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

[Copilot]

The CloseSlave() method has a check-then-act race condition. If called concurrently from multiple threads, Close() could be called multiple times on the same file descriptor, which could close a different fd if it was reused by the OS. Consider using a lock to ensure thread-safety:

private readonly object _slaveLock = new();

public void CloseSlave()
{
    lock (_slaveLock)
    {
        if (!_slaveFdClosed)
        {
            NativeMethods.Unix.Close(_slaveFd);
            _slaveFdClosed = true;
        }
    }
}

The same protection should be applied in the Dispose() method when closing the slave fd.

[Copilot]

If the UnixFdStream constructor (line 52) fails, the exception handler will call Close() on _masterFd. However, since UnixFdStream was already created with _masterFd, its Dispose() method will also try to close the same fd, leading to a double-close. The UnixFdStream.Dispose() takes ownership of the fd, so you should not close it again in the exception handler. Remove the Close(_masterFd) call from the catch block:

catch
{
    // Only close slave - UnixFdStream hasn't taken ownership if it failed to construct
    NativeMethods.Unix.Close(_slaveFd);
    throw;
}

Actually, review this more carefully - if UnixFdStream constructor throws before completion, it won't dispose the fd, so you may need to track whether the stream was successfully created.

[Copilot]

The recursive call to Read() when EINTR error occurs can potentially cause a stack overflow if signals are repeatedly interrupting the read operation. Consider using a loop instead of recursion to handle EINTR retries.

Example fix:

while (true)
{
    var bytesRead = NativeMethods.Unix.Read(_fd, ref buffer[offset], (nuint)count);
    
    if (bytesRead < 0)
    {
        var error = Marshal.GetLastWin32Error();
        if (error == NativeMethods.Unix.EINTR)
            continue; // Retry
        
        throw new IOException($"Failed to read from file descriptor {_fd}. Error code: {error}");
    }
    
    return (int)bytesRead;
}

Suggested change

	var bytesRead = NativeMethods.Unix.Read(_fd, ref buffer[offset], (nuint)count);
	if (bytesRead < 0)
	{
	var error = Marshal.GetLastWin32Error();
	// Retry if interrupted by signal
	if (error == NativeMethods.Unix.EINTR)
	return Read(buffer, offset, count);
	throw new IOException(
	$"Failed to read from file descriptor {_fd}. Error code: {error}"
	);
	}
	return (int)bytesRead;
	while (true)
	{
	var bytesRead = NativeMethods.Unix.Read(_fd, ref buffer[offset], (nuint)count);
	if (bytesRead < 0)
	{
	var error = Marshal.GetLastWin32Error();
	// Retry if interrupted by signal
	if (error == NativeMethods.Unix.EINTR)
	continue;
	throw new IOException(
	$"Failed to read from file descriptor {_fd}. Error code: {error}"
	);
	}
	return (int)bytesRead;
	}

[Copilot]

Creating a 'StringBuilder' in a loop.

[Tyrrrz]

Thank you for the PR! Left some comments. Copilot also left some good ones too.

[Tyrrrz]

Can you please add comments describing when each ignored exception type can happen and why we're ignoring them. Same for the PipePtyOutputAsync method.

[Tyrrrz]

Currently failiing on these two tests: https://github.com/Tyrrrz/CliWrap/actions/runs/20142129317?pr=309