Fix missing timezone in query

[wojnilowicz]

fixes #95

---

Important

Adds --timezone option to query command in aw_client/cli.py to specify timezone for start and stop times, defaulting to system timezone.

• Behavior:
  • Adds --timezone option to query command in aw_client/cli.py to specify timezone for start and stop.
  • Defaults to system timezone if not specified.
• Imports:
  • Adds ZoneInfo import in aw_client/cli.py.
• Functionality:
  • Adjusts start and stop times to specified timezone using ZoneInfo in query() function.

^{This description was created by for fe3996d. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to fe3996d in 2 minutes and 3 seconds. Click for details.

• Reviewed 40 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. aw_client/cli.py:106

• Draft comment:
  Default timezone value (time.tzname[0]) might not always yield a valid IANA identifier. Consider using a standard default like 'UTC' or validating the provided string.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 35% vs. threshold = 50% The comment is about a line that was added in the diff (line 106 is part of the new --timezone option). The concern is technically valid - time.tzname[0] can return abbreviations that might not work with ZoneInfo. However, I need to consider: 1) Is this a definite bug or just a potential issue? 2) Is the suggestion actionable and clear? The comment does provide a concrete suggestion (use 'UTC' instead), which is actionable. But the issue is that using the user's local timezone as default might be intentional behavior, and switching to UTC would change that behavior. The comment is somewhat speculative ("might not always") rather than pointing to a definite bug. The comment is somewhat speculative - it says "might not always" rather than definitively identifying a bug. The author may have tested that time.tzname[0] works in their environment, and changing to UTC would alter the intended behavior of defaulting to the user's local timezone. Without knowing if this actually causes failures, this could be premature optimization. While the comment is somewhat speculative, it does identify a real compatibility issue - time.tzname[0] returns abbreviations that ZoneInfo may not accept, which could cause runtime errors when ZoneInfo(timezone) is called on line 122. This is a legitimate technical concern about code that was added in this PR. However, the suggested fix (using 'UTC') changes the intended behavior rather than fixing the underlying issue. The comment identifies a valid technical issue with the new code, but the suggested fix changes the intended behavior. A better suggestion would be to use a more robust way to get the local timezone. Given the rules about only keeping comments with strong evidence and clear actionability, and that this is somewhat speculative ("might not always"), I should lean toward deleting it.

2. aw_client/cli.py:122

• Draft comment:
  Wrap ZoneInfo(timezone) in try/except to handle invalid timezone strings gracefully, providing a user-friendly error message.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 45% vs. threshold = 50% The comment is about line 122 which is new code in the diff. It's suggesting error handling for user input, which is a reasonable code quality suggestion. However, I need to consider: 1) Is this "obvious"? Error handling for user input is a common practice but not always obvious where to apply it. 2) Is this actionable? Yes, it's clear what needs to be done. 3) Does it require a code change? Yes, adding try/except is a code change. 4) Is it speculative? The comment says "to handle invalid timezone strings" - ZoneInfo will indeed raise an exception for invalid timezones, so this is not speculative. This seems like a valid, actionable suggestion for improving error handling on newly added code. This could be considered an "obvious" best practice that the developer should know. Also, many CLI tools let exceptions bubble up naturally, and Click might handle this gracefully already. The comment doesn't demonstrate that there's actually a problem with the current implementation - it's more of a "nice to have" suggestion rather than identifying a bug. While error handling is a best practice, the rules state not to make comments that are "obvious or unimportant." Whether to add try/except for every possible exception is somewhat subjective. The current code might be acceptable as-is, letting Python's natural exception handling work. Without strong evidence that this is a critical issue (not just a suggestion), this falls into the category of a code quality suggestion that may not meet the bar of being clearly necessary. This is a reasonable code quality suggestion about error handling for user input in newly added code. However, it's borderline whether this meets the threshold of being clearly necessary versus being an obvious best practice. Given the instruction to err on the side of deleting comments unless there's STRONG EVIDENCE they're correct and necessary, and that this is more of a defensive programming suggestion than identifying a clear bug, I'll give it a moderate score.

3. aw_client/cli.py:122

• Draft comment:
  Note that if start/stop is naive, astimezone() will treat it as local time before converting. If the intent is to assume the provided timezone, consider explicitly setting tzinfo (e.g., using replace) to avoid unintended conversions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This comment is about the new timezone handling code added in the diff. The concern is technically valid - astimezone() on a naive datetime does treat it as local time first. However, the comment is speculative ("if start/stop is naive") and asks the author to "consider" an alternative approach rather than clearly stating a bug. It's also not clear what the "intent" should be - the comment assumes there might be unintended behavior but doesn't definitively say there is. This violates the rule about not making speculative comments ("If X, then Y is an issue" - only comment if it's definitely an issue). The comment also doesn't provide strong evidence that the current implementation is wrong, just that there's a potential edge case to consider. The datetime objects from click.DateTime() are indeed likely to be naive, so this isn't purely speculative - it's a real behavior that will occur. The comment might be highlighting a genuine bug where the timezone conversion won't work as intended if the user provides naive datetimes. While click.DateTime() likely produces naive datetimes, the comment doesn't definitively prove the current behavior is wrong. It's possible the author intended for naive datetimes to be treated as local time before conversion. Without clear evidence this is a bug rather than intended behavior, this falls into the category of asking the author to "ensure the behavior is intended" which the rules explicitly say not to do. This comment is speculative and suggests the author "consider" an alternative without proving the current implementation is definitely wrong. It violates the rule against speculative comments and asking authors to verify their intentions. Should be deleted.

Workflow ID: wflow_ohdYekhZ85kI9F1x

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[greptile-apps]

Greptile Summary

This PR adds timezone support to the aw-client query command to fix issue #95 where datetime strings without timezone information caused ValueError: start/stop needs to have a timezone set.

Changes:

• Added --timezone option to query command with default of time.tzname[0]
• Imports time and ZoneInfo modules
• Converts naive start and stop datetimes to timezone-aware using astimezone(zone_info)

Critical Issues Found:

• The implementation calls astimezone(zone_info) on naive datetime objects, which will raise ValueError. The correct approach is to use replace(tzinfo=zone_info) to attach timezone information to naive datetimes.
• The default timezone time.tzname[0] returns abbreviated names like 'PST' or 'CET', which may not be valid IANA timezone identifiers that ZoneInfo requires, potentially causing runtime errors.

The PR attempts to solve the right problem but the implementation will not work as written.

Confidence Score: 0/5

• This PR contains critical bugs that will cause runtime errors
• Score of 0 reflects two critical logical errors that will cause the code to fail at runtime: (1) calling astimezone() with a parameter on naive datetimes raises ValueError, and (2) using time.tzname[0] as default may provide invalid IANA timezone identifiers for ZoneInfo. These are not edge cases but will occur in normal usage.
• aw_client/cli.py requires immediate attention - the timezone conversion logic must be fixed before merging

Important Files Changed

Filename	Overview
aw_client/cli.py	Added timezone handling to query command, but implementation has critical bugs with naive datetime conversion and invalid timezone default

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as CLI (cli.py)
    participant Click as click.DateTime()
    participant ZoneInfo
    participant Client as ActivityWatchClient
    
    User->>CLI: aw-client query --start '2025-08-25 12:36:00' --timezone 'CET' query.txt
    CLI->>Click: Parse start/stop datetime strings
    Click-->>CLI: Returns naive datetime objects (no tzinfo)
    CLI->>ZoneInfo: Create ZoneInfo('CET')
    ZoneInfo-->>CLI: Returns timezone object
    CLI->>CLI: start.astimezone(zone_info)
    Note over CLI: BUG: Calling astimezone(tz) on naive datetime
    CLI--xUser: ValueError: astimezone() cannot be applied to a naive datetime
    
    Note over CLI,Client: Expected flow (after fix):
    CLI->>CLI: start.replace(tzinfo=zone_info)
    CLI->>CLI: stop.replace(tzinfo=zone_info)
    CLI->>Client: query(query, [(start, stop)])
    Client->>Client: Validate timezone-aware datetimes
    Client-->>CLI: Query results
    CLI-->>User: Display results

Loading

[greptile-apps]

_{1 file reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[ErikBjare]

This would override the default UTC timezone of the defaults or any start or stop ISO-8601 strings with explicit UTC offset (which ends with Z or +HH:MM).

Maybe only allow overriding zone_info if timezone is explicitly unset? (change the default params to use timezone-naive datetimes, which we assume should be in local time)

[wojnilowicz]

I changed the code to not override the time zone of the default start and stop arguments, but AFAIK setting Z or +HH:MM is not possible for those arguments according to the documentation. Are you able to do it?