Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds a new jumpstarter-driver-ssh package (driver, client, tests, README, example, pyproject), registers it in the workspace pyproject, and updates docs to list and include an SSH driver reference page. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant CLI as CLI (Click)
participant C as SSHWrapperClient
participant D as SSHWrapper Driver
participant TCP as TcpNetwork (child)
participant PF as TcpPortforwardAdapter
participant SSH as Local ssh
U->>CLI: jumpstarter ssh [--direct] [args...]
CLI->>C: invoke command(args, direct)
C->>D: read config (default_username, ssh_command) & child "tcp"
alt direct mode
C->>TCP: resolve direct address
TCP-->>C: host:port or parse error
opt parse error
C->>PF: request port-forward
PF-->>C: local host:port
end
else forwarded mode
C->>PF: request port-forward
PF-->>C: local host:port
end
C->>C: preprocess args (handle user@host and -l)
C->>SSH: exec ssh -p <port> [-l user] <host> [opts] [cmd]
SSH-->>C: exit code
C-->>CLI: return code
CLI-->>U: process exit
sequenceDiagram
autonumber
participant C as SSHWrapperClient
note over C: Fallback from direct parse to forwarding
C->>TcpNetwork: parse direct address
TcpNetwork-->>C: parse failure
C->>TcpPortforwardAdapter: start port-forward
TcpPortforwardAdapter-->>C: local host:port
C->>ssh: execute ssh with forwarded port
ssh-->>C: exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/source/reference/package-apis/drivers/ssh.md (1)
89-91: Default ssh_command in docs mismatches codeDriver defaults include “-o LogLevel=ERROR”, but the docs omit it. Sync the default to avoid confusion.
-- `ssh_command`: SSH command to use (default: "ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null") +- `ssh_command`: SSH command to use (default: "ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -o LogLevel=ERROR")
🧹 Nitpick comments (9)
packages/jumpstarter-driver-ssh/pyproject.toml (1)
27-33: pytest config is fine; consider pinning to 8.4.xYou’re on pytest>=8.3.3 here while the repo is generally on recent 8.4.x. Aligning avoids plugin surprises (async handling changed in 8.4).
Optionally bump:
[dependency-groups] dev = [ - "pytest-cov>=6.0.0", - "pytest>=8.3.3", + "pytest-cov>=6.0.0", + "pytest>=8.4.2", ]docs/source/reference/package-apis/drivers/ssh.md (1)
61-63: Document security implications of defaultsStrictHostKeyChecking=no and UserKnownHostsFile=/dev/null disable host key verification. Add a short warning and advice to override in production.
packages/jumpstarter-driver-ssh/examples/exporter.yaml (1)
6-8: Avoid committing real tokensGood that this is a placeholder. Add a brief comment indicating how to source the token (env var, secret manager) to prevent accidental check-ins.
packages/jumpstarter-driver-ssh/README.md (1)
77-79: Add a security note about SSH defaultsClarify that the defaults disable host key verification and are intended for lab/test usage; recommend overriding for production.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (2)
14-17: Super call check is unnecessaryDriver already defines post_init; you can call super().post_init() directly.
- def __post_init__(self): - if hasattr(super(), "__post_init__"): - super().__post_init__() + def __post_init__(self): + super().__post_init__()
25-33: Annotate return types for exported accessorsMinor clarity/readability improvement.
- def get_default_username(self): + def get_default_username(self) -> str: """Get default SSH username""" return self.default_username - def get_ssh_command(self): + def get_ssh_command(self) -> str: """Get the SSH command to use""" return self.ssh_commandpackages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (3)
12-16: Replace placeholder test with skipMark as skipped to avoid a misleading pass and to document the missing get_driver API surface.
def test_drivers_ssh(): """Test that the SSH driver is properly registered""" - # Skip this test for now as get_driver is not available - pass + pytest.skip("get_driver registry not available in this package")
46-53: Assert specific exception typeThe driver raises ConfigurationError; assert that explicitly for accuracy.
-from pytest import raises -with pytest.raises(Exception): # Should raise ConfigurationError +from jumpstarter.common.exceptions import ConfigurationError +with pytest.raises(ConfigurationError): SSHWrapper( children={}, # Missing tcp child default_username="" )
320-339: Precedence test for -l vs user@host is helpfulEnsures explicit -l wins. Consider adding an assertion that multiple -l flags don’t reorder or drop values inadvertently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/ssh.md(1 hunks)packages/jumpstarter-driver-ssh/README.md(1 hunks)packages/jumpstarter-driver-ssh/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(1 hunks)packages/jumpstarter-driver-ssh/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-29T11:52:43.554Z
Learnt from: bennyz
PR: jumpstarter-dev/jumpstarter#241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.
Applied to files:
pyproject.toml
🧬 Code graph analysis (4)
packages/jumpstarter/jumpstarter/client/client.py (1)
packages/jumpstarter/jumpstarter/common/grpc.py (1)
configure_grpc_env(71-78)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-269)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (5)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(22-23)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
CompositeClient(28-60)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (2)
SSHWrapper(8-33)client(22-23)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (2)
SSHWrapperClient(50-245)run_ssh_command(71-100)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
FileNotFoundError(66-69)
🪛 GitHub Check: ruff
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
[failure] 212-212: Ruff (W293)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:212:1: W293 Blank line contains whitespace
[failure] 204-204: Ruff (W291)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:204:90: W291 Trailing whitespace
[failure] 191-191: Ruff (W291)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:191:72: W291 Trailing whitespace
[failure] 170-170: Ruff (E501)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:170:121: E501 Line too long (219 > 120)
[failure] 156-156: Ruff (W293)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:156:1: W293 Blank line contains whitespace
[failure] 147-147: Ruff (W293)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:147:1: W293 Blank line contains whitespace
[failure] 139-139: Ruff (W291)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:139:90: W291 Trailing whitespace
[failure] 130-130: Ruff (W293)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:130:1: W293 Blank line contains whitespace
[failure] 106-106: Ruff (W293)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:106:1: W293 Blank line contains whitespace
[failure] 102-102: Ruff (C901)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:102:9: C901 _run_ssh_local is too complex (29 > 10)
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
[error] 102-102: ruff check failed: C901 '_run_ssh_local' is too complex (29 > 10)
⏰ 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: e2e
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (10)
packages/jumpstarter/jumpstarter/client/client.py (1)
12-19: LGTM: configuring gRPC before channel setup.Thanks for front-loading the environment configuration so every channel gets the intended log hygiene without relying on callers.
pyproject.toml (1)
31-31: Workspace source entry LGTMAdding jumpstarter-driver-ssh to the UV workspace sources is correct and aligns with the new package.
packages/jumpstarter-driver-ssh/pyproject.toml (1)
11-17: Runtime deps look minimal and appropriateanyio/click/jumpstarter/composite/network are the right deps for this client/driver split. No issues.
If jumpstarter-driver-composite is truly required at runtime (not just for tests/CLI wiring), keep it here; otherwise consider moving it to dev to reduce install surface.
docs/source/reference/package-apis/drivers/index.md (1)
83-84: Docs catalog update LGTMSSH entry and toctree inclusion are consistent and correctly categorized.
Also applies to: 105-105
packages/jumpstarter-driver-ssh/examples/exporter.yaml (1)
9-19: Example structure looks correctExporter -> ssh -> tcp layout matches the driver’s requirement for a tcp child. Defaults are sensible.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (2)
18-20: Validation for required child is goodEnforcing presence of tcp child at init time prevents misconfiguration later.
11-13: Default includes LogLevel=ERROR — goodSuppressing SSH noise by default is consistent with CLI UX; docs should match.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (3)
68-96: Test intent is solidCovers default username injection, host placement, and command preservation. No changes requested.
157-195: Port handling via forward adapter covered wellGood coverage for -p injection and host placement.
228-244: Error handling test is correctReturning 127 on missing SSH binary matches common conventions.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
173-207: Handle more SSH options (-J, -t/-tt, -4/-6, -vvv, long options) to avoid misclassifying them as remote commandsThe rigid whitelist misses valid flags, causing options like -J, -tt, -vvv, -4/-6 to be treated as the start of the remote command. Generalize parsing.
Apply these diffs:
Import regex support:
-import shlex +import shlex +import reGeneralize option parsing:
def _separate_ssh_options_and_command_args(self, processed_args): """Separate SSH options from command arguments""" - ssh_options = [] - command_args = [] - i = 0 + ssh_options = [] + command_args = [] + # Options that take a value as the next token + opts_with_value = { + "-l", "-p", "-o", "-i", "-F", "-c", "-m", "-D", "-E", "-L", + "-O", "-P", "-Q", "-R", "-S", "-W", "-w", "-J", "-b", "-B", + } + no_value_flags = { + "-4", "-6", "-t", "-tt", "-A", "-a", "-C", "-f", "-G", "-g", + "-K", "-k", "-M", "-N", "-n", "-q", "-T", "-X", "-Y", "-v", + } + i = 0 while i < len(processed_args): arg = processed_args[i] - if arg.startswith('-'): - # Check if it's a known SSH option - if arg in [ - '-l', '-p', '-o', '-i', '-F', '-c', '-m', '-v', '-q', '-X', '-Y', - '-N', '-T', '-f', '-n', '-s', '-A', '-B', '-C', '-D', '-E', '-G', - '-K', '-L', '-M', '-O', '-P', '-Q', '-R', '-S', '-W', '-w' - ]: - # This is an SSH option - ssh_options.append(arg) - # If this option takes a value, add the next argument too - if i + 1 < len(processed_args) and not processed_args[i + 1].startswith('-'): - ssh_options.append(processed_args[i + 1]) - i += 1 - else: - # This is a command argument - everything from here on is part of the command - command_args = processed_args[i:] - break + if arg.startswith("-"): + # Accept long options and clusters like -vvv + if arg.startswith("--") or re.fullmatch(r"-v+", arg) or arg in no_value_flags or arg in opts_with_value: + ssh_options.append(arg) + if arg in opts_with_value and i + 1 < len(processed_args): + ssh_options.append(processed_args[i + 1]) + i += 1 + i += 1 + continue + # Unknown dash-prefixed token: still treat as an option for robustness + ssh_options.append(arg) + i += 1 + continue else: # This is a command argument - everything from here on is part of the command command_args = processed_args[i:] break i += 1packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
373-401: Add coverage for missing-but-valid SSH flags (-J, -tt, -vvv, -4/-6)Current parsing likely misclassifies these as remote command tokens. Add tests to lock desired behavior and guide parser updates.
Example additions:
def test_ssh_with_proxyjump_and_verbose(serve, SSHWrapper, TcpNetwork, mocker): instance = SSHWrapper(children={"tcp": TcpNetwork(host="127.0.0.1", port=22)}, default_username="u") with serve(instance) as client, mocker.patch("subprocess.run") as run, \ patch("jumpstarter_driver_ssh.client.TcpPortforwardAdapter", asynccontextmanager(lambda **_: (lambda: (_ for _ in ()).throw(RuntimeError("stub"))))): run.return_value = MagicMock(returncode=0) result = client.run_ssh_command(False, ["-J", "bastion", "-vvv", "hostname"]) assert result == 0 argv = run.call_args[0][0] assert "-J" in argv and "bastion" in argv assert any(a.startswith("-vv") for a in argv) assert "hostname" in argv def test_ssh_with_tty_and_ipv6_flag(mocker): instance = SSHWrapper(children={"tcp": TcpNetwork(host="127.0.0.1", port=22)}, default_username="") with serve(instance) as client, mocker.patch("subprocess.run") as run: run.return_value = MagicMock(returncode=0) result = client.run_ssh_command(False, ["-tt", "-6", "whoami"]) assert result == 0 argv = run.call_args[0][0] assert "-tt" in argv and "-6" in argv assert "whoami" in argv
71-99: Optional: assert exact argv ordering to harden behaviorTests only assert presence. Consider asserting ordering (ssh options, host, then remote command) to catch regressions in assembly semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/ssh.md(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/jmp.py(1 hunks)packages/jumpstarter-driver-ssh/README.md(1 hunks)packages/jumpstarter-driver-ssh/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(1 hunks)packages/jumpstarter-driver-ssh/pyproject.toml(1 hunks)packages/jumpstarter/jumpstarter/client/client.py(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/reference/package-apis/drivers/ssh.md
🚧 Files skipped from review as they are similar to previous changes (7)
- pyproject.toml
- docs/source/reference/package-apis/drivers/index.md
- packages/jumpstarter-driver-ssh/pyproject.toml
- packages/jumpstarter-driver-ssh/examples/exporter.yaml
- packages/jumpstarter-driver-ssh/README.md
- packages/jumpstarter/jumpstarter/client/client.py
- packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py
🧰 Additional context used
🧬 Code graph analysis (3)
packages/jumpstarter-cli/jumpstarter_cli/jmp.py (1)
packages/jumpstarter/jumpstarter/common/grpc.py (1)
configure_grpc_env(71-78)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (2)
client(22-23)SSHWrapper(8-33)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (2)
SSHWrapperClient(36-259)run_ssh_command(57-86)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
FileNotFoundError(66-69)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (5)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(22-23)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
CompositeClient(28-60)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
⏰ 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: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (6)
packages/jumpstarter-cli/jumpstarter_cli/jmp.py (1)
17-21: Great call configuring gRPC on importInitializing the gRPC environment before the Click group is created keeps the CLI quiet and avoids the SSL chatter we were seeing. The helper is idempotent, so doing it at import time is safe.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
74-76: Guard against recursive fallback loops and preserve original error contextIf direct mode fails with a non-address issue, recursive fallback might mask root cause. Consider logging exc_info and ensuring the fallback path can’t re-enter here.
Minimal tweak:
- except (DriverMethodNotImplemented, ValueError) as e: - self.logger.error(f"Direct address connection failed ({e}), falling back to SSH port forwarding") + except (DriverMethodNotImplemented, ValueError) as e: + self.logger.error("Direct address connection failed, falling back to SSH port forwarding", exc_info=True) return self.run_ssh_command(False, args)Please confirm the port-forward path cannot trigger the same exception cycle.
78-86: Fix: TcpPortforwardAdapter is async; currentwithwill crash
TcpPortforwardAdapteris an async context manager. Usingwithraises TypeError and the port-forward never opens. Drive it via the portal or wrap it with a sync context manager.Apply this diff to use a sync wrapper:
- # Use SSH port forwarding (default behavior) - self.logger.debug("Using SSH port forwarding for SSH connection") - with TcpPortforwardAdapter( - client=self.tcp, - ) as addr: - host = addr[0] - port = addr[1] - self.logger.debug(f"SSH port forward established - host: {host}, port: {port}") - return self._run_ssh_local(host, port, ssh_command, default_username, args) + # Use SSH port forwarding (default behavior) + self.logger.debug("Using SSH port forwarding for SSH connection") + with self._portforward_sync() as (host, port): + self.logger.debug(f"SSH port forward established - host: {host}, port: {port}") + return self._run_ssh_local(host, port, ssh_command, default_username, args)Add this helper to the class (outside the selected range):
from contextlib import contextmanager @contextmanager def _portforward_sync(self): # Enter the async context on the portal's loop and keep it open until __exit__ cm_holder = {} def _enter(): async def _runner(): cm = TcpPortforwardAdapter(client=self.tcp) addr = await cm.__aenter__() cm_holder["cm"] = cm return addr return self.portal.call_async(_runner) addr = self.portal.call(_enter) try: yield addr finally: def _exit(): async def _runner(): cm = cm_holder.get("cm") if cm: await cm.__aexit__(None, None, None) return self.portal.call_async(_runner) self.portal.call(_exit)
240-246: Replace/remove placeholder hostname when injecting the real hostIf the user typed a placeholder host (e.g., “device”), we must drop it when appending the real host to avoid running
ssh <real> device ....Apply this diff:
- # Add hostname before command arguments - if host and not hostname_replaced: - ssh_args.append(host) - - # Add command arguments - ssh_args.extend(command_args) + # Add hostname before command arguments + if host and not hostname_replaced: + # Drop a leading placeholder host token (plain hostname) + if command_args and not command_args[0].startswith("-") and not any(c in command_args[0] for c in ("@", ":", "/")): + command_args = command_args[1:] + ssh_args.append(host) + + # Add command arguments + ssh_args.extend(command_args)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (2)
171-175: Tests mock an async context manager as sync; update to async-aware mocksThese tests patch
TcpPortforwardAdapterwith sync__enter__/__exit__, cementing the incorrect API and will fail once the client fixes the adapter usage.Use an asynccontextmanager stub:
from contextlib import asynccontextmanager @asynccontextmanager async def fake_adapter(*args, **kwargs): yield ("127.0.0.1", 2222) # or desired host, port with patch("jumpstarter_driver_ssh.client.TcpPortforwardAdapter", fake_adapter): ...Also applies to: 415-418, 486-489, 521-524, 556-559
312-342: Verify expectation: should “hostname” be preserved here?Input is ["-l", "explicituser", "otheruser@hostname"] (no remote command). Expect the placeholder host (“hostname”) to be replaced by the real forwarded host, not preserved as a command arg. Please confirm and adjust assertions accordingly.
mangelajo
force-pushed
the
ssh-wrap
branch
2 times, most recently
from
fc2cdbf to
abcfa6d
Compare
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Some tests need fixing.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py
Outdated
Show resolved
Hide resolved
mangelajo
force-pushed
the
ssh-wrap
branch
2 times, most recently
from
91be14b to
23dc13a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/ssh.md(1 hunks)packages/jumpstarter-driver-ssh/README.md(1 hunks)packages/jumpstarter-driver-ssh/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(1 hunks)packages/jumpstarter-driver-ssh/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/source/reference/package-apis/drivers/ssh.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/jumpstarter-driver-ssh/examples/exporter.yaml
- packages/jumpstarter-driver-ssh/README.md
- pyproject.toml
- packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py
- docs/source/reference/package-apis/drivers/index.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T05:52:27.137Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#647
File: packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:0-0
Timestamp: 2025-09-25T05:52:27.137Z
Learning: In the SSHWrapper driver's client.py, hostname replacement should only occur for "userhostname" format arguments that get preprocessed into "-l user hostname". Regular hostname arguments (like "device" in SSH commands) should be passed through unchanged to allow the wrapper to handle connections internally.
Applied to files:
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (5)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(22-23)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
CompositeClient(28-60)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-269)
⏰ 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). (3)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (2)
packages/jumpstarter-driver-ssh/pyproject.toml (1)
23-25: Correct the package source URL.The
source_archivestill targets the placeholderjumpstarter-dev/repo, so consumers will follow a broken link. Point it to the actual Jumpstarter repository.[tool.hatch.metadata.hooks.vcs.urls] Homepage = "https://jumpstarter.dev" -source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" +source_archive = "https://github.com/jumpstarter-dev/jumpstarter/archive/{commit_hash}.zip"packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
18-20: Nice early configuration guardChecking for the required
tcpchild up front gives users immediate feedback on misconfigurations. Nicely done.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (3)
49-65: Assert on the executed argv, not just membership.Right now we only check that
mock_runwas called and that a few tokens exist. Verifying the exact argument order (e.g., usingassert mock_run.call_args[0][0] == [...]) would protect against regressions in how we build the SSH command. Please tighten these assertions.
323-337: Invaliduser@host@extrainput should probably raise.Silently coercing this string into
-l user host@extrais surprising. Consider asserting that we bubble an error (or leave the arg untouched) so callers notice malformed hosts instead of running unexpected commands.
381-404: Usecontextlibwhen fakingTcpPortforwardAdapter.The adapter is supposed to be a proper (a)sync context manager. Returning a tuple directly bypasses that protocol. Wrap your stub in
contextlib.contextmanagerso we still exercise__enter__/__exit__.packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (2)
11-19: Call the parent__post_init__unconditionally.The base
Driveralready defines__post_init__; thehasattrcheck is unnecessary and risks skipping initialization if it ever gets shadowed. Just invokesuper().__post_init__()directly.
18-19: Include driver name in the error to aid debugging.When the
tcpchild is missing, surfacing the driver name (SSHWrapper) in the exception message helps operators trace config issues. Consider extending the message accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/ssh.md(1 hunks)packages/jumpstarter-driver-ssh/README.md(1 hunks)packages/jumpstarter-driver-ssh/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(1 hunks)packages/jumpstarter-driver-ssh/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/source/reference/package-apis/drivers/ssh.md
- packages/jumpstarter-driver-ssh/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/jumpstarter-driver-ssh/pyproject.toml
- docs/source/reference/package-apis/drivers/index.md
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T05:52:27.137Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#647
File: packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:0-0
Timestamp: 2025-09-25T05:52:27.137Z
Learning: In the SSHWrapper driver's client.py, hostname replacement should only occur for "userhostname" format arguments that get preprocessed into "-l user hostname". Regular hostname arguments (like "device" in SSH commands) should be passed through unchanged to allow the wrapper to handle connections internally.
Applied to files:
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.pypackages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
🧬 Code graph analysis (3)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (2)
SSHWrapper(8-33)client(22-23)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
run(65-94)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
FileNotFoundError(66-69)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-269)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (5)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(22-23)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
CompositeClient(28-60)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
⏰ 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). (9)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (6)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (4)
522-546: Guard against leaking placeholder tokens into argv.Even for “special formats” we should ensure the resolved host replaces the placeholder when the username came from
user@host. Otherwise we invokessh … host hostname:port …, which is not a valid remote command. Please tighten the test accordingly.
407-439: Same as above—remove the placeholder host.The expectation that both
"10.0.0.5"and"hostname"remain contradicts SSH semantics. Please assert that the placeholder disappears after replacement so the final argv is valid.
441-474: Ditto for the placeholder host.Keeping
"hostname"after adding"172.16.0.10"causes the wrapper to runhostnameas the remote command. Update the test to make sure we emit only the real host plus any user-specified commands.
493-507: Drop the placeholder once the tunnel host is known.Here again we should expect only
"192.168.0.1"(plus any commands) in the final argv; otherwise SSH interprets"hostname"as the command to run. Please correct the test to reflect that.packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (2)
181-214: Short options like-p2222and-vvare misparsed.Because we compare tokens against a literal whitelist, merged flags (
-p2222,-vvv,-J,--ProxyCommand, etc.) drop intocommand_args. The resulting argv becomesssh … <host> -p2222 …, so SSH misinterprets the option as a remote command. Please switch to a generic option parser (respecting--) that consumes short/long forms without hardcoding every flag.
248-253: Preserve placeholder removal when appending the real host.If
hostname_replacedis still False but the first command arg is just the placeholder host, we need to pop it before appending the resolved host; otherwise we executessh … host placeholder …and SSH treatsplaceholderas a remote command. Drop or overwrite that arg before extending.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py
Outdated
Show resolved
Hide resolved
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py
Outdated
Show resolved
Hide resolved
|
Stop parsing the user@host and take anything after non-ssh flags as the command start. otherwise things like... Also verify that cases like this will work : j ssh ls -l root -la |
mangelajo
force-pushed
the
ssh-wrap
branch
2 times, most recently
from
8550eb1 to
bcdc5c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
74-87: Ordering causes double parsing of args; minor overhead.You call _separate_ssh_options_and_command_args twice. Cache once and pass through to helpers.
- ssh_args = self._build_ssh_command_args(ssh_command, port, default_username, args) - # Separate SSH options from command arguments - ssh_options, command_args = self._separate_ssh_options_and_command_args(args) + ssh_options, command_args = self._separate_ssh_options_and_command_args(args) + ssh_args = self._build_ssh_command_args(ssh_command, port, default_username, args)And change _build_ssh_command_args to accept precomputed ssh_options to avoid recomputation.
93-107: Username detection can misread malformed “-l” usage.If users pass “-l” as the last token (no value), we still suppress default username injection. Prefer stricter check for “-l ”.
- has_user_flag = any( - ssh_options[i] == "-l" and i + 1 < len(ssh_options) - for i in range(len(ssh_options)) - ) + has_user_flag = any( + ssh_options[i] == "-l" and i + 1 < len(ssh_options) and not ssh_options[i + 1].startswith("-") + for i in range(len(ssh_options)) + )
165-169: Confirm intended behavior: first positional token becomes remote command, not host.Given wrapper design, “j ssh hostname” runs remote command “hostname” against the tunneled host. If that’s intentional, consider a short note in the README to prevent confusion for users accustomed to “ssh host [cmd]”.
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (3)
138-142: Stub the adapter with a lightweight contextmanager for clarity.Using MagicMock works, but a small contextmanager improves readability and mirrors the contract.
- with patch('jumpstarter_driver_ssh.client.TcpPortforwardAdapter') as mock_adapter: - mock_adapter.return_value.__enter__.return_value = ("127.0.0.1", 2222) - mock_adapter.return_value.__exit__.return_value = None + from contextlib import contextmanager + @contextmanager + def _fake_adapter(**_): + yield ("127.0.0.1", 2222) + with patch('jumpstarter_driver_ssh.client.TcpPortforwardAdapter', _fake_adapter): # Test SSH command with custom port result = client.run(False, ["hostname"])
311-350: Add a test for the reported edge case “ls -l root -la”.You cover a similar case; add this exact sequence to lock behavior.
I can add a parametric test invoking client.run(False, ["ls", "-l", "root", "-la"]) and asserting: [-l testuser] before host, host present, and command args preserved after host.
251-281: Reconfirm expectation: unknown -flags start the command.This test codifies the desired behavior; keep it after parser tweaks. Also add a case for “-vvv” to ensure it remains an SSH option, not a command arg.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/ssh.md(1 hunks)packages/jumpstarter-driver-ssh/README.md(1 hunks)packages/jumpstarter-driver-ssh/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py(1 hunks)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py(1 hunks)packages/jumpstarter-driver-ssh/pyproject.toml(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-ssh/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/source/reference/package-apis/drivers/index.md
- packages/jumpstarter-driver-ssh/pyproject.toml
- packages/jumpstarter-driver-ssh/examples/exporter.yaml
- pyproject.toml
- packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-25T10:48:56.377Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#647
File: packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:71-94
Timestamp: 2025-09-25T10:48:56.377Z
Learning: TcpPortforwardAdapter has a decorator that allows both async and sync callers, so it can be used with both `with` (synchronous) and `async with` (asynchronous) context managers.
Applied to files:
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.pypackages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
📚 Learning: 2025-09-25T05:52:27.157Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#647
File: packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:0-0
Timestamp: 2025-09-25T05:52:27.157Z
Learning: In the SSHWrapper driver's client.py, hostname replacement should only occur for "userhostname" format arguments that get preprocessed into "-l user hostname". Regular hostname arguments (like "device" in SSH commands) should be passed through unchanged to allow the wrapper to handle connections internally.
Applied to files:
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.pypackages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver_test.py (4)
packages/jumpstarter-driver-network/jumpstarter_driver_network/driver.py (1)
TcpNetwork(88-121)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (2)
SSHWrapper(8-33)client(22-23)packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
run(43-72)packages/jumpstarter/jumpstarter/common/exceptions.py (1)
FileNotFoundError(66-69)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (5)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(22-23)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
CompositeClient(28-60)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)packages/jumpstarter/jumpstarter/client/base.py (1)
call(36-46)
⏰ 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). (5)
- 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: e2e
🔇 Additional comments (3)
docs/source/reference/package-apis/drivers/ssh.md (1)
1-1: Replace the bare README path with an include or linkLeaving
../../../../../packages/jumpstarter-driver-ssh/README.mdas plain text renders the raw path instead of surfacing the README content or a navigable link. Wrap it in a MyST include (with the relative options) or convert it into a Markdown link so the page renders correctly.-../../../../../packages/jumpstarter-driver-ssh/README.md +```{include} ../../../../../packages/jumpstarter-driver-ssh/README.md +:relative-docs: true +:relative-images: true +```packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (2)
66-72: LGTM: Sync context for TcpPortforwardAdapter.Per project note, adapter supports both sync and async usage; this is acceptable.
112-157: Option parsing misclassifies common SSH flags (-vv/-vvv, -tt) and misses --/combined -o= forms.Today, “-vv”/“-vvv”/“-tt” become remote command args and “-oStrictHostKeyChecking=no” is not recognized, breaking normal SSH usage. Also, “--” should stop option parsing. Keep your desired behavior of stopping at the first non-SSH flag, but recognize these cases.
Apply this diff:
def _separate_ssh_options_and_command_args(self, args): """Separate SSH options from command arguments""" # SSH flags that do not expect a parameter (simple flags) ssh_flags_no_param = { - '-4', '-6', '-A', '-a', '-C', '-f', '-G', '-g', '-K', '-k', '-M', '-N', - '-n', '-q', '-s', '-T', '-t', '-V', '-v', '-X', '-x', '-Y', '-y' + '-4', '-6', '-A', '-a', '-C', '-f', '-G', '-g', '-K', '-k', '-M', '-N', + '-n', '-q', '-s', '-T', '-t', '-V', '-v', '-X', '-x', '-Y', '-y', + # Common repeated short flags + '-vv', '-vvv', '-tt', } # SSH flags that do expect a parameter ssh_flags_with_param = { '-B', '-b', '-c', '-D', '-E', '-e', '-F', '-I', '-i', '-J', '-L', '-l', '-m', '-O', '-o', '-P', '-p', '-Q', '-R', '-S', '-W', '-w' } ssh_options = [] command_args = [] i = 0 while i < len(args): arg = args[i] + # End of options sentinel + if arg == "--": + command_args = args[i + 1:] + break if arg.startswith('-'): # Check if it's a known SSH option - if arg in ssh_flags_no_param: + # Support combined -oKey=Value form + if arg.startswith('-o') and arg != '-o': + ssh_options.append(arg) + elif arg in ssh_flags_no_param: # This is a simple SSH flag without parameter ssh_options.append(arg) elif arg in ssh_flags_with_param: # This is an SSH flag that expects a parameter ssh_options.append(arg) # If this option takes a value, add the next argument too if i + 1 < len(args) and not args[i + 1].startswith('-'): ssh_options.append(args[i + 1]) i += 1 else: # This is a command argument - everything from here on is part of the command command_args = args[i:] break else: # This is a command argument - everything from here on is part of the command command_args = args[i:] break i += 1This preserves your intent (“unknown SSH-like flags trigger command mode”) while fixing common SSH flows. Based on learnings
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (1)
174-184: Optional: capture return code and emit clearer errorsCurrent subprocess.run is fine. If desired, use check=False and text mode with env inheritance explicitly for clearer diagnostics; not blocking.
📜 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(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-25T10:48:56.377Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#647
File: packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:71-94
Timestamp: 2025-09-25T10:48:56.377Z
Learning: TcpPortforwardAdapter has a decorator that allows both async and sync callers, so it can be used with both `with` (synchronous) and `async with` (asynchronous) context managers.
Applied to files:
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
📚 Learning: 2025-09-25T05:52:27.157Z
Learnt from: mangelajo
PR: jumpstarter-dev/jumpstarter#647
File: packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py:0-0
Timestamp: 2025-09-25T05:52:27.157Z
Learning: In the SSHWrapper driver's client.py, hostname replacement should only occur for "userhostname" format arguments that get preprocessed into "-l user hostname". Regular hostname arguments (like "device" in SSH commands) should be passed through unchanged to allow the wrapper to handle connections internally.
Applied to files:
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (4)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/driver.py (1)
client(22-23)packages/jumpstarter-driver-composite/jumpstarter_driver_composite/client.py (1)
CompositeClient(28-60)packages/jumpstarter-driver-network/jumpstarter_driver_network/adapters/portforward.py (1)
TcpPortforwardAdapter(20-32)packages/jumpstarter/jumpstarter/client/core.py (1)
DriverMethodNotImplemented(39-42)
⏰ 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). (6)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
🔇 Additional comments (3)
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py (3)
21-31: CLI shape looks right for the “don’t parse user@host” goalignore_unknown_options + variadic args ensures cases like “j ssh ls -l root -la” are treated as a remote command, not SSH options or user/host. Good.
Please add/confirm a test for:
- j ssh ls -l root -la → ssh -- ls -l root -la (i.e., no attempt to parse “root” as user/host).
66-72: Sync use of TcpPortforwardAdapter is acceptable hereGiven the decorator that supports both sync and async context managers, this “with … as addr” usage won’t raise TypeError and keeps the tunnel open while ssh runs. Based on learnings.
112-157: Fix SSH option parsing: support -vvv, -p2222, -L8080:..., -oUser=..., and “--” sentinelThe current whitelist approach misclassifies common flags as command args (e.g., -vvv, -p2222, -L8080:..., -Jjump, -oUser=foo). This breaks typical SSH usage.
Apply this diff to replace the parser with a generic, SSH-friendly one that:
- treats any leading “-” token as an SSH option until the first non-option or “--”,
- recognizes inline values for options that take a value (-p2222, -L8080:host:80, -Whost:port, -Juser@host:port, -oUser=foo),
- accepts grouped no-param flags like -vvv or -NT,
- stops at “--” and does not pass it to the remote command.
@@ - def _separate_ssh_options_and_command_args(self, args): - """Separate SSH options from command arguments""" - # SSH flags that do not expect a parameter (simple flags) - ssh_flags_no_param = { - '-4', '-6', '-A', '-a', '-C', '-f', '-G', '-g', '-K', '-k', '-M', '-N', - '-n', '-q', '-s', '-T', '-t', '-V', '-v', '-X', '-x', '-Y', '-y' - } - - # SSH flags that do expect a parameter - ssh_flags_with_param = { - '-B', '-b', '-c', '-D', '-E', '-e', '-F', '-I', '-i', '-J', '-L', '-l', - '-m', '-O', '-o', '-P', '-p', '-Q', '-R', '-S', '-W', '-w' - } - - ssh_options = [] - command_args = [] - i = 0 - while i < len(args): - arg = args[i] - if arg.startswith('-'): - # Check if it's a known SSH option - if arg in ssh_flags_no_param: - # This is a simple SSH flag without parameter - ssh_options.append(arg) - elif arg in ssh_flags_with_param: - # This is an SSH flag that expects a parameter - ssh_options.append(arg) - # If this option takes a value, add the next argument too - if i + 1 < len(args) and not args[i + 1].startswith('-'): - ssh_options.append(args[i + 1]) - i += 1 - else: - # This is a command argument - everything from here on is part of the command - command_args = args[i:] - break - else: - # This is a command argument - everything from here on is part of the command - command_args = args[i:] - break - i += 1 - - # Debug output - self.logger.debug(f"SSH options: {ssh_options}") - self.logger.debug(f"Command args: {command_args}") - return ssh_options, command_args + def _separate_ssh_options_and_command_args(self, args): + """Separate SSH options from command arguments, supporting inline/clustered flags.""" + # Options that take a value (support both split and inline forms, e.g. -p 2222 or -p2222) + with_value = { + "-B", "-b", "-c", "-D", "-E", "-e", "-F", "-I", "-i", "-J", + "-L", "-l", "-m", "-O", "-o", "-P", "-p", "-Q", "-R", "-S", "-W", "-w", + } + # Options without value that can be clustered (e.g., -vvv, -NT) + no_value = { + "-4", "-6", "-A", "-a", "-C", "-f", "-G", "-g", "-K", "-k", "-M", "-N", + "-n", "-q", "-s", "-T", "-t", "-V", "-v", "-X", "-x", "-Y", "-y", + } + + ssh_options: list[str] = [] + command_args: list[str] = [] + + i = 0 + while i < len(args): + arg = args[i] + + # Stop parsing options at explicit sentinel + if arg == "--": + command_args = args[i + 1 :] + break + + # First non-option starts the remote command + if not arg.startswith("-") or arg == "-": + command_args = args[i:] + break + + # Pass through any double-dash options as-is (ssh may reject; user’s choice) + if arg.startswith("--"): + ssh_options.append(arg) + i += 1 + continue + + # Handle inline value short options: -p2222, -L8080:host:80, -Whost:port, -Juser@host:port, -oUser=val + key = arg[:2] + rest = arg[2:] + if key in with_value and rest: + ssh_options.append(arg) # keep combined token intact + i += 1 + continue + + # Handle clusters of no-value flags, e.g., -vvv or -NT + if len(arg) > 2 and all(f"-{ch}" in no_value for ch in arg[1:]): + ssh_options.append(arg) + i += 1 + continue + + # Fallback: single short option, possibly followed by a separate value + ssh_options.append(key if key in with_value else arg) + if key in with_value and not rest: + # Consume next token as the value (even if it starts with '-'; ssh allows values like '-') + if i + 1 < len(args): + ssh_options.append(args[i + 1]) + i += 1 + i += 1 + + self.logger.debug(f"SSH options: {ssh_options}") + self.logger.debug(f"Command args: {command_args}") + return ssh_options, command_args
| # Separate SSH options from command arguments | ||
| ssh_options, command_args = self._separate_ssh_options_and_command_args(args) | ||
|
|
||
| # Build final SSH command | ||
| ssh_args = self._build_final_ssh_command(ssh_args, ssh_options, host, command_args) | ||
|
|
||
| # Execute the command | ||
| return self._execute_ssh_command(ssh_args) |
There was a problem hiding this comment.
Ensure wrapper-enforced port cannot be overridden by user “-p”
If the wrapper supplies a port (direct or via forward), a user “-p …” in args currently wins because it’s appended later. Normalize options so the enforced port is last and unique.
Apply this diff to move port handling into the final assembly and drop user-provided “-p” variants before appending the enforced port:
@@ def _run_ssh_local(self, host, port, ssh_command, default_username, args):
- # Build final SSH command
- ssh_args = self._build_final_ssh_command(ssh_args, ssh_options, host, command_args)
+ # Build final SSH command
+ ssh_args = self._build_final_ssh_command(ssh_args, ssh_options, host, port, command_args)
@@
- def _build_final_ssh_command(self, ssh_args, ssh_options, host, command_args):
+ def _build_final_ssh_command(self, ssh_args, ssh_options, host, port, command_args):
"""Build the final SSH command with all components"""
- # Add SSH options
- ssh_args.extend(ssh_options)
+ # Normalize port (-p) so the wrapper-provided port (if any) wins
+ ssh_args.extend(self._normalize_port_option(ssh_options, port))
@@
# Add hostname before command arguments
if host:
ssh_args.append(host)
@@
self.logger.debug(f"Running SSH command: {ssh_args}")
return ssh_args
+
+ def _normalize_port_option(self, ssh_options, port):
+ """Remove user -p overrides and append enforced port last (if provided)."""
+ normalized: list[str] = []
+ i = 0
+ while i < len(ssh_options):
+ tok = ssh_options[i]
+ # Drop -p VALUE
+ if tok == "-p":
+ i += 2 if i + 1 < len(ssh_options) else 1
+ continue
+ # Drop inline -pVALUE
+ if tok.startswith("-p") and tok != "-p":
+ i += 1
+ continue
+ normalized.append(tok)
+ i += 1
+ if port and port != 22:
+ normalized.extend(["-p", str(port)])
+ return normalizedAlso applies to: 159-173
🤖 Prompt for AI Agents
packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py around lines
79-86 (also applies to 159-173): user-supplied “-p”/“--port” currently wins
because it’s appended after ssh_options; remove any user-provided port flags
from the combined args during final assembly, then append the wrapper-enforced
port option last (ensuring only one port flag exists) — move port handling into
_build_final_ssh_command so it normalizes duplicates (strip "-p" and "--port"
and their values from args/ssh_options), and finally append the enforced port as
the last unique option before executing.
| def _build_ssh_command_args(self, ssh_command, port, default_username, args): | ||
| """Build initial SSH command arguments""" | ||
| # Split the SSH command into individual arguments | ||
| ssh_args = shlex.split(ssh_command) | ||
|
|
||
| # Add port if specified | ||
| if port and port != 22: | ||
| ssh_args.extend(["-p", str(port)]) | ||
|
|
||
| # 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)) | ||
| ) | ||
|
|
||
| # Add default username if no -l flag provided and we have a default | ||
| if not has_user_flag and default_username: | ||
| ssh_args.extend(["-l", default_username]) | ||
|
|
||
| return ssh_args |
There was a problem hiding this comment.
Detect username via “-luser” and “-o User=…” too; don’t miss inline forms
Only checking for a standalone “-l” misses common forms (“-lroot”, “-oUser=alice”). That can incorrectly inject a default username.
Apply this diff to improve detection and avoid adding a default user when user is already specified:
@@ def _build_ssh_command_args(self, ssh_command, port, default_username, args):
- # Add port if specified
- if port and port != 22:
- ssh_args.extend(["-p", str(port)])
-
# 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))
- )
+ has_user_flag = False
+ i = 0
+ while i < len(ssh_options):
+ tok = ssh_options[i]
+ # -l user or inline -luser
+ if tok == "-l" and i + 1 < len(ssh_options):
+ has_user_flag = True
+ break
+ if tok.startswith("-l") and tok != "-l":
+ has_user_flag = True
+ break
+ # -o User=alice or inline -oUser=alice
+ if tok == "-o" and i + 1 < len(ssh_options):
+ val = str(ssh_options[i + 1]).lower()
+ if val.startswith("user=") or val.startswith("user "):
+ has_user_flag = True
+ break
+ i += 1 # skip value
+ elif tok.startswith("-o") and len(tok) > 2:
+ oval = tok[2:].lower()
+ if oval.startswith("user=") or oval.startswith("user "):
+ has_user_flag = True
+ break
+ i += 1
@@
if not has_user_flag and default_username:
ssh_args.extend(["-l", default_username])
return ssh_argsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-ssh/jumpstarter_driver_ssh/client.py around lines
88 to 109, the current detection for an already-specified username only checks
for a standalone "-l" flag and misses inline forms like "-lroot" and -o options
such as "-oUser=alice" or "-o User=alice"; update the detection to treat any
ssh_options entry that starts with "-l" (and has characters after it) as
specifying a user, and also inspect "-o" style options (both "-oUser=..." and
"-o" followed by "User=...") to detect a User= assignment, then use that
combined check to decide whether to skip adding the default username so the
default is only injected when no user is present.
|
Thank you @bennyz :-D |
|
Successfully created backport PR for |
2 participants
Configuration
Example configuration:
Usage
The SSH driver provides a CLI command that accepts all standard SSH arguments:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores