Conversation
WalkthroughThe changes introduce new utility functions for flattening and combining tracebacks in exception groups, add an asynchronous signal handler for graceful shutdown, and refactor the CLI entrypoint to use asynchronous context management and exception handling. Additionally, the linter configuration is updated to target Python 3.11. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (j)
participant j_async
participant SignalHandler
participant TaskGroup
participant ClientCLI
User->>CLI (j): Run CLI command
CLI (j)->>j_async: Call asynchronously
j_async->>TaskGroup: Start async task group
j_async->>SignalHandler: Start signal handler (SIGINT/SIGTERM)
j_async->>ClientCLI: Run CLI logic in thread
SignalHandler-->>TaskGroup: On signal, cancel tasks
TaskGroup-->>j_async: Propagate cancellation
j_async-->>CLI (j): Handle exceptions, exit
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
packages/jumpstarter-cli-common/jumpstarter_cli_common/signal.py (1)
20-22: Minor: thebreakis redundant once the surrounding scope is cancelled
scope.cancel()will immediately unwind the enclosingCancelScope, so the loop exits naturally.
Keepingbreakis harmless but not necessary—dropping it simplifies the flow slightly.packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (3)
50-58: Rename the first parameter to avoid the misleading “self” outside a classAt module level, a parameter called
selfsuggests a method, butleaf_exceptions()is a standalone function.-def leaf_exceptions(self: BaseExceptionGroup, *, fix_tracebacks: bool = True) -> list[BaseException]: +def leaf_exceptions(group: BaseExceptionGroup, *, fix_tracebacks: bool = True) -> list[BaseException]:(And adjust the internal
_flattencall accordingly.)
This improves readability without behaviour changes.
60-74: Type-safety & performance consideration for traceback cloningEvery leaf exception is copied via
with_traceback, which allocates new traceback objects for each recursion level.
For very large exception trees this can become expensive.If you only need the first combined traceback (PEP 654 typical usage), short-circuit once the first non-group exception is found to avoid unnecessary allocations, or expose a flag to skip traceback cloning altogether.
77-105: Guard against future changes intypes.TracebackTypeconstructor
types.TracebackType(...)is CPython-specific and positional/keyword parameters can change between versions.
Consider replacing the manual reconstruction with the public helper in the PEP reference implementation:import traceback new_tb = traceback.StackSummary.from_list( traceback.extract_tb(tb1) + traceback.extract_tb(tb2) ).as_traceback()This avoids relying on private internals and keeps the code implementation-agnostic.
packages/jumpstarter-cli/jumpstarter_cli/j.py (3)
17-22: Minor: drop the lambda & gain clarity
to_thread.run_synccan accept the callable directly; wrapping the call in a zero-arglambdaintroduces an extra stack frame and obscures what is being executed.- await to_thread.run_sync(lambda: client.cli()(standalone_mode=False)) + await to_thread.run_sync( + client.cli(), # click command/function + cancellable=True, + # `standalone_mode=False` is passed via to_thread’s *args + args=(), + kwargs=dict(standalone_mode=False), + )While purely cosmetic, this preserves introspection (function name in tracebacks) and removes one level of indirection.
24-31: Double-cancelling the task-group is redundant
tg.cancel_scopeis already passed tosignal_handler, which will callcancel()on SIGINT/SIGTERM.
Invokingtg.cancel_scope.cancel()again infinally:is harmless but unnecessary and slightly obscures intent.- finally: - tg.cancel_scope.cancel() + finally: + # Nothing to cancel here; `signal_handler` or normal completion + # will already close the scope. + passConsider dropping the explicit cancel for terser, clearer flow.
45-45: Qualifier for the top-levelruncallThe bare
run(j_async)relies on the earlierfrom anyio import runimport.
For better readability and to avoid confusion with the built-inrunpy.run_…helpers, consider aliasing:-from anyio import create_task_group, get_cancelled_exc_class, run, to_thread +from anyio import create_task_group, get_cancelled_exc_class, run as anyio_run, to_thread ... - run(j_async) + anyio_run(j_async)Purely stylistic, but it makes the origin explicit to future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(2 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/signal.py(1 hunks)packages/jumpstarter-cli/jumpstarter_cli/j.py(1 hunks)pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
🔇 Additional comments (1)
pyproject.toml (1)
60-62: Explicitly targeting Python 3.11 requires matching runtime & CI imagesSetting
target-version = "py311"tells Ruff to lint as if the code is executed under Python 3.11 features (e.g.except*, pattern-matching).
Please double-check that
- every published package’s
pyproject.toml/setup.cfgalready advertises a minimum Python 3.11 (e.g.requires-python = ">=3.11"), and- all CI jobs (lint, tests, wheels) run on a 3.11 interpreter.
Otherwise contributors working on 3.10 or CI still pinned to 3.10 will get false positives/failures.
| except* click.ClickException as excgroup: | ||
| for exc in leaf_exceptions(excgroup): | ||
| exc.show() | ||
|
|
||
| sys.exit(1) |
There was a problem hiding this comment.
except* misses plain ClickException – user-facing errors will leak a traceback
except* only catches ExceptionGroup; a solitary click.ClickException raised by the CLI executed in to_thread.run_sync will bypass this handler and bubble up as an unformatted traceback. Add a regular except click.ClickException branch before (or in addition to) the except* block.
+ except click.ClickException as exc:
+ exc.show()
+ sys.exit(1)
except* click.ClickException as excgroup:
for exc in leaf_exceptions(excgroup):
exc.show()
sys.exit(1)Failing to do so degrades the UX and breaks the contract of returning exit-code 1 on CLI errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except* click.ClickException as excgroup: | |
| for exc in leaf_exceptions(excgroup): | |
| exc.show() | |
| sys.exit(1) | |
| except click.ClickException as exc: | |
| exc.show() | |
| sys.exit(1) | |
| except* click.ClickException as excgroup: | |
| for exc in leaf_exceptions(excgroup): | |
| exc.show() | |
| sys.exit(1) |
There was a problem hiding this comment.
Do we ever get a single exception? should we catch it just in case @NickCao ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Successfully created backport PR for |
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores