Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

@copilot address my feedback

All feedback addressed in commit 07d9369:

• Removed partial method declarations; OpenStream/encoding methods defined directly in platform files
• Pipe creation uses SafeFileHandle.CreateAnonymousPipe directly in Process.cs:Start() with !startInfo.UseShellExecute || !OperatingSystem.IsWindows() condition
• Console.OpenStandard*Handle() called from Start() for non-redirected streams
• Windows StartWithCreateProcess just calls DuplicateAsInheritable for all 3 handles
• Removed CreatePipe from both platforms, s_invalidSafeFileHandle from Unix, Interop.Pipe.cs from csproj
• Removed XML docs from non-public methods
• Build passes all targets; 372/373 tests pass (1 flaky TestGetProcesses failure unrelated to changes)

@copilot please update the PR description based on my commits and then invoke the code-review skill and post your analysis/results as a comment on this PR

@copilot please update the PR description based on my commits and then invoke the code-review skill and post your analysis/results as a comment on this PR

PR description updated. Code review follows below.

🤖 Copilot Code Review

Holistic Assessment

Motivation: This refactoring lifts pipe creation out of platform-specific code into the shared Process.cs:Start() method, making StartCore accept pre-created SafeFileHandle parameters. This enables future scenarios where callers provide pre-existing handles and unifies the pipe creation logic across platforms.

Approach: The approach is sound — pipes are created uniformly via SafeFileHandle.CreateAnonymousPipe in shared code, with Console.OpenStandard*Handle() fallbacks for non-redirected streams. Platform-specific OpenStream and encoding methods are defined directly in each platform file without partial methods. The locking strategy is carefully designed to prevent accidental handle inheritance.

Summary: ✅ LGTM. The refactoring is correct, handles resource management properly, and the locking scheme is well-designed. Two models (Gemini 2.5 Pro and Claude Opus 4.5) reviewed this independently; their findings converge. No blocking issues found.

Detailed Findings

✅ Resource management — Console handles are safe to dispose

The finally block in Process.cs:Start() disposes childInputPipeHandle, childOutputPipeHandle, childErrorPipeHandle. When not redirecting, these are assigned from Console.OpenStandard*Handle(). Verified that both Unix and Windows implementations create these with ownsHandle: false:

• Unix: new SafeFileHandle(fd, ownsHandle: false) (ConsolePal.Unix.cs)
• Windows: new SafeFileHandle(handle, ownsHandle: false) (ConsolePal.Windows.cs)

Disposing a non-owning SafeFileHandle is a no-op — no risk of closing actual stdin/stdout/stderr descriptors.

✅ Handle ownership transfer in Unix OpenStream — No double-dispose

The Unix OpenStream method transfers ownership from SafeFileHandle to SafePipeHandle:

SafePipeHandle safePipeHandle = new(handle.DangerousGetHandle(), ownsHandle: true);
handle.SetHandleAsInvalid();
handle.Dispose();

The parent pipe handles (parentInputPipeHandle etc.) are only disposed in the catch block on exception, which runs before OpenStream is called. On success, OpenStream takes ownership and no double-dispose occurs.

✅ Platform-conditional handle creation — Assertions are correct

The condition !startInfo.UseShellExecute || !OperatingSystem.IsWindows():

• On Unix: Always true → handles are always created. The assertion stdinHandle is not null && ... is correct.
• On Windows: true only when !UseShellExecute → handles are null for ShellExecute. The assertion UseShellExecute == (handles are null) is correct.

✅ Native dup2 now unconditional — Safe

The native code now always calls dup2(stdinFd, STDIN_FILENO) etc. When not redirecting, the caller passes the actual stdin fd (from Console.OpenStandardInputHandle()), so stdinFd == STDIN_FILENO. Per POSIX, dup2(fd, fd) is a no-op that returns fd without error.

✅ Locking scheme — Correctly prevents handle inheritance leaks

• On platforms without pipe2: WriteLock wraps pipe creation (preventing other processes from inheriting pipes during the non-atomic create+set-CLOEXEC window).
• On Windows: WriteLock wraps DuplicateAsInheritable + CreateProcess (preventing concurrent processes from inheriting each other's inheritable handles).
• On Unix: ReadLock during ForkAndExecProcess allows concurrent process starts while being mutually exclusive with WriteLock operations.

The lock is shared across platforms via ProcessUtils.s_processStartLock, which is correct since the same ReaderWriterLockSlim instance must be used for read/write coordination.

💡 Minor — Comment could be more precise

The comment at line 1372 ("We MUST close the parent copies of the child handles") is slightly misleading for the non-redirect case where handles came from Console.OpenStandard*Handle() — these aren't "parent copies of child handles" but non-owning wrappers. This is a cosmetic observation, not a blocker.

Models contributing to this review: Claude Sonnet 4 (primary), Gemini 2.5 Pro, Claude Opus 4.5

extending Process with a new Start overload that takes ProcessStartInfo instance and 3 SafeFileHandle arguments

I am not sure this API shape is going to fly well - it is inconsistent with what's there today. I think the SafeFileHandles should be members on ProcessStartInfo and be mutually exclusive with the other ways to redirect input/output.

We should start from the use-cases/issues, propose the API and design (which may involve some prototyping/derisking) and then the actual implementation follows.

You may have a picture about the API and design. I don't think it is written down so others can understand what way we're heading and how it solves the user issues.

I can technically review if this refactoring isn't breaking anything, but currently I can't say why we need it, which is the more important thing.

Personally, I prefer we capture such information in an issue. PRs are already implementation centric and the coding agent workflows make it even more focused on code and code changes.

@copilot please address my feedback

Addressed in 6718589 and 6ddf417:

• Process.cs now only creates pipe handles for redirected streams (no Console.OpenStandard*Handle() calls)
• Interop.ForkAndExecProcess.cs wrapper accepts SafeFileHandle?, converts to raw int fd using DangerousAddRef/DangerousGetHandle/DangerousRelease; P/Invoke uses int parameters
• All callers (Unix/Windows StartCore) updated to handle nullable handles
• Native dup2 skips when fd is -1
• Fixed usesTerminal to correctly handle null handles (null = child inherits parent's stream, could be a terminal)

We should start from the use-cases/issues, propose the API and design (which may involve some prototyping/derisking) and then the actual implementation follows.

You may have a picture about the API and design. I don't think it is written down so others can understand what way we're heading and how it solves the user issues.

It is the problem that Tom commented on. It is not clear what exact API shapes is this refactoring expected to support.

@tmds @jkotas I am all aware of that. I must admit that the fact that ProcessStartOptions was approved and then reverted costed me (and the reviewers) a few weeks of work. I understand the decision and I am on board with it, however as a human I am simply a bit nervous and afraid that I may not deliver what I've promised on time.

Since we all agreed that exposing the ability to redirect standard handles to any file handle is good idea and the API design review meetings happen once per week, I was hoping to reduce the time it will take to get the approved API merged by preparing the repo with necessary refactor first.

My entire idea was following:

• this PR refactors the code to just accept SafeFileHandle internally, it gets reviewed and merged before API review meeting
• in the meantime I prepare another PR that implements the proposed API (Add StandardInput/Output/Error SafeFileHandle properties and LeaveHandlesOpen to ProcessStartInfo #125848) to verify it can be implemented
• right after API review meeting I send the right prompt to Copilot to make any necessary changes
• when the CI gets green, I ask humans to review it. It's smaller PR and easier to review
• as soon as this PR gets merged, I send another PR for moving Process.StartCore to SafeProcessHandle.Start as-is. It would be a step toward completing SafeProcessHandle that got hit pretty hard by the option bag direction change

So I wanted to optimize the performance and deliver the new API slightly faster. I apologize for not being clear and wasting your time.