Skip to content

Add ProcessStartOptions class with platform-aware path resolution#124271

Open
Copilot wants to merge 21 commits intomainfrom
copilot/add-processstartoptions-class
Open

Add ProcessStartOptions class with platform-aware path resolution#124271
Copilot wants to merge 21 commits intomainfrom
copilot/add-processstartoptions-class

Conversation

Copy link
Contributor

Copilot AI commented Feb 11, 2026

Implementation plan for ProcessStartOptions class - complete:

  • Add ProcessStartOptions API to reference file
  • Create ProcessStartOptions.cs implementation with lazy-allocated properties
  • Create platform-specific path resolution infrastructure
  • ProcessStartOptions.ResolvePath is the main implementation for both platforms
  • Process.Unix.cs calls ProcessStartOptions.ResolvePath
  • Made Process.IsExecutable internal for code reuse
  • All tests refactored with partial classes, new() syntax, Theory for utilities
  • Address all review feedback:
    • Add file existence check in constructor after ResolvePath
    • Fix .exe appending to check for directory separators
    • Use SR.FileNotFoundResolvePath resource string
    • Cache Windows directory value (s_cachedWindowsDirectory)
    • Fix system32 assertion to use Environment.SystemDirectory
    • Add IsNotWindowsServerCore condition to notepad tests
    • Remove unused ResolveTarget helper method
    • Remove DictionaryWrapper - use Dictionary<string, string?> directly
    • Use System.Environment.SystemDirectory instead of duplicating GetSystemDirectoryW
    • Inline GetSystemDirectory method per @adamsitnik feedback
    • Fix Unix path test to properly resolve symlinks using Path.GetFullPath on both sides
    • Change Assert.Contains to Assert.EndsWith for utility name validation
  • Build successfully
  • All review feedback addressed (47+ comments resolved)

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
@dotnet-policy-service
Copy link
Contributor

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

Copilot AI and others added 3 commits February 11, 2026 14:07
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI changed the title [WIP] Add ProcessStartOptions class to System.Diagnostics namespace Add ProcessStartOptions class with platform-aware path resolution Feb 11, 2026
Copilot AI requested a review from adamsitnik February 11, 2026 14:16
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
return filename;
}

#if WINDOWS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To other reviewers: In contrary to other classes, where we have dedicated .Windows.cs and .Unix.cs files, I would prefer this method to use #if defs and have everything in once place to make it easier to ensure they do the same thing on every OS, and when they don't it's also clear.

Copy link
Member

@jkotas jkotas Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be best to capture it in comments, including why we are doing this.

  • Why is it important that this method does the same thing on Windows and non-Windows?
  • Why is it important that this method emulates all legacy behaviors on Windows?

I understand why we have kept the Process class intact for better compatibility with .NET Framework. This is a new API. I do not think we need to carry all this baggage into the new API. Have we reviewed this baggage from a security point of view? For example, is probing current directory a good idea from a security point of view in a new API?

Copy link
Member

@adamsitnik adamsitnik Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be best to capture it in comments, including why we are doing this.

@jkotas Is it OK if I include these comments along with next PR? I don't want to start all the CI legs and then wait another 3h (as soon as this PR gets merged I have another one ready)

For example, is probing current directory a good idea from a security point of view in a new API?

I asked @GrabYourPitchforks the other day and the response was that it's ok to probe CWD. I will bring it again during Threat Model.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether it is just comments. I cannot tell whether the code is right without answers to these questions. I do not know the answers to these questions myself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective, mimicking what we were doing so far in Process will make it easy for the users to migrate to the new APIs.

For example, I expect plenty of users to not specify ".exe" extension in explicit way.

I suggest to mimic the current behavior and discuss this topic in depth during Threat Model (next month, not after we ship 11). Then the change will land in next preview.

I am going to ping @GrabYourPitchforks offline to check if he is OK with that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, I expect plenty of users to not specify ".exe" extension in explicit way

Appending .exe extension on Windows sound reasonable. The other behaviors, like the ProcessPath and current directory probing on Unix, look questionable to me. Is there any other platform where process APIs do that on Unix?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any other platform where process APIs do that on Unix?

When it comes to libc, execlp(), execvp(), and execvpe() search in PATH, when it's not defined then in CWD

Details

From https://linux.die.net/man/3/execv:

The execlp(), execvp(), and execvpe() functions duplicate the actions of the shell in searching for an executable file if the specified filename does not contain a slash (/) character. The file is sought in the colon-separated list of directory pathnames specified in the PATH environment variable. If this variable isn't defined, the path list defaults to the current directory followed by the list of directories returned by confstr(_CS_PATH). (This confstr(3) call typically returns the value "/bin:/usr/bin".)

golang seems to have a very reasonable approach:

  • If you pass absolute/relative path with separators (./tool, C:\bin\tool.exe) → no PATH search.
  • If you pass bare name (tool) → PATH search, but never implicitly from CWD as of Go 1.19
  • if you want “run from current directory”, you must say ./prog explicitly

I am pretty sure the reason why Process/ProcessStartInfo searches in CWD then in PATH is because it's the default CreateProcessW order.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copilot AI requested a review from adamsitnik February 11, 2026 18:49
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot please address my feedback

…names

Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
}

#if WINDOWS
// From: https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a comment explaining why we need to have a copy of the logic for resolving path from Windows CreateProcess implementation. I cannot think about a good reason why we want to doing that by just looking at this code. (I know you have mentioned something about it in the offline discussion.)


/// <summary>
/// Initializes a new instance of the <see cref="ProcessStartOptions"/> class.
/// </summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs for the constructor should talk about how the path resolution is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants