Skip to content

fix(time): flag ambiguous DST fall-back times in convert_time - #4781

Open
Nimra3261 wants to merge 1 commit into
modelcontextprotocol:mainfrom
Nimra3261:fix-time-ambiguous-dst-note
Open

fix(time): flag ambiguous DST fall-back times in convert_time#4781
Nimra3261 wants to merge 1 commit into
modelcontextprotocol:mainfrom
Nimra3261:fix-time-ambiguous-dst-note

Conversation

@Nimra3261

Copy link
Copy Markdown

Description

convert_time constructs the source datetime by attaching tzinfo directly to a naive year/month/day/hour/minute tuple. On the day a DST fall-back transition happens, the local wall-clock hour that repeats (e.g. 1:30 AM in America/New_York on the first Sunday of November) is ambiguous: two real, distinct moments share that same clock reading. Attaching tzinfo this way always resolves to fold=0, the earlier of the two, with nothing in the response indicating a choice was made.

Server Details

  • Server: time
  • Changes to: convert_time tool (adds an optional note field to TimeResult)

Motivation and Context

Reproduced directly:

>>> datetime(2024, 11, 3, 1, 30, tzinfo=ZoneInfo("America/New_York"))
datetime.datetime(2024, 11, 3, 1, 30, tzinfo=zoneinfo.ZoneInfo(key='America/New_York'))
# .utcoffset() is -04:00 (EDT) -- silently picks the pre-transition offset,
# with no way for the caller to know 01:30 also occurred an hour later at -05:00 (EST).

convert_time returns this as a confident, singular answer even though the requested wall-clock time was genuinely ambiguous. This adds a note field to TimeResult, populated only when the requested source time is ambiguous, explaining which of the two occurrences was used -- so a caller (or the LLM relying on this tool) can see that the answer isn't the only valid one.

Scoped relationship to #4719: I searched existing issues/PRs first. #4719 already fixes the nonexistent local time case (spring-forward gap, e.g. 2:30 AM on a day clocks jump 2:00→3:00) -- a different, more severe bug (that case has no valid answer at all, vs. this case having two valid answers). I confirmed the two PRs touch the same function but don't overlap in the specific lines/behavior changed. Happy to rebase on top of #4719 once it lands, or for a maintainer to ask me to fold this into that PR instead if preferred.

How Has This Been Tested?

New tests cover: an ambiguous NY time, a non-ambiguous time on the same transition day (note must stay unset), a timezone with no transition on that date (confirms the check is genuinely timezone-specific, not date-triggered), and a second timezone/month (Europe/Warsaw, October) to confirm the fix isn't hardcoded to the US transition rules.

Breaking Changes

None. note is a new optional field (defaults to None); existing consumers reading the other fields are unaffected.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Additional context

Detection: compare the tzinfo-attached datetime's UTC offset against the same wall-clock reading under fold=1; a mismatch means two valid offsets exist for that local time. This only fires for the source time (the one built from raw user input); target_time is always derived via .astimezone(), which is unambiguous by construction.

convert_time constructs the source datetime by attaching tzinfo directly
to a naive year/month/day/hour/minute tuple. On the day a DST fall-back
transition happens, the local wall-clock hour that repeats (e.g. 1:30 AM
in America/New_York on the first Sunday of November) is ambiguous: two
real, distinct moments share that same clock reading. Attaching tzinfo
this way always resolves to fold=0, the earlier of the two, with nothing
in the response indicating a choice was made.

This adds a `note` field to TimeResult, populated only when the requested
source time is ambiguous, explaining which of the two occurrences was
used. Detection compares the tzinfo-attached datetime's UTC offset against
the same wall-clock time under fold=1; a mismatch means two valid offsets
exist for that reading.

This is scoped to the ambiguous (fall-back) case only, distinct from and
complementary to modelcontextprotocol#4719, which addresses the separate nonexistent
(spring-forward gap) case -- confirmed via search that the two PRs don't
overlap in what they change.

4 new tests covering: an ambiguous NY time, a non-ambiguous time on the
same transition day, a timezone with no such transition on that date (to
confirm the check is genuinely timezone-specific, not date-triggered),
and a second timezone/month (Europe/Warsaw, October) to confirm the fix
isn't hardcoded to one region's transition rules. All 42 existing plus
new tests pass; pyright clean.
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