refactor exception display #707
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
WalkthroughAdds a generic exception-chain traversal utility and a TypeVar-based API; refactors retryable/non-retryable detection to use the new extractor; enhances flash error handling to aggregate leaf exceptions from ExceptionGroup/BaseExceptionGroup; adds imports and a module-level type variable. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant BaseFlasherClient as BaseFlasherClient
participant FlashOp as Flash Operation
participant ExceptionGroup as ExceptionGroup/BaseExceptionGroup
Caller->>BaseFlasherClient: flash(...)
BaseFlasherClient->>FlashOp: execute
FlashOp-->>BaseFlasherClient: result or raises Exception
alt Exception raised
BaseFlasherClient->>BaseFlasherClient: _find_exception_in_chain(e, FlashRetryableError)
alt Retryable found
BaseFlasherClient-->>Caller: raise FlashRetryableError
else No retryable
BaseFlasherClient->>BaseFlasherClient: _find_exception_in_chain(e, FlashNonRetryableError)
alt Non-retryable found
BaseFlasherClient-->>Caller: raise FlashNonRetryableError
else Unexpected
opt e is ExceptionGroup/BaseExceptionGroup
BaseFlasherClient->>ExceptionGroup: leaf_exceptions(e)
ExceptionGroup-->>BaseFlasherClient: list of leaf exceptions
Note right of BaseFlasherClient: Aggregate leaf types/messages
end
BaseFlasherClient-->>Caller: raise FlashError(with details)
end
end
else Success
BaseFlasherClient-->>Caller: result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential review focus:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
1-27: Fix import order to satisfy Ruff (I001).Imports are unsorted/mis-grouped. Reorder into stdlib, third-party, then local, alphabetically.
Apply this diff:
-import hashlib -import json -import os -import re -import sys -import threading -import time -from contextlib import contextmanager -from dataclasses import dataclass -from pathlib import Path, PosixPath -from queue import Queue -from urllib.parse import urlparse - -import click -import requests -from jumpstarter_driver_composite.client import CompositeClient -from jumpstarter_driver_opendal.client import FlasherClient, OpendalClient, operator_for_path -from jumpstarter_driver_opendal.common import PathBuf -from jumpstarter_driver_pyserial.client import Console -from opendal import Metadata, Operator - -from jumpstarter_driver_flashers.bundle import FlasherBundleManifestV1Alpha1 - -from jumpstarter.client.decorators import driver_click_group -from jumpstarter.common.exceptions import ArgumentError, JumpstarterException -from jumpstarter_cli_common.exceptions import leaf_exceptions -from typing import TypeVar, Type +import hashlib +import json +import os +import re +import sys +import threading +import time +from contextlib import contextmanager +from dataclasses import dataclass +from pathlib import Path, PosixPath +from queue import Queue +from typing import Type, TypeVar +from urllib.parse import urlparse + +import click +import requests +from opendal import Metadata, Operator + +from jumpstarter.client.decorators import driver_click_group +from jumpstarter.common.exceptions import ArgumentError, JumpstarterException +from jumpstarter_cli_common.exceptions import leaf_exceptions +from jumpstarter_driver_composite.client import CompositeClient +from jumpstarter_driver_flashers.bundle import FlasherBundleManifestV1Alpha1 +from jumpstarter_driver_opendal.client import FlasherClient, OpendalClient, operator_for_path +from jumpstarter_driver_opendal.common import PathBuf +from jumpstarter_driver_pyserial.client import Console
180-214: Handle BaseExceptionGroup separately before generic Exception
- Introduce
except BaseExceptionGroup as e:beforeexcept Exception as e:inpackages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.pyand relocate the leaf-exceptions logic there.- Declare
python_requires=">=3.11"inpyproject.tomlto ensureBaseExceptionGroupsupport.
🧹 Nitpick comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
221-254: Also traverse__context__when searching the exception chain.Exceptions linked implicitly are in
__context__. Add a context walk after the cause walk.Apply this diff:
while current is not None: if isinstance(current, exc_type): return current # Also check if the cause is a BaseExceptionGroup if isinstance(current, BaseExceptionGroup): for sub_exc in leaf_exceptions(current, fix_tracebacks=False): result = self._find_exception_in_chain(sub_exc, exc_type) if result is not None: return result current = getattr(current, '__cause__', None) - return None + # Check the context chain as well + current = getattr(exception, '__context__', None) + while current is not None: + if isinstance(current, exc_type): + return current + if isinstance(current, BaseExceptionGroup): + for sub_exc in leaf_exceptions(current, fix_tracebacks=False): + result = self._find_exception_in_chain(sub_exc, exc_type) + if result is not None: + return result + current = getattr(current, '__context__', None) + return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
leaf_exceptions(110-134)
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
[error] 1-1: RUFF I001: Import block is un-sorted or un-formatted.
🪛 GitHub Check: ruff
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
[failure] 1-29: Ruff (I001)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:1:1: I001 Import block is un-sorted or un-formatted
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: e2e
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
🔇 Additional comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
256-263: LGTM: reuse of generic finder simplifies error detection.Delegating to
_find_exception_in_chainfor retryable/non-retryable lookup is clean and maintainable.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
208-216: Good improvement for ExceptionGroup diagnostics.The enhanced error handling for
BaseExceptionGroupwill make debugging much easier by showing all leaf exceptions. The implementation correctly usesleaf_exceptions()to flatten the group.Consider limiting the number of exceptions shown in the error message if there could be many leaf exceptions:
leaves = leaf_exceptions(e, fix_tracebacks=False) max_errors = 10 displayed_leaves = leaves[:max_errors] error_details = "; ".join([f"{type(exc).__name__}: {exc}" for exc in displayed_leaves]) if len(leaves) > max_errors: error_details += f"; ... and {len(leaves) - max_errors} more" self.logger.error(f"Flash operation failed with unexpected error(s): {error_details}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: michalskrivanek
Repo: jumpstarter-dev/jumpstarter PR: 584
File: packages/jumpstarter/jumpstarter/client/grpc.py:38-54
Timestamp: 2025-08-18T07:13:37.619Z
Learning: User michalskrivanek prefers inline code over small helper function extractions when the abstraction doesn't significantly improve readability or reduce complexity. They value straightforward, readable code over reducing minor duplication.
📚 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-flashers/jumpstarter_driver_flashers/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
leaf_exceptions(110-134)
🪛 GitHub Actions: Lint
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
[error] 1-1: Ruff: Import block is un-sorted or un-formatted.
🪛 GitHub Check: ruff
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
[failure] 1-30: Ruff (I001)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:1:1: I001 Import block is un-sorted or un-formatted
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: e2e
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
Comment on lines
+27
to
+28
|
|
||
| total_time = time.time() - start_time |
Contributor
There was a problem hiding this comment.
Fix import ordering to resolve pipeline failure.
Standard library imports must come before third-party/project imports per PEP 8. The typing imports should be moved up.
Apply this diff to fix the import order:
from jumpstarter.client.decorators import driver_click_group
from jumpstarter.common.exceptions import ArgumentError, JumpstarterException
+from typing import Type, TypeVar
+
from jumpstarter_cli_common.exceptions import leaf_exceptions
-from typing import TypeVar, Type🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
around lines 27 to 28, the import order violates PEP 8 by placing the
third-party/project import before standard library/type hints; move the typing
import (TypeVar, Type) above the jumpstarter_cli_common.exceptions import, and
ensure there is a blank line separating standard library/typing imports from
project imports.
Comment on lines
+224
to
257
| # Check the current exception | ||
| if isinstance(exception, FlashRetryableError): | ||
| if isinstance(exception, exc_type): | ||
| return exception | ||
|
|
||
| # Check the cause chain | ||
| current = getattr(exception, '__cause__', None) | ||
| while current is not None: | ||
| if isinstance(current, FlashRetryableError): | ||
| if isinstance(current, exc_type): | ||
| return current | ||
| # Also check if the cause is an ExceptionGroup | ||
| if hasattr(current, 'exceptions'): | ||
| for sub_exc in current.exceptions: | ||
| result = self._get_retryable_error(sub_exc) | ||
| # Also check if the cause is a BaseExceptionGroup | ||
| if isinstance(current, BaseExceptionGroup): | ||
| for sub_exc in leaf_exceptions(current, fix_tracebacks=False): | ||
| result = self._find_exception_in_chain(sub_exc, exc_type) | ||
| if result is not None: | ||
| return result | ||
| current = getattr(current, '__cause__', None) | ||
| return None | ||
|
|
||
| def _get_non_retryable_error(self, exception: Exception) -> FlashNonRetryableError | None: | ||
| """Find a non-retryable error in an exception (or any of its causes). | ||
| Args: | ||
| exception: The exception to check | ||
| Returns: | ||
| The FlashNonRetryableError if found, None otherwise | ||
| """ | ||
| # Check if this is an ExceptionGroup and look through its exceptions | ||
| if hasattr(exception, 'exceptions'): | ||
| for sub_exc in exception.exceptions: | ||
| result = self._get_non_retryable_error(sub_exc) | ||
| if result is not None: | ||
| return result | ||
|
|
||
| # Check the current exception | ||
| if isinstance(exception, FlashNonRetryableError): | ||
| return exception | ||
| def _get_retryable_error(self, exception: Exception) -> FlashRetryableError | None: | ||
| """Find a retryable error in an exception (or any of its causes).""" | ||
| return self._find_exception_in_chain(exception, FlashRetryableError) |
Contributor
There was a problem hiding this comment.
Type hint may be too narrow for BaseExceptionGroup.
The method signature declares exception: Exception, but BaseExceptionGroup is a subclass of BaseException, not Exception. This could cause type checking issues when the method is called with a BaseExceptionGroup instance.
Apply this diff to fix the type hint:
- def _find_exception_in_chain(self, exception: Exception, exc_type: Type[E]) -> E | None:
+ def _find_exception_in_chain(self, exception: BaseException, exc_type: Type[E]) -> E | None:
"""Find a specific exception type in an exception chain, including ExceptionGroups.
Args:Also update the callers to accept BaseException:
- def _get_retryable_error(self, exception: Exception) -> FlashRetryableError | None:
+ def _get_retryable_error(self, exception: BaseException) -> FlashRetryableError | None:
"""Find a retryable error in an exception (or any of its causes)."""
return self._find_exception_in_chain(exception, FlashRetryableError)
- def _get_non_retryable_error(self, exception: Exception) -> FlashNonRetryableError | None:
+ def _get_non_retryable_error(self, exception: BaseException) -> FlashNonRetryableError | None:
"""Find a non-retryable error in an exception (or any of its causes)."""
return self._find_exception_in_chain(exception, FlashNonRetryableError)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
around lines 224-257, the parameter type hint for _find_exception_in_chain is
too narrow (Exception) because BaseExceptionGroup subclasses BaseException, not
Exception; change the method signature to accept exception: BaseException (and
adjust the generic/type variable usage if needed) and update any callers'
annotations and any places that call this function to accept and pass
BaseException (or widen their param types) so static type checkers no longer
flag passing a BaseExceptionGroup.
Comment on lines
+259
to
+265
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clean refactoring that eliminates duplication.
The refactored methods effectively consolidate the exception chain traversal logic into a single generic method, improving maintainability.
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
around lines 259 to 265, the duplicated logic for traversing exception causes
was refactored into a single generic helper and the two small wrappers were
replaced by calls to that helper; no change is required—keep the refactor as-is,
ensure the helper method signature and return types remain correct for
FlashRetryableError and FlashNonRetryableError, and run existing tests to
validate behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
should make it more readable when unknown exception happens
Summary by CodeRabbit
Improvements
Bug Fixes