Open
Conversation
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
bennyz
added
the
build-pr-images/jumpstarter
Contributor
📝 WalkthroughWalkthroughAdds a standalone (controller-less) exporter TCP/gRPC listener with optional TLS, client TCP/TLS dialing, a DirectLease type, CLI flags to run/connect directly, session TCP serving helpers, exporter standalone lifecycle, and E2E manifests/tests for direct listener and hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Run as run.py
participant Exporter as Exporter
participant Session as Session
participant gRPC as gRPC Server
participant Client as Client
User->>Run: start with --tls-grpc-listener host:port + cert/key
Run->>Run: _parse_listener_bind(), _tls_server_credentials()
Run->>Exporter: create_exporter(standalone=True)
Exporter->>Session: serve_standalone_tcp(host, port, tls_credentials)
Session->>gRPC: create aio.Server, bind TCP (TLS/insecure), start
gRPC->>Session: server started
User->>Client: shell --tls-grpc-address host:port
Client->>Client: _is_tcp_address() -> True
Client->>Client: build ssl_channel_credentials(tls_config) or insecure channel
Client->>gRPC: dial (aio_secure_channel / insecure)
Client->>gRPC: call driver methods
gRPC->>Client: responses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Container ImagesThe following container images have been built for this PR:
Images expire after 7 days. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
python/packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
38-43: Redundant import ofEventinside function.
Eventis already imported at the module level (line 11). The import inside_standalone_shutdown_waiteris unnecessary.♻️ Proposed fix
async def _standalone_shutdown_waiter(): """Wait forever; used so serve_standalone_tcp can be cancelled by stop().""" - from anyio import Event - await Event().wait()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py` around lines 38 - 43, The function _standalone_shutdown_waiter contains a redundant local import of Event; remove the inner "from anyio import Event" and use the module-level Event import instead so that _standalone_shutdown_waiter simply awaits Event().wait() without re-importing.python/packages/jumpstarter/jumpstarter/client/lease.py (1)
65-68: Method nameserve_unix_asyncis misleading for TCP addresses.This method yields a TCP address (
host:port), not a Unix socket path. The nameserve_unix_asyncis inherited from theLeaseinterface but is semantically misleading here. Consider adding a docstring clarification or renaming if the interface allows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/client/lease.py` around lines 65 - 68, The method serve_unix_async currently yields a TCP address (self.address) while its name implies a Unix socket; update the implementation to clarify intent by either renaming serve_unix_async to a more accurate name (e.g., serve_tcp_async) if the Lease interface permits, or at minimum update the docstring of serve_unix_async to explicitly state it yields a TCP host:port address (not a Unix socket); reference the serve_unix_async method and the Lease interface and change the name and all call sites or adjust the docstring on serve_unix_async accordingly so callers and implementers are not misled.e2e/tests-direct-listener.bats (1)
6-6: Consider using a dynamic port or per-test port to avoid conflicts.Using a fixed port (19090) could cause test failures if:
- Multiple test runs execute in parallel
- The port is already in use by another service
- A previous test's cleanup didn't complete before the next test starts
Consider either using port 0 (let the OS assign a free port and capture it) or using
$BATS_TEST_NUMBERto derive a unique port per test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests-direct-listener.bats` at line 6, Replace the fixed LISTENER_PORT=19090 with a dynamic per-test assignment: modify the test setup to choose a free port (either by binding to port 0 and capturing the assigned port, or by deriving a unique port using $BATS_TEST_NUMBER) and export that value as LISTENER_PORT for the rest of the test; ensure the code that starts the listener reads LISTENER_PORT and that any cleanup/fallback handles port conflicts (retry or fail with a clear message) so parallel runs and leftover processes won't collide.python/packages/jumpstarter/jumpstarter/client/client.py (1)
23-34: IPv6 addresses may be misidentified as invalid TCP addresses.The
_is_tcp_addressfunction usesrsplit(":", 1)which correctly handles IPv6 addresses like[::1]:8080, butparts[0]would be[::1](with brackets). However, bare IPv6 without brackets (e.g.,::1:8080) would fail since::1would be interpreted as the "port".This may be acceptable if IPv6 addresses are expected to always use bracket notation (
[host]:port), but consider documenting this requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/client/client.py` around lines 23 - 34, The _is_tcp_address function misparses IPv6 forms like ::1:8080; update _is_tcp_address to explicitly handle bracketed IPv6 by checking for host wrapped in [ ] (e.g., parts[0] starting with '[' and ending with ']') and treating the inner text as the host, while leaving the existing numeric port validation (int(parts[1], 10) and 1<=port<=65535) intact; also update the function docstring to state that IPv6 addresses must be bracketed as [host]:port and that bare IPv6 with embedded colons is not supported.python/packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
308-341: Consider documenting the security implications ofunsafe=Truefor direct connections.The
DirectLeaseis created withallow=[]andunsafe=True, which bypasses driver allowlisting. While this is reasonable for direct connections (the user explicitly chose to connect to a specific exporter), consider adding a comment explaining this security model choice.📝 Suggested documentation
async with anyio.from_thread.BlockingPortal() as portal: lease = DirectLease( address=tls_grpc_address, portal=portal, - allow=[], - unsafe=True, + allow=[], # Direct connection trusts the exporter + unsafe=True, # No driver allowlist filtering for direct mode tls_config=TLSConfigV1Alpha1(), grpc_options={}, insecure=tls_grpc_insecure, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py` around lines 308 - 341, The DirectLease in _shell_direct_async is created with allow=[] and unsafe=True which bypasses driver allowlisting; add a concise comment next to the DirectLease construction (referencing DirectLease, unsafe=True, allow=[]) that documents the security implications (that driver restrictions are disabled for direct connections because the user explicitly targets an exporter), and note any recommended mitigations (e.g., user-facing warning/confirmation or restricted use only for trusted endpoints) so future readers understand the risk and rationale.python/packages/jumpstarter/jumpstarter/client/client_test.py (1)
27-32: Consider whether port 0 should be valid for OS-assigned ports.Port 0 is conventionally used to request an OS-assigned ephemeral port. The test asserts
_is_tcp_address("host:0")returnsFalse, which may be intentional to prevent user confusion, but could also be a limitation if the code is later used in contexts where port 0 is meaningful.Verify this is the intended behavior for your use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/client/client_test.py` around lines 27 - 32, Decide whether port 0 should be considered a valid TCP address and make the change consistently: if port 0 should be allowed, update the _is_tcp_address implementation (the parsing/port validation logic) to accept 0 as valid (range 0..65535) and change test_invalid_port_not_tcp_address to assert _is_tcp_address("host:0") is True; if port 0 should remain invalid, add a clarifying comment in test_invalid_port_not_tcp_address (and/or in the _is_tcp_address docstring) explaining why port 0 is rejected to prevent future confusion, leaving the existing assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/run.py`:
- Around line 16-27: The _parse_listener_bind function incorrectly uses
rsplit(":", 1) which breaks on IPv6 addresses; update it to explicitly handle
bracketed IPv6 (if value starts with '[' find the matching ']', take host=inside
brackets and parse the port after the ']' ), otherwise if the string contains
more than one ':' but is not bracketed raise click.BadParameter instructing
callers to use bracketed IPv6 addresses, and for the normal hostname/IPv4 case
continue using rsplit(":", 1) to extract host and port; preserve the existing
port range validation and the param_hint "'--tls-grpc-listener'".
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 858-878: serve_standalone_tcp is missing a finally path to run the
after-lease cleanup when the standalone session is cancelled (e.g., via stop()),
so _cleanup_after_lease and any hook_executor.run_after_lease_hook never
execute; wrap the async with session.serve_tcp_and_unix_async(...) block in a
try/finally (or add a finally after the with) that clears self._lease_context
and calls self._cleanup_after_lease(lease_scope) and, if hook_executor exists,
invokes hook_executor.run_after_lease_hook(...) (mirroring
handle_lease/_handle_end_session behavior) so the afterLease logic always runs
on shutdown.
---
Nitpick comments:
In `@e2e/tests-direct-listener.bats`:
- Line 6: Replace the fixed LISTENER_PORT=19090 with a dynamic per-test
assignment: modify the test setup to choose a free port (either by binding to
port 0 and capturing the assigned port, or by deriving a unique port using
$BATS_TEST_NUMBER) and export that value as LISTENER_PORT for the rest of the
test; ensure the code that starts the listener reads LISTENER_PORT and that any
cleanup/fallback handles port conflicts (retry or fail with a clear message) so
parallel runs and leftover processes won't collide.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`:
- Around line 308-341: The DirectLease in _shell_direct_async is created with
allow=[] and unsafe=True which bypasses driver allowlisting; add a concise
comment next to the DirectLease construction (referencing DirectLease,
unsafe=True, allow=[]) that documents the security implications (that driver
restrictions are disabled for direct connections because the user explicitly
targets an exporter), and note any recommended mitigations (e.g., user-facing
warning/confirmation or restricted use only for trusted endpoints) so future
readers understand the risk and rationale.
In `@python/packages/jumpstarter/jumpstarter/client/client_test.py`:
- Around line 27-32: Decide whether port 0 should be considered a valid TCP
address and make the change consistently: if port 0 should be allowed, update
the _is_tcp_address implementation (the parsing/port validation logic) to accept
0 as valid (range 0..65535) and change test_invalid_port_not_tcp_address to
assert _is_tcp_address("host:0") is True; if port 0 should remain invalid, add a
clarifying comment in test_invalid_port_not_tcp_address (and/or in the
_is_tcp_address docstring) explaining why port 0 is rejected to prevent future
confusion, leaving the existing assertions unchanged.
In `@python/packages/jumpstarter/jumpstarter/client/client.py`:
- Around line 23-34: The _is_tcp_address function misparses IPv6 forms like
::1:8080; update _is_tcp_address to explicitly handle bracketed IPv6 by checking
for host wrapped in [ ] (e.g., parts[0] starting with '[' and ending with ']')
and treating the inner text as the host, while leaving the existing numeric port
validation (int(parts[1], 10) and 1<=port<=65535) intact; also update the
function docstring to state that IPv6 addresses must be bracketed as [host]:port
and that bare IPv6 with embedded colons is not supported.
In `@python/packages/jumpstarter/jumpstarter/client/lease.py`:
- Around line 65-68: The method serve_unix_async currently yields a TCP address
(self.address) while its name implies a Unix socket; update the implementation
to clarify intent by either renaming serve_unix_async to a more accurate name
(e.g., serve_tcp_async) if the Lease interface permits, or at minimum update the
docstring of serve_unix_async to explicitly state it yields a TCP host:port
address (not a Unix socket); reference the serve_unix_async method and the Lease
interface and change the name and all call sites or adjust the docstring on
serve_unix_async accordingly so callers and implementers are not misled.
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 38-43: The function _standalone_shutdown_waiter contains a
redundant local import of Event; remove the inner "from anyio import Event" and
use the module-level Event import instead so that _standalone_shutdown_waiter
simply awaits Event().wait() without re-importing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8b1862c-9f05-4be1-bde9-dc0842ce52fc
📒 Files selected for processing (15)
e2e/exporters/exporter-direct-listener.yamle2e/tests-direct-listener.batspython/packages/jumpstarter-cli/jumpstarter_cli/run.pypython/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter/jumpstarter/client/__init__.pypython/packages/jumpstarter/jumpstarter/client/client.pypython/packages/jumpstarter/jumpstarter/client/client_test.pypython/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/common/utils.pypython/packages/jumpstarter/jumpstarter/config/env.pypython/packages/jumpstarter/jumpstarter/config/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/session.pypython/packages/jumpstarter/jumpstarter/exporter/session_test.pypython/packages/jumpstarter/jumpstarter/utils/env.py
Comment on lines
+16
to
+27
| def _parse_listener_bind(value: str) -> tuple[str, int]: | ||
| """Parse '[host:]port' into (host, port). Default host is 0.0.0.0.""" | ||
| if ":" in value: | ||
| host, port_str = value.rsplit(":", 1) | ||
| host = host.strip() or "0.0.0.0" | ||
| port = int(port_str, 10) | ||
| else: | ||
| host = "0.0.0.0" | ||
| port = int(value, 10) | ||
| if not (1 <= port <= 65535): | ||
| raise click.BadParameter(f"port must be between 1 and 65535, got {port}", param_hint="'--tls-grpc-listener'") | ||
| return host, port |
Contributor
There was a problem hiding this comment.
IPv6 address parsing may fail with rsplit(":", 1).
IPv6 addresses contain colons (e.g., [::1]:1234 or ::1). Using rsplit(":", 1) would incorrectly split ::1:1234 as host ::1 and port 1234, but [::1]:1234 would split as host [::1] and port 1234 which is closer but not ideal.
Consider handling bracketed IPv6 addresses explicitly or documenting that IPv6 addresses must be bracketed.
🛠️ Proposed fix to handle IPv6 addresses
def _parse_listener_bind(value: str) -> tuple[str, int]:
- """Parse '[host:]port' into (host, port). Default host is 0.0.0.0."""
- if ":" in value:
- host, port_str = value.rsplit(":", 1)
- host = host.strip() or "0.0.0.0"
- port = int(port_str, 10)
+ """Parse '[host:]port' into (host, port). Default host is 0.0.0.0.
+
+ For IPv6, use bracketed notation: [::1]:1234
+ """
+ if value.startswith("["):
+ # IPv6 bracketed notation: [host]:port
+ bracket_end = value.find("]")
+ if bracket_end == -1:
+ raise click.BadParameter("Invalid IPv6 address: missing closing bracket", param_hint="'--tls-grpc-listener'")
+ host = value[1:bracket_end]
+ remainder = value[bracket_end + 1:]
+ if remainder.startswith(":"):
+ port = int(remainder[1:], 10)
+ elif remainder == "":
+ raise click.BadParameter("Port is required", param_hint="'--tls-grpc-listener'")
+ else:
+ raise click.BadParameter("Invalid format after IPv6 address", param_hint="'--tls-grpc-listener'")
+ elif ":" in value:
+ host, port_str = value.rsplit(":", 1)
+ host = host.strip() or "0.0.0.0"
+ port = int(port_str, 10)
else:
host = "0.0.0.0"
port = int(value, 10)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/run.py` around lines 16 - 27,
The _parse_listener_bind function incorrectly uses rsplit(":", 1) which breaks
on IPv6 addresses; update it to explicitly handle bracketed IPv6 (if value
starts with '[' find the matching ']', take host=inside brackets and parse the
port after the ']' ), otherwise if the string contains more than one ':' but is
not bracketed raise click.BadParameter instructing callers to use bracketed IPv6
addresses, and for the normal hostname/IPv4 case continue using rsplit(":", 1)
to extract host and port; preserve the existing port range validation and the
param_hint "'--tls-grpc-listener'".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
842-845: Reset_standaloneon method exit to avoid stale runtime mode.
self._standaloneis set at Line 842 but never reset. If the sameExporterinstance is reused, controller-status/report paths stay disabled unexpectedly.Suggested fix
async def serve_standalone_tcp( self, host: str, port: int, *, tls_credentials: grpc.ServerCredentials | None = None, ) -> None: @@ - self._standalone = True - lease_scope = LeaseContext(lease_name="standalone", before_lease_hook=Event()) - self._lease_context = lease_scope + self._standalone = True + lease_scope = LeaseContext(lease_name="standalone", before_lease_hook=Event()) + self._lease_context = lease_scope - with TemporarySocket() as hook_path: - hook_path_str = str(hook_path) - with Session( - uuid=self.uuid, - labels=self.labels, - root_device=self.device_factory(), - ) as session: - session.lease_context = lease_scope - lease_scope.session = session - lease_scope.socket_path = hook_path_str - lease_scope.hook_socket_path = hook_path_str + try: + with TemporarySocket() as hook_path: + hook_path_str = str(hook_path) + with Session( + uuid=self.uuid, + labels=self.labels, + root_device=self.device_factory(), + ) as session: + session.lease_context = lease_scope + lease_scope.session = session + lease_scope.socket_path = hook_path_str + lease_scope.hook_socket_path = hook_path_str - async with session.serve_tcp_and_unix_async( - host, port, hook_path_str, tls_credentials=tls_credentials - ): - try: - async with create_task_group() as tg: - self._tg = tg - tg.start_soon(self._handle_end_session, lease_scope) + async with session.serve_tcp_and_unix_async( + host, port, hook_path_str, tls_credentials=tls_credentials + ): + try: + async with create_task_group() as tg: + self._tg = tg + tg.start_soon(self._handle_end_session, lease_scope) - if self.hook_executor: - await self.hook_executor.run_before_lease_hook( - lease_scope, - self._report_status, - self.stop, - self._request_lease_release, - ) - else: - await self._report_status(ExporterStatus.LEASE_READY, "Ready for commands") - lease_scope.before_lease_hook.set() + if self.hook_executor: + await self.hook_executor.run_before_lease_hook( + lease_scope, + self._report_status, + self.stop, + self._request_lease_release, + ) + else: + await self._report_status(ExporterStatus.LEASE_READY, "Ready for commands") + lease_scope.before_lease_hook.set() - await _standalone_shutdown_waiter() - finally: - await self._cleanup_after_lease(lease_scope) - - self._lease_context = None - self._tg = None + await _standalone_shutdown_waiter() + finally: + await self._cleanup_after_lease(lease_scope) + finally: + self._lease_context = None + self._tg = None + self._standalone = FalseAlso applies to: 881-882
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py` around lines 842 - 845, The Exporter sets self._standalone = True and assigns self._lease_context (via LeaseContext/lease_scope) but never resets them, leaving the instance in standalone mode for reused Exporter objects; update the method(s) where you set self._standalone = True and self._lease_context = lease_scope (and the other similar block that sets those at the later location around lines with self._standalone / lease_scope) to ensure you reset state on exit: wrap the work in a try/finally (or use a context manager) and in the finally set self._standalone = False and self._lease_context = None (and also ensure any LeaseContext is properly closed/cleaned) so controller-status/report paths are re-enabled for subsequent uses of the same Exporter instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/exporters/exporter-direct-hooks-before.yaml`:
- Around line 11-14: The hook script masks failures because the final `echo
"BEFORE_HOOK_DIRECT: complete"` can return success even if `j power on` failed;
update the script around the `j power on` invocation (the lines containing `j
power on` and the surrounding `echo` commands) to propagate errors — e.g.,
enable errexit (`set -e`) at the top of the script or explicitly test the exit
status of `j power on` and `exit` with that non-zero code so failures in `j
power on` cause the hook to fail immediately.
In `@e2e/tests-direct-listener.bats`:
- Around line 157-163: The test uses a fixed sleep after calling stop_exporter
which is flaky; replace the sleep 1 with a bounded polling loop that checks
"${BATS_TEST_TMPDIR}/exporter.log" for the expected "AFTER_HOOK_DIRECT:
executed" string until a short timeout (e.g. 5–10s) and fails if not found, then
run the final assertion (keep the run cat ... and assert_output --partial).
Implement the poll as a loop that sleeps briefly between checks, uses
stop_exporter as-is, and exits early when the log contains the marker so the
test is deterministic and safe to run in isolation.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 842-845: The Exporter sets self._standalone = True and assigns
self._lease_context (via LeaseContext/lease_scope) but never resets them,
leaving the instance in standalone mode for reused Exporter objects; update the
method(s) where you set self._standalone = True and self._lease_context =
lease_scope (and the other similar block that sets those at the later location
around lines with self._standalone / lease_scope) to ensure you reset state on
exit: wrap the work in a try/finally (or use a context manager) and in the
finally set self._standalone = False and self._lease_context = None (and also
ensure any LeaseContext is properly closed/cleaned) so controller-status/report
paths are re-enabled for subsequent uses of the same Exporter instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9c826cd-3a9c-4db8-aef4-c1c296136ae6
📒 Files selected for processing (4)
e2e/exporters/exporter-direct-hooks-before.yamle2e/exporters/exporter-direct-hooks-both.yamle2e/tests-direct-listener.batspython/packages/jumpstarter/jumpstarter/exporter/exporter.py
bennyz
force-pushed
the
exporter-grpc-listener
branch
from
65117b3 to
9b379b7
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
python/packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
38-42: Remove redundant import inside function.
Eventis already imported at line 11 fromanyio. The import inside_standalone_shutdown_waiteris unnecessary.♻️ Proposed fix
async def _standalone_shutdown_waiter(): """Wait forever; used so serve_standalone_tcp can be cancelled by stop().""" - from anyio import Event - await Event().wait()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py` around lines 38 - 42, Remove the redundant local import of Event inside _standalone_shutdown_waiter: delete the line "from anyio import Event" within the async def _standalone_shutdown_waiter(), and use the Event symbol already imported at module top (line where Event is imported) so the function simply awaits Event().wait() without re-importing.e2e/tests-direct-listener.bats (1)
16-56: Consider extracting common logic to reduce duplication.
start_exporterandstart_exporter_with_logsshare identical logic except for stderr redirection. Similarly for the_bgvariants. This is acceptable for test clarity, but could be consolidated with an optional parameter if more variants are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests-direct-listener.bats` around lines 16 - 56, start_exporter and start_exporter_with_logs duplicate the same startup and readiness logic; extract that common behavior into a helper function (e.g., start_exporter_common) that accepts parameters for config and an optional stderr redirection flag or redirection path, then have start_exporter and start_exporter_with_logs (and the _bg variants) call this helper; preserve setting LISTENER_PID, writing "${BATS_TEST_TMPDIR}/exporter.pid", the jmp run invocation differences (with or without "2>..."), and the readiness loop that uses jmp shell --tls-grpc "127.0.0.1:${LISTENER_PORT}" --tls-grpc-insecure -- j --help so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests-direct-listener.bats`:
- Around line 16-56: start_exporter and start_exporter_with_logs duplicate the
same startup and readiness logic; extract that common behavior into a helper
function (e.g., start_exporter_common) that accepts parameters for config and an
optional stderr redirection flag or redirection path, then have start_exporter
and start_exporter_with_logs (and the _bg variants) call this helper; preserve
setting LISTENER_PID, writing "${BATS_TEST_TMPDIR}/exporter.pid", the jmp run
invocation differences (with or without "2>..."), and the readiness loop that
uses jmp shell --tls-grpc "127.0.0.1:${LISTENER_PORT}" --tls-grpc-insecure -- j
--help so behavior remains identical.
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 38-42: Remove the redundant local import of Event inside
_standalone_shutdown_waiter: delete the line "from anyio import Event" within
the async def _standalone_shutdown_waiter(), and use the Event symbol already
imported at module top (line where Event is imported) so the function simply
awaits Event().wait() without re-importing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef821b4d-a914-4db2-830f-cdd34e683a10
📒 Files selected for processing (4)
e2e/exporters/exporter-direct-hooks-before.yamle2e/exporters/exporter-direct-hooks-both.yamle2e/tests-direct-listener.batspython/packages/jumpstarter/jumpstarter/exporter/exporter.py
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/exporters/exporter-direct-hooks-both.yaml
- e2e/exporters/exporter-direct-hooks-before.yaml
Allow exporters to serve directly over TCP without a controller, and clients to connect directly without going through the a controller Exporter: jmp run --tls-grpc-listener [HOST:]PORT [--tls-grpc-insecure] Client: jmp shell --tls-grpc HOST:PORT [--tls-grpc-insecure] Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
Also add hook tests for the standalone mode Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
bennyz
force-pushed
the
exporter-grpc-listener
branch
from
9b379b7 to
5c82ee9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
python/packages/jumpstarter-cli/jumpstarter_cli/run.py (1)
16-27:⚠️ Potential issue | 🟡 MinorNormalize listener-bind parse failures into
click.BadParameter.
rsplit(":", 1)still doesn't support bracketed IPv6, andint(port_str, 10)will raise a rawValueErrorfor inputs likefoo:bar. Since_parse_listener_bind()runs after the fork, those CLI mistakes currently surface as child-process failures instead of a clean usage error. Please handle bracketed IPv6 explicitly and wrap bad ports inclick.BadParameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli/jumpstarter_cli/run.py` around lines 16 - 27, _Parse_listener_bind currently misparses bracketed IPv6 and lets int() ValueError leak; update _parse_listener_bind to detect bracketed IPv6 (if value starts with '[' find the matching ']' and treat what's inside as host and what follows after an optional ':' as port), otherwise use the existing host:port split; wrap the port conversion in a try/except and raise click.BadParameter with a clear message and the existing param_hint "'--tls-grpc-listener'" for any missing/invalid port or non-numeric port values, and keep the existing range check (1–65535) also raising click.BadParameter on failure.
🧹 Nitpick comments (3)
python/packages/jumpstarter/jumpstarter/exporter/exporter.py (1)
162-166: Consider resetting_standaloneflag afterserve_standalone_tcpexits.The
_standaloneflag is set toTrueinserve_standalone_tcp(line 840) but never reset toFalse. WhileExporterinstances are typically not reused, this could cause unexpected behavior if someone callsserve()on the same instance afterserve_standalone_tcp()completes.♻️ Suggested fix
self._lease_context = None self._tg = None + self._standalone = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py` around lines 162 - 166, The _standalone flag is set to True in serve_standalone_tcp but never reset, which can cause subsequent calls to serve() or methods like _report_status and __aexit__ to behave as if running standalone; update serve_standalone_tcp to ensure _standalone is set back to False when the method exits (use a try/finally or context manager) so the Exporter instance returns to its normal state, referencing the _standalone attribute and the serve, serve_standalone_tcp, _report_status, and __aexit__ methods to locate where to add the reset.python/packages/jumpstarter/jumpstarter/exporter/session.py (1)
166-205: Bound TCP port is not yielded when usingport=0.Unlike
serve_tcp_asyncwhich yields thebound_port(useful whenport=0lets the OS choose),serve_tcp_and_unix_asyncdiscards the return value fromadd_secure_port/add_insecure_portand yieldsNone.If a caller uses
port=0to get an ephemeral port, they won't be able to discover which port was actually bound.♻️ Suggested fix to capture and yield the bound port
async def serve_tcp_and_unix_async( self, host: str, port: int, unix_path: str, *, tls_credentials: grpc.ServerCredentials | None = None, ): """Serve session on TCP (and optionally TLS) plus a Unix socket (e.g. for hooks). One server listens on host:port and unix://unix_path. Use for standalone mode where clients use TCP and hook j commands use Unix. Yields: - None - server runs until context exit + int - the bound TCP port (useful when port=0) """ server = grpc.aio.server() address = f"{host}:{port}" server.add_insecure_port(f"unix://{unix_path}") logger.debug("Session server listening on unix://%s (hooks)", unix_path) if tls_credentials is not None: - server.add_secure_port(address, tls_credentials) + bound_port = server.add_secure_port(address, tls_credentials) logger.debug("Session server listening on %s (TLS)", address) else: - server.add_insecure_port(address) + bound_port = server.add_insecure_port(address) logger.debug("Session server listening on %s (insecure)", address) jumpstarter_pb2_grpc.add_ExporterServiceServicer_to_server(self, server) router_pb2_grpc.add_RouterServiceServicer_to_server(self, server) await server.start() - logger.info("Session server started on %s and unix://%s", address, unix_path) + logger.info("Session server started on %s:%d and unix://%s", host, bound_port, unix_path) try: - yield + yield bound_port finally: logger.info("Stopping session server...") await server.stop(grace=1.0) await sleep(0.1) logger.info("Session server stopped")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/exporter/session.py` around lines 166 - 205, The serve_tcp_and_unix_async coroutine currently discards the return value from server.add_secure_port/add_insecure_port so callers cannot discover the actual bound port when port=0; capture the integer returned by the chosen add_*_port call (e.g. bound_port = server.add_insecure_port(...) or bound_port = server.add_secure_port(...)), use that value when logging and yield it from serve_tcp_and_unix_async (matching serve_tcp_async behavior) so callers receive the ephemeral port; ensure the yielded value is returned before the try/finally block suspends and that shutdown still awaits server.stop as before.python/packages/jumpstarter/jumpstarter/common/utils.py (1)
96-122: Missing docstring for theinsecureparameter.The function docstring at lines 98-111 documents all parameters except the newly added
insecureparameter.📝 Suggested docstring addition
Args: host: The jumpstarter host path context: The context of the shell (e.g. "local" or exporter name) allow: List of allowed drivers unsafe: Whether to allow drivers outside of the allow list use_profiles: Whether to load shell profile files command: Optional command to run instead of launching an interactive shell lease: Optional Lease object to set up lease ending callback + insecure: Whether to use insecure gRPC communication (sets JMP_GRPC_INSECURE env var) Returns: The exit code of the shell or command process🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter/jumpstarter/common/utils.py` around lines 96 - 122, Add a docstring entry for the newly added insecure parameter in the function's Args section: document insecure as a boolean that enables insecure gRPC (it causes JMP_GRPC_INSECURE to be set in the environment), and keep it consistent with the style of the other Args entries; update the function's docstring near the existing parameters (host, context, allow, unsafe, use_profiles, command, lease) so the insecure parameter is described.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`:
- Around line 308-341: _shell_direct_async currently builds a synthetic config
that discards the resolved client configuration from shell(), causing loss of
--client/--client-config settings and TLS/driver trust policy; modify
_shell_direct_async (and the similar block at lines 393-401) to accept and use
the resolved client configuration object passed in from shell() instead of
constructing SimpleNamespace(...) with
allow=[]/unsafe=True/TLSConfigV1Alpha1()/use_profiles=False, and only construct
that fallback config when no resolved config is provided; ensure the DirectLease
instantiation still receives tls_grpc_insecure and grpc options from the real
config and pass the real config into _run_shell_with_lease_async so
import_class(...) in client_from_channel() sees the correct
allow/unsafe/TLS/profile settings.
In `@python/packages/jumpstarter/jumpstarter/client/client.py`:
- Around line 23-34: _is_tcp_address currently only validates the trailing port
and therefore accepts malformed addresses like ":1234" and "::1"; update
_is_tcp_address to require a non-empty host for the plain HOST:PORT form and
only allow IPv6 addresses when they are bracketed (e.g. "[::1]:1234").
Concretely: if path starts with '[' parse until matching ']' and ensure a
trailing ":PORT" exists and port is numeric and in 1..65535; otherwise rsplit on
the last ':' and ensure the left-hand host is non-empty and does not contain
additional ':' characters (reject IPv6 unbracketed), then validate the port as
an integer in range. Apply this logic inside the _is_tcp_address function.
In `@python/packages/jumpstarter/jumpstarter/client/lease.py`:
- Around line 44-73: DirectLease is exposed but only implements serve_unix_async
and monitor_async, so callers expecting the full Lease surface (connect,
connect_async, serve_unix, monitor, and normal sync/async context-manager
behavior) will break; either stop exporting DirectLease or implement the missing
helpers: add connect() and connect_async() that return or await the same address
yielded by serve_unix_async, add synchronous wrappers serve_unix() and monitor()
that call the async context managers (or provide equivalent sync behavior), and
ensure enter/exit methods from ContextManagerMixin/AsyncContextManagerMixin are
present and delegate to serve_unix_async/monitor_async as appropriate so
DirectLease presents the same API as Lease.
---
Duplicate comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/run.py`:
- Around line 16-27: _Parse_listener_bind currently misparses bracketed IPv6 and
lets int() ValueError leak; update _parse_listener_bind to detect bracketed IPv6
(if value starts with '[' find the matching ']' and treat what's inside as host
and what follows after an optional ':' as port), otherwise use the existing
host:port split; wrap the port conversion in a try/except and raise
click.BadParameter with a clear message and the existing param_hint
"'--tls-grpc-listener'" for any missing/invalid port or non-numeric port values,
and keep the existing range check (1–65535) also raising click.BadParameter on
failure.
---
Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/common/utils.py`:
- Around line 96-122: Add a docstring entry for the newly added insecure
parameter in the function's Args section: document insecure as a boolean that
enables insecure gRPC (it causes JMP_GRPC_INSECURE to be set in the
environment), and keep it consistent with the style of the other Args entries;
update the function's docstring near the existing parameters (host, context,
allow, unsafe, use_profiles, command, lease) so the insecure parameter is
described.
In `@python/packages/jumpstarter/jumpstarter/exporter/exporter.py`:
- Around line 162-166: The _standalone flag is set to True in
serve_standalone_tcp but never reset, which can cause subsequent calls to
serve() or methods like _report_status and __aexit__ to behave as if running
standalone; update serve_standalone_tcp to ensure _standalone is set back to
False when the method exits (use a try/finally or context manager) so the
Exporter instance returns to its normal state, referencing the _standalone
attribute and the serve, serve_standalone_tcp, _report_status, and __aexit__
methods to locate where to add the reset.
In `@python/packages/jumpstarter/jumpstarter/exporter/session.py`:
- Around line 166-205: The serve_tcp_and_unix_async coroutine currently discards
the return value from server.add_secure_port/add_insecure_port so callers cannot
discover the actual bound port when port=0; capture the integer returned by the
chosen add_*_port call (e.g. bound_port = server.add_insecure_port(...) or
bound_port = server.add_secure_port(...)), use that value when logging and yield
it from serve_tcp_and_unix_async (matching serve_tcp_async behavior) so callers
receive the ephemeral port; ensure the yielded value is returned before the
try/finally block suspends and that shutdown still awaits server.stop as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11096d3c-e65c-470f-bff9-9d883a18bb72
📒 Files selected for processing (17)
e2e/exporters/exporter-direct-hooks-before.yamle2e/exporters/exporter-direct-hooks-both.yamle2e/exporters/exporter-direct-listener.yamle2e/tests-direct-listener.batspython/packages/jumpstarter-cli/jumpstarter_cli/run.pypython/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter/jumpstarter/client/__init__.pypython/packages/jumpstarter/jumpstarter/client/client.pypython/packages/jumpstarter/jumpstarter/client/client_test.pypython/packages/jumpstarter/jumpstarter/client/lease.pypython/packages/jumpstarter/jumpstarter/common/utils.pypython/packages/jumpstarter/jumpstarter/config/env.pypython/packages/jumpstarter/jumpstarter/config/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/exporter.pypython/packages/jumpstarter/jumpstarter/exporter/session.pypython/packages/jumpstarter/jumpstarter/exporter/session_test.pypython/packages/jumpstarter/jumpstarter/utils/env.py
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/exporters/exporter-direct-hooks-both.yaml
- e2e/exporters/exporter-direct-hooks-before.yaml
- e2e/tests-direct-listener.bats
- python/packages/jumpstarter/jumpstarter/utils/env.py
Comment on lines
+23
to
+34
| def _is_tcp_address(path: str) -> bool: | ||
| """Return True if path looks like host:port (TCP address).""" | ||
| if ":" not in path: | ||
| return False | ||
| parts = path.rsplit(":", 1) | ||
| if len(parts) != 2: | ||
| return False | ||
| try: | ||
| port = int(parts[1], 10) | ||
| return 1 <= port <= 65535 | ||
| except ValueError: | ||
| return False |
Contributor
There was a problem hiding this comment.
Tighten _is_tcp_address() so malformed direct targets fail early.
Because this only validates the last :PORT, it currently accepts inputs like :1234 and ::1 as TCP endpoints. In the new jmp shell --tls-grpc flow that pushes obviously bad input to channel creation / first RPC instead of rejecting it up front. Require a non-empty host for the plain HOST:PORT form and only accept IPv6 in bracketed form; the new :1234 assertion in python/packages/jumpstarter/jumpstarter/client/client_test.py would need to change with that fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@python/packages/jumpstarter/jumpstarter/client/client.py` around lines 23 -
34, _is_tcp_address currently only validates the trailing port and therefore
accepts malformed addresses like ":1234" and "::1"; update _is_tcp_address to
require a non-empty host for the plain HOST:PORT form and only allow IPv6
addresses when they are bracketed (e.g. "[::1]:1234"). Concretely: if path
starts with '[' parse until matching ']' and ensure a trailing ":PORT" exists
and port is numeric and in 1..65535; otherwise rsplit on the last ':' and ensure
the left-hand host is non-empty and does not contain additional ':' characters
(reject IPv6 unbracketed), then validate the port as an integer in range. Apply
this logic inside the _is_tcp_address function.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allow exporters to serve directly over TCP without a controller, and clients to connect directly without going through a controller
Summary by CodeRabbit