mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-26 15:40:18 +00:00
Redesign routine create requests for LLMs (#1147)
* Redesign routine create requests for LLMs * Fix panic-check false positives in routine tests * Tighten routine schema requirements * Tighten routine schema tests * Mark test assertions safe for CI scan * Align test assertions with panic scan * Polish routine schema metadata * Simplify routine test assertions * Improve tool discovery guidance * Clarify lightweight routine delivery prompts * Fix routine delivery target defaults
This commit is contained in:
+120
-17
@@ -784,23 +784,12 @@ async fn execute_lightweight(
|
||||
Err(_) => None,
|
||||
};
|
||||
|
||||
// Build the user-facing prompt
|
||||
let mut full_prompt = String::new();
|
||||
full_prompt.push_str(prompt);
|
||||
|
||||
if !context_parts.is_empty() {
|
||||
full_prompt.push_str("\n\n---\n\n# Context\n\n");
|
||||
full_prompt.push_str(&context_parts.join("\n\n"));
|
||||
}
|
||||
|
||||
if let Some(state) = &state_content {
|
||||
full_prompt.push_str("\n\n---\n\n# Previous State\n\n");
|
||||
full_prompt.push_str(state);
|
||||
}
|
||||
|
||||
full_prompt.push_str(
|
||||
"\n\n---\n\nIf nothing needs attention, reply EXACTLY with: ROUTINE_OK\n\
|
||||
If something needs attention, provide a concise summary.",
|
||||
let full_prompt = build_lightweight_prompt(
|
||||
prompt,
|
||||
&context_parts,
|
||||
state_content.as_deref(),
|
||||
&routine.notify,
|
||||
use_tools,
|
||||
);
|
||||
|
||||
// Get system prompt
|
||||
@@ -844,6 +833,65 @@ async fn execute_lightweight(
|
||||
}
|
||||
}
|
||||
|
||||
fn build_lightweight_prompt(
|
||||
prompt: &str,
|
||||
context_parts: &[String],
|
||||
state_content: Option<&str>,
|
||||
notify: &NotifyConfig,
|
||||
use_tools: bool,
|
||||
) -> String {
|
||||
let mut full_prompt = String::new();
|
||||
full_prompt.push_str(prompt);
|
||||
|
||||
if notify.on_attention {
|
||||
full_prompt.push_str("\n\n---\n\n# Delivery\n\n");
|
||||
full_prompt.push_str(
|
||||
"If you reply with anything other than ROUTINE_OK, the host will deliver your \
|
||||
reply as the routine notification. Return the message exactly as it should be sent.\n",
|
||||
);
|
||||
|
||||
if let Some(channel) = notify.channel.as_deref() {
|
||||
full_prompt.push_str(&format!(
|
||||
"The configured delivery channel for this routine is `{channel}`.\n"
|
||||
));
|
||||
}
|
||||
|
||||
if let Some(user) = notify.user.as_deref() {
|
||||
full_prompt.push_str(&format!(
|
||||
"The configured delivery target for this routine is `{user}`.\n"
|
||||
));
|
||||
}
|
||||
|
||||
full_prompt.push_str(
|
||||
"Do not claim you lack messaging integrations or ask the user to set one up when \
|
||||
a plain reply is sufficient.\n",
|
||||
);
|
||||
}
|
||||
|
||||
if !use_tools {
|
||||
full_prompt.push_str(
|
||||
"\nTools are disabled for this routine run. Do not ask to call tools or describe tool limitations unless they prevent a necessary external action.\n",
|
||||
);
|
||||
}
|
||||
|
||||
if !context_parts.is_empty() {
|
||||
full_prompt.push_str("\n\n---\n\n# Context\n\n");
|
||||
full_prompt.push_str(&context_parts.join("\n\n"));
|
||||
}
|
||||
|
||||
if let Some(state) = state_content {
|
||||
full_prompt.push_str("\n\n---\n\n# Previous State\n\n");
|
||||
full_prompt.push_str(state);
|
||||
}
|
||||
|
||||
full_prompt.push_str(
|
||||
"\n\n---\n\nIf nothing needs attention, reply EXACTLY with: ROUTINE_OK\n\
|
||||
If something needs attention, provide a concise summary.",
|
||||
);
|
||||
|
||||
full_prompt
|
||||
}
|
||||
|
||||
/// Execute a lightweight routine without tool support (original single-call behavior).
|
||||
async fn execute_lightweight_no_tools(
|
||||
ctx: &EngineContext,
|
||||
@@ -1385,6 +1433,61 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_build_lightweight_prompt_explains_delivery_and_disabled_tools() {
|
||||
let notify = NotifyConfig {
|
||||
channel: Some("telegram".to_string()),
|
||||
user: Some("default".to_string()),
|
||||
on_attention: true,
|
||||
on_failure: true,
|
||||
on_success: false,
|
||||
};
|
||||
|
||||
let prompt = super::build_lightweight_prompt(
|
||||
"Send a Telegram reminder message to the user.",
|
||||
&[],
|
||||
None,
|
||||
¬ify,
|
||||
false,
|
||||
);
|
||||
|
||||
assert!(
|
||||
prompt.contains("the host will deliver your reply as the routine notification"),
|
||||
"delivery guidance should explain host delivery: {prompt}",
|
||||
);
|
||||
assert!(
|
||||
prompt.contains("configured delivery channel for this routine is `telegram`"),
|
||||
"delivery guidance should mention telegram channel: {prompt}",
|
||||
);
|
||||
assert!(
|
||||
prompt.contains("Do not claim you lack messaging integrations"),
|
||||
"delivery guidance should suppress fake setup chatter: {prompt}",
|
||||
);
|
||||
assert!(
|
||||
prompt.contains("Tools are disabled for this routine run"),
|
||||
"prompt should explain that tools are disabled: {prompt}",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_build_lightweight_prompt_skips_delivery_block_when_attention_notifications_disabled() {
|
||||
let notify = NotifyConfig {
|
||||
on_attention: false,
|
||||
..NotifyConfig::default()
|
||||
};
|
||||
|
||||
let prompt = super::build_lightweight_prompt("Check inbox.", &[], None, ¬ify, true);
|
||||
|
||||
assert!(
|
||||
!prompt.contains("# Delivery"),
|
||||
"prompt should not include delivery guidance when attention notifications are off: {prompt}",
|
||||
);
|
||||
assert!(
|
||||
!prompt.contains("Tools are disabled for this routine run"),
|
||||
"prompt should not claim tools are disabled when they are enabled: {prompt}",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_routine_sentinel_detection_exact_match() {
|
||||
// The execute_lightweight_no_tools checks: content == "ROUTINE_OK" || content.contains("ROUTINE_OK")
|
||||
|
||||
+1560
-332
File diff suppressed because it is too large
Load Diff
+121
-19
@@ -1,8 +1,9 @@
|
||||
//! On-demand tool discovery (like CLI `--help`).
|
||||
//!
|
||||
//! Two levels of detail:
|
||||
//! Three levels of detail:
|
||||
//! - Default: name, description, parameter names (compact ~150 bytes)
|
||||
//! - `include_schema: true`: adds the full typed JSON Schema
|
||||
//! - `detail: "summary"`: adds curated rules, notes, and examples
|
||||
//! - `detail: "schema"` / `include_schema: true`: adds the full typed JSON Schema
|
||||
//!
|
||||
//! Keeps the tools array compact (WASM tools use permissive schemas)
|
||||
//! while allowing precise discovery when needed.
|
||||
@@ -13,7 +14,59 @@ use async_trait::async_trait;
|
||||
|
||||
use crate::context::JobContext;
|
||||
use crate::tools::registry::ToolRegistry;
|
||||
use crate::tools::tool::{Tool, ToolError, ToolOutput, require_str};
|
||||
use crate::tools::tool::{Tool, ToolDiscoverySummary, ToolError, ToolOutput, require_str};
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
enum ToolInfoDetail {
|
||||
Names,
|
||||
Summary,
|
||||
Schema,
|
||||
}
|
||||
|
||||
impl ToolInfoDetail {
|
||||
fn parse(params: &serde_json::Value) -> Result<Self, ToolError> {
|
||||
if params
|
||||
.get("include_schema")
|
||||
.and_then(|v| v.as_bool())
|
||||
.unwrap_or(false)
|
||||
{
|
||||
return Ok(Self::Schema);
|
||||
}
|
||||
|
||||
match params.get("detail").and_then(|v| v.as_str()) {
|
||||
None | Some("names") => Ok(Self::Names),
|
||||
Some("summary") => Ok(Self::Summary),
|
||||
Some("schema") => Ok(Self::Schema),
|
||||
Some(other) => Err(ToolError::InvalidParameters(format!(
|
||||
"invalid detail '{other}' (expected 'names', 'summary', or 'schema')"
|
||||
))),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn schema_param_names(schema: &serde_json::Value) -> Vec<String> {
|
||||
schema
|
||||
.get("properties")
|
||||
.and_then(|p| p.as_object())
|
||||
.map(|props| props.keys().cloned().collect())
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
fn fallback_summary(schema: &serde_json::Value) -> ToolDiscoverySummary {
|
||||
ToolDiscoverySummary {
|
||||
always_required: schema
|
||||
.get("required")
|
||||
.and_then(|v| v.as_array())
|
||||
.map(|required| {
|
||||
required
|
||||
.iter()
|
||||
.filter_map(|value| value.as_str().map(str::to_string))
|
||||
.collect()
|
||||
})
|
||||
.unwrap_or_default(),
|
||||
..ToolDiscoverySummary::default()
|
||||
}
|
||||
}
|
||||
|
||||
pub struct ToolInfoTool {
|
||||
registry: Weak<ToolRegistry>,
|
||||
@@ -32,8 +85,7 @@ impl Tool for ToolInfoTool {
|
||||
}
|
||||
|
||||
fn description(&self) -> &str {
|
||||
"Get info about any tool: description and parameter names. \
|
||||
Set include_schema to true for the full typed parameter schema."
|
||||
"Get info about any tool: description, parameter names, curated summary guidance, or full discovery schema."
|
||||
}
|
||||
|
||||
fn parameters_schema(&self) -> serde_json::Value {
|
||||
@@ -44,9 +96,15 @@ impl Tool for ToolInfoTool {
|
||||
"type": "string",
|
||||
"description": "Name of the tool to get info about"
|
||||
},
|
||||
"detail": {
|
||||
"type": "string",
|
||||
"enum": ["names", "summary", "schema"],
|
||||
"description": "Response detail level. 'names' returns parameter names only. 'summary' adds curated rules/examples. 'schema' returns the full discovery schema.",
|
||||
"default": "names"
|
||||
},
|
||||
"include_schema": {
|
||||
"type": "boolean",
|
||||
"description": "If true, include the full typed JSON Schema for parameters (larger response). Default: false.",
|
||||
"description": "Deprecated compatibility alias for detail='schema'. If true, include the full discovery schema.",
|
||||
"default": false
|
||||
}
|
||||
},
|
||||
@@ -61,10 +119,7 @@ impl Tool for ToolInfoTool {
|
||||
) -> Result<ToolOutput, ToolError> {
|
||||
let start = std::time::Instant::now();
|
||||
let name = require_str(¶ms, "name")?;
|
||||
let include_schema = params
|
||||
.get("include_schema")
|
||||
.and_then(|v| v.as_bool())
|
||||
.unwrap_or(false);
|
||||
let detail = ToolInfoDetail::parse(¶ms)?;
|
||||
|
||||
let registry = self.registry.upgrade().ok_or_else(|| {
|
||||
ToolError::ExecutionFailed(
|
||||
@@ -77,13 +132,7 @@ impl Tool for ToolInfoTool {
|
||||
})?;
|
||||
|
||||
let schema = tool.discovery_schema();
|
||||
|
||||
// Extract just param names from the schema's "properties" keys
|
||||
let param_names: Vec<&str> = schema
|
||||
.get("properties")
|
||||
.and_then(|p| p.as_object())
|
||||
.map(|props| props.keys().map(|k| k.as_str()).collect())
|
||||
.unwrap_or_default();
|
||||
let param_names = schema_param_names(&schema);
|
||||
|
||||
let mut info = serde_json::json!({
|
||||
"name": tool.name(),
|
||||
@@ -91,8 +140,21 @@ impl Tool for ToolInfoTool {
|
||||
"parameters": param_names,
|
||||
});
|
||||
|
||||
if include_schema {
|
||||
info["schema"] = schema;
|
||||
match detail {
|
||||
ToolInfoDetail::Names => {}
|
||||
ToolInfoDetail::Summary => {
|
||||
let summary = tool
|
||||
.discovery_summary()
|
||||
.unwrap_or_else(|| fallback_summary(&schema));
|
||||
info["summary"] = serde_json::to_value(summary).map_err(|err| {
|
||||
ToolError::ExecutionFailed(format!(
|
||||
"failed to serialize discovery summary: {err}"
|
||||
))
|
||||
})?;
|
||||
}
|
||||
ToolInfoDetail::Schema => {
|
||||
info["schema"] = schema;
|
||||
}
|
||||
}
|
||||
|
||||
Ok(ToolOutput::success(info, start.elapsed()))
|
||||
@@ -135,6 +197,30 @@ mod tests {
|
||||
assert!(info.get("schema").is_none());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_tool_info_with_summary() {
|
||||
let registry = Arc::new(ToolRegistry::new());
|
||||
registry.register(Arc::new(EchoTool)).await;
|
||||
|
||||
let tool = ToolInfoTool::new(Arc::downgrade(®istry));
|
||||
let ctx = JobContext::default();
|
||||
let result = tool
|
||||
.execute(
|
||||
serde_json::json!({"name": "echo", "detail": "summary"}),
|
||||
&ctx,
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let info = &result.result;
|
||||
assert_eq!(info["name"], "echo");
|
||||
assert!(info["summary"].is_object());
|
||||
assert_eq!(
|
||||
info["summary"]["always_required"],
|
||||
serde_json::json!(["message"])
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_tool_info_with_schema() {
|
||||
let registry = Arc::new(ToolRegistry::new());
|
||||
@@ -157,6 +243,22 @@ mod tests {
|
||||
assert!(info["schema"]["properties"].is_object());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_tool_info_invalid_detail() {
|
||||
let registry = Arc::new(ToolRegistry::new());
|
||||
registry.register(Arc::new(EchoTool)).await;
|
||||
|
||||
let tool = ToolInfoTool::new(Arc::downgrade(®istry));
|
||||
let ctx = JobContext::default();
|
||||
let result = tool
|
||||
.execute(
|
||||
serde_json::json!({"name": "echo", "detail": "verbose"}),
|
||||
&ctx,
|
||||
)
|
||||
.await;
|
||||
assert!(matches!(result, Err(ToolError::InvalidParameters(_))));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_tool_info_unknown_tool() {
|
||||
let registry = Arc::new(ToolRegistry::new());
|
||||
|
||||
+79
-22
@@ -94,6 +94,15 @@ pub struct ToolRegistry {
|
||||
}
|
||||
|
||||
impl ToolRegistry {
|
||||
fn tool_definition(tool: &Arc<dyn Tool>) -> ToolDefinition {
|
||||
let schema = tool.schema();
|
||||
ToolDefinition {
|
||||
name: schema.name,
|
||||
description: schema.description,
|
||||
parameters: schema.parameters,
|
||||
}
|
||||
}
|
||||
|
||||
/// Create a new empty registry.
|
||||
pub fn new() -> Self {
|
||||
Self {
|
||||
@@ -206,11 +215,7 @@ impl ToolRegistry {
|
||||
.read()
|
||||
.await
|
||||
.values()
|
||||
.map(|tool| ToolDefinition {
|
||||
name: tool.name().to_string(),
|
||||
description: tool.description().to_string(),
|
||||
parameters: tool.parameters_schema(),
|
||||
})
|
||||
.map(Self::tool_definition)
|
||||
.collect();
|
||||
defs.sort_unstable_by(|a, b| a.name.cmp(&b.name));
|
||||
defs
|
||||
@@ -221,13 +226,7 @@ impl ToolRegistry {
|
||||
let tools = self.tools.read().await;
|
||||
names
|
||||
.iter()
|
||||
.filter_map(|name| {
|
||||
tools.get(*name).map(|tool| ToolDefinition {
|
||||
name: tool.name().to_string(),
|
||||
description: tool.description().to_string(),
|
||||
parameters: tool.parameters_schema(),
|
||||
})
|
||||
})
|
||||
.filter_map(|name| tools.get(*name).map(Self::tool_definition))
|
||||
.collect()
|
||||
}
|
||||
|
||||
@@ -282,11 +281,7 @@ impl ToolRegistry {
|
||||
.await
|
||||
.values()
|
||||
.filter(|tool| tool.domain() == domain)
|
||||
.map(|tool| ToolDefinition {
|
||||
name: tool.name().to_string(),
|
||||
description: tool.description().to_string(),
|
||||
parameters: tool.parameters_schema(),
|
||||
})
|
||||
.map(Self::tool_definition)
|
||||
.collect()
|
||||
}
|
||||
|
||||
@@ -312,11 +307,7 @@ impl ToolRegistry {
|
||||
ApprovalRequirement::Never
|
||||
)
|
||||
})
|
||||
.map(|tool| ToolDefinition {
|
||||
name: tool.name().to_string(),
|
||||
description: tool.description().to_string(),
|
||||
parameters: tool.parameters_schema(),
|
||||
})
|
||||
.map(Self::tool_definition)
|
||||
.collect();
|
||||
defs.sort_unstable_by(|a, b| a.name.cmp(&b.name));
|
||||
defs
|
||||
@@ -788,6 +779,7 @@ impl std::fmt::Debug for ToolRegistry {
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::tools::registry::EchoTool;
|
||||
use crate::tools::tool::ToolDiscoverySummary;
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_register_and_get() {
|
||||
@@ -818,6 +810,71 @@ mod tests {
|
||||
assert_eq!(defs[0].name, "echo");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_tool_definitions_use_tool_schema() {
|
||||
struct DiscoveryTool;
|
||||
|
||||
#[async_trait::async_trait]
|
||||
impl Tool for DiscoveryTool {
|
||||
fn name(&self) -> &str {
|
||||
"discovery_tool"
|
||||
}
|
||||
|
||||
fn description(&self) -> &str {
|
||||
"Discovery test tool"
|
||||
}
|
||||
|
||||
fn parameters_schema(&self) -> serde_json::Value {
|
||||
serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"name": { "type": "string" }
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
fn discovery_schema(&self) -> serde_json::Value {
|
||||
serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"name": { "type": "string" },
|
||||
"extra": { "type": "string" }
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
fn discovery_summary(&self) -> Option<ToolDiscoverySummary> {
|
||||
Some(ToolDiscoverySummary {
|
||||
notes: vec!["extra guidance".into()],
|
||||
..ToolDiscoverySummary::default()
|
||||
})
|
||||
}
|
||||
|
||||
async fn execute(
|
||||
&self,
|
||||
_params: serde_json::Value,
|
||||
_ctx: &crate::context::JobContext,
|
||||
) -> Result<crate::tools::tool::ToolOutput, crate::tools::tool::ToolError> {
|
||||
unreachable!()
|
||||
}
|
||||
}
|
||||
|
||||
let registry = ToolRegistry::new();
|
||||
registry.register(Arc::new(DiscoveryTool)).await;
|
||||
|
||||
let defs = registry.tool_definitions().await;
|
||||
let def = defs
|
||||
.iter()
|
||||
.find(|def| def.name == "discovery_tool")
|
||||
.expect("tool definition should be present");
|
||||
assert!(
|
||||
def.description.contains("tool_info"),
|
||||
"live tool definition should include schema hint: {}",
|
||||
def.description
|
||||
);
|
||||
assert!(def.parameters.get("extra").is_none());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_builtin_tool_cannot_be_shadowed() {
|
||||
let registry = ToolRegistry::new();
|
||||
|
||||
@@ -605,15 +605,7 @@ mod tests {
|
||||
),
|
||||
(
|
||||
"event_emit",
|
||||
serde_json::json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"event_source": { "type": "string", "description": "Event source" },
|
||||
"event_type": { "type": "string", "description": "Event type" },
|
||||
"payload": { "type": "object", "description": "Event payload", "properties": {} }
|
||||
},
|
||||
"required": ["event_source", "event_type"]
|
||||
}),
|
||||
crate::tools::builtin::routine::event_emit_parameters_schema(),
|
||||
),
|
||||
// Job tools with complex deps
|
||||
(
|
||||
|
||||
+35
-2
@@ -231,6 +231,19 @@ impl ToolSchema {
|
||||
}
|
||||
}
|
||||
|
||||
/// Curated discovery guidance surfaced by `tool_info(detail: "summary")`.
|
||||
#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq)]
|
||||
pub struct ToolDiscoverySummary {
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub always_required: Vec<String>,
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub conditional_requirements: Vec<String>,
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub notes: Vec<String>,
|
||||
#[serde(default, skip_serializing_if = "Vec::is_empty")]
|
||||
pub examples: Vec<serde_json::Value>,
|
||||
}
|
||||
|
||||
/// Trait for tools that the agent can use.
|
||||
#[async_trait]
|
||||
pub trait Tool: Send + Sync {
|
||||
@@ -347,12 +360,32 @@ pub trait Tool: Send + Sync {
|
||||
self.parameters_schema()
|
||||
}
|
||||
|
||||
/// Curated discovery guidance used by `tool_info(detail: "summary")`.
|
||||
///
|
||||
/// Default: no custom summary; callers may derive a minimal fallback from
|
||||
/// `discovery_schema()`.
|
||||
fn discovery_summary(&self) -> Option<ToolDiscoverySummary> {
|
||||
None
|
||||
}
|
||||
|
||||
/// Get the tool schema for LLM function calling.
|
||||
fn schema(&self) -> ToolSchema {
|
||||
let parameters = self.parameters_schema();
|
||||
let has_discovery_hint =
|
||||
self.discovery_summary().is_some() || self.discovery_schema() != parameters;
|
||||
let description = if has_discovery_hint {
|
||||
format!(
|
||||
"{} (call tool_info(name: \"{}\", detail: \"summary\") for rules/examples or detail: \"schema\" for the full discovery schema)",
|
||||
self.description(),
|
||||
self.name()
|
||||
)
|
||||
} else {
|
||||
self.description().to_string()
|
||||
};
|
||||
ToolSchema {
|
||||
name: self.name().to_string(),
|
||||
description: self.description().to_string(),
|
||||
parameters: self.parameters_schema(),
|
||||
description,
|
||||
parameters,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user