driver-ssh: SSHWrapperClient.run return stdout and stderr#748
driver-ssh: SSHWrapperClient.run return stdout and stderr#748mangelajo merged 3 commits intojumpstarter-dev:mainfrom
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds dataclasses Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SSHClient
participant LocalSSH as _run_ssh_local
participant RemoteSSH as port_forward_path
participant Subproc as _execute_ssh_command
Caller->>SSHClient: run(options)
alt options.direct == true
SSHClient->>LocalSSH: _run_ssh_local(options)
LocalSSH->>Subproc: _execute_ssh_command(ssh_args, options)
Subproc-->>LocalSSH: SSHCommandRunResult
LocalSSH-->>SSHClient: SSHCommandRunResult
else options.direct == false
SSHClient->>RemoteSSH: attempt port-forward/proxy (options)
RemoteSSH->>Subproc: _execute_ssh_command(ssh_args, options)
Subproc-->>RemoteSSH: SSHCommandRunResult
alt failure & fallback to direct
SSHClient->>SSHClient: run(options with direct=true) %% recursive fallback
end
RemoteSSH-->>SSHClient: SSHCommandRunResult
end
SSHClient-->>Caller: SSHCommandRunResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (4)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (5)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (4)
72-104:runfallback recursion is safe but could be flatterThe
runmethod’s recursive fallback fromdirect=Truetorun(False, args)onDriverMethodNotImplemented/ValueErroris functionally correct and will only recurse once, but it slightly obscures the control flow.If you prefer flatter logic, you could convert the direct-connection attempt into a conditional block that falls through to the port-forwarding path on failure instead of re-calling
run. Behavior would remain the same but be a bit easier to trace.
118-121: Be cautious about logging potentially sensitive SSH detailsThe new debug/error logs around:
- Temporary identity file creation/cleanup (lines 118, 145–147),
- Parsed SSH options and command args (lines 219–220),
- The final SSH command (line 236),
are very helpful for troubleshooting, but they may reveal:
- Full command lines, including arguments that could embed secrets (tokens, passwords),
- The filesystem path of the temporary private key file.
If logs can be collected centrally or accessed by broader audiences, consider:
- Redacting or truncating particularly sensitive args,
- Logging only a subset (e.g., first N characters) or hashes of arguments,
- Downgrading some of these logs to trace-level, if your logging infra supports it.
Not mandatory, but worth thinking about from a security and compliance standpoint.
Also applies to: 145-147, 219-220, 236-237
149-221: SSH option parsing has edge cases and double parsingThe
_build_ssh_command_argsand_separate_ssh_options_and_command_argshelpers are a nice improvement, but there are a couple of subtleties:
Combined
-vflags (-vv,-vvv, …)
The parser only treats-vas a no-param flag. Common SSH invocations like-vvor-vvvwill fall into the “unknown flag → start of command args” branch and end up being treated as part of the remote command instead of SSH options. This will surprise users who are accustomed tossh -vvv.A small robustness tweak could be:
- if arg in ssh_flags_no_param: + if arg in ssh_flags_no_param or ( + arg.startswith("-v") and all(ch == "v" for ch in arg[1:]) + ):so that repeated
vflags are still treated as SSH options.Double parsing (and double debug logging)
_build_ssh_command_argscalls_separate_ssh_options_and_command_args(args)just to detect a-lusername flag, and_run_ssh_localcalls the same helper again to getssh_options/command_args. This both repeats the work and causes the “SSH options / Command args” debug logs to be emitted twice per invocation.You could avoid this by parsing once in
_run_ssh_localand passingssh_optionsdown:- # Build SSH command arguments - ssh_args = self._build_ssh_command_args(ssh_command, port, default_username, identity_file, args) - - # Separate SSH options from command arguments - ssh_options, command_args = self._separate_ssh_options_and_command_args(args) + # Separate SSH options from command arguments + ssh_options, command_args = self._separate_ssh_options_and_command_args(args) + + # Build SSH command arguments + ssh_args = self._build_ssh_command_args( + ssh_command, port, default_username, identity_file, ssh_options + )and update
_build_ssh_command_argsto takessh_optionsinstead of fullargs:- def _build_ssh_command_args(self, ssh_command, port, default_username, identity_file, args): + def _build_ssh_command_args(self, ssh_command, port, default_username, identity_file, ssh_options): ... - # Check if user already provided a username with -l flag in SSH options only - # We need to separate SSH options from command args first to avoid false positives - ssh_options, _ = self._separate_ssh_options_and_command_args(args) has_user_flag = any( ssh_options[i] == "-l" and i + 1 < len(ssh_options) for i in range(len(ssh_options)) )This keeps the behavior the same while reducing overhead and log noise.
239-253:FileNotFoundErrorhandling is solid; consider standardizing stderr textThe explicit handling of
FileNotFoundErrorwith exit code127and an explanatory stderr message is a nice touch and aligns with common Unix conventions.If other parts of the system depend on parsing stderr, you may want to ensure this message shape is documented or reused (e.g., via a helper or constant) so downstream callers can rely on it without being surprised by future wording changes. Otherwise this is good as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(28-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: e2e
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
16-29:SSHCommandExecResultabstraction looks goodThe dataclass cleanly encapsulates the subprocess result, and
from_completed_processis a straightforward, type-safe factory with no obvious pitfalls. This should make call sites much clearer.
aesteve-rh
force-pushed
the
ssh-wrapper-return
branch
3 times, most recently
from
ccc395b to
4525d8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
177-237: Avoid double parsing/logging of SSH options and command args
_separate_ssh_options_and_command_args(args)is invoked both in_build_ssh_command_args(to detect-l) and again in_run_ssh_local(to getssh_options/command_args), which also means the new debug logs fire twice per call with identical content. Consider computingssh_options/command_argsonce in_run_ssh_localand passing them into_build_ssh_command_args(or factoring a small helper) to remove the redundant work and duplicate logging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(8 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(33 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(28-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: e2e
🔇 Additional comments (5)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (5)
16-29: SSHCommandExecResult wrapper is clear and correctly encapsulates subprocess outputThe dataclass and
from_completed_processhelper cleanly model the SSH execution result (code, stdout, stderr) and keep the subprocess details localized; no issues here.
48-60: CLI now correctly surfaces SSH stdout/stderr while preserving exit semanticsUsing
SSHWrapperClient.runand then echoingstdout/stderrviaclick.echoensures CLI users see the command output again, while callers still get the structuredSSHCommandExecResultand the CLI exits on non‑zero codes. This effectively resolves the earlier regression around hidden output mentioned in past reviews.
72-104: Direct vs port‑forward flow and fallback look logically sound
runcleanly chooses between direct TCP and SSH port‑forwarding, with a bounded recursive fallback (self.run(False, args)) onDriverMethodNotImplemented/ValueErrorand useful debug/error logs for both paths; I don’t see functional or resiliency issues in this control flow.
108-147: Temporary identity file handling and cleanup are robustCreating a non‑deleted
NamedTemporaryFile, forcing0o600, logging the path at debug level, and unconditionally unlinking it in afinallyblock (with a guarded warning on failure) gives a good balance of security, observability, and reliability for SSH identities.
239-253: Subprocess execution wrapper and 127 “command not found” mapping look correctWrapping
subprocess.runwithcapture_output=True, text=Trueand returning anSSHCommandExecResult, plus mappingFileNotFoundErrorto an exit code of 127 with a clear stderr message, provides a predictable, testable contract for callers and consistent CLI behaviour.
|
you probably need a different run method, I believe this will prevent the regular "j ssh" that should drop you into a shell. I haven't tried it yet with your patch, so I could be wrong. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
40-61: CLI behavior and SSH run flow are consistent and user-friendly.The refactor to have
run()returnSSHCommandExecResult, echo stdout/stderr in the Click handler, and exit based onreturn_codegives both programmatic callers and CLI users the information they need, while the added logging around connection mode, temp identity files, and argument parsing should make debugging SSH issues much easier.If you later want to micro-optimize, you could consider passing the
ssh_options/command_argsalready computed in_build_ssh_command_argsinto_run_ssh_localinstead of re-parsingargs, but this is purely optional and not a correctness concern.Also applies to: 72-75, 90-94, 101-103, 118-121, 145-147, 219-221, 236-237
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(8 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(33 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(28-29)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
run(72-103)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(28-29)
🪛 GitHub Actions: Run Tests
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py
[error] 8-8: ImportError: cannot import name 'SSHCommandExecutionResult' from 'jumpstarter_driver_ssh.client'
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
16-21: SSHCommandExecResult and subprocess wrapping look solid.The dataclass cleanly encapsulates
return_code,stdout, andstderr, and_execute_ssh_commandcorrectly wrapssubprocess.run(with captured text output) plus a sensible127result onFileNotFoundError. This matches the new tests’ expectations and provides a much nicer API for callers.Also applies to: 23-29, 239-253
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py
Outdated
Show resolved
Hide resolved
|
|
||
| def run(self, direct, args): | ||
| """Run SSH command with the given parameters and arguments""" | ||
| def run(self, direct, args) -> SSHCommandExecResult: |
There was a problem hiding this comment.
| def run(self, direct, args) -> SSHCommandExecResult: | |
| def run(self, direct, args) -> SSHCommandRunResult: |
to allow more things in the future we should break the api now (early before any users using this), and change direct for:
SSHCommandRunOptions:
direct: bool
capture_output: bool
capture_as_text: bool
then we can take some extra parameters if we need in the future.
|
|
||
|
|
||
| @dataclass | ||
| class SSHCommandExecResult: |
There was a problem hiding this comment.
| class SSHCommandExecResult: | |
| class SSHCommandRunResult: |
what do you think about calling it like this to be aligned with the method name?
There was a problem hiding this comment.
Sounds good to me, naming is hard to nail on the first iteration :D
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
some comments, thank you @aesteve-rh !! :D
aesteve-rh
force-pushed
the
ssh-wrapper-return
branch
2 times, most recently
from
ad5b755 to
d56492c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
48-69: CLIssh()is now broken (wrongrunsignature) and risks breaking interactivej ssh
SSHWrapperClient.runnow expectsoptions: SSHCommandRunOptions, but the CLI still calls it with a barebool:def ssh(direct, args): result = self.run(direct, args) # direct is a boolInside
runyou immediately accessoptions.direct, so invokingj sshwill raiseAttributeError: 'bool' object has no attribute 'direct'. This is a hard runtime break for the CLI.Independently, if you simply start passing
SSHCommandRunOptions(direct=direct)here, you’ll usecapture_output=Trueand will no longer stream SSH I/O to the terminal, which breaks the interactive shell use case thatj sshlikely relies on.A minimal fix that restores previous interactive behavior while still exposing the richer API to programmatic callers:
def cli(self): @@ @click.option("--direct", is_flag=True, help="Use direct TCP address") @click.argument("args", nargs=-1) def ssh(direct, args): - result = self.run(direct, args) + # CLI path: preserve interactive behavior by *not* capturing output + options = SSHCommandRunOptions( + direct=direct, + capture_output=False, + capture_as_text=True, + ) + result = self.run(options, args)Programmatic use (e.g. tests) can keep using
SSHCommandRunOptions(direct=False)withcapture_output=Trueto collect stdout/stderr.Also applies to: 80-105
🧹 Nitpick comments (4)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
16-37: Result/options dataclasses look good; consider loosening stdout/stderr typesThe new
SSHCommandRunResult/SSHCommandRunOptionsabstraction is clean and fits the refactor. One nuance: whenoptions.capture_outputisFalse,subprocess.runwill typically leavestdout/stderrasNone, but the dataclass annotates them asstr. This is harmless at runtime, but misleading for type checkers and callers that may assumestr.If you plan to rely on typing here, consider:
- stdout: str - stderr: str + stdout: str | None + stderr: str | Noneand adjust any call sites that assume non‑None. Otherwise, the implementation is consistent and the error‑path wrapping (
return_code=127, message instderr) looks solid.Also applies to: 249-263
87-105: Consider preserving futureSSHCommandRunOptionsfields when falling back from direct → port‑forwardThe fallback from
direct=Trueto port‑forwarding re-creates anSSHCommandRunOptionswith only the current fields:return self.run(SSHCommandRunOptions( direct=False, capture_output=options.capture_output, capture_as_text=options.capture_as_text, ), args)This works today but is brittle if you later add fields to
SSHCommandRunOptions(they’ll silently reset to defaults on fallback).A small robustness improvement is to clone and override only
direct, e.g. usingdataclasses.replace:from dataclasses import dataclass, replace @@ - return self.run(SSHCommandRunOptions( - direct=False, - capture_output=options.capture_output, - capture_as_text=options.capture_as_text, - ), args) + fallback_options = replace(options, direct=False) + return self.run(fallback_options, args)This keeps the options model future‑proof.
229-230: Debug logging of SSH args/options is useful; be mindful of potential secretsThe added debug logs for
ssh_options,command_args, and the full SSH command are very helpful for troubleshooting, but they may include credentials or tokens if callers pass them as arguments.If this client is ever used with sensitive command arguments, consider either redacting known‑sensitive options before logging or guarding these logs behind a more verbose level / config flag. Otherwise this is fine as debug‑level trace.
Also applies to: 246-247
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (1)
469-694: Identity temp-file behavior and logging are well specified; consider one extra caseThe identity-related tests (string vs file, lazy loading, temp-file creation/cleanup, creation error via
ExceptionGroup, and cleanup warning logging) map closely to the implementation in_run_ssh_localand give strong coverage of tricky paths.One optional enhancement would be a small unit test that exercises
SSHCommandRunOptions(capture_output=False)to assert that:
subprocess.runis invoked withcapture_output=False, and- the resulting
SSHCommandRunResult.stdout/.stderrare whatever you expect in that mode (likelyNone).This would lock in the intended semantics for the interactive/streaming case on the programmatic side. Otherwise, the existing tests look very solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(8 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(32 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(28-29)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
SSHCommandRunOptions(33-37)SSHCommandRunResult(17-29)run(80-113)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (4)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(42-52)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
FileNotFoundError(66-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: e2e
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
🔇 Additional comments (4)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
115-158: Identity temp file handling and cleanup look correct and well-covered by testsThe
_run_ssh_localidentity handling (creation withNamedTemporaryFile,chmod 600, and unconditional cleanup infinally) is solid and the logging on both creation and cleanup failure is appropriate. The associated tests indriver_test.pyvalidate both the happy path and error/warning cases, including permission errors and cleanup failures.No changes needed here.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (3)
8-76: Tests correctly adoptSSHCommandRunOptions/SSHCommandRunResultThe updated import and the first test (
test_ssh_command_with_default_username) now constructSSHCommandRunOptions(direct=False)and assert both the type and fields ofSSHCommandRunResult. This matches the new client API and will catch regressions in how stdout/stderr/return_code are wired.Looks good.
87-233: Programmatic result/error handling is well coveredThe tests for:
- no default username,
- explicit
-loverride,- custom port and
TcpPortforwardAdapter,- direct connection via
tcp.address(),- and
FileNotFoundError→return_code == 127behaviorall exercise the new result object (
SSHCommandRunResult) thoroughly, including stdout/stderr plumbing and command argument construction.No issues here; this suite gives good confidence in the new return type behavior.
245-380: Option/command separation and username injection tests nicely constrain behaviorThe tests around:
- multiple SSH options,
- unknown options treated as part of the command,
- “all arguments are command” cases,
- and the subtle
-lin command vs SSH login flag scenariodo a solid job of pinning down
_separate_ssh_options_and_command_argsand the username injection logic.Given the refactor, these tests are especially valuable; no changes needed.
| if result != 0: | ||
| click.get_current_context().exit(result) | ||
| return result | ||
| self.logger.debug("SSH exit code: %s", result.return_code) |
There was a problem hiding this comment.
In the case of the CLI, we should pass the new option structure, and use direct i/o, i.e. not capturing stdout/stderr, and printing through click.
Otherwise plain "j ssh" (which drops you into a shell) won't work.
There was a problem hiding this comment.
True, sorry I missed it at first!
I tested it now, and it seems to work fine.
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
left another comment, please try the "j ssh" command
|
Thanks for the quick updates! |
aesteve-rh
force-pushed
the
ssh-wrapper-return
branch
from
d56492c to
97d2bca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(8 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(32 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(28-29)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
SSHCommandRunOptions(33-37)SSHCommandRunResult(17-29)run(88-121)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (4)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(42-52)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(28-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: build
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (5)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (4)
32-38: LGTM! Good extensibility design.The options dataclass follows the suggestion from previous reviews and provides a clean, extensible API for future enhancements without breaking changes.
57-76: LGTM! Interactive shell handling is correct.The
capture_output=bool(args)logic (line 62) elegantly solves the interactive shell concern raised in the PR comments. When no arguments are provided (interactive shell),capture_output=Falseallows direct terminal interaction. When arguments are provided, output is captured and echoed to the console.
88-122: LGTM! Fallback logic preserves capture settings correctly.The method signature update is clean, and the fallback path (lines 108-112) correctly preserves
capture_outputandcapture_as_textwhen falling back from direct connection to port forwarding.
257-271: LGTM! Consistent error handling.Converting
FileNotFoundErrorto aSSHCommandRunResultwith exit code 127 maintains API consistency—callers always receive a result object rather than handling exceptions for missing SSH binaries.packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (1)
8-694: LGTM! Comprehensive test coverage for the new API.All tests have been properly updated to use
SSHCommandRunOptionsandSSHCommandRunResult. The test suite covers:
- Default and overridden username scenarios
- Port forwarding and direct connection modes
- Identity file handling and cleanup
- Error scenarios (SSH not found, invalid options)
- Edge cases (multiple SSH options, unknown options, command
-lflag disambiguation)Mock configurations correctly provide
stdoutandstderr, and assertions validate the structured result objects.
aesteve-rh
force-pushed
the
ssh-wrapper-return
branch
from
97d2bca to
28433ef
Compare
| # When no args are provided, an interactive shell is implied. | ||
| # In this case, we must not capture stdout/stderr so the shell | ||
| # can interact with the terminal. | ||
| capture_output=bool(args), |
There was a problem hiding this comment.
| capture_output=bool(args), | |
| capture_output=False, |
The idea is that in either case it should behave like a regular ssh process just wrapped by us, tuneling stdin/stdout/stderr normally without waiting for the run to finish.
There was a problem hiding this comment.
i.e. if it's a long running process... an rsync... download, whatever we want it to behave as a regular ssh user@host command
There was a problem hiding this comment.
ah makes sense, let me change it.
aesteve-rh
force-pushed
the
ssh-wrapper-return
branch
from
28433ef to
9cbdd0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (2)
16-37: Result typing is inconsistent whencapture_as_text=False(stdout/stderr may be bytes)If a caller sets
SSHCommandRunOptions.capture_as_text=False,subprocess.run(..., text=False)will populateCompletedProcess.stdout/stderrasbytes, butSSHCommandRunResult.stdout/stderrare annotated asstrandfrom_completed_processpasses them through unchanged. That breaks the type contract and could surprise callers doing string operations.Consider either:
- Restricting usage for now by always keeping
capture_as_text=Trueand/or makingcapture_as_textinternal, or- Widening
SSHCommandRunResult.stdout/stderrto acceptstr | bytes(and documenting the relationship tocapture_as_text), or- Decoding bytes to text with an explicit encoding before storing them.
48-75: CLI options preserve interactivej sshbehavior while still surfacing structured resultsUsing
SSHCommandRunOptions(direct=direct, capture_output=False)in the CLI ensures SSH runs with normal stdin/stdout/stderr streaming, so interactive shells and long‑running commands behave like plainssh. TheSSHCommandRunResultstill carries the exit code, and the stderr echoing covers synthetic error cases such as theFileNotFoundErrorpath.Functionally this looks good and addresses the concern about breaking interactive
j sshbehavior. If you keep the echo blocks only for the error path, a short comment near them clarifying that CLI currently doesn’t capture stdout/stderr could help future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(8 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(32 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(28-29)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
SSHCommandRunOptions(33-37)SSHCommandRunResult(17-29)run(87-120)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (4)
packages/jumpstarter/jumpstarter/client/base.py (1)
call(42-52)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(28-29)
🔇 Additional comments (5)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
87-121: run() options threading and direct→port‑forward fallback look correctThe new
run(self, options: SSHCommandRunOptions, args)keeps the public surface clear and threadsoptionscleanly through both the direct and port‑forward paths. On direct failures you preservecapture_output/capture_as_textwhen falling back to SSH port forwarding, which avoids surprising callers.The recursive fallback (
self.run(SSHCommandRunOptions(direct=False, ...), args)) will terminate because the second call always goes through the non‑direct branch. This flow looks sound.
122-165: Identity temp‑file lifecycle and SSH command construction/logging are robustThe
_run_ssh_localchanges properly:
- Create a temporary identity file with strict permissions,
- Log only the file path (not key contents),
- Ensure cleanup in a
finallyblock, logging a warning but not failing the SSH run ifos.unlinkraises.Combined with the debug logging of parsed SSH options, command args, and the final SSH argv, this should make diagnosing SSH issues much easier without obvious security regressions. The associated tests around creation errors and cleanup warnings in
driver_test.pycover these paths well.Also applies to: 236-237, 253-253
256-270: _execute_ssh_command() correctly wraps subprocess and surfaces a structured “command not found” errorMapping
subprocess.runintoSSHCommandRunResultand turningFileNotFoundErrorinto a deterministicreturn_code=127with a descriptivestderrstring is a nice improvement over raising. This integrates cleanly with both the CLI (which printsstderrin this case) and programmatic callers that can branch onresult.return_code.packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
8-586: Tests now align cleanly with the new run() API and result typeUpdating tests to import
SSHCommandRunOptions/SSHCommandRunResult, callclient.run(options, args), and assert onresult.return_code/stdout/stderracross username injection, port forwarding, direct connections, and identity configurations keeps them in lock‑step with the new client API. The assertions on the constructed SSH argv are still thorough and make it easy to catch regressions in option parsing.
588-694: Good coverage of identity temp‑file error and cleanup‑warning pathsThe tests around identity temp‑file creation failure and cleanup failure (
test_ssh_identity_temp_file_creation_error,test_ssh_identity_temp_file_cleanup_error) do a nice job verifying that:
- Creation errors propagate as expected (wrapped in
ExceptionGroup),- Cleanup failures only generate a warning via the client logger and do not break successful SSH runs.
This closely matches the new logging and cleanup behavior in
_run_ssh_local.
Add SSHCommandExecResult to wrap the subprocess SSH command return and use it as return type for SSHWrapperClient.run, so that the output of the command can be inspected after the call. Signed-off-by: Albert Esteve <aesteve@redhat.com>
Add SSHCommandRunOptions class and modify the run method signature to use it. This will make it easier to change the options before having to change the signature in the future. Signed-off-by: Albert Esteve <aesteve@redhat.com>
Change logging formatting to use %-formatting so that formatting is deferred until the log is printed [1]. [1] https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/logging-fstring-interpolation.html Signed-off-by: Albert Esteve <aesteve@redhat.com>
aesteve-rh
force-pushed
the
ssh-wrapper-return
branch
from
9cbdd0d to
627d1fc
Compare
|
Thank you @aesteve-rh !!! :) |
|
Successfully created backport PR for |
2 participants
Add SSHCommandExecResult to wrap the subprocess SSH command
return and use it as return type for SSHWrapperClient.run,
so that the output of the command can be inspected after
the call.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.