From 7d745d5479387a3e5de4f4e6a19c20ed23f5f713 Mon Sep 17 00:00:00 2001
From: Henry Park <henrypark133@gmail.com>
Date: Fri, 13 Mar 2026 11:24:45 -0700
Subject: [PATCH] tools: improve routine schema guidance (#1089)

---
 src/tools/builtin/routine.rs                  | 363 ++++++++++++------
 src/tools/schema_validator.rs                 |  55 +--
 tests/e2e_builtin_tool_coverage.rs            | 118 +++++-
 .../llm_traces/tools/routine_create_list.json |  10 +-
 .../tools/routine_manual_create.json          |  36 ++
 .../tools/routine_system_event_emit.json      |   6 +
 6 files changed, 413 insertions(+), 175 deletions(-)
 create mode 100644 tests/fixtures/llm_traces/tools/routine_manual_create.json

diff --git a/src/tools/builtin/routine.rs b/src/tools/builtin/routine.rs
index 577cd2d1..42a771d3 100644
--- a/src/tools/builtin/routine.rs
+++ b/src/tools/builtin/routine.rs
@@ -24,6 +24,132 @@ use crate::context::JobContext;
 use crate::db::Database;
 use crate::tools::tool::{ApprovalRequirement, Tool, ToolError, ToolOutput, require_str};
 
+pub(crate) fn routine_create_parameters_schema() -> serde_json::Value {
+    serde_json::json!({
+        "type": "object",
+        "properties": {
+            "name": {
+                "type": "string",
+                "description": "Unique routine name, for example 'daily-pr-review'."
+            },
+            "description": {
+                "type": "string",
+                "description": "Short summary of what the routine is for."
+            },
+            "trigger_type": {
+                "type": "string",
+                "enum": ["cron", "event", "system_event", "manual"],
+                "description": "When the routine fires: 'cron' for schedules, 'event' for incoming messages, 'system_event' for structured emitted events, or 'manual' for explicit runs."
+            },
+            "schedule": {
+                "type": "string",
+                "description": "Cron schedule for 'cron' triggers. Uses 6 fields: second minute hour day month weekday."
+            },
+            "event_pattern": {
+                "type": "string",
+                "description": "Regex matched against incoming message text for 'event' triggers, for example '^bug\\\\b'."
+            },
+            "event_channel": {
+                "type": "string",
+                "description": "Optional platform filter for 'event' triggers, for example 'telegram'. Omit to match any channel. Not a chat or thread ID."
+            },
+            "event_source": {
+                "type": "string",
+                "description": "Structured event source for 'system_event' triggers, for example 'github'."
+            },
+            "event_type": {
+                "type": "string",
+                "description": "Structured event type for 'system_event' triggers, for example 'issue.opened'."
+            },
+            "event_filters": {
+                "type": "object",
+                "properties": {},
+                "additionalProperties": {
+                    "type": ["string", "number", "boolean"]
+                },
+                "description": "Optional exact-match payload filters for 'system_event' triggers. Values can be strings, numbers, or booleans."
+            },
+            "prompt": {
+                "type": "string",
+                "description": "Instructions for what the routine should do after it fires."
+            },
+            "context_paths": {
+                "type": "array",
+                "items": { "type": "string" },
+                "description": "Workspace paths to load as extra context before running the routine."
+            },
+            "action_type": {
+                "type": "string",
+                "enum": ["lightweight", "full_job"],
+                "description": "Execution mode: 'lightweight' for one LLM turn or 'full_job' for a multi-step job with tools."
+            },
+            "use_tools": {
+                "type": "boolean",
+                "description": "Enable safe tool use in 'lightweight' mode. Ignored for 'full_job'."
+            },
+            "max_tool_rounds": {
+                "type": "integer",
+                "description": "Maximum tool-call rounds in 'lightweight' mode when 'use_tools' is true."
+            },
+            "cooldown_secs": {
+                "type": "integer",
+                "description": "Minimum seconds between fires."
+            },
+            "tool_permissions": {
+                "type": "array",
+                "items": { "type": "string" },
+                "description": "Pre-authorized tool names for 'full_job' routines."
+            },
+            "notify_channel": {
+                "type": "string",
+                "description": "Where routine output should be sent, for example 'telegram' or 'slack'. This does not control what triggers the routine."
+            },
+            "notify_user": {
+                "type": "string",
+                "description": "User or destination to notify, for example a username or chat ID."
+            },
+            "timezone": {
+                "type": "string",
+                "description": "IANA timezone used to evaluate 'cron' schedules, for example 'America/New_York'."
+            }
+        },
+        "required": ["name", "trigger_type", "prompt"]
+    })
+}
+
+pub(crate) fn routine_update_parameters_schema() -> serde_json::Value {
+    serde_json::json!({
+        "type": "object",
+        "properties": {
+            "name": {
+                "type": "string",
+                "description": "Name of the routine to update."
+            },
+            "enabled": {
+                "type": "boolean",
+                "description": "Set to true to enable the routine or false to disable it."
+            },
+            "prompt": {
+                "type": "string",
+                "description": "Replace the routine instructions for what it should do after it fires."
+            },
+            "schedule": {
+                "type": "string",
+                "description": "New cron schedule for existing 'cron' routines only. This does not convert other trigger types."
+            },
+            "timezone": {
+                "type": "string",
+                "description": "New IANA timezone for existing 'cron' routines only, for example 'America/New_York'."
+            },
+            "description": {
+                "type": "string",
+                "description": "Replace the routine summary."
+            }
+        },
+        "required": ["name"]
+    })
+}
+
 // ==================== routine_create ====================
 
 pub struct RoutineCreateTool {
@@ -50,92 +176,7 @@ impl Tool for RoutineCreateTool {
     }
 
     fn parameters_schema(&self) -> serde_json::Value {
-        serde_json::json!({
-            "type": "object",
-            "properties": {
-                "name": {
-                    "type": "string",
-                    "description": "Unique name for the routine (e.g. 'daily-pr-review')"
-                },
-                "description": {
-                    "type": "string",
-                    "description": "What this routine does"
-                },
-                "trigger_type": {
-                    "type": "string",
-                    "enum": ["cron", "event", "system_event", "manual"],
-                    "description": "When the routine fires"
-                },
-                "schedule": {
-                    "type": "string",
-                    "description": "Cron expression (for cron trigger). E.g. '0 9 * * MON-FRI' for weekdays at 9am. Uses 6-field cron (sec min hour day month weekday)."
-                },
-                "event_pattern": {
-                    "type": "string",
-                    "description": "Regex pattern to match messages (for event trigger)"
-                },
-                "event_channel": {
-                    "type": "string",
-                    "description": "Optional channel filter for event trigger (e.g. 'telegram')"
-                },
-                "event_source": {
-                    "type": "string",
-                    "description": "Event source for system_event triggers (e.g. 'github')"
-                },
-                "event_type": {
-                    "type": "string",
-                    "description": "Event type for system_event triggers (e.g. 'issue.opened')"
-                },
-                "event_filters": {
-                    "type": "object",
-                    "description": "Optional exact-match filters against payload fields for system_event triggers. Values can be strings, numbers, or booleans."
-                },
-                "prompt": {
-                    "type": "string",
-                    "description": "The prompt/instructions for the routine"
-                },
-                "context_paths": {
-                    "type": "array",
-                    "items": { "type": "string" },
-                    "description": "Workspace paths to load as context (e.g. ['context/priorities.md'])"
-                },
-                "action_type": {
-                    "type": "string",
-                    "enum": ["lightweight", "full_job"],
-                    "description": "Execution mode: 'lightweight' (single LLM call, default) or 'full_job' (multi-turn with tools)"
-                },
-                "use_tools": {
-                    "type": "boolean",
-                    "description": "Enable tool access in lightweight mode (default: false). Only safe tools (no approval required) are available. Ignored for full_job mode."
-                },
-                "max_tool_rounds": {
-                    "type": "integer",
-                    "description": "Max tool call rounds in lightweight mode (default: 3). Only used when use_tools is true."
-                },
-                "cooldown_secs": {
-                    "type": "integer",
-                    "description": "Minimum seconds between fires (default: 300)"
-                },
-                "tool_permissions": {
-                    "type": "array",
-                    "items": { "type": "string" },
-                    "description": "Tool names pre-authorized for Always-approval tools in full_job mode (e.g. ['shell']). UnlessAutoApproved tools are automatically permitted in routines."
-                },
-                "notify_channel": {
-                    "type": "string",
-                    "description": "Channel to send results to (e.g. 'telegram', 'slack', 'tui'). Sets the default channel for message tool calls in routine jobs."
-                },
-                "notify_user": {
-                    "type": "string",
-                    "description": "User/target to notify (e.g. username, chat ID). Defaults to 'default'."
-                },
-                "timezone": {
-                    "type": "string",
-                    "description": "IANA timezone for cron schedule evaluation (e.g. 'America/New_York'). Defaults to UTC."
-                }
-            },
-            "required": ["name", "trigger_type", "prompt"]
-        })
+        routine_create_parameters_schema()
     }
 
     async fn execute(
@@ -482,41 +523,13 @@ impl Tool for RoutineUpdateTool {
     }
 
     fn description(&self) -> &str {
-        "Update an existing routine. Can modify trigger, prompt, schedule, or toggle enabled state. \
-         Pass the routine name and only the fields you want to change."
+        "Update an existing routine. Can change prompt, description, enabled state, or cron timing. \
+         Pass the routine name and only the fields you want to change. \
+         This does not convert one trigger type into another."
     }
 
     fn parameters_schema(&self) -> serde_json::Value {
-        serde_json::json!({
-            "type": "object",
-            "properties": {
-                "name": {
-                    "type": "string",
-                    "description": "Name of the routine to update"
-                },
-                "enabled": {
-                    "type": "boolean",
-                    "description": "Enable or disable the routine"
-                },
-                "prompt": {
-                    "type": "string",
-                    "description": "New prompt/instructions"
-                },
-                "schedule": {
-                    "type": "string",
-                    "description": "New cron schedule (for cron triggers)"
-                },
-                "timezone": {
-                    "type": "string",
-                    "description": "IANA timezone for cron schedule (e.g. 'America/New_York'). Only valid for cron triggers."
-                },
-                "description": {
-                    "type": "string",
-                    "description": "New description"
-                }
-            },
-            "required": ["name"]
-        })
+        routine_update_parameters_schema()
     }
 
     async fn execute(
@@ -957,3 +970,117 @@ impl Tool for EventEmitTool {
         true
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{routine_create_parameters_schema, routine_update_parameters_schema};
+    use crate::tools::validate_tool_schema;
+
+    fn property<'a>(schema: &'a serde_json::Value, name: &str) -> &'a serde_json::Value {
+        schema
+            .get("properties")
+            .and_then(|props| props.get(name))
+            .unwrap_or_else(|| panic!("missing schema property {name}"))
+    }
+
+    #[test]
+    fn routine_create_schema_exposes_all_trigger_and_delivery_fields() {
+        let schema = routine_create_parameters_schema();
+        let errors = validate_tool_schema(&schema, "routine_create");
+        assert!(
+            errors.is_empty(),
+            "routine_create schema should validate cleanly: {errors:?}"
+        );
+
+        for field in [
+            "trigger_type",
+            "schedule",
+            "event_pattern",
+            "event_channel",
+            "event_source",
+            "event_type",
+            "event_filters",
+            "action_type",
+            "use_tools",
+            "max_tool_rounds",
+            "tool_permissions",
+            "notify_channel",
+            "notify_user",
+            "timezone",
+        ] {
+            let _ = property(&schema, field);
+        }
+    }
+
+    #[test]
+    fn routine_create_schema_descriptions_cover_event_trigger_gotchas() {
+        let schema = routine_create_parameters_schema();
+
+        let trigger_type = property(&schema, "trigger_type")
+            .get("description")
+            .and_then(|value| value.as_str())
+            .expect("trigger_type description");
+        assert!(trigger_type.contains("incoming messages"));
+        assert!(trigger_type.contains("structured emitted events"));
+
+        let event_pattern = property(&schema, "event_pattern")
+            .get("description")
+            .and_then(|value| value.as_str())
+            .expect("event_pattern description");
+        assert!(event_pattern.contains("incoming message text"));
+        assert!(event_pattern.contains("^bug\\\\b"));
+
+        let event_channel = property(&schema, "event_channel")
+            .get("description")
+            .and_then(|value| value.as_str())
+            .expect("event_channel description");
+        assert!(event_channel.contains("Omit to match any channel"));
+        assert!(event_channel.contains("Not a chat or thread ID"));
+
+        let notify_channel = property(&schema, "notify_channel")
+            .get("description")
+            .and_then(|value| value.as_str())
+            .expect("notify_channel description");
+        assert!(notify_channel.contains("does not control what triggers"));
+
+        let prompt = property(&schema, "prompt")
+            .get("description")
+            .and_then(|value| value.as_str())
+            .expect("prompt description");
+        assert!(prompt.contains("after it fires"));
+    }
+
+    #[test]
+    fn routine_update_schema_exposes_supported_fields_and_limits() {
+        let schema = routine_update_parameters_schema();
+        let errors = validate_tool_schema(&schema, "routine_update");
+        assert!(
+            errors.is_empty(),
+            "routine_update schema should validate cleanly: {errors:?}"
+        );
+
+        for field in [
+            "name",
+            "enabled",
+            "prompt",
+            "schedule",
+            "timezone",
+            "description",
+        ] {
+            let _ = property(&schema, field);
+        }
+
+        let schedule = property(&schema, "schedule")
+            .get("description")
+            .and_then(|value| value.as_str())
+            .expect("schedule description");
+        assert!(schedule.contains("existing 'cron' routines only"));
+        assert!(schedule.contains("does not convert other trigger types"));
+
+        let timezone = property(&schema, "timezone")
+            .get("description")
+            .and_then(|value| value.as_str())
+            .expect("timezone description");
+        assert!(timezone.contains("existing 'cron' routines only"));
+    }
+}
diff --git a/src/tools/schema_validator.rs b/src/tools/schema_validator.rs
index a5b8fd40..9cc2fa5f 100644
--- a/src/tools/schema_validator.rs
+++ b/src/tools/schema_validator.rs
@@ -558,48 +558,7 @@ mod tests {
             // Routine tools
             (
                 "routine_create",
-                serde_json::json!({
-                    "type": "object",
-                    "properties": {
-                        "name": { "type": "string", "description": "Routine name" },
-                        "description": { "type": "string", "description": "What it does" },
-                        "trigger_type": {
-                            "type": "string",
-                            "enum": ["cron", "event", "system_event", "manual"],
-                            "description": "When the routine fires"
-                        },
-                        "schedule": { "type": "string", "description": "Cron expression" },
-                        "event_pattern": { "type": "string", "description": "Regex pattern" },
-                        "event_channel": { "type": "string", "description": "Channel filter" },
-                        "event_source": { "type": "string", "description": "System event source" },
-                        "event_type": { "type": "string", "description": "System event type" },
-                        "event_filters": {
-                            "type": "object",
-                            "additionalProperties": { "type": "string" },
-                            "description": "Exact-match payload filters"
-                        },
-                        "prompt": { "type": "string", "description": "Instructions" },
-                        "context_paths": {
-                            "type": "array",
-                            "items": { "type": "string" },
-                            "description": "Workspace paths to load"
-                        },
-                        "action_type": {
-                            "type": "string",
-                            "enum": ["lightweight", "full_job"],
-                            "description": "Execution mode"
-                        },
-                        "cooldown_secs": { "type": "integer", "description": "Min seconds between fires" },
-                        "tool_permissions": {
-                            "type": "array",
-                            "items": { "type": "string" },
-                            "description": "Pre-authorized tools for full_job mode"
-                        },
-                        "notify_channel": { "type": "string", "description": "Channel for message tool" },
-                        "notify_user": { "type": "string", "description": "User/target to notify" }
-                    },
-                    "required": ["name", "trigger_type", "prompt"]
-                }),
+                crate::tools::builtin::routine::routine_create_parameters_schema(),
             ),
             (
                 "routine_list",
@@ -611,17 +570,7 @@ mod tests {
             ),
             (
                 "routine_update",
-                serde_json::json!({
-                    "type": "object",
-                    "properties": {
-                        "name": { "type": "string", "description": "Name" },
-                        "enabled": { "type": "boolean", "description": "Toggle" },
-                        "prompt": { "type": "string", "description": "New prompt" },
-                        "schedule": { "type": "string", "description": "New cron schedule" },
-                        "description": { "type": "string", "description": "New description" }
-                    },
-                    "required": ["name"]
-                }),
+                crate::tools::builtin::routine::routine_update_parameters_schema(),
             ),
             (
                 "routine_delete",
diff --git a/tests/e2e_builtin_tool_coverage.rs b/tests/e2e_builtin_tool_coverage.rs
index f1ae3660..4da65c23 100644
--- a/tests/e2e_builtin_tool_coverage.rs
+++ b/tests/e2e_builtin_tool_coverage.rs
@@ -10,6 +10,8 @@ mod support;
 mod tests {
     use std::time::Duration;
 
+    use ironclaw::agent::routine::{RoutineAction, Trigger};
+
     use crate::support::test_rig::TestRigBuilder;
     use crate::support::trace_llm::LlmTrace;
 
@@ -123,6 +125,39 @@ mod tests {
             "routine_list should succeed: {completed:?}"
         );
 
+        let routine = rig
+            .database()
+            .get_routine_by_name("test-user", "daily-check")
+            .await
+            .expect("get_routine_by_name")
+            .expect("daily-check should exist");
+
+        match &routine.trigger {
+            Trigger::Cron { schedule, timezone } => {
+                assert_eq!(schedule, "0 0 9 * * *");
+                assert_eq!(timezone.as_deref(), Some("America/New_York"));
+            }
+            other => panic!("expected cron trigger, got {other:?}"),
+        }
+
+        match &routine.action {
+            RoutineAction::Lightweight {
+                context_paths,
+                use_tools,
+                max_tool_rounds,
+                ..
+            } => {
+                assert_eq!(context_paths, &vec!["context/priorities.md".to_string()]);
+                assert!(*use_tools, "lightweight routine should keep use_tools=true");
+                assert_eq!(*max_tool_rounds, 2);
+            }
+            other => panic!("expected lightweight action, got {other:?}"),
+        }
+
+        assert_eq!(routine.notify.channel.as_deref(), Some("telegram"));
+        assert_eq!(routine.notify.user, "ops-team");
+        assert_eq!(routine.guardrails.cooldown.as_secs(), 600);
+
         rig.shutdown();
     }
 
@@ -168,7 +203,48 @@ mod tests {
     }
 
     // -----------------------------------------------------------------------
-    // Test 5: routine_history
+    // Test 5: routine_manual_create
+    // -----------------------------------------------------------------------
+
+    #[tokio::test]
+    async fn routine_manual_create() {
+        let trace = LlmTrace::from_file(concat!(
+            env!("CARGO_MANIFEST_DIR"),
+            "/tests/fixtures/llm_traces/tools/routine_manual_create.json"
+        ))
+        .expect("failed to load routine_manual_create.json");
+
+        let rig = TestRigBuilder::new()
+            .with_trace(trace.clone())
+            .with_auto_approve_tools(true)
+            .build()
+            .await;
+
+        rig.send_message("Create a manual routine for bug triage")
+            .await;
+        let responses = rig.wait_for_responses(1, Duration::from_secs(15)).await;
+
+        rig.verify_trace_expects(&trace, &responses);
+
+        let routine = rig
+            .database()
+            .get_routine_by_name("test-user", "manual-triage")
+            .await
+            .expect("get_routine_by_name")
+            .expect("manual-triage should exist");
+
+        assert!(matches!(routine.trigger, Trigger::Manual));
+        assert!(
+            matches!(&routine.action, RoutineAction::Lightweight { use_tools, .. } if !*use_tools),
+            "manual routine should default to lightweight without tools: {:?}",
+            routine.action
+        );
+
+        rig.shutdown();
+    }
+
+    // -----------------------------------------------------------------------
+    // Test 6: routine_history
     // -----------------------------------------------------------------------
 
     #[tokio::test]
@@ -205,7 +281,7 @@ mod tests {
     }
 
     // -----------------------------------------------------------------------
-    // Test 6: routine_system_event_emit
+    // Test 7: routine_system_event_emit
     // -----------------------------------------------------------------------
 
     #[tokio::test]
@@ -253,11 +329,47 @@ mod tests {
             emit_result.1
         );
 
+        let routine = rig
+            .database()
+            .get_routine_by_name("test-user", "gh-issue-emit-test")
+            .await
+            .expect("get_routine_by_name")
+            .expect("gh-issue-emit-test should exist");
+
+        match &routine.trigger {
+            Trigger::SystemEvent {
+                source,
+                event_type,
+                filters,
+            } => {
+                assert_eq!(source, "github");
+                assert_eq!(event_type, "issue.opened");
+                assert_eq!(
+                    filters.get("repository").map(String::as_str),
+                    Some("nearai/ironclaw")
+                );
+                assert_eq!(filters.get("priority").map(String::as_str), Some("p1"));
+            }
+            other => panic!("expected system_event trigger, got {other:?}"),
+        }
+
+        match &routine.action {
+            RoutineAction::FullJob {
+                description,
+                tool_permissions,
+                ..
+            } => {
+                assert!(description.contains("Summarize the new issue"));
+                assert_eq!(tool_permissions, &vec!["shell".to_string()]);
+            }
+            other => panic!("expected full_job action, got {other:?}"),
+        }
+
         rig.shutdown();
     }
 
     // -----------------------------------------------------------------------
-    // Test 7: skill_install_routine_webhook_sim
+    // Test 8: skill_install_routine_webhook_sim
     // -----------------------------------------------------------------------
 
     #[tokio::test]
diff --git a/tests/fixtures/llm_traces/tools/routine_create_list.json b/tests/fixtures/llm_traces/tools/routine_create_list.json
index 74d8cdb2..114bae16 100644
--- a/tests/fixtures/llm_traces/tools/routine_create_list.json
+++ b/tests/fixtures/llm_traces/tools/routine_create_list.json
@@ -18,8 +18,16 @@
               "name": "daily-check",
               "trigger_type": "cron",
               "schedule": "0 0 9 * * *",
+              "timezone": "America/New_York",
               "prompt": "Check system status and report any issues.",
-              "description": "Daily system health check"
+              "description": "Daily system health check",
+              "context_paths": ["context/priorities.md"],
+              "action_type": "lightweight",
+              "use_tools": true,
+              "max_tool_rounds": 2,
+              "cooldown_secs": 600,
+              "notify_channel": "telegram",
+              "notify_user": "ops-team"
             }
           }
         ],
diff --git a/tests/fixtures/llm_traces/tools/routine_manual_create.json b/tests/fixtures/llm_traces/tools/routine_manual_create.json
new file mode 100644
index 00000000..bf386263
--- /dev/null
+++ b/tests/fixtures/llm_traces/tools/routine_manual_create.json
@@ -0,0 +1,36 @@
+{
+  "model_name": "test-routine-manual-create",
+  "expects": {
+    "tools_used": ["routine_create"],
+    "all_tools_succeeded": true,
+    "min_responses": 1
+  },
+  "steps": [
+    {
+      "response": {
+        "type": "tool_calls",
+        "tool_calls": [
+          {
+            "id": "call_rc_manual_1",
+            "name": "routine_create",
+            "arguments": {
+              "name": "manual-triage",
+              "trigger_type": "manual",
+              "prompt": "Summarize the latest bug reports when this routine is fired."
+            }
+          }
+        ],
+        "input_tokens": 90,
+        "output_tokens": 22
+      }
+    },
+    {
+      "response": {
+        "type": "text",
+        "content": "Created the manual-triage routine. It will only run when explicitly fired.",
+        "input_tokens": 140,
+        "output_tokens": 18
+      }
+    }
+  ]
+}
diff --git a/tests/fixtures/llm_traces/tools/routine_system_event_emit.json b/tests/fixtures/llm_traces/tools/routine_system_event_emit.json
index 484574bb..3ba49c73 100644
--- a/tests/fixtures/llm_traces/tools/routine_system_event_emit.json
+++ b/tests/fixtures/llm_traces/tools/routine_system_event_emit.json
@@ -21,7 +21,12 @@
               "trigger_type": "system_event",
               "event_source": "github",
               "event_type": "issue.opened",
+              "event_filters": {
+                "repository": "nearai/ironclaw",
+                "priority": "p1"
+              },
               "action_type": "full_job",
+              "tool_permissions": ["shell"],
               "prompt": "Summarize the new issue and propose next steps."
             }
           }
@@ -42,6 +47,7 @@
               "event_type": "issue.opened",
               "payload": {
                 "repository": "nearai/ironclaw",
+                "priority": "p1",
                 "issue_number": 123,
                 "title": "Support event-driven project workflow"
               }