feat: auto-connect jmp shell to existing leases#330
Merged
raballew merged 10 commits intojumpstarter-dev:mainfrom Mar 18, 2026
Merged
feat: auto-connect jmp shell to existing leases#330raballew merged 10 commits intojumpstarter-dev:mainfrom
raballew merged 10 commits intojumpstarter-dev:mainfrom
Conversation
Resolves jumpstarter-dev#326 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Only show the current user's leases, not other clients' leases - Show exporter name, selector, and expiry time for each lease - Use numbered list with details instead of raw UUIDs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughAutomatically discover and connect to active leases when Changes
Sequence DiagramsequenceDiagram
actor User
participant Shell
participant LeaseAPI
participant TTY
participant Conn
User->>Shell: run `jmp shell` (no selector/lease)
Shell->>LeaseAPI: list_leases(current_client, only_active=True)
LeaseAPI-->>Shell: LeaseList
alt No leases
Shell-->>User: Error + guidance (no active leases found)
else Single lease
Shell->>Conn: connect(lease.name)
Conn-->>User: Connected shell session
else Multiple leases
Shell->>TTY: prompt(choices via _format_lease_display)
TTY-->>Shell: selected_lease
Shell->>Conn: connect(selected_lease)
Conn-->>User: Connected shell session
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
75-90:⚠️ Potential issue | 🟠 MajorTest masks async bug—mock returns
LeaseListdirectly.The mock
config.list_leases = Mock(return_value=_make_lease_list([]))returns aLeaseListsynchronously, but the reallist_leasesis an async method. This test passes, but production code will fail because_resolve_lease_from_activedoesn't await the coroutine.Once the async issue in
shell.pyis fixed, update this mock to return an awaitable (e.g.,AsyncMock(return_value=...)) to accurately reflect production behavior.🤖 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 75 - 90, The test uses a synchronous Mock for config.list_leases but the real method is async and _resolve_lease_from_active (called via shell.callback) should await it; update the test to use AsyncMock(return_value=_make_lease_list([])) for config.list_leases (ensure AsyncMock is imported) so the mock returns an awaitable that matches production behavior and the async path exercised by shell.callback and _resolve_lease_from_active.
🧹 Nitpick comments (1)
python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py (1)
136-151: Minor overlap withtest_shell_requires_selector_or_name_when_no_leases.This test covers the same no-leases scenario as lines 75-89 but adds the valuable assertion that
list_leaseswas called withonly_active=True. Consider consolidating or keeping both if the explicit parameter assertion is important to maintain separately.🤖 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 136 - 151, The two tests overlap on the "no active leases" scenario; choose to either consolidate them into one test or keep both but make their different intent explicit—if keeping both, ensure test_shell_no_leases_shows_guidance retains the assertion that config.list_leases was called with only_active=True (config.list_leases.assert_called_once_with(only_active=True)) and modify test_shell_requires_selector_or_name_when_no_leases to focus solely on selector/name validation (remove or adjust the duplicate lease-call assertion) so each test has a single, clear responsibility (refer to test_shell_no_leases_shows_guidance and test_shell_requires_selector_or_name_when_no_leases).
🤖 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/jumpstarter_cli/shell.py`:
- Around line 309-312: The helper _resolve_lease_from_active is calling async
method config.list_leases() synchronously; change it to an async function (async
def _resolve_lease_from_active(...)) and await config.list_leases(...) inside
it, or alternatively keep it sync but run the coroutine via
anyio.run(config.list_leases(...)) before accessing .leases; update callers
(e.g., shell() or move the logic into _shell_with_signal_handling which already
runs in an async context) so they await the new async _resolve_lease_from_active
or call it with anyio.run as appropriate.
---
Outside diff comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py`:
- Around line 75-90: The test uses a synchronous Mock for config.list_leases but
the real method is async and _resolve_lease_from_active (called via
shell.callback) should await it; update the test to use
AsyncMock(return_value=_make_lease_list([])) for config.list_leases (ensure
AsyncMock is imported) so the mock returns an awaitable that matches production
behavior and the async path exercised by shell.callback and
_resolve_lease_from_active.
---
Nitpick comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py`:
- Around line 136-151: The two tests overlap on the "no active leases" scenario;
choose to either consolidate them into one test or keep both but make their
different intent explicit—if keeping both, ensure
test_shell_no_leases_shows_guidance retains the assertion that
config.list_leases was called with only_active=True
(config.list_leases.assert_called_once_with(only_active=True)) and modify
test_shell_requires_selector_or_name_when_no_leases to focus solely on
selector/name validation (remove or adjust the duplicate lease-call assertion)
so each test has a single, clear responsibility (refer to
test_shell_no_leases_shows_guidance and
test_shell_requires_selector_or_name_when_no_leases).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a85ca56-14cf-405a-bc32-545a443c2669
📒 Files selected for processing (3)
.gitignorepython/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # .gitignore
Member
|
Thanks!, tested and working: |
Member
|
many queued jobs, github seems overwhelmed |
Keep auto-lease test variant over upstream's pre-auto-lease validation test, as the auto-lease behavior supersedes the old check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
384-396: Consider usinganyio.runfor consistency.The code uses
asyncio.run()for_resolve_lease_from_active_asyncbutanyio.run()for_shell_with_signal_handling. While this works (both complete before the other starts), usinganyio.runconsistently would align better with the codebase's async framework choice.♻️ Optional: Use anyio.run for consistency
- lease_name = asyncio.run(_resolve_lease_from_active_async(config)) + lease_name = anyio.run(_resolve_lease_from_active_async, config)Then remove the
import asyncioat line 1 if no longer needed.🤖 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.py` around lines 384 - 396, Replace the asyncio.run call with anyio.run for consistency: call anyio.run(_resolve_lease_from_active_async, config) instead of asyncio.run(...) so both asynchronous invocations use anyio; update the call site that currently invokes asyncio.run for _resolve_lease_from_active_async and then call anyio.run for _shell_with_signal_handling as before, and if asyncio is no longer used anywhere in this module remove the asyncio import.
🤖 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-cli/jumpstarter_cli/shell.py`:
- Around line 384-396: Replace the asyncio.run call with anyio.run for
consistency: call anyio.run(_resolve_lease_from_active_async, config) instead of
asyncio.run(...) so both asynchronous invocations use anyio; update the call
site that currently invokes asyncio.run for _resolve_lease_from_active_async and
then call anyio.run for _shell_with_signal_handling as before, and if asyncio is
no longer used anywhere in this module remove the asyncio import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cccb7900-74e9-4cdc-b6b5-140bfed42997
📒 Files selected for processing (2)
python/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py
Member
Author
|
@mangelajo tests seem to be flaky or is this really related to my PR? |
Use anyio.run for both async invocations in the shell command, removing the asyncio dependency from the module. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
python/packages/jumpstarter-cli/jumpstarter_cli/shell.py (1)
337-343: Avoid empty()in non-TTY lease summaries.If
_format_lease_display()returns an empty string, the error text currently renders entries likelease-name (). Small polish, but it degrades UX in sparse lease records.♻️ Proposed tweak
- lease_summaries = [ - f"{lease.name} ({_format_lease_display(lease)})" for lease in leases - ] + lease_summaries = [] + for lease in leases: + info = _format_lease_display(lease) + lease_summaries.append(f"{lease.name} ({info})" if info else lease.name)🤖 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.py` around lines 337 - 343, The lease summary list currently always appends " ({...})" even when _format_lease_display(lease) returns an empty string, producing "lease-name ()"; change the comprehension that builds lease_summaries so it only adds the parenthesized suffix when the formatted string is non-empty (use _format_lease_display(lease) once, keep it in a local variable inside the comprehension or a small helper, and construct either lease.name or f"{lease.name} ({display})"), then leave the raise click.UsageError message assembly unchanged.python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py (1)
156-178: TTY picker path is not actually under test here.Because
anyio.runis mocked, this test never reachessys.stdin.isatty(), lease listing, orclick.prompt. Consider a direct_resolve_lease_from_active_asynctest withisatty=Trueand a mocked prompt selection.🤖 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 156 - 178, The test mocks anyio.run so the TTY picker path never executes; instead directly test the async resolver _resolve_lease_from_active_async with sys.stdin.isatty patched to return True and with the lease listing and click.prompt patched to simulate the interactive selection; replace the current test_shell_multi_lease_tty_picker flow by calling _resolve_lease_from_active_async (awaiting it via anyio.run or pytest.mark.asyncio) while patching jumpstarter_cli.shell.sys.stdin.isatty to True, patching the lease list provider to return multiple leases, and patching click.prompt to return the chosen lease name (e.g., "lease-b") and assert the resolver returns that name and exits/returns expected values instead of relying on the mocked anyio.run call path.
🤖 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-cli/jumpstarter_cli/shell_test.py`:
- Around line 156-178: The test mocks anyio.run so the TTY picker path never
executes; instead directly test the async resolver
_resolve_lease_from_active_async with sys.stdin.isatty patched to return True
and with the lease listing and click.prompt patched to simulate the interactive
selection; replace the current test_shell_multi_lease_tty_picker flow by calling
_resolve_lease_from_active_async (awaiting it via anyio.run or
pytest.mark.asyncio) while patching jumpstarter_cli.shell.sys.stdin.isatty to
True, patching the lease list provider to return multiple leases, and patching
click.prompt to return the chosen lease name (e.g., "lease-b") and assert the
resolver returns that name and exits/returns expected values instead of relying
on the mocked anyio.run call path.
In `@python/packages/jumpstarter-cli/jumpstarter_cli/shell.py`:
- Around line 337-343: The lease summary list currently always appends "
({...})" even when _format_lease_display(lease) returns an empty string,
producing "lease-name ()"; change the comprehension that builds lease_summaries
so it only adds the parenthesized suffix when the formatted string is non-empty
(use _format_lease_display(lease) once, keep it in a local variable inside the
comprehension or a small helper, and construct either lease.name or
f"{lease.name} ({display})"), then leave the raise click.UsageError message
assembly unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1b6373b-09c8-42c1-858a-25c76abf3720
📒 Files selected for processing (2)
python/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py
Avoid displaying "lease-name ()" when lease has no metadata. Rewrite tests to exercise _resolve_lease_from_active_async directly instead of mocking anyio.run. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py (1)
76-90: Consider consolidating the two no-lease tests.Both tests assert the same user-facing failure path; combining them would reduce maintenance noise.
♻️ Optional consolidation sketch
-def test_shell_requires_selector_or_name_when_no_leases(): +def test_shell_requires_selector_or_name_when_no_leases(): config = Mock(spec=ClientConfigV1Alpha1) config.metadata = type("Metadata", (), {"name": "test-client"})() config.list_leases = AsyncMock(return_value=_make_lease_list([])) with pytest.raises(click.UsageError, match="no active leases found"): shell.callback.__wrapped__.__wrapped__( config=config, command=(), lease_name=None, selector=None, exporter_name=None, duration=timedelta(minutes=1), exporter_logs=False, acquisition_timeout=None, ) + config.list_leases.assert_called_once_with(only_active=True) - - -def test_shell_no_leases_shows_guidance(): - ...Also applies to: 138-154
🤖 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 76 - 90, Consolidate the duplicate "no active leases" tests by merging the other test (around lines 138-154) into a single parametrized test so both input variations assert the same UsageError; update test_shell_requires_selector_or_name_when_no_leases (which calls shell.callback.__wrapped__.__wrapped__) to use pytest.mark.parametrize over the differing inputs (e.g., different combinations of selector/lease_name/exporter_name) and remove the redundant test, ensuring the AsyncMock for config.list_leases returns _make_lease_list([]) and the pytest.raises(match="no active leases found") assertion remains.
🤖 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-cli/jumpstarter_cli/shell_test.py`:
- Around line 76-90: Consolidate the duplicate "no active leases" tests by
merging the other test (around lines 138-154) into a single parametrized test so
both input variations assert the same UsageError; update
test_shell_requires_selector_or_name_when_no_leases (which calls
shell.callback.__wrapped__.__wrapped__) to use pytest.mark.parametrize over the
differing inputs (e.g., different combinations of
selector/lease_name/exporter_name) and remove the redundant test, ensuring the
AsyncMock for config.list_leases returns _make_lease_list([]) and the
pytest.raises(match="no active leases found") assertion remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d667cccf-348b-46e2-b677-26ec63791020
📒 Files selected for processing (2)
python/packages/jumpstarter-cli/jumpstarter_cli/shell.pypython/packages/jumpstarter-cli/jumpstarter_cli/shell_test.py
mangelajo
approved these changes
Mar 18, 2026
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
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.
Summary
jmp shellwithout--selectoror--name, automatically resolve from active leasesCloses #326
Test plan
--lease,--selector,--name, andJMP_LEASEenv var still work🤖 Generated with Claude Code