Merge remote-tracking branch 'origin/pr/1423' into work/v0.8.34
# Conflicts: # crates/tui/src/core/engine.rs # crates/tui/src/core/engine/tool_catalog.rs # crates/tui/src/core/engine/turn_loop.rs
This commit is contained in:
@@ -2052,7 +2052,8 @@ use self::tool_catalog::{
|
||||
};
|
||||
#[cfg(test)]
|
||||
use self::tool_catalog::{
|
||||
TOOL_SEARCH_BM25_NAME, maybe_activate_requested_deferred_tool, should_default_defer_tool,
|
||||
TOOL_SEARCH_BM25_NAME, maybe_activate_requested_deferred_tool,
|
||||
preflight_requested_deferred_tool, should_default_defer_tool,
|
||||
};
|
||||
use self::tool_execution::emit_tool_audit;
|
||||
use self::tool_setup::sandbox_policy_for_mode;
|
||||
|
||||
@@ -507,6 +507,111 @@ fn active_tool_list_pushes_deferred_activations_to_the_tail() {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn deferred_tool_preflight_loads_edit_schema_without_executing_bad_aliases() {
|
||||
let (engine, _handle) = Engine::new(EngineConfig::default(), &Config::default());
|
||||
let registry = engine
|
||||
.build_turn_tool_registry_builder(
|
||||
AppMode::Agent,
|
||||
engine.config.todos.clone(),
|
||||
engine.config.plan_state.clone(),
|
||||
)
|
||||
.build(engine.build_tool_context(AppMode::Agent, false));
|
||||
let catalog = build_model_tool_catalog(
|
||||
registry.to_api_tools_with_cache(true),
|
||||
vec![],
|
||||
AppMode::Agent,
|
||||
);
|
||||
let mut active = initial_active_tools(&catalog);
|
||||
assert!(!active.contains("edit_file"));
|
||||
|
||||
let result = preflight_requested_deferred_tool(
|
||||
"edit_file",
|
||||
&json!({
|
||||
"path": "src/foo.rs",
|
||||
"old_string": "before",
|
||||
"new_string": "after"
|
||||
}),
|
||||
&catalog,
|
||||
&mut active,
|
||||
)
|
||||
.expect("deferred edit_file should preflight");
|
||||
|
||||
assert!(active.contains("edit_file"));
|
||||
assert!(result.success);
|
||||
assert!(result.content.contains("Tool `edit_file` was deferred"));
|
||||
assert!(result.content.contains("The tool was not executed"));
|
||||
assert!(result.content.contains("path: string required"));
|
||||
assert!(result.content.contains("search: string required"));
|
||||
assert!(result.content.contains("replace: string required"));
|
||||
assert!(result.content.contains("old_string -> search"));
|
||||
assert!(result.content.contains("new_string -> replace"));
|
||||
assert_eq!(
|
||||
result.metadata.as_ref().unwrap()["deferred_tool_loaded"],
|
||||
json!(true)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn deferred_tool_preflight_guides_checklist_update_list_replacement() {
|
||||
let (engine, _handle) = Engine::new(EngineConfig::default(), &Config::default());
|
||||
let registry = engine
|
||||
.build_turn_tool_registry_builder(
|
||||
AppMode::Agent,
|
||||
engine.config.todos.clone(),
|
||||
engine.config.plan_state.clone(),
|
||||
)
|
||||
.build(engine.build_tool_context(AppMode::Agent, false));
|
||||
let catalog = build_model_tool_catalog(
|
||||
registry.to_api_tools_with_cache(true),
|
||||
vec![],
|
||||
AppMode::Agent,
|
||||
);
|
||||
let mut active = initial_active_tools(&catalog);
|
||||
assert!(!active.contains("checklist_update"));
|
||||
|
||||
let result = preflight_requested_deferred_tool(
|
||||
"checklist_update",
|
||||
&json!({
|
||||
"todos": [
|
||||
{ "content": "wire preflight", "status": "completed" }
|
||||
]
|
||||
}),
|
||||
&catalog,
|
||||
&mut active,
|
||||
)
|
||||
.expect("deferred checklist_update should preflight");
|
||||
|
||||
assert!(active.contains("checklist_update"));
|
||||
assert!(result.success);
|
||||
assert!(
|
||||
result
|
||||
.content
|
||||
.contains("Tool `checklist_update` was deferred")
|
||||
);
|
||||
assert!(result.content.contains("id: integer required"));
|
||||
assert!(result.content.contains("status: string"));
|
||||
assert!(result.content.contains("Missing required fields:"));
|
||||
assert!(result.content.contains("id, status"));
|
||||
assert!(result.content.contains("Unexpected fields:"));
|
||||
assert!(result.content.contains("todos"));
|
||||
assert!(result.content.contains("Use checklist_write"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn deferred_tool_preflight_skips_already_active_tools() {
|
||||
let mut tool = api_tool("deferred_tool");
|
||||
tool.defer_loading = Some(true);
|
||||
let catalog = vec![tool];
|
||||
let mut active = HashSet::from(["deferred_tool".to_string()]);
|
||||
|
||||
assert!(
|
||||
preflight_requested_deferred_tool("deferred_tool", &json!({}), &catalog, &mut active,)
|
||||
.is_none(),
|
||||
"already active tools should execute normally"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn turn_tool_registry_builder_keeps_plan_mode_read_only_for_files() {
|
||||
let (engine, _handle) = Engine::new(EngineConfig::default(), &Config::default());
|
||||
|
||||
@@ -451,94 +451,147 @@ pub(super) fn maybe_hydrate_requested_deferred_tool(
|
||||
Some(deferred_tool_schema_hydration_result(def, tool_input))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(super) fn preflight_requested_deferred_tool(
|
||||
tool_name: &str,
|
||||
tool_input: &Value,
|
||||
catalog: &[Tool],
|
||||
active_tools: &mut HashSet<String>,
|
||||
) -> Option<ToolResult> {
|
||||
let active_tools_at_batch_start = active_tools.clone();
|
||||
let mut hydrated_tools_this_batch = HashSet::new();
|
||||
let result = maybe_hydrate_requested_deferred_tool(
|
||||
tool_name,
|
||||
tool_input,
|
||||
catalog,
|
||||
&active_tools_at_batch_start,
|
||||
&mut hydrated_tools_this_batch,
|
||||
);
|
||||
active_tools.extend(hydrated_tools_this_batch);
|
||||
result
|
||||
}
|
||||
|
||||
fn deferred_tool_schema_hydration_result(tool: &Tool, tool_input: &Value) -> ToolResult {
|
||||
let expected = schema_field_lines(tool);
|
||||
let expected = schema_fields(&tool.input_schema);
|
||||
let required = schema_required_fields(&tool.input_schema);
|
||||
let received = received_field_names(tool_input);
|
||||
let corrections = likely_field_corrections(&tool.name, &received);
|
||||
let missing = required
|
||||
.iter()
|
||||
.filter(|field| !received.contains(field))
|
||||
.cloned()
|
||||
.collect::<Vec<_>>();
|
||||
let unexpected = received
|
||||
.iter()
|
||||
.filter(|field| !expected.iter().any(|expected| &expected.name == *field))
|
||||
.cloned()
|
||||
.collect::<Vec<_>>();
|
||||
let corrections = likely_field_corrections(&received, &expected, &tool.name);
|
||||
|
||||
let expected_text = if expected.is_empty() {
|
||||
" (no declared fields)".to_string()
|
||||
let mut lines = vec![
|
||||
format!("Tool `{}` was deferred and has now been loaded.", tool.name),
|
||||
String::new(),
|
||||
"The tool was not executed. Retry with the loaded schema.".to_string(),
|
||||
String::new(),
|
||||
"Expected fields:".to_string(),
|
||||
];
|
||||
if expected.is_empty() {
|
||||
lines.push(" (none)".to_string());
|
||||
} else {
|
||||
expected
|
||||
.iter()
|
||||
.map(|field| format!(" {field}"))
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
};
|
||||
let received_text = if received.is_empty() {
|
||||
" (none)".to_string()
|
||||
for field in &expected {
|
||||
let required_marker = if required.contains(&field.name) {
|
||||
" required"
|
||||
} else {
|
||||
""
|
||||
};
|
||||
lines.push(format!(" {}: {}{}", field.name, field.kind, required_marker));
|
||||
}
|
||||
}
|
||||
lines.push(String::new());
|
||||
lines.push("Received fields:".to_string());
|
||||
if received.is_empty() {
|
||||
lines.push(" (none)".to_string());
|
||||
} else {
|
||||
format!(" {}", received.join(", "))
|
||||
};
|
||||
let correction_text = if corrections.is_empty() {
|
||||
String::new()
|
||||
} else {
|
||||
format!(
|
||||
"\n\nLikely correction:\n{}",
|
||||
corrections
|
||||
.iter()
|
||||
.map(|field| format!(" {field}"))
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
)
|
||||
};
|
||||
lines.push(format!(" {}", received.join(", ")));
|
||||
}
|
||||
if !missing.is_empty() {
|
||||
lines.push(String::new());
|
||||
lines.push("Missing required fields:".to_string());
|
||||
lines.push(format!(" {}", missing.join(", ")));
|
||||
}
|
||||
if !unexpected.is_empty() {
|
||||
lines.push(String::new());
|
||||
lines.push("Unexpected fields:".to_string());
|
||||
lines.push(format!(" {}", unexpected.join(", ")));
|
||||
}
|
||||
if !corrections.is_empty() {
|
||||
lines.push(String::new());
|
||||
lines.push("Likely corrections:".to_string());
|
||||
for correction in &corrections {
|
||||
lines.push(format!(" {correction}"));
|
||||
}
|
||||
}
|
||||
|
||||
ToolResult::success(format!(
|
||||
"Tool `{}` was deferred and has now been loaded.\n\nExpected schema:\n{}\n\nReceived fields:\n{}{}\n\nThe tool was not executed. Retry the same operation with the loaded schema.",
|
||||
tool.name, expected_text, received_text, correction_text
|
||||
))
|
||||
ToolResult::success(lines.join("\n"))
|
||||
.with_metadata(json!({
|
||||
"event": "tool.schema_hydrated",
|
||||
"tool": tool.name,
|
||||
"executed": false,
|
||||
"retry_required": true,
|
||||
"reason": "deferred_tool_first_use",
|
||||
"deferred_tool_loaded": true,
|
||||
"tool_name": tool.name,
|
||||
"expected_fields": expected.iter().map(|field| field.name.clone()).collect::<Vec<_>>(),
|
||||
"received_fields": received,
|
||||
"missing_required_fields": missing,
|
||||
"unexpected_fields": unexpected,
|
||||
"likely_corrections": corrections,
|
||||
}))
|
||||
}
|
||||
|
||||
fn schema_field_lines(tool: &Tool) -> Vec<String> {
|
||||
let mut required = Vec::new();
|
||||
if let Some(items) = tool.input_schema.get("required").and_then(Value::as_array) {
|
||||
for item in items {
|
||||
if let Some(field) = item.as_str() {
|
||||
required.push(field.to_string());
|
||||
}
|
||||
}
|
||||
}
|
||||
#[derive(Debug, Clone)]
|
||||
struct SchemaField {
|
||||
name: String,
|
||||
kind: String,
|
||||
}
|
||||
|
||||
let Some(properties) = tool
|
||||
.input_schema
|
||||
.get("properties")
|
||||
.and_then(Value::as_object)
|
||||
else {
|
||||
return required;
|
||||
fn schema_fields(schema: &Value) -> Vec<SchemaField> {
|
||||
let Some(properties) = schema.get("properties").and_then(Value::as_object) else {
|
||||
return Vec::new();
|
||||
};
|
||||
|
||||
let mut fields = Vec::new();
|
||||
let mut seen = HashSet::new();
|
||||
for field in &required {
|
||||
if let Some(schema) = properties.get(field) {
|
||||
fields.push(format!("{field}: {}", schema_type_label(schema)));
|
||||
seen.insert(field.as_str());
|
||||
} else {
|
||||
fields.push(field.clone());
|
||||
}
|
||||
}
|
||||
for (field, schema) in properties {
|
||||
if seen.contains(field.as_str()) {
|
||||
continue;
|
||||
}
|
||||
fields.push(format!("{field}: {} (optional)", schema_type_label(schema)));
|
||||
}
|
||||
let mut fields = properties
|
||||
.iter()
|
||||
.map(|(name, spec)| SchemaField {
|
||||
name: name.clone(),
|
||||
kind: schema_type_label(spec),
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
fields.sort_by(|a, b| a.name.cmp(&b.name));
|
||||
fields
|
||||
}
|
||||
|
||||
fn schema_type_label(schema: &Value) -> String {
|
||||
schema
|
||||
.get("type")
|
||||
.and_then(Value::as_str)
|
||||
.unwrap_or("value")
|
||||
.to_string()
|
||||
fn schema_required_fields(schema: &Value) -> Vec<String> {
|
||||
let mut required = schema
|
||||
.get("required")
|
||||
.and_then(Value::as_array)
|
||||
.into_iter()
|
||||
.flatten()
|
||||
.filter_map(|value| value.as_str().map(str::to_string))
|
||||
.collect::<Vec<_>>();
|
||||
required.sort();
|
||||
required
|
||||
}
|
||||
|
||||
fn schema_type_label(spec: &Value) -> String {
|
||||
let Some(kind) = spec.get("type").and_then(Value::as_str) else {
|
||||
return "value".to_string();
|
||||
};
|
||||
if let Some(values) = spec.get("enum").and_then(Value::as_array) {
|
||||
let labels = values.iter().filter_map(Value::as_str).collect::<Vec<_>>();
|
||||
if !labels.is_empty() {
|
||||
return format!("{kind} ({})", labels.join(" | "));
|
||||
}
|
||||
}
|
||||
kind.to_string()
|
||||
}
|
||||
|
||||
fn received_field_names(input: &Value) -> Vec<String> {
|
||||
@@ -550,25 +603,33 @@ fn received_field_names(input: &Value) -> Vec<String> {
|
||||
fields
|
||||
}
|
||||
|
||||
fn likely_field_corrections(tool_name: &str, received: &[String]) -> Vec<String> {
|
||||
if tool_name != "edit_file" {
|
||||
return Vec::new();
|
||||
}
|
||||
|
||||
let has = |name: &str| received.iter().any(|field| field == name);
|
||||
fn likely_field_corrections(
|
||||
received: &[String],
|
||||
expected: &[SchemaField],
|
||||
tool_name: &str,
|
||||
) -> Vec<String> {
|
||||
let has_expected = |name: &str| expected.iter().any(|field| field.name == name);
|
||||
let has_received = |name: &str| received.iter().any(|field| field == name);
|
||||
let mut corrections = Vec::new();
|
||||
if has("old_string") {
|
||||
|
||||
if has_received("old_string") && has_expected("search") {
|
||||
corrections.push("old_string -> search".to_string());
|
||||
} else if has("old_str") {
|
||||
} else if has_received("old_str") && has_expected("search") {
|
||||
corrections.push("old_str -> search".to_string());
|
||||
}
|
||||
if has("new_string") {
|
||||
if has_received("new_string") && has_expected("replace") {
|
||||
corrections.push("new_string -> replace".to_string());
|
||||
} else if has("new_str") {
|
||||
} else if has_received("new_str") && has_expected("replace") {
|
||||
corrections.push("new_str -> replace".to_string());
|
||||
} else if has("replacement") {
|
||||
} else if has_received("replacement") && has_expected("replace") {
|
||||
corrections.push("replacement -> replace".to_string());
|
||||
}
|
||||
if tool_name == "checklist_update" && has_received("todos") {
|
||||
corrections.push(
|
||||
"Use checklist_write to replace the full list, or retry checklist_update with id and status."
|
||||
.to_string(),
|
||||
);
|
||||
}
|
||||
corrections
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user