Skip to content

PR #62 follow-up 18: fix resolve_time_range parsing gaps - #70

Merged
VibeSWE merged 6 commits into
mainfrom
fix/pr62-18-resolve-time-range-parsing
Oct 25, 2026
Merged

VibeSWE merged 6 commits into
mainfrom
fix/pr62-18-resolve-time-range-parsing

Conversation

@VibeSWE

@VibeSWE VibeSWE commented Oct 24, 2026 •

Copy link
Copy Markdown

Stacked on #62 (upstream/tool-catalog-1831). Retarget to main after #62 merges.

Problem

resolve_time_range came in with the upstream tool-catalog port (parseablehq#1831). It had four parsing gaps:

  1. "now-1h to now" failed with invalid time duration: expected number. The now- prefix branch ran before the to split, and the split was also case-sensitive.
  2. "timezone": "utc" was rejected because parse::<Tz>() is case-sensitive. The pre-port code accepted utc case-insensitively, so existing callers silently regressed.
  3. today/yesterday returned local day boundary does not exist when local midnight falls in a DST gap (for example, America/Santiago every September).
  4. The explicit-range and fallback branches used Utc::now() rather than reference, so resolve_time_range_at wasn't deterministic for them.

Fix

  • Split on to (ASCII case-insensitive) first, before any prefix branch.
  • Map UTC (any case) to Tz::UTC. Other zones still go through IANA parsing.
  • For a missing local midnight, use the first instant after the gap: midnight read with the offset in effect before the transition (first_instant_after_gap).
  • Make TimeRange::parse_human_time_at pub(crate) and pass reference to it in both remaining branches. The RFC3339 branch became redundant with the fallback (end = "now" at reference), so it is folded into the fallback.
  • docs/tool-api.md now describes the accepted forms accurately.

Tests

New tests in src/tool_executor.rs:

  • time_resolution_parses_now_relative_explicit_ranges_against_reference: now-1h to now and now-1h TO now, plus the bare-duration fallback (1h), checked against a fixed 2020 reference.
  • time_resolution_accepts_utc_case_insensitively: utc, Utc and UTC.
  • time_resolution_starts_days_after_a_dst_gap_at_midnight: America/Santiago, 2026-09-06, for today, yesterday and the preceding day's end.

cargo test --lib time_resolution passes 5/5 locally.

Upstream fidelity

This is a small, local diff in upstream-owned code: one visibility change in src/utils/time.rs, plus branch reordering and one helper in resolve_time_range_at. It is a good candidate to propose upstream. The checked-arithmetic work on the same function (follow-ups 01–13) is separate.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

VibeSWE and others added 3 commits October 24, 2026 13:52
- Split `<start> to <end>` (case-insensitive) before the `now-` prefix
  branch, so "now-1h to now" resolves instead of failing.
- Accept `UTC` case-insensitively again, as the pre-port code did.
- When local midnight falls in a DST gap, start `today`/`yesterday` at
  the first instant after the gap instead of erroring.
- Evaluate explicit ranges and the fallback against `reference`, so
  resolve_time_range_at is deterministic in every branch.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… fix/pr62-18-resolve-time-range-parsing

# Conflicts:
#	src/tool_executor.rs
@VibeSWE
VibeSWE changed the base branch from upstream/tool-catalog-1831 to main October 24, 2026 23:25
@VibeSWE
VibeSWE merged commit 613d11c into main Oct 25, 2026
9 checks passed
@VibeSWE
VibeSWE deleted the fix/pr62-18-resolve-time-range-parsing branch October 25, 2026 00:32
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.

1 participant