j commands exception handling edge cases#339
Conversation
Handle Ctrl+C consistently in the reauthentication wrapper, simplify timeout type checks, and add direct login config fetch tests so timeout/JSON mapping behavior is actually exercised. 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. |
📝 WalkthroughWalkthroughThe changes enhance exception handling in the CLI common package by removing obsolete asyncio imports, standardizing timeout error handling to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
207-213:⚠️ Potential issue | 🟡 MinorHandle
KeyboardInterruptinsidelogin_func()at the reauthentication point.When
login_func(config)is called on line 212 from inside_handle_connection_error_with_reauth(), aKeyboardInterruptraised during re-auth will not be caught by the sibling handler on line 251. This occurs because exceptions raised within an except block are not caught by sibling except handlers at the same try/except level—only by outer ones. The verification confirms this behavior:# KeyboardInterrupt raised inside except suite bypasses sibling handler try: try: raise Exception("expired token") except Exception: raise KeyboardInterrupt() # Raised here except KeyboardInterrupt: print("mapped") # This line is never reached except BaseException as exc: print(type(exc).__name__) # Prints: KeyboardInterruptThis means
KeyboardInterruptduring login will propagate uncaught instead of being mapped toClickExceptionRed("Cancelled by user").Suggested fix
Wrap the
login_func()call in its own try/except handler:def _handle_connection_error_with_reauth(exc, login_func): """Handle ConnectionError with reauthentication logic.""" if "expired" in str(exc).lower(): click.echo(click.style("Token is expired, triggering re-authentication", fg="red")) config = exc.get_config() - login_func(config) + try: + login_func(config) + except KeyboardInterrupt as interrupt: + if cli_exc := _map_cli_exception(interrupt): + raise cli_exc from None + raise raise ClickExceptionRed("Please try again now") from None else: raise ClickExceptionRed(str(exc)) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py` around lines 207 - 213, In _handle_connection_error_with_reauth, wrap the call to login_func(config) in a try/except that catches KeyboardInterrupt and raises ClickExceptionRed("Cancelled by user") from None so a user-cancel during reauth is mapped correctly; keep the existing flow that raises ClickExceptionRed("Please try again now") on successful reauth and let other exceptions propagate unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py`:
- Around line 207-213: In _handle_connection_error_with_reauth, wrap the call to
login_func(config) in a try/except that catches KeyboardInterrupt and raises
ClickExceptionRed("Cancelled by user") from None so a user-cancel during reauth
is mapped correctly; keep the existing flow that raises
ClickExceptionRed("Please try again now") on successful reauth and let other
exceptions propagate unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 84a18029-d2f8-4d08-b81e-02172ec7608a
📒 Files selected for processing (3)
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_test.py
2 participants
No description provided.