fix(cli): avoid panic on multi-byte UTF-8 in --since duration

[andrewwhitecdw]

Summary

openshell logs <name> --since <duration> panics when the duration's last character is multi-byte UTF-8 (e.g. --since 5€). parse_duration_to_ms split the input with split_at(s.len() - 1), indexing by byte length, which is not a char boundary for multi-byte characters. The CLI now returns the intended "unknown duration unit" error instead of crashing.

This PR supersedes #2406, which was auto-closed by the vouch-check workflow before I was vouched.

Related Issue

N/A — small fix found during code review (panic verified with a standalone repro: "5€".split_at(3) → byte index 3 is not a char boundary).

Changes

• parse_duration_to_ms (now in commands/common.rs) splits off the last character using its UTF-8 length (char::len_utf8) instead of assuming a single byte
• Added regression tests in commands/common.rs for valid units and multi-byte input

Testing

• mise run pre-commit passes (mise unavailable in this environment; ran equivalent cargo fmt + cargo clippy -p openshell-cli --all-targets — clean)
• Unit tests added/updated (cargo test -p openshell-cli parse_duration_to_ms — 2 passed)
• E2E tests added/updated (if applicable)

Checklist

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

[copy-pr-bot]

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.

[github-actions]

All contributors have signed the DCO ✍️ ✅
_{Posted by the DCO Assistant Lite bot.}

[andrewwhitecdw]

I have read the DCO document and I hereby sign the DCO.

[johntmyers]

gator-agent

PR Review Status

Validation: This is a project-valid, concentrated CLI bug fix that prevents openshell logs --since from panicking on a multi-byte UTF-8 unit. It supersedes #2406, which was closed only by the earlier vouch gate.
Head SHA: 5b038779924ad59313f71d6cfafe1f7d40ee1c1f

Review findings:

• No blocking or actionable findings remain. The UTF-8 boundary calculation is correct and the regression tests cover both 5€ and a multi-byte-only input.

Docs: Not needed because this preserves the documented CLI contract and replaces a crash with the intended validation error.

E2E: Not required for this isolated duration-parser fix; no sandbox lifecycle, gateway, policy, network, credential, driver, GPU, Kubernetes, or packaging behavior changes.

Next state: gator:watch-pipeline

[johntmyers]

/ok to test 5b03877

[elezar]

As a general question, instead of fixing this here, would replacing the parsing with an exernal crate that explicitly parses to std::time::Duration here be better?

[andrewwhitecdw]

Good question. The current parser intentionally accepts a small, kubectl logs --since-compatible set of units (s, m, h) and this PR only fixes the byte-index panic on multi-byte input. Moving to a crate like humantime or similar would be cleaner for new code, but it would widen the accepted grammar and could accept inputs that the server-side duration handling rejects. I kept this PR as a strict bug-fix (same contract, no panic). Happy to do a follow-up refactor to standardize on a crate across CLI and server if the team prefers that direction.

[andrewwhitecdw]

@elezar thanks for the question — I replied above. The current change keeps the existing kubectl-compatible unit set and only fixes the multi-byte panic, so switching crates is probably best left to a follow-up if you want to widen the accepted syntax. Could a maintainer re-trigger the cancelled CI gate when convenient? The code itself should be ready for another /ok-to-test pass.

[johntmyers]

gator-agent

Monitoring Complete

Monitoring is complete because this PR has merged.

Final status: The concentrated CLI panic fix was reviewed with no blocking findings at head 5b038779924ad59313f71d6cfafe1f7d40ee1c1f, maintainer approval was present, and the required Branch Checks, Helm Lint, E2E, and GPU E2E status contexts were successful before merge.

I removed the active gator:* label because there is nothing left for gator to monitor on this PR.