Follow-up to #126192: address review comments (doc nit, ShellExecuteHelper refactor, field keyword, test, trimming, waitStateHolder fix)

[Copilot]

Follow-up to #126192 addressing post-merge review comments.

Description

Fix "OS handle" doc comment (r3007938546)

• SafeProcessHandle.Unix.cs: keeps the OS handle alive → keeps the handle alive — SafeWaitHandle on Unix is not an OS handle.

Convert ShellExecuteHelper to private static methods (r3005105880)

• Deleted ShellExecuteHelper.cs (top-level file added in Introduce SafeProcessHandle.Start and ProcessId #126192) and removed it from the .csproj.
• Moved ShellExecuteOnSTAThread and GetShellError into SafeProcessHandle.Windows.cs as private static methods — keeping the implementation co-located with its sole caller.
• GetShellError modernized to a switch expression, then converted to a static local function inside StartWithShellExecute (adjacent to its only call site).
• Renamed StartWithShellExecuteEx → StartWithShellExecute.
• ShellExecuteOnSTAThread folded into StartWithShellExecute — the split into two methods was not useful. The ShellExecuteFunction local function, STA thread setup, and Join now live directly in StartWithShellExecute; hProcess/hInstApp are read from the struct after the thread joins rather than via out parameters.
• SHELLEXECUTEINFO accepted by value (not by pointer); address of the local copy captured via nuint. The catch (EntryPointNotFoundException) block sets lastError = Interop.Errors.ERROR_CALL_NOT_IMPLEMENTED directly, eliminating the notPresent bool.
• StartWithCreateProcess folded into StartCore — mirrors the Linux factoring where StartCore directly contains the process creation logic rather than delegating to a separate method.

Use field keyword for ProcessId (r3009357619)

• Eliminated the _processId backing field in SafeProcessHandle.cs by using the C# field keyword with a property initializer:

  public int ProcessId
  {
      get { Validate(); if (field == -1) { field = GetProcessIdCore(); } return field; }
      private set;
  } = -1;

Add test for ProcessId validation (r3009467105)

• Added ProcessId_InvalidHandle_ThrowsInvalidOperationException to SafeProcessHandleTests.cs, verifying that accessing ProcessId on an invalid handle throws InvalidOperationException.

Add static delegate for UseShellExecute trimming support

Allows StartWithShellExecute (and its platform-specific dependencies) to be trimmed from apps that never set UseShellExecute. Modelled on the EnsureDecompressionHandlerFactory pattern in SocketsHttpHandler.

• SafeProcessHandle.cs: EnsureShellExecuteFunc() lives in the OS-neutral file — the body s_startWithShellExecute ??= StartWithShellExecute is textually identical on both platforms; the compiler resolves the names to the platform-specific partial class members.
• SafeProcessHandle.Windows.cs: declares private static Func<ProcessStartInfo, SafeProcessHandle>? s_startWithShellExecute — no SafeFileHandle parameters, since StartWithShellExecute on Windows does not use standard I/O handles. StartCore calls via s_startWithShellExecute!(startInfo).
• SafeProcessHandle.Unix.cs: declares a private custom delegate StartWithShellExecuteDelegate with signature (ProcessStartInfo, SafeFileHandle?, SafeFileHandle?, SafeFileHandle?, out ProcessWaitState.Holder?) → SafeProcessHandle — required so the UseShellExecute path in internal static StartCore correctly flows waitStateHolder out to the Process.Unix caller, preserving exit-state tracking for short-lived processes. StartWithShellExecute outputs the holder to the caller rather than disposing it internally; the ENOEXEC fallback path disposes the holder for the failed attempt (reusing waitStateHolder directly, without an extra local) before retrying with the default open handler. The private StartCore wrapper (used by SafeProcessHandle.Start) disposes the holder immediately after the call.
• ProcessStartInfo.cs: UseShellExecute consolidated into the shared file (previously duplicated in ProcessStartInfo.Win32.cs and ProcessStartInfo.Unix.cs) as a property whose setter calls SafeProcessHandle.EnsureShellExecuteFunc() (using the C# field keyword for the backing store).
• Process.Windows.cs: two redundant UseShellExecute = false assignments removed — false is already the default value.

[adamsitnik]

@copilot please also address #126192 (comment) and #126192 (comment)

[Copilot]

@copilot please also address #126192 (comment) and #126192 (comment)

Done in 7b2fad9:

• Used field keyword for ProcessId, eliminating the _processId backing field (r3009357619)
• Added ProcessId_InvalidHandle_ThrowsInvalidOperationException test (r3009467105)

[Copilot]

Pull request overview

Follow-up changes in System.Diagnostics.Process to incorporate post-merge feedback from #126192: documentation fix, ShellExecute refactor, use of the C# field keyword, additional tests, and trimming-oriented factoring for UseShellExecute.

Changes:

• Refactors ShellExecute support by removing ShellExecuteHelper.cs and inlining ShellExecute logic into SafeProcessHandle.Windows.cs.
• Consolidates ProcessStartInfo.UseShellExecute into the shared file and adds a trimming hook (EnsureShellExecuteFunc) to avoid rooting ShellExecute paths unless the property is used.
• Updates SafeProcessHandle.ProcessId to use the field keyword and adds a regression test for invalid-handle validation.

Reviewed changes

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

Show a summary per file

File	Description
src/libraries/System.Diagnostics.Process/tests/SafeProcessHandleTests.cs	Adds test ensuring ProcessId throws on invalid handle.
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ShellExecuteHelper.cs	Deletes helper class; logic is moved into SafeProcessHandle.Windows.cs.
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.cs	Moves UseShellExecute to shared file and invokes trimming hook in setter.
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.Win32.cs	Removes Win32-specific UseShellExecute property.
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.Unix.cs	Removes Unix-specific UseShellExecute property.
src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs	Removes redundant UseShellExecute = false assignments.
src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj	Removes ShellExecuteHelper.cs from compilation items.
src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.cs	Adds EnsureShellExecuteFunc; converts ProcessId to field keyword backing.
src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Windows.cs	Inlines ShellExecute/STA-thread logic and routes ShellExecute through delegate for trimming.
src/libraries/System.Diagnostics.Process/src/Microsoft/Win32/SafeHandles/SafeProcessHandle.Unix.cs	Adjusts shell-execute routing via delegate, adds doc comment fix, and factors Unix shell-execute into a method.

[jkotas]

Hmm,, looks like one cannot depend on copilot moving code around without introducing bugs.

[jkotas]

@copilot it is fine to reuse waitStateHolder here. You do not need an extra local

[Copilot]

Done in 989c73f. Removed firstHolder and now output directly into waitStateHolder from ForkAndExecProcess. The ENOEXEC dispose path calls waitStateHolder?.Dispose() directly before falling through to the fallback.

[github-actions]

🤖 Copilot Code Review — PR #126314

Note

This review was generated by Copilot using multi-model analysis (Claude Opus 4.6, Claude Sonnet 4.5, GPT-5.4).

Holistic Assessment

Motivation: The PR is a well-scoped follow-up to #126192, addressing post-merge review feedback from @jkotas, @adamsitnik, and @eiriktsarpalis. Each change is directly traceable to a review comment. The trimming support for UseShellExecute is a meaningful improvement.

Approach: The delegate-dispatch pattern for trimming follows the established EnsureDecompressionHandlerFactory precedent in SocketsHttpHandler. Consolidating UseShellExecute into the shared file, inlining ShellExecuteHelper, and using the field keyword are all clean improvements. The Unix waitStateHolder bug (found by Copilot's own earlier review and fixed in bb7af80) demonstrates the risk of code motion but is now resolved.

Summary: ✅ LGTM. The code is correct, well-reviewed by maintainers, and addresses all feedback. Two minor observations below — neither is blocking.

---

Detailed Findings

✅ Thread Safety — s_startWithShellExecute ??= is benign

SafeProcessHandle.cs:27 — Both models reviewed this. Claude Sonnet flagged it as a potential thread-safety error; GPT-5.4 assessed it as benign. I agree with GPT: racing threads may both read null and both write, but they always write the same method-group delegate (same target method). The final value is always correct. The only cost is a redundant delegate allocation on the rare concurrent initialization path. This matches the established EnsureDecompressionHandlerFactory pattern in SocketsHttpHandler and was specifically requested by @jkotas.

✅ Windows nuint Pointer Capture — Correct

SafeProcessHandle.Windows.cs:255 — The nuint cast of &shellExecuteInfo allows the lambda to capture the pointer without requiring unsafe in the delegate. Safety is guaranteed by Thread.Join() keeping the stack frame alive, and the struct is unmanaged stack data (no GC-movement risk). Both models confirmed this is sound.

✅ Unix waitStateHolder Lifecycle — Correct

SafeProcessHandle.Unix.cs:65-192 — All paths handle the holder correctly:

• Standalone Start (line 71): Disposed immediately after handle construction.
• Process internal path: Flows out via out parameter for caller ownership.
• ENOEXEC fallback (line 178): First attempt's holder disposed before retry.
• Error paths: Holder remains null, no cleanup needed.

Both models confirmed correctness. The earlier bug (missing out flow for UseShellExecute path) was properly fixed in bb7af80.

✅ field Keyword Usage — Correct

SafeProcessHandle.cs:38-46 and ProcessStartInfo.cs:235-243 — C# 13 semi-auto property syntax is used correctly. The ProcessId property initializes to -1 and lazily resolves on first access after validation. UseShellExecute uses field with a custom setter. Both are straightforward.

💡 UseShellExecute Setter Triggers Init Even for false

ProcessStartInfo.cs:238-242 — The setter calls EnsureShellExecuteFunc() unconditionally, so UseShellExecute = false (even though it's the default) would root the ShellExecute code for the trimmer. Both models flagged this. In practice, idiomatic code never explicitly sets UseShellExecute = false since that's the default, and the PR already removed the internal redundant assignments in Process.Windows.cs. An if (value) guard in the setter could tighten trimming further, but the current design was specifically requested by @jkotas and is sufficient for the common case. (Advisory — not blocking.)

💡 Code Duplication on Unix

SafeProcessHandle.Unix.cs:92-128 vs 132-146 — The env/cwd/credentials/terminal setup is duplicated between StartCore and StartWithShellExecute. Both models noted this. The duplication is intentional for trimming (separate methods let the linker trace independent call graphs), but a shared PrepareProcessStart helper could reduce drift risk. (Follow-up — not blocking.)

✅ Test Coverage — Adequate

SafeProcessHandleTests.cs:66-71 — The ProcessId_InvalidHandle_ThrowsInvalidOperationException test validates the Validate() guard path. The removed Start_UseShellExecute_InitializesDelegate test was correctly dropped per @jkotas's feedback (lazy init is implicitly covered by existing tests).

✅ ShellExecuteHelper Removal — Clean

The class is fully deleted with no dangling references. The GetShellError switch-case was modernized to a switch expression as a static local function. The STA thread logic was correctly inlined.

✅ Process.Windows.cs Cleanup — Correct

The removed UseShellExecute = false assignments were redundant (default is already false), and keeping them would now trigger EnsureShellExecuteFunc(), defeating the trimming optimization.

Generated by Code Review for issue #126314 · ◷