feat(channel): add deterministic temporal context#239
Merged
jamiepine merged 9 commits intospacedriveapp:mainfrom Feb 27, 2026
Merged
feat(channel): add deterministic temporal context#239jamiepine merged 9 commits intospacedriveapp:mainfrom
jamiepine merged 9 commits intospacedriveapp:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-channel/worker Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel.rs`:
- Around line 665-676: The relative time is being calculated from
first_timestamp which makes later messages show as older; change the calculation
to compute seconds between the batch tail (the latest timestamp) and each
message timestamp instead. Replace the use of first_timestamp in the
relative_secs computation with the batch tail / last message timestamp (e.g.,
derive tail_timestamp from messages.last().timestamp or your existing
batch_tail_timestamp), so relative_secs =
tail_timestamp.signed_duration_since(message.timestamp).num_seconds().max(0) and
keep the existing relative_text formatting logic unchanged; update references to
message.timestamp, first_timestamp, and relative_secs accordingly in channel.rs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/content/docs/(configuration)/config.mdxprompts/en/branch.md.j2prompts/en/channel.md.j2src/agent/channel.rssrc/agent/status.rssrc/api/agents.rssrc/config.rs
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/agent/channel.rs (1)
2378-2385:⚠️ Potential issue | 🟡 MinorBatched formatter still renders blank payloads for attachment-only messages.
raw_textis passed through directly, so empty media-text messages produce a dangling colon with no placeholder. This is the same issue already noted previously and remains unresolved.🔧 Proposed fix
fn format_batched_user_message( display_name: &str, absolute_timestamp: &str, relative_text: &str, raw_text: &str, ) -> String { - format!("[{display_name}] ({absolute_timestamp}; {relative_text}): {raw_text}") + let text_content = if raw_text.trim().is_empty() { + "[attachment or empty message]" + } else { + raw_text + }; + format!("[{display_name}] ({absolute_timestamp}; {relative_text}): {text_content}") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 2378 - 2385, The batched formatter function format_batched_user_message currently inserts raw_text unconditionally, producing a dangling colon for attachment-only messages; update format_batched_user_message to detect when raw_text is empty or whitespace (use the raw_text parameter) and conditionally either omit the ": {raw_text}" segment or substitute a clear placeholder such as "<attachment-only>" so the output for attachment-only messages is well-formed; ensure the change preserves the existing display_name, absolute_timestamp, and relative_text formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel.rs`:
- Around line 119-125: The prompt text hard-coded in worker_task_preamble should
be moved to a markdown prompt file (e.g., prompts/worker_time_context.md) and
worker_task_preamble should load and render that template with the current
timestamps instead of returning a string constant. Replace the string literal in
fn worker_task_preamble(&self) with logic that reads the prompt template from
prompts/worker_time_context.md (or uses a prompt-loading helper if one exists),
substitute/format placeholders with self.format_timestamp(self.now_utc) and
self.now_utc.format(...) (or pass those values into the template renderer), and
return the rendered string; keep the function name worker_task_preamble and use
format_timestamp and self.now_utc as the substitution values. Ensure file read
errors are handled or surfaced consistently with existing prompt-loading
conventions.
- Around line 51-83: The from_runtime function currently picks user_timezone (or
cron_timezone only if user is None) and if a present user_timezone fails to
parse it falls back directly to SystemLocal; change the logic to try parsing
user_timezone first and if parsing fails (after logging the warning) then
attempt parsing cron_timezone before finally falling back to
TemporalTimezone::SystemLocal. Concretely, in from_runtime use
runtime_config.user_timezone.load() and runtime_config.cron_timezone.load()
separately, attempt timezone_name.parse::<Tz>() on user_timezone (returning
TemporalTimezone::Named on success), on Err log the existing warning and then
attempt parsing cron_timezone and return its TemporalTimezone::Named if it
parses, otherwise return the SystemLocal variant; keep existing now_utc,
TemporalTimezone::Named construction, and warning text/fields.
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 2378-2385: The batched formatter function
format_batched_user_message currently inserts raw_text unconditionally,
producing a dangling colon for attachment-only messages; update
format_batched_user_message to detect when raw_text is empty or whitespace (use
the raw_text parameter) and conditionally either omit the ": {raw_text}" segment
or substitute a clear placeholder such as "<attachment-only>" so the output for
attachment-only messages is well-formed; ensure the change preserves the
existing display_name, absolute_timestamp, and relative_text formatting.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/agent/channel.rs (2)
119-125: 🛠️ Refactor suggestion | 🟠 MajorMove worker time-context prompt text to
prompts/instead of embedding it in Rust.Line 121 embeds system prompt instruction text as a string constant. This should be template-driven from markdown prompt files for consistency and prompt governance.
As per coding guidelines "System prompts must live in prompts/ as markdown files; do not store prompts as string constants in Rust".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 119 - 125, The hard-coded time-context prompt string inside worker_task_preamble should be moved to a markdown template under prompts/ (e.g., worker_time_context.md) and worker_task_preamble should be changed to load that template and interpolate timestamps instead of embedding the text; use the existing format_timestamp(self.now_utc) and self.now_utc.format(...) values as replacements when rendering the template so worker_task_preamble returns the filled string, and ensure any file I/O or template-loading errors are handled or propagated by the same function.
51-57:⚠️ Potential issue | 🟠 MajorFallback chain skips
cron_timezonewhenuser_timezoneis present but invalid.At Line 52-Line 57,
or_elseselectscron_timezoneonly whenuser_timezoneisNone. Ifuser_timezoneexists but fails parse at Line 61, execution falls straight toSystemLocal(Line 80-Line 83), skipping cron fallback.🔧 Proposed fix
fn from_runtime(runtime_config: &crate::config::RuntimeConfig) -> Self { - let configured_timezone = runtime_config - .user_timezone - .load() - .as_ref() - .clone() - .or_else(|| runtime_config.cron_timezone.load().as_ref().clone()); let now_utc = Utc::now(); - if let Some(timezone_name) = configured_timezone { + if let Some(timezone_name) = runtime_config.user_timezone.load().as_ref().cloned() { match timezone_name.parse::<Tz>() { Ok(timezone) => { return Self { now_utc, timezone: TemporalTimezone::Named { timezone_name, timezone, }, }; } Err(_) => { tracing::warn!( timezone = %timezone_name, - "invalid runtime timezone for channel temporal context, falling back to system local" + "invalid user timezone for channel temporal context, falling back to cron timezone" ); } } } + if let Some(timezone_name) = runtime_config.cron_timezone.load().as_ref().cloned() { + match timezone_name.parse::<Tz>() { + Ok(timezone) => { + return Self { + now_utc, + timezone: TemporalTimezone::Named { + timezone_name, + timezone, + }, + }; + } + Err(_) => { + tracing::warn!( + timezone = %timezone_name, + "invalid cron timezone for channel temporal context, falling back to system local" + ); + } + } + } + Self { now_utc, timezone: TemporalTimezone::SystemLocal, } }Also applies to: 60-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 51 - 57, In from_runtime, the configured_timezone logic uses or_else on user_timezone causing cron_timezone to be skipped when user_timezone is present but invalid; modify from_runtime (and the configured_timezone variable logic) to attempt parsing user_timezone and if parsing fails or it's None then try cron_timezone, and only after both fail fallback to SystemLocal — i.e., explicitly check result of parsing user_timezone (not just presence), then parse cron_timezone as the next fallback before defaulting to SystemLocal.
🧹 Nitpick comments (1)
src/agent/channel.rs (1)
680-685: Useformatted_authorin batched formatting for parity with single-message formatting.At Line 680-Line 685 batched formatting bypasses
formatted_author, while single-message formatting prefers it at Line 2327-Line 2337. Aligning these paths avoids platform-specific display-name drift between single and batched turns.Also applies to: 686-691, 2327-2337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 680 - 685, Batched formatting currently builds display_name by directly reading message.metadata ("sender_display_name") and falling back to message.sender_id; update the batched path to call and use the existing formatted_author helper (the same used in single-message formatting) instead of manual metadata lookup so both batched and single-message flows use identical display-name logic—replace the code around display_name (and the similar block at 686-691) to invoke formatted_author with the message (or its sender id/metadata as its signature requires) and use that result for rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.rs`:
- Around line 1051-1061: resolve_cron_timezone currently returns None as soon as
the first non-empty candidate fails validation, which prevents
resolve_user_timezone from seeing a valid SPACEBOT_CRON_TIMEZONE fallback;
update resolve_cron_timezone so it iterates through all timezone candidates
(explicit config, defaults, then env SPACEBOT_CRON_TIMEZONE) validating each and
only returns Some(valid_tz) for the first valid candidate or None after all have
been tried, so resolve_user_timezone receives the correct resolved_cron_timezone
value.
---
Duplicate comments:
In `@src/agent/channel.rs`:
- Around line 119-125: The hard-coded time-context prompt string inside
worker_task_preamble should be moved to a markdown template under prompts/
(e.g., worker_time_context.md) and worker_task_preamble should be changed to
load that template and interpolate timestamps instead of embedding the text; use
the existing format_timestamp(self.now_utc) and self.now_utc.format(...) values
as replacements when rendering the template so worker_task_preamble returns the
filled string, and ensure any file I/O or template-loading errors are handled or
propagated by the same function.
- Around line 51-57: In from_runtime, the configured_timezone logic uses or_else
on user_timezone causing cron_timezone to be skipped when user_timezone is
present but invalid; modify from_runtime (and the configured_timezone variable
logic) to attempt parsing user_timezone and if parsing fails or it's None then
try cron_timezone, and only after both fail fallback to SystemLocal — i.e.,
explicitly check result of parsing user_timezone (not just presence), then parse
cron_timezone as the next fallback before defaulting to SystemLocal.
---
Nitpick comments:
In `@src/agent/channel.rs`:
- Around line 680-685: Batched formatting currently builds display_name by
directly reading message.metadata ("sender_display_name") and falling back to
message.sender_id; update the batched path to call and use the existing
formatted_author helper (the same used in single-message formatting) instead of
manual metadata lookup so both batched and single-message flows use identical
display-name logic—replace the code around display_name (and the similar block
at 686-691) to invoke formatted_author with the message (or its sender
id/metadata as its signature requires) and use that result for rendering.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/agent/channel.rs (1)
83-93: Consider logging when cron_timezone is also invalid.When
cron_timezonefails to parse, the code silently falls through toSystemLocal. Adding a debug/warn log here would aid troubleshooting configuration issues.🔧 Suggested enhancement
if let Some(timezone_name) = cron_timezone && let Ok(timezone) = timezone_name.parse::<Tz>() { return TemporalTimezone::Named { timezone_name, timezone, }; + } else if let Some(timezone_name) = cron_timezone { + tracing::debug!( + timezone = %timezone_name, + "invalid cron timezone, falling back to system local" + ); } TemporalTimezone::SystemLocal🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 83 - 93, When cron_timezone is present but fails to parse into Tz, the code currently falls through silently to TemporalTimezone::SystemLocal; update the branch handling cron_timezone in the function that returns TemporalTimezone so that if let Some(timezone_name) = cron_timezone && let Err(err) = timezone_name.parse::<Tz>() triggers a log (debug or warn) that includes timezone_name and the parse error; keep successful parses returning TemporalTimezone::Named { timezone_name, timezone } and only return TemporalTimezone::SystemLocal after logging the parse failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel.rs`:
- Around line 74-79: The warning text in the Err arm that currently logs
timezone = %timezone_name should be changed so it accurately describes the
fallback chain (it does not immediately fall back to system local but next
attempts cron_timezone and only then system local); update the tracing::warn
message in that Err branch to something like "invalid runtime timezone for
channel temporal context, will try cron_timezone then fall back to system local"
and include timezone_name and cron_timezone variables to make the subsequent
behavior clear (locate the tracing::warn call that references timezone_name in
the channel temporal context handling and adjust its message accordingly).
---
Nitpick comments:
In `@src/agent/channel.rs`:
- Around line 83-93: When cron_timezone is present but fails to parse into Tz,
the code currently falls through silently to TemporalTimezone::SystemLocal;
update the branch handling cron_timezone in the function that returns
TemporalTimezone so that if let Some(timezone_name) = cron_timezone && let
Err(err) = timezone_name.parse::<Tz>() triggers a log (debug or warn) that
includes timezone_name and the parse error; keep successful parses returning
TemporalTimezone::Named { timezone_name, timezone } and only return
TemporalTimezone::SystemLocal after logging the parse failure.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
prompts/en/fragments/system/worker_time_context.md.j2src/agent/channel.rssrc/prompts/engine.rssrc/prompts/text.rs
✅ Files skipped from review due to trivial changes (1)
- prompts/en/fragments/system/worker_time_context.md.j2
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/config.rs (1)
2239-2244: Warning message is misleading after the refactor.The code now correctly iterates through all candidates before falling back to system local, but the warning message still says "falling back to system local timezone". This is inaccurate when there are remaining candidates to try (e.g., env var after an invalid config value).
Consider aligning the message with
resolve_user_timezonewhich says "trying next fallback":✏️ Suggested fix
tracing::warn!( agent_id, cron_timezone = %timezone, - "invalid cron timezone configured, falling back to system local timezone" + "invalid cron timezone configured, trying next fallback" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.rs` around lines 2239 - 2244, The warning message logged by the tracing::warn! call (with fields agent_id and cron_timezone = %timezone) is now misleading because the code iterates remaining candidates before ultimately falling back; update the log text to match resolve_user_timezone's wording (e.g., "invalid cron timezone configured, trying next fallback") so it accurately reflects that the resolver will continue to the next candidate rather than immediately falling back to system local timezone; keep the same structured fields (agent_id, cron_timezone) and only change the message string.src/agent/channel.rs (1)
800-805: Consider a small helper to remove duplicated temporal status rendering.
TemporalContext::from_runtime(...)+current_time_line()+render_with_time_context(...)repeats in multiple methods; extracting one helper would reduce drift risk.Also applies to: 1093-1098, 1763-1766
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 800 - 805, The repeated pattern creating a TemporalContext from a runtime, grabbing current_time_line, then rendering the status with that time is duplicated (TemporalContext::from_runtime(...), current_time_line(), status.render_with_time_context(...)), so add a small helper method (e.g., render_status_with_runtime_time or Status::render_with_runtime_time) that takes &self (or &Runtime/rc) and returns the rendered status string by performing self.state.status_block.read().await and calling render_with_time_context with the current_time_line computed via TemporalContext::from_runtime(rc.as_ref()). Replace the duplicated blocks in the spots using this pattern (including where the snippet appears and at the other noted locations) with calls to the new helper to centralize the logic and reduce drift risk.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/agent/channel.rs`:
- Around line 800-805: The repeated pattern creating a TemporalContext from a
runtime, grabbing current_time_line, then rendering the status with that time is
duplicated (TemporalContext::from_runtime(...), current_time_line(),
status.render_with_time_context(...)), so add a small helper method (e.g.,
render_status_with_runtime_time or Status::render_with_runtime_time) that takes
&self (or &Runtime/rc) and returns the rendered status string by performing
self.state.status_block.read().await and calling render_with_time_context with
the current_time_line computed via TemporalContext::from_runtime(rc.as_ref()).
Replace the duplicated blocks in the spots using this pattern (including where
the snippet appears and at the other noted locations) with calls to the new
helper to centralize the logic and reduce drift risk.
In `@src/config.rs`:
- Around line 2239-2244: The warning message logged by the tracing::warn! call
(with fields agent_id and cron_timezone = %timezone) is now misleading because
the code iterates remaining candidates before ultimately falling back; update
the log text to match resolve_user_timezone's wording (e.g., "invalid cron
timezone configured, trying next fallback") so it accurately reflects that the
resolver will continue to the next candidate rather than immediately falling
back to system local timezone; keep the same structured fields (agent_id,
cron_timezone) and only change the message string.
jamiepine
approved these changes
Feb 27, 2026
This was referenced Feb 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
user_timezoneconfig (defaults + per-agent + env + hot reload) to control channel/worker temporal grounding independently from cron scheduling.user_timezoneexamples and explicit precedence.Testing
cargo fmt --all --checkcargo clippy --all-targets -- -D warningscargo checkcargo test --libcargo test --lib test_user_timezone_resolution_precedencecargo test --lib test_user_timezone_falls_back_to_cron_timezonecargo test --lib test_user_timezone_invalid_falls_back_to_cron_timezonecargo test --lib format_user_message_handles_empty_textcargo test --lib format_batched_message_includes_absolute_and_relative_timecargo test --lib worker_task_temporal_context_preamble_includes_absolute_datescargo test --lib render_with_time_context_renders_current_time_when_emptycargo testcurrently fails locally only ontests/bulletin.rsbecause those tests require real~/.spacebotruntime data/credentials; CI should validate in the intended environment.Note
This PR introduces deterministic temporal grounding to Spacebot by adding first-class
user_timezoneconfiguration. Every channel now receives a live current date/time context line in its status block, anchoring relative time references (today/tomorrow/now) to a specific timezone. Worker tasks are prefixed with absolute date/time context to ensure delegated work interprets time-sensitive language consistently. Configuration supports per-agent overrides, environment variables, and hot-reload viaArcSwap. All user-facing messages now include absolute timestamps alongside relative hints, improving clarity in multi-user scenarios. The resolution precedence is agent → defaults → env → fallback to cron timezone → system local.Written by Tembo for commit c1cd228. This will update automatically on new commits.