Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete new NanoKVM driver package for Jumpstarter, featuring client implementations for video streaming and HID control, a composite driver for device management with reauth capabilities, comprehensive test coverage, documentation, and configuration files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NanoKVM as NanoKVM Driver
participant Client as NanoKVMVideoClient/<br/>NanoKVMHIDClient
participant APICtx as NanoKVM API Client<br/>(Context Manager)
participant API as NanoKVM Device<br/>API
User->>NanoKVM: Request operation<br/>(snapshot/HID action)
NanoKVM->>NanoKVM: _get_client()<br/>(check cached)
alt Client exists
NanoKVM->>Client: Reuse client
else Client missing
NanoKVM->>APICtx: __aenter__<br/>(setup session)
APICtx->>API: Authenticate<br/>(username/password)
APICtx-->>NanoKVM: Return APIClient
NanoKVM->>Client: Create with APIClient
end
Client->>API: Call method<br/>(snapshot/paste_text)
alt Success (200)
API-->>Client: Return data
else Unauthorized (401)
Client->>NanoKVM: with_reauth decorator
NanoKVM->>NanoKVM: _reset_client()
NanoKVM->>APICtx: __aexit__ then<br/>re-authenticate
NanoKVM->>API: Retry operation
API-->>Client: Return data
end
Client-->>User: Result (Image/confirmation)
sequenceDiagram
participant User
participant Composite as NanoKVM<br/>Composite Driver
participant Video as NanoKVMVideo
participant HID as NanoKVMHID
participant ImageOps as Image Operations
User->>Composite: get_info()
Composite->>Video: _get_client()
Video->>Video: Authenticate & cache
Composite->>Video: Call API method
Video-->>Composite: Device info
User->>Composite: snapshot()
Composite->>Video: snapshot(skip_frames=3)
Video->>Video: stream().read()
Video-->>Composite: Base64 JPEG
Composite->>ImageOps: Decode & return PIL Image
ImageOps-->>User: Image object
User->>Composite: mouse_click(button="left", x=100, y=100)
Composite->>HID: mouse_click()
HID->>HID: _get_client()
HID->>HID: API mouse_click()
HID-->>User: Confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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
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: 3
🤖 Fix all issues with AI agents
In
`@python/packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py`:
- Around line 460-466: The unmount behavior in _mount_impl is inconsistent: the
comment says the API expects an empty string for unmount, but the code uses
mount_file or None which sends None; change the logic so mount_file is set to an
empty string when file is falsy (e.g., mount_file = file if file else "") and
pass mount_file directly to client.mount_image (not mount_file or None); also
ensure the cdrom argument is computed from whether file is provided (e.g., cdrom
if file else False) so cdrom is False when unmounting.
- Around line 133-155: The retry path uses send_stream after its async context
has already exited, which is invalid; update stream_video to ensure send_stream
remains active for retries by moving the async with send_stream: to wrap the
entire streaming/retry logic (or re-enter a new async with send_stream: before
sending retry frames). Specifically, in stream_video, avoid using send_stream
after the except block exits its context — on _is_unauthorized_error(e) call
await self._reset_client() and await self._get_client() while still inside the
async with send_stream: context (or re-open it), then iterate
new_client.mjpeg_stream() to send frames; alternatively factor the per-client
frame-loop into a helper used inside the async with send_stream: so send_stream
is never used outside its context. Ensure references to client.mjpeg_stream(),
new_client.mjpeg_stream(), _is_unauthorized_error, _reset_client, and
_get_client are preserved.
In `@python/packages/jumpstarter-driver-nanokvm/pyproject.toml`:
- Around line 52-56: The dev dependency group in pyproject.toml is missing
pytest-asyncio, but tests rely on pytest's asyncio_mode = "auto"; add
pytest-asyncio (e.g., "pytest-asyncio>=0.20.3") to the [dependency-groups] dev
list so async tests run correctly and match the existing pytest asyncio_mode
configuration.
🧹 Nitpick comments (8)
python/packages/jumpstarter-driver-nanokvm/pyproject.toml (1)
16-16: Git dependency ondevbranch may cause instability.Using a dev branch (
@dev) for thenanokvmdependency can lead to unpredictable behavior as the branch may change unexpectedly. Consider pinning to a specific commit hash or creating a versioned release.- "nanokvm @ git+https://github.com/mangelajo/python-nanokvm.git@dev", + "nanokvm @ git+https://github.com/mangelajo/python-nanokvm.git@<commit-hash>",python/packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (2)
193-196: Invalid escape sequences will raise unhandledValueError.The
unicode_escapecodec raisesValueErrorfor invalid escape sequences (e.g.,\xgg). Consider wrapping with error handling for better CLI UX.♻️ Optional: Add error handling
`@base.command`() `@click.argument`("text") def paste(text): """Paste text via keyboard HID (supports \\n for newline, \\t for tab)""" - # Decode escape sequences like \n, \t, etc. - decoded_text = text.encode().decode("unicode_escape") + try: + decoded_text = text.encode().decode("unicode_escape") + except ValueError as e: + raise click.BadParameter(f"Invalid escape sequence: {e}") self.paste_text(decoded_text) click.echo(f"Pasted: {repr(decoded_text)}")
526-535: Function namelistshadows Python built-in.The command function name
listshadows the built-inlisttype. While this works in Click's context, consider renaming for clarity.♻️ Suggested rename
`@image.command`() - def list(): + def list_images(): """List available image files""" images = self.get_images()python/packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (2)
170-217: Code duplication: client management logic repeated fromNanoKVMVideo.
NanoKVMHIDduplicates the entire client lifecycle management (_client,_client_ctx,_reset_client,_get_client,close) fromNanoKVMVideo. Consider extracting this into a shared mixin class.♻️ Suggested refactor sketch
class NanoKVMClientMixin: """Mixin for NanoKVM API client lifecycle management""" host: str username: str = "admin" password: str = "admin" _client: NanoKVMAPIClient | None = field(init=False, repr=False, default=None) _client_ctx: object = field(init=False, repr=False, default=None) async def _reset_client(self): ... async def _get_client(self) -> NanoKVMAPIClient: ... def close(self): ... # Then: class NanoKVMVideo(NanoKVMClientMixin, Driver): ... class NanoKVMHID(NanoKVMClientMixin, Driver): ...
327-349:NanoKVMSerialis a non-functional placeholder.The class hardcodes
rfc2217://localhost:2217and requires manual SSH port forwarding setup. Combined with the warning at line 396, this feature is incomplete. Consider either removing it or marking it more explicitly.♻️ Suggested documentation improvement
`@dataclass`(kw_only=True) class NanoKVMSerial(PySerial): - """NanoKVM Serial console access via SSH tunnel""" + """NanoKVM Serial console access via SSH tunnel + + WARNING: This is a placeholder implementation. Users must manually + set up SSH port forwarding: ssh -L 2217:localhost:2217 root@<nanokvm> + """python/packages/jumpstarter-driver-nanokvm/README.md (1)
54-58: Consider adding a note about SSH password security.The documentation shows default SSH credentials (
root/root) in the example. While these are defaults for many embedded devices, consider adding a recommendation to change them or use key-based authentication in production environments.python/packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py (2)
67-74: Consider documenting why this fixture is needed.The
mock_aiohttp_sessionfixture is used by all tests but the session mock appears minimal. A brief comment explaining its purpose (e.g., preventing actual HTTP connections during driver initialization) would improve clarity.
208-227: Consider adding tests for missing functionality.The test suite covers core functionality well but is missing tests for:
mouse_move_relandmouse_scroll- Image management:
mount_image,download_image,get_mounted_image,get_cdrom_status,is_image_download_enabled,get_image_download_statusBased on learnings, driver implementations should add comprehensive tests.
Do you want me to help generate additional test cases for the missing functionality?
| async def stream_video(): | ||
| try: | ||
| async with send_stream: | ||
| async for frame in client.mjpeg_stream(): | ||
| buffer = BytesIO() | ||
| frame.save(buffer, format="JPEG") | ||
| data = buffer.getvalue() | ||
| # TODO(mangelajo): this needs to be tested | ||
| await send_stream.send(data) | ||
| except Exception as e: | ||
| if _is_unauthorized_error(e): | ||
| self.logger.warning("Received 401 Unauthorized during stream, re-authenticating...") | ||
| await self._reset_client() | ||
| # Retry with new client | ||
| new_client = await self._get_client() | ||
| async for frame in new_client.mjpeg_stream(): | ||
| buffer = BytesIO() | ||
| frame.save(buffer, format="JPEG") | ||
| data = buffer.getvalue() | ||
| await send_stream.send(data) | ||
| else: | ||
| self.logger.error(f"Error streaming video: {e}") | ||
| raise |
There was a problem hiding this comment.
Re-auth retry in stream may fail due to exited context manager.
When a 401 error occurs, the async with send_stream: block has already exited (exception propagated out of it). The retry code at lines 148-152 attempts to use send_stream after its context manager has exited, which will likely fail or behave unexpectedly.
♻️ Suggested restructure
async def stream_video():
- try:
- async with send_stream:
- async for frame in client.mjpeg_stream():
- buffer = BytesIO()
- frame.save(buffer, format="JPEG")
- data = buffer.getvalue()
- # TODO(mangelajo): this needs to be tested
- await send_stream.send(data)
- except Exception as e:
- if _is_unauthorized_error(e):
- self.logger.warning("Received 401 Unauthorized during stream, re-authenticating...")
- await self._reset_client()
- # Retry with new client
- new_client = await self._get_client()
- async for frame in new_client.mjpeg_stream():
- buffer = BytesIO()
- frame.save(buffer, format="JPEG")
- data = buffer.getvalue()
- await send_stream.send(data)
- else:
- self.logger.error(f"Error streaming video: {e}")
- raise
+ async with send_stream:
+ current_client = client
+ while True:
+ try:
+ async for frame in current_client.mjpeg_stream():
+ buffer = BytesIO()
+ frame.save(buffer, format="JPEG")
+ data = buffer.getvalue()
+ await send_stream.send(data)
+ break # Stream ended normally
+ except Exception as e:
+ if _is_unauthorized_error(e):
+ self.logger.warning("Received 401 Unauthorized during stream, re-authenticating...")
+ await self._reset_client()
+ current_client = await self._get_client()
+ else:
+ self.logger.error(f"Error streaming video: {e}")
+ raise🤖 Prompt for AI Agents
In
`@python/packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py`
around lines 133 - 155, The retry path uses send_stream after its async context
has already exited, which is invalid; update stream_video to ensure send_stream
remains active for retries by moving the async with send_stream: to wrap the
entire streaming/retry logic (or re-enter a new async with send_stream: before
sending retry frames). Specifically, in stream_video, avoid using send_stream
after the except block exits its context — on _is_unauthorized_error(e) call
await self._reset_client() and await self._get_client() while still inside the
async with send_stream: context (or re-open it), then iterate
new_client.mjpeg_stream() to send frames; alternatively factor the per-client
frame-loop into a helper used inside the async with send_stream: so send_stream
is never used outside its context. Ensure references to client.mjpeg_stream(),
new_client.mjpeg_stream(), _is_unauthorized_error, _reset_client, and
_get_client are preserved.
| @with_reauth | ||
| async def _mount_impl(driver): | ||
| client = await driver._get_client() | ||
| # Pass empty string or None for unmount - API expects empty string | ||
| mount_file = file if file else "" | ||
| # When unmounting, we need to pass the file as empty string or None | ||
| await client.mount_image(file=mount_file or None, cdrom=cdrom if mount_file else False) |
There was a problem hiding this comment.
Confusing unmount logic: comment contradicts code.
The comment says "API expects empty string" for unmount, but the code passes mount_file or None which evaluates to None when unmounting (since empty string is falsy). This discrepancy may cause unmount failures.
`@with_reauth`
async def _mount_impl(driver):
client = await driver._get_client()
- # Pass empty string or None for unmount - API expects empty string
- mount_file = file if file else ""
- # When unmounting, we need to pass the file as empty string or None
- await client.mount_image(file=mount_file or None, cdrom=cdrom if mount_file else False)
+ # Pass empty string for unmount, or the file path for mount
+ await client.mount_image(file=file if file else "", cdrom=cdrom if file else False)🤖 Prompt for AI Agents
In
`@python/packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py`
around lines 460 - 466, The unmount behavior in _mount_impl is inconsistent: the
comment says the API expects an empty string for unmount, but the code uses
mount_file or None which sends None; change the logic so mount_file is set to an
empty string when file is falsy (e.g., mount_file = file if file else "") and
pass mount_file directly to client.mount_image (not mount_file or None); also
ensure the cdrom argument is computed from whether file is provided (e.g., cdrom
if file else False) so cdrom is False when unmounting.
| [dependency-groups] | ||
| dev = [ | ||
| "pytest-cov>=6.0.0", | ||
| "pytest>=8.3.3", | ||
| ] |
There was a problem hiding this comment.
Missing pytest-asyncio dependency.
The pytest configuration uses asyncio_mode = "auto" (line 43), but pytest-asyncio is not listed in the dev dependencies. Tests using async functions will fail.
[dependency-groups]
dev = [
"pytest-cov>=6.0.0",
"pytest>=8.3.3",
+ "pytest-asyncio>=0.24.0",
]🤖 Prompt for AI Agents
In `@python/packages/jumpstarter-driver-nanokvm/pyproject.toml` around lines 52 -
56, The dev dependency group in pyproject.toml is missing pytest-asyncio, but
tests rely on pytest's asyncio_mode = "auto"; add pytest-asyncio (e.g.,
"pytest-asyncio>=0.20.3") to the [dependency-groups] dev list so async tests run
correctly and match the existing pytest asyncio_mode configuration.
1 participant
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.