driver: nanokvm network driver #771
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-nanokvm package implementing a NanoKVM composite driver (video, HID, optional serial), client classes with Click CLIs, docs/README, example config, tests, packaging metadata, and .gitignore updates; also registers the driver in the docs index. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client (video/hid)
participant Driver as NanoKVM Driver (Video / HID)
participant API as NanoKVMAPIClient (HTTP)
participant WS as WebSocket
participant Device as NanoKVM Device
CLI->>Driver: request snapshot / HID action
alt snapshot flow
Driver->>API: lazy init + auth -> GET snapshot
API->>Device: HTTP request
Device-->>API: JPEG bytes
API-->>Driver: JPEG bytes
Driver-->>CLI: decoded Image / base64
else HID flow
Driver->>WS: ensure token-auth WebSocket open
WS->>Device: send HID message (paste/key/mouse)
Device-->>WS: ACK
WS-->>Driver: response
Driver-->>CLI: return
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
mangelajo
force-pushed
the
nanokvm-driver
branch
from
a80c7b0 to
91dc4d8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
packages/jumpstarter-driver-nanokvm/.gitignore (1)
1-3: Standard patterns are in place; consider adding common Python project artifacts for completeness.The included patterns (
__pycache__/,.coverage,coverage.xml) are correct and appropriate for a Python package with test coverage tracking. If this.gitignoreis intended as the only ignore file for this package (and not supplemented by a root.gitignore), consider adding common Python build and development artifacts for a more complete coverage.__pycache__/ + *.egg-info/ + .egg-info/ .coverage coverage.xml + dist/ + build/ + .pytest_cache/ + .mypy_cache/ + .ruff_cache/ + *.pyc + .DS_Storepackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py (1)
165-202: Consider extracting WebSocket mock setup to reduce duplication.Both
test_nanokvm_mouse_move_absandtest_nanokvm_mouse_clickcreate identical WebSocket mock setups. This could be extracted into a reusable fixture.@pytest.fixture def mock_ws_session(): """Create a mock aiohttp ClientSession with WebSocket support""" with patch("jumpstarter_driver_nanokvm.driver.ClientSession") as mock_session_class: mock_ws = AsyncMock() mock_ws.send_json = AsyncMock() mock_session = AsyncMock() mock_session.ws_connect = AsyncMock(return_value=mock_ws) mock_session.close = AsyncMock() mock_session_class.return_value = mock_session yield mock_session, mock_wspackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (1)
185-199: Note:unicode_escapehandling has edge cases.The
encode().decode("unicode_escape")pattern works for basic escape sequences but may produce unexpected results with backslash-prefixed characters that aren't valid escapes (e.g.,\pbecomes a literalpwith dropped backslash). This is acceptable for CLI usage but worth documenting.Consider using a more explicit approach if edge cases become problematic:
import codecs decoded_text = codecs.decode(text, "unicode_escape")Or handle specific escapes manually for more predictable behavior.
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (1)
214-219: Clarify the key length validation logic.The check
len(key) > 2appears intended to warn about multi-character input, but the threshold of 2 is unclear. A newline\nis 1 character,\\nin source is 2 characters. Consider documenting or adjusting:- if len(key) > 2: # Allow for escape sequences like \n, \t - self.logger.warning(f"press_key should be used with single characters, got: {key}") + if len(key) > 1: + self.logger.warning(f"press_key is intended for single characters, got {len(key)} chars: {repr(key)}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/source/reference/package-apis/drivers/nanokvm.md(1 hunks)packages/jumpstarter-driver-nanokvm/.gitignore(1 hunks)packages/jumpstarter-driver-nanokvm/README.md(1 hunks)packages/jumpstarter-driver-nanokvm/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py(1 hunks)packages/jumpstarter-driver-nanokvm/pyproject.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
packages/jumpstarter-driver-*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver package names should be lowercase with hyphens for multi-word names (e.g.,
my-driver,custom-power,device-controller)
packages/jumpstarter-driver-*/pyproject.toml: Driver packages must follow the naming patternjumpstarter-driver-<name>
Driver packages must register via thejumpstarter.driversentry point inpyproject.toml
Driver packages must depend onjumpstarterand specific hardware libraries in theirpyproject.toml
Files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Each package's
pyproject.tomlmust include project metadata with Apache-2.0 license only
Files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
🧠 Learnings (15)
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: All driver packages should have a symlink in `docs/source/reference/package-apis/drivers/` pointing to the driver's README.md
Applied to files:
docs/source/reference/package-apis/drivers/nanokvm.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
docs/source/reference/package-apis/drivers/nanokvm.mdpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Applied to files:
docs/source/reference/package-apis/drivers/nanokvm.mdpackages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Applied to files:
docs/source/reference/package-apis/drivers/nanokvm.mdpackages/jumpstarter-driver-nanokvm/.gitignorepackages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Applied to files:
docs/source/reference/package-apis/drivers/nanokvm.mdpackages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Core packages must depend on `jumpstarter-protocol`
Applied to files:
docs/source/reference/package-apis/drivers/nanokvm.md
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
docs/source/reference/package-apis/drivers/nanokvm.mdpackages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver package names should be lowercase with hyphens for multi-word names (e.g., `my-driver`, `custom-power`, `device-controller`)
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to examples/*/pyproject.toml : Example packages should depend on relevant driver packages in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to pyproject.toml : Top-level `pyproject.toml` should define the UV workspace and include all packages
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/*/pyproject.toml : Each package's `pyproject.toml` must include project metadata with Apache-2.0 license only
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
🧬 Code graph analysis (2)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (4)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver.py (1)
Composite(17-18)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (1)
PySerial(90-125)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (7)
NanoKVMClient(252-324)snapshot(22-30)snapshot(40-45)info(304-315)get_info(275-287)reboot(289-297)reboot(319-322)packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py (1)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (3)
NanoKVM(320-390)NanoKVMHID(114-291)NanoKVMVideo(17-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (7)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py (1)
1-13: LGTM!The module initialization is clean with appropriate docstring, exports, and
__all__declaration. The omission ofNanoKVMSerialfrom exports is reasonable given it's marked as a placeholder in the driver implementation.packages/jumpstarter-driver-nanokvm/README.md (1)
3-266: LGTM!Comprehensive and well-structured documentation covering installation, configuration options, API reference, CLI usage examples, and Python code samples. The serial console feature is appropriately marked as a future/optional capability.
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py (1)
1-59: LGTM!Well-structured test fixtures that properly mock the NanoKVM API client and aiohttp session. The fixtures enable isolated testing without real network calls.
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (1)
251-324: LGTM!
NanoKVMClientcorrectly extendsCompositeClientand properly implementscli()by extending the base CLI with device-specific commands. Theget_infoandrebootmethods are well-documented with appropriate warnings. Based on learnings, this follows the pattern for composite drivers with child drivers.packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (2)
294-317: Serial driver is a non-functional placeholder.
NanoKVMSerialhas a hardcodedrfc2217://localhost:2217URL and requires manual SSH port forwarding. This is documented in comments but the driver will fail at runtime without user intervention.Consider either:
- Removing
NanoKVMSerialentirely until implemented- Adding a clear error in
__post_init__explaining the manual setup required- Implementing automatic SSH tunnel setup using paramiko (as mentioned in comments)
This placeholder approach is acceptable for initial implementation if the composite driver doesn't enable serial by default (which it doesn't -
enable_serial: bool = False).
75-111: LGTM!The video streaming implementation properly uses
asynccontextmanager, handles task cancellation gracefully, and uses anyio's memory object stream for frame delivery.docs/source/reference/package-apis/drivers/nanokvm.md (1)
1-1: The symlink is correctly configured and points to the appropriate README.md file. No changes needed.
| def close(self): | ||
| """Clean up resources""" | ||
| # Schedule cleanup of aiohttp session | ||
| if self._session is not None and not self._session.closed: | ||
| try: | ||
| import asyncio | ||
| loop = asyncio.get_event_loop() | ||
| if loop.is_running(): | ||
| loop.create_task(self._session.close()) | ||
| else: | ||
| loop.run_until_complete(self._session.close()) | ||
| except Exception as e: | ||
| self.logger.debug(f"Error closing session: {e}") | ||
|
|
There was a problem hiding this comment.
asyncio.get_event_loop() is deprecated in Python 3.12+.
This package requires Python >= 3.12, but get_event_loop() is deprecated and may not work correctly when called from a non-async context. The run_until_complete branch will fail if no loop exists.
Consider using anyio (already a dependency) for cleanup or restructure to use async cleanup:
def close(self):
"""Clean up resources"""
- # Schedule cleanup of aiohttp session
if self._session is not None and not self._session.closed:
try:
- import asyncio
- loop = asyncio.get_event_loop()
- if loop.is_running():
- loop.create_task(self._session.close())
- else:
- loop.run_until_complete(self._session.close())
+ anyio.from_thread.run(self._session.close)
except Exception as e:
self.logger.debug(f"Error closing session: {e}")Or implement an async aclose() method that the framework can call during shutdown.
Committable suggestion skipped: line range outside the PR's diff.
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
Outdated
Show resolved
Hide resolved
| if self.enable_serial: | ||
| # Note: This is a placeholder - actual serial console access via SSH | ||
| # would require additional implementation in the nanokvm library | ||
| self.logger.warning("Serial console access not yet fully implemented") | ||
|
|
||
| super().__post_init__() |
There was a problem hiding this comment.
Potential issue: self.logger accessed before super().__post_init__().
If self.logger is initialized by the parent class's __post_init__, this warning call will fail with an AttributeError.
Move the logger call after super().__post_init__():
# Optionally add serial console access
if self.enable_serial:
- # Note: This is a placeholder - actual serial console access via SSH
- # would require additional implementation in the nanokvm library
- self.logger.warning("Serial console access not yet fully implemented")
super().__post_init__()
+
+ if self.enable_serial:
+ self.logger.warning("Serial console access not yet fully implemented")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
around lines 358-363, the code logs via self.logger before calling
super().__post_init__(), which can raise AttributeError if the parent
initializes logger in its post-init; move the serial-enabled warning so that
super().__post_init__() is called first, then check self.enable_serial and call
self.logger.warning, ensuring any needed attributes are initialized by the
parent before use.
| [project] | ||
| name = "jumpstarter-driver-nanokvm" | ||
| dynamic = ["version", "urls"] | ||
| description = "Jumpstarter driver for NanoKVM devices providing video streaming, keyboard/mouse control, and serial console access" | ||
| readme = "README.md" | ||
| license = "Apache-2.0" | ||
| authors = [ | ||
| { name = "Miguel Angel Ajo", email = "miguelangel@ajo.es" } | ||
| ] | ||
| requires-python = ">=3.12" | ||
| dependencies = [ | ||
| "anyio>=4.10.0", | ||
| "jumpstarter", | ||
| "jumpstarter-driver-composite", | ||
| "jumpstarter-driver-pyserial", | ||
| "nanokvm @ git+https://github.com/puddly/python-nanokvm.git", | ||
| "aiohttp", | ||
| "pillow", | ||
| "pydantic", | ||
| "yarl", | ||
| "click", | ||
| ] | ||
|
|
||
| [tool.hatch.version] | ||
| source = "vcs" | ||
| raw-options = { 'root' = '../../'} | ||
|
|
||
| [tool.hatch.metadata] | ||
| allow-direct-references = true | ||
|
|
||
| [tool.hatch.metadata.hooks.vcs.urls] | ||
| Homepage = "https://jumpstarter.dev" | ||
| source_archive = "https://github.com/jumpstarter-dev/repo/archive/{commit_hash}.zip" | ||
|
|
||
| [tool.pytest.ini_options] | ||
| addopts = "--cov --cov-report=html --cov-report=xml" | ||
| log_cli = true | ||
| log_cli_level = "INFO" | ||
| testpaths = ["jumpstarter_driver_nanokvm"] | ||
| asyncio_mode = "auto" | ||
|
|
||
| [build-system] | ||
| requires = ["hatchling", "hatch-vcs", "hatch-pin-jumpstarter"] | ||
| build-backend = "hatchling.build" | ||
|
|
||
| [tool.hatch.build.hooks.pin_jumpstarter] | ||
| name = "pin_jumpstarter" | ||
|
|
||
| [dependency-groups] | ||
| dev = [ | ||
| "pytest-cov>=6.0.0", | ||
| "pytest>=8.3.3", | ||
| ] |
There was a problem hiding this comment.
Missing jumpstarter.drivers entry point registration.
Per the coding guidelines, driver packages must register via the jumpstarter.drivers entry point. This is required for the driver to be discoverable by Jumpstarter.
Add the entry point section to register the driver:
+[project.entry-points."jumpstarter.drivers"]
+nanokvm = "jumpstarter_driver_nanokvm"
+
[tool.hatch.version]
source = "vcs"🤖 Prompt for AI Agents
In packages/jumpstarter-driver-nanokvm/pyproject.toml around lines 1 to 53, the
package does not register the driver under the jumpstarter.drivers entry point
so Jumpstarter cannot discover it; add a [project.entry-points] section and
register the driver under the jumpstarter.drivers group using the pattern
"<driver-name> = \"<module_path>:<DriverClass>\"" (replace with the actual
driver name, module path and class), then bump any packaging metadata as needed
so the entry point is included in built distributions.
Keeping the requirement to 3.13 makes it incompatible with our current plans to use it from https://jumpstarter.dev, see [1]. i.e. current ubuntu LTS: 3.12, fedora 42 already ships 3.13, centos10 ships python 3.12 [1] jumpstarter-dev/jumpstarter#771
mangelajo
force-pushed
the
nanokvm-driver
branch
from
91dc4d8 to
7b961cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
packages/jumpstarter-driver-nanokvm/pyproject.toml (1)
1-53: Missingjumpstarter.driversentry point registration.This issue was previously flagged and remains unresolved. Per coding guidelines, driver packages must register via the
jumpstarter.driversentry point for Jumpstarter to discover the driver at runtime.Add the entry point section to register the NanoKVM driver:
+[project.entry-points."jumpstarter.drivers"] +nanokvm = "jumpstarter_driver_nanokvm:NanoKVMNetwork" + [tool.hatch.version]Note: Replace
NanoKVMNetworkwith the actual driver class name if it differs.packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (2)
43-55: Fix deprecatedasyncio.get_event_loop()usage inclose()methods.Both
NanoKVMVideo.close()andNanoKVMHID.close()still rely onasyncio.get_event_loop()plusrun_until_complete. On Python 3.12+,get_event_loop()is deprecated and will fail in threads without a current loop; usingrun_until_completeon an arbitrary loop is also fragile.A safer pattern is:
- Prefer the current running loop (if any) via
asyncio.get_running_loop()and schedule cleanup withloop.create_task(...).- If no loop is running (e.g. called from a plain sync context), run an async close helper via
asyncio.run(...), which creates and tears down its own loop.For example:
class NanoKVMVideo(Driver): @@ - def close(self): - """Clean up resources""" - # Schedule cleanup of aiohttp session - if self._session is not None and not self._session.closed: - try: - import asyncio - loop = asyncio.get_event_loop() - if loop.is_running(): - loop.create_task(self._session.close()) - else: - loop.run_until_complete(self._session.close()) - except Exception as e: - self.logger.debug(f"Error closing session: {e}") + async def _aclose(self) -> None: + if self._session is not None and not self._session.closed: + await self._session.close() + + def close(self) -> None: + """Clean up resources.""" + if self._session is None or self._session.closed: + return + try: + loop = asyncio.get_running_loop() + except RuntimeError: + # No running loop: create a temporary one + asyncio.run(self._aclose()) + else: + loop.create_task(self._aclose()) @@ class NanoKVMHID(Driver): @@ - def close(self): - """Clean up resources""" - # Schedule cleanup of aiohttp session and websocket - if self._ws is not None: - try: - import asyncio - loop = asyncio.get_event_loop() - if loop.is_running(): - loop.create_task(self._ws.close()) - except Exception as e: - self.logger.debug(f"Error closing websocket: {e}") - - if self._session is not None and not self._session.closed: - try: - import asyncio - loop = asyncio.get_event_loop() - if loop.is_running(): - loop.create_task(self._session.close()) - else: - loop.run_until_complete(self._session.close()) - except Exception as e: - self.logger.debug(f"Error closing session: {e}") + async def _aclose(self) -> None: + if self._ws is not None: + try: + await self._ws.close() + except Exception as e: + self.logger.debug(f"Error closing websocket: {e}") + if self._session is not None and not self._session.closed: + try: + await self._session.close() + except Exception as e: + self.logger.debug(f"Error closing session: {e}") + + def close(self) -> None: + """Clean up resources.""" + if self._session is None and self._ws is None: + return + try: + loop = asyncio.get_running_loop() + except RuntimeError: + asyncio.run(self._aclose()) + else: + loop.create_task(self._aclose())This avoids deprecated APIs, works both inside and outside an event loop, and centralizes async cleanup logic.
Please verify against the project’s supported Python versions and event-loop model before adopting this pattern.
#!/bin/bash # Quick sanity check: find all remaining uses of get_event_loop / run_until_complete rg -n "get_event_loop|run_until_complete" --type=pyAlso applies to: 167-188
342-363: Avoid usingself.loggerbeforeComposite.__post_init__()has run.In
NanoKVM.__post_init__, the warning forenable_serialruns beforesuper().__post_init__(). If the baseComposite(or a mixin) initializesself.loggerin its own__post_init__, this can raiseAttributeError.Reordering fixes this safely:
def __post_init__(self): # Create child drivers self.children = { @@ - # Optionally add serial console access - if self.enable_serial: - # Note: This is a placeholder - actual serial console access via SSH - # would require additional implementation in the nanokvm library - self.logger.warning("Serial console access not yet fully implemented") - - super().__post_init__() + super().__post_init__() + + # Optionally add serial console access + if self.enable_serial: + # Note: This is a placeholder - actual serial console access via SSH + # would require additional implementation in the nanokvm library + self.logger.warning("Serial console access not yet fully implemented")
🧹 Nitpick comments (1)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (1)
294-317: Align serial driver scaffolding with configuration fields.
NanoKVMSerialand thessh_*/enable_serialfields onNanoKVMimply automatic SSH-tunneled serial support, but:
NanoKVMSerial.__post_init__hardcodesurl = "rfc2217://localhost:2217"and ignores the NanoKVM host/credentials.NanoKVM.__post_init__never constructs a"serial"child driver, even whenenable_serialis true.Consider either:
- Explicitly marking these as experimental placeholders (in code and docs) and clarifying that users must set up port forwarding themselves, or
- Wiring a proper
"serial"child whenenable_serialis enabled and plumbing the SSH parameters intoNanoKVMSerial.Also applies to: 334-341
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/source/reference/package-apis/drivers/index.md(2 hunks)docs/source/reference/package-apis/drivers/nanokvm.md(1 hunks)packages/jumpstarter-driver-nanokvm/.gitignore(1 hunks)packages/jumpstarter-driver-nanokvm/README.md(1 hunks)packages/jumpstarter-driver-nanokvm/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py(1 hunks)packages/jumpstarter-driver-nanokvm/pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/source/reference/package-apis/drivers/nanokvm.md
- packages/jumpstarter-driver-nanokvm/.gitignore
- packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py
- packages/jumpstarter-driver-nanokvm/examples/exporter.yaml
🧰 Additional context used
📓 Path-based instructions (6)
packages/jumpstarter-driver-*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver package names should be lowercase with hyphens for multi-word names (e.g.,
my-driver,custom-power,device-controller)
packages/jumpstarter-driver-*/pyproject.toml: Driver packages must follow the naming patternjumpstarter-driver-<name>
Driver packages must register via thejumpstarter.driversentry point inpyproject.toml
Driver packages must depend onjumpstarterand specific hardware libraries in theirpyproject.toml
Files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
packages/*/pyproject.toml
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Each package's
pyproject.tomlmust include project metadata with Apache-2.0 license only
Files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
🧠 Learnings (15)
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
docs/source/reference/package-apis/drivers/index.mdpackages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Applied to files:
docs/source/reference/package-apis/drivers/index.mdpackages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Applied to files:
docs/source/reference/package-apis/drivers/index.mdpackages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Applied to files:
docs/source/reference/package-apis/drivers/index.mdpackages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-cli-*/pyproject.toml : CLI packages must depend on `jumpstarter` and `jumpstarter-cli-common` in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver package names should be lowercase with hyphens for multi-word names (e.g., `my-driver`, `custom-power`, `device-controller`)
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to examples/*/pyproject.toml : Example packages should depend on relevant driver packages in their `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to pyproject.toml : Top-level `pyproject.toml` should define the UV workspace and include all packages
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/*/pyproject.toml : Each package's `pyproject.toml` must include project metadata with Apache-2.0 license only
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py : Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with `*Power`, Network drivers with `*Network`, Flasher drivers with `*Flasher`, Console drivers with `*Console`, Server drivers with `*Server`
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.toml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-nanokvm/pyproject.tomlpackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
📚 Learning: 2025-02-07T10:16:05.684Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 251
File: packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py:158-165
Timestamp: 2025-02-07T10:16:05.684Z
Learning: In async operations, prefer using the existing event loop through `asyncio.get_running_loop()` rather than creating new ones, as creating and setting global event loops can lead to race conditions, resource leaks, and context switching issues.
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py (1)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (3)
NanoKVM(320-390)NanoKVMHID(114-291)NanoKVMVideo(17-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (5)
docs/source/reference/package-apis/drivers/index.md (1)
60-63: NanoKVM entry wiring in docs index looks correct.The new Media Drivers bullet and
nanokvm.mdtoctree entry are consistent with existing patterns and the package name; no further changes needed.Also applies to: 87-99
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py (1)
1-12: Public exports in__init__are coherent with the documented API.Re-exporting
NanoKVM,NanoKVMVideo, andNanoKVMHIDhere matches the docs and keeps the placeholder serial driver internal; this looks good.packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (2)
22-47: Video client snapshot/CLI behavior matches driver contract.Decoding the base64 snapshot into a
PIL.Imageand wiring it to thesnapshotCLI command is consistent withNanoKVMVideo.snapshotand the README examples.
268-317: Composite client wiring and management CLI look good.
get_info()andreboot()correctly mirror the composite driver’s exports, and theinfo/rebootcommands integrate cleanly with the base composite CLI.packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (1)
190-291: HID RPC surface and logging look solid.The HID exports (
paste_text,press_key,reset_hid,mouse_move_abs/rel,mouse_click,mouse_scroll) line up cleanly with the WebSocket protocol and API client, with sensible logging and argument validation (e.g. warnings for longpress_keyinputs).
| def mouse_click(self, button: str = "left", x: int | None = None, y: int | None = None): | ||
| """ | ||
| Click a mouse button | ||
|
|
||
| Args: | ||
| button: Mouse button to click ("left", "right", "middle") | ||
| x: Optional X coordinate for absolute positioning before click | ||
| y: Optional Y coordinate for absolute positioning before click | ||
|
|
||
| Example:: | ||
|
|
||
| # Click at current position | ||
| hid.mouse_click("left") | ||
|
|
||
| # Click at specific coordinates | ||
| hid.mouse_click("left", 32768, 32768) | ||
|
|
||
| # Right-click | ||
| hid.mouse_click("right") | ||
| """ | ||
| if x is not None and y is not None: | ||
| self.call("mouse_click", button, x, y) | ||
| else: | ||
| self.call("mouse_click", button, None, None) | ||
|
|
There was a problem hiding this comment.
Tighten mouse_click argument handling to avoid silently ignoring a single coordinate.
mouse_click() only treats coordinates as absolute when both x and y are non-None; otherwise it calls self.call("mouse_click", button, None, None). If a caller passes just x or just y, that coordinate is silently ignored.
Consider either:
- Validating that
xandyare provided together (and erroring or warning when only one is set), or - Allowing single-axis absolute moves (e.g. keep current axis when one coordinate is
None).
The CLI only exposes “both or none”, so this is mostly an API ergonomics issue, not a functional bug.
Also applies to: 171-241
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
around lines 128 to 152 (and similarly lines 171 to 241), mouse_click currently
treats coordinates as absolute only when both x and y are non-None and silently
ignores a single provided coordinate; update the method to validate inputs and
either raise a ValueError when exactly one of x or y is provided or implement
single-axis behavior by querying the current pointer position and substituting
the missing coordinate before calling self.call("mouse_click", ...); choose one
approach and apply the same change consistently to the other affected range.
There was a problem hiding this comment.
single coordinate mouse doesn't make a lot of sense, probably not even supported by nanokvm itself.
There was a problem hiding this comment.
@mangelajo, that makes sense! If the underlying NanoKVM API doesn't support single-coordinate positioning, then the current implementation is appropriate. The "both or neither" approach keeps the interface clean and avoids any confusion. Thanks for clarifying! 👍
🧠 Learnings used
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
7b961cc to
96632cf
Compare
| This package provides support for NanoKVM devices, including: | ||
| - Video streaming and snapshot capture | ||
| - Keyboard and mouse HID control | ||
| - Serial console access (optional) |
There was a problem hiding this comment.
serial isn't finished, but will come! :)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (3)
239-260: Sameasyncio.get_event_loop()deprecation issue asNanoKVMVideo.close().Apply the same fix using
anyio.from_thread.run()for both WebSocket and session cleanup.
417-438:self.loggeraccessed beforesuper().__post_init__()may causeAttributeError.If
self.loggeris initialized by the parent class's__post_init__, the warning call on line 436 will fail.Move the logger call after
super().__post_init__():self.children = { "video": NanoKVMVideo( host=self.host, username=self.username, password=self.password, ), "hid": NanoKVMHID( host=self.host, username=self.username, password=self.password, ), } - # Optionally add serial console access - if self.enable_serial: - # Note: This is a placeholder - actual serial console access via SSH - # would require additional implementation in the nanokvm library - self.logger.warning("Serial console access not yet fully implemented") - super().__post_init__() + + if self.enable_serial: + self.logger.warning("Serial console access not yet fully implemented")
79-92:asyncio.get_event_loop()is deprecated in Python 3.12+.This pattern was flagged in a previous review and appears to still be present. Since the package targets Python >= 3.12, this will emit deprecation warnings and may fail if called from a non-async context without an existing event loop.
Apply this diff to use
anyiowhich is already a dependency:def close(self): """Clean up resources""" - # Schedule cleanup of aiohttp session if self._session is not None and not self._session.closed: try: - import asyncio - loop = asyncio.get_event_loop() - if loop.is_running(): - loop.create_task(self._session.close()) - else: - loop.run_until_complete(self._session.close()) + anyio.from_thread.run(self._session.close) except Exception as e: self.logger.debug(f"Error closing session: {e}")
🧹 Nitpick comments (2)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py (1)
169-206: Consider simplifying the redundant ClientSession patching.The mouse tests re-patch
ClientSessioninside the test body even thoughmock_aiohttp_sessionfixture already patches it. While functional, this adds complexity.You could either:
- Remove the fixture usage and keep the inline patch, or
- Enhance the fixture to support WebSocket mocking and remove the inline patch.
The current approach works, so this is a minor cleanup suggestion.
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (1)
369-392: Serial driver is a placeholder - consider adding a clearer warning or disabling instantiation.The
NanoKVMSerialclass has a hardcodedrfc2217://localhost:2217URL and requires manual SSH port forwarding. Sinceenable_serialtriggers a warning in the composite driver but doesn't actually wire up this class, the current implementation could confuse users if they try to use it directly.Consider either:
- Raising
NotImplementedErrorin__post_init__to prevent direct usage, or- Adding a clear docstring warning about the placeholder status.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-driver-nanokvm/README.md(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-02-07T10:16:05.684Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 251
File: packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py:158-165
Timestamp: 2025-02-07T10:16:05.684Z
Learning: In async operations, prefer using the existing event loop through `asyncio.get_running_loop()` rather than creating new ones, as creating and setting global event loops can lead to race conditions, resource leaks, and context switching issues.
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (4)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver.py (1)
Composite(17-18)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (1)
PySerial(90-125)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (5)
NanoKVMClient(246-317)info(297-308)get_info(268-280)reboot(282-290)reboot(312-315)packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(56-273)
⏰ 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 (ubuntu-24.04, 3.11)
- 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.13)
- GitHub Check: e2e
🔇 Additional comments (18)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (5)
1-11: LGTM - Proper imports and structure.The client file correctly imports
CompositeClientfromjumpstarter_driver_composite.clientas required by the coding guidelines for composite drivers with child drivers.
13-47: LGTM - Clean video client implementation.The
NanoKVMVideoClientcorrectly decodes base64 JPEG data from the driver and provides a well-structured CLI interface.
50-170: LGTM - Comprehensive HID client with well-documented API.The HID client methods are well-documented with clear examples. The coordinate handling in
mouse_clickfollows the "both or neither" pattern which aligns with NanoKVM's capabilities as discussed.
171-242: CLI implementation is appropriate, but consider extracting the mouse subgroup.The
# noqa: C901is acceptable given the CLI complexity. The escape sequence handling withtext.encode().decode("unicode_escape")is correct for interpreting backslash sequences from shell input.
245-317: LGTM - Proper composite client implementation.The
NanoKVMClientcorrectly extendsCompositeClientand provides device management commands. The reboot command appropriately requires confirmation to prevent accidental device reboots.packages/jumpstarter-driver-nanokvm/README.md (2)
1-75: LGTM - Comprehensive documentation with clear future feature notation.The README properly documents the NanoKVM driver capabilities, configuration options, and appropriately marks serial console support as a future feature (lines 49, 74). This addresses the previous review concerns about clarity.
76-266: LGTM - Well-structured API reference and usage examples.The documentation provides clear API references using Sphinx autodoc directives, comprehensive CLI usage examples with escape sequence handling notes, and Python usage examples covering all major functionality.
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py (5)
1-52: LGTM - Well-structured mock fixtures.The fixtures properly mock the NanoKVM API client and aiohttp session, providing a solid foundation for testing without real network calls.
64-75: LGTM - Good snapshot test.The test properly verifies that the snapshot returns a PIL Image with expected dimensions.
77-111: LGTM - HID operation tests are thorough.The tests correctly verify that paste_text, reset_hid, and press_key delegate to the underlying mock client.
113-152: LGTM - Composite driver and reboot tests verify integration correctly.Tests properly verify child driver accessibility and that reboot delegates to the API client.
154-167: LGTM - Client class method tests.These tests verify the driver classes return the correct client class paths.
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (6)
17-39: LGTM - Well-designed re-authentication decorator.The
with_reauthdecorator cleanly handles 401 errors by resetting the client and retrying once. The_is_unauthorized_errorhelper handles both typed errors and wrapped string representations.
93-116: LGTM - Snapshot implementation with frame skipping.The snapshot method correctly skips stale frames from the MJPEG stream buffer before capturing a fresh frame. Good use of the
@with_reauthdecorator for automatic re-authentication.
168-237: LGTM - HID driver with proper WebSocket management.The
NanoKVMHIDclass correctly manages WebSocket connections for mouse events with token-based authentication. The_send_mouse_eventhelper provides a clean abstraction for the WebSocket protocol.
262-301: LGTM - HID keyboard operations.The
paste_text,press_key, andreset_hidmethods are properly decorated with@exportand@with_reauth, and delegate to the underlying NanoKVM API client.
303-367: LGTM - Mouse control methods.The mouse operations (move_abs, move_rel, click, scroll) correctly map to WebSocket events. The click method properly sequences move → delay → down → delay → up for reliable click registration.
440-470: LGTM - Composite driver client method and exported operations.The
get_infoandrebootmethods properly delegate to the video driver's API client. The nested_get_info_implfunction with@with_reauthis a clever pattern for applying the decorator to an inline function.
| @exportstream | ||
| @asynccontextmanager | ||
| async def stream(self): | ||
| """ | ||
| Stream video frames as JPEG images | ||
|
|
||
| Yields a stream that provides JPEG image data | ||
| """ | ||
| self.logger.debug("Starting video stream") | ||
| client = await self._get_client() | ||
|
|
||
| # Create a pair of connected streams | ||
| send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=10) | ||
|
|
||
| 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 | ||
|
|
||
| # Start the video streaming task | ||
| task = asyncio.create_task(stream_video()) | ||
|
|
||
| try: | ||
| yield receive_stream | ||
| finally: | ||
| task.cancel() | ||
| try: | ||
| await task | ||
| except asyncio.CancelledError: | ||
| pass | ||
|
|
There was a problem hiding this comment.
Stream error handling re-authenticates but doesn't close send_stream properly on retry path.
In the retry logic (lines 144-150), after re-authentication, frames are sent but send_stream is not wrapped with async with, so it won't be closed when the new stream ends or on error.
Consider refactoring the streaming logic to avoid duplication:
async def stream_video():
+ async def send_frames(kvm_client):
+ async for frame in kvm_client.mjpeg_stream():
+ buffer = BytesIO()
+ frame.save(buffer, format="JPEG")
+ data = buffer.getvalue()
+ await send_stream.send(data)
+
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)
+ await send_frames(client)
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)
+ await send_frames(new_client)
else:
self.logger.error(f"Error streaming video: {e}")
raiseCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
around lines 117 to 166, the retry path after re-authentication sends frames to
send_stream without using "async with send_stream", so the stream isn't properly
closed on completion or error; refactor the streaming logic to avoid duplication
by extracting the frame-sending loop into a helper that takes a client and
always uses "async with send_stream" (or ensures send_stream is entered before
sending), call that helper for the initial client and on retry, and ensure any
exceptions are propagated after closing send_stream; alternatively, wrap both
the initial and retry loops in a single loop that obtains a client, enters
"async with send_stream", iterates client.mjpeg_stream(), and on an unauthorized
error resets the client and repeats so send_stream is always properly closed.
Keeping the requirement to 3.13 makes it incompatible with our current plans to use it from https://jumpstarter.dev, see [1]. i.e. current ubuntu LTS: 3.12, fedora 42 already ships 3.13, centos10 ships python 3.12 [1] jumpstarter-dev/jumpstarter#771
mangelajo
force-pushed
the
nanokvm-driver
branch
from
9f582ae to
9d611a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/jumpstarter-driver-nanokvm/README.md (1)
101-107: Clarify CLI command prefix.The CLI examples use
j nanokvmcommand prefix (lines 107, 110, etc.) without explaining whatjrepresents or how it's invoked. This may be unclear to users unfamiliar with the Jumpstarter CLI conventions. Consider adding a brief sentence explaining this is shorthand within thejmp shellsession or linking to relevant CLI documentation.
📜 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 (3)
.python-version(0 hunks)packages/jumpstarter-driver-nanokvm/README.md(1 hunks)packages/jumpstarter-driver-nanokvm/pyproject.toml(1 hunks)
💤 Files with no reviewable changes (1)
- .python-version
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-nanokvm/pyproject.toml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Core packages must depend on `jumpstarter-protocol`
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-nanokvm/README.md
⏰ 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). (7)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- 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: e2e
🔇 Additional comments (1)
packages/jumpstarter-driver-nanokvm/README.md (1)
76-97: API Reference documentation is accurate and complete.All documented classes (NanoKVMClient, NanoKVMVideoClient, NanoKVMHIDClient) and their specified methods exist in the implementation with matching signatures. The usage examples (lines 196-249) correctly demonstrate the API with accurate parameter names, types, and return values. The explicit
:members:specifications in the autoclass directives appropriately document the public Python API while excluding CLI-only utilities.
mangelajo
force-pushed
the
nanokvm-driver
branch
from
9d611a7 to
a9203f2
Compare
|
HID injection not working yet, I need to test and fix that. |
mangelajo
force-pushed
the
nanokvm-driver
branch
from
357df0c to
6999405
Compare
fixed. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (3)
79-92: Replace deprecatedasyncio.get_event_loop()usage.The
get_event_loop()method is deprecated in Python 3.12+ and may fail when called from a non-async context. Consider usinganyio(already imported) for cleanup or implementing an asyncaclose()method.🔎 Proposed fix using anyio
def close(self): """Clean up resources""" - # Schedule cleanup of aiohttp session if self._session is not None and not self._session.closed: try: - import asyncio - loop = asyncio.get_event_loop() - if loop.is_running(): - loop.create_task(self._session.close()) - else: - loop.run_until_complete(self._session.close()) + anyio.from_thread.run(self._session.close) except Exception as e: self.logger.debug(f"Error closing session: {e}")Based on learnings: Prefer using
anyioor existing event loop patterns over creating/getting global event loops.
443-449: Move logger call aftersuper().__post_init__().If
self.loggeris initialized by the parent class's__post_init__(), calling it beforesuper().__post_init__()(line 449) will raise anAttributeError.🔎 Proposed fix
# Optionally add serial console access - if self.enable_serial: - # Note: This is a placeholder - actual serial console access via SSH - # would require additional implementation in the nanokvm library - self.logger.warning("Serial console access not yet fully implemented") super().__post_init__() + + if self.enable_serial: + # Note: This is a placeholder - actual serial console access via SSH + # would require additional implementation in the nanokvm library + self.logger.warning("Serial console access not yet fully implemented")
250-272: Replace deprecatedasyncio.get_event_loop()usage in HID driver close().The same
get_event_loop()deprecation issue exists here as inNanoKVMVideo.close(). Consider applying the same fix pattern usinganyio.from_thread.run()for both the WebSocket and session cleanup.🔎 Proposed fix
def close(self): """Clean up resources""" - # Schedule cleanup of aiohttp session and websocket if self._ws is not None: try: - import asyncio - loop = asyncio.get_event_loop() - if loop.is_running(): - loop.create_task(self._ws.close()) + anyio.from_thread.run(self._ws.close) except Exception as e: self.logger.debug(f"Error closing websocket: {e}") if self._session is not None and not self._session.closed: try: - import asyncio - loop = asyncio.get_event_loop() - if loop.is_running(): - loop.create_task(self._session.close()) - else: - loop.run_until_complete(self._session.close()) + anyio.from_thread.run(self._session.close) except Exception as e: self.logger.debug(f"Error closing session: {e}")
🧹 Nitpick comments (1)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (1)
117-166: Fix stream error handling to ensure proper cleanup on retry path.The retry logic (lines 144-150) sends frames after re-authentication but doesn't wrap the retry loop with
async with send_stream, sosend_streamwon't be properly closed if the retry stream ends or encounters an error. This can lead to resource leaks.🔎 Proposed refactor to eliminate duplication and ensure cleanup
@exportstream @asynccontextmanager async def stream(self): """ Stream video frames as JPEG images Yields a stream that provides JPEG image data """ self.logger.debug("Starting video stream") client = await self._get_client() # Create a pair of connected streams send_stream, receive_stream = anyio.create_memory_object_stream(max_buffer_size=10) + async def send_frames(kvm_client): + """Helper to send frames from a client""" + async for frame in kvm_client.mjpeg_stream(): + buffer = BytesIO() + frame.save(buffer, format="JPEG") + data = buffer.getvalue() + await send_stream.send(data) + 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) + await send_frames(client) 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) + await send_frames(new_client) else: self.logger.error(f"Error streaming video: {e}") raise # Start the video streaming task task = asyncio.create_task(stream_video()) try: yield receive_stream finally: task.cancel() try: await task except asyncio.CancelledError: pass
📜 Review details
Configuration used: defaults
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/nanokvm.md(1 hunks)packages/jumpstarter-driver-nanokvm/.gitignore(1 hunks)packages/jumpstarter-driver-nanokvm/README.md(1 hunks)packages/jumpstarter-driver-nanokvm/examples/exporter.yaml(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/__init__.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py(1 hunks)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py(1 hunks)packages/jumpstarter-driver-nanokvm/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/jumpstarter-driver-nanokvm/.gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/jumpstarter-driver-nanokvm/pyproject.toml
- packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/init.py
- docs/source/reference/package-apis/drivers/nanokvm.md
- docs/source/reference/package-apis/drivers/index.md
🧰 Additional context used
📓 Path-based instructions (4)
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
📚 Learning: 2025-10-06T15:44:51.111Z
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 690
File: packages/jumpstarter/jumpstarter/config/exporter.py:51-56
Timestamp: 2025-10-06T15:44:51.111Z
Learning: In jumpstarter config instantiation (packages/jumpstarter/jumpstarter/config/exporter.py), passing **self.root.config to driver_class constructors is intentional validation behavior: since Driver classes are dataclasses, the __init__ will reject any config keys that don't match declared fields with a TypeError, ensuring only declared/supported fields can be used in config YAML files. This is the desired behavior for config safety.
Applied to files:
packages/jumpstarter-driver-nanokvm/examples/exporter.yaml
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
📚 Learning: 2025-02-07T10:16:05.684Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 251
File: packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py:158-165
Timestamp: 2025-02-07T10:16:05.684Z
Learning: In async operations, prefer using the existing event loop through `asyncio.get_running_loop()` rather than creating new ones, as creating and setting global event loops can lead to race conditions, resource leaks, and context switching issues.
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-01-29T11:52:43.554Z
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 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:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py (1)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (16)
NanoKVM(406-609)NanoKVMHID(169-377)NanoKVMVideo(43-165)get_info(456-473)paste_text(275-284)reset_hid(308-312)close(79-91)close(250-271)client(58-59)client(185-186)client(401-402)client(452-453)snapshot(95-115)press_key(288-304)mouse_move_abs(315-324)mouse_click(339-365)
⏰ 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 (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
🔇 Additional comments (4)
packages/jumpstarter-driver-nanokvm/README.md (1)
1-349: LGTM! Comprehensive documentation.The README provides excellent coverage of the NanoKVM driver features, installation, configuration, API reference, CLI usage, and examples. The serial console feature is appropriately documented as a future feature (line 79), which aligns with the implementation.
packages/jumpstarter-driver-nanokvm/examples/exporter.yaml (1)
1-19: LGTM! Configuration example is clear and complete.The example YAML correctly demonstrates the NanoKVM driver configuration with all required fields and optional serial console settings appropriately commented out.
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (1)
1-501: LGTM! Well-structured client implementations.The client classes properly implement the required patterns:
NanoKVMVideoClientandNanoKVMHIDClientextendDriverClientNanoKVMClientextendsCompositeClientas required for composite drivers- All methods have comprehensive docstrings with examples
- CLI implementations provide good user experience with escape sequence handling, confirmation prompts, and helpful error messages
Based on learnings: Composite drivers correctly use
CompositeClientfromjumpstarter_driver_composite.client.packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (1)
455-610: LGTM! Well-structured composite driver methods.The device management and image management methods properly delegate to the video driver's client with appropriate
@with_reauthdecorators. The implementation correctly handles edge cases like unmounting images with empty strings.
| def test_nanokvm_mouse_move_abs(mock_nanokvm_client, mock_aiohttp_session): | ||
| """Test mouse absolute movement""" | ||
| with patch("jumpstarter_driver_nanokvm.driver.ClientSession") as mock_session_class: | ||
| mock_ws = AsyncMock() | ||
| mock_ws.send_json = AsyncMock() | ||
| mock_session = AsyncMock() | ||
| mock_session.ws_connect = AsyncMock(return_value=mock_ws) | ||
| mock_session.close = AsyncMock() | ||
| mock_session_class.return_value = mock_session | ||
|
|
||
| hid = NanoKVMHID(host="test.local", username="admin", password="admin") | ||
|
|
||
| with serve(hid) as client: | ||
| # Move mouse to absolute position | ||
| client.mouse_move_abs(32768, 32768) | ||
|
|
||
| # Verify WebSocket message was sent | ||
| mock_ws.send_json.assert_called() | ||
|
|
There was a problem hiding this comment.
Fix incorrect mouse coordinate values in test.
Line 183 passes 32768, 32768 to mouse_move_abs(), but the driver API expects normalized coordinates in the range 0.0-1.0. The driver internally scales these by multiplying by 32768 (see driver.py line 238). Passing 32768 would result in values far outside the valid range.
🔎 Proposed fix
with serve(hid) as client:
# Move mouse to absolute position
- client.mouse_move_abs(32768, 32768)
+ client.mouse_move_abs(0.5, 0.5) # Center of screen
# Verify WebSocket message was sent
mock_ws.send_json.assert_called()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_nanokvm_mouse_move_abs(mock_nanokvm_client, mock_aiohttp_session): | |
| """Test mouse absolute movement""" | |
| with patch("jumpstarter_driver_nanokvm.driver.ClientSession") as mock_session_class: | |
| mock_ws = AsyncMock() | |
| mock_ws.send_json = AsyncMock() | |
| mock_session = AsyncMock() | |
| mock_session.ws_connect = AsyncMock(return_value=mock_ws) | |
| mock_session.close = AsyncMock() | |
| mock_session_class.return_value = mock_session | |
| hid = NanoKVMHID(host="test.local", username="admin", password="admin") | |
| with serve(hid) as client: | |
| # Move mouse to absolute position | |
| client.mouse_move_abs(32768, 32768) | |
| # Verify WebSocket message was sent | |
| mock_ws.send_json.assert_called() | |
| def test_nanokvm_mouse_move_abs(mock_nanokvm_client, mock_aiohttp_session): | |
| """Test mouse absolute movement""" | |
| with patch("jumpstarter_driver_nanokvm.driver.ClientSession") as mock_session_class: | |
| mock_ws = AsyncMock() | |
| mock_ws.send_json = AsyncMock() | |
| mock_session = AsyncMock() | |
| mock_session.ws_connect = AsyncMock(return_value=mock_ws) | |
| mock_session.close = AsyncMock() | |
| mock_session_class.return_value = mock_session | |
| hid = NanoKVMHID(host="test.local", username="admin", password="admin") | |
| with serve(hid) as client: | |
| # Move mouse to absolute position | |
| client.mouse_move_abs(0.5, 0.5) # Center of screen | |
| # Verify WebSocket message was sent | |
| mock_ws.send_json.assert_called() |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py
around lines 169 to 187, the test passes absolute integer values 32768, 32768 to
mouse_move_abs() but the driver API expects normalized floats 0.0–1.0 (the
driver multiplies these by 32768 internally); update the test to call
client.mouse_move_abs(0.5, 0.5) (or another normalized pair within 0.0–1.0) so
the input matches the API, and leave the existing assertion that
mock_ws.send_json was called.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py (1)
174-192: Fix incorrect mouse coordinate values in test.The test passes
32768, 32768tomouse_move_abs(), but per the driver API (driver.py lines 272-278), coordinates should be normalized floats in the range 0.0-1.0. The value 32768 would be far outside the valid range.🔎 Proposed fix
with serve(hid) as client: # Move mouse to absolute position - client.mouse_move_abs(32768, 32768) + client.mouse_move_abs(0.5, 0.5) # Center of screen # Verify WebSocket message was sent mock_ws.send_json.assert_called()packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (4)
86-99:asyncio.get_event_loop()is deprecated in Python 3.12+.This package requires Python >= 3.12, but
get_event_loop()is deprecated and may not work correctly when called from a non-async context.🔎 Suggested fix using anyio
def close(self): """Clean up resources""" - # Schedule cleanup of client if self._client is not None: try: - import asyncio - loop = asyncio.get_event_loop() - if loop.is_running(): - loop.create_task(self._client_ctx.__aexit__(None, None, None)) - else: - loop.run_until_complete(self._client_ctx.__aexit__(None, None, None)) + anyio.from_thread.run(self._client_ctx.__aexit__, None, None, None) except Exception as e: self.logger.debug(f"Error closing client: {e}")
138-160: Stream retry path sends frames aftersend_streamcontext has exited.After the exception handler re-authenticates, frames are sent to
send_streamoutside itsasync withblock (which has already exited due to the exception). This meanssend_streamwon't be properly closed when the retry stream ends.🔎 Suggested refactor to avoid duplication and ensure proper cleanup
async def stream_video(): + async def send_frames(kvm_client): + async for frame in kvm_client.mjpeg_stream(): + buffer = BytesIO() + frame.save(buffer, format="JPEG") + data = buffer.getvalue() + await send_stream.send(data) + 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) + try: + await send_frames(client) + except Exception as e: + if _is_unauthorized_error(e): + self.logger.warning("Received 401 Unauthorized during stream, re-authenticating...") + await self._reset_client() + new_client = await self._get_client() + await send_frames(new_client) + else: + raise 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 + self.logger.error(f"Error streaming video: {e}") + raise
215-228: Sameasyncio.get_event_loop()deprecation issue.The
closemethod inNanoKVMHIDhas the same deprecated pattern asNanoKVMVideo.close().
401-406:self.loggeraccessed beforesuper().__post_init__().If
self.loggeris initialized by the parent class's__post_init__, this warning call will fail with anAttributeError.🔎 Proposed fix
# Optionally add serial console access if self.enable_serial: - # Note: This is a placeholder - actual serial console access via SSH - # would require additional implementation in the nanokvm library - self.logger.warning("Serial console access not yet fully implemented") super().__post_init__() + if self.enable_serial: + # Note: This is a placeholder - actual serial console access via SSH + # would require additional implementation in the nanokvm library + self.logger.warning("Serial console access not yet fully implemented")
🧹 Nitpick comments (1)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (1)
526-535: Consider thatlistshadows the built-in.The command function
listshadows Python's built-inlist. While this works in Click's context, it could cause confusion if the function body ever needs to use the built-in.🔎 Suggested alternative
- @image.command() - def list(): + @image.command(name="list") + def list_images(): """List available image files""" images = self.get_images() if images:
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter-driver-nanokvm/pyproject.toml
🧰 Additional context used
📓 Path-based instructions (4)
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver implementations should follow existing code style validated with
make lint(fix withmake lint-fix), perform static type checking withmake ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass withmake test-pkg-${package_name}ormake test
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)
Ruff should be used for code formatting and linting, excluding
jumpstarter-protocolpackage
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use
CompositeClientfromjumpstarter_driver_composite.clientfor composite drivers with child drivers.
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
packages/jumpstarter-driver-*/jumpstarter_driver_*/driver.py
📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)
Driver class names should be in CamelCase and be descriptive with appropriate suffixes based on functionality: Power drivers should end with
*Power, Network drivers with*Network, Flasher drivers with*Flasher, Console drivers with*Console, Server drivers with*Server
Files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must depend on `jumpstarter` and specific hardware libraries in their `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must register via the `jumpstarter.drivers` entry point in `pyproject.toml`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/pyproject.toml : Driver packages must follow the naming pattern `jumpstarter-driver-<name>`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py : Driver implementations should follow existing code style validated with `make lint` (fix with `make lint-fix`), perform static type checking with `make ty-pkg-${package_name}`, add comprehensive tests, and verify all tests pass with `make test-pkg-${package_name}` or `make test`
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-composite/pyproject.toml : Composite drivers that have child drivers should inherit from `CompositeClient` in `jumpstarter_driver_composite.client` and have a dependency on `jumpstarter-driver-composite` in `pyproject.toml`
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.pypackages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `driver.py` file containing the driver implementation
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
📚 Learning: 2025-02-07T10:16:05.684Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 251
File: packages/jumpstarter-driver-snmp/jumpstarter_driver_snmp/driver.py:158-165
Timestamp: 2025-02-07T10:16:05.684Z
Learning: In async operations, prefer using the existing event loop through `asyncio.get_running_loop()` rather than creating new ones, as creating and setting global event loops can lead to race conditions, resource leaks, and context switching issues.
Applied to files:
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (4)
packages/jumpstarter-driver-composite/jumpstarter_driver_composite/driver.py (1)
Composite(17-18)packages/jumpstarter-driver-pyserial/jumpstarter_driver_pyserial/driver.py (1)
PySerial(90-125)packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (17)
NanoKVMClient(265-537)status(491-500)snapshot(26-34)snapshot(44-49)paste_text(63-76)info(430-441)get_info(287-299)image(451-453)reboot(301-309)reboot(445-448)mount_image(311-334)download_image(336-351)get_mounted_image(353-366)get_cdrom_status(368-380)is_image_download_enabled(382-394)get_image_download_status(396-408)get_images(410-423)packages/jumpstarter/jumpstarter/driver/base.py (1)
Driver(57-274)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: e2e
🔇 Additional comments (12)
packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver_test.py (4)
12-56: Well-structured mock fixture for comprehensive testing.The
mock_nanokvm_clientfixture properly mocks all API client methods including authentication, info, video streaming, HID functions, reboot, and image management. The async generator pattern formock_streamis appropriate for testing the MJPEG streaming behavior.
69-79: LGTM!The video snapshot test properly verifies the PIL Image type and expected dimensions from the mock.
118-143: Good composite driver test coverage.The test validates child driver access (
video,hid), snapshot through composite, HID operations, andget_inforesponse structure. This ensures the composite wiring works correctly.
214-233: LGTM!The
get_imagestest properly validates the list response and mock interaction.packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/client.py (4)
17-52: LGTM!
NanoKVMVideoClientcorrectly decodes base64 JPEG data and returns a PIL Image. The CLI provides a straightforward snapshot command with file output.
54-181: Comprehensive HID client with good documentation.The
NanoKVMHIDClientprovides well-documented methods for keyboard and mouse control with clear examples in docstrings. The coordinate ranges (0.0-1.0) are properly documented.
183-261: CLI implementation looks good.The HID CLI properly handles escape sequence decoding for paste and press commands, and provides a well-organized mouse subgroup with move, click, and scroll commands.
264-286: Composite client properly extends CompositeClient.
NanoKVMClientcorrectly inherits fromCompositeClientas required by the coding guidelines for composite drivers with child drivers.packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py (4)
21-43: LGTM! Clean re-authentication decorator.The
with_reauthdecorator properly handles 401 errors and provides a single retry after re-authentication. The_is_unauthorized_errorhelper handles both directClientResponseErrorand wrapped error strings.
337-360: Serial driver is a placeholder with hardcoded localhost URL.The
NanoKVMSerialclass hardcodesrfc2217://localhost:2217and requires manual SSH port forwarding. This is noted in comments but may confuse users who enable serial without additional setup.Consider adding validation or a clearer error message when the serial connection fails due to missing tunnel setup.
46-84: LGTM!
NanoKVMVideodriver properly implements lazy client initialization with context manager handling and authentication. Thesnapshotmethod with frame skipping is a good approach for getting fresh frames from the MJPEG stream.
362-411: Composite driver structure follows guidelines.
NanoKVMcorrectly extendsComposite, creates child drivers for video and HID, and returns the appropriate client class. The children dict structure matches the expected pattern.
| @export | ||
| async def get_info(self): | ||
| """Get device information""" | ||
| # Get info from the video driver's client | ||
| video_driver = self.children["video"] | ||
|
|
||
| @with_reauth | ||
| async def _get_info_impl(driver): | ||
| client = await driver._get_client() | ||
| info = await client.get_info() | ||
| return { | ||
| "ips": [{"name": ip.name, "addr": ip.addr, "version": ip.version, "type": ip.type} for ip in info.ips], | ||
| "mdns": info.mdns, | ||
| "image": info.image, | ||
| "application": info.application, | ||
| "device_key": info.device_key, | ||
| } | ||
|
|
||
| return await _get_info_impl(video_driver) | ||
|
|
There was a problem hiding this comment.
@with_reauth decorator on inner function has incorrect self binding.
The _get_info_impl inner function is decorated with @with_reauth, but when invoked as _get_info_impl(video_driver), the decorator's self parameter receives video_driver. However, the decorator calls self._reset_client() expecting the first argument to have this method. While video_driver does have _reset_client, the pattern is fragile and unconventional.
Consider either:
- Calling the decorator's retry logic directly on the video_driver methods, or
- Using a simpler try/except pattern inline
🔎 Suggested inline approach
@export
async def get_info(self):
"""Get device information"""
video_driver = self.children["video"]
-
- @with_reauth
- async def _get_info_impl(driver):
- client = await driver._get_client()
+ try:
+ client = await video_driver._get_client()
info = await client.get_info()
- return {
+ except Exception as e:
+ if _is_unauthorized_error(e):
+ self.logger.warning("Received 401 Unauthorized, re-authenticating...")
+ await video_driver._reset_client()
+ client = await video_driver._get_client()
+ info = await client.get_info()
+ else:
+ raise
+ return {
"ips": [{"name": ip.name, "addr": ip.addr, "version": ip.version, "type": ip.type} for ip in info.ips],
"mdns": info.mdns,
"image": info.image,
"application": info.application,
"device_key": info.device_key,
}
-
- return await _get_info_impl(video_driver)| @export | ||
| async def reboot(self): | ||
| """Reboot the NanoKVM device""" | ||
| video_driver = self.children["video"] | ||
| client = await video_driver._get_client() | ||
| await client.reboot_system() | ||
| self.logger.info("NanoKVM device rebooted") |
There was a problem hiding this comment.
reboot method missing re-authentication handling.
Unlike other composite methods that use @with_reauth on inner functions, reboot directly accesses the client without any 401 retry logic. If the token expires before calling reboot_system(), the operation will fail.
🔎 Suggested fix
@export
async def reboot(self):
"""Reboot the NanoKVM device"""
video_driver = self.children["video"]
- client = await video_driver._get_client()
- await client.reboot_system()
+
+ @with_reauth
+ async def _reboot_impl(driver):
+ client = await driver._get_client()
+ await client.reboot_system()
+
+ await _reboot_impl(video_driver)
self.logger.info("NanoKVM device rebooted")🤖 Prompt for AI Agents
In packages/jumpstarter-driver-nanokvm/jumpstarter_driver_nanokvm/driver.py
around lines 432-438, the reboot method calls video_driver._get_client() and
client.reboot_system() directly without 401 re-authentication retry logic; wrap
the client call in an inner async function decorated with @with_reauth (matching
the pattern used by other composite methods) or otherwise call the driver's
reauth helper so that if a 401 occurs the code re-obtains a fresh token and
retries; ensure the inner function obtains the client after reauth (or
re-fetches it on retry), awaits client.reboot_system(), and preserves the
existing self.logger.info("NanoKVM device rebooted") and error propagation.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.