KEMBAR78
Fix up SSHConnectionInfo ssh PATH checks by jborean93 · Pull Request #25780 · PowerShell/PowerShell · GitHub
Skip to content

Fix up SSHConnectionInfo ssh PATH checks #25780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Aug 4, 2025
Merged

Fix up SSHConnectionInfo ssh PATH checks #25780

merged 3 commits into from
Aug 4, 2025

Conversation

@jborean93
Copy link
Collaborator

PR Summary

Fix the logic for finding the ssh executable in the PATH when running in a context without an active Runspace. For example running it in an SDK hosted PowerShell instance will not have an active Runspace when creating the ssh process so cannot use the PowerShell command lookup class.

I'm not sure of a way to effectively test this in CI unfortunately.

PR Context

Fixes: #15641
Fixes: #25778

PR Checklist

@jborean93
Fix up SSHConnectionInfo ssh PATH checks
ea07b93 
Fix the logic for finding the ssh executable in the PATH when running in
a context without an active Runspace. For example running it in an SDK
hosted PowerShell instance will not have an active Runspace when
creating the ssh process so cannot use the PowerShell command lookup
class.
@jborean93 jborean93 mentioned this pull request Jul 24, 2025
Closed
5 tasks
daxian-dbw
daxian-dbw approved these changes Jul 28, 2025
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM!

@daxian-dbw daxian-dbw added the CL-Engine Indicates that a PR should be marked as an engine change in the Change Log label Jul 28, 2025
iSazonov
iSazonov reviewed Jul 29, 2025
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

As for new test we could create simple xUnit test based on code from related issue.

@jborean93
Copy link
Collaborator Author

As for new test we could create simple xUnit test based on code from related issue.

I could split out the logic into it's own function and run that but I can't run the reproducer because it'll require an SSH endpoint that is configured with the PowerShell subsystem.

@daxian-dbw daxian-dbw self-assigned this Jul 29, 2025
@iSazonov
Copy link
Collaborator

As for new test we could create simple xUnit test based on code from related issue.

I could split out the logic into it's own function and run that but I can't run the reproducer because it'll require an SSH endpoint that is configured with the PowerShell subsystem.

The endpoint could be easily created in Start-PSPester but if MSFT team still hasn't considered it important for testing, then I'm not insisting.

@jborean93
Copy link
Collaborator Author

@daxian-dbw sorry I can't seem to reply inline to your last review comment.

The comment around cwd is about PowerShell setting psi.WorkingDirectory = Path.GetDirectoryName(filePath); later down in the function.

PowerShell/src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

Line 2325 in ea07b93

startInfo.WorkingDirectory = System.IO.Path.GetDirectoryName(filePath);

The comment is explaining that we can't rely on ProcessStartInfo to lookup ssh for us as we use the full path to determine what cwd to set later on so we need to look it up ourselves.

Happy to reword the comment if it's confusing but would probably need some suggestions on what to change to make it clearer.

@daxian-dbw
Copy link
Member

@jborean93 It may not be a big deal, but your current comment sounds like "we don't know why cwd is set to the parent directory below, but since it's doing that, we have to search PATH manually here".

Please take a look at my comment above (#25780 (comment)). I think it my be better to say in the comment that "we search PATH manually to keep that behavior of 'LookupCommandInfo'". What do you think?

@jborean93
Copy link
Collaborator Author

jborean93 commented Aug 4, 2025 •
edited
Loading

The comment is just trying to say we do the manual lookup because we need the full path so we can set the cwd ourselves. I can try and update the comment to be clearer but without knowing why we set the cwd later on in the code it's hard to justify the extra work when ProcessStartInfo could do it for us.

I'm assuming it's because we don't want to use ssh that might be in the cwd as LookupCommand ignores that or some other reason.

jborean93 and others added 2 commits August 5, 2025 08:27
@jborean93 @daxian-dbw
Update src/System.Management.Automation/engine/remoting/common/Runspa…
35ea03a 
…ceConnectionInfo.cs

Co-authored-by: Dongbo Wang <dongbow@microsoft.com>
@jborean93
Update src/System.Management.Automation/engine/remoting/common/Runspa…
6dba017 
…ceConnectionInfo.cs
daxian-dbw
daxian-dbw approved these changes Aug 4, 2025
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@daxian-dbw
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@daxian-dbw daxian-dbw merged commit 8dd0d2a into PowerShell:master Aug 4, 2025
37 of 39 checks passed
@Nova-Logic
Copy link

Many thanks to @jborean93 and all powershell team for fixing this!

@jborean93 jborean93 deleted the ssh-runspace branch August 4, 2025 23:40
@jborean93
Copy link
Collaborator Author

Thanks for the review/merge @daxian-dbw and @iSazonov

Amro1984 pushed a commit to Amro1984/PowerShell that referenced this pull request Aug 12, 2025
@jborean93
Fix the ssh PATH check in SSHConnectionInfo when the default Runs…
6f3b2d4 
…pace is not available (PowerShell#25780)
Amro1984 pushed a commit to Amro1984/PowerShell that referenced this pull request Aug 12, 2025
@jborean93
Fix the ssh PATH check in SSHConnectionInfo when the default Runs…
bc9cdf3 
…pace is not available (PowerShell#25780)
Nova-Logic pushed a commit to Nova-Logic/PowerShell that referenced this pull request Sep 17, 2025
backport of PR PowerShell#25780 created by jborean93
7d1b116
@Nova-Logic
Copy link

If that would help release/v7.5...Nova-Logic:PowerShell:release/v7.5 that is the same change with small tweak to make it compatible with .net 9 which is used in 7.5

pwshBot pushed a commit to pwshBot/PowerShell that referenced this pull request Oct 9, 2025
@jborean93 @pwshBot
Fix the ssh PATH check in SSHConnectionInfo when the default Runs…
aeefb75 
…pace is not available (PowerShell#25780)
@pwshBot pwshBot mentioned this pull request Oct 9, 2025
Open
9 tasks
pwshBot pushed a commit to pwshBot/PowerShell that referenced this pull request Oct 9, 2025
@jborean93 @pwshBot
Fix the ssh PATH check in SSHConnectionInfo when the default Runs…
110767c 
…pace is not available (PowerShell#25780)
@pwshBot pwshBot mentioned this pull request Oct 9, 2025
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov left review comments

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13 TravisEz13 is a code owner

Assignees

@daxian-dbw daxian-dbw

Labels

Backport-7.4.x-Migrated Backport-7.5.x-Migrated CL-Engine Indicates that a PR should be marked as an engine change in the Change Log

Projects

PowerShell Team Reviews/Investigations
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RunspaceFactory.CreateRunspace(SshConnectionInfo) is not working Runspace with SSHConnectionInfo is not working on Linux or Windows

6 participants

@jborean93 @iSazonov @daxian-dbw @Nova-Logic @adityapatwardhan @SydneyhSmith