Skip to content
This repository was archived by the owner on Jan 23, 2026. It is now read-only.

Revert "ridesx: support http and insecure https"#802

Merged
NickCao merged 1 commit intomainfrom
revert-783-ridesx-insecure
Jan 16, 2026
Merged

Revert "ridesx: support http and insecure https"#802
NickCao merged 1 commit intomainfrom
revert-783-ridesx-insecure

Conversation

@mangelajo
Copy link
Member

@mangelajo mangelajo commented Jan 16, 2026

Reverts jumpstarter-dev/jumpstarter#783

Users are hitting:

│ │ │                                                                                          │ │ │
│ │ │   114 │   │   for partition, file_path in partitions.items():                            │ │ │
│ │ │   115 │   │   │   self.logger.info(f"Processing {partition} image: {file_path}")         │ │ │
│ │ │   116 │   │   │   operator = operators.get(partition)                                    │ │ │
│ │ │ ❱ 117 │   │   │   remote_files[partition] = self._upload_file_if_needed(file_path, opera │ │ │
│ │ │   118 │   │                                                                              │ │ │
│ │ │   119 │   │   self.logger.info("Checking for fastboot devices on Exporter...")           │ │ │
│ │ │   120 │   │   detection_result = self.call("detect_fastboot_device", 5, 2.0)             │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /Users/ajo/.local/jumpstarter/venv/lib/python3.14/site-packages/jumpstarter_driver_rides │ │ │
│ │ │ x/client.py:71 in _upload_file_if_needed                                                 │ │ │
│ │ │                                                                                          │ │ │
│ │ │    68 │   │   │   if is_insecure_http or insecure_tls:                                   │ │ │
│ │ │    69 │   │   │   │   filename = Path(parsed.path).name                                  │ │ │
│ │ │    70 │   │   │   │   self.logger.info(f"Downloading {file_path} to storage as {filename │ │ │
│ │ │ ❱  71 │   │   │   │   self._download_http_to_storage(file_path, self.storage, filename,  │ │ │
│ │ │    72 │   │   │   │   return filename                                                    │ │ │
│ │ │    73 │   │                                                                              │ │ │
│ │ │    74 │   │   # use opendal for local files, https:// (secure), and other schemes        │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /Users/ajo/.local/jumpstarter/venv/lib/python3.14/site-packages/jumpstarter_driver_rides │ │ │
│ │ │ x/client.py:51 in _download_http_to_storage                                              │ │ │
│ │ │                                                                                          │ │ │
│ │ │    48 │   │   │   │   │   │   │   f.write(chunk)                                         │ │ │
│ │ │    49 │   │   │   │   │   │   return Path(f.name)                                        │ │ │
│ │ │    50 │   │                                                                              │ │ │
│ │ │ ❱  51 │   │   tmp_path = self.portal.call(_download)                                     │ │ │
│ │ │    52 │   │   try:                                                                       │ │ │
│ │ │    53 │   │   │   storage.write_from_path(filename, tmp_path)                            │ │ │
│ │ │    54 │   │   finally:                                                                   │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /Users/ajo/.local/jumpstarter/venv/lib/python3.14/site-packages/anyio/from_thread.py:334 │ │ │
│ │ │ in call                                                                                  │ │ │
│ │ │                                                                                          │ │ │
│ │ │   331 │   │   │   from within the event loop thread                                      │ │ │
│ │ │   332 │   │                                                                              │ │ │
│ │ │   333 │   │   """                                                                        │ │ │
│ │ │ ❱ 334 │   │   return cast(T_Retval, self.start_task_soon(func, *args).result())          │ │ │
│ │ │   335 │                                                                                  │ │ │
│ │ │   336 │   @overload                                                                      │ │ │
│ │ │   337 │   def start_task_soon(                                                           │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /opt/homebrew/Cellar/python@3.14/3.14.2/Frameworks/Python.framework/Versions/3.14/lib/py │ │ │
│ │ │ thon3.14/concurrent/futures/_base.py:450 in result                                       │ │ │
│ │ │                                                                                          │ │ │
│ │ │   447 │   │   │   │   if self._state in [CANCELLED, CANCELLED_AND_NOTIFIED]:             │ │ │
│ │ │   448 │   │   │   │   │   raise CancelledError()                                         │ │ │
│ │ │   449 │   │   │   │   elif self._state == FINISHED:                                      │ │ │
│ │ │ ❱ 450 │   │   │   │   │   return self.__get_result()                                     │ │ │
│ │ │   451 │   │   │   │   else:                                                              │ │ │
│ │ │   452 │   │   │   │   │   raise TimeoutError()                                           │ │ │
│ │ │   453 │   │   finally:                                                                   │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /opt/homebrew/Cellar/python@3.14/3.14.2/Frameworks/Python.framework/Versions/3.14/lib/py │ │ │
│ │ │ thon3.14/concurrent/futures/_base.py:395 in __get_result                                 │ │ │
│ │ │                                                                                          │ │ │
│ │ │   392 │   def __get_result(self):                                                        │ │ │
│ │ │   393 │   │   if self._exception is not None:                                            │ │ │
│ │ │   394 │   │   │   try:                                                                   │ │ │
│ │ │ ❱ 395 │   │   │   │   raise self._exception                                              │ │ │
│ │ │   396 │   │   │   finally:                                                               │ │ │
│ │ │   397 │   │   │   │   # Break a reference cycle with the exception in self._exception    │ │ │
│ │ │   398 │   │   │   │   self = None                                                        │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /Users/ajo/.local/jumpstarter/venv/lib/python3.14/site-packages/anyio/from_thread.py:259 │ │ │
│ │ │ in _call_func                                                                            │ │ │
│ │ │                                                                                          │ │ │
│ │ │   256 │   │   │   if isawaitable(retval_or_awaitable):                                   │ │ │
│ │ │   257 │   │   │   │   with CancelScope() as scope:                                       │ │ │
│ │ │   258 │   │   │   │   │   future.add_done_callback(callback)                             │ │ │
│ │ │ ❱ 259 │   │   │   │   │   retval = await retval_or_awaitable                             │ │ │
│ │ │   260 │   │   │   else:                                                                  │ │ │
│ │ │   261 │   │   │   │   retval = retval_or_awaitable                                       │ │ │
│ │ │   262 │   │   except get_cancelled_exc_class():                                          │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /Users/ajo/.local/jumpstarter/venv/lib/python3.14/site-packages/jumpstarter_driver_rides │ │ │
│ │ │ x/client.py:47 in _download                                                              │ │ │
│ │ │                                                                                          │ │ │
│ │ │    44 │   │   │   │   async with session.get(url) as response:                           │ │ │
│ │ │    45 │   │   │   │   │   response.raise_for_status()                                    │ │ │
│ │ │    46 │   │   │   │   │   with tempfile.NamedTemporaryFile(delete=False, dir="/var/tmp") │ │ │
│ │ │ ❱  47 │   │   │   │   │   │   async for chunk in response.content.iter_chunked(65536):   │ │ │
│ │ │    48 │   │   │   │   │   │   │   f.write(chunk)                                         │ │ │
│ │ │    49 │   │   │   │   │   │   return Path(f.name)                                        │ │ │
│ │ │    50                                                                                    │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /Users/ajo/.local/jumpstarter/venv/lib/python3.14/site-packages/aiohttp/streams.py:52 in │ │ │
│ │ │ __anext__                                                                                │ │ │
│ │ │                                                                                          │ │ │
│ │ │    49 │                                                                                  │ │ │
│ │ │    50 │   async def __anext__(self) -> _T:                                               │ │ │
│ │ │    51 │   │   try:                                                                       │ │ │
│ │ │ ❱  52 │   │   │   rv = await self.read_func()                                            │ │ │
│ │ │    53 │   │   except EofStream:                                                          │ │ │
│ │ │    54 │   │   │   raise StopAsyncIteration                                               │ │ │
│ │ │    55 │   │   if rv == b"":                                                              │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /Users/ajo/.local/jumpstarter/venv/lib/python3.14/site-packages/aiohttp/streams.py:452   │ │ │
│ │ │ in read                                                                                  │ │ │
│ │ │                                                                                          │ │ │
│ │ │   449 │   │   # because waiter maybe triggered on chunk end,                             │ │ │
│ │ │   450 │   │   # without feeding any data                                                 │ │ │
│ │ │   451 │   │   while not self._buffer and not self._eof:                                  │ │ │
│ │ │ ❱ 452 │   │   │   await self._wait("read")                                               │ │ │
│ │ │   453 │   │                                                                              │ │ │
│ │ │   454 │   │   return self._read_nowait(n)                                                │ │ │
│ │ │   455                                                                                    │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /Users/ajo/.local/jumpstarter/venv/lib/python3.14/site-packages/aiohttp/streams.py:370   │ │ │
│ │ │ in _wait                                                                                 │ │ │
│ │ │                                                                                          │ │ │
│ │ │   367 │   │                                                                              │ │ │
│ │ │   368 │   │   waiter = self._waiter = self._loop.create_future()                         │ │ │
│ │ │   369 │   │   try:                                                                       │ │ │
│ │ │ ❱ 370 │   │   │   with self._timer:                                                      │ │ │
│ │ │   371 │   │   │   │   await waiter                                                       │ │ │
│ │ │   372 │   │   finally:                                                                   │ │ │
│ │ │   373 │   │   │   self._waiter = None                                                    │ │ │
│ │ │                                                                                          │ │ │
│ │ │ /Users/ajo/.local/jumpstarter/venv/lib/python3.14/site-packages/aiohttp/helpers.py:713   │ │ │
│ │ │ in __exit__                                                                              │ │ │
│ │ │                                                                                          │ │ │
│ │ │   710 │   │   │   │   # to allow the cancellation to propagate                           │ │ │
│ │ │   711 │   │   │   │   if enter_task.uncancel() > self._cancelling:                       │ │ │
│ │ │   712 │   │   │   │   │   return None                                                    │ │ │
│ │ │ ❱ 713 │   │   │   raise asyncio.TimeoutError from exc_val                                │ │ │
│ │ │   714 │   │   return None                                                                │ │ │
│ │ │   715 │                                                                                  │ │ │
│ │ │   716 │   def timeout(self) -> None:                                                     │ │ │
│ │ ╰──────────────────────────────────────────────────────────────────────────────────────────╯ │ │
│ │ TimeoutError                                                                                 │ │
│ ╰──────────────────────────────────────────────────────────────────────────────────────────────╯ │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯

on main when this patch is in place.

Let's revert and think it again.

Summary by CodeRabbit

  • Refactor
    • Simplified the flash and file upload API by removing the insecure_tls parameter from method signatures.
    • Unified file handling for both local and remote files for improved consistency.
    • Restored the flash command in the auto-generated CLI interface.

✏️ Tip: You can customize this high-level summary in your review settings.

@mangelajo mangelajo requested a review from bennyz January 16, 2026 13:38
@netlify
Copy link

netlify bot commented Jan 16, 2026

Deploy Preview for jumpstarter-docs ready!

Name Link
🔨 Latest commit bf93388
🔍 Latest deploy log https://app.netlify.com/projects/jumpstarter-docs/deploys/696a3f501735df0008819a46
😎 Deploy Preview https://deploy-preview-802--jumpstarter-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 16, 2026

📝 Walkthrough

Walkthrough

The PR removes HTTP-specific download and TLS handling logic from RideSXClient, treating all file paths uniformly via operator_for_path. The flash_images and flash methods are simplified by removing the insecure_tls parameter. CLI wiring is updated to include all commands uniformly instead of special-casing the flash command.

Changes

Cohort / File(s) Change Summary
HTTP & TLS Logic Removal
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
Eliminated _is_http_url, _download_http_to_storage, and HTTP-specific download/handling logic. Removed insecure_tls parameter from _upload_file_if_needed, flash_images, and flash methods. All file paths now handled uniformly via operator_for_path without conditional TLS branches.
CLI Wiring Simplification
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
Changed cli() method to iterate all generic commands uniformly instead of excluding flash and injecting a custom implementation, effectively restoring the flash command in auto-generated CLI.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • jumpstarter-dev/jumpstarter#783: Modifies the same RideSXClient methods in opposite direction—that PR adds HTTP/insecure-TLS handling that this PR removes—creating a direct inverse relationship.

Poem

🐰 Away with HTTP tangles and TLS woes so deep!
The flash now flows so clean and neat,
One path for all, no branching trees—
The code hops lighter on your knees! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: reverting HTTP and insecure HTTPS support from a previous commit, which is the core objective of the pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7963e82 and bf93388.

📒 Files selected for processing (1)
  • packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
🧰 Additional context used
📓 Path-based instructions (3)
packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py

📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)

Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use CompositeClient from jumpstarter_driver_composite.client for composite drivers with child drivers.

Files:

  • packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
packages/jumpstarter-driver-**/jumpstarter_driver_**/*.py

📄 CodeRabbit inference engine (.cursor/rules/creating-new-drivers.mdc)

Driver implementations should follow existing code style validated with make lint (fix with make lint-fix), perform static type checking with make ty-pkg-${package_name}, add comprehensive tests, and verify all tests pass with make test-pkg-${package_name} or make test

Files:

  • packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
**/*.py

📄 CodeRabbit inference engine (.cursor/rules/project-structure.mdc)

Ruff should be used for code formatting and linting, excluding jumpstarter-protocol package

Files:

  • packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
🧠 Learnings (6)
📚 Learning: 2025-11-27T09:58:55.346Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/project-structure.mdc:0-0
Timestamp: 2025-11-27T09:58:55.346Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/ : Driver packages must implement a `client.py` file containing the client implementation

Applied to files:

  • packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
📚 Learning: 2025-01-29T11:52:43.554Z
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:43.554Z
Learning: The TFTP driver (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/driver.py) handles all low-level concerns like path validation, error handling, and checksum computation. The client (packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py) should remain simple as it delegates these responsibilities to the driver.

Applied to files:

  • packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
📚 Learning: 2025-11-27T09:58:41.875Z
Learnt from: CR
Repo: jumpstarter-dev/jumpstarter PR: 0
File: .cursor/rules/creating-new-drivers.mdc:0-0
Timestamp: 2025-11-27T09:58:41.875Z
Learning: Applies to packages/jumpstarter-driver-*/jumpstarter_driver_*/client.py : Driver client CLIs should either implement known classes that provide a CLI interface, or implement their own CLI interface. Use `CompositeClient` from `jumpstarter_driver_composite.client` for composite drivers with child drivers.

Applied to files:

  • packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
📚 Learning: 2025-11-05T13:31:39.304Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:100-101
Timestamp: 2025-11-05T13:31:39.304Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, the `flash()` method and its CLI command intentionally have different defaults for `fls_version`: the method defaults to `""` (empty string) while the CLI defaults to `"0.1.5"`. This is by design to provide CLI users with a convenient default while keeping the programmatic API ready for when FLS is included in the flasher images.

Applied to files:

  • packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
📚 Learning: 2025-11-05T13:45:58.271Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:15-15
Timestamp: 2025-11-05T13:45:58.271Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, pexpect is intentionally used as a transitive dependency through the jumpstarter-driver-pyserial package. The flashers package does not declare pexpect as a direct dependency because the pyserial driver package is intended to control the pexpect version.

Applied to files:

  • packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
📚 Learning: 2025-01-29T11:52:50.888Z
Learnt from: bennyz
Repo: jumpstarter-dev/jumpstarter PR: 241
File: packages/jumpstarter-driver-tftp/jumpstarter_driver_tftp/client.py:52-60
Timestamp: 2025-01-29T11:52:50.888Z
Learning: The TFTP driver (driver.py) already handles error cases and file path security, so the client (client.py) doesn't need to duplicate these safeguards.

Applied to files:

  • packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Redirect rules - jumpstarter-docs
  • GitHub Check: Header rules - jumpstarter-docs
  • GitHub Check: Pages changed - jumpstarter-docs
  • GitHub Check: build
  • GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
  • GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
  • GitHub Check: e2e
  • GitHub Check: pytest-matrix (macos-15, 3.12)
  • GitHub Check: pytest-matrix (macos-15, 3.13)
  • GitHub Check: pytest-matrix (macos-15, 3.11)
  • GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
🔇 Additional comments (4)
packages/jumpstarter-driver-ridesx/jumpstarter_driver_ridesx/client.py (4)

25-47: LGTM! Clean revert of HTTP/TLS handling.

The simplified method signature and uniform file handling via operator_for_path is appropriate for reverting the problematic HTTP download logic. The method correctly delegates protocol-specific handling to the OpenDAL operator abstraction.


49-78: LGTM! Simplified flash_images flow.

The removal of insecure_tls parameter is consistent with the revert. The method now has a cleaner flow relying on the operator abstraction for file handling, which avoids the timeout issues reported with the HTTP download logic.


80-122: LGTM! Consistent removal of insecure_tls parameter.

The flash method signature and the internal call to flash_images are both updated consistently to remove the TLS-specific handling, completing the revert.


124-140: LGTM! Simplified CLI wiring.

The CLI now uniformly includes all commands from FlasherClient.cli() without special-casing the flash command. This restores the original behavior before the HTTP/TLS changes and is consistent with the coding guidelines for driver client CLIs.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@NickCao NickCao merged commit 640229b into main Jan 16, 2026
18 checks passed
@NickCao NickCao deleted the revert-783-ridesx-insecure branch January 16, 2026 14:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants