Skip to content

fix(cli): reject out-of-range durations instead of overflowing - #3044

Merged
johntmyers merged 1 commit into
NVIDIA:mainfrom
natedemoss:fix/duration-parse-overflow
Aug 31, 2026
Merged

fix(cli): reject out-of-range durations instead of overflowing#3044
johntmyers merged 1 commit into
NVIDIA:mainfrom
natedemoss:fix/duration-parse-overflow

Conversation

@natedemoss

Copy link
Copy Markdown
Contributor

Summary

parse_duration_to_ms multiplied the parsed number by its unit multiplier without a range check, so a large --since value on openshell logs overflows i64. A debug build (what scripts/bin/openshell runs) panics with attempt to multiply with overflow; a release build wraps and the CLI silently computes a nonsense log cutoff. This replaces the bare multiply with checked_mul and a diagnostic.

Related Issue

No issue required: obvious localized bug fix, contained to one parsing helper in the CLI.

Changes

  • crates/openshell-cli/src/commands/common.rs: parse_duration_to_ms uses checked_mul and returns duration out of range: {s} (must fit in milliseconds as a 64-bit integer) instead of overflowing.
  • Added unit tests for the overflow case (positive and negative) and for the largest value that still parses, so the boundary is not accidentally tightened later.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable)

mise and Docker are not installed on this workstation, so I ran the pre-commit steps individually rather than through the task:

  • cargo fmt --all -- --check — clean.
  • cargo test -p openshell-cli --lib — 239 passed, 0 failed, including the three new tests.
  • python scripts/update_license_headers.py --check — 862 files, all headers present.
  • cargo clippy -p openshell-cli --all-targets -- -D warnings — clean for openshell-cli, but I had to add -A clippy::unused_async to get there. On Windows the #[cfg(not(unix))] connect_unix stub in crates/openshell-extension-core/src/transport.rs:185 has no .await and trips clippy::unused_async, failing the workspace lint before openshell-cli is reached. That is pre-existing on main and unrelated to this change; I left it alone rather than widen this PR. Happy to file it separately.

Reproduction, on main: openshell logs <sandbox> --since 9223372036854775807h panics. After the change it reports duration out of range. --since 30s|5m|1h resolves to the same cutoff before and after.

The call site's now_ms - dur_ms (run.rs:7056) cannot overflow once the multiply is bounded: the largest accepted value is i64::MAX rounded down to a whole multiplier, and now_ms is on the order of 1.8e12, so the difference stays above i64::MIN.

No docs change: the --since syntax documented in docs/sandboxes/manage-sandboxes.mdx is unchanged.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable)

AI assistance: an agent explored the CLI parsing helpers, wrote the fix and tests, and ran the checks. I reviewed it and can explain it — the defect is the unchecked num * multiplier at the end of parse_duration_to_ms, its only caller is the --since handling in run.rs, and the fix leaves every in-range result identical.

parse_duration_to_ms multiplied the parsed number by its unit
multiplier without a range check. A large --since value on
`openshell logs` overflows i64: a debug build panics with "attempt to
multiply with overflow", and a release build wraps to a nonsense log
cutoff.

Use checked_mul and return a diagnostic naming the offending input.
Every in-range result is unchanged, and the largest accepted duration
still leaves the caller's `now_ms - dur_ms` well inside i64.

Signed-off-by: Nathan DeMoss <ndemoss28@gmail.com>
Copilot AI lite review requested due to automatic review settings August 31, 2026 17:12
@copy-pr-bot

copy-pr-bot Bot commented Aug 31, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@johntmyers

Copy link
Copy Markdown
Collaborator

/ok to test 176401e

@johntmyers
johntmyers enabled auto-merge August 31, 2026 18:27
@johntmyers
johntmyers added this pull request to the merge queue Aug 31, 2026
Merged via the queue into NVIDIA:main with commit 9d449ef Aug 31, 2026
56 checks passed
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.

3 participants