Repository navigation
fix: cap the refresh interval read from settings.json at a year - #323
Conversation
A hand-edited refreshIntervalSeconds too long for chrono panicked the dashboard's tick (TimeDelta::seconds out of bounds) and its next-refresh countdown on every start. Read it capped at a year, as the provider backoffs cap Retry-After; the file keeps its value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@sfl review |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 1
Confidence: 9/10
Review done by claude-opus-5-5 (same family: same-family: codex gated: codex Weekly 165% SUS 61 🔴 pause, defer)
Settings::refresh_interval_secs now caps positive refreshIntervalSeconds values at MAX_REFRESH_INTERVAL_SECS (one year) and leaves the stored value untouched. A headless UI test and a store test come with it. Every reader of the value now feeds chrono::Duration::seconds a value inside its range. I found no evidenced Critical, High or Medium defect in the diff or its callers.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 1m 9s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
crates/codexbar-store/src/settings.rscrates/codexbar-app/src/ui_tests.rs
- Context read:
crates/codexbar-app/src/dashboard.rscrates/codexbar-app/src/settings_view.rscrates/codexbar-core/src/format.rscrates/codexbar-providers/src/claude.rscrates/codexbar-providers/src/copilot.rscrates/codexbar-store/Cargo.tomlAGENTS.md
- Verified:
- Compare
base_commit.shaisd9e38de394bf5a9c8c7ad4403b04bda143743a3aand the last commit is538ea267ed02a4efaf0be53fb2e83d8caa70c096(ahead 1, behind 0) - PR head and base SHAs match the reserved SHAs
- Only two Rust callers read the interval:
dashboard.rs:381-386(tick due check) anddashboard.rs:2527-2532(status countdown). Both useDuration::seconds(secs as i64), and a year in seconds is well inside chrono's range settings_view.rs:100-105current_minutesturns a year into 525600 minutes, which matches the PR's description of the dropdownset_refresh_interval_secsis unchanged; dropdown values are minutes times 60, far belowi64::MAX- The
<= 0and missing (120) branches are unchanged;unsigned_absonly runs on positive values, so it equals the old cast - The one-year ceiling matches
copilot.rs:266(365 * 24 * 3600) andclaude.rs:42(RATE_LIMIT_BACKOFF_MAX_DAYS = 365) format::clock_countdowntakes anyDurationwithout panickingcodexbar-storedepends onchrono, and the test module'sTempDir::with/readandjson!helpers exist- Comparing the stored
9000000000000000000withjson!(…i64)is sound because serde_json keeps both as a positive integer
- Compare
- Not verified:
- Runtime behavior and the PR's claimed test, clippy and fmt results, because execution is forbidden
- Large values
as_i64cannot read (abovei64::MAXor floats) still fall back to 120. That behavior predates this change and the diff does not touch it - GitHub code search may be incomplete; Rust callers were confirmed by grepping the downloaded exact-SHA files
- CI status on the head was not checked
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 538ea267ed02a4efaf0be53fb2e83d8caa70c096 • SFL v0
Summary
Settings::refresh_interval_secsnow caps a positiverefreshIntervalSecondsat one year (MAX_REFRESH_INTERVAL_SECS), the same ceiling the Claude and Copilot backoffs put on Retry-After. Every value the dashboard gets back now fits chrono'sDuration::secondsandDateTime + Duration, so a hand-edited interval can no longer crash the once-a-second tick or the status line's "next refresh" countdown. The cap applies only when the value is read.settings.jsonkeeps the number it holds, and saving another setting leaves it alone. No format, schema or migration change.Acceptance Criteria
i64, includingi64::MAX, opens, ticks and renders without panicking:a_refresh_interval_too_long_for_a_date_waits_instead_of_panicking(UI,9000000000000000000) andrefresh_interval_past_a_year_reads_as_a_year_and_stays_in_the_file(store,i64::MAX).3600,year,year + 1andi64::MAX.0, negative and missing keep their meaning (off, off, 120 s): the unchangedrefresh_interval_off_and_defaultand the unchanged<= 0branch.9000000000000000000back from disk.Definition of Done
main's store it failed withTimeDelta::seconds out of bounds(chronolib.rs:717); with the fix it passes.Validation
cargo test -p codexbar-app --locked a_refresh_interval_too_long_for_a_datewith the store change reverted: FAILED,panicked at chrono-0.4.45\src\lib.rs:717:17: TimeDelta::seconds out of bounds.cargo fmt --all --check: pass (aftercargo fmt --all).cargo clippy --workspace --all-targets --locked -- -D warnings: pass.cargo check -p codexbar-app --bin codexbar --locked: pass.cargo test --workspace --locked: pass, 1111 passed, 0 failed across 10 test binaries; the pre-commit hook passed again on commit 538ea26.No visible UI changes for real settings: intervals up to a year read as before, and the only new test is headless.
Risks and deferred work
A hand-edited interval longer than a year now refreshes once a year instead of crashing. The Settings dropdown shows it as its own option ("Every 525600 minutes"), as it already does for any non-standard value.
Closes #322
🤖 Generated with Claude Code