fix: on-device installs, rmpp 3.20 downgrade regression. Fixes #158 #159
Conversation
WalkthroughMoves Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
codexctl/device.py (1)
416-417: Cleanup subprocess calls don't check return values.If
umountfails, therm -rfstill runs which could attempt to delete a still-mounted filesystem. While in afinallyblock for best-effort cleanup, consider checking the umount result before removing the directory:finally: - subprocess.run(["umount", mount_point]) - subprocess.run(["rm", "-rf", mount_point]) + umount_result = subprocess.run(["umount", mount_point], capture_output=True) + if umount_result.returncode == 0: + subprocess.run(["rm", "-rf", mount_point])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codexctl/__init__.py(1 hunks)codexctl/device.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
codexctl/device.py
311-311: Consider moving this statement to an else block
(TRY300)
328-328: Avoid specifying long messages outside the exception class
(TRY003)
335-335: Avoid specifying long messages outside the exception class
(TRY003)
337-337: Avoid specifying long messages outside the exception class
(TRY003)
354-354: subprocess call: check for execution of untrusted input
(S603)
383-383: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
395-395: Abstract raise to an inner function
(TRY301)
395-395: Avoid specifying long messages outside the exception class
(TRY003)
405-405: subprocess call: check for execution of untrusted input
(S603)
406-406: Starting a process with a partial executable path
(S607)
410-410: Abstract raise to an inner function
(TRY301)
410-410: Avoid specifying long messages outside the exception class
(TRY003)
416-416: subprocess call: check for execution of untrusted input
(S603)
416-416: Starting a process with a partial executable path
(S607)
417-417: subprocess call: check for execution of untrusted input
(S603)
417-417: Starting a process with a partial executable path
(S607)
⏰ 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). (4)
- GitHub Check: Build for ubuntu-latest
- GitHub Check: Build for macos-latest
- GitHub Check: Build for windows-latest
- GitHub Check: Build for remarkable
🔇 Additional comments (6)
codexctl/device.py (6)
294-337: Well-structured abstraction for unified file access.The refactored
_read_version_from_pathcleanly handles both SFTP and local file access via the nested helper functions. The fallback fromupdate.conftoos-releaseis correct for supporting both old and new update engines.
339-355: Good helper for active device detection.The conditional logic for command selection based on hardware type is appropriate—Paper Pro variants use
swupdate -gwhile RM1/RM2 userootdev. The subprocess call is safe since the command string is hardcoded.
357-369: Partition parsing logic is correct for expected device paths.The helper correctly extracts partition information from device paths like
/dev/mmcblk2p2. The assumption that active/inactive partitions are 2 and 3 is valid for reMarkable's A/B partition scheme.
423-475: Good refactoring using new helper methods.The method now uses
_get_active_deviceand_parse_partition_infofor cleaner code. The version-aware partition scheme detection (3.22+ boundary) and proper fallback for both SSH and local contexts is well implemented.
499-511: Consistent local file handling with explicit encoding.The call to
_read_version_from_path()without theftpparameter correctly triggers local file access. Adding explicitencoding="utf-8"to file opens is good practice.
779-797: Implementation change correctly addresses the regression.Switching from
swupdate-from-image-fileto sourcing the args file and runningswupdatedirectly is a good approach. This bypasses the problematic "old A/B downgrade" check while maintaining proper swupdate configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
codexctl/device.py (2)
295-340: Harden SFTPfile_existshandling in_read_version_from_pathThe SFTP
file_existshelper currently only catchesFileNotFoundError. Paramiko’sSFTPClient.stattypically raises anOSError/IOError(witherrno.ENOENT) for missing files rather than a concreteFileNotFoundError, so non‑existent paths may currently bubble up as unexpected exceptions instead of a cleanFalse.You can make this more robust by widening the exception:
- if ftp: - def file_exists(path: str) -> bool: - try: - ftp.stat(path) - return True - except FileNotFoundError: - return False + if ftp: + def file_exists(path: str) -> bool: + try: + ftp.stat(path) + return True + except OSError: + # Paramiko uses OSError/IOError for missing paths. + return FalseSame comment applies to the SFTP helpers in
_get_paper_pro_partition_info, for consistency.
374-425: Close SFTP handle when reading backup-partition versionIn
_get_backup_partition_version, the remote branch opens an SFTP session but never closes it, which can leak channels if this method is called multiple times or fails early:ftp = self.client.open_sftp() ... try: version, _ = self._read_version_from_path(ftp, mount_point) return version finally: self.client.exec_command(f"umount {mount_point}") self.client.exec_command(f"rm -rf {mount_point}") # ftp is never closedYou can close the SFTP handle in the
finallyblock with minimal change:- try: - version, _ = self._read_version_from_path(ftp, mount_point) - return version - finally: - self.client.exec_command(f"umount {mount_point}") - self.client.exec_command(f"rm -rf {mount_point}") + try: + version, _ = self._read_version_from_path(ftp, mount_point) + return version + finally: + self.client.exec_command(f"umount {mount_point}") + self.client.exec_command(f"rm -rf {mount_point}") + ftp.close()This keeps resource usage under control, especially if status queries end up invoking this helper frequently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codexctl/device.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
codexctl/device.py
312-312: Consider moving this statement to an else block
(TRY300)
331-331: Avoid specifying long messages outside the exception class
(TRY003)
338-338: Avoid specifying long messages outside the exception class
(TRY003)
340-340: Avoid specifying long messages outside the exception class
(TRY003)
357-357: subprocess call: check for execution of untrusted input
(S603)
386-386: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
398-398: Abstract raise to an inner function
(TRY301)
398-398: Avoid specifying long messages outside the exception class
(TRY003)
408-408: subprocess call: check for execution of untrusted input
(S603)
409-409: Starting a process with a partial executable path
(S607)
413-413: Abstract raise to an inner function
(TRY301)
413-413: Avoid specifying long messages outside the exception class
(TRY003)
419-419: subprocess call: check for execution of untrusted input
(S603)
419-419: Starting a process with a partial executable path
(S607)
420-420: subprocess call: check for execution of untrusted input
(S603)
420-420: Starting a process with a partial executable path
(S607)
452-452: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (4)
codexctl/device.py (4)
435-487: Paper Pro partition detection logic looks consistentThe refactor to use
_get_active_device+_parse_partition_infoand then derivenext_boot_partvia theboot_part/root_partsysfs files is clear and symmetric between SSH and local access (via thefile_exists/read_filehelpers). The 3.22 boundary check oncurrent_versionis straightforward, and the fallback fromboot_parttoroot_partwhen sysfs entries are missing should handle mixed old/new layouts well.I don’t see any correctness problems in this block as written.
488-532: Good reuse of_read_version_from_pathand explicit encodings inget_device_statusRouting both remote and local version detection through
_read_version_from_pathremoves duplicated parsing logic and keeps the “old vs new update engine” detection in one place. The addedencoding="utf-8"on the local/etc/versionandxochitl.confreads is also a nice touch for consistency with the SFTP reads.No changes requested here.
810-886: Bootloader update helper looks solid overallThe
_update_paper_pro_bootloaderflow — creating temp files, uploading via SFTP, runningpreinstandpostinstwith exit‑status checks, and cleaning up both remote and local artifacts — is structured well and should be robust for the rM Pro downgrade case.No blocking issues from my side; just ensure this path is exercised on real hardware since failures here are hard to simulate in tests.
5-5: Fix remote swupdate command construction to useshlex.quotesafelyImporting
shlexand using it for the localbash -ccall is good, but the remote path currently mixes a manualbash -c '...'{shlex.quote(out_location)}'pattern withshlex.quote(out_location). That pattern is not whatshlex.quoteis designed for:
- For image paths containing spaces or shell metacharacters, embedding the
shlex.quote(...)result inside a single‑quotedbash -c '...'string can still break parsing, and it underminesshlex.quote's safety guarantees.- We already have a shell (
/bin/sh -c) around the command on the remote side; the simplest and safest option is to build a single script string and quote it once.You can keep using
bashand09-swupdate-args, but restructure the command like this:print("\nDone! Running swupdate (PLEASE BE PATIENT, ~5 MINUTES)") - command = f"bash -c 'source /usr/lib/swupdate/conf.d/09-swupdate-args && swupdate $SWUPDATE_ARGS -i {shlex.quote(out_location)}'" - self.logger.debug(command) - _stdin, stdout, _stderr = self.client.exec_command(command) + script = ( + f"source /usr/lib/swupdate/conf.d/09-swupdate-args " + f"&& swupdate $SWUPDATE_ARGS -i {shlex.quote(out_location)}" + ) + command = f"bash -c {shlex.quote(script)}" + self.logger.debug(command) + _stdin, stdout, _stderr = self.client.exec_command(command)The local branch is already in good shape:
command = [ "bash", "-c", f"source /usr/lib/swupdate/conf.d/09-swupdate-args && " f"swupdate $SWUPDATE_ARGS -i {shlex.quote(version_file)}", ]With this change, both local and remote invocations:
- Use
shlex.quoteexactly where it's intended (to build shell command strings), and- Correctly handle image paths with spaces or special characters without opening an injection surface.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
codexctl/device.py (2)
744-765: Remote swupdate command still fragile when filename needs quotingThis line tries to address the earlier quoting issue by wrapping
out_locationinshlex.quote, but because it’s interpolated inside a single-quotedbash -c '...'string, paths that actually need quoting (spaces, special chars) will still break the command:command = ( "bash -c 'source /usr/lib/swupdate/conf.d/09-swupdate-args " f"&& swupdate $SWUPDATE_ARGS -i {shlex.quote(out_location)}'" )Example: if
out_locationcontained a space,shlex.quotewould yield'/tmp/foo bar', which injects nested single quotes into the outer'...'and produces an invalid shell command.A safer pattern is to build the script separately and quote it once as an argument to
bash -c:script = ( "source /usr/lib/swupdate/conf.d/09-swupdate-args " f"&& swupdate $SWUPDATE_ARGS -i {shlex.quote(out_location)}" ) command = f"bash -c {shlex.quote(script)}"This keeps
$SWUPDATE_ARGSexpansion inside bash, while correctly handling any filename that requires quoting.Given this was intended to fix a prior quoting issue, I’d treat updating this as important before relying on arbitrary image paths.
800-816: Local swupdate invocation and quoting look correctFor the local (on-device) case:
command = [ "bash", "-c", "source /usr/lib/swupdate/conf.d/09-swupdate-args " f"&& swupdate $SWUPDATE_ARGS -i {shlex.quote(version_file)}", ]Here
shlex.quote(version_file)is passed directly into the script run bybash -c, which is exactly what we want: filenames with spaces or shell metacharacters are safely quoted in the inner shell, and$SWUPDATE_ARGSis expanded after sourcing the args file.This addresses the earlier local-path quoting concern cleanly.
🧹 Nitpick comments (7)
codexctl/device.py (7)
295-340: Unified_read_version_from_pathlooks good; small optional cleanupsThe ftp/local abstraction and reuse via
file_exists/read_fileis a nice improvement and makes callers much simpler. Two minor, optional tweaks:
update_conf_path/os_release_pathconstruction is duplicated in a few places; consider extracting a tiny helper if you end up needing similar logic elsewhere.- For readability and slightly better diagnostics, you could distinguish “file missing” vs “key missing inside file” in the
SystemErrormessages (currently both are lumped as generic failures).Functionally this looks sound and meets the PR goals.
342-369: Active-device detection and error handling are solid; minor subprocess nitThe new
_get_active_devicecorrectly:
- Picks
swupdate -gfor Paper Pro(-Move) androotdevelsewhere.- Checks remote/local exit status and stderr and raises a clear
SystemErroron failure.One minor, non-blocking suggestion: instead of
cmd.split(), you could pass the argv list explicitly, e.g.:if self.hardware in (HardwareType.RMPP, HardwareType.RMPPM): argv = ["swupdate", "-g"] else: argv = ["rootdev"] result = subprocess.run(argv, capture_output=True, text=True)This avoids any ambiguity if the command ever stops being a simple two-word string.
370-382: Partition parsing assumes 2/3 scheme; consider guarding unexpected values
_parse_partition_infois straightforward and matches the expected...p2/...p3scheme, but it will silently treat any non-2 value as “active=other, inactive=2”. Ifrootdev/swupdate -gever returns something unexpected, that could mis-point the mount.Consider explicitly validating the parsed
active_partand raising if it’s not 2 or 3:if active_part not in (2, 3): raise SystemError(f"Unexpected active partition {active_part} from {active_device}") inactive_part = 3 if active_part == 2 else 2That would fail fast with a clearer error if the platform ever changes.
384-435: Backup-partition version logic works; tighten mountpoint & cleanup handlingThe new
_get_backup_partition_versioncorrectly:
- Reuses
_get_active_device/_parse_partition_info.- Handles SSH vs local mounting.
- Treats Paper Pro failures as fatal while returning
""for RM1/RM2.A few non-blocking hardening suggestions:
- The fixed mountpoint
/tmp/mount_p{inactive_part}is workable but does open the usual/tmpTOCTOU/symlink window. Usingtempfile.TemporaryDirectory(dir="/tmp")(or similar) for the mountpoint would mitigate that and avoid the explicitrm -rf.- For the local branch, using
shutil.rmtree(mount_point)instead ofsubprocess.run(["rm", "-rf", mount_point])is safer and easier to reason about.- In the SSH branch,
ftpis never closed; not critical here, but explicitlyftp.close()in thefinallywould keep the connection tidier if this is called repeatedly.Functionally this is fine; these are just robustness improvements.
445-496: Paper Pro partition-info helper matches the new scheme; minor duplicationUsing
_get_active_device+_parse_partition_infoand then probingboot_part/root_partvia the samefile_exists/read_fileabstraction as_read_version_from_pathis a nice cleanup and should fix the on-device vs SSH divergence.Two small follow-ups you might consider:
file_exists/read_fileare now duplicated between this method and_read_version_from_path; pulling them into a tiny shared helper (e.g.,_make_file_ops(ftp) -> (file_exists, read_file)) would DRY up the code and keep behavior consistent.ftp = self.client.open_sftp()isn’t closed; adding atry/finally: ftp.close()(or a context manager helper) would avoid leaking SFTP sessions if this is called more than a handful of times.Otherwise the 3.22+ vs legacy boot-part logic looks consistent with the stated downgrade behavior.
638-681: Paper Pro restore logic for 3.22 boundary looks reasonable; document assumptionsThe new Paper Pro branch in
restore_previous_version:
- Derives
current_part/inactive_partvia_get_paper_pro_partition_info.- Distinguishes
<3.22vs>=3.22for both current and backup.- Uses
root_partsysfs writes only for legacy, andmmc bootpart enablewhen either side is “new”.- Resets the appropriate
root{a,b}_errcntcounter.This matches the described goal of handling 3.20↔3.22+ transitions in a single script. Given how hardware-specific this is, I’d just suggest adding a short comment summarizing the expected partition layout (e.g., “2→A / 3→B; boot0/boot1 mapping”) and the rationale for
inactive_part - 1/inactive_part - 2so future maintainers don’t have to reverse-engineer the arithmetic.Behavior-wise, this looks fine and aligns with the test matrix in the PR description.
820-897: Bootloader update helper is sound; minor robustness tweaks possible
_update_paper_pro_bootloaderlooks careful and appropriate for something as sensitive as a bootloader update:
- Uses tmpfiles locally, uploads via SFTP, then runs
preinstandpostinstwith explicit exit-status checks.- Cleans up both remote and local temp artifacts in a
finallyblock.- Surfaces stderr content in
SystemErrormessages when the script fails.A couple of small, optional robustness ideas:
- If you ever expect significant output from the update script, consider draining
stdout/stderrbefore or while waiting onrecv_exit_status()to avoid any chance of channel-buffer deadlock.- In the
finally, you may want to wrapos.unlink(...)calls in a smalltry/except FileNotFoundErrorso a missing temp file doesn’t mask a more interesting earlier exception.Nothing blocking here; the overall structure looks good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codexctl/device.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.14.7)
codexctl/device.py
312-312: Consider moving this statement to an else block
(TRY300)
331-331: Avoid specifying long messages outside the exception class
(TRY003)
338-338: Avoid specifying long messages outside the exception class
(TRY003)
340-340: Avoid specifying long messages outside the exception class
(TRY003)
362-362: Avoid specifying long messages outside the exception class
(TRY003)
365-365: subprocess call: check for execution of untrusted input
(S603)
367-367: Avoid specifying long messages outside the exception class
(TRY003)
396-396: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
408-408: Abstract raise to an inner function
(TRY301)
408-408: Avoid specifying long messages outside the exception class
(TRY003)
418-418: subprocess call: check for execution of untrusted input
(S603)
419-419: Starting a process with a partial executable path
(S607)
423-423: Abstract raise to an inner function
(TRY301)
423-423: Avoid specifying long messages outside the exception class
(TRY003)
429-429: subprocess call: check for execution of untrusted input
(S603)
429-429: Starting a process with a partial executable path
(S607)
430-430: subprocess call: check for execution of untrusted input
(S603)
430-430: Starting a process with a partial executable path
(S607)
462-462: Consider moving this statement to an else block
(TRY300)
⏰ 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). (4)
- GitHub Check: Build for macos-latest
- GitHub Check: Build for windows-latest
- GitHub Check: Build for remarkable
- GitHub Check: Build for ubuntu-latest
🔇 Additional comments (1)
codexctl/device.py (1)
506-533: Device-status refactor to reuse_read_version_from_pathlooks goodSwitching both SSH and local code paths in
get_device_statusto use_read_version_from_pathis a good simplification and ensures version parsing stays consistent with the new helpers.The added
encoding="utf-8"on local file reads is also a nice touch for robustness.No issues from my side here.
2 participants
This PR fixes a couple bugs/regressions related to my recent PRs:
swupdate-from-image-fileto sourcing the args file and runningswupdatesinceswupdate-from-image-filecontains the "old A/B downgrade" check. This still enables a single run of swupdate as sourcing the args file correctly identifies the backup partition.I've conducted the following tests to ensure correct behavior:
rMPP
codexctl install 3.24.0.149codexctl install 3.20.0.92(cross-3.20 boundary)rM2 (SSH)
codexctl statuscodexctl download 3.22.4.2codexctl install ~/Downloads/remarkable-production-memfault-image-3.22.4.2-rm2-publiccodexctl install 3.24.0.149codexctl restorerM2 (on-device)
codexctl statuscodexctl download 3.22.4.2codexctl install ~/Downloads/remarkable-production-memfault-image-3.22.4.2-rm2-publiccodexctl install 3.24.0.149codexctl restoreSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.