diff --git a/crates/tinytools-std/src/filesystem/apply_patch/mod.rs b/crates/tinytools-std/src/filesystem/apply_patch/mod.rs index 2e2f0fb..5311d14 100644 --- a/crates/tinytools-std/src/filesystem/apply_patch/mod.rs +++ b/crates/tinytools-std/src/filesystem/apply_patch/mod.rs @@ -50,7 +50,8 @@ impl Tool for ApplyPatchTool { fn description(&self) -> &'static str { "Apply a batch of exact-string edits across one or more files atomically. \ All edits are validated before any are written; validation failure rolls \ - back the whole batch. Each edit is `{path, old_string, new_string, replace_all?}`. \ + back the whole batch. Each edit is `{path, old_string, new_string, replace_all?}`; \ + a top-level `path` is the default for edits that omit their own. \ To CREATE a new file, pass an empty `old_string` with the full contents \ as `new_string`; the path must not already exist." } @@ -59,13 +60,20 @@ impl Tool for ApplyPatchTool { json!({ "type": "object", "properties": { + "path": { + "type": "string", + "description": "Default file for edits that omit their own `path`." + }, "edits": { "type": "array", "description": "Ordered list of edits.", "items": { "type": "object", "properties": { - "path": { "type": "string" }, + "path": { + "type": "string", + "description": "File to edit; defaults to the top-level `path`." + }, "old_string": { "type": "string", "description": "Exact text to replace. Empty means CREATE: the path must not exist and `new_string` becomes the whole file." @@ -73,7 +81,7 @@ impl Tool for ApplyPatchTool { "new_string": { "type": "string" }, "replace_all": { "type": "boolean", "default": false } }, - "required": ["path", "old_string", "new_string"] + "required": ["old_string", "new_string"] } } }, @@ -144,13 +152,33 @@ impl ApplyPatchTool { let path_policy = gate_for_context(&self.gate, context, "apply_patch"); + // "One file, several edits" is a natural call shape, and models write + // it with the path once at the top level (4 of 10 calls in one run; + // the per-edit `path` requirement rejected every one of them and the + // run halted on the fourth). The top-level path is the default; an + // edit's own path still wins. + let default_path = match args.get("path") { + Some(value) => Some( + value + .as_str() + .ok_or_else(|| anyhow::anyhow!("top-level `path` must be a string"))?, + ), + None => None, + }; + // Parse + group edits by file. let mut parsed: Vec = Vec::with_capacity(edits.len()); for (i, raw) in edits.iter().enumerate() { - let path = raw - .get("path") - .and_then(|v| v.as_str()) - .ok_or_else(|| anyhow::anyhow!("edit[{i}]: missing `path`"))?; + let path = match raw.get("path") { + Some(value) => value.as_str().ok_or_else(|| { + anyhow::anyhow!("edit[{i}]: `path` must be a string") + })?, + None => default_path.ok_or_else(|| { + anyhow::anyhow!( + "edit[{i}]: missing `path` (give each edit a `path`, or one top-level `path` for all edits)" + ) + })?, + }; let old_string = raw .get("old_string") .and_then(|v| v.as_str()) diff --git a/crates/tinytools-std/src/filesystem/apply_patch/mod_tests.rs b/crates/tinytools-std/src/filesystem/apply_patch/mod_tests.rs index 391daa0..5cb7c9e 100644 --- a/crates/tinytools-std/src/filesystem/apply_patch/mod_tests.rs +++ b/crates/tinytools-std/src/filesystem/apply_patch/mod_tests.rs @@ -157,6 +157,148 @@ async fn apply_patch_creates_a_new_file_from_an_empty_old_string() { assert!(result.output().contains("created"), "{}", result.output()); } +#[tokio::test] +async fn apply_patch_takes_a_top_level_path_as_the_default_for_every_edit() { + // The shape a model writes for "one file, several edits": the path once, + // at the top level. Each such call used to be rejected for a missing + // `edits[0].path`, and four in a row halted a run with 26 minutes left. + let dir = std::env::temp_dir().join("openhuman_test_patch_top_level_path"); + let _ = tokio::fs::remove_dir_all(&dir).await; + tokio::fs::create_dir_all(&dir).await.unwrap(); + tokio::fs::write(dir.join("a.c"), "int x = 1;\nint y = 2;\n") + .await + .unwrap(); + + let tool = ApplyPatchTool::new(test_security(dir.clone())); + let result = tool + .execute(json!({ + "path": "a.c", + "edits": [ + { "old_string": "int x = 1;", "new_string": "int x = 10;" }, + { "old_string": "int y = 2;", "new_string": "int y = 20;" } + ] + })) + .await + .unwrap(); + assert!(!result.is_error, "{}", result.output()); + assert_eq!( + tokio::fs::read_to_string(dir.join("a.c")).await.unwrap(), + "int x = 10;\nint y = 20;\n" + ); +} + +#[tokio::test] +async fn apply_patch_rejects_malformed_edit_path_instead_of_using_default() { + let dir = std::env::temp_dir().join("openhuman_test_patch_malformed_path"); + let _ = tokio::fs::remove_dir_all(&dir).await; + tokio::fs::create_dir_all(&dir).await.unwrap(); + tokio::fs::write(dir.join("default.txt"), "original") + .await + .unwrap(); + + let tool = ApplyPatchTool::new(test_security(dir.clone())); + let result = tool + .execute(json!({ + "path": "default.txt", + "edits": [{ "path": null, "old_string": "original", "new_string": "changed" }] + })) + .await; + + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("`path` must be a string") + ); + assert_eq!( + tokio::fs::read_to_string(dir.join("default.txt")) + .await + .unwrap(), + "original" + ); + let _ = tokio::fs::remove_dir_all(&dir).await; +} + +#[tokio::test] +async fn apply_patch_rejects_a_malformed_top_level_path() { + let dir = std::env::temp_dir().join("openhuman_test_patch_malformed_top_level_path"); + let _ = tokio::fs::remove_dir_all(&dir).await; + tokio::fs::create_dir_all(&dir).await.unwrap(); + tokio::fs::write(dir.join("a.txt"), "original") + .await + .unwrap(); + + let tool = ApplyPatchTool::new(test_security(dir.clone())); + let result = tool + .execute(json!({ + "path": 123, + "edits": [{ "path": "a.txt", "old_string": "original", "new_string": "changed" }] + })) + .await; + + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("top-level `path` must be a string") + ); + assert_eq!( + tokio::fs::read_to_string(dir.join("a.txt")).await.unwrap(), + "original" + ); + let _ = tokio::fs::remove_dir_all(&dir).await; +} + +#[tokio::test] +async fn apply_patch_lets_an_edits_own_path_win_over_the_top_level_one() { + let dir = std::env::temp_dir().join("openhuman_test_patch_top_level_path_override"); + let _ = tokio::fs::remove_dir_all(&dir).await; + tokio::fs::create_dir_all(&dir).await.unwrap(); + tokio::fs::write(dir.join("a.txt"), "alpha").await.unwrap(); + tokio::fs::write(dir.join("b.txt"), "bravo").await.unwrap(); + + let tool = ApplyPatchTool::new(test_security(dir.clone())); + let result = tool + .execute(json!({ + "path": "a.txt", + "edits": [ + { "old_string": "alpha", "new_string": "ALPHA" }, + { "path": "b.txt", "old_string": "bravo", "new_string": "BRAVO" } + ] + })) + .await + .unwrap(); + assert!(!result.is_error, "{}", result.output()); + assert_eq!( + tokio::fs::read_to_string(dir.join("a.txt")).await.unwrap(), + "ALPHA" + ); + assert_eq!( + tokio::fs::read_to_string(dir.join("b.txt")).await.unwrap(), + "BRAVO" + ); +} + +#[tokio::test] +async fn apply_patch_names_both_ways_to_give_a_path_when_neither_is_given() { + let dir = std::env::temp_dir().join("openhuman_test_patch_no_path_anywhere"); + let _ = tokio::fs::remove_dir_all(&dir).await; + tokio::fs::create_dir_all(&dir).await.unwrap(); + + let tool = ApplyPatchTool::new(test_security(dir.clone())); + let err = tool + .execute(json!({ + "edits": [ { "old_string": "a", "new_string": "b" } ] + })) + .await + .expect_err("no path anywhere is an argument error"); + let text = err.to_string(); + assert!(text.contains("edit[0]: missing `path`"), "{text}"); + assert!(text.contains("one top-level `path`"), "{text}"); +} + #[tokio::test] async fn apply_patch_refuses_an_empty_old_string_on_an_existing_file() { let dir = std::env::temp_dir().join("openhuman_test_patch_create_existing"); diff --git a/crates/tinytools-std/src/filesystem/fixtures/apply_patch.json b/crates/tinytools-std/src/filesystem/fixtures/apply_patch.json index 89b3b67..7b98f41 100644 --- a/crates/tinytools-std/src/filesystem/fixtures/apply_patch.json +++ b/crates/tinytools-std/src/filesystem/fixtures/apply_patch.json @@ -1,10 +1,14 @@ { - "description": "Apply a batch of exact-string edits across one or more files atomically. All edits are validated before any are written; validation failure rolls back the whole batch. Each edit is `{path, old_string, new_string, replace_all?}`. To CREATE a new file, pass an empty `old_string` with the full contents as `new_string`; the path must not already exist.", + "description": "Apply a batch of exact-string edits across one or more files atomically. All edits are validated before any are written; validation failure rolls back the whole batch. Each edit is `{path, old_string, new_string, replace_all?}`; a top-level `path` is the default for edits that omit their own. To CREATE a new file, pass an empty `old_string` with the full contents as `new_string`; the path must not already exist.", "exposure": "Direct", "name": "apply_patch", "permission_level": "Write", "schema": { "properties": { + "path": { + "description": "Default file for edits that omit their own `path`.", + "type": "string" + }, "edits": { "description": "Ordered list of edits.", "items": { @@ -17,6 +21,7 @@ "type": "string" }, "path": { + "description": "File to edit; defaults to the top-level `path`.", "type": "string" }, "replace_all": { @@ -25,7 +30,6 @@ } }, "required": [ - "path", "old_string", "new_string" ],