feat: Support for 3.22 & 3.23, Paper Pro Move, Paper Pro restores #155
Conversation
WalkthroughAdds ReMarkable Paper Pro Move (RMPPM) support, multi-provider SWU download and metadata extraction, bootloader-aware upgrade/downgrade handling with bootloader extraction/upload for Paper Pro, consolidated update-image/fuse import handling, and status output now includes backup partition version. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as codexctl CLI
participant UpdateMgr as UpdateManager
participant Analysis as SWU Analysis
participant Provider as External Provider(s)
participant Device as DeviceManager
participant FTP as Device FTP
User->>CLI: install --target rmppm 3.23.0.64
CLI->>UpdateMgr: get_latest_version(target)
UpdateMgr-->>CLI: candidate_version_info
CLI->>UpdateMgr: download_version(candidate)
loop try providers
UpdateMgr->>Provider: HTTP GET SWU (provider URL)
Provider-->>UpdateMgr: SWU file or error
end
UpdateMgr-->>CLI: downloaded_swu_file
CLI->>Analysis: extract_swu_metadata(downloaded_swu_file)
Analysis-->>CLI: (version, hardware info)
CLI->>UpdateMgr: is_bootloader_boundary_downgrade(current_version, target_version)
UpdateMgr-->>CLI: boolean
alt Bootloader boundary crossed
CLI->>UpdateMgr: obtain current SWU for current_version (download or reuse)
UpdateMgr-->>CLI: current_swu_file
CLI->>Analysis: extract update-bootloader.sh & imx-boot from current_swu_file
Analysis-->>CLI: bootloader_files (script, imx-boot)
CLI->>Device: install_sw_update(target_swu_file, bootloader_files)
Device->>FTP: upload target SWU
FTP-->>Device: ack
Device->>FTP: upload bootloader script & imx-boot
FTP-->>Device: ack
Device->>Device: run preinst/postinst and apply bootloader update
else Normal update
CLI->>Device: install_sw_update(target_swu_file, bootloader_files=None)
Device->>FTP: upload and install SWU
end
Device-->>CLI: installation result
CLI-->>User: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (11)
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 (2)
codexctl/device.py (1)
571-609: Minor: remove unusedold_part_labelin Paper Pro restore logicIn the RMPP/RMPPM restore branch,
old_part_labelis computed but never used:old_part_label = "a" if current_part == 2 else "b"You can drop this assignment (or include it in the log output if you intended to display the old label) to avoid the unused‑variable warning.
codexctl/updates.py (1)
31-36: UpdateManager: new hardware, provider URLs, and boundary helper are mostly solid; consider a small guard
- Initialising
self.remarkableppm_versionsandself.external_provider_urlsfrom the newget_remarkable_versions()tuple lines up with the updateddata/version-ids.jsonstructure.- Normalising
"external-provider-urls"/"external-provider-url"to a list and then iterating overself.external_provider_urlsindownload_versiongives a clean multi‑provider fallback, and lowering non‑200 responses todebugavoids noisy logs when a primary mirror is down.- The new
HardwareType.RMPPMbranch in bothget_latest_versionanddownload_versioncorrectly picks upself.remarkableppm_versionsand uses the same code path as the other devices.is_bootloader_boundary_downgrade’s list comparison against[3, 22]behaves as intended for versions like3.23.0.64→3.20.0.92, while safely returningFalseon malformed inputs.One small robustness improvement: if neither
"external-provider-urls"nor"external-provider-url"is present (e.g. a locally edited or very oldversion-ids.json),provider_urlsends upNoneanddownload_versionwill later fail with aTypeErrorwhen iterating it. You could fail fast with a clearer error:- provider_urls = contents.get("external-provider-urls", contents.get("external-provider-url")) - if isinstance(provider_urls, str): - provider_urls = [provider_urls] + provider_urls = contents.get("external-provider-urls", contents.get("external-provider-url")) + if isinstance(provider_urls, str): + provider_urls = [provider_urls] + if provider_urls is None: + raise SystemError("version-ids.json is missing external provider URLs")That makes configuration problems easier to diagnose.
Also applies to: 38-43, 86-96, 124-142, 174-176, 204-220, 227-238, 243-258, 310-327, 395-419
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
codexctl/__init__.py(5 hunks)codexctl/cpio.py(1 hunks)codexctl/device.py(14 hunks)codexctl/updates.py(7 hunks)data/version-ids.json(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/device.py
🧬 Code graph analysis (2)
codexctl/__init__.py (4)
codexctl/device.py (5)
HardwareType(20-85)get_device_status(418-475)restore_previous_version(553-632)new_download_hw(52-61)install_sw_update(661-773)codexctl/cpio.py (1)
extract_cpio_files(54-150)codexctl/analysis.py (1)
get_update_image(9-33)codexctl/updates.py (2)
is_bootloader_boundary_downgrade(396-419)download_version(174-258)
codexctl/updates.py (1)
codexctl/device.py (2)
HardwareType(20-85)new_download_hw(52-61)
🪛 Ruff (0.14.5)
codexctl/cpio.py
23-23: Avoid specifying long messages outside the exception class
(TRY003)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/__init__.py
129-131: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
129-131: Avoid specifying long messages outside the exception class
(TRY003)
143-145: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
143-145: Avoid specifying long messages outside the exception class
(TRY003)
334-334: Avoid specifying long messages outside the exception class
(TRY003)
359-359: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/updates.py
417-417: Consider moving this statement to an else block
(TRY300)
codexctl/device.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
85-85: Avoid specifying long messages outside the exception class
(TRY003)
308-308: Do not catch blind exception: Exception
(BLE001)
345-345: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
359-359: Do not catch blind exception: Exception
(BLE001)
366-366: Consider moving this statement to an else block
(TRY300)
368-368: Do not catch blind exception: Exception
(BLE001)
382-382: Avoid specifying long messages outside the exception class
(TRY003)
392-392: Do not catch blind exception: Exception
(BLE001)
403-403: Do not catch blind exception: Exception
(BLE001)
412-412: Do not catch blind exception: Exception
(BLE001)
576-576: Local variable old_part_label is assigned to but never used
Remove assignment to unused variable old_part_label
(F841)
581-581: Do not catch blind exception: Exception
(BLE001)
587-587: Do not catch blind exception: Exception
(BLE001)
795-795: Avoid specifying long messages outside the exception class
(TRY003)
799-799: Probable insecure usage of temporary file or directory: "/tmp/update-bootloader.sh"
(S108)
800-800: Probable insecure usage of temporary file or directory: "/tmp/imx-boot"
(S108)
828-828: Avoid specifying long messages outside the exception class
(TRY003)
837-837: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
data/version-ids.json (1)
3-10: Version metadata structure matches UpdateManager expectationsThe new 3.22/3.23 entries,
remarkableppmsection, and the top-levellast-updatedandexternal-provider-urlskeys all line up with howUpdateManager.get_remarkable_versions()consumes this file; I don’t see structural issues here.Also applies to: 193-205, 395-406, 452-466
codexctl/__init__.py (2)
70-87: Paper Pro Move listing integration looks correctWiring
self.updater.remarkableppm_versionsinto thelistcommand and gating it onHardwareType.RMPPMis consistent with the other hardware types; no issues here.Based on learnings, this continues to keep
UpdateManagercalls usingHardwareTypeenums rather than raw strings.
241-244: Status/restore output now correctly includes backup versionUsing
backup = remarkable.get_device_status()[4]and printing “Backup version” instatus, plus echoing the backup version afterrestore, matches the new 5‑tuple signature ofget_device_statusand gives users clear feedback about the inactive partition.Also applies to: 247-250
codexctl/device.py (3)
20-38: RMPPM hardware type and mappings are coherentThe new
HardwareType.RMPPMvariant, itsparsealiases, and theold_download_hw/new_download_hw/swupdate_hw/toltec_typemappings are internally consistent and match howUpdateManagerand SWUpdate expect to address the “chiappa” (Paper Pro Move) hardware.Also applies to: 40-50, 52-62, 64-74, 76-86
289-319: Version/backup helpers integrate cleanly into device statusRefactoring version reading into
_read_version_from_path, introducing_get_backup_partition_versionto mount the inactive partition read‑only, and then returning(beta, old_update_engine, xochitl_version, version_id, backup_version)fromget_device_status()gives a clear, reusable status surface and matches how__init__.pynow rendersstatusandrestore. The fallback to"unknown"on failure is reasonable for a CLI tool.Also applies to: 320-371, 418-475
661-714: Bootloader-aware SWU install flow on Paper Pro looks soundAccepting optional
bootloader_filesininstall_sw_updateand, when present, calling_update_paper_pro_bootloader()after a successful SWUpdate run is a clean way to sequence bootloader updates for boundary downgrades._update_paper_pro_bootloaderitself correctly:
- Uploads the script and
imx-bootimage to/tmpon the device,- Runs both
preinstandpostinstphases with exit‑code checks, and- Cleans up both remote and local temporary files in a
finallyblock.This should integrate well with the new bootloader extraction path in
__init__.py.Also applies to: 775-848
Eeems
left a comment
Eeems
left a comment
There was a problem hiding this comment.
Could you add some automated tests for UpdateManager.is_bootloader_boundary_downgrade
I added unit tests for this. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
codexctl/device.py (1)
418-475: get_device_status remote changes are good; local branch still has a decode bugUsing
_read_version_from_pathfor the remote path and appendingbackup_versionfrom_get_backup_partition_version()is a clean refactor and enables the new status output.However, the local path still does:
with open("/usr/share/remarkable/update.conf") as file: xochitl_version = re.search( "(?<=REMARKABLE_RELEASE_VERSION=).*", file.read().decode("utf-8").strip("\n"), ).group()Here
file.read()already returns astr, so calling.decode("utf-8")will raise anAttributeError. This looks pre‑existing, but it meansget_device_status()will fail when running on‑device with a local update.conf.Minimal fix (without changing semantics) would be:
- with open("/usr/share/remarkable/update.conf") as file: - xochitl_version = re.search( - "(?<=REMARKABLE_RELEASE_VERSION=).*", - file.read().decode("utf-8").strip("\n"), - ).group() + with open("/usr/share/remarkable/update.conf", encoding="utf-8") as file: + xochitl_version = re.search( + "(?<=REMARKABLE_RELEASE_VERSION=).*", + file.read().strip("\n"), + ).group()(and similarly drop
.decode("utf-8")in the/etc/os-releasebranch).
🧹 Nitpick comments (11)
README.md (1)
60-64: Add languages to new fenced blocks for linting and readabilityThe new examples are helpful; to satisfy markdownlint (MD040) and get syntax highlighting, please annotate the new code fences with a language, e.g.:
-- (Paper Pro Move) version 3.23.0.64 to a folder named `out` -``` +– (Paper Pro Move) version 3.23.0.64 to a folder named `out` +```bash codexctl download 3.23.0.64 --hardware rmppm -o out codexctl install ./out/remarkable-production-image-3.23.0.64-chiappa-public.swu@@
-- Extract a 3.11+ SWU firmware file to see its contents
-+– Extract a 3.11+ SWU firmware file to see its contents +bash
codexctl extract remarkable-production-image-3.22.0.64-ferrari-public.swu -o extractedAlso applies to: 87-90 </blockquote></details> <details> <summary>codexctl/__init__.py (4)</summary><blockquote> `109-133`: **SWU vs .signed extraction routing is correct; improve dependency error handling** The CPIO magic detection now correctly recognises both `"070701"` and `"070702"` SWU archives and routes them to `extract_swu_files`, while legacy `.signed` images still go through `get_update_image` and are written as a flat image, which matches the intended split. To make the SWU path failure mode as user-friendly as the legacy one, consider wrapping the `extract_swu_files` import in a guarded block similar to the `.signed` path and chaining the original ImportError: ```diff - if magic in (b'070701', b'070702'): - logger.info("Detected CPIO format (3.11+ SWU file)") - from .analysis import extract_swu_files + if magic in (b'070701', b'070702'): + logger.info("Detected CPIO format (3.11+ SWU file)") + try: + from .analysis import extract_swu_files + except ImportError as err: + raise ImportError( + "remarkable_update_image is required for extracting SWU files. Please install it!" + ) from errThis mirrors the old-format path and avoids confusing tracebacks if
remarkable-update-image(which backs the SWU helpers) is missing.
134-140: Tighten ImportError re‑raise in mount pathThe consolidated import for
get_update_imageandUpdateFSis good, but you currently drop the original ImportError context and static analysis flags this.Minor tweak to preserve the cause and align with B904:
- try: - from .analysis import get_update_image - from remarkable_update_fuse import UpdateFS - except ImportError: - raise ImportError( - "remarkable_update_image and remarkable_update_fuse are required for mounting. Please install them!" - ) + try: + from .analysis import get_update_image + from remarkable_update_fuse import UpdateFS + except ImportError as err: + raise ImportError( + "remarkable_update_image and remarkable_update_fuse are required for mounting. Please install them!" + ) from err
235-245: Status/restore output correctly incorporates backup versionUsing the new 5‑tuple
get_device_status()return to print the backup version and then referencing index 4 when reporting the restored version is consistent and should be helpful to users.If you want to avoid a second SFTP round‑trip on
restore, you could capture the earlierbackupvalue and reuse it instead of re‑callingget_device_status(), but that’s purely an optimisation.
261-299: SWU metadata and version extraction flow is sound, with minor UX nitThe
update_filehandling correctly:
- Detects SWU files via CPIO magic.
- Extracts
(version_number, swu_hardware)viaget_swu_metadata.- Enforces a strict hardware match before proceeding.
- Falls back to filename parsing (and finally an interactive prompt) when metadata isn’t available.
One small UX improvement: if the user mistypes the version at the interactive prompt, the code raises
SystemError("Invalid version!")after a single attempt. Wrapping the prompt in a simple loop with a maximum number of retries would make recovery smoother without changing semantics.codexctl/device.py (4)
289-320: _read_version_from_path cleanly unifies version detection for current and backup partitionsThe helper correctly:
- Prefers
/usr/share/remarkable/update.confwhen available.- Falls back to
/etc/os-releaseand flipsold_update_enginetoFalsewhen needed.- Accepts an optional
base_pathprefix so it works for both live and mounted backup partitions.Given previous feedback about over‑broad exception handling, your restriction to
(IOError, OSError)here is reasonable. If you want to tighten it further later, you could explicitly check for the presence of each file instead of relying on the exception, but it’s not strictly necessary for this PR.
321-372: Backup partition version helper is robust but relies on caller handling “unknown” correctly
_get_backup_partition_versiondoes a good job of:
- Supporting both swupdate‑based (RMPP/RMPPM) and rootdev‑based (RM1/RM2) schemes.
- Mounting the inactive partition read‑only under
/tmp/mount_pX.- Best‑effort reading via
_read_version_from_path, and gracefully returning"unknown"when mounting or version parsing fails.The sentinel
"unknown"is a reasonable choice, but it requires callers (notablyrestore_previous_version) to treat it specially instead of assuming it’s a parseable semver; see comment on Lines 571‑609.
660-713: install_sw_update: bootloader hook integration is correct; consider guarding hardware typeExtending
install_sw_updateto accept optionalbootloader_filesand, when present, invoke_update_paper_pro_bootloaderafter a successfulswupdateis the right place to hook the Paper Pro downgrade bootloader flow into the existing install path.Two minor considerations:
The method is now semantically “Paper Pro aware” but doesn’t currently guard against being called with
bootloader_fileson non‑Paper‑Pro hardware. Today only the RMPP downgrade path passes this argument, but adding a defensive check would make misuse harder:if bootloader_files:
if bootloader_files:if self.hardware is not HardwareType.RMPP:raise SystemError("Bootloader updates via swupdate are only supported on Paper Pro devices")
- On non‑SSH (local) installs you ignore
bootloader_files, which is fine as long as the only caller that passes them is the remote RMPP downgrade flow (current state).Nothing blocking, but worth tightening if you expect other call sites to grow.
774-848: Paper Pro bootloader update helper is well‑structured; minor hardening possible
_update_paper_pro_bootloaderis nicely self‑contained:
- Uses SFTP to upload both
update-bootloader.shandimx-bootinto/tmp/on the device.- Runs
preinstandpostinstphases, checking exit codes and surfacing stderr content viaSystemError.- Cleans up both local temporary files and remote
/tmpartifacts in afinallyblock.A couple of minor hardening options (not mandatory for this PR):
- If you want to be extra defensive about partial cleanup failures, you could wrap the local
os.unlinkcalls in smalltry/except OSErrorblocks to avoid masking a more usefulSystemErrorfrom earlier in the method.- The fixed remote paths under
/tmpare probably acceptable on a single‑user device, but if you anticipate concurrent codexctl runs, incorporating the PID into the filename (e.g./tmp/imx-boot-<pid>) would reduce any chance of clobbering.Overall, this helper looks solid.
codexctl/updates.py (2)
30-36: Version metadata + external provider URL handling looks correct; consider enforcing non‑empty listThe updated
get_remarkable_versionsneatly:
- Extends the return shape to include
remarkableppmversions and a list ofexternal_provider_urls.- Supports both
"external-provider-urls"(list) and legacy"external-provider-url"(string) keys, normalising the latter to a one‑element list.- Fails early with a clear
SystemErrorwhen provider URLs are missing.One small robustness improvement: right now an empty list of URLs passes the
provider_urls is Nonecheck and will later causedownload_versionto log “Failed to download … from all sources” without hinting that the config is incomplete. You could treat an empty list as misconfiguration as well:- if provider_urls is None: + if not provider_urls: raise SystemError( f"version-ids.json at {file_location} is missing external provider URLs. " "Please delete the file and try again, or open an issue on the repo." )Also applies to: 38-43, 86-101
400-424: Bootloader boundary detection helper encodes the 3.22 rule correctly
is_bootloader_boundary_downgradecleanly captures “current ≥ 3.22 and target < 3.22” by:
- Parsing both version strings into integer lists.
- Comparing against
[3, 22]using list comparison.- Returning
Falseon any parsing error or odd format.Given the current reMarkable versioning scheme, this should behave as intended for 3.x releases and safely ignore malformed inputs.
If you later introduce 4.x versions, you might want to align this helper with whatever you decide for
uses_new_update_engine, but for 3.20–3.23 it’s fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/main.yml(1 hunks)Makefile(1 hunks)README.md(4 hunks)codexctl/__init__.py(7 hunks)codexctl/analysis.py(2 hunks)codexctl/device.py(14 hunks)codexctl/updates.py(7 hunks)requirements.txt(1 hunks)tests/test_unit.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/device.pycodexctl/updates.py
🧬 Code graph analysis (4)
codexctl/__init__.py (3)
codexctl/device.py (4)
HardwareType(20-85)get_device_status(418-475)new_download_hw(52-61)install_sw_update(660-772)codexctl/analysis.py (3)
extract_swu_files(83-138)get_update_image(13-37)get_swu_metadata(40-80)codexctl/updates.py (3)
UpdateManager(17-424)is_bootloader_boundary_downgrade(401-424)download_version(179-263)
codexctl/analysis.py (1)
codexctl/device.py (1)
HardwareType(20-85)
tests/test_unit.py (1)
codexctl/updates.py (1)
is_bootloader_boundary_downgrade(401-424)
codexctl/updates.py (1)
codexctl/device.py (2)
HardwareType(20-85)new_download_hw(52-61)
🪛 markdownlint-cli2 (0.18.1)
README.md
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.5)
codexctl/__init__.py
124-126: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
124-126: Avoid specifying long messages outside the exception class
(TRY003)
138-140: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
138-140: Avoid specifying long messages outside the exception class
(TRY003)
276-281: Avoid specifying long messages outside the exception class
(TRY003)
294-294: Avoid specifying long messages outside the exception class
(TRY003)
354-354: Avoid specifying long messages outside the exception class
(TRY003)
379-379: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/analysis.py
58-58: Avoid specifying long messages outside the exception class
(TRY003)
65-65: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/device.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
85-85: Avoid specifying long messages outside the exception class
(TRY003)
307-307: Avoid specifying long messages outside the exception class
(TRY003)
315-315: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
315-315: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
360-360: Do not catch blind exception: Exception
(BLE001)
367-367: Consider moving this statement to an else block
(TRY300)
369-369: Do not catch blind exception: Exception
(BLE001)
383-383: Avoid specifying long messages outside the exception class
(TRY003)
394-394: Avoid specifying long messages outside the exception class
(TRY003)
581-581: Avoid specifying long messages outside the exception class
(TRY003)
587-587: Avoid specifying long messages outside the exception class
(TRY003)
794-794: Avoid specifying long messages outside the exception class
(TRY003)
798-798: Probable insecure usage of temporary file or directory: "/tmp/update-bootloader.sh"
(S108)
799-799: Probable insecure usage of temporary file or directory: "/tmp/imx-boot"
(S108)
827-827: Avoid specifying long messages outside the exception class
(TRY003)
836-836: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/updates.py
90-93: Avoid specifying long messages outside the exception class
(TRY003)
422-422: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (11)
tests/test_unit.py (1)
1-65: Good focused coverage for bootloader boundary logicThe lightweight harness and these cases exercise the boundary semantics well (including malformed/empty versions). This should give quick signal if the 3.22‑boundary downgrade logic ever regresses.
Makefile (1)
52-57: Separating unit and integration tests inmake testis a good callRunning
tests/test_unit.pyfirst withset -egives a fast failure path before the more expensive integration flow intests/test.py. The added echoes make the phases clear in CI logs..github/workflows/main.yml (1)
36-52: CI workflow unit-tests job properly configuredThe dedicated
unit-testsjob withneeds: [unit-tests]on the remote matrix provides a clean fast-fail stage before the build matrix runs. The entry pointtests/test_unit.pyexists and is tracked. Keep unit tests constrained to pure unit coverage so they remain fast and don't drift into integration test territory over time.requirements.txt (1)
3-3: Consistency confirmed; change is safeThe verification shows
requirements.remote.txtincludes all packages fromrequirements.txtvia-r requirements.txt. This meansremarkable-update-image==1.2is automatically available in both environments without duplication. The unconditional pin change is consistent across all requirement files.codexctl/__init__.py (3)
70-87: Paper Pro Move listing wiring looks correctThe additional
remarkableppm_versionsblock and theHardwareType.RMPPMcheck integrate cleanly with the existing listing logic; behavior for other hardware remains unchanged.
424-425: Passing bootloader_files into install_sw_update only for new‑engine installs is correctConditioning
remarkable.install_sw_update(update_file, bootloader_files=bootloader_files_for_install)ondevice_version_uses_new_engineand keeping the old‐engine path oninstall_ohma_updatemaintains the previous behavior for RM1/RM2 while enabling the new Paper Pro downgrade flow.
627-631: Top‑level SystemError handling gives cleaner CLI failuresWrapping
man.call_funcin atry/except SystemErrorand printing a concise message to stderr before exiting with status 1 is a nice UX improvement. It avoids tracebacks for expected operational failures while still letting unexpected exceptions surface.codexctl/device.py (1)
20-37: RMPPM hardware integration into HardwareType and mappings looks consistentAdding
RMPPMto the enum, extendingparsewith chiappa/rmppm aliases, and wiring it throughold_download_hw,new_download_hw,swupdate_hw, andtoltec_typekeeps behavior parallel to the existing RM1/RM2/RMPP cases (including correctly disallowing old engine/toltec for Pro/Move).Also applies to: 39-86
codexctl/updates.py (3)
129-147: RMPPM integration into get_latest_version mirrors other hardware typesAdding the
HardwareType.RMPPMbranch and usingself.remarkableppm_versionsis consistent with the existing RM1/RM2/RMPP logic;__max_version(versions.keys())remains the common path.
179-263: RMPPM download support and multi‑provider fallback behavior look goodThe changes in
download_version:
- Add a
HardwareType.RMPPMcase wired toself.remarkableppm_versions, matching the new JSON layout.- Keep old
.signeddownloads (<={3,11,2,5}) intact, including use ofhardware_type.old_download_hw.- For newer versions, construct a consistent local
file_nameand iterate overself.external_provider_urls, attempting each provider in sequence and returning on the first successful__download_version_filecall, with clear debug logging on each attempt and a final error if all fail.This provides a straightforward way to add or swap providers via
version-ids.jsonwithout code changes, while preserving checksum validation and size sanity checks.
329-332: Non‑200 responses are now logged at debug level, aiding provider diagnosticsThe additional debug log for non‑200 HTTP responses in
__download_version_filewill help diagnose misconfigured or failing external providers without spamming normal users.
|
Instead of adding a brand new test job and brand new test code, could you extend the existing test code which is already integrated into the workflow. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
codexctl/device.py (1)
665-717: Bootloader update integration looks solid.The
install_sw_updateextension correctly:
- Accepts optional
bootloader_filesparameter- Applies bootloader update after successful swupdate (lines 711-717)
- Continues with the existing reboot and update-service disabling flow
Minor type annotation inconsistency: Line 665 uses
dict[str, bytes]while line 9 importsDictfromtyping. For consistency throughout the file, consider:- def install_sw_update(self, version_file: str, bootloader_files: dict[str, bytes] | None = None) -> None: + def install_sw_update(self, version_file: str, bootloader_files: Dict[str, bytes] | None = None) -> None:(Note:
dict[...]is valid in Python 3.9+, but usingDictmatches the existing import style.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codexctl/__init__.py(7 hunks)codexctl/analysis.py(2 hunks)codexctl/device.py(14 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/device.py
🧬 Code graph analysis (2)
codexctl/__init__.py (3)
codexctl/device.py (2)
HardwareType(20-85)new_download_hw(52-61)codexctl/analysis.py (3)
extract_swu_files(83-142)get_update_image(13-37)get_swu_metadata(40-80)codexctl/updates.py (3)
UpdateManager(17-424)is_bootloader_boundary_downgrade(401-424)download_version(179-263)
codexctl/analysis.py (1)
codexctl/device.py (1)
HardwareType(20-85)
🪛 Ruff (0.14.5)
codexctl/device.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
85-85: Avoid specifying long messages outside the exception class
(TRY003)
307-307: Avoid specifying long messages outside the exception class
(TRY003)
315-315: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
315-315: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
360-360: Do not catch blind exception: Exception
(BLE001)
367-367: Consider moving this statement to an else block
(TRY300)
369-369: Do not catch blind exception: Exception
(BLE001)
383-383: Avoid specifying long messages outside the exception class
(TRY003)
394-394: Avoid specifying long messages outside the exception class
(TRY003)
581-581: Avoid specifying long messages outside the exception class
(TRY003)
584-586: Avoid specifying long messages outside the exception class
(TRY003)
592-592: Avoid specifying long messages outside the exception class
(TRY003)
799-799: Avoid specifying long messages outside the exception class
(TRY003)
803-803: Probable insecure usage of temporary file or directory: "/tmp/update-bootloader.sh"
(S108)
804-804: Probable insecure usage of temporary file or directory: "/tmp/imx-boot"
(S108)
832-832: Avoid specifying long messages outside the exception class
(TRY003)
841-841: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/__init__.py
124-126: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
124-126: Avoid specifying long messages outside the exception class
(TRY003)
138-140: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
138-140: Avoid specifying long messages outside the exception class
(TRY003)
276-281: Avoid specifying long messages outside the exception class
(TRY003)
294-294: Avoid specifying long messages outside the exception class
(TRY003)
354-354: Avoid specifying long messages outside the exception class
(TRY003)
371-374: Avoid specifying long messages outside the exception class
(TRY003)
384-384: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/analysis.py
58-58: Avoid specifying long messages outside the exception class
(TRY003)
65-65: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (12)
codexctl/analysis.py (2)
40-80: LGTM! SWU metadata extraction is well-structured.The function correctly:
- Uses bytes keys for Archive access (
b"sw-description")- Validates the presence of required metadata
- Maps hardware identifiers to HardwareType enum values
- Ensures cleanup via finally block
83-142: Excellent work addressing the past review concerns!The function now correctly:
- Type safety: Line 136 encodes string filenames to bytes before calling
archive.get(), fixing the key type mismatch- Path traversal protection: Lines 117-118 resolve the file path and verify it stays within
output_pathusingis_relative_to()- Flexible extraction modes (to disk or in-memory dict)
- Proper cleanup via finally block
codexctl/__init__.py (4)
73-83: LGTM! RMPPM support integrated consistently.The Paper Pro Move version listing follows the same pattern as existing hardware variants.
109-133: Good fix for CPIO magic detection!Line 113 now checks for both
b'070701'andb'070702', correctly routing all valid CPIO-based SWU files to the new extraction path. This addresses the past review concern about misclassifying"070701"archives.
261-298: Robust SWU metadata extraction with appropriate fallbacks.The logic correctly:
- Detects SWU format via magic bytes (both variants)
- Extracts version and hardware from SWU metadata
- Validates hardware compatibility before installation (lines 275-281)
- Falls back to filename parsing if metadata extraction fails
329-389: Excellent! Past critical issue resolved.The bootloader boundary downgrade flow now correctly handles download failures. Lines 370-374 raise
SystemErrorif the current version SWU cannot be downloaded, preventing the unsafe scenario where the OS downgrade would proceed without the required bootloader update.Additional safeguards:
- Lines 383-384: Validates that exactly 2 bootloader files were extracted
- Lines 386-388: Logs file sizes for verification
- Clear user warnings and confirmation prompt (lines 337-354)
codexctl/device.py (6)
24-85: LGTM! RMPPM hardware type integrated consistently.The new Paper Pro Move variant is correctly:
- Added to the HardwareType enum (line 24)
- Parsed from multiple aliases: "ppm", "rmppm", "chiappa", "remarkable chiappa" (lines 28-29)
- Mapped across all properties:
old_download_hw,new_download_hw,swupdate_hw,toltec_type- Included in error messages (line 37)
289-319: Well-structured helper for version detection.The
_read_version_from_pathfunction provides a clean abstraction for reading versions from either the current partition or a mounted backup, with appropriate fallback fromupdate.conftoos-release.
321-371: Robust backup partition version detection.The function gracefully handles failures by returning
"unknown"rather than raising exceptions, which is appropriate for a helper that may encounter unreadable partitions. The exception handling at lines 360 and 369 is intentional for robustness.Note: The S108 static analysis warning about
/tmp/mount_p{inactive_part}is a false positive—this is a temporary mount point, not a file with sensitive data.
418-475: Clean extension of device status with backup version.The function now returns a 5-tuple including the backup partition version, utilizing the new helper methods for cleaner code organization.
571-613: Excellent! Past review concerns fully addressed.The restore flow now correctly:
- Raises
SystemErrorfor all failure modes (lines 581, 584-586, 592) instead ofValueError, ensuring consistent error handling- Explicitly checks for
"unknown"backup version (lines 583-586) with a clear error message- Handles both legacy sysfs (
root_part) and new MMC bootpart methods based on version boundaries- Applies both methods when transitioning across the 3.22 boundary
779-852: Well-structured bootloader update implementation.The
_update_paper_pro_bootloadermethod correctly:
- Uses local temp files for staging (lines 806-812)
- Uploads bootloader script and image to device (lines 816-819)
- Runs both preinst and postinst phases (lines 826-841)
- Validates exit status after each phase
- Ensures cleanup in finally block (lines 845-852)
Note: The S108 static analysis warnings about
/tmppaths (lines 803-804) are false positives—these are paths on the remote device, not the local system.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
codexctl/device.py (1)
289-319: Refactor exception handling to avoid blind fallback.The current implementation catches any
IOError/OSErrorwhen readingupdate.confand blindly triesos-release. This conflates "file doesn't exist" with "file is corrupted" or other I/O errors.Based on learnings, Eeems previously requested checking for files explicitly rather than using try/catch for flow control.
Additionally, the
ValueErrorexceptions raised on lines 307 and 315 won't be caught by the top-levelSystemErrorhandler in__init__.py, potentially showing raw tracebacks to users.Consider this refactor:
def _read_version_from_path(self, ftp, base_path: str = "") -> tuple[str, bool]: """Reads version from a given path (current partition or mounted backup) Args: ftp: SFTP client connection base_path: Base path prefix (empty for current partition, /tmp/mount_pX for backup) Returns: tuple: (version_string, old_update_engine_boolean) """ - old_update_engine = True - - try: - update_conf_path = f"{base_path}/usr/share/remarkable/update.conf" if base_path else "/usr/share/remarkable/update.conf" - with ftp.file(update_conf_path) as file: - contents = file.read().decode("utf-8").strip("\n") - match = re.search("(?<=REMARKABLE_RELEASE_VERSION=).*", contents) - if not match: - raise ValueError(f"REMARKABLE_RELEASE_VERSION not found in {update_conf_path}") - version = match.group() - except (IOError, OSError): - os_release_path = f"{base_path}/etc/os-release" if base_path else "/etc/os-release" - with ftp.file(os_release_path) as file: - contents = file.read().decode("utf-8") - match = re.search("(?<=IMG_VERSION=).*", contents) - if not match: - raise ValueError(f"IMG_VERSION not found in {os_release_path}") - version = match.group().strip('"') - old_update_engine = False - - return version, old_update_engine + update_conf_path = f"{base_path}/usr/share/remarkable/update.conf" if base_path else "/usr/share/remarkable/update.conf" + os_release_path = f"{base_path}/etc/os-release" if base_path else "/etc/os-release" + + # Try old update engine format first + try: + ftp.stat(update_conf_path) + with ftp.file(update_conf_path) as file: + contents = file.read().decode("utf-8").strip("\n") + match = re.search("(?<=REMARKABLE_RELEASE_VERSION=).*", contents) + if not match: + raise SystemError(f"REMARKABLE_RELEASE_VERSION not found in {update_conf_path}") + return match.group(), True + except (IOError, OSError): + pass # File doesn't exist, try new format + + # Try new update engine format + try: + with ftp.file(os_release_path) as file: + contents = file.read().decode("utf-8") + match = re.search("(?<=IMG_VERSION=).*", contents) + if not match: + raise SystemError(f"IMG_VERSION not found in {os_release_path}") + return match.group().strip('"'), False + except (IOError, OSError) as e: + raise SystemError(f"Cannot read version from {base_path or 'current partition'}: {e}") from eThis checks file existence explicitly with
stat()and usesSystemErrorfor consistency with the error handler.
🧹 Nitpick comments (1)
codexctl/device.py (1)
321-371: Consider narrowing exception handling.The method catches broad
Exceptionon lines 360 and 369, which can mask unexpected errors (static analysis warns about BLE001). While returning"unknown"is safe for the current caller, more specific exception types would improve debuggability.Consider catching only the exceptions you expect:
try: version, _ = self._read_version_from_path(ftp, mount_point) - except Exception as e: + except (IOError, OSError, SystemError) as e: self.logger.debug(f"Failed to read version from backup partition: {e}") version = "unknown"except Exception as e: self.logger.debug(f"Error getting backup partition version: {e}") return "unknown" + except (IOError, OSError, SystemError, paramiko.SSHException) as e: + self.logger.debug(f"Error getting backup partition version: {e}") + return "unknown"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile(1 hunks)codexctl/device.py(15 hunks)tests/test.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Makefile
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/device.py
🪛 Ruff (0.14.5)
codexctl/device.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
85-85: Avoid specifying long messages outside the exception class
(TRY003)
307-307: Avoid specifying long messages outside the exception class
(TRY003)
315-315: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
315-315: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
360-360: Do not catch blind exception: Exception
(BLE001)
367-367: Consider moving this statement to an else block
(TRY300)
369-369: Do not catch blind exception: Exception
(BLE001)
383-383: Avoid specifying long messages outside the exception class
(TRY003)
394-394: Avoid specifying long messages outside the exception class
(TRY003)
581-581: Avoid specifying long messages outside the exception class
(TRY003)
584-586: Avoid specifying long messages outside the exception class
(TRY003)
592-592: Avoid specifying long messages outside the exception class
(TRY003)
799-799: Avoid specifying long messages outside the exception class
(TRY003)
803-803: Probable insecure usage of temporary file or directory: "/tmp/update-bootloader.sh"
(S108)
804-804: Probable insecure usage of temporary file or directory: "/tmp/imx-boot"
(S108)
832-832: Avoid specifying long messages outside the exception class
(TRY003)
841-841: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
tests/test.py (2)
201-202: LGTM! Version updates align with PR objectives.The updated expected versions (3.23.0.64) correctly reflect the new firmware support added in this PR.
218-257: Tests correctly validate bootloader boundary logic.The method
UpdateManager.is_bootloader_boundary_downgrade(codexctl/updates.py:401) exists with the expected signature. All 8 test cases correctly reflect the documented behavior: returnsTrueonly whencurrent_version >= 3.22ANDtarget_version < 3.22. Coverage includes boundary crossing downgrades, non-boundary transitions, upgrades, and edge cases (empty/non-numeric versions).codexctl/device.py (5)
24-85: LGTM: RMPPM hardware variant integration is consistent.The RMPPM (Paper Pro Move) enum value and property implementations follow the established patterns for RM1, RM2, and RMPP. The mappings are correct: "chiappa" for both
new_download_hwandswupdate_hw, and appropriateValueErrorfor unsupportedold_download_hwandtoltec_type.
373-416: LGTM: Partition detection logic is sound.The version-aware partition detection correctly handles both the legacy sysfs (
root_part) and new MMC bootpart schemes. Line 394 now raisesSystemErrorinstead ofValueError(past review concern resolved), ensuring clean error handling at the top level.
418-475: LGTM: Clean refactor with new backup version support.The method now delegates to
_read_version_from_pathand returns the backup partition version as a fifth return value. The refactoring maintains existing behavior while adding the new functionality.
571-613: LGTM: Robust partition switching for Paper Pro devices.The restore logic correctly handles the 3.20/3.22 partition scheme transition by conditionally applying both legacy sysfs and new MMC bootpart methods based on current and target versions. The explicit check for
"unknown"backup version on lines 583-586 addresses the past review concern, ensuring clear error messages rather than raw tracebacks.
665-717: LGTM: Bootloader update integration is clean.The optional
bootloader_filesparameter is correctly typed and conditionally applied after successful swupdate. The flow ensures bootloader updates only occur when needed (e.g., for Paper Pro downgrades to <3.20).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
codexctl/device.py (3)
9-9: Consider type hint consistency.The imports
OptionalandDictfrom typing aren't used in the visible changes (lines 667, 673 usedict[str, bytes] | Noneinstead). If they're not used elsewhere in the file, they could be removed. Alternatively, for consistency with older Python type hint styles, you could useOptional[Dict[str, bytes]]instead ofdict[str, bytes] | None.
323-373: Good defensive programming for backup partition detection.The method correctly handles various failure modes by returning "unknown" rather than crashing, which is then explicitly checked by callers. The cleanup logic properly unmounts and removes the temporary mount point.
Minor style suggestion: line 369's
return versioncould be moved to anelseblock for clarity per TRY300, though the current implementation is functionally correct.
803-807: Improve exception handling specificity and chaining.Line 806 catches bare
Exceptionwhich can hide unexpected errors, and line 807 raises without chaining the original exception, losing debug context.Apply this diff to catch specific exceptions and preserve the traceback:
- ftp_client = None - try: - ftp_client = self.client.open_sftp() - except Exception: - raise SystemError("Failed to open SFTP connection for bootloader update") + ftp_client = None + try: + ftp_client = self.client.open_sftp() + except (paramiko.SSHException, OSError) as e: + raise SystemError("Failed to open SFTP connection for bootloader update") from eThis catches likely SFTP failures specifically and chains the exception for better debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codexctl/device.py(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/device.py
🪛 Ruff (0.14.5)
codexctl/device.py
37-37: Avoid specifying long messages outside the exception class
(TRY003)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
85-85: Avoid specifying long messages outside the exception class
(TRY003)
308-308: Avoid specifying long messages outside the exception class
(TRY003)
318-318: Avoid specifying long messages outside the exception class
(TRY003)
321-321: Avoid specifying long messages outside the exception class
(TRY003)
348-348: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
369-369: Consider moving this statement to an else block
(TRY300)
385-385: Avoid specifying long messages outside the exception class
(TRY003)
396-396: Avoid specifying long messages outside the exception class
(TRY003)
583-583: Avoid specifying long messages outside the exception class
(TRY003)
586-588: Avoid specifying long messages outside the exception class
(TRY003)
594-594: Avoid specifying long messages outside the exception class
(TRY003)
801-801: Avoid specifying long messages outside the exception class
(TRY003)
806-806: Do not catch blind exception: Exception
(BLE001)
807-807: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
807-807: Avoid specifying long messages outside the exception class
(TRY003)
809-809: Probable insecure usage of temporary file or directory: "/tmp/update-bootloader.sh"
(S108)
810-810: Probable insecure usage of temporary file or directory: "/tmp/imx-boot"
(S108)
838-838: Avoid specifying long messages outside the exception class
(TRY003)
847-847: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (7)
codexctl/device.py (7)
24-85: LGTM: Comprehensive RMPPM hardware type integration.The Paper Pro Move (RMPPM) support is consistently integrated across all
HardwareTypeproperties with appropriate identifiers ("chiappa") and error messages. The parse method correctly handles multiple name variants.
289-322: Good refactoring: Version reading logic extracted.The
_read_version_from_pathhelper cleanly handles both old (update.conf) and new (os-release) version formats with appropriate fallback logic. The error handling correctly raisesSystemErrorfor unrecoverable failures, ensuring consistent error handling across the codebase.
375-418: LGTM: Correct handling of Paper Pro partition scheme transition.The method properly handles the 3.20/3.22 partition scheme change by attempting the new MMC boot_part method first (for >= 3.22), then falling back to the legacy root_part sysfs interface. The version parsing and error handling are appropriate.
420-477: LGTM: Backup version added to device status.The method now returns backup partition version as a fifth value, utilizing the new
_read_version_from_pathand_get_backup_partition_versionhelpers. Note this is a breaking API change (5 return values instead of 4), but appears intentional per the PR objectives.
573-616: LGTM: Robust restore logic for Paper Pro partition scheme transition.The restore logic correctly handles the 3.20/3.22 partition scheme change by:
- Validating backup version is readable (lines 585-588)
- Detecting version boundaries for both current and target versions
- Conditionally applying legacy sysfs method (< 3.22) and/or new MMC method (>= 3.22)
All version parsing errors appropriately raise
SystemErrorfor consistent error handling (confirmed from past review feedback).
667-719: LGTM: Clean integration of bootloader update pathway.The optional
bootloader_filesparameter allows seamless bootloader updates for Paper Pro downgrades without breaking existing callers. The update sequence (swupdate → bootloader → reboot) is correct.
781-859: Bootloader update implementation looks solid.The method correctly:
- Uses secure temporary files locally (tempfile.NamedTemporaryFile)
- Uploads script and image to the device
- Runs both preinst and postinst phases with proper error checking
- Cleans up both local and remote files in the finally block
- Guards the ftp_client.close() call to prevent NameError (past review concern addressed)
Co-authored-by: Nathaniel van Diepen <Eeems@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
codexctl/analysis.py (1)
9-9: Replace deprecated typing imports with modern syntax.The imports
Tuple,Optional, andDictfrom typing are deprecated. Line 40 already uses the moderntuple[...]syntax, but line 87 still usesOptional[Dict[str, bytes]]. Based on learnings, the maintainer has requested usingtuple,dict, and| Noneinstead.Apply this diff to use modern type hints consistently:
-from typing import Tuple, Optional, Dict +from typing import TYPE_CHECKINGThen update line 87:
-) -> Optional[Dict[str, bytes]]: +) -> dict[str, bytes] | None:Based on learnings, the maintainer has previously requested this change.
🧹 Nitpick comments (1)
codexctl/analysis.py (1)
40-80: Implementation looks correct; consider extracting exception messages as optional refactor.The function correctly:
- Uses bytes keys for archive access (
b"sw-description")- Ensures archive cleanup in the finally block
- Validates presence of version and hardware metadata
- Maps hardware identifiers to HardwareType enum values
The static analysis hints flag long exception messages at lines 58, 65, and 76. While these are functional, you could optionally extract them into constants or custom exception classes for consistency with TRY003.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(4 hunks)codexctl/analysis.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/analysis.py
🧬 Code graph analysis (1)
codexctl/analysis.py (1)
codexctl/device.py (1)
HardwareType(20-85)
🪛 markdownlint-cli2 (0.18.1)
README.md
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.6)
codexctl/analysis.py
58-58: Avoid specifying long messages outside the exception class
(TRY003)
65-65: Avoid specifying long messages outside the exception class
(TRY003)
76-76: Avoid specifying long messages outside the exception class
(TRY003)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
README.md (2)
10-12: Good addition documenting the new bootloader update workflow.The new section clearly explains the automatic bootloader handling for Paper Pro downgrades, which aligns well with the PR's downgrade support feature.
77-81: Good enhancement showing the complete restore workflow.The updated example now demonstrates the full status → restore sequence, making it clearer how to use the restore functionality on devices that support it (Paper Pro/Move and other variants).
codexctl/analysis.py (2)
110-124: Path traversal protection correctly implemented.The code now properly prevents path traversal attacks:
- Line 107 resolves the output directory to an absolute path
- Line 115 resolves each entry's target path including symlinks
- Line 117 validates the resolved path stays within the output directory
- Line 118 raises ValueError for any traversal attempts
This addresses the previous security concern flagged in past reviews.
127-140: Type consistency fix correctly implemented.The code now properly handles the bytes/str key mismatch:
- Line 136 encodes caller-provided string filenames to bytes (
filename.encode('utf-8')) before callingarchive.get()- This ensures lookups succeed since archive keys are bytes (as confirmed by the
b"TRAILER!!!"checks)This addresses the previous correctness issue where filtered extraction would silently fail to find entries.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
README.md (2)
60-64: Add bash language specification to the code block.The fenced code block is missing a language identifier, which violates markdown linting rules. Specify
bashfor proper syntax highlighting.- Downloading rmppm version 3.23.0.64 to a folder named `out` and then installing it -``` +Downloading rmppm version 3.23.0.64 to a folder named `out` and then installing it +```bash codexctl download 3.23.0.64 --hardware rmppm -o out codexctl install ./out/remarkable-production-image-3.23.0.64-chiappa-public.swu -``` +```
87-90: Add bash language specification to the code block.The fenced code block is missing a language identifier, which violates markdown linting rules. Specify
bashfor proper syntax highlighting.- Extract the contents of an upgrade file to a folder named `extracted` -``` +Extract the contents of an upgrade file to a folder named `extracted` +```bash codexctl extract remarkable-production-image-3.22.0.64-ferrari-public.swu -o extracted -``` +```
🧹 Nitpick comments (3)
codexctl/__init__.py (1)
124-130: Chain the exception for better debugging.The
raise ... from Nonepattern should be used when intentionally discarding the original exception context. However, for debugging purposes, consider preserving the original exception.except ImportError: - raise ImportError( + raise ImportError( "remarkable_update_image and remarkable_update_fuse are required for mounting. Please install them!" - ) + ) from NoneThis silences the static analysis warning while making the intent explicit.
codexctl/device.py (2)
345-356: Consider extracting duplicate partition detection logic.The partition detection code (getting active device, calculating inactive partition) is duplicated between the RMPP/RMPPM branch (lines 346-350) and the RM1/RM2 branch (lines 352-356). The logic is identical except for the command used (
swupdate -gvsrootdev).This is a minor duplication that could be consolidated in the future, but it's acceptable for now given the different command sources.
810-814: Good fix for potential NameError, but exception chaining is missing.The
ftp_client = Noneinitialization before the try block addresses the past review concern. However, the bareexcept Exceptionshould chain the exception for debugging.ftp_client = None try: ftp_client = self.client.open_sftp() - except Exception: - raise SystemError("Failed to open SFTP connection for bootloader update") + except Exception as e: + raise SystemError("Failed to open SFTP connection for bootloader update") from eThis preserves the original exception context for debugging while still presenting a clean error message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(4 hunks)codexctl/__init__.py(7 hunks)codexctl/analysis.py(2 hunks)codexctl/device.py(14 hunks)codexctl/updates.py(7 hunks)tests/test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/device.pycodexctl/analysis.py
🧬 Code graph analysis (4)
codexctl/updates.py (2)
codexctl/device.py (2)
HardwareType(19-89)new_download_hw(56-65)codexctl/__init__.py (1)
version_lookup(248-249)
tests/test.py (1)
codexctl/updates.py (2)
UpdateManager(17-435)is_bootloader_boundary_downgrade(401-435)
codexctl/__init__.py (3)
codexctl/device.py (4)
HardwareType(19-89)get_device_status(422-479)new_download_hw(56-65)install_sw_update(674-786)codexctl/analysis.py (1)
get_swu_metadata(37-62)codexctl/updates.py (1)
is_bootloader_boundary_downgrade(401-435)
codexctl/analysis.py (1)
codexctl/device.py (1)
HardwareType(19-89)
🪛 markdownlint-cli2 (0.18.1)
README.md
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.7)
codexctl/updates.py
90-93: Avoid specifying long messages outside the exception class
(TRY003)
419-419: Avoid specifying long messages outside the exception class
(TRY003)
421-421: Avoid specifying long messages outside the exception class
(TRY003)
427-427: Avoid specifying long messages outside the exception class
(TRY003)
430-430: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/__init__.py
112-114: Avoid specifying long messages outside the exception class
(TRY003)
128-130: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
128-130: Avoid specifying long messages outside the exception class
(TRY003)
266-271: Avoid specifying long messages outside the exception class
(TRY003)
284-284: Avoid specifying long messages outside the exception class
(TRY003)
344-344: Avoid specifying long messages outside the exception class
(TRY003)
361-364: Avoid specifying long messages outside the exception class
(TRY003)
375-375: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/device.py
41-41: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
309-309: Consider moving this statement to an else block
(TRY300)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
327-327: Avoid specifying long messages outside the exception class
(TRY003)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
341-341: Avoid specifying long messages outside the exception class
(TRY003)
358-358: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
368-368: Avoid specifying long messages outside the exception class
(TRY003)
387-387: Avoid specifying long messages outside the exception class
(TRY003)
398-398: Avoid specifying long messages outside the exception class
(TRY003)
585-585: Avoid specifying long messages outside the exception class
(TRY003)
588-590: Avoid specifying long messages outside the exception class
(TRY003)
596-596: Avoid specifying long messages outside the exception class
(TRY003)
808-808: Avoid specifying long messages outside the exception class
(TRY003)
813-813: Do not catch blind exception: Exception
(BLE001)
814-814: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
814-814: Avoid specifying long messages outside the exception class
(TRY003)
816-816: Probable insecure usage of temporary file or directory: "/tmp/update-bootloader.sh"
(S108)
817-817: Probable insecure usage of temporary file or directory: "/tmp/imx-boot"
(S108)
845-845: Avoid specifying long messages outside the exception class
(TRY003)
854-854: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/analysis.py
60-60: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (12)
README.md (2)
10-12: Clear and informative addition to documentation.The new "Paper Pro bootloader updates" section effectively explains the automatic bootloader handling during downgrades across the 3.20/3.22 boundary, aligning with the PR objectives for bootloader-aware upgrade flows.
77-81: Example workflow appropriately updated.The restore workflow example is clear and consistent with existing documentation patterns.
codexctl/analysis.py (1)
37-62: LGTM! Clean metadata extraction implementation.The function correctly extracts version and hardware type from SWU files using the
remarkable_update_imagelibrary. The hardware mapping aligns with theHardwareType.swupdate_hwproperty values defined incodexctl/device.py.tests/test.py (1)
213-249: Good test coverage for RMPPM and bootloader boundary downgrade logic.The tests comprehensively cover:
- RMPPM toltec error path (matching RMPP behavior)
- Boundary-crossing downgrades (3.22+ → <3.22)
- Non-boundary cases and upgrade direction
- Input validation (empty string, non-numeric versions)
This addresses the prior review feedback about adding unit tests for
is_bootloader_boundary_downgrade.codexctl/__init__.py (2)
623-627: Good addition of top-level SystemError handler.This catches
SystemErrorexceptions and provides a clean user-facing error message instead of a raw traceback, improving the CLI experience.
321-322: Bootloader boundary check should include RMPPM (Paper Pro Move).The bootloader boundary downgrade logic only checks for
HardwareType.RMPP, but based on the PR description, RMPPM devices also use the new update engine and may require similar bootloader handling for 3.22 boundary crossings.Consider extending the condition to include both Paper Pro variants:
- if (device_version_uses_new_engine and - remarkable.hardware == HardwareType.RMPP): + if (device_version_uses_new_engine and + remarkable.hardware in (HardwareType.RMPP, HardwareType.RMPPM)):If RMPPM devices do not require this handling, clarify with a comment explaining why only RMPP is checked.
codexctl/updates.py (3)
86-100: Good backward-compatible handling of provider URLs.The code gracefully handles both string (legacy) and list (new) formats for external provider URLs, with appropriate validation. This ensures smooth migration for users with older
version-ids.jsonfiles.
248-263: Multi-provider download fallback is well-implemented.The loop iterates through available providers, logging failures and attempting the next source. This improves reliability when a single provider is unavailable.
400-435: Robust bootloader boundary detection with proper validation.The implementation correctly:
- Validates empty/whitespace inputs
- Handles non-numeric version components with clear error messages
- Requires at least 2 version components
- Uses list comparison for semantic version ordering
One edge case to note:
[3, 22]comparison works correctly because Python compares lists element-by-element.codexctl/device.py (3)
293-329: Well-structured version reading helper.The
_read_version_from_pathmethod cleanly abstracts version detection from eitherupdate.conf(old engine) oros-release(new engine), with proper error messages when neither source is available.
858-866: Cleanup in finally block looks correct.The finally block properly:
- Removes remote temporary files
- Removes local temporary files
- Guards
ftp_client.close()with a None check
477-479: Verify_get_backup_partition_versionbehavior without SSH client.
get_device_statuscalls_get_backup_partition_version()unconditionally at line 477, but this method may raiseSystemErrorifself.clientisNone(line 341). When running locally (non-remote),self.clientisNone, which could cause the status command to fail.Consider handling the local (non-remote) case in
_get_backup_partition_version, or guard the call:- backup_version = self._get_backup_partition_version() + if self.client: + backup_version = self._get_backup_partition_version() + else: + # Local partition mounting would require different implementation + backup_version = "unknown"
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
codexctl/device.py (1)
783-861: Solid bootloader update implementation with proper cleanup.The method correctly handles the two-phase bootloader update (preinst/postinst), includes comprehensive cleanup in the finally block, and properly guards against the NameError that was flagged in previous reviews.
One minor refinement: the exception handling on line 808 catches all exceptions blindly. Consider being more specific:
- except Exception: - raise SystemError("Failed to open SFTP connection for bootloader update") + except (IOError, OSError, paramiko.SSHException) as e: + raise SystemError("Failed to open SFTP connection for bootloader update") from eThis improves exception chaining and makes debugging easier while being more precise about what failures are expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codexctl/device.py(14 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/device.py
🧬 Code graph analysis (1)
codexctl/device.py (1)
tests/test.py (1)
write(33-34)
🪛 Ruff (0.14.7)
codexctl/device.py
41-41: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
309-309: Consider moving this statement to an else block
(TRY300)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
327-327: Avoid specifying long messages outside the exception class
(TRY003)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
341-341: Avoid specifying long messages outside the exception class
(TRY003)
358-358: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
368-368: Avoid specifying long messages outside the exception class
(TRY003)
387-387: Avoid specifying long messages outside the exception class
(TRY003)
398-398: Avoid specifying long messages outside the exception class
(TRY003)
585-585: Avoid specifying long messages outside the exception class
(TRY003)
591-591: Avoid specifying long messages outside the exception class
(TRY003)
803-803: Avoid specifying long messages outside the exception class
(TRY003)
808-808: Do not catch blind exception: Exception
(BLE001)
809-809: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
809-809: Avoid specifying long messages outside the exception class
(TRY003)
811-811: Probable insecure usage of temporary file or directory: "/tmp/update-bootloader.sh"
(S108)
812-812: Probable insecure usage of temporary file or directory: "/tmp/imx-boot"
(S108)
840-840: Avoid specifying long messages outside the exception class
(TRY003)
849-849: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (9)
codexctl/device.py (9)
23-23: LGTM!Clean addition of the Paper Pro Move hardware variant to the enum.
27-41: LGTM!The refactor to use a match statement is cleaner and more idiomatic. The RMPPM case mapping and updated error message are well-structured.
51-53: LGTM!RMPPM is correctly integrated into all hardware-specific properties with appropriate values and error messages for unsupported features.
Also applies to: 64-65, 76-77, 87-89
293-329: LGTM!Well-structured method that cleanly handles version detection from both old and new partition layouts. The nested
file_existshelper and fallback logic are solid.
331-375: LGTM!Robust implementation for reading the backup partition version. The logic correctly handles different device types, mounts read-only, and ensures cleanup in the finally block. Error handling with
SystemErroris consistent with the codebase.
377-420: LGTM!Solid implementation that correctly detects the partition scheme based on version and handles both new (MMC bootpart) and legacy (sysfs) partition detection methods with appropriate fallbacks.
422-479: LGTM!The signature change to include backup partition version is well-integrated. The method correctly uses the new helper methods for remote execution and maintains equivalent logic for local execution.
575-617: LGTM!Excellent implementation of the partition-switching logic for Paper Pro devices. The code correctly handles all transition scenarios between old (<3.22) and new (>=3.22) partition schemes by applying the appropriate combination of legacy sysfs and new MMC bootpart methods.
669-721: LGTM!Clean integration of the bootloader update pathway. The optional parameter uses modern type hints, and the bootloader update is correctly sequenced after the main software update succeeds.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(4 hunks)codexctl/__init__.py(7 hunks)codexctl/analysis.py(2 hunks)pyproject.toml(1 hunks)requirements.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/__init__.pycodexctl/analysis.py
🧬 Code graph analysis (2)
codexctl/__init__.py (3)
codexctl/device.py (4)
HardwareType(19-89)get_device_status(422-479)new_download_hw(56-65)install_sw_update(669-781)codexctl/analysis.py (2)
get_update_image(10-34)get_swu_metadata(37-65)codexctl/updates.py (3)
UpdateManager(17-435)is_bootloader_boundary_downgrade(401-435)download_version(179-263)
codexctl/analysis.py (1)
codexctl/device.py (1)
HardwareType(19-89)
🪛 checkmake (0.2.2)
Makefile
[warning] 63-63: Target body for "test" exceeds allowed length of 5 (37).
(maxbodylength)
🪛 Ruff (0.14.7)
codexctl/__init__.py
112-114: Avoid specifying long messages outside the exception class
(TRY003)
128-130: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
128-130: Avoid specifying long messages outside the exception class
(TRY003)
266-271: Avoid specifying long messages outside the exception class
(TRY003)
284-284: Avoid specifying long messages outside the exception class
(TRY003)
344-344: Avoid specifying long messages outside the exception class
(TRY003)
361-364: Avoid specifying long messages outside the exception class
(TRY003)
375-375: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/analysis.py
60-60: Avoid specifying long messages outside the exception class
(TRY003)
63-63: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (10)
pyproject.toml (1)
15-16: Verify dependency versions support the new SWU extraction and bootloader features.The dependency version bumps from 1.1.6→1.3 and 1.2.4→1.3 are aligned with the PR scope (SWU extraction, bootloader-aware updates), but the version spec (
1.3) is permissive (allows any 1.3.x). For production device firmware management, consider whether stricter pinning (e.g.,^1.3.0) is needed to ensure deterministic behavior.Confirm that:
- Tests validate integration with these library versions
- The new capabilities (SWU metadata extraction, bootloader handling) are exercised
- No breaking changes or unexpected behavior from the library updates
Makefile (2)
7-62: LGTM! Well-structured SWU test infrastructure.The SWU test variables and download target follow the same proven pattern as the existing firmware test setup, providing consistent checksum validation and version tracking.
88-128: LGTM! Comprehensive SWU test coverage.The test flows properly validate extraction, filesystem listing, and file reading for SWU files, with appropriate error handling and output validation.
codexctl/analysis.py (1)
37-65: LGTM! Clean SWU metadata extraction using remarkable-update-image.The function correctly uses the remarkable-update-image library API to extract version and hardware type. The hardware mapping is consistent with the
HardwareType.swupdate_hwproperty, and the validation ensures both hardware support and version presence.codexctl/__init__.py (6)
73-83: LGTM! Consistent RMPPM support in list command.The Paper Pro Move version listing follows the established pattern for other hardware variants, correctly integrating with the UpdateManager's
remarkableppm_versions.
109-130: LGTM! Simplified extraction with proper error handling.The extraction flow now cleanly delegates format detection to the
UpdateImagelibrary, while mount properly requires both necessary dependencies. The error messages clearly communicate missing dependencies.
226-234: LGTM! Backup version reporting integrated correctly.The status and restore commands properly handle the expanded device status tuple, displaying the backup partition version as intended.
251-289: LGTM! Robust version extraction with hardware validation.The SWU metadata extraction provides early hardware compatibility checking, preventing firmware mismatch installations. The graceful fallback to regex extraction and user prompts ensures compatibility with non-SWU files.
623-627: LGTM! Clean error handling for user-facing failures.The SystemError catch wrapper provides a better user experience by converting internal SystemError exceptions into clean error messages on stderr with proper exit codes.
319-379: Verify remarkable-update-image version constraint to prevent runtime failures on bootloader extraction.The bootloader downgrade implementation correctly handles the critical logic flow (boundary detection, user confirmation, error handling on download failure). However, lines 370-371 depend on the
archiveattribute from unreleased remarkable-update-image PR #16. If deployed with an older library version, this will fail at runtime with anAttributeError.The code snippet shows no error handling around the archive access and no visible version constraint in dependencies. Before merging, confirm:
- remarkable-update-image version is pinned to a release that includes PR #16, OR
- a try-except block wraps lines 370-371 with a clear error message indicating the required library version
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/main.yml (1)
118-132: Remove unnecessarywc -lpipe from disk cleanup step.The
| wc -lat the end counts the number of verbose output lines but doesn't contribute to the cleanup or provide actionable information. Remove it to reduce log noise.Apply this diff:
- name: Free up disk space shell: bash run: | sudo rm -vrf \ /usr/lib/jvm \ /usr/share/dotnet \ /usr/share/swift \ /usr/local/.ghcup \ /usr/local/julia* \ /usr/local/lib/android \ /usr/local/share/chromium \ /opt/microsoft /opt/google \ /opt/az \ - /usr/local/share/powershell \ - | wc -l + /usr/local/share/powershell
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/main.yml(2 hunks)
🔇 Additional comments (1)
.github/workflows/main.yml (1)
118-132: Disk cleanup placement and directory selection look sensible.The step is well-positioned before the Docker build (which requires significant disk space for Nuitka ARM compilation), and the chosen directories (JVM, dotnet, Swift, Julia, Android, Chromium, cloud provider tooling) are standard CI environment bloat. This should free adequate space for the build pipeline.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
codexctl/updates.py (2)
86-101: LGTM! Robust parsing with backward compatibility.The provider URL parsing correctly handles both legacy single-string and new list formats, with appropriate validation to catch missing configuration early.
Note: Static analysis flags the error message on lines 90-93 as potentially verbose (TRY003), but it's helpful for troubleshooting configuration issues.
400-435: LGTM! Thorough validation and correct boundary detection logic.The bootloader boundary detection correctly identifies downgrades crossing 3.22 (from ≥3.22 to <3.22) using lexicographic list comparison. Input validation is comprehensive with clear error messages for debugging.
Note: Static analysis flags error messages on lines 419-421, 427, and 430 as potentially verbose (TRY003). These are helpful for diagnosing version format issues, so deferring changes is reasonable given the "Chill" review mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codexctl/updates.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
codexctl/updates.py (1)
codexctl/device.py (2)
HardwareType(19-89)new_download_hw(56-65)
🪛 Ruff (0.14.7)
codexctl/updates.py
90-93: Avoid specifying long messages outside the exception class
(TRY003)
419-419: Avoid specifying long messages outside the exception class
(TRY003)
421-421: Avoid specifying long messages outside the exception class
(TRY003)
427-427: Avoid specifying long messages outside the exception class
(TRY003)
430-430: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
codexctl/updates.py (5)
30-36: LGTM! New attributes initialized correctly.The addition of
remarkableppm_versionsandexternal_provider_urlsattributes aligns with the expanded functionality for Paper Pro Move support and multi-provider downloads.
145-146: LGTM! RMPPM support added correctly.The Paper Pro Move hardware type is handled consistently with other device types.
216-217: LGTM! RMPPM download support implemented.Correctly maps
HardwareType.RMPPMtoremarkableppm_versionsfor version lookups.
248-263: LGTM! Well-implemented multi-provider fallback.The download logic correctly iterates through provider URLs with appropriate fallback behavior. Returning on first success and logging at debug level for intermediate failures provides good observability without noise.
331-331: LGTM! Appropriate logging level for multi-provider context.Changing from error to debug level makes sense when multiple providers are attempted—intermediate failures are expected, and only final failure (logged at line 262) represents an actual error condition.
Eeems
left a comment
Eeems
left a comment
There was a problem hiding this comment.
I had some time to go through this from top to bottom again and found some more things and nits.
Co-authored-by: Nathaniel van Diepen <Eeems@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
README.md (2)
61-64: Add bash language identifier to code block.The code block is missing a language specifier, which violates markdown linting rules (MD040).
Apply this diff:
- Downloading rmppm version 3.23.0.64 to a folder named `out` and then installing it -``` +```bash codexctl download 3.23.0.64 --hardware rmppm -o out codexctl install ./out/remarkable-production-image-3.23.0.64-chiappa-public.swu--- `88-90`: **Add bash language identifier to code block.** The code block is missing a language specifier, which violates markdown linting rules (MD040). Apply this diff: ```diff - Extract the filesystem image of an upgrade file as a file named `extracted` -``` +```bash codexctl extract remarkable-production-image-3.22.0.64-ferrari-public.swu -o extracted</blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (2)</summary><blockquote> <details> <summary>codexctl/__init__.py (1)</summary><blockquote> `124-130`: **Consider mentioning both required libraries in the mount error message.** The mount path requires both `remarkable_update_image` (for `get_update_image`) and `remarkable_update_fuse` (for `UpdateFS`), but the error message only mentions the latter. Apply this diff to make the error more informative: ```diff try: from .analysis import get_update_image from remarkable_update_fuse import UpdateFS except ImportError: raise ImportError( - "remarkable_update_fuse is required for mounting. Please install it!" + "remarkable_update_image and remarkable_update_fuse are required for mounting. Please install them!" )codexctl/device.py (1)
798-798: Consider moving tempfile import to module level.The
tempfileimport at line 798 is inside the method. Moving it to the top of the file with other imports improves consistency and follows Python conventions.Based on learnings, imports should generally be at module level unless they're optional dependencies that need feature detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(4 hunks)codexctl/__init__.py(7 hunks)codexctl/analysis.py(1 hunks)codexctl/device.py(14 hunks)codexctl/updates.py(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/analysis.pycodexctl/__init__.pycodexctl/device.py
🧬 Code graph analysis (3)
codexctl/analysis.py (1)
codexctl/device.py (1)
HardwareType(19-89)
codexctl/__init__.py (2)
codexctl/device.py (4)
HardwareType(19-89)get_device_status(422-479)new_download_hw(56-65)install_sw_update(669-781)codexctl/updates.py (1)
is_bootloader_boundary_downgrade(402-436)
codexctl/updates.py (2)
codexctl/device.py (2)
HardwareType(19-89)new_download_hw(56-65)codexctl/__init__.py (1)
version_lookup(248-249)
🪛 markdownlint-cli2 (0.18.1)
README.md
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
88-88: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.7)
codexctl/__init__.py
112-114: Avoid specifying long messages outside the exception class
(TRY003)
128-130: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
128-130: Avoid specifying long messages outside the exception class
(TRY003)
262-262: Avoid specifying long messages outside the exception class
(TRY003)
272-272: Avoid specifying long messages outside the exception class
(TRY003)
278-283: Avoid specifying long messages outside the exception class
(TRY003)
296-296: Avoid specifying long messages outside the exception class
(TRY003)
356-356: Avoid specifying long messages outside the exception class
(TRY003)
373-376: Avoid specifying long messages outside the exception class
(TRY003)
387-387: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/updates.py
88-91: Avoid specifying long messages outside the exception class
(TRY003)
420-420: Avoid specifying long messages outside the exception class
(TRY003)
422-422: Avoid specifying long messages outside the exception class
(TRY003)
428-428: Avoid specifying long messages outside the exception class
(TRY003)
431-431: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/device.py
41-41: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
87-87: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
309-309: Consider moving this statement to an else block
(TRY300)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
327-327: Avoid specifying long messages outside the exception class
(TRY003)
329-329: Avoid specifying long messages outside the exception class
(TRY003)
341-341: Avoid specifying long messages outside the exception class
(TRY003)
358-358: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
368-368: Avoid specifying long messages outside the exception class
(TRY003)
387-387: Avoid specifying long messages outside the exception class
(TRY003)
398-398: Avoid specifying long messages outside the exception class
(TRY003)
583-583: Avoid specifying long messages outside the exception class
(TRY003)
589-589: Avoid specifying long messages outside the exception class
(TRY003)
803-803: Avoid specifying long messages outside the exception class
(TRY003)
808-808: Do not catch blind exception: Exception
(BLE001)
809-809: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
809-809: Avoid specifying long messages outside the exception class
(TRY003)
811-811: Probable insecure usage of temporary file or directory: "/tmp/update-bootloader.sh"
(S108)
812-812: Probable insecure usage of temporary file or directory: "/tmp/imx-boot"
(S108)
840-840: Avoid specifying long messages outside the exception class
(TRY003)
849-849: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (19)
codexctl/analysis.py (1)
7-7: LGTM! Import aligns with RMPPM support.The HardwareType import is correctly added and necessary for the broader hardware variant support introduced in this PR.
codexctl/__init__.py (5)
73-83: LGTM! RMPPM version list support is well-integrated.The remarkableppm_versions handling follows the established pattern for other hardware types and integrates cleanly into the list output.
226-234: LGTM! Backup version reporting is correctly integrated.The status and restore flows properly use the expanded 5-tuple from
get_device_statusand display the backup partition version clearly.
331-392: LGTM! Bootloader-aware downgrade handling is well-implemented.The boundary detection, user confirmation, SWU download with fallback, and bootloader file extraction are all correctly implemented. The use of
UpdateImage.archive[b'...']is valid with remarkable-update-image v1.3, which is specified in the project dependencies.
433-433: LGTM! Bootloader files correctly passed to install flow.The
bootloader_filesparameter is properly passed toinstall_sw_update, matching the updated signature indevice.py.
635-639: LGTM! Top-level error handling improves user experience.The
SystemErrorhandler provides clean, user-friendly error messages instead of raw stack traces, which is good practice for a CLI tool.codexctl/updates.py (5)
30-36: LGTM! Initialization correctly handles RMPPM and multi-provider URLs.The unpacking of
remarkableppm_versionsandexternal_provider_urls(as a list) correctly matches the updated return signature ofget_remarkable_versions.
86-101: LGTM! Provider URL validation is defensive and well-structured.The logic correctly handles both old (
external-provider-url) and new (external-provider-urls) formats, validates presence before use, and normalizes to a list. The safe.get()calls with empty dict defaults are good defensive programming.
146-147: LGTM! RMPPM hardware type handling is consistent.The RMPPM cases in both
get_latest_versionanddownload_versioncorrectly selectremarkableppm_versionsand follow the established pattern for other hardware types.Also applies to: 217-218
241-264: LGTM! Multi-provider download logic enhances resilience.The iteration through multiple provider URLs with appropriate debug logging provides good fallback behavior. Downgrading the non-200 log to debug level (line 332) is the right choice for multi-source downloads to avoid excessive error noise.
Also applies to: 332-332
401-436: LGTM! Bootloader boundary detection is robust and well-tested.The
is_bootloader_boundary_downgrademethod has thorough input validation, clear error messages, correct comparison logic for the 3.22 boundary, and proper error chaining. The test coverage intests/test.pyvalidates the boundary cases.codexctl/device.py (8)
23-41: LGTM! RMPPM hardware type and match-based parsing are well-implemented.The new RMPPM enum value and the refactored
matchstatement provide clear, idiomatic parsing with comprehensive alias support and a helpful error message.
51-53: LGTM! RMPPM property mappings are consistent and correct.All hardware property mappings for RMPPM follow the established pattern, use correct identifiers (chiappa for swupdate), and provide clear error messages for unsupported features.
Also applies to: 64-65, 76-77, 87-89
293-329: LGTM! Version reading abstraction is well-designed.The
_read_version_from_pathhelper cleanly handles both old (update.conf) and new (os-release) version file formats with appropriate fallback logic and error handling.
331-375: LGTM! Backup partition version detection is robust.The partition detection correctly handles both Paper Pro (using
swupdate -g) and older devices (usingrootdev), mounts read-only for safety, and guarantees cleanup via the finally block. The/tmpusage flagged by static analysis is standard practice for temporary mounts in Linux environments.
377-420: LGTM! Partition detection handles 3.20/3.22 boundary correctly.The
_get_paper_pro_partition_infomethod correctly handles the partition scheme changes between firmware versions, with defensive fallbacks and appropriate error handling for both old and new boot mechanisms.
422-479: LGTM! Status reporting correctly includes backup version.The updated
get_device_statusproperly uses the new version-reading helper and returns the 5-tuple including backup partition version, matching the signature expected by callers in__init__.py.
575-617: LGTM! Restore logic handles partition scheme transitions correctly.The enhanced
restore_previous_versionfor RMPP/RMPPM correctly handles all transition cases across the 3.22 bootloader boundary by conditionally applying both legacy (sysfs) and new (MMC bootpart) methods as needed.
669-721: LGTM! Bootloader update workflow is well-implemented.The bootloader update flow correctly uploads both the script and image, executes preinst and postinst stages, validates exit statuses, and ensures cleanup in the finally block. The error handling is appropriate for a critical firmware update operation.
Also applies to: 783-861
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
codexctl/device.py (1)
784-860: Bootloader update implementation is solid, with one exception handling refinement.The method correctly:
- Initializes
ftp_client = Nonebefore the try block (addressing past review concern)- Uploads bootloader script and image via SFTP
- Executes both preinst and postinst phases with exit status validation
- Cleans up temporary files on both device and local system in the finally block
- Guards
ftp_client.close()to avoid NameError (addressing past review concern)The logic and error handling are robust overall.
Consider making the exception handling at line 807 more specific to avoid catching unexpected errors:
- except Exception: - raise SystemError("Failed to open SFTP connection for bootloader update") + except (paramiko.SSHException, OSError) as e: + raise SystemError("Failed to open SFTP connection for bootloader update") from eThis narrows the catch to expected connection/SFTP errors and preserves the exception chain for debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codexctl/__init__.py(7 hunks)codexctl/device.py(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T16:37:10.452Z
Learnt from: Eeems
Repo: Jayy001/codexctl PR: 140
File: codexctl/__init__.py:54-55
Timestamp: 2025-09-10T16:37:10.452Z
Learning: In codexctl/__init__.py, the UpdateManager methods (get_latest_version, get_toltec_version, download_version) all expect HardwareType enum parameters, not string parameters.
Applied to files:
codexctl/device.pycodexctl/__init__.py
🪛 Ruff (0.14.7)
codexctl/device.py
42-42: Avoid specifying long messages outside the exception class
(TRY003)
52-52: Avoid specifying long messages outside the exception class
(TRY003)
54-54: Avoid specifying long messages outside the exception class
(TRY003)
88-88: Avoid specifying long messages outside the exception class
(TRY003)
90-90: Avoid specifying long messages outside the exception class
(TRY003)
310-310: Consider moving this statement to an else block
(TRY300)
320-320: Avoid specifying long messages outside the exception class
(TRY003)
328-328: Avoid specifying long messages outside the exception class
(TRY003)
330-330: Avoid specifying long messages outside the exception class
(TRY003)
342-342: Avoid specifying long messages outside the exception class
(TRY003)
359-359: Probable insecure usage of temporary file or directory: "/tmp/mount_p"
(S108)
369-369: Avoid specifying long messages outside the exception class
(TRY003)
388-388: Avoid specifying long messages outside the exception class
(TRY003)
399-399: Avoid specifying long messages outside the exception class
(TRY003)
584-584: Avoid specifying long messages outside the exception class
(TRY003)
590-590: Avoid specifying long messages outside the exception class
(TRY003)
802-802: Avoid specifying long messages outside the exception class
(TRY003)
807-807: Do not catch blind exception: Exception
(BLE001)
808-808: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
808-808: Avoid specifying long messages outside the exception class
(TRY003)
810-810: Probable insecure usage of temporary file or directory: "/tmp/update-bootloader.sh"
(S108)
811-811: Probable insecure usage of temporary file or directory: "/tmp/imx-boot"
(S108)
839-839: Avoid specifying long messages outside the exception class
(TRY003)
848-848: Avoid specifying long messages outside the exception class
(TRY003)
codexctl/__init__.py
112-114: Avoid specifying long messages outside the exception class
(TRY003)
128-130: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
128-130: Avoid specifying long messages outside the exception class
(TRY003)
213-215: Avoid specifying long messages outside the exception class
(TRY003)
269-269: Avoid specifying long messages outside the exception class
(TRY003)
279-279: Avoid specifying long messages outside the exception class
(TRY003)
285-290: Avoid specifying long messages outside the exception class
(TRY003)
303-303: Avoid specifying long messages outside the exception class
(TRY003)
363-363: Avoid specifying long messages outside the exception class
(TRY003)
380-383: Avoid specifying long messages outside the exception class
(TRY003)
394-394: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (12)
codexctl/__init__.py (6)
73-89: LGTM: Paper Pro Move support in list command.The RMPPM version listing correctly follows the existing pattern for other hardware types and integrates cleanly with the conditional output blocks.
109-130: LGTM: Extraction and mount flows now use remarkable-update-image library.The extraction path correctly uses
UpdateImageandread()for direct extraction, and the mount path consolidates the import error handling for both required libraries. This aligns with the maintainer's feedback from previous reviews.
210-215: LGTM: Install path correctly imports UpdateImage.The import is appropriately placed and will support the SWU metadata extraction logic later in the install flow.
233-241: LGTM: Status and restore now include backup partition version.The changes correctly unpack the extended return value from
get_device_statusand display the backup version in both status output and restore confirmation.
258-307: LGTM: SWU metadata extraction with hardware validation.The metadata extraction correctly uses
remarkable-update-imageto read version and hardware type from CPIO-based SWU files, validates hardware compatibility, and falls back to manual version input when metadata is unavailable. The hardware mismatch check prevents accidental installation of incompatible firmware.Minor note: Line 263 re-imports
UpdateImage, which was already imported at line 210. This is harmless but could be consolidated if you prefer.
338-398: LGTM: Bootloader boundary downgrade handling is comprehensive and safe.The implementation correctly:
- Detects 3.22+ → <3.22 downgrades for Paper Pro devices
- Prompts users with clear warnings before proceeding
- Reuses existing SWU files or downloads them automatically
- Raises a hard error if the current version SWU cannot be obtained (addressing past review concern)
- Extracts bootloader files using the
remarkable-update-image1.3 API- Validates that both required files were successfully extracted
The user confirmation prompt and validation checks make this a safe implementation of a complex and risky operation.
codexctl/device.py (6)
1-24: LGTM: Required imports and RMPPM hardware type added.The new imports support the version reading helpers and bootloader update functionality, and the RMPPM enum value correctly extends the hardware type enumeration.
28-90: LGTM: RMPPM parsing and properties correctly implemented.The hardware type parsing correctly handles RMPPM device identifiers, and all property methods appropriately return the corresponding values or raise clear errors for unsupported features (old update engine, toltec).
294-421: LGTM: Version reading and partition info helpers are well-structured.The new helper methods correctly:
- Read version information from both current and backup partitions
- Handle both old (update.conf) and new (os-release) version file formats
- Mount/unmount backup partitions safely with proper cleanup in finally blocks
- Detect Paper Pro partition schemes based on version numbers
- Raise clear SystemError exceptions for failure cases (addressing past review concerns)
The
/tmpusage flagged by static analysis is acceptable here since these are controlled, temporary mount points on reMarkable devices with proper cleanup.
423-480: LGTM: Status method correctly returns backup partition version.The refactored
get_device_statusnow uses the new helper methods and correctly returns the backup partition version as the fifth tuple element, integrating cleanly with the calling code in__init__.py.
558-618: LGTM: Paper Pro restore correctly handles partition scheme transitions.The RMPP/RMPPM-specific restore logic properly:
- Validates both current and backup version formats with early exit pattern (addressing past review feedback)
- Constructs appropriate restore commands based on version boundaries
- Uses legacy sysfs method for versions < 3.22
- Uses MMC bootpart method when either current or target is >= 3.22
- Handles all transition cases (old→old, old→new, new→old, new→new)
The conditional logic ensures the device can boot successfully regardless of which partition scheme change is involved.
670-722: LGTM: Bootloader files parameter integrated into install flow.The updated signature correctly accepts optional bootloader files and triggers the bootloader update after successful software installation. The conditional execution ensures the bootloader update only runs when explicitly provided.
Eeems
changed the title
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
2 participants
This PR adds comprehensive support for Paper Pro / Paper Pro Move devices and software versions 3.22-3.23.
Key Changes:
I have tested:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.