feat(timezone): add timezone-aware session context#671
feat(timezone): add timezone-aware session context#671ilblackdragon wants to merge 9 commits intomainfrom
Conversation
All timestamps were UTC-only, causing daily logs to split at UTC midnight, cron schedules to fire in UTC, and no quiet hours for heartbeat. This adds timezone as a per-session property flowing from the client. Key changes: - New `src/timezone.rs` module with resolution chain, parsing, and detection - `IncomingMessage` carries optional timezone from client - `JobContext.user_timezone` flows timezone to tools - `next_cron_fire()` accepts timezone for schedule evaluation - `Trigger::Cron` stores optional timezone (backward-compatible) - Workspace gains `_tz` variants for daily logs and system prompt - Heartbeat supports quiet hours (`HEARTBEAT_QUIET_START/END`) - Web frontend sends `Intl.DateTimeFormat().resolvedOptions().timeZone` - REPL auto-detects system timezone - `DEFAULT_TIMEZONE` env var / settings for server-wide default Storage stays UTC. Conversion happens at display boundaries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
scope: agent
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's ability to handle timezones, moving from a UTC-centric approach to one that respects user and system-defined timezones across various functionalities. The core principle is to maintain UTC for storage while performing conversions at display boundaries and when interpreting user-provided times. This change improves the accuracy and relevance of time-sensitive operations, such as cron job scheduling, daily log entries, and system prompts, by aligning them with the local context of the user or system. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive timezone support across the application, integrating chrono-tz and iana-time-zone dependencies. Key changes include a new timezone module for resolving and parsing timezones, and propagating timezone information through IncomingMessages, JobContexts, and web/REPL channels. This enables timezone-aware daily logs in system prompts and memory, allows cron triggers to specify a timezone for schedule evaluation, and implements heartbeat quiet hours configurable with a specific timezone. The review comments point out that the RoutineUpdateTool's logic for cron triggers needs refinement to allow timezone-only updates and prevent unintended type changes, and that the user_timezone in JobContext is hardcoded to "UTC" when loaded from the database, suggesting it should be persisted for full timezone support in jobs.
src/tools/builtin/routine.rs
Outdated
| if let Some(schedule) = params.get("schedule").and_then(|v| v.as_str()) { | ||
| let timezone = params | ||
| .get("timezone") | ||
| .and_then(|v| v.as_str()) | ||
| .map(String::from) | ||
| .or_else(|| { | ||
| // Preserve existing timezone if only schedule is changing | ||
| if let Trigger::Cron { ref timezone, .. } = routine.trigger { | ||
| timezone.clone() | ||
| } else { | ||
| None | ||
| } | ||
| }); | ||
| // Validate | ||
| next_cron_fire(schedule) | ||
| next_cron_fire(schedule, timezone.as_deref()) | ||
| .map_err(|e| ToolError::InvalidParameters(format!("invalid cron schedule: {e}")))?; | ||
|
|
||
| routine.trigger = Trigger::Cron { | ||
| schedule: schedule.to_string(), | ||
| timezone: timezone.clone(), | ||
| }; | ||
| routine.next_fire_at = next_cron_fire(schedule).unwrap_or(None); | ||
| routine.next_fire_at = next_cron_fire(schedule, timezone.as_deref()).unwrap_or(None); | ||
| } |
There was a problem hiding this comment.
This logic for updating a cron routine is only triggered if the schedule parameter is provided. This prevents updating only the timezone. This block should be triggered if either schedule or timezone is provided.
Additionally, this logic implicitly changes the trigger type to Cron if it wasn't already, which might be unintended for an update tool.
Consider refactoring to handle updates to existing cron triggers more robustly and explicitly erroring if schedule or timezone is provided for a non-cron routine.
let new_schedule_opt = params.get("schedule").and_then(|v| v.as_str());
let new_timezone_opt = params.get("timezone").and_then(|v| v.as_str());
if new_schedule_opt.is_some() || new_timezone_opt.is_some() {
if let Trigger::Cron {
schedule: old_schedule,
timezone: old_timezone,
} = &routine.trigger
{
let schedule = new_schedule_opt.unwrap_or(old_schedule);
let timezone = new_timezone_opt.map(String::from).or_else(|| old_timezone.clone());
// Validate
next_cron_fire(schedule, timezone.as_deref())
.map_err(|e| ToolError::InvalidParameters(format!("invalid cron schedule: {e}")))?;
routine.trigger = Trigger::Cron {
schedule: schedule.to_string(),
timezone: timezone.clone(),
};
routine.next_fire_at = next_cron_fire(schedule, timezone.as_deref()).unwrap_or(None);
} else {
return Err(ToolError::InvalidParameters(
"Cannot update schedule or timezone on a non-cron routine.".to_string(),
));
}
}| tool_output_stash: std::sync::Arc::new(tokio::sync::RwLock::new( | ||
| std::collections::HashMap::new(), | ||
| )), | ||
| user_timezone: "UTC".to_string(), |
There was a problem hiding this comment.
When loading a job from the database, user_timezone is hardcoded to "UTC". This means that for persisted jobs, like those created by routines, the user's timezone context is lost. Tools that rely on this context (e.g., for daily logs) will incorrectly use UTC. To fully support timezones in jobs, the user_timezone should be persisted with the job data in the agent_jobs table and loaded here.
There was a problem hiding this comment.
Pull request overview
Adds end-to-end timezone awareness as a per-session context (client → channel → agent → tools), while keeping stored timestamps in UTC and applying timezone conversions at interpretation/display boundaries.
Changes:
- Introduces
src/timezone.rsfor timezone resolution/parsing, “now/today in TZ”, and system timezone detection. - Plumbs client-provided timezone through web + REPL channels into
IncomingMessage, resolves it in the agent, and carries it viaJobContext.user_timezonefor tools (daily log/system prompt). - Adds timezone support to cron routines (
Trigger::Cron.timezone,next_cron_fire(..., timezone)) and introduces heartbeat “quiet hours” config fields.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Adds chrono-tz + iana-time-zone dependencies. |
| Cargo.lock | Locks new transitive deps for timezone support. |
| src/lib.rs | Exposes the new timezone module. |
| src/timezone.rs | Implements timezone utilities + unit tests. |
| src/settings.rs | Adds default_timezone and heartbeat quiet-hours fields to settings. |
| src/config/agent.rs | Adds DEFAULT_TIMEZONE → AgentConfig.default_timezone. |
| src/config/heartbeat.rs | Adds env parsing for heartbeat quiet-hours fields. |
| src/channels/channel.rs | Adds IncomingMessage.timezone and a setter. |
| src/channels/repl.rs | Detects system TZ and attaches it to REPL messages. |
| src/channels/web/types.rs | Extends HTTP + WS request types to carry timezone in chat messages. |
| src/channels/web/ws.rs | Threads WS message timezone into IncomingMessage. |
| src/channels/web/server.rs | Threads HTTP chat-send timezone into IncomingMessage; updates cron trigger matching. |
| src/channels/web/handlers/routines.rs | Updates cron trigger match to ignore new timezone field. |
| src/channels/web/static/app.js | Sends browser-resolved IANA timezone in chat requests. |
| src/context/state.rs | Adds JobContext.user_timezone and a builder method. |
| src/agent/dispatcher.rs | Resolves per-message timezone and uses it for system prompt + job context. |
| src/agent/thread_ops.rs | Attempts to apply message timezone during tool-approval tool execution. |
| src/workspace/mod.rs | Adds _tz variants for daily log append + system prompt daily-log date selection. |
| src/tools/builtin/memory.rs | Uses JobContext.user_timezone to write daily logs with local date/time. |
| src/agent/routine.rs | Adds optional timezone to cron triggers; evaluates cron schedules in that TZ. |
| src/tools/builtin/routine.rs | Accepts optional timezone for cron routines; uses timezone-aware next_cron_fire. |
| src/agent/routine_engine.rs | Updates cron trigger match; computes next_fire_at with timezone. |
| src/agent/heartbeat.rs | Adds quiet-hours evaluation and skips heartbeat during quiet hours. |
| src/history/store.rs | Sets default user_timezone when hydrating JobContext from DB row. |
| src/db/libsql/jobs.rs | Sets default user_timezone when hydrating JobContext from DB row. |
| src/testing.rs | Updates test fixtures for new cron trigger field. |
| tests/e2e_routine_heartbeat.rs | Updates tests to populate new timezone/cron fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/config/heartbeat.rs
Outdated
| notify_user: optional_env("HEARTBEAT_NOTIFY_USER")? | ||
| .or_else(|| settings.heartbeat.notify_user.clone()), | ||
| quiet_hours_start: parse_option_env("HEARTBEAT_QUIET_START")?, | ||
| quiet_hours_end: parse_option_env("HEARTBEAT_QUIET_END")?, |
There was a problem hiding this comment.
quiet_hours_start/quiet_hours_end are parsed only from env vars here; values set in settings.heartbeat.quiet_hours_* are ignored when the env vars are unset. This makes the new settings fields ineffective unless users configure env vars. Consider resolving these fields with an env override that falls back to the corresponding settings values, consistent with the other heartbeat fields.
src/config/heartbeat.rs
Outdated
| quiet_hours_start: parse_option_env("HEARTBEAT_QUIET_START")?, | ||
| quiet_hours_end: parse_option_env("HEARTBEAT_QUIET_END")?, |
There was a problem hiding this comment.
HEARTBEAT_QUIET_START/HEARTBEAT_QUIET_END are documented as hours in the 0–23 range, but the current parsing accepts any u32 (e.g., 24) with no validation. This can lead to surprising quiet-hour behavior at runtime. Consider validating the parsed values are <= 23 and returning a ConfigError::InvalidValue otherwise.
| let timezone = params | ||
| .get("timezone") | ||
| .and_then(|v| v.as_str()) | ||
| .map(String::from); | ||
| // Validate cron expression | ||
| next_cron_fire(schedule).map_err(|e| { | ||
| next_cron_fire(schedule, timezone.as_deref()).map_err(|e| { | ||
| ToolError::InvalidParameters(format!("invalid cron schedule: {e}")) | ||
| })?; | ||
| Trigger::Cron { | ||
| schedule: schedule.to_string(), | ||
| timezone, | ||
| } |
There was a problem hiding this comment.
The tool accepts a timezone parameter but does not validate it; invalid IANA names will be stored on the routine while next_cron_fire() silently falls back to UTC. This can create routines that appear timezone-aware but actually run in UTC. Consider validating timezone with crate::timezone::parse_timezone (and rejecting invalid values) and updating the tool schema to advertise the timezone field.
| routine.next_fire_at = next_cron_fire(schedule).unwrap_or(None); | ||
| routine.next_fire_at = next_cron_fire(schedule, timezone.as_deref()).unwrap_or(None); | ||
| } | ||
|
|
There was a problem hiding this comment.
routine_update only reads and applies the timezone parameter inside the if schedule ... branch. As a result, callers cannot update a cron routine's timezone without also changing the schedule. Consider handling a standalone timezone update (and recomputing next_fire_at) when the trigger is cron.
| // Handle standalone timezone updates for existing cron triggers | |
| if params.get("schedule").is_none() { | |
| if let Some(tz) = params.get("timezone").and_then(|v| v.as_str()) { | |
| if let Trigger::Cron { schedule, timezone } = &mut routine.trigger { | |
| let tz_string = tz.to_string(); | |
| let next = next_cron_fire(schedule, Some(&tz_string)) | |
| .map_err(|e| ToolError::InvalidParameters(format!("invalid cron schedule: {e}")))?; | |
| *timezone = Some(tz_string); | |
| routine.next_fire_at = next; | |
| } | |
| } | |
| } |
| let tz = crate::timezone::parse_timezone(&ctx.user_timezone) | ||
| .unwrap_or(chrono_tz::Tz::UTC); | ||
| self.workspace | ||
| .append_daily_log(content) | ||
| .append_daily_log_tz(content, tz) | ||
| .await | ||
| .map_err(|e| ToolError::ExecutionFailed(format!("Write failed: {}", e)))?; | ||
| format!("daily/{}.md", chrono::Utc::now().format("%Y-%m-%d")) | ||
| let today = crate::timezone::today_in_tz(tz); | ||
| format!("daily/{}.md", today.format("%Y-%m-%d")) | ||
| } |
There was a problem hiding this comment.
This computes the daily-log path using a second Utc::now() call via today_in_tz(tz) after appending. If the date rolls over between the append and this calculation (especially around midnight in the target TZ), the tool could write to one file but report a different path. Consider deriving the returned path from the same now/today value used for the append (e.g., return the used date/path from append_daily_log_tz, or compute now once in the tool and reuse it).
| let mut job_ctx = | ||
| JobContext::with_user(&message.user_id, "chat", "Interactive chat session"); | ||
| job_ctx.http_interceptor = self.deps.http_interceptor.clone(); | ||
| if let Some(ref tz) = message.timezone { | ||
| job_ctx.user_timezone = tz.clone(); | ||
| } |
There was a problem hiding this comment.
In the tool-approval flow, the incoming approval message typically won’t carry a timezone (e.g., WebSocket approval messages have no timezone field). This means approved tools will run with the default "UTC" timezone even if the thread/session was established with a different timezone. Consider storing the resolved timezone in the thread/pending-approval state (or reusing the thread’s prior context) and using that when building job_ctx, rather than relying on message.timezone here.
| let mut job_ctx = | |
| JobContext::with_user(&message.user_id, "chat", "Interactive chat session"); | |
| job_ctx.http_interceptor = self.deps.http_interceptor.clone(); | |
| if let Some(ref tz) = message.timezone { | |
| job_ctx.user_timezone = tz.clone(); | |
| } | |
| // Reuse the original job context from the pending approval so that | |
| // the previously resolved timezone and other context are preserved. | |
| let mut job_ctx = pending.job_ctx.clone(); | |
| job_ctx.http_interceptor = self.deps.http_interceptor.clone(); |
| /// Append an entry to today's daily log. | ||
| /// | ||
| /// Daily logs are raw, append-only notes for the current day. | ||
| pub async fn append_daily_log(&self, entry: &str) -> Result<(), WorkspaceError> { | ||
| let today = Utc::now().date_naive(); | ||
| let path = format!("daily/{}.md", today.format("%Y-%m-%d")); | ||
| let timestamp = Utc::now().format("%H:%M:%S"); | ||
| let timestamped_entry = format!("[{}] {}", timestamp, entry); | ||
| self.append(&path, ×tamped_entry).await | ||
| } | ||
|
|
||
| /// Append an entry to today's daily log using the given timezone. | ||
| pub async fn append_daily_log_tz( | ||
| &self, | ||
| entry: &str, | ||
| tz: chrono_tz::Tz, | ||
| ) -> Result<(), WorkspaceError> { | ||
| let now = crate::timezone::now_in_tz(tz); | ||
| let today = now.date_naive(); | ||
| let path = format!("daily/{}.md", today.format("%Y-%m-%d")); | ||
| let timestamp = now.format("%H:%M:%S"); | ||
| let timestamped_entry = format!("[{}] {}", timestamp, entry); | ||
| self.append(&path, ×tamped_entry).await | ||
| } |
There was a problem hiding this comment.
append_daily_log_tz duplicates the logic from append_daily_log. To keep behavior consistent (and avoid subtle drift like using multiple Utc::now() calls), consider implementing append_daily_log in terms of append_daily_log_tz(entry, Tz::UTC) instead of maintaining two near-identical implementations.
| fn test_today_in_tz_returns_valid_date() { | ||
| let date = today_in_tz(Tz::UTC); | ||
| assert!(date.year() >= 2024); | ||
| } |
There was a problem hiding this comment.
This test asserts the current year is >= 2024, which makes it time-dependent and potentially flaky on systems with incorrect clocks (or when run in earlier years). Consider asserting a property that’s independent of the wall clock (e.g., that today_in_tz(Tz::UTC) returns a valid date and now_in_tz doesn’t panic), or use a fixed timestamp in the test instead of Utc::now().
Resolve conflicts in src/channels/channel.rs and tests/e2e_routine_heartbeat.rs by keeping both timezone and attachments fields from the WASM channel attachments PR. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Validate quiet hours values (0-23) in HeartbeatConfig::resolve() - Fall back to settings values when env vars are unset for quiet hours - Validate IANA timezone strings in routine_create/update with parse_timezone - Add timezone field to routine_create tool schema - Allow standalone timezone update on cron routines without changing schedule - Return path from append_daily_log_tz to avoid TOCTOU race at midnight - Delegate append_daily_log to append_daily_log_tz(entry, UTC) to avoid drift - Preserve timezone through approval flow via PendingApproval.user_timezone - Improve test_today_in_tz to not depend on hardcoded year - Add 3 regression tests for quiet hours config validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 29 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.claude/scheduled_tasks.lock
Outdated
| @@ -0,0 +1 @@ | |||
| {"sessionId":"da5a1fe1-7b4f-4b22-84f7-a905b589d0af","pid":72935,"acquiredAt":1772906795706} No newline at end of file | |||
There was a problem hiding this comment.
This looks like an editor/automation lock artifact (contains a sessionId/pid/timestamp) and doesn’t appear to be part of the product. It should be removed from the repo and added to .gitignore to avoid committing per-developer state (and potential metadata leakage).
| {"sessionId":"da5a1fe1-7b4f-4b22-84f7-a905b589d0af","pid":72935,"acquiredAt":1772906795706} |
| let user_tz = crate::timezone::resolve_timezone( | ||
| message.timezone.as_deref(), | ||
| None, // user setting lookup can be added later | ||
| &self.config.default_timezone, | ||
| ); |
There was a problem hiding this comment.
Timezone resolution currently skips the “user setting” step: resolve_timezone(..., None, ...). This contradicts the stated resolution chain (client → user setting → config default → UTC) and means a persisted user timezone will never be applied. Consider plumbing the user setting lookup into this call (or update the PR description/scope if intentionally deferred).
| tool_call_id: tc.id.clone(), | ||
| context_messages: context_messages.clone(), | ||
| deferred_tool_calls: tool_calls[approval_idx + 1..].to_vec(), | ||
| user_timezone: message.timezone.clone(), | ||
| }; |
There was a problem hiding this comment.
PendingApproval.user_timezone is populated from message.timezone, which is the raw client-provided value and may be None even when a fallback timezone (e.g., DEFAULT_TIMEZONE) was used for this session. This can cause approvals to execute tools under UTC unexpectedly if the approval message doesn’t include a timezone. Store the resolved effective timezone string here instead (the same one written to job_ctx.user_timezone).
src/agent/thread_ops.rs
Outdated
| tool_call_id: tc.id.clone(), | ||
| context_messages: context_messages.clone(), | ||
| deferred_tool_calls: deferred_tool_calls[approval_idx + 1..].to_vec(), | ||
| user_timezone: message.timezone.clone(), |
There was a problem hiding this comment.
Like in dispatcher.rs, PendingApproval.user_timezone is being set from the raw message.timezone. If the effective timezone came from a fallback (config default / future user setting), it will be lost across the approval flow and tool execution will fall back to UTC. Persist the resolved timezone used for the session/job context instead.
| user_timezone: message.timezone.clone(), | |
| user_timezone: resolved_timezone.clone(), |
| /// Check whether the current time falls within configured quiet hours. | ||
| pub fn is_quiet_hours(&self) -> bool { | ||
| use chrono::Timelike; | ||
| let (Some(start), Some(end)) = (self.quiet_hours_start, self.quiet_hours_end) else { | ||
| return false; | ||
| }; | ||
| let tz = self | ||
| .timezone | ||
| .as_deref() | ||
| .and_then(crate::timezone::parse_timezone) | ||
| .unwrap_or(chrono_tz::UTC); | ||
| let now_hour = crate::timezone::now_in_tz(tz).hour(); | ||
| if start <= end { | ||
| now_hour >= start && now_hour < end | ||
| } else { | ||
| // Wraps midnight, e.g. 22..06 | ||
| now_hour >= start || now_hour < end | ||
| } | ||
| } |
There was a problem hiding this comment.
Quiet-hours fields were added to the agent-side HeartbeatConfig, but the runner appears to be constructed from HeartbeatConfig::default().with_interval(...) in the main loop (i.e., quiet hours/timezone are never populated from crate::config::HeartbeatConfig). As a result, is_quiet_hours() will always behave as if quiet hours are disabled in production unless something else sets these fields. Wire the resolved config values into the agent heartbeat config (and decide which timezone to use) or remove these fields to avoid a false sense of support.
| let mut msg = IncomingMessage::new("gateway", &state.user_id, &req.content); | ||
| if let Some(ref tz) = req.timezone { | ||
| msg = msg.with_timezone(tz); | ||
| } |
There was a problem hiding this comment.
The PR description mentions accepting timezone via X-Timezone header, but this handler only reads timezone from the JSON body. If header support is intended for backward compatibility / non-browser clients, consider reading it here as an additional input (and define precedence vs body).
| WsClientMessage::Message { | ||
| content, | ||
| thread_id, | ||
| timezone, | ||
| } => { | ||
| let mut incoming = IncomingMessage::new("gateway", user_id, &content); | ||
| if let Some(ref tz) = timezone { | ||
| incoming = incoming.with_timezone(tz); | ||
| } |
There was a problem hiding this comment.
The PR description mentions accepting timezone via WebSocket query params/handshake, but timezone is only accepted inside WsClientMessage::Message. If handshake/query support is required, it should be parsed during connection setup and applied as a default when individual messages omit timezone.
- Remove .claude/scheduled_tasks.lock from repo and add to .gitignore - Store resolved timezone (not raw message.timezone) in PendingApproval - Carry forward user_timezone through chained approvals in thread_ops - Wire quiet_hours_start/end from config to HeartbeatRunner - Support X-Timezone header as fallback in chat_send_handler [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Keep both quiet hours wiring and notify config in agent_loop. Keep both quiet hours tests and store param test in heartbeat. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 30 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut config = AgentHeartbeatConfig::default() | ||
| .with_interval(std::time::Duration::from_secs(hb_config.interval_secs)); | ||
| config.quiet_hours_start = hb_config.quiet_hours_start; | ||
| config.quiet_hours_end = hb_config.quiet_hours_end; |
There was a problem hiding this comment.
Quiet hours are evaluated in HeartbeatConfig::is_quiet_hours() using self.config.timezone (falls back to UTC when unset). Here the agent loop only wires quiet_hours_start/end, so quiet hours will always be interpreted in UTC rather than the intended default/user timezone. Consider setting config.timezone (e.g., to self.config.default_timezone, or introducing a dedicated heartbeat timezone setting/env var) when spawning the heartbeat runner.
| config.quiet_hours_end = hb_config.quiet_hours_end; | |
| config.quiet_hours_end = hb_config.quiet_hours_end; | |
| // Ensure quiet hours are evaluated in the intended timezone | |
| // rather than always defaulting to UTC. Prefer the explicit | |
| // agent timezone, falling back to the agent's default timezone. | |
| if let Some(tz) = self | |
| .config | |
| .timezone | |
| .clone() | |
| .or_else(|| self.config.default_timezone.clone()) | |
| { | |
| config.timezone = Some(tz); | |
| } |
| // Validate timezone param if provided | ||
| let new_timezone = params | ||
| .get("timezone") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|tz| { |
There was a problem hiding this comment.
routine_update now reads/validates a timezone parameter, but the tool's parameters_schema() does not advertise a timezone field. This makes it unlikely that LLM/tool callers will ever send the field and can also break structured validation on the client side. Add timezone to the update tool schema properties (matching the create tool) and document the expected IANA format.
src/config/agent.rs
Outdated
| default_timezone: parse_optional_env( | ||
| "DEFAULT_TIMEZONE", | ||
| settings.agent.default_timezone.clone(), | ||
| )?, |
There was a problem hiding this comment.
DEFAULT_TIMEZONE is accepted as an arbitrary string here and only later gets validated implicitly (invalid values silently fall back to UTC in resolve_timezone). To avoid surprising behavior, consider validating the configured timezone at config load time (e.g., parse via crate::timezone::parse_timezone and return ConfigError::InvalidValue on failure).
src/agent/heartbeat.rs
Outdated
| #[test] | ||
| fn test_quiet_hours_inside() { | ||
| let config = HeartbeatConfig { | ||
| quiet_hours_start: Some(22), | ||
| quiet_hours_end: Some(6), |
There was a problem hiding this comment.
This test doesn't assert any expected behavior (it only calls is_quiet_hours() to ensure it doesn't panic), so it won't catch regressions in the quiet-hours logic. Consider refactoring is_quiet_hours() to accept an injectable "current hour" (or time source) so tests can assert true/false for specific times and wrap-around cases.
The time tool's "now" operation now returns local_iso and timezone fields based on ctx.user_timezone, so the LLM can report time in the user's timezone instead of always UTC. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 31 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/tools/builtin/time.rs
Outdated
| // Include local time in the user's timezone if set | ||
| if let Some(tz) = crate::timezone::parse_timezone(&ctx.user_timezone) { | ||
| let local = now.with_timezone(&tz); | ||
| result["local_iso"] = serde_json::Value::String(local.to_rfc3339()); | ||
| result["timezone"] = serde_json::Value::String(tz.name().to_string()); | ||
| } |
There was a problem hiding this comment.
The time now output only includes timezone/local_iso when ctx.user_timezone parses successfully. If ctx.user_timezone is ever invalid (e.g., from unvalidated channel input), consumers will get an inconsistent schema with the timezone fields missing. Consider always including a timezone field and falling back to UTC for local_iso when parsing fails, so callers can rely on stable output structure.
| let config = HeartbeatConfig { | ||
| quiet_hours_start: Some(22), | ||
| quiet_hours_end: Some(6), | ||
| timezone: Some("UTC".to_string()), | ||
| ..HeartbeatConfig::default() | ||
| }; | ||
| // We can't control the clock, so just verify the method doesn't panic | ||
| let _ = config.is_quiet_hours(); | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
The new quiet-hours tests don't verify the actual quiet-hours logic: test_quiet_hours_inside only checks for non-panicking behavior, so regressions in the hour-window calculation (including wrap-midnight cases) could slip through. Consider refactoring is_quiet_hours() to accept an injected DateTime/clock (or splitting out a pure function that takes now_hour) so unit tests can deterministically assert inside/outside behavior.
| let config = HeartbeatConfig { | |
| quiet_hours_start: Some(22), | |
| quiet_hours_end: Some(6), | |
| timezone: Some("UTC".to_string()), | |
| ..HeartbeatConfig::default() | |
| }; | |
| // We can't control the clock, so just verify the method doesn't panic | |
| let _ = config.is_quiet_hours(); | |
| } | |
| #[test] | |
| use chrono::{Timelike, Utc}; | |
| let now_utc = Utc::now(); | |
| let hour = now_utc.hour() as u8; | |
| let start = hour; | |
| let end = (hour + 1) % 24; | |
| let config = HeartbeatConfig { | |
| quiet_hours_start: Some(start), | |
| quiet_hours_end: Some(end), | |
| timezone: Some("UTC".to_string()), | |
| ..HeartbeatConfig::default() | |
| }; | |
| // The current UTC hour should be inside [start, end) by construction. | |
| assert!(config.is_quiet_hours()); | |
| } | |
| #[test] | |
| fn test_quiet_hours_outside_simple() { | |
| use chrono::{Timelike, Utc}; | |
| let now_utc = Utc::now(); | |
| let hour = now_utc.hour() as u8; | |
| let start = (hour + 1) % 24; | |
| let end = (hour + 2) % 24; | |
| let config = HeartbeatConfig { | |
| quiet_hours_start: Some(start), | |
| quiet_hours_end: Some(end), | |
| timezone: Some("UTC".to_string()), | |
| ..HeartbeatConfig::default() | |
| }; | |
| // The current UTC hour should be outside [start, end) by construction. | |
| assert!(!config.is_quiet_hours()); | |
| } | |
| #[test] | |
| fn test_quiet_hours_wrap_midnight_excludes_now() { | |
| use chrono::{Timelike, Utc}; | |
| let now_utc = Utc::now(); | |
| let hour = now_utc.hour() as u8; | |
| let start = (hour + 1) % 24; | |
| let end = hour; | |
| let config = HeartbeatConfig { | |
| quiet_hours_start: Some(start), | |
| quiet_hours_end: Some(end), | |
| timezone: Some("UTC".to_string()), | |
| ..HeartbeatConfig::default() | |
| }; | |
| // This wrap-midnight window [start, end) covers all hours except `hour`, | |
| // so the current UTC hour should be outside quiet hours. | |
| assert!(!config.is_quiet_hours()); | |
| } | |
| #[test] |
src/agent/thread_ops.rs
Outdated
| // Prefer timezone from the approval message, fall back to the | ||
| // timezone stored when the approval was originally requested. | ||
| if let Some(ref tz) = message.timezone.as_ref().or(pending.user_timezone.as_ref()) { | ||
| job_ctx.user_timezone = tz.to_string(); | ||
| } |
There was a problem hiding this comment.
This approval path copies message.timezone directly into job_ctx.user_timezone, and it takes precedence over the previously-resolved pending.user_timezone. Because IncomingMessage.timezone is unvalidated, an invalid timezone in the approval message can overwrite a valid stored timezone and cause downstream tools to silently fall back to UTC (or lose timezone info). Consider validating/normalizing the candidate timezone (e.g., via resolve_timezone(...) or parse_timezone with fallback to pending.user_timezone) before setting job_ctx.user_timezone.
| // Validate timezone param if provided | ||
| let new_timezone = params | ||
| .get("timezone") | ||
| .and_then(|v| v.as_str()) | ||
| .map(|tz| { | ||
| crate::timezone::parse_timezone(tz) | ||
| .map(|_| tz.to_string()) | ||
| .ok_or_else(|| { | ||
| ToolError::InvalidParameters(format!("invalid IANA timezone: '{tz}'")) | ||
| }) | ||
| }) | ||
| .transpose()?; |
There was a problem hiding this comment.
routine_update now accepts a timezone parameter (it is read and validated here), but the tool's parameters_schema() does not document/allow a timezone property. This mismatch can prevent callers that rely on the schema from ever sending timezone, and makes the tool contract inaccurate. Add the timezone field to the schema (similar to routine_create).
| let timezone = config | ||
| .get("timezone") | ||
| .and_then(|v| v.as_str()) | ||
| .map(String::from); | ||
| Ok(Trigger::Cron { schedule, timezone }) |
There was a problem hiding this comment.
Trigger::from_db() stores the timezone string without validating it as an IANA timezone. If the DB contains an invalid value (e.g., from older data, manual edits, or future migrations), the system will silently treat the cron as UTC later (because next_cron_fire ignores invalid timezones), which can be hard to debug. Consider validating here (and returning a RoutineError or coercing invalid values to None) so invalid stored timezones are surfaced early.
…stic tests, schema fixes - Validate DEFAULT_TIMEZONE and HEARTBEAT_TIMEZONE at config load time - Add timezone field to HeartbeatSettings and config::HeartbeatConfig - Wire heartbeat timezone from config through agent_loop to HeartbeatRunner - Add timezone to routine_update tool schema (was accepted but not advertised) - Error on schedule/timezone update for non-cron routines - Validate timezone in Trigger::from_db (coerce invalid to None with warning) - Validate timezone in approval path (thread_ops.rs) before overwriting - Time tool always includes timezone/local_iso fields (fallback to UTC) - Make quiet hours tests deterministic using current UTC hour - Add regression tests for config validation [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Closes #661
src/timezone.rs: resolution chain (client → user setting → config default → UTC), parsing, system detectionIncomingMessage.timezonecarries IANA timezone from channel clientsJobContext.user_timezoneflows timezone to tools (daily log, memory)next_cron_fire()evaluates cron schedules in the specified timezoneTrigger::Cronstores optional timezone (backward-compatible via#[serde(default)])_tzvariants for daily logs and system prompt date selectionHEARTBEAT_QUIET_START/HEARTBEAT_QUIET_ENDenv vars)Intl.DateTimeFormat().resolvedOptions().timeZonein chat requestsiana-time-zoneDEFAULT_TIMEZONEenv var and settings field for server-wide defaultKey principle: Storage stays UTC. Conversion happens at display boundaries and when interpreting user-provided times.
Test plan
cargo fmt— cleancargo clippy --all --all-features— zero warningscargo test— all pass (pre-existingwit_compatfailure unrelated)cargo check --no-default-features --features libsql— compilesDEFAULT_TIMEZONE=America/New_York, send message, verify daily log uses ET datenext_fire_atis correct UTCchat/sendPOST body🤖 Generated with Claude Code