diff --git a/CLAUDE.md b/CLAUDE.md index 8d07b13..149c034 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -16,7 +16,7 @@ The design lives in **`blueprint/project-family.md`**. Read it before any archit Load-bearing decisions from that document: - **Not upstreamable.** Nothing here needs to preserve Skald's schema or be portable back to it. -- **Greenfield.** No users in production ⇒ **no migrations, no backwards compatibility**. Tables get restructured, renamed and moved freely; the schema collapses into a single clean baseline v1. +- **~~Greenfield~~ — no longer true. The instance is in production.** There are live users with data we cannot recreate, so the greenfield licence (restructure, rename, wipe, recreate) has expired: **every schema change now needs a versioning mechanism**, and "drop the box and re-run setup" stopped being an acceptable answer. Until that mechanism exists, the only safe change is an additive one through `db::ensure_column` (see the DB section); anything that renames, drops, retypes or moves a column or table is **blocked** on building schema versioning first, not something to do carefully by hand. A user's `{userid}.db` is SQLCipher-encrypted and readable **only while they are logged in**, so a migration cannot be a boot-time sweep over every file — it has to run per user, at unlock, and be idempotent. Design for that when the time comes. - **Dual memory**: a private per-user pool plus a shared pool. A user's private space is encrypted so that nobody else — the admin included — can read it *through normal use of the system*. Never claim "mathematically impossible": the honest promise is transparency plus verifiability (§3). - **Threat model** (§2): the adversary is the **tempted admin**, who owns the box but does not recompile the binary or dump RAM. Do not design against a forensic attacker. - **Roles are data, not enums** (§0.1): a `roles` table binds permission-group, run-context and data-handling attributes. "Children" is a seeded preset row, never a hardcoded type. @@ -132,7 +132,7 @@ The schema is split into two buckets (§5.1), and the split is the point: - **`create_registry_tables`** — instance-wide, readable without any user key: `users`, `roles`, `llm_providers`, `llm_models`, `transcribe_models`, `tts_models`, `image_generate_models`, `plugins`, `plugin_access` + `plugin_user_configs`, `approval_rules`, `tool_permission_groups`, `config`, `known_tools`, `llm_requests`, `mcp_catalog`, `mcp_global_servers` + `mcp_global_access`, `oauth_providers`, `role_capabilities`, `shared_folders` + `shared_folder_members`, `projects` + `project_members`, `supervision`, `system_agent_coverage`. The MCP tables back the Connectors model (§7/§14/§15 — see its own section); `oauth_providers` (accessor `db/oauth_providers.rs`) holds one row per identity provider (Google…) — endpoints + `client_id`/`client_secret` + `redirect_uri`, admin-owned household secrets (§4/§15b), never a per-user token. The last two pairs are junction-backed membership: `shared_folder_members` (accessor `db/shared_folders.rs`) for the on-disk shared folders (§6), `project_members` (accessor `db/project_members.rs`) for projects (see the Projects section) — both let a member be read-only (`can_write`) and both drive the container mount topology + the fs routing. Their FKs are registry→registry (same file), which is allowed — unlike an owner→registry key. - **`create_owner_tables`** — one owner's content, **identical schema in every file that has it**: `chat_sessions`, `chat_sessions_stack`, `chat_history`, `chat_llm_tools`, `chat_summaries`, `session_scratchpad`, `session_mcp_grants`, `stack_mcp_grants`, `scheduled_jobs`, `job_runs`, `system_agent_runs`, `system_agent_state`, `mcp_user_servers`, `mcp_events`, `sources`, `secrets`, `llm_request_payloads`, `memory_docs` (+ FTS5 `memory_docs_fts`), `reports`. `mcp_user_servers` (a user's activated per-user connectors) carries `catalog_name` as a **bare `TEXT` snapshot** of `mcp_catalog.name`, never a FK — an owner→registry key would fail every INSERT; for an OAuth connector it also snapshots `oauth_provider` + `deliver_json`, and its `api_key` column holds the refresh token (in the SQLCipher-encrypted file, so no column crypto). Because `memory_docs` is an owner table, one definition backs **private** memory in each `{userid}.db` and **shared** memory in `system.db` (the household owner) — see the memory namespace note below. (`projects`/`project_tickets` were owner tables in the single-user past: projects are shareable now, so `projects` + `project_members` are registry tables and `project_tickets` is gone.) -Schema is greenfield (no migrations, §0), but a purely **additive** column lands on an existing DB in place: `db::ensure_column` runs `ALTER TABLE … ADD COLUMN` and swallows the "duplicate column" error, a no-op on a fresh DB where the `CREATE TABLE` already has the column. Used for the OAuth columns on `mcp_catalog` / `mcp_user_servers` so a dev box need not be wiped for an additive change (a full recreate is still valid). +**The schema is no longer greenfield** (see the production note at the top): a full recreate is not an option anymore. `db::ensure_column` — `ALTER TABLE … ADD COLUMN` swallowing the "duplicate column" error, a no-op on a fresh DB where the `CREATE TABLE` already carries it — is therefore not a convenience for dev boxes anymore but the **only** change shape that is currently safe, and additive-with-a-default is the shape to design towards. Used for the OAuth columns on `mcp_catalog` / `mcp_user_servers`. Anything destructive waits for real versioning. **No foreign key in the owner bucket may point at a registry table.** SQLite cannot enforce a key across files, not even through `ATTACH`, and sqlx turns on `PRAGMA foreign_keys`: the `CREATE TABLE` succeeds and every `INSERT` fails. `db::tests::owner_tables_stand_alone_with_foreign_keys_on` enforces this by running the owner schema against a database holding nothing else, then inserting a row into each table. One key crossed and was fixed: `chat_history.model_db_id` (dropped — write-only, and `llm_requests.model_name` already records the model). diff --git a/agents/assistant/AGENT.md b/agents/assistant/AGENT.md index cbe75bf..1c5a15c 100644 --- a/agents/assistant/AGENT.md +++ b/agents/assistant/AGENT.md @@ -58,7 +58,7 @@ Rules of thumb: - **`mode=async`** — **the default for anything non-trivial.** It launches without blocking you, so you keep talking to the user while it runs. When it finishes, the system injects the result as a synthetic `task_completed` tool call — react to it and relay the outcome. After launching, tell the user it is running, then **do not poll** — the result arrives on its own. - **`mode=sync`** — run now and block for the answer. Only for **short** sub-tasks whose result you need immediately to finish composing your current reply. -- **`mode=cron`** — schedule a recurring or one-shot task (7-field cron expression, `Europe/London`). The result arrives as a notification. +- **`mode=cron`** — schedule a recurring or one-shot task (7-field cron expression; the tool description names the timezone it is evaluated in). The result arrives as a notification. ## Notifications diff --git a/crates/skald-core/src/config.rs b/crates/skald-core/src/config.rs index 7774a74..26a4f32 100644 --- a/crates/skald-core/src/config.rs +++ b/crates/skald-core/src/config.rs @@ -39,13 +39,15 @@ pub struct LlmConfig { } /// Controls date/time injection in the dynamic tail of each LLM request. +/// +/// The injected time is **always** truncated to the hour, and the block says so: +/// see [`crate::loop_adapters::system`]. There is deliberately no rounding knob — +/// the granularity is part of what the model is told, not an instance setting. #[derive(Debug, Clone, Deserialize)] pub struct DatetimeConfig { /// Inject the current date/time into the LLM context. Default: true. #[serde(default = "default_true")] pub enabled: bool, - /// When set, round the injected time down to the nearest N-minute boundary. - pub round_minutes: Option, /// IANA timezone name to use when formatting the injected timestamp. /// Populated at startup from the global `timezone` config field. #[serde(skip)] @@ -54,7 +56,7 @@ pub struct DatetimeConfig { impl Default for DatetimeConfig { fn default() -> Self { - Self { enabled: true, round_minutes: None, timezone: None } + Self { enabled: true, timezone: None } } } diff --git a/crates/skald-core/src/cron/mod.rs b/crates/skald-core/src/cron/mod.rs index 0049f4b..732e0f8 100644 --- a/crates/skald-core/src/cron/mod.rs +++ b/crates/skald-core/src/cron/mod.rs @@ -57,6 +57,17 @@ impl TaskManager { }) } + /// The zone cron expressions are evaluated in — the configured `timezone`, + /// else the system's. Exists so the `execute_task` tool description can name + /// it instead of hardcoding one: a model told the wrong zone writes a + /// correct-looking expression that fires at the wrong hour. + pub fn timezone_name(&self) -> String { + self.tz + .map(|tz| tz.name().to_string()) + .or_else(|| iana_time_zone::get_timezone().ok()) + .unwrap_or_else(|| "the server's local timezone".to_string()) + } + /// Called once after ChatSessionManager is built, breaking the circular dep. pub fn set_session(&self, session: Arc) { let _ = self.session.set(session); diff --git a/crates/skald-core/src/loop_adapters/system.rs b/crates/skald-core/src/loop_adapters/system.rs index 2028254..3ac20df 100644 --- a/crates/skald-core/src/loop_adapters/system.rs +++ b/crates/skald-core/src/loop_adapters/system.rs @@ -143,6 +143,18 @@ fn os_description() -> &'static str { OS.get_or_init(|| os_info::get().to_string()) } +/// Formats an instant to hour precision: `Sunday 2026-08-02 17:00 +02:00`. +/// +/// Minutes and seconds are dropped by the format string itself, so the +/// truncation always happens in the zone being displayed. The weekday is part +/// of the format on purpose — see [`AgentSystemContext::datetime_block`]. +fn render_hour(dt: chrono::DateTime) -> String +where + Tz::Offset: std::fmt::Display, +{ + dt.format("%A %Y-%m-%d %H:00 %:z").to_string() +} + /// System IANA timezone name, computed once. fn system_timezone() -> Option<&'static str> { static TZ: std::sync::OnceLock> = std::sync::OnceLock::new(); @@ -169,23 +181,25 @@ impl AgentSystemContext { /// The current date/time + OS + cwd block (`None` when disabled). /// - /// Rounding exists for the prompt cache: a timestamp that changes every - /// second would invalidate any cached suffix, so the instance can quantize - /// it (this block is in the dynamic tail, after the cached prefix, but the - /// rounding still helps providers that cache further). + /// The time is **truncated to the hour**, always, and the block says so in + /// words. Two reasons, neither of which is the prompt cache — this block is + /// the last system message, after the whole conversation, so the cached + /// prefix is identical from one turn to the next whatever the timestamp says: + /// + /// 1. **Honesty.** A second-precision timestamp reads as exact to the model + /// long after it stopped being true (it is built once per request, and a + /// turn can run for minutes). An hour-precision one that announces itself + /// as such lets the model know what it does *not* know — which matters + /// when it is about to write a cron expression from "in ten minutes". + /// 2. It keeps the block cache-safe if it ever moves into the prefix. + /// + /// The weekday is spelled out: "next Tuesday" is a far more common ask than + /// the minute, and deriving it from a date is exactly the arithmetic models + /// get wrong. fn datetime_block(&self) -> Option { if !self.datetime.enabled { return None; } - let secs = chrono::Utc::now().timestamp(); - let secs = match self.datetime.round_minutes { - Some(m) if m > 0 => { - let bucket = (m as i64) * 60; - (secs / bucket) * bucket - } - _ => secs, - }; - let tz = self .datetime .timezone @@ -193,30 +207,15 @@ impl AgentSystemContext { .and_then(|s| s.parse::().ok()) .or_else(|| system_timezone().and_then(|s| s.parse::().ok())); + // Truncate in the *displayed* zone, not on the UTC epoch: a zone at a + // 30- or 45-minute offset (Asia/Kolkata, Asia/Kathmandu) would otherwise + // render as `17:30`, which is not an hour boundary and reads as precise. let (formatted, tz_name) = match tz { - Some(tz) => { - use chrono::TimeZone as _; - let f = tz - .timestamp_opt(secs, 0) - .single() - .map(|dt| dt.format("%Y-%m-%dT%H:%M:%S%:z").to_string()) - .unwrap_or_else(|| { - chrono::Local::now().format("%Y-%m-%dT%H:%M:%S%:z").to_string() - }); - (f, Some(tz.name().to_string())) - } - None => { - let f = chrono::DateTime::from_timestamp(secs, 0) - .map(|utc| { - utc.with_timezone(&chrono::Local) - .format("%Y-%m-%dT%H:%M:%S%:z") - .to_string() - }) - .unwrap_or_else(|| { - chrono::Local::now().format("%Y-%m-%dT%H:%M:%S%:z").to_string() - }); - (f, None) - } + Some(tz) => ( + render_hour(chrono::Utc::now().with_timezone(&tz)), + Some(tz.name().to_string()), + ), + None => (render_hour(chrono::Local::now()), None), }; let date_line = match tz_name { Some(name) => format!("Current date and time: {formatted} ({name})"), @@ -227,7 +226,11 @@ impl AgentSystemContext { let cwd = "~"; Some(format!( - "{date_line}\nOperating system: {}\nWorking directory: {cwd}\n\ + "{date_line}\n\ + The time above is truncated to the hour — you do not know the current minute. \ + If you need it exactly (for instance to schedule something within the hour), \ + run `date` with execute_cmd first.\n\ + Operating system: {}\nWorking directory: {cwd}\n\ Filesystem tools and execute_cmd resolve relative paths against your home directory.", os_description() )) @@ -505,6 +508,23 @@ mod tests { assert_eq!(out, input); } + #[test] + fn hour_render_names_the_day_and_drops_the_minutes() { + let instant = chrono::DateTime::parse_from_rfc3339("2026-08-02T15:54:31Z").unwrap(); + let rome = instant.with_timezone(&chrono_tz::Europe::Rome); + assert_eq!(render_hour(rome), "Sunday 2026-08-02 17:00 +02:00"); + } + + #[test] + fn hour_render_truncates_in_the_displayed_zone() { + // Asia/Kolkata is +05:30: truncating the UTC epoch instead would render + // 20:30 — an hour off AND not on an hour boundary, so it would read as + // a precise time. Truncation must happen after the zone conversion. + let instant = chrono::DateTime::parse_from_rfc3339("2026-08-02T15:54:31Z").unwrap(); + let kolkata = instant.with_timezone(&chrono_tz::Asia::Kolkata); + assert_eq!(render_hour(kolkata), "Sunday 2026-08-02 21:00 +05:30"); + } + #[test] fn shared_folders_table_renders_access_and_description() { use crate::db::shared_folders::SharedFolderAccess; diff --git a/crates/skald-core/src/loop_adapters/testkit.rs b/crates/skald-core/src/loop_adapters/testkit.rs index 96c0761..7f25969 100644 --- a/crates/skald-core/src/loop_adapters/testkit.rs +++ b/crates/skald-core/src/loop_adapters/testkit.rs @@ -181,7 +181,7 @@ pub fn mcp() -> Arc { /// The datetime block is disabled: it embeds `now()`, which no snapshot can /// pin down. pub fn datetime() -> DatetimeConfig { - DatetimeConfig { enabled: false, round_minutes: None, timezone: None } + DatetimeConfig { enabled: false, timezone: None } } /// The base tool definitions the projection is handed. diff --git a/crates/skald-core/src/tools/cron_jobs.rs b/crates/skald-core/src/tools/cron_jobs.rs index 5088972..df8e482 100644 --- a/crates/skald-core/src/tools/cron_jobs.rs +++ b/crates/skald-core/src/tools/cron_jobs.rs @@ -18,18 +18,23 @@ use crate::tools::{SimpleExecution, Tool, ToolContext, ToolDescriptionLength, To pub struct ExecuteTask(pub Arc); impl ExecuteTask { - fn description_text() -> &'static str { - "Create and run a task. Three modes:\n\ - • mode=cron — scheduled by a 7-field cron expression (sec min hour dom month dow year, \ - Europe/London timezone). Returns task_id and next scheduled run. Recurring unless the \ - expression can only fire once.\n\ - • mode=sync — run immediately, block until the agent finishes, and return the result inline. \ - Best for short tasks (a few seconds to a few minutes).\n\ - • mode=async — start the task in the background and return the task_id immediately. \ - When the task completes its result will be delivered back to this chat automatically." + /// `tz` is the zone the scheduler actually evaluates expressions in + /// (`TaskManager::timezone_name`), never a literal: the description is what + /// the model reasons from, so a wrong zone here is an hours-off cron job. + fn description_text(tz: &str) -> String { + format!( + "Create and run a task. Three modes:\n\ + • mode=cron — scheduled by a 7-field cron expression (sec min hour dom month dow year, \ + {tz} timezone). Returns task_id and next scheduled run. Recurring unless the \ + expression can only fire once.\n\ + • mode=sync — run immediately, block until the agent finishes, and return the result inline. \ + Best for short tasks (a few seconds to a few minutes).\n\ + • mode=async — start the task in the background and return the task_id immediately. \ + When the task completes its result will be delivered back to this chat automatically." + ) } - fn schema() -> Value { + fn schema(tz: &str) -> Value { json!({ "type": "object", "required": ["mode", "title", "prompt", "agent_id"], @@ -41,7 +46,7 @@ impl ExecuteTask { }, "title": { "type": "string", "description": "Short name for this task" }, "description": { "type": "string", "description": "What this task does" }, - "cron": { "type": "string", "description": "7-field cron expression — required when mode=cron (times in Europe/London). E.g. '0 0 9 * * * *' = every day at 09:00" }, + "cron": { "type": "string", "description": format!("7-field cron expression — required when mode=cron (times in {tz}). E.g. '0 0 9 * * * *' = every day at 09:00") }, "prompt": { "type": "string", "description": "Prompt sent to the agent at each run" }, "agent_id": { "type": "string", "description": "Task agent to run (required; e.g. software-engineer, researcher, generalist). Must be a `task` agent — chat/system agents are rejected." } } @@ -107,6 +112,7 @@ pub fn build_execute_task_interface_tool( ) -> crate::session::handler::InterfaceTool { use crate::session::handler::{InterfaceTool, ToolFuture}; + let tz = task_mgr.timezone_name(); let tool = Arc::new(ExecuteTask(task_mgr)); InterfaceTool { @@ -114,8 +120,8 @@ pub fn build_execute_task_interface_tool( "type": "function", "function": { "name": "execute_task", - "description": ExecuteTask::description_text(), - "parameters": ExecuteTask::schema(), + "description": ExecuteTask::description_text(&tz), + "parameters": ExecuteTask::schema(&tz), } }), handler: Arc::new(move |args: Value| -> ToolFuture { diff --git a/default.config.yaml b/default.config.yaml index b923c17..87384e9 100644 --- a/default.config.yaml +++ b/default.config.yaml @@ -60,9 +60,12 @@ llm: # execute_subtask) in a single response. Bounds fan-out to avoid provider # rate-limit storms. Omit for the default (4); set to 1 to force sequential. max_parallel_subagents: 4 + # Injects "Current date and time: Sunday 2026-08-02 17:00 +02:00 (Europe/Rome)" + # as the last system message of each request. The time is always truncated to + # the hour and the block tells the model so, pointing it at `date` when it + # needs the exact minute — there is no rounding setting. datetime: enabled: true - round_minutes: 60 # Help with KV cache (instead of 10:54, it will pass 10:50 to the LLM) # ── Tool result size limit ────────────────────────────────────────────────── # When set, tool results from *previous* turns that exceed this character @@ -109,11 +112,13 @@ llm: # ─────────────────────────────────────────────────────────────────────────── # ── Date/time injection ───────────────────────────────────────────────────── - # Configured above as `datetime`. Rounding keeps the injected timestamp stable - # for up to N minutes, so the dynamic tail can be KV-cached across requests - # instead of changing every second. + # Configured above as `datetime`. The only setting is `enabled`. + # The injected time is always truncated to the hour: not for the prompt cache + # (the block is the LAST system message, so the cached prefix never changes + # whatever the timestamp says) but because a second-precision stamp reads as + # exact to the model long after it stopped being true. The block states the + # granularity and tells the agent to run `date` when it needs the minute. # enabled: true # set to false to disable injection entirely - # round_minutes: 10 # round down to nearest N minutes (e.g. 10:56 → 10:50) # ─────────────────────────────────────────────────────────────────────────── # ── LLM request/response log ────────────────────────────────────────────────