Repository navigation
Fix Start-Process -PassThru when no handle is returned - #27301
Tom Plant (pl4nty) wants to merge 2 commits into
Conversation
Signed-off-by: Tom Plant <tom@tplant.com.au>
38c12da to
0b7bbae
Compare
There was a problem hiding this comment.
Pull request overview
Adjusts Start-Process behavior on Windows Desktop when UseShellExecute starts a target that doesn’t yield a process handle (e.g., ms-settings:), to avoid throwing in -PassThru scenarios and align with expected ShellExecute behavior.
Changes:
- Update
StartWithShellExecuteto always return aProcessinstance and emit a debug message instead of relying onProcess.Start(ProcessStartInfo)returning non-null. - Add a Pester test covering
Start-Process ms-settings: -PassThruon full Windows.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs |
Changes ShellExecute startup path to return a Process object and write a debug message when a handle isn’t available. |
test/powershell/Modules/Microsoft.PowerShell.Management/Start-Process.Tests.ps1 |
Adds a Windows-only test invoking Start-Process ms-settings: -PassThru. |
| if (!result.Start()) { | ||
| string msg = StringUtil.Format(ProcessResources.FailedToCreateProcessObject, startInfo.FileName); | ||
| WriteDebug(msg); |
There was a problem hiding this comment.
FailedToCreateProcessObject is worded for a handle-retrieval failure and expects the reason in {0} (it’s used elsewhere with e.Message), but here {0} is startInfo.FileName, which will produce a misleading debug message. Also, the message references a Process object being output even when -PassThru isn't set; consider emitting this debug message only when PassThru (and/or Wait) is present and supply a reason that matches the resource string.
| if (!result.Start()) { | |
| string msg = StringUtil.Format(ProcessResources.FailedToCreateProcessObject, startInfo.FileName); | |
| WriteDebug(msg); | |
| if (!result.Start()) | |
| { | |
| if (PassThru || Wait) | |
| { | |
| string msg = StringUtil.Format(ProcessResources.FailedToCreateProcessObject, "Process.Start returned false."); | |
| WriteDebug(msg); | |
| } |
There was a problem hiding this comment.
Thought about this, but I reckon I'd like maintainer feedback on the overall approach before adding strings. May make more sense to just modify CannotStarttheProcess
Signed-off-by: Tom Plant <tom@tplant.com.au>
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
PR Summary
PR Context
Start-Process -PassThruthrow an exception whenProcess.Start(info)returns null, but this is expected in some cases. It confuses users when the process starts, but an exception is thrown (see #10996). The error message (CannotStarttheProcess) is also a bit ambiguous:This command cannot be run completely because the system cannot find all the information required.I'm proposing a debug message instead of an exception, like the one used when a process is started as a different user and we can't retrieve the handle. Then returning a Process object with just
StartInfo. If methods are called like$process.Kill(), a slightly more meaningful exception (NoAssociatedProcess) is thrown:No process is associated with this object. The null properties likeNamemight be confusing though.These changes also impact
-Wait- it would throwNoAssociatedProcessinstead ofCannotStarttheProcess.I'm not sure if this is a breaking change, are exceptions considered an API contract? It would break code like this:
Alternatively, we could use a more descriptive exception than
CannotStarttheProcess? Perhaps a format string that mentions-PassThruor-Wait.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header