Skip to content

[time] Cross-platform support for device timezones via tzlocal #925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

michaelfromyeg
Copy link
Contributor

@michaelfromyeg michaelfromyeg commented Mar 19, 2025

I tried to use the time server on Windows, and noticed a timezone error. Windows seems to report the timezone in a strange format...

# windows
>>> import datetime
>>> tzinfo = datetime.datetime.now().astimezone(tz=None).tzinfo
>>> tzinfo
datetime.timezone(datetime.timedelta(days=-1, seconds=61200), 'Pacific Daylight Time')

# linux
>>> import datetime
>>> tzinfo = datetime.datetime.now().astimezone(tz=None).tzinfo
>>> tzinfo
datetime.timezone(datetime.timedelta(days=-1, seconds=61200), 'PDT')

These kinds of values didn't play nice with ZoneInfo. This PR adds tzlocal to fix that.

One thing: I couldn't figure out how to test this reliably. Let me know if you'd like me to add tests.

cc @maledorak for viz!

edit: also #320 is probably worth another glance 👀

Description

Server Details

  • Server: time
  • Changes to: the get_local_tz helper

Motivation and Context

  • Fixes a bug when starting the server on windows (zoneinfo._common.ZoneInfoNotFoundError: 'No time zone found with key Pacific Daylight Time') via tzlocal

How Has This Been Tested?

  • Tested manually on Windows; working as expected

image

Breaking Changes

No

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

@maledorak
Copy link
Contributor

Hey @michaelfromyeg , thanks for cc :)
Sadly, I don't have a Windows machine, and I can't test this code on Windows :/

@michaelfromyeg
Copy link
Contributor Author

ah all good!

@olaservo olaservo added server-time Reference implementation for the Time MCP server - src/time bug Something isn't working labels Mar 27, 2025
@olaservo
Copy link
Member

Thanks for the PR! I'm wondering if we should try to combine this approach and the approach in #1272 to cover all our bases (I left a comment in that one).

@michaelfromyeg
Copy link
Contributor Author

I trust your judgement on which is best! Not sure how much fallback-ing is necessary... but better safe than sorry?

Candidly, tzlocal was what came up first when I searched up the issue (via Claude!).

@olaservo
Copy link
Member

@michaelfromyeg since your PR includes some doc formatting updates too and was created first, let's do this:

  • Could you add some tests to the existing tests to cover a few cases related to this fix?
  • Then we can merge this PR and the other PR can get updated to add any additional stuff not covered by this PR.

Copy link
Member

@olaservo olaservo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, was running the tests for this locally and ran into some errors related to McpError so I left a few suggestions to resolve those.

Also a suggestion for a test to add that would check for valid vs invalid overrides:


@pytest.mark.parametrize(
    "test_time,local_tz_override,expected_error",
    [
        # Test Windows format timezone override
        (
            "2024-01-01 12:00:00+00:00",
            "America/Los_Angeles",  # What Windows "Pacific Standard Time" should map to
            None,
        ),
        # Test direct IANA name
        (
            "2024-01-01 12:00:00+00:00", 
            "Europe/Berlin",  # What "Mitteleuropäische Zeit" should map to
            None,
        ),
        # Test invalid timezone
        (
            "2024-01-01 12:00:00+00:00",
            "Invalid/Timezone",
            "Invalid timezone: 'No time zone found with key Invalid/Timezone'",
        ),
    ],
)
def test_get_local_tz_override(test_time, local_tz_override, expected_error):
    """Test local timezone override functionality"""
    with freeze_time(test_time):
        if expected_error:
            with pytest.raises(McpError, match=expected_error):
                get_local_tz(local_tz_override)
        else:
            result = get_local_tz(local_tz_override)
            assert isinstance(result, ZoneInfo)
            assert str(result) == local_tz_override

Comment on lines 39 to 40
if local_tz_override:
return ZoneInfo(local_tz_override)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion to add try/catch if the local override results in an exception, plus it looks like McpError expects an object with a message and not a string:

Suggested change
if local_tz_override:
return ZoneInfo(local_tz_override)
if local_tz_override:
try:
return ZoneInfo(local_tz_override)
except Exception as e:
error_data = ErrorData(
code=INVALID_PARAMS,
message=f"Invalid timezone: {str(e)}"
)
raise McpError(error_data)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will also need to add ErrorData and INVALID_PARAMS fom mcp.types, for example:

from mcp.types import Tool, TextContent, ImageContent, EmbeddedResource, ErrorData, INVALID_PARAMS

tzinfo = datetime.now().astimezone(tz=None).tzinfo
if tzinfo is not None:
return ZoneInfo(str(tzinfo))

raise McpError("Could not determine local timezone - tzinfo is None")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise McpError("Could not determine local timezone - tzinfo is None")
error_data = ErrorData(
code=INVALID_PARAMS,
message="Could not determine local timezone - tzinfo is None"
)
raise McpError(error_data)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't leave comments or suggestions on unchanged lines, but I think you also want to change this in the get_zoneinfo function:

    except Exception as e:
        error_data = ErrorData(
            code=INVALID_PARAMS,
            message=f"Invalid timezone: {str(e)}"
        )
        raise McpError(error_data)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server-time Reference implementation for the Time MCP server - src/time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants