Address CodeRabbit review feedback

- Replace PIPE with DEVNULL in Popen to prevent deadlocks (pipes
  were never read)
- Wrap monitor setup in try-except to teardown subprocess on failure,
  preventing process leaks
- Use anyio.to_thread.run_sync for blocking wait() in off() to avoid
  blocking the event loop
- Raise RenodeMonitorError on error responses instead of silently
  returning error text
- Accept multi-word monitor commands in CLI via nargs=-1
- Rename test_power_close_calls_off to test_power_close_terminates_process
- Add docstrings across all public APIs

Made-with: Cursor

Author: vtz

python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/client.py

@@ -3,30 +3,36 @@
 
 
 class RenodeClient(CompositeClient):
+    """Client for interacting with a Renode composite driver."""
+
     @property
     def platform(self) -> str:
+        """The Renode platform description path."""
         return self.call("get_platform")
 
     @property
     def uart(self) -> str:
+        """The UART peripheral path in the Renode object model."""
         return self.call("get_uart")
 
     @property
     def machine_name(self) -> str:
+        """The Renode machine name."""
         return self.call("get_machine_name")
 
     def monitor_cmd(self, command: str) -> str:
         """Send an arbitrary command to the Renode monitor."""
         return self.call("monitor_cmd", command)
 
     def cli(self):
+        """Extend the composite CLI with a ``monitor`` subcommand."""
         base = super().cli()
 
         @base.command(name="monitor")
-        @click.argument("command")
+        @click.argument("command", nargs=-1, required=True)
         def monitor_command(command):
             """Send a command to the Renode monitor."""
-            result = self.monitor_cmd(command)
+            result = self.monitor_cmd(" ".join(command))
             if result.strip():
                 click.echo(result.strip())
 

python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver.py

@@ -6,9 +6,10 @@
 from collections.abc import AsyncGenerator
 from dataclasses import dataclass, field
 from pathlib import Path
-from subprocess import PIPE, Popen, TimeoutExpired
+from subprocess import DEVNULL, Popen, TimeoutExpired
 from tempfile import TemporaryDirectory
 
+from anyio import to_thread
 from anyio.streams.file import FileWriteStream
 from jumpstarter_driver_opendal.driver import FlasherInterface
 from jumpstarter_driver_power.driver import PowerInterface, PowerReading
@@ -67,18 +68,22 @@ async def flash(self, source, load_command: str | None = None):
 
     @export
     async def dump(self, target, partition: str | None = None):
+        """Not supported for Renode targets."""
         raise NotImplementedError("dump is not supported for Renode targets")
 
 
 @dataclass(kw_only=True)
 class RenodePower(PowerInterface, Driver):
+    """Power controller that manages the Renode process lifecycle."""
+
     parent: Renode
 
     _process: Popen | None = field(init=False, default=None, repr=False)
     _monitor: RenodeMonitor | None = field(init=False, default=None, repr=False)
 
     @export
     async def on(self) -> None:
+        """Start Renode, connect monitor, configure platform, and begin simulation."""
         if self._process is not None:
             self.logger.warning("already powered on, ignoring request")
             return
@@ -96,39 +101,44 @@ async def on(self) -> None:
         ]
 
         self.logger.info("starting Renode: %s", " ".join(cmdline))
-        self._process = Popen(cmdline, stdin=PIPE, stdout=PIPE, stderr=PIPE)
+        self._process = Popen(cmdline, stdin=DEVNULL, stdout=DEVNULL, stderr=DEVNULL)
 
         self._monitor = RenodeMonitor()
-        await self._monitor.connect("127.0.0.1", port)
-
-        machine = self.parent.machine_name
-        await self._monitor.execute(f'mach create "{machine}"')
-        await self._monitor.execute(
-            f'machine LoadPlatformDescription @"{self.parent.platform}"'
-        )
-
-        pty_path = self.parent._pty
-        await self._monitor.execute(
-            f'emulation CreateUartPtyTerminal "term" "{pty_path}"'
-        )
-        await self._monitor.execute(
-            f"connector Connect {self.parent.uart} term"
-        )
+        try:
+            await self._monitor.connect("127.0.0.1", port)
 
-        for cmd in self.parent.extra_commands:
-            await self._monitor.execute(cmd)
+            machine = self.parent.machine_name
+            await self._monitor.execute(f'mach create "{machine}"')
+            await self._monitor.execute(
+                f'machine LoadPlatformDescription @"{self.parent.platform}"'
+            )
 
-        if self.parent._firmware_path:
-            load_cmd = self.parent._load_command or "sysbus LoadELF"
+            pty_path = self.parent._pty
+            await self._monitor.execute(
+                f'emulation CreateUartPtyTerminal "term" "{pty_path}"'
+            )
             await self._monitor.execute(
-                f'{load_cmd} @"{self.parent._firmware_path}"'
+                f"connector Connect {self.parent.uart} term"
             )
 
-        await self._monitor.execute("start")
-        self.logger.info("Renode simulation started")
+            for cmd in self.parent.extra_commands:
+                await self._monitor.execute(cmd)
+
+            if self.parent._firmware_path:
+                load_cmd = self.parent._load_command or "sysbus LoadELF"
+                await self._monitor.execute(
+                    f'{load_cmd} @"{self.parent._firmware_path}"'
+                )
+
+            await self._monitor.execute("start")
+            self.logger.info("Renode simulation started")
+        except Exception:
+            await self.off()
+            raise
 
     @export
     async def off(self) -> None:
+        """Stop simulation, disconnect monitor, and terminate the Renode process."""
         if self._process is None:
             self.logger.warning("already powered off, ignoring request")
             return
@@ -143,16 +153,18 @@ async def off(self) -> None:
 
         self._process.terminate()
         try:
-            self._process.wait(timeout=5)
+            await to_thread.run_sync(self._process.wait, 5)
         except TimeoutExpired:
             self._process.kill()
         self._process = None
 
     @export
     async def read(self) -> AsyncGenerator[PowerReading, None]:
+        """Not supported — Renode does not provide power readings."""
         raise NotImplementedError
 
     def close(self):
+        """Synchronous cleanup for use during driver teardown."""
         if self._process is not None:
             if self._monitor is not None:
                 self._monitor = None
@@ -194,6 +206,7 @@ class Renode(Driver):
 
     @classmethod
     def client(cls) -> str:
+        """Return the fully-qualified client class name."""
         return "jumpstarter_driver_renode.client.RenodeClient"
 
     def __post_init__(self):
@@ -210,14 +223,17 @@ def _pty(self) -> str:
 
     @export
     def get_platform(self) -> str:
+        """Return the Renode platform description path."""
         return self.platform
 
     @export
     def get_uart(self) -> str:
+        """Return the UART peripheral path in the Renode object model."""
         return self.uart
 
     @export
     def get_machine_name(self) -> str:
+        """Return the Renode machine name."""
         return self.machine_name
 
     @export

python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/driver_test.py

@@ -10,7 +10,7 @@
 import pytest
 
 from jumpstarter_driver_renode.driver import Renode, RenodeFlasher, RenodePower
-from jumpstarter_driver_renode.monitor import RenodeMonitor
+from jumpstarter_driver_renode.monitor import RenodeMonitor, RenodeMonitorError
 
 from jumpstarter.common.utils import serve
 
@@ -73,7 +73,7 @@ async def test_monitor_execute_command(self):
 
     @pytest.mark.anyio
     async def test_monitor_execute_error_response(self):
-        """Monitor returns error text from Renode without raising."""
+        """Monitor raises RenodeMonitorError on error responses."""
         monitor = RenodeMonitor()
         stream = AsyncMock()
         stream.receive = AsyncMock(
@@ -82,8 +82,8 @@ async def test_monitor_execute_error_response(self):
         monitor._stream = stream
         monitor._buffer = b""
 
-        result = await monitor.execute("bad command")
-        assert "Could not find peripheral" in result
+        with pytest.raises(RenodeMonitorError, match="Could not find peripheral"):
+            await monitor.execute("bad command")
 
     @pytest.mark.anyio
     async def test_monitor_execute_not_connected(self):

python/packages/jumpstarter-driver-renode/jumpstarter_driver_renode/monitor.py

@@ -40,15 +40,25 @@ async def connect(self, host: str, port: int, timeout: float = 10) -> None:
                 except OSError:
                     await sleep(0.5)
 
+    _ERROR_MARKERS = ("Could not find", "Error", "Invalid", "Failed", "Unknown")
+
     async def execute(self, command: str) -> str:
-        """Send a command and return the response text (excluding the prompt)."""
+        """Send a command and return the response text (excluding the prompt).
+
+        Raises RenodeMonitorError if the response indicates a command failure.
+        """
         if self._stream is None:
             raise RuntimeError("not connected to Renode monitor")
 
         logger.debug("monitor> %s", command)
         await self._stream.send(f"{command}\n".encode())
         response = await self._read_until_prompt()
         logger.debug("monitor< %s", response.strip())
+
+        stripped = response.strip()
+        if stripped and any(stripped.startswith(m) for m in self._ERROR_MARKERS):
+            raise RenodeMonitorError(stripped)
+
         return response
 
     async def disconnect(self) -> None: