Skip to content

fix: small fixes batch (#516, #525, #652, #517)#681

Open
raballew wants to merge 6 commits into
jumpstarter-dev:mainfrom
raballew:488-small-fixes-batch
Open

fix: small fixes batch (#516, #525, #652, #517)#681
raballew wants to merge 6 commits into
jumpstarter-dev:mainfrom
raballew:488-small-fixes-batch

Conversation

@raballew
Copy link
Copy Markdown
Member

@raballew raballew commented May 13, 2026

Closes #516
Closes #525
Closes #652
Closes #517

raballew and others added 6 commits May 13, 2026 09:50
The TODO claimed the debug flag needed forwarding to the uboot client,
but it is already passed via debug=self._console_debug in both
reboot_to_console() call sites.

Closes jumpstarter-dev#516

Generated-By: Forge/20260513_094530_2089907_aa882926
Adds a user-friendly error message for the gRPC FAILED_PRECONDITION
status code, matching the pattern of the five existing status code
handlers in _map_grpc_exception.

Closes jumpstarter-dev#652

Generated-By: Forge/20260513_094530_2089907_aa882926
These flags only apply to client configs. When used with an exporter
config, they are silently ignored, which can confuse users. Now a
warning is emitted explaining the flags have no effect.

Closes jumpstarter-dev#525

Generated-By: Forge/20260513_094530_2089907_aa882926
Replaces the hardcoded retry count of 100 with a keyword argument
retries=100, preserving backward compatibility while allowing callers
to adjust the retry count.

Closes jumpstarter-dev#517

Generated-By: Forge/20260513_094530_2089907_aa882926
…_console retries

The existing tests only verified the function signature via inspect, which
would pass even if the retries parameter were accepted but silently ignored
in the loop body. Replace the redundant default-value test with a behavioral
test that mocks serial.pexpect() to always timeout and asserts that exactly
N ESC sends occur when retries=N, proving the parameter controls the loop.

Generated-By: Forge/20260513_094530_2089907_aa882926

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add bool type annotation to debug parameter, success-path and retries=0
edge case tests for reboot_to_console, remove unused login import.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR addresses four separate TODOs across the Jumpstarter codebase: handling gRPC FAILED_PRECONDITION errors with user-friendly messages, warning about incompatible login flags with exporter configurations, making U-Boot boot interrupt retries configurable, and cleaning up a deferred console debug propagation TODO.

Changes

CLI Exception Handling: FAILED_PRECONDITION Mapping

Layer / File(s) Summary
gRPC FAILED_PRECONDITION exception mapping and tests
python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py, exceptions_test.py
Added handling for FAILED_PRECONDITION gRPC status code in _map_grpc_exception, returning a red Click exception with user-facing message and guidance. Two unit tests verify the exception message includes "precondition" text both with and without error details.

Login Exporter/Client-Only Flag Warnings

Layer / File(s) Summary
Flag warning helper and integration
python/packages/jumpstarter-cli/jumpstarter_cli/login.py
Introduced _warn_exporter_client_only_flags(config_kind, allow, unsafe) to emit warnings when client-only flags are used with exporter configurations, replacing the prior inline TODO comment. The function is called after configuration kind is determined and before issuer/token processing.
Warning behavior validation tests
python/packages/jumpstarter-cli/jumpstarter_cli/login_test.py
Added four tests verifying the helper emits expected warnings for exporter/unsafe combinations, remains silent for client configurations, and silent when no flags are provided; used capsys to capture and validate stdout output.

U-Boot Retry Configurability

Layer / File(s) Summary
Configurable retries parameter
python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/client.py
Updated reboot_to_console method signature to accept keyword-only retries: int = 100 parameter, replacing the hardcoded 100-attempt loop with iteration over the configurable value.
Retry parameter tests
python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/client_test.py
Added comprehensive test suite validating the parameter exists with correct default, that retries limit escape-sequence attempts (raising RuntimeError on failure), that the context manager yields on successful prompt detection, and that retries=0 raises immediately without sending escape sequences.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

  • #652: The FAILED_PRECONDITION exception handler directly addresses the "excessive error message" issue by mapping the status to a user-friendly red Click exception with only the essential details.
  • #525: The login warning helper implements the requested check to warn when client-specific options (--allow, --unsafe) are used with exporter configurations.
  • #517: The retries parameter makes U-Boot boot interrupt attempts configurable as requested, replacing the hardcoded 100-attempt constant.

Possibly related PRs

  • jumpstarter-dev/jumpstarter#348: Both PRs modify UbootConsoleClient.reboot_to_console in jumpstarter_driver_uboot/client.py, changing its retry/attempt loop and control flow around how the console interaction proceeds.
  • jumpstarter-dev/jumpstarter#328: Main PR extends the centralized gRPC exception-mapping in jumpstarter_cli_common/exceptions.py by adding FAILED_PRECONDITION handling and corresponding tests, fitting with the broader gRPC error mapping work.

Suggested reviewers

  • mangelajo
  • NickCao

Poem

🐰 Four TODO's laid to rest at last,
From gRPC errors cast in stone,
Through login flags and retries vast,
The cleanup whispers, well-grown.
Warnings guide the weary way,
Configurability wins the day!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.79% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title references multiple issue numbers and concisely describes a batch of small fixes matching the changeset contents.
Linked Issues check ✅ Passed All four linked issues (#516, #525, #652, #517) have corresponding implementations in the changeset meeting their objectives.
Out of Scope Changes check ✅ Passed All changes are directly aligned with the four linked issues; no out-of-scope modifications were introduced.
Description check ✅ Passed The pull request description clearly maps four distinct issues (#516, #525, #652, #517) to specific changes in the codebase, directly aligning with the changesets shown in the raw summary.

✏️ 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.

@raballew raballew requested review from evakhoni and mangelajo May 13, 2026 08:54
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.

🧹 Nitpick comments (2)
python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/client_test.py (1)

49-76: 💤 Low value

Consider using 0 instead of None for accurate pexpect behavior.

The test mocks expect_exact to return None on successful match (line 54), but the real pexpect.expect_exact() returns the index of the matched pattern (which would be 0 for a single pattern). While the current implementation doesn't check the return value—so None works—using 0 would more accurately simulate pexpect's actual behavior.

🔍 More accurate mock
     mock_pexpect_process.expect_exact = MagicMock(
-        side_effect=[pexpect.TIMEOUT("timeout"), pexpect.TIMEOUT("timeout"), None]
+        side_effect=[pexpect.TIMEOUT("timeout"), pexpect.TIMEOUT("timeout"), 0]
     )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/client_test.py`
around lines 49 - 76, The test's mock for pexpect.expect_exact in
test_reboot_to_console_yields_on_prompt_match should return 0 instead of None to
more accurately emulate pexpect (it returns the matched pattern index); update
the side_effect in mock_pexpect_process.expect_exact (the list currently
[pexpect.TIMEOUT("timeout"), pexpect.TIMEOUT("timeout"), None]) to use 0 for the
successful match so the behavior of reboot_to_console and the assertions
(mock_pexpect_process.send.call_count and mock_power.cycle) remain valid.
python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/client.py (1)

22-35: ⚡ Quick win

Document the new retries parameter in the docstring.

The method signature now accepts a retries parameter with a default of 100, but the docstring does not mention it. Users need to know that this parameter controls how many boot-interrupt attempts are made before raising RuntimeError.

📝 Suggested docstring update
     def reboot_to_console(self, *, debug: bool = False, retries: int = 100) -> Generator[None]:
         """
         Reboot to U-Boot console
 
         Power cycle the target and wait for the U-Boot prompt
 
         Must be used as a context manager, other methods can only be
         used within the reboot_to_console context
+
+        Args:
+            debug: Enable debug logging to stdout
+            retries: Maximum number of boot-interrupt attempts (default 100)
 
         >>> with uboot.reboot_to_console(debug=True): # doctest: +SKIP
         ...     uboot.set_env("foo", "bar")
         ...     uboot.setup_dhcp()
         >>> # uboot.set_env("foo", "baz") # invalid use
         """
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/client.py`
around lines 22 - 35, Update the reboot_to_console docstring to document the new
retries parameter: state that reboot_to_console(self, *, debug: bool = False,
retries: int = 100) accepts retries (int, default 100) which controls how many
boot-interrupt/attempts are made waiting for the U-Boot prompt before raising
RuntimeError; include brief guidance on when to increase it and that it only
applies to the context-manager boot interrupt loop inside reboot_to_console.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/client_test.py`:
- Around line 49-76: The test's mock for pexpect.expect_exact in
test_reboot_to_console_yields_on_prompt_match should return 0 instead of None to
more accurately emulate pexpect (it returns the matched pattern index); update
the side_effect in mock_pexpect_process.expect_exact (the list currently
[pexpect.TIMEOUT("timeout"), pexpect.TIMEOUT("timeout"), None]) to use 0 for the
successful match so the behavior of reboot_to_console and the assertions
(mock_pexpect_process.send.call_count and mock_power.cycle) remain valid.

In `@python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/client.py`:
- Around line 22-35: Update the reboot_to_console docstring to document the new
retries parameter: state that reboot_to_console(self, *, debug: bool = False,
retries: int = 100) accepts retries (int, default 100) which controls how many
boot-interrupt/attempts are made waiting for the U-Boot prompt before raising
RuntimeError; include brief guidance on when to increase it and that it only
applies to the context-manager boot interrupt loop inside reboot_to_console.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c34bd97d-4842-4da9-8c52-4200b899f8de

📥 Commits

Reviewing files that changed from the base of the PR and between 1d056d3 and 900c12a.

📒 Files selected for processing (7)
  • python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py
  • python/packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions_test.py
  • python/packages/jumpstarter-cli/jumpstarter_cli/login.py
  • python/packages/jumpstarter-cli/jumpstarter_cli/login_test.py
  • python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
  • python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/client.py
  • python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/client_test.py
💤 Files with no reviewable changes (1)
  • python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant