Improve j commands exception handling#328
Merged
Merged
Conversation
Normalize common runtime/network exceptions into user friendly messages and add validation for login and flashing inputs so users get actionable errors before long running operations start. Assisted-by: GPT-5.3 Codex Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Add unit and cli tests for login and flasher validation plus common exception mapping to ensure user facing errors remain clear and consistent. Assisted-by: GPT-5.3 Codex Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Handle cancel/abort, invalid JSON, auth/permission gRPC failures, and invalid argument responses with concise actionable messages, and add tests to keep the mappings stable. Assisted-by: GPT-5.3 Codex Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Map common exceptions in the reauth decorator fallback, tighten test fixtures for stable local execution, and drop redundant flasher preflight validation/tests to keep behavior aligned with existing downstream handling. Assisted-by: GPT-5.3 Codex Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes CLI exception translation by adding private mappers for runtime, TLS, I/O, JSON, DNS, KeyboardInterrupt/Abort and gRPC errors; routes all sync/async handlers and reauth flows through the mapper; adds login endpoint and auth-config payload validation; and adds tests for mappings and login behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Backend as Network / gRPC / File IO
participant Mapper as Exception Mapper (private helpers)
participant Reauth as Reauth Handler
participant Click as ClickException output
CLI->>Backend: perform operation (sync/async)
Backend-->>CLI: raises exception (timeout / ssl / gRPC / JSON / IO / KeyboardInterrupt)
CLI->>Mapper: submit exception for mapping
alt mapped to ClickException
Mapper-->>Click: return user-facing ClickException
CLI-->>Click: raise/display ClickException
else authentication error
Mapper-->>Reauth: indicate auth failure
Reauth-->>CLI: attempt reauthentication (may invoke login callback)
Reauth-->>Mapper: provide retry outcome
Mapper-->>Click: return/display final ClickException or success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Member
Author
|
There are many cases that could be covered, and I’m not sure how many of them are actually needed vs. overkill, so feel free to suggest adding anything missing or removing anything unnecessary. |
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py`:
- Around line 79-89: The linter flags B009 for using getattr with a constant
after hasattr checks; update the exception attribute access by replacing
getattr(exc, "code") and getattr(exc, "details") with direct attribute access
(exc.code and exc.details) in the block that computes grpc_code/code and
details, and similarly replace getattr(grpc_code, "name", str(grpc_code)) with
direct attribute access to grpc_code.name when present (falling back to
str(grpc_code)) so the code uses exc.code(), exc.details(), and grpc_code.name
directly in the try/except handling.
- Around line 54-57: The long error message inside the ConnectionRefusedError
branch in exceptions.py exceeds 120 chars; split the string literal into two (or
use implicit concatenation inside the existing parentheses) so the line length
is under 120. Edit the block that returns ClickExceptionRed in the
isinstance(exc, ConnectionRefusedError) branch and break the message into
multiple shorter string literals (or use +/adjacent quoted strings) while
preserving the same message and the {message} interpolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddb68d07-fec0-4096-9730-4200279a9cb9
📒 Files selected for processing (5)
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.pypython/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions_test.pypython/packages/jumpstarter-cli/jumpstarter_cli/login.pypython/packages/jumpstarter-cli/jumpstarter_cli/login_test.pypython/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py`:
- Around line 225-226: Replace uses of _map_common_exception with
_map_cli_exception in the reauthentication branches so the CLI mapping is
applied consistently: locate the branches that do "elif common_exc :=
_map_common_exception(exc): raise common_exc from None" (and the similar
occurrence later) and call _map_cli_exception(exc) instead, assign to common_exc
as before and re-raise it ("raise common_exc from None") so the reauth flow uses
the same CLI exception translator as the main decorators.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/login.py`:
- Around line 28-36: The _validate_login_endpoint_url function currently accepts
"http" without checking the CLI flag; update it so that if parsed.scheme ==
"http" it raises a click.ClickException unless the explicit opt-in flag is set
(--insecure-login-http). Modify _validate_login_endpoint_url to accept an
insecure_login_http: bool parameter (or read from
click.get_current_context().params["insecure_login_http"]) and if scheme ==
"http" and not insecure_login_http raise a clear error instructing the user to
pass --insecure-login-http to allow plain HTTP; keep the existing checks for
unsupported schemes and missing host intact.
- Around line 128-131: The validation currently checks parts[0] and parts[1] for
empty strings but doesn't trim whitespace, so inputs like "name@ " slip
through; update the code that handles the split result (the parts list) to trim
whitespace (e.g., use parts[0].strip() and parts[1].strip() or reassign trimmed
variables) before performing the empty checks and raising the ClickException
messages, ensuring you validate the trimmed values rather than the raw split
tokens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04c38fec-2220-480f-ae49-062733cda8a1
📒 Files selected for processing (2)
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.pypython/packages/jumpstarter-cli/jumpstarter_cli/login.py
Refine shared exception translation paths and tighten login endpoint parsing/validation so insecure HTTP requires explicit opt-in and whitespace-only login target segments fail with clear errors. Assisted-by: GPT-5.3 Codex Signed-off-by: Bella Khizgiyaev <bkhizgiy@redhat.com>
Member
|
Nice improvement! |
mangelajo
approved these changes
Mar 18, 2026
raballew
requested changes
Mar 18, 2026
Member
raballew
left a comment
raballew
left a comment
There was a problem hiding this comment.
The timeout type tuple in _map_runtime_exception lists TimeoutError, asyncio.TimeoutError, socket.timeout, and FutureTimeoutError -- but since Python 3.11 (your minimum) these are all the same object. Just TimeoutError is enough.
handle_exceptions_with_reauthentication is missing a KeyboardInterrupt handler. The other two decorators both catch it and show "Cancelled by user." cleanly, but this one only has except Exception which won't catch KeyboardInterrupt (it inherits from BaseException), so the user gets a raw traceback instead.
The login tests (test_login_cli_shows_timeout_message, test_login_cli_shows_certificate_message) monkeypatch fetch_auth_config to raise click.ClickException directly, so they never exercise the exception mapping logic in fetch_auth_config. If that mapping code were removed, the tests would still pass.
Member
Author
|
@raballew thanks for the feedback, I"ll post it as a follow-up PR. |
Member
4 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Updated exception handling for J commands to show cleaner, actionable errors for common failures: timeouts, TLS/certificate issues, DNS/connection problems, file and permission errors, bad JSON, and user cancellations. It also now maps key gRPC errors (DEADLINE_EXCEEDED, UNAVAILABLE, UNAUTHENTICATED/PERMISSION_DENIED, INVALID_ARGUMENT) to friendlier messages.
Also made sure the re-auth wrapper uses the same fallback mapping and improved jmp login fetch handling so invalid JSON/content-type responses are reported clearly.
Related to: #57
Summary by CodeRabbit
New Features
Refactor
Tests