Skip to content

fix: attach config to ConnectionError on pre-flight token expiry check#370

Merged
mangelajo merged 1 commit intojumpstarter-dev:mainfrom
bennyz:worktree-fix-162-expired-token
Mar 23, 2026
Merged

fix: attach config to ConnectionError on pre-flight token expiry check#370
mangelajo merged 1 commit intojumpstarter-dev:mainfrom
bennyz:worktree-fix-162-expired-token

Conversation

@bennyz
Copy link
Copy Markdown
Member

@bennyz bennyz commented Mar 23, 2026

The pre-flight token expiry check in _shell_with_signal_handling raised a bare ConnectionError("token is expired") without attaching the client config. When the re-auth handler called exc.get_config() it received None, causing an AttributeError crash in relogin_client().

Fixes: #162

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 23, 2026

Deploy Preview for jumpstarter-docs ready!

Name Link
🔨 Latest commit 0fdbc73
🔍 Latest deploy log https://app.netlify.com/projects/jumpstarter-docs/deploys/69c119be011e9f00086b2b07
😎 Deploy Preview https://deploy-preview-370--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
Copy Markdown
Contributor

coderabbitai Bot commented Mar 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2cc353ba-bc65-40c4-a928-bfce01827a4a

📥 Commits

Reviewing files that changed from the base of the PR and between ac508da and 0fdbc73.

📒 Files selected for processing (2)
  • python/packages/jumpstarter-cli/jumpstarter_cli/shell.py
  • python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py

📝 Walkthrough

Walkthrough

When the shell detects an expired token it now constructs a ConnectionError("token is expired"), calls err.set_config(config) on that exception, and raises it so downstream reauthentication handlers can access the config. A test was added that supplies an expired JWT and verifies reauth is triggered.

Changes

Cohort / File(s) Summary
Token-expiry enrichment
python/packages/jumpstarter-cli/jumpstarter_cli/shell.py
On expired-token branch, create ConnectionError("token is expired"), attach config via err.set_config(config), then raise the enriched exception (replaces raising plain ConnectionError).
Token-expiry test coverage
python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py
Add _make_expired_jwt() helper and test_expired_token_triggers_reauth() which injects an expired token, wraps the shell call with handle_exceptions_with_reauthentication(login_mock), asserts a click.ClickException is raised, and verifies login_mock is invoked once with the config.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested reviewers

  • evakhoni
  • mangelajo

Poem

🐰
A token fizzled, clocked and meek,
I found the config you couldn't seek.
I tuck it in the error's sleeve,
So login wakes and you reprieve—
Hop back in, the shell won't leak.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR addresses the root cause (missing config on ConnectionError) but does not fully resolve all issue #162 requirements including graceful config handling, event-loop safety, and clear user messaging. Add validation in relogin_client for None config, implement non-blocking asyncio approach, and ensure user-facing error messages guide re-login flow completion.
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main code change: attaching config to ConnectionError on token expiry check.
Description check ✅ Passed The description clearly explains the bug fix, its context, and references the related issue #162.
Out of Scope Changes check ✅ Passed All changes focus on attaching config to ConnectionError and testing the fix; no unrelated modifications are present.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/jumpstarter_cli/shell_test.py (1)

1-19: ⚠️ Potential issue | 🟡 Minor

Fix import ordering to pass linting.

The pipeline is failing due to ruff's I001 rule (import block is un-sorted or un-formatted). Run make lint-fix to auto-fix, or manually reorder the imports.

Proposed fix
 import base64
 import inspect
 import json
 import time
 from contextlib import asynccontextmanager
 from datetime import datetime, timedelta
 from unittest.mock import AsyncMock, Mock, patch

 import anyio
 import click
 import pytest

-from jumpstarter_cli.shell import _resolve_lease_from_active_async, _shell_with_signal_handling, shell
-
-from jumpstarter_cli_common.exceptions import handle_exceptions_with_reauthentication
-
 from jumpstarter.client.grpc import Lease, LeaseList
 from jumpstarter.config.client import ClientConfigV1Alpha1
 from jumpstarter.config.env import JMP_LEASE
+from jumpstarter_cli.shell import _resolve_lease_from_active_async, _shell_with_signal_handling, shell
+from jumpstarter_cli_common.exceptions import handle_exceptions_with_reauthentication

As per coding guidelines: "Use Ruff for code formatting and linting on all Python files except jumpstarter-protocol, invoked via make lint and make lint-fix".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py` around lines 1
- 19, The import block in shell_test.py is not sorted per ruff I001; reorder the
imports into the standard groups and alphabetical order (stdlib, third-party,
local) and/or run make lint-fix to auto-fix; specifically fix the ordering of
imports such as base64, inspect, json, time, contextlib.asynccontextmanager,
datetime/timedelta, unittest.mock (AsyncMock/Mock/patch), then third-party
(anyio, click, pytest), then local module imports (from jumpstarter_cli.shell
import _resolve_lease_from_active_async, _shell_with_signal_handling, shell;
from jumpstarter_cli_common.exceptions import
handle_exceptions_with_reauthentication; from jumpstarter.client.grpc import
Lease, LeaseList; from jumpstarter.config.client import ClientConfigV1Alpha1;
from jumpstarter.config.env import JMP_LEASE) so the file passes ruff linting.
🤖 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/jumpstarter_cli/shell_test.py`:
- Around line 1-19: The import block in shell_test.py is not sorted per ruff
I001; reorder the imports into the standard groups and alphabetical order
(stdlib, third-party, local) and/or run make lint-fix to auto-fix; specifically
fix the ordering of imports such as base64, inspect, json, time,
contextlib.asynccontextmanager, datetime/timedelta, unittest.mock
(AsyncMock/Mock/patch), then third-party (anyio, click, pytest), then local
module imports (from jumpstarter_cli.shell import
_resolve_lease_from_active_async, _shell_with_signal_handling, shell; from
jumpstarter_cli_common.exceptions import
handle_exceptions_with_reauthentication; from jumpstarter.client.grpc import
Lease, LeaseList; from jumpstarter.config.client import ClientConfigV1Alpha1;
from jumpstarter.config.env import JMP_LEASE) so the file passes ruff linting.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1ce68803-f8e2-4893-84c8-85fa453c8afa

📥 Commits

Reviewing files that changed from the base of the PR and between 816e5bc and ac508da.

📒 Files selected for processing (2)
  • python/packages/jumpstarter-cli/jumpstarter_cli/shell.py
  • python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py

The pre-flight token expiry check in _shell_with_signal_handling raised
a bare ConnectionError("token is expired") without attaching the client
config. When the re-auth handler called exc.get_config() it received
None, causing an AttributeError crash in relogin_client().

This was originally fixed in PR jumpstarter-dev#256 and accidentally reverted in
54ae7d5.

Fixes: jumpstarter-dev#162

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Assisted-by: claude-opus-4.6
@bennyz bennyz force-pushed the worktree-fix-162-expired-token branch from ac508da to 0fdbc73 Compare March 23, 2026 10:45
@bennyz bennyz requested a review from evakhoni March 23, 2026 12:28
@mangelajo mangelajo merged commit 3d32308 into jumpstarter-dev:main Mar 23, 2026
31 checks passed
@bennyz bennyz deleted the worktree-fix-162-expired-token branch March 23, 2026 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

2 participants