Uh oh!
There was an error while loading. Please reload this page.
fix(datetime): floor sub-second negative timestamps instead of snapping to epoch - #262
Conversation
There was a problem hiding this comment.
🚩 Pre-existing pub(super) violations outside the PR scope
The REVIEW.md rule prohibits pub(super), and there are existing usages in src/commands/satisfying_sort/numeric.rs, render.rs, sort_state.rs, and logo_math.rs. These are not touched by this PR and are not related to the change, so they are not flagged as bugs here, but the repository owner may want to clean them up separately.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // `from_timestamp_millis` floors toward negative infinity, so timestamps | ||
| // in (-1000, 0) correctly land in the second before the epoch instead of | ||
| // collapsing to epoch via integer-division truncation. | ||
| chrono::DateTime::from_timestamp_millis(timestamp_ms).map_or_else( |
There was a problem hiding this comment.
📝 Info: Behavioral change: sub-second precision is now preserved for all timestamps
The old code discarded sub-second information for all timestamps by dividing by 1000 and passing 0 as the nanosecond component to from_timestamp. The new from_timestamp_millis preserves millisecond precision internally. Since the format strings used (%Y-%m-%d and %Y-%m-%d %H:%M:%S %Z) only render down to whole seconds, there is no visible output difference for positive timestamps. However, for negative timestamps in the (-1000, 0) range, the old code truncated toward zero (landing on epoch), while the new code correctly floors toward negative infinity (landing in 1969). This is the intended fix, validated by the new format_datetime_small_negative_is_not_epoch test.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ng to epoch `format_timestamp` divided `timestamp_ms` by 1000 with integer division, which truncates toward zero. Timestamps in the (-1000, 0) ms range collapsed onto the Unix epoch instead of formatting as the preceding second. Use `chrono::DateTime::from_timestamp_millis`, which accepts millisecond timestamps directly and floors correctly. The now-unused `MS_TO_SECONDS` constant is removed. The existing `expected_local` test helper is updated to match so the positive-path tests still hold (same output for sub-second positive timestamps). Fixes#209. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
868c954 to
52db5b1Compare| fn expected_local(timestamp_ms: i64, fmt: &str) -> String { | ||
| chrono::DateTime::from_timestamp(timestamp_ms / MS_TO_SECONDS, 0) | ||
| chrono::DateTime::from_timestamp_millis(timestamp_ms) | ||
| .expect("valid timestamp") | ||
| .with_timezone(&Local) | ||
| .format(fmt) |
There was a problem hiding this comment.
📝 Info: Test helper is tautological with the implementation under test
The expected_local helper at src/utils/datetime.rs:28-34 now uses the exact same from_timestamp_millis call as the production format_timestamp function. This means tests like format_date_known_timestamp are effectively asserting that a function equals itself, providing no independent verification of correctness. This is a pre-existing pattern (the old code also mirrored the production logic), but the migration carried it forward. A stronger approach would use hardcoded expected strings for known timestamps in a fixed timezone, or at least use an independent computation path.
(Refers to lines 28-34)
Was this helpful? React with 👍 or 👎 to provide feedback.
Uh oh!
There was an error while loading. Please reload this page.
## Summary Patch release rolling up the seven fixes merged since 0.2.1: - fix(bugs): print empty-results hint when `--vulns` alone yields no matches (#264) - fix(datetime): floor sub-second negative timestamps instead of snapping to epoch (#262) - chore(deps): bump rustls-webpki to 0.103.13 for RUSTSEC-2026-0104 (#263) - fix(repos): normalize whitespace in repo identifiers before lookup (#261) - fix(git): parse GitHub remotes with embedded http(s) credentials (#260) - fix(auth): redirect browser and surface OAuth errors on PKCE callback failure (#258) - refactor(config): rewrite `update_config` through the locked handle (#257) On merge, the release workflow will tag `v0.2.2` and publish platform artifacts via cargo-dist. ## Test plan - [x] `cargo build` succeeds with version 0.2.2 - [ ] Tag `v0.2.2` is created on merge and release workflow publishes artifacts 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/usedetail/cli/pull/265" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review"> </picture> </a> <!-- devin-review-badge-end --> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
format_timestampconvertedtimestamp_ms → secondsby integer division, which truncates toward zero. Timestamps in the (-1000, 0) ms window therefore collapsed onto the Unix epoch (1970-01-01 00:00:00) instead of formatting as the preceding second (1969-12-31 23:59:59).Swap
chrono::DateTime::from_timestamp(timestamp_ms / MS_TO_SECONDS, 0)forchrono::DateTime::from_timestamp_millis(timestamp_ms), which takes millisecond timestamps directly and floors correctly. TheMS_TO_SECONDSconstant is no longer used.Test plan
cargo test --lib utils::datetime— 7 pass, including newformat_datetime_small_negative_is_not_epochregression testformat_datetime_rounds_down_sub_second) still pass —from_timestamp_millisproduces the same formatted output for those inputscargo clippy -- -D warningscleancargo fmt --checkcleanFixes#209.
🤖 Generated with Claude Code