fix(tools): refuse unknown parameters on every File action, not just edit
#5209 taught `edit` to hard-error on a parameter it does not implement instead of dropping it and returning a success-shaped receipt. Only `edit` learned it. `read`, `write`, `list`, `search_name`, `search_content`, and `patch` all kept discarding unknown keys silently, which is the same failure wearing a quieter costume: a misspelled `start_line` on `read` was dropped, the head of the file came back under a success receipt, and nothing in the response admitted the requested window was never honored — a wrong answer shaped like a right one. Found by asking the release binary's model to probe its own tools: it called File{action:"read", path, bogus_param} and reported "no schema validation error and no rejection: the call was accepted and executed, the unknown parameter was silently dropped." `validate_edit_file_params` becomes one `ActionParams` table covering all seven actions, with the same error shape everywhere — it names the offending parameter, the allowed set, the required set, and states that the operation was not performed. Validation runs after `apply_param_aliases`, exactly as the edit path did: the alias lane's reasoning stands, so an unambiguous cross-harness synonym is still translated and only a name with no known meaning is refused. The wrapper's hand-copied cross-action forwarding (`max_results` on search_name, `query`/`limit` on search_content) moves into that same alias mechanism, so those spellings survive the new check and a direct call to the implementing tool behaves identically to a call through `File`. `fuzz` on `edit` is retired rather than kept honest. It was advertised in the schema and read into `let _fuzz`, then thrown away; asked about it live, the model described it as "an optional fuzzy-matching flag for the search", so the previous attempt to make the description honest did not land. The fuzzy fallbacks it appeared to control (indentation, punctuation, line endings) run unconditionally and are unaffected. `fuzz` remains a real integer parameter on `patch`, and the `File` wrapper now borrows its description from the action that implements it. Net effect on the per-turn tool catalog: -91 bytes and -23 estimated tokens in every mode and both surfaces. Tests: every action refuses an unknown parameter with the full error shape; the misspelled read window specifically; every action still accepts its complete legitimate parameter set; every alias still survives validation; parameters do not leak between actions; and required names are always a subset of allowed ones. Gaps left standing, for a follow-up: `Git`, `Web`, `Run`, and `Bash` still drop unknown parameters on every action, and `Bash` alone still resolves a non-string `action` to its `run` default instead of refusing it.
This commit is contained in:
Hmbown
@@ -6316,9 +6316,11 @@ fn agent_catalog_keeps_canonical_file_tool_loaded() {
|
||||
.any(|field| field.as_str() == Some("action"))
|
||||
);
|
||||
assert!(!required.iter().any(|field| field.as_str() == Some("fuzz")));
|
||||
// `fuzz` is `patch`'s integer fuzz factor and nothing else; the boolean
|
||||
// half of the old `oneOf` existed only for the inert `edit` flag.
|
||||
assert_eq!(
|
||||
file.input_schema["properties"]["fuzz"]["oneOf"][0]["type"].as_str(),
|
||||
Some("boolean"),
|
||||
file.input_schema["properties"]["fuzz"]["type"].as_str(),
|
||||
Some("integer"),
|
||||
);
|
||||
|
||||
let active_at_batch_start = initial_active_tools(&catalog);
|
||||
|
||||
@@ -13,6 +13,7 @@ use serde_json::{Value, json};
|
||||
use thiserror::Error;
|
||||
|
||||
use super::diff_format::make_unified_diff;
|
||||
use super::file::{PATCH_PARAMS, PATH_ALIASES, apply_param_aliases};
|
||||
use super::spec::{
|
||||
ApprovalRequirement, ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec,
|
||||
lsp_diagnostics_for_paths, optional_bool, optional_str, optional_u64,
|
||||
@@ -385,6 +386,11 @@ impl ToolSpec for ApplyPatchTool {
|
||||
}
|
||||
|
||||
async fn execute(&self, input: Value, context: &ToolContext) -> Result<ToolResult, ToolError> {
|
||||
let mut input = input;
|
||||
apply_param_aliases(&mut input, PATH_ALIASES, "File patch")?;
|
||||
PATCH_PARAMS.reject_unknown(&input)?;
|
||||
let input = input;
|
||||
|
||||
let fuzz = optional_u64(&input, "fuzz", DEFAULT_FUZZ as u64)?.min(MAX_FUZZ as u64);
|
||||
let fuzz = usize::try_from(fuzz).unwrap_or(DEFAULT_FUZZ);
|
||||
let normalized = normalize_apply_patch_input(&input)?;
|
||||
|
||||
+185
-44
@@ -10,7 +10,7 @@
|
||||
use super::diff_format::make_unified_diff;
|
||||
use super::spec::{
|
||||
ApprovalRequirement, ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec,
|
||||
lsp_diagnostics_for_paths, optional_bool, optional_str, required_str,
|
||||
lsp_diagnostics_for_paths, optional_str, required_str,
|
||||
};
|
||||
use async_trait::async_trait;
|
||||
use serde_json::{Value, json};
|
||||
@@ -40,7 +40,7 @@ use tokio_util::sync::CancellationToken;
|
||||
/// value is an error rather than a coin flip, and any parameter that is not a
|
||||
/// known synonym still fails validation. The #5209 guarantee — no fabricated
|
||||
/// "Replaced 1 occurrence" for an edit that never landed — is unchanged.
|
||||
struct ParamAlias {
|
||||
pub(super) struct ParamAlias {
|
||||
/// Spelling a model might emit.
|
||||
alias: &'static str,
|
||||
/// Parameter this tool implements.
|
||||
@@ -54,7 +54,8 @@ const fn alias(alias: &'static str, canonical: &'static str) -> ParamAlias {
|
||||
/// Path spellings shared by every file action. `path` is CodeWhale's
|
||||
/// canonical name and the most common one in the field, but `file_path` is
|
||||
/// widespread enough in training data to be worth accepting everywhere.
|
||||
const PATH_ALIASES: &[ParamAlias] = &[alias("file_path", "path"), alias("filePath", "path")];
|
||||
pub(super) const PATH_ALIASES: &[ParamAlias] =
|
||||
&[alias("file_path", "path"), alias("filePath", "path")];
|
||||
|
||||
/// Edit-specific spellings. Ordered most- to least-common.
|
||||
const EDIT_ALIASES: &[ParamAlias] = &[
|
||||
@@ -82,13 +83,28 @@ const READ_ALIASES: &[ParamAlias] = &[
|
||||
alias("num_lines", "max_lines"),
|
||||
];
|
||||
|
||||
/// `search_name` spellings. The `File` wrapper advertises `max_results` for
|
||||
/// both search actions, but only `search_content` implements that name; on
|
||||
/// `search_name` the same number is spelled `limit`. Folding it here (rather
|
||||
/// than copying it inside the wrapper) keeps one alias mechanism, so the
|
||||
/// result-count cap a model asks for is the cap it gets whichever name it
|
||||
/// reaches for, and a direct `file_search` call behaves the same way.
|
||||
pub(super) const SEARCH_NAME_ALIASES: &[ParamAlias] = &[alias("max_results", "limit")];
|
||||
|
||||
/// `search_content` spellings, mirroring `SEARCH_NAME_ALIASES` in the other
|
||||
/// direction: the wrapper advertises `query` and `limit` on the name-search
|
||||
/// side, and a model that carries them across to a content search means
|
||||
/// `pattern` and `max_results`.
|
||||
pub(super) const SEARCH_CONTENT_ALIASES: &[ParamAlias] =
|
||||
&[alias("query", "pattern"), alias("limit", "max_results")];
|
||||
|
||||
/// Apply `aliases` to `input`, in place.
|
||||
///
|
||||
/// An alias is consumed only when the canonical key is absent. When both are
|
||||
/// present and *equal* the alias is dropped as a harmless duplicate; when both
|
||||
/// are present and disagree the call fails, because guessing which one the
|
||||
/// model meant is exactly the fabrication this path exists to prevent.
|
||||
fn apply_param_aliases(
|
||||
pub(super) fn apply_param_aliases(
|
||||
input: &mut Value,
|
||||
aliases: &[ParamAlias],
|
||||
tool_label: &str,
|
||||
@@ -117,6 +133,163 @@ fn apply_param_aliases(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
// === Per-action parameter contracts ===
|
||||
|
||||
/// The parameter contract for one `File` action.
|
||||
///
|
||||
/// #5209 taught `edit` to refuse a parameter it does not implement instead of
|
||||
/// dropping it and returning a success-shaped receipt. Only `edit` learned it.
|
||||
/// Every other action kept silently discarding unknown keys, and for a reader
|
||||
/// that is the same failure wearing a quieter costume: a misspelled
|
||||
/// `start_line` on `read` is dropped, the head of the file comes back, and
|
||||
/// nothing in the response says the requested window was never honored — a
|
||||
/// wrong answer shaped like a right one.
|
||||
///
|
||||
/// One table, one error shape, every action.
|
||||
pub(super) struct ActionParams {
|
||||
/// Action name as the model spells it on `File` (`read`, `write`, …).
|
||||
action: &'static str,
|
||||
/// Every parameter the action implements, canonical spellings only.
|
||||
/// Aliases are folded onto these by [`apply_param_aliases`] before
|
||||
/// validation runs, so they must not be listed here.
|
||||
allowed: &'static [&'static str],
|
||||
/// Parameters the action cannot run without.
|
||||
required: &'static [&'static str],
|
||||
/// `true` when exactly one of `required` is needed rather than all of
|
||||
/// them — `patch` accepts `patch`, `replace`, or `changes`.
|
||||
required_is_choice: bool,
|
||||
}
|
||||
|
||||
const fn params(
|
||||
action: &'static str,
|
||||
allowed: &'static [&'static str],
|
||||
required: &'static [&'static str],
|
||||
) -> ActionParams {
|
||||
ActionParams {
|
||||
action,
|
||||
allowed,
|
||||
required,
|
||||
required_is_choice: false,
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) const READ_PARAMS: ActionParams = params(
|
||||
"read",
|
||||
&["path", "start_line", "max_lines", "pages"],
|
||||
&["path"],
|
||||
);
|
||||
|
||||
pub(super) const WRITE_PARAMS: ActionParams =
|
||||
params("write", &["path", "content"], &["path", "content"]);
|
||||
|
||||
pub(super) const EDIT_PARAMS: ActionParams = params(
|
||||
"edit",
|
||||
&["path", "search", "replace"],
|
||||
&["path", "search", "replace"],
|
||||
);
|
||||
|
||||
pub(super) const LIST_PARAMS: ActionParams = params("list", &["path"], &[]);
|
||||
|
||||
pub(super) const SEARCH_NAME_PARAMS: ActionParams = params(
|
||||
"search_name",
|
||||
&["query", "path", "limit", "extensions", "exclude"],
|
||||
&["query"],
|
||||
);
|
||||
|
||||
pub(super) const SEARCH_CONTENT_PARAMS: ActionParams = params(
|
||||
"search_content",
|
||||
&[
|
||||
"pattern",
|
||||
"path",
|
||||
"include",
|
||||
"exclude",
|
||||
"context_lines",
|
||||
"case_insensitive",
|
||||
"max_results",
|
||||
],
|
||||
&["pattern"],
|
||||
);
|
||||
|
||||
pub(super) const PATCH_PARAMS: ActionParams = ActionParams {
|
||||
action: "patch",
|
||||
allowed: &[
|
||||
"path",
|
||||
"patch",
|
||||
"replace",
|
||||
"changes",
|
||||
"fuzz",
|
||||
"create_if_missing",
|
||||
],
|
||||
required: &["patch", "replace", "changes"],
|
||||
required_is_choice: true,
|
||||
};
|
||||
|
||||
/// Render `names` as a backticked, comma-separated English list.
|
||||
fn quoted_list(names: &[&str], conjunction: &str) -> String {
|
||||
let quoted: Vec<String> = names.iter().map(|name| format!("`{name}`")).collect();
|
||||
match quoted.as_slice() {
|
||||
[] => "none".to_string(),
|
||||
[only] => only.clone(),
|
||||
[first, second] => format!("{first} {conjunction} {second}"),
|
||||
[head @ .., last] => format!("{}, {conjunction} {last}", head.join(", ")),
|
||||
}
|
||||
}
|
||||
|
||||
impl ActionParams {
|
||||
/// Reject parameter names this action does not implement.
|
||||
///
|
||||
/// Must run *after* [`apply_param_aliases`], exactly as the `edit` path
|
||||
/// does. The alias lane's reasoning stands: translating an unambiguous
|
||||
/// synonym is better than refusing it, so by the time this runs every
|
||||
/// spelling with a known meaning has already been folded onto its
|
||||
/// canonical name. What is left is a name with no known meaning, where
|
||||
/// continuing would mean guessing which argument was intended — so it
|
||||
/// hard-errors rather than dropping the argument and reporting success.
|
||||
pub(super) fn reject_unknown(&self, input: &Value) -> Result<(), ToolError> {
|
||||
let action = self.action;
|
||||
let required = if self.required_is_choice {
|
||||
format!("one of {}", quoted_list(self.required, "or"))
|
||||
} else {
|
||||
quoted_list(self.required, "and")
|
||||
};
|
||||
|
||||
let Some(obj) = input.as_object() else {
|
||||
return Err(ToolError::invalid_input(format!(
|
||||
"File {action} input must be an object. Allowed parameters are {}. Required: {required}. The {action} was not performed.",
|
||||
quoted_list(self.allowed, "and"),
|
||||
)));
|
||||
};
|
||||
|
||||
let unexpected: Vec<&str> = obj
|
||||
.keys()
|
||||
.map(String::as_str)
|
||||
.filter(|key| !self.allowed.contains(key))
|
||||
.collect();
|
||||
if !unexpected.is_empty() {
|
||||
return Err(ToolError::invalid_input(format!(
|
||||
"unexpected File {action} parameter(s): {}. Allowed parameters are {}. Required: {required}. The {action} was not performed.",
|
||||
unexpected.join(", "),
|
||||
quoted_list(self.allowed, "and"),
|
||||
)));
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// A required parameter that is not also allowed would make the refusal
|
||||
/// self-contradicting: it would name an argument the same check rejects.
|
||||
#[cfg(test)]
|
||||
pub(super) fn assert_required_is_allowed(&self) {
|
||||
for name in self.required {
|
||||
assert!(
|
||||
self.allowed.contains(name),
|
||||
"File {} requires `{name}` but does not allow it",
|
||||
self.action
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// === ReadFileTool ===
|
||||
|
||||
fn canonical_path_for_credential_guard(path: &Path) -> PathBuf {
|
||||
@@ -238,6 +411,7 @@ impl ToolSpec for ReadFileTool {
|
||||
let mut input = input;
|
||||
apply_param_aliases(&mut input, PATH_ALIASES, "File read")?;
|
||||
apply_param_aliases(&mut input, READ_ALIASES, "File read")?;
|
||||
READ_PARAMS.reject_unknown(&input)?;
|
||||
|
||||
let path_str = required_str(&input, "path")?;
|
||||
let file_path = context.resolve_path(path_str)?;
|
||||
@@ -735,6 +909,7 @@ impl ToolSpec for WriteFileTool {
|
||||
async fn execute(&self, input: Value, context: &ToolContext) -> Result<ToolResult, ToolError> {
|
||||
let mut input = input;
|
||||
apply_param_aliases(&mut input, PATH_ALIASES, "File write")?;
|
||||
WRITE_PARAMS.reject_unknown(&input)?;
|
||||
|
||||
let path_str = required_str(&input, "path")?;
|
||||
let file_content = required_str(&input, "content")?;
|
||||
@@ -818,7 +993,7 @@ impl ToolSpec for EditFileTool {
|
||||
}
|
||||
|
||||
fn description(&self) -> &'static str {
|
||||
"Replace text in a single file via exact search/replace after the file has been read with File `read` in this session. Use this instead of `sed -i` in `Bash` for one unambiguous in-place edit. `search` must match exactly one location by default; when no exact match is found the tool retries with leading-whitespace-tolerant fuzzy matching automatically. The optional `fuzz` parameter is accepted for backward compatibility and is no longer needed. Returns a compact unified diff, not the full file. For structural, multi-block, or cross-file changes, use File `patch` or `write` instead."
|
||||
"Replace text in a single file via exact search/replace after the file has been read with File `read` in this session. Use this instead of `sed -i` in `Bash` for one unambiguous in-place edit. `search` must match exactly one location by default; when no exact match is found the tool retries with leading-whitespace-tolerant fuzzy matching automatically. Returns a compact unified diff, not the full file. For structural, multi-block, or cross-file changes, use File `patch` or `write` instead."
|
||||
}
|
||||
|
||||
fn input_schema(&self) -> Value {
|
||||
@@ -836,10 +1011,6 @@ impl ToolSpec for EditFileTool {
|
||||
"replace": {
|
||||
"type": "string",
|
||||
"description": "Text to replace with. Aliases: `new_string`, `new_str`, `newText`"
|
||||
},
|
||||
"fuzz": {
|
||||
"type": "boolean",
|
||||
"description": "Deprecated: fuzzy fallback is now automatic. Accepted for backward compatibility but ignored."
|
||||
}
|
||||
},
|
||||
"required": ["path", "search", "replace"]
|
||||
@@ -868,12 +1039,11 @@ impl ToolSpec for EditFileTool {
|
||||
let mut input = input;
|
||||
apply_param_aliases(&mut input, PATH_ALIASES, "File edit")?;
|
||||
apply_param_aliases(&mut input, EDIT_ALIASES, "File edit")?;
|
||||
validate_edit_file_params(&input)?;
|
||||
EDIT_PARAMS.reject_unknown(&input)?;
|
||||
|
||||
let path_str = required_str(&input, "path")?;
|
||||
let search = required_str(&input, "search")?;
|
||||
let replace = required_str(&input, "replace")?;
|
||||
let _fuzz = optional_bool(&input, "fuzz", false)?;
|
||||
|
||||
if search == replace {
|
||||
// #5003 — long-text edits repeatedly failed here because the model
|
||||
@@ -1075,39 +1245,6 @@ impl ToolSpec for EditFileTool {
|
||||
}
|
||||
}
|
||||
|
||||
/// Reject parameter names for File/`edit_file` that this tool does not
|
||||
/// implement, before any mutation or success-shaped receipt is produced
|
||||
/// (#5209).
|
||||
///
|
||||
/// Runs *after* [`apply_param_aliases`], so the cross-harness spellings a
|
||||
/// model is most likely to reach for have already been folded onto
|
||||
/// `search`/`replace`/`path` and are not seen here. What remains is a name
|
||||
/// with no known meaning, where continuing would mean guessing — so this
|
||||
/// still hard-errors rather than dropping the argument and reporting success.
|
||||
fn validate_edit_file_params(input: &Value) -> Result<(), ToolError> {
|
||||
let Some(obj) = input.as_object() else {
|
||||
return Err(ToolError::invalid_input(
|
||||
"File edit input must be an object with `path`, `search`, and `replace`",
|
||||
));
|
||||
};
|
||||
|
||||
const ALLOWED: &[&str] = &["path", "search", "replace", "fuzz"];
|
||||
|
||||
let unexpected: Vec<&str> = obj
|
||||
.keys()
|
||||
.map(String::as_str)
|
||||
.filter(|key| !ALLOWED.contains(key))
|
||||
.collect();
|
||||
if !unexpected.is_empty() {
|
||||
return Err(ToolError::invalid_input(format!(
|
||||
"unexpected File edit parameter(s): {}. Allowed parameters are `path`, `search`, `replace`, and optional `fuzz`. Required: `path`, `search`, `replace`. The edit was not applied.",
|
||||
unexpected.join(", ")
|
||||
)));
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Detect catastrophic argument corruption of brace-structured edits.
|
||||
///
|
||||
/// Models (and some host XML/JSON bridges) occasionally deliver a `replace`
|
||||
@@ -1534,6 +1671,10 @@ impl ToolSpec for ListDirTool {
|
||||
}
|
||||
|
||||
async fn execute(&self, input: Value, context: &ToolContext) -> Result<ToolResult, ToolError> {
|
||||
let mut input = input;
|
||||
apply_param_aliases(&mut input, PATH_ALIASES, "File list")?;
|
||||
LIST_PARAMS.reject_unknown(&input)?;
|
||||
|
||||
let path_str = optional_str(&input, "path")?.unwrap_or(".");
|
||||
let dir_path = context.resolve_path(path_str)?;
|
||||
|
||||
|
||||
@@ -1205,40 +1205,43 @@ async fn edit_file_rejects_non_unique_exact_match() {
|
||||
assert_eq!(unchanged, "hello world hello");
|
||||
}
|
||||
|
||||
/// `fuzz` on `edit` was an advertised parameter with no implementation: it
|
||||
/// was parsed into `let _fuzz` and thrown away, and a live model read the
|
||||
/// schema as offering "an optional fuzzy-matching flag for the search". The
|
||||
/// advertisement is gone, so the name now means nothing to `edit` and is
|
||||
/// refused like any other name with no known meaning — the fuzzy fallbacks it
|
||||
/// appeared to control run unconditionally either way.
|
||||
#[tokio::test]
|
||||
async fn test_edit_file_accepts_omitted_and_explicit_fuzz() {
|
||||
async fn edit_file_refuses_the_retired_fuzz_parameter() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
let ctx = ToolContext::new(tmp.path().to_path_buf());
|
||||
let tool = EditFileTool;
|
||||
let test_file = tmp.path().join("fuzz_retired.txt");
|
||||
fs::write(&test_file, "hello world").expect("write");
|
||||
read_before_edit(&ctx, "fuzz_retired.txt").await;
|
||||
|
||||
for (file_name, fuzz) in [
|
||||
("fuzz_omitted.txt", None),
|
||||
("fuzz_false.txt", Some(false)),
|
||||
("fuzz_true.txt", Some(true)),
|
||||
] {
|
||||
let test_file = tmp.path().join(file_name);
|
||||
fs::write(&test_file, "hello world").expect("write");
|
||||
read_before_edit(&ctx, file_name).await;
|
||||
|
||||
let mut input = serde_json::Map::from_iter([
|
||||
("path".to_string(), json!(file_name)),
|
||||
("search".to_string(), json!("hello")),
|
||||
("replace".to_string(), json!("hi")),
|
||||
]);
|
||||
if let Some(fuzz) = fuzz {
|
||||
input.insert("fuzz".to_string(), json!(fuzz));
|
||||
}
|
||||
|
||||
let result = tool
|
||||
.execute(Value::Object(input), &ctx)
|
||||
.await
|
||||
.expect("execute");
|
||||
|
||||
assert!(result.success, "{file_name}: {}", result.content);
|
||||
assert!(result.content.contains("Replaced 1 occurrence"));
|
||||
let edited = fs::read_to_string(&test_file).expect("read");
|
||||
assert_eq!(edited, "hi world");
|
||||
}
|
||||
let err = EditFileTool
|
||||
.execute(
|
||||
json!({
|
||||
"path": "fuzz_retired.txt",
|
||||
"search": "hello",
|
||||
"replace": "hi",
|
||||
"fuzz": true,
|
||||
}),
|
||||
&ctx,
|
||||
)
|
||||
.await
|
||||
.expect_err("a parameter edit does not implement must be refused");
|
||||
let msg = err.to_string();
|
||||
assert!(msg.contains("fuzz"), "must name the parameter: {msg}");
|
||||
assert!(
|
||||
msg.contains("was not performed"),
|
||||
"must deny having edited: {msg}"
|
||||
);
|
||||
assert_eq!(
|
||||
fs::read_to_string(&test_file).expect("read"),
|
||||
"hello world",
|
||||
"a refused edit must not touch the file"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -1283,8 +1286,7 @@ async fn test_edit_file_fuzz_tolerates_leading_whitespace() {
|
||||
json!({
|
||||
"path": "fuzzy.txt",
|
||||
"search": "if true {\n let value = 1;\n}",
|
||||
"replace": " if true {\n let value = 2;\n }",
|
||||
"fuzz": true
|
||||
"replace": " if true {\n let value = 2;\n }"
|
||||
}),
|
||||
&ctx,
|
||||
)
|
||||
@@ -1315,8 +1317,7 @@ async fn test_edit_file_fuzz_tolerates_leading_whitespace_after_multibyte_start(
|
||||
json!({
|
||||
"path": "fuzzy_cjk.txt",
|
||||
"search": " 数据",
|
||||
"replace": "记录",
|
||||
"fuzz": true
|
||||
"replace": "记录"
|
||||
}),
|
||||
&ctx,
|
||||
)
|
||||
@@ -1348,8 +1349,7 @@ async fn test_edit_file_fuzz_tolerates_smart_quote_substitution() {
|
||||
"path": "smart.rs",
|
||||
// \u{201C} \u{201D} are the curly double-quote pair.
|
||||
"search": "let s = \u{201C}hello world\u{201D};",
|
||||
"replace": "let s = \"hello universe\";",
|
||||
"fuzz": true
|
||||
"replace": "let s = \"hello universe\";"
|
||||
}),
|
||||
&ctx,
|
||||
)
|
||||
@@ -1381,8 +1381,7 @@ async fn test_edit_file_fuzz_tolerates_smart_quote_after_multibyte_start() {
|
||||
json!({
|
||||
"path": "smart_cjk.md",
|
||||
"search": "数据 \u{201C}x\u{201D}",
|
||||
"replace": "数据 y",
|
||||
"fuzz": true
|
||||
"replace": "数据 y"
|
||||
}),
|
||||
&ctx,
|
||||
)
|
||||
@@ -1413,8 +1412,7 @@ async fn test_edit_file_fuzz_tolerates_em_dash_and_nbsp() {
|
||||
// Search uses em-dash + NBSP, common after a copy-paste
|
||||
// from a styled document.
|
||||
"search": "alpha\u{00A0}\u{2014}\u{00A0}beta",
|
||||
"replace": "alpha - gamma",
|
||||
"fuzz": true
|
||||
"replace": "alpha - gamma"
|
||||
}),
|
||||
&ctx,
|
||||
)
|
||||
@@ -2091,10 +2089,10 @@ fn test_input_schemas() {
|
||||
let required_fields: Vec<_> = required.iter().filter_map(|value| value.as_str()).collect();
|
||||
assert_eq!(required_fields, vec!["path", "search", "replace"]);
|
||||
assert!(!required_fields.contains(&"fuzz"));
|
||||
assert_eq!(
|
||||
edit_schema["properties"]["fuzz"]["type"].as_str(),
|
||||
Some("boolean")
|
||||
);
|
||||
// `fuzz` was never read by `edit` — it was parsed into a discarded
|
||||
// binding while the schema advertised it. An unimplemented parameter has
|
||||
// no place in a schema the model is asked to trust.
|
||||
assert!(edit_schema["properties"].get("fuzz").is_none());
|
||||
let search_desc = edit_schema["properties"]["search"]["description"]
|
||||
.as_str()
|
||||
.expect("search description");
|
||||
|
||||
@@ -12,6 +12,7 @@ use tokio_util::sync::CancellationToken;
|
||||
|
||||
use crate::tools::search::matches_glob;
|
||||
|
||||
use super::file::{PATH_ALIASES, SEARCH_NAME_ALIASES, SEARCH_NAME_PARAMS, apply_param_aliases};
|
||||
use super::spec::{
|
||||
ApprovalRequirement, ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec,
|
||||
optional_str, optional_u64, required_str,
|
||||
@@ -82,6 +83,11 @@ impl ToolSpec for FileSearchTool {
|
||||
}
|
||||
|
||||
async fn execute(&self, input: Value, context: &ToolContext) -> Result<ToolResult, ToolError> {
|
||||
let mut input = input;
|
||||
apply_param_aliases(&mut input, PATH_ALIASES, "File search_name")?;
|
||||
apply_param_aliases(&mut input, SEARCH_NAME_ALIASES, "File search_name")?;
|
||||
SEARCH_NAME_PARAMS.reject_unknown(&input)?;
|
||||
|
||||
let query = required_str(&input, "query")?.trim();
|
||||
if query.is_empty() {
|
||||
return Err(ToolError::invalid_input("query cannot be empty"));
|
||||
|
||||
@@ -167,6 +167,7 @@ impl ToolSpec for FileTool {
|
||||
let content_search = GrepFilesTool.input_schema();
|
||||
let write = WriteFileTool.input_schema();
|
||||
let edit = EditFileTool.input_schema();
|
||||
let patch = ApplyPatchTool.input_schema();
|
||||
json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
@@ -209,11 +210,8 @@ impl ToolSpec for FileTool {
|
||||
]
|
||||
},
|
||||
"fuzz": {
|
||||
"oneOf": [{ "type": "boolean" }, { "type": "integer" }],
|
||||
"description": format!(
|
||||
"{} Integer: max fuzz for action=patch.",
|
||||
borrowed(&edit, "fuzz"),
|
||||
)
|
||||
"type": "integer",
|
||||
"description": describe(&patch, "fuzz", "action=patch")
|
||||
},
|
||||
"query": {
|
||||
"type": "string",
|
||||
@@ -334,35 +332,19 @@ impl ToolSpec for FileTool {
|
||||
self.available_actions().join(", ")
|
||||
)));
|
||||
}
|
||||
let mut input = self.strip_action(input)?;
|
||||
let input = self.strip_action(input)?;
|
||||
|
||||
match action.as_str() {
|
||||
"read" => ReadFileTool.execute(input, context).await,
|
||||
"list" => ListDirTool.execute(input, context).await,
|
||||
"search_name" => {
|
||||
if let Some(obj) = input.as_object_mut()
|
||||
&& !obj.contains_key("limit")
|
||||
&& let Some(max) = obj.get("max_results").cloned()
|
||||
{
|
||||
obj.insert("limit".to_string(), max);
|
||||
}
|
||||
FileSearchTool.execute(input, context).await
|
||||
}
|
||||
"search_content" => {
|
||||
if let Some(obj) = input.as_object_mut() {
|
||||
if !obj.contains_key("pattern")
|
||||
&& let Some(query) = obj.get("query").cloned()
|
||||
{
|
||||
obj.insert("pattern".to_string(), query);
|
||||
}
|
||||
if !obj.contains_key("max_results")
|
||||
&& let Some(limit) = obj.get("limit").cloned()
|
||||
{
|
||||
obj.insert("max_results".to_string(), limit);
|
||||
}
|
||||
}
|
||||
GrepFilesTool.execute(input, context).await
|
||||
}
|
||||
// The cross-action spellings the wrapper advertises
|
||||
// (`max_results` on search_name, `query`/`limit` on
|
||||
// search_content) used to be copied here. They are alias-table
|
||||
// entries on the implementing tools now — one mechanism, applied
|
||||
// before the same unknown-parameter check every other action runs,
|
||||
// and a direct call to the inner tool behaves identically.
|
||||
"search_name" => FileSearchTool.execute(input, context).await,
|
||||
"search_content" => GrepFilesTool.execute(input, context).await,
|
||||
"write" => WriteFileTool.execute(input, context).await,
|
||||
"edit" => EditFileTool.execute(input, context).await,
|
||||
"patch" => ApplyPatchTool.execute(input, context).await,
|
||||
@@ -474,7 +456,7 @@ mod tests {
|
||||
let schema = tool().input_schema();
|
||||
let properties = schema["properties"].as_object().expect("properties");
|
||||
for (name, property) in properties {
|
||||
let text = if name == "replace" || name == "fuzz" {
|
||||
let text = if name == "replace" {
|
||||
property.to_string()
|
||||
} else {
|
||||
property["description"]
|
||||
@@ -513,21 +495,28 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// `fuzz` is ignored by `edit`. Advertising it as an edit-time fuzzy
|
||||
/// switch is a capability claim the code does not honor.
|
||||
/// `fuzz` belongs to `patch` alone. `edit` read it into a discarded
|
||||
/// binding while the schema kept advertising it, and a live model read
|
||||
/// that as "an optional fuzzy-matching flag for the search" — a
|
||||
/// capability claim no code honored. The advertisement is gone; what
|
||||
/// remains must describe only the integer `patch` really uses.
|
||||
#[test]
|
||||
fn fuzz_does_not_claim_edit_time_fuzzy_matching() {
|
||||
fn fuzz_is_advertised_only_for_patch() {
|
||||
let schema = tool().input_schema();
|
||||
assert_eq!(schema["properties"]["fuzz"]["type"], "integer");
|
||||
let fuzz = schema["properties"]["fuzz"]["description"]
|
||||
.as_str()
|
||||
.expect("fuzz description");
|
||||
assert!(fuzz.contains("action=patch"), "{fuzz}");
|
||||
assert!(
|
||||
fuzz.contains("Deprecated") || fuzz.contains("ignored"),
|
||||
"must say the edit-side flag is inert: {fuzz}"
|
||||
!fuzz.contains("action=edit"),
|
||||
"edit no longer accepts fuzz: {fuzz}"
|
||||
);
|
||||
assert!(
|
||||
fuzz.contains("patch"),
|
||||
"must keep the patch meaning: {fuzz}"
|
||||
EditFileTool.input_schema()["properties"]
|
||||
.get("fuzz")
|
||||
.is_none(),
|
||||
"edit must not advertise a parameter it does not implement"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -557,6 +546,313 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
// === Per-action parameter validation ===
|
||||
//
|
||||
// #5209 taught `edit` to refuse a parameter it does not implement. Only
|
||||
// `edit` learned it, so every other action still dropped unknown keys and
|
||||
// answered anyway: a misspelled `start_line` on `read` returned the head
|
||||
// of the file with nothing in the response admitting the requested window
|
||||
// was never honored. These cover the refusal on every action, and — the
|
||||
// half that keeps a refusal honest — that each action still accepts its
|
||||
// full legitimate parameter set, optional names and aliases included.
|
||||
|
||||
/// A workspace with one file, already read, so `edit` and `patch` are
|
||||
/// past their freshness precondition and reach parameter validation.
|
||||
async fn workspace() -> (tempfile::TempDir, ToolContext) {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
let ctx = ToolContext::new(tmp.path().to_path_buf());
|
||||
std::fs::write(tmp.path().join("doc.txt"), "alpha\nbeta\ngamma\n").expect("write");
|
||||
tool()
|
||||
.execute(json!({"action": "read", "path": "doc.txt"}), &ctx)
|
||||
.await
|
||||
.expect("seed read");
|
||||
(tmp, ctx)
|
||||
}
|
||||
|
||||
/// The minimal call that dispatches for each action, in schema order.
|
||||
fn minimal_calls() -> Vec<(&'static str, Value)> {
|
||||
vec![
|
||||
("read", json!({"action": "read", "path": "doc.txt"})),
|
||||
("list", json!({"action": "list"})),
|
||||
(
|
||||
"search_name",
|
||||
json!({"action": "search_name", "query": "doc"}),
|
||||
),
|
||||
(
|
||||
"search_content",
|
||||
json!({"action": "search_content", "pattern": "alpha"}),
|
||||
),
|
||||
(
|
||||
"write",
|
||||
json!({"action": "write", "path": "new.txt", "content": "x\n"}),
|
||||
),
|
||||
(
|
||||
"edit",
|
||||
json!({"action": "edit", "path": "doc.txt", "search": "alpha", "replace": "delta"}),
|
||||
),
|
||||
(
|
||||
"patch",
|
||||
json!({"action": "patch", "path": "doc.txt", "patch": "@@ -1,1 +1,1 @@\n-alpha\n+delta\n"}),
|
||||
),
|
||||
]
|
||||
}
|
||||
|
||||
fn with_key(mut input: Value, key: &str, value: Value) -> Value {
|
||||
input
|
||||
.as_object_mut()
|
||||
.expect("object")
|
||||
.insert(key.to_string(), value);
|
||||
input
|
||||
}
|
||||
|
||||
/// The gap this closes. A parameter with no known meaning is refused by
|
||||
/// every action, not just `edit`, and the refusal carries the same four
|
||||
/// facts everywhere: what was wrong, what is allowed, what is required,
|
||||
/// and that nothing was done.
|
||||
#[tokio::test]
|
||||
async fn every_action_refuses_an_unknown_parameter() {
|
||||
for (action, call) in minimal_calls() {
|
||||
let (_tmp, ctx) = workspace().await;
|
||||
let message = tool()
|
||||
.execute(with_key(call, "bogus_param", json!(true)), &ctx)
|
||||
.await
|
||||
.expect_err("an unknown parameter must be refused")
|
||||
.to_string();
|
||||
assert!(
|
||||
message.contains("bogus_param"),
|
||||
"{action} must name the offending parameter: {message}"
|
||||
);
|
||||
assert!(
|
||||
message.contains(&format!("unexpected File {action} parameter")),
|
||||
"{action} must name the action it refused: {message}"
|
||||
);
|
||||
assert!(
|
||||
message.contains("Allowed parameters are"),
|
||||
"{action} must name the allowed set: {message}"
|
||||
);
|
||||
assert!(
|
||||
message.contains("Required:"),
|
||||
"{action} must name the required set: {message}"
|
||||
);
|
||||
assert!(
|
||||
message.contains(&format!("The {action} was not performed")),
|
||||
"{action} must deny having done the work: {message}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// The specific silent wrong answer that motivated this: a misspelled
|
||||
/// read window used to be dropped, and the head of the file came back
|
||||
/// under a success receipt as if it were the requested range.
|
||||
#[tokio::test]
|
||||
async fn a_misspelled_read_window_is_refused_rather_than_answered_with_the_head() {
|
||||
let (_tmp, ctx) = workspace().await;
|
||||
let message = tool()
|
||||
.execute(
|
||||
json!({"action": "read", "path": "doc.txt", "start_lien": 2}),
|
||||
&ctx,
|
||||
)
|
||||
.await
|
||||
.expect_err("a misspelled window must not silently return the head")
|
||||
.to_string();
|
||||
assert!(message.contains("start_lien"), "{message}");
|
||||
assert!(message.contains("`start_line`"), "{message}");
|
||||
}
|
||||
|
||||
/// A refusal is only worth having if the legitimate call still lands.
|
||||
/// Every action's full parameter set — every optional name included —
|
||||
/// must survive validation.
|
||||
#[tokio::test]
|
||||
async fn every_action_accepts_its_full_legitimate_parameter_set() {
|
||||
let full: Vec<(&str, Value)> = vec![
|
||||
(
|
||||
"read",
|
||||
json!({"action": "read", "path": "doc.txt", "start_line": 1, "max_lines": 2, "pages": "1"}),
|
||||
),
|
||||
("list", json!({"action": "list", "path": "."})),
|
||||
(
|
||||
"search_name",
|
||||
json!({"action": "search_name", "query": "doc", "path": ".", "limit": 5,
|
||||
"extensions": ["txt"], "exclude": ["target/**"]}),
|
||||
),
|
||||
(
|
||||
"search_content",
|
||||
json!({"action": "search_content", "pattern": "alpha", "path": ".",
|
||||
"include": ["*.txt"], "exclude": ["target/**"], "context_lines": 1,
|
||||
"case_insensitive": true, "max_results": 5}),
|
||||
),
|
||||
(
|
||||
"write",
|
||||
json!({"action": "write", "path": "new.txt", "content": "x\n"}),
|
||||
),
|
||||
(
|
||||
"edit",
|
||||
json!({"action": "edit", "path": "doc.txt", "search": "alpha", "replace": "delta"}),
|
||||
),
|
||||
(
|
||||
"patch",
|
||||
json!({"action": "patch", "path": "doc.txt",
|
||||
"patch": "@@ -1,1 +1,1 @@\n-alpha\n+delta\n",
|
||||
"fuzz": 3, "create_if_missing": false}),
|
||||
),
|
||||
];
|
||||
|
||||
for (action, call) in full {
|
||||
let (_tmp, ctx) = workspace().await;
|
||||
let result = tool()
|
||||
.execute(call, &ctx)
|
||||
.await
|
||||
.unwrap_or_else(|error| panic!("{action} must accept its own parameters: {error}"));
|
||||
assert!(result.success, "{action}: {}", result.content);
|
||||
}
|
||||
}
|
||||
|
||||
/// Validation runs *after* alias translation, so every cross-harness
|
||||
/// spelling the alias lane folds must still reach the action it names.
|
||||
/// A refusal that fired first would undo #5209's fix.
|
||||
#[tokio::test]
|
||||
async fn every_alias_survives_validation() {
|
||||
let aliased: Vec<(&str, Value)> = vec![
|
||||
// Path spellings, on every action that takes a path.
|
||||
("read", json!({"action": "read", "file_path": "doc.txt"})),
|
||||
("read", json!({"action": "read", "filePath": "doc.txt"})),
|
||||
("list", json!({"action": "list", "file_path": "."})),
|
||||
(
|
||||
"search_name",
|
||||
json!({"action": "search_name", "query": "doc", "file_path": "."}),
|
||||
),
|
||||
(
|
||||
"search_content",
|
||||
json!({"action": "search_content", "pattern": "alpha", "file_path": "."}),
|
||||
),
|
||||
(
|
||||
"write",
|
||||
json!({"action": "write", "file_path": "new.txt", "content": "x\n"}),
|
||||
),
|
||||
// Read-window spellings.
|
||||
(
|
||||
"read",
|
||||
json!({"action": "read", "path": "doc.txt", "offset": 2, "limit": 1}),
|
||||
),
|
||||
(
|
||||
"read",
|
||||
json!({"action": "read", "path": "doc.txt", "line_offset": 2, "n_lines": 1}),
|
||||
),
|
||||
(
|
||||
"read",
|
||||
json!({"action": "read", "path": "doc.txt", "num_lines": 1}),
|
||||
),
|
||||
// Search spellings the wrapper advertises across both actions.
|
||||
(
|
||||
"search_name",
|
||||
json!({"action": "search_name", "query": "doc", "max_results": 5}),
|
||||
),
|
||||
(
|
||||
"search_content",
|
||||
json!({"action": "search_content", "query": "alpha", "limit": 5}),
|
||||
),
|
||||
];
|
||||
|
||||
for (action, call) in aliased {
|
||||
let (_tmp, ctx) = workspace().await;
|
||||
let result = tool()
|
||||
.execute(call.clone(), &ctx)
|
||||
.await
|
||||
.unwrap_or_else(|error| panic!("{action} must accept {call}: {error}"));
|
||||
assert!(result.success, "{action} / {call}: {}", result.content);
|
||||
}
|
||||
|
||||
// Edit spellings need their own loop: each one mutates the file.
|
||||
for (search, replace) in [
|
||||
("old_string", "new_string"),
|
||||
("old_str", "new_str"),
|
||||
("oldText", "newText"),
|
||||
("old_text", "new_text"),
|
||||
] {
|
||||
let (_tmp, ctx) = workspace().await;
|
||||
let result = tool()
|
||||
.execute(
|
||||
json!({"action": "edit", "path": "doc.txt",
|
||||
search: "alpha", replace: "delta"}),
|
||||
&ctx,
|
||||
)
|
||||
.await
|
||||
.unwrap_or_else(|error| panic!("edit must accept {search}/{replace}: {error}"));
|
||||
assert!(result.success, "{search}/{replace}: {}", result.content);
|
||||
}
|
||||
let (_tmp, ctx) = workspace().await;
|
||||
let result = tool()
|
||||
.execute(
|
||||
json!({"action": "edit", "path": "doc.txt", "search": "alpha", "replacement": "delta"}),
|
||||
&ctx,
|
||||
)
|
||||
.await
|
||||
.expect("edit must accept `replacement`");
|
||||
assert!(result.success, "{}", result.content);
|
||||
}
|
||||
|
||||
/// A parameter that belongs to a *different* action is still unknown to
|
||||
/// this one. Silently dropping it is how a model learns a call worked
|
||||
/// when the argument it cared about was discarded.
|
||||
#[tokio::test]
|
||||
async fn parameters_do_not_leak_between_actions() {
|
||||
for (action, call) in [
|
||||
(
|
||||
"read",
|
||||
json!({"action": "read", "path": "doc.txt", "case_insensitive": true}),
|
||||
),
|
||||
(
|
||||
"write",
|
||||
json!({"action": "write", "path": "new.txt", "content": "x\n", "start_line": 2}),
|
||||
),
|
||||
(
|
||||
"list",
|
||||
json!({"action": "list", "path": ".", "context_lines": 3}),
|
||||
),
|
||||
(
|
||||
"search_name",
|
||||
json!({"action": "search_name", "query": "doc", "context_lines": 3}),
|
||||
),
|
||||
(
|
||||
"search_content",
|
||||
json!({"action": "search_content", "pattern": "alpha", "extensions": ["txt"]}),
|
||||
),
|
||||
] {
|
||||
let (_tmp, ctx) = workspace().await;
|
||||
let message = tool()
|
||||
.execute(call, &ctx)
|
||||
.await
|
||||
.expect_err("another action's parameter must be refused")
|
||||
.to_string();
|
||||
assert!(
|
||||
message.contains(&format!("The {action} was not performed")),
|
||||
"{action}: {message}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Every action's required names must be names it also allows, or a
|
||||
/// refusal would tell the model to pass something the action rejects.
|
||||
#[test]
|
||||
fn every_required_parameter_is_also_an_allowed_one() {
|
||||
use crate::tools::file::{
|
||||
EDIT_PARAMS, LIST_PARAMS, PATCH_PARAMS, READ_PARAMS, SEARCH_CONTENT_PARAMS,
|
||||
SEARCH_NAME_PARAMS, WRITE_PARAMS,
|
||||
};
|
||||
|
||||
for params in [
|
||||
READ_PARAMS,
|
||||
WRITE_PARAMS,
|
||||
EDIT_PARAMS,
|
||||
LIST_PARAMS,
|
||||
SEARCH_NAME_PARAMS,
|
||||
SEARCH_CONTENT_PARAMS,
|
||||
PATCH_PARAMS,
|
||||
] {
|
||||
params.assert_required_is_allowed();
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn advertised_actions_match_the_actions_that_dispatch() {
|
||||
for (tool, expected) in [
|
||||
|
||||
@@ -3,6 +3,9 @@
|
||||
//! These tools provide powerful code search capabilities within the workspace,
|
||||
//! similar to ripgrep/grep functionality.
|
||||
|
||||
use super::file::{
|
||||
PATH_ALIASES, SEARCH_CONTENT_ALIASES, SEARCH_CONTENT_PARAMS, apply_param_aliases,
|
||||
};
|
||||
use super::spec::{
|
||||
ToolCapability, ToolContext, ToolError, ToolResult, ToolSpec, optional_bool, optional_str,
|
||||
optional_u64, required_str,
|
||||
@@ -104,6 +107,11 @@ impl ToolSpec for GrepFilesTool {
|
||||
}
|
||||
|
||||
async fn execute(&self, input: Value, context: &ToolContext) -> Result<ToolResult, ToolError> {
|
||||
let mut input = input;
|
||||
apply_param_aliases(&mut input, PATH_ALIASES, "File search_content")?;
|
||||
apply_param_aliases(&mut input, SEARCH_CONTENT_ALIASES, "File search_content")?;
|
||||
SEARCH_CONTENT_PARAMS.reject_unknown(&input)?;
|
||||
|
||||
let pattern_str = required_str(&input, "pattern")?;
|
||||
let path_str = optional_str(&input, "path")?.unwrap_or(".");
|
||||
let context_lines = usize::try_from(optional_u64(&input, "context_lines", 2)?)
|
||||
|
||||
Reference in New Issue
Block a user