client: attach config to exception by bennyz · Pull Request #256 · jumpstarter-dev/jumpstarter

bennyz · 2026-02-23T10:06:11Z

client config was not attached to the exception when token was expired, while connection errors did.

this patch introduces a helper to attach the config to the exceptions when possible

Summary by CodeRabbit

Bug Fixes
- Enhanced token expiration error handling for improved consistency and reliability in error reporting across the command-line interface

client config was not attached to the exception when token was expired, while connection errors did. this patch introduces a helper to attach the config to the exceptions when possible Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assited-by: cursor auto

coderabbitai · 2026-02-23T10:06:29Z

📝 Walkthrough

Walkthrough

This change refactors token expiry error handling by extracting inlined logic into two reusable helper functions in the config client module, which are then used by the shell module to consolidate error handling.

Changes

Cohort / File(s)	Summary
Token Expiry Error Helpers `python/packages/jumpstarter/jumpstarter/config/client.py`	Introduced `_attach_config_if_expired_token()` and `raise_expired_token_error()` functions to centralize token expiry error handling. Updated `_handle_connection_error()` to use the new helper. Added `NoReturn` to typing imports.
Shell Integration `python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`	Imported `raise_expired_token_error()` and replaced inline ConnectionError raising with a call to the new helper function for consistent token expiry error handling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 Tokens expire, errors arise clear,
But helpers extracted bring joy and cheer!
Config attached, messages unified,
The shell now calls what once was typed—
Refactored grace in code so neat! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'client: attach config to exception' directly and clearly summarizes the main change: attaching client configuration to exceptions when a token expires.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

📝 Generate docstrings (stacked PR)
📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

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.}

coderabbitai

🧹 Nitpick comments (1)

python/packages/jumpstarter/jumpstarter/config/client.py (1)

53-63: Consider extracting "token is expired" to a module-level constant.

The same literal is used in both _attach_config_if_expired_token (detection, line 55) and raise_expired_token_error (construction, line 61). If either string drifts they'll silently decouple — the helper will stop recognising errors raised by its sibling.

♻️ Proposed refactor

+_TOKEN_EXPIRED_MSG = "token is expired"
+

 def _attach_config_if_expired_token(exc: ConnectionError, config: ClientConfigV1Alpha1) -> None:
     """Attach config to a ConnectionError so re-auth can use it. No-op if not token-expired."""
-    if "token is expired" in str(exc):
+    if _TOKEN_EXPIRED_MSG in str(exc):
         exc.set_config(config)


 def raise_expired_token_error(config: ClientConfigV1Alpha1) -> NoReturn:
     """Raise ConnectionError for expired token with config attached so re-auth can run."""
-    err = ConnectionError("token is expired")
+    err = ConnectionError(_TOKEN_EXPIRED_MSG)
     err.set_config(config)
     raise err

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@python/packages/jumpstarter/jumpstarter/config/client.py` around lines 53 -
63, The literal "token is expired" is duplicated between
_attach_config_if_expired_token and raise_expired_token_error; extract it to a
module-level constant (e.g., TOKEN_EXPIRED_MESSAGE = "token is expired") and use
that constant in both functions (replace the string check in
_attach_config_if_expired_token and the constructor call in
raise_expired_token_error) so detection and construction stay in sync.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@python/packages/jumpstarter/jumpstarter/config/client.py`:
- Around line 53-63: The literal "token is expired" is duplicated between
_attach_config_if_expired_token and raise_expired_token_error; extract it to a
module-level constant (e.g., TOKEN_EXPIRED_MESSAGE = "token is expired") and use
that constant in both functions (replace the string check in
_attach_config_if_expired_token and the constructor call in
raise_expired_token_error) so detection and construction stay in sync.

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bacfa00 and dc544de.

📒 Files selected for processing (2)

python/packages/jumpstarter-cli/jumpstarter_cli/shell.py
python/packages/jumpstarter/jumpstarter/config/client.py

evakhoni · 2026-02-23T12:36:44Z

just checked the fix - it worked, but i'm seeing a warning just as the shell starts

 ~/repos/jumpstarter/python  ➦ dc544de2  uv run jmp shell --selector board-type=virtual 
[02/23/26 14:32:04] INFO     INFO:jumpstarter.client.lease:Acquiring lease      lease.py:78
                             019c8a7c-3d6e-735e-8c85-a0aa92761f54 for selector             
                             board-type=virtual for duration 0:30:00                       
                    INFO     INFO:jumpstarter.client.lease:Waiting for ready   lease.py:252
                             connection at                                                 
                             /run/user/1000/jumpstarter-mwvh70uc/socket                    
python ⚡virtual-aarch64-4c-8g-01 ➤ [02/23/26 14:32:05] WARNING  WARNING:jumpstarter.streams.common:stream copy    common.py:30
                             interrupted (BrokenResourceError):

which wasn't there prior to the fix

 ~/repos/jumpstarter/python  ↰ main  uv run jmp shell --selector board-type=virtual 
[02/23/26 14:32:44] INFO     INFO:jumpstarter.client.lease:Acquiring lease      lease.py:78
                             019c8a7c-dd43-7bbb-b9ee-5822dc41ac2d for selector             
                             board-type=virtual for duration 0:30:00                       
[02/23/26 14:32:45] INFO     INFO:jumpstarter.client.lease:Waiting for ready   lease.py:252
                             connection at                                                 
                             /run/user/1000/jumpstarter-qxzv53zl/socket                    
python ⚡virtual-aarch64-4c-8g-01 ➤

edit: nop, my main was few commits old. fetched the latest and the warning is already there as well. so not your regression. your fix LGTM

bennyz · 2026-02-23T12:44:26Z

warning is actually from #247 😅

evakhoni · 2026-02-23T12:52:11Z

right, just seen it there lol

evakhoni · 2026-02-23T12:58:58Z

so wdyt should I open an issue for that one? seeing it in every session like that is far from ideal 🤔

bennyz · 2026-02-23T13:03:51Z

so wdyt should I open an issue for that one? seeing it in every session like that is far from ideal 🤔

yeah, at the time we weren't sure why all of these were suppressed (now we know)

coderabbitai bot reviewed Feb 23, 2026

View reviewed changes

bennyz requested review from evakhoni and mangelajo February 23, 2026 10:30

bennyz mentioned this pull request Feb 23, 2026

crash when token expires and we try to renew the token with a jmp shell command. #162

Open

evakhoni approved these changes Feb 23, 2026

View reviewed changes

evakhoni mentioned this pull request Feb 23, 2026

prevent BrokenResourceError message during normal stream connection teardown #257

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

client: attach config to exception#256

client: attach config to exception#256
bennyz wants to merge 1 commit intojumpstarter-dev:mainfrom
bennyz:fix-missing-config

bennyz commented Feb 23, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Feb 23, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

evakhoni commented Feb 23, 2026 •

edited

Loading

Uh oh!

bennyz commented Feb 23, 2026

Uh oh!

evakhoni commented Feb 23, 2026

Uh oh!

evakhoni commented Feb 23, 2026

Uh oh!

bennyz commented Feb 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

bennyz commented Feb 23, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

evakhoni commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bennyz commented Feb 23, 2026

Uh oh!

evakhoni commented Feb 23, 2026

Uh oh!

evakhoni commented Feb 23, 2026

Uh oh!

bennyz commented Feb 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bennyz commented Feb 23, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Feb 23, 2026 •

edited

Loading

evakhoni commented Feb 23, 2026 •

edited

Loading