fix: guard DurationParamType against OverflowError on large values by raballew · Pull Request #722 · jumpstarter-dev/jumpstarter

raballew · 2026-06-01T08:15:39Z

Summary

Fixes OverflowError crash in DurationParamType.convert() when extremely large integer values (e.g. 86400000000000) are passed, which cause timedelta(seconds=...) to overflow
Catches OverflowError in both the integer and string-integer parsing paths and converts it to a user-friendly click.BadParameter error
Extracts string parsing logic into _parse_string() for clarity and adds OverflowError to the pytimeparse2 exception handling
Adds regression tests for large positive integers, large string integers, and large negative string integers

Closes #717

Test plan

test_large_string_integer_raises_bad_parameter_not_overflow -- verifies string "86400000000000" raises BadParameter not OverflowError
test_large_integer_raises_bad_parameter_not_overflow -- verifies int 86400000000000 raises BadParameter not OverflowError
test_large_negative_string_raises_bad_parameter_not_overflow -- verifies string "-86400000000000" raises BadParameter not OverflowError
All existing DurationParamType tests continue to pass

🤖 Generated with Claude Code

…alues Catch OverflowError from timedelta(seconds=...) in all three input paths (integer type, string-parsed integer, pytimeparse2 result) and raise click.BadParameter with a user-friendly message instead of letting the OverflowError propagate uncaught. Extract string parsing into _parse_string to keep convert complexity within the C901 threshold. Generated-By: Forge/20260601_094654_364912_325fe880

coderabbitai · 2026-06-01T08:15:48Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Extracts DurationParamType string parsing into _parse_string (int seconds → pytimeparse2 → TypeAdapter(timedelta)) with OverflowError handling; convert() delegates str/int to it, retains timedelta passthrough and minimum-duration validation, and adds tests for oversized inputs causing click.BadParameter.

Changes

Duration Parsing Refactor with Overflow Protection

Layer / File(s)	Summary
Duration Parsing Helper and Convert Refactor `python/packages/jumpstarter-cli/jumpstarter_cli/common.py`	`DurationParamType._parse_string()` centralizes parsing with fallbacks: integer-seconds → `pytimeparse2` → `TypeAdapter(timedelta)`, catches `OverflowError` and reports “exceeds the maximum allowed duration”; `convert()` delegates str/int to the helper, keeps `timedelta` passthrough, and retains minimum-duration validation. A blank line was added after `_opt_selector_callback`.
Overflow Test Coverage `python/packages/jumpstarter-cli/jumpstarter_cli/common_test.py`	Added tests asserting that extremely large numeric and string durations cause `click.BadParameter` with an “exceeds the maximum allowed duration” message; the unsupported-type test was moved after these cases.

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I nibbled at seconds, short and tall,
Guarded the bounds to stop the fall,
Timedeltas now sleep without alarm,
No OverflowError breaks the charm,
Hooray — safe parsing for one and all!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.09% 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 PR title clearly and concisely describes the main change: guarding DurationParamType against OverflowError on large values, which is the primary objective of this changeset.
Description check	✅ Passed	The PR description is directly related to the changeset, explaining the OverflowError issue, the fix implementation, and the test plan for validating the changes.
Linked Issues check	✅ Passed	The code changes fully address the requirements in issue `#717`: OverflowError is caught in both integer and string-integer parsing paths within _parse_string(), converted to click.BadParameter with a user-friendly message, and comprehensive regression tests validate large positive/negative values.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the OverflowError issue in DurationParamType: the _parse_string() extraction, OverflowError handling, and corresponding tests remain within the stated scope.

_{✏️ 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.}

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

The ty type checker cannot infer that self.fail() is NoReturn, so it reports an implicit None return. Re-raise the caught exception after self.fail() to make the control flow explicit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

evakhoni

all in all looking good! we can simplify a bit as I commented below:

- F001: catch OverflowError separately from (ValueError, TypeError) in the parse_duration path so overflowing pytimeparse2 results produce the correct "exceeds the maximum allowed duration" message instead of falling through to the generic "is not a valid duration" error - F002: remove unreachable `raise exc` after self.fail() (which is NoReturn) and drop the now-unused `as exc` binding to satisfy ruff F841 - F003: merge the isinstance(value, int) branch in convert() into the str path by converting int to str before calling _parse_string, eliminating duplicated overflow guard logic Add test for pytimeparse2-triggered overflow to cover the F001 fix path. Generated-By: Forge/20260603_092845_714320_2a6250dc

coderabbitai

Actionable comments posted: 1

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

Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/common.py`:
- Around line 40-67: The _parse_string function can implicitly return None
because mypy/ty doesn't treat self.fail(...) as non-returning; update
_parse_string to make all error paths explicitly non-returning by adding an
explicit raise after each self.fail call (for example: self.fail(...); raise
RuntimeError from None) so the type-checker knows these branches never return;
target the self.fail calls inside _parse_string (the ones after
int/OverflowError, after parse_duration/OverflowError, and in the final except
handling TypeAdapter.validate_python) and keep the rest of the logic
(parse_duration, TypeAdapter(timedelta).validate_python) unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6c43dfd8-294b-4648-8e06-aabe8229c2ac

📥 Commits

Reviewing files that changed from the base of the PR and between d706fa7 and 0f3bd18.

📒 Files selected for processing (2)

python/packages/jumpstarter-cli/jumpstarter_cli/common.py
python/packages/jumpstarter-cli/jumpstarter_cli/common_test.py

ty cannot statically verify that click's self.fail() is NoReturn, causing an invalid-return-type error on _parse_string(). Adding unreachable raise statements after each self.fail() call makes the non-returning control flow explicit for the type checker. Generated-By: Forge/20260603_100418_749257_f488b03d Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-authored-by: Mohamad Abo Ras <maboras@redhat.com>

…sistency The _parse_string method consistently adds `raise` after every self.fail() call to satisfy the ty type-checker (which cannot verify that self.fail is NoReturn). The convert() method was missing this pattern in two locations: the else branch for unsupported types and the minimum duration check. This could cause ty to report possibly-unbound diagnostics for the `td` variable. Generated-By: Forge/20260604_121105_16706_dd080c63

Add a test verifying that a value at the upper boundary of timedelta.max (999999999 days = 86399999913600 seconds) succeeds, complementing the existing overflow tests that use clearly-overflowing values. This documents the exact threshold and would catch any off-by-one errors in boundary handling. Generated-By: Forge/20260604_121105_16706_dd080c63

coderabbitai

Actionable comments posted: 1

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

Inline comments:
In `@python/packages/jumpstarter-cli/jumpstarter_cli/common_test.py`:
- Around line 111-114: The test test_value_at_timedelta_max_seconds_succeeds
uses the wrong boundary value: replace the input string "86399999913600" passed
to DURATION.convert with the true max whole-second boundary "86399999999999" and
update the assertion to expect the corresponding maximal timedelta of 999999999
days and 23:59:59 (e.g., timedelta(days=999999999, seconds=86399) or equivalent)
so the test validates the immediate pre-overflow edge.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f7292149-263a-4008-8049-eb3388642e26

📥 Commits

Reviewing files that changed from the base of the PR and between c877983 and f5981e7.

📒 Files selected for processing (2)

python/packages/jumpstarter-cli/jumpstarter_cli/common.py
python/packages/jumpstarter-cli/jumpstarter_cli/common_test.py

🚧 Files skipped from review as they are similar to previous changes (1)

python/packages/jumpstarter-cli/jumpstarter_cli/common.py

Use 86399999999999 (999999999d 23h 59m 59s) instead of 86399999913600 to test the true maximum seconds value that fits in timedelta.max. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

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

evakhoni

ok looks good

raballew and others added 2 commits June 1, 2026 12:28

fix: resolve formatting issues in common.py and common_test.py

d7d1bb2

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

raballew enabled auto-merge (squash) June 1, 2026 15:04

raballew requested a review from evakhoni June 1, 2026 17:29

evakhoni reviewed Jun 2, 2026

View reviewed changes

Comment thread python/packages/jumpstarter-cli/jumpstarter_cli/common.py Outdated

Comment thread python/packages/jumpstarter-cli/jumpstarter_cli/common.py Outdated

Comment thread python/packages/jumpstarter-cli/jumpstarter_cli/common.py

coderabbitai Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread python/packages/jumpstarter-cli/jumpstarter_cli/common.py

raballew requested a review from evakhoni June 3, 2026 08:54

maboras-rh reviewed Jun 3, 2026

View reviewed changes

Comment thread python/packages/jumpstarter-cli/jumpstarter_cli/common.py Outdated

Comment thread python/packages/jumpstarter-cli/jumpstarter_cli/common.py

evakhoni reviewed Jun 4, 2026

View reviewed changes

Comment thread python/packages/jumpstarter-cli/jumpstarter_cli/common.py

raballew and others added 3 commits June 4, 2026 12:07

Update python/packages/jumpstarter-cli/jumpstarter_cli/common.py

c877983

Co-authored-by: Mohamad Abo Ras <maboras@redhat.com>

coderabbitai Bot reviewed Jun 4, 2026

View reviewed changes

Comment thread python/packages/jumpstarter-cli/jumpstarter_cli/common_test.py

raballew and others added 2 commits June 4, 2026 13:04

fix: use exact timedelta max boundary value in test

b611c26

Use 86399999999999 (999999999d 23h 59m 59s) instead of 86399999913600 to test the true maximum seconds value that fits in timedelta.max. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fix: accept float values in DurationParamType.convert

800d4f5

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

evakhoni approved these changes Jun 4, 2026

View reviewed changes

raballew merged commit c141332 into jumpstarter-dev:main Jun 4, 2026
31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: guard DurationParamType against OverflowError on large values#722

fix: guard DurationParamType against OverflowError on large values#722
raballew merged 10 commits into
jumpstarter-dev:mainfrom
raballew:512-duration-overflow-guard

raballew commented Jun 1, 2026

Uh oh!

coderabbitai Bot commented Jun 1, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Estimated Code Review Effort

Poem

❌ Failed checks (1 warning)

Uh oh!

evakhoni left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

evakhoni left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

raballew commented Jun 1, 2026

Summary

Test plan

Uh oh!

coderabbitai Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated Code Review Effort

Poem

❌ Failed checks (1 warning)

Uh oh!

evakhoni left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evakhoni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jun 1, 2026 •

edited

Loading

evakhoni left a comment •

edited

Loading