@tmds @jkotas @stephentoub the PR is ready for review

🤖 Copilot Code Review — PR #125848

Holistic Assessment

Motivation: The PR adds StandardInputHandle, StandardOutputHandle, and StandardErrorHandle properties to ProcessStartInfo, enabling callers to pass pre-existing SafeFileHandle instances directly to child processes. This fills a real gap — the existing RedirectStandard* properties only support pipe-based redirection and don't allow scenarios like sharing a file handle, piping between processes, or discarding output via a null handle. The need is well-motivated by real-world use cases (process piping, output-to-file, handle inheritance).

Approach: The approach is clean — new nullable SafeFileHandle? properties on ProcessStartInfo with mutual exclusion enforcement against the RedirectStandard* booleans. The implementation correctly handles handle duplication on Windows (only when not already inheritable), falls back to Console.OpenStandard*Handle() for the default case, and properly manages handle ownership (user-provided handles are never disposed by the runtime). The removal of LeaveHandlesOpen in favor of "never dispose user handles" simplifies the API significantly.

Summary: ⚠️ Needs Human Review. The code is well-structured and handles most edge cases correctly. However, there is a behavioral change in the usesTerminal logic on Unix for UseShellExecute paths that warrants human judgment, and I could not verify the API approval status. A human reviewer should confirm these areas.

Detailed Findings

⚠️ API Approval — Cannot verify api-approved issue linkage

This PR introduces three new public API members on ProcessStartInfo (StandardInputHandle, StandardOutputHandle, StandardErrorHandle) and modifies the ref assembly accordingly. Per dotnet/runtime process, new public APIs require an approved proposal (issue with api-approved label) before PR submission. I could not find an explicit api-approved issue reference in the commit messages or PR branch. Since the PR author and co-author (adamsitnik) is a dotnet team member, this may be tracked externally, but a human reviewer should confirm that the API shape has been approved.

Ref assembly change at ref/System.Diagnostics.Process.cs lines 256–258

⚠️ Behavioral Change — usesTerminal logic for UseShellExecute on Unix

File: Process.Unix.cs, lines 389–392

The usesTerminal computation changed from an inclusive check (handle is null || IsATty) to a restrictive check (handle is not null && IsATty):

// OLD: null handle → assumes terminal (conservative)
bool usesTerminal = (stdinHandle is null || Interop.Sys.IsATty(stdinHandle)) || ...

// NEW: null handle → assumes NOT terminal
bool usesTerminal = (stdinHandle is not null && Interop.Sys.IsATty(stdinHandle)) || ...

When UseShellExecute = true, handles are null (the Console.OpenStandard*Handle() fallback only runs in the !UseShellExecute branch). This means:

• Old behavior: usesTerminal = true → terminal echo configured for child
• New behavior: usesTerminal = false → terminal echo NOT configured

The comment says "Handle can be null only for UseShellExecute or platforms that don't support Console.Open* methods like Android", suggesting this is intentional. On Unix, UseShellExecute typically launches via xdg-open (GUI apps, not terminal apps), so this may be acceptable. However, if someone uses UseShellExecute to launch an executable that interacts with the terminal (the first branch of the UseShellExecute code path resolves executables directly), terminal echo won't be configured. A human reviewer should confirm this behavioral change is acceptable.

⚠️ Missing Tests — Handle validation paths

The ValidateHandle method (Process.cs, lines 1446–1456) checks for IsInvalid (→ ArgumentException) and IsClosed (→ ObjectDisposedException), but there are no tests exercising these paths. Adding tests that verify:

1. Passing an invalid SafeFileHandle throws ArgumentException
2. Passing a disposed SafeFileHandle throws ObjectDisposedException

would prevent regressions in the validation logic.

✅ Handle Lifecycle — Ownership semantics are correct

The handle ownership design is sound:

• User-provided handles (StandardInputHandle etc.): never disposed by Process.Start() — the finally block (lines 1410–1421) skips disposal when the handle was user-provided.
• Console fallback handles (Console.OpenStandard*Handle()): created with ownsHandle: false, so Dispose() is safe and won't close the underlying fd/handle.
• Pipe handles (CreateAnonymousPipe): properly owned and disposed.

I verified both the Windows path (inheritableStdinHandle?.Dispose() in the finally block only disposes duplicates) and the Unix path (handles flow through to ForkAndExecProcess unchanged). The same-handle-for-stdout-and-stderr scenario works correctly — each duplication creates an independent handle.

✅ Windows Handle Duplication — Well-designed optimization

DuplicateAsInheritableIfNeeded (Process.Windows.cs, lines 619–649) correctly:

1. Skips invalid handles (from Console.OpenStandard*Handle() when the parent has no standard handle)
2. Skips already-inheritable handles via GetHandleInformation (avoids unnecessary DuplicateHandle calls)
3. Falls through to DuplicateHandle when GetHandleInformation fails (backward compatibility)

The Debug.Assert at line 456 correctly validates the "all or none" invariant for the three handles.

✅ Console Fallback — Default behavior preserved

The new code (lines 1349–1378) falls back to Console.OpenStandard*Handle() when no explicit handle and no redirection is requested. On Unix, these return fds 0/1/2 (stdin/stdout/stderr), and the native dup2 call correctly skips the no-op case (stdinFd != STDIN_FILENO), preserving the original behavior where the child inherits the parent's streams. The !OperatingSystem.IsAndroid() guard correctly handles Android where these APIs aren't supported.

✅ Locking — Appropriate and correct

The write lock in Process.cs is now only taken when anyRedirection && !SupportsAtomicNonInheritablePipeCreation (line 1332), which is correct — the lock exists to protect non-atomic pipe creation from race conditions, and user-provided handles or Console handles don't need this protection. On Windows, StartWithCreateProcess always takes its own write lock around CreateProcess to prevent handle inheritance races.

💡 Test Output Ordering — CanRedirectOutputAndErrorToSamePipe

ProcessHandlesTests.cs, lines 89–118: The test asserts a specific ordering of stdout then stderr output through the same pipe. While the test commands use sequential echo statements which should produce deterministic output, the ordering of interleaved stdout/stderr through a shared pipe is technically not guaranteed by the OS. The test is likely reliable in practice but could theoretically be flaky on heavily loaded systems. Consider noting this in a comment, or reading all output and asserting both lines are present without strict ordering.

💡 Removed Redundant Dispose() — Correct cleanup

Process.Unix.cs, line 720: The removal of handle.Dispose() after handle.SetHandleAsInvalid() is correct — once a handle is marked as invalid, Dispose() is a no-op since there's nothing to close. The simplified comment is also accurate.

Generated by Code Review for issue #125848 · ◷

/ba-g the failure are unrelated